feat(sync): ws subscriber + canonical-id mapping (Phase 1.f.desktop.4b)#199
Merged
Conversation
Closes the multi-device sync loop opened by the outbound drain task in 1.f.desktop.4a: the desktop now receives ops broadcast by peer devices via WebSocket, applies them through a canonical-id mapping, and catches up via REST on reconnect. - Migration plants `playlist.canonical_id` + `sync_id_map` + backfills every existing playlist with a fresh UUIDv4 + seeds the mapping. - `sync::canonical` is the local<->canonical translation layer; every outbound op now carries the canonical UUID as `entity_id` instead of the local rowid, so peer devices can route to their own rowid. - `sync::apply::apply_remote_op_in_tx` mirrors `enqueue_op_in_tx` on the inbound side WITHOUT touching the outbox (avoids ping-pong). Handles insert/delete/set on whole entity + name/desc/color/icon + track-list ops, with last-writer-wins on lamport_ts. - `sync::ws` runs the WebSocket subscriber + catch-up REST pull loop with exponential backoff and 410 Gone (resurrected device) handling. Frames feed the apply path; ACKs advance both the local cursor (`profile_setting['sync.last_seen_id']`) and the server cursor row. - `sync::cursor` persists last_seen_id with the same TEXT-affinity defence the lamport bump uses. - `sync::lamport::observe_remote_conn` joins the apply tx so the bump + entity write + mapping land atomically. Outbound playlist hooks refactored to mint canonical-ids inside the tx and emit the UUID as entity_id. Boot wires `sync::ws::spawn` alongside drain; both gate on mode=Hybrid + JWT. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
InstaZDLL
added
scope: backend
|
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)
📝 WalkthroughWalkthroughImplémente la pile client de synchronisation multi‑device : migration canonical_id pour playlists, mapping canonical↔local, curseur de progression, apply d'op distantes, client WebSocket avec rattrapage REST, et intégration dans l'app. ChangesDesktop Multi-Device Sync System
Sequence DiagramsequenceDiagram
participant App as App
participant WSClient as sync::ws
participant Server as WaveflowServer
participant DB as ProfileDB
App->>WSClient: spawn background subscriber
WSClient->>Server: GET /api/v1/sync/ops?since=N
Server-->>WSClient: page of ops or 410 Gone
WSClient->>DB: begin tx and apply op (apply_remote_op_in_tx + cursor.advance_conn)
DB-->>WSClient: commit tx
WSClient->>Server: POST /api/v1/sync/ack
WSClient->>Server: open wss://.../api/v1/sync/ws?device_id=...
Server-->>WSClient: {"type":"op", ...}
WSClient->>DB: begin tx and apply op (apply_remote_op_in_tx + cursor.advance_conn)
DB-->>WSClient: commit tx
WSClient->>Server: WS {"ack": op_id}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/sync/apply.rs`:
- Around line 155-190: La synchro échoue actuellement sur payloads invalides
parce que des appels comme playlist_draft_from_payload,
string_value_from_payload (et autres parseurs dans les branches autour de
252-357) propagent l'erreur via ?; change ces appels pour capturer les erreurs
de parsing et traiter les ops malformées comme non-appliquées (return
Ok(AppliedOutcome::Ignored)) tout en conservant la propagation des vraies
erreurs DB; concretement, remplacer usages de playlist_draft_from_payload(op,
now)? et string_value_from_payload(op)? (et autres parseurs similaires) par un
match/if let Ok(...) { ... } else { tracing::warn!(...); return
Ok(AppliedOutcome::Ignored); } et garder les appels DB (insert_custom_conn,
canonical::local_for_canonical, delete_conn, canonical::set_canonical_playlist,
canonical::drop_mapping, etc.) inchangés pour que seules les erreurs de payload
soient neutralisées.
In `@src-tauri/crates/app/src/sync/ws.rs`:
- Around line 356-361: The `applied` counter is being incremented for every
variant because the matches! includes AppliedOutcome::Skipped and ::Ignored, so
either restrict the increment to only AppliedOutcome::Applied (change the
condition to match only AppliedOutcome::Applied) or, if the intent is to count
all processed ops, rename the variable `applied` to something like
`processed`/`handled` and update usages; locate the increment around the
`applied += 1` line and the enum `AppliedOutcome` to apply the fix.
In `@src-tauri/migrations/profile/20260603000000_sync_canonical_id.sql`:
- Around line 28-58: The migration leaves playlist.canonical_id nullable (SQLite
UNIQUE allows multiple NULLs), so add runtime enforcement: create a BEFORE
INSERT trigger (e.g. trg_playlist_set_canonical_id) that generates and sets the
same UUIDv4 expression when NEW.canonical_id IS NULL (reuse the same
randomblob/hex/ substr(...) expression from the UPDATE that backfills via WITH
src) and a BEFORE UPDATE trigger (e.g. trg_playlist_prevent_null_canonical_id)
that prevents or regenerates if NEW.canonical_id IS NULL, ensuring no future row
can be created/updated with canonical_id NULL; keep the existing UPDATE backfill
and the CREATE UNIQUE INDEX idx_playlist_canonical_id as-is so the triggers and
index together enforce the non-null+unique invariant.
🪄 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: 6404bb89-f03a-4954-b266-a254390cfe30
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock,!src-tauri/Cargo.lock
📒 Files selected for processing (12)
src-tauri/crates/app/Cargo.tomlsrc-tauri/crates/app/src/commands/playlist.rssrc-tauri/crates/app/src/commands/sync.rssrc-tauri/crates/app/src/lib.rssrc-tauri/crates/app/src/state.rssrc-tauri/crates/app/src/sync/apply.rssrc-tauri/crates/app/src/sync/canonical.rssrc-tauri/crates/app/src/sync/cursor.rssrc-tauri/crates/app/src/sync/lamport.rssrc-tauri/crates/app/src/sync/mod.rssrc-tauri/crates/app/src/sync/ws.rssrc-tauri/migrations/profile/20260603000000_sync_canonical_id.sql
- apply: parser errors return Ignored instead of bubbling as Err — a malformed frame would otherwise roll back the tx, leave the cursor unmoved, and replay forever on reconnect. - ws: live_applied + catchup_applied only count Applied; echoes and ignores still advance the cursor without inflating the counter. - migration: AFTER INSERT trigger plants a fresh UUID when one is missing, BEFORE UPDATE trigger rejects setting canonical_id to NULL. Together with the UNIQUE index, the table behaves as NOT NULL UNIQUE without an ALTER COLUMN. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
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 `@src-tauri/crates/app/src/sync/apply.rs`:
- Around line 204-215: The handlers in apply.rs currently treat partial or
absent parses as successful (e.g., string_value_from_payload returning Ok(None)
or track_ids_from_payload using filter_map), allowing malformed payloads to be
partially applied; change the logic so that any non-exact type mismatch is
treated as an error and the whole op is ignored: for string_value_from_payload
(used where you currently match Ok(v) / Err(err)) treat Ok(None) as Err and
return AppliedOutcome::Ignored with a warning, and replace/adjust
track_ids_from_payload usage so it returns Err if any element fails to parse (do
not silently filter_map away bad elements) — update the callers in apply.rs (the
blocks around string_value_from_payload and calls to track_ids_from_payload) to
propagate those errors and return AppliedOutcome::Ignored on malformed payloads.
🪄 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: 6882a6a0-4a43-44dc-8216-734b4a1758e3
📒 Files selected for processing (3)
src-tauri/crates/app/src/sync/apply.rssrc-tauri/crates/app/src/sync/ws.rssrc-tauri/migrations/profile/20260603000000_sync_canonical_id.sql
string_value_from_payload accepted any JSON type via as_str() → None fallback, silently coercing a number/bool payload to 'clear the field'. track_ids_from_payload filter_mapped non-integer entries, applying a partial track op. Both paths now reject type mismatches as Err so the caller short-circuits to Ignored. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src-tauri/crates/app/src/sync/apply.rs (1)
371-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winN’accepte pas un
valueabsent comme un clear valide.
{}ou tout payload sans clévaluesort encore enOk(None)ici, donc une frame malformée peut effacer le champ au lieu d’être ignorée. Ça réintroduit une mutation silencieuse alors que le contrat de ce module est bienpayloadinvalide ⇒Ignored.Correctif minimal
match payload.get("value") { Some(serde_json::Value::String(s)) => Ok(Some(s.clone())), - Some(serde_json::Value::Null) | None => Ok(None), + Some(serde_json::Value::Null) => Ok(None), + None => Err(AppError::Other("set op: value missing".into())), Some(_) => Err(AppError::Other( "set op: value must be a string or null".into(), )), }🤖 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 `@src-tauri/crates/app/src/sync/apply.rs` around lines 371 - 380, The current match on payload.get("value") treats a missing "value" key (None) as Ok(None), allowing malformed frames to silently clear fields; change the match so only Some(serde_json::Value::Null) returns Ok(None) while a missing key (None) returns the module's Ignored error (e.g. Err(Ignored) or Err(ApplyError::Ignored) as appropriate). Keep the Some(String) => Ok(Some(...)) arm unchanged and update any imports/usages so the correct Ignored error type is returned from the function.
🤖 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/sync/apply.rs`:
- Around line 900-944: The test malformed_tracks_array_mixed_types_is_ignored
currently seeds no tracks so a permissive track_ids_from_payload could still
produce a green test; modify the test to seed at least one track row whose id
matches one of the numeric IDs in the payload (e.g. id 1) before calling
apply_remote_op_in_tx so that filter_existing_track_ids would leave a non-empty
list if partial applies were allowed; add a clear seeding step (using the
existing test DB helper or a sqlx insert) prior to the two apply_remote_op_in_tx
calls and keep the final assertion that playlist_track count is 0 to ensure
partial inserts would fail the test.
---
Duplicate comments:
In `@src-tauri/crates/app/src/sync/apply.rs`:
- Around line 371-380: The current match on payload.get("value") treats a
missing "value" key (None) as Ok(None), allowing malformed frames to silently
clear fields; change the match so only Some(serde_json::Value::Null) returns
Ok(None) while a missing key (None) returns the module's Ignored error (e.g.
Err(Ignored) or Err(ApplyError::Ignored) as appropriate). Keep the Some(String)
=> Ok(Some(...)) arm unchanged and update any imports/usages so the correct
Ignored error type is returned from the function.
🪄 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: 4f43f254-c5fa-46fc-ad36-bb76dd36761d
📒 Files selected for processing (1)
src-tauri/crates/app/src/sync/apply.rs
- string_value_from_payload: a missing 'value' key now returns Err (was Ok(None)). Outbound never emits an empty payload, so missing key is corruption and should land Ignored, not silently clear the field. - malformed_tracks_array_mixed_types_is_ignored now seeds track id=1 so the OLD filter_map behaviour would have appended a row. The green test now pins the strict-array invariant rather than the empty-resolved short-circuit. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
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 `@src-tauri/crates/app/src/sync/apply.rs`:
- Around line 371-385: The shared helper string_value_from_payload() currently
treats {"value": null} as a valid reset (Ok(None)) for all fields, but name,
color_id and icon_id must reject nulls; update the code to distinguish nullable
vs non-nullable fields: either add a boolean parameter (e.g. allow_null: bool)
to string_value_from_payload() and return Err(AppError::Other("set op: value
must be a string"…)) when allow_null is false and value is Null, or introduce a
separate non_nullable_string_value_from_payload() used for name, color_id and
icon_id that treats Null as an error (and keeps description using the nullable
variant); ensure the returned error text remains clear (e.g. "set op: value must
be a string" for non-nullable fields) and update all call sites where name,
color_id and icon_id are processed to use the non-nullable path.
🪄 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: c424cd06-7c6a-4430-bec2-5e2e03cfa488
📒 Files selected for processing (1)
src-tauri/crates/app/src/sync/apply.rs
Schema invariant: only `description` is nullable on `playlist`. A
`{"value": null}` frame on `name` / `color_id` / `icon_id` used to
return Ok(None); the COALESCE in `update_conn` absorbed it as a
silent no-op, but the op still surfaced as Applied. Match now keys
on the field — null is Applied only for description and Ignored
elsewhere.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/crates/app/src/sync/apply.rs (1)
204-217:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLe
nullsurdescriptionest perdu avantupdate_conn.Ici,
description = nulldevientOk(None), puisbuild_patch("description", None)produit le mêmePlaylistUpdatequ’un no-op. Commeupdate_connne reçoit plus lefield, il ne peut pas distinguer “clear explicite” de “ne rien toucher”, donc l’effacement distant peut être accepté sans être appliqué localement. Le testset_value_null_clears_fieldne verrouille pas cette régression, car il n’assert pas queplaylist.descriptionest bien passée àNULL.Correctif attendu
- encode trois états côté repo (
unchanged/set(string)/set(null)), ou- ajoute un chemin dédié pour
description = NULLexposé parcrates/core, puis- fais un assert en base dans
set_value_null_clears_fieldpour vérifier que la colonne vaut bienNULL.As per coding guidelines, "Vérifie que la logique réutilisable côté serveur reste poussée vers
crates/core/plutôt que dupliquée ici."Also applies to: 383-383, 912-952
🤖 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 `@src-tauri/crates/app/src/sync/apply.rs` around lines 204 - 217, La valeur null pour `description` est perdue parce que `string_value_from_payload` retourne Ok(None), `build_patch("description", None)` produces a no-op `PlaylistUpdate` so `update_conn(conn, local_id, &patch, now)` cannot tell "clear to NULL" from "leave unchanged"; fix by moving/adding three-state encoding into shared core logic (crates/core) so payload -> repo update represents {unchanged | set(string) | set(null>) (or add a dedicated sentinel/enum variant for NULL), update `build_patch`/`PlaylistUpdate` creators to carry that variant instead of Option<String>, and ensure `update_conn` and its callers (`string_value_from_payload`, `build_patch`, `update_conn`) handle the explicit "set null" path; also update the test `set_value_null_clears_field` to assert the DB column is actually NULL. Ensure the same change is applied to the other occurrences noted in the comment.
🤖 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.
Outside diff comments:
In `@src-tauri/crates/app/src/sync/apply.rs`:
- Around line 204-217: La valeur null pour `description` est perdue parce que
`string_value_from_payload` retourne Ok(None), `build_patch("description",
None)` produces a no-op `PlaylistUpdate` so `update_conn(conn, local_id, &patch,
now)` cannot tell "clear to NULL" from "leave unchanged"; fix by moving/adding
three-state encoding into shared core logic (crates/core) so payload -> repo
update represents {unchanged | set(string) | set(null>) (or add a dedicated
sentinel/enum variant for NULL), update `build_patch`/`PlaylistUpdate` creators
to carry that variant instead of Option<String>, and ensure `update_conn` and
its callers (`string_value_from_payload`, `build_patch`, `update_conn`) handle
the explicit "set null" path; also update the test `set_value_null_clears_field`
to assert the DB column is actually NULL. Ensure the same change is applied to
the other occurrences noted in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 503787d2-c8fd-4c4b-8cba-b8656232fb40
📒 Files selected for processing (1)
src-tauri/crates/app/src/sync/apply.rs
The previous null-on-description path returned Ok(None) and let update_conn's COALESCE silently leave the column unchanged — the op surfaced Applied but the DB never moved. Rather than wire a three-state encoding through crates/core for a path the outbound side does not emit today, reject null on every field for now and defer the proper clear-to-NULL feature to a real product surface. The renamed test pins the actual DB state: description must equal 'old' after the null op, not just rely on the outcome label. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/crates/app/src/sync/apply.rs (1)
388-404: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueLe type de retour
Option<String>est désormais superflu.Depuis que
nullest rejeté pour tous les champs, la fonction ne peut plus retournerOk(None)— seulsOk(Some(...))ouErr(...)sont possibles. Le type de retour pourrait être simplifié enAppResult<String>pour refléter ce contrat.♻️ Simplification proposée
-fn string_value_from_payload(op: &RemoteSyncOp, field: &str) -> AppResult<Option<String>> { +fn string_value_from_payload(op: &RemoteSyncOp, field: &str) -> AppResult<String> { let payload = op .payload .as_ref() .ok_or_else(|| AppError::Other("set op missing payload (expected {value: ...})".into()))?; match payload.get("value") { - Some(serde_json::Value::String(s)) => Ok(Some(s.clone())), + Some(serde_json::Value::String(s)) => Ok(s.clone()), Some(serde_json::Value::Null) => Err(AppError::Other(format!( "set op: '{field}' value cannot be null — outbound never emits null today \ and the inbound clear path is not wired through (see module docstring)" ))), Some(_) => Err(AppError::Other("set op: value must be a string".into())), None => Err(AppError::Other( "set op: payload missing required 'value' key".into(), )), } }Puis au call-site (ligne 216) :
-let patch = build_patch(field, value); +let patch = build_patch(field, Some(value));🤖 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 `@src-tauri/crates/app/src/sync/apply.rs` around lines 388 - 404, Change the function string_value_from_payload to return AppResult<String> (not AppResult<Option<String>>): update the signature and its match arms so the String case returns Ok(s.clone()) directly and remove any Ok(None) possibility; keep the Null and other/error branches as Err(...) and the missing "value" case as Err(...). Then update the call-site(s) that currently expect Option<String> to accept a String (handle the Err paths unchanged and remove any handling for None). Ensure you only reference string_value_from_payload and adjust its callers accordingly.
🤖 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.
Outside diff comments:
In `@src-tauri/crates/app/src/sync/apply.rs`:
- Around line 388-404: Change the function string_value_from_payload to return
AppResult<String> (not AppResult<Option<String>>): update the signature and its
match arms so the String case returns Ok(s.clone()) directly and remove any
Ok(None) possibility; keep the Null and other/error branches as Err(...) and the
missing "value" case as Err(...). Then update the call-site(s) that currently
expect Option<String> to accept a String (handle the Err paths unchanged and
remove any handling for None). Ensure you only reference
string_value_from_payload and adjust its callers accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6339819f-5c7d-46ef-aaac-5a7d56437e1c
📒 Files selected for processing (1)
src-tauri/crates/app/src/sync/apply.rs
- rustls: install ring CryptoProvider at boot. tokio-tungstenite 0.29 with rustls 0.23 needs a process-level default; without it the first wss:// handshake panics. reqwest configures its own per-connector and doesn't install the global one. - apply: filter_existing_track_ids batches into one IN-clause SELECT + HashSet filter, preserves input order. Was N+1 (one SELECT per id) for no reason. - canonical: set_canonical_playlist defensively DELETEs prior (entity, local_id) rows pointing at a different canonical before INSERTing the new mapping. Closes the "orphan mapping" path even though current callers can't trigger it. - ws/cursor: extract cursor::reset(pool) so the 410 Gone handler doesn't open-code a DELETE against profile_setting. - migration: replace abs(random()) % 4 with random() & 3 in the variant-nibble expression (both backfill + AFTER INSERT trigger). abs(INT64_MIN) is an integer overflow in SQLite; the bitmask is overflow-safe. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Since the helper rejects null on every field, the only Ok branch is Ok(Some(String)). Return String directly; caller wraps with Some at the build_patch call site. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the multi-device sync loop opened by #196 (Phase 1.f.desktop.4a). The desktop now receives ops broadcast by peer devices via WebSocket, applies them through a canonical-id mapping, and catches up via REST on reconnect.
This is the final piece of Phase 1.f desktop side. With this PR, two desktops signed into the same Better Auth account on the same
waveflow-serverwill see each other's playlist edits in (near) real time.What lands
Canonical-id mapping
A new migration plants
playlist.canonical_id TEXT UNIQUE+ async_id_map (entity, canonical_id, local_id)table, and backfills every pre-existing playlist row with a fresh UUIDv4 generated viarandomblob(16)+ version/variant nibble patching. The mapping table is seeded for those backfilled rows.Why: the outbound queue up to 4a sent the local
playlist.id INTEGERasentity_id. Fine for push (server keys on(user_id, device_id, …)so disjoint namespaces); not for inbound — device A's playlist#42 isn't device B's playlist#42. Every outbound op now carries the canonical UUID; every inbound op routes throughsync_id_map.Inbound apply path (
sync::apply)apply_remote_op_in_tx(conn, &RemoteSyncOp, local_device_id) -> AppliedOutcome:op.device_id == local_device_id) without touching the local DB.lamport::observe_remote_connpast the remotelamport_tsIN THE SAME TX.(entity, op, field). Today handlesplaylist:insert/deletewhole entity (mints/drops the mapping row)setonname | description | color_id | icon_idinsert | delete | setontracks(track-list ops translate toappend_tracks_conn/remove_track_conn/reorder_track_conn)AppliedOutcome::Ignored(cursor still advances).enqueue_op_in_txentirely — no inbound→outbox ping-pong.WebSocket subscriber (
sync::ws)Spawned at boot alongside the drain task. Each session:
GET /api/v1/sync/ops?since=NwhereN = profile_setting['sync.last_seen_id']. Pages until the server returns short; each op opens a fresh tx forapply + cursor::advance_conn + commit. A410 Gone(compaction watermark above our cursor) resets the cursor to 0 and re-pulls.wss://<server>/api/v1/sync/ws?device_id=…with the Bearer JWT in theAuthorizationupgrade header (the server's middleware extracts it BEFORE routing to the WS handler). Each{"type":"op","op":{…}}frame feeds the same apply path; server-id advances the cursor;{"ack": N}frames go back upstream.Localmode short-circuits cleanly. ASubscribeHandle::wake()onAppState::wslets thesync_set_modecommand nudge the loop without waiting on the 30 s idle gate.Outbound hooks refactored
commands/playlist.rsnow callssync::canonical::ensure_local_playlist(&mut tx, id)inside every CRUD tx and emits the returned UUID asentity_id. Delete drops the mapping row in the same tx so future ops referencing the same canonical resolve to nothing instead of a dangling rowid.Architecture invariants
lamport observe + apply + cursor advanceeither all commit or all roll back. A crash mid-op leaves the cursor unmoved → next reconnect's catch-up replays.playlist.canonical_idis a convenience for JOIN-free queries;sync_id_mapis whatapply_remote_op_in_txkeys on for cross-device routing.lamport_ts: per-field ops commute trivially; concurrentset namecollisions resolve via the higher lamport (the server's monotonic id orders them on the wire).Tests
sync::canonical(5 tests): mints UUID, idempotent, lookup round-trip, set/overwrite from server, drop_mapping idempotentsync::apply(6 tests): insert + mapping, echo skipped, duplicate idempotent, set translates via mapping, delete-then-replay ignored, unknown entity ignoredsync::cursor(3 tests): fresh = 0, monotonic, no-op on zero/negativesync::ws(4 tests): http→ws URL rewrite (scheme + path), URL-encoded device_id, scheme rejection, frame parse + unknown type rejectionAll
cargo test --workspace+cargo clippy --all-targets -- -D warnings+bun run typecheckgreen.Out of scope (deferred)
library/track/liked_trackenqueue + apply hooks — same pattern, each gets its own canonical-id migration in a follow-up.trackops: the apply path currently filterstrack_idsagainst the localtracktable directly (drops unknown ids). A future sub-PR will mirrorsync_id_mapfor tracks.SubscribeOutcome(catchup_applied / live_applied counts).Test plan
waveflow-servercargo test --workspace,cargo clippy,bun run typecheckSummary by CodeRabbit
Nouvelles Fonctionnalités
Améliorations
Tests
Chores