Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 67 additions & 1 deletion apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ipcMain, BrowserWindow, shell, app } from 'electron';
import { IPC_CHANNELS, AUTO_BUILD_PATHS, DEFAULT_APP_SETTINGS, DEFAULT_FEATURE_MODELS, DEFAULT_FEATURE_THINKING, MODEL_ID_MAP, THINKING_BUDGET_MAP } from '../../../shared/constants';
import { IPC_CHANNELS, AUTO_BUILD_PATHS, DEFAULT_APP_SETTINGS, DEFAULT_FEATURE_MODELS, DEFAULT_FEATURE_THINKING, MODEL_ID_MAP, THINKING_BUDGET_MAP, getSpecsDir } from '../../../shared/constants';
import type { IPCResult, WorktreeStatus, WorktreeDiff, WorktreeDiffFile, WorktreeMergeResult, WorktreeDiscardResult, WorktreeListResult, WorktreeListItem, SupportedIDE, SupportedTerminal, AppSettings } from '../../../shared/types';
import path from 'path';
import { existsSync, readdirSync, statSync, readFileSync } from 'fs';
Expand Down Expand Up @@ -2536,4 +2536,70 @@
}
}
);

/**
* Clear the staged state for a task
* This allows the user to re-stage changes if needed
*/
ipcMain.handle(
IPC_CHANNELS.TASK_CLEAR_STAGED_STATE,
async (_, taskId: string): Promise<IPCResult<{ cleared: boolean }>> => {
try {
const { task, project } = findTaskAndProject(taskId);
if (!task || !project) {
return { success: false, error: 'Task not found' };
}

const specsBaseDir = getSpecsDir(project.autoBuildPath);
const specDir = path.join(project.path, specsBaseDir, task.specId);
const planPath = path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN);

if (!existsSync(planPath)) {
return { success: false, error: 'Implementation plan not found' };
}

// Read, update, and write the plan file
const { promises: fsPromises } = require('fs');
const planContent = await fsPromises.readFile(planPath, 'utf-8');
const plan = JSON.parse(planContent);

// Clear the staged state flags
delete plan.stagedInMainProject;
delete plan.stagedAt;
plan.updated_at = new Date().toISOString();

await fsPromises.writeFile(planPath, JSON.stringify(plan, null, 2));

// Also update worktree plan if it exists
const worktreePath = findTaskWorktree(project.path, task.specId);
if (worktreePath) {
const worktreePlanPath = path.join(worktreePath, specsBaseDir, task.specId, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN);
if (existsSync(worktreePlanPath)) {
try {
const worktreePlanContent = await fsPromises.readFile(worktreePlanPath, 'utf-8');
const worktreePlan = JSON.parse(worktreePlanContent);
delete worktreePlan.stagedInMainProject;
delete worktreePlan.stagedAt;
worktreePlan.updated_at = new Date().toISOString();
await fsPromises.writeFile(worktreePlanPath, JSON.stringify(worktreePlan, null, 2));
} catch (e) {
// Non-fatal - worktree plan update is best-effort
console.warn('[CLEAR_STAGED_STATE] Failed to update worktree plan:', e);
}
}
}
Comment on lines 2873 to 2911
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This block has a couple of areas for improvement:

  1. Code Duplication: The logic to read, update, and write the plan file is repeated for the main project and the worktree. This can be extracted into a helper function to improve readability and maintainability.
  2. Module Import: fs/promises is being required inline. For consistency, it's better to import it at the top of the file with the other fs modules.

Here's an example of how you could define a helper function for this:

async function clearStagedFlagsInPlan(planPath: string) {
  const planContent = await fsPromises.readFile(planPath, 'utf-8');
  const plan = JSON.parse(planContent);

  delete plan.stagedInMainProject;
  delete plan.stagedAt;
  plan.updated_at = new Date().toISOString();

  await fsPromises.writeFile(planPath, JSON.stringify(plan, null, 2));
}


// Invalidate tasks cache to force reload
projectStore.invalidateTasksCache(project.id);

return { success: true, data: { cleared: true } };
} catch (error) {
console.error('Failed to clear staged state:', error);
return {
success: false,
error: error instanceof Error ? error.message : 'Failed to clear staged state'
};
}
}
);
}
4 changes: 4 additions & 0 deletions apps/frontend/src/preload/api/task-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export interface TaskAPI {
mergeWorktree: (taskId: string, options?: { noCommit?: boolean }) => Promise<IPCResult<import('../../shared/types').WorktreeMergeResult>>;
mergeWorktreePreview: (taskId: string) => Promise<IPCResult<import('../../shared/types').WorktreeMergeResult>>;
discardWorktree: (taskId: string) => Promise<IPCResult<import('../../shared/types').WorktreeDiscardResult>>;
clearStagedState: (taskId: string) => Promise<IPCResult<{ cleared: boolean }>>;
listWorktrees: (projectId: string) => Promise<IPCResult<import('../../shared/types').WorktreeListResult>>;
worktreeOpenInIDE: (worktreePath: string, ide: SupportedIDE, customPath?: string) => Promise<IPCResult<{ opened: boolean }>>;
worktreeOpenInTerminal: (worktreePath: string, terminal: SupportedTerminal, customPath?: string) => Promise<IPCResult<{ opened: boolean }>>;
Expand Down Expand Up @@ -142,6 +143,9 @@ export const createTaskAPI = (): TaskAPI => ({
discardWorktree: (taskId: string): Promise<IPCResult<import('../../shared/types').WorktreeDiscardResult>> =>
ipcRenderer.invoke(IPC_CHANNELS.TASK_WORKTREE_DISCARD, taskId),

clearStagedState: (taskId: string): Promise<IPCResult<{ cleared: boolean }>> =>
ipcRenderer.invoke(IPC_CHANNELS.TASK_CLEAR_STAGED_STATE, taskId),

listWorktrees: (projectId: string): Promise<IPCResult<import('../../shared/types').WorktreeListResult>> =>
ipcRenderer.invoke(IPC_CHANNELS.TASK_LIST_WORKTREES, projectId),

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ function TaskDetailModalContent({ open, task, onOpenChange, onSwitchToTerminals,
onClose={handleClose}
onSwitchToTerminals={onSwitchToTerminals}
onOpenInbuiltTerminal={onOpenInbuiltTerminal}
onReviewAgain={state.handleReviewAgain}
/>
</>
)}
Expand Down
29 changes: 19 additions & 10 deletions apps/frontend/src/renderer/components/task-detail/TaskReview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ interface TaskReviewProps {
onClose?: () => void;
onSwitchToTerminals?: () => void;
onOpenInbuiltTerminal?: (id: string, cwd: string) => void;
onReviewAgain?: () => void;
}

/**
Expand Down Expand Up @@ -83,7 +84,8 @@ export function TaskReview({
onLoadMergePreview,
onClose,
onSwitchToTerminals,
onOpenInbuiltTerminal
onOpenInbuiltTerminal,
onReviewAgain
}: TaskReviewProps) {
return (
<div className="space-y-4">
Expand All @@ -98,10 +100,24 @@ export function TaskReview({
/>
)}

{/* Workspace Status - hide if staging was successful (worktree is deleted after staging) */}
{/* Workspace Status - priority: loading > fresh staging success > already staged (persisted) > worktree exists > no workspace */}
{isLoadingWorktree ? (
<LoadingMessage />
) : worktreeStatus?.exists && !stagedSuccess ? (
) : stagedSuccess ? (
/* Fresh staging just completed - StagedSuccessMessage is rendered above */
null
) : task.stagedInMainProject ? (
/* Task was previously staged (persisted state) - show even if worktree still exists */
<StagedInProjectMessage
task={task}
projectPath={stagedProjectPath}
hasWorktree={worktreeStatus?.exists || false}
worktreeStatus={worktreeStatus}
onClose={onClose}
onReviewAgain={onReviewAgain}
/>
) : worktreeStatus?.exists ? (
/* Worktree exists but not yet staged - show staging UI */
<WorkspaceStatus
worktreeStatus={worktreeStatus}
workspaceError={workspaceError}
Expand All @@ -120,13 +136,6 @@ export function TaskReview({
onSwitchToTerminals={onSwitchToTerminals}
onOpenInbuiltTerminal={onOpenInbuiltTerminal}
/>
) : task.stagedInMainProject && !stagedSuccess ? (
<StagedInProjectMessage
task={task}
projectPath={stagedProjectPath}
hasWorktree={worktreeStatus?.exists || false}
onClose={onClose}
/>
) : (
<NoWorkspaceMessage task={task} onClose={onClose} />
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,37 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) {
}
}, [task.id]);

// Handle "Review Again" - clears staged state and reloads worktree info
const handleReviewAgain = useCallback(async () => {
// Clear staged success state if it was set in this session
setStagedSuccess(null);
setStagedProjectPath(undefined);
setSuggestedCommitMessage(undefined);

// Reset merge preview to force re-check
setMergePreview(null);
hasLoadedPreviewRef.current = null;

// Reload worktree status
setIsLoadingWorktree(true);
try {
const [statusResult, diffResult] = await Promise.all([
window.electronAPI.getWorktreeStatus(task.id),
window.electronAPI.getWorktreeDiff(task.id)
]);
if (statusResult.success && statusResult.data) {
setWorktreeStatus(statusResult.data);
}
if (diffResult.success && diffResult.data) {
setWorktreeDiff(diffResult.data);
}
} catch (err) {
console.error('Failed to reload worktree info:', err);
} finally {
setIsLoadingWorktree(false);
}
}, [task.id]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider surfacing reload errors to the UI.

The handleReviewAgain handler logs errors to console but doesn't expose them to the user. If the worktree status/diff reload fails, the user has no indication of the failure.

Consider adding an error state or returning a success/failure indicator:

♻️ Optional: Surface errors to calling component
   // Handle "Review Again" - clears staged state and reloads worktree info
-  const handleReviewAgain = useCallback(async () => {
+  const handleReviewAgain = useCallback(async (): Promise<{ success: boolean; error?: string }> => {
     // Clear staged success state if it was set in this session
     setStagedSuccess(null);
     setStagedProjectPath(undefined);
     setSuggestedCommitMessage(undefined);
     
     // Reset merge preview to force re-check
     setMergePreview(null);
     hasLoadedPreviewRef.current = null;
     
     // Reload worktree status
     setIsLoadingWorktree(true);
     try {
       const [statusResult, diffResult] = await Promise.all([
         window.electronAPI.getWorktreeStatus(task.id),
         window.electronAPI.getWorktreeDiff(task.id)
       ]);
       if (statusResult.success && statusResult.data) {
         setWorktreeStatus(statusResult.data);
       }
       if (diffResult.success && diffResult.data) {
         setWorktreeDiff(diffResult.data);
       }
+      return { success: true };
     } catch (err) {
       console.error('Failed to reload worktree info:', err);
+      return { success: false, error: err instanceof Error ? err.message : 'Failed to reload' };
     } finally {
       setIsLoadingWorktree(false);
     }
   }, [task.id]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Handle "Review Again" - clears staged state and reloads worktree info
const handleReviewAgain = useCallback(async () => {
// Clear staged success state if it was set in this session
setStagedSuccess(null);
setStagedProjectPath(undefined);
setSuggestedCommitMessage(undefined);
// Reset merge preview to force re-check
setMergePreview(null);
hasLoadedPreviewRef.current = null;
// Reload worktree status
setIsLoadingWorktree(true);
try {
const [statusResult, diffResult] = await Promise.all([
window.electronAPI.getWorktreeStatus(task.id),
window.electronAPI.getWorktreeDiff(task.id)
]);
if (statusResult.success && statusResult.data) {
setWorktreeStatus(statusResult.data);
}
if (diffResult.success && diffResult.data) {
setWorktreeDiff(diffResult.data);
}
} catch (err) {
console.error('Failed to reload worktree info:', err);
} finally {
setIsLoadingWorktree(false);
}
}, [task.id]);
// Handle "Review Again" - clears staged state and reloads worktree info
const handleReviewAgain = useCallback(async (): Promise<{ success: boolean; error?: string }> => {
// Clear staged success state if it was set in this session
setStagedSuccess(null);
setStagedProjectPath(undefined);
setSuggestedCommitMessage(undefined);
// Reset merge preview to force re-check
setMergePreview(null);
hasLoadedPreviewRef.current = null;
// Reload worktree status
setIsLoadingWorktree(true);
try {
const [statusResult, diffResult] = await Promise.all([
window.electronAPI.getWorktreeStatus(task.id),
window.electronAPI.getWorktreeDiff(task.id)
]);
if (statusResult.success && statusResult.data) {
setWorktreeStatus(statusResult.data);
}
if (diffResult.success && diffResult.data) {
setWorktreeDiff(diffResult.data);
}
return { success: true };
} catch (err) {
console.error('Failed to reload worktree info:', err);
return { success: false, error: err instanceof Error ? err.message : 'Failed to reload' };
} finally {
setIsLoadingWorktree(false);
}
}, [task.id]);
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts
around lines 241 - 270, The handler handleReviewAgain currently swallows reload
errors (only console.error); add a user-visible error path by introducing an
error state (e.g., worktreeReloadError via useState) or by returning a
success/failure boolean from handleReviewAgain so callers can react; update the
catch block to setWorktreeReloadError(err) (or set the error return value) and
ensure finally still calls setIsLoadingWorktree(false); also clear the error at
the start of the function (setWorktreeReloadError(null)) so repeated calls reset
the UI; keep existing updates to setWorktreeStatus and setWorktreeDiff on
success.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing workspaceError reset in handleReviewAgain causes stale errors

Low Severity

The handleReviewAgain function resets several state variables (stagedSuccess, stagedProjectPath, suggestedCommitMessage, mergePreview) but doesn't reset workspaceError. The original worktree load effect at line 124 clears workspaceError before reloading, but handleReviewAgain omits this. If a user has a merge/discard error and then clicks "Review Again", the stale error message would continue to display in the WorkspaceStatus component even though the state is being refreshed.

Fix in Cursor Fix in Web


// NOTE: Merge preview is NO LONGER auto-loaded on modal open.
// User must click "Check for Conflicts" button to trigger the expensive preview operation.
// This improves modal open performance significantly (avoids 1-30+ second Python subprocess).
Expand Down Expand Up @@ -318,5 +349,6 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) {
handleLogsScroll,
togglePhase,
loadMergePreview,
handleReviewAgain,
};
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { AlertCircle, GitMerge, Loader2, Trash2, Check } from 'lucide-react';
import { AlertCircle, GitMerge, Loader2, Trash2, Check, RotateCcw } from 'lucide-react';
import { useState } from 'react';
import { Button } from '../../ui/button';
import { persistTaskStatus } from '../../../stores/task-store';
import type { Task } from '../../../../shared/types';
import type { Task, WorktreeStatus } from '../../../../shared/types';

interface LoadingMessageProps {
message?: string;
Expand Down Expand Up @@ -88,14 +88,18 @@
task: Task;
projectPath?: string;
hasWorktree?: boolean;
worktreeStatus?: WorktreeStatus | null;
onClose?: () => void;
onReviewAgain?: () => void;
}

/**
* Displays message when changes have already been staged in the main project
*/
export function StagedInProjectMessage({ task, projectPath, hasWorktree = false, onClose }: StagedInProjectMessageProps) {
export function StagedInProjectMessage({ task, projectPath, hasWorktree = false, worktreeStatus, onClose, onReviewAgain }: StagedInProjectMessageProps) {
const [isDeleting, setIsDeleting] = useState(false);
const [isMarkingDone, setIsMarkingDone] = useState(false);
const [isResetting, setIsResetting] = useState(false);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing skipStatusChange causes task status race condition

Medium Severity

The handleDeleteWorktreeAndMarkDone function calls discardWorktree(task.id) without passing skipStatusChange=true. Since the handler sends a TASK_STATUS_CHANGE event with status 'backlog' when skipStatusChange is not true, and then persistTaskStatus(task.id, 'done') is immediately called, there's a race condition. The task status will briefly change to 'backlog' before being set to 'done', causing UI flickering and unnecessary state transitions. The KanbanBoard.tsx correctly passes true for the same use case at line 472.

Fix in Cursor Fix in Web

const [error, setError] = useState<string | null>(null);

const handleDeleteWorktreeAndMarkDone = async () => {
Expand Down Expand Up @@ -124,6 +128,46 @@
}
};

const handleMarkDoneOnly = async () => {
setIsMarkingDone(true);
setError(null);

try {
await persistTaskStatus(task.id, 'done');
onClose?.();
} catch (err) {
console.error('Error marking task as done:', err);
setError(err instanceof Error ? err.message : 'Failed to mark as done');
} finally {
setIsMarkingDone(false);
}
};

const handleReviewAgain = async () => {
if (!onReviewAgain) return;

setIsResetting(true);
setError(null);

try {
// Clear the staged flag via IPC
const result = await window.electronAPI.clearStagedState(task.id);

if (!result.success) {
setError(result.error || 'Failed to reset staged state');
return;
}

// Trigger re-render by calling parent callback
onReviewAgain();
} catch (err) {
console.error('Error resetting staged state:', err);
setError(err instanceof Error ? err.message : 'Failed to reset staged state');
} finally {
setIsResetting(false);
}
};
Comment on lines +145 to +168
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There's a potential race condition here. After clearStagedState succeeds, it invalidates the backend cache, but the UI's task object isn't immediately updated. onReviewAgain() refreshes local UI state but might do so with a stale task prop. To ensure the UI reflects the change correctly, you should explicitly reload the tasks for the project after the IPC call succeeds. This will update the store and provide the component with fresh data.

You'll need to import loadTasks from ../../../stores/task-store.

  const handleReviewAgain = async () => {
    if (!onReviewAgain) return;
    
    setIsResetting(true);
    setError(null);

    try {
      // Clear the staged flag via IPC
      const result = await window.electronAPI.clearStagedState(task.id);
      
      if (!result.success) {
        setError(result.error || 'Failed to reset staged state');
        return;
      }

      // Trigger re-render by calling parent callback
      onReviewAgain();
      // Force a reload of tasks to get the updated `stagedInMainProject` flag
      await loadTasks(task.projectId);
    } catch (err) {
      console.error('Error resetting staged state:', err);
      setError(err instanceof Error ? err.message : 'Failed to reset staged state');
    } finally {
      setIsResetting(false);
    }
  };


return (
<div className="rounded-xl border border-success/30 bg-success/10 p-4">
<h3 className="font-medium text-sm text-foreground mb-2 flex items-center gap-2">
Expand All @@ -143,12 +187,13 @@
</div>

{/* Action buttons */}
{hasWorktree && (
<div className="flex flex-col gap-2">
<div className="flex gap-2">
<div className="flex flex-col gap-2">
<div className="flex gap-2">
{/* Primary action: Mark Done or Delete Worktree & Mark Done */}
{hasWorktree ? (
<Button
onClick={handleDeleteWorktreeAndMarkDone}
disabled={isDeleting}
disabled={isDeleting || isMarkingDone || isResetting}
size="sm"
variant="default"
className="flex-1"
Expand All @@ -165,15 +210,88 @@
</>
)}
</Button>
</div>
{error && (
<p className="text-xs text-destructive">{error}</p>
) : (
<Button
onClick={handleMarkDoneOnly}
disabled={isDeleting || isMarkingDone || isResetting}
size="sm"
variant="default"
className="flex-1"
>
{isMarkingDone ? (
<>
<Loader2 className="h-4 w-4 mr-2 animate-spin" />
Marking done...
</>
) : (
<>
<Check className="h-4 w-4 mr-2" />
Mark as Done
</>
)}
</Button>
)}
</div>

{/* Secondary actions row */}
<div className="flex gap-2">
{/* Mark Done Only (when worktree exists) - allows keeping worktree */}
{hasWorktree && (
<Button
onClick={handleMarkDoneOnly}
disabled={isDeleting || isMarkingDone || isResetting}
size="sm"
variant="outline"
className="flex-1"
>
{isMarkingDone ? (
<>
<Loader2 className="h-4 w-4 mr-2 animate-spin" />
Marking done...
</>
) : (
<>
<Check className="h-4 w-4 mr-2" />
Mark Done Only
</>
Comment on lines +252 to +255
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

New user-facing strings should use i18n translation keys.

This PR introduces new hardcoded user-facing strings that should use the useTranslation() hook per the coding guidelines. New strings include:

  • "Mark Done Only" (line 255)
  • "Review Again" (line 278)
  • "Marking done..." (line 224, 249)
  • "Resetting..." (line 273)

As per coding guidelines for apps/frontend/src/**/*.{ts,tsx}: "Use useTranslation() hook with namespace prefixes for accessing translation strings."

🌐 Example i18n fix for new strings
+import { useTranslation } from 'react-i18next';

 export function StagedInProjectMessage({ task, projectPath, hasWorktree = false, worktreeStatus, onClose, onReviewAgain }: StagedInProjectMessageProps) {
+  const { t } = useTranslation('taskReview');
   const [isDeleting, setIsDeleting] = useState(false);
   // ... rest of state

   // In the JSX:
-  Mark Done Only
+  {t('actions.markDoneOnly')}

-  Review Again
+  {t('actions.reviewAgain')}

-  Marking done...
+  {t('actions.markingDone')}

-  Resetting...
+  {t('actions.resetting')}

Also applies to: 276-279

)}
</Button>
)}

{/* Review Again button - only show if worktree exists and callback provided */}
{hasWorktree && onReviewAgain && (
<Button
onClick={handleReviewAgain}
disabled={isDeleting || isMarkingDone || isResetting}
size="sm"
variant="outline"
className="flex-1"
>
{isResetting ? (
<>
<Loader2 className="h-4 w-4 mr-2 animate-spin" />
Resetting...
</>
) : (
<>
<RotateCcw className="h-4 w-4 mr-2" />
Review Again
</>
)}
</Button>
)}
</div>

{error && (
<p className="text-xs text-destructive">{error}</p>
)}

{hasWorktree && (
<p className="text-xs text-muted-foreground">
This will delete the isolated workspace and mark the task as complete.
"Delete Worktree & Mark Done" cleans up the isolated workspace. "Mark Done Only" keeps it for reference.
</p>
</div>
)}
)}
</div>
</div>
);
}
Loading
Loading