diff --git a/docs/superpowers/plans/2026-04-18-base-csv-export.md b/docs/superpowers/plans/2026-04-18-base-csv-export.md new file mode 100644 index 00000000..032c2596 --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-base-csv-export.md @@ -0,0 +1,803 @@ +# Base CSV Export Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add a server-streamed CSV export for a base's rows, triggered from a download button in the base toolbar. + +**Architecture:** A new `/bases/export-csv` Nest controller route writes directly to the Fastify reply as `text/csv`. The service pipes `BaseRowRepo.streamByBaseId` (the existing keyset-paginated async generator used by type-conversion / cell-gc) through `csv-stringify` in streaming mode so memory stays flat for 100k-row bases. Cells are serialized per property type by a pure helper with a pre-built property index and a per-chunk user-name map (same pattern as `base-type-conversion.task.ts`). The client fetches the endpoint as a blob and hands it to `file-saver`, mirroring `exportPage` in `page-service.ts`. + +**Tech Stack:** NestJS + Fastify (server), Kysely (db), `csv-stringify` (stream CSV encoder), React + Mantine (client), `file-saver` (download). + +**Scope (v1):** +- Exports **all live rows** of the base (ignoring current view's filter / sort / column visibility). View-scoped export is a later follow-up. +- All properties (including hidden + system) in property-position order. Primary property first. +- Permission: same as reading the base (`SpaceCaslAction.Read` on `SpaceCaslSubject.Base`). +- UTF-8, RFC 4180 CSV, with BOM for Excel compatibility. +- Filename: `{sanitize(base.name)}.csv`. + +**Non-goals (v1):** row selection export, filtered/sorted export, choosing columns, alternative formats (xlsx), background jobs. Only the current synchronous streamed export. + +--- + +## File Structure + +**New files:** +- `apps/server/src/core/base/export/cell-csv-serializer.ts` — pure per-type cell → string function. +- `apps/server/src/core/base/export/cell-csv-serializer.spec.ts` — unit tests for the serializer. +- `apps/server/src/core/base/services/base-csv-export.service.ts` — orchestrates streaming from repo → CSV stringifier → Fastify reply. Builds per-chunk user-name map for PERSON / LAST_EDITED_BY. +- `apps/server/src/core/base/dto/export-base.dto.ts` — `{ baseId: string }` DTO. + +**Modified files:** +- `apps/server/src/core/base/controllers/base.controller.ts` — new `POST /bases/export-csv` handler (returns `FastifyReply`). +- `apps/server/src/core/base/base.module.ts` — register `BaseCsvExportService` as provider. +- `apps/server/package.json` — add `csv-stringify` dependency. +- `apps/client/src/features/base/services/base-service.ts` — new `exportBaseToCsv(baseId)` function. +- `apps/client/src/features/base/components/base-toolbar.tsx` — new `IconDownload` button that calls the export service. + +--- + +## Task 1: Add `csv-stringify` dependency + +**Files:** +- Modify: `apps/server/package.json` + +- [ ] **Step 1: Install the dependency** + +Run from repo root: +```bash +pnpm --filter server add csv-stringify@^6 +``` + +Expected: `csv-stringify` appears under `dependencies` in `apps/server/package.json` (latest v6 or newer). + +- [ ] **Step 2: Verify server still builds** + +```bash +pnpm nx run server:build +``` + +Expected: build succeeds. + +- [ ] **Step 3: Commit** + +```bash +git add apps/server/package.json pnpm-lock.yaml +git commit -m "chore(server): add csv-stringify dependency" +``` + +--- + +## Task 2: Pure cell-to-CSV serializer — failing test first + +**Files:** +- Create: `apps/server/src/core/base/export/cell-csv-serializer.spec.ts` + +Contract the serializer must satisfy: + +| Property type | Input | Output | +|---|---|---| +| `text`, `url`, `email` | `"hi"` | `"hi"` | +| `number` | `42` | `"42"` | +| `checkbox` | `true` / `false` / `null` | `"true"` / `"false"` / `""` | +| `date` | `"2026-04-18T12:00:00Z"` | same ISO string | +| `select` / `status` | choice-uuid | choice name from `typeOptions.choices` | +| `multiSelect` | `[uuid1, uuid2]` | `"Name 1; Name 2"` (in given order) | +| `person` | `uuid` or `[uuid, ...]` | `"Alice; Bob"` from `userNames` map; fallback to `""` when missing | +| `file` | `[{fileName: "a.pdf"}, {fileName: "b.png"}]` | `"a.pdf; b.png"` | +| `createdAt` | `row.createdAt` (ISO) | same | +| `lastEditedAt` | `row.updatedAt` (ISO) | same | +| `lastEditedBy` | `row.lastUpdatedById` | resolved name from map or `""` | +| any | `null` / `undefined` | `""` | + +- [ ] **Step 1: Write the failing spec** + +Create `apps/server/src/core/base/export/cell-csv-serializer.spec.ts`: + +```ts +import { serializeCellForCsv } from './cell-csv-serializer'; +import { BasePropertyType } from '../base.schemas'; + +const p = (type: string, typeOptions: unknown = {}) => ({ + id: 'prop-1', + type: type as any, + typeOptions, +}); + +describe('serializeCellForCsv', () => { + const userNames = new Map([ + ['u1', 'Alice'], + ['u2', 'Bob'], + ]); + + it('returns empty string for null/undefined', () => { + expect(serializeCellForCsv(p(BasePropertyType.TEXT), null, {})).toBe(''); + expect(serializeCellForCsv(p(BasePropertyType.NUMBER), undefined, {})).toBe(''); + }); + + it('stringifies text/url/email as-is', () => { + expect(serializeCellForCsv(p(BasePropertyType.TEXT), 'hi', {})).toBe('hi'); + expect(serializeCellForCsv(p(BasePropertyType.URL), 'https://x', {})).toBe('https://x'); + expect(serializeCellForCsv(p(BasePropertyType.EMAIL), 'a@b.com', {})).toBe('a@b.com'); + }); + + it('stringifies number', () => { + expect(serializeCellForCsv(p(BasePropertyType.NUMBER), 42, {})).toBe('42'); + expect(serializeCellForCsv(p(BasePropertyType.NUMBER), 0, {})).toBe('0'); + }); + + it('renders checkbox as true/false', () => { + expect(serializeCellForCsv(p(BasePropertyType.CHECKBOX), true, {})).toBe('true'); + expect(serializeCellForCsv(p(BasePropertyType.CHECKBOX), false, {})).toBe('false'); + }); + + it('resolves select/status choice name', () => { + const prop = p(BasePropertyType.SELECT, { + choices: [ + { id: 'c1', name: 'Red', color: 'red' }, + { id: 'c2', name: 'Green', color: 'green' }, + ], + }); + expect(serializeCellForCsv(prop, 'c1', {})).toBe('Red'); + expect(serializeCellForCsv(prop, 'unknown', {})).toBe(''); + }); + + it('joins multiSelect names with "; " preserving order', () => { + const prop = p(BasePropertyType.MULTI_SELECT, { + choices: [ + { id: 'c1', name: 'A', color: 'red' }, + { id: 'c2', name: 'B', color: 'blue' }, + ], + }); + expect(serializeCellForCsv(prop, ['c2', 'c1'], {})).toBe('B; A'); + }); + + it('resolves person scalar and array', () => { + const prop = p(BasePropertyType.PERSON); + expect(serializeCellForCsv(prop, 'u1', { userNames })).toBe('Alice'); + expect(serializeCellForCsv(prop, ['u1', 'u2', 'missing'], { userNames })).toBe( + 'Alice; Bob', + ); + }); + + it('joins file names from cell payload', () => { + const prop = p(BasePropertyType.FILE); + expect( + serializeCellForCsv( + prop, + [ + { id: 'f1', fileName: 'a.pdf' }, + { id: 'f2', fileName: 'b.png' }, + ], + {}, + ), + ).toBe('a.pdf; b.png'); + }); + + it('dates pass through as ISO strings', () => { + const iso = '2026-04-18T12:00:00.000Z'; + expect(serializeCellForCsv(p(BasePropertyType.DATE), iso, {})).toBe(iso); + }); + + it('lastEditedBy resolves via userNames', () => { + const prop = p(BasePropertyType.LAST_EDITED_BY); + expect(serializeCellForCsv(prop, 'u2', { userNames })).toBe('Bob'); + expect(serializeCellForCsv(prop, 'missing', { userNames })).toBe(''); + }); +}); +``` + +- [ ] **Step 2: Run spec — verify it fails** + +```bash +pnpm --filter server test -- cell-csv-serializer.spec +``` + +Expected: FAIL — `Cannot find module './cell-csv-serializer'`. + +--- + +## Task 3: Implement `cell-csv-serializer.ts` + +**Files:** +- Create: `apps/server/src/core/base/export/cell-csv-serializer.ts` + +- [ ] **Step 1: Implement the serializer** + +```ts +import { BasePropertyType, BasePropertyTypeValue } from '../base.schemas'; + +export type CellCsvContext = { + userNames?: Map; +}; + +type PropertyLike = { + id: string; + type: BasePropertyTypeValue | string; + typeOptions?: unknown; +}; + +function resolveChoiceName(typeOptions: unknown, id: unknown): string { + if (!typeOptions || typeof typeOptions !== 'object') return ''; + const choices = (typeOptions as any).choices; + if (!Array.isArray(choices)) return ''; + const match = choices.find((c: any) => c?.id === id); + return typeof match?.name === 'string' ? match.name : ''; +} + +function resolveUser(id: unknown, ctx: CellCsvContext): string { + if (typeof id !== 'string') return ''; + return ctx.userNames?.get(id) ?? ''; +} + +export function serializeCellForCsv( + property: PropertyLike, + value: unknown, + ctx: CellCsvContext, +): string { + if (value === null || value === undefined) return ''; + + switch (property.type) { + case BasePropertyType.TEXT: + case BasePropertyType.URL: + case BasePropertyType.EMAIL: + return String(value); + + case BasePropertyType.NUMBER: + return typeof value === 'number' ? String(value) : String(value ?? ''); + + case BasePropertyType.CHECKBOX: + return value === true ? 'true' : 'false'; + + case BasePropertyType.DATE: + case BasePropertyType.CREATED_AT: + case BasePropertyType.LAST_EDITED_AT: + if (value instanceof Date) return value.toISOString(); + return String(value); + + case BasePropertyType.SELECT: + case BasePropertyType.STATUS: + return resolveChoiceName(property.typeOptions, value); + + case BasePropertyType.MULTI_SELECT: + if (!Array.isArray(value)) return ''; + return value + .map((v) => resolveChoiceName(property.typeOptions, v)) + .filter((s) => s.length > 0) + .join('; '); + + case BasePropertyType.PERSON: { + const ids = Array.isArray(value) ? value : [value]; + return ids + .map((id) => resolveUser(id, ctx)) + .filter((s) => s.length > 0) + .join('; '); + } + + case BasePropertyType.FILE: + if (!Array.isArray(value)) return ''; + return value + .map((f: any) => + f && typeof f === 'object' && typeof f.fileName === 'string' + ? f.fileName + : '', + ) + .filter((s) => s.length > 0) + .join('; '); + + case BasePropertyType.LAST_EDITED_BY: + return resolveUser(value, ctx); + + default: + return typeof value === 'object' ? JSON.stringify(value) : String(value); + } +} +``` + +- [ ] **Step 2: Run spec — verify it passes** + +```bash +pnpm --filter server test -- cell-csv-serializer.spec +``` + +Expected: PASS (11 tests). + +- [ ] **Step 3: Commit** + +```bash +git add apps/server/src/core/base/export/cell-csv-serializer.ts apps/server/src/core/base/export/cell-csv-serializer.spec.ts +git commit -m "feat(base): add csv cell serializer with per-type rules" +``` + +--- + +## Task 4: Export DTO + +**Files:** +- Create: `apps/server/src/core/base/dto/export-base.dto.ts` + +- [ ] **Step 1: Write the DTO** + +```ts +import { IsUUID } from 'class-validator'; + +export class ExportBaseCsvDto { + @IsUUID() + baseId: string; +} +``` + +- [ ] **Step 2: Commit** + +```bash +git add apps/server/src/core/base/dto/export-base.dto.ts +git commit -m "feat(base): add export base csv dto" +``` + +--- + +## Task 5: `BaseCsvExportService` — streams rows through `csv-stringify` + +**Files:** +- Create: `apps/server/src/core/base/services/base-csv-export.service.ts` + +**Design notes:** +- Sync values are pushed to a `csv-stringify` `Stringifier` stream. The stream is `pipe`d to `FastifyReply.raw` (Fastify's underlying Node http response). +- Per chunk (from `streamByBaseId`, chunkSize 1000): + 1. Collect all user IDs referenced by PERSON cells + all `lastUpdatedById` values. + 2. One `SELECT id, name, email FROM users WHERE id IN (...)` per chunk. Build `Map`. + 3. For each row, for each property in order, run `serializeCellForCsv`. `push` the record array into the stringifier. +- Header row: property names in property-position order. Primary property is first because properties are already sorted by `position`. +- Column for system types (`createdAt` / `lastEditedAt` / `lastEditedBy`): value pulled from the row column, not cells. +- BOM: write `\ufeff` to the raw socket before the stream pipe so Excel auto-detects UTF-8. + +- [ ] **Step 1: Implement the service** + +```ts +import { Injectable, Logger, NotFoundException } from '@nestjs/common'; +import { InjectKysely } from 'nestjs-kysely'; +import { KyselyDB } from '@docmost/db/types/kysely.types'; +import { BaseRepo } from '@docmost/db/repos/base/base.repo'; +import { BasePropertyRepo } from '@docmost/db/repos/base/base-property.repo'; +import { BaseRowRepo } from '@docmost/db/repos/base/base-row.repo'; +import { stringify } from 'csv-stringify'; +import { FastifyReply } from 'fastify'; +import { PassThrough } from 'node:stream'; +import { sanitize } from 'sanitize-filename-ts'; +import { + BasePropertyType, + BasePropertyTypeValue, +} from '../base.schemas'; +import { + CellCsvContext, + serializeCellForCsv, +} from '../export/cell-csv-serializer'; + +const CHUNK_SIZE = 1000; + +@Injectable() +export class BaseCsvExportService { + private readonly logger = new Logger(BaseCsvExportService.name); + + constructor( + @InjectKysely() private readonly db: KyselyDB, + private readonly baseRepo: BaseRepo, + private readonly basePropertyRepo: BasePropertyRepo, + private readonly baseRowRepo: BaseRowRepo, + ) {} + + async streamBaseAsCsv( + baseId: string, + workspaceId: string, + reply: FastifyReply, + ): Promise { + const base = await this.baseRepo.findById(baseId); + if (!base || base.workspaceId !== workspaceId) { + throw new NotFoundException('Base not found'); + } + + const properties = await this.basePropertyRepo.findByBaseId(baseId); + + const fileName = sanitize(base.name || 'base') + '.csv'; + + const stringifier = stringify({ + header: true, + columns: properties.map((p) => ({ key: p.id, header: p.name })), + }); + + // Prepend UTF-8 BOM so Excel auto-detects encoding, then pipe the + // CSV stream through. Using a PassThrough instead of `reply.raw` + // keeps us inside Fastify's managed reply lifecycle — backpressure + // is handled by the pipe, matching the existing `/spaces/export` + // pattern (stream handed to `res.send`). + const out = new PassThrough(); + out.write('\ufeff'); + + stringifier.on('error', (err) => { + this.logger.error('csv stringifier error', err); + out.destroy(err); + }); + stringifier.pipe(out); + + reply.headers({ + 'Content-Type': 'text/csv; charset=utf-8', + 'Content-Disposition': + 'attachment; filename="' + encodeURIComponent(fileName) + '"', + }); + reply.send(out); + + try { + for await (const chunk of this.baseRowRepo.streamByBaseId(baseId, { + workspaceId, + chunkSize: CHUNK_SIZE, + })) { + const ctx = await this.buildCtx(chunk, properties); + + for (const row of chunk) { + const record: Record = {}; + const cells = (row.cells ?? {}) as Record; + + for (const prop of properties) { + const type = prop.type as BasePropertyTypeValue; + let value: unknown; + if (type === BasePropertyType.CREATED_AT) { + value = row.createdAt; + } else if (type === BasePropertyType.LAST_EDITED_AT) { + value = row.updatedAt; + } else if (type === BasePropertyType.LAST_EDITED_BY) { + value = row.lastUpdatedById; + } else { + value = cells[prop.id]; + } + record[prop.id] = serializeCellForCsv(prop, value, ctx); + } + + // Pipe handles backpressure internally, but honor the + // stringifier's `write() === false` to avoid unbounded + // internal buffering on very large bases. + if (!stringifier.write(record)) { + await new Promise((resolve) => + stringifier.once('drain', resolve), + ); + } + } + } + + stringifier.end(); + } catch (err) { + this.logger.error(`csv export failed base=${baseId}`, err); + stringifier.destroy(err as Error); + throw err; + } + } + + private async buildCtx( + chunk: Array<{ cells: unknown; lastUpdatedById: string | null }>, + properties: Array<{ id: string; type: string }>, + ): Promise { + const needsUsers = properties.some( + (p) => + p.type === BasePropertyType.PERSON || + p.type === BasePropertyType.LAST_EDITED_BY, + ); + if (!needsUsers) return {}; + + const userIds = new Set(); + const personPropIds = properties + .filter((p) => p.type === BasePropertyType.PERSON) + .map((p) => p.id); + + for (const row of chunk) { + if (row.lastUpdatedById) userIds.add(row.lastUpdatedById); + const cells = (row.cells ?? {}) as Record; + for (const pid of personPropIds) { + const v = cells[pid]; + if (typeof v === 'string') userIds.add(v); + else if (Array.isArray(v)) { + for (const id of v) if (typeof id === 'string') userIds.add(id); + } + } + } + + if (userIds.size === 0) return {}; + + const rows = await this.db + .selectFrom('users') + .select(['id', 'name', 'email']) + .where('id', 'in', Array.from(userIds)) + .execute(); + + return { + userNames: new Map(rows.map((u) => [u.id, u.name || u.email || ''])), + }; + } +} +``` + +- [ ] **Step 2: Commit** + +```bash +git add apps/server/src/core/base/services/base-csv-export.service.ts +git commit -m "feat(base): add streaming csv export service" +``` + +--- + +## Task 6: Register service in module + +**Files:** +- Modify: `apps/server/src/core/base/base.module.ts` + +- [ ] **Step 1: Add `BaseCsvExportService` to providers** + +Add import: +```ts +import { BaseCsvExportService } from './services/base-csv-export.service'; +``` + +Add to `providers` array (no need to export — only the controller uses it). + +- [ ] **Step 2: Commit** + +```bash +git add apps/server/src/core/base/base.module.ts +git commit -m "feat(base): register csv export service in module" +``` + +--- + +## Task 7: Controller route `POST /bases/export-csv` + +**Files:** +- Modify: `apps/server/src/core/base/controllers/base.controller.ts` + +- [ ] **Step 1: Add handler** + +Follow the exact precedent in `apps/server/src/integrations/export/export.controller.ts:47-109` (Fastify reply injection + permission check pattern) and the Read permission check pattern from the existing `info` handler in this controller. + +```ts +// Add to constructor args: +private readonly baseCsvExportService: BaseCsvExportService, + +// Imports: +import { FastifyReply } from 'fastify'; +import { Res } from '@nestjs/common'; +import { ExportBaseCsvDto } from '../dto/export-base.dto'; +import { BaseCsvExportService } from '../services/base-csv-export.service'; +import { AuthWorkspace } from '../../../common/decorators/auth-workspace.decorator'; +import { Workspace } from '@docmost/db/types/entity.types'; + +// Handler: +@HttpCode(HttpStatus.OK) +@Post('export-csv') +async exportCsv( + @Body() dto: ExportBaseCsvDto, + @AuthUser() user: User, + @AuthWorkspace() workspace: Workspace, + @Res() res: FastifyReply, +) { + const base = await this.baseRepo.findById(dto.baseId); + if (!base) { + throw new NotFoundException('Base not found'); + } + + const ability = await this.spaceAbility.createForUser(user, base.spaceId); + if (ability.cannot(SpaceCaslAction.Read, SpaceCaslSubject.Base)) { + throw new ForbiddenException(); + } + + await this.baseCsvExportService.streamBaseAsCsv( + dto.baseId, + workspace.id, + res, + ); +} +``` + +- [ ] **Step 2: Build to verify wiring** + +```bash +pnpm nx run server:build +``` + +Expected: build succeeds. + +- [ ] **Step 3: Commit** + +```bash +git add apps/server/src/core/base/controllers/base.controller.ts +git commit -m "feat(base): add csv export http endpoint" +``` + +--- + +## Task 8: Manual server smoke test — USER-DRIVEN + +> ⚠️ **Do not run `pnpm dev` as an agent.** Per `CLAUDE.md`, the agent builds but does not launch. Hand off to the user with the instructions below; resume the plan at Task 9 after the user confirms the curl succeeds. + +- [ ] **Step 1: Ask the user to start the dev servers (`pnpm dev`) and open the app** + +- [ ] **Step 2: Ask the user to run this curl, replacing `` and ``** + +Get a session cookie by logging into the client at http://localhost:3000, then grab `authToken` from DevTools → Application → Cookies. + +```bash +curl -v -X POST http://localhost:3001/api/bases/export-csv \ + -H "Content-Type: application/json" \ + -H "Cookie: authToken=" \ + -d '{"baseId": ""}' \ + --output /tmp/base-export.csv + +head -5 /tmp/base-export.csv +wc -l /tmp/base-export.csv +``` + +Expected: +- `Content-Disposition: attachment; filename="..."` in response headers. +- First bytes of the file are the UTF-8 BOM (`efbbbf`) — check with `xxd /tmp/base-export.csv | head -1`. +- Header row contains property names. +- Line count ≈ live row count + 1. +- Opens cleanly in `less /tmp/base-export.csv` (no JSON blobs, no raw UUIDs for select / person / file). + +If the base has a PERSON column, confirm it renders the user's name, not a UUID. + +--- + +## Task 9: Client service function + +**Files:** +- Modify: `apps/client/src/features/base/services/base-service.ts` + +- [ ] **Step 1: Add `exportBaseToCsv`** + +Mirror `exportPage` in `apps/client/src/features/page/services/page-service.ts:116-135`: + +```ts +import { saveAs } from "file-saver"; + +export async function exportBaseToCsv(baseId: string): Promise { + const req = await api.post( + "/bases/export-csv", + { baseId }, + { responseType: "blob" }, + ); + + const header = req?.headers["content-disposition"] ?? ""; + const match = header.match(/filename="?([^"]+)"?/); + let fileName = match ? match[1] : "base.csv"; + try { + fileName = decodeURIComponent(fileName); + } catch { + // fallback to raw filename + } + + saveAs(req.data, fileName); +} +``` + +- [ ] **Step 2: Commit** + +```bash +git add apps/client/src/features/base/services/base-service.ts +git commit -m "feat(base): add client csv export service call" +``` + +--- + +## Task 10: Toolbar export button + +**Files:** +- Modify: `apps/client/src/features/base/components/base-toolbar.tsx` + +- [ ] **Step 1: Wire the button** + +Add imports: +```tsx +import { IconDownload } from "@tabler/icons-react"; +import { notifications } from "@mantine/notifications"; +import { exportBaseToCsv } from "@/features/base/services/base-service"; +``` + +Add a handler inside the component: +```tsx +const [exporting, setExporting] = useState(false); +const handleExport = useCallback(async () => { + if (exporting) return; + setExporting(true); + try { + await exportBaseToCsv(base.id); + } catch (err) { + notifications.show({ + color: "red", + message: t("Failed to export CSV"), + }); + } finally { + setExporting(false); + } +}, [base.id, exporting, t]); +``` + +Insert the button inside `
` — place it before the filter/sort/fields group (leftmost of the right cluster): + +```tsx + + + + + +``` + +- [ ] **Step 2: Build client** + +```bash +pnpm nx run client:build +``` + +Expected: build succeeds. + +- [ ] **Step 3: Commit** + +```bash +git add apps/client/src/features/base/components/base-toolbar.tsx +git commit -m "feat(base): add csv export button to base toolbar" +``` + +--- + +## Task 11: End-to-end UI smoke test — USER-DRIVEN + +> ⚠️ **Do not run `pnpm dev` as an agent.** Ask the user to verify in their own browser session; resume at Task 12 after confirmation. + +- [ ] **Step 1: Ask the user to open a base in the browser (dev server already running from Task 8)** + +Navigate to a base with ≥ 1 row of each property type (or create cells manually: text, select, multi-select, person, file, checkbox, number, date). + +- [ ] **Step 2: Click the download icon in the toolbar** + +Expected: +- Button shows loading spinner briefly. +- Browser downloads a `.csv` named after the base. +- CSV opens in Excel / Numbers / Google Sheets with correct column headers. +- Select, multi-select, person, and file columns render names (not UUIDs). +- No console errors in either tab. + +- [ ] **Step 3: Test an empty base** + +Open a brand-new base (only primary property, no rows). Click export. + +Expected: file contains just the header row + BOM. No error. + +- [ ] **Step 4: Test permission** + +As a user without access to the space, hit the endpoint (or simulate by setting user in session). Expected: 403. + +--- + +## Task 12: Final commit + handoff + +- [ ] **Step 1: Verify branch is clean and all commits are on the branch** + +```bash +git status +git log --oneline main..HEAD +``` + +Expected: clean working tree, 8 commits (Tasks 1, 3, 4, 5, 6, 7, 9, 10). + +- [ ] **Step 2: Open the `superpowers:finishing-a-development-branch` skill to decide on merge/PR** + +--- + +## Follow-ups (out of scope for v1) + +- Export respecting current view (filter / sort / column visibility / column order). +- Export only selected rows (wired into `selection-action-bar.tsx`). +- Format-aware number rendering (currency, percent, progress). +- Configurable date format (respect view/base locale). +- Large-export BullMQ job + email download link for very large bases. +- Alternative formats: JSON, XLSX. diff --git a/docs/superpowers/plans/2026-04-18-base-table-skeleton.md b/docs/superpowers/plans/2026-04-18-base-table-skeleton.md new file mode 100644 index 00000000..3556271a --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-base-table-skeleton.md @@ -0,0 +1,342 @@ +# Base Table Skeleton Loading State Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace the centered Mantine `` that currently renders while a base is loading with a layout-matching skeleton of the toolbar + grid built from Mantine `` shimmers, so there is no layout shift when real data lands. + +**Architecture:** A new self-contained component that renders the same DOM skeleton as the real table (toolbar row + header row + N body rows) using Mantine's `Skeleton` primitive, styled with the existing grid CSS module so tracks and heights match 1:1. `BaseTable` swaps its loading branch from `` to ``. + +**Tech Stack:** React 18, Mantine v8 `Skeleton`, existing CSS module (`grid.module.css`). + +--- + +## Background + +Current loading branch in [`base-table.tsx:157-163`](apps/client/src/features/base/components/base-table.tsx:157): + +```tsx +if (baseLoading || rowsLoading) { + return ( +
+ +
+ ); +} +``` + +`.loadingOverlay` ([`grid.module.css:290-295`](apps/client/src/features/base/styles/grid.module.css:290)) is a centered flex container. Only used here. + +Real table structure (for reference so the skeleton matches): + +- **Toolbar row** — view tabs on the left (each is a ~32px-wide pill), four `ActionIcon`s on the right (16px icons). +- **Header row** — subgrid. Pinned row-number column (64px). Primary column pinned. Each header cell is 34px tall, has a 14px type icon, and a short property-name label. +- **Body rows** — subgrid, 36px min-height, cells separated by 1px borders. + +Matching the real layout 1:1 means: +- Same `display: grid` + `grid-template-columns` on the outer container. +- Same `.headerRow` / `.row` / `.cell` classes from `grid.module.css` so padding, borders, and heights line up. +- When the real data lands, the only visual change is `` → real content — no reflow, no column-width jump. + +**Skeleton dimensions (tuned for a neutral default, since we don't yet know the view's column widths):** + +- 6 columns, 180px each (matches `DEFAULT_COLUMN_WIDTH` in [`use-base-table.ts:25`](apps/client/src/features/base/hooks/use-base-table.ts:25)). +- Row-number column: 64px (matches `ROW_NUMBER_COLUMN_WIDTH`). +- 10 body rows. +- Toolbar: 3 view tab pills (44px each), 4 action icons (22px each). + +Varying the per-cell skeleton width within each column (between ~50% and ~85% of the cell width) adds realism — otherwise every cell skeleton is identical and screams "fake". + +--- + +## File Structure + +**New files:** +- `apps/client/src/features/base/components/base-table-skeleton.tsx` — the skeleton component. +- `apps/client/src/features/base/styles/base-table-skeleton.module.css` — minimal additional styles (the skeleton cell wrapper needs width-constrained `` children that center vertically in the 36px cell). + +**Modified files:** +- `apps/client/src/features/base/components/base-table.tsx` — swap the loading branch to render ``; drop the now-unused `Loader` import. +- `apps/client/src/features/base/styles/grid.module.css` — remove `.loadingOverlay` (dead). + +No new deps — `Skeleton` is already exported from `@mantine/core`. + +--- + +## Task 1: Build the skeleton component + +**Files:** +- Create: `apps/client/src/features/base/components/base-table-skeleton.tsx` +- Create: `apps/client/src/features/base/styles/base-table-skeleton.module.css` + +- [ ] **Step 1: Create the CSS module** + +`apps/client/src/features/base/styles/base-table-skeleton.module.css`: + +```css +.toolbar { + display: flex; + align-items: center; + gap: var(--mantine-spacing-xs); + padding: var(--mantine-spacing-xs) 0; + margin-bottom: var(--mantine-spacing-xs); +} + +.toolbarTabs { + display: flex; + gap: 6px; + flex: 1; +} + +.toolbarActions { + display: flex; + gap: var(--mantine-spacing-xs); + margin-left: auto; +} + +.gridWrapper { + overflow: hidden; + flex: 1; + border-top: 1px solid + light-dark(var(--mantine-color-gray-2), var(--mantine-color-dark-4)); +} + +.grid { + display: grid; +} + +.cellInner { + display: flex; + align-items: center; + height: 100%; + width: 100%; + padding: 0 8px; +} + +.headerCellInner { + display: flex; + align-items: center; + gap: 6px; + height: 100%; + width: 100%; + padding: 0 8px; +} +``` + +- [ ] **Step 2: Create the skeleton component** + +`apps/client/src/features/base/components/base-table-skeleton.tsx`: + +```tsx +import { Skeleton } from "@mantine/core"; +import gridClasses from "@/features/base/styles/grid.module.css"; +import classes from "@/features/base/styles/base-table-skeleton.module.css"; + +const ROW_NUMBER_WIDTH = 64; +const COLUMN_WIDTH = 180; +const COLUMN_COUNT = 6; +const ROW_COUNT = 10; + +// Pseudo-random but deterministic widths so the skeleton doesn't flicker +// between renders. Values are a rough normal distribution around +// 60-85 % of the cell width. +const CELL_WIDTH_RATIOS = [0.78, 0.62, 0.84, 0.55, 0.71, 0.66]; +const HEADER_WIDTH_RATIOS = [0.42, 0.58, 0.5, 0.64, 0.46, 0.54]; + +export function BaseTableSkeleton() { + const gridTemplateColumns = [ + `${ROW_NUMBER_WIDTH}px`, + ...Array.from({ length: COLUMN_COUNT }, () => `${COLUMN_WIDTH}px`), + ].join(" "); + + return ( +
+
+
+ + + +
+
+ + + + +
+
+ +
+
+ {/* Header row */} +
+
+ +
+
+ {Array.from({ length: COLUMN_COUNT }).map((_, colIndex) => ( +
+
+ + +
+
+ ))} + + {/* Body rows */} + {Array.from({ length: ROW_COUNT }).map((_, rowIndex) => ( +
+
+
+ +
+
+ {Array.from({ length: COLUMN_COUNT }).map((_, colIndex) => ( +
+
+ +
+
+ ))} +
+ ))} +
+
+
+ ); +} +``` + +Key points the implementer must not change: +- `gridClasses.headerCell` and `gridClasses.cell` come from the REAL table's CSS module so borders, heights, and hover semantics match exactly. Don't reinvent them. +- The `style={{ display: "contents" }}` row wrapper is intentional — the outer `.grid` is a single CSS grid, and each "row" is just a flattened sequence of cells that span the grid columns via `display: contents`. This mirrors how the real table flattens rows (see `.row` with `grid-column: 1 / -1; grid-template-columns: subgrid;` in [`grid.module.css:119-123`](apps/client/src/features/base/styles/grid.module.css:119)). We use `contents` instead of subgrid because the skeleton's outer grid is not a subgrid. +- Using `Skeleton` with `circle` prop for the row-number placeholder and type-icon placeholders — these match the real UI's round/small icon presence. +- The `CELL_WIDTH_RATIOS[(rowIndex + colIndex) % ...]` gives each cell a deterministic-but-varied skeleton width so it doesn't look like a stamped pattern. + +- [ ] **Step 3: Build to verify TypeScript compiles** + +```bash +pnpm nx run client:build +``` + +Expected: success. + +- [ ] **Step 4: Commit** + +```bash +git add \ + apps/client/src/features/base/components/base-table-skeleton.tsx \ + apps/client/src/features/base/styles/base-table-skeleton.module.css +git commit -m "feat(base): add layout-matching skeleton loading component" +``` + +--- + +## Task 2: Swap the loader for the skeleton in BaseTable + +**Files:** +- Modify: `apps/client/src/features/base/components/base-table.tsx` +- Modify: `apps/client/src/features/base/styles/grid.module.css` + +- [ ] **Step 1: Replace the loading branch** + +In `base-table.tsx`: + +Drop `Loader` from the `@mantine/core` import (line 2). Leave `Text` and `Stack` — they're still used by the error branch. + +Add near the other imports: +```tsx +import { BaseTableSkeleton } from "@/features/base/components/base-table-skeleton"; +``` + +Change lines 157-163: + +Before: +```tsx +if (baseLoading || rowsLoading) { + return ( +
+ +
+ ); +} +``` + +After: +```tsx +if (baseLoading || rowsLoading) { + return ; +} +``` + +- [ ] **Step 2: Remove the dead `.loadingOverlay` class** + +In `apps/client/src/features/base/styles/grid.module.css`, delete lines 290-295 (the `.loadingOverlay { ... }` block — exact content): + +```css +.loadingOverlay { + display: flex; + align-items: center; + justify-content: center; + padding: var(--mantine-spacing-xl); +} +``` + +- [ ] **Step 3: Build** + +```bash +pnpm nx run client:build +``` + +Expected: success with no "unused" warnings from the removed class. + +- [ ] **Step 4: Commit** + +```bash +git add \ + apps/client/src/features/base/components/base-table.tsx \ + apps/client/src/features/base/styles/grid.module.css +git commit -m "feat(base): show table skeleton instead of centered loader on load" +``` + +--- + +## Task 3: USER smoke test + +> ⚠️ **Do not run `pnpm dev` as an agent.** Hand off. + +Ask the user to: + +- [ ] **Fresh load.** Open a base fresh (full tab reload). The skeleton should render immediately, then transition cleanly to the real table. No jarring jump, no centered spinner. + +- [ ] **Throttled load.** DevTools → Network tab → throttle to "Slow 3G" → reload. The skeleton should stay visible for the duration of the slow request, shimmer visible the whole time. + +- [ ] **Dark mode.** Toggle to dark mode. Skeleton colors should render appropriately (Mantine's `Skeleton` handles this automatically via light-dark tokens). + +- [ ] **Window resize during load.** Resize the browser window while the skeleton is showing. Skeleton's CSS grid should stretch the columns proportionally — no layout break. + +- [ ] **Error state still works.** Hard to trigger; if you can, disable network entirely and reload. You should see the existing "Failed to load base" message, NOT the skeleton stuck forever. + +- [ ] **No console errors / CSS warnings during transition from skeleton → real table.** + +If all pass, the swap is done. + +--- + +## Out of scope + +- Matching the exact column count and widths the view ends up rendering. The skeleton is a neutral placeholder; a perfect match would require knowing the view config, which we don't have before the base query resolves. A 6-column, 180px default is "close enough to not flash". +- Skeleton for `GridContainer` inside an already-loaded base (e.g., when switching views of the same base, where we already have properties). Out of scope — this plan only addresses the initial load path. +- Progressive hydration (render the toolbar first, then skeleton rows as they stream in). Overkill for a small base query. diff --git a/docs/superpowers/plans/2026-04-18-cell-person-keyboard-nav.md b/docs/superpowers/plans/2026-04-18-cell-person-keyboard-nav.md new file mode 100644 index 00000000..8c44603f --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-cell-person-keyboard-nav.md @@ -0,0 +1,776 @@ +# Base Cell Dropdown Keyboard Navigation Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add keyboard navigation (ArrowUp/Down/Home/End/Enter) to all four base cell dropdowns — `CellPerson`, `CellSelect`, `CellMultiSelect`, `CellStatus` — so users can pick values without a mouse, matching Mantine `MultiSelect`'s keyboard UX. + +**Architecture:** All four cells use the same custom `Popover` + HTML dropdown pattern (not `useCombobox`). Instead of editing each in isolation, factor the shared logic (activeIndex, arrow/Home/End handling, reset-on-filter, scroll-into-view, option ref tracking) into one hook `useListKeyboardNav`. Each cell flattens its visible items into a single linear list (including the "Add option" row for select/multi, and flattening across status categories), passes the count to the hook, and wires up Enter selection locally. + +**Tech Stack:** React 18, TypeScript, Mantine v8 `Popover` / `TextInput`, CSS Modules. + +--- + +## Scope + +**In scope:** +- `cell-person.tsx` — members, tag input with Backspace-removes-tag behavior. +- `cell-select.tsx` — single choice, optional "Add option" row. +- `cell-multi-select.tsx` — multi choice, optional "Add option" row. +- `cell-status.tsx` — single choice, grouped by category (flattened for nav). +- Shared hook `use-list-keyboard-nav.ts`. +- One new CSS class for keyboard-active highlight. + +**Out of scope:** +- Swapping any cell to Mantine `MultiSelect` / `useCombobox` — too disruptive; all four have deliberate custom UIs. +- Automated tests — this codebase has no existing unit tests for base cells, and a harness just for this is scope creep. Task 7 is a manual QUX walkthrough. +- Other editors in the base feature (e.g., toolbar pickers, filter UIs) — out of scope unless they hit the same bug. + +--- + +## File Structure + +**Create:** +- `apps/client/src/features/base/hooks/use-list-keyboard-nav.ts` — shared hook. + +**Modify:** +- `apps/client/src/features/base/styles/cells.module.css` — add `.selectOptionKeyboardActive`. +- `apps/client/src/features/base/components/cells/cell-person.tsx` +- `apps/client/src/features/base/components/cells/cell-select.tsx` +- `apps/client/src/features/base/components/cells/cell-multi-select.tsx` +- `apps/client/src/features/base/components/cells/cell-status.tsx` + +**No other files touched.** + +--- + +## Design Notes (read before coding) + +### Why a hook and not copy-paste per cell + +Four cells would get the same 40-line block. C-9 ("SHOULD NOT extract unless reused") is satisfied here — the logic is reused in 4 places, and diverging across them later would be a bug farm. The hook owns only the *navigation* concern (activeIndex, arrow keys, scroll). Each cell still owns its own Enter semantics, Escape, Backspace, and filter computation, because those diverge. + +### Why `activeIndex: -1` initially + +No highlight on open. First ArrowDown moves to 0. Enter at `-1` is a no-op — we don't guess, we don't commit. This matches Mantine MultiSelect behavior. + +### Why a new CSS class instead of reusing `selectOptionActive` + +`selectOptionActive` means "this item is currently selected" (blue). Keyboard nav needs a *separate* "Enter will land here" state or users can't tell which unselected option is focused. Add `selectOptionKeyboardActive` and stack it with `selectOptionActive` when both apply (selected + keyboard-focused uses a slightly darker blue). + +### Flattening the nav list + +- **cell-person:** `filteredMembers` (already flat). +- **cell-select / cell-multi-select:** `filteredChoices` plus one trailing "Add option" virtual entry when `showAddOption`. Arrow nav must include it; Enter on that index calls `handleAddOption()`. Represent as a discriminated union so Enter can dispatch correctly. +- **cell-status:** flatten `groups.flatMap(g => g.choices)`. Build a `Map` once per render and use it to attach refs and compute highlighting inside the grouped render loop. + +### Mouse + keyboard sync + +Mouse hover on an option sets `activeIndex` to that index. Prevents the "mouse hover shows A, keyboard focus is B, Enter selects B" mismatch. Applies to all four cells. + +### `onMouseDown.preventDefault` on options + +Without it, clicking an option can blur the input before `onClick` fires; in some browsers, Mantine's `trapFocus` + popover close sequence then cancels the selection. Mantine's `useCombobox` hides this — we don't use it, so add the guard. Applies to all four cells. + +### Reset triggers + +Reset `activeIndex` to `-1` whenever: +- the search string changes (filter changed → stale index) +- `isEditing` flips (reopening the editor) +- for cell-select/multi: whether the "Add option" row is visible, since that also changes the list length + +Pass all three as `resetDeps` to the hook. + +### Scroll into view + +Dropdowns are capped at `max-height: 240px` with `overflow-y: auto` ([cells.module.css:219-222](apps/client/src/features/base/styles/cells.module.css:219)). Use `scrollIntoView({ block: "nearest" })` on the active option when `activeIndex` changes. + +### Preserve existing behaviors + +- **cell-person:** Escape cancels, Backspace on empty search removes last tag — keep both. +- **cell-select:** Escape cancels, Enter with `showAddOption` and no active index adds — keep the Enter-adds-when-no-nav fallback. Priority: if `activeIndex >= 0`, Enter uses that (which may itself be the add-option virtual entry). Else fall through to the existing `handleAddOption()` call if `showAddOption`. +- **cell-multi-select:** same as cell-select. +- **cell-status:** Escape cancels. No Add-option row. Enter with `activeIndex >= 0` selects. + +--- + +## Task 1: Add keyboard-active CSS class + +**Files:** +- Modify: `apps/client/src/features/base/styles/cells.module.css` + +- [ ] **Step 1: Append after the existing `.selectOptionActive` rule (ending at line 240)** + +```css +.selectOptionKeyboardActive { + background-color: light-dark(var(--mantine-color-gray-0), var(--mantine-color-dark-5)); +} + +.selectOptionActive.selectOptionKeyboardActive { + background-color: light-dark(var(--mantine-color-blue-1), var(--mantine-color-blue-8)); +} +``` + +First rule: unselected + keyboard-focused (matches hover shade). Second: selected + keyboard-focused (slightly darker blue than plain selected, distinguishable). + +- [ ] **Step 2: Commit** + +```bash +git add apps/client/src/features/base/styles/cells.module.css +git commit -m "style(base): add keyboard-active option style for cell dropdowns" +``` + +--- + +## Task 2: Create the `useListKeyboardNav` hook + +**Files:** +- Create: `apps/client/src/features/base/hooks/use-list-keyboard-nav.ts` + +- [ ] **Step 1: Write the hook** + +```tsx +import { useCallback, useEffect, useRef, useState } from "react"; + +type UseListKeyboardNavResult = { + activeIndex: number; + setActiveIndex: (idx: number) => void; + handleNavKey: (e: React.KeyboardEvent) => boolean; + setOptionRef: (idx: number) => (el: HTMLElement | null) => void; +}; + +export function useListKeyboardNav( + itemCount: number, + resetDeps: ReadonlyArray, +): UseListKeyboardNavResult { + const [activeIndex, setActiveIndex] = useState(-1); + const optionRefs = useRef>([]); + + // Reset highlight when filter/open-state changes. resetDeps is intentional. + useEffect(() => { + setActiveIndex(-1); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, resetDeps); + + useEffect(() => { + if (activeIndex < 0) return; + const el = optionRefs.current[activeIndex]; + if (el) el.scrollIntoView({ block: "nearest" }); + }, [activeIndex]); + + const setOptionRef = useCallback( + (idx: number) => (el: HTMLElement | null) => { + optionRefs.current[idx] = el; + }, + [], + ); + + const handleNavKey = useCallback( + (e: React.KeyboardEvent): boolean => { + if (itemCount === 0) return false; + if (e.key === "ArrowDown") { + e.preventDefault(); + setActiveIndex((idx) => (idx < itemCount - 1 ? idx + 1 : 0)); + return true; + } + if (e.key === "ArrowUp") { + e.preventDefault(); + setActiveIndex((idx) => (idx <= 0 ? itemCount - 1 : idx - 1)); + return true; + } + if (e.key === "Home") { + e.preventDefault(); + setActiveIndex(0); + return true; + } + if (e.key === "End") { + e.preventDefault(); + setActiveIndex(itemCount - 1); + return true; + } + return false; + }, + [itemCount], + ); + + return { activeIndex, setActiveIndex, handleNavKey, setOptionRef }; +} +``` + +Notes: +- `handleNavKey` returns `true` if it handled the key, so callers can `if (nav.handleNavKey(e)) return;` before their own Enter/Escape/Backspace branches. +- Wrap-around on both ends. +- `resetDeps` uses an eslint-disable because it's a variadic dep array by design — the hook name and the `resetDeps` argument make the intent clear without a comment inside the body beyond the one-liner. This is the C-7-approved kind of caveat comment. + +- [ ] **Step 2: Build verification** + +Run: `pnpm nx run client:build`. +Expected: success, no TypeScript errors. + +- [ ] **Step 3: Commit** + +```bash +git add apps/client/src/features/base/hooks/use-list-keyboard-nav.ts +git commit -m "feat(base): add useListKeyboardNav hook for dropdown keyboard nav" +``` + +--- + +## Task 3: Wire keyboard nav into `CellPerson` + +**Files:** +- Modify: `apps/client/src/features/base/components/cells/cell-person.tsx` + +- [ ] **Step 1: Import the hook** + +Add to the imports at the top: + +```tsx +import { useListKeyboardNav } from "@/features/base/hooks/use-list-keyboard-nav"; +``` + +- [ ] **Step 2: Instantiate the hook** + +After the `filteredMembers` declaration (currently ending line 61), add: + +```tsx +const nav = useListKeyboardNav(filteredMembers.length, [search, isEditing]); +``` + +- [ ] **Step 3: Extend `handleKeyDown`** + +Replace the existing `handleKeyDown` `useCallback` (lines 97–109) with: + +```tsx +const handleKeyDown = useCallback( + (e: React.KeyboardEvent) => { + if (e.key === "Escape") { + e.preventDefault(); + onCancel(); + return; + } + if (nav.handleNavKey(e)) return; + if (e.key === "Enter") { + if (nav.activeIndex < 0 || nav.activeIndex >= filteredMembers.length) return; + e.preventDefault(); + handleSelect(filteredMembers[nav.activeIndex].id); + return; + } + if (e.key === "Backspace" && search === "" && personIds.length > 0) { + e.preventDefault(); + handleRemove(personIds[personIds.length - 1]); + } + }, + [onCancel, nav, filteredMembers, handleSelect, search, personIds, handleRemove], +); +``` + +- [ ] **Step 4: Render the keyboard-active highlight, ref, hover sync, and mousedown guard** + +Replace the filtered-members map (currently lines 173–191) with: + +```tsx +{filteredMembers.map((member, idx) => { + const isSelected = selectedSet.has(member.id); + const isKeyboardActive = idx === nav.activeIndex; + const className = [ + cellClasses.selectOption, + isSelected ? cellClasses.selectOptionActive : "", + isKeyboardActive ? cellClasses.selectOptionKeyboardActive : "", + ] + .filter(Boolean) + .join(" "); + return ( +
nav.setActiveIndex(idx)} + onMouseDown={(e) => { + // Keep focus on the search input so click doesn't blur + close popover. + e.preventDefault(); + }} + onClick={() => handleSelect(member.id)} + > + + + {member.name} + +
+ ); +})} +``` + +- [ ] **Step 5: Build verification** + +Run: `pnpm nx run client:build`. +Expected: success. + +- [ ] **Step 6: Commit** + +```bash +git add apps/client/src/features/base/components/cells/cell-person.tsx +git commit -m "feat(base): keyboard navigation for person cell dropdown" +``` + +--- + +## Task 4: Wire keyboard nav into `CellSelect` + +**Files:** +- Modify: `apps/client/src/features/base/components/cells/cell-select.tsx` + +Recall: this cell has `filteredChoices` plus a conditional "Add option" row when `showAddOption === true`. Both must be navigable. Enter on a choice selects it; Enter on the add-option virtual entry calls `handleAddOption`. + +- [ ] **Step 1: Import the hook** + +```tsx +import { useListKeyboardNav } from "@/features/base/hooks/use-list-keyboard-nav"; +``` + +- [ ] **Step 2: Build the nav item list and instantiate the hook** + +After the `showAddOption` declaration (currently line 71), add: + +```tsx +type NavItem = + | { kind: "choice"; choice: Choice } + | { kind: "add" }; + +const navItems: NavItem[] = useMemo( + () => [ + ...filteredChoices.map((c) => ({ kind: "choice" as const, choice: c })), + ...(showAddOption ? [{ kind: "add" as const }] : []), + ], + [filteredChoices, showAddOption], +); + +const nav = useListKeyboardNav(navItems.length, [search, isEditing, showAddOption]); +``` + +- [ ] **Step 3: Replace `handleKeyDown`** + +Replace the existing `handleKeyDown` `useCallback` (currently lines 98–110) with: + +```tsx +const handleKeyDown = useCallback( + (e: React.KeyboardEvent) => { + if (e.key === "Escape") { + e.preventDefault(); + onCancel(); + return; + } + if (nav.handleNavKey(e)) return; + if (e.key === "Enter") { + if (nav.activeIndex >= 0 && nav.activeIndex < navItems.length) { + e.preventDefault(); + const item = navItems[nav.activeIndex]; + if (item.kind === "choice") handleSelect(item.choice); + else handleAddOption(); + return; + } + if (showAddOption) { + e.preventDefault(); + handleAddOption(); + } + } + }, + [onCancel, nav, navItems, handleSelect, handleAddOption, showAddOption], +); +``` + +- [ ] **Step 4: Update the choices render loop** + +Replace the `filteredChoices.map` block (currently lines 146–161) with: + +```tsx +{filteredChoices.map((choice, idx) => { + const isSelected = choice.id === selectedId; + const isKeyboardActive = idx === nav.activeIndex; + const className = [ + cellClasses.selectOption, + isSelected ? cellClasses.selectOptionActive : "", + isKeyboardActive ? cellClasses.selectOptionKeyboardActive : "", + ] + .filter(Boolean) + .join(" "); + return ( +
nav.setActiveIndex(idx)} + onMouseDown={(e) => { + e.preventDefault(); + }} + onClick={() => handleSelect(choice)} + > + + {choice.name} + +
+ ); +})} +``` + +- [ ] **Step 5: Update the "Add option" row** + +Replace the `showAddOption && (...)` block (currently lines 162–175) with: + +```tsx +{showAddOption && (() => { + const idx = filteredChoices.length; + const isKeyboardActive = idx === nav.activeIndex; + return ( +
nav.setActiveIndex(idx)} + onMouseDown={(e) => { + e.preventDefault(); + }} + onClick={handleAddOption} + > + Add option: + + {trimmedSearch} + +
+ ); +})()} +``` + +The IIFE is the least-disruptive way to introduce a local `idx` binding without restructuring the parent JSX. + +- [ ] **Step 6: Build verification** + +Run: `pnpm nx run client:build`. +Expected: success. + +- [ ] **Step 7: Commit** + +```bash +git add apps/client/src/features/base/components/cells/cell-select.tsx +git commit -m "feat(base): keyboard navigation for single-select cell dropdown" +``` + +--- + +## Task 5: Wire keyboard nav into `CellMultiSelect` + +**Files:** +- Modify: `apps/client/src/features/base/components/cells/cell-multi-select.tsx` + +This mirrors Task 4. Only differences: `handleSelect` is named `handleToggle`, selected check uses `selectedSet.has(...)`, and `handleAddOption` commits `[...selectedIds, newChoice.id]` rather than replacing. + +- [ ] **Step 1: Import the hook** + +```tsx +import { useListKeyboardNav } from "@/features/base/hooks/use-list-keyboard-nav"; +``` + +- [ ] **Step 2: Build the nav item list and instantiate the hook** + +After `showAddOption` (currently line 74), add: + +```tsx +type NavItem = + | { kind: "choice"; choice: Choice } + | { kind: "add" }; + +const navItems: NavItem[] = useMemo( + () => [ + ...filteredChoices.map((c) => ({ kind: "choice" as const, choice: c })), + ...(showAddOption ? [{ kind: "add" as const }] : []), + ], + [filteredChoices, showAddOption], +); + +const nav = useListKeyboardNav(navItems.length, [search, isEditing, showAddOption]); +``` + +- [ ] **Step 3: Replace `handleKeyDown`** + +Replace lines 102–114 with: + +```tsx +const handleKeyDown = useCallback( + (e: React.KeyboardEvent) => { + if (e.key === "Escape") { + e.preventDefault(); + onCancel(); + return; + } + if (nav.handleNavKey(e)) return; + if (e.key === "Enter") { + if (nav.activeIndex >= 0 && nav.activeIndex < navItems.length) { + e.preventDefault(); + const item = navItems[nav.activeIndex]; + if (item.kind === "choice") handleToggle(item.choice); + else handleAddOption(); + return; + } + if (showAddOption) { + e.preventDefault(); + handleAddOption(); + } + } + }, + [onCancel, nav, navItems, handleToggle, handleAddOption, showAddOption], +); +``` + +- [ ] **Step 4: Update the choices render loop** + +Replace `filteredChoices.map(...)` (currently lines 143–160) with: + +```tsx +{filteredChoices.map((choice, idx) => { + const isSelected = selectedSet.has(choice.id); + const isKeyboardActive = idx === nav.activeIndex; + const className = [ + cellClasses.selectOption, + isSelected ? cellClasses.selectOptionActive : "", + isKeyboardActive ? cellClasses.selectOptionKeyboardActive : "", + ] + .filter(Boolean) + .join(" "); + return ( +
nav.setActiveIndex(idx)} + onMouseDown={(e) => { + e.preventDefault(); + }} + onClick={() => handleToggle(choice)} + > + + {choice.name} + +
+ ); +})} +``` + +- [ ] **Step 5: Update the "Add option" row** + +Replace `showAddOption && (...)` (currently lines 161–174) with the same IIFE pattern as Task 4: + +```tsx +{showAddOption && (() => { + const idx = filteredChoices.length; + const isKeyboardActive = idx === nav.activeIndex; + return ( +
nav.setActiveIndex(idx)} + onMouseDown={(e) => { + e.preventDefault(); + }} + onClick={handleAddOption} + > + Add option: + + {trimmedSearch} + +
+ ); +})()} +``` + +- [ ] **Step 6: Build verification** + +Run: `pnpm nx run client:build`. +Expected: success. + +- [ ] **Step 7: Commit** + +```bash +git add apps/client/src/features/base/components/cells/cell-multi-select.tsx +git commit -m "feat(base): keyboard navigation for multi-select cell dropdown" +``` + +--- + +## Task 6: Wire keyboard nav into `CellStatus` + +**Files:** +- Modify: `apps/client/src/features/base/components/cells/cell-status.tsx` + +This cell renders choices grouped by category. Flatten the groups for nav indexing while keeping the grouped rendering. + +- [ ] **Step 1: Import the hook** + +```tsx +import { useListKeyboardNav } from "@/features/base/hooks/use-list-keyboard-nav"; +``` + +- [ ] **Step 2: Flatten and instantiate the hook** + +After the `groups` declaration (currently ending line 74), add: + +```tsx +const flatChoices = useMemo( + () => groups.flatMap((g) => g.choices), + [groups], +); +const choiceIdxMap = useMemo(() => { + const m = new Map(); + flatChoices.forEach((c, i) => m.set(c.id, i)); + return m; +}, [flatChoices]); + +const nav = useListKeyboardNav(flatChoices.length, [search, isEditing]); +``` + +- [ ] **Step 3: Replace `handleKeyDown`** + +Replace lines 83–91 with: + +```tsx +const handleKeyDown = useCallback( + (e: React.KeyboardEvent) => { + if (e.key === "Escape") { + e.preventDefault(); + onCancel(); + return; + } + if (nav.handleNavKey(e)) return; + if (e.key === "Enter") { + if (nav.activeIndex < 0 || nav.activeIndex >= flatChoices.length) return; + e.preventDefault(); + handleSelect(flatChoices[nav.activeIndex]); + } + }, + [onCancel, nav, flatChoices, handleSelect], +); +``` + +- [ ] **Step 4: Update the choice render inside groups** + +Replace the inner `group.choices.map(...)` block (currently lines 132–149) with: + +```tsx +{group.choices.map((choice) => { + const idx = choiceIdxMap.get(choice.id) ?? -1; + const isSelected = choice.id === selectedId; + const isKeyboardActive = idx === nav.activeIndex; + const className = [ + cellClasses.selectOption, + isSelected ? cellClasses.selectOptionActive : "", + isKeyboardActive ? cellClasses.selectOptionKeyboardActive : "", + ] + .filter(Boolean) + .join(" "); + return ( +
nav.setActiveIndex(idx)} + onMouseDown={(e) => { + e.preventDefault(); + }} + onClick={() => handleSelect(choice)} + > + + {choice.name} + +
+ ); +})} +``` + +- [ ] **Step 5: Build verification** + +Run: `pnpm nx run client:build`. +Expected: success. + +- [ ] **Step 6: Commit** + +```bash +git add apps/client/src/features/base/components/cells/cell-status.tsx +git commit -m "feat(base): keyboard navigation for status cell dropdown" +``` + +--- + +## Task 7: Manual QUX verification + +No automated tests exist for cell components. Verify manually in a running dev client (only if the user asks — per CLAUDE.md, do not launch the client yourself). + +For **each** of the four cells (person, select, multi-select, status), walk this checklist in the base grid: + +**Golden path:** +1. Click the cell → popover opens, search focused, no initial highlight. +2. ArrowDown → first option highlights. +3. Repeat ArrowDown → highlight moves; dropdown scrolls past viewport. +4. Enter on highlight → that value is selected/toggled. +5. ArrowUp from index 0 → wraps to last item. +6. ArrowDown from last → wraps to first. + +**Search + keyboard:** +7. Type a partial string → list filters, highlight resets. +8. ArrowDown → lands on first *filtered* option, not a stale index. +9. Clear the search → list expands, highlight resets. + +**Mouse + keyboard interplay:** +10. Hover an option with mouse → that option becomes keyboard-active. +11. Move mouse away, press ArrowDown → nav continues from hovered index. +12. Click an option → selects cleanly, popover does not flicker-close (validates `onMouseDown.preventDefault`). + +**Edge cases:** +13. Empty filter result → ArrowUp/Down/Home/End/Enter are no-ops; Escape still closes; cell-person's Backspace-removes-tag still works. +14. Home / End → jump to first / last item. +15. Escape at any time → popover closes, no commit. + +**Cell-specific:** +- **cell-person** (multi mode): Backspace on empty search removes the last tag (existing behavior preserved). +- **cell-person** (single mode, `allowMultiple: false`): Enter still selects; selecting an already-selected person clears it. +- **cell-select** with typed new value: the "Add option" row appears as the last navigable item; ArrowDown reaches it and Enter triggers `handleAddOption`. Enter with no active index (user typed and hasn't pressed ArrowDown) still triggers `handleAddOption` (fallback preserved). +- **cell-multi-select**: same as cell-select for add-option behavior. +- **cell-status**: navigation crosses category boundaries seamlessly (To Do → In Progress → Complete). + +**Visual:** +- Selected-only → blue. +- Keyboard-focused-only → gray. +- Both → darker blue (distinguishable from plain selected). + +- [ ] **Step 1: Walk the checklist per cell. If any scenario fails, fix and re-verify that cell before moving on.** + +--- + +## Remember + +- Exact file paths above; don't grep for them at edit time. +- Preserve existing Escape/Backspace/Enter-adds-new behaviors verbatim. +- Don't swap to `useCombobox` — scope creep. +- One CSS class, one hook, four cell edits — that's the whole change. +- Commit after each task (GH-1, Conventional Commits). +- No Anthropic/Claude attribution in commits (undercover mode). diff --git a/docs/superpowers/plans/2026-04-18-filter-sort-draft-add.md b/docs/superpowers/plans/2026-04-18-filter-sort-draft-add.md new file mode 100644 index 00000000..c8b4dd61 --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-filter-sort-draft-add.md @@ -0,0 +1,527 @@ +# Draft-Then-Save Flow for New Sort / Filter Entries Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Clicking "Add sort" / "Add filter" no longer persists an incomplete entry to the view config; instead it opens a local draft row with Save / Cancel buttons so the user sets everything up in one go and commits once. + +**Architecture:** Each popover (`ViewSortConfigPopover`, `ViewFilterConfigPopover`) gets local `draft` state. When non-null, a draft row renders at the bottom with property/direction (or property/operator/value) selects and Save/Cancel buttons. Save appends the draft to the existing `sorts` / `conditions` array via the existing `onChange` callback — a single mutation round-trip. Cancel clears the draft. Existing rows keep their current inline auto-save behavior (out of scope — they're already workable). + +**Tech Stack:** React 18, Mantine v8 (`Popover`, `Select`, `Button`), existing `onChange` contract on both popovers (no changes to parents). + +--- + +## Background + +Current sort flow ([`view-sort-config.tsx:47-52`](apps/client/src/features/base/components/views/view-sort-config.tsx:47)): + +```ts +const handleAdd = useCallback(() => { + const usedIds = new Set(sorts.map((s) => s.propertyId)); + const available = properties.find((p) => !usedIds.has(p.id)); + if (!available) return; + onChange([...sorts, { propertyId: available.id, direction: "asc" }]); +}, [sorts, properties, onChange]); +``` + +Clicking "Add sort" immediately calls `onChange` with a default-populated entry — that fires a mutation. The user then picks property → mutation. Picks direction → mutation. Four round-trips for one configured sort. + +Filter is the same shape ([`view-filter-config.tsx:176-187`](apps/client/src/features/base/components/views/view-filter-config.tsx:176)). + +## Desired behavior + +1. Click "Add sort" / "Add filter" → draft row appears locally, NOT persisted yet. +2. User picks property, direction/operator, value in the draft row. +3. Click **Save** → draft is appended to `sorts` / `conditions` via `onChange` (one mutation). Draft clears. +4. Click **Cancel** → draft clears, nothing persisted. +5. While a draft is open, the "Add" button is hidden (or shown as "+ Save draft first"). +6. Closing the popover (outside click / ESC) with a draft open: the draft is discarded silently. (Matches how Notion / Airtable treat incomplete-and-abandoned filter drafts.) +7. Existing entries continue to auto-save on inline edit — unchanged. + +## File Structure + +**Modified:** +- `apps/client/src/features/base/components/views/view-sort-config.tsx` +- `apps/client/src/features/base/components/views/view-filter-config.tsx` + +No new files, no new deps, no server changes. + +--- + +## Task 1: Sort popover — draft-then-save + +**File:** `apps/client/src/features/base/components/views/view-sort-config.tsx` + +### Design + +Add local state: +```ts +const [draft, setDraft] = useState(null); +``` + +- On "Add sort" click: compute a sensible default (first unused property, `"asc"`) and call `setDraft(...)`. Do NOT call `onChange`. +- Render the draft row after the committed rows when `draft !== null`. It looks identical to a committed row but the property/direction selects bind to draft state via `setDraft`, and there are **Save** and **Cancel** buttons instead of a Trash icon. +- Save: `onChange([...sorts, draft]); setDraft(null);` +- Cancel: `setDraft(null);` +- When the popover closes (`opened` transitions `true → false`), auto-clear the draft via effect. +- Hide the "Add sort" button while a draft is open. + +### Step 1: Add `useState` + `useEffect` imports + +The file currently imports `useCallback` only. Replace with `useCallback, useEffect, useState`: + +```ts +import { useCallback, useEffect, useState } from "react"; +``` + +### Step 2: Import `Button` + +Add `Button` to the existing `@mantine/core` import block so the draft can show Save/Cancel buttons. + +### Step 3: Replace the component body + +Read the current component carefully (lines 27-153) and replace it with: + +```tsx +export function ViewSortConfigPopover({ + opened, + onClose, + sorts, + properties, + onChange, + children, +}: ViewSortConfigProps) { + const { t } = useTranslation(); + const [draft, setDraft] = useState(null); + + // Discard any half-configured draft when the popover closes. + useEffect(() => { + if (!opened) setDraft(null); + }, [opened]); + + const propertyOptions = properties.map((p) => ({ + value: p.id, + label: p.name, + })); + + const directionOptions = [ + { value: "asc", label: t("Ascending") }, + { value: "desc", label: t("Descending") }, + ]; + + const handleStartDraft = useCallback(() => { + const usedIds = new Set(sorts.map((s) => s.propertyId)); + const available = properties.find((p) => !usedIds.has(p.id)); + if (!available) return; + setDraft({ propertyId: available.id, direction: "asc" }); + }, [sorts, properties]); + + const handleSaveDraft = useCallback(() => { + if (!draft) return; + onChange([...sorts, draft]); + setDraft(null); + }, [draft, sorts, onChange]); + + const handleCancelDraft = useCallback(() => { + setDraft(null); + }, []); + + const handleRemove = useCallback( + (index: number) => { + onChange(sorts.filter((_, i) => i !== index)); + }, + [sorts, onChange], + ); + + const handlePropertyChange = useCallback( + (index: number, propertyId: string | null) => { + if (!propertyId) return; + onChange( + sorts.map((s, i) => (i === index ? { ...s, propertyId } : s)), + ); + }, + [sorts, onChange], + ); + + const handleDirectionChange = useCallback( + (index: number, direction: string | null) => { + if (!direction) return; + onChange( + sorts.map((s, i) => + i === index + ? { ...s, direction: direction as "asc" | "desc" } + : s, + ), + ); + }, + [sorts, onChange], + ); + + const canAddMore = properties.length > sorts.length + (draft ? 1 : 0); + + return ( + + {children} + + + + {t("Sort by")} + + + {sorts.length === 0 && !draft && ( + + {t("No sorts applied")} + + )} + + {sorts.map((sort, index) => ( + + handleDirectionChange(index, val)} + w={110} + /> + handleRemove(index)} + > + + + + ))} + + {draft && ( + + + + val && + setDraft({ + ...draft, + direction: val as "asc" | "desc", + }) + } + w={110} + /> + + + + + + + )} + + {!draft && canAddMore && ( + + + {t("Add sort")} + + )} + + + + ); +} +``` + +### Step 4: Build + +```bash +pnpm nx run client:build +``` + +Expected: success. `IconSortAscending` import becomes unused — remove it from the import line. + +### Step 5: Commit + +```bash +git add apps/client/src/features/base/components/views/view-sort-config.tsx +git commit -m "feat(base): draft flow with save and cancel for new view sorts" +``` + +--- + +## Task 2: Filter popover — draft-then-save + +**File:** `apps/client/src/features/base/components/views/view-filter-config.tsx` + +### Design + +Same shape as sort, with additional operator / value fields in the draft row. The `FilterValueInput` sub-component works as-is since it takes a `FilterCondition` and an `onChange(value)` — we pass the draft as the condition and a setter that updates the draft. + +Edge case: when the user changes the draft's **property**, the valid operator set changes. Mirror the existing `handlePropertyChange` logic: keep the current operator if still valid, otherwise reset to the first valid operator and clear the value. + +### Step 1: Add `useState, useEffect` to the react import + +Current: `import { useCallback } from "react";`. Change to: +```ts +import { useCallback, useEffect, useState } from "react"; +``` + +### Step 2: Add `Button` to the `@mantine/core` import + +### Step 3: Add three draft helpers inside the component + +After the existing `propertyOptions` declaration and before `handleAdd`, add: + +```ts +const [draft, setDraft] = useState(null); + +useEffect(() => { + if (!opened) setDraft(null); +}, [opened]); + +const handleStartDraft = useCallback(() => { + const firstProperty = properties[0]; + if (!firstProperty) return; + const validOperators = getOperatorsForType(firstProperty.type); + const defaultOperator = validOperators.includes("contains") + ? ("contains" as FilterOperator) + : validOperators[0]; + setDraft({ propertyId: firstProperty.id, op: defaultOperator }); +}, [properties]); + +const handleSaveDraft = useCallback(() => { + if (!draft) return; + onChange([...conditions, draft]); + setDraft(null); +}, [draft, conditions, onChange]); + +const handleCancelDraft = useCallback(() => { + setDraft(null); +}, []); + +const handleDraftPropertyChange = useCallback( + (propertyId: string | null) => { + if (!propertyId || !draft) return; + const newProperty = properties.find((p) => p.id === propertyId); + if (!newProperty) { + setDraft({ ...draft, propertyId }); + return; + } + const validOperators = getOperatorsForType(newProperty.type); + const currentOperatorValid = validOperators.includes(draft.op); + setDraft({ + ...draft, + propertyId, + op: currentOperatorValid ? draft.op : validOperators[0], + value: currentOperatorValid ? draft.value : undefined, + }); + }, + [draft, properties], +); + +const handleDraftOperatorChange = useCallback( + (operator: string | null) => { + if (!operator || !draft) return; + const op = operator as FilterOperator; + const needsValue = !NO_VALUE_OPERATORS.includes(op); + setDraft({ ...draft, op, value: needsValue ? draft.value : undefined }); + }, + [draft], +); + +const handleDraftValueChange = useCallback( + (value: string) => { + if (!draft) return; + setDraft({ ...draft, value: value || undefined }); + }, + [draft], +); +``` + +### Step 4: Replace the existing `handleAdd` function + +Delete the existing `handleAdd` declaration entirely — `handleStartDraft` replaces it. + +### Step 5: Render the draft row and hide the Add button when drafting + +Find the `` at the bottom of the dropdown (around line 325-338). Replace it with: + +```tsx +{draft && (() => { + const needsValue = !NO_VALUE_OPERATORS.includes(draft.op); + const property = properties.find((p) => p.id === draft.propertyId); + const validOperators = property + ? getOperatorsForType(property.type) + : OPERATORS.map((op) => op.value); + const operatorOptions = OPERATORS.filter((op) => + validOperators.includes(op.value), + ).map((op) => ({ value: op.value, label: t(op.labelKey) })); + + return ( + + + + {needsValue && ( + + )} + + + + + + + ); +})()} + +{!draft && ( + + + {t("Add filter")} + +)} +``` + +### Step 6: Update the empty-state text conditional + +The existing `{conditions.length === 0 && No filters applied}` should also hide when a draft is open — change to: + +```tsx +{conditions.length === 0 && !draft && ( + + {t("No filters applied")} + +)} +``` + +### Step 7: Build + +```bash +pnpm nx run client:build +``` + +Expected: success. + +### Step 8: Commit + +```bash +git add apps/client/src/features/base/components/views/view-filter-config.tsx +git commit -m "feat(base): draft flow with save and cancel for new view filters" +``` + +--- + +## Task 3: USER smoke test + +> ⚠️ **Do not run `pnpm dev` as an agent.** Hand off. + +Ask the user to: + +- [ ] **Sort: draft flow works.** + 1. Open the Sort popover. Click "Add sort". + 2. A draft row appears with Save / Cancel. The "Add sort" button is hidden. + 3. Pick a property, pick direction. + 4. Network tab stays quiet (no mutation yet). + 5. Click Save. A single `POST /bases/views/update` fires. The sort appears as a committed row; Save/Cancel row is gone; the "Add sort" button reappears. + +- [ ] **Sort: cancel discards without mutation.** + 1. Click "Add sort". Configure something in the draft. + 2. Click Cancel. No mutation fires. Draft disappears. + +- [ ] **Sort: closing popover with open draft discards it.** + 1. Click "Add sort". Click outside the popover (or press ESC). + 2. Popover closes. Re-open it — no draft, no committed new sort, no mutation was fired. + +- [ ] **Sort: existing rows still auto-save.** + 1. After committing a sort, change its direction via its Select. The usual mutation fires as before. + +- [ ] **Sort: max-reached hides Add button.** + 1. Add sorts until every property is used. The "Add sort" button should disappear (`canAddMore` is false). + +- [ ] **Filter: repeat the same five checks.** Also verify: + - Changing the draft's property resets the operator when incompatible (e.g., start with `contains` on text, switch property to a number → operator becomes `eq`). + - "Is empty" / "Is not empty" operators hide the value field in the draft. + +- [ ] **Regression: existing sorts/filters still load correctly on page load.** + +Report back if any step misbehaves. + +--- + +## Out of scope + +- Edit-mode for existing rows (also behind Save/Cancel). User didn't ask for this; existing inline auto-save is fine. +- Batching multiple quick edits on existing rows into a single mutation (a debounce like the hide flow has). Separate optimization. +- Adding a "reorder sorts" UI — unrelated. +- Any server-side change. The `onChange` contract is unchanged from the popovers' parents' perspective (`base-toolbar.tsx`). diff --git a/docs/superpowers/plans/2026-04-18-fix-client-sort-override.md b/docs/superpowers/plans/2026-04-18-fix-client-sort-override.md new file mode 100644 index 00000000..53b49219 --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-fix-client-sort-override.md @@ -0,0 +1,152 @@ +# Stop Client from Overriding Server Sort Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** When a view has a sort applied, stop the client from re-sorting the fetched rows by `position` (the fractional-index column), which silently overwrites the server's sort order and visibly corrupts the list as more pages are loaded. + +**Architecture:** One-line conditional in the `rows` `useMemo` in `base-table.tsx`: if a sort is active, pass through the server-ordered rows unchanged; otherwise keep the existing position-based sort (which is useful when no sort is active so that optimistically-created rows and ws-pushed rows land in their natural order). + +**Tech Stack:** React 18, TanStack Query v5 (infinite), the existing server-side keyset pagination on `(sort keys..., position, id)`. + +--- + +## Background + +Reported symptom: user applies "Title ascending" in the sort popover. Initial view looks plausible. Scroll down — further pages load in title order. Scroll back up — the top of the list is now a random-looking mess (screenshots confirm `Update Proposal Sierra`, `Echo November ...` at the top instead of `Alpha ...`). + +### Why it happens + +Row fetch uses an infinite query, keyed by sort/filter/search: + +```ts +// apps/client/src/features/base/queries/base-row-query.ts:57-72 +return useInfiniteQuery({ + queryKey: ["base-rows", baseId, activeFilter, activeSorts, activeSearch], + queryFn: ({ pageParam }) => + listRows(baseId!, { cursor: pageParam, limit: 100, filter, sorts, search }), + ... +}); +``` + +Server-side, [`base-row.repo.ts:list`](apps/server/src/database/repos/base/base-row.repo.ts) takes a different path when sort/filter/search is present: `runListQuery(base, opts)` builds a keyset-paginated SELECT ordered by the requested sort keys (with `position, id` as stable tie-breakers). Page N+1's cursor picks up exactly where page N left off. Server pages are correct. + +Client-side, [`base-table.tsx:66-69`](apps/client/src/features/base/components/base-table.tsx:66): + +```ts +const rows = useMemo(() => { + const flat = flattenRows(rowsData); + return flat.sort((a, b) => (a.position < b.position ? -1 : a.position > b.position ? 1 : 0)); +}, [rowsData]); +``` + +This flattens all fetched pages and then re-sorts them by `position` — the fractional-index that the server uses only as a tie-breaker. That completely discards the server's sort-key ordering and re-orders the list by an unrelated key. Each additional page fetched adds more "small position" rows near the top, progressively clobbering whatever made sense before. + +### Why the sort-by-position exists in the first place + +When NO sort is active, the server's fast path returns rows ordered by `position` (see `base-row.repo.ts:99-120`). Optimistic row creates (from `useCreateRowMutation.onSuccess`) and ws-pushed rows from other clients append to the last page in cache. In the no-sort case, the client-side position sort puts those appended rows into the correct visual slot without waiting for a refetch — a real benefit. + +So we can't remove the sort unconditionally. We just need to skip it when the server already sorted the rows for us. + +--- + +## File Structure + +**Modified:** `apps/client/src/features/base/components/base-table.tsx` — one `useMemo`. + +No new files, no new deps, no server changes. + +--- + +## Task 1: Conditionally skip the client-side position sort + +**File:** `apps/client/src/features/base/components/base-table.tsx` + +- [ ] **Step 1: Replace the rows memo** + +Find the existing memo at roughly lines 66-69: + +```ts +const rows = useMemo(() => { + const flat = flattenRows(rowsData); + return flat.sort((a, b) => (a.position < b.position ? -1 : a.position > b.position ? 1 : 0)); +}, [rowsData]); +``` + +Replace with: + +```ts +const rows = useMemo(() => { + const flat = flattenRows(rowsData); + // When a sort is active, the server returns rows in the requested + // sort order via keyset pagination. Re-sorting by `position` on the + // client would override that with fractional-index order — visibly + // breaking the sort as more pages load. Only apply the position + // sort when no view sort is active (where it keeps + // optimistically-created and ws-pushed rows in place without a + // refetch). + if (activeSorts && activeSorts.length > 0) { + return flat; + } + return flat.sort((a, b) => + a.position < b.position ? -1 : a.position > b.position ? 1 : 0, + ); +}, [rowsData, activeSorts]); +``` + +`activeSorts` is already in scope above this memo (line 46: `const activeSorts = activeView?.config?.sorts;`). Adding it to the dep array is safe. + +- [ ] **Step 2: Build** + +```bash +pnpm nx run client:build +``` + +Expected: success. + +- [ ] **Step 3: Commit** + +```bash +git add apps/client/src/features/base/components/base-table.tsx +git commit -m "fix(base): don't override server sort with client-side position sort" +``` + +--- + +## Task 2: USER smoke test + +> ⚠️ **Do not run `pnpm dev` as an agent.** Hand off to the user. + +Ask the user to verify on the 1K or 10K seed base: + +- [ ] **Sort by Title ascending, scroll to bottom, scroll back to top.** + - Top of the list should still be the lowest-title rows (e.g. `Alpha ...`). + - Bottom of the list should be the highest-title rows (e.g. `Zulu ...`). + - No re-ordering after scrolling. + +- [ ] **Sort by Estimate descending.** + - Highest numeric estimates at the top. Same stability check after scrolling. + +- [ ] **Remove all sorts.** + - Rows appear in insert/position order (the default). Same as before the fix — this path shouldn't regress. + +- [ ] **Add a new row with no sort active.** + - New row appears at its natural position without waiting for a refetch (the position-sort path still protects this). + +- [ ] **Add a new row WITH a sort active.** + - Row appears at the end of the current list (known limitation — see Out of scope). This is acceptable for now. + +- [ ] **Two-sort case.** Sort by Status ascending, then by Title ascending. + - Rows group by Status (all "Not Started" first, then "In Progress", etc.); within each group, sorted by Title. Scrolling doesn't break it. + +- [ ] **Regression: Virtualization still works.** + - Scroll through the 10K base. Smooth, no crashes. + +If any step misbehaves, report back with which one. + +--- + +## Out of scope + +- **New rows landing in the right slot when a sort is active.** Fixing that cleanly would require a binary insertion into the cached pages based on the sort keys, which depends on per-cell comparators (select/multi-select resolve via choice order, person via display name, etc.). Separate work. For now, users with a sort active see newly-created rows at the bottom until the next refetch. +- **ws-pushed rows with a sort active.** Same limitation — they append to the last page. +- **Removing the client-side `position` sort entirely.** Keeping it in the no-sort path preserves the good behavior for optimistic creates. diff --git a/docs/superpowers/plans/2026-04-18-hide-popover-dismiss.md b/docs/superpowers/plans/2026-04-18-hide-popover-dismiss.md new file mode 100644 index 00000000..2a98cc3b --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-hide-popover-dismiss.md @@ -0,0 +1,128 @@ +# Hide-Fields Popover Dismiss Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the hide-fields popover in the base toolbar dismiss on ESC and on outside click, matching the sort and filter popovers. + +**Architecture:** One-line fix — the hide popover is missing `trapFocus` that the other two toolbar popovers already have. Mantine `Popover` only honours `closeOnEscape` when focus is trapped inside the dropdown, so without `trapFocus` the ESC key is never captured. We also make `closeOnClickOutside` and `closeOnEscape` explicit so the intent is clear and immune to future default changes. + +**Tech Stack:** React, Mantine v8 `Popover`. + +--- + +## Background + +[apps/client/src/features/base/components/views/view-sort-config.tsx:86-93](apps/client/src/features/base/components/views/view-sort-config.tsx:86): + +```tsx + +``` + +[apps/client/src/features/base/components/views/view-filter-config.tsx:252-259](apps/client/src/features/base/components/views/view-filter-config.tsx:252) — same, has `trapFocus`. + +[apps/client/src/features/base/components/views/view-field-visibility.tsx:65-72](apps/client/src/features/base/components/views/view-field-visibility.tsx:65): + +```tsx + +``` + +Mantine v8 defaults: `closeOnClickOutside=true`, `closeOnEscape=true`, `trapFocus=false`. ESC handling is wired to the trapped focus context; without `trapFocus` the ESC never fires `onClose`. + +--- + +## File Structure + +**Modified files:** +- `apps/client/src/features/base/components/views/view-field-visibility.tsx` — one `` props edit. + +No new files, no new deps, no tests (pure UI behaviour wiring; identical change pattern to the other two popovers already in production). + +--- + +## Task 1: Enable focus trap + explicit dismiss flags + +**Files:** +- Modify: `apps/client/src/features/base/components/views/view-field-visibility.tsx:65-72` + +- [ ] **Step 1: Edit the Popover props** + +Before: +```tsx + +``` + +After: +```tsx + +``` + +- [ ] **Step 2: Build the client** + +```bash +pnpm nx run client:build +``` + +Expected: success. + +- [ ] **Step 3: Commit** + +```bash +git add apps/client/src/features/base/components/views/view-field-visibility.tsx +git commit -m "fix(base): dismiss hide-fields popover on escape and outside click" +``` + +--- + +## Task 2: USER smoke test + +> ⚠️ **Do not run `pnpm dev` as an agent.** Hand off to the user. + +Ask the user, with the dev server already running, to: + +- [ ] Open a base → click the "Hide fields" (eye icon) button. Popover opens. +- [ ] Press ESC. Popover closes. ✓ +- [ ] Open it again → click anywhere outside the popover (e.g. on the grid). Popover closes. ✓ +- [ ] Open it → click a Switch inside. Column toggles. Popover stays open. ✓ +- [ ] Open it → tab through with keyboard. Focus stays inside. ESC closes. ✓ +- [ ] Regression: the sort and filter popovers still dismiss correctly on ESC + outside click. + +If any step fails, report back; otherwise the fix is complete. + +--- + +## Out of scope + +- No refactor of the three popovers into a shared wrapper — `trapFocus` alone is the only behaviour diff, and the existing components are small and focused. Not worth abstracting three instances. +- No test added — Mantine `Popover` dismiss behaviour isn't something we own; testing it here would be testing the library. diff --git a/docs/superpowers/plans/2026-04-18-hide-property-persistence-bug.md b/docs/superpowers/plans/2026-04-18-hide-property-persistence-bug.md new file mode 100644 index 00000000..aa6a1fb0 --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-hide-property-persistence-bug.md @@ -0,0 +1,404 @@ +# Hide-Property Persistence Bug Fix Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop hidden column toggles from silently reappearing when another view-config mutation or a websocket-driven refetch lands between the toggle and the 300 ms debounced persist. + +**Architecture:** Two narrow fixes in the client — nothing server-side. (1) The blind `setColumnVisibility(derivedColumnVisibility)` sync effect in [`use-base-table.ts`](apps/client/src/features/base/hooks/use-base-table.ts) is scoped to run only when the user switches views, so refetches of the same view can't overwrite pending local toggles. (2) The sort/filter mutations in [`base-toolbar.tsx`](apps/client/src/features/base/components/base-toolbar.tsx) merge in the live table state (visibility / order / widths) instead of spreading stale `activeView.config` — so saving a sort no longer clobbers a not-yet-persisted hide. + +**Tech Stack:** React 18, TanStack Table v8, TanStack Query v5, Mantine, TypeScript. + +--- + +## Background — why this happens + +`useBaseTable` keeps `columnVisibility` as local React state, seeded from the persisted view config via a `useMemo`-derived value (`derivedColumnVisibility`). Two effects currently do an unconditional re-sync: + +```ts +// apps/client/src/features/base/hooks/use-base-table.ts:223-229 +useEffect(() => { + setColumnOrder(derivedColumnOrder); +}, [derivedColumnOrder]); + +useEffect(() => { + setColumnVisibility(derivedColumnVisibility); +}, [derivedColumnVisibility]); +``` + +Any mutation that calls `queryClient.setQueryData(["bases", baseId], ...)` — or `queryClient.invalidateQueries` at [`use-base-socket.ts:254`](apps/client/src/features/base/hooks/use-base-socket.ts:254) on ws `base:property:*` / `base:view:*` events — minted a new `activeView.config` reference. That recomputes `derivedColumnVisibility`, and the effect slams the local state back to whatever the server currently has. + +Persistence is debounced 300 ms by `persistViewConfig`. During that window, any of these triggers a stomp: + +1. **A concurrent sort / filter mutation from the same user.** `handleSortsChange` / `handleFiltersChange` in [`base-toolbar.tsx:93-120`](apps/client/src/features/base/components/base-toolbar.tsx:93) spread `activeView.config` (stale — still has old `hiddenPropertyIds`) and `mutate` immediately, without going through the debounced path. The `onMutate` optimistic write, the `onSuccess` server write, and the ws echo's `invalidateQueries` each re-trigger the effect. +2. **Another client sending any `base:property:*` / `base:view:*` ws event.** +3. **The optimistic / success writes from our OWN `persistViewConfig` mutation when the server-side state hasn't persisted yet** (e.g., response races). + +Symptom: user toggles a column hidden → column disappears → column reappears within ~instant to a few hundred ms → reload confirms the toggle was never saved. + +**Deterministic repro (no second client needed):** +1. Open a base with ≥ 3 non-primary columns and no sorts. +2. In the toolbar, open "Hide fields" and toggle column X off. Column vanishes. +3. Immediately (well under 300 ms) open the Sort popover and add a sort. +4. Column X reappears. +5. Reload: X is visible; sort is saved; hide was lost. + +`hiddenPropertyIds` wins semantics check: [`use-base-table.ts:117-143`](apps/client/src/features/base/hooks/use-base-table.ts:117) — correct (hidden list is checked first, then legacy `visiblePropertyIds`, then default all-visible). That isn't the bug. + +--- + +## File Structure + +**Modified files:** +- `apps/client/src/features/base/hooks/use-base-table.ts` — gate the re-sync effect behind a view-id ref; export a new `buildViewConfigFromTable` helper used by both the debounced persist and the toolbar's direct mutations. +- `apps/client/src/features/base/components/base-toolbar.tsx` — `handleSortsChange` / `handleFiltersChange` build the full config from live table state (not `activeView.config`) before `mutate`. + +No new files. No server-side changes. No new deps. + +--- + +## Task 1: Verify the repro against the current build + +- [ ] **Step 1: Build the client** to make sure the branch is clean before changing anything. + +```bash +pnpm nx run client:build +``` + +Expected: build succeeds. + +- [ ] **Step 2: Ask the user to run the deterministic repro above.** + +(Per `CLAUDE.md` the agent must not run `pnpm dev`.) User should confirm: +- Column X disappears, then reappears within ~300 ms of adding the sort. +- Reload shows X visible, hide lost. + +If the repro does NOT reproduce for the user, stop the plan here and ask for actual repro steps — the fixes below target this specific chain. + +--- + +## Task 2: Extract `buildViewConfigFromTable` helper + +**Files:** +- Modify: `apps/client/src/features/base/hooks/use-base-table.ts` + +- [ ] **Step 1: Add the helper near the other build functions** (above `useBaseTable`, after `buildColumnPinning`) + +```ts +// Serializes the live react-table state into a persisted ViewConfig. +// Sort/filter toolbar mutations and the debounced `persistViewConfig` +// both go through this so a direct mutation (e.g. adding a sort) can't +// clobber a pending hide/reorder/resize by reading stale `activeView.config`. +export function buildViewConfigFromTable( + table: Table, + base: ViewConfig | undefined, + overrides: Partial = {}, +): ViewConfig { + const state = table.getState(); + + const sorts = state.sorting.map((s) => ({ + propertyId: s.id, + direction: (s.desc ? "desc" : "asc") as "asc" | "desc", + })); + + const propertyWidths: Record = {}; + Object.entries(state.columnSizing).forEach(([id, width]) => { + if (id !== "__row_number") propertyWidths[id] = width; + }); + + const propertyOrder = state.columnOrder.filter((id) => id !== "__row_number"); + + const hiddenPropertyIds = Object.entries(state.columnVisibility) + .filter(([id, visible]) => id !== "__row_number" && !visible) + .map(([id]) => id); + + return { + ...base, + sorts, + propertyWidths, + propertyOrder, + hiddenPropertyIds, + visiblePropertyIds: undefined, + ...overrides, + }; +} +``` + +- [ ] **Step 2: Replace the inline serialization inside `persistViewConfig`** (lines roughly 267-297 of the current file) with a call to the helper. + +Before: +```ts +persistTimerRef.current = setTimeout(() => { + const state = table.getState(); + const sorts = state.sorting.map((s) => ({ ... })); + // ...lots of inline serialization... + const config: ViewConfig = { + ...activeView.config, + sorts, + propertyWidths, + propertyOrder, + hiddenPropertyIds, + visiblePropertyIds: undefined, + }; + updateViewMutation.mutate({ viewId: activeView.id, baseId: base.id, config }); +}, 300); +``` + +After: +```ts +persistTimerRef.current = setTimeout(() => { + const config = buildViewConfigFromTable(table, activeView.config); + updateViewMutation.mutate({ viewId: activeView.id, baseId: base.id, config }); +}, 300); +``` + +- [ ] **Step 3: Build** + +```bash +pnpm nx run client:build +``` + +Expected: success. + +- [ ] **Step 4: Commit** + +```bash +git add apps/client/src/features/base/hooks/use-base-table.ts +git commit -m "refactor(base): extract buildViewConfigFromTable helper" +``` + +--- + +## Task 3: Route sort/filter mutations through the helper + +**Files:** +- Modify: `apps/client/src/features/base/components/base-toolbar.tsx` + +`handleSortsChange` and `handleFiltersChange` currently read `activeView.config` directly, which reflects the server-persisted state, not the user's pending local changes. They must build the new config from `table.getState()` + the new sort or filter. + +- [ ] **Step 1: Import the helper and accept the full view config for merging filters** + +Add at the top of the file: +```ts +import { buildViewConfigFromTable } from "@/features/base/hooks/use-base-table"; +``` + +- [ ] **Step 2: Rewrite `handleSortsChange`** + +Before: +```ts +const handleSortsChange = useCallback( + (newSorts: ViewSortConfig[]) => { + if (!activeView) return; + updateViewMutation.mutate({ + viewId: activeView.id, + baseId: base.id, + config: { ...activeView.config, sorts: newSorts }, + }); + }, + [activeView, base.id, updateViewMutation], +); +``` + +After: +```ts +const handleSortsChange = useCallback( + (newSorts: ViewSortConfig[]) => { + if (!activeView) return; + const config = buildViewConfigFromTable(table, activeView.config, { + sorts: newSorts, + }); + updateViewMutation.mutate({ + viewId: activeView.id, + baseId: base.id, + config, + }); + }, + [activeView, base.id, table, updateViewMutation], +); +``` + +- [ ] **Step 3: Rewrite `handleFiltersChange` the same way** + +Before: +```ts +const handleFiltersChange = useCallback( + (newConditions: FilterCondition[]) => { + if (!activeView) return; + const filter: FilterGroup | undefined = + newConditions.length > 0 + ? { op: "and", children: newConditions } + : undefined; + const { filter: _drop, ...rest } = activeView.config ?? {}; + updateViewMutation.mutate({ + viewId: activeView.id, + baseId: base.id, + config: filter ? { ...rest, filter } : rest, + }); + }, + [activeView, base.id, updateViewMutation], +); +``` + +After: +```ts +const handleFiltersChange = useCallback( + (newConditions: FilterCondition[]) => { + if (!activeView) return; + const filter: FilterGroup | undefined = + newConditions.length > 0 + ? { op: "and", children: newConditions } + : undefined; + // `filter: undefined` in overrides removes the filter key; the helper's + // spread-then-overrides order means `undefined` wins over any base filter. + const config = buildViewConfigFromTable(table, activeView.config, { + filter, + }); + updateViewMutation.mutate({ + viewId: activeView.id, + baseId: base.id, + config, + }); + }, + [activeView, base.id, table, updateViewMutation], +); +``` + +- [ ] **Step 4: Build** + +```bash +pnpm nx run client:build +``` + +Expected: success. + +- [ ] **Step 5: Commit** + +```bash +git add apps/client/src/features/base/components/base-toolbar.tsx +git commit -m "fix(base): merge live table state into sort and filter mutations" +``` + +--- + +## Task 4: Gate the sync effect behind view-id change + +**Files:** +- Modify: `apps/client/src/features/base/hooks/use-base-table.ts` + +Within the same view, local state is the source of truth and the debounced persist flushes it. The sync effect must only run when the user switches to a different view (where the server's config is authoritative). + +- [ ] **Step 1: Replace both sync effects** + +Before (around lines 220-229): +```ts +const [columnOrder, setColumnOrder] = useState(derivedColumnOrder); +const [columnVisibility, setColumnVisibility] = useState(derivedColumnVisibility); + +useEffect(() => { + setColumnOrder(derivedColumnOrder); +}, [derivedColumnOrder]); + +useEffect(() => { + setColumnVisibility(derivedColumnVisibility); +}, [derivedColumnVisibility]); +``` + +After: +```ts +const [columnOrder, setColumnOrder] = useState(derivedColumnOrder); +const [columnVisibility, setColumnVisibility] = useState(derivedColumnVisibility); + +// Re-seed from server only when the user switches views. Within the same +// view, local state is the source of truth — the debounced persist flushes +// it. Without this guard, any ws-driven `invalidateQueries(["bases", baseId])` +// or concurrent view mutation lands a new `derivedColumnVisibility` +// reference and the effect would overwrite a pending hide/reorder toggle +// before `persistViewConfig` has a chance to flush it. +const lastSyncedViewIdRef = useRef(activeView?.id); +useEffect(() => { + const currentViewId = activeView?.id; + if (currentViewId !== lastSyncedViewIdRef.current) { + lastSyncedViewIdRef.current = currentViewId; + setColumnOrder(derivedColumnOrder); + setColumnVisibility(derivedColumnVisibility); + } +}, [activeView?.id, derivedColumnOrder, derivedColumnVisibility]); +``` + +Note: `derivedColumnOrder` and `derivedColumnVisibility` remain in the dependency array so the effect reads the current derived values when a view switch happens. The ref-guarded branch only fires on id change. + +- [ ] **Step 2: Build** + +```bash +pnpm nx run client:build +``` + +Expected: success. + +- [ ] **Step 3: Commit** + +```bash +git add apps/client/src/features/base/hooks/use-base-table.ts +git commit -m "fix(base): only re-seed column state when view identity changes" +``` + +--- + +## Task 5: USER manual verification — scripted steps + +> ⚠️ **Do not run `pnpm dev` as an agent.** Hand off to the user with the cases below. + +Ask the user to run through these. Each should now behave correctly: + +- [ ] **Repro from Task 1 — hide then add sort** + 1. Open a base with ≥ 3 non-primary columns. + 2. Hide column X via the popover. + 3. Within 300 ms, add a sort via the Sort popover. + 4. Expected: column X stays hidden. Sort indicator shows. Reload: both persisted. + +- [ ] **Hide then change filter** + 1. Hide column Y. + 2. Immediately add a filter condition. + 3. Expected: column Y stays hidden, filter applied. Reload: both persisted. + +- [ ] **Hide all / show all** + 1. Click "Hide all" in the popover. All non-primary columns disappear. + 2. Reload. Expected: same state. + 3. Click "Show all". Reload. Expected: all columns visible. + +- [ ] **View switch** + 1. Hide column Z on view A. + 2. Switch to view B (no hide). Expected: Z visible in view B. + 3. Switch back to view A. Expected: Z hidden. + +- [ ] **WebSocket reconcile doesn't stomp** + 1. Hide column W. + 2. From another browser / incognito session on the same base, add a new property. + 3. In the first window, the new property appears visible, W stays hidden. + +- [ ] **Primary property can't be hidden** (regression check) + 1. Open the popover — the primary column's switch is disabled (already enforced by `enableHiding: !property.isPrimary` at `use-base-table.ts:87`). Confirm. + +--- + +## Task 6: Final commit check + handoff + +- [ ] **Step 1: Confirm branch state** + +```bash +git status +git log --oneline main..HEAD +``` + +Expected: clean tree, 3 new commits atop the CSV-export branch (refactor, fix sort/filter merge, fix sync effect). + +- [ ] **Step 2: Trigger `superpowers:finishing-a-development-branch`** to choose merge/PR. + +--- + +## Out of scope (explicitly not fixing here) + +- **`hiddenFieldCount` badge dep** in `base-toolbar.tsx:88-91` — works by accident today (re-renders produce fresh `getState()` references). Low-impact cosmetic risk; leave alone unless it manifests. +- **Legacy `visiblePropertyIds` migration.** Views created before `hiddenPropertyIds` existed may show new properties as hidden by default in `buildColumnVisibility`. No reports of this; migration would be a separate plan. +- **Batching `handleHideAll`/`handleShowAll`** into a single `table.setColumnVisibility(map)` call instead of iterating `toggleVisibility`. React 18 batches these anyway; not a bug. +- **Server-side zod schema.** `viewConfigSchema` already accepts `hiddenPropertyIds: []` correctly; no change needed. diff --git a/docs/superpowers/plans/2026-04-18-hide-property-still-broken.md b/docs/superpowers/plans/2026-04-18-hide-property-still-broken.md new file mode 100644 index 00000000..81413217 --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-hide-property-still-broken.md @@ -0,0 +1,224 @@ +# Hide Property Still Broken — Diagnose & Fix Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Identify and fix the reason that toggling a property in the hide-fields popover no longer ends up in the POST payload sent to `/bases/views/update`. + +**Architecture:** Instrument the toggle → state → debounced persist pipeline, have the user reproduce, read the logs, fix the exact bug. Do NOT blindly apply a "defensive" fix before the bug is pinpointed — we've burned several "fixes" on this already and need ground truth first. + +**Tech Stack:** React 18, TanStack Table v8 (controlled `state.columnVisibility` + `onColumnVisibilityChange`), TanStack Query v5. + +--- + +## What we know so far + +1. **Server is fine.** Direct API call to `POST /api/bases/views/update` with `{config: {hiddenPropertyIds: [...]}}` persists exactly what's sent (verified against the user's base `019c69a5-1d84-7985-a7f6-8ee2871d8669`). +2. **Client outgoing payload is wrong.** User observed: existing `hiddenPropertyIds = [A, B]`. They toggled a new column C via the hide popover. The outgoing POST payload still contained `hiddenPropertyIds: [A, B]` — C never made it in. +3. **`buildViewConfigFromTable`** (at [`use-base-table.ts:179-211`](apps/client/src/features/base/hooks/use-base-table.ts:179)) reads `table.getState().columnVisibility` and derives `hiddenPropertyIds` by filtering entries where `visible === false`. +4. **Only three code paths modify `columnVisibility` state** (confirmed by grep): + - `use-base-table.ts:275` — view-switch full re-seed (only fires when `activeView?.id` changes). + - `use-base-table.ts:297` — reconcile branch (function updater, preserves `prev` for existing ids). + - `use-base-table.ts:336` — `onColumnVisibilityChange: setColumnVisibility` passed to react-table. +5. **react-table v8 `useReactTable`** ([verified from node_modules source](node_modules/.pnpm/@tanstack+react-table@8.21.3_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@tanstack/react-table/build/lib/index.mjs:47-70)) returns a STABLE `table` reference held in `useState(() => ({current: createTable(...)}))`. Every render it calls `setOptions` to merge fresh options and state. So the stale-`table`-closure hypothesis from an earlier investigation is LIKELY wrong — `table.getState()` at setTimeout time should return current state. +6. **`col.toggleVisibility(value)`** at [`@tanstack/table-core/.../ColumnVisibility.js:30`](node_modules/.pnpm/@tanstack+table-core@8.21.3/node_modules/@tanstack/table-core/build/lib/features/ColumnVisibility.js) uses a function updater: `table.setColumnVisibility(old => ({...old, [id]: value}))`. Which forwards to our `setColumnVisibility` — React queues the function updater; the next render commits new state. + +Given all of that, the toggle SHOULD reach `state.columnVisibility` and SHOULD land in the debounced payload. Something is interfering that we can't see by reading. + +## Hypotheses to rule in or out (the diagnostic is designed to distinguish them) + +- **H1: The handler-to-setState path is broken.** `handleToggle` fires but `setColumnVisibility` never commits the toggle (e.g., the setter is somehow stale, the function updater sees a stale `prev`, or react-table's intermediate logic drops the call). +- **H2: Something immediately stomps the toggle.** The reconcile effect fires between the toggle and the debounce, with a `prev` that doesn't yet reflect the toggle, and writes an updated map that "preserves" a stale value for the toggled id. +- **H3: Debounce timer fires with a fresh closure that reads a different `table` instance.** Contradicts what we saw in the react-table source, but worth falsifying. +- **H4: The popover's `col` reference comes from a STALE `columns` memo.** If `table.getAllLeafColumns()` was captured when an older version of the table existed, `col.toggleVisibility` would call a stale `table.setColumnVisibility`. +- **H5: The state IS updated but `buildViewConfigFromTable` reads a different state shape.** E.g., internal react-table state defeats our controlled state for `columnVisibility`. + +--- + +## File Structure + +**Modified (instrumentation task — revert before shipping):** +- `apps/client/src/features/base/hooks/use-base-table.ts` — add `console.log` calls at four checkpoints. +- `apps/client/src/features/base/components/views/view-field-visibility.tsx` — log the `handleToggle` call site. + +**Modified (fix task — depends on findings):** +- Whatever the diagnostic reveals. + +--- + +## Task 1: Instrument the pipeline + +**Files:** +- Modify: `apps/client/src/features/base/hooks/use-base-table.ts` +- Modify: `apps/client/src/features/base/components/views/view-field-visibility.tsx` + +- [ ] **Step 1: Add a logging helper at the top of `use-base-table.ts`** + +Right after the existing imports, add: +```ts +const DEBUG_HIDE = true; +function hideLog(label: string, data: unknown) { + if (DEBUG_HIDE) console.log(`[hide-debug] ${label}`, data); +} +``` + +- [ ] **Step 2: Log inside the view-switch re-seed branch** + +In the effect at ~line 268, inside the `if (currentViewId !== lastSyncedViewIdRef.current) { ... }` branch, add: +```ts +hideLog("VIEW_SWITCH_RESEED", { + viewId: currentViewId, + newOrder: derivedColumnOrder, + newVisibility: derivedColumnVisibility, +}); +``` + +- [ ] **Step 3: Log inside the reconcile branch's setColumnVisibility updater** + +In the reconcile `setColumnVisibility((prev) => ...)` callback (around line 297), at the VERY START, log: +```ts +hideLog("RECONCILE_ENTER", { prev, derivedColumnVisibility }); +``` + +And right before `return changed ? next : prev;`, log: +```ts +hideLog("RECONCILE_EXIT", { changed, next, returning: changed ? next : prev }); +``` + +- [ ] **Step 4: Wrap `setColumnVisibility` to log every call** + +In `useReactTable`, replace: +```ts +onColumnVisibilityChange: setColumnVisibility, +``` + +with an instrumented passthrough: +```ts +onColumnVisibilityChange: (updater) => { + hideLog("RT_onColumnVisibilityChange", { + updaterType: typeof updater, + applied: + typeof updater === "function" + ? updater(columnVisibility) + : updater, + }); + setColumnVisibility(updater as Parameters[0]); +}, +``` + +(Apply the same pattern to `onColumnOrderChange` if you want symmetry — optional.) + +- [ ] **Step 5: Log inside the debounced persist** + +Inside the `setTimeout(() => { ... }, 300)` callback in `persistViewConfig`, BEFORE the `buildViewConfigFromTable` call, log: +```ts +const liveState = table.getState(); +hideLog("PERSIST_TICK", { + viewId: activeView.id, + stateColumnVisibility: liveState.columnVisibility, + stateColumnOrder: liveState.columnOrder, +}); +``` + +AFTER `buildViewConfigFromTable`, log: +```ts +hideLog("PERSIST_OUTGOING", { config }); +``` + +- [ ] **Step 6: Log inside `handleToggle` in the popover** + +In `apps/client/src/features/base/components/views/view-field-visibility.tsx`, modify `handleToggle`: +```ts +const handleToggle = useCallback( + (columnId: string, visible: boolean) => { + const col = table.getColumn(columnId); + console.log("[hide-debug] HANDLE_TOGGLE", { + columnId, + visibleRequested: visible, + canHide: col?.getCanHide(), + currentlyVisible: col?.getIsVisible(), + }); + if (!col) return; + col.toggleVisibility(visible); + onPersist(); + }, + [table, onPersist], +); +``` + +- [ ] **Step 7: Build (do not commit — this is throwaway instrumentation)** + +```bash +pnpm nx run client:build +``` + +Expected: success. + +--- + +## Task 2: USER reproduces and shares logs + +> ⚠️ **Do not run `pnpm dev` as an agent.** User runs dev; user hard-reloads; user interacts and copies the console output. + +Hand off to the user with this script: + +1. Open DevTools Console, clear it. Keep it open. +2. Open the base. Open the "Hide fields" popover. +3. Toggle ONE property that is currently visible → hidden. Do not click anything else. +4. Wait ~1 second (debounce fires). +5. Copy EVERY `[hide-debug] ...` line from the console, in order, and paste them back here. Also paste the resulting Network POST `/api/bases/views/update` request payload (Network tab → the one `update` request that fires 300 ms after the click → Payload → Request Payload). + +The interesting sequence, if everything is working, is: +``` +HANDLE_TOGGLE { columnId: "X", visibleRequested: false, canHide: true, currentlyVisible: true } +RT_onColumnVisibilityChange { updaterType: "function", applied: { ..., X: false } } +PERSIST_TICK { stateColumnVisibility: { ..., X: false } } +PERSIST_OUTGOING { config: { hiddenPropertyIds: [..., "X"] } } +``` + +If the bug is present, one of those lines will be missing or wrong. The exact position of the deviation pinpoints which hypothesis (H1–H5) is correct. + +--- + +## Task 3: Fix based on findings + +Do NOT pre-write the fix. Tasks 3a-3c below are the dispatch table — exactly ONE applies. + +### Task 3a: If `RT_onColumnVisibilityChange` never logs, or `applied` doesn't include the toggled id → H1 + +react-table isn't calling our setter, OR the updater resolves to the wrong value. This points to: +- a stale `col` object (columns memo invalidation issue) +- or a react-table options propagation bug + +Investigate `col.toggleVisibility` step-by-step (add logs inside `toggleVisibility` via a wrapped column accessor), ensure `columns` useMemo dep list includes everything that affects `getCanHide`/`getIsVisible`. + +### Task 3b: If `RT_onColumnVisibilityChange` logs `applied: {X: false}` but `PERSIST_TICK` shows `stateColumnVisibility` without `X: false` → H2 or H3 + +The setter was called correctly but state didn't commit. Check if `RECONCILE_ENTER` fired between them and what `prev` it saw. + +- If `RECONCILE_ENTER.prev` has `X: false` and `RECONCILE_EXIT.returning` does NOT → bug in the reconcile logic. +- If `RECONCILE_ENTER.prev` does NOT have `X: false` → React batching issue; the toggle's setState ran AFTER the effect's setState. Fix by using a ref to latest `derivedColumnVisibility` so the reconcile effect can safely be a no-op except on view-id change (same-view drift will be covered by `columns` prop going through react-table's internal columnVisibility seeding). + +### Task 3c: If `PERSIST_TICK.stateColumnVisibility` has `X: false` but `PERSIST_OUTGOING.config.hiddenPropertyIds` doesn't include `X` → bug in `buildViewConfigFromTable` + +This would be surprising given the code at [`use-base-table.ts:198-200`](apps/client/src/features/base/hooks/use-base-table.ts:198), but check type coercion and filter predicate. + +--- + +## Task 4: Remove instrumentation, commit fix, hand back to user + +- [ ] **Step 1:** Remove all `[hide-debug]` logs and the `DEBUG_HIDE` / `hideLog` helper. +- [ ] **Step 2:** Build + self-verify by thinking through the fix with the log evidence in hand. +- [ ] **Step 3:** +```bash +pnpm nx run client:build +git add +git commit -m "fix(base): " +``` +- [ ] **Step 4:** User smoke test: hide a column, verify payload contains the id, verify the column stays hidden after refresh. + +--- + +## Anti-goals + +- **No "defensive" fixes.** We've cycled through "wrap in useRef" / "gate the effect" / "merge table state" — each touched a real issue but none hit this particular bug. A plausible-sounding fix is worse than silence: it burns trust when it doesn't work. +- **No code edits without the log evidence.** Task 3 only runs after Task 2 returns concrete data. diff --git a/docs/superpowers/plans/2026-04-18-new-property-not-in-view.md b/docs/superpowers/plans/2026-04-18-new-property-not-in-view.md new file mode 100644 index 00000000..531c5ae8 --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-new-property-not-in-view.md @@ -0,0 +1,197 @@ +# New Property Not In View Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** When a user creates a new property, it lands in the grid's local column state immediately (appended to the end, visible by default) so the grid can size and scroll to reveal it. + +**Architecture:** Single-file fix in [`apps/client/src/features/base/hooks/use-base-table.ts`](apps/client/src/features/base/hooks/use-base-table.ts). Extend the existing gated re-seed effect so that on property add / remove (within the same view) it reconciles local `columnOrder` and `columnVisibility` instead of ignoring the change. Existing per-column user toggles (hide, reorder) are preserved; new columns are appended; deleted columns are dropped. + +**Tech Stack:** React 18, TanStack Table v8, TanStack Query v5. + +--- + +## Background + +Earlier in this branch (commit `c6f993b6`) the sync effect that copied `derivedColumnOrder` / `derivedColumnVisibility` into local state was gated behind a view-id ref to stop ws-driven base refetches from stomping the user's pending hide-column toggles. Current code: + +```ts +// apps/client/src/features/base/hooks/use-base-table.ts:267-281 +const lastSyncedViewIdRef = useRef(activeView?.id); +useEffect(() => { + const currentViewId = activeView?.id; + if (currentViewId !== lastSyncedViewIdRef.current) { + lastSyncedViewIdRef.current = currentViewId; + setColumnOrder(derivedColumnOrder); + setColumnVisibility(derivedColumnVisibility); + } +}, [activeView?.id, derivedColumnOrder, derivedColumnVisibility]); +``` + +**What goes wrong with a new property in the same view:** + +1. User opens `CreatePropertyPopover`, submits → `useCreatePropertyMutation.onSuccess` appends the new property to `["bases", baseId].properties`. +2. `base.properties` has a new reference → `properties` useMemo re-runs → new array reference. +3. `derivedColumnOrder` recomputes — includes the new property id. +4. The gated effect sees `currentViewId === lastSyncedViewIdRef.current`, so it **does nothing**. Local `columnOrder` state still lists the OLD column ids. +5. `columns` prop to `useReactTable` is rebuilt (it memos on `[properties]`), so react-table does know the new column def exists. +6. But state passed via `state={{columnOrder}}` still references only the old columns → `table.getState().columnOrder` → stale → `gridTemplateColumns` (which depends on `table.getVisibleLeafColumns()` + `table.getState().columnOrder` in [`grid-container.tsx:149`](apps/client/src/features/base/components/grid/grid-container.tsx:149)) doesn't get a track for the new column. +7. Result: the new column renders in the DOM (react-table still yields it as visible), but the grid wrapper's `scrollWidth` doesn't extend to contain it, so `handlePropertyCreated`'s [`scrollRef.current.scrollTo({left: scrollWidth})`](apps/client/src/features/base/components/grid/grid-container.tsx:183-192) ends at the OLD scrollWidth — the new column stays clipped at the edge. + +The same mechanism also breaks property **deletion** within the same view (local state keeps a dead id) — not the filed symptom, but worth fixing in the same patch. + +The same mechanism does NOT break **rename**, because rename changes `property.name` but not property IDs; order + visibility maps key on id, so they stay correct. Rename is already working after the earlier memo-prop-threading fix. + +--- + +## File Structure + +**Modified files:** +- `apps/client/src/features/base/hooks/use-base-table.ts` — extend the sync effect. + +No new files, no new deps, nothing server-side. + +--- + +## Task 1: Reconcile local column state on property add / remove + +**Files:** +- Modify: `apps/client/src/features/base/hooks/use-base-table.ts:260-281` + +- [ ] **Step 1: Replace the effect** + +Before (current): +```ts +const lastSyncedViewIdRef = useRef(activeView?.id); +useEffect(() => { + const currentViewId = activeView?.id; + if (currentViewId !== lastSyncedViewIdRef.current) { + lastSyncedViewIdRef.current = currentViewId; + setColumnOrder(derivedColumnOrder); + setColumnVisibility(derivedColumnVisibility); + } +}, [activeView?.id, derivedColumnOrder, derivedColumnVisibility]); +``` + +After: +```ts +const lastSyncedViewIdRef = useRef(activeView?.id); +useEffect(() => { + const currentViewId = activeView?.id; + + // View switch → full re-seed from the server's stored config. + if (currentViewId !== lastSyncedViewIdRef.current) { + lastSyncedViewIdRef.current = currentViewId; + setColumnOrder(derivedColumnOrder); + setColumnVisibility(derivedColumnVisibility); + return; + } + + // Same view — preserve user toggles, but reconcile the id set: + // append properties that were just created, drop properties that + // were deleted. Without this, creating a new column leaves it + // invisible to `table.getState().columnOrder` / `gridTemplateColumns`, + // and the grid's scrollWidth never grows to include it. + const validIds = new Set(["__row_number"]); + for (const p of properties) validIds.add(p.id); + + setColumnOrder((prev) => { + const prevSet = new Set(prev); + const kept = prev.filter((id) => validIds.has(id)); + const appended = derivedColumnOrder.filter( + (id) => !prevSet.has(id) && validIds.has(id), + ); + if (appended.length === 0 && kept.length === prev.length) return prev; + return [...kept, ...appended]; + }); + + setColumnVisibility((prev) => { + let changed = false; + const next: VisibilityState = {}; + for (const [id, visible] of Object.entries(prev)) { + if (validIds.has(id)) { + next[id] = visible; + } else { + changed = true; + } + } + for (const id of derivedColumnOrder) { + if (!(id in next)) { + next[id] = derivedColumnVisibility[id] ?? true; + changed = true; + } + } + return changed ? next : prev; + }); +}, [ + activeView?.id, + derivedColumnOrder, + derivedColumnVisibility, + properties, +]); +``` + +Notes for the implementer: +- `VisibilityState` is already imported from `@tanstack/react-table` — no new import needed. +- `properties` is already declared as a memoized value at the top of this hook, so adding it to the dep list is safe. +- The two `setX((prev) => ...)` updaters both short-circuit (return `prev`) when nothing actually changed, which matters because `derivedColumnOrder` / `derivedColumnVisibility` have a new identity every time `properties` does — without the short-circuit the set would fire every render in the same view and blow away user toggles. +- `kept.length === prev.length` is a proxy for "no deletions" and is safe because `prev` can't contain duplicates (react-table enforces id uniqueness, and our own `derivedColumnOrder` is also unique). + +- [ ] **Step 2: Build** + +```bash +pnpm nx run client:build +``` + +Expected: success. + +- [ ] **Step 3: Commit** + +```bash +git add apps/client/src/features/base/hooks/use-base-table.ts +git commit -m "fix(base): include new properties in local column state so the grid can scroll to them" +``` + +--- + +## Task 2: USER smoke test + +> ⚠️ **Do not run `pnpm dev` as an agent.** Hand off to the user. + +After a hard reload: + +- [ ] **Create a new property — grid sizes for it and scroll reaches it.** + 1. Open a base with enough columns that horizontal scrolling is already active. + 2. Click the "+" / create-property button, pick a type, submit. + 3. The grid should scroll right automatically; the new column is fully visible; the horizontal scrollbar extends. + +- [ ] **New property is visible in the Hide fields popover.** + 1. Open the eye icon (Hide fields). + 2. The new property appears in the list, toggle ON. + +- [ ] **Existing toggles are preserved.** + 1. Hide column X. + 2. Create a new column Y. Column X stays hidden; Y appears at the end, visible. + +- [ ] **Delete a property.** + 1. From a property's menu, click Delete. + 2. Column disappears from the grid; grid scrollWidth contracts. No stale column left. + +- [ ] **View switch still works cleanly.** + 1. Switch to a different view; then switch back. + 2. Hidden / reordered state for that view loads correctly. + +- [ ] **Rename still works (regression check).** + 1. Rename a property; the header text updates without reload. + +- [ ] **Hide + concurrent sort mutation (regression for the original hide bug).** + 1. Hide a column, then add a sort within 300 ms. Column stays hidden; sort applies. + +If any step fails, report back with the specific case. + +--- + +## Out of scope + +- Scrolling behavior on row add (orthogonal, not broken). +- The two `rAF` delay in `handlePropertyCreated` — it already waits long enough once state reconciliation happens in the same render cycle. +- `columnSizing` reconciliation — a new column uses its defined size automatically via react-table's `initialState`, and a deleted column's entry in the sizing state is harmless. diff --git a/docs/superpowers/plans/2026-04-18-property-rename-not-reflecting.md b/docs/superpowers/plans/2026-04-18-property-rename-not-reflecting.md new file mode 100644 index 00000000..cd49fd9e --- /dev/null +++ b/docs/superpowers/plans/2026-04-18-property-rename-not-reflecting.md @@ -0,0 +1,375 @@ +# Property Rename Not Reflecting Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make property (column) rename reflect immediately in the grid header and the hide-fields popover, both for the editing user and other clients in the same base room — without a tab reload. + +**Architecture:** Three small frontend changes. The server path is already correct (rename persists, emits `base:property:updated` ws event, and `useBaseSocket` invalidates `["bases", baseId]`). The cache updates too. The bug is purely client-side memoization: `GridHeader`, `GridHeaderCell`, and `ViewFieldVisibility` memo on `[table]` / their prop object — and the `table` reference returned by `useReactTable` is STABLE across renders. So when properties change under the hood (new column defs, new `meta.property` objects, new header strings), memo'd consumers never re-render. Fix: thread `properties` / `property` down as explicit props so shallow-compare catches the change. + +**Tech Stack:** React 18, TanStack Table v8, TanStack Query v5. + +--- + +## Background — the trace that explains the bug + +1. User renames property "Email" → "Mail" from the header's property menu. +2. `updatePropertyMutation.mutate` fires. Server persists, returns `{property: {...name: "Mail"}}`. +3. `onSuccess` in [`base-property-query.ts:52-65`](apps/client/src/features/base/queries/base-property-query.ts:52) calls `queryClient.setQueryData(["bases", baseId], old => ({...old, properties: old.properties.map(p => p.id === result.property.id ? result.property : p)}))`. The renamed property is a new object reference; the rest of the array reuses old refs. +4. `useBaseQuery` returns a new `IBase` object with a new `properties` array. +5. In [`use-base-table.ts`](apps/client/src/features/base/hooks/use-base-table.ts), `properties = useMemo(() => base?.properties ?? [], [base?.properties])` picks up the new array. `columns = useMemo(() => buildColumns(properties), [properties])` rebuilds all column defs, including new `meta.property` objects and new `header: property.name` values. +6. `useReactTable({columns, ...})` receives the new columns array. Internally TanStack Table updates its column state. +7. The `table` instance returned by `useReactTable` is the SAME reference it was before — it's memoized for stability. +8. `` is wrapped in `React.memo`. Its props: `table` (stable), `columnOrder` (unchanged — rename doesn't reorder), `loadedRowIds` (unchanged). Memo shallow-compare says "no change" → **no re-render**. +9. Even if `GridHeader` did re-render, each `` is also `memo`'d. `header` is reused by TanStack Table across renders for the same column id, so same ref → memo skips → **no re-render**. +10. [`view-field-visibility.tsx:27-31`](apps/client/src/features/base/components/views/view-field-visibility.tsx:27): `const columns = useMemo(() => table.getAllLeafColumns().filter(...), [table])`. `table` is stable → memo never invalidates → shows stale names. + +The rename only becomes visible after a full mount (tab reload), which recomputes everything from scratch. + +## Files + +**Modified:** +- `apps/client/src/features/base/components/grid/grid-header.tsx` — accept `properties: IBaseProperty[]` prop; pass `property={...}` to each `GridHeaderCell`. Memo picks up the change. +- `apps/client/src/features/base/components/grid/grid-header-cell.tsx` — accept `property` as an explicit prop instead of deriving from `header.column.columnDef.meta?.property`. Use it for header rendering (and anywhere else in this file that currently reads it through the meta). +- `apps/client/src/features/base/components/grid/grid-container.tsx` — accept `properties` prop; pass to ``. +- `apps/client/src/features/base/components/base-table.tsx` — pass `base?.properties` to ``. +- `apps/client/src/features/base/components/base-toolbar.tsx` — pass `properties={base.properties}` to ``. +- `apps/client/src/features/base/components/views/view-field-visibility.tsx` — accept `properties` prop; include it in the `useMemo([table, properties])` dep list. + +No new files. No server changes. No new deps. + +--- + +## Task 1: `GridHeaderCell` — accept `property` as a prop + +**Files:** +- Modify: `apps/client/src/features/base/components/grid/grid-header-cell.tsx` + +- [ ] **Step 1: Add `property` to `GridHeaderCellProps`** + +```tsx +type GridHeaderCellProps = { + header: Header; + property: IBaseProperty | undefined; + loadedRowIds: string[]; +}; +``` + +- [ ] **Step 2: Replace the internal property derivation with the prop** + +Find the line near the top of the component: +```tsx +const property = header.column.columnDef.meta?.property as + | IBaseProperty + | undefined; +``` + +Remove it. Add `property` to the function parameter destructuring: +```tsx +export const GridHeaderCell = memo(function GridHeaderCell({ + header, + property, + loadedRowIds, +}: GridHeaderCellProps) { +``` + +Everything else in the file continues to reference the same `property` variable, now a prop. No further changes needed in this file. + +- [ ] **Step 3: Build** + +```bash +pnpm nx run client:build +``` + +Build will FAIL because `GridHeader` doesn't yet pass `property`. That's fine — fixed in the next task. Do not commit yet. + +--- + +## Task 2: `GridHeader` — thread `properties` / `property` through + +**Files:** +- Modify: `apps/client/src/features/base/components/grid/grid-header.tsx` + +- [ ] **Step 1: Add `properties` prop and use it to look up per-cell property** + +Before: +```tsx +type GridHeaderProps = { + table: Table; + baseId?: string; + // Passed explicitly to break memo when columns change + // (table ref is stable from useReactTable, so memo won't fire without this) + columnOrder: ColumnOrderState; + loadedRowIds: string[]; + onPropertyCreated?: () => void; +}; + +export const GridHeader = memo(function GridHeader({ + table, + baseId, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + columnOrder: _columnOrder, + loadedRowIds, + onPropertyCreated, +}: GridHeaderProps) { + const headerGroups = table.getHeaderGroups(); + + return ( +
+ {headerGroups[0]?.headers.map((header) => ( + + ))} + ... +``` + +After: +```tsx +import { IBaseProperty, IBaseRow } from "@/features/base/types/base.types"; + +type GridHeaderProps = { + table: Table; + baseId?: string; + // Passed explicitly to break memo when columns change + // (table ref is stable from useReactTable, so memo won't fire without these) + columnOrder: ColumnOrderState; + properties: IBaseProperty[]; + loadedRowIds: string[]; + onPropertyCreated?: () => void; +}; + +export const GridHeader = memo(function GridHeader({ + table, + baseId, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + columnOrder: _columnOrder, + properties, + loadedRowIds, + onPropertyCreated, +}: GridHeaderProps) { + const headerGroups = table.getHeaderGroups(); + const propertyById = useMemo(() => { + const map = new Map(); + for (const p of properties) map.set(p.id, p); + return map; + }, [properties]); + + return ( +
+ {headerGroups[0]?.headers.map((header) => ( + + ))} + ... +``` + +Make sure `useMemo` is imported from react (it already imports `memo`; add `useMemo` alongside). + +- [ ] **Step 2: Build** + +```bash +pnpm nx run client:build +``` + +Still expected to fail — `GridContainer` doesn't yet pass `properties`. Next task. + +--- + +## Task 3: `GridContainer` — accept and forward `properties` + +**Files:** +- Modify: `apps/client/src/features/base/components/grid/grid-container.tsx` + +- [ ] **Step 1: Add `properties` to props** + +Near the existing `GridContainerProps` type (look near the top of the file), add: +```tsx +properties: IBaseProperty[]; +``` + +Add `IBaseProperty` to the existing `@/features/base/types/base.types` import at the top of the file if it's not already imported. + +Destructure it in the component signature. + +- [ ] **Step 2: Pass it to ``** + +Find the `` JSX at roughly line 239-245 and add: +```tsx + +``` + +- [ ] **Step 3: Build** + +Still expected to fail — `BaseTable` doesn't yet pass `properties` to `GridContainer`. Next task. + +--- + +## Task 4: `BaseTable` — pass `base.properties` to `GridContainer` + +**Files:** +- Modify: `apps/client/src/features/base/components/base-table.tsx` + +- [ ] **Step 1: Add the prop** + +Find the `` JSX at line 187. Add: +```tsx + +``` + +`base` is already guaranteed non-null at this point — line 174 has `if (!base) return null;`. + +- [ ] **Step 2: Build** + +```bash +pnpm nx run client:build +``` + +Should succeed now — grid path is complete. + +- [ ] **Step 3: Commit the grid-side changes as one unit** + +```bash +git add \ + apps/client/src/features/base/components/grid/grid-header-cell.tsx \ + apps/client/src/features/base/components/grid/grid-header.tsx \ + apps/client/src/features/base/components/grid/grid-container.tsx \ + apps/client/src/features/base/components/base-table.tsx +git commit -m "fix(base): refresh grid headers when a property is renamed" +``` + +The four files have to land together or the build is broken — one commit. + +--- + +## Task 5: `ViewFieldVisibility` — accept `properties` and include it in the memo + +**Files:** +- Modify: `apps/client/src/features/base/components/views/view-field-visibility.tsx` + +- [ ] **Step 1: Add `properties` to the props type** + +```tsx +type ViewFieldVisibilityProps = { + opened: boolean; + onClose: () => void; + table: Table; + properties: IBaseProperty[]; + onPersist: () => void; + children: React.ReactNode; +}; +``` + +- [ ] **Step 2: Add `properties` to the `useMemo` dep list** + +Change: +```tsx +const columns = useMemo(() => { + return table + .getAllLeafColumns() + .filter((col) => col.id !== "__row_number"); +}, [table]); +``` + +To: +```tsx +const columns = useMemo(() => { + return table + .getAllLeafColumns() + .filter((col) => col.id !== "__row_number"); +}, [table, properties]); +``` + +We still derive columns from `table` (that's where `col.getIsVisible()` / `col.getCanHide()` live), but `properties` is added as a dep so the memo invalidates whenever properties change — forcing a re-read of `table.getAllLeafColumns()` which by then reflects the renamed metadata. + +Also destructure `properties` in the function signature. + +- [ ] **Step 3: Build** + +Expected to fail until the toolbar passes `properties`. + +--- + +## Task 6: `BaseToolbar` — pass `base.properties` to `ViewFieldVisibility` + +**Files:** +- Modify: `apps/client/src/features/base/components/base-toolbar.tsx` + +- [ ] **Step 1: Pass the prop** + +Find `` (near the bottom of the file). Add: +```tsx + setFieldsOpened(false)} + table={table} + properties={base.properties} + onPersist={onPersistViewConfig} +> +``` + +`base` is already a prop on `BaseToolbar` — no other plumbing needed. + +- [ ] **Step 2: Build** + +```bash +pnpm nx run client:build +``` + +Should succeed. + +- [ ] **Step 3: Commit the popover-side changes** + +```bash +git add \ + apps/client/src/features/base/components/views/view-field-visibility.tsx \ + apps/client/src/features/base/components/base-toolbar.tsx +git commit -m "fix(base): refresh hide-fields popover when a property is renamed" +``` + +--- + +## Task 7: USER smoke test + +> ⚠️ **Do not run `pnpm dev` as an agent.** Hand off to the user. + +Ask the user to run through: + +- [ ] **Local rename updates the grid header instantly.** + 1. Open a base. + 2. Click a column header → Rename → type a new name → press Enter. + 3. The column header text updates immediately — no reload. + +- [ ] **Local rename updates the hide-fields popover instantly.** + 1. After renaming, open the Hide fields popover. + 2. The property's entry in the list shows the new name. + +- [ ] **Remote rename (other client) updates without reload.** + 1. Open the same base in two browsers / tabs (A and B). + 2. In A, rename a property. + 3. In B, within a second, the header text (and hide-fields popover) show the new name. + +- [ ] **Regression: column resize, reorder, hide, sort, filter all still work.** + +If all pass, the fix is complete. Otherwise report back with which case failed. + +--- + +## Out of scope + +- `ViewSortConfigPopover` / `ViewFilterConfigPopover` also show property names, but they read from `base.properties` directly (they already get `base` as a prop and re-render when it changes), so they weren't broken by this bug. Not touching them. +- Property icons (`property.type` change). A type change already bumps `schemaVersion` on the base, which invalidates and refetches — that path works. Out of scope here. +- Server-side — rename already persists + broadcasts correctly. diff --git a/docs/superpowers/plans/2026-04-19-infinite-scroll-loop-fix-v2.md b/docs/superpowers/plans/2026-04-19-infinite-scroll-loop-fix-v2.md new file mode 100644 index 00000000..fd1dfe93 --- /dev/null +++ b/docs/superpowers/plans/2026-04-19-infinite-scroll-loop-fix-v2.md @@ -0,0 +1,231 @@ +# Infinite Scroll Fetch Loop Fix v2 Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Kill the pagination loop that still fires extra `POST /bases/rows` requests as the user scrolls continuously through a 10K+ base, even after the v1 rows-length guard. + +**Architecture:** Replace the `virtualItems`-polling effect with a scroll-event-driven trigger plus a **synchronous** pending-fetch ref. Two independent dedup mechanisms: (a) only consider firing after a real scroll event (debounced), (b) never dispatch a second `fetchNextPage` until the previous one's resulting page has been committed to the cache. The previous v1 fix relied on `rows.length` growing after each commit, which under sustained scrolling lets react-query schedule multiple fetches before its own `isFetchingNextPage` observed-true state propagates to our render. + +**Tech Stack:** React 18, `@tanstack/react-virtual`, `@tanstack/react-query` v5. + +--- + +## Background + +### What v1 fixed and what it missed + +v1 ([grid-container.tsx, commit `c4d8b6c3`](apps/client/src/features/base/components/grid/grid-container.tsx)) added `lastTriggeredRowsLenRef` — "don't fire again until `rows.length` grows past the last value we triggered on." That breaks the *idle-at-bottom* loop (where the effect was firing every render on the same `rows.length`). + +It still leaks under sustained scrolling: + +1. User scrolls rapidly. Effect runs. Condition holds. Ref updates to N. `onFetchNextPage()` dispatched. +2. react-query enqueues a fetch. But the internal state object mutations reaching our `isFetchingNextPage` snapshot happen across a microtask boundary; within the SAME React render commit there's a window where `isFetchingNextPage` is still `false` from React's perspective. +3. The effect dep list includes `virtualItems`, which `virtualizer.getVirtualItems()` mints fresh every render. Any other state change (scroll position, virtual measurements, React 18 batching flushes) re-fires the effect. +4. Our `rows.length <= ref` gate blocks further fires AT THE SAME LENGTH. But because the user is SCROLLING, as soon as a page DOES commit, `rows.length` jumps, and on that same commit render the effect can fire repeatedly (e.g., due to scroll-driven re-measures) because the `isFetchingNextPage` false window can overlap with the new render. +5. If enough fetches pile up, react-query dispatches each in sequence. Requests are only partly deduped — each call while a fetch is in flight returns the same promise, BUT if the commit has already landed and `isFetchingNextPage` flipped briefly to false before our render observed it, that call dispatches a NEW fetch. Net: 10× more dispatches than pages committed. + +Reported symptom: on the 10K base the loop kicks in after a while of scrolling. Network panel shows `1176 requests` initiated, only `124` loaded — ~10× over-dispatch. + +### The fix shape + +Two orthogonal locks that together make over-dispatch impossible: + +**Lock A — synchronous pending flag.** A `pendingFetchRef` set to `true` SYNCHRONOUSLY right before `onFetchNextPage()` is called. Cleared in a separate effect that watches `isFetchingNextPage` transitioning back to `false` (i.e., the fetch that we started has resolved). While the ref is set, the trigger never re-fires. This eliminates the same-render-double-fire window that the v1 length guard couldn't cover. + +**Lock B — real scroll events, not render-driven polling.** Attach a listener to `scrollRef`'s `scroll` event, debounced ~50 ms. On each debounced scroll, compute "am I near the bottom" from raw `scrollTop + clientHeight >= scrollHeight - threshold`. This removes `virtualItems` from the trigger path entirely — no effect is running on every render. + +With both locks, the worst-case dispatch cadence is *one per debounced scroll tick where you're near the bottom*. Combined with Lock A's pending gate, you get *at most one request in flight at a time*, committing sequentially as the user scrolls. + +Keep the v1 `lastTriggeredRowsLenRef` guard as a safety net (it's cheap and prevents re-trigger against stale row-set data). + +--- + +## File Structure + +**Modified:** +- `apps/client/src/features/base/components/grid/grid-container.tsx` — replace the current trigger effect; add a pending ref + a reset effect. + +No new files, no new deps, nothing server-side. + +--- + +## Task 1: Replace the trigger with a scroll-driven + pending-guarded version + +**File:** `apps/client/src/features/base/components/grid/grid-container.tsx` + +### Step 1: Add a pending ref next to the existing `lastTriggeredRowsLenRef` + +Inside `GridContainer`, right after `const lastTriggeredRowsLenRef = useRef(0);` (around line 70), add: + +```ts +// Synchronous guard: set to true the moment we dispatch `onFetchNextPage`, +// cleared only after `isFetchingNextPage` has transitioned back to false. +// This closes the "React 18 batching / snapshot staleness" window where +// `isFetchingNextPage` from the hook is still observed false even though +// a dispatch is already in flight — which is how the v1 length-only +// guard still permits over-dispatch during sustained scrolling. +const pendingFetchRef = useRef(false); +``` + +### Step 2: DELETE the existing trigger effect + +Locate the effect at roughly lines 122-130: + +```ts +useEffect(() => { + if (!hasNextPage || isFetchingNextPage || !onFetchNextPage) return; + const lastItem = virtualItems[virtualItems.length - 1]; + if (!lastItem) return; + if (lastItem.index < rows.length - OVERSCAN * 2) return; + if (rows.length <= lastTriggeredRowsLenRef.current) return; + lastTriggeredRowsLenRef.current = rows.length; + onFetchNextPage(); +}, [virtualItems, rows.length, hasNextPage, isFetchingNextPage, onFetchNextPage]); +``` + +Delete it entirely. We're replacing render-polling with scroll-event-driven. + +### Step 3: Add a new scroll-event-driven effect in its place + +Insert this (same location): + +```ts +// Scroll-event-driven pagination trigger. Previously this was a +// render-effect that polled `virtualItems` — which runs on every render +// (virtualItems has fresh identity each call) and over-dispatches when +// React's `isFetchingNextPage` snapshot is stale relative to react-query's +// in-flight state. A plain scroll event with a small debounce and a +// synchronous pending ref fires at most once per scroll pulse AND +// at most one in-flight request at a time. +useEffect(() => { + const el = scrollRef.current; + if (!el) return; + + const NEAR_BOTTOM_PX = ROW_HEIGHT * OVERSCAN * 2; + let debounceTimer: ReturnType | null = null; + + const maybeFetch = () => { + if (!onFetchNextPage) return; + if (!hasNextPage) return; + if (isFetchingNextPage) return; + if (pendingFetchRef.current) return; + const node = scrollRef.current; + if (!node) return; + const distanceFromBottom = + node.scrollHeight - (node.scrollTop + node.clientHeight); + if (distanceFromBottom > NEAR_BOTTOM_PX) return; + if (rows.length <= lastTriggeredRowsLenRef.current) return; + + pendingFetchRef.current = true; + lastTriggeredRowsLenRef.current = rows.length; + onFetchNextPage(); + }; + + const onScroll = () => { + if (debounceTimer) clearTimeout(debounceTimer); + debounceTimer = setTimeout(maybeFetch, 50); + }; + + // Also evaluate once on mount / when deps change — covers the case + // where the user hasn't scrolled yet but the viewport is already + // past the near-bottom threshold (e.g. first page is short). + maybeFetch(); + + el.addEventListener("scroll", onScroll, { passive: true }); + return () => { + el.removeEventListener("scroll", onScroll); + if (debounceTimer) clearTimeout(debounceTimer); + }; +}, [rows.length, hasNextPage, isFetchingNextPage, onFetchNextPage]); +``` + +Notes the implementer must not change: +- The `maybeFetch()` call BEFORE adding the scroll listener is intentional — it handles "viewport already past threshold on mount/commit" without requiring a scroll. +- `NEAR_BOTTOM_PX = ROW_HEIGHT * OVERSCAN * 2` keeps the trigger threshold equivalent to the old `lastItem.index >= rows.length - OVERSCAN * 2` rule (20 rows * 36 px = 720 px). +- `pendingFetchRef.current = true` is set BEFORE `onFetchNextPage()` so a synchronous re-entry can't slip through. +- `passive: true` on the listener is performance-critical on large lists. + +### Step 4: Add an effect that clears `pendingFetchRef` when a fetch resolves + +Immediately after the effect from Step 3, add: + +```ts +useEffect(() => { + if (!isFetchingNextPage) { + // react-query's fetch we triggered has resolved (data committed + + // isFetchingNextPage back to false). Release the pending gate. + pendingFetchRef.current = false; + } +}, [isFetchingNextPage]); +``` + +This is the counterpart to Step 3's `pendingFetchRef.current = true`. The flag lifecycle: +- `false` initially +- set `true` synchronously just before `onFetchNextPage()` +- set back to `false` as soon as `isFetchingNextPage` observed goes back to `false` + +Between those, no new dispatch is allowed. + +### Step 5: Keep the existing reset effect + +The effect at roughly lines 132-140 that resets `lastTriggeredRowsLenRef` to 0 when `rows.length` drops (view/filter/sort switch) must stay as-is. Do NOT delete it. + +### Step 6: Build + +```bash +pnpm nx run client:build +``` + +Expected: success. + +### Step 7: Commit + +```bash +git add apps/client/src/features/base/components/grid/grid-container.tsx +git commit -m "fix(base): drive pagination from scroll events with in-flight gate to kill dispatch loop" +``` + +--- + +## Task 2: USER smoke test + +> ⚠️ **Do not run `pnpm dev` as an agent.** Hand off. + +On the 10K base (should also work on 100K): + +- [ ] **Rapid continuous scroll to bottom.** + 1. DevTools → Network → filter to `Fetch/XHR` → clear. + 2. Scroll the scrollbar smoothly from top to bottom of the 10K base, no pauses. + 3. Expected: roughly one `POST /bases/rows` per page (~100 total for 10K rows / 100 per page). NO "n requests in flight with only k loaded" state — completed count should track initiated count closely. + +- [ ] **Idle at bottom for 30 s.** + 1. After reaching the bottom, wait. + 2. Zero additional requests fire. + +- [ ] **Scroll up and back down.** + 1. Scroll up to row 5000, then back to bottom. + 2. No refetch of already-cached pages; only new pages (if any remain) fire. + +- [ ] **Sort change mid-scroll.** + 1. While at row 5000, change the sort from Title-asc to Title-desc. + 2. Pagination resets cleanly. Fetches the first page of the new sort order; scrolling continues to fetch normally. + +- [ ] **Filter that narrows to few hundred rows.** + 1. Apply a filter producing ~200 matches. + 2. Scroll to bottom. Exactly ~2 fetches (first page was already loaded + next one). Stops cleanly. + +- [ ] **Regression: unsorted base.** + 1. Remove sort/filter. Scroll through. Fetches still fire correctly. + +- [ ] **Regression: create row at top of scroll.** + 1. Scroll to top, add a new row. It appears. No extra pagination fires. + +If any step shows over-dispatch (initiated » loaded) or misses a page, report which with approximate counts. + +--- + +## Out of scope + +- Swapping to an `IntersectionObserver` sentinel. Scroll-event + debounce achieves the same dedup without the complexity of maintaining a sentinel element inside a virtualized grid. +- Measuring real row heights via `measureElement`. Unrelated to the dispatch loop; useful later for pixel-perfect scrolling. +- A visible "loading more…" indicator during fetch. UX only. diff --git a/docs/superpowers/plans/2026-04-19-infinite-scroll-loop-fix.md b/docs/superpowers/plans/2026-04-19-infinite-scroll-loop-fix.md new file mode 100644 index 00000000..0bed8775 --- /dev/null +++ b/docs/superpowers/plans/2026-04-19-infinite-scroll-loop-fix.md @@ -0,0 +1,189 @@ +# Infinite Scroll Fetch Loop Fix Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop `fetchNextPage` from firing in a tight re-render loop when the user is near the bottom of a large sorted base — currently produces 900+ redundant page fetches and only stops when the user scrolls back up. + +**Architecture:** The current trigger is a `useEffect` whose dependency list includes `virtualItems` (a fresh array from `virtualizer.getVirtualItems()` on every render). The effect re-runs on every render, and once the "near bottom" condition is satisfied it re-fires until the virtualizer's computed visible range moves away from the end. Fix: gate the trigger with a ref that records the `rows.length` at which we last fetched — guarantees at most one fetch per new page of data until the user actually scrolls further down. + +**Tech Stack:** React 18, `@tanstack/react-virtual`, `@tanstack/react-query` v5 `useInfiniteQuery`. + +--- + +## Background + +Screenshot from the field shows the Network panel accumulating `POST /bases/rows` requests — roughly 1 request per ~40 ms — for the 10K-row seed base while a sort is active. Status stays at "50 / 971 requests" and climbing. The loop ends only when the user scrolls back up. + +### Why the loop happens + +Current trigger at [grid-container.tsx:114-121](apps/client/src/features/base/components/grid/grid-container.tsx:114): + +```ts +useEffect(() => { + if (!hasNextPage || isFetchingNextPage || !onFetchNextPage) return; + const lastItem = virtualItems[virtualItems.length - 1]; + if (!lastItem) return; + if (lastItem.index >= rows.length - OVERSCAN * 2) { + onFetchNextPage(); + } +}, [virtualItems, rows.length, hasNextPage, isFetchingNextPage, onFetchNextPage]); +``` + +Three compounding issues: + +1. **`virtualItems` is a fresh array on every render.** `virtualizer.getVirtualItems()` returns a newly-constructed array each call, so React sees a changed dep every render. The effect executes on every render. + +2. **After a fetch resolves, rows grow but `lastItem.index` can still satisfy the "near bottom" threshold** — particularly when the user was scrolled all the way to the bottom. The virtualizer preserves `scrollTop`, so the previously-rendered last row is now near the middle-end of the new (larger) virtual list, and its index is right inside the `rows.length - OVERSCAN * 2` window. + +3. **Heights are estimated via `estimateSize: () => 36`.** When `.cell`'s actual rendered height exceeds 36 (min-height only), the virtualizer re-measures, which shifts the reported `virtualItems` by a few indices each render — enough to keep `lastItem.index` hovering inside the trigger zone indefinitely. + +So on each render: `virtualItems` has new identity → effect runs → `isFetchingNextPage` just transitioned to `false` after the previous page's commit → condition still holds → fire again. The loop can only break when the user scrolls far enough up that `lastItem.index < rows.length - OVERSCAN * 2` on the next render. + +### The fix shape + +Add a ref `lastTriggeredRowsLenRef` that records the `rows.length` value at which we last fired `onFetchNextPage`. The trigger is allowed only when `rows.length > lastTriggeredRowsLenRef.current` — i.e., the previous fetch has committed new rows, AND we haven't yet fired against this new length. When a page arrives, `rows.length` grows, the guard permits one fire, we update the ref, and subsequent re-renders at the same `rows.length` are no-ops. + +This pattern is standard for effect-based infinite scroll and is exactly what's missing here. + +--- + +## File Structure + +**Modified:** +- `apps/client/src/features/base/components/grid/grid-container.tsx` — one ref + one guard line added to the existing effect. + +No new files, no new deps, nothing server-side. + +--- + +## Task 1: Guard the fetch trigger against repeat fires at the same rows.length + +**File:** `apps/client/src/features/base/components/grid/grid-container.tsx` + +- [ ] **Step 1: Add `useRef` to the existing react import** + +The file currently imports `{ useCallback, useEffect, useMemo, useRef, useState }` — confirm `useRef` is already present (it almost certainly is, given `scrollRef` exists). If not, add it. + +- [ ] **Step 2: Declare the ref next to the other refs inside `GridContainer`** + +Find where `scrollRef` is declared near the top of the component. Add immediately after it: + +```ts +// Records the `rows.length` at which we last triggered a page fetch. +// The trigger effect re-runs on every render (its `virtualItems` dep +// has a new identity each call) and can't rely on `isFetchingNextPage` +// alone: once a page commits, `isFetchingNextPage` flips to false for +// one render, the "near bottom" condition still holds because the +// virtualizer anchors on the old scroll position, and we'd fire again. +// Gating on `rows.length` guarantees at most one fire per new page. +const lastTriggeredRowsLenRef = useRef(0); +``` + +- [ ] **Step 3: Add the guard to the trigger effect** + +Replace the existing effect block at roughly lines 114-121: + +Before: +```ts +useEffect(() => { + if (!hasNextPage || isFetchingNextPage || !onFetchNextPage) return; + const lastItem = virtualItems[virtualItems.length - 1]; + if (!lastItem) return; + if (lastItem.index >= rows.length - OVERSCAN * 2) { + onFetchNextPage(); + } +}, [virtualItems, rows.length, hasNextPage, isFetchingNextPage, onFetchNextPage]); +``` + +After: +```ts +useEffect(() => { + if (!hasNextPage || isFetchingNextPage || !onFetchNextPage) return; + const lastItem = virtualItems[virtualItems.length - 1]; + if (!lastItem) return; + if (lastItem.index < rows.length - OVERSCAN * 2) return; + if (rows.length <= lastTriggeredRowsLenRef.current) return; + lastTriggeredRowsLenRef.current = rows.length; + onFetchNextPage(); +}, [virtualItems, rows.length, hasNextPage, isFetchingNextPage, onFetchNextPage]); +``` + +Why `<=` and not `<`: after a fire, the ref holds the pre-fetch `rows.length`. When the page commits, `rows.length` grows, so `rows.length > ref` and the next fire is allowed (exactly once, since the ref then captures the new length). + +- [ ] **Step 4: Reset the ref when the row set resets (new base / view / sort / filter)** + +Different query-key means `rowsData` is discarded and pagination starts over from page 1. We need to reset our guard too, or it'll block the first fetch of the next query. + +Find the existing effect (around line 62 in the parent `BaseTable`, or an equivalent in `GridContainer`) that reacts to `baseId` / activeView id changing, OR use the infinite query's data being discarded. In `GridContainer`, the `rows` prop comes in fresh when the parent re-runs the infinite query. When `rows.length` becomes `0` (reset) OR becomes *smaller* than what we'd last seen, the ref must reset. Add this effect AFTER the trigger effect from Step 3: + +```ts +useEffect(() => { + // When the underlying row set shrinks (filter changed, sort toggled, + // view switched) or resets to zero, we're on a fresh pagination + // sequence — un-gate the trigger so the first page triggers a + // potential next fetch correctly. + if (rows.length === 0 || rows.length < lastTriggeredRowsLenRef.current) { + lastTriggeredRowsLenRef.current = 0; + } +}, [rows.length]); +``` + +- [ ] **Step 5: Build** + +```bash +pnpm nx run client:build +``` + +Expected: success. + +- [ ] **Step 6: Commit** + +```bash +git add apps/client/src/features/base/components/grid/grid-container.tsx +git commit -m "fix(base): stop infinite fetch loop when sorted list scrolled to bottom" +``` + +--- + +## Task 2: USER smoke test + +> ⚠️ **Do not run `pnpm dev` as an agent.** Hand off. + +On the 10K seed base: + +- [ ] **Sorted scroll to bottom, count requests.** + 1. Open the 10K base. Apply sort by Title ascending. + 2. Open DevTools → Network → filter to `Fetch/XHR`. + 3. Clear the Network log. + 4. Scroll to the very bottom using the scrollbar. + 5. Count the `bases/rows` requests. Expected: roughly 10K / 100 = **~100**, not 900+. It's fine if it's slightly over 100 due to pre-fetch, but it should cleanly stop within a few seconds of reaching the bottom. + +- [ ] **Dwell at the bottom without scrolling.** + 1. Stay at the bottom for 10 seconds. + 2. No additional `bases/rows` requests should fire. (Before the fix: would be dozens per second.) + +- [ ] **Scroll back up, then back down.** + 1. Scroll to top. Verify sort order is still correct (the earlier fix for client-side position sort should still hold). + 2. Scroll back to bottom. Should not refetch already-cached pages. + +- [ ] **Change the sort.** + 1. Remove the current sort and add a different one. + 2. Query resets. Pagination restarts from page 1. No lingering loop. + +- [ ] **Unsorted base (regression).** + 1. Remove all sorts. + 2. Scroll through. Fetching still works and stops correctly. + +- [ ] **Filter applied.** + 1. Add a filter that returns ~2,000 matches. + 2. Scroll. Should fetch ~20 pages and stop cleanly. + +If any step misbehaves — especially if requests still loop — capture the Network count and which filter/sort was active, and report back. + +--- + +## Out of scope + +- Switching from the effect-based trigger to an `IntersectionObserver` sentinel. Cleaner pattern but larger diff; the ref guard is the surgical fix for the reported bug. +- Making the virtualizer measure actual row heights via `measureElement` (would remove one of the three compounding causes). Worth it later; not required to fix the loop. +- Showing a visible "loading more…" indicator at the bottom during fetch. Orthogonal UX improvement.