diff --git a/AGENTS.md b/AGENTS.md index f1451e0..c5b6dae 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,9 +22,12 @@ src/ Runtime/Tapper.php Client side: collects debug info, sends it over the wire, blocks for the reply Rpc/ Minimal JSON-RPC-ish request/response types + blocking socket client Server.php TUI-process side: unix socket server, decodes requests, mutates AppState + SocketPath.php Resolves the shared unix-socket path + LogPath.php Resolves tapper.log's path (same directory as the socket) Console/ main.php DI container wiring (php-di) for the TUI process Application.php Owns the ReactPHP event loop: render timer, resize timer, input handling + ErrorHandler.php Redirects PHP warnings/notices (and, via bin/tapper, uncaught throwables) to tapper.log instead of stdout, which would otherwise corrupt the raw-mode/alt-screen render Component.php Base class for every UI component (see docs/console-framework.md) EventBus.php Pub/sub used for key/mouse/custom events CommandInvoker.php Thin wrapper around php-di's Invoker (see Commands note below) diff --git a/bin/tapper b/bin/tapper index 08c8f62..2546e90 100755 --- a/bin/tapper +++ b/bin/tapper @@ -9,7 +9,8 @@ $app = (require_once __DIR__.'/../src/Console/main.php'); try { $app->run(); -} catch (\Exception $e) { +} catch (\Throwable $e) { $app->close(); + \Tapper\Console\ErrorHandler::logThrowable($e); throw $e; } diff --git a/docs/console-framework.md b/docs/console-framework.md index 3aa47f6..b52a6b3 100644 --- a/docs/console-framework.md +++ b/docs/console-framework.md @@ -61,7 +61,7 @@ A single, app-wide, mutable object. It is the *only* channel by which the socket - **Magic properties**: `__get`/`__set` proxy to real private/constructor-promoted properties. The `@property` PHPDoc block at the top of the class is what makes these visible to IDEs/static analysis at all — it is not generated, it must be hand-kept in sync with the constructor's promoted properties. If you add a field to one and forget the other, you get either a silent dynamic property (constructor not updated) or an IDE that can't see a real property (docblock not updated). - **Change notification**: every `__set` (outside a batch) calls `notifyChange()` (the single `$change` callback registered once, by `Application::startRendering()`, to flip `shouldDraw = true`) and `callObservers($name)` (per-field subscribers registered via `observe($name, $callable)`). - **Field-specific observers**: `observe(string $name, callable $callable)` validates `$name` exists via `get_class_vars($this::class)` at call time (a `RuntimeException` if you typo a field name — this is your only safety net, and it's runtime-only). Multiple components observe overlapping fields today, e.g. `LogList::beforeInit()` observes `logs`, `cursor`, and `live` to keep unread counts and live-follow behavior in sync — read that method as the canonical example of cross-field reactive logic. -- **Batching**: `deffer()` (sic — note the actual method name has this typo, not "defer") sets a `batching` flag; subsequent `__set` calls accumulate field names into `$changed` instead of notifying immediately. `commit()` flips batching off, replays observers for every accumulated field, then calls `notifyChange()` once. **There is a documented, unresolved bug here** — see the `@TODO` comment directly in `AppState.php`: repeatedly setting the same field while batched (the example given is pressing Enter repeatedly while `tp()->wait()` is paused) causes `$changed` to "overflow" in some way that isn't fully diagnosed. Don't build new batched-write logic on top of `deffer()`/`commit()` until this is understood; prefer direct `__set` (unbatched) if you're unsure. +- **Batching**: `deffer()` (sic — note the actual method name has this typo, not "defer") sets a `batching` flag; subsequent `__set` calls accumulate field names into `$changed` instead of notifying immediately. `commit()` flips batching off, replays observers for every accumulated field, then calls `notifyChange()` once. `$changed` is keyed by field name (a set, not a list), so repeatedly setting the same field while batched — e.g. pressing Enter many times while a `tp()->wait()` is paused — no longer grows `$changed` unbounded or replays observers once per write; each field's observers replay exactly once per `commit()`. ## `Support/Scroll.php` diff --git a/docs/known-issues.md b/docs/known-issues.md index 93b329f..9363ecd 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -12,47 +12,38 @@ Verified with an isolated smoke test (no terminal required): booted `Server` dir Note: `SocketPath::resolve()` still resolves to "the installed package's own directory" (`realpath(__DIR__.'/..')`), same semantics as the original `Server.php` code — when Tapper is installed as a dependency, that's `vendor/tapperphp/tapper/tapper.sock`, not the consuming project's root. That's fine functionally (both processes resolve the same install, so they agree), but if this ever needs to be more conventional (e.g. `sys_get_temp_dir()`-based, to avoid touching `vendor/`), that's a deliberate follow-up, not part of this fix. -### `wait()` listeners on `EventBus` are never cleaned up +### ~~`wait()` listeners on `EventBus` are never cleaned up~~ — fixed 2026-08-15 -See `rpc-protocol.md`'s `wait` section — every `tp(...)->wait()` call adds a permanent `KeyCode::Enter` listener to `EventBus` that's never removed, even after it fires. Two overlapping `wait()` calls will both resolve on the same keypress instead of one at a time. `EventBus` has no unsubscribe mechanism at all today (see below). +Previously every `tp(...)->wait()` call added a permanent `KeyCode::Enter` listener to `EventBus`, never removed, so overlapping `wait()` calls all resolved on the same keypress instead of one at a time. Fixed in `Server.php`: wait resolvers are now pushed onto a FIFO queue (`$waitResolvers`), and a single `KeyCode::Enter` listener is registered once (`registerWaitListener()`, guarded by `$waitListenerRegistered`) that pops and resolves the oldest pending wait per keypress. No per-call listener is added anymore, so there's nothing to leak, and N overlapping waits now require N separate Enter presses, in order. `EventBus` itself still has no general unsubscribe mechanism — see below, this remains a real gap for any future feature with dynamic listener lifecycles. -### `Application::run()` registers signal handlers after the blocking call that would need them +### ~~`Application::run()` registers signal handlers after the blocking call that would need them~~ — fixed 2026-08-15 -```php -$this->loop->run(); // blocks until loop stops +`addSignal(SIGINT, ...)`/`addSignal(SIGTERM, ...)` are now registered before `$this->loop->run()`, so they're actually reachable, and their bodies call `$this->close()` (restores raw mode, mouse capture, cursor, alternate screen) instead of `echo 'kill'`. **This alone does not make Ctrl+C work**, though — see below. -$this->loop->addSignal(SIGINT, function () { echo 'kill'; }); -$this->loop->addSignal(SIGTERM, function () { echo 'kill'; }); -``` +### Ctrl+C did nothing even after the signal-handler fix above — fixed 2026-08-15 -Both `addSignal` calls are unreachable until `loop->run()` returns, at which point registering them is moot. Move them before `$this->loop->run()`. Also worth deciding what SIGINT/SIGTERM *should* do — right now the handler bodies just `echo 'kill'` rather than calling `$this->close()` to restore the terminal, so a Ctrl+C could leave the terminal in raw/alternate-screen mode. +Root cause: `Terminal::enableRawMode()` shells out to `stty raw` (`vendor/php-tui/term/src/RawMode/SttyRawMode.php`), which disables `ISIG` at the tty driver level. With `ISIG` off, Ctrl+C is never translated into a `SIGINT` signal in the first place — it arrives as a normal byte, `\x03` (ETX), on stdin, same as any other keypress. So `$this->loop->addSignal(SIGINT, ...)` was structurally unreachable for the Ctrl+C case specifically, regardless of registration order (it still matters for external `kill -INT`/`SIGTERM`, which aren't tty-mediated). Fixed in `Application::startInputHandling()` by treating `"\x03"` the same as the existing `'q'` raw-byte check — both now call `$this->close()`. -### `AppState` batching bug (documented in-code, unresolved) +### ~~`AppState` batching bug (documented in-code, unresolved)~~ — fixed 2026-08-15 -`src/Console/State/AppState.php`, directly above `__set`: +`$changed` was a list, so `__set`-ing the same field repeatedly while batched (e.g. pressing Enter many times while `tp()->wait()` is paused) appended a duplicate entry per write, growing unbounded and replaying that field's observers once per duplicate on `commit()`. Fixed by keying `$changed` on field name (`$this->changed[$name] = true`) so it behaves as a set; `commit()` iterates `array_keys($this->changed)`. Same fix applied to `appendLog()`'s batched branch (`'logs'` key). -```php -/* - * @TODO investigate why `changed` overflows - * when setting something multiple times, - * like pressing enter many times - * when waiting is set on tp - */ -``` +### ~~`JsonRpcRequest::payload()` references an undefined `$id`~~ — fixed 2026-08-15 -Don't build new logic on `deffer()`/`commit()` batching until this is diagnosed — prefer unbatched (direct) `__set` calls if a new feature doesn't clearly need batching. +Added a real `private ?string $id = null` constructor parameter; `payload()` now reads `$this->id ?? uniqid('rpc_', true)` instead of an undefined `$id` that only worked by accident via `??`'s notice suppression. -### `JsonRpcRequest::payload()` references an undefined `$id` +### ~~`Server::$id` is `static` inside a container-managed singleton~~ — fixed 2026-08-15 -```php -'id' => $id ?? uniqid('rpc_', true), -``` +Changed to `private int $id = 0` (instance property), consistent with the rest of the class's DI-managed design. -`$id` is never assigned in scope, so this always evaluates to `uniqid('rpc_', true)` — it works, but only because of PHP's `??` suppressing the undefined-variable notice, not because there's an actual optional-id feature. Either add a real `?string $id = null` constructor parameter to `JsonRpcRequest` if per-request ids are wanted, or simplify to a direct `uniqid('rpc_', true)` call and drop the `??`. +### ~~PHP warnings/notices printed straight onto the live TUI, corrupting the render~~ — fixed 2026-08-16 -### `Server::$id` is `static` inside a container-managed singleton +Reported case: pressing Space in `LogList` with zero log entries hit `LogList::select()` reading `$this->appState->logs()[$this->appState->cursor]` with an empty `logs` array — an undefined-array-key warning, repeated once per render tick, each one printed raw over the alternate-screen buffer. Two fixes: -`Server` is constructor-injected with `AppState`/`EventBus` (i.e., it's a normal DI-managed instance), but its log-id counter is `private static $id = 0`. Harmless today (one `Server` instance ever exists), but inconsistent with the rest of the class's design and would break if `Server` were ever instantiated more than once (e.g. in a test). Make it an instance property. +1. Root cause: `LogList::select()` now reads via `?? null` and no-ops when there's no log at the cursor, instead of indexing blind. +2. General case: `Console\ErrorHandler::install()` (called first thing in `Application::run()`, before raw mode/alt-screen are enabled) installs a `set_error_handler` that logs any warning/notice/deprecation to `tapper.log` (`LogPath::resolve()`, same directory as `tapper.sock`) and sets a 5-second `AppState::errorNotice` banner (rendered by `Header`) instead of letting PHP print to stdout. Uncaught `Throwable`s still propagate normally — `bin/tapper`'s `catch (\Throwable $e)` (widened from `\Exception`) calls `Application::close()` to restore the terminal, logs via `ErrorHandler::logThrowable()`, then rethrows so the trace prints on a normal, restored terminal rather than corrupting the TUI. + +Note: this intentionally does *not* revive `Windows/Popup.php` for the "check the logs" notice — see the dead-scaffolding entry below; the banner lives in `Header` via two new `AppState` fields (`errorNotice`, `errorNoticeExpiresAt`) instead. ## Incomplete abstractions (finish or remove, don't extend as-is) @@ -61,7 +52,7 @@ Don't build new logic on `deffer()`/`commit()` batching until this is diagnosed - **`docs/openrpc.json`** — describes a method (`appendLog`, nested `details` param) that doesn't match the implemented protocol (`log`/`wait`, flat params). Regenerate from `Server.php` or delete. - **`Windows/Popup.php`** — exists, is instantiated and checked (`Application::draw()` renders it when `isActive()`), but its `view()` just returns an empty `BlockWidget::default()` and nothing in the codebase ever calls `activate()` on it (confirmed by grep — zero hits). Dead scaffolding for a not-yet-built feature (likely a modal/dialog system) — fine to leave, but don't assume it's a working popup system if you go looking for one. - **`AppState::typingMode` is only ever set to `false`**, never `true`, anywhere in the codebase (confirmed by grep). `Application::handleEventInTypingMode()` therefore always returns early — its entire body (redirecting character input, exiting on Esc) is currently unreachable dead code, presumably scaffolding for a not-yet-built text-input feature. -- **`EventBus` has no unsubscribe** — every `listen()` is permanent. This is fine for the fixed component tree Tapper has today (components are never torn down mid-run), but is a real gap the moment anything needs dynamic component lifecycles (the `wait()` bug above is a direct symptom of this gap). +- **`EventBus` has no unsubscribe** — every `listen()` is permanent. This is fine for the fixed component tree Tapper has today (components are never torn down mid-run), but is a real gap the moment anything needs dynamic component lifecycles. (The `wait()` leak that used to be a direct symptom of this gap is fixed above by having `Server` register one permanent listener instead of one per call — that sidesteps the gap for this one case, it doesn't close it.) ## Framework-extraction readiness diff --git a/src/Console/Application.php b/src/Console/Application.php index 86a2f34..ee65e9c 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -52,6 +52,8 @@ public function __construct( public function run(): int { + ErrorHandler::install($this->appState); + $this->area = $this->phpTermBackend->size(); $this->appState->version = 'v0.1.1'; $this->terminal->execute(Actions::alternateScreenEnable()); @@ -64,15 +66,16 @@ public function run(): int $this->startRendering(); $this->startInputHandling(); $this->server->run(); - $this->loop->run(); $this->loop->addSignal(SIGINT, function () { - echo 'kill'; + $this->close(); }); $this->loop->addSignal(SIGTERM, function () { - echo 'kill'; + $this->close(); }); + $this->loop->run(); + return 0; } @@ -127,7 +130,9 @@ private function startInputHandling(): void $this->handleEventInTypingMode($event, $data); } - if ($data === 'q') { + // `stty raw` (enabled by Terminal::enableRawMode()) disables ISIG, so Ctrl+C + // never reaches us as SIGINT — it arrives as byte 0x03 (ETX) on stdin instead. + if ($data === 'q' || $data === "\x03") { $this->close(); } }); diff --git a/src/Console/Components/Header.php b/src/Console/Components/Header.php index b9ea5bc..a4ccc3f 100644 --- a/src/Console/Components/Header.php +++ b/src/Console/Components/Header.php @@ -4,17 +4,29 @@ namespace Tapper\Console\Components; +use PhpTui\Tui\Color\RgbColor; use PhpTui\Tui\Display\Area; use PhpTui\Tui\Extension\Core\Widget\BlockWidget; use PhpTui\Tui\Extension\Core\Widget\ParagraphWidget; +use PhpTui\Tui\Style\Style; use PhpTui\Tui\Text\Span; use PhpTui\Tui\Widget\Borders; use PhpTui\Tui\Widget\BorderType; use PhpTui\Tui\Widget\Widget; +use Tapper\Console\CommandAttributes\Periodic; use Tapper\Console\Component; +use Tapper\Console\Palette; class Header extends Component { + #[Periodic(1.0)] + public function clearExpiredErrorNotice(): void + { + if ($this->appState->errorNotice !== null && microtime(true) >= $this->appState->errorNoticeExpiresAt) { + $this->appState->errorNotice = null; + } + } + protected function view(Area $area): Widget { $waiting = $this->appState->pendingWaits > 0; @@ -41,6 +53,9 @@ protected function view(Area $area): Widget $unread ? Span::fromString(sprintf(' (↓%s)', $this->appState->unread))->yellow() : Span::fromString(''), Span::fromString(' | '), Span::fromString(sprintf('port: %s', $this->appState->port)), + $this->appState->errorNotice !== null + ? Span::styled(' | '.$this->appState->errorNotice, Style::default()->fg(RgbColor::fromHex(Palette::ERROR))) + : Span::fromString(''), ), ); } diff --git a/src/Console/Components/LogList.php b/src/Console/Components/LogList.php index 259eff3..fbce96a 100644 --- a/src/Console/Components/LogList.php +++ b/src/Console/Components/LogList.php @@ -111,7 +111,13 @@ public function pageDown(): void #[KeyPressed(' ')] public function select(): void { - $this->appState->previewLog = $this->appState->logs()[$this->appState->cursor]; + $log = $this->appState->logs()[$this->appState->cursor] ?? null; + + if ($log === null) { + return; + } + + $this->appState->previewLog = $log; } #[KeyPressed(KeyCode::Enter)] diff --git a/src/Console/ErrorHandler.php b/src/Console/ErrorHandler.php new file mode 100644 index 0000000..102d62d --- /dev/null +++ b/src/Console/ErrorHandler.php @@ -0,0 +1,74 @@ +getMessage(), + $e->getFile(), + $e->getLine(), + $e->getTraceAsString(), + )); + } + + private static function log(string $entry): void + { + $line = sprintf('[%s] %s%s', date('Y-m-d H:i:s'), $entry, PHP_EOL); + + file_put_contents(LogPath::resolve(), $line, FILE_APPEND); + } + + private static function notify(AppState $appState): void + { + $appState->errorNotice = '⚠ error — see tapper.log'; + $appState->errorNoticeExpiresAt = microtime(true) + self::NOTICE_SECONDS; + } + + private static function severityLabel(int $severity): string + { + return match ($severity) { + E_WARNING, E_USER_WARNING => 'Warning', + E_NOTICE, E_USER_NOTICE => 'Notice', + E_DEPRECATED, E_USER_DEPRECATED => 'Deprecated', + default => 'Error', + }; + } +} diff --git a/src/Console/State/AppState.php b/src/Console/State/AppState.php index 6a92849..7b6380d 100644 --- a/src/Console/State/AppState.php +++ b/src/Console/State/AppState.php @@ -21,6 +21,8 @@ * @property int $detailsOffset * @property int $pendingWaits * @property bool $popupOpen + * @property ?string $errorNotice + * @property float $errorNoticeExpiresAt */ class AppState { @@ -50,6 +52,8 @@ public function __construct( private int $detailsOffset = 0, private int $pendingWaits = 0, private bool $popupOpen = false, + private ?string $errorNotice = null, + private float $errorNoticeExpiresAt = 0.0, ) {} /** @@ -83,15 +87,13 @@ public function appendLog(LogItem $logItem): bool $this->logs[] = $logItem; } - if (! $this->batching) { + if ($this->batching) { + $this->changed['logs'] = true; + } else { $this->notifyChange(); $this->callObservers('logs'); } - if ($this->batching) { - $this->changed[] = 'logs'; - } - return ! $isRepeat; } @@ -121,7 +123,7 @@ public function commit(): void { $this->batching = false; - foreach ($this->changed as $field) { + foreach (array_keys($this->changed) as $field) { $this->callObservers($field); } @@ -130,21 +132,16 @@ public function commit(): void $this->notifyChange(); } - /* - * @TODO investigate why `changed` overflows - * when setting something multiple times, - * like pressing enter many times - * when waiting is set on tp - */ public function __set($name, $value) { $this->$name = $value; if ($this->batching) { - $this->changed[] = $name; - } - - if (! $this->batching) { + // Keyed by field name (a set, not a list) so repeatedly setting the same + // field while batched — e.g. pressing Enter many times while a tp()->wait() + // is pending — doesn't grow $changed unbounded or replay observers per write. + $this->changed[$name] = true; + } else { $this->notifyChange(); $this->callObservers($name); } diff --git a/src/LogPath.php b/src/LogPath.php new file mode 100644 index 0000000..af61417 --- /dev/null +++ b/src/LogPath.php @@ -0,0 +1,21 @@ + '2.0', 'method' => $this->method, 'params' => $this->params, - 'id' => $id ?? uniqid('rpc_', true), + 'id' => $this->id ?? uniqid('rpc_', true), ]; } } diff --git a/src/Server.php b/src/Server.php index b313c09..f945734 100644 --- a/src/Server.php +++ b/src/Server.php @@ -14,7 +14,12 @@ class Server { - private static $id = 0; + private int $id = 0; + + /** @var list FIFO queue of pending wait() resolvers, one per outstanding tp()->wait() call */ + private array $waitResolvers = []; + + private bool $waitListenerRegistered = false; public function __construct( private readonly AppState $appState, @@ -54,7 +59,7 @@ public function run(): void case 'log': $kind = $params['kind'] ?? 'log'; $isAppended = $this->appState->appendLog(new LogItem( - self::$id, + $this->id, $params['microtime'], $kind === 'error' ? $params['message'] : json_encode($params['message'], JSON_UNESCAPED_UNICODE), $params['caller'], @@ -71,13 +76,13 @@ public function run(): void ]); if ($isAppended) { - self::$id++; + $this->id++; } break; case 'wait': $isAppended = $this->appState->appendLog(new LogItem( - self::$id, + $this->id, $params['microtime'], "⏸ {$params['message']} — press ENTER to continue", $params['caller'], @@ -88,12 +93,12 @@ public function run(): void )); if ($isAppended) { - self::$id++; + $this->id++; } $this->appState->pendingWaits++; - $this->eventBus->listen(KeyCode::Enter, function () use ($encoder, $id) { + $this->waitResolvers[] = function () use ($encoder, $id) { $encoder->write([ 'jsonrpc' => '2.0', 'result' => 'continue', @@ -101,7 +106,9 @@ public function run(): void ]); $this->appState->pendingWaits = max(0, $this->appState->pendingWaits - 1); - }); + }; + + $this->registerWaitListener(); break; @@ -118,4 +125,26 @@ public function run(): void }); }); } + + /** + * Registers a single, permanent Enter listener the first time it's needed, instead of + * one per wait() call. Each keypress resolves the oldest pending wait in FIFO order, so + * overlapping wait() calls no longer all resolve on the same keypress. + */ + private function registerWaitListener(): void + { + if ($this->waitListenerRegistered) { + return; + } + + $this->waitListenerRegistered = true; + + $this->eventBus->listen(KeyCode::Enter, function () { + $resolver = array_shift($this->waitResolvers); + + if ($resolver) { + $resolver(); + } + }); + } }