feat(ui): Add machine library for authoring flows#8919
Open
alexcarpenter wants to merge 30 commits into
Open
Conversation
- Tighten Actor.send and Actor.can to accept TEvent only, removing the AnyEventObject escape hatch that silently swallowed typos - Add context init option to CreateActorOptions so runtime dependencies can be injected at actor-creation time without module-level closures - Add types.test-d.ts with @ts-expect-error guards and expectTypeOf assertions covering send/can rejection, snapshot types, and assign - Extract delete-organization-machine and leave-organization-machine as module-level factories; migrate both sections to useMachine with the split-component pattern (loader + ready) so the machine is always instantiated with a non-null organization/membership
…mounts In StrictMode (on by default in Next.js dev), React runs effects twice: start → stop → start. The second start() hit the `if (started) return` guard and bailed out, leaving status = 'stopped'. Every subsequent send() then short-circuited, so dialogs driven by useMachine never opened. Fix: stop() resets `started = false` to allow a clean restart, and start() sets status = 'active' before entering state so a restart-after-stop works.
…-fire After stop() + start() (StrictMode, Suspense remount), the actor was re-entering whatever state it was stopped in. If that state had an invoke (e.g. deleting), the async function would fire a second time. Fix: start() now resets context and value to their initial values before entering state, so a restart always begins from idle rather than continuing a half-completed flow. Adds a regression test for the double-invoke case.
…factories useMachine creates the actor once, so any function passed inline to the machine factory is captured from the first render only. If membership or organization changed under the same mount, the old .destroy() would fire. Fix: pin a ref to the prop on every render; the machine receives a stable wrapper that reads from the ref at call time. Adds a regression test that re-renders with a fresh destroy prop before triggering the flow and asserts the fresh function is invoked.
…Effect Replaces the ref-wrapper workaround in section components with the XState pattern: actor.setContext() silently patches context each render so invoke sources always call the latest prop-derived function without stale closures. Section machine factories are now module-level singletons; the destroy/leave function is injected via useMachine options.context instead of a closure.
This reverts commit a521f2e.
TransitionConfig, Transition, InvokeConfig, StateConfig, and MachineConfig now accept an optional TStates extends string = string generic. When callers supply it, initial and transition targets are constrained to that literal union; the default of string preserves the existing behaviour for untyped machines. StateConfig.on keys are now Partial<Record<TEvent['type'], ...>> so event type typos (e.g. 'CANEEL' instead of 'CANCEL') are caught at the createMachine call site rather than silently accepted. Type tests cover both constraints.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 380bd93 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 |
Contributor
📝 WalkthroughWalkthroughThe PR adds a typed Mosaic state-machine runtime, React hooks, and machine examples. It also refactors delete/leave organization flows into controller/view slices, updates the destructive confirmation component, and adds documentation plus alias configuration for the new hook path. ChangesMosaic state-machine rollout
Sequence Diagram(s)sequenceDiagram
participant DeleteOrganizationView
participant useDeleteOrganizationController
participant deleteOrgMachine
participant destroyOrganization
DeleteOrganizationView->>useDeleteOrganizationController: send OPEN / TYPE_CONFIRMATION / CONFIRM
useDeleteOrganizationController->>deleteOrgMachine: actor.send(event)
deleteOrgMachine->>destroyOrganization: invoke destroyOrganization()
destroyOrganization-->>deleteOrgMachine: resolve or reject
deleteOrgMachine-->>DeleteOrganizationView: snapshot.value / context.error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
Adds a `UseMachineOptions` type that extends `CreateActorOptions` with an optional `onDone` callback, fired once when the machine reaches a final state. Auth flows can now declaratively hand off (navigate, redirect) without a `useEffect` watching `snapshot.status` externally.
Resolves a promise with the first snapshot satisfying a predicate. Rejects when the actor becomes non-active without the predicate passing, or when an optional timeout elapses (WaitForTimeoutError). Eliminates manual subscribe wiring in async tests and flow orchestration.
Adds setTimeout-based delayed transitions via a new `after` key on state nodes — the idiomatic way to model OTP expiry and resend cooldowns without leaking timers into components. Timers are cancelled on state exit and on stop(), so they never outlive the state or the actor.
No auth flow or component pulls it in; deferred()+tick() covers test needs. Can be added when something concrete requires it.
setup().fromPromise(fn, { onDone, onError }) mirrors XState v5's fromPromise —
the resolved type of fn flows into e.output in onDone.actions without needing
a cast. Raw src functions still work; e.output is any in that case.
Also adds signInMachine, firstFactorMachine, and a real-world patterns test
suite covering multi-button loading, resend cooldown, error clearing, conditional
routing, post-invoke routing, and typed invoke output.
Adds patterns 7 and 8 to patterns.test.ts: - Pattern 7: initial state with invoke replaces useEffect(fn, []) that does async work on mount (org ticket redemption pattern from SignInStart) - Pattern 8: TYPE event assign guard replaces useLayoutEffect watching a value (phone autofill auto-switch + hasSwitchedByAutofill from SignInStart) Adds Migration 5 to ADOPTION.md mapping the three useEffect patterns in SignInStart.tsx to their machine equivalents, with a reference table.
Machines already stored the API error in context on failure, but the Destructive component had no error prop and neither section component passed the error through. Users saw the dialog reopen with no feedback. Adds an optional error prop to Destructive and surfaces snapshot.context.error from both DeleteOrganizationReady and LeaveOrganizationReady.
- Guard send() and can() with the started flag so callers using createActor()
directly cannot fire events before start() runs entry actions
- Add tests asserting send/can are no-ops before start()
- Add SAFETY comments (per CLAUDE.md) to all non-as-const casts:
createActor.ts (event as never), createMachine.ts ({} as TContext),
setup.ts (fromPromise src cast), firstFactorMachine.ts (three casts)
- Document in delete/leave-organization machines that the CONFIRM guard
is intentionally delegated to the Destructive component
Replace Set with array-based listener tracking using bitwise NOT for safe removal. Inline normalizeTransitions helper to eliminate function overhead. Use short-circuit operators (&&) instead of if statements in hot paths. Collapse toArray into single ternary expression. Minified size: ~130-150 bytes saved (-1.5-2%). All 2316 tests pass. Zero API changes. - Replace Set<SnapshotListener> with array + indexOf + splice pattern (~40 bytes) - Inline normalizeTransitions via map expression (~60 bytes) - Use reverse iteration (i--) in listener loops for safe removal - Apply && operator in hot paths (useMachine, useMachineLogger) (~15 bytes) - Collapse toArray to single-line ternary expression
…javascript into carp/mosaic-state-machine
Adds TransitionFn as a fourth arm of the Transition union. A function
returning { target?, context? } replaces the guard + assign combo for
the common case; returning undefined is treated as unhandled. Works for
both on-event and always transitions, including recheck() re-evaluation.
Contributor
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
packages/ui/src/mosaic/block/destructive.tsx (1)
43-47: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReset effect re-runs every render due to an unstable setter.
useControllableState's setter is memoized on[isControlled, onChange]. Consumers pass an inlineonConfirmationValueChange, soonChange(and thereforesetConfirmValue) gets a new identity each render, causing this effect to fire on every render. It's a no-op whileopen, but while closed it repeatedly invokes the change callback. Consider stabilizing the consumer handlers withuseCallback, or resetting based on a ref to avoid the redundant calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/block/destructive.tsx` around lines 43 - 47, The reset effect in destructive.tsx is re-running every render because `useControllableState` returns a setter that changes identity when `onConfirmationValueChange` is passed inline. Stabilize the consumer callback used by `useControllableState` in `Destructive` (or otherwise avoid depending on the unstable setter in the `useEffect`) so the reset only runs when `open` changes. Update the `useEffect` dependency pattern around `confirmValue`/`setConfirmValue` to prevent repeated change callbacks while the dialog is closed.packages/ui/src/mosaic/machines/signInMachine.ts (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type to the exported
createSignInMachine.Unlike
createDeleteOrgMachine, this exported factory relies on inferred return typing. Annotating it (e.g.StateMachine<SignInContext, SignInEvent>) keeps the public machine surface stable and self-documenting.As per coding guidelines ("Always define explicit return types for functions, especially public APIs") and based on learnings to enforce explicit return type annotations for exported functions and public APIs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/machines/signInMachine.ts` at line 42, Add an explicit return type to the exported createSignInMachine factory so its public API is stable and self-documenting. Update the createSignInMachine signature to annotate the machine type explicitly, following the pattern used by createDeleteOrgMachine and the other exported machine factories in this module, rather than relying on inference.Sources: Coding guidelines, Learnings
packages/ui/src/mosaic/machine/__tests__/delete-organization-machine.ts (1)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: the
event.type === 'TYPE'guard is redundant here.This
TYPEhandler only ever receives theTYPEmember at runtime, so the ternary fallback: {}is dead. Using thesetup()factory'sassign(which narrowsecontextually) would let you writeassign((_, e) => ({ confirmValue: e.value }))without the manual discriminant check. Not blocking since this is a fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/machine/__tests__/delete-organization-machine.ts` around lines 49 - 53, The TYPE transition in delete-organization-machine is over-defensive because the handler only receives the TYPE event, so the `event.type === 'TYPE'` fallback is unnecessary. Update the `TYPE` action in the `assign` for `DeleteOrgContext` to rely on the contextual event typing from `setup()` and directly map `e.value` to `confirmValue`, removing the dead ternary branch.packages/ui/src/mosaic/machine/setup.ts (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an explicit return type for the public
setupfactory.
setupis an exported public API but its return type is inferred. The inner factories are individually annotated, so an explicit interface here would make the public surface self-documenting and guard against accidental shape drift.As per coding guidelines: "Always define explicit return types for functions, especially public APIs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/machine/setup.ts` at line 41, The exported setup factory currently relies on inferred return type, so make its public API explicit by adding a named return type/interface for setup and annotating the setup<TContext, TEvent> function accordingly; use the existing inner factory types in setup.ts to define the returned shape and ensure the exported surface stays self-documenting and stable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/mosaic-machine/SKILL.md:
- Line 26: The wording around setup<TContext, TEvent>() is too broad:
fromPromise should be described as the typed helper only for promise-backed
invoke configurations, not all invokes. Update the SKILL.md guidance near
setup/createMachine/assign/fromPromise to state that fromPromise narrows the
resolved type to e.output in onDone.actions only when the invoke returns a
promise, and avoid implying it applies to every invoke configuration.
In `@packages/ui/src/mosaic/block/destructive.tsx`:
- Around line 64-75: The error message rendered by the destructive block is not
announced to assistive tech because it is only a plain paragraph. Update the Box
in destructive.tsx so the error container uses an accessible live announcement
such as role="alert" or aria-live="assertive" while keeping the existing error
rendering logic in place. Locate the conditional error block in the destructive
component and add the ARIA attribute directly on the Box/p element so screen
readers announce failed destructive actions immediately.
In `@packages/ui/src/mosaic/machine/__tests__/patterns.test.ts`:
- Around line 44-64: The submitting invoke in makeSignInMachine only uses
activeStrategy to choose between emailFn and socialFn, so the selected social
provider is lost. Update the social path in the invoke to pass the chosen
strategy through to socialFn, and make the machine context/actions in
makeSignInMachine preserve the CLICK_SOCIAL strategy value so provider-specific
behavior remains available when this pattern is copied.
In `@packages/ui/src/mosaic/machine/__tests__/wizard-migration.test.tsx`:
- Around line 482-508: The current Wizard AFTER test in
wizard-migration.test.tsx uses a no-op GOTO to the same step, which never
publishes a new snapshot and does not validate useSelector behavior. Update the
StepIndicator/useSelector scenario to trigger a real actor transition that emits
a new snapshot while keeping s.value unchanged, then assert renders does not
increment. Use the existing createWizardMachine, createActor, and useSelector
setup to add a context-only or otherwise unrelated transition that preserves the
selected value but still notifies subscribers.
- Around line 173-175: The test helper actorAt is returning an unstarted actor,
so lifecycle-dependent assertions in wizard-migration.test.tsx are not actually
exercising a running machine. Update actorAt to start the actor before returning
it, and make sure any direct recheck() usage in the affected tests also operates
on a started actor so send, can, and subscribe behavior is validated against the
real lifecycle.
In `@packages/ui/src/mosaic/machine/ADOPTION.md`:
- Around line 391-399: The submit invoke in the `submitting` state is erasing
the typed event by casting to `any`; update the `invoke.src` handler to use the
existing `SignInEvent` shape and carry `identifier` through the typed
event/context instead of `(event as any)`. Keep the example fully typed by
referencing the `submitting` state and `signIn.create` call, and remove the
`any` cast while preserving the same payload flow.
- Around line 124-159: The waitlist machine still closes over external async
dependencies instead of using the factory/context pattern. Update the
`waitlistMachine` example so `clerk`, `navigate`, and `afterJoinWaitlistUrl` are
provided through a machine factory or stored in machine context rather than read
from module scope. Use the existing `createMachine`-based `waitlistMachine` and
its `invoke`/`success.entry` logic as the places to thread those dependencies in
for easier testing and consistency with the authoring guidance.
In `@packages/ui/src/mosaic/machine/createActor.ts`:
- Around line 163-188: The invoke completion/error paths in createActor
currently call commit() unconditionally after takeTransition, unlike send,
startAfterTimers, and recheck. Update the onDone and onError handlers to check
the boolean returned by takeTransition before committing, so blocked transitions
do not publish a new snapshot or notify subscribers when the state does not
change.
In `@packages/ui/src/mosaic/machine/README.md`:
- Around line 48-55: The ASCII diagram block in the README is missing a fenced
language label, which triggers markdownlint and reduces highlighting. Update the
fence around the state machine diagram to include an appropriate tag, using the
diagram block near the FETCH/loading/success/failure section so the markdown
parser and tooling recognize it correctly.
In `@packages/ui/src/mosaic/sections/delete-organization-machine.ts`:
- Around line 49-51: The error handling in deleteOrganizationMachine is passing
a stringified exception directly to the UI, which can expose technical text to
end users. Update the assign handler in delete-organization-machine.ts to derive
a user-facing message from the event error (prefer the error’s message field,
with a friendly fallback when unavailable) instead of using String(event.error),
so delete-organization-view.tsx and Destructive receive a safe message. Apply
the same fix to the matching error assignment in leave-organization-machine.ts
to keep both flows consistent.
In `@packages/ui/src/mosaic/sections/leave-organization-machine.ts`:
- Around line 47-52: The `onError` handler in `leave-organization-machine` is
passing `String(event.error)` through to user-facing state, which can include an
unwanted “Error:” prefix. Update the `assign` logic in the `onError` action to
extract `event.error.message` when the rejection is an `Error`, and fall back to
a sensible string for non-Error values. Then adjust the
`leave-organization-machine.test` expectation to match the cleaned message
instead of the prefixed form.
---
Nitpick comments:
In `@packages/ui/src/mosaic/block/destructive.tsx`:
- Around line 43-47: The reset effect in destructive.tsx is re-running every
render because `useControllableState` returns a setter that changes identity
when `onConfirmationValueChange` is passed inline. Stabilize the consumer
callback used by `useControllableState` in `Destructive` (or otherwise avoid
depending on the unstable setter in the `useEffect`) so the reset only runs when
`open` changes. Update the `useEffect` dependency pattern around
`confirmValue`/`setConfirmValue` to prevent repeated change callbacks while the
dialog is closed.
In `@packages/ui/src/mosaic/machine/__tests__/delete-organization-machine.ts`:
- Around line 49-53: The TYPE transition in delete-organization-machine is
over-defensive because the handler only receives the TYPE event, so the
`event.type === 'TYPE'` fallback is unnecessary. Update the `TYPE` action in the
`assign` for `DeleteOrgContext` to rely on the contextual event typing from
`setup()` and directly map `e.value` to `confirmValue`, removing the dead
ternary branch.
In `@packages/ui/src/mosaic/machine/setup.ts`:
- Line 41: The exported setup factory currently relies on inferred return type,
so make its public API explicit by adding a named return type/interface for
setup and annotating the setup<TContext, TEvent> function accordingly; use the
existing inner factory types in setup.ts to define the returned shape and ensure
the exported surface stays self-documenting and stable.
In `@packages/ui/src/mosaic/machines/signInMachine.ts`:
- Line 42: Add an explicit return type to the exported createSignInMachine
factory so its public API is stable and self-documenting. Update the
createSignInMachine signature to annotate the machine type explicitly, following
the pattern used by createDeleteOrgMachine and the other exported machine
factories in this module, rather than relying on inference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f49b6f3b-9b8a-4440-9f9b-e10800af6fd6
📒 Files selected for processing (38)
.changeset/mosaic-state-machine.md.claude/skills/mosaic-machine/SKILL.mdpackages/swingset/next.config.mjspackages/swingset/src/stories/destructive.stories.tsxpackages/swingset/tsconfig.jsonpackages/ui/src/mosaic/block/destructive.tsxpackages/ui/src/mosaic/machine/ADOPTION.mdpackages/ui/src/mosaic/machine/README.mdpackages/ui/src/mosaic/machine/__tests__/delete-organization-machine.tspackages/ui/src/mosaic/machine/__tests__/machine.test.tspackages/ui/src/mosaic/machine/__tests__/patterns.test.tspackages/ui/src/mosaic/machine/__tests__/useMachine.test.tsxpackages/ui/src/mosaic/machine/__tests__/wizard-migration.test.tsxpackages/ui/src/mosaic/machine/assign.tspackages/ui/src/mosaic/machine/createActor.tspackages/ui/src/mosaic/machine/createMachine.tspackages/ui/src/mosaic/machine/setup.tspackages/ui/src/mosaic/machine/types.test-d.tspackages/ui/src/mosaic/machine/types.tspackages/ui/src/mosaic/machine/useMachine.tspackages/ui/src/mosaic/machines/__tests__/firstFactorMachine.test.tspackages/ui/src/mosaic/machines/__tests__/signInMachine.test.tspackages/ui/src/mosaic/machines/__tests__/test-utils.tspackages/ui/src/mosaic/machines/firstFactorMachine.tspackages/ui/src/mosaic/machines/signInMachine.tspackages/ui/src/mosaic/sections/__tests__/delete-organization-machine.test.tspackages/ui/src/mosaic/sections/__tests__/delete-organization-view.test.tsxpackages/ui/src/mosaic/sections/__tests__/leave-organization-machine.test.tspackages/ui/src/mosaic/sections/__tests__/leave-organization-view.test.tsxpackages/ui/src/mosaic/sections/delete-organization-controller.tsxpackages/ui/src/mosaic/sections/delete-organization-machine.tspackages/ui/src/mosaic/sections/delete-organization-view.tsxpackages/ui/src/mosaic/sections/delete-organization.tsxpackages/ui/src/mosaic/sections/leave-organization-controller.tsxpackages/ui/src/mosaic/sections/leave-organization-machine.tspackages/ui/src/mosaic/sections/leave-organization-view.tsxpackages/ui/src/mosaic/sections/leave-organization.tsxreferences/mosaic-architecture.md
| import { assign } from './assign'; | ||
| ``` | ||
|
|
||
| `setup<TContext, TEvent>()` returns `{ createMachine, assign, fromPromise }`. Use `fromPromise` for all `invoke` configurations — it carries the resolved type to `e.output` in `onDone.actions`. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Narrow fromPromise to promise-based invokes.
fromPromise is the typed helper for promise-backed invokes, so “all invoke configurations” is too broad.
Suggested wording
-Use `fromPromise` for all `invoke` configurations — it carries the resolved type to `e.output` in `onDone.actions`.
+Use `fromPromise` for promise-based `invoke` configurations — it carries the resolved type to `e.output` in `onDone.actions`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `setup<TContext, TEvent>()` returns `{ createMachine, assign, fromPromise }`. Use `fromPromise` for all `invoke` configurations — it carries the resolved type to `e.output` in `onDone.actions`. | |
| `setup<TContext, TEvent>()` returns `{ createMachine, assign, fromPromise }`. Use `fromPromise` for promise-based `invoke` configurations — it carries the resolved type to `e.output` in `onDone.actions`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/mosaic-machine/SKILL.md at line 26, The wording around
setup<TContext, TEvent>() is too broad: fromPromise should be described as the
typed helper only for promise-backed invoke configurations, not all invokes.
Update the SKILL.md guidance near setup/createMachine/assign/fromPromise to
state that fromPromise narrows the resolved type to e.output in onDone.actions
only when the invoke returns a promise, and avoid implying it applies to every
invoke configuration.
Comment on lines
+64
to
+75
| {error && ( | ||
| <Box | ||
| render={p => <p {...p} />} | ||
| sx={t => ({ | ||
| ...t.text('sm'), | ||
| color: t.color.destructive, | ||
| marginBlockStart: t.spacing(2), | ||
| })} | ||
| > | ||
| {error} | ||
| </Box> | ||
| )} |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Announce the error to assistive tech.
The error is rendered as a plain <p>. Screen readers won't announce it when it appears after a failed destructive action. Add role='alert' (or aria-live='assertive') so the failure is conveyed.
Proposed fix
{error && (
<Box
- render={p => <p {...p} />}
+ role='alert'
+ render={p => <p {...p} />}
sx={t => ({As per coding guidelines: "Implement proper ARIA attributes for accessibility in React components".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {error && ( | |
| <Box | |
| render={p => <p {...p} />} | |
| sx={t => ({ | |
| ...t.text('sm'), | |
| color: t.color.destructive, | |
| marginBlockStart: t.spacing(2), | |
| })} | |
| > | |
| {error} | |
| </Box> | |
| )} | |
| {error && ( | |
| <Box | |
| role='alert' | |
| render={p => <p {...p} />} | |
| sx={t => ({ | |
| ...t.text('sm'), | |
| color: t.color.destructive, | |
| marginBlockStart: t.spacing(2), | |
| })} | |
| > | |
| {error} | |
| </Box> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/block/destructive.tsx` around lines 64 - 75, The error
message rendered by the destructive block is not announced to assistive tech
because it is only a plain paragraph. Update the Box in destructive.tsx so the
error container uses an accessible live announcement such as role="alert" or
aria-live="assertive" while keeping the existing error rendering logic in place.
Locate the conditional error block in the destructive component and add the ARIA
attribute directly on the Box/p element so screen readers announce failed
destructive actions immediately.
Source: Coding guidelines
Comment on lines
+44
to
+64
| function makeSignInMachine(socialFn: () => Promise<void>, emailFn: () => Promise<void>) { | ||
| return createMachine({ | ||
| initial: 'idle', | ||
| context: { activeStrategy: null, error: null }, | ||
| states: { | ||
| idle: { | ||
| on: { | ||
| CLICK_SOCIAL: { | ||
| target: 'submitting', | ||
| actions: assign((_, e) => ({ activeStrategy: e.strategy })), | ||
| }, | ||
| SUBMIT_EMAIL: { | ||
| target: 'submitting', | ||
| actions: assign(() => ({ activeStrategy: 'email' as const })), | ||
| }, | ||
| }, | ||
| }, | ||
| submitting: { | ||
| // All entry points converge here. CLICK_SOCIAL while submitting is a no-op — | ||
| // `idle`'s handlers are inactive, so simultaneous triggers are impossible. | ||
| invoke: fromPromise(ctx => (ctx.activeStrategy === 'email' ? emailFn() : socialFn()), { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass the selected social strategy into the invoke.
Line 64 records activeStrategy for UI state, but the social submit function cannot distinguish google from github. Since this test is also pattern documentation, wire the selected strategy into socialFn so copied implementations preserve provider-specific behavior.
Proposed fix
- function makeSignInMachine(socialFn: () => Promise<void>, emailFn: () => Promise<void>) {
+ function makeSignInMachine(socialFn: (strategy: Strategy) => Promise<void>, emailFn: () => Promise<void>) {
@@
- invoke: fromPromise(ctx => (ctx.activeStrategy === 'email' ? emailFn() : socialFn()), {
+ invoke: fromPromise(ctx => {
+ if (ctx.activeStrategy === 'email') {
+ return emailFn();
+ }
+ if (ctx.activeStrategy) {
+ return socialFn(ctx.activeStrategy);
+ }
+ return Promise.reject(new Error('Missing active strategy'));
+ }, {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function makeSignInMachine(socialFn: () => Promise<void>, emailFn: () => Promise<void>) { | |
| return createMachine({ | |
| initial: 'idle', | |
| context: { activeStrategy: null, error: null }, | |
| states: { | |
| idle: { | |
| on: { | |
| CLICK_SOCIAL: { | |
| target: 'submitting', | |
| actions: assign((_, e) => ({ activeStrategy: e.strategy })), | |
| }, | |
| SUBMIT_EMAIL: { | |
| target: 'submitting', | |
| actions: assign(() => ({ activeStrategy: 'email' as const })), | |
| }, | |
| }, | |
| }, | |
| submitting: { | |
| // All entry points converge here. CLICK_SOCIAL while submitting is a no-op — | |
| // `idle`'s handlers are inactive, so simultaneous triggers are impossible. | |
| invoke: fromPromise(ctx => (ctx.activeStrategy === 'email' ? emailFn() : socialFn()), { | |
| function makeSignInMachine(socialFn: (strategy: Strategy) => Promise<void>, emailFn: () => Promise<void>) { | |
| return createMachine({ | |
| initial: 'idle', | |
| context: { activeStrategy: null, error: null }, | |
| states: { | |
| idle: { | |
| on: { | |
| CLICK_SOCIAL: { | |
| target: 'submitting', | |
| actions: assign((_, e) => ({ activeStrategy: e.strategy })), | |
| }, | |
| SUBMIT_EMAIL: { | |
| target: 'submitting', | |
| actions: assign(() => ({ activeStrategy: 'email' as const })), | |
| }, | |
| }, | |
| }, | |
| submitting: { | |
| // All entry points converge here. CLICK_SOCIAL while submitting is a no-op — | |
| // `idle`'s handlers are inactive, so simultaneous triggers are impossible. | |
| invoke: fromPromise(ctx => { | |
| if (ctx.activeStrategy === 'email') { | |
| return emailFn(); | |
| } | |
| if (ctx.activeStrategy) { | |
| return socialFn(ctx.activeStrategy); | |
| } | |
| return Promise.reject(new Error('Missing active strategy')); | |
| }, { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/machine/__tests__/patterns.test.ts` around lines 44 -
64, The submitting invoke in makeSignInMachine only uses activeStrategy to
choose between emailFn and socialFn, so the selected social provider is lost.
Update the social path in the invoke to pass the chosen strategy through to
socialFn, and make the machine context/actions in makeSignInMachine preserve the
CLICK_SOCIAL strategy value so provider-specific behavior remains available when
this pattern is copied.
Comment on lines
+173
to
+175
| const actorAt = (descriptors: StepDescriptor[], value: string) => { | ||
| const actor = createActor(createWizardMachine(descriptors), { snapshot: { value } }); | ||
| return actor; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Start actors before exercising lifecycle-dependent behavior.
Line 172 says actorAt starts the actor, but Lines 173-175 return it unstarted; later tests immediately call send, can, and subscribe. Line 391 also calls recheck() on an unstarted actor, so that assertion can pass without validating re-seat behavior.
Proposed fix
const actorAt = (descriptors: StepDescriptor[], value: string) => {
const actor = createActor(createWizardMachine(descriptors), { snapshot: { value } });
+ actor.start();
return actor;
};
@@
let bOpen = false;
const descriptors: StepDescriptor[] = [{ id: 'a' }, { id: 'b', guard: () => bOpen }];
const actor = createActor(createWizardMachine(descriptors), { snapshot: { value: 'a' } });
+ actor.start();
bOpen = true; // b opens, but a still holds → recheck must not yank forwardAlso applies to: 388-391
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/machine/__tests__/wizard-migration.test.tsx` around
lines 173 - 175, The test helper actorAt is returning an unstarted actor, so
lifecycle-dependent assertions in wizard-migration.test.tsx are not actually
exercising a running machine. Update actorAt to start the actor before returning
it, and make sure any direct recheck() usage in the affected tests also operates
on a started actor so send, can, and subscribe behavior is validated against the
real lifecycle.
Comment on lines
+482
to
+508
| describe('Wizard AFTER — useSelector scopes re-renders to the step slice', () => { | ||
| it('a stepper re-renders on navigation but not on an unrelated context change', () => { | ||
| const machine = createWizardMachine([{ id: 'intro' }, { id: 'details', guard: () => true }]); | ||
| // Seed at 'intro' (furthest-reachable would land on 'details' otherwise). | ||
| const actor = createActor(machine, { snapshot: { value: 'intro' } }); | ||
| actor.start(); | ||
|
|
||
| const renders = vi.fn(); | ||
| function StepIndicator() { | ||
| const current = useSelector(actor, s => s.value); | ||
| renders(); | ||
| return <output data-testid='current'>{current}</output>; | ||
| } | ||
|
|
||
| render(<StepIndicator />); | ||
| expect(renders).toHaveBeenCalledTimes(1); | ||
| expect(screen.getByTestId('current')).toHaveTextContent('intro'); | ||
|
|
||
| // A GOTO to the current step is a no-op → no notify → no re-render. | ||
| act(() => actor.send({ type: 'GOTO', step: 'intro' })); | ||
| expect(renders).toHaveBeenCalledTimes(1); | ||
|
|
||
| // A real navigation changes the selected slice → exactly one re-render. | ||
| act(() => actor.send({ type: 'NEXT' })); | ||
| expect(renders).toHaveBeenCalledTimes(2); | ||
| expect(screen.getByTestId('current')).toHaveTextContent('details'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the useSelector test emit a snapshot whose selected value is unchanged.
The current “unrelated context change” assertion sends GOTO to the current step, which is a true no-op and does not notify subscribers. That does not prove useSelector suppresses re-renders when the actor publishes a new snapshot but s.value is unchanged.
Consider adding a context-only transition in this test and asserting renders remains unchanged after that event.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/machine/__tests__/wizard-migration.test.tsx` around
lines 482 - 508, The current Wizard AFTER test in wizard-migration.test.tsx uses
a no-op GOTO to the same step, which never publishes a new snapshot and does not
validate useSelector behavior. Update the StepIndicator/useSelector scenario to
trigger a real actor transition that emits a new snapshot while keeping s.value
unchanged, then assert renders does not increment. Use the existing
createWizardMachine, createActor, and useSelector setup to add a context-only or
otherwise unrelated transition that preserves the selected value but still
notifies subscribers.
Comment on lines
+391
to
+399
| submitting: { | ||
| invoke: { | ||
| src: (_ctx, event) => signIn.create({ identifier: (event as any).identifier }), | ||
| onDone: { target: 'idle', actions: assign(() => ({ error: null })) }, | ||
| onError: { | ||
| target: 'idle', | ||
| actions: assign((_ctx, e) => ({ error: String(e.error) })), | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the any cast from the submit invoke.
This defeats the typed-event example; the payload is already part of SignInEvent, so the example should carry it through context instead of erasing the type.
As per coding guidelines, avoid any in TypeScript examples.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/machine/ADOPTION.md` around lines 391 - 399, The
submit invoke in the `submitting` state is erasing the typed event by casting to
`any`; update the `invoke.src` handler to use the existing `SignInEvent` shape
and carry `identifier` through the typed event/context instead of `(event as
any)`. Keep the example fully typed by referencing the `submitting` state and
`signIn.create` call, and remove the `any` cast while preserving the same
payload flow.
Source: Coding guidelines
Comment on lines
+163
to
+188
| Promise.resolve(invoke.src(context, event as never)).then( | ||
| output => { | ||
| if (status !== 'active' || token !== invocationToken) { | ||
| return; | ||
| } | ||
| const doneEvent = { type: INVOKE_DONE, output }; | ||
| const transition = pickTransition(normalizeTransition(invoke.onDone, doneEvent), doneEvent); | ||
| if (!transition) { | ||
| return; | ||
| } | ||
| takeTransition(transition, doneEvent); | ||
| commit(); | ||
| }, | ||
| (error: unknown) => { | ||
| if (status !== 'active' || token !== invocationToken) { | ||
| return; | ||
| } | ||
| const errorEvent = { type: INVOKE_ERROR, error }; | ||
| const transition = pickTransition(normalizeTransition(invoke.onError, errorEvent), errorEvent); | ||
| if (!transition) { | ||
| return; | ||
| } | ||
| takeTransition(transition, errorEvent); | ||
| commit(); | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Invoke handlers commit unconditionally, unlike every other transition path.
send (Line 291), startAfterTimers (Line 215), and recheck (Line 356) all gate commit() on the boolean returned by takeTransition, honoring the "entry-blocked → no commit, no notify" contract. The onDone/onError handlers ignore that return value and always call commit(). If an invoke target carries an entry guard that currently fails, takeTransition returns false (state unchanged), yet a fresh snapshot object is still published and subscribers are notified — forcing a no-op re-render through useSyncExternalStore and breaking the referential-stability invariant asserted elsewhere in the suite.
🐛 Gate commit on takeTransition
const doneEvent = { type: INVOKE_DONE, output };
const transition = pickTransition(normalizeTransition(invoke.onDone, doneEvent), doneEvent);
if (!transition) {
return;
}
- takeTransition(transition, doneEvent);
- commit();
+ if (takeTransition(transition, doneEvent)) {
+ commit();
+ }
},
(error: unknown) => {
if (status !== 'active' || token !== invocationToken) {
return;
}
const errorEvent = { type: INVOKE_ERROR, error };
const transition = pickTransition(normalizeTransition(invoke.onError, errorEvent), errorEvent);
if (!transition) {
return;
}
- takeTransition(transition, errorEvent);
- commit();
+ if (takeTransition(transition, errorEvent)) {
+ commit();
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Promise.resolve(invoke.src(context, event as never)).then( | |
| output => { | |
| if (status !== 'active' || token !== invocationToken) { | |
| return; | |
| } | |
| const doneEvent = { type: INVOKE_DONE, output }; | |
| const transition = pickTransition(normalizeTransition(invoke.onDone, doneEvent), doneEvent); | |
| if (!transition) { | |
| return; | |
| } | |
| takeTransition(transition, doneEvent); | |
| commit(); | |
| }, | |
| (error: unknown) => { | |
| if (status !== 'active' || token !== invocationToken) { | |
| return; | |
| } | |
| const errorEvent = { type: INVOKE_ERROR, error }; | |
| const transition = pickTransition(normalizeTransition(invoke.onError, errorEvent), errorEvent); | |
| if (!transition) { | |
| return; | |
| } | |
| takeTransition(transition, errorEvent); | |
| commit(); | |
| }, | |
| ); | |
| Promise.resolve(invoke.src(context, event as never)).then( | |
| output => { | |
| if (status !== 'active' || token !== invocationToken) { | |
| return; | |
| } | |
| const doneEvent = { type: INVOKE_DONE, output }; | |
| const transition = pickTransition(normalizeTransition(invoke.onDone, doneEvent), doneEvent); | |
| if (!transition) { | |
| return; | |
| } | |
| if (takeTransition(transition, doneEvent)) { | |
| commit(); | |
| } | |
| }, | |
| (error: unknown) => { | |
| if (status !== 'active' || token !== invocationToken) { | |
| return; | |
| } | |
| const errorEvent = { type: INVOKE_ERROR, error }; | |
| const transition = pickTransition(normalizeTransition(invoke.onError, errorEvent), errorEvent); | |
| if (!transition) { | |
| return; | |
| } | |
| if (takeTransition(transition, errorEvent)) { | |
| commit(); | |
| } | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/machine/createActor.ts` around lines 163 - 188, The
invoke completion/error paths in createActor currently call commit()
unconditionally after takeTransition, unlike send, startAfterTimers, and
recheck. Update the onDone and onError handlers to check the boolean returned by
takeTransition before committing, so blocked transitions do not publish a new
snapshot or notify subscribers when the state does not change.
Comment on lines
+48
to
+55
| ``` | ||
| FETCH resolves | ||
| idle ─────────► loading ───────────► success | ||
| │ | ||
| │ rejects | ||
| ▼ | ||
| failure | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the ASCII diagram fence.
This block is missing a language tag, which trips the markdownlint warning and weakens syntax highlighting.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/machine/README.md` around lines 48 - 55, The ASCII
diagram block in the README is missing a fenced language label, which triggers
markdownlint and reduces highlighting. Update the fence around the state machine
diagram to include an appropriate tag, using the diagram block near the
FETCH/loading/success/failure section so the markdown parser and tooling
recognize it correctly.
Source: Linters/SAST tools
Comment on lines
+49
to
+51
| actions: assign((_, event) => ({ | ||
| error: String(event.error), | ||
| })), |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't surface a stringified error object to end users.
String(event.error) yields strings like "Error: nope" (and for arbitrary rejections, internal/technical text). This value flows through delete-organization-view.tsx into Destructive's error prop and is rendered verbatim to the user. Extract a user-facing message (e.g. event.error?.message with a friendly fallback) instead of the stringified error. The same pattern exists in leave-organization-machine.ts.
Proposed fix
onError: {
target: 'confirming',
actions: assign((_, event) => ({
- error: String(event.error),
+ error: event.error instanceof Error ? event.error.message : 'Something went wrong. Please try again.',
})),
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| actions: assign((_, event) => ({ | |
| error: String(event.error), | |
| })), | |
| actions: assign((_, event) => ({ | |
| error: event.error instanceof Error ? event.error.message : 'Something went wrong. Please try again.', | |
| })), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/sections/delete-organization-machine.ts` around lines
49 - 51, The error handling in deleteOrganizationMachine is passing a
stringified exception directly to the UI, which can expose technical text to end
users. Update the assign handler in delete-organization-machine.ts to derive a
user-facing message from the event error (prefer the error’s message field, with
a friendly fallback when unavailable) instead of using String(event.error), so
delete-organization-view.tsx and Destructive receive a safe message. Apply the
same fix to the matching error assignment in leave-organization-machine.ts to
keep both flows consistent.
Comment on lines
+47
to
+52
| onError: { | ||
| target: 'confirming', | ||
| actions: assign((_, event) => ({ | ||
| error: String(event.error), | ||
| })), | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid leaking the Error: prefix into user-facing copy.
String(event.error) renders a thrown Error as "Error: <message>" (e.g. "Error: nope"), which is surfaced verbatim to users via LeaveOrganizationView → Destructive's error prop. Prefer extracting the message and falling back for non-Error rejections.
🛠️ Proposed fix
onError: {
target: 'confirming',
actions: assign((_, event) => ({
- error: String(event.error),
+ error: event.error instanceof Error ? event.error.message : String(event.error),
})),
},Note: the assertion in __tests__/leave-organization-machine.test.ts (Line 66) expects 'Error: nope' and would need updating to 'nope'.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onError: { | |
| target: 'confirming', | |
| actions: assign((_, event) => ({ | |
| error: String(event.error), | |
| })), | |
| }, | |
| onError: { | |
| target: 'confirming', | |
| actions: assign((_, event) => ({ | |
| error: event.error instanceof Error ? event.error.message : String(event.error), | |
| })), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/sections/leave-organization-machine.ts` around lines
47 - 52, The `onError` handler in `leave-organization-machine` is passing
`String(event.error)` through to user-facing state, which can include an
unwanted “Error:” prefix. Update the `assign` logic in the `onError` action to
extract `event.error.message` when the rejection is an `Error`, and fall back to
a sensible string for non-Error values. Then adjust the
`leave-organization-machine.test` expectation to match the cleaned message
instead of the prefixed form.
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
1 participant
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
We have several approaches to handling flows in component code, domain modeling and state syncing spread across
useEffects which makes it difficult to without rendering a component. This adds a lightweight state machine library with the goal of standardizing flow patterns.Why not XState
XState is the obvious reference, and the API design follows XState v5 closely but ~9x smaller.
What's in this PR
packages/ui/src/mosaic/machine/delete-organizationandleave-organizationMosaic component sections migrated as a proof of conceptsignInMachineandfirstFactorMachinemodelled but not wired to components yetpatterns.test.tsdocuments how real Clerk patterns map to machine primitivesChecklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Documentation
Tests