rebase

[leonarduk]

No description provided.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[github-actions]

GPT AI Code Review

1. Acceptance criteria

There are no specific acceptance criteria provided in the linked issue, so the review must focus on the code changes themselves. The changes primarily introduce a new feature toggle (familyMvpEnabled) that alters the behavior of various components based on whether the Family MVP mode is enabled or not. The changes appear to be well-scoped and consistent throughout the codebase, ensuring that the new feature does not disrupt existing functionality.

2. Bugs and logic errors

The logic for handling the familyMvpEnabled flag seems sound, with appropriate checks in place to conditionally render components and alter behavior. However, there is a typo in the import statement in PortfolioView.tsx where familyMvpEnabledy is used instead of familyMvpEnabled (line 219). This will lead to a runtime error as familyMvpEnabledy is undefined. This needs to be corrected.

3. API, data, and workflow safety

The changes maintain the alignment between the backend and frontend contracts, as the new familyMvpEnabled flag is consistently integrated into the relevant components. There are no indications that this will break local smoke tests or deployment workflows. The CI tests have been updated to include cases for the new feature toggle, which is a good practice.

4. Test coverage

The test coverage appears to be adequate, with new tests added to familyMvpRedirect.test.ts to cover the behavior of the getFamilyMvpRedirectPath function based on the familyMvpEnabled flag. However, it would be beneficial to see more unit tests that cover the UI components affected by this flag, particularly in HoldingsTable.tsx, PortfolioView.tsx, and Menu.tsx, to ensure that all conditional rendering paths are tested.

5. Minor issues (optional)

• The typo in PortfolioView.tsx (line 219) should be corrected from familyMvpEnabledy to familyMvpEnabled.
• The comment about excluding the transactions tab in ConfigProvider has been removed, but it may be worth noting that this could lead to confusion if the feature is intended to exclude the transactions tab in the future.

Summary Verdict: REQUEST CHANGES (due to the typo and the need for additional tests for UI components).

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

Claude AI Code Review

PR Review: "rebase"

1. Acceptance criteria

No linked issue. The PR appears to be threading familyMvpEnabled as a runtime config flag through the UI to gate features, and removing some hard-coded Family MVP mode restrictions. I'll assess on its own merits.

---

2. Bugs and Logic Errors

Critical: Typo causes runtime error in PortfolioView.tsx

// PortfolioView.tsx, line 219
const { baseCurrency, familyMvpEnabledy, enableAdvancedAnalytics = true } = useConfig();

familyMvpEnabledy is a typo — it will destructure as undefined, meaning !familyMvpEnabled will always be true (since undefined is falsy), so all the !familyMvpEnabled guards in this component will be inverted. The date picker, export buttons, and sector contribution chart will always render; the "family MVP" path will never hide them. This is a silent bug, not a crash, which makes it worse.

Critical: Broken useEffect in App.tsx — unclosed JSX/JS block

// App.tsx, around line 260
useEffect(() => {
  if (getFamilyMvpRedirectPath(location.pathname, location.search, familyMvpEnabled)) {
    return;
}, [location.pathname, location.search, navigate, familyMvpEntryPath]);

This useEffect has an if block that is never closed (return; with no closing }), and the effect body itself has no closing }. This is almost certainly a merge/rebase artifact that would fail to compile. The intent is unclear — was this meant to be a guard in the existing effect, or a new effect that was half-written and half-deleted?

Critical: Unclosed JSX fragment in PortfolioView.tsx

