feat(ui): Add Tier 2 important components#17
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds eight new UI components (Breadcrumbs, ConfirmButton, FormField, Pagination, SplitButton, Stepper, TagInput, Toolbar) with implementations, styles, stories, and tests; introduces pagination range utility; updates package barrel exports and COMPONENT_STATUS documentation; extends Popover Storybook examples. Changes
Sequence Diagram(s)(Skipped — changes are additions of independent components and supporting utilities; no new multi-component control flow warranting a sequence diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.module.css`:
- Around line 7-13: Update the lg breadcrumb font-size variable so the large
variant is visually larger than default: change the declaration in
.Root[data-ov-size='lg'] to use a larger size token (e.g.,
--ov-font-size-subheading or the component-consistent token like
--ov-size-typography-text-lg) instead of --ov-font-size-body so the lg variant
differs from the default; edit the .Root[data-ov-size='lg'] rule to assign the
chosen larger CSS variable.
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.test.tsx`:
- Around line 106-118: Update the test "renders links for items with href" so it
actually exercises the branch described in the comment: render the second item
as <Breadcrumbs.Item active href="/current">Current</Breadcrumbs.Item> (instead
of active-only) and then assert that screen.queryByRole('link', { name:
'Current' }) is not in the document; this keeps the existing first assertion for
Home and verifies Breadcrumbs.Item respects active and suppresses link rendering
even when href is provided.
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsx`:
- Around line 76-78: The collapse logic in the Breadcrumbs component can produce
overlapping/duplicate slices when itemsBeforeCollapse and itemsAfterCollapse
don't fit within maxItems; clamp and compute safe effectiveBefore and
effectiveAfter values before performing slices. Specifically, in the Breadcrumbs
component use a guard that if items.length > maxItems computes effectiveBefore =
Math.min(Math.max(0, itemsBeforeCollapse), Math.max(0, maxItems - 1 -
itemsAfterCollapse)) and then effectiveAfter = Math.min(Math.max(0,
itemsAfterCollapse), Math.max(0, maxItems - 1 - effectiveBefore)); use these
effectiveBefore/effectiveAfter when creating the front and back slices (instead
of the raw props) so slices cannot overlap and total rendered entries (including
the collapse placeholder) never exceed maxItems; also ensure when items.length
<= maxItems you skip collapsing entirely.
In `@packages/base-ui/src/components/confirm-button/ConfirmButton.test.tsx`:
- Around line 91-105: The test for ConfirmButton's disabled behavior should also
assert the native disabled attribute on the rendered element: in the 'disabled
state prevents interaction' test (ConfirmButton in ConfirmButton.test.tsx) after
retrieving the button via screen.getByRole('button', { name: 'Delete' }), add an
assertion that the underlying button element has the disabled state (e.g.,
expect(button).toBeDisabled() or expect(button).toHaveAttribute('disabled')) so
the component forwards the disabled prop to the native <button> and preserves
keyboard/focus semantics.
- Around line 130-145: The test "cleans up timeout on unmount" currently only
ensures vi.advanceTimersByTime() doesn't throw, which doesn't prove the timer
was cleared; update the test for ConfirmButton to make cleanup observable by (a)
passing a mock onConfirm (vi.fn()) when rendering with renderWithTheme, (b)
after firing the click and calling unmount(), assert that vi.getTimerCount() is
0 (or that clearTimeout was called via a mocked global clearTimeout), and then
call vi.advanceTimersByTime(3000) and assert the onConfirm mock was not called;
this ensures the timeout was actually cleared on unmount.
In `@packages/base-ui/src/components/confirm-button/ConfirmButton.tsx`:
- Around line 42-75: The ConfirmButton currently allows callers to pass an
onClick via props which can override the internal confirmation flow; update the
ConfirmButtonProps type to omit 'onClick' from the HTML attributes (e.g., change
Omit<HTMLAttributes<HTMLButtonElement>, 'color'> to
Omit<HTMLAttributes<HTMLButtonElement>, 'color' | 'onClick'>) so that the
component's handleClick/onConfirm logic (used in handleClick and attached as
onClick) cannot be overridden by {...props}; ensure no other places rely on a
passed onClick and keep using onConfirm for caller callbacks.
- Around line 6-15: The prop type currently extends
Omit<HTMLAttributes<HTMLButtonElement>, 'color'> which exposes generic HTML
attributes instead of native button attributes; update the import to pull in
type ButtonHTMLAttributes from 'react' and change ConfirmButtonProps to extend
Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'color'>, StyledComponentProps so
consumers get native button props like form, name, and value (refer to
ConfirmButtonProps in ConfirmButton.tsx).
In `@packages/base-ui/src/components/form-field/FormField.stories.tsx`:
- Around line 39-47: Move the existing inputStyle constant earlier in the file
(just after the imports) and replace all duplicated inline style objects in the
story components Playground, WithDescription, WithError, and RequiredField with
references to that shared inputStyle; for WithError use a spread override (e.g.,
{ ...inputStyle, border: '1px solid var(--ov-color-fg-danger)' }) to change the
border only. Locate and remove the inline style blocks currently defined inside
those stories (the objects at the positions corresponding to 64-72, 89-97,
115-123, and the current 129-139) and ensure each story's JSX uses
style={inputStyle} or the spread override for error state.
In `@packages/base-ui/src/components/form-field/FormField.tsx`:
- Around line 37-47: The current FormField renders a standalone <label> when
htmlFor is optional, leaving the control unassociated; update FormField to
conditionally render: if htmlFor is provided keep the existing separate <label
htmlFor={htmlFor}> and <div className={styles.Control}>{children}</div>, but if
htmlFor is absent wrap the control inside the same <label> (move the <div
className={styles.Control}>{children}</div> into the <label> along with the
label text/required/description) so the input (children) is programmatically
associated; adjust rendering logic around the htmlFor prop in the FormField
component accordingly.
- Line 1: Change the props type for the fieldset-rendering component to use
FieldsetHTMLAttributes instead of generic HTMLAttributes: update the
FormSectionProps (in FormField.tsx) so it extends
FieldsetHTMLAttributes<HTMLFieldSetElement> rather than
HTMLAttributes<HTMLFieldSetElement>, and ensure any imports reflect this change
(replace or add import of FieldsetHTMLAttributes from 'react'); no other
behavior changes are required since the component renders a <fieldset>.
In `@packages/base-ui/src/components/pagination/Pagination.module.css`:
- Around line 57-63: The selectors using chained :not() on .Item should use the
modern complex :not() notation; replace
.Item:not([disabled]):not([data-ov-active='true']):hover with
.Item:not(:disabled, [data-ov-active='true']):hover and likewise replace
.Item:not([disabled]):not([data-ov-active='true']):active with
.Item:not(:disabled, [data-ov-active='true']):active so the rules target the
same states but satisfy the Stylelint selector-not-notation rule (update both
hover and active rules referencing .Item and the data-ov-active attribute).
In `@packages/base-ui/src/components/pagination/Pagination.test.tsx`:
- Around line 63-67: The test "renders ellipsis with many pages" in
Pagination.test.tsx is too broad because
container.querySelectorAll('[aria-hidden="true"]') picks up hidden nav icons;
change the selector to target the actual ellipsis wrappers instead (e.g., query
elements whose visible text is the ellipsis character "…" using
testing-library's getAllByText or a querySelectorAll that checks
element.textContent === '…'), then assert that count is >= 2; update the
assertion in the test for the Pagination component accordingly.
In `@packages/base-ui/src/components/pagination/Pagination.tsx`:
- Around line 47-53: The click handler currently always calls onChange(target)
even when target equals the active page; update the handlers (handleClick and
the similar handlers around lines 95-101) to guard against no-op changes by only
invoking onChange when target is within range AND target !== page (the current
active page prop/state). Locate the symbol handleClick and any other
change-invoking functions (e.g., previous/next handlers) and add the target !==
page check before calling onChange to avoid emitting redundant events.
In `@packages/base-ui/src/components/split-button/SplitButton.stories.tsx`:
- Around line 41-43: The menu action buttons in SplitButton.stories.tsx are
missing explicit types and default to type="submit", so update each menu action
<button> used in the story (the three buttons in the first menu and the other
occurrences around the file as noted) to include type="button"; search for the
buttons rendered inside the story (e.g., the menu item buttons in
SplitButton.stories.tsx) and add type="button" to each JSX <button> element to
prevent accidental form submissions in form contexts.
In `@packages/base-ui/src/components/split-button/SplitButton.test.tsx`:
- Line 13: The inline JSX <button> elements in SplitButton.test.tsx (e.g., the
one rendering "Save as Draft" and the other inline menu buttons) lack an
explicit type and therefore default to submit when rendered inside a form;
update each inline button element to include type="button" (apply the same
change to every inline menu <button> occurrence in the file) so they no longer
have submit semantics during tests.
In `@packages/base-ui/src/components/split-button/SplitButton.tsx`:
- Around line 86-88: The forwarded ref on SplitButtonMenu is declared as
forwardRef<HTMLDivElement, SplitButtonMenuProps> but is cast and applied to the
trigger <button>, causing a type mismatch; update the implementation so the
forwarded ref actually targets the container <div> (or, if you need the ref on
the button, change forwardRef's element type to HTMLButtonElement and update
Prop types accordingly). Locate the forwardRef call for SplitButtonMenu and
either move the forwarded ref to the container div (recommended) or change the
forwardRef generic from HTMLDivElement to HTMLButtonElement and adjust any ref
casts (the React.Ref<HTMLButtonElement> cast and places where ref is passed into
the trigger button) so types are consistent.
- Around line 117-138: Add an explicit id on the dropdown element and reference
it from the trigger button via aria-controls to create a clear ARIA
relationship: update the button (the element using ref, className
styles.MenuTrigger, aria-haspopup, aria-expanded, onClick={toggle}) to include
aria-controls="<dropdownId>" and ensure the dropdown div (className
styles.Dropdown, role="menu") has the same id. Generate a stable id for the pair
(e.g. with React's useId or a prop-generated id) so aria-controls always points
to the correct dropdown element even if multiple SplitButton instances are
rendered.
In `@packages/base-ui/src/components/stepper/Stepper.stories.tsx`:
- Around line 14-16: The story's activeStep control allows a value (max: 4) that
can exceed the actual last step index for the Stepper story, leaving the
component in an out-of-range state; update the Stepper.stories.tsx story to
clamp activeStep to the real last index by using the actual steps length (e.g.,
derive max from steps.length - 1) and also guard any handlers that advance the
step (such as the Finish/next-step handler used in the story) to set activeStep
= Math.min(activeStep, steps.length - 1) so interactive clicks cannot push the
state past the last real step; apply the same clamp/update to the other story
variant mentioned (lines 158-163).
In `@packages/base-ui/src/components/tag-input/TagInput.module.css`:
- Around line 28-30: The lg size variant in TagInput.module.css
(.Root[data-ov-size='lg']) only sets --_ov-min-height but fails to override the
font size like the sm variant does; update that selector to also set
--_ov-font-size to the large body token (e.g., --ov-font-size-body-lg or the
project's equivalent token) so the lg TagInput uses a proportionally larger font
consistent with other typography components and the sm rule.
In `@packages/base-ui/src/components/tag-input/TagInput.tsx`:
- Around line 75-83: The splitByDelimiter callback contains redundant branches
(both if and else call text.split(delimiter)); simplify by removing the
conditional and return text.split(delimiter) directly inside the
splitByDelimiter function (keep the useCallback and its dependency on delimiter
as-is) so the logic is identical but clearer; update the function reference
splitByDelimiter and ensure no other callers rely on the previous branching.
- Around line 146-154: The container div in TagInput.tsx uses onClick (handled
by handleContainerClick) but lacks semantic role; update the root div to include
role="group" and add aria-disabled={disabled} to mirror data-ov-disabled for
better accessibility and screen-reader semantics, while leaving focus and
keyboard interaction to the internal input; ensure you modify the JSX in the
TagInput component where ref, className, data-ov-* props and onClick are defined
(the element that currently spreads {...props}) to include role="group" and
aria-disabled={disabled}.
In `@packages/base-ui/src/components/toolbar/Toolbar.module.css`:
- Around line 10-13: The stylelint failure comes from the
.Root[data-ov-size='sm'] rule where the second declaration (font-size:
var(--ov-font-size-caption)) violates declaration-empty-line-before; open
Toolbar.module.css and either insert a single blank line before the font-size
declaration (i.e., leave a blank line after --_ov-toolbar-height: 1.75rem;) so
declarations obey the empty-line rule, or if intentional, add a specific
stylelint disable comment scoped to this rule; reference the
.Root[data-ov-size='sm'] selector and the --_ov-toolbar-height and font-size
declarations when making the change.
In `@packages/base-ui/src/components/toolbar/Toolbar.stories.tsx`:
- Around line 18-25: The buttons inside the Toolbar story examples lack explicit
type attributes; update every <button> element in this file (notably the buttons
within Toolbar.Group in the Toolbar.stories component) to include type="button"
to avoid implicit submit behavior—apply the same change to all other story
button instances referenced in the review (the other groups at lines noted in
the comment) so every <button> has type="button".
In `@packages/base-ui/src/components/toolbar/Toolbar.test.tsx`:
- Line 11: The test contains plain button elements (e.g., the
<button>Save</button> instance in Toolbar.test.tsx) missing an explicit type
attribute; update each button to include type="button" to avoid implicit submit
behavior and satisfy lint rules, and apply the same change to the other button
occurrences mentioned (lines with buttons at 66, 67, 80, 83, 96, 110, and 122)
so every <button> element in the file becomes <button type="button">…</button>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a68e21eb-7c56-4c89-91cf-9670108dd6fb
📒 Files selected for processing (44)
docs/COMPONENT_STATUS.mdpackages/base-ui/src/components/breadcrumbs/Breadcrumbs.module.csspackages/base-ui/src/components/breadcrumbs/Breadcrumbs.stories.tsxpackages/base-ui/src/components/breadcrumbs/Breadcrumbs.test.tsxpackages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsxpackages/base-ui/src/components/breadcrumbs/index.tspackages/base-ui/src/components/confirm-button/ConfirmButton.module.csspackages/base-ui/src/components/confirm-button/ConfirmButton.stories.tsxpackages/base-ui/src/components/confirm-button/ConfirmButton.test.tsxpackages/base-ui/src/components/confirm-button/ConfirmButton.tsxpackages/base-ui/src/components/confirm-button/index.tspackages/base-ui/src/components/form-field/FormField.module.csspackages/base-ui/src/components/form-field/FormField.stories.tsxpackages/base-ui/src/components/form-field/FormField.test.tsxpackages/base-ui/src/components/form-field/FormField.tsxpackages/base-ui/src/components/form-field/index.tspackages/base-ui/src/components/index.tspackages/base-ui/src/components/pagination/Pagination.module.csspackages/base-ui/src/components/pagination/Pagination.stories.tsxpackages/base-ui/src/components/pagination/Pagination.test.tsxpackages/base-ui/src/components/pagination/Pagination.tsxpackages/base-ui/src/components/pagination/index.tspackages/base-ui/src/components/pagination/usePaginationRange.tspackages/base-ui/src/components/popover/Popover.stories.tsxpackages/base-ui/src/components/split-button/SplitButton.module.csspackages/base-ui/src/components/split-button/SplitButton.stories.tsxpackages/base-ui/src/components/split-button/SplitButton.test.tsxpackages/base-ui/src/components/split-button/SplitButton.tsxpackages/base-ui/src/components/split-button/index.tspackages/base-ui/src/components/stepper/Stepper.module.csspackages/base-ui/src/components/stepper/Stepper.stories.tsxpackages/base-ui/src/components/stepper/Stepper.test.tsxpackages/base-ui/src/components/stepper/Stepper.tsxpackages/base-ui/src/components/stepper/index.tspackages/base-ui/src/components/tag-input/TagInput.module.csspackages/base-ui/src/components/tag-input/TagInput.stories.tsxpackages/base-ui/src/components/tag-input/TagInput.test.tsxpackages/base-ui/src/components/tag-input/TagInput.tsxpackages/base-ui/src/components/tag-input/index.tspackages/base-ui/src/components/toolbar/Toolbar.module.csspackages/base-ui/src/components/toolbar/Toolbar.stories.tsxpackages/base-ui/src/components/toolbar/Toolbar.test.tsxpackages/base-ui/src/components/toolbar/Toolbar.tsxpackages/base-ui/src/components/toolbar/index.ts
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
packages/base-ui/src/components/pagination/Pagination.tsx (1)
95-104: 🧹 Nitpick | 🔵 TrivialConsider disabling the active page button for clearer accessibility.
The active page button correctly won't fire
onChange, but it remains keyboard-focusable and announced as interactive. Addingdisabledwould provide clearer semantics to assistive technologies.♿ Suggested improvement
<button key={item} type="button" className={styles.Item} aria-current={item === page ? 'page' : undefined} data-ov-active={item === page ? 'true' : undefined} + disabled={item === page} onClick={handleClick(item)} > {item} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/pagination/Pagination.tsx` around lines 95 - 104, The active page button should be marked disabled for accessibility: update the button rendering (the element using styles.Item and props aria-current/data-ov-active) to include disabled={item === page} and prevent attaching the click handler when disabled (e.g., onClick={item === page ? undefined : handleClick(item)}); keep aria-current for semantics but ensure keyboard focus/interaction is removed for the active page.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.test.tsx`:
- Around line 21-25: The test currently uses
nav.querySelectorAll('[aria-hidden="true"]') which matches both separator
wrappers and nested SVGs; replace that selector with a precise one that targets
separator elements (e.g. nav.querySelectorAll('[role="separator"]') or a
component-specific data attribute like '[data-testid="breadcrumb-separator"]')
and assert the exact expected count (expect(separators.length).toBe(2)) so the
test fails on separator regressions; update the references to nav and separators
in Breadcrumbs.test.tsx accordingly.
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsx`:
- Around line 15-16: The prop docs for separator are incorrect: update the JSDoc
for the separator prop in the Breadcrumbs component to reflect the actual
default (the LuChevronRight icon) instead of `'/'`; locate the separator?:
ReactNode declaration in Breadcrumbs.tsx and change the comment to mention
LuChevronRight (or describe "chevron right icon") so generated docs match the
implementation.
- Around line 88-121: The collapse logic can produce an ellipsis-only trail when
effectiveBefore and effectiveAfter are both 0; modify the shouldCollapse/branch
before building visibleItems so that if effectiveBefore + effectiveAfter === 0
you skip collapsing and set visibleItems = childArray (i.e., leave expanded
false but render all items), using the existing variables (shouldCollapse,
effectiveBefore, effectiveAfter, childArray, visibleItems) and keep the ellipsis
button/behavior unchanged when there are real items to hide; ensure this check
occurs after computing effectiveBefore/effectiveAfter and before constructing
visibleItems and the ellipsis.
In `@packages/base-ui/src/components/confirm-button/ConfirmButton.tsx`:
- Around line 14-15: The component currently allows consumers to override the
internal type="button" and lets a submit happen on the first click; fix it by 1)
removing 'type' from the inherited props so consumers cannot pass type (change
the interface to extend Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'color' |
'onClick' | 'type'>) so the component-controlled type="button" on ConfirmButton
remains authoritative, and 2) update handleClick to accept the click event and
call event.preventDefault() when !confirming (i.e., on the first click that only
toggles confirming) to ensure a submit is blocked even if type somehow ends up
as submit.
In `@packages/base-ui/src/components/form-field/FormField.test.tsx`:
- Around line 11-137: Add a test to verify the wrapped-label accessibility
pattern in FormField: when htmlFor is omitted the control should be wrapped by
the label. In the FormField test suite, use renderWithTheme to render <FormField
label="Wrapped"> with an <input data-testid="wrapped-input" />, then locate the
label via screen.getByText('Wrapped').closest('label') and assert that the label
contains the input (screen.getByTestId('wrapped-input')). Reference the
FormField component and existing renderWithTheme/screen helpers to implement
this new "wraps children inside label when htmlFor is absent" test.
In `@packages/base-ui/src/components/form-field/FormField.tsx`:
- Around line 51-62: The JSX renders a <label> wrapper that contains block-level
<p> and <div> elements when htmlFor is not provided, which is invalid HTML;
update the alternate branch in FormField.tsx to replace the <p
className={styles.Description}> and <div className={styles.Control}> with inline
elements (e.g., <span>) so they remain valid phrasing content inside the
<label>, and adjust .Description and .Control CSS to display: block to preserve
layout; ensure the required star span remains unchanged and children are
rendered inside the new inline container (refer to label, description, children,
styles.Description, and styles.Control).
In `@packages/base-ui/src/components/pagination/Pagination.test.tsx`:
- Around line 71-77: Add a regression test to ensure the Pagination component
does not emit onChange when the active page is clicked: in Pagination.test.tsx
add a test that renders <Pagination count={5} page={3} onChange={onChange} />
(using renderWithTheme), simulates a click via
fireEvent.click(screen.getByRole('button', { name: '3' })), and asserts that the
mocked onChange function was not called
(expect(onChange).not.toHaveBeenCalled()) to protect the guard in the Pagination
component.
In `@packages/base-ui/src/components/split-button/SplitButton.tsx`:
- Around line 145-148: The dropdown currently renders as a menu (role="menu")
but accepts arbitrary children and does not implement menu behaviors; remove the
misleading role or implement the full menu pattern: either delete role="menu"
from the div rendered in SplitButton (the element using dropdownId and
styles.Dropdown) so it becomes a generic popover/list, or else implement
keyboard navigation (arrow keys), Escape handling, focus management, and ensure
child items have role="menuitem" (or the appropriate actionable roles) within
SplitButton.Menu so the ARIA semantics are correct.
- Around line 128-143: The chevron-only trigger button in SplitButton.tsx (the
element with className styles.MenuTrigger, props isDisabled, open, dropdownId
and onClick={toggle}) lacks an accessible name; add an aria-label (e.g.,
aria-label="More actions") or allow a prop (e.g., triggerLabel) and set
aria-label={triggerLabel || "More actions"} on that button so screen readers can
announce it; keep the LuChevronDown aria-hidden so the icon remains decorative.
- Around line 26-28: Update the SplitButtonActionProps type to use
ButtonHTMLAttributes<HTMLButtonElement> instead of
HTMLAttributes<HTMLButtonElement> so all native button props (form, name, value,
autoFocus, etc.) are supported; keep the existing disabled?: boolean override.
Import ButtonHTMLAttributes from React and replace the Omit reference
(Omit<HTMLAttributes<HTMLButtonElement>, 'color'>) with
Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'color'> in the
SplitButtonActionProps declaration and ensure any usage in SplitButton.Action or
prop forwarding still type-checks.
In `@packages/base-ui/src/components/tag-input/TagInput.module.css`:
- Around line 76-82: The hover rule currently changes .TagRemove color even when
the button is disabled; update TagInput.module.css to explicitly override the
hover state for disabled buttons by adding a more specific selector such as
.TagRemove:disabled:hover { color: inherit; } (or set to the muted/dim color
variable you use) placed after the existing .TagRemove:hover rule so the
disabled hover won’t apply, and keep the existing .TagRemove:disabled { cursor:
not-allowed; } behavior.
In `@packages/base-ui/src/components/toolbar/Toolbar.stories.tsx`:
- Around line 94-99: The label in Toolbar.stories.tsx inside the size demo map
uses a hardcoded color "#888" which bypasses theming; update the inline style on
that label (the div rendering size="{size}") to avoid a fixed hex color and
instead inherit the current text color or use a theme token/opacity-based muted
color (e.g., use 'color: inherit' or the project's muted color variable) so the
label respects dark/high-contrast themes during visual review.
In `@packages/base-ui/src/components/toolbar/Toolbar.test.tsx`:
- Around line 76-90: The test currently only checks that Toolbar.Group sets
data-ov-separator="true" but doesn't assert that the actual visible separator
node is rendered; update the test in Toolbar.test.tsx to also assert the
presence of the Separator element produced by the component (e.g., query the DOM
for the separator node – by role "separator", a test id, or the CSS class name
used in Toolbar.module.css such as "Separator" – and expect it to be in the
document), keeping the existing check for data-ov-separator on Toolbar.Group and
adding one assertion that the actual separator element is rendered and visible.
---
Duplicate comments:
In `@packages/base-ui/src/components/pagination/Pagination.tsx`:
- Around line 95-104: The active page button should be marked disabled for
accessibility: update the button rendering (the element using styles.Item and
props aria-current/data-ov-active) to include disabled={item === page} and
prevent attaching the click handler when disabled (e.g., onClick={item === page
? undefined : handleClick(item)}); keep aria-current for semantics but ensure
keyboard focus/interaction is removed for the active page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d9f0295-285d-42b4-9f28-1245b9b5594f
📒 Files selected for processing (20)
packages/base-ui/src/components/breadcrumbs/Breadcrumbs.module.csspackages/base-ui/src/components/breadcrumbs/Breadcrumbs.test.tsxpackages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsxpackages/base-ui/src/components/confirm-button/ConfirmButton.test.tsxpackages/base-ui/src/components/confirm-button/ConfirmButton.tsxpackages/base-ui/src/components/form-field/FormField.stories.tsxpackages/base-ui/src/components/form-field/FormField.test.tsxpackages/base-ui/src/components/form-field/FormField.tsxpackages/base-ui/src/components/pagination/Pagination.module.csspackages/base-ui/src/components/pagination/Pagination.test.tsxpackages/base-ui/src/components/pagination/Pagination.tsxpackages/base-ui/src/components/split-button/SplitButton.stories.tsxpackages/base-ui/src/components/split-button/SplitButton.test.tsxpackages/base-ui/src/components/split-button/SplitButton.tsxpackages/base-ui/src/components/stepper/Stepper.stories.tsxpackages/base-ui/src/components/tag-input/TagInput.module.csspackages/base-ui/src/components/tag-input/TagInput.tsxpackages/base-ui/src/components/toolbar/Toolbar.module.csspackages/base-ui/src/components/toolbar/Toolbar.stories.tsxpackages/base-ui/src/components/toolbar/Toolbar.test.tsx
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Add 8 new components and audit Popover for completeness: - Breadcrumbs: Hierarchical navigation trail with collapse support - Pagination: Page navigation with ellipsis, boundary/sibling counts - Stepper: Step-by-step progress indicator (horizontal/vertical) - Toolbar: Horizontal action bar with groups and separators - SplitButton: Primary action button with dropdown menu - ConfirmButton: Two-click confirmation button with timeout - TagInput: Input field that creates tag chips from typed text - FormField/FormSection: Form layout wrappers with label/error/description - Popover audit: Already complete, added comprehensive story variants Each component includes .tsx, .module.css, .test.tsx, .stories.tsx, and index.ts barrel export. All registered in components/index.ts and docs/COMPONENT_STATUS.md. 518 tests passing, zero new lint errors.
Ensure all Tier 2 components use react-icons/lu exclusively — no raw Unicode characters, HTML entities, emojis, or inline SVGs.
- Breadcrumbs: use larger font token for lg size, clamp collapse slices to prevent overlap, test active+href suppresses link rendering - ConfirmButton: use ButtonHTMLAttributes, omit onClick from props, assert disabled attribute in tests, verify timer cleanup on unmount - FormField: wrap control inside label when htmlFor is absent for implicit association, use FieldsetHTMLAttributes for FormSection, deduplicate inputStyle in stories - Pagination: use modern :not() notation in CSS, target span elements for ellipsis test, skip onChange when target equals current page - SplitButton: add type="button" to all menu buttons in stories/tests, fix ref to target container div, add aria-controls/id for dropdown - Stepper: clamp activeStep control max to last index, cap interactive demo at steps.length - 1 - TagInput: add lg font-size token, simplify splitByDelimiter, add role="group" and aria-disabled to container - Toolbar: fix declaration-empty-line-before in CSS, add type="button" to all buttons in stories/tests
- Breadcrumbs: fix separator JSDoc, use data-ov-role selector in tests for precise separator targeting, guard against ellipsis-only collapse - ConfirmButton: omit 'type' from props, preventDefault on first click - FormField: use span instead of p/div inside wrapping label for valid HTML phrasing content, add display:block to .Description CSS, add test for wrapped-label accessibility pattern - Pagination: disable active page button, skip onClick for current page, add regression test for no-op click on active page - SplitButton: use ButtonHTMLAttributes for Action props, add aria-label to menu trigger, remove misleading role="menu" from dropdown - TagInput: prevent hover color change on disabled remove button - Toolbar: replace hardcoded #888 with opacity for theme compliance, assert separator element is rendered in tests
joshuapare
force-pushed
the
feat/tier2-important-components
branch
from
921adc9 to
a0ef233
Compare
There was a problem hiding this comment.
Actionable comments posted: 23
♻️ Duplicate comments (1)
packages/base-ui/src/components/form-field/FormField.tsx (1)
19-20:⚠️ Potential issue | 🟠 MajorRequire an explicit control association instead of wrapping arbitrary children in
<label>.The fallback branch only works when
childrenresolves to a single labelable control with valid phrasing-content markup. For composite inputs or multiple descendants, the label is still not reliably associated. MakinghtmlForoptional leaves this component unable to guarantee accessible output.Also applies to: 51-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/form-field/FormField.tsx` around lines 19 - 20, The FormField component currently allows htmlFor to be optional and falls back to wrapping children inside a <label>, which fails to reliably associate composite or multiple descendant controls; make htmlFor a required prop on the FormField props (remove the optional marker from htmlFor), update the render logic in the FormField component to always render the label element with htmlFor={htmlFor} (do not rely on wrapping arbitrary children), and adjust any callers/tests to pass an explicit htmlFor tied to the control id so label-control association is always explicit and accessible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsx`:
- Around line 85-88: The expanded state in Breadcrumbs (expanded, setExpanded)
is sticky across prop changes; add a useEffect inside the Breadcrumbs component
that resets setExpanded(false) whenever the breadcrumb input changes (watch
children or derived childArray length and maxItems) so the collapsed/expanded
logic (shouldCollapse) recalculates for new trails; reference the
expanded/setExpanded state and the childArray/totalItems or children prop in the
effect cleanup to ensure expansion is cleared when the trail updates.
- Around line 27-30: Replace the current BreadcrumbItemProps interface with a
discriminated union so Breadcrumbs.Item exposes anchor-specific props when href
is present: define one variant that uses HTMLAttributes<HTMLElement> with href?:
never for non-link items and another variant that uses
AnchorHTMLAttributes<HTMLAnchorElement> with href: string for link items; update
the public type used by Breadcrumbs.Item and remove the runtime type casts
currently applied where the component renders an <a> (the casts around the
link-rendering branch) so callers can pass target, rel, download, etc., with
proper typing.
In `@packages/base-ui/src/components/confirm-button/ConfirmButton.stories.tsx`:
- Around line 21-29: The Storybook argTypes for ConfirmButton omit the shipped
"info" color; update the argTypes in ConfirmButton.stories.tsx so the color and
confirmColor controls include 'info' in their options arrays (the entries
currently defining color and confirmColor) to accurately reflect ConfirmButton's
supported colors and enable visual testing of the info state.
In `@packages/base-ui/src/components/confirm-button/ConfirmButton.tsx`:
- Around line 49-55: In handleClick (ConfirmButton) clear the confirming state
before calling external onConfirm to avoid leaving the button stuck if onConfirm
throws or unmounts the component: call setConfirming(false) immediately when
confirming is true, then invoke onConfirm(); optionally wrap onConfirm in
try/catch if you need to handle synchronous errors, but the key change is to
move setConfirming(false) to execute prior to onConfirm() so the internal reset
always happens.
In `@packages/base-ui/src/components/index.ts`:
- Around line 5-6: The barrel file exports are out of alphabetical order:
'breadcrumbs' appears before 'avatar' and several later-added exports were
appended instead of being merged; please reorder all export statements
alphabetically (e.g., place 'avatar' before 'breadcrumbs') and integrate the new
exports into that sorted sequence so the entire exports list is consistently
alphabetical; update the export block containing 'avatar', 'breadcrumbs', and
the recently added exports to reflect the new ordering.
In `@packages/base-ui/src/components/pagination/Pagination.module.css`:
- Around line 67-79: The active page button (set in Pagination.tsx via
data-ov-active="true") is being faded by the generic disabled selector
(.Item:disabled); update Pagination.module.css so the disabled rule excludes the
active item (e.g. change .Item:disabled to
.Item:disabled:not([data-ov-active='true'])) or explicitly override the active
rule (add .Item[data-ov-active='true']:disabled { opacity: 1; cursor: default;
}) so the current page keeps its brand background, full opacity and default
cursor instead of looking unavailable.
In `@packages/base-ui/src/components/pagination/Pagination.stories.tsx`:
- Around line 80-93: The stories FewPages, ManyPages and WithoutFirstLast ignore
Storybook controls by hardcoding props in their render functions; update each
render to accept the Storybook args parameter and spread it into
SimplePagination (e.g., render: (args) => <SimplePagination {...args} />) and
move story-specific defaults into the story's args (set count, page,
showFirstLast, etc.), leaving AllSizesPagination unchanged if it manages its own
states; reference the SimplePagination component and the story names FewPages,
ManyPages, WithoutFirstLast when making these changes.
- Around line 5-16: ControlledPagination uses useState(props.page) so its local
page won't follow Storybook control changes or clamp when count shrinks; add a
useEffect in ControlledPagination that listens to props.page and props.count and
calls setPage(Math.min(props.page, props.count)) (or otherwise clamps to valid
bounds) so the local page state stays in sync with props and Pagination's
onChange continues to call props.onChange as before.
In `@packages/base-ui/src/components/pagination/Pagination.test.tsx`:
- Around line 14-31: Tighten the three pagination tests to assert exact expected
arrays from buildPaginationRange rather than loose checks: for "places right
ellipsis when current page is near start" (buildPaginationRange(10, 2, 1, 1))
assert [1, 2, 3, 'ellipsis', 10]; for "places left ellipsis when current page is
near end" (buildPaginationRange(10, 9, 1, 1)) assert [1, 'ellipsis', 8, 9, 10];
and for "places both ellipses when current page is in the middle"
(buildPaginationRange(20, 10, 1, 1)) assert [1, 'ellipsis', 9, 10, 11,
'ellipsis', 20]; update the tests that reference buildPaginationRange to use
these exact array equality assertions.
In `@packages/base-ui/src/components/pagination/usePaginationRange.ts`:
- Around line 42-45: The expression computing siblingsEnd uses a non-null
assertion on endPages[0] in the Math.min call; replace or augment that usage by
either providing a defensive fallback (e.g., use endPages[0] ?? count to avoid
the `!`) or add a short clarifying comment above the computation documenting the
invariant that endPages is guaranteed non-empty when this branch runs; update
the code around the siblingsEnd computation (symbol: siblingsEnd) and reference
endPages to implement the fallback or add the comment so future maintainers
won't be confused by the non-null assertion.
In `@packages/base-ui/src/components/split-button/SplitButton.stories.tsx`:
- Around line 25-34: The menuItemStyle declaration uses React.CSSProperties but
this file does not import React; update the module to import the CSSProperties
type directly from 'react' and use that type instead of React.CSSProperties
(i.e., replace React.CSSProperties with CSSProperties) so the menuItemStyle
variable is properly type-checked; ensure the import statement includes
CSSProperties (type import) to keep imports minimal.
In `@packages/base-ui/src/components/split-button/SplitButton.tsx`:
- Around line 94-116: The dropdown can remain open when the component becomes
disabled; update the effects to force-close it when isDisabled flips true: add a
useEffect that watches isDisabled (and optionally open) and calls setOpen(false)
whenever isDisabled is true so the menu is closed as soon as the root becomes
disabled; apply the same change to the other similar effect/instance referenced
around the other block (the one at lines ~147-150) and ensure you reference the
same state and refs (open, setOpen, isDisabled, containerRef) so the dropdown
cannot remain interactive when disabled.
- Around line 129-149: The button in SplitButton.tsx currently sets
aria-haspopup="true" (see the MenuTrigger button using props
aria-expanded={open} and aria-controls={dropdownId}), which falsely advertises
an ARIA menu for a generic dropdown container; remove the aria-haspopup
attribute from the MenuTrigger button (or replace it with a more specific token
only if the dropdown is a real ARIA menu/listbox) so the trigger’s accessible
semantics match the actual dropdown implementation.
- Around line 65-67: SplitButtonAction currently computes isDisabled as
disabledProp ?? ctx?.disabled which lets an explicit action disabled={false}
override a disabled root; change the precedence so the root wins by computing
isDisabled from the context first: replace the expression with ctx?.disabled ??
disabledProp ?? false (or equivalently use a conditional that returns true when
ctx?.disabled === true), updating the isDisabled assignment in the
SplitButtonAction function that references SplitButtonContext and disabledProp
so SplitButton.Root.disabled always overrides SplitButton.Action.disabled.
In `@packages/base-ui/src/components/stepper/Stepper.module.css`:
- Around line 71-78: The vertical Connector uses a fixed height so multi-line
steps break the rail; in the .Step[data-ov-orientation='vertical'] .Connector
rule remove the fixed height and the translateY transform and instead anchor the
connector between a computed top and bottom so it stretches with the step (e.g.
set top: calc(var(--_ov-circle-size) * -1 - var(--ov-space-stack-xs)); set
bottom: 0; remove height and transform). Update the rule so the connector spans
dynamically for multi-line labels while keeping its left/width positioning.
In `@packages/base-ui/src/components/stepper/Stepper.test.tsx`:
- Around line 165-175: The test's connector selector is brittle; update the
Stepper implementation so the connector DOM element includes a stable attribute
(e.g., add data-ov-role="connector" to the element rendered by the
Stepper/Connector logic), then change the test (the case using
renderWithTheme(<Stepper activeStep={0}><Stepper.Step .../>) in
Stepper.test.tsx) to query for connectors via
document.querySelectorAll('[data-ov-role="connector"]') instead of
'[class*="Connector"]'; ensure the attribute is applied where the Stepper
renders the connector between Stepper.Step items so the test reliably finds none
when there is only one step.
- Around line 49-50: The test uses document.querySelectorAll('svg') (assigned to
svgs) which can pick up SVGs outside the component; scope the query to the
rendered Stepper instance instead (e.g., use the container returned by
render(...) or Testing Library's within(...) on a specific step or test-id) and
assert the number of SVGs from that scoped node so external SVGs won't cause
false positives.
In `@packages/base-ui/src/components/stepper/Stepper.tsx`:
- Around line 132-140: The current step element only marks state via
data-ov-status, which isn't exposed to assistive tech; update the JSX return for
the Stepper item (the div with ref, className={cn(styles.Step, className)} and
data-ov-status={status}) to set aria-current="step" when this item is active
(i.e., when the status prop equals the active state used in this component),
otherwise omit the attribute or set it to undefined; ensure you reference the
existing status prop and data-ov-status usage so the attribute is conditionally
added in the same element.
- Around line 72-77: Only clone and inject _index/_isFirst into true Step
components (e.g., Stepper.Step) instead of every valid element: in the mapping
for Children.map(children, ...) detect Step nodes by checking the element type
or a marker prop (for example child.type === Stepper.Step or child.props?.__TYPE
=== 'STEP') and only call cloneElement for those, returning other valid elements
unchanged. To support Stepper.Step wrapped inside other components, detect
wrapper elements that have children and recursively map/transform their children
(replacing the wrapper with a cloned wrapper containing transformed children) so
nested Step components still receive InjectedStepProps; leave arbitrary DOM
nodes untouched. Use existing symbols like children, cloneElement,
isValidElement, InjectedStepProps, StepProps and Stepper.Step to locate and
update the logic.
In `@packages/base-ui/src/components/tag-input/TagInput.tsx`:
- Around line 155-165: The tag remove buttons in TagInput.tsx currently set
tabIndex={-1} (the <button> with className styles.TagRemove and
aria-label={`Remove ${tag}`}) which prevents keyboard users from focusing them;
remove the tabIndex={-1} (or set it to 0) so the native <button> can be reached
via Tab and activated by Enter/Space, and verify removeTag(index) continues to
be called on onClick (no extra keyboard handlers needed).
- Around line 89-113: The handleKeyDown handler currently commits tags on Enter
or delimiter keystrokes even during IME composition; add an early guard at the
start of handleKeyDown to check event.nativeEvent.isComposing (or
event.isComposing if appropriate) and return immediately when composition is
active so Enter/delimiter logic (using inputValue, splitByDelimiter, addTags,
removeTag, delimiter) is skipped while composing.
In `@packages/base-ui/src/components/toolbar/Toolbar.module.css`:
- Around line 35-44: The .Separator rule contains both align-self: stretch and
height: 60%, which is confusing because stretch is effectively overridden by the
explicit height; remove the align-self: stretch declaration from the .Separator
CSS rule so the element uses height: 60% with margin-block: auto to remain
vertically centered (leave other properties like width, background-color,
margin-inline-end, and flex-shrink intact).
In `@packages/base-ui/src/components/toolbar/Toolbar.test.tsx`:
- Around line 91-93: The test uses a broad selector
group.querySelector('[aria-hidden="true"]') which can yield false positives;
replace it with a more specific selector for the Toolbar separator (e.g., use
within(group).queryByRole('separator', { hidden: true }) or a dedicated
class/test-id such as querySelector('.Separator') or
getByTestId('toolbar-separator')), update the import to include within from
`@testing-library/react` if needed, and keep the assertion as
expect(separator).toBeInTheDocument() after selecting the more specific element.
---
Duplicate comments:
In `@packages/base-ui/src/components/form-field/FormField.tsx`:
- Around line 19-20: The FormField component currently allows htmlFor to be
optional and falls back to wrapping children inside a <label>, which fails to
reliably associate composite or multiple descendant controls; make htmlFor a
required prop on the FormField props (remove the optional marker from htmlFor),
update the render logic in the FormField component to always render the label
element with htmlFor={htmlFor} (do not rely on wrapping arbitrary children), and
adjust any callers/tests to pass an explicit htmlFor tied to the control id so
label-control association is always explicit and accessible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d604acb1-7194-4076-8865-34770498a95e
📒 Files selected for processing (44)
docs/COMPONENT_STATUS.mdpackages/base-ui/src/components/breadcrumbs/Breadcrumbs.module.csspackages/base-ui/src/components/breadcrumbs/Breadcrumbs.stories.tsxpackages/base-ui/src/components/breadcrumbs/Breadcrumbs.test.tsxpackages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsxpackages/base-ui/src/components/breadcrumbs/index.tspackages/base-ui/src/components/confirm-button/ConfirmButton.module.csspackages/base-ui/src/components/confirm-button/ConfirmButton.stories.tsxpackages/base-ui/src/components/confirm-button/ConfirmButton.test.tsxpackages/base-ui/src/components/confirm-button/ConfirmButton.tsxpackages/base-ui/src/components/confirm-button/index.tspackages/base-ui/src/components/form-field/FormField.module.csspackages/base-ui/src/components/form-field/FormField.stories.tsxpackages/base-ui/src/components/form-field/FormField.test.tsxpackages/base-ui/src/components/form-field/FormField.tsxpackages/base-ui/src/components/form-field/index.tspackages/base-ui/src/components/index.tspackages/base-ui/src/components/pagination/Pagination.module.csspackages/base-ui/src/components/pagination/Pagination.stories.tsxpackages/base-ui/src/components/pagination/Pagination.test.tsxpackages/base-ui/src/components/pagination/Pagination.tsxpackages/base-ui/src/components/pagination/index.tspackages/base-ui/src/components/pagination/usePaginationRange.tspackages/base-ui/src/components/popover/Popover.stories.tsxpackages/base-ui/src/components/split-button/SplitButton.module.csspackages/base-ui/src/components/split-button/SplitButton.stories.tsxpackages/base-ui/src/components/split-button/SplitButton.test.tsxpackages/base-ui/src/components/split-button/SplitButton.tsxpackages/base-ui/src/components/split-button/index.tspackages/base-ui/src/components/stepper/Stepper.module.csspackages/base-ui/src/components/stepper/Stepper.stories.tsxpackages/base-ui/src/components/stepper/Stepper.test.tsxpackages/base-ui/src/components/stepper/Stepper.tsxpackages/base-ui/src/components/stepper/index.tspackages/base-ui/src/components/tag-input/TagInput.module.csspackages/base-ui/src/components/tag-input/TagInput.stories.tsxpackages/base-ui/src/components/tag-input/TagInput.test.tsxpackages/base-ui/src/components/tag-input/TagInput.tsxpackages/base-ui/src/components/tag-input/index.tspackages/base-ui/src/components/toolbar/Toolbar.module.csspackages/base-ui/src/components/toolbar/Toolbar.stories.tsxpackages/base-ui/src/components/toolbar/Toolbar.test.tsxpackages/base-ui/src/components/toolbar/Toolbar.tsxpackages/base-ui/src/components/toolbar/index.ts
Comment on lines
5
to
6
| export * from './breadcrumbs'; | ||
| export * from './avatar'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider maintaining alphabetical order for consistency.
The breadcrumbs export on line 5 is inserted before avatar (line 6), breaking alphabetical order. Similarly, the new exports at lines 71-75 are appended rather than integrated alphabetically.
While not a functional issue, consistent ordering improves discoverability in large barrel files.
♻️ Suggested ordering
export * from './accordion';
export * from './action-list';
export * from './alert-dialog';
+export * from './aspect-ratio';
export * from './autocomplete';
export * from './avatar';
export * from './avatar-group';
-export * from './breadcrumbs';
export * from './badge';
+export * from './banner';
+export * from './basic-list';
+export * from './box';
+export * from './breadcrumbs';
+export * from './button';
+export * from './button-group';
...Consider sorting all exports alphabetically for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/index.ts` around lines 5 - 6, The barrel file
exports are out of alphabetical order: 'breadcrumbs' appears before 'avatar' and
several later-added exports were appended instead of being merged; please
reorder all export statements alphabetically (e.g., place 'avatar' before
'breadcrumbs') and integrate the new exports into that sorted sequence so the
entire exports list is consistently alphabetical; update the export block
containing 'avatar', 'breadcrumbs', and the recently added exports to reflect
the new ordering.
Visual fixes, story improvements, and test corrections across all Tier 2 components based on Storybook testing and code review.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
packages/base-ui/src/components/pagination/Pagination.stories.tsx (1)
25-27:⚠️ Potential issue | 🟡 MinorSync
SimplePaginationwith Storybook controls too.Line 26 seeds local state once, so
FewPages,ManyPages, andWithoutFirstLaststill stop reflectingpagechanges after mount and can keep an out-of-range page whencountis reduced.🔧 Suggested fix
function SimplePagination(props: Omit<PaginationProps, 'onChange'>) { const [page, setPage] = useState(props.page); + + useEffect(() => { + setPage(Math.max(1, Math.min(props.page, props.count))); + }, [props.page, props.count]); + return <Pagination {...props} page={page} onChange={setPage} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/pagination/Pagination.stories.tsx` around lines 25 - 27, SimplePagination seeds local state from props.page once which causes it to go out-of-sync with Storybook controls and stay on an out-of-range page if props.count decreases; update the component to sync local page state when props.page or props.count change (use useEffect) and clamp the value into the valid range (e.g., between 1 and props.count when count is provided) before calling setPage so Pagination (and controls like FewPages/ManyPages/WithoutFirstLast) always reflect current props; refer to SimplePagination, PaginationProps, page and onChange to locate where to add the effect and clamping logic.packages/base-ui/src/components/stepper/Stepper.tsx (1)
72-77:⚠️ Potential issue | 🟠 MajorSupport wrapped
Stepper.Stepchildren.This only indexes direct
Stepper.Stepnodes. If a consumer groups steps in aReact.Fragmentor wrapper component, the nested steps keep_index=0/_isFirst=false, so they render with the wrong number/status and can all appear active whenactiveStep === 0. Recurse into wrapper children, or derive indices from actual step nodes only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/stepper/Stepper.tsx` around lines 72 - 77, The mapping only assigns _index/_isFirst to direct StepperStep children, so nested Stepper.Step inside wrappers or React.Fragment get wrong indices; modify the steps computation (the Children.map block) to traverse children recursively or compute indices from the sequence of actual StepperStep nodes only: walk the children tree, maintain a counter for StepperStep occurrences, and when you encounter an element whose type is StepperStep clone it with {_index: counter, _isFirst: counter === 0} and increment the counter; for non-StepperStep elements that have children (including Fragments or wrapper components) clone them with their processed child subtree so nested StepperStep nodes receive correct indices. Ensure you still use isValidElement and cloneElement for all cloned nodes and preserve other props.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.module.css`:
- Around line 53-60: Remove the redundant color declaration in the active-state
rule: delete the color assignment from the .Item[data-ov-active='true'] .Text
selector since .Text already sets color: var(--ov-color-fg-default); (leave the
selector only if additional active-state styles will be added later); target the
.Text and .Item[data-ov-active='true'] .Text selectors in Breadcrumbs.module.css
to locate and remove the duplicate property.
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.stories.tsx`:
- Around line 9-14: The story sets args.maxItems to 4 which differs from the
component default in BreadcrumbsRoot (default 8); update the story to either
align the arg with the component by changing args.maxItems to 8 or add a clear
inline comment above the args block explaining the intentional mismatch for
playground/collapse demonstration and ensure any story-level docs/autodocs note
the component default (reference BreadcrumbsRoot and the args object).
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsx`:
- Around line 116-137: Remove the extra blank line and fix indentation inside
the else block so it matches the surrounding style: collapse the stray blank
line after the `} else {` and align the declarations for `before`, `after`,
`ellipsis` and the `visibleItems = [...before, ellipsis, ...after];` statement
with the rest of the function; ensure the `button` onClick that calls
`setExpanded(true)` and usage of `LuEllipsis`, `styles.Item`, and
`styles.Ellipsis` remain unchanged but indented consistently.
In `@packages/base-ui/src/components/form-field/FormField.stories.tsx`:
- Around line 85-94: The RequiredField story (and the other required example at
lines ~101-109) only sets FormField.required which renders the visual asterisk
but does not make the actual input required; update the story render functions
so the underlying input elements (e.g., the <input id="api-key" ... /> used in
the RequiredField Story) include the required attribute (or required prop) so
the DOM input is truly required and reflects accessibility/validation behavior
for FormField.
In `@packages/base-ui/src/components/form-field/FormField.tsx`:
- Around line 75-79: The legend is currently nested inside the SectionHeader
wrapper which breaks native fieldset semantics; move the <legend> element so it
is the first direct child of the <fieldset> in the FormField component: place
the legend (styles.SectionTitle) immediately inside the fieldset before the
div.SectionHeader, then render the description and any other header content
inside the div (styles.SectionDescription) or alongside the legend as needed,
preserving existing classNames (SectionTitle, SectionHeader, SectionDescription)
and props on the fieldset.
In `@packages/base-ui/src/components/split-button/SplitButton.stories.tsx`:
- Around line 29-33: Replace the blocking alert() calls used as onClick handlers
in the story for SplitButton.Action and Menu.Item with non-blocking stubs or
Storybook action loggers; locate the handlers on SplitButton.Action and
Menu.Item in SplitButton.stories.tsx (including the other occurrence mentioned)
and change each onClick={() => alert('...')} to a safe alternative such as a
no-op stub or action('Save clicked') so the preview iframe isn't frozen and
Storybook interactions remain stable.
In `@packages/base-ui/src/components/split-button/SplitButton.tsx`:
- Around line 17-25: SplitButton currently inherits color from
StyledComponentProps (ComponentColor) which allows values like "info" that have
no corresponding CSS in SplitButton.module.css; narrow the prop by defining a
component-specific color union (e.g., type SplitButtonColor = 'neutral' |
'brand' | 'success' | 'warning' | 'danger') and replace the color type on
SplitButtonProps and SplitButtonActionProps (where StyledComponentProps or its
color is used) with this new SplitButtonColor, or alternatively add an "info"
style block to SplitButton.module.css; update references to StyledComponentProps
or props destructuring in the SplitButton component to use the narrowed color
type so invalid colors no longer type-check.
In `@packages/base-ui/src/components/tag-input/TagInput.tsx`:
- Around line 17-36: The TagInput currently places HTML attributes (id, name,
aria-*, autoComplete, etc.) only on the wrapper div; add an explicit prop on
TagInputProps (e.g., inputProps?: React.InputHTMLAttributes<HTMLInputElement>)
and update the TagInput component to spread those attributes onto the inner
<input> (and avoid duplicating them on the wrapper), ensuring label consumers
(FormField/htmlFor) can target the real control; keep existing
Omit<HTMLAttributes<HTMLDivElement>, 'onChange'> for wrapper props and forward
all other wrapper props to the div while forwarding inputProps to the input
element.
- Around line 58-73: The addTags function currently may accept zero tags but
callers still call setInputValue(''), wiping the user's input; change addTags
(in TagInput.tsx) to return a boolean or number indicating how many tags were
actually added, and update all callers (e.g., the Enter/delimiter and paste/blur
handlers that currently invoke addTags) to only call setInputValue('') when
addTags signals that at least one tag was added; ensure you reference the
addTags function and any handlers (onKeyDown/onPaste/onBlur or similar) that
clear the input so they conditionally clear only on successful additions and
leave the text for the user to fix otherwise.
In `@packages/base-ui/src/components/toolbar/Toolbar.module.css`:
- Around line 35-42: .The separator's percentage height (height: 60%) can't
resolve because its containing block (.Group) has no definite height, causing
the divider to collapse; replace the percentage with a definite computed value
derived from the toolbar height custom property (use the inherited
--_ov-toolbar-height from .Root) so .Separator gets a concrete height (e.g.,
compute 60% of --_ov-toolbar-height via calc) and keep the other styles (.Group,
.Root, --_ov-toolbar-height) intact.
In `@packages/base-ui/src/components/toolbar/Toolbar.tsx`:
- Around line 6-9: The toolbar currently applies role="toolbar" by default
without ARIA requirements or keyboard support; update ToolbarProps and the
component so either (A) role="toolbar" is opt-in (remove default role and only
pass role when prop provided) or (B) enforce an accessible name and implement
roving tabindex + arrow-key navigation when role="toolbar" is used: add required
ariaLabel/ariaLabelledby types to ToolbarProps (or require them conditionally
when role === "toolbar"), add internal focus state and a focusableIndex used to
set tabIndex=0 for the active child and -1 for others, and add a handleKeyDown
in Toolbar to move focus on ArrowLeft/ArrowRight (and Home/End; support
orientation prop if needed) so consumers of the Toolbar component (symbols:
ToolbarProps, Toolbar component, handleKeyDown/focusableIndex) either opt into
toolbar semantics or get full keyboard/accessible name enforcement.
---
Duplicate comments:
In `@packages/base-ui/src/components/pagination/Pagination.stories.tsx`:
- Around line 25-27: SimplePagination seeds local state from props.page once
which causes it to go out-of-sync with Storybook controls and stay on an
out-of-range page if props.count decreases; update the component to sync local
page state when props.page or props.count change (use useEffect) and clamp the
value into the valid range (e.g., between 1 and props.count when count is
provided) before calling setPage so Pagination (and controls like
FewPages/ManyPages/WithoutFirstLast) always reflect current props; refer to
SimplePagination, PaginationProps, page and onChange to locate where to add the
effect and clamping logic.
In `@packages/base-ui/src/components/stepper/Stepper.tsx`:
- Around line 72-77: The mapping only assigns _index/_isFirst to direct
StepperStep children, so nested Stepper.Step inside wrappers or React.Fragment
get wrong indices; modify the steps computation (the Children.map block) to
traverse children recursively or compute indices from the sequence of actual
StepperStep nodes only: walk the children tree, maintain a counter for
StepperStep occurrences, and when you encounter an element whose type is
StepperStep clone it with {_index: counter, _isFirst: counter === 0} and
increment the counter; for non-StepperStep elements that have children
(including Fragments or wrapper components) clone them with their processed
child subtree so nested StepperStep nodes receive correct indices. Ensure you
still use isValidElement and cloneElement for all cloned nodes and preserve
other props.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: faf1a38b-5743-4c8d-983a-ec0d59856ae1
📒 Files selected for processing (29)
packages/base-ui/src/components/breadcrumbs/Breadcrumbs.module.csspackages/base-ui/src/components/breadcrumbs/Breadcrumbs.stories.tsxpackages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsxpackages/base-ui/src/components/confirm-button/ConfirmButton.stories.tsxpackages/base-ui/src/components/confirm-button/ConfirmButton.tsxpackages/base-ui/src/components/form-field/FormField.module.csspackages/base-ui/src/components/form-field/FormField.stories.tsxpackages/base-ui/src/components/form-field/FormField.test.tsxpackages/base-ui/src/components/form-field/FormField.tsxpackages/base-ui/src/components/index.tspackages/base-ui/src/components/pagination/Pagination.module.csspackages/base-ui/src/components/pagination/Pagination.stories.tsxpackages/base-ui/src/components/pagination/Pagination.test.tsxpackages/base-ui/src/components/pagination/usePaginationRange.tspackages/base-ui/src/components/split-button/SplitButton.module.csspackages/base-ui/src/components/split-button/SplitButton.stories.tsxpackages/base-ui/src/components/split-button/SplitButton.test.tsxpackages/base-ui/src/components/split-button/SplitButton.tsxpackages/base-ui/src/components/stepper/Stepper.module.csspackages/base-ui/src/components/stepper/Stepper.stories.tsxpackages/base-ui/src/components/stepper/Stepper.test.tsxpackages/base-ui/src/components/stepper/Stepper.tsxpackages/base-ui/src/components/tag-input/TagInput.module.csspackages/base-ui/src/components/tag-input/TagInput.stories.tsxpackages/base-ui/src/components/tag-input/TagInput.tsxpackages/base-ui/src/components/toolbar/Toolbar.module.csspackages/base-ui/src/components/toolbar/Toolbar.stories.tsxpackages/base-ui/src/components/toolbar/Toolbar.test.tsxpackages/base-ui/src/components/toolbar/Toolbar.tsx
- Remove redundant active-state color in Breadcrumbs CSS - Fix indentation and add maxItems comment in Breadcrumbs - Move legend to direct child of fieldset in FormSection - Add required attr to inputs in FormField stories - Replace alert() with noop stubs in SplitButton stories - Narrow SplitButton color type to exclude unsupported 'info' - Add inputProps forwarding and conditional input clearing in TagInput - Fix toolbar separator height with calc-based value - Make toolbar role="toolbar" opt-in - Sync SimplePagination state with Storybook controls - Support nested StepperStep children via recursive traversal
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
packages/base-ui/src/components/tag-input/TagInput.tsx (1)
126-132:⚠️ Potential issue | 🟠 MajorDon't swallow pasted text when no tag was accepted.
handlePaste()blocks the browser paste beforemax, duplicate, andvalidatechecks run, so rejected pasted content disappears instead of staying editable. This is the same input-loss issue that was already fixed for the Enter/delimiter path.🩹 Minimal fix
const handlePaste = useCallback( (event: ClipboardEvent<HTMLInputElement>) => { - event.preventDefault(); const pasted = event.clipboardData.getData('text/plain'); const parts = splitByDelimiter(pasted); - addTags(parts); + if (addTags(parts) > 0) { + event.preventDefault(); + } }, [addTags, splitByDelimiter], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/tag-input/TagInput.tsx` around lines 126 - 132, handlePaste currently always calls event.preventDefault(), causing the pasted text to disappear if none of the split tags are accepted; change it to only preventDefault and consume the paste when at least one tag will be added. Do this by splitting the pasted text with splitByDelimiter(pasted), then determine which parts would be accepted (use the same validation/duplicate/max logic or have addTags return the number/array of accepted tags) and if acceptedParts.length > 0 call event.preventDefault() and addTags(acceptedParts); otherwise do nothing so the browser leaves the pasted text in the input. Ensure you reference handlePaste, splitByDelimiter and addTags (or update addTags to return accepted items) when making the change.packages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsx (2)
93-96:⚠️ Potential issue | 🟡 MinorReset expansion from the actual collapse inputs.
Lines 93-96 only watch
totalItemsandmaxItems, so a same-length trail replacement or a change toitemsBeforeCollapse/itemsAfterCollapseleaves the new trail stuck expanded. Reset from a trail identity signal plus all collapse props, not just the item count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsx` around lines 93 - 96, The effect that resets expansion currently only depends on totalItems and maxItems; change the dependency list for the useEffect that calls setExpanded(false) so it also includes the trail identity (e.g., the array/prop that represents the breadcrumb items or a trailId) plus the collapse-related props itemsBeforeCollapse and itemsAfterCollapse (and any other collapse settings), ensuring the effect watches the actual trail replacement and all collapse inputs rather than just item counts so new trails or changed collapse props always reset expansion.
34-36:⚠️ Potential issue | 🟠 MajorDisallow
activeon thehrefvariant.
BreadcrumbItemPropsstill letshrefandactivecoexist. That combination type-checks, but the runtime falls back to the non-link branch and can forward anchor-only props liketargetorrelonto a<span>. Make the link variantactive?: falseso the public API matches the rendered DOM.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsx` around lines 34 - 36, BreadcrumbItemProps currently allows href and active together which mismatches runtime rendering; update the link branch of the union so the anchor variant explicitly disables active (e.g., add active?: false to the second union member) so that (BreadcrumbItemBaseProps & Omit<AnchorHTMLAttributes<HTMLAnchorElement>, 'color'> & { href: string; active?: false }) prevents consumers from passing active with href while keeping active allowed on the non-link variant; change only the type definition for BreadcrumbItemProps referencing BreadcrumbItemBaseProps and the anchor branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.stories.tsx`:
- Around line 33-39: The story uses absolute hrefs on Breadcrumbs.Item which
cause the Storybook preview iframe to navigate away; update each
Breadcrumbs.Item (e.g., the instances in Breadcrumbs.stories using
Breadcrumbs.Item) to use non-navigating URLs or handlers by replacing absolute
URLs with hash URLs (e.g., href="#") or by adding onClick={(e) =>
e.preventDefault()} alongside a placeholder href, so clicks exercise the
component without leaving the story preview; apply this change to all listed
instances (lines referenced: the items around the example and the other groups
called out).
In `@packages/base-ui/src/components/form-field/FormField.stories.tsx`:
- Around line 98-113: The story FormSectionGrouping defines unused args ({
label, htmlFor }) but its render uses hardcoded values; either remove the args
property from FormSectionGrouping to avoid confusing controls, or update the
render to consume the story args by spreading them into the appropriate
FormField (e.g., pass the story args into the Host FormField and/or other
FormField instances as intended) and/or add Storybook parameters to hide
controls; update FormSectionGrouping accordingly so args are either removed or
actually used via spread to keep Storybook controls consistent.
In `@packages/base-ui/src/components/pagination/Pagination.stories.tsx`:
- Around line 5-35: The Storybook controls don't get updated when pagination
changes because ControlledPagination and SimplePagination only update local
state; import useArgs from `@storybook/preview-api`, call const [, updateArgs] =
useArgs() inside each component, and in the onChange handlers call updateArgs({
page: nextPage }) (in ControlledPagination replace props.onChange(p) with
updateArgs and local setPage, in SimplePagination call updateArgs when setPage
is invoked) so Storybook args stay in sync with the local page state and props.
In `@packages/base-ui/src/components/split-button/SplitButton.tsx`:
- Around line 101-105: The hard-coded aria-label on Menu.Trigger should be
configurable: add a prop (e.g., menuAriaLabel or triggerAriaLabel) to the
SplitButton.Menu component's props and types (update the SplitButton.Menu
props/interface) with a default value of "More actions", then pass that prop
into Menu.Trigger's aria-label instead of the literal string; update any
consumers or prop forwarding so callers can override the label for localization
or context-specific text while preserving the current string as the default.
In `@packages/base-ui/src/components/tag-input/TagInput.tsx`:
- Around line 152-161: The TagInput wrapper currently spreads {...props} and
later overwrites inputProps, allowing consumer handlers/refs/classNames to be
replaced; update TagInput to compose forwarded event handlers and refs instead
of overwriting them: for the container use handleContainerClick but call
props.onClick (if present) as well (or vice‑versa) so both run; for the internal
<input> merge inputProps with internal handlers (onFocus, onBlur, onKeyDown,
onPaste, className) by composing functions and concatenating className, and use
a ref merger (e.g., mergeRefs) to combine inputRef with inputProps.ref; ensure
you reference TagInput, handleContainerClick, inputProps, inputRef and the
internal handler names when making the changes so consumer-supplied handlers and
refs are preserved.
In `@packages/base-ui/src/components/toolbar/Toolbar.test.tsx`:
- Around line 18-22: Add a regression test to ensure the Toolbar component does
not default to role="toolbar" and only surfaces that role when explicitly
passed; in Toolbar.test.tsx (using renderWithTheme, screen, and the Toolbar
component) add a test that renders <Toolbar> without a role and asserts
screen.queryByRole('toolbar') is null, while keeping the existing test that
checks role="toolbar" when the prop is provided.
- Around line 96-107: The test for Toolbar.Group should assert that no actual
separator DOM node is rendered by checking for the separator role rather than
only the absence of the data-ov-separator attribute; update the test that uses
Toolbar and Toolbar.Group to query within the rendered group (e.g., using
within(group).queryByRole('separator') or screen.queryByRole('separator', {
hidden: true })) and assert the result is null (or
toBeNull()/not.toBeInTheDocument()), so it fails if a real separator element is
present even without the data-ov-separator attribute.
---
Duplicate comments:
In `@packages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsx`:
- Around line 93-96: The effect that resets expansion currently only depends on
totalItems and maxItems; change the dependency list for the useEffect that calls
setExpanded(false) so it also includes the trail identity (e.g., the array/prop
that represents the breadcrumb items or a trailId) plus the collapse-related
props itemsBeforeCollapse and itemsAfterCollapse (and any other collapse
settings), ensuring the effect watches the actual trail replacement and all
collapse inputs rather than just item counts so new trails or changed collapse
props always reset expansion.
- Around line 34-36: BreadcrumbItemProps currently allows href and active
together which mismatches runtime rendering; update the link branch of the union
so the anchor variant explicitly disables active (e.g., add active?: false to
the second union member) so that (BreadcrumbItemBaseProps &
Omit<AnchorHTMLAttributes<HTMLAnchorElement>, 'color'> & { href: string;
active?: false }) prevents consumers from passing active with href while keeping
active allowed on the non-link variant; change only the type definition for
BreadcrumbItemProps referencing BreadcrumbItemBaseProps and the anchor branch.
In `@packages/base-ui/src/components/tag-input/TagInput.tsx`:
- Around line 126-132: handlePaste currently always calls
event.preventDefault(), causing the pasted text to disappear if none of the
split tags are accepted; change it to only preventDefault and consume the paste
when at least one tag will be added. Do this by splitting the pasted text with
splitByDelimiter(pasted), then determine which parts would be accepted (use the
same validation/duplicate/max logic or have addTags return the number/array of
accepted tags) and if acceptedParts.length > 0 call event.preventDefault() and
addTags(acceptedParts); otherwise do nothing so the browser leaves the pasted
text in the input. Ensure you reference handlePaste, splitByDelimiter and
addTags (or update addTags to return accepted items) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 096bdba4-6f50-4cb7-8833-4f2d9fee0895
📒 Files selected for processing (14)
packages/base-ui/src/components/breadcrumbs/Breadcrumbs.module.csspackages/base-ui/src/components/breadcrumbs/Breadcrumbs.stories.tsxpackages/base-ui/src/components/breadcrumbs/Breadcrumbs.tsxpackages/base-ui/src/components/form-field/FormField.stories.tsxpackages/base-ui/src/components/form-field/FormField.tsxpackages/base-ui/src/components/pagination/Pagination.stories.tsxpackages/base-ui/src/components/split-button/SplitButton.stories.tsxpackages/base-ui/src/components/split-button/SplitButton.tsxpackages/base-ui/src/components/stepper/Stepper.tsxpackages/base-ui/src/components/tag-input/TagInput.tsxpackages/base-ui/src/components/toolbar/Toolbar.module.csspackages/base-ui/src/components/toolbar/Toolbar.stories.tsxpackages/base-ui/src/components/toolbar/Toolbar.test.tsxpackages/base-ui/src/components/toolbar/Toolbar.tsx
Comment on lines
+5
to
+35
| function ControlledPagination(props: PaginationProps) { | ||
| const [page, setPage] = useState(props.page); | ||
|
|
||
| // Keep local state in sync with Storybook controls | ||
| useEffect(() => { | ||
| setPage((prev) => Math.min(props.page, props.count) || prev); | ||
| }, [props.page, props.count]); | ||
|
|
||
| return ( | ||
| <Pagination | ||
| {...props} | ||
| page={page} | ||
| onChange={(p) => { | ||
| setPage(p); | ||
| props.onChange(p); | ||
| }} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| function SimplePagination(props: Omit<PaginationProps, 'onChange'>) { | ||
| const [page, setPage] = useState(props.page); | ||
|
|
||
| useEffect(() => { | ||
| setPage((prev) => { | ||
| const clamped = Math.max(1, Math.min(prev, props.count)); | ||
| return props.page !== prev ? Math.max(1, Math.min(props.page, props.count)) : clamped; | ||
| }); | ||
| }, [props.page, props.count]); | ||
|
|
||
| return <Pagination {...props} page={page} onChange={setPage} />; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the Storybook React version used in this repository, what is the recommended way to keep a controlled story's args synchronized with user interactions? Please confirm whether useArgsfrom@storybook/preview-apishould be used to updatepageinsideonChange.
💡 Result:
To keep a controlled story’s args synchronized with user interactions (and therefore keep the Controls panel in sync), Storybook recommends updating the story’s args from inside the story using the useArgs hook, then calling the returned updateArgs function in your event handler. (storybook.js.org)
So yes—use useArgs to update page inside onChange (i.e., updateArgs({ page: nextPage })), rather than storing page in local React state that would drift from args/Controls. (storybook.js.org)
Example pattern:
import { useArgs } from 'storybook/preview-api'; // (some repos use `@storybook/preview-api`)
render: function Render(args) {
const [{ page }, updateArgs] = useArgs();
return (
<YourComponent
{...args}
page={page}
onChange={(nextPage) => updateArgs({ page: nextPage })}
/>
);
}
``` ([storybook.js.org](https://storybook.js.org/docs/writing-stories/args))
Citations:
- 1: https://storybook.js.org/docs/writing-stories/args
- 2: https://storybook.js.org/docs/writing-stories/args
- 3: https://storybook.js.org/docs/writing-stories/args
---
</details>
**Synchronize `page` changes back to story args using `useArgs`.**
When users interact with the pagination, `ControlledPagination` and `SimplePagination` update only their local state—the Storybook `page` control does not reflect the new value. This causes the Controls panel to diverge from the rendered output. Import `useArgs` from `@storybook/preview-api` and call `updateArgs({ page: nextPage })` inside the `onChange` handler instead of just updating local state.
<details>
<summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @packages/base-ui/src/components/pagination/Pagination.stories.tsx around
lines 5 - 35, The Storybook controls don't get updated when pagination changes
because ControlledPagination and SimplePagination only update local state;
import useArgs from @storybook/preview-api, call const [, updateArgs] =
useArgs() inside each component, and in the onChange handlers call updateArgs({
page: nextPage }) (in ControlledPagination replace props.onChange(p) with
updateArgs and local setPage, in SimplePagination call updateArgs when setPage
is invoked) so Storybook args stay in sync with the local page state and props.
</details>
<!-- fingerprinting:phantom:poseidon:grasshopper -->
<!-- This is an auto-generated comment by CodeRabbit -->
- Replace absolute hrefs with hash URLs in Breadcrumbs stories - Disallow active+href combination in BreadcrumbItemProps type - Add itemsBeforeCollapse/itemsAfterCollapse to useEffect deps - Remove unused args from FormSection/AllSizes stories - Add configurable menuAriaLabel prop to SplitButton.Menu - Compose TagInput container/input handlers with consumer callbacks - Only preventDefault on paste when tags are accepted - Add regression test for Toolbar not defaulting to role="toolbar" - Assert no separator DOM node when separator prop is absent
--ov-color-fg-danger was never defined in the theme, causing error text and required asterisks to inherit the default text color (white in dark mode). Replace with the actual --ov-color-danger token.
This was referenced Mar 9, 2026
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
react-icons/luexclusively — no raw Unicode, HTML entities, emojis, or inline SVGs.tsx,.module.css,.test.tsx,.stories.tsx, andindex.tsCOMPONENT_STATUS.mdTest plan
pnpm --filter @omniview/base-ui test)Summary by CodeRabbit