Skip to content

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.

Sign up for GitHub

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
ytahdn wants to merge 14 commits into from
Closed

feat(web-shell): improve thinking and tool progress display #8872

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
36c9da2
feat(web-shell): improve thinking and tool progress display
Aug 10, 2026
8060181
fix(web-shell): address review feedback on tool timing and summaries …
qwen-code-dev-bot Aug 11, 2026
4180051
fix(web-shell): address round-2 feedback on tool timing and test cove…
qwen-code-dev-bot Aug 11, 2026
71a64b3
fix(web-shell): address remaining review feedback
Aug 11, 2026
ed03d38
chore: merge main into feat/optimize-web-shell-thinking-tools
Aug 11, 2026
7669bfc
chore: merge main into feat/optimize-web-shell-thinking-tools
Aug 11, 2026
183db61
chore: merge main into feat/optimize-web-shell-thinking-tools
Aug 11, 2026
4c628e8
Merge branch 'main' into feat/optimize-web-shell-thinking-tools
qwen-code-dev-bot Aug 11, 2026
e228244
fix(web-shell): preserve compact mode semantics
Aug 11, 2026
fab05f1
chore: revert unrelated ACP test formatting
Aug 11, 2026
5b7a2ec
Merge branch 'main' into feat/optimize-web-shell-thinking-tools
ytahdn Aug 11, 2026
33aa860
Merge branch 'main' into feat/optimize-web-shell-thinking-tools
ytahdn Aug 11, 2026
3795fb3
fix(web-shell): restore running duration fallback when tool start tim…
qwen-code-dev-bot Aug 11, 2026
9859fe4
fix(web-shell): anchor completed timing on the client clock (#8872)
qwen-code-dev-bot Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions docs/design/web-shell-thinking-and-tool-progress.md
View file
Open in desktop
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. Thinking and tool messages use that authoritative pair for completed durations only when it contains a positive elapsed interval. Live durations project the elapsed daemon duration onto the client clock, avoiding mixed-clock subtraction while still surviving transcript replay. Legacy and partial records without a usable daemon pair use the client-time pair.

## 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.
69 changes: 44 additions & 25 deletions packages/sdk-typescript/src/daemon/ui/transcript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -444,7 +444,7 @@ function handleStateResyncRequired(
lastDeliveredId: event.lastDeliveredId,
earliestAvailableId: event.earliestAvailableId,
};
propagateCancellationToInFlightTools(state);
propagateCancellationToInFlightTools(state, event);
appendStatusBlock(
state,
'error',
Expand Down Expand Up @@ -498,11 +498,12 @@ 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) {
block.serverTimestamp = event.serverTimestamp;

@chiga0 chiga0 Aug 11, 2026

Copy link
Copy Markdown
Collaborator

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_000 on a block with createdAt: 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 at transcriptToMessages.ts:377 and as setAt at App.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.

hasServerTimingPair protects the duration path (no serverUpdatedAt, so it falls back to the client pair — correct). The absolute-timestamp consumers are unprotected. Cheapest fix is to write the terminator time to serverUpdatedAt here as well and leave serverTimestamp unset, 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

@ytahdn ytahdn Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@chiga0 chiga0 Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nit] N6 — still present at 3795fb3. Your "partially confirmed" is the accurate reading.

The else branch (L504-505) correctly routes a later stamp to serverUpdatedAt, so the field this PR adds behaves as documented. What remains is L502-503: when a block was never stamped, the terminator event's serverTimestamp is seeded into block.serverTimestamp, a field documented at daemon/ui/types.ts:826 as the first observed time of the block.

You confirmed the hover-time consequence (transcriptToMessages.ts:377 reads serverTimestamp and would show the terminator's time as the block's start) and correctly noted my goal-state claim does not hold here. Agreed it is pre-existing rather than introduced, and low-impact — a block reaching finalization with no prior stamp is already a degraded record.

Recording it because this PR is what makes the distinction load-bearing: with serverUpdatedAt now present, "first observed" vs. "latest observed" has real semantics, and a comment at L502 noting that this seed is a degraded-record fallback (not a true start) would stop a future reader from treating it as authoritative — especially since hasServerTimingPair deliberately rejects the resulting degenerate pair.

This review was generated by QoderWork AI

} else {
block.serverUpdatedAt = event.serverTimestamp;
}
}
}
}
Expand Down Expand Up @@ -650,7 +651,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;

@chiga0 chiga0 Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] M2 — the ??= silently changes serverTimestamp semantics for two existing consumers.

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 = contradicted the documented contract, and the strict > in hasServerTimingPair depends on this. R1-13 already asked for a test pinning it.

