Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<svg …> intercepts pointer events` |
| `test:compile` fails in unrelated shared code with **`Property 'children' does not exist on type 'Partial<XProps>'`** 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 <propA>, <propB>` to the component's JSDoc and regenerate — the prop then arrives as a slotted child and any React subtree works, including a raw `<svg>` 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 <file>` 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 <file> --update`) or pin the flag's value (`--update=true <file>`). 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) |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export const CodeEditor = flowComponent("CodeEditor", (props) => {
"aria-label": ariaLabel,
"aria-labelledby": ariaLabelledBy,
...rest
} = useControlledHostValueProps(props);
} = useControlledHostValueProps(props, "");

const {
FieldErrorView,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,15 @@ export interface DateRangePickerProps<T extends Aria.DateValue = Aria.DateValue>

/** @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,
onChange,
ref,
withDatePickerPresets = false,
...rest
} = useControlledHostValueProps(props);
} = useControlledHostValueProps(props, null);

const popoverController = useOverlayController("Popover");
const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<MarkdownEditorMode>("editor");
Expand Down
54 changes: 54 additions & 0 deletions packages/components/src/components/Modal/Modal.browser.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<ModalTrigger>
<Button>Open Modal</Button>
<Modal>
<Content>
<Text data-testid="modal-text">Hello World</Text>
<Form form={form} onSubmit={vitest.fn()}>
<Field name="testField">
<TextField aria-label="Test field" />
</Field>
</Form>
</Content>
<ActionGroup>
<Action closeModal>
<Button>Close modal</Button>
</Action>
</ActionGroup>
</Modal>
</ModalTrigger>
);
};

try {
const dom = await render(<Test />);

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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export const PasswordCreationField = flowComponent(
value,
onChange,
...rest
} = useControlledHostValueProps(props);
} = useControlledHostValueProps(props, "");

const {
FieldErrorView,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/components/TextArea/TextArea.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(<TextField aria-label="test" />);
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();
}
});
2 changes: 1 addition & 1 deletion packages/components/src/components/TextField/TextField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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<ReturnType<typeof render>>;

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<void>;
}

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) => (
<TextField aria-label="Field" onChange={onChange} />
),
controlled: (onChange) => (
<TextField aria-label="Field" value="held" onChange={onChange} />
),
change: typeIntoTextbox,
},
{
toString: () => "TextArea",
uncontrolled: (onChange) => (
<TextArea aria-label="Field" onChange={onChange} />
),
controlled: (onChange) => (
<TextArea aria-label="Field" value="held" onChange={onChange} />
),
change: typeIntoTextbox,
},
{
toString: () => "SearchField",
uncontrolled: (onChange) => (
<SearchField aria-label="Field" onChange={onChange} />
),
controlled: (onChange) => (
<SearchField aria-label="Field" value="held" onChange={onChange} />
),
change: async (dom) => {
await userEvent.type(dom.getByRole("searchbox"), "x");
},
},
{
toString: () => "MarkdownEditor",
uncontrolled: (onChange) => (
<MarkdownEditor aria-label="Field" onChange={onChange} />
),
controlled: (onChange) => (
<MarkdownEditor aria-label="Field" value="held" onChange={onChange} />
),
change: typeIntoTextbox,
},
{
toString: () => "PasswordCreationField",
uncontrolled: (onChange) => (
<PasswordCreationField aria-label="Field" onChange={onChange} />
),
controlled: (onChange) => (
<PasswordCreationField
aria-label="Field"
value="Held-Passphrase-1"
onChange={onChange}
/>
),
change: typeIntoInput,
},
{
toString: () => "CodeEditor",
uncontrolled: (onChange) => (
<CodeEditor aria-label="Field" onChange={onChange} />
),
controlled: (onChange) => (
<CodeEditor aria-label="Field" value="held" onChange={onChange} />
),
change: async (dom) => {
await userEvent.type(dom.getByLocator(".cm-content"), "x");
},
},
{
toString: () => "NumberField",
uncontrolled: (onChange) => (
<NumberField aria-label="Field" onChange={onChange} />
),
controlled: (onChange) => (
<NumberField aria-label="Field" value={3} onChange={onChange} />
),
change: typeIntoInput,
},
{
toString: () => "DateRangePicker",
uncontrolled: (onChange) => (
<DateRangePicker aria-label="Field" onChange={onChange} />
),
controlled: (onChange) => (
<DateRangePicker
aria-label="Field"
value={{
start: new CalendarDate(2025, 9, 1),
end: new CalendarDate(2025, 9, 5),
}}
onChange={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<void>,
) => {
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);
},
);
Loading
Loading