fix(undo): operation-aware json_patch cell undo via read-modify-write (#425 part 1 of 2)#427
Conversation
…write (#425 part 1) Design doc for the undo-fidelity half of #425, superseding the closed PR #426 inverse-merge-patch attempt. Read-modify-write (read current cell, restore only the forward patch's touched keys, write the full object back; else value-replace) is non-lossy across the edge cases #426 closed unresolved (nested explicit nulls; non-object current cell) and collapses the edge-case treadmill that caused #426's review churn. Part 2 (futureStack hot-exit persistence) is a separate spec/PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Task-by-task TDD plan: pure computeJsonPatchUndo helper, WASM + native engine read-modify-write wiring, full verification. No version/changelog (1.5.1 ships with part 2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
computeJsonPatchUndo restores only the forward patch's touched keys from the recorded prior, preserving concurrent sibling edits; value-replaces when current, prior, or the forward patch is not a JSON object. isObject now excludes arrays (RFC 7396 atomic). Pure + exhaustively unit-tested; engine wiring follows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
undoCellUpdate now reads the current cell and routes json_patch undos through computeJsonPatchUndo (full-object SET), preserving concurrent sibling edits; non-json_patch and non-object-current cells value-replace. Batch undos that touch any json_patch cell are wrapped in a SAVEPOINT for atomicity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
undoModification cell_update now reads current then writes the restored object (or value-replaces) via computeJsonPatchUndo — matching the WASM engine. Batch json_patch undos write through one execBatch for atomicity. 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 Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a read-modify-write undo strategy for JSON cell edits. A pure ChangesJSON Patch Undo via Read-Modify-Write
Sequence DiagramsequenceDiagram
participant Editor as Client Edit
participant WASM as WASM Engine
participant Native as Native Worker
participant JSON as computeJsonPatchUndo
participant SQLite
Editor->>WASM: undoModification(json_patch cell)
WASM->>SQLite: readCellValue (current JSON)
SQLite-->>WASM: currentValue
WASM->>JSON: compute(current, forward, prior)
JSON-->>WASM: JsonUndoPlan (restore|replace)
WASM->>SQLite: updateCellBatch (restored value)
Editor->>Native: undoModification(json_patch cell batch)
Native->>SQLite: query (current JSON per cell)
SQLite-->>Native: [current1, current2, ...]
Native->>JSON: compute per cell
JSON-->>Native: [plan1, plan2, ...]
Native->>SQLite: execBatch (atomic UPDATEs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ 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 an operation-aware json_patch cell undo mechanism using a read-modify-write (RMW) approach in JavaScript/TypeScript. It introduces a pure decision helper computeJsonPatchUndo in json-utils.ts to surgically restore only the keys touched by a forward patch while preserving concurrent sibling edits, and integrates this logic into both the WASM and native database engines. Reviewers suggested simplifying and optimizing the batch undo logic in both engines: first, by leveraging the existing updateCellBatch method in the WASM engine to avoid sequential query overhead and manual savepoint management; and second, by unifying the batch update path in the native worker to eliminate redundant conditional branching.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (affectedCells) { | ||
| // Batch undo | ||
| const updates = affectedCells.map(cell => ({ | ||
| rowId: cell.rowId, | ||
| column: cell.columnName, | ||
| value: cell.priorValue ?? null | ||
| })); | ||
| await this.updateCellBatch(targetTable, updates); | ||
| const hasJsonPatchUndo = affectedCells.some(cell => cell.operation === 'json_patch'); | ||
| if (hasJsonPatchUndo) { | ||
| // JSON-patch undo reads current cells before writing restored values, so | ||
| // the whole history entry is protected by one savepoint for atomicity. | ||
| const savepointName = this.createSavepointName('sp_undo_cell_update'); | ||
| await this.executeQuery(`SAVEPOINT ${savepointName}`); | ||
| try { | ||
| for (const cell of affectedCells) { | ||
| await this.undoOneCell( | ||
| targetTable, | ||
| cell.rowId, | ||
| cell.columnName, | ||
| cell.priorValue, | ||
| cell.newValue, | ||
| cell.operation | ||
| ); | ||
| } | ||
| await this.executeQuery(`RELEASE ${savepointName}`); | ||
| } catch (err) { | ||
| await this.safeRollbackSavepoint(savepointName, 'undoCellUpdate'); | ||
| throw err; | ||
| } | ||
| } else { | ||
| // Pure value-replacement batches keep the existing batch update path. | ||
| const updates = affectedCells.map(cell => ({ | ||
| rowId: cell.rowId, | ||
| column: cell.columnName, | ||
| value: cell.priorValue ?? null | ||
| })); | ||
| await this.updateCellBatch(targetTable, updates); | ||
| } |
There was a problem hiding this comment.
We can simplify the batch undo logic and significantly improve performance by reusing the existing updateCellBatch method. Instead of executing individual updateCell queries sequentially in a loop (which incurs high query overhead), we can collect all the computed restored values first and then perform a single batched update. This also eliminates the need for manual savepoint/transaction management here, as updateCellBatch already handles it atomically.
if (affectedCells) {
const updates: CellUpdate[] = [];
for (const cell of affectedCells) {
let value = cell.priorValue ?? null;
if (cell.operation === 'json_patch') {
const currentValue = await this.readCellValue(targetTable, cell.rowId, cell.columnName);
const plan = computeJsonPatchUndo(currentValue, cell.newValue, cell.priorValue);
if (plan.kind === 'restore') {
value = plan.value;
}
}
updates.push({
rowId: cell.rowId,
column: cell.columnName,
value
});
}
await this.updateCellBatch(targetTable, updates);
}| if (affectedCells) { | ||
| // Batch undo | ||
| const updates = affectedCells.map(c => ({ | ||
| rowId: c.rowId, | ||
| column: c.columnName, | ||
| value: c.priorValue | ||
| } as CellUpdate)); | ||
| await worker.call('execBatch', [ | ||
| updates.map(u => ({ | ||
| sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(u.column)} = ? WHERE rowid = ?`, | ||
| params: [u.value, Number(u.rowId)] | ||
| })) | ||
| ]); | ||
| const hasJsonPatchUndo = affectedCells.some(cell => cell.operation === 'json_patch'); | ||
| if (hasJsonPatchUndo) { | ||
| // Read and compute all values first, then send one execBatch | ||
| // so native SQLite applies the write set atomically. | ||
| const batchItems: { sql: string; params: CellValue[] }[] = []; | ||
| for (const cell of affectedCells) { | ||
| const rowIdNum = validateRowId(cell.rowId); | ||
| const value = await resolveUndoValue( | ||
| cell.rowId, | ||
| cell.columnName, | ||
| cell.priorValue, | ||
| cell.newValue, | ||
| cell.operation | ||
| ); | ||
| batchItems.push({ | ||
| sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(cell.columnName)} = ? WHERE rowid = ?`, | ||
| params: [value, rowIdNum] | ||
| }); | ||
| } | ||
| if (batchItems.length > 0) { | ||
| await worker.call('execBatch', [batchItems]); | ||
| } | ||
| } else { | ||
| // Pure value-replacement batches keep the existing SQL shape. | ||
| const updates = affectedCells.map(c => ({ | ||
| rowId: c.rowId, | ||
| column: c.columnName, | ||
| value: c.priorValue | ||
| } as CellUpdate)); | ||
| await worker.call('execBatch', [ | ||
| updates.map(u => ({ | ||
| sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(u.column)} = ? WHERE rowid = ?`, | ||
| params: [u.value, Number(u.rowId)] | ||
| })) | ||
| ]); | ||
| } |
There was a problem hiding this comment.
We can simplify the batch undo logic by removing the conditional branching on hasJsonPatchUndo. Since resolveUndoValue immediately returns cellPrior ?? null without any database query when the operation is not json_patch, we can run the same unified loop for all cells. This removes code duplication and keeps the batch update logic clean and consistent.
if (affectedCells) {
const batchItems: { sql: string; params: CellValue[] }[] = [];
for (const cell of affectedCells) {
const rowIdNum = validateRowId(cell.rowId);
const value = await resolveUndoValue(
cell.rowId,
cell.columnName,
cell.priorValue,
cell.newValue,
cell.operation
);
batchItems.push({
sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(cell.columnName)} = ? WHERE rowid = ?`,
params: [value, rowIdNum]
});
}
if (batchItems.length > 0) {
await worker.call('execBatch', [batchItems]);
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md`:
- Line 97: The fenced code block that begins with the triple backticks at Line
97 is missing a language tag and triggers markdownlint MD040; update that
opening fence (the ``` line) to include an appropriate language identifier
(e.g., ```json or ```diff) so the block is language-highlighted and the lint
rule is satisfied.
- Line 43: The ATX heading on the line that begins "`#426` built an **inverse
merge patch** (`invertMergePatch` / `tryCreateInverseMergePatch`) and applied"
is missing the required space after the "##"; update the heading to include a
space after the hashes (e.g., "## 426 built ...") so it conforms to MD018 and
renders correctly; the change touches the heading text referencing
invertMergePatch and tryCreateInverseMergePatch.
In `@src/core/json-utils.ts`:
- Around line 165-190: The code currently uses plain object literals and spread
({ ...currentObj }) which allow special keys like "__proto__" to mutate
prototypes; update restoreInto so temporary containers are created with no
prototype and only own properties are copied: replace "const result:
Record<string, unknown> = { ...currentObj }" with creating result via
Object.create(null) and copy currentObj's own keys into it (e.g., for (const k
of Object.keys(currentObj)) result[k] = currentObj[k]); similarly ensure "base"
and any other temporary nested containers use Object.create(null) and
copy-only-own-properties before calling restoreInto, and continue to use
Object.prototype.hasOwnProperty.call(priorObj, key) for existence checks to
avoid prototype side-effects.
In `@src/nativeWorker.ts`:
- Around line 543-617: The current json_patch undo path calls
operationsFacade.executeQuery per cell inside resolveUndoValue, causing many IPC
round-trips; instead, collect all SELECT statements for affectedCells (using
validateRowId and escapeIdentifier on column/targetTable), call
worker.call('queryBatch') once to fetch current values in the same order, then
for each cell run computeJsonPatchUndo(currentValue, cell.newValue,
cell.priorValue) and build batchItems for worker.call('execBatch') as before;
ensure you preserve the mapping order between affectedCells and queryBatch
results and keep resolveUndoValue logic but refactor it to consume pre-fetched
current values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e34fea64-b459-49e4-8304-5f9de88a71f2
📒 Files selected for processing (8)
docs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mddocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdsrc/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
🧰 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/sqlite_db.test.tstests/unit/nativeWorker.test.tstests/unit/json_utils.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/json-utils.tssrc/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.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/json-utils.tssrc/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.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
🧠 Learnings (33)
📓 Common learnings
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:34:38.498Z
Learning: Use JSON Merge Patch (RFC 7396) for cell updates with `json_patch()` when available, falling back to JS-side `applyMergePatch()` from `json-utils.ts`.
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/sqlite-db.ts : Use `json_patch()` SQLite function when available for cell updates (JSON1 extension); fall back to JS-side `applyMergePatch()` when unavailable
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:34:49.264Z
Learning: Applies to src/**/*.{ts,tsx} : Use JSON Patch (RFC 7396) via `applyMergePatch()` from `json-utils.ts` for cell updates when SQLite's `json_patch()` is unavailable
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:03.123Z
Learning: Applies to src/core/json-utils.ts : Apply JSON Merge Patch (RFC 7396) using generate() and apply() from json-utils.ts for cell patch operations
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:22.670Z
Learning: Applies to src/**/*.ts : Use `SAVEPOINT`/`RELEASE`/`ROLLBACK TO` instead of `BEGIN TRANSACTION` in `updateCellBatch` to safely handle nested transactions
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:34:42.846Z
Learning: Applies to tests/unit/**/*.test.ts : Test JSON patch behavior (merged result), not implementation (SQL function)
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:32.320Z
Learning: Applies to tests/unit/*.test.ts : Test JSON patch behavior (merged result) rather than implementation details (specific SQL functions).
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/core/sqlite-db.ts : Probe for SQLite `json_patch()` at engine construction time to optimize `updateCell` and `updateCellBatch` operations
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:28.282Z
Learning: Applies to tests/**/*.test.ts : Test JSON patch behavior (merged result) not implementation (SQL function) to ensure correctness
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:51.645Z
Learning: Applies to tests/unit/*.test.ts : Test JSON patch behavior (merged result) not implementation (SQL function) in unit tests
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/sqlite-db.ts : Probe for SQLite's `json_patch()` at engine construction time and use `json_patch(COALESCE(col, '{}'), ?)` in single UPDATE when available instead of SELECT round-trip
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/core/sqlite-db.ts : Use `json_patch(COALESCE(col, '{}'), ?)` in a single UPDATE when JSON1 extension is available; fall back to JS-side `applyMergePatch()` otherwise
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:00.429Z
Learning: Applies to src/core/sqlite-db.ts : Probe for SQLite's `json_patch()` at engine construction time; use `json_patch(COALESCE(col, '{}'), ?)` when available, otherwise fall back to JS-side `applyMergePatch()`
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:32.320Z
Learning: Applies to src/sqlite-db.ts : Use JSON1 extension's `json_patch()` function when available; fall back to JS-side `applyMergePatch()` when unavailable.
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/sqlite-db.ts : Probe for SQLite's `json_patch()` at engine construction time and use the `hasJsonPatch` flag to conditionally optimize JSON updates.
📚 Learning: 2026-03-15T21:35:32.320Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:32.320Z
Learning: Applies to tests/unit/*.test.ts : Test JSON patch behavior (merged result) rather than implementation details (specific SQL functions).
Applied to files:
tests/unit/sqlite_db.test.tssrc/core/json-utils.tssrc/core/engine/wasm/WasmDatabaseEngine.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tsdocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdtests/unit/json_utils.test.tssrc/nativeWorker.ts
📚 Learning: 2026-04-04T19:34:42.846Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:34:42.846Z
Learning: Applies to tests/unit/**/*.test.ts : Test JSON patch behavior (merged result), not implementation (SQL function)
Applied to files:
tests/unit/sqlite_db.test.tssrc/core/json-utils.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tsdocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdtests/unit/json_utils.test.ts
📚 Learning: 2026-03-15T21:35:51.645Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:51.645Z
Learning: Applies to tests/unit/*.test.ts : Test JSON patch behavior (merged result) not implementation (SQL function) in unit tests
Applied to files:
tests/unit/sqlite_db.test.tssrc/core/json-utils.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tsdocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdtests/unit/json_utils.test.ts
📚 Learning: 2026-03-15T21:35:42.332Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:42.332Z
Learning: Applies to tests/unit/**/*.test.ts : Test JSON patch behavior (merged result) rather than implementation details (SQL function usage)
Applied to files:
tests/unit/sqlite_db.test.tssrc/core/json-utils.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tsdocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdtests/unit/json_utils.test.ts
📚 Learning: 2026-04-04T19:34:49.264Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:34:49.264Z
Learning: Applies to tests/unit/**/*.test.ts : Test behavior (merged result) not implementation (SQL function) for JSON patch functionality
Applied to files:
tests/unit/sqlite_db.test.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tsdocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdtests/unit/json_utils.test.tssrc/nativeWorker.ts
📚 Learning: 2026-03-15T21:35:28.282Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:28.282Z
Learning: Applies to tests/**/*.test.ts : Test JSON patch behavior (merged result) not implementation (SQL function) to ensure correctness
Applied to files:
tests/unit/sqlite_db.test.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tsdocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdtests/unit/json_utils.test.ts
📚 Learning: 2026-04-04T19:34:38.498Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:34:38.498Z
Learning: Applies to src/**/*.test.ts : Test JSON patch behavior (merged result) rather than implementation details (SQL function usage).
Applied to files:
tests/unit/sqlite_db.test.tstests/unit/nativeWorker.test.tstests/unit/json_utils.test.ts
📚 Learning: 2026-04-23T12:32:55.133Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/sqlite-db.ts : Use `json_patch()` SQLite function when available for cell updates (JSON1 extension); fall back to JS-side `applyMergePatch()` when unavailable
Applied to files:
tests/unit/sqlite_db.test.tssrc/core/json-utils.tssrc/core/engine/wasm/WasmDatabaseEngine.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tsdocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdsrc/nativeWorker.ts
📚 Learning: 2026-04-23T12:32:55.133Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/core/sqlite-db.ts : Probe for SQLite `json_patch()` at engine construction time to optimize `updateCell` and `updateCellBatch` operations
Applied to files:
tests/unit/sqlite_db.test.tssrc/core/json-utils.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mddocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdsrc/nativeWorker.ts
📚 Learning: 2026-04-23T12:32:55.133Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/sqlite-db.ts : Probe for SQLite's `json_patch()` at engine construction time and use `json_patch(COALESCE(col, '{}'), ?)` in single UPDATE when available instead of SELECT round-trip
Applied to files:
tests/unit/sqlite_db.test.tssrc/core/json-utils.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mddocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdsrc/nativeWorker.ts
📚 Learning: 2026-04-04T19:35:02.965Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:35:02.965Z
Learning: Applies to tests/unit/*.test.ts : Test behavior (merged result) not implementation when testing JSON patch operations
Applied to files:
tests/unit/sqlite_db.test.tstests/unit/nativeWorker.test.tstests/unit/json_utils.test.ts
📚 Learning: 2026-04-23T15:21:00.429Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:00.429Z
Learning: Applies to tests/unit/**/*.test.ts : Test behavior (merged result) not implementation when testing JSON patch operations
Applied to files:
tests/unit/sqlite_db.test.tstests/unit/nativeWorker.test.tstests/unit/json_utils.test.ts
📚 Learning: 2026-04-04T19:34:38.498Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:34:38.498Z
Learning: Use JSON Merge Patch (RFC 7396) for cell updates with `json_patch()` when available, falling back to JS-side `applyMergePatch()` from `json-utils.ts`.
Applied to files:
tests/unit/sqlite_db.test.tssrc/core/json-utils.tssrc/core/engine/wasm/WasmDatabaseEngine.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tsdocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdtests/unit/json_utils.test.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T15:21:03.123Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:03.123Z
Learning: Applies to src/core/json-utils.ts : Apply JSON Merge Patch (RFC 7396) using generate() and apply() from json-utils.ts for cell patch operations
Applied to files:
src/core/json-utils.tssrc/core/engine/wasm/WasmDatabaseEngine.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mddocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdtests/unit/json_utils.test.tssrc/nativeWorker.ts
📚 Learning: 2026-04-04T19:34:49.264Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:34:49.264Z
Learning: Applies to src/**/*.{ts,tsx} : Use JSON Patch (RFC 7396) via `applyMergePatch()` from `json-utils.ts` for cell updates when SQLite's `json_patch()` is unavailable
Applied to files:
src/core/json-utils.tssrc/core/engine/wasm/WasmDatabaseEngine.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mddocs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.mdsrc/nativeWorker.ts
📚 Learning: 2026-03-15T21:35:30.312Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:30.312Z
Learning: Applies to **/*.{ts,tsx,js} : Use JSON Merge Patch (RFC 7396) via `json-utils.ts` for applying incremental updates to JSON data
Applied to files:
src/core/json-utils.tstests/unit/json_utils.test.ts
📚 Learning: 2026-03-15T21:35:32.320Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:32.320Z
Learning: Applies to src/sqlite-db.ts : Use JSON1 extension's `json_patch()` function when available; fall back to JS-side `applyMergePatch()` when unavailable.
Applied to files:
src/core/json-utils.tssrc/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T12:32:55.133Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/core/sqlite-db.ts : Use `json_patch(COALESCE(col, '{}'), ?)` in a single UPDATE when JSON1 extension is available; fall back to JS-side `applyMergePatch()` otherwise
Applied to files:
src/core/json-utils.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T15:21:00.429Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:00.429Z
Learning: Applies to src/core/sqlite-db.ts : Probe for SQLite's `json_patch()` at engine construction time; use `json_patch(COALESCE(col, '{}'), ?)` when available, otherwise fall back to JS-side `applyMergePatch()`
Applied to files:
src/core/json-utils.tssrc/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-04T18:52:38.095Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T18:52:38.095Z
Learning: Applies to src/sqlite-db.ts : Fall back to JS-side `applyMergePatch()` from `json-utils.ts` when the JSON1 extension is unavailable
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T15:21:03.123Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:03.123Z
Learning: Applies to src/core/sqlite-db.ts : Fall back to JS-side applyMergePatch() from json-utils.ts when SQLite JSON1 extension is unavailable
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T15:21:22.670Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:22.670Z
Learning: Applies to src/**/*.ts : Check for SQLite `json_patch()` availability at engine construction time and use it in UPDATE statements when available, falling back to JS-side `applyMergePatch()` when unavailable
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T15:21:22.670Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:22.670Z
Learning: Applies to src/**/*.ts : Use `SAVEPOINT`/`RELEASE`/`ROLLBACK TO` instead of `BEGIN TRANSACTION` in `updateCellBatch` to safely handle nested transactions
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tssrc/nativeWorker.ts
📚 Learning: 2026-03-15T21:35:51.645Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:51.645Z
Learning: Applies to src/core/undo-history.ts : Track database modifications using `ModificationTracker` before saving to support undo/redo operations
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-04T19:35:02.965Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:35:02.965Z
Learning: Applies to src/core/undo-history.ts : Track database modifications in `ModificationTracker` before saving to enable undo/redo functionality
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T16:37:35.582Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T16:37:35.582Z
Learning: Applies to src/core/sqlite-db.ts : Use `SAVEPOINT`/`RELEASE`/`ROLLBACK TO` instead of `BEGIN TRANSACTION` in `updateCellBatch` to allow safe nesting within outer transactions
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tsdocs/superpowers/plans/2026-06-03-json-patch-undo-rmw.mdtests/unit/nativeWorker.test.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T16:37:35.582Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T16:37:35.582Z
Learning: Applies to src/core/sqlite-db.ts : Use SAVEPOINT/RELEASE/ROLLBACK TO instead of BEGIN TRANSACTION in updateCellBatch to ensure transaction safety when called from outer transactions
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T15:21:03.123Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:03.123Z
Learning: Applies to src/core/undo-history.ts : Track all database modifications in ModificationTracker from undo-history.ts for undo/redo support
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-04T18:52:25.579Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T18:52:25.579Z
Learning: Applies to src/databaseModel.ts : Implement DatabaseDocument with support for undo/redo via ModificationTracker and save/revert via VS Code API
Applied to files:
src/core/engine/wasm/WasmDatabaseEngine.tssrc/nativeWorker.ts
📚 Learning: 2026-04-23T12:32:55.133Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T12:32:55.133Z
Learning: Applies to src/nativeWorker.ts : Use `batched` IPC calls when executing multiple related SQL queries to reduce round-trip overhead (e.g., schema fetching via `queryBatch`).
Applied to files:
tests/unit/nativeWorker.test.ts
📚 Learning: 2026-03-15T21:35:42.332Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:35:42.332Z
Learning: Applies to src/**/*.ts : Probe for SQLite's `json_patch()` function at engine construction time and use `json_patch(COALESCE(col, '{}'), ?)` when available to avoid SELECT round-trips
Applied to files:
src/nativeWorker.ts
📚 Learning: 2026-04-23T15:21:22.670Z
Learnt from: CR
Repo: zknpr/SQLite-Explorer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-23T15:21:22.670Z
Learning: Applies to src/**/*.ts : Record database modifications in `ModificationTracker` via `recordModification()` before committing changes to track undo/redo history
Applied to files:
src/nativeWorker.ts
🪛 markdownlint-cli2 (0.22.1)
docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md
[warning] 43-43: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (9)
src/core/engine/wasm/WasmDatabaseEngine.ts (3)
27-27: LGTM!
284-321: LGTM!
323-356: LGTM!tests/unit/sqlite_db.test.ts (1)
247-398: LGTM!src/nativeWorker.ts (1)
41-41: LGTM!tests/unit/nativeWorker.test.ts (3)
18-18: LGTM!Also applies to: 28-28, 54-57
203-220: LGTM!
290-431: LGTM!tests/unit/json_utils.test.ts (1)
2-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winImport the VS Code mock setup first.
This file matches
tests/unit/*.test.ts, but the requiredtests/unit/vscode_mock_setup.tsbootstrap import is still missing from the top of the file. Please add it before the other imports so the unit-test environment stays consistent with the repo harness. As per coding guidelines, "Importtests/unit/vscode_mock_setup.tsat the beginning of unit test files to ensure VS Code mocks are initialized before running tests".⛔ Skipped due to learnings
Learnt from: CR Repo: zknpr/SQLite-Explorer PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T21:35:51.645Z Learning: Applies to tests/unit/*.test.ts : Test JSON patch behavior (merged result) not implementation (SQL function) in unit testsLearnt from: CR Repo: zknpr/SQLite-Explorer PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T21:35:32.320Z Learning: Applies to tests/unit/*.test.ts : Test JSON patch behavior (merged result) rather than implementation details (specific SQL functions).Learnt from: CR Repo: zknpr/SQLite-Explorer PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-04-04T19:34:42.846Z Learning: Applies to tests/unit/**/*.test.ts : Test JSON patch behavior (merged result), not implementation (SQL function)Learnt from: CR Repo: zknpr/SQLite-Explorer PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T21:35:42.332Z Learning: Applies to tests/unit/**/*.test.ts : Test JSON patch behavior (merged result) rather than implementation details (SQL function usage)Learnt from: CR Repo: zknpr/SQLite-Explorer PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-04-04T19:35:02.965Z Learning: Applies to tests/unit/*.test.ts : Test behavior (merged result) not implementation when testing JSON patch operationsLearnt from: CR Repo: zknpr/SQLite-Explorer PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-04-23T15:21:00.429Z Learning: Applies to tests/unit/**/*.test.ts : Test behavior (merged result) not implementation when testing JSON patch operationsLearnt from: CR Repo: zknpr/SQLite-Explorer PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-04-04T19:34:49.264Z Learning: Applies to tests/unit/**/*.test.ts : Test behavior (merged result) not implementation (SQL function) for JSON patch functionalityLearnt from: CR Repo: zknpr/SQLite-Explorer PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T21:35:28.282Z Learning: Applies to tests/**/*.test.ts : Test JSON patch behavior (merged result) not implementation (SQL function) to ensure correctnessLearnt from: CR Repo: zknpr/SQLite-Explorer PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-04-04T19:34:38.498Z Learning: Applies to src/**/*.test.ts : Test JSON patch behavior (merged result) rather than implementation details (SQL function usage).Learnt from: zknpr Repo: zknpr/SQLite-Explorer PR: 0 File: :0-0 Timestamp: 2026-05-31T20:57:58.186Z Learning: In the zknpr/SQLite-Explorer repository, unit tests in `tests/unit/` only import `./vscode_mock_setup` when they directly or transitively touch a `vscode`-importing module. Pure-logic tests (e.g., `threadPool.test.ts`, `rpc-protocol.test.ts`, `json-utils.test.ts`) intentionally omit the mock import. As of 2026-05, 13 of 35 unit test files do not import the mock. New test files that do not load any `vscode`-importing modules should NOT import `./vscode_mock_setup`.
![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: 80d821f712
ℹ️ 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".
|
|
||
| if (isObject(pv)) { | ||
| if (priorHas && isObject(priorVal)) { | ||
| const base = isObject(result[key]) ? (result[key] as Record<string, unknown>) : {}; |
There was a problem hiding this comment.
When a nested object that was patched has since been replaced by a scalar or array, this branch uses {} as the restore base and writes back only the keys present in the forward patch. For example, undoing a patch from {meta:{reviewed:false, owner:"ada"}} to {meta:{reviewed:true, owner:"ada"}} after a concurrent edit changed meta to "archived" produces {meta:{reviewed:false}}, dropping the untouched prior owner field instead of doing the same kind of safe replacement used for non-object current cells at the root. Since surgical restore is not possible once the nested current value is non-object, this should fall back to the prior subtree or full prior value rather than constructing a partial prior object.
Useful? React with 👍 / 👎.
| const read = await operationsFacade.executeQuery( | ||
| `SELECT ${escapeIdentifier(column)} FROM ${escapeIdentifier(targetTable)} WHERE rowid = ?`, | ||
| [rowIdNum] |
There was a problem hiding this comment.
In the native path this SELECT is a separate RPC from the later run/execBatch write, so if another database operation is queued while undoModification is awaiting the read, the worker can process that operation before the undo write is sent. In that case a sibling key added to the same JSON cell after this read is not included in computeJsonPatchUndo and is overwritten by the subsequent full-cell SET, leaving the native engine with the same clobbering race this change is meant to avoid. The read/compute/write needs to be serialized in one worker operation or otherwise guarded by a native-side transaction/mutex.
Useful? React with 👍 / 👎.
| if (!current || !forwardPatch || !prior) { | ||
| return { kind: 'replace' }; | ||
| } | ||
| return { kind: 'restore', value: JSON.stringify(restoreInto(current, forwardPatch, prior, 0)) }; |
There was a problem hiding this comment.
For JSON cells that contain large integers or high-precision numbers, this new undo path parses the entire current/prior documents into JavaScript numbers and then writes the whole document back with JSON.stringify. Even if the patch only touched a different key, a value like 9007199254740993 in an untouched sibling is rounded during this parse/stringify cycle, so undoing a JSON patch can corrupt data that the previous raw priorValue replacement preserved exactly. The restore should avoid reparsing/re-emitting untouched numeric tokens or fall back when exact JSON fidelity cannot be maintained.
Useful? React with 👍 / 👎.
… guard (review) Address PR #427 review of restoreInto/computeJsonPatchUndo: - CR3 (CodeRabbit, Major): plain object literals + spread let a JSON key like __proto__/constructor mutate the temp object's prototype. Build containers with Object.create(null) + hasOwnProperty guards so such keys are treated as data. - CX1 (Codex, P2): when a concurrent edit replaced a patched nested object with a scalar, restore built a partial object and dropped untouched prior siblings. Fall back to the full prior subtree when the current nested value is not an object. - CX3 (Codex, P2): RMW reparsed the whole cell, corrupting JSON integers > 2^53 even in untouched siblings (regression vs verbatim value-replace). hasPrecisionRiskyInteger detects them and value-replaces (byte-exact). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #427 review G1 (Gemini): the manual SAVEPOINT + per-cell updateCell loop duplicated atomicity that updateCellBatch already provides. Compute each cell's undo value, then write via a single updateCellBatch. sqlite_db tests cover the nested-object fallback and precision-risky-integer cases through the real engine. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…view) Address PR #427 review: - CX2 (Codex, P2): native json_patch undo read+write were separate worker RPCs, so a concurrent same-cell write could interleave and re-introduce the clobber. Add a per-connection promise-chain operation lock around the public operations; internal facade-to-facade calls use the raw (unlocked) implementation to avoid deadlock. Worker ops are already FIFO, so the lock only keeps each operation's messages contiguous. - G2 (Gemini): drop the hasJsonPatchUndo branch; one unified compute/write path. - CR4 (CodeRabbit): batch the per-cell SELECT reads into a single queryBatch round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review feedback addressed — all 9 findingsAcross 4 commits (
Note: automated reviewers run on PR open only, so these fixes won't trigger a re-review pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44f2b727c5
ℹ️ 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".
| const structural = raw.replace(/"(?:\\.|[^"\\])*"/g, '""'); | ||
| const digitRuns = structural.match(/\d{16,}/g); | ||
| return !!digitRuns && digitRuns.some(run => !Number.isSafeInteger(Number(run))); |
There was a problem hiding this comment.
Although the new guard checks long digit runs, it still lets valid JSON numbers such as 1e999 through because neither digit run has 16+ digits. In a json_patch undo where that number is an untouched sibling (for example current/prior contain {"huge":1e999,"a":2} and only a is being restored), JSON.parse turns it into Infinity and the restore path then writes it back with JSON.stringify as null, corrupting data that the previous full prior-value replacement preserved. The unsafe-number detection needs to reject exponent/overflow tokens before allowing the parse/stringify surgical restore.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 71cb539. hasPrecisionRiskyInteger → hasPrecisionRiskyNumber, which now also rejects any token that parses to a non-finite number (covers 1e999 → Infinity), so those cells value-replace and stay byte-exact. Engine test asserts a {"huge":1e999,...} cell survives a json_patch undo with huge unchanged (not null).
| operation: ModificationEntry['operation'] | ||
| ): Promise<CellValue> { | ||
| if (operation === 'json_patch') { | ||
| const currentValue = await this.readCellValue(table, rowId, column); |
There was a problem hiding this comment.
In the browser WASM path, src/workerFactory.ts exposes undoModification and updateCell by calling the in-process endpoint directly, so another public edit can run after this awaited read but before the later updateCell/updateCellBatch write. For example, if a json_patch undo reads {a:2,b:1} here and a grid/VFS edit then adds c before the undo continuation resumes, the undo writes the restored object computed from the stale read and drops c; the native path added a facade lock for this same read/compute/write sequence, but the WASM endpoint does not have an equivalent guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 96d0f89. Correct — the native CX2 lock didn't cover the WASM in-process path. Extracted the per-connection promise-chain lock into a shared serializeOperations() (Proxy over DatabaseOperations; passes engineKind/dispose through; deadlock-free since internal facade calls bypass it) and applied it to both facades: native (replacing the inline lock) and the WASM in-process facade in workerFactory. The worker-thread WASM path is one-RPC-per-op, so it's already atomic and isn't wrapped. Added a serializeOperations unit test (FIFO + error-release) alongside the existing native serialization test.
…view) Address PR #427 Codex re-review (json-utils.ts:216): the precision guard only checked 16+ digit integer runs, so an exponent like 1e999 passed; RMW then JSON.parse'd it to Infinity and wrote it back as null, corrupting an untouched sibling. Rename hasPrecisionRiskyInteger -> hasPrecisionRiskyNumber and also reject any token that parses to a non-finite number, so those cells value-replace (byte-exact). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ck (review) Address PR #427 Codex re-review (WasmDatabaseEngine.ts:327): the native CX2 fix did not cover the WASM in-process path, where workerFactory exposes undoModification and updateCell as direct endpoint calls, so a concurrent edit could still interleave between a json_patch undo's read and write. Extract the per-connection promise-chain lock into a shared serializeOperations() (Proxy over DatabaseOperations; passes through engineKind/dispose symbols; deadlock-free since internal facade calls bypass it) and apply it to BOTH facades: native (replacing the inline lock) and the WASM in-process facade in workerFactory. The worker-thread WASM path is one-RPC-per-op and needs no lock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96d0f8927f
ℹ️ 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".
| return tokens.some(tok => { | ||
| const n = Number(tok); | ||
| if (!Number.isFinite(n)) return true; | ||
| if (/^-?\d+$/.test(tok)) return !Number.isSafeInteger(n); |
There was a problem hiding this comment.
The fresh evidence in this revision is that hasPrecisionRiskyNumber only rejects integer tokens here; fractional tokens always return safe. When a json_patch undo touches a different key in a cell like {"precise":0.1234567890123456789012345,"a":2}, computeJsonPatchUndo takes the surgical path, JSON.parse rounds the untouched decimal, and JSON.stringify writes it back as 0.12345678901234568, corrupting data that the replace fallback would have preserved. The guard should also reject decimal tokens that cannot round-trip exactly before allowing the parse/stringify restore path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e0e6b3b. Replaced the integer-only check with a uniform round-trip test — a token is risky if non-finite or JSON.stringify(Number(tok)) !== tok — so large integers, exponent overflow (1e999), and high-precision decimals (0.1234567890123456789012345) all fall back to byte-exact value-replace in one rule. Pure + engine tests assert the decimal survives undo unrounded. Full suite 390/390 (no over-flag regression — canonical numbers still take the surgical path).
Address PR #427 Codex round-3 (json-utils.ts:219): the precision guard accepted high-precision decimals like 0.1234567890123456789012345, which an RMW reparse rounds, corrupting an untouched sibling. Replace the integer-only check with a uniform round-trip test — a token is risky if non-finite or JSON.stringify(Number(tok)) !== tok — covering large integers, overflow, and high-precision decimals in one rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ive state reconcileRestoredDatabase replays forward or reverts saved-then-undone edits for WASM (the saved file is the checkpoint state); native is a no-op (on-disk file is already live). vscode-free, unit-tested against a real engine (incl. a json_patch revert exercising the #427 undo). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The docs/superpowers/ spec and plan files are internal brainstorming/implementation working notes, not project documentation, and should not be tracked. Removes both Part A's and the files inherited from #427; the design rationale lives in the PR descriptions and commit messages.
…ive state reconcileRestoredDatabase replays forward or reverts saved-then-undone edits for WASM (the saved file is the checkpoint state); native is a no-op (on-disk file is already live). vscode-free, unit-tested against a real engine (incl. a json_patch revert exercising the #427 undo). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Part 1 of 2 for #425. Fixes the undo-fidelity sub-problem: undo of a JSON cell edit no longer clobbers a concurrent change to a different key of the same cell.
Undo previously replaced the whole cell with the recorded prior document (
SET col = priorValue). If a different key changed between the tracked edit and the undo — e.g. the cell is open in a VFS editor tab while it's also edited inline in the grid — that concurrent change was lost. P2 (data fidelity; single-pane/sequential undo was already correct).Approach — read-modify-write (RMW)
A pure helper
computeJsonPatchUndo(current, forwardPatch, prior)reads the cell's current value, restores only the keys the forward merge-patch touched (drawing values from the recorded prior), and writes the full object back; it value-replaces whencurrent/prior/forwardPatchis not a JSON object. All JSON reasoning happens in JS — nojson_patch()SQL on the undo path — which eliminates RFC 7396'snull=delete ambiguity andjson_patch()'s silent-wrong behavior on non-object inputs.This supersedes the closed PR #426, whose inverse-merge-patch approach was architecturally lossy and was closed with two unresolved P2 edges (nested explicit nulls; non-object current cell). RMW closes that entire class. It reuses #426's test scenarios, the
isObjectarrays-are-atomic fix (RFC 7396), and theoperation/newValuerecording plumbing already onmain.Behavior (acceptance scenarios)
{}).nullrestored exactly (including nested) while preserving concurrent siblings.NULL/ scalar / array / non-JSON text) → value-replace to prior.NULL→ value-replace.SAVEPOINT/ native singleexecBatch).Verification
npx tsc --noEmitclean ·node scripts/build.mjsOK (all four bundles +sqlite3.wasm) · full unit suite 375 / 375.s1/s4/s9); restoring → green.Scope
1.5.1ships with Part 2 (futureStack hot-exit persistence), the PR that closes Hot-exit does not persist redo/undone state (futureStack) — undo of a saved edit is lost on reopen #425. This PR references but does not close Hot-exit does not persist redo/undone state (futureStack) — undo of a saved edit is lost on reopen #425.Design + plan:
docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md,docs/superpowers/plans/2026-06-03-json-patch-undo-rmw.md.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests