Skip to content

perf(ui): make the composer in-place measurement work on phone viewports - #552

Merged
sanity merged 1 commit into
mainfrom
fix-468-mobile
Jul 29, 2026
Merged

perf(ui): make the composer in-place measurement work on phone viewports#552
sanity merged 1 commit into
mainfrom
fix-468-mobile

Conversation

@sanity

@sanity sanity commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #543 (issue #468). Fixes a regression that #543 introduced on
phone-width viewports, found by independent review.

Problem

#543 sizes the composer by measuring it in place, which only works while the
textarea reports scrollHeight > clientHeight. Below 768px tailwind.css
clamps the body font to 13px; at the default line-height: 1.5 that is a
19.5px line box, so one line plus py-2.5 came to 41.5px against the
min-h-[44px] minimum. min-height then owned the box, the two metrics were
equal on every keystroke, and the fast path never applied.

That is not merely "no benefit" — it is worse than doing nothing. The probing
read is wasted and the collapse forces a second layout, where collapsing
unconditionally forced one. So #543 made its own target metric worse on the
weakest hardware.

Confirmed in the built app, not in a model of it. The recorded op trace per
keystroke at 390px:

pre-#543      W:height=auto  R.sh:42  W:height=42px          <- 1 forced layout
#543 merged   R.sh:42  R.ch:42  W:height=auto  R.sh:42  W:height=42px   <- 2
this change   R.sh:44  R.ch:42                                <- 1, and no writes

CDP Performance.getMetrics at 390x844 over 26 keystrokes:

LayoutCount RecalcStyleCount LayoutDuration RecalcStyleDuration
pre-#543 78 99 7.95 ms 17.05 ms
#543 as merged 77 86 12.10 ms 19.49 ms
this change 28 38 5.58 ms 3.85 ms

Note the honest detail: LayoutCount barely separates the first two rows,
because frame-level layouts coalesce and hide the extra forced one. The op
trace is the precise evidence for the forced-layout claim; LayoutDuration
(+52% as merged) is the metric that shows the cost.

Break-even measured by sweeping viewports: the fast path fails at 390 / 393 /
414px and works from 600px up, matching the ~587px predicted from the font
clamp.

Approach

leading-6 pins the line box to 24px, so one line of text plus padding clears
the 44px minimum at every body font size and the in-place measurement
applies everywhere. Verified across 390 / 393 / 414 / 600 / 640 / 768 / 1280:
fastPath=true, zero writes and zero collapses over five keystrokes at all of
them.

Resting composer height is unchanged at 44px at every viewport (min-height was
already producing 44px on phones), so there is no visual change at rest. The
only visual difference is that multi-line drafts on phones get 24px line
spacing instead of 19.5px — a 2-line draft goes 63.5px to 68px. Checked at
390x844 for horizontal overflow (none) and that the composer stays fully on
screen at the clamped maximum.

Alternatives considered: making the algorithm stateful so it could predict a
failing probe, or remembering the failure. Both add state that needs
invalidating on viewport change. Fixing the geometric inconsistency — a
composer whose natural height sits below its own minimum — is the smaller and
more durable change, and it makes the invariant unconditional rather than
configuration-dependent.

Testing

The pin missed this because both composer tests fixed the viewport at
1280x800, and test.use({viewport}) overrides the project's device
viewport
— so running under mobile-chrome / mobile-safari varied the engine
but not the layout regime. There was no composer-height coverage below 768px
at all.

  • The test body is now a shared function run at both 1280x800 and 390x844.
  • It asserts scrollHeight > clientHeight after priming, before relying on it,
    so a configuration where the measurement cannot work fails loudly instead
    of passing vacuously
    . That guard is what makes the phone case a real
    discriminator rather than a second copy of the desktop one.

Verified non-vacuous by rebuilding the wasm and re-running: with leading-6
removed, the phone test fails and the desktop test still passes —

Error: the composer must be sized by its content, not by min-height
(scrollHeight 42, clientHeight 42, font 13px, line-height 19.5px, min-height 44px)

Full Playwright suite on the rebased HEAD: 707 passed, 23 skipped, 0 failed.
The tests called out on #468 pass on all five projects: textarea height resets after sending a multi-line message (#221) and edit container fits within viewport at narrow widths (#205). cargo fmt --check clean, clippy adds no
warnings in the changed file, cargo test -p river-ui 792 passed.

One flake appeared on an earlier (pre-rebase) full-suite run — mobile-touch-ux
"React from the kebab", failing in waitForApp at app boot — and did not recur
on the rebased run. It passed 5/5 in isolation. I am deliberately not
asserting a root cause: the machine was at load 56 on 16 cores from parallel
agent jobs, which would explain it, but #545's note on #538 records that this
exact "timeout under parallel load" theory was plausible and wrong before. With
trace: "on-first-retry" now in place from #545, the next occurrence will
leave evidence worth reading.

Does leading-6 also dodge iOS Safari's zoom-on-focus?

No, and it cannot — raised on review as unverified, so answering it rather than
leaving it open.

