refactor(tables): make lib/table/orchestration the single implementation - #6134
refactor(tables): make lib/table/orchestration the single implementation#6134TheodoreSpeaks wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@cursor review |
PR SummaryMedium Risk Overview Adds Routes shrink to auth → parse → orchestration call → map Fixes that come from consolidation: v2 column PATCH gets the same guards as other surfaces (e.g. unchanged type + options, unique on select); UI/copilot column edits get audits; duplicate route-level audits on table delete are removed; locked tables/rows return 423 instead of 500 or false 200. Reviewed by Cursor Bugbot for commit f3dd1fe. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR consolidates table column updates and table/row deletion behavior into shared orchestration functions.
Confidence Score: 4/5The PR does not yet appear safe to merge because table updates can remain partially committed after an error and delete audits lose request provenance. Column renames execute and commit before later required-state validation, so a rejected update can leave the new name persisted; successful HTTP table deletions also reach recordAudit without the request needed to retain client IP and user-agent data. Files Needing Attention: apps/sim/lib/table/orchestration/columns.ts, apps/sim/lib/table/orchestration/tables.ts, apps/sim/lib/table/service.ts
|
| Filename | Overview |
|---|---|
| apps/sim/lib/table/orchestration/columns.ts | Centralizes column mutation ordering, guards, option normalization, error classification, and auditing, but the previously reported partial-commit path through an early rename remains. |
| apps/sim/lib/table/orchestration/tables.ts | Centralizes table and row deletion, while the table-delete audit path still omits HTTP request provenance. |
| apps/sim/app/api/v2/lib/response.ts | Adds consistent v2 envelope translation for shared orchestration error classes. |
| apps/sim/lib/core/orchestration/types.ts | Moves the shared orchestration error contract into a resource-neutral module and adds locked-status mapping. |
| apps/sim/lib/table/select-options.ts | Centralizes normalization that preserves supplied or name-matched select-option identifiers. |
Sequence Diagram
sequenceDiagram
participant Caller as UI / v1 / v2 / Copilot
participant Route as Route or Tool Adapter
participant Orch as Table Orchestration
participant Service as Table Services
participant DB as PostgreSQL
Caller->>Route: Table mutation
Route->>Route: Authenticate and parse
Route->>Orch: Normalized mutation and actor
Orch->>Service: Guarded column/table/row operation
Service->>DB: Locked transaction
DB-->>Service: Updated table state
Service-->>Orch: Result
Orch-->>Route: Success or classified error
Route-->>Caller: Surface-specific response
Reviews (2): Last reviewed commit: "refactor(tables): make lib/table/orchest..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return v2Error('NOT_FOUND', 'Table not found') | ||
| } | ||
|
|
||
| await deleteTable(tableId, requestId) | ||
|
|
||
| recordAudit({ | ||
| workspaceId, | ||
| actorId: userId, | ||
| action: AuditAction.TABLE_DELETED, | ||
| resourceType: AuditResourceType.TABLE, | ||
| resourceId: tableId, | ||
| resourceName: result.table.name, | ||
| description: `Archived table "${result.table.name}"`, | ||
| request, | ||
| }) | ||
| // The actor makes the service audit the delete — and only when a row was | ||
| // actually archived. Auditing out here emitted TABLE_DELETED for a no-op | ||
| // delete of an already-archived table. | ||
| await deleteTable(tableId, requestId, userId) |
There was a problem hiding this comment.
Successful v1 and v2 table deletions now delegate auditing to deleteTable without passing the request, causing new TABLE_DELETED audit rows to omit the client IP address and user-agent that the previous route-level audit recorded.
Knowledge Base Used: User Tables (apps/sim/lib/table)
| ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}), | ||
| // Forwarded so the conversion validates against the constraint this | ||
| // same request is about to set, not the column's current one. | ||
| ...(updates.required !== undefined ? { required: updates.required } : {}), |
There was a problem hiding this comment.
When one request renames a column, changes its type, and sets required: true while existing cells are empty, the rename commits first and the forwarded required-state validation then rejects the type update, causing an error response with the rename still applied.
Knowledge Base Used: User Tables (apps/sim/lib/table)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…rkflows OrchestrationErrorCode and statusForOrchestrationError are the contract every lib/[resource]/orchestration module returns against, but they lived inside the workflows module, so resource-neutral code (lib/folders) already had to import from a workflow path. Moved to lib/core/orchestration/types. Adds a 'locked' class mapping to 423. Both tables and workflows have a lock that forbids a mutation, and each caller was translating that to a status itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Column update was implemented four times — the UI route, v1, v2, and the copilot table tool — each calling the same column services but owning its own guards, error mapping, and audit. The copies had drifted, and the drift was the bug: v2 was missing both guards, only the copilot copy minted stable option ids, and only v1/v2 audited. performUpdateTableColumn, performDeleteTable, and performDeleteTableRow now own that logic; all ten call sites reduce to auth, parse, call, render. The guards are asserted once in lib/table/orchestration rather than four times against four routes. Behavior this consolidates, previously true on only some paths: - The typeChanging guard. updateColumnType early-returns on an unchanged type and drops any options sent with it, so restating the current type alongside new options silently discarded them. v2 had no guard at all and, since its contract shares v1's body schema, accepted options and ignored them. - The select-unique guard. Each write is its own locked transaction, so a rename or type change paired with a constraint write that is going to fail commits first and then throws, half-applying the schema change. - Stable select-option ids. Cells reference the option id, so an edit that re-sends an option by name has to reuse it or every cell holding it is orphaned. Only the copilot path did this; normalizeSelectOptionsInput moves to lib/table/select-options and now covers every caller. It preserves a supplied id, so it is a no-op for the fully-formed options the HTTP contracts accept. - required forwarded into the type and options writes, so a conversion validates against the constraint the same request is setting. - An audit on every successful update. The UI route and the copilot tool emitted none. - Single-row delete through the row service. v2 did a raw db.delete, skipping assertRowDelete and deleteOrderedRow, so a delete-locked table returned 200 and the row-count bookkeeping never ran. - The delete actor handed to deleteTable, which audits only when a row was actually archived. v1 and v2 omitted it and audited themselves outside that check, emitting TABLE_DELETED for a no-op delete of an archived table. Failure classes come back as OrchestrationErrorCode; v2 renders them through a new v2ErrorForOrchestration, mirroring statusForOrchestrationError on the v1 and UI surfaces, so a given failure maps to the same status everywhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
569f89f to
9736288
Compare
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 9736288. Configure here.
The base rewrote column update while this branch was extracting it: writes now address the column by stable id, a rename and constraint change ride inside the typed write's transaction instead of following it, and `currencyCode` joins the update shape for the new `currency` column type. performUpdateTableColumn is rebuilt from that implementation rather than the one this branch originally extracted, so the extraction carries the base's logic forward instead of regressing it. All four callers delegate unchanged. lib/table/select-options.ts arrived independently on the base as a client-safe leaf module for option-id resolution; normalizeSelectOptionsInput joins it there rather than replacing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mprovement/orchestration-migration
The base's route tests assert which column service each payload reaches — the behavior that now lives in performUpdateTableColumn. They mocked the `@/lib/table` barrel; the orchestration module imports the service directly, so they mock that too and keep asserting the same thing through the extracted implementation. The orchestration tests move onto the base's semantics: writes address the stable column id, a rename rides inside the write it accompanies rather than running first, and the currency guards replace the non-select options guard the service now owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f3dd1fe. Configure here.
| return { success: false, error: error.message, errorCode: 'locked' } | ||
| } | ||
| logger.error(`[${requestId}] Failed to delete table ${table.id}`, { error }) | ||
| return { success: false, error: toError(error).message, errorCode: 'internal' } |
There was a problem hiding this comment.
Internal delete errors leak messages
Medium Severity
On unexpected failures, performDeleteTable and performDeleteTableRow return toError(error).message with errorCode: 'internal'. v1/UI routes surface outcome.error directly, so raw DB or infrastructure detail that used to stay behind a generic 500 can now reach clients. performUpdateTableColumn already uses a safe generic internal message; these delete helpers do not.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f3dd1fe. Configure here.
1 participant
Stacked on #5273 — base is
improvement/v2-endpoints, notstaging.The repo's convention is that business logic lives in
lib/[resource]/orchestrationso the Sim UI, the public API, and the copilot tools share one implementation.lib/table/orchestrationexported exactly one function (performRestoreTable); everything else was implemented per-caller. Column update existed four times — the UI route, v1, v2, and the copilot table tool — each calling the same column services but owning its own guards, error mapping, and audit.The copies had drifted, and the drift was the bug. This PR extracts the logic and deletes the copies; the fixes fall out of the extraction rather than being patched at each call site.
performUpdateTableColumn,performDeleteTable, andperformDeleteTableRownow own the logic, and all ten call sites reduce to auth → parse → call → render. Net ~570 lines deleted from routes.Behavior this consolidates
Each of these was previously true on only some paths:
typeChangingguard.updateColumnTypeearly-returns on an unchanged type and drops anyoptionssent with it, so restating the current type alongside new options silently discarded them. v2 had no guard at all — and since its contract shares v1's body schema, it acceptedoptionsand ignored them, returning 200.normalizeSelectOptionsInputmoves tolib/table/select-optionsand now covers every caller — it preserves a supplied id, so it is a no-op for the fully-formed options the HTTP contracts accept and a repair for name-only options.requiredforwarded into the type and options writes, so a conversion validates against the constraint the same request is setting rather than the column's current one.db.deleteonuser_table_rows, skippingassertRowDeleteanddeleteOrderedRow: a delete-locked table returned 200 and the row-count/order bookkeeping never ran. v1 carries a comment warning against exactly this.deleteTable, which audits only when a row was actually archived — omitting the actor is how rollback callers opt out. v1 and v2 omitted it and audited themselves outside that check, emittingTABLE_DELETEDfor a no-op delete of an already-archived table.Error contract
Failure classes come back as
OrchestrationErrorCode. The shared contract moves out oflib/workflows/orchestration/typesintolib/core/orchestration/types— resource-neutral code (lib/folders) already had to import it from a workflow path — and gains alockedclass mapping to 423, since both tables and workflows have a lock that forbids a mutation and every caller was translating that itself.v2 renders these through a new
v2ErrorForOrchestration, mirroringstatusForOrchestrationErroron the v1/UI surfaces, so a given failure maps to the same status on every surface.Review guide
The guard logic is in
lib/table/orchestration/columns.ts— read that once instead of four route diffs. The route diffs are mechanical deletions.Tests follow the same split:
lib/table/orchestration/columns.test.ts(12 cases) andtables.test.tscover the guards, classification, and audit; the route tests are now thin, asserting delegation and the failure-class → envelope mapping.Not in scope
performCreateTable(where the copilot path has no explicit permission check of its own and the UI has folder validation the others lack) and the knowledge resource (where the copilot path emits no audit for any KB mutation, defaultsminSize: 1against the API's100, and KB search has ~1200 duplicated lines) follow in the next PR on this branch.One user-visible bug found but left for the connector consolidation: the copilot's
delete_connectorreports "Associated documents have been removed", but reaches the route by internal HTTP self-call without thedeleteDocumentsparam, which defaults to false — the documents are kept.Verification
app/api/v2+app/api/v1/tables+app/api/table+lib/table+lib/copilot→ 1838 pass.bun run check:api-validation,bun run check:openapi(6 specs, 57 operations, 48 contracts),bun run type-check, and fullbun run lint:checkall clean.🤖 Generated with Claude Code