Repository navigation
feat(ui-kit): DataTable organism (TanStack, headless) - #521
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 TanStack-based, DOM-rendered ChangesDataTable
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: 🟡 Moderate · up to The DataTable adds sorting, filtering, pinned columns, and row-detail behavior, but the current implementation can misfilter blank numeric values and visually overlap pinned data with row actions. These correctness issues should be fixed or explicitly accepted before merging; the remaining concerns are localized follow-up items. Sequence Diagram(s)sequenceDiagram
participant Consumer
participant DataTable
participant TanStackTable
participant Table
Consumer->>DataTable: provide data, columns, and feature props
DataTable->>TanStackTable: create table instance and apply state
TanStackTable-->>DataTable: return rows and headers
DataTable->>Table: render DOM table content
Consumer->>DataTable: trigger filtering, editing, selection, or reordering
DataTable-->>Consumer: invoke configured callbacks
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Greptile SummaryThe PR introduces a headless DataTable organism and extends the presentational Table API.
Confidence Score: 3/5The PR is not yet safe to merge because duplicate-reference row reordering can move the wrong record and clickable rows still discard consumer keyboard handlers. Row drag completion maps distinct row IDs back to source positions with reference-based Files Needing Attention: libs/ui/src/organisms/data-table.tsx
|
| Filename | Overview |
|---|---|
| libs/ui/src/organisms/data-table.tsx | Implements the DataTable controller and rendering pipeline; two previously reported row passthrough and reorder defects remain outstanding. |
| libs/ui/src/organisms/data-table.fields.tsx | Adds typed filter and editor renderers for supported DataTable column types. |
| libs/ui/src/organisms/data-table.helpers.ts | Adds filtering, sizing, pinning, and cell-span helpers plus TanStack type augmentation. |
| libs/ui/src/organisms/table.tsx | Adds declarative start, center, and end alignment styling for headers and cells. |
| libs/ui/stories/organisms/data-table.stories.tsx | Adds DataTable examples and interaction coverage across the component's feature set. |
| libs/ui/package.json | Adds the TanStack and dnd-kit runtime dependencies required by DataTable. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Consumer[Consumer data and column definitions] --> DataTable[DataTable controller]
DataTable --> TanStack[TanStack Table state and row models]
DataTable --> Fields[Typed filters and inline editors]
DataTable --> DnD[dnd-kit row and column reorder]
DataTable --> Virtual[React Virtual windowing]
TanStack --> Table[Presentational Table organism]
Fields --> Table
DnD --> Table
Virtual --> Table
Table --> DOM[Accessible table DOM]
Reviews (37): Last reviewed commit: "docs: translate the DataTable library co..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/data-table/00-library-comparison.md`:
- Around line 13-19: Add a concise legend near the comparison table clarifying
that the “Integrácia do DS” 1–5 scale measures integration effort/cost, with
higher values indicating more effort, rather than library quality. Keep the
existing requirement-matrix legend unchanged.
In `@libs/ui/skills/data-table-usage/SKILL.md`:
- Around line 61-74: Update the Feature flags section in the data-table usage
skill to document enableColumnResizing and the server-driven props
manualSorting, manualFiltering, manualPagination, rowCount, and pageCount,
including their intended usage. After updating the source skill, regenerate the
verbatim mirrored skill through the established sync script.
In `@libs/ui/src/organisms/data-table.helpers.ts`:
- Around line 218-234: Update the public type re-export block in data-table.tsx
to include DataTableInstance and DataTableCellSpan alongside the existing
DataTableGetCellSpan exports, making both types reachable from the published
DataTable entry point.
- Around line 125-148: Update the numeric comparison cases in the data-table
filter helper so an unparseable target value is treated as no constraint,
matching the existing between branch behavior. Check target for NaN before
applying gt, gte, lt, or lte comparisons, while preserving normal comparisons
for valid numeric values and the existing text operators.
In `@libs/ui/src/organisms/data-table.tsx`:
- Around line 143-168: Track a follow-up issue before release to extract the
direct semantic tokens used by the data-table slot definitions into
component-specific --color-data-table-* tokens. Reference the affected slots
sortIcon, dragHandle, and paginationBar, and preserve the current styling until
the token extraction is implemented.
- Around line 313-341: The column drag activator is missing dnd-kit
accessibility attributes. Update SortableHeaderContent’s render-prop type and
payload to include sortable.attributes, then accept and spread attributes on
HeaderDragHandle’s button before listeners; thread this field through
renderHeaderCell’s dnd parameter so the column handle receives role,
aria-roledescription, and aria-describedby.
- Around line 1091-1096: Add the row-actions column as a trailing ColumnDef in
buildColumns when renderRowActions or renderQuickActions is provided, mirroring
the existing built-in columns. Ensure this shared column definition flows
through headerGroups, the filter row, columnCount, and the virtualization spacer
colSpan, while preserving the existing action rendering for each row.
- Around line 874-876: Update the useEffect invoking onReady in the data-table
component to include the stable table instance in its dependency array, so the
callback runs only when that instance changes rather than after every render.
- Around line 240-286: Update DefaultHeaderFilter to derive the fallback
operator from the filter variant: use a valid NUMBER_FILTER_OPERATORS default
for number and range columns, while retaining "contains" for text columns.
Ensure the operator Select value and subsequent setValue behavior always use an
operator present in the selected operators list.
- Around line 927-943: Update handleColumnDragEnd to build the fallback current
order from all table leaf columns, not the visible-only leafColumns collection,
so hidden column IDs remain in columnOrder during reordering. Preserve the
existing active/over validation, arrayMove behavior, and onColumnReorder
callback.
- Around line 107-117: Extend the public exports in data-table.tsx to re-export
the DataTableInstance and DataTableCellSpan types from data-table.helpers
alongside the existing conditional-filter types, so consumers can import these
types without accessing the internal helper module.
- Around line 1115-1123: Update the row-index handling in the renderRows mapping
so getCellSpan receives the row’s index in the full rows array rather than the
virtual slice index. Reuse the virtual item’s original index when available, or
otherwise derive the full-row index before calling renderBodyRow; preserve the
existing sortable and non-sortable rendering paths.
- Around line 945-958: Update handleRowDragEnd to translate the active and over
row-model positions into raw data indices before calling arrayMove. Use each
row’s source index from the rows model (including the existing sorted, filtered,
and paginated mapping) so the moved records in next match the dragged records,
while preserving the current invalid-target early returns and onRowReorder
payload.
- Around line 807-813: Memoize the compiled columns returned by buildColumns in
the surrounding component before passing them to useReactTable, using
dependencies userColumns, enableRowReorder, enableRowSelection, and
enableExpanding. Add useMemo to the React import and preserve the existing
buildColumns arguments and behavior.
- Line 1249: Update the DataTable root element in the component’s render path so
its id is no longer unconditionally hard-coded to the generated instanceId.
Either remove the id attribute or add an optional consumer id prop and use it
with instanceId as the fallback, preserving the existing useId-generated
behavior when no id is supplied.
- Around line 1017-1033: Add a stable key to the non-reorderable header element
returned by renderHeaderCell in the headerGroup.headers mapping, using the same
header identifier as the SortableHeaderContent branch. Keep the existing
reorderable logic and renderHeaderCell behavior unchanged.
In `@libs/ui/stories/organisms/data-table.stories.tsx`:
- Around line 411-420: Replace the native div and span elements in the custom
cell renderer with the appropriate existing layout and text components from
src/. Preserve the current flex-column layout, typography classes, and
firstName, lastName, and email content.
- Around line 521-529: Update the pagination interaction in the story’s play
function to require the page-2 control with a hard role query instead of
queryByRole. Remove the conditional guard so the test always clicks page 2 and
asserts args.onPaginationChange was called.
- Around line 157-165: Update the affected DataTable stories, including
ColumnFiltersWithConditions, so play-function queries target controls explicitly
rendered by each story’s DataTable composition rather than assuming
renderer-generated labels such as “Filter value for firstName.” Add the required
explicit controls or props in the story setup, or narrow each query to a
story-owned control while preserving the existing interaction assertions.
- Around line 226-234: Update the renderRowActions onClick handler to use a
stable stored mock, such as args.onDeleteClick, so play can assert the delete
action was called; do not create and immediately invoke a new fn() instance for
each click.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a67f5df-f41a-421d-bba0-14e05bbb9562
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
docs/data-table/00-library-comparison.mdlibs/ui/agent-plugin/skills/data-table-usage/SKILL.mdlibs/ui/package.jsonlibs/ui/skills/data-table-usage/SKILL.mdlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsxlibs/ui/stories/changelog/changelog.stories.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: storybook-a11y / storybook-a11y
- GitHub Check: Greptile Review
- GitHub Check: main
⚠️ CI failures not shown inline (1)
GitHub Check: Kilo Code Review: Kilo Code Review failed
Conclusion: failure
Review failed: The message could not be delivered
🧰 Additional context used
📓 Path-based instructions (6)
**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Use pnpm CLI to add dependencies; never edit package.json directly
Files:
libs/ui/package.json
libs/**
📄 CodeRabbit inference engine (AGENTS.md)
Use RSLib for building libraries in the monorepo
Files:
libs/ui/package.jsonlibs/ui/stories/changelog/changelog.stories.tsxlibs/ui/skills/data-table-usage/SKILL.mdlibs/ui/agent-plugin/skills/data-table-usage/SKILL.mdlibs/ui/src/organisms/data-table.helpers.tslibs/ui/stories/organisms/data-table.stories.tsxlibs/ui/src/organisms/data-table.tsx
libs/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement UI library components using Zag.js for React and Tailwind CSS for styling
libs/ui/**/*.{ts,tsx}: UI library must use Zag.js for React components with Tailwind CSS styling
Organize UI library components into Atoms, Molecules, and Tokens directories following atomic design pattern
Files:
libs/ui/stories/changelog/changelog.stories.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/stories/organisms/data-table.stories.tsxlibs/ui/src/organisms/data-table.tsx
libs/ui/stories/**/*.stories.{tsx,ts}
📄 CodeRabbit inference engine (libs/ui/AGENTS.md)
libs/ui/stories/**/*.stories.{tsx,ts}: Define Storybook stories using CSF3 format with autodocs support
Use components fromsrc/instead of native HTML elements in Storybook stories
Use semantic and layout tokens in storyclassNameattributes instead of hardcoded Tailwind values
Define Storybook controls only for props that change appearance or behavior; excludeid,ref, internal callbacks
Order Storybook stories as: Playground, Variants (if exist), Sizes (if exist), States (if exist), Component-specific stories
UseVariantContainerandVariantGroupfor visual matrices in Storybook stories
Usefn()fromstorybook/testfor event handlers in Storybook stories
Files:
libs/ui/stories/changelog/changelog.stories.tsxlibs/ui/stories/organisms/data-table.stories.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome linting and formatting only on changed files using 'bunx biome check --write path/to/file'
Files:
libs/ui/stories/changelog/changelog.stories.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/stories/organisms/data-table.stories.tsxlibs/ui/src/organisms/data-table.tsx
libs/ui/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (libs/ui/AGENTS.md)
libs/ui/src/**/*.{tsx,ts}: Usetv()for styling components instead of manual class concatenation
Never useforwardReforuseCallbackfor handlers in components (React 19 doesn't need them)
Usetypefor type definitions instead ofinterface
Never use arbitrary Tailwind values (e.g.,bg-[#ff0000],p-[1rem]); use tokens instead
Never use default exports or barrel files (index.ts) unless the framework requires them
Use data attributes for state styling in Tailwind (e.g.,data-disabled:...,data-[state=open]:...)
Use component-specific token classes in component implementations, not direct semantic tokens likebg-primaryortext-fg
Implement compound components usingComponent.Subcomponent = function ...syntax instead of object exports
Files:
libs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsx
🧠 Learnings (10)
📚 Learning: 2026-02-05T14:43:17.404Z
Learnt from: KaiUweCZE
Repo: NMIT-WR/new-engine PR: 324
File: apps/medusa-be/package.json:0-0
Timestamp: 2026-02-05T14:43:17.404Z
Learning: Validate and enforce React 19 compatibility across monorepo workspaces. Since Medusa UI supports React 19 via root package.json overrides and Medusa Cloud prerequisites show React 19 overrides for npm workspaces, ensure workspace root and all relevant package.json files align with React 19 (18+ requirement is satisfied). When reviewing, verify that overrides exist in the root package.json and that dependent packages in apps or packages directories declare React 19 (or compatible) in their peerDependencies or dependencies as appropriate for workspace usage.
Applied to files:
libs/ui/package.json
📚 Learning: 2026-05-07T19:05:58.339Z
Learnt from: redeyecz
Repo: TechsioCZ/new-engine PR: 390
File: apps/medusa-be/package.json:78-81
Timestamp: 2026-05-07T19:05:58.339Z
Learning: When reviewing changes to `package.json`, do not automatically flag dependency additions/removals as "manually edited" or as "bypassing the pnpm lockfile" just because the `package.json` diff shows only that file changed. First verify whether `pnpm-lock.yaml` is missing the corresponding entries. Since `pnpm add` updates both `package.json` and `pnpm-lock.yaml` together, legitimate changes can appear in the `package.json` diff while still being properly tracked in the lockfile.
Applied to files:
libs/ui/package.json
📚 Learning: 2025-12-16T19:45:17.746Z
Learnt from: BleedingDev
Repo: NMIT-WR/new-engine PR: 207
File: libs/ui/src/molecules/select.tsx:50-50
Timestamp: 2025-12-16T19:45:17.746Z
Learning: When reviewing Tailwind classes in TSX/TS files, prefer using square brackets for arbitrary CSS values and complex expressions. Specifically: - Do not use the parentheses syntax (z-(--z-index)) for anything beyond simple CSS variable references; this syntax auto-wraps in var() and cannot handle calc or complex functions. - Use the square brackets syntax (e.g., h-[calc(var(--available-height)-var(--spacing-content))]) for calc expressions, var with calc, and any complex CSS expressions. This rule applies broadly to Tailwind v4 usage in TSX code across the project.
Applied to files:
libs/ui/stories/changelog/changelog.stories.tsxlibs/ui/stories/organisms/data-table.stories.tsxlibs/ui/src/organisms/data-table.tsx
📚 Learning: 2025-12-29T13:25:52.634Z
Learnt from: KaiUweCZE
Repo: NMIT-WR/new-engine PR: 250
File: libs/ui/src/molecules/slider.tsx:277-285
Timestamp: 2025-12-29T13:25:52.634Z
Learning: In the libs/ui project, when a child component defines a default value for a prop (for example StatusText has status = 'default'), do not override this value from the parent with a nullish coalescing operator (??) or other fallbacks. Centralize defaults in the child component (via default parameter or defaultProps) to ensure a single source of truth. Apply this pattern to TSX components under libs/ui (e.g., any files in libs/ui/src/**/*.tsx).
Applied to files:
libs/ui/stories/changelog/changelog.stories.tsxlibs/ui/stories/organisms/data-table.stories.tsxlibs/ui/src/organisms/data-table.tsx
📚 Learning: 2026-06-12T05:11:34.288Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 439
File: libs/ui/src/molecules/combobox.tsx:96-98
Timestamp: 2026-06-12T05:11:34.288Z
Learning: When reviewing Tailwind `text-*` class usage in `libs/ui` components (including `libs/ui/src/molecules/combobox.tsx`), note that `text-*` may map to Figma tokens that are intentionally namespace-ambiguous: e.g. `text-combobox-trigger` may resolve to a **font-size** token (CSS `font-size`), while `text-combobox-trigger-fg-base` may resolve to a **color** token (CSS `color`). Because they set different CSS properties, both classes may coexist on the same element without overriding each other. Do not flag these as duplicate/dead classes; instead, verify whether the referenced token is a size vs. color (and thus whether they affect `font-size` vs `color`) before raising any concern.
Applied to files:
libs/ui/stories/changelog/changelog.stories.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/stories/organisms/data-table.stories.tsxlibs/ui/src/organisms/data-table.tsx
📚 Learning: 2026-07-24T10:35:29.044Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 515
File: libs/ui/stories/changelog/changelog.stories.tsx:0-0
Timestamp: 2026-07-24T10:35:29.044Z
Learning: In `libs/ui` Storybook stories, follow the layout-wrapper convention: use native semantic/layout HTML elements and apply the project’s Tailwind semantic/layout token class names (not generic text/prose or custom primitives). For preformatted/documentation content, use the semantic HTML `<pre>` element (rather than a non-existent “preformatted-text” primitive), so the markup stays semantically correct and styled via the Tailwind token classes.
Applied to files:
libs/ui/stories/changelog/changelog.stories.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📚 Learning: 2026-07-13T10:44:15.656Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 510
File: libs/ui/agent-plugin/skills/component-usage-ux/SKILL.md:23-31
Timestamp: 2026-07-13T10:44:15.656Z
Learning: In this repo, `libs/ui/agent-plugin/skills/**/SKILL.md` files are generated verbatim mirrors (synced by `libs/ui/agent-plugin/scripts/sync-skills.mjs`) from the source-of-truth docs under `libs/ui/skills/<skill-name>/SKILL.md`. During code review, do not propose content/documentation edits directly to the mirrored `libs/ui/agent-plugin/skills/**/SKILL.md` files; instead, make the change in the corresponding `libs/ui/skills/<skill-name>/SKILL.md` file and submit a PR for that source file.
Applied to files:
libs/ui/agent-plugin/skills/data-table-usage/SKILL.md
📚 Learning: 2026-07-13T10:44:24.386Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 510
File: libs/ui/agent-plugin/skills/numeric-input-usage/SKILL.md:198-205
Timestamp: 2026-07-13T10:44:24.386Z
Learning: Treat `libs/ui/agent-plugin/skills/**/SKILL.md` as generated mirror files (synced from `libs/ui/agent-plugin/scripts/sync-skills.mjs`, which copies verbatim from `libs/ui/skills/**/SKILL.md`). In code reviews, do not recommend edits to these mirror files. If a correction is needed (including fixing command syntax used to generate/search them or correcting the SKILL content), apply the change to the corresponding source file under `libs/ui/skills/**/SKILL.md` instead so it will be reflected by the sync process.
Applied to files:
libs/ui/agent-plugin/skills/data-table-usage/SKILL.md
📚 Learning: 2026-05-11T12:36:23.341Z
Learnt from: BleedingDev
Repo: TechsioCZ/new-engine PR: 404
File: libs/ui/src/molecules/figma/tree-view.figma.tsx:2-2
Timestamp: 2026-05-11T12:36:23.341Z
Learning: Inside the `libs/ui` library source (e.g., under `libs/ui/src/`), require component imports to use relative paths (such as `../tree-view`) instead of the `libs/ui` alias. Treat `libs/ui/...` imports as reserved for external consumers of the published library; do not flag them or suggest replacing them with relative imports when they appear in consumer code or in contexts outside the library source.
Applied to files:
libs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsx
📚 Learning: 2026-06-15T07:26:29.844Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 448
File: libs/ui/src/organisms/footer.tsx:0-0
Timestamp: 2026-06-15T07:26:29.844Z
Learning: In `libs/ui` components, when using Tailwind v4 border-width utilities with a CSS variable (e.g., `--border-footer-width`, `--border-table-width`), always use the explicit `length:` hint syntax so the variable is treated as a border *width* (e.g., `border-t-(length:--border-footer-width)`, `border-b-(length:--border-table-width)`). If you use bare `(--var)` without `length:`, Tailwind v4 interprets it as a border *color* and the width will not be applied. Apply this rule to all border-width utilities: `border-t`, `border-b`, `border-l`, `border-r`, and `border`.
Applied to files:
libs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsx
🪛 LanguageTool
libs/ui/skills/data-table-usage/SKILL.md
[uncategorized] ~87-~87: Loose punctuation mark.
Context: ...(open DOM paths) renderToolbar(table), renderEmpty(), `renderRowActions(row)...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~95-~95: Loose punctuation mark.
Context: ...Pass getCellSpan(cell, ctx) returning { colSpan?, rowSpan?, hidden? }; mark cells swallowed by a span with `...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~113-~113: Would you like to use the Oxford spelling “tokenized”? The spelling ‘tokenised’ is also correct.
Context: ...code colors/padding — the grid is fully tokenised via Table. - Do not reach for a canva...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~115-~115: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... they cannot use our Tailwind tokens. - Do not mutate data in place for row reor...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/data-table/00-library-comparison.md
[typographical] ~21-~21: Nepárový symbol: zdá sa, že chýba '“'
Context: .../ 5 |
(UNPAIRED_BRACKETS)
[typographical] ~21-~21: Nepárový symbol: zdá sa, že chýba '"'
Context: ...less | Upresnenie k „Table od Alibaby": to, na čo sme mysleli (`ali-react-ta...
(UNPAIRED_BRACKETS)
[typographical] ~21-~21: Nepárový symbol: zdá sa, že chýba '“'
Context: ...v context7 nulové pokrytie. Reálny živý „Alibaba/Ant Group" table je AntV S2 ...
(UNPAIRED_BRACKETS)
[typographical] ~21-~21: Nepárový symbol: zdá sa, že chýba '"'
Context: ...pokrytie. Reálny živý „Alibaba/Ant Group" table je AntV S2 — ale je to canvas...
(UNPAIRED_BRACKETS)
[uncategorized] ~25-~25:
Dajte čiarku pred „ako“: ", ako".
Context: ...m pre náš DS VTable aj S2 kreslia bunky ako pixely na jeden <canvas>: - **Ži...
(AKO)
[typographical] ~28-~28: Nepárový symbol: zdá sa, že chýba '“'
Context: ...ky. - Štýlovanie ide cez paralelný JS „theme" objekt (themeCfg, VRender sty...
(UNPAIRED_BRACKETS)
[typographical] ~28-~28: Nepárový symbol: zdá sa, že chýba '"'
Context: ...Štýlovanie ide cez paralelný JS „theme" objekt (themeCfg, VRender style obj...
(UNPAIRED_BRACKETS)
[typographical] ~29-~29: Nepárový symbol: zdá sa, že chýba '“'
Context: ...ho jazyka a udržiavať bridge adaptér. - „Nested vrstvy cez propsy" reálne neexist...
(UNPAIRED_BRACKETS)
[typographical] ~29-~29: Nepárový symbol: zdá sa, že chýba '"'
Context: ...dge adaptér. - „Nested vrstvy cez propsy" reálne neexistujú — dostaneš len **over...
(UNPAIRED_BRACKETS)
[typographical] ~30-~30: Nepárový symbol: zdá sa, že chýba ')'
Context: ...upnosť (ARIA na elementoch) a debugging (S2 potrebuje špeciálny „G devtools" plug...
(UNPAIRED_BRACKETS)
[typographical] ~30-~30: Nepárový symbol: zdá sa, že chýba '“'
Context: ...ch) a debugging (S2 potrebuje špeciálny „G devtools" plugin). To je filozofický ...
(UNPAIRED_BRACKETS)
[typographical] ~30-~30: Nepárový symbol: zdá sa, že chýba '"'
Context: ...ging (S2 potrebuje špeciálny „G devtools" plugin). To je filozofický opak Zag.js...
(UNPAIRED_BRACKETS)
[typographical] ~30-~30: Nepárový symbol: zdá sa, že chýba '('
Context: ... potrebuje špeciálny „G devtools" plugin). To je filozofický opak Zag.js + Tailw...
(UNPAIRED_BRACKETS)
[typographical] ~32-~32: Nepárový symbol: zdá sa, že chýba '“'
Context: ... + Tailwind DS, ktorého celá premisa je „ty vlastníš DOM a štýl, my vlastníme spr...
(UNPAIRED_BRACKETS)
[typographical] ~32-~32: Nepárový symbol: zdá sa, že chýba '"'
Context: ...stníš DOM a štýl, my vlastníme správanie". ## Matica požiadaviek (natívna podpor...
(UNPAIRED_BRACKETS)
[typographical] ~62-~62: Nepárový symbol: zdá sa, že chýba '“'
Context: ...* VTable a S2 vyhrávajú na počte ✅ — sú „batteries-included". TanStack má viac 🟡...
(UNPAIRED_BRACKETS)
[typographical] ~62-~62: Nepárový symbol: zdá sa, že chýba '"'
Context: ...vajú na počte ✅ — sú „batteries-included". TanStack má viac 🟡/⚙️, lebo **zámerne...
(UNPAIRED_BRACKETS)
[uncategorized] ~66-~66:
Dajte čiarku pred „ako“: ", ako".
Context: ... TanStack — 1–2/5: rovnaká filozofia ako Zag.js (len logika, nula markupu/CSS). ...
(AKO)
[uncategorized] ~75-~75:
Dajte čiarku pred „ako“: ", ako".
Context: ...stovateľnosti cez DOM. Dávajú zmysel len ako izolovaný canvas analytics widget m...
(AKO)
[uncategorized] ~75-~75:
Dajte čiarku pred „ako“: ", Ako".
Context: ...o DS (canvas-scale 100k+ buniek, pivot). Ako sme pokryli slabé miesta TanStacku: -...
(AKO)
libs/ui/agent-plugin/skills/data-table-usage/SKILL.md
[uncategorized] ~87-~87: Loose punctuation mark.
Context: ...(open DOM paths) renderToolbar(table), renderEmpty(), `renderRowActions(row)...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~95-~95: Loose punctuation mark.
Context: ...Pass getCellSpan(cell, ctx) returning { colSpan?, rowSpan?, hidden? }; mark cells swallowed by a span with `...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~113-~113: Would you like to use the Oxford spelling “tokenized”? The spelling ‘tokenised’ is also correct.
Context: ...code colors/padding — the grid is fully tokenised via Table. - Do not reach for a canva...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~115-~115: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... they cannot use our Tailwind tokens. - Do not mutate data in place for row reor...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (12)
libs/ui/stories/changelog/changelog.stories.tsx (1)
23-25: LGTM!libs/ui/stories/organisms/data-table.stories.tsx (3)
1-155: LGTM!Also applies to: 167-224, 236-330, 341-353, 362-364, 374-376, 386-388, 402-410, 421-472, 485-520
473-484: 🗄️ Data Integrity & IntegrationNo change needed.
DataTablealready wiresmeta.updateDatato callonCellEditCommit, soEditableRoleCell’s call path is supported.
244-254: 🗄️ Data Integrity & Integrationchore: no change needed —
ActionIcon.iconalready accepts raw Iconify classes.
ActionIconpasses theiconprop throughIcon, whoseIconTypeaccepts bothtoken-icon-${string}andicon-[${string}], soicon="icon-[mdi--email-outline]"is valid.docs/data-table/00-library-comparison.md (1)
23-32: LGTM!Also applies to: 71-83
libs/ui/skills/data-table-usage/SKILL.md (2)
93-115: LGTM!
34-59: 📐 Maintainability & Code QualityNo change needed. The documented
@techsio/ui-kit/organisms/data-tableimport matches the./organisms/*export.libs/ui/src/organisms/data-table.helpers.ts (2)
17-50: LGTM!
185-216: LGTM!libs/ui/src/organisms/data-table.tsx (2)
1275-1355: LGTM!Also applies to: 1359-1465
1202-1237: 🩺 Stability & AvailabilityKeep column and row reorders in separate
DndContextproviders rather than merging them.Nested providers are supported when each is responsible for an independent drag-and-drop interface. Putting both axes under one provider would risk dragging a column vertically or a row horizontally because the same sortable context would apply both modifiers; the current row context wrapping the column provider is the intended containment for independent axes.
libs/ui/package.json (1)
81-90: 🔒 Security & PrivacyVerify the dnd-kit peer compatibility with React 19 before publishing.
@dnd-kit/core@6.3.1,@dnd-kit/modifiers@9.0.0,@dnd-kit/sortable@10.0.0, and@dnd-kit/utilities@3.2.2all publishreact >=16.8.0, and these are hard dependencies for@techsio/ui-kit; React 18-only consumers may pass, but React 19 consumers need compatibility checked. No advisories surfaced for these published versions.
Storybook A11y Report
Light
By group
New violations (101)
Resolved violations (0)No resolved violations. Dark
By group
New violations (101)
Resolved violations (0)No resolved violations. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/ui/src/organisms/data-table.fields.tsx`:
- Around line 612-644: Update matchNumber so unparseable non-between filter
values are treated as having no constraint, matching the existing between-bound
behavior. After checking for a blank value, validate target with Number.isNaN
and return true before the operator switch when it cannot be parsed; preserve
all existing operator comparisons for valid numeric targets.
- Around line 302-336: Clarify the intended one-bound behavior in the datetime
renderer’s setValue callback by adding a short comment explaining that `{ from
}` deliberately creates an open-ended filter matching values at or after the
selected instant; do not alter the existing date or datetime filtering logic.
In `@libs/ui/src/organisms/data-table.tsx`:
- Around line 755-781: Route all specified user-facing strings in
libs/ui/src/organisms/data-table.tsx through the resolved DataTableTranslations
contract while preserving the current English defaults: update validateDraft at
lines 755-781 to receive translations and use its required-field message, source
the reorder aria-label at lines 665-699 from translations, translate Save row,
Cancel edit, and the Edit row ${row.id} label at lines 1350-1394, and thread
translations into buildColumns at lines 1696-1727 for Select all rows and Select
row ${row.id}.
- Around line 1396-1414: Update renderActionsCell to remove the inline sticky
position style containing right: 0, while preserving the sticky end-0 class and
existing conditional class behavior so the actions cell remains pinned logically
in both LTR and RTL layouts.
- Around line 1426-1427: Update the colSpan calculations in the row rendering
and empty-state paths to reuse the existing hasActionsColumn predicate,
including enableInlineEdit, instead of independently checking renderRowActions
or renderQuickActions. Keep the expanded-row and body-cell spans consistent for
inline-edit-only tables.
- Around line 518-525: Update the DataTable paginationProps type to make the
omitted PaginationProps passthrough partial, including getPageUrl and all other
optional overrides, while keeping count, page, pageSize, onPageChange, and
onChange owned by the table.
In `@libs/ui/stories/organisms/data-table.stories.tsx`:
- Around line 571-577: Run Biome formatting and linting with `bunx biome check
--write` on the changed data-table story file, ensuring the `employees` fixtures
and related assertions are reformatted according to repository guidelines.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6f1c21e7-fd58-4286-8696-a0eb5f3bd0e6
📒 Files selected for processing (7)
docs/data-table/00-library-comparison.mdlibs/ui/agent-plugin/skills/data-table-usage/SKILL.mdlibs/ui/skills/data-table-usage/SKILL.mdlibs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: storybook-a11y / storybook-a11y
⚠️ CI failures not shown inline (1)
GitHub Check: Kilo Code Review: Kilo Code Review failed
Conclusion: failure
Review failed: The message could not be delivered
🧰 Additional context used
📓 Path-based instructions (5)
libs/**
📄 CodeRabbit inference engine (AGENTS.md)
Use RSLib for building libraries in the monorepo
Files:
libs/ui/skills/data-table-usage/SKILL.mdlibs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/agent-plugin/skills/data-table-usage/SKILL.mdlibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
libs/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement UI library components using Zag.js for React and Tailwind CSS for styling
libs/ui/**/*.{ts,tsx}: UI library must use Zag.js for React components with Tailwind CSS styling
Organize UI library components into Atoms, Molecules, and Tokens directories following atomic design pattern
Files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
libs/ui/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (libs/ui/AGENTS.md)
libs/ui/src/**/*.{tsx,ts}: Usetv()for styling components instead of manual class concatenation
Never useforwardReforuseCallbackfor handlers in components (React 19 doesn't need them)
Usetypefor type definitions instead ofinterface
Never use arbitrary Tailwind values (e.g.,bg-[#ff0000],p-[1rem]); use tokens instead
Never use default exports or barrel files (index.ts) unless the framework requires them
Use data attributes for state styling in Tailwind (e.g.,data-disabled:...,data-[state=open]:...)
Use component-specific token classes in component implementations, not direct semantic tokens likebg-primaryortext-fg
Implement compound components usingComponent.Subcomponent = function ...syntax instead of object exports
Files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome linting and formatting only on changed files using 'bunx biome check --write path/to/file'
Files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
libs/ui/stories/**/*.stories.{tsx,ts}
📄 CodeRabbit inference engine (libs/ui/AGENTS.md)
libs/ui/stories/**/*.stories.{tsx,ts}: Define Storybook stories using CSF3 format with autodocs support
Use components fromsrc/instead of native HTML elements in Storybook stories
Use semantic and layout tokens in storyclassNameattributes instead of hardcoded Tailwind values
Define Storybook controls only for props that change appearance or behavior; excludeid,ref, internal callbacks
Order Storybook stories as: Playground, Variants (if exist), Sizes (if exist), States (if exist), Component-specific stories
UseVariantContainerandVariantGroupfor visual matrices in Storybook stories
Usefn()fromstorybook/testfor event handlers in Storybook stories
Files:
libs/ui/stories/organisms/data-table.stories.tsx
🧠 Learnings (8)
📚 Learning: 2025-12-16T19:45:17.746Z
Learnt from: BleedingDev
Repo: NMIT-WR/new-engine PR: 207
File: libs/ui/src/molecules/select.tsx:50-50
Timestamp: 2025-12-16T19:45:17.746Z
Learning: When reviewing Tailwind classes in TSX/TS files, prefer using square brackets for arbitrary CSS values and complex expressions. Specifically: - Do not use the parentheses syntax (z-(--z-index)) for anything beyond simple CSS variable references; this syntax auto-wraps in var() and cannot handle calc or complex functions. - Use the square brackets syntax (e.g., h-[calc(var(--available-height)-var(--spacing-content))]) for calc expressions, var with calc, and any complex CSS expressions. This rule applies broadly to Tailwind v4 usage in TSX code across the project.
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📚 Learning: 2025-12-29T13:25:52.634Z
Learnt from: KaiUweCZE
Repo: NMIT-WR/new-engine PR: 250
File: libs/ui/src/molecules/slider.tsx:277-285
Timestamp: 2025-12-29T13:25:52.634Z
Learning: In the libs/ui project, when a child component defines a default value for a prop (for example StatusText has status = 'default'), do not override this value from the parent with a nullish coalescing operator (??) or other fallbacks. Centralize defaults in the child component (via default parameter or defaultProps) to ensure a single source of truth. Apply this pattern to TSX components under libs/ui (e.g., any files in libs/ui/src/**/*.tsx).
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📚 Learning: 2026-05-11T12:36:23.341Z
Learnt from: BleedingDev
Repo: TechsioCZ/new-engine PR: 404
File: libs/ui/src/molecules/figma/tree-view.figma.tsx:2-2
Timestamp: 2026-05-11T12:36:23.341Z
Learning: Inside the `libs/ui` library source (e.g., under `libs/ui/src/`), require component imports to use relative paths (such as `../tree-view`) instead of the `libs/ui` alias. Treat `libs/ui/...` imports as reserved for external consumers of the published library; do not flag them or suggest replacing them with relative imports when they appear in consumer code or in contexts outside the library source.
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsx
📚 Learning: 2026-06-15T07:26:29.844Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 448
File: libs/ui/src/organisms/footer.tsx:0-0
Timestamp: 2026-06-15T07:26:29.844Z
Learning: In `libs/ui` components, when using Tailwind v4 border-width utilities with a CSS variable (e.g., `--border-footer-width`, `--border-table-width`), always use the explicit `length:` hint syntax so the variable is treated as a border *width* (e.g., `border-t-(length:--border-footer-width)`, `border-b-(length:--border-table-width)`). If you use bare `(--var)` without `length:`, Tailwind v4 interprets it as a border *color* and the width will not be applied. Apply this rule to all border-width utilities: `border-t`, `border-b`, `border-l`, `border-r`, and `border`.
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsx
📚 Learning: 2026-06-12T05:11:34.288Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 439
File: libs/ui/src/molecules/combobox.tsx:96-98
Timestamp: 2026-06-12T05:11:34.288Z
Learning: When reviewing Tailwind `text-*` class usage in `libs/ui` components (including `libs/ui/src/molecules/combobox.tsx`), note that `text-*` may map to Figma tokens that are intentionally namespace-ambiguous: e.g. `text-combobox-trigger` may resolve to a **font-size** token (CSS `font-size`), while `text-combobox-trigger-fg-base` may resolve to a **color** token (CSS `color`). Because they set different CSS properties, both classes may coexist on the same element without overriding each other. Do not flag these as duplicate/dead classes; instead, verify whether the referenced token is a size vs. color (and thus whether they affect `font-size` vs `color`) before raising any concern.
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📚 Learning: 2026-07-13T10:44:15.656Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 510
File: libs/ui/agent-plugin/skills/component-usage-ux/SKILL.md:23-31
Timestamp: 2026-07-13T10:44:15.656Z
Learning: In this repo, `libs/ui/agent-plugin/skills/**/SKILL.md` files are generated verbatim mirrors (synced by `libs/ui/agent-plugin/scripts/sync-skills.mjs`) from the source-of-truth docs under `libs/ui/skills/<skill-name>/SKILL.md`. During code review, do not propose content/documentation edits directly to the mirrored `libs/ui/agent-plugin/skills/**/SKILL.md` files; instead, make the change in the corresponding `libs/ui/skills/<skill-name>/SKILL.md` file and submit a PR for that source file.
Applied to files:
libs/ui/agent-plugin/skills/data-table-usage/SKILL.md
📚 Learning: 2026-07-13T10:44:24.386Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 510
File: libs/ui/agent-plugin/skills/numeric-input-usage/SKILL.md:198-205
Timestamp: 2026-07-13T10:44:24.386Z
Learning: Treat `libs/ui/agent-plugin/skills/**/SKILL.md` as generated mirror files (synced from `libs/ui/agent-plugin/scripts/sync-skills.mjs`, which copies verbatim from `libs/ui/skills/**/SKILL.md`). In code reviews, do not recommend edits to these mirror files. If a correction is needed (including fixing command syntax used to generate/search them or correcting the SKILL content), apply the change to the corresponding source file under `libs/ui/skills/**/SKILL.md` instead so it will be reflected by the sync process.
Applied to files:
libs/ui/agent-plugin/skills/data-table-usage/SKILL.md
📚 Learning: 2026-07-24T10:35:29.044Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 515
File: libs/ui/stories/changelog/changelog.stories.tsx:0-0
Timestamp: 2026-07-24T10:35:29.044Z
Learning: In `libs/ui` Storybook stories, follow the layout-wrapper convention: use native semantic/layout HTML elements and apply the project’s Tailwind semantic/layout token class names (not generic text/prose or custom primitives). For preformatted/documentation content, use the semantic HTML `<pre>` element (rather than a non-existent “preformatted-text” primitive), so the markup stays semantically correct and styled via the Tailwind token classes.
Applied to files:
libs/ui/stories/organisms/data-table.stories.tsx
🪛 LanguageTool
docs/data-table/00-library-comparison.md
[typographical] ~21-~21: Nepárový symbol: zdá sa, že chýba '“'
Context: ... | 3 / 5 |
(UNPAIRED_BRACKETS)
[typographical] ~21-~21: Nepárový symbol: zdá sa, že chýba '"'
Context: ... headless | > **Škála „Integrácia do DS" = náklad/úsilie integrácie (1 = najmene...
(UNPAIRED_BRACKETS)
🔇 Additional comments (17)
libs/ui/src/organisms/data-table.tsx (7)
962-970: 🚀 Performance & Scalability | ⚡ Quick win
columnsis still rebuilt on every render.
buildColumnsreturns a fresh array each pass, so TanStack Table re-derives its column model on every keystroke; wrapping this inuseMemokeyed onuserColumns, the three feature flags,lockedandsizewould keep the reference stable. This repeats an earlier review note on the same call site.
1289-1296: 🎯 Functional Correctness | ⚡ Quick winHeader cells in the non-reorderable branch still have no
key.
renderHeaderCell(header)returns a bareTable.ColumnHeader, so the default (non-reorderable) path renders a keyless list and React will warn for every header group. Previously raised; still outstanding.
84-104: LGTM!Also applies to: 134-180
204-249: LGTM!
316-341: LGTM!
1146-1186: LGTM!
1076-1095: 🩺 Stability & AvailabilityNo change needed
The component does not expose
lockInteractionsWhileEditing, so this escape-hatch scenario is not supported by the current API.> Likely an incorrect or invalid review comment.libs/ui/stories/organisms/data-table.stories.tsx (6)
8-23: LGTM!
234-254: LGTM!
527-543: LGTM!
715-774: LGTM!
19-23: 🎯 Functional CorrectnessNo change needed;
betweenbounds are already coerced numerically.The
typedFilterFnpath convertsvalueandtowithNumber()before comparing salary ranges, so the string bounds do not cause lexicographic filtering.> Likely an incorrect or invalid review comment.
645-651: 🎯 Functional CorrectnessDefault filter
aria-labelstrings are correct.The story queries for
Filter active,Filter shiftStart from, andFilter startDatematch the default renderer labels.libs/ui/agent-plugin/skills/data-table-usage/SKILL.md (1)
24-177: Mirror file — reviewed via the source instead.This file is a generated mirror of
libs/ui/skills/data-table-usage/SKILL.md; content is identical, so the substantive review is on the source file. No edits proposed here.Based on learnings that
libs/ui/agent-plugin/skills/**/SKILL.mdfiles are generated verbatim mirrors synced fromlibs/ui/skills/<skill-name>/SKILL.md, and edits should target the source file instead.Source: Learnings
docs/data-table/00-library-comparison.md (1)
21-22: LGTM!libs/ui/skills/data-table-usage/SKILL.md (1)
61-177: LGTM! Both previously flagged gaps (enableColumnResizingdocs and the manual/server-driven props) are now covered, and the type/escape-hatch tables line up with thedata-table.fields.tsxrenderer contract.libs/ui/src/organisms/data-table.helpers.ts (1)
15-92: LGTM! TheColumnMeta/FilterFnsaugmentation andtypedFilterFnline up correctly with thedata-table.fields.tsxtype contracts and the downstreamfilterFnsregistration indata-table.tsx.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
libs/ui/src/organisms/data-table.tsx (2)
276-317: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winColumn drag handle still doesn't receive dnd-kit's
attributes.
SortableHeaderContent's children payload omitsattributes, and theHeaderDragHandlecall site doesn't pass it either — so the column drag button still ends up withoutrole,aria-roledescription,aria-describedby, unlikeSortableRow(line 355:...sortable.attributes) which does this correctly for rows. This is the same gap raised in a previous review round (marked as addressed in an earlier commit), but the code shown here still lacks the fix — please double-check whether that fix regressed or never landed.🐛 Proposed fix
children: (args: { setActivatorNodeRef: (node: HTMLElement | null) => void listeners: Record<string, unknown> | undefined style: CSSProperties setNodeRef: (node: HTMLElement | null) => void isDragging: boolean dropSide?: "start" | "end" + attributes: Record<string, unknown> | undefined }) => ReactNode }) { const sortable = useSortable({ id: columnId }) ... return ( <> {children({ setActivatorNodeRef: sortable.setActivatorNodeRef, listeners: sortable.listeners, style, setNodeRef: sortable.setNodeRef, isDragging: sortable.isDragging, dropSide, + attributes: sortable.attributes, })} </> ) }Then thread
attributes={dnd.attributes}intoHeaderDragHandleat the call site (~line 1486) and have it spreadattributeson the<button>beforelisteners.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui/src/organisms/data-table.tsx` around lines 276 - 317, Update SortableHeaderContent to include dnd-kit attributes in its children payload, thread sortable.attributes through the HeaderDragHandle call site, and spread those attributes onto the drag handle button before the listeners. Preserve the existing column drag behavior and styling.
1136-1146: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winMemoise
buildColumns's output before handing it touseReactTable.
buildColumns<T>(...)is invoked directly in the render body, producing a new array (plus a freshonBlockedSelectclosure) on every render. TanStack Table relies on a stablecolumnsreference to avoid unnecessary internal churn — this was flagged before without a confirmed fix.🐛 Proposed fix
- const columns = buildColumns<T>({ - userColumns, - enableRowReorder, - enableRowSelection, - enableExpanding, - locked, - size, - showSelectAll: selectionMode === "multiple" && maxSelectedRows == null, - onBlockedSelect: () => blocked("select"), - }) + const columns = useMemo( + () => + buildColumns<T>({ + userColumns, + enableRowReorder, + enableRowSelection, + enableExpanding, + locked, + size, + showSelectAll: selectionMode === "multiple" && maxSelectedRows == null, + onBlockedSelect: () => blocked("select"), + }), + [userColumns, enableRowReorder, enableRowSelection, enableExpanding, locked, size, selectionMode, maxSelectedRows] + )TanStack Table v8 stable columns reference requirement 2026🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui/src/organisms/data-table.tsx` around lines 1136 - 1146, Memoize the columns result created by buildColumns<T> before it is passed to useReactTable, including all current inputs and the onBlockedSelect callback in the dependency list. Preserve the existing column configuration and ensure the memoized reference remains stable when those inputs have not changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/ui/src/organisms/data-table.fields.tsx`:
- Line 16: Replace the inline CSSProperties-based OPERATOR_WIDTH and VALUE_WIDTH
styling in the filter-row layout with tv() variants or slots that define the
corresponding width tokens. Update the affected filter controls and remove the
now-unused CSSProperties import, preserving the existing 7rem and 6rem widths
through token-driven classes.
In `@libs/ui/src/organisms/data-table.tsx`:
- Around line 2022-2031: Replace the hard-coded "Select all rows" aria label in
the data-table column header with the corresponding value from
DataTableTranslations, using the existing translations object passed into the
component. Preserve the current selection behavior and fallback behavior if the
translation contract defines one.
- Line 1016: Add an optional id override to DataTableProps and update the
DataTable root wrapper to use the consumer-provided id when present, falling
back to the generated instanceId from useId() otherwise. Ensure both affected
root-element usages follow this precedence.
- Around line 1548-1584: Update the filter-row cells in the enableColumnFilters
rendering to apply the same column pinning class and horizontal styles used by
the header and body cells, using getPinningStyles with the filter-row context as
appropriate. Preserve the existing vertical stickyHeader positioning and ensure
the actions filter cell continues using its existing stickyActions behavior.
- Around line 1041-1051: Update the row-selection change handler around
onRowSelectionChange to track the previous selected count or capped state, and
invoke onSelectionLimitReached only when the count transitions from below
maxSelectedRows to at least maxSelectedRows. Preserve normal selection updates
and allow the callback to fire again after the selection falls below the limit
and later reaches it.
---
Duplicate comments:
In `@libs/ui/src/organisms/data-table.tsx`:
- Around line 276-317: Update SortableHeaderContent to include dnd-kit
attributes in its children payload, thread sortable.attributes through the
HeaderDragHandle call site, and spread those attributes onto the drag handle
button before the listeners. Preserve the existing column drag behavior and
styling.
- Around line 1136-1146: Memoize the columns result created by buildColumns<T>
before it is passed to useReactTable, including all current inputs and the
onBlockedSelect callback in the dependency list. Preserve the existing column
configuration and ensure the memoized reference remains stable when those inputs
have not changed.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2197607-8a52-4cb8-9996-1fa4b7d682f1
📒 Files selected for processing (6)
libs/ui/agent-plugin/skills/data-table-usage/SKILL.mdlibs/ui/skills/data-table-usage/SKILL.mdlibs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: storybook-a11y / storybook-a11y
⚠️ CI failures not shown inline (1)
GitHub Check: Kilo Code Review: Kilo Code Review failed
Conclusion: failure
Review failed: The message could not be delivered
🧰 Additional context used
📓 Path-based instructions (5)
libs/**
📄 CodeRabbit inference engine (AGENTS.md)
Use RSLib for building libraries in the monorepo
Files:
libs/ui/agent-plugin/skills/data-table-usage/SKILL.mdlibs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/skills/data-table-usage/SKILL.mdlibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
libs/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement UI library components using Zag.js for React and Tailwind CSS for styling
libs/ui/**/*.{ts,tsx}: UI library must use Zag.js for React components with Tailwind CSS styling
Organize UI library components into Atoms, Molecules, and Tokens directories following atomic design pattern
Files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
libs/ui/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (libs/ui/AGENTS.md)
libs/ui/src/**/*.{tsx,ts}: Usetv()for styling components instead of manual class concatenation
Never useforwardReforuseCallbackfor handlers in components (React 19 doesn't need them)
Usetypefor type definitions instead ofinterface
Never use arbitrary Tailwind values (e.g.,bg-[#ff0000],p-[1rem]); use tokens instead
Never use default exports or barrel files (index.ts) unless the framework requires them
Use data attributes for state styling in Tailwind (e.g.,data-disabled:...,data-[state=open]:...)
Use component-specific token classes in component implementations, not direct semantic tokens likebg-primaryortext-fg
Implement compound components usingComponent.Subcomponent = function ...syntax instead of object exports
Files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome linting and formatting only on changed files using 'bunx biome check --write path/to/file'
Files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
libs/ui/stories/**/*.stories.{tsx,ts}
📄 CodeRabbit inference engine (libs/ui/AGENTS.md)
libs/ui/stories/**/*.stories.{tsx,ts}: Define Storybook stories using CSF3 format with autodocs support
Use components fromsrc/instead of native HTML elements in Storybook stories
Use semantic and layout tokens in storyclassNameattributes instead of hardcoded Tailwind values
Define Storybook controls only for props that change appearance or behavior; excludeid,ref, internal callbacks
Order Storybook stories as: Playground, Variants (if exist), Sizes (if exist), States (if exist), Component-specific stories
UseVariantContainerandVariantGroupfor visual matrices in Storybook stories
Usefn()fromstorybook/testfor event handlers in Storybook stories
Files:
libs/ui/stories/organisms/data-table.stories.tsx
🧠 Learnings (8)
📚 Learning: 2026-07-13T10:44:15.656Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 510
File: libs/ui/agent-plugin/skills/component-usage-ux/SKILL.md:23-31
Timestamp: 2026-07-13T10:44:15.656Z
Learning: In this repo, `libs/ui/agent-plugin/skills/**/SKILL.md` files are generated verbatim mirrors (synced by `libs/ui/agent-plugin/scripts/sync-skills.mjs`) from the source-of-truth docs under `libs/ui/skills/<skill-name>/SKILL.md`. During code review, do not propose content/documentation edits directly to the mirrored `libs/ui/agent-plugin/skills/**/SKILL.md` files; instead, make the change in the corresponding `libs/ui/skills/<skill-name>/SKILL.md` file and submit a PR for that source file.
Applied to files:
libs/ui/agent-plugin/skills/data-table-usage/SKILL.md
📚 Learning: 2026-07-13T10:44:24.386Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 510
File: libs/ui/agent-plugin/skills/numeric-input-usage/SKILL.md:198-205
Timestamp: 2026-07-13T10:44:24.386Z
Learning: Treat `libs/ui/agent-plugin/skills/**/SKILL.md` as generated mirror files (synced from `libs/ui/agent-plugin/scripts/sync-skills.mjs`, which copies verbatim from `libs/ui/skills/**/SKILL.md`). In code reviews, do not recommend edits to these mirror files. If a correction is needed (including fixing command syntax used to generate/search them or correcting the SKILL content), apply the change to the corresponding source file under `libs/ui/skills/**/SKILL.md` instead so it will be reflected by the sync process.
Applied to files:
libs/ui/agent-plugin/skills/data-table-usage/SKILL.md
📚 Learning: 2025-12-16T19:45:17.746Z
Learnt from: BleedingDev
Repo: NMIT-WR/new-engine PR: 207
File: libs/ui/src/molecules/select.tsx:50-50
Timestamp: 2025-12-16T19:45:17.746Z
Learning: When reviewing Tailwind classes in TSX/TS files, prefer using square brackets for arbitrary CSS values and complex expressions. Specifically: - Do not use the parentheses syntax (z-(--z-index)) for anything beyond simple CSS variable references; this syntax auto-wraps in var() and cannot handle calc or complex functions. - Use the square brackets syntax (e.g., h-[calc(var(--available-height)-var(--spacing-content))]) for calc expressions, var with calc, and any complex CSS expressions. This rule applies broadly to Tailwind v4 usage in TSX code across the project.
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📚 Learning: 2025-12-29T13:25:52.634Z
Learnt from: KaiUweCZE
Repo: NMIT-WR/new-engine PR: 250
File: libs/ui/src/molecules/slider.tsx:277-285
Timestamp: 2025-12-29T13:25:52.634Z
Learning: In the libs/ui project, when a child component defines a default value for a prop (for example StatusText has status = 'default'), do not override this value from the parent with a nullish coalescing operator (??) or other fallbacks. Centralize defaults in the child component (via default parameter or defaultProps) to ensure a single source of truth. Apply this pattern to TSX components under libs/ui (e.g., any files in libs/ui/src/**/*.tsx).
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📚 Learning: 2026-05-11T12:36:23.341Z
Learnt from: BleedingDev
Repo: TechsioCZ/new-engine PR: 404
File: libs/ui/src/molecules/figma/tree-view.figma.tsx:2-2
Timestamp: 2026-05-11T12:36:23.341Z
Learning: Inside the `libs/ui` library source (e.g., under `libs/ui/src/`), require component imports to use relative paths (such as `../tree-view`) instead of the `libs/ui` alias. Treat `libs/ui/...` imports as reserved for external consumers of the published library; do not flag them or suggest replacing them with relative imports when they appear in consumer code or in contexts outside the library source.
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsx
📚 Learning: 2026-06-15T07:26:29.844Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 448
File: libs/ui/src/organisms/footer.tsx:0-0
Timestamp: 2026-06-15T07:26:29.844Z
Learning: In `libs/ui` components, when using Tailwind v4 border-width utilities with a CSS variable (e.g., `--border-footer-width`, `--border-table-width`), always use the explicit `length:` hint syntax so the variable is treated as a border *width* (e.g., `border-t-(length:--border-footer-width)`, `border-b-(length:--border-table-width)`). If you use bare `(--var)` without `length:`, Tailwind v4 interprets it as a border *color* and the width will not be applied. Apply this rule to all border-width utilities: `border-t`, `border-b`, `border-l`, `border-r`, and `border`.
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsx
📚 Learning: 2026-06-12T05:11:34.288Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 439
File: libs/ui/src/molecules/combobox.tsx:96-98
Timestamp: 2026-06-12T05:11:34.288Z
Learning: When reviewing Tailwind `text-*` class usage in `libs/ui` components (including `libs/ui/src/molecules/combobox.tsx`), note that `text-*` may map to Figma tokens that are intentionally namespace-ambiguous: e.g. `text-combobox-trigger` may resolve to a **font-size** token (CSS `font-size`), while `text-combobox-trigger-fg-base` may resolve to a **color** token (CSS `color`). Because they set different CSS properties, both classes may coexist on the same element without overriding each other. Do not flag these as duplicate/dead classes; instead, verify whether the referenced token is a size vs. color (and thus whether they affect `font-size` vs `color`) before raising any concern.
Applied to files:
libs/ui/src/organisms/data-table.fields.tsxlibs/ui/src/organisms/data-table.helpers.tslibs/ui/src/organisms/data-table.tsxlibs/ui/stories/organisms/data-table.stories.tsx
📚 Learning: 2026-07-24T10:35:29.044Z
Learnt from: Luko248
Repo: TechsioCZ/new-engine PR: 515
File: libs/ui/stories/changelog/changelog.stories.tsx:0-0
Timestamp: 2026-07-24T10:35:29.044Z
Learning: In `libs/ui` Storybook stories, follow the layout-wrapper convention: use native semantic/layout HTML elements and apply the project’s Tailwind semantic/layout token class names (not generic text/prose or custom primitives). For preformatted/documentation content, use the semantic HTML `<pre>` element (rather than a non-existent “preformatted-text” primitive), so the markup stays semantically correct and styled via the Tailwind token classes.
Applied to files:
libs/ui/stories/organisms/data-table.stories.tsx
🪛 LanguageTool
libs/ui/agent-plugin/skills/data-table-usage/SKILL.md
[uncategorized] ~114-~114: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... with onReachEnd for infinite scroll so the user sees the next page being fetch...
(COMMA_COMPOUND_SENTENCE_2)
[typographical] ~120-~120: Consider adding a comma after this introductory phrase.
Context: ...hile still being discoverable. During a drag the source is dimmed and lifted (`data-...
(AS_A_NN_COMMA)
libs/ui/skills/data-table-usage/SKILL.md
[uncategorized] ~114-~114: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... with onReachEnd for infinite scroll so the user sees the next page being fetch...
(COMMA_COMPOUND_SENTENCE_2)
[typographical] ~120-~120: Consider adding a comma after this introductory phrase.
Context: ...hile still being discoverable. During a drag the source is dimmed and lifted (`data-...
(AS_A_NN_COMMA)
🔇 Additional comments (4)
libs/ui/stories/organisms/data-table.stories.tsx (1)
775-927: 📐 Maintainability & Code Qualitychore(stories): verify Biome for the updated story.
Confirm the changed file passes Biome; apply the required
--writecommand locally if it reports changes.#!/usr/bin/env bash set -euo pipefail bunx biome check libs/ui/stories/organisms/data-table.stories.tsxSource: Coding guidelines
libs/ui/skills/data-table-usage/SKILL.md (1)
109-159: LGTM!libs/ui/src/organisms/data-table.fields.tsx (1)
628-663: LGTM!libs/ui/src/organisms/data-table.helpers.ts (1)
231-258: LGTM!
Guard the missing-maxHeight dev warning with an instance-scoped ref and run the effect on [enableVirtualization, maxHeight] so a config that becomes invalid post-mount still gets its single warning, per review feedback on #521.
…edit-focus race
Round 12 (8 parallel finder angles, each independently verified against
source before acting; two claims — a lockfile version mismatch and an
inverted `typeof process` check — were checked and found false, see below).
Correctness:
- `evaluateCondition`/`evaluateBetween` (the conditional filter) had the
exact hole `matchNumber` was fixed for two rounds ago, just in the
other filter system: `Number(null)` and `Number("")` are both `0`, not
`NaN`, so a blank cell satisfied `lt 5`, `gte 0`, any `between` range —
literal 0, not "no value". Mirrored `matchNumber`'s `cellHasNoNumber`
guard.
- `handleRowDragEnd` matched the dragged/target row back to its `data`
array position via `data.indexOf(row.original)` — reference equality
against the row's original object. Duplicate or content-equal-but-
distinct-reference entries in `data` silently broke this (wrong index,
or no match at all, dropping the reorder with no error). Replaced with
`table.getCoreRowModel().rowsById[id].index` — an O(1) id lookup that
doesn't depend on `data` containing that exact reference, and a
reorder-count improvement over the two `.find()` + two `.indexOf()`
scans it replaced.
- The edit-row focus effect read a ref written by each row's own ref
callback — fragile the moment `editingRowId` jumps directly from one
row to an *earlier* one (reachable via the controlled `editingRowId`
prop, which has no "must close the current edit first" guard the
internal `startEdit` enforces): React detaches/attaches sibling refs
in DOM order, so the old row's detach lands after the new row's
attach and nulls out what it just set. Replaced the ref with a DOM id
resolved fresh after the commit lands, which isn't exposed to that
ordering at all — and let `composeRowRef` drop the `editRowRef`
parameter it existed only to serve.
- The expand button's `aria-controls` unconditionally named
`${instanceId}-detail-${row.id}`, but that id is only ever rendered
by `renderExpandedDetailRow` — a tree/`getSubRows` table with no
`renderExpandedRow` points assistive tech at an element that is never
in the DOM. Now omitted unless `renderExpandedRow` is actually in use.
- `reorderableLeafIds` — the column `SortableContext`'s `items` — listed
every non-builtin leaf column, including pinned ones, while
`renderHeaderCell` never wraps a pinned column in `SortableHeaderContent`.
dnd-kit's sorting strategy assumes every listed id has a registered
sortable node; an id with none skews index/offset math for the
columns around it. Narrowed the filter to match what actually gets
wrapped.
- `editingRowId`'s setter always wrote internal state even when the prop
was controlled, unlike every other piece of table state, which goes
through `useControllable`'s `isControlled` guard. Switched it onto
that same hook — an unnecessary re-render on every controlled edit,
and correct handling if `editingRowIdProp` is ever removed mid-session.
- The empty-state row and the master-detail expanded row rendered a
bare `<td>` instead of `Table.Cell`, so neither picked up the `size`
variant's cell padding/text classes — invisible at the default size,
wrong at `sm`/`lg`. Verified live: `size="lg"` now puts
`p-table-cell-lg text-table-lg` on the empty-state cell.
Efficiency (all confirmed via before/after code reading, not estimated):
- `getRowLabel?.(row)` was called once per cell instead of once per row
— `columnCount`× the necessary calls on every render.
- Both dnd-kit `SortableContext` `items` arrays (`reorderableLeafIds`,
the row-reorder root-id list) were rebuilt with a fresh `filter+map`
on every render regardless of whether rows/columns actually changed,
defeating `SortableContext`'s own identity-based memoization.
`BUILTIN_COLUMN_IDS` also moved to module scope — it was a `new Set`
literal recreated every render for no reason.
- `validateDraft`'s inline blank check reimplemented the already-
exported `isBlank` instead of calling it.
Not changed (checked, not just asserted):
- "pnpm-lock.yaml pairs react-table@8.20.5 with table-core@9.1.2" —
false. `libs/ui`'s own importer resolves react-table to 9.1.2, whose
own dependency snapshot lists table-core 9.1.2; the 8.20.5 entries
belong to medusa-be's unrelated, independent dependency in the same
monorepo lockfile.
- "the dev-only `typeof process === \"undefined\"` warning guard never
fires in real browser bundles" — this is the same conservative check
used for the chart molecule's series-overflow warning earlier in this
branch's history, and deliberately so: it exists to avoid a
`ReferenceError` in environments with no `process` polyfill (Vite
browser builds, unbundled ESM), at the cost of the dev warning being
a no-op there too. A missing best-effort console message is a far
smaller cost than crashing every consumer without a `process` global.
- `data-[numeric=true]:text-end` vs. `data-[align=*]` class-ordering
ambiguity in `table.tsx`, and the native-`align` HTML attribute being
shadowed by the typed `align` prop — both pre-existing, both already
documented (the v1.1.0 changelog names `data-align` as the mechanism;
the class-order caveat has its own comment: "set one or the other").
- `evaluateBetween`/`matchBetween` duplication, "custom" filterFn
fallback, global-search debounce, grouped-sticky-header /
Select-width-heuristic architecture belonging in the shared `Table`/
`Select` components — repeats or design-scope items already answered
in earlier rounds; reasoning unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ric ranges Round 13. - The "re-arm after new rows land" effect ran unconditionally on every `data.length` change, regardless of `maxHeight`. In window-scroll mode (no `maxHeight`) it measured `scrollRef`'s element — the same unbounded div `onWindowScroll` deliberately ignores — whose `scrollHeight`/`clientHeight` are always equal, so `distance` always read as "at the bottom" and force-set `reachedEndRef.current = true` the instant a page of rows landed. A user scrolling continuously through freshly-appended content, with no incidental scroll-away in between, would have their next real reach-end silently swallowed by a latch that had already been marked "reported" before they ever got there. Guarded the effect on `maxHeight` to match the mode boundary `onWindowScroll` itself already respects. Added `WindowScrollInfiniteLoad`, the first story to exercise this path — both existing infinite-scroll stories set `maxHeight`, which is exactly why an 8-agent review round needed to find this rather than a human clicking through Storybook. Building the regression test caught a flaw in my own first attempt at it: repeatedly jumping straight to "the bottom" via `scrollTo` never fires an intermediate "scrolled away" event, so even correct code can't re-arm a latch designed exactly to be quiet while parked at the bottom. Verified both directions by hand: reverting the fix makes the story fail (`expected 30 to be greater than 30` — the very first reach-end never fires), and the real fix makes it pass through two full load cycles (30 → 60 → 90). - Both `between` implementations (`matchBetween` for the typed filter, `evaluateBetween` for the conditional one) rejected every row when the two bounds were entered in the wrong order (`From > To`): the low-bound check and the high-bound check individually correct, but jointly unsatisfiable by any value. Swap the bounds when both are present and inverted, so "100 to 10" reads as "10 to 100" instead of "nothing matches, silently." Verified live through the actual FilterConditionMenu UI: an inverted 100/10 range on Age now correctly returns the one row in range. - `HeaderDragHandle`'s label and the column-visibility menu's item label each reimplemented `columnLabel()`'s exact fallback logic inline (`typeof header === "string" ? header : column.id`) instead of calling the already-imported function — including missing its empty-string guard. Both now call `columnLabel(column)`. - `DataTableToolbarAction.id` documented as "falls back to the array index" with no caveat; a `toolbarActions` array that adds, removes, or reorders based on state needs a real `id` or an index-keyed action can inherit a sibling's transient state across a re-render. Documented the condition under which the fallback isn't safe, rather than guessing at a fallback-key heuristic that cannot fully solve the general case itself. Not changed — repeats or design-scope items already answered in earlier rounds, reasoning unchanged: raw semantic tokens in `dataTableVariants` (deliberately deferred, documented at the top of the file); the trailing actions cell being a hand-rolled extra cell instead of a real TanStack column, and grouped-sticky-header math living in DataTable instead of the shared `Table` component (both real, both bigger architectural moves than a review round); global-search/column-filter debounce (this library's filters are already fully controllable — a consumer wanting debounced filtering can debounce their own `onGlobalFilterChange` state before feeding it back, same as any other headless table would expect you to). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ized striping
- conditionalFilterFn's "="/"≠" compared as strings even on numeric columns
(NUMBER_FILTER_OPERATORS offers both), so "07" wouldn't match 7 even
though gt/gte/lt/lte on the same column already compare numerically.
Resolves the column's type and numeric-compares equals/notEquals when it's
a number column, folding them into the same half-typed/blank-cell guard
gt/gte/lt/lte already use.
- renderCellEditor resolved type via meta.type ?? "string" instead of the
shared resolveColumnType helper, so a filterVariant-only numeric column
got a number filter but a text editor.
- Row id: `id={rowElementId}` on <Table.Row> always won over
slotProps.row.id even when undefined (not editing), silently dropping any
consumer-supplied row id. Falls back to the passthrough id instead.
- striped rows used odd:/even: structural pseudo-classes, which key off DOM
sibling position rather than logical row index — under
enableVirtualization the windowed slice of mounted rows shifts, flipping
a row's stripe color as it scrolls. Colors from the row's real index
instead.
- getCellSpan's rowSpan/colSpan can't survive enableVirtualization's
windowing (the owning row can scroll out while a hidden follower stays
mounted, leaving a gap) — documented via a one-time dev warning rather
than attempting a per-window span recompute.
- the unbounded-virtualization dev warning's one-shot latch never reset, so
a config that went invalid, got fixed, then went invalid again a second
way stayed silent. Resets whenever the config is valid.
- deduplicated the reach-end distance/latch arithmetic (previously
hand-copied in both the window-scroll listener and the bounded
container's onScroll handler) into one checkReachEnd helper.
Declined: dataTableVariants' direct semantic-token usage is the already-
documented "PROTOTYPE styling" deviation (see the comment above the tv()
call) pending the Figma sync — not a new issue.
Also split renderBodyRow's <Table.Row> JSX into buildMainRow and extracted
its onClick/onKeyDown handlers to bring cognitive complexity back under the
lint threshold after the above changes.
…striped variant - conditionalFilterFn's numeric-equals fix from the previous round only checked meta.type === "number", missing "int" — which typedFilterMatch already treats identically to "number" (both route to matchNumber). An int column's "="/"≠" still string-compared. - renderCellEditor built its options list from meta.options only, dropping the meta.filterOptions fallback the header filter (renderColumnFilter) already honors — a column still on the deprecated filterOptions field got a working filter dropdown but an empty inline editor. - variant="striped" forwards straight to the Table organism, which stripes via odd:/even: DOM-sibling pseudo-classes — the same drift bug fixed for the `striped` boolean prop under enableVirtualization, just reachable through the other spelling. Routes both through the one correct, row-index-based implementation; Table only ever sees variant="line" or "outline" now. - renderExpandedDetailRow rendered a bare <tr> instead of Table.Row, so expanded/nested detail rows never got the base row bottom border every other row has. - documented (not changed) matchRangeOverlap's date-only `to` convention — symmetric between cell and filter bounds, so not a bug, but previously unexplained. Declined: DataTableBodyCell memoization and drag-handle/ActionIcon reuse are real but out-of-scope perf/consistency items, not correctness bugs; the select-label width heuristic's ReactNode edge case has no reachable caller today.
…rward menu size - data-table.tsx never re-exported DataTableColumnType, DataTableControlSize, DataTableEditorContext/Renderer, DataTableFilterContext/Renderer or DataTableOption, even though DataTableProps' own documented filterRenderers/editorRenderers/renderHeaderFilter/renderEditor props require consumers to type against them. The PR's own stories file worked around this by importing straight from the internal data-table.fields module — now updated to use the public barrel instead. - evaluateBetween (conditional filter) hand-duplicated matchBetween's (typed filter) numeric-range/NaN-guard logic instead of sharing it, so a future edge-case fix to one could silently diverge from the other. Now delegates to the exported matchBetween. - FilterConditionMenu hardcoded size="sm" on its Menu instead of forwarding the size prop it already uses for its own trigger — a table configured at size="lg" got a full-size filter icon button next to a small operator dropdown. - Documented (not changed) that conditionalFilterFn is text/number-oriented: its operator set has no real semantics for enum/multiEnum/date/dateRange columns, which silently fall back to string comparison against the raw cell value. - Added a dev warning (matching the existing getCellSpan one) for enableRowReorder + enableVirtualization together: SortableContext is seeded with every root row id, but only the rendered window actually mounts a useSortable node, so dragging toward an off-screen target is unreliable — not fixable without a deeper rework of how virtualization and dnd-kit interact. Declined: the two hand-rolled drag-handle buttons not using ActionIcon, and selectMinWidthCh's tuned +7ch buffer, are tracked separately (see the icon-button unification work) rather than new issues; global search's unthrottled setGlobalFilter-per-keystroke is a real perf concern but a debounce is a behavior change with test/story implications beyond this review pass's scope.
…ow warning, i18n required-field error
- maxSelectedRows was only enforced inside setRowSelection's updater, so it
never re-applied when the prop itself dropped below the current selection
count (e.g. a permission change lowering the cap after rows were already
selected) — the stale over-cap selection just sat there until the next
selection action. Added an effect that trims and re-notifies whenever
maxSelectedRows or the resolved selection changes.
- DataTable.ToolbarActions's overflow warning had no NODE_ENV production
gate or once-latch, unlike every other dev warning in this file — a
production consumer with too many toolbar actions got console.warn spam
on every render where the count changed.
- The inline-edit "required" validation error was a hardcoded English
string ("Required"), bypassing the translations system every other
user-facing string in this component goes through. Added
translations.requiredLabel (defaults to "Required") and threaded it into
validateDraft.
Declined: the "custom" filter type's matchNumber fallback is an existing,
already-documented tradeoff (see the comment above it) with a stated escape
hatch, not a new silent bug. dataTableVariants' raw semantic tokens are the
already-documented PROTOTYPE-styling deviation pending Figma sync. Save/
Cancel buttons not using ActionIcon, and the number filter using a raw
Input instead of NumericInput, are real consistency gaps but out of scope
for this pass — same class as the already-tracked icon-button unification
work, and NumericInput's number-typed onChange doesn't obviously preserve
the half-typed-string states matchNumber/evaluateCondition depend on, so
swapping it in needs dedicated verification rather than a review-loop fix.
The infinite-scroll re-arm effect's "no in-flight guard beyond loadingMore"
is the documented contract, not a gap — loadingMore is the mechanism by
which a consumer signals in-flight state.
… selection clamp - onReady fired on every render, not once. useTable's returned wrapper is useMemo(..., [table, tableOptions, state]) (verified in the installed @tanstack/react-table 9.1.2 useTable.js), and tableOptions is the options object literal built inline in this file — a fresh reference each render — so the wrapper identity churned even though the underlying constructTable instance (created once via useState) never changed. Now mount-only, which is what "ready" means. - Sticky multi-row header offsets were measured in a post-paint useEffect while headerOffsets starts at [0], so grouped sticky rows rendered stacked at top: 0 for one frame on mount. Moved to a layout effect, via an SSR-safe useIsomorphicLayoutEffect alias (this library is consumed by server-rendered apps, and React warns on useLayoutEffect during SSR). - maxSelectedRows was clamped in two places with different tie-break rules: the setRowSelection updater prioritized already-selected rows, while the prop-shrink effect I added last round just sliced Object.keys. Extracted the shared clampSelection helper so they can't drift. Documented rather than changed: - The prop-shrink clamp has no principled recency to preserve — every selected row is equally pre-existing and RowSelectionState carries no ordering (for default index-based ids Object.keys yields ascending numeric order, not selection order). Noted that a caller needing a specific survivor set should control rowSelection and trim it themselves. - boolean/enum/multiEnum editors omit editorKeyHandlers not by oversight: Switch, FieldSelect and Combobox all expose closed prop surfaces with no onKeyDown and no rest spread, and for the two dropdowns Enter/Escape already belong to the Zag machines (Enter picks the highlighted option, Escape closes the popup) — intercepting them would break selection rather than add a shortcut. Also extracted dateLikeEditor: date/datetime/time were three identical Input bodies differing only in the type attribute.
…move cleared filters
Critical: the number/int inline editor rendered nothing at all.
`NumericInput` is a compound component whose root renders only its
children, and the editor self-closed it — so clicking Edit on any numeric
column produced an empty div with no field to type into and no way for
onChange to fire. The InlineEdit story missed it because the aria-label
landed on that empty root and getByLabelText matched it. Now renders the
required .Control/.Input/trigger children, with the label and key handlers
on .Input (which spreads onto the real <input>) and describedBy/invalid on
the root, where they reach the input through context.
- `Select` destructures a closed prop list with no rest spread, so the
`aria-label` FieldSelect passed it never reached the DOM (hyphenated JSX
attributes skip TS excess-property checks, so nothing flagged it). Every
FieldSelect consumer was unlabelled: the boolean/enum header filters, the
enum inline editor, and the pagination page-size picker whose own comment
says it is "labelled only for assistive tech". Moved onto Select.Trigger,
which does spread rest props onto the focusable button.
- Our filterFns defined no `autoRemove`, unlike every built-in. Since the
filter controls always store an object, TanStack's fallback (which only
drops bare empty strings) never removed a cleared filter — so typing one
character into a filter and deleting it left a phantom entry in
`columnFilters` forever, and `rowReorderActive` requires
`columnFilters.length === 0`, killing row reorder for the session.
- Virtualized rows were paired with their indices positionally: renderRows
filtered out missing entries but bodyRows re-read `virtualItems[i]`, so a
single transient miss (rows shrinking before the virtualizer's count
catches up) shifted aria-rowindex, striped parity and getCellSpan's
rowIndex for every later row. Indices now travel with their rows.
- rowActions disabled on `isEditing` rather than `locked`, so they greyed
out during an edit even with lockInteractionsWhileEditing={false}.
Documented rather than changed:
- onReady's instance is a spread copy, so `table.state`/`table.options` read
off it are a mount-time snapshot (methods stay live). Noted on the prop.
- Dropped the no-op aria-describedby/aria-invalid from the boolean editor:
Switch's prop surface is closed, so they never reached the DOM and only
read as though the error were wired up. validateStatus is the channel that
works; real error association needs Switch/Combobox to accept describedBy
as NumericInput does, which versions separately from this organism.
…ject NaN numerics
Regression from the previous commit: `filterValueIsEmpty` treated any
value-less filter as empty, but picking an operator before typing stores
`{operator}` alone — so selecting "Starts with" was auto-removed and the
condition menu snapped back to "Contains", and "Between" reverted to "="
so its second input never appeared. Now a value-less filter is only empty
when it still sits on the type's default operator; a deliberate pick is
kept. Verified against the built helper across eight shapes (cleared
default, explicit operator, empty/notEmpty, enum, boolean false, real
value) — the original cleared-filter/row-reorder fix still holds.
- Row reorder did nothing whenever `enableColumnReorder` was also on. Both
wrappers enclosed the whole table with the column contexts innermost, and
`useSortable`/`useDraggable` resolve to the nearest context — so body rows
looked themselves up in `reorderableLeafIds` (index -1), got the
horizontal-axis modifier, and delivered the row id to the column drag
handler, which bails on `indexOf === -1`. Now one DndContext dispatches by
dragged id and picks the axis modifier to match, and each SortableContext
is scoped to the region whose items it lists. SortableContext renders no
DOM so it is safe inside `<table>`; a DndContext is not, since its
accessibility markup would render as a div child of the table.
- Clearing a numeric inline editor stored `valueAsNumber: NaN`, which
rendered as the literal text "NaN" and passed `required` validation
(NaN is neither null nor ""), handing the consumer NaN on commit. The
editor now maps NaN to undefined, and validateDraft treats it as empty.
Not changed: virtualization assumes uniform row heights (no measureElement
is wired), so a row taller than estimateRowHeight drifts against the
spacers. Real, but attaching measurement is a behavioural change to the
virtualized path that wants its own verification pass rather than a
drive-by. Also left the clampSelection docstring accurate to what it now
says — the shrink effect deliberately does not use it, as its own comment
explains.
- Infinite `onReachEnd` loop at end of data. `loadingMore` is a dependency
of the re-arm effect, so a fetch that returned nothing flipped it back to
false, re-ran the effect with the container still at the bottom, and
requested again — forever, with no new rows to move the scroll position.
Now only re-fires once the row count has actually grown.
- `handleScroll` fired `onReachEnd` on *horizontal* scroll. Without
`maxHeight` the div has no bounded height, so the distance-to-bottom is
permanently 0 and the first sideways scroll of a wide table read as "at
the bottom". The page-scroll listener already owns that case; this now
mirrors its guard.
- Date filters mixed UTC and local parsing: a bare `YYYY-MM-DD` bound (what
`<input type="date">` produces) parses as UTC, a zoneless cell timestamp
as local. Verified across four timezones — before, a cell at 20:00 on
Jan 1 was excluded from a "Jan 1" filter in New York, and Jan 2 00:30 was
wrongly *included* in Prague and Tokyo (the latter direction was not in
the report). Date-only bounds are now read as local midnight; all four
zones pass.
- `filterValueIsEmpty` used the wrong default operator for `custom` columns:
`typedFilterMatch` routes them to `matchNumber` (default "equals"), but
the helper assumed "contains", so `{operator:"equals"}` was never removed
— the phantom-filter case that keeps row reorder disabled. My own bug
from two commits ago.
- `slotProps.row.onKeyDown` and `tabIndex` were overwritten rather than
composed, and overwritten with `undefined` on tables without `onRowClick`
— so a consumer's handler was dropped entirely. Every other row
passthrough already composes.
- Pagination rendered real `<a href="#">` links, so each page click scrolled
the viewport to the top of the document and pushed a history entry.
Paging runs through `onPageChange`, so the navigation was pure side
effect; suppressed via linkProps (zag's mergeProps composes handlers, so
the page still changes).
Not changed, flagged for follow-up: the virtualizer still never measures
real row heights (no `measureElement`), so rows taller than
`estimateRowHeight` desync scroll position from row index. And
`tintNestedRows` uses an inset box-shadow on a `<tr>` under
`border-collapse`, which Chrome/Safari may not paint — both want a browser
check rather than a blind edit.
…s in caller frame
- The NumericInput editor fix two commits ago put `editorKeyHandlers` on
`.Input`, which renders `<Input {...api.getInputProps()} {...props} />` —
props last — so it replaced Zag's own `onKeyDown` outright and silently
killed ArrowUp/ArrowDown stepping and Home/End. The spinner triggers still
worked, so nothing looked broken. Moved to the NumericInput root: it has
no keydown of its own, and keydown bubbles, so Zag's input handler runs
first and Enter/Escape still reach the commit/cancel handler.
- `onColumnReorder` reported `from`/`to` as indices into the internal column
list, which carries the injected `__drag`/`__select` columns the consumer
never declared — so with row reorder or selection on, the indices were
offset by one or two and `arrayMove(myColumns, from, to)` moved the wrong
column. Both indices and `order` are now expressed over the caller's own
columns; verified that `arrayMove(myColumns, from, to)` reproduces the
resulting order exactly. Documented in the skill.
- Corrected the `clampSelection` docstring, which claimed to be shared with
the maxSelectedRows-shrink effect and to prevent drift. It isn't and
doesn't — that effect deliberately does not use it, as its own comment
explains. My comment, wrong since I wrote it.
- Documented `onReady`'s mount-snapshot semantics in the skill alongside the
prop doc.
Still open, unchanged: paginationProps can clobber the href="#" guard if a
consumer passes their own linkProps; an end-pinned column and the sticky
actions cell both freeze at the right edge with the same z-index; the
virtualizer never measures real row heights; and tintNestedRows' inset
box-shadow on a <tr> may not paint under border-collapse in Chrome/Safari.
The last two want a browser check rather than a blind edit.
…olumns
An end-pinned column and the sticky actions cell both freeze at the trailing
edge — `getAfter("end")` is 0 for the last end-pinned column and the actions
cell is `end-0` — and both carried the same z-index, so which one painted on
top came down to DOM order. The actions column is not part of TanStack's
column model, so end-pinned offsets cannot reserve room for its width; the
overlap itself is not fixable without measuring that width and threading it
through header, filter and body sticky offsets.
Added dedicated stacking levels so the actions cell deterministically wins,
and a dev warning for the combination — matching how this file already
handles getCellSpan+virtualization and rowReorder+virtualization.
Also refuted, after checking rather than assuming: the review flagged
`tintNestedRows`' inset box-shadow on a `<tr>` as unpaintable under
`border-collapse` in Blink/WebKit. Verified in headless Chromium 149 that it
paints correctly under both `border-collapse: collapse` and
`border-separate` — no change made. (Not verified in WebKit; only the
Chromium build is installed locally.)
… order `getVisibleLeafColumns()` is only `getAllLeafColumns().filter(isVisible)` — it does not apply pinning — while `getHeaderGroups()` and `row.getVisibleCells()` both reorder to [...start, ...center, ...end]. Everything derived from the plain leaf list therefore fell out of step with the header and body as soon as any column was pinned. Verified against a real pinned table (start:["email"], end:["age"] over name/age/email/role): the leaf list gave name,age,email,role while header and body both rendered email,name,role,age. Consequences were: - the filter row put every control under the wrong header, and the pinned filter cell stuck a mid-row cell to the start edge over its neighbours; - `indentColumnId` attached the tree indent to a column that was no longer leftmost; - the loading skeleton's columns did not line up with the table it replaces; - `reorderableLeafIds` seeded SortableContext in a different order than the headers are rendered in. Leaf columns are now taken from the last header group — the leaf header row, which is exactly what the header renders — so all four agree. Confirmed the new derivation matches `getVisibleCells()` exactly.
…bbering nav guard
- Window-mode infinite scroll could stall permanently. `checkReachEnd`'s
latch only clears on a scroll event reporting `distance > threshold`, and
the re-arm effect deliberately excludes the no-`maxHeight` case (measuring
`scrollRef` there is meaningless — it is unbounded and always reads as at
the bottom). So if a consumer appended a page shorter than the threshold,
the document still ended inside it: every later scroll read "at the
bottom", the latch was never released and loading stopped for good unless
the user scrolled back up past the threshold. Added the missing
counterpart that measures the *document*, carrying the same end-of-data
guard as the bounded path.
Simulated the resulting state machine over three scenarios: repeated short
pages keep loading (previously stalled), end-of-data fires at most twice
then stops (no runaway), and scrolling away from the bottom clears the
latch so returning re-fires.
- `{...paginationProps}` was spread *after* `linkProps`, so a consumer
passing their own `linkProps` (e.g. to add a `data-testid`) silently
dropped the `preventDefault` and reinstated the `href="#"` viewport jump
the comment right above it warns about. Now declared after the spread and
composed with the consumer's handler.
- Dropped `getPageUrl` from the `paginationProps` type: it is required on
`PaginationProps` but DataTable always overrides it, so a consumer who
only wanted `siblingCount` was forced to invent one that is then ignored.
- `pageCount` never reached the pager. `Pagination` builds its page list from `count`/`pageSize` and has no `pageCount` prop, and `getRowCount()` is `options.rowCount ?? prePaginatedRowModel.rows.length` — so a consumer following the documented server-side contract (`manualPagination` + `pageCount`, no `rowCount`) handed over one page of rows and got a single-page pager with every other page unreachable. Falls back to the span `pageCount` implies; verified against a real table that client-side and `rowCount` cases are unchanged (243 rows -> 243) while `pageCount: 10` now yields 10 reachable pages instead of 1. Documented that this figure is an upper bound on the last page, unlike exact `rowCount`. - Restored the leaf-column memoization I broke two commits ago: deriving `leafColumns` from `headerGroups.at(-1)` fixed the pinned-order bug but mapped inline, producing a fresh array every render. That silently defeated the `useMemo` on `reorderableLeafIds`, whose whole stated purpose is to keep `SortableContext`'s `items` identity stable — so every keystroke rebuilt dnd-kit's id index. `getHeaderGroups` is itself memoized on columns/order/grouping/pinning/visibility, so keying off it is stable. - The filter row's actions cell took its z-index from the `stickyHeader` branch while its `sticky end-0` class comes from `stickyActions`. Pinned filter cells get `pinnedHeaderCell` unconditionally via `getPinningStyles`, so with a non-sticky header an end-pinned column's filter control painted over the actions cell — the inversion the header and body cells both avoid. - The infinite-scroll count guard was only written by the re-arm effects, so a scroll-driven fire left it at its initial value and the next effect run issued one redundant duplicate request. Every firing path now records it.
…umns - Pinned body cells and the sticky actions cell hard-coded an opaque `bg-table-bg`, but row colour lives on the `<tr>` (selected, hover, and the striped zebra). A `<td>` background paints over it, so selecting a row highlighted everything except the frozen columns, and striping stopped dead at the sticky actions column. Rows now carry a concrete base surface and the frozen cells take `bg-inherit`, which tracks the row's real colour while staying opaque. Verified the CSS mechanism in headless Chromium: a sticky cell over horizontally-scrolled content renders white on a plain row and the row colour on a coloured one — i.e. correct colour *and* no show-through. - The leaf-column memo I added last commit did not work: its deps included `table`, whose identity changes every render (`useTable` returns `useMemo(..., [table, tableOptions, state])` over a fresh `tableOptions` literal — the same fact this file already documents to justify the mount-only `onReady`). So it recomputed every render and `reorderableLeafIds` still churned, which was the entire point of the memo. Keyed on `headerGroups` alone, which TanStack does memoize. - `handleScroll` had no "is this actually vertically scrollable?" check, while the re-arm effect does. With `maxHeight` set but too few rows to fill it, `distance` is permanently 0, so any horizontal scroll on a wide table fired `onReachEnd` and refetched — the same trap already closed for the unbounded case. - The `maxSelectedRows` shrink effect could report the same over-cap selection repeatedly: in controlled mode `useControllable` holds no state, so a parent that ignores the trim (and rebuilds `rowSelection` each render) got `onSelectionLimitReached` on every render. Now reported once per distinct over-cap selection. Re-checked and still not reproducible: `tintNestedRows`' inset box-shadow on a `<tr>` under `border-collapse` was flagged again as unpaintable in Blink — headless Chromium 149 paints it correctly under both collapse modes, so no change. Unverified in WebKit; only Chromium is installed here.
…lidate visible columns only - Regression from the previous commit: the `reportedSelectionLimitRef` signature was stored and never cleared, so a controlled parent that re-applied a previously-seen over-cap selection (select 3 with a cap of 2, clear, select the same 3 again) matched the stale signature, returned early, and rendered an over-cap selection with no `onSelectionLimitReached`. Now cleared whenever the selection is back within the cap. Simulated all three paths: reports once, does not repeat while the parent ignores the trim, and re-arms after dropping under the cap. - Numeric `≠` hid rows with empty cells. `matchNumber` short-circuits every operator on a blank cell, but a blank cell is trivially "not 5" — and the text matcher already keeps blanks for `notContains`, so the same "does not equal" filter behaved oppositely on a string and a number column. Fixed in both the typed and conditional matchers; verified blanks now pass `≠` while `=`/`>` still exclude them. - A hidden `required` + `editable` column permanently blocked inline-edit commit: `validateDraft` walked every leaf column including hidden ones, so it could raise `Required` for a field with no rendered editor — the message was stored where nothing displays it and Save silently did nothing, with no recovery but cancelling. Validates visible columns only; a field the user cannot see is one they cannot fix. - Added a dev warning for a pinned column with a string `meta.width`: frozen offsets are sums of `getSize()`, and only numeric widths are mirrored into it, so such a column lays out at its real width but is offset as if it were 150px and overlaps its neighbours. Not resolvable without measuring, so it gets the same warn-and-document treatment as this file's other known incompatible combinations. Still deliberately unfixed: virtualization never measures real row heights (and expanded detail rows are not in the virtualizer's count), so any row taller than `estimateRowHeight` drifts. Wiring `measureElement` changes scrolling behaviour and needs browser verification rather than a blind edit. Re-checked for the third time and still not reproducible: `tintNestedRows`' inset box-shadow on a `<tr>` under `border-collapse` paints correctly in headless Chromium 149 under both collapse modes. Unverified in WebKit.
… assertions
- `filterFn: "typed"` is applied to every column that does not name one, but
the typed matchers only understood the object shapes DataTable's own
controls write. A consumer using the plain TanStack API —
`column.setFilterValue("Ada")` or a controlled `columnFilters` entry — hit
`matchText(cell, "Ada")`, which read `.operator`/`.value` off a string,
found `undefined`, and returned true for every row. The entry was kept and
the filter looked active while matching everything: a silent no-op, not an
error. Bare values are now coerced to the right shape per type. Date/time
columns are deliberately excluded — a single value there could mean `from`,
`to` or both, so guessing would be worse than the fall-through. Verified
bare strings/numbers/enums/booleans filter correctly and every existing
object shape is unchanged.
- `HiddenHeader`'s play assertion checked `closest("tr").parentElement` for
`sr-only`, but `hideHeader` puts that class on the `<tr>` itself (not on
`<thead>`, which must keep the filter row visible), so it asserted against
`<thead>` and would fail whenever interaction tests run.
- `CustomFilterTemplate` put `aria-label` on the `Select` root, which drops it
— the same closed-prop-surface trap already fixed in `FieldSelect` and
documented there. The play test's `getByLabelText` would throw, and since
this story is the example consumers copy for custom filters, it was teaching
the broken pattern. Moved to `Select.Trigger`.
- Removed a dead `case "notEquals"` I left in `matchNumber` last commit when
hoisting that operator above the blank-cell guard.
- The skill claimed `filterVariant` "no longer selects a control", which
contradicts `resolveColumnType` — it does, via `FILTER_VARIANT_TYPE`. A
consumer trusting the doc would leave a legacy column mis-wired. Corrected,
added the missing `int`/`datetime` to the inline type union, and documented
the filter value shapes (their absence is what made the no-op above silent).
Refuted, no change: the review also reported the type table omits
`int`/`datetime` — it lists them as combined `int / number` and
`date / datetime` rows.
- `conditionalFilterFn` returned `true` for any filter value without an
`operator`. The enum, multiEnum, boolean and date/time controls never write
one — they store `{ values }`, `{ value }` or `{ from, to }` — so a column
opting into `filterFn: "conditional"` with one of those types matched every
row, while `autoRemove` kept the entry so the UI and `onColumnFiltersChange`
both reported an active filter doing nothing. Operator-less values are now
routed to the typed matcher. The docstring claimed these types "fall back to
a case-insensitive string comparison"; they short-circuited instead, so that
claim is corrected too.
- The stories' shared `columns` fixture hit exactly that: the Role column
paired `filterFn: "conditional"` with `filterVariant: "select"`, so picking
a role in Playground / ColumnFiltersWithConditions left every row visible.
Since the stories are the component's reference documentation, this shipped
a broken example. Dropped the pairing — a select has no operators to choose,
so the default `"typed"` filter is both correct and the clearer example.
- Regression from the previous commit: `normalizeFilterValue` bailed out on
`typeof filterValue === "object"`, and `typeof [] === "object"`, so
`setFilterValue(["react"])` passed through uncoerced and matched every row —
reproducing the exact silent no-op that function was added to prevent.
Arrays are now coerced to `{ values }` before that check.
- Sticky filter cells had no z-index unless the column was pinned. Their
`<td>`s are raw elements, so they never receive the `sticky top-0 z-10` the
Table organism puts on real header cells, leaving them at `auto` while
pinned body cells carry `pinnedCell` — so with stickyHeader + column filters
+ pinning, scrolling painted the frozen column over the filter row. Added a
`stickyHeaderCell` level between the body and pinned-header levels.
Verified the whole filter matrix against the built output: arrays, bare
primitives and object shapes across enum/multiEnum/string/number, plus the
conditional path with and without an operator — 14 cases, all passing.
…o-opping
Third defect in `normalizeFilterValue`, and this one contradicted the
docblock I wrote for it.
- Bare date/time values silently matched everything. The fallback wrapped
every remaining type as `{ value }`, but `matchDateRange`/`matchTime` read
only `from`/`to`, so `setFilterValue("2024-01-01")` on a date column was
stored, reported active via `onColumnFiltersChange`, disabled the row-reorder
guard — and matched every row. Note the docblock's stated intent ("left to
fall through rather than guessing") would not have helped either: an
untouched string reads as "no bounds" just the same. A lone value is now a
closed range on itself, which those matchers already handle exactly right —
the whole of that day for a date (the `to` bound runs to end-of-day), that
single minute for a time.
- `{ value: Boolean(filterValue) }` made every non-empty string `true`, so
`setFilterValue("false")` — the natural shape from a <select>, a URL query
param or restored state — filtered for `true` and showed precisely the rows
the user asked to exclude. Parsed rather than coerced.
Corrected the docblock and the matching claim in SKILL.md, both of which
described the old (wrong) intent.
Verified against the built output: bare dates match their own day and reject
the days either side, bare times match that minute, `"false"` excludes `true`
rows, and every existing object/array/primitive shape is unchanged — 14 cases,
all passing.
…ng datetime bounds
- Controlled `editingRowId` could silently blank or cross-contaminate a row.
`draft` was only ever populated by `startEdit`, which early-returns while
already editing, and the reset effect only fired on the transition to
`null`. So a parent setting `editingRowId` directly opened every editor
empty and handed `onEditCommit` a `draft` of `{}` — writing blanks over the
row's real values — and switching row A → B without passing through `null`
left A's draft in place, showing A's values under B and committing A's data
against B's id. Both paths now seed from the row the id names, through the
same helper `startEdit` uses so the two cannot drift.
- Regression from the previous commit: coercing a bare `datetime` value to
`{ from: v, to: v }` met an end-of-day extension that was applied to every
`to` bound unconditionally, turning `setFilterValue("2024-06-01T20:00")`
into a rolling ~24-hour window. The extension only ever made sense for
date-*only* bounds (its own comment said so); it is now conditional on the
bound actually being date-only, in one helper shared by both date matchers
rather than three separate call sites. Verified in two timezones: a bare
datetime matches that instant and rejects +2h, while a bare date still
covers its whole day and rejects both neighbours.
- A `type: "custom"` + `editable` + `required` column with no `renderEditor`
renders read-only content, but was still validated — the same unfixable,
invisible error that the visible-columns filter was added to prevent, just
reached by a different route. Validation now runs only over columns that
actually render an editor.
… type
- A missed i18n lookup took the whole table down. `{ ...DEFAULTS, ...prop }`
spreads explicitly-undefined values over the defaults, and
`translations={{ rangeLabel: t?.range }}` is the ordinary call shape — so a
missing key left `rangeLabel` undefined, and `DataTable.Pagination` calls it
unconditionally: "translations.rangeLabel is not a function" as soon as
`enablePagination` was set. Strings had the milder version, rendering the
literal "undefined". Undefined entries are now dropped when merging.
- Row and column drags were dispatched by testing the dragged id against the
reorderable column list, so a row whose id collided with a column id — very
reachable with `getRowId={(r) => r.slug}` — reordered the columns and never
fired `onRowReorder`. Both `useSortable` call sites now tag `data.type`, and
dispatch reads that instead of guessing from the id.
- `setDraftValue` composed the next draft from the render-scoped `draft`, so
two editors committing in the same tick (the `dateRange` pair, or a batched
blur+change) both resolved against one snapshot and the first write was
dropped. Uses a synchronously-advanced ref: a functional updater would fix
the stored state but not `onEditChange`'s payload, since React may run the
updater after the call returns.
- `loadingMore` with no rows yet rendered "No records": the append skeleton
lives in the rows branch, so an infinite-scroll table fetching its first
page showed the empty state mid-fetch. Treated as loading instead.
- The two `multiEnum` renderers rebuilt their `items` array every render, and
`Combobox` mirrors `items` into state and resets on identity change — so any
unrelated re-render snapped the dropdown back to the unfiltered list while
the user was typing. `DataTableOption` already satisfies `ComboboxItem`, so
the mapping was pure waste; passing `options` through keeps the consumer's
own array identity.
- The skeleton and footer actions cells omitted the sticky treatment the real
actions cell and the header use, so with `stickyActions` the frozen column
visibly broke apart while loading or when a column declared a footer.
…d rows Regression from my own `bg-inherit` change. Making pinned and sticky-actions cells inherit the row's background fixed the "striping stops at the frozen column" complaint, but every row-level tint in this system is an alpha overlay meant to composite over a surface — `--color-table-row-striped-secondary` is literally `oklch(0 0 0 / 0)`, and the hover and selected tints are 5–10% fills. Inheriting one therefore produced a *transparent* cell, and the horizontally scrolled body content showed straight through the frozen column on every even striped row, every hovered row and every selected row. Fixed by compositing rather than inheriting, the same trick `OPAQUE_HEADER_BG` already uses for the header: the frozen cell paints the opaque table surface as its background-color and the row's tint over it as a gradient image, read from a `--dt-row-tint` custom property the row publishes. Custom properties inherit, so the row drives its frozen cells without either side knowing about the other, and selection/hover win over the stripe on specificity rather than source order. Verified in headless Chromium against the real composite values: an odd striped frozen cell renders 242,242,242 (5% black over white), an even one 255,255,255, and both stay opaque over red scrolled content — while the previous `bg-inherit` version renders 255,0,0 in the same test, i.e. the bug reproduces on demand and the fix removes it. (My first two attempts at this measurement were themselves wrong — the cells did not overlap at that scroll offset, and the comparison baseline was off-screen. Worth noting that the browser check only became meaningful once the harness was fixed.)
Two regressions of my own, plus the expand toggle escaping the edit lock. - `commitEdit` validated and committed the render-scoped `draft` while `setDraftValue` maintains `draftRef` for exactly the same-tick case. So a `setValue(v); commit()` in one handler — the only way to commit the enum and boolean editors, which have no key handlers, and what Zag's INPUT.ENTER does on the numeric editor — silently dropped the value just entered, and a `required` field the user had just filled in failed validation as blank. Both reads now use the ref. - The `scrollHeight > clientHeight` guard I added to stop horizontal scroll firing `onReachEnd` also dead-ended the ordinary case: a first page too short to fill `maxHeight` never scrolls, so no scroll event ever arrives, and requiring overflow meant the re-arm effect refused to fire too — the list stuck on page one forever. Underflow is precisely when another page is wanted, and runaway is already prevented by the row-count guard, so the requirement is dropped from the effect and kept only in the scroll handler (where it is the right discriminator). Also gave the scroll handler the end-of-data guard both effects have, so scrolling up and back down after a fetch returned nothing no longer re-issues the same request. Simulated all four paths: an underfilled first page requests more, growth continues until the data is exhausted and then stops, an exhausted list does not re-request on scroll, and horizontal scroll on a non-scrolling container stays silent. - The expand toggle had no edit-lock guard, so during an inline edit a user could expand a row above the edited one and shift it out from under themselves — the exact thing `lockInteractionsWhileEditing` promises to prevent. Now disabled while locked and reported as a blocked "expand". Also recorded a verification note on `data-table-row-nested-tint`: the claim that box-shadow on a `<tr>` is unpainted under `border-collapse` has now been raised four times, and a differential screenshot in Chromium 149 (plain 255,255,255 vs tinted 178,178,178, same collapsed table) shows it does paint. Noted as unverified in WebKit, with the gradient-layer fix if it ever is.
- The end-of-data guard I added to the scroll handler and both re-arm effects
was missing from the fourth path, the window-scroll listener. Without
`maxHeight`, once a fetch came back with no new rows, `checkReachEnd`'s latch
cleared as soon as the user scrolled up past the threshold, so every scroll
back to the bottom re-issued the exhausted request indefinitely — the exact
bug the bounded path's comment describes.
- Round-tripping `onColumnReorder` moved the checkbox to the far right.
`order` is reported over the consumer's own columns (built-ins stripped, so
the indices are usable), but TanStack appends any column absent from
`columnOrder` to the end — so feeding that order back, or authoring
`columnOrder` from your own ids, sent `__select`/`__drag` from the leading
edge to the trailing edge. Verified against a real table:
`columnOrder: ["age","name"]` rendered `age, name, __select`. Built-ins are
now re-inserted at the front when a supplied order omits them.
- `headerRowRefs.current.filter((n) => n !== null)` let `undefined` through —
the array is written by index, so an unwritten slot survives a `!== null`
test and then throws on `getBoundingClientRect`, taking down the sticky
header layout effect and the render with it.
- `renderRowActions`'s `disabled` was keyed on `isEditing` while the sibling
declarative `rowActions` path uses `locked`, so with
`lockInteractionsWhileEditing={false}` a consumer's own row actions rendered
disabled despite the opt-out. The two paths now agree.
- Inline-edit error text used `text-danger` (the fill/surface red) where
AGENTS.md pins status text to `--color-<semantic>-fg`, the contrast-checked
foreground.
BleedingDev
approved these changes
Aug 23, 2026
|
🎉 This PR is included in version 0.38.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This branch was successfully deployed
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.
Summary
Adds a headless, data-driven DataTable organism to
@techsio/ui-kit, built on@tanstack/react-tablev8 and rendering into the existing presentationalTableorganism (so it inherits--color-table-*tokens).Chosen after a context7-sourced comparison of VTable / AntV S2 / TanStack (see
docs/data-table/00-library-comparison.md): the canvas libraries (VTable, S2) can't use our Tailwind/Zag stack, TanStack is the only headless/DOM fit.Features (all opt-in, each exposes a callback)
Sorting · conditional column filters · header filter template · global search · empty template · row actions · quick actions · freeze columns L/R · sticky header · striped rows · infinite scroll / virtualization · colSpan/rowSpan (
getCellSpan) · onRowClick · checkbox selection · column reorder · row reorder · column show/hide · custom cell/row templates · tree structure · inline edit · pagination · hideHeader (headerless layout).Weak spots of TanStack are covered via
@tanstack/react-virtual,@dnd-kitand a customgetCellSpan.Testing
ui-kit:build,ui-kit:typecheck,biome check,build:storybook— all green.fn()spies andplayinteraction tests.Notes / follow-up
Table+ semantic tokens. Component-specific--color-data-table-*tokens and the Figma token export are a follow-up once the MVP look is signed off.data-table.figma.tsx) deferred until the Figma component exists.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation