feat(tui): clickable plan-step detail + plan/sidebar UX & rendering polish - #315
Merged
Conversation
The plan panel could read as stuck mid-task even while work continued. Three fixes: - Header showed steps[0] forever, so the title never advanced as the plan progressed; show the current step (in_progress, else first incomplete). - updateFromItems matched steps by content only, so an in-place reword reset a step's elapsed timer to zero; add positional carry-over when the step count is unchanged. - Prompt: tell the model to mark each step completed + the next in_progress before starting it, so the plan tracks real progress instead of jumping at the end. Regression tests included.
Each plan step now records the file mutations (write_file/edit_file/apply_patch) made while it was in_progress, captured from tool-result rows. Clicking a step row in the context sidebar drops a transcript card listing those changes — the implementation for that step. Step->line mapping mirrors sidebarAgentSelectables' offset accounting (one line per step); capture is keyed to the active step and cleared per run. Read-only and additive. Tests cover the offset math + capture.
Extend the clickable plan-step detail: capture bash/exec_command runs in addition to file mutations, store each tool's full output (the diff for edit/apply_patch, stdout/stderr for commands), and render the card grouped into Changes and Commands with a short truncated excerpt of each item's diff/output.
Re-clicking a plan step now hides its card (a stable transcript id + drop-by-id), and clicking a different step replaces it, so at most one detail card is shown. Previously each click appended a fresh card and they piled up.
Clicking a PLAN step now shows a fresh, plain-English write-up of that
step, generated on demand by the active model:
- past tense ('what we did') for completed/failed steps, future tense
('what we'll do') for pending/in_progress steps
- immediate feedback: the card drops in showing 'Writing explanation…'
and updates in place (by stable row id) when the text returns
- cached per step (content+status) so re-clicking is instant with no
second model call; re-clicking still toggles the card closed
- the prompt is built from already-captured local data (step text,
status, notes, plus a compact digest of file edits, commands, and the
agent's narration) and asks for short, non-technical prose
- reuses the TUI's existing provider via a one-shot StreamCompletion;
no new client, no hardcoded provider. Falls back to the local summary
when no provider is configured or the request fails.
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughThe PR strengthens agent prompt plan-tracking rules, adds a configurable post-turn recap feature (persisted via ChangesPlan workflow, recaps, tool previews, and TUI transcript updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/plan_panel.go (1)
94-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear stale completion timestamps for non-terminal carried-over steps.
The positional fallback can copy
completedAtfrom a prior completed/failed step into a newpendingorin_progressstep. Line 100 then keeps that stale terminal timestamp, so the live step can render with an old completed duration instead of a running clock.Proposed fix
switch step.status { case "in_progress": + step.completedAt = time.Time{} if step.startedAt.IsZero() { step.startedAt = now } case "completed", "failed": if step.startedAt.IsZero() { step.startedAt = now } if step.completedAt.IsZero() { step.completedAt = now } + default: + step.completedAt = time.Time{} }🤖 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/tui/plan_panel.go` around lines 94 - 103, The carried-over step matching in plan_panel.go can copy a stale completedAt from a previous terminal step into a current pending or in_progress step, causing live steps to show an old finished duration. Update the step reconciliation logic around the positional fallback and switch on step.status so that non-terminal statuses clear any inherited completedAt (and any other terminal-only timing state) before rendering, while still preserving startedAt when appropriate. Use the existing step matching block and the status handling in the plan panel logic to locate the fix.
🤖 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/tui/model.go`:
- Around line 1620-1633: Drop stale plan-step explanation results by associating
each planStepExplanationMsg with the current run/detail generation and ignoring
any message whose generation does not match the active one. Update the message
handling around planStepExplanationMsg in internal/tui/model.go and the sender
path that triggers the async explanation request so the generation is captured
when the request starts, then validate it before recreating stepExplanation or
storing text. Ensure beginRun still resets state, but stale in-flight results
from a previous run cannot repopulate m.stepExplanation or overwrite newer data.
- Around line 1084-1090: The Ctrl+B toggle in the no-blocking modal path is
using sidebarAvailable() in a way that lets an auto-hidden sidebar become
manually hidden, which can keep the later plan view suppressed. Update the
toggle logic in model.go around m.noBlockingModal and m.sidebarHidden so Ctrl+B
only flips the user-controlled hidden state when the sidebar is actually
available to show, and separate that from the auto-hidden case; keep the change
localized to the sidebar visibility handling and any checks that determine
whether the pinned plan should render.
In `@internal/tui/plan_step_detail.go`:
- Around line 459-473: The digest truncation in planStepDigestItems is using
byte slicing on w.summary, which can break UTF-8 when non-ASCII text is present.
Update the truncation logic to use the existing rune-safe helper already used in
this package, such as truncateRunes, and keep its ellipsis behavior consistent
with the current summary formatting.
---
Outside diff comments:
In `@internal/tui/plan_panel.go`:
- Around line 94-103: The carried-over step matching in plan_panel.go can copy a
stale completedAt from a previous terminal step into a current pending or
in_progress step, causing live steps to show an old finished duration. Update
the step reconciliation logic around the positional fallback and switch on
step.status so that non-terminal statuses clear any inherited completedAt (and
any other terminal-only timing state) before rendering, while still preserving
startedAt when appropriate. Use the existing step matching block and the status
handling in the plan panel logic to locate the fix.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79bae045-1538-4431-837c-13649dc2bf0b
📒 Files selected for processing (18)
internal/agent/system_prompt.mdinternal/tui/command_output.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/mouse_test.gointernal/tui/plan_panel.gointernal/tui/plan_panel_progress_test.gointernal/tui/plan_panel_test.gointernal/tui/plan_step_detail.gointernal/tui/plan_step_detail_test.gointernal/tui/rendering.gointernal/tui/sidebar.gointernal/tui/sidebar_test.gointernal/tui/stage10_test.gointernal/tui/streaming_fade.gointernal/tui/tool_render_registry.gointernal/tui/transcript_selection.gointernal/tui/view.go
💤 Files with no reviewable changes (2)
- internal/tui/stage10_test.go
- internal/tui/streaming_fade.go
…lan progress, narration prompt, write_file line counts)
…ol-call streaming
…planation, Ctrl+B persistence, UTF-8 truncation
Collaborator
Author
|
Addressed all review comments (with tests):
This push also brings the rest of the branch's work (narration bullet, recap line + @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/model.go (1)
1340-1346: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh the quiet-generation timer on reasoning deltas.
Lines 1344-1346 advance the live token count but never touch
lastStreamActivity, soquietGenerationHint()can flip to “still generating…” while reasoning is actively streaming. That makes the new liveness hint contradict the actual provider activity.Suggested fix
case agentReasoningMsg: if msg.runID != m.activeRunID { return m, nil } m.streamingReasoning += msg.delta m.turnStreamedRunes += utf8.RuneCountInString(msg.delta) + if msg.delta != "" { + m.lastStreamActivity = m.now() + } return m, 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/tui/model.go` around lines 1340 - 1346, The quiet-generation state is not being refreshed for reasoning-only streaming updates, so `quietGenerationHint()` can report “still generating…” even while `agentReasoningMsg` deltas are arriving. In the `model.go` reasoning stream handling, update `lastStreamActivity` alongside the existing `streamingReasoning` and `turnStreamedRunes` updates whenever `msg.runID` matches `m.activeRunID`, so the liveness hint stays aligned with active provider output.
🧹 Nitpick comments (2)
internal/tui/sidebar.go (1)
719-744: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftThis ACTIVITY lookup is still linear in transcript length.
len(work) < limitcaps matches, not rows inspected. If matching tool-result rows are sparse, every sidebar render can still walk most ofm.transcript. In this hot path, consider keeping a bounded activity deque/index as rows are appended instead of rescanning on render.🤖 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/tui/sidebar.go` around lines 719 - 744, The activity summary lookup in sidebar rendering is still scanning m.transcript linearly, which can make every render expensive when matching rows are sparse. Update the sidebar state around the transcript appends and the activity rendering path so recent plan-work tool results are kept in a bounded deque or index as rows arrive, instead of recomputing them inside the render loop. Use the existing helpers and symbols in sidebar.go such as m.transcript, rowToolResult, isPlanWorkTool, m.activitySummary, and the code that builds work/live lines to locate and replace the hot-path scan.internal/tui/command_polish_test.go (1)
78-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the recap value, not just the label.
This only proves the
/configoutput mentionsrecaps:. It will still pass if the runtime default oronOff(m.recapsEnabled)rendering regresses. Set the flag explicitly in the test model and assertrecaps: onorrecaps: offexactly.🤖 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/tui/command_polish_test.go` around lines 78 - 80, The /config test in command_polish_test is only checking that the recaps label appears, so it won’t catch regressions in the rendered value. Update the test model to set the recaps flag explicitly, then assert the exact output from commandPolish and onOff(m.recapsEnabled) by checking for either recaps: on or recaps: off, not just the presence of recaps:.
🤖 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/config/writer_test.go`:
- Around line 300-303: The round-trip test in writer_test.go is ignoring the
error from SetRecapsEnabled when turning recaps back on, which can hide a failed
write and still pass on a zero-value PreferencesConfig. Update the test around
SetRecapsEnabled and RecapsEnabled to assert the error is nil for the re-enable
call, then verify the returned config reflects a persisted true value so the
test only passes when the second write actually succeeds.
In `@internal/tools/apply_patch.go`:
- Line 103: The apply_patch preview is still rendering raw patch content, so
secrets can leak through Display.Preview. Update applyPatch’s preview generation
to use the same redaction flow as the other preview renderers instead of passing
patch directly into capPreviewDiff, and make sure the Display field in
apply_patch.go uses the redacted preview path. Add test coverage for
secret-containing hunks to verify the preview is redacted before being stored or
rendered.
In `@internal/tools/preview.go`:
- Around line 33-35: The preview hunk rendering in preview.go is losing real
trailing and empty lines because strings.TrimRight(..., "\n") is applied before
splitting, which also turns empty content into a fake blank diff line. Update
the logic in the preview hunk builder paths that use this pattern, including the
code around the content-to-body loop and the edit/delete hunk rendering, so line
splitting preserves trailing blank lines and empty content exactly. Keep the
hunk construction in the relevant functions (such as the preview body assembly
for write_file and edit_file) aligned with the original content shape rather
than trimming newline endings first.
In `@internal/tui/recap.go`:
- Around line 80-87: The recap flow in generateRecapCmd and the recap completion
handling is appending results to the current end of the transcript without
checking whether they belong to the latest run, so stale recap messages can land
on the wrong conversation. Fix this by anchoring each recap request to a stable
insertion point or by validating the runID before applying the result, and
update the handler that processes recapGeneratedMsg so it inserts at the
captured position or drops outdated results instead of always appending.
- Around line 124-131: The `recaps on` and `recaps off` branches in
`recap`/`handleRecaps` are returning success even when `persistRecapsEnabled()`
fails. Update these cases to check the error from `persistRecapsEnabled()` and
only return `Config\nrecaps: on/off` when persistence succeeds; on failure,
return an error or failure message instead of success. Keep the in-memory toggle
and persisted state aligned by handling the write failure explicitly in the same
control flow.
In `@internal/tui/rendering.go`:
- Around line 143-144: The row filtering in renderRow/isHiddenPlumbingTool is
hiding all plumbing tool results, including failed ones like StatusError for
update_plan and tool_search. Adjust the rowToolResult handling so only
successful plumbing outputs are suppressed, while error/failure results still
render. Use the rowToolResult switch case and isHiddenPlumbingTool helper to
locate the logic, and preserve visibility for any failed deferred-tool or plan
updates.
In `@internal/tui/sidebar.go`:
- Around line 752-754: The ACTIVITY summary is matching tool-call rows only by
id, which can pick up the wrong command when the same id is reused later in the
transcript. Update activityCommandForRow to scope the lookup by both id and
runID, and make the callers that build the ACTIVITY label in sidebar.go use that
run-aware match so the summary stays tied to the same run. Use the existing
rowToolCall data and the row.runID from the current row to keep the association
local.
---
Outside diff comments:
In `@internal/tui/model.go`:
- Around line 1340-1346: The quiet-generation state is not being refreshed for
reasoning-only streaming updates, so `quietGenerationHint()` can report “still
generating…” even while `agentReasoningMsg` deltas are arriving. In the
`model.go` reasoning stream handling, update `lastStreamActivity` alongside the
existing `streamingReasoning` and `turnStreamedRunes` updates whenever
`msg.runID` matches `m.activeRunID`, so the liveness hint stays aligned with
active provider output.
---
Nitpick comments:
In `@internal/tui/command_polish_test.go`:
- Around line 78-80: The /config test in command_polish_test is only checking
that the recaps label appears, so it won’t catch regressions in the rendered
value. Update the test model to set the recaps flag explicitly, then assert the
exact output from commandPolish and onOff(m.recapsEnabled) by checking for
either recaps: on or recaps: off, not just the presence of recaps:.
In `@internal/tui/sidebar.go`:
- Around line 719-744: The activity summary lookup in sidebar rendering is still
scanning m.transcript linearly, which can make every render expensive when
matching rows are sparse. Update the sidebar state around the transcript appends
and the activity rendering path so recent plan-work tool results are kept in a
bounded deque or index as rows arrive, instead of recomputing them inside the
render loop. Use the existing helpers and symbols in sidebar.go such as
m.transcript, rowToolResult, isPlanWorkTool, m.activitySummary, and the code
that builds work/live lines to locate and replace the hot-path scan.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af6b47d2-3568-4831-a40a-618674c8bf48
📒 Files selected for processing (34)
internal/agent/system_prompt.mdinternal/agent/system_prompt_test.gointernal/cli/app.gointernal/config/resolver.gointernal/config/types.gointernal/config/writer.gointernal/config/writer_test.gointernal/tools/apply_patch.gointernal/tools/edit_file.gointernal/tools/preview.gointernal/tools/preview_test.gointernal/tools/registry.gointernal/tools/types.gointernal/tools/write_file.gointernal/tools/write_tools_test.gointernal/tui/command_polish_test.gointernal/tui/command_views.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/options.gointernal/tui/plan_panel.gointernal/tui/plan_panel_progress_test.gointernal/tui/plan_step_detail.gointernal/tui/recap.gointernal/tui/recap_test.gointernal/tui/rendering.gointernal/tui/rendering_lime_test.gointernal/tui/sidebar.gointernal/tui/sidebar_test.gointernal/tui/tool_render_registry.gointernal/tui/transcript.gointernal/tui/transcript_selection.gointernal/tui/working_status_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/agent/system_prompt_test.go
- internal/tui/commands.go
- internal/tools/preview_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/tui/sidebar_test.go
- internal/tui/plan_step_detail.go
- internal/tui/plan_panel.go
- internal/tui/model_test.go
Collaborator
Author
|
Round 2 — addressed all 7 actionable comments + both nitpicks (each with a test):
Nitpicks:
Note on apply_patch (Display.Preview redaction): the preview is already scrubbed at the universal boundary — All suites (tui/tools/config/agent) green, gofmt clean. @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/cli/dryrun_test.go (1)
132-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up stale/contradictory test comment text.
The comment mentions
--dry-run=fooand “bad value” behavior, but the test isTestDryRunWithExplicitTrue.🤖 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/cli/dryrun_test.go` around lines 132 - 135, The test comment in TestDryRunWithExplicitTrue is stale and contradicts the test name and current boolean flag behavior. Update the comment above TestDryRunWithExplicitTrue in dryrun_test.go to describe the actual scenario being verified, remove references to “bad value” and --dry-run=foo, and make sure the wording matches the boolean --dry-run/--dry-run=true/false behavior reflected by the test.
🤖 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/cli/dryrun_test.go`:
- Around line 227-233: The dry-run tests only check for “Would” in the output
and can miss regressions that still write to disk. Update
TestDryRunSandboxGrantSetPrintsWould and TestDryRunHooksAddPrintsWould in
dryrun_test.go to also assert the expected file is not created or modified,
using the relevant dry-run helpers and output checks already in place. Keep the
existing stdout/stderr assertions, but add a file non-existence/no-write side
effect check for the target path used by each test.
- Line 186: The dry-run assertion is referencing a field that does not exist on
the launched agent options, so the test does not compile. Update the assertion
in the runWithDeps test to check the actual dry-run field carried through
tui.Options/agent.Options instead of launchedOptions.AgentOptions.DryRun, and
use the concrete options symbol that runWithDeps populates so the test matches
the real structure.
- Line 15: The dry-run test file has an unused zeroruntime import that causes go
test to fail typecheck; remove the unused import from the test module and keep
the remaining imports in internal/cli/dryrun_test.go aligned with actual
references in the test code.
- Around line 222-223: The dry-run test setup references sandbox symbols that
are not currently in scope, so the test won’t compile. Add the missing sandbox
package import in the test file that defines newSandboxStore, then keep the
existing uses of sandbox.GrantStore and sandbox.NewGrantStore(...) unchanged so
the closure can resolve those symbols correctly.
---
Nitpick comments:
In `@internal/cli/dryrun_test.go`:
- Around line 132-135: The test comment in TestDryRunWithExplicitTrue is stale
and contradicts the test name and current boolean flag behavior. Update the
comment above TestDryRunWithExplicitTrue in dryrun_test.go to describe the
actual scenario being verified, remove references to “bad value” and
--dry-run=foo, and make sure the wording matches the boolean
--dry-run/--dry-run=true/false behavior reflected by the test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8297c1d2-d5d7-4b09-8a51-421de54cf2bb
📒 Files selected for processing (14)
internal/cli/dryrun_test.gointernal/config/writer_test.gointernal/reltime/reltime.gointernal/tools/preview.gointernal/tools/preview_test.gointernal/tools/registry_test.gointernal/tui/command_polish_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/recap.gointernal/tui/recap_test.gointernal/tui/rendering.gointernal/tui/sidebar.gointernal/tui/working_status_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/tui/command_polish_test.go
- internal/tools/preview_test.go
- internal/config/writer_test.go
- internal/tui/working_status_test.go
- internal/tui/recap.go
- internal/tui/sidebar.go
- internal/tui/model.go
- internal/tui/rendering.go
It is an unrelated, pre-existing dry-run test that does not compile (references a non-existent agent.Options.DryRun field, an unused import, and out-of-scope sandbox symbols). It was swept into the prior commit by 'git add -A' and is not part of this PR's scope.
Collaborator
Author
|
All four comments were on It was never meant to be part of this PR — it got swept in by a @coderabbitai review |
|
I’ll review the current diff as it stands, focusing on the intended TUI/plan/recap/sidebar changes and ignoring that removed dry-run test work for this PR. ✅ Action performedReview finished.
|
Reconcile the overlapping write/edit code-preview feature with main's #318: keep main's robust library-based boundedUnifiedDiff (proper red/green for create/overwrite/edit) but route it through the card-only Display.Preview so the model's Output stays the one-line summary — the rich preview costs zero model tokens. Drop the now-redundant hand-rolled diff builders; keep the 'N lines' summary. apply_patch keeps capPreviewDiff.
Reconcile the narration guidance: keep #317's plain-language preamble lead AND this branch's per-significant-step running narration (the story flow), with both sides' 'don't narrate trivial calls / skip for one-step tasks'.
Collaborator
Author
|
@CodeRabbit review |
Collaborator
Author
|
@CodeRabbit review again |
|
✅ Action performedFull review finished. |
Collaborator
Author
|
@CodeRabbit approvable ? |
|
Tip For best results, initiate chat on the files or code changes.
The previously raised correctness issues appear addressed, the accidental I don’t see a remaining blocking issue in the intended scope of this PR. If you want me to actually approve it, post |
Collaborator
Author
|
@CodeRabbit if approvable then approve it |
Collaborator
Author
|
@CodeRabbit approve |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/tools/preview_test.go (1)
8-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTidy test coverage, one small gap. The bound check uses a bare
previewBodyLines+5slack with no comment — fine, but it silently tolerates up to 5 extra lines (header/trailer). A quick comment on why 5 keeps future maintainers from guessing. Also consider asserting the cap retains the diff header lines (--- a/x/+++ b/x) so a regression that drops context headers doesn't slip through.🤖 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/tools/preview_test.go` around lines 8 - 21, Update TestCapPreviewDiff to make the bounded-line assertion self-explanatory by adding a brief note around the previewBodyLines+5 allowance, and strengthen the test by explicitly checking that capPreviewDiff preserves the diff header lines in the long case. Use the existing TestCapPreviewDiff and capPreviewDiff symbols to keep the intent tied to the current coverage.internal/tui/plan_panel.go (1)
378-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale doc comment on
renderPlanHeader. The function now titles the header with the current step (in-progress → first incomplete → first), but the doc comment at Lines 378-381 still says it shows "the truncated first step." Tidy it so the next reader isn't misled.📝 Comment tweak
// renderPlanHeader builds the single header line. While running it shows the -// live spinner, the truncated first step, the done/total count, and the -// elapsed time in the accent color; once complete it shows a green check and -// "PLAN COMPLETE". +// live spinner, the truncated current step (in_progress, else first incomplete, +// else first), the done/total count, and the elapsed time in the accent color; +// once complete it shows a green check and "PLAN COMPLETE".🤖 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/tui/plan_panel.go` around lines 378 - 393, Update the doc comment for renderPlanHeader so it matches the current behavior: the header now shows the current step via currentStepContent(state.steps), not always the truncated first step. Keep the comment aligned with the function’s completed-state behavior (“PLAN COMPLETE”) and running-state fields (spinner, current step, done/total, elapsed time) so future readers aren’t misled.
🤖 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/reltime/reltime.go`:
- Around line 5-65: RelTime is currently unused dead code, so either hook it up
in the relative-time rendering path (such as the plan/sidebar renderer that
should consume relative timestamps) or remove the RelTime function for now. Use
the unique symbol RelTime in internal/reltime/reltime.go to locate it, and
ensure the chosen path is wired into whatever formatter or UI code will actually
call it.
---
Nitpick comments:
In `@internal/tools/preview_test.go`:
- Around line 8-21: Update TestCapPreviewDiff to make the bounded-line assertion
self-explanatory by adding a brief note around the previewBodyLines+5 allowance,
and strengthen the test by explicitly checking that capPreviewDiff preserves the
diff header lines in the long case. Use the existing TestCapPreviewDiff and
capPreviewDiff symbols to keep the intent tied to the current coverage.
In `@internal/tui/plan_panel.go`:
- Around line 378-393: Update the doc comment for renderPlanHeader so it matches
the current behavior: the header now shows the current step via
currentStepContent(state.steps), not always the truncated first step. Keep the
comment aligned with the function’s completed-state behavior (“PLAN COMPLETE”)
and running-state fields (spinner, current step, done/total, elapsed time) so
future readers aren’t misled.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ef56864-f98f-431e-bc99-037c86ad1a5b
📒 Files selected for processing (43)
internal/agent/system_prompt.mdinternal/agent/system_prompt_test.gointernal/cli/app.gointernal/config/resolver.gointernal/config/types.gointernal/config/writer.gointernal/config/writer_test.gointernal/reltime/reltime.gointernal/tools/apply_patch.gointernal/tools/edit_file.gointernal/tools/preview.gointernal/tools/preview_test.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/types.gointernal/tools/write_file.gointernal/tools/write_tools_test.gointernal/tui/command_output.gointernal/tui/command_polish_test.gointernal/tui/command_views.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/mouse_test.gointernal/tui/options.gointernal/tui/plan_panel.gointernal/tui/plan_panel_progress_test.gointernal/tui/plan_panel_test.gointernal/tui/plan_step_detail.gointernal/tui/plan_step_detail_test.gointernal/tui/recap.gointernal/tui/recap_test.gointernal/tui/rendering.gointernal/tui/rendering_lime_test.gointernal/tui/sidebar.gointernal/tui/sidebar_test.gointernal/tui/stage10_test.gointernal/tui/streaming_fade.gointernal/tui/tool_render_registry.gointernal/tui/transcript.gointernal/tui/transcript_selection.gointernal/tui/view.gointernal/tui/working_status_test.go
💤 Files with no reviewable changes (2)
- internal/tui/stage10_test.go
- internal/tui/streaming_fade.go
|
✅ Action performedComments resolved and changes approved. |
✅ Action performedComments resolved and changes approved. |
This was referenced Jul 14, 2026
This was referenced Aug 1, 2026
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
1 participant
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
A focused set of TUI improvements centered on the plan panel and context sidebar, plus several rendering-polish fixes. The headline feature makes plan steps clickable to surface a plain-English explanation of each step; the rest tightens correctness (a finished task now shows its plan as complete) and removes visual noise (a glow animation and a shaded card band).
What's in this PR
Clickable plan-step detail (new feature)
Plan panel correctness
update_plan, the panel now reconciles the remaining steps to complete (so it shows "PLAN COMPLETE" instead of freezing mid-progress). Guarded to fire only on a genuine successful turn — never on an error, cancellation, or a mid-plan pause (ask-user / permission / spec-review). A legitimately failed step keeps its failed status.Context sidebar UX
Ctrl+Btoggle: hiding/showing the sidebar no longer prints a "Context sidebar hidden/shown" line into the transcript on every toggle.Ctrl+Bhides the plan entirely: collapsing the sidebar no longer resurrects the plan as a pinned panel above the composer. The pinned panel is now reserved for terminals too narrow to host the sidebar at all.Rendering polish
panelbackground while preserving exact cell widths and the status-tinted left rail; unrelated panel surfaces (sidebar, sessions list, ask-user card) are untouched.Testing
Ctrl+B-hides-plan behavior.go build ./...clean; the fullinternal/tuisuite passes.Summary by CodeRabbit
New Features
Bug Fixes