fix(frontend): detangle saving and scene switching logic#426
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR replaces the authoring tool's patch/commit/ ChangesScene Persistence and History Refactor
Sequence Diagram(s)sequenceDiagram
participant Editor
participant SceneContextProvider
participant generatePatch
participant modifyMutation
participant ReactQueryCache
participant API
rect rgba(173, 216, 230, 0.5)
Note over Editor,API: modifyScene Optimistic Update
Editor->>SceneContextProvider: modifyMutationWrapper(scene)
SceneContextProvider->>generatePatch: generatePatch(modified, saved)
generatePatch-->>SceneContextProvider: patch
SceneContextProvider->>modifyMutation: mutate(patch)
modifyMutation->>ReactQueryCache: applyPatch (optimistic)
modifyMutation->>API: PATCH /api/scenario/:id/scene/:id
alt success
API-->>ReactQueryCache: ✓
else error
modifyMutation->>ReactQueryCache: rollback snapshot
end
end
rect rgba(144, 238, 144, 0.5)
Note over Editor,API: switchScene with Persistence
Editor->>SceneContextProvider: switchScene(currentScene, targetId)
SceneContextProvider->>SceneContextProvider: modifyMutationWrapper(flush changes)
SceneContextProvider->>Editor: clearSelection
SceneContextProvider->>API: replace route + history.push
SceneContextProvider->>localStorage: activeScene = targetId
end
rect rgba(255, 218, 185, 0.5)
Note over AuthoringToolPage,API: Undo/Redo with History Events
historyEvents->>AuthoringToolPage: "update" event (op="undo"/"redo", record)
alt sceneId differs
AuthoringToolPage->>SceneContextProvider: switchScene
end
AuthoringToolPage->>replaceComponent: replaceComponent(id, record.before/after)
AuthoringToolPage->>modifyScene: debounced modifyScene(getScene())
modifyScene->>API: PATCH /api/scenario/:id/scene/:id
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
harbassan
force-pushed
the
vps-114-scene-switching-is-noticeably-slow
branch
from
cdf59a5 to
4bc43a2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
frontend/src/features/authoring/SceneNavigator/ContextableThumb.tsx (1)
59-66: 💤 Low valueType annotation mismatch:
switchScenereturnsPromise<void>, notvoid.The type cast declares
switchScene: (scene: Scene, id: string) => void, but the actual implementation inSceneContextProviderisasync function switchScene(...)which returnsPromise<void>.While this doesn't cause runtime issues since the promise is ignored, the type should accurately reflect the async nature for correctness and to enable proper
awaitif needed in the future.♻️ Suggested type fix
const { scenes, reFetch, switchScene, deleteScene } = useContext( SceneContext ) as { scenes: Scene[]; reFetch: () => Promise<void>; - switchScene: (scene: Scene, id: string) => void; + switchScene: (scene: Scene, id: string) => Promise<void>; deleteScene: (id: string) => void; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/SceneNavigator/ContextableThumb.tsx` around lines 59 - 66, The type annotation for the switchScene function in the useContext type cast declares it as returning void, but the actual implementation is async and returns Promise<void>. Update the switchScene property in the type cast from (scene: Scene, id: string) => void to (scene: Scene, id: string) => Promise<void> to accurately reflect its async nature and enable proper await usage if needed.frontend/src/context/SceneContextProvider.jsx (2)
156-183: 💤 Low valueConsider adding
onSettledoronSuccessto ensure cache consistency.The optimistic update pattern correctly rolls back on error, but there's no
onSettledoronSuccesshandler to invalidate/refetch the query after a successful mutation. If the server modifies additional fields (e.g.,updatedAttimestamps) or if the optimistic update diverges slightly from the server response, the cache may become stale.Depending on your consistency requirements, consider adding:
onSettled: () => { queryClient.invalidateQueries(["scenes", scenarioId]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/context/SceneContextProvider.jsx` around lines 156 - 183, The modifyMutation object in SceneContextProvider has onMutate and onError handlers but lacks an onSettled or onSuccess callback to refresh the cache after a successful mutation. Add an onSettled handler to the modifyMutation configuration that invalidates the query cache for ["scenes", scenarioId] to ensure cache consistency and prevent stale data when the server response may contain additional fields or differ from the optimistic update.
46-56: 💤 Low valueData structure mismatch:
modified.componentsis an object butsaved.componentsis an array.In
generatePatch,currentComponentsis derived frommodified.componentswhich is aRecord<string, Component>(object keyed by id), whilesavedComponentscomes from the server response wheresaved.componentsis an array. The comparison at line 52 usessavedComponents.find((c) => c.id === id)which is correct for the array, but the logic assumes inconsistent data shapes.This will work but is fragile. Consider normalizing both to the same structure at the start of the function for clarity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/context/SceneContextProvider.jsx` around lines 46 - 56, The data structure inconsistency in the generatePatch function stems from currentComponents being an object (from modified.components) while savedComponents is an array (from saved.components). To fix this, normalize both structures to the same format at the beginning of the generatePatch function before performing comparisons. Choose a single consistent data structure and convert whichever one doesn't match (for example, convert the savedComponents array into an object keyed by id to match currentComponents, or vice versa) so that the comparison logic becomes clearer and more maintainable rather than mixing object and array operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/context/SceneContextProvider.jsx`:
- Around line 184-191: The modifyMutationWrapper function does not validate that
scenesQuery.data.find() returns a valid scene before passing it to
generatePatch. When the scene is not found in the cache, undefined is passed to
generatePatch, which causes a TypeError when it tries to call forEach() on
undefined at line 58. Add a defensive check after the find() call to verify the
scene exists before calling modifyMutation.mutate() with generatePatch, and
either skip the mutation or handle the missing scene scenario appropriately.
- Around line 193-199: The switchScene function attempts to await
modifyMutationWrapper but the function does not return a Promise because it
calls modifyMutation.mutate which is fire-and-forget. To fix this, locate the
modifyMutationWrapper function and replace the call to modifyMutation.mutate
with modifyMutation.mutateAsync so that it returns a Promise that can be
properly awaited. This ensures the editor state is cleared and the scene is
switched only after the save mutation completes successfully.
- Line 196: The switchScene function calls replace() with the result of find()
without verifying the scene exists, which would pass undefined to replace() if
the target scene is deleted or not found. Add a guard condition in the
switchScene function to check that scenesQuery.data.find((s) => s._id === id)
returns a valid scene object before passing it to replace(), and handle the case
where the scene is not found by either returning early, logging an error, or
displaying a user-friendly message.
In `@frontend/src/features/authoring/AuthoringToolPage.jsx`:
- Around line 48-66: The useEffect hook in the listener function has an empty
dependency array but references variables sceneId, switchScene, and setSelected
that are captured at mount time and will become stale if they change. Add
sceneId, switchScene, and setSelected to the dependency array of the useEffect.
Additionally, consider wrapping the save function with useCallback to properly
manage its dependency and avoid unnecessary re-registrations of the event
listener, as save should also be included in the dependencies but currently
references the saving state.
- Around line 113-120: The guard condition `if (saving) return` in the save()
function is using the UI state indicator flag to prevent saves, but this flag is
set to true in the listener before the debounced save() executes, causing the
function to always return early without calling modifyScene(getScene()).
Separate the concerns by creating a separate ref to track in-flight mutations
for the guard condition, while keeping the `saving` state variable only for UI
indication. The guard should check if a mutation is already in flight using the
ref, not the UI state flag, and set the ref to true before calling modifyScene,
then reset it to false after the call completes.
In `@frontend/src/features/authoring/scene/history.ts`:
- Around line 39-45: The undo/redo snapshots are capturing state from the wrong
scene context because getComponent(id) calls on lines 54 and 68 read from the
currently active scene, but the actual scene switch happens later in
AuthoringToolPage after event dispatch. To fix this, ensure that when capturing
the state for undo/redo snapshots in the dispatchTypedEvent call, you retrieve
the state from the correct scene context (sceneId) rather than relying on the
currently active scene. Either pass the sceneId explicitly to getComponent or
refactor to capture the state using the correct scene context before the scene
switching occurs.
In `@frontend/src/util/debounce.ts`:
- Around line 7-10: In the debounce function, the setTimeout callback is calling
fn(args) which passes the entire arguments tuple as a single argument, breaking
the original function signature. Change the fn(args) call inside the setTimeout
to fn(...args) by adding the spread operator to properly forward all arguments
to the original function with their individual parameter positions preserved.
---
Nitpick comments:
In `@frontend/src/context/SceneContextProvider.jsx`:
- Around line 156-183: The modifyMutation object in SceneContextProvider has
onMutate and onError handlers but lacks an onSettled or onSuccess callback to
refresh the cache after a successful mutation. Add an onSettled handler to the
modifyMutation configuration that invalidates the query cache for ["scenes",
scenarioId] to ensure cache consistency and prevent stale data when the server
response may contain additional fields or differ from the optimistic update.
- Around line 46-56: The data structure inconsistency in the generatePatch
function stems from currentComponents being an object (from modified.components)
while savedComponents is an array (from saved.components). To fix this,
normalize both structures to the same format at the beginning of the
generatePatch function before performing comparisons. Choose a single consistent
data structure and convert whichever one doesn't match (for example, convert the
savedComponents array into an object keyed by id to match currentComponents, or
vice versa) so that the comparison logic becomes clearer and more maintainable
rather than mixing object and array operations.
In `@frontend/src/features/authoring/SceneNavigator/ContextableThumb.tsx`:
- Around line 59-66: The type annotation for the switchScene function in the
useContext type cast declares it as returning void, but the actual
implementation is async and returns Promise<void>. Update the switchScene
property in the type cast from (scene: Scene, id: string) => void to (scene:
Scene, id: string) => Promise<void> to accurately reflect its async nature and
enable proper await usage if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d961e75-bdc0-4187-bb8d-6916949435aa
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
frontend/package.jsonfrontend/src/context/SceneContextProvider.jsxfrontend/src/features/authoring/AuthoringToolPage.jsxfrontend/src/features/authoring/CanvasSideBar/SceneSettings.jsxfrontend/src/features/authoring/SceneNavigator/ContextableThumb.tsxfrontend/src/features/authoring/scene/history.tsfrontend/src/features/authoring/scene/operations/modifiers.tsfrontend/src/features/authoring/scene/scene.tsfrontend/src/util/debounce.ts
harbassan
force-pushed
the
vps-114-scene-switching-is-noticeably-slow
branch
from
34fa835 to
70edf8e
Compare
rgin216
reviewed
Jun 20, 2026
harbassan
force-pushed
the
vps-114-scene-switching-is-noticeably-slow
branch
from
ab82ea8 to
8db6bb9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/StateVariables/EditStateVariable.jsx (1)
100-110:⚠️ Potential issue | 🟠 MajorSerialize
modifyScenecalls to prevent rollback conflicts with optimistic mutations.The current
Promise.all(updatePromises)does not actually await mutations becausemodifyScene(implemented asmodifyMutationWrapper) returnsundefinedrather than a promise. Mutations fire concurrently, and when overlappingonMutate/onErrorcallbacks execute, rollback can restore stale state snapshots, dropping successful updates. Each mutation independently capturespreviousScenesduringonMutate; if one fails and restores to its snapshot, it overwrites optimistic updates from other concurrent mutations.Suggested fix
- const updatePromises = updatedScenes.map((scene) => - modifyScene({ - _id: scene._id, - fields: {}, - components: scene.components, - deletedComponentIds: [], - }) - ); - - await Promise.all(updatePromises); - await reFetch(); + for (const scene of updatedScenes) { + await modifyScene({ + _id: scene._id, + fields: {}, + components: scene.components, + deletedComponentIds: [], + }); + } + await reFetch();To fully fix this,
modifyMutationWrappershould usemutateAsync()instead ofmutate()and return the promise, or the mutations must be serialized. The serial approach is safer for this use case sincemodifyMutationrollback behavior is not designed for concurrent writes to the same query cache.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/StateVariables/EditStateVariable.jsx` around lines 100 - 110, The concurrent execution of `modifyScene` calls via `Promise.all(updatePromises)` causes rollback conflicts where stale state snapshots from one mutation can overwrite successful updates from other concurrent mutations. Replace the map and `Promise.all()` pattern with a serialized approach that awaits each `modifyScene` call sequentially, ensuring each mutation completes before the next one begins. This prevents overlapping `onMutate`/`onError` callbacks from restoring conflicting state snapshots.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/features/authoring/AuthoringToolPage.jsx`:
- Around line 49-52: The debounced callback in the modifyScene call chain is
setting setSaving(false) immediately without waiting for the async operation to
complete, which causes the saving state to be cleared before persistence
actually finishes. Make the debounced callback function async and await the
modifyScene(getScene()) call before calling setSaving(false). Apply this same
fix to the other location mentioned (lines 117-120) where modifyScene is called,
ensuring that setSaving(false) only executes after the async persistence
operation has completed in both places.
- Around line 57-60: The switchScene function call on line 57 is not being
awaited, which causes replaceComponent on line 60 to execute against the old
scene before the scene switch completes. Modify the code to await the
switchScene call before proceeding with the rest of the undo/redo logic. This
ensures that when replaceComponent is called with the record id and state, it
operates on the correct active scene for cross-scene undo/redo operations.
In `@frontend/src/features/authoring/scene/history.ts`:
- Around line 40-45: The record object stores prevState by reference while
current is properly cloned with structuredClone, causing history snapshots to be
mutated by the consumer when applying undo/redo operations. In the record object
creation, clone prevState using structuredClone just like current is cloned for
the after field. Additionally, at the dispatch locations (lines 51, 60, and 69
where the record is dispatched), ensure that either the record object itself is
cloned before dispatching or a new record object is created for each dispatch to
prevent the same mutable reference from being reused across multiple operations.
---
Outside diff comments:
In `@frontend/src/components/StateVariables/EditStateVariable.jsx`:
- Around line 100-110: The concurrent execution of `modifyScene` calls via
`Promise.all(updatePromises)` causes rollback conflicts where stale state
snapshots from one mutation can overwrite successful updates from other
concurrent mutations. Replace the map and `Promise.all()` pattern with a
serialized approach that awaits each `modifyScene` call sequentially, ensuring
each mutation completes before the next one begins. This prevents overlapping
`onMutate`/`onError` callbacks from restoring conflicting state snapshots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01dc6714-825a-44d0-9030-382e093df59d
📒 Files selected for processing (6)
frontend/src/components/StateVariables/EditStateVariable.jsxfrontend/src/context/SceneContextProvider.jsxfrontend/src/features/authoring/AuthoringToolPage.jsxfrontend/src/features/authoring/CanvasSideBar/SceneSettings.jsxfrontend/src/features/authoring/scene/history.tsfrontend/src/util/debounce.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/util/debounce.ts
- frontend/src/features/authoring/CanvasSideBar/SceneSettings.jsx
- frontend/src/context/SceneContextProvider.jsx
rgin216
previously requested changes
Jun 22, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/features/authoring/scene/history.ts (1)
94-103: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd defensive validation for scene existence before dispatching redo events.
Same concern as
undo(): no validation thatrecord.sceneIdexists before dispatching. If the scene was deleted, the consumer'sswitchScenemay fail.🛡️ Proposed fix
export function redo() { const record = redoStack.pop(); if (!record) return; + + // Add same validation as undo() undoStack.push(record); historyEvents.dispatchTypedEvent( "update", new HistoryEvent("redo", cloneHistoryRecord(record)) ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/scene/history.ts` around lines 94 - 103, The redo() function does not validate that the scene from the record exists before dispatching the redo event to consumers. Add defensive validation after popping the record from redoStack to verify that record.sceneId points to a valid existing scene, similar to the validation pattern used in the undo() function. Only dispatch the historyEvents update event if the scene validation passes, to prevent downstream errors in switchScene calls from attempting to switch to a deleted scene.
🧹 Nitpick comments (1)
frontend/src/features/authoring/AuthoringToolPage.jsx (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary
asynckeyword.The listener function is declared
asyncbut never awaits anything. Per the past comment thread,switchSceneis synchronous. Declaring the functionasyncis misleading.♻️ Proposed fix
- const listener = async ({ operation, record }) => { + const listener = ({ operation, record }) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/AuthoringToolPage.jsx` at line 58, The listener function is declared with the async keyword but does not contain any await statements, making the async declaration misleading since the function operates synchronously. Remove the async keyword from the listener function declaration to accurately reflect its synchronous nature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/features/authoring/AuthoringToolPage.jsx`:
- Around line 49-73: The saving flag race condition occurs because the finally
block in the debounced function clears the saving state when one save completes,
even if another debounced save is still pending. To fix this, create a ref to
track the count of in-flight operations (e.g., pendingOpsRef initialized to 0).
In the debounced callback within the listener function, increment pendingOpsRef
before calling modifyScene. In the finally block of the debounced callback,
decrement pendingOpsRef and only call setSaving(false) when pendingOpsRef
reaches 0. This ensures the saving indicator remains true until all scheduled
saves have completed, preventing the UI from showing a false "saved" state while
changes are still pending.
In `@frontend/src/features/authoring/scene/history.ts`:
- Around line 42-50: The module-level undo/redo stacks (undoStack and redoStack)
are not cleared when switching between scenarios, causing stale history records
from one scenario to persist and potentially corrupt data in another scenario.
The clearHistory() function only runs once on initial component mount due to an
empty dependency array in useEffect, so it doesn't execute when the scenarioId
changes. Fix this by either adding scenarioId to the HistoryObject type and
calling clearHistory() whenever the scenario changes, or by including the
current scenarioId in the useEffect dependency array so that clearHistory() runs
whenever the active scenario changes. This ensures that each scenario has its
own isolated undo/redo stack without cross-scenario contamination.
---
Duplicate comments:
In `@frontend/src/features/authoring/scene/history.ts`:
- Around line 94-103: The redo() function does not validate that the scene from
the record exists before dispatching the redo event to consumers. Add defensive
validation after popping the record from redoStack to verify that record.sceneId
points to a valid existing scene, similar to the validation pattern used in the
undo() function. Only dispatch the historyEvents update event if the scene
validation passes, to prevent downstream errors in switchScene calls from
attempting to switch to a deleted scene.
---
Nitpick comments:
In `@frontend/src/features/authoring/AuthoringToolPage.jsx`:
- Line 58: The listener function is declared with the async keyword but does not
contain any await statements, making the async declaration misleading since the
function operates synchronously. Remove the async keyword from the listener
function declaration to accurately reflect its synchronous nature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 284110dd-8571-4948-95a1-2fa75719335d
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
frontend/package.jsonfrontend/src/components/StateVariables/EditStateVariable.jsxfrontend/src/context/SceneContextProvider.jsxfrontend/src/features/authoring/AuthoringToolPage.jsxfrontend/src/features/authoring/CanvasSideBar/SceneSettings.jsxfrontend/src/features/authoring/SceneNavigator/ContextableThumb.tsxfrontend/src/features/authoring/scene/history.tsfrontend/src/features/authoring/scene/operations/modifiers.tsfrontend/src/features/authoring/scene/scene.tsfrontend/src/util/debounce.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/src/features/authoring/CanvasSideBar/SceneSettings.jsx
- frontend/package.json
- frontend/src/util/debounce.ts
- frontend/src/components/StateVariables/EditStateVariable.jsx
- frontend/src/features/authoring/SceneNavigator/ContextableThumb.tsx
- frontend/src/features/authoring/scene/operations/modifiers.ts
- frontend/src/context/SceneContextProvider.jsx
- frontend/src/features/authoring/scene/scene.ts
leowla
reviewed
Jun 23, 2026
Contributor
Author
Yep, realistically a user would never have to manual save but ive left it there for now just in case we might ever need it.
Aware of this, not something i addressed in this PR as its a seperate concern. |
This was referenced Jun 29, 2026
GogaIV
pushed a commit
that referenced
this pull request
Jul 7, 2026
* fix(frontend): detangle saving and scene switching logic * fix(authoring): history behaviour * fix: improper dependency array * fix: concurrency and async support for modifyScene mutation * fix(history): make scene prop modifications trigger events, clear history on editor load * chore: change autosave debounce timer to 2500 ms * fix: immediate modification save status collision
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
The scene saving dispatch logic, especially the scene switching, is tangled throughout the history, scene, operation and scene context parts of the code base. The result is scene switching and saving behavior that feels sluggish to use.
Solution
Scene switching logic has been moved to a central place: SceneContext, and it is only called from one place: AuthoringTool. Instead of class-like initialisation of the history module that was required to allow the history functions to perform actions, the history module is now just a store of the history undo/redo stack that dispatches events when those functions are called. The actual actioning of the undo and redo is performed in the same place as the logic it requires, making use of the SceneContext.
Important!
MANUAL SAVE is mostly removed in this PR, in favour of a modification-aware autosave. With the addition of the history events, the autosave now just listens for any of these events to be dispatched as a sign of "modification". To prevent against server overload, there is a 2000ms debounce.
I didn't initially intend to change this behaviour, but it became a natural extension using the history events system.
There is a NOTE in the code about the combining of the save functionality and the "history actioning" functionality in the AuthoringTool component. This is not ideal, but i've done it this way due to the limitation of SceneContext only being accessible from within the React tree. If we move the scene context into a zustand store, this should be separated for better structuring of the codebase.
Risk
Both saving and history are modified in this PR, so its actually quite risky. Need to test well.
Checklist
Summary by CodeRabbit