Skip to content

feat(app): run config migrations at start-up, document backups - #307

Merged
rowkav09 merged 2 commits into
mainfrom
feat/config-migrate-on-start
Sep 23, 2026
Merged

rowkav09 merged 2 commits into
mainfrom
feat/config-migrate-on-start

Conversation

@rowkav09

Copy link
Copy Markdown
Member

Closes #120

  • loadAppConfig now runs the existing migration store (feat: add versioned config migration core #182/feat: back up config before migration #183) before reading the config. CONFIG_VERSION = 1. Future format changes only need a new entry in CONFIG_MIGRATIONS, and the store backs up to config.json.backup-<time> before replacing the file with a validated result.
  • A config from a newer app fails with a new CONFIG_TOO_NEW start-up error and a plain message. The file isn't touched.
  • A failed, malformed or unreadable migration leaves the file untouched and falls through to the usual CONFIG_INVALID / CONFIG_LOAD_FAILED message.
  • docs/troubleshooting.md: new "Config backups and recovery" section with the backup location and manual restore steps.
  • Tests: a current config is unchanged with no backup, a newer version is refused and left untouched, and version 0 is rejected with no stray files.

@mira-reviewer-rk

mira-reviewer-rk Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Mira PR Walkthrough

This PR adds config migration support to the app's startup process. It automatically runs migrations when loading the config, creates backups before applying changes, and properly handles version mismatches (rejecting newer configs). Documentation is updated to explain backup recovery procedures.

3 files reviewed


Comment @mira-reviewer-rk help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

/mira pause

@github-actions github-actions Bot added mira-paused Pause automatic Mira reviews on this pull request area:docs size:M and removed mira-paused Pause automatic Mira reviews on this pull request labels Sep 23, 2026
@codecov

codecov Bot commented Sep 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@mira-reviewer-rk mira-reviewer-rk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mira Review Summary

The PR adds config migration at startup with backup support. Found a bug where the version 0 migration throws a generic Error instead of RangeError, causing incorrect error handling. Also, the test expects the wrong error code for version 0 config. Added suggestions for JSDoc and error handling clarity.

Key Issues

Issue Location
🔴 Migration for version 0 incorrectly throws generic Error, causing misleading error messages src/app-config.js:70
🔴 Test expects CONFIG_INVALID for version 0 but migration may throw CONFIG_TOO_NEW test/app-config.test.js:194

Comment thread src/app-config.js
Comment thread test/app-config.test.js
Comment thread src/app-config.js
Comment thread src/app-config.js
@rowkav09

Copy link
Copy Markdown
Member Author

Thanks Mira. Went through each point:

  • Version 0 should be CONFIG_TOO_NEW: no. Version 0 is older than this app, not newer. CONFIG_TOO_NEW is only for files written by a newer NowPlaying (RangeError from the migrator). A version-0 file was never released, so the right result is the normal "damaged or out of date, run setup" message (CONFIG_INVALID). The test expects exactly that. Leaving both as they are.
  • Migration failure ignored in loadAppConfig: that's on purpose. The file is left untouched, and parsing then gives the user the right message. There's no logger in this module, and app-log records start-up failures by code.
  • JSDoc: added.
    Rebased on main.

@rowkav09
rowkav09 force-pushed the feat/config-migrate-on-start branch from 9b264fb to 42ec0fb Compare September 23, 2026 23:09
@rowkav09

Copy link
Copy Markdown
Member Author

Review-lane note on Mira's migration warning: the current behavior is intended, don't apply its suggestion. A version-0 file was never released, so it should fail as damaged/out-of-date (CONFIG_INVALID via the deliberate fall-through in migrateAppConfig), not as "saved by a newer NowPlaying" - Mira's own comment admits that message would be inaccurate. The test asserting CONFIG_INVALID for version 0 documents the intended contract.

@rowkav09
rowkav09 merged commit b26760f into main Sep 23, 2026
11 of 13 checks passed
@rowkav09
rowkav09 deleted the feat/config-migrate-on-start branch September 23, 2026 23:51
@github-project-automation github-project-automation Bot moved this from Backlog to Done in nowplaying Sep 23, 2026
This was referenced Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Add versioned configuration migrations with automatic backup

1 participant