Skip to content

Don't apply the worktree default when resuming a session - #31

Merged
JeanBaptisteRenard merged 1 commit into
devsuitup:mainfrom
ymajoros:fix/no-worktree-on-resume-jbr
Jun 4, 2026
Merged

Don't apply the worktree default when resuming a session#31
JeanBaptisteRenard merged 1 commit into
devsuitup:mainfrom
ymajoros:fix/no-worktree-on-resume-jbr

Conversation

@ymajoros

@ymajoros ymajoros commented Jun 1, 2026

Copy link
Copy Markdown

Problem

A plain click on a session in the sidebar does nothing — no terminal appears, the session stays "inactive" — yet Resume with config works. So a session can only be interacted with through the config dialog.

Cause

openSession() resumes with resolveDefaultSessionOptions(), which is shared with new-session creation and includes the global worktree: true default. Resuming an existing session with --worktree tries to spin it up in a fresh git worktree and fails to attach, so the terminal never shows.

showResumeSessionDialog() has no worktree control and never sets the flag — which is exactly why "Resume with config" works. Plain click and the dialog should behave the same.

Fix

Strip worktree/worktreeName from the resolved options on the resume path in openSession():

const resumeOptions = customOptions || await resolveDefaultSessionOptions({ projectPath });
if (resumeOptions) { delete resumeOptions.worktree; delete resumeOptions.worktreeName; }

New-session creation is untouched (still honors the worktree default). Worktree is a new-session concept — resuming must reuse the session's existing directory.

Testing

Built locally (Electron 41, Linux) with worktree: true as the default: plain click now resumes in place and attaches, matching the dialog. New-session-in-worktree still works.

@JeanBaptisteRenard JeanBaptisteRenard left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @ymajoros! Good diagnosis — resolveDefaultSessionOptions() was indeed designed for new-session creation, and the global worktree: true default leaking into resume is exactly what broke the plain sidebar click. The fix works for that path.

Test suite passes locally on your branch: 101/101 pass.

One gap before merge:

The schedule-creator resume path has the same bug (required)

launchScheduleCreator in public/dialogs.js also calls resolveDefaultSessionOptions(project) (line 33) and then passes the result unstripped to window.api.openTerminal(result.sessionId, project.projectPath, false, options) (line 68) — isNew=false, so it's a resume, and main.js appends the worktree flag without checking isNew (the guard at ~1465 only switches the command form). So "Create scheduled task" still hits the worktree-on-resume bug; your fix in app.js (openSession) is never reached on that path.

Suggestion: rather than deleting the keys at each resume call site, strip them once in a shared helper (e.g. a resolveResumeSessionOptions() next to resolveDefaultSessionOptions()), or have main.js ignore the worktree option when isNew === false — that closes every current and future resume path at the source.

Minor

  • delete resumeOptions.worktree mutates the caller's customOptions in place. No current caller is harmed, but a shallow copy ({ ...customOptions }) is safer against future reuse.
  • No test — the jsdom harness under test/ (see test/dom-setup.js) makes this unit-testable: assert that openTerminal is called without worktree when resuming. Would make a nice regression guard, especially if you go the shared-helper route.

Verdict: request changes — only for the dialogs.js path. Thanks again for the three PRs, much appreciated! 🙏

@ymajoros
ymajoros force-pushed the fix/no-worktree-on-resume-jbr branch from 5d90f28 to 2f56c4e Compare June 4, 2026 13:02
--worktree creates a fresh isolated git worktree, which only makes sense when
STARTING a session. On resume (isNew === false) it makes claude try to spin up
a new worktree and fail to attach — so a plain sidebar click, and equally the
"Create scheduled task" flow (launchScheduleCreator also resumes via
openTerminal with isNew=false), silently broke.

Rather than stripping the option at each renderer call site, gate it in the
open-terminal handler: only append --worktree when isNew. That closes every
current and future resume path in one place, and avoids mutating the caller's
options object.
@ymajoros
ymajoros force-pushed the fix/no-worktree-on-resume-jbr branch from 2f56c4e to 5384778 Compare June 4, 2026 13:05
@ymajoros

ymajoros commented Jun 4, 2026

Copy link
Copy Markdown
Author

Thanks! Reworked per your suggestion — force-pushed (also rebased onto current main).

Moved the fix to the source: the open-terminal handler now only appends --worktree when isNew, so the worktree option is ignored on every resume path — the sidebar click, the launchScheduleCreator ("Create scheduled task") flow you flagged, fork, and any future caller — in one place. The diff is now just that one-line guard in main.js; the per-call-site delete in app.js is gone, so the customOptions-mutation concern is moot too.

On the test: with the logic now in the main-process command-building (not the renderer), the jsdom harness doesn't reach it, and there's no clean unit seam without spawning a PTY — so I left it out. Happy to add one if you'd prefer a specific approach.

@JeanBaptisteRenard JeanBaptisteRenard left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed the rework — fixing it at the source is exactly right, and the execution is clean:

  • isNew && sessionOptions.worktree guard covers every resume path in one place (sidebar click, schedule creator, fork, future callers) — verified all four openTerminal call sites
  • ✅ Resume of a session originally created in a worktree is unaffected: the PTY cwd always comes from the stored projectPath, the flag only ever mattered at creation
  • ✅ The app.js delete-hack is gone — nice and minimal now

On the no-test question: I checked the fork's conventions — main.js exports nothing and no IPC handler has a runtime unit test (existing tests treat it as static text only), so requiring one here would exceed the local bar. Fine as-is.

Local run: 119/119 tests pass, CI green on Node 20/22.

Merging — thanks for the rework and for PR #30's honest self-close, much appreciated! 🙏

@JeanBaptisteRenard
JeanBaptisteRenard merged commit dec59bd into devsuitup:main Jun 4, 2026
7 checks passed
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