Merge pull request #1135 from djabhi31/fix/director-scene-results-971

fix(director): keep scene results and save status accurate (#971)

changelog.d/1135-director-save-status-run-results.md

@@ -0,0 +1,5 @@
+- Scenes keep the "Scene not saved" status when a capture, update, append or
+  import cannot be saved, and an unreadable saved project no longer overwrites
+  the recovery checkpoint. `startScene()` resolves to its documented result,
+  `afterShotId` works without an explicit scene, and `scenesRun` counts the
+  distinct scenes queued (djabhi31, #1135).

src/scenes/director.js

@@ -189,6 +189,7 @@ export class SceneDirector {
       ...this._presentation,
       hasRun: !!this._lastRunJson,
     }));
+    if (!this._storageReadError) {
       this._bootstrapLegacyShotPacks();
       // Upgrade only already-installed packs; unrelated Scenes are untouched.
       for (const scene of this._project.scenes) {
@@ -206,6 +207,7 @@ export class SceneDirector {
           }
         }
       }
+    }
     this._initUI();
     this._visibilityUnsubscribe =
       this.dataManager.subscribeVisibilityRequests?.((change) => {
@@ -324,13 +326,14 @@ export class SceneDirector {
           'Scene not saved — existing saved project could not be read. Export edits or import a valid file.',
         ),
       );
-      return;
+      return false;
     }
     this._project.updatedAt = new Date().toISOString();
     try {
       const payload = JSON.stringify(this._project);
       parseSceneDocument(payload);
       localStorage.setItem(STORAGE_KEY, payload);
+      return true;
     } catch (e) {
       // Private browsing / block-all-cookies / quota-exceeded throws here. The
       // in-memory project stays usable this session, but persistence failed —
@@ -351,6 +354,7 @@ export class SceneDirector {
             })
           : undefined,
       );
+      return false;
     }
   }
 
@@ -923,12 +927,12 @@ export class SceneDirector {
     }
     this._selectedSceneId = scene.id;
     this._selectedShotId = appendedShots[0]?.id || this._selectedShotId;
-    this._saveProject();
+    const saved = this._saveProject();
     if (render) {
       this._renderSceneSelect();
       this._renderShotList();
     }
