refactor(terminal): split input handling#69
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughExtracts prompt lifecycle, response streaming/side-effects, cancellation, follow-up queueing, UI toggles/helpers, and escape/force-exit handling out of input.go into focused files under internal/terminal; input.go is simplified to event dispatch and key routing. ChangesTerminal prompt and input escape refactoring
Sequence Diagram — prompt send & response flow: sequenceDiagram
participant User
participant App
participant Runtime
participant SessionRepo
User->>App: submit composer text
App->>App: trim/validate, run extensions
App->>Runtime: runtime.Prompt(PromptRequest) [goroutine]
Runtime-->>App: stream callbacks / final PromptResponse
App->>SessionRepo: persist session settings (on response)
App->>App: apply streamed side-effects, append assistant message, process queued prompts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
+ Coverage 62.42% 63.70% +1.28%
==========================================
Files 188 195 +7
Lines 17803 17812 +9
==========================================
+ Hits 11113 11348 +235
+ Misses 5591 5369 -222
+ Partials 1099 1095 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/terminal/prompt_toggle_test.go (1)
10-56: ⚡ Quick winPrefer table-driven subtests for the toggle behavior cases.
TestToggleToolsExpandedandTestToggleThinkingHiddenare structurally identical; converting these into a shared table-driven test will reduce duplication and align with repo test style.Proposed refactor
+func TestToggleFlags(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + run func(*App) + getFlag func(*App) bool + wantOnStatus string + wantOffStatus string + flagName string + }{ + { + name: "toolsExpanded", + run: (*App).toggleToolsExpanded, + getFlag: func(a *App) bool { return a.toolsExpanded }, + wantOnStatus: "tool output expanded: on", + wantOffStatus: "tool output expanded: off", + flagName: "toolsExpanded", + }, + { + name: "hideThinking", + run: (*App).toggleThinkingHidden, + getFlag: func(a *App) bool { return a.hideThinking }, + wantOnStatus: "thinking hidden: on", + wantOffStatus: "thinking hidden: off", + flagName: "hideThinking", + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + app := newRenderTestApp(t) + + tc.run(app) + if !tc.getFlag(app) { + t.Fatalf("%s should be true after first toggle", tc.flagName) + } + if got := app.statusMessage; got != tc.wantOnStatus { + t.Fatalf("statusMessage = %q, want %q", got, tc.wantOnStatus) + } + + tc.run(app) + if tc.getFlag(app) { + t.Fatalf("%s should be false after second toggle", tc.flagName) + } + if got := app.statusMessage; got != tc.wantOffStatus { + t.Fatalf("statusMessage = %q, want %q", got, tc.wantOffStatus) + } + }) + } +}As per coding guidelines, "
**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/prompt_toggle_test.go` around lines 10 - 56, Combine the two near-identical tests into a single table-driven test that iterates over cases describing the toggle action, initial expectations and messages; for each case use t.Run to create a subtest that creates app := newRenderTestApp(t), calls the toggle function (toggleToolsExpanded or toggleThinkingHidden) twice, and asserts the boolean fields (toolsExpanded or hideThinking) and statusMessage after the first and second toggles. Ensure each table entry contains: a name, a function pointer to call (e.g., app.toggleToolsExpanded / app.toggleThinkingHidden), references to the target boolean field to check (toolsExpanded / hideThinking) and the expected statusMessage values ("...: on"/"...: off"), and perform both checks within the subtest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/terminal/prompt_cancel.go`:
- Line 38: The cancel handler currently wipes app.queuedMessages which drops any
follow-up text; instead, stop clearing app.queuedMessages in the cancel flow and
only revert the in-flight turn/state (e.g., reset the active prompt/inFlightTurn
or currentPrompt variables used in this file) so queuedMessages remain intact
for subsequent turns; move any clearing of app.queuedMessages to the code path
that explicitly starts a new turn or a deliberate "discard queued" action rather
than inside the cancel routine.
In `@internal/terminal/prompt_queue.go`:
- Around line 18-20: The queueFollowUpText helper currently appends text
verbatim to app.queuedMessages; add a guard in queueFollowUpText to trim
whitespace (e.g., strings.TrimSpace) and return early if the trimmed string is
empty so empty/whitespace-only payloads are never enqueued. Update the function
that references queueFollowUpText to rely on this invariant and ensure
app.queuedMessages only contains non-empty messages.
In `@internal/terminal/prompt_response.go`:
- Around line 24-27: The nil-response branch in the function leaves
app.streamedToolEvents populated, causing the next prompt's
applyRemainingSideEffects to incorrectly skip tool events; update the branch
that handles response == nil (the block that sets app.activePrompt = nil and
calls app.processQueuedPrompt(ctx)) to also reset app.streamedToolEvents (e.g.,
set it to nil or empty) before calling app.processQueuedPrompt(ctx) and
returning so streamed events do not leak into the next prompt.
---
Nitpick comments:
In `@internal/terminal/prompt_toggle_test.go`:
- Around line 10-56: Combine the two near-identical tests into a single
table-driven test that iterates over cases describing the toggle action, initial
expectations and messages; for each case use t.Run to create a subtest that
creates app := newRenderTestApp(t), calls the toggle function
(toggleToolsExpanded or toggleThinkingHidden) twice, and asserts the boolean
fields (toolsExpanded or hideThinking) and statusMessage after the first and
second toggles. Ensure each table entry contains: a name, a function pointer to
call (e.g., app.toggleToolsExpanded / app.toggleThinkingHidden), references to
the target boolean field to check (toolsExpanded / hideThinking) and the
expected statusMessage values ("...: on"/"...: off"), and perform both checks
within the subtest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a7dffee-e345-4f34-874e-36fcfd347183
📒 Files selected for processing (11)
internal/terminal/prompt.gointernal/terminal/prompt_cancel.gointernal/terminal/prompt_queue.gointernal/terminal/prompt_queue_test.gointernal/terminal/prompt_response.gointernal/terminal/prompt_response_test.gointernal/terminal/prompt_send.gointernal/terminal/prompt_submit.gointernal/terminal/prompt_toggle.gointernal/terminal/prompt_toggle_test.gointernal/terminal/queue_test.go
💤 Files with no reviewable changes (2)
- internal/terminal/queue_test.go
- internal/terminal/prompt.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/terminal/prompt_response_test.go (1)
45-61: 💤 Low valueOptional: assert
workingis reset.The test sets
app.working = true(Line 51) but never asserts its final state. If applying a nil response is expected to clear the working flag, assertingapp.working == falsewould make the test's intent explicit; otherwise the assignment is dead setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/prompt_response_test.go` around lines 45 - 61, TestApplyPromptResponseNilClearsStreamedToolEvents sets app.working = true but never asserts it; update the test to assert that calling app.applyPromptResponse(context.Background(), nil, app.activePrompt.ID) also clears the working flag by adding an assertion that app.working is false (or use t.Fatalf/t.Fatal with a clear message) after the existing checks so the intent that applyPromptResponse resets working is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/terminal/prompt_response_test.go`:
- Around line 45-61: TestApplyPromptResponseNilClearsStreamedToolEvents sets
app.working = true but never asserts it; update the test to assert that calling
app.applyPromptResponse(context.Background(), nil, app.activePrompt.ID) also
clears the working flag by adding an assertion that app.working is false (or use
t.Fatalf/t.Fatal with a clear message) after the existing checks so the intent
that applyPromptResponse resets working is explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae597692-ce0b-4b9b-ad6e-d0352d9bd682
📒 Files selected for processing (7)
internal/terminal/prompt_cancel.gointernal/terminal/prompt_cancel_test.gointernal/terminal/prompt_queue.gointernal/terminal/prompt_queue_test.gointernal/terminal/prompt_response.gointernal/terminal/prompt_response_test.gointernal/terminal/prompt_toggle_test.go
💤 Files with no reviewable changes (1)
- internal/terminal/prompt_cancel.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/terminal/prompt_queue.go
- internal/terminal/prompt_response.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/terminal/prompt_send_test.go (1)
15-15: 💤 Low valueOptional: annotate the blank SQLite driver import.
SonarCloud flags the side-effect import. A short comment documents intent and silences the warning.
Proposed tweak
- _ "modernc.org/sqlite" + _ "modernc.org/sqlite" // registers the "sqlite" database/sql driver used by the in-memory test DB🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/prompt_send_test.go` at line 15, The side-effect import _ "modernc.org/sqlite" in internal/terminal/prompt_send_test.go should be annotated to document intent and silence SonarCloud; update the import line for the sqlite driver (the blank import) to include a short comment (e.g., "// register sqlite driver for tests" or similar) explaining it is intentional and used for driver registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/terminal/prompt_queue_test.go`:
- Around line 170-188: The third test case leaves an in-flight goroutine racing
with test cleanup because processQueuedPrompt → sendPrompt launches runPrompt
which posts async events; modify that test case to wait for the dispatched
prompt before returning (e.g., after calling app.processQueuedPrompt, call
readPromptAsyncEvent(app) or waitForPromptRequest(app) or wait on client.ready)
so the goroutine has completed its runtime/DB work and posted its event before
t.Cleanup runs; ensure you only add synchronization in the specific case that
dequeues/sends a prompt (the case asserting "sends first queued prompt") and do
not change other assertions.
---
Nitpick comments:
In `@internal/terminal/prompt_send_test.go`:
- Line 15: The side-effect import _ "modernc.org/sqlite" in
internal/terminal/prompt_send_test.go should be annotated to document intent and
silence SonarCloud; update the import line for the sqlite driver (the blank
import) to include a short comment (e.g., "// register sqlite driver for tests"
or similar) explaining it is intentional and used for driver registration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae2ee9ae-ac45-4a46-aecb-7c00109fcc7d
📒 Files selected for processing (6)
internal/terminal/focus_test.gointernal/terminal/interrupt_test.gointernal/terminal/prompt_cancel_test.gointernal/terminal/prompt_queue_test.gointernal/terminal/prompt_response_test.gointernal/terminal/prompt_send_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/terminal/focus_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/terminal/prompt_send_test.go (1)
395-395:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unreachable return statement.
Both branches of the select statement either return (line 390) or call
t.Fatal(line 392), which does not return. Thereturn nilon line 395 is unreachable dead code.🧹 Proposed fix
return promptEvent case <-time.After(time.Second): t.Fatal("timed out waiting for async event") } - - return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/prompt_send_test.go` at line 395, The return nil after the select in the test is unreachable because both select branches either return a value or call t.Fatal, so remove the dead statement (delete the trailing "return nil") in internal/terminal/prompt_send_test.go within the test function that contains the select to clean up unreachable code.
🧹 Nitpick comments (1)
internal/terminal/prompt_send_test.go (1)
50-50: 💤 Low valueRemove redundant zero-value initialization.
The zero value of
sync.Mutexis already a valid, unlocked mutex. Explicitly writingsync.Mutex{}is redundant.♻️ Simplify field initialization
- lock: sync.Mutex{}, + lock: sync.Mutex{}, // or omit entirely and rely on zero valueOr simply omit the field from the literal and rely on the zero value:
return &terminalPromptClient{ response: response, request: nil, err: err, ready: make(chan struct{}), - lock: sync.Mutex{}, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/prompt_send_test.go` at line 50, The struct literal initializes the lock field as a zero-value sync.Mutex (lock: sync.Mutex{}), which is redundant; remove the explicit sync.Mutex{} entry (or omit the lock field entirely from the literal) so the field relies on the zero value mutex instead — look for the literal that sets lock and remove that key/value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/terminal/prompt_send_test.go`:
- Line 395: The return nil after the select in the test is unreachable because
both select branches either return a value or call t.Fatal, so remove the dead
statement (delete the trailing "return nil") in
internal/terminal/prompt_send_test.go within the test function that contains the
select to clean up unreachable code.
---
Nitpick comments:
In `@internal/terminal/prompt_send_test.go`:
- Line 50: The struct literal initializes the lock field as a zero-value
sync.Mutex (lock: sync.Mutex{}), which is redundant; remove the explicit
sync.Mutex{} entry (or omit the lock field entirely from the literal) so the
field relies on the zero value mutex instead — look for the literal that sets
lock and remove that key/value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d058d164-dfb1-4cf4-9744-fa8b8907396c
📒 Files selected for processing (1)
internal/terminal/prompt_send_test.go
|
This was referenced Jun 1, 2026
Merged
Merged
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
2 participants
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.
Summary
Validation