From ef722b08d6e7d0f18665a50254529579041f5602 Mon Sep 17 00:00:00 2001 From: Dmitrii Kataraev Date: Fri, 24 Jul 2026 13:28:18 -0700 Subject: [PATCH 1/4] fix(shell-ui): surface dashboard fetch failures instead of loading forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _fetchDashboard() caught every error and only console.log'd it, leaving _data null. Consumers render their loading state whenever data is null, so a failed fetch was indistinguishable from a slow one and the view sat on "Loading dashboard data..." indefinitely. That is what the "Dashboard hangs on Loading" report was (rocketride-saas #373): the request was returning Permission 'task.monitor' denied and the UI simply never said so. Track the failure as first-class state, clear it on success, phrase permission denials in plain language, and expose it as `error` on the hook so views can render it. Adding a field is backward compatible — the existing consumer destructures a subset and is unaffected. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP --- apps/shell-ui/src/hooks/useDashboardData.ts | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/apps/shell-ui/src/hooks/useDashboardData.ts b/apps/shell-ui/src/hooks/useDashboardData.ts index 19ee757e4..34318cdbe 100644 --- a/apps/shell-ui/src/hooks/useDashboardData.ts +++ b/apps/shell-ui/src/hooks/useDashboardData.ts @@ -54,6 +54,7 @@ const MAX_EVENTS = 200; let _data: DashboardResponse | null = null; let _events: ActivityEvent[] = []; +let _error: string | null = null; let _refCount = 0; let _intervalId: ReturnType | null = null; let _eventUnsub: (() => void) | null = null; @@ -76,6 +77,8 @@ interface DashboardSnapshot { data: DashboardResponse | null; /** Activity events (newest first). */ events: ActivityEvent[]; + /** Last fetch failure, or null when healthy. */ + error: string | null; } // Explicitly annotated so control-flow analysis does not narrow the initial @@ -83,11 +86,11 @@ interface DashboardSnapshot { // defined further down); without the annotation, the reassignment in `_emit` // below would not type-check. Runtime behavior is unchanged. /** Stable snapshot object — only replaced when data or events change. */ -let _snapshot: DashboardSnapshot = { data: _data, events: _events }; +let _snapshot: DashboardSnapshot = { data: _data, events: _events, error: _error }; /** Notify all subscribed React components that data changed. */ function _emit(): void { - _snapshot = { data: _data, events: _events }; + _snapshot = { data: _data, events: _events, error: _error }; _listeners.forEach(fn => fn()); } @@ -111,10 +114,20 @@ function _fetchDashboard(): Promise { const dashboard = await client.getDashboard(); if (dashboard?.overview) { _data = dashboard; + _error = null; _emit(); } } catch (err) { + // Surface the failure rather than only logging it. Swallowing it left + // `_data` null, and consumers render their loading state whenever data + // is null — so a permission denial was indistinguishable from a slow + // load and the view sat on "Loading..." forever (saas #373). console.log('[useDashboardData] Dashboard fetch failed:', err); + const message = err instanceof Error ? err.message : String(err); + _error = /denied|permission|forbidden|403/i.test(message) + ? `You do not have permission to view the dashboard. ${message}` + : message; + _emit(); } finally { _fetchPromise = null; } @@ -182,6 +195,8 @@ export interface DashboardData { data: DashboardResponse | null; /** Activity events (newest first). */ events: ActivityEvent[]; + /** Last fetch failure (e.g. a permission denial), or null when healthy. */ + error: string | null; /** Trigger a manual refresh. */ refresh: () => void; } @@ -237,6 +252,7 @@ export function useDashboardData(): DashboardData { return { data: snapshot.data, events: snapshot.events, + error: snapshot.error, refresh, }; } From c33ba6bb3cb61200d4a9f31a693319f3104c6381 Mon Sep 17 00:00:00 2001 From: Dmitrii Kataraev Date: Fri, 24 Jul 2026 21:50:45 -0700 Subject: [PATCH 2/4] fix(shell-ui): don't double up the denial wording; clear data on denial only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both of @anandray's points. **Doubled wording.** The raw message already says "permission denied" in exactly the case the friendly sentence exists to explain, so the two concatenated into "You do not have permission to view the dashboard. Permission denied". Replaced rather than appended; the verbatim text still goes to the console line directly above for anyone debugging. **_data retention — answering rather than confirming.** The retention was intentional for transient failures and should stay: a poll that blips must leave the last good numbers on screen with an error beside them, not blank the dashboard every time the network hiccups. Going empty on each failed poll would be its own bug. But it was wrong to apply that uniformly. A denial is not a blip — it means this user is not entitled to these numbers, and continuing to show a previous session's data under an "access denied" banner is both confusing and the wrong default for something access-scoped. So `_data` is now cleared on denial only, and the split is spelled out in the comment so the next reader does not have to guess which half was deliberate. Note the existing 104 tsc errors in apps/shell-ui are unrelated (unbuilt workspace deps); count is identical with and without this change, and none are in this file. CI does not typecheck shell-ui, so this was checked locally. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP --- apps/shell-ui/src/hooks/useDashboardData.ts | 22 +++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/apps/shell-ui/src/hooks/useDashboardData.ts b/apps/shell-ui/src/hooks/useDashboardData.ts index 34318cdbe..cfece18f6 100644 --- a/apps/shell-ui/src/hooks/useDashboardData.ts +++ b/apps/shell-ui/src/hooks/useDashboardData.ts @@ -124,9 +124,27 @@ function _fetchDashboard(): Promise { // load and the view sat on "Loading..." forever (saas #373). console.log('[useDashboardData] Dashboard fetch failed:', err); const message = err instanceof Error ? err.message : String(err); - _error = /denied|permission|forbidden|403/i.test(message) - ? `You do not have permission to view the dashboard. ${message}` + const denied = /denied|permission|forbidden|403/i.test(message); + // The raw message already says "permission denied" in the case we most + // want to explain, so prefixing it produced "You do not have permission + // to view the dashboard. Permission denied". Replace it instead of + // appending — the verbatim text stays in the console line above for + // anyone debugging. + _error = denied + ? 'You do not have permission to view the dashboard.' : message; + // Deliberate: `_data` is NOT cleared on a transient failure. A poll that + // blips should leave the last good numbers on screen with an error + // alongside them, not blank the dashboard every time the network hiccups + // — going empty on each failed poll would be its own bug. + // + // A denial is different. It means this user is not entitled to these + // numbers, and continuing to display a previous session's data under an + // "access denied" banner is both confusing and the wrong default for + // something access-scoped. So clear it in that case only. + if (denied) { + _data = null; + } _emit(); } finally { _fetchPromise = null; From d4c39a24aac0469baf374c62e60ccd8d1464a724 Mon Sep 17 00:00:00 2001 From: Dmitrii Kataraev Date: Mon, 27 Jul 2026 12:21:09 -0700 Subject: [PATCH 3/4] fix(shell-ui): classify a denial from the structured error, not the prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unresolved CodeRabbit threads. The second is the substantive one and it breaks this PR on its own terms. Denial detection was /denied|permission|forbidden|403/ over err.message. But getDashboard() goes through call(), so a structured DAP failure arrives as DAPException with the detail in `dapResult` — the human-readable message is not guaranteed to carry it. And even on the text path the pattern had 403 but not 401 and not "unauthorized", so a plain `Unauthorized` fell through as a transient blip. Either way the result is the branch failing to fire for exactly the case it exists for: a real denial would keep rendering the previous session's numbers under an "access denied" banner, which is the behaviour this PR is meant to remove. Now reads dapResult.status / statusCode / code first and falls back to the text, with 401 and unauthori[sz]ed added. Checked the classification against both: Unauthorized, unauthorised, Not authorized, 403 Forbidden, bare 401 and "failed with status 401" all classify as denied; timeout and network error stay transient, so cached data still survives a blip. Also corrected the snapshot comment, which claimed the object is replaced "only when data or events change" — it is replaced on error too. Related: #1673. --- apps/shell-ui/src/hooks/useDashboardData.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/apps/shell-ui/src/hooks/useDashboardData.ts b/apps/shell-ui/src/hooks/useDashboardData.ts index cfece18f6..0df98cdd8 100644 --- a/apps/shell-ui/src/hooks/useDashboardData.ts +++ b/apps/shell-ui/src/hooks/useDashboardData.ts @@ -85,7 +85,7 @@ interface DashboardSnapshot { // `data` to `null` at this declaration (the function that assigns `_data` is // defined further down); without the annotation, the reassignment in `_emit` // below would not type-check. Runtime behavior is unchanged. -/** Stable snapshot object — only replaced when data or events change. */ +/** Stable snapshot object — replaced whenever _emit() publishes a state update. */ let _snapshot: DashboardSnapshot = { data: _data, events: _events, error: _error }; /** Notify all subscribed React components that data changed. */ @@ -124,7 +124,22 @@ function _fetchDashboard(): Promise { // load and the view sat on "Loading..." forever (saas #373). console.log('[useDashboardData] Dashboard fetch failed:', err); const message = err instanceof Error ? err.message : String(err); - const denied = /denied|permission|forbidden|403/i.test(message); + + // getDashboard() goes through call(), so a structured DAP error arrives + // as DAPException with the detail in `dapResult` — the human message is + // not guaranteed to carry it. Matching prose alone therefore misses the + // case this branch exists for: a real denial would keep rendering the + // previous session's numbers under an "access denied" banner. Read the + // structured status first and fall back to the text. + const dapResult = + (err as { dapResult?: Record } | undefined)?.dapResult; + const status = Number(dapResult?.status ?? dapResult?.statusCode ?? dapResult?.code); + const denied = + status === 401 || + status === 403 || + /denied|permission|forbidden|unauthori[sz]ed|not\s*authori[sz]ed|\b40[13]\b/i.test( + message, + ); // The raw message already says "permission denied" in the case we most // want to explain, so prefixing it produced "You do not have permission // to view the dashboard. Permission denied". Replace it instead of From e771353f1c0d5eea5bcf125a48ab076397cc4065 Mon Sep 17 00:00:00 2001 From: Dmitrii Kataraev Date: Mon, 27 Jul 2026 13:25:46 -0700 Subject: [PATCH 4/4] fix(shell-ui): treat symbolic DAP denial codes as denials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit caught a real hole in this PR. `Number()` over dapResult.code turns a symbolic 'UNAUTHORIZED' or 'FORBIDDEN' into NaN, so neither status check fired, and the prose regex only ever saw `message`. With a generic human message — which is exactly the case this branch was written for, since the structured detail is not guaranteed to reach the text — `denied` stayed false. The consequence is the specific bug this PR is meant to kill: `_data` is deliberately not cleared on failure so a network blip leaves the last good numbers on screen, so a missed denial keeps rendering the previous session's figures under a vague error instead of "you do not have permission". Now matches the code as a string as well. The pattern is lifted to a named const rather than duplicated, and carries no `g` flag, so reusing it across two .test() calls has no lastIndex statefulness. Verified with tsc against apps/shell-ui/tsconfig.json: no errors in this file. (The remaining TS2307 "Cannot find module 'rocketride'" errors are the known unbuilt client-typescript dist/types artifact, not this change — and worth saying out loud that CI would not have caught a type error here either, since nothing typechecks apps/** on a PR.) Refs #1673. --- apps/shell-ui/src/hooks/useDashboardData.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/apps/shell-ui/src/hooks/useDashboardData.ts b/apps/shell-ui/src/hooks/useDashboardData.ts index 0df98cdd8..a82765811 100644 --- a/apps/shell-ui/src/hooks/useDashboardData.ts +++ b/apps/shell-ui/src/hooks/useDashboardData.ts @@ -134,12 +134,17 @@ function _fetchDashboard(): Promise { const dapResult = (err as { dapResult?: Record } | undefined)?.dapResult; const status = Number(dapResult?.status ?? dapResult?.statusCode ?? dapResult?.code); + // `code` is not always numeric — the server may send a symbolic + // 'UNAUTHORIZED' / 'FORBIDDEN', which Number() turns into NaN. Testing + // only the numeric status and the prose would then miss the denial + // whenever the human message is generic, which is precisely the failure + // this branch exists to prevent: the previous session's figures would + // stay on screen under a vague error. Match the code as text too. + const code = String(dapResult?.code ?? ''); + const DENIAL = + /denied|permission|forbidden|unauthori[sz]ed|not\s*authori[sz]ed|\b40[13]\b/i; const denied = - status === 401 || - status === 403 || - /denied|permission|forbidden|unauthori[sz]ed|not\s*authori[sz]ed|\b40[13]\b/i.test( - message, - ); + status === 401 || status === 403 || DENIAL.test(code) || DENIAL.test(message); // The raw message already says "permission denied" in the case we most // want to explain, so prefixing it produced "You do not have permission // to view the dashboard. Permission denied". Replace it instead of