feat(ui): add field composition primitives - #629
Conversation
|
@coderabbitai review |
WalkthroughAdds shared ChangesField component extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant FieldRoot
participant FieldControl
participant DescriptionError
App->>FieldRoot: render field with id and metadata
FieldRoot->>FieldControl: provide shared field context
FieldControl->>DescriptionError: reference description and error ids
DescriptionError-->>FieldControl: render accessible metadata
FieldControl-->>App: render control with aria attributes
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/ui/src/components/field.tsx (3)
65-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelf-closing JSX with children passed via prop spread works but is unusual.
<div {...props} />and<fieldset {...props} />render children becausechildrenremains in...props. This is valid React 19 behavior and confirmed by tests, but the self-closing syntax may confuse readers expecting explicit children. Consider adding a brief comment or rendering children explicitly for clarity.Also applies to: 165-196
🤖 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/components/field.tsx` around lines 65 - 86, Clarify the implicit children rendering in FieldRoot and the analogous fieldset component by either rendering props.children explicitly inside the div/fieldset or adding a brief comment explaining that children are preserved through the prop spread; keep the existing FieldContext and prop behavior unchanged.
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
interfaceforMetadataper coding guidelines.The guidelines for
**/*.{ts,tsx}state to "prefer interfaces over types."Metadatais a simple object shape with no union or intersection, so it can be aninterface. The other type aliases (FieldRootProps,FieldGroupRootProps,MetadataProps) use intersection/Omitand correctly remain astype.♻️ Proposed refactor
-type Metadata = { +interface Metadata { id: string description: ReactNode | undefined error: ReactNode | undefined descriptionId: string errorId: string -} +}🤖 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/components/field.tsx` around lines 14 - 20, Change the simple object-shaped Metadata declaration to an interface, keeping its existing properties and types unchanged; leave FieldRootProps, FieldGroupRootProps, and MetadataProps as type aliases because they use intersections or Omit.Source: Coding guidelines
128-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting a shared Description/Error renderer to reduce duplication.
FieldDescription/FieldGroupDescriptionandFieldError/FieldGroupErrorare near-identical pairs differing only in the context hook call. A shared internal component that accepts pre-resolved metadata would eliminate ~30 lines of duplication while preserving the same behavior.Also applies to: 206-235
🤖 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/components/field.tsx` around lines 128 - 157, Extract a shared internal metadata renderer for the duplicated FieldDescription/FieldGroupDescription and FieldError/FieldGroupError markup. Have each component resolve its context-specific values via useField or the group hook, then pass the description/error text, ID, className, and alert-role requirement into the shared renderer while preserving current styling, props, IDs, and null behavior.apps/wodsmith-start/src/components/ui/field.stories.tsx (2)
181-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a play function to verify error and
aria-invalidstate.
InvalidGrouprenders with an error but doesn't verifyaria-invalid="true"or that the alert is visible.CompositeGrouphas a play function, so adding one here would ensure the invalid group state is covered.✨ Suggested play function
export const InvalidGroup: Story = { render: () => ( <FieldGroup.Root id="heat-days" description="At least one day is required." error="Select Saturday or Sunday." className="max-w-sm" > <FieldGroup.Legend>Competition days</FieldGroup.Legend> <FieldGroup.Description /> <div className="space-y-2"> <label htmlFor="heat-saturday" className="flex items-center gap-2 text-sm" > <Checkbox id="heat-saturday" /> Saturday </label> <label htmlFor="heat-sunday" className="flex items-center gap-2 text-sm" > <Checkbox id="heat-sunday" /> Sunday </label> </div> <FieldGroup.Error /> </FieldGroup.Root> ), + play: async ({ canvasElement }) => { + const canvas = within(canvasElement) + const group = canvas.getByRole("group", { + name: "Competition days", + }) + await expect(group).toHaveAccessibleDescription( + "At least one day is required. Select Saturday or Sunday.", + ) + await expect(group).toHaveAttribute("aria-invalid", "true") + await expect(canvas.getByRole("alert")).toBeVisible() + }, }🤖 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 `@apps/wodsmith-start/src/components/ui/field.stories.tsx` around lines 181 - 208, Add a play function to the InvalidGroup story that queries the rendered group and verifies it has aria-invalid="true", then confirms the error alert is visible. Follow the existing CompositeGroup play-function pattern and use the story's testing utilities.
40-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a play function to verify
aria-describedbywiring.The
Defaultstory assertsaria-describedbyis absent, butWithDescriptiondoesn't verify it's present and points to the correct description element. Adding a play function here would close the coverage loop.✨ Suggested play function
export const WithDescription: Story = { render: () => ( <Field.Root id="team-name" description="This name appears on public leaderboards." className="max-w-sm" > <Field.Label>Team name</Field.Label> <Field.Control> <Input defaultValue="Downtown Strength" /> </Field.Control> <Field.Description /> <Field.Error /> </Field.Root> ), + play: async ({ canvasElement }) => { + const input = within(canvasElement).getByRole("textbox", { + name: "Team name", + }) + await expect(input).toHaveAccessibleDescription( + "This name appears on public leaderboards.", + ) + await expect(input).not.toHaveAttribute("aria-invalid") + }, }🤖 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 `@apps/wodsmith-start/src/components/ui/field.stories.tsx` around lines 40 - 55, Add a play function to the WithDescription story that queries the rendered input and description elements, then asserts the input’s aria-describedby references the description element’s ID. Use the existing Field.Root, Field.Control, Field.Description, and WithDescription symbols to keep the accessibility wiring coverage consistent with the Default story.
🤖 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 `@packages/ui/test/field.test.tsx`:
- Around line 18-63: Add exactly one `@lat`: comment immediately adjacent to the
test in the “Field composition” describe block, referencing
[[lat.md/ui-library.md#Field composition]]. Apply the same requirement to every
it(...) test in both describe blocks, using the appropriate spec section for
each test and never placing comments at file scope.
---
Nitpick comments:
In `@apps/wodsmith-start/src/components/ui/field.stories.tsx`:
- Around line 181-208: Add a play function to the InvalidGroup story that
queries the rendered group and verifies it has aria-invalid="true", then
confirms the error alert is visible. Follow the existing CompositeGroup
play-function pattern and use the story's testing utilities.
- Around line 40-55: Add a play function to the WithDescription story that
queries the rendered input and description elements, then asserts the input’s
aria-describedby references the description element’s ID. Use the existing
Field.Root, Field.Control, Field.Description, and WithDescription symbols to
keep the accessibility wiring coverage consistent with the Default story.
In `@packages/ui/src/components/field.tsx`:
- Around line 65-86: Clarify the implicit children rendering in FieldRoot and
the analogous fieldset component by either rendering props.children explicitly
inside the div/fieldset or adding a brief comment explaining that children are
preserved through the prop spread; keep the existing FieldContext and prop
behavior unchanged.
- Around line 14-20: Change the simple object-shaped Metadata declaration to an
interface, keeping its existing properties and types unchanged; leave
FieldRootProps, FieldGroupRootProps, and MetadataProps as type aliases because
they use intersections or Omit.
- Around line 128-157: Extract a shared internal metadata renderer for the
duplicated FieldDescription/FieldGroupDescription and FieldError/FieldGroupError
markup. Have each component resolve its context-specific values via useField or
the group hook, then pass the description/error text, ID, className, and
alert-role requirement into the shared renderer while preserving current
styling, props, IDs, and null behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2501433c-5a92-4d8a-8b2c-500c271192cb
📒 Files selected for processing (10)
apps/crew/src/components/ui/field.tsxapps/wodsmith-start/docs/ui-library-inventory.mdapps/wodsmith-start/scripts/generate-ui-library-inventory.mjsapps/wodsmith-start/src/components/ui/field.stories.tsxapps/wodsmith-start/src/components/ui/field.tsxlat.md/ui-library.mdpackages/ui/package.jsonpackages/ui/src/components/field.tsxpackages/ui/test/compatibility.test.tspackages/ui/test/field.test.tsx
|
Review pinned to exact head Findings:
Scope otherwise looks clean: the delta is the expected 10 files, contains identity-only Start/Crew adapters and no feature migrations, package exports/adapters are covered, native fieldset/legend usage is sound, and the Storybook state matrix is representative. GitNexus reports LOW blast radius with zero affected processes, which matches the no-consumer extraction. At this head all 14 GitHub checks are green, including app builds, lint, tests, typechecks, E2E, security, and CodeRabbit. Local package suite reported 55/55 and |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Follow-up review pinned to exact head Finding:
The other two prior findings are fully fixed: normalized child props now win after Radix Slot merging while described-by tokens are ordered and deduplicated, and Field.Control explicitly rejects Fragment controls. The focused tests exercise conflicting child id/ARIA state and the Fragment misuse path. LAT now has nine distinct field leaf specs with one adjacent test reference each; Live GitHub state at review time: lint, stack, security, and the rate-limited CodeRabbit status are complete; builds, typechecks/tests, and E2E are still running. |
|
Final follow-up review pinned to exact head LGTM — no findings. The sole residual is fully fixed. All earlier findings remain cleared:
The final remediation delta touches only |
d6316c1
into
codex/series-event-movements-form-context-fix
1 participant
Summary
FieldandFieldGroupcompounds at the direct@repo/ui/fieldsubpathStack
codex/series-event-movements-form-context-fix)aa3bc60e3Validation
pnpm --filter @repo/ui test(55 tests)pnpm --filter @repo/ui type-checkpnpm --filter @repo/ui buildlat checkmainSummary by cubic
Added
Field,FieldGroup,EmptyState, andMetricprimitives in@repo/uito standardize forms, empty states, and summary metrics, improve accessibility, and tighten mobile responsiveness; adopted across Crew and Start via thin re-exports.New Features
@repo/ui/empty-stateand@repo/ui/metricwith Card/Root composition; Storybook showcases typical patterns.@repo/ui/field(ignores empty fragment metadata) and composedFieldGroupfor grouped controls.Bug Fixes
fieldset/legendviaFieldGroup, and preserved empty-state actions.Written for commit d6316c1. Summary will update on new commits.
Summary by CodeRabbit
New Features
FieldandFieldGroupcomponents for accessible form layouts.Documentation
Tests