Skip to content

Add existing worktree import flow#16

Open
joshdoe wants to merge 1 commit intojohannesjo:mainfrom
joshdoe:feat/import-existing-worktrees
Open

Add existing worktree import flow#16
joshdoe wants to merge 1 commit intojohannesjo:mainfrom
joshdoe:feat/import-existing-worktrees

Conversation

@joshdoe
Copy link
Copy Markdown

@joshdoe joshdoe commented Mar 9, 2026

Summary

  • add a UI flow for importing existing Git worktrees into the app
  • add Electron IPC support for discovering and importing worktree metadata
  • connect imported worktrees into task persistence and sidebar/task panel flows

@joshdoe joshdoe marked this pull request as ready for review March 9, 2026 18:54
Copy link
Copy Markdown
Owner

@johannesjo johannesjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this and sorry for the late reply. Somehow I wasn't receiving notifications for PRs opened here....

Code Review

Overall: Well-structured PR. The implementation follows codebase patterns closely, the externalWorktree flag is properly threaded through close/merge/persistence paths, and the backend worktree parsing is solid. Security is handled correctly (path validation, array-form exec, preload allowlist).

Important Issues

1. Misleading close message in TilingLayout.tsx:117-119

The force-close confirmation says "The worktree and branch will be deleted" for external worktrees, which is incorrect — they're intentionally kept. This file wasn't updated in the PR:

// Current (incorrect for external worktrees):
const msg = task.directMode
  ? 'Close this task? Running agents and shells will be stopped.'
  : 'Close this task? The worktree and branch will be deleted.';

// Suggested:
const msg = task.directMode || task.externalWorktree
  ? 'Close this task? Running agents and shells will be stopped.'
  : 'Close this task? The worktree and branch will be deleted.';

2. No duplicate import guard in createImportedTask (src/store/tasks.ts)

The UI filters already-tracked paths via trackedWorktreePaths, but createImportedTask itself has no guard against importing the same worktree path twice. A rapid double-click or programmatic call could create duplicates. Compare with createDirectTask which has the hasDirectModeTask() guard at line 144. Suggestion:

const allTaskIds = [...store.taskOrder, ...store.collapsedTaskOrder];
if (allTaskIds.some((id) => store.tasks[id]?.worktreePath === worktree.path)) {
  throw new Error('Worktree is already tracked as a task');
}

Minor Notes

  • Partial import failure (ImportWorktreesDialog.tsx): Sequential import means if one fails mid-batch, earlier ones are already created. The dialog shows the error but doesn't indicate how many succeeded (e.g. "2 of 5 imported"). Works correctly on reopen though (filters out already-tracked).
  • File length: ImportWorktreesDialog.tsx is 364 lines vs. the 300-line guideline. The StatusBadge helper at the bottom could be extracted.

What's done well

  • externalWorktree flag correctly propagated through all persistence layers (types, save, restore, autosave)
  • Close task properly skips git cleanup for external worktrees
  • Merge dialog defaults initialCleanup: false for imports
  • Backend: validatePath on IPC, array-form exec, safeRealpath fallback
  • Auto-import prompt on project add is a nice UX touch

@johannesjo
Copy link
Copy Markdown
Owner

Thank you very much! <3

@johannesjo
Copy link
Copy Markdown
Owner

Code Review — Additional Finding

The existing review by @johannesjo is thorough. One new issue not yet noted:

Missing required gitIsolation field in createImportedTask (src/store/tasks.ts)

The Task interface has gitIsolation: GitIsolationMode as a required field (no ?). The Task object constructed in createImportedTask does not set it:

const task: Task = {
  id,
  name,
  projectId,
  branchName: worktree.branch_name,
  worktreePath: worktree.path,
  agentIds: [agentId],
  shellAgentIds: [],
  notes: '',
  lastPrompt: '',
  externalWorktree: true,
  // ❌ missing: gitIsolation
};

This is a TypeScript strict-mode error (npm run typecheck should fail). At runtime, task.gitIsolation will be undefined, which causes a second-order problem: the force-close message in TilingLayout.tsx:119 uses task.gitIsolation === 'direct' — with undefined that evaluates to false, so it shows "The worktree and branch will be deleted" for imported tasks (the same user-facing bug the first review identified, but caused here). The correct value to set is 'worktree', since imported worktrees do have their own worktree:

const task: Task = {
  // ...existing fields...
  gitIsolation: 'worktree',
  externalWorktree: true,
};

The TilingLayout.tsx fix already noted in the first review (adding || task.externalWorktree) is still needed as a safety net, but setting gitIsolation is the root fix.

Copy link
Copy Markdown
Owner

@johannesjo johannesjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript error: createImportedTask in src/store/tasks.ts constructs a Task without the required gitIsolation: GitIsolationMode field. This causes undefined at runtime and surfaces as the wrong close-confirmation message in TilingLayout.tsx. Fix: add gitIsolation: 'worktree' to the task literal.

The existing review findings (missing duplicate-import guard) also still need addressing. Details in the review comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants