feat-vm-snapshot-api #30
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR introduces a VM snapshot system enabling serialization and deserialization of Scratch VM runtime state—including targets, threads, monitors, and IO devices. It adds a snapshot module with serialize/restore functionality and three public VirtualMachine methods (takeSnapshot, readSnapshot, loadSnapshot) for capturing and restoring VM state. Comprehensive unit tests validate snapshot roundtrip behavior with clones, variables, and thread contexts. Changes
Sequence DiagramssequenceDiagram
actor User
participant VM as VirtualMachine
participant Snap as snapshotSerializer
participant Runtime as Runtime State
User->>VM: takeSnapshot(options)
VM->>Snap: serialize(vm, options)
Snap->>Runtime: Access targets, threads, monitors, IO
Runtime-->>Snap: State data
Snap->>Snap: Convert complex types<br/>(Set, Map, Timer, etc.)
Snap-->>VM: snapshot object
VM->>VM: emit SNAPSHOT_TAKEN
VM-->>User: snapshot data
sequenceDiagram
actor User
participant VM as VirtualMachine
participant Snap as snapshotSerializer
participant Runtime as Runtime State
User->>VM: loadSnapshot(snapshotData, options)
VM->>VM: readSnapshot(snapshotData)
VM->>Snap: validateSnapshot(snapshot)
Snap-->>VM: validation result
VM->>Snap: restore(vm, snapshot, options)
Snap->>Snap: Deserialize & reconstruct<br/>complex types
Snap->>Runtime: Apply targets, clones,<br/>variables, monitors, IO
Runtime-->>Snap: State updated
Snap-->>VM: restoration complete
VM->>VM: emit SNAPSHOT_LOADED
VM-->>User: completion
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related Issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/serialization/snapshot.js (5)
25-28: Consider more robust immutable.js detection.The current check relies on the presence of
toJSandtoJSONmethods, which could match non-immutable objects. Consider usingImmutable.isImmutable()if available, or check for a more specific marker.🔎 Suggested improvement
+const Immutable = require('immutable'); + // immutable.js -if (value && typeof value.toJS === 'function' && typeof value.toJSON === 'function') { +if (Immutable.isImmutable && Immutable.isImmutable(value)) { return {__type: 'immutable', value: value.toJS()}; }
117-125: Clone detection logic may miss edge cases.The clone detection checks
!target.isStage && !target.isOriginal && target.sprite.clones[0], butclones[0]being truthy doesn't guarantee it's a different target than the current one. If the original was deleted but clones remain,clones[0]might be the clone itself.🔎 Safer clone detection
const isClone = ( !target.isStage && !target.isOriginal && target.sprite && target.sprite.clones && - target.sprite.clones[0] + target.sprite.clones[0] && + target.sprite.clones[0] !== target );
210-244: Reliance on private IO device properties.The code accesses internal properties like
_keysPressed,_clientX,_buttons, etc. While necessary for complete state capture, these are implementation details that could change. Consider adding version guards or defensive checks.
458-464: warpTimer restoration uses Date.now() directly, inconsistent with other timer handling.The warpTimer is created with
Date.now()as the time source, while elsewhere in the code (line 77-86 indeserializeAny), timers can be restored with eitherDateorruntime.currentMSecs. For consistency, consider using the same pattern.🔎 Consistent timer restoration
if (state.warpTimer && typeof state.warpTimer.elapsed === 'number') { - const timer = new Timer(); - timer.startTime = Date.now() - state.warpTimer.elapsed; + const timer = new Timer({now: () => runtime.currentMSecs}); + timer.startTime = runtime.currentMSecs - state.warpTimer.elapsed; thread.warpTimer = timer; } else { thread.warpTimer = null; }
605-607: Linear search in reordering loop has O(n²) complexity.Using
reorderedTargets.includes(t)inside a loop overruntime.targetsresults in O(n²) complexity. For large numbers of targets, consider using a Set for O(1) lookups.🔎 Use Set for O(n) reordering
+ const reorderedSet = new Set(reorderedTargets); for (const t of runtime.targets) { - if (!reorderedTargets.includes(t)) reorderedTargets.push(t); + if (!reorderedSet.has(t)) reorderedTargets.push(t); }test/unit/vm_snapshot_state.js (1)
155-163: Consider adding thread target verification robustness.The test asserts
restoredThread.target.idis'clone', but doesn't verify that this is actually the restored clone (not the original that was mutated). Consider adding a check that the clone is a distinct target.🔎 Enhanced thread target verification
// Threads t.equal(vm.runtime.threads.length, 1); const restoredThread = vm.runtime.threads[0]; t.equal(restoredThread.target.id, 'clone'); + t.equal(restoredThread.target.isOriginal, false, 'thread target is a clone'); t.same(restoredThread.stack, ['top', 'next']);src/virtual-machine.js (1)
737-746: JSON parsing errors are not handled.
JSON.parseat lines 739 and 741 can throwSyntaxErrorfor malformed input. The caller would need to handle this, but it might be cleaner to wrap and rethrow with a more descriptive error message.🔎 Add error handling for JSON parsing
readSnapshot (snapshotData) { + try { if (typeof snapshotData === 'string') { snapshotData = JSON.parse(snapshotData); } else if (snapshotData instanceof ArrayBuffer || ArrayBuffer.isView(snapshotData)) { snapshotData = JSON.parse(Buffer.from(snapshotData).toString()); } + } catch (e) { + throw new Error(`Invalid snapshot data: ${e.message}`); + } snapshotSerializer.validateSnapshot(snapshotData); return snapshotData; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/serialization/snapshot.jssrc/virtual-machine.jstest/unit/vm_snapshot_state.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/virtual-machine.js (1)
src/serialization/snapshot.js (1)
snapshot(492-515)
test/unit/vm_snapshot_state.js (2)
src/serialization/snapshot.js (11)
require(1-1)MonitorRecord(3-3)Thread(4-4)Timer(5-5)t(19-19)t(68-68)target(413-413)thread(416-416)timer(79-83)timer(459-459)snapshot(492-515)src/virtual-machine.js (7)
require(20-20)require(24-24)require(25-25)require(26-26)require(28-28)Sprite(17-17)Variable(21-21)
🔇 Additional comments (9)
src/serialization/snapshot.js (3)
1-9: LGTM on imports and constants.The module structure is clean with well-defined constants for snapshot type and version, enabling future versioning support.
472-485: LGTM on validation logic.Clear, descriptive error messages for invalid snapshots. The validation appropriately checks type, version, and runtimeState presence.
102-104: No changes needed. The deserialization behavior is correct.The
__type: 'immutable'handler returns plain JS objects (value.value), which is appropriate because_customState,_edgeActivatedHatValues, andextensionStorageare always initialized and used as plain objects throughout the codebase (initialized as{}inengine/target.jsand accessed via bracket notation, never immutable methods). The serialization/deserialization conversion from immutable.js structures to plain JS is intentional and consistent with how these fields are actually used—consumers do not expect immutable methods on these deserialized fields.Likely an incorrect or invalid review comment.
test/unit/vm_snapshot_state.js (3)
12-29: LGTM on test setup.Good coverage of minimal project setup with stage and sprite. The test properly initializes the VM runtime with targets.
30-55: Good coverage of target state variations.The test sets up distinct values for the clone vs. original, enabling proper verification that each target's state is restored independently.
76-92: LGTM on thread state setup.Good coverage of thread serialization including custom Timer in executionContext. This exercises the Timer deserialization path with a runtime-based time source.
src/virtual-machine.js (3)
30-30: LGTM on import.Clean import of the snapshot serializer module.
715-729: LGTM on takeSnapshot implementation.Clean API with proper event emission. The JSDoc clearly documents the purpose and options.
748-763: LGTM on loadSnapshot implementation.Clean async implementation with proper event emission. The method correctly delegates to readSnapshot for validation and returns a Promise for the async restore operation.
| if (typeof ioState.video.skinId === 'number' && video._skinId !== -1) { | ||
| video._skinId = ioState.video.skinId; | ||
| } | ||
| if (typeof ioState.video.drawable === 'number' && video._drawable !== -1) { | ||
| video._drawable = ioState.video.drawable; | ||
| } |
There was a problem hiding this comment.
Video skinId/drawable restoration may cause inconsistencies.
The code only sets skinId and drawable if they're not -1, but the stored values from a different session may not correspond to valid renderer resources. This could lead to invalid drawable references.
🔎 Consider skipping video resource restoration entirely
if (ioState.video) {
const video = runtime.ioDevices.video;
video._ghost = +ioState.video.ghost || 0;
video._forceTransparentPreview = !!ioState.video.forceTransparentPreview;
- // Do not attempt to set skinId/drawable unless they already exist,
- // as these are renderer-specific resources.
- if (typeof ioState.video.skinId === 'number' && video._skinId !== -1) {
- video._skinId = ioState.video.skinId;
- }
- if (typeof ioState.video.drawable === 'number' && video._drawable !== -1) {
- video._drawable = ioState.video.drawable;
- }
+ // Skip restoring skinId/drawable as they are renderer-session-specific
if (typeof video.setPreviewGhost === 'function') {
video.setPreviewGhost(video._ghost);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (typeof ioState.video.skinId === 'number' && video._skinId !== -1) { | |
| video._skinId = ioState.video.skinId; | |
| } | |
| if (typeof ioState.video.drawable === 'number' && video._drawable !== -1) { | |
| video._drawable = ioState.video.drawable; | |
| } | |
| if (ioState.video) { | |
| const video = runtime.ioDevices.video; | |
| video._ghost = +ioState.video.ghost || 0; | |
| video._forceTransparentPreview = !!ioState.video.forceTransparentPreview; | |
| // Skip restoring skinId/drawable as they are renderer-session-specific | |
| if (typeof video.setPreviewGhost === 'function') { | |
| video.setPreviewGhost(video._ghost); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/serialization/snapshot.js around lines 274 to 279, restoring
video._skinId and video._drawable directly from ioState can create invalid
references; instead validate the stored IDs against the renderer/resource
manager before assigning: query the appropriate resource lookup (e.g.
hasSkin/hasDrawable or getSkin/getDrawable) and only assign if the resource
exists and is compatible, otherwise set the property to -1 (or a safe default)
and optionally log a warning; alternatively, if restoration of renderer
resources is undesirable, skip restoring these fields entirely and leave them at
their runtime defaults.
| runtime.addTarget(newClone); | ||
| newClone.id = targetState.id; |
There was a problem hiding this comment.
Clone ID reassignment after addTarget may cause inconsistencies.
The clone is added to runtime with its auto-generated ID, then the ID is changed to match the snapshot. This could cause issues if addTarget uses the ID for internal indexing (e.g., in maps or caches).
🔎 Set ID before adding to runtime
const newClone = original.makeClone();
if (!newClone) {
log.warn('Could not restore clone: makeClone() returned null', targetState);
continue;
}
+ newClone.id = targetState.id;
runtime.addTarget(newClone);
- newClone.id = targetState.id;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runtime.addTarget(newClone); | |
| newClone.id = targetState.id; | |
| newClone.id = targetState.id; | |
| runtime.addTarget(newClone); |
🤖 Prompt for AI Agents
In src/serialization/snapshot.js around lines 590-591, the code assigns
newClone.id after calling runtime.addTarget which can cause runtime indexing
inconsistencies; set newClone.id to targetState.id before calling
runtime.addTarget (or construct the clone with the correct id) so the runtime
receives the object with the intended id, and verify no duplicate-id errors
occur (handle or assert if runtime already contains that id).
1 participant
This PR was automatically generated. Git text generation failed after retries. The changes in this PR were made by the engine, but we were unable to generate a detailed description.
Powered by CTO.new