fix(web-shell): defer session creation until first prompt - #6066
Conversation
|
Re-triage — 19 commits pushed since the last triage (June 30). Re-evaluating against HEAD Template looks good ✓ — all required sections present, bilingual body, "Tested on" table filled. On direction: still aligned. The controlled On approach: the latest 19 commits have tightened the implementation considerably. The focused fix commits ( The active review conversation — multiple Moving on to code review. 🔍 中文说明重新审查 — 自上次审查（6月30日）以来推送了 19 个提交。基于 HEAD 模板完整 ✓ — 所有必需章节齐全，中英文对照，"Tested on" 表格已填写。 方向：仍然对齐。受控 方案：最新 19 个提交大幅收紧了实现。聚焦修复提交显示了对维护者审查反馈的系统性迭代。上次审查中的范围顾虑（approvalMode 传播、voice model 重构、turn collapse key 修复）仍然打包在一起，但此时已被作为整体变更审查和处理。 活跃的审查对话——@wenshao 和 @doudouOUC 的多轮 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code Review (re-run on HEAD
|
Reflection (re-run on HEAD
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ Two caveats flagged in Stage 3 for the human reviewer (concurrent create+reconnect path worth a stress test; browser visual pass through the four scenarios before merge on Windows/Linux).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
Good overall approach — deferring session creation until the first prompt is a solid optimization. I found a few correctness issues in the session lifecycle coordination that could cause user-visible bugs:
attachedExistingSessionskips necessary fetches — after deferred creation, commands/skills/context remain undefinedensureSessionForPromptchain failure — ifsetModel()throws, the session is created but never attached (no SSE), causing silent hangsclearSession/loadSessionrace — concurrent session load during clear'sawait detach()can have its sessionId stripped
Plus two suggestions for robustness. See inline comments for details.
|
Updated the PR with fixes for the review threads:
Verification run locally: All passed. |
|
Follow-up: fixed the CI build failure from the previous run ( Both passed. Latest CI has been retriggered on |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test comment submission
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
The deferral approach is correct — keeping the welcome state until the first real prompt is a solid UX improvement. However, the new lifecycle coordination has a few correctness bugs that can produce user-visible failures, plus two load-bearing paths without test coverage.
Critical bugs
- ControlledSession + ensureSessionForPrompt race — host using
activeSessionId={undefined}in controlled mode gets the just-created session destroyed. - Deferred connect
setConnection({...})— plain-object state set can clobbersessionId/clientIdfrom a concurrentcreateSession. /goalcommand orphaned message —store.appendLocalUserMessage(text)runs beforesendGoalPrompt(). IfensureSessionForPromptthrows, the transcript entry is orphaned.
Untested load-bearing paths
4. createSession activeSession branch (createOrAttachSession path).
5. skipNextCleanupDetachSessionIdRef / keepSessionForNextEffect cleanup guard.
Verified clean: npm run build, npm run typecheck, and 142 tests across 5 suites (web-shell index, App, sessionPreparation; webui DaemonSessionProvider, actions; cli workspace-providers-status).
— qwen3.7-max via Qwen Code /review
|
Qwen Code review did not complete successfully. Qwen review exited with status 1. See workflow logs. |
| reportError(error, t('mode.plan')); | ||
| }); | ||
| return true; | ||
| return prompt ? false : true; |
There was a problem hiding this comment.
[Suggestion — low confidence] The /plan existing-session path returns prompt ? false : true instead of always true. When prompt exists, returning false means the editor is not cleared immediately — it is only cleared asynchronously when sendPrompt eventually runs editorRef.current?.clear() inside the .then() of setApprovalMode(). Unlike the no-session path (where isPreparingPrompt=true blocks duplicate submissions via the spinner), the existing-session path sets no such guard. A very fast second Enter before the async setApprovalMode round-trip completes could re-submit the same text.
The race window is tiny (one setApprovalMode network round-trip), but consider returning true to clear the composer immediately, matching the pre-PR behavior.
There was a problem hiding this comment.
Thanks for the callout. I checked this path again and am leaving it unchanged for now: returning true would clear the composer before setApprovalMode(plan) succeeds, so a mode-switch failure would drop the user prompt. The current behavior keeps the prompt until the async mode switch reaches sendPrompt, which matches the existing failure-safety behavior. Since this was marked low confidence and the race window is only the setApprovalMode round trip, I prefer not to trade it for potential input loss in this lifecycle PR.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
15 findings reported by 9 parallel review agents → 7 confirmed after verification and reverse audit (3 rounds, converged at round 3) → 8 rejected as false positives or already-discussed.
Build & Tests
- Typecheck (
@qwen-code/webui): ✅ PASS - webui tests (
DaemonSessionProvider.test.tsx,actions.test.ts): ✅ 133 passed - web-shell tests (
index.test.tsx,sessionPreparation.test.ts): ✅ 8 passed
Critical (high confidence)
-
actions.ts:583—createSession()unconditionally overwritessessionRef.currentand restoresconnection.sessionIdafterawait createDetachedSession(), with no check formanualSessionClearRef.current. IfclearSession()runs during the await, the result is an orphaned session with open SSE stream. Fix: checkmanualSessionClearRef.currentafter the await and detach the orphan if set. -
DaemonSessionProvider.tsx:1291—sessionRef.currentnot cleared on unmount during the skip-detach window. Block 3's condition!keepSessionForNextEffectdoesn't considerisUnmounting, leaving a staleDaemonSessionClientinsessionRefafter unmount. Fix:!keepSessionForNextEffect || isUnmounting.
Suggestions
-
actions.ts:172—startPendingSessionLoadtimeout misclassifiesattachmode asload_session. Addmode === 'attach' ? 'attach_session'branch. -
App.tsx:2467(low confidence) —/planexisting-session path returnsprompt ? false : trueinstead oftrue, potentially allowing duplicate prompt on very fast double-Enter. -
App.tsx:1828-1831(low confidence, not in diff hunk) —currentModel/currentModereset effects includeconnection.sessionIdin deps, causing a brief UI flicker when a deferred session is created. ThependingModel/pendingModemechanism partially mitigates.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Third-pass review — LGTM
All previously raised issues have been addressed:
-
onSessionIdChangenotification gap — Fixed vialastNotifiedSessionIdRefdedup pattern (df0df956). The callback now fires for both truthy andundefinedsession IDs, and avoids redundant notifications when the value hasn't changed. -
Test type alignment — Mock factories now produce structurally correct
CommandInfo,SupportedCommandsStatus, andContextStatusobjects (9e9d8fa1), eliminating silent type mismatches. -
createNewSessionno longer manually callsonSessionIdChange?.(undefined)— The effect-based notification makes this redundant. Removing it prevents double-fire edge cases.
Architecture assessment
The controlled-session refactor is well-structured:
- Stale event guard (
sessionRef.current?.sessionId !== activeSession.sessionId) correctly breaks the SSE loop on session swap/clear. manualSessionClearRefprevents unwanted reconnection after user-initiated clear.skipNextCleanupDetachSessionIdRefcorrectly preserves the session across the create→attach effect cycle, consumed in all three terminal paths (success, error, cleanup unmount).ensureSessionForPromptsingleton with.catch()+useEffectcleanup handles both concurrent calls and session-creation failures gracefully.createAndAttachSessionForPrompthas a correct cleanup chain: attach failure → close → clear → rethrow, with parallel non-blocking model/mode setup.
Test coverage is solid: 6 test files with unit tests for actions, session preparation, provider lifecycle, and mappers.
Minor nit (non-blocking)
MessageList.tsx L934-935: ref mutation during render (previousLiveStartedAtRef.current = liveStartedAt) is intentional to avoid elapsed-timer flash, but worth a brief inline comment noting the tradeoff for future readers.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ Two caveats flagged in Stage 3 for the human reviewer (approvalMode scope split; browser visual pass through the four scenarios before merge).
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
Analyzed all 24 changed files (+2629/-586) across the web-shell session lifecycle refactoring. After cross-referencing with the 25+ existing inline comments and their resolutions, no new high-confidence issues were found.
Assessment
The deferred session creation pattern is correctly implemented:
ensureSessionForPromptdedup (App.tsx): ThecreateSessionPromiseRefpattern correctly prevents concurrent session creation, and the.catchcleanup allows retry.createAndAttachSessionForPrompt(sessionPreparation.ts): Properly creates then attaches, with cleanup (closeSession+clearSession) on attach failure.- Composer clear timing (
App.tsx):clearComposerOnPromptStartcorrectly defers editor clearing until after session preparation succeeds, preserving user text on failure. getConnectionAfterSessionClear(actions.ts): Correctly guards field deletion with!clearedSessionId || current.sessionId === clearedSessionId.skipNextCleanupDetachSessionIdRef(DaemonSessionProvider.tsx): The unmount guard (!keepSessionForNextEffect || isUnmounting) correctly ensures detach runs on unmount even when session preservation is active.- Controlled session transitions (
DaemonSessionProvider.tsx): ThelastHandledSessionIdRefdedup plusconnectionRef.current.sessionId === sessionIdearly return prevents double-loading. - Deferred connect path (
DaemonSessionProvider.tsx): Correctly fetches workspace providers and returns early without creating a session whenshouldDeferInitialSessionCreationis true. - Heartbeat gating on
connection.status === 'connected': Prevents heartbeats from firing before the session is established or during disconnected states.
The prior review rounds (from @wenshao, @doudouOUC, @ytahdn, @qwen-code-ci-bot, @yiliang114, @DragonnZhang, @qqqys) covered the critical paths thoroughly, and the author addressed all findings across multiple fix commits.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Triage re-run on HEAD 48ad570: core change correct and well-tested through 20 commits, 223 tests pass, typecheck clean. No critical blockers found in independent code review. Approving.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed all 24 files (+2381/-507). Build passes, all 160 tests pass. The deferred-session architecture is well-designed and the extensive prior review iterations have resolved most lifecycle coordination issues. Two remaining suggestions below on edge cases in the error-recovery and UX paths.
— qwen3.7-max via Qwen Code /review
| try { | ||
| await sessionActions.attachSession(); | ||
| } catch (error) { | ||
| warn('[WebShell] failed to attach new session:', error); |
There was a problem hiding this comment.
[Suggestion] If clearSession() runs concurrently between createSession() returning and attachSession() being called (e.g., user clicks New Chat during the microtask gap), sessionRef.current becomes undefined. When attachSession() then throws, closeSession() here also fails (no session ref) and clearSession() is a no-op. The daemon session was detached in clearSession() step 3 but never closed — it leaks until idle timeout.
Consider capturing the session client reference from createSession()'s return value before calling attachSession(), so the catch block can close the daemon session directly even if sessionRef was cleared concurrently:
const createdSession = await sessionActions.createSession();
try {
await sessionActions.attachSession();
} catch (error) {
warn('[WebShell] failed to attach new session:', error);
// Use the captured reference, not sessionRef (which may be cleared)
await createdSession?.close?.().catch((e: unknown) => {
warn('[WebShell] failed to close unattached session:', e);
});
await sessionActions.clearSession().catch((clearError: unknown) => {
warn('[WebShell] failed to clear unattached session:', clearError);
});
throw error;
}— qwen3.7-max via Qwen Code /review
| setShowRetryHint(false); | ||
| const shouldShowPreparing = !connectionRef.current.sessionId; | ||
| if (shouldShowPreparing) { | ||
| setIsPreparingPrompt(true); |
There was a problem hiding this comment.
[Suggestion] When the first prompt triggers session creation, isPreparingPrompt disables the composer entirely. The user has no cancel button, no abort controller, and no visible timeout. Session creation can take up to 30 seconds (the startPendingSessionLoad timeout). If the daemon is congested, the user is locked out with no escape hatch.
Consider wiring an AbortController to ensureSessionForPrompt() and exposing a cancel action on the composer during the preparing state. At minimum, adding log entries for isPreparingPrompt transitions would help debugging:
if (shouldShowPreparing) {
console.info('[WebShell] preparing prompt: creating session');
setIsPreparingPrompt(true);
}— qwen3.7-max via Qwen Code /review
✅ Local real-build verification (maintainer)I built this PR from source and drove the real Environment
1. Build / typecheck / bundle
2. Unit tests shipped by the PR — 223 pass
3. Real-UI E2E — 17/17 assertions pass (PR build)
4. A/B vs merge-base — proves the fix is load-bearingSame script, same fake model, two builds:
The two exact problems the PR describes (eager empty-page session; silent replacement of a missing session) reproduce on base and are eliminated on the PR. 5. Extra confirmations
6. Minor, non-blocking observations
Conclusion: deferred creation, empty-page-with-no-session, New-Chat-to-empty, and missing-session-disconnect-without-replacement all verified on a real build, with an A/B contrast against base. LGTM to merge; the notes above are polish, not blockers. 中文说明（点击展开）✅ 本地真实构建验证（维护者）我从源码构建了本 PR，并用无头 Chromium（Playwright）驱动真实的 环境
1. 构建 / 类型检查 / bundle
2. PR 自带单测 —— 223 通过
3. 真实 UI E2E —— 17/17 断言通过（PR 构建）
4. 与 merge-base 的 A/B —— 证明该修复是真正承重的同一脚本、同一伪模型、两个构建：
PR 描述的两个问题（空页提前建 session、缺失 session 被静默替代）在 base 上均复现，在 PR 上均被消除。 5. 额外确认
6. 次要、不阻塞合并的观察
结论： 延迟创建、空页无 session、New Chat 回空页、缺失 session 断开而不静默替代，均已在真实构建上验证，并与 base 做了 A/B 对照。建议合并；上面两条只是打磨项，不构成阻塞。 Verified locally on head |
|
|
||
| void request.catch((error: unknown) => { | ||
| console.warn( | ||
| '[DaemonSessionProvider] controlled session transition failed:', |
There was a problem hiding this comment.
[Suggestion] The controlled-session transition failure log does not include the target sessionId being transitioned to, which is in scope from the enclosing effect. When debugging a failed transition (load or clear), the log is ambiguous — you cannot tell which session was targeted without reproducing the issue.
| '[DaemonSessionProvider] controlled session transition failed:', | |
| `[DaemonSessionProvider] controlled session transition failed (target=${sessionId ?? 'clear'}):`, |
— qwen3.7-max via Qwen Code /review
| hasCurrentSessionActivePromptRef.current = hasSessionActivePrompt; | ||
| setPromptStatus(hasSessionActivePrompt() ? 'streaming' : 'idle'); | ||
|
|
||
| const canReuseSessionMetadata = |
There was a problem hiding this comment.
[Suggestion — test gap] The canReuseSessionMetadata optimization skips 3 daemon API calls (workspaceProviders(), supportedCommands(), context()) when re-attaching an existing session with cached metadata, but no test asserts that these fetches are actually skipped. A regression that accidentally always-fetches would not be caught, silently re-introducing unnecessary daemon round-trips on every SSE effect re-run.
Consider adding a test that loads a session (populating commands/skills/context), triggers an SSE effect re-run entering the attachedExistingSession branch, and asserts the three daemon calls were NOT invoked a second time.
— qwen3.7-max via Qwen Code /review
| }, [connection.sessionId, heartbeatFailureThreshold, heartbeatIntervalMs]); | ||
| }, [ | ||
| connection.sessionId, | ||
| connection.status, |
There was a problem hiding this comment.
[Suggestion — test gap] The heartbeat effect gained connection.status in its dependency array, adding a guard that the heartbeat should not run when the connection is not 'connected'. No test verifies that clearing a session (which deletes connection.sessionId) stops an active heartbeat timer. The existing heartbeat test only covers failure threshold behavior, not timer cleanup after session clear.
Consider adding a test that: (1) loads a session with client_heartbeat capability, (2) verifies heartbeat calls start, (3) calls clearSession(), (4) advances time and asserts no further heartbeat calls occur.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
9 findings reported by 9 parallel review agents → 0 confirmed high-confidence, 9 confirmed low-confidence after verification and reverse audit (1 round, converged). All findings are suggestions, not correctness bugs.
Build & Tests
- Typecheck: passes for webui and cli workspaces
- Build: webui and web-shell build successfully
- Tests: web-shell client tests pass (5/5); DaemonSessionProvider tests pass (after building dependency packages)
Findings (all low-confidence suggestions)
- CDP tunnel auto-enable lacks operator diagnostic —
run-qwen-serve.ts:1316-1319. Auto-enabled by chrome-extension origin without a stderr log line, unlike the analogousclientMcpOverWsfeature. removeChromeDevToolsMcpIfUnusedTOCTOU —acp-http/index.ts:449-530. Narrow gap betweenhasActive()check and removal.- Channel memory type duplication —
channel-memory.ts+types.ts.filePathoptionality mismatch. - Channel memory dispatch duplicated —
ChannelBase.ts:327-381vs2132-2175. Two paths with subtly different claim strategies. isDaemonApprovalModeuntested —sessionPreparation.ts:15. Invalid mode IDs silently skipped.createSession()error path untested —actions.ts:559-605.clearSession()detach failure untested —actions.ts:616-630.newSession()untested —actions.ts:633-636. Load-bearingmanualSessionClearRefdistinction.canReuseSessionMetadataoptimization untested —DaemonSessionProvider.tsx:591-598.
Scope Splitting
The PR mixes session lifecycle refactoring (web-shell/webui/SDK) with channel memory (core/channels) and CDP tunnel changes (CLI serve). The channel memory and CDP tunnel changes are orthogonal and could be extracted into separate PRs for easier review and safer rollback.
Verdict: Comment — no blocking issues found.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Additional findings (not mapped to specific diff lines):
- [Critical]
handleFastModelSelect(App.tsx:3261) still routes throughsendPrompt, which now callsensureSessionForPrompt()— creating a full daemon session. Selecting a fast model from the UI when no session exists will silently create a session, establish SSE streaming, and send/model --fast <id>as a prompt. This is both a behavioral regression (previously showed a "session not connected" error) and inconsistent withhandleModelSelect(usessetPendingModel) andhandleVoiceModelSelect(usessetWorkspaceSetting), both of which handle the no-session case without creating a session.
Additional test coverage gaps:
DaemonClient.ts— the newdetachSessionmethod (POST/session/:id/detach) has no unit test. It handles 204, 404, and error responses; none are tested.DaemonSessionProvider.tsxcontrolled-session effect — no test covers theundefined → sessionIdtransition (prop changes fromundefinedto a concrete session id after the provider already rendered in the empty state). This is the path a host hits when navigating from "new chat" to "resume session" without remounting.actions.ts— no test forcreateSession()whencreateDetachedSession()throws (the error path that posts a notice), and no test forclearSession()whensessionRef.currentis alreadyundefined(the no-op path).
— qwen3.7-max via Qwen Code /review
| }) | ||
| : Promise.resolve(), | ||
| modeId && isDaemonApprovalMode(modeId) | ||
| ? sessionActions.setApprovalMode(modeId).catch((error: unknown) => { |
There was a problem hiding this comment.
[Critical] setModel and setApprovalMode failures are swallowed with console.warn only. The session is still created and attached, so the user submits a prompt believing they're in plan mode or using a specific model — but the session runs with defaults. No toast, no notice, no error state.
This is especially dangerous for approval mode: a user expecting plan (requiring explicit approval for edits) could end up in default mode where edits happen automatically.
Consider propagating the error or surfacing a user-visible notice:
if (modeId && isDaemonApprovalMode(modeId)) {
await sessionActions.setApprovalMode(modeId); // let failure propagate
}— qwen3.7-max via Qwen Code /review
|
|
||
| if (!request) return; | ||
|
|
||
| void request.catch((error: unknown) => { |
There was a problem hiding this comment.
[Critical] When the controlled sessionId prop changes and loadSession or clearSession fails, the error is silently caught with console.warn. The provider's actual session diverges from the prop value with no error surfaced to the user or the parent component.
An embedded consumer passes sessionId="abc-123" via URL, the daemon returns 500, and the UI stays on the previous session. The user sees stale transcript and may take actions referencing a session they're not actually viewing.
Surface the error through addNotice or a connection error state:
void request.catch((error: unknown) => {
addNotice({
code: 'daemon.session_transition.failed',
severity: 'error',
message: `Failed to switch to session ${sessionId}`,
operation: sessionId ? 'load_session' : 'clear_session',
});
});— qwen3.7-max via Qwen Code /review
| async clearSession() { | ||
| const session = sessionRef.current; | ||
| manualSessionClearRef.current = true; | ||
| clearActiveSessionState(); |
There was a problem hiding this comment.
[Critical] clearSession sets sessionRef.current = undefined before awaiting session.detach(). If detach times out or fails (line 642 catches with console.warn), the session reference is already lost from the client side with no recovery path. The daemon retains the session, but the client can never clean it up.
Consider detaching first, then clearing the ref:
const session = sessionRef.current;
manualSessionClearRef.current = true;
clearActiveSessionState();
if (session) {
try {
await withActionTimeout(session.detach(), 'Clear session timed out');
} catch (error) {
console.warn('[DaemonSessionActions] detach on clear failed:', error);
}
}
sessionRef.current = undefined;
setConnection((current) =>
getConnectionAfterSessionClear(current, session?.sessionId),
);— qwen3.7-max via Qwen Code /review
| ? session | ||
| : undefined; | ||
| if (activeSession) { | ||
| const nextSession = await withActionTimeout( |
There was a problem hiding this comment.
[Critical] The active-session path returns nextSession without updating sessionRef.current (contrast with the detached path at line ~598 which sets sessionRef.current = nextSession). A subsequent attachSession() reads sessionRef.current (still the old DaemonSessionClient) and attaches the old session instead of the newly created one.
While createAndAttachSessionForPrompt avoids this path (early-returns when a session exists), any external consumer of DaemonSessionActions that calls createSession() + attachSession() with an active session will silently re-attach the old session.
— qwen3.7-max via Qwen Code /review
| @@ -1168,14 +1265,20 @@ export function DaemonSessionProvider({ | |||
| hasCurrentSessionActivePromptRef.current = () => false; | |||
There was a problem hiding this comment.
[Suggestion] The session lifecycle is coordinated via three shared mutable refs (sessionRef, skipNextCleanupDetachSessionIdRef, manualSessionClearRef) with no documentation of the protocol. createSession() writes to sessionRef.current and sets skipNextCleanupDetachSessionIdRef to prevent the effect's cleanup from detaching the new session. The caller must follow with attachSession() to start event streaming.
Any future modification to createSession, clearSession, newSession, or the effect's dependency array risks silently detaching sessions or leaking server-side sessions. Consider adding a block comment above the ref declarations documenting this state machine protocol, or encoding it as a discriminated union instead of three independent refs.
— qwen3.7-max via Qwen Code /review
| warn('[WebShell] failed to attach new session:', error); | ||
| await sessionActions.closeSession().catch((closeError: unknown) => { | ||
| warn('[WebShell] failed to close unattached session:', closeError); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] When attachSession() fails, cleanup attempts closeSession() then clearSession(). If both also fail, the daemon retains an orphaned session with no subscribed client. The session's conversation history and any captured file contents persist on the daemon server indefinitely with no way for the client to reclaim or delete them.
Consider adding a final fallback that force-deletes the session by ID:
} catch (error) {
// ... existing cleanup ...
// Last resort: force-delete the session by ID
await sessionActions.deleteSession?.(createdSessionId).catch(() => {});
throw error;
}— qwen3.7-max via Qwen Code /review
| current: DaemonConnectionState, | ||
| clearedSessionId: string | undefined, | ||
| ): DaemonConnectionState { | ||
| const next = { ...current }; |
There was a problem hiding this comment.
[Suggestion] getConnectionAfterSessionClear unconditionally forces status: 'connected' regardless of the actual daemon connection state. If clearSession() is called while the daemon is unreachable (connection was 'connecting' or 'disconnected'), the result is a connection that says 'connected' with no sessionId. Downstream consumers that branch on connection.status === 'connected' will incorrectly treat a disconnected daemon as ready.
Consider preserving the prior status when it was not 'connected', or documenting explicitly that clearSession always transitions to 'connected'.
— qwen3.7-max via Qwen Code /review
| typeof settings.fastModel === 'string' && settings.fastModel.length > 0 | ||
| ? settings.fastModel | ||
| : undefined; | ||
| const approvalMode = resolveApprovalMode(settings); |
There was a problem hiding this comment.
[Critical] This uses the raw merged tools.approvalMode from settings, but session creation normally ignores that setting in --safe/--bare modes and downgrades unsafe modes for untrusted folders. The deferred web-shell path copies this value into connection.currentMode and then applies it with setApprovalMode() before the first prompt, so a workspace setting like yolo can re-enable a mode that the normal session config would have forced back to default. Please return the daemon's effective approval mode with the same safety/trust precedence as session config, or omit it until a real session context is available.
— gpt-5 via Qwen Code /review
| models: sessionModels, | ||
| currentModel: sessionCurrentModel, | ||
| currentMode, | ||
| commands: commands.length > 0 ? commands : current.commands, |
There was a problem hiding this comment.
[Critical] These fallbacks reuse current.commands, current.skills, current.supportedCommands, and current.context even when current.sessionId !== activeSession.sessionId. If switching from session A to session B and B's supportedCommands() or context() request fails, the connection now says B but still exposes A's session-scoped command/context metadata to the UI. Please only preserve these fields for same-session reconnects or the explicit attachedExistingSession reuse case; for a different session, clear them or use the fresh empty values.
— gpt-5 via Qwen Code /review
| await sessionActions.createSession(); | ||
| try { | ||
| await sessionActions.attachSession(); | ||
| } catch (error) { |
There was a problem hiding this comment.
[Critical] attachSession() can reject because it was superseded by a later controlled loadSession() (the pending load rejection is an AbortError), not because the just-created session failed to attach. This catch still calls global closeSession() and clearSession(), which operate on whatever session is currently in the provider, so a host sessionId switch during first-prompt preparation can close or clear the newly selected session. Treat superseded/abort attach errors as cancellation, and for real attach failures clean up only the session id that was just created.
— gpt-5 via Qwen Code /review
| }, [connection.catchingUp, connection.sessionId, sidebarSwitchingSessionId]); | ||
|
|
||
| const openTasksPanel = useCallback(() => { | ||
| if (!requireActiveSessionForLocalCommand()) return; |
There was a problem hiding this comment.
[Critical] The no-session guard inside openTasksPanel() returns early, but this branch still returns true, so useComposerCore treats /tasks as accepted and clears the editor on the empty page. The same pattern remains for /branch, /context, /recap, and /btw: they show the no-session toast but still consume the typed command. Please propagate false when the guard fails, or guard directly in each branch, so the composer preserves the user's input like the fixed /rewind, /fork, /rename, and /stats paths.
— gpt-5 via Qwen Code /review
… prompt (#6153) * fix(web-shell): show skill slash commands (e.g. /review) before first prompt Since session creation is deferred until the first prompt (#6066), the deferred connect path reported 'connected' but only fetched workspace providers — it never populated the slash-command list. Before sending a message the composer therefore fell back to the hardcoded local command list, which omits skills, so '/rev' would not autocomplete '/review'. Fetch the session-less /workspace/skills status alongside providers in the deferred connect path and seed connection.commands/skills from it, so skill-backed slash commands autocomplete immediately. The full session-scoped supported-commands snapshot (which also carries custom, MCP-prompt and workflow commands) still replaces this once the first prompt creates a session. * test(web-shell): cover deferred workspace skills fetch failure Add a parallel test to the deferred-connect warn coverage: when client.workspaceSkills() rejects, the connection still reports 'connected' (skills are non-blocking) and the failure is logged via console.warn, mirroring the existing workspaceProviders-failure test. Addresses review feedback on #6153.
7 participants
What This PR Does
This PR makes web-shell session selection explicit and host-controlled through a single
sessionIdprop. WhensessionIdis omitted orundefined, web-shell now stays on the empty welcome page and does not create a daemon session. A session is created only when the user sends the first real prompt from that empty page.The external API is intentionally simplified:
sessionId="existing-id"loads that session.sessionIdswitches to the new session.sessionId={undefined}clears the current session and returns to the empty page.sessionIdleaves the provider disconnected instead of silently creating a replacement session.This replaces the earlier split between
initialSessionIdandactiveSessionId. That split made the lifecycle hard to reason about: initial load, controlled switching, empty-page behavior, and first-prompt creation all had overlapping semantics. The PR now keeps only one internal meaning: the session selected by the host issessionId; no selected session means no active chat.Why It's Needed
Previously, opening web-shell without a session id could eagerly create an empty daemon session. That caused several user-visible problems:
initialSessionId/activeSessionIdsplit made controlled embedding ambiguous, especially when the host started empty and later selected a session.The intended product behavior is simpler: an empty page is a real empty state. The first manual send creates a session. Hosts that want to control the visible session pass
sessionId.Implementation Notes
DaemonSessionProvidernow acceptssessionIdand treats it as the controlled selected session.sessionIdno longer triggers automatic session creation.WebShellWithProviderspassessessionIddirectly toDaemonSessionProvider; the oldControlledSessionwrapper is removed.main.tsxuses controlled mode throughsessionIdand no longer passes an initial session id.sessionIdAPI.Reviewer Test Plan
How To Verify
Open web-shell without
sessionIdand confirm it stays on the empty welcome page without creating a daemon session. Send a prompt and confirm a session is created only then. Click New Chat and confirm the UI returns to the empty page without creating another session. RenderWebShellWithProvidersorDaemonSessionProviderwithsessionId, change it to another existing session, and confirm the visible transcript switches. SetsessionIdtoundefinedand confirm the view clears. SetsessionIdto a nonexistent session and confirm it disconnects/errors instead of creating a replacement session.Local verification run on this branch:
Root
npm run typecheckwas also attempted. It reaches the relevant CLI/core/sdk/webui workspaces cleanly, then stops in the chrome-extension workspace because the local environment is missing thechrometype definition file. That failure is unrelated to this PR's session changes.Tested On
Risk And Scope
The main risk is around session lifecycle timing: first-prompt creation, clearing to empty, controlled session switches, missing-session handling, and stale streaming events. The focused tests cover those paths at the provider/wrapper level.
This is intentionally a breaking API cleanup for web-shell embedding: hosts should use
sessionIdinstead ofinitialSessionIdoractiveSessionId.Linked Issues
N/A
中文说明
这个 PR 做了什么
这个 PR 把 web-shell 的 session 选择统一成一个外部参数：
sessionId。不传sessionId或传undefined时，web-shell 会停留在空白欢迎页，不再自动创建 daemon session。只有用户从空页面真正发送第一条消息时，才创建 session。新的对外语义是：
sessionId="existing-id"：加载这个 session。sessionId：切换到新的 session。sessionId={undefined}：清空当前 session，回到空页面。sessionId指向不存在/已删除的 session：进入 disconnected/error 状态，不自动创建替代 session。这个 PR 移除了之前
initialSessionId和activeSessionId的拆分。之前这两个参数让“初始加载”“受控切换”“空页面”“首次发送创建 session”几种语义交叠在一起，代码和使用方式都比较绕。现在内部只有一个含义：宿主选择的 session 就是sessionId；没有选择 session 就是没有活跃对话。为什么需要
之前打开未传 session id 的 web-shell 时，可能会提前创建空的 daemon session，带来几个问题：
initialSessionId/activeSessionId的双参数语义让外部受控嵌入很难判断，尤其是“初始为空，后续再选择 session”的场景。目标产品行为更简单：空页面就是空页面。用户第一次手动发送才创建 session。宿主要控制当前显示哪个 session，就传
sessionId。实现说明
DaemonSessionProvider改为接收sessionId，并把它作为受控选中 session。sessionId不再触发自动创建 session。WebShellWithProviders直接把sessionId传给DaemonSessionProvider，移除旧的ControlledSession包装层。main.tsx使用受控模式，不再传 initial session id。sessionIdAPI。Reviewer Test Plan
如何验证
打开未传
sessionId的 web-shell，确认停留在空欢迎页且不会创建 daemon session。发送 prompt，确认此时才创建 session。点击 New Chat，确认回到空页面且不会再创建一个 session。给WebShellWithProviders或DaemonSessionProvider传sessionId，切换到另一个已有 session，确认 transcript 会切换。把sessionId改为undefined，确认页面清空。传不存在的sessionId，确认进入断开/错误状态，不创建替代 session。本地验证命令：
也尝试了根目录
npm run typecheck。相关的 CLI/core/sdk/webui workspace 均执行通过，随后在 chrome-extension workspace 因本地环境缺少chrometype definition file 中断。该失败与本 PR 的 session 改动无关。风险与范围
主要风险在 session 生命周期时序：首次发送创建、清空到空页面、受控切换、缺失 session 处理、旧流式事件隔离。当前已用 provider/wrapper 层测试覆盖这些路径。
这是 web-shell 嵌入 API 的有意收敛：宿主应使用
sessionId，不再使用initialSessionId或activeSessionId。