fix/active-prompt-agent-inspection - #239
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe terminal now preserves per-session UI state during agent-task inspection, routes prompt and completion events to owning sessions, supports inspection while a parent prompt runs, and applies read-only input and escape-key behavior with expanded coverage. ChangesPrompt-aware inspection flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
==========================================
- Coverage 84.72% 84.72% -0.01%
==========================================
Files 315 316 +1
Lines 29299 29550 +251
==========================================
+ Hits 24825 25035 +210
- Misses 3068 3096 +28
- Partials 1406 1419 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/terminal/agent_tasks.go (1)
1631-1675: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
leaveAgentTaskSession's success path never refreshes the durable transcript.
messagesis fetched viaapp.sessionMessages(ctx, parentSessionID)up front, but it's only consumed inside theif !app.restoreSessionView(parentSessionID)fallback branch. WhenrestoreSessionViewsucceeds (the common case — the parent session was previously saved on the way into inspection), the freshly-fetchedmessagesis discarded and the durable transcript relies entirely on whatever was captured in the in-memorysessionViews[parentSessionID]snapshot. Contrast withswitchToAgentTaskSession's success branch (lines 1560-1577), which explicitly nullsapp.transcript.Historyand re-appends freshmessagesbefore restoring transient state. Any message persisted to the parent session's durable store through a path that doesn't route throughwithSessionView's save while the child is being inspected will not appear once the user returns to the parent — and this isn't covered by any existing test (TestRevisitAgentTaskSessionRefreshesDurableTranscriptonly exercises the equivalent refresh on theinspectAgentTask/switchToAgentTaskSessionside).🐛 Proposed fix to mirror switchToAgentTaskSession's refresh-on-restore behavior
app.stopAgentTaskWatches() app.saveSessionView() app.agentTaskSessionStack = app.agentTaskSessionStack[:last] - if !app.restoreSessionView(parentSessionID) { + if app.restoreSessionView(parentSessionID) { + promptHistory := app.promptHistory + promptHistoryDraft := app.promptHistoryDraft + promptHistoryIndex := app.promptHistoryIndex + app.transcript.History = nil + app.transcript.LineCache.reset() + app.appendSessionMessages(messages) + app.promptHistory = promptHistory + app.promptHistoryDraft = promptHistoryDraft + app.promptHistoryIndex = promptHistoryIndex + } else { app.sessionID = parentSessionID app.pendingParentID = nil app.resetMessages() app.resetStreamingBlocks() if settingsFound { app.applySessionSettings(&settings) } app.appendSessionMessages(messages) }Consider also adding a regression test mirroring
TestRevisitAgentTaskSessionRefreshesDurableTranscriptbut for the leave-to-parent direction.🤖 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/agent_tasks.go` around lines 1631 - 1675, Update leaveAgentTaskSession to refresh the durable parent transcript when restoreSessionView(parentSessionID) succeeds, mirroring the refresh behavior in switchToAgentTaskSession: clear the current transcript history and re-append the freshly loaded messages before restoring transient session state. Preserve the existing fallback behavior and add a regression test covering durable messages added while inspecting the child before returning to the parent.
🧹 Nitpick comments (4)
internal/terminal/agent_tasks_behavior_internal_test.go (1)
1010-1183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared setup for the new inspection/prompt tests.
TestActivePromptInspectionAllowsNestedTaskSelection,TestDoubleEscapeCancelsParentPromptWithoutLeavingInspection,TestAltEscapeLeavesInspectionWithoutCancelingParentPrompt, andTestActivePromptInspectionBlocksGlobalAndExtensionShortcutsall repeat the same ~15 lines of fixture/task/stub/app/activePrompt/inspectAgentTask setup nearly verbatim. Extracting a small helper (e.g. returning the fixture, app, and task with the parent prompt already active and the child already inspected) would cut duplication significantly with minimal effort.As per coding guidelines,
**/*_test.goshould "Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs" — a shared setup helper achieves most of the intended DRY benefit here since the individual assertions are too distinct to naturally tabulate.🤖 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/agent_tasks_behavior_internal_test.go` around lines 1010 - 1183, Extract the duplicated fixture, task, stub, runtime, active-prompt, and inspectAgentTask setup from the four named tests into a focused test helper that returns the fixture and initialized app (and task data if needed). Update each test to call the helper while preserving its existing assertions, cleanup, and test-specific configuration such as nested tasks or composer text.Source: Coding guidelines
internal/terminal/session_view.go (1)
117-145: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftRepeated full clone round-trip on every streamed event while inspecting a different session.
withSessionViewfast-paths only when the target session matches the currently displayed one; wheneverinspectingWhilePromptRuns()is true, every prompt delta/thinking-delta/tool event routed throughhandlePromptAsyncEvent(async_events.go) triggers a fullsaveSessionView+restoreSessionView+saveSessionView+restoreSessionViewcycle, each doing multiplemaps.Clone/slices.Clone/buffer clones. During active token streaming this could run many times per second. Given the project guideline to keep terminal code allocation-conscious, it's worth benchmarking this path before merge, or batching/coalescing the save/restore so it isn't paid per-delta.Separately (minor): the doc comment on lines 117-119 sits directly above
inspectingWhilePromptRuns(no blank line), but its content ("routes an event to its owning session...") documentswithSessionView, notinspectingWhilePromptRuns. Consider moving it directly abovewithSessionView.As per coding guidelines, "Keep the default render path hot and allocation-conscious; do not route default UI through Lua unless explicitly required and benchmarked."
🤖 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/session_view.go` around lines 117 - 145, Reduce allocation overhead in withSessionView for repeated streamed events while inspecting another session by batching or coalescing the saveSessionView/restoreSessionView round-trip, and benchmark the resulting prompt-event path to confirm improvement. Move the existing routing comment directly above withSessionView, leaving inspectingWhilePromptRuns without that unrelated documentation.Source: Coding guidelines
internal/terminal/input.go (2)
103-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated read-only status string into a constant.
Static analysis flags
"agent task inspection is read-only while the parent response runs"duplicated across lines 103, 136, and 260.♻️ Proposed fix
+const readOnlyInspectionStatus = "agent task inspection is read-only while the parent response runs" + func (app *App) handleModalPriorityKey(ctx context.Context, event *tcell.EventKey) keyHandlingResult { ... - app.setStatus("agent task inspection is read-only while the parent response runs") + app.setStatus(readOnlyInspectionStatus) ... }Apply the same substitution at lines 136 and 260.
Also applies to: 136-136, 259-261
🤖 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/input.go` at line 103, Extract the duplicated status text used by app.setStatus into a named package-level constant in the input handling code, then replace the occurrences at the referenced status-update sites, including the calls near lines 103, 136, and 260, with that constant while preserving the exact message.Source: Linters/SAST tools
87-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable read-only re-checks
handlePriorityKey()already consumes everyinspectingWhilePromptRuns()path, so the guards inhandleInlineListsAndExtensionKey()andhandleInputKey()never fire. Fold the read-only status text into one helper to avoid keeping the same branch and message in three places.🤖 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/input.go` around lines 87 - 139, The read-only inspection handling is duplicated and unreachable in downstream key handlers. Remove the redundant inspectingWhilePromptRuns guards and status branches from handleInlineListsAndExtensionKey and handleInputKey, and centralize the shared read-only status behavior in handleReadOnlyInspectionPriorityKey while preserving its existing inspection navigation handling.
🤖 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/async_events.go`:
- Around line 262-273: The return value of withSessionView is ignored, allowing
events to be silently dropped when the owner view is missing. In
internal/terminal/async_events.go lines 262-273, check the result around
handlePromptLifecycleEvent and handlePromptStreamEvent and log or set a status
on failure; in internal/terminal/agent_tasks.go lines 961-963, update
deliverAgentTaskCompletion to surface failure via setStatus; and in
internal/terminal/agent_tasks.go lines 979-981, update
deliverAgentTaskCompletionEvent similarly while preserving its existing
unresolved-owner status handling.</code>
---
Outside diff comments:
In `@internal/terminal/agent_tasks.go`:
- Around line 1631-1675: Update leaveAgentTaskSession to refresh the durable
parent transcript when restoreSessionView(parentSessionID) succeeds, mirroring
the refresh behavior in switchToAgentTaskSession: clear the current transcript
history and re-append the freshly loaded messages before restoring transient
session state. Preserve the existing fallback behavior and add a regression test
covering durable messages added while inspecting the child before returning to
the parent.
---
Nitpick comments:
In `@internal/terminal/agent_tasks_behavior_internal_test.go`:
- Around line 1010-1183: Extract the duplicated fixture, task, stub, runtime,
active-prompt, and inspectAgentTask setup from the four named tests into a
focused test helper that returns the fixture and initialized app (and task data
if needed). Update each test to call the helper while preserving its existing
assertions, cleanup, and test-specific configuration such as nested tasks or
composer text.
In `@internal/terminal/input.go`:
- Line 103: Extract the duplicated status text used by app.setStatus into a
named package-level constant in the input handling code, then replace the
occurrences at the referenced status-update sites, including the calls near
lines 103, 136, and 260, with that constant while preserving the exact message.
- Around line 87-139: The read-only inspection handling is duplicated and
unreachable in downstream key handlers. Remove the redundant
inspectingWhilePromptRuns guards and status branches from
handleInlineListsAndExtensionKey and handleInputKey, and centralize the shared
read-only status behavior in handleReadOnlyInspectionPriorityKey while
preserving its existing inspection navigation handling.
In `@internal/terminal/session_view.go`:
- Around line 117-145: Reduce allocation overhead in withSessionView for
repeated streamed events while inspecting another session by batching or
coalescing the saveSessionView/restoreSessionView round-trip, and benchmark the
resulting prompt-event path to confirm improvement. Move the existing routing
comment directly above withSessionView, leaving inspectingWhilePromptRuns
without that unrelated documentation.
🪄 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 Plus
Run ID: b149d34b-35d3-4d4d-a864-04f4ddad9aff
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
internal/terminal/agent_tasks.gointernal/terminal/agent_tasks_behavior_internal_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/input.gointernal/terminal/input_escape.gointernal/terminal/message_layout.gointernal/terminal/session_view.gointernal/terminal/session_view_internal_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/input.go (1)
226-235: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude Alt+Escape from autocomplete dismissal. This helper runs before
handleAgentTaskSessionEscape, so when autocomplete is open Alt+Escape closes autocomplete instead of leaving inspection. Skiptcell.ModAlthere (or reorder the checks), and add ahandleKeyregression test.🤖 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/input.go` around lines 226 - 235, The handleInspectionAutocompleteEscape method should not dismiss autocomplete for Alt+Escape, allowing handleAgentTaskSessionEscape to leave inspection instead. Add a modifier check for tcell.ModAlt while preserving existing escape handling, and add a handleKey regression test covering Alt+Escape with autocomplete active.
🤖 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/input.go`:
- Around line 226-235: The handleInspectionAutocompleteEscape method should not
dismiss autocomplete for Alt+Escape, allowing handleAgentTaskSessionEscape to
leave inspection instead. Add a modifier check for tcell.ModAlt while preserving
existing escape handling, and add a handleKey regression test covering
Alt+Escape with autocomplete active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 72b8eee4-efe0-401f-adbf-fc236a43347c
📒 Files selected for processing (7)
internal/terminal/agent_tasks.gointernal/terminal/agent_tasks_behavior_internal_test.gointernal/terminal/async_events.gointernal/terminal/bench_terminal_internal_test.gointernal/terminal/input.gointernal/terminal/session_view.gointernal/terminal/session_view_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/terminal/session_view_internal_test.go
- internal/terminal/async_events.go
- internal/terminal/session_view.go
- internal/terminal/agent_tasks.go
- internal/terminal/agent_tasks_behavior_internal_test.go
1 participant
No description provided.