Skip to content

fix: preserve scene material document state - #560

Closed
ShiroKSH wants to merge 1 commit into
pascalorg:mainfrom
ShiroKSH:fix/preserve-scene-materials
Closed

fix: preserve scene material document state#560
ShiroKSH wants to merge 1 commit into
pascalorg:mainfrom
ShiroKSH:fix/preserve-scene-materials

Conversation

@ShiroKSH

@ShiroKSH ShiroKSH commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Preserve custom materials across API saves, clone/fork, SQLite, MCP import/export, and live sync.
  • Preserve collections and installed plugins at the bridge and storage boundaries.
  • Suppress only an exact live-event echo, so a following local edit is saved.

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 build
  • bun run --cwd packages/mcp build
  • Targeted app TypeScript check for scene-sync.ts and scene-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: materials are now part of the core SceneGraph clone/fork path (deep-cloned on duplicate), validated on API (apiGraphSchema with SceneMaterial, including unsafe texture URL rejection) and SQLite graph parsing. MCP SceneBridge exportJSON / loadJSON and setScene extras round-trip collections, materials, and explicit plugin installs in one load.

Live sync: Shared scene-sync helpers build a signature that includes materials (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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

❌ 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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9970b0c. Configure here.


versionRef.current = payload.version
lastRemoteGraphJsonRef.current = sceneGraphSignature(payload.graph)
suppressRemoteSaveUntilRef.current = Date.now() + 2500

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9970b0c. Configure here.

@Aymericr Aymericr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 unknown materials key. Parsing a graph with materials on main returns keys ["nodes","rootNodeIds"].
  • packages/mcp/src/storage/sqlite-scene-store.tssave() then load() on main returns ["nodes","rootNodeIds"] only; collections, materials and installedPlugins are all gone.
  • packages/mcp/src/bridge/scene-bridge.tsloadJSON on main drops collections entirely (collections = {} after a round trip). Routing everything through one setScene(nodes, roots, extra) call is the right shape.
  • packages/core/src/utils/clone-scene-graph.ts — no materials in the SceneGraph type 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.

@Aymericr

Aymericr commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Landed in #597 (2e526e91) — thank you @ShiroKSH, and sorry it took a while to get to. You're credited as co-author on the commit.

Your diagnosis held up completely. I probed all eight boundaries on main before touching anything and every one of them dropped the palette, exactly as you described. The call to fix it in packages/core and packages/mcp rather than only in apps/editor is the part that mattered most: the hosted app forks scenes through @pascal-app/core/clone-scene-graph, so an app-only fix would have left both npm consumers and production still broken.

Two things fell out while wiring up the round trip that weren't in this PR:

loadJSON was dropping non-empty collections too, for a different reason than materials. It applied plugin state in a second call after setScene, and setScene overwrites collections and materials with {} whenever they aren't in its extra bag — so anything applied afterwards was discarded. Everything goes in one call now, which also fixes dirty-tracking for plugin-owned nodes (setScene marks every node dirty at the end, and markDirty returns early for a node whose plugin isn't installed yet).

The echo-suppression signature omitting materials was losing user edits, not just noise. A local change that touched only the palette signed identically to the last remote payload, so scene-loader.tsx read it as an echo and skipped the save. Verified against main's signature function — byte-identical output for a changed material.

What I changed from this PR

  • Kept the 2.5s suppressRemoteSaveUntilRef timer. It's still the fallback for the window before the first remote payload arrives, where there's no signature to compare against.
  • Moved material validation from the SQLite read path to the API write path. Putting SceneMaterial on read looked right, but nothing validates on write and parseGraph throws — so one odd stored value would make a saved scene permanently unloadable rather than merely odd. There's a test now that saves a material with an ftp:// texture and asserts it still loads. The allowlist is enforced in graph-schema.ts instead, in superRefine so it validates without transforming: the routes persist that schema's output, and SceneMaterial injects MaterialProperties defaults and strips unknown keys, so a validating shape there would make every save silently rewrite the caller's palette (measured: a sparse material grew from 1 property to 6).
  • Dropped the one-line isRemoteSceneEcho wrapper, and extracted scene-signature.ts so the echo logic is testable.
  • Rebased onto the 1.0-beta CHANGELOG.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants