fix: config reset on app restart and improve migrations handling #394
Conversation
This file contains 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
Test this pull request on windows-latestDownload the correct version for your architecture: |
Test this pull request on macos-latestDownload the correct version for your architecture:Click here if you don't know which version to downloadFor running this unsigned version of the app, you will need to run the xattr command on it:
|
cazala
changed the title
cazala
commented
Jan 28, 2025
|
|
||
| // Initialize with default values if empty | ||
| const existingConfig = await storage.get<Record<string, any>>(''); |
There was a problem hiding this comment.
This line was the bug, this always returns undefined, and causes the default config to be applied over the existing one.
cazala
commented
Jan 28, 2025
| const [projectPath] = await invoke('electron.showOpenDialog', { | ||
| title: 'Import project', | ||
| properties: ['openDirectory'], | ||
| defaultPath: config.settings.scenesPath, |
There was a problem hiding this comment.
This is a quality-of-life improvement for users
cazala
commented
Jan 28, 2025
| for (const [key, value] of Object.entries(mergedConfig)) { | ||
| await storage.set(key, value); | ||
| export async function getConfig(): Promise<IFileSystemStorage> { | ||
| await waitForMigrations(); |
There was a problem hiding this comment.
This is just to avoid possible race conditions between the migration of the config and the initialization of the default values.
cazala
force-pushed
the
feat/set-default-path-import-project
branch
2 times, most recently
from
266c68e to
7b01fac
Compare
cazala
force-pushed
the
feat/set-default-path-import-project
branch
from
7b01fac to
cb87e5d
Compare
cyaiox
approved these changes
Jan 28, 2025
|
I just finished testing on Windows and Mac 🤦🏽. Anyways, here's the evidence and the tests performed:
Scene Preview:ch.persist.mp4Scenes persist after restarting:CH.V0.17.0.FIX.mp4Publish a scene:scene.published.mp4 |
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.
Fix config reset on app restart and improve migrations handling
Config Reset Bug
The config was being reset to default values on app restart because we were incorrectly using
storage.get('')to check for config existence. This was unreliable and caused the config to reset even when valid data existed.Fix
getAll()andsetAll()methods to read/write entire config state at onceMigrations Improvement
Added synchronization to prevent race conditions between config operations and migrations:
waitForMigrations()utility usingfp-futuregetConfig()to await migrations before proceedingV2 Migration
Added version-based migration that automatically includes scene directories in workspace:
scenesPathfor valid scenes (containing scene.json)Testing