fix(desktop): stop spurious folder picker on settings → dashboard nav#3602
fix(desktop): stop spurious folder picker on settings → dashboard nav#3602
Conversation
The folder-first import flow was plumbed through a zustand counter plus a useEffect in AddRepositoryModals. The counter outlives component mounts, but the effect runs on every mount — so once a user ever clicked "Import existing folder," every subsequent remount of the dashboard layout (including the one that happens when navigating back from Settings) re-ran the effect and re-opened the native directory picker. Remove the indirection. DashboardSidebarHeader now owns useFolderFirstImport and renders FolderFirstImportModal itself; the dropdown item calls folderImport.start() directly on click. No store pulse, no effect.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFolder-first import was removed from AddRepositoryModals and reimplemented in DashboardSidebarHeader using a local Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarHeader as Sidebar Header
participant FolderModal as FolderFirstImportModal
participant Toast as Sonner Toast
User->>SidebarHeader: Click "Import existing folder"
SidebarHeader->>FolderModal: folderImport.start() (open modal)
FolderModal->>SidebarHeader: return state (confirm / cancel)
alt confirmCreateAsNew
SidebarHeader->>Toast: onSuccess toast (rgba(40,167,69,0.5))
else error
SidebarHeader->>Toast: onError toast (rgba(220,53,69,0.5))
end
SidebarHeader->>FolderModal: folderImport.cancel() (close modal)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR fixes a bug where the native folder picker opened unprompted every time the user navigated from Settings back to the dashboard, after having used "Import existing folder" at least once in the session. Root cause fixed: The old architecture threaded the "start folder import" signal through a module-scoped Zustand counter incremented on user click, then consumed it in a Fix: The indirection is removed entirely. Key changes:
Confidence Score: 5/5Safe to merge — the fix is correct, the root cause is well-understood, and the new architecture is simpler and more direct. The counter+effect anti-pattern is eliminated and replaced with a direct event handler. The hook instance is correctly shared across both collapsed/expanded branches (single call at component level), only one FolderFirstImportModal renders at a time (mutually exclusive return branches), and the Zustand store is properly cleaned up. The only minor observation is a redundant .catch() that cannot cause harm beyond a theoretical double-toast in unreachable error conditions. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx | Now owns useFolderFirstImport and renders FolderFirstImportModal (Dialog portal) in both collapsed/expanded branches sharing one hook instance. Both DropdownMenuItems call handleImportFolder directly on select, eliminating the effect-based indirect trigger. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/AddRepositoryModals.tsx | Simplified to only own NewProjectModal; FolderFirstImportModal and its hook moved to DashboardSidebarHeader. The counter+effect pattern and related imports/refs are cleanly removed. |
| apps/desktop/src/renderer/stores/add-repository-modal.ts | Zustand store simplified: folderImportTrigger counter, triggerFolderImport action, and related selectors removed. Only new-project modal state remains. |
Sequence Diagram
sequenceDiagram
participant User
participant DropdownMenuItem
participant DashboardSidebarHeader
participant useFolderFirstImport
participant FolderFirstImportModal
Note over DashboardSidebarHeader,useFolderFirstImport: Before (broken): counter in Zustand + useEffect in AddRepositoryModals
Note over DashboardSidebarHeader,useFolderFirstImport: After (fixed): direct call on user click
User->>DropdownMenuItem: clicks "Import existing folder"
DropdownMenuItem->>DashboardSidebarHeader: onSelect → handleImportFolder()
DashboardSidebarHeader->>useFolderFirstImport: folderImport.start()
useFolderFirstImport->>User: Native directory picker opens
User->>useFolderFirstImport: picks folder (or cancels)
alt Cancelled
useFolderFirstImport-->>DashboardSidebarHeader: returns (no-op)
else No matching project
useFolderFirstImport->>FolderFirstImportModal: state → no-match
FolderFirstImportModal->>User: "Create as new" dialog
User->>FolderFirstImportModal: confirms name
FolderFirstImportModal->>useFolderFirstImport: confirmCreateAsNew()
useFolderFirstImport-->>DashboardSidebarHeader: onSuccess toast
else Single match
useFolderFirstImport->>useFolderFirstImport: runSetup()
useFolderFirstImport-->>DashboardSidebarHeader: onSuccess toast
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx
Line: 50-55
Comment:
**Redundant `.catch()` on non-rejecting promise**
`folderImport.start()` is an `async` function that catches every error path internally (all three `try/catch` blocks in the hook return early after calling `reportError`, never re-throwing). This means `start()` will always resolve, never reject — making the `.catch()` in `handleImportFolder` unreachable in normal and error flows alike.
If `start()` did somehow throw unexpectedly, the user would see **two** error toasts: one from the `onError` callback registered with `useFolderFirstImport`, and a second from this `.catch()`. Consider removing the outer catch and relying solely on the `onError` option already passed to the hook, keeping the code DRY:
```suggestion
const handleImportFolder = () => {
void folderImport.start();
};
```
If you prefer to keep it as a defensive belt-and-suspenders guard, that's also fine — just be aware of the potential double-toast if the hook's internal error handling ever changes.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): stop spurious folder picke..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const handleImportFolder = () => { | ||
| folderImport.start().catch((err) => { | ||
| toast.error( | ||
| `Import failed: ${err instanceof Error ? err.message : String(err)}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
.catch() on non-rejecting promise
folderImport.start() is an async function that catches every error path internally (all three try/catch blocks in the hook return early after calling reportError, never re-throwing). This means start() will always resolve, never reject — making the .catch() in handleImportFolder unreachable in normal and error flows alike.
If start() did somehow throw unexpectedly, the user would see two error toasts: one from the onError callback registered with useFolderFirstImport, and a second from this .catch(). Consider removing the outer catch and relying solely on the onError option already passed to the hook, keeping the code DRY:
| const handleImportFolder = () => { | |
| folderImport.start().catch((err) => { | |
| toast.error( | |
| `Import failed: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| }); | |
| const handleImportFolder = () => { | |
| void folderImport.start(); | |
| }; |
If you prefer to keep it as a defensive belt-and-suspenders guard, that's also fine — just be aware of the potential double-toast if the hook's internal error handling ever changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx
Line: 50-55
Comment:
**Redundant `.catch()` on non-rejecting promise**
`folderImport.start()` is an `async` function that catches every error path internally (all three `try/catch` blocks in the hook return early after calling `reportError`, never re-throwing). This means `start()` will always resolve, never reject — making the `.catch()` in `handleImportFolder` unreachable in normal and error flows alike.
If `start()` did somehow throw unexpectedly, the user would see **two** error toasts: one from the `onError` callback registered with `useFolderFirstImport`, and a second from this `.catch()`. Consider removing the outer catch and relying solely on the `onError` option already passed to the hook, keeping the code DRY:
```suggestion
const handleImportFolder = () => {
void folderImport.start();
};
```
If you prefer to keep it as a defensive belt-and-suspenders guard, that's also fine — just be aware of the potential double-toast if the hook's internal error handling ever changes.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx (1)
25-26: Move folder-import internals out ofAddRepositoryModals.This component now owns the folder-first flow, but it still imports the modal/hook from the old
AddRepositoryModalssubtree. Please relocate them underDashboardSidebarHeaderif single-use, or a shared dashboard-level folder if reused.♻️ Suggested import shape after relocating
-import { FolderFirstImportModal } from "renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/components/FolderFirstImportModal"; -import { useFolderFirstImport } from "renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/hooks/useFolderFirstImport"; +import { FolderFirstImportModal } from "./components/FolderFirstImportModal"; +import { useFolderFirstImport } from "./hooks/useFolderFirstImport";As per coding guidelines, “Co-locate dependencies (utils, hooks, constants, config, tests, stories) next to the file using them. If a utility is used once, nest it under the parent's directory. If used 2+ times, promote it to the highest shared parent's directory or top-level
components/.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx` around lines 25 - 26, The DashboardSidebarHeader component currently imports FolderFirstImportModal and useFolderFirstImport from the AddRepositoryModals subtree; move these two single-use internals into the DashboardSidebarHeader directory (or into a shared dashboard-level folder if they are used elsewhere), update their file paths and imports inside DashboardSidebarHeader to the new local locations, and ensure any related tests/stories/constants are co-located as well; specifically relocate the FolderFirstImportModal component and the useFolderFirstImport hook and update all imports referencing these symbols so DashboardSidebarHeader imports them from the new local/shared dashboard path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx`:
- Around line 25-26: The DashboardSidebarHeader component currently imports
FolderFirstImportModal and useFolderFirstImport from the AddRepositoryModals
subtree; move these two single-use internals into the DashboardSidebarHeader
directory (or into a shared dashboard-level folder if they are used elsewhere),
update their file paths and imports inside DashboardSidebarHeader to the new
local locations, and ensure any related tests/stories/constants are co-located
as well; specifically relocate the FolderFirstImportModal component and the
useFolderFirstImport hook and update all imports referencing these symbols so
DashboardSidebarHeader imports them from the new local/shared dashboard path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95d4fca5-5dab-4a81-abeb-1dcc7e4352aa
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/AddRepositoryModals/AddRepositoryModals.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsxapps/desktop/src/renderer/stores/add-repository-modal.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/stores/add-repository-modal.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
useEffectinAddRepositoryModals. Settings lives outside_dashboard, so returning to the dashboard remountedAddRepositoryModals, and the effect fired again with the already-incremented counter.DashboardSidebarHeadernow ownsuseFolderFirstImportand rendersFolderFirstImportModal; the dropdown item callsfolderImport.start()directly on click.Why / Context
The "pulse via counter + effect" pattern is the wrong shape for "invoke this imperative thing on user click."
useEffectfires on every mount regardless of whether the dep actually changed since last render — it has no memory of the prior mount's value. A module-scoped counter that outlives the component is therefore indistinguishable from an increment on remount, and the existing=== 0guard only caught the pristine initial-load case.The only reason the indirection existed was that the header didn't have a reference to the hook instance owned by
AddRepositoryModals. Collapsing both into the header removes that coupling entirely.How It Works
DashboardSidebarHeadercallsuseFolderFirstImport({ onSuccess, onError })directly and renders<FolderFirstImportModal>(Dialog portals, so DOM position doesn't matter).onSelect={handleImportFolder}, which callsfolderImport.start()and toasts on rejection.AddRepositoryModalsnow only ownsNewProjectModal.folderImportTrigger,triggerFolderImport,useFolderImportTrigger,useTriggerFolderImportremoved from the zustand store.Manual QA Checklist
NewProjectModal.Testing
bun run typecheck✅Summary by cubic
Fixes the folder picker opening unexpectedly when returning from Settings to the dashboard. The folder import flow is now triggered directly on click in the sidebar header, so remounts no longer reopen the picker.
Bug Fixes
useEffecttrigger with directfolderImport.start()on dropdown item click.useFolderFirstImportandFolderFirstImportModalinto the dashboard sidebar header.Refactors
AddRepositoryModalsnow only rendersNewProjectModal.folderImportTriggerand related selectors from theadd-repository-modalstore.folderImport.start(); errors are handled by the hook.Written for commit 30a9b0e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor