Skip to content

Commit d1c0109

Browse files
authored
🤖 refactor: trim chat transcript scroll cleanup (#3210)
## Summary Follow-up cleanup after #3201: trims maintenance-only code in the chat transcript bottom-lock and bash auto-expand paths while preserving the validated behavior. After a 4-lane sub-agent review, this PR also consolidates duplicated scroll-metric test scaffolding and removes a few small dead/indirect code paths. ## Background #3201 stabilized transcript bottom ownership and bash row expansion. This PR keeps that behavior but reduces surface area in the related code paths. Cleanup candidates were split across sub-agent lanes for auto-scroll, bash/tool rows, ChatPane/LayoutStack, and related renderer test helpers; the accepted low-risk findings were consolidated here. ## Implementation - Replaces the bash auto-expand hook's raw latest-bash id input with a direct `hasReplacementStreamingBash` boolean. - Consolidates bash auto-expand timer cleanup into the layout effect and removes the separate unmount-only effect. - Simplifies local Bash row derived state for output/background results. - Removes the unused `observedHeightRef` writes from `LayoutStackLane`. - Drops an unused auto-scroll distance helper. - Removes dead `todoCount` prop plumbing from `ChatInputPane`. - Shares the happy-dom scroll-metric shim between hook and integration tests. - Reuses the scroll-metric helper in the bottom-layout integration test and makes the viewport-resize case actually shrink the viewport. ## Validation - `make fmt` - `bun test src/browser/hooks/useAutoScroll.test.tsx` - `bun test src/browser/features/Tools/Shared/useBashAutoExpand.test.tsx` - `bun test src/browser/components/ChatPane/LayoutStackLane.test.tsx` - `TEST_INTEGRATION=1 bun x jest tests/ui/chat/bottomLayoutShift.test.ts --runInBand --silent` - `make static-check` ## Risks Low. The production changes are small refactors in already-covered paths. The most behavior-adjacent changes are the bash auto-expand hook API and shared test scroll shim, both covered by targeted tests plus `make static-check`. --- _Generated with `mux` • Model: `openai:gpt-5.5` • Thinking: `xhigh` • Cost: `$386.28`_ <!-- mux-attribution: model=openai:gpt-5.5 thinking=xhigh costs=386.28 -->
1 parent ba13455 commit d1c0109

9 files changed

Lines changed: 123 additions & 221 deletions

File tree

src/browser/components/ChatPane/ChatPane.tsx

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -284,8 +284,7 @@ export const ChatPane: React.FC<ChatPaneProps> = (props) => {
284284
hasOlderHistory,
285285
loadingOlderHistory,
286286
} = workspaceState;
287-
const todoCount = workspaceState.todos.length;
288-
const shouldShowPinnedTodoList = todoCount > 0;
287+
const shouldShowPinnedTodoList = workspaceState.todos.length > 0;
289288
const shouldShowReviewsBanner = reviews.reviews.length > 0;
290289
const shouldRenderLoadOlderMessagesButton = hasOlderHistory && !isChromaticStorybookEnvironment();
291290
const loadOlderMessagesShortcutLabel = formatKeybind(KEYBINDS.LOAD_OLDER_MESSAGES);
@@ -1025,7 +1024,6 @@ export const ChatPane: React.FC<ChatPaneProps> = (props) => {
10251024
isQueuedAgentTask={isQueuedAgentTask}
10261025
isCompacting={isCompacting}
10271026
shouldShowPinnedTodoList={shouldShowPinnedTodoList}
1028-
todoCount={todoCount}
10291027
shouldShowReviewsBanner={shouldShowReviewsBanner}
10301028
canInterrupt={canInterrupt}
10311029
autoCompactionResult={autoCompactionResult}
@@ -1078,7 +1076,6 @@ interface ChatInputPaneProps {
10781076
isStreamStarting: boolean;
10791077
isHydratingTranscript: boolean;
10801078
shouldShowPinnedTodoList: boolean;
1081-
todoCount: number;
10821079
shouldShowReviewsBanner: boolean;
10831080
canInterrupt: boolean;
10841081
autoCompactionResult: ReturnType<typeof checkAutoCompaction>;

src/browser/components/ChatPane/LayoutStackLane.tsx

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ export const LayoutStackLane: React.FC<LayoutStackLaneProps> = (props) => {
3737
const contentRef = useRef<HTMLDivElement>(null);
3838
const stackHeightByWorkspaceIdRef = useRef(new Map<string, number>());
3939
const lastMeasuredStackHeightRef = useRef(0);
40-
const observedHeightRef = useRef<number | null>(null);
4140

4241
const hasItems = props.items.length > 0;
4342
const reservedStackHeightPx = getReservedLayoutStackHeightPx({
@@ -50,14 +49,11 @@ export const LayoutStackLane: React.FC<LayoutStackLaneProps> = (props) => {
5049
useLayoutEffect(() => {
5150
const content = contentRef.current;
5251
if (!content) {
53-
observedHeightRef.current = null;
5452
return;
5553
}
5654

5755
const observer = new ResizeObserver((entries) => {
5856
const nextHeight = measureLayoutStackHeightPx(content, entries[0]?.contentRect.height);
59-
observedHeightRef.current = nextHeight;
60-
6157
if (nextHeight === 0) {
6258
// Some owners (e.g. background-process dialogs) stay mounted while
6359
// rendering nothing. Only drop the reservation after hydration ends —
@@ -94,7 +90,6 @@ export const LayoutStackLane: React.FC<LayoutStackLaneProps> = (props) => {
9490
}
9591

9692
if (!hasItems) {
97-
observedHeightRef.current = 0;
9893
clearLayoutStackHeight(
9994
props.workspaceId,
10095
stackHeightByWorkspaceIdRef.current,
@@ -109,7 +104,6 @@ export const LayoutStackLane: React.FC<LayoutStackLaneProps> = (props) => {
109104
}
110105

111106
const settledHeightPx = measureLayoutStackHeightPx(content);
112-
observedHeightRef.current = settledHeightPx;
113107
if (settledHeightPx === 0) {
114108
clearLayoutStackHeight(
115109
props.workspaceId,

src/browser/features/Tools/BashToolCall.tsx

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ export const BashToolCall: React.FC<BashToolCallProps> = ({
5555
const { expanded, setExpanded, toggleExpanded } = useToolExpansion();
5656
const [outputDialogOpen, setOutputDialogOpen] = useState(false);
5757

58-
const resultHasOutput = typeof (result as { output?: unknown } | undefined)?.output === "string";
58+
const resultHasOutput = typeof result?.output === "string";
5959
const shouldTrackLiveBashState = Boolean(
6060
workspaceId &&
6161
toolCallId &&
@@ -78,6 +78,7 @@ export const BashToolCall: React.FC<BashToolCallProps> = ({
7878
shouldTrackLatestStreamingBash ? workspaceId : undefined
7979
);
8080
const isLatestStreamingBash = latestStreamingBashId === toolCallId;
81+
const hasReplacementStreamingBash = latestStreamingBashId !== null && !isLatestStreamingBash;
8182

8283
const outputRef = useRef<HTMLPreElement>(null);
8384
const outputPinnedRef = useRef(true);
@@ -94,7 +95,7 @@ export const BashToolCall: React.FC<BashToolCallProps> = ({
9495
const userToggledRef = useRef(false);
9596
useBashAutoExpand({
9697
isLatestStreamingBash,
97-
latestStreamingBashId,
98+
hasReplacementStreamingBash,
9899
status,
99100
startedAt,
100101
setExpanded,
@@ -109,7 +110,7 @@ export const BashToolCall: React.FC<BashToolCallProps> = ({
109110
// Override status for backgrounded processes: the aggregator sees success=true and marks "completed",
110111
// but for a foreground→background migration we want to show "backgrounded"
111112
const effectiveStatus: ToolStatus =
112-
status === "completed" && result && "backgroundProcessId" in result ? "backgrounded" : status;
113+
status === "completed" && backgroundProcessId !== null ? "backgrounded" : status;
113114

114115
const showLiveOutput =
115116
!isBackground && (status === "executing" || (Boolean(liveOutput) && !resultHasOutput));
@@ -134,7 +135,7 @@ export const BashToolCall: React.FC<BashToolCallProps> = ({
134135
const truncatedInfo = result && "truncated" in result ? result.truncated : undefined;
135136
const note = result && "note" in result ? result.note : undefined;
136137

137-
const isBackgroundResult = Boolean(result && "backgroundProcessId" in result);
138+
const isBackgroundResult = backgroundProcessId !== null;
138139
const completedOutput = isBackgroundResult ? undefined : result?.output;
139140
const completedHasOutput = typeof completedOutput === "string" && completedOutput.length > 0;
140141
const showCompletedOutputSection = !isBackgroundResult && (completedHasOutput || Boolean(note));
@@ -311,12 +312,12 @@ export const BashToolCall: React.FC<BashToolCallProps> = ({
311312
)}
312313

313314
{/* Background process info */}
314-
{result && "backgroundProcessId" in result && (
315+
{backgroundProcessId && (
315316
<div className="flex items-center gap-2 text-[11px]">
316317
<Layers size={12} className="text-muted shrink-0" />
317318
<span className="text-muted">Background process</span>
318319
<code className="rounded bg-[var(--color-bg-tertiary)] px-1.5 py-0.5 font-mono text-[10px]">
319-
{result.backgroundProcessId}
320+
{backgroundProcessId}
320321
</code>
321322
</div>
322323
)}

src/browser/features/Tools/Shared/useBashAutoExpand.test.tsx

Lines changed: 9 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
22
import { act, cleanup, renderHook } from "@testing-library/react";
3-
import type { Dispatch, SetStateAction, MutableRefObject } from "react";
4-
import { useState } from "react";
3+
import { useRef, useState } from "react";
54

65
import { installDom } from "../../../../../tests/ui/dom";
76
import { useBashAutoExpand } from "./useBashAutoExpand";
87
import type { ToolStatus } from "./toolUtils";
98

109
interface HarnessOptions {
1110
isLatestStreamingBash: boolean;
12-
latestStreamingBashId: string | null;
11+
hasReplacementStreamingBash?: boolean;
1312
status: ToolStatus;
1413
startedAt?: number;
1514
initialExpanded?: boolean;
@@ -18,27 +17,23 @@ interface HarnessOptions {
1817

1918
interface HarnessResult {
2019
expanded: boolean;
21-
setExpanded: Dispatch<SetStateAction<boolean>>;
22-
userToggledRef: MutableRefObject<boolean>;
2320
}
2421

2522
function useTestHarness(options: HarnessOptions): HarnessResult {
2623
const [expanded, setExpanded] = useState(options.initialExpanded ?? false);
2724
// Stable ref across renders so the harness simulates the parent component.
28-
const userToggledRef = useState(() => ({
29-
current: options.initialUserToggled ?? false,
30-
}))[0] as MutableRefObject<boolean>;
25+
const userToggledRef = useRef(options.initialUserToggled ?? false);
3126

3227
useBashAutoExpand({
3328
isLatestStreamingBash: options.isLatestStreamingBash,
34-
latestStreamingBashId: options.latestStreamingBashId,
29+
hasReplacementStreamingBash: options.hasReplacementStreamingBash ?? false,
3530
status: options.status,
3631
startedAt: options.startedAt,
3732
setExpanded,
3833
userToggledRef,
3934
});
4035

41-
return { expanded, setExpanded, userToggledRef };
36+
return { expanded };
4237
}
4338

4439
let cleanupDom: (() => void) | null = null;
@@ -58,7 +53,6 @@ describe("useBashAutoExpand", () => {
5853
const { result } = renderHook(() =>
5954
useTestHarness({
6055
isLatestStreamingBash: true,
61-
latestStreamingBashId: "tool-bash-1",
6256
status: "executing",
6357
startedAt: 0,
6458
})
@@ -73,7 +67,6 @@ describe("useBashAutoExpand", () => {
7367
const { result } = renderHook(() =>
7468
useTestHarness({
7569
isLatestStreamingBash: true,
76-
latestStreamingBashId: "tool-bash-1",
7770
status: "executing",
7871
startedAt: Date.now(),
7972
})
@@ -90,7 +83,6 @@ describe("useBashAutoExpand", () => {
9083
test("delays expand by 300ms for new in-chat bash transitions", async () => {
9184
const initialProps: HarnessOptions = {
9285
isLatestStreamingBash: false,
93-
latestStreamingBashId: null,
9486
status: "pending",
9587
};
9688
const { result, rerender } = renderHook((p: HarnessOptions) => useTestHarness(p), {
@@ -104,7 +96,6 @@ describe("useBashAutoExpand", () => {
10496
// expand immediately (flash protection).
10597
rerender({
10698
isLatestStreamingBash: true,
107-
latestStreamingBashId: "tool-bash-1",
10899
status: "executing",
109100
});
110101
expect(result.current.expanded).toBe(false);
@@ -120,7 +111,6 @@ describe("useBashAutoExpand", () => {
120111
const { result } = renderHook(() =>
121112
useTestHarness({
122113
isLatestStreamingBash: true,
123-
latestStreamingBashId: "tool-bash-1",
124114
status: "executing",
125115
initialUserToggled: true,
126116
})
@@ -132,7 +122,6 @@ describe("useBashAutoExpand", () => {
132122
test("auto-collapses when a different bash takes over", () => {
133123
const initialProps: HarnessOptions = {
134124
isLatestStreamingBash: true,
135-
latestStreamingBashId: "tool-bash-1",
136125
status: "executing",
137126
startedAt: 0,
138127
};
@@ -142,10 +131,10 @@ describe("useBashAutoExpand", () => {
142131
expect(result.current.expanded).toBe(true);
143132

144133
// A NEW bash starts streaming. From this row's perspective, it stops being
145-
// the latest streaming bash and a non-null id replaces it.
134+
// the latest streaming bash and should collapse back to its default state.
146135
rerender({
147136
isLatestStreamingBash: false,
148-
latestStreamingBashId: "tool-bash-2",
137+
hasReplacementStreamingBash: true,
149138
status: "executing",
150139
});
151140

@@ -155,7 +144,6 @@ describe("useBashAutoExpand", () => {
155144
test("does NOT auto-collapse when the bash itself completes", () => {
156145
const initialProps: HarnessOptions = {
157146
isLatestStreamingBash: true,
158-
latestStreamingBashId: "tool-bash-1",
159147
status: "executing",
160148
startedAt: 0,
161149
};
@@ -164,12 +152,10 @@ describe("useBashAutoExpand", () => {
164152
});
165153
expect(result.current.expanded).toBe(true);
166154

167-
// Bash finishes: latestStreamingBashId becomes null because there's no
168-
// bash currently streaming. The row should keep its expanded state so the
169-
// user can read the completed output without re-clicking.
155+
// Bash finishes with no replacement streaming. The row should stay expanded
156+
// so the user can read the completed output without re-clicking.
170157
rerender({
171158
isLatestStreamingBash: false,
172-
latestStreamingBashId: null,
173159
status: "completed",
174160
});
175161

@@ -180,7 +166,6 @@ describe("useBashAutoExpand", () => {
180166
const { result } = renderHook(() =>
181167
useTestHarness({
182168
isLatestStreamingBash: false,
183-
latestStreamingBashId: null,
184169
status: "pending",
185170
})
186171
);
Lines changed: 32 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,19 @@
1-
import { useEffect, useLayoutEffect, useRef } from "react";
1+
import { useLayoutEffect, useRef } from "react";
22

33
import type { ToolStatus } from "./toolUtils";
44

55
const FAST_COMMAND_FLASH_DELAY_MS = 300;
66

77
/**
8-
* Manages auto-expand/collapse of a bash tool row based on whether it's the
9-
* latest streaming bash in the workspace.
10-
*
11-
* Two distinct cases:
12-
*
13-
* 1. **Chat-open with an already-streaming bash.** The row mounts with
14-
* `isLatestStreamingBash && status === "executing"`. The bash has clearly
15-
* been running for some time (otherwise the chat wouldn't be in this
16-
* state when opened), so flash protection is not needed. Expand immediately
17-
* in a layout effect so the user does not perceive a delay between the chat
18-
* appearing and the bash output being readable.
19-
*
20-
* 2. **In-chat new bash.** A bash transitions to executing while the user is
21-
* already mounted in the chat. Delay the expand by 300 ms so commands that
22-
* complete in under that window don't briefly expand and re-collapse,
23-
* preventing a layout flash for fast-completing commands.
24-
*
25-
* The hook also auto-collapses the row when a new bash takes over and respects
26-
* any manual user toggle (a manual click pins the row's expanded state).
8+
* Keeps the latest streaming bash readable without flashing short commands:
9+
* hydrated rows that already outlived the flash window expand before paint,
10+
* while freshly-started rows wait for that same window before expanding.
11+
* Manual user toggles pin the row, and auto-expanded rows collapse only when a
12+
* different bash becomes the latest streaming bash.
2713
*/
2814
export function useBashAutoExpand(options: {
2915
isLatestStreamingBash: boolean;
30-
latestStreamingBashId: string | null;
16+
hasReplacementStreamingBash: boolean;
3117
status: ToolStatus;
3218
/** Timestamp from the tool part. Used to distinguish hydrated long-running rows from fresh mounts. */
3319
startedAt?: number;
@@ -37,7 +23,7 @@ export function useBashAutoExpand(options: {
3723
}): void {
3824
const {
3925
isLatestStreamingBash,
40-
latestStreamingBashId,
26+
hasReplacementStreamingBash,
4127
status,
4228
startedAt,
4329
setExpanded,
@@ -51,60 +37,52 @@ export function useBashAutoExpand(options: {
5137
const isFreshMountRef = useRef(true);
5238

5339
useLayoutEffect(() => {
40+
const clearExpandTimer = () => {
41+
if (expandTimerRef.current) {
42+
clearTimeout(expandTimerRef.current);
43+
expandTimerRef.current = null;
44+
}
45+
};
5446
const isFreshMount = isFreshMountRef.current;
5547
isFreshMountRef.current = false;
5648

57-
if (userToggledRef.current) return;
49+
if (userToggledRef.current) {
50+
return clearExpandTimer;
51+
}
5852

5953
if (isLatestStreamingBash && status === "executing") {
6054
const hasOutlivedFlashWindow =
6155
typeof startedAt === "number" && Date.now() - startedAt >= FAST_COMMAND_FLASH_DELAY_MS;
6256
if (isFreshMount && hasOutlivedFlashWindow) {
63-
// Chat-open with an already-running bash: expand synchronously before paint.
64-
// Fresh in-chat tool mounts have a current timestamp and still take the
65-
// delayed path below to preserve fast-command flash protection.
6657
setExpanded(true);
6758
wasAutoExpandedRef.current = true;
68-
return;
59+
return clearExpandTimer;
6960
}
7061

71-
if (wasAutoExpandedRef.current || expandTimerRef.current) return;
72-
73-
// In-chat new bash: delay expand to suppress flash for fast-completing commands.
74-
expandTimerRef.current = setTimeout(() => {
75-
expandTimerRef.current = null;
76-
if (!userToggledRef.current) {
77-
setExpanded(true);
78-
wasAutoExpandedRef.current = true;
79-
}
80-
}, FAST_COMMAND_FLASH_DELAY_MS);
81-
return;
62+
if (!wasAutoExpandedRef.current && !expandTimerRef.current) {
63+
expandTimerRef.current = setTimeout(() => {
64+
expandTimerRef.current = null;
65+
if (!userToggledRef.current) {
66+
setExpanded(true);
67+
wasAutoExpandedRef.current = true;
68+
}
69+
}, FAST_COMMAND_FLASH_DELAY_MS);
70+
}
71+
return clearExpandTimer;
8272
}
8373

84-
// No longer the latest streaming bash: cancel any pending expand and collapse
85-
// if a NEW bash took over (latestStreamingBashId !== null && !== us).
86-
if (expandTimerRef.current) {
87-
clearTimeout(expandTimerRef.current);
88-
expandTimerRef.current = null;
89-
}
90-
if (wasAutoExpandedRef.current && latestStreamingBashId !== null) {
74+
clearExpandTimer();
75+
if (wasAutoExpandedRef.current && hasReplacementStreamingBash) {
9176
setExpanded(false);
9277
wasAutoExpandedRef.current = false;
9378
}
79+
return undefined;
9480
}, [
9581
isLatestStreamingBash,
96-
latestStreamingBashId,
82+
hasReplacementStreamingBash,
9783
status,
9884
startedAt,
9985
setExpanded,
10086
userToggledRef,
10187
]);
102-
103-
useEffect(() => {
104-
return () => {
105-
if (expandTimerRef.current) {
106-
clearTimeout(expandTimerRef.current);
107-
}
108-
};
109-
}, []);
11088
}

0 commit comments

Comments
 (0)