fix: preserve scene material document state - #560
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9970b0c. Configure here.
| nodes: state.nodes, | ||
| rootNodeIds: state.rootNodeIds, | ||
| collections: state.collections ?? {}, | ||
| materials: state.materials, |
There was a problem hiding this comment.
MCP save drops materials
High Severity
SceneBridge.exportJSON now includes materials, but SceneOperations.exportSceneGraph still returns only nodes, roots, collections, and plugins. MCP save_scene and live sync publish through exportSceneGraph, so custom scene materials are dropped on persist even though import/export round-trips them in tests.
Reviewed by Cursor Bugbot for commit 9970b0c. Configure here.
|
|
||
| versionRef.current = payload.version | ||
| lastRemoteGraphJsonRef.current = sceneGraphSignature(payload.graph) | ||
| suppressRemoteSaveUntilRef.current = Date.now() + 2500 |
There was a problem hiding this comment.
Remote echo uses pre-apply graph
Medium Severity
Live sync stores the echo signature from the SSE payload.graph before applySceneGraphToEditor, while autosave signs the store graph after setScene migration and cleanup. Those graphs often differ, so exact echo fails and the removed 2.5s save suppress no longer blocks redundant PUTs after remote updates.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9970b0c. Configure here.
Aymericr
left a comment
There was a problem hiding this comment.
Thanks for this — the underlying bug is real and I reproduced it on main at every boundary you touched, which is more than most reports of this kind survive:
apps/editor/lib/graph-schema.ts— zod.object()silently strips the unknownmaterialskey. Parsing a graph with materials on main returns keys["nodes","rootNodeIds"].packages/mcp/src/storage/sqlite-scene-store.ts—save()thenload()on main returns["nodes","rootNodeIds"]only; collections, materials and installedPlugins are all gone.packages/mcp/src/bridge/scene-bridge.ts—loadJSONon main drops collections entirely (collections = {}after a round trip). Routing everything through onesetScene(nodes, roots, extra)call is the right shape.packages/core/src/utils/clone-scene-graph.ts— nomaterialsin theSceneGraphtype or either clone path.
I also want to credit the layering: these fixes land in packages/core and packages/mcp, not just the app. That matters, because the hosted app forks scenes through @pascal-app/core/clone-scene-graph, so a fix confined to apps/editor would have left both npm consumers and production broken. The structuredClone non-identity assertion at clone-scene-graph.test.ts:74-80 is exactly the right test. And your "83 passed" is accurate — I reproduced it byte for byte.
Three things need to change before this can go in.
1. Echo suppression is now dead, not narrowed (apps/editor/components/scene-loader.tsx:153). You sign the raw SSE payload.graph, then line 154 applies it via setScene, which unconditionally writes materials (packages/core/src/store/use-scene.ts:1371) even when the payload had no materials key. One side of the compare serializes "materials":{}, the other omits it, so the strings never match. MCP live-sync emits exactly such a payload — packages/mcp/src/operations/scene-operations.ts:147 builds only nodes/rootNodeIds/collections/installedPlugins. I ran main's signature function and yours against that identical payload: main suppressed the echo (true), yours did not (false). Since suppressRemoteSaveUntilRef is gone, there is no fallback, so every remote update now fires a redundant PUT and bumps the version.
Fix: default the fields in scene-sync.ts:8-14 (graph.materials ?? {}, collections ?? {}, installedPlugins ?? []) and capture lastRemoteGraphJsonRef from store state after applySceneGraphToEditor, not from payload.graph. I verified the normalized form makes the MCP payload match the post-setScene store signature.
2. The MCP persist path still drops materials, so "across API saves … and live sync" isn't true yet. packages/mcp/src/operations/scene-operations.ts:147 hand-copies four fields and omits materials; both save_scene (tools/scene-lifecycle/save-scene.ts:87) and publishLiveSceneSnapshot (tools/live-sync.ts:34) go through it. Probe: exportJSON() returns materials, exportSceneGraph() returns ["nodes","rootNodeIds","collections","installedPlugins"]. One line — materials: exported.materials — plus a test on exportSceneGraph (the current bridge test only covers exportJSON). Worth adding materials to getSceneOutput in tools/get-scene.ts too, since #575 just tightened those schemas.
3. SceneMaterial in the SQLite read schema can permanently brick a saved scene (sqlite-scene-store.ts:78-79). Nothing validates on write, but parseGraph throws SceneInvalidError on any mismatch. I saved a graph whose material had texture.url: "ftp://host/a.png" — save() succeeded at v1, and load() then threw forever. Same outcome for name: 3, missing name, material: null, roughness: 1.5, and installedPlugins: ['']. Zod also strips unknown material fields (maps, and anything future) and injects MaterialProperties defaults on read, so a load→save cycle rewrites stored data (219 → 296 bytes in my probe). That's a behavior change the description doesn't mention.
Fix: keep the read path permissive (z.record(z.string(), z.unknown()).optional()) so a stored row can never become unreadable, and move SceneMaterial validation to the write path inside save() before serializeGraph, where the caller can still react. If strict-on-read is what you want, safeParse per material and drop just the offending entry with a warning.
Two smaller items: bun run check fails on 5 biome errors in your own files (import sort + formatting in scene-sync.ts, scene-sync.test.ts, scene-loader.tsx) — main is clean and CI now gates on this, so bun run check:fix should do it. And isRemoteSceneEcho is a one-line === wrapper whose two tests only prove that === works; I'd inline it and re-point that test file at sceneGraphSignature behavior instead — a test asserting that a materials-less payload and the post-apply store produce the same signature would have caught item 1.
Not your fault, for the record: the check-types failures reproduce identically on main in my environment, and the conflicts in CHANGELOG.md / graph-schema.ts / graph-schema.test.ts are just a stale base — #490 and the 1.0-beta release landed after you branched. A rebase resolves them; the ## Unreleased section will need to sit above ## 1.0.0-beta.1 rather than ## 0.6.0.
Happy to take this as-is with items 1-3 addressed — the diagnosis and the layering are both right, it's the suppression rewrite and the read-side strictness that need to come back down.
|
Landed in #597 ( Your diagnosis held up completely. I probed all eight boundaries on Two things fell out while wiring up the round trip that weren't in this PR:
The echo-suppression signature omitting What I changed from this PR
If you want to keep going on this area, the natural follow-up is the palette UI: there's no way to rename or delete a scene material once it's authored, and nothing garbage-collects entries no node references anymore. Happy to review that one faster. |


Summary
Checks
bun test packages/core/src/utils/clone-scene-graph.test.ts packages/mcp/src/storage/sqlite-scene-store.test.ts packages/mcp/src/bridge/scene-bridge.test.ts apps/editor/lib/graph-schema.test.ts apps/editor/lib/scene-sync.test.ts— 83 passed.bun run --cwd packages/core buildbun run --cwd packages/mcp buildscene-sync.tsandscene-loader.tsx.Note
Medium Risk
Touches untrusted graph validation and multi-session save/sync behavior; changes are bounded by schema checks and targeted tests but affect how scene payloads are accepted and written.
Overview
Fixes loss of custom scene document state (materials, collections, installed plugins) when scenes are saved, cloned, imported/exported, or synced live.
Persistence & validation:
materialsare now part of the coreSceneGraphclone/fork path (deep-cloned on duplicate), validated on API (apiGraphSchemawithSceneMaterial, including unsafe texture URL rejection) and SQLite graph parsing. MCP SceneBridgeexportJSON/loadJSONandsetSceneextras round-trip collections, materials, and explicit plugin installs in one load.Live sync: Shared
scene-synchelpers build a signature that includesmaterials(and other persisted fields). The editor skips autosave only when the outgoing graph exactly matches the last applied remote event—replacing a broad time-based suppress—so a local edit right after a live update still saves.Reviewed by Cursor Bugbot for commit 9970b0c. Bugbot is set up for automated code reviews on this repo. Configure here.