fix(gui): dedupe dashboard API token prompts on concurrent 401s - #651
Conversation
Re-read the in-memory token after a 401 and share one refresh gate so a management fan-out prompts once instead of once per in-flight /api request when OPENCODEX_API_AUTH_TOKEN is required. Fixes lidge-jun#647.
📝 WalkthroughWalkthroughThe API authentication wrapper coordinates concurrent 401 responses through shared in-memory token state, prevents stale requests from clearing newer tokens, suppresses repeated prompts after cancellation, and retries requests once. Three tests cover prompt deduplication, token races, and cancellation fan-out. ChangesAPI token refresh coordination
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ConcurrentRequests
participant installApiAuthFetch
participant resolveTokenAfter401
participant window.prompt
participant originalFetch
ConcurrentRequests->>installApiAuthFetch: send API requests
installApiAuthFetch->>originalFetch: issue requests
originalFetch-->>installApiAuthFetch: return concurrent 401 responses
installApiAuthFetch->>resolveTokenAfter401: resolve shared token
resolveTokenAfter401->>window.prompt: prompt once when needed
window.prompt-->>resolveTokenAfter401: return token or null
resolveTokenAfter401-->>installApiAuthFetch: provide shared result
installApiAuthFetch->>originalFetch: retry requests once
originalFetch-->>ConcurrentRequests: return final statuses
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Only clear memory when it still holds the failed token, so a sibling request that already stored a valid token is not erased before the refresh retry.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc6d55700d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const refreshed = readToken(); | ||
| if (refreshed && refreshed !== token) { |
There was a problem hiding this comment.
When concurrent requests were sent with an already-cached stale token, the first 401 can clear it, prompt once, and store the replacement before a later stale 401 resumes. That later callback executes clearToken() immediately before this re-read, deleting the replacement, so refreshed is always null and another prompt opens. Guard token clearing with readToken() === token (including the retry paths) so a late failure cannot erase a newer token.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2bee4af / tip: clearTokenIfCurrent only clears when memory still holds the failed token, so a concurrent refresh is preserved. Covered by the new stale concurrent 401 regression test.
| })().finally(() => { | ||
| promptInFlight = null; | ||
| }); |
There was a problem hiding this comment.
When the dashboard starts without a token and the user cancels or submits whitespace, window.prompt blocks the browser thread so staggered 401 continuations generally cannot join the gate while it is active. Clearing the gate immediately after that null result makes every remaining request open its own prompt, so dismissing one dialog can still produce the full dashboard fan-out of dialogs. Track the completed auth-attempt generation for requests from the same fan-out while still allowing a later, newly initiated request to prompt again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 92e9677: promptCancelled suppresses further prompts for the rest of the page lifetime after cancel/blank. Covered by the cancel fan-out regression test.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/api.ts (1)
86-104: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnconditional
clearToken()calls race with concurrent token refreshes — can clobber a good token another request just stored.Three sites here (Line 86, Line 94, Line 102) call
clearToken()based only on the locally-captured stale token being truthy, without re-checking whether the sharedmemoryTokenstill equals that stale value. This is a classic TOCTOU/lost-update: between the moment a slow request capturedtokenand the moment its 401 response arrives, a concurrent request may have already prompted and stored a fresh, valid token. The slow request's unconditionalclearToken()then wipes out that good token, forcing yet another prompt — reintroducing exactly the prompt-storm behavior this PR (#647) is meant to fix, just under a narrower race window (existing token invalidation with concurrent in-flight requests, rather than the initial no-token bootstrap case the new test covers).Concretely: request A gets a 401 on stale token
T1, clears it, prompts, and storesT2. If request B (also carrying staleT1) resumes its own 401 handling after A storedT2, B'sif (token) clearToken()at Line 86 wipesT2back tonull, and B goes on to prompt again sincereadToken()now returnsnull.Guard each clear against the current in-memory value:
🔒 Proposed fix — only clear if memoryToken still matches the token that just failed
- if (token) clearToken(); + if (token && readToken() === token) clearToken(); // Another request may have stored a token while this one was in flight (or while prompt blocked). const refreshed = readToken(); if (refreshed && refreshed !== token) { const [retryInput, retryInit] = withToken(input, init, refreshed); const retry = await originalFetch(retryInput, retryInit); if (retry.status !== 401) return retry; - clearToken(); + if (readToken() === refreshed) clearToken(); } const nextToken = await resolveTokenAfter401(token); if (!nextToken) return response; const [retryInput, retryInit] = withToken(input, init, nextToken); const retry = await originalFetch(retryInput, retryInit); - if (retry.status === 401) clearToken(); + if (retry.status === 401 && readToken() === nextToken) clearToken(); return retry;Worth adding a regression test for this scenario (existing token invalidated while a second concurrent request is still holding the stale value) since the current concurrency test only exercises the no-token bootstrap path.
🤖 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 `@gui/src/api.ts` around lines 86 - 104, Guard every clearToken call in the 401 retry flow around resolveTokenAfter401 so it only clears when the shared current token still equals the locally captured failed token. Apply this check to the initial clear, the refreshed-token retry failure, and the final retry failure, preserving the existing retry behavior while preventing a concurrent refresh from being erased.
🤖 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 `@gui/src/api.ts`:
- Line 62: Update the API token prompt in the authentication flow to use the
existing i18n translation key for the provider alias prompt instead of the
hardcoded “OpenCodex API token” text. Thread the translated string into the code
around the prompted token value, following the established t(...) usage in
CodexAccountPool and useProviderAccountPools.
---
Outside diff comments:
In `@gui/src/api.ts`:
- Around line 86-104: Guard every clearToken call in the 401 retry flow around
resolveTokenAfter401 so it only clears when the shared current token still
equals the locally captured failed token. Apply this check to the initial clear,
the refreshed-token retry failure, and the final retry failure, preserving the
existing retry behavior while preventing a concurrent refresh from being erased.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e3f39e2c-2bed-4406-a340-88dd7856cb0d
📒 Files selected for processing (2)
gui/src/api.tsgui/tests/api-auth-memory.test.ts
If the user dismisses the dashboard token dialog, remember the cancel for this page load so staggered 401 continuations do not reopen the prompt for every remaining /api request.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 51 seconds. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@gui/src/api.ts`:
- Around line 4-9: Confirm that the permanent page-lifetime behavior of
promptCancelled is intentional UX: after cancellation or blank submission,
subsequent /api/* 401 responses must not reopen the prompt until a full reload.
If this is not desired, revise the cancellation state and related prompt flow to
provide an in-app retry path while preserving fan-out suppression.
In `@gui/tests/api-auth-memory.test.ts`:
- Around line 180-236: Extract the duplicated gate polling and release/settle
logic from the affected concurrency tests into shared helpers, such as
waitForGateCount and releaseAndSettleAll. Reuse those helpers in the current
test and the preceding related test, preserving the existing polling limits,
release order, settlement assertions, and test behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 5effb695-ebb1-4cf1-a786-02e225ac4d5d
📒 Files selected for processing (2)
gui/src/api.tsgui/tests/api-auth-memory.test.ts
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Maintainer review — merge-ready (cannot self-APPROVE; not merging)
Issue: #647 — dashboard opened one window.prompt per concurrent /api/* 401 under OPENCODEX_API_AUTH_TOKEN. Not fixed on main or prior dev tip; no screenshots on the issue.
Fix (tip 96726a18): shared resolveTokenAfter401 gate, re-read memoryToken, clearTokenIfCurrent, promptCancelled after cancel/blank.
Bot findings addressed:
- Codex P2 clear-token race → fixed + regression test
- Codex P2 cancel fan-out → fixed + regression test
- CodeRabbit critical clearToken → fixed; tip re-review only trivial UX/DRY nits (acknowledged / skipped)
- CodeRabbit i18n on pre-existing prompt string → out of scope; Rabbit withdrew
- Codex tip re-review blocked by usage limits; earlier findings verified fixed on tip
Subagent reviews: security — no medium+; defect review — no actionable defects.
CI: all checks green on tip (ubuntu/macos/windows + npm-global + react-doctor + label + CodeRabbit + enforce-target).
Leave unmerged per request.
|
Ready to merge. |
Carries #651. GUI-only — no Go counterpart.
|
Transition complete:
|
1 participant
Summary
OPENCODEX_API_AUTH_TOKENrequired), the dashboard no longer opens onewindow.promptper in-flight/api/*401.sessionStoragewrite) and cross-origin token isolation.Test plan
cd gui && bun test tests/api-auth-memory.test.ts— concurrent 401 fan-out expects exactly 1 prompt and all 200scd gui && bun run lint && bun run buildSummary by CodeRabbit