refactor(ui): Shared sign-in hooks + snapshot test helper#8652
refactor(ui): Shared sign-in hooks + snapshot test helper#8652alexcarpenter wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 42718ad The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Deduplicate status-routing and error-handling logic from 8 sign-in sub-cards into three shared hooks: useHandleFirstFactorResult, useHandleSecondFactorResult, and useHandleUserLockedError. Also adds a lightweight renderForSnapshot test helper for future snapshot tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| export type SignInStartState = { | ||
| screen: 'form' | 'loading' | 'alternativePhoneCode'; | ||
| identifierAttribute: SignInStartIdentifier; | ||
| identifierValue: string; | ||
| passwordValue: string; | ||
| shouldAutofocus: boolean; | ||
| hasSwitchedByAutofill: boolean; | ||
| alternativePhoneCodeProvider: PhoneCodeChannelData | null; | ||
| cardError: ClerkAPIError | string | undefined; | ||
| isSubmitting: boolean; | ||
| config: SignInStartConfig; | ||
| }; |
There was a problem hiding this comment.
I haven't fully verified, but I think there are a few small issues with this PR, like no protection against double-submitting, the component reverting to the form state while navigating to the next step instead of staying on a loading state etc.
I think they are a result of how this state is modelled. It's not a full state machine, it's kind of halfway there, with screen standing in for the different well-defined states, but then isSubmitting is not modelled as such a state, it's a separate thing, and we are missing a "transitioning" state as well.
The state logic begs for more states than the UI.
I would suggest expanding the state to be something like:
type IdleStatus = { tag: 'idle' };
type AltPhoneCodeStatus = { tag: 'altPhoneCode'; provider: PhoneCodeChannelData };
type FormViewStatus = IdleStatus | AltPhoneCodeStatus;
type SubmittingStatus = { tag: 'submitting'; resumeTo: FormViewStatus };
type TicketProcessingStatus = { tag: 'ticketProcessing' };
type TransitioningStatus = { tag: 'transitioning' };
export type SignInStartStatus =
| IdleStatus
| AltPhoneCodeStatus
| SubmittingStatus
| TicketProcessingStatus
| TransitioningStatus;
export type SignInStartState = {
status: SignInStartStatus;Since the components don't care about all of those states (which might be a reason you didn't want to go there), I'd implement a small view selector:
export type SignInStartViewModel =
| { kind: 'loading' }
| { kind: 'form' }
| { kind: 'altPhoneCode'; provider: PhoneCodeChannelData };
export function getViewModel(state: SignInStartState): SignInStartViewModel {
switch (state.status.tag) {
case 'idle':
return { kind: 'form' };
case 'altPhoneCode':
return { kind: 'altPhoneCode', provider: state.status.provider };
case 'submitting':
return state.status.resumeTo.tag === 'altPhoneCode'
? { kind: 'altPhoneCode', provider: state.status.resumeTo.provider }
: { kind: 'form' };
case 'ticketProcessing':
case 'transitioning':
return { kind: 'loading' };
}
}That would keep most of the UI logic the same, but clean some things up like this, and altPhoneProvider would just always be defined in that state:
- if (screen === 'altPhoneCode' && altPhoneProvider) {
+ if (view.kind === 'altPhoneCode') {I would also go all in on the state machine idea and have the events be coupled to the statuses, so the machine itself rejects impossible state transitions instead of that being enforced only in the components at the time of dispatch. Relying on when dispatches happen means you still can't test the state machine properly without the UI, and tends to lead to bugs down the line when refactoring code.
This would be the reducer:
export function signInStartReducer(state: SignInStartState, event: SignInStartEvent): ReducerResult {
switch (state.status.tag) {
case 'idle':
return reduceIdle(state, state.status, event);
case 'altPhoneCode':
return reduceAltPhoneCode(state, state.status, event);
case 'submitting':
return reduceSubmitting(state, state.status, event);
case 'ticketProcessing':
return reduceTicketProcessing(state, event);
case 'transitioning':
return reduceTransitioning(state, event);
}
}and each sub-reducer would only handle events that are actually valid in that state.
This approach does lead to some more boilerplate, but I think that's worth it. Enforcing the transitions between states might be the most valuable thing about a state machine to me:
**idle**
- SET_IDENTIFIER
- SET_PASSWORD
- SWITCH_IDENTIFIER
- AUTOFILL_PHONE_SWITCH
- SELECT_ALT_PHONE_PROVIDER → altPhoneCode
- SUBMIT → submitting
- SET_CARD_ERROR
**altPhoneCode**
- SET_IDENTIFIER
- CLEAR_ALT_PHONE_PROVIDER → idle
- SUBMIT → submitting
- SET_CARD_ERROR
**submitting**
- SUBMIT_SUCCESS → transitioning | idle | altPhoneCode
- SUBMIT_ERROR → submitting | transitioning | idle | altPhoneCode
- SET_CARD_ERROR
**ticketProcessing**
- TICKET_SUCCESS → transitioning | idle
- TICKET_ERROR → submitting | transitioning | idle
- TICKET_DONE → transitioning | idle
- SET_CARD_ERROR
**transitioning**
- SET_CARD_ERROR
Summary
useHandleFirstFactorResult,useHandleSecondFactorResult,useHandleUserLockedErrorrenderForSnapshottest helper with minimal provider stack for future snapshot testsTest plan
Stack: PR 1 of 4 — Foundation. PRs 2-4 build on these shared hooks to make sub-cards prop-driven and add snapshot tests.
🤖 Generated with Claude Code