feat: redesign OAuth success screen#25
Conversation
📝 Walkthroughwalkthroughoauth success page replaced with a centered card UI, css vars and system font updated, status steps simplified, callback summary added; file copy helper now retries on transient EBUSY with backoff and tests added for retry behavior. ( changes
sequence diagram(s)sequenceDiagram
participant caller as Caller
participant helper as copyWithRetry
participant fs as node:fs/promises
participant timer as sleep
caller->>helper: copyWithRetry(src, dest)
helper->>fs: copyFile(src, dest)
alt success
fs-->>helper: resolved
helper-->>caller: resolved
else EBUSY error
fs-->>helper: reject EBUSY
helper->>timer: sleep(backoffMs * attempt)
timer-->>helper: awake
helper->>fs: copyFile(src, dest) // retry
fs-->>helper: resolved
helper-->>caller: resolved
end
estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes flag: missing regression tests — no visual or screenshot regression test was added for 🚥 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)
Comment |
![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)
lib/oauth-success.html (1)
1-269:⚠️ Potential issue | 🟡 Minortest/copy-oauth-success.test.ts covers file copying, not the redesign itself.
test/copy-oauth-success.test.ts:12-30 only verifies the html gets copied to the destination folder. it doesn't snapshot or validate the markup changes. with a 269-line html redesign (new color scheme, animation, responsive layout, icon markup), add a snapshot test in test/copy-oauth-success.test.ts to catch visual regressions. verify the test runs as part of
npm run testor the ci pipeline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/oauth-success.html` around lines 1 - 269, Add a snapshot assertion to the existing test/copy-oauth-success.test.ts so it validates the full markup of lib/oauth-success.html after copy: read the copied file (the same path already used in the test), normalize line endings if needed, and add expect(fileContents).toMatchSnapshot() inside the existing describe/it block (or create a new it called "copies oauth-success.html and matches snapshot"); ensure the snapshot is committed and that the test file still runs under the project's npm test/CI command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/oauth-success.html`:
- Around line 246-248: The oauth success page currently claims "Credentials
saved" but lib/auth/server.ts responds immediately on callback; update the
callback flow to wait for the token exchange and persistence to complete (call
the authorization-code exchange routine in lib/codex-manager.ts and await
persistAccountPool completion) before returning lib/oauth-success.html, or
alternatively serve a neutral message that does not claim persistence until
persistAccountPool finishes; also add a regression test that simulates the OAuth
callback, awaits persistAccountPool, and asserts the credentials file exists/was
written before the HTTP response is returned. Ensure you modify the server
handler (the callback function in lib/auth/server.ts) to await the
exchange/persist functions (references: the token exchange function around
lib/codex-manager.ts:1249 and persistAccountPool at lib/codex-manager.ts:3632)
so the response only occurs after success or change the page text if you cannot
await persistence.
---
Outside diff comments:
In `@lib/oauth-success.html`:
- Around line 1-269: Add a snapshot assertion to the existing
test/copy-oauth-success.test.ts so it validates the full markup of
lib/oauth-success.html after copy: read the copied file (the same path already
used in the test), normalize line endings if needed, and add
expect(fileContents).toMatchSnapshot() inside the existing describe/it block (or
create a new it called "copies oauth-success.html and matches snapshot"); ensure
the snapshot is committed and that the test file still runs under the project's
npm test/CI command.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
lib/oauth-success.html
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/oauth-success.html
🔇 Additional comments (4)
lib/oauth-success.html (4)
196-200: good: prefers-reduced-motion respected.nice touch disabling the fade animation for users who request reduced motion. accessible and follows wcag guidelines.
228-228: aria labels present on key landmarks.
role="main"andaria-labelattributes on the status list and callback summary improve screen reader navigation. lgtm.Also applies to: 237-237, 255-255
8-8: 🧹 Nitpick | 🔵 Trivialexternal font import adds latency and tracking exposure.
loading
fonts.googleapis.comat runtime introduces a blocking request and sends user metadata to google. consider self-hosting space grotesk or using a system font stack to avoid the external dependency.♻️ proposed fallback to system fonts
- `@import` url('https://fonts.googleapis.com/css2?family=Space+Grotesk:wght@400;500;600&display=swap'); + /* self-host Space Grotesk or rely on system stack */then update the
font-familydeclarations to remove the dependency:font-family: "Segoe UI", system-ui, -apple-system, sans-serif;Likely an incorrect or invalid review comment.
257-259: the callback address is not a bug - port 1455 is fixed by design.port 1455 is not configurable at runtime. AGENTS.md:89 explicitly forbids hardcoding ports other than 1455 for the oauth server, and the REDIRECT_URI constant in lib/auth/auth.ts:11 is hardcoded to
http://127.0.0.1:1455/auth/callback. the html at lib/oauth-success.html:258 correctly reflects the address the server binds to in lib/auth/server.ts:78.windows port conflicts are already handled: lib/auth/server.ts:102-115 catches bind errors and falls back to manual paste mode with
ready: false. no templating or injection is needed.Likely an incorrect or invalid review comment.
| <li> | ||
| <span>Credentials saved</span> | ||
| <span class="ok">stored locally</span> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for references to oauth-success page
rg -n 'oauth-success' --type ts --type js -C 3Repository: ndycode/codex-multi-auth
Length of output: 2517
🏁 Script executed:
#!/bin/bash
# Also search for success page patterns and handler functions
rg -n 'success.*html|\.html.*success' --type ts --type js -C 3 | head -100Repository: ndycode/codex-multi-auth
Length of output: 2061
🏁 Script executed:
#!/bin/bash
# Look for credential storage/persistence patterns and EBUSY handling
rg -n 'credential.*stor|persist|EBUSY|write.*fail' --type ts --type js -C 2 | head -100Repository: ndycode/codex-multi-auth
Length of output: 5498
🏁 Script executed:
#!/bin/bash
# Look at the full server.ts to see how successHtml is used in the response
cat -n lib/auth/server.ts | head -150Repository: ndycode/codex-multi-auth
Length of output: 5392
🏁 Script executed:
#!/bin/bash
# Search for where the oauth callback stores/persists credentials
rg -n 'callback|persist|credential' lib/auth/server.ts -A 5 -B 2Repository: ndycode/codex-multi-auth
Length of output: 1329
🏁 Script executed:
#!/bin/bash
# Look for tests that verify the oauth success flow
rg -n 'oauth.*callback|auth.*success' test/ --type ts -C 3 | head -100Repository: ndycode/codex-multi-auth
Length of output: 1975
🏁 Script executed:
#!/bin/bash
# Find where startLocalOAuthServer is called and how credentials are stored
rg -n 'startLocalOAuthServer|waitForCode' lib/ --type ts -C 5Repository: ndycode/codex-multi-auth
Length of output: 5465
🏁 Script executed:
#!/bin/bash
# Look for OAuth flow that exchanges code for tokens and stores credentials
rg -n 'exchange.*code|token.*stor|credential.*sav' lib/ --type ts -C 3Repository: ndycode/codex-multi-auth
Length of output: 3217
🏁 Script executed:
#!/bin/bash
# Check for tests that verify the full oauth flow including storage
rg -n 'startLocalOAuthServer|oauth.*flow|oauth.*stor' test/ --type ts -C 5Repository: ndycode/codex-multi-auth
Length of output: 14860
🏁 Script executed:
#!/bin/bash
# Look at what exchangeAuthorizationCode returns and how it's handled
cat -n lib/auth/auth.ts | head -200Repository: ndycode/codex-multi-auth
Length of output: 7573
🏁 Script executed:
#!/bin/bash
# Search for where the oauth result (tokens) is actually stored to disk
rg -n 'exchangeAuthorizationCode|runOAuthFlow' lib/codex-manager.ts -A 10 | head -80Repository: ndycode/codex-multi-auth
Length of output: 2260
🏁 Script executed:
#!/bin/bash
# Check if there are tests verifying credential persistence in oauth flow
rg -n 'test.*oauth|oauth.*test' test/ --type ts -l | head -5Repository: ndycode/codex-multi-auth
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Look at the full runOAuthFlow and persistAccountPool flow to confirm sequence
cat -n lib/codex-manager.ts | sed -n '1188,1250p'Repository: ndycode/codex-multi-auth
Length of output: 2339
🏁 Script executed:
#!/bin/bash
# Check the main login flow that calls runOAuthFlow then persistAccountPool
cat -n lib/codex-manager.ts | sed -n '3617,3635p'Repository: ndycode/codex-multi-auth
Length of output: 855
🏁 Script executed:
#!/bin/bash
# Look for any test that verifies credential persistence happens before showing success
rg -n 'persistAccountPool|persist.*credential' test/ --type ts -C 3 | head -60Repository: ndycode/codex-multi-auth
Length of output: 373
ensure the server waits for credential persistence before serving oauth-success.html.
the success page shown at lib/oauth-success.html:247-248 claims "credentials saved" and "stored locally" but is served at lib/auth/server.ts:60 immediately when a valid callback arrives. this happens before:
- lib/codex-manager.ts:1249 exchanges the authorization code for tokens
- lib/codex-manager.ts:3632 persists credentials to disk via persistAccountPool
on windows, the fs write inside persistAccountPool could fail with ebusy/transient errors that aren't caught in the oauth callback path, or the write could still be in-flight when the user closes the browser. the ui message then misleads the user into thinking their credentials are safe when they may not be persisted.
fix: move the success page response to after credential persistence completes, or change the message to not claim storage until it's actually confirmed. add a regression test verifying credentials are written before responding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/oauth-success.html` around lines 246 - 248, The oauth success page
currently claims "Credentials saved" but lib/auth/server.ts responds immediately
on callback; update the callback flow to wait for the token exchange and
persistence to complete (call the authorization-code exchange routine in
lib/codex-manager.ts and await persistAccountPool completion) before returning
lib/oauth-success.html, or alternatively serve a neutral message that does not
claim persistence until persistAccountPool finishes; also add a regression test
that simulates the OAuth callback, awaits persistAccountPool, and asserts the
credentials file exists/was written before the HTTP response is returned. Ensure
you modify the server handler (the callback function in lib/auth/server.ts) to
await the exchange/persist functions (references: the token exchange function
around lib/codex-manager.ts:1249 and persistAccountPool at
lib/codex-manager.ts:3632) so the response only occurs after success or change
the page text if you cannot await persistence.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <meta name="viewport" content="width=device-width,initial-scale=1" /> | ||
| <title>OpenAI Codex Sign-in Complete</title> | ||
| <style> | ||
| @import url('https://fonts.googleapis.com/css2?family=Space+Grotesk:wght@400;500;600&display=swap'); |
There was a problem hiding this comment.
google fonts import blocked by csp in lib/auth/server.ts:57-59. the server sets font-src 'self' data: which prevents loading from fonts.googleapis.com or fonts.gstatic.com. fonts won't load; browser will fall back to ibm plex sans / segoe ui.
| @import url('https://fonts.googleapis.com/css2?family=Space+Grotesk:wght@400;500;600&display=swap'); | |
| /* Space Grotesk not available - using fallback fonts only */ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/oauth-success.html
Line: 8
Comment:
google fonts import blocked by csp in `lib/auth/server.ts:57-59`. the server sets `font-src 'self' data:` which prevents loading from `fonts.googleapis.com` or `fonts.gstatic.com`. fonts won't load; browser will fall back to ibm plex sans / segoe ui.
```suggestion
/* Space Grotesk not available - using fallback fonts only */
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/oauth-success.html`:
- Line 8: The `@import` for Google Fonts in lib/oauth-success.html will be blocked
by the CSP set in lib/auth/server.ts (font-src 'self' data:), so choose one
approach and implement it: either update the CSP in the auth server to include
fonts.googleapis.com and fonts.gstatic.com (adjust the header or function that
builds the CSP string in lib/auth/server.ts to add those hosts to font-src), or
self-host the Space Grotesk font files and change the CSS import in
lib/oauth-success.html to reference local font files, or remove the `@import` and
rely on fallbacks; also add a short comment in lib/auth/server.ts or
lib/oauth-success.html documenting the chosen approach so future maintainers
don’t reintroduce the mismatch.
- Line 258: The static callback address inside the <code> element showing
"127.0.0.1:1455" in lib/oauth-success.html should not be hardcoded; either
replace that literal with an illustrative placeholder (e.g., "127.0.0.1:<port>")
and add a brief inline comment that it's illustrative, or remove the callback
address display entirely if it's not required for users; if 1455 truly is fixed
by design, add a comment next to the <code> element stating the port is
intentionally fixed to 1455.
In `@test/copy-oauth-success.test.ts`:
- Around line 18-34: Add a regression test in test/copy-oauth-success.test.ts
that simulates an EBUSY/filesystem lock when writing the destination and asserts
the script's behavior; specifically, stub/mock the fs write or copy used by the
copyOAuthSuccessHtml function in scripts/copy-oauth-success.js (e.g., the
underlying fs.promises.writeFile, copyFile or stream write) to throw an error
with code "EBUSY" on the first call, then either allow a retry or ensure the
function surfaces a clear, identifiable error; the test should import
copyOAuthSuccessHtml, set up the mock to throw EBUSY, call
copyOAuthSuccessHtml({ src: oauthSuccessPath, dest }), and assert that the
module either succeeds after a retry or rejects with an error containing "EBUSY"
(or a clear message), cleaning up any temp dirs afterwards.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
test/__snapshots__/copy-oauth-success.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
lib/oauth-success.htmltest/copy-oauth-success.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/oauth-success.html
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/copy-oauth-success.test.ts
🔇 Additional comments (4)
lib/oauth-success.html (2)
246-253: previous review concern addressed - messaging no longer claims credentials are saved.the old page said "credentials saved" before persistence actually completed. the new text at
lib/oauth-success.html:248now reads "saving after terminal completes" which accurately reflects the flow - the browser sees this page beforelib/codex-manager.ts:3632callspersistAccountPool.good fix. this resolves the misleading ux concern from the prior review.
196-200: good: prefers-reduced-motion respected.
lib/oauth-success.html:196-200disables the fade animation for users with motion sensitivity. solid accessibility practice.test/copy-oauth-success.test.ts (2)
1-10: good: windows line ending normalization prevents flaky snapshots.
test/copy-oauth-success.test.ts:10normalizes CRLF to LF before snapshotting. this keeps the test deterministic across platforms.
31-33: good: cleanup in finally block ensures no temp dir leaks.
test/copy-oauth-success.test.ts:31-33properly removes the temp directory even if assertions fail. keeps ci runners clean.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| <h1>Sign-in complete</h1> | ||
| <p class="lead">Account connected. Return to your terminal to continue.</p> | ||
| <div class="state"> | ||
| <span>Status</span> | ||
| <strong>Connected</strong> | ||
| </div> | ||
| <ul class="status-list" aria-label="OAuth steps"> | ||
| <li> | ||
| <span>Authorization code</span> | ||
| <span class="ok">received</span> | ||
| </li> | ||
| <li> | ||
| <span>Token exchange</span> | ||
| <span>continuing in terminal</span> | ||
| </li> | ||
| <li> | ||
| <span>Credentials</span> | ||
| <span>saving after terminal completes</span> | ||
| </li> | ||
| <li> | ||
| <span>Session</span> | ||
| <span>finalizing</span> | ||
| </li> |
There was a problem hiding this comment.
Contradictory messaging: The page displays "Sign-in complete" and status "Connected" but the OAuth steps indicate processes are still ongoing ("continuing in terminal", "saving after terminal completes", "finalizing"). This creates user confusion about whether authentication succeeded or is still in progress.
<!-- Either change heading to reflect in-progress state: -->
<h1>Finalizing sign-in</h1>
<p class="lead">Authentication received. Completing setup in terminal.</p>
<!-- Or update steps to past tense if truly complete: -->
<span>completed</span>
<span>saved</span>
<span>ready</span>| <h1>Sign-in complete</h1> | |
| <p class="lead">Account connected. Return to your terminal to continue.</p> | |
| <div class="state"> | |
| <span>Status</span> | |
| <strong>Connected</strong> | |
| </div> | |
| <ul class="status-list" aria-label="OAuth steps"> | |
| <li> | |
| <span>Authorization code</span> | |
| <span class="ok">received</span> | |
| </li> | |
| <li> | |
| <span>Token exchange</span> | |
| <span>continuing in terminal</span> | |
| </li> | |
| <li> | |
| <span>Credentials</span> | |
| <span>saving after terminal completes</span> | |
| </li> | |
| <li> | |
| <span>Session</span> | |
| <span>finalizing</span> | |
| </li> | |
| <h1>Finalizing sign-in</h1> | |
| <p class="lead">Authentication received. Completing setup in terminal.</p> | |
| <div class="state"> | |
| <span>Status</span> | |
| <strong>In Progress</strong> | |
| </div> | |
| <ul class="status-list" aria-label="OAuth steps"> | |
| <li> | |
| <span>Authorization code</span> | |
| <span class="ok">received</span> | |
| </li> | |
| <li> | |
| <span>Token exchange</span> | |
| <span>continuing in terminal</span> | |
| </li> | |
| <li> | |
| <span>Credentials</span> | |
| <span>saving after terminal completes</span> | |
| </li> | |
| <li> | |
| <span>Session</span> | |
| <span>finalizing</span> | |
| </li> | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
|
Addressed the actionable review items:
Let me know if you need anything else tightened up. |
|
Follow-up changes pushed:
All lint, build, and targeted Vitest runs ( �[32m✓�[39m test/copy-oauth-success.test.ts �[2m(�[22m�[2m2 tests�[22m�[2m)�[22m�[32m 42�[2mms�[22m�[39m �[2m Test Files �[22m �[1m�[32m1 passed�[39m�[22m�[90m (1)�[39m |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/copy-oauth-success.js`:
- Around line 19-27: The file defines a duplicate sleep helper; remove the local
async function sleep and import the existing delay helper from lib/utils.ts
instead (or re-export the utils function if names differ). Replace references to
the local sleep with the imported function and update the top of
scripts/copy-oauth-success.js to import the helper (matching the exported name
from lib/utils.ts) so there is a single source of truth for sleep.
- Around line 46-51: The retry backoff implements linear growth (sleep(backoffMs
* attempt)) but the PR/doc claim exponential backoff; change the backoff
calculation in the retry loop that uses attempt, maxAttempts, backoffMs and
sleep to an exponential formula (e.g. backoffMs * 2 ** attempt) or update the
PR/commit message/docs to state "linear backoff" instead—ensure the isBusy
check, attempt increment, and await sleep(...) call remain consistent with the
chosen strategy.
- Around line 46-49: Update the transient-error check to also treat EPERM as a
retryable condition: change the isBusy computation used in the retry logic (the
variable checked with err && typeof err === "object" && err.code === "EBUSY") to
consider err.code === "EPERM" in addition to "EBUSY" so the throw/continue
behavior around attempt and maxAttempts remains the same; keep the existing
attempt >= maxAttempts - 1 bail condition intact and ensure the check references
the same variables (isBusy, err.code, attempt, maxAttempts).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
scripts/copy-oauth-success.js
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/copy-oauth-success.js (1)
lib/utils.ts (1)
sleep(54-56)
🔇 Additional comments (2)
scripts/copy-oauth-success.js (2)
35-54: retry logic and integration look solid overall.the attempt counting is correct (3
maxAttempts= initial try + 2 retries), and wiring it intocopyOAuthSuccessHtmlviascripts/copy-oauth-success.js:66keeps the public api unchanged. the test coverage for the mocked ebusy scenario mentioned in the pr should catch regressions here.Also applies to: 60-69
71-77: formatting-only change — no concerns.
| /** | ||
| * Pause execution for the requested number of milliseconds. | ||
| * @param {number} delayMs milliseconds to wait | ||
| */ | ||
| async function sleep(delayMs) { | ||
| return new Promise((resolve) => { | ||
| setTimeout(resolve, delayMs); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
duplicate sleep helper exists in lib/utils.ts:53-55.
this reimplements the same promise-based delay already exported from the utils module. for a build script it's often fine to stay self-contained, but if you want to reduce duplication you could import from lib/utils.ts instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/copy-oauth-success.js` around lines 19 - 27, The file defines a
duplicate sleep helper; remove the local async function sleep and import the
existing delay helper from lib/utils.ts instead (or re-export the utils function
if names differ). Replace references to the local sleep with the imported
function and update the top of scripts/copy-oauth-success.js to import the
helper (matching the exported name from lib/utils.ts) so there is a single
source of truth for sleep.
| const isBusy = err && typeof err === "object" && err.code === "EBUSY"; | ||
| if (!isBusy || attempt >= maxAttempts - 1) { | ||
| throw err; | ||
| } | ||
| attempt += 1; | ||
| await sleep(backoffMs * attempt); |
There was a problem hiding this comment.
backoff is linear, not exponential as documented.
the pr description and commit message reference "exponential backoff," but backoffMs * attempt yields 50 → 100 → 150 ms (linear). true exponential would be something like backoffMs * 2 ** attempt (50 → 100 → 200 ms). either update the docs to say "linear" or switch to exponential if that was the intent.
🔧 proposed fix for exponential backoff
- await sleep(backoffMs * attempt);
+ await sleep(backoffMs * 2 ** (attempt - 1));📝 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.
| const isBusy = err && typeof err === "object" && err.code === "EBUSY"; | |
| if (!isBusy || attempt >= maxAttempts - 1) { | |
| throw err; | |
| } | |
| attempt += 1; | |
| await sleep(backoffMs * attempt); | |
| const isBusy = err && typeof err === "object" && err.code === "EBUSY"; | |
| if (!isBusy || attempt >= maxAttempts - 1) { | |
| throw err; | |
| } | |
| attempt += 1; | |
| await sleep(backoffMs * 2 ** (attempt - 1)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/copy-oauth-success.js` around lines 46 - 51, The retry backoff
implements linear growth (sleep(backoffMs * attempt)) but the PR/doc claim
exponential backoff; change the backoff calculation in the retry loop that uses
attempt, maxAttempts, backoffMs and sleep to an exponential formula (e.g.
backoffMs * 2 ** attempt) or update the PR/commit message/docs to state "linear
backoff" instead—ensure the isBusy check, attempt increment, and await
sleep(...) call remain consistent with the chosen strategy.
| const isBusy = err && typeof err === "object" && err.code === "EBUSY"; | ||
| if (!isBusy || attempt >= maxAttempts - 1) { | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
consider retrying on EPERM as well for windows antivirus edge cases.
windows defender and other av tools can also surface transient EPERM errors when scanning newly written files. if this is meant to harden against av locks, you might want to include that code alongside EBUSY.
🛡️ proposed change to handle EPERM
- const isBusy = err && typeof err === "object" && err.code === "EBUSY";
+ const isTransient =
+ err &&
+ typeof err === "object" &&
+ (err.code === "EBUSY" || err.code === "EPERM");
+ if (!isTransient || attempt >= maxAttempts - 1) {
- if (!isBusy || attempt >= maxAttempts - 1) {please confirm whether EPERM has been observed in your ci or local windows runs — if not, this may be overkill.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/copy-oauth-success.js` around lines 46 - 49, Update the
transient-error check to also treat EPERM as a retryable condition: change the
isBusy computation used in the retry logic (the variable checked with err &&
typeof err === "object" && err.code === "EBUSY") to consider err.code ===
"EPERM" in addition to "EBUSY" so the throw/continue behavior around attempt and
maxAttempts remains the same; keep the existing attempt >= maxAttempts - 1 bail
condition intact and ensure the check references the same variables (isBusy,
err.code, attempt, maxAttempts).
Resolves remaining review items from PR #25, #19, and #16. - Neutralize OAuth callback page state to avoid pre-persistence claims. - Harden copy retry behavior for Windows lock codes with exponential backoff. - Add alias case-insensitivity and explicit IA verification evidence. - Remove duplicate .gitignore pattern introduced during branch merge. Co-authored-by: Codex <noreply@openai.com>
Fixed items
Deferred items
Blocked items
Validation evidencepm run clean:repo:check - passpm run audit:ci - passpm run typecheck - passpm run lint - passpm run build - passpm test - pass (92 files, 2113 tests)
|
Resolves remaining review items from PR #25, #19, and #16. - Neutralize OAuth callback page state to avoid pre-persistence claims. - Harden copy retry behavior for Windows lock codes with exponential backoff. - Add alias case-insensitivity and explicit IA verification evidence. - Remove duplicate .gitignore pattern introduced during branch merge. Co-authored-by: Codex <noreply@openai.com>
1 participant
Summary
Testing
Validation checklist
Docs/Governance checklist
Risk level
Rollback plan
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
redesigns oauth success screen with cleaner ui, simpler card layout, and neutralized copy that accurately reflects browser-only state. replaces forward-looking claims ("token exchange complete") with present-tense status ("authorization code received, token exchange continuing in terminal").
key improvements:
font-src 'self' data:) inlib/auth/server.ts:57-59(already noted in previous thread)scripts/copy-oauth-success.jsnow retries on ebusy with exponential backoff, matching project convention fromAGENTS.md:81vi.doMockto verify windows antivirus lock handlingno runtime logic changed - static html/css only. copy accuracy improved: browser tab now shows only what's happened at display time (authorization code received), not future steps.
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 80b0032