// PortfolioView.tsx, around line 419
            ) : (
              <p className="text-sm text-gray-400">No sector data available.</p>
            )}
          </div>
        )}
          {enableAdvancedAnalytics && (

The {!familyMvpEnabled && ( block that wraps the sector contribution section is never closed before {enableAdvancedAnalytics && .... This is a JSX syntax error that would fail to compile.

Syntax error in test file

// familyMvpRedirect.test.ts, around line 56
it('redirects group query routes to transactions because group is non-MVP', () => {
  expect(getFamilyMvpRedirectPath('/', '?group=kids', true)).toBe('/transactions');
  it('redirects non-MVP routes to the configured entry path', () => {    // ← nested it(), never closed

A new it() block is nested inside another it() without closing the outer one. This file will not parse. This looks like a rebase conflict that ate the closing }); lines.

getFamilyMvpRedirectPath signature change is a breaking API change with no default

// App.tsx
export function getFamilyMvpRedirectPath(
  pathname: string,
  search: string,
  familyMvpEnabled: boolean   // ← new required parameter, no default
  entryPath: string | null = '/portfolio'
): string | null {

The new familyMvpEnabled parameter is inserted before the defaulted entryPath parameter with no default value of its own. Any existing call sites that pass only (pathname, search) will now receive a type error. The test file calls it as getFamilyMvpRedirectPath('/market', '', false) — passing false as familyMvpEnabled and relying on the entryPath default — which is correct, but the old tests at the bottom of the file call it as getFamilyMvpRedirectPath('/market', '') with only two args, which is now a type error.

Missing comma in function call (App.tsx)

const redirectPath = getFamilyMvpRedirectPath(
  location.pathname,
  location.search,
  familyMvpEnabled   // ← no trailing comma before next arg
  familyMvpEntryPath
);

This is a syntax error — missing comma between familyMvpEnabled and familyMvpEntryPath.

familyMvpEnabled default value semantics change

// ConfigContext.tsx
familyMvpEnabled: true,  // in defaultContext and fallback state

Previously the flag didn't exist (implicitly false/absent). Now the default is true, meaning before config loads, the app will enforce MVP mode. This could cause a flash of restricted UI during startup. Consider defaulting to false until config is confirmed loaded, gated on configLoaded.

transactions tab now on by default, then conditionally disabled

// ConfigContext.tsx line 78
transactions: true,   // changed from false

The old code set it false and the Family MVP logic called disableTab("transactions"). The new code sets it true and removes the disableTab call. This is probably intentional but the net effect needs verification — if transactions: true in defaultTabs and the disableTab call is removed, transactions will now show for non-MVP users who didn't have it before. Confirm this is desired.

performance removed from FAMILY_MVP_MODES

// familyMvp.ts
export const FAMILY_MVP_MODES: ReadonlySet<Mode> = new Set<Mode>([
  'transactions',
  'owner',
  // 'performance' removed
]);

New tests assert /performance/alex redirects away (correct given this change), but existing integration tests that expect performance to be visible in MVP mode will now fail. No evidence those tests were updated.

---

3. API, Data, and Workflow Safety

• The backend contract is not touched — this is purely frontend. No payload shape concerns.
• The multiple syntax errors mean this PR will not compile. CI will fail on tsc and the test runner. This is not deployable.
• No secrets or permissions concerns.

---

4. Test Coverage

The new tests in familyMvpRedirect.test.ts have the right intent but are syntactically broken (nested it(), missing closing braces). The Menu.test.tsx updates correctly wrap tests in a configContext.Provider with familyMvpEnabled: false — that's the right approach.

Missing coverage:

• PortfolioView with familyMvpEnabled: true vs false (the typo means these can't be tested meaningfully anyway).
• The transactions tab default change has no regression test.
• No test for the familyMvpEnabled: true default causing pre-load MVP enforcement.

---

5. Minor Issues

• The useEffect dependency arrays were split: one now depends on [familyMvpEnabled, location.pathname, location.search, navigate] and the other on [location.pathname, location.search, navigate, familyMvpEntryPath]. The intent of the split is unclear from the diff, and the second effect body is broken.
• isFamilyMvpMode in Menu.tsx is being replaced by !familyMvpEnabled || isFamilyMvpMode(entry.mode). The logic inversion is correct in intent but worth a careful read — when familyMvpEnabled is false, all modes are allowed; when true, only MVP modes are allowed. That reads correctly.

---

Verdict

REQUEST CHANGES — The PR has at least five separate syntax/compilation errors (missing comma in function call, unclosed useEffect body, unclosed JSX fragment, nested it() in tests, typo familyMvpEnabledy) that guarantee CI failure and broken runtime behavior. This looks like an incomplete rebase that was pushed before conflicts were fully resolved.

---

Reviewed by Claude via claude-pr-review.yml. Advisory only.