Repository navigation
fix(dashboard): render error and unknown-type states instead of an infinite spinner - #3098
Conversation
…finite spinner Two console pages ordered their loading guard ahead of the terminal state, making the latter unreachable: - ApplicationDetailPage checked `isLoading || !instance || !ad` before `if (error)`. On a failed GET, isLoading is false and instance stays undefined, so the page spun forever and never showed the not-found message. The error branch now runs first. - ApplicationListPage's `defsLoading || (!ad && plural)` guard stayed true for any unrecognized plural once definitions had loaded, so 'Unknown application type.' was unreachable. The guard now only spins while definitions are loading. Both are pre-existing upstream behaviors carried in by the cozystack-ui vendoring (#2963). Adds co-located regression tests for the failed-GET and unknown-plural paths. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
github-actions
Bot
added
area/dashboard
dosubot
Bot
added
the
kind/backport
📝 WalkthroughWalkthroughApplication list and detail routes now separate loading states from unresolved or failed application lookups. The list page shows unknown-type only after definitions load, and the detail page shows not found when the instance fetch returns an error. ChangesConsole route guards
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses UI issues where certain console pages would remain stuck in a loading state indefinitely when encountering errors or unknown resource types. By reordering the conditional rendering logic, the application now correctly prioritizes error handling and 'not found' states, improving the user experience and providing better feedback on failed requests. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces unit tests and fixes loading guard bugs in both ApplicationListPage and ApplicationDetailPage to prevent infinite loading spinners when an application type is unknown or a fetch error occurs. The reviewer suggested a further improvement in ApplicationDetailPage to prevent an infinite spinner when the application definitions have loaded but the specific application definition (ad) is not found (e.g., due to an unrecognized plural parameter).
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Check the fetch error before the loading guard: on a failed GET, isLoading | ||
| // is false and instance stays undefined, so a !instance-first guard would | ||
| // spin forever and never reach this branch. | ||
| if (error) { | ||
| return ( | ||
| <div className="flex items-center gap-2 p-8 text-slate-500"> | ||
| <Spinner /> Loading… | ||
| <div className="p-8 text-red-600"> | ||
| Application <code>{name}</code> not found. | ||
| </div> | ||
| ) | ||
| } | ||
| if (error) { | ||
| if (isLoading || !instance || !ad) { | ||
| return ( | ||
| <div className="p-8 text-red-600"> | ||
| Application <code>{name}</code> not found. | ||
| <div className="flex items-center gap-2 p-8 text-slate-500"> | ||
| <Spinner /> Loading… | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
If the application definitions (defs) have finished loading but the specific application definition (ad) is not found (e.g., due to an unrecognized or invalid plural parameter), and there is no fetch error, the page will still render the loading spinner forever because !ad remains true.\n\nBy checking !defs instead of !ad in the loading guard, we can correctly show the spinner only while definitions are still loading. Once they have loaded, if ad is still undefined, we can render a clear "Unknown application type" error message instead of spinning indefinitely.
// Check the fetch error before the loading guard: on a failed GET, isLoading
// is false and instance stays undefined, so a !instance-first guard would
// spin forever and never reach this branch.
if (error) {
return (
<div className="p-8 text-red-600">
Application <code>{name}</code> not found.
</div>
)
}
if (isLoading || !instance || !defs) {
return (
<div className="flex items-center gap-2 p-8 text-slate-500">
<Spinner /> Loading…
</div>
)
}
if (!ad) {
return (
<div className="p-8 text-red-600">
Unknown application type <code>{plural}</code>.
</div>
)
}
![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
`@packages/system/dashboard/images/console/apps/console/src/routes/detail/ApplicationDetailPage.tsx`:
- Around line 75-79: The error branch in ApplicationDetailPage currently treats
every GET failure as if the application is missing. Update the conditional
around the existing error handling to distinguish 404 responses from other
failures, and use the application not found copy only for that case. For other
errors, keep the same component but render a generic “could not be loaded”
message, using the existing ApplicationDetailPage logic and the error state from
the fetch.
🪄 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: CHILL
Plan: Pro
Run ID: 24600e59-ca40-48e0-8cd7-bdd429c4f56c
📒 Files selected for processing (4)
packages/system/dashboard/images/console/apps/console/src/routes/ApplicationListPage.test.tsxpackages/system/dashboard/images/console/apps/console/src/routes/ApplicationListPage.tsxpackages/system/dashboard/images/console/apps/console/src/routes/detail/ApplicationDetailPage.test.tsxpackages/system/dashboard/images/console/apps/console/src/routes/detail/ApplicationDetailPage.tsx
| if (error) { | ||
| return ( | ||
| <div className="flex items-center gap-2 p-8 text-slate-500"> | ||
| <Spinner /> Loading… | ||
| <div className="p-8 text-red-600"> | ||
| Application <code>{name}</code> not found. | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don’t label every fetch failure as “not found.”
This branch now catches all GET errors, and the new test drives it with 403 Forbidden, so permission and transient backend failures will be shown as if the resource does not exist. Prefer 404-specific copy when you can detect it, or a generic “could not be loaded” message otherwise.
Suggested direction
if (error) {
return (
<div className="p-8 text-red-600">
- Application <code>{name}</code> not found.
+ Application <code>{name}</code> could not be loaded.
</div>
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (error) { | |
| return ( | |
| <div className="flex items-center gap-2 p-8 text-slate-500"> | |
| <Spinner /> Loading… | |
| <div className="p-8 text-red-600"> | |
| Application <code>{name}</code> not found. | |
| </div> | |
| if (error) { | |
| return ( | |
| <div className="p-8 text-red-600"> | |
| Application <code>{name}</code> could not be loaded. | |
| </div> |
🤖 Prompt for 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.
In
`@packages/system/dashboard/images/console/apps/console/src/routes/detail/ApplicationDetailPage.tsx`
around lines 75 - 79, The error branch in ApplicationDetailPage currently treats
every GET failure as if the application is missing. Update the conditional
around the existing error handling to distinguish 404 responses from other
failures, and use the application not found copy only for that case. For other
errors, keep the same component but render a generic “could not be loaded”
message, using the existing ApplicationDetailPage logic and the error state from
the fetch.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the guard reordering correctly makes the terminal states reachable, and both fixed paths have regression tests.
- ApplicationDetailPage: moving
if (error)ahead of theif (isLoading || !instance || !ad)guard is the right fix. On a failed initial GET the loading guard's!instancewas true (isLoading=false, instance=undefined), so the error branch was dead and the page spun forever. I checked the data-retained-during-refetch case (the 5s refetchInterval): the old code already fell through to the same error branch there, so error-first changes only the genuinely-broken initial-failure path — no new regression. Both branches return before any instance/ad field access, so no nil-deref. - ApplicationListPage: dropping
(!ad && plural)from the loading guard makes the "Unknown application type." branch reachable once definitions have loaded; the instance-list spinner remains separately handled, so no infinite spin after the guard.
Two non-blocking notes:
- The error branch renders "not found." for every GET failure including 403/5xx — pre-existing copy, just now reachable; a generic "could not be loaded" would be more accurate (follow-up).
- The detail page folds
!adinto the loading guard rather than having an explicit unknown-type branch like the list page; in practice an unknown plural 404s and hits the error branch, so it terminates — the only residual is the orthogonal case where definitions fail to load but the instance GET succeeds. Out of scope here.
Both new states are covered by co-located vitest files, and the UI typecheck/test job is green.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.5
git worktree add -d .worktree/backport-3098-to-release-1.5 origin/release-1.5
cd .worktree/backport-3098-to-release-1.5
git switch --create backport-3098-to-release-1.5
git cherry-pick -x 4838bafbc1fde8c505d58d403edc9f03138132ba |
|
Not backportable to The console is not in this branch's tree. So the Removing the |
myasnikovdaniil
removed
the
kind/backport
2 participants
What this PR does
Two console pages ordered their loading guard ahead of the terminal state, making the latter unreachable. Both are pre-existing upstream behaviors carried in by the cozystack-ui vendoring (#2963) and flagged as non-blocking findings in that PR's review.
ApplicationDetailPagecheckedisLoading || !instance || !adbeforeif (error). On a failed GET (403/404/5xx)isLoadingis false andinstancestays undefined, so the page rendered the loading spinner forever and never reached the "not found" branch. The error branch now runs first.ApplicationListPage'sdefsLoading || (!ad && plural)guard stayed true for any unrecognizedpluralonce definitions had loaded, so the "Unknown application type." branch was unreachable. The guard now only spins while definitions are loading.Verification
pnpm typecheckclean; full console suite green (pnpm test: 317 passed, +4 new).Summary by CodeRabbit