fix(dashboard): hide Rate Limits nav item when the feature is disabled#512
Merged
Conversation
The sidebar item, the Rate Limits page affordances, and the models-page gauge buttons were already gated on `rateLimitsEnabled()`, and the backend already served `RATE_LIMITS_ENABLED: "off"`. The flag never reached the gate: `workflowRuntimeConfigKeys()` is a client-side allowlist that filters the `/admin/runtime/config` payload, and `RATE_LIMITS_ENABLED` was missing from it. The key was dropped, `workflowRuntimeBooleanFlag` saw an empty value, and fell back to its `true` default. `BUDGETS_ENABLED` and `GUARDRAILS_ENABLED` are allowlisted, which is why only rate limits misbehaved. Fixing the allowlist exposed a second defect. `dashboardDataFetches()` starts `fetchWorkflowsPage()` (which loads the flags) and `fetchRateLimitsPage()` in the same eager batch, so the gate still read an empty config, defaulted to `true`, and issued `GET /admin/rate-limits` — a 503 logged as an error on every models-page load with the feature off. `fetchBudgetsPage()` had the same latent race. `fetchWorkflowRuntimeConfig()` now shares its in-flight request, and a new `ensureWorkflowRuntimeConfig()` lets a feature gate await the flags instead of racing them. Both gates await it. This also collapses the duplicate `/admin/runtime/config` GET that overlapping callers used to issue. The JS allowlist silently duplicates `DashboardConfigResponse`'s json tags, which is precisely the drift that caused this. A contract test now reflects over the struct, parses the allowlist, and fails in both directions. User-visible impact: with `RATE_LIMITS_ENABLED=false` the Rate Limits nav item is hidden and the dashboard no longer calls the disabled endpoint. No behavior change when the feature is enabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 adds shared runtime-config promise caching in ChangesRuntime Config Caching and Consumer Wiring
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BudgetsPage as budgets.js
participant RateLimitsPage as rate-limits.js
participant Workflows as workflows.js
participant API as /admin/runtime/config
BudgetsPage->>Workflows: ensureWorkflowRuntimeConfig()
RateLimitsPage->>Workflows: ensureWorkflowRuntimeConfig()
Workflows->>API: GET runtime config
API-->>Workflows: config JSON incl. RATE_LIMITS_ENABLED
Workflows-->>BudgetsPage: config loaded
Workflows-->>RateLimitsPage: config loaded
BudgetsPage->>BudgetsPage: check budgetManagementEnabled()
RateLimitsPage->>RateLimitsPage: check rateLimitsEnabled()
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
internal/admin/dashboard/static/js/modules/workflows.js (1)
759-822: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOnly mark the runtime config as loaded after a successful fetch. A transient failure on the first
/admin/runtime/configrequest setsworkflowRuntimeConfigLoaded = true, soensureWorkflowRuntimeConfig()stops retrying and feature gates stay on their default values for the rest of the session.🤖 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/workflows.js` around lines 759 - 822, The runtime config fetch path in fetchWorkflowRuntimeConfig/loadWorkflowRuntimeConfig is marking workflowRuntimeConfigLoaded as true even when /admin/runtime/config fails, which prevents ensureWorkflowRuntimeConfig from retrying. Move the loaded flag update so it only happens after a successful fetch and response handling, while leaving failures to keep workflowRuntimeConfigPromise cleared and allow later retries. Use the existing symbols fetchWorkflowRuntimeConfig, ensureWorkflowRuntimeConfig, and loadWorkflowRuntimeConfig to update the success/failure flow without changing the caller contract.
🤖 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/workflows.js`:
- Around line 759-822: The runtime config fetch path in
fetchWorkflowRuntimeConfig/loadWorkflowRuntimeConfig is marking
workflowRuntimeConfigLoaded as true even when /admin/runtime/config fails, which
prevents ensureWorkflowRuntimeConfig from retrying. Move the loaded flag update
so it only happens after a successful fetch and response handling, while leaving
failures to keep workflowRuntimeConfigPromise cleared and allow later retries.
Use the existing symbols fetchWorkflowRuntimeConfig,
ensureWorkflowRuntimeConfig, and loadWorkflowRuntimeConfig to update the
success/failure flow without changing the caller contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0acadc3-ac60-4a19-b2cb-1d17fd9a7fc4
📒 Files selected for processing (8)
internal/admin/dashboard/static/js/modules/budgets.jsinternal/admin/dashboard/static/js/modules/budgets.test.cjsinternal/admin/dashboard/static/js/modules/rate-limits.jsinternal/admin/dashboard/static/js/modules/rate-limits.test.cjsinternal/admin/dashboard/static/js/modules/workflows.jsinternal/admin/dashboard/static/js/modules/workflows.test.cjsinternal/admin/dashboard_config_contract_test.gointernal/admin/handler_test.go
`fetchWorkflowRuntimeConfig()` marked the flags loaded from a `.finally()`, so a failed `/admin/runtime/config` fetch counted as a successful load. `loadWorkflowRuntimeConfig()` swallows its own errors and resolves normally on a network throw, on an unhandled response (auth failure, non-2xx), and on stale auth, so every failure path set `workflowRuntimeConfigLoaded = true` alongside an empty config. `ensureWorkflowRuntimeConfig()` then short-circuited forever: a single failed load left every feature gate reading an empty config and falling back to its default. `_applyRoute()` calls `fetchRateLimitsPage()` on its own when navigating, with no config fetch alongside, so the Rate Limits nav item reappeared and the dashboard called `/admin/rate-limits` — a 503 — even with `RATE_LIMITS_ENABLED=false`. The flag now tracks whether `workflowRuntimeConfig` holds a real response: set on success, cleared wherever the config is wiped. `.finally()` keeps clearing the in-flight promise, so failures stay retryable and the caller contract is unchanged. The stale-auth path deliberately leaves the flag untouched, since a superseded request must not invalidate a good prior load. Verified end-to-end against a running gateway: booting with a bad API key then supplying the correct one previously left `loaded=true` with no retry and one 503; it now retries, reads `RATE_LIMITS_ENABLED=off`, and issues no request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Problem
With
RATE_LIMITS_ENABLED=false, the Rate Limits sidebar item stayed visible, and the dashboard kept callingGET /admin/rate-limits— which returns 503feature_unavailable— logging a server-side error on every models-page load.Root cause
All the gating was already in place:
sidebar.htmlhasx-show="rateLimitsEnabled()", and the backend correctly serves"RATE_LIMITS_ENABLED": "off".The flag never reached the gate.
workflowRuntimeConfigKeys()inworkflows.jsis a client-side allowlist that filters the/admin/runtime/configpayload, andRATE_LIMITS_ENABLEDwas missing from it. The key was dropped,workflowRuntimeBooleanFlagsaw an empty value, and fell back to itstruedefault.BUDGETS_ENABLEDandGUARDRAILS_ENABLEDare allowlisted, which is why only rate limits misbehaved.Second defect, found while verifying
Fixing the allowlist hid the nav item, but the dashboard still issued the doomed request.
dashboardDataFetches()startsfetchWorkflowsPage()(which loads the flags) andfetchRateLimitsPage()in the same eager batch, so the gate read an empty config and defaulted totrue. A race, not a logic error.fetchBudgetsPage()had the identical latent defect.Fixed at the source:
fetchWorkflowRuntimeConfig()now shares its in-flight request, and a newensureWorkflowRuntimeConfig()lets a feature gate await the flags instead of racing them. Both gates await it. Side effect: collapses the duplicate/admin/runtime/configGET that overlapping callers used to issue.Guard against recurrence
The JS allowlist silently duplicates
DashboardConfigResponse's json tags — that drift is this bug.internal/admin/dashboard_config_contract_test.goreflects over the struct, parses the allowlist out ofworkflows.js, and fails in both directions (backend emits a key the JS drops; JS allowlists a key the backend never sends).Verification
Driven against a real running gateway in headless Chrome (CDP), reading the actual DOM:
/admin/rate-limitscallsRATE_LIMITS_ENABLED=falsedisplay: none)RATE_LIMITS_ENABLED=trueWith the feature off, also confirmed: the deep-linked page shows "Rate limit management is unavailable.", the Create button and models-page gauge buttons are hidden, and
/admin/runtime/configis fetched exactly once.Each new test was confirmed non-vacuous by reverting the corresponding fix and watching it fail with the right diagnosis.
go build ./...,go vet,make test-race,make lint, and 446 dashboard JS tests pass.User-visible impact
With
RATE_LIMITS_ENABLED=false, the Rate Limits nav item is hidden and the dashboard no longer calls the disabled endpoint. No behavior change when the feature is enabled. No config, API, or provider behavior changes.Notes on scope
Two things deliberately left alone:
/admin/dashboard/rate-limitsstill renders the page shell with an "unavailable" warning rather than redirecting — this matches Budgets and Guardrails, so it stays consistent.trueuntil the config lands), so there is a sub-100ms window on first paint where the item can flash before hiding. That is the pre-existing trade-off shared with the other two flags: a config-fetch failure shows working features rather than hiding them. Changing it would be a broader UX decision than this fix.🤖 Generated with Claude Code
Summary by CodeRabbit