feat(review): add Create PR option to human review workflow #412
Conversation
Adds a "Create PR" button alongside Stage/Merge in the human review workflow. Users can select a target branch and create a GitHub PR directly from the review panel. Features: - Auto-push branch to remote if not already pushed - Smart default branch selection (task baseBranch → project defaultBranch → main/master) - "PR Created" badge displayed on task cards and detail modal - Task stays in human_review status after PR creation (user decides next action) - PR URL stored in implementation_plan.json for tracking - i18n support (English and French)
github-actions
bot
added
area/frontend
📝 WalkthroughWalkthroughA new PR feature is added, enabling users to create pull requests directly from task worktrees. Changes span backend IPC handlers for PR creation with git/gh integration, project-store updates for PR metadata storage, frontend API layer wiring, UI components for branch selection and PR status display, state management via hooks, localization strings, and type definitions. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as TaskDetailModal/<br/>WorkspaceStatus
participant Hook as useTaskDetail
participant API as TaskAPI
participant Main as Main Process<br/>(IPC Handler)
participant Git as Git/GH CLI
participant Fs as File System
User->>UI: Click "Create PR"
activate UI
UI->>UI: Validate target branch
UI->>Hook: setIsCreatingPR(true)
activate Hook
Hook-->>UI: Update state
deactivate Hook
UI->>API: createPullRequest(taskId, branch)
activate API
API->>Main: IPC invoke TASK_CREATE_PR
deactivate API
activate Main
Main->>Main: Validate task & worktree
Main->>Git: Get current branch
Main->>Git: Ensure upstream set
Main->>Git: Push branch (if needed)
rect rgba(76, 175, 80, 0.2)
Note over Main,Git: PR Creation Phase
Main->>Git: gh pr create --body...
Git-->>Main: PR URL & number
end
Main->>Fs: Write to implementation_plan.json<br/>(async, best-effort)
Main-->>API: Return PR details
deactivate Main
activate UI
UI->>Hook: setPrSuccess({prUrl, message})
activate Hook
Hook-->>UI: Update state
deactivate Hook
UI->>Hook: setShowPRBranchSelector(false)
UI->>Hook: setIsCreatingPR(false)
UI->>UI: Display PR Created badge
deactivate UI
User->>UI: View PR (click link)
activate UI
UI->>UI: Open prUrl in browser
deactivate UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| plan.prUrl = prUrl; | ||
| plan.prCreatedAt = new Date().toISOString(); | ||
| plan.updated_at = new Date().toISOString(); | ||
| writeFileSync(planPath, JSON.stringify(plan, null, 2)); |
Check failure
Code scanning / CodeQL
Potential file system race condition High
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/main/project-store.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/renderer/components/TaskCard.tsxapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/task-detail/TaskDetailPanel.tsxapps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.tsapps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsxapps/frontend/src/renderer/lib/mocks/workspace-mock.tsapps/frontend/src/shared/constants/ipc.tsapps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.jsonapps/frontend/src/shared/types/ipc.tsapps/frontend/src/shared/types/task.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/main/project-store.tsapps/frontend/src/renderer/lib/mocks/workspace-mock.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/shared/types/task.tsapps/frontend/src/shared/types/ipc.tsapps/frontend/src/shared/constants/ipc.tsapps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.tsapps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsxapps/frontend/src/renderer/components/task-detail/TaskDetailPanel.tsxapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/TaskCard.tsx
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/project-store.tsapps/frontend/src/renderer/lib/mocks/workspace-mock.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/shared/types/task.tsapps/frontend/src/shared/types/ipc.tsapps/frontend/src/shared/constants/ipc.tsapps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.tsapps/frontend/src/renderer/components/task-detail/TaskReview.tsxapps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsxapps/frontend/src/renderer/components/task-detail/TaskDetailPanel.tsxapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsxapps/frontend/src/renderer/components/TaskCard.tsx
apps/frontend/src/shared/i18n/locales/**/*.json
📄 CodeRabbit inference engine (CLAUDE.md)
Add new translation keys to ALL language files (minimum:
en/*.jsonandfr/*.json) inapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.json
🧠 Learnings (2)
📚 Learning: 2025-12-25T18:29:32.954Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T18:29:32.954Z
Learning: Applies to apps/frontend/src/shared/i18n/locales/**/*.json : Add new translation keys to ALL language files (minimum: `en/*.json` and `fr/*.json`) in `apps/frontend/src/shared/i18n/locales/`
Applied to files:
apps/frontend/src/shared/i18n/locales/en/tasks.jsonapps/frontend/src/shared/i18n/locales/fr/tasks.json
📚 Learning: 2025-12-25T18:29:32.954Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T18:29:32.954Z
Learning: Applies to apps/frontend/src/**/*.{ts,tsx} : Always use translation keys with `useTranslation()` for all user-facing text in React/TypeScript frontend components - use format `namespace:section.key` (e.g., `navigation:items.githubPRs`)
Applied to files:
apps/frontend/src/renderer/components/TaskCard.tsx
🧬 Code graph analysis (6)
apps/frontend/src/preload/api/task-api.ts (1)
apps/frontend/src/shared/constants/ipc.ts (1)
IPC_CHANNELS(6-378)
apps/frontend/src/shared/types/ipc.ts (1)
apps/frontend/src/shared/types/task.ts (1)
CreatePullRequestResult(409-415)
apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx (1)
.design-system/src/components/Button.tsx (1)
Button(10-44)
apps/frontend/src/renderer/components/task-detail/TaskDetailPanel.tsx (1)
apps/frontend/src/renderer/components/settings/utils/hookProxyFactory.ts (1)
error(18-18)
apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx (1)
.design-system/src/components/Badge.tsx (1)
Badge(9-27)
apps/frontend/src/renderer/components/TaskCard.tsx (1)
.design-system/src/components/Badge.tsx (1)
Badge(9-27)
🪛 GitHub Check: CodeQL
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts
[failure] 2313-2313: Potential file system race condition
The file may have changed since it was checked.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (17)
apps/frontend/src/preload/api/task-api.ts (2)
54-54: LGTM! Clean API addition.The new
createPullRequestmethod is properly typed and follows the established pattern for IPC communication in this codebase.
145-146: LGTM! Proper IPC wiring.The implementation correctly invokes the IPC channel with the expected parameters and return type.
apps/frontend/src/renderer/components/TaskCard.tsx (1)
257-266: LGTM! Proper i18n usage and consistent styling.The PR Created badge correctly uses the translation key
t('labels.prCreated')as per coding guidelines, and the styling is consistent with other status badges in the component.As per coding guidelines, the translation key must exist in all language files. This will be verified in the review of the locale files.
apps/frontend/src/shared/constants/ipc.ts (1)
42-42: LGTM! Clean constant addition.The new IPC channel constant follows the established naming convention and is properly documented with an inline comment.
apps/frontend/src/shared/types/task.ts (2)
253-254: LGTM! Clean type extension.The new optional fields
prUrlandprCreatedAtare properly integrated into the Task interface and align with the PR creation feature requirements.
406-415: LGTM! Well-documented interface.The
CreatePullRequestResultinterface is properly defined with clear JSDoc comments and appropriate field types. The optionalwasPushedfield provides useful context about the operation.apps/frontend/src/shared/types/ipc.ts (2)
41-41: LGTM! Proper type import.The
CreatePullRequestResulttype is correctly imported from the task types module.
153-153: LGTM! Clean API signature.The
createPullRequestmethod signature is properly typed and consistent with other workspace management operations in theElectronAPIinterface.apps/frontend/src/shared/i18n/locales/en/tasks.json (1)
29-29: All French translations are already present inapps/frontend/src/shared/i18n/locales/fr/tasks.json. All 12 new keys have been added with proper French translations:
labels.prCreated: "PR créée"review.createPR,createPRTitle,targetBranch,selectBranch,creatingPR,viewPR,prCreated,prAlreadyExists,ghNotInstalled,pushFailed,noCommitsare all present with appropriate French translations.No action required.
apps/frontend/src/shared/i18n/locales/fr/tasks.json (1)
87-99: LGTM! French translations for PR feature are complete.The new
reviewsection properly provides French translations for all PR-related strings. The translations appear accurate and consistent with the existing French localization style. Based on learnings, this aligns with the requirement to add translation keys to all language files.apps/frontend/src/renderer/lib/mocks/workspace-mock.ts (1)
94-103: LGTM! Mock correctly simulates PR creation response.The mock method signature and return structure align with the
CreatePullRequestResulttype and will support testing of the PR creation flow without requiring actual GitHub integration.apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx (1)
155-178: LGTM! PR creation handler is well-structured.The
handleCreatePRfunction properly:
- Validates target branch before proceeding
- Manages loading state with
isCreatingPR- Clears errors before the operation
- Uses try/catch/finally pattern for proper cleanup
- Handles both success and error cases appropriately
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts (1)
2215-2267: LGTM! PR creation handler with proper git workflow.The handler correctly:
- Validates task and worktree existence
- Gets current branch from worktree
- Checks for upstream tracking and auto-pushes if needed
- Provides clear error messages for push failures
apps/frontend/src/renderer/components/task-detail/TaskReview.tsx (2)
45-53: LGTM! PR props interface is well-defined.The new PR-related props are appropriately marked as optional, allowing backward compatibility with existing usages. The types correctly match the expected shapes from the parent components.
140-148: LGTM! Clean prop forwarding to WorkspaceStatus.The PR-related props are correctly forwarded to the child component, maintaining the component's role as a composition layer without adding logic.
apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx (2)
428-440: LGTM! PR button with proper i18n and state management.The Create PR button correctly:
- Uses translation key
t('tasks:review.createPR')- Disables during merge, discard, or PR creation operations
- Toggles the branch selector visibility
As per coding guidelines, user-facing text is properly internationalized.
453-507: LGTM! PR branch selector UI is well-implemented.The branch selector panel:
- Uses translation keys consistently for all text
- Properly manages disabled states during PR creation
- Provides clear cancel action
- Uses Radix Select component correctly with proper placeholder
| // Check if gh CLI is available | ||
| const ghPath = getToolPath('gh'); | ||
| if (!ghPath) { | ||
| return { | ||
| success: false, | ||
| error: 'GitHub CLI (gh) not found. Install it from https://cli.github.com/' | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# First, read the specific file and surrounding context
wc -l apps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 131
🏁 Script executed:
# Read the context around lines 2269-2276
sed -n '2250,2350p' apps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 3551
🏁 Script executed:
# Search for gh command usage and error handling patterns in this file
rg -n 'gh\s+(pr|auth|status)' apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts -A 3Repository: AndyMik90/Auto-Claude
Length of output: 393
🏁 Script executed:
# Check if there are any existing authentication checks in the codebase
rg -n 'gh auth status' apps/frontend/src/main/ipc-handlers/task/ -A 2Repository: AndyMik90/Auto-Claude
Length of output: 47
Add GitHub CLI authentication verification before PR creation.
The handler checks if gh is installed but doesn't verify authentication status. If gh is installed but not authenticated, the gh pr create command fails with a generic error message rather than clearly indicating an authentication issue.
Consider adding an explicit gh auth status check before attempting PR creation, or add specific error pattern matching to detect and report authentication failures with a clear message like "GitHub CLI not authenticated. Run 'gh auth login' to authenticate."
🤖 Prompt for AI Agents
In apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts around lines
2269 to 2276, the code only checks that the GitHub CLI binary exists but does
not verify the user is authenticated; add a pre-check that runs "gh auth status"
(or equivalent) and treat non-zero exit or auth-specific output as
unauthenticated, returning a clear error like "GitHub CLI not authenticated. Run
'gh auth login' to authenticate."; additionally, when running gh pr create,
catch and pattern-match auth-related errors and convert them to the same clear
message so callers get an actionable response.
| if (existsSync(planPath)) { | ||
| try { | ||
| const plan = JSON.parse(readFileSync(planPath, 'utf-8')); | ||
| plan.prUrl = prUrl; | ||
| plan.prCreatedAt = new Date().toISOString(); | ||
| plan.updated_at = new Date().toISOString(); | ||
| writeFileSync(planPath, JSON.stringify(plan, null, 2)); | ||
| } catch (planError) { | ||
| console.error('Failed to update implementation plan:', planError); | ||
| // Don't fail the PR creation just because plan update failed | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix TOCTOU race condition in plan file update.
The existsSync check at line 2307 followed by readFileSync/writeFileSync creates a time-of-check to time-of-use race condition. The file could be modified or deleted between the check and the actual file operations.
🔎 Proposed fix using EAFP pattern
- if (existsSync(planPath)) {
- try {
- const plan = JSON.parse(readFileSync(planPath, 'utf-8'));
- plan.prUrl = prUrl;
- plan.prCreatedAt = new Date().toISOString();
- plan.updated_at = new Date().toISOString();
- writeFileSync(planPath, JSON.stringify(plan, null, 2));
- } catch (planError) {
- console.error('Failed to update implementation plan:', planError);
- // Don't fail the PR creation just because plan update failed
- }
+ try {
+ const plan = JSON.parse(readFileSync(planPath, 'utf-8'));
+ plan.prUrl = prUrl;
+ plan.prCreatedAt = new Date().toISOString();
+ plan.updated_at = new Date().toISOString();
+ writeFileSync(planPath, JSON.stringify(plan, null, 2));
+ } catch (planError) {
+ // File may not exist or could have been modified - this is non-critical
+ if ((planError as NodeJS.ErrnoException).code !== 'ENOENT') {
+ console.error('Failed to update implementation plan:', planError);
+ }
+ // Don't fail the PR creation just because plan update failed
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (existsSync(planPath)) { | |
| try { | |
| const plan = JSON.parse(readFileSync(planPath, 'utf-8')); | |
| plan.prUrl = prUrl; | |
| plan.prCreatedAt = new Date().toISOString(); | |
| plan.updated_at = new Date().toISOString(); | |
| writeFileSync(planPath, JSON.stringify(plan, null, 2)); | |
| } catch (planError) { | |
| console.error('Failed to update implementation plan:', planError); | |
| // Don't fail the PR creation just because plan update failed | |
| } | |
| } | |
| try { | |
| const plan = JSON.parse(readFileSync(planPath, 'utf-8')); | |
| plan.prUrl = prUrl; | |
| plan.prCreatedAt = new Date().toISOString(); | |
| plan.updated_at = new Date().toISOString(); | |
| writeFileSync(planPath, JSON.stringify(plan, null, 2)); | |
| } catch (planError) { | |
| // File may not exist or could have been modified - this is non-critical | |
| if ((planError as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| console.error('Failed to update implementation plan:', planError); | |
| } | |
| // Don't fail the PR creation just because plan update failed | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 2313-2313: Potential file system race condition
The file may have changed since it was checked.
🤖 Prompt for AI Agents
In apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts around lines
2307-2318, replace the existsSync+readFileSync/writeFileSync sequence (TOCTOU)
with an EAFP approach: attempt to read and parse the file inside a try/catch
(handle ENOENT by treating the file as absent and not failing PR creation),
update the parsed object if successful, and write the updated content atomically
by writing to a temporary file in the same directory and renaming it over the
original; ensure JSON parse errors and write errors are caught and logged but do
not abort PR creation.
| // Extract staged status and PR info from plan | ||
| const planWithExtras = plan as unknown as { stagedInMainProject?: boolean; stagedAt?: string; prUrl?: string; prCreatedAt?: string } | null; | ||
| const stagedInMainProject = planWithExtras?.stagedInMainProject; | ||
| const stagedAt = planWithExtras?.stagedAt; | ||
| const prUrl = planWithExtras?.prUrl; | ||
| const prCreatedAt = planWithExtras?.prCreatedAt; |
There was a problem hiding this comment.
Consider safer type handling for plan extensions.
The type assertion as unknown as bypasses TypeScript's type safety. While the code works because all fields are optional and safely accessed, this pattern could mask future issues if the plan structure changes.
💡 Suggested approach using type guards or extending the ImplementationPlan type
Option 1: Extend ImplementationPlan type (if this is now part of the standard structure)
In apps/frontend/src/shared/types/task.ts:
export interface ImplementationPlan {
// ... existing fields
stagedInMainProject?: boolean;
stagedAt?: string;
prUrl?: string;
prCreatedAt?: string;
}Then no type assertion is needed:
-// Extract staged status and PR info from plan
-const planWithExtras = plan as unknown as { stagedInMainProject?: boolean; stagedAt?: string; prUrl?: string; prCreatedAt?: string } | null;
-const stagedInMainProject = planWithExtras?.stagedInMainProject;
-const stagedAt = planWithExtras?.stagedAt;
-const prUrl = planWithExtras?.prUrl;
-const prCreatedAt = planWithExtras?.prCreatedAt;
+// Extract staged status and PR info from plan
+const stagedInMainProject = plan?.stagedInMainProject;
+const stagedAt = plan?.stagedAt;
+const prUrl = plan?.prUrl;
+const prCreatedAt = plan?.prCreatedAt;Option 2: Use type guard (if these fields are dynamically added)
// Helper function at top of file
function hasPRInfo(plan: ImplementationPlan | null): plan is ImplementationPlan & {
stagedInMainProject?: boolean;
stagedAt?: string;
prUrl?: string;
prCreatedAt?: string;
} {
return plan !== null;
}
// Then use:
const prUrl = hasPRInfo(plan) ? plan.prUrl : undefined;
const prCreatedAt = hasPRInfo(plan) ? plan.prCreatedAt : undefined;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
apps/frontend/src/main/project-store.ts lines 425-430: the code unsafely uses an
`as unknown as` assertion to read optional PR/staging fields from `plan`; either
extend the shared ImplementationPlan type
(apps/frontend/src/shared/types/task.ts) to include stagedInMainProject?,
stagedAt?, prUrl?, prCreatedAt? and remove the assertion, or add a narrow type
guard that checks plan is non-null/has those optional keys and use that guard
before reading the properties, replacing the `as unknown as` pattern with the
safer typed access.
| // Load available branches for PR creation when task is in human_review | ||
| useEffect(() => { | ||
| if (needsReview && selectedProject) { | ||
| // Get branches | ||
| window.electronAPI.getGitBranches(selectedProject.path).then((result) => { | ||
| if (result.success && result.data) { | ||
| setAvailableBranches(result.data); | ||
|
|
||
| // Set default target branch: | ||
| // 1. Task's baseBranch if set | ||
| // 2. Project's defaultBranch | ||
| // 3. Detected main branch (main/master/develop) | ||
| const taskBaseBranch = task.metadata?.baseBranch; | ||
| if (taskBaseBranch && result.data.includes(taskBaseBranch)) { | ||
| setPrTargetBranch(taskBaseBranch); | ||
| } else { | ||
| window.electronAPI.getProjectEnv(selectedProject.id).then((envResult) => { | ||
| if (envResult.success && envResult.data?.defaultBranch && result.data?.includes(envResult.data.defaultBranch)) { | ||
| setPrTargetBranch(envResult.data.defaultBranch); | ||
| } else { | ||
| // Use main/master/develop as fallback | ||
| const mainBranch = result.data?.find(b => ['main', 'master', 'develop'].includes(b)); | ||
| if (mainBranch) { | ||
| setPrTargetBranch(mainBranch); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| }, [needsReview, selectedProject, task.metadata?.baseBranch]); |
There was a problem hiding this comment.
Add error handling and simplify nested promise chain.
The nested promise chain lacks error handling, which means failures from getGitBranches or getProjectEnv will silently fail without notifying the user. Additionally, the deeply nested structure makes the logic harder to follow and maintain.
🔎 Proposed refactor with error handling and flattened structure
// Load available branches for PR creation when task is in human_review
useEffect(() => {
if (needsReview && selectedProject) {
- // Get branches
- window.electronAPI.getGitBranches(selectedProject.path).then((result) => {
- if (result.success && result.data) {
- setAvailableBranches(result.data);
-
- // Set default target branch:
- // 1. Task's baseBranch if set
- // 2. Project's defaultBranch
- // 3. Detected main branch (main/master/develop)
- const taskBaseBranch = task.metadata?.baseBranch;
- if (taskBaseBranch && result.data.includes(taskBaseBranch)) {
- setPrTargetBranch(taskBaseBranch);
- } else {
- window.electronAPI.getProjectEnv(selectedProject.id).then((envResult) => {
- if (envResult.success && envResult.data?.defaultBranch && result.data?.includes(envResult.data.defaultBranch)) {
- setPrTargetBranch(envResult.data.defaultBranch);
- } else {
- // Use main/master/develop as fallback
- const mainBranch = result.data?.find(b => ['main', 'master', 'develop'].includes(b));
- if (mainBranch) {
- setPrTargetBranch(mainBranch);
- }
- }
- });
- }
- }
- });
+ const loadBranchesAndDefaults = async () => {
+ try {
+ const result = await window.electronAPI.getGitBranches(selectedProject.path);
+ if (!result.success || !result.data) {
+ console.error('[useTaskDetail] Failed to load branches:', result.error);
+ return;
+ }
+
+ setAvailableBranches(result.data);
+
+ // Set default target branch with priority:
+ // 1. Task's baseBranch if set
+ const taskBaseBranch = task.metadata?.baseBranch;
+ if (taskBaseBranch && result.data.includes(taskBaseBranch)) {
+ setPrTargetBranch(taskBaseBranch);
+ return;
+ }
+
+ // 2. Project's defaultBranch
+ const envResult = await window.electronAPI.getProjectEnv(selectedProject.id);
+ if (envResult.success && envResult.data?.defaultBranch && result.data.includes(envResult.data.defaultBranch)) {
+ setPrTargetBranch(envResult.data.defaultBranch);
+ return;
+ }
+
+ // 3. Detected main branch (main/master/develop)
+ const mainBranch = result.data.find(b => ['main', 'master', 'develop'].includes(b));
+ if (mainBranch) {
+ setPrTargetBranch(mainBranch);
+ }
+ } catch (err) {
+ console.error('[useTaskDetail] Error loading branches:', err);
+ }
+ };
+
+ loadBranchesAndDefaults();
}
}, [needsReview, selectedProject, task.metadata?.baseBranch]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Load available branches for PR creation when task is in human_review | |
| useEffect(() => { | |
| if (needsReview && selectedProject) { | |
| // Get branches | |
| window.electronAPI.getGitBranches(selectedProject.path).then((result) => { | |
| if (result.success && result.data) { | |
| setAvailableBranches(result.data); | |
| // Set default target branch: | |
| // 1. Task's baseBranch if set | |
| // 2. Project's defaultBranch | |
| // 3. Detected main branch (main/master/develop) | |
| const taskBaseBranch = task.metadata?.baseBranch; | |
| if (taskBaseBranch && result.data.includes(taskBaseBranch)) { | |
| setPrTargetBranch(taskBaseBranch); | |
| } else { | |
| window.electronAPI.getProjectEnv(selectedProject.id).then((envResult) => { | |
| if (envResult.success && envResult.data?.defaultBranch && result.data?.includes(envResult.data.defaultBranch)) { | |
| setPrTargetBranch(envResult.data.defaultBranch); | |
| } else { | |
| // Use main/master/develop as fallback | |
| const mainBranch = result.data?.find(b => ['main', 'master', 'develop'].includes(b)); | |
| if (mainBranch) { | |
| setPrTargetBranch(mainBranch); | |
| } | |
| } | |
| }); | |
| } | |
| } | |
| }); | |
| } | |
| }, [needsReview, selectedProject, task.metadata?.baseBranch]); | |
| // Load available branches for PR creation when task is in human_review | |
| useEffect(() => { | |
| if (needsReview && selectedProject) { | |
| const loadBranchesAndDefaults = async () => { | |
| try { | |
| const result = await window.electronAPI.getGitBranches(selectedProject.path); | |
| if (!result.success || !result.data) { | |
| console.error('[useTaskDetail] Failed to load branches:', result.error); | |
| return; | |
| } | |
| setAvailableBranches(result.data); | |
| // Set default target branch with priority: | |
| // 1. Task's baseBranch if set | |
| const taskBaseBranch = task.metadata?.baseBranch; | |
| if (taskBaseBranch && result.data.includes(taskBaseBranch)) { | |
| setPrTargetBranch(taskBaseBranch); | |
| return; | |
| } | |
| // 2. Project's defaultBranch | |
| const envResult = await window.electronAPI.getProjectEnv(selectedProject.id); | |
| if (envResult.success && envResult.data?.defaultBranch && result.data.includes(envResult.data.defaultBranch)) { | |
| setPrTargetBranch(envResult.data.defaultBranch); | |
| return; | |
| } | |
| // 3. Detected main branch (main/master/develop) | |
| const mainBranch = result.data.find(b => ['main', 'master', 'develop'].includes(b)); | |
| if (mainBranch) { | |
| setPrTargetBranch(mainBranch); | |
| } | |
| } catch (err) { | |
| console.error('[useTaskDetail] Error loading branches:', err); | |
| } | |
| }; | |
| loadBranchesAndDefaults(); | |
| } | |
| }, [needsReview, selectedProject, task.metadata?.baseBranch]); |
🤖 Prompt for AI Agents
In apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts
around lines 138-169, the current nested promise chain for getGitBranches and
getProjectEnv has no error handling and is deeply nested; refactor the effect to
an async IIFE (or separate async function) with try/catch to handle and log (or
surface) errors from both API calls, flatten the logic by awaiting
getGitBranches first, setAvailableBranches when successful, then determine pr
target branch by checking task.metadata?.baseBranch, then
project.env.defaultBranch, then fallback to main/master/develop, and include a
mounted/aborted flag to avoid state updates on unmounted components; ensure all
promise rejections are caught and handled (e.g., set an error state or notify
the user) and remove the nested .then chains.
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-7 px-2" | ||
| onClick={() => window.electronAPI.openExternal(prSuccess.prUrl)} | ||
| > | ||
| <ExternalLink className="h-3.5 w-3.5 mr-1" /> | ||
| {t('tasks:review.viewPR')} | ||
| </Button> |
There was a problem hiding this comment.
Add error handling for openExternal call.
The window.electronAPI.openExternal(prSuccess.prUrl) call at line 521 is invoked without error handling. If the URL is malformed or the external handler fails, the error will be silently swallowed.
🔎 Proposed fix
<Button
variant="ghost"
size="sm"
className="h-7 px-2"
- onClick={() => window.electronAPI.openExternal(prSuccess.prUrl)}
+ onClick={async () => {
+ try {
+ await window.electronAPI.openExternal(prSuccess.prUrl);
+ } catch (err) {
+ console.error('Failed to open PR URL:', err);
+ }
+ }}
>🤖 Prompt for AI Agents
In
apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx
around lines 517 to 525, the call to
window.electronAPI.openExternal(prSuccess.prUrl) is invoked without any error
handling; wrap the openExternal invocation in a try/catch (or handle the
returned Promise with .catch) and validate the URL before calling, and on error
log the failure and surface a user-friendly notification/toast so failures
aren’t silently swallowed. Ensure the handler is synchronous-friendly (use async
onClick if awaiting) and include the URL in the error log for debugging.
| {task.prUrl && ( | ||
| <Badge | ||
| variant="outline" | ||
| className="text-xs flex items-center gap-1 bg-purple-500/10 text-purple-400 border-purple-500/30" | ||
| > | ||
| <GitPullRequest className="h-3 w-3" /> | ||
| PR Created | ||
| </Badge> | ||
| )} |
There was a problem hiding this comment.
Use translation key for user-facing text.
The "PR Created" text on line 326 is hardcoded. As per coding guidelines, all user-facing text in React components should use useTranslation() with translation keys.
🔎 Proposed fix
+import { useTranslation } from 'react-i18next';
// ... in component:
+const { t } = useTranslation('tasks');
// ... in the badge:
{task.prUrl && (
<Badge
variant="outline"
className="text-xs flex items-center gap-1 bg-purple-500/10 text-purple-400 border-purple-500/30"
>
<GitPullRequest className="h-3 w-3" />
- PR Created
+ {t('labels.prCreated')}
</Badge>
)}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx around
lines 320 to 328, the "PR Created" string is hardcoded; replace it with a
translation key using the useTranslation() hook (e.g. const { t } =
useTranslation()) and call t('task.prCreated') (or your chosen key) where the
text is rendered; ensure the hook is imported from react-i18next, update the
component to use t, and add the corresponding key/value to the locale JSON files
so translations exist.
| const handleCreatePR = async () => { | ||
| if (!state.prTargetBranch) { | ||
| state.setWorkspaceError('Please select a target branch'); | ||
| return; | ||
| } | ||
| state.setIsCreatingPR(true); | ||
| state.setWorkspaceError(null); | ||
| try { | ||
| const result = await window.electronAPI.createPullRequest(task.id, state.prTargetBranch); | ||
| if (result.success && result.data?.success) { | ||
| state.setPrSuccess({ | ||
| prUrl: result.data.prUrl || '', | ||
| message: result.data.message | ||
| }); | ||
| state.setShowPRBranchSelector(false); | ||
| } else { | ||
| state.setWorkspaceError(result.data?.message || result.error || 'Failed to create pull request'); | ||
| } | ||
| } catch (error) { | ||
| state.setWorkspaceError(error instanceof Error ? error.message : 'Unknown error creating PR'); | ||
| } finally { | ||
| state.setIsCreatingPR(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting duplicated PR creation logic.
The handleCreatePR function is nearly identical to the one in TaskDetailModal.tsx (lines 155-178). This violates the DRY principle and creates maintenance burden.
Consider extracting this logic to either:
- A custom hook (e.g.,
usePRCreation) - The existing
useTaskDetailhook which already manages PR-related state
🔎 Suggested approach
// In useTaskDetail.ts or a new usePRCreation.ts hook:
const handleCreatePR = useCallback(async () => {
if (!prTargetBranch) {
setWorkspaceError('Please select a target branch');
return;
}
setIsCreatingPR(true);
setWorkspaceError(null);
try {
const result = await window.electronAPI.createPullRequest(task.id, prTargetBranch);
if (result.success && result.data?.success) {
setPrSuccess({
prUrl: result.data.prUrl || '',
message: result.data.message
});
setShowPRBranchSelector(false);
} else {
setWorkspaceError(result.data?.message || result.error || 'Failed to create pull request');
}
} catch (error) {
setWorkspaceError(error instanceof Error ? error.message : 'Unknown error creating PR');
} finally {
setIsCreatingPR(false);
}
}, [task.id, prTargetBranch, /* other deps */]);🤖 Prompt for AI Agents
In apps/frontend/src/renderer/components/task-detail/TaskDetailPanel.tsx around
lines 118 to 141, the PR creation logic is duplicated (also present in
TaskDetailModal.tsx) — extract it into a shared hook (e.g., usePRCreation or add
to existing useTaskDetail) that accepts task.id and exposes handleCreatePR plus
the PR-related state setters (isCreatingPR, workspaceError, prSuccess,
showPRBranchSelector, prTargetBranch) so both components call the same callback;
implement the hook using useCallback with correct dependencies (task.id,
prTargetBranch, state setters), move the try/catch/finally logic there, and
replace the inline function in both components with the hook's handleCreatePR to
remove duplication and keep behavior identical.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const [showPRBranchSelector, setShowPRBranchSelector] = useState(false); | ||
| const [prTargetBranch, setPrTargetBranch] = useState<string>(''); | ||
| const [availableBranches, setAvailableBranches] = useState<string[]>([]); | ||
| const [prSuccess, setPrSuccess] = useState<{ prUrl: string; message: string } | null>(null); |
There was a problem hiding this comment.
PR success state persists incorrectly when switching tasks
The PR-related state variables (prSuccess, showPRBranchSelector) are not reset when task.id changes. The TaskDetailModal is rendered without a key prop, so when a user clicks a different task while the modal is open, the same component instance receives a new task prop but useState values persist. If a user creates a PR for task A, then clicks task B, the success message from task A will incorrectly appear in task B's view. The codebase already has a pattern for handling this issue (lines 238-246 reset mergePreview when task.id changes), but this pattern wasn't applied to the new PR state.
| > | ||
| <GitPullRequest className="h-3 w-3" /> | ||
| PR Created | ||
| </Badge> |
There was a problem hiding this comment.
Hardcoded PR badge text breaks localization
The PR badge text "PR Created" is hardcoded instead of using the i18n translation key. In TaskCard.tsx, the same badge correctly uses {t('labels.prCreated')} which displays "PR créée" for French users. However, TaskDetailModal.tsx has the hardcoded English string, breaking the localization feature. French users would see inconsistent language between the task card ("PR créée") and the detail modal ("PR Created").
AlexMadera
left a comment
AlexMadera
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟡 MERGE WITH CHANGES
2 high-priority issues to address
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Medium | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 2 issue(s)
- Medium: 5 issue(s)
- Low: 2 issue(s)
Generated by Auto Claude PR Review
Findings (9 selected of 9 total)
🟠 [HIGH] Insufficient sanitization of task.title for gh CLI
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2298
task.title is passed directly to 'gh pr create --title' without validation. While execFileSync with array arguments prevents shell injection, a malicious title containing newlines, null bytes, or extremely long strings could cause issues with gh CLI or DoS. Titles should be sanitized: stripped of control characters, length-limited (256 chars), and validated for printable characters only.
Suggested fix:
Sanitize title before use: const sanitizedTitle = task.title.replace(/[\r\n\0]/g, '').substring(0, 256).trim(); Validate with /^[\x20-\x7E]+$/
🟠 [HIGH] User-controlled task.description embedded in PR body without sanitization
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2293
task.description is embedded directly into the prBody string and passed to 'gh pr create --body'. A malicious description could contain null bytes, extremely long content (DoS), or malicious markdown that renders dangerously in GitHub UI. Consider using --body-file instead of --body to write to a temp file, and validate/limit description length.
Suggested fix:
1. Limit description to 65536 chars max. 2. Strip null bytes. 3. Consider using gh pr create --body-file with a temp file instead of --body to avoid any argument parsing edge cases.
🟡 [MEDIUM] No validation of targetBranch parameter from IPC
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2218
targetBranch is passed directly from IPC call without validation. While Git has some protections, the parameter should be validated against Git ref naming rules, checked for path traversal (no '..'), null bytes stripped, and length-limited to prevent abuse.
Suggested fix:
Add validation: if (!targetBranch || typeof targetBranch !== 'string') return error; Validate against regex /^[a-zA-Z0-9][a-zA-Z0-9._\/-]*$/ and targetBranch.length <= 255; Reject if contains '..' or null bytes.
🟡 [MEDIUM] Raw error messages may leak sensitive information
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2318
Error messages from git and gh CLI are returned directly to the client via stderr/error.message. This could expose: file paths, git remote URLs with embedded credentials, GitHub tokens if gh CLI prints them, and internal project structure. Error messages should be sanitized before returning.
Suggested fix:
Create sanitizeErrorMessage() that: removes ghp_ tokens, redacts file paths, removes URLs with embedded credentials, and limits message length to 500 chars. Apply to all error returns.
🟡 [MEDIUM] Path traversal risk with task.specId in file path construction
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2301
task.specId is used to construct file paths (specDir, planPath) without explicit validation for path traversal sequences. If specId contains '../', it could access files outside the intended directory. While projectStore may provide some validation, explicit checks should be added here.
Suggested fix:
Validate specId: if (task.specId.includes('..') || task.specId.includes('/') || task.specId.includes('\\')) return error. Also normalize and verify resolved path starts with project.path.
🟡 [MEDIUM] Non-atomic file write to implementation_plan.json
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:2310
writeFileSync on implementation_plan.json is not atomic - if the process crashes mid-write, the file could be corrupted. The write should use a temp file + rename pattern for atomicity. Additionally, no validation of JSON structure before modification.
Suggested fix:
Write to temp file first (planPath + '.tmp'), then rename to planPath. Validate plan is an object before modifying. Wrap in try/finally to clean up temp file on failure.
🟡 [MEDIUM] handleCreatePR duplicated between Modal and Panel components
📁 apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx:152
The handleCreatePR function is defined with identical logic in both TaskDetailModal.tsx (lines 152-177) and TaskDetailPanel.tsx (lines 115-140). This DRY violation creates maintenance burden - bug fixes must be applied in two places and logic could diverge over time.
Suggested fix:
Extract handleCreatePR to the useTaskDetail hook which is already shared between both components. Return it from the hook and call with appropriate onSuccess callback (onOpenChange(false) vs onClose).
🔵 [LOW] Nested async calls without cleanup on unmount
📁 apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts:136
The useEffect for loading branches has nested async calls (getGitBranches then getProjectEnv) but no cleanup. If component unmounts during these calls, state updates will occur on unmounted component. Add isMounted flag and cleanup function to prevent memory leaks and React warnings.
Suggested fix:
Add let isMounted = true at effect start, check if (isMounted) before each setState, and return cleanup function: return () => { isMounted = false; }
🔵 [LOW] prSuccess state not cleared when task changes
📁 apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts:50
The prSuccess state is set when PR creation succeeds but is never cleared when switching to a different task or when the modal is closed and reopened. This could show stale success message for wrong task.
Suggested fix:
Add useEffect that clears prSuccess when task.id changes: useEffect(() => { setPrSuccess(null); }, [task.id]);
This review was generated by Auto Claude.
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Adds a "Create PR" button alongside Stage/Merge in the human review workflow. Users can select a target branch from a dropdown and create a GitHub PR directly from the review panel. The branch is auto-pushed to remote if needed, and a "PR Created" badge is displayed on the task card.
Related Issue
Closes #
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemChecklist
developbranchCI/Testing Requirements
Screenshots
Feature Toggle
use_feature_nameBreaking Changes
Breaking: No
Implementation Details
Features
baseBranchif set (task-level override)defaultBranchsettinghuman_reviewstatus after PR creation (user decides next action)Files Changed (15 files)
src/shared/types/task.ts- AddedprUrlandprCreatedAtfields to Task interfacesrc/shared/types/ipc.ts- AddedcreatePullRequestto ElectronAPIsrc/shared/constants/ipc.ts- AddedTASK_CREATE_PRchannelsrc/main/ipc-handlers/task/worktree-handlers.ts- PR creation handler usingghCLIsrc/main/project-store.ts- Load PR info from implementation_plan.jsonsrc/preload/api/task-api.ts- Exposed createPullRequest APIsrc/renderer/components/task-detail/hooks/useTaskDetail.ts- PR-related statesrc/renderer/components/task-detail/task-review/WorkspaceStatus.tsx- Create PR UIsrc/renderer/components/task-detail/TaskReview.tsx- Pass PR propssrc/renderer/components/task-detail/TaskDetailModal.tsx- handleCreatePR + badgesrc/renderer/components/task-detail/TaskDetailPanel.tsx- handleCreatePR + propssrc/renderer/components/TaskCard.tsx- PR Created badgesrc/shared/i18n/locales/en/tasks.json- English translationssrc/shared/i18n/locales/fr/tasks.json- French translationssrc/renderer/lib/mocks/workspace-mock.ts- Browser mockSummary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.