Skip to content

fix(mobile): coalesce keyboard viewport settling - #229

Merged
Ark0N merged 3 commits into
Ark0N:masterfrom
Lint111:feat/keyboard-viewport-settle
Aug 8, 2026
Merged

fix(mobile): coalesce keyboard viewport settling#229
Ark0N merged 3 commits into
Ark0N:masterfrom
Lint111:feat/keyboard-viewport-settle

Conversation

@Lint111

Copy link
Copy Markdown
Contributor

Reopening this as the first of the three from #173, rebased onto current master (fa1700d, after #227 landed). Same fix as the original #194, with a stronger test — details below.

Summary

Coalesce the burst of mobile visualViewport resize events into one final xterm layout and PTY resize after the software-keyboard animation settles.

The bug

Keyboard open and close used separate fixed 150 ms and 100 ms timers. Mobile browsers report several intermediate viewport heights during the OS animation, so each step armed its own timer and refit xterm against a transient height. The visible symptom is repeated reflow while the keyboard slides up, with the terminal briefly laid out against a size that is already stale.

To see it: on a phone (or a touch-emulated viewport), focus the terminal so the keyboard animates in. Each intermediate viewport height triggers its own fit; the measurement below counts three for a three-step animation.

What changed

  • one 80 ms settle timer, reset on every viewport resize, instead of per-event fixed timers
  • the open/close request to return the terminal to the bottom is preserved across reschedules
  • the existing fit, padding correction, helper synchronisation and resize transport run only once the viewport goes quiet
  • pending settlement is cancelled during handler cleanup

Only src/web/public/mobile-handlers.js changes. That file has had no commits on master since this branch point, so the rebase was clean.

Test

test/mobile/keyboard.test.ts gains two cases:

  1. refits once for a burst of keyboard viewport steps — drives the public onKeyboardShow three times in quick succession and asserts exactly one refit.

    On master this fails with expected 3 to be 1: one refit per viewport step. With the change it is 1. That is the behavioural difference stated as a number rather than an absent-method error.

  2. coalesces keyboard animation frames into one final terminal fit — the finer-grained version, asserting nothing fires before the settle window and exactly one fit/resize/scroll-restore fires after.

Both needed a harness fix to be meaningful at all: the suite never selects a session, so initTerminal() never runs and app.terminal / app.fitAddon are both null. _scheduleViewportSettle returns early on a falsy terminal, so as written the assertions never reached the code they claimed to cover — they errored on Cannot read properties of null. The tests now install the minimum surface the settle callback touches and restore it afterwards.

Validation

  • npm run test:ci4033 passed, 0 failed
  • test/mobile/keyboard.test.ts — both new cases pass; 8 failures remain, all pre-existing on unmodified master (same null-initialisation limit of the headless harness, unrelated to this change). I verified the identical 8 fail on a clean origin/master checkout.
  • tsc --noEmit, npm run lint, npm run check:frontend-syntax, prettier --check — all clean

Worth noting test/mobile/** is excluded from CI (config/vitest.ci.config.ts), so CI will not exercise the new tests. They are reproducible locally with:

npx vitest run --config config/vitest.config.ts test/mobile/keyboard.test.ts -t "refits once for a burst"

Scope

No change to resize arbitration, mobile terminal controls, input handling, session replay, or frame capture. Two commits: the fix, then the test-harness repair.

The suite never selects a session, so initTerminal() does not run and both
`app.terminal` and `app.fitAddon` are null at rest. `_scheduleViewportSettle`
returns early on a falsy terminal, so the coalescing assertions could not
reach the behavior they claimed to cover -- the test errored on
`Cannot read properties of null` rather than measuring anything.
Installs the minimum surface the settle callback touches and restores it
afterwards, so the coalescing path executes for real.
Adds a behavioral counterpart driven through the PUBLIC entry point
(`onKeyboardShow`) instead of the internal scheduler: three viewport steps
in quick succession must produce exactly ONE refit. On master that returns
3 (each show arms its own uncoalesced 150ms timeout), so this fails by
COUNT rather than by a missing method -- which is the failure mode that
actually demonstrates the bug.
Verified: `expected 3 to be 1` on unmodified master; passes here. The
remaining 8 failures in this file are pre-existing on master and unrelated
(same null-initialization limitation of the headless harness).
CopilotAI lite review requested due to automatic review settings August 8, 2026 05:49
CopilotAI reviewed Aug 8, 2026

CopilotAI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

A visualViewport resize event without a pending show/hide transition now
only pushes a pending settle back (_deferViewportSettle) instead of arming
fit + PTY-resize work of its own. Keyboard detection can miss a
fine-grained OS animation entirely (each step under 150px, with the
baseline chasing the animation down), while MobileDetection's own listener
still shrinks --app-height, so the per-event settle fitted xterm against a
mid-animation container with no keyboard CSS compensation and resized the
PTY to transient dims. The resulting SIGWINCH thrash (58 -> 10 -> 50 rows)
duplicated prompts and left tmux dot filler in the transcript on keyboard
close. Reproduced with a faked visualViewport driving the real handler;
master is unaffected because it never resized the PTY from this path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Ark0N

Ark0N commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Thanks @Lint111, good fix and a much stronger test than the original #194.

Manual testing on an iPhone hit one regression before merge: closing the keyboard corrupted the terminal view (duplicated prompts, tmux dot-filler rows). Root cause: scheduling the settle from handleViewportResize on every viewport event arms fit + PTY-resize work even when the show/hide state machine never engaged. Keyboard detection can miss a fine-grained OS animation entirely (each step under the 150px threshold, with the baseline chasing the animation down), while MobileDetection.updateAppHeight() still shrinks --app-height, so the settle fitted xterm against a mid-animation container with no keyboard CSS compensation and resized the PTY to transient dims (58 -> 10 -> 50 rows in my repro). That SIGWINCH thrash is what garbles the transcript. Master is unaffected on that path because it never resized the PTY from handleViewportResize at all.

I pushed 1f164bc to your branch (maintainer edit): only a real show/hide transition arms the settle work; handleViewportResize now calls _deferViewportSettle(), which pushes a pending settle back and is otherwise a no-op. Your coalescing behavior is preserved (one fit per transition, after the viewport goes quiet), your two tests pass unchanged, and a third test pins the new gate. Verified with a faked-visualViewport harness driving the real handler in both animation profiles, plus on-device on an iPhone: corruption gone, single clean refit on open and close.

The baseline-chasing detection flaw itself (sub-150px steps never flip keyboardVisible) predates this PR and is worth a follow-up of its own.

Merging, thanks again!

@Ark0N
Ark0N merged commit 2524759 into Ark0N:masterAug 8, 2026
2 checks passed
Sign up for freeto 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.

4 participants

@Lint111@Ark0N@claude