feat(admin): refine dashboard model and overview UI#426
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates dashboard value placeholders, combined token displays, shared empty states, provider status polling, and virtual model redirect/removal behavior, while also adjusting models table labels, pricing columns, and row actions. ChangesAdmin Dashboard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Confidence Score: 4/5This is close, but the provider details toggle should be fixed before merging.
internal/admin/dashboard/static/js/modules/providers.js Reviews (2): Last reviewed commit: "fix(admin): serialize virtual model row ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 946-949: Update formatCost in dashboard.js to treat non-finite
inputs the same as missing values, since NaN and Infinity currently fall through
to the currency formatting and produce invalid output. Add a guard at the start
of formatCost that returns the placeholder for null, undefined, NaN, and
infinite values, while keeping the existing small-value and toFixed formatting
behavior unchanged.
In `@internal/admin/dashboard/static/js/modules/budgets.js`:
- Around line 375-379: Treat null as a missing value in budgetAmountLabel before
numeric coercion. Update the budgetAmountLabel helper in budgets.js so it
returns '---' when the input value is null or otherwise absent, and only then
proceeds to Number(value) and the Number.isFinite check for valid numeric
formatting.
In `@internal/admin/dashboard/static/js/modules/providers.js`:
- Around line 60-62: The toggle label logic in providerStatusDetailsToggleLabel
is reversed relative to providerStatusDetailsExpanded. Update the label returned
from this method in providers.js so collapsed state shows “Show Details” and
expanded state shows “Hide Details”, matching the intended toggle behavior.
- Around line 31-39: The initProviderStatusPreferences method is overwriting the
saved provider details preference on every load by always writing a collapsed
value, which destroys the user’s stored choice. Update
initProviderStatusPreferences in providers.js to only initialize
this.providerStatusDetailsExpanded and read any existing value from
browserStorage()/PROVIDER_STATUS_DETAILS_STORAGE_KEY, leaving storage unchanged
unless the user explicitly toggles the details state elsewhere. Make sure the
saved preference is preserved across reloads instead of being reset during
initialization.
In `@internal/admin/dashboard/static/js/modules/providers.test.cjs`:
- Around line 94-101: The provider status details tests need a reload-style
persistence check in providers.test.cjs: current assertions around
toggleProviderStatusDetails only confirm storage writes, not that a previously
saved value is restored on module init. Update the test to seed storage with
'true', create a fresh module instance using the existing initialization path,
and assert providerStatusDetailsExpanded starts true (and the label matches) so
the persisted preference readback is covered.
In `@internal/admin/dashboard/static/js/modules/virtual-models.js`:
- Around line 59-78: buildDisplayModels() and hasConcreteSourceModel() are only
checking alias.name against qualifiedModelName()/selector, but source-backed
redirect detection should use the same identity set as the rest of the module.
Update the redirect lookup in buildDisplayModels and the concrete-model check in
hasConcreteSourceModel to reuse modelKeys() (and the existing
findConcreteModelByName path if needed) so any valid concrete key resolves
correctly. Keep the logic around aliases, rows, and
masking_alias/has_virtual_model unchanged aside from expanding the match
criteria.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e570d79-5e7f-4d98-8314-7715b8807b27
📒 Files selected for processing (19)
internal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/budgets.jsinternal/admin/dashboard/static/js/modules/budgets.test.cjsinternal/admin/dashboard/static/js/modules/dashboard-display.test.cjsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.cjsinternal/admin/dashboard/static/js/modules/providers.jsinternal/admin/dashboard/static/js/modules/providers.test.cjsinternal/admin/dashboard/static/js/modules/usage.jsinternal/admin/dashboard/static/js/modules/usage.test.cjsinternal/admin/dashboard/static/js/modules/virtual-models.jsinternal/admin/dashboard/static/js/modules/virtual-models.test.cjsinternal/admin/dashboard/templates/model-table-body.htmlinternal/admin/dashboard/templates/no-data-icon.htmlinternal/admin/dashboard/templates/page-audit-logs.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/admin/dashboard/templates/page-overview.htmlinternal/admin/dashboard/templates/page-usage.htmlinternal/admin/dashboard/templates/trash-icon.html
Comment on lines
60
to
+62
| providerStatusDetailsToggleLabel() { | ||
| return this.providerStatusDetailsExpanded ? 'Hide Details' : 'Show Details'; | ||
| return this.providerStatusDetailsExpanded ? 'Show Details' : 'Hide Details'; | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Flip the toggle action labels.
When details are collapsed, the toggle should offer “Show Details”; when expanded, it should offer “Hide Details”.
Proposed fix
providerStatusDetailsToggleLabel() {
- return this.providerStatusDetailsExpanded ? 'Show Details' : 'Hide Details';
+ return this.providerStatusDetailsExpanded ? 'Hide Details' : 'Show Details';
},📝 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.
Suggested change
| providerStatusDetailsToggleLabel() { | |
| return this.providerStatusDetailsExpanded ? 'Hide Details' : 'Show Details'; | |
| return this.providerStatusDetailsExpanded ? 'Show Details' : 'Hide Details'; | |
| }, | |
| providerStatusDetailsToggleLabel() { | |
| return this.providerStatusDetailsExpanded ? 'Hide Details' : 'Show Details'; | |
| }, |
🤖 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 `@internal/admin/dashboard/static/js/modules/providers.js` around lines 60 -
62, The toggle label logic in providerStatusDetailsToggleLabel is reversed
relative to providerStatusDetailsExpanded. Update the label returned from this
method in providers.js so collapsed state shows “Show Details” and expanded
state shows “Hide Details”, matching the intended toggle behavior.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/admin/dashboard/static/js/modules/virtual-models.js (2)
520-526: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow displayed alias rows to be removed even when their source matches a concrete model.
Line 84 only folds enabled valid source-backed redirects into concrete model rows; disabled/invalid source-backed redirects still render as alias rows. But Line 521 blocks
removeAliasRow()for those rows, androwRedirectCanRemove()does not apply because they are alias rows, so the row-level delete action is unavailable for that displayed virtual model.Proposed fix
aliasRowCanRemove(row) { - return Boolean(row && row.is_alias && row.alias && row.alias.name && !row.source_model_exists); + return Boolean(row && row.is_alias && row.alias && row.alias.name); },🤖 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 `@internal/admin/dashboard/static/js/modules/virtual-models.js` around lines 520 - 526, Displayed alias rows that still represent source-backed redirects are being blocked from removal because aliasRowCanRemove only allows deletion when source_model_exists is false, even though these rows are rendered as alias rows. Update aliasRowCanRemove and the related removeAliasRow flow in virtual-models.js so displayed alias rows can be deleted regardless of whether their source matches a concrete model, while keeping rowRedirectCanRemove limited to redirect rows.
970-990: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSerialize row deletes or track multiple in-flight rows.
rowDeletingKeyis a single value, but the guards only block the same row key. Deleting two different rows concurrently can overwriterowDeletingKey, race thefetchModels()/fetchVirtualModels()refreshes, and leavedisplayModelswith stale data from the slower request.Proposed fix
async removeAliasRow(row) { - if (!this.aliasRowCanRemove(row) || this.rowDeletingKey === row.key) { + if (!this.aliasRowCanRemove(row) || this.rowDeletingKey) { return; } @@ async removeRedirectRow(row) { - if (!this.rowRedirectCanRemove(row) || this.rowDeletingKey === row.key) { + if (!this.rowRedirectCanRemove(row) || this.rowDeletingKey) { return; }Also applies to: 992-1035
🤖 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 `@internal/admin/dashboard/static/js/modules/virtual-models.js` around lines 970 - 990, Serialize deletes in the virtual models row handlers because rowDeletingKey only tracks one row and concurrent removes can race the refreshes; update removeAliasRow, removeRedirectRow, and the shared removeVirtualModelSource flow to prevent overlapping deletes or to track multiple in-flight row keys. Make sure the guards around rowDeletingKey, fetchModels, fetchVirtualModels, and displayModels refreshes cannot interleave stale results from an earlier request overwriting newer data.
🤖 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.
Outside diff comments:
In `@internal/admin/dashboard/static/js/modules/virtual-models.js`:
- Around line 520-526: Displayed alias rows that still represent source-backed
redirects are being blocked from removal because aliasRowCanRemove only allows
deletion when source_model_exists is false, even though these rows are rendered
as alias rows. Update aliasRowCanRemove and the related removeAliasRow flow in
virtual-models.js so displayed alias rows can be deleted regardless of whether
their source matches a concrete model, while keeping rowRedirectCanRemove
limited to redirect rows.
- Around line 970-990: Serialize deletes in the virtual models row handlers
because rowDeletingKey only tracks one row and concurrent removes can race the
refreshes; update removeAliasRow, removeRedirectRow, and the shared
removeVirtualModelSource flow to prevent overlapping deletes or to track
multiple in-flight row keys. Make sure the guards around rowDeletingKey,
fetchModels, fetchVirtualModels, and displayModels refreshes cannot interleave
stale results from an earlier request overwriting newer data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f49c21cf-f0a1-4ea6-9fa2-2e4904e0e651
📒 Files selected for processing (8)
internal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/budgets.jsinternal/admin/dashboard/static/js/modules/budgets.test.cjsinternal/admin/dashboard/static/js/modules/dashboard-display.test.cjsinternal/admin/dashboard/static/js/modules/providers.jsinternal/admin/dashboard/static/js/modules/providers.test.cjsinternal/admin/dashboard/static/js/modules/virtual-models.jsinternal/admin/dashboard/static/js/modules/virtual-models.test.cjs
Comment on lines
65
to
+67
| providerStatusDetailsToggleLabel() { | ||
| return this.providerStatusDetailsExpanded ? 'Hide Details' : 'Show Details'; | ||
| return this.providerStatusDetailsExpanded ? 'Show Details' : 'Hide Details'; | ||
| }, |
There was a problem hiding this comment.
providerStatusDetailsExpanded controls whether the details panel is visible. With the current return values, the default collapsed state shows Hide Details, even though clicking will show the panel, and the expanded state shows Show Details, even though clicking will hide it. Return the next action label so the control matches what the click does.
Suggested change
| providerStatusDetailsToggleLabel() { | |
| return this.providerStatusDetailsExpanded ? 'Hide Details' : 'Show Details'; | |
| return this.providerStatusDetailsExpanded ? 'Show Details' : 'Hide Details'; | |
| }, | |
| providerStatusDetailsToggleLabel() { | |
| return this.providerStatusDetailsExpanded ? 'Hide Details' : 'Show Details'; | |
| }, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes
---.Tests