From 7452d01c1809590b3f6281755328bb64dcab5b3e Mon Sep 17 00:00:00 2001 From: Amal Date: Sat, 1 Aug 2026 11:06:29 -0700 Subject: [PATCH 1/3] fix(tabular): date the folder-rows migration after already-shipped ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WHY THIS MATTERS README's upgrade procedure tells operators to "apply the migrations dated AFTER the version of Mike you currently have deployed, in filename order." This migration was named 20260724_02, which sorts BEFORE two migrations already on main (20260726_01 pagination, 20260727_01 ids overview). An operator who deployed at/after 20260727 would skip 20260724 entirely — the new tabular_review_rows / _sources tables would never be created, and the folder-rows feature would fail at runtime against a schema that silently lacks its tables. WHAT IS A DATE-ORDERED MIGRATION LOG The repo has no migration-state table; the only ordering signal is the filename date. "Apply everything newer than my deploy" is only correct if new work always carries a date later than everything already shipped. A back-dated file is invisible to that rule — a classic migration-ordering trap. HOW IT WORKS - Rename 20260724_02_tabular_folder_rows.sql -> 20260801_01_… so it sorts after every migration currently on main. - Add the repo-convention "-- Migration date: 2026-08-01" header comment (every other migration carries one; the linter/reader relies on it). - Order the legacy-row backfill by filename (matching the create path, which sorts rows by label) instead of by opaque document_id, so upgraded reviews get the same row order as freshly created ones. Co-Authored-By: Claude Opus 4.8 --- ...lder_rows.sql => 20260801_01_tabular_folder_rows.sql} | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) rename backend/migrations/{20260724_02_tabular_folder_rows.sql => 20260801_01_tabular_folder_rows.sql} (90%) diff --git a/backend/migrations/20260724_02_tabular_folder_rows.sql b/backend/migrations/20260801_01_tabular_folder_rows.sql similarity index 90% rename from backend/migrations/20260724_02_tabular_folder_rows.sql rename to backend/migrations/20260801_01_tabular_folder_rows.sql index 7cbb50314..7c893575f 100644 --- a/backend/migrations/20260724_02_tabular_folder_rows.sql +++ b/backend/migrations/20260801_01_tabular_folder_rows.sql @@ -1,3 +1,5 @@ +-- Migration date: 2026-08-01 + -- Support one tabular-review row per project folder, with multiple source documents. alter table public.tabular_reviews add column if not exists document_grouping text not null default 'document' @@ -42,7 +44,12 @@ select distinct on (cell.review_id, cell.document_id) coalesce(document.filename, 'Untitled document'), 'document', cell.document_id, - row_number() over (partition by cell.review_id order by cell.document_id) - 1 + -- Order legacy rows by filename (matching the create path, which sorts rows + -- by label) rather than by opaque document_id; document_id breaks ties. + row_number() over ( + partition by cell.review_id + order by coalesce(document.filename, 'Untitled document'), cell.document_id + ) - 1 from public.tabular_cells cell join public.documents document on document.id = cell.document_id where cell.row_id is null From 0ac1a53184c83e66dee0b95cbef3261ef03bb963 Mon Sep 17 00:00:00 2001 From: Amal Date: Sat, 1 Aug 2026 11:06:57 -0700 Subject: [PATCH 2/3] fix(tabular): generate, regenerate and edit folder rows by row_id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WHY THIS MATTERS Folder grouping persisted rows but nothing consumed them: /generate, /regenerate-cell and PATCH all keyed cells on document_id. A folder row's cells carry document_id NULL and a row_id, so those cells could never be addressed — a folder-grouped review sat on "pending" forever, and PATCH edited cells without ever touching the row tables, desyncing them. This wires the whole write path to rows so the feature actually works end to end. WHAT IS A "ROW" HERE A tabular_review_rows row is the unit a cell belongs to. A document review has one row per document (row_id + document_id both set). A folder review has one row per project subfolder (row_id set, document_id NULL) whose source documents live in tabular_review_row_sources. Every cell now hangs off a row_id (the migration backfills legacy cells), so row_id — not document_id — is the correct join key for generation and edits. HOW IT WORKS - /generate iterates rows (loadReviewRows), not documents. For each row it concatenates its source documents' extracted markdown, updates cells by row_id, and emits SSE events carrying BOTH row_id and document_id (the latter for backward-compatible clients). A legacy document-only fallback keeps pre-rows reviews generating. - buildRowMarkdown caps the concatenated text at ROW_MARKDOWN_MAX_CHARS (120k) — a folder row can hold N documents, and without a ceiling one row could blow past the model context window / the per-cell 120k slice. Each source is headed by its filename so the model can tell them apart. - /regenerate-cell accepts row_id (folder cells) as well as document_id (unchanged for document cells); either identifier resolves the same cell. - PATCH reconciles the row tables instead of poking cells by document_id: removed docs delete fully-emptied rows (FK cascade wipes their cells + sources) and trim partially-emptied folder rows' sources; added docs are planned via the shared planRows() and either merged into an existing folder row or inserted as new rows; then one pending cell is ensured per (live row x active column), keyed on row_id — so grouped reviews no longer desync and new cells always carry a row_id. - fetchSourceDocuments / getFolderPathMap / loadReviewRows now throw on the Supabase error instead of returning empty. A swallowed transient read would otherwise create a review with zero rows/cells and no signal; throwing lets the create-path rollback (delete the review) run. WHAT IS AN UNCHECKED SUPABASE ERROR supabase-js resolves (never rejects) with { data, error }. Destructuring only `data` turns a failed query into a silent empty result. Reading `error` and throwing converts that into a real failure the caller can compensate for. TESTS - folder-row generation reaches status "done" for a document_id-NULL cell (the F1 regression), asserted on the SSE stream. - create-path rollback when the rows insert (and when the source-doc read) fails. - GET returns rows with resolved source_document_ids. - PATCH inserts a new column's cell with a row_id, and creates a review row when a document is added (proving the row tables stay in sync). - nested-folder path labels ("Parent / Child") + "Unknown folder" fallback. - regenerate-cell addressed by row_id. - tabular_review_rows + _sources added to the stack test's PUBLIC_TABLES so the deny-all RLS lockdown on them is CI-enforced. Co-Authored-By: Claude Opus 4.8 --- .../integration/stack.supabase.test.ts | 1 + .../integration/tabular.routes.test.ts | 458 ++++++++++ backend/src/routes/tabular.ts | 795 ++++++++++++------ 3 files changed, 1019 insertions(+), 235 deletions(-) diff --git a/backend/src/__tests__/integration/stack.supabase.test.ts b/backend/src/__tests__/integration/stack.supabase.test.ts index b74a051c4..300c69c3b 100644 --- a/backend/src/__tests__/integration/stack.supabase.test.ts +++ b/backend/src/__tests__/integration/stack.supabase.test.ts @@ -28,6 +28,7 @@ const PUBLIC_TABLES = [ "document_versions", "documents", "hidden_workflows", "library_folders", "project_subfolders", "projects", "tabular_cells", "tabular_review_chat_messages", "tabular_review_chats", "tabular_reviews", + "tabular_review_rows", "tabular_review_row_sources", "user_api_keys", "user_mcp_connector_tools", "user_mcp_connectors", "user_mcp_oauth_states", "user_mcp_oauth_tokens", "user_mcp_tool_audit_logs", "user_profiles", diff --git a/backend/src/__tests__/integration/tabular.routes.test.ts b/backend/src/__tests__/integration/tabular.routes.test.ts index 02ae34567..598d8ea75 100644 --- a/backend/src/__tests__/integration/tabular.routes.test.ts +++ b/backend/src/__tests__/integration/tabular.routes.test.ts @@ -14,12 +14,14 @@ const { filterAccessibleDocumentIds, getUserModelSettings, loadActiveVersion, + streamChatWithTools, } = vi.hoisted(() => ({ ensureReviewAccess: vi.fn(), checkProjectAccess: vi.fn(), filterAccessibleDocumentIds: vi.fn(), getUserModelSettings: vi.fn(), loadActiveVersion: vi.fn(), + streamChatWithTools: vi.fn(), })); // --------------------------------------------------------------------------- @@ -119,6 +121,27 @@ vi.mock("../../lib/documentVersions", () => ({ loadActiveVersion: (...args: unknown[]) => loadActiveVersion(...args), })); +// Storage + LLM are mocked so the generation loop actually runs (past its +// guards) without real IO. downloadFile → null makes markdown extraction a +// no-op; streamChatWithTools drives the per-column JSON callback. providerForModel +// stays real so the missing-API-key guard still resolves providers correctly. +vi.mock("../../lib/storage", () => ({ + downloadFile: vi.fn(async () => null), +})); + +vi.mock("../../lib/llm", async (importOriginal) => { + const actual = (await importOriginal()) as Record; + return { + ...actual, + completeText: vi.fn( + async () => + '{"summary":"Extracted","flag":"green","reasoning":"ok"}', + ), + streamChatWithTools: (...args: unknown[]) => + streamChatWithTools(...args), + }; +}); + import { app } from "../../app"; const AUTH = ["Authorization", "Bearer test"] as const; @@ -145,6 +168,18 @@ describe("tabular.routes", () => { api_keys: { claude: "sk-test" }, }); loadActiveVersion.mockResolvedValue(null); + // Default: the LLM returns one done JSON line for column 0. + streamChatWithTools.mockImplementation( + async ({ + callbacks, + }: { + callbacks?: { onContentDelta?: (t: string) => void }; + }) => { + callbacks?.onContentDelta?.( + '{"column_index":0,"summary":"Extracted","flag":"green","reasoning":"ok"}\n', + ); + }, + ); }); // ── GET /tabular-review (overview) ──────────────────────────────────── @@ -834,4 +869,427 @@ describe("tabular.routes", () => { ]); }); }); + + // ── POST /tabular-review create-path rollback ───────────────────────── + describe("POST /tabular-review rollback", () => { + it("rolls back (deletes the review) when row creation fails", async () => { + supabaseState.tables.tabular_reviews = { + data: { id: "r-rollback", title: "Bad", document_ids: ["d1"] }, + error: null, + }; + supabaseState.tables.documents = { + data: [ + { id: "d1", filename: "A.pdf", file_type: "pdf", folder_id: null }, + ], + error: null, + }; + // The rows insert fails — createRowsForReview throws, the route must + // delete the just-created review and surface the error. + supabaseState.tables.tabular_review_rows = { + data: null, + error: { message: "rows insert failed" }, + }; + + const res = await request(app) + .post("/tabular-review") + .set(...AUTH) + .send({ + title: "Bad", + document_ids: ["d1"], + columns_config: [{ index: 0, name: "Col", prompt: "p" }], + }); + + expect(res.status).toBe(500); + expect(res.body.detail).toBe("rows insert failed"); + }); + + it("throws (and rolls back) when the source-document read errors", async () => { + supabaseState.tables.tabular_reviews = { + data: { id: "r-src", title: "Src", document_ids: ["d1"] }, + error: null, + }; + // fetchSourceDocuments must no longer swallow this error. + supabaseState.tables.documents = { + data: null, + error: { message: "documents read failed" }, + }; + + const res = await request(app) + .post("/tabular-review") + .set(...AUTH) + .send({ + document_ids: ["d1"], + columns_config: [{ index: 0, name: "Col", prompt: "p" }], + }); + + expect(res.status).toBe(500); + expect(res.body.detail).toBe("documents read failed"); + }); + }); + + // ── GET /:reviewId rows payload (folder rows) ───────────────────────── + describe("GET /tabular-review/:reviewId rows", () => { + it("returns folder rows with resolved source_document_ids", async () => { + supabaseState.tables.tabular_reviews = { + data: { + id: "r1", + user_id: "u1", + project_id: "p1", + document_ids: ["d1", "d2"], + columns_config: [{ index: 0, name: "Col", prompt: "p" }], + document_grouping: "folder", + }, + error: null, + }; + supabaseState.tables.tabular_cells = { + data: [ + { + id: "c1", + row_id: "row-folder", + document_id: null, + column_index: 0, + content: null, + status: "pending", + }, + ], + error: null, + }; + supabaseState.tables.tabular_review_rows = { + data: [ + { + id: "row-folder", + review_id: "r1", + label: "Contracts", + row_type: "folder", + folder_id: "f1", + document_id: null, + sort_index: 0, + }, + ], + error: null, + }; + supabaseState.tables.tabular_review_row_sources = { + data: [ + { row_id: "row-folder", document_id: "d1" }, + { row_id: "row-folder", document_id: "d2" }, + ], + error: null, + }; + supabaseState.tables.documents = { + data: [ + { id: "d1", current_version_id: null }, + { id: "d2", current_version_id: null }, + ], + error: null, + }; + + const res = await request(app) + .get("/tabular-review/r1") + .set(...AUTH); + + expect(res.status).toBe(200); + expect(res.body.rows).toEqual([ + { + id: "row-folder", + review_id: "r1", + label: "Contracts", + row_type: "folder", + folder_id: "f1", + document_id: null, + sort_index: 0, + source_document_ids: ["d1", "d2"], + }, + ]); + }); + }); + + // ── POST /:reviewId/generate — folder rows (F1 regression) ──────────── + describe("POST /tabular-review/:reviewId/generate (folder rows)", () => { + it("generates folder-row cells to status done (keyed on row_id)", async () => { + supabaseState.tables.tabular_reviews = { + data: { + id: "r1", + user_id: "u1", + project_id: null, + columns_config: [{ index: 0, name: "Col", prompt: "p" }], + document_grouping: "folder", + }, + error: null, + }; + // The folder cell carries a row_id and NO document_id — before the + // fix it could never be addressed and stayed "pending" forever. + supabaseState.tables.tabular_cells = { + data: [ + { + id: "cell-folder", + row_id: "row-folder", + document_id: null, + column_index: 0, + content: null, + status: "pending", + }, + ], + error: null, + }; + supabaseState.tables.tabular_review_rows = { + data: [ + { + id: "row-folder", + review_id: "r1", + label: "Contracts", + row_type: "folder", + folder_id: "f1", + document_id: null, + sort_index: 0, + }, + ], + error: null, + }; + supabaseState.tables.tabular_review_row_sources = { + data: [ + { row_id: "row-folder", document_id: "d1" }, + { row_id: "row-folder", document_id: "d2" }, + ], + error: null, + }; + supabaseState.tables.documents = { + data: [ + { id: "d1", current_version_id: null }, + { id: "d2", current_version_id: null }, + ], + error: null, + }; + + const res = await request(app) + .post("/tabular-review/r1/generate") + .set(...AUTH); + + expect(res.status).toBe(200); + // The SSE stream must emit a "done" cell_update for the folder row, + // identified by row_id, with document_id null. + const doneLine = res.text + .split("\n") + .find( + (line) => + line.includes('"row_id":"row-folder"') && + line.includes('"status":"done"'), + ); + expect(doneLine).toBeDefined(); + expect(doneLine).toContain('"document_id":null'); + // The LLM was actually invoked for the folder row. + expect(streamChatWithTools).toHaveBeenCalledTimes(1); + }); + }); + + // ── PATCH maintains rows / cells row_id (F4) ────────────────────────── + describe("PATCH /tabular-review/:reviewId row maintenance", () => { + it("adds cells for a new column keyed on row_id", async () => { + supabaseState.tables.tabular_reviews = { + data: { + id: "r1", + user_id: "u1", + project_id: null, + document_grouping: "document", + columns_config: [{ index: 0, name: "A", prompt: "p" }], + }, + error: null, + }; + supabaseState.tables.tabular_review_rows = { + data: [ + { + id: "row-1", + review_id: "r1", + label: "A.pdf", + row_type: "document", + folder_id: null, + document_id: "d1", + sort_index: 0, + }, + ], + error: null, + }; + supabaseState.tables.tabular_review_row_sources = { + data: [{ row_id: "row-1", document_id: "d1" }], + error: null, + }; + supabaseState.tables.tabular_cells = { + data: [{ row_id: "row-1", document_id: "d1", column_index: 0 }], + error: null, + }; + + const res = await request(app) + .patch("/tabular-review/r1") + .set(...AUTH) + .send({ + columns_config: [ + { index: 0, name: "A", prompt: "p" }, + { index: 1, name: "B", prompt: "q" }, + ], + }); + + expect(res.status).toBe(200); + const cellInsert = supabaseState.inserts.find( + (i) => i.table === "tabular_cells", + ); + // Only the NEW column gets a cell, and it carries row_id + document_id. + expect(cellInsert?.payload).toEqual([ + { + review_id: "r1", + row_id: "row-1", + document_id: "d1", + column_index: 1, + status: "pending", + }, + ]); + }); + + it("creates a review row when a document is added (no desync)", async () => { + supabaseState.tables.tabular_reviews = { + data: { + id: "r1", + user_id: "u1", + project_id: null, + document_grouping: "document", + columns_config: [{ index: 0, name: "A", prompt: "p" }], + }, + error: null, + }; + supabaseState.tables.tabular_review_rows = { + data: [ + { + id: "row-1", + review_id: "r1", + label: "A.pdf", + row_type: "document", + folder_id: null, + document_id: "d1", + sort_index: 0, + }, + ], + error: null, + }; + supabaseState.tables.tabular_review_row_sources = { + data: [{ row_id: "row-1", document_id: "d1" }], + error: null, + }; + supabaseState.tables.tabular_cells = { + data: [{ row_id: "row-1", document_id: "d1", column_index: 0 }], + error: null, + }; + supabaseState.tables.documents = { + data: [ + { id: "d2", filename: "B.pdf", file_type: "pdf", folder_id: null }, + ], + error: null, + }; + + const res = await request(app) + .patch("/tabular-review/r1") + .set(...AUTH) + .send({ document_ids: ["d1", "d2"] }); + + expect(res.status).toBe(200); + // The added document must produce a NEW review row — the old code + // only ever touched cells, leaving the row tables desynced. + const rowInsert = supabaseState.inserts.find( + (i) => i.table === "tabular_review_rows", + ); + expect(rowInsert?.payload).toMatchObject({ + review_id: "r1", + row_type: "document", + document_id: "d2", + }); + }); + }); + + // ── nested folder labels + Unknown folder fallback ──────────────────── + describe("POST /tabular-review nested folder labels", () => { + it("labels folder rows with the nested path and falls back to Unknown folder", async () => { + supabaseState.tables.tabular_reviews = { + data: { id: "r1", title: "Nested", document_ids: ["d1", "d2"] }, + error: null, + }; + supabaseState.tables.documents = { + data: [ + { id: "d1", filename: "Child.pdf", file_type: "pdf", folder_id: "child" }, + { id: "d2", filename: "Ghost.pdf", file_type: "pdf", folder_id: "missing" }, + ], + error: null, + }; + supabaseState.tables.project_subfolders = { + data: [ + { id: "parent", name: "Parent", parent_folder_id: null }, + { id: "child", name: "Child", parent_folder_id: "parent" }, + ], + error: null, + }; + // Row insert echo isn't needed — we assert on the recorded payload. + supabaseState.tables.tabular_review_rows = { data: [], error: null }; + + const res = await request(app) + .post("/tabular-review") + .set(...AUTH) + .send({ + title: "Nested", + project_id: "p1", + document_ids: ["d1", "d2"], + document_grouping: "folder", + columns_config: [{ index: 0, name: "Col", prompt: "p" }], + }); + + expect(res.status).toBe(201); + const rowInsert = supabaseState.inserts.find( + (i) => i.table === "tabular_review_rows", + ); + const labels = (rowInsert?.payload as { label: string }[]).map( + (r) => r.label, + ); + expect(labels).toContain("Parent / Child"); + expect(labels).toContain("Unknown folder"); + }); + }); + + // ── regenerate-cell accepts row_id (folder cells) ───────────────────── + describe("POST /tabular-review/:reviewId/regenerate-cell (folder row)", () => { + it("regenerates a folder cell addressed by row_id", async () => { + supabaseState.tables.tabular_reviews = { + data: { + id: "r1", + user_id: "u1", + project_id: null, + columns_config: [{ index: 0, name: "Col", prompt: "p" }], + document_grouping: "folder", + }, + error: null, + }; + supabaseState.tables.tabular_review_rows = { + data: { + id: "row-folder", + review_id: "r1", + label: "Contracts", + row_type: "folder", + folder_id: "f1", + document_id: null, + sort_index: 0, + }, + error: null, + }; + supabaseState.tables.tabular_review_row_sources = { + data: [{ row_id: "row-folder", document_id: "d1" }], + error: null, + }; + supabaseState.tables.documents = { + data: [{ id: "d1", current_version_id: null }], + error: null, + }; + + const res = await request(app) + .post("/tabular-review/r1/regenerate-cell") + .set(...AUTH) + .send({ row_id: "row-folder", column_index: 0 }); + + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ flag: "green" }); + }); + }); }); diff --git a/backend/src/routes/tabular.ts b/backend/src/routes/tabular.ts index c45e459d2..c96155b38 100644 --- a/backend/src/routes/tabular.ts +++ b/backend/src/routes/tabular.ts @@ -99,10 +99,14 @@ async function fetchSourceDocuments( documentIds: string[], ): Promise { if (documentIds.length === 0) return []; - const { data } = await db + const { data, error } = await db .from("documents") .select("id, filename, file_type, folder_id, created_at") .in("id", documentIds); + // A transient read failure here previously returned an empty list, so the + // review would be created with zero rows/cells and no error surfaced. + // Throw so the create-path rollback (delete review) runs. + if (error) throw new Error(error.message); const position = new Map(documentIds.map((id, index) => [id, index])); return ((data ?? []) as SourceDocument[]).sort( (a, b) => (position.get(a.id) ?? 0) - (position.get(b.id) ?? 0), @@ -114,10 +118,13 @@ async function getFolderPathMap( projectId: string | null | undefined, ): Promise> { if (!projectId) return new Map(); - const { data } = await db + const { data, error } = await db .from("project_subfolders") .select("id, name, parent_folder_id") .eq("project_id", projectId); + // Surface DB failures instead of silently labelling every folder row + // "Unknown folder" — the create-path rollback depends on this throw. + if (error) throw new Error(error.message); const folders = (data ?? []) as { id: string; name: string; @@ -140,24 +147,25 @@ async function getFolderPathMap( return paths; } -async function createRowsForReview( - db: SupabaseDb, - reviewId: string, - projectId: string | null | undefined, - documentIds: string[], - columns: Column[], - grouping: DocumentGrouping, -): Promise { - const docs = await fetchSourceDocuments(db, documentIds); - const folderPaths = await getFolderPathMap(db, projectId); - const inputs: { - label: string; - row_type: "document" | "folder"; - folder_id: string | null; - document_id: string | null; - sourceIds: string[]; - }[] = []; +type RowInput = { + label: string; + row_type: "document" | "folder"; + folder_id: string | null; + document_id: string | null; + sourceIds: string[]; +}; +// Pure planner: turn a set of source documents into the review rows that should +// represent them under the chosen grouping. Folder grouping collapses same- +// folder docs into one folder row; loose docs (and all docs under "document" +// grouping) become one-document rows. Shared by the create path and PATCH. +function planRows( + docs: SourceDocument[], + folderPaths: Map, + grouping: DocumentGrouping, + projectId: string | null | undefined, +): RowInput[] { + const inputs: RowInput[] = []; if (grouping === "folder" && projectId) { const byFolder = new Map(); for (const doc of docs) { @@ -171,7 +179,10 @@ async function createRowsForReview( }); continue; } - byFolder.set(doc.folder_id, [...(byFolder.get(doc.folder_id) ?? []), doc]); + byFolder.set(doc.folder_id, [ + ...(byFolder.get(doc.folder_id) ?? []), + doc, + ]); } for (const [folderId, folderDocs] of byFolder) { inputs.push({ @@ -193,8 +204,22 @@ async function createRowsForReview( }); } } - inputs.sort((a, b) => a.label.localeCompare(b.label)); + return inputs; +} + +async function createRowsForReview( + db: SupabaseDb, + reviewId: string, + projectId: string | null | undefined, + documentIds: string[], + columns: Column[], + grouping: DocumentGrouping, +): Promise { + const docs = await fetchSourceDocuments(db, documentIds); + const folderPaths = await getFolderPathMap(db, projectId); + const inputs = planRows(docs, folderPaths, grouping, projectId); + const { data, error } = await db .from("tabular_review_rows") .insert( @@ -245,18 +270,22 @@ async function loadReviewRows( db: SupabaseDb, reviewId: string, ): Promise { - const { data } = await db + const { data, error } = await db .from("tabular_review_rows") .select("*") .eq("review_id", reviewId) .order("sort_index", { ascending: true }); + // Don't mask a read failure as "no rows" — a caller (generate/PATCH) would + // then act on an empty row set and desync the review. + if (error) throw new Error(error.message); const rows = (data ?? []) as ReviewRow[]; if (!rows.length) return rows; - const { data: sources } = await db + const { data: sources, error: sourcesError } = await db .from("tabular_review_row_sources") .select("row_id, document_id") .in("row_id", rows.map((row) => row.id)) .order("sort_index", { ascending: true }); + if (sourcesError) throw new Error(sourcesError.message); const byRow = new Map(); for (const source of sources ?? []) { byRow.set(source.row_id, [ @@ -271,6 +300,50 @@ async function loadReviewRows( })); } +// A folder row concatenates every source document into ONE LLM call. Without a +// ceiling, an N-document folder could blow past the model context window (and +// the per-cell prompt already slices to 120k chars downstream). Cap the joined +// text here so the size is bounded by the row, not by how many docs it holds. +const ROW_MARKDOWN_MAX_CHARS = 120_000; + +type EnrichedDoc = { + id: string; + filename: string; + storage_path: string | null; + file_type: string | null; +}; + +// Concatenate the extracted markdown of a row's source documents, bounded by +// ROW_MARKDOWN_MAX_CHARS. Each document is prefixed with a heading so the model +// can tell one source from another inside a grouped (folder) row. +async function buildRowMarkdown(sources: EnrichedDoc[]): Promise { + const parts: string[] = []; + let used = 0; + for (const doc of sources) { + if (used >= ROW_MARKDOWN_MAX_CHARS) break; + if (!doc.storage_path) continue; + const buf = await downloadFile(doc.storage_path); + if (!buf) continue; + let text = ""; + try { + text = await extractDocumentMarkdown(buf, doc.file_type); + } catch (err) { + console.error( + `[tabular/generate] extraction error doc=${doc.id}`, + safeErrorLog(err), + ); + continue; + } + if (!text) continue; + const heading = sources.length > 1 ? `# ${doc.filename}\n\n` : ""; + const remaining = ROW_MARKDOWN_MAX_CHARS - used; + const chunk = (heading + text).slice(0, remaining); + parts.push(chunk); + used += chunk.length; + } + return parts.join("\n\n"); +} + function providerLabel(provider: Provider): string { if (provider === "claude") return "Anthropic"; if (provider === "openai") return "OpenAI"; @@ -688,104 +761,241 @@ tabularRouter.patch("/:reviewId", requireAuth, async (req, res) => { Array.isArray(req.body.columns_config) || Array.isArray(req.body.document_ids) ) { - const { data: existingCells } = await db - .from("tabular_cells") - .select("document_id,column_index") - .eq("review_id", reviewId); - const existingKeys = new Set( - (existingCells ?? []).map( - (cell) => `${cell.document_id}:${cell.column_index}`, - ), - ); + // Everything below is keyed on rows, not document_id. The pre-folder + // code deleted/inserted cells by document_id and never touched + // tabular_review_rows / _sources — so any edit desynced grouped + // reviews (removed docs lingered in a folder row's source list, added + // docs got orphan cells with no row). Reconcile the row tables first, + // then ensure one cell per (row × active column). + try { + const grouping = normalizeGrouping(updatedReview.document_grouping); + const existingRows = await loadReviewRows(db, reviewId); + const { data: existingCells } = await db + .from("tabular_cells") + .select("row_id,document_id,column_index") + .eq("review_id", reviewId); + const existingKeys = new Set( + (existingCells ?? []) + .filter((cell) => cell.row_id) + .map((cell) => `${cell.row_id}:${cell.column_index}`), + ); - let documentIds: string[]; + // Rows we must (re)create cells for: existing minus removed plus + // added. Mutated as we reconcile below. + let liveRows: { id: string; document_id: string | null }[] = + existingRows.map((r) => ({ + id: r.id, + document_id: r.document_id, + })); + + if (Array.isArray(req.body.document_ids)) { + const requestedDocIds = req.body.document_ids as string[]; + // Document membership spans folder + document rows. + const currentDocIds = [ + ...new Set( + existingRows.flatMap( + (r) => r.source_document_ids ?? [], + ), + ), + ]; + const currentSet = new Set(currentDocIds); + const addCandidates = requestedDocIds.filter( + (id) => !currentSet.has(id), + ); + const addAllowed = new Set( + await filterAccessibleDocumentIds( + addCandidates, + userId, + userEmail, + db, + ), + ); + const newDocIds = requestedDocIds.filter( + (id) => currentSet.has(id) || addAllowed.has(id), + ); + const newDocSet = new Set(newDocIds); + const removedSet = new Set( + currentDocIds.filter((id) => !newDocSet.has(id)), + ); - if (Array.isArray(req.body.document_ids)) { - // document_ids is the new source of truth — delete removed docs' cells - const requestedDocIds = req.body.document_ids as string[]; - const existingDocIds = (existingCells ?? []).map( - (cell) => cell.document_id, - ); - const existingDocIdSet = new Set(existingDocIds); - const newDocCandidates = requestedDocIds.filter( - (id) => !existingDocIdSet.has(id), - ); - const newDocAllowed = await filterAccessibleDocumentIds( - newDocCandidates, - userId, - userEmail, - db, - ); - const newDocAllowedSet = new Set(newDocAllowed); - const newDocIds = requestedDocIds.filter( - (id) => existingDocIdSet.has(id) || newDocAllowedSet.has(id), - ); - const removedDocIds = existingDocIds.filter( - (id) => !newDocIds.includes(id), - ); + // Removals: delete a row whose every source is gone (cascade + // wipes its cells + sources); trim just the removed sources + // from folder rows that only partially lost documents. + if (removedSet.size > 0) { + const rowsToDelete = existingRows + .filter((row) => { + const src = row.source_document_ids ?? []; + return ( + src.length > 0 && + src.every((id) => removedSet.has(id)) + ); + }) + .map((row) => row.id); + if (rowsToDelete.length > 0) { + const { error: delRowErr } = await db + .from("tabular_review_rows") + .delete() + .in("id", rowsToDelete); + if (delRowErr) + return void res + .status(500) + .json({ detail: delRowErr.message }); + const deleted = new Set(rowsToDelete); + liveRows = liveRows.filter((r) => !deleted.has(r.id)); + } + const survivingRowIds = liveRows.map((r) => r.id); + if (survivingRowIds.length > 0) { + const { error: trimErr } = await db + .from("tabular_review_row_sources") + .delete() + .in("row_id", survivingRowIds) + .in("document_id", [...removedSet]); + if (trimErr) + return void res + .status(500) + .json({ detail: trimErr.message }); + } + } - if (removedDocIds.length > 0) { - const { error: deleteError } = await db + // Additions: plan rows for the newly added docs, merging into + // an existing folder row when one already covers that folder. + const addedDocIds = newDocIds.filter( + (id) => !currentSet.has(id), + ); + if (addedDocIds.length > 0) { + const projectId = + (updatedReview.project_id as string | null) ?? null; + const addedDocs = await fetchSourceDocuments( + db, + addedDocIds, + ); + const folderPaths = await getFolderPathMap(db, projectId); + const planned = planRows( + addedDocs, + folderPaths, + grouping, + projectId, + ); + const existingFolderRow = new Map(); + for (const row of existingRows) { + if (row.row_type === "folder" && row.folder_id) + existingFolderRow.set(row.folder_id, row.id); + } + let nextSort = + existingRows.reduce( + (max, r) => Math.max(max, r.sort_index), + -1, + ) + 1; + for (const input of planned) { + const mergeRowId = + input.row_type === "folder" && input.folder_id + ? existingFolderRow.get(input.folder_id) + : undefined; + if (mergeRowId) { + const { error: srcErr } = await db + .from("tabular_review_row_sources") + .insert( + input.sourceIds.map((document_id, i) => ({ + row_id: mergeRowId, + document_id, + sort_index: i, + })), + ); + if (srcErr) + return void res + .status(500) + .json({ detail: srcErr.message }); + continue; + } + const { data: rowData, error: rowErr } = await db + .from("tabular_review_rows") + .insert({ + review_id: reviewId, + label: input.label, + row_type: input.row_type, + folder_id: input.folder_id, + document_id: input.document_id, + sort_index: nextSort++, + }) + .select("*") + .single(); + if (rowErr || !rowData) + return void res.status(500).json({ + detail: rowErr?.message ?? "Failed to add row", + }); + const newRow = rowData as ReviewRow; + if (input.sourceIds.length) { + const { error: srcErr } = await db + .from("tabular_review_row_sources") + .insert( + input.sourceIds.map((document_id, i) => ({ + row_id: newRow.id, + document_id, + sort_index: i, + })), + ); + if (srcErr) + return void res + .status(500) + .json({ detail: srcErr.message }); + } + liveRows.push({ + id: newRow.id, + document_id: newRow.document_id, + }); + } + } + + persistedDocumentIds = newDocIds; + const { error: documentIdsError } = await db + .from("tabular_reviews") + .update({ + document_ids: newDocIds, + updated_at: new Date().toISOString(), + }) + .eq("id", reviewId); + if (documentIdsError) + return void res.status(500).json({ + detail: documentIdsError.message, + }); + } + + // Ensure a pending cell exists for every live row × active column. + // Covers new rows AND newly added columns; keyed by row_id so + // folder rows (document_id null) get their cells too. + const activeColumns = Array.isArray(req.body.columns_config) + ? req.body.columns_config + : (updatedReview.columns_config ?? []); + const newCells = liveRows.flatMap((row) => + activeColumns + .filter( + (column: { index: number }) => + !existingKeys.has(`${row.id}:${column.index}`), + ) + .map((column: { index: number }) => ({ + review_id: reviewId, + row_id: row.id, + document_id: row.document_id, + column_index: column.index, + status: "pending", + })), + ); + if (newCells.length > 0) { + const { error: insertError } = await db .from("tabular_cells") - .delete() - .eq("review_id", reviewId) - .in("document_id", removedDocIds); - if (deleteError) + .insert(newCells); + if (insertError) return void res .status(500) - .json({ detail: deleteError.message }); + .json({ detail: insertError.message }); } - - documentIds = newDocIds; - } else { - // No document change — derive from existing cells - documentIds = [ - ...new Set( - (existingCells ?? []).map((cell) => cell.document_id), - ), - ]; - } - - if (Array.isArray(req.body.document_ids)) { - persistedDocumentIds = documentIds; - const { error: documentIdsError } = await db - .from("tabular_reviews") - .update({ - document_ids: documentIds, - updated_at: new Date().toISOString(), - }) - .eq("id", reviewId); - if (documentIdsError) - return void res.status(500).json({ - detail: documentIdsError.message, - }); - } - - const activeColumns = Array.isArray(req.body.columns_config) - ? req.body.columns_config - : (updatedReview.columns_config ?? []); - const newCells = documentIds.flatMap((documentId) => - activeColumns - .filter( - (column: { index: number }) => - !existingKeys.has(`${documentId}:${column.index}`), - ) - .map((column: { index: number }) => ({ - review_id: reviewId, - document_id: documentId, - column_index: column.index, - status: "pending", - })), - ); - - if (newCells.length > 0) { - const { error: insertError } = await db - .from("tabular_cells") - .insert(newCells); - if (insertError) - return void res - .status(500) - .json({ detail: insertError.message }); + } catch (err) { + return void res.status(500).json({ + detail: + err instanceof Error + ? err.message + : "Failed to update review rows", + }); } } @@ -852,12 +1062,16 @@ tabularRouter.post( const userId = res.locals.userId as string; const userEmail = res.locals.userEmail as string | undefined; const { reviewId } = req.params; - const { document_id, column_index } = req.body as { - document_id: string; + // A folder cell has no document_id — the client sends the row_id + // instead. Either identifier addresses one cell; a document cell can + // still be regenerated by document_id (backward compatible). + const { document_id, row_id, column_index } = req.body as { + document_id?: string; + row_id?: string; column_index: number; }; - if (!document_id || column_index == null) + if ((!document_id && !row_id) || column_index == null) return void res .status(400) .json({ detail: "document_id and column_index are required" }); @@ -886,22 +1100,85 @@ tabularRouter.post( if (!column) return void res.status(400).json({ detail: "Column not found" }); - const docAllowed = await filterAccessibleDocumentIds( - [document_id], - userId, - userEmail, - db, - ); - if (docAllowed.length === 0) - return void res.status(404).json({ detail: "Document not found" }); - const { data: doc } = await db - .from("documents") - .select("id, current_version_id") - .eq("id", document_id) - .single(); - if (!doc) - return void res.status(404).json({ detail: "Document not found" }); - const docActive = await loadActiveVersion(document_id, db); + // Resolve the source documents + display label for the target cell. + let sources: EnrichedDoc[] = []; + let label = "Untitled document"; + if (row_id) { + const { data: row, error: rowError } = await db + .from("tabular_review_rows") + .select("*") + .eq("id", row_id) + .eq("review_id", reviewId) + .single(); + if (rowError || !row) + return void res.status(404).json({ detail: "Row not found" }); + const { data: srcData } = await db + .from("tabular_review_row_sources") + .select("document_id, sort_index") + .eq("row_id", row_id) + .order("sort_index", { ascending: true }); + let srcIds = ((srcData ?? []) as { document_id: string }[]).map( + (s) => s.document_id, + ); + if (!srcIds.length && row.document_id) srcIds = [row.document_id]; + const allowed = new Set( + await filterAccessibleDocumentIds( + srcIds, + userId, + userEmail, + db, + ), + ); + const filtered = srcIds.filter((id) => allowed.has(id)); + if (filtered.length === 0) + return void res + .status(404) + .json({ detail: "Document not found" }); + const { data: docData } = await db + .from("documents") + .select("id, current_version_id") + .in("id", filtered); + const docRows = (docData ?? []) as { + id: string; + current_version_id?: string | null; + }[]; + await attachActiveVersionPaths(db, docRows); + sources = docRows as unknown as EnrichedDoc[]; + label = row.label?.trim() || sources[0]?.filename || label; + } else if (document_id) { + const docAllowed = await filterAccessibleDocumentIds( + [document_id], + userId, + userEmail, + db, + ); + if (docAllowed.length === 0) + return void res + .status(404) + .json({ detail: "Document not found" }); + const { data: doc } = await db + .from("documents") + .select("id, current_version_id") + .eq("id", document_id) + .single(); + if (!doc) + return void res + .status(404) + .json({ detail: "Document not found" }); + const docActive = await loadActiveVersion(document_id, db); + if (docActive) { + sources = [ + { + id: document_id, + filename: + docActive.filename?.trim() || "Untitled document", + storage_path: docActive.storage_path, + file_type: docActive.file_type, + }, + ]; + label = docActive.filename?.trim() || label; + } + } const { tabular_model, api_keys } = await getUserModelSettings( userId, @@ -915,34 +1192,26 @@ tabularRouter.post( }); } - await db - .from("tabular_cells") - .update({ status: "generating", content: null }) - .eq("review_id", reviewId) - .eq("document_id", document_id) - .eq("column_index", column_index); - - let markdown = ""; - if (docActive) { - const buf = await downloadFile(docActive.storage_path); - if (buf) { - try { - markdown = await extractDocumentMarkdown( - buf, - docActive.file_type, - ); - } catch (err) { - console.error( - `[regenerate-cell] extraction error doc=${document_id}`, - err, - ); - } - } - } + // The row_id / document_id branch selects the same one cell either way. + const applyTarget = T }>( + q: T, + ): T => + row_id + ? q.eq("row_id", row_id) + : q.eq("document_id", document_id as string); + + await applyTarget( + db + .from("tabular_cells") + .update({ status: "generating", content: null }) + .eq("review_id", reviewId), + ).eq("column_index", column_index); + + const markdown = await buildRowMarkdown(sources); const result = await queryTabularCell( tabular_model, - docActive?.filename?.trim() || "Untitled document", + label, markdown, column.prompt, column.format, @@ -951,21 +1220,21 @@ tabularRouter.post( ); if (!result) { - await db - .from("tabular_cells") - .update({ status: "error" }) - .eq("review_id", reviewId) - .eq("document_id", document_id) - .eq("column_index", column_index); + await applyTarget( + db + .from("tabular_cells") + .update({ status: "error" }) + .eq("review_id", reviewId), + ).eq("column_index", column_index); return void res.status(500).json({ detail: "Generation failed" }); } - await db - .from("tabular_cells") - .update({ content: JSON.stringify(result), status: "done" }) - .eq("review_id", reviewId) - .eq("document_id", document_id) - .eq("column_index", column_index); + await applyTarget( + db + .from("tabular_cells") + .update({ content: JSON.stringify(result), status: "done" }) + .eq("review_id", reviewId), + ).eq("column_index", column_index); res.json(result); }, @@ -1003,40 +1272,94 @@ tabularRouter.post("/:reviewId/generate", requireAuth, async (req, res) => { .from("tabular_cells") .select("*") .eq("review_id", reviewId); + // Generation runs per ROW, not per document. A folder row groups several + // source documents into one cell, and its cells carry `document_id: null` + // + a `row_id` — so keying anything on document_id would silently drop + // folder cells (they would stay "pending" forever). Every cell has a + // row_id after the folder-rows migration, so key the lookup on it. const cellMap = new Map>(); - for (const cell of cells ?? []) - cellMap.set(`${cell.document_id}:${cell.column_index}`, cell); + for (const cell of cells ?? []) { + const key = (cell.row_id ?? cell.document_id) as string | null; + if (key) cellMap.set(`${key}:${cell.column_index}`, cell); + } + + // Build the units to generate. Prefer the persisted rows (document + folder + // rows); fall back to the legacy document-derived path only when a review + // predates rows entirely, so old data still generates. + const rows = await loadReviewRows(db, reviewId); + type GenUnit = { + rowId: string | null; + documentId: string | null; + label: string; + sourceDocIds: string[]; + }; + let units: GenUnit[]; + if (rows.length > 0) { + units = rows.map((row) => ({ + rowId: row.id, + documentId: row.document_id, + label: row.label, + sourceDocIds: row.source_document_ids ?? [], + })); + } else { + // Legacy fallback: one unit per document, keyed on document_id. + const legacyDocIds = [ + ...new Set((cells ?? []).map((c) => c.document_id as string)), + ].filter(Boolean); + if (legacyDocIds.length > 0) { + units = legacyDocIds.map((id) => ({ + rowId: null, + documentId: id, + label: id, + sourceDocIds: [id], + })); + } else if (review.project_id) { + const { data } = await db + .from("documents") + .select("id") + .eq("project_id", review.project_id) + .order("created_at", { ascending: true }); + units = ((data ?? []) as { id: string }[]).map((d) => ({ + rowId: null, + documentId: d.id, + label: d.id, + sourceDocIds: [d.id], + })); + } else { + units = []; + } + } - const docIds = [...new Set((cells ?? []).map((c) => c.document_id))]; + // Access-filter and enrich every source document once. attachActiveVersionPaths + // resolves storage_path/file_type/filename from the active version. + const allSourceIds = [ + ...new Set(units.flatMap((u) => u.sourceDocIds)), + ]; const allowedDocIds = new Set( - await filterAccessibleDocumentIds(docIds, userId, userEmail, db), + await filterAccessibleDocumentIds( + allSourceIds, + userId, + userEmail, + db, + ), ); - let docs: Record[] = []; - if (docIds.length > 0) { - const filteredIds = docIds.filter((id) => allowedDocIds.has(id)); - const { data } = - filteredIds.length > 0 - ? await db - .from("documents") - .select("id, current_version_id") - .in("id", filteredIds) - : { data: [] as Record[] }; - docs = data ?? []; - } else if (review.project_id) { + const filteredSourceIds = allSourceIds.filter((id) => + allowedDocIds.has(id), + ); + let enrichedDocs: EnrichedDoc[] = []; + if (filteredSourceIds.length > 0) { const { data } = await db .from("documents") .select("id, current_version_id") - .eq("project_id", review.project_id) - .order("created_at", { ascending: true }); - docs = data ?? []; - } - await attachActiveVersionPaths( - db, - docs as { + .in("id", filteredSourceIds); + const docRows = (data ?? []) as { id: string; current_version_id?: string | null; - }[], - ); + }[]; + await attachActiveVersionPaths(db, docRows); + enrichedDocs = docRows as unknown as EnrichedDoc[]; + } + const docById = new Map(enrichedDocs.map((d) => [d.id, d])); const { tabular_model, api_keys } = await getUserModelSettings(userId, db); const missingKey = missingModelApiKey(tabular_model, api_keys); @@ -1057,38 +1380,30 @@ tabularRouter.post("/:reviewId/generate", requireAuth, async (req, res) => { try { await Promise.all( - docs.map(async (doc) => { - const docId = doc.id as string; - let markdown = ""; - - const filename = - (typeof doc.filename === "string" && doc.filename.trim() - ? doc.filename.trim() - : "Untitled document"); - const storagePath = - typeof doc.storage_path === "string" ? doc.storage_path : ""; - const fileType = - typeof doc.file_type === "string" ? doc.file_type : ""; - if (storagePath) { - const buf = await downloadFile(storagePath); - if (buf) { - try { - markdown = await extractDocumentMarkdown( - buf, - fileType, - ); - } catch (err) { - console.error( - `[tabular/generate] extraction error doc=${docId}`, - err, - ); - } - } - } + units.map(async (unit) => { + // Which cells does this unit address? Row-based when we have a + // row_id (document + folder rows), else legacy document_id. + const cellKey = unit.rowId ?? unit.documentId; + if (!cellKey) return; + // A stable SSE identity the frontend can match on: row_id for + // grouped/row-based cells, plus document_id for backward compat. + const sseId = { + row_id: unit.rowId, + document_id: unit.documentId, + }; + + const sources = unit.sourceDocIds + .map((id) => docById.get(id)) + .filter((d): d is EnrichedDoc => !!d); + const label = + unit.label?.trim() || + sources[0]?.filename || + "Untitled document"; + const markdown = await buildRowMarkdown(sources); // Filter to only columns that need processing const columnsToProcess = columns.filter((col) => { - const cell = cellMap.get(`${docId}:${col.index}`); + const cell = cellMap.get(`${cellKey}:${col.index}`); return !(cell?.status === "done" && cell?.content); }); if (columnsToProcess.length === 0) return; @@ -1096,9 +1411,9 @@ tabularRouter.post("/:reviewId/generate", requireAuth, async (req, res) => { // Mark all as generating upfront for (const col of columnsToProcess) { write( - `data: ${JSON.stringify({ type: "cell_update", document_id: docId, column_index: col.index, content: null, status: "generating" })}\n\n`, + `data: ${JSON.stringify({ type: "cell_update", ...sseId, column_index: col.index, content: null, status: "generating" })}\n\n`, ); - const existingCell = cellMap.get(`${docId}:${col.index}`); + const existingCell = cellMap.get(`${cellKey}:${col.index}`); if (existingCell) { await db .from("tabular_cells") @@ -1107,7 +1422,8 @@ tabularRouter.post("/:reviewId/generate", requireAuth, async (req, res) => { } else { await db.from("tabular_cells").insert({ review_id: reviewId, - document_id: docId, + row_id: unit.rowId, + document_id: unit.documentId, column_index: col.index, status: "generating", }); @@ -1119,29 +1435,35 @@ tabularRouter.post("/:reviewId/generate", requireAuth, async (req, res) => { try { await queryTabularAllColumns( tabular_model, - filename, + label, markdown, columnsToProcess, async (columnIndex, result) => { receivedColumns.add(columnIndex); - await db + let doneUpdate = db .from("tabular_cells") .update({ content: JSON.stringify(result), status: "done", }) .eq("review_id", reviewId) - .eq("document_id", docId) .eq("column_index", columnIndex); + doneUpdate = unit.rowId + ? doneUpdate.eq("row_id", unit.rowId) + : doneUpdate.eq( + "document_id", + unit.documentId, + ); + await doneUpdate; write( - `data: ${JSON.stringify({ type: "cell_update", document_id: docId, column_index: columnIndex, content: result, status: "done" })}\n\n`, + `data: ${JSON.stringify({ type: "cell_update", ...sseId, column_index: columnIndex, content: result, status: "done" })}\n\n`, ); }, api_keys, ); } catch (err) { console.error( - `[tabular/generate] queryTabularAllColumns error doc=${docId}`, + `[tabular/generate] queryTabularAllColumns error row=${cellKey}`, safeErrorLog(err), ); } @@ -1149,14 +1471,17 @@ tabularRouter.post("/:reviewId/generate", requireAuth, async (req, res) => { // Mark any columns the LLM didn't return as error for (const col of columnsToProcess) { if (!receivedColumns.has(col.index)) { - await db + let errUpdate = db .from("tabular_cells") .update({ status: "error" }) .eq("review_id", reviewId) - .eq("document_id", docId) .eq("column_index", col.index); + errUpdate = unit.rowId + ? errUpdate.eq("row_id", unit.rowId) + : errUpdate.eq("document_id", unit.documentId); + await errUpdate; write( - `data: ${JSON.stringify({ type: "cell_update", document_id: docId, column_index: col.index, content: null, status: "error" })}\n\n`, + `data: ${JSON.stringify({ type: "cell_update", ...sseId, column_index: col.index, content: null, status: "error" })}\n\n`, ); } } From 00a77254175b08e69f0421690027cf287d1407f6 Mon Sep 17 00:00:00 2001 From: Amal Date: Sat, 1 Aug 2026 11:07:20 -0700 Subject: [PATCH 3/3] fix(tabular-ui): render folder rows and fix the string|null tsc breaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WHY THIS MATTERS Two problems shipped together. (1) The PR widened TabularCell.document_id to `string | null`, which broke three call sites in TabularReviewView (TS2345) — frontend `tsc` exited 1 and, because next.config doesn't ignore build errors, `next build` failed outright. (2) The table rendered from `documents[]` and matched cells by document_id, so folder rows (document_id NULL) never appeared and their cells never populated even once the backend generated them. This makes the client render and drive reviews by row. WHAT IS THE ROW/CELL MATCH RULE A cell belongs to a row by row_id first (folder cells have no document_id), falling back to document_id so optimistic and legacy cells still line up. cellMatchesRow() centralises this predicate and every mutation path — the table, the generate SSE handler, regenerate, delete/clear — uses it, so what renders and what updates can never diverge. HOW IT WORKS - TRTable takes a `rows: TableRow[]` prop and renders one row each. Folder rows show their label with no per-row checkbox (no single document to select); document rows behave exactly as before. getCell() uses the match rule above. - TabularReviewView loads the `rows` payload into state and derives `tableRows` (falling back to one-row-per-document for pre-rows reviews). The generate SSE handler matches cell_update by row_id (with document_id fallback); regenerate sends row_id for folder cells; add/delete-documents keep the optimistic row list in sync; the detail side panel falls back to a folder row's first source document so citations still have a document to show (a documented v1 limitation). - regenerateTabularCell() sends { row_id } for folder cells, else { document_id } — matching the backend's either-identifier contract. - The three TS2345 sites are null-guarded (a NULL document_id is simply never "included" in a document-id selection), turning tsc — and the production build — green. - Remove the dead `document_grouping` field from updateTabularReview's payload type: the PATCH endpoint never reads it (grouping is fixed at create time), so advertising it in the client was misleading. TESTS - TRTable renders a folder row and matches its row_id-keyed, document_id-NULL cell (proving the render fix). - NewTRModal passes document_grouping "folder" when the subfolder checkbox is ticked and "document" when left unticked. - frontend `tsc --noEmit` and `next build` both pass; full vitest suite green. Co-Authored-By: Claude Opus 4.8 --- .../components/tabular/NewTRModal.test.tsx | 82 +++++++++ .../app/components/tabular/TRTable.test.tsx | 63 ++++++- .../src/app/components/tabular/TRTable.tsx | 76 ++++++-- .../components/tabular/TabularReviewView.tsx | 166 ++++++++++++++---- frontend/src/app/lib/mikeApi.ts | 15 +- 5 files changed, 343 insertions(+), 59 deletions(-) create mode 100644 frontend/src/app/components/tabular/NewTRModal.test.tsx diff --git a/frontend/src/app/components/tabular/NewTRModal.test.tsx b/frontend/src/app/components/tabular/NewTRModal.test.tsx new file mode 100644 index 000000000..e4a3bd955 --- /dev/null +++ b/frontend/src/app/components/tabular/NewTRModal.test.tsx @@ -0,0 +1,82 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { describe, expect, it, vi, beforeEach } from "vitest"; +import { NewTRModal } from "./NewTRModal"; +import type { Document } from "../shared/types"; + +// The modal loads workflow templates on open; stub the API so the test is +// hermetic. No other network is touched in project mode. +vi.mock("@/app/lib/mikeApi", () => ({ + listWorkflows: vi.fn(async () => []), + getProject: vi.fn(async () => ({ documents: [] })), + uploadProjectDocument: vi.fn(), + uploadStandaloneDocument: vi.fn(), +})); + +const projectDocs = [ + { id: "d1", filename: "A.pdf", status: "ready" } as Document, + { id: "d2", filename: "B.pdf", status: "ready" } as Document, +]; + +describe("NewTRModal folder-grouping checkbox", () => { + beforeEach(() => vi.clearAllMocks()); + + it("passes document_grouping 'folder' when the subfolder checkbox is ticked", async () => { + const user = userEvent.setup(); + const onAdd = vi.fn(); + render( + , + ); + + // Step 1: title, then advance to the documents step. + await user.type( + screen.getByPlaceholderText(/review/i), + "Grouped review", + ); + await user.click(screen.getByRole("button", { name: "Next" })); + + // The grouping checkbox is only offered inside a project context. + const checkbox = await screen.findByRole("checkbox", { + name: /same project subfolder as one review row/i, + }); + expect(checkbox).not.toBeChecked(); + await user.click(checkbox); + expect(checkbox).toBeChecked(); + + await user.click(screen.getByRole("button", { name: "Create" })); + + await waitFor(() => expect(onAdd).toHaveBeenCalledTimes(1)); + // 5th argument is the document grouping. + expect(onAdd.mock.calls[0][4]).toBe("folder"); + }); + + it("defaults to 'document' grouping when the checkbox is left unticked", async () => { + const user = userEvent.setup(); + const onAdd = vi.fn(); + render( + , + ); + + await user.type( + screen.getByPlaceholderText(/review/i), + "Plain review", + ); + await user.click(screen.getByRole("button", { name: "Next" })); + await user.click(screen.getByRole("button", { name: "Create" })); + + await waitFor(() => expect(onAdd).toHaveBeenCalledTimes(1)); + expect(onAdd.mock.calls[0][4]).toBe("document"); + }); +}); diff --git a/frontend/src/app/components/tabular/TRTable.test.tsx b/frontend/src/app/components/tabular/TRTable.test.tsx index 7d13e7817..09c9084a9 100644 --- a/frontend/src/app/components/tabular/TRTable.test.tsx +++ b/frontend/src/app/components/tabular/TRTable.test.tsx @@ -1,17 +1,32 @@ import { render, screen } from "@testing-library/react"; import { describe, expect, it, vi } from "vitest"; import { TRTable } from "./TRTable"; -import type { Document } from "../shared/types"; +import type { TableRow } from "./TRTable"; +import type { ColumnConfig, Document, TabularCell } from "../shared/types"; const doc = { id: "doc-1", filename: "report.pdf" } as Document; -function renderTable() { +const documentRow: TableRow = { + id: "row-1", + label: "report.pdf", + documentId: "doc-1", + rowId: "row-1", + rowType: "document", +}; + +function renderTable(overrides?: { + rows?: TableRow[]; + documents?: Document[]; + columns?: ColumnConfig[]; + cells?: TabularCell[]; +}) { return render( { // One select-all checkbox in the header plus one per document row. expect(screen.getAllByRole("checkbox")).toHaveLength(2); }); + + it("renders folder rows and matches their cells by row_id", () => { + const folderRow: TableRow = { + id: "row-folder", + label: "Contracts", + documentId: null, + rowId: "row-folder", + rowType: "folder", + }; + const columns: ColumnConfig[] = [ + { index: 0, name: "Party", prompt: "p" } as ColumnConfig, + ]; + // A folder cell carries a row_id and NO document_id — it must still + // render because the table matches cells to rows by row_id. + const folderCell = { + id: "cell-folder", + review_id: "r1", + row_id: "row-folder", + document_id: null, + column_index: 0, + content: { summary: "Acme Corp" }, + status: "done", + created_at: "now", + } as TabularCell; + + renderTable({ + rows: [folderRow], + documents: [doc], + columns, + cells: [folderCell], + }); + + expect(screen.getByText("Contracts")).toBeInTheDocument(); + expect(screen.getByText("Acme Corp")).toBeInTheDocument(); + // A folder row has no single document, so only the header select-all + // checkbox is present — no per-row checkbox for the folder row. + expect(screen.getAllByRole("checkbox")).toHaveLength(1); + }); }); diff --git a/frontend/src/app/components/tabular/TRTable.tsx b/frontend/src/app/components/tabular/TRTable.tsx index 1da4b263d..90cb90178 100644 --- a/frontend/src/app/components/tabular/TRTable.tsx +++ b/frontend/src/app/components/tabular/TRTable.tsx @@ -45,10 +45,23 @@ export interface TRTableHandle { scrollToCell: (colIdx: number, rowIdx: number) => void; } +// One rendered table row. A folder-grouped review turns several documents into +// a single folder row (rowType "folder", documentId null); a document review +// renders one row per document. `rowId` is the persisted tabular_review_rows id +// (null only in the legacy fallback where a review predates rows entirely). +export interface TableRow { + id: string; + label: string; + documentId: string | null; + rowId: string | null; + rowType: "document" | "folder"; +} + interface Props { loading: boolean; columns: ColumnConfig[]; documents: Document[]; + rows: TableRow[]; cells: TabularCell[]; savingColumn: boolean; savingColumnsConfig: boolean; @@ -77,6 +90,7 @@ export const TRTable = forwardRef(function TRTable( loading, columns, documents, + rows, cells, savingColumn, savingColumnsConfig, @@ -140,23 +154,35 @@ export const TRTable = forwardRef(function TRTable( }, })); - function getCell(docId: string, colIdx: number) { + // Match a cell to a row by row_id first (folder cells have no document_id), + // falling back to document_id so optimistic/legacy cells still render. + function getCell(row: TableRow, colIdx: number) { return cells.find( - (c) => c.document_id === docId && c.column_index === colIdx, + (c) => + c.column_index === colIdx && + ((row.rowId != null && c.row_id === row.rowId) || + (row.documentId != null && + c.document_id === row.documentId)), ); } + // Bulk selection + actions operate on document ids; folder rows have no + // single document, so only document rows are individually selectable. + const selectableDocIds = rows + .map((r) => r.documentId) + .filter((id): id is string => !!id); const allSelected = - documents.length > 0 && - documents.every((d) => selectedDocIds.includes(d.id)); + selectableDocIds.length > 0 && + selectableDocIds.every((id) => selectedDocIds.includes(id)); const someSelected = - !allSelected && documents.some((d) => selectedDocIds.includes(d.id)); + !allSelected && + selectableDocIds.some((id) => selectedDocIds.includes(id)); function toggleAll() { if (allSelected) { onSelectionChange([]); } else { - onSelectionChange(documents.map((d) => d.id)); + onSelectionChange(selectableDocIds); } } @@ -362,8 +388,10 @@ export const TRTable = forwardRef(function TRTable(
))} - {documents.map((doc, docIdx) => { - const isSelected = selectedDocIds.includes(doc.id); + {rows.map((row, docIdx) => { + const isSelected = + row.documentId != null && + selectedDocIds.includes(row.documentId); const rowBg = isSelected ? APP_SURFACE_ACTIVE_CLASS : APP_SURFACE_HOVER_CLASS; @@ -372,28 +400,40 @@ export const TRTable = forwardRef(function TRTable( : TR_STICKY_CELL_BG; return (
- toggleDoc(doc.id)} - className={TABLE_CHECKBOX_CLASS} - /> + {row.documentId != null ? ( + + toggleDoc(row.documentId as string) + } + className={TABLE_CHECKBOX_CLASS} + /> + ) : ( + // Folder row: no single document to select. + + )} - {doc.filename} + {row.label}
{columns.map((col) => { - const cell = getCell(doc.id, col.index); + const cell = getCell(row, col.index); const colPos = sortedColumns.findIndex( (c) => c.index === col.index, ); diff --git a/frontend/src/app/components/tabular/TabularReviewView.tsx b/frontend/src/app/components/tabular/TabularReviewView.tsx index 760fd1576..dc6bbb18b 100644 --- a/frontend/src/app/components/tabular/TabularReviewView.tsx +++ b/frontend/src/app/components/tabular/TabularReviewView.tsx @@ -36,6 +36,7 @@ import type { Project, TabularCell, TabularReview, + TabularReviewRow, Workflow, } from "../shared/types"; import { AddColumnModal } from "./AddColumnModal"; @@ -56,7 +57,7 @@ import { } from "@/app/lib/modelAvailability"; import { TRSidePanel } from "./TRSidePanel"; import { TRTable } from "./TRTable"; -import type { TRTableHandle } from "./TRTable"; +import type { TRTableHandle, TableRow } from "./TRTable"; import { TRChatPanel } from "./TRChatPanel"; import { TabularReviewDetailsModal } from "./TabularReviewDetailsModal"; import { exportTabularReviewToExcel } from "./exportToExcel"; @@ -70,12 +71,26 @@ interface Props { projectId?: string; } +// Match a cell to a row: row_id first (folder cells carry no document_id), then +// document_id so optimistic/legacy cells still line up. Shared by every cell +// mutation path so folder-grouped reviews update the same cells they render. +function cellMatchesRow( + cell: Pick, + row: { rowId: string | null; documentId: string | null }, +): boolean { + return ( + (row.rowId != null && cell.row_id === row.rowId) || + (row.documentId != null && cell.document_id === row.documentId) + ); +} + export function TRView({ reviewId, projectId }: Props) { const { setSidebarOpen } = useSidebar(); const [review, setReview] = useState(null); const [project, setProject] = useState(null); const [cells, setCells] = useState([]); const [documents, setDocuments] = useState([]); + const [reviewRows, setReviewRows] = useState([]); const [columns, setColumns] = useState([]); const [loading, setLoading] = useState(true); const [generating, setGenerating] = useState(false); @@ -133,6 +148,26 @@ export function TRView({ reviewId, projectId }: Props) { const apiKeys = profile?.apiKeys; const tabularModel = profile?.tabularModel ?? "gemini-3-flash-preview"; + // One rendered row per persisted review row (document or folder). Folder + // rows group several documents into a single row (documentId null). Reviews + // created before the folder-rows feature have no rows payload, so fall back + // to a one-row-per-document view. + const tableRows: TableRow[] = reviewRows.length + ? reviewRows.map((r) => ({ + id: r.id, + label: r.label, + documentId: r.document_id, + rowId: r.id, + rowType: r.row_type, + })) + : documents.map((d) => ({ + id: d.id, + label: d.filename, + documentId: d.id, + rowId: null, + rowType: "document" as const, + })); + useEffect(() => { const params = new URLSearchParams(window.location.search); if (chatOpen) { @@ -161,12 +196,15 @@ export function TRView({ reviewId, projectId }: Props) { useEffect(() => { const fetches: Promise[] = [ - getTabularReview(reviewId).then(({ review, cells, documents }) => { - setReview(review); - setCells(cells); - setDocuments(documents); - setColumns(review.columns_config || []); - }), + getTabularReview(reviewId).then( + ({ review, cells, documents, rows }) => { + setReview(review); + setCells(cells); + setDocuments(documents); + setReviewRows(rows ?? []); + setColumns(review.columns_config || []); + }, + ), ]; if (projectId) { fetches.push( @@ -219,6 +257,27 @@ export function TRView({ reviewId, projectId }: Props) { columns_config: columns, }); setDocuments((prev) => [...prev, ...toAdd]); + // The table renders from rows; when a review already has a rows payload, + // optimistically append a document row per added doc so it shows up + // before the next reload reconciles the server-assigned row ids. (A + // legacy review with no rows renders straight from `documents`.) + setReviewRows((prev) => + prev.length === 0 + ? prev + : [ + ...prev, + ...toAdd.map((doc, i) => ({ + id: doc.id, + review_id: reviewId, + label: doc.filename, + row_type: "document" as const, + folder_id: null, + document_id: doc.id, + sort_index: prev.length + i, + source_document_ids: [doc.id], + })), + ], + ); if (columns.length > 0) { setCells((prev) => [ ...prev, @@ -264,15 +323,26 @@ export function TRView({ reviewId, projectId }: Props) { } } - async function handleRegenerateCell(docId: string, colIndex: number) { + async function handleRegenerateCell(cell: TabularCell) { if (apiKeys && !isModelAvailable(tabularModel, apiKeys)) { setApiKeyModalProvider(getModelProvider(tabularModel)); return; } + const { row_id: rowId, document_id: docId, column_index: colIndex } = + cell; + // Identify this one cell by row_id (folder cells have no document_id) + // or document_id — the same predicate the table uses to render it. + const matches = (c: TabularCell) => + c.column_index === colIndex && + cellMatchesRow(c, { + rowId: rowId ?? null, + documentId: docId, + }); + setCells((prev) => prev.map((c) => - c.document_id === docId && c.column_index === colIndex + matches(c) ? { ...c, status: "generating" as const, content: null } : c, ), @@ -287,10 +357,11 @@ export function TRView({ reviewId, projectId }: Props) { reviewId, docId, colIndex, + rowId, ); setCells((prev) => prev.map((c) => - c.document_id === docId && c.column_index === colIndex + matches(c) ? { ...c, status: "done" as const, content: result } : c, ), @@ -304,9 +375,7 @@ export function TRView({ reviewId, projectId }: Props) { console.error("Regeneration failed", err); setCells((prev) => prev.map((c) => - c.document_id === docId && c.column_index === colIndex - ? { ...c, status: "error" as const } - : c, + matches(c) ? { ...c, status: "error" as const } : c, ), ); setExpandedCell((prev) => @@ -346,14 +415,16 @@ export function TRView({ reviewId, projectId }: Props) { } if (!response.body) throw new Error("No body"); - // Optimistically set empty/pending/error cells to generating (skip done cells) + // Optimistically set empty/pending/error cells to generating (skip + // done cells). Iterate rows, not documents, so folder-grouped cells + // (document_id null, keyed by row_id) get marked too. setCells((prev) => - documents.flatMap((doc) => + tableRows.flatMap((row) => columns.map((col) => { const existing = prev.find( (c) => - c.document_id === doc.id && - c.column_index === col.index, + c.column_index === col.index && + cellMatchesRow(c, row), ); if (existing?.status === "done" && existing?.content) { return existing; @@ -365,9 +436,10 @@ export function TRView({ reviewId, projectId }: Props) { content: null, } : { - id: `${doc.id}-${col.index}`, + id: `${row.id}-${col.index}`, review_id: reviewId, - document_id: doc.id, + row_id: row.rowId, + document_id: row.documentId, column_index: col.index, content: null, status: "generating" as const, @@ -395,10 +467,16 @@ export function TRView({ reviewId, projectId }: Props) { try { const data = JSON.parse(dataStr); if (data.type === "cell_update") { + // Backend emits row_id (+ document_id for legacy + // compatibility). Prefer row_id so folder cells + // update; fall back to document_id otherwise. setCells((prev) => prev.map((c) => - c.document_id === data.document_id && - c.column_index === data.column_index + c.column_index === data.column_index && + cellMatchesRow(c, { + rowId: data.row_id ?? null, + documentId: data.document_id ?? null, + }) ? { ...c, content: data.content, @@ -521,10 +599,22 @@ export function TRView({ reviewId, projectId }: Props) { if (idsToDelete.length === 0) return; const previousDocuments = documents; const previousCells = cells; + const previousRows = reviewRows; const remaining = documents.filter((d) => !idsToDelete.includes(d.id)); setDocuments(remaining); + setReviewRows((prev) => + prev.filter( + (r) => + r.document_id == null || + !idsToDelete.includes(r.document_id), + ), + ); setCells((prev) => - prev.filter((c) => !idsToDelete.includes(c.document_id)), + prev.filter( + (c) => + c.document_id == null || + !idsToDelete.includes(c.document_id), + ), ); setSelectedDocIds([]); setActionsOpen(false); @@ -535,6 +625,7 @@ export function TRView({ reviewId, projectId }: Props) { }); } catch (err) { setDocuments(previousDocuments); + setReviewRows(previousRows); setCells(previousCells); setSelectedDocIds(idsToDelete); console.error("Failed to delete tabular review documents", err); @@ -545,7 +636,7 @@ export function TRView({ reviewId, projectId }: Props) { if (docIds.length === 0) return; setCells((prev) => prev.map((c) => - docIds.includes(c.document_id) + c.document_id != null && docIds.includes(c.document_id) ? { ...c, content: null, status: "pending" } : c, ), @@ -674,6 +765,9 @@ export function TRView({ reviewId, projectId }: Props) { const filteredDocuments = q ? documents.filter((d) => d.filename.toLowerCase().includes(q)) : documents; + const filteredTableRows = q + ? tableRows.filter((r) => r.label.toLowerCase().includes(q)) + : tableRows; return (
@@ -975,6 +1069,7 @@ export function TRView({ reviewId, projectId }: Props) { loading={loading} columns={columns} documents={filteredDocuments} + rows={filteredTableRows} cells={cells} highlightedCell={highlightedCell} savingColumn={savingColumn} @@ -1033,9 +1128,23 @@ export function TRView({ reviewId, projectId }: Props) { {/* Cell detail side panel */} {expandedCell && (() => { - const expandedDoc = documents.find( - (d) => d.id === expandedCell.document_id, - ); + // Folder cells have no document_id — fall back to the row's + // first source document so the citation/PDF panel still has + // a document to show (a documented v1 limitation: a folder + // cell surfaces one representative source). + const expandedDoc = + documents.find( + (d) => d.id === expandedCell.document_id, + ) ?? + (() => { + const row = reviewRows.find( + (r) => r.id === expandedCell.row_id, + ); + const firstSrc = row?.source_document_ids?.[0]; + return firstSrc + ? documents.find((d) => d.id === firstSrc) + : undefined; + })(); const expandedCol = columns.find( (c) => c.index === expandedCell.column_index, ); @@ -1064,10 +1173,7 @@ export function TRView({ reviewId, projectId }: Props) { } }} onRegenerate={() => - handleRegenerateCell( - expandedCell.document_id, - expandedCell.column_index, - ) + handleRegenerateCell(expandedCell) } displayDocument={expandedCellCitation !== undefined} citationQuote={expandedCellCitation?.quote} diff --git a/frontend/src/app/lib/mikeApi.ts b/frontend/src/app/lib/mikeApi.ts index 5c64fc280..5d5c7e88d 100644 --- a/frontend/src/app/lib/mikeApi.ts +++ b/frontend/src/app/lib/mikeApi.ts @@ -1080,7 +1080,6 @@ export async function updateTabularReview( columns_config?: { index: number; name: string; prompt: string }[]; document_ids?: string[]; project_id?: string | null; - document_grouping?: "document" | "folder"; shared_with?: string[]; }, ): Promise { @@ -1266,20 +1265,24 @@ export async function renameTabularChat( export async function regenerateTabularCell( reviewId: string, - documentId: string, + documentId: string | null, columnIndex: number, + rowId?: string | null, ): Promise<{ summary: string; flag: "green" | "grey" | "yellow" | "red"; reasoning: string; }> { + // A folder cell has no document_id — address it by row_id instead. The + // backend accepts either identifier and resolves the same cell. return apiRequest(`/tabular-review/${reviewId}/regenerate-cell`, { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ - document_id: documentId, - column_index: columnIndex, - }), + body: JSON.stringify( + rowId + ? { row_id: rowId, column_index: columnIndex } + : { document_id: documentId, column_index: columnIndex }, + ), }); }