Skip to content

v2.2.4#102

Closed
byteful wants to merge 17 commits into
mainfrom
dev
Closed

v2.2.4#102
byteful wants to merge 17 commits into
mainfrom
dev

Conversation

@byteful

@byteful byteful commented May 21, 2026

Copy link
Copy Markdown
Member

Confidence Score: 4/5

Safe to merge with the settings fallback fix applied; all other changes are well-scoped refactors or additive features.

The auth handler for panel settings silently swallows non-auth errors and serves a stripped public-only settings shape for up to 5 minutes, meaning panel settings components receive undefined fields with no error surfaced. The realtime layer and proto migration are otherwise solid.

client/src/hooks/data/settings.ts — the outer catch fallback branches (lines 118–144) need to be swapped so a panel-page error falls back to the public endpoint and vice versa.

Important Files Changed

Filename Overview
client/src/hooks/data/settings.ts New settings hook with complex nested fallback logic; outer catch swaps public/panel fallback branches, silently serving public-only settings to panel users on non-auth errors
client/src/hooks/use-realtime.tsx New realtime WebSocket provider with full reconnect/heartbeat/dedup logic; well-structured but close code 1013 permanently stops reconnects and ticket detail cache key mismatch were flagged in prior review
client/src/hooks/data/tickets.ts Migrated tickets data layer to proto-JSON; preserves legacy any contract via mapper functions; query keys are consistent with realtime invalidation targets
client/src/lib/proto-fetch.ts New typed proto-JSON fetch utility; clean abstraction over apiFetch with typed error class and ignoreUnknownFields for forward compatibility
client/src/pages/home.tsx Dashboard now shows alerts section; isLoadingAlerts included in isInitialDashboardLoading which blocks the whole dashboard (flagged in prior review)
client/src/App.tsx Adds /rate-limit to routing guards and wires RealtimeProvider; /rate-limit still absent from ALWAYS_REACHABLE_PATHS (flagged in prior review)
vite.config.ts Rewrites manual chunk strategy to prevent circular TDZ issues; adds WebSocket proxy for realtime dev; enables hidden sourcemaps

