ros/automaker
1
0
Fork 0
mirror of https://github.com/AutoMaker-Org/automaker.git synced 2026-03-19 10:43:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Address code review comments

Browse Source
This commit is contained in:
gsxdsm
2026-02-17 23:04:42 -08:00
parent 43c19c70ca
commit dd4c738e91
43 changed files with 1128 additions and 359 deletions
Download Patch File Download Diff File Expand all files Collapse all files

60
apps/ui/src/components/views/board-view.tsx
View File

@@ -68,6 +68,7 @@ import type {
WorktreeInfo,
MergeConflictInfo,
BranchSwitchConflictInfo,
StashPopConflictInfo,
} from './board-view/worktree-panel/types';
import { COLUMNS, getColumnsWithPipeline } from './board-view/constants';
import {
@@ -1083,6 +1084,64 @@ export function BoardView() {
[handleAddFeature, handleStartImplementation, defaultSkipTests]
);
// Handler called when checkout fails AND the stash-pop restoration produces merge conflicts.
// Creates an AI-assisted board task to guide the user through resolving the conflicts.
const handleStashPopConflict = useCallback(
async (conflictInfo: StashPopConflictInfo) => {
const description =
`Resolve merge conflicts that occurred when attempting to switch to branch "${conflictInfo.branchName}". ` +
`The checkout failed and, while restoring the previously stashed local changes, git reported merge conflicts. ` +
`${conflictInfo.stashPopConflictMessage} ` +
`Please review all conflicted files, resolve the conflicts, ensure the code compiles and tests pass, ` +
`then re-attempt the branch switch.`;
// Create the feature
const featureData = {
title: `Resolve Stash-Pop Conflicts: branch switch to ${conflictInfo.branchName}`,
category: 'Maintenance',
description,
images: [],
imagePaths: [],
skipTests: defaultSkipTests,
model: 'opus' as const,
thinkingLevel: 'none' as const,
branchName: conflictInfo.branchName,
workMode: 'custom' as const,
priority: 1,
planningMode: 'skip' as const,
requirePlanApproval: false,
};
// Capture existing feature IDs before adding
const featuresBeforeIds = new Set(useAppStore.getState().features.map((f) => f.id));
try {
await handleAddFeature(featureData);
} catch (error) {
logger.error('Failed to create stash-pop conflict resolution feature:', error);
toast.error('Failed to create feature', {
description: error instanceof Error ? error.message : 'An error occurred',
});
return;
}
// Find the newly created feature by looking for an ID that wasn't in the original set
const latestFeatures = useAppStore.getState().features;
const newFeature = latestFeatures.find((f) => !featuresBeforeIds.has(f.id));
if (newFeature) {
await handleStartImplementation(newFeature);
} else {
logger.error(
'Could not find newly created stash-pop conflict feature to start it automatically.'
);
toast.error('Failed to auto-start feature', {
description: 'The feature was created but could not be started automatically.',
});
}
},
[handleAddFeature, handleStartImplementation, defaultSkipTests]
);
// Handler for "Make" button - creates a feature and immediately starts it
const handleAddAndStartFeature = useCallback(
async (featureData: Parameters<typeof handleAddFeature>[0]) => {
@@ -1523,6 +1582,7 @@ export function BoardView() {
onResolveConflicts={handleResolveConflicts}
onCreateMergeConflictResolutionFeature={handleCreateMergeConflictResolutionFeature}
onBranchSwitchConflict={handleBranchSwitchConflict}
onStashPopConflict={handleStashPopConflict}
onBranchDeletedDuringMerge={(branchName) => {
// Reset features that were assigned to the deleted branch (same logic as onDeleted in DeleteWorktreeDialog)
hookFeatures.forEach((feature) => {

13
apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx
View File

@@ -174,6 +174,7 @@ export function CherryPickDialog({
setCommitsError(null);
setCommitLimit(30);
setHasMoreCommits(false);
setLoadingBranches(false);
}
}, [open]);
@@ -321,9 +322,7 @@ export function CherryPickDialog({
} else {
// Check for conflicts
const errorMessage = result.error || '';
const hasConflicts =
errorMessage.toLowerCase().includes('conflict') ||
(result as { hasConflicts?: boolean }).hasConflicts;
const hasConflicts = errorMessage.toLowerCase().includes('conflict') || result.hasConflicts;
if (hasConflicts && onCreateConflictResolutionFeature) {
setConflictInfo({
@@ -333,7 +332,7 @@ export function CherryPickDialog({
});
setStep('conflict');
toast.error('Cherry-pick conflicts detected', {
description: 'The cherry-pick has conflicts that need to be resolved.',
description: 'The cherry-pick was aborted due to conflicts. No changes were applied.',
});
} else {
toast.error('Cherry-pick failed', {
@@ -359,7 +358,7 @@ export function CherryPickDialog({
});
setStep('conflict');
toast.error('Cherry-pick conflicts detected', {
description: 'The cherry-pick has conflicts that need to be resolved.',
description: 'The cherry-pick was aborted due to conflicts. No changes were applied.',
});
} else {
toast.error('Cherry-pick failed', {
@@ -469,7 +468,7 @@ export function CherryPickDialog({
<DialogContent className="w-full h-full max-w-full max-h-full sm:w-[90vw] sm:max-w-[640px] sm:max-h-[100dvh] sm:h-auto sm:rounded-xl rounded-none flex flex-col">
<DialogHeader>
<DialogTitle className="flex items-center gap-2">
<Cherry className="w-5 h-5 text-black dark:text-black" />
<Cherry className="w-5 h-5 text-foreground" />
Cherry Pick Commits
</DialogTitle>
<DialogDescription>
@@ -673,7 +672,7 @@ export function CherryPickDialog({
<DialogContent>
<DialogHeader>
<DialogTitle className="flex items-center gap-2">
<Cherry className="w-5 h-5 text-black dark:text-black" />
<Cherry className="w-5 h-5 text-foreground" />
Cherry Pick
</DialogTitle>
<DialogDescription asChild>

5
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx
View File

@@ -407,7 +407,7 @@ export function CommitWorktreeDialog({
const handleKeyDown = (e: React.KeyboardEvent) => {
if (
e.key === 'Enter' &&
e.metaKey &&
(e.metaKey || e.ctrlKey) &&
!isLoading &&
!isGenerating &&
message.trim() &&
@@ -658,7 +658,8 @@ export function CommitWorktreeDialog({
</div>
<p className="text-xs text-muted-foreground">
Press <kbd className="px-1 py-0.5 bg-muted rounded text-xs">Cmd+Enter</kbd> to commit
Press <kbd className="px-1 py-0.5 bg-muted rounded text-xs">Cmd/Ctrl+Enter</kbd> to
commit
</p>
</div>

12
apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx
View File

@@ -78,9 +78,13 @@ export function CreateBranchDialog({
if (result.success && result.result) {
setBranches(result.result.branches);
// Default to current branch
if (result.result.currentBranch) {
setBaseBranch(result.result.currentBranch);
// Only set the default base branch if no branch is currently selected,
// or if the currently selected branch is no longer present in the fetched list
const branchNames = result.result.branches.map((b: BranchInfo) => b.name);
if (!baseBranch || !branchNames.includes(baseBranch)) {
if (result.result.currentBranch) {
setBaseBranch(result.result.currentBranch);
}
}
}
} catch (err) {
@@ -88,7 +92,7 @@ export function CreateBranchDialog({
} finally {
setIsLoadingBranches(false);
}
}, [worktree]);
}, [worktree, baseBranch]);
// Reset state and fetch branches when dialog opens
useEffect(() => {

18
apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx
View File

@@ -75,6 +75,12 @@ export function CreatePRDialog({
const [remotes, setRemotes] = useState<RemoteInfo[]>([]);
const [selectedRemote, setSelectedRemote] = useState<string>('');
const [isLoadingRemotes, setIsLoadingRemotes] = useState(false);
// Keep a ref in sync with selectedRemote so fetchRemotes can read the latest value
// without needing it in its dependency array (which would cause re-fetch loops)
const selectedRemoteRef = useRef<string>(selectedRemote);
useEffect(() => {
selectedRemoteRef.current = selectedRemote;
}, [selectedRemote]);
// Generate description state
const [isGeneratingDescription, setIsGeneratingDescription] = useState(false);
@@ -110,10 +116,16 @@ export function CreatePRDialog({
);
setRemotes(remoteInfos);
// Auto-select 'origin' if available, otherwise first remote
// Preserve existing selection if it's still valid; otherwise fall back to 'origin' or first remote
if (remoteInfos.length > 0) {
const defaultRemote = remoteInfos.find((r) => r.name === 'origin') || remoteInfos[0];
setSelectedRemote(defaultRemote.name);
const remoteNames = remoteInfos.map((r) => r.name);
const currentSelection = selectedRemoteRef.current;
const currentSelectionStillExists =
currentSelection !== '' && remoteNames.includes(currentSelection);
if (!currentSelectionStillExists) {
const defaultRemote = remoteInfos.find((r) => r.name === 'origin') || remoteInfos[0];
setSelectedRemote(defaultRemote.name);
}
}
}
} catch {

3
apps/ui/src/components/views/board-view/dialogs/discard-worktree-changes-dialog.tsx
View File

@@ -319,6 +319,7 @@ export function DiscardWorktreeChangesDialog({
}
} catch (err) {
console.warn('Failed to load diffs for discard dialog:', err);
setError(err instanceof Error ? err.message : String(err));
} finally {
setIsLoadingDiffs(false);
}
@@ -370,7 +371,7 @@ export function DiscardWorktreeChangesDialog({
if (result.success && result.result) {
if (result.result.discarded) {
const fileCount = filesToDiscard ? filesToDiscard.length : result.result.filesDiscarded;
const fileCount = filesToDiscard ? filesToDiscard.length : selectedFiles.size;
toast.success('Changes discarded', {
description: `Discarded ${fileCount} ${fileCount === 1 ? 'file' : 'files'} in ${worktree.branch}`,
});

17
apps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsx
View File

@@ -167,10 +167,15 @@ export function MergeRebaseDialog({
}
};
const handleConfirm = () => {
const handleConfirm = async () => {
if (!worktree || !selectedBranch) return;
onConfirm(worktree, selectedBranch, selectedStrategy);
onOpenChange(false);
try {
await onConfirm(worktree, selectedBranch, selectedStrategy);
onOpenChange(false);
} catch (err) {
logger.error('Failed to confirm merge/rebase:', err);
throw err;
}
};
const selectedRemoteData = remotes.find((r) => r.name === selectedRemote);
@@ -347,7 +352,11 @@ export function MergeRebaseDialog({
className="bg-purple-600 hover:bg-purple-700 text-white"
>
<GitMerge className="w-4 h-4 mr-2" />
Merge & Rebase
{selectedStrategy === 'merge'
? 'Merge'
: selectedStrategy === 'rebase'
? 'Rebase'
: 'Merge & Rebase'}
</Button>
</DialogFooter>
</DialogContent>

12
apps/ui/src/components/views/board-view/dialogs/select-remote-dialog.tsx
View File

@@ -69,6 +69,12 @@ export function SelectRemoteDialog({
url: r.url,
}));
setRemotes(remoteInfos);
setSelectedRemote((prev) => {
if (prev && remoteInfos.some((r) => r.name === prev)) {
return prev;
}
return remoteInfos.find((r) => r.name === 'origin')?.name ?? remoteInfos[0]?.name ?? '';
});
} else {
setError(result.error || 'Failed to fetch remotes');
}
@@ -120,6 +126,12 @@ export function SelectRemoteDialog({
url: r.url,
}));
setRemotes(remoteInfos);
setSelectedRemote((prev) => {
if (prev && remoteInfos.some((r) => r.name === prev)) {
return prev;
}
return remoteInfos.find((r) => r.name === 'origin')?.name ?? remoteInfos[0]?.name ?? '';
});
} else {
setError(result.error || 'Failed to refresh remotes');
}

3
apps/ui/src/components/views/board-view/dialogs/stash-changes-dialog.tsx
View File

@@ -132,6 +132,9 @@ function parseDiff(diffText: string): ParsedFileDiff[] {
for (let i = 0; i < lines.length; i++) {
const line = lines[i];
// Skip trailing empty string produced by a final newline in diffText
if (line === '' && i === lines.length - 1) continue;
if (line.startsWith('diff --git')) {
if (currentFile) {
if (currentHunk) currentFile.hunks.push(currentHunk);

2
apps/ui/src/components/views/board-view/dialogs/view-commits-dialog.tsx
View File

@@ -47,7 +47,9 @@ interface ViewCommitsDialogProps {
}
function formatRelativeDate(dateStr: string): string {
if (!dateStr) return 'unknown date';
const date = new Date(dateStr);
if (isNaN(date.getTime())) return 'unknown date';
const now = new Date();
const diffMs = now.getTime() - date.getTime();
const diffSecs = Math.floor(diffMs / 1000);

13
apps/ui/src/components/views/board-view/hooks/use-board-actions.ts
View File

@@ -280,6 +280,8 @@ export function useBoardActions({
});
}
}
return createdFeature;
},
[
addFeature,
@@ -1221,13 +1223,12 @@ export function useBoardActions({
dependencies: [parentFeature.id],
};
await handleAddFeature(duplicatedFeatureData);
const newFeature = await handleAddFeature(duplicatedFeatureData);
// Get the newly created feature (last added feature) to use as parent for next iteration
const currentFeatures = useAppStore.getState().features;
const newestFeature = currentFeatures[currentFeatures.length - 1];
if (newestFeature) {
parentFeature = newestFeature;
// Use the returned feature directly as the parent for the next iteration,
// avoiding a fragile assumption that the newest feature is the last item in the store
if (newFeature) {
parentFeature = newFeature;
}
}

17
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
View File

@@ -451,31 +451,28 @@ export function WorktreeActionsDropdown({
)}
{/* Stash operations - combined submenu */}
{(onStashChanges || onViewStashes) && (
<TooltipWrapper
showTooltip={!gitRepoStatus.isGitRepo}
tooltipContent="Not a git repository"
>
<TooltipWrapper showTooltip={!canPerformGitOps} tooltipContent={gitOpsDisabledReason}>
<DropdownMenuSub>
<div className="flex items-center">
{/* Main clickable area - stash changes (primary action) */}
<DropdownMenuItem
onClick={() => {
if (!gitRepoStatus.isGitRepo) return;
if (!canPerformGitOps) return;
if (worktree.hasChanges && onStashChanges) {
onStashChanges(worktree);
} else if (onViewStashes) {
onViewStashes(worktree);
}
}}
disabled={!gitRepoStatus.isGitRepo}
disabled={!canPerformGitOps}
className={cn(
'text-xs flex-1 pr-0 rounded-r-none',
!gitRepoStatus.isGitRepo && 'opacity-50 cursor-not-allowed'
!canPerformGitOps && 'opacity-50 cursor-not-allowed'
)}
>
<Archive className="w-3.5 h-3.5 mr-2" />
{worktree.hasChanges && onStashChanges ? 'Stash Changes' : 'Stashes'}
{!gitRepoStatus.isGitRepo && (
{!canPerformGitOps && (
<AlertCircle className="w-3 h-3 ml-auto text-muted-foreground" />
)}
</DropdownMenuItem>
@@ -483,9 +480,9 @@ export function WorktreeActionsDropdown({
<DropdownMenuSubTrigger
className={cn(
'text-xs px-1 rounded-l-none border-l border-border/30 h-8',
!gitRepoStatus.isGitRepo && 'opacity-50 cursor-not-allowed'
!canPerformGitOps && 'opacity-50 cursor-not-allowed'
)}
disabled={!gitRepoStatus.isGitRepo}
disabled={!canPerformGitOps}
/>
</div>
<DropdownMenuSubContent>

7
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts
View File

@@ -20,6 +20,12 @@ interface UseWorktreeActionsOptions {
branchName: string;
previousBranch: string;
}) => void;
/** Callback when checkout fails AND the stash-pop restoration produces merge conflicts */
onStashPopConflict?: (info: {
worktreePath: string;
branchName: string;
stashPopConflictMessage: string;
}) => void;
}
export function useWorktreeActions(options?: UseWorktreeActionsOptions) {
@@ -29,6 +35,7 @@ export function useWorktreeActions(options?: UseWorktreeActionsOptions) {
// Use React Query mutations
const switchBranchMutation = useSwitchBranch({
onConflict: options?.onBranchSwitchConflict,
onStashPopConflict: options?.onStashPopConflict,
});
const pullMutation = usePullWorktree();
const pushMutation = usePushWorktree();

9
apps/ui/src/components/views/board-view/worktree-panel/types.ts
View File

@@ -86,6 +86,13 @@ export interface BranchSwitchConflictInfo {
previousBranch: string;
}
/** Info passed when a checkout failure triggers a stash-pop that itself produces conflicts */
export interface StashPopConflictInfo {
worktreePath: string;
branchName: string;
stashPopConflictMessage: string;
}
export interface WorktreePanelProps {
projectPath: string;
onCreateWorktree: () => void;
@@ -98,6 +105,8 @@ export interface WorktreePanelProps {
onCreateMergeConflictResolutionFeature?: (conflictInfo: MergeConflictInfo) => void;
/** Called when branch switch stash reapply results in merge conflicts */
onBranchSwitchConflict?: (conflictInfo: BranchSwitchConflictInfo) => void;
/** Called when checkout fails and the stash-pop restoration itself produces merge conflicts */
onStashPopConflict?: (conflictInfo: StashPopConflictInfo) => void;
/** Called when a branch is deleted during merge - features should be reassigned to main */
onBranchDeletedDuringMerge?: (branchName: string) => void;
onRemovedWorktrees?: (removedWorktrees: Array<{ path: string; branch: string }>) => void;

14
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx
View File

@@ -60,6 +60,7 @@ export function WorktreePanel({
onResolveConflicts,
onCreateMergeConflictResolutionFeature,
onBranchSwitchConflict,
onStashPopConflict,
onBranchDeletedDuringMerge,
onRemovedWorktrees,
runningFeatureIds = [],
@@ -113,6 +114,7 @@ export function WorktreePanel({
handleOpenInExternalTerminal,
} = useWorktreeActions({
onBranchSwitchConflict: onBranchSwitchConflict,
onStashPopConflict: onStashPopConflict,
});
const { hasRunningFeatures } = useRunningFeatures({
@@ -563,8 +565,12 @@ export function WorktreePanel({
setSelectRemoteWorktree(worktree);
setSelectRemoteOperation('pull');
setSelectRemoteDialogOpen(true);
} else if (result.success && result.result && result.result.remotes.length === 1) {
// Exactly one remote - use it directly
const remoteName = result.result.remotes[0].name;
handlePull(worktree, remoteName);
} else {
// Single or no remote - proceed with default behavior
// No remotes - proceed with default behavior
handlePull(worktree);
}
} catch {
@@ -587,8 +593,12 @@ export function WorktreePanel({
setSelectRemoteWorktree(worktree);
setSelectRemoteOperation('push');
setSelectRemoteDialogOpen(true);
} else if (result.success && result.result && result.result.remotes.length === 1) {
// Exactly one remote - use it directly
const remoteName = result.result.remotes[0].name;
handlePush(worktree, remoteName);
} else {
// Single or no remote - proceed with default behavior
// No remotes - proceed with default behavior
handlePush(worktree);
}
} catch {

33
apps/ui/src/components/views/project-settings-view/worktree-preferences-section.tsx
View File

@@ -47,7 +47,7 @@ export function WorktreePreferencesSection({ project }: WorktreePreferencesSecti
const setDefaultDeleteBranch = useAppStore((s) => s.setDefaultDeleteBranch);
const getAutoDismissInitScriptIndicator = useAppStore((s) => s.getAutoDismissInitScriptIndicator);
const setAutoDismissInitScriptIndicator = useAppStore((s) => s.setAutoDismissInitScriptIndicator);
const getWorktreeCopyFiles = useAppStore((s) => s.getWorktreeCopyFiles);
const copyFiles = useAppStore((s) => s.worktreeCopyFilesByProject[project.path] ?? []);
const setWorktreeCopyFiles = useAppStore((s) => s.setWorktreeCopyFiles);
// Get effective worktrees setting (project override or global fallback)
@@ -64,7 +64,6 @@ export function WorktreePreferencesSection({ project }: WorktreePreferencesSecti
// Copy files state
const [newCopyFilePath, setNewCopyFilePath] = useState('');
const [fileSelectorOpen, setFileSelectorOpen] = useState(false);
const copyFiles = getWorktreeCopyFiles(project.path);
// Get the current settings for this project
const showIndicator = getShowInitScriptIndicator(project.path);
@@ -245,15 +244,15 @@ export function WorktreePreferencesSection({ project }: WorktreePreferencesSecti
if (!normalized) return;
// Check for duplicates
const currentFiles = getWorktreeCopyFiles(project.path);
if (currentFiles.includes(normalized)) {
if (copyFiles.includes(normalized)) {
toast.error('File already in list', {
description: `"${normalized}" is already configured for copying.`,
});
return;
}
const updatedFiles = [...currentFiles, normalized];
const prevFiles = copyFiles;
const updatedFiles = [...copyFiles, normalized];
setWorktreeCopyFiles(project.path, updatedFiles);
setNewCopyFilePath('');
@@ -267,16 +266,19 @@ export function WorktreePreferencesSection({ project }: WorktreePreferencesSecti
description: `"${normalized}" will be copied to new worktrees.`,
});
} catch (error) {
// Rollback optimistic update on failure
setWorktreeCopyFiles(project.path, prevFiles);
setNewCopyFilePath(normalized);
console.error('Failed to persist worktreeCopyFiles:', error);
toast.error('Failed to save copy files setting');
}
}, [project.path, newCopyFilePath, getWorktreeCopyFiles, setWorktreeCopyFiles]);
}, [project.path, newCopyFilePath, copyFiles, setWorktreeCopyFiles]);
// Remove a file path from copy list
const handleRemoveCopyFile = useCallback(
async (filePath: string) => {
const currentFiles = getWorktreeCopyFiles(project.path);
const updatedFiles = currentFiles.filter((f) => f !== filePath);
const prevFiles = copyFiles;
const updatedFiles = copyFiles.filter((f) => f !== filePath);
setWorktreeCopyFiles(project.path, updatedFiles);
// Persist to server
@@ -287,26 +289,27 @@ export function WorktreePreferencesSection({ project }: WorktreePreferencesSecti
});
toast.success('Copy file removed');
} catch (error) {
// Rollback optimistic update on failure
setWorktreeCopyFiles(project.path, prevFiles);
console.error('Failed to persist worktreeCopyFiles:', error);
toast.error('Failed to save copy files setting');
}
},
[project.path, getWorktreeCopyFiles, setWorktreeCopyFiles]
[project.path, copyFiles, setWorktreeCopyFiles]
);
// Handle files selected from the file selector dialog
const handleFileSelectorSelect = useCallback(
async (paths: string[]) => {
const currentFiles = getWorktreeCopyFiles(project.path);
// Filter out duplicates
const newPaths = paths.filter((p) => !currentFiles.includes(p));
const newPaths = paths.filter((p) => !copyFiles.includes(p));
if (newPaths.length === 0) {
toast.info('All selected files are already in the list');
return;
}
const updatedFiles = [...currentFiles, ...newPaths];
const prevFiles = copyFiles;
const updatedFiles = [...copyFiles, ...newPaths];
setWorktreeCopyFiles(project.path, updatedFiles);
// Persist to server
@@ -319,11 +322,13 @@ export function WorktreePreferencesSection({ project }: WorktreePreferencesSecti
description: newPaths.map((p) => `"${p}"`).join(', '),
});
} catch (error) {
// Rollback optimistic update on failure
setWorktreeCopyFiles(project.path, prevFiles);
console.error('Failed to persist worktreeCopyFiles:', error);
toast.error('Failed to save copy files setting');
}
},
[project.path, getWorktreeCopyFiles, setWorktreeCopyFiles]
[project.path, copyFiles, setWorktreeCopyFiles]
);
return (