feat(api): expand v2 tables with stateless multipart transfers - #6188
Conversation
Adds 16 operations so a v2 caller can do what the internal surface can: rename/move/lock a table, restore it, manage saved views, run enrichment columns, look up rows, and import/export with observable job control. Extracts lib/table/orchestration/import.ts (performTableCsvImport, performCreateTableFromCsv) and lib/table/export-stream.ts from the first-party routes, then repoints those routes at them, so v1 and v2 cannot drift on what an import or export actually does. events/stream, metadata and dispatches stay internal — they are editor state, not public API.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview Files: The buffered Tables: List/create examples and the The tables spec also grows the components section (predicates, views, workflow groups, job envelopes, etc.) and adds Conflict, Locked, PayloadTooLarge, and Gone response refs. Reviewed by Cursor Bugbot for commit 739bd6d. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThe PR substantially expands the public v2 Tables API and introduces shared stateless multipart transfer contracts for Files and table imports.
Confidence Score: 5/5The PR appears safe to merge with respect to the previously reported issues. No blocking failure remains: deterministic PATCH rejections occur before writes, and every post-write failure path now reports the operations that were applied.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/v2/tables/[tableId]/route.ts | The PATCH route now prevalidates deterministic failures, reports committed operations on every post-write error path, and signals schema changes after partial success. |
| apps/sim/lib/api/contracts/v2/tables.ts | Defines the expanded v2 Tables boundary contracts and strict update validation used by the routes. |
| apps/sim/lib/uploads/multipart-session/service.ts | Implements the shared stateless multipart transfer lifecycle used by file uploads and table imports. |
| packages/db/schema.ts | Aligns persistent schema with stateless transfer state while retaining the larger file-size field. |
| packages/db/migrations/0280_smart_la_nuit.sql | Applies the additive workspace-file size migration required by the revised upload limits. |
Reviews (12): Last reviewed commit: "fix(api): make table import completion r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ry 423 Greptile P1: PATCH applied locks, rename and move as three sequential transactions, so a folder rejected mid-request left the earlier writes persisted while the response reported failure — and the schema-changed signal was skipped, leaving open clients on stale state. Every rejectable condition now runs before the first write, and the signal fires whenever anything did land. Cursor: v2TableLockError dropped the lock kind, so async import, column run, enrichment and table mutations returned a bare LOCKED. A table has four independent locks, so the caller could not tell which to clear.
|
@cursor review |
…n ones
The previous commit named the lock only where the rejection was thrown and
caught at the route boundary. Where it instead arrives as a classified
`errorCode: 'locked'` outcome — delete table, delete row, update column,
and the table mutations — the kind was dropped, so those 423s stayed
unactionable while their neighbours improved.
The orchestration results now carry `lock`, and a shared
`v2TableOrchestrationError` renders both arrival paths into the same
`{ code, message, details: { lock } }` body. `details` is omitted rather
than sent null when the kind is unknown, so a caller branching on it sees
absence instead of a phantom value.
|
@cursor review |
`POST /import-async` pointed callers at `GET /api/v2/tables/jobs` to track progress, but that endpoint filters to `type = 'export'` — imports are derived onto the table itself, one write job at a time, and exports get a separate list precisely because they are excluded from that derivation. The public Table shape omitted those derived fields, so an async import could be started and cancelled but never observed to completion, failure, or progress. That is the gap the import/export/job-control set was meant to close. Table now carries `job` — id, type, status, rowsProcessed, error, or null when idle — and the import-async docs point at the table rather than the export list.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 83e04cb. Configure here.
Greptile held the PR at 4/5 on the residual non-atomicity and named two acceptable resolutions: make PATCH atomic, or have the contract adopt and expose partial-success explicitly. Atomicity would mean threading one transaction through renameTable, moveTableToFolder and updateTableLocks — three shared service functions with four non-test callers including the first-party route and two copilot tools — and deferring their per-operation audits to commit time. That is a refactor of shared write paths well outside this PR. So the contract states it instead. Every rejectable condition is already pre-validated, so a failure here is a genuine fault; when one follows a successful operation the error now carries `details.applied` listing what is live. Absent when nothing applied, so its presence always means "these changes took effect despite the error". Documented on the operation. `v2ErrorForOrchestration` gained the optional `details` this needs.
|
Addressing the 4/5 hold on non-atomic PATCH (b25c426) — the finding is fair, and I took the second of the two resolutions you named. Why not atomicity. It would require one transaction spanning What landed instead. The endpoint now adopts partial-success explicitly rather than implying atomicity it does not have:
Two tests pin both halves: a move failing after a successful rename asserts This required giving Worth flagging for the human reviewer: the first-party |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b25c426. Configure here.
…eat/v2-tables-coverage # Conflicts: # scripts/check-api-validation-contracts.ts
The new PATCH /api/v2/tables/[tableId] accepted a `locks` object, gated on workspace admin plus the table-locks feature. That still lets an API key clear the guard placed there to stop it: `write` is the floor for the endpoint, and admin keys are ordinary API keys, so a lock is no longer a boundary the key cannot cross. Locks stay readable on the table resource and enforcement is unchanged (a locked verb still returns 423). Changing one is now a first-party admin action only. The v2 body is declared here rather than reusing the first-party updateTableBodySchema, which keeps its `locks` field so the UI can still toggle them. It is .strict(), so a request carrying `locks` is rejected with a 400 naming the field instead of silently succeeding without applying it.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 03e6ee9. Configure here.
Drops the three synchronous entry points: POST /tables/[tableId]/import, POST /tables/import-csv, and GET /tables/[tableId]/export. Sync import tied a write to the lifetime of an HTTP request. The body *was* the data, so it carried a 10 MB cap that Next silently truncates past — a partial import reporting success. It also had no job, so a timeout mid-write left rows in place with nothing to poll and nothing to cancel. The async path reads the file from storage instead: upload via POST /api/v2/files for a key, start with POST /import-async, watch GET /tables/[tableId] -> job, stop with POST /job/cancel. Sync export carried no such hazard, but one shape per operation beats two: with both removed the surface has exactly one way to move a table in or out, and the CLI wraps the extra calls. This also removes the last multipart handling in v2 tables. Those were the only routes bypassing parseRequest — form fields were parsed by hand against separate form schemas, outside the contract system every other v2 write goes through. Create-a-table-from-CSV is now two calls: POST /tables, then /import-async with createColumns. csvImportModeSchema is append|replace, so there is no single-call create. Route baseline 1064 -> 1061.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f6f5084. Configure here.
The docstring claimed there is no synchronous upload endpoint and so no request-body size cliff. Both are wrong: POST /api/v2/files is a synchronous multipart upload with a 100 MB cap, and it is the only v2 upload path (presigned is deliberately absent). What async-only actually bought: the cap went 10 MB -> 100 MB, it fails on an explicit size check and a bounded body read rather than a proxy cap that silently truncates, authorization completes before any body is buffered, and the table write is a job that can be watched and cancelled.
TheodoreSpeaks
changed the title
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6614484. Configure here.
TheodoreSpeaks
changed the title
TheodoreSpeaks
changed the title
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a36741f. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 739bd6d. Configure here.
1 participant
Summary
Type of Change
Testing
Checklist