Conversation
Reviewer's GuideThis PR adds a new “review comments” banner to separate reviewer summaries from individual comment threads by defining a new constant, exposing helper functions to write and print it, integrating it into the CLI flow, and updating docs and tests accordingly. Class diagram for updated summary module with comments banner helpersclassDiagram
class summary {
+START_BANNER: &str
+END_BANNER: &str
+COMMENTS_BANNER: &str
+print_start_banner() std::io::Result<()>
+print_end_banner() std::io::Result<()>
+print_comments_banner() std::io::Result<()>
+write_comments_banner<W: Write>(out: W) std::io::Result<()>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughAdd a mid-pipeline “review comments” banner and public helpers in the summary module; call the new banner after printing reviewer summaries in the PR output flow, return early on I/O errors, and update docs, tests, and dev-dependencies to reflect the revised sequence. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as pr subcommand
participant SUM as summary module
participant IO as stdout
U->>CLI: run_pr
rect rgba(200,230,255,0.12)
CLI->>SUM: print_start_banner()
SUM-->>IO: START_BANNER
end
CLI->>CLI: print reviewer summaries
rect rgba(220,255,220,0.12)
CLI->>SUM: print_comments_banner()
alt I/O error (e.g., BrokenPipe)
SUM-->>CLI: Err
CLI-->>U: terminate early (skip threads & end banner)
else OK
SUM-->>IO: COMMENTS_BANNER
CLI->>CLI: render threads
CLI->>SUM: print_end_banner()
SUM-->>IO: END_BANNER
CLI-->>U: done
end
end
note over CLI,SUM: COMMENTS_BANNER is only emitted when threads will be printed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
README.md(1 hunks)docs/vk-design.md(2 hunks)src/main.rs(3 hunks)src/summary.rs(3 hunks)tests/cli.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Files:
tests/cli.rssrc/main.rssrc/summary.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/cli.rssrc/main.rssrc/summary.rs
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
README.mddocs/vk-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
README.mddocs/vk-design.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/vk-design.md
🧬 Code graph analysis (1)
src/main.rs (1)
src/summary.rs (5)
print_comments_banner(162-164)print_end_banner(196-198)print_start_banner(129-131)print_summary(87-94)summarize_files(35-47)
🔍 Remote MCP Deepwiki
Here’s what the DeepWiki lookup surfaces as directly relevant to reviewing PR #97:
• There is no built-in vk command or GraphQL call that lists all files changed in a pull request—vk only fetches review threads (with each comment’s path and diffHunk) and uses that to drive its output.
• The summarize_files function walks each fetched ReviewThread and tallies comment counts per path, producing a (file_path, comment_count) list that gets printed by print_summary. This means any “file summary” in vk reflects where comments landed, not the complete diff itself.
• The GraphQL THREADS_QUERY used under the hood retrieves path and diffHunk only for comments—it does not pull a full file-list or change overview.
These facts clarify that vk’s PR output focuses on comment locations rather than a full per-file diff summary.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (4)
src/summary.rs (2)
133-165: LGTM: banner helpers are minimal, correct, and documented.Public API and error propagation mirror existing start/end helpers.
297-306: LGTM: precise unit test for banner output.Test asserts exact text plus newline; failure will be crisp.
src/main.rs (2)
11-16: LGTM: module docs accurately describe the new banner and helpers.Wording is clear and aligned with README/docs.
40-42: LGTM: import the new banner helper alongside existing exports.Import grouping is consistent.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/main.rs (1)
254-259: Fix stdout double-lock risk before printing the comments banner.A StdoutLock is still held when calling
print_comments_banner(), which locks stdout again and can deadlock.Apply:
let skin = MadSkin::default(); let latest = latest_reviews(reviews); - let stdout = std::io::stdout(); - let mut handle = stdout.lock(); - if let Err(e) = print_reviews(&mut handle, &skin, &latest) { - if caused_by_broken_pipe(&e) { - return Ok(()); - } - error!("error printing review: {e}"); - } + { + let stdout = std::io::stdout(); + let mut handle = stdout.lock(); + if let Err(e) = print_reviews(&mut handle, &skin, &latest) { + if caused_by_broken_pipe(&e) { + return Ok(()); + } + error!("error printing review: {e}"); + } + } // drop handle before locking stdout again - // Stop if the comments banner cannot be written, usually indicating stdout - // has been closed, as printing threads would also fail. + // Stop if the comments banner cannot be written, usually indicating stdout + // has been closed, as printing threads would also fail. if handle_banner(print_comments_banner, "comments") { return Ok(()); }AI agent prompt: Implement the scoped drop of the StdoutLock in src/main.rs::generate_pr_output as shown in the diff, ensuring no function attempts to lock stdout while a lock is already held. Re-run CLI integration tests that exercise the new banner to validate no regressions.src/summary.rs (2)
16-17: Align COMMENTS_BANNER width with START/END banners.Match delimiter width for visual symmetry and to keep tests/docs consistent.
-/// Banner printed before individual review comments. -pub const COMMENTS_BANNER: &str = "======== review comments ========"; +/// Banner printed before individual review comments. +pub const COMMENTS_BANNER: &str = "========== review comments ==========";Follow up: update tests/cli.rs, docs/vk-design.md, and README.md literals to match.
309-327: Deduplicate three nearly identical IO error tests via rstest parameterisation.Collapse the start/comments/end variants into one parameterised test to eliminate duplication (also addresses the CodeScene warning).
- #[test] - fn write_start_banner_propagates_io_errors() { - use std::io::{self, Write}; - struct ErrorWriter; - impl Write for ErrorWriter { - fn write(&mut self, _buf: &[u8]) -> io::Result<usize> { - Err(io::Error::other("Simulated stdout write error")) - } - fn flush(&mut self) -> io::Result<()> { Ok(()) } - } - let mut writer = ErrorWriter; - let err = write_start_banner(&mut writer).expect_err("expect error"); - assert_eq!(err.to_string(), "Simulated stdout write error"); - } - - #[test] - fn write_comments_banner_propagates_io_errors() { - use std::io::{self, Write}; - struct ErrorWriter; - impl Write for ErrorWriter { - fn write(&mut self, _buf: &[u8]) -> io::Result<usize> { - Err(io::Error::other("Simulated stdout write error")) - } - fn flush(&mut self) -> io::Result<()> { Ok(()) } - } - let mut writer = ErrorWriter; - let err = write_comments_banner(&mut writer).expect_err("expect error"); - assert_eq!(err.to_string(), "Simulated stdout write error"); - } - - #[test] - fn write_end_banner_propagates_io_errors() { - use std::io::{self, Write}; - struct ErrorWriter; - impl Write for ErrorWriter { - fn write(&mut self, _buf: &[u8]) -> io::Result<usize> { - Err(io::Error::other("Simulated stdout write error")) - } - fn flush(&mut self) -> io::Result<()> { Ok(()) } - } - let mut writer = ErrorWriter; - let err = write_end_banner(&mut writer).expect_err("expect error"); - assert_eq!(err.to_string(), "Simulated stdout write error"); - } + fn call_start(w: &mut dyn Write) -> std::io::Result<()> { write_start_banner(w) } + fn call_comments(w: &mut dyn Write) -> std::io::Result<()> { write_comments_banner(w) } + fn call_end(w: &mut dyn Write) -> std::io::Result<()> { write_end_banner(w) } + + #[rstest] + #[case(call_start as fn(&mut dyn Write) -> std::io::Result<()>)] + #[case(call_comments as fn(&mut dyn Write) -> std::io::Result<()>)] + #[case(call_end as fn(&mut dyn Write) -> std::io::Result<()>)] + fn banner_functions_propagate_io_errors( + #[case] banner_fn: fn(&mut dyn Write) -> std::io::Result<()>, + ) { + use std::io::{self, Write}; + struct ErrorWriter; + impl Write for ErrorWriter { + fn write(&mut self, _buf: &[u8]) -> io::Result<usize> { + Err(io::Error::other("Simulated stdout write error")) + } + fn flush(&mut self) -> io::Result<()> { Ok(()) } + } + let mut writer = ErrorWriter; + let err = banner_fn(&mut writer).expect_err("expect error"); + assert_eq!(err.to_string(), "Simulated stdout write error"); + }AI agent prompt: Replace the three IO-error banner tests with a single rstest-parameterised test using wrapper functions (`call_start`, `call_comments`, `call_end`) that adapt each banner writer to `fn(&mut dyn Write) -> io::Result<()>`. Ensure imports remain unchanged and all assertions are preserved.Also applies to: 358-376, 269-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(1 hunks)src/main.rs(3 hunks)src/summary.rs(4 hunks)tests/cli.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Mandate caret requirements for all dependencies: All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3").
Prohibit unstable version specifiers: The use of wildcard (*), or open-ended inequality (>=) version requirements are strictly forbidden inCargo.toml. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Files:
tests/cli.rssrc/main.rssrc/summary.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/cli.rssrc/main.rssrc/summary.rs
🧬 Code graph analysis (1)
src/main.rs (1)
src/summary.rs (5)
print_comments_banner(162-165)print_end_banner(197-199)print_start_banner(129-131)print_summary(87-94)summarize_files(35-47)
🔍 Remote MCP
Here’s what the DeepWiki lookup surfaces as directly relevant to reviewing PR #97:
• There is no built-in vk command or GraphQL call that lists all files changed in a pull request—vk only fetches review threads (with each comment’s path and diffHunk) and uses that to drive its output.
• The summarize_files function walks each fetched ReviewThread and tallies comment counts per path, producing a (file_path, comment_count) list that gets printed by print_summary. This means any “file summary” in vk reflects where comments landed, not the complete diff itself.
• The GraphQL THREADS_QUERY used under the hood retrieves path and diffHunk only for comments—it does not pull a full file-list or change overview.
These facts clarify that vk’s PR output focuses on comment locations rather than a full per-file diff summary.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (3)
src/main.rs (2)
11-16: Docs: call out the new mid-stream banner.The module docs accurately describe the new “review comments” banner and placement.
40-42: Import the new banner helpers.The import set is correct and compiles cleanly.
src/summary.rs (1)
299-307: LGTM: exact-text assertion for the comments banner.This asserts the newline and guards against drift. Keep it.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos Let me verify each of your points by examining the current code: 1. I/O error propagation tests: ✅ RESOLVED
2. CLI test banner ordering: ✅ RESOLVED
3. Early return behavior on handle_banner failure: This behavior appears intentional and consistent with the existing error handling pattern, but you should confirm if this is the desired behavior for your use case. All three points have been addressed, with the third requiring your confirmation on the intended error handling behavior. 🏁 Script executed: Length of output: 1677 🏁 Script executed: Length of output: 2347 @leynos Based on my examination of the code, here's the status of your three points: 1. I/O error propagation tests: ❌ NOT FULLY RESOLVED 2. CLI test banner ordering: ❌ NOT FULLY RESOLVED 3. Early return behavior: ✅ CONFIRMED AS INTENDED Let me get more complete information about the missing tests to give you a definitive answer. 🏁 Script executed: Length of output: 2247 🏁 Script executed: Length of output: 2612 🏁 Script executed: Length of output: 5310 @leynos Based on my thorough examination of the codebase, here's the definitive status of your three points: 1. I/O error propagation tests: ✅ RESOLVED
2. CLI test banner ordering: ❌ NOT FULLY RESOLVED 3. Early return behavior: ✅ CONFIRMED AS INTENDED Let me check the 🏁 Script executed: Length of output: 1515 @leynos Perfect! Now I have the complete picture. Here's the final status of your three points: 1. I/O error propagation tests: ✅ FULLY RESOLVED
2. CLI test banner ordering: ✅ FULLY RESOLVED
3. Early return behavior: ✅ CONFIRMED AS INTENDED All three points have been successfully resolved in the current PR! 🎉 ( ・ω・) 🏁 Script executed: Length of output: 858 @leynos Excellent! Now I can confirm with complete certainty: All three points have been FULLY RESOLVED ✅
The implementation is thorough and follows best practices. All your requirements have been met! (｡◕‿◕｡) |
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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)
tests/cli.rs (1)
44-51: Eliminate hard-coded banner strings in empty-state cases.Build expected output with exported constants to prevent test breakage if banner text changes.
#[rstest] #[case( - Vec::new(), - "========== code review ==========\nNo unresolved comments.\n========== end of code review ==========\n" + Vec::new(), + format!("{START_BANNER}\nNo unresolved comments.\n{END_BANNER}\n") )] #[case( - vec!["no_such_file.rs"], - "========== code review ==========\nNo unresolved comments for the specified files.\n========== end of code review ==========\n", + vec!["no_such_file.rs"], + format!("{START_BANNER}\nNo unresolved comments for the specified files.\n{END_BANNER}\n") )]Apply outside this range:
- Change parameter type to accept owned Strings.
- Remove the redundant to_string().
-async fn pr_empty_state( - #[case] extra_args: Vec<&'static str>, - #[case] expected_output: &'static str, -) { +async fn pr_empty_state( + #[case] extra_args: Vec<&'static str>, + #[case] expected_output: String, +) { @@ - let output = expected_output.to_string(); + let output = expected_output;
♻️ Duplicate comments (5)
Cargo.toml (1)
59-61: LGTM: Gate libc to Unix-only dev builds.Scope
libcto Unix dev targets to avoid unnecessary pulls on non-Unix CI/machines. This addresses the earlier ask.docs/vk-design.md (2)
39-42: LGTM: Document control flow and broken-pipe behaviour precisely.Call out early termination and conditional emission of the comments banner.
25-26: Echo the empty-state omission here for consistency.State the omission inline with the first mention of the comments banner to avoid readers missing the later clarification.
- `end of code review` banner. A `review comments` banner separates reviewer - summaries from the printed threads. + `end of code review` banner. A `review comments` banner separates reviewer + summaries from the printed threads; omit this banner when no threads will be printed.src/main.rs (1)
256-261: Fix stdout deadlock: drop StdoutLock before printing the comments banner.A
StdoutLockis held whileprint_comments_banner()re-locks stdout, risking self-deadlock. Release the lock before callinghandle_banner.- let stdout = std::io::stdout(); - let mut handle = stdout.lock(); - if let Err(e) = print_reviews(&mut handle, &skin, &latest) { - if caused_by_broken_pipe(&e) { - return Ok(()); - } - error!("error printing review: {e}"); - } + { + let stdout = std::io::stdout(); + let mut handle = stdout.lock(); + if let Err(e) = print_reviews(&mut handle, &skin, &latest) { + if caused_by_broken_pipe(&e) { + return Ok(()); + } + error!("error printing review: {e}"); + } + } // drop handle before (re)locking stdout for the bannerKeep the subsequent
handle_banner(print_comments_banner, "comments")as-is.src/summary.rs (1)
129-161: Unify write_comments_banner signature with start/end; simplify print.Take W by value for parity with write_start_banner/write_end_banner and avoid the temporary in print_comments_banner.
-pub fn write_comments_banner<W: Write>(out: &mut W) -> std::io::Result<()> { - write_banner(out, COMMENTS_BANNER) -} +pub fn write_comments_banner<W: Write>(out: W) -> std::io::Result<()> { + write_banner(out, COMMENTS_BANNER) +} @@ -pub fn print_comments_banner() -> std::io::Result<()> { - let mut out = std::io::stdout().lock(); - write_comments_banner(&mut out) -} +pub fn print_comments_banner() -> std::io::Result<()> { + write_comments_banner(std::io::stdout().lock()) +}Doc examples and existing tests remain valid because passing &mut T satisfies W: Write.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Cargo.toml(1 hunks)docs/vk-design.md(2 hunks)src/banners.rs(1 hunks)src/lib.rs(1 hunks)src/main.rs(3 hunks)src/summary.rs(5 hunks)tests/cli.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Files:
src/banners.rssrc/lib.rstests/cli.rssrc/summary.rssrc/main.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/banners.rssrc/lib.rstests/cli.rssrc/summary.rssrc/main.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Mandate caret requirements for all dependencies: All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3").
Prohibit unstable version specifiers: The use of wildcard (*), or open-ended inequality (>=) version requirements are strictly forbidden inCargo.toml. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/vk-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/vk-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/vk-design.md
🧬 Code graph analysis (2)
tests/cli.rs (1)
tests/utils.rs (1)
shutdown(28-31)
src/main.rs (1)
src/summary.rs (5)
print_comments_banner(158-161)print_end_banner(193-195)print_start_banner(125-127)print_summary(83-90)summarize_files(31-43)
🔍 Remote MCP
Here’s what the DeepWiki lookup surfaces as directly relevant to reviewing PR #97:
• There is no built-in vk command or GraphQL call that lists all files changed in a pull request—vk only fetches review threads (with each comment’s path and diffHunk) and uses that to drive its output.
• The summarize_files function walks each fetched ReviewThread and tallies comment counts per path, producing a (file_path, comment_count) list that gets printed by print_summary. This means any “file summary” in vk reflects where comments landed, not the complete diff itself.
• The GraphQL THREADS_QUERY used under the hood retrieves path and diffHunk only for comments—it does not pull a full file-list or change overview.
These facts clarify that vk’s PR output focuses on comment locations rather than a full per-file diff summary.
🔇 Additional comments (12)
src/banners.rs (1)
1-10: LGTM: Centralise banner constants with docs.Provide shared, documented constants and a module-level
//!header. Matches project docs and tests.src/lib.rs (1)
6-6: LGTM: Exposebannersfrom the library.Enable external tests and crates to import banner tokens.
src/main.rs (2)
11-17: LGTM: Update crate docs to mention the comments banner and empty-state.Docs accurately describe sequencing and omission.
42-44: LGTM: Wire inprint_comments_banneralongside existing helpers.Import set is coherent with the new mid-pipeline step.
tests/cli.rs (3)
13-13: Import banner constants to avoid string drift — good.Keep tests coupled to the single source of truth for banner text.
128-133: Delegate assertions to helpers — good.Keep the test body focused on setup and execution.
160-180: Ordering checks are correct and complete.Verify start < comments < thread < end — nice.
src/summary.rs (5)
10-13: Import banner constants from the shared module — good.Depend on crate-wide constants for consistency.
211-223: Introduce ErrorWriter for IO error simulation — good.Test-only utility is minimal and scoped correctly.
277-286: Parametrise IO error propagation across all banners — good.Remove duplication and exercise each writer path.
300-307: Verify exact output from write_comments_banner — good.Assert the trailing newline and token precisely.
309-339: Unix BrokenPipe test includes SAFETY rationale — good.Validate stderr-on-broken-pipe behaviour and restore stdout without leaking FDs.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
1 participant
Summary
review commentsbanner between reviewer summaries and thread contentTesting
make fmtmake lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_68b01c568ad48322b7dd71842fcfced8
Summary by Sourcery
Add a new "review comments" banner and integrate it into the PR output flow to separate reviewer summaries from comment threads
New Features:
Enhancements:
prcommandDocumentation:
Tests: