fix(dashboard): CodeRabbit follow-ups on budgets, usage, and tool tests#404
Merged
Conversation
Address the reproducible findings from issue #377 (CodeRabbit's out-of-scope review of #367). Each was verified against current source; two of the four did not reproduce. Budgets: the custom reset-period field rounded fractional days with Math.round, so 1.5 silently became 2 days. Validate with Number.isInteger via a local draft, reject non-integer or non-positive input, flag it inline, and leave it unsaved. Usage: drilling from a breakdown row into the Activity log dropped the other active filter (drilling into a model while filtering by user lost the user, and vice versa). Merge the existing filter with the clicked value before navigating. Tools & Guardrails: a URL reachability result was gated only by an onChange reset, which misses a result stranded when the committed URL changes from a refetch (no keystroke). Track the tested URL and render a result only while it still matches the field. Verified as already-handled, no change needed: - Provider diagnostics (finding 1): ActivityPage renders a generic message, not entry.error_message; ProvidersPage test/health errors are already sanitized and capped server-side (_short_error), by design for the master-key operator. - Editing a URL while a test is pending (part of finding 3): the existing onChange reset already detaches the mutation observer and drops that result. Rebuild the committed dashboard bundle. Adds regression tests for each fix (the Tools test fails without the URL-match guard). Fixes #377 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
July 23, 2026 20:59 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
the
missing-template
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
WalkthroughDashboard updates validate custom budget periods, prevent stale tool URL test results, preserve Usage drill-down filters, and regenerate the static dashboard assets and hashed entrypoint references. ChangesDashboard follow-up fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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-actions
Bot
removed
the
missing-template
![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
🧹 Nitpick comments (1)
web/src/pages/UsagePage.test.tsx (1)
156-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an accessible location probe instead of
getByTestId.The new assertions use
getByTestId("loc"). Expose the test route through an accessible role and query that role instead.Suggested test selector
function LocationProbe() { const loc = useLocation(); - return <div data-testid="loc">{`${loc.pathname}${loc.search}`}</div>; + return <div role="status" aria-label="Current location">{`${loc.pathname}${loc.search}`}</div>; } -const loc = screen.getByTestId("loc").textContent ?? ""; +const loc = screen.getByRole("status", { name: "Current location" }).textContent ?? "";As per coding guidelines, “query the UI as a user would with
getByRole,getByLabelText, orgetByTextinstead ofgetByTestId.”Also applies to: 178-199
🤖 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 `@web/src/pages/UsagePage.test.tsx` around lines 156 - 176, Replace the getByTestId("loc") query in the drilling/navigation tests with an accessible-role query for the test route location probe, using the probe’s exposed accessible name or label. Keep the existing URL assertions unchanged and apply the same selector update to the additional affected test block.Source: Coding guidelines
🤖 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 `@src/gateway/static/dashboard/assets/UsagePage-Dk-t74Uz.js`:
- Line 1: Rebuild the dashboard bundle containing the UsagePage module so the
generated asset has only one copy of its imports and top-level symbols,
including the De/UsagePage export. Verify that all hashed asset references point
to the newly generated files, then commit the rebuilt output under the static
dashboard assets.
---
Nitpick comments:
In `@web/src/pages/UsagePage.test.tsx`:
- Around line 156-176: Replace the getByTestId("loc") query in the
drilling/navigation tests with an accessible-role query for the test route
location probe, using the probe’s exposed accessible name or label. Keep the
existing URL assertions unchanged and apply the same selector update to the
additional affected test block.
🪄 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 Plus
Run ID: d9176c77-0ddb-4fb6-bf10-a34db264eb6d
📒 Files selected for processing (23)
src/gateway/static/dashboard/assets/ActivityPage-Cw34Jlno.jssrc/gateway/static/dashboard/assets/AliasesPage-CCRTl9Co.jssrc/gateway/static/dashboard/assets/BudgetsPage-iAjnK5BF.jssrc/gateway/static/dashboard/assets/BudgetsPage-inMAr92N.jssrc/gateway/static/dashboard/assets/KeysPage-C6eaoTUP.jssrc/gateway/static/dashboard/assets/ModelScopeControl-CkKcIwGe.jssrc/gateway/static/dashboard/assets/ModelsPage-DwBDAaxV.jssrc/gateway/static/dashboard/assets/OverviewPage-B_g2blUc.jssrc/gateway/static/dashboard/assets/ProvidersPage-DhHL_fxe.jssrc/gateway/static/dashboard/assets/SettingsPage-BPWZBXQE.jssrc/gateway/static/dashboard/assets/ToolsGuardrailsPage-Zh-JPOxJ.jssrc/gateway/static/dashboard/assets/ToolsGuardrailsPage-rcsPeR9V.jssrc/gateway/static/dashboard/assets/UsagePage-Dk-t74Uz.jssrc/gateway/static/dashboard/assets/UsersPage-D6v_0kF6.jssrc/gateway/static/dashboard/assets/index-CHQYSDoG.csssrc/gateway/static/dashboard/assets/index-DAVt9uB3.jssrc/gateway/static/dashboard/index.htmlweb/src/pages/BudgetsPage.test.tsxweb/src/pages/BudgetsPage.tsxweb/src/pages/ToolsGuardrailsPage.test.tsxweb/src/pages/ToolsGuardrailsPage.tsxweb/src/pages/UsagePage.test.tsxweb/src/pages/UsagePage.tsx
💤 Files with no reviewable changes (2)
- src/gateway/static/dashboard/assets/ToolsGuardrailsPage-rcsPeR9V.js
- src/gateway/static/dashboard/assets/BudgetsPage-inMAr92N.js
Self-review: the budgets custom-period validity check used Number.isInteger, which admits a finite-but-overflowing day count (1e308 is an integer, yet 1e308 * DAY overflows to Infinity). Switch both the inline validity check and onChange to Number.isSafeInteger, restoring the finiteness guard the original Math.round path had and rejecting overflowing input outright. CodeRabbit nitpick: the UsagePage test location probe was queried by test id. Expose it via a status role with an accessible name and query getByRole, per the frontend testing guideline; migrate the pre-existing usage too for consistency. Rebuild the committed dashboard bundle for the budgets change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
njbrake
temporarily deployed
to
integration-tests
July 23, 2026 21:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Updates the Otari admin dashboard to address CodeRabbit follow-ups around budgets, usage drill-down filtering, and tools/guardrails URL testing, including rebuilt static dashboard assets shipped by the backend.
Changes:
- Preserve active user/model filters when drilling from Usage breakdown tables into the Activity log.
- Reject fractional/non-positive custom budget reset periods in the Budgets “Reset period” picker, and add regression coverage.
- Prevent stale tool URL test results from rendering after the committed URL changes, with a regression test, then rebuild the committed dashboard bundle.
Reviewed changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/src/pages/UsagePage.tsx | Keeps the other active filter when drilling into Activity (model drill keeps user; user drill keeps model). |
| web/src/pages/UsagePage.test.tsx | Adds coverage for drill-down filter preservation and updates navigation probe querying. |
| web/src/pages/ToolsGuardrailsPage.tsx | Gates reachability test result rendering on the URL that was tested. |
| web/src/pages/ToolsGuardrailsPage.test.tsx | Adds regression coverage to ensure stale results don’t display after a refetch changes the committed URL. |
| web/src/pages/BudgetsPage.tsx | Switches custom reset-period input to draft+validation to reject fractional/non-positive day counts. |
| web/src/pages/BudgetsPage.test.tsx | Adds regression test that fractional day input is rejected (not rounded). |
| src/gateway/static/dashboard/index.html | Updates asset hashes for the rebuilt dashboard bundle. |
| src/gateway/static/dashboard/assets/UsersPage-D5CmT8Bq.js | Rebuilt dashboard asset (hash/content updated). |
| src/gateway/static/dashboard/assets/UsagePage-B95PT-hq.js | Rebuilt dashboard asset reflecting Usage drill-down filter preservation. |
| src/gateway/static/dashboard/assets/ToolsGuardrailsPage-rcsPeR9V.js | Removed old hashed bundle chunk after rebuild. |
| src/gateway/static/dashboard/assets/ToolsGuardrailsPage-DWSik0Ga.js | Added new hashed bundle chunk after rebuild. |
| src/gateway/static/dashboard/assets/SettingsPage-hENBFRMI.js | Rebuilt dashboard asset (hash/content updated). |
| src/gateway/static/dashboard/assets/ProvidersPage-BEN5bHZD.js | Rebuilt dashboard asset (hash/content updated). |
| src/gateway/static/dashboard/assets/OverviewPage-KThfhw1Z.js | Rebuilt dashboard asset (hash/content updated). |
| src/gateway/static/dashboard/assets/ModelScopeControl-DiX2Etqz.js | Rebuilt dashboard asset (hash/content updated). |
| src/gateway/static/dashboard/assets/KeysPage-DCo4FFYU.js | Rebuilt dashboard asset (hash/content updated). |
| src/gateway/static/dashboard/assets/BudgetsPage-inMAr92N.js | Removed old hashed bundle chunk after rebuild. |
| src/gateway/static/dashboard/assets/BudgetsPage-Ctwj8twD.js | Added new hashed bundle chunk after rebuild (includes custom period validation). |
| src/gateway/static/dashboard/assets/AliasesPage-RDVKM-by.js | Rebuilt dashboard asset (hash/content updated). |
| src/gateway/static/dashboard/assets/ActivityPage-BrL4O-hT.js | Rebuilt dashboard asset (hash/content updated). |
Files not reviewed (2)
- src/gateway/static/dashboard/assets/BudgetsPage-Ctwj8twD.js: Generated file
- src/gateway/static/dashboard/assets/ToolsGuardrailsPage-DWSik0Ga.js: Generated file
khaledosman
reviewed
Jul 24, 2026
Member
There was a problem hiding this comment.
CodeRabbit follow-ups: reject fractional budget periods, carry the complementary filter on Usage drill-down, and gate a tool-test result on the URL it was run against. The triage table is well-reasoned and the two false positives are correctly left alone.
Two of the three fixes are solid; the budget one has a real defect (inline). Assessment of the parts I checked:
- Usage drill-down (finding 4): correct and complete — the Usage page only has model + user filters (plus the time window, which
drillTocarries viastart_date), so merging model↔user covers every cross-filter that exists. No api-key/status filter is dropped because none exists on this page. - Tool test URL gate (finding 3): correct.
testedUrl+resultMatchescloses the no-keystroke refetch path thattest.reset()misses, and the regression test genuinely fails without the guard (it drives a PATCH-triggered re-seed of the field). Good, subtle catch. - Budgets (finding 2): the validation intent is right but the draft is wiped on the interactive path — see inline.
🤖 Generated with Claude Code
khaledosman
approved these changes
Jul 24, 2026
Addresses review feedback from @khaledosman and Copilot on the budgets fix. khaledosman: the draft-reseed effect defeated the validation on the interactive and edit paths. Making a committed value fractional (e.g. 14 -> 14.5) emits onChange(null), which recomputed committedDays to "" and the effect then wiped the draft, so the error never showed. durationSec is only ever changed by PeriodPicker itself, so reseeding the draft from it always reacted to this component's own emission. Drop the effect; seed the draft on mount and reset it only on an explicit preset click (switching budgets already remounts the keyed form). Copilot: an invalid entry emitting null let Save clear an existing period to "no reset". Lift the period's validity to the form (like the limit field) and disable Save while the custom entry is invalid, so an invalid value can neither be rounded nor silently saved. Update the two paste-into-empty tests to assert Save is blocked, and add an edit test that makes a committed 14-day period fractional and asserts the field keeps "14.5", shows the error, and disables Save (the path the paste tests missed). Rebuild the committed dashboard bundle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…erabbit-fixes # Conflicts: # src/gateway/static/dashboard/assets/ActivityPage-dbjTjmS5.js # src/gateway/static/dashboard/assets/AliasesPage-C2JIWSiZ.js # src/gateway/static/dashboard/assets/AliasesPage-DqjEr6cA.js # src/gateway/static/dashboard/assets/AliasesPage-QYibN81v.js # src/gateway/static/dashboard/assets/BudgetsPage-CpV2TFpx.js # src/gateway/static/dashboard/assets/KeysPage-DkXRnk8b.js # src/gateway/static/dashboard/assets/ModelScopeControl-CV1Wulbr.js # src/gateway/static/dashboard/assets/ModelScopeControl-DTCXvnvU.js # src/gateway/static/dashboard/assets/ModelScopeControl-MeaPLlQS.js # src/gateway/static/dashboard/assets/ModelsPage-C-ZlmFwf.js # src/gateway/static/dashboard/assets/ModelsPage-CN4L8fzW.js # src/gateway/static/dashboard/assets/ModelsPage-DuDhd7FJ.js # src/gateway/static/dashboard/assets/OverviewPage-Cb1DW9ZC.js # src/gateway/static/dashboard/assets/ProvidersPage-CLe2cN4-.js # src/gateway/static/dashboard/assets/ProvidersPage-CmlcXh68.js # src/gateway/static/dashboard/assets/ProvidersPage-SpQb9A7m.js # src/gateway/static/dashboard/assets/SettingsPage-BdorKXf4.js # src/gateway/static/dashboard/assets/SettingsPage-Biy7SR7l.js # src/gateway/static/dashboard/assets/SettingsPage-UC2EHTIR.js # src/gateway/static/dashboard/assets/ToolsGuardrailsPage-KRNTlQJw.js # src/gateway/static/dashboard/assets/UsagePage-CRzEneXv.js # src/gateway/static/dashboard/assets/UsersPage-BGSuZgqf.js # src/gateway/static/dashboard/assets/UsersPage-EoUcpZFH.js # src/gateway/static/dashboard/assets/UsersPage-zP5EVG6k.js # src/gateway/static/dashboard/assets/index-BFG8RNrJ.css # src/gateway/static/dashboard/assets/index-C94BH4OZ.js # src/gateway/static/dashboard/assets/index-CHQYSDoG.css # src/gateway/static/dashboard/assets/index-CqZ8NnYY.css # src/gateway/static/dashboard/index.html
njbrake
temporarily deployed
to
integration-tests
July 24, 2026 09:29 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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
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.
Description
Addresses #377 (CodeRabbit's out-of-scope review of #367). Each of the four findings was verified against current source; two did not reproduce.
Budgets (finding 2):
PeriodPickerusedMath.round(n) * DAY, so 1.5 silently became 2 days. Now uses a local draft withNumber.isIntegervalidation, rejects non-integer or non-positive input, flags it inline ("Enter a whole number of days"), and leaves it unsaved rather than coercing the operator's value.Usage (finding 4): Drilling from a breakdown row into the Activity log dropped the other active filter (drilling into a model while filtering by user lost the user, and vice versa). Now merges the existing filter with the clicked value before navigating.
Tools and Guardrails (finding 3): The described scenario (editing the URL while a test is pending) is already handled: the
onChangetest.reset()detaches the mutation observer, so that result is dropped. The residual gapreset()misses is the committed URL changing from a refetch (no keystroke), which strands a result against a new URL. Now tracks the tested URL and renders a result only while it still matches the field. The added regression test fails without this guard.Already handled, no change (finding 1):
ActivityPagerenders a hardcoded generic message, neverentry.error_message.ProvidersPagetest and health errors are already sanitized and capped server-side (_short_error: 300 char cap, strips API keys, drops the provider tag) and echoed to the master-key operator by design.Rebuilt the committed dashboard bundle. The build is deterministic and produces identical hashes under Node 22 (CI) and Node 24, so the staleness check passes.
PR Type
Relevant issues
Fixes #377
Checklist
web/src; 215 pass, 4 added).npm --prefix web run typecheck,npm --prefix web test,npm --prefix web run build; this PR is frontend only).AI Usage
AI Model/Tool used: Claude Code (Claude Opus 4.8), directed by @njbrake.
Any additional AI details you'd like to share: Each CodeRabbit finding was verified against current source; two were confirmed false positives and left unchanged, with the reasoning recorded above and in the commit message.
Summary