Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdds a glamour-based Markdown renderer, caches it per theme/width, and wires it into chat, scenario, report, and evaluation views. MessageHistoryView can toggle Markdown rendering; app version bumped to 0.1.12 and go.mod updated with Markdown/terminal styling deps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Model as TUI Model
participant RendererLib as Markdown Util
participant Views as Chat/Report/Summary Views
User->>Model: Open report/eval/interview
Model->>Model: getMarkdownRenderer(width, theme)
alt Cache miss or theme/width changed
Model->>RendererLib: GetMarkdownRenderer(width, bg)
RendererLib-->>Model: *glamour.TermRenderer
Model->>Model: Cache renderer
else Cache valid
Model-->>Model: Reuse cached renderer
end
Model->>Views: SetMarkdownRenderer(renderer)
Views-->>User: Render content (Markdown-enabled)
sequenceDiagram
autonumber
participant View as MessageHistoryView
participant Renderer as glamour.TermRenderer
View->>View: RenderMessage(msg)
alt Markdown enabled and renderer set
View->>Renderer: Render(msg.Content)
Renderer-->>View: renderedText or error
alt Render ok
View-->>View: Append renderedText
else Render error
View-->>View: Fallback to plain text path
end
else Markdown disabled
View-->>View: Wrap/indent plain text
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/tui/go.sumis excluded by!**/*.sumuv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
packages/tui/go.mod(1 hunks)packages/tui/internal/components/chat_view.go(2 hunks)packages/tui/internal/components/message_history_view.go(4 hunks)packages/tui/internal/components/scenario_editor.go(3 hunks)packages/tui/internal/components/scenario_list.go(1 hunks)packages/tui/internal/tui/app.go(6 hunks)packages/tui/internal/tui/eval_detail.go(1 hunks)packages/tui/internal/tui/markdown_renderer.go(1 hunks)packages/tui/internal/tui/report_view.go(1 hunks)pyproject.toml(1 hunks)rogue/__init__.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
pyproject.toml
📄 CodeRabbit inference engine (AGENTS.md)
Manage dependencies with uv and declare them in pyproject.toml
Files:
pyproject.toml
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format Python code with Black
Ensure code passes flake8 linting
Run mypy with the repository configuration for static typing
Run Bandit security checks using .bandit.yaml configuration
Use isort import conventions for import ordering
Add type hints to all function signatures
Follow PEP 8 naming (snake_case for variables/functions, PascalCase for classes)
Use try/except around code that may raise exceptions
Files:
rogue/__init__.py
🧬 Code graph analysis (2)
packages/tui/internal/tui/app.go (3)
packages/tui/internal/tui/eval_ui.go (1)
EvaluationViewState(11-36)packages/tui/internal/theme/manager.go (2)
CurrentTheme(67-76)CurrentThemeName(79-84)packages/tui/internal/tui/markdown_renderer.go (1)
GetMarkdownRenderer(21-28)
packages/tui/internal/tui/markdown_renderer.go (2)
packages/tui/internal/theme/manager.go (1)
CurrentTheme(67-76)packages/tui/internal/styles/styles.go (2)
TextMuted(22-22)Error(17-17)
⏰ 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: Cursor Bugbot
- GitHub Check: codestyle
- GitHub Check: rogue_sanity
🔇 Additional comments (8)
rogue/__init__.py (1)
47-47: LGTM! Version bump aligns with the markdown rendering feature.The version increment to 0.1.12 is appropriate for the markdown rendering capabilities added in this PR.
pyproject.toml (1)
3-3: LGTM! Version bump is consistent.The version update matches the corresponding change in
rogue/__init__.pyand is appropriate for this feature release.packages/tui/go.mod (1)
17-33: LGTM! Indirect dependencies added for markdown rendering.These transitive dependencies are correctly managed by Go modules and support the glamour-based markdown rendering feature. The indirect markers indicate these were automatically resolved by
go mod tidy.Also applies to: 39-39
packages/tui/internal/components/scenario_list.go (1)
165-169: LGTM! Markdown renderer integration follows defensive pattern.The nil check before applying the markdown renderer is good defensive programming. The renderer is correctly propagated to the interview chat view after initialization.
packages/tui/internal/components/chat_view.go (1)
78-83: LGTM! Clean delegation to message history.The SetMarkdownRenderer method properly forwards the renderer to the underlying MessageHistoryView with appropriate nil checking.
packages/tui/internal/tui/eval_detail.go (1)
199-201: LGTM! Markdown renderer integration for evaluation summary.The markdown renderer is correctly applied to the summary history after color configuration, enabling formatted rendering of evaluation summaries.
packages/tui/internal/tui/report_view.go (1)
71-73: LGTM! Markdown renderer integration for report content.The markdown renderer is correctly wired into the report rendering flow after color configuration, enabling formatted display of evaluation reports.
packages/tui/internal/components/scenario_editor.go (1)
58-59: LGTM! Markdown renderer storage and propagation.The implementation properly stores the markdown renderer and forwards it to the interview chat view when available. The nil check ensures safe propagation even when the chat view hasn't been initialized yet.
Also applies to: 113-119
| messageLines = append(messageLines, prefix, rendered) | ||
| } else { |
There was a problem hiding this comment.
Preserve role styling when markdown is enabled
In the markdown branch we append the raw prefix, so the user/assistant colors applied via textStyle disappear as soon as markdown rendering is turned on. That regression makes the roles much harder to distinguish. Please keep rendering the prefix through the existing style before appending it.
- messageLines = append(messageLines, prefix, rendered)
+ styledPrefix := textStyle.Render(prefix)
+ messageLines = append(messageLines, styledPrefix, rendered)📝 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.
| messageLines = append(messageLines, prefix, rendered) | |
| } else { | |
| styledPrefix := textStyle.Render(prefix) | |
| messageLines = append(messageLines, styledPrefix, rendered) | |
| } else { |
🤖 Prompt for AI Agents
In packages/tui/internal/components/message_history_view.go around lines
337-338, the markdown branch currently appends the raw prefix which drops the
role textStyle (user/assistant) coloring; instead render the prefix with the
existing textStyle before appending so the role styling is preserved when
markdown is enabled—apply the same styling/render path used for the non-markdown
branch to the prefix, then append the styled prefix and the rendered markdown
content in the same order.
| func AdaptiveColorToString(color compat.AdaptiveColor) *string { | ||
| if _, ok := color.Dark.(lipgloss.NoColor); ok { | ||
| return nil | ||
| } | ||
| c1, _ := colorful.MakeColor(color.Dark) | ||
| return stringPtr(c1.Hex()) |
There was a problem hiding this comment.
Fix adaptive color selection for dark backgrounds
Right now AdaptiveColorToString always returns the .Dark branch of an adaptive color. On a dark terminal that branch holds the low-contrast variant that is meant for light backgrounds, so markdown text rendered through glamour becomes nearly invisible as soon as you switch to a dark theme. Please resolve the adaptive color first (e.g. via lipgloss.HasDarkBackground() or the theme’s own darkness flag) and only then convert it to hex, still honoring NoColor. That prevents the regression where dark themes lose readable markdown output.
func AdaptiveColorToString(color compat.AdaptiveColor) *string {
- if _, ok := color.Dark.(lipgloss.NoColor); ok {
- return nil
- }
- c1, _ := colorful.MakeColor(color.Dark)
+ var source lipgloss.Color
+ if lipgloss.HasDarkBackground() {
+ source = color.Light
+ } else {
+ source = color.Dark
+ }
+ if _, ok := source.(lipgloss.NoColor); ok {
+ return nil
+ }
+ c1, _ := colorful.MakeColor(source)
return stringPtr(c1.Hex())
}📝 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 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(color compat.AdaptiveColor) *string { | |
| var source lipgloss.Color | |
| if lipgloss.HasDarkBackground() { | |
| source = color.Light | |
| } else { | |
| source = color.Dark | |
| } | |
| if _, ok := source.(lipgloss.NoColor); ok { | |
| return nil | |
| } | |
| c1, _ := colorful.MakeColor(source) | |
| return stringPtr(c1.Hex()) | |
| } |
🤖 Prompt for AI Agents
In packages/tui/internal/tui/markdown_renderer.go around lines 270 to 275, the
function currently always uses color.Dark which picks the low-contrast variant
on dark terminals; instead resolve which branch to use first (e.g. check
lipgloss.HasDarkBackground() or the theme’s darkness flag) and then handle
NoColor for that resolved branch, returning nil if it’s a lipgloss.NoColor;
finally convert the chosen adaptive color to hex and return its pointer so the
proper high-contrast color is used on dark backgrounds.
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
LGTM 👍
There is a cursor comment that I think we should tackle
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/tui/internal/tui/app.go (1)
883-896: Duplicate initialization logic already flagged.This code duplicates the
evalStateinitialization from thenew_evaluationcommand handler (lines 495-507), which was already identified in previous reviews as causing potential inconsistencies.
🧹 Nitpick comments (1)
packages/tui/internal/tui/app.go (1)
169-179: Well-implemented cell escaping for markdown tables.The function correctly handles the critical cases:
- Pipes escaped with HTML entities to preserve table structure
- Newlines replaced with spaces to keep content on single line
- Whitespace trimmed for clean presentation
Optional enhancement: Consider also escaping backticks (
`) to prevent unintended inline code formatting within cells, though this is a minor concern.If you want to add backtick escaping:
func escapeMarkdownTableCell(s string) string { // Replace pipe characters with HTML entity to prevent table structure break s = strings.ReplaceAll(s, "|", "|") + // Replace backticks to prevent inline code formatting + s = strings.ReplaceAll(s, "`", "`") // Replace newlines with spaces to keep content on single line s = strings.ReplaceAll(s, "\n", " ") s = strings.ReplaceAll(s, "\r", " ") // Trim extra whitespace s = strings.TrimSpace(s) return s }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tui/internal/tui/app.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tui/internal/tui/app.go (3)
packages/tui/internal/tui/eval_ui.go (1)
EvaluationViewState(11-36)packages/tui/internal/theme/manager.go (2)
CurrentTheme(67-76)CurrentThemeName(79-84)packages/tui/internal/tui/markdown_renderer.go (1)
GetMarkdownRenderer(21-28)
⏰ 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: rogue_sanity
- GitHub Check: Cursor Bugbot
- GitHub Check: codestyle
🔇 Additional comments (6)
packages/tui/internal/tui/app.go (6)
13-13: LGTM: Glamour import added for Markdown rendering.Clean addition to support the new markdown rendering feature.
127-139: Excellent fix for markdown table formatting.This implementation properly addresses the markdown table formatting issue flagged in previous reviews by:
- Creating well-structured markdown tables with headers and separators
- Escaping special characters in each cell via
escapeMarkdownTableCell- Preventing table structure breaks from pipes and newlines in the data
The table will now render correctly even when
Scenario,Status, andOutcomefields contain special characters.
231-234: Good caching strategy for markdown renderer.The added fields support efficient caching by tracking the renderer instance along with the width and theme parameters used to create it. This avoids unnecessary recreation of the renderer when dimensions or theme haven't changed.
306-306: Version bumped to v0.1.12.Standard version increment for this feature release.
872-876: LGTM: Markdown renderer properly wired to scenario editor.The renderer is obtained via the caching getter and correctly passed to the scenario editor for rendering interview responses.
878-902: Well-implemented renderer caching with lazy initialization.The caching logic is sound:
- Recreates renderer only when width or theme changes
- Width calculation (
m.width - 8) accounts for padding/margins with a safe minimum of 40- Lazy initialization pattern is appropriate for this use case
- Cache invalidation checks all relevant parameters
The pointer receiver correctly enables caching mutations, and this pattern works correctly within Bubble Tea's Update flow where the modified Model copy is returned.
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