Conversation
drorIvry
commented
Oct 9, 2025
drorIvry
commented
- Hotfix/auto updater bug (Hotfix/auto updater bug #56)
- Use clipboard from bubbletea instead of os binaries (HOTFIX | rogue-tui | Use clipboard from bubbletea instead of os binaries #57)
- Hotfix | version bump & pre-commit -> lefthook (Hotfix | version bump & pre-commit -> lefthook #58)
- focused support for interview
- scroll
- scroll
- refactor
- --wip-- [skip ci]
- more refactoring
- message history
- message history
- ui
- focuse
- size fixed
- themes
Summary by CodeRabbit
WalkthroughAdds two new TUI components (ChatView, MessageHistoryView), migrates multiple UI flows (interview, evaluations, reports, events) to use them, removes a legacy evaluation view file, and introduces new evaluation form/detail renderers plus markdown helpers. Input, focus, spinner, and rendering are routed through the new components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ChatView
participant History as MessageHistoryView
participant Model
participant API
User->>ChatView: Type input / Press Enter
ChatView->>History: AddMessage(role:"user", content)
ChatView->>Model: Submit input (via Update)
Model->>API: Send request
API-->>Model: Response / Error
Model->>ChatView: SetLoading(false) / SetError? / Add assistant message
ChatView->>History: AddMessage(role:"assistant", content)
User->>ChatView: Scroll / Change focus
ChatView->>History: Scroll / Focus update
sequenceDiagram
autonumber
actor User
participant Scenario
participant ChatView
participant Approver
User->>Scenario: Start Interview
Scenario->>ChatView: NewChatView / ClearMessages / SetLoading(true)
Scenario->>ChatView: Add assistant intro
User->>ChatView: Type / Enter
ChatView->>Scenario: Submit user message
Scenario->>ChatView: SetLoading(true)
Scenario-->>ChatView: Add assistant response / SetLoading(false)
alt Awaiting approval
Scenario->>Approver: Enter approval mode
User->>Approver: Tab / Shift+Tab / Enter / Esc
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/tui/internal/components/CHAT_VIEW_EXAMPLE.md(1 hunks)packages/tui/internal/components/MESSAGE_HISTORY_VIEW_EXAMPLE.md(1 hunks)packages/tui/internal/components/chat_view.go(1 hunks)packages/tui/internal/components/message_history_view.go(1 hunks)packages/tui/internal/components/scenario_editor.go(6 hunks)packages/tui/internal/components/scenario_interview.go(10 hunks)packages/tui/internal/components/scenario_list.go(1 hunks)packages/tui/internal/tui/app.go(5 hunks)packages/tui/internal/tui/eval_view.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/tui/internal/tui/eval_view.go (1)
packages/tui/internal/components/message_history_view.go (1)
Message(13-16)
packages/tui/internal/components/chat_view.go (4)
packages/tui/internal/components/message_history_view.go (3)
MessageHistoryView(19-40)NewMessageHistoryView(43-62)Message(13-16)packages/tui/internal/components/textarea.go (2)
TextArea(92-114)NewTextArea(117-142)packages/tui/internal/theme/theme.go (1)
Theme(10-78)packages/tui/internal/components/spinner.go (1)
SpinnerTickMsg(11-13)
packages/tui/internal/components/scenario_interview.go (2)
packages/tui/internal/components/dialog.go (3)
NewConfirmationDialog(81-95)DialogOpenMsg(60-62)Dialog(38-51)packages/tui/internal/components/scenario_editor.go (1)
ScenarioEditor(11-63)
packages/tui/internal/tui/app.go (3)
packages/tui/internal/components/message_history_view.go (2)
MessageHistoryView(19-40)NewMessageHistoryView(43-62)packages/tui/internal/components/viewport.go (2)
Viewport(52-71)NewViewport(74-93)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)
packages/tui/internal/components/message_history_view.go (3)
packages/tui/internal/components/viewport.go (2)
Viewport(52-71)NewViewport(74-93)packages/tui/internal/components/spinner.go (3)
Spinner(16-22)NewSpinner(25-35)SpinnerTickMsg(11-13)packages/tui/internal/theme/theme.go (1)
Theme(10-78)
packages/tui/internal/components/scenario_list.go (3)
packages/tui/internal/components/chat_view.go (1)
NewChatView(34-56)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/components/scenario_types.go (1)
StartInterviewMsg(42-42)
packages/tui/internal/components/scenario_editor.go (1)
packages/tui/internal/components/chat_view.go (1)
ChatView(13-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: codestyle
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/tui/internal/tui/eval_detail.go(1 hunks)packages/tui/internal/tui/eval_form.go(1 hunks)packages/tui/internal/tui/eval_helpers.go(1 hunks)packages/tui/internal/tui/eval_view.go(0 hunks)
💤 Files with no reviewable changes (1)
- packages/tui/internal/tui/eval_view.go
🧰 Additional context used
🧬 Code graph analysis (3)
packages/tui/internal/tui/eval_detail.go (4)
packages/tui/internal/tui/app.go (1)
Model(179-213)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/styles/styles.go (4)
Primary(13-13)Border(23-23)Success(15-15)TextMuted(22-22)packages/tui/internal/components/message_history_view.go (1)
Message(13-16)
packages/tui/internal/tui/eval_helpers.go (2)
packages/tui/internal/theme/theme.go (1)
Theme(10-78)packages/tui/internal/styles/styles.go (3)
NewStyle(45-47)Primary(13-13)Success(15-15)
packages/tui/internal/tui/eval_form.go (2)
packages/tui/internal/tui/app.go (1)
Model(179-213)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codestyle
- GitHub Check: rogue_sanity
| totalContentHeight := m.height - 8 // More conservative calculation to prevent footer override | ||
|
|
||
| var eventsHeight, summaryHeight int | ||
| var showSummary bool | ||
|
|
||
| // If evaluation completed and we have a summary OR are generating one, split 50/50 | ||
| if m.evalState.Completed && (m.evalState.Summary != "" || m.summarySpinner.IsActive()) { | ||
| showSummary = true | ||
| eventsHeight = (totalContentHeight / 2) - 2 // Reduced margin to prevent overflow | ||
| summaryHeight = totalContentHeight - eventsHeight - 1 // -1 for spacer between them | ||
| } else { | ||
| // No summary, events take full height with conservative margin | ||
| eventsHeight = totalContentHeight - 2 // Leave extra space to prevent footer override | ||
| summaryHeight = 0 | ||
| } |
There was a problem hiding this comment.
Clamp viewport heights before calling SetSize.
When the terminal is short (e.g., m.height <= 18), totalContentHeight becomes small enough that eventsHeight, summaryHeight, and the derived summaryHeight-10 turn negative. Passing negative dimensions into MessageHistoryView.SetSize or the summary viewport yields broken layouts and, in some cases, panics inside lipgloss. Please clamp these computed heights to a sane minimum before using them.
Here’s one way to harden the sizing math:
@@
- var eventsHeight, summaryHeight int
- var showSummary bool
+ minHeight := 1
+ clamp := func(v int) int {
+ if v < minHeight {
+ return minHeight
+ }
+ return v
+ }
+
+ var eventsHeight, summaryHeight int
+ var showSummary bool
@@
- eventsHeight = (totalContentHeight / 2) - 2 // Reduced margin to prevent overflow
- summaryHeight = totalContentHeight - eventsHeight - 1 // -1 for spacer between them
+ eventsHeight = clamp((totalContentHeight / 2) - 2) // Reduced margin to prevent overflow
+ summaryHeight = clamp(totalContentHeight - eventsHeight - 1) // -1 for spacer between them
@@
- eventsHeight = totalContentHeight - 2 // Leave extra space to prevent footer override
+ eventsHeight = clamp(totalContentHeight - 2) // Leave extra space to prevent footer override
summaryHeight = 0
@@
- m.eventsHistory.SetSize(m.width, eventsHeight)
+ m.eventsHistory.SetSize(m.width, clamp(eventsHeight))
@@
- m.summaryViewport.SetSize(m.width-4, summaryHeight-10)
+ m.summaryViewport.SetSize(m.width-4, clamp(summaryHeight-10))
@@
- .Height(summaryHeight - 6)
+ .Height(clamp(summaryHeight - 6))Also applies to: 187-205
🤖 Prompt for AI Agents
In packages/tui/internal/tui/eval_detail.go around lines 69-83 (and also apply
same fix to 187-205), the computed totalContentHeight/eventsHeight/summaryHeight
can become zero or negative on small terminals which breaks
MessageHistoryView.SetSize and the summary viewport; clamp these values to sane
minimums before calling SetSize (e.g., ensure totalContentHeight >= 1,
eventsHeight >= 1, summaryHeight >= 0 and adjust split logic accordingly), and
when computing derived sizes like summaryHeight-10 ensure you max with 0 so no
negative dimensions are passed into view SetSize calls.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tui/internal/components/message_history_view.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tui/internal/components/message_history_view.go (4)
packages/tui/internal/components/viewport.go (2)
Viewport(52-71)NewViewport(74-93)packages/tui/internal/components/spinner.go (3)
Spinner(16-22)NewSpinner(25-35)SpinnerTickMsg(11-13)packages/tui/internal/theme/theme.go (1)
Theme(10-78)packages/tui/internal/styles/styles.go (6)
NewStyle(45-47)TextMuted(22-22)Primary(13-13)Border(23-23)Style(40-42)Success(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codestyle
- GitHub Check: rogue_sanity
| func NewMessageHistoryView(id int, width, height int, t theme.Theme) *MessageHistoryView { | ||
| viewport := NewViewport(id, width-4, height) | ||
| viewport.WrapContent = true | ||
|
|
||
| spinner := NewSpinner(id + 100) | ||
|
|
||
| return &MessageHistoryView{ | ||
| messages: make([]Message, 0), | ||
| width: width, | ||
| height: height, | ||
| viewport: &viewport, | ||
| focused: false, | ||
| userPrefix: "👤 You: ", | ||
| assistantPrefix: "🤖 AI: ", | ||
| showSpinner: false, | ||
| spinner: &spinner, | ||
| userColor: t.Text(), // Will use theme Primary | ||
| assistantColor: t.Text(), // Will use theme Accent | ||
| } |
There was a problem hiding this comment.
Fix initial viewport height underflow
NewMessageHistoryView shrinks the viewport width by 4 to account for border + padding but leaves the height untouched. On first render (before any SetSize call), the viewport therefore believes it can draw height rows while the wrapper only allocates height-4, which clips the last four rows and reintroduces the overflow the previous review warned about. Please subtract the same 4 rows (and clamp at zero) when instantiating the viewport so the internal height always matches the available content box.
- viewport := NewViewport(id, width-4, height)
+ innerWidth := width - 4
+ if innerWidth < 0 {
+ innerWidth = 0
+ }
+ innerHeight := height - 4
+ if innerHeight < 0 {
+ innerHeight = 0
+ }
+ viewport := NewViewport(id, innerWidth, innerHeight)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewMessageHistoryView(id int, width, height int, t theme.Theme) *MessageHistoryView { | |
| viewport := NewViewport(id, width-4, height) | |
| viewport.WrapContent = true | |
| spinner := NewSpinner(id + 100) | |
| return &MessageHistoryView{ | |
| messages: make([]Message, 0), | |
| width: width, | |
| height: height, | |
| viewport: &viewport, | |
| focused: false, | |
| userPrefix: "👤 You: ", | |
| assistantPrefix: "🤖 AI: ", | |
| showSpinner: false, | |
| spinner: &spinner, | |
| userColor: t.Text(), // Will use theme Primary | |
| assistantColor: t.Text(), // Will use theme Accent | |
| } | |
| func NewMessageHistoryView(id int, width, height int, t theme.Theme) *MessageHistoryView { | |
| innerWidth := width - 4 | |
| if innerWidth < 0 { | |
| innerWidth = 0 | |
| } | |
| innerHeight := height - 4 | |
| if innerHeight < 0 { | |
| innerHeight = 0 | |
| } | |
| viewport := NewViewport(id, innerWidth, innerHeight) | |
| viewport.WrapContent = true | |
| spinner := NewSpinner(id + 100) | |
| return &MessageHistoryView{ | |
| messages: make([]Message, 0), | |
| width: width, | |
| height: height, | |
| viewport: &viewport, | |
| focused: false, | |
| userPrefix: "👤 You: ", | |
| assistantPrefix: "🤖 AI: ", | |
| showSpinner: false, | |
| spinner: &spinner, | |
| userColor: t.Text(), // Will use theme Primary | |
| assistantColor: t.Text(), // Will use theme Accent | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/tui/internal/components/message_history_view.go around lines 43 to
61, the viewport is created with width-4 but the height is left as-is causing a
4-row mismatch on first render; change the viewport instantiation to use max(0,
height-4) for its height (i.e., compute viewportHeight := height-4; if
viewportHeight < 0 { viewportHeight = 0 } and call NewViewport(id, width-4,
viewportHeight)) so the internal viewport height matches the available content
box.
| func (m *MessageHistoryView) SetSize(width, height int) { | ||
| m.width = width | ||
| m.height = height | ||
| if m.viewport != nil { | ||
| m.viewport.Width = width - 4 | ||
| // Account for border (2) + padding (2) = 4 lines total | ||
| m.viewport.Height = height - 4 | ||
| } |
There was a problem hiding this comment.
Guard viewport sizing against negative values
When the parent hands us a width/height smaller than the border/padding budget, width-4 or height-4 goes negative. We pass those straight into the viewport, which breaks scroll math and can trigger rendering glitches. Clamp both inner dimensions to >=0 inside SetSize before assigning them.
m.width = width
m.height = height
if m.viewport != nil {
- m.viewport.Width = width - 4
- // Account for border (2) + padding (2) = 4 lines total
- m.viewport.Height = height - 4
+ innerWidth := width - 4
+ if innerWidth < 0 {
+ innerWidth = 0
+ }
+ m.viewport.Width = innerWidth
+
+ innerHeight := height - 4
+ if innerHeight < 0 {
+ innerHeight = 0
+ }
+ m.viewport.Height = innerHeight
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *MessageHistoryView) SetSize(width, height int) { | |
| m.width = width | |
| m.height = height | |
| if m.viewport != nil { | |
| m.viewport.Width = width - 4 | |
| // Account for border (2) + padding (2) = 4 lines total | |
| m.viewport.Height = height - 4 | |
| } | |
| func (m *MessageHistoryView) SetSize(width, height int) { | |
| m.width = width | |
| m.height = height | |
| if m.viewport != nil { | |
| innerWidth := width - 4 | |
| if innerWidth < 0 { | |
| innerWidth = 0 | |
| } | |
| m.viewport.Width = innerWidth | |
| innerHeight := height - 4 | |
| if innerHeight < 0 { | |
| innerHeight = 0 | |
| } | |
| m.viewport.Height = innerHeight | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/tui/internal/components/message_history_view.go around lines 65 to
72, clamp the computed inner viewport dimensions to be non-negative before
assigning them: compute innerWidth = width - 4 and innerHeight = height - 4,
ensure each is at least 0 (e.g., take the max of 0 and the computed value), and
then assign those clamped values to m.viewport.Width and m.viewport.Height so
negative sizes are never passed into the viewport.
| // Calculate content height (viewport height) to prevent overflow | ||
| // Viewport is already sized to m.height - 4 to account for border/padding | ||
| contentHeight := m.height - 4 | ||
| if contentHeight < 0 { | ||
| contentHeight = 0 | ||
| } | ||
|
|
||
| historyStyle := lipgloss.NewStyle(). | ||
| Border(lipgloss.RoundedBorder()). | ||
| BorderForeground(borderColor). | ||
| Background(t.Background()). | ||
| Padding(1, 1). | ||
| Width(m.width - 4). | ||
| Height(contentHeight) | ||
|
|
There was a problem hiding this comment.
Base wrapper width/height on viewport dimensions
After correcting the viewport sizing, we should reuse those clamped values when rendering. Driving the wrapper off m.width-4/m.height-4 reintroduces negative widths on tiny layouts and diverges from the actual viewport height (the exact issue flagged in the earlier review). Please pull the width/height from m.viewport (falling back to zero when nil) before applying them to the Lip Gloss style.
- // Calculate content height (viewport height) to prevent overflow
- // Viewport is already sized to m.height - 4 to account for border/padding
- contentHeight := m.height - 4
- if contentHeight < 0 {
- contentHeight = 0
- }
+ contentHeight := 0
+ if m.viewport != nil {
+ contentHeight = m.viewport.Height
+ }
+
+ contentWidth := 0
+ if m.viewport != nil {
+ contentWidth = m.viewport.Width
+ }
historyStyle := lipgloss.NewStyle().
Border(lipgloss.RoundedBorder()).
BorderForeground(borderColor).
Background(t.Background()).
Padding(1, 1).
- Width(m.width - 4).
+ Width(contentWidth).
.Height(contentHeight)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Calculate content height (viewport height) to prevent overflow | |
| // Viewport is already sized to m.height - 4 to account for border/padding | |
| contentHeight := m.height - 4 | |
| if contentHeight < 0 { | |
| contentHeight = 0 | |
| } | |
| historyStyle := lipgloss.NewStyle(). | |
| Border(lipgloss.RoundedBorder()). | |
| BorderForeground(borderColor). | |
| Background(t.Background()). | |
| Padding(1, 1). | |
| Width(m.width - 4). | |
| Height(contentHeight) | |
| // Base wrapper size on the already-sized viewport (clamped to ≥0) | |
| contentHeight := 0 | |
| if m.viewport != nil { | |
| contentHeight = m.viewport.Height | |
| } | |
| contentWidth := 0 | |
| if m.viewport != nil { | |
| contentWidth = m.viewport.Width | |
| } | |
| historyStyle := lipgloss.NewStyle(). | |
| Border(lipgloss.RoundedBorder()). | |
| BorderForeground(borderColor). | |
| Background(t.Background()). | |
| Padding(1, 1). | |
| Width(contentWidth). | |
| Height(contentHeight) |
🤖 Prompt for AI Agents
In packages/tui/internal/components/message_history_view.go around lines 231 to
245, the wrapper style uses m.width-4 and a contentHeight derived from
m.height-4 which can become negative and diverges from the actual viewport;
instead read width and height from m.viewport (fall back to 0 if m.viewport is
nil), clamp both to >=0, then use those clamped viewportWidth and viewportHeight
values for the Lip Gloss style Width and Height (apply any padding/border
adjustments as needed, e.g., subtract 4 if the viewport already represents outer
size), ensuring you no longer reference m.width or m.height directly for the
wrapper dimensions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/tui/internal/components/message_history_view.go (3)
44-54: Clamp initial viewport dimensions.Width and height later lose four cells to border/padding, so feeding
width-4/heightdirectly means the viewport starts taller than the content box and even goes negative on tiny layouts. Clamp both inner dimensions up front and subtract the vertical padding so the viewport begins with sane, non-negative values.- viewport := NewViewport(id, width-4, height) + innerWidth := width - 4 + if innerWidth < 0 { + innerWidth = 0 + } + innerHeight := height - 4 + if innerHeight < 0 { + innerHeight = 0 + } + viewport := NewViewport(id, innerWidth, innerHeight)
65-72: GuardSetSizeagainst negative inner dimensions.If the parent hands us fewer than four rows/columns, the current math pushes negative values into the viewport, breaking scroll calculations and lipgloss rendering. Clamp the computed inner width/height before assigning them.
if m.viewport != nil { - m.viewport.Width = width - 4 - // Account for border (2) + padding (2) = 4 lines total - m.viewport.Height = height - 4 + innerWidth := width - 4 + if innerWidth < 0 { + innerWidth = 0 + } + m.viewport.Width = innerWidth + + innerHeight := height - 4 + if innerHeight < 0 { + innerHeight = 0 + } + // Account for border (2) + padding (2) = 4 lines total + m.viewport.Height = innerHeight }
239-252: Drive the wrapper size from the viewport.Even after clamping the viewport, the wrapper style still recomputes
m.width-4/m.height-4, reintroducing negative/overflow dimensions and causing the border to overrun. Base the style width/height on the sanitized viewport measurements instead.- // Calculate content height (viewport height) to prevent overflow - // Viewport is already sized to m.height - 4 to account for border/padding - contentHeight := m.height - 4 - if contentHeight < 0 { - contentHeight = 0 - } + contentWidth := 0 + contentHeight := 0 + if m.viewport != nil { + contentWidth = m.viewport.Width + if contentWidth < 0 { + contentWidth = 0 + } + contentHeight = m.viewport.Height + if contentHeight < 0 { + contentHeight = 0 + } + } historyStyle := lipgloss.NewStyle(). Border(lipgloss.RoundedBorder()). BorderForeground(borderColor). Background(t.Background()). Padding(1, 1). - Width(m.width - 4). + Width(contentWidth). .Height(contentHeight)packages/tui/internal/tui/eval_detail.go (1)
71-206: Prevent negative heights when splitting the evaluation layout.For small windows the current math makes
remainingHeight(and thuseventsHeight/summaryHeight) negative, so we pass invalid dimensions intoSetSizeandlipgloss.Place, breaking the UI. Clamp the intermediate values before splitting them.- // Start with total height minus header and help - availableHeight := m.height - 5 // header(3) + helpText(2) + // Start with total height minus header and help + availableHeight := m.height - 5 // header(3) + helpText(2) + if availableHeight < 0 { + availableHeight = 0 + } @@ - // Remaining height split between events and summary - remainingHeight := availableHeight - statusAndSpacersHeight - eventsHeight = remainingHeight / 2 - summaryHeight = remainingHeight - eventsHeight + remainingHeight := availableHeight - statusAndSpacersHeight + if remainingHeight < 0 { + remainingHeight = 0 + } + + if showSummary { + eventsHeight = remainingHeight / 2 + summaryHeight = remainingHeight - eventsHeight + } else { + eventsHeight = remainingHeight + summaryHeight = 0 + } - remainingHeight := availableHeight - statusAndSpacersHeight - eventsHeight = remainingHeight - summaryHeight = 0 - } + } @@ - mainContentHeight := availableHeight + mainContentHeight := availableHeight
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/tui/internal/components/message_history_view.go(1 hunks)packages/tui/internal/tui/app.go(7 hunks)packages/tui/internal/tui/eval_detail.go(1 hunks)packages/tui/internal/tui/report_view.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/tui/internal/tui/report_view.go (1)
packages/tui/internal/styles/styles.go (2)
Success(15-15)NewStyle(45-47)
packages/tui/internal/tui/app.go (3)
packages/tui/internal/components/message_history_view.go (2)
MessageHistoryView(19-40)NewMessageHistoryView(43-62)packages/tui/internal/components/viewport.go (2)
Viewport(52-71)NewViewport(74-93)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)
packages/tui/internal/components/message_history_view.go (3)
packages/tui/internal/components/viewport.go (2)
Viewport(52-71)NewViewport(74-93)packages/tui/internal/components/spinner.go (3)
Spinner(16-22)NewSpinner(25-35)SpinnerTickMsg(11-13)packages/tui/internal/theme/theme.go (1)
Theme(10-78)
packages/tui/internal/tui/eval_detail.go (4)
packages/tui/internal/tui/app.go (1)
Model(179-213)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/styles/styles.go (6)
NewStyle(45-47)Primary(13-13)Border(23-23)Style(40-42)Success(15-15)TextMuted(22-22)packages/tui/internal/components/message_history_view.go (1)
Message(13-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: codestyle
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tui/internal/tui/app.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tui/internal/tui/app.go (3)
packages/tui/internal/components/message_history_view.go (2)
MessageHistoryView(19-40)NewMessageHistoryView(43-62)packages/tui/internal/components/viewport.go (2)
Viewport(52-71)NewViewport(74-93)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)
🔇 Additional comments (12)
packages/tui/internal/tui/app.go (12)
202-206: LGTM! Field declarations properly refactored.The migration from
Viewportto*MessageHistoryViewfor events, summary, and report is correctly implemented. Using pointers is appropriate for these components, and thehelpViewportcorrectly remains as a regularViewport.
288-292: LGTM! Initialization correctly refactored.The MessageHistoryView components are properly initialized with unique IDs, reasonable default dimensions, and the current theme. The dimensions will be updated on the first
tea.WindowSizeMsg, so the hardcoded values are acceptable.
399-402: LGTM! Auto-scroll behavior correctly configured.Blurring the events history when entering the evaluation detail screen is the correct approach to enable auto-scroll for new evaluation events. The nil check prevents potential panics.
431-439: LGTM! Window resize handling properly updated.The resize logic correctly applies to all message history views with appropriate nil guards. The pattern is consistent and defensive.
944-949: LGTM! Report screen navigation correctly updated.Blurring the report history before transitioning back to the evaluation detail screen is appropriate and prevents stale focus state.
1210-1214: LGTM! Report navigation correctly configured.Focusing the report history view when entering the report screen enables immediate user interaction for scrolling, which is good UX.
1224-1243: LGTM! End/Home navigation with smart focus management.The implementation correctly pairs navigation with focus state:
End: Jump to bottom + blur (enables auto-scroll)Home: Jump to top + focus (disables auto-scroll)This prevents auto-scroll from disrupting users viewing historical content, while allowing them to easily re-enable it.
1305-1308: LGTM! Explicit default case improves clarity.Adding an explicit default case that returns early prevents unintended key handling and makes the code's behavior more predictable.
1315-1319: LGTM! Report screen back navigation correctly implemented.Blurring the report history before returning to the dashboard maintains consistent focus state management.
1329-1340: LGTM! Report navigation simplified appropriately.Since reports are static (no auto-scroll needed), the simpler
GotoTop()/GotoBottom()without focus management is correct.
1341-1363: Consistent pattern, but verify redundant Update() calls.This follows the same pattern as the evaluation detail screen scrolling (lines 1244-1308), where explicit scroll methods are called before passing the key to
Update(). See the earlier comment about potential double-scrolling.Otherwise, the implementation is correct:
- Comprehensive key handling
- Proper nil guards
- Explicit default case
1244-1308: No redundant scrolling—MessageHistoryView.Updatedoesn’t process arrow keys
Updateonly handles spinner ticks and view re-renders; it won’t double-scroll after your explicitScrollUp/ScrollDowncalls.
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
LGTM, left a few suggestions
|
|
||
| case "shift+enter": | ||
| // Insert newline in the input | ||
| if c.input != nil && !c.IsLoading() && !c.IsViewportFocused() { |
There was a problem hiding this comment.
Should we implement a new isInputFocused function? In case we will add more components to this in the future, this might be easier to maintain
| if c.input != nil && !c.IsLoading() && !c.IsViewportFocused() { | |
| if c.input != nil && !c.IsLoading() && c.isInputFocused() { |
|
|
||
| default: | ||
| // Forward to TextArea for text input (only when input is focused) | ||
| if c.input != nil && !c.IsLoading() && !c.IsViewportFocused() { |
There was a problem hiding this comment.
same comment as this https://github.com/qualifire-dev/rogue/pull/59/files#r2423383668
Co-authored-by: yuval-qf <yuval@qualifire.ai>
Co-authored-by: yuval-qf <yuval@qualifire.ai>
Co-authored-by: yuval-qf <yuval@qualifire.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/tui/internal/components/chat_view.go (1)
36-37: Derive history height instead of hard-coding 24.The fixed
height-24subtraction survives from the earlier revision and still breaks on modest terminal sizes (≤24 rows). It drives theMessageHistoryViewviewport height negative, which underflows the viewport math and can panic or blank the history area. Please compute the available space from the actual header/input/help blocks (clamp to ≥0) rather than relying on a magic constant.Also applies to: 63-64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tui/internal/components/chat_view.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tui/internal/components/chat_view.go (5)
packages/tui/internal/components/message_history_view.go (3)
MessageHistoryView(19-40)NewMessageHistoryView(43-62)Message(13-16)packages/tui/internal/components/textarea.go (2)
TextArea(92-114)NewTextArea(117-142)packages/tui/internal/theme/theme.go (1)
Theme(10-78)packages/tui/internal/components/spinner.go (1)
SpinnerTickMsg(11-13)packages/tui/internal/styles/whitespace.go (1)
WhitespaceStyle(8-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: codestyle
| case "down": | ||
| // When viewport is focused: scroll down or move to input if at bottom | ||
| if !c.IsLoading() { | ||
| if c.IsViewportFocused() { | ||
| // Check if viewport is at bottom | ||
| if c.messageHistory != nil && !c.messageHistory.AtBottom() { | ||
| // Not at bottom - scroll down | ||
| c.messageHistory.ScrollDown(1) | ||
| } else { | ||
| // At bottom or messageHistory is nil - shift focus to input | ||
| c.FocusInput() | ||
| } | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Let the TextArea handle ↓ when it has focus.
Catching "down" here always returns without forwarding the event, so a focused TextArea never sees the key. That prevents cursor navigation inside multi-line input (you can add lines via Shift+Enter but can’t move the caret down to edit them), which is a serious UX regression. Only intercept the key when the viewport is focused; otherwise delegate to the TextArea (c.input.Update(msg)) so multiline editing keeps working.
🤖 Prompt for AI Agents
In packages/tui/internal/components/chat_view.go around lines 256-270, the
handler for the "down" key unconditionally returns without forwarding the event
which prevents a focused TextArea from receiving the key; change the logic so
you only intercept "down" when the viewport is focused: if the viewport is
focused handle scroll or shift focus to input as currently implemented,
otherwise forward the message to the TextArea by calling c.input.Update(msg)
(and return its result) so multiline caret navigation works.
Co-authored-by: yuval-qf <yuval@qualifire.ai>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/tui/internal/components/message_history_view.go (4)
298-310: Custom role prefix question remains.The past review on line 302 asked about custom role handling. The current code defaults any non-"assistant" and non-"system" role to the user prefix and color (lines 303-310), which may not be the intended behavior if custom roles are supposed to have distinct styling.
</review_comment_end -->
43-62: Viewport height mismatch remains unfixed.The constructor still creates the viewport with the full
heightparameter while the wrapper (inView) allocates onlyheight-4for content. This 4-row discrepancy causes the viewport to believe it can render more lines than the styled container actually provides, leading to clipping or overflow on first render before anySetSizecall.</thinking_end -->
65-73: Negative dimension guards still missing.
SetSizestill computeswidth-4andheight-4without clamping to zero, so tight parent layouts can pass negative values into the viewport and break scroll calculations.</review_comment_end -->
236-250: Wrapper dimensions still computed from m.height rather than viewport.The lipgloss wrapper still derives
contentHeightfromm.height-4instead of readingm.viewport.Height. This reintroduces the sizing mismatch flagged in earlier reviews: if the viewport was clamped or adjusted, the wrapper won't reflect that, causing visual overflow or clipping.</review_comment_end -->
🧹 Nitpick comments (1)
packages/tui/internal/components/message_history_view.go (1)
350-364: Emoji width detection is oversimplified.The heuristic
r >= 0x1F300 && r <= 0x1FAFFcaptures some common emoji blocks but misses many others (e.g., emoticons at 0x2600-0x26FF, miscellaneous symbols, variation selectors, combining characters, and zero-width joiners). This can cause misalignment when messages contain emojis outside this range.Consider using a more comprehensive emoji/width detection library (e.g.,
github.com/mattn/go-runewidthorgithub.com/rivo/uniseg) or expanding the range checks to cover additional emoji blocks and wide characters.+import ( + "github.com/mattn/go-runewidth" +) + func calculateVisualWidth(s string) int { - width := 0 - for _, r := range s { - // Emojis and wide unicode characters typically take 2 columns - // Simple heuristic: if rune is > 0x1F300, it's likely an emoji - if r >= 0x1F300 && r <= 0x1FAFF { - width += 2 - } else { - width += 1 - } - } - return width + return runewidth.StringWidth(s) }</review_comment_end -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tui/internal/components/message_history_view.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tui/internal/components/message_history_view.go (3)
packages/tui/internal/components/viewport.go (2)
Viewport(52-71)NewViewport(74-93)packages/tui/internal/components/spinner.go (3)
Spinner(16-22)NewSpinner(25-35)SpinnerTickMsg(11-13)packages/tui/internal/theme/theme.go (1)
Theme(10-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: rogue_sanity
- GitHub Check: codestyle
🔇 Additional comments (2)
packages/tui/internal/components/message_history_view.go (2)
114-120: StartSpinner implementation is appropriate.While the past review suggested simplifying this to
m.SetSpinner(true), the current implementation correctly returnsm.spinner.Start()which provides thetea.Cmdnecessary for the Bubble Tea update loop to animate the spinner. This cannot be reduced to just aSetSpinnercall without losing the animation command.</review_comment_end -->
330-330: Confirm wrapText availability: wrapText is implemented in packages/tui/internal/components/scenario_helpers.go; dependency present.
…b.com/qualifire-dev/rogue-private into feature/scrolling-and-wrapping-issues
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/tui/internal/components/chat_view.go (3)
34-56: Still breaks on small terminals: height-24 causes underflow.Line 36 subtracts a magic constant (24) from height without clamping, so on typical 20–24 line terminals or smaller windows the messageHistory height goes negative, causing the viewport to panic or render nothing. This is a critical blocker already flagged in past reviews but not yet fixed.
Derive the split from actual subcomponent heights (header + label + input + help padding) and clamp to ≥1 instead of subtracting a constant.
- messageHistory := NewMessageHistoryView(id, width, height-24, t) + // Calculate available height for message history + // Reserve space for: input label (~3 lines), input area (~6 lines), help/error (~3 lines) + reservedHeight := 12 + historyHeight := height - reservedHeight + if historyHeight < 1 { + historyHeight = 1 + } + messageHistory := NewMessageHistoryView(id, width, historyHeight, t)
58-68: Same underflow issue in SetSize.Line 63 repeats the brittle height-24 constant that panics on small terminals, duplicating the critical issue in the constructor.
Apply the same fix here:
func (c *ChatView) SetSize(width, height int) { c.width = width c.height = height if c.messageHistory != nil { - c.messageHistory.SetSize(width, height-24) + reservedHeight := 12 + historyHeight := height - reservedHeight + if historyHeight < 1 { + historyHeight = 1 + } + c.messageHistory.SetSize(width, historyHeight) } if c.input != nil { c.input.Width = width - 4 } }
264-278: Still blocks TextArea down-arrow navigation.The critical UX issue flagged in past reviews remains: when the input is focused, "down" is caught at line 264 but only processed if
IsViewportFocused()is true (line 267). If the viewport is not focused, line 278 returns nil without forwarding the event to the TextArea, so users cannot move the cursor down in multi-line input (you can add lines with Shift+Enter but cannot navigate to edit them).Apply this diff to forward the key when input is focused:
case "down": - // When viewport is focused: scroll down or move to input if at bottom if !c.IsLoading() { if c.IsViewportFocused() { + // Viewport focused: scroll down or move to input if at bottom if c.messageHistory != nil && !c.messageHistory.AtBottom() { - // Not at bottom - scroll down c.messageHistory.ScrollDown(1) } else { - // At bottom or messageHistory is nil - shift focus to input c.FocusInput() } + } else if c.IsInputFocused() { + // Input focused: forward down arrow to TextArea for cursor navigation + if c.input != nil { + updatedTextArea, cmd := c.input.Update(msg) + *c.input = *updatedTextArea + return cmd + } } } return nil
🧹 Nitpick comments (1)
packages/tui/internal/components/chat_view.go (1)
144-150: Direct field access breaks encapsulation.Line 147 directly accesses
c.messageHistory.showSpinner, bypassing any potential getter logic. IfMessageHistoryViewexposes a method for this (e.g.,IsSpinnerActive()), use it; otherwise this creates tight coupling and makes refactoring harder.Check if MessageHistoryView has a getter:
#!/bin/bash # Description: Check if MessageHistoryView has a spinner state getter method. ast-grep --pattern $'type MessageHistoryView struct { $$$ } $$$ func ($_ *MessageHistoryView) IsSpinnerActive() $_ { $$$ }'If no getter exists, either add one or document why direct access is acceptable here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tui/internal/components/chat_view.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tui/internal/components/chat_view.go (6)
packages/tui/internal/components/message_history_view.go (3)
MessageHistoryView(19-40)NewMessageHistoryView(43-62)Message(13-16)packages/tui/internal/components/textarea.go (2)
TextArea(92-114)NewTextArea(117-142)packages/tui/internal/theme/theme.go (1)
Theme(10-78)packages/tui/internal/components/spinner.go (1)
SpinnerTickMsg(11-13)packages/tui/internal/styles/styles.go (5)
NewStyle(45-47)Primary(13-13)TextMuted(22-22)Border(23-23)Error(17-17)packages/tui/internal/styles/whitespace.go (1)
WhitespaceStyle(8-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codestyle
- GitHub Check: rogue_sanity
🔇 Additional comments (2)
packages/tui/internal/components/chat_view.go (2)
350-350: Good: Pre-allocated slice capacity.The suggestion from past reviews to pre-allocate slice capacity has been implemented, improving performance by avoiding potential reallocations.
280-286: Remove Update call suggestion for shift+enter
TextArea.InsertNewline()already mutates the buffer and callsupdateViewport(). Invokingc.input.Update(msg)for “shift+enter” wouldn’t insert a newline (it only matches plain “enter”/“ctrl+m”). The existing code is correct.Likely an incorrect or invalid review comment.
| // IsViewportFocused returns true if viewport is focused | ||
| func (c *ChatView) IsInputFocused() bool { | ||
| if c.input != nil { | ||
| return c.input.IsFocused() | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Fix copy-paste error in doc comment.
Line 185's comment incorrectly says "IsViewportFocused" when the method is IsInputFocused.
-// IsViewportFocused returns true if viewport is focused
+// IsInputFocused returns true if input is focused
func (c *ChatView) IsInputFocused() bool {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // IsViewportFocused returns true if viewport is focused | |
| func (c *ChatView) IsInputFocused() bool { | |
| if c.input != nil { | |
| return c.input.IsFocused() | |
| } | |
| return false | |
| } | |
| // IsInputFocused returns true if input is focused | |
| func (c *ChatView) IsInputFocused() bool { | |
| if c.input != nil { | |
| return c.input.IsFocused() | |
| } | |
| return false | |
| } |
🤖 Prompt for AI Agents
In packages/tui/internal/components/chat_view.go around lines 185 to 191, the
doc comment above the IsInputFocused method mistakenly reads
"IsViewportFocused"; update the comment to correctly describe the method (e.g.,
"IsInputFocused returns true if input is focused") so it matches the method name
and behavior, keeping Go doc comment style.
| case "up": | ||
| // When viewport is focused: scroll up | ||
| // When input is focused: move focus to viewport | ||
| if !c.IsLoading() { | ||
| if c.IsViewportFocused() { | ||
| // Scroll viewport up | ||
| if c.messageHistory != nil { | ||
| c.messageHistory.ScrollUp(1) | ||
| } | ||
| } else { | ||
| // Move focus to viewport | ||
| c.FocusViewport() | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Up arrow also blocks TextArea navigation.
Same issue as "down": when the input is focused, "up" moves focus to the viewport instead of forwarding to the TextArea, preventing users from moving the cursor up in multi-line input.
Apply this diff to forward the key when input is focused:
case "up":
- // When viewport is focused: scroll up
- // When input is focused: move focus to viewport
if !c.IsLoading() {
if c.IsViewportFocused() {
- // Scroll viewport up
+ // Viewport focused: scroll up
if c.messageHistory != nil {
c.messageHistory.ScrollUp(1)
}
- } else {
- // Move focus to viewport
+ } else if c.IsInputFocused() {
+ // Input focused: forward up arrow to TextArea for cursor navigation
+ if c.input != nil {
+ updatedTextArea, cmd := c.input.Update(msg)
+ *c.input = *updatedTextArea
+ return cmd
+ }
+ } else {
+ // Neither focused: move focus to viewport
c.FocusViewport()
}
}
return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "up": | |
| // When viewport is focused: scroll up | |
| // When input is focused: move focus to viewport | |
| if !c.IsLoading() { | |
| if c.IsViewportFocused() { | |
| // Scroll viewport up | |
| if c.messageHistory != nil { | |
| c.messageHistory.ScrollUp(1) | |
| } | |
| } else { | |
| // Move focus to viewport | |
| c.FocusViewport() | |
| } | |
| } | |
| return nil | |
| case "up": | |
| if !c.IsLoading() { | |
| if c.IsViewportFocused() { | |
| // Viewport focused: scroll up | |
| if c.messageHistory != nil { | |
| c.messageHistory.ScrollUp(1) | |
| } | |
| } else if c.IsInputFocused() { | |
| // Input focused: forward up arrow to TextArea for cursor navigation | |
| if c.input != nil { | |
| updatedTextArea, cmd := c.input.Update(msg) | |
| *c.input = *updatedTextArea | |
| return cmd | |
| } | |
| } else { | |
| // Neither focused: move focus to viewport | |
| c.FocusViewport() | |
| } | |
| } | |
| return nil |
🤖 Prompt for AI Agents
In packages/tui/internal/components/chat_view.go around lines 248-262, the "up"
key handling currently moves focus to the viewport when the input is focused,
blocking TextArea navigation; instead, detect when the input/TextArea is focused
and forward the key event to the TextArea component (call its existing key
handler or movement method) so the caret/selection moves up in multi-line input,
otherwise keep the existing behavior of scrolling the viewport or focusing it;
mirror the same forwarding approach used to fix the "down" key case.
2 participants
