fix(db): add manual migration from 38 to 37 to prevent data loss on downgrade

[kairosci]

Problem

While testing PRs or branches containing a database schema bump to version 38 (such as PR 3807), I noticed a fatal IllegalStateException when returning to main or downgrading to version 37. Room crashes because it requires a downgrade migration path that doesn't exist. Using destructive migration fallback on downgrade is not an option as it results in total user data loss.
Furthermore, I discovered that the dependency injection in AppModule.kt was instantiating the Room database using a bare Room.databaseBuilder, completely bypassing InternalDatabase.newInstance(context). This caused ALL manual database configurations, migrations, and callbacks to be ignored by the app.

Cause

• When I upgrade the Room database to version 38 and then return to an older build running database version 37, it triggers a downgrade path. Since no downgrade migration was provided in main, Room throws an exception: A migration from 38 to 37 was required but not found.
• AppModule.kt was ignoring MusicDatabase.newInstance() which meant that even if a migration was added there, Room would never see it.

Solution

• I fixed AppModule.kt to actually use InternalDatabase.newInstance(context) and properly extract the delegate, so all manual migrations and configurations are correctly applied.
• I added a manual downgrade migration MIGRATION_38_37 inside MusicDatabase.kt.
• My migration safely drops the ArtistPageCache table (added in 38) and gracefully reverts the artist table structure to its schema at version 37 by reconstructing the table.
• I removed the destructive migration fallback on downgrade to guarantee no user data is wiped when downgrading.

Testing

• I verified that MusicDatabase.kt compiles successfully and the SQL matches the Room schema for version 37 exactly.
• I ran ./gradlew :app:assembleFossDebug to ensure ksp verification passes successfully on my end.
• The migrations are now correctly registered and executed.

Related Issues

• Related to fix(audio): custom volume normalization to resolve muffled audio #3807

Summary by CodeRabbit

• Bug Fixes
  • Improved artist information handling and data management to enhance app stability.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6c5e75b1-69c9-4505-9109-baa87f730f4c

📥 Commits

Reviewing files that changed from the base of the PR and between ec48a01 and b3c3120.

📒 Files selected for processing (2)

• app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt
• app/src/main/kotlin/com/metrolist/music/di/AppModule.kt

🚧 Files skipped from review as they are similar to previous changes (1)

• app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt

---

📝 Walkthrough

Walkthrough

Adds a Room migration MIGRATION_38_37 (schema 38→37) that rebuilds the artist table and drops ArtistPageCache, exposes MusicDatabase.delegate as public, and refactors DI providers to create MusicDatabase via InternalDatabase.newInstance and derive InternalDatabase from MusicDatabase.delegate.

Changes

DB migration + DI provider changes

Layer / File(s)	Summary
Expose InternalDatabase delegate on MusicDatabase app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt	Changes MusicDatabase constructor property to val delegate: InternalDatabase, making the internal Room DB delegate publicly accessible.
Artist table downmigration definition and registration app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt	Adds MIGRATION_38_37 (Migration(38, 37)) which drops ArtistPageCache, creates artist_new with updated columns, copies selected columns from artist, drops artist, and renames artist_new to artist. The migration is registered in InternalDatabase.newInstance(...) via .addMigrations(...).
Refactor Hilt providers to use MusicDatabase/delegate app/src/main/kotlin/com/metrolist/music/di/AppModule.kt	provideDatabase now returns InternalDatabase.newInstance(context) as a MusicDatabase; provideInternalDatabase now accepts MusicDatabase and returns database.delegate instead of building the Room database in the module.

Sequence Diagram(s)

sequenceDiagram
  participant AppModule
  participant InternalDatabase
  participant Room
  participant MusicDatabase
  AppModule->>InternalDatabase: call InternalDatabase.newInstance(context)
  InternalDatabase->>Room: build database with .addMigrations(MIGRATION_38_37)
  InternalDatabase->>MusicDatabase: construct MusicDatabase(delegate)
  AppModule->>MusicDatabase: obtain MusicDatabase
  AppModule->>MusicDatabase: access .delegate to provide InternalDatabase

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(db): add manual migration from 38 to 37 to prevent data loss on downgrade' clearly and specifically summarizes the main change: adding a database migration to handle downgrade scenarios safely.
Description check	✅ Passed	The pull request description is comprehensive and well-structured, covering all required template sections: Problem, Cause, Solution, Testing, and Related Issues. Each section provides specific, relevant details about the database migration fix.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt`:
- Around line 821-823: The CREATE TABLE statement in MIGRATION_38_37 (in
MusicDatabase.kt) uses `DEFAULT false` for the INTEGER boolean-like columns
`isLocal` and `isPodcastChannel` on the `artist_new` table; change those
defaults to `DEFAULT 0` so the SQL is compatible with older SQLite
versions—locate the SQL string used in MIGRATION_38_37 that creates `artist_new`
and replace both `DEFAULT false` occurrences with `DEFAULT 0`.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 62ba354a-5b9b-48f8-9da9-cc2ab9acd689

📥 Commits

Reviewing files that changed from the base of the PR and between ee2e8c3 and ec48a01.

📒 Files selected for processing (1)

• app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt

[kairosci]

mark to draft because I want understand before if v38 with cached artist and songs works