fix(dashboard): Forward transformQueryKey and view options from ListPage - #5066
Conversation
ListPage built its commonTableProps object from most of its props but omitted transformQueryKey, disableViewOptions and includeSelectionColumn, so those PaginatedListDataTable props could never be set through ListPage. The transformQueryKey gap is the harmful one: a page that injects state via transformVariables cannot keep the react-query cache key in sync, so the list fetches once and then serves that first response for every value of the injected state, with no error and no empty table. Forward all three props. Fixes vendurehq#5026
… value and negative case
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/dashboard/src/lib/framework/page/list-page.spec.tsx (1)
5-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Readonly<>for the mocked component props.Type the mock props as
Readonly<Record<string, unknown>>to prevent accidental mutation and comply with the dashboard props guideline.As per coding guidelines, React component props objects should use
Readonly<>for type safety.Proposed diff
-const captured: { props?: Record<string, any> } = {}; +const captured: { props?: Readonly<Record<string, unknown>> } = {}; vi.mock('`@/vdb/components/shared/paginated-list-data-table.js`', () => ({ - PaginatedListDataTable: (props: Record<string, any>) => { + PaginatedListDataTable: (props: Readonly<Record<string, unknown>>) => {🤖 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 `@packages/dashboard/src/lib/framework/page/list-page.spec.tsx` around lines 5 - 13, Update the mocked PaginatedListDataTable component in the test to accept props typed as Readonly<Record<string, unknown>> instead of a mutable Record<string, any>. Keep captured.props compatible with the updated props type and preserve the existing capture behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/dashboard/src/lib/framework/page/list-page.spec.tsx`:
- Around line 5-13: Update the mocked PaginatedListDataTable component in the
test to accept props typed as Readonly<Record<string, unknown>> instead of a
mutable Record<string, any>. Keep captured.props compatible with the updated
props type and preserve the existing capture behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ea7f701-13a4-42ea-a27e-9f17556da755
📒 Files selected for processing (2)
packages/dashboard/src/lib/framework/page/list-page.spec.tsxpackages/dashboard/src/lib/framework/page/list-page.tsx
michaelbromley
added
the
T1: Fast track
biggamesmallworld
left a comment
biggamesmallworld
left a comment
There was a problem hiding this comment.
Fix looks right — I diffed every PaginatedListDataTableProps prop against commonTableProps and after this change all 27 are accounted for. The @default true claim on includeSelectionColumn checks out against use-generated-columns.tsx:82.
Two issues in the spec, both with committable suggestions below.
The second test can never fail. I ran the spec with the fix reverted: Tests 1 failed | 1 passed — does not inject the forwarded props when they are not provided passes identically before and after. It asserts undefined === undefined, and because of the captured.props?. optional chaining it would also pass if ListPage rendered nothing at all.
React.createElement(ListPage as any, ...) casts away the half of the bug that mattered. The omission was in three places, one of which was the ListPageProps interface. With as any, the test goes green even if you add the props to commonTableProps and forget the interface — public API still broken, guard still says fine. JSX with the props inline gets check-types covering that half.
Also a stray // eslint-disable-next-line import/first — that rule isn't configured in .eslintrc.js (only import/order is), and vitest hoists vi.mock so the bottom-of-file import is unnecessary anyway.
Verified the suggested version locally:
- GREEN:
1 passed - Runtime RED (drop the three props from
commonTableProps):AssertionError: expected undefined to be [Function transformQueryKey] - Type RED (drop
transformQueryKeyfromListPagePropsonly):list-page.spec.tsx(49,17): error TS2322: ... not assignable to ... ListPageProps<...>
Both halves of the regression now guarded.
Non-blocking, for a follow-up: this class of bug exists because every pass-through prop has to be hand-maintained in three places — the ListPageProps interface, the destructuring, and the commonTableProps literal. #5026 wasn't "someone forgot transformQueryKey", it was "the shape guarantees someone forgets something eventually". Deriving the pass-through subset via Pick<PaginatedListDataTableProps<...>, ...> and forwarding by rest-spread would make it unrepresentable. Not asking for that in a master bug fix, but worth an issue.
A behavioural test (render the real PaginatedListDataTable with a stubbed useQuery, assert the query key changes when injected state changes) would cover the actual #5026 symptom rather than just the forwarding. Optional — the forwarding guard is the regression that matters.
biggamesmallworld
left a comment
biggamesmallworld
left a comment
There was a problem hiding this comment.
Approving on the production change — the prop forwarding is correct and complete (all 27 PaginatedListDataTableProps props accounted for in commonTableProps after this).
Please commit the spec suggestion above before merging. As it stands the second test passes with the fix reverted, and the as any means the ListPageProps interface half of the regression is unguarded — so the current spec would not catch a recurrence of half of #5026. The suggested version is verified RED→GREEN on both halves locally.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/dashboard/src/lib/framework/page/list-page.spec.tsx (1)
34-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
basePropsas readonly props.
basePropsis passed to<ListPage>, but its type is inferred as mutable. Use an explicitReadonly<...>type for this fixture to prevent accidental mutation at compile time.As per coding guidelines: “Set React component props objects to Readonly<> type for type safety.”
🤖 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 `@packages/dashboard/src/lib/framework/page/list-page.spec.tsx` around lines 34 - 37, Declare the baseProps fixture with an explicit Readonly props type matching ListPage’s expected props, while preserving its existing values and behavior. Update the baseProps declaration rather than the route, title, or listQuery fields.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/dashboard/src/lib/framework/page/list-page.spec.tsx`:
- Around line 34-37: Declare the baseProps fixture with an explicit Readonly
props type matching ListPage’s expected props, while preserving its existing
values and behavior. Update the baseProps declaration rather than the route,
title, or listQuery fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b76943b-c8a0-4541-a358-552390f8589e
📒 Files selected for processing (2)
packages/dashboard/src/lib/framework/page/list-page.spec.tsxpackages/dashboard/src/lib/framework/page/list-page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dashboard/src/lib/framework/page/list-page.tsx
3 participants
Summary
ListPagenow forwardstransformQueryKey,disableViewOptionsandincludeSelectionColumnto the underlyingPaginatedListDataTable, so pages built onListPagecan actually use them.Root cause
ListPagebuilds acommonTablePropsobject and spreads it intoPaginatedListDataTable, forwarding most props — but three realPaginatedListDataTableprops were missing from that object:transformQueryKey,disableViewOptionsandincludeSelectionColumn.The harmful one is
transformQueryKey. The default react-query key only reflectspage,itemsPerPage,sorting, the column filters and the debounced search term. Any state a page injects throughtransformVariables(a language selector, an action-bar status filter, a parent entity id) is invisible to the cache key. Without a way to also transform the key, the list fetches once and then serves that first response for every value of the injected state — no error, no empty table, the rows simply never change.disableViewOptions/includeSelectionColumnwere missing the same way but fail visibly.Change
Add the three props in
list-page.tsxin the three places a forwarded prop must appear: theListPagePropsinterface (with JSDoc), the component destructuring, and thecommonTablePropsobject literal. Types mirrorPaginatedListDataTableexactly. No behaviour change unless a prop is set.Test plan
Automated —
list-page.spec.tsx(new) mocksPaginatedListDataTableand assertsListPageforwards the three props (identity fortransformQueryKey, non-defaultfalseforincludeSelectionColumn), plus a negative case asserting they stayundefinedwhen not provided. This is a RED→GREEN guard for the exact omission: without the fix,transformQueryKeyarrivesundefined.The downstream refetch behaviour lives in
PaginatedListDataTable+ react-query (unchanged here), which already consumestransformQueryKeyto build the query key.Manual — a
ListPageusingtransformVariables+transformQueryKeyto inject toolbar state now refetches when that state changes instead of serving the stale first page.Fixes #5026
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.