Code Review: feat: support folders without git

Thanks for this contribution, @MaicolBen! This is a well-structured feature that cleanly extends the existing git isolation model. I've done a thorough review of all 14 changed files. Below are my findings.

CI Status

• GitGuardian Security Checks: Passed (no secrets detected)
• No other CI checks configured on this PR.

Overall Assessment

The approach of adding 'none' as a third GitIsolationMode value is clean and leverages the existing architecture well. The majority of the changes are correctly hiding git-specific UI behind <Show> guards. Backward compatibility is well handled with isGitRepo?: boolean (undefined treated as true).

Positive Findings

1. Clean type extension: Adding 'none' to the GitIsolationMode union type in src/store/types.ts is the right approach. TypeScript strict mode will catch most missing branches at compile time.

2. Backward compatibility is solid: isGitRepo?: boolean on Project defaults to undefined (treated as true via !== false checks), so existing projects are unaffected. The persistence layer at src/store/persistence.ts reads gitIsolation as a string from JSON and the 'none' value round-trips correctly.

3. Backend safety: The ensureStepsIgnored() function in electron/ipc/steps.ts already handles missing .git gracefully (returns null from getGitExcludePath). The ensurePlansDirectory() in electron/ipc/plans.ts only touches .claude/ settings, not .git. No backend changes were needed, and none were made — correct.

4. Guard in refreshTaskGitStatus (src/store/taskStatus.ts): Adding the task.gitIsolation === 'none' early return prevents the backend from attempting git commands (via IPC.GetWorktreeStatus) on a non-git folder. This prevents errors.

5. Guard in getTaskDotStatus: Returning 'waiting' for 'none' tasks before the git status check is correct. The placement after the hasActive and steps-review checks means busy/review states still work for non-git tasks.

6. Multiple 'none' tasks per project: hasDirectTask() only checks for 'direct' mode, correctly allowing multiple non-git tasks on the same project simultaneously.

7. mergeTask and pushTask guards: Changed from === 'direct' to !== 'worktree', which correctly blocks both 'direct' and 'none' modes from attempting git merge/push operations.

Issues Found

Medium Severity

1. NewTaskDialog.tsx — Misplaced onCleanup inside early return path (lines 273-275 of diff)

if (!path || !isGit) {
  setIgnoredDirs([]);
  setSelectedDirs(new Set<string>());
  onCleanup(() => {
    cancelled = true;
  });
  return;
}

In SolidJS, onCleanup registers a disposal callback for the current reactive scope (the createEffect). Calling it conditionally inside an early-return path means it only registers when the early-return is taken, but the cancelled variable is declared outside this block and used in the async continuation below. This works because when the effect re-runs (and takes the early-return), SolidJS disposes the previous scope anyway. However, this is an unusual pattern — the onCleanup(() => { cancelled = true; }) at the bottom of the effect (which already existed before this PR) is the conventional SolidJS approach and handles all cases. The added onCleanup in the early return is redundant but not harmful — it just registers a cleanup for a scope that has no async work to cancel. Consider removing it for clarity, or leave it if you prefer defensive coding.

Low Severity

2. TaskBranchInfoBar.tsx — "Open in editor" button hidden for non-git tasks

The branch icon button (git branch icon + branch name) is wrapped in <Show when={props.task.gitIsolation !== 'none'}>. This hides both the branch name display and the "open in editor" click handler for that specific button. However, the folder path button (second button at the bottom) still provides the same handleOpenInEditor functionality, so users can still open the project in their editor. This is a UI design choice rather than a bug — just noting that non-git users lose one of the two "open in editor" click targets.

3. Missing test plan items

The PR test plan has several unchecked items:

• Agents spawn and run in the project folder
• Add a git folder — all git UI still works as before
• Persistence: close and reopen app, non-git project and tasks restore correctly
• Edit non-git project settings — git settings are hidden

It would be good to confirm these before merging.

4. EditProjectDialog.tsx — defaultGitIsolation still saved for non-git projects

When editing a non-git project, the git isolation selector is hidden, but handleSave() still persists defaultGitIsolation: defaultGitIsolation() (which defaults to 'worktree'). This is harmless (the value is inert for non-git projects and could be useful if the folder later becomes a git repo), but worth noting for awareness.

Informational

5. Potential merge conflict with taskStatus.ts

Commit cabbc6b (Copilot support) modified src/store/taskStatus.ts after the PR's base commit (b48ecca). The diff context around getTaskDotStatus may produce a merge conflict depending on how close the changes are. The logical placement of the 'none' guard is correct regardless.

6. isGitRepo detection is point-in-time only

The isGitRepo flag is set once at pickAndAddProject() / relinkProject() time and stored on the project. If a user later runs git init in the folder (or conversely, deletes .git), the flag becomes stale. This is an acceptable trade-off for a v1 — re-detection could be added later if needed. Users can relink the project to update the flag.

Security

• IPC validation: CheckIsGitRepo handler in electron/ipc/register.ts already validates the path with validatePath(args.path, 'path') before calling isGitRepo(). No new IPC channels are introduced.
• No path injection risks: The worktreePath for 'none' tasks is set to projectRoot (the validated project path), same as 'direct' mode.
• No new any types: All new code respects strict: true TypeScript.

SolidJS Patterns

• <Show> guards are used correctly throughout for conditional rendering.
• Signals and reactive reads (projectIsGitRepo(pid) inside createEffect) are properly tracked.
• The notesPanelContent extraction in TaskNotesPanel.tsx (converting inline JSX to a function that returns JSX) is a clean way to reuse the notes panel between the ResizablePanel path and the 'none' fallback path without duplicating code.
• No anti-patterns detected (no createEffect without dependencies, no untracked reads).

Summary

This is a solid contribution. The approach is clean, backward compatible, and handles the major code paths correctly. The only actionable item is the potentially redundant onCleanup in the early return (medium severity), which is harmless but worth cleaning up. The remaining items are minor or informational.

Recommendation: Approve after confirming the unchecked test plan items and optionally cleaning up the redundant onCleanup.

Generated by Claude Code

Generated by Claude Code

Thank you very much! <3