Skip to content

fix(fork): clip the composer placeholder out of the prompt scrollport - #31

Merged
NoahHendrickson merged 3 commits into
customfrom
fix/fork-composer-placeholder-overflow
Jul 29, 2026
Merged

fix(fork): clip the composer placeholder out of the prompt scrollport#31
NoahHendrickson merged 3 commits into
customfrom
fix/fork-composer-placeholder-overflow

Conversation

@NoahHendrickson

Copy link
Copy Markdown
Owner

Follow-up to #28. The scrollport move stopped the editor's own ink from inflating a scroll container, but missed the editor's siblings.

The bug

On the merged build the composer showed a scrollbar while the prompt was empty, focused or not, and it disappeared on typing — the inverse of the phantom #28 fixed.

Root cause (measured live)

  • Wrapper scrollHeight 17 vs clientHeight 16 on an empty prompt.
  • The Lexical placeholder is the editor's sibling, outside the editor's overflow-y: hidden clip. At the fork's 14/16 line box its text line box is 17px inside its 16px inset-0 box, and scrollable overflow of an absolutely-positioned descendant propagates to the wrapper scrollport.
  • Typing unmounts the placeholder → overflow gone → scrollbar gone. Hiding just the placeholder in devtools dropped scrollHeight to 16, confirming it as the sole contributor.

Fix

