Feature/data table 716#769
Open
PoonamKPatil wants to merge 16 commits intomindfiredigital:developmentfrom
Open
Feature/data table 716#769PoonamKPatil wants to merge 16 commits intomindfiredigital:developmentfrom
PoonamKPatil wants to merge 16 commits intomindfiredigital:developmentfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new generic, feature-rich DataTable (hooks, UI parts, responsive layouts, tests, stories, docs, and registry entry) and refactors contact-form toast handling to a context-based Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DataTable as DataTable<T>
participant Hooks as Hooks\n(useFilter / useSort / usePagination / useColumnVisibility / useRowSelection)
participant UI as UI Components\n(TableHeader/TableRow/Pagination/BulkActionBar)
participant DOM as Rendered Output
User->>DataTable: Provide data, columns, config
DataTable->>Hooks: Initialize (filter → sort → paginate) pipeline
User->>DataTable: Enter search term / toggle column / click header / change page
DataTable->>Hooks: useFilter/useSort/useColumnVisibility/usePagination handle state
Hooks-->>DataTable: Return filtered/sorted/paged data + visible columns
DataTable->>UI: Render desktop table or mobile cards with visible columns
UI->>DOM: Animate rows (Framer Motion), show skeletons or empty/no-results
User->>UI: Select rows, trigger bulk action
UI->>Hooks: useRowSelection updates selection Set
Hooks-->>UI: BulkActionBar shows selected count and actions
User->>DOM: Download CSV / Confirm delete (bulk action)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docs/src/components/Demo/DataTableDemo.tsx`:
- Around line 128-231: The generated sample returned by generateCodeString is
missing imports and has incorrect render/handler code: add imports for Avatar,
TrashIcon, DownloadIcon at top of the snippet; update the Delete BulkAction in
bulkActions to include a required onClick handler (e.g., onClick: (rows) => { /*
delete logic */ }); fix the salary column render in columns (currently returns
the literal "value.toLocaleString()") to call the method on the value (e.g.,
render: (value) => value?.toLocaleString()); ensure joinDate render still
converts value to Date as shown; update the template string in
generateCodeString accordingly so the pasted Code tab compiles without TSX
errors.
In `@apps/docs/src/components/UI/data-table/index.tsx`:
- Around line 225-262: useRowSelection keeps selectedRows across data changes
which causes stale flags; prune and rederive selection on data updates. Add an
effect in useRowSelection that computes currentPageKeys = new
Set(data.map(keyExtractor)) and updates selectedRows to its intersection (remove
keys not in currentPageKeys) so getSelectedRowData() stays accurate; change
isAllSelected and isIndeterminate to compare intersectionSize (or selectedRows
after pruning) against data.length rather than the raw previous size, and update
toggleAll to select from data.map(keyExtractor) (current page keys) so bulk
actions reflect the current data set.
- Around line 323-341: The table header <th> with role="button" is missing
keyboard activation; add an onKeyDown handler on the header (where the current
onClick and role are set) that listens for Enter or Space, prevents default, and
calls the existing onSort(column.key) when column.sortable !== false (or replace
the header content with a semantic <button> to delegate activation to the
browser); update the element that uses column.key, column.sortable and onSort to
ensure keyboard users can trigger sorting.
- Around line 40-53: Add onRowClick?: (row: T) => void to the exported
DataTableProps<T> interface so consumers can type the callback; then thread that
prop through the desktop rendering path by passing onRowClick into the TableRow
component (the same prop already used by the mobile path), and ensure TableRow
invokes the callback on user interaction (e.g., row click/keyboard activation)
so desktop rows behave the same as mobile. Reference: DataTableProps<T>,
onRowClick, TableRow.
- Around line 166-168: Replace the render-phase useMemo that calls
setCurrentPage(1) with a useEffect so the page reset runs as a side effect after
render; specifically, update the hook around setCurrentPage to useEffect(() => {
setCurrentPage(1); }, [data.length]) instead of useMemo, targeting the block
where useMemo currently depends on data.length and calls setCurrentPage.
In
`@apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx`:
- Around line 1521-1525: The API table incorrectly documents props that the
component does not support: remove the `variant` and `mobileBreakpoint` rows
from the props table so it matches the actual DataTable implementation and the
TypeScript type DataTableProps<T>; locate the table section in the DataTable
docs (where the props rows for `variant` and `mobileBreakpoint` appear) and
delete those two entries, or alternatively update DataTableProps<T> and the
component implementation to actually support those props if they were intended
features—ensure the docs and the DataTableProps<T> signature remain in sync
after the change.
- Around line 1568-1571: Update the documentation comments to reflect the actual
hook behavior: for useSort, change the toggleSort comment to indicate it flips
only between "asc ↔ desc" (no "none" state) and reference the useSort hook and
toggleSort symbol; for useFilter, update the comment to state that filtering
searches all provided columns (the columns passed into useFilter) rather than
"all visible columns", referencing the useFilter symbol so readers know which
hook semantics are shipped.
- Around line 1646-1662: Update the example so the TypeScript types and TSX are
valid: make the User interface include the actual fields used by columns and
data (e.g., id, name, email, role) so Column<User>[] matches the data shape,
ensure the columns array keys match those properties, and fix the DataTable
usage by moving props inside the opening tag (e.g., <DataTable data={users}
columns={columns} keyExtractor={row => row.id} />). Adjust the second occurrence
(lines ~1707-1732) the same way to keep both examples copy-paste runnable.
In
`@apps/docs/versioned_docs/version-1.0.3/templates/pages/forms/contact-form.mdx`:
- Around line 198-203: Remove the try/catch around the hook call—do not call
useToast() conditionally; instead either delete the toast usage from this
example or ensure the component is wrapped with a ToastProvider and call const
toast = useToast() at the top level of the component; if a safe wrapper exists,
replace useToast() with useSafeToast() and call it top-level. Ensure the
identifier toast is only assigned via a top-level hook call (useToast or
useSafeToast) and not inside control flow or try/catch.
In `@apps/storybook/src/templates/pages/data-management/index.tsx`:
- Around line 225-262: The selection persists across data changes causing stale
flags; inside useRowSelection prune and reconcile selectedRows whenever data
changes by computing the current page keys (data.map(keyExtractor)) and setting
selectedRows to the intersection of the previous set and those keys (use an
effect or setState callback driven by data and keyExtractor). Also compute
isAllSelected and isIndeterminate from the pruned set vs the current data length
(i.e., use the intersection size rather than selectedRows.size directly) and
ensure getSelectedRowData filters using the pruned set; update dependencies to
include keyExtractor and data so reconciliation runs on paging/filtering.
- Around line 323-341: The sortable header <th> that currently sets role,
tabIndex and onClick should also handle keyboard activation: add an onKeyDown
handler on the same element (where column, onSort, sortKey, sortDirection are
used) that checks column.sortable !== false and if e.key is 'Enter' or ' '
(space) calls e.preventDefault() and invokes onSort(column.key); keep the
existing role and tabIndex so keyboard focus and activation behave per WCAG.
- Around line 40-53: Add an optional onRowClick?: (row: T) => void to the
exported DataTableProps<T> interface (instead of relying on an intersection at
the function level), add the same optional prop to TableRowProps<T>, and thread
that prop through wherever rows are rendered: pass onRowClick into
MobileCardView (already done) and also into TableRow when rendering desktop rows
(look for usages inside the DataTable render function and any map over data to
create <TableRow .../>). Update the DataTable component signature to use
DataTableProps<T> directly so consumers see onRowClick in IntelliSense and
ensure TableRow handles the callback (prop name match) to trigger row clicks.
- Around line 166-168: The code is using useMemo to call setCurrentPage(1) when
data.length changes; replace that with a useEffect to perform the side effect:
locate the useMemo that references setCurrentPage and data.length and change it
to useEffect(() => { setCurrentPage(1); }, [data.length]) so the state reset
runs as a proper effect rather than during render.
In `@packages/registry/registry.json`:
- Around line 1454-1469: The registry entry for "data-table" points main.path to
the wrong template; update the "data-table" entry's main.path value (in the
object named "data-table") to the actual template location
"templates/pages/data-table/index.tsx" so consumers resolve the real file
(ensure you edit the main.path property within the "data-table" entry rather
than other keys).
In `@packages/registry/templates/pages/data-table/data-table.test.tsx`:
- Around line 137-204: Add a regression test in the data-table.test.tsx suite
that verifies selection persists and the bulk-action bar stays in sync when the
page changes: render <DataTable {...defaultProps} data={largeDataset}
defaultPageSize={5}>, select one or more rows on page 1 by clicking their
row-checkbox test ids, assert the bulk-action bar appears and shows the correct
selected count (or that Delete action is present), then navigate to the next
page (use the next page button via getAllByLabelText(/next page/i)[0]) and
assert the bulk-action bar still reflects the actual selected payload (e.g.,
selected count remains the same or bulk actions reflect the selected ids) and
that unselected rows on the new page are not counted; include cleanup assertions
by deselecting or clicking select-all and verifying the bulk-action bar updates
accordingly.
In `@packages/registry/templates/pages/data-table/index.tsx`:
- Around line 323-341: The sortable table header currently sets role and
tabIndex on the TH but lacks keyboard handlers; update the header rendering (the
TH that references column.key, column.sortable, onSort, sortKey and
sortDirection) to be keyboard-operable by either replacing the interactive TH
content with a real <button> inside the TH or by adding an onKeyDown handler
that listens for Enter and Space and calls onSort(column.key) when
column.sortable !== false; ensure aria-sort is preserved and that the handler
ignores keys when column.sortable === false.
- Around line 40-53: DataTableProps<T> currently omits the onRowClick prop and
the desktop row render path (TableRow) never invokes it; add onRowClick: (row:
T) => void | Promise<void> to DataTableProps<T>, thread that prop into the
component where rows are rendered, and call it from the desktop TableRow click
handler (the code that renders TableRow in the desktop branch) the same way the
mobile card view does so row clicks work at runtime and match the public API;
ensure any places that construct row click handlers (e.g., the functions around
the TableRow render and row click utilities between lines ~365–431 and
~1128–1144) receive and pass through the new prop.
- Around line 1147-1160: Selection state in useRowSelection is out-of-sync with
paging/filtering because selectedRows persists across currentData changes;
update selection whenever the visible dataset changes by pruning keys not
present in the current/filtered set. Concretely, when using
useRowSelection(currentData, keyExtractor) (and when
filteredData/sortedData/currentData changes), intersect the internal
selectedRows set with the set of keys derived from the new currentData (via
keyExtractor) and update isAllSelected/isIndeterminate accordingly (or call
clearSelection when appropriate). You can implement this either inside
useRowSelection (watching its data argument) or by adding a useEffect in this
component that derives visibleKeys from currentData and calls a new
pruneSelection/clearSelection API on the hook to remove keys not present.
- Around line 206-216: The toggleColumn function currently allows removing the
last visible column which causes divide-by-zero rendering issues; modify
toggleColumn (and the same logic used at lines ~1284-1288) to guard against
removing the final key: when newSet.has(key) and newSet.size === 1, return prev
(do nothing) instead of deleting, otherwise proceed with add/delete and
setVisibleColumns as before; reference the toggleColumn function and the other
identical block to apply the same size-check guard to keep at least one visible
column.
- Line 2: The pagination reset is currently performed by calling
setCurrentPage(1) inside the useMemo that computes pagination, which mutates
state during render and can cause re-renders; move this reset into a useEffect
that watches data.length (or the data array length source) and calls
setCurrentPage(1) when it changes. Locate the useMemo that references
pagination/currentPage and replace the in-render setCurrentPage call with a new
useEffect tied to data.length so state updates happen after render (keep
setCurrentPage and the pagination logic intact, only relocate the reset to the
effect).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 204c6356-5cf8-461d-9399-c5556839f41a
📒 Files selected for processing (10)
apps/docs/src/components/Demo/DataTableDemo.tsxapps/docs/src/components/UI/data-table/index.tsxapps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdxapps/docs/versioned_docs/version-1.0.3/templates/pages/forms/contact-form.mdxapps/docs/versioned_sidebars/version-1.0.3-sidebars.jsonapps/storybook/src/templates/pages/data-management/data-table.stories.tsxapps/storybook/src/templates/pages/data-management/index.tsxpackages/registry/registry.jsonpackages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/data-table/index.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
packages/registry/registry.json
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Register components in
packages/registry/components/directory by creating a component subdirectory and adding an entry topackages/registry/registry.json
Files:
packages/registry/registry.json
**/*.tsx
📄 CodeRabbit inference engine (README.md)
**/*.tsx: Component files must follow the naming pattern: componentName.tsx for the main component file
Integrate seamlessly with Tailwind CSS using smart class merging that respects custom styles
Files:
apps/docs/src/components/Demo/DataTableDemo.tsxpackages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/data-table/index.tsxapps/storybook/src/templates/pages/data-management/data-table.stories.tsxapps/storybook/src/templates/pages/data-management/index.tsxapps/docs/src/components/UI/data-table/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Build components with TypeScript for type-safety and rich IntelliSense support
Files:
apps/docs/src/components/Demo/DataTableDemo.tsxpackages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/data-table/index.tsxapps/storybook/src/templates/pages/data-management/data-table.stories.tsxapps/storybook/src/templates/pages/data-management/index.tsxapps/docs/src/components/UI/data-table/index.tsx
**/*.test.tsx
📄 CodeRabbit inference engine (README.md)
Component test files must follow the naming pattern: componentName.test.tsx for unit tests
Files:
packages/registry/templates/pages/data-table/data-table.test.tsx
**/index.tsx
📄 CodeRabbit inference engine (README.md)
Each component must have an index.tsx file for proper module exports
Files:
packages/registry/templates/pages/data-table/index.tsxapps/storybook/src/templates/pages/data-management/index.tsxapps/docs/src/components/UI/data-table/index.tsx
🧠 Learnings (9)
📚 Learning: 2026-04-02T07:32:04.251Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: README.md:0-0
Timestamp: 2026-04-02T07:32:04.251Z
Learning: Applies to registry.json : registry.json must be updated when new components are added to the project
Applied to files:
packages/registry/registry.json
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to packages/registry/registry.json : Register components in `packages/registry/components/` directory by creating a component subdirectory and adding an entry to `packages/registry/registry.json`
Applied to files:
packages/registry/registry.json
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/docs/docs/components/**/*.mdx : Add MDX documentation files for components in `apps/docs/docs/components/` and link them in `apps/docs/sidebars.ts`
Applied to files:
packages/registry/registry.jsonapps/docs/versioned_docs/version-1.0.3/templates/pages/forms/contact-form.mdxapps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdxapps/docs/src/components/UI/data-table/index.tsx
📚 Learning: 2026-03-31T07:07:47.374Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: packages/registry/templates/pages/contact-form/index.tsx:143-148
Timestamp: 2026-03-31T07:07:47.374Z
Learning: In `packages/registry/templates/pages/contact-form/index.tsx` (React/TypeScript), the `useToast()` call is deliberately wrapped in try/catch to make toast notifications optional — the ContactForm is designed to work without a ToastProvider. The correct fix is a `useSafeToast` wrapper that calls `React.useContext(ToastContext)` directly and returns null when the context is absent, rather than throwing.
Applied to files:
apps/docs/versioned_docs/version-1.0.3/templates/pages/forms/contact-form.mdx
📚 Learning: 2026-03-31T05:22:49.923Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/versioned_docs/version-1.0.3/templates/pages/forms/contact-form.mdx:29-31
Timestamp: 2026-03-31T05:22:49.923Z
Learning: In mindfiredigital/ignix-ui documentation under apps/docs/versioned_docs, the CLI command `ignix add component <name>` is an intentional unified command that imports both components and templates. When this command appears in template-related docs/examples (e.g., template documentation snippets), do not flag it as incorrect or misleading; it is the expected usage.
Applied to files:
apps/docs/versioned_docs/version-1.0.3/templates/pages/forms/contact-form.mdxapps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/storybook/src/components/**/*.{test,spec}.{tsx,ts} : Write unit tests for new components using Vitest, located alongside the component source code
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/storybook/src/components/**/*.stories.{tsx,ts} : Create a Story File named `[component-name].stories.tsx` in the same directory as the component file with argTypes, controls, and example stories
Applied to files:
apps/storybook/src/templates/pages/data-management/data-table.stories.tsx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/storybook/src/components/**/*.{tsx,ts,jsx,js} : Create Component Files in `apps/storybook/src/components/` directory with a component subdirectory for each component
Applied to files:
apps/storybook/src/templates/pages/data-management/data-table.stories.tsx
📚 Learning: 2026-03-31T05:21:58.748Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/src/components/UI/contact-form/index.tsx:15-426
Timestamp: 2026-03-31T05:21:58.748Z
Learning: In this repository, it is intentional for UI components to be duplicated as self-contained copies across the docs, storybook, and registry template implementations. Do not raise refactor/DRY duplication review findings when the same component appears in these separate locations, since each copy is maintained independently for its respective app/template.
Applied to files:
apps/docs/src/components/UI/data-table/index.tsx
🔇 Additional comments (2)
apps/docs/src/components/UI/data-table/index.tsx (1)
24-53: Clean typed surface area.Nice split of
Column<T>,BulkAction<T>, andDataTableProps<T>—it keeps the table API discoverable and easy to consume from docs and stories.apps/storybook/src/templates/pages/data-management/index.tsx (1)
24-53: Clean typed surface area.Nice split of
Column<T>,BulkAction<T>, andDataTableProps<T>—it keeps the table API discoverable and easy to consume from stories.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
packages/registry/templates/pages/data-table/index.tsx (2)
398-417:⚠️ Potential issue | 🟠 MajorDesktop rows still ignore
onRowClick.The mobile card path uses this callback, but
TableRownever accepts or invokes it andrenderDesktopView()doesn't pass it through. WireonRowClickintoTableRow, add row keyboard activation, and stop checkbox clicks from bubbling into the row handler so the desktop path matches the public API.Also applies to: 420-463, 1406-1417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/templates/pages/data-table/index.tsx` around lines 398 - 417, Add an onRowClick prop to the TableRow interface and function signature, propagate it into renderDesktopView and into any desktop row element, and ensure the row element calls onRowClick(row) on click and on keyboard activation (Enter/Space) to support accessibility; also ensure the checkbox/input used by enableRowSelection and onToggleSelect calls event.stopPropagation() so clicks on the checkbox don't bubble up to the row handler; update any other desktop-rendering helpers referenced by renderDesktopView to accept and forward onRowClick (also apply same changes to the other occurrences noted).
238-289:⚠️ Potential issue | 🟠 MajorFinish deriving selection state from the current page.
The prune effect removes stale keys after render, but
toggleAll, the header checkbox flags, andselectedCount={selectedRows.size}still use the raw set size. On the first render after paging/filtering, the bulk bar can still report rows thatgetSelectedRowData()no longer returns, andtoggleAllcan clear the page when the old and new page happen to have the same selection count. Compute those values fromselectedRows ∩ currentKeys(orgetSelectedRowData().length) instead.Also applies to: 1477-1480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/templates/pages/data-table/index.tsx` around lines 238 - 289, The selection-derived UI (header checkbox, selectedCount, toggleAll) still uses raw selectedRows size instead of the pruned/current-page selection; update derived values and toggleAll to operate on selectedRows ∩ currentKeys (or use getSelectedRowData().length). Specifically: compute a derivedSelectedKeys = new Set([...selectedRows].filter(k => currentKeys.has(k))) and use its size for isAllSelected/isIndeterminate/selectedCount; change toggleAll to only add/remove keys from currentKeys (when all current page keys are selected remove them from selectedRows, otherwise add them) while preserving selections on other pages; keep the existing prune useEffect and reuse keyExtractor/currentKeys in these updates (refer to toggleAll, isAllSelected, isIndeterminate, getSelectedRowData, and the prune useEffect).apps/docs/src/components/UI/data-table/index.tsx (2)
238-289:⚠️ Potential issue | 🟠 MajorFinish deriving selection state from the current page.
The prune effect removes stale keys after render, but
toggleAll, the header checkbox flags, andselectedCount={selectedRows.size}still use the raw set size. On the first render after paging/filtering, the bulk bar can still report rows thatgetSelectedRowData()no longer returns, andtoggleAllcan clear the page when the old and new page happen to have the same selection count. Compute those values fromselectedRows ∩ currentKeys(orgetSelectedRowData().length) instead.Also applies to: 1477-1480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/src/components/UI/data-table/index.tsx` around lines 238 - 289, The selection state should be derived against the current page keys instead of using raw selectedRows size; update toggleAll, the header checkbox booleans (isAllSelected, isIndeterminate) and any displayed selectedCount to compute from selectedRows ∩ currentKeys (or use getSelectedRowData().length) so they reflect pruned/stale keys; in particular change toggleAll (which currently builds keys = data.map(keyExtractor) and compares prev.size) to compare prev intersection size with keys.length and to set selection based on keys (new Set(keys) or removing only those keys), and compute isAllSelected/isIndeterminate/selectedCount from the intersection between selectedRows and currentKeys (or getSelectedRowData()) instead of selectedRows.size.
398-417:⚠️ Potential issue | 🟠 MajorDesktop rows still ignore
onRowClick.The mobile card path uses this callback, but
TableRownever accepts or invokes it andrenderDesktopView()doesn't pass it through. WireonRowClickintoTableRow, add row keyboard activation, and stop checkbox clicks from bubbling into the row handler so the desktop path matches the public API.Also applies to: 420-463, 1406-1417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/src/components/UI/data-table/index.tsx` around lines 398 - 417, TableRow currently lacks the onRowClick prop and desktop rendering doesn't wire it or keyboard activation; update the TableRowProps interface to include onRowClick?: (row: T) => void and accept it in the TableRow component signature, then pass onRowClick into renderDesktopView wherever it's invoked; inside renderDesktopView attach an onClick to the row container that calls onRowClick(row) (guarded), add an onKeyDown handler that triggers onRowClick for Enter/Space (preventDefault) to support keyboard activation, and ensure the row selection checkbox's click handler calls event.stopPropagation() so checkbox interactions don't bubble into the row click handler—use symbols TableRow, TableRowProps, renderDesktopView, onToggleSelect, enableRowSelection, keyExtractor, and onRowClick to locate spots to change.apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx (2)
1643-1669:⚠️ Potential issue | 🟠 MajorThe example blocks still aren't standalone-runnable.
The basic snippet still uses invalid TSX (
<DataTable>followed by props on separate lines), and the bulk-actions snippet still referencescolumns,handleDelete, andexportToCsvwithout defining them. Make each block self-contained before publishing.Also applies to: 1711-1746
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx` around lines 1643 - 1669, The example blocks are not self-contained and contain invalid TSX and undefined symbols; update each block so the JSX is valid (use a single opening tag with props like <DataTable data={users} columns={columns} keyExtractor={...} /> or wrap props inside an opening tag with a closing tag) and declare any referenced types/values/functions (ensure interface User, const users, const columns, and implementations for keyExtractor, handleDelete, and exportToCsv are included in the same snippet); specifically fix the snippets that reference DataTable, Column, User, users, columns, keyExtractor, handleDelete, and exportToCsv so they compile on their own.
1521-1524:⚠️ Potential issue | 🟠 MajorRemove
mobileBreakpointfrom the props table.The component switches layouts with Tailwind's fixed
smbreakpoint;DataTableProps<T>doesn't expose a configurable breakpoint. Leaving this row here documents a prop TypeScript rejects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx` around lines 1521 - 1524, The docs list a non-existent prop mobileBreakpoint which is not part of DataTableProps<T>; remove the mobileBreakpoint row from the props table in the DataTable docs (and any adjacent descriptive text referencing mobileBreakpoint) so the table matches the actual component API (search for "mobileBreakpoint" and update any references near the DataTableProps table or the DataTable component docs to avoid leaving stale documentation).apps/docs/src/components/Demo/DataTableDemo.tsx (1)
127-247:⚠️ Potential issue | 🟠 MajorThe Code tab still references an undefined dataset.
generateCodeString()rendersdata={employees}, but the snippet never declaresemployees. Reuse the existing sample dataset inside the string (or rename the prop) so the generated TSX is actually copy-paste runnable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/src/components/Demo/DataTableDemo.tsx` around lines 127 - 247, The generated code string from generateCodeString includes data={employees} but never declares employees, making the snippet non-runnable; update generateCodeString to either (A) inline a sample dataset declaration inside the template string (declare const employees = [...] above the <DataTable .../> JSX) or (B) change the prop to a named dataset that you do declare in the template (e.g., data={sampleData} and include const sampleData = [...] in the same template). Locate generateCodeString in DataTableDemo.tsx and modify the template literal so the sample dataset (matching the Employee interface used) is defined within the returned string before the <DataTable /> usage. Ensure the sample array includes the fields id, name, email, department, role, status, salary, joinDate so copy-pasting the TSX runs without errors.apps/storybook/src/templates/pages/data-management/index.tsx (1)
398-417:⚠️ Potential issue | 🟠 MajorThread
onRowClickthroughTableRowfor desktop parity.
onRowClickwas added toDataTableProps<T>and works inMobileCardView, butTableRowProps<T>and theTableRowcomponent don't receive or handle it. Desktop users can't click rows while mobile users can.Add
onRowClicktoTableRowProps<T>and invoke it from withinTableRow:🔧 Proposed fix
interface TableRowProps<T> { row: T; columns: Column<T>[]; index: number; isSelected: boolean; onToggleSelect: (row: T) => void; enableRowSelection?: boolean; keyExtractor: (row: T) => string | number; theme?: 'light' | 'dark'; + onRowClick?: (row: T) => void; } export function TableRow<T>({ row, columns, index, isSelected, onToggleSelect, enableRowSelection, keyExtractor, theme = 'light', + onRowClick, }: TableRowProps<T>) { return ( <motion.tr initial={{ opacity: 0, y: -10 }} animate={{ opacity: 1, y: 0 }} exit={{ opacity: 0, y: 10 }} transition={{ duration: 0.2, delay: index * 0.02 }} + onClick={() => onRowClick?.(row)} + style={{ cursor: onRowClick ? 'pointer' : undefined }} className={cn(Then pass it in
renderDesktopView:<TableRow key={keyExtractor(row)} row={row} columns={visibleColumnsList} index={idx} isSelected={selectedRows.has(keyExtractor(row))} onToggleSelect={toggleRow} enableRowSelection={enableRowSelection} keyExtractor={keyExtractor} theme={theme} + onRowClick={onRowClick} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/storybook/src/templates/pages/data-management/index.tsx` around lines 398 - 417, TableRowProps<T> and the TableRow component are missing the onRowClick prop so desktop rows don't respond like MobileCardView; add onRowClick?: (row: T) => void to TableRowProps<T>, accept onRowClick in the TableRow<T> parameter list, and invoke it inside TableRow when a row is clicked (e.g., in the row container's onClick handler), and ensure renderDesktopView passes the DataTableProps.onRowClick through to each TableRow so desktop behavior matches MobileCardView.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx`:
- Around line 27-28: The code currently calls setCurrentPage(1) inside a useMemo
(mutating state during render) — update the React imports to include useEffect
(add useEffect to import in the top-level import line) and replace the
useMemo(() => setCurrentPage(1), [data.length]) usage with a useEffect that
calls setCurrentPage(1) when data.length changes (i.e. useEffect(() => {
setCurrentPage(1); }, [data.length])); apply the same replacement for the other
analogous occurrence that resets pagination (the second useMemo that calls
setCurrentPage) so both useEffect-based resets reference setCurrentPage and
depend on data.length.
In `@apps/storybook/src/templates/pages/data-management/index.tsx`:
- Around line 664-676: The Next button can be enabled when totalPages is 0;
update its disabled logic in the button that uses onNextPage so it also disables
when totalPages is <= 1 (e.g. disabled={currentPage === totalPages || totalPages
<= 1}) — you can also include !hasData if you prefer (disabled={currentPage ===
totalPages || totalPages <= 1 || !hasData}); change the condition where the
ChevronRight button is rendered (the element using onNextPage, currentPage and
totalPages) so the button is inert when there are no pages.
- Around line 495-545: The exit animation never runs because Radix unmounts the
portal before AnimatePresence can animate; change to a controlled menu and keep
the content mounted so Framer Motion can handle exit: make DropdownMenu.Root
controlled using the open and onOpenChange props (e.g., track an "open" state),
add forceMount to DropdownMenu.Content so Radix doesn't unmount it, and render
the motion.div inside an AnimatePresence that conditionally renders based on the
controlled open state (while keeping the motion.div's exit prop intact).
Reference: DropdownMenu.Root (open, onOpenChange), DropdownMenu.Content
(forceMount), AnimatePresence, and the motion.div exit animation.
In `@packages/registry/templates/pages/data-table/data-table.test.tsx`:
- Around line 25-35: The test mocks the wrong module so the fake Checkbox never
applies; update the mock specifier in the test (the vi.mock call that currently
targets '@ignix-ui/checkbox') to match the actual import used by DataTable by
replacing it with "../../../components/checkbox" so the mocked Checkbox
component is used and getAllByTestId('checkbox') becomes stable.
In `@packages/registry/templates/pages/data-table/index.tsx`:
- Around line 1209-1222: The current logic treats an explicit empty bulkActions
array as falsy and injects placeholder defaults (the defaultBulkActions array)
which show destructive controls with no real handlers; change the logic so that
only when bulkActions is undefined or null do you provide the fallback defaults,
otherwise respect an explicit [] as “no bulk actions” and render nothing; update
the selection bar rendering to check the final actions array length > 0 before
showing the bar and ensure the placeholders (the injected 'Delete'/'Export'
actions that reference handleDeleteSelected and the export noop) are removed
unless real actions are supplied; apply the same fix to the analogous block that
creates default bulk actions elsewhere in this file.
---
Duplicate comments:
In `@apps/docs/src/components/Demo/DataTableDemo.tsx`:
- Around line 127-247: The generated code string from generateCodeString
includes data={employees} but never declares employees, making the snippet
non-runnable; update generateCodeString to either (A) inline a sample dataset
declaration inside the template string (declare const employees = [...] above
the <DataTable .../> JSX) or (B) change the prop to a named dataset that you do
declare in the template (e.g., data={sampleData} and include const sampleData =
[...] in the same template). Locate generateCodeString in DataTableDemo.tsx and
modify the template literal so the sample dataset (matching the Employee
interface used) is defined within the returned string before the <DataTable />
usage. Ensure the sample array includes the fields id, name, email, department,
role, status, salary, joinDate so copy-pasting the TSX runs without errors.
In `@apps/docs/src/components/UI/data-table/index.tsx`:
- Around line 238-289: The selection state should be derived against the current
page keys instead of using raw selectedRows size; update toggleAll, the header
checkbox booleans (isAllSelected, isIndeterminate) and any displayed
selectedCount to compute from selectedRows ∩ currentKeys (or use
getSelectedRowData().length) so they reflect pruned/stale keys; in particular
change toggleAll (which currently builds keys = data.map(keyExtractor) and
compares prev.size) to compare prev intersection size with keys.length and to
set selection based on keys (new Set(keys) or removing only those keys), and
compute isAllSelected/isIndeterminate/selectedCount from the intersection
between selectedRows and currentKeys (or getSelectedRowData()) instead of
selectedRows.size.
- Around line 398-417: TableRow currently lacks the onRowClick prop and desktop
rendering doesn't wire it or keyboard activation; update the TableRowProps
interface to include onRowClick?: (row: T) => void and accept it in the TableRow
component signature, then pass onRowClick into renderDesktopView wherever it's
invoked; inside renderDesktopView attach an onClick to the row container that
calls onRowClick(row) (guarded), add an onKeyDown handler that triggers
onRowClick for Enter/Space (preventDefault) to support keyboard activation, and
ensure the row selection checkbox's click handler calls event.stopPropagation()
so checkbox interactions don't bubble into the row click handler—use symbols
TableRow, TableRowProps, renderDesktopView, onToggleSelect, enableRowSelection,
keyExtractor, and onRowClick to locate spots to change.
In
`@apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx`:
- Around line 1643-1669: The example blocks are not self-contained and contain
invalid TSX and undefined symbols; update each block so the JSX is valid (use a
single opening tag with props like <DataTable data={users} columns={columns}
keyExtractor={...} /> or wrap props inside an opening tag with a closing tag)
and declare any referenced types/values/functions (ensure interface User, const
users, const columns, and implementations for keyExtractor, handleDelete, and
exportToCsv are included in the same snippet); specifically fix the snippets
that reference DataTable, Column, User, users, columns, keyExtractor,
handleDelete, and exportToCsv so they compile on their own.
- Around line 1521-1524: The docs list a non-existent prop mobileBreakpoint
which is not part of DataTableProps<T>; remove the mobileBreakpoint row from the
props table in the DataTable docs (and any adjacent descriptive text referencing
mobileBreakpoint) so the table matches the actual component API (search for
"mobileBreakpoint" and update any references near the DataTableProps table or
the DataTable component docs to avoid leaving stale documentation).
In `@apps/storybook/src/templates/pages/data-management/index.tsx`:
- Around line 398-417: TableRowProps<T> and the TableRow component are missing
the onRowClick prop so desktop rows don't respond like MobileCardView; add
onRowClick?: (row: T) => void to TableRowProps<T>, accept onRowClick in the
TableRow<T> parameter list, and invoke it inside TableRow when a row is clicked
(e.g., in the row container's onClick handler), and ensure renderDesktopView
passes the DataTableProps.onRowClick through to each TableRow so desktop
behavior matches MobileCardView.
In `@packages/registry/templates/pages/data-table/index.tsx`:
- Around line 398-417: Add an onRowClick prop to the TableRow interface and
function signature, propagate it into renderDesktopView and into any desktop row
element, and ensure the row element calls onRowClick(row) on click and on
keyboard activation (Enter/Space) to support accessibility; also ensure the
checkbox/input used by enableRowSelection and onToggleSelect calls
event.stopPropagation() so clicks on the checkbox don't bubble up to the row
handler; update any other desktop-rendering helpers referenced by
renderDesktopView to accept and forward onRowClick (also apply same changes to
the other occurrences noted).
- Around line 238-289: The selection-derived UI (header checkbox, selectedCount,
toggleAll) still uses raw selectedRows size instead of the pruned/current-page
selection; update derived values and toggleAll to operate on selectedRows ∩
currentKeys (or use getSelectedRowData().length). Specifically: compute a
derivedSelectedKeys = new Set([...selectedRows].filter(k => currentKeys.has(k)))
and use its size for isAllSelected/isIndeterminate/selectedCount; change
toggleAll to only add/remove keys from currentKeys (when all current page keys
are selected remove them from selectedRows, otherwise add them) while preserving
selections on other pages; keep the existing prune useEffect and reuse
keyExtractor/currentKeys in these updates (refer to toggleAll, isAllSelected,
isIndeterminate, getSelectedRowData, and the prune useEffect).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2863921-8af4-4b4a-a67c-d31085901702
📒 Files selected for processing (7)
apps/docs/src/components/Demo/DataTableDemo.tsxapps/docs/src/components/UI/data-table/index.tsxapps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdxapps/storybook/src/templates/pages/data-management/index.tsxpackages/registry/registry.jsonpackages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/data-table/index.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
packages/registry/registry.json
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Register components in
packages/registry/components/directory by creating a component subdirectory and adding an entry topackages/registry/registry.json
Files:
packages/registry/registry.json
**/*.tsx
📄 CodeRabbit inference engine (README.md)
**/*.tsx: Component files must follow the naming pattern: componentName.tsx for the main component file
Integrate seamlessly with Tailwind CSS using smart class merging that respects custom styles
Files:
packages/registry/templates/pages/data-table/data-table.test.tsxapps/docs/src/components/Demo/DataTableDemo.tsxapps/docs/src/components/UI/data-table/index.tsxpackages/registry/templates/pages/data-table/index.tsxapps/storybook/src/templates/pages/data-management/index.tsx
**/*.test.tsx
📄 CodeRabbit inference engine (README.md)
Component test files must follow the naming pattern: componentName.test.tsx for unit tests
Files:
packages/registry/templates/pages/data-table/data-table.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Build components with TypeScript for type-safety and rich IntelliSense support
Files:
packages/registry/templates/pages/data-table/data-table.test.tsxapps/docs/src/components/Demo/DataTableDemo.tsxapps/docs/src/components/UI/data-table/index.tsxpackages/registry/templates/pages/data-table/index.tsxapps/storybook/src/templates/pages/data-management/index.tsx
**/index.tsx
📄 CodeRabbit inference engine (README.md)
Each component must have an index.tsx file for proper module exports
Files:
apps/docs/src/components/UI/data-table/index.tsxpackages/registry/templates/pages/data-table/index.tsxapps/storybook/src/templates/pages/data-management/index.tsx
🧠 Learnings (6)
📚 Learning: 2026-04-02T07:32:04.251Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: README.md:0-0
Timestamp: 2026-04-02T07:32:04.251Z
Learning: Applies to registry.json : registry.json must be updated when new components are added to the project
Applied to files:
packages/registry/registry.json
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to packages/registry/registry.json : Register components in `packages/registry/components/` directory by creating a component subdirectory and adding an entry to `packages/registry/registry.json`
Applied to files:
packages/registry/registry.json
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/docs/docs/components/**/*.mdx : Add MDX documentation files for components in `apps/docs/docs/components/` and link them in `apps/docs/sidebars.ts`
Applied to files:
packages/registry/registry.jsonapps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/storybook/src/components/**/*.{test,spec}.{tsx,ts} : Write unit tests for new components using Vitest, located alongside the component source code
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-03-31T05:22:49.923Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/versioned_docs/version-1.0.3/templates/pages/forms/contact-form.mdx:29-31
Timestamp: 2026-03-31T05:22:49.923Z
Learning: In mindfiredigital/ignix-ui documentation under apps/docs/versioned_docs, the CLI command `ignix add component <name>` is an intentional unified command that imports both components and templates. When this command appears in template-related docs/examples (e.g., template documentation snippets), do not flag it as incorrect or misleading; it is the expected usage.
Applied to files:
apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx
📚 Learning: 2026-03-31T05:21:58.748Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/src/components/UI/contact-form/index.tsx:15-426
Timestamp: 2026-03-31T05:21:58.748Z
Learning: In this repository, it is intentional for UI components to be duplicated as self-contained copies across the docs, storybook, and registry template implementations. Do not raise refactor/DRY duplication review findings when the same component appears in these separate locations, since each copy is maintained independently for its respective app/template.
Applied to files:
apps/docs/src/components/UI/data-table/index.tsx
🔇 Additional comments (8)
apps/storybook/src/templates/pages/data-management/index.tsx (8)
40-54: Well-structured props interface withonRowClicknow included.Good to see
onRowClick?: (row: T) => voidproperly added toDataTableProps<T>(line 53). The interface is clean, well-typed, and provides good IntelliSense support for consumers.
62-113: Solid CVA variant pattern for theming.Clean use of
class-variance-authorityfor managing light/dark theme variants. The variants are well-organized and follow Tailwind best practices with semantic color tokens.
167-169: Correct use ofuseEffectfor page reset.The side effect for resetting to page 1 when data length changes is now properly in
useEffectinstead ofuseMemo. This is the semantically correct hook for dependency-driven side effects.
238-253: Selection pruning effect prevents stale state.The effect correctly reconciles
selectedRowswithcurrentKeyswhen data changes, preventing stale checkbox states after paging/filtering. Good use of thechangedflag to avoid unnecessary state updates.
359-364: Keyboard accessibility properly implemented.The
onKeyDownhandler correctly supports Enter and Space key activation for sortable headers, meeting WCAG 2.1 requirements. Good use ofe.preventDefault()to prevent Space from scrolling.
1161-1175: Clean component composition with proper hook orchestration.The main
DataTablecomponent elegantly composes the filter → sort → paginate → visibility → selection pipeline. Props are well-typed with theT extends Record<string, any>constraint, and theme propagation is consistent throughout.
1204-1222: Stubbed bulk actions with console.log are acceptable for Storybook.The
console.logstatements inhandleDeleteSelectedand the default export action are appropriate for a template/demo context. If this were production code, these would need proper implementations.
1279-1288: Mobile view correctly passesonRowClicktoMobileCardView.Good implementation—
onRowClickis threaded through to the mobile card component, enabling row click interactions. Just needs the same treatment forTableRowin desktop view (see earlier comment).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/registry/templates/pages/contact-form/contact-form.test.tsx (1)
175-218:⚠️ Potential issue | 🟡 MinorRe-add test coverage for default success toast path.
The fallback success branch still exists in
packages/registry/templates/pages/contact-form/index.tsx(Line 227-237), but this suite no longer asserts it whenonSuccessis omitted. That leaves a regression gap.Proposed test addition
+it("shows success toast when onSuccess is not provided", async () => { + const onSubmit = vi.fn().mockResolvedValue(undefined); + const localToast = { addToast: vi.fn() } as any; + + render( + <ToastContext.Provider value={localToast}> + <ContactForm variant="default" onSubmit={onSubmit}> + <ContactForm.Header /> + <ContactForm.Content> + <ContactForm.Field name="name" label="Name" /> + <ContactForm.Field name="email" label="Email" /> + <ContactForm.Field name="subject" label="Subject" /> + <ContactForm.Textarea name="message" /> + </ContactForm.Content> + <ContactForm.Actions /> + </ContactForm> + </ToastContext.Provider> + ); + + fireEvent.change(screen.getByPlaceholderText("Name"), { target: { value: "John" } }); + fireEvent.change(screen.getByPlaceholderText("Email"), { target: { value: "john@test.com" } }); + fireEvent.change(screen.getByPlaceholderText("Subject"), { target: { value: "Hello" } }); + fireEvent.click(screen.getByText("Send Message")); + + await waitFor(() => { + expect(localToast.addToast).toHaveBeenCalledWith( + expect.objectContaining({ + message: "Message sent successfully!", + variant: "success", + }) + ); + }); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/templates/pages/contact-form/contact-form.test.tsx` around lines 175 - 218, Add a test that covers the default success toast path by rendering ContactForm with no onSuccess prop and an onSubmit mock that resolves (e.g., vi.fn().mockResolvedValue(undefined)), provide a mock ToastContext (mockToast with addToast spy) and fill the form fields then click the "Send Message" button; assert that mockToast.addToast was called with an objectContaining the fallback success message used by ContactForm (check the fallback message in ContactForm component) to ensure the default success branch is exercised.
♻️ Duplicate comments (6)
apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx (2)
1539-1542:⚠️ Potential issue | 🟠 MajorDrop
mobileBreakpointfrom the props table.
DataTableProps<T>doesn't expose this prop, and the implementation switches layouts with Tailwind'ssmclasses rather than a numeric breakpoint. Keeping it here sends readers looking for an API that TypeScript rejects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx` around lines 1539 - 1542, The props table incorrectly lists mobileBreakpoint as part of DataTableProps<T> even though the component (DataTable) does not expose that prop and layout switching uses Tailwind responsive classes; remove the `mobileBreakpoint` row from the props table so the docs match the actual API (remove the `mobileBreakpoint | number | 768 | ...` line referencing `mobileBreakpoint` and ensure no other references to `mobileBreakpoint` remain in the DataTableProps documentation or the DataTable component docs).
1683-1687:⚠️ Potential issue | 🟠 MajorFix the basic usage snippet so it compiles as pasted.
The props are currently written outside the opening tag, so this example is invalid TSX.
📝 Suggested fix
-<DataTable> - data={users} - columns={columns} - keyExtractor={(row) => row.id} -/> +<DataTable + data={users} + columns={columns} + keyExtractor={(row) => row.id} +/>Verification: inspect the current snippet around Lines 1683-1687; it still renders
<DataTable>on its own line and the props as sibling lines below it.#!/bin/bash sed -n '1683,1687p' apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx` around lines 1683 - 1687, The DataTable JSX is malformed because its props (data, columns, keyExtractor) are written on separate lines outside the opening tag; fix by putting the props inside the DataTable opening tag and making it a proper self-closing element so the component reads as a single tag with props (referencing DataTable, props data, columns, and keyExtractor) rather than a lone <DataTable> line followed by sibling prop lines.packages/registry/templates/pages/data-table/index.tsx (2)
40-54:⚠️ Potential issue | 🟠 MajorWire
onRowClickthrough the desktop row.
DataTableProps<T>exposes this callback, but the desktopTableRownever receives or invokes it, so the prop only works fromMobileCardView. The docs page also advertises a “Row click handler (mobile + desktop)” example, so this is user-visible.🧩 Suggested fix
interface TableRowProps<T> { row: T; columns: Column<T>[]; index: number; isSelected: boolean; onToggleSelect: (row: T) => void; enableRowSelection?: boolean; keyExtractor: (row: T) => string | number; + onRowClick?: (row: T) => void; theme?: 'light' | 'dark'; } @@ export function TableRow<T>({ row, columns, index, isSelected, onToggleSelect, enableRowSelection, keyExtractor, + onRowClick, theme = 'light', }: TableRowProps<T>) { return ( <motion.tr @@ + onClick={() => onRowClick?.(row)} className={cn( TableRowVariants({ theme }), index % 2 === 0 ? (theme === 'dark' ? 'bg-gray-900/50' : 'bg-background') : (theme === 'dark' ? 'bg-gray-900/30' : 'bg-muted/10'), isSelected && (theme === 'dark' ? 'bg-primary/10' : 'bg-primary/5') )} > {enableRowSelection && ( - <td className="px-4 py-3"> + <td className="px-4 py-3" onClick={(e) => e.stopPropagation()}> <Checkbox @@ {currentData.map((row, idx) => ( <TableRow key={keyExtractor(row)} row={row} columns={visibleColumnsList} index={idx} isSelected={selectedRows.has(keyExtractor(row))} onToggleSelect={toggleRow} enableRowSelection={enableRowSelection} keyExtractor={keyExtractor} + onRowClick={onRowClick} theme={theme} /> ))}Also applies to: 398-407, 419-463, 1394-1405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/templates/pages/data-table/index.tsx` around lines 40 - 54, DataTableProps<T> exposes onRowClick but the desktop row rendering path never receives or invokes it—wire the onRowClick prop through the desktop renderer: pass props.onRowClick into the desktop row component (the TableRow / DesktopRow used when not in MobileCardView) and add an onClick handler inside that row component to call onRowClick(row) when a row is clicked; ensure the row click does not interfere with row selection by preventing propagation or checking selection controls before invoking onRowClick. Use the existing symbols DataTableProps<T>, onRowClick, TableRow (or DesktopRow) and MobileCardView to locate where to pass and call the callback.
268-289:⚠️ Potential issue | 🟠 MajorBase selection logic on current-page membership, not raw set size.
After a page/filter change, the stale key set can still have the same cardinality as the new page until the pruning effect runs. In that render,
toggleAll()can clear instead of select the current page, the header checkbox can look checked, and the bulk-action bar can render withselectedRows=[].🧮 Suggested fix
const toggleAll = useCallback(() => { const keys = data.map(keyExtractor); setSelectedRows(prev => { - if (prev.size === keys.length) { + const allCurrentSelected = + keys.length > 0 && keys.every(key => prev.has(key)); + if (allCurrentSelected) { return new Set(); } return new Set(keys); }); }, [data, keyExtractor]); @@ - const isAllSelected = data.length > 0 && selectedRows.size === data.length; - const isIndeterminate = selectedRows.size > 0 && selectedRows.size < data.length; + const selectedCount = data.reduce( + (count, row) => count + (selectedRows.has(keyExtractor(row)) ? 1 : 0), + 0 + ); + const isAllSelected = data.length > 0 && selectedCount === data.length; + const isIndeterminate = selectedCount > 0 && selectedCount < data.length;+ const selectedRowData = getSelectedRowData(); @@ - selectedCount={selectedRows.size} + selectedCount={selectedRowData.length} actions={bulkActions} - selectedRows={getSelectedRowData()} + selectedRows={selectedRowData} onClearSelection={clearSelection} theme={theme} />Also applies to: 1464-1468
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/templates/pages/data-table/index.tsx` around lines 268 - 289, The selection logic must be based on the current page's keys instead of comparing selectedRows.size to data.length; compute currentPageKeys = data.map(keyExtractor) (memoize with useMemo) and update toggleAll to check whether every key in currentPageKeys is present in selectedRows (using selectedRows.has) and then either remove only those keys or add only those keys (preserving selections on other pages). Likewise, make getSelectedRowData filter rows by selectedRows.has(keyExtractor(row)), and compute isAllSelected and isIndeterminate from the intersection size between selectedRows and currentPageKeys (e.g., count how many currentPageKeys are in selectedRows) rather than comparing against data.length.apps/storybook/src/templates/pages/data-management/index.tsx (2)
238-253:⚠️ Potential issue | 🟠 MajorSelection math still uses the stale set size.
The prune runs in the effect, but
toggleAll,isAllSelected, andisIndeterminatestill readselectedRows.size. After paging/filtering to another slice with the same length, the header checkbox can briefly report “all selected”, andtoggleAll()can clear instead of selecting the current page. Compute these values fromselectedRows ∩ currentKeysinstead of the raw set size.Also applies to: 268-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/storybook/src/templates/pages/data-management/index.tsx` around lines 238 - 253, The current selection state uses the stale selectedRows.size causing header checkbox and toggleAll behavior to be wrong after paging/filtering; update any logic in the component that computes selection summaries (notably toggleAll, isAllSelected, isIndeterminate and any header checkbox handlers) to base their calculations on the intersection between selectedRows and currentKeys (i.e., compute const visibleSelectedCount = Array.from(selectedRows).filter(k => currentKeys.has(k)).length and compare that to currentKeys.size) instead of using selectedRows.size directly; ensure the same change is applied to the other similar block referenced (lines around the other effect/handler at 268-289) so all UI decisions reflect only currently visible rows.
398-418:⚠️ Potential issue | 🟠 Major
onRowClickis still inconsistent across breakpoints.
DataTableProps<T>exposes the callback now, but the desktop path still drops it while mobile turns the row into a clickablediv. That leaves different behavior by breakpoint and no keyboard path on mobile. ThreadonRowClickthroughTableRow, and when it exists render a true interactive surface or addrole="button",tabIndex={0}, and Enter/Space handling while keeping checkbox clicks from bubbling.Also applies to: 828-846, 1392-1403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/storybook/src/templates/pages/data-management/index.tsx` around lines 398 - 418, TableRow currently drops the onRowClick callback on the desktop path causing inconsistent behavior across breakpoints; thread the DataTableProps<T>.onRowClick into TableRow and, when onRowClick is present, render the row as a true interactive surface (or at minimum add role="button", tabIndex={0} and keyboard handlers) so clicks and Enter/Space activate the handler; ensure checkbox clicks (the enableRowSelection/onToggleSelect flow) are stopped from bubbling to the row handler so toggling selection doesn't trigger onRowClick, and apply the same changes to the other occurrences (lines referenced around TableRow and the duplicates) so desktop/mobile share identical accessible behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/storybook/src/templates/pages/data-management/index.tsx`:
- Around line 486-495: The button controls (e.g., the Toggle Columns <button
className=...> element) are missing explicit types and default to "submit";
update every non-submit <button> in this component to include type="button"
(including the similar button instances in the other ranges noted: 622-682,
718-750, 1002-1125) so that opening menus, pagination, and bulk actions do not
submit surrounding forms—search for the JSX <button ...> elements in this file
(the Toggle Columns button and other control buttons) and add type="button" to
each one that is not intended to submit.
- Around line 202-205: The visible-columns Set in useColumnVisibility is only
created on mount and doesn't reconcile when initialColumns changes; update the
hook to watch initialColumns and reconcile stored keys: in useColumnVisibility
(and the related getVisibleColumns logic) add an effect that computes the new
defaultVisibleKeys from initialColumns (columns where col.visible !== false)
then update setVisibleColumns by merging existing keys with those defaults and
removing any keys not present in the new columns list so newly added
default-visible columns become visible and removed columns are dropped; keep the
existing API (useColumnVisibility, getVisibleColumns) but perform this
reconciliation inside a useEffect triggered by initialColumns changes.
- Around line 518-545: The menu items embed a native Checkbox inside
DropdownMenu.Item which prevents Enter/Space from toggling because Radix expects
CheckboxItem; replace DropdownMenu.Item with DropdownMenu.CheckboxItem in the
columns.map render and wire its checked and onCheckedChange props to the current
state and toggle handler (use checked={visibleColumns.has(column.key)} and
onCheckedChange={() => onToggleColumn(column.key)}), remove the nested
Checkbox's event wiring (or remove the nested Checkbox entirely and keep the
visual label/content using column.title and the same id/label pairing) so
keyboard activation and ARIA semantics are provided by DropdownMenu.CheckboxItem
instead of the nested Checkbox.
In `@packages/registry/templates/pages/data-table/data-table.test.tsx`:
- Around line 205-209: Remove the stray screen.debug() call in the test so it no
longer dumps the DOM during successful runs: locate the assertion block that
grabs rowCheckbox (using screen.getAllByTestId('row-checkbox')[0]) and the
subsequent await userEvent.click(rowCheckbox) and delete the screen.debug() line
so the test only asserts
expect(screen.getByText('Delete')).toBeInTheDocument();.
In `@packages/registry/templates/pages/data-table/index.tsx`:
- Around line 1334-1387: The desktop empty-state branches in the table still
render hard-coded headings ("No results found" and "No data available") instead
of using the passed-in/localized strings; update the two desktop branches that
render the FileIcon to remove the hard-coded paragraph text and render the
provided noResultsMessage and emptyStateMessage (the same values used on mobile)
so localization/custom copy flows through; adjust the JSX in the branches that
reference visibleColumnsList and enableRowSelection to replace the hard-coded
headings with the corresponding message variables (noResultsMessage for the
search-no-results branch, emptyStateMessage for the no-data branch) and ensure
styling/structure remains the same.
---
Outside diff comments:
In `@packages/registry/templates/pages/contact-form/contact-form.test.tsx`:
- Around line 175-218: Add a test that covers the default success toast path by
rendering ContactForm with no onSuccess prop and an onSubmit mock that resolves
(e.g., vi.fn().mockResolvedValue(undefined)), provide a mock ToastContext
(mockToast with addToast spy) and fill the form fields then click the "Send
Message" button; assert that mockToast.addToast was called with an
objectContaining the fallback success message used by ContactForm (check the
fallback message in ContactForm component) to ensure the default success branch
is exercised.
---
Duplicate comments:
In
`@apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx`:
- Around line 1539-1542: The props table incorrectly lists mobileBreakpoint as
part of DataTableProps<T> even though the component (DataTable) does not expose
that prop and layout switching uses Tailwind responsive classes; remove the
`mobileBreakpoint` row from the props table so the docs match the actual API
(remove the `mobileBreakpoint | number | 768 | ...` line referencing
`mobileBreakpoint` and ensure no other references to `mobileBreakpoint` remain
in the DataTableProps documentation or the DataTable component docs).
- Around line 1683-1687: The DataTable JSX is malformed because its props (data,
columns, keyExtractor) are written on separate lines outside the opening tag;
fix by putting the props inside the DataTable opening tag and making it a proper
self-closing element so the component reads as a single tag with props
(referencing DataTable, props data, columns, and keyExtractor) rather than a
lone <DataTable> line followed by sibling prop lines.
In `@apps/storybook/src/templates/pages/data-management/index.tsx`:
- Around line 238-253: The current selection state uses the stale
selectedRows.size causing header checkbox and toggleAll behavior to be wrong
after paging/filtering; update any logic in the component that computes
selection summaries (notably toggleAll, isAllSelected, isIndeterminate and any
header checkbox handlers) to base their calculations on the intersection between
selectedRows and currentKeys (i.e., compute const visibleSelectedCount =
Array.from(selectedRows).filter(k => currentKeys.has(k)).length and compare that
to currentKeys.size) instead of using selectedRows.size directly; ensure the
same change is applied to the other similar block referenced (lines around the
other effect/handler at 268-289) so all UI decisions reflect only currently
visible rows.
- Around line 398-418: TableRow currently drops the onRowClick callback on the
desktop path causing inconsistent behavior across breakpoints; thread the
DataTableProps<T>.onRowClick into TableRow and, when onRowClick is present,
render the row as a true interactive surface (or at minimum add role="button",
tabIndex={0} and keyboard handlers) so clicks and Enter/Space activate the
handler; ensure checkbox clicks (the enableRowSelection/onToggleSelect flow) are
stopped from bubbling to the row handler so toggling selection doesn't trigger
onRowClick, and apply the same changes to the other occurrences (lines
referenced around TableRow and the duplicates) so desktop/mobile share identical
accessible behavior.
In `@packages/registry/templates/pages/data-table/index.tsx`:
- Around line 40-54: DataTableProps<T> exposes onRowClick but the desktop row
rendering path never receives or invokes it—wire the onRowClick prop through the
desktop renderer: pass props.onRowClick into the desktop row component (the
TableRow / DesktopRow used when not in MobileCardView) and add an onClick
handler inside that row component to call onRowClick(row) when a row is clicked;
ensure the row click does not interfere with row selection by preventing
propagation or checking selection controls before invoking onRowClick. Use the
existing symbols DataTableProps<T>, onRowClick, TableRow (or DesktopRow) and
MobileCardView to locate where to pass and call the callback.
- Around line 268-289: The selection logic must be based on the current page's
keys instead of comparing selectedRows.size to data.length; compute
currentPageKeys = data.map(keyExtractor) (memoize with useMemo) and update
toggleAll to check whether every key in currentPageKeys is present in
selectedRows (using selectedRows.has) and then either remove only those keys or
add only those keys (preserving selections on other pages). Likewise, make
getSelectedRowData filter rows by selectedRows.has(keyExtractor(row)), and
compute isAllSelected and isIndeterminate from the intersection size between
selectedRows and currentPageKeys (e.g., count how many currentPageKeys are in
selectedRows) rather than comparing against data.length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e32b5b7d-d284-45bd-bde4-fafdd90702f7
📒 Files selected for processing (11)
apps/docs/src/components/UI/contact-form/index.tsxapps/docs/src/components/UI/data-table/index.tsxapps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdxapps/docs/versioned_sidebars/version-1.0.3-sidebars.jsonapps/storybook/src/templates/pages/data-management/index.tsxapps/storybook/src/templates/pages/form-pages/contact-form/index.tsxpackages/registry/registry.jsonpackages/registry/templates/pages/contact-form/contact-form.test.tsxpackages/registry/templates/pages/contact-form/index.tsxpackages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/data-table/index.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.tsx
📄 CodeRabbit inference engine (README.md)
**/*.tsx: Component files must follow the naming pattern: componentName.tsx for the main component file
Integrate seamlessly with Tailwind CSS using smart class merging that respects custom styles
Files:
apps/storybook/src/templates/pages/form-pages/contact-form/index.tsxpackages/registry/templates/pages/contact-form/index.tsxapps/docs/src/components/UI/contact-form/index.tsxpackages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/contact-form/contact-form.test.tsxapps/docs/src/components/UI/data-table/index.tsxpackages/registry/templates/pages/data-table/index.tsxapps/storybook/src/templates/pages/data-management/index.tsx
**/index.tsx
📄 CodeRabbit inference engine (README.md)
Each component must have an index.tsx file for proper module exports
Files:
apps/storybook/src/templates/pages/form-pages/contact-form/index.tsxpackages/registry/templates/pages/contact-form/index.tsxapps/docs/src/components/UI/contact-form/index.tsxapps/docs/src/components/UI/data-table/index.tsxpackages/registry/templates/pages/data-table/index.tsxapps/storybook/src/templates/pages/data-management/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Build components with TypeScript for type-safety and rich IntelliSense support
Files:
apps/storybook/src/templates/pages/form-pages/contact-form/index.tsxpackages/registry/templates/pages/contact-form/index.tsxapps/docs/src/components/UI/contact-form/index.tsxpackages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/contact-form/contact-form.test.tsxapps/docs/src/components/UI/data-table/index.tsxpackages/registry/templates/pages/data-table/index.tsxapps/storybook/src/templates/pages/data-management/index.tsx
packages/registry/registry.json
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Register components in
packages/registry/components/directory by creating a component subdirectory and adding an entry topackages/registry/registry.json
Files:
packages/registry/registry.json
**/*.test.tsx
📄 CodeRabbit inference engine (README.md)
Component test files must follow the naming pattern: componentName.test.tsx for unit tests
Files:
packages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/contact-form/contact-form.test.tsx
🧠 Learnings (10)
📚 Learning: 2026-03-31T07:07:47.374Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: packages/registry/templates/pages/contact-form/index.tsx:143-148
Timestamp: 2026-03-31T07:07:47.374Z
Learning: In `packages/registry/templates/pages/contact-form/index.tsx` (React/TypeScript), the `useToast()` call is deliberately wrapped in try/catch to make toast notifications optional — the ContactForm is designed to work without a ToastProvider. The correct fix is a `useSafeToast` wrapper that calls `React.useContext(ToastContext)` directly and returns null when the context is absent, rather than throwing.
Applied to files:
apps/storybook/src/templates/pages/form-pages/contact-form/index.tsxpackages/registry/templates/pages/contact-form/index.tsxapps/docs/src/components/UI/contact-form/index.tsxpackages/registry/templates/pages/contact-form/contact-form.test.tsx
📚 Learning: 2026-03-31T05:21:58.748Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/src/components/UI/contact-form/index.tsx:15-426
Timestamp: 2026-03-31T05:21:58.748Z
Learning: In this repository, it is intentional for UI components to be duplicated as self-contained copies across the docs, storybook, and registry template implementations. Do not raise refactor/DRY duplication review findings when the same component appears in these separate locations, since each copy is maintained independently for its respective app/template.
Applied to files:
apps/docs/src/components/UI/contact-form/index.tsxapps/docs/src/components/UI/data-table/index.tsx
📚 Learning: 2026-04-02T07:32:04.251Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: README.md:0-0
Timestamp: 2026-04-02T07:32:04.251Z
Learning: Applies to registry.json : registry.json must be updated when new components are added to the project
Applied to files:
packages/registry/registry.json
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to packages/registry/registry.json : Register components in `packages/registry/components/` directory by creating a component subdirectory and adding an entry to `packages/registry/registry.json`
Applied to files:
packages/registry/registry.json
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/docs/docs/components/**/*.mdx : Add MDX documentation files for components in `apps/docs/docs/components/` and link them in `apps/docs/sidebars.ts`
Applied to files:
packages/registry/registry.jsonapps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/storybook/src/components/**/*.{test,spec}.{tsx,ts} : Write unit tests for new components using Vitest, located alongside the component source code
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/contact-form/contact-form.test.tsx
📚 Learning: 2026-03-31T05:22:02.508Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/src/components/UI/contact-form/index.tsx:15-426
Timestamp: 2026-03-31T05:22:02.508Z
Learning: In the mindfiredigital/ignix-ui repository, it is intentional and expected for component implementations to be duplicated across `apps/docs/src/components/UI/`, `apps/storybook/src/`, and `packages/registry/templates/`. Each location maintains its own self-contained copy of the component. Do not flag this duplication as a refactor issue in future reviews.
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Ensure all tests pass by running `pnpm vitest` before submitting changes
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-04-02T07:32:04.251Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: README.md:0-0
Timestamp: 2026-04-02T07:32:04.251Z
Learning: Applies to **/*.test.tsx : Component test files must follow the naming pattern: componentName.test.tsx for unit tests
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-03-31T05:22:49.923Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/versioned_docs/version-1.0.3/templates/pages/forms/contact-form.mdx:29-31
Timestamp: 2026-03-31T05:22:49.923Z
Learning: In mindfiredigital/ignix-ui documentation under apps/docs/versioned_docs, the CLI command `ignix add component <name>` is an intentional unified command that imports both components and templates. When this command appears in template-related docs/examples (e.g., template documentation snippets), do not flag it as incorrect or misleading; it is the expected usage.
Applied to files:
apps/docs/versioned_docs/version-1.0.3/templates/pages/data-management/data-table.mdx
🔇 Additional comments (5)
packages/registry/templates/pages/contact-form/contact-form.test.tsx (1)
176-197: Good move to provider-backed toast testing.This aligns the test with runtime context wiring and avoids relying on implementation-specific hook mocks.
Based on learnings: the ContactForm is intentionally designed to work without a ToastProvider, and the preferred pattern is a
useSafeToastwrapper usingToastContext.packages/registry/templates/pages/contact-form/index.tsx (1)
151-154:useSafeToastintegration looks correct.This keeps toast calls optional while preserving existing success/error behavior through guarded
toast?.addToast(...).Based on learnings: the expected fix is to use a
useSafeToastwrapper that readsToastContextdirectly and tolerates missing providers.Also applies to: 171-172
apps/storybook/src/templates/pages/form-pages/contact-form/index.tsx (1)
150-153: Nice parity update with the safe toast pattern.This is a clean alignment of Storybook behavior with the contact-form optional toast design.
Based on learnings: ContactForm should keep toast notifications optional via a
useSafeToastwrapper overToastContext.Also applies to: 169-170
apps/docs/src/components/UI/contact-form/index.tsx (1)
150-153: Safe toast access change is solid here as well.Behavior remains resilient when a toast provider is absent, with no change to guarded toast emission paths.
Based on learnings: using
useSafeToastwith directToastContextaccess is the intended approach, and these docs/storybook/registry copies are intentionally maintained independently.Also applies to: 169-170
apps/storybook/src/templates/pages/data-management/index.tsx (1)
358-373: Nice keyboard support on sortable headers.
Enter/Spaceactivation plusaria-sortcloses the keyboard-access gap here cleanly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
packages/registry/templates/pages/data-table/index.tsx (2)
819-855:⚠️ Potential issue | 🟠 MajorDesktop rows still drop
onRowClick.The prop is part of the public API and works in
MobileCardView, but desktop rows never receive it. That makes template behavior inconsistent by breakpoint. When you wire it throughTableRow, stop propagation from the checkbox cell so selection does not also trigger the row action.Also applies to: 1405-1417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/templates/pages/data-table/index.tsx` around lines 819 - 855, Desktop rows are not wired with the onRowClick handler causing inconsistent behavior vs MobileCardView; update the desktop row rendering (the TableRow component usage) to accept and call onRowClick (e.g., add onClick={() => onRowClick?.(row)} on the TableRow or wrapping element) and ensure the checkbox cell stops propagation (use event.stopPropagation in the checkbox's onClick/onChange handler) so selecting a row via the checkbox doesn't trigger the row click; refer to MobileCardView, TableRow, keyExtractor, onRowClick and the checkbox cell rendering (also apply the same fix for the duplicate block around the other occurrence).
1245-1275:⚠️ Potential issue | 🟡 MinorDesktop empty states still hard-code English headings.
Mobile uses
noResultsMessageandemptyStateMessagedirectly, but desktop prepends fixed"No results found"/"No data available"copy. That keeps localization and custom messaging inconsistent across breakpoints.Also applies to: 1346-1399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/templates/pages/data-table/index.tsx` around lines 1245 - 1275, Desktop empty-state blocks currently prepend hard-coded English headings; replace those literals with the same localized/custom props used on mobile by using noResultsMessage and emptyStateMessage instead of "No results found"/"No data available". Locate the desktop branches rendering the FileIcon and paragraph (the JSX near the MobileSkeletonCard and FileIcon usage) and remove/replace the fixed heading strings so the paragraph displays the provided noResultsMessage or emptyStateMessage (respecting the theme-based classNames) to keep messaging consistent across breakpoints.apps/storybook/src/templates/pages/data-management/data-table/index.tsx (2)
521-550:⚠️ Potential issue | 🟠 MajorSwitch the column menu to
DropdownMenu.CheckboxItem.This still uses a focusable
DropdownMenu.Itemwith a nested checkbox, so keyboard activation targets the item rather than a checkable Radix primitive. The result is unreliable Enter/Space toggling for column visibility.According to the official `@radix-ui/react-dropdown-menu` documentation, should a multi-select dropdown use DropdownMenu.CheckboxItem instead of nesting a checkbox inside DropdownMenu.Item?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/storybook/src/templates/pages/data-management/data-table/index.tsx` around lines 521 - 550, The menu currently maps columns into DropdownMenu.Item containing a separate Checkbox and label which prevents proper keyboard activation; replace each DropdownMenu.Item with DropdownMenu.CheckboxItem (use the same key from columns.map and id like `col-${String(column.key)}`) and wire its checked state to visibleColumns.has(column.key) and its onCheckedChange to call onToggleColumn(column.key); remove the nested Checkbox and label elements (or move label text into the CheckboxItem children) and keep/adjust data-testid attributes (e.g., `checkbox-${String(column.key)}`) so the Radix CheckboxItem is the actual interactive primitive for correct Enter/Space behavior.
819-855:⚠️ Potential issue | 🟠 MajorDesktop rows still ignore
onRowClick.The mobile card view invokes
onRowClick, but the desktop<TableRow>path still never receives it. That leaves row actions breakpoint-dependent. When you thread it through, stop propagation from the selection cell so checkbox clicks do not also fire the row action.Also applies to: 1405-1417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/storybook/src/templates/pages/data-management/data-table/index.tsx` around lines 819 - 855, Desktop rows currently don't call onRowClick while MobileCardView does; add the same onClick behavior to the desktop row renderer (the TableRow/row render block) so it invokes onRowClick(row) when present, and ensure the selection cell's click handler (the checkbox cell that uses onToggleSelect / enableRowSelection) calls event.stopPropagation() to prevent checkbox clicks from also firing the row onClick; update the desktop row element (TableRow / the element rendering each row) to include the onClick and adjust the selection cell click handler to stop propagation.packages/registry/templates/pages/data-table/data-table.test.tsx (1)
144-165:⚠️ Potential issue | 🟡 MinorPlease add the page-change selection regression case.
The suite still tests pagination and selection separately, but not whether bulk-selection state stays in sync after moving between pages. That reconciliation path was already called out and is still unguarded.
Also applies to: 171-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/templates/pages/data-table/data-table.test.tsx` around lines 144 - 165, Add a regression test in the Pagination suite that verifies bulk-selection stays in sync when changing pages: render DataTable (using defaultProps, defaultPageSize and bigData as in this test), perform a bulk select on page 1 (e.g., click the header "select all" checkbox), assert selected state for items on page 1, then fireEvent.click to move to the next page (using the existing next-page button lookup) and assert that selection state reconciles (e.g., selected count/checkboxes reflect previously selected items or that "select all" is not incorrectly toggled); also navigate back to page 1 and re-assert selection to ensure state persisted across page changes.apps/docs/src/components/UI/data-table/index.tsx (1)
819-855:⚠️ Potential issue | 🟠 MajorDesktop rows still drop
onRowClick.The mobile card path calls
onRowClick, but the desktop row path never receives it. Consumers get different behavior by breakpoint. When you wire this throughTableRow, also stop propagation from the selection cell so checking a row does not trigger the row action.Concrete wiring fix
interface TableRowProps<T> { row: T; columns: Column<T>[]; index: number; isSelected: boolean; onToggleSelect: (row: T) => void; enableRowSelection?: boolean; keyExtractor: (row: T) => string | number; theme?: 'light' | 'dark'; + onRowClick?: (row: T) => void; } export function TableRow<T>({ row, columns, index, isSelected, onToggleSelect, enableRowSelection, keyExtractor, theme = 'light', + onRowClick, }: TableRowProps<T>) { return ( <motion.tr + onClick={() => onRowClick?.(row)} initial={{ opacity: 0, y: -10 }} animate={{ opacity: 1, y: 0 }} exit={{ opacity: 0, y: 10 }} transition={{ duration: 0.2, delay: index * 0.02 }} className={cn( TableRowVariants({ theme }), + onRowClick && "cursor-pointer", index % 2 === 0 ? (theme === 'dark' ? 'bg-gray-900/50' : 'bg-background') : (theme === 'dark' ? 'bg-gray-900/30' : 'bg-muted/10'), isSelected && (theme === 'dark' ? 'bg-primary/10' : 'bg-primary/5') )} > {enableRowSelection && ( - <td className="px-4 py-3"> + <td className="px-4 py-3" onClick={(e) => e.stopPropagation()}> <Checkbox className={cn( "w-4 h-4 rounded focus:outline-none focus:ring-2", theme === 'dark' ? "border-gray-600 focus:ring-primary/20 bg-gray-800" @@ {currentData.map((row, idx) => ( <TableRow key={keyExtractor(row)} row={row} columns={visibleColumnsList} index={idx} isSelected={selectedRows.has(keyExtractor(row))} onToggleSelect={toggleRow} enableRowSelection={enableRowSelection} keyExtractor={keyExtractor} theme={theme} + onRowClick={onRowClick} /> ))}Also applies to: 1405-1417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/src/components/UI/data-table/index.tsx` around lines 819 - 855, The desktop row path isn't wired to onRowClick and clicks on the selection cell bubble and trigger row actions; pass the onRowClick handler into the desktop row component (same way MobileCardView uses onRowClick) — locate the TableRow / desktop row render (the component rendering the desktop <tr> or row element) and add onClick={() => onRowClick?.(row)} (or forward onRowClick prop into TableRow) so desktop rows match mobile behavior, and in the selection cell (checkbox/cell click handler) call e.stopPropagation() before toggling selection (and keep using onToggleSelect/keyExtractor/selectedRows) so checking a box does not trigger the row click action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docs/src/components/UI/data-table/index.tsx`:
- Around line 1245-1275: The desktop empty-state branches render hard-coded
English headings instead of the provided copy, breaking localization; update the
desktop JSX that currently shows the FileIcon and hard-coded headings to use the
same message props as mobile (noResultsMessage and emptyStateMessage) so the
component uses the provided localized/custom copy for both desktop and mobile,
keeping the existing theme-based className logic and structure around FileIcon,
and remove the hard-coded heading strings in those desktop branches (the
conditional blocks that check !hasData && !hasSearchResults && searchTerm and
!hasData).
In
`@apps/storybook/src/templates/pages/data-management/data-table/data-table.stories.tsx`:
- Around line 85-98: The fixtures in the employees array use human-readable
joinDate strings which sort lexicographically; update the employees fixture (the
employees: Employee[] array) to use machine-sortable values (ISO 8601 date
strings or actual Date objects) for the joinDate field, then move any
human-friendly formatting into the DataTable column renderer for the joinDate
column so sorting is chronological (adjust the column renderer used by the
DataTable story rather than the fixture values).
In `@packages/registry/templates/pages/data-table/data-table.test.tsx`:
- Around line 182-194: The test in the "selects all rows" case is only asserting
the header checkbox because it queries getAllByTestId('checkbox'); update the
assertion to target the actual row inputs (getAllByTestId('row-checkbox')) or
assert the table's selected-row UI state after clicking the "select all rows"
control (label text /select all rows/i). Specifically, inside the it('selects
all rows') test that renders DataTable, keep the
fireEvent.click(screen.getByLabelText(/select all rows/i)) call but replace
checkboxes = screen.getAllByTestId('checkbox') with rowCheckboxes =
screen.getAllByTestId('row-checkbox') and assert each rowCheckbox isChecked (or
alternatively assert the component's selected-row class/count) so the test truly
verifies rows are selected.
In `@packages/registry/templates/pages/data-table/index.tsx`:
- Around line 521-550: Replace the nested Checkbox inside DropdownMenu.Item with
Radix's DropdownMenu.CheckboxItem to restore proper keyboard/focus behavior: in
the columns.map block, stop rendering DropdownMenu.Item containing <Checkbox ...
/> and instead render DropdownMenu.CheckboxItem (use column.key for key/id) and
pass checked={visibleColumns.has(column.key)} and onCheckedChange={() =>
onToggleColumn(column.key)}; keep the label text from column.title and preserve
className/theme styling for the menu item and label, and remove the inner
Checkbox component to avoid double test IDs and focus conflicts.
---
Duplicate comments:
In `@apps/docs/src/components/UI/data-table/index.tsx`:
- Around line 819-855: The desktop row path isn't wired to onRowClick and clicks
on the selection cell bubble and trigger row actions; pass the onRowClick
handler into the desktop row component (same way MobileCardView uses onRowClick)
— locate the TableRow / desktop row render (the component rendering the desktop
<tr> or row element) and add onClick={() => onRowClick?.(row)} (or forward
onRowClick prop into TableRow) so desktop rows match mobile behavior, and in the
selection cell (checkbox/cell click handler) call e.stopPropagation() before
toggling selection (and keep using onToggleSelect/keyExtractor/selectedRows) so
checking a box does not trigger the row click action.
In `@apps/storybook/src/templates/pages/data-management/data-table/index.tsx`:
- Around line 521-550: The menu currently maps columns into DropdownMenu.Item
containing a separate Checkbox and label which prevents proper keyboard
activation; replace each DropdownMenu.Item with DropdownMenu.CheckboxItem (use
the same key from columns.map and id like `col-${String(column.key)}`) and wire
its checked state to visibleColumns.has(column.key) and its onCheckedChange to
call onToggleColumn(column.key); remove the nested Checkbox and label elements
(or move label text into the CheckboxItem children) and keep/adjust data-testid
attributes (e.g., `checkbox-${String(column.key)}`) so the Radix CheckboxItem is
the actual interactive primitive for correct Enter/Space behavior.
- Around line 819-855: Desktop rows currently don't call onRowClick while
MobileCardView does; add the same onClick behavior to the desktop row renderer
(the TableRow/row render block) so it invokes onRowClick(row) when present, and
ensure the selection cell's click handler (the checkbox cell that uses
onToggleSelect / enableRowSelection) calls event.stopPropagation() to prevent
checkbox clicks from also firing the row onClick; update the desktop row element
(TableRow / the element rendering each row) to include the onClick and adjust
the selection cell click handler to stop propagation.
In `@packages/registry/templates/pages/data-table/data-table.test.tsx`:
- Around line 144-165: Add a regression test in the Pagination suite that
verifies bulk-selection stays in sync when changing pages: render DataTable
(using defaultProps, defaultPageSize and bigData as in this test), perform a
bulk select on page 1 (e.g., click the header "select all" checkbox), assert
selected state for items on page 1, then fireEvent.click to move to the next
page (using the existing next-page button lookup) and assert that selection
state reconciles (e.g., selected count/checkboxes reflect previously selected
items or that "select all" is not incorrectly toggled); also navigate back to
page 1 and re-assert selection to ensure state persisted across page changes.
In `@packages/registry/templates/pages/data-table/index.tsx`:
- Around line 819-855: Desktop rows are not wired with the onRowClick handler
causing inconsistent behavior vs MobileCardView; update the desktop row
rendering (the TableRow component usage) to accept and call onRowClick (e.g.,
add onClick={() => onRowClick?.(row)} on the TableRow or wrapping element) and
ensure the checkbox cell stops propagation (use event.stopPropagation in the
checkbox's onClick/onChange handler) so selecting a row via the checkbox doesn't
trigger the row click; refer to MobileCardView, TableRow, keyExtractor,
onRowClick and the checkbox cell rendering (also apply the same fix for the
duplicate block around the other occurrence).
- Around line 1245-1275: Desktop empty-state blocks currently prepend hard-coded
English headings; replace those literals with the same localized/custom props
used on mobile by using noResultsMessage and emptyStateMessage instead of "No
results found"/"No data available". Locate the desktop branches rendering the
FileIcon and paragraph (the JSX near the MobileSkeletonCard and FileIcon usage)
and remove/replace the fixed heading strings so the paragraph displays the
provided noResultsMessage or emptyStateMessage (respecting the theme-based
classNames) to keep messaging consistent across breakpoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef9e0381-d008-4535-b0b4-be58836f3fa8
📒 Files selected for processing (5)
apps/docs/src/components/UI/data-table/index.tsxapps/storybook/src/templates/pages/data-management/data-table/data-table.stories.tsxapps/storybook/src/templates/pages/data-management/data-table/index.tsxpackages/registry/templates/pages/data-table/data-table.test.tsxpackages/registry/templates/pages/data-table/index.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (README.md)
**/*.tsx: Component files must follow the naming pattern: componentName.tsx for the main component file
Integrate seamlessly with Tailwind CSS using smart class merging that respects custom styles
Files:
apps/storybook/src/templates/pages/data-management/data-table/data-table.stories.tsxapps/docs/src/components/UI/data-table/index.tsxpackages/registry/templates/pages/data-table/data-table.test.tsxapps/storybook/src/templates/pages/data-management/data-table/index.tsxpackages/registry/templates/pages/data-table/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Build components with TypeScript for type-safety and rich IntelliSense support
Files:
apps/storybook/src/templates/pages/data-management/data-table/data-table.stories.tsxapps/docs/src/components/UI/data-table/index.tsxpackages/registry/templates/pages/data-table/data-table.test.tsxapps/storybook/src/templates/pages/data-management/data-table/index.tsxpackages/registry/templates/pages/data-table/index.tsx
**/index.tsx
📄 CodeRabbit inference engine (README.md)
Each component must have an index.tsx file for proper module exports
Files:
apps/docs/src/components/UI/data-table/index.tsxapps/storybook/src/templates/pages/data-management/data-table/index.tsxpackages/registry/templates/pages/data-table/index.tsx
**/*.test.tsx
📄 CodeRabbit inference engine (README.md)
Component test files must follow the naming pattern: componentName.test.tsx for unit tests
Files:
packages/registry/templates/pages/data-table/data-table.test.tsx
🧠 Learnings (8)
📓 Common learnings
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: packages/registry/templates/pages/contact-form/index.tsx:143-148
Timestamp: 2026-03-31T07:07:47.374Z
Learning: In `packages/registry/templates/pages/contact-form/index.tsx` (React/TypeScript), the `useToast()` call is deliberately wrapped in try/catch to make toast notifications optional — the ContactForm is designed to work without a ToastProvider. The correct fix is a `useSafeToast` wrapper that calls `React.useContext(ToastContext)` directly and returns null when the context is absent, rather than throwing.
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/storybook/src/components/**/*.stories.{tsx,ts} : Create a Story File named `[component-name].stories.tsx` in the same directory as the component file with argTypes, controls, and example stories
Applied to files:
apps/storybook/src/templates/pages/data-management/data-table/data-table.stories.tsx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/storybook/src/components/**/*.{tsx,ts,jsx,js} : Create Component Files in `apps/storybook/src/components/` directory with a component subdirectory for each component
Applied to files:
apps/storybook/src/templates/pages/data-management/data-table/data-table.stories.tsx
📚 Learning: 2026-03-31T05:21:58.748Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/src/components/UI/contact-form/index.tsx:15-426
Timestamp: 2026-03-31T05:21:58.748Z
Learning: In this repository, it is intentional for UI components to be duplicated as self-contained copies across the docs, storybook, and registry template implementations. Do not raise refactor/DRY duplication review findings when the same component appears in these separate locations, since each copy is maintained independently for its respective app/template.
Applied to files:
apps/docs/src/components/UI/data-table/index.tsx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/storybook/src/components/**/*.{test,spec}.{tsx,ts} : Write unit tests for new components using Vitest, located alongside the component source code
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-03-31T05:22:02.508Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/src/components/UI/contact-form/index.tsx:15-426
Timestamp: 2026-03-31T05:22:02.508Z
Learning: In the mindfiredigital/ignix-ui repository, it is intentional and expected for component implementations to be duplicated across `apps/docs/src/components/UI/`, `apps/storybook/src/`, and `packages/registry/templates/`. Each location maintains its own self-contained copy of the component. Do not flag this duplication as a refactor issue in future reviews.
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Ensure all tests pass by running `pnpm vitest` before submitting changes
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-04-02T07:32:04.251Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: README.md:0-0
Timestamp: 2026-04-02T07:32:04.251Z
Learning: Applies to **/*.test.tsx : Component test files must follow the naming pattern: componentName.test.tsx for unit tests
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
🔇 Additional comments (1)
apps/docs/src/components/UI/data-table/index.tsx (1)
521-550: Please provide the review comment that needs to be rewritten within<review_comment>tags, along with any relevant context or file information for verification.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/registry/templates/pages/data-table/data-table.test.tsx (1)
144-195:⚠️ Potential issue | 🟡 MinorAdd the pagination+selection interaction regression test.
Pagination and selection are tested independently, but there’s still no assertion that selected-row/bulk-action state stays correct after a page change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/registry/templates/pages/data-table/data-table.test.tsx` around lines 144 - 195, Add a regression test that verifies selection state survives pagination: in the existing tests for DataTable (use the DataTable component and the Pagination and Row Selection suites), render a larger dataset (e.g., bigData with defaultPageSize={5}), select a row (or click the "select all" control via getByLabelText(/select all rows/i) or the first row checkbox via getAllByTestId('row-checkbox')[0]), navigate to the next page with the "next page" control (getAllByLabelText(/next page/i)[0]), then navigate back and assert the originally selected row(s) or the bulk-selection state are still checked (expect(...).toBeChecked()) to ensure selection persists across page changes; name the new test something like 'selection persists across pages'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/registry/templates/pages/data-table/data-table.test.tsx`:
- Around line 93-96: The test for the DataTable empty state is asserting the
wrong message; when rendering DataTable with emptyStateMessage="No data" (using
defaultProps) the expectation should look for "No data" instead of "No data
available" or else change the prop to "No data available" to match the
assertion. Update the assertion in the test (referencing the render call and
screen.getByText usage in the data-table.test.tsx) to expect the exact
emptyStateMessage passed, or adjust the emptyStateMessage prop to match the
expected string so they are consistent.
---
Duplicate comments:
In `@packages/registry/templates/pages/data-table/data-table.test.tsx`:
- Around line 144-195: Add a regression test that verifies selection state
survives pagination: in the existing tests for DataTable (use the DataTable
component and the Pagination and Row Selection suites), render a larger dataset
(e.g., bigData with defaultPageSize={5}), select a row (or click the "select
all" control via getByLabelText(/select all rows/i) or the first row checkbox
via getAllByTestId('row-checkbox')[0]), navigate to the next page with the "next
page" control (getAllByLabelText(/next page/i)[0]), then navigate back and
assert the originally selected row(s) or the bulk-selection state are still
checked (expect(...).toBeChecked()) to ensure selection persists across page
changes; name the new test something like 'selection persists across pages'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8246ae2a-94fd-40f6-8dac-23fa45b61090
📒 Files selected for processing (1)
packages/registry/templates/pages/data-table/data-table.test.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (README.md)
**/*.tsx: Component files must follow the naming pattern: componentName.tsx for the main component file
Integrate seamlessly with Tailwind CSS using smart class merging that respects custom styles
Files:
packages/registry/templates/pages/data-table/data-table.test.tsx
**/*.test.tsx
📄 CodeRabbit inference engine (README.md)
Component test files must follow the naming pattern: componentName.test.tsx for unit tests
Files:
packages/registry/templates/pages/data-table/data-table.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (README.md)
Build components with TypeScript for type-safety and rich IntelliSense support
Files:
packages/registry/templates/pages/data-table/data-table.test.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: packages/registry/templates/pages/contact-form/index.tsx:143-148
Timestamp: 2026-03-31T07:07:47.374Z
Learning: In `packages/registry/templates/pages/contact-form/index.tsx` (React/TypeScript), the `useToast()` call is deliberately wrapped in try/catch to make toast notifications optional — the ContactForm is designed to work without a ToastProvider. The correct fix is a `useSafeToast` wrapper that calls `React.useContext(ToastContext)` directly and returns null when the context is absent, rather than throwing.
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Applies to apps/storybook/src/components/**/*.{test,spec}.{tsx,ts} : Write unit tests for new components using Vitest, located alongside the component source code
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-03-31T05:22:02.508Z
Learnt from: PoonamKPatil
Repo: mindfiredigital/ignix-ui PR: 723
File: apps/docs/src/components/UI/contact-form/index.tsx:15-426
Timestamp: 2026-03-31T05:22:02.508Z
Learning: In the mindfiredigital/ignix-ui repository, it is intentional and expected for component implementations to be duplicated across `apps/docs/src/components/UI/`, `apps/storybook/src/`, and `packages/registry/templates/`. Each location maintains its own self-contained copy of the component. Do not flag this duplication as a refactor issue in future reviews.
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-03-18T09:43:08.260Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-03-18T09:43:08.260Z
Learning: Ensure all tests pass by running `pnpm vitest` before submitting changes
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
📚 Learning: 2026-04-02T07:32:04.251Z
Learnt from: CR
Repo: mindfiredigital/ignix-ui PR: 0
File: README.md:0-0
Timestamp: 2026-04-02T07:32:04.251Z
Learning: Applies to **/*.test.tsx : Component test files must follow the naming pattern: componentName.test.tsx for unit tests
Applied to files:
packages/registry/templates/pages/data-table/data-table.test.tsx
🔇 Additional comments (2)
packages/registry/templates/pages/data-table/data-table.test.tsx (2)
25-35: Verify the Checkbox mock target still matches the actual import path.Line 25 mocks
@ignix-ui/checkbox; ifDataTableimportsCheckboxfrom a different specifier, the mock is inert and selection tests become misleading.#!/bin/bash # Verify DataTable's Checkbox import specifier and compare with test mock specifier. rg -n "import\\s+\\{\\s*Checkbox\\s*\\}\\s+from\\s+['\"][^'\"]+['\"]" packages/registry/templates/pages/data-table/index.tsx rg -n "vi\\.mock\\(" packages/registry/templates/pages/data-table/data-table.test.tsx
190-193: Nice improvement: row-level selection is asserted correctly.Using
row-checkboxhere matches the DataTable row checkbox test IDs and makes the select-all test meaningful.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Title
Added feature #716 .
Description
What does this PR do?
Provide a concise summary of your changes and what issue or feature they address.
Related Issues
Link any related issue(s) with references to help reviewers understand the context.
Type of Change
Checklist
Please ensure the following have been completed before submitting:
npm run lint.Screenshots (if applicable)
Provide screenshots or GIFs if this PR changes the UI or has visible results.
Additional Context
Include any other context, references, or information that may be useful for the reviewers.
Thank you for your contribution!
We appreciate your efforts in making this project better.
New Components or Component API Changes
Bug Fixes
Tooling / Config Changes
Docs / Storybook Updates
Breaking changes