Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the Summary by CodeRabbit
WalkthroughConverted many receiver-based screen renderers and input handlers to stateless, parameterized functions; reorganized packages (shared, config, scenarios, report, evaluations, help, dashboard); promoted config persistence to exported Save/Load; added text-wrapping in message history; updated app and controllers to call new renderers and handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Keyboard
participant Handler
participant Screen
rect rgb(245,250,255)
note right of App: Stateless handlers & renderers
App->>Keyboard: receive key msg
Keyboard->>Handler: route to exported Handle*(m, msg)
Handler-->>App: (Model, tea.Cmd)
App->>Screen: call Render(width,height, params...)
Screen-->>App: rendered string
end
sequenceDiagram
participant View
participant ConfigPkg as config
participant FS
View->>ConfigPkg: Save(cfg)
ConfigPkg->>ConfigPkg: toml.Marshal(cfg)
ConfigPkg->>FS: write file
FS-->>ConfigPkg: ok / error
ConfigPkg-->>View: return error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
packages/tui/internal/tui/eval_types.go (2)
49-84: Data races: goroutine mutates EvaluationViewState directlyThe goroutine appends to st.Events and writes multiple st fields without synchronization. In Bubble Tea, state should be updated on the UI thread via messages; this will otherwise race with reads in Update/View and cause nondeterminism.
Recommended:
- Emit a TeaMsg (e.g., EvalEventMsg) via program.Send/tea.Cmd and mutate state inside Update.
- If you must keep background writes, guard EvaluationViewState with a mutex and batch updates.
Example pattern (conceptual):
// in producer goroutine for ev := range ch { m.program.Send(EvalEventMsg{Event: ev}) } // in Update(msg tea.Msg) case EvalEventMsg: st.Events = append(st.Events, msg.Event) // update st.Status/Progress/JobID/Completed hereThis also solves the repaint TODO and avoids slice races.
86-99: Function name vs behavior: spinner starts but no summary job is triggeredtriggerSummaryGeneration only activates the spinner; no work is started, so the UI may spin indefinitely.
Either:
- Kick off a summary job (goroutine + tea.Cmd) and set st.Summary on completion, or
- Rename to indicate it's a UI-only spinner toggle and call it from where actual generation starts.
packages/tui/internal/shared/markdown_renderer.go (2)
21-27: Don’t ignore glamour.NewTermRenderer error — avoid returning a nil rendererIf NewTermRenderer fails, r will be nil and later use will panic. Handle the error and provide a safe fallback.
Apply this diff:
-func GetMarkdownRenderer(width int, backgroundColor compat.AdaptiveColor) *glamour.TermRenderer { - r, _ := glamour.NewTermRenderer( +func GetMarkdownRenderer(width int, backgroundColor compat.AdaptiveColor) *glamour.TermRenderer { + r, err := glamour.NewTermRenderer( glamour.WithStyles(generateMarkdownStyleConfig(backgroundColor)), glamour.WithWordWrap(width), glamour.WithChromaFormatter("terminal16m"), ) - return r + if err != nil { + // Fallback: minimal renderer to keep UI functional + fallback, _ := glamour.NewTermRenderer(glamour.WithWordWrap(width)) + return fallback + } + return r
268-276: Fix AdaptiveColorToString: honor light/dark and handle NoColor safelyCurrently always uses Dark and mixes v1/v2 types. Use HasDarkBackground(), handle NoColor, and don’t return a color on conversion failure.
-func AdaptiveColorToString(color compat.AdaptiveColor) *string { - if _, ok := color.Dark.(lipgloss.NoColor); ok { - return nil - } - c1, _ := colorful.MakeColor(color.Dark) - return stringPtr(c1.Hex()) +func AdaptiveColorToString(ac compat.AdaptiveColor) *string { + var raw any + if lipgloss.HasDarkBackground() { + raw = ac.Dark + } else { + raw = ac.Light + } + if _, ok := raw.(lipgloss.NoColor); ok { + return nil + } + c1, err := colorful.MakeColor(raw) + if err != nil { + return nil + } + hex := c1.Hex() + return &hex }packages/tui/internal/components/message_history_view.go (1)
363-372: Markdown messages lose styled inline prefix and alignmentPrefix is emitted as a separate, unstyled line, and continuation lines aren’t indented. Compose the styled prefix with the first markdown line and indent the rest without overriding Glamour’s ANSI.
- if m.renderMarkdown && m.markdownRenderer != nil && msg.Role != "system" { - // Render markdown content - rendered, err := m.markdownRenderer.Render(msg.Content) - if err != nil { - // Fall back to plain text if markdown rendering fails - rendered = msg.Content - } - - messageLines = append(messageLines, prefix, rendered) + if m.renderMarkdown && m.markdownRenderer != nil && msg.Role != "system" { + rendered, err := m.markdownRenderer.Render(msg.Content) + if err != nil { + rendered = msg.Content + } + // Compose prefix with first line; indent continuations + prefixVisualWidth := calculateVisualWidth(prefix) + indentation := strings.Repeat(" ", prefixVisualWidth) + lines := strings.Split(strings.TrimRight(rendered, "\n"), "\n") + if len(lines) == 0 { + lines = []string{""} + } + // Style only the prefix; keep Glamour colors intact + first := lipgloss.JoinHorizontal(lipgloss.Left, textStyle.Render(prefix), lines[0]) + messageLines = append(messageLines, first) + for i := 1; i < len(lines); i++ { + messageLines = append(messageLines, indentation+lines[i]) + }packages/tui/internal/screens/config/persistence.go (1)
21-23: Tighten config perms (secrets inside) — use 0700 dir and 0600 fileConfig contains API keys; 0755/0644 are too permissive.
- if err := os.MkdirAll(rogueConfigDir, 0755); err != nil { + if err := os.MkdirAll(rogueConfigDir, 0700); err != nil { return fmt.Errorf("failed to create config directory: %w", err) } @@ - if err := os.WriteFile(configFile, data, 0644); err != nil { + if err := os.WriteFile(configFile, data, 0600); err != nil { return fmt.Errorf("failed to write config file: %w", err) }Optional: write atomically (tmp file + rename) to avoid partial writes on crash.
Also applies to: 35-37
packages/tui/internal/tui/app.go (2)
22-27: Critical: fallback sets unregistered theme "aura"; leads to nil theme and panicsWhen theme loading fails you register "default" but set "aura". CurrentTheme() becomes nil and any t.Background() etc. will panic. Set the registered default instead.
if err := theme.LoadThemesFromJSON(); err != nil { fmt.Printf("Warning: Failed to load themes: %v\n", err) // Create a fallback theme if loading fails theme.RegisterTheme("default", theme.NewSystemTheme(nil, true)) - theme.SetTheme("aura") + theme.SetTheme("default") }
66-67: Ensure a valid theme after loading configIf config specifies a non-existent theme, CurrentTheme() can be nil. Guard and fall back to a safe default.
theme.SetTheme(model.config.Theme) + if theme.CurrentTheme() == nil { + // Ensure a usable theme exists + theme.RegisterTheme("default", theme.NewSystemTheme(nil, true)) + theme.SetTheme("default") + }packages/tui/internal/tui/config_controller.go (1)
123-134: Editing Server URL: navigating Down discards pending editsWhen leaving the Server URL field with "down", IsEditing is set false but m.config.ServerURL isn’t updated nor saved. Apply on exit.
- if m.configState.IsEditing { - m.configState.IsEditing = false - } + if m.configState.IsEditing { + m.configState.IsEditing = false + // Apply and persist server URL on exit + m.config.ServerURL = m.configState.ServerURL + m.configState.HasChanges = true + if err := config.Save(&m.config); err != nil { + fmt.Println("failed to save config:", err) + } + }Mirror the same pattern in the "up" path when moving away from Server URL if applicable.
packages/tui/internal/screens/report/view.go (1)
20-28: Guard against nil themeSame crash risk as other screens; add a minimal fallback.
- if evalState == nil { + if evalState == nil { return lipgloss.NewStyle(). Width(width). Height(height). Background(t.Background()). Foreground(t.Text()). Align(lipgloss.Center, lipgloss.Center). Render("No evaluation report available") } + if t == nil { + return "No evaluation report available (theme not initialized)" + }packages/tui/internal/tui/common_controller.go (1)
280-296: Config file with API keys stored with world-readable permissions.The config file saved at
packages/tui/internal/screens/config/persistence.go:35usesos.WriteFile(configFile, data, 0644)and contains sensitive fields includingAPIKeysandQualifireAPIKey. Change permissions to0600to restrict read access to the owner only.
🧹 Nitpick comments (31)
packages/tui/internal/screens/scenarios/edit.go (1)
246-280: Remove unusedrenderTextAreafunction.Verification confirms this function is never called anywhere in the codebase. Since it's marked as "legacy, may be unused" and has no callers, remove it along with any helper functions it exclusively depends on.
Lines 246-280 in
packages/tui/internal/screens/scenarios/edit.go.packages/tui/internal/tui/eval_types.go (2)
12-47: Minor robustness: handle Getwd error and return empty sliceSafer defaults improve callers: handle Getwd failure and prefer empty slice over nil when nothing is found.
Apply:
- func loadScenariosFromWorkdir() []EvalScenario { - wd, _ := os.Getwd() + func loadScenariosFromWorkdir() []EvalScenario { + wd, err := os.Getwd() + if err != nil { + return []EvalScenario{} + } dir := wd @@ - return nil + return []EvalScenario{} }
10-10: Comment clarity“The EvaluationViewState is now defined in tui package” can confuse within package tui. Consider clarifying the module path (e.g., “defined in this package in another file” or the exact package path) to avoid ambiguity.
packages/tui/internal/shared/markdown_renderer.go (2)
36-71: Select light/dark base styles dynamically, not always “dark”Using styles.DefaultStyles["dark"] on light terminals yields low-contrast headings. Detect background and choose “light” when appropriate.
Apply this diff:
- darkTheme := styles.DefaultStyles["dark"] + themeKey := "dark" + if !lipgloss.HasDarkBackground() { + themeKey = "light" + } + darkTheme := styles.DefaultStyles[themeKey]
241-247: Avoid setting BackgroundColor to empty stringAn empty string may be treated as an invalid color; nil lets Glamour use defaults.
- BackgroundColor: stringPtr(""), + BackgroundColor: nil,packages/tui/internal/screens/scenarios/scenario_types.go (1)
35-37: Remove stale comment for deleted InterviewMessageThis empty comment block is a leftover and can confuse readers.
-// InterviewMessage represents a message in the interview conversation -packages/tui/internal/components/message_history_view.go (2)
377-381: Use viewport width (no magic -8) and avoid hard minimumsavailableWidth should derive from m.viewport.Width to reflect actual content area. A hard minimum of 40 can overflow narrow layouts.
- availableWidth := m.width - prefixVisualWidth - 8 - if availableWidth < 40 { - availableWidth = 40 - } + vw := m.viewport.Width + if vw <= 0 { + vw = m.width - 4 // fallback: border/padding + } + availableWidth := vw - prefixVisualWidth + if availableWidth < 10 { + availableWidth = 10 + }
414-428: Prefer go-runewidth for accurate terminal column widthThe emoji range heuristic is brittle. Use runewidth.StringWidth for correct width across locales.
-import ( - "strings" +import ( + "strings" + "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 -} +func calculateVisualWidth(s string) int { + return runewidth.StringWidth(s) +}packages/tui/internal/screens/interview/view.go (1)
9-17: Accept theme and avoid hard-coded colors for interview screenTo keep visuals consistent with the rest of the TUI, pass theme.Theme and use it instead of fixed color codes.
-import ( - "fmt" - - "github.com/charmbracelet/lipgloss/v2" -) +import ( + "fmt" + "github.com/charmbracelet/lipgloss/v2" + "github.com/rogue/tui/internal/theme" +) @@ -// Render renders the interview screen -func Render(width, height int) string { - style := lipgloss.NewStyle(). +// Render renders the interview screen +func Render(width, height int, t theme.Theme) string { + style := lipgloss.NewStyle(). Border(lipgloss.RoundedBorder()). - BorderForeground(lipgloss.Color("62")). + BorderForeground(t.Border()). Padding(1, 2). Width(width - 4). Height(height - 4) @@ - title := lipgloss.NewStyle(). - Foreground(lipgloss.Color("205")). + title := lipgloss.NewStyle(). + Foreground(t.Primary()). Bold(true). Render("💬 Interview Mode")packages/tui/internal/screens/dashboard/view.go (1)
58-59: Use direct pointer method calls (idiomatic Go)Replace (*commandInput).Method() with commandInput.Method().
- inputField := (*commandInput).ViewInput() + inputField := commandInput.ViewInput() @@ - if (*commandInput).HasSuggestions() { - suggestions := (*commandInput).ViewSuggestions() + if commandInput.HasSuggestions() { + suggestions := commandInput.ViewSuggestions()Also applies to: 83-85
packages/tui/internal/screens/help/view.go (1)
118-125: Clamp viewport dimensions to avoid negatives on small terminalswidth/height can be small; current math can produce negative sizes and panic or render badly. Clamp outer and inner sizes and reuse them in style.
@@ - // Calculate viewport dimensions - viewportWidth := width - 8 - viewportHeight := height - 6 + // Calculate viewport dimensions (clamped for tiny terminals) + viewportWidth := width - 8 + viewportHeight := height - 6 + if viewportWidth < 40 { + viewportWidth = 40 + } + if viewportHeight < 10 { + viewportHeight = 10 + } + innerW := viewportWidth - 4 + innerH := viewportHeight - 4 + if innerW < 1 { + innerW = 1 + } + if innerH < 1 { + innerH = 1 + } @@ - // Update the viewport size and content - viewport.SetSize(viewportWidth-4, viewportHeight-4) + // Update the viewport size and content + viewport.SetSize(innerW, innerH) viewport.SetContent(helpContent) @@ // Apply viewport styling viewport.Style = lipgloss.NewStyle(). Foreground(t.Text()). Background(t.BackgroundPanel()). - Width(viewportWidth-4). - Height(viewportHeight-4). + Width(innerW). + Height(innerH). Padding(1, 2)Also applies to: 135-141
packages/tui/internal/tui/eval_form_view.go (2)
3-7: Drop unused imports after delegating nil-state to evaluations.RenderFormAfter refactor below, lipgloss/theme imports become unused. Remove them.
-import ( - "github.com/charmbracelet/lipgloss/v2" - "github.com/rogue/tui/internal/screens/evaluations" - "github.com/rogue/tui/internal/theme" -) +import ( + "github.com/rogue/tui/internal/screens/evaluations" +)
11-20: Delegate nil-state rendering to evaluations.RenderForm(nil)Avoid duplicate “not initialized” view and keep behavior consistent in one place.
func (m Model) RenderNewEvaluation() string { - if m.evalState == nil { - t := theme.CurrentTheme() - return lipgloss.NewStyle(). - Width(m.width). - Height(m.height). - Background(t.Background()). - Foreground(t.Text()). - Align(lipgloss.Center, lipgloss.Center). - Render("New evaluation not initialized") - } + if m.evalState == nil { + return evaluations.RenderForm(nil) + }packages/tui/internal/screens/evaluations/detail_view.go (1)
50-57: Use Completed flag to drive status styling; keep spinner for in‑progressThis avoids string coupling on "completed" and makes styling consistent.
- var statusText string - if state.Status != "completed" && state.EvalSpinnerActive { - statusText = fmt.Sprintf("Status: %s %s", state.Status, state.EvalSpinnerView) - } else { - statusText = fmt.Sprintf("Status: %s", state.Status) - statusStyle = statusStyle. - Foreground(t.Success()) - } + var statusText string + if state.Completed { + statusText = "Status: completed" + statusStyle = statusStyle.Foreground(t.Success()) + } else if state.EvalSpinnerActive { + statusText = fmt.Sprintf("Status: %s %s", state.Status, state.EvalSpinnerView) + } else { + statusText = fmt.Sprintf("Status: %s", state.Status) + }packages/tui/internal/screens/scenarios/list.go (3)
218-237: Clamp panel width to avoid negative/zero widthsCompute once and reuse for both selected/unselected branches.
// Choose border style based on selection state var borderStyle lipgloss.Style + // Clamp panel width for tiny terminals + panelWidth := e.width - 8 + if panelWidth < 20 { + panelWidth = 20 + } if e.bizContextSelected { // Primary border and panel background when selected borderStyle = lipgloss.NewStyle(). Border(lipgloss.RoundedBorder()). BorderForeground(t.Primary()). Background(t.BackgroundPanel()). BorderBackground(t.BackgroundPanel()). Padding(0, 1). - Width(e.width - 8) // Account for padding and border + Width(panelWidth) // Account for padding and border } else { // Subtle border when not selected borderStyle = lipgloss.NewStyle(). Border(lipgloss.RoundedBorder()). BorderForeground(t.TextMuted()). BorderBackground(t.BackgroundPanel()). Background(t.BackgroundPanel()). Padding(0, 1). - Width(e.width - 8) // Account for padding and border + Width(panelWidth) // Account for padding and border }
244-246: Clamp ellipsis width for business context previewAvoid negative width when terminal is narrow.
- content := ellipsis(bizContext, e.width-20) + ellWidth := e.width - 20 + if ellWidth < 10 { + ellWidth = 10 + } + content := ellipsis(bizContext, ellWidth)
251-258: Clamp banner width in empty statePrevents malformed borders when width is very small.
- bannerWidth := e.width - 12 + bannerWidth := e.width - 12 + if bannerWidth < 40 { + bannerWidth = 40 + }packages/tui/internal/screens/evaluations/types.go (1)
32-36: Prefer a typed Status enum and document Progress boundsReduce stringly-typed errors by declaring a type (e.g., type EvalStatus string) with constants ("running","completed",...). Also document Progress expected range [0.0–1.0] to avoid misuse.
+type EvalStatus string +const ( + EvalStatusRunning EvalStatus = "running" + EvalStatusCompleted EvalStatus = "completed" + EvalStatusError EvalStatus = "error" +) @@ - Status string - Progress float64 + Status EvalStatus // running|completed|error + Progress float64 // 0.0..1.0packages/tui/internal/screens/config/view.go (2)
101-114: Cursor-safe editing for multi-byte runesServer URL slicing by byte index will misplace cursor for non-ASCII. Use runes.
- before := urlText[:configState.CursorPos] - atCursor := string(urlText[configState.CursorPos]) - after := "" - if configState.CursorPos+1 < len(urlText) { - after = urlText[configState.CursorPos+1:] - } + r := []rune(urlText) + pos := configState.CursorPos + if pos < 0 { pos = 0 } + if pos > len(r) { pos = len(r) } + before := string(r[:pos]) + atCursor := " " + if pos < len(r) { + atCursor = string(r[pos]) + } + after := "" + if pos+1 <= len(r) { + after = string(r[pos+1:]) + }Also update other URL edits to operate on []rune.
151-159: Avoid mutating configState in RenderRender should be pure; updating configState.QualifireEnabled here introduces hidden side effects. Move this sync logic into the controller before calling Render.
- // Update config state if not initialized - if configState != nil { - configState.QualifireEnabled = true - } + // Keep render pure; set QualifireEnabled in controller prior to Render(...)Apply similarly in the else-branch.
packages/tui/internal/tui/config_controller.go (1)
64-65: Also handle save errors when toggling QualifireSaving after toggle can fail; handle errors similarly.
- config.Save(&m.config) + if err := config.Save(&m.config); err != nil { + fmt.Println("failed to save config:", err) + }Also applies to: 190-191
packages/tui/internal/screens/report/view.go (2)
50-55: Avoid ClearMessages each render to preserve scroll stateRebuilding on every frame can reset viewport scroll and is O(n). Consider incremental updates or only clearing when source content actually changes.
51-55: Guard markdownRenderer only; reportHistory is always initializedThe
reportHistoryis initialized during app construction (app.go:55), so nil checks there are unnecessary. However,markdownRenderercan be nil:GetMarkdownRenderer()discards the error fromglamour.NewTermRenderer()and returns nil on failure (shared/markdown_renderer.go:21-28).Add a guard only for
markdownRenderer:- renderer := markdownRenderer - reportHistory.SetMarkdownRenderer(renderer) + if markdownRenderer != nil { + reportHistory.SetMarkdownRenderer(markdownRenderer) + }Alternatively, consider propagating errors from
GetMarkdownRenderer()instead of silently discarding them.packages/tui/internal/screens/report/controller.go (1)
33-35: Let “Back” work even when eval state is missing.The early return on hasEvalState=false also suppresses “b” back navigation. Consider handling “b” before the guard so users can always exit the report screen.
- if !hasEvalState { - return result - } +// Allow navigation keys even if eval state is missing +switch msg.String() { +case "b": + if reportHistory != nil { + reportHistory.Blur() + } + result.Action = ActionBackToDashboard + return result +} +if !hasEvalState { + return result +}Also applies to: 37-44
packages/tui/internal/screens/scenarios/editor.go (2)
80-96: Avoid magic IDs; define named constants.IDs 9996–9999 are opaque and easy to collide with later. Extract as consts for clarity and reuse.
+const ( + bizViewportID = 9999 + bizTextAreaID = 9998 + scenarioTextAreaID = 9997 + expectedOutcomeTAID = 9996 +) - vp := components.NewViewport(9999, 80, 10) + vp := components.NewViewport(bizViewportID, 80, 10) - ta := components.NewTextArea(9998, 80, 10, theme.CurrentTheme()) + ta := components.NewTextArea(bizTextAreaID, 80, 10, theme.CurrentTheme()) - scenTA := components.NewTextArea(9997, 80, 8, theme.CurrentTheme()) + scenTA := components.NewTextArea(scenarioTextAreaID, 80, 8, theme.CurrentTheme()) - outTA := components.NewTextArea(9996, 80, 8, theme.CurrentTheme()) + outTA := components.NewTextArea(expectedOutcomeTAID, 80, 8, theme.CurrentTheme())
122-156: Clamp widths and reuse a contentWidth variable.On narrow terminals width-4 can underflow; also you repeat width-4. Compute once and clamp to ≥1.
func (e *ScenarioEditor) SetSize(width, height int) { e.width = width e.height = height // Calculate visible items more accurately based on actual UI elements e.calculateVisibleItems() + // Common content width with safe clamp + contentWidth := width - 4 + if contentWidth < 1 { + contentWidth = 1 + } // Update business context components size - make it adaptive if e.bizViewport != nil { // Use smaller business context to leave more room for scenarios bizHeight := 3 // Reduce from 10 to 3 lines for business context if height < 20 { bizHeight = 2 // Even smaller on very small terminals } - e.bizViewport.SetSize(width-4, bizHeight) + e.bizViewport.SetSize(contentWidth, bizHeight) } if e.bizTextArea != nil { // Size will be set dynamically in renderBusinessContextView // Just update width here - e.bizTextArea.SetSize(width-4, e.bizTextArea.Height) + e.bizTextArea.SetSize(contentWidth, e.bizTextArea.Height) } // Update scenario editing TextAreas size if e.scenarioTextArea != nil { - e.scenarioTextArea.SetSize(width-4, 8) + e.scenarioTextArea.SetSize(contentWidth, 8) } if e.expectedOutcomeTextArea != nil { - e.expectedOutcomeTextArea.SetSize(width-4, 8) + e.expectedOutcomeTextArea.SetSize(contentWidth, 8) } // Update interview chat view size if e.interviewChatView != nil { e.interviewChatView.SetSize(width, height) } }packages/tui/internal/tui/interview_controller.go (1)
3-12: Add timeouts to SDK calls to avoid hanging ops.Wrap calls with context.WithTimeout to improve resilience.
import ( "context" "fmt" + "time" tea "github.com/charmbracelet/bubbletea/v2" "github.com/charmbracelet/glamour" "github.com/rogue/tui/internal/screens/scenarios" "github.com/rogue/tui/internal/shared" "github.com/rogue/tui/internal/theme" ) @@ func (m *Model) startInterviewCmd() tea.Cmd { return func() tea.Msg { - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() sdk := NewRogueSDK(m.config.ServerURL)Apply similarly in sendInterviewMessageCmd and generateScenariosCmd. Consider aligning default model strings across the app (gpt-4o vs gpt-4.1) for consistency.
Also applies to: 14-51
packages/tui/internal/screens/evaluations/form_view.go (2)
55-67: Don’t render a caret for non-editable fields (Deep Test).The cursor insertion runs for any active field; for a toggle field it’s distracting. Pass an “editable” flag into renderField and gate the caret insertion.
// renderField(fieldIndex int, label, value string, editable bool) if active && editable { // insert caret at state.CursorPos }Call sites: agent/judge => editable=true, deep test => editable=false.
24-27: Clamp dimensions to avoid negative/zero sizes on tiny terminals.Small guard improves robustness on constrained TTYs.
- mainStyle := lipgloss.NewStyle(). - Width(state.Width). - Height(state.Height - 1). + h := state.Height - 1 + if h < 1 { h = 1 } + w := state.Width + if w < 1 { w = 1 } + mainStyle := lipgloss.NewStyle(). + Width(w). + Height(h). @@ - fieldContainer := lipgloss.NewStyle().Width(state.Width-4) + cw := state.Width - 4; if cw < 1 { cw = 1 } + fieldContainer := lipgloss.NewStyle().Width(cw) @@ - spacer := lipgloss.NewStyle().Background(t.Background()).Width(state.Width).Render("") + spacer := lipgloss.NewStyle().Background(t.Background()).Width(w).Render("")Also applies to: 72-77, 176-185
packages/tui/internal/tui/common_controller.go (1)
338-347: Add timeout to ReportSummary call.Avoid indefinite hangs on network errors by using a bounded context.
- sdk := NewRogueSDK(m.config.ServerURL) - err := sdk.ReportSummary( - context.Background(), + sdk := NewRogueSDK(m.config.ServerURL) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + err := sdk.ReportSummary( - context.Background(), + ctx, m.evalState.JobID, m.evalState.StructuredSummary, m.evalState.DeepTest, m.evalState.JudgeModel, parsedAPIKey, )Remember to import time at top of this file if not already present.
packages/tui/internal/tui/types.go (1)
118-123: Consider using the ScenarioType type for type safety.The
ScenarioTypefield is declared asstring, but there's a distinctScenarioTypetype defined inutils.go(line 26). Using the typed version would provide better type safety and prevent invalid scenario type values.Apply this diff to use the typed version:
// EvalScenario represents a single evaluation scenario type EvalScenario struct { Scenario string `json:"scenario"` - ScenarioType string `json:"scenario_type"` + ScenarioType ScenarioType `json:"scenario_type"` ExpectedOutcome string `json:"expected_outcome,omitempty"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
packages/tui/Makefile(1 hunks)packages/tui/cmd/rogue/main.go(3 hunks)packages/tui/internal/components/message_history_view.go(1 hunks)packages/tui/internal/screens/config/persistence.go(5 hunks)packages/tui/internal/screens/config/types.go(1 hunks)packages/tui/internal/screens/config/view.go(10 hunks)packages/tui/internal/screens/dashboard/view.go(6 hunks)packages/tui/internal/screens/evaluations/detail_view.go(1 hunks)packages/tui/internal/screens/evaluations/form_view.go(12 hunks)packages/tui/internal/screens/evaluations/types.go(1 hunks)packages/tui/internal/screens/help/controller.go(1 hunks)packages/tui/internal/screens/help/view.go(4 hunks)packages/tui/internal/screens/interview/view.go(1 hunks)packages/tui/internal/screens/report/controller.go(1 hunks)packages/tui/internal/screens/report/view.go(2 hunks)packages/tui/internal/screens/scenarios/business_context.go(1 hunks)packages/tui/internal/screens/scenarios/edit.go(1 hunks)packages/tui/internal/screens/scenarios/editor.go(5 hunks)packages/tui/internal/screens/scenarios/editor_test.go(1 hunks)packages/tui/internal/screens/scenarios/file.go(1 hunks)packages/tui/internal/screens/scenarios/interview.go(3 hunks)packages/tui/internal/screens/scenarios/list.go(4 hunks)packages/tui/internal/screens/scenarios/scenario_helpers.go(1 hunks)packages/tui/internal/screens/scenarios/scenario_types.go(1 hunks)packages/tui/internal/shared/markdown_renderer.go(1 hunks)packages/tui/internal/shared/version.go(1 hunks)packages/tui/internal/tui/app.go(5 hunks)packages/tui/internal/tui/common_controller.go(6 hunks)packages/tui/internal/tui/config_controller.go(5 hunks)packages/tui/internal/tui/detail_controller.go(1 hunks)packages/tui/internal/tui/eval_detail.go(0 hunks)packages/tui/internal/tui/eval_detail_view.go(1 hunks)packages/tui/internal/tui/eval_form_view.go(1 hunks)packages/tui/internal/tui/eval_types.go(2 hunks)packages/tui/internal/tui/form_controller.go(1 hunks)packages/tui/internal/tui/help_controller.go(0 hunks)packages/tui/internal/tui/interview_controller.go(6 hunks)packages/tui/internal/tui/keyboard_controller.go(4 hunks)packages/tui/internal/tui/report_controller.go(0 hunks)packages/tui/internal/tui/scenario_utils.go(1 hunks)packages/tui/internal/tui/types.go(3 hunks)packages/tui/internal/tui/utils.go(0 hunks)
💤 Files with no reviewable changes (4)
- packages/tui/internal/tui/report_controller.go
- packages/tui/internal/tui/eval_detail.go
- packages/tui/internal/tui/utils.go
- packages/tui/internal/tui/help_controller.go
🧰 Additional context used
🧬 Code graph analysis (26)
packages/tui/internal/screens/help/controller.go (1)
packages/tui/internal/components/viewport.go (1)
Viewport(52-71)
packages/tui/internal/tui/eval_form_view.go (4)
packages/tui/internal/tui/types.go (1)
Model(51-90)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/screens/evaluations/types.go (1)
FormState(4-24)packages/tui/internal/screens/evaluations/form_view.go (1)
RenderForm(11-215)
packages/tui/internal/screens/scenarios/interview.go (1)
packages/tui/internal/components/dialog.go (3)
NewConfirmationDialog(81-95)DialogOpenMsg(60-62)Dialog(38-51)
packages/tui/cmd/rogue/main.go (1)
packages/tui/internal/shared/version.go (1)
Version(5-5)
packages/tui/internal/tui/scenario_utils.go (1)
packages/tui/internal/screens/scenarios/scenario_types.go (1)
ScenarioData(4-10)
packages/tui/internal/screens/help/view.go (1)
packages/tui/internal/components/viewport.go (1)
Viewport(52-71)
packages/tui/internal/screens/evaluations/form_view.go (1)
packages/tui/internal/screens/evaluations/types.go (1)
FormState(4-24)
packages/tui/internal/tui/eval_types.go (1)
packages/tui/internal/tui/utils.go (1)
ScenarioType(27-27)
packages/tui/internal/tui/config_controller.go (4)
packages/tui/internal/tui/types.go (3)
Model(51-90)ConfigFieldServerURL(113-113)ConfigFieldTheme(114-114)packages/tui/internal/screens/config/types.go (2)
ConfigFieldServerURL(7-7)ConfigFieldTheme(8-8)packages/tui/internal/screens/config/persistence.go (1)
Save(12-40)packages/tui/internal/theme/manager.go (2)
AvailableThemes(87-109)SetTheme(49-63)
packages/tui/internal/tui/app.go (10)
packages/tui/internal/shared/version.go (1)
Version(5-5)packages/tui/internal/components/command_line.go (1)
NewCommandInput(38-52)packages/tui/internal/screens/scenarios/editor.go (1)
NewScenarioEditor(71-105)packages/tui/internal/screens/config/persistence.go (1)
Load(43-71)packages/tui/internal/screens/scenarios/scenario_types.go (7)
StartInterviewMsg(38-38)SendInterviewMessageMsg(59-62)InterviewStartedMsg(40-44)InterviewResponseMsg(46-51)GenerateScenariosMsg(64-66)ScenariosGeneratedMsg(53-57)ScenarioEditorMsg(30-33)packages/tui/internal/screens/interview/view.go (1)
Render(10-33)packages/tui/internal/screens/config/view.go (1)
Render(11-213)packages/tui/internal/screens/report/view.go (2)
Render(17-126)EvalState(11-14)packages/tui/internal/screens/dashboard/view.go (1)
Render(11-112)packages/tui/internal/screens/help/view.go (1)
Render(12-185)
packages/tui/internal/tui/keyboard_controller.go (5)
packages/tui/internal/tui/config_controller.go (2)
HandleConfigEnter(11-76)HandleConfigInput(79-216)packages/tui/internal/tui/form_controller.go (1)
HandleEvalFormInput(11-145)packages/tui/internal/tui/detail_controller.go (1)
HandleEvalDetailInput(9-149)packages/tui/internal/screens/report/controller.go (3)
HandleInput(25-91)ActionBackToDashboard(13-13)ActionRegenerateSummary(14-14)packages/tui/internal/screens/help/controller.go (1)
HandleInput(10-32)
packages/tui/internal/screens/evaluations/detail_view.go (3)
packages/tui/internal/screens/evaluations/types.go (1)
DetailState(27-47)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/tui/interview_controller.go (2)
packages/tui/internal/screens/scenarios/scenario_types.go (4)
InterviewStartedMsg(40-44)InterviewResponseMsg(46-51)ScenariosGeneratedMsg(53-57)ScenarioData(4-10)packages/tui/internal/shared/markdown_renderer.go (1)
GetMarkdownRenderer(21-28)
packages/tui/internal/tui/eval_detail_view.go (5)
packages/tui/internal/tui/types.go (1)
Model(51-90)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/components/message_history_view.go (1)
Message(48-51)packages/tui/internal/screens/evaluations/types.go (1)
DetailState(27-47)packages/tui/internal/screens/evaluations/detail_view.go (1)
RenderDetail(11-163)
packages/tui/internal/screens/report/view.go (2)
packages/tui/internal/screens/config/view.go (1)
Render(11-213)packages/tui/internal/components/message_history_view.go (1)
MessageHistoryView(54-79)
packages/tui/internal/screens/config/types.go (1)
packages/tui/internal/tui/types.go (1)
ConfigField(109-109)
packages/tui/internal/screens/config/persistence.go (2)
packages/tui/internal/tui/types.go (1)
Config(107-107)packages/tui/internal/screens/config/types.go (1)
Config(24-35)
packages/tui/internal/screens/scenarios/list.go (3)
packages/tui/internal/components/dialog.go (5)
NewInputDialog(98-113)DialogButton(22-26)PrimaryButton(32-32)DialogOpenMsg(60-62)Dialog(38-51)packages/tui/internal/components/dialog_helpers.go (1)
ShowDeleteConfirmationDialog(39-53)packages/tui/internal/components/chat_view.go (1)
NewChatView(35-57)
packages/tui/internal/screens/report/controller.go (1)
packages/tui/internal/components/message_history_view.go (1)
MessageHistoryView(54-79)
packages/tui/internal/screens/config/view.go (3)
packages/tui/internal/screens/report/view.go (1)
Render(17-126)packages/tui/internal/tui/types.go (5)
Config(107-107)ConfigState(108-108)ConfigFieldServerURL(113-113)ConfigFieldQualifire(115-115)ConfigFieldTheme(114-114)packages/tui/internal/screens/config/types.go (5)
Config(24-35)ConfigState(13-21)ConfigFieldServerURL(7-7)ConfigFieldQualifire(9-9)ConfigFieldTheme(8-8)
packages/tui/internal/screens/dashboard/view.go (3)
packages/tui/internal/components/command_line.go (1)
CommandInput(21-30)packages/tui/internal/theme/theme.go (1)
Theme(10-78)packages/tui/internal/styles/whitespace.go (1)
WhitespaceStyle(8-10)
packages/tui/internal/screens/scenarios/editor.go (5)
packages/tui/internal/components/viewport.go (2)
Viewport(52-71)NewViewport(74-93)packages/tui/internal/components/textarea.go (2)
TextArea(92-114)NewTextArea(117-142)packages/tui/internal/components/chat_view.go (1)
ChatView(14-32)packages/tui/internal/components/spinner.go (1)
SpinnerTickMsg(11-13)packages/tui/internal/components/dialog.go (1)
DialogClosedMsg(54-57)
packages/tui/internal/tui/common_controller.go (3)
packages/tui/internal/screens/config/persistence.go (1)
Save(12-40)packages/tui/internal/tui/types.go (1)
Model(51-90)packages/tui/internal/screens/scenarios/scenario_types.go (7)
StartInterviewMsg(38-38)SendInterviewMessageMsg(59-62)InterviewStartedMsg(40-44)InterviewResponseMsg(46-51)GenerateScenariosMsg(64-66)ScenariosGeneratedMsg(53-57)ScenarioEditorMsg(30-33)
packages/tui/internal/tui/types.go (3)
packages/tui/internal/screens/scenarios/editor.go (1)
ScenarioEditor(13-68)packages/tui/internal/screens/config/types.go (6)
Config(24-35)ConfigState(13-21)ConfigField(4-4)ConfigFieldServerURL(7-7)ConfigFieldTheme(8-8)ConfigFieldQualifire(9-9)packages/tui/internal/tui/utils.go (1)
ScenarioType(27-27)
packages/tui/internal/tui/form_controller.go (1)
packages/tui/internal/tui/types.go (1)
Model(51-90)
packages/tui/internal/tui/detail_controller.go (1)
packages/tui/internal/tui/types.go (1)
Model(51-90)
🔇 Additional comments (28)
packages/tui/internal/tui/scenario_utils.go (2)
12-12: LGTM! Type consolidation improves maintainability.Importing the
scenariospackage to use the centralizedScenarioDatatype eliminates duplication and follows the DRY principle.
20-23: LGTM! Type reference correctly updated.The
Scenariosfield now properly references the externalscenarios.ScenarioDatatype, maintaining JSON compatibility while centralizing the type definition.packages/tui/internal/screens/scenarios/business_context.go (1)
1-1: Package rename verified—all references updated correctly.The refactoring from
package componentstopackage scenariosis complete. Verification confirms:
business_context.gohas correctpackage scenariosdeclaration- No references to old
screens/componentspath- All imports in the file are appropriate (standard lib, external deps,
internal/theme)- The separate
internal/componentspackage (UI utilities) remains unchanged and is correctly referenced elsewhere in the codebasepackages/tui/internal/screens/scenarios/edit.go (1)
1-1: Package rename verification complete—all imports correctly updated to scenarios package.The verification confirms the package rename from
componentstoscenariosis already complete and consistent throughout the codebase:
- edit.go correctly declares
package scenarios- No lingering imports or references to the old
componentspackage exist- The codebase contains 30+ verified references to
scenarios.ScenarioEditor,scenarios.ScenarioData, and other exported types, all functioning correctly- The old
componentsdirectory has been removedpackages/tui/internal/tui/eval_types.go (1)
29-34: No type mismatch; review comment is incorrectThe
EvalScenario.ScenarioTypefield is of typestring(not the named typeScenarioTypefrom utils.go), ands.ScenarioTypefrom the unmarshaled JSON is alsostring. Both are identical types, so no explicit conversion is needed or removed. The assignment at line 32 is type-safe.Likely an incorrect or invalid review comment.
packages/tui/internal/screens/scenarios/interview.go (1)
49-54: Dialog dispatch via components.DialogOpenMsg looks good — verify handler wiringThe new confirmation dialog creation and dispatch are correct. Please confirm the global update loop listens for components.DialogOpenMsg and opens the dialog accordingly.
Also applies to: 188-193
packages/tui/internal/tui/detail_controller.go (1)
8-9: Exporting HandleEvalDetailInput aligns with the new public handler patternSignature and behavior look good. Ensure all call sites use the new function and remove any now-dead receiver methods.
packages/tui/cmd/rogue/main.go (1)
8-8: LGTM! Import path and version references correctly updated.The import path change from
internal/tuitointernal/sharedand the correspondingshared.Versionreferences align correctly with the package reorganization.Also applies to: 40-40, 51-51
packages/tui/internal/screens/scenarios/editor_test.go (2)
1-1: LGTM! Package rename aligns with refactoring.The package rename from
componentstoscenariosis consistent with the broader architectural reorganization described in the PR.
7-8: Good cleanup: stringPtr helper consolidated.Moving the helper to the top and removing the duplicate improves code organization.
packages/tui/internal/shared/version.go (1)
1-5: LGTM! Package rename is consistent.The package rename from
tuitosharedcorrectly aligns with the new import paths used inmain.goandMakefile.packages/tui/internal/screens/scenarios/scenario_helpers.go (1)
1-1: LGTM! Package rename is consistent.The package rename from
componentstoscenariosaligns with the architectural reorganization across scenario-related files.packages/tui/Makefile (1)
14-14: LGTM! Build flags correctly updated.The linker flags now correctly reference
github.com/rogue/tui/internal/shared.Version, matching the package reorganization.packages/tui/internal/screens/scenarios/file.go (1)
1-1: LGTM! Package rename is consistent.The package rename from
componentstoscenariosmaintains consistency with the broader refactoring effort.packages/tui/internal/tui/form_controller.go (1)
10-11: LGTM! Method-to-function refactoring is well-executed.Converting
handleEvalFormInputto the exported functionHandleEvalFormInputwith an explicitModelparameter aligns well with the functional renderer pattern described in the PR objectives. The signature change is idiomatic and preserves the existing logic.packages/tui/internal/screens/help/controller.go (1)
10-31: LGTM! Clean input handling implementation.The
HandleInputfunction provides straightforward keyboard handling for the help screen:
- Home/end navigation for quick access to top/bottom
- Proper delegation to viewport for scrolling
- Correct command batching
The implementation is clear and follows the functional pattern established in this refactoring.
packages/tui/internal/tui/eval_detail_view.go (2)
98-117: State handoff to evaluations.RenderDetail looks solidDetailState composition is cohesive; spinner flags/views and pre-rendered contents are correctly passed through.
55-56: No issues found — role normalizer is correctly implemented and mapped.The
normalizeEvaluationRolefunction exists atpackages/tui/internal/tui/helpers.go:16and reliably maps all role inputs to the three supportedMessageHistoryViewroles:
- "agent" → "assistant"
- "rogue"/"judge"/"evaluator" → "user"
- "system" → "system"
- Unknown/empty → "user" (default)
The usage at
eval_detail_view.go:55-56is correct.packages/tui/internal/tui/config_controller.go (1)
218-260: Based on my verification:Remove the unused
handleConfigSavemethod instead of consolidatingThe method
handleConfigSave(lines 218–260) is dead code—it's never called anywhere in the codebase. The config persistence logic already exists inline in theUpdatehandler (around lines 20–35 and elsewhere), with multipleconfig.Save()calls at lines 24, 33, 64, and 190. Rather than consolidating, deletehandleConfigSaveentirely to eliminate the unused code.Likely an incorrect or invalid review comment.
packages/tui/internal/tui/interview_controller.go (1)
151-175: Renderer caching via shared.GetMarkdownRenderer looks solid.Width/theme cache keys and min-width guard are appropriate. No issues.
packages/tui/internal/tui/keyboard_controller.go (3)
313-332: Report input routing and actions look correct.Gates regeneration on JobID and spinner state; updates history and batches cmds properly.
333-337: Help viewport handler integration is clean.Pass-by-pointer, assignment of updated value, and cmd propagation are correct.
270-276: handleNewEvalEnter exists and is correctly invoked.Verified: the function is defined in
eval_form_view.go:45and called atkeyboard_controller.go:272. The code will build successfully.packages/tui/internal/tui/types.go (5)
8-9: LGTM! Clean package reorganization.The import additions align with the architectural refactoring to separate config and scenarios into dedicated screen packages.
65-65: LGTM! Consistent with package restructuring.The field type update correctly reflects the migration of ScenarioEditor from the components package to the scenarios package.
106-116: LGTM! Effective use of type aliases for package restructuring.The type aliases and constant re-exports maintain API compatibility while cleanly delegating configuration types to the dedicated config package. This is a good pattern for internal refactoring.
125-135: LGTM! Well-structured event type.The EvaluationEvent type is well-designed with appropriate JSON tags and flexible field types. The use of
anyfor the Data field provides needed flexibility for different event payloads.
137-175: LGTM! Comprehensive evaluation state types.The StructuredSummary and EvaluationViewState types are well-organized with logical field grouping. The inline comments provide useful categorization of the many state fields.
| // Config represents application configuration | ||
| type Config struct { | ||
| ServerURL string `toml:"server_url"` | ||
| Theme string `toml:"theme"` | ||
| APIKeys map[string]string `toml:"api_keys"` | ||
| SelectedModel string `toml:"selected_model"` | ||
| SelectedProvider string `toml:"selected_provider"` | ||
| InterviewModel string `toml:"interview_model"` | ||
| InterviewProvider string `toml:"interview_provider"` | ||
| QualifireAPIKey string `toml:"qualifire_api_key"` | ||
| QualifireEnabled bool `toml:"qualifire_enabled"` | ||
| DontShowQualifirePrompt bool `toml:"dont_show_qualifire_prompt"` | ||
| } |
There was a problem hiding this comment.
Protect secrets at rest: ensure config file is 0600 (not world‑readable).
Config carries API keys (APIKeys, QualifireAPIKey). Verify persistence writes with 0600 permissions and avoids leaking keys via logs.
Suggested change in packages/tui/internal/screens/config/persistence.go:
- if err := os.WriteFile(configFile, data, 0644); err != nil {
+ // Write with user-only permissions
+ if err := os.WriteFile(configFile, data, 0600); err != nil {
return fmt.Errorf("failed to write config file: %w", err)
}Also consider masking keys in any telemetry or debug logs. Based on learnings.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/tui/internal/screens/config/types.go around lines 23 to 35, the
Config struct contains secrets (APIKeys, QualifireAPIKey) and persistence must
protect them at rest and avoid leaking them in logs; update the persistence
logic to write config files with 0600 permissions (use os.OpenFile with 0600 or
call os.Chmod after write and handle umask appropriately), ensure any existing
config file is tightened to 0600 if found, and remove or mask secret fields
before any logging or telemetry (e.g., redact APIKeys/QualifireAPIKey when
serializing for logs or telemetry).
| // Render renders the configuration screen | ||
| func Render(width, height int, cfg *Config, configState *ConfigState) string { |
There was a problem hiding this comment.
Add nil-theme fallback to avoid crash on cold start
theme.CurrentTheme() may be nil; guard before using style colors.
-func Render(width, height int, cfg *Config, configState *ConfigState) string {
- t := theme.CurrentTheme()
+func Render(width, height int, cfg *Config, configState *ConfigState) string {
+ t := theme.CurrentTheme()
+ if t == nil {
+ return "Configuration (theme not initialized)"
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/tui/internal/screens/config/view.go around lines 10 to 11,
theme.CurrentTheme() can be nil on cold start so protect accesses to theme
colors: obtain the theme into a local variable, check for nil, and if nil
substitute a small fallback/default theme (or default color values) before using
its style/color fields; replace direct theme.CurrentTheme() dereferences with
the safe variable so the render path never panics when no theme is loaded.
| Width(width - 4). | ||
| Height(height - 1). | ||
| Background(t.BackgroundPanel()) |
There was a problem hiding this comment.
Clamp style widths/heights to non-negative values
Width(height) calculations like width-4, width-8, height-1 can go negative on small terminals. Clamp to >=0 to keep lipgloss happy.
- Width(width - 4).
- Height(height - 1).
+ Width(max(0, width-4)).
+ Height(max(0, height-1)).
@@
- Width(width - 8)
+ Width(max(0, width-8))
@@
- Width(width - 8)
+ Width(max(0, width-8))Add a small helper at top of file:
+func max(a, b int) int {
+ if a > b { return a }
+ return b
+}Also applies to: 30-30, 78-79
🤖 Prompt for AI Agents
In packages/tui/internal/screens/config/view.go around lines 20-22 (and also
apply to lines 30, and 78-79), clamp any computed Width(...) and Height(...)
arguments (e.g., width-4, width-8, height-1) to be non-negative before passing
them to lipgloss; add a small helper at the top of the file like
clampNonNegative(n int) int that returns 0 for negative inputs and the value
otherwise, then replace occurrences of raw arithmetic in Width/Height calls with
clampNonNegative(width-4), clampNonNegative(width-8),
clampNonNegative(height-1), etc., ensuring the helper is used consistently where
terminal size math could go negative.
| case "up", "down", "pgup", "pgdown": | ||
| // Scroll the report | ||
| if reportHistory != nil { | ||
| switch msg.String() { | ||
| case "up": | ||
| reportHistory.ScrollUp(1) | ||
| case "down": | ||
| reportHistory.ScrollDown(1) | ||
| case "pgup": | ||
| reportHistory.ScrollUp(10) | ||
| case "pgdown": | ||
| reportHistory.ScrollDown(10) | ||
| } | ||
| cmd := reportHistory.Update(msg) | ||
| if cmd != nil { | ||
| cmds = append(cmds, cmd) | ||
| } | ||
| } | ||
| result.Cmd = tea.Batch(cmds...) | ||
| return result | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Small ergonomics: cache key string and support vim-style/jump keys (optional).
- Cache key := msg.String() to avoid repeated conversions.
- Optionally accept “j/k” for down/up and “g/G” for top/bottom for power users.
Please confirm Bubble Tea v2 key strings for PageUp/PageDown (“pgup”/“pgdown”) match your keymap.
🌐 Web query:
What are the tea.KeyMsg.String() names for PageUp and PageDown in Bubble Tea v2?
💡 Result:
They are "pageup" and "pagedown". [1][2]
Sources:
- Bubble Tea (Key.String docs). [1]
- Example key-name lists showing Page Up / Page Down as "pageup" / "pagedown". [2]
Use correct Bubble Tea v2 key names: "pageup"/"pagedown" instead of "pgup"/"pgdown".
Bubble Tea v2 uses "pageup" and "pagedown" for PageUp/PageDown keys, not the abbreviated forms in the current code. Change lines 74 and 77 from "pgup"/"pgdown" to "pageup"/"pagedown" or these keys won't be handled.
Optional: cache key := msg.String() to avoid repeated conversions.
🤖 Prompt for AI Agents
In packages/tui/internal/screens/report/controller.go around lines 66 to 86, the
key handling uses non-standard Bubble Tea v2 names "pgup"/"pgdown" so
PageUp/PageDown won't be caught; change the cases for lines ~74 and ~77 from
"pgup"/"pgdown" to "pageup"/"pagedown" and optionally cache msg.String() into a
local variable (e.g., key := msg.String()) and use that variable in the switch
to avoid repeated conversions; keep the existing ScrollUp/ScrollDown call counts
and the Update()/Batch logic unchanged.
| reportHeight := height - 5 | ||
|
|
There was a problem hiding this comment.
Clamp reportHeight for small terminals
Prevent negative heights in downstream components.
- reportHeight := height - 5
+ reportHeight := height - 5
+ if reportHeight < 0 {
+ reportHeight = 0
+ }📝 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.
| reportHeight := height - 5 | |
| reportHeight := height - 5 | |
| if reportHeight < 0 { | |
| reportHeight = 0 | |
| } |
🤖 Prompt for AI Agents
In packages/tui/internal/screens/report/view.go around lines 48-49, reportHeight
is computed as "height - 5" which can become negative on small terminals; change
the assignment to clamp it to a non-negative value (e.g. reportHeight := height
- 5; if reportHeight < 0 { reportHeight = 0 }) so downstream components never
receive a negative height.
| t := theme.CurrentTheme() | ||
| if m.evalState == nil { | ||
| return lipgloss.NewStyle(). | ||
| Width(m.width). | ||
| Height(m.height). | ||
| Background(t.Background()). | ||
| Foreground(t.Text()). | ||
| Align(lipgloss.Center, lipgloss.Center). | ||
| Render("No evaluation active") | ||
| } |
There was a problem hiding this comment.
Guard against nil theme to prevent panic
theme.CurrentTheme() can return nil; any t.Background()/t.Text() call would panic. Add a safe fallback or bail early with a minimal text render.
func (m Model) RenderEvaluationDetail() string {
- t := theme.CurrentTheme()
+ t := theme.CurrentTheme()
+ if t == nil {
+ // Fallback if theme not initialized yet
+ return "No evaluation active (theme not initialized)"
+ }
if m.evalState == nil {Committable suggestion skipped: line range outside the PR's diff.
| // Calculate available height for content area | ||
| availableHeight := m.height - 5 // header(3) + helpText(2) | ||
| statusAndSpacersHeight := 5 | ||
|
|
||
| var eventsHeight, summaryHeight int | ||
| var showSummary bool | ||
|
|
||
| // Determine if we show summary | ||
| if m.evalState.Completed && (m.evalState.Summary != "" || m.summarySpinner.IsActive()) { | ||
| showSummary = true | ||
| statusAndSpacersHeight = 6 | ||
| remainingHeight := availableHeight - statusAndSpacersHeight | ||
| eventsHeight = remainingHeight / 2 | ||
| summaryHeight = remainingHeight - eventsHeight | ||
| } else { | ||
| remainingHeight := availableHeight - statusAndSpacersHeight | ||
| eventsHeight = remainingHeight | ||
| summaryHeight = 0 | ||
| } |
There was a problem hiding this comment.
Clamp computed heights to non-negative values
availableHeight and derived event/summary heights can go negative on small terminals, causing layout glitches or crashes in child components.
- availableHeight := m.height - 5 // header(3) + helpText(2)
+ availableHeight := m.height - 5 // header(3) + helpText(2)
+ if availableHeight < 0 {
+ availableHeight = 0
+ }
statusAndSpacersHeight := 5
@@
- eventsHeight = remainingHeight / 2
- summaryHeight = remainingHeight - eventsHeight
+ if remainingHeight < 0 {
+ remainingHeight = 0
+ }
+ eventsHeight = remainingHeight / 2
+ summaryHeight = remainingHeight - eventsHeight
} else {
- remainingHeight := availableHeight - statusAndSpacersHeight
+ remainingHeight := availableHeight - statusAndSpacersHeight
+ if remainingHeight < 0 {
+ remainingHeight = 0
+ }
eventsHeight = remainingHeight
summaryHeight = 0
}🤖 Prompt for AI Agents
In packages/tui/internal/tui/eval_detail_view.go around lines 24 to 42, the
computed availableHeight, remainingHeight, eventsHeight and summaryHeight can
become negative on very small terminals; clamp availableHeight to a minimum of
0, ensure remainingHeight is computed as max(0, availableHeight -
statusAndSpacersHeight), and then set eventsHeight and summaryHeight using
max(0, computedValue) so neither is negative (use math.Max or an integer
conditional to enforce >=0).
| // Clear existing messages and rebuild from events | ||
| m.eventsHistory.ClearMessages() | ||
|
|
||
| // Process events and add as messages | ||
| for _, ev := range m.evalState.Events { | ||
| switch ev.Type { | ||
| case "status": | ||
| if ev.Status != "running" { | ||
| m.eventsHistory.AddMessage("system", fmt.Sprintf("✓ %s", ev.Status)) | ||
| } | ||
| case "chat": | ||
| normalizedRole := normalizeEvaluationRole(ev.Role) | ||
| m.eventsHistory.AddMessage(normalizedRole, ev.Content) | ||
| case "error": | ||
| m.eventsHistory.AddMessage("system", fmt.Sprintf("⚠ ERROR: %s", ev.Message)) | ||
| } | ||
| } | ||
|
|
||
| // Check if we have any messages | ||
| if len(m.eventsHistory.GetMessages()) == 0 { | ||
| m.eventsHistory.AddMessage("system", "Waiting for evaluation events...") | ||
| } | ||
|
|
||
| // Update size and customize prefixes | ||
| m.eventsHistory.SetSize(m.width, eventsHeight) | ||
| m.eventsHistory.SetPrefixes("🔍 Rogue: ", "🤖 Agent: ") | ||
| m.eventsHistory.SetColors(t.Primary(), t.Text()) | ||
|
|
||
| // Render events using MessageHistoryView | ||
| eventsContent := m.eventsHistory.View(t) |
There was a problem hiding this comment.
Nil checks for MessageHistoryView dependencies
m.eventsHistory is dereferenced multiple times without a nil check. Same pattern exists for m.summaryHistory later. Add guards or initialize prior to use to avoid panics.
- // Clear existing messages and rebuild from events
- m.eventsHistory.ClearMessages()
+ // Clear existing messages and rebuild from events
+ if m.eventsHistory == nil {
+ return "Events view unavailable"
+ }
+ m.eventsHistory.ClearMessages()
@@
- m.eventsHistory.SetSize(m.width, eventsHeight)
+ m.eventsHistory.SetSize(m.width, eventsHeight)Apply a similar guard for m.summaryHistory below (see lines 76-96).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/tui/internal/tui/eval_detail_view.go lines 44-73 (and similarly 76-96):
m.eventsHistory (and m.summaryHistory further down) are dereferenced without nil
checks; add a guard at the top of each block to either initialize the field if
nil (e.g., construct a new MessageHistoryView with appropriate defaults) or
return/skip the rendering logic when nil, then proceed to use the instance;
ensure you perform the nil-check before ClearMessages, AddMessage, SetSize,
SetPrefixes, SetColors and View calls so the code cannot panic.
| // Build summary content if needed | ||
| var summaryContent string | ||
| if showSummary { | ||
| m.summaryHistory.ClearMessages() | ||
|
|
||
| if m.summarySpinner.IsActive() { | ||
| m.summaryHistory.AddMessage("system", "Generating evaluation summary...") | ||
| } else if m.evalState.Summary != "" { | ||
| m.summaryHistory.AddMessage("assistant", m.evalState.Summary) | ||
| } else { | ||
| m.summaryHistory.AddMessage("system", "Summary not available") | ||
| } | ||
|
|
||
| m.summaryHistory.SetSize(m.width, summaryHeight) | ||
| m.summaryHistory.SetPrefixes("", "") | ||
| m.summaryHistory.SetColors(t.Success(), t.Text()) | ||
|
|
||
| renderer := m.getMarkdownRenderer() | ||
| m.summaryHistory.SetMarkdownRenderer(renderer) | ||
|
|
||
| summaryContent = m.summaryHistory.View(t) | ||
| } |
There was a problem hiding this comment.
Also guard m.summaryHistory before use
Avoid nil dereference when summary is shown but history view wasn’t initialized.
- if showSummary {
- m.summaryHistory.ClearMessages()
+ if showSummary {
+ if m.summaryHistory == nil {
+ return "Summary view unavailable"
+ }
+ m.summaryHistory.ClearMessages()📝 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.
| // Build summary content if needed | |
| var summaryContent string | |
| if showSummary { | |
| m.summaryHistory.ClearMessages() | |
| if m.summarySpinner.IsActive() { | |
| m.summaryHistory.AddMessage("system", "Generating evaluation summary...") | |
| } else if m.evalState.Summary != "" { | |
| m.summaryHistory.AddMessage("assistant", m.evalState.Summary) | |
| } else { | |
| m.summaryHistory.AddMessage("system", "Summary not available") | |
| } | |
| m.summaryHistory.SetSize(m.width, summaryHeight) | |
| m.summaryHistory.SetPrefixes("", "") | |
| m.summaryHistory.SetColors(t.Success(), t.Text()) | |
| renderer := m.getMarkdownRenderer() | |
| m.summaryHistory.SetMarkdownRenderer(renderer) | |
| summaryContent = m.summaryHistory.View(t) | |
| } | |
| // Build summary content if needed | |
| var summaryContent string | |
| if showSummary { | |
| if m.summaryHistory == nil { | |
| return "Summary view unavailable" | |
| } | |
| m.summaryHistory.ClearMessages() | |
| if m.summarySpinner.IsActive() { | |
| m.summaryHistory.AddMessage("system", "Generating evaluation summary...") | |
| } else if m.evalState.Summary != "" { | |
| m.summaryHistory.AddMessage("assistant", m.evalState.Summary) | |
| } else { | |
| m.summaryHistory.AddMessage("system", "Summary not available") | |
| } | |
| m.summaryHistory.SetSize(m.width, summaryHeight) | |
| m.summaryHistory.SetPrefixes("", "") | |
| m.summaryHistory.SetColors(t.Success(), t.Text()) | |
| renderer := m.getMarkdownRenderer() | |
| m.summaryHistory.SetMarkdownRenderer(renderer) | |
| summaryContent = m.summaryHistory.View(t) | |
| } |
🤖 Prompt for AI Agents
In packages/tui/internal/tui/eval_detail_view.go around lines 75 to 96, the code
uses m.summaryHistory without checking for nil which can cause a panic if the
view wasn't initialized; before calling methods on m.summaryHistory, check
whether m.summaryHistory is nil and either initialize it (create a new history
view with the same configuration used elsewhere) or skip the summary-building
block if it’s nil, then proceed to set size, prefixes, colors, renderer and call
View only when m.summaryHistory is non-nil.
|
|
||
| // Scenario generation API types matching server's rogue_sdk.types | ||
|
|
||
| // ScenarioData represents a single scenario aligned with Python schema |
There was a problem hiding this comment.
Remove stale comment.
This comment refers to ScenarioData, which is no longer defined in this file after the refactoring. The type now lives in the scenarios package.
Apply this diff to remove the stale comment:
-// ScenarioData represents a single scenario aligned with Python schema
-
// ScenariosList represents the collection of scenarios with business context📝 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.
| // ScenarioData represents a single scenario aligned with Python schema | |
| // ScenariosList represents the collection of scenarios with business context |
🤖 Prompt for AI Agents
In packages/tui/internal/tui/scenario_utils.go around line 17, remove the stale
comment "// ScenarioData represents a single scenario aligned with Python
schema" because ScenarioData no longer exists in this file after refactor;
delete that comment line so the file no longer references a moved type and save
the file.
| } | ||
|
|
||
| // StructuredSummary represents a structured evaluation summary | ||
| type StructuredSummary struct { |
There was a problem hiding this comment.
Why isn't this in the report package?
There was a problem hiding this comment.
it is used in the common controller so to avoid import cycle it's here
Resolved conflicts: - form_view.go: Merged new Protocol/Transport fields with refactored FormState structure - help/view.go: Merged embedded help.md with refactored Render function - eval_types.go: Merged Protocol/Transport types and helper functions Changes: - Added Protocol and Transport fields to FormState and EvaluationViewState - Moved help.md to internal/screens/help/ package - Updated help/view.go to use shared.GetMarkdownRenderer - Removed duplicate EvaluationViewState definition from types.go - Updated eval_form_view.go to pass Protocol/Transport to FormState
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tui/internal/tui/keyboard_controller.go (1)
139-153: “/” not insertable in JudgeModel due to wrong field conditionCondition uses currentField <= 1 but the switch also targets field 3, making the JudgeModel branch unreachable. Users can’t type “/” in model names.
-// Don't intercept "/" if we're editing text in NewEvaluationScreen -if m.currentScreen == NewEvaluationScreen && m.evalState != nil && m.evalState.currentField <= 1 { +// Allow "/" in text fields: AgentURL (0) and JudgeModel (3) +if m.currentScreen == NewEvaluationScreen && m.evalState != nil && + (m.evalState.currentField == 0 || m.evalState.currentField == 3) {
🧹 Nitpick comments (3)
packages/tui/internal/screens/help/view.go (1)
27-27: Avoid per-render glamour renderer allocationGetMarkdownRenderer creates a new glamour.TermRenderer every render. Bubble Tea re-renders often; cache/memoize the renderer per (contentWidth, theme background) to reduce allocations and improve responsiveness.
packages/tui/internal/tui/keyboard_controller.go (2)
337-339: Avoid copying Viewport; update in placehelp.HandleInput mutates via pointer and returns the same pointer. Reassigning m.helpViewport = *updatedViewport copies the struct (slice headers etc.) unnecessarily.
- updatedViewport, cmd := help.HandleInput(&m.helpViewport, msg) - m.helpViewport = *updatedViewport - return m, cmd + _, cmd := help.HandleInput(&m.helpViewport, msg) + return m, cmd
323-335: On report “Back” action, also refocus/clear the command input for consistencyESC path focuses and clears the global command input when returning to the dashboard. Mirror that here for consistent UX.
switch result.Action { case report.ActionBackToDashboard: m.currentScreen = DashboardScreen + m.commandInput.SetFocus(true) + m.commandInput.SetValue("")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/tui/internal/screens/evaluations/form_view.go(16 hunks)packages/tui/internal/screens/evaluations/types.go(1 hunks)packages/tui/internal/screens/help/view.go(4 hunks)packages/tui/internal/shared/markdown_renderer.go(1 hunks)packages/tui/internal/tui/common_controller.go(6 hunks)packages/tui/internal/tui/eval_form_view.go(1 hunks)packages/tui/internal/tui/eval_types.go(2 hunks)packages/tui/internal/tui/form_controller.go(1 hunks)packages/tui/internal/tui/keyboard_controller.go(4 hunks)packages/tui/internal/tui/types.go(3 hunks)packages/tui/internal/tui/utils.go(0 hunks)
💤 Files with no reviewable changes (1)
- packages/tui/internal/tui/utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tui/internal/tui/form_controller.go
- packages/tui/internal/shared/markdown_renderer.go
- packages/tui/internal/screens/evaluations/types.go
🧰 Additional context used
🧬 Code graph analysis (7)
packages/tui/internal/screens/help/view.go (3)
packages/tui/internal/components/viewport.go (1)
Viewport(52-71)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/shared/markdown_renderer.go (1)
GetMarkdownRenderer(21-28)
packages/tui/internal/tui/eval_form_view.go (4)
packages/tui/internal/tui/types.go (1)
Model(51-90)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/screens/evaluations/types.go (1)
FormState(4-26)packages/tui/internal/screens/evaluations/form_view.go (1)
RenderForm(11-295)
packages/tui/internal/tui/keyboard_controller.go (5)
packages/tui/internal/tui/config_controller.go (2)
HandleConfigEnter(11-76)HandleConfigInput(79-216)packages/tui/internal/tui/form_controller.go (1)
HandleEvalFormInput(11-153)packages/tui/internal/tui/detail_controller.go (1)
HandleEvalDetailInput(9-149)packages/tui/internal/screens/report/controller.go (3)
HandleInput(25-91)ActionBackToDashboard(13-13)ActionRegenerateSummary(14-14)packages/tui/internal/screens/help/controller.go (1)
HandleInput(10-32)
packages/tui/internal/tui/types.go (3)
packages/tui/internal/screens/scenarios/editor.go (1)
ScenarioEditor(13-68)packages/tui/internal/screens/config/types.go (6)
Config(24-35)ConfigState(13-21)ConfigField(4-4)ConfigFieldServerURL(7-7)ConfigFieldTheme(8-8)ConfigFieldQualifire(9-9)packages/tui/internal/tui/utils.go (1)
ScenarioType(27-27)
packages/tui/internal/tui/eval_types.go (1)
packages/tui/internal/tui/utils.go (1)
ScenarioType(27-27)
packages/tui/internal/tui/common_controller.go (3)
packages/tui/internal/screens/config/persistence.go (1)
Save(12-40)packages/tui/internal/tui/types.go (1)
Model(51-90)packages/tui/internal/screens/scenarios/scenario_types.go (7)
StartInterviewMsg(38-38)SendInterviewMessageMsg(59-62)InterviewStartedMsg(40-44)InterviewResponseMsg(46-51)GenerateScenariosMsg(64-66)ScenariosGeneratedMsg(53-57)ScenarioEditorMsg(30-33)
packages/tui/internal/screens/evaluations/form_view.go (1)
packages/tui/internal/screens/evaluations/types.go (1)
FormState(4-26)
⏰ 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 (12)
packages/tui/internal/tui/eval_types.go (2)
28-28: LGTM! Helpful clarification.The comment explains the architectural decision to define
EvaluationViewStatein the tui package to avoid import cycles, which is useful context for future maintainers.
78-78: LGTM! Assignment is correct.The direct field assignment from the JSON-unmarshaled struct to
EvalScenariois appropriate here.packages/tui/internal/tui/eval_form_view.go (2)
10-44: LGTM! Clean delegation pattern.The function correctly converts internal
Modelstate toFormStateand delegates rendering to theevaluationspackage. The nil check provides sensible fallback rendering.
46-54: LGTM! Appropriate use of pointer receiver.The pointer receiver is correct here since the method modifies model state (line 53:
m.evalSpinner.SetActive(true)).packages/tui/internal/screens/evaluations/form_view.go (2)
1-21: LGTM! Good refactoring to functional approach.The shift from receiver-based to functional rendering with explicit state is a solid improvement for testability and reusability. The nil check with sensible defaults (80x24) provides a good fallback.
25-294: LGTM! Consistent state-driven rendering.All references to model fields have been properly updated to use
stateparameter fields. The helper functions correctly referencestate.CurrentField,state.CursorPos,state.Width, etc., maintaining consistency throughout the refactor.packages/tui/internal/tui/common_controller.go (3)
10-11: LGTM! Proper import organization.Adding the
configandscenariospackages enables centralized config persistence and scenario-based messaging, aligning with the broader refactor.
281-281: LGTM! Centralized config persistence.Replacing
m.saveConfig()withconfig.Save(&m.config)centralizes configuration persistence logic, improving maintainability.Also applies to: 357-357, 405-405, 454-454
523-559: LGTM! Consistent message type updates.All handler signatures have been correctly updated to accept scenario-based message types (
scenarios.StartInterviewMsg,scenarios.SendInterviewMessageMsg, etc.) instead of component-based ones, maintaining consistency across the refactor.packages/tui/internal/tui/types.go (3)
106-116: LGTM! Type aliases enable clean imports.The type aliases and re-exported constants allow internal code to reference
config.Config,config.ConfigState, etc. through the tui package, simplifying imports while maintaining separation of concerns.
65-65: LGTM! Type updated to match refactor.Updating
scenarioEditorfromcomponents.ScenarioEditortoscenarios.ScenarioEditoraligns with the broader package reorganization.
118-147: Current package organization is appropriate—no changes needed.The verification shows all three types (
EvalScenario,EvaluationEvent,StructuredSummary) are used exclusively within thepackages/tui/internal/tui/package across multiple files (eval_types.go,utils.go,common_controller.go,commands.go). Moving them to a separate package would introduce unnecessary inter-package dependencies without organizational benefit. No import cycles exist with the current placement.
| t := theme.CurrentTheme() | ||
|
|
There was a problem hiding this comment.
Guard against nil theme to prevent panic
theme.CurrentTheme() can return nil (see manager returning nil when unset). Subsequent calls (Background(), Primary(), etc.) will panic. Add an early guard.
t := theme.CurrentTheme()
+if t == nil {
+ log.Println("help.Render: no current theme; falling back to raw help content")
+ // Safe fallback avoids nil deref; optionally render plain content.
+ return helpContent
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/tui/internal/screens/help/view.go around lines 19-20,
theme.CurrentTheme() can return nil and subsequent method calls (Background(),
Primary(), etc.) will panic; add an early nil guard after calling
CurrentTheme(), and if nil either assign a safe default theme (e.g.,
theme.DefaultTheme()) or return/handle gracefully (log and use fallback colors)
before using theme methods so no dereference of nil occurs.
| viewportWidth := width - 11 | ||
| viewportHeight := height - 9 | ||
| contentWidth := viewportWidth - 6 |
There was a problem hiding this comment.
Clamp layout sizes to avoid negative dimensions and layout glitches on small terminals
viewportWidth/Height, contentWidth, and container height can become negative when the terminal is small, leading to bad wrapping or panics in renderers. Clamp to safe minimums.
-viewportWidth := width - 11
-viewportHeight := height - 9
-contentWidth := viewportWidth - 6
+viewportWidth := width - 11
+viewportHeight := height - 9
+contentWidth := viewportWidth - 6
+// Clamp to safe minimums
+if viewportWidth < 1 {
+ viewportWidth = 1
+}
+if viewportHeight < 1 {
+ viewportHeight = 1
+}
+if contentWidth < 1 {
+ contentWidth = 1
+}
@@
-mainStyle := lipgloss.NewStyle().
- Width(width).
- Height(height - 1).
+containerHeight := height - 1
+if containerHeight < 0 {
+ containerHeight = 0
+}
+mainStyle := lipgloss.NewStyle().
+ Width(width).
+ Height(containerHeight).
@@
-viewport.SetSize(contentWidth, viewportHeight)
+viewport.SetSize(contentWidth, viewportHeight) // dimensions already clampedAlso applies to: 37-39, 52-53, 56-64, 69-71, 85-91
🤖 Prompt for AI Agents
In packages/tui/internal/screens/help/view.go around lines 22-24 (and also
update similarly at 37-39, 52-53, 56-64, 69-71, 85-91), layout width/height
calculations (viewportWidth, viewportHeight, contentWidth and container heights)
can become negative on very small terminals; clamp each computed dimension to
safe minimums (e.g., min 1 or other component-specific minimums) before using
them in view/viewport/container creation, ensure subtraction operations use the
clamped values and replace direct arithmetic with helper clamp calls or inline
max(x, minVal) to prevent negative sizes and avoid layout glitches or panics.
| ) | ||
|
|
||
| // Minimal state for eval screens | ||
| // EvaluationViewState is defined in tui package to avoid import cycles |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
packages/tui/internal/screens/help/view.go (2)
21-21: Guard against nil theme to prevent panic.
theme.CurrentTheme()can returnnilwhen no theme is set. Subsequent method calls (Background(),Primary(), etc.) will panic. This issue was previously flagged but persists.Apply this diff to add a nil guard:
t := theme.CurrentTheme() +if t == nil { + // Fallback to safe default or return early with error message + return "Help screen unavailable: no theme configured" +}
25-26: Clamp layout dimensions to prevent negative values and rendering issues.Dimension calculations like
viewportWidth := width - 11andheight - 1can become negative on very small terminals, causing layout glitches or panics. This issue was previously flagged but persists.Apply this diff to clamp dimensions:
-viewportWidth := width - 11 -viewportHeight := height - 9 +viewportWidth := max(width-11, 1) +viewportHeight := max(height-9, 1)mainStyle := lipgloss.NewStyle(). Width(width). - Height(height - 1). + Height(max(height-1, 0)).Similar clamping should be applied to other dimension calculations throughout the function (lines 39, 47-48, 59-60, 65-66, 78).
Also applies to: 30-31
packages/tui/internal/tui/eval_detail_view.go (4)
12-22: Guard against nil theme to prevent panic.
theme.CurrentTheme()can returnnil, causing any method call liket.Background()to panic. Add an early nil check with a fallback render.func (m Model) RenderEvaluationDetail() string { t := theme.CurrentTheme() + if t == nil { + return "Theme not initialized" + } if m.evalState == nil {Based on past review comments.
24-42: Clamp computed heights to prevent negative values.
availableHeightand derived heights (eventsHeight,summaryHeight) can go negative on small terminals, causing layout issues or crashes in child components. Add clamping to ensure non-negative values.// Calculate available height for content area availableHeight := m.height - 5 // header(3) + helpText(2) + if availableHeight < 0 { + availableHeight = 0 + } statusAndSpacersHeight := 5 var eventsHeight, summaryHeight int @@ remainingHeight := availableHeight - statusAndSpacersHeight + if remainingHeight < 0 { + remainingHeight = 0 + } eventsHeight = remainingHeight / 2Based on past review comments.
44-73: Add nil check for m.eventsHistory.
m.eventsHistoryis dereferenced multiple times (lines 45, 52, 56, 58, 63, 64, 68-70, 73) without a nil check. This will panic if the history view wasn't initialized. Add a guard at the beginning of this section.// Clear existing messages and rebuild from events + if m.eventsHistory == nil { + return "Events view unavailable" + } m.eventsHistory.ClearMessages()Based on past review comments.
76-96: Add nil check for m.summaryHistory.
m.summaryHistoryis used without a nil check whenshowSummaryis true. Add a guard to prevent panics if the summary view wasn't initialized.// Build summary content if needed var summaryContent string if showSummary { + if m.summaryHistory == nil { + return "Summary view unavailable" + } m.summaryHistory.ClearMessages()Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/tui/internal/screens/evaluations/detail_view.go(1 hunks)packages/tui/internal/screens/evaluations/types.go(1 hunks)packages/tui/internal/screens/help/view.go(3 hunks)packages/tui/internal/tui/common_controller.go(7 hunks)packages/tui/internal/tui/detail_controller.go(2 hunks)packages/tui/internal/tui/eval_detail_view.go(1 hunks)packages/tui/internal/tui/help_utils.go(1 hunks)packages/tui/internal/tui/keyboard_controller.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/tui/internal/tui/help_utils.go (4)
packages/tui/internal/tui/types.go (1)
Model(51-90)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/shared/markdown_renderer.go (1)
GetMarkdownRenderer(21-28)packages/tui/internal/screens/help/view.go (2)
GetHelpContent(15-17)Render(20-96)
packages/tui/internal/tui/eval_detail_view.go (5)
packages/tui/internal/tui/types.go (1)
Model(51-90)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/components/message_history_view.go (1)
Message(48-51)packages/tui/internal/screens/evaluations/types.go (1)
DetailState(29-52)packages/tui/internal/screens/evaluations/detail_view.go (1)
RenderDetail(11-188)
packages/tui/internal/tui/keyboard_controller.go (6)
packages/tui/internal/tui/config_controller.go (2)
HandleConfigEnter(11-76)HandleConfigInput(79-216)packages/tui/internal/tui/form_controller.go (1)
HandleEvalFormInput(11-153)packages/tui/internal/tui/types.go (4)
EvaluationDetailScreen(36-36)ReportScreen(38-38)DashboardScreen(34-34)HelpScreen(42-42)packages/tui/internal/tui/detail_controller.go (1)
HandleEvalDetailInput(9-164)packages/tui/internal/screens/help/controller.go (1)
HandleInput(10-32)packages/tui/internal/screens/report/controller.go (3)
HandleInput(25-91)ActionBackToDashboard(13-13)ActionRegenerateSummary(14-14)
packages/tui/internal/screens/help/view.go (2)
packages/tui/internal/components/viewport.go (1)
Viewport(52-71)packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)
packages/tui/internal/tui/detail_controller.go (1)
packages/tui/internal/tui/types.go (1)
Model(51-90)
packages/tui/internal/screens/evaluations/detail_view.go (3)
packages/tui/internal/screens/evaluations/types.go (1)
DetailState(29-52)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/tui/common_controller.go (3)
packages/tui/internal/screens/config/persistence.go (1)
Save(12-40)packages/tui/internal/tui/types.go (1)
Model(51-90)packages/tui/internal/screens/scenarios/scenario_types.go (7)
StartInterviewMsg(38-38)SendInterviewMessageMsg(59-62)InterviewStartedMsg(40-44)InterviewResponseMsg(46-51)GenerateScenariosMsg(64-66)ScenariosGeneratedMsg(53-57)ScenarioEditorMsg(30-33)
⏰ 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). (3)
- GitHub Check: Import Time Gate
- GitHub Check: rogue_sanity
- GitHub Check: codestyle
🔇 Additional comments (17)
packages/tui/internal/screens/evaluations/detail_view.go (2)
96-159: Well-structured dual-viewport rendering.The conditional layout for showing summary alongside events is cleanly implemented. Using pre-rendered content strings from the state simplifies the rendering logic and separates concerns effectively.
160-188: LGTM!The single-viewport path and final layout assembly are consistent with the dual-viewport branch. The use of
JoinVerticalandPlacewith background styling ensures proper alignment and appearance.packages/tui/internal/tui/detail_controller.go (2)
8-9: LGTM!The refactoring from a receiver-based method to an exported function aligns with the PR's goal of creating stateless, parameter-driven functions. The signature is idiomatic.
56-73: Defensive viewport management with proper focus handling.The addition of blur/focus calls ensures clean viewport state transitions. The nil checks for
m.eventsHistoryandm.summaryHistoryprevent panics during viewport switching.packages/tui/internal/tui/eval_detail_view.go (1)
98-120: Clean state construction and delegation.The conversion from
ModeltoDetailStateis well-structured, and delegating toevaluations.RenderDetailfollows the refactoring pattern of separating state from presentation.packages/tui/internal/tui/keyboard_controller.go (4)
6-7: LGTM!The new imports for
helpandreportpackages align with the refactoring to organize screen-specific logic into separate packages.
52-56: LGTM!Initializing the help viewport before switching to the help screen ensures the content is ready to display. This is good defensive programming.
262-264: Consistent refactoring to exported handlers.The routing now uses exported handler functions (
HandleConfigEnter,HandleConfigInput,HandleEvalFormInput,HandleEvalDetailInput) consistently, improving modularity and testability.Also applies to: 302-316
318-341: Well-integrated screen handlers with different patterns.The report screen uses an action-based result pattern (lines 321-336) which cleanly handles multiple outcomes (BackToDashboard, RegenerateSummary). The help screen uses a simpler pointer-update pattern (lines 339-341). Both approaches are appropriate for their respective complexity levels.
packages/tui/internal/screens/evaluations/types.go (2)
3-26: Well-organized state structure.
FormStateis cleanly organized with logical grouping (Dimensions, Form fields, Editing state, UI state) and clear inline documentation. This makes the data contract explicit for rendering functions.
28-52: Excellent state documentation.
DetailStateis well-documented with clear field groupings. The comments onFocusedViewport(line 40) and pre-rendered content (lines 42, 48) make the data contract explicit and help future maintainers understand the rendering model.packages/tui/internal/tui/common_controller.go (6)
10-11: LGTM! Package reorganization aligned with refactoring.The new imports for
screens/configandscreens/scenariossupport the centralized config persistence and scenario message type migration. This improves package organization and separates concerns.
283-283: LGTM! Centralized config persistence improves maintainability.The migration from
m.saveConfig()toconfig.Save(&m.config)centralizes configuration persistence logic. All call sites properly handle errors and display appropriate dialogs. This refactoring improves code organization and reduces duplication.Also applies to: 359-359, 407-407, 456-456
549-558: LGTM! Scenario generation handlers updated consistently.The scenario generation handlers now use
scenarios.GenerateScenariosMsgandscenarios.ScenariosGeneratedMsg, consistent with the package reorganization. The implementations correctly access message fields and forward to the appropriate commands.
561-583: LGTM! Scenario editor message handler properly refactored.The
handleScenarioEditorMsgnow acceptsscenarios.ScenarioEditorMsgand correctly handles different actions (saved, scenarios_generated, exit) with appropriate dialogs and screen transitions.
243-244: No issues found—initializeHelpViewport()is properly defined.The method is defined in
packages/tui/internal/tui/help_utils.goat line 12 as a method on the Model struct. The call at lines 243-244 incommon_controller.gois correct.
525-546: Message type migration verified and complete—no action needed.All four handler methods are correctly dispatched by the
Updatemethod inapp.go(lines 127–137). The implementation properly routesscenarios.StartInterviewMsg,scenarios.SendInterviewMessageMsg,scenarios.InterviewStartedMsg, andscenarios.InterviewResponseMsgto their respective handlers incommon_controller.go, which correctly use the message fields and forward to appropriate commands or update the scenario editor.
| func RenderDetail(state *DetailState) string { | ||
| t := theme.CurrentTheme() | ||
| if state == nil { | ||
| return lipgloss.NewStyle(). | ||
| Width(80). | ||
| Height(24). | ||
| Background(t.Background()). | ||
| Foreground(t.Text()). | ||
| Align(lipgloss.Center, lipgloss.Center). | ||
| Render("No evaluation active") | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard against nil theme to prevent panic.
theme.CurrentTheme() can return nil when no theme is set. Any subsequent calls to t.Background() or t.Text() would panic. Add an early nil check with a safe fallback.
func RenderDetail(state *DetailState) string {
t := theme.CurrentTheme()
+ if t == nil {
+ return "Theme not initialized"
+ }
if state == 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.
| func RenderDetail(state *DetailState) string { | |
| t := theme.CurrentTheme() | |
| if state == nil { | |
| return lipgloss.NewStyle(). | |
| Width(80). | |
| Height(24). | |
| Background(t.Background()). | |
| Foreground(t.Text()). | |
| Align(lipgloss.Center, lipgloss.Center). | |
| Render("No evaluation active") | |
| } | |
| func RenderDetail(state *DetailState) string { | |
| t := theme.CurrentTheme() | |
| if t == nil { | |
| return "Theme not initialized" | |
| } | |
| if state == nil { | |
| return lipgloss.NewStyle(). | |
| Width(80). | |
| Height(24). | |
| Background(t.Background()). | |
| Foreground(t.Text()). | |
| Align(lipgloss.Center, lipgloss.Center). | |
| Render("No evaluation active") | |
| } |
🤖 Prompt for AI Agents
In packages/tui/internal/screens/evaluations/detail_view.go around lines 11 to
22, theme.CurrentTheme() may return nil and subsequent calls to t.Background()
or t.Text() will panic; add an immediate nil check after obtaining t and replace
it with a safe fallback (e.g., theme.DefaultTheme() or a simple struct providing
sensible Background/Text values) so the subsequent rendering calls use non-nil
theme values and cannot panic.
| // Calculate available height for content area | ||
| availableHeight := state.Height - 5 // header(3) + helpText(2) | ||
|
|
||
| // Determine if we show summary section | ||
| showSummary := state.HasSummary | ||
|
|
||
| // Help text style (for bottom of screen) | ||
| helpStyle := lipgloss.NewStyle(). | ||
| Foreground(t.TextMuted()). | ||
| Background(t.Background()). | ||
| Width(state.Width). | ||
| Align(lipgloss.Center). | ||
| Padding(0, 1) | ||
|
|
||
| var helpMsg string | ||
| if state.Completed && showSummary { | ||
| helpMsg = "b Back s Stop r Report Tab Switch Focus ↑↓ scroll end auto-scroll" | ||
| } else if state.Completed { | ||
| helpMsg = "b Back s Stop r Report ↑↓ scroll end auto-scroll" | ||
| } else { | ||
| helpMsg = "b Back s Stop ↑↓ scroll end auto-scroll" | ||
| } | ||
| helpText := helpStyle.Render(helpMsg) | ||
|
|
||
| // Calculate main content area height | ||
| mainContentHeight := availableHeight | ||
|
|
||
| // Create content area | ||
| contentArea := lipgloss.NewStyle(). | ||
| Width(state.Width). | ||
| Height(mainContentHeight). | ||
| Background(t.Background()) |
There was a problem hiding this comment.
Clamp computed heights to prevent negative values.
On very small terminals, availableHeight (line 61) and mainContentHeight (line 85) can become negative, potentially causing layout glitches or panics in lipgloss. Add bounds checking to ensure non-negative values.
// Calculate available height for content area
availableHeight := state.Height - 5 // header(3) + helpText(2)
+ if availableHeight < 0 {
+ availableHeight = 0
+ }
// Determine if we show summary section📝 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 available height for content area | |
| availableHeight := state.Height - 5 // header(3) + helpText(2) | |
| // Determine if we show summary section | |
| showSummary := state.HasSummary | |
| // Help text style (for bottom of screen) | |
| helpStyle := lipgloss.NewStyle(). | |
| Foreground(t.TextMuted()). | |
| Background(t.Background()). | |
| Width(state.Width). | |
| Align(lipgloss.Center). | |
| Padding(0, 1) | |
| var helpMsg string | |
| if state.Completed && showSummary { | |
| helpMsg = "b Back s Stop r Report Tab Switch Focus ↑↓ scroll end auto-scroll" | |
| } else if state.Completed { | |
| helpMsg = "b Back s Stop r Report ↑↓ scroll end auto-scroll" | |
| } else { | |
| helpMsg = "b Back s Stop ↑↓ scroll end auto-scroll" | |
| } | |
| helpText := helpStyle.Render(helpMsg) | |
| // Calculate main content area height | |
| mainContentHeight := availableHeight | |
| // Create content area | |
| contentArea := lipgloss.NewStyle(). | |
| Width(state.Width). | |
| Height(mainContentHeight). | |
| Background(t.Background()) | |
| // Calculate available height for content area | |
| availableHeight := state.Height - 5 // header(3) + helpText(2) | |
| if availableHeight < 0 { | |
| availableHeight = 0 | |
| } | |
| // Determine if we show summary section | |
| showSummary := state.HasSummary | |
| // Help text style (for bottom of screen) | |
| helpStyle := lipgloss.NewStyle(). | |
| Foreground(t.TextMuted()). | |
| Background(t.Background()). | |
| Width(state.Width). | |
| Align(lipgloss.Center). | |
| Padding(0, 1) | |
| var helpMsg string | |
| if state.Completed && showSummary { | |
| helpMsg = "b Back s Stop r Report Tab Switch Focus ↑↓ scroll end auto-scroll" | |
| } else if state.Completed { | |
| helpMsg = "b Back s Stop r Report ↑↓ scroll end auto-scroll" | |
| } else { | |
| helpMsg = "b Back s Stop ↑↓ scroll end auto-scroll" | |
| } | |
| helpText := helpStyle.Render(helpMsg) | |
| // Calculate main content area height | |
| mainContentHeight := availableHeight | |
| // Create content area | |
| contentArea := lipgloss.NewStyle(). | |
| Width(state.Width). | |
| Height(mainContentHeight). | |
| Background(t.Background()) |
🤖 Prompt for AI Agents
In packages/tui/internal/screens/evaluations/detail_view.go around lines 60 to
91, the computed availableHeight and mainContentHeight can be negative on very
small terminals; clamp both values to a non-negative minimum (e.g., 0) after
their calculation so subsequent lipgloss layout calls never receive negative
dimensions; adjust availableHeight := max(0, state.Height-5) and ensure
mainContentHeight is similarly bounded before creating contentArea.
| // Calculate viewport dimensions | ||
| viewportWidth := m.width - 11 | ||
| viewportHeight := m.height - 9 | ||
| contentWidth := viewportWidth - 6 | ||
| actualViewportHeight := viewportHeight - 4 // Account for padding and border |
There was a problem hiding this comment.
Clamp dimensions to prevent negative values on small terminals.
The dimension calculations can produce negative values when the terminal is very small (e.g., m.width < 11 or m.height < 9), leading to rendering glitches or panics in downstream components like the markdown renderer and viewport.
Apply this diff to clamp dimensions:
-viewportWidth := m.width - 11
-viewportHeight := m.height - 9
-contentWidth := viewportWidth - 6
-actualViewportHeight := viewportHeight - 4 // Account for padding and border
+viewportWidth := max(m.width-11, 1)
+viewportHeight := max(m.height-9, 1)
+contentWidth := max(viewportWidth-6, 1)
+actualViewportHeight := max(viewportHeight-4, 1) // Account for padding and border📝 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 viewport dimensions | |
| viewportWidth := m.width - 11 | |
| viewportHeight := m.height - 9 | |
| contentWidth := viewportWidth - 6 | |
| actualViewportHeight := viewportHeight - 4 // Account for padding and border | |
| // Calculate viewport dimensions | |
| viewportWidth := max(m.width-11, 1) | |
| viewportHeight := max(m.height-9, 1) | |
| contentWidth := max(viewportWidth-6, 1) | |
| actualViewportHeight := max(viewportHeight-4, 1) // Account for padding and border |
🤖 Prompt for AI Agents
In packages/tui/internal/tui/help_utils.go around lines 19 to 23, the viewport
and content dimension calculations can become negative when the terminal is very
small (e.g., m.width < 11 or m.height < 9); clamp viewportWidth, viewportHeight,
contentWidth, and actualViewportHeight to non-negative minimums (0) after
computing them so downstream renderers won't receive negative sizes — replace
direct subtractions with the same calculations followed by bounds checks (if < 0
set to 0) or use a min/max helper to ensure each of viewportWidth = max(0,
m.width-11), viewportHeight = max(0, m.height-9), contentWidth = max(0,
viewportWidth-6), and actualViewportHeight = max(0, viewportHeight-4).
| t := theme.CurrentTheme() | ||
| renderer := shared.GetMarkdownRenderer(contentWidth, t.Background()) |
There was a problem hiding this comment.
Guard against nil theme to prevent panic.
theme.CurrentTheme() can return nil when no theme is set (see packages/tui/internal/theme/manager.go). Calling t.Background() on a nil theme will panic. Add an early guard and fallback.
Apply this diff to add a nil check:
t := theme.CurrentTheme()
+if t == nil {
+ // Fallback: skip markdown rendering or use plain content
+ m.helpViewport.SetSize(contentWidth, actualViewportHeight)
+ m.helpViewport.SetContent(help.GetHelpContent())
+ m.helpViewport.GotoTop()
+ return
+}
renderer := shared.GetMarkdownRenderer(contentWidth, t.Background())📝 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.
| t := theme.CurrentTheme() | |
| renderer := shared.GetMarkdownRenderer(contentWidth, t.Background()) | |
| t := theme.CurrentTheme() | |
| if t == nil { | |
| // Fallback: skip markdown rendering or use plain content | |
| m.helpViewport.SetSize(contentWidth, actualViewportHeight) | |
| m.helpViewport.SetContent(help.GetHelpContent()) | |
| m.helpViewport.GotoTop() | |
| return | |
| } | |
| renderer := shared.GetMarkdownRenderer(contentWidth, t.Background()) |
🤖 Prompt for AI Agents
In packages/tui/internal/tui/help_utils.go around lines 26-27, guard against
theme.CurrentTheme() returning nil by checking if t == nil and assigning a
sensible fallback (for example theme.DefaultTheme() or a newly constructed
default theme) before calling t.Background(); ensure any required import or
function (DefaultTheme or constructor) is used and available so the renderer
call uses a non-nil theme.
2 participants
Description
Motivation and Context
Type of Change
Changes Made
Screenshots/Examples (if applicable)
Checklist
uv run black .to format my codeuv run flake8 .and fixed all issuesuv run mypy --config-file .mypy.ini .and addressed type checking issuesuv run bandit -c .bandit.yaml -r .for security checksuv run pytestand all tests passTesting
Test Configuration:
Test Steps:
1.
2.
3.
Additional Notes
Related Issues/PRs