Make chat timeline height estimates width-aware with attachment-aware tests - #167
Conversation
- extract `estimateTimelineMessageHeight` into `timelineHeight.ts` - account for timeline width and explicit newlines when estimating wrapped lines - re-measure virtualized rows on width changes to fix attachment/message height sizing - add Vitest coverage for assistant/user wrapping and attachment row height rules
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: ResizeObserver never activates for initially empty threads
- Replaced useRef with a state-backed callback ref (useState + setTimelineRootNode as the ref callback) so the useLayoutEffect re-runs when the timeline div mounts after transitioning from the empty-thread state.
Or push these changes by commenting:
@cursor push fd13dceb67
Preview (fd13dceb67)
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -3187,12 +3187,11 @@
onImageExpand,
markdownCwd,
}: MessagesTimelineProps) {
- const timelineRootRef = useRef<HTMLDivElement | null>(null);
+ const [timelineRootNode, setTimelineRootNode] = useState<HTMLDivElement | null>(null);
const [timelineWidthPx, setTimelineWidthPx] = useState<number | null>(null);
useLayoutEffect(() => {
- const timelineRoot = timelineRootRef.current;
- if (!timelineRoot) return;
+ if (!timelineRootNode) return;
const updateWidth = (nextWidth: number) => {
setTimelineWidthPx((previousWidth) => {
@@ -3203,7 +3202,7 @@
});
};
- updateWidth(timelineRoot.getBoundingClientRect().width);
+ updateWidth(timelineRootNode.getBoundingClientRect().width);
if (typeof ResizeObserver === "undefined") return;
const observer = new ResizeObserver((entries) => {
@@ -3211,11 +3210,11 @@
if (!entry) return;
updateWidth(entry.contentRect.width);
});
- observer.observe(timelineRoot);
+ observer.observe(timelineRootNode);
return () => {
observer.disconnect();
};
- }, []);
+ }, [timelineRootNode]);
const rows = useMemo<TimelineRow[]>(() => {
const nextRows: TimelineRow[] = [];
@@ -3645,7 +3644,7 @@
}
return (
- <div ref={timelineRootRef} className="mx-auto w-full min-w-0 max-w-3xl overflow-x-hidden">
+ <div ref={setTimelineRootNode} className="mx-auto w-full min-w-0 max-w-3xl overflow-x-hidden">
{virtualizedRowCount > 0 && (
<div className="relative" style={{ height: `${rowVirtualizer.getTotalSize()}px` }}>
{virtualRows.map((virtualRow: VirtualItem) => {- add browser E2E tests validating timeline height estimator against rendered DOM - configure Playwright test runner and scripts in `apps/web` - run browser tests in CI with Playwright browser caching and install step
- Replace Playwright e2e timeline-height test setup with Vitest browser config - Add `ChatView.browser.tsx` parity tests for text wrapping and attachment height estimation - Add row data attributes in `ChatView` to support in-browser measurement targeting
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Inconsistent width measurement APIs cause initial value mismatch
- Replaced
getBoundingClientRect().width(border-box) withclientWidth - paddingLeft - paddingRight(content-box) for the initial measurement, making it consistent with the ResizeObserver'scontentRect.width.
- Replaced
Or push these changes by commenting:
@cursor push e1d5bbad14
Preview (e1d5bbad14)
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -3203,7 +3203,12 @@
});
};
- updateWidth(timelineRoot.getBoundingClientRect().width);
+ const cs = getComputedStyle(timelineRoot);
+ updateWidth(
+ timelineRoot.clientWidth -
+ parseFloat(cs.paddingLeft) -
+ parseFloat(cs.paddingRight),
+ );
if (typeof ResizeObserver === "undefined") return;
const observer = new ResizeObserver((entries) => {- Replace mocked router hooks with a memory router + RouterProvider - Render ChatView through a test route to exercise attachment height layout in realistic routing context
- Run ChatView browser tests through the real app router/store stack - Add MSW worker setup and mocked WS/attachment responses for full-app rendering - Extract shared `getRouter` setup and add a timeline root data hook for reliable measurement
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Document title no longer set after refactor
- Fixed the meta format from
{ name: "title", content: ... }to{ title: APP_DISPLAY_NAME }and added the missing<HeadContent />component to both render paths in the root route so TanStack Router's head management actually takes effect.
- Fixed the meta format from
Or push these changes by commenting:
@cursor push 7f77794bdd
Preview (7f77794bdd)
diff --git a/apps/web/src/routes/__root.tsx b/apps/web/src/routes/__root.tsx
--- a/apps/web/src/routes/__root.tsx
+++ b/apps/web/src/routes/__root.tsx
@@ -1,5 +1,6 @@
import { ThreadId } from "@t3tools/contracts";
import {
+ HeadContent,
Outlet,
createRootRouteWithContext,
type ErrorComponentProps,
@@ -27,26 +28,30 @@
component: RootRouteView,
errorComponent: RootRouteErrorView,
head: () => ({
- meta: [{ name: "title", content: APP_DISPLAY_NAME }],
+ meta: [{ title: APP_DISPLAY_NAME }],
}),
});
function RootRouteView() {
if (!readNativeApi()) {
return (
- <div className="flex h-screen flex-col bg-background text-foreground">
- <div className="flex flex-1 items-center justify-center">
- <p className="text-sm text-muted-foreground">
- Connecting to {APP_DISPLAY_NAME} server...
- </p>
+ <>
+ <HeadContent />
+ <div className="flex h-screen flex-col bg-background text-foreground">
+ <div className="flex flex-1 items-center justify-center">
+ <p className="text-sm text-muted-foreground">
+ Connecting to {APP_DISPLAY_NAME} server...
+ </p>
+ </div>
</div>
- </div>
+ </>
);
}
return (
<ToastProvider>
<AnchoredToastProvider>
+ <HeadContent />
<EventRouter />
<DesktopProjectBootstrap />
<Outlet />- replace manual `createRoot` mounting with `render`/`unmount` from vitest-browser-react - add `vitest-browser-react` to web devDependencies and lockfile
- Replace manual timeout polling loops with `vi.waitFor` in test helpers - Improve reliability of locating and measuring virtualized user rows in attachment-height tests
- recompute timeline width from the root element on ResizeObserver updates - rerun width effect when message/working state changes - reduce minimum user chars per line for narrow layouts and add regression test
- Add viewport matrix coverage for long text and attachment row-height parity - Reuse a mount/measure test harness to validate resize behavior in one session - Ensure production CSS and viewport sizing are applied before measurements
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Condition inversion changes system message height estimation
- Changed
message.role !== "user"back tomessage.role === "assistant"so system messages use user sizing rules (matching the original behavior) instead of being incorrectly routed to assistant sizing.
- Changed
Or push these changes by commenting:
@cursor push 7fbf52b7af
Preview (7fbf52b7af)
diff --git a/apps/web/src/components/timelineHeight.ts b/apps/web/src/components/timelineHeight.ts
--- a/apps/web/src/components/timelineHeight.ts
+++ b/apps/web/src/components/timelineHeight.ts
@@ -67,7 +67,7 @@
message: TimelineMessageHeightInput,
layout: TimelineHeightEstimateLayout = { timelineWidthPx: null },
): number {
- if (message.role !== "user") {
+ if (message.role === "assistant") {
const charsPerLine = estimateCharsPerLineForAssistant(layout.timelineWidthPx);
const estimatedLines = estimateWrappedLineCount(message.text, charsPerLine);
return ASSISTANT_BASE_HEIGHT_PX + estimatedLines * LINE_HEIGHT_PX;- Route `system` messages through assistant height estimation - Keep user attachment height logic scoped to user messages - Add regression test for system-message sizing behavior
Merges `pingdotgg/t3code` into the fork: `0c5771d60` → `1bbca0e78`, 32 commits, landed as a merge commit. `201` files changed in the upstream range and `204` landed. The gap is exactly three files, all fork docs written by this merge — `docs/fork/gaps.md`, `inventory.json` and `upstream-merge-log.md`. Nothing in the range failed to land. ## Conflicts Nine files, each resolved to the verdict `preflight.mjs` printed. No conflict was left undecided, so this is not a draft. Most of them are one upstream change hitting fork gates from several sides: the compact-sidebar work (pingdotgg#11525, pingdotgg#9417, pingdotgg#11644) rewrote `Sidebar.tsx`, `LegacySidebar.tsx`, `ui/sidebar.tsx`, `SidebarThreadHeader.tsx` and `ThreadStatusIndicators.tsx`. Took upstream's structure whole in each and re-applied the fork delta at its anchor; four of the five were import-block unions or a single re-placed element. `AGENTS.md` — upstream's only change in range was the reusable dev credential, restated into the fork's own prose rather than taken as a block. `auth.ts` — took upstream's new `urlCredential` parameter on top of the fork's Moatless cookie-session probe; a desktop credential is now exchanged only when the server advertises `desktop-bootstrap`, which the Moatless auth descriptor never does, so a stale one is never sent. `pnpm-lock.yaml` was reset to `upstream/main` and the fork edges re-derived by `vp i`. ### Two things that did not announce themselves **A gated control was replaced, so the gate had to move rather than survive.** pingdotgg#11678 deleted the `enableLegacyTokenStreaming` switch the fork gates and put a three-way `responseStreamingMode` select in its place. Taking upstream drops the gate along with the row it sat on and un-hides the replacement — no conflict marker, and no test covers a surface that should not render. The gate moved onto the new row. The same check turned up its second half: upstream also added a `response-streaming` entry to `settingsSearch.ts`, and a gate on a row does not reach the command palette, so the result would have landed on General at a hash for a control that is not rendered. Added `assistantStreamingOnly` beside the existing `providerConfigurationOnly` filter. **`authBootstrap.test.ts` auto-merged into something neither side wrote.** Upstream's two new tests assert `status: "requires-auth"`; the fork renamed that gate status to `requires-login`. No marker and no script catches this — `resolution-check.mjs` documents it as the case it cannot see. The fork's own suite was the only thing that caught it. ## What the merge brings in ### Usable as-is **Sidebar and chrome (pure client, no RPC).** `ca2cc1339` compact sidebar rail, `77bca8b2d` compact thread list mode, `df7ccc8fd` compact thread row badges, `0118b5229` sparse sidebar shelves, `46140c96a` unified panel resizing, `3689c98d2` hover-highlight fix. The two new settings keys are `ClientSettings`, which the fork persists to `localStorage` through `splitPatch`, so nothing reaches `server.updateSettings`. **`7b6109988` — linked pull request in the compact rail.** Resolves through `pullRequests.summary`, the one method in that group Moatless serves, and opens a surface gated on `capabilities.threadPullRequests`, which Moatless reports. **`2ec59ca1f` — hide the back button for a single linked pull request.** Same capability, so this surface is live here and the fix applies. **`42b6bcc6f` — opt-in in-app thread notifications.** Derived entirely from the `environmentShell` snapshot; the toggle is a `ClientSettings` key. No new RPC. **`6e5e986f1` — badge background thread notifications.** The web half rides `navigator.setAppBadge` and window focus; the desktop IPC half is inert in a browser tab. **`4a39cade9` / `e62868393` — routing recovery.** A `notFoundComponent`, and `router.invalidate()` in place of `reset()` so startup retries once the server is back. Both improve the fork's failure path directly. **`8b3ddf51c`** — `resolveProjectSettings` accepts a null project; filed as a mobile fix, but the helper is shared and the looser signature is safer for every caller. **Mobile-only, in tree but unverified against Moatless** (`564719165`, `9086a1f71`, `17f8e2a8a`, `0a91b9a11`, `20a8f1de3`): merge cleanly and change nothing this fork tests — see _Mobile testing against Moatless is undocumented_ in gaps.md. ### Unsupported in Moatless / needs implementation **`1bbca0e78` — response streaming mode.** `apps/web/src/components/settings/SettingsPanels.tsx`, `settingsSearch.ts`, `packages/contracts/src/settings.ts`. Methods: `server.updateSettings` (refuses), `server.getSettings` (answers `{}`). Flag: `FEATURES.assistantStreaming`. Gated in this PR, palette entry filtered. Closing it properly means Moatless dispatching `server.updateSettings` *and* honouring the mode server-side — see the backend bucket below. **`5e961d3d7` + `f26198d79` — the Connections page, flattened and then reorganized by environment.** `ConnectionsSettings.tsx`, `EnvironmentRow.tsx`, `LoadBalancingSettings.tsx`, `CloudEnvironmentConnectList.tsx` and friends. Flag: `FEATURES.connections`, via `FEATURE_BY_SETTINGS_PATH["/settings/connections"]` — the nav entry is dropped and a typed URL redirects. Both rewrites land entirely on a page this build does not show: device pairing, SSH environments, WSL, server exposure. `CloudEnvironmentConnectList` is T3 Connect, which the inventory's `cloud-relay-connect` concern says not to adopt. Nothing to implement unless the fork grows a multi-environment story. **`db6e0531e` — route pull request operations across matching GitHub accounts.** New: `packages/client-runtime/src/state/pullRequestRouting.ts`, `connection/githubRoutingPermissions.ts`, `apps/web/src/components/settings/GitHubRoutingSettings.tsx`, plus `apps/server/src/pullRequest/*`. Methods: `pullRequests.routing` and `pullRequests.routingIdentity`, both declaring `PullRequestRpcError` — so they arrived already refusing and the derivation reports nothing to add. `routingIdentity` answers which GitHub account an environment is authenticated as; `routing` picks the environment whose account can act on a given PR. That is a git-host integration Moatless does not have, and it means nothing before the `pullRequests.*` group underneath it is served, so it closes with the group rather than separately. **`66e39ca2a` — apply device settings to selected environments.** `DeviceHostsSettings.tsx`, `IntegrationsSettings.tsx`, `deviceHostsSettings.logic.ts`. Methods: `server.updateSettings` plus the whole `device.*` group (`configure`, `list`, `testHost`, `open`, `close`, `shutdown`, `detail`, `action`) and the `subscribeDeviceState` stream — all nine refuse. Flag: `FEATURES.deviceHub`. Rewires device toggles onto `useScopedSettings` so they fan out to the selected scope. Closing this means Moatless running an iOS Simulator or Android emulator host for a task — real work, since the hub needs Xcode or the Android SDK on the host plus a second stream beside the RPC connection. **`d7c71f91d` — float the pull request comment composer.** New `PullRequestCommentComposer.tsx`. Methods: `pullRequests.comment`, on a panel fed by `pullRequests.detail` and `.activity`. Flag: `FEATURES.pullRequestSurface`, plus `capabilities.pullRequests`, which Moatless does not report — `ChatView` renders the unavailable state before the composer can mount. **`20363c32c` — provider selector in the pull request toolbar.** `routes/_chat.pull-requests.tsx`. No fork gate needed and none added: the route computes support from `capabilities.pullRequests`, so with none it short-circuits and the new menu never renders. ### Backend behavior to consider reproducing in Moatless **`c07575f57` — deliver finished paragraphs and closed code blocks mid-turn.** `apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts`. The most interesting item in this range. Upstream splits the buffered assistant message at the last blank line or closing fence that is *not* inside an open code block, flushing at most once every 400 ms. That needs no token stream — it is a buffer-and-split on the message the backend already builds, which makes it the reachable middle ground between what Moatless does now (whole messages, which is why `FEATURES.assistantStreaming` is off) and real streaming. The client half merged inert and is ready: the fade-in is gated on `data-streaming` and the smooth end-scroll on `isWorking`. Reproduce the split boundary faithfully — a naive split on `\n\n` breaks fenced blocks and tables. **`21d53ca2e` — browse ignored files and load folders on demand.** `packages/contracts/src/project.ts`, `apps/server/src/workspace/WorkspaceEntries.ts`. This one is a served method that now under-serves: `projects.listEntries` gained an optional `directoryPath` (present → one directory's immediate children including gitignored ones; omitted → the old recursive indexed listing), an `ignored` flag on `ProjectEntry`, and a `directory_list_failed` literal. Nothing breaks — `useDirectoryEntries` filters by parent path, so a full listing still yields the right rows — but it degrades quietly: every folder expansion re-reads the entire workspace index, the new four-way request pool has nothing to pool, and gitignored files stay invisible. Small, well-scoped backend change with a clear payoff on large repos. **`3b75e607e` — the handshake now fails on an environment-id mismatch.** The server half is upstream's own dev-auth feature, which the fork does not adopt. The client half applies regardless: after `initialSync`, `packages/client-runtime/src/rpc/session.ts` compares `config.environment.environmentId` against the id the connection was registered under and fails with a `ConnectionBlockedError` instead of proceeding. **This is a live risk, not a missing feature** — it passes every check in this repository and would fail on first contact with a deployment. Moatless must echo, in its `subscribeServerConfig` descriptor, exactly the `environmentId` the client registered. **`dd6ba84dc` — fall back when a new worktree is unavailable.** Preflights whether the project cwd is a git repository and whether the base ref names a real commit, degrading to the project checkout rather than failing thread creation. Largely moot today (`FEATURES.worktreeSelection` is off, Moatless never sets `worktreePath`); recorded because the shape is the one to copy if worktrees ever arrive. **`3138f5716` — task lifecycle for monitors and background shells.** Grok-adapter-specific and not reusable, but the contract-level behaviour is: the client already knows how to render a task's lifecycle, so a Moatless task that spawns a background shell or watcher lights that surface up for free by emitting the same events. **`9bf349cf6` — preserve internal agent errors.** A `(?!\[internal\])` so an over-broad transport-error pattern stops erasing the agent's own diagnostics. Cursor-specific; the failure mode generalizes anywhere Moatless normalizes sandbox or agent errors into transport-versus-agent buckets. ## Verification `tripwires`, `resolution-check`, `unsupported-methods`, `fmt:check`, `lint` and `typecheck` all pass. Tests pass per package: web `419`, mobile `319`, relay `30`. - `resolution-check` — 15 fork-delta paths still differ from upstream, 18 carry upstream's change, 17 `theirs-verbatim` paths byte-identical. - `tripwires` — Clerk/T3 Connect 4 files, device pairing 90 files, T3 session bootstrap 8 matches, exactly the 5 known deletions, 3 active workflows all allowed. - `unsupported-methods` — **ADD 0, DROP 0**, so `packages/contracts/src/rpc.ts` needed no edit. 151 methods, 93 declaring `UnsupportedMethodError`, 67 backend-dispatched. - Sweep — 8 owned-concern hits, all false positives: auth and pairing strings inside upstream's own `apps/server` auth work, which was not taken. - No route file was added, deleted or renamed upstream, so `regen-route-tree.mjs` correctly skipped. Three caveats, none of them a merge regression: 1. **`@t3tools/desktop` fails on a missing `libsecret-1`.** `scripts/browser-secret-native.test.mjs` shells out to `pkg-config --cflags --libs libsecret-1`, which is not installed in this sandbox. The file is untouched by this merge (`git log HEAD^1..HEAD -- <path>` is empty). Already recorded in gaps.md as _The desktop suite needs libsecret_. 2. **`duplicate-adds.mjs` reports 2 false positives.** `auth.ts` → `if (` is a line written by the `decide` resolution: it appears once in the merge and in neither parent, so it cannot be a kept-twice duplicate. `authBootstrap.test.ts` → `expect(testApi.calls.browserSession).toEqual([]);` traces to two genuinely distinct tests, one from each parent. The script's own tell is that a real duplicate breaks lint, typecheck and test at once; all three are green. 3. `@t3tools/mobile` failed once under the parallel run and passed alone — CPU contention. ## Fork documents - `docs/fork/inventory.json` — four new `pathPolicy` rows for files resolved this merge that had no policy: `settings-surface-gates`, `settings-search-filters`, `sidebar-fork-chrome`, `root-route`. `inventory-check.mjs` passes. - `docs/fork/gaps.md` — extended the pull-requests entry with the routing pair, and added four entries: the `listEntries` degradation, the streaming gate and the paragraph-split behaviour behind it, the environment-id handshake guard, and three more server fixes under _Runtime fixes upstream made to its own server_. - `docs/fork/upstream-merge-log.md` — dated entry. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- Moatless task: https://moatless.soaplabstest.com/tasks/13aa8647-9eb6-4052-8edf-e417893d7abf

Summary
timelineHeight.tsand reuse it fromChatViewResizeObserverTesting
apps/web/src/components/timelineHeight.test.tsbun lintbun typecheckNote
Medium Risk
Changes
ChatViewvirtualization sizing logic and introduces width-driven remeasurement, which can affect scrolling behavior and performance across viewports. Adds new Playwright/MSW browser-test infrastructure and CI steps that may introduce flakiness if timings or browser deps differ in CI.Overview
Chat timeline virtualization now estimates row heights using measured width. The inline
estimateTimelineMessageHeightlogic is extracted totimelineHeight.ts, updated to account for wrapping based ontimelineWidthPx(and attachment row sizing), and wired intoMessagesTimelinevia aResizeObserver+rowVirtualizer.measure()on width changes.Adds automated coverage and CI for real browser layout parity. Introduces unit tests for
estimateTimelineMessageHeight, plus a Vitest+Playwright browser test (ChatView.browser.tsx) that boots the full app with MSW-mocked WS/HTTP and asserts measured DOM heights stay within tolerance across multiple viewport sizes and with attachments; CI now installs/caches Playwright Chromium and runsapps/webtest:browser. Also factors router creation intogetRouter, adds MSW worker config/asset, and updates.gitignorefor Playwright artifacts/screenshots.Written by Cursor Bugbot for commit 93412ac. This will update automatically on new commits. Configure here.
Note
Make
apps/web/src/components/ChatView.MessagesTimelineheight estimation width-aware usingtimelineHeight.estimateTimelineMessageHeightand add Playwright-driven browser tests to validate attachment and text wrapping behaviorIntroduce a width-aware
timelineHeight.estimateTimelineMessageHeightand wire it intoChatView.MessagesTimeline; add browser tests with Playwright/Vitest and MSW; update CI to install/cache Playwright and run browser tests; refactor router setup into a factory.📍Where to Start
Start with the estimator in timelineHeight.ts, then review its integration in ChatView.tsx and validations in ChatView.browser.tsx.
Macroscope summarized 93412ac.