Increase Ink composer and input-state coverage (Fixes #2018) - #3375
Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (10)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe PR adds tests for ChangesComposer and input-state coverage
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: 🔵 Low · up to The PR is largely test-only and does not change production behavior, but one added paste test contains a TypeScript error that the repository typecheck does not cover. The change is mergeable with explicit owner awareness or follow-up to ensure the test is type-checked. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The reviewable changes address composer visibility, input-state gating, startup errors, dialog suppression, escape handling, paste handling, component and hook combinations, and the tmux smoke test harness. Verification of the tmux smoke implementation is incomplete because the referenced script is excluded from review. Resolution Inspect scripts/tmux-script.issue2018-composer-visibility-smoke.json, which was excluded by the !scripts/tmux-script.*.json path filter, to verify that the gated tmux smoke test meets issue Full details: Docstring CoverageExplanation Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 9 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description includes all required template sections and provides detailed scope, testing instructions, test results, known limitations, and the linked issue. The testing matrix is partially unverified, but the description is otherwise complete. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
WalkthroughBefore this PR, the CLI’s Ink composer and input-state behavior were only indirectly exercised, and the logic deciding whether input was active lived inline inside the app-input hook, making it harder to verify in isolation. After this PR, that input-active determination is extracted into a reusable pure helper without changing runtime behavior, and the affected UI paths—composer visibility, input prompt interactions, notifications, and layout rendering—are covered by new unit and smoke tests. Release NotesNew Features
Bug FixesTests
Documentation
Refactor
ChoreChanges
Magnitude🎯 2 (M) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
OpenCodeReview — PR #3375
Findings routed to summary
|
OCR review triageBoth inline findings examined against the source. Both rejected, with evidence. 1.
|
The Ink surfaces that decide whether the user can type had no direct tests. `isInputActive` was computed inline inside an unexported hook; `Composer.tsx` and `Notifications.tsx` had no test file at all; `InlineContent.test.tsx` always passed `isInputActive: false`, so the branch that actually renders the composer was never taken; `DefaultAppLayout.test.tsx` covered 2 of the 25 dialog flags; and the large-paste placeholder path had no coverage. Production change, and the only one: extract the inline predicate in useAppInput.ts into an exported pure `computeIsInputActive`. Same expression, same short-circuit order, same truthiness for `!initError` / `!isProcessing` / `!!slashCommands`, and the input types are the real hook return types rather than widened ones. `useInputFinish` delegates to it. Coverage added, by behaviour: - Composer visible when command initialization completes. The predicate flips false to true when `slashCommands` goes from undefined to loaded (an empty array counts as loaded), and `InlineContent` renders the real Composer -> InputPrompt with the default, vim and shell placeholders. - Composer hidden only for intentional blocking states. The streaming-state case is table-driven over `Object.values(StreamingState)`, so a newly added state is automatically covered and defaults to hidden. `initError` and `isProcessing` are each proven independently sufficient with every other input held ready. - Dialogs suppress the composer. Driven off the full flag list rather than a hand-picked subset, and guarded by a Proxy that records every property the real `hasActiveDialog` reads, asserting the recorded set equals the test table. That catches drift in both directions; adding `uiState.isNarrow` to the predicate fails the guard. `createBaseUIState` was also missing `isPoliciesDialogOpen`, which this surfaced. - Startup errors render actionable UI. `Notifications` renders the composed "Initialization Error: ..." line plus the remediation line, prefers a fuller matching history error (proven with a non-error distractor and an unrelated error distractor ahead of it), suppresses everything while Responding, and renders warnings alongside an init error. - Escape priority ordering, for the combinations the existing suites did not cover: shell-path suggestions close while shell mode stays active, and dismissing slash suggestions leaves the buffer intact without arming the double-escape prompt. - Large-paste placeholders: line and character thresholds, the three-line boundary, cursor position relative to the placeholder, distinct labels, and round-trip expansion through the real `InputPrompt` submit path so that removing the `expandLargePastePlaceholders` call site fails a test. - One tmux startup smoke asserting the first meaningful frame contains the composer, using a dedicated offline fake-provider fixture. Test-quality notes: Assertions are on rendered output. The `DefaultAppLayout` doubles now render text sentinels instead of being spied on, because asserting mock call counts is not evidence under dev-docs/RULES.md. The new tests were verified to have teeth by breaking the corresponding production code and observing the expected failures: disabling `expandLargePastePlaceholders` breaks the paste submit tests, forcing the composer branch breaks the dialog tests, removing `completion.resetCompletionState()` breaks the escape test, and adding a flag to `hasActiveDialog` breaks the drift guard. Production was restored in each case. The escape tests live in a new `InputPrompt.escape.test.tsx` rather than being appended to `InputPrompt.completion.test.tsx`, which would have pushed that file past the 800-line lint budget. The new file is not on the legacy typecheck-exclusion list, so unlike its siblings it is fully typechecked. An earlier draft of the shell-path test completed against the repository root. That was slow and its assertion depended on the checkout's directory listing, so it failed under full-suite load. It now completes against a dedicated temporary directory. AC4.2 as originally scoped is withdrawn and recorded in the plan. It asked to prove the shell-mode branch of `handleEscapeKey` beats the completion branch, but `useCommandCompletion` passes `reverseSearchActive || shellModeActive` as the disable flag to `useSlashCompletion`, so that combination cannot occur. Covering it would have required mocking the UI into an impossible state. Also noted for follow-up, not changed here: no live call site currently assigns a non-null `initError`. Fixes #2018
OCR review triage (post-retarget run)One accepted and fixed, one rejected with evidence. 1.
|
Pattern injected into hasActiveDialog |
Guard result |
|---|---|
'isNarrow' in uiState |
fails, reporting + "isNarrow" in the read-set diff |
Object.keys(uiState) |
fails: enumerated the UI state via ownKeys |
Object.prototype.hasOwnProperty.call(uiState, 'isNarrow') |
fails: enumerated the UI state via getOwnPropertyDescriptor |
Before this change, pattern A passed silently. DefaultAppLayoutHelpers.tsx was restored to zero diff after each check; the suite is 27 pass / 0 fail.
2. InputPrompt.escape.test.tsx:345 — useState inside mockImplementation violates the Rules of Hooks
Rejected. The mock replaces useCommandCompletion, which is itself a custom hook. React's rule is that hooks may be called from function components or from custom hooks, and the mock occupies exactly the position the real hook occupied: called unconditionally, once per render, from InputPrompt's render body. A stateful stand-in for a stateful hook is not a rules violation; it is the same contract the real implementation satisfies.
The state is not incidental, either. An earlier revision of this test used a plain getter over a closure variable rather than useState. It did not work: resetCompletionState flipped the variable but nothing re-rendered, so the suggestion stayed on screen and the test failed. Driving a real re-render is the point, because the assertion is on the rendered frame rather than on the stub's internals.
That assertion is also what gives the test teeth. Commenting out completion.resetCompletionState() in handleEscapeKey fails it with clear still present in the frame.
Moving the state into a wrapper component would add a layer whose only job is to re-render the component the mocked hook already re-renders, without changing what is proven. Left as is.
Review remediation for PR #3375. The Proxy-based guard in DefaultAppLayout.test.tsx recorded only `get` traps. `hasActiveDialog` reads every flag with a plain property access today, so the guard was accurate for the current implementation, but it promised more than it could deliver: a future edit using `in`, `hasOwnProperty`, `Object.keys` or `for...in` would have reached flags absent from ACTIVE_DIALOG_FLAGS while the guard stayed green. That is precisely the drift the guard exists to catch. The Proxy now also traps `has`, so `'x' in uiState` is recorded like a read. Enumeration is rejected rather than approximated: `ownKeys` and `getOwnPropertyDescriptor` throw, because enumeration hands the predicate every flag at once and makes a recorded read-set meaningless. Symbol keys throw for the same reason instead of being silently dropped. Each error names the fix: update the guard and the flag table together. Verified by injecting each pattern into hasActiveDialog and reverting: 'isNarrow' in uiState -> fails, read-set diff shows + "isNarrow" Object.keys(uiState) -> fails, enumerated via ownKeys Object.prototype.hasOwnProperty.call(uiState,'isNarrow') -> fails, enumerated via getOwnPropertyDescriptor The first of those passed silently before this change. Also rejected, with reasoning recorded on the PR: the claim that `useState` inside the `useCommandCompletion` mock in InputPrompt.escape.test.tsx breaks the Rules of Hooks. The mock stands in for a custom hook and is called unconditionally once per render from InputPrompt, which is exactly where a custom hook may call useState. The state is also required: an earlier revision used a getter over a closure variable, which flipped without re-rendering, so the suggestion stayed on screen and the assertion on the rendered frame failed. Verification: format, lint, typecheck, build and the full suite all pass with zero failures. Refs #2018
1 participant
TLDR
Test coverage for the Ink surfaces that decide whether the user can type: composer visibility, input active-state gating, startup readiness, escape priority, and large-paste handling.
The only production change is a behaviour-preserving extraction: the
isInputActiveexpression inside the unexporteduseInputFinishbecomes an exported purecomputeIsInputActive. Same expression, same short-circuit order, same truthiness semantics, real hook return types rather than widened ones. Everything else in the diff is tests, one tmux script, and one fixture.Reviewers should look hardest at two things. First, the
hasActiveDialogdrift guard inDefaultAppLayout.test.tsx, which uses a Proxy to record every property the real predicate reads and asserts that set equals the test table. Second, whether the new tests actually have teeth; I broke the corresponding production code for each one and recorded the failures below.Dive Deeper
What had no coverage
isInputActivewas computed inline inside an unexported hook, so nothing exercised the predicate.Composer.tsxandNotifications.tsxhad no test file at all.InlineContent.test.tsxalways passedisInputActive: false, so the branch that actually renders the composer was never taken.DefaultAppLayout.test.tsxcovered 2 of the 25 flags read byhasActiveDialog.handleLargePasteandexpandLargePastePlaceholdershad no tests; only the below-threshold branch was touched, indirectly.What the tests now prove
Composer visible when command initialization completes. The predicate flips false to true when
slashCommandsgoes fromundefinedto loaded, with an empty array counting as loaded.InlineContentrenders the realComposerand realInputPromptand shows the default, vim and shell placeholders.Composer hidden only for intentional blocking states. The streaming-state case is table-driven over
Object.values(StreamingState)rather than a hand-listed set, so a newly added state is automatically covered and defaults to hidden.initErrorandisProcessingare each proven independently sufficient with every other input held at its ready value.Dialogs suppress the composer. Every flag, not a hand-picked subset. The guard test wraps the base UI state in a
Proxy, calls the realhasActiveDialog, and asserts the recorded read-set equals the test table, which catches drift in both directions. This also surfaced thatcreateBaseUIStatewas missingisPoliciesDialogOpen.Startup errors render actionable UI.
Notificationsrenders the composedInitialization Error: ...line plus the remediation line; prefers a fuller matching history error, proven with a non-error distractor and an unrelated error distractor ahead of the match so that "render the first error item" would fail; suppresses all of it whileResponding; and renders warnings alongside an init error.Escape priority ordering, limited to the combinations the existing suites did not already cover: shell-path suggestions close while shell mode stays active, and dismissing slash suggestions leaves the buffer intact without arming the double-escape prompt.
Large-paste placeholders: line threshold, character threshold, the three-line boundary, cursor position derived from placeholder length, distinct labels for successive pastes, and round-trip expansion through the real
InputPromptsubmit path.One tmux startup smoke asserting the first meaningful frame contains the composer, using a dedicated offline fake-provider fixture so it needs no credentials.
Teeth checks
Each new test was verified by breaking the production code it covers and observing the failure, then restoring the code:
expandLargePastePlaceholderscall removed frominputPromptHooks.ts[4 lines pasted #1]instead of the bodyMainControlsforced to the Composer branchDIALOG_MANAGER_RENDEREDabsentcompletion.resetCompletionState()removed fromhandleEscapeKeyclearstill present in the frameuiState.isNarrowadded tohasActiveDialogDecisions worth flagging
AC4.2 withdrawn. The plan originally asked for a case proving the shell-mode branch of
handleEscapeKeybeats the completion branch. That state cannot occur:useCommandCompletionpassesreverseSearchActive || shellModeActiveas the disable flag touseSlashCompletion, andinputPromptRender.tsxwill not render slash suggestions in shell mode. A first draft covered it by mockinguseCommandCompletioninto that impossible state; it was removed rather than kept. The reasoning is recorded in the plan document.New file instead of appending. The escape tests are in a new
InputPrompt.escape.test.tsx. Appending them toInputPrompt.completion.test.tsxpushed that file past the 800-line lint budget. The new file is not on the legacy typecheck-exclusion list inpackages/cli/tsconfig.json, so unlike its five siblings it is fully typechecked.Assertions are on rendered output, not spies. The
DialogManagerandComposerdoubles inDefaultAppLayout.test.tsxnow render text sentinels instead of being asserted viatoHaveBeenCalledTimes, since mock-call verification is not evidence underdev-docs/RULES.md.A flaky draft was fixed. An earlier version of the shell-path test completed against the repository root. It was slow and its assertion depended on the checkout's directory listing, and it failed under full-suite load. It now completes against a dedicated temporary directory.
Known unrelated flake
One full-suite run showed a failure in
packages/agents/src/core/turn.watchdog.test.ts, "whole-stream liveness ... => NO timeout". That test is byte-identical tomain, this PR touches no file inpackages/agents, and the test uses real timers with a 30ms idle threshold against eight 10ms sleeps, so heavy parallel load can erase the margin. It passes 5/5 in isolation on this branch and 3/3 under synthetic CPU load on a cleanmainworktree. Flagging it rather than hiding it; CI is the arbiter.Follow-up, not changed here
No live call site currently assigns a non-null
initError. TheInitErrorBoxpath may be dead. Noted in the plan document; changing production behaviour was out of scope for a coverage issue.Reviewer Test Plan
To confirm the tests are not decorative, break the production code and watch them fail:
The tmux smoke is gated and does not run by default:
LLXPRT_E2E_TMUX=1 bun test scripts/tests/interactive-ui.test.tsInteractively, the behaviour under test is the composer itself: start the CLI and confirm the
Type your message or @path/to/fileprompt appears, then check that opening a dialog (/theme,/settings) replaces it, that Escape closes an open suggestion list without clearing what you typed, and that pasting more than four lines collapses to a[N lines pasted #1]placeholder which expands back to the full text on submit.Testing Matrix
Verified on macOS:
npm run format,npm run lint,npm run typecheckandnpm run buildall exit 0. The fullnpm run testrun had the single unrelatedpackages/agentswatchdog flake described above and no failures in any file this PR touches. The tmux lane is gated behindLLXPRT_E2E_TMUX=1and was validated for JSON shape and registration rather than executed locally.Linked issues / bugs
Fixes #2018
Summary by CodeRabbit
Bug Fixes
Tests