Conversation
Split the flat messages.go file into domain-specific files: - session.go: Session lifecycle (new, exit, save, load, etc.) - theme.go: Theme selection and preview - agent.go: Agent switching and model selection - toggle.go: UI state toggles (YOLO, thinking, sidebar) - input.go: Editor input, attachments, and speech - mcp.go: MCP prompt interactions This follows the Elm Architecture principle of grouping messages by the domain they affect, making it easier to understand which components handle which messages. Assisted-By: cagent
Extract the collapsed layout computation into a pure data structure (CollapsedViewModel) and move rendering to a standalone function (RenderCollapsedView). This follows the Elm Architecture principle of keeping Model (data) separate from View (rendering). The ViewModel pattern makes the data-to-view transformation explicit and testable: - CollapsedViewModel: pure data with layout decisions - RenderCollapsedView: pure function that renders the view model The model's computeCollapsedViewModel() builds the view model, and the view calls RenderCollapsedView() with it. Assisted-By: cagent
Add a SessionStateReader interface that provides read-only access to session state. Tool components that only need to read state (not modify it) now depend on this interface rather than the concrete *SessionState type. This follows the Interface Segregation Principle (ISP) and makes data flow more explicit: - Tool components receive SessionStateReader (read-only) - Only components that need to mutate state get *SessionState Benefits: - Clearer contracts about what components can do - Easier to reason about state changes - Better alignment with Elm's unidirectional data flow Assisted-By: cagent
Add an animation.Subscription type that encapsulates the animation tick registration lifecycle. This provides several benefits: 1. Clearer ownership: Components own their subscription state 2. Safer API: Start()/Stop() are idempotent and safe to call multiple times 3. Better encapsulation: Components don't need to track registration state separately The Subscription type follows the Elm Architecture principle of making subscriptions explicit and declarative. Components declare they need animation ticks by holding a Subscription, rather than making imperative calls to global functions. Updated Spinner to use the new Subscription type as a demonstration of the pattern. Assisted-By: cagent
Add a Registration type and RegisterAll method to the tool registry that makes the declarative nature of tool-to-component mapping more explicit. Changes: - Move Registration type from factory to registry (single source of truth) - Add RegisterAll() for bulk declarative registration - Update factory to use the cleaner API This follows the principle of making configuration explicit and declarative, similar to Elm's approach where mappings are data structures rather than imperative code. Assisted-By: cagent
Add a HitTest type that centralizes all mouse hit-testing logic in one place. This separates the concerns of: 1. What UI element is at a given coordinate (HitTest.At) 2. What to do when that element is clicked (handleMouseClick) The MouseTarget enum makes the possible targets explicit, and the At() method checks them in priority order. This follows the Elm pattern of making the domain explicit through types. Benefits: - Single source of truth for clickable regions - Easier to understand and modify hit areas - Clear priority ordering when regions overlap - Better testability of hit detection logic Assisted-By: cagent
Move titleInput.SetWidth() calls from View rendering methods to SetSize(), eliminating side effects from view functions. Before: SetWidth was called during computeCollapsedViewModel() and sessionInfo(), which are called from View(). This violated the Elm Architecture principle that view should be a pure function. After: SetWidth is called once in SetSize() via updateTitleInputWidth(), and View methods simply render the pre-configured input. Benefits: - View() is now a pure function (no state mutations) - Width calculation happens at the right time (on resize) - Easier to reason about when state changes occur - Better performance (SetWidth not called on every render) Assisted-By: cagent
Add a Builder type that provides a fluent API for accumulating tea.Cmd
values, reducing boilerplate in Update functions.
Features:
- Add(cmd): append a single command (nil-safe)
- AddIf(cond, cmd): conditionally append
- AddAll(cmds...): append multiple commands
- Batch(): return batched command (optimizes single/empty cases)
This follows the Elm Architecture pattern of accumulating effects
(commands) during the update phase, making the intent clearer:
Before:
var cmds []tea.Cmd
cmds = append(cmds, cmd1)
if condition { cmds = append(cmds, cmd2) }
return m, tea.Batch(cmds...)
After:
return m, cmdbatch.New().
Add(cmd1).
AddIf(condition, cmd2).
Batch()
Assisted-By: cagent
Restructure runtime_events.go to group event handlers by category, making the event-to-update mapping more explicit and easier to navigate. Categories: - Error/Warning Events: Error display and notifications - Stream Lifecycle: Start/stop spinners, pending state - Content Events: Text appending, reasoning blocks - Tool Events: Tool call lifecycle (partial → running → response) - Sidebar Info Events: Agent/team/toolset info updates - RAG Indexing Events: Forwarded to sidebar - Dialog Events: Max iterations, elicitation dialogs Also added documentation header explaining the event mapping pattern, following the Elm Architecture principle of making the event-to-update relationship explicit and discoverable. Assisted-By: cagent
Add a subscription package that provides patterns for converting external event sources (channels, etc.) into the Bubble Tea message flow, following the Elm Architecture subscription pattern. Features: - FromChannel: Convert channel reads to tea.Cmd/tea.Msg - FromChannelWithClose: With optional close handler - ChannelSubscription: Reusable wrapper with Listen() method Updated the theme file watcher to use ChannelSubscription, making the external event source explicit and demonstrating the pattern. In Elm, subscriptions are declared at the top level and the runtime manages them. This package provides similar patterns for Go/Bubble Tea, making external event handling explicit and consistent. Assisted-By: cagent
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
TUI Refactor Review
Found 1 high-severity bug in the animation subscription refactoring.
✅ Good Refactoring
- Clean separation of concerns with view models
- SessionStateReader interface follows principle of least privilege
- Message organization by domain is much clearer
- HitTest centralization improves maintainability
🐛 Critical Bug Found
The spinner animation subscription has a resource leak due to value receiver semantics - see inline comment for details and fix.
| @@ -119,13 +120,13 @@ func (s Spinner) SetSize(_, _ int) tea.Cmd { return nil } | |||
| // Init registers the spinner with the animation coordinator. | |||
| // If this is the first active animation, it starts the global tick. | |||
| func (s Spinner) Init() tea.Cmd { | |||
Contributor
There was a problem hiding this comment.
🐛 HIGH SEVERITY: Resource leak - animation subscriptions not properly managed
The Init() and Stop() methods use value receivers, which means they operate on copies of the Spinner struct. This breaks the subscription lifecycle:
The Problem:
Init()callss.animSub.Start()which setsactive=trueon a copy of the Spinner- The original spinner's
animSub.activeremainsfalse(changes are lost) - Later,
Stop()operates on another copy whereactive=false Stop()seesactive=falseand returns early without callingUnregister()
The Impact:
- Animation coordinator's register count is incremented but never decremented
- Animation ticks continue running even when no spinners are active
- Memory and CPU leak that accumulates over time
The Fix:
Change both methods to use pointer receivers:
func (s *Spinner) Init() tea.Cmd {
return s.animSub.Start()
}
func (s *Spinner) Stop() {
s.animSub.Stop()
}Alternatively, if value receivers are required for Bubble Tea compatibility, the Spinner should be stored by pointer everywhere it's used (e.g., in toolcommon/base.go).
Contributor
There was a problem hiding this comment.
seems irrelevant looking better at the changes, wdyt? @dgageot
krissetto
approved these changes
Feb 1, 2026
Contributor
krissetto
left a comment
krissetto
left a comment
There was a problem hiding this comment.
also gave this a test on one of my slower machines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.