mirror of
https://github.com/coder/coder.git
synced 2026-10-31 02:43:21 +08:00
fix(site): measure virtualized log row heights (#28251)
> 🤖 This PR was written by Coder Agents on behalf of Jake Howell. Fixes coder/coder#25692. Closes coder/coder#25692. Supersedes coder/coder#28250. ## Problem `AgentLogs` virtualizes rows with a **fixed** `itemSize` (20px), so react-window sets the total scroll height to `itemCount * 20`. Log entries that contain newlines render taller than 20px, so the estimate falls short: tall rows overlap the rows below them and the true bottom sits past where `scrollToItem(..., "end")` can reach. The last lines become unreachable. ## Fix Switch to `VariableSizeList` and measure each row with a `ResizeObserver`, feeding the real height back via `resetAfterIndex`. The total scroll height now tracks the real content, so the bottom is always reachable. `AgentRow` and `TaskPage` only change the ref type; their existing autoscroll now lands correctly. ## Before / After Dogfooded on a local instance with a workspace whose agent logs include multi-line entries (241 entries, every 30th a 6-line block, plus a final bottom-marker line). Same workspace, same scroll interaction, `main` vs this branch: <!-- linear:table-colwidths:266,266,266 --> | | Before (`main`, `FixedSizeList`) | After (this PR, `VariableSizeList`) | | -- | -- | -- | | **A multi-line entry** | Painted into a 20px slot, so it collides with the rows above and below (text on top of text). | Rendered at full height in its own space, neighbours no longer overlap. | | |  |  | | **Scrolled to the bottom** | Total height is short, so normal scrolling stops early: the final marker line is clipped and unreachable. | Height tracks real content: the final marker line is fully visible and reachable. | | |  |  | ## Validation The new `AgentLogs.test.tsx` renders rows measured taller (40px) than the estimate and asserts the scroll area is sized from measured heights. Run against the `main` implementation it fails, against this branch it passes: ``` # main (FixedSizeList): scroll area = 5 rows * 20px - Expected 200 + Received 100 → FAIL # this branch (VariableSizeList): scroll area = 5 rows * measured 40px ✓ AgentLogs.test.tsx (1 test) → PASS ``` Also green: `tsc`, `biome check`, and the AgentRow (25) + TaskPage (46) + AgentLogs (3) Storybook interaction tests. <details><summary>Dogfood details and decision log</summary> **Dogfood setup** (SHA `1228dcfe8d`, `./scripts/develop.sh`): template `lognl2` (a `coder_agent` on a `null_resource` so the agent authenticates against a real build), workspace `lognl-demo2`. Posted 241 log entries via `PATCH /api/v2/workspaceagents/me/logs`, every 30th a 6-line `\n` entry plus a bottom marker; confirmed the 8 multi-line entries persisted with embedded newlines. On the fixed build, at maximum scroll the DevTools console reported `scrollTop 5204`, `scrollHeight 5460`, `clientHeight 256`, `remaining 0`, marker clipped `0px`; height is self-consistent (`innerHeight 5428` + 16px top + 16px bottom padding = `5460`). **Decision log** * coder/coder#28250 treated this as an autoscroll problem. Wrong: the height calculation is the bug, so no scroll target can reach a bottom placed out of range. Reverted. * Only embedded newlines make a row exceed 20px; long lines do not wrap (they overflow horizontally). A fixed constant can't cover a variable line count, so measuring is the robust fix. * `resetAfterIndex` can't run from a row's first-mount measurement effect because the list ref is assigned after child rows commit; the lowest changed index is recorded and flushed from a parent layout effect. </details> Evidence images are hosted on the `devex-407-evidence` branch of the author's fork (`jakehwll/coder`) to keep this repo's diff clean.
This commit is contained in:
@@ -7,7 +7,7 @@ export const AGENT_LOG_LINE_HEIGHT = 20;
|
||||
|
||||
interface AgentLogLineProps {
|
||||
line: Line;
|
||||
style: React.CSSProperties;
|
||||
style?: React.CSSProperties;
|
||||
sourceIcon: ReactNode;
|
||||
}
|
||||
|
||||
@@ -28,7 +28,7 @@ export const AgentLogLine: FC<AgentLogLineProps> = ({
|
||||
}, [line.time]);
|
||||
|
||||
return (
|
||||
<LogLine className="pl-4" level={line.level} style={style}>
|
||||
<LogLine className="pl-4 min-h-5" level={line.level} style={style}>
|
||||
{sourceIcon}
|
||||
<LogLinePrefix>{timestamp}</LogLinePrefix>
|
||||
<AnsiHtml text={output} />
|
||||
|
||||
@@ -0,0 +1,86 @@
|
||||
import { render, waitFor } from "@testing-library/react";
|
||||
import type { Line } from "#/components/Logs/LogLine";
|
||||
import { AGENT_LOG_LINE_HEIGHT } from "./AgentLogLine";
|
||||
import { AgentLogs } from "./AgentLogs";
|
||||
import { MockSources } from "./mocks";
|
||||
|
||||
// A real log row renders taller than the AGENT_LOG_LINE_HEIGHT estimate (e.g.
|
||||
// when output wraps). We simulate that by reporting a fixed measured height
|
||||
// that is larger than the estimate.
|
||||
const MEASURED_ROW_HEIGHT = 40;
|
||||
const ROW_COUNT = 5;
|
||||
|
||||
const makeLogs = (count: number): Line[] =>
|
||||
Array.from({ length: count }, (_, i) => ({
|
||||
id: i + 1,
|
||||
level: "info",
|
||||
output: `log line ${i + 1}`,
|
||||
sourceId: MockSources[0].id,
|
||||
time: "2024-03-14T11:31:04.090715Z",
|
||||
}));
|
||||
|
||||
const getInnerSizer = (container: HTMLElement): HTMLElement => {
|
||||
const outer = container.querySelector<HTMLElement>(
|
||||
'div[style*="overflow: auto"]',
|
||||
);
|
||||
const inner = outer?.firstElementChild as HTMLElement | null;
|
||||
if (!inner) {
|
||||
throw new Error("react-window inner sizing element not found");
|
||||
}
|
||||
return inner;
|
||||
};
|
||||
|
||||
describe("AgentLogs virtualized height", () => {
|
||||
let boundingRect: ReturnType<typeof vi.spyOn>;
|
||||
|
||||
beforeAll(() => {
|
||||
// jsdom performs no layout, so make every row report a height that is
|
||||
// taller than the fixed estimate. The list must measure and use this
|
||||
// height rather than assuming AGENT_LOG_LINE_HEIGHT per row.
|
||||
boundingRect = vi
|
||||
.spyOn(HTMLElement.prototype, "getBoundingClientRect")
|
||||
.mockReturnValue({
|
||||
height: MEASURED_ROW_HEIGHT,
|
||||
width: 600,
|
||||
top: 0,
|
||||
left: 0,
|
||||
right: 600,
|
||||
bottom: MEASURED_ROW_HEIGHT,
|
||||
x: 0,
|
||||
y: 0,
|
||||
toJSON: () => ({}),
|
||||
} as DOMRect);
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
boundingRect.mockRestore();
|
||||
});
|
||||
|
||||
it("sizes the scroll area from measured row heights, not the fixed estimate", async () => {
|
||||
const { container } = render(
|
||||
<AgentLogs
|
||||
logs={makeLogs(ROW_COUNT)}
|
||||
sources={MockSources}
|
||||
overflowed={false}
|
||||
showSourceIcons={false}
|
||||
// Tall enough to render (and therefore measure) every row.
|
||||
height={1000}
|
||||
width={600}
|
||||
/>,
|
||||
);
|
||||
|
||||
// The virtualized height must account for the real, taller rows.
|
||||
// react-window's fixed itemSize would produce ROW_COUNT *
|
||||
// AGENT_LOG_LINE_HEIGHT, which is short of the real content and is what
|
||||
// left the last lines unreachable (coder/coder#25692).
|
||||
await waitFor(() => {
|
||||
const totalHeight = Number.parseFloat(
|
||||
getInnerSizer(container).style.height,
|
||||
);
|
||||
expect(totalHeight).toBe(ROW_COUNT * MEASURED_ROW_HEIGHT);
|
||||
});
|
||||
expect(
|
||||
Number.parseFloat(getInnerSizer(container).style.height),
|
||||
).toBeGreaterThan(ROW_COUNT * AGENT_LOG_LINE_HEIGHT);
|
||||
});
|
||||
});
|
||||
@@ -1,5 +1,14 @@
|
||||
import type { JSX } from "react";
|
||||
import { FixedSizeList as List } from "react-window";
|
||||
import {
|
||||
type CSSProperties,
|
||||
type FC,
|
||||
type JSX,
|
||||
type ReactNode,
|
||||
type Ref,
|
||||
useCallback,
|
||||
useLayoutEffect,
|
||||
useRef,
|
||||
} from "react";
|
||||
import { VariableSizeList as List } from "react-window";
|
||||
import type { WorkspaceAgentLogSource } from "#/api/typesGenerated";
|
||||
import { Badge } from "#/components/Badge/Badge";
|
||||
import { ExternalImage } from "#/components/ExternalImage/ExternalImage";
|
||||
@@ -24,32 +33,95 @@ const fallbackLog: WorkspaceAgentLogSource = {
|
||||
};
|
||||
|
||||
type AgentLogsProps = Omit<
|
||||
React.ComponentPropsWithRef<typeof List>,
|
||||
React.ComponentPropsWithoutRef<typeof List>,
|
||||
"children" | "itemSize" | "itemCount" | "itemKey"
|
||||
> & {
|
||||
logs: readonly Line[];
|
||||
sources: readonly WorkspaceAgentLogSource[];
|
||||
overflowed: boolean;
|
||||
showSourceIcons?: boolean;
|
||||
ref?: Ref<List>;
|
||||
};
|
||||
|
||||
export const AgentLogs: React.FC<AgentLogsProps> = ({
|
||||
export const AgentLogs: FC<AgentLogsProps> = ({
|
||||
logs,
|
||||
sources,
|
||||
overflowed,
|
||||
className,
|
||||
showSourceIcons = true,
|
||||
ref,
|
||||
...listProps
|
||||
}) => {
|
||||
const logSourceById = Object.fromEntries(sources.map((s) => [s.id, s]));
|
||||
const getLogSource = (id: string) => logSourceById[id] || fallbackLog;
|
||||
|
||||
const listRef = useRef<List>(null);
|
||||
const mergeListRef = useCallback(
|
||||
(instance: List | null) => {
|
||||
listRef.current = instance;
|
||||
if (typeof ref === "function") {
|
||||
ref(instance);
|
||||
} else if (ref) {
|
||||
ref.current = instance;
|
||||
}
|
||||
},
|
||||
[ref],
|
||||
);
|
||||
|
||||
// A log line's real height depends on its content (long lines wrap, so a
|
||||
// single log entry can span multiple visual rows). A fixed itemSize makes
|
||||
// react-window compute the total scroll height as itemCount * itemSize,
|
||||
// which drifts further from reality with every taller-than-expected row.
|
||||
// With enough rows the estimated height falls short of the real content
|
||||
// and the list can no longer scroll to the last line (coder/coder#25692).
|
||||
// Measuring each row and feeding the true height back keeps the total
|
||||
// height accurate regardless of how many rows there are.
|
||||
const sizeMapRef = useRef<Map<number, number>>(new Map());
|
||||
const pendingResetIndexRef = useRef<number | null>(null);
|
||||
// resetAfterIndex tells react-window to recompute offsets from the first
|
||||
// row whose height changed. The list ref is assigned after the child rows
|
||||
// commit, so on the initial mount we can't reset from within a row's
|
||||
// measurement effect; instead we record the lowest changed index and flush
|
||||
// it from a parent layout effect once the ref is available.
|
||||
const flushPendingReset = useCallback(() => {
|
||||
const index = pendingResetIndexRef.current;
|
||||
if (index !== null && listRef.current) {
|
||||
pendingResetIndexRef.current = null;
|
||||
listRef.current.resetAfterIndex(index);
|
||||
}
|
||||
}, []);
|
||||
const setRowHeight = useCallback(
|
||||
(index: number, height: number) => {
|
||||
if (sizeMapRef.current.get(index) === height) {
|
||||
return;
|
||||
}
|
||||
sizeMapRef.current.set(index, height);
|
||||
pendingResetIndexRef.current =
|
||||
pendingResetIndexRef.current === null
|
||||
? index
|
||||
: Math.min(pendingResetIndexRef.current, index);
|
||||
flushPendingReset();
|
||||
},
|
||||
[flushPendingReset],
|
||||
);
|
||||
const getRowHeight = useCallback(
|
||||
(index: number) => sizeMapRef.current.get(index) ?? AGENT_LOG_LINE_HEIGHT,
|
||||
[],
|
||||
);
|
||||
|
||||
// Flush measurements taken before the list ref existed (initial mount).
|
||||
useLayoutEffect(() => {
|
||||
flushPendingReset();
|
||||
});
|
||||
|
||||
return (
|
||||
<div className="bg-surface-secondary relative">
|
||||
<List
|
||||
{...listProps}
|
||||
ref={mergeListRef}
|
||||
itemCount={logs.length}
|
||||
itemSize={AGENT_LOG_LINE_HEIGHT}
|
||||
itemSize={getRowHeight}
|
||||
estimatedItemSize={AGENT_LOG_LINE_HEIGHT}
|
||||
itemKey={(index) => logs[index]?.id || index}
|
||||
// We need the div selector to be able to apply the padding
|
||||
// top from startupLogs
|
||||
@@ -125,26 +197,31 @@ export const AgentLogs: React.FC<AgentLogsProps> = ({
|
||||
}
|
||||
|
||||
return (
|
||||
<AgentLogLine
|
||||
line={log}
|
||||
<MeasuredLogRow
|
||||
index={index}
|
||||
style={style}
|
||||
sourceIcon={
|
||||
showSourceIcons ? (
|
||||
<Tooltip>
|
||||
<TooltipTrigger asChild>{icon}</TooltipTrigger>
|
||||
<TooltipContent side="bottom">
|
||||
{logSource.display_name}
|
||||
{assignedIcon && (
|
||||
<i>
|
||||
<br />
|
||||
No icon specified!
|
||||
</i>
|
||||
)}
|
||||
</TooltipContent>
|
||||
</Tooltip>
|
||||
) : null
|
||||
}
|
||||
/>
|
||||
onMeasure={setRowHeight}
|
||||
>
|
||||
<AgentLogLine
|
||||
line={log}
|
||||
sourceIcon={
|
||||
showSourceIcons ? (
|
||||
<Tooltip>
|
||||
<TooltipTrigger asChild>{icon}</TooltipTrigger>
|
||||
<TooltipContent side="bottom">
|
||||
{logSource.display_name}
|
||||
{assignedIcon && (
|
||||
<i>
|
||||
<br />
|
||||
No icon specified!
|
||||
</i>
|
||||
)}
|
||||
</TooltipContent>
|
||||
</Tooltip>
|
||||
) : null
|
||||
}
|
||||
/>
|
||||
</MeasuredLogRow>
|
||||
);
|
||||
}}
|
||||
</List>
|
||||
@@ -180,6 +257,50 @@ export const AgentLogs: React.FC<AgentLogsProps> = ({
|
||||
);
|
||||
};
|
||||
|
||||
interface MeasuredLogRowProps {
|
||||
index: number;
|
||||
// react-window's positioning style for the row (absolute top/left/width).
|
||||
style: CSSProperties;
|
||||
onMeasure: (index: number, height: number) => void;
|
||||
children: ReactNode;
|
||||
}
|
||||
|
||||
// Wraps a log line and reports its rendered height back to the virtualized
|
||||
// list. The height is left to the content (`height: auto`) so wrapped or
|
||||
// multi-line output is measured accurately instead of being assumed to be a
|
||||
// single fixed-height row.
|
||||
const MeasuredLogRow: FC<MeasuredLogRowProps> = ({
|
||||
index,
|
||||
style,
|
||||
onMeasure,
|
||||
children,
|
||||
}) => {
|
||||
const rowRef = useRef<HTMLDivElement>(null);
|
||||
|
||||
useLayoutEffect(() => {
|
||||
const el = rowRef.current;
|
||||
if (!el) {
|
||||
return;
|
||||
}
|
||||
const report = () => {
|
||||
const height = el.getBoundingClientRect().height;
|
||||
if (height > 0) {
|
||||
onMeasure(index, height);
|
||||
}
|
||||
};
|
||||
report();
|
||||
const observer = new ResizeObserver(report);
|
||||
observer.observe(el);
|
||||
return () => observer.disconnect();
|
||||
}, [index, onMeasure]);
|
||||
|
||||
return (
|
||||
<div ref={rowRef} style={{ ...style, height: "auto" }}>
|
||||
{children}
|
||||
</div>
|
||||
);
|
||||
};
|
||||
|
||||
// These colors were picked at random. Feel free to add more, adjust, or change!
|
||||
// Users will not depend on these colors.
|
||||
const scriptDisplayColors: readonly string[] = [
|
||||
|
||||
@@ -17,7 +17,7 @@ import {
|
||||
} from "react";
|
||||
import { Link as RouterLink } from "react-router";
|
||||
import AutoSizer from "react-virtualized-auto-sizer";
|
||||
import type { FixedSizeList as List, ListOnScrollProps } from "react-window";
|
||||
import type { VariableSizeList as List, ListOnScrollProps } from "react-window";
|
||||
import type {
|
||||
AgentScriptTiming,
|
||||
Template,
|
||||
|
||||
@@ -18,7 +18,7 @@ import {
|
||||
import { useMutation, useQuery, useQueryClient } from "react-query";
|
||||
import { Panel, PanelGroup, PanelResizeHandle } from "react-resizable-panels";
|
||||
import { Link as RouterLink, useParams } from "react-router";
|
||||
import type { FixedSizeList } from "react-window";
|
||||
import type { VariableSizeList } from "react-window";
|
||||
import { toast } from "sonner";
|
||||
import { API } from "#/api/api";
|
||||
import { getErrorDetail, getErrorMessage, isApiError } from "#/api/errors";
|
||||
@@ -830,7 +830,7 @@ type TaskStartingAgentProps = {
|
||||
|
||||
const TaskStartingAgent: FC<TaskStartingAgentProps> = ({ task, agent }) => {
|
||||
const logs = useAgentLogs({ agentId: agent.id });
|
||||
const listRef = useRef<FixedSizeList>(null);
|
||||
const listRef = useRef<VariableSizeList>(null);
|
||||
const queryClient = useQueryClient();
|
||||
const pauseMutation = useMutation({
|
||||
...pauseTask(task, queryClient),
|
||||
|
||||
Reference in New Issue
Block a user