-    if (announce) {
+    if (announce && saved) {
       this._updateStatus(
         marker
           ? tn(
@@ -984,8 +988,9 @@ export class SceneDirector {
 
     scene.shots.push(shot);
     this._selectedShotId = shot.id;
-    this._saveProject();
+    const saved = this._saveProject();
     this._shotOutcome('shot-captured', scene, shot);
+    if (saved) {
       this._updateStatus(
         t('Captured: {scene} / {shot}', {
           scene: displaySceneTitle(scene),
@@ -993,6 +998,7 @@ export class SceneDirector {
         }),
       );
     }
+  }
 
   /**
    * Overwrite the currently selected shot's camera, visual, and layer states
@@ -1017,8 +1023,9 @@ export class SceneDirector {
     shot.visual = this.styleManager.getVisualState();
     shot.layers = this._captureLayerStates();
 
-    this._saveProject();
+    const saved = this._saveProject();
     this._shotOutcome('shot-updated', scene, shot);
+    if (saved) {
       this._updateStatus(
         t('Updated: {scene} / {shot}', {
           scene: displaySceneTitle(scene),
@@ -1026,6 +1033,7 @@ export class SceneDirector {
         }),
       );
     }
+  }
 
   /**
    * Delete a specific shot from a scene after user confirmation.
@@ -1755,13 +1763,13 @@ export class SceneDirector {
   ) {
     if (this._running) return { started: false, reason: 'already-running' };
 
-    let queue = this._buildPlaybackQueue(
+    const startSceneId =
-      sceneId || this._selectedSceneId || this._project.scenes[0]?.id,
+      sceneId || this._selectedSceneId || this._project.scenes[0]?.id;
-      { single },
+    let queue = this._buildPlaybackQueue(startSceneId, { single });
-    );
     if (afterShotId !== null) {
       const index = queue.findIndex(
-        ({ scene, shot }) => scene.id === sceneId && shot.id === afterShotId,
+        ({ scene, shot }) =>
+          scene.id === startSceneId && shot.id === afterShotId,
       );
       if (index < 0) return { started: false, reason: 'shot-not-found' };
       queue = queue.slice(index + 1);
@@ -1834,7 +1842,7 @@ export class SceneDirector {
       title: 'Editable Scene Run',
       startedAt: new Date().toISOString(),
       estimatedDurationSec,
-      scenesRun: queue.length,
+      scenesRun: new Set(queue.map(({ scene }) => scene.id)).size,
       events: [],
     };
 
@@ -1842,6 +1850,7 @@ export class SceneDirector {
     this._logEvent('scene_run_start', { count: queue.length });
     this._setPlaybackKeyboardEnabled(true);
 
+    let runResult = { started: true, shots: queue.length };
     try {
       await playSceneQueue(queue, {
         token,
@@ -1860,9 +1869,11 @@ export class SceneDirector {
       this._logEvent('scene_run_error', {
         message: error.message || 'unknown error',
       });
+      runResult = { started: false, reason: 'run-failed' };
     } finally {
       this._finishRun();
     }
+    return runResult;
   }
 
   /**
@@ -2090,9 +2101,9 @@ export class SceneDirector {
       this._selectedShotId =
         selection?.shotId || project.scenes[0]?.shots[0]?.id || null;
       this._loadedSceneId = null;
-      this._saveProject();
+      const saved = this._saveProject();
       this._publish({ type: 'project-imported', project });
-      this._updateStatus(t('Imported {file}', { file: file.name }));
+      if (saved) this._updateStatus(t('Imported {file}', { file: file.name }));
       return true;
     } catch (error) {
       if (

src/scenes/director.test.mjs

@@ -601,8 +601,12 @@ const PROJECT_FIXTURE = {
   }],
 };
 
-/** Stub the browser globals the director touches, headlessly. */
+/**
-function installSceneRuntime(project = PROJECT_FIXTURE) {
+ * Stub the browser globals the director touches, headlessly. `storage` replaces
+ * the seeded entries (key -> raw string); setting `localStorage.full` makes
+ * every write throw a quota error.
+ */
+function installSceneRuntime(project = PROJECT_FIXTURE, { storage } = {}) {
   const originalDocument = globalThis.document;
   const originalLocalStorage = globalThis.localStorage;
   const noopClassList = { add() {}, remove() {}, toggle() {}, contains: () => false };
@@ -614,10 +618,17 @@ function installSceneRuntime(project = PROJECT_FIXTURE) {
     removeEventListener() {},
     body: { classList: noopClassList, appendChild() {} },
   };
-  const stored = new Map([['godsEyeView.sceneProject.v2', JSON.stringify(project)]]);
+  const stored = new Map(storage
+    ? Object.entries(storage)
+    : [['godsEyeView.sceneProject.v2', JSON.stringify(project)]]);
   globalThis.localStorage = {
+    stored,
+    full: false,
     getItem: (key) => stored.get(key) ?? null,
-    setItem: (key, value) => stored.set(key, value),
+    setItem(key, value) {
+      if (this.full) throw new DOMException('quota exceeded', 'QuotaExceededError');
+      stored.set(key, value);
+    },
     removeItem: (key) => stored.delete(key),
   };
 
@@ -718,7 +729,8 @@ async function settle(turns = 8) {
 
 /** Build a director over doubles, with the fixture project loaded. */
 function makeDirector(options = {}) {
-  const restore = installSceneRuntime(options.project);
+  const restore = installSceneRuntime(options.project, { storage: options.storage });
+  const storage = globalThis.localStorage;
   const viewer = fakeViewer();
   const styleManager = fakeStyleManager(options.style);
   const dataManager = fakeDataManager(options.data);
@@ -728,7 +740,7 @@ function makeDirector(options = {}) {
   // Telemetry is only accumulated during a run; observable-failure assertions
   // need the accumulator without driving a whole run.
   director._activeRun = { events: [] };
-  return { director, viewer, styleManager, dataManager, restore };
+  return { director, viewer, styleManager, dataManager, storage, restore };
 }
 
 test('only Play Shot or a scene run grants transient media authority; LOAD, Stop and replacement revoke it', async () => {
@@ -1584,3 +1596,65 @@ test('camera refusal starts no authored frame or playback clock', async () => {
     assert.deepEqual(viewer.flights, []);
   } finally { restore(); }
 });
+
+test('an unreadable saved project does not overwrite its recovery checkpoint (#971)', async () => {
+  const unreadable = '{"version":99,"scenes":[]}';
+  const warn = console.warn;
+  console.warn = () => {};
+  const { director, storage, restore } = makeDirector({ storage: {
+    'godsEyeView.sceneProject.v2': unreadable,
+    'godsEyeView.sceneProject.checkpoint.v1': 'EARLIER-CHECKPOINT',
+  } });
+  try {
+    assert.equal(storage.stored.get('godsEyeView.sceneProject.v2'), unreadable);
+    assert.equal(storage.stored.get('godsEyeView.sceneProject.checkpoint.v1'), 'EARLIER-CHECKPOINT');
+  } finally { console.warn = warn; await director.destroy(); restore(); }
+});
+
+test('a capture, update or import that cannot be saved keeps the "Scene not saved" status (#971)', async () => {
+  const warn = console.warn;
+  console.warn = () => {};
+  const { director, storage, restore } = makeDirector();
+  let status = '';
+  director.subscribe(({ state }) => { status = state.status; });
+  try {
+    storage.full = true;
+    director._selectedSceneId = 'scene-1';
+    director._selectedShotId = 'shot-a';
+    director.captureShot();
+    assert.match(status, /^Scene not saved/, 'failed capture keeps the save failure');
+    director.updateSelectedShot();
+    assert.match(status, /^Scene not saved/, 'failed update keeps the save failure');
+    assert.equal(await director.importProjectFile(
+      { name: 'fixture.json' },
+      { prepared: { project: structuredClone(PROJECT_FIXTURE), assets: [] } },
+    ), true);
+    assert.match(status, /^Scene not saved/, 'failed import keeps the save failure');
+  } finally { console.warn = warn; await director.destroy(); restore(); }
+});
+
+test('startScene resolves its result, resumes after a shot without a scene id, and counts distinct scenes (#971)', async () => {
+  const { director, restore } = makeDirector();
+  try {
+    const options = { single: true, preview: false };
+    assert.deepEqual(await director.startScene('scene-1', options), { started: true, shots: 2 });
+    assert.equal(director._lastRun.scenesRun, 1, 'one scene with two shots is one scene run');
+    assert.deepEqual(
+      await director.startScene(undefined, { ...options, afterShotId: 'shot-a' }),
+      { started: true, shots: 1 },
+    );
+  } finally { await director.destroy(); restore(); }
+});
+
+test('a caught run failure resolves as run-failed, not success (#971)', async () => {
+  const { director, styleManager, restore } = makeDirector();
+  const warn = console.warn;
+  console.warn = () => {};
+  styleManager.applyVisualState = async () => { throw new Error('visual failed'); };
+  try {
+    assert.deepEqual(
+      await director.startScene('scene-1', { single: true, preview: false }),
+      { started: false, reason: 'run-failed' },
+    );
+  } finally { console.warn = warn; await director.destroy(); restore(); }
+});