fix(web,desktop): navigation error recovery and release build init#166
Conversation
When navigateTo() fails to load a subfolder (IPNS resolve timeout, decryption error, etc.), the catch block was removing the folder placeholder from the store but leaving the URL at /files/<uuid>. This caused the app to render ~/root with an empty directory state, breaking subsequent E2E navigation. Now navigates back to the parent folder on failure, matching the existing behavior pattern used elsewhere in the codebase. Also adds retry logic to E2E navigateIntoFolder helper to handle transient IPNS propagation delays in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds recovery to folder navigation on subfolder load failures (navigates to parent or root and clears loading state) and hardens an e2e folder-navigation helper with a retry for transient CI navigation failures. Changes
Sequence Diagram(s)(Skipped — changes are limited to a hook and test retry; not introducing a new multi-component sequential feature that requires visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/hooks/useFolderNavigation.ts (1)
274-284:⚠️ Potential issue | 🔴 Critical
setIsLoading(false)is never called on the error path — loading state leaks.Tracing the execution:
setIsLoading(true)is set at line 209.- The catch block runs; the guard at line 269 passes (
latestNavTarget.current === targetFolderId).- Line 274 mutates
latestNavTarget.currenttoparentId ?? 'root'.- Finally evaluates
latestNavTarget.current === targetFolderId→ false (ref was just overwritten).setIsLoading(false)is skipped; the loading spinner is stuck forever after any subfolder load failure.The guard in
finallyis designed to avoid clearing a newer navigation's loading state — but here it incorrectly fires against the same navigation that just errored out.🐛 Proposed fix — clear loading before mutating the ref
useFolderStore.getState().removeFolder(targetFolderId); + setIsLoading(false); latestNavTarget.current = parentId ?? 'root'; if (parentId) { navigate(`/files/${parentId}`); } else { navigate('/files'); } } finally { - // Only clear loading if this is still the latest navigation if (latestNavTarget.current === targetFolderId) { setIsLoading(false); } }Calling
setIsLoading(false)before the ref update lets the existingfinallyguard still protect concurrent navigations while ensuring the failed navigation clears its own loading state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useFolderNavigation.ts` around lines 274 - 284, The loading state is never cleared on error because latestNavTarget.current is overwritten before the finally guard runs; update finally in useFolderNavigation so setIsLoading(false) is called before mutating latestNavTarget.current (or alternatively clear loading inside the catch for the failing navigation) — specifically, ensure setIsLoading(false) runs while latestNavTarget.current still equals targetFolderId (referencing latestNavTarget, targetFolderId, setIsLoading, and parentId) and then update latestNavTarget.current = parentId ?? 'root' and perform navigate; this preserves the guard against newer navigations while preventing a leaked loading state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/web/src/hooks/useFolderNavigation.ts`:
- Around line 274-284: The loading state is never cleared on error because
latestNavTarget.current is overwritten before the finally guard runs; update
finally in useFolderNavigation so setIsLoading(false) is called before mutating
latestNavTarget.current (or alternatively clear loading inside the catch for the
failing navigation) — specifically, ensure setIsLoading(false) runs while
latestNavTarget.current still equals targetFolderId (referencing
latestNavTarget, targetFolderId, setIsLoading, and parentId) and then update
latestNavTarget.current = parentId ?? 'root' and perform navigate; this
preserves the guard against newer navigations while preventing a leaked loading
state.
The catch block set latestNavTarget to the parent ID before the finally block ran. Since finally checks `latestNavTarget === targetFolderId`, setIsLoading(false) was never called, leaving the "Loading..." overlay permanently visible. This prevented the file list from rendering after navigating back to the parent, breaking the E2E retry logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
navigateTo()fails to load a subfolder (IPNS timeout, decryption error), the catch block was removing the folder placeholder from the store but leaving the URL at/files/<uuid>. Now navigates back to the parent folder on failure.latestNavTargetto the parent before thefinallyblock ran, sosetIsLoading(false)was never called, leaving the "Loading..." overlay permanently visible.get_dev_keyis only registered as a Tauri command in debug builds (#[cfg(debug_assertions)]), but the JS calledawait invoke('get_dev_key')without try/catch. In release builds the promise rejects, silently aborting the entire init function.navigateIntoFoldernow retries once if navigation fails (handles CI IPNS propagation delays).Test plan
🤖 Generated with Claude Code