From 35c6beca37332e92ba655b4ac5a0b5b4e5e5bcc2 Mon Sep 17 00:00:00 2001 From: Cody Seibert Date: Wed, 17 Dec 2025 23:24:54 -0500 Subject: [PATCH] fix: address PR 161 review comments - Fix unknown status bypassing worktree filtering in use-board-column-features.ts - Remove unused props projectPath and onWorktreeCreated from use-board-drag-drop.ts - Fix test expecting worktreePath during edit (worktrees created at execution time) - Remove unused setAutoModeRunning from dependency array - Remove unused imports (BranchAutocomplete, cn) - Fix htmlFor accessibility issue in branch-selector.tsx - Remove empty finally block in resume-feature.ts - Remove duplicate setTimeout state reset in create-pr-dialog.tsx - Consolidate duplicate state reset logic in context-view.tsx - Simplify branch name defaulting logic in use-board-actions.ts - Fix branchName reset to null when worktree is deleted --- apps/app/src/components/views/board-view.tsx | 53 ++++++++++--------- .../board-view/dialogs/add-feature-dialog.tsx | 1 - .../board-view/dialogs/create-pr-dialog.tsx | 34 ++++++------ .../dialogs/edit-feature-dialog.tsx | 1 - .../board-view/hooks/use-board-actions.ts | 8 +-- .../hooks/use-board-column-features.ts | 4 +- .../board-view/hooks/use-board-drag-drop.ts | 4 -- .../board-view/shared/branch-selector.tsx | 4 +- .../app/src/components/views/context-view.tsx | 14 ++--- apps/app/src/hooks/use-auto-mode.ts | 1 - apps/app/tests/worktree-integration.spec.ts | 18 ++++--- .../routes/auto-mode/routes/resume-feature.ts | 3 +- 12 files changed, 67 insertions(+), 78 deletions(-) diff --git a/apps/app/src/components/views/board-view.tsx b/apps/app/src/components/views/board-view.tsx index ec38e5f0..ce86ea26 100644 --- a/apps/app/src/components/views/board-view.tsx +++ b/apps/app/src/components/views/board-view.tsx @@ -412,6 +412,12 @@ export function BoardView() { autoModeRunningRef.current = autoMode.isRunning; }, [autoMode.isRunning]); + // Use a ref to track the latest features to avoid effect re-runs when features change + const hookFeaturesRef = useRef(hookFeatures); + useEffect(() => { + hookFeaturesRef.current = hookFeatures; + }, [hookFeatures]); + // Track features that are pending (started but not yet confirmed running) const pendingFeaturesRef = useRef>(new Set()); @@ -489,33 +495,30 @@ export function BoardView() { } // Filter backlog features by the currently selected worktree branch - const primaryBranch = currentProject.path - ? getPrimaryWorktreeBranch(currentProject.path) - : null; - const backlogFeatures = hookFeatures.filter((f) => { + // This logic mirrors use-board-column-features.ts for consistency + // Use ref to get the latest features without causing effect re-runs + const currentFeatures = hookFeaturesRef.current; + const backlogFeatures = currentFeatures.filter((f) => { if (f.status !== "backlog") return false; - // Determine the feature's branch (default to primary branch if not set) - const featureBranch = f.branchName || primaryBranch || "main"; + const featureBranch = f.branchName; - // If no worktree is selected (currentWorktreeBranch is null or matches primary), - // show features with no branch or primary branch - if ( - !currentWorktreeBranch || - (currentProject.path && - isPrimaryWorktreeBranch( - currentProject.path, - currentWorktreeBranch - )) - ) { - return ( - !f.branchName || - (currentProject.path && - isPrimaryWorktreeBranch(currentProject.path, featureBranch)) - ); + // Features without branchName are considered unassigned (show only on primary worktree) + if (!featureBranch) { + // No branch assigned - show only when viewing primary worktree + const isViewingPrimary = currentWorktreePath === null; + return isViewingPrimary; } - // Otherwise, only show features matching the selected worktree branch + if (currentWorktreeBranch === null) { + // We're viewing main but branch hasn't been initialized yet + // Show features assigned to primary worktree's branch + return currentProject.path + ? isPrimaryWorktreeBranch(currentProject.path, featureBranch) + : false; + } + + // Match by branch name return featureBranch === currentWorktreeBranch; }); @@ -531,7 +534,7 @@ export function BoardView() { // Filter out features with blocking dependencies if dependency blocking is enabled const eligibleFeatures = enableDependencyBlocking ? sortedBacklog.filter((f) => { - const blockingDeps = getBlockingDependencies(f, hookFeatures); + const blockingDeps = getBlockingDependencies(f, currentFeatures); return blockingDeps.length === 0; }) : sortedBacklog; @@ -591,7 +594,7 @@ export function BoardView() { currentProject, runningAutoTasks, maxConcurrency, - hookFeatures, + // hookFeatures is accessed via hookFeaturesRef to prevent effect re-runs currentWorktreeBranch, currentWorktreePath, getPrimaryWorktreeBranch, @@ -617,8 +620,6 @@ export function BoardView() { runningAutoTasks, persistFeatureUpdate, handleStartImplementation, - projectPath: currentProject?.path || null, - onWorktreeCreated: () => setWorktreeRefreshKey((k) => k + 1), }); // Use column features hook diff --git a/apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx b/apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx index 438261ab..3800a751 100644 --- a/apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx +++ b/apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx @@ -14,7 +14,6 @@ import { Button } from "@/components/ui/button"; import { HotkeyButton } from "@/components/ui/hotkey-button"; import { Label } from "@/components/ui/label"; import { CategoryAutocomplete } from "@/components/ui/category-autocomplete"; -import { BranchAutocomplete } from "@/components/ui/branch-autocomplete"; import { DescriptionImageDropZone, FeatureImagePath as DescriptionImagePath, diff --git a/apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx b/apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx index 31f58103..dd5dd344 100644 --- a/apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx +++ b/apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx @@ -1,6 +1,6 @@ "use client"; -import { useState, useEffect } from "react"; +import { useState, useEffect, useRef } from "react"; import { Dialog, DialogContent, @@ -49,6 +49,8 @@ export function CreatePRDialog({ const [prUrl, setPrUrl] = useState(null); const [browserUrl, setBrowserUrl] = useState(null); const [showBrowserFallback, setShowBrowserFallback] = useState(false); + // Track whether an operation completed that warrants a refresh + const operationCompletedRef = useRef(false); // Reset state when dialog opens or worktree changes useEffect(() => { @@ -65,6 +67,8 @@ export function CreatePRDialog({ setPrUrl(null); setBrowserUrl(null); setShowBrowserFallback(false); + // Reset operation tracking + operationCompletedRef.current = false; } else { // Reset everything when dialog closes setTitle(""); @@ -76,6 +80,7 @@ export function CreatePRDialog({ setPrUrl(null); setBrowserUrl(null); setShowBrowserFallback(false); + operationCompletedRef.current = false; } }, [open, worktree?.path]); @@ -102,6 +107,8 @@ export function CreatePRDialog({ if (result.success && result.result) { if (result.result.prCreated && result.result.prUrl) { setPrUrl(result.result.prUrl); + // Mark operation as completed for refresh on close + operationCompletedRef.current = true; toast.success("Pull request created!", { description: `PR created from ${result.result.branch}`, action: { @@ -122,6 +129,8 @@ export function CreatePRDialog({ if (prError === "gh_cli_not_available" || !result.result.ghCliAvailable) { setBrowserUrl(result.result.browserUrl ?? null); setShowBrowserFallback(true); + // Mark operation as completed - branch was pushed successfully + operationCompletedRef.current = true; toast.success("Branch pushed", { description: result.result.committed ? `Commit ${result.result.commitHash} pushed to ${result.result.branch}` @@ -147,6 +156,8 @@ export function CreatePRDialog({ // Show error but also provide browser option setBrowserUrl(result.result.browserUrl ?? null); setShowBrowserFallback(true); + // Mark operation as completed - branch was pushed even though PR creation failed + operationCompletedRef.current = true; toast.error("PR creation failed", { description: errorMessage, duration: 8000, @@ -187,22 +198,13 @@ export function CreatePRDialog({ }; const handleClose = () => { - // Call onCreated() to refresh worktrees when dialog closes - // This ensures the worktree list is updated after any operation - onCreated(); + // Only call onCreated() if an actual operation completed + // This prevents unnecessary refreshes when user cancels + if (operationCompletedRef.current) { + onCreated(); + } onOpenChange(false); - // Reset state after dialog closes - setTimeout(() => { - setTitle(""); - setBody(""); - setCommitMessage(""); - setBaseBranch("main"); - setIsDraft(false); - setError(null); - setPrUrl(null); - setBrowserUrl(null); - setShowBrowserFallback(false); - }, 200); + // State reset is handled by useEffect when open becomes false }; if (!worktree) return null; diff --git a/apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx b/apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx index 54e3a70d..dcc5dd98 100644 --- a/apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx +++ b/apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx @@ -14,7 +14,6 @@ import { Button } from "@/components/ui/button"; import { HotkeyButton } from "@/components/ui/hotkey-button"; import { Label } from "@/components/ui/label"; import { CategoryAutocomplete } from "@/components/ui/category-autocomplete"; -import { BranchAutocomplete } from "@/components/ui/branch-autocomplete"; import { DescriptionImageDropZone, FeatureImagePath as DescriptionImagePath, diff --git a/apps/app/src/components/views/board-view/hooks/use-board-actions.ts b/apps/app/src/components/views/board-view/hooks/use-board-actions.ts index 9584d7ae..68d986f8 100644 --- a/apps/app/src/components/views/board-view/hooks/use-board-actions.ts +++ b/apps/app/src/components/views/board-view/hooks/use-board-actions.ts @@ -103,10 +103,7 @@ export function useBoardActions({ // Simplified: Only store branchName, no worktree creation on add // Worktrees are created at execution time (when feature starts) // Empty string means user chose "use current branch" - don't save a branch name - const finalBranchName = - featureData.branchName === "" - ? undefined - : featureData.branchName || undefined; + const finalBranchName = featureData.branchName || undefined; const newFeatureData = { ...featureData, @@ -139,8 +136,7 @@ export function useBoardActions({ requirePlanApproval?: boolean; } ) => { - const finalBranchName = - updates.branchName === "" ? undefined : updates.branchName || undefined; + const finalBranchName = updates.branchName || undefined; const finalUpdates = { ...updates, diff --git a/apps/app/src/components/views/board-view/hooks/use-board-column-features.ts b/apps/app/src/components/views/board-view/hooks/use-board-column-features.ts index 24810f41..6b70ed59 100644 --- a/apps/app/src/components/views/board-view/hooks/use-board-column-features.ts +++ b/apps/app/src/components/views/board-view/hooks/use-board-column-features.ts @@ -98,7 +98,9 @@ export function useBoardColumnFeatures({ } } else { // Unknown status, default to backlog - map.backlog.push(f); + if (matchesWorktree) { + map.backlog.push(f); + } } } }); diff --git a/apps/app/src/components/views/board-view/hooks/use-board-drag-drop.ts b/apps/app/src/components/views/board-view/hooks/use-board-drag-drop.ts index 92366e5b..6dd68f41 100644 --- a/apps/app/src/components/views/board-view/hooks/use-board-drag-drop.ts +++ b/apps/app/src/components/views/board-view/hooks/use-board-drag-drop.ts @@ -14,8 +14,6 @@ interface UseBoardDragDropProps { updates: Partial ) => Promise; handleStartImplementation: (feature: Feature) => Promise; - projectPath: string | null; // Main project path - onWorktreeCreated?: () => void; // Callback when a new worktree is created } export function useBoardDragDrop({ @@ -24,8 +22,6 @@ export function useBoardDragDrop({ runningAutoTasks, persistFeatureUpdate, handleStartImplementation, - projectPath, - onWorktreeCreated, }: UseBoardDragDropProps) { const [activeFeature, setActiveFeature] = useState(null); const { moveFeature } = useAppStore(); diff --git a/apps/app/src/components/views/board-view/shared/branch-selector.tsx b/apps/app/src/components/views/board-view/shared/branch-selector.tsx index 3fccf384..a395edf5 100644 --- a/apps/app/src/components/views/board-view/shared/branch-selector.tsx +++ b/apps/app/src/components/views/board-view/shared/branch-selector.tsx @@ -3,7 +3,6 @@ import { Label } from "@/components/ui/label"; import { BranchAutocomplete } from "@/components/ui/branch-autocomplete"; import { RadioGroup, RadioGroupItem } from "@/components/ui/radio-group"; -import { cn } from "@/lib/utils"; interface BranchSelectorProps { useCurrentBranch: boolean; @@ -32,12 +31,13 @@ export function BranchSelector({ return (
- + onUseCurrentBranchChange(value === "current")} disabled={disabled} data-testid={`${testIdPrefix}-radio-group`} + aria-labelledby={`${testIdPrefix}-label`} >
diff --git a/apps/app/src/components/views/context-view.tsx b/apps/app/src/components/views/context-view.tsx index 27d78e35..bfb1fbad 100644 --- a/apps/app/src/components/views/context-view.tsx +++ b/apps/app/src/components/views/context-view.tsx @@ -212,20 +212,14 @@ export function ContextView() { // Write text file with content (or empty if no content) await api.writeFile(filePath, newFileContent); } - - // Close dialog and reset state immediately after successful file write - setIsAddDialogOpen(false); - setNewFileName(""); - setNewFileType("text"); - setUploadedImageData(null); - setNewFileContent(""); - setIsDropHovering(false); - // Load files after dialog is closed + // Only reload files on success await loadContextFiles(); } catch (error) { console.error("Failed to add file:", error); - // Still close the dialog even if loadContextFiles fails + // Optionally show error toast to user here + } finally { + // Close dialog and reset state setIsAddDialogOpen(false); setNewFileName(""); setNewFileType("text"); diff --git a/apps/app/src/hooks/use-auto-mode.ts b/apps/app/src/hooks/use-auto-mode.ts index 9616dc01..9b4f346e 100644 --- a/apps/app/src/hooks/use-auto-mode.ts +++ b/apps/app/src/hooks/use-auto-mode.ts @@ -327,7 +327,6 @@ export function useAutoMode() { projectId, addRunningTask, removeRunningTask, - setAutoModeRunning, addAutoModeActivity, getProjectIdFromPath, setPendingPlanApproval, diff --git a/apps/app/tests/worktree-integration.spec.ts b/apps/app/tests/worktree-integration.spec.ts index c21228c4..f7cb7851 100644 --- a/apps/app/tests/worktree-integration.spec.ts +++ b/apps/app/tests/worktree-integration.spec.ts @@ -2353,7 +2353,7 @@ test.describe("Worktree Integration Tests", () => { // Edit Feature with Branch Change // ========================================================================== - test("should create worktree when editing a feature and selecting a new branch", async ({ + test("should update branchName when editing a feature and selecting a new branch", async ({ page, }) => { await setupProjectWithPath(page, testRepo.path); @@ -2399,7 +2399,7 @@ test.describe("Worktree Integration Tests", () => { const newBranchName = "feature/edited-branch"; const expectedWorktreePath = getWorktreePath(testRepo.path, newBranchName); - // Verify worktree does NOT exist before editing + // Verify worktree does NOT exist before editing (worktrees are created at execution time) expect(fs.existsSync(expectedWorktreePath)).toBe(false); // Find and click the edit button on the feature card @@ -2431,20 +2431,22 @@ test.describe("Worktree Integration Tests", () => { const saveButton = page.locator('[data-testid="confirm-edit-feature"]'); await saveButton.click(); - // Wait for the dialog to close and worktree to be created + // Wait for the dialog to close await page.waitForTimeout(2000); - // Verify worktree was automatically created - expect(fs.existsSync(expectedWorktreePath)).toBe(true); + // Verify worktree was NOT created during editing (worktrees are created at execution time) + expect(fs.existsSync(expectedWorktreePath)).toBe(false); - // Verify the branch was created + // Verify the branch was created (if branch creation is part of the autocomplete flow) const branches = await listBranches(testRepo.path); expect(branches).toContain(newBranchName); - // Verify feature was updated with correct branch and worktreePath + // Verify feature was updated with correct branchName only + // Note: worktreePath is no longer stored - worktrees are created server-side at execution time featureData = JSON.parse(fs.readFileSync(featureFilePath, "utf-8")); expect(featureData.branchName).toBe(newBranchName); - expect(featureData.worktreePath).toBe(expectedWorktreePath); + // worktreePath should not exist in the feature data + expect(featureData.worktreePath).toBeUndefined(); }); test("should not create worktree when editing a feature and selecting main branch", async ({ diff --git a/apps/server/src/routes/auto-mode/routes/resume-feature.ts b/apps/server/src/routes/auto-mode/routes/resume-feature.ts index 45410ba7..134c36df 100644 --- a/apps/server/src/routes/auto-mode/routes/resume-feature.ts +++ b/apps/server/src/routes/auto-mode/routes/resume-feature.ts @@ -32,8 +32,7 @@ export function createResumeFeatureHandler(autoModeService: AutoModeService) { .resumeFeature(projectPath, featureId, useWorktrees ?? false) .catch((error) => { logger.error(`[AutoMode] Resume feature ${featureId} error:`, error); - }) - .finally(() => {}); + }); res.json({ success: true }); } catch (error) {