fix(undo): operation-aware json_patch undo — preserve concurrent keys (1.5.1)#426
fix(undo): operation-aware json_patch undo — preserve concurrent keys (1.5.1)#426zknpr wants to merge 2 commits into
Conversation
… (v1.5.1) Undo-side mirror of the 1.5.0 json_patch forward fixes (caught by a Codex PR review). Undo of a JSON cell edit restored the captured full priorValue, ignoring operation, so a concurrent change to a DIFFERENT key of the same cell was clobbered (e.g. the cell open in a VFS tab while edited inline). Single-pane/sequential undo was already correct; this only affected interleaved same-cell edits. Fix: at undo time, when a cell edit was recorded with operation === 'json_patch', apply an INVERSE merge patch that touches only the keys the forward patch changed — restoring each to its prior value, null to delete keys the edit added — instead of overwriting the whole document. No history-format change: newValue (forward patch), priorValue (full prior) and operation are already recorded per cell (incl. batch affectedCells). Falls back to full value-replacement for non-patch edits and when the prior value was NULL / not a JSON object. - new invertMergePatch + tryCreateInverseMergePatch in core/json-utils.ts (RFC 7396, recursive, restricted to the forward patch's key structure, depth-guarded) - WasmDatabaseEngine.undoCellUpdate (web) and nativeWorker.undoModification (native) both route through the shared helper; native now delegates to operationsFacade (consistent with the redo unification) instead of raw SQL; paired-contract comments - isObject() now excludes arrays so merge patches treat arrays as atomic values (RFC 7396), aligning the JS fallback with SQLite json_patch() Tests (+11, revert-proof): inverse-patch round-trip + concurrent-sibling-key survival; web + native undo preserve a concurrent key while reverting the edited key; NULL-prior edge falls back to value replacement. tsc clean; build OK; 368/368. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 14 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements inverse JSON merge-patch generation to fix undo behavior for JSON cell edits. When users undo a JSON patch, changes now apply only to the keys modified by the forward patch, preserving any concurrent edits to other keys. Arrays are treated as atomic values per RFC 7396. Both WASM and native database engines are updated to use the new inverse-patch logic, with comprehensive test coverage across unit and integration tests. ChangesJSON Patch Undo via Inverse Merge Patches
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a precise undo mechanism for JSON cell edits using inverse merge patches, ensuring that undoing an edit does not clobber concurrent changes to other keys in the same cell. This logic is integrated into both the WASM and native database engines, accompanied by extensive unit tests. Feedback on the changes suggests a critical improvement to verify that the forward patch is a JSON object before attempting an inverse patch, preventing potential database cell corruption when applying SQLite's native json_patch to non-object values.
|
|
||
| const forwardPatch = parseJsonValue(forwardPatchValue); | ||
| const prior = parseJsonValue(priorValue); | ||
| if (!forwardPatch.ok || !prior.ok || !isObject(prior.value)) { |
There was a problem hiding this comment.
If the forward patch is not a JSON object (e.g., it is a scalar or an array), applying an inverse merge patch using SQLite's native json_patch will fail. SQLite's json_patch(T, P) requires both arguments to be JSON objects; if the target T (which would be the non-object value from the forward patch) is not an object, json_patch returns NULL, leading to database cell corruption on undo.
To prevent this, we should ensure that both the forwardPatch and the prior values are JSON objects. If either is not, we should return undefined to safely fall back to the legacy full-value replacement undo.
| if (!forwardPatch.ok || !prior.ok || !isObject(prior.value)) { | |
| if (!forwardPatch.ok || !prior.ok || !isObject(forwardPatch.value) || !isObject(prior.value)) { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e45effde6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (isObject(forwardVal) && isObject(priorVal)) { | ||
| inverse[key] = invertMergePatchAtDepth(forwardVal, priorVal, depth + 1); | ||
| } else { | ||
| inverse[key] = priorHas ? priorVal : null; |
There was a problem hiding this comment.
When a tracked JSON patch changes a property that was null in the prior document, this returns an inverse patch containing that null. In SQLite/RFC merge patch semantics a null patch member deletes the key, so undoing a change like {"a":null,"b":1} -> patch {"a":2} leaves {"b":1} instead of restoring {"a":null,"b":1}. Since this new undo path chooses json_patch instead of the previous full-value restore, touched null-valued keys are lost; fall back to set or otherwise detect nulls that cannot be represented in a merge patch inverse.
Useful? React with 👍 / 👎.
| const inversePatch = tryCreateInverseMergePatch(newValue, priorValue); | ||
| if (inversePatch !== undefined) { | ||
| // The inverse merge patch touches only keys from the original forward patch. | ||
| return { rowId, column, value: inversePatch, operation: 'json_patch' }; |
There was a problem hiding this comment.
When the cell has been changed to plain text or otherwise invalid JSON between the recorded JSON edit and its undo, this returns a json_patch update solely from the recorded values, and the SQLite path executes json_patch(COALESCE(col, '{}'), ?). SQLite raises malformed JSON for an invalid current value, so undo now fails instead of restoring the captured prior value as it did before this change; use a guarded SQL expression or fall back to a full set when the current cell is not valid JSON.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8761fd69-5ed0-4022-afe4-dc00ca661f27
📒 Files selected for processing (8)
CHANGELOG.mdpackage.jsonsrc/core/engine/wasm/WasmDatabaseEngine.tssrc/core/json-utils.tssrc/nativeWorker.tstests/unit/json_utils.test.tstests/unit/nativeWorker.test.tstests/unit/sqlite_db.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, typecheck & test
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (5)
tests/unit/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/*.test.ts: UseObject.defineProperty(obj, prop, { value, writable: true, configurable: true })for setting readonly VS Code API fields likevscode.env.uiKindin unit tests
Importtests/unit/vscode_mock_setup.tsat the beginning of unit test files to ensure VS Code mocks are initialized before running tests
Files:
tests/unit/json_utils.test.tstests/unit/sqlite_db.test.tstests/unit/nativeWorker.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Use prepared statements with?placeholders for all query parameter values to prevent SQL injection
Always useescapeIdentifier()function for table and column names in SQL queries to prevent identifier-based SQL injection
UsevalidateSqlType()for all user-provided SQL types in DDL statements to prevent type-based SQL injection
Validate PRAGMA string values with the regex/^[a-zA-Z0-9_-]+$/whitelist and check numeric PRAGMA values withNumber.isFinite()
UseescapeLikePattern()for user input in LIKE queries with theESCAPE '\\'clause to prevent LIKE wildcard injection
Use zero-copy transfer for large binary data (ArrayBuffers) in RPC communication by wrapping with theTransferwrapper
UseSAVEPOINT/RELEASE/ROLLBACK TOinstead ofBEGIN TRANSACTIONinupdateCellBatchto safely handle nested transactions
Use thesafeRollback(context)helper when handling transaction errors to log failures instead of throwing, preventing secondary rollback errors
Check for SQLitejson_patch()availability at engine construction time and use it in UPDATE statements when available, falling back to JS-sideapplyMergePatch()when unavailable
UsegetNodeFs()fromsqlite-db.tsto safely require the Node.jsfsmodule, which returnsundefinedin browser environments
Checkimport.meta.env.VSCODE_BROWSER_EXTto conditionally handle environment-specific code paths for browser vs Node.js platforms
Use the Core RPC protocol defined insrc/core/rpc.tsfor all Worker communication and when the Extension invokes Webview methods
UsebuildMethodProxy()fromsrc/core/rpc.tsto create proxy objects that automatically serialize RPC calls to workers or the webview
Record database modifications inModificationTrackerviarecordModification()before committing changes to track undo/redo history
Write all executed SQL (both read and write operations) to the 'SQLite Explorer' output channel viaGlobalOutputChannel?.appendLine()for debugging...
Files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.tssrc/core/json-utils.ts
{src/**/*.ts,core/ui/modules/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Serialize
Uint8Arrayusing the marker format{ __type: 'Uint8Array', base64: '...' }with exactly 2 keys to prevent collisions with user data
Files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.tssrc/core/json-utils.ts
src/nativeWorker.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
queryBatch()in the native worker to send multiple SQL queries in a single IPC round-trip for improved performance
Files:
src/nativeWorker.ts
src/{databaseWorker,nativeWorker}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Route worker logs through RPC using
LogEnvelopeformat{ kind: 'log', level, args }instead ofconsole.*to ensure proper log handling
Files:
src/nativeWorker.ts
🔇 Additional comments (2)
package.json (1)
6-6: LGTM!CHANGELOG.md (1)
3-8: LGTM!
…view round 2)
Bot review of the first inverse-patch pass found 4 real edge cases (RFC 7396
merge patches are lossy). Principle: emit an inverse patch ONLY when provably
faithful; otherwise fall back to full value-replacement (safe — never corrupts,
just doesn't preserve concurrent keys).
- Gemini (high): tryCreateInverseMergePatch now also requires the forward patch
to be a JSON object; non-object forward -> undefined -> value-replace.
- Codex (P2): a touched key whose prior value was explicitly null cannot be
restored by a merge patch (null member = delete). Inversion throws a private
UnfaithfulInverseMergePatchError in that case; tryCreate catches it and falls
back to value-replacement. Null emitted to DELETE a key the forward patch ADDED
stays faithful and is kept.
- CodeRabbit (major): nested additions no longer over-delete. A forward object
patch always recurses — missing prior -> recurse against {} so undo deletes only
the leaves the patch added (preserving concurrent nested siblings); scalar/array
prior -> restore it wholesale.
- Codex (P2): undo never hard-fails if the cell became non-JSON since the edit.
applyUndoCellUpdate (web + native) tries the json_patch undo and, on any error,
falls back to value-replacement using the recorded prior. Batch undo applies
cells individually when any is json_patch so the per-cell fallback applies.
Tests (+14, revert-proof): non-object forward, explicit-null prior, nested-add
sibling survival, and non-JSON current cell — each asserts faithful-inverse or
safe value-replacement. tsc clean; build OK; full suite 382/382.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review round 2 addressed (commit 0fcae2d)All four edge cases were real (RFC 7396 merge patches are lossy). Fixed safe-by-construction: emit an inverse patch only when provably faithful; otherwise fall back to full value-replacement (the safe pre-1.5.1 behavior — never corrupts, just doesn't preserve concurrent keys).
Verification (local): tsc clean · build OK · full suite 382/382 (+14, revert-proof — reverting the 3 source files fails exactly the 14 new edge tests). |
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces inverse JSON merge patches to ensure that undoing a JSON cell edit does not overwrite concurrent changes to other keys of the same cell. It implements the patch inversion logic, integrates it into both the WASM and native database engines with safe fallbacks, and adds extensive unit tests. The review feedback suggests simplifying a redundant conditional branch in the JSON utility, optimizing mixed batch updates by partitioning standard and patch updates, and extracting a duplicated helper function to a shared utility file to improve maintainability.
| } else if (forwardVal === null) { | ||
| inverse[key] = createInverseLeafValue(priorHas, priorVal); | ||
| } else { | ||
| inverse[key] = createInverseLeafValue(priorHas, priorVal); | ||
| } |
There was a problem hiding this comment.
The else if (forwardVal === null) branch is redundant because it executes the exact same logic as the else branch. Simplifying this improves code readability and maintainability.
} else {
inverse[key] = createInverseLeafValue(priorHas, priorVal);
}| if (updates.some(update => update.operation === 'json_patch')) { | ||
| for (const update of updates) { | ||
| await this.applyUndoCellUpdate(targetTable, update); | ||
| } | ||
| } else { | ||
| await this.updateCellBatch(targetTable, updates); | ||
| } |
There was a problem hiding this comment.
When a batch of updates contains at least one json_patch operation, all updates in the batch are currently executed sequentially. We can optimize this by partitioning the updates: batching all standard set updates together using updateCellBatch, and only executing the json_patch updates sequentially. This significantly improves performance for mixed batches.
| if (updates.some(update => update.operation === 'json_patch')) { | |
| for (const update of updates) { | |
| await this.applyUndoCellUpdate(targetTable, update); | |
| } | |
| } else { | |
| await this.updateCellBatch(targetTable, updates); | |
| } | |
| const patchUpdates = updates.filter(u => u.operation === 'json_patch'); | |
| const setUpdates = updates.filter(u => u.operation !== 'json_patch'); | |
| if (setUpdates.length > 0) { | |
| await this.updateCellBatch(targetTable, setUpdates); | |
| } | |
| for (const update of patchUpdates) { | |
| await this.applyUndoCellUpdate(targetTable, update); | |
| } |
| if (updates.some(update => update.operation === 'json_patch')) { | ||
| for (const update of updates) { | ||
| await applyUndoCellUpdate(update); | ||
| } | ||
| } else { | ||
| await operationsFacade.updateCellBatch(targetTable, updates); | ||
| } |
There was a problem hiding this comment.
When a batch of updates contains at least one json_patch operation, all updates in the batch are currently executed sequentially. We can optimize this by partitioning the updates: batching all standard set updates together using operationsFacade.updateCellBatch, and only executing the json_patch updates sequentially. This significantly improves performance for mixed batches.
| if (updates.some(update => update.operation === 'json_patch')) { | |
| for (const update of updates) { | |
| await applyUndoCellUpdate(update); | |
| } | |
| } else { | |
| await operationsFacade.updateCellBatch(targetTable, updates); | |
| } | |
| const patchUpdates = updates.filter(u => u.operation === 'json_patch'); | |
| const setUpdates = updates.filter(u => u.operation !== 'json_patch'); | |
| if (setUpdates.length > 0) { | |
| await operationsFacade.updateCellBatch(targetTable, setUpdates); | |
| } | |
| for (const update of patchUpdates) { | |
| await applyUndoCellUpdate(update); | |
| } |
| private createUndoCellUpdate( | ||
| rowId: RecordId, | ||
| column: string, | ||
| priorValue: CellValue | undefined, | ||
| newValue: CellValue | undefined, | ||
| operation: ModificationEntry['operation'] | ||
| ): CellUpdate { | ||
| if (operation === 'json_patch') { | ||
| const inversePatch = tryCreateInverseMergePatch(newValue, priorValue); | ||
| if (inversePatch !== undefined) { | ||
| // The inverse merge patch touches only keys from the original forward patch. | ||
| return { rowId, column, value: inversePatch, originalValue: priorValue ?? null, operation: 'json_patch' }; | ||
| } | ||
| } | ||
|
|
||
| // Non-patch edits and unparseable/non-object priors retain legacy value replacement undo. | ||
| return { rowId, column, value: priorValue ?? null, originalValue: priorValue ?? null, operation: 'set' }; | ||
| } |
There was a problem hiding this comment.
The createUndoCellUpdate helper function is duplicated exactly between WasmDatabaseEngine.ts and nativeWorker.ts. To improve maintainability and prevent future logic drift, consider extracting this function to a shared utility file (such as src/core/json-utils.ts or a dedicated shared helpers file) and importing it in both places.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fcae2d0a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| inverse[key] = priorVal; | ||
| } else { | ||
| // Recurse into added objects so undo deletes only the leaves touched by the forward patch. | ||
| inverse[key] = invertMergePatchAtDepth(forwardVal, {}, depth + 1); |
There was a problem hiding this comment.
When the tracked patch adds a whole object and no nested sibling was added later (for example prior {} and forward patch {"meta":{"reviewed":true}}), this branch emits an inverse like {"meta":{"reviewed":null}}; SQLite merge patching that over {"meta":{"reviewed":true}} leaves {"meta":{}} instead of restoring the prior {}. This makes a plain undo of nested object additions leave empty container keys behind; fall back to deleting the parent key when there are no concurrent nested keys, or otherwise clean up the empty object after the inverse patch.
Useful? React with 👍 / 👎.
| for (const update of updates) { | ||
| await this.applyUndoCellUpdate(targetTable, update); | ||
| } |
There was a problem hiding this comment.
When a batch undo contains any JSON patch, this path applies every cell update one at a time instead of using the existing updateCellBatch savepoint/transaction. If a later cell fails during discard/undo (for example because its column was removed externally or a constraint rejects the restored value), earlier cells in the same recorded batch have already been reverted and are not rolled back, leaving a partially undone modification; wrap this loop in a savepoint/transaction like updateCellBatch does. The same sequential pattern is mirrored in src/nativeWorker.ts, while native execBatch is explicitly transactional.
Useful? React with 👍 / 👎.
| throw new UnfaithfulInverseMergePatchError(); | ||
| } | ||
|
|
||
| return priorVal; |
There was a problem hiding this comment.
The fresh case here is that this helper only rejects priorVal === null; when the prior value being emitted is an object that contains explicit nulls, those nested nulls are still serialized into the inverse merge patch and are interpreted as deletions. For example, undoing a forward delete of a from prior {"a":{"x":null,"y":1}} emits {"a":{"x":null,"y":1}}, and SQLite restores {"a":{"y":1}} instead of the captured prior value, so recursively detect nulls in whole-subtree restore values and fall back to a full set.
Useful? React with 👍 / 👎.
| private async applyUndoCellUpdate(targetTable: string, update: CellUpdate): Promise<void> { | ||
| if (update.operation === 'json_patch') { | ||
| try { | ||
| await this.updateCell(targetTable, update.rowId, update.column, null, update.value as string); |
There was a problem hiding this comment.
If the cell is changed to SQL NULL or a valid JSON scalar/array between the tracked JSON edit and undo, this json_patch call does not throw, so the catch-based full-restore fallback never runs. SQLite treats non-object targets as an empty object here (and COALESCE explicitly maps SQL NULL to {}), so undoing a patch like prior {"status":"draft","owner":"ada"} with inverse {"status":"draft"} leaves only status and drops untouched prior keys; guard the current value/object shape before applying the inverse patch or fall back to originalValue.
Useful? React with 👍 / 👎.
|
Closing without merging — architectural decision, not abandoning the fix. After 3 review rounds the inverse-merge-patch approach accumulated 8 edge cases, and round 3 showed it can now silently produce wrong results (adding-then-undoing leaves an empty Decision: keep the safe value-replacement undo that already ships in 1.5.0 (correct for all normal/sequential undo; only imperfect for the narrow case of concurrent edits to different keys of the same cell), and do the concurrent-key-safe undo properly in #425 with a non-lossy approach. Recommended approach for #425 (read-modify-write): at undo time read the current cell value; if it's a JSON object, restore only the keys the forward patch touched (restore prior value exactly, including explicit nulls; delete keys the edit added; recurse for nested); write the full result back. Fall back to value-replacement if the current value isn't a JSON object. This handles all 8 edge cases AND keeps batch atomicity (results are plain This branch ( |
1 participant
Summary
Undo-side mirror of the 1.5.0 json_patch forward fixes, caught by a post-merge Codex PR review on #424 (review).
Undo of a JSON cell edit restored the captured full
priorValue, ignoringoperation, so a concurrent change to a different key of the same cell was clobbered (e.g. the cell open in a VFS tab while it's also edited inline). Single-pane / sequential undo was already correct; this only affected interleaved same-cell edits. P2.Fix
At undo time, when a cell edit was recorded with
operation === 'json_patch', apply an inverse merge patch that touches only the keys the forward patch changed — restoring each to its prior value,nullto delete keys the edit added — instead of overwriting the whole document. No history-format change:newValue(forward patch),priorValue(full prior) andoperationare already recorded per cell (incl. batchaffectedCells). Falls back to full value-replacement for non-patch edits and when the prior value was NULL / not a JSON object.invertMergePatch+tryCreateInverseMergePatchincore/json-utils.ts(RFC 7396, recursive, restricted to the forward patch's key structure, depth-guarded)WasmDatabaseEngine.undoCellUpdate(web) andnativeWorker.undoModification(native) route through the shared helper; native now delegates tooperationsFacade(consistent with the redo unification) instead of raw SQL; paired-contract commentsisObject()now excludes arrays so merge patches treat arrays as atomic values (RFC 7396), aligning the JS fallback with SQLitejson_patch()Verification
tsc --noEmitclean ·node scripts/build.mjsOK · full suite 368/368 (+11)Notes
🤖 Generated with Claude Code
Summary by CodeRabbit