refactor(web): model session lifecycle as one discriminated union#33219
Conversation
Replace the `hasPlatformSession` + `platformSessionResolved` boolean pair in the auth store with one `PlatformSessionStatus = "unknown" | "absent" | "present"`. The two booleans encoded one concept — session liveness — across two fields, which made "false that really means unknown" representable: any reader that grabbed `hasPlatformSession` without also gating on `platformSessionResolved` silently treated the pre-settle probe window as a confirmed negative. That ambiguity caused the same class of onboarding-fork race three times. The union makes the ambiguous state unrepresentable: readers that want liveness compare to `"present"`; the one imperative consumer that must wait reads `!== "unknown"`. The mapping is exact, so every reader migrates mechanically: - `hasPlatformSession` ≡ `platformSession === "present"` - `platformSessionResolved` ≡ `platformSession !== "unknown"` One internal behavior change: the probe no longer reopens the `"unknown"` window on app-resume re-probes. Only the initial boot starts from `"unknown"`; a re-run leaves the last `"present"`/`"absent"` in place until the new result lands, so reactive consumers (`usePlatformGate`, `useIsOrgReady`, root layout, organization store) keep showing the last-known session instead of flickering to disabled/redirecting on every resume. No reader re-waits mid-session, so this is safe. Closes LUM-2220. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@noanflaherty — design ack requested before this merges, since it rewrites the auth store's public platform-session shape and sits in the LUM-1788 (gateway-as-BFF) blast radius. The mechanical part is behavior-preserving ( The probe no longer reopens the Holding the |
…helper The three local gateway auth entry points (initSession, refreshSession, connectLocalAssistant) each inlined the same reachability gate: probe the platform session when one could exist, otherwise settle directly to "absent". Collapse them onto a shared probePlatformSessionIfReachable helper so the gate has a single definition. connectLocalAssistant's copy still settled the pre-enum platformSessionResolved boolean; routing it through the helper settles the tri-state "absent" instead, matching the other two paths. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Collapse the auth store's `isLoggedIn`/`isLoading` boolean pair into a single
`sessionStatus: "initializing" | "authenticated" | "unauthenticated"` enum,
alongside the existing `platformSession` tri-state, so the contradictory
"logged in yet still loading" state is unrepresentable.
Three layers of repetition are removed:
- Read side: consumers no longer re-encode enum literals
(`=== "authenticated"`, `=== "present"`). Pure predicates
(`isAuthenticated`, `isSessionSettled`, `hasLivePlatformSession`) own each
question once, with matching reactive hooks (`useIsAuthenticated`,
`useIsSessionInitializing`, `useHasPlatformSession`) composing them over
atomic selectors.
- Write side: the hand-listed `set({...})` field combos collapse into named
transitions (`authenticatedPlatformUser`, `authenticatedLocalUser`,
`sessionEnded`) so the actions read as a state machine.
- The boolean-pair anti-pattern itself becomes the single `sessionStatus` enum.
The pure types and predicates live in a dependency-free `session-status`
module so modules the store depends on (the assistant lifecycle service) can
read session meaning without an import cycle — `import type` cannot break a
cycle through a runtime value. Reactive components read the hooks from the
store; imperative readers (middleware, route resolvers, lifecycle) call the
predicates directly.
Documents the discriminated-union and shared-predicate conventions in
STATE_MANAGEMENT.md.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
![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: 78fe427a4e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…n enum reshape Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…probe resolveLocalOnboardingRoute reads platform-session liveness imperatively. A re-probe leaves the prior "present"/"absent" displayed while it confirms in the background (so reactive consumers don't flicker), so the one-shot fork could read a stale value rather than the fresh probe result. Expose whenPlatformSessionSettled() from the auth store and have the resolver await it (under the same 5s bound as the boot wait) before choosing hosting vs welcome, decoupled from the displayed tri-state so reactive readers are unaffected. Covered by a regression test. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4652598e68
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The settle accessor returned the current probe's promise by value, so a probe that became latest after a caller began waiting was not awaited: resolveLocalOnboardingRoute could proceed when the probe it first observed settled (now stale) while a newer one was still in flight, reading a stale platformSession. Loop until the module-level settle reference is unchanged across an await, so the wait covers any probe launched mid-wait. Add a regression test for a probe that becomes latest after the wait begins. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE (with merge-gate notes below)
Value: This is textbook illegal-state elimination on the auth store. The (isLoggedIn, isLoading) pair had four combinations but only three legal ones — (true, true) was meaningless and every reader had to remember which pairs meant what. Same anti-pattern stacked one layer down on (hasPlatformSession, platformSessionResolved), where "false that really means unknown" let pre-settle readers silently treat the probe window as a confirmed negative. Both collapse into discriminated unions; both make the illegal state unrepresentable.
Layered cleanly:
SessionStatus = "initializing" | "authenticated" | "unauthenticated"— boot lifecycle, names each phase oncePlatformSessionStatus = "unknown" | "absent" | "present"— liveness as one value- Read side —
isAuthenticated(status)/hasLivePlatformSession(status)/isSessionSettled(status)predicates + matching reactive hooks (useIsAuthenticated,useIsSessionInitializing,useHasPlatformSession) compose the predicates over the store's atomic selectors. ~10 call sites stop re-encoding enum literals at the boundary.
The stores/session-status module is the right call. Dependency-free, so the lifecycle service (which the store itself depends on) can read session meaning without creating an import cycle. The docstring explains why import type couldn't break that cycle — predicates are runtime values. Good engineering decision worth the new file.
Verified substantive code at HEAD 4641b8b20f
-
probePlatformSessioncorrectly resolves overlapping probes vialatestPlatformProbeID +isStale()guards on all three branches (.then/.catch/.finally). Probes never reopen the"unknown"window (so reactive consumers don't flicker on app-resume refreshes), and only the initial boot probe starts from"unknown"— the.finally()resolves first-time"unknown"→"absent"when neither branch confirmed a session. Read it; matches the docstring's "latest-wins" + "no flicker" claims. -
whenPlatformSessionSettledchases the module-level reference exactly as Devin described in the Codex P2 #2 rebuttal:let awaited = platformProbeSettled; await awaited; while (platformProbeSettled !== awaited) { awaited = platformProbeSettled; await awaited; }
Correct: after each await, re-checks whether a newer probe replaced the reference and waits that one out too; returns only once the reference is unchanged across an await. The previous by-value version returned the originally-captured promise and would let a stale settle proceed while a newer probe was still in flight. The regression test ("waits for a probe that becomes latest after the wait begins") locks this in.
-
Docs sweep: Devin caught + fixed the
STATE_MANAGEMENT.md:275staleisLoggedInexample AND a sibling stale docstring inprechat-steps.tsreferencing the removedisLoadingfield. The intentionalisLoggedIn/isLoadingreferences atSTATE_MANAGEMENT.md:203/212(illustrating the retired anti-pattern) are correctly preserved.
Codex P2 #1 (bf14db21de) — Devin's mutually-exclusive-gates argument, then hardening anyway
Codex flagged: refreshSession() starts an async probe and returns before getSession() settles, so resolveLocalOnboardingRoute() could read a stale "absent" and redirect a freshly-authed user to /onboarding/welcome instead of /onboarding/hosting.
Devin's investigation: resolveLocalOnboardingRoute() has exactly one caller (auth-middleware.ts:43), which only fires when isLocalMode() && !hasAssistants(). In that state, refreshSession()'s probe takes the synchronous else branch and sets platformSession: "absent" immediately — it does NOT start an async getSession() probe. The two conditions (probe-launch gate fires vs resolver gate fires) can't co-occur. Specific misroute = not reachable.
But — Devin then hardened anyway in 4652598e68 with whenPlatformSessionSettled() + 5s deadline awaiting probe-settle in addition to the boot !== "unknown" wait. That's the right call: relying on a non-obvious cross-module invariant for correctness is a footgun. The hardening makes resolveLocalOnboardingRoute() correct independent of the gate-mutual-exclusion invariant.
Non-blocking, resolved.
Codex P2 #2 (4652598e68) — real gap, fixed in 4641b8b20f
Codex caught: whenPlatformSessionSettled() returned platformProbeSettled by value, so a probe that became latest AFTER the caller began waiting wasn't awaited. First-observed probe settles stale, no-ops, then resolver proceeds and reads platformSession while the newer probe is still in flight.
Devin acknowledged as a real gap and fixed by chasing the module-level reference (code above). Added regression test verified to fail against by-value version and pass with the fix.
Resolved — this is the substantive correctness improvement in the final commit.
Anti-pattern scan
- No
ascasts on runtime-boundary shapes ✓ - Discriminated unions used correctly; no
(status as "authenticated")smuggling ✓ - Pure predicates separated from reactive hooks (no
useStore.getState()hidden in selectors) ✓ - No new async/concurrent paths beyond the explicit probe coordination ✓
- TanStack Query / Zustand patterns untouched in this PR — consumer-side mechanical migration only ✓
- No
///Swift-style comments ✓ - Test coverage:
auth-store.test.ts+124/-50,local-onboarding-route.test.ts+6/-21,auth-middleware.test.ts+7/-16, regression test for the new chase-latest behavior ✓ - Single-purpose new module (session-status), justified by import-cycle reasoning ✓
Territory (R11e): Auth store + boot-probe lifecycle + onboarding route fork. Boss/Noa territory (this is the LUM-1788 gateway-as-BFF blast radius noted in the PR body). Not Vargas (SSE/reconciliation) or Mahmoud (vembda). ✓
Merge-gate status at HEAD 4641b8b20f:
- ✅ Vex APPROVED (this review)
⚠️ Devin clearance is stale: "No Issues Found" was atd8f853040e(first commit, 4 commits ago). Engagement at HEAD is author-fixing-own-Codex-P2, not reviewer-clearing the chase-latest pattern.⚠️ Codex hasn't seen HEAD: last review at4652598e68(the commit containing the bug Devin then fixed in HEAD). Need fresh Codex pass on the chase-latest implementation.⚠️ Noa design ack pending: Devin pinged @noanflaherty at 10:58 AM for design coordination because this rewrites the auth store's public contract. No reply yet (~9h). PR was opened as a draft for exactly this reason; the draft flag was flipped but the human-in-the-loop intent stands.- ✅ CI 7/7 success
Recommendation: Hold merge until Noa acks + fresh Codex review at HEAD clears. Triggering Codex now (next comment). If Codex 👍s the chase-latest fix and Noa lands a design ack, gate is met.
Vellum Constitution — Yours: this PR is the kind of work that pays compounding interest. Every reader that stops asking "is logged-in-and-loading even a thing?" is one less footgun for the next feature. The encoding lives in one module forever.
|
@codex review Re-review request: the P2 you flagged on |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Prompt / plan
Approved follow-up to the additive consolidation (#33203): retire the parallel-boolean
session representation in the web auth store in favor of single discriminated unions, the
model change deferred and tracked in LUM-2220. Opened as a draft and tagged for design
coordination because it rewrites the auth store's public contract, which sits in the
LUM-1788 (gateway-as-BFF) blast radius.
The store had three layers of the same anti-pattern stacked on top of each other. This PR
fixes all three so the store reads as a state machine and no consumer re-encodes its
internals.
What changed
1.
sessionStatus— session lifecycle as one union (wasisLoggedIn+isLoading)"initializing"— the boot probe has not settled; logged-in status is not yet known."authenticated"— a user is signed in (platform or local gateway)."unauthenticated"— the probe settled with no session.The
(isLoggedIn, isLoading)pair had four combinations but only three legal ones —(true, true)("logged in yet still loading") was meaningless, and every reader had toremember which combination meant what. The union makes the illegal state unrepresentable
and names each phase once.
2.
platformSession— liveness as one tri-state (washasPlatformSession+platformSessionResolved)This is the change from #33203's deferred model: two booleans encoded one concept
(session liveness) across two fields, making "false that really means unknown"
representable. Any reader that grabbed
hasPlatformSessionwithout also gating onplatformSessionResolvedsilently treated the pre-settle probe window as a confirmednegative. The union makes that unrepresentable.
3. Read side — predicates own each question once
Consumers stopped re-encoding enum literals (
=== "authenticated",=== "present") at~10 call sites. Each question is answered in one place:
stores/session-statusmodule —isAuthenticated(status),isSessionSettled(status),hasLivePlatformSession(status)— for imperative readers (auth middleware,
resolveLocalOnboardingRoute, the lifecycleservice) that already hold a value.
useIsAuthenticated(),useIsSessionInitializing(),useHasPlatformSession()— that compose the predicates over atomic selectors, forcomponents.
The raw
platformSessionfield stays exported for the two sites that genuinely need thetri-state (
isPlatformFunnelAvailable,resolveLocalOnboardingRoute).4. Write side — named transitions instead of hand-listed field combos
The ~13 inline
set({...})combinations acrossinitSession's three mode branches,refreshSession,connectLocalAssistant, logout, and biometric recovery collapse into afew named transitions —
authenticatedPlatformUser(user),authenticatedLocalUser(),sessionEnded()— so the actions declare which state they enter rather than pokingfields.
DRY: one reachability gate (carried from #33203)
The three local gateway auth entry points —
initSession,refreshSession, andconnectLocalAssistant— share one gate,probePlatformSessionIfReachable, so "whatcounts as reachable" has a single definition.
Breaking an import cycle the correct way
The lifecycle service is a value the store imports (for
resetForLogout), but it alsoneeds to read session meaning.
import typecan't break that cycle because the predicatesare runtime values, not types. Rather than leak
status === "authenticated"back into thelifecycle service, the pure types and predicates live in a dependency-free
stores/session-statusmodule that both the store and its dependencies import. This is acohesive domain module (the session-state model), not a
-types.tscycle band-aid — itowns runtime logic, which is why it is not the
import typecase AGENTS.md warns against.Test plan
bun test(isolated) forauth-store,auth-middleware,lifecycle-service,preferences-menu,inspect-page,onboarding-lifecycle-sync,profile-card,use-platform-gate,local-onboarding-route,general-page,prechat-steps— allgreen. Test mocks and fixtures migrated from the boolean pairs to the enums.
bunx tsc --noEmit— clean (the unions remove the overloadedfalse/boolean pair, so areader that forgets a gate is now a type error rather than a silent race).
bun run lint— clean (0 errors; pre-existing exhaustive-deps warnings only).Root cause analysis
boolean; the "is it known yet?" / "is it still loading?" dimension was bolted on later
as a second boolean rather than promoting the field to a status. Two booleans for a
three-state concept left one combination overloaded.
modeling the state space. Each addition looked locally minimal but globally created an
unrepresentable-states gap, then leaked its encoding to every reader.
funnel (fix(web): gate onboarding Google step on a live platform session, not a cached id #33123), middleware fork (fix(web): await the platform-session probe before the onboarding hosting/welcome fork #33201), and the latent retire-assistant navigation
(refactor(web): consolidate platform-session waits behind one primitive and fork resolver #33203). Three instances of one bug class is the signal to model the state, not patch
readers.
from the start; extract a shared predicate the moment the same derivation appears in a
second module.
docs/STATE_MANAGEMENT.md— "status as one discriminated union, never parallelbooleans" and "where derivation lives: inline vs shared predicate" — capturing the
final wording once now that the reshape is settled.
Coordination
This rewrites the auth store's public shape, which is hot and multi-owner and sits in the
LUM-1788 (gateway-as-BFF) redesign blast radius. Tagging @noaflaherty for a design ack
before merge so the model converges with the BFF track rather than being redone when that
work resumes.
References
Closes LUM-2220.
Link to Devin session: https://app.devin.ai/sessions/15bca57bd4c64a3085cfb80e1f26355a
Requested by: @ashleeradka