Enable terminal input on mobile devices#652
Enable terminal input on mobile devices#652JMBattista wants to merge 2 commits intoasheshgoplani:mainfrom
Conversation
The web terminal previously blocked input on touch devices via
`disableStdin: mobile`, gated `terminal.onData` behind `!mobile`, and
installed a touchstart preventDefault that stopped xterm's hidden
textarea from receiving focus (suppressing the soft keyboard). That
was fine before Tailscale-exposed deployments, but users accessing
`agent-deck web` from a phone could only view, not type.
All mobile input restrictions were client-side; the server already
accepts `input` WS messages and gates them on `--read-only`. This
commit removes the mobile gate in `TerminalPanel.js` so:
- `disableStdin` is now driven solely by the server-sent `readOnly`.
- `terminal.onData` is registered unconditionally.
- The mobile `touchstart` preventDefault is deleted, letting xterm's
hidden textarea focus on tap and pop the soft keyboard.
- The yellow "READ-ONLY: terminal input is disabled on mobile"
banner is removed; the `--read-only` flag remains the sole source
of truth for read-only deployments.
Touch-drag scrollback continues to work: `installTouchScroll` never
called preventDefault on touchstart, only on touchmove. The removed
listener was the mobile-only suppressor, not the scroll handler.
Battery optimizations unrelated to input are kept unchanged:
- `cursorBlink: !mobile`
- WebGL preload is skipped on mobile
- `terminal.focus()` on connect stays desktop-only so opening a
session on mobile doesn't auto-pop the keyboard.
Test updates (TDD-first):
- `tests/e2e/visual/p1-bug6-terminal-padding.spec.ts`: flip the
banner assertion to pin absence; add two new structural tests
asserting `terminal.onData` is not gated on `!mobile` and
`disableStdin` is not OR-ed with `mobile`.
- `tests/e2e/visual/p8-perf-e-listener-cleanup.spec.ts`: drop
expected `controller.signal` count from 9 to 8 (the mobile-only
touchstart listener is gone) and update doc comments.
Verification:
- `go test ./internal/web/... -race -count=1` passes
(TestTmuxPTYBridgeResize is pre-existing flaky on HEAD, same rate
with and without this change).
- Updated/new Playwright structural specs pass (5/5).
There was a problem hiding this comment.
Pull request overview
Enables interactive terminal keyboard input on mobile by removing mobile-only read-only restrictions and relying solely on the server-provided readOnly signal to gate stdin.
Changes:
- Remove mobile-only UI/input blocking (banner, touchstart preventDefault,
terminal.onDatagating). - Always register
terminal.onData()to forward keystrokes on both desktop and mobile. - Update Playwright structural tests to reflect the new unified input behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
internal/web/static/app/TerminalPanel.js |
Removes mobile-only read-only banner and enables stdin + keystroke forwarding on mobile (server readOnly is sole gate). |
tests/e2e/visual/p8-perf-e-listener-cleanup.spec.ts |
Updates listener-count expectations after removing the mobile-only touchstart handler. |
tests/e2e/visual/p1-bug6-terminal-padding.spec.ts |
Updates structural assertions to require that the legacy mobile read-only banner and mobile input gating are absent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test('structural: contains controller.signal at least 8 times (one per addEventListener site)', () => { | ||
| const src = source(); | ||
| const matches = src.match(/controller\.signal/g) || []; | ||
| expect( | ||
| matches.length, | ||
| `Expected controller.signal to appear on every addEventListener site (>=9), found ${matches.length}. Sites: 4 touch on container + 1 window resize + 1 anonymous touchstart + 4 ws.`, | ||
| ).toBeGreaterThanOrEqual(9); | ||
| `Expected controller.signal to appear on every addEventListener site (>=8), found ${matches.length}. Sites: 4 touch on container + 1 window resize + 4 ws. (Mobile-only touchstart preventDefault was removed when mobile input was enabled.)`, | ||
| ).toBeGreaterThanOrEqual(8); |
There was a problem hiding this comment.
PERF-E structural scanner no bare addEventListener call lacking a signal option will currently flag controller.signal.addEventListener('abort', ...) in TerminalPanel.js, because it matches addEventListener( but cannot (and should not) include an options object with signal:. This will make the test fail/flake unless the scanner excludes AbortSignal/EventTarget cases like *.signal.addEventListener(...) or is scoped to the DOM/WebSocket listeners you actually want to enforce.
| * - 8 addEventListener sites inside the main useEffect: | ||
| * - 4 container touch listeners (touchstart, touchmove, touchend, touchcancel) | ||
| * - 1 window resize listener (already uses a LOCAL windowResizeController -- partially done) | ||
| * - 1 anonymous touchstart preventDefault on container (mobile only) | ||
| * - 4 ws.addEventListener (open, message, error, close) | ||
| * - The mobile-only anonymous touchstart preventDefault was removed when | ||
| * mobile console input was enabled; that dropped the site count from 9 to 8. | ||
| * - Only 1 of these currently uses controller.signal (the window resize block). | ||
| * - The existing cleanup at line 67 only manually removes the first touchstart; the | ||
| * remaining 8 listeners leak on every unmount / reconnect. | ||
| * remaining 7 listeners leak on every unmount / reconnect. |
There was a problem hiding this comment.
The header comment block is now inconsistent with TerminalPanel.js: it claims there are 8 addEventListener sites and that only 1 uses controller.signal, plus it describes an existing leak. In the current TerminalPanel.js, all touch/window/ws listeners already pass { signal: controller.signal } and cleanup calls controller.abort(). Please update/remove the outdated 'CURRENT STATE' commentary so the spec doesn’t mislead future maintenance.
| test('structural: mobile READ-ONLY banner is absent (mobile input enabled)', () => { | ||
| const src = readTerminalPanelSrc(); | ||
| expect( | ||
| /READ-ONLY: terminal input is disabled on mobile/.test(src), | ||
| 'TerminalPanel.js mobile READ-ONLY banner was removed — LAYT-03 must leave the mobile banner above the padded wrapper intact.', | ||
| 'TerminalPanel.js must NOT render the legacy mobile READ-ONLY banner; mobile input is enabled and only the server --read-only flag disables input now.', | ||
| ).toBe(false); |
There was a problem hiding this comment.
The spec docstring still states "Keep the mobile READ-ONLY banner above the wrapper untouched", but the updated structural test below now requires that banner to be absent (mobile input enabled). Please update the docstring to reflect the new intended behavior so the test documentation matches the assertions.
| // PERF-E: a single controller.abort() detaches every event listener | ||
| // registered inside the main useEffect (9 total: 4 touch, 1 window | ||
| // resize, 1 anonymous mobile touchstart, 4 ws). | ||
| // registered inside the main useEffect (8 total: 4 touch, 1 window | ||
| // resize, 4 ws). |
There was a problem hiding this comment.
This comment says there are 8 total event listeners in the main useEffect (4 touch + 1 window resize + 4 ws), but the effect also registers controller.signal.addEventListener('abort', ...) for the preload cleanup. Either update the count/description, or clarify that the count is only for DOM/WebSocket listeners that require { signal } teardown enforcement.
| test('structural: contains controller.signal at least 8 times (one per addEventListener site)', () => { | ||
| const src = source(); | ||
| const matches = src.match(/controller\.signal/g) || []; | ||
| expect( | ||
| matches.length, | ||
| `Expected controller.signal to appear on every addEventListener site (>=9), found ${matches.length}. Sites: 4 touch on container + 1 window resize + 1 anonymous touchstart + 4 ws.`, | ||
| ).toBeGreaterThanOrEqual(9); | ||
| `Expected controller.signal to appear on every addEventListener site (>=8), found ${matches.length}. Sites: 4 touch on container + 1 window resize + 4 ws. (Mobile-only touchstart preventDefault was removed when mobile input was enabled.)`, | ||
| ).toBeGreaterThanOrEqual(8); |
There was a problem hiding this comment.
The controller.signal count assertion is internally inconsistent: the message enumerates 4 touch + 1 window resize + 4 ws = 9 addEventListener sites, but the test only requires >= 8. With the mobile-only touchstart removed, the minimum should still be 9 (or 10 if you also count the controller.signal.addEventListener('abort', ...) handler). As written, this check could pass even if one listener site regresses and drops controller.signal.
asheshgoplani
left a comment
asheshgoplani
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up, @JMBattista — unifying input gating through payload.readOnly is the right model, and the structural specs are a nice touch.
Two items before we can merge:
-
Performance Budget CI is failing. (visual-regression we treat as known-flaky, but Performance Budget is a separate signal.) Could you take a look and either fix or explain why it's unrelated to this diff?
-
Behavior-change release note. Before this PR, mobile users on a non-readonly server were implicitly read-only because of the client-side
!mobileguard. After this PR, they can send input. That's the intended behavior, but operators who relied on that implicit default need a heads-up. Please add a one-line note to the PR body / CHANGELOG: something like "Mobile clients no longer enforce implicit read-only; use server-side--read-onlyto preserve the old behavior." -
(Minor, optional) Refresh any visual-regression snapshots that the banner removal changed.
Once #1 is green and the release-note is in, happy to merge. Nice cleanup.
|
Hey @JMBattista — thanks for this, it's a clean diff and the Before merging, we're escalating one product question with the maintainer: agent-deck's conductor pattern in Your PR is a reasonable change if web-terminal-from-mobile is a direction we want to support. We've pinged the maintainer on that question and will update this PR once we have a direction. Two notes regardless of the outcome:
No action needed from you right now — this is just a status update so you know the PR isn't abandoned. |
Performance Budget — unrelated to this diff (gate has never passed)The Lighthouse CI workflow only triggers on
All 5 runs, from 4 different authors, touching different files, land within ~400 bytes of each other and all fail. The gate has never produced a ✅ since it was introduced. The budget is Impact of this diff on bundle weight
Net effect: the bundle is ~0.2% smaller after this PR than before. Suggested path forwardThe workflow header already documents the recalibration path: Either:
Either belongs in its own PR. Happy to open a follow-up to recalibrate from |
Operators who relied on the implicit client-side mobile read-only default need to switch to `agent-deck web --read-only` to preserve that behavior. Flag this under an Unreleased / Changed entry so release notes pick it up.
Summary
This PR enables keyboard input on mobile devices by removing mobile-specific input restrictions. Previously, the terminal was set to read-only on mobile with a banner notification. Now mobile users can interact with the terminal the same way desktop users do, with input gating controlled solely by the server's
--read-onlyflag.Key Changes
isMobilevariable and all mobile-specific input blocking logicterminal.onData()handler from being conditionally registered only on desktop to always being registered, allowing mobile users to send keystrokes|| mobilecondition fromdisableStdinassignment, so input is now gated only bypayload.readOnlyfrom the servertouchstartpreventDefault handler that was blocking the mobile soft keyboardImplementation Details
readOnlySignalnow serves as the single source of truth for input enablement across all device typesterminal.onData()callback still validates connection state and read-only status before forwarding input to the WebSocketAbortControllerwas already in place; this change reduces the listener count from 9 to 8 (removes the mobile-only anonymous touchstart listener)https://claude.ai/code/session_013aNLQniGMa4VETHTzN7eyn