various: sync local favorites and playlists through library folders - #606
various: sync local favorites and playlists through library folders#606temidaradev wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThis change adds portable metadata databases for local libraries. It adds configuration, SQLite initialization, path encoding, source routing, change detection, settings controls, localized labels, styling utilities, and integration coverage for moving libraries between mount paths. ChangesPortable metadata support
Sequence Diagram(s)sequenceDiagram
participant Settings
participant SourceResolver
participant LocalSource
participant PortableDatabase
participant MetadataWatcher
participant AppDatabase
Settings->>SourceResolver: enable portable metadata
SourceResolver->>LocalSource: resolve source with configured directories
LocalSource->>PortableDatabase: initialize or migrate database
LocalSource->>PortableDatabase: read and write encoded metadata
MetadataWatcher->>PortableDatabase: poll file signature
PortableDatabase-->>MetadataWatcher: changed metadata and activity
MetadataWatcher->>AppDatabase: merge listen counts
MetadataWatcher-->>LocalSource: invalidate metadata generations
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
crates/db/src/backend/mod.rs (1)
47-52: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider snapshotting the portable database before migrating it.
opencallsmigrations::snapshot_if_pending(path)beforerun_migrations.open_portableomits that step. The portable file is the user's only copy of the shared favorites and playlists, and it can be written by a different Kopuz version on another computer. A snapshot gives the same recovery path the app database already has.The fallback in
crates/server/src/source/local.rshandles an open failure by using the app database, so this is resilience hardening, not a current data loss.♻️ Proposed change
pub async fn open_portable(path: &Path) -> Result<Self, DbError> { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent).map_err(|e| DbError::Io(e.to_string()))?; } + migrations::snapshot_if_pending(path).await; let pool = open_portable_pool(path).await?; migrations::run_migrations(&pool).await?;🤖 Prompt for 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. In `@crates/db/src/backend/mod.rs` around lines 47 - 52, Update open_portable to call migrations::snapshot_if_pending(path) after ensuring the parent directory exists and before migrations::run_migrations(&pool), matching the existing open flow while preserving the current pool-opening and migration error propagation.crates/hooks/src/portable_metadata.rs (1)
35-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why a path change does not bump the generations.
The
previous.0 == current.0condition is the subtle part of this hook. It suppresses a bump when the portable path itself changes, because a source rotation already re-runs the queries. A future reader can easily read the condition as a missing case and "fix" it into an extra bump.Add one line that states the reason.
♻️ Proposed comment
let current = (path, signature); + // Only a same-path signature change means another computer + // committed. A path change is a source rotation, and the identity + // memo already rebuilds the queries for that. if let Some(previous) = observed.as_ref() && previous.0 == current.0 && previous.1 != current.1As per coding guidelines: "Keep comments focused on the non-obvious reason why something exists; do not restate the code."
🤖 Prompt for 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. In `@crates/hooks/src/portable_metadata.rs` around lines 35 - 42, Add a concise comment immediately above the `previous.0 == current.0` condition explaining that portable path changes are intentionally excluded because source rotation already reruns the queries; keep the surrounding generation-bump logic unchanged.Source: Coding guidelines
🤖 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 `@crates/components/src/settings_items.rs`:
- Around line 385-387: Update the settings removal flow around
default_metadata_path and the portable_metadata_path/metadata_path handling so
the first portable directory cannot be removed unless its existing database is
migrated safely before the directory change. Preserve the current portable
database and favorites/playlists when local or named local library directories
are modified, using the existing portable_path and LocalSource::new behavior as
the integration points.
In `@crates/i18n/locales/ru.ftl`:
- Line 30: Update the shared_library_data translation to use grammatically
correct Russian agreement while preserving the meaning of shared favorites and
playlists.
In `@crates/kopuz/assets/tailwind.css`:
- Around line 907-910: Update the affected Tailwind CSS rules, including
.-translate-x-1 and the four additional Stylelint-reported rules, by inserting
the required empty line before the flagged declaration. If this stylesheet is
generated, make the formatting change in its generator and regenerate the file
so all five violations are corrected consistently.
In `@crates/server/src/source/local.rs`:
- Around line 137-142: Make the portable metadata toggle’s contract explicit at
the settings UI, documenting that switching between portable and app metadata
stores is one-way and does not migrate existing favorites, playlists, or
folders. Update the setting associated with
local_portable_metadata/portable_metadata rather than changing metadata_store or
seed_portable.
- Around line 144-165: The portable reference format currently encodes the
positional root index in encode_ref, which is unstable across machines and
reordered or removed directories. Replace root_index-based encoding and
decode_ref lookup with a stable, persisted root identifier associated with each
library root and resolve references by that key; when no key matches, discard
the reference rather than returning the encoded kopuz-root-v1 string.
- Around line 73-135: Update seed_portable to execute the existing
portable-state checks, favorites/playlists/folders seeding, and
PORTABLE_INIT_KEY marker write within one database transaction. Add the
necessary transactional wrapper through the Db/Storage seam, preserving the
current early return when the marker already exists and ensuring any failure
rolls back all seed writes so retries can complete.
---
Nitpick comments:
In `@crates/db/src/backend/mod.rs`:
- Around line 47-52: Update open_portable to call
migrations::snapshot_if_pending(path) after ensuring the parent directory exists
and before migrations::run_migrations(&pool), matching the existing open flow
while preserving the current pool-opening and migration error propagation.
In `@crates/hooks/src/portable_metadata.rs`:
- Around line 35-42: Add a concise comment immediately above the `previous.0 ==
current.0` condition explaining that portable path changes are intentionally
excluded because source rotation already reruns the queries; keep the
surrounding generation-bump logic unchanged.
🪄 Autofix
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 Plus
Run ID: ec9e802d-178d-4c51-8c8e-e313556bac3a
📒 Files selected for processing (43)
crates/components/src/settings_items.rscrates/config/src/lib.rscrates/config/src/source.rscrates/db/src/backend/mod.rscrates/db/src/lib.rscrates/db/tests/config.rscrates/hooks/src/lib.rscrates/hooks/src/portable_metadata.rscrates/hooks/src/use_db_queries.rscrates/i18n/locales/ar.ftlcrates/i18n/locales/de.ftlcrates/i18n/locales/en.ftlcrates/i18n/locales/es.ftlcrates/i18n/locales/fil.ftlcrates/i18n/locales/fr.ftlcrates/i18n/locales/gr.ftlcrates/i18n/locales/he.ftlcrates/i18n/locales/hu.ftlcrates/i18n/locales/id.ftlcrates/i18n/locales/it.ftlcrates/i18n/locales/ja.ftlcrates/i18n/locales/ko.ftlcrates/i18n/locales/ml.ftlcrates/i18n/locales/nl.ftlcrates/i18n/locales/pl.ftlcrates/i18n/locales/pt-BR.ftlcrates/i18n/locales/pt-PT.ftlcrates/i18n/locales/ro.ftlcrates/i18n/locales/ru.ftlcrates/i18n/locales/sv.ftlcrates/i18n/locales/ta.ftlcrates/i18n/locales/tok-SP.ftlcrates/i18n/locales/tok.ftlcrates/i18n/locales/tr.ftlcrates/i18n/locales/uk.ftlcrates/i18n/locales/vi-VN.ftlcrates/i18n/locales/zh-CN.ftlcrates/kopuz/assets/tailwind.csscrates/kopuz/src/main.rscrates/pages/src/settings/mod.rscrates/server/src/source.rscrates/server/src/source/local.rscrates/server/tests/source.rs
| .-translate-x-1 { | ||
| --tw-translate-x: calc(var(--spacing) * -1); | ||
| translate: var(--tw-translate-x) var(--tw-translate-y); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Stylelint empty-line errors.
Stylelint reports declaration-empty-line-before at Line 909, Line 921, Line 1099, Line 2069, and Line 2367. Add an empty line before the following declaration in each affected rule. If this file is generated, update the generator and regenerate it.
Proposed formatting fix
.-translate-x-1 {
--tw-translate-x: calc(var(--spacing) * -1);
+
translate: var(--tw-translate-x) var(--tw-translate-y);
}Apply the same blank-line change to the other four flagged rules.
Also applies to: 919-922, 1096-1102, 2067-2069, 2365-2368
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 909-909: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for 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.
In `@crates/kopuz/assets/tailwind.css` around lines 907 - 910, Update the affected
Tailwind CSS rules, including .-translate-x-1 and the four additional
Stylelint-reported rules, by inserting the required empty line before the
flagged declaration. If this stylesheet is generated, make the formatting change
in its generator and regenerate the file so all five violations are corrected
consistently.
Source: Linters/SAST tools
| async fn seed_portable(&self, portable: &Db) -> Result<(), SourceError> { | ||
| if portable | ||
| .meta_get(PORTABLE_INIT_KEY, PORTABLE_INIT_KIND) | ||
| .await? | ||
| .is_some() | ||
| { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let existing_favorites = portable.favorites(Source::Local.as_str()).await?; | ||
| let existing_playlists = portable.load_playlists(&Source::Local).await?; | ||
| if existing_favorites.is_empty() | ||
| && existing_playlists.playlists.is_empty() | ||
| && existing_playlists.folders.is_empty() | ||
| { | ||
| let favorites = self.db.favorites(self.source.as_str()).await?; | ||
| let favorites: Vec<String> = favorites | ||
| .iter() | ||
| .map(|reference| self.encode_ref(reference)) | ||
| .collect(); | ||
| portable | ||
| .replace_favorites_clean(Source::Local.as_str(), &favorites) | ||
| .await?; | ||
|
|
||
| let store = self.db.load_playlists(&self.source).await?; | ||
| for playlist in &store.playlists { | ||
| let cover = playlist | ||
| .cover_path | ||
| .as_ref() | ||
| .map(|path| self.encode_ref(&path.to_string_lossy())); | ||
| portable | ||
| .upsert_playlist_meta( | ||
| &Source::Local, | ||
| &playlist.id, | ||
| &playlist.name, | ||
| cover.as_deref(), | ||
| playlist.image_tag.as_deref(), | ||
| ) | ||
| .await?; | ||
| let tracks: Vec<String> = playlist | ||
| .tracks | ||
| .iter() | ||
| .map(|reference| self.encode_ref(reference)) | ||
| .collect(); | ||
| portable | ||
| .set_playlist_tracks(&Source::Local, &playlist.id, &tracks) | ||
| .await?; | ||
| } | ||
| for folder in &store.folders { | ||
| portable.create_folder(&folder.id, &folder.name).await?; | ||
| for playlist_id in &folder.playlist_ids { | ||
| portable | ||
| .set_playlist_folder(playlist_id, Some(&folder.id)) | ||
| .await?; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| portable | ||
| .meta_put(PORTABLE_INIT_KEY, PORTABLE_INIT_KIND, "1") | ||
| .await | ||
| .map_err(SourceError::from) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for transaction support on the db crate's public seam.
set -euo pipefail
fd -t f -e rs . crates/db/src | head -50
# Trait surface of the storage seam
ast-grep outline crates/db/src/lib.rs --items all
# Any existing transaction / begin helpers
rg -nP --type=rs -C4 '\b(begin|transaction|Transaction|with_tx|in_transaction)\b' crates/db/srcRepository: Kopuz-org/kopuz
Length of output: 3311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the seed implementation and nearby portable/db init helpers.
sed -n '1,180p' crates/server/src/source/local.rs
printf '\n--- db init/getters ---\n'
sed -n '520,580p' crates/db/src/lib.rs
printf '\n--- portable metadata methods usages ---\n'
rg -n 'meta_put|meta_get|PORTABLE_INIT_KEY|seed_portable|load_playlists|favorites' crates/server/src/source/local.rs crates -g '*.rs'
printf '\n--- transaction-related text outside filetype filter ---\n'
rg -n -C3 '\b(begin|transaction|Transaction|with_tx|in_transaction|conn|pool)\b' crates/db/src crates/server/src/source crates/server/src -g '*.rs'Repository: Kopuz-org/kopuz
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Storage trait and Seed/transaction helpers'
sed -n '420,530p' crates/db/src/lib.rs
printf '%s\n' '--- portable backend outline and implementation points'
sed -n '50,320p' crates/db/src/backend/mod.rs
sed -n '780,890p' crates/db/src/backend/writes.rs
printf '%s\n' '--- exact transaction-related terms in backend/rs'
rg -n -C2 'BEGIN\s+DEFERRED|BEGIN\s+IMMEDIATE|BEGIN\s+EXCLUSIVE|BEGIN\sWORK|transaction\(|conn\.begin|Begin|rollback|ROLLBACK|with_txn|run_and_commit|run' crates/db/src crates/server/src -g '*.rs' | head -240
printf '%s\n' '--- playlist/favorite upsert SQL snippets'
rg -n -C3 'INSERT OR IGNORE INTO playlists|REPLACE INTO favorites|DELETE FROM favorites|INSERT INTO favorites|INSERT INTO PLAYLISTS|UPDATES playlists' crates/db/src/backend/writes.rs crates/db/src/backend/queries.rs | head -240Repository: Kopuz-org/kopuz
Length of output: 36502
Wrap portable seeding and the init marker in one transaction.
seed_portable writes favorites, playlist metadata, playlist tracks, and folders, then writes PORTABLE_INIT_KEY at the end. If seeding fails after some writes, the next launch sees a non-empty portable DB and skips seeding again, leaving an incomplete portable import. The public Db/Storage seam does not expose a transaction handle for this code path, so add a transactional wrapper and use it for the entire seed plus marker write.
🤖 Prompt for 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.
In `@crates/server/src/source/local.rs` around lines 73 - 135, Update
seed_portable to execute the existing portable-state checks,
favorites/playlists/folders seeding, and PORTABLE_INIT_KEY marker write within
one database transaction. Add the necessary transactional wrapper through the
Db/Storage seam, preserving the current early return when the marker already
exists and ensuring any failure rolls back all seed writes so retries can
complete.
| async fn metadata_store(&self) -> (&Db, Source) { | ||
| match self.portable_db().await { | ||
| Some(portable) => (portable, Source::Local), | ||
| None => (&self.db, self.source.clone()), | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Toggling portable metadata swaps to a different metadata set with no migration.
metadata_store selects the portable database when directories is non-empty, and the app database otherwise. crates/server/src/source.rs:857-875 makes that choice depend directly on the local_portable_metadata / portable_metadata setting, and the settings UI exposes that setting as a per-library toggle.
The two stores are independent. The consequences for the user:
- Turning the toggle off hides every favorite, playlist, and folder created while it was on. The app database still holds only the pre-toggle set.
- Turning it back on does not re-import anything.
seed_portablereturns early at lines 74-80 because the init marker already exists. - Anything created while the toggle was off stays in the app database and never reaches the shared library.
Decide the intended contract and make it explicit. Either migrate the metadata when the toggle changes, or state the one-way nature in the settings UI so the user does not lose sight of a set of playlists.
🤖 Prompt for 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.
In `@crates/server/src/source/local.rs` around lines 137 - 142, Make the portable
metadata toggle’s contract explicit at the settings UI, documenting that
switching between portable and app metadata stores is one-way and does not
migrate existing favorites, playlists, or folders. Update the setting associated
with local_portable_metadata/portable_metadata rather than changing
metadata_store or seed_portable.
| fn encode_ref(&self, reference: &str) -> String { | ||
| let path = Path::new(reference); | ||
| for (root_index, root) in self.directories.iter().enumerate() { | ||
| let Ok(relative) = path.strip_prefix(root) else { | ||
| continue; | ||
| }; | ||
| let mut segments = Vec::new(); | ||
| for component in relative.components() { | ||
| match component { | ||
| Component::Normal(segment) => { | ||
| segments.push(segment.to_string_lossy().into_owned()); | ||
| } | ||
| Component::CurDir => {} | ||
| Component::ParentDir | Component::RootDir | Component::Prefix(_) => { | ||
| return reference.to_owned(); | ||
| } | ||
| } | ||
| } | ||
| return format!("{PORTABLE_REF_PREFIX}{root_index}:{}", segments.join("/")); | ||
| } | ||
| reference.to_owned() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The positional root index makes shared references break when folder order differs.
encode_ref stores the root as directories position (root_index). decode_ref resolves it with self.directories.get(index). The position is not stable across the computers that share the library:
- Computer A configures
["/music", "/archive"]. Computer B configures the same two folders in the opposite order. Everykopuz-root-v1:0:…reference now resolves against the wrong root on B. - If the user removes a folder, the index shifts or goes out of range.
decode_refthen returns the rawkopuz-root-v1:0:album/x.flacstring, and that string reaches the UI as the track path.
The failure is silent. The user sees favorites and playlist entries that point at the wrong file or display an encoded pseudo-path.
Encode a stable root identifier instead of a position. Store a root key in the portable database (for example a generated root id, persisted per library alongside the mount path) and resolve references through that key. When the key does not resolve, drop the reference from the decoded result instead of returning the encoded string.
🤖 Prompt for 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.
In `@crates/server/src/source/local.rs` around lines 144 - 165, The portable
reference format currently encodes the positional root index in encode_ref,
which is unstable across machines and reordered or removed directories. Replace
root_index-based encoding and decode_ref lookup with a stable, persisted root
identifier associated with each library root and resolve references by that key;
when no key matches, discard the reference rather than returning the encoded
kopuz-root-v1 string.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/db/src/backend/cfg_store.rs (1)
249-275: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for the monotonic merge invariant.
The supplied relocation test merges into an empty destination database. It does not verify that a smaller snapshot preserves an existing larger count. Add a test that verifies counts remain
2after merging1, remain2after merging2, and become3after merging3.🤖 Prompt for 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. In `@crates/db/src/backend/cfg_store.rs` around lines 249 - 275, Add a focused test for merge_listen_counts that seeds an existing listen count of 2, then merges snapshots containing 1, 2, and 3, asserting the stored count remains 2 for the first two merges and becomes 3 for the final merge. Reuse the existing database setup and count-query helpers from the nearby relocation tests.crates/db/src/lib.rs (1)
429-436: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winClarify
merge_listen_countsinput key format.
merge_listen_countsqualifies each input withsource.listen_count_key, so the trait should state thatcountscontains unqualified track keys from the portable/local library database; already-qualified application keys would be stored incorrectly.🤖 Prompt for 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. In `@crates/db/src/lib.rs` around lines 429 - 436, Clarify the documentation for the merge_listen_counts method to state that counts contains unqualified track keys from the portable/local library database. Explicitly note that the method applies source.listen_count_key to each key, so callers must not provide already-qualified application keys.
🤖 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 `@crates/hooks/src/portable_metadata.rs`:
- Around line 42-61: Update the polling logic around sync_portable_activity so
current is stored in observed and the Table generation counters are bumped only
after successful synchronization. On the Err(error) path, retain the previous
observed signature while preserving the warning, allowing the next poll to retry
the same change.
In `@crates/hooks/src/use_db_queries.rs`:
- Around line 250-258: Update the recently played query in use_recently_played
so database key resolution uses source_handle.source().clone() rather than the
separate source() value, ensuring both recent-key queries use the ActiveSource
identity. Retain active_source.read() as the reactive dependency and preserve
the existing source_handle recently_played call.
---
Nitpick comments:
In `@crates/db/src/backend/cfg_store.rs`:
- Around line 249-275: Add a focused test for merge_listen_counts that seeds an
existing listen count of 2, then merges snapshots containing 1, 2, and 3,
asserting the stored count remains 2 for the first two merges and becomes 3 for
the final merge. Reuse the existing database setup and count-query helpers from
the nearby relocation tests.
In `@crates/db/src/lib.rs`:
- Around line 429-436: Clarify the documentation for the merge_listen_counts
method to state that counts contains unqualified track keys from the
portable/local library database. Explicitly note that the method applies
source.listen_count_key to each key, so callers must not provide
already-qualified application keys.
🪄 Autofix
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 Plus
Run ID: e59a461f-2d77-48ef-8e7d-2122fe323d1b
📒 Files selected for processing (39)
crates/config/src/lib.rscrates/config/src/source.rscrates/db/src/backend/cfg_store.rscrates/db/src/backend/mod.rscrates/db/src/lib.rscrates/hooks/src/portable_metadata.rscrates/hooks/src/use_db_queries.rscrates/i18n/locales/ar.ftlcrates/i18n/locales/de.ftlcrates/i18n/locales/en.ftlcrates/i18n/locales/es.ftlcrates/i18n/locales/fil.ftlcrates/i18n/locales/fr.ftlcrates/i18n/locales/gr.ftlcrates/i18n/locales/he.ftlcrates/i18n/locales/hu.ftlcrates/i18n/locales/id.ftlcrates/i18n/locales/it.ftlcrates/i18n/locales/ja.ftlcrates/i18n/locales/ko.ftlcrates/i18n/locales/ml.ftlcrates/i18n/locales/nl.ftlcrates/i18n/locales/pl.ftlcrates/i18n/locales/pt-BR.ftlcrates/i18n/locales/pt-PT.ftlcrates/i18n/locales/ro.ftlcrates/i18n/locales/ru.ftlcrates/i18n/locales/sv.ftlcrates/i18n/locales/ta.ftlcrates/i18n/locales/tok-SP.ftlcrates/i18n/locales/tok.ftlcrates/i18n/locales/tr.ftlcrates/i18n/locales/uk.ftlcrates/i18n/locales/vi-VN.ftlcrates/i18n/locales/zh-CN.ftlcrates/kopuz/src/main.rscrates/server/src/source.rscrates/server/src/source/local.rscrates/server/tests/source.rs
🚧 Files skipped from review as they are similar to previous changes (33)
- crates/i18n/locales/id.ftl
- crates/i18n/locales/ko.ftl
- crates/config/src/lib.rs
- crates/i18n/locales/it.ftl
- crates/i18n/locales/fil.ftl
- crates/i18n/locales/de.ftl
- crates/i18n/locales/ml.ftl
- crates/i18n/locales/ta.ftl
- crates/i18n/locales/gr.ftl
- crates/i18n/locales/pt-BR.ftl
- crates/i18n/locales/nl.ftl
- crates/i18n/locales/vi-VN.ftl
- crates/i18n/locales/he.ftl
- crates/i18n/locales/pt-PT.ftl
- crates/i18n/locales/fr.ftl
- crates/i18n/locales/zh-CN.ftl
- crates/i18n/locales/ar.ftl
- crates/i18n/locales/uk.ftl
- crates/config/src/source.rs
- crates/i18n/locales/ru.ftl
- crates/i18n/locales/tok.ftl
- crates/i18n/locales/ja.ftl
- crates/kopuz/src/main.rs
- crates/i18n/locales/pl.ftl
- crates/i18n/locales/ro.ftl
- crates/i18n/locales/en.ftl
- crates/i18n/locales/hu.ftl
- crates/i18n/locales/es.ftl
- crates/i18n/locales/tr.ftl
- crates/i18n/locales/tok-SP.ftl
- crates/server/src/source/local.rs
- crates/i18n/locales/sv.ftl
- crates/server/src/source.rs
| if needs_refresh { | ||
| match source.sync_portable_activity().await { | ||
| Ok(counts) => { | ||
| let mut config = config.write(); | ||
| for (key, count) in counts { | ||
| let current = config.listen_counts.entry(key).or_insert(0); | ||
| *current = (*current).max(count); | ||
| } | ||
| } | ||
| Err(error) => { | ||
| tracing::warn!(%error, "failed to refresh shared library activity"); | ||
| } | ||
| } | ||
| gens.bump(Table::Favorites); | ||
| gens.bump(Table::Playlists); | ||
| gens.bump(Table::Folders); | ||
| gens.bump(Table::Recents); | ||
| gens.bump(Table::Tracks); | ||
| } | ||
| observed = Some(current); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Retry after a synchronization failure.
Line 61 stores current after sync_portable_activity() returns Err. The next poll sees the same signature and skips synchronization. A temporary database lock can leave portable activity unsynchronized until another file change occurs.
Only store current and bump generations after a successful synchronization. Keep the previous signature on failure so the polling loop retries.
Proposed fix
if needs_refresh {
match source.sync_portable_activity().await {
Ok(counts) => {
let mut config = config.write();
for (key, count) in counts {
let current = config.listen_counts.entry(key).or_insert(0);
*current = (*current).max(count);
}
+ gens.bump(Table::Favorites);
+ gens.bump(Table::Playlists);
+ gens.bump(Table::Folders);
+ gens.bump(Table::Recents);
+ gens.bump(Table::Tracks);
+ observed = Some(current);
}
Err(error) => {
tracing::warn!(%error, "failed to refresh shared library activity");
}
}
- gens.bump(Table::Favorites);
- gens.bump(Table::Playlists);
- gens.bump(Table::Folders);
- gens.bump(Table::Recents);
- gens.bump(Table::Tracks);
+} else {
+ observed = Some(current);
}
-observed = Some(current);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if needs_refresh { | |
| match source.sync_portable_activity().await { | |
| Ok(counts) => { | |
| let mut config = config.write(); | |
| for (key, count) in counts { | |
| let current = config.listen_counts.entry(key).or_insert(0); | |
| *current = (*current).max(count); | |
| } | |
| } | |
| Err(error) => { | |
| tracing::warn!(%error, "failed to refresh shared library activity"); | |
| } | |
| } | |
| gens.bump(Table::Favorites); | |
| gens.bump(Table::Playlists); | |
| gens.bump(Table::Folders); | |
| gens.bump(Table::Recents); | |
| gens.bump(Table::Tracks); | |
| } | |
| observed = Some(current); | |
| if needs_refresh { | |
| match source.sync_portable_activity().await { | |
| Ok(counts) => { | |
| let mut config = config.write(); | |
| for (key, count) in counts { | |
| let current = config.listen_counts.entry(key).or_insert(0); | |
| *current = (*current).max(count); | |
| } | |
| gens.bump(Table::Favorites); | |
| gens.bump(Table::Playlists); | |
| gens.bump(Table::Folders); | |
| gens.bump(Table::Recents); | |
| gens.bump(Table::Tracks); | |
| observed = Some(current); | |
| } | |
| Err(error) => { | |
| tracing::warn!(%error, "failed to refresh shared library activity"); | |
| } | |
| } | |
| } else { | |
| observed = Some(current); | |
| } |
🤖 Prompt for 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.
In `@crates/hooks/src/portable_metadata.rs` around lines 42 - 61, Update the
polling logic around sync_portable_activity so current is stored in observed and
the Table generation counters are bumped only after successful synchronization.
On the Err(error) path, retain the previous observed signature while preserving
the warning, allowing the next poll to retry the same change.
| let active_source = use_context::<Signal<server::source::ActiveSource>>(); | ||
| let gens = use_generations(); | ||
| use_resource(move || { | ||
| let _ = gens.generation(Table::Recents); | ||
| let (db, s) = (db.clone(), source()); | ||
| let (db, s, source_handle) = (db.clone(), source(), active_source.read().clone()); | ||
| let span = tracing::info_span!("query.recently_played", source = s.as_str()); | ||
| offload( | ||
| async move { | ||
| let keys = db.recently_played(&s, 50).await.unwrap_or_default(); | ||
| let keys = source_handle.recently_played(50).await.unwrap_or_default(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find callers and inspect which reactive source memo they pass.
rg -n -C 5 '\buse_recently_played\s*\(' crates
# Inspect ActiveSource updates and source-switch ordering.
rg -n -C 6 '\bActiveSource\b|active_source\.(set|write)\b' cratesRepository: Kopuz-org/kopuz
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== use_db_queries outline =="
ast-grep outline crates/hooks/src/use_db_queries.rs --view compact || true
echo
echo "== use_recently_played implementation =="
sed -n '236,276p' crates/hooks/src/use_db_queries.rs | cat -n -v
echo
echo "== source hook definition and ActiveSource definition =="
rg -n -C 4 'pub fn use_active_source|type.*ActiveSource|struct ActiveSource|fn source' crates/hooks crates/server/src crates/kopuz/src -g '*.rs'
echo
echo "== relevant main.rs identity/source effect =="
sed -n '460,524p' crates/kopuz/src/main.rs | cat -n -v
echo
echo "== read/write Accessors from ActiveSource source field =="
rg -n -C 3 'impl ActiveSource|ActiveSource|source::ActiveSource|\.source|fn source' crates/server crates/hooks -g '*.rs' | head -n 220Repository: Kopuz-org/kopuz
Length of output: 29832
Use one source identity for both recent-key queries.
recently_played() returns keys for the ActiveSource instance, but use_recently_played resolves keys with the Memo<Source> value passed by callers. During a source switch, those values can diverge and return recent tracks from one source resolved through another source partition. Derive the database lookup source from source_handle.source().clone(); keep active_source as the shared reactive dependency.
🤖 Prompt for 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.
In `@crates/hooks/src/use_db_queries.rs` around lines 250 - 258, Update the
recently played query in use_recently_played so database key resolution uses
source_handle.source().clone() rather than the separate source() value, ensuring
both recent-key queries use the ActiveSource identity. Retain
active_source.read() as the reactive dependency and preserve the existing
source_handle recently_played call.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/db/tests/persistence.rs (1)
274-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new cross-source folder scoping.
These updates only thread
&Source::Localthrough the existing assertions. The new behavior in this PR is the source partitioning itself, and no test exercises it. Add a case that creates folderf1forSource::Localand forSource::LocalLibrary("local:lib-1"), then asserts thatrename_folder,delete_folder, andload_playlistsfor one source do not affect the other. That case would also pin down thefolders.idconflict semantics flagged incrates/db/src/backend/writes.rs.I can generate the test. Do you want me to open an issue to track it?
🤖 Prompt for 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. In `@crates/db/tests/persistence.rs` around lines 274 - 295, Extend the persistence test around create_folder, rename_folder, delete_folder, and load_playlists with a second folder using the same id under Source::LocalLibrary("local:lib-1"). Assert that each source retains its own folder, and that renaming or deleting f1 for one source leaves the other source unchanged, covering source-scoped folders.id conflict behavior.crates/db/src/backend/dump.rs (1)
87-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse checked folder queries here too.
sqlx::query_as!keeps compile-time SQL checking;sqlx::query_asruns these only at runtime. The init migration already definesfolders.source, so these can move to checkedquery_as!with.sqlxupdated per thecrates/dbguideline.🤖 Prompt for 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. In `@crates/db/src/backend/dump.rs` around lines 87 - 99, Replace the runtime-checked query_as calls for folder_rows and member_rows with compile-time-checked sqlx::query_as! queries, preserving their existing SQL, bindings, result types, ordering, and fetch behavior. Update the crates/db .sqlx metadata according to the repository guideline so the checked queries compile successfully.Source: Coding guidelines
🤖 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 `@crates/db/src/backend/writes.rs`:
- Around line 766-775: Make folder identity source-scoped: update create_folder
to use (source, id) as the conflict target and stop updating source from
excluded, and add equivalent conflict handling to the folders INSERT in
replace_playlist_store so portable sync continues when IDs already exist. Add a
migration replacing the folders.id-only uniqueness constraint with the composite
(source, id) constraint; apply these changes at
crates/db/src/backend/writes.rs:766-775 and
crates/db/src/backend/writes.rs:736-742.
In `@crates/server/src/source/local.rs`:
- Around line 202-229: Replace the per-mutation tokio::spawn flow in
queue_portable_mutation with a single long-lived worker consuming an ordered
tokio::sync::mpsc queue, preserving submission order for dependent mutations.
Remove the portable_write_lock-based serialization and make the worker update
portable_pending and portable_idle only after the queue is drained, so
wait_for_portable_mutations and sync_portable_activity cannot observe a false
idle state.
---
Nitpick comments:
In `@crates/db/src/backend/dump.rs`:
- Around line 87-99: Replace the runtime-checked query_as calls for folder_rows
and member_rows with compile-time-checked sqlx::query_as! queries, preserving
their existing SQL, bindings, result types, ordering, and fetch behavior. Update
the crates/db .sqlx metadata according to the repository guideline so the
checked queries compile successfully.
In `@crates/db/tests/persistence.rs`:
- Around line 274-295: Extend the persistence test around create_folder,
rename_folder, delete_folder, and load_playlists with a second folder using the
same id under Source::LocalLibrary("local:lib-1"). Assert that each source
retains its own folder, and that renaming or deleting f1 for one source leaves
the other source unchanged, covering source-scoped folders.id conflict behavior.
🪄 Autofix
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 Plus
Run ID: 124d7fbe-7001-468a-9562-ca6857c0eeb8
📒 Files selected for processing (8)
crates/db/src/backend/dump.rscrates/db/src/backend/mod.rscrates/db/src/backend/writes.rscrates/db/src/lib.rscrates/db/tests/persistence.rscrates/server/src/source.rscrates/server/src/source/local.rscrates/server/tests/source.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/server/tests/source.rs
| sqlx::query( | ||
| "INSERT INTO folders (id, source, name) VALUES (?1, ?2, ?3) \ | ||
| ON CONFLICT(id) DO UPDATE SET source = excluded.source, name = excluded.name", | ||
| ) | ||
| .bind(id) | ||
| .bind(source.as_str()) | ||
| .bind(name) | ||
| .execute(pool) | ||
| .await?; | ||
| Ok(()) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Folder identity is global while every folder operation is source-scoped. create_folder conflicts on id alone, which makes folders.id unique across all sources. rename_folder, delete_folder, set_playlist_folder, and load_playlists all match on id AND source. That mismatch produces two failures: a cross-source id conflict aborts replace_playlist_store, and an upsert silently moves a folder between source partitions. Make (source, id) the folder identity, then align both statements with it.
crates/db/src/backend/writes.rs#L766-L775: change the conflict target fromidto(source, id)and stop overwritingsourcefromexcluded, socreate_foldercannot claim a folder owned by another source. This needs a migration that replaces the single-column uniqueness onfolders.idwith(source, id).crates/db/src/backend/writes.rs#L736-L742: add matching conflict handling to the foldersINSERTinsidereplace_playlist_store, so a snapshot folder id that already exists does not abort the whole portable sync transaction.
📍 Affects 1 file
crates/db/src/backend/writes.rs#L766-L775(this comment)crates/db/src/backend/writes.rs#L736-L742
🤖 Prompt for 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.
In `@crates/db/src/backend/writes.rs` around lines 766 - 775, Make folder identity
source-scoped: update create_folder to use (source, id) as the conflict target
and stop updating source from excluded, and add equivalent conflict handling to
the folders INSERT in replace_playlist_store so portable sync continues when IDs
already exist. Add a migration replacing the folders.id-only uniqueness
constraint with the composite (source, id) constraint; apply these changes at
crates/db/src/backend/writes.rs:766-775 and
crates/db/src/backend/writes.rs:736-742.
| fn queue_portable_mutation(&self, mutation: PortableMutation) { | ||
| if self.portable_path().is_none() || self.portable_failed.load(Ordering::Acquire) { | ||
| return; | ||
| } | ||
| self.portable_pending.fetch_add(1, Ordering::AcqRel); | ||
| let source = self.clone(); | ||
| tokio::spawn(async move { | ||
| let _write_guard = source.portable_write_lock.lock().await; | ||
| if let Some(portable) = source.portable_db().await | ||
| && let Err(error) = source.apply_portable_mutation(portable, mutation).await | ||
| { | ||
| source.disable_portable(&error); | ||
| } | ||
| if source.portable_pending.fetch_sub(1, Ordering::AcqRel) == 1 { | ||
| source.portable_idle.notify_waiters(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| async fn wait_for_portable_mutations(&self) { | ||
| while self.portable_pending.load(Ordering::Acquire) != 0 { | ||
| let idle = self.portable_idle.notified(); | ||
| if self.portable_pending.load(Ordering::Acquire) == 0 { | ||
| break; | ||
| } | ||
| idle.await; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Spawned mutations can apply to the portable database out of order.
queue_portable_mutation spawns one task per mutation. tokio::spawn does not preserve submission order, and portable_write_lock only serializes the tasks; it does not order them. Two dependent mutations can therefore apply in reverse.
A concrete sequence: create_playlist at Line 660 queues CreatePlaylist, then the UI calls add_to_playlist at Line 644, which queues AddPlaylistTracks for the same playlist id. If AddPlaylistTracks wins the lock, it appends refs first, and the later CreatePlaylist calls set_playlist_tracks, which replaces the membership and drops the appended refs. The app database keeps the correct membership, so the portable database silently diverges and another computer reads the wrong playlist. SetPlaylistTracks followed by DeletePlaylist, and CreateFolder followed by SetPlaylistFolder, have the same exposure.
The same design also weakens sync_portable_activity at Lines 571-576: a mutation queued between wait_for_portable_mutations and the lock makes the sync return Ok(Vec::new()), so the caller receives no counts and cannot tell that the sync did not run.
Replace the per-mutation spawn with one long-lived worker that consumes an ordered tokio::sync::mpsc queue. That preserves submission order, removes the need for portable_write_lock, and lets the idle signal reflect a drained queue.
🤖 Prompt for 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.
In `@crates/server/src/source/local.rs` around lines 202 - 229, Replace the
per-mutation tokio::spawn flow in queue_portable_mutation with a single
long-lived worker consuming an ordered tokio::sync::mpsc queue, preserving
submission order for dependent mutations. Remove the portable_write_lock-based
serialization and make the worker update portable_pending and portable_idle only
after the queue is drained, so wait_for_portable_mutations and
sync_portable_activity cannot observe a false idle state.
1 participant
store local favorites and playlists in
.kopuz-library.db, support different mount paths across computers, add per-library controls in Settings, refresh metadata changed by another computerSanity Checking
rules.
contribution guidelines, or this pull request did not use AI assistance.
Style and Consistency
style.
cargo fmt --all --checkorcargo fmt --allas appropriate.cargo clippy --workspace --all-targets -- -D warnings, orexplained why it could not be run.
this change depends on them.
Testing
Tested on platform(s):
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwin