diff --git a/.changeset/mosaic-docs-align.md b/.changeset/mosaic-docs-align.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/mosaic-docs-align.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/.claude/skills/mosaic/SKILL.md b/.claude/skills/mosaic/SKILL.md index fcae97157a8..f588f08e9ad 100644 --- a/.claude/skills/mosaic/SKILL.md +++ b/.claude/skills/mosaic/SKILL.md @@ -63,16 +63,21 @@ canonical contract for the whole design system — the `--cl-*` tokens, the architecture" section that defines the split. Read it for the _what_; this skill is the _how-to_. -`packages/mosaic/src/features/user-button/` is the fullest worked example of the split -in the repo — model, controller, view, wrapper, types, messages, and a feature -test. Copy from it. Tests go in its `__tests__/` folder, as -`packages/mosaic/docs/testing.md` describes under "Test placement". +`packages/mosaic/src/features/switcher/` is the fullest worked example of the split +in the repo — model, controller, view, wrapper, types, layout and styles. Copy from +it. `UserButton` wraps it, and its feature test is in +`packages/mosaic/src/features/user-button/__tests__/`. Tests go in a `__tests__/` +folder, as `packages/mosaic/docs/testing.md` describes under "Test placement". `packages/mosaic/AGENTS.md` loads for any work in the package. It maps common problems (errors, pending state, focus after removal, debouncing, confirm steps) to the shared helper that already solves them, and says to ask the user before adding a new pattern. Check it before writing any of those by hand. +Its "Shaping a fix" section covers bug fixes: fix the shared helper rather than +wrapping it, and treat a changed shared signature as a new pattern. Read it +before any `fix` PR. + ## Which reference to read Use the "Task guides" table in `packages/mosaic/AGENTS.md`. It is the shared diff --git a/packages/mosaic/AGENTS.md b/packages/mosaic/AGENTS.md index c5840e8f8a0..82744a0bc13 100644 --- a/packages/mosaic/AGENTS.md +++ b/packages/mosaic/AGENTS.md @@ -98,6 +98,22 @@ Stop and check the table if you are writing any of these in a feature: The form rule applies when you add or rewrite a form. Some existing forms predate it. +## Shaping a fix + +A bug in a feature is often a gap in a shared helper. These rules apply to `fix` PRs as much as to features. + +- **Fix the owner, don't wrap it.** When a shared helper (`src/hooks/`, `src/blocks/`, `src/components/`, `src/machine/`, or `@clerk/shared`) behaves wrongly, change the helper and its tests, then delete the workarounds. Do not spread a helper's return value to swap one method, copy a `||` that undoes one of its fields, or add a second hook beside it for one caller. +- **A changed shared signature is a new pattern.** A new parameter, a return type that goes from `void` to a promise, or a returned field whose meaning changes all go through "When nothing fits: ask first". +- **One source of truth for busy.** When a machine, `usePendingAction` or `useForm` already knows an action is running, derive the flag from it: `isDisabled: actions.isPending || remove.isPending`. Do not mirror it into a second lock, or turn machine state into a promise to feed one. +- **Handle staleness through lifecycle.** A new identity remounts through a React `key`. A busy state ignores close (Controllers → Rules). Do not thread run ids or `AbortSignal`s through the model, controller and machine. +- **Extend a contract with an options object.** Make a field required when every caller has it: `connect(strategy, { walletName, onTarget })`, not `connect(strategy, walletName?, onTarget?)`. +- **Pass shared results through whole.** The view gets `form`, not an object that renames `form.id`, `form.isSubmitting` and `form.handleSubmit`. +- **Keep controller state off model types.** Pending and error flags the controller sets go on a row type the controller builds, not as optional fields on the type the model returns. +- **Narrow by mounting.** A hook that needs loaded data takes it non-null. Mount its component once the model is ready instead of checking for `null` inside (Controllers → Composition and dialog lifetime). +- **Machines:** `assign` stays pure. An effect the machine starts runs in `invoke`. The runtime has no spawned actors, so effects that run side by side, like `useForm`'s per-field `validateAsync`, run in the hook and report back by event, and the machine drops stale results. A phase of an action the user can see (queued, pending, failed) is a named state, not a context field plus a predicate. Screens that can sit on top of any state, like reverification's method picker and help, may be a stack in context, because the runtime has no parallel or history states. + +If code or a test contradicts a rule in these docs, do not copy the code. Stop and ask which one is right, and fix the loser in the same PR. + ## Nested radii A part inset from a rounded surface gets its radius from that surface: `max(0px, calc(outer - border - padding))`. Do not pick a fixed step, or it stops matching when `--cl-radius-*` or `--cl-spacing` is overridden. This applies to small insets. When the inset is about as large as the outer radius or larger (Card content, TagInput tags), the part keeps its own radius. diff --git a/packages/mosaic/ARCHITECTURE.md b/packages/mosaic/ARCHITECTURE.md index c9b4c5e9c30..a743247fff0 100644 --- a/packages/mosaic/ARCHITECTURE.md +++ b/packages/mosaic/ARCHITECTURE.md @@ -59,10 +59,15 @@ A theme styles a part from CSS, against that class and its attributes: } ``` -A part exposes no `className` or `style` prop. Inside `@clerk/mosaic`, code that renders a part and needs to nudge it passes StyleX atoms through `xstyle`, declared in its own `stylex.create` and composed last so they win: +A part exposes no `className` or `style` prop. Inside `@clerk/mosaic`, code that renders a part and needs to nudge it passes StyleX atoms through `xstyle`, declared in its own `*.styles.ts` file and composed last so they win: + +```ts +// reverification.styles.ts +export const styles = stylex.create({ helpText: { textAlign: 'center' } }); +``` ```tsx -const styles = stylex.create({ helpText: { textAlign: 'center' } }); +import { styles } from './reverification.styles'; /` and is split by role, one file per layer, prefixed with the feature name. Use `.ts` unless the file renders JSX: ```text -user-button.model.ts(x) // Clerk adapter — the only file that imports Clerk -user-button.controller.ts(x) // local state (React state or a machine) + action wrapping -user-button.view.tsx // rendering only -user-button.tsx // composition wrapper and the public props type +switcher.model.ts(x) // Clerk adapter — the only file that imports Clerk +switcher.controller.ts(x) // local state (React state or a machine) + action wrapping +switcher.view.tsx // rendering only +switcher.tsx // composition wrapper and the public props type ``` Supporting files carry the parts that would otherwise bloat those four: ```text -user-button.types.ts // the data contract the model and view both agree on -user-button.messages.ts // every string the surface renders, as one `as const` object -user-button.layout.ts // pure derivation (which affordance goes in which slot) -user-button.utils.ts // pure helpers -user-button.styles.ts // `stylex.create` atoms (see the StyleX authoring rules) +.types.ts // the data contract the model and view both agree on +.messages.ts // every string the surface renders, as one `as const` object +.layout.ts // pure derivation (which affordance goes in which slot) +.utils.ts // pure helpers +.styles.ts // `stylex.create` atoms (see the StyleX authoring rules) .dialog.tsx // a dialog the view opens, when it would crowd the view file ``` @@ -270,10 +275,10 @@ Two shapes, chosen by whether the slice fetches its own data. wrapper resolves the model, hands it to the controller, and branches on `status`: ```tsx -export function UserButton(props: UserButtonProps = {}) { +export function Switcher(props: SwitcherProps = {}) { const { showTriggerLabel, mode, fallback, ...options } = props; - const model = useUserButtonModel(options); - const controller = useUserButtonController(model, { mode }); + const model = useSwitcherModel(options); + const controller = useSwitcherController(model, { mode }); if (controller.status === 'loading') { return <>{fallback}; @@ -285,7 +290,7 @@ export function UserButton(props: UserButtonProps = {}) { const { status: _status, ...viewController } = controller; return ( - @@ -313,12 +318,12 @@ branches on one value rather than on a scatter of `isLoaded` flags. Every callba it exposes is a plain function over plain ids — never a Clerk resource: ```tsx -export type UserButtonModel = +export type SwitcherModel = | { status: 'loading' } | { status: 'hidden' } - | (UserButtonData & UserButtonCallbacks & { status: 'ready'; organizationsEnabled: boolean }); + | (SwitcherData & SwitcherCallbacks & { status: 'ready'; organizationsEnabled: boolean }); -export function useUserButtonModel(options?: UserButtonModelOptions): UserButtonModel { +export function useSwitcherModel(options?: SwitcherModelOptions): SwitcherModel { const { isLoaded: isUserLoaded, user } = useUser(); const { isLoaded: isSessionLoaded, session } = useSession(); const clerk = useClerk(); @@ -353,19 +358,19 @@ local state, wraps the model's actions to drive pending state, keeps the surface stable while an action runs, and closes the surface on the actions that should close it. -How it holds that state is a complexity call. `UserButton` earns a machine — an +How it holds that state is a complexity call. `Switcher` earns a machine — an action is async, `open` and `pendingKey` constrain each other, and dismissing must not abandon an in-flight invoke — so the machine is declared in the same file with `setup()`, and its context carries the injected effect. Abridged from `features/switcher/switcher.controller.tsx`: ```tsx -const { createMachine, assign, fromPromise } = setup(); +const { createMachine, assign, fromPromise } = setup(); const settled = { pendingKey: null, frozenModel: null }; -const userButtonMachine = createMachine({ - id: 'userButton', +const switcherMachine = createMachine({ + id: 'switcher', initial: 'idle', context: { open: false, pendingKey: null, frozenModel: null, run: () => Promise.resolve(), closeOnSuccess: false }, states: { @@ -400,14 +405,20 @@ const userButtonMachine = createMachine({ }, { target: 'idle', actions: assign(() => settled) }, ], - onError: { target: 'idle', actions: assign(() => settled) }, + onError: { + target: 'idle', + actions: [ + assign(() => settled), + (_, event) => console.error('[Clerk] User button action failed', event.error), + ], + }, }), }, }, }); -export function useUserButtonController(model: UserButtonModel, options = {}): UserButtonController { - const [{ context }, send] = useMachine(userButtonMachine); +export function useSwitcherController(model: SwitcherModel, options = {}): SwitcherController { + const [{ context }, send] = useMachine(switcherMachine); const displayPendingKey = useSpinDelay(context.pendingKey, { delay: 0 }); // While an action runs, render the model it started from, not the live one. @@ -433,7 +444,7 @@ export function useUserButtonController(model: UserButtonModel, options = {}): U open: context.open, onOpenChange: next => send(next ? { type: 'OPEN' } : { type: 'CLOSE' }), pendingKey: displayPendingKey, - onSelectOrganization: runAction(userButtonBusyKeys.selectOrganization, resolvedModel.onSelectOrganization, true), + onSelectOrganization: runAction(switcherBusyKeys.selectOrganization, resolvedModel.onSelectOrganization, true), // … }; } @@ -474,7 +485,7 @@ export function useUserProfileEditUsernameController({ username = '', required = ``` Reaching for a machine here would produce a two-state machine with one event, -which is a boolean spelled long. Reaching for `useState` in `UserButton` would +which is a boolean spelled long. Reaching for `useState` in `Switcher` would produce the flag soup the machine exists to prevent. `src/machine/ADOPTION.md` has the criteria and worked before/afters for the calls in between. @@ -485,7 +496,7 @@ derived — `open`, `pendingKey`, an absent callback — never on a machine snap so the view needs neither the machine nor Clerk: ```tsx -export function UserButtonView({ open, onOpenChange, pendingKey, onSignOutAll, ...data }: UserButtonProps) { +export function SwitcherView({ open, onOpenChange, pendingKey, onSignOutAll, ...data }: SwitcherViewProps) { return ( {m.accounts.signOutAll} diff --git a/packages/mosaic/docs/controllers.md b/packages/mosaic/docs/controllers.md index 94f05e37503..f07c2c0cf8e 100644 --- a/packages/mosaic/docs/controllers.md +++ b/packages/mosaic/docs/controllers.md @@ -65,7 +65,7 @@ error and releases the lock. It is not for: `useConfirmController` earns a machine: the confirm is async, a failure must land back on the confirming step with a reason, and a cancelled reverification must -close without one. `useUserButtonController` earns one because `open` and +close without one. `useSwitcherController` earns one because `open` and `pendingKey` constrain each other and dismissing must not abandon an in-flight invoke. @@ -161,34 +161,35 @@ it. The runtime is documented next to the code: are the full API. Shared async test helpers are in `src/__tests__/async.ts`. -A machine never calls Clerk. The effect it invokes arrives through context as a -plain function, seated by `useMachine(machine, { context })`, which re-seats -context every render so the machine always invokes the latest one. From -`user-profile-rename-passkey.controller.ts`: +A machine never calls Clerk. The effect it runs arrives as a plain function. An +effect that differs per action rides on the event and is assigned to context. +From `blocks/confirm/confirm.controller.ts`: ```ts -saving: { - invoke: fromPromise(async context => context.onRename?.(context.passkeyId, context.name), { - onDone: { target: 'idle' }, +confirming: { + on: { + CONFIRM: { target: 'pending', actions: assign((_, event) => ({ run: event.run, error: undefined })) }, + }, +}, +pending: { + invoke: fromPromise(context => context.run(), { + onDone: { target: 'idle', actions: assign(() => ({ error: undefined, open: false })) }, onError: { - target: 'editing', + target: 'confirming', actions: assign((_, event) => ({ error: toLocalizableError(event.error) })), }, }), -} - -const [snapshot, send] = useMachine(userProfileRenamePasskeyMachine, { - context: { passkeyId: id, savedName: name, onRename }, -}); +}, ``` -An effect that differs per action can ride on the event instead and be assigned -to context, as `confirmationMachine` does with `CONFIRM` -(`blocks/confirmation/confirmation.controller.ts`). +An effect fixed for the controller's lifetime is seated by +`useMachine(machine, { context })`, which re-seats context every render so the +machine always runs the latest one (`onRedirect` in +`features/waitlist/waitlist.controller.ts`). Context holds the `ErrorDescription`, never copy (see "Errors"). A machine whose surface shows no error still logs it, so a failure is never silent -(`user-button.controller.tsx`). +(`features/switcher/switcher.controller.tsx`). ## Responsibilities @@ -334,7 +335,7 @@ One path, so every error a user sees goes through the `errors.*` catalog: dialogs), or `useForm` (forms). The first two call `toLocalizableError(cause)` from `utils/errors.ts`. Use one of them instead of catching in a feature. A controller with its own machine maps in the machine's - `onError` (`user-profile-rename-passkey.controller.ts`). + `onError` (`organization-list.controller.ts`). - **Machines that display all errors use `toLocalizableErrors(cause)`.** It preserves each error and its parameter name. Reverification stores this array, routes matching parameters to the active field and everything else to the diff --git a/packages/mosaic/docs/models.md b/packages/mosaic/docs/models.md index 545dfb49553..4fca659dae8 100644 --- a/packages/mosaic/docs/models.md +++ b/packages/mosaic/docs/models.md @@ -14,10 +14,10 @@ A discriminated union on `status`, so consumers branch on one value instead of a scatter of `isLoaded` flags: ```tsx -export type UserButtonModel = +export type SwitcherModel = | { status: 'loading' } | { status: 'hidden' } - | (UserButtonData & UserButtonCallbacks & { status: 'ready'; organizationsEnabled: boolean }); + | (SwitcherData & SwitcherCallbacks & { status: 'ready'; organizationsEnabled: boolean }); ``` `hidden` is an **answer**, not an absence: signed out is settled, so the wrapper @@ -53,17 +53,23 @@ drops the fallback instead of holding the space open. Keep the two apart. The view hides the affordance an absent callback drives, so the model never has to describe UI. -- **Own revalidate timing.** Call `.revalidate()` / `.reload()` after a mutation, - from inside the callback that made it. Deciding _when_ is model logic: +- **Own cache updates.** Update the cache after a mutation, from inside the + callback that made it. Deciding _how_ is model logic: patch the returned + resource in with `replaceInPages` on success, and revalidate on failure, since + a failed mutation might mean stale state: ```tsx onAcceptSuggestion: async suggestionId => { try { - await suggestionData.find(s => s.id === suggestionId)?.accept(); - } finally { - // Always revalidate — a failed accept might be stale state. allSettled never throws, + const accepted = await suggestionData.find(s => s.id === suggestionId)?.accept(); + if (accepted) { + await userSuggestions.setData?.(pages => replaceInPages(pages ?? [], accepted)); + } + } catch (error) { + // A failed accept might be stale state. allSettled never throws, // so a failed revalidate doesn't look like a failed accept. await Promise.allSettled([userSuggestions.revalidate?.()]); + throw error; } }, ``` diff --git a/packages/mosaic/docs/skeletons.md b/packages/mosaic/docs/skeletons.md index 0bb0bfe3300..a605af4ab72 100644 --- a/packages/mosaic/docs/skeletons.md +++ b/packages/mosaic/docs/skeletons.md @@ -109,7 +109,7 @@ peaking in the middle, sweeps left to right across two bone widths, over 1.6s Not decided by the skeleton. The component that owns the loading state shows it for as long as the data is loading. The planned gate is -`useSpinDelay(loading, { delay: 150, minDuration: 500 })` with the skeleton mounted +`useSpinDelay(loading || null, { delay: 150, minDuration: 500 }) !== null` with the skeleton mounted but `visibility: hidden` until the delay passes, so a fast load never flashes it and nothing shifts when it appears. diff --git a/packages/mosaic/docs/views.md b/packages/mosaic/docs/views.md index bc5c08a00d9..3025faadcaf 100644 --- a/packages/mosaic/docs/views.md +++ b/packages/mosaic/docs/views.md @@ -13,7 +13,7 @@ The view renders plain props and calls plain callbacks. Nothing else. machine guard or works out for itself whether an action is allowed. ```tsx -export function UserButtonView({ open, onOpenChange, pendingKey, onSignOutAll, ...data }: UserButtonProps) { +export function SwitcherView({ open, onOpenChange, pendingKey, onSignOutAll, ...data }: SwitcherViewProps) { return ( {m.accounts.signOutAll} @@ -37,7 +37,7 @@ export function UserButtonView({ open, onOpenChange, pendingKey, onSignOutAll, . Two shapes, chosen by whether the slice fetches its own data: - A **wrapper composes** model + controller + view, and the view is a pure - function of props (`user-button.tsx`). + function of props (`switcher.tsx`). - A **leaf view owns its controller** and takes the effect as a prop (`UserProfileUsernameRowView` calls `useUserProfileEditUsernameController` with its `onSubmit`). Still no Clerk — the effect arrives from above. diff --git a/packages/mosaic/src/machine/ADOPTION.md b/packages/mosaic/src/machine/ADOPTION.md index dee1600a84c..710a375ff47 100644 --- a/packages/mosaic/src/machine/ADOPTION.md +++ b/packages/mosaic/src/machine/ADOPTION.md @@ -794,8 +794,8 @@ collocated with the value that drives it. The controllers that already made this call are the best reference: -- `blocks/confirmation/confirmation.controller.ts`: a machine -- `blocks/destructive/destructive.controller.ts`: one `useState` holding a status union +- `blocks/confirm/confirm.controller.ts`: a machine +- `features/user-profile/user-profile-profile-section/user-profile-edit-username.controller.ts`: one `useState` beside `useForm` - `features/switcher/switcher.controller.tsx`: a machine - `hooks/use-pending-action.ts`: keyed `useState` plus a lock diff --git a/packages/mosaic/src/machine/README.md b/packages/mosaic/src/machine/README.md index 6bad81cb237..f4205427a94 100644 --- a/packages/mosaic/src/machine/README.md +++ b/packages/mosaic/src/machine/README.md @@ -83,7 +83,7 @@ const loader = createMachine({ }, onError: { target: 'failure', - actions: assign((_ctx, event) => ({ error: String(event.error) })), + actions: assign((_ctx, event) => ({ error: toLocalizableError(event.error) })), }, }), }, @@ -137,7 +137,7 @@ const loader = createMachine({ }, onError: { target: 'failure', - actions: assign((_, e) => ({ error: String(e.error) })), // e: ErrorInvokeEvent + actions: assign((_, e) => ({ error: toLocalizableError(e.error) })), // e: ErrorInvokeEvent }, }), }, @@ -250,7 +250,7 @@ you can't easily click to (a `deleting` state hidden behind a 2-second mutation, a guard-gated wizard step), `mockActor` drops you straight in: ```ts -const actor = mockActor(loader, { value: 'failure', context: { error: 'Network error' } }); +const actor = mockActor(loader, { value: 'failure', context: { error: { cause: new Error('Network error') } } }); render(); // snapshot a state you'd otherwise have to provoke ```