What I did not see raised is that serverTimestamp has two pre-existing consumers whose output changes as a result, neither of which is about durations:

  • packages/web-shell/client/adapters/transcriptToMessages.ts:377 — const blockTime = block.serverTimestamp ?? block.clientReceivedAt becomes every message's hover timestamp. For a multi-delta assistant/thought block the tooltip previously drifted to the latest delta and now pins to the first. Better, but user-visible and unannounced.
  • packages/web-shell/client/App.tsx:554 — getLatestActiveGoalFromBlocks uses status.setAt ?? block.serverTimestamp ?? block.createdAt. Same shift.

Suggestion: call the semantics change out in the PR description / design doc (it is a public SDK field), and add one assertion on blockTime for a multi-delta block so the hover-timestamp behavior is pinned too — the currently-requested test only covers the reducer side.

This review was generated by QoderWork AI

@ytahdn ytahdn Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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.

@chiga0 chiga0 Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] M2 — still present at 3795fb3. Your rationale is sound; the gap is that the two changed consumers are still unpinned.

This file is byte-for-byte identical to 71a64b3, so existing.serverTimestamp ??= event.serverTimestamp stands.

I accept the design argument: serverTimestamp is documented as first observed and serverUpdatedAt now carries the latest stamp, so ??= makes the field match its own doc comment. Reclassifying from "silent change" to "intentional contract tightening" is fair.

What still concerns me is that two pre-existing consumers changed observable behavior with no test:

  • web-shell/client/adapters/transcriptToMessages.ts:377 — blockTime = block.serverTimestamp ?? block.clientReceivedAt drives the hover tooltip. A multi-delta assistant/thought block now shows its first delta time instead of its last.
  • web-shell/client/App.tsx:563 — setAt: status.setAt ?? block.serverTimestamp ?? block.createdAt for goal state.

Both are defensible (arguably more correct), but nothing in the suite would notice a regression back to last-write-wins. A single daemonUi case sending two timestamped thought.text.deltas into one block and asserting {serverTimestamp: <first>, serverUpdatedAt: <second>} pins the new contract cheaply — this is also qwen-code-ci-bot's still-open R1-13.

This review was generated by QoderWork AI

existing.serverUpdatedAt = event.serverTimestamp;
}
Comment thread
ytahdn marked this conversation as resolved.
if ('meta' in event && event.meta) {
existing.meta = { ...existing.meta, ...event.meta };
Expand Down Expand Up @@ -687,15 +689,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);
}
}

Expand Down Expand Up @@ -776,6 +778,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;
Expand Down Expand Up @@ -884,7 +889,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] }
Expand Down Expand Up @@ -930,7 +938,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(
Expand Down Expand Up @@ -1019,6 +1027,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
Expand All @@ -1033,6 +1042,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;
}
Comment thread
ytahdn marked this conversation as resolved.
}
state.currentToolCallId = undefined;
}
Expand Down Expand Up @@ -1068,7 +1080,7 @@ function appendShellBlock(
...(event.stream ? { stream: event.stream } : {}),
};
appendBlock(state, block);
clearActiveText(state);
clearActiveText(state, undefined, event);
}

function appendUserShellBlock(
Expand Down Expand Up @@ -1113,7 +1125,7 @@ function appendUserShellBlock(
};
state.pendingUserShellCommand = undefined;
appendBlock(state, block);
clearActiveText(state);
clearActiveText(state, undefined, event);
}

function upsertPermissionBlock(
Expand Down Expand Up @@ -1155,7 +1167,7 @@ function upsertPermissionBlock(
};
appendBlock(state, block);
state.permissionBlockByRequestId[event.requestId] = block.id;
clearActiveText(state);
clearActiveText(state, undefined, event);
}

function resolvePermissionBlock(
Expand Down Expand Up @@ -1208,7 +1220,7 @@ function resolvePermissionBlock(
};
appendBlock(state, block);
state.permissionBlockByRequestId[event.requestId] = block.id;
clearActiveText(state);
clearActiveText(state, undefined, event);
}

function appendStatusBlock(
Expand Down Expand Up @@ -1263,7 +1275,7 @@ function appendStatusBlock(
: {}),
};
appendBlock(state, block);
if (opts.clearActiveText !== false) clearActiveText(state);
if (opts.clearActiveText !== false) clearActiveText(state, undefined, event);
// 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.
Expand All @@ -1287,7 +1299,7 @@ function appendPromptCancelledBlock(
: {}),
};
appendBlock(state, block);
clearActiveText(state);
clearActiveText(state, undefined, event);
}

function createTextBlock(
Expand All @@ -1308,7 +1320,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 } } : {}),
};
Expand Down Expand Up @@ -1609,12 +1623,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;
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/sdk-typescript/src/daemon/ui/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,8 @@ export interface DaemonTranscriptBlockBase {
* display: clients viewing the same session see the same value.
*/
serverTimestamp?: number;
/** Daemon-authoritative wall clock for the latest event merged into this block. */
serverUpdatedAt?: number;
/** Ordered persisted ChatRecord identities that contributed to this block. */
sourceRecordIds?: readonly string[];
/**
Expand Down
Loading
Loading