fix(dashboard): make auth prompt visible and resilient#229
Merged
SantiagoDePolonia merged 12 commits intomainfrom Apr 14, 2026
Merged
fix(dashboard): make auth prompt visible and resilient#229SantiagoDePolonia merged 12 commits intomainfrom
SantiagoDePolonia merged 12 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 45 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthrough📝 WalkthroughAdded a modal auth dialog and CSS, centralized request options and auth-generation tagging for fetches, implemented stale-auth suppression logic, normalized/persisted API keys, updated many fetch call sites to use the new request flow, and extended tests to cover auth and request-cancellation behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as Dashboard UI
participant LS as LocalStorage
participant Fetch as FetchLayer
participant Server
User->>UI: Click "Enter API key" (openAuthDialog)
UI->>UI: focus `#authDialogApiKey`
User->>UI: Submit API key (submitApiKey)
UI->>UI: normalizeApiKey(), increment authRequestGeneration
UI->>LS: save normalized key (gomodel_api_key)
UI->>Fetch: build request via requestOptions(headers(), authGeneration)
Fetch->>Server: fetch(..., Authorization: Bearer token, authGeneration)
Server-->>Fetch: 401 Unauthorized (or OK)
Fetch->>UI: handleFetchResponse(res, label, request)
alt response authGeneration < current
UI->>UI: return staleAuthResponseResult (ignore)
else response is 401 and current
UI->>UI: set authError/needsAuth, open auth dialog
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 7
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/contribution-calendar.js (1)
10-25:⚠️ Potential issue | 🟠 MajorConstruct
optionsinsidetryso abort-controller cleanup is guaranteed.A throw from
this.requestOptions()/this.headers()on Line 22 skipsfinally, which can leave_calendarFetchControlleruncleared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/js/modules/contribution-calendar.js` around lines 10 - 25, The options object is built before the try block so if this.requestOptions() or this.headers() throws the finally cleanup for the abort controller can be skipped; move creation of options (the call to requestOptions() or headers()) and the assignment of options.signal into the try block that surrounds the fetch/async work so that _calendarFetchController is always cleared in the finally; keep the existing logic that obtains controller via this._startAbortableRequest('_calendarFetchController') and the isCurrentRequest helper that uses this._isCurrentAbortableRequest or compares this._calendarFetchController and controller.signal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 482-490: handleFetchResponse currently returns false for both real
401 errors and stale auth responses, causing callers like fetchCategories and
fetchExecutionPlans to treat late stale 401s as real failures and clear state;
change handleFetchResponse (and its callers) to distinguish stale responses by
returning/throwing a distinct sentinel (e.g. return "STALE_AUTH" or throw a
StaleAuthError) when isStaleAuthResponse(request) is true, update callers to
check for that sentinel (or catch StaleAuthError) and early-return without
mutating state (do not clear this.categories / this.executionPlans when the
response is stale), and keep the existing behavior for genuine 401s.
- Around line 310-316: submitApiKey() currently treats an empty/whitespace
apiKey as a successful unlock; before calling saveApiKey(), incrementing
authRequestGeneration, closing the dialog, or calling fetchAll(), validate the
stored/entered apiKey (from saveApiKey()/this.apiKey) and if it's empty or only
whitespace, do not proceed—keep the dialog open (do not call closeAuthDialog()),
do not increment authRequestGeneration, and set authError (or another UI flag)
so the user sees an error; only on a non-blank trimmed apiKey call saveApiKey(),
increment authRequestGeneration, clear authError/needsAuth, closeAuthDialog(),
and then call fetchAll().
In `@internal/admin/dashboard/static/js/modules/aliases.js`:
- Around line 150-154: In fetchAliases (and the similar block around lines
180–184), move construction of the request object (the call to
this.requestOptions() or headers via this.headers()) inside the try block so any
exceptions thrown during requestOptions()/headers() are caught; ensure
aliasLoading and aliasError are set/reset in the finally or catch so
aliasLoading cannot remain true after an error, referencing the fetchAliases
function, this.requestOptions, this.headers, aliasLoading and aliasError to
locate the changes.
In `@internal/admin/dashboard/static/js/modules/auth-keys.js`:
- Around line 90-94: The code sets this.authKeysLoading = true then constructs
request = typeof this.requestOptions === 'function' ? this.requestOptions() : {
headers: this.headers() } outside the try, so if this.requestOptions() or
this.headers() throws the finally won’t run and authKeysLoading can remain true;
move the request construction into the try block that wraps the fetch call (the
block containing fetch('/admin/api/v1/auth-keys', request)) so any exceptions
from this.requestOptions()/this.headers() are caught and the existing finally
will always execute to reset this.authKeysLoading and handle this.authKeyError.
In `@internal/admin/dashboard/static/js/modules/guardrails.js`:
- Around line 250-253: The guardrail fetch paths currently set
this.guardrailTypesLoading and build const request = typeof this.requestOptions
=== 'function' ? this.requestOptions() : { headers: this.headers() }; outside
the try block, so if requestOptions() or this.headers() throws the finally block
won't run and the loading flags (e.g., this.guardrailTypesLoading) can remain
true; move the request construction into the try in both the guardrail types
fetch (around fetch('/admin/api/v1/guardrails/types', request)) and the other
guardrail fetch path (the block around lines referencing requestOptions/headers
and fetch at the 282–286 area) so any exceptions during request creation are
caught and the finally handlers always execute to clear loading state.
In `@internal/admin/dashboard/static/js/modules/providers.js`:
- Around line 65-75: The code constructs request options before entering the try
block which can throw (e.g., in requestOptions() or headers()) and bypass the
finally that clears _providerStatusFetchController; move the options
construction so the controller is acquired first via
this._startAbortableRequest('_providerStatusFetchController') and then build
options (calling this.requestOptions() or this.headers()) inside the try block
(or ensure the try begins before calling requestOptions()/headers()), and keep
the existing logic that sets options.signal when controller exists and that
calls this.handleFetchResponse; this ensures _providerStatusFetchController is
always cleared in the finally.
In `@internal/admin/dashboard/static/js/modules/usage.js`:
- Around line 40-46: The request-options and header construction (calls to
this.requestOptions() / this.headers()) and the creation of the abort controller
(this._startAbortableRequest('_cacheOverviewFetchController')) are executed
before the try blocks, so any exception there skips the finally cleanup; move
the controller creation and evaluation of requestOptions()/headers() inside the
try block for each fetcher (the five spots that use this._startAbortableRequest,
requestOptions, and headers), only assign options.signal if controller exists
after those calls, and keep the existing finally to always call
controller.abort()/this._stopAbortableRequest so the abort-controller cleanup
runs even if requestOptions()/headers() throw.
---
Outside diff comments:
In `@internal/admin/dashboard/static/js/modules/contribution-calendar.js`:
- Around line 10-25: The options object is built before the try block so if
this.requestOptions() or this.headers() throws the finally cleanup for the abort
controller can be skipped; move creation of options (the call to
requestOptions() or headers()) and the assignment of options.signal into the try
block that surrounds the fetch/async work so that _calendarFetchController is
always cleared in the finally; keep the existing logic that obtains controller
via this._startAbortableRequest('_calendarFetchController') and the
isCurrentRequest helper that uses this._isCurrentAbortableRequest or compares
this._calendarFetchController and controller.signal.
🪄 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: de0a288b-c270-478d-9f6f-1e6f8dd9d70f
📒 Files selected for processing (15)
internal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/aliases.jsinternal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/auth-keys.jsinternal/admin/dashboard/static/js/modules/contribution-calendar.jsinternal/admin/dashboard/static/js/modules/conversation-drawer.jsinternal/admin/dashboard/static/js/modules/dashboard-display.test.jsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.jsinternal/admin/dashboard/static/js/modules/execution-plans.jsinternal/admin/dashboard/static/js/modules/guardrails.jsinternal/admin/dashboard/static/js/modules/providers.jsinternal/admin/dashboard/static/js/modules/usage.jsinternal/admin/dashboard/templates/auth-banner.htmlinternal/admin/dashboard/templates/layout.html
# Conflicts: # internal/admin/dashboard/static/js/modules/workflows.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 270-274: normalizeApiKey currently returns the literal "Bearer"
when given "Bearer" or "Bearer "; change normalizeApiKey to explicitly treat a
bare Bearer scheme as empty (e.g., if key matches /^Bearer\s*$/i return ''),
otherwise extract the token as before, and add a regression test asserting
normalizeApiKey('Bearer') and normalizeApiKey('Bearer ') both return '' and
that submitApiKey()/headers() only emit an Authorization header when the
normalized key is non-empty; reference normalizeApiKey, submitApiKey, and
headers when making these changes.
In `@internal/admin/dashboard/static/js/modules/aliases.js`:
- Around line 153-161: The write-path functions toggleAliasEnabled,
submitAliasForm, submitModelOverrideForm, and deleteModelOverride still send
bare headers() and set needsAuth unconditionally on 401; update each to
construct the request the same way as fetchAliases/fetchModelOverrides (const
request = typeof this.requestOptions === 'function' ? this.requestOptions() : {
headers: this.headers() };), use that request in the fetch call, and pass the
request into handleFetchResponse(res, ...) so that auth handling is
generation-aware; remove the unconditional this.needsAuth = true on 401 and let
handleFetchResponse decide/return whether auth is needed.
In `@internal/admin/dashboard/static/js/modules/dashboard-layout.test.js`:
- Around line 187-190: Remove the four negative assertions that forbid dialog
semantics and instead assert the dialog's required behavior: delete the
assert.doesNotMatch lines that reference aria-describedby, the two helper
sentences, and the <label> for "authDialogApiKey", and replace them with
positive checks on the template (the same template variable used in this test)
that verify the API-key input and accompanying guidance are present (e.g.,
assert.match(template, /id="authDialogApiKey"/ and assert.match(template, /Enter
a different API key for this browser\./) or similar), so accessibility
attributes like aria-describedby or an explicit <label> are allowed.
In `@internal/admin/dashboard/static/js/modules/providers.js`:
- Around line 74-79: After awaiting fetch('/admin/api/v1/providers/status',
options) check whether the request was aborted (e.g., if options.signal &&
options.signal.aborted) and return early before calling
this.handleFetchResponse; this prevents handling a stale/aborted response and
avoids later branches (the !handled branch that can reopen auth or clear
providerStatus) from running. Update the sequence around the fetch so the abort
check occurs immediately after the fetch resolves and before invoking
handleFetchResponse (reference: options, handleFetchResponse,
isStaleAuthFetchResult).
In `@internal/admin/dashboard/static/js/modules/usage.js`:
- Around line 116-123: The early-return on failed live fetches leaves stale UI
data; after determining neither summaryHandled nor dailyHandled is truthy (but
before returning), reset this.summary and this.daily to the empty usage state
(e.g., this.summary = { total: 0, ... } and this.daily = [] or whatever the
component's empty shape is) and trigger a rerender (call the component's render
method, e.g., this.render()) while preserving the existing STALE_AUTH check via
isStaleAuthFetchResult and leaving that early return unchanged; update the block
around handleFetchResponse, summaryHandled, dailyHandled and
isStaleAuthFetchResult to perform the reset+render on real failures before
returning.
🪄 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: 7f276a06-b0b3-489c-b564-ed08466c95d3
📒 Files selected for processing (14)
internal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/aliases.jsinternal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/auth-keys.jsinternal/admin/dashboard/static/js/modules/contribution-calendar.jsinternal/admin/dashboard/static/js/modules/conversation-drawer.jsinternal/admin/dashboard/static/js/modules/dashboard-display.test.jsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.jsinternal/admin/dashboard/static/js/modules/guardrails.jsinternal/admin/dashboard/static/js/modules/providers.jsinternal/admin/dashboard/static/js/modules/usage.jsinternal/admin/dashboard/static/js/modules/workflows.jsinternal/admin/dashboard/templates/layout.html
Comment on lines
+270
to
+274
| normalizeApiKey(value) { | ||
| const key = String(value || '').trim(); | ||
| const match = key.match(/^Bearer\s+(.+)$/i); | ||
| return match ? match[1].trim() : key; | ||
| }, |
Contributor
There was a problem hiding this comment.
Reject a bare Bearer scheme as an empty key.
normalizeApiKey('Bearer') currently returns 'Bearer', so submitApiKey() accepts it and headers() emits Authorization: Bearer Bearer. That still breaks the pasted-token flow for incomplete header values. Normalize Bearer/Bearer to '' and add a regression test for that input.
🔧 Suggested fix
normalizeApiKey(value) {
const key = String(value || '').trim();
- const match = key.match(/^Bearer\s+(.+)$/i);
- return match ? match[1].trim() : key;
+ const match = key.match(/^Bearer(?:\s+(.*))?$/i);
+ return match ? String(match[1] || '').trim() : key;
},📝 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
| normalizeApiKey(value) { | |
| const key = String(value || '').trim(); | |
| const match = key.match(/^Bearer\s+(.+)$/i); | |
| return match ? match[1].trim() : key; | |
| }, | |
| normalizeApiKey(value) { | |
| const key = String(value || '').trim(); | |
| const match = key.match(/^Bearer(?:\s+(.*))?$/i); | |
| return match ? String(match[1] || '').trim() : key; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/static/js/dashboard.js` around lines 270 - 274,
normalizeApiKey currently returns the literal "Bearer" when given "Bearer" or
"Bearer "; change normalizeApiKey to explicitly treat a bare Bearer scheme as
empty (e.g., if key matches /^Bearer\s*$/i return ''), otherwise extract the
token as before, and add a regression test asserting normalizeApiKey('Bearer')
and normalizeApiKey('Bearer ') both return '' and that
submitApiKey()/headers() only emit an Authorization header when the normalized
key is non-empty; reference normalizeApiKey, submitApiKey, and headers when
making these changes.
internal/admin/dashboard/static/js/modules/dashboard-layout.test.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/static/js/modules/dashboard-layout.test.js`:
- Line 158: The test assertion using assert.doesNotMatch(template, /<input
id="apiKey"/) is attribute-order dependent and can miss inputs where id is not
the first attribute; update the assertion in dashboard-layout.test.js to use an
order-agnostic regex that looks for an <input> element containing an id="apiKey"
anywhere in its attribute list (e.g. match
<input\b[^>]*\bid\s*=\s*["']?apiKey["']?) so the check on the variable template
reliably detects any legacy input with id apiKey.
🪄 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: 88411a1d-2d24-4044-abe5-eb2c0092ef09
📒 Files selected for processing (9)
internal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/aliases.jsinternal/admin/dashboard/static/js/modules/aliases.test.jsinternal/admin/dashboard/static/js/modules/dashboard-display.test.jsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.jsinternal/admin/dashboard/static/js/modules/providers.jsinternal/admin/dashboard/static/js/modules/providers.test.jsinternal/admin/dashboard/static/js/modules/request-cancellation.test.jsinternal/admin/dashboard/static/js/modules/usage.js
| const template = readFixture('../../../templates/layout.html'); | ||
| const css = readFixture('../../css/dashboard.css'); | ||
|
|
||
| assert.doesNotMatch(template, /<input id="apiKey"/); |
Contributor
There was a problem hiding this comment.
Make the legacy input absence check attribute-order agnostic.
Line 158 can miss <input ... id="apiKey" ...> when id is not the first attribute, so the test may falsely pass.
Suggested fix
- assert.doesNotMatch(template, /<input id="apiKey"/);
+ assert.doesNotMatch(template, /<input[^>]*\bid="apiKey"\b/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/static/js/modules/dashboard-layout.test.js` at line
158, The test assertion using assert.doesNotMatch(template, /<input
id="apiKey"/) is attribute-order dependent and can miss inputs where id is not
the first attribute; update the assertion in dashboard-layout.test.js to use an
order-agnostic regex that looks for an <input> element containing an id="apiKey"
anywhere in its attribute list (e.g. match
<input\b[^>]*\bid\s*=\s*["']?apiKey["']?) so the check on the variable template
reliably detects any legacy input with id apiKey.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/admin/dashboard/static/js/modules/dashboard-layout.test.js (1)
158-158:⚠️ Potential issue | 🟡 MinorMake legacy API-key input absence check attribute-order agnostic.
Line 158 can miss
<input ... id="apiKey" ...>whenidis not first, leading to false passes.Suggested fix
- assert.doesNotMatch(template, /<input id="apiKey"/); + assert.doesNotMatch(template, /<input\b[^>]*\bid\s*=\s*["']apiKey["'][^>]*>/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/js/modules/dashboard-layout.test.js` at line 158, The test currently looks for the legacy API-key input by matching the literal sequence with id immediately after the input tag, which fails if attributes are reordered; update the assertion in dashboard-layout.test.js (the assert.doesNotMatch call that inspects template) to use an order-agnostic pattern that matches an input element with any attributes followed by an id="apiKey" attribute (i.e., match an input tag, allow any attributes, then require the id attribute), so the check passes regardless of attribute order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/admin/dashboard/static/js/modules/dashboard-layout.test.js`:
- Line 158: The test currently looks for the legacy API-key input by matching
the literal sequence with id immediately after the input tag, which fails if
attributes are reordered; update the assertion in dashboard-layout.test.js (the
assert.doesNotMatch call that inspects template) to use an order-agnostic
pattern that matches an input element with any attributes followed by an
id="apiKey" attribute (i.e., match an input tag, allow any attributes, then
require the id attribute), so the check passes regardless of attribute order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24d5983b-9d69-445f-8f4a-3f4dbd79def8
📒 Files selected for processing (14)
internal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/modules/dashboard-layout.test.jsinternal/app/app_test.gointernal/authkeys/service_test.gointernal/cache/modelcache/local_test.gointernal/cache/modelcache/redis_test.gointernal/httpclient/client_test.gointernal/llmclient/client_test.gointernal/modeldata/enricher_test.gointernal/modeldata/fetcher_test.gointernal/modeldata/merger_test.gointernal/observability/metrics_test.gointernal/providers/anthropic/anthropic_test.gointernal/providers/anthropic/passthrough_semantics_test.go
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
1 participant
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
Style
Tests