One rule in the existing >=40rem block: [data-fork-composer-prompt] [data-testid="composer-editor"] ~ div { overflow-y: hidden } — the same general-sibling seam the type-ramp rule already uses (catches the placeholder and Lexical's zero-height spacer). The clip matches what typed text already gets from the editor's own overflow-y: hidden, so paint is unchanged. Below 40rem nothing changes.

Guard extended in forkComposerShell.test.ts (same cssRules at-rule-ancestry style pinning the rule inside the media block); manifest intent updated.

Verified against the running app

  • Empty prompt: wrapper not scrollable, no thumb (slim and tall shells share the seam)
  • Growing content under the 200px cap: no thumb
  • Past the cap: wrapper scrolls (368px content, 168px scroll travel)
  • Cleared: placeholder returns, still not scrollable
  • Guard suite 21 files / 166 tests passing; files formatted

🤖 Generated with Claude Code

The scrollport move (#28) stopped the editor's own ink from inflating a
scroll container, but the placeholder sits beside the editor, outside its
clip. At the fork's 14/16 line box the placeholder's line box is a pixel
taller than its inset-0 box, and absolutely-positioned overflow propagates
to the wrapper's scrollable overflow — so the wrapper painted a scrollbar
exactly while the prompt was empty, and typing (which unmounts the
placeholder) made it vanish.

Clip the editor's siblings (placeholder and Lexical's zero-height spacer)
the way the editor clips its own ink, in the same >=40rem media block.
Verified live: empty prompt not scrollable, content under the 200px cap
grows without a thumb, content past it scrolls on the wrapper, clearing
returns to a clean empty state.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Jul 28, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S labels Jul 28, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thermo-nuclear: not approved.

The empty-prompt phantom thumb diagnosis is right, but the fix adds a fourth near-duplicate selector in a media block that already styles editor + siblings via :is([data-testid="composer-editor"], [data-testid="composer-editor"] ~ div).

Code-judo: collapse the editor unclamp and the new sibling clip into one prompt-scoped :is(...) with max-height: none; overflow-y: hidden;, delete the sibling-only rule, and point the guard at that unified selector. That removes the new complexity instead of documenting it, and avoids further sprawl on an already >1k theme.custom.css.

Behavior can stay identical; the structure should get smaller before this merges.

Open in Web View Automation 

Sent by Cursor Automation: Thermo-nuclear PR review

Comment on lines +408 to +413
:root[data-fork="noahhendrickson-t3code"]
[data-fork-composer-prompt]
[data-testid="composer-editor"]
~ div {
overflow-y: hidden;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fourth selector is unnecessary. The media block already uses :is([data-testid="composer-editor"], [data-testid="composer-editor"] ~ div) for the type ramp — reuse that shape for the clip.

Fold the editor unclamp and this sibling-only rule into one prompt-scoped :is(...):

:root[data-fork="noahhendrickson-t3code"]
  [data-fork-composer-prompt]
  :is([data-testid="composer-editor"], [data-testid="composer-editor"] ~ div) {
  max-height: none;
  overflow-y: hidden;
}

max-height: none on the abspos spacer/placeholder is harmless (initial value; they are sized by inset). Delete this sibling-only rule and most of the comment; keep a short note that siblings must share the clip because abspos overflow bypasses the editor. Update the fork guard to assert the unified :is(...) rule instead of locking in a fourth near-duplicate.

Net: −1 rule, reuse the existing selector shape, and stop growing an already >1k theme.custom.css for a duplicate.

Copy link
Copy Markdown
Owner Author

Review: the diagnosis is right, the primitive is wrong

The root-cause analysis here is excellent and I could not fault it — abspos scrollable overflow propagating to the nearest scroll container is exactly the mechanism, the media scoping is right, and below 40rem there is no wrapper scrollport so there is genuinely no exposure. The scope discipline is also right: three files, one declaration, guard + manifest updated.

But overflow-y: hidden is the wrong tool for "clip this element," and it introduces a new phantom-scrollbar bug of the same family this PR series has been chasing — rotated 90°.

1. overflow-y: hidden makes the placeholder a scroll container, and it can eat the entire prompt line

Per CSS Overflow, when one axis is not visible/clip and the other is visible, the visible one computes to auto. So this rule silently sets overflow-x: auto on the placeholder, turning a pure-paint element into a scroll box. If the placeholder text has no break opportunity wider than the prompt, Chromium reserves horizontal scrollbar space out of a 16px-tall box — and there is nothing left to paint the text in.

I reproduced the fork's box structure (wrapper scrollport → div.relative → editor + spacer + inset-0 placeholder at 14/16) in headless Chromium and measured all three options against the same placeholder string, Run rm -rf /Users/noah/projects/t3code/apps/web/src/components/chat/ComposerPendingApprovalPanel.tsx:

variant computed overflow-x placeholder clientHeight wrapper scrollable (the #31 bug)
no fix (merged build) visible 16 yes
this PR — overflow-y: hidden auto 1 no
overflow-y: clip visible 16 no

A screenshot of the three side by side is unambiguous: under this PR's rule the placeholder text does not render at all — the 16px box is entirely consumed by a horizontal scrollbar. Under clip the text renders and the vertical phantom is still fixed. Both fixes stop the propagation equally (wrapper scrollHeight 17 → 16); only hidden pays for it with the other axis.

This is reachable in production, not theoretical. ChatComposer.tsx:3210 passes activePendingApproval?.detail straight through as the placeholder, and that is server/model-supplied text. A path, a URL, and a commit SHA all reproduced it (scrollWidth 604 / 623 / 536 against a 400px box) — Chromium does not take a break opportunity at / under default word-break. The prompt value is "" in the approval state, so the placeholder is exactly what is on screen when this fires.

Suggested change — one word:

  :root[data-fork="noahhendrickson-t3code"]
    [data-fork-composer-prompt]
    [data-testid="composer-editor"]
    ~ div {
    overflow-y: clip;
  }

clip clips without creating a scroll container, so the other axis stays visible and neither axis can ever paint a scrollbar or be scrolled programmatically. It is strictly the safer primitive for "this element is paint, not a scrollport," and it is already used in this codebase (index.css:1028, index.css:1528, overflow-clip in ui/tooltip.tsx and ui/popover.tsx), so it breaks no new browser-support ground.

Caveat, stated plainly: my repro used default scrollbar metrics in headless Chromium, not the running app. The fork styles ::-webkit-scrollbar globally (index.css:1076, --app-scrollbar-width: 6px), which forces classic space-reserving scrollbars — but that rule sets width only, so the horizontal bar's height is the UA default. Worth confirming live, the same way the original 17-vs-16 was measured. The computed-value change to overflow-x: auto is not environment-dependent regardless.

The editor's own overflow-y: hidden from #28 has the identical side effect, but it is whitespace-pre-wrap wrap-break-word so it cannot overflow horizontally, and changing that rule is #28's business — separate change, per the repo's no-mixed-fixes rule.

2. A wrapping placeholder is now silently truncated

Independent of which primitive is used, and inherent to clipping. Measured with a two-line placeholder in a 400px box: scrollHeight 34 against clientHeight 16. Before this PR the wrapper scrolled, so the second line was reachable; after, it is clipped with no affordance.

For "Ask anything, @tag files/folders, $use skills, or / for commands" at desktop widths this never fires. But activePendingApproval.detail is arbitrary prose, and the composer can be narrow while the viewport is ≥40rem. The manifest's justification — "the clip matches what typed text already gets… so paint is unchanged" — only reasons about the one-line case and is not true for the wrapping one. I would either accept it and say so in the manifest, or truncate the placeholder deliberately so it degrades to an ellipsis instead of a hard cut.

3. The guard's editor lookup now also matches the new rule

cssRules returns rules in source order and rules.find takes the first match. The editor predicate is inDesktopLineBoxMedia && includes("[data-fork-composer-prompt]") && includes('[data-testid="composer-editor"]') — and the new sibling selector satisfies all three. It resolves correctly today only because the unclamp rule happens to sit earlier in the file. Reorder the two blocks and the max-height: none assertion starts interrogating the sibling rule and fails for a reason that has nothing to do with the invariant.

Given this file already ate one bug from a predicate that was satisfied by the wrong thing (the @media prefix-match noted in the comment at forkComposerShell.test.ts:191), this seam deserves the same treatment:

const editor = rules.find(
  (rule) =>
    inDesktopLineBoxMedia(rule) &&
    rule.selector.includes("[data-fork-composer-prompt]") &&
    rule.selector.includes('[data-testid="composer-editor"]') &&
    !/~\s*div/u.test(rule.selector),
);

plus expect(siblings).not.toBe(editor) to pin that these are two distinct rules.

4. The guard pins the mechanism, not the invariant

expect(siblings!.body).toMatch(/overflow-y:\s*hidden/u) fails if anyone adopts clip — the guard would block its own fix. The invariant is "the editor's siblings do not contribute scrollable overflow to the wrapper," so /overflow-y:\s*(hidden|clip)/u expresses it without freezing the primitive.

5. ~ div now carries clipping, not just type — worth a watch note

I checked: every Lexical plugin in ComposerPromptEditor.tsx returns null, so today the seam matches only the placeholder and Lexical's spacer, exactly as the PR says. No live bug. But the type-ramp rule's ~ div sets font-size and leading — benign for anything that lands there — whereas this rule clips. The first plugin that renders a mention or command dropdown in place rather than through a portal gets clipped to the prompt's line box and the cause will be non-obvious. One line in the manifest's watch note is enough.

Question

With the type ramp already setting line-height: 16px on this exact ~ div selector, what makes the placeholder's line box 17px inside a 16px box? If it is sub-pixel rounding surfacing through scrollHeight, then the overhang is font- and zoom-dependent, the clip is the right remedy anyway, and the manifest's "its text line box is 17px" is worth recording as a measurement rather than a mechanism — otherwise the next reader will go looking for a leading bug that is not there.


Verdict: finding 1 is worth fixing before merge — it is a one-word change that trades a scroll container for a clip and removes a live regression path through the approval-state placeholder. 3 and 4 are cheap and keep this guard honest. 2 and 5 are judgement calls I would at least write down.

Not run locally: this container has no installed workspace deps, so I could not execute the guard suite — CI and your own run cover that. Everything above is from reading the diff plus the Chromium measurements described.


Generated by Claude Code

Review caught overflow-y: hidden computing the sibling's overflow-x to
auto — a pure-paint element became a scroll box, and the approval-state
placeholder is server-supplied text with no break opportunity, so the
reserved horizontal scrollbar would consume the entire 16px line.

Live measurement while applying the fix surfaced the adjacent case: with
x left visible (overflow-y: clip alone), that same unbreakable text
propagates sideways overflow into the wrapper, whose overflow-x computes
to auto alongside its overflow-y: auto, and paints a 16px horizontal
scrollbar there — a latent approval-state bug that exists on custom
today. overflow: clip contains the siblings on both axes without
creating a scroll container anywhere.

Verified live: empty prompt not scrollable; a 1877px unbreakable
placeholder in a 736px box renders its 16px line with zero reserved
scrollbar height and zero sideways scroll range on the wrapper.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@NoahHendrickson

Copy link
Copy Markdown
Owner Author

Review addressed in f2b06db — and it went one step further than the suggestion.

Taken: clip, and on both axes. The overflow-y: hidden → computed overflow-x: auto analysis is confirmed; the approval-state placeholder (activePendingApproval?.detail, ChatComposer ~3211) is indeed server-supplied and unbreakable. But measuring overflow-y: clip live surfaced the adjacent case the repro table didn't cover: with x left visible, the same unbreakable text propagates sideways overflow into the wrapper, whose overflow-x computes to auto alongside its overflow-y: auto — measured a 16px-tall horizontal scrollbar on the wrapper and 939px of sideways scroll range. That latent approval-state bug exists on merged custom today (the placeholder's overflow was fully visible before this PR). overflow: clip closes both in the same declaration; verified live that a 1877px placeholder in a 736px box renders its 16px line with zero reserved scrollbar height and zero wrapper scroll range, and the original empty-prompt fix still holds.

Declined (Cursor): merging into the editor's :is(...). After this change the two rules intentionally diverge — the editor keeps max-height: none; overflow-y: hidden (safe there: pre-wrap + wrap-break-word forecloses horizontal overflow, and changing #28's rule is out of scope per the no-mixed-fixes rule), while the siblings need overflow: clip for the reasons above. One :is() can no longer express both.

🤖 Generated with Claude Code

@github-actions github-actions Bot added size:M and removed size:S labels Jul 28, 2026
The `editor` rule lookup matched on prompt scope + the composer-editor
testid inside the >=40rem media — three predicates the sibling clip rule
added in f2b06db satisfies as well. `rules.find` returned the unclamp
rule only because it happens to sit earlier in theme.custom.css.

Verified by swapping the two blocks: the guard failed with
`expected '\n overflow: clip;\n ' to match /max-height:\s*none/u` — the
unclamp assertion interrogating the clip rule, a failure with nothing to
do with the invariant it names. With the `~ div` exclusion the same swap
stays green, and `not.toBe(editor)` pins the two rules as distinct so a
future collapse into one selector is caught rather than silently halving
the coverage.

Same class of wrong-thing-satisfied-the-predicate bug as the @media
prefix match already recorded in this file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@NoahHendrickson

Copy link
Copy Markdown
Owner Author

Addressed the remaining guard nit from the review (finding 3) in e1a91b4.

The editor lookup matched on prompt scope + the composer-editor testid inside the >=40rem media — three predicates the sibling clip rule satisfies too. rules.find was returning the unclamp rule only because it happens to sit earlier in theme.custom.css.

Confirmed it was live, not theoretical: swapping the two CSS blocks failed the suite with expected '\n overflow: clip;\n ' to match /max-height:\s*none/u — the unclamp assertion interrogating the clip rule, failing for a reason unrelated to the invariant it names. With !/~\s*div/u added the same swap stays green. Also took the suggested expect(siblings).not.toBe(editor), which pins the two as distinct rules so a future collapse into one selector is caught rather than silently halving coverage.

Findings 2 and 5 (wrapping-placeholder truncation; a watch note for the day a Lexical plugin renders in place rather than through a portal) stay as recorded judgement calls — neither changes behaviour today, and both are manifest prose rather than code.

27 tests in this file green, vp lint clean.

@NoahHendrickson
NoahHendrickson merged commit 4975596 into custom Jul 29, 2026
10 checks passed
@NoahHendrickson
NoahHendrickson deleted the fix/fork-composer-placeholder-overflow branch July 29, 2026 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 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