-
Notifications
You must be signed in to change notification settings - Fork 240
Enable terminal input on mobile devices #652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,14 +77,34 @@ test.describe('BUG #6 / LAYT-03 — terminal panel has 16px padding on all edges | |
| ).toBe(true); | ||
| }); | ||
|
|
||
| test('structural: mobile READ-ONLY banner is untouched', () => { | ||
| 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); | ||
|
Comment on lines
+80
to
+85
|
||
| }); | ||
|
|
||
| test('structural: terminal.onData is not gated on !mobile', () => { | ||
| const src = readTerminalPanelSrc(); | ||
| expect( | ||
| /if \(!mobile\)\s*\{\s*inputDisposable\s*=\s*terminal\.onData/.test(src), | ||
| 'TerminalPanel.js must not gate terminal.onData behind !mobile; mobile input is enabled.', | ||
| ).toBe(false); | ||
| expect( | ||
| /terminal\.onData\s*\(/.test(src), | ||
| 'TerminalPanel.js must retain an unconditional terminal.onData(...) call to forward keystrokes to the tmux bridge.', | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| test('structural: disableStdin is not OR-ed with mobile on status messages', () => { | ||
| const src = readTerminalPanelSrc(); | ||
| expect( | ||
| /disableStdin\s*=\s*!!payload\.readOnly\s*\|\|\s*mobile/.test(src), | ||
| 'TerminalPanel.js must not OR mobile into disableStdin; only payload.readOnly should disable input.', | ||
| ).toBe(false); | ||
| }); | ||
|
|
||
| // RUNTIME — skips without a fixture session; measures real computed styles. | ||
| test('runtime: xterm wrapper has 16px padding on all four edges', async ({ page }) => { | ||
| await page.setViewportSize({ width: 1280, height: 800 }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,14 +6,15 @@ import { join } from 'path'; | |
| * Phase 8 / Plan 02 / Task 1: PERF-E regression test (TerminalPanel listener leak). | ||
| * | ||
| * CURRENT STATE (verified by grep of TerminalPanel.js as of 2026-04-09): | ||
| * - 9 addEventListener sites inside the main useEffect: | ||
| * - 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. | ||
|
Comment on lines
+9
to
+17
|
||
| * | ||
| * FIX TO ENFORCE (PERF-E): | ||
| * - ONE new AbortController() declared at the top of the useEffect. | ||
|
|
@@ -53,13 +54,13 @@ test.describe('PERF-E -- TerminalPanel listener cleanup via AbortController', () | |
| ).toBe(true); | ||
| }); | ||
|
|
||
| test('structural: contains controller.signal at least 9 times (one per addEventListener site)', () => { | ||
| 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); | ||
|
Comment on lines
+57
to
+63
|
||
| }); | ||
|
|
||
| test('structural: contains controller.abort() in the effect cleanup', () => { | ||
|
|
@@ -91,7 +92,7 @@ test.describe('PERF-E -- TerminalPanel listener cleanup via AbortController', () | |
| ).toEqual([]); | ||
| }); | ||
|
|
||
| test('structural: no manual removeEventListener for the 9 migrated listeners', () => { | ||
| test('structural: no manual removeEventListener for the 8 migrated listeners', () => { | ||
| const src = source(); | ||
| // The AbortController pattern replaces manual removeEventListener. | ||
| // If any removeEventListener for touch* events remains, the cleanup | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.