test(tui): extend coverage gate to Ink components + App.tsx (closes #1501)

[cliffhall]

Closes #1501.

Summary

Lifts the interim React-surface coverage exclusion in the TUI client (the follow-up to #1484 / #1532). The whole clients/tui/src tree now clears the uniform 90/90/90/90 per-file gate with no React-surface exclusions — App.tsx, every Ink component, and the useSelectableList hook are all exercised under coverage.

Test-only change: no source files were modified.

How it's tested

Added ink-testing-library and renderer-based tests that mount components in-process and drive keypresses through stdin. Two shared passthrough doubles in __tests__/helpers/ make the components mountable under the non-TTY test renderer:

• inkScrollViewMock.tsx — the real ScrollView renders a placeholder minimap and never mounts its children in a non-TTY env; the double renders children directly and stubs the imperative ref API (scrollBy/scrollTo/getViewportHeight).
• inkFormMock.tsx — the real ink-form Form is hard to drive field-by-field; the double fires onSubmit on Enter.

Per-component approach:

• Tabs + hook (Info/Auth/Resources/Prompts/Tools/Notifications/History/Requests, useSelectableList, SelectableItem, Tabs): assert on rendered frame text and drive list navigation / scroll keys.
• Modals (Tool/Resource/Prompt test modals, DetailsModal): these render position="absolute", which produces an empty frame under the renderer, so their tests assert on behavior — injected InspectorClient fakes being called, onClose, and state transitions — while the passthrough ScrollView still mounts the inner JSX for coverage.
• App.tsx (~1885 lines): mounts against a controllable mock of the entire @inspector/core surface (InspectorClient, the 7 managed-state classes, the 8 react hooks, transport, auth) plus the callback server. 67 tests drive the keyboard state machine, per-tab content, modal-open flows, OAuth (guided/quick/clear, success + error), and connect/disconnect. Each render is unmounted in afterEach (concurrent ink mounts share raw-mode stdin and interfere with useInput).

Verification

• clients/tui npm run validate — clean (format / lint / build / test).
• clients/tui npm run test:coverage — green, 260 tests, every file ≥90 on all four dimensions, no ERROR lines. App.tsx: 94.6 stmts / 90.6 branch / 94.5 funcs / 96.6 lines.

Config

clients/tui/vitest.config.ts: removed all React-surface entries from the coverage exclude (only the pure re-export tui-servers.ts and test/type globs remain) and widened the include glob to *.test.tsx. The interim-scope comment from #1484 is updated to reflect the completed lift.

🤖 Generated with Claude Code

[cliffhall]

Follow-up: bumped App.tsx branch coverage from 90.6% → 91.48% for a safer margin above the 90 gate — added tests for the OAuth handlers' non-Error throw arms (quick onCallback, guided start/advance, run-to-completion) and a re-entrant quick-auth guard.

Also hardened the App renderer tests against the heavier v8 coverage instrumentation (where React+ink's multi-macrotask render scheduling races fixed ticks): press() now settles two ticks per key, and frame/async-state assertions poll via expectFrame/waitUntil instead of asserting after a single tick. Plain npm test (the CI path) was already stable; this makes the local coverage gate stable too — 13/13 clean runs vs ~2/8 flaky before.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 39s —— View job

---

Review: test(tui) coverage gate extension (#1538)

• Read PR diff vs origin/v2/main
• Review test approach & helper mocks
• Review vitest config changes
• Verify suite + coverage locally
• Post review feedback

Verified this is a purely additive, test-only change — git diff origin/v2/main...HEAD over clients/tui/src/** is empty, so no production code was touched. I ran both gates locally:

• npm test → 265 passed (23 files)
• npm run test:coverage → green, every file ≥90 on all four dimensions. App.tsx lands at 94.78 / 91.48 / 94.54 / 96.64 (stmts/branch/funcs/lines), matching the PR description.

This is high-quality work. The tests are genuinely behavior-driven, not coverage-padding: the OAuth state machine, per-server tab gating, modal open/close flows, and connect/disconnect are all exercised through real keypresses and asserted on rendered frames or injected-fake call sites. The two passthrough doubles are the right call and are well-documented at their definition. The decision to assert on behavior for the position="absolute" modals (which render empty frames under the non-TTY renderer) while still mounting the inner JSX for coverage is a reasonable, clearly-explained tradeoff.

Observations (all non-blocking)

1. MaxListenersExceededWarning still printed during the run. Both npm test and test:coverage emit:

   MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 resize listeners added to [Socket].

   App.test.tsx:205 raises process.stdout.setMaxListeners(50), but the warning is on a [Socket] and persists — the modal test files (ToolTestModal, ResourceTestModal, PromptTestModal, DetailsModal) each mount components that also register a process.stdout "resize" listener per mount, and those files don't raise the limit. It's cosmetic (the listeners are cleaned up on unmount; this is just transient overlap), but it adds noise to CI logs that a future reader may mistake for a real leak. Consider hoisting setMaxListeners into a shared vitest setupFiles entry so every renderer test file inherits it rather than only App.test.tsx. Fix this →

2. Shared mutable global globalThis.__INK_FORM_SUBMIT_VALUE__. The ink-form double reads form-submit values from a global. It's cleaned up in ToolTestModal.test.tsx's afterEach, but ResourceTestModal/PromptTestModal/App tests also rely on the same global — worth a quick confirm that each file resets it in afterEach (or that the default-{} fallback makes a stale value harmless). Since vitest isolates module state per test file this is safe today, but it's a latent cross-test coupling if these are ever merged into one file.

3. Timing model. The suite leans on fixed setTimeout ticks plus waitUntil/expectFrame polling. This is inherent to driving Ink through stdin and you've already hardened it (the follow-up comment's 13/13 clean-run result is the right kind of evidence). No action needed — flagging only so reviewers know the polling helpers are deliberate, not accidental.

4. vitest.config.ts exclude list is now correctly reduced to just the pure re-export (tui-servers.ts) plus test/type globs, and the interim-scope comment from CLI/TUI: add source coverage gates (in-process runCli + TUI flows) #1484 is updated to reflect the completed lift. This matches the AGENTS.md direction that the TUI React surface was an interim exclusion pending TUI: extend coverage gate to Ink components (lift interim exclusion) #1501. 👍

Verdict

Solid, well-documented test-only PR that delivers exactly what #1501 asked for: the full clients/tui/src tree under the uniform 90/90/90/90 gate with no React-surface exclusions. Nothing here blocks merge — the items above are minor cleanups, the warning in (1) being the most worth a follow-up.
· branch tui/1501-ink-component-coverage

[cliffhall]

Thanks for the thorough review! Addressed the two actionable items in ce033056:

1. MaxListenersExceededWarning — hoisted process.std{out,in}.setMaxListeners(100) into a shared vitest setupFile (clients/tui/__tests__/setup.ts) wired via setupFiles, so every renderer test file inherits it. Removed the per-file call in App.test.tsx. The warning no longer prints under npm test or test:coverage.
2. Shared __INK_FORM_SUBMIT_VALUE__ global — the ink-form double now consumes the value (clears it) on submit, so a value set by one test can't leak into a later submit regardless of how the modal test files are organized. The latent cross-test coupling is gone at the source.

(3) and (4) needed no action. Suite + coverage gate stay green (App.tsx 94.8/91.5/94.5/96.6).

@claude review

[claude]

Claude finished @cliffhall's task in 1m 48s —— View job

---

Re-review: review follow-ups in ce033056

• Read the diff for the two addressed items
• Verify setupFile wiring + max-listeners fix
• Verify ink-form consume-once global fix
• Run suite + confirm warning gone

Both follow-ups are correctly addressed. Verified locally — 265 passed (23 files) and grep -i MaxListeners over the run output is clean (no warning prints).

1. MaxListenersExceededWarning → shared setupFile ✅

__tests__/setup.ts raises both process.stdout.setMaxListeners(100) and process.stdin.setMaxListeners(100), wired via setupFiles: ["./__tests__/setup.ts"] in vitest.config.ts. Since a setupFile runs once per test file (each worker), every renderer test file now inherits the raised cap — not just App.test.tsx, whose per-file call is correctly removed. Including stdin as well as stdout is a nice touch (ink's raw-mode useInput registers stdin listeners too). The comment accurately explains why it's transient (listeners removed on unmount, no real leak) so a future reader won't mistake it for masking a bug.

2. __INK_FORM_SUBMIT_VALUE__ consume-once ✅

The ink-form double now reads-then-deletes the global on submit:

const value = g.__INK_FORM_SUBMIT_VALUE__ ?? {};
delete g.__INK_FORM_SUBMIT_VALUE__;

This kills the latent cross-test coupling at the source — a value set by one test can no longer leak into a later submit regardless of file organization, and the ?? {} fallback keeps the default-empty behavior. The fix is in the right place (the shared helper), the JSDoc is updated to document the consume semantics, and it removes the reliance on per-file afterEach cleanup that observation (2) flagged. The one behavioral implication — a test submitting twice must re-set the global before each Enter — is the correct, intended contract and matches the documented "set before pressing Enter" usage.

Verdict

Both non-blocking items from the prior review are resolved cleanly, the change stays purely test-only, and the suite + coverage gate remain green (App.tsx 94.8 / 91.5 / 94.5 / 96.6 per the PR description). LGTM. 👍
· branch tui/1501-ink-component-coverage