feat(sync): wire playlist CRUD enqueue hooks (Phase 1.f.desktop.2b)#192
Conversation
Branches the local sync queue (built in #191) into every playlist mutation command. Once the user signs into a waveflow-server (Phase 1.f.desktop.1 / 1.f.desktop.1b), each CRUD action now appends a matching op to the local sync_pending_op log alongside the SQLite write. A future drain task (Phase 1.f.desktop.4) posts these to /api/v1/sync/ops and removes the rows the server accepts. New helper \`crate::sync::hooks::enqueue_op(state, draft)\` β single entry point the command handlers call. Returns nothing; every failure is logged via tracing and swallowed. The user's CRUD write already succeeded by the time we hit the queue, so aborting would surface confusing errors for sync-pipeline hiccups. When no waveflow_server JWT is stored for the active profile, the hook short-circuits without writing. Hooks wired into every mutation in commands/playlist.rs: create_playlist, update_playlist (one op per supplied field with its own lamport_ts), delete_playlist, add_track_to_playlist, add_tracks_to_playlist (coalesced into one op per batch), remove_track_from_playlist, reorder_playlist_track, add_source_to_playlist. Tests: composition is 4 lines of glue and each brick was unit tested by #191. Dedicated hook tests need a full AppState mock; manual smoke against sync_get_queue_state covers the integration. Out of scope (documented in PR description): - library.rs (device-specific folder paths) - edit.rs (writes to audio file metadata) - M3U import (compound op) Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
No actionable comments were generated in the recent review. π βΉοΈ Recent review infoβοΈ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: π Files selected for processing (1)
π WalkthroughWalkthroughCette PR ajoute ChangesSynchronisation des commandes Playlist
Sequence DiagramsequenceDiagram
participant Client as CommandePlaylist
participant Hooks as sync::hooks::enqueue_op
participant Inner as sync::hooks::enqueue_op_inner
participant Lamport as sync::lamport
participant Queue as sync::queue::enqueue
Client->>Hooks: PendingOpDraft
Hooks->>Inner: appel async
alt JWT prΓ©sent
Inner->>Lamport: demande timestamp
Inner->>Queue: enqueue(op + lamport_ts)
else Pas de JWT
Inner-->>Hooks: court-circuit (aucun enqueue)
end
alt Erreur
Hooks-->>Hooks: tracing::error! (champs struct)
end
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~25 minutes Possibly related PRs
Suggested labels
Poem
π₯ Pre-merge checks | β 5β Passed checks (5 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing Touchesπ Generate docstrings
π§ͺ Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
π€ 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 `@src-tauri/crates/app/src/commands/playlist.rs`:
- Around line 424-441: The sync payload is using the original new_position
rather than the clamped/effective position computed in reorder_track, causing
server replays to disagree with local order; update the call that constructs the
PendingOpDraft inside reorder_track (the crate::sync::hooks::enqueue_op
invocation) to send the clamped/effective position variable (the value after the
clamp logic) as "position" in the payload instead of the input new_position, so
the enqueued op matches the actual local reorder that was applied.
- Around line 105-123: The outbox write (crate::sync::hooks::enqueue_op with
PendingOpDraft) is done in a separate await after committing the local playlist
mutation, so a crash or failure can leave the local change persisted while the
sync op is lost; wrap the playlist mutation and the enqueue_op together in the
same SQLite transaction so they either both commit or both roll back. Modify the
handler in playlist.rs to begin a DB transaction, perform the playlist
insert/update using that transaction, and call a transactional variant of
enqueue_op (or change enqueue_op to accept a &Transaction / transactional
wrapper) so enqueue_op writes into the same transaction; on any error
return/await failure ensure the transaction is rolled back and only commit when
both steps succeed. Ensure the same pattern is applied to the other occurrences
you noted (lines ~179-206, 223-233, 312-322, 346-359, 380-390, 428-441, 482-495)
so playlist mutation and outbox enqueue are atomic.
πͺ 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f938d2a-6bcc-47d8-acd1-34c1cd569466
π Files selected for processing (3)
src-tauri/crates/app/src/commands/playlist.rssrc-tauri/crates/app/src/sync/hooks.rssrc-tauri/crates/app/src/sync/mod.rs
@coderabbitai PR #192 surfaced two findings: 1. reorder_playlist_track sent the raw new_position rather than the value the repo's internal clamp landed on. A "move to 999" in a 10-track playlist would replay on the server as 999; both sides re-clamping to (len-1) only converges as long as both sides agree on len. SELECT the post-write position from playlist_track and send THAT instead, so the sync payload mirrors the row's actual state. Falls back to new_position if the SELECT fails (defensive, doesn't block the user's edit). 2. The SQLite write + enqueue_op are two separate commits, so a crash in the few-ms window between them leaves the local state ahead of the queue. CR is right that this should be atomic. The proper fix needs every Sqlite*Repository method on the playlist surface to accept &mut SqliteConnection β a waveflow-core change wide enough for its own PR. Documented the gap explicitly in the sync::hooks module with the target API shape (enqueue_op_in_tx + tx.commit pattern) so the follow-up reviewer has the contract already laid out. Keeping the bounded-window behaviour for now since the drain task (1.f.desktop.4) is positioned to detect any drift via operation_id reconciliation. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Actionable comments posted: 1
βΉοΈ Review info
βοΈ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0d7ebb93-98f1-4740-90f7-0c7e54f044dd
π Files selected for processing (2)
src-tauri/crates/app/src/commands/playlist.rssrc-tauri/crates/app/src/sync/hooks.rs
β¦g back to raw position @coderabbitai PR #192 caught that my previous patch fell back to the raw new_position when the post-write SELECT failed β which silently reintroduces the exact divergence the readback was supposed to prevent. Match-on-result instead: log + return Ok(()) on either Ok(None) (row vanished concurrently) or Err (DB hiccup). The local reorder still stands; the sync op for this one action just drops, and the next mutation requeues normally. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
* feat(sync): per-profile sync mode toggle (Phase 1.f.desktop.3) Adds the user-facing "off-switch" the enqueue hooks #192 shipped need to be controllable. Per-profile sync mode persisted in profile_setting['sync.mode']; the existing sync::hooks gate now short-circuits when mode = Local even with a JWT configured. ## Backend (~250 LOC) - crate::sync::mode β SyncMode enum (Local, Hybrid), read/write helpers around profile_setting, 5 unit tests covering fresh-profile default, round-trip both directions, unknown-storage-value fallback, and const as_str/from_storage symmetry. - sync::hooks::enqueue_op gate: if no JWT skip; if SyncMode::Local skip; otherwise lamport::next + queue::enqueue. Fresh profile defaults to Hybrid so the post-sign-in flow Just Works. - New Tauri commands sync_get_mode and sync_set_mode (canonical string round-trip, rejects unknown modes with 400-style error). - sync_get_queue_state extended with a mode field so the Settings card renders both queue stats AND the active mode in one round-trip. ## Frontend (~120 LOC) - src/lib/tauri/serverAuth.ts wrapper: SyncMode type + syncGetMode + syncSetMode. - ServerAccountCard gains a radio under the JWT section, gated on signedIn && mode loaded so we don't flash an empty group during hydration. Promise.all([serverGetStatus, syncGetMode]) on initial load batches the two reads. - syncGetMode failure during initial hydration is caught and the radio simply stays hidden rather than blowing up the whole card (race against profile-switch). ## i18n - settings.serverAccount.{modeLabel, modes.hybrid.label, modes.hybrid.description, modes.local.label, modes.local.description} propagated to all 17 locales per the CLAUDE.md convention. ## Why no Server-connected mode? RFC-001 listed a third "thin-client" mode where reads come from HTTP instead of local SQLite. Deferred β waveflow-web already covers the thin-client use case, and routing desktop reads through HTTP forfeits the value the local audio engine + file scanner provide. The SyncMode enum is intentionally open-shaped so a future ServerOnly variant lands without touching the persistence + gate logic. ## Test plan - cargo test -p waveflow --lib sync::mode (5/5 green) - cargo clippy -p waveflow --all-targets -- -D warnings - cargo fmt -p waveflow --check - bun run typecheck - bun run lint - Manual smoke (requires #190 + waveflow-web #18 deployed): - Sign in to a profile β Settings β Compte serveur shows radio defaulted to "Hybrid" - Create a playlist β sync_get_queue_state shows pending_count: 1 - Flip to Local β pending_count stays 1, no new ops enqueue - Update playlist name β pending_count stays 1 (queue gated by mode) - Flip back to Hybrid β update playlist again β pending_count: 2 Signed-off-by: InstaZDLL <github.105mh@8shield.net> * fix(sync): update value_type on profile_setting upsert conflict @coderabbitai on PR #194 flagged that the mode::write UPSERT didn't refresh value_type on conflict. For sync.mode specifically value_type never drifts (mode::write is the only writer and always inserts 'string'), but the fix is four characters and closes the class of bugs where a hypothetical future writer puts a wrong type in the row that this UPSERT would then silently preserve. Cheap defence in depth β same shape every future UPSERT against profile_setting should adopt. Signed-off-by: InstaZDLL <github.105mh@8shield.net> --------- Signed-off-by: InstaZDLL <github.105mh@8shield.net>
* refactor(sync): atomic playlist write + outbox enqueue in one tx Closes #193. Closes the drift window #192 documented where the playlist write committed in one SQLite transaction and the matching sync_pending_op row landed in a separate await β a crash or DB hiccup between the two could leave the local state ahead of the queue and the server would never hear about the user's edit. ## waveflow-core Every write method on SqlitePlaylistRepository now has a sibling free function in the same module taking &mut SqliteConnection (insert_custom_conn, update_conn, delete_conn, append_track_conn, append_tracks_conn, remove_track_conn, reorder_track_conn). The trait impls become thin wrappers that acquire pool.begin() and delegate, so every existing caller keeps working unchanged. reorder_track_conn returns Option<i64> (effective position post-clamp) instead of bool, so the caller can stamp the sync payload with the row's actual new state β closes the divergence the #192 readback patch worked around with a post-write SELECT. The trait method maps the Option to a bool for back-compat. ## waveflow desktop - lamport::next_conn, queue::enqueue_conn, mode::read_conn, server_client::read_token_conn β sibling variants that take &mut SqliteConnection so they compose in a caller's open tx. - sync::hooks::enqueue_op_in_tx(conn, draft) β atomic outbox path. Returns AppResult<bool> so the caller can ? the failure and let the surrounding tx roll back the entity write too. The fire-and- forget enqueue_op variant is gone β playlist commands all use the atomic path now, and a future non-playlist hook would too. Module docstring rewritten: atomicity is no longer a documented known-gap, it's the guaranteed shape. ## commands/playlist.rs The 8 mutation commands (create / update / delete / add_track / add_tracks / add_source / remove_track / reorder) all wrap their write + enqueue in a single pool.begin() β ..._conn(&mut tx) β ... β enqueue_op_in_tx(&mut tx) β tx.commit() block. Filesystem-level side effects (playlist_cover::maybe_regen_auto_cover) run OUTSIDE the tx as before β they're not transactional and shouldn't pin the write window open. reorder_playlist_track sheds its post-write SELECT (the position readback #192 added) because reorder_track_conn now returns the effective position directly inside the same tx. ## Tests - 22 desktop sync unit tests still green. - 35 waveflow-core unit tests still green. - The trait impl methods (existing call paths) are now thin wrappers, so behaviour is unchanged for everyone except the newly-tx-aware playlist commands. ## Out of scope - library + edit hooks still use the (now-removed) fire-and-forget path conceptually, but they're not actually hooked anywhere yet (#192 deferred them). When they get wired, they'll use enqueue_op_in_tx like playlist. Signed-off-by: InstaZDLL <github.105mh@8shield.net> * fix(sync): close two race windows in playlist atomic path @coderabbitai PR #195 surfaced two valid windows the initial atomic refactor missed: 1. remove_track_from_playlist enqueued a "delete tracks" op even when remove_track_conn returned false (track wasn't in the playlist β concurrent removal, double-click, stale UI state). The server would then replay the delete and drop a row that legitimately belonged there. Capture the boolean; only enqueue when removed == true. Tx still commits so the no-op stays idempotent from the caller's POV. Cover regen also gated on the removed flag since there's no cover change to recompute. 2. update_playlist did its exists() probe BEFORE opening the tx, so a concurrent delete between the two could leave update_conn touching zero rows while the per-field enqueue loop still fired. Refactor update_conn in waveflow-core to return CoreResult<bool> (rows_affected > 0). The trait method drops the boolean for back-compat (non-tx callers like smart_playlists never consumed it anyway). The command site uses the new signal: same-tx existence check, return the same 404-style error when false, tx auto-rolls-back on drop without enqueueing anything. 22 desktop sync tests still green, 35 core tests still green, clippy + fmt clean. Signed-off-by: InstaZDLL <github.105mh@8shield.net> --------- Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Branches the local sync queue (built in #191) into every playlist mutation command. Once the user signs into a waveflow-server (Phase 1.f.desktop.1 / 1.f.desktop.1b), each CRUD action now appends a matching op to the local `sync_pending_op` log alongside the SQLite write. A future drain task (Phase 1.f.desktop.4) posts these to `/api/v1/sync/ops` and removes the rows the server accepts.
New helper
`crate::sync::hooks::enqueue_op(state, draft)` β single entry point the command handlers call. Returns nothing; every failure is logged via `tracing` and swallowed. Two rationales documented in the module docstring:
When no `waveflow_server` JWT is stored for the active profile, `enqueue_op` short-circuits without writing. A local-only user never accumulates ops they'll never sync.
Hooks wired
Test plan
Composition is 4 lines of glue (`read_token` β `lamport::next` β `queue::enqueue`); each brick was individually tested by the 17 unit tests #191 shipped. Dedicated hook-level tests would require constructing a full `AppState` (paths + app_db + active profile + dlna), harder than the integration surface is worth at this scope.
Out of scope (documented in the helper module docstring)
Summary by CodeRabbit