Skip to content

feat: add folder-grouped tabular review rows - #274

Merged
willchen96 merged 4 commits into
Open-Legal-Products:mainfrom
ecarjat:codex/folder-grouped-tabular-reviews
Aug 3, 2026
Merged

feat: add folder-grouped tabular review rows#274
willchen96 merged 4 commits into
Open-Legal-Products:mainfrom
ecarjat:codex/folder-grouped-tabular-reviews

Conversation

@ecarjat

@ecarjat ecarjat commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Focused replacement for #54, rebased on the current main branch.

Changes

  • Adds an optional folder grouping mode when creating a project tabular review.
  • Persists review rows and their source documents, while preserving document-level rows for files outside a subfolder.
  • Adds the schema migration, fresh-schema support, and backend-owned table access hardening.

Why

Lets a tabular review process all documents in a project subfolder as one logical row without coupling the change to the separate page-limit or PDF.js cleanup work from #54.

Testing

  • npm test --prefix frontend
  • npm test --prefix backend
  • npm run build --prefix backend

@CLAassistant

CLAassistant commented Jul 29, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@ecarjat
ecarjat force-pushed the codex/folder-grouped-tabular-reviews branch from 95d040e to 5f996cf Compare July 29, 2026 16:59
@ecarjat
ecarjat marked this pull request as ready for review July 29, 2026 17:00
@amal66

amal66 commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Thanks for splitting this out of #54. The idempotent migration with a legacy backfill, the create-path rollback, and the revoke + RLS on the two new tables — matching, even exceeding, the repo's backend-owned-table pattern — are genuinely good work.

The core problem is that the feature currently ends at the database. Must-fix:

  • Folder rows are write-only. Cells are inserted with document_id: null, but /generate, /regenerate-cell, and the table UI all key on document_id, so folder cells stay pending forever and the checkbox ships a broken state.
  • Frontend tsc --noEmit fails with three errors from widening TabularCell.document_id to string | null, which breaks next build.
  • Rebase: schema.sql conflicts with main's pagination RPC rework — the resolution must keep main's new functions — and the migration is dated before ones already shipped on main, so operators would skip it; rename it to a later date.
  • PATCH /:reviewId edits cells by document_id only and never maintains the new rows/sources tables, so folder reviews desync on edit.

Smaller: several helpers ignore Supabase errors (a transient failure yields a silently empty review), and adding the two new tables to the stack test's PUBLIC_TABLES would enforce the hardening claim in CI.

To make this easy to act on I've opened ecarjat#1 against your branch: row-based generation and rendering wired end to end, the tsc errors and migration date fixed, tests added — merge it or cherry-pick. It sits directly on your branch as three focused commits; the rebase onto main (including the schema.sql resolution) is left to you. On the result, both typechecks are clean and both suites pass (backend 268, frontend 41).

@amal66 amal66 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Details in my comment below — the schema work is solid, but generation/rendering aren't wired to the new rows yet, the frontend typecheck fails, and the migration needs re-dating. There's a follow-up PR against your branch implementing the fixes.

@willchen96

Copy link
Copy Markdown
Collaborator

Pushed follow-up commit 6ae1f98 with the completed folder-grouped tabular review experience.

What changed:

  • Added folder-based review rows for project folders and nested library folders, while retaining individual rows for ungrouped documents.
  • Moved document grouping to the first creation step and replaced the checkbox with the shared toggle primitive.
  • Updated the file picker to render nested folders consistently across Files and Projects; the tabular-review picker no longer shows Templates.
  • Updated the first column to Folder / Document with the appropriate folder or file-type icon. Folder rows expand to show their source documents without divider lines.
  • Made grouped results and the side panel expandable so users can inspect and open each underlying document.
  • Added document IDs to citations so citations from grouped rows open the correct source document.
  • Updated exports, table navigation, chat context, API types, migrations, schema, and access handling for logical review rows.
  • Removed the previous 120,000-character aggregate truncation so later documents in a grouped folder are not silently excluded from extraction.
  • Added focused frontend and backend coverage for folder rows, nested directories, citations, document switching, and the toggle primitive.

Validation completed: backend TypeScript build, 33 tabular route tests, focused frontend tests, frontend type-checking, ESLint, and diff checks.

@willchen96
willchen96 merged commit 5f78bb2 into Open-Legal-Products:main Aug 3, 2026
7 of 8 checks passed
willchen96 pushed a commit that referenced this pull request Aug 3, 2026
…with lib tests

WHY THIS MATTERS

Main's "Frontend build and tests" job has been red for everyone:
statements 52.14% vs the 54% floor and branches 62.57% vs 73%. A
coverage ratchet only works if regressions are paid down with tests —
if we lower the floor instead, the ratchet becomes a decoration and
every future untested merge quietly erodes the suite. This commit
restores green by testing the code that caused the drop, then re-arms
the ratchet at the new level.

WHAT HAPPENED

The ratchet floors were measured in #255 (2266446) before three merges
landed untested code inside the gated scope (src/app/lib/**):

- #263 (db-pagination) + #274 (folder-grouped tabular reviews) grew
  mikeApi.ts's listTabularReviews/listTabularReviewIds into
  query-string builders with seven conditional params each, added the
  document_grouping field, the uploadReviewDocument orchestration, and
  the tabular chat/cell endpoints — nearly all unexercised. Lines
  1044-1456 were the bulk of the uncovered report.
- #280 (workflow slash triggers) leans on the workflow endpoints
  (listWorkflows feeds the slash menu), which were also untested.

Because coverage is a global percentage over the gated files, adding
untested statements/branches anywhere in scope dilutes the totals even
though no tested line got worse.

HOW THE FIX WORKS

Extend the existing mikeApi.test.ts fetch/session mocking pattern to
the regressed surface, asserting behavior (URLs, methods, exact
payloads, error contracts), not just execution:

- listTabularReviews/listTabularReviewIds: every pagination knob
  serialized under its snake_case name, scope="all" omitted (backend
  default), abort signals forwarded, and the ids query scoped
  identically to the list query — the invariant that keeps
  select-all-then-delete from deleting reviews the user cannot see.
- createTabularReview/updateTabularReview: document_grouping (the #274
  field) passes through unchanged; PATCH sends only the given fields.
- uploadReviewDocument: project vs standalone upload routing, and that
  the follow-up PATCH appends to existing document_ids instead of
  replacing them (the review-shrinking failure mode).
- Multipart uploads: FormData with auth header only (a manual JSON
  content type would break the boundary), optional filename field, and
  the plain-Error-with-response-text failure contract.
- Tabular chats/cells, workflow list/hide/unhide, query and payload
  defaults (getDocumentUrl version param, createChat "{}" body,
  parent_folder_id null-vs-undefined, empty error bodies).
- supabase.ts: importing without env vars fails loudly at module load —
  the desired crash-at-startup behavior for a misconfigured build.
- deleteTabularReviewsWithConcurrency: empty input short-circuits;
  concurrency<=0 clamps to one worker instead of silently deleting
  nothing.
- utils.diceCoefficient: sub-bigram inputs score 0.

Coverage moves from 52.14/62.57/34.67/52.41 (stmts/branch/funcs/lines)
to 81.18/98.24/55.64/79.18. Per the ratchet's own rule ("floors only go
up: when you add tests, raise them in the same PR"), the floors move to
79/96/53/77 — about two points under the new measurement, so one small
innocent addition doesn't instantly re-redden main, while a real drop
still fails CI.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
willchen96 added a commit that referenced this pull request Aug 3, 2026
test(frontend): fix the red main — pay down the coverage regression from #274/#263/#280
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants