Repository navigation
fix(runtime): stop re-seeding a runtime-owned workspace the user emptied #18513
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
Closed
nwparker
wants to merge
9
commits into
main
from
neil/sta-6173-bug-focusing-a-runtime-owned-workspace-seeds-a-terminal
Closed
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
455a74f
fix(runtime): stop re-seeding a runtime-owned workspace the user emptied
nwparker 53c5932
fix(runtime): harden the runtime-owned initial-terminal bootstrap latch
nwparker 3a62734
fix(runtime): key the initial-terminal bootstrap latch per environment
github-actions[bot] 0b7b161
fix(runtime): release the bootstrap latch when the create returns failed
nwparker 4f775d6
fix(runtime): release a parked bootstrap on the mirror's next frame
nwparker 6e57084
Merge remote-tracking branch 'origin/main' into pr18513
nwparker d02ac37
fix(runtime): let a failed create retry inside its own subscription
nwparker 7620a8e
fix(runtime): close the second re-seed door on an emptied workspace
nwparker ae38bb1
fix(runtime): stop the worktree-id re-key inventing a tombstone
nwparker File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| /** | ||
| * Re-keying a workspace's session rows must not invent state the user never created. | ||
| * | ||
| * `tabsByWorktree` carries three states, not two: an absent row is "never initialized" and an | ||
| * explicit empty row is the closed-last-terminal tombstone that `shouldAutoCreateInitialTerminal` | ||
| * honours. Writing `[]` for a source that had no row turns the first into the second, and the | ||
| * workspace then never gets its initial terminal. | ||
| */ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { getDefaultWorkspaceSession } from '../../shared/constants' | ||
| import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' | ||
| import { canonicalizeTerminalSessionWorktreeId } from './workspace-session-worktree-id' | ||
|
|
||
| const SOURCE = 'repo-1::/old/path' | ||
| const TARGET = 'repo-1::/new/path' | ||
|
|
||
| function tab(id: string, worktreeId: string) { | ||
| return { | ||
| id, | ||
| ptyId: `pty-${id}`, | ||
| worktreeId, | ||
| title: id, | ||
| customTitle: null, | ||
| color: null, | ||
| sortOrder: 0, | ||
| createdAt: 1 | ||
| } | ||
| } | ||
|
|
||
| function session(overrides: Partial<WorkspaceSessionState> = {}): WorkspaceSessionState { | ||
| return { ...getDefaultWorkspaceSession(), ...overrides } | ||
| } | ||
|
|
||
| describe('canonicalizeTerminalSessionWorktreeId', () => { | ||
| it('does not fabricate a tombstone for a workspace that had no tabs row', () => { | ||
| const next = session({ tabsByWorktree: {} }) | ||
|
|
||
| canonicalizeTerminalSessionWorktreeId(next, SOURCE, TARGET) | ||
|
|
||
| expect(Object.hasOwn(next.tabsByWorktree, TARGET)).toBe(false) | ||
| }) | ||
|
|
||
| it('carries a real closed-last-terminal tombstone across the re-key', () => { | ||
| // The other direction: an explicit empty row is user intent and must survive, so the guard has | ||
| // to be about the source row's presence and not about the tabs being empty. | ||
| const next = session({ tabsByWorktree: { [SOURCE]: [] } }) | ||
|
|
||
| canonicalizeTerminalSessionWorktreeId(next, SOURCE, TARGET) | ||
|
|
||
| expect(Object.hasOwn(next.tabsByWorktree, TARGET)).toBe(true) | ||
| expect(next.tabsByWorktree[TARGET]).toEqual([]) | ||
| expect(Object.hasOwn(next.tabsByWorktree, SOURCE)).toBe(false) | ||
| }) | ||
|
|
||
| it('re-keys the tabs and their own worktree ids', () => { | ||
| const next = session({ tabsByWorktree: { [SOURCE]: [tab('tab-1', SOURCE)] } }) | ||
|
|
||
| canonicalizeTerminalSessionWorktreeId(next, SOURCE, TARGET) | ||
|
|
||
| expect(next.tabsByWorktree[TARGET]?.map((entry) => entry.worktreeId)).toEqual([TARGET]) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
52 changes: 52 additions & 0 deletions
52
src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import { useAppStore } from '../store' | ||
| import { createWebRuntimeSessionTerminal } from './web-runtime-session' | ||
| import type { WebRuntimeTerminalCreateOutcome } from './web-runtime-session-types' | ||
| import { | ||
| beginWebRuntimeInitialTerminalBootstrap, | ||
| endWebRuntimeInitialTerminalBootstrap, | ||
| markWebRuntimeInitialTerminalBootstrapAwaitingMirror | ||
| } from './web-runtime-initial-terminal-bootstrap' | ||
|
|
||
| /** | ||
| * Claim the initial-terminal latch for this environment's worktree, create the terminal, and decide | ||
| * how the latch is released. | ||
| * | ||
| * A thrown or returned failure releases it at once: nothing was created, so a later focus may retry. | ||
| * A success whose mirrored `tabsByWorktree` row already exists releases too. A success with no row | ||
| * yet is neither: the refresh the create awaits can resolve on an empty, unconfirmed frame while the | ||
| * host does hold the tab, and releasing there let the next empty frame seed a duplicate. That case | ||
| * is parked as awaiting-mirror and released by the next frame the mirror accepts for the worktree | ||
| * (see web-runtime-initial-terminal-bootstrap.ts). | ||
| * | ||
| * Returns true only when this call owned a create that was not reported as failed, because the | ||
| * caller latches a closure-local flag on it. A failure must report false whichever way it arrives: | ||
| * releasing the shared latch alone still leaves the subscription that issued the failed create | ||
| * unable to retry for as long as its closure lives, which is the same suppression one level up. | ||
| */ | ||
| export async function dispatchWebRuntimeInitialTerminalBootstrap( | ||
| environmentId: string, | ||
| worktreeId: string | ||
| ): Promise<boolean> { | ||
| if (!beginWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId)) { | ||
| return false | ||
| } | ||
| let outcome: WebRuntimeTerminalCreateOutcome | ||
| try { | ||
| outcome = await createWebRuntimeSessionTerminal({ worktreeId, environmentId, activate: true }) | ||
| } catch (error) { | ||
| endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) | ||
| throw error | ||
| } | ||
| // Why check the outcome: the create reports RPC and network failures as `{ status: 'failed' }` | ||
| // rather than throwing, so the catch above never sees them. Both arms report the same way. | ||
| if (outcome.status === 'failed') { | ||
| endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) | ||
| return false | ||
| } | ||
| if (Object.hasOwn(useAppStore.getState().tabsByWorktree, worktreeId)) { | ||
| endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) | ||
| } else { | ||
| markWebRuntimeInitialTerminalBootstrapAwaitingMirror(environmentId, worktreeId) | ||
| } | ||
| return true | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.