Compare commits

..

4 Commits

Author SHA1 Message Date
Developer bb38b37ceb fix: reinitialize terminal on mobile viewport changes
Recreate the terminal when responsive classification changes so mobile scrollback and touch listeners are installed.\n\nOpenSpec: fix-mobile-terminal-scrolling\nQuality gates: npm run typecheck, npm run lint, npm test (89 passed), npm run build
2026-07-14 19:41:34 +00:00
Developer 6698c20f25 fix: restore mobile terminal scrolling
- Retain xterm normal-buffer history on mobile while preserving desktop zero-scrollback behavior\n- Repair ProjectsPage tests for session context and current project list markup\n- Add focused terminal scrollback coverage\n\nOpenSpec: fix-mobile-terminal-scrolling\nQuality gates: npm run typecheck, npm run lint, npm test (89 passed), npm run build
2026-07-14 19:07:02 +00:00
Developer 5e06a2a226 Merge fix/terminal-paste-partial-writes into dev 2026-07-14 10:34:33 +00:00
Developer 49180e4c6d fix: fully drain terminal paste writes to PTY
- Make the host PTY master non-blocking and wait for write readiness.
- Retry partial writes so a large bracketed paste always delivers its closing
  marker instead of leaving pi in paste mode.
- Add regression coverage for partial PTY writes and resolve diagnostics.

