feat: DAH-0000 Read the signed-in user from one session provider - #3058
Draft
fwextensions wants to merge 1 commit into
Draft
feat: DAH-0000 Read the signed-in user from one session provider#3058fwextensions wants to merge 1 commit into
fwextensions wants to merge 1 commit into
Conversation
The React app asked "who is signed in" in several different ways: some call sites read the Clerk flag then branched to useAuth or UserContext, others checked isTokenValid directly. Each site re-derived the same answer from booleans, and the combinations that were never covered turned into bugs. Introduce a single SessionProvider that owns that decision, and a Session discriminated union that call sites consume with an exhaustive switch, so an unhandled state is a type error instead of a silent fallthrough. useAuth is confined to the Clerk source component, which is only mounted inside the ClerkProvider that withAppSetup adds under the flag. Migrating the existing call sites onto it fixes three defects: - ListingDetailsApply rendered an apply button with an empty href while the session was still resolving; it is now disabled until the session is known. - IdleTimeout called Devise's timeOut even under Clerk, so a timed-out user landed on the sign-in page still signed in. It now ends the session through the facade, and passes the alert reason as the redirect path so the query param survives BaseIdleTimeout's own navigation. - Layout forked its sign-out between the two backends; it now has one path. Header chrome reads hasCredentials rather than the async union, so signed-in Devise users no longer see a "Sign In" flash on first paint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Description
Replaces the per-call-site "is the Clerk flag on?" branching with one
SessionProviderthat owns the answer, exposing session state as a discriminated union so each consumer handles every case or fails to compile. Migrating the existing consumers onto it fixes three defects: the apply button's emptyhrefwhile loading (DAH-4340), an idle timeout that never ended a Clerk session, andLayout's duplicated sign-out.What's here
New in
app/javascript/authentication/session/:types.tsSessionunion:Loading/SignedOut/SignedInWithoutProfile/SignedIn, with each variant carrying its own payloadSessionProvider.tsxuseSession.ts,SessionContext.ts,index.tsMigrated:
withAuthentication,Layout,IdleTimeout,ListingDetailsApply.Why a provider rather than a hook.
useAuth()throws outside aClerkProvider, andwithAppSetupmountsClerkProvideronly when the flag is on. So any hook callinguseAuth()unconditionally crashes the Devise path.useAuth()is confined toClerkSessionSource, which is only ever mounted insideClerkProvider.Why
hasCredentialsexists alongside the union. The header needs a synchronous answer or signed-in Devise users see "Sign In" for one paint and watch it flip. It's documented as chrome-only — please push back if you see it gating access or data anywhere.One deliberate
providercheck remains. Under Devise the Angular flow still accepts anonymous applications, so the apply button links straight to the form; under Clerk it routes signed-out users to sign in first. That's product behaviour, not plumbing, and it's commented as such.Review instructions
Applies to review/staging, and to local dev. Everything below is on the listing details page and the account area; there is no new UI.
The Clerk path needs
?featureFlag[temp.webapp.auth.clerk]=trueon the URL; omit it for the Devise path.Accounts to use. For the Devise path (section 3), review apps preload
test@test.com/abcd1234viarake preload:useron postdeploy (app.json:35) — already confirmed, with a Salesforce Contact attached. That account does not work on the Clerk path, because it exists only in the Railsuserstable and Clerk sign-in never consults it. Sections 1 and 2 therefore need a Clerk identity: either a test account created directly in the Clerk instance, or one created through/create-accounton the review app.If you create one through the sign-up flow, a delayed verification email can trap you: clicking "Send again" invalidates the earlier code, so if the first email lands after you have requested a second, entering the first code fails. The on-screen error does say "Sent more than 1 code? Use the newest one" — trust it and use the most recent email. This is Clerk's behaviour and predates this branch; it is noted because it will bite anyone walking these steps. A newly created account also lands on
/add-profilefirst, so complete that before returning to a listing in section 1.1. Apply button no longer dead-clicks (DAH-4340)
2. Idle timeout actually signs Clerk users out
/account./accountbecause only the Devise sign-out ran.3. Devise path unchanged (regression check)
Not yet performed end-to-end by me — I ran the unit tests and typechecker, not the browser flows. The 30-minute idle wait in particular wants someone with a real environment. Flagging that rather than checking the box below.
Notes for reviewers
accountUtils.setupUserContextnow also stubsSessionContext; without it, migrated components see theLoadingdefault and rendernull. That one change repaired eight suites. It takes aclerkEnabledparam defaulting to Clerk, matching what the suites using it already mock.userEvent5s timeouts under parallel load. At--testTimeout=30000only two suites fail, both also fail onmain, and both pass in isolation on this branch.tsc --noEmitand ESLint are clean. Leaving the "all checks pass" box unchecked for CI to confirm.delegatestate.Before requesting eng review
Version Control
angularif it contains updates to Angular codetype: TICKET-NUMBER Descriptionformat, useDAH-000if it does not need a ticketurgent: Descriptionformat if it is urgent and does not need a ticketCode quality
Code conventions
.scssstylesheets andui-seedstokens, rather than inline styles or TailwindReview instructions
Request eng review
needs reviewlabelHousing Enggroup to automatically assign reviewers, and/or assign specific engineersBefore merging
Request product acceptance (PA) testing
needs product acceptancelabel)temp.webapp.auth.clerkflag; the Devise path changes are not flagged, so section 3 above is the regression check that matters