Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
LibraryItemandPlaylistto skip DB writes when nothing changed. - Switches
LibraryItemkeys fromusizeto UUIDStringand 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_schemastill drops all tables wheneverschema_versionchanges, and this PR bumpsSCHEMA_VERSIONagain. 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
TEXTtoi64) can cause unexpected query failures, index misses, and potential panics if a generatedusizeexceedsi64::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>.
No description provided.