feat: mobile responsiveness, discard changes, open in terminal, and UX improvements #497
Conversation
…nd various UX improvements - Add mobile-responsive UI across all views (context, memory, issues, PRs) - Add "Open in Terminal" action for worktrees with cross-platform support - Add "Discard Changes" action to reset worktree to clean state - Add "View Changes" dialog to inspect worktree modifications - Add responsive toolbar with overflow menu for narrow screens - Add target branch selection for merge operations (not just main) - Add remote tracking branch detection for push button state - Fix featureId sanitization consistency across all worktree routes - Fix branch association for features created in "current" work mode - Improve sidebar behavior on mobile devices Co-Authored-By: eclipxe <discord> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces comprehensive worktree management enhancements including cross-platform terminal integration, change-discard functionality, and path sanitization. It adds backend endpoints for terminal launching and change discarding, extends branch metadata with remote-tracking flags, enables configurable merge targets, and improves mobile responsiveness across multiple UI views while threading worktree branch context through the component hierarchy. Changes
Sequence DiagramssequenceDiagram
participant User as UI User
participant Handler as Route Handler
participant Platform as `@automaker/platform`
participant Terminal as OS Terminal
User->>Handler: POST /open-in-terminal {worktreePath}
Handler->>Handler: Validate worktreePath exists
Handler->>Handler: Validate absolute path
Handler->>Platform: openInTerminal(worktreePath)
Platform->>Terminal: Platform-specific launch<br/>(AppleScript/wt/x-terminal-emulator)
Terminal-->>Platform: Terminal opened
Platform-->>Handler: {terminalName}
Handler-->>User: 200 {success, message, terminalName}
sequenceDiagram
participant User as UI User
participant Handler as Route Handler
participant Git as Git Operations
participant Status as Status Check
User->>Handler: POST /discard-changes {worktreePath}
Handler->>Handler: Validate worktreePath
Handler->>Status: git status (before)
Status-->>Handler: {uncommitted files}
Handler->>Git: git reset HEAD
Handler->>Git: git checkout .
Handler->>Git: git clean -fd
Handler->>Status: git status (after)
Status-->>Handler: {remaining changes}
Handler-->>User: 200 {discarded, filesDiscarded,<br/>filesRemaining, branch}
sequenceDiagram
participant User as UI User
participant BoardView as Board View
participant ConflictDialog as Conflict Resolution
participant Git as Git Remote
User->>BoardView: Click worktree (with Pull & Resolve)
BoardView->>ConflictDialog: Show source selection<br/>(Worktree vs Main Branch)
User->>ConflictDialog: Select source (worktree/selected)
ConflictDialog->>Git: Pull origin/{sourceBranch}
Git-->>ConflictDialog: Remote changes
ConflictDialog->>Git: Merge with resolution strategy
Git-->>ConflictDialog: Conflicts resolved
ConflictDialog-->>BoardView: Update worktree state
BoardView-->>User: Refresh UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 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
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 |
Summary of ChangesHello @Shironex, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's user experience by introducing comprehensive mobile responsiveness across various views and adding several crucial Git-related functionalities. It empowers users with more control over their worktrees through new actions like opening in terminal, discarding changes, and viewing modifications, while also streamlining merge workflows with target branch selection. The changes aim to provide a more robust and adaptable development environment, particularly for users on diverse screen sizes and those requiring finer-grained Git operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Shironex
added
Enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/ui/src/components/views/github-prs-view.tsx (1)
367-371: Bug:group-hoverwon't work withoutgroupclass on parent.The external link button uses
group-hover:opacity-100(line 435), but the parentdiv(line 368) is missing thegroupclass. This means the button will always remain invisible (opacity-0) since the hover trigger class is absent.🐛 Proposed fix
<div className={cn( - 'flex items-start gap-3 p-3 cursor-pointer hover:bg-accent/50 transition-colors', + 'group flex items-start gap-3 p-3 cursor-pointer hover:bg-accent/50 transition-colors', isSelected && 'bg-accent' )} onClick={onClick} >Also applies to: 432-442
apps/server/src/routes/worktree/routes/merge.ts (2)
60-71: Commit message is vulnerable to shell injection.The
options?.messagevalue is interpolated directly into shell commands without escaping. A message containing"or backticks would break the command or allow arbitrary code execution.🔒 Suggested fix — escape or use environment variables
One safe approach is to pass the message via environment variable:
- const mergeCmd = options?.squash - ? `git merge --squash ${branchName}` - : `git merge ${branchName} -m "${options?.message || `Merge ${branchName}`}"`; - - await execAsync(mergeCmd, { cwd: projectPath }); + const mergeMessage = options?.message || `Merge ${branchName}`; + const mergeCmd = options?.squash + ? `git merge --squash -- "${branchName}"` + : `git merge -- "${branchName}" -m "$MERGE_MSG"`; + + await execAsync(mergeCmd, { + cwd: projectPath, + env: { ...process.env, MERGE_MSG: mergeMessage } + }); // If squash merge, need to commit if (options?.squash) { - await execAsync(`git commit -m "${options?.message || `Merge ${branchName} (squash)`}"`, { + const squashMessage = options?.message || `Merge ${branchName} (squash)`; + await execAsync(`git commit -m "$COMMIT_MSG"`, { cwd: projectPath, + env: { ...process.env, COMMIT_MSG: squashMessage } }); }
73-78:worktreePathalso requires sanitization.Similar to branch names,
worktreePathis user input and should be validated before shell interpolation. A path like"; rm -rf /"would execute arbitrary commands.Consider validating that
worktreePathis a real path under an expected directory, or use path resolution and validation before use.apps/ui/src/hooks/use-media-query.ts (1)
44-50: Documentation doesn't match implementation.The docstring states the breakpoint is 768px, but the actual implementation uses 900px. This inconsistency could confuse developers maintaining this code.
📝 Suggested fix
/** - * Hook to detect if the device is mobile (screen width <= 768px) + * Hook to detect if the device is mobile (screen width <= 900px) * `@returns` boolean indicating if the device is mobile */ export function useIsMobile(): boolean { return useMediaQuery('(max-width: 900px)'); }apps/server/src/routes/worktree/routes/file-diff.ts (1)
49-65: Fix command injection vulnerability in file-diff handlers.The
filePathparameter is user-provided and passed directly toexecAsyncwith only double-quote wrapping. Double quotes do not protect against shell metacharacters—attackers can use$(...)or backticks to execute arbitrary commands.This affects:
apps/server/src/routes/worktree/routes/file-diff.ts:49,61apps/server/src/routes/git/routes/file-diff.ts:28,40Use
execFileorspawninstead ofexecto avoid shell interpretation, or validatefilePathto ensure it's a safe relative path without special characters.
🤖 Fix all issues with AI agents
In `@apps/server/src/routes/worktree/routes/open-in-terminal.ts`:
- Around line 12-49: createOpenInTerminalHandler currently performs the
open-in-terminal action but doesn't emit a server event; update the handler to
obtain/create the event emitter (via createEventEmitter()) and emit a
descriptive event (e.g., "worktree.openInTerminal" or similar) on success with
payload including worktreePath and result.terminalName, and emit a failure event
on catch including worktreePath and error details (use getErrorMessage(error))
before sending the HTTP response; reference the createOpenInTerminalHandler
function, the openInTerminal result, and the catch block to add the emitter.emit
calls.
In `@apps/ui/src/components/views/board-view/header-overflow-menu.tsx`:
- Around line 133-138: The DropdownMenuSeparator is rendered unconditionally and
can produce adjacent separators when only certain sections are collapsed; update
the render logic in header-overflow-menu.tsx to conditionally render the
DropdownMenuSeparator only when at least one of shouldCollapseAgents or
shouldCollapseAutoMode (or any other adjacent collapsed section) is true so you
don’t produce consecutive separators between the "Controls" label and the Plan
button; locate the separator instance near the Plan button render and wrap it in
a conditional that checks shouldCollapseAgents || shouldCollapseAutoMode (or
equivalent collapse flags) before rendering.
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Around line 569-607: In createTerminalWithCwd, the toast success uses
pendingCwd.split('/') which breaks on Windows paths; change the basename
extraction to split on both separators (e.g., pendingCwd?.split(/[/\\]/).pop()
|| pendingCwd) or compute a safe basename variable before calling toast.success,
and use that variable in toast.success and any related logs; ensure pendingCwd
is null-checked (pendingCwd?) to avoid exceptions when clearing the cwd.
- Around line 530-622: The pending-cwd effect can race with the existing
createTerminal flow and produce duplicate terminals; modify createTerminal (the
shared function used at Lines ~931-934) to consume and clear
terminalState.pendingTerminalCwd atomically: check
terminalState.pendingTerminalCwd at start, use that value when creating the
session (instead of letting the effect also do it), call
setPendingTerminalCwd(null) immediately after successfully enqueuing/creating
the session (and also on error), and remove or guard the effect’s creation so it
only runs when createTerminal hasn't already consumed the pending value (e.g.,
have the effect return early if pendingTerminalCwdRef.current === pendingCwd or
if a consumed flag is set). Ensure you still set
pendingTerminalCreatedRef.current / call addTerminalToLayout and
setNewSessionIds in the same success path so behavior remains unchanged.
In `@apps/ui/src/routes/__root.tsx`:
- Around line 826-836: The breakpoint inconsistency causes the streamer panel to
overlap at 1024px because useIsTablet() uses max-width:1024px while the JSX/CSS
uses the Tailwind lg breakpoint (min-width:1024px) for lg:block; to fix, make
the threshold consistent by updating useIsTablet (or its underlying
use-media-query) to use max-width:1023px (so tablet is <1024) or change the CSS
logic to use min-width:1025px equivalent, and ensure the JSX logic around
streamerPanelOpen and the inline marginRight calculation (where isTablet is
referenced) aligns with the same numeric breakpoint so the panel visibility
(lg:block) and isTablet boolean never both apply at 1024px.
In `@libs/platform/src/editor.ts`:
- Around line 384-395: openInTerminal currently interpolates targetPath directly
into an AppleScript shell command (script variable) causing possible shell
injection; instead avoid embedding raw shell content and use AppleScript's safe
quoting: build an AppleScript that assigns the path to a variable and uses
quoted form of POSIX path (or quoted form of POSIX file) when composing the do
script command, and stop directly concatenating targetPath into the shell
string; update the script generation in openInTerminal (and the execFileAsync
call) to use this AppleScript quoting approach so all special characters in
targetPath are safely handled.
- Around line 433-463: The Linux branch currently awaits execFileAsync for each
terminal (see terminals array and the usage of execFileAsync(terminal.command,
terminal.args)), which blocks until the terminal exits; modify this to use
child_process.spawn in a non-blocking way: call spawn(terminal.command,
terminal.args, { detached: true, stdio: 'ignore' }), do not await it, and call
child.unref() after spawn; keep the commandExists check and return {
terminalName: terminal.name } immediately after spawning, and throw the same
Error('No terminal emulator found') if none are available.
🧹 Nitpick comments (22)
apps/ui/src/components/ui/command.tsx (1)
67-72: Redundant font-size declaration for iOS zoom prevention.Both
text-base(16px) andstyle={{ fontSize: '16px' }}set the same size. The inline style is likely a defensive measure to prevent iOS Safari auto-zoom on input focus (which triggers when font-size < 16px), but it's redundant with the Tailwind class and reduces flexibility for className overrides.Consider either:
- Remove the inline style and rely on
text-base(cleaner, more flexible)- Keep the inline style but add a comment explaining the iOS zoom workaround for future maintainers
Option 1: Remove inline style
className={cn( 'placeholder:text-muted-foreground flex h-10 w-full rounded-md bg-transparent py-3 text-base outline-hidden disabled:cursor-not-allowed disabled:opacity-50', className )} - style={{ fontSize: '16px' }} {...props}Option 2: Add explanatory comment
className={cn( 'placeholder:text-muted-foreground flex h-10 w-full rounded-md bg-transparent py-3 text-base outline-hidden disabled:cursor-not-allowed disabled:opacity-50', className )} + // Explicit 16px prevents iOS Safari auto-zoom on input focus style={{ fontSize: '16px' }} {...props}apps/ui/src/components/views/github-prs-view.tsx (2)
73-80: Consider extracting to a shared utility.This
formatDatefunction is identical to the one inapps/ui/src/components/views/github-issues-view/utils.ts. Consider moving it to a shared location (e.g.,@/lib/utilsor a dedicated date utilities file) and importing from there to avoid duplication.♻️ Suggested refactor
- const formatDate = (dateString: string) => { - const date = new Date(dateString); - return date.toLocaleDateString('en-US', { - month: 'short', - day: 'numeric', - year: 'numeric', - }); - }; + // Import from shared utils instead: + // import { formatDate } from '@/lib/date-utils';Or import from the existing location if appropriate:
import { formatDate } from '@/components/views/github-issues-view/utils';
277-286: Avoid redundant function calls.
getReviewStatus(selectedPR)is called 4 times. Store the result in a variable at the start of the detail section to improve readability and avoid repeated computation.♻️ Suggested improvement
+ {(() => { + const reviewStatus = getReviewStatus(selectedPR); + return reviewStatus && ( <span className={cn( 'px-2 py-0.5 rounded-full text-xs font-medium', - getReviewStatus(selectedPR)!.bg, - getReviewStatus(selectedPR)!.color + reviewStatus.bg, + reviewStatus.color )} > - {getReviewStatus(selectedPR)!.label} + {reviewStatus.label} </span> + ); + })()} - )}Or compute it once before the JSX block and reference the variable.
apps/server/src/routes/worktree/routes/merge.ts (2)
9-13: DuplicateexecAsyncdefinition — import fromcommon.tsinstead.
execAsyncis already exported from../common.ts(as shown in the relevant code snippets). Remove the local definition and import it alongside the existing imports.♻️ Suggested fix
import type { Request, Response } from 'express'; -import { exec } from 'child_process'; -import { promisify } from 'util'; -import { getErrorMessage, logError } from '../common.js'; - -const execAsync = promisify(exec); +import { execAsync, getErrorMessage, logError } from '../common.js';
83-83: Consider includingtargetBranchin the response.The response only includes
mergedBranchbut not the target branch it was merged into. This information may be useful for the caller, especially now that the target is configurable.- res.json({ success: true, mergedBranch: branchName }); + res.json({ success: true, mergedBranch: branchName, targetBranch: mergeTarget });apps/ui/src/components/views/board-view/board-header.tsx (1)
131-140: Comment slightly inaccurate - button shows on tablet as well.The comment says "only shows when sidebar is closed on mobile" but
lg:hiddenmeans it's visible on both mobile and tablet screens (below 1024px). Not a functional issue, just a minor documentation inaccuracy.📝 Suggested comment fix
- {/* Sidebar toggle button - only shows when sidebar is closed on mobile */} + {/* Sidebar toggle button - shows when sidebar is closed on screens below lg breakpoint */}apps/ui/src/components/views/context-view.tsx (1)
756-762: Consider extracting the layout logic for improved readability.The IIFE pattern works correctly, but extracting
showListandshowDetailas component-level computed values would improve readability and allow reuse elsewhere in the component.📝 Alternative: Extract to component level
// Near other derived state (after isMobile declaration) const showList = !isMobile || !selectedFile; const showDetail = selectedFile !== null; // Then in JSX, replace the IIFE with direct usage: {showList && ( <div className={cn(...)}> {/* file list */} </div> )} {showDetail && selectedFile && ( <div className="flex-1 flex flex-col overflow-hidden"> {/* detail view */} </div> )} {!selectedFile && !isMobile && ( <div className="flex-1 flex items-center justify-center"> {/* empty state */} </div> )}apps/ui/src/components/views/memory-view.tsx (2)
339-343: Consider extracting the IIFE to named variables for clarity.The inline IIFE pattern works but is unconventional in React JSX. Since
showListandshowDetailare derived fromisMobileandselectedFile, consider computing them before the return statement for better readability.♻️ Suggested refactor
Move the computation before the return:
+ // On mobile, hide list when a file is selected + const showList = !isMobile || !selectedFile; + const showDetail = selectedFile !== null; + return ( <div className="flex-1 flex flex-col overflow-hidden content-bg" data-testid="memory-view"> {/* Header */} ... {/* Main content area with file list and editor */} - {/* On mobile, hide list when a file is selected */} - {(() => { - const showList = !isMobile || !selectedFile; - const showDetail = selectedFile !== null; - - return ( - <div className="flex-1 flex overflow-hidden"> + <div className="flex-1 flex overflow-hidden"> ... - </div> - ); - })()} + </div>
434-443: Add aria-label for accessibility on the icon-only back button.The back button only contains an icon without text. Screen reader users would benefit from an explicit label.
♿ Suggested fix
<Button variant="ghost" size="sm" onClick={() => setSelectedFile(null)} className="mr-1 -ml-1" + aria-label="Back to file list" > <ArrowLeft className="w-4 h-4" /> </Button>apps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsx (1)
78-83: Add aria-label for accessibility on the icon-only back button.Similar to the memory-view, this back button only contains an icon. Adding an aria-label improves accessibility for screen reader users.
♿ Suggested fix
{isMobile && ( - <Button variant="ghost" size="sm" onClick={onClose} className="mr-1 -ml-1"> + <Button variant="ghost" size="sm" onClick={onClose} className="mr-1 -ml-1" aria-label="Back to issues list"> <ArrowLeft className="h-4 w-4" /> </Button> )}apps/server/src/routes/worktree/routes/info.ts (1)
31-34: Good defensive sanitization for consistent path resolution.This sanitization ensures the worktree lookup uses the same path format as when worktrees are created. The inline comment referencing
create.tsis helpful for maintainability.Consider extracting this sanitization logic into a shared utility function (e.g.,
sanitizeFeatureIdincommon.ts) to ensure consistency and avoid duplication across all worktree routes.♻️ Optional: Extract to shared utility
In
apps/server/src/routes/worktree/common.ts:export function sanitizeFeatureId(featureId: string): string { return featureId.replace(/[^a-zA-Z0-9_-]/g, '-'); }Then in each route:
-const sanitizedFeatureId = featureId.replace(/[^a-zA-Z0-9_-]/g, '-'); +const sanitizedFeatureId = sanitizeFeatureId(featureId);libs/platform/src/editor.ts (1)
345-371: Remove or usegetTerminalInfo()(currently dead code).
It’s unused, andopenInTerminal()has its own platform branching anyway. Keeping this increases maintenance surface.apps/ui/src/store/app-store.ts (1)
434-435:pendingTerminalCwdshould ideally be scoped (or at least guarded) by project.
Because it’s global, a project switch before it’s consumed could open a terminal in a path unrelated to the active project. Consider storing{ cwd, projectPath }(and ignore if project mismatches) or clearing it on project change if that’s acceptable UX.Also applies to: 1092-1093, 2645-2648, 2739-2745, 1297-1298
apps/ui/src/components/layout/project-switcher/project-switcher.tsx (1)
270-280: Consider adding a backdrop overlay for mobile consistency.The responsive logic is well-structured: fixed overlay on mobile when open, hidden when closed, and always relative on desktop. However, the
Sidebarcomponent (sidebar.tsx, lines 257-263) includes a backdrop overlay (bg-black/50) when open on mobile to allow clicking outside to close the sidebar. TheProjectSwitcherlacks this, which may create an inconsistent UX where users can't tap outside to close it on mobile.If the intent is for both panels to share a single backdrop controlled elsewhere, this is fine. Otherwise, consider adding a similar backdrop for parity.
apps/ui/src/components/views/board-view/dialogs/view-worktree-changes-dialog.tsx (2)
11-17: Consider importingWorktreeInfofrom the existing types module.This local
WorktreeInfointerface duplicates a subset of the interface already defined inworktree-panel/types.ts. Importing from the shared types ensures consistency and reduces maintenance burden if the type evolves.♻️ Proposed refactor
-import { GitDiffPanel } from '@/components/ui/git-diff-panel'; +import { GitDiffPanel } from '@/components/ui/git-diff-panel'; +import type { WorktreeInfo } from '@/components/views/board-view/worktree-panel'; -interface WorktreeInfo { - path: string; - branch: string; - isMain: boolean; - hasChanges?: boolean; - changedFilesCount?: number; -} -Then update the props interface to use
Pickif you only need a subset:interface ViewWorktreeChangesDialogProps { open: boolean; onOpenChange: (open: boolean) => void; worktree: Pick<WorktreeInfo, 'path' | 'branch' | 'isMain' | 'hasChanges' | 'changedFilesCount'> | null; projectPath: string; }
55-55: Minor: Extra whitespace in className.There's a double space between
sm:max-h-[60vh]andoverflow-y-auto.✨ Suggested fix
- <div className="flex-1 sm:min-h-[600px] sm:max-h-[60vh] overflow-y-auto scrollbar-visible -mx-6 -mb-6"> + <div className="flex-1 sm:min-h-[600px] sm:max-h-[60vh] overflow-y-auto scrollbar-visible -mx-6 -mb-6">apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx (1)
394-394: Consider renaming thefeatureIdprop inGitDiffPanelfor semantic clarity.The
branchNamevalue is passed toGitDiffPanel'sfeatureIdprop via fallback (branchName || featureId). While this works as intended, the prop name doesn't reflect that it accepts a branch name when available. The prop could be renamed tobranchOrFeatureId,worktreeId, or similar to make the intent clearer and align with the worktree API it ultimately feeds to.apps/server/src/routes/worktree/routes/discard-changes.ts (2)
61-75: Verify: The three-step discard approach is sound but silently swallows errors.The discard sequence (
git reset HEAD,git checkout .,git clean -fd) is correct and follows Git best practices. Silently catching errors is acceptable since these commands may legitimately fail (e.g., nothing staged for reset).However, consider logging these errors at debug level for troubleshooting, even if they're expected to fail sometimes.
Optional: Add debug logging for suppressed errors
// 1. Reset any staged changes - await execAsync('git reset HEAD', { cwd: worktreePath }).catch(() => { - // Ignore errors - might fail if there's nothing staged - }); + await execAsync('git reset HEAD', { cwd: worktreePath }).catch((err) => { + // Expected to fail if nothing staged - log for debugging + console.debug('git reset HEAD (expected):', err.message); + });
20-111: Consider emitting events for frontend streaming.Per coding guidelines, server operations should use
createEventEmitter()fromlib/events.tsto emit events that stream to the frontend via WebSocket. This would allow the UI to show real-time progress during the discard operation.Currently, this handler only returns a final response. For a destructive operation like discarding changes, progress feedback could improve UX.
apps/ui/src/lib/electron.ts (1)
1865-1878: Minor: discardChanges mock always returns filesDiscarded: 0.The mock returns
filesDiscarded: 0even whendiscarded: true. While this works, a more realistic mock might return a non-zero count to better simulate the actual behavior.Optional: Return a realistic file count in the mock
discardChanges: async (worktreePath: string) => { console.log('[Mock] Discarding changes:', { worktreePath }); return { success: true, result: { discarded: true, - filesDiscarded: 0, + filesDiscarded: 3, filesRemaining: 0, branch: 'main', - message: 'Mock: Changes discarded successfully', + message: 'Discarded 3 files', }, }; },apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (2)
346-375: Consider: Dialog components are duplicated between mobile and desktop views.The
ViewWorktreeChangesDialog,ConfirmDialog, andDevServerLogsPanelare rendered in both the mobile view (lines 346-375) and desktop view (lines 532-560). Since they share the same state and handlers, they could be extracted to render once after the conditional.This is a minor refactor that would reduce code duplication and potential maintenance overhead.
Optional: Extract dialogs to render once
// Mobile/Desktop view code above... + // Shared dialogs rendered once + const dialogs = ( + <> + <ViewWorktreeChangesDialog + open={viewChangesDialogOpen} + onOpenChange={setViewChangesDialogOpen} + worktree={viewChangesWorktree} + projectPath={projectPath} + /> + <ConfirmDialog + open={discardChangesDialogOpen} + onOpenChange={setDiscardChangesDialogOpen} + onConfirm={handleConfirmDiscardChanges} + title="Discard Changes" + description={`Are you sure you want to discard all changes in "${discardChangesWorktree?.branch}"? This will reset staged changes, discard modifications to tracked files, and remove untracked files. This action cannot be undone.`} + icon={Undo2} + iconClassName="text-destructive" + confirmText="Discard Changes" + confirmVariant="destructive" + /> + <DevServerLogsPanel + open={logPanelOpen} + onClose={handleCloseLogPanel} + worktree={logPanelWorktree} + onStopDevServer={handleStopDevServer} + onOpenDevServerUrl={handleOpenDevServerUrl} + /> + </> + ); if (isMobile) { return ( <div className="..."> {/* Mobile content without dialogs */} - <ViewWorktreeChangesDialog ... /> - <ConfirmDialog ... /> - <DevServerLogsPanel ... /> + {dialogs} </div> ); } return ( <div className="..."> {/* Desktop content without dialogs */} - <ViewWorktreeChangesDialog ... /> - <ConfirmDialog ... /> - <DevServerLogsPanel ... /> + {dialogs} </div> );Also applies to: 532-560
205-228: Consider adding a loading state during discard operation.The
handleConfirmDiscardChangesfunction is async but there's no visual feedback while the discard operation is in progress. Users might click the confirm button multiple times or think the UI is frozen.Optional: Add loading state for discard operation
+ const [isDiscarding, setIsDiscarding] = useState(false); const handleConfirmDiscardChanges = useCallback(async () => { if (!discardChangesWorktree) return; + setIsDiscarding(true); try { const api = getHttpApiClient(); const result = await api.worktree.discardChanges(discardChangesWorktree.path); // ... rest of handler } catch (error) { // ... error handling + } finally { + setIsDiscarding(false); } }, [discardChangesWorktree, fetchWorktrees]);Then pass
isDiscardingtoConfirmDialogas a loading prop if it supports one.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
apps/server/src/routes/worktree/index.tsapps/server/src/routes/worktree/routes/diffs.tsapps/server/src/routes/worktree/routes/discard-changes.tsapps/server/src/routes/worktree/routes/file-diff.tsapps/server/src/routes/worktree/routes/info.tsapps/server/src/routes/worktree/routes/list-branches.tsapps/server/src/routes/worktree/routes/merge.tsapps/server/src/routes/worktree/routes/open-in-terminal.tsapps/server/src/routes/worktree/routes/status.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/components/layout/project-switcher/project-switcher.tsxapps/ui/src/components/layout/sidebar.tsxapps/ui/src/components/ui/command.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/views/board-view/board-search-bar.tsxapps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsxapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/components/views/board-view/dialogs/index.tsapps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsxapps/ui/src/components/views/board-view/dialogs/view-worktree-changes-dialog.tsxapps/ui/src/components/views/board-view/header-mobile-menu.tsxapps/ui/src/components/views/board-view/header-overflow-menu.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsxapps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsxapps/ui/src/components/views/board-view/worktree-panel/hooks/use-branches.tsapps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.tsapps/ui/src/components/views/board-view/worktree-panel/index.tsapps/ui/src/components/views/board-view/worktree-panel/types.tsapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsxapps/ui/src/components/views/context-view.tsxapps/ui/src/components/views/github-issues-view.tsxapps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsxapps/ui/src/components/views/github-issues-view/types.tsapps/ui/src/components/views/github-prs-view.tsxapps/ui/src/components/views/graph-view-page.tsxapps/ui/src/components/views/memory-view.tsxapps/ui/src/components/views/running-agents-view.tsxapps/ui/src/components/views/terminal-view.tsxapps/ui/src/hooks/use-media-query.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tsapps/ui/src/types/electron.d.tslibs/platform/src/editor.tslibs/platform/src/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (
@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/board-view/worktree-panel/index.tsapps/server/src/routes/worktree/routes/info.tsapps/ui/src/components/views/github-issues-view/types.tsapps/ui/src/components/views/board-view/dialogs/view-worktree-changes-dialog.tsxapps/server/src/routes/worktree/routes/diffs.tsapps/server/src/routes/worktree/routes/open-in-terminal.tsapps/server/src/routes/worktree/routes/status.tsapps/server/src/routes/worktree/routes/merge.tsapps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsxapps/ui/src/components/views/github-prs-view.tsxapps/server/src/routes/worktree/routes/file-diff.tsapps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsxapps/ui/src/components/views/board-view/dialogs/index.tsapps/ui/src/components/views/board-view/worktree-panel/types.tsapps/server/src/routes/worktree/routes/discard-changes.tsapps/ui/src/components/views/board-view/header-overflow-menu.tsxlibs/platform/src/index.tsapps/ui/src/components/views/board-view/header-mobile-menu.tsxlibs/platform/src/editor.tsapps/ui/src/components/views/board-view/worktree-panel/hooks/use-branches.tsapps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.tsapps/ui/src/components/layout/sidebar.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/server/src/routes/worktree/index.tsapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/store/app-store.tsapps/ui/src/components/views/github-issues-view.tsxapps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsxapps/ui/src/routes/__root.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/ui/command.tsxapps/ui/src/components/views/context-view.tsxapps/ui/src/components/layout/project-switcher/project-switcher.tsxapps/ui/src/components/views/running-agents-view.tsxapps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsxapps/ui/src/components/views/memory-view.tsxapps/ui/src/components/views/graph-view-page.tsxapps/ui/src/components/views/board-view/board-search-bar.tsxapps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsxapps/ui/src/hooks/use-media-query.tsapps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsxapps/ui/src/lib/http-api-client.tsapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsxapps/ui/src/lib/electron.tsapps/ui/src/types/electron.d.tsapps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsxapps/server/src/routes/worktree/routes/list-branches.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/terminal-view.tsxapps/server/src/services/auto-mode-service.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from@automaker/model-resolverto convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/board-view/worktree-panel/index.tsapps/server/src/routes/worktree/routes/info.tsapps/ui/src/components/views/github-issues-view/types.tsapps/ui/src/components/views/board-view/dialogs/view-worktree-changes-dialog.tsxapps/server/src/routes/worktree/routes/diffs.tsapps/server/src/routes/worktree/routes/open-in-terminal.tsapps/server/src/routes/worktree/routes/status.tsapps/server/src/routes/worktree/routes/merge.tsapps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsxapps/ui/src/components/views/github-prs-view.tsxapps/server/src/routes/worktree/routes/file-diff.tsapps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsxapps/ui/src/components/views/board-view/dialogs/index.tsapps/ui/src/components/views/board-view/worktree-panel/types.tsapps/server/src/routes/worktree/routes/discard-changes.tsapps/ui/src/components/views/board-view/header-overflow-menu.tsxlibs/platform/src/index.tsapps/ui/src/components/views/board-view/header-mobile-menu.tsxlibs/platform/src/editor.tsapps/ui/src/components/views/board-view/worktree-panel/hooks/use-branches.tsapps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.tsapps/ui/src/components/layout/sidebar.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/server/src/routes/worktree/index.tsapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/store/app-store.tsapps/ui/src/components/views/github-issues-view.tsxapps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsxapps/ui/src/routes/__root.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/ui/command.tsxapps/ui/src/components/views/context-view.tsxapps/ui/src/components/layout/project-switcher/project-switcher.tsxapps/ui/src/components/views/running-agents-view.tsxapps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsxapps/ui/src/components/views/memory-view.tsxapps/ui/src/components/views/graph-view-page.tsxapps/ui/src/components/views/board-view/board-search-bar.tsxapps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsxapps/ui/src/hooks/use-media-query.tsapps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsxapps/ui/src/lib/http-api-client.tsapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsxapps/ui/src/lib/electron.tsapps/ui/src/types/electron.d.tsapps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsxapps/server/src/routes/worktree/routes/list-branches.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/terminal-view.tsxapps/server/src/services/auto-mode-service.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/worktree/routes/info.tsapps/server/src/routes/worktree/routes/diffs.tsapps/server/src/routes/worktree/routes/open-in-terminal.tsapps/server/src/routes/worktree/routes/status.tsapps/server/src/routes/worktree/routes/merge.tsapps/server/src/routes/worktree/routes/file-diff.tsapps/server/src/routes/worktree/routes/discard-changes.tsapps/server/src/routes/worktree/index.tsapps/server/src/routes/worktree/routes/list-branches.tsapps/server/src/services/auto-mode-service.ts
🧠 Learnings (3)
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/server/src/routes/worktree/routes/info.tsapps/server/src/routes/worktree/routes/diffs.tsapps/server/src/routes/worktree/routes/status.tsapps/server/src/routes/worktree/routes/file-diff.tsapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view.tsxapps/server/src/services/auto-mode-service.ts
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/board-view/dialogs/view-worktree-changes-dialog.tsxapps/ui/src/components/views/board-view/worktree-panel/components/dev-server-logs-panel.tsxapps/ui/src/components/views/github-prs-view.tsxapps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsxapps/ui/src/components/views/board-view/header-overflow-menu.tsxapps/ui/src/components/views/board-view/header-mobile-menu.tsxapps/ui/src/components/layout/sidebar.tsxapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/components/views/github-issues-view.tsxapps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsxapps/ui/src/routes/__root.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/ui/command.tsxapps/ui/src/components/views/context-view.tsxapps/ui/src/components/layout/project-switcher/project-switcher.tsxapps/ui/src/components/views/running-agents-view.tsxapps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsxapps/ui/src/components/views/memory-view.tsxapps/ui/src/components/views/graph-view-page.tsxapps/ui/src/components/views/board-view/board-search-bar.tsxapps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsxapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/terminal-view.tsx
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Frontend UI must use TanStack Router for file-based routing, organize components in components/views/, implement stores with Zustand, and use custom hooks in hooks/ directory
Applied to files:
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts
🧬 Code graph analysis (13)
apps/ui/src/components/views/board-view/dialogs/view-worktree-changes-dialog.tsx (5)
apps/ui/src/components/views/board-view/worktree-panel/index.ts (1)
WorktreeInfo(3-3)apps/ui/src/components/views/board-view/worktree-panel/types.ts (1)
WorktreeInfo(9-18)apps/ui/src/lib/electron.ts (1)
WorktreeInfo(193-193)apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(625-630)apps/ui/src/components/views/board-view/dialogs/index.ts (1)
ViewWorktreeChangesDialog(11-11)
apps/server/src/routes/worktree/routes/open-in-terminal.ts (2)
libs/platform/src/editor.ts (1)
openInTerminal(384-464)libs/platform/src/index.ts (1)
openInTerminal(172-172)
apps/server/src/routes/worktree/routes/merge.ts (1)
apps/server/src/routes/worktree/common.ts (1)
execAsync(12-12)
apps/ui/src/components/views/github-prs-view.tsx (3)
apps/ui/src/hooks/use-media-query.ts (1)
useIsMobile(48-50)apps/ui/src/lib/utils.ts (1)
cn(6-8)apps/ui/src/components/views/github-issues-view/utils.ts (1)
formatDate(21-28)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (2)
apps/ui/src/components/views/board-view/worktree-panel/index.ts (2)
ConflictResolutionSource(8-8)WorktreeInfo(3-3)apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(625-630)
apps/server/src/routes/worktree/index.ts (4)
apps/server/src/middleware/validate-paths.ts (1)
validatePathParams(38-87)apps/server/src/routes/worktree/routes/open-in-terminal.ts (1)
createOpenInTerminalHandler(12-50)apps/server/src/routes/worktree/middleware.ts (1)
requireGitRepoOnly(72-75)apps/server/src/routes/worktree/routes/discard-changes.ts (1)
createDiscardChangesHandler(20-112)
apps/ui/src/components/views/github-issues-view.tsx (5)
apps/ui/src/hooks/use-media-query.ts (1)
useIsMobile(48-50)apps/ui/src/lib/utils.ts (1)
cn(6-8)apps/ui/src/components/views/github-issues-view/components/issues-list-header.tsx (1)
IssuesListHeader(12-38)apps/ui/src/components/views/github-issues-view/components/issue-row.tsx (1)
IssueRow(16-136)apps/ui/src/components/views/github-issues-view/utils.ts (1)
formatDate(21-28)
apps/ui/src/routes/__root.tsx (1)
apps/ui/src/hooks/use-media-query.ts (1)
useIsTablet(56-58)
apps/ui/src/components/views/board-view/board-header.tsx (2)
apps/ui/src/hooks/use-media-query.ts (3)
useIsNarrowToolbarForAgents(92-94)useIsNarrowToolbarForPlan(74-76)useIsNarrowToolbarForAutoMode(83-85)apps/ui/src/components/views/board-view/header-overflow-menu.tsx (1)
HeaderOverflowMenu(38-157)
apps/ui/src/components/views/memory-view.tsx (3)
apps/ui/src/hooks/use-media-query.ts (1)
useIsMobile(48-50)apps/ui/src/lib/utils.ts (1)
cn(6-8)apps/ui/src/components/ui/markdown.tsx (1)
Markdown(16-51)
apps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsx (3)
apps/ui/src/components/ui/button.tsx (1)
Button(108-108)apps/ui/src/lib/utils.ts (1)
cn(6-8)apps/ui/src/components/shared/model-override-trigger.tsx (1)
ModelOverrideTrigger(37-126)
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (4)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (3)
WorktreeInfo(9-18)PRInfo(42-65)ConflictResolutionSource(68-68)libs/types/src/settings.ts (1)
WorktreeInfo(576-587)apps/ui/src/components/views/board-view/worktree-panel/components/tooltip-wrapper.tsx (1)
TooltipWrapper(21-44)apps/ui/src/lib/utils.ts (1)
cn(6-8)
apps/ui/src/components/views/board-view.tsx (5)
apps/ui/src/components/views/board-view/worktree-panel/index.ts (2)
WorktreeInfo(3-3)ConflictResolutionSource(8-8)apps/ui/src/components/views/board-view/worktree-panel/types.ts (2)
WorktreeInfo(9-18)ConflictResolutionSource(68-68)apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(625-630)libs/types/src/settings.ts (1)
WorktreeInfo(576-587)libs/types/src/index.ts (1)
WorktreeInfo(130-130)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| // Validate source branch exists | ||
| try { | ||
| await execAsync(`git rev-parse --verify ${branchName}`, { cwd: projectPath }); |
There was a problem hiding this comment.
Command injection vulnerability — sanitize branch names before shell interpolation.
branchName and mergeTarget are user-supplied inputs interpolated directly into shell commands. A malicious branch name like "; rm -rf /" or $(malicious_command) could execute arbitrary code.
Use -- to separate options from arguments and validate/escape branch names, or use a library like execa with array arguments to avoid shell interpretation.
🔒 Suggested safer approach
- await execAsync(`git rev-parse --verify ${branchName}`, { cwd: projectPath });
+ // Validate branch name format (alphanumeric, hyphens, underscores, slashes)
+ const validBranchRegex = /^[a-zA-Z0-9_\-\/]+$/;
+ if (!validBranchRegex.test(branchName)) {
+ res.status(400).json({
+ success: false,
+ error: 'Invalid branch name format',
+ });
+ return;
+ }
+ await execAsync(`git rev-parse --verify -- "${branchName}"`, { cwd: projectPath });Apply similar validation and quoting for mergeTarget and all other git commands in this file (lines 50, 61-62, 68, 75, 78).
📝 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.
| await execAsync(`git rev-parse --verify ${branchName}`, { cwd: projectPath }); | |
| // Validate branch name format (alphanumeric, hyphens, underscores, slashes) | |
| const validBranchRegex = /^[a-zA-Z0-9_\-\/]+$/; | |
| if (!validBranchRegex.test(branchName)) { | |
| res.status(400).json({ | |
| success: false, | |
| error: 'Invalid branch name format', | |
| }); | |
| return; | |
| } | |
| await execAsync(`git rev-parse --verify -- "${branchName}"`, { cwd: projectPath }); |
| export function createOpenInTerminalHandler() { | ||
| return async (req: Request, res: Response): Promise<void> => { | ||
| try { | ||
| const { worktreePath } = req.body as { | ||
| worktreePath: string; | ||
| }; | ||
|
|
||
| if (!worktreePath) { | ||
| res.status(400).json({ | ||
| success: false, | ||
| error: 'worktreePath required', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Security: Validate that worktreePath is an absolute path | ||
| if (!isAbsolute(worktreePath)) { | ||
| res.status(400).json({ | ||
| success: false, | ||
| error: 'worktreePath must be an absolute path', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Use the platform utility to open in terminal | ||
| const result = await openInTerminal(worktreePath); | ||
| res.json({ | ||
| success: true, | ||
| result: { | ||
| message: `Opened terminal in ${worktreePath}`, | ||
| terminalName: result.terminalName, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| logError(error, 'Open in terminal failed'); | ||
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider emitting an event via createEventEmitter() for this server operation.
The repo guideline for apps/server/src/**/* says server operations should emit events that stream to the frontend. This endpoint currently doesn’t.
🤖 Prompt for AI Agents
In `@apps/server/src/routes/worktree/routes/open-in-terminal.ts` around lines 12 -
49, createOpenInTerminalHandler currently performs the open-in-terminal action
but doesn't emit a server event; update the handler to obtain/create the event
emitter (via createEventEmitter()) and emit a descriptive event (e.g.,
"worktree.openInTerminal" or similar) on success with payload including
worktreePath and result.terminalName, and emit a failure event on catch
including worktreePath and error details (use getErrorMessage(error)) before
sending the HTTP response; reference the createOpenInTerminalHandler function,
the openInTerminal result, and the catch block to add the emitter.emit calls.
Tighten authorization/validation: absolute path isn’t enough.
This endpoint can open a terminal at any absolute path on the machine. Please constrain worktreePath to a known worktree for the current project/user (or accept featureId and resolve the path server-side), and at least verify it exists + is a directory (ideally using realpath before allowlisting).
Possible hardening sketch
import type { Request, Response } from 'express';
import { isAbsolute } from 'path';
+import { realpath, stat } from 'fs/promises';
import { openInTerminal } from '@automaker/platform';
import { getErrorMessage, logError } from '../common.js';
export function createOpenInTerminalHandler() {
return async (req: Request, res: Response): Promise<void> => {
try {
const { worktreePath } = req.body as {
worktreePath: string;
};
if (!worktreePath) {
res.status(400).json({
success: false,
error: 'worktreePath required',
});
return;
}
// Security: Validate that worktreePath is an absolute path
if (!isAbsolute(worktreePath)) {
res.status(400).json({
success: false,
error: 'worktreePath must be an absolute path',
});
return;
}
+ const rp = await realpath(worktreePath);
+ const st = await stat(rp);
+ if (!st.isDirectory()) {
+ res.status(400).json({ success: false, error: 'worktreePath must be a directory' });
+ return;
+ }
+ // TODO: enforce rp is within an allowlisted project/worktree root
+
// Use the platform utility to open in terminal
const result = await openInTerminal(worktreePath);📝 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.
| export function createOpenInTerminalHandler() { | |
| return async (req: Request, res: Response): Promise<void> => { | |
| try { | |
| const { worktreePath } = req.body as { | |
| worktreePath: string; | |
| }; | |
| if (!worktreePath) { | |
| res.status(400).json({ | |
| success: false, | |
| error: 'worktreePath required', | |
| }); | |
| return; | |
| } | |
| // Security: Validate that worktreePath is an absolute path | |
| if (!isAbsolute(worktreePath)) { | |
| res.status(400).json({ | |
| success: false, | |
| error: 'worktreePath must be an absolute path', | |
| }); | |
| return; | |
| } | |
| // Use the platform utility to open in terminal | |
| const result = await openInTerminal(worktreePath); | |
| res.json({ | |
| success: true, | |
| result: { | |
| message: `Opened terminal in ${worktreePath}`, | |
| terminalName: result.terminalName, | |
| }, | |
| }); | |
| } catch (error) { | |
| logError(error, 'Open in terminal failed'); | |
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | |
| } | |
| }; | |
| import type { Request, Response } from 'express'; | |
| import { isAbsolute } from 'path'; | |
| import { realpath, stat } from 'fs/promises'; | |
| import { openInTerminal } from '@automaker/platform'; | |
| import { getErrorMessage, logError } from '../common.js'; | |
| export function createOpenInTerminalHandler() { | |
| return async (req: Request, res: Response): Promise<void> => { | |
| try { | |
| const { worktreePath } = req.body as { | |
| worktreePath: string; | |
| }; | |
| if (!worktreePath) { | |
| res.status(400).json({ | |
| success: false, | |
| error: 'worktreePath required', | |
| }); | |
| return; | |
| } | |
| // Security: Validate that worktreePath is an absolute path | |
| if (!isAbsolute(worktreePath)) { | |
| res.status(400).json({ | |
| success: false, | |
| error: 'worktreePath must be an absolute path', | |
| }); | |
| return; | |
| } | |
| const rp = await realpath(worktreePath); | |
| const st = await stat(rp); | |
| if (!st.isDirectory()) { | |
| res.status(400).json({ success: false, error: 'worktreePath must be a directory' }); | |
| return; | |
| } | |
| // TODO: enforce rp is within an allowlisted project/worktree root | |
| // Use the platform utility to open in terminal | |
| const result = await openInTerminal(worktreePath); | |
| res.json({ | |
| success: true, | |
| result: { | |
| message: `Opened terminal in ${worktreePath}`, | |
| terminalName: result.terminalName, | |
| }, | |
| }); | |
| } catch (error) { | |
| logError(error, 'Open in terminal failed'); | |
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | |
| } | |
| }; | |
| } |
| )} | ||
|
|
||
| <DropdownMenuSeparator /> | ||
|
|
||
| {/* Plan Button with Settings */} | ||
| <div |
There was a problem hiding this comment.
Unconditional separator can cause visual inconsistency.
When shouldCollapseAgents and shouldCollapseAutoMode are both false (e.g., only Plan collapsed into overflow), this results in consecutive separators between the "Controls" label and the Plan button.
📝 Suggested fix: Conditionally render the separator
- )}
+ )}
- <DropdownMenuSeparator />
+ {/* Only show separator if at least one item above it was rendered */}
+ {(shouldCollapseAgents || shouldCollapseAutoMode) && <DropdownMenuSeparator />}
{/* Plan Button with Settings */}🤖 Prompt for AI Agents
In `@apps/ui/src/components/views/board-view/header-overflow-menu.tsx` around
lines 133 - 138, The DropdownMenuSeparator is rendered unconditionally and can
produce adjacent separators when only certain sections are collapsed; update the
render logic in header-overflow-menu.tsx to conditionally render the
DropdownMenuSeparator only when at least one of shouldCollapseAgents or
shouldCollapseAutoMode (or any other adjacent collapsed section) is true so you
don’t produce consecutive separators between the "Controls" label and the Plan
button; locate the separator instance near the Plan button render and wrap it in
a conditional that checks shouldCollapseAgents || shouldCollapseAutoMode (or
equivalent collapse flags) before rendering.
| // Handle pending terminal cwd (from "open in terminal" action on worktree menu) | ||
| // When pendingTerminalCwd is set and we're ready, create a terminal with that cwd | ||
| const pendingTerminalCwdRef = useRef<string | null>(null); | ||
| const pendingTerminalCreatedRef = useRef<boolean>(false); | ||
| useEffect(() => { | ||
| const pendingCwd = terminalState.pendingTerminalCwd; | ||
|
|
||
| // Skip if no pending cwd | ||
| if (!pendingCwd) { | ||
| // Reset the created ref when there's no pending cwd | ||
| pendingTerminalCreatedRef.current = false; | ||
| pendingTerminalCwdRef.current = null; | ||
| return; | ||
| } | ||
|
|
||
| // Skip if we already created a terminal for this exact cwd | ||
| if (pendingCwd === pendingTerminalCwdRef.current && pendingTerminalCreatedRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| // Skip if still loading or terminal not enabled | ||
| if (loading || !status?.enabled) { | ||
| logger.debug('Waiting for terminal to be ready before creating terminal for pending cwd'); | ||
| return; | ||
| } | ||
|
|
||
| // Skip if password is required but not unlocked yet | ||
| if (status.passwordRequired && !terminalState.isUnlocked) { | ||
| logger.debug('Waiting for terminal unlock before creating terminal for pending cwd'); | ||
| return; | ||
| } | ||
|
|
||
| // Track that we're processing this cwd | ||
| pendingTerminalCwdRef.current = pendingCwd; | ||
|
|
||
| // Create a terminal with the pending cwd | ||
| logger.info('Creating terminal from pending cwd:', pendingCwd); | ||
|
|
||
| // Create terminal with the specified cwd | ||
| const createTerminalWithCwd = async () => { | ||
| try { | ||
| const headers: Record<string, string> = {}; | ||
| const authToken = useAppStore.getState().terminalState.authToken; | ||
| if (authToken) { | ||
| headers['X-Terminal-Token'] = authToken; | ||
| } | ||
|
|
||
| const response = await apiFetch('/api/terminal/sessions', 'POST', { | ||
| headers, | ||
| body: { cwd: pendingCwd, cols: 80, rows: 24 }, | ||
| }); | ||
| const data = await response.json(); | ||
|
|
||
| if (data.success) { | ||
| // Mark as successfully created | ||
| pendingTerminalCreatedRef.current = true; | ||
| addTerminalToLayout(data.data.id); | ||
| // Mark this session as new for running initial command | ||
| if (defaultRunScript) { | ||
| setNewSessionIds((prev) => new Set(prev).add(data.data.id)); | ||
| } | ||
| fetchServerSettings(); | ||
| toast.success(`Opened terminal in ${pendingCwd.split('/').pop() || pendingCwd}`); | ||
| // Clear the pending cwd after successful creation | ||
| setPendingTerminalCwd(null); | ||
| } else { | ||
| logger.error('Failed to create session from pending cwd:', data.error); | ||
| toast.error('Failed to open terminal', { | ||
| description: data.error || 'Unknown error', | ||
| }); | ||
| // Clear pending cwd on failure to prevent infinite retries | ||
| setPendingTerminalCwd(null); | ||
| } | ||
| } catch (err) { | ||
| logger.error('Create session error from pending cwd:', err); | ||
| toast.error('Failed to open terminal'); | ||
| // Clear pending cwd on error to prevent infinite retries | ||
| setPendingTerminalCwd(null); | ||
| } | ||
| }; | ||
|
|
||
| createTerminalWithCwd(); | ||
| }, [ | ||
| terminalState.pendingTerminalCwd, | ||
| terminalState.isUnlocked, | ||
| loading, | ||
| status?.enabled, | ||
| status?.passwordRequired, | ||
| setPendingTerminalCwd, | ||
| addTerminalToLayout, | ||
| defaultRunScript, | ||
| fetchServerSettings, | ||
| ]); |
There was a problem hiding this comment.
Fix potential duplicate terminal creation when pendingTerminalCwd is also consumed elsewhere.
Right now, pendingTerminalCwd can be used by createTerminal() (Line 931-934) and independently by the pending-cwd useEffect (Line 534+), and only the effect clears the pending value. If the user triggers a manual create while pendingTerminalCwd is set, the effect can still create a second terminal.
One way to de-duplicate (reuse createTerminal + clear pending once consumed)
- // Create terminal with the specified cwd
- const createTerminalWithCwd = async () => {
+ const createTerminalWithCwd = async () => {
try {
- const headers: Record<string, string> = {};
- const authToken = useAppStore.getState().terminalState.authToken;
- if (authToken) {
- headers['X-Terminal-Token'] = authToken;
- }
-
- const response = await apiFetch('/api/terminal/sessions', 'POST', {
- headers,
- body: { cwd: pendingCwd, cols: 80, rows: 24 },
- });
- const data = await response.json();
-
- if (data.success) {
- // Mark as successfully created
- pendingTerminalCreatedRef.current = true;
- addTerminalToLayout(data.data.id);
- // Mark this session as new for running initial command
- if (defaultRunScript) {
- setNewSessionIds((prev) => new Set(prev).add(data.data.id));
- }
- fetchServerSettings();
- toast.success(`Opened terminal in ${pendingCwd.split('/').pop() || pendingCwd}`);
- // Clear the pending cwd after successful creation
- setPendingTerminalCwd(null);
- } else {
- logger.error('Failed to create session from pending cwd:', data.error);
- toast.error('Failed to open terminal', {
- description: data.error || 'Unknown error',
- });
- // Clear pending cwd on failure to prevent infinite retries
- setPendingTerminalCwd(null);
- }
+ await createTerminal(undefined, undefined, pendingCwd);
+ pendingTerminalCreatedRef.current = true;
+ setPendingTerminalCwd(null); // clear once consumed
} catch (err) {
logger.error('Create session error from pending cwd:', err);
toast.error('Failed to open terminal');
// Clear pending cwd on error to prevent infinite retries
setPendingTerminalCwd(null);
}
};- const createTerminal = async (
+ const createTerminal = async (
direction?: 'horizontal' | 'vertical',
targetSessionId?: string,
cwd?: string
) => {
@@
- const terminalCwd =
- cwd || terminalState.pendingTerminalCwd || currentProject?.path || undefined;
+ const usedPending = !cwd && !!terminalState.pendingTerminalCwd;
+ const terminalCwd = cwd || terminalState.pendingTerminalCwd || currentProject?.path || undefined;
@@
if (data.success) {
addTerminalToLayout(data.data.id, direction, targetSessionId);
+ if (usedPending) setPendingTerminalCwd(null);Also applies to: 913-972
🤖 Prompt for AI Agents
In `@apps/ui/src/components/views/terminal-view.tsx` around lines 530 - 622, The
pending-cwd effect can race with the existing createTerminal flow and produce
duplicate terminals; modify createTerminal (the shared function used at Lines
~931-934) to consume and clear terminalState.pendingTerminalCwd atomically:
check terminalState.pendingTerminalCwd at start, use that value when creating
the session (instead of letting the effect also do it), call
setPendingTerminalCwd(null) immediately after successfully enqueuing/creating
the session (and also on error), and remove or guard the effect’s creation so it
only runs when createTerminal hasn't already consumed the pending value (e.g.,
have the effect return early if pendingTerminalCwdRef.current === pendingCwd or
if a consumed flag is set). Ensure you still set
pendingTerminalCreatedRef.current / call addTerminalToLayout and
setNewSessionIds in the same success path so behavior remains unchanged.
| const createTerminalWithCwd = async () => { | ||
| try { | ||
| const headers: Record<string, string> = {}; | ||
| const authToken = useAppStore.getState().terminalState.authToken; | ||
| if (authToken) { | ||
| headers['X-Terminal-Token'] = authToken; | ||
| } | ||
|
|
||
| const response = await apiFetch('/api/terminal/sessions', 'POST', { | ||
| headers, | ||
| body: { cwd: pendingCwd, cols: 80, rows: 24 }, | ||
| }); | ||
| const data = await response.json(); | ||
|
|
||
| if (data.success) { | ||
| // Mark as successfully created | ||
| pendingTerminalCreatedRef.current = true; | ||
| addTerminalToLayout(data.data.id); | ||
| // Mark this session as new for running initial command | ||
| if (defaultRunScript) { | ||
| setNewSessionIds((prev) => new Set(prev).add(data.data.id)); | ||
| } | ||
| fetchServerSettings(); | ||
| toast.success(`Opened terminal in ${pendingCwd.split('/').pop() || pendingCwd}`); | ||
| // Clear the pending cwd after successful creation | ||
| setPendingTerminalCwd(null); | ||
| } else { | ||
| logger.error('Failed to create session from pending cwd:', data.error); | ||
| toast.error('Failed to open terminal', { | ||
| description: data.error || 'Unknown error', | ||
| }); | ||
| // Clear pending cwd on failure to prevent infinite retries | ||
| setPendingTerminalCwd(null); | ||
| } | ||
| } catch (err) { | ||
| logger.error('Create session error from pending cwd:', err); | ||
| toast.error('Failed to open terminal'); | ||
| // Clear pending cwd on error to prevent infinite retries | ||
| setPendingTerminalCwd(null); |
There was a problem hiding this comment.
Toast basename extraction isn’t Windows-safe.
pendingCwd.split('/') won’t handle C:\path\to\dir. Consider splitting on both separators.
🤖 Prompt for AI Agents
In `@apps/ui/src/components/views/terminal-view.tsx` around lines 569 - 607, In
createTerminalWithCwd, the toast success uses pendingCwd.split('/') which breaks
on Windows paths; change the basename extraction to split on both separators
(e.g., pendingCwd?.split(/[/\\]/).pop() || pendingCwd) or compute a safe
basename variable before calling toast.success, and use that variable in
toast.success and any related logs; ensure pendingCwd is null-checked
(pendingCwd?) to avoid exceptions when clearing the cwd.
| style={{ marginRight: !isTablet && streamerPanelOpen ? '250px' : '0' }} | ||
| > | ||
| <Outlet /> | ||
| </div> | ||
|
|
||
| {/* Hidden streamer panel - opens with "\" key, pushes content */} | ||
| {/* Hidden streamer panel - opens with "\" key, pushes content - desktop only */} | ||
| <div | ||
| className={`fixed top-0 right-0 h-full w-[250px] bg-background border-l border-border transition-transform duration-300 ${ | ||
| className={`fixed top-0 right-0 h-full w-[250px] bg-background border-l border-border transition-transform duration-300 hidden lg:block ${ | ||
| streamerPanelOpen ? 'translate-x-0' : 'translate-x-full' | ||
| }`} | ||
| /> |
There was a problem hiding this comment.
Breakpoint mismatch at 1024px boundary.
The useIsTablet() hook uses max-width: 1024px while the CSS uses lg:block (min-width: 1024px). At exactly 1024px viewport width, both conditions are satisfied: isTablet is true (no margin applied) but lg:block makes the panel visible, causing the panel to overlap content when open.
Consider aligning the breakpoints:
Suggested fix in use-media-query.ts
export function useIsTablet(): boolean {
- return useMediaQuery('(max-width: 1024px)');
+ return useMediaQuery('(max-width: 1023px)');
}Alternatively, use a CSS variable or consistent threshold across both JS and CSS logic.
🤖 Prompt for AI Agents
In `@apps/ui/src/routes/__root.tsx` around lines 826 - 836, The breakpoint
inconsistency causes the streamer panel to overlap at 1024px because
useIsTablet() uses max-width:1024px while the JSX/CSS uses the Tailwind lg
breakpoint (min-width:1024px) for lg:block; to fix, make the threshold
consistent by updating useIsTablet (or its underlying use-media-query) to use
max-width:1023px (so tablet is <1024) or change the CSS logic to use
min-width:1025px equivalent, and ensure the JSX logic around streamerPanelOpen
and the inline marginRight calculation (where isTablet is referenced) aligns
with the same numeric breakpoint so the panel visibility (lg:block) and isTablet
boolean never both apply at 1024px.
| export async function openInTerminal(targetPath: string): Promise<{ terminalName: string }> { | ||
| if (isMac) { | ||
| // Use AppleScript to open Terminal.app in the specified directory | ||
| const script = ` | ||
| tell application "Terminal" | ||
| do script "cd ${targetPath.replace(/"/g, '\\"').replace(/\$/g, '\\$')}" | ||
| activate | ||
| end tell | ||
| `; | ||
| await execFileAsync('osascript', ['-e', script]); | ||
| return { terminalName: 'Terminal' }; | ||
| } else if (isWindows) { |
There was a problem hiding this comment.
Block shell injection in the macOS AppleScript cd command.
targetPath is interpolated into a shell command executed by Terminal; escaping only " and $ is not sufficient (e.g., backticks, newlines). Please use AppleScript’s quoted form of (and avoid embedding raw shell).
Proposed fix
export async function openInTerminal(targetPath: string): Promise<{ terminalName: string }> {
if (isMac) {
// Use AppleScript to open Terminal.app in the specified directory
- const script = `
- tell application "Terminal"
- do script "cd ${targetPath.replace(/"/g, '\\"').replace(/\$/g, '\\$')}"
- activate
- end tell
- `;
+ const script = `
+ tell application "Terminal"
+ do script "cd " & quoted form of ${JSON.stringify(targetPath)}
+ activate
+ end tell
+ `;
await execFileAsync('osascript', ['-e', script]);
return { terminalName: 'Terminal' };
} else if (isWindows) {🤖 Prompt for AI Agents
In `@libs/platform/src/editor.ts` around lines 384 - 395, openInTerminal currently
interpolates targetPath directly into an AppleScript shell command (script
variable) causing possible shell injection; instead avoid embedding raw shell
content and use AppleScript's safe quoting: build an AppleScript that assigns
the path to a variable and uses quoted form of POSIX path (or quoted form of
POSIX file) when composing the do script command, and stop directly
concatenating targetPath into the shell string; update the script generation in
openInTerminal (and the execFileAsync call) to use this AppleScript quoting
approach so all special characters in targetPath are safely handled.
| } else { | ||
| // Linux: Try common terminal emulators in order | ||
| const terminals = [ | ||
| { | ||
| name: 'GNOME Terminal', | ||
| command: 'gnome-terminal', | ||
| args: ['--working-directory', targetPath], | ||
| }, | ||
| { name: 'Konsole', command: 'konsole', args: ['--workdir', targetPath] }, | ||
| { | ||
| name: 'xfce4-terminal', | ||
| command: 'xfce4-terminal', | ||
| args: ['--working-directory', targetPath], | ||
| }, | ||
| { name: 'xterm', command: 'xterm', args: ['-e', `cd "${targetPath}" && $SHELL`] }, | ||
| { | ||
| name: 'x-terminal-emulator', | ||
| command: 'x-terminal-emulator', | ||
| args: ['--working-directory', targetPath], | ||
| }, | ||
| ]; | ||
|
|
||
| for (const terminal of terminals) { | ||
| if (await commandExists(terminal.command)) { | ||
| await execFileAsync(terminal.command, terminal.args); | ||
| return { terminalName: terminal.name }; | ||
| } | ||
| } | ||
|
|
||
| throw new Error('No terminal emulator found'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l libs/platform/src/editor.tsRepository: AutoMaker-Org/automaker
Length of output: 98
🏁 Script executed:
head -100 libs/platform/src/editor.ts | cat -nRepository: AutoMaker-Org/automaker
Length of output: 3452
🏁 Script executed:
sed -n '350,480p' libs/platform/src/editor.ts | cat -nRepository: AutoMaker-Org/automaker
Length of output: 4347
Linux terminal launch blocks the caller; use non-blocking spawn pattern like Windows.
The Linux implementation uses await execFileAsync(), which waits for the child process to exit. For terminal emulators like xterm (especially with -e), this blocks the server/CLI until the terminal closes. Use the same non-blocking pattern as Windows: spawn() with detached: true, stdio: 'ignore', and child.unref().
Proposed fix
for (const terminal of terminals) {
if (await commandExists(terminal.command)) {
- await execFileAsync(terminal.command, terminal.args);
+ const child: ChildProcess = spawn(terminal.command, terminal.args, {
+ stdio: 'ignore',
+ detached: true,
+ });
+ child.unref();
return { terminalName: terminal.name };
}
}🤖 Prompt for AI Agents
In `@libs/platform/src/editor.ts` around lines 433 - 463, The Linux branch
currently awaits execFileAsync for each terminal (see terminals array and the
usage of execFileAsync(terminal.command, terminal.args)), which blocks until the
terminal exits; modify this to use child_process.spawn in a non-blocking way:
call spawn(terminal.command, terminal.args, { detached: true, stdio: 'ignore'
}), do not await it, and call child.unref() after spawn; keep the commandExists
check and return { terminalName: terminal.name } immediately after spawning, and
throw the same Error('No terminal emulator found') if none are available.
![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 new features and significant UI/UX improvements, particularly for worktree management and mobile responsiveness. Key backend changes include new API endpoints for opening a terminal in a worktree and discarding changes, along with updates to existing endpoints to sanitize feature IDs and allow merging into a specified target branch. The discard-changes endpoint, however, has a review comment suggesting more robust error logging for git commands to prevent silent failures.
On the frontend, a new ViewWorktreeChangesDialog and ConfirmDialog for discarding changes have been added. The WorktreePanel now includes actions to open a terminal, view changes, and discard changes, with the 'Pull & Resolve Conflicts' action offering options to pull from the worktree's own remote branch or the main worktree's branch. A review comment highlights a discrepancy where the UI's "Open in Terminal" action currently navigates to the in-app terminal, while a new backend endpoint was created for opening a native OS terminal, suggesting the UI should be updated to use the native terminal API.
UI/UX enhancements focus on responsive design across various views: the ProjectSwitcher, Sidebar, ContextView, GitHubIssuesView, GitHubPRsView, and MemoryView have been updated to improve their layout and functionality on mobile devices. This includes conditional rendering of elements, dynamic sizing, and the introduction of a HeaderOverflowMenu in BoardHeader to manage controls (Agents, Auto Mode, Plan) that collapse based on screen width. Additionally, the use-media-query hook has been expanded with new breakpoints for responsive toolbar elements, and the AgentOutputModal now passes the branch name to the GitDiffPanel for more accurate diff viewing. A minor cleanup suggestion was made to remove an unused getTerminalInfo function in the platform library.
| await execAsync('git reset HEAD', { cwd: worktreePath }).catch(() => { | ||
| // Ignore errors - might fail if there's nothing staged | ||
| }); | ||
|
|
||
| // 2. Discard changes in tracked files | ||
| await execAsync('git checkout .', { cwd: worktreePath }).catch(() => { | ||
| // Ignore errors - might fail if there are no tracked changes | ||
| }); | ||
|
|
||
| // 3. Remove untracked files and directories | ||
| await execAsync('git clean -fd', { cwd: worktreePath }).catch(() => { | ||
| // Ignore errors - might fail if there are no untracked files | ||
| }); |
There was a problem hiding this comment.
Silently catching and ignoring all errors from these execAsync calls is risky. While the comments mention expected failures (e.g., nothing to clean), this will also suppress unexpected errors like file system permissions issues, which could leave the worktree in an inconsistent state without any indication to the user or in the logs. It would be more robust to log these errors, so you can monitor for unexpected failures while still not blocking the discard operation.
await execAsync('git reset HEAD', { cwd: worktreePath }).catch((error) => {
// Ignore errors - might fail if there's nothing staged
logError(error, 'Ignoring error from `git reset HEAD`');
});
// 2. Discard changes in tracked files
await execAsync('git checkout .', { cwd: worktreePath }).catch((error) => {
// Ignore errors - might fail if there are no tracked changes
logError(error, 'Ignoring error from `git checkout .`');
});
// 3. Remove untracked files and directories
await execAsync('git clean -fd', { cwd: worktreePath }).catch((error) => {
// Ignore errors - might fail if there are no untracked files
logError(error, 'Ignoring error from `git clean -fd`');
});| const handleOpenInTerminal = useCallback( | ||
| (worktree: WorktreeInfo) => { | ||
| // Set the pending terminal cwd to the worktree path | ||
| setPendingTerminalCwd(worktree.path); | ||
| // Navigate to the terminal page | ||
| navigate({ to: '/terminal' }); | ||
| logger.info('Opening terminal for worktree:', worktree.path); | ||
| }, | ||
| [navigate, setPendingTerminalCwd] | ||
| ); |
There was a problem hiding this comment.
There's a discrepancy between the UI implementation and the new backend capability for the "Open in Terminal" feature. This implementation navigates to the in-app terminal view, but a new backend endpoint (/api/worktree/open-in-terminal) was added to open a native OS terminal, which seems to be the intended feature based on the PR description. To provide the cross-platform native terminal experience, this should call the backend API instead.
You'll need to import getHttpApiClient and toast to use the suggested code.
const handleOpenInTerminal = useCallback(async (worktree: WorktreeInfo) => {
const api = getHttpApiClient();
try {
const result = await api.worktree.openInTerminal(worktree.path);
if (result.success) {
toast.success(result.result?.message || `Opened terminal for ${worktree.branch}`);
} else {
toast.error('Failed to open terminal', { description: result.error });
}
} catch (error) {
logger.error('Failed to open terminal', error);
toast.error('Failed to open terminal', { description: error instanceof Error ? error.message : 'Unknown error' });
}
}, []);| function getTerminalInfo(): { name: string; command: string; args: string[] } { | ||
| if (isMac) { | ||
| // On macOS, use Terminal.app with AppleScript to open in a specific directory | ||
| return { | ||
| name: 'Terminal', | ||
| command: 'open', | ||
| args: ['-a', 'Terminal'], | ||
| }; | ||
| } else if (isWindows) { | ||
| // On Windows, use Windows Terminal if available, otherwise cmd | ||
| return { | ||
| name: 'Windows Terminal', | ||
| command: 'wt', | ||
| args: ['-d'], | ||
| }; | ||
| } else { | ||
| // On Linux, try common terminal emulators in order of preference | ||
| return { | ||
| name: 'Terminal', | ||
| command: 'x-terminal-emulator', | ||
| args: ['--working-directory'], | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
This getTerminalInfo function appears to be unused. The openInTerminal function implements its own platform-specific logic directly. To improve code maintainability, this dead code should be removed.
Summary
Community contribution from Discord member eclipxe with various improvements:
Changes
Test plan
Credits
Thanks to eclipxe from Discord for this contribution!
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.