Repository navigation
fix(auth): keep sign-ins another tab added when this tab writes its markers - #1969
Merged
dawsontoth merged 6 commits intoOct 15, 2026
Merged
dawsontoth merged 6 commits into
dawsontoth merged 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request improves cross-tab authentication state synchronization and robustness against storage write failures. It introduces tracking for unremoved markers when storage writes fail, updates local and remote sign-out flows to handle multiple URLs per entity, and adds comprehensive test suites for cross-tab marker behaviors and storage failures. The review feedback suggests enhancing TypeScript type safety by adding explicit type assertions to the return values of Object.fromEntries and Object.entries in readMarkers and rememberedSignIns respectively.
This was referenced Oct 15, 2026
dawsontoth
marked this pull request as ready for review
October 15, 2026 12:22
…arkers Each tab read Studio:PotentiallyAuthenticated once at startup and wrote that whole copy back on every sign-in and sign-out. That erased every marker another tab had added since. An app sign-out from the stale tab then sent those sign-ins no logout, and a sign-out of an entity only another tab held left its marker behind. - flagKeyAsSignedIn and flagKeyAsSignedOut now change one entity in the record as stored now, and leave the rest alone. A sign-in updates this tab's copy only once its write lands, so a refused write is retried at the next sign-in. - A passing check of a sign-in this tab already marked still writes nothing, and a failing one removes the marker only when it still holds the URL it checked. Neither undoes another tab's change. - When storage refuses to take a signed-out entity out of the record, this tab's later writes leave it out until one lands, instead of trusting or discarding the whole record. - App sign-out, the local full sign-out and signOutEntities act on this tab's markers and every stored one, and log out each URL an entity is marked at, since another tab may have signed in again at a newer one. - signOutEntities' forgetStoredMarkers is gone: flagKeyAsSignedOut now drops another tab's marker for the entity it signs out, so managed cluster termination, which signs out locally, reaches it too. - A stored record that is not an object is treated as unreadable. - DESIGN.md records what remains: storage has no atomic read-modify-write, so two tabs writing at once can still lose one change, and a marker removed elsewhere stays removed. Closes #1968 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The markers lived in one shared Studio:PotentiallyAuthenticated record, so every write read and rewrote the whole of it. Two tabs writing at once could still lose a change, a refused write could take every marker with it, and a full sign-out rewrote the record once per entity. - Each marker now has its own key, Studio:PotentiallyAuthenticated:<id>, holding the URL signed into. A sign-in or sign-out touches only its entity's key, and sweeps enumerate the keys by prefix. - On load, the legacy record is copied into per-entity keys once per storage, and a marker already stored per entity wins. A record that is not one (an array, or not JSON) is removed at every load, since the previous release cannot start on it. - For one release, every marker change is mirrored into the legacy record, and sign-out sweeps read it. A tab still on the previous release, or a rollback, then still sees this release's sign-ins and sign-outs, and an app sign-out here reaches a sign-in only such a tab recorded. The copy is never repeated: that tab writes its whole load-time record back, which would bring back what this release signed out. - A sign-in storage refuses to record no longer throws out of setUserForIdAndKey or a check. Its next sign-in or check records it. A marker storage would not remove is retried at every marker write. - A check's answer is dropped when this tab signed in again at another URL while the check was out. - Unit tests seed and read markers through signInMarkersTestHelpers, and the e2e anon specs through e2e/tests/signInMarkers.ts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ng silently - The sign-out wipe keeps the migration flag, so a previous-release tab writing its record back after a full sign-out isn't migrated again. - Until a migration lands, as when a full quota refuses it, the legacy record also seeds this tab's markers, so its sign-ins still check out. - Migration decides from one read, and also drops entries that aren't markers from the legacy record. A sign-in leaves a legacy record it can't read alone instead of replacing other tabs' markers. - A sign-in always records itself, so signing in again restores a marker another tab removed; only a check's passing answer skips it. - A check's answer is dropped when this tab signed in again while it was out, at the same URL or another, using a per-entity sign-in revision. - A marker storage refused to remove is retried only while it still holds the value it had, so another tab's sign-in there survives, and a marker that couldn't be read at sign-out is still removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A failed check is a probe, not a sign-out: when it can't read the marker it leaves it, and a mirror write it can't make no longer takes the whole legacy record. - For the app sign-in, which every /User/current answer re-signs, those answers no longer write the marker, so they can't put back one another tab's failed check dropped. - A failing app check that was out during a new app sign-in is dropped, as an entity's already was. - This tab's own sign-in cancels a removal of its marker still owed from a refused write. - Migration checks its flag before the best-effort cleanup, so a refused cleanup on a migrated storage can't seed signed-out entities. The sign-out wipe marks the storage migrated, as nothing of the legacy record survives it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A page-load app check still out when the user signed in again applied its answer for the old session over the new sign-in: getCurrentUser synced it into the store, and loadUser applied it again. - The store's app check now fetches through fetchCurrentUser, which doesn't sync, so loadUser applies the answer itself, and the sign-in revision guard drops a stale passing answer as it drops a failing one. - getCurrentUser syncs its answer only if no sign-in landed while it was in flight, using the store's new getSignInRevision. - DESIGN.md links #1970 for removing the legacy mirror. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- getCurrentUser syncs through the new syncAppUser, which writes no marker this tab already holds and leaves the sign-in revision alone. A sync landing during the store's own app check therefore no longer makes that check drop its answer and skip its sign-out re-ask. - setUserForIdAndKey treats every sign-in, the app's included, as one that always records itself. - A removal storage still owes is cancelled up front only when the kept marker already holds the URL being signed in, and otherwise once the new marker is written. - A migration flag that can't be read is reported as migrated, so a previous-release write-back can't seed that load. - getSignInRevision sits under its own doc comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dawsontoth
force-pushed
the
claude/1954-self-hosted-logout
branch
from
October 15, 2026 13:33
e61ed02 to
0b14c61
Compare
dawsontoth
force-pushed
the
claude/1968-signin-markers-per-key
branch
from
October 15, 2026 13:33
ea84900 to
e7ae19f
Compare
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
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
Each tab read the sign-in markers once at load and wrote that copy back on every sign-in and sign-out, erasing what other tabs had added since. An app sign-out from the older tab then sent those sign-ins no logout. Signing out an entity only another tab had marked, including at a Fabric cluster's termination, left its marker behind.
🤔 Should this be separate from #1966?
Yes, as agreed there: it changes how every tab stores its markers. It also covers #1966's deferred Fabric-termination finding, now tracked in #1968.
💡 Solution
localStoragekey, so a sign-in or sign-out touches only its entity.🤔 What does a tab still on the previous release see?
This release's changes, at its next load: they are mirrored into the old record, and sweeps here read it, so an app sign-out here logs out that tab's sign-ins too. Only its sign-out of an entity misses the new key, which matters only if its logout failed. Without the mirror a rollback shows everyone signed out; #1970 drops it later.
Alternatives
🔧 Changes
signInMarkers.ts(new): the key layout, the one-time migration and the mirror into the old record.authStore.ts:flagKeyAsSignedInwrites one key and no longer throws when storage refuses.flagKeyAsSignedOutremoves one, and only the URL a failed check used.getCurrentUser.ts: the store's app check fetches without syncing, and a/User/currentanswer syncs only if no sign-in landed meanwhile, throughsyncAppUser, which counts as a check.signInMarkersTestHelpers.ts, and the e2e specs throughe2e/tests/signInMarkers.ts.The other 21 files: the new key's name, and seeds moved onto the helpers
e2e/README.mdnames the new key.auth.setup.tsnames the new key.cluster-form.anon.spec.tsseeds throughrememberAppSignIn.clusters.anon.spec.tsseeds throughrememberAppSignIn.global-search.anon.spec.tsseeds throughrememberAppSignIn.new-org.anon.spec.tsseeds throughrememberAppSignIn.organizations.anon.spec.tsseeds throughrememberAppSignIn.profile.anon.spec.tsseeds throughrememberAppSignIn.sidebar-help.anon.spec.tsseeds throughrememberAppSignIn.clearAuthStateLocally.tssays the wipe keeps the migration flag.signOutHandlers.test.tsexpects the flag after the wipe.appSignOutElsewhere.test.tsseeds and reads markers through the helper.entityAuthCheckScope.test.tsseeds and reads markers through the helper.isKnownSignedOut.test.tsseeds and reads markers through the helper.signOutEntities.test.tsseeds and reads markers through the helper.signOutRaces.test.tsseeds and reads markers through the helper.ClusterHome.signIns.test.tsxseeds and reads markers through the helper.crossTabSignOut.test.tsxseeds through the helper.endedSessionRouteFailures.test.tsxseeds and reads markers through the helper.rememberedClusterDeepLink.test.tsseeds and reads markers through the helper.terminatedClusterDeepLink.test.tsxseeds and reads markers through the helper.DESIGN.mdrecords the layout and the mixed-release rules;AGENTS.mdnames the new key.✅ Verification
End-to-end route: (a). I extended
account-menu.anon.spec.tsand ran the anon Playwright suite in local Chrome against this branch: 60 passed, 1 skipped. Its two new cases fail without migration or the old-record sweep.vitest(562 files, 5,762 passed),tsc -b,oxlint,dprint checkand the e2etscexit 0 on Node 24.21.0.What the tests cover, mutation checks, and a flaky unrelated file
crossTabMarkers.test.ts: per-key isolation, sweeps over other tabs' markers and both URLs, checks that leave other tabs' changes, the app sign-in, and stale check answers.legacySignInMarkers.test.ts: migration once per storage (also across a full sign-out and a refused first copy), junk records, the mirror, and sweeps over previous-release sign-ins.localSignOutStorageFailure.test.ts: refused writes and removals, and their retries.terminateCluster.test.ts: Fabric termination forgets markers only another tab holds.getCurrentUser.test.ts: a/User/currentanswer a sign-in overtook leaves that sign-in.cardLinksDuringPendingNavigation,AddOrganizationRoleModal,RenameFileModal). None runs this code, each passes alone, and the pre-commit runs were clean.Closes #1968
🤖 Generated with Claude Code
Related PRs: #1966 overlaps (stacked on it)
Complexity: complicated
Deferred findings
d861d82357ec0895minor concurrency —src/features/auth/store/authStore.ts— non-atomic cross-tab read-modify-write of the shared marker record can lose a concurrent sign-in or sign-out — filed Sign-in markers: stop mirroring to the legacy shared record one release after #1969 #1970a947843f6d1dd466nit correctness —src/features/auth/store/authStore.ts— constructor accepts an array or junk marker record; filtered junk survives sign-outs — filed Sign-in markers: stop mirroring to the legacy shared record one release after #1969 #19701282fb67a2760cbcnit data-integrity —src/features/auth/store/authStore.ts— a transient unreadable record replaces every other tab's marker with this sign-in — filed Sign-in markers: stop mirroring to the legacy shared record one release after #1969 #19703784139b4d036a24nit maintainability —src/features/auth/store/crossTabMarkers.test.ts— new test imports omit file extensions required by the checklist — filed Decide whether relative imports should name their file extension #1972face1e3da4c920f7nit security —src/features/auth/store/authStore.ts— both URLs' logout clients carry the single shared BasicAuth credential, so one logout can fail — filed Sign-out: a logout to each marked URL carries the one stored Basic Auth credential #197143cd604ec076ea0enit concurrency —src/features/auth/store/signInMarkers.ts— migration's check-then-set can overwrite a newer per-entity sign-in — filed Sign-in markers: stop mirroring to the legacy shared record one release after #1969 #1970defde12cf7a0afc8nit correctness —src/features/auth/store/signInMarkers.ts— a legacy record holding only filtered-out entries persists as {} — filed Sign-in markers: stop mirroring to the legacy shared record one release after #1969 #197058c8df7ce1fb2420nit concurrency —src/features/auth/store/signInMarkers.ts— non-atomic cross-tab read-modify-write of the shared marker record can lose a concurrent sign-in or sign-out — filed Sign-in markers: stop mirroring to the legacy shared record one release after #1969 #1970a09380796d04a58fnit data-integrity —src/features/auth/store/authStore.ts— retryUnremovedSignInMarkers removes another tab's same-URL re-sign-in, or any replacement after an unreadable refusal — filed Sign-out: reach other tabs when storage refuses the sign-out's writes even after the wipe #196081c98bc4312a6a42nit performance —src/features/auth/store/authStore.ts— rememberedSignIns deduplicates with findIndex inside filter, quadratic in remembered URLs — filed Sign-in markers: stop mirroring to the legacy shared record one release after #1969 #1970Review-Coverage: authored=claude; ran=gemini,codex,cursor-grok,cursor-muse,cursor-composer; adjudicated=domain; declined=cursor-kimi,muse; rounds=10; full=2 @ ea84900
Review-Attention: deep ~60m (sensitive: clearAuthStateLocally.ts, getCurrentUser.ts +3; decisions: unreadable-migration-fails-closed, check-specific-sync, same-value-retry-removes, legacy-mirror-one-release, do-less-alternative; raised: degraded review) @ ea84900