Comments Outside Diff (3)

  1. client/src/pages/HomePage.tsx, line 108-109 (link)

    P1 Unwrap homepage responses

    This page still stores the public knowledgebase response as if it were the category array, but the sibling fixes in this PR now unwrap the same endpoint family from envelope fields like data.categories, data.cards, and data.articles. When the backend returns { categories: [...] }, this sets categories to an object, and the render path later calls categories.map, which crashes the public homepage instead of showing the category cards.

    Rule Used: This is a React frontend project on React 19 with ... (source)

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: client/src/pages/HomePage.tsx
    Line: 108-109
    
    Comment:
    **Unwrap homepage responses**
    
    This page still stores the public knowledgebase response as if it were the category array, but the sibling fixes in this PR now unwrap the same endpoint family from envelope fields like `data.categories`, `data.cards`, and `data.articles`. When the backend returns `{ categories: [...] }`, this sets `categories` to an object, and the render path later calls `categories.map`, which crashes the public homepage instead of showing the category cards.
    
    
    
    **Rule Used:** This is a React frontend project on React 19 with ... ([source](https://app.greptile.com/modl-gg/-/custom-context?memory=b7532101-0c9e-4ab6-b168-353a105ba593))
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. client/src/pages/HomePage.tsx, line 128-129 (link)

    P1 Unwrap homepage cards

    The homepage cards endpoint was migrated elsewhere in this PR to read data.cards, but this public homepage path still stores the full response object. With an envelope response like { cards: [...] }, homepageCards becomes an object and the card rendering path calls .map on it, so the homepage can crash before rendering its configured cards.

    Rule Used: This is a React frontend project on React 19 with ... (source)

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: client/src/pages/HomePage.tsx
    Line: 128-129
    
    Comment:
    **Unwrap homepage cards**
    
    The homepage cards endpoint was migrated elsewhere in this PR to read `data.cards`, but this public homepage path still stores the full response object. With an envelope response like `{ cards: [...] }`, `homepageCards` becomes an object and the card rendering path calls `.map` on it, so the homepage can crash before rendering its configured cards.
    
    
    
    **Rule Used:** This is a React frontend project on React 19 with ... ([source](https://app.greptile.com/modl-gg/-/custom-context?memory=b7532101-0c9e-4ab6-b168-353a105ba593))
    
    How can I resolve this? If you propose a fix, please make it concise.
  3. client/src/pages/HomePage.tsx, line 156-157 (link)

    P1 Unwrap search results

    The matching knowledgebase search page now unwraps search results from data.articles, but this homepage search path still assigns the whole response object. When search returns { articles: [...] }, searchResults is no longer an array and the search-results render path calls .map, which breaks homepage search instead of showing results.

    Rule Used: This is a React frontend project on React 19 with ... (source)

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: client/src/pages/HomePage.tsx
    Line: 156-157
    
    Comment:
    **Unwrap search results**
    
    The matching knowledgebase search page now unwraps search results from `data.articles`, but this homepage search path still assigns the whole response object. When search returns `{ articles: [...] }`, `searchResults` is no longer an array and the search-results render path calls `.map`, which breaks homepage search instead of showing results.
    
    
    
    **Rule Used:** This is a React frontend project on React 19 with ... ([source](https://app.greptile.com/modl-gg/-/custom-context?memory=b7532101-0c9e-4ab6-b168-353a105ba593))
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
client/src/hooks/data/settings.ts:118-144
**Swapped fallback branches silently serve wrong settings shape**

The outer `catch` reverses the intended fallbacks: when `isPublic === false` (a panel user) and the panel endpoint throws a non-auth error (e.g. a 500), the catch calls `fetchPublic()`, which returns only `{ serverDisplayName, panelIconUrl, homepageIconUrl, ticketForms }`. That stripped shape is then cached for 5 minutes (`staleTime: 300000`, `refetchOnWindowFocus: false`), so every panel settings component that expects webhook settings, label settings, or the full `general` envelope silently reads `undefined` fields with no error state surfaced to the user.

The `if (isPublic)` branch (line 120) also tries the authenticated panel endpoint as a fallback when the public endpoint has failed for an unauthenticated visitor — this will always throw a 401, wasting a round-trip before falling through to the hardcoded defaults.

The two branches appear to need swapping: `!isPublic` → try the panel endpoint, else → try the public endpoint.

Reviews (10): Last reviewed commit: "fixes" | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented May 22, 2026

Copy link
Copy Markdown

Deploying modl-panel with  Cloudflare Pages  Cloudflare Pages

Latest commit: c6443c8
Status: ✅  Deploy successful!
Preview URL: https://90949bca.staging-panel.pages.dev
Branch Preview URL: https://dev.staging-panel.pages.dev

View logs

@byteful byteful marked this pull request as ready for review May 22, 2026 04:36
Comment thread client/src/hooks/use-realtime.tsx
Comment thread client/src/hooks/use-realtime.tsx
Comment thread client/src/hooks/use-realtime.tsx
Comment thread client/src/pages/home.tsx
Comment thread client/src/hooks/use-data.tsx Outdated
Comment on lines +1275 to +1278
const res = await apiFetch('/v1/panel/dashboard/alerts');
if (!res.ok) {
throw new Error('Failed to fetch dashboard alerts');
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Alerts errors are unhandled

This throws for every non-OK alerts response, including permission or rollout failures. The dashboard does not read or display the alerts error, so a user whose account cannot access the new endpoint gets no alert section and a manual refresh can fail with only the generic dashboard refresh error.

Prompt To Fix With AI
This is a comment left during a code review.
Path: client/src/hooks/use-data.tsx
Line: 1275-1278

Comment:
**Alerts errors are unhandled**

This throws for every non-OK alerts response, including permission or rollout failures. The dashboard does not read or display the alerts error, so a user whose account cannot access the new endpoint gets no alert section and a manual refresh can fail with only the generic dashboard refresh error.

How can I resolve this? If you propose a fix, please make it concise.

Comment thread client/src/App.tsx

// Recovery/verification flows that must stay reachable even when the tenant is not fully
// provisioned (serverExists===false) or the platform is in maintenance mode.
const ALWAYS_REACHABLE_PATHS = ['/verify-email', '/verify/', '/replay'];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 The /rate-limit page was added to the router but not to ALWAYS_REACHABLE_PATHS. When a user is being rate-limited, the /v1/public/settings request will also fail, so isSettingsError is true and publicSettings is undefined (no prior cache). The guard on the next line then fires and replaces the rate-limit page with "Unable to reach the server" — the exact screen this page is meant to replace.

Suggested change
const ALWAYS_REACHABLE_PATHS = ['/verify-email', '/verify/', '/replay'];
const ALWAYS_REACHABLE_PATHS = ['/verify-email', '/verify/', '/replay', '/rate-limit'];
Prompt To Fix With AI
This is a comment left during a code review.
Path: client/src/App.tsx
Line: 194

Comment:
The `/rate-limit` page was added to the router but not to `ALWAYS_REACHABLE_PATHS`. When a user is being rate-limited, the `/v1/public/settings` request will also fail, so `isSettingsError` is `true` and `publicSettings` is `undefined` (no prior cache). The guard on the next line then fires and replaces the rate-limit page with "Unable to reach the server" — the exact screen this page is meant to replace.

```suggestion
const ALWAYS_REACHABLE_PATHS = ['/verify-email', '/verify/', '/replay', '/rate-limit'];
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +118 to +144
} catch (error) {
try {
if (isPublic) {
const decoded = await protoFetch(GeneralSettingsEnvelopeSchema, '/v1/panel/settings/general');
const general = decoded.data || {};
return {
settings: {
...general,
general,
generalMeta: metaToShape(decoded.meta),
}
};
}
return await fetchPublic();
} catch {
return {
settings: {
general: {
serverDisplayName: 'modl',
panelIconUrl: null,
homepageIconUrl: null
},
ticketForms: {}
}
};
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Swapped fallback branches silently serve wrong settings shape

The outer catch reverses the intended fallbacks: when isPublic === false (a panel user) and the panel endpoint throws a non-auth error (e.g. a 500), the catch calls fetchPublic(), which returns only { serverDisplayName, panelIconUrl, homepageIconUrl, ticketForms }. That stripped shape is then cached for 5 minutes (staleTime: 300000, refetchOnWindowFocus: false), so every panel settings component that expects webhook settings, label settings, or the full general envelope silently reads undefined fields with no error state surfaced to the user.

The if (isPublic) branch (line 120) also tries the authenticated panel endpoint as a fallback when the public endpoint has failed for an unauthenticated visitor — this will always throw a 401, wasting a round-trip before falling through to the hardcoded defaults.

The two branches appear to need swapping: !isPublic → try the panel endpoint, else → try the public endpoint.

Rule Used: This is a React frontend project on React 19 with ... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: client/src/hooks/data/settings.ts
Line: 118-144

Comment:
**Swapped fallback branches silently serve wrong settings shape**

The outer `catch` reverses the intended fallbacks: when `isPublic === false` (a panel user) and the panel endpoint throws a non-auth error (e.g. a 500), the catch calls `fetchPublic()`, which returns only `{ serverDisplayName, panelIconUrl, homepageIconUrl, ticketForms }`. That stripped shape is then cached for 5 minutes (`staleTime: 300000`, `refetchOnWindowFocus: false`), so every panel settings component that expects webhook settings, label settings, or the full `general` envelope silently reads `undefined` fields with no error state surfaced to the user.

The `if (isPublic)` branch (line 120) also tries the authenticated panel endpoint as a fallback when the public endpoint has failed for an unauthenticated visitor — this will always throw a 401, wasting a round-trip before falling through to the hardcoded defaults.

The two branches appear to need swapping: `!isPublic` → try the panel endpoint, else → try the public endpoint.

**Rule Used:** This is a React frontend project on React 19 with ... ([source](https://app.greptile.com/modl-gg/-/custom-context?memory=b7532101-0c9e-4ab6-b168-353a105ba593))

How can I resolve this? If you propose a fix, please make it concise.

@byteful byteful closed this Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant