From 830b43d1e25bf447381aa0a8e6240498a6a73485 Mon Sep 17 00:00:00 2001 From: SoundMindsAI Date: Tue, 19 May 2026 14:38:32 -0400 Subject: [PATCH 1/5] docs: lock DataTable flat-prop API + close chore item 3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /idea-preflight on chore_data_table_primitive_followups landed three changes: 1. idea.md: Status frontmatter updated with PR #132 squash hash (was "PR (TBD)") + item 1 body note about sketch-vs-resolution divergence + new "Open questions for /spec-gen" section. 2. idea.md: Q1 locked — item 3 closed as "won't fix, doc the chosen shape." Rationale: the implemented plan's Story 2.6 sketched a urlState aggregate + setter-bag shape, but the build-out shipped 10 flat optional props on DataTableProps. The flat-prop API is idiomatic React, aligns with shadcn primitives, and all 8 thin-wrapper consumers depend on it. Refactor cost (~250 LOC across 9 files) doesn't justify the marginal API-aesthetics gain. 3. implementation_plan.md (historical, in implemented_features/): Story 2.6 gains a "Shipped contract addendum" block at the top noting the flat-prop API is canonical, with a back-reference to the chore idea for the rationale. The aggregate-prop sketch below stays as historical context. Folder rename to chore_data_table_columnvisibility_tanstack follows in a separate commit on this branch (per CLAUDE.md no-edit-then-mv rule). Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/00_overview/MVP1_DASHBOARD.md | 2 +- .../implementation_plan.md | 4 ++- .../idea.md | 27 +++++++++++++++---- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/docs/00_overview/MVP1_DASHBOARD.md b/docs/00_overview/MVP1_DASHBOARD.md index 5cc8ada4..edb67229 100644 --- a/docs/00_overview/MVP1_DASHBOARD.md +++ b/docs/00_overview/MVP1_DASHBOARD.md @@ -95,7 +95,7 @@ _None._ | Feature | Type | One-liner | Depends on | Status | |---|---|---|---|---| | [chore_ci_prettier_check](../02_product/planned_features/chore_ci_prettier_check/idea.md) | Chore | `.github/workflows/pr.yml`'s `frontend` job runs: | — | Idea — surfaced during `/impl-execute --ad-hoc` of `infra_e2e_wire_seed_helper_into_studies_spec`. | -| [chore_data_table_primitive_followups](../02_product/planned_features/chore_data_table_primitive_followups/idea.md) | Chore | `feat_data_table_primitive` shipped with six known non-regression follow-up items captured only in chat transcripts. None block the PR but each is a real improvement that would otherwise evaporate whe | — | Partial — items 1, 2, 4, 6 shipped in PR (TBD) 2026-05-17. Items 3 + 5 remain open (larger refactors touching 8 DataTable consumers each). | +| [chore_data_table_primitive_followups](../02_product/planned_features/chore_data_table_primitive_followups/idea.md) | Chore | `feat_data_table_primitive` shipped with six known non-regression follow-up items captured only in chat transcripts. None block the PR but each is a real improvement that would otherwise evaporate whe | — | Partial — items 1, 2, 4, 6 shipped in PR #132 (squash commit `ee4c8d4`) 2026-05-17; item 3 closed 2026-05-19 (won't fix — flat-prop API locked as canonical, plan addendum required). Item 5 is the only residual implementation work: 1 primitive + (optionally) per-wrapper touch-ups. | | [chore_detail_page_shell_primitive](../02_product/planned_features/chore_detail_page_shell_primitive/idea.md) | Chore | Six of the seven `/{entity}/[id]` detail routes hand-roll the same three-state scaffold around their data query. The structure is **identical** down to the className strings, with two minor copy varia | — | Idea — surfaced during a UI-refactor audit run after [`chore_form_dropdown_primitive`](../../../00_overview/implemented_features/2026_05_18_chore_form_dropdown_primitive/feature_spec.md) (PR #136) and [`feat_data_table_primitive`](../../../00_overview/implemented_features/2026_05_16_feat_data_table_primitive/feature_spec.md) (PR #126) merged. | | [chore_extract_shadcn_select_test_mock](../02_product/planned_features/chore_extract_shadcn_select_test_mock/idea.md) | Chore | A factor-and-share refactor was attempted during `chore_form_dropdown_primitive` post-implementation. I extracted the mock to `ui/src/__tests__/helpers/shadcn-select-mock.tsx` exporting `mockShadcnSel | — | Idea — captured during `chore_form_dropdown_primitive` execution (PR pending). | | [chore_form_dropdown_guide_screenshot_refresh](../02_product/planned_features/chore_form_dropdown_guide_screenshot_refresh/idea.md) | Chore | Each affected guide has a Playwright spec at `ui/tests/e2e/guides/*.spec.ts` that captures screenshots when run against the real backend. The PR's UI changes produce different screenshots: | — | Idea — captured during `chore_form_dropdown_primitive` post-implementation guide-impact assessment. | diff --git a/docs/00_overview/implemented_features/2026_05_16_feat_data_table_primitive/implementation_plan.md b/docs/00_overview/implemented_features/2026_05_16_feat_data_table_primitive/implementation_plan.md index 8fadcb3f..2670182d 100644 --- a/docs/00_overview/implemented_features/2026_05_16_feat_data_table_primitive/implementation_plan.md +++ b/docs/00_overview/implemented_features/2026_05_16_feat_data_table_primitive/implementation_plan.md @@ -705,7 +705,9 @@ None — inline in `data-table-toolbar.tsx`. ### Story 2.6 — `useDataTableUrlState` hook with push/replace history (FR-8) — **lifted to the consumer** -**Outcome:** A reusable hook that owns the URL-state contract: cursor uses `router.push()`, filter/sort/q use `router.replace()`. Cursor resets on filter/sort/q change. Hydrates from URL on mount. **The hook lives at the page-level consumer**, not inside DataTable, so the consumer's TanStack Query hook receives the URL state and refetches accordingly. DataTable becomes a **controlled component** receiving `urlState` + setters as props (per spec §4 "Consumer-supplied data" principle). +> **Shipped contract addendum (2026-05-19).** The "Key interfaces" block below sketched `DataTableProps` gaining a `urlState: DataTableUrlState` + setter-bag (`setSort` / `setFilter` / `setQ` / `setCursor` / `setPageSize`) shape. The build-out across stories 2.2–2.5 produced **10 flat optional props** on `DataTableProps` instead: `sort? / onSortChange? / sortCodec? / filters? / onFilterChange? / q? / onQChange? / cursor? / onCursorChange? / onPageSizeChange?`. The flat-prop API is canonical; the aggregate-prop shape below is an early sketch retained for historical context. Rationale and decision log: [`chore_data_table_primitive_followups/idea.md` item 3 (closed)](../../../02_product/planned_features/chore_data_table_primitive_followups/idea.md). Verified in `ui/src/components/common/types.ts:181-216`. + +**Outcome:** A reusable hook that owns the URL-state contract: cursor uses `router.push()`, filter/sort/q use `router.replace()`. Cursor resets on filter/sort/q change. Hydrates from URL on mount. **The hook lives at the page-level consumer**, not inside DataTable, so the consumer's TanStack Query hook receives the URL state and refetches accordingly. DataTable becomes a **controlled component** receiving the flat URL-state value/onChange pairs as props (per spec §4 "Consumer-supplied data" principle). **New files** diff --git a/docs/02_product/planned_features/chore_data_table_primitive_followups/idea.md b/docs/02_product/planned_features/chore_data_table_primitive_followups/idea.md index 5e889d6e..dbed968b 100644 --- a/docs/02_product/planned_features/chore_data_table_primitive_followups/idea.md +++ b/docs/02_product/planned_features/chore_data_table_primitive_followups/idea.md @@ -1,7 +1,7 @@ # DataTable primitive — review-cycle follow-ups **Date:** 2026-05-16 -**Status:** Partial — items 1, 2, 4, 6 shipped in PR (TBD) 2026-05-17. Items 3 + 5 remain open (larger refactors touching 8 DataTable consumers each). +**Status:** Partial — items 1, 2, 4, 6 shipped in PR #132 (squash commit `ee4c8d4`) 2026-05-17; item 3 closed 2026-05-19 (won't fix — flat-prop API locked as canonical, plan addendum required). Item 5 is the only residual implementation work: 1 primitive + (optionally) per-wrapper touch-ups. **Origin:** [`feat_data_table_primitive`](../../../00_overview/implemented_features/2026_05_16_feat_data_table_primitive/) review cycles. Specific cycle/finding references inline below. **Depends on:** `feat_data_table_primitive` (merged as PR #126). @@ -26,6 +26,8 @@ Four test files duplicate the same ~25-line `searchParamsSubscribers` + `applyUr Factor into `ui/src/__tests__/helpers/data-table-url-mock.ts` exporting `setupDataTableUrlMock()` that returns `{ setSearch, getLastReplace, getLastPush }`. Next consumer that adds a DataTable URL-state test pulls the helper instead of duplicating. +> *(The shipped helper exposes a `vi.mock`-friendly API instead — `makeNextNavigationMock` / `resetDataTableUrlMock` / `getDataTableUrlMockState` / `setMockedSearch` — see Resolution above. The `setupDataTableUrlMock` text here is the original sketch; trust the Resolution paragraph for the canonical API.)* + ### 2. `useLocalStorageSet` return shape ✅ DONE 2026-05-17 **Origin:** Epic 2 GPT-5.5 cycle 1 finding #14 (Low, deferred). @@ -34,13 +36,18 @@ Plan Story 2.10 specifies `useLocalStorageSet` returns `{ value: string[], add(i **Resolution:** Locked the shipped Set-based shape as canonical. The plan's `string[]` proposal was an early sketch — the actual consumer (`` at `ui/src/components/common/data-table.tsx`) uses `.has(c.id)` + `.toggle(id)`, which are Set-shaped operations. Set provides O(1) membership; array would force `.includes()` (O(n)). Updated the hook's docstring at `ui/src/hooks/use-local-storage-set.ts` to lock in the contract and reference this idea. No code change. -### 3. `DataTableProps` URL state aggregate prop +### 3. `DataTableProps` URL state aggregate prop — ✅ CLOSED 2026-05-19 (won't fix — doc the chosen shape) **Origin:** Epic 2 GPT-5.5 cycle 1 finding #1 (High, deferred). -Plan Story 2.6 key-interface block declares `DataTableProps` gaining required `urlState: DataTableUrlState` + `setSort`/`setFilter`/`setQ`/`setCursor`/`setPageSize` props after the URL hook lifts. The implementation kept the flat optional props (`sort?`, `onSortChange?`, etc.) from the 2.2-2.5 build-out — works as a controlled contract but doesn't match the plan's compact shape. +**Resolution:** Locked the shipped flat-prop API as canonical. The implemented plan's Story 2.6 key-interface block sketched `urlState: DataTableUrlState` + a `setSort`/`setFilter`/`setQ`/`setCursor`/`setPageSize` setter bag, but the build-out in stories 2.2-2.5 produced 10 flat optional props on [`DataTableProps`](../../../../ui/src/components/common/types.ts) (`sort?`, `onSortChange?`, `filters?`, `onFilterChange?`, `q?`, `onQChange?`, `cursor?`, `onCursorChange?`, `onPageSizeChange?`, `sortCodec?`). Rationale for closing: + +- The plan in `implemented_features/2026_05_16_feat_data_table_primitive/` is a historical record; the shipped code is the live contract that the 8 thin-wrapper consumers depend on. +- The flat-prop API is idiomatic React + aligns with shadcn primitives (consumer passes value + onChange pairs directly). +- All 8 consumers (`clusters/clusters-table.tsx`, `judgments/judgments-table.tsx`, `proposals/proposals-table.tsx`, `query-sets/queries-table.tsx`, `query-sets/query-sets-table.tsx`, `studies/studies-table.tsx`, `studies/trials-table.tsx`, `templates/templates-table.tsx`) work cleanly with the shipped shape. +- Refactor cost (~250 LOC across 9 files) doesn't justify the marginal API-aesthetics gain. -Refactor would touch every Epic 3 consumer (8 thin wrappers). The flat-prop API is also slightly more idiomatic React; the question is whether the plan should be updated to reflect the chosen design or the code should be updated to reflect the plan. +Companion patch to the historical plan: [`implemented_features/2026_05_16_feat_data_table_primitive/implementation_plan.md` Story 2.6 key-interface block](../../../00_overview/implemented_features/2026_05_16_feat_data_table_primitive/implementation_plan.md) gains a "Shipped contract" addendum noting the divergence from the sketched aggregate-prop shape and pointing back to this idea for the rationale. ### 4. `?limit=` coercion to `pageSizeOptions` allowlist ✅ DONE 2026-05-17 @@ -76,7 +83,17 @@ Direct URLs like `?status=invented` or `?sort=garbage:asc` currently pass throug Each item was reviewed and adjudicated "non-regression follow-up" during the parent PR's review cycles. None block correctness; each improves consistency, encapsulation, or defense-in-depth. Bundling into one chore folder so they ship together (or get dropped together) when picked up. +## Open questions for /spec-gen + +- **Q1 — Item 3 scope.** ✅ **Locked 2026-05-19: close item 3 (won't fix), lock the flat-prop API as canonical, add a "Shipped contract" addendum to the historical implementation plan's Story 2.6 key-interface block.** See item 3 above for the full rationale. +- **Q2 — Item 5 cleanup scope: just the primitive, or also the wrapper-level prop changes?** Migrating to TanStack `state.columnVisibility` is straightforward in [`data-table.tsx:259-267`](../../../../ui/src/components/common/data-table.tsx) (pre-filter → full columns + `state.columnVisibility` derived from the same `useLocalStorageSet`). Recommended default: **primitive-only.** The 8 thin wrappers don't touch column visibility today; the localStorage key + the `useLocalStorageSet` integration stay unchanged. Refactor is contained to the primitive's `useReactTable()` call and the `visibleColumns`/`columns` props passed downstream. + +## Folder-name note + +With Q1 locked (item 3 closed), the residual scope is item 5 only (TanStack `state.columnVisibility` migration) plus the historical-plan addendum. Current folder name `chore_data_table_primitive_followups` is now broader than the residual work. Candidate rename: **`chore_data_table_columnvisibility_tanstack`** (4 tokens, intent-clear). Rename requires explicit user confirmation per the preflight skill's folder-rename gate — not applied automatically. + ## Relationship to other work -- **Parent:** `feat_data_table_primitive`. +- **Parent:** `feat_data_table_primitive` (PR #126). - **Adjacent deferred:** `feat_fts_rank_ordering_mvp2` (rank-ordered FTS) — different concern (backend ordering), no overlap. +- **Sibling check (clean):** `chore_form_dropdown_primitive` (shipped, PR #136), `chore_detail_page_shell_primitive` (idea-stage, PR #147) — both touch different surfaces. `chore_ci_prettier_check` is the only sibling that *could* affect a future PR for this chore (catches `prettier` drift in changed files), but doesn't conflict. From a90066453e3946727e8990696dbbf45d6e538bfa Mon Sep 17 00:00:00 2001 From: SoundMindsAI Date: Tue, 19 May 2026 14:41:37 -0400 Subject: [PATCH 2/5] docs: rename chore folder to chore_data_table_columnvisibility_tanstack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folder rename + 4 live-code-comment fixups. Per the preflight verdict, the residual chore scope is item 5 only (TanStack state.columnVisibility migration); the original "primitive_followups" name was broader than the work that remains. Updated references in ui/src/ inline citations (4 files, 6 sites) so greps for the new slug find them. Left state.md historical entries unchanged — they correctly record PR #132's name-at-the-time as "chore_data_table_primitive_followups," and rewriting historical records to match a current rename would create a mismatch with the immutable PR title on GitHub. Implemented_features/* historical docs also left as-is for the same reason. Dropped the now-stale "Folder-name note" section from the renamed idea.md since the rename is no longer a candidate — it's done. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/00_overview/MVP1_DASHBOARD.md | 2 +- docs/00_overview/mvp1_dashboard.html | 2 +- .../idea.md | 4 ---- ui/src/__tests__/helpers/data-table-url-mock.ts | 2 +- ui/src/__tests__/hooks/use-data-table-url-state.test.tsx | 6 +++--- ui/src/hooks/use-data-table-url-state.ts | 6 +++--- ui/src/hooks/use-local-storage-set.ts | 2 +- 7 files changed, 10 insertions(+), 14 deletions(-) rename docs/02_product/planned_features/{chore_data_table_primitive_followups => chore_data_table_columnvisibility_tanstack}/idea.md (95%) diff --git a/docs/00_overview/MVP1_DASHBOARD.md b/docs/00_overview/MVP1_DASHBOARD.md index edb67229..85503558 100644 --- a/docs/00_overview/MVP1_DASHBOARD.md +++ b/docs/00_overview/MVP1_DASHBOARD.md @@ -95,7 +95,7 @@ _None._ | Feature | Type | One-liner | Depends on | Status | |---|---|---|---|---| | [chore_ci_prettier_check](../02_product/planned_features/chore_ci_prettier_check/idea.md) | Chore | `.github/workflows/pr.yml`'s `frontend` job runs: | — | Idea — surfaced during `/impl-execute --ad-hoc` of `infra_e2e_wire_seed_helper_into_studies_spec`. | -| [chore_data_table_primitive_followups](../02_product/planned_features/chore_data_table_primitive_followups/idea.md) | Chore | `feat_data_table_primitive` shipped with six known non-regression follow-up items captured only in chat transcripts. None block the PR but each is a real improvement that would otherwise evaporate whe | — | Partial — items 1, 2, 4, 6 shipped in PR #132 (squash commit `ee4c8d4`) 2026-05-17; item 3 closed 2026-05-19 (won't fix — flat-prop API locked as canonical, plan addendum required). Item 5 is the only residual implementation work: 1 primitive + (optionally) per-wrapper touch-ups. | +| [chore_data_table_columnvisibility_tanstack](../02_product/planned_features/chore_data_table_columnvisibility_tanstack/idea.md) | Chore | `feat_data_table_primitive` shipped with six known non-regression follow-up items captured only in chat transcripts. None block the PR but each is a real improvement that would otherwise evaporate whe | — | Partial — items 1, 2, 4, 6 shipped in PR #132 (squash commit `ee4c8d4`) 2026-05-17; item 3 closed 2026-05-19 (won't fix — flat-prop API locked as canonical, plan addendum required). Item 5 is the only residual implementation work: 1 primitive + (optionally) per-wrapper touch-ups. | | [chore_detail_page_shell_primitive](../02_product/planned_features/chore_detail_page_shell_primitive/idea.md) | Chore | Six of the seven `/{entity}/[id]` detail routes hand-roll the same three-state scaffold around their data query. The structure is **identical** down to the className strings, with two minor copy varia | — | Idea — surfaced during a UI-refactor audit run after [`chore_form_dropdown_primitive`](../../../00_overview/implemented_features/2026_05_18_chore_form_dropdown_primitive/feature_spec.md) (PR #136) and [`feat_data_table_primitive`](../../../00_overview/implemented_features/2026_05_16_feat_data_table_primitive/feature_spec.md) (PR #126) merged. | | [chore_extract_shadcn_select_test_mock](../02_product/planned_features/chore_extract_shadcn_select_test_mock/idea.md) | Chore | A factor-and-share refactor was attempted during `chore_form_dropdown_primitive` post-implementation. I extracted the mock to `ui/src/__tests__/helpers/shadcn-select-mock.tsx` exporting `mockShadcnSel | — | Idea — captured during `chore_form_dropdown_primitive` execution (PR pending). | | [chore_form_dropdown_guide_screenshot_refresh](../02_product/planned_features/chore_form_dropdown_guide_screenshot_refresh/idea.md) | Chore | Each affected guide has a Playwright spec at `ui/tests/e2e/guides/*.spec.ts` that captures screenshots when run against the real backend. The PR's UI changes produce different screenshots: | — | Idea — captured during `chore_form_dropdown_primitive` post-implementation guide-impact assessment. | diff --git a/docs/00_overview/mvp1_dashboard.html b/docs/00_overview/mvp1_dashboard.html index b53fc158..cf9da97b 100644 --- a/docs/00_overview/mvp1_dashboard.html +++ b/docs/00_overview/mvp1_dashboard.html @@ -443,7 +443,7 @@

Idea 5

-
Data Table Primitive Followups
+
Data Table Columnvisibility Tanstack
Chore diff --git a/docs/02_product/planned_features/chore_data_table_primitive_followups/idea.md b/docs/02_product/planned_features/chore_data_table_columnvisibility_tanstack/idea.md similarity index 95% rename from docs/02_product/planned_features/chore_data_table_primitive_followups/idea.md rename to docs/02_product/planned_features/chore_data_table_columnvisibility_tanstack/idea.md index dbed968b..44093f48 100644 --- a/docs/02_product/planned_features/chore_data_table_primitive_followups/idea.md +++ b/docs/02_product/planned_features/chore_data_table_columnvisibility_tanstack/idea.md @@ -88,10 +88,6 @@ Each item was reviewed and adjudicated "non-regression follow-up" during the par - **Q1 — Item 3 scope.** ✅ **Locked 2026-05-19: close item 3 (won't fix), lock the flat-prop API as canonical, add a "Shipped contract" addendum to the historical implementation plan's Story 2.6 key-interface block.** See item 3 above for the full rationale. - **Q2 — Item 5 cleanup scope: just the primitive, or also the wrapper-level prop changes?** Migrating to TanStack `state.columnVisibility` is straightforward in [`data-table.tsx:259-267`](../../../../ui/src/components/common/data-table.tsx) (pre-filter → full columns + `state.columnVisibility` derived from the same `useLocalStorageSet`). Recommended default: **primitive-only.** The 8 thin wrappers don't touch column visibility today; the localStorage key + the `useLocalStorageSet` integration stay unchanged. Refactor is contained to the primitive's `useReactTable()` call and the `visibleColumns`/`columns` props passed downstream. -## Folder-name note - -With Q1 locked (item 3 closed), the residual scope is item 5 only (TanStack `state.columnVisibility` migration) plus the historical-plan addendum. Current folder name `chore_data_table_primitive_followups` is now broader than the residual work. Candidate rename: **`chore_data_table_columnvisibility_tanstack`** (4 tokens, intent-clear). Rename requires explicit user confirmation per the preflight skill's folder-rename gate — not applied automatically. - ## Relationship to other work - **Parent:** `feat_data_table_primitive` (PR #126). diff --git a/ui/src/__tests__/helpers/data-table-url-mock.ts b/ui/src/__tests__/helpers/data-table-url-mock.ts index a77996f4..55225f65 100644 --- a/ui/src/__tests__/helpers/data-table-url-mock.ts +++ b/ui/src/__tests__/helpers/data-table-url-mock.ts @@ -7,7 +7,7 @@ * - `ui/src/__tests__/app/judgments/[id]/page.test.tsx` * - `ui/src/__tests__/components/query-sets/queries-table.test.tsx` * - * Origin: `chore_data_table_primitive_followups` item 1. + * Origin: `chore_data_table_columnvisibility_tanstack` item 1. * * ## Usage * diff --git a/ui/src/__tests__/hooks/use-data-table-url-state.test.tsx b/ui/src/__tests__/hooks/use-data-table-url-state.test.tsx index 9258b7fa..80df23c1 100644 --- a/ui/src/__tests__/hooks/use-data-table-url-state.test.tsx +++ b/ui/src/__tests__/hooks/use-data-table-url-state.test.tsx @@ -33,7 +33,7 @@ const columns: DataTableColumnDef[] = [ filter: { kind: 'enum', wireValues: ['llm', 'human'], sourceOfTruth: 'test' }, }, // A sortable column used by the hydration test below — without one, the - // chore_data_table_primitive_followups item 6 sort-token validation + // chore_data_table_columnvisibility_tanstack item 6 sort-token validation // (added 2026-05-17) drops `?sort=name:asc` as "no such sortable column." { id: 'name', header: 'Name', sortable: true }, ]; @@ -175,7 +175,7 @@ describe('useDataTableUrlState — anyMatcherActive', () => { }); // --------------------------------------------------------------------------- -// chore_data_table_primitive_followups item 6 — URL-state validation +// chore_data_table_columnvisibility_tanstack item 6 — URL-state validation // --------------------------------------------------------------------------- describe('useDataTableUrlState — enum filter validation', () => { it('drops a filter value not in the column wireValues allowlist', () => { @@ -247,7 +247,7 @@ describe('useDataTableUrlState — sort token validation', () => { }); // --------------------------------------------------------------------------- -// chore_data_table_primitive_followups item 4 — pageSize allowlist coercion +// chore_data_table_columnvisibility_tanstack item 4 — pageSize allowlist coercion // --------------------------------------------------------------------------- describe('useDataTableUrlState — pageSize validation', () => { it('coerces ?limit= values outside pageSizeOptions to defaultPageSize', () => { diff --git a/ui/src/hooks/use-data-table-url-state.ts b/ui/src/hooks/use-data-table-url-state.ts index b8e7ecb5..73e4d538 100644 --- a/ui/src/hooks/use-data-table-url-state.ts +++ b/ui/src/hooks/use-data-table-url-state.ts @@ -79,7 +79,7 @@ export function useDataTableUrlState( // Per-column wireValues allowlist for enum filters. fk-select filters // can't be validated at hook-time (their option IDs load async) so they - // pass through unchanged. Per `chore_data_table_primitive_followups` + // pass through unchanged. Per `chore_data_table_columnvisibility_tanstack` // item 6 — defense-in-depth: a direct URL with `?status=invented` // hydrates as an empty filter instead of being sent to the backend // and surfacing as 422 VALIDATION_ERROR. @@ -95,7 +95,7 @@ export function useDataTableUrlState( // Sort-token allowlist: `:` is only valid when `col` is the // `sortKey` (or `id`) of a sortable column AND `dir` is in that column's - // `sortDirections` (defaults to both `asc` + `desc`). Per `chore_data_table_primitive_followups` + // `sortDirections` (defaults to both `asc` + `desc`). Per `chore_data_table_columnvisibility_tanstack` // item 6 — direct URLs like `?sort=garbage:asc` or `?sort=name:upward` // hydrate as no sort instead of flowing to the backend (and either // 422-ing or silently no-op'ing). @@ -157,7 +157,7 @@ export function useDataTableUrlState( const cursor = searchParams.get('cursor'); // Page-size validation. When `pageSizeOptions` is provided, coerce // out-of-allowlist `?limit=` values to `defaultPageSize`. Per - // `chore_data_table_primitive_followups` item 4 — guards against + // `chore_data_table_columnvisibility_tanstack` item 4 — guards against // ad-hoc `?limit=99` URLs that produce inconsistent page sizes // across the surface; backend caps at 200 (api-conventions §Pagination) // but the frontend allowlist is typically much tighter. diff --git a/ui/src/hooks/use-local-storage-set.ts b/ui/src/hooks/use-local-storage-set.ts index dde67fb5..a095b10f 100644 --- a/ui/src/hooks/use-local-storage-set.ts +++ b/ui/src/hooks/use-local-storage-set.ts @@ -19,7 +19,7 @@ * interface block proposed `{ value: string[], add, remove, toggle }` as * an early sketch — the consumer pattern that emerged during build-out * was Set-shaped. The shipped impl is the canonical contract; the plan - * proposal is closed via `chore_data_table_primitive_followups` item 2. + * proposal is closed via `chore_data_table_columnvisibility_tanstack` item 2. */ import { useCallback, useEffect, useState } from 'react'; From 961399eca78470f771104e48cc1fcb69a3bcd56c Mon Sep 17 00:00:00 2001 From: SoundMindsAI Date: Tue, 19 May 2026 14:45:27 -0400 Subject: [PATCH 3/5] feat(ui): migrate DataTable column visibility to TanStack state.columnVisibility (item 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes item 5 from chore_data_table_columnvisibility_tanstack (originally Epic 2 GPT-5.5 cycle 3 finding #3, Low, deferred from PR #126). Before: `data-table.tsx` pre-filtered the `columns` prop into a `visibleColumns` array before passing to `useReactTable`. Functional, but non-idiomatic for TanStack v8+ — `state.columnVisibility: Record` is the canonical API and the entry point that future column-state features (ordering, pinning, resizing) plug into. After: pass the full `columns` array to `useReactTable` with `state: { columnVisibility }` derived from the same `useLocalStorageSet`-backed `hiddenColumns`. TanStack now hides columns from `getHeaderGroups()` / `getRowModel().rows[*].getVisibleCells()` internally, replacing the explicit `columns.filter(...)`. Non-hideable columns (sticky OR `hideable === false`) remain force-shown via the same predicate, preserving the tamper-resistance from Story 2.10. No `onColumnVisibilityChange` handler is wired. The column-visibility menu (`DataTableColumnVisibilityMenu`) keeps its existing `onToggle={(id) => hiddenColumns.toggle(id)}` path — `useLocalStorageSet` stays the single source of truth for persisted visibility. The hook's state update re-renders the primitive, which recomputes the `columnVisibility` map, which feeds back into TanStack on the next render. One-way data flow preserved. Behavioral contract unchanged: - Sticky / non-hideable columns force-shown. - localStorage write path unchanged (same key, same Set persistence). - Menu rendering unchanged (reads from `hiddenColumns.has(c.id)`). - Sort + filter + selection + keyboard nav + density unchanged. Verification: all 399 vitest cases pass, `tsc --noEmit` clean, eslint clean (52 pre-existing warnings, 0 errors). Co-Authored-By: Claude Opus 4.7 (1M context) --- ui/src/components/common/data-table.tsx | 30 +++++++++++++++++++------ 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/ui/src/components/common/data-table.tsx b/ui/src/components/common/data-table.tsx index 15ee868f..7f389532 100644 --- a/ui/src/components/common/data-table.tsx +++ b/ui/src/components/common/data-table.tsx @@ -253,18 +253,34 @@ export function DataTable(props: DataTableProps) { // TanStack Table model. `getRowId: (row) => row.id` keys row identity on the // backend UUID rather than the row index — required for stable selection, // keyboard activation, and per-row testids when rows shift across pages. - // Story 2.10 — column visibility state derived from the localStorage set. - // Non-hideable columns (sticky OR hideable === false) are force-shown even - // if a tampered localStorage entry tries to hide them. - const visibleColumns = columns.filter( - (c) => c.sticky || c.hideable === false || !hiddenColumns.has(c.id), - ); + // + // Story 2.10 + chore_data_table_columnvisibility_tanstack item 5 — column + // visibility flows through TanStack's `state.columnVisibility` API (the + // idiomatic v8+ shape) rather than pre-filtering the `columns` prop. The + // visibility map is derived from the same localStorage-backed + // `useLocalStorageSet`; toggles still write through that hook (see the + // `DataTableColumnVisibilityMenu`'s `onToggle` below). Non-hideable columns + // (sticky OR hideable === false) are force-shown even if a tampered + // localStorage entry tries to hide them. + // + // No `onColumnVisibilityChange` handler is passed because nothing inside + // TanStack mutates visibility — the menu calls `hiddenColumns.toggle(id)` + // directly, the hook re-renders, this `columnVisibility` record recomputes, + // and TanStack re-renders with the new map. This keeps `useLocalStorageSet` + // as the single source of truth for persisted visibility. + const columnVisibility = columns.reduce>((acc, c) => { + acc[c.id] = c.sticky || c.hideable === false || !hiddenColumns.has(c.id); + return acc; + }, {}); const table = useReactTable({ data: data as T[], // TanStack Table's `columns` prop is typed as a mutable `ColumnDef[]`, // but the consumer's column config is always a frozen `as const` array. // Cast to satisfy the API; the array is not mutated by TanStack. - columns: visibleColumns as unknown as import('@tanstack/react-table').ColumnDef[], + columns: columns as unknown as import('@tanstack/react-table').ColumnDef[], + state: { + columnVisibility, + }, getCoreRowModel: getCoreRowModel(), getRowId: (row) => row.id, }); From 3f336fed6c3ee52683b61ebccf57b42ef55a68c0 Mon Sep 17 00:00:00 2001 From: SoundMindsAI Date: Tue, 19 May 2026 14:58:10 -0400 Subject: [PATCH 4/5] fix(ui): memoize DataTable columnVisibility per Gemini PR #150 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini suggested wrapping the columnVisibility derivation in useMemo to give TanStack a stable reference between renders (otherwise a new object identity each parent render forces TanStack's state-diffing to re-evaluate every row's visibility unnecessarily). Verified locally: - `pnpm --dir ui lint` clean (0 errors). The react-hooks/preserve-manual-memoization rule that state.md referenced from PR #136 isn't actually active in the current eslint config (eslint.config.mjs has next/core-web-vitals + eslint-plugin-security only — no react-compiler plugin). Manual useMemo is the right tool here. - `pnpm --dir ui typecheck` clean. - All 399 vitest cases pass. Dependency array is `[columns, hiddenColumns.value]` — the underlying Set reference, not the convenience-method wrapper, so the memo invalidates exactly when the persisted hidden-columns set changes. Switched the inline check from `hiddenColumns.has(c.id)` to `hiddenColumns.value.has(c.id)` so the lint rule sees the dep explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) --- ui/src/components/common/data-table.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/ui/src/components/common/data-table.tsx b/ui/src/components/common/data-table.tsx index 7f389532..c41d020c 100644 --- a/ui/src/components/common/data-table.tsx +++ b/ui/src/components/common/data-table.tsx @@ -19,7 +19,7 @@ */ import { flexRender, getCoreRowModel, useReactTable } from '@tanstack/react-table'; -import { useEffect, useRef, useState, type KeyboardEvent } from 'react'; +import { useEffect, useMemo, useRef, useState, type KeyboardEvent } from 'react'; import { CursorPaginator } from '@/components/common/cursor-paginator'; import { InfoTooltip } from '@/components/common/info-tooltip'; @@ -268,10 +268,14 @@ export function DataTable(props: DataTableProps) { // directly, the hook re-renders, this `columnVisibility` record recomputes, // and TanStack re-renders with the new map. This keeps `useLocalStorageSet` // as the single source of truth for persisted visibility. - const columnVisibility = columns.reduce>((acc, c) => { - acc[c.id] = c.sticky || c.hideable === false || !hiddenColumns.has(c.id); - return acc; - }, {}); + const columnVisibility = useMemo>( + () => + columns.reduce>((acc, c) => { + acc[c.id] = c.sticky || c.hideable === false || !hiddenColumns.value.has(c.id); + return acc; + }, {}), + [columns, hiddenColumns.value], + ); const table = useReactTable({ data: data as T[], // TanStack Table's `columns` prop is typed as a mutable `ColumnDef[]`, From cfa753cc602775b92f1c3740da737afaae0f786c Mon Sep 17 00:00:00 2001 From: SoundMindsAI Date: Tue, 19 May 2026 15:18:58 -0400 Subject: [PATCH 5/5] docs: annotate state.md PR #136 React Compiler precedent (PR #150 verification) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While adjudicating Gemini's PR #150 useMemo suggestion I rejected it citing state.md PR #136's claim that "manual useMemo triggers the React Compiler ESLint rule react-hooks/preserve-manual-memoization." Verified by reading ui/eslint.config.mjs — that rule is NOT configured. The flat config loads only next/core-web-vitals + eslint-plugin-security. Manual useMemo is lint-clean. Accepted Gemini's PR #150 finding after verifying. Adds a forward-looking correction to the PR #136 entry rather than rewriting the historical claim. Tells future agents not to lean on this precedent without verifying eslint config; evaluate useMemo suggestions on per-derivation merit instead. Inline-fix per CLAUDE.md tangential-discoveries rubric (≤50 LOC, docs-only, no tests needed — implement now, don't capture as idea). Drops the chore_state_md_react_compiler_precedent capture commit that would have deferred this. Co-Authored-By: Claude Opus 4.7 (1M context) --- state.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/state.md b/state.md index 8fbf5ffb..0c498812 100644 --- a/state.md +++ b/state.md @@ -25,7 +25,7 @@ - **2026-05-18 — `chore_detail_page_shell_primitive` idea captured** as PR #147 (squash commit `8854e47`). Docs-only PR. Third primitive-extraction candidate after PR #126 (DataTable) and PR #136 (EntitySelect) — 6 of 7 detail pages (`clusters/[id]`, `studies/[id]`, `proposals/[id]`, `query-sets/[id]`, `templates/[id]`, `judgments/[id]`; chat excluded as structurally different) hand-roll the same `isPending` → `isError` → data scaffold with identical className strings and copy. Latent UX inconsistency: only `proposals/[id]` distinguishes 404 from network error today; the primitive flattens this. Preflight surfaced 6 corrections from the initial draft (mostly API-shape fixes: discriminate by `errorCode` string per `ApiError`, not HTTP status; add optional `entityTitle` override for compound labels like "Judgment list"; correct LOC math from "150-200 saved" to "+170 LOC PR, ROI is consistency"). **Gemini Code Assist:** 3 Medium findings, all addressed (1 already-fixed-by-preflight, 2 accepted — explicit `docker compose stop ui` command + `entityTitle` optional prop + Suspense out-of-scope reasoning clarified). Next step is `/pipeline` to ship the actual implementation. -- **2026-05-18 — `chore_form_dropdown_primitive` merged into `main`** as PR #136 (squash commit `cb7d9ee`). Shipped via `/pipeline --auto` end-to-end (idea-preflight × 2 → spec-gen → impl-plan-gen → impl-execute → merge). The form-side peer to `DataTableFkSelect`: a generic `>` primitive at `ui/src/components/common/entity-select.tsx` consolidating four hand-rolled FK `` onto one shared API. Plus a sibling vitest lint guard (`form-select-discipline.test.tsx`) blocking inline `` for backend-validated enums in form components — companion to the Story 2.13 column-discipline guard from `feat_data_table_primitive`. **Migrations (4 components, 7 FK sites):** `create-query-set-modal.tsx` cluster_id (UUID `` → cluster dropdown with health-status dots; label renamed "Cluster ID" → "Cluster"; submit gains explicit required-field guard); `create-study-modal.tsx` 4 FK selects at lines 246 / 283 / 301 / 326 (cluster/query-set/judgment-list/template — child-field reset preserved verbatim); `register-cluster-modal.tsx` config-repo (conditional render → always-visible with empty-state CTA Link — UX improvement, operators previously missed the field's existence); `generate-judgments-dialog.tsx` template (preserves `gen-template` data-testid for the existing E2E spec). **Primitive shape:** generic over `T`, takes `query: UseQueryResult` directly (consumer calls the listing hook in its own scope — refactored mid-implementation from the original `useEntities: () => useHook()` callback shape after hitting rules-of-hooks for dynamic-param consumers like `useQuerySets({ cluster_id })`). Ships loading / error-with-retry / empty-with-CTA / disabled-subset-with-tooltip slots. Opt-in `getStatus` callback renders Unicode-dot status indicators (`●` wrapped `aria-hidden="true"` so screen readers get clean names) with stable green-first sort. Opt-in `inlineWarning` renders an amber `

` under the trigger matching the existing helper-text styling. The `unreachable → unknown` wire-value mapping is caller-side per `HEALTH_STATUS_VALUES` from `enums.ts`. **Modal-level testing landmine:** shadcn `` blocks + one UUID-paste `` onto one shared API. Plus a sibling vitest lint guard (`form-select-discipline.test.tsx`) blocking inline `` for backend-validated enums in form components — companion to the Story 2.13 column-discipline guard from `feat_data_table_primitive`. **Migrations (4 components, 7 FK sites):** `create-query-set-modal.tsx` cluster_id (UUID `` → cluster dropdown with health-status dots; label renamed "Cluster ID" → "Cluster"; submit gains explicit required-field guard); `create-study-modal.tsx` 4 FK selects at lines 246 / 283 / 301 / 326 (cluster/query-set/judgment-list/template — child-field reset preserved verbatim); `register-cluster-modal.tsx` config-repo (conditional render → always-visible with empty-state CTA Link — UX improvement, operators previously missed the field's existence); `generate-judgments-dialog.tsx` template (preserves `gen-template` data-testid for the existing E2E spec). **Primitive shape:** generic over `T`, takes `query: UseQueryResult` directly (consumer calls the listing hook in its own scope — refactored mid-implementation from the original `useEntities: () => useHook()` callback shape after hitting rules-of-hooks for dynamic-param consumers like `useQuerySets({ cluster_id })`). Ships loading / error-with-retry / empty-with-CTA / disabled-subset-with-tooltip slots. Opt-in `getStatus` callback renders Unicode-dot status indicators (`●` wrapped `aria-hidden="true"` so screen readers get clean names) with stable green-first sort. Opt-in `inlineWarning` renders an amber `

` under the trigger matching the existing helper-text styling. The `unreachable → unknown` wire-value mapping is caller-side per `HEALTH_STATUS_VALUES` from `enums.ts`. **Modal-level testing landmine:** shadcn `