-
Notifications
You must be signed in to change notification settings - Fork 3.3k
feat(web): group sidebar v2 by worktree and scope panes to the checkout #4521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ import { | |
| OrchestrationDispatchCommandError, | ||
| type OrchestrationEvent, | ||
| type OrchestrationShellStreamEvent, | ||
| type OrchestrationThreadShell, | ||
| type OrchestrationShellStreamItem, | ||
| type OrchestrationThreadStreamItem, | ||
| OrchestrationGetFullThreadDiffError, | ||
|
|
@@ -1100,6 +1101,13 @@ const makeWsRpcLayer = ( | |
| }; | ||
| }); | ||
|
|
||
| // Mirrors the client's worktree identity rule (worktreeCleanup.ts): | ||
| // blank and null both mean "the project's local checkout". | ||
| const normalizeWorktreePathForArchive = (worktreePath: string | null): string | null => { | ||
| const trimmed = worktreePath?.trim(); | ||
| return trimmed !== undefined && trimmed.length > 0 ? trimmed : null; | ||
| }; | ||
|
|
||
| const refreshGitStatus = (cwd: string) => | ||
| vcsStatusBroadcaster | ||
| .refreshStatus(cwd) | ||
|
|
@@ -1111,21 +1119,17 @@ const makeWsRpcLayer = ( | |
| ORCHESTRATION_WS_METHODS.dispatchCommand, | ||
| Effect.gen(function* () { | ||
| const normalizedCommand = yield* normalizeDispatchCommand(command); | ||
| const shouldStopSessionAfterArchive = | ||
| const threadShellBeforeArchive = | ||
| normalizedCommand.type === "thread.archive" | ||
| ? yield* projectionSnapshotQuery | ||
| .getThreadShellById(normalizedCommand.threadId) | ||
| .pipe( | ||
| Effect.map( | ||
| Option.match({ | ||
| onNone: () => false, | ||
| onSome: (thread) => | ||
| thread.session !== null && thread.session.status !== "stopped", | ||
| }), | ||
| ), | ||
| Effect.orElseSucceed(() => false), | ||
| ) | ||
| : false; | ||
| .pipe(Effect.orElseSucceed(() => Option.none<OrchestrationThreadShell>())) | ||
| : Option.none<OrchestrationThreadShell>(); | ||
| const shouldStopSessionAfterArchive = Option.match(threadShellBeforeArchive, { | ||
| onNone: () => false, | ||
| onSome: (thread) => | ||
| thread.session !== null && thread.session.status !== "stopped", | ||
| }); | ||
| const result = yield* dispatchNormalizedCommand(normalizedCommand); | ||
| if (normalizedCommand.type === "thread.archive") { | ||
| if (shouldStopSessionAfterArchive) { | ||
|
|
@@ -1150,14 +1154,36 @@ const makeWsRpcLayer = ( | |
| ); | ||
| } | ||
|
|
||
| yield* terminalManager.close({ threadId: normalizedCommand.threadId }).pipe( | ||
| Effect.catch((error) => | ||
| Effect.logWarning("failed to close thread terminals after archive", { | ||
| threadId: normalizedCommand.threadId, | ||
| error: error.message, | ||
| }), | ||
| ), | ||
| ); | ||
| // Terminals are worktree-scoped on the client (every thread in | ||
| // a checkout shares one set of ptys via a canonical thread id), | ||
| // so archiving one thread must not kill sessions that sibling | ||
| // threads still use — e.g. a running dev server. | ||
| const worktreeStillInUse = yield* Option.match(threadShellBeforeArchive, { | ||
| onNone: () => Effect.succeed(false), | ||
| onSome: (archivedThread) => | ||
| projectionSnapshotQuery.getShellSnapshot().pipe( | ||
| Effect.map((snapshot) => | ||
| snapshot.threads.some( | ||
| (thread) => | ||
| thread.id !== archivedThread.id && | ||
| thread.projectId === archivedThread.projectId && | ||
| normalizeWorktreePathForArchive(thread.worktreePath) === | ||
| normalizeWorktreePathForArchive(archivedThread.worktreePath), | ||
| ), | ||
| ), | ||
| Effect.orElseSucceed(() => false), | ||
| ), | ||
| }); | ||
| if (!worktreeStillInUse) { | ||
| yield* terminalManager.close({ threadId: normalizedCommand.threadId }).pipe( | ||
| Effect.catch((error) => | ||
| Effect.logWarning("failed to close thread terminals after archive", { | ||
| threadId: normalizedCommand.threadId, | ||
| error: error.message, | ||
| }), | ||
| ), | ||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Archive closes wrong terminal ownerHigh Severity Terminal closure and UI state clearing logic incorrectly targets the specific archived or deleted thread ID. Client terminal sessions, however, are shared across a worktree and associated with a canonical thread ID. This mismatch can lead to PTY and dev server leaks when a worktree is fully parked, as the canonical session may not be closed. It can also cause unintended UI state resets for other active threads in the same worktree. Reviewed by Cursor Bugbot for commit 48a7020. Configure here. |
||
| } | ||
| } | ||
| return result; | ||
| }).pipe( | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the final unarchived thread in a checkout is archived after any sibling was archived earlier, shell snapshots still contain that earlier thread with a non-null
archivedAt, and this predicate counts it as continued use. The close branch is therefore skipped, leaving the canonical checkout PTYs and subprocesses—including dev servers—running indefinitely with no live thread from which to access them. This check must consider only non-archived siblings and, when none remain, close the checkout's canonical terminal owner rather than assuming the just-archived thread owns the sessions.Useful? React with 👍 / 👎.