From cd1d07d96ca1e9ebd64381ca5f52a20a5e186377 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Tue, 15 Sep 2026 22:45:53 +0300 Subject: [PATCH] fix: tour -- close the second funnel round 4 found, correct the overclaim Round 4's review confirmed the commit() guard genuinely closes every project-save path (keyboard shortcuts included), but caught the fix's own comment overreaching: "every mutating path in this module funnels through here" is false. store.js's shortcut/category/transition CRUD functions (saveShortcut, deleteShortcut, addCategory, importShortcuts, clearShortcuts, saveTransition, deleteTransition, importTransitions, clearTransitions) each call their own real, unmocked API.* endpoint directly and never touch commit() at all -- a second, separate funnel the guard doesn't cover. Currently dormant (no Settings > Shortcuts panel is wired up in v5 yet, confirmed by grep -- these functions have no caller anywhere outside store.js/api.js today), same shape as the saveClipToMediaBin landmine rounds 2-3 already closed pre-emptively rather than waiting for it to go live. These write GLOBAL state (the on-disk shortcut/transition library), not sandboxed per-project state like everything commit() guards, so if left open they'd be worse than a stray project PUT once a Shortcuts UI ships. Fixed with the same block-list pattern, and corrected both comments (store.js's commit() guard, tour.js's blocklist) to state their actual scope instead of the overclaim. Verified live: all nine now return the block toast instead of reaching Store's real implementation; full tour sequence still completes with zero thrown errors. Co-Authored-By: Claude Sonnet 5 --- app/legacy/store.js | 19 ++++++++++++------- app/legacy/tour.js | 32 ++++++++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/app/legacy/store.js b/app/legacy/store.js index f02cb2f6..615f0e66 100644 --- a/app/legacy/store.js +++ b/app/legacy/store.js @@ -969,16 +969,21 @@ } async function commit(opts = {}) { - // Every mutating path in this module funnels through here before it can - // reach a real network write -- scheduleSave()'s own setTimeout calls - // this closure-scoped function directly, so patching Store.commit (or - // any individual caller) from outside cannot stop it. The tour's own + // Every PROJECT-save path in this module funnels through here before it + // can reach a real network write -- scheduleSave()'s own setTimeout + // calls this closure-scoped function directly, so patching Store.commit + // (or any individual caller) from outside cannot stop it. The tour's own // welcome copy promises "nothing here touches your real files," and - // enumerating every caller in tour.js's blocklist misses ones reachable + // enumerating every caller in tour.js's blocklist missed ones reachable // outside a click (keyboard shortcuts bypass the tour's click-blocking // backdrop entirely, since CSS pointer-events does nothing to keydown - // listeners) -- so the guard belongs here, once, covering every path by - // construction instead of by an ever-growing enumeration. + // listeners) -- so the guard belongs here, once, covering every project- + // save path by construction instead of by an ever-growing enumeration. + // NOTE: this is not the one true funnel for every mutation tour.js needs + // to worry about -- shortcut/transition/category CRUD (saveShortcut, + // deleteTransition, etc.) call their own API.* functions directly and + // never reach commit() at all; those still need their own entries in + // tour.js's patchStore() blocklist. if (window.__FUNPACK_TOUR__) return; if (!state.project) return; if (_commitPromise) { _commitQueued = true; return _commitPromise; } diff --git a/app/legacy/tour.js b/app/legacy/tour.js index 6d0cf7e4..946eab9d 100644 --- a/app/legacy/tour.js +++ b/app/legacy/tour.js @@ -345,6 +345,13 @@ "saveSelectedToMediaBin", "commit", "newProject", "loadProject", "deleteProject", "importProject", "downloadProject", "uploadMedia", "deleteMedia", "deleteMediaMany", "interrupt", "resetStudioSession", "syncFromPreview", "saveClipToMediaBin", + // Not reachable via any current UI (no Settings > Shortcuts panel is + // wired up in v5 yet) -- same "block it now, before a future UI wires + // it up" reasoning as saveClipToMediaBin above. These all call real, + // unmocked API.* functions that (unlike project saves) don't funnel + // through commit(), so the store.js guard doesn't cover them. + "saveShortcut", "deleteShortcut", "addCategory", "importShortcuts", "clearShortcuts", + "saveTransition", "deleteTransition", "importTransitions", "clearTransitions", ]; const labels = { generate: "Generate", @@ -364,10 +371,13 @@ // Blocking "commit" only stops external callers of Store.commit -- the // property gets reassigned below, but scheduleSave()'s own setTimeout // calls the closure-scoped `commit` function directly, bypassing this - // patch. syncFromPreview() is the one UI entry point that reaches - // scheduleSave() (and, through it, a real API.saveProject network - // call) without going through anything else already on this list, so - // it needs its own block rather than relying on "commit" to cover it. + // patch. store.js's commit() now has its own window.__FUNPACK_TOUR__ + // guard as a backstop for every PROJECT-save path (including ones this + // list doesn't name, like the keyboard shortcuts that reach + // removeSelectedScenes()/splitScene()/etc.) -- but that guard only + // covers commit()'s own callers. syncFromPreview() and every entry + // below it call a DIFFERENT real, unmocked API.* function that never + // reaches commit() at all, so they still need their own block here. syncFromPreview: "Sync scenes from preview", // Not reachable via a real network call today -- API.importClipToMediaBin // is itself a stub ("no render/stitch stage yet") -- but the button @@ -375,6 +385,20 @@ // clipSaveableToMediaBin()), so this needs blocking now rather than // whenever that stage lands and the stub silently becomes a real PUT. saveClipToMediaBin: "Save video", + // None of these are reachable via any current UI -- no Settings > + // Shortcuts panel exists in v5 yet -- but each calls a real, unmocked + // API.* function (POST/DELETE /api/shortcuts, /api/transitions) that, + // unlike a project save, writes GLOBAL state, not per-project state. + // Blocking pre-emptively, same reasoning as saveClipToMediaBin. + saveShortcut: "Save shortcut", + deleteShortcut: "Delete shortcut", + addCategory: "Add category", + importShortcuts: "Import shortcuts", + clearShortcuts: "Clear shortcuts", + saveTransition: "Save transition", + deleteTransition: "Delete transition", + importTransitions: "Import transitions", + clearTransitions: "Clear transitions", }; blocked.forEach((name) => { if (typeof Store[name] !== "function") return;