Review fixes for #274: wire folder-row generation + frontend tsc + tests - #1
Open
amal66 wants to merge 3 commits into
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
amal66
force-pushed
the
review-fixes/pr274-folder-rows
branch
from
August 2, 2026 03:02
3008bf9 to
00a7725
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the review feedback from Open-Legal-Products#274. Folder grouping persisted rows, but nothing consumed them — this wires folder rows end to end:
/generate,/regenerate-cellandPATCHnow address cells byrow_id(folder cells havedocument_idNULL), the UI renders folder rows, the frontend typecheck is fixed, the migration is re-dated so upgrade-by-filename-order still applies it, and tests cover the new paths.7452d01— re-dates the folder-rows migration to20260801_01so it sorts after migrations already shipped on main, and orders the legacy-row backfill by filename to match the create path.0ac1a53— generation iterates rows instead of documents (folder rows concatenate their source documents' markdown, capped at 120k chars with per-file headings);regenerate-cellacceptsrow_id;PATCHreconciles the row tables so grouped reviews don't desync; Supabase read errors now throw instead of silently returning empty. Adds backend integration tests, including the folder-cell "stuck on pending" regression.00a7725— renders folder rows in the review table (folder label + source-document list,row_id-keyed cells and regeneration) and fixes the threestring | nulltsc errors that broketsc --noEmit.Verified on this branch: backend
tscclean, 268 backend tests pass (260 baseline + the new ones); frontendtsc --noEmitclean; 41 frontend tests pass.The branch is based directly on your
codex/folder-grouped-tabular-reviews— no merge of main. Rebasing onto current main is intentionally left to you, including resolvingschema.sqlandbackend/src/routes/tabular.tsagainst main's paginated-reviews RPC rework (d6b35c9).