From 4d9bf8a276dff16ee8446d7f60c455f01c86371d Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Thu, 26 Feb 2026 12:05:29 +0300 Subject: [PATCH] fix(ui): resolve queue deadlock and anchor queued messages to input Fix a deadlock where submitting a message while the agent is streaming locks the app. App.Run() and App.ClearQueue() were calling prog.Send() synchronously from within Bubble Tea's Update() loop, blocking when the internal event channel was full from streaming events. Run() now returns the queue depth instead of sending events, and ClearQueue() no longer sends events. The UI layer updates state directly. Additionally, queued messages are now rendered with a "queued" badge between the separator and input, anchored in the managed region until the agent picks them up. Previously they were printed to scrollback immediately and only a count badge was shown. --- internal/app/app.go | 27 +++++--- internal/ui/children_test.go | 29 ++++++--- internal/ui/input.go | 15 +++-- internal/ui/model.go | 123 ++++++++++++++++++++++++++++------- internal/ui/model_test.go | 64 +++++++++++++----- 5 files changed, 189 insertions(+), 69 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index c851a882..b611cd3d 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -24,7 +24,7 @@ import ( // // App satisfies the ui.AppController interface defined in internal/ui/model.go: // -// Run(prompt string) +// Run(prompt string) int // CancelCurrentStep() // QueueLength() int // ClearQueue() @@ -88,30 +88,36 @@ func (a *App) SetProgram(p *tea.Program) { // Run queues a prompt for execution. If the app is idle the prompt is // executed immediately in a background goroutine; otherwise it is appended -// to the queue and a QueueUpdatedEvent is sent to the program. +// to the queue. +// +// Returns the current queue depth after the operation: 0 means the prompt +// was started immediately (or the app is closed), >0 means it was queued. +// The caller is responsible for updating any UI state (e.g. queue badge) +// based on the returned value — Run does NOT send events to the program, +// because it may be called synchronously from within Bubble Tea's Update +// loop where prog.Send would deadlock. // // Satisfies ui.AppController. -func (a *App) Run(prompt string) { +func (a *App) Run(prompt string) int { a.mu.Lock() if a.closed { a.mu.Unlock() - return + return 0 } if a.busy { a.queue = append(a.queue, prompt) qLen := len(a.queue) a.mu.Unlock() - // sendEvent must be called without a.mu held (see sendEvent comment). - a.sendEvent(QueueUpdatedEvent{Length: qLen}) - return + return qLen } a.busy = true a.wg.Add(1) a.mu.Unlock() go a.drainQueue(prompt) + return 0 } // CancelCurrentStep cancels the currently executing agent step. Safe to call @@ -134,15 +140,16 @@ func (a *App) QueueLength() int { return len(a.queue) } -// ClearQueue discards all queued prompts and sends a QueueUpdatedEvent. +// ClearQueue discards all queued prompts. The caller is responsible for +// updating any UI state (e.g. queue badge) — ClearQueue does NOT send +// events to the program, because it may be called synchronously from +// within Bubble Tea's Update loop where prog.Send would deadlock. // // Satisfies ui.AppController. func (a *App) ClearQueue() { a.mu.Lock() a.queue = a.queue[:0] a.mu.Unlock() - // sendEvent must be called without a.mu held (see sendEvent comment). - a.sendEvent(QueueUpdatedEvent{Length: 0}) } // ClearMessages empties the conversation history. diff --git a/internal/ui/children_test.go b/internal/ui/children_test.go index d774598e..00481923 100644 --- a/internal/ui/children_test.go +++ b/internal/ui/children_test.go @@ -209,11 +209,13 @@ func TestInputComponent_ClearNilCtrl_NoPanic(t *testing.T) { } // -------------------------------------------------------------------------- -// TestInputComponent_ClearQueueCallsClearQueue verifies that /clear-queue (and -// its alias /cq) calls appCtrl.ClearQueue() and returns no submitMsg. +// TestInputComponent_ClearQueue_ForwardsAsSubmitMsg verifies that /clear-queue +// (and its alias /cq) are forwarded as submitMsg to the parent model (so the +// parent can call ClearQueue and update queueCount directly, avoiding a +// deadlock from calling prog.Send within Update). // -------------------------------------------------------------------------- -func TestInputComponent_ClearQueueCallsClearQueue(t *testing.T) { +func TestInputComponent_ClearQueue_ForwardsAsSubmitMsg(t *testing.T) { aliases := []string{"/clear-queue", "/cq"} for _, alias := range aliases { t.Run(alias, func(t *testing.T) { @@ -224,14 +226,21 @@ func TestInputComponent_ClearQueueCallsClearQueue(t *testing.T) { _, cmd := sendInputMsg(c, tea.KeyPressMsg{Code: tea.KeyEnter}) - if ctrl.clearQueueCalled != 1 { - t.Fatalf("%s: expected ClearQueue() called once, got %d", alias, ctrl.clearQueueCalled) + // ClearQueue should NOT be called directly by InputComponent. + if ctrl.clearQueueCalled != 0 { + t.Fatalf("%s: expected ClearQueue() not called, got %d", alias, ctrl.clearQueueCalled) } - if cmd != nil { - msg := runCmd(cmd) - if _, ok := msg.(submitMsg); ok { - t.Fatalf("%s: /clear-queue should not emit submitMsg, got submitMsg", alias) - } + // Instead, a submitMsg should be emitted so the parent handles it. + if cmd == nil { + t.Fatalf("%s: expected submitMsg cmd, got nil", alias) + } + msg := runCmd(cmd) + sm, ok := msg.(submitMsg) + if !ok { + t.Fatalf("%s: expected submitMsg, got %T", alias, msg) + } + if sm.Text != alias { + t.Fatalf("%s: expected submitMsg text %q, got %q", alias, alias, sm.Text) } }) } diff --git a/internal/ui/input.go b/internal/ui/input.go index 2d05271b..ee3826ab 100644 --- a/internal/ui/input.go +++ b/internal/ui/input.go @@ -17,7 +17,10 @@ import ( // Slash commands handled locally (not forwarded to app layer): // - /quit, /q, /exit → tea.Quit // - /clear, /cls, /c → appCtrl.ClearMessages() then clear the textarea -// - /clear-queue → appCtrl.ClearQueue() then clear the textarea +// +// /clear-queue is forwarded to the parent via submitMsg so the parent can +// update queueCount directly (calling ClearQueue from within Update would +// require prog.Send which deadlocks). // // All other input is returned via submitMsg for the parent to forward to // app.Run(). @@ -196,6 +199,10 @@ func (s *InputComponent) handleSubmit(value string) tea.Cmd { } // Resolve via canonical command lookup so aliases are handled uniformly. + // Only /quit and /clear are handled locally — /clear-queue must go + // through the parent model so it can update queueCount directly + // (calling ClearQueue here would skip the UI state update since we + // can't send events from within Update without deadlocking). if sc := GetCommandByName(trimmed); sc != nil { switch sc.Name { case "/quit": @@ -207,12 +214,6 @@ func (s *InputComponent) handleSubmit(value string) tea.Cmd { } // Don't forward to app.Run(); just clear silently. return nil - - case "/clear-queue": - if s.appCtrl != nil { - s.appCtrl.ClearQueue() - } - return nil } } diff --git a/internal/ui/model.go b/internal/ui/model.go index d1cbb143..d83471f1 100644 --- a/internal/ui/model.go +++ b/internal/ui/model.go @@ -2,6 +2,7 @@ package ui import ( "fmt" + "strings" "time" tea "charm.land/bubbletea/v2" @@ -28,14 +29,19 @@ const ( // makes the parent model easily testable with a mock. type AppController interface { // Run queues or immediately starts a new agent step with the given prompt. - // If an agent step is already in progress the prompt is queued and a - // QueueUpdatedEvent is sent to the program. - Run(prompt string) + // Returns the current queue depth: 0 means the prompt started immediately + // (or the app is closed), >0 means it was queued. The caller must update + // UI state (e.g. queueCount) based on the return value — Run does NOT + // send events to the program to avoid deadlocking when called from + // within Update(). + Run(prompt string) int // CancelCurrentStep cancels any in-progress agent step. CancelCurrentStep() // QueueLength returns the number of prompts currently waiting in the queue. QueueLength() int - // ClearQueue discards all queued prompts and emits a QueueUpdatedEvent. + // ClearQueue discards all queued prompts. The caller must update UI state + // (e.g. queueCount) — ClearQueue does NOT send events to the program to + // avoid deadlocking when called from within Update(). ClearQueue() // ClearMessages clears the conversation history. ClearMessages() @@ -83,7 +89,9 @@ type AppModelOptions struct { // // ┌─ stream region (variable height) ─────────────────┐ // │ │ -// ├─ separator line (with optional queue badge) ───────┤ +// ├─ separator line (with optional queue count) ───────┤ +// │ queued How do I fix the build? │ +// │ queued Also check the tests │ // └─ input region (fixed height from textarea) ────────┘ // // Completed responses are emitted above the BT-managed region via tea.Println() @@ -116,8 +124,10 @@ type AppModel struct { // modelName is the LLM model name shown in rendered messages. modelName string - // queueCount is cached from the last QueueUpdatedEvent for badge rendering. - queueCount int + // queuedMessages stores the text of prompts that were queued (not yet + // submitted to the agent). They are rendered with a "queued" badge above + // the input and move to scrollback when the agent picks them up. + queuedMessages []string // canceling tracks whether the user has pressed ESC once during stateWorking. // A second ESC within 2 seconds will cancel the current step. @@ -324,11 +334,24 @@ func (m *AppModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.Batch(cmds...) } - // Regular prompt — print user message and forward to the app layer. - cmds = append(cmds, m.printUserMessage(msg.Text)) + // Regular prompt — forward to the app layer. if m.appCtrl != nil { - // app.Run() handles queueing internally if a step is in progress. - m.appCtrl.Run(msg.Text) + // Run returns the queue depth: >0 means the prompt was queued + // (agent is busy). We update queuedMessages directly here + // instead of relying on an event from prog.Send(), which would + // deadlock when called synchronously from within Update(). + if qLen := m.appCtrl.Run(msg.Text); qLen > 0 { + // Queued: anchor the message text above the input with a + // "queued" badge. It will be printed to scrollback when + // the agent picks it up (on QueueUpdatedEvent). + m.queuedMessages = append(m.queuedMessages, msg.Text) + m.distributeHeight() + } else { + // Started immediately: print to scrollback now. + cmds = append(cmds, m.printUserMessage(msg.Text)) + } + } else { + cmds = append(cmds, m.printUserMessage(msg.Text)) } if m.state != stateWorking { m.state = stateWorking @@ -398,7 +421,15 @@ func (m *AppModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // Informational — no action needed by parent. case app.QueueUpdatedEvent: - m.queueCount = msg.Length + // drainQueue popped item(s) from the queue. Move consumed messages + // from the anchored display to scrollback (they are now being processed + // or about to be). + for len(m.queuedMessages) > msg.Length { + text := m.queuedMessages[0] + m.queuedMessages = m.queuedMessages[1:] + cmds = append(cmds, m.printUserMessage(text)) + } + m.distributeHeight() case app.StepCompleteEvent: // Flush any remaining streamed text to scrollback, then reset stream @@ -438,17 +469,21 @@ func (m *AppModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } // View implements tea.Model. It renders the stacked layout: -// stream region + separator + input region. +// stream region + separator + [queued messages] + input region. func (m *AppModel) View() tea.View { streamView := m.renderStream() separator := m.renderSeparator() inputView := m.renderInput() - content := lipgloss.JoinVertical(lipgloss.Left, - streamView, - separator, - inputView, - ) + parts := []string{streamView, separator} + + if queuedView := m.renderQueuedMessages(); queuedView != "" { + parts = append(parts, queuedView) + } + + parts = append(parts, inputView) + + content := lipgloss.JoinVertical(lipgloss.Left, parts...) return tea.NewView(content) } @@ -478,15 +513,16 @@ func (m *AppModel) renderStream() string { return m.stream.View().Content } -// renderSeparator renders the separator line with an optional queue badge. +// renderSeparator renders the separator line with an optional queue count badge. func (m *AppModel) renderSeparator() string { theme := GetTheme() lineStyle := lipgloss.NewStyle().Foreground(theme.Muted) + queueLen := len(m.queuedMessages) - if m.queueCount > 0 { + if queueLen > 0 { badge := lipgloss.NewStyle(). Foreground(theme.Secondary). - Render(fmt.Sprintf("%d queued", m.queueCount)) + Render(fmt.Sprintf("%d queued", queueLen)) // Fill the separator with dashes up to the badge. dashWidth := m.width - lipgloss.Width(badge) - 1 @@ -508,6 +544,40 @@ func (m *AppModel) renderInput() string { return m.input.View().Content } +// renderQueuedMessages renders queued prompts with "queued" badges, anchored +// between the separator and input. Each message is shown on a single line +// with a styled badge so the user can see what is pending. +func (m *AppModel) renderQueuedMessages() string { + if len(m.queuedMessages) == 0 { + return "" + } + theme := GetTheme() + + badgeStyle := lipgloss.NewStyle(). + Foreground(theme.Secondary). + Bold(true) + badge := badgeStyle.Render("queued") + badgeWidth := lipgloss.Width(badge) + + textStyle := lipgloss.NewStyle().Foreground(theme.Muted) + // Indent to align with the input container (2-space left padding). + indent := " " + + var lines []string + for _, msg := range m.queuedMessages { + // Truncate long messages to fit on one line: + // indent(2) + badge + gap(2) + text must fit within m.width. + maxTextWidth := m.width - len(indent) - badgeWidth - 2 + display := msg + if maxTextWidth > 3 && len(display) > maxTextWidth { + display = display[:maxTextWidth-3] + "..." + } + line := indent + badge + " " + textStyle.Render(display) + lines = append(lines, line) + } + return strings.Join(lines, "\n") +} + // -------------------------------------------------------------------------- // Print helpers — emit content to scrollback via tea.Println // -------------------------------------------------------------------------- @@ -613,6 +683,8 @@ func (m *AppModel) handleSlashCommand(sc *SlashCommand) tea.Cmd { if m.appCtrl != nil { m.appCtrl.ClearQueue() } + m.queuedMessages = m.queuedMessages[:0] + m.distributeHeight() return nil default: return m.printSystemMessage(fmt.Sprintf("Unknown command: %s", sc.Name)) @@ -722,18 +794,21 @@ func (m *AppModel) flushStreamContent() tea.Cmd { } // distributeHeight recalculates child component heights after a window resize -// and propagates the computed stream height to the StreamComponent. +// or queue change, and propagates the computed stream height to the +// StreamComponent. // // Layout (line counts): // -// stream region = total - separator(1) - input(5) +// stream region = total - separator(1) - queued(N) - input(5) // separator = 1 line +// queued msgs = len(queuedMessages) lines (0 when queue is empty) // input region = 5 lines: title(1) + textarea(3) + help(1) func (m *AppModel) distributeHeight() { const separatorLines = 1 const inputLines = 5 // title (1) + textarea (3) + help (1) + queuedLines := len(m.queuedMessages) - streamHeight := m.height - separatorLines - inputLines + streamHeight := m.height - separatorLines - queuedLines - inputLines if streamHeight < 0 { streamHeight = 0 } diff --git a/internal/ui/model_test.go b/internal/ui/model_test.go index 219b28cf..4babc200 100644 --- a/internal/ui/model_test.go +++ b/internal/ui/model_test.go @@ -23,8 +23,9 @@ type stubAppController struct { queueLen int } -func (s *stubAppController) Run(prompt string) { +func (s *stubAppController) Run(prompt string) int { s.runCalls = append(s.runCalls, prompt) + return s.queueLen } func (s *stubAppController) CancelCurrentStep() { @@ -286,36 +287,63 @@ func TestESCCancel_clearedOnStepComplete(t *testing.T) { } // -------------------------------------------------------------------------- -// Queue badge update +// Queued messages // -------------------------------------------------------------------------- -// TestQueueBadge_updatesOnEvent verifies that QueueUpdatedEvent sets queueCount. -func TestQueueBadge_updatesOnEvent(t *testing.T) { - ctrl := &stubAppController{} +// TestQueuedMessages_storedOnQueuedSubmit verifies that submitting a prompt +// while the agent is busy stores the text in queuedMessages (not scrollback). +func TestQueuedMessages_storedOnQueuedSubmit(t *testing.T) { + ctrl := &stubAppController{queueLen: 1} // simulate busy (will return 1) m, _, _ := newTestAppModel(ctrl) + m.state = stateWorking - if m.queueCount != 0 { - t.Fatalf("expected queueCount=0 initially, got %d", m.queueCount) + _, cmd := m.Update(submitMsg{Text: "queued prompt"}) + + if len(m.queuedMessages) != 1 { + t.Fatalf("expected 1 queued message, got %d", len(m.queuedMessages)) } - - m = sendMsg(m, app.QueueUpdatedEvent{Length: 3}) - - if m.queueCount != 3 { - t.Fatalf("expected queueCount=3 after QueueUpdatedEvent, got %d", m.queueCount) + if m.queuedMessages[0] != "queued prompt" { + t.Fatalf("expected queued message text 'queued prompt', got %q", m.queuedMessages[0]) + } + // Should NOT produce a tea.Println cmd (message is anchored, not in scrollback). + if cmd != nil { + t.Fatal("expected nil cmd for queued submit (message should not print to scrollback)") } } -// TestQueueBadge_resetsToZero verifies that a QueueUpdatedEvent with Length=0 -// resets the badge count. -func TestQueueBadge_resetsToZero(t *testing.T) { +// TestQueuedMessages_poppedOnQueueUpdated verifies that QueueUpdatedEvent pops +// consumed messages from queuedMessages and prints them to scrollback. +func TestQueuedMessages_poppedOnQueueUpdated(t *testing.T) { ctrl := &stubAppController{} m, _, _ := newTestAppModel(ctrl) - m.queueCount = 5 + m.queuedMessages = []string{"first", "second", "third"} + + // Simulate drainQueue popping one item (length goes from 3 to 2). + _, cmd := m.Update(app.QueueUpdatedEvent{Length: 2}) + + if len(m.queuedMessages) != 2 { + t.Fatalf("expected 2 queued messages after pop, got %d", len(m.queuedMessages)) + } + if m.queuedMessages[0] != "second" { + t.Fatalf("expected first remaining message 'second', got %q", m.queuedMessages[0]) + } + // Should produce a cmd (tea.Println for the popped user message). + if cmd == nil { + t.Fatal("expected non-nil cmd (tea.Println) for popped message") + } +} + +// TestQueuedMessages_allPoppedOnDrain verifies that QueueUpdatedEvent with +// Length=0 pops all remaining queued messages. +func TestQueuedMessages_allPoppedOnDrain(t *testing.T) { + ctrl := &stubAppController{} + m, _, _ := newTestAppModel(ctrl) + m.queuedMessages = []string{"alpha", "beta"} m = sendMsg(m, app.QueueUpdatedEvent{Length: 0}) - if m.queueCount != 0 { - t.Fatalf("expected queueCount=0 after QueueUpdatedEvent{Length:0}, got %d", m.queueCount) + if len(m.queuedMessages) != 0 { + t.Fatalf("expected 0 queued messages after drain, got %d", len(m.queuedMessages)) } }