diff --git a/.changeset/mosaic-review-guidance.md b/.changeset/mosaic-review-guidance.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/mosaic-review-guidance.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/.claude/skills/mosaic/SKILL.md b/.claude/skills/mosaic/SKILL.md index f32ac1b90cc..3554edae36f 100644 --- a/.claude/skills/mosaic/SKILL.md +++ b/.claude/skills/mosaic/SKILL.md @@ -68,20 +68,11 @@ adding a new pattern. Check it before writing any of those by hand. ## Which reference to read -| You are… | Read | -| ------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------- | -| Building on / authoring a headless primitive (`src/primitives/`) | `packages/mosaic/docs/headless.md` | -| Styling a component (tokens, `stylex.create`, `themeProps`, CSS build) | `packages/mosaic/docs/stylex.md` | -| Building an enter/exit transition, a row that expands and collapses, or any motion that reads as wrong | `packages/mosaic/docs/motion.md` | -| Writing the model (the Clerk adapter, `status`, permissions) | `packages/mosaic/docs/models.md` | -| Writing the controller (local state, pending, action wrapping) | `packages/mosaic/docs/controllers.md` | -| Authoring or debugging a state machine, or wiring one to React | `packages/mosaic/docs/controllers.md` → "Machines", then `src/machine/README.md` | -| Writing the view (rendering plain props) | `packages/mosaic/docs/views.md` | -| Testing a feature (feature tests, unit tests) | `packages/mosaic/docs/testing.md` | -| Migrating a legacy component into Mosaic (the end-to-end workflow) | `packages/mosaic/docs/migration.md` | -| Running the parity audit that guards a migration | `packages/mosaic/docs/parity-audit.md` | - -The migration workflow (`migration.md`) ties the flow references together: it +Use the "Task guides" table in `packages/mosaic/AGENTS.md`. It is the shared +task index for agents, including those that do not discover this skill. Read +the guide for the layer or behavior you are changing. + +The migration workflow (`packages/mosaic/docs/migration.md`) ties the flow references together: it treats the legacy component as the spec and drives you through the model, controller, and view layers, then verifies parity with `parity-audit.md`. @@ -114,8 +105,6 @@ this axis. Which surface the dialog holds, and the geometry that comes with it. ``` -This is the opposite of the rule for **code**, where a comment earns its place by -explaining what the code cannot say for itself — the cascade fight behind a -`null`, the measured reason a duration is what it is. Keep that reasoning where a -future maintainer will hit it, which is the source file. The trade you rejected -belongs in a code comment or the PR description; the docs get the conclusion. +Follow the root `AGENTS.md` rule for code comments. Put implementation rationale +and rejected alternatives in the PR description. Component docs describe the +resulting API. diff --git a/packages/mosaic/AGENTS.md b/packages/mosaic/AGENTS.md index 2cda7b34488..6a7d7670d7d 100644 --- a/packages/mosaic/AGENTS.md +++ b/packages/mosaic/AGENTS.md @@ -5,6 +5,23 @@ Mosaic is Clerk's next-generation component library. These rules apply to everyt - `ARCHITECTURE.md` (this folder) is the contract: tokens, the `.cl-` + `data-` styling API, the CSS build, and the model → controller → view split. - `docs/` is the how-to, one file per task: StyleX authoring, motion, headless primitives, each flow layer, testing, and migration. +## Task guides + +Read the guide for the task you are doing. Paths below are relative to this package. + +| Task | Read | +| ----------------------------------------- | --------------------------------------------------------------------------------- | +| Author a headless primitive | [Headless primitives](docs/headless.md) | +| Style a component or change the CSS build | [StyleX](docs/stylex.md) | +| Add or debug motion | [Motion](docs/motion.md) | +| Write a model | [Models](docs/models.md) | +| Write a controller | [Controllers](docs/controllers.md) | +| Author or debug a state machine | [Controllers](docs/controllers.md), then [Machine runtime](src/machine/README.md) | +| Write a view | [Views](docs/views.md) | +| Test a feature | [Testing](docs/testing.md) | +| Migrate legacy behavior | [Migration](docs/migration.md) | +| Review migration parity | [Parity audit](docs/parity-audit.md) | + ## Layers in one paragraph A flow lives in `src/features//`. The **model** (`*.model.ts(x)`) is the only file that reads Clerk hooks or calls Clerk resources, and answers with plain data, plain callbacks and a `status`. The **controller** (`*.controller.ts(x)`) holds local state and wraps the model's callbacks for pending, errors and closing. The **view** (`*.view.tsx`) renders plain props. No Clerk resource reaches the controller and no machine snapshot reaches the view. @@ -25,6 +42,7 @@ Most problems a feature hits already have a shared answer. Find the row for your | Moving focus after a list row is removed | `useListRemovalFocus` (`src/hooks/`) | | A debounced, abortable async check as the user types | `useDebouncedAsync` (`src/hooks/`) | | A spinner that should not flash or flicker | `useSpinDelay` (`src/hooks/`) | +| Current time for display labels | `useNow` + `MosaicProvider` ([#10098](https://github.com/clerk/javascript/pull/10098), once available) | | An inline message row that animates open and closed | `useHeldMessage`, `useMessageHeight`, `FeedbackBody` (`src/utils/feedback.tsx`) + `styles/feedback.styles.ts` | | Siblings that slide when an item is added or removed | `useLayoutAnimation` (`src/primitives/hooks/`) | | An interaction with an async lifecycle or two values that move together | A machine in the controller file, via `setup()` (`src/machine/`) | @@ -41,6 +59,14 @@ Stop and check the table if you are writing any of these in a feature: - `setTimeout` to debounce or to delay a spinner - `stylex.create` inside a `.tsx` file +## Readability before complexity scores + +Do not extract a helper or introduce a machine only to reduce a lint score. +Avoid turning a component into a hook solely to work around React's children +typing. An abstraction must clarify a responsibility or hide useful complexity. When complexity is inherent, keep the relevant logic +together and explain the tradeoff in review. Follow repository rules for any lint +exception; this guidance does not authorize a suppression. + ## When nothing fits: ask first If no row fits, or a shared helper almost fits but would need a change, **stop and ask the user before writing code**. Say: diff --git a/packages/mosaic/ARCHITECTURE.md b/packages/mosaic/ARCHITECTURE.md index 376847cdf0e..c13463c4e1c 100644 --- a/packages/mosaic/ARCHITECTURE.md +++ b/packages/mosaic/ARCHITECTURE.md @@ -88,19 +88,27 @@ import { MosaicProvider } from './mosaic-provider'; Styles are declared once per component with `stylex.create`, keyed off the same axes the component exposes as props, then fused with `themeProps` output and the props the component was called with: -```tsx -const styles = stylex.create({ +Declare the styles in `src/components/button/button.styles.ts`: + +```ts +export const styles = stylex.create({ base: { display: 'inline-flex', borderRadius: radiusVars['--cl-radius-md'] }, }); // `color` and `variant` are independent props, but every pair resolves to one style, so they are // keyed compositely rather than merged at render time. -const variants = stylex.create({ +export const variants = stylex.create({ 'filled-primary': { backgroundColor: colorVars['--cl-color-brand'], borderColor: 'transparent' }, 'filled-negative': { backgroundColor: colorVars['--cl-color-negative'], borderColor: 'transparent' }, 'outline-primary': { backgroundColor: 'transparent', borderColor: colorVars['--cl-color-brand'] }, 'outline-negative': { backgroundColor: 'transparent', borderColor: colorVars['--cl-color-negative'] }, }); +``` + +Import them in `src/components/button/button.tsx`: + +```tsx +import { styles, variants } from './button.styles'; export const Button = React.forwardRef(function Button( { color = 'primary', variant = 'filled', size = 'md', disabled, xstyle, ...rest }, @@ -168,7 +176,7 @@ once the interaction has an async lifecycle or two values that must change together. A controller can also do both — a machine for the coordinated subset, `useState` for the UI-only flags beside it. The choice is invisible from the outside: the controller returns plain props either way, so the view cannot tell -and neither can its tests. `machine/ADOPTION.md` holds the criteria. +and neither can its tests. `src/machine/ADOPTION.md` holds the criteria. ### File shape @@ -450,7 +458,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 -produce the flag soup the machine exists to prevent. `machine/ADOPTION.md` has +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. ### Views diff --git a/packages/mosaic/docs/controllers.md b/packages/mosaic/docs/controllers.md index 079a8007430..f5bd211ebaa 100644 --- a/packages/mosaic/docs/controllers.md +++ b/packages/mosaic/docs/controllers.md @@ -177,6 +177,31 @@ surface shows no error still logs it, so a failure is never silent `useSpinDelay`, a `mode` forced by a capability flag — anything the view would otherwise have to re-derive. +## Composition and dialog lifetime + +When a wrapper composes the controller above the view, prefer one +controller-derived contract, forwarding the model's display data through the +controller when needed. Where practical, mount a +separate component only once the model is ready, so its controller receives the +narrowed ready model instead of repeating loading branches. Keep the supported +injected-effect leaf composition described in `views.md`. + +Decide which dialog state survives closing and reopening. Verify that the next +interaction does not retain an unintended error or draft, and keep reset behavior +consistent with the pending action's lifetime. Handle rejected dynamic imports +through an appropriate error owner rather than leaving unhandled rejections. + +For a dialog that clears its draft and error on close, test a failed submit, +close it, and reopen it. Assert that the error is gone and the field has its +initial value. If the draft is meant to survive, assert the saved draft instead. +Hold the submit request while dismissing to verify that closing does not unlock +a second mutation or abandon the first. When a feature uses a dynamic import, +reject the loader and assert the error UI and recovery behavior it promises. + +When introducing lazy loading or Suspense, consider a boundary around dialog +content that leaves the trigger and parent available. Choose that boundary from +the feature's loading behavior; it does not require restructuring every dialog. + ## Rules - **No Clerk imports.** If a controller needs a Clerk fact, the model supplies it @@ -226,11 +251,16 @@ One path, so every error a user sees goes through the `errors.*` catalog: password section maps its own errors (`user-profile-password-errors.ts`). - **`errorFallback` is the feature's copy for faults Clerk cannot describe** (code bugs, non-Clerk errors). An `UnlocalizableError` renders the fallback, - never its cause's `.message`. A network failure has its own catalog entry, - `network_error`. + never its cause's `.message`. A failure carrying the Clerk code `network_error` + has catalog copy, but network failures may also arrive as raw errors and remain + unlocalizable. Preserve the cause and check the unknown-error fallback too. - In tests, reject with `clerkApiError(code, message, { paramName })` from `src/__tests__/clerk-errors.ts` to assert catalog copy. Reject with a plain `Error` (silence `console.error`) to assert the fallback. + For example, reject with `new Error('socket disconnected')`, assert the + feature's fallback is visible, and assert that `socket disconnected` is not + rendered. Test `clerkApiError('network_error', 'server detail')` separately + to verify catalog copy rather than assuming every network fault has a code. ## Testing diff --git a/packages/mosaic/docs/headless.md b/packages/mosaic/docs/headless.md index a27f569d9a0..28d2e893008 100644 --- a/packages/mosaic/docs/headless.md +++ b/packages/mosaic/docs/headless.md @@ -46,7 +46,9 @@ params are documented on `UseRenderParams*` in `utils/use-render.tsx`. - **`mergeProps(internal, consumer)` — internal first, consumer second.** Event handlers chain (internal fires, then consumer), `style` shallow-merges, `className` concatenates, everything else the consumer overwrites. This lets - consumers extend behavior without breaking ARIA/handlers the primitive owns. + consumers extend event handlers while retaining the internal handlers. ARIA + attributes can be overwritten. A render function must apply the computed props + and ref to preserve the primitive's behavior. - **Positioners pass `enabled: mounted`**, so the floating DOM doesn't exist until the first frame (`select/select-positioner.tsx`). diff --git a/packages/mosaic/docs/migration.md b/packages/mosaic/docs/migration.md index 8076bc45634..ac084d19997 100644 --- a/packages/mosaic/docs/migration.md +++ b/packages/mosaic/docs/migration.md @@ -34,13 +34,14 @@ hit is a row you must consciously place or drop later: | `revalidate` / `.reload()` | cache invalidation timing after a mutation | | `.model.ts(x)` · `.controller.ts(x)` · diff --git a/packages/mosaic/docs/models.md b/packages/mosaic/docs/models.md index f05bd69b0ca..edc6d280def 100644 --- a/packages/mosaic/docs/models.md +++ b/packages/mosaic/docs/models.md @@ -96,6 +96,25 @@ drops the fallback instead of holding the space open. Keep the two apart. - No React state machinery beyond the Clerk hooks themselves; the model is a derivation of what Clerk currently says. +## Time-dependent display data + +Use the shared `useNow` hook and `MosaicProvider` clock support introduced in +[#10098](https://github.com/clerk/javascript/pull/10098). These APIs must be +available in the consuming branch before following this guidance. + +`MosaicProvider` supplies a shared initial `Date`. Read it with `useNow` from +`src/hooks/use-now.ts` in the consuming component or controller, then pass the +value to pure formatting helpers. For relative labels that refresh each minute, +use `useNow({ updateInterval: 60_000 })`. Without an interval, the hook retains +its initial value. Reuse this API instead of adding a feature-specific clock +provider or timer. + +In tests, supply a fixed `Date` through `MosaicNowProvider`, assert the displayed +label, then advance fake timers across a label boundary and assert the update. +If a feature supports SSR, also verify matching initial times during server +rendering and hydration. Sharing time within one provider tree does not by +itself guarantee agreement between server and client. + ## Testing The feature test covers the model by default, running it against a real Clerk diff --git a/packages/mosaic/docs/testing.md b/packages/mosaic/docs/testing.md index b66061ec2c8..7b557aa8b6c 100644 --- a/packages/mosaic/docs/testing.md +++ b/packages/mosaic/docs/testing.md @@ -94,6 +94,10 @@ it('makes the selected organization active and closes', async () => { await user.click(screen.getByRole('button', { name: /Open account menu/ })); await user.click(await screen.findByRole('button', { name: 'Other' })); await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); + + await user.click(screen.getByRole('button', { name: /Open account menu/ })); + expect(await screen.findByRole('button', { name: 'Acme' })).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'Other' })).toBeNull(); }); ```