feat: phase 1 — one terminal in the browser#2
Merged
Conversation
Full terminal stack: Axum spawns a PTY (portable-pty), pipes output through a broadcast channel to WebSocket clients. Browser renders via ghostty-web through a thin JS bridge (ghostty-bridge.js) with 1:1 wasm-bindgen bindings. - server: pty.rs (spawn/read/write/resize via channels), ws.rs (bidirectional WS↔PTY pipe with scrollback replay), state.rs - client: ghostty-bridge.js, terminal.rs (FFI bindings), terminal_view.rs (Leptos component with WS status signal) - common: WsClientMessage/WsServerMessage enums, DEFAULT_COLS/ROWS - nix: fetchurl ghostty-web tarball, inject into buildTrunkPackage - UI: compact header with branding + WS status indicator - e2e: smoke (branding + canvas), terminal input, resize survival - justfile: test-dev recipe for fast testing against running dev server
- Add ResizeObserver so terminal reflows when browser viewport changes - Extract send_resize() helper to deduplicate 3x WS resize pattern - Create e2e test DSL (tests/e2e/dsl/) inspired by openspatial: scenario() entry point, TerminalView/AppView typed abstractions - Rewrite smoke + terminal tests using the DSL - Add border around terminal container - Inline ghosttyWebTgz into single nix derivation
nix build compiles Rust + WASM from scratch, which can exceed the previous 120s timeout. Bump to 600s.
srid
commented
Mar 19, 2026
srid
commented
Mar 19, 2026
srid
commented
Mar 19, 2026
srid
commented
Mar 19, 2026
srid
commented
Mar 19, 2026
srid
commented
Mar 19, 2026
srid
commented
Mar 19, 2026
Address PR review: split terminal_view.rs monolith into focused modules, extract named functions from closure nests, add doc comments throughout. Update code-review skill with learnings on module structure and readability.
Replace manual Closure::wrap + forget with leptos-use hooks: - use_websocket_with_options for WS connection - use_resize_observer for container resize - use_event_listener_with_options for font zoom keydown - on_cleanup for terminal dispose on unmount Extract bridge.rs for non-Leptos JS helpers (wait_animation_frame, extract_size, localStorage, build_ws_url). Simplify ws.rs to just WsStatus type with From<ConnectionReadyState>.
- measureCells() now uses actual grid dimensions instead of hardcoded 80x24, so cell size is correct after font size changes - stop_propagation() on zoom shortcuts prevents =/-/+ leaking to PTY - Add e2e tests: canvas fills container, fills after zoom, no keystroke leaks
ws_open() is non-blocking — the resize message sent immediately after was silently dropped. Now watches ready_state signal and sends resize when WS transitions to Open. Adds e2e test verifying cols > 80.
- bridge::extract_size now returns Option<(u16, u16)> instead of hardcoding defaults from kolu_common - Collapse nested if in ws.rs (clippy::collapsible_if) - Update CLAUDE.md with clippy and pre-commit quality gates
This was referenced Mar 27, 2026
Closed
This was referenced Apr 8, 2026
This was referenced Apr 14, 2026
srid
added a commit
that referenced
this pull request
Apr 17, 2026
**The Debug → Diagnostic info dialog now surfaces the memory and WebGL axes that were invisible before** — JS heap (used/total/limit), DOM node count, canvas count, per-terminal scrollback depth, and, for the focused WebGL tile, the char-atlas canvas dimensions. Copy-JSON becomes a forensic-grade payload a user can paste into a bug report and pair with Chrome's Task Manager numbers to see the whole picture in one glance. Motivated by **issue #591** (WebGL _zombie-context_ leak — kolu tabs growing from 300 MB to ~2 GB over hours). Live monitoring showed the kolu PWA sitting at 622 MB total footprint with **500 MB of GPU memory** but only 55 MB of JS heap — _six times_ the next-highest tab's GPU use, matching a ~16 × ~30 MB zombie-context ceiling almost exactly. The JS side told us nothing; the leak lives outside the heap. This dialog now shows enough to confirm that gap without reaching for DevTools. A small structural side note: the per-terminal `webglAtlas` probe is volatile (it flips on/off as focus moves between tiles), while `TerminalRefs` so far only held permanent-lifetime handles (`xterm`, `serialize`). The PR namespaces the volatile accessors under a new `probes: { webglAtlas }` sub-object so the _stability contrast stays visible in the type_ and the next diagnostic probe doesn't quietly land next to `xterm` as a grab-bag field. While touching the area, I also hoisted the WebGL2-support probe to module scope — the old code created a throwaway WebGL2 context on every dialog mount, which is itself a tiny zombie-context leak. _The exact pattern #591 exists to diagnose, fixed in passing._ > This PR is diagnostic tooling, not the fix. The prevailing hypothesis (from the #591 investigation) is that `Terminal.tsx`'s `unloadWebgl()` needs to call `w.dispose()` _before_ `loseContext()` — xterm's `webglcontextlost` listener `preventDefault()`s while the addon is still registered, which likely keeps contexts alive past their disposal. That fix is out of scope here. > **Deferred:** Centralized GPU/WebGL introspection surface (future `MAX_TEXTURE_SIZE`, `WEBGL_debug_renderer_info`, etc.) — today's `webglAtlas` is the first probe; probe #2 should get a dedicated home rather than accreting onto `TerminalProbes`. See #591. ### Try it locally ```sh nix run github:juspay/kolu/feat/diagnostic-memory-fields ```
srid
added a commit
that referenced
this pull request
Apr 18, 2026
Per Lowy review: lookup-fn props are unenforced coupling — every consumer must be hand-wired identically, and the indirection looks like encapsulation but isn't. - useTerminalStore: cache + createRoot singleton, every consumer reads the same store - useThemeManager: drop deps argument; pulls active id / metadata / setTheme from the store + RPC client directly. Cached + createRoot. - New canvas/useTileTheme.ts: singleton accessor mapping id → TileTheme, centralising the (xterm ITheme → bg/fg with fallbacks) adapter that used to be inline in App.tsx as `getChromeTileTheme`. - ChromeBar / PillTree / TerminalCanvas / CanvasMinimap / MobileChromeSheet / MobileTileView: drop the lookup-fn props, read from the singletons. Their prop surface shrinks substantially. - App.tsx: stops threading store accessors through layout components. Hickey #2: while we were in TerminalCanvas, fold the duplicated CanvasTile prop-spell-out (tiled vs maximized) into a single renderTile(id, maximized) helper.
This was referenced Apr 19, 2026
srid
added a commit
that referenced
this pull request
Apr 30, 2026
…n autosave clobber (#796) **Three independent macOS CI failures, each with a distinct root cause.** Locally each one repros at 10–25% under parallel load on darwin; on x86_64-linux they're either masked by faster timing or genuinely don't apply. Together they account for every red step the e2e + unit suites were posting on darwin. | # | Symptom | Root cause | Fix | | --- | --- | --- | --- | | 1 | `Close terminal on worktree …` times out clicking the Remove button | `closeTerminal()` snapshots `meta.git?.isWorktree` once at dialog-open; the test only waited for CWD, not the async git-metadata stream | Wait for the worktree indicator before initiating close | | 2 | 9 `watchGitHead` / `subscribeGitInfo` unit tests assert `_sharedHeadWatcherCount() = 0/1/2`, get 2/4/7… | `git rev-parse --git-dir` returns relative `.git` from the repo root but a realpath-resolved absolute path from a subdir; on macOS `/tmp` ↔ `/private/tmp` collides the registry | Pipe the resolved path through `fs.realpathSync` | | 3 | `session-restore` scenarios — restore card flashes visible then disappears mid-scenario | Pending autosave timer scheduled by a stale `terminals:dirty` event fires 500ms later, runs `saveSession([])` and writes `null`, clobbering the test-set session | `setSavedSession` cancels the pending autosave timer before writing | ### #2 in detail The shared-watcher registry is keyed by the resolved gitDir string. When two callers reach the same repo via different paths — one from the root, one from a subdir — `git rev-parse --git-dir` reports them differently: ``` cwd=/tmp/repo → "git rev-parse --git-dir" → ".git" → /tmp/repo/.git cwd=/tmp/repo/src/deep → "git rev-parse --git-dir" → "/private/... → /private/tmp/repo/.git tmp/repo/.git" ``` Different keys → two `fs.watch` handles for one `.git`. `fs.realpathSync` collapses both to `/private/tmp/repo/.git` and the singleton invariant holds. ### #3 in detail The Before hook drains terminals and posts `session: null`; the Given step posts the test session; the page loads. _Between those_, a lingering provider event from a drained terminal in the previous scenario fires `terminals:dirty`, the leading-edge throttle (#767) schedules a save 500ms out, and the callback runs with an empty terminal snapshot. `saveSession([])` rewrites the session to `null`, the client's `session.get` subscription pushes `null`, and the `<Show when={savedSession}>` unmounts the restore card the test was about to assert on. > _The fix is local to `setSavedSession`. Production callers (test-only path today) get a free upgrade: an explicit set always wins over a pending autosave._ ### Verification - `CUCUMBER_PARALLEL=1 just test-quick features/worktree.feature` — 5/5 (was 5/5; fix is for a parallel-only race in CI) - `just test-quick features/session-restore.feature` ×5 with parallel=4 — 5/5 (was 1–2/5 failing on master and on this branch before the autosave fix) - `pnpm -r test:unit` — all suites green, including the 9 previously-failing `watchGitHead` / `subscribeGitInfo` cases _Generated by [`/do`](https://github.com/srid/agency) on Claude Code (model \`claude-opus-4-7\`)._
srid
added a commit
that referenced
this pull request
May 1, 2026
… runtime Both Lowy F2 and Hickey #2 flagged Cell.name as dual-use (contract key AND publisher channel). Re-checking the actual code: cell.name is used only for type identity and one error string in collectionHandlers. Channel names are explicit strings in cellBus ("preferences:changed", "terminal-list"); contract dispatch goes through procedure refs the caller passes to hooks. The dual-use risk was theoretical, never actual. Update the doc to say so — name is a descriptor identifier, not a runtime-load-bearing dispatch key.
srid
added a commit
that referenced
this pull request
May 6, 2026
…a renderer factory Hickey #1: the virtualization-mode predicate was decided at the constructor site (vanilla vs. virtualized class) and re-checked at the render site (containerWrapper vs. fileContainer), with nothing enforcing that the two branches stay in sync. Extract the pairing into a `createDiffRenderer` / `createFileRenderer` factory so a future render-option addition can't be applied to one arm and missed on the other. Also addresses Hickey #2 (collapsed ternary) and Lowy #3 (the branch point is now named and commented inside the factory).
srid
added a commit
that referenced
this pull request
May 6, 2026
Lowy #2: virtualization on/off is decided ambiently by whether <Virtualizer> sits in the JSX tree, but the contract that the root must scroll is implicit — a caller who wraps in <Virtualizer> without `overflow-auto` activates the virtualized Pierre class but gets full-DOM rendering anyway, because the IntersectionObserver root computes against the layout box and sees everything as visible. Catch the misconfig with a dev-mode console warning at the boundary where the contract lives, not in informal documentation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.