Skip to content

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

Open
ecarjat wants to merge 1 commit into
Open-Legal-Products:mainfrom
ecarjat:codex/folder-grouped-tabular-reviews
Open

feat: add folder-grouped tabular review rows#274
ecarjat wants to merge 1 commit into
Open-Legal-Products:mainfrom
ecarjat:codex/folder-grouped-tabular-reviews

Conversation

@ecarjat

@ecarjat ecarjat commented Jul 29, 2026

Copy link
Copy Markdown

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.

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.

3 participants