fix(command_queue): deleteFile now cascade-deletes live artboards and their state machines#111
Open
mfazekas wants to merge 1 commit into
Open
Conversation
… their state machines CommandServer::m_fileDependencies[file] was created empty at loadFile and walked by deleteFile, but instantiateArtboard never appended to it - so the cascade was always a no-op. Deleting a file while artboards instantiated from it were still alive orphaned them (and their state machines) server-side, unreachable through any cleanup path. - instantiateArtboard now registers the artboard in m_fileDependencies, mirroring how instantiateStateMachine registers in m_artboardDependencies. - cleanupArtboard un-registers the artboard from m_fileDependencies so explicit create/delete cycles on a long-lived file don't grow the dependency vector unboundedly. - deleteFile detaches the dependency list before cascading (cleanupArtboard now mutates it) and erases the file only after the cascade, since dependent artboards and state machines may reference file-owned data.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes CommandServer::deleteFile dependency cascading so that deleting a file also deletes any live artboards instantiated from it (and their state machines), preventing server-side orphan instances and adding regression tests to cover the behavior.
Changes:
- Register instantiated artboards into
m_fileDependenciesand cascade-delete them fromdeleteFile. - Unregister artboards from file dependency tracking during
cleanupArtboard, and adjustdeleteFileto detach the dependency list before iterating. - Add unit tests validating cascade deletion, idempotency with explicit deletes, and safe rejection of commands on cascade-deleted handles.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit_tests/runtime/command_queue_test.cpp | Adds regression tests for cascade deletion behavior and unknown-handle safety after deleteFile. |
| src/command_server.cpp | Implements file→artboard dependency registration and safe cascade deletion ordering in deleteFile. |
| include/rive/command_server.hpp | Updates cleanupArtboard to unregister artboards from m_fileDependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+177
to
+181
| for (auto& fileDependency : m_fileDependencies) | ||
| { | ||
| auto& artboardVector = fileDependency.second; | ||
| artboardVector.erase(std::remove(artboardVector.begin(), | ||
| artboardVector.end(), |
Comment on lines
+1030
to
+1032
| assert(m_fileDependencies.find(fileHandle) != | ||
| m_fileDependencies.end()); | ||
| m_fileDependencies[fileHandle].push_back(handle); |
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
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.
CommandServer::m_fileDependencies[file]is created empty atloadFileand walked bydeleteFileto cascade-delete dependents, butinstantiateArtboardnever appends to it — so the cascade is always a no-op. Deleting a file while artboards instantiated from it are still alive orphans them (and their state machines) server-side, with no remaining documented cleanup path. Existing tests mask this by always deleting artboards explicitly before the file.This PR:
m_fileDependenciesatinstantiateArtboard, mirroring howinstantiateStateMachineregisters inm_artboardDependencies.cleanupArtboard, so explicit create/delete cycles on a long-lived file don't grow the dependency vector unboundedly.deleteFiledetach the dependency list before cascading (sincecleanupArtboardnow mutates it), and erases the file only after the cascade, since dependent artboards and state machines may reference file-owned data.Tests added to
command_queue_test.cpp(the first one fails without the fix):deleteFilewith two live artboards + state machines cleans all of them up.deleteFilestay idempotent.Full unit test suite passes.