feat: Phase 16 — conflict detection via optimistic concurrency#253
Merged
Conversation
Phase 16: Advanced Sync (rescoped to conflict detection only) - Implementation decisions documented - Phase boundary established - Offline queue + idempotent replay deferred to M3 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 989b5ebaf81f
- Note conflict detection dependency on API relay in BYO-IPFS todo - Rename resolved debug session (desktop-e2e-ci-all-platforms) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 71abf58abc0a
Phase 16: Advanced Sync (conflict detection) - Current IPNS publish architecture mapped across web + desktop - folder_ipns schema and sequence number management documented - All publish call sites identified (web: 10 paths, desktop: 6 paths) - Optimistic concurrency approach validated against existing infrastructure - Pitfalls catalogued: stale closures, batch conflicts, move half-conflicts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9cc7512ff842
Phase 16: Advanced Sync (Conflict Detection) - 3 plans in 2 waves - Wave 1: API optimistic concurrency (expectedSequenceNumber, 409 Conflict) - Wave 2: Web client + Desktop client conflict handling (parallel) - Ready for execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: bedcf8d98a42
Phase 16: Advanced Sync - Plan 16-04: Web E2E tests (Playwright) for conflict detection scenarios - Plan 16-05: Desktop E2E tests (bash/PowerShell) for FUSE conflict detection - Updated roadmap: 3 plans -> 5 plans Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d70c0a52cdaf
Address 2 blockers and 4 warnings from plan verification: - Plan 03: desktop retry now re-fetches remote metadata and merges local mutation instead of re-encrypting stale local metadata - Plan 02: add checkAndRotateIfNeeded conflict handling, split Task 2 into service layer (Task 2) and hooks/UI (Task 3), detail subfolder re-sync path - Plan 04: document expected delegated routing warning from dummy record - Plan 03: add write_ops.rs and all IpnsPublishRequest sites to files_modified Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4e2fde443210
…check Add optional expectedSequenceNumber field to PublishIpnsDto and PublishIpnsEntryDto. When present, upsertFolderIpns compares against stored sequence number and throws ConflictException (409) on mismatch. Batch publish re-throws ConflictException to fail entire batch if any folder record conflicts. API client regenerated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 94157609492b
- Stale expectedSequenceNumber rejected with 409 ConflictException - Matching expectedSequenceNumber accepted, returns incremented seq - Missing expectedSequenceNumber preserves backward compat (no check) - Batch rejects entirely when folder record has stale sequence - Batch succeeds when folder record has matching sequence Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f0b5acf46518
Tasks completed: 2/2 - Add expectedSequenceNumber to DTOs and conflict check to service - Unit tests for conflict detection SUMMARY: .planning/phases/16-advanced-sync/16-01-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f689cdc0242d
- Create apps/web/src/lib/errors.ts with isConflictError() function - Detects HTTP 409 from Orval custom-instance error shape (.status property) - getConflictSequenceNumber() provided as placeholder; callers re-sync via IPNS - API client already contains expectedSequenceNumber in PublishIpnsDto and PublishIpnsEntryDto from Plan 01 regeneration Entire-Checkpoint: 4afe66f3c73b
…PI client - Add PublishResult enum with Success and Conflict variants - Add optional expected_sequence_number field to IpnsPublishRequest - Update publish_ipns to return PublishResult, handling 409 Conflict responses - Existing callers will fail to compile until Task 2 updates all construction sites Entire-Checkpoint: f70e53f0da3b
…ell) - test-conflict-detection.sh: bash script with bump_server_sequence helper - test-conflict-detection.ps1: PowerShell port with Invoke-BumpServerSequence - Test 1: write file, bump server seq, write again -> both readable - Test 2: mkdir, bump server seq, write in dir -> dir and file accessible - Both scripts follow existing pass/fail counter and exit code patterns Entire-Checkpoint: 9f4553d54dd0
- conflict-helpers.ts: bumpServerSequence helper that increments server-side folder sequence via unconditional API publish (simulates another device) - conflict-detection.spec.ts: 3 E2E tests covering: 1. Upload file with stale sequence -> 409 -> re-sync -> retry -> file visible 2. Create folder with stale parent sequence -> 409 -> re-sync -> folder visible 3. Negative: per-file IPNS update does NOT trigger conflict detection - Tests follow existing serial describe pattern with page objects - Uses loginViaTestEndpoint (or loginViaEmail fallback) for auth - bumpServerSequence reads current CID from resolve, publishes without expectedSequenceNumber (unconditional) to increment DB sequence Entire-Checkpoint: d5bf2398c768
- run-all.sh: invoke test-conflict-detection.sh after Step 3 (API round-trip) - run-all.ps1: invoke test-conflict-detection.ps1 after Step 3 (API round-trip) - Failure count from conflict tests aggregated into TOTAL_FAIL - Existing Steps 1-3 unchanged Entire-Checkpoint: 03b08012064b
- ipns.service.ts: createAndPublishIpnsRecord and batchPublishIpnsRecords accept and forward expectedSequenceNumber - folder.service.ts: updateFolderMetadata passes sequenceNumber.toString() as expectedSequenceNumber (pre-increment value) - folder.service.ts: buildFolderIpnsRecord includes expectedSequenceNumber in folder batch record payload - folder.service.ts: checkAndRotateIfNeeded wraps updateFolderMetadata call with 409 handling; logs warning and returns new key on conflict, deferring metadata re-encryption to next sync - Per-file IPNS publishes do NOT include expectedSequenceNumber (last-write-wins) Entire-Checkpoint: afd2d27979a0
Tasks completed: 1/1 - Task 1: Create conflict helper utility and E2E test suite SUMMARY: .planning/phases/16-advanced-sync/16-04-SUMMARY.md Entire-Checkpoint: bfc04944970f
Tasks completed: 2/2 - Task 1: Create conflict detection test scripts (bash + PowerShell) - Task 2: Add conflict detection step to run-all orchestrators SUMMARY: .planning/phases/16-advanced-sync/16-05-SUMMARY.md Entire-Checkpoint: af8b782757af
…and retry - Add merge_folder_children() helper for additive conflict resolution - spawn_metadata_publish sends expected_sequence_number with folder publishes - On 409 Conflict: re-fetches remote metadata via IPNS resolve + IPFS fetch + decrypt, merges local changes onto remote children preserving both devices edits, adds jitter, re-encrypts merged metadata, retries once with fresh sequence number - On persistent conflict (retry also 409): logs error, does not loop infinitely - write_ops.rs: new folder publish uses None (seq 0), parent publish uses Some(seq) with conflict warning and debounce retry fallback - operations.rs: per-file IPNS publishes use None (no conflict detection for files) - commands/vault.rs: vault init uses None (seq 0, unconditional) - registry/mod.rs: device registry uses None (separate IPNS namespace) - Windows variants (windows/write_ops.rs, windows/operations.rs) updated identically Entire-Checkpoint: ec52ca93814c
Tasks completed: 2/2 - Task 1: Add PublishResult enum and expected_sequence_number to API client - Task 2: Handle conflicts in FUSE publish with re-fetch + re-apply + retry SUMMARY: .planning/phases/16-advanced-sync/16-03-SUMMARY.md Entire-Checkpoint: 640e66abac3c
…pdate STATE.md - 16-03-SUMMARY.md: Documents Rust implementation of optimistic concurrency control in FUSE publish layer (PublishResult enum, merge_folder_children, re-fetch+retry) - STATE.md: Add decisions for PublishResult enum, IPNS merge key, OS notification deferral Entire-Checkpoint: 18554fbade42
Tasks completed: 3/3 - Task 1: Add isConflictError utility for 409 IPNS conflict detection - Task 2: Wire expectedSequenceNumber through service layer - Task 3: Wire 409 conflict handling in hooks, store, and SyncIndicator SUMMARY: .planning/phases/16-advanced-sync/16-02-SUMMARY.md Entire-Checkpoint: 0d5eb37aeab6
Phase 16 verified: 3/3 must-haves confirmed against codebase. SYNC-04 requirement marked complete. Roadmap updated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 95c3190c95c9
- Wait for either file list grid OR empty state in beforeAll (fresh vaults show EmptyState, not FileList) - Upload a seed file before conflict tests to establish the root folder IPNS record (bumpServerSequence needs an existing record) - Extend upload zone page object to find file input in both .upload-zone and .empty-state dropzone containers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f31291f127ce
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds API optimistic concurrency for IPNS folder publishes via an optional expectedSequenceNumber (409 Conflict on mismatch); implements client and desktop conflict detection, resync + single-retry flows, UI conflict state, DTO/OpenAPI updates, unit tests, and E2E plans/scripts for verification. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant WebClient as Web Client
participant Store as Sync Store
participant API as API Server
participant IPFS as IPFS Network
User->>WebClient: Create/Update folder
WebClient->>API: Publish with expectedSequenceNumber
API->>API: Compare expectedSequenceNumber vs stored sequence
alt Sequences match
API-->>WebClient: 2xx Success
WebClient->>Store: clearConflict()
else Sequence mismatch (409)
API-->>WebClient: 409 Conflict (currentSequenceNumber)
WebClient->>Store: setConflict(...)
WebClient->>IPFS: resolveIpnsRecord (fetch remote CID)
IPFS-->>WebClient: Remote metadata CID
WebClient->>WebClient: Fetch & decrypt remote metadata, merge local changes
WebClient->>API: Retry publish with fresh expectedSequenceNumber
alt Retry success
API-->>WebClient: 2xx Success
WebClient->>Store: clearConflict()
else Retry 409
API-->>WebClient: 409 Conflict (persistent)
WebClient->>WebClient: surface persistent conflict error
end
end
sequenceDiagram
actor User
participant Desktop as Desktop Client (FUSE)
participant API as API Server
participant DB as Database
participant IPFS as IPFS Network
participant Keychain as Keychain
User->>Desktop: Write file / modify folder
Desktop->>API: publish_ipns with expected_sequence_number
API->>DB: Check folder sequence
alt Sequences match
DB-->>API: Success
API-->>Desktop: PublishResult::Success
Desktop->>Desktop: normal completion
else Sequence mismatch (Conflict)
DB-->>API: 409 Conflict
API-->>Desktop: PublishResult::Conflict { current_sequence_number }
Desktop->>API: resolve current IPNS to get CID
Desktop->>IPFS: Fetch & decrypt remote metadata
Desktop->>Desktop: merge_folder_children(local, remote)
Desktop->>Desktop: jitter delay
Desktop->>Keychain: re-encrypt merged metadata
Desktop->>IPFS: upload new content CID
Desktop->>API: retry publish_ipns with fresh expected_sequence_number
alt Retry success
API-->>Desktop: PublishResult::Success
Desktop->>Desktop: complete write
else Retry conflict
API-->>Desktop: PublishResult::Conflict
Desktop->>Desktop: log persistent conflict, stop retry
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #253 +/- ##
==========================================
+ Coverage 46.31% 47.94% +1.63%
==========================================
Files 106 106
Lines 8266 8639 +373
Branches 591 594 +3
==========================================
+ Hits 3828 4142 +314
- Misses 4271 4329 +58
- Partials 167 168 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src-tauri/src/fuse/write_ops.rs (1)
506-524:⚠️ Potential issue | 🟠 MajorSeparate success and conflict post-actions for parent publish.
On conflict, the code still falls through to Line 520-522/Line 524 success-tail behavior, and the freshly uploaded
parent_meta_cidis not cleaned up.🧹 Proposed fix
- match crate::api::ipns::publish_ipns(&api, &parent_req).await? { + match crate::api::ipns::publish_ipns(&api, &parent_req).await? { crate::api::ipns::PublishResult::Success => { coordinator.record_publish(&parent_ipns_name, new_seq); + if let Some(old) = parent_old_cid { + let _ = crate::api::ipfs::unpin_content(&api, &old).await; + } + log::info!("Parent metadata published after mkdir"); } crate::api::ipns::PublishResult::Conflict { current_sequence_number } => { log::warn!( "Conflict on parent publish after mkdir (expected seq {}, server has {}). \ Debounced publish will retry.", seq, current_sequence_number ); - // Do not record_publish -- let the next debounced publish pick up fresh seq + // Cleanup orphaned upload from this failed publish attempt. + let _ = crate::api::ipfs::unpin_content(&api, &parent_meta_cid).await; } } - - if let Some(old) = parent_old_cid { - let _ = crate::api::ipfs::unpin_content(&api, &old).await; - } - - log::info!("Parent metadata published after mkdir");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/fuse/write_ops.rs` around lines 506 - 524, The parent-publish conflict handling falls through to the success-tail cleanup/logging, leaving the newly uploaded parent_meta_cid pinned and emitting a misleading success log; modify the publish_ipns match so that PublishResult::Success performs coordinator.record_publish(&parent_ipns_name, new_seq), unpins parent_old_cid (calling crate::api::ipfs::unpin_content(&api, &old).await) and logs "Parent metadata published after mkdir", while PublishResult::Conflict explicitly unpins the freshly uploaded parent_meta_cid (call crate::api::ipfs::unpin_content(&api, &parent_meta_cid).await or equivalent) and logs or warns about conflict without running the success-tail actions; ensure the unpin of parent_old_cid is not executed for Conflict unless intended and keep references to publish_ipns, PublishResult::Success, PublishResult::Conflict, coordinator.record_publish, parent_old_cid, parent_meta_cid, and crate::api::ipfs::unpin_content to locate the changes.apps/api/src/ipns/ipns.service.ts (2)
175-205:⚠️ Potential issue | 🟠 MajorSequence compare and increment are not atomic.
At Line 175-Line 205, OCC check (
expectedSequenceNumber) and increment/save are separate operations. Two concurrent requests with the same expected sequence can both pass and overwrite each other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/ipns/ipns.service.ts` around lines 175 - 205, The OCC check in the update path (where getFolderIpns is used then folderIpnsRepository.save is called) is not atomic: convert the read-then-save pattern into an atomic conditional update (e.g., use a transaction with SELECT ... FOR UPDATE or perform a single DB UPDATE via folderIpnsRepository.createQueryBuilder().update(...) with a WHERE that includes userId, ipnsName and sequenceNumber = expectedSequenceNumber and uses sequenceNumber = sequenceNumber + 1 and sets other fields), then check affectedRows and throw the same ConflictException if no rows were updated; if the update succeeds, return the updated record (or re-load it) and avoid the non-atomic existing -> save flow.
96-144:⚠️ Potential issue | 🟠 MajorBatch 409 abort still commits partial side effects inside the chunk.
At Line 96-Line 132, chunk entries execute concurrently. If one conflicts and is rethrown at Line 142-Line 144, other entries in that same chunk may already be persisted/published. That breaks “fail entire batch” semantics for callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/ipns/ipns.service.ts` around lines 96 - 144, The chunk currently uses Promise.allSettled over batch.map which allows other entries to persist/publish while one later throws a ConflictException, causing partial commits; replace the concurrent Promise.allSettled approach with a sequential for-loop that processes each entry one-by-one (validate base64, call this.delegatedRouting.publish, call this.upsertFolderIpns) and on any thrown ConflictException immediately rethrow to abort the chunk (do not continue processing remaining entries), so that functions like upsertFolderIpns and delegatedRouting.publish are not executed for subsequent entries after a conflict; remove or adapt the settled/result handling logic accordingly.apps/web/src/hooks/useFolderMutations.ts (1)
461-492:⚠️ Potential issue | 🟠 MajorRe-run move validations after resync before retrying batch move.
Name-collision and depth checks are done once against pre-conflict state. After a 409 + resync, retry uses fresh folders but skips those validations, so concurrent changes can slip through and publish invalid destination state.
Also applies to: 500-511, 542-553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useFolderMutations.ts` around lines 461 - 492, The current pre-flight validations (involving batchNames, duplicate name checks against destFolder.children, and folder-specific checks using useFolderStore.getState().folders, folderService.isDescendantOf, folderService.getDepth, folderService.calculateSubtreeDepth and MAX_FOLDER_DEPTH) are only run once before the retry loop, so after a 409 + resync the retry path skips them; re-run the same validations against the freshly synced state immediately after resync and before any retry of the batch move (i.e., rebuild batchNames and re-check intra-batch duplicates, re-check nameExists against the current destFolder.children, and re-run the folder descendant and depth checks using the updated folders from useFolderStore.getState()) so concurrent changes cannot bypass collision/depth prevention.
🧹 Nitpick comments (2)
packages/api-client/openapi.json (1)
789-791: Add explicit 409 response body schemas for conflict endpoints.Both 409 responses (lines 789–791 for
/ipns/publishand 837–839 for/ipns/publish-batch) lack content schemas despite their descriptions referencingcurrentSequenceNumber. Addingcontent.application/json.schemawith a typed conflict response (includingcurrentSequenceNumber) will keep generated clients strongly typed and align with the pattern used for 200 responses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/openapi.json` around lines 789 - 791, The 409 responses for the endpoints /ipns/publish and /ipns/publish-batch are missing response body schemas; add a content.application/json.schema for the 409 response in each endpoint describing a typed conflict object that includes currentSequenceNumber (e.g., { type: "object", properties: { currentSequenceNumber: { type: "integer" } }, required: ["currentSequenceNumber"] }) so generated clients are strongly typed and consistent with the 200 responses; update the 409 entries for both endpoints accordingly.tests/e2e-desktop/scripts/test-conflict-detection.sh (1)
130-133: Replace fixed sleeps with condition-based waits to reduce E2E flakes.Using fixed
sleep 8/sleep 15makes this timing-sensitive test brittle under variable CI load.⏱️ Proposed refactor
+wait_for_content() { + local path="$1" + local expected="$2" + local timeout_s="${3:-30}" + local waited=0 + while [ "$waited" -lt "$timeout_s" ]; do + local got + got=$(cat "$path" 2>/dev/null || true) + if [ "$got" = "$expected" ]; then + return 0 + fi + sleep 1 + waited=$((waited + 1)) + done + return 1 +} ... -sleep 8 +sleep 1 # small settle ... -sleep 15 +wait_for_content "$MP/conflict-test-2.txt" "file-after-bump" 30 || trueAlso applies to: 140-143, 163-164, 172-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-desktop/scripts/test-conflict-detection.sh` around lines 130 - 133, Replace the fixed sleeps (e.g., the "sleep 8" after writing "conflict-test-1.txt" and other "sleep 15"/"sleep 8" uses) with condition-based polling: after writing or updating files like "conflict-test-1.txt" poll for the expected observable state (for example, the published FUSE mount reflecting the new file content, a specific file timestamp, or the presence/contents of a sync/publish marker) with a short interval and a timeout, rather than a fixed delay; update all occurrences (the sleep commands around the writes and bumps) to use the same wait-until function/loop that checks the actual condition and exits early when satisfied to avoid CI timing flakes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/fuse/mod.rs`:
- Around line 271-283: The loop over remote_by_ipns currently treats any
remote_child missing from local_by_ipns as an explicit local delete whenever
local.children is non-empty; instead, change the logic in the for (ipns,
remote_child) loop so that a missing local child is only treated as a delete
when there is an explicit local deletion marker (e.g., a tombstone set or
deletion flag) for that ipns; otherwise push remote_child into merged. Update
the condition that uses local.children to check for a concrete deletion
indicator (e.g., local.deletions.contains(ipns) or a tombstone field) and fall
back to preserving remote_child (merged.push(remote_child.clone())) when no
explicit deletion is present.
- Around line 367-370: The retry/error paths in the metadata update flow leave
pinned CIDs orphaned: ensure calls to crate::api::ipfs::unpin_content(&api, ...)
are added to clean up new_cid and retry_cid where appropriate. Specifically, in
the function handling metadata update (the block that creates new_cid, attempts
the write, and then performs a retry path), unpin old_metadata_cid after a
successful replacement (already done), and add unpin_content calls to (1) unpin
new_cid if a later retry succeeded with retry_cid (so only the final CID remains
pinned), and (2) unpin both new_cid and retry_cid on persistent conflict/failure
so neither remains pinned; use the existing api variable and match/Result
branches around the retry logic to place these unpin calls.
In `@apps/desktop/src-tauri/src/fuse/windows/write_ops.rs`:
- Around line 166-173: The coordinator.record_publish(...) call is being
executed regardless of the publish result; update the logic so that
coordinator.record_publish(&ipns_name_clone, 0) is invoked only when
crate::api::ipns::publish_ipns(&api, &req).await? returns
crate::api::ipns::PublishResult::Success, and do not record the sequence on
PublishResult::Conflict (or other variants); locate the match handling around
publish_ipns and move or conditionally call coordinator.record_publish inside
the Success arm to ensure initial folder publish sequence is recorded only on
success.
- Around line 202-216: The code runs success-tail logic after any PublishResult,
causing conflicts to share post-publish cleanup (e.g., unpinning parent_old_cid)
and leaving the orphaned parent_meta_cid; fix by moving all success-only
post-processing into the Success arm of the match (e.g., keep
coordinator.record_publish(&parent_ipns_name, new_seq) and any unpinning of
parent_old_cid inside PublishResult::Success), and handle
PublishResult::Conflict separately to cleanup the failed-attempt meta CID: call
crate::api::ipfs::unpin_content(&api, &parent_meta_cid).await for the orphaned
parent_meta_cid (if Some) and do not call coordinator.record_publish or unpin
parent_old_cid in the Conflict branch so no success-tail runs on conflict.
Ensure references to publish_ipns, PublishResult::Success/Conflict,
coordinator.record_publish, parent_old_cid, parent_meta_cid, and unpin_content
are adjusted accordingly.
In `@apps/desktop/src-tauri/src/fuse/write_ops.rs`:
- Around line 457-466: The code currently calls
coordinator.record_publish(&ipns_name_clone, 0) and logs success regardless of
publish_ipns outcome, which will desync PublishCoordinator when publish_ipns
returns PublishResult::Conflict; update the logic in the async block handling
crate::api::ipns::publish_ipns(&api, &req).await? so that
PublishCoordinator::record_publish is only invoked for the
PublishResult::Success path (and not for PublishResult::Conflict), i.e. move the
coordinator.record_publish(&ipns_name_clone, 0) and the success log into the
Success arm and do not record sequence 0 on Conflict to preserve per-name
serialization and monotonic sequence numbers.
In `@apps/web/src/hooks/useFileOperations.ts`:
- Around line 140-153: The retry branch that handles conflict errors (uses
isConflictError, useSyncStore.getState().setConflict/clearConflict,
resyncFolder, and performAddFile) must always clear the conflict state even if
resyncFolder throws: wrap the retry logic so setConflict is called before resync
and ensure clearConflict is invoked in a finally block (or ensure both the outer
and inner try/catch use finally to call clearConflict) around the resync + retry
performAddFile flow; apply the same finally-based clearConflict pattern to the
other similar retry branch that uses the same helpers to prevent the UI
remaining stuck in conflict state.
In `@apps/web/src/hooks/useFolderMutations.ts`:
- Around line 136-149: The conflict-state can stay set if an exception occurs
before the retry block's clearConflict is reached; in the blocks around
performParentUpdate/resyncFolder where you call
useSyncStore.getState().setConflict(...), wrap the resync + retry logic in a try
{ await resyncFolder(...); await delay(...); try { finalChildren = await
performParentUpdate(); } catch(retryErr) { if (isConflictError(retryErr)) throw
new Error('Conflict persists after re-sync. Please try again.'); throw retryErr;
} } finally { useSyncStore.getState().clearConflict(); } so clearConflict() is
always executed; apply the same change to the other occurrences noted (lines
near performParentUpdate, resyncFolder, and any setConflict/clearConflict
pairs).
- Around line 390-401: The bug is that movedItem is looked up after the source
children were modified, so the item can be missing when appending to the
destination; fix by reading/storing the moved item from the source before you
mutate it. Specifically, call getSourceFolder() (or use the existing
freshSource) and find the movedItem (by itemId) prior to calling
useFolderStore.getState().updateFolderChildren(sourceParentId, ...) so that
movedItem is available when you later update the dest folder via
updateFolderChildren(destParentId, ...); keep references to getSourceFolder,
getDestFolder, movedItem, sourceParentId and the
useFolderStore.getState().updateFolderChildren calls so reviewers can locate the
change.
In `@tests/e2e-desktop/scripts/test-conflict-detection.ps1`:
- Around line 106-148: Invoke-BumpServerSequence currently only logs warnings on
failure so callers continue and tests may skip the 409/resync path; change it to
treat bump failures as hard preconditions by throwing or exiting with a non-zero
status when the publish fails or the response does not indicate success (i.e.,
replace the soft Write-Host warnings in Invoke-BumpServerSequence with a
terminating action such as throw or exit/return non-zero), and ensure the
failure propagates to callers so tests abort instead of continuing; apply the
same change to the other similar publish/resolution blocks that only warn (the
other publish/resolve sections using $PublishResp/$ResolveResp) so any inability
to bump the server sequence fails the test immediately.
- Around line 201-209: The test is bumping $RootIpns but then writes to
"conflict-dir\nested.txt", so the wrong IPNS record is being bumped; change the
flow to identify and bump the containing folder's IPNS (the per-folder entry for
"conflict-dir") instead of $RootIpns (i.e., call Invoke-BumpServerSequence with
the IPNS for the folder being modified), and after Set-Content on
conflict-dir\nested.txt ensure the test triggers a republish of the containing
folder's IPNS by re-encrypting the folder metadata (children array, encrypted
names/CIDs/keys/timestamps) and publishing that folder-level IPNS entry so
per-folder IPNS semantics are exercised.
- Around line 156-157: Replace fixed 8s (and other fixed) sleeps with a
deterministic wait for the desktop publish to complete: after writing/mutating
files in the script, query the PublishCoordinator (or the publish/metadata API)
for the given IPNS name and wait until the monotonic sequence number advances
(or an explicit publish-complete indicator) or until the 10s safety-valve
timeout elapses; respect the 1.5s debounce window before expecting a publish.
Update the wait logic used after writing conflict-test-1.txt (and the similar
block at lines 195-196) to poll the PublishCoordinator/status endpoint with a
max wait of 10s and treat success when sequence number increments (or a
publish-complete response) is observed.
In `@tests/e2e-desktop/scripts/test-conflict-detection.sh`:
- Around line 45-47: The current AUTH_RESPONSE curl invocation embeds the secret
in the -d JSON string (AUTH_RESPONSE=$(curl ... -d
"{\"email\":\"$TEST_EMAIL\",\"secret\":\"$SECRET\"}") ), which exposes it via
process args; change the call to feed the JSON payload over stdin using
--data-binary `@-` (or --data `@-`) and echo/printf the JSON into curl so the secret
comes from the TEST_SECRET/SECRET environment variable on stdin rather than a
command-line argument; update the AUTH_RESPONSE assignment to use the
stdin-based curl invocation and ensure TEST_EMAIL and TEST_SECRET/SECRET are
read from env vars prior to the call.
In `@tests/e2e/tests/conflict-detection.spec.ts`:
- Line 36: The "remote cleanup" test currently lives inside
test.describe.serial('Conflict Detection') which causes it to be skipped if any
prior serial test fails; move the cleanup logic out of that serial test flow and
into a test.afterAll() hook so it always runs. Locate the existing cleanup test
(the one performing remote-item teardown) inside the Conflict Detection describe
block and extract its body into a test.afterAll(async () => { ... }) placed next
to the describe, invoking the same remote cleanup helper/logic (or call the same
cleanup function used in the test) and remove the original cleanup test case
from the serial suite.
---
Outside diff comments:
In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 175-205: The OCC check in the update path (where getFolderIpns is
used then folderIpnsRepository.save is called) is not atomic: convert the
read-then-save pattern into an atomic conditional update (e.g., use a
transaction with SELECT ... FOR UPDATE or perform a single DB UPDATE via
folderIpnsRepository.createQueryBuilder().update(...) with a WHERE that includes
userId, ipnsName and sequenceNumber = expectedSequenceNumber and uses
sequenceNumber = sequenceNumber + 1 and sets other fields), then check
affectedRows and throw the same ConflictException if no rows were updated; if
the update succeeds, return the updated record (or re-load it) and avoid the
non-atomic existing -> save flow.
- Around line 96-144: The chunk currently uses Promise.allSettled over batch.map
which allows other entries to persist/publish while one later throws a
ConflictException, causing partial commits; replace the concurrent
Promise.allSettled approach with a sequential for-loop that processes each entry
one-by-one (validate base64, call this.delegatedRouting.publish, call
this.upsertFolderIpns) and on any thrown ConflictException immediately rethrow
to abort the chunk (do not continue processing remaining entries), so that
functions like upsertFolderIpns and delegatedRouting.publish are not executed
for subsequent entries after a conflict; remove or adapt the settled/result
handling logic accordingly.
In `@apps/desktop/src-tauri/src/fuse/write_ops.rs`:
- Around line 506-524: The parent-publish conflict handling falls through to the
success-tail cleanup/logging, leaving the newly uploaded parent_meta_cid pinned
and emitting a misleading success log; modify the publish_ipns match so that
PublishResult::Success performs coordinator.record_publish(&parent_ipns_name,
new_seq), unpins parent_old_cid (calling crate::api::ipfs::unpin_content(&api,
&old).await) and logs "Parent metadata published after mkdir", while
PublishResult::Conflict explicitly unpins the freshly uploaded parent_meta_cid
(call crate::api::ipfs::unpin_content(&api, &parent_meta_cid).await or
equivalent) and logs or warns about conflict without running the success-tail
actions; ensure the unpin of parent_old_cid is not executed for Conflict unless
intended and keep references to publish_ipns, PublishResult::Success,
PublishResult::Conflict, coordinator.record_publish, parent_old_cid,
parent_meta_cid, and crate::api::ipfs::unpin_content to locate the changes.
In `@apps/web/src/hooks/useFolderMutations.ts`:
- Around line 461-492: The current pre-flight validations (involving batchNames,
duplicate name checks against destFolder.children, and folder-specific checks
using useFolderStore.getState().folders, folderService.isDescendantOf,
folderService.getDepth, folderService.calculateSubtreeDepth and
MAX_FOLDER_DEPTH) are only run once before the retry loop, so after a 409 +
resync the retry path skips them; re-run the same validations against the
freshly synced state immediately after resync and before any retry of the batch
move (i.e., rebuild batchNames and re-check intra-batch duplicates, re-check
nameExists against the current destFolder.children, and re-run the folder
descendant and depth checks using the updated folders from
useFolderStore.getState()) so concurrent changes cannot bypass collision/depth
prevention.
---
Nitpick comments:
In `@packages/api-client/openapi.json`:
- Around line 789-791: The 409 responses for the endpoints /ipns/publish and
/ipns/publish-batch are missing response body schemas; add a
content.application/json.schema for the 409 response in each endpoint describing
a typed conflict object that includes currentSequenceNumber (e.g., { type:
"object", properties: { currentSequenceNumber: { type: "integer" } }, required:
["currentSequenceNumber"] }) so generated clients are strongly typed and
consistent with the 200 responses; update the 409 entries for both endpoints
accordingly.
In `@tests/e2e-desktop/scripts/test-conflict-detection.sh`:
- Around line 130-133: Replace the fixed sleeps (e.g., the "sleep 8" after
writing "conflict-test-1.txt" and other "sleep 15"/"sleep 8" uses) with
condition-based polling: after writing or updating files like
"conflict-test-1.txt" poll for the expected observable state (for example, the
published FUSE mount reflecting the new file content, a specific file timestamp,
or the presence/contents of a sync/publish marker) with a short interval and a
timeout, rather than a fixed delay; update all occurrences (the sleep commands
around the writes and bumps) to use the same wait-until function/loop that
checks the actual condition and exits early when satisfied to avoid CI timing
flakes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/debug/desktop-e2e-ci-all-platforms-resolved.md.planning/phases/16-advanced-sync/16-01-PLAN.md.planning/phases/16-advanced-sync/16-01-SUMMARY.md.planning/phases/16-advanced-sync/16-02-PLAN.md.planning/phases/16-advanced-sync/16-02-SUMMARY.md.planning/phases/16-advanced-sync/16-03-PLAN.md.planning/phases/16-advanced-sync/16-03-SUMMARY.md.planning/phases/16-advanced-sync/16-04-PLAN.md.planning/phases/16-advanced-sync/16-04-SUMMARY.md.planning/phases/16-advanced-sync/16-05-PLAN.md.planning/phases/16-advanced-sync/16-05-SUMMARY.md.planning/phases/16-advanced-sync/16-CONTEXT.md.planning/phases/16-advanced-sync/16-RESEARCH.md.planning/phases/16-advanced-sync/16-VERIFICATION.md.planning/todos/pending/2026-02-14-bring-your-own-ipfs-node.mdapps/api/src/ipns/dto/publish.dto.tsapps/api/src/ipns/ipns.controller.tsapps/api/src/ipns/ipns.service.spec.tsapps/api/src/ipns/ipns.service.tsapps/desktop/src-tauri/src/api/ipns.rsapps/desktop/src-tauri/src/commands/vault.rsapps/desktop/src-tauri/src/fuse/mod.rsapps/desktop/src-tauri/src/fuse/operations.rsapps/desktop/src-tauri/src/fuse/windows/operations.rsapps/desktop/src-tauri/src/fuse/windows/write_ops.rsapps/desktop/src-tauri/src/fuse/write_ops.rsapps/desktop/src-tauri/src/registry/mod.rsapps/web/src/App.cssapps/web/src/api/models/publishIpnsDto.tsapps/web/src/api/models/publishIpnsEntryDto.tsapps/web/src/components/file-browser/SyncIndicator.tsxapps/web/src/hooks/useFileOperations.tsapps/web/src/hooks/useFolderMutations.tsapps/web/src/lib/errors.tsapps/web/src/services/folder.service.tsapps/web/src/services/ipns.service.tsapps/web/src/stores/sync.store.tspackages/api-client/openapi.jsontests/e2e-desktop/scripts/run-all.ps1tests/e2e-desktop/scripts/run-all.shtests/e2e-desktop/scripts/test-conflict-detection.ps1tests/e2e-desktop/scripts/test-conflict-detection.shtests/e2e/page-objects/file-browser/upload-zone.page.tstests/e2e/tests/conflict-detection.spec.tstests/e2e/utils/conflict-helpers.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Implements Phase 16 optimistic concurrency (“expectedSequenceNumber”) for IPNS folder publishes across API, web, and desktop clients, plus E2E coverage and planning/verification docs.
Changes:
- API: adds optional
expectedSequenceNumberto publish DTOs and returns HTTP 409 on sequence mismatch (plus OpenAPI updates + unit tests). - Web: threads
expectedSequenceNumberthrough folder publish paths, adds conflict UI state, and implements catch → re-sync → single retry for folder mutations and uploads. - Desktop: adds
PublishResult::Conflicthandling, retries folder metadata publishes with re-fetch + merge + jitter, and adds desktop E2E scripts.
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/utils/conflict-helpers.ts | Adds helper to bump server sequence via unconditional publish for conflict simulation. |
| tests/e2e/tests/conflict-detection.spec.ts | Adds Playwright E2E suite covering upload/create-folder conflict + per-file negative case. |
| tests/e2e/page-objects/file-browser/upload-zone.page.ts | Expands file input locator to work in empty-state dropzone. |
| tests/e2e-desktop/scripts/test-conflict-detection.sh | Adds bash FUSE conflict detection script (seq bump + verify retry). |
| tests/e2e-desktop/scripts/test-conflict-detection.ps1 | Adds PowerShell FUSE conflict detection script (Windows). |
| tests/e2e-desktop/scripts/run-all.sh | Integrates conflict detection script into desktop E2E orchestrator. |
| tests/e2e-desktop/scripts/run-all.ps1 | Integrates conflict detection script into desktop E2E orchestrator. |
| packages/api-client/openapi.json | Documents new expectedSequenceNumber fields and 409 responses in OpenAPI. |
| apps/web/src/stores/sync.store.ts | Adds conflict sync status + message and actions. |
| apps/web/src/services/ipns.service.ts | Threads expectedSequenceNumber through publish + batch publish. |
| apps/web/src/services/folder.service.ts | Sends pre-increment expectedSequenceNumber on folder publishes; handles rotation publish conflicts. |
| apps/web/src/lib/errors.ts | Adds isConflictError() utility matching Orval custom-instance error shape. |
| apps/web/src/hooks/useFolderMutations.ts | Adds conflict catch → re-sync → retry to folder mutation handlers. |
| apps/web/src/hooks/useFileOperations.ts | Adds conflict catch → re-sync → retry to addFile/addFiles paths. |
| apps/web/src/components/file-browser/SyncIndicator.tsx | Shows amber “conflict” state during re-sync. |
| apps/web/src/api/models/publishIpnsEntryDto.ts | Regenerated client model with expectedSequenceNumber. |
| apps/web/src/api/models/publishIpnsDto.ts | Regenerated client model with expectedSequenceNumber. |
| apps/web/src/App.css | Adds styling for conflict sync indicator icon. |
| apps/desktop/src-tauri/src/registry/mod.rs | Updates publish call sites to handle PublishResult and set expected_sequence_number. |
| apps/desktop/src-tauri/src/fuse/write_ops.rs | Updates folder publish call sites to include/omit expected_sequence_number appropriately. |
| apps/desktop/src-tauri/src/fuse/windows/write_ops.rs | Windows equivalent updates for folder publish call sites. |
| apps/desktop/src-tauri/src/fuse/windows/operations.rs | Ensures per-file publish uses expected_sequence_number: None and handles PublishResult. |
| apps/desktop/src-tauri/src/fuse/operations.rs | Ensures per-file publish uses expected_sequence_number: None and handles PublishResult. |
| apps/desktop/src-tauri/src/fuse/mod.rs | Implements conflict re-fetch + merge + jitter + single retry for folder metadata publish. |
| apps/desktop/src-tauri/src/commands/vault.rs | Updates initial vault publish to handle PublishResult and omit conflict checks. |
| apps/desktop/src-tauri/src/api/ipns.rs | Adds expected_sequence_number, PublishResult enum, and 409 parsing. |
| apps/api/src/ipns/ipns.service.ts | Adds conflict detection in upsert + threads expected seq through publish/batch. |
| apps/api/src/ipns/ipns.service.spec.ts | Adds API unit tests for conflict detection paths. |
| apps/api/src/ipns/ipns.controller.ts | Documents 409 responses for publish endpoints. |
| apps/api/src/ipns/dto/publish.dto.ts | Adds optional expectedSequenceNumber with validation to DTOs. |
| .planning/todos/pending/2026-02-14-bring-your-own-ipfs-node.md | Notes interaction between BYO-IPFS and API-based conflict detection. |
| .planning/phases/16-advanced-sync/16-VERIFICATION.md | Adds Phase 16 verification report with artifacts/links. |
| .planning/phases/16-advanced-sync/16-CONTEXT.md | Adds Phase 16 context and scope decisions. |
| .planning/phases/16-advanced-sync/16-05-SUMMARY.md | Documents desktop conflict E2E plan execution. |
| .planning/phases/16-advanced-sync/16-05-PLAN.md | Adds desktop conflict E2E execution plan. |
| .planning/phases/16-advanced-sync/16-04-SUMMARY.md | Documents web conflict E2E plan execution. |
| .planning/phases/16-advanced-sync/16-04-PLAN.md | Adds web conflict E2E execution plan. |
| .planning/phases/16-advanced-sync/16-03-SUMMARY.md | Documents desktop conflict handling implementation. |
| .planning/phases/16-advanced-sync/16-03-PLAN.md | Adds desktop conflict handling execution plan. |
| .planning/phases/16-advanced-sync/16-02-SUMMARY.md | Documents web conflict handling implementation. |
| .planning/phases/16-advanced-sync/16-02-PLAN.md | Adds web conflict handling execution plan. |
| .planning/phases/16-advanced-sync/16-01-SUMMARY.md | Documents API optimistic concurrency implementation. |
| .planning/phases/16-advanced-sync/16-01-PLAN.md | Adds API optimistic concurrency execution plan. |
| .planning/STATE.md | Updates project state to mark Phase 16 complete. |
| .planning/ROADMAP.md | Updates roadmap to mark Phase 16 complete and clarify scope. |
| .planning/REQUIREMENTS.md | Marks SYNC-04 as complete. |
- merge_folder_children: preserve remote children not in local to prevent concurrent addition data loss; use deterministic Vec iteration order instead of HashMap (Threads 1, 14, 15) - Unpin orphaned CIDs in conflict retry paths (Thread 2) - Only record_publish on Success for new folder IPNS in both macOS and Windows write_ops (Threads 3, 4, 5) - Move parent old CID unpin into Success branch only (Thread 4) - Extract resyncFolder to shared folder-helpers.ts to eliminate duplication between useFileOperations and useFolderMutations (Threads 18, 19) - Use try/finally for clearConflict in all retry paths to prevent stuck conflict UI state (Threads 6, 7) - Fix handleMove bookkeeping: save movedItem BEFORE updating source children to prevent undefined lookup (Threads 8, 17) - Make PowerShell sequence bump a hard precondition (Thread 9) - Pass test secret via stdin in bash test script (Thread 12) - Move E2E cleanup to afterAll to run even if serial tests fail (Thread 13) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e4cae8e39220
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/utils/conflict-helpers.ts (1)
97-99: Validate publish response shape before returning sequence.
as { sequenceNumber: string }is unchecked at runtime; add a guard so malformed success payloads fail with a clear error.♻️ Suggested refactor
- const published = (await publishResponse.json()) as { sequenceNumber: string }; - - return { newSequenceNumber: published.sequenceNumber }; + const published = (await publishResponse.json()) as { sequenceNumber?: unknown }; + if (typeof published.sequenceNumber !== 'string' || !published.sequenceNumber) { + throw new Error('Publish response missing valid sequenceNumber'); + } + + return { newSequenceNumber: published.sequenceNumber };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/conflict-helpers.ts` around lines 97 - 99, The code assumes publishResponse.json() returns { sequenceNumber: string } without runtime checks; update the logic around the published variable to validate the response shape (e.g., confirm published is an object and typeof published.sequenceNumber === 'string') before returning, and throw a clear error if the validation fails so callers of the function (the code that awaits publishResponse.json() and returns { newSequenceNumber: published.sequenceNumber }) receive an explicit failure for malformed payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/fuse/mod.rs`:
- Line 359: The two calls to crate::api::ipns::publish_ipns currently use await?
immediately after uploading metadata CIDs, which causes early return on any
non-409 error and leaves freshly uploaded CIDs pinned; change each call so you
explicitly match the Result from crate::api::ipns::publish_ipns(...) instead of
using await?, and on Err run the cleanup that unpins the uploaded metadata CIDs
(or call the existing unpin routine used for temporary uploads / implement unpin
logic to remove those orphaned CIDs) before propagating the error; apply this
fix at both publish sites where publish_ipns is awaited after a CID upload.
In `@tests/e2e/utils/conflict-helpers.ts`:
- Around line 50-53: The thrown Error in resolve logic currently embeds the full
response body from resolveResponse.text(), which may leak sensitive data; update
the code in tests/e2e/utils/conflict-helpers.ts (the block that constructs the
Error for "Failed to resolve IPNS for sequence bump" and the similar error at
lines ~93-94) to omit the raw body and instead include only non-sensitive
context such as resolveResponse.status(), resolveResponse.statusText() or a
truncated/sanitized summary (e.g., first N characters or a fixed message like
"response body omitted"), so replace inclusion of body with a safe summary when
building the Error message.
---
Nitpick comments:
In `@tests/e2e/utils/conflict-helpers.ts`:
- Around line 97-99: The code assumes publishResponse.json() returns {
sequenceNumber: string } without runtime checks; update the logic around the
published variable to validate the response shape (e.g., confirm published is an
object and typeof published.sequenceNumber === 'string') before returning, and
throw a clear error if the validation fails so callers of the function (the code
that awaits publishResponse.json() and returns { newSequenceNumber:
published.sequenceNumber }) receive an explicit failure for malformed payloads.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/fuse/mod.rsapps/web/src/stores/sync.store.tstests/e2e/utils/conflict-helpers.ts
Add 19 Rust unit tests covering the Phase 16 conflict detection code: - merge_folder_children: 12 tests covering empty inputs, additive merge, last-writer-wins per child, concurrent additions from both devices, rename handling, ordering guarantees, deduplication, and scale - IpnsPublishRequest serialization: 4 tests for camelCase output, optional field omission, and TEE field inclusion - IpnsResolveResponse deserialization: 2 tests for camelCase parsing - PublishResult Debug derive: 1 test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e5d6c9d085ad
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/api/ipns.rs`:
- Around line 106-115: The code in ipns.rs that handles a 409 conflict should
not fabricate "0" when parsing resp.json().await and reading
body["currentSequenceNumber"]; instead, if resp.json() fails or the field is
missing/not a string, return an Err describing a malformed 409 conflict (don't
use unwrap_or_default()/unwrap_or("0")). Update the branch that currently
returns PublishResult::Conflict with current_sequence_number: current_seq to
validate resp.json().await and body["currentSequenceNumber"] and return a proper
error (or map to an existing publish error type) when parsing/field extraction
fails, only returning PublishResult::Conflict when a valid sequence string is
present.
Replace unwrap_or("0") fallback with proper error propagation when the
409 Conflict response body is malformed or missing currentSequenceNumber.
Fabricating "0" could mislead retry logic into using an incorrect sequence.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: f2a572bb01d3
After a 409 conflict triggers resync in handleMoveItems, the retry was calling performBatchMove() without re-running name-collision and depth- limit checks. Concurrent changes from another device could introduce duplicate names or depth violations in the destination folder. Extract validateBatchMove() and call it before each attempt. Also documents all CodeRabbit review findings in the phase 16 verification report: 14 issues fixed in this phase and 6 pre-existing issues deferred to future work with rationale. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4986ee5c5652
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/hooks/useFolderMutations.ts`:
- Around line 97-104: The calls to updateFolderMetadata() currently ignore its
returned updated metadata (including the new sequenceNumber), and new folders
are saved with sequenceNumber: 0n causing stale local state and 409s; change the
code to capture the result of updateFolderMetadata(...) into a variable, update
the in-memory folder metadata/state with the returned object (specifically set
sequenceNumber to the returned sequenceNumber as a BigInt), and when
creating/persisting a new folder replace the hardcoded 0n with the
sequenceNumber from the publish response/updateFolderMetadata result so local
state reflects the published sequence.
- Around line 443-444: The current batch-move logic computes
currentMovedChildren by filtering currentSource.children using itemIds, which
allows silent partial success if some selected IDs are missing; change the flow
so after computing currentMovedChildren you explicitly verify that
currentMovedChildren.length === itemIds.size and if not, abort the entire
operation (return an error/throw or surface a user-facing failure) instead of
continuing; apply the same check at the second occurrence where you derive moved
items (the block around the other currentMovedChildren/itemIds usage near lines
490–491), and ensure the caller/user sees a clear error message so no partial
moves are performed.
- Around line 716-726: The batch-delete path calls
folderService.updateFolderMetadata but does not persist the updated sequence
number into local in-memory metadata, causing future writes to use a stale
sequence; modify the flow in useFolderMutations.ts so that when calling
folderService.updateFolderMetadata (the call using freshParent, updatedChildren,
folderKey, ipnsPrivateKey, ipnsName, sequenceNumber) you capture the
returned/updated folder metadata (including the new sequenceNumber), update the
local in-memory folder state (the freshParent entry in the cache/store and any
local children update logic that runs around lines 748–750), and ensure you also
trigger the POST /vault/publish-ipns step with the signed entry as required by
guidelines so the local sequenceNumber matches the published one before
returning updatedChildren.
- Around line 635-638: The single-folder delete only calls
useFolderStore.getState().removeFolder(itemId) and leaves loaded descendant
folders and their key material orphaned; update this flow to remove descendants
too by either using/adding a recursive removal helper (e.g.,
removeFolderRecursively or removeFolderAndDescendants) or by retrieving
descendant IDs from the store (traverse
useFolderStore.getState().folders/children to collect descendant IDs) and
iterating: for each descendant ID first zero/clear sensitive key material (e.g.,
folder.keyMaterial = undefined or call a wipeKeyMaterial(folderId)) and then
call useFolderStore.getState().removeFolder(descendantId), and finally remove
the root itemId the same way so no descendants or key material remain in local
state.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.planning/phases/16-advanced-sync/16-VERIFICATION.mdapps/desktop/src-tauri/src/api/ipns.rsapps/web/src/hooks/useFolderMutations.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/api/ipns.rs
1. Persist sequence numbers after create publishes: capture newSequenceNumber from updateFolderMetadata and update the store for both parent folder and new folder (was using stale 0n). 2. Fail batch move if items disappear: add count check after filtering source children by itemIds in both validateBatchMove and performBatchMove to prevent silent partial moves. 3. Recursively remove descendant folders on single-folder delete: walk loaded children and removeFolder for each, preventing orphaned entries with key material in the store. 4. Update parent sequence after batch delete publish: capture and persist newSequenceNumber to prevent stale-sequence 409s on the next mutation against the same folder. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e3650ebf70ad
getAccessToken() and getRootIpnsName() were preferring the authState captured during beforeAll, which could become stale if tokens refresh mid-suite. Now reads from the live Zustand store first and only falls back to captured authState if the store isn't available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 231012bfc58c
4 tasks
This was referenced Mar 7, 2026
4 tasks
Closed
8 tasks
Merged
This was referenced Apr 2, 2026
5 tasks
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
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
expectedSequenceNumberfield to IPNS publish endpoints. When present, the API compares against the stored sequence infolder_ipns— mismatch returns HTTP 409 with the current sequence number. Backward-compatible: omitting the field uses unconditional publish (TEE republish, per-file publishes, old clients).expectedSequenceNumberand handle 409 with automatic re-sync + single retry. Sync indicator shows amber "conflict" state during re-sync. Per-file IPNS publishes use last-write-wins (no conflict detection).PublishResult::Conflictenum, re-fetches remote metadata, performs additive merge of folder children using IPNS name as stable key, and retries once. OS notification sent on conflict detection.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI
Tests
Documentation