fix(web): update sequence numbers after single-item folder mutations#256
Merged
Conversation
handleMove, handleRename, and handleDelete in useFolderMutations did not call updateFolderSequence after successful IPNS publishes, leaving stale sequence numbers in the Zustand store. The next operation on the same folder would send an outdated expectedSequenceNumber, triggering a 409 conflict that the UI swallowed silently (e.g. move dialog staying open). Changes: - folder.service: moveFolder, moveFile, renameFolder, renameFile, deleteFolder, deleteFileFromFolder now return new sequence numbers from their updateFolderMetadata calls - useFolderMutations: handleMove, handleRename, handleDelete capture returned sequences and call store.updateFolderSequence() This aligns single-item operations with the batch operations (handleMoveItems, handleDeleteItems) which already tracked sequences. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 18b82a4a029c
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughCentralized conflict-retry via a new withConflictRetry helper; folder/file service methods now return sequence-number metadata which mutation hooks consume to persist updated sequences to local store. File add/move/rename/delete flows updated to use the retry helper; public hook APIs unchanged. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Hook as useFileOperations / useFolderMutations
participant Retry as withConflictRetry
participant Service as folder.service
participant Resync as resyncFolder
participant Store as LocalStore
User->>Hook: trigger operation (create/rename/move/delete/add)
Hook->>Retry: call perform operation via withConflictRetry
Retry->>Service: perform operation (publish/update metadata)
Service->>Store: publish/update (IPNS / metadata)
Service-->>Retry: return { newSequenceNumber / destSequenceNumber / sourceSequenceNumber }
Retry-->>Hook: return operation result
Retry->>Resync: on 409 -> show sync, call resyncFolder, wait, then retry
Hook->>Store: persist returned sequence numbers and updated children
Hook-->>User: return result (public API unchanged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes stale IPNS sequence numbers in the web app’s Zustand folder store after single-item mutations, preventing follow-up operations from sending an outdated expectedSequenceNumber and hitting 409 conflicts.
Changes:
- Updated single-item folder/file mutation service functions to return the new IPNS
sequenceNumberfromupdateFolderMetadata. - Updated
useFolderMutationssingle-item handlers (rename/move/delete) to persist returned sequence numbers viaupdateFolderSequence. - Adjusted move handlers to persist both source and destination sequence numbers after a move.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/web/src/services/folder.service.ts | Changes mutation APIs to return new sequence numbers from IPNS publishes. |
| apps/web/src/hooks/useFolderMutations.ts | Captures returned sequence numbers and persists them to Zustand after rename/move/delete. |
On WinFSP FUSE mounts, Remove-Item can throw terminating .NET exceptions
("The system cannot find the file specified") that -ErrorAction
SilentlyContinue does not suppress. This caused the conflict detection
test suite to report 1 failure even though all actual tests passed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 1cbe792ba670
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e-desktop/scripts/test-conflict-detection.ps1 (1)
232-234: Avoid fully silent catches in cleanup to keep CI failures diagnosable.Line 232, Line 233, and Line 234 swallow all exceptions. Consider logging a low-noise warning/verbose message in each catch so unexpected mount/runtime issues are still visible without failing cleanup.
Proposed tweak
-try { Remove-Item -Path "$MountPoint\conflict-test-1.txt" -Force -ErrorAction SilentlyContinue } catch {} -try { Remove-Item -Path "$MountPoint\conflict-test-2.txt" -Force -ErrorAction SilentlyContinue } catch {} -try { Remove-Item -Path "$MountPoint\conflict-dir" -Recurse -Force -ErrorAction SilentlyContinue } catch {} +try { Remove-Item -Path "$MountPoint\conflict-test-1.txt" -Force -ErrorAction SilentlyContinue } catch { Write-Verbose "Cleanup skipped conflict-test-1.txt: $_" } +try { Remove-Item -Path "$MountPoint\conflict-test-2.txt" -Force -ErrorAction SilentlyContinue } catch { Write-Verbose "Cleanup skipped conflict-test-2.txt: $_" } +try { Remove-Item -Path "$MountPoint\conflict-dir" -Recurse -Force -ErrorAction SilentlyContinue } catch { Write-Verbose "Cleanup skipped conflict-dir: $_" }🤖 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.ps1` around lines 232 - 234, The three try/catch cleanup blocks that call Remove-Item for "$MountPoint\conflict-test-1.txt", "$MountPoint\conflict-test-2.txt" and "$MountPoint\conflict-dir" currently swallow all exceptions; change each catch to emit a low-noise diagnostic (e.g., Write-Verbose or Write-Warning) that includes the target path and the caught exception ($_ or $PSItem) so CI logs show unexpected mount/runtime errors while still not failing cleanup, keeping Remove-Item and the same -Force/-Recurse/-ErrorAction usage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e-desktop/scripts/test-conflict-detection.ps1`:
- Around line 232-234: The three try/catch cleanup blocks that call Remove-Item
for "$MountPoint\conflict-test-1.txt", "$MountPoint\conflict-test-2.txt" and
"$MountPoint\conflict-dir" currently swallow all exceptions; change each catch
to emit a low-noise diagnostic (e.g., Write-Verbose or Write-Warning) that
includes the target path and the caught exception ($_ or $PSItem) so CI logs
show unexpected mount/runtime errors while still not failing cleanup, keeping
Remove-Item and the same -Force/-Recurse/-ErrorAction usage intact.
… moves moveFolder() and moveFile() used the stale pre-increment sequence number for the second updateFolderMetadata call when moving within the same folder, which would trigger a spurious 409 conflict. The batch variant (handleMoveItems) already had this guard. Align single-item variants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 62a37c394b40
- Extract `withConflictRetry<T>()` into folder-helpers.ts, replacing 8 copy-pasted conflict-retry scaffolds across useFolderMutations and useFileOperations (-89 lines net) - Remove unused `unpinCid` parameter from deleteFolder service - Propagate orphaned file IPNS names through handleDelete and fire-and-forget resolve+unpin their metadata CIDs - Unify handleCreate to use performParentUpdate for both initial attempt and retry (was using different code paths) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 2f940c493c11
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
==========================================
+ Coverage 46.31% 46.68% +0.37%
==========================================
Files 106 106
Lines 8266 8255 -11
Branches 591 594 +3
==========================================
+ Hits 3828 3854 +26
+ Misses 4271 4233 -38
- 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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
apps/web/src/hooks/useFolderMutations.ts:636
- If
deleteFolderdrops the unusedunpinCidparameter (it’s not used in the service), this call site should also stop passingunpinFromIpfsto avoid carrying dead arguments through the stack.
* Delete multiple files/folders in a single IPNS publish.
*
* Removes all items from parent's children array, collects CIDs to unpin,
* then publishes metadata and IPNS once for the entire batch.
*
* @param items - Array of { id, type } to delete
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/web/src/hooks/useFolderMutations.ts`:
- Around line 611-618: The fire-and-forget loop calls
resolveIpnsRecord(...).then(...) and invokes unpinFromIpfs(resolved.cid) without
returning or catching its Promise, which can create unhandled rejections; update
the then handler in the loop that iterates deleteResult.orphanedIpnsNames so it
returns or explicitly catches the Promise from unpinFromIpfs (e.g., return
unpinFromIpfs(resolved.cid).catch(() => {}) or await inside an async then and
handle errors) so any rejection from unpinFromIpfs is observed and
swallowed/handled; ensure you reference resolveIpnsRecord, unpinFromIpfs, and
deleteResult.orphanedIpnsNames when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/src/hooks/folder-helpers.tsapps/web/src/hooks/useFileOperations.tsapps/web/src/hooks/useFolderMutations.tsapps/web/src/services/folder.service.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f5855bbc496a
This was referenced Mar 4, 2026
5 tasks
Closed
This was referenced Mar 23, 2026
Merged
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
handleMove,handleRename,handleDelete) inuseFolderMutationsdid not callupdateFolderSequenceafter IPNS publishes, leaving stale sequence numbers in the Zustand storeexpectedSequenceNumber→ 409 conflict that the UI swallowed silently (move dialog staying open, rename/delete appearing to succeed but leaving inconsistent state)moveFolder,moveFile,renameFolder,renameFile,deleteFolder,deleteFileFromFolder) to return new sequence numbers from theirupdateFolderMetadatacallsstore.updateFolderSequence()Root cause of E2E failure: Test 4.13 (single delete) left the root folder's sequence stale → Test 5.1 (move file) hit 409 →
handleMoveConfirmcaught error silently → dialog stayed open → 15s timeoutTest plan
full-workflow.spec.tspasses (specifically test 5.1 "Move file via context menu")pnpm typecheck)pnpm --filter web build)🤖 Generated with Claude Code
Summary by CodeRabbit