Quality gates: ruff, mypy, focused pytest (3 passed)
2026-07-14 10:34:33 +00:00
7 changed files with 183 additions and 68 deletions
@@ -19,6 +19,8 @@ from typing import Any
logger = logging.getLogger(__name__) logger = logging.getLogger(__name__)
_session_counters_by_instance: dict[str, int] = {}
class TerminalSession: class TerminalSession:
"""Manages a single terminal session with event-driven PTY I/O. """Manages a single terminal session with event-driven PTY I/O.
@@ -46,9 +48,6 @@ class TerminalSession:
# Max WebSocket frame size # Max WebSocket frame size
MAX_FRAME_SIZE = 64 * 1024 MAX_FRAME_SIZE = 64 * 1024
# Session number counter per instance_id for auto-naming
_instance_counters: dict[str, int] = {}
def __init__( def __init__(
self, self,
session_id: str, session_id: str,
@@ -99,11 +98,11 @@ class TerminalSession:
# Ack timeout fallback # Ack timeout fallback
self._ack_timeout_handle: asyncio.TimerHandle | None = None self._ack_timeout_handle: asyncio.TimerHandle | None = None
@classmethod @staticmethod
def _generate_name(cls, instance_id: str) -> str: def _generate_name(instance_id: str) -> str:
"""Generate an auto-incremented session name for the instance.""" """Generate an auto-incremented session name for the instance."""
count = cls._instance_counters.get(instance_id, 0) + 1 count = _session_counters_by_instance.get(instance_id, 0) + 1
cls._instance_counters[instance_id] = count _session_counters_by_instance[instance_id] = count
return f"Session {count}" return f"Session {count}"
async def start(self, startup_command: str | None = None) -> None: async def start(self, startup_command: str | None = None) -> None:
@@ -121,8 +120,12 @@ class TerminalSession:
if self.container_user: if self.container_user:
exec_cmd.extend(["--user", self.container_user]) exec_cmd.extend(["--user", self.container_user])
# Create a pseudo-terminal on the host # Create a pseudo-terminal on the host. The master must be
# non-blocking: a browser paste can be larger than the PTY input
# buffer, and write_input() drains it asynchronously without dropping
# the closing bracketed-paste marker.
self._master_fd, slave_fd = pty.openpty() self._master_fd, slave_fd = pty.openpty()
os.set_blocking(self._master_fd, False)
# Put the host PTY into raw mode so it behaves as a pass-through # Put the host PTY into raw mode so it behaves as a pass-through
# pipe. openpty() leaves the slave in canonical mode by default, # pipe. openpty() leaves the slave in canonical mode by default,
@@ -204,6 +207,9 @@ class TerminalSession:
try: try:
data = os.read(self._master_fd, 4096) data = os.read(self._master_fd, 4096)
except BlockingIOError:
# The readiness notification raced with another callback.
return
except (OSError, IOError) as exc: except (OSError, IOError) as exc:
logger.debug("PTY read error for session %s: %s", self.session_id, exc) logger.debug("PTY read error for session %s: %s", self.session_id, exc)
self._handle_eof() self._handle_eof()
@@ -343,12 +349,42 @@ class TerminalSession:
pass pass
logger.info("Session %s EOF handled, websockets closed", self.session_id) logger.info("Session %s EOF handled, websockets closed", self.session_id)
async def _wait_for_write_ready(self, fd: int) -> None:
"""Wait until a non-blocking PTY master can accept more input."""
loop = asyncio.get_running_loop()
writable = loop.create_future()
def mark_writable() -> None:
if not writable.done():
writable.set_result(None)
loop.add_writer(fd, mark_writable)
try:
await writable
finally:
loop.remove_writer(fd)
async def write_input(self, data: bytes) -> None: async def write_input(self, data: bytes) -> None:
"""Write input to the PTY master.""" """Write all terminal input bytes to the PTY master in order."""
if self._master_fd is None or self._closed: if self._master_fd is None or self._closed:
return return
fd = self._master_fd
remaining = memoryview(data)
try: try:
os.write(self._master_fd, data) while remaining and not self._closed and self._master_fd == fd:
try:
written = os.write(fd, remaining)
except BlockingIOError:
await self._wait_for_write_ready(fd)
continue
if written == 0:
await self._wait_for_write_ready(fd)
continue
remaining = remaining[written:]
self.last_activity = time.time() self.last_activity = time.time()
except (OSError, IOError) as exc: except (OSError, IOError) as exc:
logger.debug("PTY write error for session %s: %s", self.session_id, exc) logger.debug("PTY write error for session %s: %s", self.session_id, exc)
+53 -22
View File
@@ -1,9 +1,9 @@
"""Unit tests for TerminalSession docker exec invocation.""" """Unit tests for TerminalSession PTY handling."""
import uuid import uuid
from unittest.mock import AsyncMock, patch from unittest.mock import AsyncMock, patch
import pytest import pytest # pyright: ignore[reportMissingImports]
from src.services.terminal.terminal_session import TerminalSession from src.services.terminal.terminal_session import TerminalSession
@@ -19,17 +19,20 @@ async def test_start_passes_container_user_to_docker_exec() -> None:
container_user="dev", container_user="dev",
) )
with patch( with (
"src.services.terminal.terminal_session.pty.openpty", patch(
return_value=(1, 2), "src.services.terminal.terminal_session.pty.openpty",
return_value=(1, 2),
),
patch("src.services.terminal.terminal_session.os.set_blocking") as set_blocking,
patch("src.services.terminal.terminal_session.tty.setraw"),
patch(
"src.services.terminal.terminal_session.asyncio.create_subprocess_exec",
new=AsyncMock(),
) as mock_exec,
patch("src.services.terminal.terminal_session.os.close"),
): ):
with patch("src.services.terminal.terminal_session.tty.setraw"): await session.start()
with patch(
"src.services.terminal.terminal_session.asyncio.create_subprocess_exec",
new=AsyncMock(),
) as mock_exec:
with patch("src.services.terminal.terminal_session.os.close"):
await session.start()
args, _kwargs = mock_exec.call_args args, _kwargs = mock_exec.call_args
assert "docker" in args assert "docker" in args
@@ -38,6 +41,7 @@ async def test_start_passes_container_user_to_docker_exec() -> None:
user_index = args.index("--user") user_index = args.index("--user")
assert args[user_index + 1] == "dev" assert args[user_index + 1] == "dev"
assert "container-123" in args assert "container-123" in args
set_blocking.assert_called_once_with(1, False)
@pytest.mark.unit @pytest.mark.unit
@@ -50,17 +54,44 @@ async def test_start_omits_user_when_not_configured() -> None:
container_id="container-123", container_id="container-123",
) )
with patch( with (
"src.services.terminal.terminal_session.pty.openpty", patch(
return_value=(1, 2), "src.services.terminal.terminal_session.pty.openpty",
return_value=(1, 2),
),
patch("src.services.terminal.terminal_session.os.set_blocking"),
patch("src.services.terminal.terminal_session.tty.setraw"),
patch(
"src.services.terminal.terminal_session.asyncio.create_subprocess_exec",
new=AsyncMock(),
) as mock_exec,
patch("src.services.terminal.terminal_session.os.close"),
): ):
with patch("src.services.terminal.terminal_session.tty.setraw"): await session.start()
with patch(
"src.services.terminal.terminal_session.asyncio.create_subprocess_exec",
new=AsyncMock(),
) as mock_exec:
with patch("src.services.terminal.terminal_session.os.close"):
await session.start()
args, _kwargs = mock_exec.call_args args, _kwargs = mock_exec.call_args
assert "--user" not in args assert "--user" not in args
@pytest.mark.unit
@pytest.mark.asyncio
async def test_write_input_retries_partial_pty_writes() -> None:
"""A large paste is fully written even when the PTY accepts it in chunks."""
session = TerminalSession(
session_id=str(uuid.uuid4()),
instance_id=uuid.uuid4(),
container_id="container-123",
)
session._master_fd = 42
with patch(
"src.services.terminal.terminal_session.os.write",
side_effect=[2, 2, 1],
) as write:
await session.write_input(b"hello")
assert [bytes(call.args[1]) for call in write.call_args_list] == [
b"hello",
b"llo",
b"o",
]
@@ -0,0 +1,13 @@
import { describe, expect, it } from "vitest";
import { getTerminalScrollbackLimit } from "./terminal.tsx";
describe("getTerminalScrollbackLimit", () => {
it("retains normal-buffer history for custom mobile swipe scrolling", () => {
expect(getTerminalScrollbackLimit(true)).toBe(10_000);
});
it("keeps desktop scrollback disabled to prevent stale-frame wheel scrolling", () => {
expect(getTerminalScrollbackLimit(false)).toBe(0);
});
});
@@ -53,6 +53,10 @@ const BRACKETED_PASTE_DISABLE_SEQUENCE = [0x1b, 0x5b, 0x3f, 0x32, 0x30, 0x30, 0x
const BRACKETED_PASTE_CONTROL_TAIL_LENGTH = const BRACKETED_PASTE_CONTROL_TAIL_LENGTH =
BRACKETED_PASTE_ENABLE_SEQUENCE.length - 1; BRACKETED_PASTE_ENABLE_SEQUENCE.length - 1;
export function getTerminalScrollbackLimit(isMobile: boolean): number {
return isMobile ? 10_000 : 0;
}
function matchesByteSequence( function matchesByteSequence(
data: Uint8Array, data: Uint8Array,
start: number, start: number,
@@ -326,14 +330,11 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
lineHeight: 1.2, lineHeight: 1.2,
letterSpacing: 0, letterSpacing: 0,
allowTransparency: false, allowTransparency: false,
// This terminal only ever hosts full-screen TUI tools (pi-agent, // Desktop tools repaint in place, so retaining their normal buffer
// opencode), which repaint in place in the normal buffer and do not // creates stale frames that native wheel scrolling can revisit. Mobile
// use the alternate screen or mouse tracking. With scrollback, every // instead uses its custom touch handler to scroll normal-buffer output,
// repaint accumulates as history → a viewport scrollbar appears and // which requires retained history.
// the mouse-wheel scrolls through stale frames instead of the app. scrollback: getTerminalScrollbackLimit(isMobile),
// scrollback:0 keeps only the live viewport: no bar, no stale-frame
// wheel jank. (Scrollbar is also hidden via CSS for belt-and-suspenders.)
scrollback: 0,
ignoreBracketedPasteMode: false, ignoreBracketedPasteMode: false,
fastScrollSensitivity: 0, fastScrollSensitivity: 0,
scrollSensitivity: 0, scrollSensitivity: 0,
@@ -699,7 +700,7 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
// Ignore disposal errors from partially torn-down terminal // Ignore disposal errors from partially torn-down terminal
} }
}; };
}, [instanceId, connectWebSocket]); }, [instanceId, connectWebSocket, isMobile]);
useImperativeHandle(ref, () => ({ useImperativeHandle(ref, () => ({
fit: () => { fit: () => {
+22 -27
View File
@@ -1,17 +1,26 @@
import "@testing-library/jest-dom/vitest";
import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react";
import { MemoryRouter } from "react-router-dom"; import { MemoryRouter } from "react-router-dom";
import { afterEach, describe, expect, it, vi } from "vitest"; import { afterEach, describe, expect, it, vi } from "vitest";
import { ProjectsPage } from "./ProjectsPage"; import { ProjectsPage } from "./ProjectsPage";
import * as projectsApi from "../api/projects"; import * as projectsApi from "../api/projects";
import { SessionsProvider } from "../state/sessions";
import type { ProjectWithRepos } from "../types";
const mockProjects = [ vi.mock("../api/sessions", () => ({
getUserSessions: vi.fn().mockResolvedValue([]),
}));
const mockProjects: ProjectWithRepos[] = [
{ {
id: "proj-1", id: "proj-1",
name: "Alpha Project", name: "Alpha Project",
description: "First project", description: "First project",
owner_id: "user-1", owner_id: "user-1",
default_ssh_key_id: null, default_ssh_key_id: null,
repositories: [],
created_at: "2026-07-01T00:00:00Z",
}, },
{ {
id: "proj-2", id: "proj-2",
@@ -19,6 +28,8 @@ const mockProjects = [
description: null, description: null,
owner_id: "user-1", owner_id: "user-1",
default_ssh_key_id: null, default_ssh_key_id: null,
repositories: [],
created_at: "2026-07-01T00:00:00Z",
}, },
]; ];
@@ -31,9 +42,7 @@ describe("ProjectsPage", () => {
it("renders loading state initially", () => { it("renders loading state initially", () => {
vi.spyOn(projectsApi, "listProjects").mockImplementation(() => new Promise(() => {})); vi.spyOn(projectsApi, "listProjects").mockImplementation(() => new Promise(() => {}));
render( render(
<MemoryRouter> <MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
<ProjectsPage />
</MemoryRouter>
); );
expect(screen.getByText(/loading projects/i)).toBeInTheDocument(); expect(screen.getByText(/loading projects/i)).toBeInTheDocument();
}); });
@@ -41,9 +50,7 @@ describe("ProjectsPage", () => {
it("renders project list after loading", async () => { it("renders project list after loading", async () => {
vi.spyOn(projectsApi, "listProjects").mockResolvedValue(mockProjects); vi.spyOn(projectsApi, "listProjects").mockResolvedValue(mockProjects);
render( render(
<MemoryRouter> <MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
<ProjectsPage />
</MemoryRouter>
); );
await waitFor(() => { await waitFor(() => {
@@ -56,9 +63,7 @@ describe("ProjectsPage", () => {
it("renders empty state when no projects", async () => { it("renders empty state when no projects", async () => {
vi.spyOn(projectsApi, "listProjects").mockResolvedValue([]); vi.spyOn(projectsApi, "listProjects").mockResolvedValue([]);
render( render(
<MemoryRouter> <MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
<ProjectsPage />
</MemoryRouter>
); );
await waitFor(() => { await waitFor(() => {
@@ -69,9 +74,7 @@ describe("ProjectsPage", () => {
it("renders error state with retry button", async () => { it("renders error state with retry button", async () => {
vi.spyOn(projectsApi, "listProjects").mockRejectedValue(new Error("fail")); vi.spyOn(projectsApi, "listProjects").mockRejectedValue(new Error("fail"));
render( render(
<MemoryRouter> <MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
<ProjectsPage />
</MemoryRouter>
); );
await waitFor(() => { await waitFor(() => {
@@ -85,9 +88,7 @@ describe("ProjectsPage", () => {
const createMock = vi.spyOn(projectsApi, "createProject").mockResolvedValue(mockProjects[0]); const createMock = vi.spyOn(projectsApi, "createProject").mockResolvedValue(mockProjects[0]);
render( render(
<MemoryRouter> <MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
<ProjectsPage />
</MemoryRouter>
); );
await waitFor(() => { await waitFor(() => {
@@ -118,9 +119,7 @@ describe("ProjectsPage", () => {
vi.spyOn(projectsApi, "listProjects").mockResolvedValue([]); vi.spyOn(projectsApi, "listProjects").mockResolvedValue([]);
render( render(
<MemoryRouter> <MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
<ProjectsPage />
</MemoryRouter>
); );
await waitFor(() => { await waitFor(() => {
@@ -138,16 +137,14 @@ describe("ProjectsPage", () => {
const updateMock = vi.spyOn(projectsApi, "updateProject").mockResolvedValue(mockProjects[0]); const updateMock = vi.spyOn(projectsApi, "updateProject").mockResolvedValue(mockProjects[0]);
render( render(
<MemoryRouter> <MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
<ProjectsPage />
</MemoryRouter>
); );
await waitFor(() => { await waitFor(() => {
expect(screen.getByText("Alpha Project")).toBeInTheDocument(); expect(screen.getByText("Alpha Project")).toBeInTheDocument();
}); });
const alphaCard = screen.getByText("Alpha Project").closest(".project-card") as HTMLElement | null; const alphaCard = screen.getByText("Alpha Project").closest(".project-list-item") as HTMLElement | null;
if (!alphaCard) throw new Error("Card not found"); if (!alphaCard) throw new Error("Card not found");
fireEvent.click(within(alphaCard).getByRole("button", { name: /edit/i })); fireEvent.click(within(alphaCard).getByRole("button", { name: /edit/i }));
@@ -171,16 +168,14 @@ describe("ProjectsPage", () => {
const deleteMock = vi.spyOn(projectsApi, "deleteProject").mockResolvedValue(undefined); const deleteMock = vi.spyOn(projectsApi, "deleteProject").mockResolvedValue(undefined);
render( render(
<MemoryRouter> <MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
<ProjectsPage />
</MemoryRouter>
); );
await waitFor(() => { await waitFor(() => {
expect(screen.getByText("Alpha Project")).toBeInTheDocument(); expect(screen.getByText("Alpha Project")).toBeInTheDocument();
}); });
const alphaCard = screen.getByText("Alpha Project").closest(".project-card") as HTMLElement | null; const alphaCard = screen.getByText("Alpha Project").closest(".project-list-item") as HTMLElement | null;
if (!alphaCard) throw new Error("Card not found"); if (!alphaCard) throw new Error("Card not found");
fireEvent.click(within(alphaCard).getByRole("button", { name: /delete/i })); fireEvent.click(within(alphaCard).getByRole("button", { name: /delete/i }));
@@ -0,0 +1,30 @@
# Restore Mobile Terminal Scrolling
## Summary
The recent terminal scrollback optimization disabled scrollback for every viewport. Mobile terminal swipe handling still scrolls xterm's normal buffer programmatically, so swipes in normal-buffer tools no longer have retained output to move through.
## Root Cause
`468f342` changed the terminal configuration to `scrollback: 0` globally to prevent stale repaint frames and wheel scrolling on desktop. The mobile touch handler calls `term.scrollLines()` when the normal buffer is active. With zero scrollback, that call has no scrollable history and becomes a no-op.
## Scope
- `apps/web/src/components/features/terminal/terminal.tsx`
- Focused terminal configuration test
## Fix
Retain a bounded xterm scrollback buffer on mobile only (`10000` lines), while leaving desktop at zero scrollback and with wheel sensitivity disabled. Mobile's existing custom touch handler remains responsible for moving through normal-buffer history; alternate-screen swipes continue to send SGR wheel events to the active TUI.
## Acceptance Criteria
- [ ] A mobile terminal with normal-buffer output exceeding one screen scrolls via a vertical swipe.
- [ ] Alternate-screen terminal scrolling continues to use the existing SGR wheel-event path.
- [ ] Desktop keeps zero xterm scrollback and disabled native wheel scrolling, so stale repaint frames do not return.
- [ ] Focused unit test and frontend quality gates pass.
## Related
- `468f342 fix(terminal): hide scrollbar and stop stale-frame wheel scroll for TUI tools`
- `openspec/changes/fix-terminal-container-overflow`
@@ -0,0 +1,9 @@
# Restore Mobile Terminal Scrolling — Tasks
- [x] Add a mobile-specific terminal scrollback limit while preserving zero scrollback on desktop.
- [x] Reinitialize the terminal when the responsive mobile classification changes so its scrollback and touch handler match the active viewport.
- [x] Add focused tests for the responsive scrollback configuration.
- [x] Run the full frontend test suite (89 tests passed after repairing the `ProjectsPage` test setup).
- [x] Run frontend typecheck, lint, focused tests, and production build.
- [ ] Perform mobile normal-buffer and alternate-screen manual QA.
- [ ] Update project maps for changed source files (the map patch tool currently fails with an unsupported `temperature` parameter).