feat(ui): fix breadcrumbs, improve ActionList, add MoreActionMenu #517
Conversation
WalkthroughAdds a MoreActionsMenu component and unifies ActionList to accept a single Changes
Sequence Diagram(s)sequenceDiagram
participant Block as Block (OrderDetails / TicketList)
participant ActionList as ActionList
participant MoreMenu as MoreActionsMenu
participant Dropdown as DropdownMenu
participant User as User
Block->>ActionList: pass unified actions array
ActionList->>ActionList: render actions[0] and actions[1]
ActionList->>MoreMenu: pass overflow actions (actions.slice(2))
User->>ActionList: click a visible action
User->>MoreMenu: open menu
MoreMenu->>Dropdown: render dropdown items
User->>Dropdown: select dropdown item
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ui/src/components/ActionList/ActionList.stories.tsx (2)
22-41: Duplicate React keys in theactionsarray.Several actions share the same
keyvalues, which will cause React warnings and potential rendering issues:
- Lines 23 & 29: both use
key="primary"- Lines 26 & 32: both use
key="secondary"Keys must be unique within the same array.
🔧 Suggested fix
actions: [ <Button key="primary" variant="primary"> Primary Action </Button>, <Button key="secondary" variant="secondary"> Secondary Action </Button>, - <Link key="primary" variant="primary"> + <Link key="link-primary" variant="primary"> Primary Action </Link>, - <Link key="secondary" variant="secondary"> + <Link key="link-secondary" variant="secondary"> Secondary Action </Link>, <Link key="tertiary1" variant="outline"> Tertiary Action 1 </Link>, <Link key="tertiary2" variant="ghost"> Tertiary Action 2 </Link>, ],
74-93: Same duplicate key issue inWithDifferentVariantstory.Lines 75 & 81 both use
key="primary", and lines 78 & 84 both usekey="secondary". Apply the same fix as suggested for the Default story.
🤖 Fix all issues with AI agents
In @packages/blocks/order-details/src/frontend/OrderDetails.client.tsx:
- Around line 214-232: The actions mapping creates a sparse array and risks key
collisions by using action.label as the key; first filter the actions array to
only include actionable items (e.g., actions.filter(a => a.label)) before
mapping to avoid falsy holes, and use a stable unique key instead of
action.label (prefer an existing unique property like action.id, or fall back to
a composed key such as `${action.label}-${index}`) when rendering
TooltipHover/Button/DynamicIcon so keys are deterministic and collisions are
prevented.
In @packages/blocks/ticket-list/src/frontend/TicketList.client.tsx:
- Around line 178-193: The actions mapping can emit false for entries with a
falsy action.label, leaking falsy values into the array passed to
ActionList/MoreActionsMenu; fix this by filtering out actions with no label
before mapping (or use flatMap to return an empty array for falsy labels) so the
prop receives only React elements—apply this change where actions is transformed
(the map that returns <Button ...> elements referencing Button, LinkComponent
and DynamicIcon) to ensure only valid elements are produced.
In @packages/ui/src/components/Carousel/Carousel.tsx:
- Around line 28-31: The carousel's local state (index, isEnd) isn't initialized
from the startingSlideIndex prop or Swiper's actual initial state, causing
incorrect disabled button states on mount; update the Swiper component to use
its lifecycle events (onInit and/or onSlideChange) to setIndex(swiper.realIndex)
and setIsEnd(swiper.isEnd) and remove any manual index/isEnd adjustments in the
next/prev click handlers, and on mount initialize index using startingSlideIndex
(or defer to onInit to read swiper.realIndex) so single-slide/carousels starting
at a non-zero slide reflect correct button enabled/disabled status; reference
swiperRef, startingSlideIndex, index/setIndex, isEnd/setIsEnd, and the Swiper
onInit/onSlideChange handlers when making the change.
- Line 68: The className on the carousel button uses an invalid Tailwind utility
`z-1`; update the class string in the Carousel component (the JSX element with
className="rounded-full absolute z-1 top-2/4 !-translate-y-8 -left-5") to use a
valid z-index utility such as `z-10` (or another valid class like `z-0`/`z-20`)
so the button layers correctly.
🧹 Nitpick comments (9)
packages/ui/src/components/Breadcrumbs/Breadcrumbs.tsx (1)
46-48: Consider making the React key more robust.Pre-existing: The key uses
item.slugdirectly, but for the last breadcrumb (current page),slugmay be undefined—as evidenced by the conditional check on line 26. This could cause React key warnings.For consistency with line 24's pattern:
Suggested improvement
- <BreadcrumbItem key={item.slug}> + <BreadcrumbItem key={`${item.slug}-${index}`}>packages/ui/src/components/Chart/ChartTooltip/ChartTooltip.tsx (1)
19-40: Inconsistent optional chaining usage.
item?.coloranditem?.nameuse optional chaining, butitem.valueanditem.uniton lines 31-32 and 36 do not. Consider applying optional chaining consistently across all property accesses for uniformity.Suggested consistency fix
<svg width="12" height="12" xmlns="http://www.w3.org/2000/svg"> - <rect x="0" y="0" width="12" height="12" fill={item?.color} rx="4" ry="4" /> + <rect x="0" y="0" width="12" height="12" fill={item.color} rx="4" ry="4" /> </svg> - <Typography variant="small">{`${item?.name} :`}</Typography> + <Typography variant="small">{`${item.name} :`}</Typography>Alternatively, if optional chaining is intentional for safety, apply it to all accesses including
item?.valueanditem?.unit.packages/ui/src/components/ActionList/ActionList.types.ts (1)
5-10: Consider makingshowMoreLabeloptional sinceactionsis optional.If
actionsisundefinedor an empty array, the "show more" trigger would never render, makingshowMoreLabelunnecessary in those cases. For API consistency, consider makingshowMoreLabeloptional as well, or provide a sensible default.💡 Suggested change
export type ActionListProps = { actions?: React.ReactNode[]; - showMoreLabel: string; + showMoreLabel?: string; className?: string; triggerVariant?: VariantProps<typeof baseVariant>['variant']; };packages/blocks/order-details/src/frontend/OrderDetails.client.tsx (1)
169-189: Placeholder actions noted - ensure these are tracked for implementation.All actions currently display a "Coming Soon" tooltip rather than performing actual operations. This is fine for a preview/placeholder, but ensure there's tracking (e.g., TODO comment or issue) to implement the actual handlers for
payOnline,reorder, andtrackOrderfunctionality.Would you like me to open an issue to track the implementation of these action handlers?
packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.tsx (1)
26-41: Consider using a stable key instead of array index.Using
index.toString()as a key can cause issues with React reconciliation if actions are dynamically reordered or removed. Ifactionscontains React elements with identifiable props (like anidor uniquelabel), consider extracting that for the key. However, if the actions array is always static per render, the current approach is acceptable.packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (2)
91-114: Hardcoded array indices are fragile and may cause undefined access.Accessing
data.forms?.[0],[1],[2]assumes exactly 3 form items exist. If fewer items are present, the corresponding action objects will have undefined values forlabel,icon, and only the fallback''forurl. This creates actions with no visible content.Consider iterating over
data.formsdynamically or adding guards:♻️ Suggested approach using dynamic mapping
const variantMap: Record<number, Action['variant']> = { }; const classNameMap: Record<number, string> = { }; const actions: Action[] = (data.forms ?? []).slice(0, 3).map((form, index) => ({ label: form?.label, icon: form?.icon, url: form?.url || '', variant: variantMap[index] ?? 'default', className: classNameMap[index] ?? '', }));
151-165: Non-null assertion ondata.table.actions!.labelis safe here but consider defensive coding.The non-null assertion on line 159 is guarded by the ternary condition on line 151, so it's technically safe. However, if the conditional logic changes, this could become a source of bugs.
♻️ Store reference to avoid non-null assertion
-const tableActions = data.table.actions - ? { - ...data.table.actions, - render: (ticket: Model.Ticket) => { - return ( - <Button asChild variant="link"> - <LinkComponent href={ticket.detailsUrl} className="flex items-center justify-end gap-2"> - <ArrowRight className="h-4 w-4" /> - {data.table.actions!.label} - </LinkComponent> - </Button> - ); - }, - } - : undefined; +const tableActionsConfig = data.table.actions; +const tableActions = tableActionsConfig + ? { + ...tableActionsConfig, + render: (ticket: Model.Ticket) => { + return ( + <Button asChild variant="link"> + <LinkComponent href={ticket.detailsUrl} className="flex items-center justify-end gap-2"> + <ArrowRight className="h-4 w-4" /> + {tableActionsConfig.label} + </LinkComponent> + </Button> + ); + }, + } + : undefined;packages/ui/src/components/Carousel/Carousel.tsx (2)
70-76: Redundant optional chaining after null check.After the
if (swiperRef.current)guard, the optional chaining operators (?.) are unnecessary.Suggested cleanup
onClick={() => { if (swiperRef.current) { - swiperRef.current?.slidePrev(); - - setIndex(swiperRef.current?.realIndex); - setIsEnd(swiperRef.current?.isEnd); + swiperRef.current.slidePrev(); + setIndex(swiperRef.current.realIndex); + setIsEnd(swiperRef.current.isEnd); } }}
87-93: Same redundant optional chaining as the left button.Suggested cleanup
onClick={() => { if (swiperRef.current) { - swiperRef.current?.slideNext(); - - setIndex(swiperRef.current?.realIndex); - setIsEnd(swiperRef.current?.isEnd); + swiperRef.current.slideNext(); + setIndex(swiperRef.current.realIndex); + setIsEnd(swiperRef.current.isEnd); } }}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.changeset/quiet-ties-sin.mdpackages/blocks/order-details/src/frontend/OrderDetails.client.tsxpackages/blocks/order-details/src/frontend/OrderDetails.types.tspackages/blocks/ticket-list/src/frontend/TicketList.client.tsxpackages/blocks/ticket-list/src/frontend/TicketList.types.tspackages/ui/src/components/ActionList/ActionList.stories.tsxpackages/ui/src/components/ActionList/ActionList.tsxpackages/ui/src/components/ActionList/ActionList.types.tspackages/ui/src/components/Breadcrumbs/Breadcrumbs.tsxpackages/ui/src/components/Cards/InformativeCard/InformativeCard.tsxpackages/ui/src/components/Carousel/Carousel.tsxpackages/ui/src/components/Chart/ChartTooltip/ChartTooltip.tsxpackages/ui/src/components/Chart/ChartTooltip/ChartTooltip.types.tspackages/ui/src/components/MoreActionsMenu/MoreActionsMenu.stories.tsxpackages/ui/src/components/MoreActionsMenu/MoreActionsMenu.tsxpackages/ui/src/components/MoreActionsMenu/MoreActionsMenu.types.tspackages/ui/src/components/MoreActionsMenu/index.ts
💤 Files with no reviewable changes (1)
- packages/ui/src/components/Chart/ChartTooltip/ChartTooltip.types.ts
🧰 Additional context used
🧬 Code graph analysis (8)
packages/blocks/ticket-list/src/frontend/TicketList.types.ts (2)
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
Action(23-28)packages/ui/src/lib/utils.ts (1)
baseVariant(9-33)
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
packages/ui/src/lib/utils.ts (1)
baseVariant(9-33)
packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.types.ts (2)
packages/ui/src/components/MoreActionsMenu/index.ts (1)
MoreActionsMenuProps(2-2)packages/ui/src/lib/utils.ts (1)
baseVariant(9-33)
packages/ui/src/components/Carousel/Carousel.tsx (2)
packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/elements/button.tsx (1)
Button(19-19)
packages/ui/src/components/Chart/ChartTooltip/ChartTooltip.tsx (2)
packages/ui/src/elements/typography.tsx (1)
Typography(55-55)packages/framework/src/utils/models/price.ts (1)
Price(3-7)
packages/ui/src/components/ActionList/ActionList.tsx (3)
packages/ui/src/components/ActionList/ActionList.types.ts (1)
ActionListProps(5-10)packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.tsx (1)
MoreActionsMenu(15-42)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (3)
packages/blocks/ticket-list/src/frontend/TicketList.types.ts (1)
Action(24-30)packages/ui/src/elements/button.tsx (1)
Button(19-19)packages/ui/src/components/DynamicIcon/DynamicIcon.tsx (1)
DynamicIcon(8-42)
packages/ui/src/components/ActionList/ActionList.stories.tsx (1)
packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.stories.tsx (2)
TwoActions(45-58)WithDifferentVariant(60-80)
🔇 Additional comments (13)
packages/ui/src/components/Cards/InformativeCard/InformativeCard.tsx (1)
65-65: LGTM!The border token update from
border-bordertoborder-card-borderaligns with the PR objective of improving visual consistency. The hover behavior and layout classes are correctly preserved.packages/ui/src/components/Breadcrumbs/Breadcrumbs.tsx (1)
17-17: LGTM! Good UX improvement.Hiding breadcrumbs when only a single item is present is appropriate since a single breadcrumb provides no navigational context. The guard logic correctly handles both empty/undefined arrays and the single-item case.
.changeset/quiet-ties-sin.md (1)
1-9: Verify if the ActionList API refactor constitutes a breaking change.The ActionList API refactor replaces
visibleActionsanddropdownActionsprops with a unifiedactionsprop. If@o2s/uiis consumed by external packages or applications outside this monorepo, this change would break existing usages and may warrant a major version bump instead of minor.If ActionList is only used internally within this monorepo (as appears to be the case with OrderDetails and TicketList), then a minor bump with coordinated migration is appropriate.
packages/ui/src/components/MoreActionsMenu/index.ts (1)
1-2: LGTM!Clean barrel export following standard patterns for the new MoreActionsMenu component.
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
23-28: No changes needed. The Action type difference between OrderDetails and TicketList is appropriate.The omission of the
urlfield in OrderDetails is intentional: OrderDetails actions use click handlers with a "Coming soon" tooltip (onClick={() => setIsOpen(true)}), while TicketList actions navigate to external forms viaLinkComponent href={action.url}. Each type correctly reflects its component's specific needs. Aligning them or makingurloptional would reduce type safety without benefit.Likely an incorrect or invalid review comment.
packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.stories.tsx (1)
1-80: Well-structured stories demonstrating the MoreActionsMenu API.The stories effectively cover the component's key props (
showMoreLabel,triggerVariant,triggerIcon,actions) with varied configurations. Good coverage of different action counts and trigger variants.Minor observation:
TwoActions(line 48) andWithDifferentVariant(line 63) both usetriggerVariant: 'destructive'. Consider using a different variant forTwoActionsto increase visual differentiation in the Storybook catalog.packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.types.ts (1)
1-13: Clean type definition with good controlled component support.The props interface is well-designed with optional
openandonOpenChangefor controlled usage. ThetriggerVariantproperly references the sharedbaseVariantfor type safety.packages/ui/src/components/ActionList/ActionList.tsx (1)
9-36: Clean refactor to unifiedactionsprop with good composition.The implementation effectively:
- Derives visible vs. dropdown actions from a single array (first 2 visible, rest in dropdown)
- Composes with
MoreActionsMenufor overflow handling- Guards against empty/undefined actions
The logic is sound and the component is more maintainable than the previous split-prop approach.
packages/blocks/ticket-list/src/frontend/TicketList.types.ts (1)
24-30: TheActiontype difference between blocks is intentional and appropriate.TicketList's
Actiontype requires aurlfield because actions are rendered as navigation links (<LinkComponent href={action.url}>). OrderDetails'Actiontype omitsurlbecause its actions are rendered as buttons with click handlers, not navigation links. This difference correctly reflects their different purposes and should not be consolidated. The types are appropriately typed for their respective use cases.Likely an incorrect or invalid review comment.
packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.tsx (1)
15-24: LGTM - Good defensive handling.The early return for empty actions array prevents rendering an empty dropdown menu, which is the correct UX behavior.
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
30-30: LGTM - Clean type import.The
Actiontype import aligns with the new unified action structure defined inTicketList.types.ts.packages/ui/src/components/Carousel/Carousel.tsx (2)
3-5: LGTM!Clean imports for the new navigation functionality - lucide-react icons, React hooks, and the Swiper type for proper typing.
42-61: LGTM!Good structure - wrapping Swiper in a relative container for button positioning, disabling built-in navigation, and capturing the instance via
onBeforeInit.
packages/blocks/order-details/src/frontend/OrderDetails.client.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
23-28: Consider requiring at leastlabeloriconfor visibility.Both
labelandiconare optional, which allows creating anActionwith no visible content. If the consuming component doesn't have fallback rendering, this could result in empty/invisible action buttons.If an action should always have at least one of these, consider a discriminated union or runtime validation in the component.
💡 Optional: Type-safe approach using union
export type Action = { variant: VariantProps<typeof baseVariant>['variant']; className?: string; } & ( | { label: string; icon?: string } | { label?: string; icon: string } );Please verify that the
ActionListcomponent handles the case where bothlabelandiconare undefined, or confirm this is intentional for specific use cases.packages/blocks/ticket-list/src/frontend/TicketList.types.ts (1)
24-30: Action type inconsistency with OrderDetails.The
Actiontype here includes a requiredurlfield, but theActiontype inpackages/blocks/order-details/src/frontend/OrderDetails.types.tsdoes not have this field:// OrderDetails.types.ts export type Action = { label?: string; icon?: string; variant: VariantProps<typeof baseVariant>['variant']; className?: string; };Given the PR objective mentions "unified ActionList API for consistency," consider aligning these types. Either extract a shared base type or ensure both blocks use the same
Actiondefinition to avoid confusion and maintenance issues.packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (2)
93-116: Hardcoded array indices are fragile and may cause issues.Accessing
data.forms?.[0],data.forms?.[1],data.forms?.[2]with hardcoded indices assumes a fixed structure and ordering. Ifdata.formshas fewer than 3 items, the corresponding action will havelabel: undefined,icon: undefined, andurl: '', which will still create action objects (just with empty/undefined values).Consider dynamically mapping over
data.formsinstead:♻️ Suggested refactor
- const actions: Action[] = [ - { - label: data.forms?.[0]?.label, - icon: data.forms?.[0]?.icon, - url: data.forms?.[0]?.url || '', - variant: 'default', - className: 'no-underline hover:no-underline', - }, - { - label: data.forms?.[1]?.label, - icon: data.forms?.[1]?.icon, - url: data.forms?.[1]?.url || '', - variant: 'secondary', - className: 'no-underline hover:no-underline flex-1', - }, - { - label: data.forms?.[2]?.label, - icon: data.forms?.[2]?.icon, - url: data.forms?.[2]?.url || '', - variant: 'ghost', - className: - 'flex items-center gap-2 !no-underline hover:!no-underline cursor-pointer h-8 w-full justify-start', - }, - ]; + const variantConfig: Array<{ variant: Action['variant']; className: string }> = [ + { variant: 'default', className: 'no-underline hover:no-underline' }, + { variant: 'secondary', className: 'no-underline hover:no-underline flex-1' }, + { variant: 'ghost', className: 'flex items-center gap-2 !no-underline hover:!no-underline cursor-pointer h-8 w-full justify-start' }, + ]; + + const actions: Action[] = (data.forms ?? []).map((form, index) => ({ + label: form.label, + icon: form.icon, + url: form.url || '', + variant: variantConfig[index]?.variant ?? 'default', + className: variantConfig[index]?.className ?? '', + }));
180-195: Consider filtering before mapping for cleaner code.The current pattern
action.label && (...)returnsfalsefor actions without labels, creating an array with potential falsy values. While React handles this correctly, it's cleaner to filter first:♻️ Suggested refactor
<ActionList - actions={actions.map( - (action) => - action.label && ( - <Button - asChild - variant={action.variant} - key={action.label} - className={action.className} - > - <LinkComponent href={action.url}> - {action.icon && <DynamicIcon name={action.icon} size={16} />} - {action.label} - </LinkComponent> - </Button> - ), - )} + actions={actions + .filter((action) => action.label) + .map((action) => ( + <Button + asChild + variant={action.variant} + key={action.label} + className={action.className} + > + <LinkComponent href={action.url}> + {action.icon && <DynamicIcon name={action.icon} size={16} />} + {action.label} + </LinkComponent> + </Button> + ))} showMoreLabel={data.labels.showMore} />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/blocks/order-details/src/frontend/OrderDetails.types.tspackages/blocks/ticket-list/src/frontend/TicketList.client.tsxpackages/blocks/ticket-list/src/frontend/TicketList.types.tspackages/ui/src/components/ActionList/ActionList.stories.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/ui/src/components/ActionList/ActionList.stories.tsx (1)
packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.stories.tsx (2)
TwoActions(45-58)WithDifferentVariant(60-80)
packages/blocks/ticket-list/src/frontend/TicketList.types.ts (2)
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
Action(23-28)packages/ui/src/lib/utils.ts (1)
baseVariant(9-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy-preview
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
1-5: LGTM!Imports are correctly added to support the new
Actiontype definition. The extraction of variant types viaVariantProps<typeof baseVariant>ensures type safety with the UI library's CVA configuration.packages/ui/src/components/ActionList/ActionList.stories.tsx (1)
17-82: LGTM! Stories correctly updated for unifiedactionsAPI.The refactoring from
visibleActions/dropdownActionsto a singleactionsprop is consistently applied across all story variants. Good coverage of edge cases (single action, two actions, multiple actions with different styling).packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
153-167: LGTM with a minor style note.The
tableActionsconfiguration correctly handles the conditional case. The non-null assertion on line 161 (data.table.actions!.label) is safe since it's inside the truthy branch of the ternary, but consider using a variable to avoid the assertion:const tableActions = data.table.actions ? (() => { const { label, ...rest } = data.table.actions; return { ...rest, render: (ticket: Model.Ticket) => ( // use label directly ), }; })() : undefined;This is optional - the current code works correctly.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ui/src/components/Carousel/Carousel.tsx:
- Line 28: The ref declaration uses useRef<SwiperType>(null) which yields a
readonly RefObject and prevents assignment to swiperRef.current; update the
generic to include null (useRef<SwiperType | null>(null)) so swiperRef.current
is mutable where it's assigned (see swiperRef and any assignment to
swiperRef.current in Carousel component).
🧹 Nitpick comments (4)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
93-109: Index-based variant assignment is fragile.The
variantConfigarray hardcodes exactly 3 configurations and relies on form index to determine styling. Ifdata.formshas more, fewer, or differently ordered items, the styling will be incorrect or fall back unpredictably.Consider deriving the variant from a property of the form itself (e.g.,
form.typeorform.variant) rather than positional index.♻️ Suggested approach
- const variantConfig: Array<{ variant: Action['variant']; className: string }> = [ - { variant: 'default', className: 'no-underline hover:no-underline' }, - { variant: 'secondary', className: 'no-underline hover:no-underline flex-1' }, - { - variant: 'ghost', - className: - 'flex items-center gap-2 !no-underline hover:!no-underline cursor-pointer h-8 w-full justify-start', - }, - ]; - - const actions: Action[] = (data.forms ?? []).map((form, index) => ({ - label: form.label, - icon: form.icon, - url: form.url || '', - variant: variantConfig[index]?.variant ?? 'default', - className: variantConfig[index]?.className ?? '', - })); + const actions: Action[] = (data.forms ?? []).map((form, index) => ({ + label: form.label, + icon: form.icon, + url: form.url || '', + // Consider adding variant/className to the form data model from the backend + variant: form.variant ?? (index === 0 ? 'default' : index === 1 ? 'secondary' : 'ghost'), + className: form.className ?? 'no-underline hover:no-underline', + }));packages/blocks/ticket-list/src/frontend/TicketList.types.ts (1)
24-28: Consider consolidating duplicate Action type definitions.This
Actiontype is nearly identical toOrderDetails.types.tsAction (lines 22-25 in that file). The only difference is theurlfield here. Both share the samevariant,className, andlabel/iconunion structure.A shared base type in a common location (e.g.,
@o2s/ui) could reduce duplication:// In shared types type BaseAction = { variant: VariantProps<typeof baseVariant>['variant']; className?: string; } & ({ label: string; icon?: string } | { label?: string; icon: string }); // In TicketList.types.ts export type Action = BaseAction & { url: string }; // In OrderDetails.types.ts export type Action = BaseAction;packages/ui/src/components/Carousel/Carousel.tsx (2)
71-96: Add aria-labels to navigation buttons for accessibility.The navigation buttons lack accessible labels, which would make them unclear to screen reader users.
Proposed fix
<Button variant="outline" size="icon" className="rounded-full absolute z-10 top-2/4 !-translate-y-8 -left-5" disabled={index === 0} onClick={() => { swiperRef.current?.slidePrev(); }} + aria-label="Previous slide" > <ArrowLeftIcon /> </Button> <Button variant="outline" size="icon" className="rounded-full absolute z-10 top-2/4 !-translate-y-8 -right-5" disabled={isEnd} onClick={() => { swiperRef.current?.slideNext(); }} + aria-label="Next slide" > <ArrowRightIcon /> </Button>
72-72: Simplify wrapper element.The flex classes on this div have no effect since the child buttons use absolute positioning. Consider simplifying to a fragment or removing unnecessary classes.
Proposed fix
{showNavigation && ( - <div className="flex items-center gap-8 lg:justify-start justify-center"> + <> <Button ... </Button> <Button ... </Button> - </div> + </> )}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/blocks/order-details/src/frontend/OrderDetails.client.tsxpackages/blocks/order-details/src/frontend/OrderDetails.types.tspackages/blocks/ticket-list/src/frontend/TicketList.client.tsxpackages/blocks/ticket-list/src/frontend/TicketList.types.tspackages/ui/src/components/ActionList/ActionList.tsxpackages/ui/src/components/Carousel/Carousel.tsxpackages/ui/src/components/MoreActionsMenu/MoreActionsMenu.stories.tsxpackages/ui/src/components/MoreActionsMenu/MoreActionsMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.tsx
- packages/blocks/order-details/src/frontend/OrderDetails.types.ts
- packages/ui/src/components/MoreActionsMenu/MoreActionsMenu.stories.tsx
- packages/ui/src/components/ActionList/ActionList.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (4)
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
Action(23-26)packages/blocks/ticket-list/src/frontend/TicketList.types.ts (1)
Action(24-28)packages/ui/src/elements/button.tsx (1)
Button(19-19)packages/ui/src/components/DynamicIcon/DynamicIcon.tsx (1)
DynamicIcon(8-42)
packages/blocks/ticket-list/src/frontend/TicketList.types.ts (2)
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
Action(23-26)packages/ui/src/lib/utils.ts (1)
baseVariant(9-33)
packages/ui/src/components/Carousel/Carousel.tsx (2)
packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/elements/button.tsx (1)
Button(19-19)
packages/blocks/order-details/src/frontend/OrderDetails.client.tsx (2)
packages/blocks/order-details/src/frontend/OrderDetails.types.ts (1)
Action(23-26)packages/ui/src/components/DynamicIcon/DynamicIcon.tsx (1)
DynamicIcon(8-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (6)
packages/blocks/order-details/src/frontend/OrderDetails.client.tsx (2)
169-189: LGTM! Clean action definition with conditional styling.The
actionsDefinitionarray cleanly encapsulates action metadata, and the conditional variant/className logic based ondata.order.overdue.isOverdueis well-structured. The slicing approach to include/exclude the "pay online" action based on overdue state is straightforward.
213-231: LGTM! Unified ActionList API usage.The migration to the unified
actionsprop is well-implemented. The filter ensures only actions with labels are rendered, and the TooltipHover pattern for "coming soon" functionality is consistent.packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (2)
146-160: LGTM! Clean tableActions definition.The conditional creation of
tableActionsand the render function properly encapsulate the per-row action button linking to ticket details.
173-187: LGTM! Unified ActionList API usage.The migration to the unified
actionsprop is well-implemented, consistent with the OrderDetails pattern. The filter ensures only actions with labels are rendered.packages/ui/src/components/Carousel/Carousel.tsx (2)
3-5: LGTM!The imports are appropriate. Using
lucide-reacticons aligns with the design-system consistency goal mentioned in the PR objectives.
42-69: LGTM!The Swiper configuration is well-structured. The lifecycle hooks properly synchronize React state with Swiper's internal state, and disabling the built-in navigation while using custom buttons is the right approach.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ui/src/components/Carousel/Carousel.tsx:
- Around line 71-96: The wrapper div's flex classes are ineffective because the
two Button components are absolutely positioned; remove the absolute positioning
from the Buttons (or move positioning to the wrapper) so the flex layout in the
wrapper (the div containing the Buttons) actually controls placement, and ensure
the Buttons (the previous/next controls that call swiperRef.current?.slidePrev()
and slideNext()) include descriptive aria-label attributes (e.g., "Previous
slide" and "Next slide") and keep their disabled logic using index and isEnd
unchanged.
🧹 Nitpick comments (2)
packages/ui/src/components/Carousel/Carousel.tsx (2)
33-39: Consider removing the Navigation module since it's no longer used.The
Navigationmodule is conditionally added toallModuleswhenshowNavigationis true, but Swiper's built-in navigation is disabled (navigation={false}on line 58) in favor of custom buttons. Loading the module adds unnecessary overhead.Suggested improvement
const allModules = [ A11y, Keyboard, - ...(showNavigation ? [Navigation] : []), ...(showPagination ? [Pagination] : []), ...modules, ];Also consider removing the
Navigationimport on line 9 if no longer needed elsewhere.
64-68: Using array index as key.Using the array index as a key works for static slide content but can cause rendering issues if slides are dynamically added, removed, or reordered. If slides have stable unique identifiers, prefer using those as keys.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/components/Carousel/Carousel.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (2)
packages/ui/src/components/Carousel/Carousel.tsx (2)
3-5: LGTM on imports and state setup.The ref and state setup correctly prepares for controlled navigation. The
onInitcallback properly synchronizes initial state with Swiper's actual state.Also applies to: 14-15, 28-31
42-69: LGTM on Swiper configuration and state synchronization.The event handlers (
onBeforeInit,onInit,onSlideChange) correctly synchronize React state with Swiper's internal state. Disabling built-in navigation while using custom buttons is properly configured.
…/openselfservice into feature/ui-component-improvements
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ui/src/components/Carousel/Carousel.tsx:
- Around line 75-102: The navigation disable logic doesn't account for looped
carousels; update the disabled checks on the prev/next Buttons to respect loop
mode by deriving a boolean loop flag (prefer swiperProps.loop if passed,
otherwise fallback to swiperRef.current?.params?.loop) and use disabled={index
=== 0 && !loop} for the previous button and disabled={isEnd && !loop} for the
next button (adjust where loop is computed so both ArrowLeftIcon and
ArrowRightIcon button props reference it).
🧹 Nitpick comments (2)
packages/ui/src/components/Carousel/Carousel.tsx (2)
68-72: Consider makingshadow-lgconfigurable.The hardcoded
shadow-lgclass on each slide may not suit all use cases. Consider exposing aslideClassNameprop if consumers need different styling.
37-43: Navigation module may no longer be needed.Since you're using custom external buttons with
navigation={false}, theNavigationmodule from Swiper might be unnecessary. Consider removing it fromallModulesto reduce bundle size.const allModules = [ A11y, Keyboard, - ...(showNavigation ? [Navigation] : []), ...(showPagination ? [Pagination] : []), ...modules, ];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui/src/components/Carousel/Carousel.tsxpackages/ui/src/components/Carousel/Carousel.types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/Carousel/Carousel.tsx (2)
packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/elements/button.tsx (1)
Button(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (4)
packages/ui/src/components/Carousel/Carousel.types.ts (1)
11-14: LGTM!The new
labelsprop is well-defined with optional fields, appropriately matching the default values provided in the component implementation.packages/ui/src/components/Carousel/Carousel.tsx (3)
3-16: LGTM!Clean import organization with appropriate use of design system components (
Button) and lucide-react icons for consistency.
32-35: LGTM!State initialization is appropriate. The
isEndinitial value offalseis corrected immediately viaonInit, which properly synchronizes bothindexandisEndfrom the Swiper instance.
18-31: Good implementation of configurable labels with sensible defaults.The default aria-labels ("Previous slide" / "Next slide") provide good accessibility out of the box while allowing customization for i18n or different contexts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ui/src/components/Carousel/Carousel.tsx:
- Around line 69-74: The prop spread {...swiperProps} currently comes after
explicit Swiper props in Carousel.tsx so consumer-provided keys (e.g.,
navigation, initialSlide, pagination, keyboard, noSwipingSelector) can override
internal config; move the spread before the explicit props or sanitize
swiperProps by removing/ignoring these specific keys (navigation, pagination,
initialSlide, keyboard, noSwipingSelector) before spreading so the
component-controlled values take precedence (update the JSX where swiperProps is
spread and/or add a small helper to omit conflicting keys).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/components/Carousel/Carousel.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/Carousel/Carousel.tsx (2)
packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/elements/button.tsx (1)
Button(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (2)
packages/ui/src/components/Carousel/Carousel.tsx (2)
26-29: Good accessibility defaults for navigation labels.The
labelsprop with sensible defaults ensures screen readers can announce navigation actions even when consumers don't explicitly provide labels.
83-111: Custom navigation implementation looks solid.The navigation buttons correctly handle both loop and non-loop modes, use the design system's Button component for consistency, and include proper aria-labels for accessibility. The disable logic appropriately prevents navigation beyond bounds when not in loop mode.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ui/src/components/Carousel/Carousel.tsx:
- Around line 34-36: The initial isEnd state can be wrong when
startingSlideIndex is the last slide; update the useState initialization for
isEnd to derive from startingSlideIndex and slides length (e.g., const [isEnd,
setIsEnd] = useState(startingSlideIndex === slides.length - 1 || false)) or
equivalent logic so isEnd is true when startingSlideIndex === slides.length - 1;
leave onInit and setIsEnd calls unchanged.
🧹 Nitpick comments (1)
packages/ui/src/components/Carousel/Carousel.tsx (1)
76-80: Variable shadowing:indexin map shadows the state variable.The
indexparameter in the map callback shadows theindexstate variable from line 34. While functionally harmless here, it reduces readability and could cause confusion during maintenance.Suggested fix
- {slides.map((slide, index) => ( - <SwiperSlide key={index} className="!h-auto"> + {slides.map((slide, slideIndex) => ( + <SwiperSlide key={slideIndex} className="!h-auto"> {slide} </SwiperSlide> ))}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/components/Carousel/Carousel.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/Carousel/Carousel.tsx (2)
packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/elements/button.tsx (1)
Button(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (2)
packages/ui/src/components/Carousel/Carousel.tsx (2)
83-111: LGTM!The custom navigation implementation is well done:
- Proper disabled state logic accounting for loop mode
- Good accessibility with configurable aria-labels
- Defensive optional chaining on the swiper ref
18-31: Good use of default prop values for thelabelsprop.The default labels ensure accessibility out of the box while allowing customization for internationalization.
What does this PR do?
Related Ticket(s)
Key Changes
MoreActionsMenucomponent to provide a reusable dropdown menu for actions, extracted fromActionListto improve code reusabilityActionListcomponent API by replacingvisibleActionsanddropdownActionsprops with a unifiedactionsprop array, simplifying the component interface and reducing complexityOrderDetailsandTicketListblocks to use the new unifiedActionListAPI, improving consistency across the codebaseCarouselcomponent with custom navigation buttons using lucide-react icons, replacing default Swiper navigation for better design system consistencyChartTooltipcomponent by removing explicit type annotations in map function, improving code readabilityInformativeCardborder styling to useborder-card-borderfor better visual consistencySummary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.