Skip to content

fix(tables): make the atomic per-cell merge the only row-write path (fix row-level LWW) - #5970

Merged
waleedlatif1 merged 3 commits into
stagingfrom
table-cell-atomic-patch
Jul 27, 2026
Merged

fix(tables): make the atomic per-cell merge the only row-write path (fix row-level LWW)#5970
waleedlatif1 merged 3 commits into
stagingfrom
table-cell-atomic-patch

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • A table row stores every cell in one jsonb data column. Row updates ran a full-object read-modify-write (write the whole data), which is row-level last-write-wins: two users — or a user and an agent/copilot — editing different cells of the same row concurrently clobbered each other.
  • updateRow now always writes the changed cells via a shared jsonbMergePatch helper (data = data || {changed}::jsonb), which Postgres evaluates against the current committed row under its write lock — so concurrent edits to different cells both survive. The dataWriteMode: 'replace' | 'patch' option is removed: replace never actually replaced (the merge only ever adds/overwrites keys, never deletes), so it was content-identical to patch and differed only by being racy — no caller benefited from it.
  • batchUpdateRows (the multi-cell grid / copilot batch path) applies the same per-row merge, so batch edits can't clobber a concurrent single-cell edit either.
  • Because the fix lives entirely in the service layer, the user-facing route + copilot callers are unchanged vs staging, and the two data:{} exec-clear writes in background/workflow-column-execution.ts are covered automatically. upsertRow (whole-row by design) and updateRowsByFilter (already ||-concat) are untouched. computedWrite (the workflow-engine lock exemption) is unaffected.

Type of Change

  • Bug fix

Testing

  • update-row.test.ts: asserts updateRow and batchUpdateRows each write data || {changed}::jsonb carrying only the changed cells (never the whole row).
  • Reviewed via a 4-lens parallel audit (correctness/concurrency, downstream side-effects, completeness/scope, conventions): confirmed non-concurrent parity, no side effects (returned row unchanged; peers reconcile via authoritative refetch), and the mechanism is already prod-proven by cell-write.ts.
  • check:api-validation:strict, check:utils, tsc --noEmit, biome, and the table + copilot suites (481 + 111 + 46) pass.

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)

…w-level LWW)

A table row stores every cell in one jsonb `data` column, and the row-edit paths
called updateRow in the default replace mode — a full-object read-modify-write. Two
users (or a user and an agent/copilot) editing DIFFERENT cells of the same row
concurrently would clobber each other: whichever write committed last overwrote the
whole row from its stale snapshot.

Pass dataWriteMode: 'patch' (Postgres `data = data || patch::jsonb`) from the four
partial-update callers — the UI row route, the v1 API row route, the copilot table
tool, and the workflow-group cancel-write — so each cell edit merges atomically under
the row lock. upsertRow (whole-row by design) and updateRowsByFilter (already uses
||) are unchanged. computedWrite stays unset on the user paths, preserving the update
lock. Adds a route test asserting the UI route opts into patch mode; the updateRow
patch mechanism itself is already covered.
@vercel

vercel Bot commented Jul 26, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 27, 2026 4:01pm

Request Review

@cursor

cursor Bot commented Jul 26, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches core table row persistence and concurrency behavior for all updateRow/batchUpdateRows callers; behavior change is intentional but widespread across user edits and workflow writes.

Overview
Fixes row-level last-write-wins when concurrent writers touch different cells on the same row (users, copilot, or workflow progress).

updateRow now always persists via a shared jsonbMergePatch (data = data || {changed}::jsonb) instead of sometimes writing the full merged data object. The dataWriteMode: 'replace' | 'patch' option is removed — callers no longer opt into patch mode; cell-write only passes computedWrite.

batchUpdateRows uses the same per-row merge (changed column ids only), so grid/copilot batch edits don’t overwrite concurrent single-cell edits.

Tests in update-row.test.ts assert the SQL merge shape for updateRow and add coverage for batchUpdateRows; the duplicate patch-mode test is dropped because patch is the only path.

Reviewed by Cursor Bugbot for commit e7c903b. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes atomic JSONB cell merging the sole row-update strategy.

  • Adds a shared jsonbMergePatch helper for updateRow and batchUpdateRows.
  • Removes the obsolete dataWriteMode option and updates workflow-cell callers.
  • Adds tests verifying that writes contain only explicitly changed cells.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/lib/table/rows/service.ts Centralizes partial JSONB row writes and applies them consistently to single and batched updates.
apps/sim/lib/table/tests/update-row.test.ts Verifies single-row and batch paths persist only changed cells through JSONB concatenation.
apps/sim/lib/table/cell-write.ts Removes the obsolete write-mode option while preserving the computed-write lock exemption.
apps/sim/lib/table/workflow-columns.ts Documents that execution-only updates now produce an empty, non-destructive cell patch.

Sequence Diagram

sequenceDiagram
  participant A as Editor A
  participant B as Editor B
  participant DB as PostgreSQL row
  A->>DB: "UPDATE data = data || {cellA}"
  DB->>DB: Lock row and merge cellA
  B->>DB: "UPDATE data = data || {cellB}"
  DB->>DB: Wait, then merge cellB into committed row
  DB-->>A: Updated
  DB-->>B: Updated
  Note over DB: Both distinct-cell changes survive
Loading

Reviews (3): Last reviewed commit: "refactor(tables): make the atomic cell-m..." | Re-trigger Greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ 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 7aff1ee. Configure here.

Audit of the opt-in-per-caller approach found it incomplete and the wrong shape:
- `replace` mode never actually replaces — updateRow computes mergedData =
  {...existing, ...patch}, which only ever adds/overwrites keys, never deletes. So
  replace is content-identical to patch and differs ONLY by being racy (full-object
  read-modify-write = row-level last-write-wins). No caller benefits from it.
- Two structurally identical `data:{}` exec-clears in background/workflow-column-
  execution.ts were left in replace mode, exposed to the same clobber the PR fixes.
- batchUpdateRows (the multi-cell grid/copilot path) still full-replaced with no
  patch option at all, so the PR's own goal was only half-met.

Remove the dataWriteMode option entirely and make updateRow ALWAYS write the changed
cells via a shared jsonbMergePatch helper (data = data || {changed}::jsonb). Apply the
same helper per-row in batchUpdateRows. This auto-covers every updateRow caller —
including the two exec-clears — and closes the grid path, with the user-facing route
and copilot callers now unchanged vs staging (the fix lives entirely in the service
layer). computedWrite (the workflow-engine lock exemption) is untouched. Tests updated
to assert the always-on JSONB merge for both updateRow and batchUpdateRows.
@waleedlatif1 waleedlatif1 changed the title fix(tables): write single-cell edits as an atomic JSONB patch (fix row-level LWW) fix(tables): make the atomic per-cell merge the only row-write path (fix row-level LWW) Jul 27, 2026
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ 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 e7c903b. Configure here.

@waleedlatif1
waleedlatif1 merged commit 104a6fc into staging Jul 27, 2026
21 checks passed
@waleedlatif1
waleedlatif1 deleted the table-cell-atomic-patch branch July 27, 2026 16:10
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