fix: review-bot findings on the provider-hosted shell
- Keep ConversationSession mounted for blank sessions (chrome-less) so the draft-persistence mirror stays bound in the hero; hero typing reaches the chat store again. - Restore the baselines-ready gate in AppFrame: empty boot snapshots no longer flash the New Workspace hero before either baseline lands. - Commit ordinary sends through the machine (send-committed event + Shell.commitSend): undo can no longer resurrect already-sent content on the default-sink path. - Give the production InputMachine a real wall clock so the typing-run merge window actually expires. - Coalesce concurrent connectWorkspace creates per workspace: the summary has no cwd until the host frame lands, so a second New Session inside that window minted a duplicate hidden blank session.
This commit is contained in:
@@ -27,6 +27,10 @@ export class WorkspacesService {
|
|||||||
readonly list: SnapshotStore<WorkspaceListState>
|
readonly list: SnapshotStore<WorkspaceListState>
|
||||||
/** Workspace baseline and frame owner. */
|
/** Workspace baseline and frame owner. */
|
||||||
private readonly manager: WorkspaceManager
|
private readonly manager: WorkspaceManager
|
||||||
|
/** In-flight blank-session creates keyed by workspace (connectWorkspace coalescing). */
|
||||||
|
private readonly connecting = new Map<WorkspaceId, Promise<SessionId>>()
|
||||||
|
/** Guards the runtime-owned one-shot initial-selection subscription. */
|
||||||
|
private initialSelectionStarted = false
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @param ctx - client root context.
|
* @param ctx - client root context.
|
||||||
@@ -59,6 +63,11 @@ export class WorkspacesService {
|
|||||||
async connectWorkspace(workspaceId: WorkspaceId): Promise<SessionId> {
|
async connectWorkspace(workspaceId: WorkspaceId): Promise<SessionId> {
|
||||||
const workspace = this.list.getSnapshot().items.find(item => item.workspaceId === workspaceId)
|
const workspace = this.list.getSnapshot().items.find(item => item.workspaceId === workspaceId)
|
||||||
if (workspace === undefined) throw new Error(`workspaces.connectWorkspace: unknown workspace ${workspaceId}`)
|
if (workspace === undefined) throw new Error(`workspaces.connectWorkspace: unknown workspace ${workspaceId}`)
|
||||||
|
// Coalesce concurrent connects: a create's summary lands without cwd
|
||||||
|
// until the host frame arrives, so a second call inside that window
|
||||||
|
// would miss the reuse scan and mint another hidden blank session.
|
||||||
|
const inflight = this.connecting.get(workspaceId)
|
||||||
|
if (inflight !== undefined) return inflight
|
||||||
// Reuse: blank && same canonical cwd (workspace.path is the host realpath
|
// Reuse: blank && same canonical cwd (workspace.path is the host realpath
|
||||||
// canon; summary cwd is the session header passthrough of the same canon).
|
// canon; summary cwd is the session header passthrough of the same canon).
|
||||||
const sessions = this.sessions.list.getSnapshot()
|
const sessions = this.sessions.list.getSnapshot()
|
||||||
@@ -66,7 +75,59 @@ export class WorkspacesService {
|
|||||||
const summary = sessions.byId[id]
|
const summary = sessions.byId[id]
|
||||||
if (summary !== undefined && summary.blank && summary.cwd === workspace.path) return summary.id
|
if (summary !== undefined && summary.blank && summary.cwd === workspace.path) return summary.id
|
||||||
}
|
}
|
||||||
return this.sessions.create({ workspaceId })
|
const attempt = this.sessions.create({ workspaceId })
|
||||||
|
.finally(() => { this.connecting.delete(workspaceId) })
|
||||||
|
this.connecting.set(workspaceId, attempt)
|
||||||
|
return attempt
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Follow the first complete Workspace/Session baseline and select a default
|
||||||
|
* session exactly once. A restored current session wins; otherwise the most
|
||||||
|
* recent Workspace is connected (reusing or creating its blank session).
|
||||||
|
* Later explicit clears stay cleared instead of retriggering this startup
|
||||||
|
* policy. A failed connect may retry on the next baseline projection.
|
||||||
|
* @returns disposer for the baseline subscription; late work cannot navigate after disposal.
|
||||||
|
*/
|
||||||
|
startInitialSelection(): () => void {
|
||||||
|
if (this.initialSelectionStarted) {
|
||||||
|
throw new Error('workspaces.startInitialSelection: already started')
|
||||||
|
}
|
||||||
|
this.initialSelectionStarted = true
|
||||||
|
let state: 'waiting' | 'connecting' | 'done' = 'waiting'
|
||||||
|
let disposed = false
|
||||||
|
const reconcile = (): void => {
|
||||||
|
if (disposed || state !== 'waiting') return
|
||||||
|
const workspace = this.list.getSnapshot()
|
||||||
|
if (!workspace.baselinesReady) return
|
||||||
|
const current = this.sessions.list.getSnapshot().current
|
||||||
|
const target = workspace.recentWorkspaceId
|
||||||
|
if (current !== undefined || target === undefined) {
|
||||||
|
state = 'done'
|
||||||
|
return
|
||||||
|
}
|
||||||
|
state = 'connecting'
|
||||||
|
void this.connectWorkspace(target).then(
|
||||||
|
(sessionId) => {
|
||||||
|
if (disposed) return
|
||||||
|
if (this.sessions.list.getSnapshot().current === undefined) {
|
||||||
|
this.sessions.open(sessionId)
|
||||||
|
}
|
||||||
|
state = 'done'
|
||||||
|
},
|
||||||
|
(reason: unknown) => {
|
||||||
|
if (disposed) return
|
||||||
|
state = 'waiting'
|
||||||
|
console.warn('initial workspace selection failed:', reason)
|
||||||
|
},
|
||||||
|
)
|
||||||
|
}
|
||||||
|
const unsubscribe = this.list.subscribe(reconcile)
|
||||||
|
reconcile()
|
||||||
|
return () => {
|
||||||
|
disposed = true
|
||||||
|
unsubscribe()
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -250,6 +250,8 @@ export type InputEvent =
|
|||||||
| { readonly type: 'adjudicated'; readonly attempt: SubmitAttempt; readonly outcome: PickOutcome }
|
| { readonly type: 'adjudicated'; readonly attempt: SubmitAttempt; readonly outcome: PickOutcome }
|
||||||
| { readonly type: 'adjudication-failed'; readonly attempt: SubmitAttempt; readonly message: string }
|
| { readonly type: 'adjudication-failed'; readonly attempt: SubmitAttempt; readonly message: string }
|
||||||
| { readonly type: 'submit-settled'; readonly attempt: SubmitAttempt; readonly ok: boolean; readonly outcome?: SubmitOutcome; readonly message?: string }
|
| { readonly type: 'submit-settled'; readonly attempt: SubmitAttempt; readonly ok: boolean; readonly outcome?: SubmitOutcome; readonly message?: string }
|
||||||
|
/** An ordinary (default-sink) send was accepted: clear the draft as a COMMIT — undo must not resurrect sent content (mirrors the command submit-settled success arm). */
|
||||||
|
| { readonly type: 'send-committed' }
|
||||||
| { readonly type: 'release' }
|
| { readonly type: 'release' }
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -71,7 +71,9 @@ export class SessionInputShell implements SessionInput {
|
|||||||
submit: (mode) => { this.submit(mode) },
|
submit: (mode) => { this.submit(mode) },
|
||||||
}
|
}
|
||||||
|
|
||||||
private readonly core = new InputMachine()
|
// Real wall clock: the typing-run merge window must actually expire in
|
||||||
|
// production (the machine's no-clock default is a constant for pure tests).
|
||||||
|
private readonly core = new InputMachine({ now: () => Date.now() })
|
||||||
private noticeSeq = 0
|
private noticeSeq = 0
|
||||||
private lastDraft = ''
|
private lastDraft = ''
|
||||||
private disposed = false
|
private disposed = false
|
||||||
@@ -95,6 +97,15 @@ export class SessionInputShell implements SessionInput {
|
|||||||
this.run(this.core.dispatch({ type: 'draft-changed', draft: text, ...(editRange !== undefined ? { editRange } : {}) }))
|
this.run(this.core.dispatch({ type: 'draft-changed', draft: text, ...(editRange !== undefined ? { editRange } : {}) }))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Clear the draft as a successful-send commit: no undo unit is recorded and
|
||||||
|
* the undo history is cut, so Ctrl/Cmd-Z cannot resurrect sent content
|
||||||
|
* (the command path gets the same discipline from submit-settled success).
|
||||||
|
*/
|
||||||
|
commitSend(): void {
|
||||||
|
this.run(this.core.dispatch({ type: 'send-committed' }))
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Insert a newline at the selection as one machine transaction (the
|
* Insert a newline at the selection as one machine transaction (the
|
||||||
* execCommand path is gone — a second undo history would fork).
|
* execCommand path is gone — a second undo history would fork).
|
||||||
|
|||||||
@@ -116,7 +116,8 @@ export class InputHub implements InputService {
|
|||||||
private sink(session: Session, text: string, mode: 'queue' | 'steer'): void {
|
private sink(session: Session, text: string, mode: 'queue' | 'steer'): void {
|
||||||
if (text === '') return
|
if (text === '') return
|
||||||
const shell = this.shells.get(session.sessionId)
|
const shell = this.shells.get(session.sessionId)
|
||||||
shell?.setDraft('')
|
// Commit, not an editable clear: undo must not resurrect sent content.
|
||||||
|
shell?.commitSend()
|
||||||
void session.prompt([{ type: 'text', text }], mode).then(
|
void session.prompt([{ type: 'text', text }], mode).then(
|
||||||
(result) => {
|
(result) => {
|
||||||
if (!result.ok && shell?.snapshot.draft === '') shell.setDraft(text)
|
if (!result.ok && shell?.snapshot.draft === '') shell.setDraft(text)
|
||||||
|
|||||||
@@ -167,6 +167,7 @@ export class InputMachine {
|
|||||||
case 'adjudicated': return this.onAdjudicated(ev.attempt, ev.outcome)
|
case 'adjudicated': return this.onAdjudicated(ev.attempt, ev.outcome)
|
||||||
case 'adjudication-failed': return this.onAdjudicationFailed(ev.attempt, ev.message)
|
case 'adjudication-failed': return this.onAdjudicationFailed(ev.attempt, ev.message)
|
||||||
case 'submit-settled': return this.onSubmitSettled(ev)
|
case 'submit-settled': return this.onSubmitSettled(ev)
|
||||||
|
case 'send-committed': return this.onSendCommitted()
|
||||||
case 'release': return this.onRelease()
|
case 'release': return this.onRelease()
|
||||||
default: return unreachable(ev)
|
default: return unreachable(ev)
|
||||||
}
|
}
|
||||||
@@ -542,6 +543,19 @@ export class InputMachine {
|
|||||||
return [{ type: 'notice', level: 'error', text }]
|
return [{ type: 'notice', level: 'error', text }]
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Ordinary send accepted: clear as a commit (no undo unit; sent content
|
||||||
|
* must not be resurrectable — same discipline as submit-settled success). */
|
||||||
|
private onSendCommitted(): InputEffect[] {
|
||||||
|
this.claim = undefined
|
||||||
|
this.occurrences = []
|
||||||
|
this.adopt('')
|
||||||
|
this.log = []
|
||||||
|
this.redoStack = []
|
||||||
|
this.typingRun = undefined
|
||||||
|
this.paste = undefined
|
||||||
|
return []
|
||||||
|
}
|
||||||
|
|
||||||
private onRelease(): InputEffect[] {
|
private onRelease(): InputEffect[] {
|
||||||
if (this.inflight !== undefined) {
|
if (this.inflight !== undefined) {
|
||||||
this.inflight.controller.abort()
|
this.inflight.controller.abort()
|
||||||
|
|||||||
@@ -77,7 +77,11 @@ export function ConversationRoot({
|
|||||||
|
|
||||||
return (
|
return (
|
||||||
<div className={css.root} data-phase={hero ? 'hero' : 'active'}>
|
<div className={css.root} data-phase={hero ? 'hero' : 'active'}>
|
||||||
{!hero && renderSlot('conversation.session', {})}
|
{/* Mounted for every real session, hero included: ConversationSession
|
||||||
|
renders no chrome while blank but owns the draft-persistence mirror
|
||||||
|
bind — unmounting it in the hero would lose pre-first-send text on
|
||||||
|
a refresh or scope rebuild. */}
|
||||||
|
{sessionId !== undefined && renderSlot('conversation.session', {})}
|
||||||
{renderSlotChain(
|
{renderSlotChain(
|
||||||
'conversation.composer',
|
'conversation.composer',
|
||||||
{ interactions: pending },
|
{ interactions: pending },
|
||||||
|
|||||||
@@ -162,12 +162,12 @@ describe('ConversationRoot resident composer', () => {
|
|||||||
// Hero chrome present, view ring absent.
|
// Hero chrome present, view ring absent.
|
||||||
expect(b.view.getByText("Let's start building")).toBeTruthy()
|
expect(b.view.getByText("Let's start building")).toBeTruthy()
|
||||||
expect(b.view.queryByTestId('view-chat')).toBeNull()
|
expect(b.view.queryByTestId('view-chat')).toBeNull()
|
||||||
// The same machine-backed textarea is live in the hero. The chat-store
|
// The same machine-backed textarea is live in the hero, and the
|
||||||
// mirror binds with ConversationSession (unmounted in hero), so the
|
// persistence mirror stays bound (ConversationSession mounts chrome-less
|
||||||
// draft's truth here is the machine itself.
|
// for blank sessions): hero typing reaches the chat store.
|
||||||
const box = b.view.getByRole('textbox')
|
const box = b.view.getByRole('textbox')
|
||||||
fireEvent.change(box, { target: { value: 'draft in hero' } })
|
fireEvent.change(box, { target: { value: 'draft in hero' } })
|
||||||
expect((box as HTMLTextAreaElement).value).toBe('draft in hero')
|
expect(b.chat.store.getSnapshot().draft).toBe('draft in hero')
|
||||||
// Picker: open through the chip; a pick switches to the other
|
// Picker: open through the chip; a pick switches to the other
|
||||||
// workspace's blank session (draft carry is apply-layer wiring).
|
// workspace's blank session (draft carry is apply-layer wiring).
|
||||||
fireEvent.click(b.view.getByRole('button', { name: 'Choose workspace' }))
|
fireEvent.click(b.view.getByRole('button', { name: 'Choose workspace' }))
|
||||||
|
|||||||
@@ -85,7 +85,12 @@ export function AppFrame({
|
|||||||
useStore,
|
useStore,
|
||||||
actions,
|
actions,
|
||||||
renderSlot,
|
renderSlot,
|
||||||
|
useWorkspaces,
|
||||||
}: AppFrameProps) {
|
}: AppFrameProps) {
|
||||||
|
// Baseline gate: before both object-layer baselines land, empty snapshots
|
||||||
|
// are indistinguishable from a genuine no-session state — rendering the
|
||||||
|
// conversation shell then would flash the New Workspace hero on boot.
|
||||||
|
const baselinesReady = useWorkspaces(s => s.baselinesReady)
|
||||||
const panels = useStore((s) => s)
|
const panels = useStore((s) => s)
|
||||||
const frameRef = useRef<HTMLDivElement | null>(null)
|
const frameRef = useRef<HTMLDivElement | null>(null)
|
||||||
const [viewport, setViewport] = useState(() => window.innerWidth)
|
const [viewport, setViewport] = useState(() => window.innerWidth)
|
||||||
@@ -151,13 +156,24 @@ export function AppFrame({
|
|||||||
width: cols.sidebar,
|
width: cols.sidebar,
|
||||||
})}
|
})}
|
||||||
</div>
|
</div>
|
||||||
<>
|
{baselinesReady
|
||||||
{/* Both column occupants stay at fixed tree positions. The
|
? (
|
||||||
conversation is session-maybe; the strict details entry
|
<>
|
||||||
naturally renders empty while no session is current. */}
|
{/* Both column occupants stay at fixed tree positions. The
|
||||||
<CenterColumn>{renderSlot('conversation', {})}</CenterColumn>
|
conversation is session-maybe; the strict details entry
|
||||||
<DetailsColumn>{renderSlot('details', {})}</DetailsColumn>
|
naturally renders empty while no session is current. */}
|
||||||
</>
|
<CenterColumn>{renderSlot('conversation', {})}</CenterColumn>
|
||||||
|
<DetailsColumn>{renderSlot('details', {})}</DetailsColumn>
|
||||||
|
</>
|
||||||
|
)
|
||||||
|
: (
|
||||||
|
<>
|
||||||
|
<CenterColumn>
|
||||||
|
<div role="status">Loading workspaces and sessions…</div>
|
||||||
|
</CenterColumn>
|
||||||
|
<DetailsColumn />
|
||||||
|
</>
|
||||||
|
)}
|
||||||
{/* The collapsed rail is fixed-width: no resize handle while closed. */}
|
{/* The collapsed rail is fixed-width: no resize handle while closed. */}
|
||||||
{panels.sidebar > 0 && <DragHandle side="sidebar" left={cols.sidebar} onStart={onSidebarStart} onDrag={onSidebarDrag} onEnd={onDragEnd} />}
|
{panels.sidebar > 0 && <DragHandle side="sidebar" left={cols.sidebar} onStart={onSidebarStart} onDrag={onSidebarDrag} onEnd={onDragEnd} />}
|
||||||
{cols.details > 0 && <DragHandle side="details" left={viewport - cols.details} onStart={onDetailsStart} onDrag={onDetailsDrag} onEnd={onDragEnd} />}
|
{cols.details > 0 && <DragHandle side="details" left={viewport - cols.details} onStart={onDetailsStart} onDrag={onDetailsDrag} onEnd={onDragEnd} />}
|
||||||
|
|||||||
@@ -160,13 +160,11 @@ describe('AppFrame', () => {
|
|||||||
expect(slotCalls.map((c) => c.key)).toContain('conversation')
|
expect(slotCalls.map((c) => c.key)).toContain('conversation')
|
||||||
})
|
})
|
||||||
|
|
||||||
it('renders both column occupants before baselines settle (no loading gate)', () => {
|
it('keeps the loading branch until both object-layer baselines are ready', () => {
|
||||||
// The loading branch is gone: fixed tree positions from first paint, the
|
|
||||||
// occupants render their own pending states.
|
|
||||||
baselinesReady.current = false
|
baselinesReady.current = false
|
||||||
const { slotCalls } = mountFrame()
|
const { slotCalls, getByRole } = mountFrame()
|
||||||
expect(slotCalls.map((c) => c.key)).toContain('conversation')
|
expect(getByRole('status').textContent).toContain('Loading workspaces and sessions')
|
||||||
expect(slotCalls.map((c) => c.key)).toContain('details')
|
expect(slotCalls.map((c) => c.key)).not.toContain('conversation')
|
||||||
})
|
})
|
||||||
|
|
||||||
it('sidebar slot receives live concession output as owner props', () => {
|
it('sidebar slot receives live concession output as owner props', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user