Skip to content

fix(planreview): dock the comment box under the plan, and stop it stealing scroll - #118

Merged
tusharbhardwaj-bk merged 1 commit into
expbkmainfrom
t3code/plan-comment-box
Aug 18, 2026
Merged

tusharbhardwaj-bk merged 1 commit into
expbkmainfrom
t3code/plan-comment-box

Conversation

@tusharbhardwaj-bk

@tusharbhardwaj-bk tusharbhardwaj-bk commented Aug 18, 2026 •

Copy link
Copy Markdown
Collaborator

Four reported problems with the comment composer, three of them one root cause.

Scroll jumped to the top when the box opened

useSelectionAnchor returned before computing anything when frozen was set:

useEffect(() => {
  if (frozen) return;          // ← never subscribes, never syncs
  ...
}, [frozen, syncToSelection]);

The composer passed frozen: true, so its anchor was permanently null and it always took the fallback branch — position: absolute; bottom: 1rem inside the scrolling document — and then autofocused itself, which scrolled the plan to wherever that had landed. From the bottom of a long plan that reads as a jump to the top.

The frozen branch was mine, from the previous PR, and never worked. It and the option are removed; the toolbar simply doesn't render while the composer is open.

The box was too small, and now it docks

Per the request, the composer goes back to being docked under the plan rather than floating over it. That also fixes the scroll problem structurally: it is a sibling of the scroll container, not a child, so focusing it cannot move the plan. focus({ preventScroll: true }) is a second guard.

It starts at five lines and grows with what is typed, capped at 30vh so it can never swallow the plan it is about. Measured in JS rather than CSS field-sizing, which Safari and Firefox still lack.

"Approve with comments" was cut in half

Two buttons sharing a row in a 288px rail. They stack now, so no label truncates at any panel width.

Verification

155 tests passed across 11 scoped files; tsgo --noEmit clean for apps/web; lint and check-fork-markers.ts pass.

New tests pin the two properties that were asked for and could silently regress: the composer's root element carries no absolute/fixed/sticky positioning, and it renders min-height:6rem / max-height:30vh. Both assert against the root element's own classes — a whole-markup search would match the before:absolute and disabled: utilities every button carries, and pass for the wrong reason.

Reviewer check: scroll to the bottom of a long plan, select a line, hit Comment — the plan should not move, and the box should grow as you type and then stop at a third of the window.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…aling scroll

Writing a comment from the bottom of a long plan threw the reviewer back to the
top. `useSelectionAnchor` returned before computing anything when `frozen` was
set, so the composer's anchor was always null and it fell back to absolute
positioning *inside* the scroll container — then autofocused itself, scrolling the
plan to reach it. The frozen branch was mine and never worked; it is gone, and the
option with it.

The composer now docks below the document as a sibling of the scroll container,
so taking focus cannot move the plan, with `preventScroll` as a second guard. It
is also the size a review comment deserves: five lines to start, growing with
what is typed up to 30vh so it can never swallow the plan it is about.

Separately, "Approve with comments" was being cut in half — two buttons sharing a
row in a 288px rail. They stack now, so no label truncates at any panel width.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L labels Aug 18, 2026
@github-actions

Copy link
Copy Markdown

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 11.4 KiB — 15.1 KiB ✅
Codex Thread snapshot wire — 5.7 KiB — 7.3 KiB ✅
Codex Live turn WebSocket wire — 5.7 KiB — 7.8 KiB ✅
Codex Live turn WebSocket decoded — 50.3 KiB — 66.4 KiB ✅
Codex Live turn messages — 10 — 21 ✅
Claude Total thread wire — 11.4 KiB — 15.1 KiB ✅
Claude Thread snapshot wire — 5.7 KiB — 7.3 KiB ✅
Claude Live turn WebSocket wire — 5.7 KiB — 7.8 KiB ✅
Claude Live turn WebSocket decoded — 51.2 KiB — 66.4 KiB ✅
Claude Live turn messages — 11 — 21 ✅

Baseline: unavailable · PR result: 02c858a · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 95.6 KiB
  • Claude decoded thread snapshot: 96.3 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@tusharbhardwaj-bk
tusharbhardwaj-bk merged commit d1690c5 into expbkmain Aug 18, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant