-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(web-shell): improve thinking and tool progress display #8872
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
Changes from all commits
36c9da2
8060181
4180051
71a64b3
ed03d38
7669bfc
183db61
4c628e8
e228244
fab05f1
5b7a2ec
33aa860
3795fb3
9859fe4
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 |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Web Shell compact mode and tool progress | ||
|
|
||
| ## Goal | ||
|
|
||
| Update the existing Web Shell compact mode to hide transcript thinking without changing model behavior, make parallel tool summaries describe every active foreground tool until all tools finish, and keep thinking/tool elapsed times stable across transcript replay. | ||
|
|
||
| ## Design | ||
|
|
||
| `App` keeps the existing `Ctrl+O` compact-mode shortcut, context, Help terminology, and `ui.compactMode` workspace-setting write. Compact mode no longer switches message bodies to their old condensed cards. Instead, `MessageList` removes thinking rows only from its rendered item list, leaving the transcript and model behavior unchanged. | ||
|
|
||
| In compact mode, regular tool groups separated only by hidden thinking are merged within the same activity sequence. Outside compact mode, visible thinking preserves the original interleaved transcript order. User, assistant, system, plan, approval, agent, todo, and question UI boundaries remain separate. Running tool summaries are derived from all active foreground tools and reuse the existing tool descriptions. Completed summaries remain unchanged and appear only after no tool is active. Expanded tool rows reuse the existing tool-kind icons. | ||
|
|
||
| Transcript blocks retain the first and latest daemon timestamps. When a block carries an authoritative pair with a positive elapsed interval, thinking and tool messages keep the daemon-measured duration but anchor it onto the client clock, so every start/end timestamp stays in one domain and remains comparable across tools. Live durations use the same projection, avoiding mixed-clock subtraction while still surviving transcript replay. Legacy and partial records without a usable daemon pair use the client-time pair. Consecutive thinking blocks merge regardless of which timing source produced them, accumulating each block's own duration. | ||
|
|
||
| ## Compatibility | ||
|
|
||
| The existing compact-mode concept and persistence path remain unchanged. No new setting, URL parameter, public transcript prop, or `localStorage` key is introduced. The read-only `WebShellTranscript` remains outside compact mode. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -292,7 +292,7 @@ function applyDaemonTranscriptEvent( | |
| // those reasons; the post-reconnect `tool_call_update` stream | ||
| // will deliver the real terminal status. | ||
| if (event.reason === 'cancelled' || event.reason === 'error') { | ||
| propagateCancellationToInFlightTools(next); | ||
| propagateCancellationToInFlightTools(next, event); | ||
| } | ||
| break; | ||
| case 'assistant.usage': | ||
|
|
@@ -376,7 +376,7 @@ function applyDaemonTranscriptEvent( | |
| // UIs don't show a tool spinning forever after a peer cancel. | ||
| // Idempotent — safe if the daemon also later emits terminal | ||
| // tool_call_update frames. | ||
| propagateCancellationToInFlightTools(next); | ||
| propagateCancellationToInFlightTools(next, event); | ||
| if (event.reason !== 'forward_failed') { | ||
| appendPromptCancelledBlock(next, event); | ||
| } | ||
|
|
@@ -444,7 +444,7 @@ function handleStateResyncRequired( | |
| lastDeliveredId: event.lastDeliveredId, | ||
| earliestAvailableId: event.earliestAvailableId, | ||
| }; | ||
| propagateCancellationToInFlightTools(state); | ||
| propagateCancellationToInFlightTools(state, event); | ||
| appendStatusBlock( | ||
| state, | ||
| 'error', | ||
|
|
@@ -498,11 +498,15 @@ function finalizeStreamingTextBlock( | |
| if (event?.eventId !== undefined) block.eventId = event.eventId; | ||
| // Preserve the text event's own timestamp during history replay; later | ||
| // finalize/status events can be much newer and would skew message times. | ||
| if ( | ||
| block.serverTimestamp === undefined && | ||
| event?.serverTimestamp !== undefined | ||
| ) { | ||
| block.serverTimestamp = event.serverTimestamp; | ||
| if (event?.serverTimestamp !== undefined) { | ||
| if (block.serverTimestamp === undefined) { | ||
| // Degraded-record fallback: the block was never stamped while | ||
| // streaming, so the terminator's stamp approximates its first | ||
| // observed time rather than being the true start. | ||
| block.serverTimestamp = event.serverTimestamp; | ||
|
Collaborator
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. [Nit] N6 — still present at The You confirmed the hover-time consequence ( Recording it because this PR is what makes the distinction load-bearing: with This review was generated by QoderWork AI |
||
| } else { | ||
| block.serverUpdatedAt = event.serverTimestamp; | ||
| } | ||
|
ytahdn marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
|
|
@@ -650,7 +654,8 @@ function appendTextDelta( | |
| existing.updatedAt = state.now; | ||
| if (event.eventId !== undefined) existing.eventId = event.eventId; | ||
| if (event.serverTimestamp !== undefined) { | ||
| existing.serverTimestamp = event.serverTimestamp; | ||
| existing.serverTimestamp ??= event.serverTimestamp; | ||
|
Collaborator
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. [Minor] M2 — the This flips merged text deltas from last-write-wins to first-write-wins. It is almost certainly the right change — the field's own JSDoc says "captured when the block was first observed", so the old What I did not see raised is that
Suggestion: call the semantics change out in the PR description / design doc (it is a public SDK field), and add one assertion on This review was generated by QoderWork AI
Collaborator
Author
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. No code change. First-write-wins is intentional: serverTimestamp is documented as first observed, serverUpdatedAt carries the latest stamp, and the multi-delta reducer test pins the 1000 to 6000 pair. The hover timestamp now using the first observed stamp is the desired consequence. An additional adapter assertion would add coverage rather than correct behavior, so it is deferred under the late-review scope.
Collaborator
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. [Minor] M2 — still present at This file is byte-for-byte identical to I accept the design argument: What still concerns me is that two pre-existing consumers changed observable behavior with no test:
Both are defensible (arguably more correct), but nothing in the suite would notice a regression back to last-write-wins. A single This review was generated by QoderWork AI |
||
| existing.serverUpdatedAt = event.serverTimestamp; | ||
| } | ||
|
ytahdn marked this conversation as resolved.
|
||
| if ('meta' in event && event.meta) { | ||
| existing.meta = { ...existing.meta, ...event.meta }; | ||
|
|
@@ -687,15 +692,15 @@ function appendTextDelta( | |
|
|
||
| if (parentId != null) { | ||
| if (kind === 'assistant') { | ||
| clearActiveThoughtForParent(state, parentId); | ||
| clearActiveThoughtForParent(state, parentId, event); | ||
| } | ||
| if (kind === 'thought') { | ||
| clearActiveAssistantForParent(state, parentId); | ||
| clearActiveAssistantForParent(state, parentId, event); | ||
| } | ||
| } else { | ||
| if (kind !== 'user') state.activeUserBlockId = undefined; | ||
| if (kind !== 'assistant') clearActiveAssistant(state); | ||
| if (kind !== 'thought') clearActiveThought(state); | ||
| if (kind !== 'assistant') clearActiveAssistant(state, event); | ||
| if (kind !== 'thought') clearActiveThought(state, event); | ||
|
Comment on lines
+702
to
+703
Collaborator
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. [Suggestion] R3-3: Cross-kind text-delta finalization passes the raw successor event (with const stamp = { ...event, eventId: undefined };
if (kind !== 'assistant') clearActiveAssistant(state, stamp);
if (kind !== 'thought') clearActiveThought(state, stamp);中文说明跨类型 text-delta 终结把带 const stamp = { ...event, eventId: undefined };
if (kind !== 'assistant') clearActiveAssistant(state, stamp);
if (kind !== 'thought') clearActiveThought(state, stamp);— qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -776,6 +781,9 @@ function upsertToolBlock( | |
| } | ||
| existing.updatedAt = state.now; | ||
| if (event.eventId !== undefined) existing.eventId = event.eventId; | ||
| if (event.serverTimestamp !== undefined) { | ||
| existing.serverUpdatedAt = event.serverTimestamp; | ||
| } | ||
| if (event.details) existing.details = event.details; | ||
| if (compactTaskOutput) delete existing.content; | ||
| else if (event.content !== undefined) existing.content = event.content; | ||
|
|
@@ -884,7 +892,10 @@ function upsertToolBlock( | |
| updatedAt: state.now, | ||
| ...(event.eventId !== undefined ? { eventId: event.eventId } : {}), | ||
| ...(event.serverTimestamp !== undefined | ||
| ? { serverTimestamp: event.serverTimestamp } | ||
| ? { | ||
| serverTimestamp: event.serverTimestamp, | ||
| serverUpdatedAt: event.serverTimestamp, | ||
| } | ||
| : {}), | ||
| ...(event.sourceRecordIds | ||
| ? { sourceRecordIds: [...event.sourceRecordIds] } | ||
|
|
@@ -930,7 +941,7 @@ function upsertToolBlock( | |
| // never points at it. Effective-status keeps the pointer in sync | ||
| // with what was actually written to the block. | ||
| updateCurrentToolPointer(state, event.toolCallId, event.status ?? 'pending'); | ||
| clearActiveText(state, event.parentToolCallId); | ||
| clearActiveText(state, event.parentToolCallId, event); | ||
| } | ||
|
|
||
| function discardToolBlock( | ||
|
|
@@ -1019,6 +1030,7 @@ function findLatestInFlightToolCallId( | |
| */ | ||
| function propagateCancellationToInFlightTools( | ||
| state: DaemonTranscriptState, | ||
| event?: DaemonUiEvent, | ||
| ): void { | ||
| // Skip trimmed sentinels up front. Without this filter | ||
| // each cancellation walked the entire historical tool-call index (which | ||
|
|
@@ -1033,6 +1045,9 @@ function propagateCancellationToInFlightTools( | |
| if (!IN_FLIGHT_TOOL_STATUSES.has(block.status)) continue; | ||
| block.status = 'cancelled'; | ||
| block.updatedAt = state.now; | ||
| if (event?.serverTimestamp !== undefined) { | ||
| block.serverUpdatedAt = event.serverTimestamp; | ||
| } | ||
|
ytahdn marked this conversation as resolved.
|
||
| } | ||
| state.currentToolCallId = undefined; | ||
| } | ||
|
|
@@ -1068,7 +1083,7 @@ function appendShellBlock( | |
| ...(event.stream ? { stream: event.stream } : {}), | ||
| }; | ||
| appendBlock(state, block); | ||
| clearActiveText(state); | ||
| clearActiveText(state, undefined, event); | ||
| } | ||
|
|
||
| function appendUserShellBlock( | ||
|
|
@@ -1113,7 +1128,7 @@ function appendUserShellBlock( | |
| }; | ||
| state.pendingUserShellCommand = undefined; | ||
| appendBlock(state, block); | ||
| clearActiveText(state); | ||
| clearActiveText(state, undefined, event); | ||
| } | ||
|
|
||
| function upsertPermissionBlock( | ||
|
|
@@ -1155,7 +1170,7 @@ function upsertPermissionBlock( | |
| }; | ||
| appendBlock(state, block); | ||
| state.permissionBlockByRequestId[event.requestId] = block.id; | ||
| clearActiveText(state); | ||
| clearActiveText(state, undefined, event); | ||
| } | ||
|
|
||
| function resolvePermissionBlock( | ||
|
|
@@ -1208,7 +1223,7 @@ function resolvePermissionBlock( | |
| }; | ||
| appendBlock(state, block); | ||
| state.permissionBlockByRequestId[event.requestId] = block.id; | ||
| clearActiveText(state); | ||
| clearActiveText(state, undefined, event); | ||
| } | ||
|
|
||
| function appendStatusBlock( | ||
|
|
@@ -1263,7 +1278,7 @@ function appendStatusBlock( | |
| : {}), | ||
| }; | ||
| appendBlock(state, block); | ||
| if (opts.clearActiveText !== false) clearActiveText(state); | ||
| if (opts.clearActiveText !== false) clearActiveText(state, undefined, event); | ||
|
Collaborator
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. [Suggestion] R3-1: The 中文说明经由 — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| // Opt-out only protects the streaming assistant/thought block; the user | ||
| // pointer must still reset, otherwise a later mergeable user.text.delta | ||
| // (e.g. a peer client's prompt echo) appends onto the command echo block. | ||
|
|
@@ -1287,7 +1302,7 @@ function appendPromptCancelledBlock( | |
| : {}), | ||
| }; | ||
| appendBlock(state, block); | ||
| clearActiveText(state); | ||
| clearActiveText(state, undefined, event); | ||
| } | ||
|
|
||
| function createTextBlock( | ||
|
|
@@ -1308,7 +1323,9 @@ function createTextBlock( | |
| createdAt: state.now, | ||
| updatedAt: state.now, | ||
| ...(eventId !== undefined ? { eventId } : {}), | ||
| ...(serverTimestamp !== undefined ? { serverTimestamp } : {}), | ||
| ...(serverTimestamp !== undefined | ||
| ? { serverTimestamp, serverUpdatedAt: serverTimestamp } | ||
| : {}), | ||
| ...(sourceRecordIds ? { sourceRecordIds: [...sourceRecordIds] } : {}), | ||
| ...(meta ? { meta: { ...meta } } : {}), | ||
| }; | ||
|
|
@@ -1609,12 +1626,17 @@ function allocateBlockId(state: DaemonTranscriptState, prefix: string): string { | |
| function clearActiveText( | ||
| state: DaemonTranscriptState, | ||
| parentToolCallId?: string, | ||
| event?: DaemonUiEvent, | ||
| ): void { | ||
| // Terminator events close the streaming block but do not own its content: | ||
| // stamp the server-time boundary while keeping the block's eventId, which | ||
| // anchors replay ordering. | ||
| const stamp = event ? { ...event, eventId: undefined } : undefined; | ||
| if (parentToolCallId) { | ||
| clearActiveAssistantForParent(state, parentToolCallId); | ||
| clearActiveThoughtForParent(state, parentToolCallId); | ||
| clearActiveAssistantForParent(state, parentToolCallId, stamp); | ||
| clearActiveThoughtForParent(state, parentToolCallId, stamp); | ||
| } else { | ||
| finishAssistant(state); | ||
| finishAssistant(state, stamp); | ||
| state.activeUserBlockId = undefined; | ||
| } | ||
| } | ||
|
|
||
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.
[Nit] N6 — this branch stamps the terminator's time into a field documented as the block's start.
Pre-existing rather than introduced here, but the PR now pins it with
does not create a server timing pair from a stamped thought end only(serverTimestamp: 6_000on a block withcreatedAt: 100_000), so it is worth naming.When a block was created without a stamp and is finalized by a much later event,
serverTimestamp— documented as "captured when the block was first observed", and consumed as the hover wall-clock attranscriptToMessages.ts:377and assetAtatApp.tsx:554— becomes the block's end time. The comment two lines above says the intent is that "later finalize/status events … would skew message times", which is exactly what this branch does in the one case it applies to.hasServerTimingPairprotects the duration path (noserverUpdatedAt, so it falls back to the client pair — correct). The absolute-timestamp consumers are unprotected. Cheapest fix is to write the terminator time toserverUpdatedAthere as well and leaveserverTimestampunset, letting the pair gate reject it; alternatively just extend the comment to say the field is deliberately end-anchored in this case.This review was generated by QoderWork AI
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.
Partially confirmed: in the mixed unstamped-start and stamped-terminator edge, hover time uses the terminator as the block start. The claimed goal setAt impact is not reachable because that helper reads status blocks while finalizeStreamingTextBlock handles assistant and thought blocks. Not fixed here because deciding whether to discard the sole daemon stamp from absolute display changes an existing timestamp fallback contract outside this PR. Keeping the real hover issue open for a focused follow-up.