That behaviour keys off font-size, not line-height: iOS Safari zooms the
page when a focused input's font size is under 16px. leading-6 sets only
line-height, so it leaves the trigger condition exactly as it was. Measured
computed font-size on the textarea, unchanged by this PR:

viewport 390 393 414 600 640 768 1280
font-size 13px 13px 13px 15px 16px 16px 16px

clamp(13px, 2.5vw, 16px) reaches 16px at a 640px viewport, so any width below
640px puts the composer under the threshold. If the zoom-on-focus happens today
it happened before this PR too, and it still will after.

Out of scope here, and I could not have verified it anyway: the harness has
no real iOS. Playwright's mobile-safari emulates viewport and user agent, not
the native zoom heuristic, so a green run there would have been no evidence
either way. Fixing it means raising the composer's font to 16px on phones,
which is a deliberate typography decision affecting the mobile clamp, not a
side effect to smuggle into a perf PR.

Refs #468

[AI-assisted - Claude]

#543 sized the composer by measuring in place, which is only possible while
the textarea reports `scrollHeight > clientHeight`. Below 768px `tailwind.css`
clamps the body font to 13px, and at the default `line-height: 1.5` that is a
19.5px line box, so one line plus `py-2.5` came to 41.5px against the
`min-h-[44px]` minimum. min-height then owned the box, the two metrics were
equal on every keystroke, and the fast path never applied.

That is worse than doing nothing: the probing read is wasted and the collapse
forces a SECOND layout, where collapsing unconditionally forced one. Measured
in the built app at 390px over 26 keystrokes:

  pre-#543          78 layouts, 99 recalcs,  7.95 ms layout, 17.05 ms style
  #543 as merged    77 layouts, 86 recalcs, 12.10 ms layout, 19.49 ms style
  this change       28 layouts, 38 recalcs,  5.58 ms layout,  3.85 ms style

The recorded op trace shows the mechanism directly: as merged, every keystroke
below 768px ran read -> write auto -> read -> write, against pre-#543's
write auto -> read -> write.

`leading-6` pins the line box to 24px, so one line clears the 44px minimum at
every body font size and the in-place measurement applies everywhere. Resting
composer height is unchanged (44px at every viewport); only multi-line drafts
on phones get the roomier 24px line spacing.

The pin missed this because both composer tests fixed the viewport at
1280x800, and `test.use({viewport})` overrides the project's device viewport —
so running under mobile-chrome / mobile-safari varied the engine but not the
layout regime. The test body now runs at 390x844 as well, and asserts
`scrollHeight > clientHeight` before relying on it, so a configuration where
the measurement cannot work fails loudly instead of passing vacuously.

Refs #468
@sanity

sanity commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Self-review (first pass) — 2 lenses, run serially

Not the gating review; independent reviewers run separately. Recording what I
checked and, more usefully, the one bound I could not remove.

Lens 1 — code-first

# Claim under test Result
1 leading-6 is actually generated by the Tailwind v4 scan of *.rs Clean. Computed line-height: 24px at all seven viewports measured. Not inferred from the class list.
2 Nothing else on the textarea sets a conflicting line-height Clean. No other leading-*; computed value is 24px.
3 Both describes really run at different viewports (test.use scoping through a shared function) Clean, and proven rather than assumed: with leading-6 removed the phone test fails while the desktop test passes. If scoping were broken both would behave alike.
4 selectRoom widens to 1280 to click the room at 390px — does it leave the composer sized for the wrong width? Clean. It restores the viewport before any typing, and the test primes with a keystroke before measuring. Guard reads scroll 44 / client 42 at 390px.
5 The phone viewport doesn't invalidate the growth assertions (toHaveLength(1), > oneLine + 10, clamp) Clean. 44 -> 68 -> 116 -> 168 at 390px; "second line" does not wrap at that width.

Lens 2 — skeptical

# Attack Result
A Does leading-6 disturb the desktop regime it was fine in? No-op there. Desktop was already 16px x 1.5 = 24px. Desktop measurements are byte-identical before and after.
B The clamp is a range, not two values — is the middle covered? Clean. With line-height pinned in rem, the body font size stops mattering at all. Verified at 600px (font 15px) as well as the 13px and 16px ends.
C Is the premise guard tautological with the assertion that follows it? No. The guard checks one state after priming; the no-writes assertion checks behaviour over 19 keystrokes. The write-always mutation from the previous round fails the second while passing the first.

The bound I could not remove, stated plainly: this works because the mobile
clamp targets body, while leading-6 resolves in rem against the root
font — so the clamp cannot touch it. That is the whole mechanism. It does mean
a user who sets their browser's default font size below ~14.67px would put
1.5rem back under the 44px minimum and silently lose the fast path again. CI
runs at the default, so no test covers that, and I would rather say so than
imply the invariant is unconditional. The code comment is worded as "every body
font size", which is the claim actually supported.

Worth noting the failure stays safe in that case — the composer still sizes
correctly, it just pays the slower path. This is a performance invariant, not a
correctness one.

[AI-assisted - Claude]

@sanity
sanity merged commit 96f0bf0 into main Jul 29, 2026
6 checks passed
@sanity
sanity deleted the fix-468-mobile branch July 29, 2026 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant