Repository navigation
feat(sounds): sync saved sound library across devices - #6856
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Review notes (agent review — my own PR, so comment rather than approval)I read the
Two things to deal with. 1. The PR has merge conflicts
2. Two devices on first run can fork the vault key permanently
That's the exact failure the file's own doc warns about ("publishing a second vault key would replace the first and render every previously synced item permanently unreadable") — just reached device-to-device instead of callsite-to-callsite. And installing on a second device is a completely ordinary thing to do the same evening you install on the first, which is precisely when both caches are cold.
Given the whole design rests on the vault key being singular per account, I don't think this should ship without a read-back. 3. Newest-wins has no tie-break (makes #2 worse)Both readers resolve "newest" by // vault_key_service.dart
..sort((a, b) => b.createdAt.compareTo(a.createdAt));
// sync_index_client.dart
if (existing == null || event.createdAt > existing.createdAt) { ... }On equal Smaller
On the unrun E2EFlagging it this loudly is the right thing to have done, and placing it outside Everything else in here is careful work; the crypto and the fail-closed discipline in particular are well above the bar. It's the multi-device key establishment that needs another pass. |
realmeylisdev
left a comment
There was a problem hiding this comment.
ran this on iOS against relay.divine.video, and got your two-device E2E running against local_stack. three things I think need fixing before it ships, details inline: publishEvent never reads the relay's OK, so a rejected publish gets recorded as applied and never retried. the vault key lookup reads "relay hasn't flushed the write yet" as "no key exists" and mints a second key over the first. and the new delete-retry loop treats an unreadable local library as an empty one, which tombstones the whole library.
rest are nits. fwiw the guards you say are pinned really are, I mutated five of them and they all went red.
| throw SyncIndexException('signer refused to sign ${ref.dTag}'); | ||
| } | ||
|
|
||
| final result = await _client.publishEvent(signed); |
There was a problem hiding this comment.
blocking. publishEvent reports success as soon as the frame is written to a socket, not when the relay accepts the event. RelayPool._sendCollect says it outright:
An entry in the returned list means the relay's WebSocket accepted the frame (or it was queued for pending-auth delivery). It does NOT mean the relay accepted the event at the protocol level, use
sendEventAwaitOkfor that guarantee.
Nostr.sendEvent returns the event whenever _pool.send is true, and publishEvent maps that to PublishSuccess. So every OK false reads as success here, and the caller records applied[dTag] = (stamped, hash). That is exactly the state that makes reconcile()'s push loop skip the item forever, so the retry this PR is built around never fires.
Rejections that reach this path today: a NIP-86 disallowkind, created_at more than 60s ahead (relay.rs:1340, max_created_at_drift_seconds, which SyncClock can produce from a fast clock or from clamping to latestKnownRemote + 1), content over 100KB, and rate limiting.
Tombstones are worse. publishLocalDeletion records tombstoneHash, and the delete-retry loop skips tombstoneHash entries by design, so a rejected deletion is never retried and the sound stays alive on every other device.
Measured: three creator-sync shaped kind-30078 events sent to local_stack came back ["OK",<id>,false,"blocked: kind 30078 not in allowed list"] and a readback returned zero events. That exchange is PublishSuccess on this code path.
publishEventAwaitOk gated on acceptedByAny is the fix. Your own harness already knows this one: sync_e2e_harness.dart:100 polls until the vault key is queryable, with the comment "publishEvent, which does not await relay OK".
There was a problem hiding this comment.
Fixed in a2f3b53. Publish goes through publishEventAwaitOk gated on acceptedByAny, so an OK false no longer gets recorded as applied. Test pins the every-relay-rejects case and goes red without the guard.
| .toList() | ||
| ..sort((a, b) => b.createdAt.compareTo(a.createdAt)); | ||
|
|
||
| return matches.isEmpty ? null : matches.first; |
There was a problem hiding this comment.
blocking. The fail-closed handling above covers noRelays and timedOut, but not the case that actually happens: a relay that answers promptly and honestly with nothing because it has not committed the write yet. Relay::query goes straight to storage.query_events_with_limit with no in-flight batch overlay, and production runs BATCH_INTERVAL_MS=180000 (divine-iac-coreconfig funnelcake-relay deployment.yaml:80).
Returning null here sends obtain() into _generateAndPublish, which is the outcome the doc comment above says must never happen. Same d tag, so the second key replaces the first. Device A keeps its cached key forever since obtain() short-circuits on the cache and never rechecks, and from then on every record either device publishes is undecryptable to the other. fetch swallows that as "skipping undecryptable sync record", so both devices sit at "Sounds up to date" with nothing syncing, permanently, and no recovery path.
This is not gated on opening the Sounds tab. main.dart reads soundSyncRepositoryStreamProvider, identityStreamOf uses container.listen, so obtain() runs at cold start. On my iOS run the vault key event's created_at lands at app launch, about two minutes before I first opened the tab. A second device only has to be launched inside the window.
Repro is in your own E2E: a sound saved on device A appears on device B fails Expected: <1> Actual: <0> as written, and passes with 8s inserted between the two reconciles.
There was a problem hiding this comment.
Already fixed in a87b936 before this landed — _adoptSettled reads the key back off the relays and adopts whichever one they retained, caching nothing until then. Same fix shape, arrived at independently. Nothing further from me.
| for (final entry in applied.entries) { | ||
| if (entry.value.bodyHash == SyncItemState.tombstoneHash) continue; | ||
| final ref = SyncItemRef.tryParse(entry.key); | ||
| if (ref == null || localSounds.containsKey(ref.id)) continue; |
There was a problem hiding this comment.
blocking. This infers "the user deleted this sound" from "it is not in readAll()", but SavedSoundsService.loadSavedSounds() returns [] for at least three reasons that are not a deletion:
jsonDecodethrows, swallowed by the outercatch (_)(saved_sounds_service.dart:74)- the payload was written by a newer build,
_isNewerSchemareturns[](:146) - the key is absent
Any one of them makes every entry in applied look locally deleted in a single pass, so this loop publishes a tombstone for the whole library and the user's other devices apply them as genuine deletions via _local.remove.
The newer-build case is the pointed one. _writeSavedSounds deliberately throws Refusing to overwrite a saved sound library written by a newer build so a v1 client cannot flatten a v2 library locally. This loop routes around that protection by deleting it remotely instead.
Worth having LocalSoundStore.readAll() distinguish "empty" from "could not read", or at minimum skipping the retry pass when localSounds is empty and applied is not. Leaving the shape to you since it changes the port contract.
There was a problem hiding this comment.
Fixed in 0514ae5. SavedSoundsService gained isLibraryUnreadable, the adapter throws LocalStoreUnreadableException instead of reporting an empty library, and LocalSoundStore.readAll documents the contract. Tests cover corrupt JSON, a newer-build payload and a versioned map with no sound list, plus the two readable-but-empty shapes that must still tombstone normally.
| final String id; | ||
|
|
||
| /// The `d` tag value for this item's addressable event. | ||
| String get dTag => '$prefix${kind.name}:$id'; |
There was a problem hiding this comment.
should-fix, and probably your call since it is a wire-format decision. The body is encrypted, the d tag is not, and it carries the full sound id. Anyone can send REQ {"kinds":[30078],"authors":["<pubkey>"]} unauthenticated and read back exactly which sounds an account saved, how many, and when from created_at. I confirmed the unauthenticated read works against relay.divine.video (it returned the divine:sync:vault-key event for my own pubkey).
For a feature whose premise is an encrypted index that is a real leak, and it widens in slices 2 and 3 when clip and draft ids go the same way. An HMAC of the id under the vault key would keep addressability and replaceability while making the tag opaque.
There was a problem hiding this comment.
Left for you, as flagged — the remedy is a wire-format change with a migration cost for anything already published, which is your call rather than something to push into the branch.
| /// | ||
| /// Matching is allowlist-based rather than prefix-based. The | ||
| /// subscription that feeds this also receives foreign kind-30078 events | ||
| /// — notably `dm_repository`'s read-state cursors and this package's own |
There was a problem hiding this comment.
nit, the code is right but the reason given is not. dm_repository's read cursors can never reach this subscription: publishSelfApplicationMarker builds the kind-30078 marker as a rumor and publishes it inside a NIP-59 gift wrap (nip17_message_service.dart:756, dm_repository.dart:1807), so on the relay it is a kind-1059 event and Filter(kinds: [30078]) cannot return it. Allowlist parsing is still correct and still needed, just for the vault-key event, which is a real bare 30078 sharing the prefix. Same claim is in the PR description.
There was a problem hiding this comment.
Fixed in 7488aa9, comment only. Both sites now name the vault-key event and other apps' app-specific data instead of DM read cursors, which cannot reach a kind-30078 filter.
| super(const SavedSoundsState()) { | ||
| on<SavedSoundsEvent>(_onEvent, transformer: sequential()); | ||
| _syncRepositorySubscription = syncRepositoryStream.listen( | ||
| (repository) => add(SavedSoundSyncRepositoryChanged(repository)), |
There was a problem hiding this comment.
should-fix. close() calls super.close() first, which closes _eventController synchronously, and only cancels this subscription a microtask later. Anything the stream delivers in that window hits add() on a closed bloc. In bloc 9.2.1 add() calls _eventController.add, which throws StateError, passes it to onError, and then rethrows. A throw out of an onData handler goes to the zone's uncaught handler rather than this subscription's onError, so it surfaces through main.dart's runZonedGuarded.
Account switch is where both sides fire together: the provider re-resolves and re-emits while SavedSoundsScope re-keys and closes the old bloc. _probe() a few methods down already guards with if (!isClosed), the same guard works here.
There was a problem hiding this comment.
Withdrawn — I could not reproduce it. close() awaits the subscription cancel immediately after super.close() flips isClosed, and the cancel drops queued deliveries before the listener runs. Probed a single-subscription controller, a sync broadcast controller, and the Stream.multi-over-broadcast shape identityStreamOf actually builds, including closing right after construction so the seed was still in flight: the listener never once ran with isClosed true. The code is fine as written.
| } | ||
|
|
||
| await _mirror( | ||
| () => _syncRepository!.publishLocalDeletion(event.soundId), |
There was a problem hiding this comment.
should-fix. The local delete already succeeded above, but the completer and the emit that actually removes the row both wait on this relay round trip. RelayPool.perRelaySendTimeout is 5s per relay and the fan-out is sequential, and publishEvent calls retryDisconnectedRelays() first when the pool is empty, so on a degraded network the deleted sound stays on screen and the caller's future stays pending for seconds. _save has the same shape, the save callback waits on the publish before it resolves.
_mirror swallows everything, so this can never fail the operation. For work its own doc calls best-effort it should run after the emit, not in front of it.
There was a problem hiding this comment.
Fixed in 4b61220. Both paths complete the completer and emit off the local write, then mirror. Two tests hold a publish open and assert the save/removal has already landed.
|
|
||
| /// local_stack's relay, reached the same way every other | ||
| /// `mobile/integration_test/` suite reaches it. | ||
| const String _relayUrl = 'ws://$localHost:$localRelayPort'; |
There was a problem hiding this comment.
nit, but it means the run instructions in the test header do not work as written. localHost is 10.0.2.2, the Android emulator host alias, so flutter test integration_test/creator_sync/... on macOS never reaches local_stack.
Two more things I hit getting it to run, so nobody has to rediscover them. local_stack's relay image predates the 30000-39999 auto-allow in relay.rs:1506, so it answers every kind-30078 publish with blocked: kind 30078 not in allowed list. Unblock with INSERT INTO nostr.allowed_kinds (kind) SELECT 30078 WHERE 30078 NOT IN (SELECT kind FROM nostr.allowed_kinds) and restart local_stack-funnelcake-relay-1.
With the host pointed at 127.0.0.1 and the kind allowed, the suite runs: 2 pass, 2 fail. The failures are the propagation issue in my vault_key_service comment. Also note a deletion on A removes the sound on B and stays deleted currently passes vacuously, B never received the sound so readAll() is empty either way.
There was a problem hiding this comment.
Fixed in c1cef2b, header comment only — it now says the documented command reaches local_stack only from an Android emulator, and why (localHost is hardcoded to the emulator-only 10.0.2.2 alias).
| 'soundsRemoveFailed', | ||
| // Cross-device sound library sync status copy; translation is deferred to | ||
| // the next sound-library localization pass. | ||
| 'soundSyncStatusSyncing', |
There was a problem hiding this comment.
nit. These four are user-visible banner copy, so they are better translated into the 16 locales than parked in the debt list. hm21 pushed back on exactly this for new strings on #5168.
There was a problem hiding this comment.
Fixed in 39fca60. All four keys translated into the 21 non-English locales and removed from _knownUntranslatedDebt; lib/l10n/generated regenerated and the consistency test passes.
e0aa2ee to
e74f55c
Compare
|
Rebased onto Rebase — 29 commits, three conflicts
Vault-key fork — confirmed, fixed in
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
realmeylisdev
left a comment
There was a problem hiding this comment.
Pushed six commits, one per finding, all checks green. Revert any of them freely.
a2f3b53— publish viapublishEventAwaitOkgated onacceptedByAny, so a relay'sOK falsestops being recorded as applied and silently never retried.0514ae5—readAll()fails closed on a saved-sound payload it cannot decode, instead of reporting an empty library that the delete-retry pass then tombstones account-wide.4b61220—_save/_removecomplete and emit off the local write, then mirror._editalready had this shape.7488aa9,c1cef2b— comment corrections (the kind-30078 allowlist, and why the E2E run command needs an emulator).39fca60— the foursoundSyncStatus*keys translated into all 21 locales, out of the debt list.
Two things worth reading rather than taking from the list.
I withdrew one finding. My note on saved_sounds_bloc.dart:42 — add() reaching a closed bloc from the stream listener — does not reproduce. close() awaits the subscription cancel immediately after super.close() flips isClosed, and that cancel drops queued deliveries before the listener runs. I probed a single-subscription controller, a sync broadcast controller, and the Stream.multi-over-broadcast shape identityStreamOf actually builds, closing right after construction so the seed was still in flight. The listener never ran with isClosed true. No guard needed; the code is right as written.
One is still yours: the plaintext d tag. Changing it is a wire-format decision with a migration cost for anything already published, so it stays with you.
Your vault-key fix landed before my review did, and it's the right shape — _adoptSettled caching nothing until the relays serve the key back closes the fork I was worried about, and the NIP-01 tie-break removes the ordering nondeterminism underneath it. I read both closely rather than assuming.
Verification: creator_sync 141 tests, 325/325 lines; flutter analyze lib test integration_test clean; the sound, sync, library-widget and l10n suites pass locally. Each new guard was checked by breaking it first — the publish-acceptance check, the unreadable-library check and both mirror-ordering pins all go red when reverted.
One note on CI while you're reading shard results: Tests (shard 0/4) failed twice on this branch before these commits, on ClipThumbnailImage renders a custom placeholder for a missing file and once also on an UploadManager count assertion. Both pass standalone and both cleared on a re-run with a fresh ordering seed — CI runs --test-randomize-ordering-seed random, so these are merged-isolate pollution surfacing seed by seed. The ClipThumbnailImage one also failed on main's own latest run, so it isn't from this branch either way.
c1cef2b to
f3b295c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Scaffolds the creator_sync package that will host cross-device sound library sync over encrypted Nostr app-data events. SyncClock exists because a device with a skewed clock could otherwise publish a created_at far in the future and permanently win every last-write-wins comparison; clamping to max(now, latestRemote + 1) lets a correcting device reclaim the lead. Dropped the bare `library;` directive from the barrel file that the task brief specified verbatim: very_good_analysis's unnecessary_library_directive lint flags an undocumented library directive as an analyze error, and other barrel files in this repo (app_update_repository, analytics, etc.) already omit it, so removing it keeps `flutter analyze` clean without deviating from house style.
Index payloads need bulk encryption that doesn't round-trip through a remote signer (NIP-46 bunker, NIP-55 Amber) per item; SyncCipher seals and opens payloads under a per-account vault key with a random nonce. Added exceptions.dart now (SyncDecryptException plus the two typed exceptions the vault key and index client tasks will need) so later tasks in this slice share one exception module instead of each adding their own. Coverage: added exceptions_test.dart and an extra sync_cipher_test.dart case (authenticated-but-non-UTF8 payload) beyond the brief's test file to reach the package's 100% min_coverage gate — the brief's tests alone left the exception toString() paths and the utf8.decode FormatException branch uncovered.
The vault key event is replaceable, so publishing a second one silently orphans every already-synced item as undecryptable ciphertext with no recovery path. VaultKeyService.obtain() therefore only generates a key after a confirmed-empty relay query, and throws VaultKeyUnavailableException on any ambiguous outcome (relay error, signer refusal, empty unwrap/wrap) instead of falling back to generation.
queryEvents collapses a failed relay lookup (timeout, no relays connected) to an empty list with no way to tell it apart from "confirmed empty", so obtain() could generate and publish a second vault key over a real one it never actually saw. Switch _fetchRemote to queryEventsDetailed and fail closed on noRelays/timedOut, and stop trusting the relay's filter compliance by re-checking kind and d tag on every returned event. Also: decode and validate a candidate key before writing it to the local cache instead of after, so a malformed unwrap can no longer poison the cache into throwing forever; report signer/relay failures by exception type instead of interpolating the exception itself, since a NIP-46/NIP-55 signer that echoes request params in its error would otherwise leak the plaintext vault key into logs; and memoize in-flight obtain() calls so two callers racing on a cold cache can't each independently decide no key exists and publish distinct replacements.
The two never-fork guard tests only stubbed queryEventsDetailed, so deleting the noRelays||timedOut throw still passed both: execution fell through to _generateAndPublish, hit the unstubbed nip44Encrypt, and mocktail's MissingStubError got caught by the generic catch and rethrown as VaultKeyUnavailableException — every assertion held while the invariant was broken. Add verifyNever(nip44Encrypt) to both so they fail when the guard is removed, and add two more tests: one pinning that a bad cached key's exception message never contains the key material (the one _decodeKey call site without that assertion), and one pinning that a failed obtain() clears its in-flight future so a later call re-queries instead of replaying the same rejection. Verified by deleting the noRelays||timedOut throw and re-running: both guard tests failed with "Unexpected calls: _MockSigner.nip44Encrypt(...)" showing a freshly generated key handed to the signer. Restored the guard and confirmed both pass again with no diff left behind in the implementation file.
Implements SyncItemRef value type for formatting and parsing Nostr d tags identifying synced items (sounds, clips, drafts). Parsing is allowlist-based to prevent misinterpreting foreign kind-30078 events (DM cursors, vault-key events) as sync items, which would corrupt unrelated state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds tests validating the hashCode and toString contracts on SyncItemRef: - hashCode: equal refs produce equal hashes; different kinds/ids don't collide - toString: contains full untruncated dTag; never truncates Nostr IDs Achieves 100% line coverage (18/18 lines). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
const SyncItemRef literals with identical field values compile to the same object, so the hashCode equality test was comparing an instance to itself and would still pass with hashCode deleted or replaced with identityHashCode(this). Build refs through a runtime helper so equal refs are genuinely distinct objects, add identical() guards so canonicalized literals fail loudly if reintroduced, and drop the redundant toString length assertion already covered by contains(). Mutation-tested: commenting out the hashCode override turns the 'equal refs produce equal hashes' test red as expected; restored with no diff on the production file.
SyncIndexEntry is the decrypted content of a kind-30078 sync event: a versioned envelope carrying either a live item body or a deletion tombstone. Task 6 will AES-GCM seal toPayloadJson() into the event's content; Task 8's reconciler reads `deleted` to drive local deletes.
…g relay flakes A publish failure mid-reconcile discarded the pull loop's applied-state progress even though the local upsert/remove had already landed on disk, so the next reconcile treated the pulled item as unseen and re-applied the stale remote body over whatever the user had edited locally in the meantime. Commit applied state after the pull loop, before the push loop runs, so pull progress survives a later push failure. Also stop reporting SyncIndexException (relay/network failures) to Crashlytics across all three public methods — expected transient failures per the decision matrix in error_handling.md — while still reporting genuine programming-invariant failures via the existing generic catch. Scope latestKnownRemote to the specific item's prior createdAt instead of the max across the whole per-kind map, closing a window where publishing many new items in one pass could stamp later items up to N seconds into the future and lose LWW against a genuine concurrent edit. Add a regression test for each, plus tests pinning the pulled item's recorded body hash and created_at, a tombstoned sound that gets re-added, and the publishLocalChange no-op path.
publishLocalChange and publishLocalDeletion already scoped latestKnownRemote to the item's own prior createdAt, but no test asserted the captured value, so reverting either to null (losing LWW protection for a lagging-clock device's edit or deletion) passed all tests. Add captureAny-based assertions for both the null (no prior entry) and prior-createdAt cases, mirroring the existing push-loop tests. Also widen the reconcile() SyncIndexException comment: the exception covers expected auth/signer failures (signed out, signer refused to sign), not only relay/network failures, so the comment was narrower than the code it describes.
Adds the app-layer adapters (SecureVaultKeyCache, PrefsSyncStateStore, SavedSoundsLocalStore), SoundSyncCubit/SoundSyncState, and the soundSyncRepositoryProvider that connect the creator_sync package to Divine's saved-sound library. The provider follows the nullable-gate pattern (returns null until the vault key resolves) so no consumer can capture a half-initialised dependency across auth transitions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ale refs Addresses code review on the app-layer sync wiring: - SoundSyncCubit: an uncaught error from reconcile() (e.g. a malformed remote body raising FormatException) previously left status stuck at `syncing` forever, since only the two expected exception types were caught. Add a catch-all that reports via Reportable and always reaches a terminal status, plus isClosed guards before every emit so closing the cubit mid-pass (e.g. navigating away) can't throw. - soundSyncRepositoryProvider: use the app's already-configured flutterSecureStorageProvider (AndroidOptions.encryptedSharedPreferences + appMacOsSecureStorageOptions) instead of a bare FlutterSecureStorage(), which reproduced #5563 on macOS debug builds. Hoist every ref.watch above the await so a sign-out/account-switch mid-obtain() can't throw UnmountedRefException, and so dependencies are registered even on the vault-key-unavailable null path. - Add soundSyncRepositoryProvider tests covering all four gate branches. - SavedSoundsLocalStore.upsert now asserts the id argument agrees with the body's own encoded id, since replaceSavedSound keys off the body. - Correct the PrefsSyncStateStore doc comment: discarding a malformed cursor is not "recoverable" — it makes every remote record look unseen, so remote unconditionally overwrites local edits and can resurrect locally-deleted sounds. Document the real tradeoff instead of asserting safety. - Strengthen the local-store round-trip test to full map equality (matching what the reconciler's hash comparison actually depends on) and add @description metadata to the four new ARB keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tasks 1-9 built a complete sound-sync engine that nothing called; this wires it into the real save/edit/remove/enrich paths in SavedSoundsBloc (mirroring every persist site to the user's other devices) and runs one reconcile pass when the Sounds tab opens, surfacing status via the existing l10n keys. Sync is best-effort and nullable-gated end to end: a relay outage or an unresolved vault key never fails the local save. Also fixes an unasserted finding in SoundSyncCubit's close-mid-flight test: it checked the future completed but not that the isClosed guard actually suppressed the spurious Reportable<StateError> a closed-cubit emit would otherwise produce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review response, ordered as given: - Critical: SavedSoundsScope's BlocProvider sat above MaterialApp.router, so keying it on the resolved sync repository (which flips from null to non-null a few seconds into every signed-in cold launch) re-inflated the whole app shell (#6477/#6480 hazard). SavedSoundsBloc now takes a syncRepositoryStream and re-points its mutable _syncRepository field in place via a bloc event, matching PeopleListsBloc's established shape; the BlocProvider key reverts to the account storage key alone. - Critical: swapped the raw TextStyle the sync banner introduced for VineTheme.labelSmallFont, restoring the frozen ceiling ratchet. - Important: _mirror's catch set only named the two expected sync exceptions. Anything else escaped _save (false "save failed" to the user despite a successful local write) and _remove (removeSound() never completing, hanging the caller). Added the same catch-all + Reportable shape SoundSyncCubit already uses. - Important: SavedSoundsBloc was not the only path that persists to My Sounds — VideoEventPublisher's imported-audio and extracted-audio flows call SavedSoundsService.saveSound directly. Mirrored both, best-effort, via a new nullable SoundSyncRepository dependency, rather than leaving an undocumented instant-vs-lazy asymmetry. - Important: added a widget test overriding soundSyncRepositoryProvider to prove the reconcile trigger actually fires and the status banner renders all four states, plus a saved_sounds_scope_test case pinning that a sync-repository stream emission re-points the bloc without remounting it. - Minor: corrected creator_sync_provider.dart's doc comment, which had claimed no ValueKey guard is ever needed. Every new regression test was mutation-tested against the described bug and reverted to a byte-identical diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lisher Re-review response. Important — videoEventPublisherProvider (keepAlive) watched the resolved sync repository directly, so it rebuilt every time the vault key resolved — strictly later than the auth-readiness transitions it already rebuilds on. A fresh instance starts with an empty _inFlightDirectPublishes, the #6018 duplicate-publish coalescer, so two publishDirectUpload calls straddling that rebuild could each sign and broadcast the same addressable event. VideoEventPublisher now takes SoundSyncRepository? Function() soundSyncRepositoryGetter and reads the current value via ref.read inside the closure instead of ref.watch in the provider body, so the getter still reflects the live repository without adding a rebuild trigger. Added test/providers/video_providers_sync_repository_test.dart, overriding every videoEventPublisherProvider dependency to prove the same instance survives a sync-repository resolution; mutation-tested by reintroducing the watch — the test caught it. Minor — strengthened "a sync failure does not fail the video publish" to verify publishLocalChange was actually called. The mirror sits inside an enclosing catch-and-log, so neither deleting _mirrorSavedSound's own try/catch nor deleting the mirror call would have reddened it before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Proves the sound-sync slice end to end: two SoundSyncRepository instances sharing one signer stand in for two devices on one account, driving reconcile/publishLocalChange/publishLocalDeletion against a real local_stack relay with real NIP-44 vault-key wrapping. Lives outside integration_test/e2e/ and carries no `service` tag, so CI only format/analyzes it — Docker was unresponsive in this environment, so the test itself has never been executed. See the file header for the required manual run steps.
The new creator_sync CI workflow wasn't recorded in package_coverage_floors.txt, so check_package_coverage_floor.sh failed as an unrecorded 100% floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_fetchRemote() filtered candidate vault-key events by kind and d tag but never checked the author, unlike its SyncIndexClient sibling. A relay returning a newer foreign-author decoy sharing the vault-key d tag would shadow the genuine event and fail obtain() closed. Mirrors the wrong-author regression test already covering SyncIndexClient. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
soundSyncRepositoryProvider collapsed "session not ready" and "vault key unavailable" to the same null value, so SoundsTab never created a SoundSyncCubit for a locked vault key and the spec-mandated locked state was unreachable — a NIP-46 bunker refusal or a relay outage at key time produced silence instead of the locked banner. Replace the nullable-gate provider with soundSyncAvailabilityProvider, a sealed SoundSyncAvailability (SoundSyncSessionNotReady / SoundSyncVaultLocked / SoundSyncAvailable) that distinguishes the two cases. SoundsTab now renders a locked banner directly from context.l10n.soundSyncStatusLocked when locked, without needing a cubit. soundSyncRepositoryValueProvider and soundSyncRepositoryStreamProvider keep their existing SoundSyncRepository? shape for the consumers that only need the repository. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… refactor The provider stopped returning null when SoundSyncAvailability landed; the header still claimed it did.
…lete Destructive account cleanup wiped the saved-sounds bucket but left the per-item applied cursor behind. A later reconcile would then read every previously-synced sound as newly deleted and republish tombstones for the whole library. Clear the creator-sync cursor for the leaving pubkey alongside the sounds bucket so the next reconcile treats it as a fresh pull from remote instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reconcile retried a failed add or edit publish (via the applied-hash drift check) but never retried a failed delete: a tombstoned sound is by definition absent from the local store, so the push loop — which only iterates local items — could never see it. After the push phase, scan `applied` for entries with a real body hash whose id is no longer local and republish their tombstone, tracked in a new `SoundSyncOutcome.deletionsRetried` field. Relies on the previous commit clearing the sync cursor on destructive cleanup, or this retry would misread a wiped-but-not-yet-reconciled library as a mass delete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rebase The rebase onto origin/main dropped consentSoundsRepository from videoEventPublisher — #6772 removed AudioReuseConsentResolver's soundsRepository parameter, so the local had no consumer left. That changed the provider body, so the riverpod source hash needed a build_runner pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both readers of an addressable event resolved "newest wins" on created_at alone: VaultKeyService._fetchRemote sorted by it, and SyncIndexClient.fetch compared it when deduping per d tag. On equal created_at the winner was therefore whatever order the relay happened to return, so two devices handed the same two events could pick different ones and stay disagreeing indefinitely — for the vault key that means each device seals items under a key the other cannot open. NIP-01 resolves same-timestamp collisions on replaceable events by retaining "the event with the lowest id (first in lexical order)". Both call sites now apply that, via one shared helper so the rule has a single definition. Both new tests assert the same answer for both relay orderings and were mutation-tested against the previous created_at-only comparison: each fails on one of the two orderings without the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_generateAndPublish cached the key it had just generated and never read it back. The in-flight future documented above obtain() only dedups callers inside one process; it says nothing about a second device. Two devices with cold caches can each query, each get a genuine relay-confirmed "no vault key exists", and each publish a kind-30078 at d-tag divine:sync:vault-key. Addressable-event semantics mean the relays retain exactly one of them, but both devices had already cached their own — and the loser never finds out, because every later obtain() short-circuits on its cache. It goes on sealing sounds under a key no other device can open, permanently. That is the same failure the file's own doc warns about, reached device-to-device rather than callsite-to-callsite. Installing on a second device the same day is ordinary. The narrower single-device variant also existed: publishEvent reports success once the pool accepted the frame, not once a relay stored the event, so the cached key could correspond to an event that never landed. _generateAndPublish now ends in _adoptSettled, which re-reads the vault-key event and adopts whichever one the relays actually retained, caching that. Nothing is cached until a relay has served it back, so both racing devices converge before either seals an item. Read-back failure — relays stayed empty, went unreachable, or returned an event this signer cannot unwrap — throws VaultKeyUnavailableException and caches nothing. That is safe precisely because nothing was cached: no item was sealed under the discarded key, so the next obtain() finds the published event and adopts it, or mints again, orphaning nothing. The app layer already maps this exception to SoundSyncVaultLocked, so it surfaces as the existing locked banner rather than a new failure mode. Relays acknowledge from an in-memory queue and commit afterwards, so the read-back is retried up to settleAttempts times settleInterval apart (20 x 250ms = 5s, the budget the two-device E2E harness already found sufficient). The delay is injected rather than hard-called so tests drive the loop without real waits. The convergence test was mutation-tested against the previous cache-the-minted-key behavior and fails without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Unqualified [publishEvent] tripped comment_references in the package's own analyze pass, which the app-level `flutter analyze lib test integration_test` does not cover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ording it publishEvent resolves once a relay's socket took the frame, so an OK false — a kind the relay's allowlist refuses, a clamped created_at, an oversized payload, a rate limit — was indistinguishable from acceptance. The reconciler records applied state off that result and then skips the retry while the body hash matches, so an item the relay refused was never published again. Rejected tombstones fare worse: the delete-retry loop skips entries already carrying tombstoneHash by design. Route through publishEventAwaitOk and gate on acceptedByAny, which is what that API's own docs prescribe for callers recording durable state. The vault-key publish keeps publishEvent: _adoptSettled reads the event back off the relays before caching anything, which is a stronger check than the OK frame.
SavedSoundsService.loadSavedSounds() answers [] for a library it cannot decode — unparseable JSON, a payload from a newer build, a versioned map with no sound list — exactly as it does for one that is genuinely empty. The adapter passed that straight through as an empty map, and reconcile's delete-retry pass reads "in applied state, absent locally" as "the user deleted it": one unreadable read publishes a tombstone for every sound the account has, and every other device applies them via _local.remove. The newer-build case is the sharp one. _writeSavedSounds already refuses to flatten a newer-schema library locally; sync routed around that guard by deleting it remotely instead. Give the service an isLibraryUnreadable getter that separates "present but undecodable" from "empty", have the adapter throw the new LocalStoreUnreadableException instead of reporting an empty library, and document the contract on LocalSoundStore.readAll so the next implementation cannot reintroduce it. The exception is an expected domain failure, not a Reportable one: it is what a downgrade looks like. reconcile, publishLocalChange, the sync cubit and the bloc's mirror path all catch it above their generic handlers, so it surfaces as a sync failure status rather than reaching Crashlytics.
_save and _remove awaited _mirror before completing the caller's completer and emitting. _mirror is best-effort by construction — it catches every error and reports through addError, so it can never fail the operation — but awaiting it still held the UI: RelayPool.perRelaySendTimeout is 5s per relay, the fan-out is sequential, and publishEvent retries disconnected relays first when the pool is empty. On a degraded network a saved sound took that long to appear, and a deleted one stayed on screen for it. Emit and complete off the local write, which has already succeeded, then mirror. _edit already had this shape.
Both comments name dm_repository's read cursors as the foreign events the allowlist rejects. They can never appear: publishSelfApplicationMarker builds that marker as a kind-30078 *rumor* and publishes it inside a NIP-59 gift wrap, so what reaches a relay is kind 1059 and never matches a kind-30078 filter. The allowlist is still right, and still necessary — the query cannot filter on d, so it returns every kind-30078 event the account has published, including this package's own vault-key event and anything another app stores under the shared app-specific-data kind. Name those instead.
The four soundSyncStatus* keys shipped English-only, parked in _knownUntranslatedDebt. They are user-visible banner copy on the Sounds tab, so a non-English user saw an untranslated string in the middle of an otherwise translated screen. Translated into all 21 non-English locales and dropped from the debt list. Regenerated lib/l10n/generated.
…ator The header's `flutter test integration_test/creator_sync/...` only reaches local_stack from an Android emulator. The relay URL is built from `localHost`, which environment_config.dart hardcodes to '10.0.2.2' with no platform branch — an emulator-only alias for the host. Run from macOS it routes out the default gateway and the harness just times out connecting, with nothing pointing at the cause.
`localHost` is a getter that picks the right loopback alias per platform, so a top-level `const` could not hold the interpolation and Analyze failed on it. A getter keeps the per-platform resolution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0d5f265 to
b30b702
Compare
Mobile PR PreviewPreview refreshed for Last refresh:
|
Closes #6857
What
Slice 1 of 3 of creator sync: a user's saved sound library now mirrors across every device signed into the same Nostr account.
Metadata-only — no blob layer, no server dependency, free for all accounts. Slices 2 (clips) and 3 (drafts) add the media layer and depend on server work tracked separately.
New pure-Dart package
mobile/packages/creator_sync:kind-30078event. Unwrapped once per device, so bulk crypto never touches the signer (a NIP-46 bunker user would otherwise face one round-trip per synced item).kind-30078event per sound. Replaceable-event semantics supply last-write-wins for free.Plus app-layer adapters, a status cubit, l10n, Riverpod wiring, and the triggers in
SavedSoundsBloc/SoundsTab/VideoEventPublisher.Design decisions worth reviewing
Fails closed on every relay read that gates a write. Both reads use
queryEventsDetailedand rejectnoRelays/timedOutexplicitly.queryEventsdiscards those flags and returns[], so an unreachable relay is indistinguishable from "nothing exists" — in the vault-key lookup that would have generated a second key, replaced the first, and permanently orphaned the user's whole library. The vault-key read also passesuseCache: falseso "empty" is a statement about the relay, not a local cache hit.Relay events are untrusted. Kind, pubkey, and an allowlisted
dtag are all re-checked before the newest-wins dedup. Without the pubkey check a poisoned event with a newercreated_atwins dedup, fails to decrypt, gets skipped — and silently erases a genuine sound whilefetchreports success.d-tag parsing is allowlist-based, not prefix-based. Relay filters can't prefix-matchdtags, so this subscription also receivesdm_repository's read cursors and this package's own vault-key event. Mis-parsing either would corrupt unrelated state.Pull commits before push.
_local.upsertpersists immediately but the cursor was previously committed only after the push loop — so a mid-push relay failure discarded pull progress, and the next pass re-applied the stale relay body over the user's edit. Pinned by a regression test reproducing that exact symptom.App-shell bloc is re-pointed, not re-keyed.
SavedSoundsScopesits aboveMaterialApp.router; adding the async-resolving repository to its key would remount the whole app seconds into every cold launch (#6477/#6480). UsesidentityStreamOf, matching thePeopleListsBlocprecedent from #6482.Crashlytics discipline. Relay, auth, and signer failures are expected here and reach
addErrorunwrapped; only genuine programming errors areReportable. Pure-Dart package uses the reporter-port typedef.Testing
creator_sync: 127 tests, 100% coverage (287/287 lines, nocoverage:ignoreescapes). App layer: adapters, cubit, provider gates, bloc triggers, and widget tests for the sync banner.Guards are mutation-verified — deleted, watched go red, restored byte-identical. Several tests carry inline comments naming which assertion is load-bearing.
mobile/integration_test/creator_sync/sound_sync_two_device_test.dartis committed but was never run — the Docker daemon was unresponsive in the authoring environment, solocal_stackcould not start. It is deliberately placed outsideintegration_test/e2e/and untagged, so CI analyzes and format-checks it but does not run it.Do not read it as coverage. Someone with a working Docker should run it once:
Assumptions flagged in the file that are likely to need adjustment on first run:
localHostis10.0.2.2(Android-emulator-specific; plainflutter teston macOS probably needslocalhost), and the vault-key settlement poll interval is an untested guess.Known follow-ups (not blocking)
sync_cipher's length guard isn't pinned at the exact 27-vs-28 boundary; an off-by-one would surface anArgumentErrorthatfetchdoesn't catch.schemaVersionuses strict equality, so a future v2 client and a v1 client would ping-pong. Zero effect today._canonicalsorts keys viatoStringbut indexes with the stringified key — unreachable for JSON-derived bodies, one-line fix.package_coverage_floors.txthas a pre-existing sort inversion (image_metadata_stripperbeforeiap_repository) that will cause a spurious re-sort on the nextUPDATE_BASELINE=1. Already onmain; worth a separate one-liner.Verification
dart format,flutter analyze lib test integration_test,package_coverage_floor,untested_services_floor,raw_textstyle_ceiling,test_unit_structure,shared_channel_overrides,integration_test_error_restore_safety, andarb_consistency_testall pass — re-run after rebasing onto currentorigin/main.🤖 Generated with Claude Code