Improve pull request flow, add branch selection for worktree creation, fix auto-mode concurrency count#784
Closed
gsxdsm wants to merge 5 commits intoAutoMaker-Org:v0.15.0rcfrom
Closed
Conversation
…eation, fix for automode concurrency count
📝 WalkthroughWalkthroughAdds cross-remote PR targeting and resolution, remote-aware branch detection/fetch for worktree create/rebase/merge, "push after commit" UI, propagation of remotes/trackingRemote through worktree UI, utilities for remote name validation, and multiple UI/layout and PWA safe-area improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client(UI)
participant EA as Electron API
participant Server as Server Route
participant Git as Local Git
participant GH as Remote Host
UI->>EA: createPR({ head, base, targetRemote? })
EA->>Server: POST /worktree/create-pr { ..., targetRemote? }
Server->>Git: git remote -v -> parse remotes (parsedRemotes)
alt targetRemote provided
Server->>Server: validate isValidRemoteName(targetRemote)
Server->>Git: ensure targetRemote ∈ parsedRemotes
Server->>Server: derive targetRepo, pushOwner, upstreamRepo, originOwner
else infer from pushRemote
Server->>Server: infer upstream/origin from parsedRemotes
end
Server->>GH: create PR (gh CLI args use --repo/upstream and --head originOwner:branch if fork)
Server-->>EA: { prUrl, crossRemoteInfo? }
EA-->>UI: open PR URL / show result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for Git workflows by introducing more flexible and intuitive options for pull request creation, worktree management, and commit operations. It allows for independent control over push and target remotes, provides granular control over base branch selection for new worktrees, and streamlines the commit-and-push process. These changes aim to improve efficiency and clarity, particularly in complex repository setups like fork workflows, while also addressing a concurrency bug in auto-mode task tracking. 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. Changelog
Activity
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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to Git operations within the application, primarily focusing on improved flexibility for creating pull requests, worktrees, and handling merge/rebase operations. Key changes include adding a targetRemote option for PR creation, allowing users to specify a different remote for the PR target than for pushing, and updating the UI to support this selection. The create worktree functionality now supports specifying a base branch, including remote branches, with the system automatically fetching from the remote if needed. Merge and rebase operations have been enhanced to include an optional remote parameter, ensuring up-to-date refs by fetching from the specified remote before execution. The UI for committing changes now includes an option to 'Push to remote after commit' with a remote selector. Additionally, several UI texts have been updated to use 'Integrate Branch' instead of 'Merge Branch' for consistency, and the 'Create PR' option is now always visible if no PR exists. The BranchAutocomplete component was also updated to allow more control over branch creation and empty messages.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx (1)
110-110:⚠️ Potential issue | 🟡 MinorIncomplete rename — several user-facing strings still say "merge/merged/merging".
The title (line 277) and error toasts (lines 135, 160) were updated to "Integrate Branch" / "integrate branch", but the following strings were left unchanged, producing an inconsistent UX:
Line Still reads 110 toast.success('Branch merged to ${targetBranch}')108–109 success description: "has been merged into"282 dialog body: Merge <code>…</code> into:330 checkbox label: "Delete worktree and branch after merging"356 spinner text: "Merging…"361 button label: "Merge"🔤 Proposed fix to align all user-facing strings
- toast.success(`Branch merged to ${targetBranch}`, { description }); + toast.success(`Branch integrated into ${targetBranch}`, { description });- ? `Branch "${worktree.branch}" has been merged into "${targetBranch}" and the worktree and branch were deleted` - : `Branch "${worktree.branch}" has been merged into "${targetBranch}"`; + ? `Branch "${worktree.branch}" has been integrated into "${targetBranch}" and the worktree and branch were deleted` + : `Branch "${worktree.branch}" has been integrated into "${targetBranch}"`;- <span className="block"> - Merge <code className="font-mono bg-muted px-1 rounded">{worktree.branch}</code>{' '} - into: - </span> + <span className="block"> + Integrate <code className="font-mono bg-muted px-1 rounded">{worktree.branch}</code>{' '} + into: + </span>- Delete worktree and branch after merging + Delete worktree and branch after integrating- <Spinner size="sm" variant="foreground" className="mr-2" /> - Merging... + <Spinner size="sm" variant="foreground" className="mr-2" /> + Integrating...- <GitMerge className="w-4 h-4 mr-2" /> - Merge + <GitMerge className="w-4 h-4 mr-2" /> + IntegrateAlso applies to: 135-135, 160-160, 277-277, 282-282, 330-330, 356-356, 361-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx` at line 110, Update all remaining user-facing "merge" wording to the new "integrate" terminology inside the MergeWorktreeDialog component: change the toast.success call (toast.success(`Branch merged to ${targetBranch}`, { description })) to use "integrated" (e.g., `Branch integrated into ${targetBranch}` and description "has been integrated into"), update the dialog body text that currently reads "Merge <code>…</code> into:" to "Integrate <code>…</code> into:", change the checkbox label "Delete worktree and branch after merging" to "Delete worktree and branch after integrating", change the spinner text "Merging…" to "Integrating…", and change the action button label "Merge" to "Integrate" so all user-facing strings in MergeWorktreeDialog consistently use "integrate/integrated".
🧹 Nitpick comments (4)
apps/ui/src/store/app-store.ts (1)
945-951: LGTM — duplicate-task guard is correct and consistent.The
Array.includes()check before appending cleanly prevents inflatedrunningTaskscounts whenauto_mode_feature_startfires from multiple services. Returning the samestatereference is the right Zustand no-op idiom (already used at lines 968 and 984 inremoveRunningTask/clearRunningTasks), so no spurious re-renders are triggered.One optional structural alternative worth considering in a future refactor: storing
runningTasksas aSet<string>instead ofstring[]would make uniqueness a structural invariant rather than a guarded invariant, and would reduce the check to O(1). This would require a corresponding type change in@automaker/types, so it's out of scope for this fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/app-store.ts` around lines 945 - 951, The duplicate-task guard using current.runningTasks.includes(taskId) in the auto_mode_feature_start handler is correct — no code change required; keep returning the same state reference to avoid re-renders. If you want a future refactor, consider changing runningTasks from string[] to Set<string> (update the runningTasks type in `@automaker/types` and adjust usages in removeRunningTask and clearRunningTasks accordingly) to make uniqueness O(1).apps/server/src/routes/worktree/routes/create.ts (1)
209-229: Good: targeted fetch of only the specific branch ref.Unlike the broader
fetch <remote>in merge/rebase services, this fetches onlyremoteBranchInfo.branchfrom the remote, which is more efficient for large repos. The non-fatal pattern is consistent with the rest of the PR.One minor note: line 26 imports
execGitCommandfrom the local../../../lib/git.jswhile line 18 importsisGitRepofrom@automaker/git-utils. As per coding guidelines, git operations should use@automaker/git-utils. This is a pre-existing pattern in the file, but worth consolidating when convenient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/create.ts` around lines 209 - 229, This code uses a local execGitCommand (imported from ../../../lib/git.js) while other git helpers like isGitRepo come from `@automaker/git-utils`; replace the local execGitCommand usage/import with the corresponding git utility from `@automaker/git-utils` (update the import and call site where execGitCommand is invoked inside the block that also uses detectRemoteBranch) so all git operations (e.g., the fetch of remoteBranchInfo.remote/remoteBranchInfo.branch) consistently use the shared `@automaker/git-utils` implementation and remove the duplicate local import.apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx (1)
140-146: Effect resetsbaseBranchcorrectly but includes it in its own dependency array — verify no excessive re-renders.The effect reads and conditionally writes
baseBranch, and lists it as a dependency. This is functionally correct (a second trigger will no-op because the new value IS inbranches), but it causes one extra render cycle each time the reset fires. Consider using a functional check that doesn't requirebaseBranchin deps, e.g. usingsetBaseBranch(prev => ...).Optional: remove baseBranch from deps
- useEffect(() => { - if (branches.length > 0 && baseBranch && !branches.includes(baseBranch)) { - // Current base branch is not in the filtered list — pick the best match - const mainBranch = branches.find((b) => b === 'main' || b === 'master'); - setBaseBranch(mainBranch || branches[0]); - } - }, [branches, baseBranch]); + useEffect(() => { + if (branches.length > 0) { + setBaseBranch((prev) => { + if (prev && branches.includes(prev)) return prev; + return branches.find((b) => b === 'main' || b === 'master') || branches[0]; + }); + } + }, [branches]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx` around lines 140 - 146, The effect currently depends on both branches and baseBranch, causing an extra render when it reads then writes baseBranch; change the effect to depend only on branches and use the functional updater form of setBaseBranch to check previous baseBranch (setBaseBranch(prev => { if (prev && branches.includes(prev)) return prev; const mainBranch = branches.find(b => b === 'main' || b === 'master'); return mainBranch || branches[0]; })) so you can remove baseBranch from the dependency array; update the useEffect that references branches, baseBranch and setBaseBranch accordingly to avoid the extra render.apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx (1)
42-45:RemoteInfois duplicated across dialog files.The same
{ name: string; url: string }shape exists increate-pr-dialog.tsx(with an extra optionalbranchesfield). Consider extracting a sharedRemoteInfotype to reduce duplication. Low priority since the type is small.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx` around lines 42 - 45, The RemoteInfo interface in commit-worktree-dialog.tsx duplicates the same shape used in create-pr-dialog.tsx (the other file adds an optional branches field); extract a single shared type (e.g., export type RemoteInfo = { name: string; url: string; branches?: string[] }) into a common module (a shared types file or a sibling index) and import it into both CommitWorktreeDialog and CreatePrDialog components, updating references to RemoteInfo in both files to use the centralized exported type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/create-pr.ts`:
- Around line 236-257: The handler currently ignores a client-supplied
targetRemote when parsedRemotes.get(targetRemote) is undefined; add an explicit
validation step before the override block: if targetRemote is provided (and
passes isValidRemoteName earlier) but parsedRemotes.has(targetRemote) is false,
immediately return a 400 error response indicating the requested targetRemote is
not found. Implement this check right before the if (targetRemote &&
parsedRemotes.size > 0) block (referencing targetRemote, parsedRemotes, and
parsedRemotes.get) so the endpoint fails fast instead of silently falling back
to auto-detection.
In `@apps/server/src/services/merge-service.ts`:
- Around line 93-104: Validate the remote parameter before calling
execGitCommand(['fetch', remote], projectPath) to prevent git option injection:
import and use the existing isValidRemoteName helper (from
apps/server/src/routes/worktree/common.ts) in merge-service.ts,
rebase-service.ts, and pull-service.ts, check the remote value (or default) with
isValidRemoteName and if it fails return/throw the same kind of error you use
for invalid branch names (or log and abort the fetch), ensuring you validate
before invoking execGitCommand so only safe remote names are passed.
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 369-394: The dialog can be dismissed mid-push; prevent that by
short-circuiting any external open-change requests while a push is in progress:
when using the CommitWorktreeDialog's local state (setIsPushing / isPushing)
wrap or replace the onOpenChange handler so it no-ops while isPushing is true
(i.e., ignore calls to onOpenChange(false) from overlay clicks or Escape during
a push), and ensure the UI still clears isPushing in the push finally block and
then calls onOpenChange(false) only after push completion (hook into the
existing push logic that uses api.worktree.push, pushAfterCommit,
selectedRemote, onCommitted).
In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx`:
- Around line 131-132: The fetch error for api.worktree.listBranches in
CreateWorktreeDialog is currently swallowed, leaving availableBranches empty
and, because BranchAutocomplete is rendered with allowCreate={false}, the user
cannot enter a custom branch; update the catch block in the effect that calls
api.worktree.listBranches to (1) set a visible inline error state (e.g.,
branchFetchError) or a short message to show to the user, and (2) populate
availableBranches with a safe fallback (e.g., ['main'] or previously cached
branches) and/or set a local flag to enable allowCreate as a fallback; then pass
that flag or error state into the BranchAutocomplete render (replace
allowCreate={false} with allowCreate={enableCreateFallback}) so the user can
type a custom branch when the fetch fails and display the error inline near the
autocomplete.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 220-223: Update the stale JSX comment that still describes the old
behavior so it matches the new logic where showCreatePR is computed as "!hasPR"
(i.e., Create PR is shown for any worktree with no existing PR); locate the
comment adjacent to the worktree actions JSX that references PR visibility (the
block related to showCreatePR, showPRInfo, hasChangesSectionContent) and replace
the outdated text ("Show PR option for non-primary worktrees, or primary
worktree with changes") with a concise note like "Show Create PR when no
existing PR is linked" to reflect the new behavior of showCreatePR, leaving the
surrounding logic (showCreatePR, showPRInfo, hasChangesSectionContent)
unchanged.
---
Outside diff comments:
In `@apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx`:
- Line 110: Update all remaining user-facing "merge" wording to the new
"integrate" terminology inside the MergeWorktreeDialog component: change the
toast.success call (toast.success(`Branch merged to ${targetBranch}`, {
description })) to use "integrated" (e.g., `Branch integrated into
${targetBranch}` and description "has been integrated into"), update the dialog
body text that currently reads "Merge <code>…</code> into:" to "Integrate
<code>…</code> into:", change the checkbox label "Delete worktree and branch
after merging" to "Delete worktree and branch after integrating", change the
spinner text "Merging…" to "Integrating…", and change the action button label
"Merge" to "Integrate" so all user-facing strings in MergeWorktreeDialog
consistently use "integrate/integrated".
---
Duplicate comments:
In `@apps/server/src/services/rebase-service.ts`:
- Around line 72-83: The code passes options?.remote directly into
execGitCommand(['fetch', remote]) which allows a value starting with '-' to be
interpreted as a flag; update the rebase logic to validate/sanitize the remote
variable (the remote assignment and the fetch call using execGitCommand) by
checking if options?.remote exists and does not startWith('-'); if it does start
with '-', log a warning via logger.warn and fall back to the safe default
'origin' (or reject the value), then call execGitCommand(['fetch', remoteSafe],
worktreePath); ensure the same guarded behavior mirrors the fix used in
merge-service.ts.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/create.ts`:
- Around line 209-229: This code uses a local execGitCommand (imported from
../../../lib/git.js) while other git helpers like isGitRepo come from
`@automaker/git-utils`; replace the local execGitCommand usage/import with the
corresponding git utility from `@automaker/git-utils` (update the import and call
site where execGitCommand is invoked inside the block that also uses
detectRemoteBranch) so all git operations (e.g., the fetch of
remoteBranchInfo.remote/remoteBranchInfo.branch) consistently use the shared
`@automaker/git-utils` implementation and remove the duplicate local import.
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 42-45: The RemoteInfo interface in commit-worktree-dialog.tsx
duplicates the same shape used in create-pr-dialog.tsx (the other file adds an
optional branches field); extract a single shared type (e.g., export type
RemoteInfo = { name: string; url: string; branches?: string[] }) into a common
module (a shared types file or a sibling index) and import it into both
CommitWorktreeDialog and CreatePrDialog components, updating references to
RemoteInfo in both files to use the centralized exported type.
In `@apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx`:
- Around line 140-146: The effect currently depends on both branches and
baseBranch, causing an extra render when it reads then writes baseBranch; change
the effect to depend only on branches and use the functional updater form of
setBaseBranch to check previous baseBranch (setBaseBranch(prev => { if (prev &&
branches.includes(prev)) return prev; const mainBranch = branches.find(b => b
=== 'main' || b === 'master'); return mainBranch || branches[0]; })) so you can
remove baseBranch from the dependency array; update the useEffect that
references branches, baseBranch and setBaseBranch accordingly to avoid the extra
render.
In `@apps/ui/src/store/app-store.ts`:
- Around line 945-951: The duplicate-task guard using
current.runningTasks.includes(taskId) in the auto_mode_feature_start handler is
correct — no code change required; keep returning the same state reference to
avoid re-renders. If you want a future refactor, consider changing runningTasks
from string[] to Set<string> (update the runningTasks type in `@automaker/types`
and adjust usages in removeRunningTask and clearRunningTasks accordingly) to
make uniqueness O(1).
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx (1)
311-311:⚠️ Potential issue | 🟡 MinorMissed text update: "before merging" should be "before integrating".
This warning text was not updated along with the rest of the dialog, leaving a user-visible inconsistency.
📝 Proposed fix
- commit or discard them before merging. + commit or discard them before integrating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx` at line 311, The warning string "commit or discard them before merging." in the MergeWorktreeDialog component should be updated to "commit or discard them before integrating." Locate the message text in apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx (inside the MergeWorktreeDialog render/JSX) and replace the old phrase with the new one so the dialog matches the updated wording elsewhere.apps/server/src/routes/worktree/routes/create-pr.ts (1)
296-307:⚠️ Potential issue | 🟡 MinorAdd validation for
baseBranchparameter or percent-encode it in the browser URL.
baseBranchis an optional request parameter (line 32) that flows directly into the URL at line 303/306 without validation or encoding. WhilebranchNameis validated withisValidBranchName()(which rejects special characters like#,%, spaces),baseBranchlacks this validation. If a user passesbaseBranchcontaining characters like#or%, the GitHub compare URL would be malformed.The fix is either to:
- Apply
isValidBranchName()validation tobaseBranchbefore use, or- Percent-encode
basewhen constructing the URL (consistent with title/body encoding at lines 298–299)Note:
upstreamRepoandoriginOwnerare derived from git remote config (parsed from git URLs), not directly from user input, so they are less at risk despitetargetRemotebeing user-supplied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/create-pr.ts` around lines 296 - 307, baseBranch (used as variable base) is currently inserted into the GitHub compare URL without validation or encoding; either run it through the same isValidBranchName() check used for branchName or percent-encode it with encodeURIComponent before building browserUrl. Update the code that constructs browserUrl (the branches comparison string that uses base and branchName in both the upstreamRepo/originOwner and regular-repo branches) to use the validated/encoded base value so special chars like # or % can’t produce a malformed URL; keep encodedTitle/encodedBody usage as-is.
🧹 Nitpick comments (3)
apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx (2)
131-133: Conflict resolution dialog/toasts still use "Merge" terminology — consider aligning with "Integrate".The success path and most button labels now say "integrate", but the conflict toast titles ("Merge conflicts detected"), the conflict dialog title ("Merge Conflicts Detected"), and the description text ("There are conflicts when merging") were left untouched. Whether this is intentional (preserving the technical git term) or an oversight is worth confirming. If the intent is a full UX rebrand to "Integrate", these should also be updated.
Also applies to: 156-158, 192-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx` around lines 131 - 133, Update the remaining UI strings that use "Merge" to instead use "Integrate" within the MergeWorktreeDialog component: replace the toast.error title 'Merge conflicts detected', the dialog title 'Merge Conflicts Detected', and any description text like 'There are conflicts when merging' (also the occurrences around the blocks noted at 156-158 and 192-199) so they read with "Integrate" (e.g., 'Integrate conflicts detected', 'Integrate Conflicts Detected', 'There are conflicts when integrating'); ensure you update the toast.error call and the dialog title/description props in MergeWorktreeDialog so wording is consistent with the rest of the success/button labels.
27-28: Align prop naming and JSDoc with "Integrate" terminology used in UI.The component displays "Branch integrated into..." in toast messages and the button says "Integrate Branch", but the prop callback is
onMergedwith JSDoc saying "merge is successful". This terminology mismatch is visible to callers. Rename the prop toonIntegrated, the parameter tointegratedWorktree, and update the JSDoc to reference "integration" for consistency.Call sites using
onMergedappear in worktree-panel.tsx and will need updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx` around lines 27 - 28, Rename the prop and JSDoc to use "Integrate" terminology: change the prop signature from onMerged: (mergedWorktree: WorktreeInfo, deletedBranch: boolean) => void to onIntegrated: (integratedWorktree: WorktreeInfo, deletedBranch: boolean) => void and update the JSDoc comment to reference "integration" (e.g., "Called when integration is successful. integratedWorktree indicates the integrated worktree and deletedBranch indicates if the branch was also deleted."). Update all call sites that use onMerged (e.g., where worktree-panel.tsx invokes the callback) to call onIntegrated and rename the first argument from mergedWorktree to integratedWorktree to match the new signature.apps/server/src/services/merge-service.ts (1)
40-42: JSDoc@param optionsis stale — missingremote.📝 Proposed update
- * `@param` options - Merge options (squash, message, deleteWorktreeAndBranch) + * `@param` options - Merge options (squash, message, deleteWorktreeAndBranch, remote)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/merge-service.ts` around lines 40 - 42, The JSDoc for the merge function in merge-service.ts is out of date: the `@param` options docblock does not document the options.remote property; update the JSDoc for the function that accepts targetBranch and options (the merge service method in this file) to include a line documenting `@param` options.remote (describe its purpose, type, and default behavior) alongside the existing `@param` options entries so the docs reflect the actual parameter shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/create-pr.ts`:
- Around line 244-269: When targetRemote !== pushRemote and parsedRemotes
contains targetRemote but parsedRemotes.get(pushRemote) returns undefined
(pushInfo undefined), the code ends up setting originOwner to null and later
builds an incorrect browser URL; update the cross-remote branch in the create-pr
handler to detect missing pushInfo and fail fast: check pushInfo after const
pushInfo = parsedRemotes.get(pushRemote) and if undefined either throw / return
an error response (or at minimum processLogger.warn and return an error) instead
of continuing, so you don't set originOwner/upstreamRepo/repoUrl incorrectly;
reference targetRemote, pushRemote, parsedRemotes, targetInfo, pushInfo,
pushOwner, originOwner, upstreamRepo, and repoUrl when adding the defensive
check and error handling.
In `@apps/server/src/services/merge-service.ts`:
- Line 10: The service imports isValidRemoteName from a route module which
violates the shared-package rule; extract isValidRemoteName (and any related
validation) into a canonical shared location (e.g., `@automaker/utils` or
apps/server/src/services/common.ts), update merge-service.ts to import it from
that shared module instead of '../routes/worktree/common.js', and update any
route handlers to also import from the new shared location; additionally, update
the JSDoc on the merge function (the comment around lines ~40–41 in
merge-service.ts) to list all options including the newly added remote parameter
(e.g., "squash, message, deleteWorktreeAndBranch, remote").
- Around line 94-105: The current logic in merge-service that assigns const
rawRemote = options?.remote || 'origin' and then falls back to remote = 'origin'
when isValidRemoteName(rawRemote) is false should instead reject invalid remotes
like branch validation does: remove the silent fallback and return an error when
isValidRemoteName(rawRemote) fails (include the invalid value in the error/log),
so the caller knows their remote was rejected; update the code paths that use
remote (the pre-merge fetch) to only proceed when a validated remote is present,
and apply the same change to the identical logic in rebase-service (the
rawRemote/remote + isValidRemoteName check) so both services consistently
hard-fail on invalid remote names.
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 728-730: The span showing the remote URL is currently inline so
Tailwind's truncate and max-w-[200px] don't apply; update the element used in
CommitWorktreeDialog (the <span> containing {remote.url}) to be an inline-block
(e.g., add the "inline-block" utility or convert to a <div> with inline-block
behavior) so truncate and max-w-[200px] take effect and long URLs are
ellipsized.
- Around line 213-250: The effect that fetches remotes (the useEffect guarded by
pushAfterCommit, worktree, remotes.length) can loop forever when listRemotes
returns an empty array; add a new boolean state remotesFetched (with setter
setRemotesFetched) and change the effect guard to run only when !remotesFetched
instead of remotes.length === 0; inside fetchRemotes setRemotesFetched(true)
after any successful call (even if result.remotes is empty) and still call
setRemotes(result.remotes) as before; also reset remotesFetched to false
wherever you currently reset remotes when the dialog opens (the dialog-open
effect) so fetching will re-run on new dialogs.
In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx`:
- Around line 117-147: fetchBranches currently never clears branchFetchError on
a successful call and also silently ignores API responses where
branchResult.success === false; update fetchBranches to (1) call
setBranchFetchError(null) when branchResult.success and branchResult.result are
present and you successfully call setAvailableBranches, and (2) treat
branchResult.success === false as an error path: setBranchFetchError to a
user-facing message and setAvailableBranches to the safe fallback [{ name:
'main', isRemote: false }], so the UI shows the fallback and allowCreate
behavior consistently; use the existing identifiers fetchBranches, branchResult,
setBranchFetchError, setAvailableBranches and setIsLoadingBranches to locate and
implement these changes.
In `@apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx`:
- Around line 476-486: The fire-and-forget call to api.worktree.listRemotes
(obtained via getHttpApiClient()) lacks a .catch, causing unhandled promise
rejections; wrap the promise returned by api.worktree.listRemotes(worktree.path)
with a .catch that swallows or logs the error (using console.warn or the app
logger) and does not rethrow, while keeping the existing success handling that
calls setRemotesCache for worktree.path — ensure the catch is attached to the
same promise chain so any network/server rejection is suppressed.
---
Outside diff comments:
In `@apps/server/src/routes/worktree/routes/create-pr.ts`:
- Around line 296-307: baseBranch (used as variable base) is currently inserted
into the GitHub compare URL without validation or encoding; either run it
through the same isValidBranchName() check used for branchName or percent-encode
it with encodeURIComponent before building browserUrl. Update the code that
constructs browserUrl (the branches comparison string that uses base and
branchName in both the upstreamRepo/originOwner and regular-repo branches) to
use the validated/encoded base value so special chars like # or % can’t produce
a malformed URL; keep encodedTitle/encodedBody usage as-is.
In `@apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx`:
- Line 311: The warning string "commit or discard them before merging." in the
MergeWorktreeDialog component should be updated to "commit or discard them
before integrating." Locate the message text in
apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx
(inside the MergeWorktreeDialog render/JSX) and replace the old phrase with the
new one so the dialog matches the updated wording elsewhere.
---
Duplicate comments:
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 912-913: The JSX comment above the conditional rendering
referencing showCreatePR is stale and describes the old logic; update or remove
it so it matches the current condition where showCreatePR is simply !hasPR.
Locate the JSX comment near the conditional that renders when showCreatePR is
true in worktree-actions-dropdown.tsx and either rewrite it to reflect the new
meaning (e.g., "Show PR option when there is no existing PR (showCreatePR ===
!hasPR)") or remove the comment altogether to avoid confusion.
---
Nitpick comments:
In `@apps/server/src/services/merge-service.ts`:
- Around line 40-42: The JSDoc for the merge function in merge-service.ts is out
of date: the `@param` options docblock does not document the options.remote
property; update the JSDoc for the function that accepts targetBranch and
options (the merge service method in this file) to include a line documenting
`@param` options.remote (describe its purpose, type, and default behavior)
alongside the existing `@param` options entries so the docs reflect the actual
parameter shape.
In `@apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx`:
- Around line 131-133: Update the remaining UI strings that use "Merge" to
instead use "Integrate" within the MergeWorktreeDialog component: replace the
toast.error title 'Merge conflicts detected', the dialog title 'Merge Conflicts
Detected', and any description text like 'There are conflicts when merging'
(also the occurrences around the blocks noted at 156-158 and 192-199) so they
read with "Integrate" (e.g., 'Integrate conflicts detected', 'Integrate
Conflicts Detected', 'There are conflicts when integrating'); ensure you update
the toast.error call and the dialog title/description props in
MergeWorktreeDialog so wording is consistent with the rest of the success/button
labels.
- Around line 27-28: Rename the prop and JSDoc to use "Integrate" terminology:
change the prop signature from onMerged: (mergedWorktree: WorktreeInfo,
deletedBranch: boolean) => void to onIntegrated: (integratedWorktree:
WorktreeInfo, deletedBranch: boolean) => void and update the JSDoc comment to
reference "integration" (e.g., "Called when integration is successful.
integratedWorktree indicates the integrated worktree and deletedBranch indicates
if the branch was also deleted."). Update all call sites that use onMerged
(e.g., where worktree-panel.tsx invokes the callback) to call onIntegrated and
rename the first argument from mergedWorktree to integratedWorktree to match the
new signature.
Comment on lines
94
to
105
| // Validate and sanitize the remote name to prevent git option injection. | ||
| // If the caller supplied an invalid remote name, fall back to 'origin' and | ||
| // log a warning rather than passing a potentially dangerous value to git. | ||
| const rawRemote = options?.remote || 'origin'; | ||
| let remote = rawRemote; | ||
| if (!isValidRemoteName(rawRemote)) { | ||
| logger.warn('Invalid remote name supplied to merge-service; falling back to "origin"', { | ||
| remote: rawRemote, | ||
| projectPath, | ||
| }); | ||
| remote = 'origin'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, search for isValidRemoteName implementation to verify it rejects injection patterns
rg -n "isValidRemoteName" --type=ts -A 10 -B 2Repository: AutoMaker-Org/automaker
Length of output: 10711
🏁 Script executed:
#!/bin/bash
# Search for branch name validation logic in merge-service.ts to understand the "above" reference
fd merge-service.ts --type=ts -x cat -n {} | head -120Repository: AutoMaker-Org/automaker
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Check rebase-service for similar patterns with remote/branch validation
rg -n "rebase-service" --type=ts -lRepository: AutoMaker-Org/automaker
Length of output: 251
🏁 Script executed:
#!/bin/bash
# Read the full merge-service.ts file to see branch validation and return types
cat -n apps/server/src/services/merge-service.ts | head -150Repository: AutoMaker-Org/automaker
Length of output: 6374
🏁 Script executed:
#!/bin/bash
# Also check the MergeServiceResult type definition and how errors are returned
rg -n "MergeServiceResult\|interface.*Result\|type.*Result" apps/server/src/services/merge-service.ts -A 5Repository: AutoMaker-Org/automaker
Length of output: 49
Silent remote fallback to 'origin' masks configuration intent and can fetch from wrong ref namespace in multi-remote scenarios.
The code silently substitutes 'origin' when options.remote fails validation, without returning an error. This inconsistency contrasts sharply with the hard-reject pattern used for branch name validation above (lines 61–72), where invalid branchName or targetBranch return an error immediately.
In fork-PR workflows, silently using 'origin' instead of a caller-specified remote (e.g., 'upstream') means the pre-merge fetch pulls from the wrong remote namespace, causing the merge to proceed against stale remote-tracking branches without the caller knowing their remote choice was ignored.
Align with the existing branch-validation pattern by returning a hard error:
Suggested fix
- const rawRemote = options?.remote || 'origin';
- let remote = rawRemote;
- if (!isValidRemoteName(rawRemote)) {
- logger.warn('Invalid remote name supplied to merge-service; falling back to "origin"', {
- remote: rawRemote,
- projectPath,
- });
- remote = 'origin';
- }
+ const remote = options?.remote || 'origin';
+ if (!isValidRemoteName(remote)) {
+ return {
+ success: false,
+ error: `Invalid remote name: "${remote}"`,
+ };
+ }Note: Apply the same fix to rebase-service.ts (lines 76–84), which uses an identical fallback pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/merge-service.ts` around lines 94 - 105, The current
logic in merge-service that assigns const rawRemote = options?.remote ||
'origin' and then falls back to remote = 'origin' when
isValidRemoteName(rawRemote) is false should instead reject invalid remotes like
branch validation does: remove the silent fallback and return an error when
isValidRemoteName(rawRemote) fails (include the invalid value in the error/log),
so the caller knows their remote was rejected; update the code paths that use
remote (the pre-merge fetch) to only proceed when a validated remote is present,
and apply the same change to the identical logic in rebase-service (the
rawRemote/remote + isValidRemoteName check) so both services consistently
hard-fail on invalid remote names.
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
apps/ui/public/sw.js (2)
320-371: Cache-first navigation strategy is well-structured for SPA use.The approach is sound for an SPA where the shell rarely changes meaningfully between deploys. The preload response is correctly consumed in only one branch (cached path vs. first-visit path), and the
/fallback on line 334 correctly handles client-side routing.One minor note: on line 362,
cache.putis fire-and-forget (not awaited, not wrapped inwaitUntil). If the SW is terminated very quickly after the first visit, the shell might not be cached until the next navigation. In practice this is unlikely to matter since the SW stays alive while streaming the response, but wrapping it inevent.waitUntilwould be more robust — consistent with how you handle it on line 353.- cache.put(event.request, response.clone()); + event.waitUntil(cache.put(event.request, response.clone()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/public/sw.js` around lines 320 - 371, The first-visit branch in the navigation handler uses cache.put(event.request, response.clone()) as fire-and-forget inside the try block of the event.respondWith handler; wrap that cache update in event.waitUntil (or otherwise ensure the promise is observed) so the Service Worker isn't terminated before the shell is cached—mirror the pattern used by the updateCache() helper (and the cached-path where you call event.waitUntil(updateCache())), targeting the cache.put call in the anonymous async passed to event.respondWith.
253-264: Background fetch may be terminated before cache update completes on mobile.When the 2-second timeout wins the race (line 261) and the stale
cachedResponseis returned,fetchPromisecontinues in the background withoutevent.waitUntil(). On mobile — the exact environment this code targets — the SW can be killed aggressively, losing the cache update.Consider extending the event's lifetime so the background fetch completes:
Suggested fix
if (cachedResponse) { - // Give network a brief window (2s) to respond, otherwise use stale cache - const networkResult = await Promise.race([ + const fetchWithCacheUpdate = fetchPromise.catch(() => null); + // Keep SW alive until the background fetch completes + event.waitUntil(fetchWithCacheUpdate); + // Give network a brief window (2s) to respond, otherwise use stale cache + const networkResult = await Promise.race([ fetchPromise, new Promise((resolve) => setTimeout(() => resolve(null), 2000)), ]); return networkResult || cachedResponse;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/public/sw.js` around lines 253 - 264, When the 2s timeout wins and cachedResponse is returned, the ongoing fetchPromise (from startNetworkFetch) must be kept alive via the service worker event's lifetime so the cache update can complete; update the branch that handles cachedResponse to call event.waitUntil(fetchPromise.then(...).catch(...)) (or .finally(...)) so the background fetch continues to completion and updates the cache even after returning cachedResponse, referencing fetchPromise and startNetworkFetch to locate the code and using event.waitUntil to extend the worker lifetime.apps/ui/src/components/ui/git-diff-panel.tsx (1)
927-976: Floating Promises inonClickhandlers + duplicated staging controls blockLines 956 and 967 call async functions without
void:onClick={() => handleUnstageFile(file.path)} // line 956 onClick={() => handleStageFile(file.path)} // line 967This is inconsistent with
onClick={() => void loadDiffs()}used at lines 768 and 872 in the same file. While internaltry/catchprevents crashes, the floating Promise is a lint hazard and breaks the codebase's established pattern.Additionally, the staging controls block here (lines 948–974) is nearly identical to
FileDiffSection's staging block (lines 391–425). Even as a "safety net," diverging copies raise maintenance risk when the staging UI evolves.Proposed fixes
Fix the unhandled Promises:
- onClick={() => handleUnstageFile(file.path)} + onClick={() => void handleUnstageFile(file.path)} ... - onClick={() => handleStageFile(file.path)} + onClick={() => void handleStageFile(file.path)}Long-term, consider extracting a shared
FileRowcomponent that owns the two-zone layout + staging controls, then composing it in bothFileDiffSectionand this fallback:// Shared component function FileRow({ fileIcon, filePath, stagingState, enableStaging, isStagingFile, onStage, onUnstage, children }: FileRowProps) { return ( <div className="w-full px-3 py-2 flex flex-col gap-1 text-left bg-card sm:flex-row sm:items-center sm:gap-2"> {children /* left zone (clickable or static) */} <div className="flex items-center gap-2 flex-shrink-0 pl-6 sm:pl-0"> {/* shared indicators + staging controls */} </div> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/git-diff-panel.tsx` around lines 927 - 976, The onClick handlers create floating Promises: change onClick={() => handleUnstageFile(file.path)} and onClick={() => handleStageFile(file.path)} to the established pattern (e.g. onClick={() => void handleUnstageFile(file.path)}) so the async calls are explicitly ignored and satisfy the lint rule; then remove the duplicated staging controls by extracting and reusing a shared component (e.g. FileRow or FileStagingControls) used by both this fallback block and FileDiffSection so the staging UI (stagingState, stagingInProgress, handleStageFile, handleUnstageFile, enableStaging) is implemented once and composed in both places.apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx (1)
195-198:isRemoteBaseBranchis alwaysfalsefor manually typed remote branch names.When
allowCreate={!!branchFetchError}istrue(fetch failed,availableBranchesis[{ name: 'main', isRemote: false }]), a user who typesorigin/featuremanually won't see the "Remote branch — will fetch latest" hint (line 352) and the button label stays "Creating…" rather than "Fetching & Creating…". ThebaseBranchvalue is still forwarded to the API correctly, so this is a UX labeling gap only.A simple heuristic to cover the typed case:
♻️ Suggested improvement
const isRemoteBaseBranch = useMemo(() => { if (!baseBranch) return false; - return availableBranches.some((b) => b.name === baseBranch && b.isRemote); + // Prefer the authoritative list; fall back to name-based heuristic when + // allowCreate is active (fetch failed) and the branch wasn't in the list. + const fromList = availableBranches.some((b) => b.name === baseBranch && b.isRemote); + if (fromList) return true; + // Treat "remote/branch" patterns as remote when the list is unavailable. + const inList = availableBranches.some((b) => b.name === baseBranch); + return !inList && baseBranch.includes('/'); }, [baseBranch, availableBranches]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx` around lines 195 - 198, isRemoteBaseBranch currently only checks availableBranches and thus returns false when the user types a remote branch like "origin/feature"; update the isRemoteBaseBranch computation (used by baseBranch, availableBranches, allowCreate, branchFetchError) to also detect manually entered remote-style names by checking the baseBranch string pattern (e.g., contains '/' or matches /^remoteName\/.+$/ or startsWith('origin/')) and treat those as remote when availableBranches doesn't contain a match so the UI shows the "Remote branch — will fetch latest" hint and switches the button label to "Fetching & Creating…".apps/server/src/services/merge-service.ts (1)
97-123: Past remote-validation issues fully addressed — one cosmetic nit.All three concerns from prior reviews have been resolved: import now sourced from
@automaker/utils, invalid remote names return a hard error (not a silent fallback), and validation precedes any git operation.The only minor nit:
const remote = rawRemote;on line 111 is a no-op alias — after the validation block,rawRemoteis already the validated value. You can drop the intermediatelet rawRemote / const remotesplit and just use a singleconst remote.🔧 Suggested simplification
- const rawRemote = options?.remote || 'origin'; - if (!isValidRemoteName(rawRemote)) { + const remote = options?.remote || 'origin'; + if (!isValidRemoteName(remote)) { logger.warn('Invalid remote name supplied to merge-service', { - remote: rawRemote, + remote, projectPath, }); return { success: false, - error: `Invalid remote name: "${rawRemote}"`, + error: `Invalid remote name: "${remote}"`, }; } - const remote = rawRemote;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/merge-service.ts` around lines 97 - 123, The const remote = rawRemote alias is unnecessary; simplify by using a single validated variable: replace rawRemote + remote usage with one const (e.g., const remote = options?.remote || 'origin'), keep the isValidRemoteName(remote) check and the logger/execGitCommand(projectPath) calls unchanged, and remove the redundant rawRemote declaration so all subsequent references use remote directly (ensuring logger.warn and execGitCommand still reference the same symbol).libs/utils/src/git-validation.ts (1)
66-73:isValidRemoteNameimplementation — solid overall; one redundant check.The guard
!name || name.length === 0is redundant:!namealready short-circuits on an empty string (and is the only falsy value reachable from astringparameter). Thename.length === 0branch is dead code.🔧 Suggested simplification
export function isValidRemoteName(name: string): boolean { - if (!name || name.length === 0 || name.length >= MAX_BRANCH_NAME_LENGTH) return false; + if (!name || name.length >= MAX_BRANCH_NAME_LENGTH) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/utils/src/git-validation.ts` around lines 66 - 73, The initial guard in isValidRemoteName redundantly checks name.length === 0 after !name; remove the dead branch so the first line becomes a single falsy/length and max-length check (e.g. if (!name || name.length >= MAX_BRANCH_NAME_LENGTH) return false;), leaving the other guards (startsWith '-', startsWith '.', '..', '/', '\0' and the final regex /^[a-zA-Z0-9._-]+$/) and the MAX_BRANCH_NAME_LENGTH reference unchanged to preserve behavior.apps/server/src/services/rebase-service.ts (1)
72-98: Consistent withmerge-service.ts; same redundant alias nit applies.The
rawRemote→const remote = rawRemotepattern is a no-op alias identical tomerge-service.ts. Collapsing it keeps the diff minimal.🔧 Suggested simplification (mirrors merge-service fix)
- const rawRemote = options?.remote || 'origin'; - if (!isValidRemoteName(rawRemote)) { + const remote = options?.remote || 'origin'; + if (!isValidRemoteName(remote)) { logger.warn('Invalid remote name supplied to rebase-service', { - remote: rawRemote, + remote, worktreePath, }); return { success: false, - error: `Invalid remote name: "${rawRemote}"`, + error: `Invalid remote name: "${remote}"`, }; } - const remote = rawRemote;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/rebase-service.ts` around lines 72 - 98, The code creates a redundant alias by assigning const remote = rawRemote after validating rawRemote; remove the no-op alias and use rawRemote directly (or rename rawRemote to remote) to collapse the variables in rebase-service.ts—update references to use the single name (e.g., use rawRemote in the isValidRemoteName check and in the execGitCommand(['fetch', remote], worktreePath) call or rename rawRemote to remote) so only one variable is used around isValidRemoteName, rawRemote/remote, and the fetch error handling (fetchError/getErrorMessage).apps/server/src/routes/worktree/routes/create-pr.ts (1)
23-285: Consider extracting the PR creation logic into a service.
This route is accruing more business logic; moving the new target-remote handling into a service would better align with the server layering rules.As per coding guidelines: “Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/create-pr.ts` around lines 23 - 285, The createCreatePRHandler route contains substantial business logic around parsing remotes and computing targetRepo/pushOwner/pushRemote/upstreamRepo (see parsedRemotes map population and the targetRemote handling block); extract this into a new service function (e.g., services/prService.ts export an async function like resolvePrTarget({worktreePath, pushRemote, targetRemote})) that accepts the repo path and remote inputs and returns structured data ({repoUrl, targetRepo, pushOwner, upstreamRepo, originOwner, parsedRemotes}) or throws validation errors; update createCreatePRHandler to call resolvePrTarget and handle its returned values or errors, and move all parsing, validation (including the targetRemote existence check) and related logging from createCreatePRHandler into that service.apps/ui/src/components/ui/dialog.tsx (1)
88-91:hasCustomMaxWidthstring detection is fragile — consider an explicit prop.
className.includes('max-w-')works for every current consumer but is an implicit convention: any class containing the substringmax-w-(e.g., a future utility likeclamp-max-w-smor a composed class) would inadvertently suppress thesm:max-w-2xldefault. An explicit opt-out prop (noDefaultMaxWidth?: booleanor similar) expresses intent unambiguously.♻️ Suggested refactor
export type DialogContentProps = Omit< React.ComponentProps<typeof DialogPrimitive.Content>, 'ref' > & { showCloseButton?: boolean; compact?: boolean; + noDefaultMaxWidth?: boolean; }; const DialogContent = React.forwardRef<HTMLDivElement, DialogContentProps>( - ({ className, children, showCloseButton = true, compact = false, ...props }, ref) => { - const hasCustomMaxWidth = typeof className === 'string' && className.includes('max-w-'); + ({ className, children, showCloseButton = true, compact = false, noDefaultMaxWidth = false, ...props }, ref) => { return ( ... - compact ? 'max-w-4xl p-4' : !hasCustomMaxWidth ? 'sm:max-w-2xl p-6' : 'p-6', + compact ? 'max-w-4xl p-4' : !noDefaultMaxWidth ? 'sm:max-w-2xl p-6' : 'p-6',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/dialog.tsx` around lines 88 - 91, The className substring check in DialogContent (hasCustomMaxWidth) is fragile; add an explicit boolean prop (e.g., noDefaultMaxWidth?: boolean) to DialogContentProps and use it instead of or in conjunction with hasCustomMaxWidth to decide whether to apply the default max-width; update the forwardRef signature and the destructuring ({ ..., noDefaultMaxWidth = false, ... }) in the DialogContent function, replace/augment the hasCustomMaxWidth usage with a condition that respects noDefaultMaxWidth, and update any consumers/tests to pass the new prop where they intentionally want to suppress the default max-width.apps/ui/src/renderer.tsx (1)
6-11: Redundantpwa-standaloneclass application —index.htmlalready covers this.
apps/ui/index.html(lines 96–101) appliespwa-standalonesynchronously in an inline<script>block that runs before any CSS or modules load — which is exactly the stated goal of "before CSS renders." By the time this module executes, the class is already ondocument.documentElement.
classList.addis idempotent so there's no behavioral issue, but if the intent is purely defensive (guard against the inline script failing), a brief comment saying so would make the duplication intentional rather than accidental.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/renderer.tsx` around lines 6 - 11, The duplicate application of the 'pwa-standalone' class via the isPwaStandalone check is redundant because index.html's inline script already sets it; update the block around isPwaStandalone and the document.documentElement.classList.add('pwa-standalone') call to either remove it or keep it with a clear comment stating it's a defensive fallback if the inline script fails, e.g., collapse the current if (isPwaStandalone) { document.documentElement.classList.add('pwa-standalone'); } into a single-line defensive comment or remove it entirely depending on desired behavior so reviewers know the duplication is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/execution-service.ts`:
- Around line 214-218: The internal-call branch unconditionally calls
updateFeatureStatusFn when options?._calledInternally, which can overwrite a
concurrent terminal status; modify the internal branch to mirror the external
guard by checking the loaded feature's status (the same condition used
externally: feature.status === 'backlog' || feature.status === 'ready') before
calling updateFeatureStatusFn (use the feature returned from loadFeatureFn), so
updateFeatureStatusFn is only invoked when the feature is in backlog or ready.
In `@apps/ui/index.html`:
- Around line 94-101: The PWA standalone detection using
window.matchMedia('(display-mode: standalone)') and navigator.standalone is
placed outside the existing try/catch and may throw uncaught exceptions; move
that block into the same try/catch inside the IIFE so any exceptions from
window.matchMedia or navigator.standalone are caught, and then call
document.documentElement.classList.add('pwa-standalone') within that guarded
block; ensure you don’t change the detection logic, only its location so it runs
under the existing error handling.
In `@apps/ui/src/components/views/agent-view.tsx`:
- Around line 133-138: handleCreateSessionFromEmptyState can be a no-op on
mobile because quickCreateSessionRef is cleared when SessionManager is
unmounted; change the handler to ensure the SessionManager is visible before
calling the ref: if quickCreateSessionRef.current is null, call the state setter
that shows SessionManager (showSessionManager -> setShowSessionManager(true)),
await a microtask/next render (e.g., await new Promise(r =>
requestAnimationFrame(r)) or next tick) so the component mounts and re-populates
quickCreateSessionRef, then invoke quickCreateSessionRef.current(); otherwise
call it immediately. Reference: handleCreateSessionFromEmptyState,
quickCreateSessionRef, SessionManager, and the showSessionManager state setter.
In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx`:
- Around line 164-173: The dialog reset effect currently clears branchName,
baseBranch, showBaseBranch, error, availableBranches, and branchFetchError but
not isLoadingBranches, so add setIsLoadingBranches(false) to that useEffect;
additionally, update the fetchBranches implementation to accept and use an
AbortController (or return a cancel token) so in-flight fetches can be aborted
on dialog close to prevent stale writes to availableBranches and
branchFetchError—ensure the cleanup/close path calls controller.abort() (or the
cancel function) and that fetchBranches checks for abort before setting state
(affecting isLoadingBranches, availableBranches, branchFetchError and the
BranchAutocomplete/Refresh button state).
In `@apps/ui/src/styles/global.css`:
- Around line 564-577: The .dialog-fullscreen-mobile rules are unintentionally
overriding the p-6 shorthand from DialogContent; update
.dialog-fullscreen-mobile in global.css to preserve the base p-6 padding by
using calc() so safe-area insets are added to the existing 1.5rem padding (p-6).
Concretely, change the padding-top and padding-bottom rules to compute
env(safe-area-inset-*, 0px) + 1.5rem to avoid collapsing p-6 on non-notched
devices, and in the `@media` (min-width: 640px) block restore the plain 1.5rem
top/bottom padding (or equivalent calc(0px + 1.5rem)) so desktop centered
dialogs keep the original p-6 spacing.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/create-pr.ts`:
- Around line 23-285: The createCreatePRHandler route contains substantial
business logic around parsing remotes and computing
targetRepo/pushOwner/pushRemote/upstreamRepo (see parsedRemotes map population
and the targetRemote handling block); extract this into a new service function
(e.g., services/prService.ts export an async function like
resolvePrTarget({worktreePath, pushRemote, targetRemote})) that accepts the repo
path and remote inputs and returns structured data ({repoUrl, targetRepo,
pushOwner, upstreamRepo, originOwner, parsedRemotes}) or throws validation
errors; update createCreatePRHandler to call resolvePrTarget and handle its
returned values or errors, and move all parsing, validation (including the
targetRemote existence check) and related logging from createCreatePRHandler
into that service.
In `@apps/server/src/services/merge-service.ts`:
- Around line 97-123: The const remote = rawRemote alias is unnecessary;
simplify by using a single validated variable: replace rawRemote + remote usage
with one const (e.g., const remote = options?.remote || 'origin'), keep the
isValidRemoteName(remote) check and the logger/execGitCommand(projectPath) calls
unchanged, and remove the redundant rawRemote declaration so all subsequent
references use remote directly (ensuring logger.warn and execGitCommand still
reference the same symbol).
In `@apps/server/src/services/rebase-service.ts`:
- Around line 72-98: The code creates a redundant alias by assigning const
remote = rawRemote after validating rawRemote; remove the no-op alias and use
rawRemote directly (or rename rawRemote to remote) to collapse the variables in
rebase-service.ts—update references to use the single name (e.g., use rawRemote
in the isValidRemoteName check and in the execGitCommand(['fetch', remote],
worktreePath) call or rename rawRemote to remote) so only one variable is used
around isValidRemoteName, rawRemote/remote, and the fetch error handling
(fetchError/getErrorMessage).
In `@apps/ui/public/sw.js`:
- Around line 320-371: The first-visit branch in the navigation handler uses
cache.put(event.request, response.clone()) as fire-and-forget inside the try
block of the event.respondWith handler; wrap that cache update in
event.waitUntil (or otherwise ensure the promise is observed) so the Service
Worker isn't terminated before the shell is cached—mirror the pattern used by
the updateCache() helper (and the cached-path where you call
event.waitUntil(updateCache())), targeting the cache.put call in the anonymous
async passed to event.respondWith.
- Around line 253-264: When the 2s timeout wins and cachedResponse is returned,
the ongoing fetchPromise (from startNetworkFetch) must be kept alive via the
service worker event's lifetime so the cache update can complete; update the
branch that handles cachedResponse to call
event.waitUntil(fetchPromise.then(...).catch(...)) (or .finally(...)) so the
background fetch continues to completion and updates the cache even after
returning cachedResponse, referencing fetchPromise and startNetworkFetch to
locate the code and using event.waitUntil to extend the worker lifetime.
In `@apps/ui/src/components/ui/dialog.tsx`:
- Around line 88-91: The className substring check in DialogContent
(hasCustomMaxWidth) is fragile; add an explicit boolean prop (e.g.,
noDefaultMaxWidth?: boolean) to DialogContentProps and use it instead of or in
conjunction with hasCustomMaxWidth to decide whether to apply the default
max-width; update the forwardRef signature and the destructuring ({ ...,
noDefaultMaxWidth = false, ... }) in the DialogContent function, replace/augment
the hasCustomMaxWidth usage with a condition that respects noDefaultMaxWidth,
and update any consumers/tests to pass the new prop where they intentionally
want to suppress the default max-width.
In `@apps/ui/src/components/ui/git-diff-panel.tsx`:
- Around line 927-976: The onClick handlers create floating Promises: change
onClick={() => handleUnstageFile(file.path)} and onClick={() =>
handleStageFile(file.path)} to the established pattern (e.g. onClick={() => void
handleUnstageFile(file.path)}) so the async calls are explicitly ignored and
satisfy the lint rule; then remove the duplicated staging controls by extracting
and reusing a shared component (e.g. FileRow or FileStagingControls) used by
both this fallback block and FileDiffSection so the staging UI (stagingState,
stagingInProgress, handleStageFile, handleUnstageFile, enableStaging) is
implemented once and composed in both places.
In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx`:
- Around line 195-198: isRemoteBaseBranch currently only checks
availableBranches and thus returns false when the user types a remote branch
like "origin/feature"; update the isRemoteBaseBranch computation (used by
baseBranch, availableBranches, allowCreate, branchFetchError) to also detect
manually entered remote-style names by checking the baseBranch string pattern
(e.g., contains '/' or matches /^remoteName\/.+$/ or startsWith('origin/')) and
treat those as remote when availableBranches doesn't contain a match so the UI
shows the "Remote branch — will fetch latest" hint and switches the button label
to "Fetching & Creating…".
In `@apps/ui/src/renderer.tsx`:
- Around line 6-11: The duplicate application of the 'pwa-standalone' class via
the isPwaStandalone check is redundant because index.html's inline script
already sets it; update the block around isPwaStandalone and the
document.documentElement.classList.add('pwa-standalone') call to either remove
it or keep it with a clear comment stating it's a defensive fallback if the
inline script fails, e.g., collapse the current if (isPwaStandalone) {
document.documentElement.classList.add('pwa-standalone'); } into a single-line
defensive comment or remove it entirely depending on desired behavior so
reviewers know the duplication is intentional.
In `@libs/utils/src/git-validation.ts`:
- Around line 66-73: The initial guard in isValidRemoteName redundantly checks
name.length === 0 after !name; remove the dead branch so the first line becomes
a single falsy/length and max-length check (e.g. if (!name || name.length >=
MAX_BRANCH_NAME_LENGTH) return false;), leaving the other guards (startsWith
'-', startsWith '.', '..', '/', '\0' and the final regex /^[a-zA-Z0-9._-]+$/)
and the MAX_BRANCH_NAME_LENGTH reference unchanged to preserve behavior.
apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/ui/dialog.tsx (1)
115-126:⚠️ Potential issue | 🟡 Minor
compactmode loses the viewport-margin constraint via tailwind-merge conflictLine 115 adds
max-w-[calc(100%-2rem)]to enforce a 1rem gutter on each side of the dialog. However, whencompact=true, line 126 injectsmax-w-4xl(no breakpoint prefix) into the samecn()call.Since
cn()usestwMerge(clsx(inputs)), it resolves conflicts between classes in the same utility group. Bothmax-w-[calc(100%-2rem)]andmax-w-4xlare unqualifiedmax-widthutilities; tailwind-merge keeps only the last one, dropping the earliermax-w-[calc(100%-2rem)]. This means compact dialogs will render at max-w-4xl (448px) without the 1rem viewport margin — there's no edge-to-edge protection on narrow screens.This doesn't affect the non-compact path because
sm:max-w-2xl(breakpoint-prefixed) andmax-w-[calc(100%-2rem)](unprefixed) are independent modifiers and both are preserved.🐛 Proposed fix — encode both constraints into a single `max-w-` token
Replace the two-part approach with a single class that uses CSS
min()to enforce both constraints at once:- 'flex flex-col w-full max-w-[calc(100%-2rem)]', + 'flex flex-col w-full',- compact ? 'max-w-4xl p-4' : !hasCustomMaxWidth ? 'sm:max-w-2xl p-6' : 'p-6', + compact + ? 'max-w-[min(56rem,calc(100%-2rem))] p-4' + : !hasCustomMaxWidth + ? 'max-w-[min(42rem,calc(100%-2rem))] sm:max-w-2xl p-6' + : 'p-6',Alternatively, keep
max-w-[calc(100%-2rem)]and add a wrapping check so compact mode also references a capped arbitrary value rather than a named token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/dialog.tsx` around lines 115 - 126, The compact branch in the cn() class list (the ternary using compact and hasCustomMaxWidth) overrides the earlier unprefixed max-w-[calc(100%-2rem)] because tailwind-merge drops earlier max-w tokens; update the compact branch in apps/ui/src/components/ui/dialog.tsx so it emits a single max-w token that encodes both the 1rem gutter and the intended cap (e.g., use CSS min() or an equivalent arbitrary value combining calc(100%-2rem) and the 4xl cap) instead of plain "max-w-4xl", keeping the other padding classes the same; this ensures compact dialogs keep the viewport-margin constraint while still being capped.
🧹 Nitpick comments (13)
apps/ui/src/components/views/agent-view.tsx (1)
133-149: SinglerequestAnimationFramemay not guarantee theSessionManagerhas committed in React 19 concurrent modeThe past review's fix (showing the
SessionManagerand waiting a frame) is a real improvement. However, because the await occurs inside anasyncfunction — not a synchronous browser event handler — React 19's concurrent scheduler is not required to have committed thesetStatebefore the first RAF fires. If the commit races past the RAF,quickCreateSessionRef.currentis stillnullafter the wait and the call is silently dropped (same failure mode as before the fix, just now with the SessionManager visible).Two nested
requestAnimationFramecalls, or asetTimeout(0), give the scheduler a full JS turn to commit:🛡️ Proposed fix — more reliable mount wait
- await new Promise<void>((r) => requestAnimationFrame(() => r())); + await new Promise<void>((r) => requestAnimationFrame(() => requestAnimationFrame(() => r())));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/agent-view.tsx` around lines 133 - 149, The current handleCreateSessionFromEmptyState uses a single requestAnimationFrame after setShowSessionManager but in React 19 concurrent mode that may not guarantee SessionManager has committed; update the wait logic (inside handleCreateSessionFromEmptyState) to wait one more tick (e.g., use two nested requestAnimationFrame calls or a setTimeout(() => ..., 0)) before re-checking quickCreateSessionRef.current so the component has time to mount and populate the ref, then call createFn if present.apps/ui/public/sw.js (1)
444-461: Precache fetches may be terminated early — noevent.waitUntil.The
PRECACHE_ASSETShandler fires independentfetchcalls without wrapping them inevent.waitUntil(). The browser may terminate the SW before all fetches complete, especially on mobile where SW lifetime is aggressively managed. Since precaching is best-effort this is acceptable, but wrapping inwaitUntilwould improve reliability.Optional: keep SW alive during precache
if (event.data?.type === 'PRECACHE_ASSETS' && Array.isArray(event.data.urls)) { - caches.open(IMMUTABLE_CACHE).then((cache) => { - event.data.urls.forEach((url) => { - cache.match(url).then((existing) => { - if (!existing) { - fetch(url, { priority: 'low' }) - .then((response) => { - if (response.ok) { - cache.put(url, response); - } - }) - .catch(() => { - // Silently ignore precache failures - }); - } - }); - }); - }); + event.waitUntil( + caches.open(IMMUTABLE_CACHE).then((cache) => { + return Promise.all( + event.data.urls.map((url) => + cache.match(url).then((existing) => { + if (existing) return; + return fetch(url, { priority: 'low' }) + .then((response) => { + if (response.ok) { + return cache.put(url, response); + } + }) + .catch(() => { + // Silently ignore precache failures + }); + }) + ) + ); + }) + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/public/sw.js` around lines 444 - 461, The PRECACHE_ASSETS handler starts independent fetches that can be cut off because they aren't awaited; update the block handling event.data?.type === 'PRECACHE_ASSETS' (and IMMUATABLE_CACHE / the cache.open logic) to build a single Promise that waits for all cache match/fetch/put operations (e.g., using Promise.all over event.data.urls.map(...)) and pass that Promise into event.waitUntil(...) so the service worker stays alive until precache work completes.apps/server/src/routes/worktree/routes/create-pr.ts (2)
236-243:gh pr listcommand built via string interpolation — inconsistent withgh pr create.The
gh pr listcommand (line 238) is assembled as a shell string withexecAsync, whilegh pr create(line 296) usesspawnProcesswith an args array.repoArgandheadRefcontain values derived from remote-parsed output (owner/repo), which could theoretically contain shell metacharacters. UsingspawnProcesswith an args array would eliminate this risk and be consistent with thegh pr createpattern below.Suggested refactor
- const listCmd = `gh pr list${repoArg} --head "${headRef}" --json number,title,url,state --limit 1`; - logger.debug(`Running: ${listCmd}`); - const { stdout: existingPrOutput } = await execAsync(listCmd, { - cwd: worktreePath, - env: execEnv, - }); + const listArgs = ['pr', 'list', '--head', headRef, '--json', 'number,title,url,state', '--limit', '1']; + if (upstreamRepo) { + listArgs.push('--repo', upstreamRepo); + } + logger.debug(`Checking for existing PR: gh ${listArgs.join(' ')}`); + const listResult = await spawnProcess({ + command: 'gh', + args: listArgs, + cwd: worktreePath, + env: execEnv, + }); + const existingPrOutput = listResult.stdout;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/create-pr.ts` around lines 236 - 243, The gh pr list invocation builds a shell string via listCmd and execAsync (using repoArg and headRef), which is inconsistent with the gh pr create usage and risks shell injection; replace the execAsync call with the spawnProcess pattern used for gh pr create: call spawnProcess("gh", ["pr","list", repoArg, "--head", headRef, "--json","number,title,url,state","--limit","1"], { cwd: worktreePath, env: execEnv }) (or equivalent) and consume its stdout to populate existingPrOutput, ensuring repoArg and headRef are passed as separate args rather than interpolated into a shell string.
146-153: Shell string interpolation ingit push— prefer arg-array form.Lines 146 and 153 pass user-influenced values (
pushRemote,branchName) via string interpolation intoexecAsync(shell execution). While both are validated, the same file uses the saferexecGitCommand(array args, no shell) forgit commitat line 103 andspawnProcessforgh pr createat line 296. Switching toexecGitCommandhere would eliminate the shell-parsing layer entirely and make the security posture consistent.Suggested refactor
- await execAsync(`git push ${pushRemote} ${branchName}`, { - cwd: worktreePath, - env: execEnv, - }); + await execGitCommand(['push', pushRemote, branchName], worktreePath); } catch { // If push fails, try with --set-upstream try { - await execAsync(`git push --set-upstream ${pushRemote} ${branchName}`, { - cwd: worktreePath, - env: execEnv, - }); + await execGitCommand(['push', '--set-upstream', pushRemote, branchName], worktreePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/create-pr.ts` around lines 146 - 153, The git push calls use shell-interpolated strings with user-influenced values (pushRemote, branchName) passed to execAsync; replace both attempts with the existing safe helper execGitCommand (which accepts an args array) to avoid shell parsing. Concretely, in the try/catch around the first git push and the inner retry that adds --set-upstream, call execGitCommand with cwd set to worktreePath, env execEnv, and args like ["push", pushRemote, branchName] (and ["push", "--set-upstream", pushRemote, branchName] for the retry) so you remove string interpolation while preserving the same behavior and error handling. Ensure you keep the surrounding try/catch logic and any logging unchanged.apps/server/src/services/pr-service.ts (2)
60-63:git remote -vuses shell string viaexecAsyncinstead ofexecGitCommand.The service uses
execAsync('git remote -v', ...)(shell-based) instead ofexecGitCommand(['remote', '-v'], ...)(array-based, no shell). While the command here has no user-supplied arguments so injection isn't a risk, usingexecGitCommandwould be consistent with the rest of the codebase and the coding guidelines around using@automaker/git-utils.Similarly applies to line 150:
execAsync('git config --get remote.origin.url', ...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/pr-service.ts` around lines 60 - 63, Replace the shell-based execAsync invocations with the array-based helper execGitCommand to match project conventions: swap execAsync('git remote -v', {cwd: worktreePath, env: execEnv}) (which populates remotes) for execGitCommand(['remote','-v'], {cwd: worktreePath, env: execEnv}) and likewise replace execAsync('git config --get remote.origin.url', {cwd: worktreePath, env: execEnv}) with execGitCommand(['config','--get','remote.origin.url'], {cwd: worktreePath, env: execEnv}); keep using the same variables (remotes, execEnv, worktreePath) and ensure the returned stdout handling remains unchanged.
47-48: RedundantpushRemote !== undefinedcheck.
pushRemoteis typed as a requiredstringparameter (line 43), so it's always defined. The!== undefinedguard is a no-op.Suggested simplification
- if (pushRemote !== undefined && !isValidRemoteName(pushRemote)) { + if (!isValidRemoteName(pushRemote)) { throw new Error(`Invalid push remote name: "${pushRemote}"`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/pr-service.ts` around lines 47 - 48, The conditional guarding `isValidRemoteName(pushRemote)` is using a redundant `pushRemote !== undefined` check because `pushRemote` is a required string parameter; remove the `!== undefined` part and simplify the condition to directly validate the parameter (i.e., replace the `if (pushRemote !== undefined && !isValidRemoteName(pushRemote))` check with a direct `if (!isValidRemoteName(pushRemote))` branch), keeping the existing error throw for `"Invalid push remote name: \"${pushRemote}\""`.apps/ui/src/components/ui/dialog.tsx (1)
102-104:hasCustomMaxWidthname is misleading whennoDefaultMaxWidth=trueWhen
noDefaultMaxWidthistrue, the variable is set totrueregardless of whetherclassNameactually contains amax-w-class. The name implies "a custom max-width exists in className," but thenoDefaultMaxWidthbranch means "suppress the default even without a custom one." Consider renaming toskipDefaultMaxWidthto match the actual semantics.♻️ Suggested rename
- // Check if className contains a custom max-width (fallback heuristic) - const hasCustomMaxWidth = - noDefaultMaxWidth || (typeof className === 'string' && className.includes('max-w-')); + // Skip default max-width when explicitly requested or when caller already provides one + const skipDefaultMaxWidth = + noDefaultMaxWidth || (typeof className === 'string' && className.includes('max-w-'));- compact ? 'max-w-4xl p-4' : !hasCustomMaxWidth ? 'sm:max-w-2xl p-6' : 'p-6', + compact ? 'max-w-4xl p-4' : !skipDefaultMaxWidth ? 'sm:max-w-2xl p-6' : 'p-6',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/dialog.tsx` around lines 102 - 104, The variable hasCustomMaxWidth is misnamed because it becomes true when noDefaultMaxWidth is true even if className lacks a max-w- token; rename hasCustomMaxWidth to skipDefaultMaxWidth (or similar) and update all references (including the declaration and any conditional logic that uses hasCustomMaxWidth) so the name reflects that the default max-width should be suppressed when noDefaultMaxWidth is true, while retaining the original fallback heuristic that also sets it when className contains 'max-w-'.apps/ui/src/components/views/terminal-view/terminal-scripts-dropdown.tsx (2)
52-52:as unknown as Array<...>double-cast is a code smell fromas const.Using
as constonDEFAULT_SCRIPTSwidens the type to deeply readonly literals, forcing the cast when the consumer expects a mutable type. This will be eliminated if the shared constant (see refactor suggestion onterminal-scripts-section.tsx) uses an explicit type annotation instead ofas const.♻️ Alternative — explicit type annotation avoids the cast entirely
In the shared constants file:
export const DEFAULT_TERMINAL_SCRIPTS: Array<{ id: string; name: string; command: string }> = [ ... ];No
as const, no cast needed at the call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/terminal-scripts-dropdown.tsx` at line 52, The double-cast on DEFAULT_SCRIPTS in terminal-scripts-dropdown.tsx is compensating for an upstream `as const` on the shared constant; update the shared constant (referenced in terminal-scripts-section.tsx) to declare an explicit type like Array<{id: string; name: string; command: string}> (e.g. export const DEFAULT_TERMINAL_SCRIPTS: Array<...> = [...]) instead of using `as const`, then remove the `as unknown as Array<...>` cast and return the constant directly (use the symbol DEFAULT_SCRIPTS / DEFAULT_TERMINAL_SCRIPTS to locate and adjust the declaration and consumer).
94-94:cn()is unnecessary here — only a single static class string is passed.♻️ Proposed change
-<Play className={cn('h-3.5 w-3.5 shrink-0 text-brand-500')} /> +<Play className="h-3.5 w-3.5 shrink-0 text-brand-500" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/terminal-scripts-dropdown.tsx` at line 94, The JSX uses the cn utility unnecessarily for a single static class string; in the Play component invocation inside terminal-scripts-dropdown.tsx replace the call to cn('h-3.5 w-3.5 shrink-0 text-brand-500') with the plain className string 'h-3.5 w-3.5 shrink-0 text-brand-500' (i.e., remove cn) to simplify the code and avoid an extra function call.apps/ui/src/components/views/project-settings-view/terminal-scripts-section.tsx (1)
76-76: PreferstructuredClone()overJSON.parse(JSON.stringify())for deep cloning.
structuredCloneis available in all modern browsers and Electron (Chromium 98+), is faster than a JSON round-trip, and handles more data types correctly.♻️ Proposed change
-setOriginalScripts(JSON.parse(JSON.stringify(scriptList))); +setOriginalScripts(structuredClone(scriptList));Apply the same change at lines 99 and 107.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/terminal-scripts-section.tsx` at line 76, Replace the JSON round-trip deep clones with structuredClone: where the code calls setOriginalScripts(JSON.parse(JSON.stringify(scriptList))) and the similar two occurrences later in the file, use setOriginalScripts(structuredClone(scriptList)) (and the corresponding setX(structuredClone(scriptList))) instead so cloning uses structuredClone(scriptList) for correctness and performance.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (2)
334-352: Consider adding a separator after the Auto Mode toggle block.The Auto Mode item immediately precedes the Dev Server section with no visual divider. Since the Dev Server section appends its own separator at the bottom, Auto Mode and Dev Server render as a single visual group. Adding a
<DropdownMenuSeparator />after the closing</>of the Auto Mode block would give it its own grouping.✏️ Proposed fix
)} + {onToggleAutoMode && <DropdownMenuSeparator />} {isDevServerRunning ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx` around lines 334 - 352, Add a visual separator after the Auto Mode toggle group so it’s visually distinct from the Dev Server section: inside worktree-actions-dropdown.tsx, after the conditional block that renders the Auto Mode items (the block that checks onToggleAutoMode and uses isAutoModeRunning and calls onToggleAutoMode(worktree)), insert a <DropdownMenuSeparator /> immediately following the closing fragment so the Auto Mode items are separated from subsequent menu items.
582-649: Pull and Push inner item markup is copy-pasted across split/simple branches.Both the Pull (Lines 584-602 vs 634-649) and Push (Lines 658-701 vs 733-771) sections duplicate their entire
DropdownMenuItembody across the split-button and simple-item branches. Badge rendering (behindCount,aheadCount,CloudOff,trackingRemote) and the disabled/opacity logic are maintained in two places each.Extracting small helper components (e.g.,
PullMenuItemContent/PushMenuItemContent) and wrapping them conditionally inDropdownMenuSubwould eliminate the duplication and make future badge/logic changes a single-site edit.Also applies to: 656-771
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx` around lines 582 - 649, Extract the duplicated inner markup for Pull and Push into small pure components (e.g., PullMenuItemContent and PushMenuItemContent) that accept props: worktree, isGitOpsAvailable, isPulling/isPushing, behindCount/aheadCount, trackingRemote, and icons like CloudOff; render badges and disabled/opacity logic inside those components and return only the common DropdownMenuItem children (icon + label + badges/alerts). Then in the current branches replace the duplicated bodies under DropdownMenuSub and the simple DropdownMenuItem with a call to the new component (for split remotes wrap it in DropdownMenuSubContent and map remotes to call onPullWithRemote/onPushWithRemote, for single/no-remote use onPull/onPush), keeping the existing event handlers (onPull, onPullWithRemote, onPush, onPushWithRemote), props (worktree, remotes) and className/disabled wiring intact so behavior is unchanged.apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx (1)
226-238: Heuristic can produce a misleading "Remote branch — will fetch latest" hint in the branch-fetch-error path.When
branchFetchErroris set,availableBranchesis the[{ name: 'main', isRemote: false }]fallback andallowCreatebecomestrue. A user who types a new local branch likefeature/my-branchwill trigger thebaseBranch.includes('/')path, find it is not a known local branch (it isn't in the fallback list), andisRemoteBaseBranchreturnstrue— showing the "Remote branch — will fetch latest" hint even though the user intends a new local branch.The comment documents this as an intentional heuristic, and the worktree creation itself is unaffected. A simple mitigation is to suppress the hint when the branch list couldn't be loaded (i.e., when
branchFetchErroris set):- const isRemoteBaseBranch = useMemo(() => { + const isRemoteBaseBranch = useMemo(() => { if (!baseBranch) return false; + // Suppress heuristic when branch list is unavailable; we can't distinguish remote from local + if (branchFetchError && availableBranches.length === 1) return false; ... - }, [baseBranch, availableBranches]); + }, [baseBranch, availableBranches, branchFetchError]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx` around lines 226 - 238, The heuristic in the isRemoteBaseBranch useMemo can misclassify a user-entered local branch as remote when availableBranches is the fallback due to a failed fetch; update the useMemo (isRemoteBaseBranch) to first check branchFetchError and return false if it's set so the "Remote branch — will fetch latest" hint is suppressed on fetch failures; keep the rest of the logic (checks against availableBranches, the '/' heuristic) unchanged and ensure the dependency array still includes baseBranch, availableBranches, and branchFetchError so the memo updates correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/pr-service.ts`:
- Around line 96-106: The catch block swallowing failures from execAsync('git
remote -v', ...) leads to parsedRemotes remaining empty and an explicit
targetRemote being silently ignored; update the logic so that if targetRemote is
provided but parsedRemotes.size === 0 you throw a clear error (e.g., referencing
parsedRemotes and targetRemote) instead of proceeding to silent fallback—apply
this check after the catch and before any auto-detection/override logic so
callers are notified when remote parsing failed and their explicit target cannot
be validated.
- Line 8: pr-service.ts currently imports route-level helpers execAsync,
execEnv, and isValidRemoteName from routes/worktree/common.js which inverts
dependency direction; replace the import of isValidRemoteName to import directly
from `@automaker/utils` (use the named export isValidRemoteName) and remove the
import of execAsync and execEnv from that route module, then wire
execAsync/execEnv to a shared utility once created (add a TODO referencing
lib/exec.ts or `@automaker/utils` and temporarily implement a thin wrapper or mock
within the service boundary if necessary) so that services no longer depend on
route internals.
In `@apps/ui/public/sw.js`:
- Around line 160-164: Update the misleading comment above self.clients.claim():
explain that self.clients.claim() is called unconditionally on activation, while
the activate event itself only fires at safe times (first install, after
skipWaiting() is called, or after all old clients are closed), and mention
skipWaiting() and the activate event to clarify why unconditional claim() is
safe; keep the comment concise and reference self.clients.claim() and
skipWaiting().
- Around line 123-141: The conditional guard using the skipWaitingRequested flag
inside the install event handler is dead code because skipWaitingRequested is
only set by the SKIP_WAITING message handler after install has completed; remove
the unreachable if (skipWaitingRequested) { self.skipWaiting(); } from the
install listener (or alternatively simplify the install handler comments) and
ensure the real activation path is documented to point to the SKIP_WAITING
message handler where self.skipWaiting() is actually invoked (reference
skipWaitingRequested, the install event listener, the SKIP_WAITING message
handler, and the self.skipWaiting() call).
In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx`:
- Around line 267-268: The code currently forwards effectiveBaseBranch (derived
from baseBranch) to api.worktree.create without the same validation applied to
branchName; update the dialog to validate/sanitize baseBranch using the same
/^[a-zA-Z0-9._/-]+$/ check used for branchName (or reuse the same validation
helper if one exists) before assigning effectiveBaseBranch, and only pass the
validated value (or undefined when empty) to api.worktree.create to prevent
shell-special characters or invalid names from reaching the API.
- Line 129: The call to api.worktree.listBranches(projectPath, true) doesn't
accept an AbortSignal so the underlying HTTP request keeps running after
controller.abort(); update the API and HTTP client to accept and forward an
optional AbortSignal: add an optional signal parameter to worktree.listBranches
and propagate it into the http client's post(...) call, then modify post in
http-api-client.ts to accept a signal and pass it into fetch({ signal }) so
controller.abort() cancels the in-flight request; ensure create-worktree-dialog
uses controller.signal when calling listBranches and keep the existing
post-await abort check.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 892-912: The dropdown renders a visible, clickable "Stashes" item
that is a silent no-op when worktree.hasChanges is false and onViewStashes is
undefined (even if onStashChanges exists); update the rendering guard so the
simple DropdownMenuItem only renders when at least one action is meaningful:
i.e. render the block only if (worktree.hasChanges && onStashChanges) ||
onViewStashes, keep disabled tied to isGitOpsAvailable, and preserve the
existing labels/icons (references: DropdownMenuItem, worktree.hasChanges,
onStashChanges, onViewStashes, isGitOpsAvailable).
In
`@apps/ui/src/components/views/project-settings-view/terminal-scripts-section.tsx`:
- Around line 152-172: handleDragEnd currently always applies the reorder if
draggedIndex and dragOverIndex are set; change handleDragEnd to accept the drag
event (e: React.DragEvent) and only perform the splice-reorder when
e.dataTransfer.dropEffect !== 'none' (meaning the drop target accepted the
drop). Keep the existing checks for draggedIndex/dragOverIndex before touching
setScripts, and always reset setDraggedIndex(null) and setDragOverIndex(null) at
the end. Update any onDragEnd usages to pass the event into handleDragEnd;
reference functions/vars: handleDragEnd, handleDragOver, draggedIndex,
dragOverIndex, setScripts, setDraggedIndex, setDragOverIndex.
- Around line 12-17: DEFAULT_SCRIPTS is duplicated between
terminal-scripts-section.tsx and terminal-scripts-dropdown.tsx; extract the
shared array into a single exported constant and import it where needed. Create
a new module (e.g., terminal-scripts-constants.ts) that exports
DEFAULT_TERMINAL_SCRIPTS (same shape: id, name, command) and update
terminal-scripts-section.tsx to replace DEFAULT_SCRIPTS with that import; also
update terminal-scripts-dropdown.tsx to import the same constant (removing any
casts like as unknown as Array<...>). Ensure both files reference the exported
symbol name (DEFAULT_TERMINAL_SCRIPTS) consistently.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 381-385: sendCommand always appends '\n' which breaks Windows
shells; make isWindowsShell a component-level ref so sendCommand can use it.
Create a useRef<boolean> isWindowsShellRef, set isWindowsShellRef.current inside
the existing ws.onmessage detection logic (where isWindowsShell is computed),
and update sendCommand to read isWindowsShellRef.current and append '\r\n' when
true or '\n' otherwise (use wsRef.current.send as before). Ensure you reference
sendCommand, runCommandOnConnect (for consistency), ws.onmessage, wsRef and
isWindowsShellRef in the change.
In `@apps/ui/src/styles/global.css`:
- Around line 419-424: The CSS rule setting padding-bottom: 0 on
:root.pwa-standalone `#app` removes safe-area inset for iOS PWAs and can cause
content to be obscured; revert to using the safe-area inset (e.g., leave
calc(env(safe-area-inset-bottom, 0px) * 0.5)) or apply the zero padding only
when running in a desktop/Electron context by gating the rule with an
Electron-specific root class or via a media query for non-touch/hover-capable
devices; use the existing isPwaStandalone/mobile-detect flag (from
mobile-detect.ts) or an Electron runtime marker to ensure the zero-padding rule
does not apply to iOS standalone PWAs.
---
Outside diff comments:
In `@apps/ui/src/components/ui/dialog.tsx`:
- Around line 115-126: The compact branch in the cn() class list (the ternary
using compact and hasCustomMaxWidth) overrides the earlier unprefixed
max-w-[calc(100%-2rem)] because tailwind-merge drops earlier max-w tokens;
update the compact branch in apps/ui/src/components/ui/dialog.tsx so it emits a
single max-w token that encodes both the 1rem gutter and the intended cap (e.g.,
use CSS min() or an equivalent arbitrary value combining calc(100%-2rem) and the
4xl cap) instead of plain "max-w-4xl", keeping the other padding classes the
same; this ensures compact dialogs keep the viewport-margin constraint while
still being capped.
---
Duplicate comments:
In `@apps/ui/src/components/views/terminal-view/terminal-scripts-dropdown.tsx`:
- Around line 17-22: DEFAULT_SCRIPTS is duplicated; extract the constant into a
single shared module (e.g., export DEFAULT_SCRIPTS from a new/shared file) and
replace the local definitions with imports. Update the files that currently
declare DEFAULT_SCRIPTS (referenced here by the symbol DEFAULT_SCRIPTS in
terminal-scripts-dropdown and the duplicate in terminal-scripts-section) to
import the exported constant instead of defining it inline, and ensure the
export uses the same readonly/const typing so callers continue to work without
type changes.
In `@apps/ui/src/styles/global.css`:
- Around line 564-580: No code change needed for the padding bug — keep the
.dialog-fullscreen-mobile selector using padding-top/bottom:
calc(env(safe-area-inset-*, 0px) + 1.5rem) and the `@media` (min-width: 640px)
block restoring padding-top/bottom: 1.5rem; ensure both rules remain inside the
same `@layer` utilities scope so cascade/order is preserved and remove the
duplicate review comment markers.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/create-pr.ts`:
- Around line 236-243: The gh pr list invocation builds a shell string via
listCmd and execAsync (using repoArg and headRef), which is inconsistent with
the gh pr create usage and risks shell injection; replace the execAsync call
with the spawnProcess pattern used for gh pr create: call spawnProcess("gh",
["pr","list", repoArg, "--head", headRef,
"--json","number,title,url,state","--limit","1"], { cwd: worktreePath, env:
execEnv }) (or equivalent) and consume its stdout to populate existingPrOutput,
ensuring repoArg and headRef are passed as separate args rather than
interpolated into a shell string.
- Around line 146-153: The git push calls use shell-interpolated strings with
user-influenced values (pushRemote, branchName) passed to execAsync; replace
both attempts with the existing safe helper execGitCommand (which accepts an
args array) to avoid shell parsing. Concretely, in the try/catch around the
first git push and the inner retry that adds --set-upstream, call execGitCommand
with cwd set to worktreePath, env execEnv, and args like ["push", pushRemote,
branchName] (and ["push", "--set-upstream", pushRemote, branchName] for the
retry) so you remove string interpolation while preserving the same behavior and
error handling. Ensure you keep the surrounding try/catch logic and any logging
unchanged.
In `@apps/server/src/services/pr-service.ts`:
- Around line 60-63: Replace the shell-based execAsync invocations with the
array-based helper execGitCommand to match project conventions: swap
execAsync('git remote -v', {cwd: worktreePath, env: execEnv}) (which populates
remotes) for execGitCommand(['remote','-v'], {cwd: worktreePath, env: execEnv})
and likewise replace execAsync('git config --get remote.origin.url', {cwd:
worktreePath, env: execEnv}) with
execGitCommand(['config','--get','remote.origin.url'], {cwd: worktreePath, env:
execEnv}); keep using the same variables (remotes, execEnv, worktreePath) and
ensure the returned stdout handling remains unchanged.
- Around line 47-48: The conditional guarding `isValidRemoteName(pushRemote)` is
using a redundant `pushRemote !== undefined` check because `pushRemote` is a
required string parameter; remove the `!== undefined` part and simplify the
condition to directly validate the parameter (i.e., replace the `if (pushRemote
!== undefined && !isValidRemoteName(pushRemote))` check with a direct `if
(!isValidRemoteName(pushRemote))` branch), keeping the existing error throw for
`"Invalid push remote name: \"${pushRemote}\""`.
In `@apps/ui/public/sw.js`:
- Around line 444-461: The PRECACHE_ASSETS handler starts independent fetches
that can be cut off because they aren't awaited; update the block handling
event.data?.type === 'PRECACHE_ASSETS' (and IMMUATABLE_CACHE / the cache.open
logic) to build a single Promise that waits for all cache match/fetch/put
operations (e.g., using Promise.all over event.data.urls.map(...)) and pass that
Promise into event.waitUntil(...) so the service worker stays alive until
precache work completes.
In `@apps/ui/src/components/ui/dialog.tsx`:
- Around line 102-104: The variable hasCustomMaxWidth is misnamed because it
becomes true when noDefaultMaxWidth is true even if className lacks a max-w-
token; rename hasCustomMaxWidth to skipDefaultMaxWidth (or similar) and update
all references (including the declaration and any conditional logic that uses
hasCustomMaxWidth) so the name reflects that the default max-width should be
suppressed when noDefaultMaxWidth is true, while retaining the original fallback
heuristic that also sets it when className contains 'max-w-'.
In `@apps/ui/src/components/views/agent-view.tsx`:
- Around line 133-149: The current handleCreateSessionFromEmptyState uses a
single requestAnimationFrame after setShowSessionManager but in React 19
concurrent mode that may not guarantee SessionManager has committed; update the
wait logic (inside handleCreateSessionFromEmptyState) to wait one more tick
(e.g., use two nested requestAnimationFrame calls or a setTimeout(() => ..., 0))
before re-checking quickCreateSessionRef.current so the component has time to
mount and populate the ref, then call createFn if present.
In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx`:
- Around line 226-238: The heuristic in the isRemoteBaseBranch useMemo can
misclassify a user-entered local branch as remote when availableBranches is the
fallback due to a failed fetch; update the useMemo (isRemoteBaseBranch) to first
check branchFetchError and return false if it's set so the "Remote branch — will
fetch latest" hint is suppressed on fetch failures; keep the rest of the logic
(checks against availableBranches, the '/' heuristic) unchanged and ensure the
dependency array still includes baseBranch, availableBranches, and
branchFetchError so the memo updates correctly.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 334-352: Add a visual separator after the Auto Mode toggle group
so it’s visually distinct from the Dev Server section: inside
worktree-actions-dropdown.tsx, after the conditional block that renders the Auto
Mode items (the block that checks onToggleAutoMode and uses isAutoModeRunning
and calls onToggleAutoMode(worktree)), insert a <DropdownMenuSeparator />
immediately following the closing fragment so the Auto Mode items are separated
from subsequent menu items.
- Around line 582-649: Extract the duplicated inner markup for Pull and Push
into small pure components (e.g., PullMenuItemContent and PushMenuItemContent)
that accept props: worktree, isGitOpsAvailable, isPulling/isPushing,
behindCount/aheadCount, trackingRemote, and icons like CloudOff; render badges
and disabled/opacity logic inside those components and return only the common
DropdownMenuItem children (icon + label + badges/alerts). Then in the current
branches replace the duplicated bodies under DropdownMenuSub and the simple
DropdownMenuItem with a call to the new component (for split remotes wrap it in
DropdownMenuSubContent and map remotes to call
onPullWithRemote/onPushWithRemote, for single/no-remote use onPull/onPush),
keeping the existing event handlers (onPull, onPullWithRemote, onPush,
onPushWithRemote), props (worktree, remotes) and className/disabled wiring
intact so behavior is unchanged.
In
`@apps/ui/src/components/views/project-settings-view/terminal-scripts-section.tsx`:
- Line 76: Replace the JSON round-trip deep clones with structuredClone: where
the code calls setOriginalScripts(JSON.parse(JSON.stringify(scriptList))) and
the similar two occurrences later in the file, use
setOriginalScripts(structuredClone(scriptList)) (and the corresponding
setX(structuredClone(scriptList))) instead so cloning uses
structuredClone(scriptList) for correctness and performance.
In `@apps/ui/src/components/views/terminal-view/terminal-scripts-dropdown.tsx`:
- Line 52: The double-cast on DEFAULT_SCRIPTS in terminal-scripts-dropdown.tsx
is compensating for an upstream `as const` on the shared constant; update the
shared constant (referenced in terminal-scripts-section.tsx) to declare an
explicit type like Array<{id: string; name: string; command: string}> (e.g.
export const DEFAULT_TERMINAL_SCRIPTS: Array<...> = [...]) instead of using `as
const`, then remove the `as unknown as Array<...>` cast and return the constant
directly (use the symbol DEFAULT_SCRIPTS / DEFAULT_TERMINAL_SCRIPTS to locate
and adjust the declaration and consumer).
- Line 94: The JSX uses the cn utility unnecessarily for a single static class
string; in the Play component invocation inside terminal-scripts-dropdown.tsx
replace the call to cn('h-3.5 w-3.5 shrink-0 text-brand-500') with the plain
className string 'h-3.5 w-3.5 shrink-0 text-brand-500' (i.e., remove cn) to
simplify the code and avoid an extra function call.
Comment on lines
+160
to
164
| // Claim clients so the SW controls the page — but only on first install | ||
| // (when skipWaiting was requested) or when the page loads fresh. | ||
| // This is safe because without skipWaiting, the activate event only fires | ||
| // after all old-SW-controlled pages are closed and reopened. | ||
| self.clients.claim(); |
There was a problem hiding this comment.
Comment on clients.claim() is misleading — it executes unconditionally.
Lines 160–163 say claim happens "only on first install (when skipWaiting was requested) or when the page loads fresh," but the code calls self.clients.claim() unconditionally on every activation. The comment should reflect the actual behavior: claim() is always called, but activate itself only fires at safe times (first install, after skipWaiting(), or after all old tabs close).
Suggested comment fix
- // Claim clients so the SW controls the page — but only on first install
- // (when skipWaiting was requested) or when the page loads fresh.
- // This is safe because without skipWaiting, the activate event only fires
- // after all old-SW-controlled pages are closed and reopened.
+ // Claim clients so the SW immediately controls all open pages.
+ // This is safe because the activate event only fires at safe times:
+ // on first install, after an explicit skipWaiting() call, or after all
+ // tabs using the old SW have been closed.
self.clients.claim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/public/sw.js` around lines 160 - 164, Update the misleading comment
above self.clients.claim(): explain that self.clients.claim() is called
unconditionally on activation, while the activate event itself only fires at
safe times (first install, after skipWaiting() is called, or after all old
clients are closed), and mention skipWaiting() and the activate event to clarify
why unconditional claim() is safe; keep the comment concise and reference
self.clients.claim() and skipWaiting().
apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/project-settings-view/terminal-scripts-section.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/project-settings-view/terminal-scripts-section.tsx
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces several UX improvements to the worktree workflow, focusing on making pull request creation more flexible and adding base branch selection when creating worktrees.
🔀 PR Dialog: Target Remote Support
originandupstream)originwhile creating the PR againstupstreammain/master) when the target remote changes🌿 Create Worktree Dialog: Base Branch Selection
HEAD)⬆️ Commit Dialog: Push After Commit
Cmd/Ctrl+Entershortcut is also gated on the push remote being selected when push is enabled🛠️ Backend: Remote Branch Fetching & Target Remote
origin/main)targetRemoteparameter to explicitly control which remote the PR is created againstremoteandtargetRemotevalues are now validated against injection attacks independently🐛 Fix: Auto-mode Concurrency Count
auto_mode_feature_startevents being fired by both the execution-service and pipeline-orchestrator for the same task IDapp-store.tsto reject duplicate task entries before they are appendedTest Plan
origin/main) as the base branch — verify the latest is fetched and the worktree starts from that reforiginandupstreamremotes) — verify independent push remote and target remote selection🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
UI/UX