Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughAdds DataGrid and DataView UI components, a shared renderCell utility, a new FilterViewModeToggle and viewMode wiring, and a cardHeaderSlots property propagated from CMS into multiple list block models, mappers, mocks, frontends, stories, and types. (28 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Block as List Block Component
participant Filters
participant ViewState as viewMode State
participant DataView
participant DataGrid
participant DataList
participant renderCell
User->>Block: Load block (data + filters + cardHeaderSlots)
Block->>ViewState: extract initial viewMode from filters
Block->>Filters: render filters (includes viewMode toggle)
User->>Filters: toggle viewMode
Filters->>Block: call item.onChange with new viewMode
Block->>ViewState: set viewMode
Block->>DataView: render(viewMode, data, columns, cardHeaderSlots)
alt viewMode == "grid"
DataView->>DataGrid: render grid with cardHeaderSlots + actions
DataGrid->>User: card-grid UI
else viewMode == "list"
DataView->>DataList: render list with actions
DataList->>renderCell: render cells via shared renderer
DataList->>User: list UI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
♻️ Duplicate comments (1)
packages/blocks/ticket-list/src/frontend/TicketList.client.stories.tsx (1)
58-62: Story slots also pointleftto non‑existentidcolumnThis story uses the same
slotsconfig as the mocks (left: 'id'), but the table columns still don’t include anidcolumn, so the left slot won’t render anything in the grid header. This is the same issue noted in the CMS ticket‑list mocks; once you adjust the slots/columns there, please mirror the fix here to keep the story in sync.
🧹 Nitpick comments (9)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (1)
27-32: Slots API shape looks good and stays backward compatibleThe optional
slotsobject mirrors the CMS model and cleanly extendsTicketListBlockwithout breaking existing consumers. If you find yourself reusing this shape elsewhere, consider extracting a sharedSlotsConfigtype to avoid duplication, but it’s not urgent.packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts (1)
146-150: View‑mode toggle mocks are consistent across localesThe added
FilterViewModeToggleitems withid: 'viewMode'and defaultvalue: 'grid'are consistent in EN/DE/PL and line up with the new filter type. If you want the legacy list view to remain the default, you might switch these defaults to'list', but as mocks they’re fine either way.Also applies to: 311-315, 477-481
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
39-46: View‑mode state is independent of filter reset; confirm intended UX
initialViewModeis derived from the incoming filters once and stored in local state:const initialViewMode = component.filters?.items.find((item) => item.__typename === 'FilterViewModeToggle')?.value || 'list'; const [viewMode, setViewMode] = useState<'list' | 'grid'>(initialViewMode);
handleResetresets backend filters/data but doesn’t resetviewMode, andFilterViewModeToggleis wired only viaonChange: setViewMode. That means “Reset/Clear filters” keeps the current view mode, effectively treating it as a persistent UI preference rather than a filter.If that’s the desired behavior, this is fine. If you expect reset to also revert the view mode to its initial value, you could additionally call
setViewMode(initialViewMode)insidehandleReset.Also applies to: 58-64, 167-183
packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts (1)
12-30: Filters/viewMode and slots typing look consistent; consider centralizing the slots typeThe added
viewModein the filters type matches the new toggle and should integrate cleanly with the form model, and theslotsshape is aligned with the DataGrid/DataList usage. As a small cleanup, you might want to extract a sharedSlotsConfigtype (e.g. in a common models/util file) so the{ top; left; right; bottom }structure is not duplicated between CMS models and UI props.packages/framework/src/utils/models/filters.ts (1)
12-17: MakeFilterViewModeToggle.onChangerequired to match UI usage
FilterViewModeToggle.onChangeis optional here, but the UI renderer (FilterItemcase 'FilterViewModeToggle') callsitem.onChange!unconditionally. This makes it easy to introduce a runtime error if a caller forgets to wireonChange. Making it required keeps the types honest and prevents silent misconfiguration.-export class FilterViewModeToggle { +export class FilterViewModeToggle { __typename!: 'FilterViewModeToggle'; id!: 'viewMode'; - value?: 'list' | 'grid'; - onChange?: (value: 'list' | 'grid') => void; + value?: 'list' | 'grid'; + onChange!: (value: 'list' | 'grid') => void; }Also applies to: 61-66
packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
29-53: Tightenslotstyping to column IDs for better safetyThe props shape looks good and matches the component, but
slotsbeing plainstringloses type-safety w.r.t. the configured columns. You can make slot ids line up withDataListColumnConfig<T>['id']so mismatched slot names are caught at compile time:-export interface DataGridProps<T> { +export interface DataGridProps<T> { @@ - slots?: { - top?: string; - left?: string; - right?: string; - bottom?: string; - }; + slots?: { + top?: DataListColumnConfig<T>['id']; + left?: DataListColumnConfig<T>['id']; + right?: DataListColumnConfig<T>['id']; + bottom?: DataListColumnConfig<T>['id']; + };Optionally, you could also align with the component by constraining
There (DataGridProps<T extends Record<string, any>>) so callers get consistent expectations aboutT.packages/ui/src/lib/renderCell.tsx (1)
16-69: Loosenvaluetyping and guard badge access to avoid unnecessary casts
renderCellcurrently requiresvalue: Record<string, unknown>, but many callers pass primitives and have to cast arbitrarily (as Record<string, unknown>). You’re already checkingtypeof value === 'object'in most cases; you could reflect that in the type and slightly harden the badge branch:-export function renderCell<T>( - value: Record<string, unknown>, +export function renderCell<T>( + // Allow primitives; branches already narrow as needed + value: unknown, @@ - case 'badge': { - const badgeLabel = String(value[column.labelField || 'label']); - const badgeValue = String(value[column.valueField || 'value']); + case 'badge': { + if (typeof value !== 'object' || value === null) { + return null; + } + const record = value as Record<string, unknown>; + const badgeLabel = String(record[column.labelField || 'label']); + const badgeValue = String(record[column.valueField || 'value']); const variant = column.variant ? column.variant(badgeValue) : 'default';This makes the signature better match actual usage and avoids unsafe indexing when a badge cell value isn’t an object.
packages/ui/src/components/DataGrid/DataGrid.stories.tsx (1)
148-261: Tighten story typings and de‑duplicate common argsThe stories are fine functionally, but the repeated
as Record<string, unknown>[]/as DataListColumnConfig<Record<string, unknown>>[]casts weaken type‑safety and duplicate the samedata/columns/slotssetup in several places.If you want stronger typing with minimal change, consider:
- Typing
argsasDataGridProps<Ticket>directly (e.g. via a sharedconst baseArgs: DataGridProps<Ticket> = { … }) and- Reusing
baseArgswith spreads in individual stories to reduce duplication.This keeps the stories easier to maintain and lets TS catch drift between
TicketandticketColumns.packages/ui/src/components/Filters/Filters.tsx (1)
17-33:separateLeadingItemcorrectly isolates the view mode toggleThe updated
separateLeadingItemcleanly separates:
- a single
leadingItem,- a single
FilterViewModeToggle, and- the remaining
filteredItems.This matches the intended behavior where view mode is orthogonal to form‑driven filters. It assumes there is at most one
FilterViewModeToggle, which is reasonable for current usage; if that ever changes, you might want to assert or log when multiple toggles are passed instead of silently taking the last one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts(1 hunks)packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts(1 hunks)packages/blocks/ticket-list/src/frontend/TicketList.client.stories.tsx(2 hunks)packages/blocks/ticket-list/src/frontend/TicketList.client.tsx(5 hunks)packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts(2 hunks)packages/framework/src/utils/models/filters.ts(2 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts(6 hunks)packages/ui/src/components/DataGrid/DataGrid.stories.tsx(1 hunks)packages/ui/src/components/DataGrid/DataGrid.tsx(1 hunks)packages/ui/src/components/DataGrid/DataGrid.types.ts(1 hunks)packages/ui/src/components/DataGrid/index.ts(1 hunks)packages/ui/src/components/DataList/DataList.tsx(1 hunks)packages/ui/src/components/Filters/FilterItem.tsx(2 hunks)packages/ui/src/components/Filters/Filters.tsx(3 hunks)packages/ui/src/components/Filters/FiltersSection.tsx(1 hunks)packages/ui/src/lib/renderCell.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
packages/ui/src/lib/renderCell.tsx (2)
packages/framework/src/utils/models/badge.ts (1)
Badge(1-4)packages/framework/src/utils/models/price.ts (1)
Price(3-7)
packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts (3)
packages/framework/src/utils/models/filters.ts (1)
Filters(1-10)packages/ui/src/components/Filters/Filters.tsx (1)
Filters(35-160)packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (1)
Ticket(35-52)
packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
packages/ui/src/components/DataGrid/index.ts (2)
DataGridColumnsCount(2-2)DataGridProps(2-2)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (4)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (2)
TicketListBlock(7-33)Ticket(35-52)packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts (1)
TicketListBlock(6-31)packages/ui/src/components/DataGrid/DataGrid.tsx (1)
DataGrid(19-194)packages/ui/src/components/DataList/DataList.tsx (1)
DataList(13-88)
packages/ui/src/components/Filters/FilterItem.tsx (1)
packages/ui/src/elements/toggle-group.tsx (2)
ToggleGroup(70-70)ToggleGroupItem(70-70)
packages/ui/src/components/DataGrid/DataGrid.stories.tsx (3)
packages/ui/src/components/DataGrid/DataGrid.tsx (1)
DataGrid(19-194)packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (1)
Ticket(35-52)packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
DataGridProps(29-74)
packages/ui/src/components/DataGrid/DataGrid.tsx (3)
packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
DataGridProps(29-74)packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/lib/renderCell.tsx (1)
renderCell(16-100)
packages/ui/src/components/Filters/Filters.tsx (2)
packages/framework/src/utils/models/filters.ts (2)
FilterViewModeToggle(61-66)FilterItem(12-17)packages/ui/src/components/Filters/FilterItem.tsx (1)
FilterItem(16-227)
packages/framework/src/utils/models/filters.ts (1)
packages/ui/src/components/Filters/FilterItem.tsx (1)
FilterItem(16-227)
⏰ 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 (8)
packages/ui/src/components/Filters/FiltersSection.tsx (1)
23-23: Robust leading‑item check for mixed filter item typesUsing
'isLeading' in item && item.isLeading === trueis a good improvement: it avoids relying on properties that may not exist on all filter item variants and keeps layout logic from being triggered by items that don’t supportisLeading.packages/blocks/ticket-list/src/frontend/TicketList.client.stories.tsx (1)
216-220: Storybook wiring forFilterViewModeTogglelooks goodIncluding a
FilterViewModeTogglewithvalue: 'grid'ensures the new grid view is exercised in Storybook and makes it easy to validate the toggle UI.packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts (1)
29-29: Slots passthrough keeps API harmonization aligned with CMSMapping
slots: cms.slotsdirectly into theTicketListBlockoutput cleanly exposes the CMS configuration to the frontend and matches the updated model type. No issues here.packages/ui/src/components/DataGrid/index.ts (1)
1-2: Public DataGrid exports are straightforward and consistentRe‑exporting
DataGridplus its props andDataGridColumnsCounttype from an index barrel is aligned with the rest of the UI components and keeps the import surface tidy.packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
97-120: Grid/list‑aware actions and body rendering look consistentOverriding
actions.renderto switch button variant/layout based onviewMode, and conditionally rendering eitherDataGrid(withslots={data.slots}) orDataListusing the samecolumnsconfig, keeps behavior consistent between views while adapting the UI affordances appropriately. No issues spotted here.Also applies to: 195-204
packages/ui/src/components/DataGrid/DataGrid.stories.tsx (1)
23-146: Sample Ticket model and column config are coherent with DataGrid expectationsThe
Ticketshape,sampleTickets, andticketColumnsline up well with the DataGrid + sharedrenderCellmodel (label/value objects for text/date fields and astatusbadge variant based on the value). This should give very representative stories for ticket‑list usage.packages/ui/src/components/DataList/DataList.tsx (1)
3-87: Refactor to sharedrenderCelland table primitives looks solidDelegating cell rendering to
renderCelland using the localTableprimitives keeps DataList lean while preserving behavior (row keys, header rendering, per‑column cellClassName, actions column, and text truncation). I don’t see regressions here; this should make it much easier to keep DataList/DataGrid rendering consistent.packages/ui/src/components/Filters/Filters.tsx (1)
51-96: Rendering leading filter and view mode toggle side‑by‑side is wired correctlyThe top bar now:
- Keeps the leading filter inside the horizontal
ScrollContainer, and- Places the
viewModeToggleimmediately next to it as a regularFilterItem.
viewModeToggleis excluded from the sheet’sfilteredItemslist, so it only appears once, and it doesn’t interfere with Formik state (consistent withFilterItem’s standalone handling forFilterViewModeToggle). The flex layout and keys all look correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
41-43: Type narrowing in the find() predicate is not being utilized, but the code is functionally correct.The pattern
component.filters?.items.find((item) => item.__typename === 'FilterViewModeToggle')?.valuedoes not narrow TypeScript's understanding of the item type within the find predicate because it lacks an explicit type predicate. However, this doesn't create a runtime issue—the optional chaining?.valuesafely handles access regardless of the union type, and TypeScript compilation passes without errors.While the code works correctly, it could be improved by using an explicit type predicate (
item is FilterViewModeToggle) for better type safety. Contrast this with line 173, where the if-statement guard does provide proper type narrowing. For consistency and clarity, consider using a type predicate in the find call:const initialViewMode = component.filters?.items.find((item): item is FilterViewModeToggle => item.__typename === 'FilterViewModeToggle')?.value || 'list';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (176)
packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/index.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/index.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/index.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/index.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.client.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.client.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.client.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.client.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.controller.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.controller.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.controller.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.controller.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.mapper.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.mapper.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.mapper.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.mapper.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.model.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.model.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.model.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.model.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.module.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.module.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.module.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.module.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.request.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.request.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.request.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.request.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.service.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.service.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.service.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/api-harmonization/api-harmonization/notification-summary.service.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/api-harmonization/notification-summary.client.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/api-harmonization/notification-summary.client.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/api-harmonization/notification-summary.client.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/api-harmonization/notification-summary.model.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/api-harmonization/notification-summary.model.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/api-harmonization/notification-summary.model.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/api-harmonization/notification-summary.request.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/api-harmonization/notification-summary.request.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/api-harmonization/notification-summary.request.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.client.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.client.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.client.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.client.stories.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.client.stories.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.client.stories.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.renderer.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.renderer.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.renderer.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.server.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.server.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.server.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.types.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.types.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/frontend/NotificationSummary.types.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/index.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/frontend/index.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/frontend/index.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/sdk/index.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/sdk/index.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/sdk/index.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/sdk/notification-summary.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/frontend/sdk/notification-summary.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/frontend/sdk/notification-summary.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.client.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.client.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.client.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.client.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.model.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.model.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.model.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.model.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.request.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.request.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.request.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/api-harmonization/notification-summary.request.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/sdk/sdk/index.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/sdk/index.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/sdk/sdk/index.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/sdk/index.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/sdk/sdk/notification-summary.d.tsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/sdk/notification-summary.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/sdk/sdk/notification-summary.jsis excluded by!**/dist/**packages/blocks/notification-summary/dist/sdk/sdk/notification-summary.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/notification-summary/dist/tsconfig.api.tsbuildinfois excluded by!**/dist/**packages/blocks/notification-summary/dist/tsconfig.frontend.tsbuildinfois excluded by!**/dist/**packages/blocks/notification-summary/dist/tsconfig.sdk.tsbuildinfois excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/index.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/index.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/index.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/index.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.client.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.client.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.client.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.client.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.controller.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.controller.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.controller.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.controller.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.mapper.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.mapper.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.mapper.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.mapper.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.model.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.model.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.model.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.model.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.module.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.module.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.module.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.module.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.request.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.request.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.request.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.request.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.service.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.service.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.service.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/api-harmonization/api-harmonization/ticket-summary.service.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/api-harmonization/ticket-summary.client.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/api-harmonization/ticket-summary.client.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/api-harmonization/ticket-summary.client.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/api-harmonization/ticket-summary.model.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/api-harmonization/ticket-summary.model.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/api-harmonization/ticket-summary.model.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/api-harmonization/ticket-summary.request.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/api-harmonization/ticket-summary.request.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/api-harmonization/ticket-summary.request.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.client.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.client.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.client.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.client.stories.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.client.stories.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.client.stories.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.renderer.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.renderer.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.renderer.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.server.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.server.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.server.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.types.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.types.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/frontend/TicketSummary.types.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/index.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/frontend/index.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/frontend/index.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/sdk/index.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/sdk/index.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/sdk/index.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/sdk/ticket-summary.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/frontend/sdk/ticket-summary.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/frontend/sdk/ticket-summary.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.client.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.client.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.client.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.client.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.model.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.model.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.model.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.model.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.request.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.request.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.request.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/api-harmonization/ticket-summary.request.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/sdk/sdk/index.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/sdk/index.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/sdk/sdk/index.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/sdk/index.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/sdk/sdk/ticket-summary.d.tsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/sdk/ticket-summary.d.ts.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/sdk/sdk/ticket-summary.jsis excluded by!**/dist/**packages/blocks/ticket-summary/dist/sdk/sdk/ticket-summary.js.mapis excluded by!**/dist/**,!**/*.mappackages/blocks/ticket-summary/dist/tsconfig.api.tsbuildinfois excluded by!**/dist/**packages/blocks/ticket-summary/dist/tsconfig.frontend.tsbuildinfois excluded by!**/dist/**packages/blocks/ticket-summary/dist/tsconfig.sdk.tsbuildinfois excluded by!**/dist/**
📒 Files selected for processing (3)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts(1 hunks)packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts(1 hunks)packages/blocks/ticket-list/src/frontend/TicketList.client.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts
- packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts
⏰ 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/ticket-list/src/frontend/TicketList.client.tsx (6)
11-13: LGTM!The new imports are necessary for the grid view functionality and are properly used throughout the component.
47-47: LGTM!The viewMode state is properly typed and initialized from the filters, enabling the view mode toggle functionality.
101-120: LGTM!The conditional action rendering appropriately adapts button variants for grid (tertiary) and list (link) views, maintaining visual consistency with each layout style.
60-66: Consider resetting viewMode in handleReset.The
handleResetfunction resets filter state toinitialFiltersbut does not resetviewModetoinitialViewMode. This means the view mode persists when users click reset.If this is intentional (treating view mode as a separate user preference), it's fine. However, if "reset" should restore all UI state including the view mode, consider adding:
const handleReset = () => { startTransition(async () => { const newData = await sdk.blocks.getTicketList(initialFilters, { 'x-locale': locale }, accessToken); setFilters(initialFilters); + setViewMode(initialViewMode); setData(newData); }); };
196-205: Original review comment is incorrect; no issues with slots handling.The
slotsproperty is defined as optional in theTicketListBlockmodel (line 24-29 inpackages/framework/src/modules/cms/models/blocks/ticket-list.model.ts), and theDataGridcomponent safely handles undefined slots using optional chaining (slots?.top,slots?.left, etc.) on lines 72-75 ofDataGrid.tsx. No explicit undefined check is needed—the component gracefully handles both defined and undefined slots.
168-184: Type safety for FilterViewModeToggle property assignment is correct.The FilterViewModeToggle class (packages/framework/src/utils/models/filters.ts:61-66) already defines
value?: 'list' | 'grid'andonChange?: (value: 'list' | 'grid') => voidas optional properties. The code spreads the item and overrides these properties with matching types, making this pattern type-safe. No concerns.
…ssary slot handling
…nality and code clarity
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ui/src/components/DataGrid/DataGrid.tsx (1)
43-53: Move pure helper function outside the component.
getGridColsClassdoesn't depend on component state or props and can be moved outside to avoid recreation on each render.This was previously flagged by marcinkrasowski.
🧹 Nitpick comments (1)
.changeset/long-gifts-divide.md (1)
13-13: Commit message prefix inconsistent with change type.The description uses
fix:but this PR introduces new features (DataGrid component, FilterViewModeToggle, view mode state). Consider usingfeat:to align with semantic versioning conventions and the minor version bumps declared above.-fix: update FilterItem and renderCell components for improved functionality and code clarity +feat: add DataGrid component and view mode toggle for data lists
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/long-gifts-divide.md(1 hunks)packages/ui/src/components/DataGrid/DataGrid.tsx(1 hunks)packages/ui/src/components/Filters/FilterItem.tsx(2 hunks)packages/ui/src/lib/renderCell.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/components/Filters/FilterItem.tsx
- packages/ui/src/lib/renderCell.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/DataGrid/DataGrid.tsx (7)
packages/ui/src/components/DataGrid/index.ts (2)
DataGrid(1-1)DataGridProps(2-2)packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
DataGridProps(29-74)packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/elements/card.tsx (3)
CardHeader(31-31)CardContent(31-31)CardFooter(31-31)packages/ui/src/lib/renderCell.tsx (1)
renderCell(16-100)packages/ui/src/elements/typography.tsx (1)
Typography(56-56)packages/ui/src/elements/separator.tsx (1)
Separator(23-23)
⏰ 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 (3)
packages/ui/src/components/DataGrid/DataGrid.tsx (3)
88-128: LGTM!The slot-based CardHeader rendering with Typography variants for left/bottom columns and flexible layout is well-structured.
131-151: LGTM!The body column rendering correctly handles null/undefined values with
cellContent == null, andbodyColumnsis computed once before iteration—both addressing previous review feedback.
153-158: LGTM!The actions footer rendering with conditional display and Separator is clean.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/ui/src/components/DataGrid/DataGrid.tsx (1)
56-62: Dynamic Tailwind classes won't work at build time.Tailwind CSS purges classes not present in source as complete strings. The template literals on lines 59-60 (
md:${getGridColsClass(...)}andlg:${getGridColsClass(...)}) generate classes dynamically at runtime, which Tailwind's JIT compiler cannot detect during the build.Consider using a safelist approach or pre-generating the full class strings:
+const RESPONSIVE_GRID_COLS: Record<number, { mobile: string; tablet: string; desktop: string }> = { + 1: { mobile: 'grid-cols-1', tablet: 'md:grid-cols-1', desktop: 'lg:grid-cols-1' }, + 2: { mobile: 'grid-cols-2', tablet: 'md:grid-cols-2', desktop: 'lg:grid-cols-2' }, + 3: { mobile: 'grid-cols-3', tablet: 'md:grid-cols-3', desktop: 'lg:grid-cols-3' }, + 4: { mobile: 'grid-cols-4', tablet: 'md:grid-cols-4', desktop: 'lg:grid-cols-4' }, + 5: { mobile: 'grid-cols-5', tablet: 'md:grid-cols-5', desktop: 'lg:grid-cols-5' }, + 6: { mobile: 'grid-cols-6', tablet: 'md:grid-cols-6', desktop: 'lg:grid-cols-6' }, +}; + +const getResponsiveGridClasses = (mobile: number, tablet: number, desktop: number): string => { + const mobileClass = RESPONSIVE_GRID_COLS[mobile]?.mobile || 'grid-cols-1'; + const tabletClass = RESPONSIVE_GRID_COLS[tablet]?.tablet || 'md:grid-cols-2'; + const desktopClass = RESPONSIVE_GRID_COLS[desktop]?.desktop || 'lg:grid-cols-3'; + return cn(mobileClass, tabletClass, desktopClass); +}; + - // Build responsive grid classes const gridClasses = cn( 'grid gap-4', - getGridColsClass(mobileCols), - `md:${getGridColsClass(tabletCols)}`, - `lg:${getGridColsClass(desktopCols)}`, + getResponsiveGridClasses(mobileCols, tabletCols, desktopCols), className, );Alternatively, add the possible classes to Tailwind's
safelistintailwind.config.js:module.exports = { safelist: [ 'grid-cols-1', 'grid-cols-2', 'grid-cols-3', 'grid-cols-4', 'grid-cols-5', 'grid-cols-6', 'md:grid-cols-1', 'md:grid-cols-2', 'md:grid-cols-3', 'md:grid-cols-4', 'md:grid-cols-5', 'md:grid-cols-6', 'lg:grid-cols-1', 'lg:grid-cols-2', 'lg:grid-cols-3', 'lg:grid-cols-4', 'lg:grid-cols-5', 'lg:grid-cols-6', ], // ... rest of config }
🧹 Nitpick comments (1)
packages/ui/src/components/DataGrid/DataGrid.tsx (1)
38-39: Consider usingunknowninstead ofanyin the generic constraint.The generic constraint
Record<string, any>usesany, which bypasses type checking. Consider usingRecord<string, unknown>for better type safety while maintaining flexibility.Apply this diff:
-// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function DataGrid<T extends Record<string, any>>({ +export function DataGrid<T extends Record<string, unknown>>({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx(5 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts(6 hunks)packages/ui/src/components/DataGrid/DataGrid.stories.tsx(1 hunks)packages/ui/src/components/DataGrid/DataGrid.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/blocks/ticket-list/src/frontend/TicketList.client.tsx
- packages/ui/src/components/DataGrid/DataGrid.stories.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts
🧬 Code graph analysis (1)
packages/ui/src/components/DataGrid/DataGrid.tsx (3)
packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
DataGridProps(29-74)packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/lib/renderCell.tsx (1)
renderCell(16-100)
⏰ 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 (5)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts (2)
36-40: Slots configuration correctly references existing columns.The slot mappings now use valid column IDs (
topic,status,updatedAt) that exist intable.columns, resolving the issue flagged in previous reviews. The implementation looks good.Optional observation: The
topslot is not defined, though the DataGrid component supports it according to the PR summary. If a top row (e.g., for category badges or tags) isn't needed for ticket cards, this is fine; otherwise, consider whether any content should occupy that position.
Note: This addresses the concern raised in previous review comments where
slots.left = 'id'didn't match any column.Also applies to: 199-203, 365-369
146-150: Verify that FilterViewModeToggle requires no label.The new
FilterViewModeTogglefilter item doesn't include alabelproperty, unlike other filter types in the array (FilterSelect,FilterToggleGroup,FilterDateRangeall have labels). If this toggle is rendered as an icon-only control (e.g., list/grid icons), the absence of a label is intentional and appropriate. Please confirm this design choice.Also applies to: 311-315, 477-481
packages/ui/src/components/DataGrid/DataGrid.tsx (3)
1-21: LGTM! Clean imports and well-structured constant.The module-level
GRID_COLS_MAPconstant is well-organized and addresses the past review feedback about moving helper functions outside the component.
23-32: LGTM! Helper functions are well-implemented.Both
getGridColsClassanddefaultGetRowKeyare correctly placed at module level and provide sensible defaults.
77-146: LGTM! Rendering logic is well-structured and addresses past review feedback.The implementation correctly:
- Computes
bodyColumnsonce outside the map (line 75), avoiding unnecessary recomputation per row- Uses loose equality
cellContent == null(line 120) to catch both null and undefined while preserving falsy values like'',0, andfalse- Employs a clear Card-based layout with proper slot positioning
- Uses static Tailwind classes in the body grid (line 115), which won't be affected by purging
…id and list rendering
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/ui/src/components/DataGrid/DataGrid.tsx (1)
53-59: Dynamic Tailwind classes will be purged at build time.Lines 56-57 use template literals (
md:${getGridColsClass(tabletCols)}) to construct Tailwind classes dynamically. Tailwind's JIT compiler cannot detect these at build time, so the classes will be purged. This was flagged in a previous review but the fix appears incomplete.Apply explicit breakpoint-prefixed mappings:
const GRID_COLS_MAP: Record<number, string> = { }; +const MD_GRID_COLS_MAP: Record<number, string> = { + 1: 'md:grid-cols-1', + 2: 'md:grid-cols-2', + 3: 'md:grid-cols-3', + 4: 'md:grid-cols-4', + 5: 'md:grid-cols-5', + 6: 'md:grid-cols-6', +}; + +const LG_GRID_COLS_MAP: Record<number, string> = { + 1: 'lg:grid-cols-1', + 2: 'lg:grid-cols-2', + 3: 'lg:grid-cols-3', + 4: 'lg:grid-cols-4', + 5: 'lg:grid-cols-5', + 6: 'lg:grid-cols-6', +}; const getGridColsClass = (cols: number): string => { return GRID_COLS_MAP[cols] || 'grid-cols-1'; }; +const getMdGridColsClass = (cols: number): string => { + return MD_GRID_COLS_MAP[cols] || 'md:grid-cols-2'; +}; + +const getLgGridColsClass = (cols: number): string => { + return LG_GRID_COLS_MAP[cols] || 'lg:grid-cols-3'; +};Then update the class construction:
const gridClasses = cn( 'grid gap-4', getGridColsClass(mobileCols), - `md:${getGridColsClass(tabletCols)}`, - `lg:${getGridColsClass(desktopCols)}`, + getMdGridColsClass(tabletCols), + getLgGridColsClass(desktopCols), className, );
🧹 Nitpick comments (4)
packages/ui/src/components/DataView/DataView.types.ts (1)
3-19: Add generic constraint for consistency with implementation.The
DataViewProps<T>interface lacks the generic constraint that exists inDataView.tsxandDataGrid.tsx(T extends Record<string, any>). This inconsistency could allow incompatible types at the interface level that would fail at runtime.-export interface DataViewProps<T> { +export interface DataViewProps<T extends Record<string, unknown>> {Using
Record<string, unknown>is also safer thananyfor the constraint.packages/ui/src/components/DataView/DataView.tsx (2)
8-14: Component shadows globalDataView- acceptable in this context.Biome flags this as shadowing the global
DataView(used for typed array buffers). While technically correct, the name is semantically appropriate for this UI component, and the globalDataViewis unlikely to be used in React component code. If the team prefers strict linting compliance, consider renaming toDataViewSwitchor similar.The implementation logic is correct -
slotsandcolumnsCountare properly forwarded only toDataGridsinceDataListdoesn't support them.
15-23: Unnecessary fragment wrapper.The
<>...</>fragment is not needed when there's only one conditional expression.- return ( - <> - {viewMode === 'grid' ? ( - <DataGrid {...props} slots={slots} columnsCount={columnsCount} /> - ) : ( - <DataList {...props} /> - )} - </> - ); + return viewMode === 'grid' ? ( + <DataGrid {...props} slots={slots} columnsCount={columnsCount} /> + ) : ( + <DataList {...props} /> + );packages/ui/src/components/DataView/DataView.stories.tsx (1)
30-38: ReduceRecord<string, unknown>casts by typing stories onTicket(optional)The repetitive casts to
Record<string, unknown>fordata,columns, andactionsweaken type safety around theTicketshape in these stories. IfDataViewPropsandDataViewsupport generics, you can often keep theTickettyping end‑to‑end and avoid theasnoise.For example (adjusting to your actual component typing):
type TicketStory = StoryObj<DataViewProps<Ticket>>; export const ListView: TicketStory = { args: { viewMode: 'list', data: sampleTickets, columns: ticketColumns, actions, }, }; export const GridView: TicketStory = { args: { viewMode: 'grid', data: sampleTickets, columns: ticketColumns, slots: { left: 'id', right: 'status', bottom: 'updatedAt', }, actions, }, };If DataView isn’t generic or StoryObj typing makes this awkward, feel free to keep the current approach, but this can make refactors to
Ticketsafer.Please double‑check how
DataViewPropsis declared; if it’s not generic over row type, this suggestion may not apply as‑is.Also applies to: 70-108, 122-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx(5 hunks)packages/ui/src/components/DataGrid/DataGrid.tsx(1 hunks)packages/ui/src/components/DataView/DataView.stories.tsx(1 hunks)packages/ui/src/components/DataView/DataView.tsx(1 hunks)packages/ui/src/components/DataView/DataView.types.ts(1 hunks)packages/ui/src/components/DataView/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/ui/src/components/DataView/DataView.tsx (2)
packages/ui/src/components/DataView/DataView.types.ts (1)
DataViewProps(3-19)packages/ui/src/components/DataGrid/DataGrid.tsx (1)
DataGrid(36-144)
packages/ui/src/components/DataGrid/DataGrid.tsx (3)
packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
DataGridProps(29-74)packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/lib/renderCell.tsx (1)
renderCell(16-100)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (4)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (2)
TicketListBlock(7-35)Ticket(37-54)packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts (1)
TicketListBlock(6-32)packages/ui/src/components/DataView/DataView.tsx (1)
DataView(9-24)packages/ui/src/components/DataView/index.ts (1)
DataView(1-1)
🪛 Biome (2.1.2)
packages/ui/src/components/DataView/DataView.tsx
[error] 9-9: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx
[error] 12-12: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/ui/src/components/DataView/DataView.stories.tsx
[error] 5-5: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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/ui/src/components/DataGrid/DataGrid.tsx (2)
15-26: Good refactoring: helper functions moved outside component.The
GRID_COLS_MAP,getGridColsClass, anddefaultGetRowKeyare correctly placed outside the component to avoid recomputation on each render. This addresses the past reviewer's feedback.Also applies to: 28-33
111-131: Clean body column rendering with proper null check.The
cellContent == nullcheck (line 117) correctly handles bothnullandundefinedwhile allowing falsy values like0or''to render. The body columns are computed once before the map operation (line 72), avoiding redundant calculations per row.packages/ui/src/components/DataView/index.ts (1)
1-2: LGTM!Clean barrel exports following the established pattern in the codebase.
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (3)
157-173: Filter transformation correctly injects viewMode state.The approach of mapping over filter items to inject
valueandonChangeforFilterViewModeToggleis a clean pattern for connecting component state to filter controls. The spread operator preserves other properties appropriately.
185-191: DataView integration looks correct.The
DataViewcomponent receives all required props:viewMode,data,columns,actions, andslots. This properly delegates to eitherDataGridorDataListbased on the current view mode.Note: The Biome warning about shadowing global
DataViewapplies to the import statement, not a local redefinition. This is a false positive in the context of importing a named export.
40-42: The code is already type-safe; no type assertion or type guard is needed.The
FilterViewModeToggletype is properly defined inpackages/framework/src/utils/models/filters.tswith an optionalvalueproperty typed as'list' | 'grid'. The code on line 42 correctly uses optional chaining (?.value) combined with a fallback (|| 'list'), which safely handles both cases: whenvalueis present or when the entire filter item doesn't exist. TypeScript's type inference ensuresinitialViewModeis correctly typed as'list' | 'grid'for the subsequentuseStatecall.
…onfig for consistency
… for layout flexibility
… based on view mode
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/blocks/notification-list/src/frontend/NotificationList.client.tsx (1)
123-135: Avoid nested interactive elements in row actions (Link wrapping Button)
LinkComponentlikely renders an<a>and currently wraps a<Button>, which results in an anchor containing a button – an invalid and confusing interactive structure. Use the same pattern as inTicketList.client.tsx:Button asChildwithLinkComponentas the child.- const actions = data.table.actions - ? { - ...data.table.actions, - render: (notification: Model.Notification) => ( - <LinkComponent href={notification.detailsUrl}> - <Button variant="link" className="flex items-center justify-end gap-2"> - {data.table.actions!.label} - <ArrowRight className="h-4 w-4" /> - </Button> - </LinkComponent> - ), - } - : undefined; + const actions = data.table.actions + ? { + ...data.table.actions, + render: (notification: Model.Notification) => ( + <Button asChild variant="link"> + <LinkComponent + href={notification.detailsUrl} + className="flex items-center justify-end gap-2" + > + {data.table.actions!.label} + <ArrowRight className="h-4 w-4" /> + </LinkComponent> + </Button> + ), + } + : undefined;This matches the established pattern and avoids nested interactive controls.
♻️ Duplicate comments (5)
packages/ui/src/components/DataView/DataView.tsx (1)
9-9: Consider renaming to avoid confusion with the global DataView.Same naming concern as in consumer files. The component name conflicts with JavaScript's native
DataViewfor binary data. ConsiderDataListView,ListGridView, or similar alternatives.packages/blocks/order-list/src/frontend/OrderList.client.tsx (1)
10-10: Consider renaming to avoid confusion with the global DataView.Same naming concern applies here. See earlier comments about potential confusion with JavaScript's native DataView object.
packages/ui/src/components/DataView/DataView.stories.tsx (2)
5-5: AliasDataViewin stories to avoid shadowing the global and Biome errorThe story imports and types against
DataViewdirectly, which shadows the JS global and trips Biome’snoShadowRestrictedNamesrule. Alias the import and update meta/Story typing:-import { DataView } from '@o2s/ui/components/DataView'; +import { DataView as DataViewComponent } from '@o2s/ui/components/DataView'; -const meta: Meta<typeof DataView> = { +const meta: Meta<typeof DataViewComponent> = { title: 'Components/DataView', - component: DataView, + component: DataViewComponent, @@ -export default meta; -type Story = StoryObj<typeof DataView>; +export default meta; +type Story = StoryObj<typeof DataViewComponent>;This keeps Storybook typing intact while satisfying Biome.
Also applies to: 11-14, 27-28
110-120: Use a semantic link anddetailsUrlin the “View Details” actionThe actions renderer currently ignores
_item.detailsUrland usesButton asChildwith a non‑semantic<span>, so rows are not actually navigable in the story. Consider wiring the action todetailsUrlwith a link:-const actions = { - title: 'Actions', - render: (_item: Ticket) => ( - <Button asChild variant="link" className="flex items-center gap-2"> - <span> - <ArrowRight className="h-4 w-4" /> - View Details - </span> - </Button> - ), -}; +const actions = { + title: 'Actions', + render: (item: Ticket) => ( + <Button asChild variant="link"> + <a href={item.detailsUrl} className="flex items-center gap-2"> + <ArrowRight className="h-4 w-4" /> + <span>View Details</span> + </a> + </Button> + ), +};This better demonstrates real‑world usage and keeps the action focusable per row.
packages/ui/src/components/DataGrid/DataGrid.tsx (1)
14-21: Replace dynamic Tailwind class composition with static responsive mappings
gridClassesbuildsmd:${getGridColsClass(...)}/lg:${...}at runtime, so Tailwind’s compiler can’t see the actual class names and may not generate the responsive grid utilities. This was flagged before and is still present.Consider precomputing breakpoint‑specific class names:
-const GRID_COLS_MAP: Record<number, string> = { - 1: 'grid-cols-1', - 2: 'grid-cols-2', - 3: 'grid-cols-3', - 4: 'grid-cols-4', - 5: 'grid-cols-5', - 6: 'grid-cols-6', -}; - -const getGridColsClass = (cols: number): string => { - return GRID_COLS_MAP[cols] || 'grid-cols-1'; -}; +const GRID_COLS_MAP: Record<number, string> = { + 1: 'grid-cols-1', + 2: 'grid-cols-2', + 3: 'grid-cols-3', + 4: 'grid-cols-4', + 5: 'grid-cols-5', + 6: 'grid-cols-6', +}; + +const MD_GRID_COLS_MAP: Record<number, string> = { + 1: 'md:grid-cols-1', + 2: 'md:grid-cols-2', + 3: 'md:grid-cols-3', + 4: 'md:grid-cols-4', + 5: 'md:grid-cols-5', + 6: 'md:grid-cols-6', +}; + +const LG_GRID_COLS_MAP: Record<number, string> = { + 1: 'lg:grid-cols-1', + 2: 'lg:grid-cols-2', + 3: 'lg:grid-cols-3', + 4: 'lg:grid-cols-4', + 5: 'lg:grid-cols-5', + 6: 'lg:grid-cols-6', +}; @@ - const mobileCols = columnsCount?.mobile ?? 1; - const tabletCols = columnsCount?.tablet ?? 2; - const desktopCols = columnsCount?.desktop ?? 3; - - // Build responsive grid classes - const gridClasses = cn( - 'grid gap-4', - getGridColsClass(mobileCols), - `md:${getGridColsClass(tabletCols)}`, - `lg:${getGridColsClass(desktopCols)}`, - className, - ); + const mobileCols = columnsCount?.mobile ?? 1; + const tabletCols = columnsCount?.tablet ?? 2; + const desktopCols = columnsCount?.desktop ?? 3; + + const gridClasses = cn( + 'grid gap-4', + GRID_COLS_MAP[mobileCols] || 'grid-cols-1', + MD_GRID_COLS_MAP[tabletCols] || 'md:grid-cols-2', + LG_GRID_COLS_MAP[desktopCols] || 'lg:grid-cols-3', + className, + );This keeps all possible Tailwind classes explicit so they’re preserved at build time.
Also applies to: 23-25, 47-58
🧹 Nitpick comments (7)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (1)
29-34: Extraction to shared type is valid—CardHeaderSlots is identically duplicated across 4 blocks.Verification confirms the review comment is accurate. The
cardHeaderSlotstype definition is byte-for-byte identical in invoice-list, notification-list, order-list, and ticket-list blocks:{ top?: string; left?: string; right?: string; bottom?: string; }The codebase has an established pattern for shared types at
packages/framework/src/utils/models/, which already includes similar card-related types (InfoCard, CardWithImage, PricingCard). ExtractingCardHeaderSlotsthere would follow the existing convention and eliminate the duplication.The refactoring suggestion is sound and recommended. Consider creating
packages/framework/src/utils/models/card-header-slots.tsand updating the index export accordingly.packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (1)
13-13: Consider renaming to avoid confusion with the global DataView.The component name
DataViewshadows the global JavaScriptDataViewobject (used for binary data manipulation). While ES modules scope imports locally and this won't cause runtime errors, it may confuse developers or tools expecting the native DataView. Consider a more specific name likeDataListVieworListGridView.packages/ui/src/components/DataView/DataView.tsx (1)
15-23: Remove unnecessary Fragment wrapper.The Fragment (
<>...</>) wrapping the conditional render is unnecessary since there's only a single child. You can return the ternary expression directly.Apply this diff:
- return ( - <> - {viewMode === 'grid' ? ( - <DataGrid {...props} cardHeaderSlots={cardHeaderSlots} columnsCount={columnsCount} /> - ) : ( - <DataList {...props} /> - )} - </> - ); + return viewMode === 'grid' ? ( + <DataGrid {...props} cardHeaderSlots={cardHeaderSlots} columnsCount={columnsCount} /> + ) : ( + <DataList {...props} /> + );packages/framework/src/modules/cms/models/blocks/notification-list.model.ts (1)
11-11: Confirm whetherinitialFiltersshould also supportviewMode
filtersis now typed withviewMode?: 'list' | 'grid', butinitialFiltersremainsPartial<Notification & { sort?: string }>. If the view mode toggle is meant to participate in persisted defaults / reset behaviour, consider extendinginitialFiltersto includeviewModeas well for consistency with the filters shape and with other list block models; otherwise, it would be good to document thatviewModeis intentionally client-only and excluded from initial filters.Also applies to: 23-28
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
40-47: KeepviewModein sync with filter reset semanticsThe view mode is initialized from the
FilterViewModeToggleand then driven via local state and the FiltersSection mapping, which is good. However,handleResetonly resetsfilters/data, leavingviewModeunchanged, so “Reset filters” won’t restore the initial list/grid mode.Consider resetting
viewModealongside filters, e.g.:const handleReset = () => { startTransition(async () => { const newData = await sdk.blocks.getTicketList(initialFilters, { 'x-locale': locale }, accessToken); setFilters(initialFilters); setData(newData); + setViewMode(initialViewMode); }); };This keeps the toggle, filters, and rendered
DataViewaligned after a reset.Also applies to: 59-65, 157-173
packages/blocks/notification-list/src/frontend/NotificationList.client.tsx (1)
43-49: AlignviewModewith filter reset and FiltersSection behaviourThe view mode is nicely initialized from
FilterViewModeToggleand wired intoFiltersSectionso the toggle reflects current state. However,handleResetonly resets filters/data, leavingviewModeunchanged, so “Reset filters” won’t restore the initial list/grid choice.Consider resetting
viewModeas well:const handleReset = () => { startTransition(async () => { const newData = await sdk.blocks.getNotificationList(initialFilters, { 'x-locale': locale }, accessToken); setFilters(initialFilters); setData(newData); + setViewMode(initialViewMode); }); };This keeps the toggle, filters, and rendered
DataViewconsistent after resets.Also applies to: 62-69, 144-160, 169-175
packages/ui/src/components/DataGrid/DataGrid.stories.tsx (1)
160-178: Drop unnecessaryasChildusage in action storiesIn
WithActions,CustomColumnCount, andWithTopSlot,Buttonis rendered withasChildbut without a link/alternate root element—just icon + text children.asChildisn’t buying anything here and can be confusing given the design‑system pattern.You can simplify to a regular button:
- actions: { - title: '', - render: (_item) => ( - <Button asChild variant="link" className="flex items-center gap-2"> - <ArrowRight className="h-4 w-4" /> - View Details - </Button> - ), - }, + actions: { + title: '', + render: (_item) => ( + <Button variant="link" className="flex items-center gap-2"> + <ArrowRight className="h-4 w-4" /> + View Details + </Button> + ), + },Apply the same pattern to the other stories using
asChildwithout a link.Also applies to: 181-205, 207-225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
packages/blocks/invoice-list/src/api-harmonization/invoice-list.mapper.ts(1 hunks)packages/blocks/invoice-list/src/api-harmonization/invoice-list.model.ts(1 hunks)packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx(4 hunks)packages/blocks/notification-list/src/api-harmonization/notification-list.mapper.ts(1 hunks)packages/blocks/notification-list/src/api-harmonization/notification-list.model.ts(1 hunks)packages/blocks/notification-list/src/frontend/NotificationList.client.tsx(4 hunks)packages/blocks/order-list/src/api-harmonization/order-list.mapper.ts(1 hunks)packages/blocks/order-list/src/api-harmonization/order-list.model.ts(1 hunks)packages/blocks/order-list/src/frontend/OrderList.client.tsx(5 hunks)packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts(1 hunks)packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts(1 hunks)packages/blocks/ticket-list/src/frontend/TicketList.client.stories.tsx(2 hunks)packages/blocks/ticket-list/src/frontend/TicketList.client.tsx(5 hunks)packages/framework/src/modules/cms/models/blocks/invoice-list.model.ts(2 hunks)packages/framework/src/modules/cms/models/blocks/notification-list.model.ts(2 hunks)packages/framework/src/modules/cms/models/blocks/order-list.model.ts(2 hunks)packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts(2 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts(6 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.notification-list.mapper.ts(6 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts(6 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts(6 hunks)packages/ui/src/components/DataGrid/DataGrid.stories.tsx(1 hunks)packages/ui/src/components/DataGrid/DataGrid.tsx(1 hunks)packages/ui/src/components/DataGrid/DataGrid.types.ts(1 hunks)packages/ui/src/components/DataView/DataView.stories.tsx(1 hunks)packages/ui/src/components/DataView/DataView.tsx(1 hunks)packages/ui/src/components/DataView/DataView.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts
- packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts
- packages/ui/src/components/DataGrid/DataGrid.types.ts
- packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/framework/src/modules/cms/models/blocks/order-list.model.tspackages/framework/src/modules/cms/models/blocks/invoice-list.model.tspackages/framework/src/modules/cms/models/blocks/notification-list.model.tspackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts
🧬 Code graph analysis (8)
packages/framework/src/modules/cms/models/blocks/order-list.model.ts (2)
packages/blocks/order-list/src/api-harmonization/order-list.model.ts (1)
Order(36-59)packages/framework/src/modules/orders/orders.model.ts (1)
Order(24-49)
packages/framework/src/modules/cms/models/blocks/invoice-list.model.ts (1)
packages/blocks/invoice-list/src/api-harmonization/invoice-list.model.ts (1)
Invoice(35-56)
packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (1)
packages/ui/src/components/DataView/DataView.tsx (1)
DataView(9-24)
packages/framework/src/modules/cms/models/blocks/notification-list.model.ts (1)
packages/blocks/notification-list/src/api-harmonization/notification-list.model.ts (1)
Notification(31-49)
packages/ui/src/components/DataView/DataView.stories.tsx (2)
packages/ui/src/components/DataView/DataView.tsx (1)
DataView(9-24)packages/ui/src/components/DataView/DataView.types.ts (1)
DataViewProps(3-19)
packages/blocks/notification-list/src/frontend/NotificationList.client.tsx (3)
packages/blocks/notification-list/src/api-harmonization/notification-list.model.ts (1)
NotificationListBlock(7-29)packages/framework/src/modules/cms/models/blocks/notification-list.model.ts (1)
NotificationListBlock(5-29)packages/ui/src/components/DataView/DataView.tsx (1)
DataView(9-24)
packages/blocks/order-list/src/frontend/OrderList.client.tsx (3)
packages/blocks/order-list/src/api-harmonization/order-list.model.ts (1)
OrderListBlock(7-34)packages/framework/src/modules/cms/models/blocks/order-list.model.ts (1)
OrderListBlock(5-31)packages/ui/src/components/DataView/DataView.tsx (1)
DataView(9-24)
packages/ui/src/components/DataView/DataView.tsx (2)
packages/ui/src/components/DataView/DataView.types.ts (1)
DataViewProps(3-19)packages/ui/src/components/DataGrid/DataGrid.tsx (1)
DataGrid(35-148)
🪛 Biome (2.1.2)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx
[error] 12-12: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx
[error] 13-13: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/ui/src/components/DataView/DataView.stories.tsx
[error] 5-5: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/blocks/notification-list/src/frontend/NotificationList.client.tsx
[error] 12-12: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/blocks/order-list/src/frontend/OrderList.client.tsx
[error] 10-10: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/ui/src/components/DataView/DataView.tsx
[error] 9-9: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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 (26)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.notification-list.mapper.ts (1)
118-122: LGTM! FilterViewModeToggle and cardHeaderSlots added consistently.The addition of FilterViewModeToggle with default value 'list' and cardHeaderSlots configuration is properly structured. The same pattern is correctly applied across all locales (EN, DE, PL).
Also applies to: 135-140
packages/blocks/invoice-list/src/api-harmonization/invoice-list.model.ts (1)
27-32: LGTM! cardHeaderSlots property added correctly.The optional cardHeaderSlots property is properly typed and maintains backward compatibility. The structure aligns with similar additions in other blocks (NotificationListBlock, OrderListBlock, TicketListBlock).
packages/blocks/notification-list/src/api-harmonization/notification-list.model.ts (1)
23-28: LGTM! cardHeaderSlots property matches the pattern.The implementation is consistent with other block models and properly typed as an optional property.
packages/blocks/invoice-list/src/api-harmonization/invoice-list.mapper.ts (1)
31-31: LGTM! cardHeaderSlots mapping added.The mapper correctly passes through cardHeaderSlots from the CMS source to the InvoiceListBlock model. This aligns with the mocked integration mapper that provides this data.
packages/blocks/ticket-list/src/frontend/TicketList.client.stories.tsx (1)
58-62: LGTM! Story demonstrates grid view mode.The story correctly includes cardHeaderSlots configuration and FilterViewModeToggle. Note that this story defaults to 'grid' mode (line 219), which is appropriate for demonstrating the new tile view feature in Storybook.
Also applies to: 216-220
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts (1)
112-116: LGTM! Consistent implementation across all locales.The FilterViewModeToggle and cardHeaderSlots are correctly added with appropriate defaults. The slot configuration (left: id, right: paymentStatus, bottom: paymentDueDate) is well-suited for invoice display.
Also applies to: 130-134
packages/blocks/order-list/src/api-harmonization/order-list.model.ts (1)
28-33: LGTM! cardHeaderSlots property follows the established pattern.The implementation is consistent with other block models in this PR.
packages/framework/src/modules/cms/models/blocks/invoice-list.model.ts (1)
11-11: FilterViewModeToggle type correctly constrains viewMode values to 'list' | 'grid'.The
FilterViewModeToggleclass (packages/framework/src/utils/models/filters.ts, lines 61-64) properly definesvalue?: 'list' | 'grid', which is consistent with the filters type extension in invoice-list.model.ts. Type safety is maintained across all usages in mappers and components.packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (3)
40-42: LGTM!The initialViewMode extraction safely handles missing filters or FilterViewModeToggle items with appropriate optional chaining and a sensible default.
151-167: LGTM!The filter wiring correctly implements controlled component pattern by injecting the current viewMode value and setViewMode callback into FilterViewModeToggle items.
176-182: LGTM!DataView integration is clean and maintains backward compatibility by forwarding all necessary props. The component will correctly delegate to DataGrid or DataList based on the viewMode state.
packages/ui/src/components/DataView/DataView.tsx (1)
17-21: LGTM!The conditional rendering correctly delegates to DataGrid or DataList, passing appropriate props to each. DataGrid-specific props (cardHeaderSlots, columnsCount) are properly isolated from DataList.
packages/framework/src/modules/cms/models/blocks/order-list.model.ts (2)
11-11: LGTM!The filters type extension correctly adds optional viewMode support while maintaining backward compatibility. This aligns with the FilterViewModeToggle feature introduced across list blocks.
25-30: LGTM!The cardHeaderSlots property correctly defines the slot configuration interface. The optional nature maintains backward compatibility, and the structure aligns with DataGrid's slot-based rendering approach.
packages/ui/src/components/DataView/DataView.types.ts (1)
1-19: LGTM!The DataViewProps interface is well-designed, reusing existing DataList types for consistency and maintaining type safety through generics. The distinction between required (viewMode, data, columns) and optional props (cardHeaderSlots, columnsCount, actions) is appropriate.
packages/blocks/notification-list/src/api-harmonization/notification-list.mapper.ts (1)
28-28: LGTM!The cardHeaderSlots property is correctly forwarded from the CMS configuration, extending the NotificationListBlock public shape consistently with other list blocks.
packages/blocks/order-list/src/api-harmonization/order-list.mapper.ts (1)
30-30: LGTM!The cardHeaderSlots property correctly extends the OrderListBlock shape, maintaining consistency with the notification-list and other block mappers.
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.order-list.mapper.ts (2)
92-96: LGTM!The FilterViewModeToggle additions are consistent across all three locales (EN, DE, PL), with appropriate __typename discriminator, id, and default value. This provides the necessary mock data for testing the view mode toggle feature.
Also applies to: 207-211, 322-326
111-115: LGTM!The cardHeaderSlots configuration is consistent across all locales and maps to valid column IDs. The slot assignment (ID left, status right, date bottom) provides a logical card layout for grid view.
Also applies to: 226-230, 341-345
packages/blocks/order-list/src/frontend/OrderList.client.tsx (4)
42-44: LGTM!The initialViewMode extraction follows the same safe pattern as in InvoiceList, with appropriate fallback handling.
118-118: LGTM!The conditional cellClassName appropriately adjusts action column styling based on view mode. List mode gets constrained dimensions while grid mode remains flexible, which aligns with the different layout requirements.
154-170: LGTM!The filter wiring correctly implements the controlled component pattern, binding FilterViewModeToggle to the local viewMode state.
179-185: LGTM!DataView integration is complete with all necessary props, maintaining consistency with the InvoiceList implementation while adapting to OrderList-specific needs.
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
97-111: Row action rendering looks correct and accessibleWrapping
LinkComponentwithButton asChildand using a flex container for icon + label is a solid pattern here: it avoids nested interactive elements, keeps semantics as a link per row, and matches the design system usage.packages/ui/src/components/DataGrid/DataGrid.tsx (1)
27-32: DataGrid structure andrenderCellreuse look solidThe default row‑key extractor, slot column resolution (
top/left/right/bottom), and exclusion of slotIds frombodyColumnsread cleanly, and usingcellContent == nullensures falsy but valid values (like0or'') still render. Reusing the sharedrenderCellfor both header slots and body cells should keep DataGrid aligned with DataList behaviour.Also applies to: 65-77, 84-143
packages/blocks/notification-list/src/frontend/NotificationList.client.tsx (1)
12-12: Based on my verification, I found critical issues with the review comment:The review comment is incorrect and based on false premises
Key findings:
No Biome warning exists: The biome.json configuration uses
"recommended": truebut does NOT include any rule for shadowing built-in globals (such asnoShadow). Biome is not flagging theDataViewimport as a problem.False reference to TicketList pattern: The review claims "Same as in TicketList/DataView stories," but TicketList.client.tsx also uses the unaliased import at line 12:
import { DataView } from '@o2s/ui/components/DataView';— not the aliased version suggested.Inconsistency across codebase: All similar files (InvoiceList, OrderList, and the DataView stories) use the unaliased import without issue. There is no established pattern of aliasing.
No actual problem: While shadowing global constructors can theoretically be a code smell, it is not flagged by this project's Biome configuration, and the component works correctly.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/ui/src/components/DataGrid/DataGrid.stories.tsx (1)
150-151: Consider a helper to reduce type casting verbosity.All stories repeat the same type casting pattern from
Ticket[]toRecord<string, unknown>[]and column types. This is verbose but may be necessary for DataGrid's generic typing.If refactoring, consider extracting a helper:
const toDataGridArgs = ( data: Ticket[], columns: DataListColumnConfig<Ticket>[], overrides: Partial<DataGridProps<Record<string, unknown>>> = {} ): DataGridProps<Record<string, unknown>> => ({ data: data as Record<string, unknown>[], columns: columns as DataListColumnConfig<Record<string, unknown>>[], ...overrides, });Then stories would be cleaner:
export const BasicGrid: Story = { args: toDataGridArgs(sampleTickets, ticketColumns, { cardHeaderSlots: { left: 'id', right: 'status', bottom: 'updatedAt', }, }), };Also applies to: 162-163, 183-184, 209-210, 231-232, 238-239
packages/blocks/order-list/src/frontend/OrderList.client.tsx (1)
10-10: Consider the naming collision with the global DataView.The imported
DataViewcomponent shadows the built-in JavaScriptDataViewglobal (used for binary data manipulation). While unlikely to cause issues in this React component context, it violates the principle of not shadowing restricted names.If this becomes a concern, the component could be renamed in the upstream UI library (e.g.,
DataViewComponent,GridListView) or imported with an alias:-import { DataView } from '@o2s/ui/components/DataView'; +import { DataView as DataViewComponent } from '@o2s/ui/components/DataView';Then update references throughout the file accordingly.
packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (2)
13-13: Consider the naming conflict with global DataView (optional).The imported
DataViewcomponent shadows JavaScript's built-inDataViewglobal (used for ArrayBuffer operations). While unlikely to cause issues in this React component, you might consider using a more specific name likeDataViewComponentorListDataViewif you prefer to avoid the shadowing.As per static analysis hints: "Do not shadow the global 'DataView' property."
151-167: The onChange signature and filter transformation are type-safe; memoization is optional.The
onChangesignature onFilterViewModeToggleis(value: 'list' | 'grid') => void(line 65 inpackages/framework/src/utils/models/filters.ts), which matchessetViewModeexactly. The filter transformation at lines 151-167 is correct.The suggested
useMemooptimization is optional, as the current implementation functions correctly. Consider it only if profiling shows unnecessary re-renders or performance concerns arise.packages/blocks/notification-list/src/frontend/NotificationList.client.tsx (2)
43-46: InlineinitialViewModewith??to tighten types and simplify state initCurrent code is fine at runtime, but
|| 'list'plus optional chaining can widen the type and fight stricter TS settings. Inlining intouseStatewith??keeps the type to'list' | 'grid'and removes the extra variable.- // Extract initial viewMode from filters if available - const initialViewMode = - component.filters?.items?.find((item) => item.__typename === 'FilterViewModeToggle')?.value || 'list'; - - const [viewMode, setViewMode] = useState<'list' | 'grid'>(initialViewMode); + // Extract initial viewMode from filters if available + const [viewMode, setViewMode] = useState<'list' | 'grid'>( + component.filters?.items?.find((item) => item.__typename === 'FilterViewModeToggle')?.value ?? 'list', + );Also applies to: 49-49
144-160: Confirm UX: Reset currently does not resetviewModeThe FilterViewModeToggle wiring (
value: viewMode,onChange: setViewMode) looks consistent, and view mode is intentionally purely client‑side. However,handleResetonly resets backend filters/data, notviewMode, so pressing Reset will keep whatever view (list/grid) the user last chose.If the product expectation is that Reset should also revert the view to the server‑defined default, you may want to call
setViewMode('list')(or the derived initial value) in the reset path; otherwise this behavior is fine as‑is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx(4 hunks)packages/blocks/notification-list/src/frontend/NotificationList.client.tsx(4 hunks)packages/blocks/order-list/src/frontend/OrderList.client.tsx(5 hunks)packages/ui/src/components/DataGrid/DataGrid.stories.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (1)
packages/ui/src/components/DataView/DataView.tsx (1)
DataView(9-24)
🪛 Biome (2.1.2)
packages/blocks/order-list/src/frontend/OrderList.client.tsx
[error] 10-10: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx
[error] 13-13: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/blocks/notification-list/src/frontend/NotificationList.client.tsx
[error] 12-12: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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 (11)
packages/ui/src/components/DataGrid/DataGrid.stories.tsx (5)
1-21: LGTM!The imports and Storybook meta configuration are well-structured. Good reuse of
DataListColumnConfigfrom DataList, which aligns with the shared rendering strategy mentioned in the PR.
23-35: LGTM!The Ticket type is well-structured with a consistent label/value pattern for all data fields, which provides flexibility for display and internal representation.
37-86: LGTM!The sample data is comprehensive, realistic, and provides good variety for testing different scenarios. Status values align correctly with the badge variant function.
88-146: LGTM!The column configuration is well-defined with appropriate types and a sensible badge variant mapping for status values. Column IDs correctly match the Ticket type properties.
249-249: The Button component does support the "tertiary" variant—this concern is unfounded.The
tertiaryvariant is properly defined inpackages/ui/src/lib/utils.ts(line 19–20) as part of thebaseVariantconfiguration. It includes complete styling with tertiary-specific colors and hover states. The usage at line 249 is valid and will not cause any runtime errors.packages/blocks/order-list/src/frontend/OrderList.client.tsx (3)
42-45: LGTM! Initial view mode extraction is well-handled.The logic safely extracts the view mode from filters with appropriate fallback to 'list'. The optional chaining prevents runtime errors if filters or items are undefined.
180-186: Perfect! I now have all the information needed to verify the review comment. The investigation confirms that:
cardHeaderSlotsis properly typed as optional inDataViewProps<T>interface with the?modifier- The component safely passes it through to
DataGridwhen in grid mode- All nested properties are also optional, providing full flexibility
- Type safety is enforced by TypeScript at compile time
The original review comment's approval is valid and the concern about undefined handling is already addressed by the component's design.
LGTM! DataView integration is well-implemented with proper TypeScript support for optional props.
The migration from DataList to DataView is clean and maintains existing props while adding
viewModeandcardHeaderSlots. ThecardHeaderSlotsprop is properly typed as optional in theDataViewPropsinterface, soDataViewgracefully handles undefined values—no runtime issues expected.
155-171: Based on my verification, I can now provide the rewritten review comment:
Type safety is verified; persistence asymmetry is intentional.
The
FilterViewModeToggle.onChangesignature is(value: 'list' | 'grid') => void, which perfectly matchessetViewMode. View mode changes update local state only (not persisted viahandleFilterlike other filters), but this is intentional: view mode is a client-side UI preference affecting layout only, not a server filter criterion. The initial value is sourced from the server-provided filters but subsequent changes remain local.packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (2)
39-42: I'll help you verify the type safety of the initialViewMode value extraction. Let me search for the type definitions and verify the concern.
<function_calls>
#!/bin/bashSearch for FilterViewModeToggle type definition
echo "=== Searching for FilterViewModeToggle type definition ==="
rg -n "FilterViewModeToggle" --type=ts --type=tsx -C5 | head -100echo ""
echo "=== Searching for viewMode type definition ==="
rg -n "viewMode|ViewMode" --type=ts --type=tsx -C3 | head -100
</function_calls>
176-182: Great! Now I have confirmed the components exist. Let me examine the DataView implementation and how it handles keys:
<function_calls>
#!/bin/bashExamine DataView component implementation
echo "=== DataView.tsx content ==="
wc -l packages/ui/src/components/DataView/DataView.tsx
echo ""
cat packages/ui/src/components/DataView/DataView.tsx
</function_calls>Now let me check the DataList and DataGrid implementations:
<function_calls>
#!/bin/bashExamine DataList component
echo "=== DataList.tsx content ==="
wc -l packages/ui/src/components/DataList/DataList.tsx
echo ""
head -100 packages/ui/src/components/DataList/DataList.tsx
</function_calls>Let me check DataGrid as well:
<function_calls>
#!/bin/bashExamine DataGrid component
echo "=== DataGrid.tsx content ==="
wc -l packages/ui/src/components/DataGrid/DataGrid.tsx
echo ""
head -100 packages/ui/src/components/DataGrid/DataGrid.tsx
</function_calls>Let me check the types to understand the prop structure:
<function_calls>
#!/bin/bashCheck DataView types
echo "=== DataView.types.ts ==="
cat packages/ui/src/components/DataView/DataView.types.tsecho -e "\n=== DataList.types.ts ==="
cat packages/ui/src/components/DataList/DataList.types.tsecho -e "\n=== DataGrid.types.ts ==="
cat packages/ui/src/components/DataGrid/DataGrid.types.ts
</function_calls>Now let me check the invoice data structure to see if it has an id field:
<function_calls>
#!/bin/bashCheck invoice model and data structure
echo "=== Looking at invoice model files ==="
echo "=== invoice-list.model.ts ==="
cat packages/framework/src/modules/cms/models/blocks/invoice-list.model.tsecho -e "\n=== InvoiceList.types.ts ==="
cat packages/blocks/invoice-list/src/frontend/InvoiceList.types.ts
</function_calls>Let me also check the mapper to understand the data structure:
<function_calls>
#!/bin/bashCheck invoice-list mapper
echo "=== invoice-list.mapper.ts ==="
cat packages/blocks/invoice-list/src/api-harmonization/invoice-list.mapper.ts
</function_calls>Let me check the entire InvoiceList.client.tsx file to get full context:
<function_calls>
#!/bin/bashRead the full InvoiceList.client.tsx file
echo "=== Full InvoiceList.client.tsx ==="
cat packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx
</function_calls>Let me check how DataList renders items with keys:
<function_calls>
#!/bin/bashSearch for key rendering patterns in DataList and DataGrid
echo "=== Searching for key patterns in DataList ==="
grep -n "key=" packages/ui/src/components/DataList/DataList.tsxecho -e "\n=== Full DataList.tsx ==="
cat packages/ui/src/components/DataList/DataList.tsx
</function_calls>Now let me check DataGrid more thoroughly:
<function_calls>
#!/bin/bashFull DataGrid.tsx
echo "=== Full DataGrid.tsx ==="
cat packages/ui/src/components/DataGrid/DataGrid.tsx
</function_calls>Let me verify by checking the stories to see how these components are used:
<function_calls>
#!/bin/bashCheck DataView stories for usage patterns
echo "=== DataView.stories.tsx ==="
cat packages/ui/src/components/DataView/DataView.stories.tsx
</function_calls>Perfect! Now I have all the information I need. Let me verify one more detail about the prop passing:
<function_calls>
#!/bin/bashCheck if getRowKey or similar is exposed in DataView
echo "=== Checking DataView prop forwarding to DataGrid ==="
grep -n "getRowKey" packages/ui/src/components/DataView/DataView.tsxecho -e "\n=== Checking DataGrid getRowKey usage ==="
grep -n "getRowKey" packages/ui/src/components/DataGrid/DataGrid.tsxecho -e "\n=== Checking DataList item.id usage ==="
grep -n "item.id" packages/ui/src/components/DataList/DataList.tsx
</function_calls>Based on my comprehensive verification of the code, I can now provide the rewritten review comment:
Row keys are properly handled through the underlying DataList/DataGrid components.
The
DataViewintegration is correct. Row key handling is already implemented:
- DataList renders items with explicit
key={item.id}when iterating over the data array- DataGrid uses a
getRowKeyprop to extract row identifiers- Invoice data includes the
idfield required for keyingThe props passed to
DataView(includingviewMode,data,columns,actions, andcardHeaderSlots) are correctly forwarded to the appropriate underlying component (DataList or DataGrid), ensuring no React key warnings will occur.packages/blocks/notification-list/src/frontend/NotificationList.client.tsx (1)
12-12: Rewrite: Review comment is incorrect—no Biome error exists and DataView global is not used in this fileThe
DataViewimport does not shadow any code in use. The globalDataViewWeb API is never referenced in this file (nonew DataView(),typeof DataView, or typed array operations), so shadowing has no practical impact. Biome's recommended rule set does not include anoShadowcheck by default, and no such rule is explicitly enabled inbiome.json. The import is correct as-is; aliasing is unnecessary.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/ui/src/components/DataGrid/DataGrid.tsx (1)
47-57: Dynamic Tailwind class names will be purged; use static breakpoint mappings or a safelist
md:${getGridColsClass(tabletCols)}andlg:${getGridColsClass(desktopCols)}generate Tailwind classes at runtime, so the JIT compiler won’t see them and the responsive grid layout may not work in production.Consider either:
- Building explicit breakpoint-prefixed mappings (e.g.
'md:grid-cols-2','lg:grid-cols-4') and selecting from those, or- Adding all needed
md:grid-cols-{1..6}andlg:grid-cols-{1..6}classes to your Tailwind safelist.For example, fully static mappings could look like:
-const GRID_COLS_MAP: Record<number, string> = { - 1: 'grid-cols-1', - 2: 'grid-cols-2', - 3: 'grid-cols-3', - 4: 'grid-cols-4', - 5: 'grid-cols-5', - 6: 'grid-cols-6', -}; - -const getGridColsClass = (cols: number): string => { - return GRID_COLS_MAP[cols] || 'grid-cols-1'; -}; +const GRID_COLS_MAP: Record<number, string> = { + 1: 'grid-cols-1', + 2: 'grid-cols-2', + 3: 'grid-cols-3', + 4: 'grid-cols-4', + 5: 'grid-cols-5', + 6: 'grid-cols-6', +}; + +const MD_GRID_COLS_MAP: Record<number, string> = { + 1: 'md:grid-cols-1', + 2: 'md:grid-cols-2', + 3: 'md:grid-cols-3', + 4: 'md:grid-cols-4', + 5: 'md:grid-cols-5', + 6: 'md:grid-cols-6', +}; + +const LG_GRID_COLS_MAP: Record<number, string> = { + 1: 'lg:grid-cols-1', + 2: 'lg:grid-cols-2', + 3: 'lg:grid-cols-3', + 4: 'lg:grid-cols-4', + 5: 'lg:grid-cols-5', + 6: 'lg:grid-cols-6', +}; + +const getGridColsClass = (cols: number): string => GRID_COLS_MAP[cols] || 'grid-cols-1'; +const getMdGridColsClass = (cols: number): string => MD_GRID_COLS_MAP[cols] || 'md:grid-cols-2'; +const getLgGridColsClass = (cols: number): string => LG_GRID_COLS_MAP[cols] || 'lg:grid-cols-3'; @@ - const gridClasses = cn( - 'grid gap-4', - getGridColsClass(mobileCols), - `md:${getGridColsClass(tabletCols)}`, - `lg:${getGridColsClass(desktopCols)}`, - className, - ); + const gridClasses = cn( + 'grid gap-4', + getGridColsClass(mobileCols), + getMdGridColsClass(tabletCols), + getLgGridColsClass(desktopCols), + className, + );Alternatively, if you already maintain a Tailwind safelist, adding all
md:grid-cols-*andlg:grid-cols-*variants there is another valid fix.
🧹 Nitpick comments (3)
packages/ui/src/components/DataGrid/DataGrid.tsx (1)
60-77: Header slot resolution andbodyColumnscomputation are correct; consider a small lookup optimizationThe header slot mapping and
bodyColumnsexclusion logic look correct and are nicely computed once per render, not per row. If you see many columns or frequent renders, you could optionally prebuild aMapfromid→ column to avoid multiple linearfindcalls, but it’s not required for correctness here.packages/blocks/order-list/src/frontend/OrderList.client.tsx (2)
10-10: Optional: AliasDataViewimport to satisfy Biome and avoid global name shadowingBiome flags
DataViewas shadowing the globalDataView; if you want a clean lint run, consider aliasing the import:-import { DataView } from '@o2s/ui/components/DataView'; +import { DataView as DataViewComponent } from '@o2s/ui/components/DataView';and update the usage accordingly:
- <DataView + <DataViewComponentThis is purely stylistic; runtime behavior is unchanged.
158-174: FilterViewModeToggle wiring looks correct; clarify whether view mode is purely client-sideMapping
FilterViewModeToggleitems to a controlledvalue(viewMode) plusonChange: setViewModecleanly turns the toggle into local UI state, independent of the server filters object.If the intention is that view mode is just a presentation preference, this is perfect. If instead you also want to persist the user’s choice via filters/query params (e.g. survive full reloads or deep-links), you may eventually want to also propagate
viewModeinto the filter values handled byhandleFilteror whatever persistence mechanism you use elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/blocks/order-list/src/frontend/OrderList.client.tsx(6 hunks)packages/ui/src/components/DataGrid/DataGrid.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/ui/src/components/DataGrid/DataGrid.tsx (3)
packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
DataGridProps(29-74)packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/lib/renderCell.tsx (1)
renderCell(16-100)
packages/blocks/order-list/src/frontend/OrderList.client.tsx (3)
packages/blocks/order-list/src/api-harmonization/order-list.model.ts (1)
OrderListBlock(7-34)packages/framework/src/modules/cms/models/blocks/order-list.model.ts (1)
OrderListBlock(5-31)packages/ui/src/components/DataView/DataView.tsx (1)
DataView(9-24)
🪛 Biome (2.1.2)
packages/blocks/order-list/src/frontend/OrderList.client.tsx
[error] 10-10: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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/ui/src/components/DataGrid/DataGrid.tsx (2)
14-25: Top-levelGRID_COLS_MAP/getGridColsClassextraction looks goodNice job pulling the mapping and helper out of the component; this keeps the render path lighter and avoids recomputing on prop changes.
78-149: Row rendering, header layout, and cell null-guard look solidThe Card-based row rendering is consistent and:
- Uses
getRowKeywith a sensibleid/index fallback.- Applies per-row class names via
getRowClassName.- Renders header slots only when configured.
- Shares
renderCellfor all regions.- Uses
cellContent == nullso falsy but valid values (0,'',false) are rendered whilenull/undefinedare skipped.- Computes
bodyColumnsonce, then reuses it for all rows.This aligns well with the shared DataList/renderCell behavior.
packages/blocks/order-list/src/frontend/OrderList.client.tsx (4)
42-49: InitialviewModederivation is defensive and keeps a sane defaultDeriving
initialViewModefrom theFilterViewModeToggleand falling back to'list'if it's missing or has an unexpected value is safe and keeps behavior predictable across missing/misconfigured filters. The separateviewModestate tied to that initial value is a clean pattern here.
119-123: ConditionalcellClassNamefor actions column matches list vs grid layoutsTightening the actions column width only in list mode (
'py-0 w-[180px]') and leaving it unconstrained for grid (undefined) is a sensible layout distinction and should prevent card layouts from being constrained by table‑style widths.
183-189: DataView integration and cardHeaderSlots propagation are consistentSwitching from direct
DataListusage toDataViewwithviewMode,columns,actions, andcardHeaderSlotspassed through aligns with the shared pattern described in the PR. This should let the same configuration drive both list and grid layouts without duplicating logic.
80-82: Grid view does have navigation affordance via primary action button, but theidcolumn UX difference merits attentionThe concern is partially addressed: while the
idcolumn in grid mode renders as plain text (not a link), the component includes a primary action button in theCardFooter(with an ArrowRight icon) that links toorder.detailsUrlfor every card. However, the navigation pattern is less consistent between list and grid modes—list mode makes theidcell itself clickable, while grid mode relies solely on the footer action button.The primary action button is visible and accessible in grid view, providing the necessary navigation path. Consider whether this consistency difference affects user discoverability or if additional affordances (e.g., clickable card header or card click handler) would improve the UX.
2 participants
What does this PR do?
Related Ticket(s)
Key Changes
DataGridcomponent as a tile/card-based alternative view toDataListfor displaying data in a responsive grid layoutpackages/ui/src/components/DataGrid/with full TypeScript types, Storybook stories, and responsive column configuration (mobile/tablet/desktop)DataListfor consistency, with slot-based layout (top, left, right, bottom) for flexible card content arrangementDataListcomponent to extract cell rendering logic into sharedrenderCellutility functionDataList.tsxtopackages/ui/src/lib/renderCell.tsxto enable code reuse betweenDataListandDataGridFilterViewModeTogglefilter type to enable users to switch between list and grid viewspackages/framework/src/utils/models/filters.tswith newFilterViewModeToggleclassFilterscomponent to detect and render view mode toggle alongside other filter itemsFilterItemcomponent to handle view mode toggle rendering with appropriate UI controlsTicketListblock to demonstrate the new featureTicketList.client.tsxwith initial value extraction from filtersDataListorDataGridbased on selected view modeFilterViewModeToggleto control view mode state and persist selectionSide Effects
DataListusages remain unchanged and continue to work as before (backward compatible)Summary by CodeRabbit
New Features
Enhancements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.