Noticed during the live sweep: every screenshot of a long conversation showed its oldest messages after a turn, with the new reply out of view below.
Measured in the live panel: after sending and after the reply, the thread scroller's scrollTop stayed at 0. scrollIntoView({ behavior: 'smooth', block: 'end' }) on the bottom marker never moves this nested scroller in Chrome, while the instant form scrolls to the end (4593/4593). The effect was firing; the smooth scroll just never happened.
One-line fix: instant scrollIntoView({ block: 'end' }), with the why next to it.
Noticed during the live sweep: every screenshot of a long conversation showed its oldest messages after a turn, with the new reply out of view below.
Measured in the live panel: after sending and after the reply, the thread scroller's `scrollTop` stayed at 0. `scrollIntoView({ behavior: 'smooth', block: 'end' })` on the `bottom` marker never moves this nested scroller in Chrome, while the instant form scrolls to the end (4593/4593). The effect was firing; the smooth scroll just never happened.
One-line fix: instant `scrollIntoView({ block: 'end' })`, with the why next to it.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
scrollIntoView({ behavior: 'smooth' }) on the thread's nested scroller never
moved it in Chrome — measured live: scrollTop stayed 0 after sending and
after the reply, while the instant form scrolled to the end. A long
conversation therefore showed its oldest messages after every turn, with
the new reply out of view below. Instant it is.
Co-Authored-By: Claude Fable 5 <[email protected]>
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 07:06:04Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
Verdict: Minor issues · 1 finding (0 with multi-model agreement)
1 single-model finding (lower confidence)
Finding
Where
Model
Lens
🟠
Instant scrollIntoView fires on every streaming step, snapping user view
web/src/editor/AssistantTab.tsx:47
kimi-k2.6:cloud
error-handling
Per-model detailclaude-code/opus (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
The change is a one-line frontend scroll-behavior fix: dropping behavior: 'smooth' from bottom.current?.scrollIntoView(...) in web/src/editor/AssistantTab.tsx:48. I read the surrounding component to confirm the diff matches the checked-out code and to check for any security-relevant surface.
Through the 🔒 Security lens there is nothing to flag:
No authentication/authorization logic is touched — this is a DOM scroll call on a ref.
No untrusted/tainted input flows into the change. scrollIntoView takes a static options object; no user- or server-controlled data reaches it.
No injection surface (no HTML/SQL/command/path/template construction). Message rendering goes through the pre-existing MarkdownMessage/MarkdownBoundary path, which is unchanged by this PR.
No secrets, deserialization, SSRF, or default-configuration changes.
The security posture is identical before and after the diff. Clean under this lens.
🎯 Correctness — No material issues found
VERDICT: No material issues found
The change is a single-line fix that drops behavior: 'smooth' from a scrollIntoView call, leaving { block: 'end' }.
Through the Correctness lens, I verified:
The scroll target is correct.bottom (the <div ref={bottom} /> at AssistantTab.tsx:148) is the final child inside the overflow-y-auto thread container (:98), so aligning its bottom edge with block: 'end' scrolls the nested scroller to the very end — exactly the intended "show the newest reply" behavior.
block: 'end' is the right keyword. Plain scrollIntoView() defaults to block: 'start'; the explicit 'end' is retained, so scroll-to-bottom semantics are unchanged. Only the behavior was removed, which defaults to 'auto' (instant) — matching the description's empirical finding that the instant form actually moves this nested scroller in Chrome while smooth did not.
No side effects in the change. The effect's dependency array [history.data, pending] and the ref are untouched, so firing timing is identical; the fix strictly swaps a no-op smooth scroll for a working instant one.
No stale sibling. Grepped the whole web/src tree — this is the only scrollIntoView/smooth-scroll site, so nothing else needed a parallel update.
The change is well within its stated scope and introduces no logic bug.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
This is a one-line change in my lens (readability/maintainability), and it's clean:
web/src/editor/AssistantTab.tsx:48 — the only code change drops behavior: 'smooth' from the scrollIntoView call. Verified by reading the file: bottom is a useRef<HTMLDivElement> (line 38) used only here, the effect deps [history.data, pending] are unchanged, and the call remains a plain, idiomatic one-liner. No dead code, no duplication, no naming or structure problems introduced.
The accompanying comment (lines 43–46) matches this codebase's strongly-established "explain the why next to it" convention (CLAUDE.md is emphatic about this), cites the concrete symptom and date, and is accurate to the new behavior. It's on the verbose side but well within house style.
One thing I checked and am deliberately not flagging as a defect: the pre-existing "Deliberately NOT aborted on unmount" comment (lines 40–42) sits directly above this scroll useEffect even though it describes the abort-controller lifecycle, not the scroll. Stacking the new scroll rationale under it makes two unrelated comment blocks share one useEffect header, which reads slightly ambiguously. But that adjacency is pre-existing (the abort comment is unchanged context in the diff), not introduced here, and it's cosmetic — not worth churn on this PR.
Nothing material in the cleanliness/maintainability lens.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed web/src/editor/AssistantTab.tsx:47-49 through the performance lens.
The change only drops behavior: 'smooth' from a single scrollIntoView call in a useEffect. The effect's dependency array ([history.data, pending]) is unchanged, so it fires no more often than before. If anything, the instant scroll is cheaper than an animated smooth scroll — no per-frame animation work. Verified by reading the full component region: bottom is a plain ref (line 38), the effect runs one guarded scrollIntoView (line 48), and there's no loop, allocation, query, or unbounded growth introduced.
Nothing in the performance lane is materially wrong here.
🧯 Error handling & edge cases — No material issues found
The change is a single-line behavior tweak: dropping behavior: 'smooth' from the scrollIntoView call. I've read the surrounding code to check the unhappy paths.
VERDICT: No material issues found
Through the error-handling & edge-cases lens, this diff is clean:
Nil ref is already guarded.bottom.current?.scrollIntoView(...) (AssistantTab.tsx:48) uses optional chaining, so an unmounted/not-yet-attached ref is a no-op — the removal of behavior: 'smooth' doesn't change that safety. On first render before the div mounts, bottom.current is null and the call is skipped.
No error path added or removed. The change touches only the scroll option object; it neither swallows an error, changes cleanup, nor introduces a code path that can throw. scrollIntoView with { block: 'end' } is well-defined and doesn't throw on empty/short threads (nothing to scroll → no-op).
Effect deps unchanged ([history.data, pending]), so the firing edge cases (empty history, pending null, first paint) behave exactly as before — only the scroll animation mode differs.
Nothing in the error-handling / edge-case domain is affected by removing the smooth-scroll behavior.
claude-code/sonnet (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The changed line removes behavior: 'smooth' from a scrollIntoView call on a hardcoded empty <div ref={bottom} />. No untrusted data touches this code path.
Verified by reading AssistantTab.tsx:
bottom ref attaches to an empty sentinel <div> (line 148) — no user content flows into it.
history.data that triggers the effect is Zod-validated on ingest; it does not reach this DOM call at all.
MarkdownMessage.tsx disables rehype-raw and blocks <img> tags, so no XSS or SSRF via rendered assistant output.
Nothing in this diff touches authentication, authorization, input handling, serialization, or any security boundary.
🎯 Correctness — No material issues found
No material issues found.
Verification performed:
Read web/src/editor/AssistantTab.tsx in full: the change at line 47–49 matches the diff exactly — scrollIntoView({ block: 'end' }) on the bottom sentinel div.
Searched the entire web/ tree for any scroll-behavior CSS rule. None exists — no scroll-behavior: smooth in index.css, index.html, or anywhere else.
Confirmed the scrollable container (overflow-y-auto div at line 98) carries no scroll-behavior class or inline style.
Analysis:
Omitting behavior from scrollIntoView options defaults to 'auto', which delegates to the CSS scroll-behavior computed value. Since no element in the scroll ancestor chain sets scroll-behavior: smooth, 'auto' resolves to instant scrolling — exactly the intended outcome. The dep array [history.data, pending] is correct: it fires on send (pending becomes non-null) and on reply arrival (history.data updates), covering both trigger points. The optional-chain on bottom.current?.scrollIntoView is safe when the ref is unattached.
The one robustness nit — using behavior: 'instant' instead of relying on the CSS default — is not a correctness defect under the current codebase, merely a defensive style preference. Flagging it would be outside the correctness-bug lane.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The diff is a one-word behavioural fix (behavior: 'smooth' removed) plus a comment explaining the workaround. From a cleanliness standpoint:
The comment is well-placed — browser-specific workarounds with no obvious reason invite reversion, and the explanation ("found live, 2026-08-23, smooth never moved the nested scroller") is exactly the kind of non-obvious "why" that belongs in code.
The comment is appended to a pre-existing block that also covers the abort behaviour; both topics share the same useEffect neighbourhood and the mixing is not introduced by this PR.
No dead code, duplication, or naming issues.
⚡ Performance — No material issues found
No material issues found
The diff is a one-liner: dropping behavior: 'smooth' from a scrollIntoView call. Through the performance lens there is nothing to flag:
Instant scroll is lighter than smooth. Smooth scroll drives a multi-frame compositor animation; instant scroll is a single synchronous layout pass. The change removes overhead, not adds it.
Effect triggers on pending updates during streaming. Each incremental step fires the effect and calls scrollIntoView. That was true before and remains true after; the diff doesn't change the dependency array or introduce any new work in the hot path. Verified in file — the effect at line 47–49 is unchanged except for the removed option.
No allocations, no queries, no loops touched. The rest of the file is unmodified.
🧯 Error handling & edge cases — No material issues found
No material issues found.
The only error-handling surface in this diff is the ?. optional chain on bottom.current, which was already present before the change and correctly guards against the ref being null on initial render or after unmount. The removed behavior: 'smooth' key has no effect on error semantics — scrollIntoView with or without it is a void, synchronous DOM call that cannot throw under normal conditions.
Nothing new is introduced on the unhappy paths: no new promise, no new async op, no resource to clean up, no boundary condition changed.
glm-5.2:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The change swaps behavior: 'smooth' for the default instant scrollIntoView({ block: 'end' }) on a locally-held bottom ref (web/src/editor/AssistantTab.tsx:48). This is purely a scroll-behavior tweak inside the component — no input handling, network, auth, deserialization, or untrusted-data path is touched. The ref points at an empty <div ref={bottom} /> (line 148) rendered within the component's own thread container; nothing user-controlled flows into it. Nothing in the security lens applies.
🎯 Correctness — No material issues found
No material issues found
The change removes behavior: 'smooth' from a scrollIntoView call, leaving the effect's dependency array ([history.data, pending]) and firing semantics untouched. I verified the surrounding code at web/src/editor/AssistantTab.tsx:38-49: bottom is a useRef<HTMLDivElement> rendered as the scroll marker, and the effect's trigger conditions are unchanged by this diff. Through the correctness lens there is no logic, formula, or threshold change to scrutinize — it's a browser-scroll-behavior swap with no impact on when or whether the scroll attempt happens. Nothing material to flag.
🧹 Code cleanliness & maintainability — No material issues found
The change is minimal and clean through my lens. The comment block is verbose but explains a non-obvious behavioral quirk, which aids maintainability. No structural issues.
Verdict: No material issues found
The one-line behavioral change (scrollIntoView({ block: 'end' }) without behavior: 'smooth') is clear and consistent with the surrounding pattern. The added comment, while long, documents a real Chromium quirk that a future reader would otherwise be tempted to "fix" back to smooth scrolling — a maintainability win, not noise. No duplicated/dead code introduced; the effect dependency array and ref usage are unchanged. Verified by reading AssistantTab.tsx:30-49.
⚡ Performance — No material issues found
VERDICT: No material issues found
The change removes behavior: 'smooth' from a single scrollIntoView call inside a useEffect. This is a behavior/correctness fix, not a performance path. Verified at web/src/editor/AssistantTab.tsx:47-49 — the effect fires on history.data / pending changes (one scroll per turn/step update), and the instant form is if anything cheaper than the smooth form it replaces. No allocations, loops, or queries are touched.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
The change is a one-liner swapping behavior: 'smooth' for the instant scrollIntoView({ block: 'end' }). Through the error-handling/edge-case lens, the unhappy paths are already covered: bottom.current?. null-guards the ref (verified at AssistantTab.tsx:48), and scrollIntoView returns no error to swallow. The effect's dependency array [history.data, pending] (line 49) fires on both the persisted-turn update and the pending clear, so the scroll runs after the new reply is committed to the DOM — no race with empty content. The loading (history.isPending) and empty (messages.length === 0) states are harmless: the ref still exists and scrolling an empty thread to its bottom is a no-op. No deferred cleanup is needed because the effect performs an instantaneous, idempotent DOM read+scroll with no resources to release. I confirmed the surrounding code and ref lifecycle by reading the full component (AssistantTab.tsx:1-200).
This change replaces a smooth scrollIntoView with an instant one in the assistant chat thread. Through a security lens, there is nothing to report:
The bottom ref targets a hardcoded, empty <div> owned by the component itself (<div ref={bottom} /> at line 148). It is not user-controlled and cannot be hijacked to scroll to attacker-supplied content.
The scrollIntoView call is inside a useEffect gated on history.data and pending — both are local/React-Query-managed state, not raw DOM manipulation of untrusted nodes.
The change removes behavior: 'smooth'; this is purely a UX workaround for a Chrome nested-scroller bug. It does not alter the scroll target, introduce new attack surface, or affect how messages are rendered.
Existing security posture of message rendering (React escaping for user messages, ReactMarkdown without rehype-raw, disallowedElements: ['img'], and noopener noreferrer on links) is unchanged by this diff.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 3: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
Clean one-line fix with a precise, dated inline comment explaining the Chrome quirk. No readability or maintainability concerns.
⚡ Performance — No material issues found
Verdict: No material issues found
This change is a small performance improvement, not a regression. Removing behavior: 'smooth' eliminates expensive, overlapping smooth-scroll animations that were firing on every history.data/pending change. From the Performance lens, there is nothing to flag.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
Findings:
web/src/editor/AssistantTab.tsx:47 — The useEffect depends on pending, which is recreated on every streaming step (setPending((p) => (p ? { ...p, steps: [...p.steps, step] } : p)) in send() line 65). With the previous behavior: 'smooth', this was benign because smooth scrolling didn't move the nested scroller. Switching to instant scrollIntoView means the effect now forcibly snaps the view to the bottom on every single step update during streaming. In a long conversation, a user who scrolls up to read earlier messages will be yanked back down on each step, making it impossible to reference prior context while the assistant is working. This is an unhandled edge case introduced by the change.
Suggested fix: Only scroll when pending transitions from null to non-null (user sent a message) or when history.data length increases, not on intermediate step mutations. For example, track the previous pending?.message in a ref and skip the scroll if only pending.steps changed, or split the concern into two effects with tighter dependencies.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 4 models
**Verdict: Minor issues** · 1 finding (0 with multi-model agreement)
<details><summary>1 single-model finding (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Instant scrollIntoView fires on every streaming step, snapping user view | `web/src/editor/AssistantTab.tsx:47` | kimi-k2.6:cloud | error-handling |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**VERDICT: No material issues found**
The change is a one-line frontend scroll-behavior fix: dropping `behavior: 'smooth'` from `bottom.current?.scrollIntoView(...)` in `web/src/editor/AssistantTab.tsx:48`. I read the surrounding component to confirm the diff matches the checked-out code and to check for any security-relevant surface.
Through the 🔒 Security lens there is nothing to flag:
- No authentication/authorization logic is touched — this is a DOM scroll call on a ref.
- No untrusted/tainted input flows into the change. `scrollIntoView` takes a static options object; no user- or server-controlled data reaches it.
- No injection surface (no HTML/SQL/command/path/template construction). Message rendering goes through the pre-existing `MarkdownMessage`/`MarkdownBoundary` path, which is unchanged by this PR.
- No secrets, deserialization, SSRF, or default-configuration changes.
The security posture is identical before and after the diff. Clean under this lens.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
## VERDICT: No material issues found
The change is a single-line fix that drops `behavior: 'smooth'` from a `scrollIntoView` call, leaving `{ block: 'end' }`.
Through the Correctness lens, I verified:
- **The scroll target is correct.** `bottom` (the `<div ref={bottom} />` at `AssistantTab.tsx:148`) is the final child inside the `overflow-y-auto` thread container (`:98`), so aligning its bottom edge with `block: 'end'` scrolls the nested scroller to the very end — exactly the intended "show the newest reply" behavior.
- **`block: 'end'` is the right keyword.** Plain `scrollIntoView()` defaults to `block: 'start'`; the explicit `'end'` is retained, so scroll-to-bottom semantics are unchanged. Only the `behavior` was removed, which defaults to `'auto'` (instant) — matching the description's empirical finding that the instant form actually moves this nested scroller in Chrome while smooth did not.
- **No side effects in the change.** The effect's dependency array `[history.data, pending]` and the ref are untouched, so firing timing is identical; the fix strictly swaps a no-op smooth scroll for a working instant one.
- **No stale sibling.** Grepped the whole `web/src` tree — this is the only `scrollIntoView`/smooth-scroll site, so nothing else needed a parallel update.
The change is well within its stated scope and introduces no logic bug.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
VERDICT: No material issues found
This is a one-line change in my lens (readability/maintainability), and it's clean:
- `web/src/editor/AssistantTab.tsx:48` — the only code change drops `behavior: 'smooth'` from the `scrollIntoView` call. Verified by reading the file: `bottom` is a `useRef<HTMLDivElement>` (line 38) used only here, the effect deps `[history.data, pending]` are unchanged, and the call remains a plain, idiomatic one-liner. No dead code, no duplication, no naming or structure problems introduced.
- The accompanying comment (lines 43–46) matches this codebase's strongly-established "explain the why next to it" convention (CLAUDE.md is emphatic about this), cites the concrete symptom and date, and is accurate to the new behavior. It's on the verbose side but well within house style.
One thing I checked and am deliberately *not* flagging as a defect: the pre-existing "Deliberately NOT aborted on unmount" comment (lines 40–42) sits directly above this scroll `useEffect` even though it describes the abort-controller lifecycle, not the scroll. Stacking the new scroll rationale under it makes two unrelated comment blocks share one `useEffect` header, which reads slightly ambiguously. But that adjacency is pre-existing (the abort comment is unchanged context in the diff), not introduced here, and it's cosmetic — not worth churn on this PR.
Nothing material in the cleanliness/maintainability lens.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed `web/src/editor/AssistantTab.tsx:47-49` through the performance lens.
- The change only drops `behavior: 'smooth'` from a single `scrollIntoView` call in a `useEffect`. The effect's dependency array (`[history.data, pending]`) is unchanged, so it fires no more often than before. If anything, the instant scroll is *cheaper* than an animated smooth scroll — no per-frame animation work. Verified by reading the full component region: `bottom` is a plain ref (line 38), the effect runs one guarded `scrollIntoView` (line 48), and there's no loop, allocation, query, or unbounded growth introduced.
Nothing in the performance lane is materially wrong here.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
The change is a single-line behavior tweak: dropping `behavior: 'smooth'` from the `scrollIntoView` call. I've read the surrounding code to check the unhappy paths.
## VERDICT: No material issues found
Through the error-handling & edge-cases lens, this diff is clean:
- **Nil ref is already guarded.** `bottom.current?.scrollIntoView(...)` (`AssistantTab.tsx:48`) uses optional chaining, so an unmounted/not-yet-attached ref is a no-op — the removal of `behavior: 'smooth'` doesn't change that safety. On first render before the div mounts, `bottom.current` is null and the call is skipped.
- **No error path added or removed.** The change touches only the scroll option object; it neither swallows an error, changes cleanup, nor introduces a code path that can throw. `scrollIntoView` with `{ block: 'end' }` is well-defined and doesn't throw on empty/short threads (nothing to scroll → no-op).
- **Effect deps unchanged** (`[history.data, pending]`), so the firing edge cases (empty history, pending null, first paint) behave exactly as before — only the scroll animation mode differs.
Nothing in the error-handling / edge-case domain is affected by removing the smooth-scroll behavior.
</details>
</details>
<details><summary><b>claude-code/sonnet</b> (claude-code) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
The changed line removes `behavior: 'smooth'` from a `scrollIntoView` call on a hardcoded empty `<div ref={bottom} />`. No untrusted data touches this code path.
Verified by reading `AssistantTab.tsx`:
- `bottom` ref attaches to an empty sentinel `<div>` (line 148) — no user content flows into it.
- `history.data` that triggers the effect is Zod-validated on ingest; it does not reach this DOM call at all.
- `MarkdownMessage.tsx` disables `rehype-raw` and blocks `<img>` tags, so no XSS or SSRF via rendered assistant output.
Nothing in this diff touches authentication, authorization, input handling, serialization, or any security boundary.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found.
**Verification performed:**
- Read `web/src/editor/AssistantTab.tsx` in full: the change at line 47–49 matches the diff exactly — `scrollIntoView({ block: 'end' })` on the `bottom` sentinel div.
- Searched the entire `web/` tree for any `scroll-behavior` CSS rule. None exists — no `scroll-behavior: smooth` in `index.css`, `index.html`, or anywhere else.
- Confirmed the scrollable container (`overflow-y-auto` div at line 98) carries no `scroll-behavior` class or inline style.
**Analysis:**
Omitting `behavior` from `scrollIntoView` options defaults to `'auto'`, which delegates to the CSS `scroll-behavior` computed value. Since no element in the scroll ancestor chain sets `scroll-behavior: smooth`, `'auto'` resolves to instant scrolling — exactly the intended outcome. The dep array `[history.data, pending]` is correct: it fires on send (pending becomes non-null) and on reply arrival (history.data updates), covering both trigger points. The optional-chain on `bottom.current?.scrollIntoView` is safe when the ref is unattached.
The one robustness nit — using `behavior: 'instant'` instead of relying on the CSS default — is not a correctness defect under the current codebase, merely a defensive style preference. Flagging it would be outside the correctness-bug lane.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found**
The diff is a one-word behavioural fix (`behavior: 'smooth'` removed) plus a comment explaining the workaround. From a cleanliness standpoint:
- The comment is well-placed — browser-specific workarounds with no obvious reason invite reversion, and the explanation ("found live, 2026-08-23, smooth never moved the nested scroller") is exactly the kind of non-obvious "why" that belongs in code.
- The comment is appended to a pre-existing block that also covers the abort behaviour; both topics share the same `useEffect` neighbourhood and the mixing is not introduced by this PR.
- No dead code, duplication, or naming issues.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
The diff is a one-liner: dropping `behavior: 'smooth'` from a `scrollIntoView` call. Through the performance lens there is nothing to flag:
- **Instant scroll is lighter than smooth.** Smooth scroll drives a multi-frame compositor animation; instant scroll is a single synchronous layout pass. The change removes overhead, not adds it.
- **Effect triggers on `pending` updates during streaming.** Each incremental step fires the effect and calls `scrollIntoView`. That was true before and remains true after; the diff doesn't change the dependency array or introduce any new work in the hot path. Verified in file — the effect at line 47–49 is unchanged except for the removed option.
- **No allocations, no queries, no loops touched.** The rest of the file is unmodified.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found.
The only error-handling surface in this diff is the `?.` optional chain on `bottom.current`, which was already present before the change and correctly guards against the ref being null on initial render or after unmount. The removed `behavior: 'smooth'` key has no effect on error semantics — `scrollIntoView` with or without it is a void, synchronous DOM call that cannot throw under normal conditions.
Nothing new is introduced on the unhappy paths: no new promise, no new async op, no resource to clean up, no boundary condition changed.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
The change swaps `behavior: 'smooth'` for the default instant `scrollIntoView({ block: 'end' })` on a locally-held `bottom` ref (`web/src/editor/AssistantTab.tsx:48`). This is purely a scroll-behavior tweak inside the component — no input handling, network, auth, deserialization, or untrusted-data path is touched. The ref points at an empty `<div ref={bottom} />` (line 148) rendered within the component's own thread container; nothing user-controlled flows into it. Nothing in the security lens applies.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
The change removes `behavior: 'smooth'` from a `scrollIntoView` call, leaving the effect's dependency array (`[history.data, pending]`) and firing semantics untouched. I verified the surrounding code at `web/src/editor/AssistantTab.tsx:38-49`: `bottom` is a `useRef<HTMLDivElement>` rendered as the scroll marker, and the effect's trigger conditions are unchanged by this diff. Through the correctness lens there is no logic, formula, or threshold change to scrutinize — it's a browser-scroll-behavior swap with no impact on when or whether the scroll attempt happens. Nothing material to flag.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
The change is minimal and clean through my lens. The comment block is verbose but explains a non-obvious behavioral quirk, which aids maintainability. No structural issues.
**Verdict: No material issues found**
- The one-line behavioral change (`scrollIntoView({ block: 'end' })` without `behavior: 'smooth'`) is clear and consistent with the surrounding pattern. The added comment, while long, documents a real Chromium quirk that a future reader would otherwise be tempted to "fix" back to smooth scrolling — a maintainability win, not noise. No duplicated/dead code introduced; the effect dependency array and ref usage are unchanged. Verified by reading `AssistantTab.tsx:30-49`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
- The change removes `behavior: 'smooth'` from a single `scrollIntoView` call inside a `useEffect`. This is a behavior/correctness fix, not a performance path. Verified at `web/src/editor/AssistantTab.tsx:47-49` — the effect fires on `history.data` / `pending` changes (one scroll per turn/step update), and the instant form is if anything cheaper than the smooth form it replaces. No allocations, loops, or queries are touched.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
VERDICT: No material issues found
- The change is a one-liner swapping `behavior: 'smooth'` for the instant `scrollIntoView({ block: 'end' })`. Through the error-handling/edge-case lens, the unhappy paths are already covered: `bottom.current?.` null-guards the ref (verified at `AssistantTab.tsx:48`), and `scrollIntoView` returns no error to swallow. The effect's dependency array `[history.data, pending]` (line 49) fires on both the persisted-turn update and the pending clear, so the scroll runs after the new reply is committed to the DOM — no race with empty content. The loading (`history.isPending`) and empty (`messages.length === 0`) states are harmless: the ref still exists and scrolling an empty thread to its bottom is a no-op. No deferred cleanup is needed because the effect performs an instantaneous, idempotent DOM read+scroll with no resources to release. I confirmed the surrounding code and ref lifecycle by reading the full component (`AssistantTab.tsx:1-200`).
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
This change replaces a smooth `scrollIntoView` with an instant one in the assistant chat thread. Through a security lens, there is nothing to report:
- The `bottom` ref targets a hardcoded, empty `<div>` owned by the component itself (`<div ref={bottom} />` at line 148). It is not user-controlled and cannot be hijacked to scroll to attacker-supplied content.
- The `scrollIntoView` call is inside a `useEffect` gated on `history.data` and `pending` — both are local/React-Query-managed state, not raw DOM manipulation of untrusted nodes.
- The change removes `behavior: 'smooth'`; this is purely a UX workaround for a Chrome nested-scroller bug. It does not alter the scroll target, introduce new attack surface, or affect how messages are rendered.
- Existing security posture of message rendering (React escaping for user messages, `ReactMarkdown` without `rehype-raw`, `disallowedElements: ['img']`, and `noopener noreferrer` on links) is unchanged by this diff.
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 3: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: model returned an empty response
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found.**
Clean one-line fix with a precise, dated inline comment explaining the Chrome quirk. No readability or maintainability concerns.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict:** No material issues found
This change is a small performance improvement, not a regression. Removing `behavior: 'smooth'` eliminates expensive, overlapping smooth-scroll animations that were firing on every `history.data`/`pending` change. From the Performance lens, there is nothing to flag.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict:** Minor issues
**Findings:**
- **`web/src/editor/AssistantTab.tsx:47`** — The `useEffect` depends on `pending`, which is recreated on every streaming step (`setPending((p) => (p ? { ...p, steps: [...p.steps, step] } : p))` in `send()` line 65). With the previous `behavior: 'smooth'`, this was benign because smooth scrolling didn't move the nested scroller. Switching to instant `scrollIntoView` means the effect now forcibly snaps the view to the bottom on **every single step update** during streaming. In a long conversation, a user who scrolls up to read earlier messages will be yanked back down on each step, making it impossible to reference prior context while the assistant is working. This is an unhandled edge case introduced by the change.
**Suggested fix:** Only scroll when `pending` transitions from `null` to non-null (user sent a message) or when `history.data` length increases, not on intermediate step mutations. For example, track the previous `pending?.message` in a ref and skip the scroll if only `pending.steps` changed, or split the concern into two effects with tighter dependencies.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
The instant scroll runs on every step of a turn, so it now follows new
content only while the view is at the end of the thread (within 80px).
Scrolling up to read something stays put until the person comes back
down or sends the next message, which returns them to the end.
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
merged commit a37122fc77 into main2026-08-23 07:10:27 +00:00
steve
deleted branch fix/assistant-thread-scroll2026-08-23 07:10:27 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Noticed during the live sweep: every screenshot of a long conversation showed its oldest messages after a turn, with the new reply out of view below.
Measured in the live panel: after sending and after the reply, the thread scroller's
scrollTopstayed at 0.scrollIntoView({ behavior: 'smooth', block: 'end' })on thebottommarker never moves this nested scroller in Chrome, while the instant form scrolls to the end (4593/4593). The effect was firing; the smooth scroll just never happened.One-line fix: instant
scrollIntoView({ block: 'end' }), with the why next to it.🤖 Generated with Claude Code
scrollIntoView({ behavior: 'smooth' }) on the thread's nested scroller never moved it in Chrome — measured live: scrollTop stayed 0 after sending and after the reply, while the instant form scrolled to the end. A long conversation therefore showed its oldest messages after every turn, with the new reply out of view below. Instant it is. Co-Authored-By: Claude Fable 5 <[email protected]>🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 07:06:04Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly review — consensus across 4 models
Verdict: Minor issues · 1 finding (0 with multi-model agreement)
1 single-model finding (lower confidence)
web/src/editor/AssistantTab.tsx:47Per-model detail
claude-code/opus (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
The change is a one-line frontend scroll-behavior fix: dropping
behavior: 'smooth'frombottom.current?.scrollIntoView(...)inweb/src/editor/AssistantTab.tsx:48. I read the surrounding component to confirm the diff matches the checked-out code and to check for any security-relevant surface.Through the 🔒 Security lens there is nothing to flag:
scrollIntoViewtakes a static options object; no user- or server-controlled data reaches it.MarkdownMessage/MarkdownBoundarypath, which is unchanged by this PR.The security posture is identical before and after the diff. Clean under this lens.
🎯 Correctness — No material issues found
VERDICT: No material issues found
The change is a single-line fix that drops
behavior: 'smooth'from ascrollIntoViewcall, leaving{ block: 'end' }.Through the Correctness lens, I verified:
bottom(the<div ref={bottom} />atAssistantTab.tsx:148) is the final child inside theoverflow-y-autothread container (:98), so aligning its bottom edge withblock: 'end'scrolls the nested scroller to the very end — exactly the intended "show the newest reply" behavior.block: 'end'is the right keyword. PlainscrollIntoView()defaults toblock: 'start'; the explicit'end'is retained, so scroll-to-bottom semantics are unchanged. Only thebehaviorwas removed, which defaults to'auto'(instant) — matching the description's empirical finding that the instant form actually moves this nested scroller in Chrome while smooth did not.[history.data, pending]and the ref are untouched, so firing timing is identical; the fix strictly swaps a no-op smooth scroll for a working instant one.web/srctree — this is the onlyscrollIntoView/smooth-scroll site, so nothing else needed a parallel update.The change is well within its stated scope and introduces no logic bug.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
This is a one-line change in my lens (readability/maintainability), and it's clean:
web/src/editor/AssistantTab.tsx:48— the only code change dropsbehavior: 'smooth'from thescrollIntoViewcall. Verified by reading the file:bottomis auseRef<HTMLDivElement>(line 38) used only here, the effect deps[history.data, pending]are unchanged, and the call remains a plain, idiomatic one-liner. No dead code, no duplication, no naming or structure problems introduced.One thing I checked and am deliberately not flagging as a defect: the pre-existing "Deliberately NOT aborted on unmount" comment (lines 40–42) sits directly above this scroll
useEffecteven though it describes the abort-controller lifecycle, not the scroll. Stacking the new scroll rationale under it makes two unrelated comment blocks share oneuseEffectheader, which reads slightly ambiguously. But that adjacency is pre-existing (the abort comment is unchanged context in the diff), not introduced here, and it's cosmetic — not worth churn on this PR.Nothing material in the cleanliness/maintainability lens.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed
web/src/editor/AssistantTab.tsx:47-49through the performance lens.behavior: 'smooth'from a singlescrollIntoViewcall in auseEffect. The effect's dependency array ([history.data, pending]) is unchanged, so it fires no more often than before. If anything, the instant scroll is cheaper than an animated smooth scroll — no per-frame animation work. Verified by reading the full component region:bottomis a plain ref (line 38), the effect runs one guardedscrollIntoView(line 48), and there's no loop, allocation, query, or unbounded growth introduced.Nothing in the performance lane is materially wrong here.
🧯 Error handling & edge cases — No material issues found
The change is a single-line behavior tweak: dropping
behavior: 'smooth'from thescrollIntoViewcall. I've read the surrounding code to check the unhappy paths.VERDICT: No material issues found
Through the error-handling & edge-cases lens, this diff is clean:
bottom.current?.scrollIntoView(...)(AssistantTab.tsx:48) uses optional chaining, so an unmounted/not-yet-attached ref is a no-op — the removal ofbehavior: 'smooth'doesn't change that safety. On first render before the div mounts,bottom.currentis null and the call is skipped.scrollIntoViewwith{ block: 'end' }is well-defined and doesn't throw on empty/short threads (nothing to scroll → no-op).[history.data, pending]), so the firing edge cases (empty history, pending null, first paint) behave exactly as before — only the scroll animation mode differs.Nothing in the error-handling / edge-case domain is affected by removing the smooth-scroll behavior.
claude-code/sonnet (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The changed line removes
behavior: 'smooth'from ascrollIntoViewcall on a hardcoded empty<div ref={bottom} />. No untrusted data touches this code path.Verified by reading
AssistantTab.tsx:bottomref attaches to an empty sentinel<div>(line 148) — no user content flows into it.history.datathat triggers the effect is Zod-validated on ingest; it does not reach this DOM call at all.MarkdownMessage.tsxdisablesrehype-rawand blocks<img>tags, so no XSS or SSRF via rendered assistant output.Nothing in this diff touches authentication, authorization, input handling, serialization, or any security boundary.
🎯 Correctness — No material issues found
No material issues found.
Verification performed:
web/src/editor/AssistantTab.tsxin full: the change at line 47–49 matches the diff exactly —scrollIntoView({ block: 'end' })on thebottomsentinel div.web/tree for anyscroll-behaviorCSS rule. None exists — noscroll-behavior: smoothinindex.css,index.html, or anywhere else.overflow-y-autodiv at line 98) carries noscroll-behaviorclass or inline style.Analysis:
Omitting
behaviorfromscrollIntoViewoptions defaults to'auto', which delegates to the CSSscroll-behaviorcomputed value. Since no element in the scroll ancestor chain setsscroll-behavior: smooth,'auto'resolves to instant scrolling — exactly the intended outcome. The dep array[history.data, pending]is correct: it fires on send (pending becomes non-null) and on reply arrival (history.data updates), covering both trigger points. The optional-chain onbottom.current?.scrollIntoViewis safe when the ref is unattached.The one robustness nit — using
behavior: 'instant'instead of relying on the CSS default — is not a correctness defect under the current codebase, merely a defensive style preference. Flagging it would be outside the correctness-bug lane.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The diff is a one-word behavioural fix (
behavior: 'smooth'removed) plus a comment explaining the workaround. From a cleanliness standpoint:useEffectneighbourhood and the mixing is not introduced by this PR.⚡ Performance — No material issues found
No material issues found
The diff is a one-liner: dropping
behavior: 'smooth'from ascrollIntoViewcall. Through the performance lens there is nothing to flag:pendingupdates during streaming. Each incremental step fires the effect and callsscrollIntoView. That was true before and remains true after; the diff doesn't change the dependency array or introduce any new work in the hot path. Verified in file — the effect at line 47–49 is unchanged except for the removed option.🧯 Error handling & edge cases — No material issues found
No material issues found.
The only error-handling surface in this diff is the
?.optional chain onbottom.current, which was already present before the change and correctly guards against the ref being null on initial render or after unmount. The removedbehavior: 'smooth'key has no effect on error semantics —scrollIntoViewwith or without it is a void, synchronous DOM call that cannot throw under normal conditions.Nothing new is introduced on the unhappy paths: no new promise, no new async op, no resource to clean up, no boundary condition changed.
glm-5.2:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The change swaps
behavior: 'smooth'for the default instantscrollIntoView({ block: 'end' })on a locally-heldbottomref (web/src/editor/AssistantTab.tsx:48). This is purely a scroll-behavior tweak inside the component — no input handling, network, auth, deserialization, or untrusted-data path is touched. The ref points at an empty<div ref={bottom} />(line 148) rendered within the component's own thread container; nothing user-controlled flows into it. Nothing in the security lens applies.🎯 Correctness — No material issues found
No material issues found
The change removes
behavior: 'smooth'from ascrollIntoViewcall, leaving the effect's dependency array ([history.data, pending]) and firing semantics untouched. I verified the surrounding code atweb/src/editor/AssistantTab.tsx:38-49:bottomis auseRef<HTMLDivElement>rendered as the scroll marker, and the effect's trigger conditions are unchanged by this diff. Through the correctness lens there is no logic, formula, or threshold change to scrutinize — it's a browser-scroll-behavior swap with no impact on when or whether the scroll attempt happens. Nothing material to flag.🧹 Code cleanliness & maintainability — No material issues found
The change is minimal and clean through my lens. The comment block is verbose but explains a non-obvious behavioral quirk, which aids maintainability. No structural issues.
Verdict: No material issues found
scrollIntoView({ block: 'end' })withoutbehavior: 'smooth') is clear and consistent with the surrounding pattern. The added comment, while long, documents a real Chromium quirk that a future reader would otherwise be tempted to "fix" back to smooth scrolling — a maintainability win, not noise. No duplicated/dead code introduced; the effect dependency array and ref usage are unchanged. Verified by readingAssistantTab.tsx:30-49.⚡ Performance — No material issues found
VERDICT: No material issues found
behavior: 'smooth'from a singlescrollIntoViewcall inside auseEffect. This is a behavior/correctness fix, not a performance path. Verified atweb/src/editor/AssistantTab.tsx:47-49— the effect fires onhistory.data/pendingchanges (one scroll per turn/step update), and the instant form is if anything cheaper than the smooth form it replaces. No allocations, loops, or queries are touched.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
behavior: 'smooth'for the instantscrollIntoView({ block: 'end' }). Through the error-handling/edge-case lens, the unhappy paths are already covered:bottom.current?.null-guards the ref (verified atAssistantTab.tsx:48), andscrollIntoViewreturns no error to swallow. The effect's dependency array[history.data, pending](line 49) fires on both the persisted-turn update and the pending clear, so the scroll runs after the new reply is committed to the DOM — no race with empty content. The loading (history.isPending) and empty (messages.length === 0) states are harmless: the ref still exists and scrolling an empty thread to its bottom is a no-op. No deferred cleanup is needed because the effect performs an instantaneous, idempotent DOM read+scroll with no resources to release. I confirmed the surrounding code and ref lifecycle by reading the full component (AssistantTab.tsx:1-200).kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
This change replaces a smooth
scrollIntoViewwith an instant one in the assistant chat thread. Through a security lens, there is nothing to report:bottomref targets a hardcoded, empty<div>owned by the component itself (<div ref={bottom} />at line 148). It is not user-controlled and cannot be hijacked to scroll to attacker-supplied content.scrollIntoViewcall is inside auseEffectgated onhistory.dataandpending— both are local/React-Query-managed state, not raw DOM manipulation of untrusted nodes.behavior: 'smooth'; this is purely a UX workaround for a Chrome nested-scroller bug. It does not alter the scroll target, introduce new attack surface, or affect how messages are rendered.ReactMarkdownwithoutrehype-raw,disallowedElements: ['img'], andnoopener noreferreron links) is unchanged by this diff.🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 3: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
Clean one-line fix with a precise, dated inline comment explaining the Chrome quirk. No readability or maintainability concerns.
⚡ Performance — No material issues found
Verdict: No material issues found
This change is a small performance improvement, not a regression. Removing
behavior: 'smooth'eliminates expensive, overlapping smooth-scroll animations that were firing on everyhistory.data/pendingchange. From the Performance lens, there is nothing to flag.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
Findings:
web/src/editor/AssistantTab.tsx:47— TheuseEffectdepends onpending, which is recreated on every streaming step (setPending((p) => (p ? { ...p, steps: [...p.steps, step] } : p))insend()line 65). With the previousbehavior: 'smooth', this was benign because smooth scrolling didn't move the nested scroller. Switching to instantscrollIntoViewmeans the effect now forcibly snaps the view to the bottom on every single step update during streaming. In a long conversation, a user who scrolls up to read earlier messages will be yanked back down on each step, making it impossible to reference prior context while the assistant is working. This is an unhandled edge case introduced by the change.Suggested fix: Only scroll when
pendingtransitions fromnullto non-null (user sent a message) or whenhistory.datalength increases, not on intermediate step mutations. For example, track the previouspending?.messagein a ref and skip the scroll if onlypending.stepschanged, or split the concern into two effects with tighter dependencies.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.