fix: await removeProject to prevent race condition on project re-add (fixes #1910)#1998
Conversation
📝 WalkthroughWalkthroughThe change adds async state tracking for project removal operations in the App component. The remove-project dialog now waits for the removal operation to complete, disables buttons during removal, and only closes on successful removal with error feedback otherwise. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
|
octo-patch
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a loading state and asynchronous handling for project removal to prevent race conditions. It ensures the backend operation completes before the UI updates and disables action buttons during the process. A review comment suggests removing a redundant namespace prefix in a translation key for consistency.
| setShowRemoveProjectDialog(false); | ||
| setProjectToRemove(null); | ||
| } else { | ||
| setRemoveProjectError(t('dialogs:removeProject.error')); |
There was a problem hiding this comment.
The useTranslation hook is initialized with the dialogs namespace on line 314. Including the dialogs: prefix here is redundant and inconsistent with other translation calls in this file (e.g., lines 1137 and 1140).
| setRemoveProjectError(t('dialogs:removeProject.error')); | |
| setRemoveProjectError(t('removeProject.error')); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/App.tsx (1)
696-701: 🧹 Nitpick | 🔵 TrivialMinor edge case: canceling during removal may cause inconsistent state.
If the user presses Escape while removal is in progress (the dialog's
onOpenChangecallshandleCancelRemoveProject),isRemovingProjectresets tofalsebut the async operation continues in the background. This is unlikely to cause problems in practice since the main race condition (re-adding project) is now blocked by awaiting the removal.Consider disabling dialog dismissal while
isRemovingProjectis true:🔧 Optional: Prevent dialog dismissal during removal
- <Dialog open={showRemoveProjectDialog} onOpenChange={(open) => { - if (!open) handleCancelRemoveProject(); + <Dialog open={showRemoveProjectDialog} onOpenChange={(open) => { + if (!open && !isRemovingProject) handleCancelRemoveProject(); }}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/App.tsx` around lines 696 - 701, Prevent the dialog from being dismissed while an async removal is in progress by making the dialog ignore outside clicks/Escape when isRemovingProject is true: update the dialog's open/onOpenChange handling so that onOpenChange (which currently calls handleCancelRemoveProject) returns early or is a no-op if isRemovingProject is true, and/or pass a prop to the Dialog component to disable dismissal when isRemovingProject is true; keep handleCancelRemoveProject as the cancel path when not removing (it should continue to call setShowRemoveProjectDialog(false), setProjectToRemove(null), setRemoveProjectError(null), setIsRemovingProject(false)).
🤖 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/renderer/App.tsx`:
- Around line 1135-1141: Add a loading indicator to the destructive "Remove"
button so users see progress when isRemovingProject is true: in the DialogFooter
where the Button with onClick={handleConfirmRemoveProject} and
disabled={isRemovingProject} is rendered, show the same spinner icon/visual used
by the Initialize button when isInitializing is true (condition on
isRemovingProject) and swap the button label to the new translation key
removeProject.removing while keeping the existing removeProject.remove for the
idle state; ensure to add the removeProject.removing translation entry and
preserve the disabled prop and onClick handler (handleConfirmRemoveProject).
---
Outside diff comments:
In `@apps/desktop/src/renderer/App.tsx`:
- Around line 696-701: Prevent the dialog from being dismissed while an async
removal is in progress by making the dialog ignore outside clicks/Escape when
isRemovingProject is true: update the dialog's open/onOpenChange handling so
that onOpenChange (which currently calls handleCancelRemoveProject) returns
early or is a no-op if isRemovingProject is true, and/or pass a prop to the
Dialog component to disable dismissal when isRemovingProject is true; keep
handleCancelRemoveProject as the cancel path when not removing (it should
continue to call setShowRemoveProjectDialog(false), setProjectToRemove(null),
setRemoveProjectError(null), setIsRemovingProject(false)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e56fa00-3813-44f9-9d52-be580364a1cd
📒 Files selected for processing (1)
apps/desktop/src/renderer/App.tsx
| <DialogFooter> | ||
| <Button variant="outline" onClick={handleCancelRemoveProject}> | ||
| <Button variant="outline" onClick={handleCancelRemoveProject} disabled={isRemovingProject}> | ||
| {t('removeProject.cancel')} | ||
| </Button> | ||
| <Button variant="destructive" onClick={handleConfirmRemoveProject}> | ||
| <Button variant="destructive" onClick={handleConfirmRemoveProject} disabled={isRemovingProject}> | ||
| {t('removeProject.remove')} | ||
| </Button> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a loading indicator to the Remove button.
The buttons are correctly disabled during removal, but unlike the Initialize button (which shows a spinner when isInitializing), the Remove button provides no visual feedback that an operation is in progress. This is a minor UX inconsistency.
💅 Optional: Add loading spinner for consistency
- <Button variant="destructive" onClick={handleConfirmRemoveProject} disabled={isRemovingProject}>
- {t('removeProject.remove')}
+ <Button variant="destructive" onClick={handleConfirmRemoveProject} disabled={isRemovingProject}>
+ {isRemovingProject ? (
+ <>
+ <RefreshCw className="mr-2 h-4 w-4 animate-spin" />
+ {t('removeProject.removing')}
+ </>
+ ) : (
+ t('removeProject.remove')
+ )}
</Button>Note: This would require adding a removeProject.removing translation key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/App.tsx` around lines 1135 - 1141, Add a loading
indicator to the destructive "Remove" button so users see progress when
isRemovingProject is true: in the DialogFooter where the Button with
onClick={handleConfirmRemoveProject} and disabled={isRemovingProject} is
rendered, show the same spinner icon/visual used by the Initialize button when
isInitializing is true (condition on isRemovingProject) and swap the button
label to the new translation key removeProject.removing while keeping the
existing removeProject.remove for the idle state; ensure to add the
removeProject.removing translation entry and preserve the disabled prop and
onClick handler (handleConfirmRemoveProject).
2 participants
Fixes #1910
Problem
After closing a project tab and immediately re-adding the same directory, the UI shows up completely empty — no Kanban cards, no roadmap.
The root cause is a race condition in
handleConfirmRemoveProject:removeProject()was called withoutawait, so the dialog closed immediately. If the user quickly re-added the same directory,addProjectIPC could arrive at the main process beforeremoveProjectcompleted — so the backend still found the old project and returned the old project ID. When removal finally resolved, it calledstore.removeProject(oldId)+store.closeProjectTab(oldId), wiping all tasks from state and leaving the UI empty.Solution
Make
handleConfirmRemoveProjectasync and awaitremoveProject(). The dialog stays open until removal fully completes, eliminating the race window. Also disable dialog buttons while removal is in progress to prevent double-submission.Testing
Summary by CodeRabbit
Bug Fixes