-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: preserve scene material document state #560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import { describe, expect, test } from 'bun:test' | ||
| import { apiGraphSchema } from './graph-schema' | ||
|
|
||
| const material = { | ||
| id: 'mat_accent', | ||
| name: 'Accent', | ||
| material: { | ||
| texture: { url: 'https://cdn.example.com/accent.png' }, | ||
| }, | ||
| } | ||
|
|
||
| describe('apiGraphSchema', () => { | ||
| test('preserves validated scene materials', () => { | ||
| const graph = apiGraphSchema.parse({ | ||
| nodes: {}, | ||
| rootNodeIds: [], | ||
| materials: { mat_accent: material }, | ||
| }) | ||
|
|
||
| expect(graph.materials?.mat_accent).toMatchObject(material) | ||
| }) | ||
|
|
||
| for (const url of ['file:///private/key', 'javascript:alert(1)']) { | ||
| test(`rejects unsafe material texture URL: ${url}`, () => { | ||
| const result = apiGraphSchema.safeParse({ | ||
| nodes: {}, | ||
| rootNodeIds: [], | ||
| materials: { | ||
| mat_accent: { | ||
| ...material, | ||
| material: { texture: { url } }, | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| expect(result.success).toBe(false) | ||
| }) | ||
| } | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { describe, expect, test } from 'bun:test' | ||
| import { | ||
| isRemoteSceneEcho, | ||
| sceneGraphSignature, | ||
| type PersistedSceneGraph, | ||
| } from './scene-sync' | ||
|
|
||
| function makeGraph(materialName = 'Accent'): PersistedSceneGraph { | ||
| return { | ||
| nodes: {}, | ||
| rootNodeIds: [], | ||
| materials: { | ||
| mat_accent: { | ||
| id: 'mat_accent', | ||
| name: materialName, | ||
| material: { preset: 'custom', properties: { color: '#123456' } }, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| describe('isRemoteSceneEcho', () => { | ||
| test('recognizes only the exact remote graph', () => { | ||
| const remote = makeGraph() | ||
|
|
||
| expect(isRemoteSceneEcho(sceneGraphSignature(remote), sceneGraphSignature(remote))).toBe(true) | ||
| }) | ||
|
|
||
| test('does not suppress a local material change', () => { | ||
| const remote = makeGraph() | ||
| const local = makeGraph('Updated accent') | ||
|
|
||
| expect(isRemoteSceneEcho(sceneGraphSignature(remote), sceneGraphSignature(local))).toBe(false) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import type { SceneGraph } from '@pascal-app/editor' | ||
|
|
||
| export type PersistedSceneGraph = SceneGraph & { | ||
| collections?: Record<string, unknown> | ||
| } | ||
|
|
||
| export function sceneGraphSignature(graph: PersistedSceneGraph): string { | ||
| return JSON.stringify({ | ||
| nodes: graph.nodes, | ||
| rootNodeIds: graph.rootNodeIds, | ||
| collections: graph.collections, | ||
| materials: graph.materials, | ||
| installedPlugins: graph.installedPlugins, | ||
| }) | ||
| } | ||
|
|
||
| export function isRemoteSceneEcho( | ||
| lastRemoteGraphJson: string | null, | ||
| graphJson: string, | ||
| ): boolean { | ||
| return lastRemoteGraphJson === graphJson | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,8 @@ export type ActiveSceneMeta = Pick< | |
| 'id' | 'name' | 'projectId' | 'ownerId' | 'thumbnailUrl' | 'version' | ||
| > | ||
|
|
||
| type SetSceneExtra = Parameters<ReturnType<typeof useScene.getState>['setScene']>[2] | ||
|
|
||
| /** | ||
| * Headless bridge to the `@pascal-app/core` Zustand store. | ||
| * | ||
|
|
@@ -59,8 +61,12 @@ export class SceneBridge { | |
| } | ||
|
|
||
| /** Replace entire scene (undoable via Zundo). */ | ||
| setScene(nodes: Record<AnyNodeId, AnyNode>, rootNodeIds: AnyNodeId[]): void { | ||
| useScene.getState().setScene(nodes, rootNodeIds) | ||
| setScene( | ||
| nodes: Record<AnyNodeId, AnyNode>, | ||
| rootNodeIds: AnyNodeId[], | ||
| extra?: SetSceneExtra, | ||
| ): void { | ||
| useScene.getState().setScene(nodes, rootNodeIds, extra) | ||
| } | ||
|
|
||
| /** Full snapshot for export, including collections. */ | ||
|
|
@@ -72,6 +78,7 @@ export class SceneBridge { | |
| nodes: state.nodes, | ||
| rootNodeIds: state.rootNodeIds, | ||
| collections: state.collections ?? {}, | ||
| materials: state.materials, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MCP save drops materialsHigh Severity
Reviewed by Cursor Bugbot for commit 9970b0c. Configure here. |
||
| ...(state.hasExplicitPluginInstallState || state.installedPlugins.length > 0 | ||
| ? { installedPlugins: state.installedPlugins } | ||
| : {}), | ||
|
|
@@ -119,13 +126,23 @@ export class SceneBridge { | |
| } | ||
| } | ||
|
|
||
| this.setScene(nodes as Record<AnyNodeId, AnyNode>, rootNodeIds as AnyNodeId[]) | ||
| if (Array.isArray(obj.installedPlugins)) { | ||
| useScene.getState().setInstalledPlugins( | ||
| obj.installedPlugins.filter((id): id is string => typeof id === 'string'), | ||
| { explicit: true }, | ||
| ) | ||
| } | ||
| const collections = | ||
| obj.collections && typeof obj.collections === 'object' && !Array.isArray(obj.collections) | ||
| ? (obj.collections as NonNullable<SetSceneExtra>['collections']) | ||
| : undefined | ||
| const materials = | ||
| obj.materials && typeof obj.materials === 'object' && !Array.isArray(obj.materials) | ||
| ? (obj.materials as NonNullable<SetSceneExtra>['materials']) | ||
| : undefined | ||
| const installedPlugins = Array.isArray(obj.installedPlugins) | ||
| ? obj.installedPlugins.filter((id): id is string => typeof id === 'string') | ||
| : undefined | ||
|
|
||
| this.setScene(nodes as Record<AnyNodeId, AnyNode>, rootNodeIds as AnyNodeId[], { | ||
| ...(collections && { collections }), | ||
| ...(materials && { materials }), | ||
| ...(installedPlugins && { installedPlugins, hasExplicitPluginInstallState: true }), | ||
| }) | ||
| } | ||
|
|
||
| /** Read a single node, or `null` if not present. */ | ||
|
|
||


There was a problem hiding this comment.
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.graphbeforeapplySceneGraphToEditor, while autosave signs the store graph aftersetScenemigration 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)
apps/editor/lib/scene-sync.ts#L6-L14Reviewed by Cursor Bugbot for commit 9970b0c. Configure here.