Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Application State Improvements #3166

Merged
merged 14 commits into from
Nov 14, 2024
Merged

Application State Improvements #3166

merged 14 commits into from
Nov 14, 2024

Conversation

jameskerr
Copy link
Member

@jameskerr jameskerr commented Nov 13, 2024

Fixes #3164

The way that our application system handled migration errors until now was to wipe out the state and start fresh. This is problematic for obvious reasons. This PR makes a few changes to allow the application to recover from errors in the future.

  1. When migrations are needed, we will now backup the current appState.json file into an adjacent folder called "backups". This will not happen on every startup, just when migrations are needed.

  2. If an exception is thrown in a migration, we halt the application startup process and display an error dialog box. The existing appState.json will not be touched. Once the migration error is addressed, we can try again with the same appState.json file.

  3. If you backup the same appState.json file multiple times (as would happen if you keep re-opening the app when it contained a bad migration), it will keep making backups appending a "_{n}.json" suffix. I opted for this safer-route instead of overwriting a previous backup. Curious to hear your thoughts on this Phil.

CleanShot 2024-11-13 at 15 33 35@2x

There have been tests written in this PR for cases like:

  • The app state file is not present (we initialize with fresh state)
  • The app state file is present but empty (we initialize with fresh state)
  • The app state file contains non-parsable json (we throw an error and halt startup)
  • The app state file contains parsable json but does not contain a top-level version key with a number value (we throw an error and halt startup)

If any of these cases occur, we display an error dialog, then quit the app.

CleanShot 2024-11-13 at 15 24 30@2x

Also:

I am removing some logic that was written years ago to allow very very old versions of the then Brim app to be migrated in one go to the most current version. Instead, it will now display the error above because those appState.json files don't have a version key. Users could then manually update the appState.json to look like {version: 0, data: ...} if they happened to be on a version from 3-4 years ago and need to upgrade.

@jameskerr jameskerr changed the title Improve App State" Application State Improvements Nov 13, 2024
@philrz
Copy link
Contributor

philrz commented Nov 14, 2024

A couple responses:

If you backup the same appState.json file multiple times (as would happen if you keep re-opening the app when it contained a bad migration), it will keep making backups appending a "_{n}.json" suffix. I opted for this safer-route instead of overwriting a previous backup. Curious to hear your thoughts on this Phil.

I just experienced this as a user while testing it out. I think what you've got here is great. If we're lucky enough to be bug free with all future migrations (🤞) the excess _n backups would never get generated. If we do have bugs and users have problems, I'm absolutely in favor of being conservative and risk leaving some extra clutter behind if there's any chance at all that it saves the user from losing data they'd have wanted saved. The appState.json files are not that large in the grand scheme of things and if anyone ever notices them accumulating and makes a fuss I'm happy to revisit the topic, but I doubt it'll come up.

I am removing some logic that was written years ago to allow very very old versions of the then Brim app to be migrated in one go to the most current version. Instead, it will now display the error above because those appState.json files don't have a version key. Users could then manually update the appState.json to look like {version: 0, data: ...} if they happened to be on a version from 3-4 years ago and need to upgrade.

That seems fine to me too. If a user happens to revisit their app for the first time in 3-4 years and gets into this spot I'd be pleased to meet them on community Slack and help them recover. 😄

@philrz philrz merged commit edbe753 into main Nov 14, 2024
4 checks passed
@philrz philrz deleted the save-backups branch November 14, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query with syntax error in history causes failed migration to snapshots
2 participants