Feat/tui2 phase3 parity - #187
Conversation
…wup, search, verbose - Strip single-char hotkeys (?, /, j, k, g, G) — Ctrl+ only per design decision - Add :help, :search, :top, :bottom, :verbose, :stop, :steer, :banner commands - Fix :compact to call OnCompact → sess.Compact() (was CollapseAll bug) - Wire OnSteer/OnFollowUp/OnStop callbacks in agent frame - Enter-while-streaming → follow-up (submitFollowUp) - :steer <text> for mid-turn injection - :stop to abort running agent + hide thinking - :search <query> scrolls to first match + ephemeral result - :verbose toggles verbose mode - :banner toggles banner visibility - SetEphemeral() displays temporary messages in info pane (3s timeout) - Update plan doc for Phase 3 progress
- Stop passing viewport width to tviewmd (was causing table overflow + garbled wrapping). tviewmd only needs Width for table column math, not word wrap — tview's SetWrap handles that. - Command palette: use centered bordered modal (same pattern as old help component) instead of full-screen overlay. Escape now closes the palette. - Help modal: rebuilt with same centered pattern, lists all keybindings + commands.
- Revert raw markdown storage (Phase 2.4): convItem.text now stores
rendered output, not raw markdown. renderMarkdown() is called once
at flush time, not on every 200ms refresh.
- Fixes garbled/interleaved output during streaming: the old code
re-rendered ALL accumulated markdown every 200ms via the spinner
ticker, producing corrupted output as partial markdown changed shape.
- Add border + title to messages TextView ('Conversation').
- Pass real version string to tui2.New(version) for info pane display.
- Command palette uses centered bordered modal (same as old help.Show).
- Help modal lists all keybindings + command palette commands.
Store raw text (markdown or plain) in convItem.text with isMarkdown flag. Render lazily in refreshMessages() — markdown rendered once and cached by viewport width. Cache invalidates on width change (resize reflow). Plain text (user messages, system notices) used directly. Fixes garbled output by avoiding re-render on every 200ms tick.
- CRITICAL: Fix data race on subagentBlocks — move iteration inside QueueUpdateDraw - CRITICAL: Fix broken style tags in messages/* sub-packages (raw Dim/Accent without brackets) - HIGH: Move spinner to info bar (updateInfoBar); ticker no longer calls refreshMessages - HIGH: Throttled refresh from TokenDeltaEvent (150ms) instead of blind 200ms timer - HIGH: Fix modelpicker focus restore — pass focusAfter param, call app.SetFocus on dismiss - HIGH: Fix approval focus restore — app.SetFocus(modal) on show - MEDIUM: All overlay pages now use resize=false (tview best practice for modals) - Cache renderMarkdown output by convItem width; only re-render on width change
Group fields into three documented sections: - Set before Run() — tview infrastructure, theme, version - QueueUpdateDraw ONLY — conversationLog, subagentBlocks, pendingTokens, toolBlocks, reasoningBlocks, lastRefresh, ephemeralMsg, and all other mutable state - Atomic — isStreaming Clear wall comment marks the boundary to prevent future data races.
Flex wrappers with spacer items need resize=true to fill the screen and center content. resize=false only works for tview.NewModal() (approval). Reverted for command palette, help, question, and modelpicker overlays.
- Remove renderMarkdown calls: raw text flows directly to TextView - Remove convItem cached/cachedWidth fields (no longer needed) - Remove TokenDeltaEvent throttle (ticker handles refresh timing) - Restore refreshMessages() in spinner ticker with isStreaming gate - Keep messageWidth() for RenderCtx.Width (component borders still sized) - If garbling persists without tviewmd, the issue is in tview/TextView layer
Streaming tokens now start one blank line below the last conversation block instead of appearing directly on the next line without spacing.
- Arrow-navigable list of all commands with descriptions - Enter to execute, Escape to dismiss - Same centered modal pattern as help - Removes old InputField-based palette (single-line, unreadable)
…tText() Root cause confirmed: SetText() on the full conversation buffer every 200ms corrupts tview's TextView rendering, even with raw text (no tviewmd). The fix: - TokenDeltaEvent: Write() appends raw token text directly to TextView - Ticker: skip refreshMessages() during streaming (no SetText rebuild) - Flush boundaries: flushPendingTokens + refreshMessages replaces Write'd tokens with properly ordered conversation entries via SetText - Streaming text now appears in real-time without 200ms rebuild cycle
The reviewer trace showed 114 powershell calls vs 0 read calls, causing context bloat, crippling prune overhead (25-32s), and eventual timeout at 50 iterations. Added explicit tool-selection guidance: prefer dedicated file tools (read/grep/glob/ls/file_info) over shell subprocesses for efficiency and context hygiene.
…ruption Root cause of persistent garbled output: raw markdown text contains [...] patterns (links, code references, bracketed terms) that tview's SetDynamicColors parser consumes as color/style directives, silently swallowing or miscoloring surrounding text. Fix: escapeBrackets() converts '[' to '[[]' (tview literal-escape sequence) before storing text in convItem or writing to TextView. Applied to: - addAssistantResponse: escaped before convItem storage - TokenDeltaEvent Write(): escaped before streaming to TextView
…ors bracket parsing
The real fix for garbled text, confirmed by tui1's approach: render
markdown BEFORE it reaches the TextView. tviewmd produces valid tview
color tags — no raw [...] brackets survive to be consumed as directives.
- Restore renderMarkdown with width-based convItem caching
- Streaming: Write() raw tokens with bracket escaping (safe during stream)
- Flushed: renderMarkdown via cache, re-render on width change (resize reflow)
- Keep Write()/SetText() streaming fix (no 200ms rebuild during stream)
This matches tui1's Message{Content: renderMarkdown(raw), Raw: raw} pattern.
Borders on the messages pane were eating inner-rect space, causing renderMarkdown to compute wrong width for table column math. Tables overflowed, tview wrapping broke column-aligned text mid-word. Removing borders eliminates the width miscalculation. Also removed dead placeholder text from infopane.Build() and unused imports in infopane/todo packages.
…urce Remove ::b and ::d modifiers from mdTheme to determine if underline comes from theme styles or tviewmd's internal heading renderer. Also remove leftover escapeBrackets from streaming Write() path.
The indicator used colors.Dim (#5f5f5f::d — dark gray faint) which was nearly invisible on dark backgrounds. Removed the color wrapper so the lolcat rainbow text renders at default brightness, matching tui1's visible thinking indicator.
Pass the full ' ⠋ Thinking...' string through lolcat.Rainbow instead of only the label. Matches tui1's lolcatRender() which colors the entire line including the spinner frame.
The pre-token spinner now shows 'Thinking...' (matching tui1). Reasoning blocks still show 'Reasoning...' for actual reasoning content. This gives the same dual-indicator pattern: Thinking for the wait phase, Reasoning for the analysis phase.
…ttern Replace string += with strings.Builder for streaming token accumulation. Tui1 uses strings.Builder for streamContent; the += pattern creates O(n^2) allocations that may cause memory fragmentation under rapid token arrival. Also restore renderMarkdown for flushed content.
TokenDeltaEvent uses broker.Publish() which silently drops events when the subscriber channel (buffer 4096) is full. Without Write(), tokens accumulate until flush, clogging the channel. Once full, new tokens are dropped — causing character loss in the final response text that gets worse over longer conversations. Write() drains the broker channel via fast terminal append, preventing the buffer from filling up. The slow flush/SetText path still handles the proper rendering on flush boundaries.
Instrument TokenDeltaEvent handler (tui2.token) and refreshMessages (tui2.refresh) with OTel spans. Token span carries the text attribute so we can see exact token content in SigNoz. Also add atomic counters (tokensRx, charsWritten, charsRendered) for future diagnostic display.
QueueUpdateDraw triggers ForceDraw() after every callback, which redraws the entire screen. During streaming (~100 tokens/sec), this floods the draw pipeline and the 100-slot update queue backs up, blocking HandleEvent, blocking the broker subscriber, causing silent event drops (→ garbled text). QueueUpdate appends to the buffer without ForceDraw. The ticker (200ms) handles screen redraws. Reduces draw pressure from ~100/sec to ~5/sec, preventing queue contention and broker drops.
…ounters Per-token OTel spans create 100+ allocations/sec during streaming. Replaced with zero-overhead atomic counters: tokensRx, charsWritten, charsRendered. Counters exposed as span attributes on tui2.refresh (fires at most 200ms). charsRendered tracked in refreshMessages.
Your edits in d802e8b removed the explicit 'Avoid powershell and bash for file reading' guidance. Without it, reviewers defaulted to shell commands — 75 powershell calls vs 20 reads in the 'Core Framework' reviewer. Re-adding the guidance that was dropped.
Shell tools are the root cause of every reviewer timeout — reviewers use them for file reading (75 powershell calls vs 20 reads in latest trace) causing context bloat and prune overhead. No dedicated tools missing: read/grep/glob for file inspection, git/diff for history, staticcheck for analysis, go_outline for structure. webfetch/http also removed as irrelevant to code review.
Add rx/wr/rn counters to the status bar during streaming: tokens received (rx), characters written via Write() (wr), and characters in the last full render (rn). If rx >> wr, the broker is dropping events. SigNoz flat export drops custom span attributes, so status bar display is the reliable path for live diagnosis.
Creating a standalone trace (context.Background) every 200ms produces 5 new traces/second — wasted overhead. The status bar diagnostic counters (rx/wr/rn) give better live visibility with zero allocation.
Status bar is only updated on DoneEvent — invisible during streaming. Info pane refreshes every 200ms via the ticker during active streaming, so the rx/wr/rn counters are always visible. Also removes unused UpdateDiagnostic from statusbar package.
One tui2.flush span per flush boundary (at most ~10/response, not per tick). Captures tokens_rx, chars_written, chars_rendered, and pending_len. Compare: if chars_written < tokens_rx, broker drops. If chars_rendered < pending_len, render pipeline loses text. Not in hot path — fires only at flush, not 5/sec like the removed refresh span.
guardContextBeforeCall fired compactContext on every turn regardless of pipeline config. CompactionMiddleware.PrepareStep already handles this for loops with the middleware pipeline. Subagents have nil middleware, so compaction is now properly disabled — reviewers won't lose their working memory mid-analysis from unwarranted compaction. This replaces the NoCompaction field approach: instead of adding a new flag to suppress the duplicate, we simply remove the duplicate and let the middleware pipeline own compaction end-to-end.
Streaming tokens are raw markdown containing [...] patterns that tview's SetDynamicColors consumes as color directives, causing text to disappear during streaming. The flush replaces everything with renderMarkdown output (valid tview tags), so brackets only need escaping in the Write() path. Final rendered output is unaffected.
… turns SendHeartbeat fires only at loop iteration boundaries. A hung LLM call (>60s, zero tokens) kills the subagent via stuck_child_timeout despite the subagent being actively waiting for a response. Now the stream handler emits throttled heartbeats (every 5s) while tokens arrive. A hung stream with no tokens still times out, but any stream producing tokens resets the watchdog, preventing false-positives on slow-but-productive LLM calls.
Replace raw bracket-escaped Write() with tviewmd.RenderPartial which line-buffers: complete lines render through goldmark, the last incomplete line shows as escaped raw text. On flush, Render replaces everything with fully formatted output. This gives live markdown rendering during streaming — headings, lists, code blocks, and tables appear incrementally as lines complete, instead of showing escaped raw brackets until flush. Also pin tviewmd to local via replace directive (sibling repo).
RenderPartial on full pendingTokens via Write() double-appended: each token triggered a full render+append of all accumulated text. Now: - Write(): bare token bytes (fast, streaming) - refreshMessages(): RenderPartial on pendingTokens with custom theme (renders complete lines, holds back incomplete as raw text) - Flush: Render (full goldmark with theme) Incremental rendering is correct: complete lines appear formatted, incomplete line shows as raw text until the newline arrives.
…wer budgets Reviewers hitting max_iterations (50) produce incomplete results because they run out of iterations mid-analysis. New guidance tells the orchestrator to split large review areas into smaller focused tasks: 'Review theme.go and colors/' not 'Review all of tui2'.
Sub-agents previously got WrapUpThreshold=1 — one turn of warning before hitting max_iterations. Reviewers need more time to synthesize findings when they approach their budget. Default 5 gives reviewers 5 turns of advance notice to wrap up and produce a structured contract. Adds WrapUpThreshold to SubAgentConfig and LoopConfig wiring.
Streaming: remove Write() and RenderPartial — just show Thinking... spinner during token arrival. Flushed content renders via renderMarkdown as before. Eliminates garbled streaming output by deferring all rendering to flush boundaries. Scroll snapback: track userScrolled flag set on Up/PgUp/Top, cleared on Down/PgDn/Bottom. refreshMessages only auto-ScrollToEnd when userScrolled is false. Prevents the view from yanking back to the tail when the user manually scrolls up to read overflow content.
Mouse scroll up sets userScrolled=true (prevents auto-snap on next refresh). Mouse scroll down clears it only when already at the bottom of the conversation. Works alongside keyboard scroll tracking.
Multiple reviewers shared the same Role string, causing AddSubAgentEnd and AddSubAgentError to always target the first block. Subsequent subagents with the same role never received completion/failure updates — the first block absorbed all events.
Width -2 prevents horizontal rules and tables from bleeding into adjacent panes. Empty inline code background uses inherited color instead of 'default' which may be misinterpreted by tview.
…old close tag - infopane.Build() now accepts Theme and sets a purple border (#af5fff) - Added PaneBorder field to Theme struct - MCP section wired to mcpinfo.Format() instead of hardcoded dash - TagBold close tag changed from [-] to [-:-:-] to reset bold flag - Removed unused escapeBrackets function
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change advances TUI2 Phase 3. It adds command-palette actions, Ctrl-based input handling, streaming metrics and rendering updates, scrolling and search, agent steer/follow-up/stop callbacks, heartbeat notifications, configurable wrap-up thresholds, and middleware-owned compaction. ChangesTUI2 hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/agent/turn.go (1)
80-91: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReport the pre-prepare message count in the error text.
Inside this branch
len(req.Messages)is always0, so the error always reports "0 messages after prepare". Uselen(*messages)to make the error actionable. The doc comment also breaks mid-identifier; move the period inside the parentheses.🛠️ Proposed fix
- // Compaction is handled by the middleware pipeline (CompactionMiddleware. - // PrepareStep). guardContextBeforeCall only validates that the request - // is not empty — it does not trigger compaction. + // Compaction is handled by the middleware pipeline + // (CompactionMiddleware.PrepareStep). guardContextBeforeCall only + // validates that the request is not empty — it does not trigger compaction. if len(req.Messages) == 0 { - err := fmt.Errorf("refusing to send empty message list to provider — %d messages after prepare", len(req.Messages)) + err := fmt.Errorf("refusing to send empty message list to provider — %d messages before prepare", len(*messages))🤖 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/agent/turn.go` around lines 80 - 91, Update the empty-message error in the guardContextBeforeCall branch to report len(*messages), preserving the post-prepare context while exposing the pre-prepare count. Also fix the preceding comment punctuation so “CompactionMiddleware.PrepareStep” remains intact inside the parentheses.internal/tui2/tui2.go (1)
654-662: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe toggle checks the wrong page name, so Ctrl+P stacks duplicate modals.
toggleCommandPalettetestsHasPage("cmdpalette_modal"), butshowCommandListregisters the page as"cmdlist_modal"(Line 581). The condition is therefore always false. Each Ctrl+P press callsshowCommandListagain and pushes another page ontot.Pages. The user must press Escape once per press to unwind them.
t.CmdPaletteis also built inbuildUI(Line 234) but is no longer added to any page, so thecmdpalette_modalname is now unreachable.🐛 Proposed fix
+const cmdListModal = "cmdlist_modal" + func (t *TUI2) toggleCommandPalette() { - const cmdModal = "cmdpalette_modal" - if t.Pages.HasPage(cmdModal) { - t.Pages.RemovePage(cmdModal) + if t.Pages.HasPage(cmdListModal) { + t.Pages.RemovePage(cmdListModal) t.App.SetFocus(t.Input) t.focus = focusNormal } else { t.showCommandList() } }Then use
cmdListModalinshowCommandListin place of its localmodalName, and remove the now-unusedt.CmdPalettewiring.🤖 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/tui2/tui2.go` around lines 654 - 662, Update toggleCommandPalette to check the page identifier used by showCommandList, cmdListModal, so repeated Ctrl+P presses remove the existing command-list modal instead of stacking duplicates. In buildUI, remove the obsolete t.CmdPalette wiring now that it is not registered on any page.
🧹 Nitpick comments (8)
internal/tui2/keymap.go (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a different key for
ActionToggleSubAgents.Many terminals consume Ctrl+S as XOFF (software flow control) and never deliver it to the application. On those terminals the binding appears dead and the display freezes until the user presses Ctrl+Q. Ctrl+G or Ctrl+B avoids the conflict.
🤖 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/tui2/keymap.go` at line 49, Change the key binding for ActionToggleSubAgents in the keymap definition from Ctrl+S to a non-flow-control key such as Ctrl+G or Ctrl+B, updating its displayed Label to match while preserving the action and help text.internal/tui2/tui2.go (4)
176-201: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe idle check does not prevent the redraw.
QueueUpdateDrawalways callsDrawafter the callback returns, including when the callback returns early at Line 187. The TUI therefore still repaints the full screen five times per second while completely idle. UseQueueUpdateand request the draw only when work happened.♻️ Proposed refactor
- t.App.QueueUpdateDraw(func() { + t.App.QueueUpdate(func() { anyActive := t.thinkingInd.Visible() || t.isStreaming.Load() @@ if !anyActive { return } @@ if !t.isStreaming.Load() { t.refreshMessages() t.renderInfoPane() } + t.App.Draw() })🤖 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/tui2/tui2.go` around lines 176 - 201, Replace the unconditional redraw flow around the ticker callback with QueueUpdate, track whether the callback performed animation or refresh work, and call Draw only when activity was detected. Preserve the existing idle early-return behavior and spinner, subagent, refreshMessages, and renderInfoPane updates for active states.
88-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe comment block places atomic counters under "QueueUpdateDraw only".
tokensRx,charsWritten, andcharsRenderedareatomic.Int64and are updated outside the queue.events.go:18callst.tokensRx.Add(1)before enteringQueueUpdate. The types make that safe, but the section header states the opposite rule. Move the three counters into the "Atomic fields" group so the documented invariant matches the code.🤖 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/tui2/tui2.go` around lines 88 - 105, Move tokensRx, charsWritten, and charsRendered out of the streaming-state group and into the Atomic fields group in the TUI state definition. Update the section placement so the streaming-state header applies only to QueueUpdateDraw-managed fields, while preserving the existing atomic counter types and usage.
559-561: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or implement
CmdVerbose.t.verboseis only toggled and never read, so the command causes no visible change.🤖 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/tui2/tui2.go` around lines 559 - 561, Implement visible verbose-mode behavior for the CmdVerbose branch by making t.verbose affect message rendering or refresh behavior, and ensure t.refreshMessages() reflects the updated setting; alternatively remove the CmdVerbose case and its unused t.verbose state if verbose mode is not needed.
214-227: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the full-buffer copy and split with
GetWrappedLineCount().tview.TextViewv0.42.0 has noGetLineCount()method.GetOriginalLineCount()ignores wrapping, whileGetWrappedLineCount()reuses the internal wrapped-line index and avoids repeated buffer and slice allocations.🤖 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/tui2/tui2.go` around lines 214 - 227, Update the MouseScrollDown branch in the SetMouseCapture callback to use t.Messages.GetWrappedLineCount() instead of retrieving the full text and splitting it with strings.Split. Compare the wrapped-line count with row+h to preserve the existing userScrolled reset behavior, and remove the now-unused strings dependency if applicable.internal/tui2/events.go (1)
131-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis span records nothing useful.
Startis immediately followed byEnd, so the span has a duration near zero and measures no operation. It also usescontext.Background(), so it never attaches to the active turn span and appears as an orphan root in the trace. A counter metric or a span that wrapsaddAssistantResponsewould carry real information.♻️ Proposed refactor
- _, span := otel.Tracer("yaah").Start(context.Background(), "tui2.flush", - trace.WithAttributes( - attribute.Int64("tokens_rx", t.tokensRx.Load()), - attribute.Int64("chars_written", t.charsWritten.Load()), - attribute.Int64("chars_rendered", t.charsRendered.Load()), - attribute.Int("pending_len", len(raw)), - )) - span.End() - - t.addAssistantResponse(raw) + _, span := otel.Tracer("yaah").Start(context.Background(), "tui2.flush", + trace.WithAttributes( + attribute.Int64("tokens_rx", t.tokensRx.Load()), + attribute.Int64("chars_written", t.charsWritten.Load()), + attribute.Int64("chars_rendered", t.charsRendered.Load()), + attribute.Int("pending_len", len(raw)), + )) + t.addAssistantResponse(raw) + span.End()Hoist
otel.Tracer("yaah")into a package-level variable so the flush path does not look up the tracer on every call.🤖 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/tui2/events.go` around lines 131 - 138, Remove the immediately-ended span in the flush path, or move span creation to wrap the actual addAssistantResponse operation using the active context instead of context.Background(). If retaining tracing, hoist otel.Tracer("yaah") into a package-level tracer variable and ensure the recorded attributes describe the wrapped work.internal/tui2/components/command/command.go (1)
52-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the prefix table so
Parseand the argument extraction cannot drift.Each argument command now declares its prefix twice: once in
Parseand once in theSetDoneFuncswitch. If someone renamesfollowupin one place only, the command parses but loses its argument. A shared table removes that risk.♻️ Proposed refactor
var argCommands = []struct { prefix string cmd Cmd }{ {"model ", CmdModel}, {"steer ", CmdSteer}, {"followup ", CmdFollowUp}, {"search ", CmdSearch}, } // ParseArg returns the command and its argument. func ParseArg(input string) (Cmd, string) { input = strings.TrimSpace(input) cmd := Parse(input) for _, ac := range argCommands { if cmd == ac.cmd && strings.HasPrefix(input, ac.prefix) { return cmd, strings.TrimPrefix(input, ac.prefix) } } return cmd, "" }Also applies to: 119-128
🤖 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/tui2/components/command/command.go` around lines 52 - 67, Extract the argument-command prefixes used by Parse and SetDoneFunc into one shared argCommands table containing each prefix and Cmd value. Add or update ParseArg to trim input, parse the command, and derive the argument from that shared table, then use it for argument extraction so commands such as CmdModel, CmdSteer, CmdFollowUp, and CmdSearch cannot drift between parsing and argument handling.cmd/yaah/tui2.go (1)
111-123: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueReuse the stop callback
OnStopduplicatesOnAbort; define onestopAgentcallback and assign it to both fields.SteerandFollowUpuse non-blocking channel operations, so they do not need goroutines.🤖 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 `@cmd/yaah/tui2.go` around lines 111 - 123, In the callback setup around app.OnSteer, app.OnFollowUp, and app.OnStop, define a single stopAgent callback containing the existing HideThinking and cancelAgent cleanup, then assign it to both app.OnStop and app.OnAbort. Keep Steer and FollowUp as direct synchronous callback bodies without adding goroutines.
🤖 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 @.agents/plans/tui2-hardening/PLAN.md:
- Around line 457-464: Replace the Markdown checkbox entries under “Remaining
items” with references to the corresponding Beads issues, preserving this
section as a concise status summary. Use existing issue IDs where available and
do not retain any `☐` TODO-list syntax.
- Around line 480-481: Synchronize the B.1/B.2 feature-inventory rows with the
resolved statuses recorded in the “Ctrl+T conflict” and “:compact semantic bug”
entries: mark Ctrl+T as resolved with toggle tools/reasoning assignments, and
mark :compact as fixed via OnCompact calling sess.Compact().
- Around line 426-455: Update the Phase 3 parity inventory and exit gate to
distinguish the completed items from remaining unimplemented parity work,
explicitly marking unfinished items as deferred or keeping Phase 3 open until
they are delivered. Ensure the documented completion status and exit criteria
remain consistent.
In `@go.mod`:
- Around line 84-85: Remove the local replace directive for
github.com/buchenberg/tviewmd from go.mod and restore the published
github.com/buchenberg/tviewmd v0.1.0 dependency so module downloads work without
a sibling ../tviewmd checkout.
In `@internal/agent/llm/stream.go`:
- Around line 53-59: Update the stream handling logic in SendStream to emit
heartbeats independently of stream deltas by adding a ticker case to its select
loop, using a cadence shorter than StuckChildTimeout and invoking the existing
heartbeat mechanism. Preserve delta processing while ensuring reasoning-only,
tool-call-only, and idle waits reset the watchdog.
In `@internal/agent/subagent_loop.go`:
- Line 30: Update the sub-agent loop options construction in runner.go to pass
opts.defaults.WrapUpThreshold into WrapUpThreshold instead of allowing the
default value of 5 to replace configured values. Preserve negative thresholds
unchanged so wrap_up_turns < 0 disables wrap-up notices, and add tests covering
both a positive configured threshold and a negative disabled value.
In `@internal/prompts/roles/reviewer.md`:
- Around line 33-34: Update the reviewer role description to use the hyphenated
compound modifier, changing “easy to maintain code” to “easy-to-maintain code”
while preserving the rest of the wording.
In `@internal/tui2/colors/theme.go`:
- Line 154: Update the package-level TagBold helper in colors.go to use the full
style reset after the ::b attribute, matching the reset behavior in the
theme-level helper. Preserve its existing prefix and formatting so
sessioninfo.Format and mcpinfo.Format no longer leave following text bold.
In `@internal/tui2/components/command/command.go`:
- Around line 78-79: Update the command matching case in the command parser to
accept “model” only when the input is exactly the bare command or begins with
“model ” followed by arguments; reject inputs such as “models” and “modelx”
while preserving CmdModel for valid argument forms.
In `@internal/tui2/components/infopane/infopane.go`:
- Around line 9-19: Update Build in the infopane component to apply the border
and title colors only when the theme is non-nil and NoColor is false, matching
input.Build’s guard behavior; leave the default TextView styling unchanged
otherwise.
In `@internal/tui2/control.go`:
- Around line 100-107: Move the “Ephemeral messages (search results, notices)”
comment to the block beginning at the next ephemeral-message rendering section,
and update the token counter’s `wr` value in the surrounding render method to
use the actually maintained write-count metric identified in
`internal/tui2/events.go` instead of `t.charsWritten`, preserving the existing
counter label and formatting.
In `@internal/tui2/events.go`:
- Around line 17-23: Update the TokenDeltaEvent handling in the event switch so
each token delta is rendered in Messages while streaming, either by appending
the pending token content to the displayed messages or by updating the refresh
path to include pendingTokens. Ensure the rendered output stays synchronized
with pendingTokens; changing ticker refresh behavior alone is insufficient.
In `@internal/tui2/helpers_msg.go`:
- Around line 11-16: Update addAssistantResponse and appendMessage so they call
t.App.SetFocus(t.Input) only when the main page is currently on top; preserve
message appending and refresh behavior regardless of modal state, and leave
focus on approval or question modals when either is active.
In `@internal/tui2/markdown.go`:
- Around line 10-25: Move the mdTheme definition out of package scope and into
renderMarkdown, or construct it through a helper called by renderMarkdown before
tviewmd.Render. Preserve all existing theme values and rendering behavior.
In `@internal/tui2/tui2.go`:
- Around line 801-814: Update TUI2.searchMessages to derive the match index and
newline count from the same lowercased text value, avoiding slicing the original
text with an offset from a transformed string. Also convert the logical line
containing the match into the wrapped display row expected by
t.Messages.ScrollTo, accounting for SetWrap(true) and long lines.
- Around line 816-826: Update TUI2.SetEphemeral to prevent older delayed clears
from removing newer messages: add an ephemeralSeq field beside ephemeralMsg,
increment it on each call, capture the current sequence in the goroutine, and
clear/render only when the captured sequence still matches the latest value.
- Around line 583-614: Update the command-list callback around entries and
HandleCommand so argument-required commands prompt for input before dispatching.
Use the existing command.Palette input through promptForArg for CmdSteer and
CmdSearch, then pass the entered value to t.HandleCommand; keep no-argument
commands unchanged and add the missing CmdFollowUp entry.
- Around line 509-512: Update the ActionScrollUp/ActionPageUp/ActionTop and
ActionScrollDown/ActionPageDown/ActionBottom handling in globalInputCapture to
change userScrolled only when t.Messages has focus. Leave arrow-key events
directed at t.Input from modifying the auto-scroll state.
- Around line 780-799: Update toggleBanner to keep Header in its original Root
flex position by using ResizeItem rather than removing and re-adding it. Store
the line count returned by banner.Build() and have headerHeight use that stored
value, since Banner.GetInnerRect() is zero after collapse.
---
Outside diff comments:
In `@internal/agent/turn.go`:
- Around line 80-91: Update the empty-message error in the
guardContextBeforeCall branch to report len(*messages), preserving the
post-prepare context while exposing the pre-prepare count. Also fix the
preceding comment punctuation so “CompactionMiddleware.PrepareStep” remains
intact inside the parentheses.
In `@internal/tui2/tui2.go`:
- Around line 654-662: Update toggleCommandPalette to check the page identifier
used by showCommandList, cmdListModal, so repeated Ctrl+P presses remove the
existing command-list modal instead of stacking duplicates. In buildUI, remove
the obsolete t.CmdPalette wiring now that it is not registered on any page.
---
Nitpick comments:
In `@cmd/yaah/tui2.go`:
- Around line 111-123: In the callback setup around app.OnSteer, app.OnFollowUp,
and app.OnStop, define a single stopAgent callback containing the existing
HideThinking and cancelAgent cleanup, then assign it to both app.OnStop and
app.OnAbort. Keep Steer and FollowUp as direct synchronous callback bodies
without adding goroutines.
In `@internal/tui2/components/command/command.go`:
- Around line 52-67: Extract the argument-command prefixes used by Parse and
SetDoneFunc into one shared argCommands table containing each prefix and Cmd
value. Add or update ParseArg to trim input, parse the command, and derive the
argument from that shared table, then use it for argument extraction so commands
such as CmdModel, CmdSteer, CmdFollowUp, and CmdSearch cannot drift between
parsing and argument handling.
In `@internal/tui2/events.go`:
- Around line 131-138: Remove the immediately-ended span in the flush path, or
move span creation to wrap the actual addAssistantResponse operation using the
active context instead of context.Background(). If retaining tracing, hoist
otel.Tracer("yaah") into a package-level tracer variable and ensure the recorded
attributes describe the wrapped work.
In `@internal/tui2/keymap.go`:
- Line 49: Change the key binding for ActionToggleSubAgents in the keymap
definition from Ctrl+S to a non-flow-control key such as Ctrl+G or Ctrl+B,
updating its displayed Label to match while preserving the action and help text.
In `@internal/tui2/tui2.go`:
- Around line 176-201: Replace the unconditional redraw flow around the ticker
callback with QueueUpdate, track whether the callback performed animation or
refresh work, and call Draw only when activity was detected. Preserve the
existing idle early-return behavior and spinner, subagent, refreshMessages, and
renderInfoPane updates for active states.
- Around line 88-105: Move tokensRx, charsWritten, and charsRendered out of the
streaming-state group and into the Atomic fields group in the TUI state
definition. Update the section placement so the streaming-state header applies
only to QueueUpdateDraw-managed fields, while preserving the existing atomic
counter types and usage.
- Around line 559-561: Implement visible verbose-mode behavior for the
CmdVerbose branch by making t.verbose affect message rendering or refresh
behavior, and ensure t.refreshMessages() reflects the updated setting;
alternatively remove the CmdVerbose case and its unused t.verbose state if
verbose mode is not needed.
- Around line 214-227: Update the MouseScrollDown branch in the SetMouseCapture
callback to use t.Messages.GetWrappedLineCount() instead of retrieving the full
text and splitting it with strings.Split. Compare the wrapped-line count with
row+h to preserve the existing userScrolled reset behavior, and remove the
now-unused strings dependency if applicable.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bd0b1b8-b7ad-447d-aefe-049fca174239
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
.agents/plans/tui2-hardening/PLAN.mdcmd/yaah/tui2.gogo.modinternal/agent/llm/stream.gointernal/agent/subagent_loop.gointernal/agent/turn.gointernal/prompts/identity.mdinternal/prompts/roles/reviewer.mdinternal/tui2/colors/theme.gointernal/tui2/components/approval/approval.gointernal/tui2/components/command/command.gointernal/tui2/components/infopane/infopane.gointernal/tui2/components/messages/error/error.gointernal/tui2/components/messages/messages.gointernal/tui2/components/messages/subagent/subagent.gointernal/tui2/components/messages/system/system.gointernal/tui2/components/messages/tool/tool.gointernal/tui2/components/messages/user/user.gointernal/tui2/components/modelpicker/modelpicker.gointernal/tui2/components/thinking/thinking.gointernal/tui2/components/todo/todo.gointernal/tui2/control.gointernal/tui2/events.gointernal/tui2/helpers_msg.gointernal/tui2/keymap.gointernal/tui2/markdown.gointernal/tui2/tui2.go
💤 Files with no reviewable changes (1)
- internal/tui2/components/todo/todo.go
|
|
||
| ### 3.1 Ctrl-only hotkeys + command palette (✅ complete 2026-08-07) | ||
| - ✅ Stripped single-char hotkeys (`?`, `/`, `j`, `k`, `g`, `G`) from `DefaultBindings()` | ||
| - ✅ Only Ctrl+ combinations, Esc, Enter, arrows, navigation keys, Tab remain as hotkeys | ||
| - ✅ All other actions routed through command palette (`:help`, `:search`, `:top`, `:bottom`, `:verbose`, `:stop`, `:steer`, `:banner`) | ||
|
|
||
| ### 3.2 Steer + FollowUp (✅ complete 2026-08-07) | ||
| - ✅ Wire `OnSteer`/`OnFollowUp` callbacks in `tui2.go` | ||
| - ✅ Enter-while-streaming → follow-up (submitFollowUp) | ||
| - ✅ `:steer <text>` command for mid-turn injection | ||
|
|
||
| ### 3.3 Fix `:compact` (✅ complete 2026-08-07) | ||
| - ✅ `:compact` now calls `OnCompact` → `sess.Compact()` instead of `CollapseAll` | ||
|
|
||
| ### 3.4 Search (✅ complete 2026-08-07) | ||
| - ✅ `:search <query>` scrolls to first match in messages TextView | ||
| - ✅ Ephemeral result message in info pane | ||
|
|
||
| ### 3.5 Verbose toggle (✅ complete 2026-08-07) | ||
| - ✅ `:verbose` command toggles verbose mode | ||
| - ✅ `verbose` field on TUI2 struct | ||
|
|
||
| ### 3.6 Stop (✅ complete 2026-08-07) | ||
| - ✅ `:stop` command calls `OnStop` → aborts running agent + hides thinking | ||
|
|
||
| ### 3.7 Banner toggle (✅ complete 2026-08-07) | ||
| - ✅ `:banner` toggles banner visibility | ||
|
|
||
| ### 3.8 Ephemeral messages (✅ complete 2026-08-07) | ||
| - ✅ `SetEphemeral(msg)` displays temporary messages in info pane (3s timeout) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the parity exit gate consistent with the documented scope.
The new notes mark the priority work complete, but the plan still lists unimplemented parity items. Update the exit gate and inventory to identify deferred items, or keep Phase 3 open until they are 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 @.agents/plans/tui2-hardening/PLAN.md around lines 426 - 455, Update the
Phase 3 parity inventory and exit gate to distinguish the completed items from
remaining unimplemented parity work, explicitly marking unfinished items as
deferred or keeping Phase 3 open until they are delivered. Ensure the documented
completion status and exit criteria remain consistent.
| ### Remaining items | ||
| - ☐ Click-to-expand via tview regions | ||
| - ☐ Clipboard: `:copyview` + copy-last-response | ||
| - ☐ `:login`/`:logout`/`:mcp` command wiring (callbacks declared, not dispatched) | ||
| - ☐ Model picker data + filter correctness | ||
| - ☐ Min terminal size guard | ||
| - ☐ Per-block verbose collapse semantics (match tui1) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the remaining-work checklist to Beads.
The ☐ entries form a Markdown TODO list. Replace them with Beads references and keep this file as a status summary. As per coding guidelines, use Beads (bd) for all task tracking and do not use Markdown TODO lists.
🤖 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 @.agents/plans/tui2-hardening/PLAN.md around lines 457 - 464, Replace the
Markdown checkbox entries under “Remaining items” with references to the
corresponding Beads issues, preserving this section as a concise status summary.
Use existing issue IDs where available and do not retain any `☐` TODO-list
syntax.
Source: Coding guidelines
| - **Ctrl+T conflict**: ✅ **Resolved 2026-08-07** — Ctrl+T = toggle tools, Ctrl+R = toggle reasoning. No conflict. | ||
| - **`:compact` semantic bug**: ✅ **Fixed 2026-08-07** — now calls `sess.Compact()` via `OnCompact`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize resolved risks with the feature inventory.
These lines mark the Ctrl+T conflict and :compact bug as resolved, while the earlier B.1/B.2 tables still show them as unresolved. Update those rows in the same change so the plan has one authoritative status.
🤖 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 @.agents/plans/tui2-hardening/PLAN.md around lines 480 - 481, Synchronize the
B.1/B.2 feature-inventory rows with the resolved statuses recorded in the
“Ctrl+T conflict” and “:compact semantic bug” entries: mark Ctrl+T as resolved
with toggle tools/reasoning assignments, and mark :compact as fixed via
OnCompact calling sess.Compact().
|
|
||
| replace github.com/buchenberg/tviewmd => ../tviewmd |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
test -f ../tviewmd/go.mod || {
echo "missing ../tviewmd/go.mod: the committed replacement is not reproducible" >&2
exit 1
}Repository: buchenberg/yaah
Length of output: 226
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- go.mod ---'
sed -n '1,130p' go.mod
printf '%s\n' '--- tracked workflow/config candidates ---'
git ls-files | rg '(^|/)(\.github/workflows/|Makefile$|Taskfile|Justfile|Dockerfile|go\.work$|scripts?/|.*\.(sh|ya?ml|yaml|toml|json))' | sed -n '1,240p'
printf '%s\n' '--- tviewmd references ---'
rg -n --hidden -g '!.git' -g '!go.sum' 'tviewmd|replace .*=>' . | sed -n '1,240p'Repository: buchenberg/yaah
Length of output: 6883
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- .github/workflows/ci.yml ---'
cat -n .github/workflows/ci.yml
printf '%s\n' '--- .github/workflows/release-please.yml ---'
cat -n .github/workflows/release-please.yml
printf '%s\n' '--- Dockerfile ---'
cat -n Dockerfile
printf '%s\n' '--- install.sh ---'
cat -n install.sh
printf '%s\n' '--- relevant plan sections ---'
sed -n '1,75p' .agents/plans/tui2-hardening/PLAN.md
sed -n '345,385p' .agents/plans/tui2-hardening/PLAN.md
sed -n '475,505p' .agents/plans/tui2-hardening/PLAN.mdRepository: buchenberg/yaah
Length of output: 18041
Remove the local module replacement before merging. CI checks out only yaah and does not provision ../tviewmd; go mod download therefore fails. Use the published github.com/buchenberg/tviewmd v0.1.0 module without the replacement.
🤖 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 `@go.mod` around lines 84 - 85, Remove the local replace directive for
github.com/buchenberg/tviewmd from go.mod and restore the published
github.com/buchenberg/tviewmd v0.1.0 dependency so module downloads work without
a sibling ../tviewmd checkout.
| var lastHeartbeat time.Time | ||
| emitHeartbeat := func() { | ||
| if time.Since(lastHeartbeat) > 5*time.Second { | ||
| tools.SendHeartbeat(ctx) | ||
| lastHeartbeat = time.Now() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- outline ---'
ast-grep outline internal/agent/llm/stream.go --view expanded || true
echo '--- stream.go relevant ranges ---'
nl -ba internal/agent/llm/stream.go | sed -n '1,180p'
echo '--- heartbeat and stream symbols ---'
rg -n -C 4 'SendStream|SendHeartbeat|emitHeartbeat|Reason|reason|tool.?call|delta|Heartbeat' internal/agent internal 2>/dev/null | head -n 500Repository: buchenberg/yaah
Length of output: 39292
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- stream.go ---'
sed -n '1,180p' internal/agent/llm/stream.go
printf '%s\n' '--- relevant definitions and call sites ---'
rg -n -C 6 'func .*Send(Stream|Heartbeat)|SendStream|SendHeartbeat|emitHeartbeat|Stream' internal/agent internal 2>/dev/null | head -n 600Repository: buchenberg/yaah
Length of output: 42601
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- target file ---'
nl -ba internal/agent/llm/stream.go | sed -n '1,180p'
echo '--- symbols ---'
rg -n -C 5 'SendStream|SendHeartbeat|emitHeartbeat|type .*Delta|Reasoning|ToolCall|heartbeat' internal/agent internal 2>/dev/null | head -n 600Repository: buchenberg/yaah
Length of output: 40463
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target ---'
nl -ba internal/agent/llm/stream.go | sed -n '1,180p'
printf '%s\n' '--- related definitions ---'
rg -n -C 8 'SendStream|SendHeartbeat|emitHeartbeat|Heartbeat|Reasoning|ToolCall|Stream' internal/agent internal 2>/dev/null | head -n 800Repository: buchenberg/yaah
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -eu
git ls-files | rg '(^|/)(stream|.*agent.*|.*llm.*)' | head -n 200
nl -ba internal/agent/llm/stream.go | sed -n '1,180p'
rg -n -C 8 'SendStream|SendHeartbeat|emitHeartbeat|Heartbeat|Reasoning|ToolCall' . --glob '!vendor/**' --glob '!node_modules/**' | head -n 800Repository: buchenberg/yaah
Length of output: 50373
🌐 Web query:
"Emit heartbeats independently of content deltas" "SendStream" "SendHeartbeat"
💡 Result:
The concept of emitting heartbeats independently of content deltas—often implemented as SendHeartbeat or similar mechanisms—is a common design pattern used in streaming applications (such as Server-Sent Events, WebSockets, or gRPC streams) to maintain connection liveness and prevent timeouts [1][2][3][4]. Key characteristics of this pattern include: 1. Protocol Liveness: When a stream is idle (e.g., waiting for a slow upstream process like LLM reasoning), intermediaries like proxies, load balancers, or firewalls may close the connection due to inactivity [1][3][5]. Emitting a synthetic heartbeat (e.g., an SSE comment frame like: keepalive or: prefill) at regular intervals ensures the TCP connection remains active without interfering with the actual content stream, as these comments are specified to be ignored by clients [1][3][6][4]. 2. Independent Cadence: Heartbeats are often generated by a separate timer or goroutine that monitors for inactivity [2][7][4]. This ticker is typically gated by the actual transmission of data; if a visible chunk is sent, the heartbeat timer is reset [1][3][6][4]. This ensures heartbeats are emitted only during silent windows and never interleave with or corrupt real data payloads [1][7][4]. 3. Implementation Variations: - Streaming APIs: In systems like SSE or custom streaming protocols, this involves injecting non-functional frames (comments or empty-delta chunks) during idle periods [1][3][5][4]. - Request-Response Heartbeats: In some control-plane protocols, SendHeartbeat may require an explicit acknowledgment (ACK) from the peer to verify the connection is healthy, triggering a state update (e.g., marking a connection unhealthy) if the ACK is not received within a timeout period [8][9][4]. - Consumer Group Management: In messaging services, heartbeat operations inform the server that a consumer instance is still active and participating in a consumer group, preventing it from being removed from the group due to apparent inactivity [10]. Technical implementations often use configuration variables (e.g., HEARTBEAT_INTERVAL) to allow operators to tune or disable this behavior without modifying core streaming logic [3][6][5].
Citations:
- 1: warpfront/hipfire@a314d8b
- 2: moves sse hearbeats to a common structure to reuse maximhq/bifrost#5850
- 3: dtzp555-max/ocp@b871b72
- 4: feat: harden control-plane protocol for reliable board communication docker/docker-agent#3491
- 5: fix(sse-heartbeat): shape-aware keepalives keep streams alive through stricter proxies diegosouzapw/OmniRoute#2233
- 6: feat(server): SSE heartbeat on streaming path (#47) dtzp555-max/ocp#49
- 7: https://github.com/omnigent-ai/omnigent/blob/main/omnigent/runtime/session_stream.py
- 8: Mmx233/QMux@48ce169
- 9: https://docs.aws.amazon.com/sagemaker/latest/APIReference/API_edge_SendHeartbeat.html
- 10: https://docs.oracle.com/en-us/iaas/Content/Streaming/Tasks/send-heartbeat-consumer-group.htm
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- heartbeat implementation ---'
sed -n '80,112p' internal/jobs/context.go
echo '--- child watchdog ---'
sed -n '160,225p' internal/agent/agent_tools.go
echo '--- focused source verifier ---'
python3 - <<'PY'
from pathlib import Path
source = Path("internal/agent/llm/stream.go").read_text()
required = [
'emitHeartbeat()',
'if delta.Content != ""',
'if delta.ReasoningContent != ""',
'for _, tc := range delta.ToolCalls',
'case <-ctx.Done():',
]
missing = [text for text in required if text not in source]
if missing:
raise SystemExit(f"missing expected source fragments: {missing}")
content_start = source.index('if delta.Content != ""')
heartbeat_pos = source.index('emitHeartbeat()', content_start)
content_end = source.index('\n\t\t\t}', content_start)
if not (content_start < heartbeat_pos < content_end):
raise SystemExit("heartbeat call is not inside the content-delta branch")
select_start = source.index('\t\tselect {')
select_end = source.index('\n\t\t}', select_start)
select_body = source[select_start:select_end]
if 'time.NewTicker' in select_body or 'ticker.C' in select_body:
raise SystemExit("unexpected ticker found in stream select")
print("heartbeat call is content-only; stream select has no ticker case")
PYRepository: buchenberg/yaah
Length of output: 3404
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '80,112p' internal/jobs/context.go
sed -n '160,225p' internal/agent/agent_tools.go
python3 - <<'PY'
from pathlib import Path
source = Path("internal/agent/llm/stream.go").read_text()
assert source.index("emitHeartbeat()") > source.index('if delta.Content != ""')
assert source.index("emitHeartbeat()") < source.index("for _, tc := range delta.ToolCalls")
select = source[source.index("\t\tselect {"):source.index("\n\t\t}", source.index("\t\tselect {"))]
assert "ticker.C" not in select
print("content-only heartbeat and no ticker case")
PYRepository: buchenberg/yaah
Length of output: 3293
Emit heartbeats independently of stream deltas.
When StuckChildTimeout is active, line 107 resets the watchdog only for delta.Content. Reasoning-only deltas, tool-call-only deltas, and idle SendStream waits can cancel an active sub-agent. Add a ticker case to the stream select with a cadence below the watchdog timeout.
🤖 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/agent/llm/stream.go` around lines 53 - 59, Update the stream
handling logic in SendStream to emit heartbeats independently of stream deltas
by adding a ticker case to its select loop, using a cadence shorter than
StuckChildTimeout and invoking the existing heartbeat mechanism. Preserve delta
processing while ensuring reasoning-only, tool-call-only, and idle waits reset
the watchdog.
| case ActionScrollUp, ActionPageUp, ActionTop: | ||
| t.userScrolled = true | ||
| case ActionScrollDown, ActionPageDown, ActionBottom: | ||
| t.userScrolled = false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Arrow keys inside the input area disable auto-scroll.
globalInputCapture runs before the focused widget sees the event, and these two cases do not check focus. The user presses Up to move the cursor within a multi-line prompt in t.Input, and userScrolled becomes true. Auto-scroll then stays off for the rest of the session until the user presses Down or End. Apply the scroll state only when t.Messages has focus.
🛠️ Proposed fix
case ActionScrollUp, ActionPageUp, ActionTop:
- t.userScrolled = true
+ if t.App.GetFocus() != t.Input {
+ t.userScrolled = true
+ }
case ActionScrollDown, ActionPageDown, ActionBottom:
- t.userScrolled = false
+ if t.App.GetFocus() != t.Input {
+ t.userScrolled = false
+ }🤖 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/tui2/tui2.go` around lines 509 - 512, Update the
ActionScrollUp/ActionPageUp/ActionTop and
ActionScrollDown/ActionPageDown/ActionBottom handling in globalInputCapture to
change userScrolled only when t.Messages has focus. Leave arrow-key events
directed at t.Input from modifying the auto-scroll state.
| entries := []struct { | ||
| label string | ||
| desc string | ||
| cmd command.Cmd | ||
| }{ | ||
| {"help", "Show keybindings and commands", command.CmdHelp}, | ||
| {"clear", "Clear conversation", command.CmdClear}, | ||
| {"compact", "Compact context window", command.CmdCompact}, | ||
| {"stop", "Abort running agent", command.CmdStop}, | ||
| {"steer", "Inject steering text (requires arg)", command.CmdSteer}, | ||
| {"model", "Switch model", command.CmdModel}, | ||
| {"search", "Search messages (requires arg)", command.CmdSearch}, | ||
| {"verbose", "Toggle verbose mode", command.CmdVerbose}, | ||
| {"banner", "Toggle banner", command.CmdBanner}, | ||
| {"top", "Scroll to top", command.CmdTop}, | ||
| {"bottom", "Scroll to bottom", command.CmdBottom}, | ||
| {"quit", "Exit yaah", command.CmdQuit}, | ||
| } | ||
|
|
||
| list := tview.NewList(). | ||
| ShowSecondaryText(true). | ||
| SetHighlightFullLine(true). | ||
| SetWrapAround(false) | ||
|
|
||
| for _, e := range entries { | ||
| list.AddItem(e.label, e.desc, 0, func() { | ||
| t.Pages.RemovePage(modalName) | ||
| t.App.SetFocus(t.Input) | ||
| t.focus = focusNormal | ||
| t.HandleCommand(e.cmd, "") | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Selecting steer or search in the command list does nothing.
Every entry dispatches t.HandleCommand(e.cmd, ""). HandleCommand requires a non-empty arg for CmdSteer (Line 552) and searchMessages returns immediately on an empty query (Line 802). The two entries labeled "requires arg" are therefore inert. followup is also missing from the list although CmdFollowUp exists.
Prompt for the argument when the entry needs one.
🛠️ Proposed approach
entries := []struct {
label string
desc string
cmd command.Cmd
+ arg bool
}{
@@
- {"steer", "Inject steering text (requires arg)", command.CmdSteer},
+ {"steer", "Inject steering text", command.CmdSteer, true},
+ {"followup", "Queue a follow-up prompt", command.CmdFollowUp, true},
@@
- {"search", "Search messages (requires arg)", command.CmdSearch},
+ {"search", "Search messages", command.CmdSearch, true},
@@
for _, e := range entries {
list.AddItem(e.label, e.desc, 0, func() {
t.Pages.RemovePage(modalName)
t.App.SetFocus(t.Input)
t.focus = focusNormal
+ if e.arg {
+ t.promptForArg(e.label, e.cmd)
+ return
+ }
t.HandleCommand(e.cmd, "")
})
}promptForArg can reuse the existing command.Palette input field, which is otherwise unused now.
🤖 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/tui2/tui2.go` around lines 583 - 614, Update the command-list
callback around entries and HandleCommand so argument-required commands prompt
for input before dispatching. Use the existing command.Palette input through
promptForArg for CmdSteer and CmdSearch, then pass the entered value to
t.HandleCommand; keep no-argument commands unchanged and add the missing
CmdFollowUp entry.
| func (t *TUI2) toggleBanner() { | ||
| t.showBanner = !t.showBanner | ||
| if t.showBanner { | ||
| t.Root.RemoveItem(t.Header) | ||
| t.Root.AddItem(t.Header, t.headerHeight(), 0, false) | ||
| } else { | ||
| t.Root.RemoveItem(t.Header) | ||
| } | ||
| } | ||
|
|
||
| func (t *TUI2) headerHeight() int { | ||
| if !t.showBanner { | ||
| return 0 | ||
| } | ||
| _, _, _, h := t.Banner.GetInnerRect() | ||
| if h <= 0 { | ||
| return 8 | ||
| } | ||
| return h | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
tview Flex ResizeItem AddItem ordering append behavior
💡 Result:
In the tview package, the Flex container manages its items using an internal slice [1][2]. Understanding how AddItem, ResizeItem, and append-related behaviors work is straightforward: 1. AddItem Behavior: The AddItem method appends new items to the end of the Flex container's internal items slice [1][2]. The order in which you call AddItem determines the final display order (from left to right for horizontal layouts, or top to bottom for vertical layouts) [3][4]. Because it uses an append operation, the item is always placed after any existing items [1][2]. 2. ResizeItem Behavior: The ResizeItem method iterates through the entire list of items in the Flex container to find any that match the provided primitive [1][2]. If it finds a match, it updates the fixedSize and proportion values for that item [1][2]. If multiple items in the Flex container share the same primitive, ResizeItem will update all of them to the new size settings [1][2]. 3. Ordering Considerations: The internal order of items is preserved during resizing and removal [1][2]. The RemoveItem method removes all instances of a given primitive and preserves the relative order of the remaining items by shifting the slice accordingly [1][2]. While there have been historical discussions regarding the order of items affecting drawing in other components like Grid [5], Flex maintains a predictable, sequential list based on the order of addition [1][2]. If you need to change the order of items after they have been added, there is no direct "reorder" method; you typically need to use RemoveItem to remove the relevant primitives and then AddItem to add them back in the desired sequence [1][2].
Citations:
- 1: https://github.com/rivo/tview/blob/master/flex.go
- 2: https://github.com/rivo/tview/blob/v0.42.0/flex.go
- 3: https://github.com/rivo/tview/wiki/Flex
- 4: https://oboe.com/learn/go-terminal-user-interfaces-m7qscs/building-interactive-tuis-with-tview-go-terminal-user-interfaces-3
- 5: fix(grid): order of adding items should not affect drawing rivo/tview#991
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant symbols ---'
rg -n -C 6 'func \(t \*TUI2\) (buildUI|toggleBanner|headerHeight)|ResizeItem|AddItem|RemoveItem|showBanner|Banner\.Build' internal/tui2 --glob '*.go'
printf '%s\n' '--- module dependency ---'
rg -n 'rivo/tview|tview' go.mod go.sum internal/tui2 --glob '*.go'
printf '%s\n' '--- file outline and target section ---'
wc -l internal/tui2/tui2.go
sed -n '650,830p' internal/tui2/tui2.goRepository: buchenberg/yaah
Length of output: 30407
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- buildUI header setup ---'
sed -n '200,292p' internal/tui2/tui2.go
printf '%s\n' '--- banner implementation ---'
cat -n internal/tui2/components/banner/banner.go
printf '%s\n' '--- local tview source locations ---'
go env GOPATH GOMODCACHE 2>/dev/null || true
find "${GOMODCACHE:-/nonexistent}" -path '*github.com/rivo/tview@v0.42.0/flex.go' -o -path '*github.com/rivo/tview@v0.42.0/primitive.go' 2>/dev/null | head
printf '%s\n' '--- references to banner height state ---'
rg -n -C 3 'bannerLines|headerHeight|toggleBanner|Build\(\)' internal/tui2 --glob '*.go'Repository: buchenberg/yaah
Length of output: 10431
🏁 Script executed:
#!/bin/bash
set -eu
TV=/home/jailuser/go/pkg/mod/github.com/rivo/tview@v0.42.0
printf '%s\n' '--- Flex methods ---'
rg -n -C 12 'func \(f \*Flex\) (AddItem|RemoveItem|ResizeItem|Draw)' "$TV/flex.go"
printf '%s\n' '--- Primitive inner-rect methods ---'
rg -n -C 10 'func \(p \*Box\) (GetInnerRect|SetRect)|func \(.*\) GetInnerRect' "$TV" --glob '*.go' | head -160
printf '%s\n' '--- relevant TUI2 fields ---'
sed -n '35,115p' internal/tui2/tui2.go
printf '%s\n' '--- exact module version metadata ---'
sed -n '1,80p' "$TV/go.mod" 2>/dev/null || trueRepository: buchenberg/yaah
Length of output: 7795
🏁 Script executed:
#!/bin/bash
set -eu
TV=/home/jailuser/go/pkg/mod/github.com/rivo/tview@v0.42.0
printf '%s\n' '--- complete Flex allocation logic ---'
sed -n '131,255p' "$TV/flex.go"
printf '%s\n' '--- Box rectangle state and reset behavior ---'
sed -n '1,145p' "$TV/box.go"
rg -n -C 8 'SetRect\(|innerHeight|innerWidth|SetInnerRect' "$TV" --glob '*.go' | head -220
printf '%s\n' '--- toggle call context ---'
sed -n '430,585p' internal/tui2/tui2.goRepository: buchenberg/yaah
Length of output: 32878
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- banner generator ---'
fd -i -t f . internal | rg '/banner|banner'
rg -n -C 8 'func GeneratePlain|GeneratePlain|figlet|tagline|yaah' internal/banner internal/tui2/components/banner --glob '*.go' 2>/dev/null
printf '%s\n' '--- banner-related tests ---'
rg -n -C 5 'toggleBanner|headerHeight|bannerLines|GeneratePlain|CmdBanner' . --glob '*_test.go' --glob '*.go'
printf '%s\n' '--- current status and diff summary ---'
git diff --stat
git status --shortRepository: buchenberg/yaah
Length of output: 26477
Keep the header in the original layout position.
tview.Flex.AddItem appends the header after StatusBar when the banner is shown again. Use ResizeItem instead of removing and re-adding it. Store the banner.Build() line count and use it in headerHeight; GetInnerRect becomes zero after the header collapses.
🤖 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/tui2/tui2.go` around lines 780 - 799, Update toggleBanner to keep
Header in its original Root flex position by using ResizeItem rather than
removing and re-adding it. Store the line count returned by banner.Build() and
have headerHeight use that stored value, since Banner.GetInnerRect() is zero
after collapse.
| func (t *TUI2) searchMessages(query string) { | ||
| if query == "" { | ||
| return | ||
| } | ||
| text := t.Messages.GetText(true) | ||
| idx := strings.Index(strings.ToLower(text), strings.ToLower(query)) | ||
| if idx >= 0 { | ||
| line := strings.Count(text[:idx], "\n") | ||
| t.Messages.ScrollTo(line, 0) | ||
| t.SetEphemeral(fmt.Sprintf("Found at line %d", line+1)) | ||
| } else { | ||
| t.SetEphemeral("No matches found") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The reported line number can point at the wrong row.
Two problems:
idxis a byte offset intostrings.ToLower(text), but Line 808 slices the originaltextwith it. For most input the lengths match. For a few Unicode charactersToLowerchanges the byte length, and the slice then lands mid-rune and reports a wrong line.t.Messagesis built withSetWrap(true).ScrollTotakes a row in the wrapped display, whilestrings.Countcounts logical newlines. On long lines the view scrolls short of the match.
Lower the text once and index that single value to fix the first problem.
🛠️ Proposed fix
text := t.Messages.GetText(true)
- idx := strings.Index(strings.ToLower(text), strings.ToLower(query))
+ lower := strings.ToLower(text)
+ idx := strings.Index(lower, strings.ToLower(query))
if idx >= 0 {
- line := strings.Count(text[:idx], "\n")
+ line := strings.Count(lower[:idx], "\n")🤖 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/tui2/tui2.go` around lines 801 - 814, Update TUI2.searchMessages to
derive the match index and newline count from the same lowercased text value,
avoiding slicing the original text with an offset from a transformed string.
Also convert the logical line containing the match into the wrapped display row
expected by t.Messages.ScrollTo, accounting for SetWrap(true) and long lines.
| func (t *TUI2) SetEphemeral(msg string) { | ||
| t.ephemeralMsg = msg | ||
| t.renderInfoPane() | ||
| go func() { | ||
| time.Sleep(3 * time.Second) | ||
| t.App.QueueUpdateDraw(func() { | ||
| t.ephemeralMsg = "" | ||
| t.renderInfoPane() | ||
| }) | ||
| }() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Overlapping ephemeral messages clear each other early.
Each call starts an independent 3-second goroutine. If a second message arrives one second after the first, the first goroutine still fires at t=3s and clears the newer message after only two seconds. Guard the clear with a sequence number.
🛠️ Proposed fix
func (t *TUI2) SetEphemeral(msg string) {
t.ephemeralMsg = msg
+ t.ephemeralSeq++
+ seq := t.ephemeralSeq
t.renderInfoPane()
go func() {
time.Sleep(3 * time.Second)
t.App.QueueUpdateDraw(func() {
+ if seq != t.ephemeralSeq {
+ return
+ }
t.ephemeralMsg = ""
t.renderInfoPane()
})
}()
}Add ephemeralSeq int next to ephemeralMsg in the TUI2 struct.
🤖 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/tui2/tui2.go` around lines 816 - 826, Update TUI2.SetEphemeral to
prevent older delayed clears from removing newer messages: add an ephemeralSeq
field beside ephemeralMsg, increment it on each call, capture the current
sequence in the goroutine, and clear/render only when the captured sequence
still matches the latest value.
1 participant
Summary by CodeRabbit