add support for multiple output formats#2
Conversation
📝 WalkthroughWalkthroughThis PR introduces a multi-format output rendering system for the ghlog CLI. It adds 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/domain/events.rs (1)
111-113: ⚡ Quick winRemove
DeleteEvent::ref_path()since it’s unused and yields misleadingtree/{ref_name}paths for deleted refs.
src/output/presentation.rsrenders deletions withtext(delete.ref_name())and the only.ref_path()call sites arepush.ref_path()/create.ref_path()—soDeleteEvent::ref_path()is currently dead code and a future footgun.🧹 Proposed fix
impl DeleteEvent { pub fn ref_name(&self) -> &str { strip_git_ref_prefix(&self.git_ref) } - - pub fn ref_path(&self) -> String { - format!("tree/{}", self.ref_name()) - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/events.rs` around lines 111 - 113, Remove the unused and misleading DeleteEvent::ref_path() method from the DeleteEvent implementation; delete the entire fn ref_path(&self) -> String { ... } for DeleteEvent and ensure no call sites rely on it (only PushEvent::ref_path() and CreateEvent::ref_path() should remain); verify presentation code renders deletions via delete.ref_name() so behavior is unchanged and no other types expect a ref_path on DeleteEvent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cmds/run.rs`:
- Around line 17-19: The early return on events.is_empty() unconditionally skips
all rendering (including HTML), so change the logic to only short-circuit for
non-HTML outputs: instead of returning immediately when events.is_empty(), check
the selected output format (the variable representing the format, e.g., format
or output_format) and only return Ok(()) when events.is_empty() AND the format
is not HTML (e.g., Format::Html); otherwise continue into the HTML rendering
path so an empty document/shell is produced. Ensure you update the branch around
events.is_empty() where it currently returns to reference the format enum/flag
and preserve the existing render_html/render() call flow for HTML.
In `@src/output/markdown.rs`:
- Around line 24-27: The fragment renderer inserts raw part.text into Markdown
which can break formatting; implement a helper like escape_markdown(text: &str)
-> String that escapes Markdown-sensitive characters (at minimum: \, `, *, _, {,
}, [, ], (, ), #, +, -, ., !) and converts newlines to spaces or escaped
newlines, then call this helper inside render_fragment for both the link label
and the plain text branch (i.e., replace uses of part.text.clone() with
escape_markdown(&part.text)); ensure the function is referenced from
render_fragment and named so it’s easy to find.
In `@src/output/terminal.rs`:
- Around line 33-37: render_fragment currently injects raw fragment.text into
OSC/ANSI sequences, enabling control-sequence injection; add a sanitizer and use
it before any terminal rendering. Implement a helper (e.g.,
sanitize_for_terminal) that removes or escapes terminal-control characters (ESC
0x1B, BEL, C0 controls, and any OSC/ST sequences) or percent-encodes unsafe
bytes, then call it on fragment.text inside render_fragment (both the Some(url)
branch and None branch) so only the sanitized string is embedded in format! and
returned.
---
Nitpick comments:
In `@src/domain/events.rs`:
- Around line 111-113: Remove the unused and misleading DeleteEvent::ref_path()
method from the DeleteEvent implementation; delete the entire fn ref_path(&self)
-> String { ... } for DeleteEvent and ensure no call sites rely on it (only
PushEvent::ref_path() and CreateEvent::ref_path() should remain); verify
presentation code renders deletions via delete.ref_name() so behavior is
unchanged and no other types expect a ref_path on DeleteEvent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee2f0f5c-f7e3-4bbf-8439-2db64a0ccd6b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlsrc/cli.rssrc/cmds/command.rssrc/cmds/run.rssrc/domain/events.rssrc/main.rssrc/output/assets/templates/editorial.htmlsrc/output/assets/templates/notebook.htmlsrc/output/assets/templates/terminal.htmlsrc/output/assets/templates/zine.htmlsrc/output/html.rssrc/output/markdown.rssrc/output/mod.rssrc/output/plain.rssrc/output/presentation.rssrc/output/terminal.rs
Add a dedicated output module that renders deterministic output in four formats:
html,plain,markdown, andterminal.Each renderer relies on a shared
EventPresentationshape that describes an event in display terms. This keeps event wording, link targets, and format-specific metadata in one place while letting each renderer decide how to present the final output.