diff --git a/AGENTS.md b/AGENTS.md index 5619c94860..a3ae4adf54 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -311,6 +311,7 @@ where the error points. | A CI workflow that runs `pnpm install` and then pushes, merges or checks out spends minutes in `eslint`/`stylelint`/`prettier`, or reinstalls in the middle of a merge | simple-git-hooks is allowlisted in `allowBuilds`, so its `postinstall` writes `.git/hooks` on **every** `pnpm install` — runners included. `pre-push` is `pnpm lint`, `post-merge`/`post-checkout` are `pnpm install` | Add `SKIP_INSTALL_SIMPLE_GIT_HOOKS: "1"` to the workflow's `env` (see `publish.yml`). Where a failed push would strand something already published, also pass `git push --no-verify` — that guard sits at the push and does not depend on the env var staying put (#2932) | | A visual test that hovers before `testScreenshot` captures the **non-hovered** state — the diff looks as if the CSS never applied | `testScreenshot` calls `setNeutralPointerPosition()` before every screenshot, so a CSS `:hover` state is gone by capture time. An overlay opened by hover survives (`Tooltip`) — its visibility is state, not a CSS `:hover` rule; a pure `:hover` style never does | Assert the computed style in a `*.browser.test.tsx` instead of screenshotting it. And hover the **label**, not the `input` inside it — stacked icons cover the input and Playwright refuses with ` intercepts pointer events` | | `test:compile` fails in unrelated shared code with **`Property 'children' does not exist on type 'Partial'`** right after you registered a new component | `dynamic()` and other shared helpers are typed over the **union of every** registered props type, so code reading `p.children` breaks as soon as one registered component has no `children` (`Field.tsx`, `flowComponent.browser.test.tsx`) | Declare `children?: never` on the new props type. It keeps the union accessible and documents that the component takes no children — do **not** add a real `children` slot you then ignore | +| `test:compile` passes locally, but CI's `unit` job fails with a TS error in a **different package** than the one you ran it for — e.g. **`TS2339: Property 'value' does not exist on type 'HTMLElement \| SVGElement'`** on `Locator.element()` | A per-package nx target typechecks that package only, so a diff that adds or edits files in a second package leaves that package's `test:compile` unrun. Its **browser tests passing proves nothing about types** — vitest transpiles without typechecking, so the one gate that reads types there is exactly the one a per-package run skips | Run it for **every** package the diff touches (`git status --short` names them), or use `pnpm nx run-many -t test:compile` / `pnpm test`, which is what CI does | | A `ReactElement`-valued prop renders fine in the visual suite's **Remote** environment but kills a real remote connection with **`DataCloneError: Symbol(react.transitional.element) could not be cloned`** (the host then shows `Remote rendering failed: Timeout reached`) | The prop was generated as a **remote property**, and properties travel through `postMessage`/structured clone, which cannot carry a React element. `remote-dom-react` turns an element-valued prop into a slotted child _only_ when the element declares it as a slot (`Element.remoteSlotDefinitions.has(prop)`), and the generator declares a slot only for props typed exactly `ReactNode` or listed in `@flr-slot-props` (`dev/remote-components-generator/lib/propClassifiers.ts`) | Add `@flr-slot-props , ` to the component's JSDoc and regenerate — the prop then arrives as a slotted child and any React subtree works, including a raw `` or a Tabler icon. Verify in `apps/remote-dom-demo` over the iframe connection: the visual suite's Remote environment is **in-process** and never serializes, so it cannot see this class of bug | | `vitest run --update ` rewrites **every** baseline instead of the one file's, and `git status` shows snapshots you never touched | `--update` takes an optional value, so it swallows the positional test filter that follows it. The run then matches all files, updates all of them, and reports the full test count (354, not 2) — the only signal that anything went wrong | Put the filter **before** the flag (`vitest run --update`) or pin the flag's value (`--update=true `). Check `git status` after any `--update` and revert baselines outside your change; a stray one is indistinguishable from an intentional update once committed | | A baseline for a component you never touched changes in your PR, or a scenario starts failing right after an unrelated PR merged with `update-screenshots` | The `update-screenshots` label runs `pnpm test:visual --update` over the **whole** suite and then `git add -A` — it is not scoped to the PR's diff. Any scenario that is failing or flaky at that moment gets whatever renders then committed as its new truth | Only apply the label when the suite is otherwise green, and read the resulting commit's file list before merging. #2945 (a CodeBlock change) took a tooltip-less frame of a racy `Tooltip` scenario this way and committed it as the `firefox-linux` baseline, contradicting the three beside it and keeping the scheduled run red in both environments (#2985) | diff --git a/packages/components/src/components/CodeEditor/CodeEditor.tsx b/packages/components/src/components/CodeEditor/CodeEditor.tsx index 539b2c78d0..bd54a41b9b 100644 --- a/packages/components/src/components/CodeEditor/CodeEditor.tsx +++ b/packages/components/src/components/CodeEditor/CodeEditor.tsx @@ -91,7 +91,7 @@ export const CodeEditor = flowComponent("CodeEditor", (props) => { "aria-label": ariaLabel, "aria-labelledby": ariaLabelledBy, ...rest - } = useControlledHostValueProps(props); + } = useControlledHostValueProps(props, ""); const { FieldErrorView, diff --git a/packages/components/src/components/DateRangePicker/DateRangePicker.tsx b/packages/components/src/components/DateRangePicker/DateRangePicker.tsx index eea42044fe..8f8222b788 100644 --- a/packages/components/src/components/DateRangePicker/DateRangePicker.tsx +++ b/packages/components/src/components/DateRangePicker/DateRangePicker.tsx @@ -31,6 +31,7 @@ export interface DateRangePickerProps /** @flr-generate all */ export const DateRangePicker = flowComponent("DateRangePicker", (props) => { + // `null` is what react-aria's date range picker state uses for "no range" const { children, className, @@ -38,7 +39,7 @@ export const DateRangePicker = flowComponent("DateRangePicker", (props) => { ref, withDatePickerPresets = false, ...rest - } = useControlledHostValueProps(props); + } = useControlledHostValueProps(props, null); const popoverController = useOverlayController("Popover"); const { diff --git a/packages/components/src/components/MarkdownEditor/MarkdownEditor.tsx b/packages/components/src/components/MarkdownEditor/MarkdownEditor.tsx index 35b4b97c8b..7920c1a544 100644 --- a/packages/components/src/components/MarkdownEditor/MarkdownEditor.tsx +++ b/packages/components/src/components/MarkdownEditor/MarkdownEditor.tsx @@ -51,7 +51,7 @@ export const MarkdownEditor = flowComponent("MarkdownEditor", (props) => { onChange, ref, ...rest - } = useControlledHostValueProps(props); + } = useControlledHostValueProps(props, ""); const inputRef = useObjectRef(ref); const [mode, setMode] = useState("editor"); diff --git a/packages/components/src/components/Modal/Modal.browser.test.tsx b/packages/components/src/components/Modal/Modal.browser.test.tsx index 28d2f3d3b6..f4d44d92c3 100644 --- a/packages/components/src/components/Modal/Modal.browser.test.tsx +++ b/packages/components/src/components/Modal/Modal.browser.test.tsx @@ -72,6 +72,60 @@ test("Modal can be controlled with modal controller", async () => { expect(modalText).toBeInTheDocument(); }); +/* + * Nothing inside a mounted modal may cross React's controlled/uncontrolled + * line. #3026 read the nine warnings this file used to emit as a flip of the + * modal's own open state; they came from the `TextField` in the tests below, + * whose value only became controlled once the first keystroke landed. + */ +test("Modal and its fields stay on one side of the controlled line", async () => { + const warn = vitest.spyOn(console, "warn"); + + const Test = () => { + const form = useForm(); + + return ( + + + + + Hello World +
+ + + +
+ + + + + + + + + ); + }; + + try { + const dom = await render(); + + await userEvent.click( + dom.getByRole("button", { name: "Open Modal", exact: true }), + ); + await userEvent.type(dom.getByRole("textbox"), "Some changes"); + await userEvent.click( + dom.getByRole("button", { name: "Close modal", exact: true }), + ); + expect(dom.getByTestId("modal-text")).not.toBeInTheDocument(); + + expect(warn.mock.calls.flat().join("\n")).not.toContain( + "uncontrolled to controlled", + ); + } finally { + warn.mockRestore(); + } +}); + test("Modal with dirty form requires confirmation", async () => { const Test = () => { const form = useForm(); diff --git a/packages/components/src/components/NumberField/NumberField.tsx b/packages/components/src/components/NumberField/NumberField.tsx index 491f09e80f..bd1d0f5327 100644 --- a/packages/components/src/components/NumberField/NumberField.tsx +++ b/packages/components/src/components/NumberField/NumberField.tsx @@ -23,13 +23,14 @@ export interface NumberFieldProps /** @flr-generate all */ export const NumberField = flowComponent("NumberField", (props) => { + // `NaN` is what react-aria's number field state uses for "no number" const { children, className, isWheelDisabled = true, ref, ...rest - } = useControlledHostValueProps(props); + } = useControlledHostValueProps(props, NaN); const { FieldErrorView, diff --git a/packages/components/src/components/PasswordCreationField/PasswordCreationField.tsx b/packages/components/src/components/PasswordCreationField/PasswordCreationField.tsx index 5189366706..bb3ce7adba 100644 --- a/packages/components/src/components/PasswordCreationField/PasswordCreationField.tsx +++ b/packages/components/src/components/PasswordCreationField/PasswordCreationField.tsx @@ -91,7 +91,7 @@ export const PasswordCreationField = flowComponent( value, onChange, ...rest - } = useControlledHostValueProps(props); + } = useControlledHostValueProps(props, ""); const { FieldErrorView, diff --git a/packages/components/src/components/SearchField/SearchField.tsx b/packages/components/src/components/SearchField/SearchField.tsx index ad4513cacd..dd7373e4f4 100644 --- a/packages/components/src/components/SearchField/SearchField.tsx +++ b/packages/components/src/components/SearchField/SearchField.tsx @@ -21,8 +21,10 @@ export interface SearchFieldProps /** @flr-generate all */ export const SearchField = flowComponent("SearchField", (props) => { - const { children, className, ref, ...rest } = - useControlledHostValueProps(props); + const { children, className, ref, ...rest } = useControlledHostValueProps( + props, + "", + ); const { FieldErrorView, diff --git a/packages/components/src/components/TextArea/TextArea.tsx b/packages/components/src/components/TextArea/TextArea.tsx index 00261ad725..d9c56fde21 100644 --- a/packages/components/src/components/TextArea/TextArea.tsx +++ b/packages/components/src/components/TextArea/TextArea.tsx @@ -49,7 +49,7 @@ export const TextArea = flowComponent("TextArea", (props) => { onChange, isReadOnly, ...rest - } = useControlledHostValueProps(props); + } = useControlledHostValueProps(props, ""); const [charactersCount, setCharactersCount] = useState( props.defaultValue?.length ?? props.value?.length ?? 0, diff --git a/packages/components/src/components/TextField/TextField.browser.test.tsx b/packages/components/src/components/TextField/TextField.browser.test.tsx index ab16158f9d..a1871ccbb1 100644 --- a/packages/components/src/components/TextField/TextField.browser.test.tsx +++ b/packages/components/src/components/TextField/TextField.browser.test.tsx @@ -10,3 +10,26 @@ test("TextField has typed value on blur", async () => { await userEvent.tab(); expect(input).toHaveDisplayValue("test"); }); + +/* + * `useControlledHostValueProps` owns the value from the first change on, so a + * field that renders without `value` and without `defaultValue` must still + * start out controlled – otherwise the value changes owner mid-flight, from the + * DOM input to the hook, and react-aria warns about the transition. + */ +test("TextField stays controlled across the first change", async () => { + const warn = vitest.spyOn(console, "warn"); + + try { + const dom = await render(); + const input = dom.getByRole("textbox"); + await userEvent.type(input, "test"); + await expect.element(input).toHaveDisplayValue("test"); + + expect(warn.mock.calls.flat().join("\n")).not.toContain( + "uncontrolled to controlled", + ); + } finally { + warn.mockRestore(); + } +}); diff --git a/packages/components/src/components/TextField/TextField.tsx b/packages/components/src/components/TextField/TextField.tsx index 0fbbda4330..144477b726 100644 --- a/packages/components/src/components/TextField/TextField.tsx +++ b/packages/components/src/components/TextField/TextField.tsx @@ -37,7 +37,7 @@ export const TextField = flowComponent("TextField", (props) => { children, onChange, ...rest - } = useControlledHostValueProps(props); + } = useControlledHostValueProps(props, ""); const [charactersCount, setCharactersCount] = useState( props.defaultValue?.length ?? props.value?.length ?? 0, diff --git a/packages/components/src/lib/remote/useControlledHostValueProps.browser.test.tsx b/packages/components/src/lib/remote/useControlledHostValueProps.browser.test.tsx new file mode 100644 index 0000000000..5035c28d53 --- /dev/null +++ b/packages/components/src/lib/remote/useControlledHostValueProps.browser.test.tsx @@ -0,0 +1,199 @@ +import CodeEditor from "@/components/CodeEditor"; +import DateRangePicker from "@/components/DateRangePicker"; +import MarkdownEditor from "@/components/MarkdownEditor"; +import NumberField from "@/components/NumberField"; +import PasswordCreationField from "@/components/PasswordCreationField"; +import SearchField from "@/components/SearchField"; +import TextArea from "@/components/TextArea"; +import TextField from "@/components/TextField"; +import { CalendarDate } from "@internationalized/date"; +import type { ReactNode } from "react"; +import { expect, test, vitest } from "vitest"; +import { render } from "vitest-browser-react"; +import { userEvent } from "vitest/browser"; + +/* + * `useControlledHostValueProps` mirrors the value of every field that has to + * survive interleaved host and remote updates, and that mirror is what the + * field renders. So the mirror decides whether react-stately sees a controlled + * or an uncontrolled field, and it has to stay on one side of that line for the + * field's whole life: `useControlledState` reads only `undefined` as + * uncontrolled, so a mirror that starts out `undefined` and becomes defined + * with the first change flips the field from uncontrolled to controlled (#3027). + */ + +type Rendered = Awaited>; + +interface FieldUnderTest { + toString: () => string; + /** Rendered with neither `value` nor `defaultValue`. */ + uncontrolled: (onChange: () => void) => ReactNode; + /** Rendered with a `value` the test never replaces. */ + controlled: (onChange: () => void) => ReactNode; + /** One interaction that makes the field report a change. */ + change: (dom: Rendered) => Promise; +} + +const typeIntoTextbox = async (dom: Rendered) => { + await userEvent.type(dom.getByRole("textbox"), "x"); +}; + +const typeIntoInput = async (dom: Rendered) => { + await userEvent.type(dom.getByLocator("input"), "5"); + await userEvent.tab(); +}; + +const fields: FieldUnderTest[] = [ + { + toString: () => "TextField", + uncontrolled: (onChange) => ( + + ), + controlled: (onChange) => ( + + ), + change: typeIntoTextbox, + }, + { + toString: () => "TextArea", + uncontrolled: (onChange) => ( + + ), + controlled: (onChange) => ( + + ), + change: typeIntoTextbox, + }, + { + toString: () => "SearchField", + uncontrolled: (onChange) => ( + + ), + controlled: (onChange) => ( + + ), + change: async (dom) => { + await userEvent.type(dom.getByRole("searchbox"), "x"); + }, + }, + { + toString: () => "MarkdownEditor", + uncontrolled: (onChange) => ( + + ), + controlled: (onChange) => ( + + ), + change: typeIntoTextbox, + }, + { + toString: () => "PasswordCreationField", + uncontrolled: (onChange) => ( + + ), + controlled: (onChange) => ( + + ), + change: typeIntoInput, + }, + { + toString: () => "CodeEditor", + uncontrolled: (onChange) => ( + + ), + controlled: (onChange) => ( + + ), + change: async (dom) => { + await userEvent.type(dom.getByLocator(".cm-content"), "x"); + }, + }, + { + toString: () => "NumberField", + uncontrolled: (onChange) => ( + + ), + controlled: (onChange) => ( + + ), + change: typeIntoInput, + }, + { + toString: () => "DateRangePicker", + uncontrolled: (onChange) => ( + + ), + controlled: (onChange) => ( + + ), + change: async (dom) => { + await dom.getByLocator("button").click(); + await userEvent.keyboard("{Enter}{Enter}"); + }, + }, +]; + +/** + * The transition is warned about twice: by `react-stately` for the state, and + * by React DOM for the input element behind it. Both messages name the + * direction, so one substring each covers a switch either way. + */ +const controlledSwitchWarnings = [ + "uncontrolled to controlled", + "controlled to uncontrolled", + "uncontrolled input to be controlled", + "controlled input to be uncontrolled", +]; + +const expectNoControlledSwitch = async ( + build: (onChange: () => void) => ReactNode, + change: (dom: Rendered) => Promise, +) => { + const warn = vitest.spyOn(console, "warn"); + const error = vitest.spyOn(console, "error"); + const onChange = vitest.fn(); + + try { + const dom = await render(build(onChange)); + await change(dom); + + /* Without a reported change nothing below could ever fail. */ + expect(onChange).toHaveBeenCalled(); + + const messages = [...warn.mock.calls, ...error.mock.calls] + .flat() + .join("\n"); + + for (const warning of controlledSwitchWarnings) { + expect(messages).not.toContain(warning); + } + } finally { + warn.mockRestore(); + error.mockRestore(); + } +}; + +test.each(fields)( + "%s does not switch between controlled and uncontrolled when nothing controls its value", + async (field) => { + await expectNoControlledSwitch(field.uncontrolled, field.change); + }, +); + +test.each(fields)( + "%s does not switch between controlled and uncontrolled while its value is held", + async (field) => { + await expectNoControlledSwitch(field.controlled, field.change); + }, +); diff --git a/packages/components/src/lib/remote/useControlledHostValueProps.ts b/packages/components/src/lib/remote/useControlledHostValueProps.ts index 05e7bdeafa..da1b340d06 100644 --- a/packages/components/src/lib/remote/useControlledHostValueProps.ts +++ b/packages/components/src/lib/remote/useControlledHostValueProps.ts @@ -7,10 +7,18 @@ import type { FieldProps } from "@/lib/remote/types"; * by omitting values resulting from a remotely executed event handler. These * values are marked by the `controlledRemoteValueMarker`. * - * This hook is noly necessary for text inputs. If not use the controlled input + * This hook is only necessary for text inputs. If not used the controlled input * value may be corrupted by interleaved host inputs and remote events. + * + * @param emptyValue What the field reports while nothing has been entered — + * `""` for a text input, `NaN` for a number, `null` for a date range. It is + * the sentinel that keeps the field controlled from the first render; see + * that convention in the package's AGENTS.md. */ -export const useControlledHostValueProps = (props: FieldProps) => { +export const useControlledHostValueProps = ( + props: FieldProps, + emptyValue: T, +) => { const { value: valueFromProps, onChange: onChangeFromProps, @@ -20,7 +28,19 @@ export const useControlledHostValueProps = (props: FieldProps) => { const regularValue = valueFromProps === controlledRemoteValueMarker ? undefined : valueFromProps; - const [value, setValue] = useState(regularValue ?? defaultValue); + /* + * Only `undefined` falls back, because only `undefined` reads as + * uncontrolled: a caller-supplied `null` controls a `DateRangePicker` with no + * range selected, so `??` would swallow it. The marker cannot appear on the + * first render, so `regularValue` is the caller's own value here. + */ + const [value, setValue] = useState( + regularValue !== undefined + ? regularValue + : defaultValue !== undefined + ? defaultValue + : emptyValue, + ); useLayoutEffect(() => { if (regularValue !== undefined) { diff --git a/packages/remote-react-components/src/tests/RemoteControlledValue.browser.test.tsx b/packages/remote-react-components/src/tests/RemoteControlledValue.browser.test.tsx new file mode 100644 index 0000000000..6c1e102f7b --- /dev/null +++ b/packages/remote-react-components/src/tests/RemoteControlledValue.browser.test.tsx @@ -0,0 +1,169 @@ +import { testEnvironments } from "@/tests/lib/environments"; +import type { ScenarioComponents } from "@/tests/lib/visualScenario"; +import { useState } from "react"; +import { expect, test, vitest } from "vitest"; +import { page, userEvent } from "vitest/browser"; + +/* + * A field the remote app controls reports every keystroke to the remote side and + * gets the value back a round trip later. The host must not apply that echo — it + * is already showing the character, and an echo of an earlier keystroke would + * drop everything typed since. `useControlledRemoteValueProps` marks the echo + * (`controlledRemoteValueMarker`) and `useControlledHostValueProps` renders its + * own mirror of the value instead. + * + * A value the remote app sets itself carries no marker and has to arrive, or the + * app could no longer drive its own field. + * + * That mirror is also what decides whether the field is controlled at all + * (#3027), so a change to one of the two is a change to the other — which is + * why both are asserted here. The `Local` environment runs the same trees + * without a connection, so nothing is ever echoed there. + */ + +const typedText = "correcthorsebattery"; +const valueFromTheRemoteSide = "set by the remote app"; + +interface Props { + components: ScenarioComponents; + onChange?: (value: string) => void; +} + +/** Value owned by the remote app — the echo path. */ +const ControlledField = (props: Props) => { + const { Button, Label, TextField } = props.components; + const [value, setValue] = useState(""); + + return ( + <> + { + setValue(newValue); + props.onChange?.(newValue); + }} + > + + + + + ); +}; + +/** Value owned by the field itself, the remote app only listens. */ +const UncontrolledField = (props: Props) => { + const { Label, TextField } = props.components; + + return ( + + + + ); +}; + +const inputLocator = page.getByLocator("input"); + +const controlledSwitchWarnings = [ + "uncontrolled to controlled", + "controlled to uncontrolled", + "uncontrolled input to be controlled", + "controlled input to be uncontrolled", +]; + +const expectNoControlledSwitch = async ( + type: () => Promise, +): Promise => { + const warn = vitest.spyOn(console, "warn"); + const error = vitest.spyOn(console, "error"); + + try { + await type(); + + const messages = [...warn.mock.calls, ...error.mock.calls] + .flat() + .join("\n"); + + for (const warning of controlledSwitchWarnings) { + expect(messages).not.toContain(warning); + } + } finally { + warn.mockRestore(); + error.mockRestore(); + } +}; + +test.each(testEnvironments)( + "a field the remote app controls keeps everything typed into it (%s)", + async ({ render, components }) => { + const onChange = vitest.fn(); + + await render( + , + ); + await expect.element(inputLocator).toBeVisible(); + + await userEvent.type(inputLocator, typedText); + + await expect.element(inputLocator).toHaveDisplayValue(typedText); + + /* + * Every reported change carries the whole text, so an echo the host applied + * would show up as a value that is not a prefix of what was typed. The + * remote side does not necessarily see every keystroke — a host event that + * fires while a remote render is in flight is dropped, which is + * load-dependent and has nothing to do with the echo. + */ + for (const [reportedValue] of onChange.mock.calls) { + expect(typedText.startsWith(reportedValue)).toBe(true); + } + }, +); + +test.each(testEnvironments)( + "a value the remote app sets reaches the field (%s)", + async ({ render, components }) => { + await render(); + await expect.element(inputLocator).toBeVisible(); + + await userEvent.type(inputLocator, typedText); + await expect.element(inputLocator).toHaveDisplayValue(typedText); + + await page.getByRole("button", { name: "Overwrite" }).click(); + + await expect + .element(inputLocator) + .toHaveDisplayValue(valueFromTheRemoteSide); + }, +); + +test.each(testEnvironments)( + "a field the remote app controls does not switch between controlled and uncontrolled (%s)", + async ({ render, components }) => { + await render(); + await expect.element(inputLocator).toBeVisible(); + + await expectNoControlledSwitch(async () => { + await userEvent.type(inputLocator, typedText); + await expect.element(inputLocator).toHaveDisplayValue(typedText); + }); + }, +); + +test.each(testEnvironments)( + "a field the remote app does not control does not switch between controlled and uncontrolled (%s)", + async ({ render, components }) => { + const onChange = vitest.fn(); + + await render( + , + ); + await expect.element(inputLocator).toBeVisible(); + + await expectNoControlledSwitch(async () => { + await userEvent.type(inputLocator, typedText); + await expect.poll(() => onChange).toHaveBeenCalled(); + }); + }, +);