feat: config hot reload, unit tests, server restart, and client UX polish - #14
Conversation
The /api/server/stop handler was calling httpServer.close() immediately, racing against WS close frame delivery. Apply the same pattern as SIGINT: let httpServer.close() drain connections with a 1s fallback timeout.
… for terminal and server options
…oved error handling
…-comments dependency
…s for server control functions
…m spawn method and improve timeout handling
…et banner with styled session information
…n banner help command
There was a problem hiding this comment.
Pull request overview
This PR introduces a new JSON-based config system with hot reload boundaries (page load + new PTY spawn), adds a webtty restart lifecycle command, and refactors WebSocket close handling while polishing the client UX (banner + unified lifecycle messages).
Changes:
- Add
src/config.ts(defaults + load/merge + first-run file creation) and wire config into server/client/PTY spawn paths for hot reload. - Add
stopServer()and arestartCLI command; refactor server shutdown/session-close handling into WebSocket helpers. - Update client terminal rendering and lifecycle messaging; expand unit tests and docs/ADRs/specs accordingly.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/websocket.ts | Adds WS close constants/helpers, loads config for PTY spawn + scrollback, and replaces the welcome banner. |
| src/server/websocket.test.ts | Updates banner assertions and strips ANSI codes for stable checks. |
| src/server/session.ts | Removes SCROLLBACK_MAX constant (scrollback now driven by config). |
| src/server/session.test.ts | Adds tests for setLastUsedId / lastUsedId. |
| src/server/routes.ts | Uses closeSession() for deletes and injects config into rendered client HTML. |
| src/server/index.ts | Loads startup config for host/port and uses closeAllSessions() on shutdown. |
| src/server/client.ts | Injects config-derived terminal options/theme and unifies WS lifecycle messages. |
| src/server/client.test.ts | Expands coverage for config injection into client HTML. |
| src/pty/node.ts | Extends PTY spawn to accept term + colorTerm from config. |
| src/pty/index.ts | Updates spawnForSession signature to accept config-driven shell/term vars. |
| src/pty/bun.ts | Extends Bun PTY spawn env with config-driven TERM/COLORTERM. |
| src/config.ts | Implements config defaults + load/merge + first-run config file creation. |
| src/config.test.ts | Adds unit tests for config save/load/merge/error paths. |
| src/cli/http.ts | Adds stopServer() and makes spawn injectable for tests; adds NODE_ENV=test browser guard. |
| src/cli/http.test.ts | Adds unit tests for server start/stop and browser-open platform branches. |
| src/cli/commands.ts | Implements restart command and refactors stop to use shared helper. |
| src/cli/commands.test.ts | Adds integration tests for restart behavior. |
| package.json | Adds test:coverage script. |
| docs/specs/webtty.md | Updates spec to reflect config-driven shell + completed config feature. |
| docs/specs/release-process.md | Marks release-process checklist items as done. |
| docs/specs/config.md | Adds full config specification (schema + lifecycle + examples). |
| docs/specs/client.md | Documents banner + lifecycle messaging and injected terminal config. |
| docs/specs/cli.md | Updates CLI spec to include restart and proposed webtty/help. |
| docs/adrs/011.cli.default-and-help.md | Adds ADR for no-arg webtty and webtty help alias (proposed). |
| docs/adrs/010.client.ux-polish.md | Adds ADR documenting banner/message style decisions. |
| docs/adrs/009.webtty.config-hot-reload.md | Adds ADR for config hot reload strategy. |
| docs/adrs/008.webtty.config.md | Adds ADR defining config file location/format/merge strategy. |
| docs/adrs/007.webtty.session-client.md | Updates close-delay semantics and documents server-stop close code. |
| docs/adrs/002.cli.start-stop.md | Updates ADR to include restart and shared stop logic. |
| .gitignore | Ignores lcov.info for coverage output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function stopServer(baseUrl: string = BASE_URL, timeoutMs = 5000): Promise<boolean> { | ||
| try { | ||
| const res = await fetch(`${baseUrl}/api/server/stop`, { method: 'POST' }); | ||
| if (!res.ok) return false; | ||
| const deadline = Date.now() + timeoutMs; | ||
| while (Date.now() < deadline) { | ||
| if (!(await isServerRunning())) return true; | ||
| await new Promise((r) => setTimeout(r, 100)); | ||
| } |
There was a problem hiding this comment.
stopServer(baseUrl, timeoutMs) posts to ${baseUrl}/api/server/stop, but the polling loop calls isServerRunning() which always checks the module-level BASE_URL. If baseUrl differs from BASE_URL (tests or future multi-port support), stopServer can report the wrong result or wait on the wrong server. Consider parameterizing isServerRunning(baseUrl) and using the same base URL throughout stopServer.
There was a problem hiding this comment.
The non-default baseUrl path exists only for tests (free port isolation). In production all CLI commands use BASE_URL so the polling always checks the right server. Threading baseUrl into isServerRunning adds complexity for a test-only edge case.
jesse23
left a comment
jesse23
left a comment
There was a problem hiding this comment.
Thanks for the thorough review! Addressed 7 of the 10 comments (committed as fix: address copilot review comments). Leaving 3 open with rationale:
Port mismatch CLI vs config (#2978719620) — Intentional. Both the CLI and server derive port from process.env.PORT, so setting PORT=XXXX in the environment makes both agree automatically. The config file's port is a fallback for the server only — wiring the CLI to parse the config would add latency to every invocation and a circular dependency. Documented in ADR 008.
stopServer polling wrong base URL (#2978719651) — The isServerRunning() call inside stopServer checking BASE_URL vs the baseUrl param is only observable when passing a non-default baseUrl. In production this never happens (all CLI commands use the default). The baseUrl param exists solely for testability (so tests can use a free port). Fixing it properly would require threading baseUrl into isServerRunning, which adds complexity for a test-only concern. Acceptable as-is.
scrollback string vs bytes (#2978719668) — Valid observation. .slice() on a JS string operates on UTF-16 code units, so for multibyte characters (CJK, emoji) the effective buffer is smaller than config.scrollback bytes. This is a known limitation — fixing it properly requires switching session.scrollback from a string to a Buffer. That's a non-trivial refactor touching session, websocket, and test code. Deferring to a separate issue.
Summary
strip-json-comments), hot reload on tab reload and new PTY spawn, Campbell as default theme, OS-native font stackwebtty restartcommand (stop + start),stopServer()extracted tohttp.tsWS_CLOSEconstants,closeSession/closeAllSessionshelpers — raw close codes no longer leak intoroutes.ts/index.tsconfig.test.ts(18 tests, 100% coverage), expandedclient.test.ts(config injection),http.test.ts(startServer,stopServer,openBrowserplatform branches),session.test.ts(setLastUsedId);NODE_ENV=testguard prevents browser tabs during test runs[ webtty ]identity, slogan, help command with runtime-detectedbunx/npx); all WS lifecycle messages unified with[ webtty ]prefix + dim italic stylewebtty help); specs updated for config, CLI, and clientTest coverage
All files at 100% lines / 100% functions (
bun run test:coverage).