Compare commits
4 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| bb38b37ceb | |||
| 6698c20f25 | |||
| 5e06a2a226 | |||
| 49180e4c6d |
@@ -19,6 +19,8 @@ from typing import Any
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
_session_counters_by_instance: dict[str, int] = {}
|
||||
|
||||
|
||||
class TerminalSession:
|
||||
"""Manages a single terminal session with event-driven PTY I/O.
|
||||
@@ -46,9 +48,6 @@ class TerminalSession:
|
||||
# Max WebSocket frame size
|
||||
MAX_FRAME_SIZE = 64 * 1024
|
||||
|
||||
# Session number counter per instance_id for auto-naming
|
||||
_instance_counters: dict[str, int] = {}
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
session_id: str,
|
||||
@@ -99,11 +98,11 @@ class TerminalSession:
|
||||
# Ack timeout fallback
|
||||
self._ack_timeout_handle: asyncio.TimerHandle | None = None
|
||||
|
||||
@classmethod
|
||||
def _generate_name(cls, instance_id: str) -> str:
|
||||
@staticmethod
|
||||
def _generate_name(instance_id: str) -> str:
|
||||
"""Generate an auto-incremented session name for the instance."""
|
||||
count = cls._instance_counters.get(instance_id, 0) + 1
|
||||
cls._instance_counters[instance_id] = count
|
||||
count = _session_counters_by_instance.get(instance_id, 0) + 1
|
||||
_session_counters_by_instance[instance_id] = count
|
||||
return f"Session {count}"
|
||||
|
||||
async def start(self, startup_command: str | None = None) -> None:
|
||||
@@ -121,8 +120,12 @@ class TerminalSession:
|
||||
if 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()
|
||||
os.set_blocking(self._master_fd, False)
|
||||
|
||||
# Put the host PTY into raw mode so it behaves as a pass-through
|
||||
# pipe. openpty() leaves the slave in canonical mode by default,
|
||||
@@ -204,6 +207,9 @@ class TerminalSession:
|
||||
|
||||
try:
|
||||
data = os.read(self._master_fd, 4096)
|
||||
except BlockingIOError:
|
||||
# The readiness notification raced with another callback.
|
||||
return
|
||||
except (OSError, IOError) as exc:
|
||||
logger.debug("PTY read error for session %s: %s", self.session_id, exc)
|
||||
self._handle_eof()
|
||||
@@ -343,12 +349,42 @@ class TerminalSession:
|
||||
pass
|
||||
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:
|
||||
"""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:
|
||||
return
|
||||
|
||||
fd = self._master_fd
|
||||
remaining = memoryview(data)
|
||||
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()
|
||||
except (OSError, IOError) as exc:
|
||||
logger.debug("PTY write error for session %s: %s", self.session_id, exc)
|
||||
|
||||
@@ -1,9 +1,9 @@
|
||||
"""Unit tests for TerminalSession docker exec invocation."""
|
||||
"""Unit tests for TerminalSession PTY handling."""
|
||||
|
||||
import uuid
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
import pytest # pyright: ignore[reportMissingImports]
|
||||
|
||||
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",
|
||||
)
|
||||
|
||||
with patch(
|
||||
"src.services.terminal.terminal_session.pty.openpty",
|
||||
return_value=(1, 2),
|
||||
with (
|
||||
patch(
|
||||
"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"):
|
||||
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()
|
||||
await session.start()
|
||||
|
||||
args, _kwargs = mock_exec.call_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")
|
||||
assert args[user_index + 1] == "dev"
|
||||
assert "container-123" in args
|
||||
set_blocking.assert_called_once_with(1, False)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@@ -50,17 +54,44 @@ async def test_start_omits_user_when_not_configured() -> None:
|
||||
container_id="container-123",
|
||||
)
|
||||
|
||||
with patch(
|
||||
"src.services.terminal.terminal_session.pty.openpty",
|
||||
return_value=(1, 2),
|
||||
with (
|
||||
patch(
|
||||
"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"):
|
||||
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()
|
||||
await session.start()
|
||||
|
||||
args, _kwargs = mock_exec.call_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 =
|
||||
BRACKETED_PASTE_ENABLE_SEQUENCE.length - 1;
|
||||
|
||||
export function getTerminalScrollbackLimit(isMobile: boolean): number {
|
||||
return isMobile ? 10_000 : 0;
|
||||
}
|
||||
|
||||
function matchesByteSequence(
|
||||
data: Uint8Array,
|
||||
start: number,
|
||||
@@ -326,14 +330,11 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
lineHeight: 1.2,
|
||||
letterSpacing: 0,
|
||||
allowTransparency: false,
|
||||
// This terminal only ever hosts full-screen TUI tools (pi-agent,
|
||||
// opencode), which repaint in place in the normal buffer and do not
|
||||
// use the alternate screen or mouse tracking. With scrollback, every
|
||||
// repaint accumulates as history → a viewport scrollbar appears and
|
||||
// the mouse-wheel scrolls through stale frames instead of the app.
|
||||
// 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,
|
||||
// Desktop tools repaint in place, so retaining their normal buffer
|
||||
// creates stale frames that native wheel scrolling can revisit. Mobile
|
||||
// instead uses its custom touch handler to scroll normal-buffer output,
|
||||
// which requires retained history.
|
||||
scrollback: getTerminalScrollbackLimit(isMobile),
|
||||
ignoreBracketedPasteMode: false,
|
||||
fastScrollSensitivity: 0,
|
||||
scrollSensitivity: 0,
|
||||
@@ -699,7 +700,7 @@ export const TerminalComponent = React.forwardRef<TerminalRef, TerminalProps>(
|
||||
// Ignore disposal errors from partially torn-down terminal
|
||||
}
|
||||
};
|
||||
}, [instanceId, connectWebSocket]);
|
||||
}, [instanceId, connectWebSocket, isMobile]);
|
||||
|
||||
useImperativeHandle(ref, () => ({
|
||||
fit: () => {
|
||||
|
||||
@@ -1,17 +1,26 @@
|
||||
import "@testing-library/jest-dom/vitest";
|
||||
import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react";
|
||||
import { MemoryRouter } from "react-router-dom";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
import { ProjectsPage } from "./ProjectsPage";
|
||||
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",
|
||||
name: "Alpha Project",
|
||||
description: "First project",
|
||||
owner_id: "user-1",
|
||||
default_ssh_key_id: null,
|
||||
repositories: [],
|
||||
created_at: "2026-07-01T00:00:00Z",
|
||||
},
|
||||
{
|
||||
id: "proj-2",
|
||||
@@ -19,6 +28,8 @@ const mockProjects = [
|
||||
description: null,
|
||||
owner_id: "user-1",
|
||||
default_ssh_key_id: null,
|
||||
repositories: [],
|
||||
created_at: "2026-07-01T00:00:00Z",
|
||||
},
|
||||
];
|
||||
|
||||
@@ -31,9 +42,7 @@ describe("ProjectsPage", () => {
|
||||
it("renders loading state initially", () => {
|
||||
vi.spyOn(projectsApi, "listProjects").mockImplementation(() => new Promise(() => {}));
|
||||
render(
|
||||
<MemoryRouter>
|
||||
<ProjectsPage />
|
||||
</MemoryRouter>
|
||||
<MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
|
||||
);
|
||||
expect(screen.getByText(/loading projects/i)).toBeInTheDocument();
|
||||
});
|
||||
@@ -41,9 +50,7 @@ describe("ProjectsPage", () => {
|
||||
it("renders project list after loading", async () => {
|
||||
vi.spyOn(projectsApi, "listProjects").mockResolvedValue(mockProjects);
|
||||
render(
|
||||
<MemoryRouter>
|
||||
<ProjectsPage />
|
||||
</MemoryRouter>
|
||||
<MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
|
||||
);
|
||||
|
||||
await waitFor(() => {
|
||||
@@ -56,9 +63,7 @@ describe("ProjectsPage", () => {
|
||||
it("renders empty state when no projects", async () => {
|
||||
vi.spyOn(projectsApi, "listProjects").mockResolvedValue([]);
|
||||
render(
|
||||
<MemoryRouter>
|
||||
<ProjectsPage />
|
||||
</MemoryRouter>
|
||||
<MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
|
||||
);
|
||||
|
||||
await waitFor(() => {
|
||||
@@ -69,9 +74,7 @@ describe("ProjectsPage", () => {
|
||||
it("renders error state with retry button", async () => {
|
||||
vi.spyOn(projectsApi, "listProjects").mockRejectedValue(new Error("fail"));
|
||||
render(
|
||||
<MemoryRouter>
|
||||
<ProjectsPage />
|
||||
</MemoryRouter>
|
||||
<MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
|
||||
);
|
||||
|
||||
await waitFor(() => {
|
||||
@@ -85,9 +88,7 @@ describe("ProjectsPage", () => {
|
||||
const createMock = vi.spyOn(projectsApi, "createProject").mockResolvedValue(mockProjects[0]);
|
||||
|
||||
render(
|
||||
<MemoryRouter>
|
||||
<ProjectsPage />
|
||||
</MemoryRouter>
|
||||
<MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
|
||||
);
|
||||
|
||||
await waitFor(() => {
|
||||
@@ -118,9 +119,7 @@ describe("ProjectsPage", () => {
|
||||
vi.spyOn(projectsApi, "listProjects").mockResolvedValue([]);
|
||||
|
||||
render(
|
||||
<MemoryRouter>
|
||||
<ProjectsPage />
|
||||
</MemoryRouter>
|
||||
<MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
|
||||
);
|
||||
|
||||
await waitFor(() => {
|
||||
@@ -138,16 +137,14 @@ describe("ProjectsPage", () => {
|
||||
const updateMock = vi.spyOn(projectsApi, "updateProject").mockResolvedValue(mockProjects[0]);
|
||||
|
||||
render(
|
||||
<MemoryRouter>
|
||||
<ProjectsPage />
|
||||
</MemoryRouter>
|
||||
<MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
|
||||
);
|
||||
|
||||
await waitFor(() => {
|
||||
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");
|
||||
|
||||
fireEvent.click(within(alphaCard).getByRole("button", { name: /edit/i }));
|
||||
@@ -171,16 +168,14 @@ describe("ProjectsPage", () => {
|
||||
const deleteMock = vi.spyOn(projectsApi, "deleteProject").mockResolvedValue(undefined);
|
||||
|
||||
render(
|
||||
<MemoryRouter>
|
||||
<ProjectsPage />
|
||||
</MemoryRouter>
|
||||
<MemoryRouter><SessionsProvider><ProjectsPage /></SessionsProvider></MemoryRouter>
|
||||
);
|
||||
|
||||
await waitFor(() => {
|
||||
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");
|
||||
|
||||
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).
|
||||
Reference in New Issue
Block a user