Skip to content

fix(table): preserve workflow groups on CSV column-add and dispatch after tx commit#4503

Open
TheodoreSpeaks wants to merge 1 commit intostagingfrom
fix/csv-import-tx-and-schema
Open

fix(table): preserve workflow groups on CSV column-add and dispatch after tx commit#4503
TheodoreSpeaks wants to merge 1 commit intostagingfrom
fix/csv-import-tx-and-schema

Conversation

@TheodoreSpeaks
Copy link
Copy Markdown
Collaborator

Summary

  • `addTableColumnsWithTx` rebuilt the schema with only `columns`, dropping `workflowGroups`. Importing CSV columns into a table that already had workflow groups silently erased the group config. Spread `table.schema` first.
  • `batchInsertRowsWithTx` fired `fireTableTrigger` + `scheduleRunsForRows` from inside the caller's transaction. Both read through the global db connection, so they could run before the inserts committed and see no rows. Extracted the dispatch into `dispatchAfterBatchInsert`; non-tx wrapper and the CSV import route both call it after the tx commits.

Type of Change

  • Bug fix

Testing

`bunx vitest run lib/table app/api/table` — 151/151 pass. Lint clean.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…fter tx commit

Two bugs in the CSV-import path:

- addTableColumnsWithTx rebuilt the schema with only `columns`, dropping
  `workflowGroups` (and any other top-level schema fields). Importing CSV
  into a table that has workflow groups erased the group config. Spread
  `table.schema` first so siblings survive.

- batchInsertRowsWithTx fired fireTableTrigger and scheduleRunsForRows from
  inside the caller's transaction. Both read through the global db
  connection, so they could run before the inserts committed and see no
  rows. Extracted the dispatch into dispatchAfterBatchInsert; non-tx
  wrapper fires it after `db.transaction(...)` resolves, and the CSV
  import route does the same after its tx.
@vercel
Copy link
Copy Markdown

vercel Bot commented May 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 7, 2026 10:41pm

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 7, 2026

PR Summary

Medium Risk
Changes when table insert side-effects (triggers/scheduling) run and how schema updates are constructed, which can affect workflow execution timing and downstream consumers. Scope is limited to CSV import and batch insert paths but touches core table write behavior.

Overview
Fixes two CSV-import-related table bugs.

addTableColumnsWithTx now preserves non-column schema fields (e.g. workflowGroups) when adding columns, avoiding silent loss of schema config during CSV-driven column creation.

Insert side-effects are moved to a new dispatchAfterBatchInsert helper and are invoked after the surrounding transaction commits (both in batchInsertRows and the CSV import append flow), ensuring fireTableTrigger/scheduleRunsForRows see the newly inserted rows. Tests for the import route are updated to mock this new dispatch.

Reviewed by Cursor Bugbot for commit 50817dc. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 7, 2026

Greptile Summary

This PR patches two related bugs in the table CSV-import path. addTableColumnsWithTx was reconstructing the schema object from scratch, silently dropping workflowGroups and any other top-level schema fields on every CSV column-add; the fix spreads table.schema first. batchInsertRowsWithTx was firing fireTableTrigger and scheduleRunsForRows from inside the caller's transaction, where the global DB connection could not yet see the committed rows; dispatch is now extracted into dispatchAfterBatchInsert and called by both the non-tx wrapper (batchInsertRows) and the CSV append route after db.transaction resolves.

  • service.ts: Schema spread preserves workflowGroups; dispatchAfterBatchInsert is the new public helper that callers invoke post-commit.
  • route.ts (append mode): Accumulates all inserted rows inside the transaction, then fires dispatch once after the transaction commits using the updated working table (correct schema for trigger context).
  • route.test.ts: Wires dispatchAfterBatchInsert into the service mock but omits assertions in happy-path tests that it was invoked.

Confidence Score: 4/5

Safe to merge — both bug fixes are well-scoped and correct; the only gap is a missing assertion in the test suite.

The schema spread in addTableColumnsWithTx correctly preserves workflowGroups, and dispatchAfterBatchInsert is called after db.transaction resolves in both the non-tx wrapper and the CSV append route. The only weak spot is that route.test.ts registers mockDispatchAfterBatchInsert but never asserts it was called, so a future regression dropping the dispatch call would go undetected.

apps/sim/app/api/table/[tableId]/import/route.test.ts — happy-path tests should assert dispatchAfterBatchInsert is invoked

Important Files Changed

Filename Overview
apps/sim/lib/table/service.ts Two targeted fixes: (1) schema spread in addTableColumnsWithTx to preserve workflowGroups, and (2) dispatch extracted to dispatchAfterBatchInsert so triggers fire only after the transaction commits.
apps/sim/app/api/table/[tableId]/import/route.ts Append mode collects all inserted rows during the transaction, then calls dispatchAfterBatchInsert after db.transaction resolves using the updated working table.
apps/sim/app/api/table/[tableId]/import/route.test.ts Adds dispatchAfterBatchInsert mock but no happy-path test asserts it was called, leaving a coverage gap.

Sequence Diagram

sequenceDiagram
    participant Route as CSV Import Route
    participant DB as db.transaction
    participant Service as service (Tx helpers)
    participant Trigger as fireTableTrigger / scheduleRunsForRows

    Route->>DB: begin transaction
    DB->>Service: addTableColumnsWithTx (schema spread fix)
    Service-->>DB: updated table (workflowGroups preserved)
    DB->>Service: batchInsertRowsWithTx x N batches
    Service-->>DB: allInserted rows
    DB-->>Route: inserted rows + finalTable
    Note over DB: transaction committed
    Route->>Trigger: dispatchAfterBatchInsert(finalTable, insertedRows) - runs AFTER commit
Loading

Comments Outside Diff (1)

  1. apps/sim/app/api/table/[tableId]/import/route.test.ts, line 45-50 (link)

    P2 dispatchAfterBatchInsert mock is never asserted

    mockDispatchAfterBatchInsert is hoisted and registered in the service mock but none of the happy-path tests (e.g. "appends rows via batchInsertRows", "auto-creates columns") assert expect(mockDispatchAfterBatchInsert).toHaveBeenCalledTimes(1). If the dispatchAfterBatchInsert call were accidentally dropped from the route, all tests would still pass.

Reviews (1): Last reviewed commit: "fix(table): preserve workflow groups on ..." | Re-trigger Greptile

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.

1 participant