Skip to content

Refactor database architecture: dirty tracking, UUIDs, ghost files#32

Merged
RetricSu merged 4 commits into
developfrom
fix-db
Apr 14, 2026
Merged

Refactor database architecture: dirty tracking, UUIDs, ghost files#32
RetricSu merged 4 commits into
developfrom
fix-db

Conversation

@RetricSu
Copy link
Copy Markdown
Owner

No description provided.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request transitions the library identification system from path-based to hash-based uniqueness and changes track keys from integers to UUID strings. It introduces a "dirty" flag mechanism for optimized database persistence and improves picture loading performance by eliminating N+1 queries. Feedback points out that resetting dirty flags before a successful transaction commit could lead to data loss. Additionally, the use of blocking I/O in LibraryItem::new during database loading is inefficient, and the non-deterministic UUID fallback for file hashes may cause duplicate entries if metadata retrieval fails.

Comment thread src/lib/library.rs
Comment thread src/lib/library.rs Outdated
Comment thread src/lib/library.rs
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the library/playlist persistence layer to reduce unnecessary DB writes (“dirty” tracking), switch identifiers to UUID strings, and improve library load performance by avoiding N+1 picture queries—aiming to address “ghost file” behavior and scalability concerns.

Changes:

  • Introduces dirty flags for LibraryItem and Playlist to skip DB writes when nothing changed.
  • Switches LibraryItem keys from usize to UUID String and propagates API changes across services/UI.
  • Updates DB schema (adds file_hash, adds pictures index) and refactors picture loading to batch-fetch.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/lib/services/library_import.rs Sends boxed LibraryItem commands to the app layer.
src/lib/playlist.rs Adds playlist dirty tracking; updates key lookups to string keys; save now early-returns if clean.
src/lib/library.rs Adds item dirty tracking + UUID keys + file_hash; batches picture loading.
src/app/services/player_service.rs Updates removed-track return type to UUID string key.
src/app/services/persistence_service.rs Makes library mutable during save so dirty flags can be reset.
src/app/services/lyrics_service.rs Updates lyrics update path to use string keys and new library API.
src/app/services/library_service.rs Adjusts LibraryCommand::AddItem to box/unbox library items.
src/app/services/db_persistence.rs Allows Library::save_to_db to mutate/reset dirty flags.
src/app/db.rs Bumps schema version, adds file_hash column and pictures index.
src/app/core.rs Threads updated key types + mutable library into persistence/lyrics flows.
src/app/components/playlist_table/services.rs Updates clear-lyrics action to use string keys.
src/app/components/playlist_table/columns.rs Updates lyrics column API to use string keys.
src/app/components/playlist_table/actions.rs Stores pending clear-lyrics actions as string keys.
src/app/components/player_component.rs Updates playlist removal path to use string key lookup.
docs/DATABASE_ISSUES.md Updates/shortens DB-issues documentation.
Cargo.toml Adds uuid dependency.
Cargo.lock Locks new dependency graph (uuid + transitive updates).
Comments suppressed due to low confidence (1)

docs/DATABASE_ISSUES.md:42

  • This doc section removal is misleading: the code in Database::initialize_schema still drops all tables whenever schema_version changes, and this PR bumps SCHEMA_VERSION again. Either keep the warning/issue documented here, or (preferably) implement a non-destructive migration and update the doc to reflect the new behavior.
  • Impact: Relying on SQLite's implicit type conversion (from TEXT to i64) can cause unexpected query failures, index misses, and potential panics if a generated usize exceeds i64::MAX.
  • Solution: Use standardized unique identifiers (such as ULIDs or UUIDs), store them strictly as TEXT, and query them as strings.
</details>



---

💡 <a href="/RetricSu/bird-player/new/develop?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

Comment thread src/lib/playlist.rs
Comment thread Cargo.toml Outdated
Comment thread src/app/db.rs
Comment thread src/app/db.rs
Comment thread src/lib/library.rs Outdated
Comment thread src/lib/library.rs
Comment thread src/lib/library.rs
@RetricSu RetricSu merged commit 0077632 into develop Apr 14, 2026
4 checks passed
@RetricSu RetricSu deleted the fix-db branch April 14, 2026 00:43
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.

2 participants