fix(api-explorer): clear the typed password on the mint, not on a later render - #1656
Conversation
…er render The login form dropped the typed credentials from a `useEffect` keyed on a `pending` -> `idle` status transition. React flushes passive effects in a Scheduler task *after* the commit they belong to, and `shouldYieldToHost()` splits the two into separate macrotasks whenever a commit blows the 5ms frame budget. In that ordering the clear arrives a commit late: a user who starts typing the next credentials as soon as the panel says "Credential set" has the username wiped out from under them, which disables the submit button on an empty field. `runMint` now reports how it finished (`applied` / `discarded` / `failed`) and the form clears off that promise, so the clear batches into the same commit as the status and credential updates. Only `applied` clears: a `discarded` mint no longer owns the form, and re-picking the auth type mid-flight supersedes a mint without unmounting it, so clearing there would blank credentials the user had already retyped -- the same defect one race narrower. The one clear that *should* beat live typing is a sign-out, which the deleted effect covered incidentally (revocation drove status back to `idle`). A revocation counter now keys the form instead, so typed credentials cannot outlive the session they were meant for whether or not a mint was in flight. The counter advances only on a newly-observed epoch for this entity: `onExplorerAuthInvalidated` names no entity and fires on any sign-out, so an unconditional bump would blank a form belonging to an instance nobody signed out of. Guards are mutation-checked, none by timeout: restoring the effect fails the re-auth test in 64ms on `expected 'dave' to be ''`; widening the clear to `discarded` fails the superseded-mint test on `expected '' to be 'bob'`; dropping the revocation key fails the sign-out test on `expected 'alice' to be ''`; bumping revocation unconditionally fails the cross-entity test on `expected '' to be 'alice'`. Verified: 20 consecutive full-suite runs green (it reproduced twice in 12 runs before this). Refs #1655 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where typed credentials in the API Explorer login form were incorrectly cleared due to late-flushing passive effects or discarded/failed mint operations. It replaces the useEffect-based status tracking with a promise-based outcome (MintOutcome) to conditionally drop typed credentials only when a mint is successfully applied. Additionally, it introduces a revocation counter to reset the form key when authentication is revoked, and adds comprehensive tests to verify these behaviors. I have no further feedback to provide as there are no review comments.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
…render `useRef(authStore.getExplorerAuthEpoch(entityId))` evaluates its initializer on every render, not just the first, and reading the epoch does a `localStorage.getItem` plus a `JSON.parse`. `useResizableSidebar` keeps the sidebar width in `useState` and calls `setWidth` from `mousemove`, so a sidebar drag re-renders `ApiExplorer` continuously -- putting that parse on exactly the path whose own comment says writing localStorage per `mousemove` "would stutter". The effect below already baselines the ref on mount and on an `entityId` change, which is the only point the value is read, so the eager initializer was redundant as well as costly. That baseline is load-bearing and was untested: an entity that mounts on a non-zero epoch would otherwise read the next unrelated sign-out as a change to itself and blank the form. Covered now, and mutation-checked -- dropping the baseline fails the new test on `expected '' to be 'alice'`. Refs #1655 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kriszyp
left a comment
There was a problem hiding this comment.
Sounds good
🤖 Reviewed with Codex
Three review findings on #1656, all sharing one cause: the explorer's sign-out signal names no entity, so every explorer acted on every sign-out. `onExplorerAuthInvalidated` now compares the epoch for the `id` it was given — the parameter existed but was ignored (`_id`) — so an unrelated instance's sign-out no longer increments this entity's `attemptRef`, which was silently discarding its in-flight mint. It also notifies in-process subscribers on a same-tab write, which fires no `storage` event of its own, so a same-tab sign-out now empties the form directly rather than waiting for the cancelled mint to settle. The mint's epoch-mismatch branch no longer bumps revocation: the subscription owns that now, and a second bump would remount the form again and take any credentials retyped since with it. `credentialIsCurrent` re-read the epoch during render, so the localStorage parse was still on the sidebar-resize path that 81a642f claimed to clear -- that commit removed a `useRef` initializer and missed this one. The epoch is held in state and advanced by the subscription instead. An `applied` mint no longer clears unconditionally. The inputs stay live while a mint runs, so a user can submit Alice's credentials and start on Bob's before a slow mint returns; clearing on Alice's success erased a draft it never saw. The form counts edits and compares at settle, so the clear stays tied to the values actually submitted. Each guard is mutation-checked: ignoring the edit generation fails on `expected '' to be 'bob'`; dropping the store's epoch comparison fails three unrelated-entity tests; dropping the same-tab notification fails on `expected 'alice' to be ''`. Refs #1655 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`onExplorerAuthInvalidated` read its baseline epoch, then registered its listener. A sign-out dispatched between the two reached nobody -- the event was gone, and no later one was coming -- so the subscriber sat on a revoked generation it believed was current. `ApiExplorer` had the same shape one level up, seeding its cached epoch before subscribing. Both now re-check once subscribed. The store compares against its baseline after registering, which is a no-op unless the gap actually swallowed an event, and the explorer reads its epoch after the subscription is live. Reordering the baseline to after `addEventListener` was the first attempt and is worse: `fire` closes over it, so an event arriving in that window throws a TDZ `ReferenceError` rather than being missed quietly. The new store test caught that, which is the reason it exists. Covers the store's subscription directly for the first time -- entity scoping, the global-logout wildcard, unsubscribe, and the race above. The race test stages the sign-out *before* `addEventListener` registers, because staging it after lets the ordinary event path satisfy it: written the obvious way it passes against the unfixed code, and only fails on `expected 1 times, got 0` in this order. Refs #1655 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed Fixed. Two notes on how that landed, since both are the kind of thing worth not repeating:
This also adds the first direct coverage of the store subscription itself — entity scoping, the global-logout wildcard, unsubscribe, and that race. Not fixed, and flagged rather than folded in: if the Gate: |
#1656 merged by rebase, so stage carries rebased copies of this branch's parent commits. Both conflicts are that duplication: stage holds the #1656 versions of `authStore.ts` and the explorer-invalidation test, this branch holds the same content plus the epoch fallback and its coverage. Verified `git diff stage` over both files is exactly this branch's additions and nothing of stage's is lost, so each was resolved to this branch's side. Also corrects the `explorerEpochFallback` doc comment, which still claimed the memory-only count fails safe across a reload -- the claim the previous commit disproved and replaced with destroying the credential outright. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes #1655.
ApiExplorer.test.tsx > clears the login password after re-authenticating while already authorizedfailed only in full-suite runs — green every time in isolation, red on whateverPR happened to be running when it tripped. It reproduced here twice in 12
pnpm testrunsand never once on the file alone.
It is not a timeout, and it is not test-only. The password genuinely never cleared, because
the clear could arrive a commit late and land on the next thing the user typed.
The mechanism
LoginFormdropped the typed credentials from auseEffectkeyed on apending → idlestatus transition. React flushes passive effects in a Scheduler task after the commit they
belong to, and
shouldYieldToHost()splits the two into separate macrotasks whenever acommit blows its 5 ms frame budget — which is what a loaded full-suite run does and an idle
single-file run does not.
findByText(/Credential set —/)resolves off the commit's DOM mutation, so in the splitordering the test proceeded while the clear was still outstanding. The clear then landed on
the next
fireEvent.change, wipingusername, which disabled the submit button(
disabled={pending || !username || !password}), so the second mint never ran and the fieldkept
pw2until the 1000 mswaitFordeadline. That accounts for the issue's 1165 msfigure: the first
awaitwas fast, the second consumed the whole budget.Pinning the ordering with an always-advancing
performance.now()reproduces it 3/3:Under the idle ordering the same probe reads
username="" password=""atfindByText— thefast path everyone debugs with.
This is a real UX bug, not only a test artifact. A user who starts typing new credentials
as soon as the panel says "Credential set" can have the username wiped out from under them
and the Authorize button go dead on the empty field.
The change
runMintnow reports how it finished —applied/discarded/failed— and the formclears off that promise instead of off a re-render. The clear batches into the same commit as
the status and credential updates, so no commit that shows the credential still shows the
password that bought it.
failedkeeps the fields, so a typo is correctable without retyping.Only
appliedclears. Adiscardedmint no longer owns the form: the method buttons staylive while a mint is pending, so re-picking the auth type supersedes a mint without
unmounting it, and clearing there would blank credentials the user had already retyped — the
same defect one race narrower.
The one clear that should beat live typing is a sign-out, which the deleted effect covered
incidentally (revocation drove status back to
idle). A revocation counter now keysLoginForm, so typed credentials cannot outlive the session they were meant for whether ornot a mint was in flight. It advances only on a newly-observed epoch for this entity —
onExplorerAuthInvalidatednames no entity and fires on any sign-out, so an unconditionalbump would blank a form belonging to an instance nobody signed out of.
The test
The flaky test now installs that same forced-yield clock, so it exercises the bad
interleaving on every run rather than roughly one in twelve.
Every guard is mutation-checked — reverted locally, confirmed red, and red on a real
assertion rather than a timeout:
expected 'dave' to be ''discardedexpected '' to be 'bob'expected 'alice' to be ''expected '' to be 'alice'expected '' to be 'alice'That check is the point rather than a formality: written naively, all four of these tests pass
against the broken code.
Second commit: a per-
mousemovelocalStorage parseReview caught that
useRef(authStore.getExplorerAuthEpoch(entityId))evaluates itsinitializer on every render, and reading the epoch does a
localStorage.getItemplus aJSON.parse.useResizableSidebarkeeps the sidebar width inuseStateand callssetWidthfrom
mousemove, so a sidebar drag re-rendersApiExplorercontinuously — putting that parseon exactly the path whose own comment says writing localStorage per
mousemove"wouldstutter". It's now
useRef(0), baselined by the effect that already sets it on mount and onan
entityIdchange, which is the only point the value is read.Fixing it surfaced that the baseline is load-bearing and was untested: an entity mounting on a
non-zero epoch would read the next unrelated sign-out as its own and blank the form. Covered
now, and mutation-checked.
Verification
tsc -b,oxlint,dprint check— all clean.tests with a mocked mint.
Reviewer notes
Three things I decided rather than fixed, so you can overrule them:
during that same request. Flagged in review; I kept it, since clearing after a mint that
succeeded is the stated point of the code and the erased text is credentials the user just
proved obsolete. The inputs are disabled while
pending, so the window is narrow.performance.now()fixture depends on React's current yield heuristic. If a Reactbump stops consulting it, the fixture goes inert and the suite would not go red. Real, and
not fixable from inside the test. The superseded-mint and sign-out tests cover neighbouring
invariants without the clock.
MintOutcomelives inSettingsPanel.tsx, which consumes it, and is imported back intoApiExplorer.tsx, which produces it.types.tswould be the tidier home; happy to move it.Two findings review carried forward that are pre-existing and deliberately out of scope —
both predate this PR and neither is touched by it. Happy to file them:
increments
attemptRefand resets status unconditionally, so signing out of instance Bsilently kills a login you have in flight against instance A. I gated the revocation on
this entity's epoch but left those two lines alone, since changing them alters behavior
that predates the bug.
storageevent, so revocation clearing waits on the stalemint's epoch-mismatch continuation rather than firing directly.
One review finding I did not act on, because I could not reproduce it: a claim that the
epoch-mismatch branch in
runMintdouble-remounts the form after a cross-tab sign-out. Itcannot — the same subscriber does
attemptRef.current++, so an in-flight mint exits at thesuperseded guard and never reaches the epoch check. Demonstrated rather than argued: with the
revocation bump made unconditional, the double-settle test stayed green, which is only
possible if that branch never runs.
Review coverage
Four pre-push cross-model rounds ran; rounds 2 and 3 each found a genuine defect in the
previous round's fix, both now fixed. Coverage was thinner than the round count suggests —
every round had
geminifail onquotaandcursor-composerfail on the 1Password SSHagent (
sign_and_send_pubkey: signing failed), withcursor-grokpruned. Actual coverageeach round was
codex+harper-domainonly, so an extra human pass on the concurrencyreasoning is worth more here than the round count implies.
Separately, and out of scope for this PR: the suite has a second flake. One of the 12
baseline runs failed with
Errors 1 errorand no test named — an unhandledTypeError: The "event" argument must be an instance of Eventfrom undici's WebSocket,attributed to
ToolCallGroup.test.tsxbut originating from a socket opened elsewhere(
NotificationsSubscriptionManager.tsxis the onlynew WebSocketinsrc). #1655 liststhat
Errors 1 errorhusky shape as the same defect; it is not. Worth its own issue.🤖 Generated with Claude Code
Review-Coverage: authored=claude; ran=codex; blocked=gemini(quota); declined=cursor-grok,cursor-composer,domain; rounds=5 @ 81a642f
Human-Review-Need: 3 @ 81a642f