fix(shell-ui): surface dashboard fetch failures instead of loading forever - #1673
Conversation
…rever _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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesDashboard error state
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant useDashboardData
participant getDashboard
participant Subscribers
useDashboardData->>getDashboard: Fetch dashboard data
getDashboard-->>useDashboardData: Return data or fetch failure
useDashboardData->>useDashboardData: Update data and error state
useDashboardData->>Subscribers: Emit updated snapshot
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/shell-ui/src/hooks/useDashboardData.tsOops! Something went wrong! :( ESLint: 9.39.5 TypeError: expand is not a function Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/shell-ui/src/hooks/useDashboardData.ts`:
- Around line 88-93: Update the comment above _snapshot in the snapshot state
section to state that the object is replaced when data, events, or error
changes, matching the behavior in _emit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b5476f68-5b1c-4ca9-9462-848dea40ef66
📒 Files selected for processing (1)
apps/shell-ui/src/hooks/useDashboardData.ts
anandray
left a comment
anandray
left a comment
There was a problem hiding this comment.
Verdict: approve. Right shape, backward-compatible, useful comment. Two minor notes.
What's good
- Emits on failure now, not just success. That's the whole fix — consumers stop being trapped in
data === nulllimbo when a fetch fails. Correct. - Backward-compatible additive change to
DashboardSnapshot/DashboardData.monitor-uidestructures a subset and won't type-error; verified by kwit'stsc --noEmit. - Comment is exactly right — points at saas #373 as the incident source, explains why
_data === nullmasked the failure. Reader lands here on triage and immediately knows why the field exists. - Permission-denial humanisation so callers can render user-friendly text rather than a raw exception string.
- Consolidates the fix in the shell-ui hook rather than duplicating it in each consumer app. Right call given #314 moved this hook here.
Minor notes
1. _data is not cleared on error — intentional?
Current behaviour: a successful fetch populates _data; a subsequent failure sets _error but keeps _data. Consumers can render stale data alongside the error banner — usually the right UX ("here's what we had, warn about the failure"), but confirm it's intentional. If not, add _data = null in the catch block; if so, worth a line in the comment saying so.
2. console.log for the raw error, not console.error.
console.log is fine for dev but doesn't get the red styling / level filtering in browser devtools that console.error gets. Preexisting style, so defer if it matches convention here. Minor.
3. Permission-detection regex is broad but not incorrect.
/denied|permission|forbidden|403/i will catch legitimate permission denials and also match any 5xx that happens to contain "permission" in an unrelated context. Over-matching means "You do not have permission…" is shown for something that isn't strictly a permission issue — mildly confusing but never harmful. Fine as heuristic.
4. Wording nit: You do not have permission to view the dashboard. ${message} produces "You do not have permission to view the dashboard. Permission 'task.monitor' denied" — the doubled-up prefix reads a bit awkwardly. Consider dropping the raw message in the permission case (or preserving it only in a details tooltip).
Approving.
…al only 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/shell-ui/src/hooks/useDashboardData.ts`:
- Around line 127-147: Update the dashboard failure handling around
getDashboard/call so authorization failures are classified from structured
dapResult status or code, not only the message text. Include Unauthorized and
equivalent 401/403 forms in the denied detection, then preserve the existing
behavior of clearing _data only for denied failures while retaining cached data
for transient errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4914e19-b3cd-4a02-af0f-b4252b34d086
📒 Files selected for processing (1)
apps/shell-ui/src/hooks/useDashboardData.ts
|
Both done in c33ba6b. 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
Keeping the last good data on a transient failure is correct and should stay: a poll that blips must leave the numbers on screen with an error beside them, not blank the dashboard every time the network hiccups. Clearing on every failed poll would be its own bug. A denial is not a blip. It means this user is not entitled to these numbers, and continuing to render a previous session's data under an "access denied" banner is both confusing and the wrong default for something access-scoped. So On the tsc situation, for the record: |
anandray
left a comment
anandray
left a comment
There was a problem hiding this comment.
Re-review approve. The transient-blip vs denial split on _data retention is a better answer than the binary I flagged — keeping stale data behind a permission banner would have been its own bug, keeping it through a network hiccup is right. The double-message wording is fixed by replacing rather than prefixing, and the raw message is preserved in the console line for debugging. Clean.
…rose 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/shell-ui/src/hooks/useDashboardData.ts`:
- Around line 134-142: Update the denial detection near dapResult and denied to
recognize symbolic DAP authorization codes such as “UNAUTHORIZED” and
“FORBIDDEN” in addition to numeric status values. Ensure these codes set denied
even when the error message is generic, so prior access-scoped _data is not
retained after authorization failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f85bba92-b85b-4484-8d9c-113542fb7412
📒 Files selected for processing (1)
apps/shell-ui/src/hooks/useDashboardData.ts
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.
anandray
left a comment
anandray
left a comment
There was a problem hiding this comment.
Re-review approve. Reading the structured DAP error first is the right shape and it's what I should have suggested rather than just noting the regex was broad.
What's good
- Structured status first, prose fallback second. DAP call() surfaces the detail on
dapResult; matching only the human message is fragile because the message is not guaranteed to carry the code. Your framing ("a real denial would keep rendering the previous session's numbers under an 'access denied' banner") names the exact false-negative this branch exists to prevent. - Symbolic + numeric coverage. DAP might send
code: 403(numeric) orcode: 'FORBIDDEN'(string).Number(...)on the symbolic form gives NaN → text-match path kicks in. Both handled. - Regex broadened correctly:
unauthori[sz]ed,\b40[13]\b, both spellings. Wide enough to catch symbolic variants without over-matching random 5xx bodies. - Type-safe access pattern:
(err as { dapResult?: Record<string, unknown> } | undefined)?.dapResult— optional chaining + narrowed cast. Safe. - No breaking type change: still exports
error: string | null— unlike #1707, no consumer type is silently widened.
Nit (non-blocking)
The three-source disjunction (status, code as text, message) is a lot to hold in the head.
Would read cleaner extracted into a isDenial(err) helper next time you're near this file. Not for this PR — the inline version is fine and the comments do the work. Just a shape-nit for future refactoring.
Approving.
2 participants
Why
_fetchDashboard()catches every error and onlyconsole.logs it, leaving_datanull. Consumers render their loading state wheneverdatais null — so a failed fetch is indistinguishable from a slow one, and the view sits on "Loading dashboard data..." forever.That is exactly the incident in rocketride-saas #373: the call was returning
Permission 'task.monitor' deniedand the UI never surfaced it, so it read as a hang and reached us from a user rather than from an alert.What
_error), clear it on success.erroron the hook so views can render it.Backward compatible — this only adds a field; the existing consumer (
monitor-ui) destructures a subset and is unaffected. Verified withtsc --noEmitonapps/shell-ui: no errors in the changed file.Note on where this lives
I originally fixed this in
rocketride-saas(apps/admin-ui/src/useDashboardData.ts), but #314 (UI consistency program) has since deleted that copy and moved the hook here, carrying the swallow with it unchanged. This is the right home — fixing it here covers every consumer instead of one app. The saas-side PR is being closed in favour of this.Follow-up
The saas
DashboardViewneeds a small companion change to rendererror(with a Retry) instead of itsEmptyStateloading title. That lands after this, once the hook exposes the field.Related: rocketride-saas #373 (incident + postmortem), #374 (backend root cause), server #1670 (engine stops swallowing the denial reason).
🤖 Generated with Claude Code
Summary by CodeRabbit
errorfield in the dashboard data.