pull-request #173
pull-request #173
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds PR discovery, creation, and persistence for worktrees; surfaces PR status and actions in the board UI; propagates PR URLs into features; adds server endpoints to fetch PR details/comments; and introduces branch validation, gh CLI detection, and filesystem-backed worktree metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI (board-view)
participant Client as HttpApiClient / Electron
participant Server as Automaker Server
participant FS as .automaker/worktrees (metadata)
UI->>Client: POST /api/worktree/create-pr (worktreePath, branch, projectPath?)
Client->>Server: forward request
Server->>Server: inspect remotes, determine repo & fork context
Server->>Server: gh pr list (check existing PR for branch)
alt PR exists
Server->>FS: updateWorktreePRInfo(branch, prInfo)
Server-->>Client: { success, prUrl, prNumber, prAlreadyExisted }
else create PR
Server->>Server: gh pr create (or build browser URL)
Server->>FS: updateWorktreePRInfo(branch, prInfo)
Server-->>Client: { success, prUrl, prNumber, prCreated }
end
Client-->>UI: return pr metadata
UI->>UI: board-view.onCreated(prUrl) — update features on branch (state + persist)
UI->>Client: POST /api/worktree/pr-info (worktreePath, branchName)
Client->>Server: forward request
Server->>Server: gh queries -> fetch PR, comments, reviewComments
Server-->>Client: { success, prInfo }
Client-->>UI: deliver prInfo
UI->>UI: invoke onAddressPRComments(worktree, prInfo)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @webdevcody, 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 implements a new feature that allows the application to track and display Pull Request (PR) URLs directly within the feature management interface. By extending the "Feature" data model with a "prUrl" field, updating the UI to show a clickable PR link on Kanban cards, and automating the capture and persistence of this URL during PR creation, the changes significantly improve the workflow by providing immediate access to associated PRs from the feature board. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for tracking pull request URLs associated with features. The implementation is well-structured, touching the data store, UI components, and adding integration tests to ensure correctness. The changes are logical and cover the full lifecycle of the new prUrl property, from creation to display. I've included a few suggestions to enhance robustness and user experience. Specifically, I've pointed out an opportunity to improve asynchronous handling when updating features, a way to make the new pull request link on the Kanban card more informative, and a best practice for writing more reliable tests by avoiding fixed timeouts.
| hookFeatures | ||
| .filter((f) => f.branchName === branchName) | ||
| .forEach((feature) => { | ||
| updateFeature(feature.id, { prUrl }); | ||
| persistFeatureUpdate(feature.id, { prUrl }); | ||
| }); |
There was a problem hiding this comment.
The use of forEach with an async function (persistFeatureUpdate) can lead to unhandled promises and potential race conditions, as forEach does not wait for promises to resolve. It's better to separate the synchronous state updates from the asynchronous persistence operations. You can update the local state for all features first, and then use Promise.all to handle all persistence calls concurrently. This ensures that all persistence operations are tracked and any potential errors are caught.
const featuresToUpdate = hookFeatures.filter((f) => f.branchName === branchName);
// Update local state synchronously
featuresToUpdate.forEach((feature) => {
updateFeature(feature.id, { prUrl });
});
// Persist changes asynchronously and in parallel
Promise.all(
featuresToUpdate.map((feature) =>
persistFeatureUpdate(feature.id, { prUrl })
)
).catch(console.error);
| {feature.prUrl && ( | ||
| <div className="mb-2"> | ||
| <a | ||
| href={feature.prUrl} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| onClick={(e) => e.stopPropagation()} | ||
| onPointerDown={(e) => e.stopPropagation()} | ||
| className="inline-flex items-center gap-1.5 text-[11px] text-purple-500 hover:text-purple-400 transition-colors" | ||
| title={feature.prUrl} | ||
| data-testid={`pr-url-${feature.id}`} | ||
| > | ||
| <GitPullRequest className="w-3 h-3 shrink-0" /> | ||
| <span className="truncate max-w-[150px]">Pull Request</span> | ||
| <ExternalLink className="w-2.5 h-2.5 shrink-0" /> | ||
| </a> | ||
| </div> |
There was a problem hiding this comment.
Displaying the pull request number would provide more context than the generic "Pull Request" text. You can extract the PR number from the prUrl and display it. Using an IIFE (Immediately Invoked Function Expression) allows you to declare a variable for the PR number cleanly within the JSX.
{feature.prUrl && (() => {
const prNumber = feature.prUrl.split('/').pop();
return (
<div className="mb-2">
<a
href={feature.prUrl}
target="_blank"
rel="noopener noreferrer"
onClick={(e) => e.stopPropagation()}
onPointerDown={(e) => e.stopPropagation()}
className="inline-flex items-center gap-1.5 text-[11px] text-purple-500 hover:text-purple-400 transition-colors"
title={feature.prUrl}
data-testid={`pr-url-${feature.id}`}
>
<GitPullRequest className="w-3 h-3 shrink-0" />
<span className="truncate max-w-[150px]">
{prNumber ? `Pull Request #${prNumber}` : 'Pull Request'}
</span>
<ExternalLink className="w-2.5 h-2.5 shrink-0" />
</a>
</div>
);
})()}
| category: "Testing", | ||
| }); | ||
| await confirmAddFeature(page); | ||
| await page.waitForTimeout(1000); |
There was a problem hiding this comment.
Using page.waitForTimeout() can lead to flaky tests. It's better to wait for a specific condition to be met. In this case, you could wait for the feature file to be created on the filesystem, or for the corresponding UI element to appear. Playwright's waitForFunction or expect().toPass() are great for these kinds of assertions. This comment applies to other uses of waitForTimeout in this test file as well.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/app/src/components/views/board-view.tsx (1)
1153-1170: Branch-level PR URL propagation looks correctUsing
onCreated(prUrl)to update and persistprUrlon all features whosebranchNamematches the selected worktree’s branch is consistent with the “one PR per branch” model, and the guards aroundprUrlandselectedWorktreeForAction?.branchavoid null/undefined issues. If this ever needs to scale to many features per branch, you might consider a small helper or batching to avoid repeatedpersistFeatureUpdatecalls, but it’s fine as-is.apps/app/tests/worktree-integration.spec.ts (1)
2613-2735: PR URL integration tests are well targeted and consistent with existing patternsBoth tests correctly exercise the new
prUrlfield end-to-end (JSON storage → UI render → persistence across edits) using the same filesystem + UI flow as the surrounding worktree tests. The additionalwaitForTimeoutcalls mirror existing tests and should be acceptable here; if these ever get flaky, you could tighten them with file-based polling or more specific UI waits.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/app/src/components/views/board-view.tsx(1 hunks)apps/app/src/components/views/board-view/components/kanban-card.tsx(2 hunks)apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx(2 hunks)apps/app/src/store/app-store.ts(1 hunks)apps/app/tests/worktree-integration.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/app/tests/worktree-integration.spec.ts (3)
apps/app/tests/utils/git/worktree.ts (2)
setupProjectWithPath(309-353)waitForBoardView(462-464)apps/app/tests/utils/core/waiting.ts (1)
waitForNetworkIdle(7-9)apps/app/tests/utils/views/board.ts (3)
clickAddFeature(121-126)fillAddFeatureDialog(131-180)confirmAddFeature(185-192)
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (2)
apps/app/src/store/app-store.ts (1)
281-309: Feature model extension withprUrlis safe and consistentAdding
prUrl?: stringtoFeatureis an additive, backward-compatible change; it fits existingupdateFeature/persistFeatureUpdateflows and will be naturally persisted via the existingfeaturesslice without requiring a migration bump.apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx (1)
29-41:onCreated(prUrl?)wiring correctly encodes PR vs non-PR outcomesThe updated
onCreated: (prUrl?: string) => voidcontract plus thehandleCloselogic cleanly distinguish cases:
- Successful PR →
onCreated(prUrl)fired on close.- Branch push with browser fallback or gh error →
onCreated(undefined)on close, so callers can refresh without assigning a PR URL.- Plain push/no PR & no browser URL →
onCreated()fromhandleCreate, dialog closes immediately andhandleClosedoesn’t fire a second time.This matches the BoardView usage and avoids double-notification.
Also applies to: 200-209
- Added logic to display a Verify button for features in the "waiting_approval" status with a PR URL, replacing the Commit button. - Updated WorktreePanel and WorktreeTab components to include properties for tracking uncommitted changes and file counts. - Implemented tooltips to indicate the number of uncommitted files in the WorktreeTab. - Added integration tests to verify the correct display of the Verify and Commit buttons based on feature status and PR URL presence.
- Added a new route for fetching PR info, allowing users to retrieve details about existing pull requests associated with worktrees. - Updated the create PR handler to store metadata for existing PRs and handle cases where a PR already exists. - Enhanced the UI components to display PR information, including a new button to address PR comments directly from the worktree panel. - Improved the overall user experience by integrating PR state indicators and ensuring seamless interaction with the GitHub CLI for PR management.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
apps/ui/src/components/views/board-view.tsx (1)
1254-1267: Duplicate: Unhandled promises in feature update loop.This issue has already been identified in the past review comments. The
forEachwith asyncpersistFeatureUpdatecalls can lead to unhandled promise rejections and race conditions.As noted in the previous review, use
Promise.allto properly handle the async persistence operations.
🧹 Nitpick comments (4)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (1)
1-7: Consider unifying common PR fields and removing redundantisReviewCommentflag.
WorktreePRInfoandPRInfosharenumber,url,title,statefields. Consider extracting a base type or havingPRInfoextendWorktreePRInfo.The
isReviewCommentfield in bothcommentsandreviewCommentsarrays is redundant since the array membership already distinguishes them.🔎 Optional refactor to reduce duplication
+interface BasePRComment { + id: number; + author: string; + body: string; + createdAt: string; +} + +interface PRIssueComment extends BasePRComment {} + +interface PRReviewComment extends BasePRComment { + path?: string; + line?: number; +} + export interface PRInfo { number: number; title: string; url: string; state: string; author: string; body: string; - comments: Array<{ - id: number; - author: string; - body: string; - createdAt: string; - isReviewComment: boolean; - }>; - reviewComments: Array<{ - id: number; - author: string; - body: string; - path?: string; - line?: number; - createdAt: string; - isReviewComment: boolean; - }>; + comments: PRIssueComment[]; + reviewComments: PRReviewComment[]; }Also applies to: 37-60
apps/server/src/routes/worktree/routes/pr-info.ts (1)
12-43: Extract PATH extension logic to a shared utility.This PATH extension pattern for locating
ghandgitbinaries is duplicated in bothpr-info.tsandcreate-pr.ts. Moving this ~30-line block to a shared utility module would improve maintainability and reduce duplication.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (2)
185-217: PR state badge color is hardcoded to green regardless of actual state.The dropdown label at line 191 always uses
bg-green-500/20 text-green-600for the PR state badge, but PRs can be in different states (open, closed, merged, draft). Consider varying the color based on the state for visual consistency with the PR badge inworktree-tab.tsx.🔎 Proposed enhancement for state-based coloring
+ const stateClasses = (() => { + const state = worktree.pr.state?.toLowerCase(); + switch (state) { + case "open": + case "reopened": + return "bg-green-500/20 text-green-600"; + case "draft": + return "bg-amber-500/20 text-amber-600"; + case "merged": + return "bg-purple-500/20 text-purple-600"; + case "closed": + return "bg-rose-500/20 text-rose-600"; + default: + return "bg-muted text-muted-foreground"; + } + })(); <DropdownMenuLabel className="text-xs flex items-center gap-2"> <GitPullRequest className="w-3 h-3" /> PR #{worktree.pr.number} - <span className="ml-auto text-[10px] bg-green-500/20 text-green-600 px-1.5 py-0.5 rounded"> + <span className={cn("ml-auto text-[10px] px-1.5 py-0.5 rounded", stateClasses)}> {worktree.pr.state} </span> </DropdownMenuLabel>
196-209: Consider documenting the partial PRInfo contract more explicitly.The code correctly constructs a partial
PRInfoobject with empty fields that will be fetched later. The inline comment explains this, but since this creates a contract between this component and the handler, consider adding a brief JSDoc or ensuring the handler gracefully handles these empty fields.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/server/src/lib/worktree-metadata.ts(1 hunks)apps/server/src/routes/worktree/index.ts(2 hunks)apps/server/src/routes/worktree/routes/create-pr.ts(9 hunks)apps/server/src/routes/worktree/routes/list.ts(4 hunks)apps/server/src/routes/worktree/routes/pr-info.ts(1 hunks)apps/ui/src/components/views/board-view.tsx(6 hunks)apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx(4 hunks)apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx(4 hunks)apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx(7 hunks)apps/ui/src/components/views/board-view/worktree-panel/types.ts(3 hunks)apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx(3 hunks)apps/ui/src/lib/electron.ts(1 hunks)apps/ui/src/lib/http-api-client.ts(1 hunks)apps/ui/src/types/electron.d.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
apps/server/src/routes/worktree/index.ts (1)
apps/server/src/routes/worktree/routes/pr-info.ts (1)
createPRInfoHandler(66-296)
apps/server/src/routes/worktree/routes/pr-info.ts (2)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (1)
PRInfo(37-60)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/routes/worktree/routes/list.ts (2)
apps/server/src/lib/worktree-metadata.ts (2)
WorktreePRInfo(9-15)readAllWorktreeMetadata(113-139)apps/ui/src/components/views/board-view/worktree-panel/types.ts (1)
WorktreePRInfo(1-7)
apps/server/src/routes/worktree/routes/create-pr.ts (1)
apps/server/src/lib/worktree-metadata.ts (1)
updateWorktreePRInfo(77-97)
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (2)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (2)
WorktreeInfo(9-18)PRInfo(37-60)apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(578-583)
apps/ui/src/components/views/board-view.tsx (4)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (2)
WorktreeInfo(9-18)PRInfo(37-60)apps/ui/src/lib/electron.ts (1)
WorktreeInfo(52-52)apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(578-583)apps/server/src/routes/worktree/routes/pr-info.ts (1)
PRInfo(55-64)
apps/server/src/lib/worktree-metadata.ts (1)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (1)
WorktreePRInfo(1-7)
🪛 Biome (2.1.2)
apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx
[error] 33-35: Expected a statement but instead found '=======
projectPath: string | null'.
Expected a statement here.
(parse)
[error] 36-38: Expected a statement but instead found '>>>>>>> Stashed changes
}'.
Expected a statement here.
(parse)
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx
[error] 12-13: Expected a statement but instead found '======='.
Expected a statement here.
(parse)
[error] 16-17: Expected a statement but instead found '>>>>>>> Stashed changes'.
Expected a statement here.
(parse)
[error] 14-14: Shouldn't redeclare 'RefreshCw'. Consider to delete it or rename it.
'RefreshCw' is defined here:
(lint/suspicious/noRedeclare)
[error] 14-14: Shouldn't redeclare 'Globe'. Consider to delete it or rename it.
'Globe' is defined here:
(lint/suspicious/noRedeclare)
[error] 14-14: Shouldn't redeclare 'Loader2'. Consider to delete it or rename it.
'Loader2' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'cn'. Consider to delete it or rename it.
'cn' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'WorktreeInfo'. Consider to delete it or rename it.
'WorktreeInfo' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'BranchInfo'. Consider to delete it or rename it.
'BranchInfo' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'DevServerInfo'. Consider to delete it or rename it.
'DevServerInfo' is defined here:
(lint/suspicious/noRedeclare)
[error] 97-98: Expected a statement but instead found '<<<<<<< Updated upstream'.
Expected a statement here.
(parse)
[error] 113-114: Expected a statement but instead found '======='.
Expected a statement here.
(parse)
[error] 199-200: Expected a statement but instead found '>>>>>>> Stashed changes'.
Expected a statement here.
(parse)
[error] 229-229: expected ... but instead found hasChanges
Remove hasChanges
(parse)
[error] 248-249: Expected a JSX attribute but instead found '======='.
Expected a JSX attribute here.
(parse)
[error] 250-250: expected ... but instead found prBadge
Remove prBadge
(parse)
[error] 228-251: Expected corresponding JSX closing tag for 'Updated'.
Opening tag
closing tag
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 296-296: expected ... but instead found hasChanges
Remove hasChanges
(parse)
[error] 315-316: Expected a JSX attribute but instead found '======='.
Expected a JSX attribute here.
(parse)
[error] 317-317: expected ... but instead found prBadge
Remove prBadge
(parse)
[error] 295-318: Expected corresponding JSX closing tag for 'Updated'.
Opening tag
closing tag
(parse)
[error] 318-318: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 318-318: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 318-318: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 318-318: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 318-318: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 318-318: Unexpected token. Did you mean {'>'} or >?
(parse)
🪛 GitHub Actions: PR Build Check
apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx
[error] 32-32: Build failed during Vite/esbuild transform. Unexpected '<<<<<< Updated upstream' merge conflict marker found in the source file.
⏰ 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
🔇 Additional comments (20)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (1)
69-69: LGTM!The
onAddressPRCommentscallback is well-typed with appropriate parameters for handling PR comment addressing workflow.apps/server/src/routes/worktree/routes/pr-info.ts (1)
66-104: LGTM!The handler structure is well-organized with proper input validation, gh CLI availability check, and graceful fallback when the CLI is unavailable.
apps/server/src/routes/worktree/index.ts (1)
15-15: LGTM!The new PR info route is properly imported and registered following the established patterns in this router.
Also applies to: 44-44
apps/ui/src/lib/http-api-client.ts (1)
675-676: LGTM!The
getPRInfomethod is correctly implemented following the established pattern for worktree API methods.apps/ui/src/lib/electron.ts (1)
1356-1366: LGTM!The mock implementation correctly simulates the unavailable gh CLI scenario and follows the established pattern for mock worktree API methods.
apps/server/src/routes/worktree/routes/list.ts (2)
13-13: LGTM!The import and type extension are well-aligned with the
WorktreePRInfointerface defined inworktree-metadata.ts. The optionalprfield correctly extends the existingWorktreeInfointerface.Also applies to: 25-25
111-141: LGTM!The implementation correctly:
- Reads all worktree metadata once after the worktree list is built
- Attaches PR info to each worktree by matching on branch name
- Handles the case where metadata or PR info doesn't exist gracefully via optional chaining
The metadata reading is appropriately placed after pruning removed worktrees, ensuring we only attach PR info to valid worktrees.
apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx (2)
118-136: Good enhancement for differentiating PR creation vs. discovery.The toast messaging logic correctly distinguishes between finding an existing PR (
prAlreadyExisted) and creating a new one, providing appropriate user feedback in each case.
218-227: LGTM!The
handleClosefunction correctly passes the PR URL toonCreatedwhen an operation completed, enabling the parent component to update feature records with the PR URL as described in the PR objectives.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
22-25: LGTM!The new imports, prop additions, and
hasPRhelper are well-structured. The prop type correctly uses thePRInfointerface from the types file.Also applies to: 44-44, 68-68, 74-76
apps/server/src/routes/worktree/routes/create-pr.ts (5)
9-9: LGTM!The import addition and request body extension are well-structured. The
effectiveProjectPathlogic correctly handles the case whereprojectPathmay not be provided, falling back toworktreePath.Also applies to: 52-54, 70-72
219-228: LGTM!The cross-platform
ghCLI availability check correctly useswhereon Windows andcommand -von Unix-like systems.
247-289: Good implementation for PR discovery before creation.The logic correctly:
- Constructs fork-aware head references for the
gh pr listcommand- Parses the JSON output to detect existing PRs
- Stores metadata for discovered PRs
- Gracefully handles failures without blocking PR creation
The logging statements will help with debugging fork-related issues.
274-282: LGTM! Consistent metadata storage across all PR paths.The PR metadata is correctly stored in all three scenarios:
- When an existing PR is discovered via
gh pr list- When a new PR is created successfully
- When PR creation fails with "already exists" error and we fetch via
gh pr viewThis ensures PR info is always persisted for UI consumption.
Also applies to: 324-330, 357-363
388-390: LGTM!The response payload correctly includes the new
prNumberandprAlreadyExistedfields, enabling the UI to differentiate between newly created and discovered PRs.apps/ui/src/components/views/board-view.tsx (2)
41-41: LGTM! Type imports properly support PR addressing workflow.The imported types enable the new PR addressing feature and are correctly sourced from the worktree-panel types module.
973-973: LGTM! PR addressing callback properly wired.The
onAddressPRCommentshandler is correctly passed to the WorktreePanel component.apps/ui/src/types/electron.d.ts (1)
670-676: LGTM! Type definitions properly extend the Electron API for PR support.The type additions are well-structured and consistent with existing API patterns:
- PR metadata in
listAllentries provides UI visibilitygetPRInfomethod signature aligns with the nested result pattern used throughout the API- Extended
createPRoptions and result support the full PR workflowAlso applies to: 747-747, 762-764, 908-944
apps/server/src/lib/worktree-metadata.ts (2)
9-21: LGTM! Well-structured type definitions for worktree metadata.The interfaces clearly separate PR-specific info from general worktree metadata, with appropriate optional fields.
42-154: LGTM! Robust error handling for metadata operations.The implementation correctly:
- Returns
nullfor missing or unreadable metadata (appropriate for optional data)- Ensures directories exist before writing
- Silently skips invalid entries when reading all metadata (good for robustness)
- Uses
force: truefor safe deletionThe silent error handling pattern is appropriate for metadata operations that should degrade gracefully.
| try { | ||
| const reviewsEndpoint = targetRepo | ||
| ? `repos/${targetRepo}/pulls/${prNumber}/comments` | ||
| : `repos/{owner}/{repo}/pulls/${prNumber}/comments`; | ||
| const reviewsCmd = `gh api ${reviewsEndpoint}`; | ||
| const { stdout: reviewsOutput } = await execAsync( | ||
| reviewsCmd, | ||
| { cwd: worktreePath, env: execEnv } | ||
| ); |
There was a problem hiding this comment.
Placeholder {owner}/{repo} will not be interpolated by gh api.
When targetRepo is null, the endpoint becomes repos/{owner}/{repo}/pulls/${prNumber}/comments with literal {owner} and {repo} strings. The gh api command doesn't automatically replace these placeholders in this context - it only does so for certain documented patterns.
🔎 Proposed fix: Handle the case when targetRepo is unavailable
// Get review comments (inline code comments)
let reviewComments: PRComment[] = [];
try {
- const reviewsEndpoint = targetRepo
- ? `repos/${targetRepo}/pulls/${prNumber}/comments`
- : `repos/{owner}/{repo}/pulls/${prNumber}/comments`;
+ if (!targetRepo) {
+ console.warn("[PRInfo] Cannot fetch review comments: repository info not available");
+ } else {
+ const reviewsEndpoint = `repos/${targetRepo}/pulls/${prNumber}/comments`;
- const reviewsCmd = `gh api ${reviewsEndpoint}`;
- const { stdout: reviewsOutput } = await execAsync(
- reviewsCmd,
- { cwd: worktreePath, env: execEnv }
- );
+ const reviewsCmd = `gh api ${reviewsEndpoint}`;
+ const { stdout: reviewsOutput } = await execAsync(
+ reviewsCmd,
+ { cwd: worktreePath, env: execEnv }
+ );
- const reviewsData = JSON.parse(reviewsOutput);
+ const reviewsData = JSON.parse(reviewsOutput);
- reviewComments = reviewsData.map((c: {
+ reviewComments = reviewsData.map((c: {
// ... rest of mapping
- }));
+ }));
+ }
} catch (error) {
console.warn("[PRInfo] Failed to fetch review comments:", error);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/routes/pr-info.ts around lines 229-237, the
code currently builds reviewsEndpoint using a literal placeholder when
targetRepo is null which results in `repos/{owner}/{repo}/...` being passed to
`gh api` unchanged; instead detect the missing targetRepo and resolve the actual
owner/repo before building the endpoint (for example, read the repo from the
worktree git remote or call `gh repo view` to get owner and name), then
construct `repos/${owner}/${repo}/pulls/${prNumber}/comments` with those values;
ensure you handle errors when resolving the repo and only call `gh api` with a
fully-qualified endpoint.
| // Handler for addressing PR comments - creates a feature and starts it automatically | ||
| const handleAddressPRComments = useCallback( | ||
| async (worktree: WorktreeInfo, prInfo: PRInfo) => { | ||
| // If comments are empty, fetch them from GitHub | ||
| let fullPRInfo = prInfo; | ||
| if (prInfo.comments.length === 0 && prInfo.reviewComments.length === 0) { | ||
| try { | ||
| const api = getElectronAPI(); | ||
| if (api?.worktree?.getPRInfo) { | ||
| const result = await api.worktree.getPRInfo( | ||
| worktree.path, | ||
| worktree.branch | ||
| ); | ||
| if ( | ||
| result.success && | ||
| result.result?.hasPR && | ||
| result.result.prInfo | ||
| ) { | ||
| fullPRInfo = result.result.prInfo; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error("[Board] Failed to fetch PR comments:", error); | ||
| } | ||
| } | ||
|
|
||
| // Format PR comments into a feature description | ||
| const allComments = [ | ||
| ...fullPRInfo.comments.map((c) => ({ | ||
| ...c, | ||
| type: "comment" as const, | ||
| })), | ||
| ...fullPRInfo.reviewComments.map((c) => ({ | ||
| ...c, | ||
| type: "review" as const, | ||
| })), | ||
| ].sort( | ||
| (a, b) => | ||
| new Date(a.createdAt).getTime() - new Date(b.createdAt).getTime() | ||
| ); | ||
|
|
||
| let description = `Address PR #${fullPRInfo.number} feedback: "${fullPRInfo.title}"\n\n`; | ||
| description += `PR URL: ${fullPRInfo.url}\n\n`; | ||
|
|
||
| if (allComments.length === 0) { | ||
| description += `No comments found on this PR yet. Check the PR for any new feedback.\n`; | ||
| } else { | ||
| description += `## Feedback to address:\n\n`; | ||
| for (const comment of allComments) { | ||
| if (comment.type === "review" && comment.path) { | ||
| description += `### ${comment.path}${comment.line ? `:${comment.line}` : ""}\n`; | ||
| } | ||
| description += `**@${comment.author}:**\n${comment.body}\n\n`; | ||
| } | ||
| } | ||
|
|
||
| // Create the feature | ||
| const featureData = { | ||
| category: "PR Review", | ||
| description: description.trim(), | ||
| steps: [], | ||
| images: [], | ||
| imagePaths: [], | ||
| skipTests: defaultSkipTests, | ||
| model: "sonnet" as const, | ||
| thinkingLevel: "medium" as const, | ||
| branchName: worktree.branch, | ||
| priority: 1, // High priority for PR feedback | ||
| planningMode: "skip" as const, | ||
| requirePlanApproval: false, | ||
| }; | ||
|
|
||
| await handleAddFeature(featureData); | ||
|
|
||
| // Find the newly created feature and start it | ||
| // We need to wait a moment for the feature to be created | ||
| setTimeout(async () => { | ||
| const latestFeatures = useAppStore.getState().features; | ||
| const newFeature = latestFeatures.find( | ||
| (f) => | ||
| f.branchName === worktree.branch && | ||
| f.status === "backlog" && | ||
| f.description.includes(`PR #${fullPRInfo.number}`) | ||
| ); | ||
|
|
||
| if (newFeature) { | ||
| await handleStartImplementation(newFeature); | ||
| } | ||
| }, 500); | ||
| }, | ||
| [handleAddFeature, handleStartImplementation, defaultSkipTests] | ||
| ); |
There was a problem hiding this comment.
Race condition and error handling issues in PR comment addressing flow.
The implementation has several reliability concerns:
-
Race condition (Lines 497-509): The
setTimeoutwith a fixed 500ms delay assumes the feature will be created within that timeframe. IfhandleAddFeatureor state updates take longer, the newly created feature won't be found and won't auto-start. -
Direct store access (Line 498): Using
useAppStore.getState().featuresbypasses the React hook and could reference stale data instead of usinghookFeatures. -
Fragile feature matching (Lines 499-504): The search criteria (branch name + backlog status + PR number in description) could match the wrong feature if multiple similar features exist.
-
No error handling: The entire setTimeout block lacks try-catch, and the async
handleStartImplementationcall is fire-and-forget.
🔎 Suggested refactor using a more reliable approach
Consider refactoring to use a callback or Promise-based approach instead of setTimeout:
- await handleAddFeature(featureData);
-
- // Find the newly created feature and start it
- // We need to wait a moment for the feature to be created
- setTimeout(async () => {
- const latestFeatures = useAppStore.getState().features;
- const newFeature = latestFeatures.find(
- (f) =>
- f.branchName === worktree.branch &&
- f.status === "backlog" &&
- f.description.includes(`PR #${fullPRInfo.number}`)
- );
-
- if (newFeature) {
- await handleStartImplementation(newFeature);
- }
- }, 500);
+ // Add feature and get the result
+ const newFeature = await handleAddFeature(featureData);
+
+ // Start implementation immediately with the newly created feature
+ if (newFeature) {
+ try {
+ await handleStartImplementation(newFeature);
+ } catch (error) {
+ console.error("[Board] Failed to start PR addressing feature:", error);
+ }
+ }This requires updating handleAddFeature to return the created feature, which would eliminate the race condition entirely.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/ui/src/components/views/board-view.tsx around lines 421-512, the
post-create logic uses a fragile setTimeout + direct store access and weak
matching to find the new feature, leading to race conditions and no error
handling. Modify handleAddFeature to return the created feature (or its id),
then await that Promise here instead of using setTimeout; use the returned
feature object (or id) to uniquely identify the created feature (avoid searching
by description/branch); call handleStartImplementation(awaitedFeature) and wrap
the start flow in try/catch to handle errors; remove direct use of
useAppStore.getState().features in this handler and update any callers of
handleAddFeature to handle the new return value.
apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx
Outdated
Show resolved
Hide resolved
| <<<<<<< Updated upstream | ||
| import { RefreshCw, Globe, Loader2, CircleDot } from "lucide-react"; | ||
| import { cn } from "@/lib/utils"; | ||
| import { | ||
| Tooltip, | ||
| TooltipContent, | ||
| TooltipProvider, | ||
| TooltipTrigger, | ||
| } from "@/components/ui/tooltip"; | ||
| import type { WorktreeInfo, BranchInfo, DevServerInfo } from "../types"; | ||
| ======= | ||
| import { RefreshCw, Globe, Loader2, GitPullRequest } from "lucide-react"; | ||
| import { cn } from "@/lib/utils"; | ||
| import type { WorktreeInfo, BranchInfo, DevServerInfo, PRInfo } from "../types"; | ||
| >>>>>>> Stashed changes |
There was a problem hiding this comment.
Unresolved merge conflict in imports section.
This file contains multiple unresolved git merge conflict markers that prevent compilation. The conflicts span imports, component logic, and JSX rendering.
🔎 Proposed fix for imports section
-<<<<<<< Updated upstream
-import { RefreshCw, Globe, Loader2, CircleDot } from "lucide-react";
-import { cn } from "@/lib/utils";
-import {
- Tooltip,
- TooltipContent,
- TooltipProvider,
- TooltipTrigger,
-} from "@/components/ui/tooltip";
-import type { WorktreeInfo, BranchInfo, DevServerInfo } from "../types";
-=======
-import { RefreshCw, Globe, Loader2, GitPullRequest } from "lucide-react";
-import { cn } from "@/lib/utils";
-import type { WorktreeInfo, BranchInfo, DevServerInfo, PRInfo } from "../types";
->>>>>>> Stashed changes
+import { RefreshCw, Globe, Loader2, CircleDot, GitPullRequest } from "lucide-react";
+import { cn } from "@/lib/utils";
+import {
+ Tooltip,
+ TooltipContent,
+ TooltipProvider,
+ TooltipTrigger,
+} from "@/components/ui/tooltip";
+import type { WorktreeInfo, BranchInfo, DevServerInfo, PRInfo } from "../types";📝 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.
| <<<<<<< Updated upstream | |
| import { RefreshCw, Globe, Loader2, CircleDot } from "lucide-react"; | |
| import { cn } from "@/lib/utils"; | |
| import { | |
| Tooltip, | |
| TooltipContent, | |
| TooltipProvider, | |
| TooltipTrigger, | |
| } from "@/components/ui/tooltip"; | |
| import type { WorktreeInfo, BranchInfo, DevServerInfo } from "../types"; | |
| ======= | |
| import { RefreshCw, Globe, Loader2, GitPullRequest } from "lucide-react"; | |
| import { cn } from "@/lib/utils"; | |
| import type { WorktreeInfo, BranchInfo, DevServerInfo, PRInfo } from "../types"; | |
| >>>>>>> Stashed changes | |
| import { RefreshCw, Globe, Loader2, CircleDot, GitPullRequest } from "lucide-react"; | |
| import { cn } from "@/lib/utils"; | |
| import { | |
| Tooltip, | |
| TooltipContent, | |
| TooltipProvider, | |
| TooltipTrigger, | |
| } from "@/components/ui/tooltip"; | |
| import type { WorktreeInfo, BranchInfo, DevServerInfo, PRInfo } from "../types"; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 12-13: Expected a statement but instead found '======='.
Expected a statement here.
(parse)
[error] 16-17: Expected a statement but instead found '>>>>>>> Stashed changes'.
Expected a statement here.
(parse)
[error] 14-14: Shouldn't redeclare 'RefreshCw'. Consider to delete it or rename it.
'RefreshCw' is defined here:
(lint/suspicious/noRedeclare)
[error] 14-14: Shouldn't redeclare 'Globe'. Consider to delete it or rename it.
'Globe' is defined here:
(lint/suspicious/noRedeclare)
[error] 14-14: Shouldn't redeclare 'Loader2'. Consider to delete it or rename it.
'Loader2' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'cn'. Consider to delete it or rename it.
'cn' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'WorktreeInfo'. Consider to delete it or rename it.
'WorktreeInfo' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'BranchInfo'. Consider to delete it or rename it.
'BranchInfo' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'DevServerInfo'. Consider to delete it or rename it.
'DevServerInfo' is defined here:
(lint/suspicious/noRedeclare)
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx
around lines 3 to 17, there are unresolved git conflict markers (<<<<<<<,
=======, >>>>>>>) in the imports; remove those markers and reconcile the two
sides by merging needed imports into a single import block: include RefreshCw,
Globe, Loader2 and any icon actually used (CircleDot and/or GitPullRequest) from
"lucide-react", preserve cn from "@/lib/utils", restore
Tooltip/TooltipContent/TooltipProvider/TooltipTrigger imports if the component
uses them, and merge types so WorktreeInfo, BranchInfo, DevServerInfo and PRInfo
(if referenced) are imported from "../types"; ensure no duplicate or unused
imports remain and run TypeScript to verify compilation.
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx
Outdated
Show resolved
Hide resolved
| <<<<<<< Updated upstream | ||
| {hasChanges && ( | ||
| <TooltipProvider> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <span className={cn( | ||
| "inline-flex items-center justify-center h-4 min-w-[1rem] px-1 text-[10px] font-medium rounded border", | ||
| isSelected | ||
| ? "bg-amber-500 text-amber-950 border-amber-400" | ||
| : "bg-amber-500/20 text-amber-600 dark:text-amber-400 border-amber-500/30" | ||
| )}> | ||
| <CircleDot className="w-2.5 h-2.5 mr-0.5" /> | ||
| {changedFilesCount ?? "!"} | ||
| </span> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| <p>{changedFilesCount ?? "Some"} uncommitted file{changedFilesCount !== 1 ? "s" : ""}</p> | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| </TooltipProvider> | ||
| )} | ||
| ======= | ||
| {prBadge} | ||
| >>>>>>> Stashed changes |
There was a problem hiding this comment.
Unresolved merge conflicts in JSX rendering blocks.
The JSX sections have conflicts between rendering the hasChanges badge with tooltips vs. rendering the prBadge. The resolution should include both badges, as they serve different purposes (uncommitted changes indicator vs. PR status).
Also applies to: 295-318
🧰 Tools
🪛 Biome (2.1.2)
[error] 229-229: expected ... but instead found hasChanges
Remove hasChanges
(parse)
[error] 248-249: Expected a JSX attribute but instead found '======='.
Expected a JSX attribute here.
(parse)
[error] 250-250: expected ... but instead found prBadge
Remove prBadge
(parse)
[error] 228-251: Expected corresponding JSX closing tag for 'Updated'.
Opening tag
closing tag
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 251-251: Unexpected token. Did you mean {'>'} or >?
(parse)
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx
around lines 228-251 (and similarly 295-318), there is an unresolved merge
conflict that currently chooses either the uncommitted-changes badge block
(hasChanges Tooltip) or the prBadge; both must be rendered. Remove the conflict
markers and combine the UI so both elements are shown (e.g., render the
hasChanges Tooltip block and also render prBadge adjacent to it), preserving
existing props/conditional rendering (only show hasChanges when true and prBadge
when present) and keeping styles/TooltipProvider/TooltipContent intact.
Security fixes:
- Enhanced branch name sanitization for cross-platform filesystem safety
(handles Windows-invalid chars, reserved names, path length limits)
- Added branch name validation in pr-info.ts to prevent command injection
- Sanitized prUrl in kanban-card to only allow http/https URLs
Code quality improvements:
- Fixed placeholder issue where {owner}/{repo} was passed literally to gh api
- Replaced async forEach with Promise.all for proper async handling
- Display PR number extracted from URL in kanban cards
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Updated WorktreeActionsDropdown to use DropdownMenuItem for better interaction with PR links. - Enhanced WorktreeTab to include hover and active states for buttons, and improved accessibility with updated titles and aria-labels. - Ensured PR URLs are safely opened only if they exist, enhancing user experience and preventing errors.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/ui/src/components/views/board-view.tsx (1)
421-464: Race condition persists in PR comment addressing flow.This issue was flagged in a previous review and remains unaddressed. The
setTimeoutwith a fixed 500ms delay is fragile:
- Race condition: If
handleAddFeaturetakes longer than 500ms, the feature won't be found.- Direct store access:
useAppStore.getState().featuresbypasses React's state management.- Fragile matching: Searching by
branchName + status + description.includes()could match the wrong feature.Consider refactoring
handleAddFeatureto return the created feature, eliminating the need for the timeout entirely.apps/server/src/routes/worktree/routes/create-pr.ts (1)
11-20:shellEscapefunction defined but not used.The
shellEscapefunction is defined but never used in this file. The PR title, body, and commit message are still using simple.replace(/"/g, '\\"')escaping at lines 124 and 333, which doesn't protect against command substitution via$()or backticks.This is the same command injection vulnerability flagged in a previous review. The
shellEscapefunction should be applied to user-provided inputs:🔎 Proposed fix
- await execAsync(`git commit -m "${message.replace(/"/g, '\\"')}"`, { + await execAsync(`git commit -m ${shellEscape(message)}`, { cwd: worktreePath, env: execEnv, });And for PR creation:
- prCmd += ` --title "${title.replace(/"/g, '\\"')}" --body "${body.replace(/"/g, '\\"')}" ${draftFlag}`; + prCmd += ` --title ${shellEscape(title)} --body ${shellEscape(body)} ${draftFlag}`;
🧹 Nitpick comments (9)
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
200-214: Consider documenting the partial PRInfo contract.The
PRInfoobject is constructed with placeholder values (author: "",body: "", empty arrays). While this works becausehandleAddressPRCommentsonly usesprInfo.numberto build the feature description, this implicit contract could be fragile if the handler's implementation changes.A brief inline comment explaining that only
numberis used would improve maintainability.🔎 Suggested documentation
onClick={() => { - // Convert stored PR info to the full PRInfo format for the handler - // The handler will fetch full comments from GitHub + // Only prInfo.number is used by the handler to reference the PR. + // Other fields are placeholders - the agent fetches full PR details directly. const prInfo: PRInfo = { number: worktree.pr!.number,apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx (1)
224-244: Consider extracting duplicate change indicator badge.The uncommitted changes badge (Tooltip with CircleDot icon) is duplicated between main and non-main tab branches. Extracting this into a small helper component or variable would reduce duplication.
🔎 Example extraction
// Before the return statement, create a reusable badge: const changesBadge = hasChanges && ( <TooltipProvider> <Tooltip> <TooltipTrigger asChild> <span className={cn( "inline-flex items-center justify-center h-4 min-w-[1rem] px-1 text-[10px] font-medium rounded border", isSelected ? "bg-amber-500 text-amber-950 border-amber-400" : "bg-amber-500/20 text-amber-600 dark:text-amber-400 border-amber-500/30" )}> <CircleDot className="w-2.5 h-2.5 mr-0.5" /> {changedFilesCount ?? "!"} </span> </TooltipTrigger> <TooltipContent> <p>{changedFilesCount ?? "Some"} uncommitted file{changedFilesCount !== 1 ? "s" : ""}</p> </TooltipContent> </Tooltip> </TooltipProvider> ); // Then use {changesBadge} in both branchesAlso applies to: 288-308
apps/server/src/routes/worktree/routes/pr-info.ts (3)
78-98: Missing validation forworktreePathparameter.While
branchNameis validated against command injection,worktreePathis passed directly toexecAsyncas thecwdoption. Althoughcwditself doesn't execute shell commands, a malicious path could still be used for path traversal attacks or to access unintended directories.Consider adding path validation to ensure
worktreePathis within expected bounds:🔎 Suggested validation
if (!worktreePath || !branchName) { res.status(400).json({ success: false, error: "worktreePath and branchName required", }); return; } +// Basic path validation - ensure it's an absolute path and doesn't contain suspicious patterns +const normalizedPath = path.resolve(worktreePath); +if (normalizedPath !== worktreePath || worktreePath.includes('..')) { + res.status(400).json({ + success: false, + error: "Invalid worktreePath", + }); + return; +} // Validate branch name to prevent command injection
12-43: Duplicate code: PATH extension logic.This PATH extension logic (lines 12-43) is duplicated across both
pr-info.tsandcreate-pr.ts. Consider extracting this into a shared utility incommon.tsto improve maintainability.
54-73: Share type definitions between server and UI to prevent drift.The
PRCommentandPRInfointerfaces are duplicated across the codebase. The server exports them fromapps/server/src/routes/worktree/routes/pr-info.ts, while the UI redefines them inline inapps/ui/src/components/views/board-view/worktree-panel/types.tswithout importing from the server. Both currently have identical structures, but this duplication increases the risk of unintended divergence if one side changes without updating the other.Consider extracting these shared types to a dedicated package or ensuring a single source of truth to maintain consistency across the full stack.
apps/server/src/routes/worktree/routes/create-pr.ts (1)
140-150: Branch name used in shell command without escaping.Although
branchNameis validated withisValidBranchName, the validation allows/characters which could have unintended behavior in certain contexts. The validated pattern[a-zA-Z0-9._\-/]is safe for git branch names, but consider usingshellEscapefor consistency.apps/server/src/lib/worktree-metadata.ts (3)
31-36: Minor: Sanitization order could leave edge cases.The current order removes trailing dots (line 34) before collapsing dashes (line 35). If a branch like
foo..--is processed:
- After line 32:
foo.---- After line 33:
foo.---- After line 34:
foo.---(trailing dots only, not mid-string)- After line 35:
foo.-- After line 36:
foo.This could still leave a trailing dot. Consider reordering or adding a final trim:
🔎 Suggested improvement
let safeBranch = branch .replace(/[/\\:*?"<>|]/g, "-") // Replace invalid chars with dash .replace(/\s+/g, "_") // Replace spaces with underscores - .replace(/\.+$/g, "") // Remove trailing dots (Windows issue) .replace(/-+/g, "-") // Collapse multiple dashes - .replace(/^-|-$/g, ""); // Remove leading/trailing dashes + .replace(/^-|-$/g, "") // Remove leading/trailing dashes + .replace(/\.+$/g, ""); // Remove trailing dots (Windows issue) - do last
68-80: Silent error swallowing may hide issues.The
readWorktreeMetadatafunction catches all errors and returnsnull. While this is appropriate for "file not found" scenarios, it also silently hides JSON parse errors from corrupted files. Consider distinguishing between these cases for better debugging:🔎 Suggested improvement
export async function readWorktreeMetadata( projectPath: string, branch: string ): Promise<WorktreeMetadata | null> { try { const metadataPath = getWorktreeMetadataPath(projectPath, branch); const content = await fs.readFile(metadataPath, "utf-8"); return JSON.parse(content) as WorktreeMetadata; - } catch (error) { - // File doesn't exist or can't be read + } catch (error: unknown) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + console.warn(`[WorktreeMetadata] Failed to read metadata for ${branch}:`, error); + } return null; } }
145-159: Sequential file reads could be parallelized.The loop reads metadata files sequentially. For projects with many worktrees, parallel reads would be more efficient:
🔎 Suggested improvement
- for (const dir of dirs) { - if (dir.isDirectory()) { - const metadataPath = path.join(worktreesDir, dir.name, "worktree.json"); - try { - const content = await fs.readFile(metadataPath, "utf-8"); - const metadata = JSON.parse(content) as WorktreeMetadata; - result.set(metadata.branch, metadata); - } catch { - // Skip if file doesn't exist or can't be read - } - } - } + const readPromises = dirs + .filter(dir => dir.isDirectory()) + .map(async (dir) => { + const metadataPath = path.join(worktreesDir, dir.name, "worktree.json"); + try { + const content = await fs.readFile(metadataPath, "utf-8"); + return JSON.parse(content) as WorktreeMetadata; + } catch { + return null; + } + }); + + const metadataResults = await Promise.all(readPromises); + for (const metadata of metadataResults) { + if (metadata) { + result.set(metadata.branch, metadata); + } + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/server/src/lib/worktree-metadata.ts(1 hunks)apps/server/src/routes/worktree/routes/create-pr.ts(9 hunks)apps/server/src/routes/worktree/routes/pr-info.ts(1 hunks)apps/ui/src/components/views/board-view.tsx(6 hunks)apps/ui/src/components/views/board-view/components/kanban-card.tsx(4 hunks)apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx(4 hunks)apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx(4 hunks)apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/components/views/board-view/components/kanban-card.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
apps/server/src/routes/worktree/routes/pr-info.ts (3)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (1)
PRInfo(37-60)init.mjs (3)
req(169-171)lines(79-79)match(82-82)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/routes/worktree/routes/create-pr.ts (1)
apps/server/src/lib/worktree-metadata.ts (1)
updateWorktreePRInfo(103-123)
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx (3)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (2)
WorktreeInfo(9-18)PRInfo(37-60)apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(578-583)apps/server/src/routes/worktree/routes/pr-info.ts (1)
PRInfo(64-73)
apps/ui/src/components/views/board-view.tsx (3)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (2)
WorktreeInfo(9-18)PRInfo(37-60)apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(578-583)apps/server/src/routes/worktree/routes/pr-info.ts (1)
PRInfo(64-73)
apps/server/src/lib/worktree-metadata.ts (1)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (1)
WorktreePRInfo(1-7)
⏰ 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
🔇 Additional comments (26)
apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx (3)
32-33: LGTM! Merge conflict resolved and props properly wired.The
projectPathprop and updatedonCreatedsignature are correctly added and destructured. The PR URL propagation flow is well-implemented.Also applies to: 40-41
114-132: Good differentiation between existing and newly created PRs.The toast messaging correctly distinguishes between finding an existing PR versus creating a new one, providing appropriate feedback to users.
214-223: Clean close handler with proper PR URL propagation.The
handleClosefunction correctly passes the PR URL when an operation completed, and the state reset is properly handled by theuseEffectwhenopenbecomes false.apps/ui/src/components/views/board-view.tsx (4)
41-41: LGTM! Type imports properly added.The
PRInfoandWorktreeInfotypes are correctly imported from the centralized types module.
273-284: Clean formatting improvement for branchCardCounts.The reduce logic is properly formatted and the type annotation is correct.
1205-1223: Good fix: Promise.all for parallel persistence.The previous review's suggestion to separate synchronous state updates from asynchronous persistence has been properly implemented. The
Promise.allpattern ensures all persistence operations are tracked together.
925-925: LGTM! Handler properly wired to WorktreePanel.The
onAddressPRCommentscallback is correctly passed down to enable the PR addressing workflow.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (3)
22-25: LGTM! New imports for PR-related functionality.The
MessageSquareicon andPRInfotype are correctly imported to support the new PR addressing feature.
74-76: Clean derivation of PR presence.The
hasPRvariable provides a clear boolean check for conditional rendering.
179-184: LGTM! PR-related dropdown items well structured.The conditional rendering correctly shows "Create Pull Request" when no PR exists, and shows PR info with "Address PR Comments" when a PR is present. The click handlers properly prevent event bubbling and open links in new tabs.
Also applies to: 185-222
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx (4)
3-11: LGTM! Merge conflicts resolved.All imports are properly consolidated with
CircleDot,GitPullRequest, Tooltip components, and type imports correctly combined.
92-106: Good visual state indication logic.The border classes correctly prioritize running state (cyan) over uncommitted changes (amber), providing clear visual hierarchy.
108-196: Well-implemented PR badge with accessibility.The PR badge includes:
- State-based color coding for different PR states
- Proper accessibility attributes (
aria-label,aria-hidden)- Keyboard navigation support (
onKeyDown)- Event propagation prevention to avoid triggering parent handlers
The styling logic correctly handles both selected and unselected tab states.
346-346: LGTM! Handler properly forwarded.The
onAddressPRCommentscallback is correctly passed to theWorktreeActionsDropdowncomponent.apps/server/src/routes/worktree/routes/pr-info.ts (3)
45-52: Good addition of branch name validation.The
isValidBranchNamefunction properly addresses the command injection concern from previous reviews by restricting branch names to safe characters and limiting length.
245-279: Good fix for the{owner}/{repo}placeholder issue.The conditional check at line 248 (
if (targetRepo)) now properly skips fetching review comments when repository info is unavailable, with an appropriate warning logged at line 278. This addresses the previous review concern about literal placeholders being passed togh api.
196-215: Proper use ofgh pr listwith JSON output.Good approach using
gh pr listwith--jsonflag for structured output. The--limit 1is appropriate since you only need the first matching PR.apps/server/src/routes/worktree/routes/create-pr.ts (5)
22-27: Good: Branch name validation function added.The
isValidBranchNamefunction provides proper input validation matching the implementation inpr-info.ts. This addresses the branch name injection concern.
99-106: Good: Branch validation applied before use.The branch name is now validated before being used in shell commands, preventing command injection through malicious branch names.
301-309: Good: PR metadata persistence for existing PRs.The code properly stores metadata when discovering an existing PR, ensuring the metadata is synchronized regardless of whether the PR was just created or already existed.
349-362: Good: PR metadata storage with error handling.Appropriate error handling around metadata storage that doesn't fail the entire PR creation if metadata persistence fails.
370-396: Good: Graceful recovery when PR already exists.The fallback logic to fetch existing PR details when creation fails with "already exists" provides good resilience against race conditions and edge cases.
apps/server/src/lib/worktree-metadata.ts (4)
26-48: Good: Comprehensive branch name sanitization.The sanitization function now properly addresses cross-platform filesystem safety:
- Replaces invalid characters (including Windows-specific ones)
- Handles trailing dots and spaces
- Truncates to safe length
- Handles Windows reserved names
This addresses the previous review concern about incomplete sanitization.
9-21: Type definitions align with UI types.The
WorktreePRInfointerface matches the corresponding type inapps/ui/src/components/views/board-view/worktree-panel/types.ts. Good consistency between server and client types.
103-123: Good: Read-modify-write pattern for PR info updates.The
updateWorktreePRInfofunction properly reads existing metadata before updating, preserving other fields likecreatedAt. This is the correct pattern for partial updates.
170-180: Good: Safe deletion withforce: true.Using
fs.rmwith{ recursive: true, force: true }is appropriate for cleanup operations where the directory may or may not exist.
| // Use projectPath if provided, otherwise derive from worktreePath | ||
| // For worktrees, projectPath is needed to store metadata in the main project's .automaker folder | ||
| const effectiveProjectPath = projectPath || worktreePath; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the create-pr.ts file and examine its context
find . -name "create-pr.ts" -type f 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Search for create-pr related code and projectPath/worktreePath usage
rg -l "create-pr|projectPath|worktreePath" --type ts --type js 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 995
🏁 Script executed:
#!/bin/bash
# Search for .automaker usage in the codebase
rg "\.automaker|automaker" --type ts --type js 2>/dev/null | head -30Repository: AutoMaker-Org/automaker
Length of output: 3481
🏁 Script executed:
#!/bin/bash
# Look for the function that contains lines 88-90 of create-pr.ts
fd "create-pr.ts" -type f --exec cat {} \;Repository: AutoMaker-Org/automaker
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Read the create-pr.ts file to see the full context around lines 88-90
cat -n apps/server/src/routes/worktree/routes/create-pr.tsRepository: AutoMaker-Org/automaker
Length of output: 19264
🏁 Script executed:
#!/bin/bash
# Search for callers of create-pr endpoint
rg "create-pr|createPr" --type ts --type js -B 3 -A 3 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 7841
🏁 Script executed:
#!/bin/bash
# Search for the route definition to understand API contract
rg -A 10 "effectiveProjectPath" apps/server/src/routes/worktree/routes/create-pr.ts 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 2162
🏁 Script executed:
#!/bin/bash
# Check how metadata is stored and where .automaker is used
rg "\.automaker|effectiveProjectPath" apps/server/src -type ts -B 2 -A 2 2>/dev/null | head -150Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check the CreatePRDialog to see if it passes projectPath
rg -A 30 "CreatePRDialog" apps/ui/src/components/views/board-view/dialogs/ -type ts 2>/dev/null | grep -A 30 "createPR"Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check the worktree-metadata.js to see how updateWorktreePRInfo uses effectiveProjectPath
find . -name "worktree-metadata.js" -o -name "worktree-metadata.ts" | xargs cat 2>/dev/null | head -150Repository: AutoMaker-Org/automaker
Length of output: 4334
🏁 Script executed:
#!/bin/bash
# Search for where the createPR API is called in the UI
rg "api\.createPR\|createPR\(" apps/ui/src/components -A 5 -B 5 --type ts 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check how createPR is called in the board-view - specifically the commit/PR dialog
find apps/ui/src/components/views/board-view/dialogs -name "*pr*" -o -name "*commit*" | xargs cat 2>/dev/null | grep -A 20 "createPR\|api\." | head -150Repository: AutoMaker-Org/automaker
Length of output: 1791
🏁 Script executed:
#!/bin/bash
# Check if there's any logic to derive or resolve projectPath from worktreePath
rg "getProjectPath|resolveProjectPath|projectPath" apps/server/src/lib -A 5 -B 2 --type ts 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 6654
🏁 Script executed:
#!/bin/bash
# Check how the createPR endpoint is registered and what the expected contract is
rg -B 10 "createCreatePRHandler" apps/server/src/routes/worktree/index.ts 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 1091
Require projectPath parameter or implement explicit derivation logic for metadata storage.
The fallback const effectiveProjectPath = projectPath || worktreePath breaks the intended architecture. Metadata must be stored in the main project's .automaker/worktrees/ directory, not in the linked worktree's directory. Since projectPath is optional in the API contract, callers can omit it, causing PR metadata to be stored in the wrong location and become isolated to individual worktrees. Either make projectPath a required parameter or implement explicit logic to derive the main project path from the worktree, but do not silently fall back to worktreePath.
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/routes/create-pr.ts around lines 88 to 90,
the current fallback "const effectiveProjectPath = projectPath || worktreePath"
allows metadata to be written into the worktree instead of the main project's
.automaker directory; either require projectPath or derive the main project path
explicitly. Fix by validating and returning a 4xx error when projectPath is
missing (making it effectively required for this endpoint) OR implement
deterministic derivation: resolve the worktree's link/parent to locate the main
project root (e.g., follow symlink or search upward for the main project's
marker like .git or .automaker) and set effectiveProjectPath to that root; then
always write metadata to path.join(effectiveProjectPath, '.automaker',
'worktrees', <id>). Ensure tests and API docs are updated and add a clear error
message when derivation fails.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
apps/ui/src/components/views/board-view.tsx (1)
424-467: Race condition and fragile feature matching (duplicate concern).This implementation has the same issues previously flagged in the review of lines 424-467:
- Race condition: The
setTimeoutwithFEATURE_CREATION_SETTLE_DELAY_MS(500ms) assumes the feature will be created within that timeframe.- Direct store access: Line 453 uses
useAppStore.getState().featuresinstead ofhookFeatures, which could reference stale data.- Fragile matching (Lines 454-459): Searching by
branchName,status === "backlog", anddescription.includes(\PR #${prNumber}`)` could match the wrong feature.- No error handling: The async
handleStartImplementationcall lacks try-catch.The previously suggested fix remains valid: modify
handleAddFeatureto return the created feature (or its ID), then use that directly instead of searching.apps/server/src/routes/worktree/routes/create-pr.ts (3)
73-76: Fix command injection vulnerability in commit message.The code uses
execAsync(which spawns a shell viachild_process.exec()) with user-providedcommitMessageinterpolated into a double-quoted shell string. The.replace(/"/g, '\\"')escaping is insufficient because dollar signs ($), backticks (`), and backslashes (\) retain special meaning within double quotes in shell contexts.Attack example:
commitMessage = 'Fix $(rm -rf /tmp/test)' // Executes rm command commitMessage = 'Update `cat /etc/passwd`' // Executes cat commandUse
execFileor pass the message via stdin to avoid shell interpolation, or properly escape all shell metacharacters (which is error-prone).🔎 Recommended fix using execFile
+import { execFile } from "child_process"; +import { promisify } from "util"; +const execFileAsync = promisify(execFile); // Create commit -await execAsync(`git commit -m "${message.replace(/"/g, '\\"')}"`, { - cwd: worktreePath, - env: execEnv, -}); +await execFileAsync("git", ["commit", "-m", message], { + cwd: worktreePath, + env: execEnv, +});
37-39: RequireprojectPathparameter or implement explicit derivation logic for metadata storage.The fallback
const effectiveProjectPath = projectPath || worktreePathbreaks the intended architecture. Metadata must be stored in the main project's.automaker/worktrees/directory, not in the linked worktree's directory. SinceprojectPathis optional in the API contract, callers can omit it, causing PR metadata to be stored in the wrong location and become isolated to individual worktrees. Either makeprojectPatha required parameter or implement explicit logic to derive the main project path from the worktree, but do not silently fall back toworktreePath.
260-304: Fix command injection vulnerability in PR title and body.The code constructs a shell command with user-provided
titleandbodyparameters using only.replace(/"/g, '\\"')for escaping. This is insufficient when usingexecAsync(which spawns a shell), as dollar signs ($), backticks (`), and backslashes (\) remain interpreted within double-quoted strings.Attack examples:
prTitle = 'Feature $(whoami)' // Executes whoami prBody = 'Description `ls -la`' // Executes lsReplace
execAsyncwithexecFileto pass arguments directly without shell interpolation, or use stdin for multi-line content.🔎 Recommended fix using execFile
+import { execFile } from "child_process"; +import { promisify } from "util"; +const execFileAsync = promisify(execFile); - prCmd += ` --title "${title.replace(/"/g, '\\"')}" --body "${body.replace(/"/g, '\\"')}" ${draftFlag}`; - prCmd = prCmd.trim(); - - console.log(`[CreatePR] Creating PR with command: ${prCmd}`); - const { stdout: prOutput } = await execAsync(prCmd, { - cwd: worktreePath, - env: execEnv, - }); + const args = ["pr", "create", "--base", base]; + if (upstreamRepo && originOwner) { + args.push("--repo", upstreamRepo, "--head", `${originOwner}:${branchName}`); + } else { + args.push("--head", branchName); + } + args.push("--title", title, "--body", body); + if (draft) { + args.push("--draft"); + } + + console.log(`[CreatePR] Creating PR with command: gh ${args.join(' ')}`); + const { stdout: prOutput } = await execFileAsync("gh", args, { + cwd: worktreePath, + env: execEnv, + });
🧹 Nitpick comments (4)
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx (1)
223-243: Consider extracting the hasChanges badge to reduce duplication.The uncommitted changes badge (CircleDot with tooltip) is duplicated in both the main worktree path (lines 223-243) and the non-main path (lines 287-307). Consider extracting this into a shared component or helper function.
🔎 Example: Extract to a separate component
Create a new component in the same file:
function UncommittedChangesBadge({ isSelected, changedFilesCount }: { isSelected: boolean; changedFilesCount?: number; }) { return ( <TooltipProvider> <Tooltip> <TooltipTrigger asChild> <span className={cn( "inline-flex items-center justify-center h-4 min-w-[1rem] px-1 text-[10px] font-medium rounded border", isSelected ? "bg-amber-500 text-amber-950 border-amber-400" : "bg-amber-500/20 text-amber-600 dark:text-amber-400 border-amber-500/30" )}> <CircleDot className="w-2.5 h-2.5 mr-0.5" /> {changedFilesCount ?? "!"} </span> </TooltipTrigger> <TooltipContent> <p>{changedFilesCount ?? "Some"} uncommitted file{changedFilesCount !== 1 ? "s" : ""}</p> </TooltipContent> </Tooltip> </TooltipProvider> ); }Then replace both instances with:
{hasChanges && <UncommittedChangesBadge isSelected={isSelected} changedFilesCount={changedFilesCount} />}Also applies to: 287-307
apps/server/src/routes/worktree/routes/pr-info.ts (2)
89-98: Consider consolidating duplicate regex patterns.The three regex patterns for matching remote URLs are very similar and differ only slightly in structure. This duplication makes maintenance harder.
🔎 Proposed consolidation
- let match = - line.match( - /^(\w+)\s+.*[:/]([^/]+)\/([^/\s]+?)(?:\.git)?\s+\(fetch\)/ - ) || - line.match( - /^(\w+)\s+git@[^:]+:([^/]+)\/([^\s]+?)(?:\.git)?\s+\(fetch\)/ - ) || - line.match( - /^(\w+)\s+https?:\/\/[^/]+\/([^/]+)\/([^\s]+?)(?:\.git)?\s+\(fetch\)/ - ); + // Match SSH (git@host:owner/repo), HTTPS (https://host/owner/repo), or generic (remote host:owner/repo) + const match = line.match( + /^(\w+)\s+(?:git@[^:]+:|https?:\/\/[^/]+\/|.*[:/])([^/]+)\/([^/\s]+?)(?:\.git)?\s+\(fetch\)/ + );
36-269: Add tests to meet coverage thresholds.This new file contributes to the pipeline coverage failure (statements 64.81% < 65%, lines 64.75% < 65%, branches 57.99% < 58%). Add unit tests covering:
- Input validation paths (valid/invalid inputs)
- gh CLI unavailable scenario
- Repository detection fallbacks
- PR found vs not found scenarios
- Error handling paths
Would you like me to generate example test cases for this handler?
apps/server/src/lib/worktree-metadata.ts (1)
142-168: Consider logging skipped metadata files.Lines 158-160 silently skip files that can't be read or parsed. While this provides graceful degradation, it might hide issues. Consider logging a warning when metadata files fail to parse, which could help diagnose configuration problems.
🔎 Proposed enhancement
try { const content = await fs.readFile(metadataPath, "utf-8"); const metadata = JSON.parse(content) as WorktreeMetadata; result.set(metadata.branch, metadata); } catch { - // Skip if file doesn't exist or can't be read + // Skip if file doesn't exist or can't be read + console.warn(`[WorktreeMetadata] Failed to read metadata from ${metadataPath}`); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/server/src/lib/worktree-metadata.ts(1 hunks)apps/server/src/routes/worktree/common.ts(1 hunks)apps/server/src/routes/worktree/routes/create-pr.ts(8 hunks)apps/server/src/routes/worktree/routes/pr-info.ts(1 hunks)apps/ui/src/components/views/board-view.tsx(7 hunks)apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/ui/src/components/views/board-view.tsx (1)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (2)
WorktreeInfo(9-18)PRInfo(37-60)
apps/server/src/routes/worktree/routes/create-pr.ts (2)
apps/server/src/routes/worktree/common.ts (4)
execAsync(17-17)execEnv(65-68)isValidBranchName(79-81)isGhCliAvailable(86-96)apps/server/src/lib/worktree-metadata.ts (1)
updateWorktreePRInfo(106-126)
apps/server/src/lib/worktree-metadata.ts (1)
apps/ui/src/components/views/board-view/worktree-panel/types.ts (1)
WorktreePRInfo(1-7)
apps/server/src/routes/worktree/common.ts (1)
apps/server/src/services/feature-loader.ts (1)
FeatureLoader(49-463)
🪛 GitHub Actions: Test Suite
apps/server/src/routes/worktree/routes/create-pr.ts
[error] 1-1: Coverage thresholds not met: global statements 64.81% (required 65%), lines 64.75% (required 65%), branches 57.99% (required 58%). The command 'vitest run --coverage' did not meet the coverage thresholds.
apps/server/src/routes/worktree/routes/pr-info.ts
[error] 1-1: Coverage thresholds not met: global statements 64.81% (required 65%), lines 64.75% (required 65%), branches 57.99% (required 58%). The command 'vitest run --coverage' did not meet the coverage thresholds.
apps/server/src/lib/worktree-metadata.ts
[error] 1-1: Coverage thresholds not met: global statements 64.81% (required 65%), lines 64.75% (required 65%), branches 57.99% (required 58%). The command 'vitest run --coverage' did not meet the coverage thresholds.
apps/server/src/routes/worktree/common.ts
[error] 1-1: Coverage thresholds not met: global statements 64.81% (required 65%), lines 64.75% (required 65%), branches 57.99% (required 58%). The command 'vitest run --coverage' did not meet the coverage thresholds.
⏰ 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
🔇 Additional comments (12)
apps/ui/src/components/views/board-view.tsx (4)
41-41: LGTM!The import of
PRInfoandWorktreeInfotypes is correctly placed and supports the new PR addressing functionality.
277-287: LGTM!The refactored
branchCardCountsmemoization is cleaner and more efficient by mutating the accumulator directly instead of spreading.
928-928: LGTM!The wiring of
onAddressPRCommentstoWorktreePanelis correct.
1209-1226: Well-structured PR URL propagation.The implementation correctly separates synchronous state updates from asynchronous persistence operations:
- Local state is updated synchronously for all features (Lines 1216-1218)
- Persistence calls are handled concurrently with
Promise.all(Lines 1221-1225)This addresses the concern raised in the previous review comment about using
forEachwith async functions.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx (5)
3-11: LGTM!The imports are clean and properly organized, with all necessary icons, UI components, and types included.
18-19: LGTM!The new props (
hasChanges,changedFilesCount,onAddressPRComments) are properly typed and integrated into the component signature.Also applies to: 47-47, 57-58, 86-86
92-106: LGTM!The border styling logic is well-documented and appropriately prioritizes running state (cyan) over uncommitted changes (amber), providing clear visual feedback.
198-198: LGTM!The application of
borderClassesto the wrapper div correctly applies the computed border styling based on worktree state.
345-345: LGTM!The
onAddressPRCommentscallback is correctly passed toWorktreeActionsDropdown.apps/server/src/routes/worktree/routes/pr-info.ts (1)
197-231: LGTM! Repository info check properly implemented.The code now correctly checks for
targetRepoavailability before attempting to fetch review comments, addressing the previous concern about placeholder interpolation.apps/server/src/routes/worktree/routes/create-pr.ts (1)
48-55: LGTM! Branch name validation added.The new validation using
isValidBranchNameproperly addresses command injection concerns by ensuring branch names only contain safe characters before use in shell commands.apps/server/src/lib/worktree-metadata.ts (1)
29-51: LGTM! Comprehensive branch name sanitization implemented.The sanitization function now properly handles all cross-platform filesystem concerns raised in the previous review:
- Invalid characters for Windows and Unix
- Windows reserved names (CON, PRN, etc.)
- Trailing dots and spaces
- Length limits
- Empty string edge case
| // Unix/Mac paths | ||
| additionalPaths.push( | ||
| "/opt/homebrew/bin", // Homebrew on Apple Silicon | ||
| "/usr/local/bin", // Homebrew on Intel Mac, common Linux location | ||
| "/home/linuxbrew/.linuxbrew/bin", // Linuxbrew | ||
| `${process.env.HOME}/.local/bin`, // pipx, other user installs | ||
| ); |
There was a problem hiding this comment.
Guard against undefined process.env.HOME.
Line 51 uses ${process.env.HOME}/.local/bin in a template literal. If process.env.HOME is undefined (which can happen in some environments), this produces the string "undefined/.local/bin" which passes the .filter(Boolean) check but is an invalid path.
🔎 Proposed fix
} else {
// Unix/Mac paths
additionalPaths.push(
"/opt/homebrew/bin", // Homebrew on Apple Silicon
"/usr/local/bin", // Homebrew on Intel Mac, common Linux location
"/home/linuxbrew/.linuxbrew/bin", // Linuxbrew
- `${process.env.HOME}/.local/bin`, // pipx, other user installs
);
+ if (process.env.HOME) {
+ additionalPaths.push(`${process.env.HOME}/.local/bin`);
+ }
}📝 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.
| // Unix/Mac paths | |
| additionalPaths.push( | |
| "/opt/homebrew/bin", // Homebrew on Apple Silicon | |
| "/usr/local/bin", // Homebrew on Intel Mac, common Linux location | |
| "/home/linuxbrew/.linuxbrew/bin", // Linuxbrew | |
| `${process.env.HOME}/.local/bin`, // pipx, other user installs | |
| ); | |
| // Unix/Mac paths | |
| additionalPaths.push( | |
| "/opt/homebrew/bin", // Homebrew on Apple Silicon | |
| "/usr/local/bin", // Homebrew on Intel Mac, common Linux location | |
| "/home/linuxbrew/.linuxbrew/bin", // Linuxbrew | |
| ); | |
| if (process.env.HOME) { | |
| additionalPaths.push(`${process.env.HOME}/.local/bin`); // pipx, other user installs | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/common.ts around lines 46 to 52, the template
literal `${process.env.HOME}/.local/bin` can produce "undefined/.local/bin" when
process.env.HOME is not set; change the code to guard against undefined by using
a reliable home directory (e.g. require('os').homedir()) or only push the path
when process.env.HOME is truthy — update the additionalPaths push to either use
os.homedir() + '/.local/bin' or wrap the `${process.env.HOME}/.local/bin` entry
in a conditional so it is added only if process.env.HOME is defined.
| let prBadge: JSX.Element | null = null; | ||
| if (worktree.pr) { | ||
| const prState = worktree.pr.state?.toLowerCase() ?? "open"; | ||
| const prStateClasses = (() => { | ||
| // When selected (active tab), use high contrast solid background (paper-like) | ||
| if (isSelected) { | ||
| return "bg-background text-foreground border-transparent shadow-sm"; | ||
| } | ||
|
|
||
| // When not selected, use the colored variants | ||
| switch (prState) { | ||
| case "open": | ||
| case "reopened": | ||
| return "bg-emerald-500/15 dark:bg-emerald-500/20 text-emerald-600 dark:text-emerald-400 border-emerald-500/30 dark:border-emerald-500/40 hover:bg-emerald-500/25"; | ||
| case "draft": | ||
| return "bg-amber-500/15 dark:bg-amber-500/20 text-amber-600 dark:text-amber-400 border-amber-500/30 dark:border-amber-500/40 hover:bg-amber-500/25"; | ||
| case "merged": | ||
| return "bg-purple-500/15 dark:bg-purple-500/20 text-purple-600 dark:text-purple-400 border-purple-500/30 dark:border-purple-500/40 hover:bg-purple-500/25"; | ||
| case "closed": | ||
| return "bg-rose-500/15 dark:bg-rose-500/20 text-rose-600 dark:text-rose-400 border-rose-500/30 dark:border-rose-500/40 hover:bg-rose-500/25"; | ||
| default: | ||
| return "bg-muted text-muted-foreground border-border/60 hover:bg-muted/80"; | ||
| } | ||
| })(); | ||
|
|
||
| const prLabel = `Pull Request #${worktree.pr.number}, ${prState}${worktree.pr.title ? `: ${worktree.pr.title}` : ""}`; | ||
|
|
||
| // Helper to get status icon color for the selected state | ||
| const getStatusColorClass = () => { | ||
| if (!isSelected) return ""; | ||
| switch (prState) { | ||
| case "open": | ||
| case "reopened": | ||
| return "text-emerald-600 dark:text-emerald-500"; | ||
| case "draft": | ||
| return "text-amber-600 dark:text-amber-500"; | ||
| case "merged": | ||
| return "text-purple-600 dark:text-purple-500"; | ||
| case "closed": | ||
| return "text-rose-600 dark:text-rose-500"; | ||
| default: | ||
| return "text-muted-foreground"; | ||
| } | ||
| }; | ||
|
|
||
| prBadge = ( | ||
| <button | ||
| type="button" | ||
| className={cn( | ||
| "ml-1.5 inline-flex items-center gap-1 rounded-full border px-1.5 py-0.5 text-[10px] font-medium transition-colors", | ||
| "focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-1 focus:ring-offset-background", | ||
| "appearance-none cursor-pointer hover:opacity-80 active:opacity-70", // Reset button appearance but keep cursor, add hover/active states | ||
| prStateClasses | ||
| )} | ||
| style={{ | ||
| // Override any inherited button styles | ||
| backgroundImage: "none", | ||
| boxShadow: "none", | ||
| }} | ||
| title={`${prLabel} - Click to open`} | ||
| aria-label={`${prLabel} - Click to open pull request`} | ||
| onClick={(e) => { | ||
| e.stopPropagation(); // Prevent triggering worktree selection | ||
| if (worktree.pr?.url) { | ||
| window.open(worktree.pr.url, "_blank", "noopener,noreferrer"); | ||
| } | ||
| }} | ||
| onKeyDown={(e) => { | ||
| // Prevent event from bubbling to parent button | ||
| e.stopPropagation(); | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| if (worktree.pr?.url) { | ||
| window.open(worktree.pr.url, "_blank", "noopener,noreferrer"); | ||
| } | ||
| } | ||
| }} | ||
| > | ||
| <GitPullRequest className={cn("w-3 h-3", getStatusColorClass())} aria-hidden="true" /> | ||
| <span aria-hidden="true" className={isSelected ? "text-foreground font-semibold" : ""}> | ||
| PR #{worktree.pr.number} | ||
| </span> | ||
| <span className={cn("capitalize", getStatusColorClass())} aria-hidden="true"> | ||
| {prState} | ||
| </span> | ||
| </button> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Invalid HTML: button nested inside Button component.
Lines 154-194 render a <button> element that is later placed inside the parent <Button> component (lines 201-244 for main, 260-308 for non-main). This creates nested interactive elements, which is invalid HTML and causes:
- HTML5 validation errors: Interactive content cannot be nested inside interactive elements.
- Accessibility issues: Screen readers may behave unpredictably with nested buttons.
- Focus management problems: Tab navigation and focus states may conflict.
🔎 Refactor to use a non-button element
Replace the <button> with a <span> or <div> styled to look clickable:
- <button
+ <span
type="button"
+ role="button"
+ tabIndex={0}
className={cn(
"ml-1.5 inline-flex items-center gap-1 rounded-full border px-1.5 py-0.5 text-[10px] font-medium transition-colors",
"focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-1 focus:ring-offset-background",
"appearance-none cursor-pointer hover:opacity-80 active:opacity-70",
prStateClasses
)}
// ... rest of props unchanged
- </button>
+ </span>Remove the type="button" attribute and add role="button" and tabIndex={0} to maintain accessibility.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx
around lines 108 to 195, the code renders a <button> that will be placed inside
a parent <Button>, causing nested interactive elements; replace the inner
<button> with a non-interactive element (e.g., <div> or <span>) styled like a
button, remove the type attribute, add role="button" and tabIndex={0} to
preserve semantics, keep the existing onClick/onKeyDown handlers and
e.stopPropagation() logic, and ensure aria-label/title remain unchanged so
keyboard and screen-reader accessibility are preserved while avoiding invalid
nested buttons.
- Updated the description input locator to use a more specific selector. - Added a visibility check for the description textarea before filling it, improving test reliability.
- Added tests to ensure mkdirSafe handles ELOOP and EEXIST errors gracefully. - Implemented checks for existsSafe to return true for ELOOP errors and throw for other errors. - Improved overall robustness of filesystem utility tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/server/tests/unit/lib/fs-utils.test.ts (1)
69-80: Remove unused variable.Line 71 declares
originalLstatbut never uses it. This appears to be leftover code from development.🔎 Proposed fix
it("should handle ELOOP error gracefully when checking path", async () => { // Mock lstat to throw ELOOP error - const originalLstat = fs.lstat; const mkdirSafePath = path.join(testDir, "eloop-path"); vi.spyOn(fs, "lstat").mockRejectedValueOnce({ code: "ELOOP" });apps/server/tests/unit/lib/worktree-metadata.test.ts (2)
32-69: Good coverage of branch name sanitization edge cases.The tests effectively verify that branch names with special characters and Windows reserved names are handled correctly through the write/read cycle.
Consider adding tests for additional edge cases:
- Empty branch names
- Very long branch names (filesystem path length limits)
- Branch names with only spaces or dots
- Branch names ending with periods (Windows restriction)
Example additional test cases
it("should handle empty branch names gracefully", async () => { const branch = ""; const metadata: WorktreeMetadata = { branch, createdAt: new Date().toISOString(), }; await writeWorktreeMetadata(testProjectPath, branch, metadata); const result = await readWorktreeMetadata(testProjectPath, branch); expect(result).toEqual(metadata); }); it("should handle very long branch names", async () => { const branch = "a".repeat(300); const metadata: WorktreeMetadata = { branch, createdAt: new Date().toISOString(), }; await writeWorktreeMetadata(testProjectPath, branch, metadata); const result = await readWorktreeMetadata(testProjectPath, branch); expect(result).toEqual(metadata); });
1-368: Excellent test suite with comprehensive coverage.This test file demonstrates high-quality testing practices:
- ✅ Complete coverage of all exported functions
- ✅ Thorough edge case testing (malformed JSON, missing files, empty directories)
- ✅ Proper test isolation with temporary directories
- ✅ Clear organization with descriptive test names
- ✅ Proper cleanup to avoid test pollution
The test suite provides strong confidence in the worktree-metadata module's reliability and robustness.
For even more comprehensive coverage, consider adding tests for:
- Concurrent operations (if the module should support them)
- Invalid input types (null/undefined parameters)
- Permission errors (read-only directories)
- Filesystem edge cases (symbolic links, very deeply nested paths)
These are optional enhancements that could further strengthen the test suite but are not essential given the already strong coverage.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/tests/unit/lib/fs-utils.test.ts(2 hunks)apps/server/tests/unit/lib/worktree-metadata.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/server/tests/unit/lib/fs-utils.test.ts (1)
apps/server/src/lib/fs-utils.ts (2)
mkdirSafe(12-47)existsSafe(53-67)
apps/server/tests/unit/lib/worktree-metadata.test.ts (1)
apps/server/src/lib/worktree-metadata.ts (7)
WorktreeMetadata(20-24)writeWorktreeMetadata(88-101)readWorktreeMetadata(71-83)updateWorktreePRInfo(106-126)getWorktreePRInfo(131-137)readAllWorktreeMetadata(142-168)deleteWorktreeMetadata(173-183)
⏰ 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
🔇 Additional comments (11)
apps/server/tests/unit/lib/fs-utils.test.ts (4)
82-94: LGTM!This test correctly verifies the race condition handling where a directory might be created between the existence check and the mkdir call.
96-108: LGTM!This test correctly verifies graceful handling of ELOOP errors that occur during directory creation, which can happen with symlink loops in the path.
154-162: LGTM!This test correctly verifies that ELOOP (symlink loop) is treated as true existence, which aligns with the implementation's design decision that a looping symlink still "exists" as a filesystem entity.
164-171: LGTM!This test correctly verifies that errors other than ENOENT and ELOOP are propagated to the caller, ensuring the function doesn't silently swallow unexpected filesystem errors.
apps/server/tests/unit/lib/worktree-metadata.test.ts (7)
19-30: Excellent test isolation and cleanup.The setup creates isolated temporary directories for each test run, and the cleanup properly handles errors during teardown. This follows best practices for test isolation.
71-107: Comprehensive coverage of read operations.The tests cover all expected scenarios: missing metadata, basic metadata, and metadata with PR information. The assertions correctly verify the full object structure.
109-145: Good coverage of write operations.The tests verify both creating new metadata and overwriting existing metadata, ensuring the write operation is reliable in both scenarios.
147-210: Excellent coverage of PR info updates.The tests thoroughly verify that
updateWorktreePRInfohandles both creation and update scenarios correctly. The test at lines 187-209 is particularly valuable as it ensures existing metadata fields are preserved during PR info updates.
212-244: Complete coverage of PR info retrieval scenarios.The tests verify all possible outcomes: missing metadata, metadata without PR info, and metadata with PR info. This ensures the function behaves correctly in all cases.
246-341: Outstanding edge case coverage for bulk read operations.The test suite for
readAllWorktreeMetadatais exceptionally thorough, covering:
- Missing and empty directories
- Multiple valid entries
- Graceful handling of invalid entries (missing files, non-directories, malformed JSON)
This comprehensive coverage ensures the function is robust against various filesystem states.
343-366: Good coverage of deletion scenarios.The tests verify both successful deletion and graceful handling of non-existent metadata. The idempotent behavior is particularly important for cleanup operations.
Changes from branch pull-request
Summary by CodeRabbit
New Features
UI Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.