Improvements #5
Improvements #5
Conversation
WalkthroughBumps dependency windows-sys 0.61.0 → 0.61.1. Adds webhook.body_preview_length (serde default 80) and Config helpers: Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config.rs (1)
12-47: Blocking: add a Serde default forbody_preview_length.Existing
config.tomlfiles in the wild do not contain the newbody_preview_lengthfield, sotoml::from_strwill now fail and the CLI will refuse to start. Please make the field optional by adding a Serde default (e.g.#[serde(default = "WebhookConfig::default_body_preview_length")]) or similar so older configs deserialize cleanly. This is a regression that must be fixed before release.#[derive(Debug, Deserialize, Serialize)] pub struct WebhookConfig { pub base_url: String, pub default_count: u32, pub default_interval: u64, pub show_headers_by_default: bool, pub show_full_body_by_default: bool, - pub body_preview_length: usize, + #[serde(default = "WebhookConfig::default_body_preview_length")] + pub body_preview_length: usize, } impl WebhookConfig { + fn default_body_preview_length() -> usize { + 80 + } }
🧹 Nitpick comments (4)
src/commands.rs (4)
16-16: Avoid referencing a temporary String in join_url_segments.Bind token.to_string() to a local first for clarity/lifetime readability.
- let webhook_url = Config::join_url_segments(config.get_base_url(), &[&token.to_string()]); + let token_str = token.to_string(); + let webhook_url = Config::join_url_segments(config.get_base_url(), &[&token_str]);
108-119: Keep iteration order consistent for “new requests” and fix rustfmt.Reverse here too so newest appears last, matching initial/logs views; also reflow the summary call.
- for request in &new_requests { + for request in new_requests.iter().rev() { println!("{}", "NEW REQUEST".bright_green().bold()); - print_request_summary(request, !full_body, config.get_body_preview_length()); // Don't show body preview in full body mode + print_request_summary( + request, + !full_body, // Don't show body preview in full body mode + config.get_body_preview_length(), + );
69-71: Avoid allocations and MSRV friction in method filtering.Use eq_ignore_ascii_case and Option::map_or to avoid to_lowercase allocations and reliance on Option::is_none_or.
- .filter(|req| { - method_filter.is_none_or(|method| { - req.message_object.method.to_lowercase() == method.to_lowercase() - }) - }) + .filter(|req| { + method_filter.map_or(true, |method| { + req.message_object.method.eq_ignore_ascii_case(method) + }) + })- .filter(|req| { - method_filter.is_none_or(|method| { - req.message_object.method.to_lowercase() == method.to_lowercase() - }) - }) + .filter(|req| { + method_filter.map_or(true, |method| { + req.message_object.method.eq_ignore_ascii_case(method) + }) + })Also applies to: 153-155
179-182: Format print_request_summary call across multiple lines to satisfy rustfmt Confirm reverse iteration still places newest requests at the end.- for request in filtered_requests.iter().rev() { - print_request_summary(request, !full_body, config.get_body_preview_length()); // Don't show body preview in full body mode + for request in filtered_requests.iter().rev() { + print_request_summary( + request, + !full_body, // Don't show body preview in full body mode + config.get_body_preview_length(), + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)config.toml(1 hunks)src/client.rs(1 hunks)src/commands.rs(6 hunks)src/config.rs(3 hunks)src/display.rs(5 hunks)src/main.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/client.rs (1)
src/config.rs (1)
join_url_segments(64-76)
src/commands.rs (2)
src/display.rs (4)
print_full_request_body(46-79)print_request_details(81-155)print_request_headers(35-44)print_request_summary(10-33)src/config.rs (1)
join_url_segments(64-76)
🪛 GitHub Actions: Rust
src/config.rs
[error] 64-64: cargo fmt --check failed: formatting issues detected. Remove unnecessary whitespace around code block.
[error] 71-71: cargo fmt --check failed: formatting issues detected. Ensure consistent formatting of URL assembly.
src/commands.rs
[error] 87-87: cargo fmt --check failed: formatting issues detected. Adjust the print_request_summary call formatting.
[error] 107-107: cargo fmt --check failed: formatting issues detected. Adjust the print_request_summary call formatting.
src/display.rs
[error] 7-7: cargo fmt --check failed: formatting issues detected. Function signature formatting for print_request_summary does not conform to style.
[error] 8-14: cargo fmt --check failed: formatting issues detected. Reformat function signature and parameters for print_request_summary.
🔇 Additional comments (3)
src/commands.rs (3)
11-12: Imports update looks good.The display API changes are reflected correctly.
41-48: Threading Config into monitor_requests is correct.Enables config-driven preview length. No issues spotted.
133-139: show_logs now receiving Config: LGTM.Allows passing preview length; no issues.
| // Reverse the order so latest requests appear at the end | ||
| for request in filtered_requests.iter().rev() { | ||
| print_request_summary(request, !full_body, config.get_body_preview_length()); // Don't show body preview in full body mode | ||
| if show_headers { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Fix rustfmt failure and verify ordering assumption.
- CI is failing due to formatting; reflow the call to satisfy rustfmt.
- Confirm get_requests sort order; reversing here assumes newest is first. If not, this may invert unexpectedly.
- // Reverse the order so latest requests appear at the end
- for request in filtered_requests.iter().rev() {
- print_request_summary(request, !full_body, config.get_body_preview_length()); // Don't show body preview in full body mode
+ // Reverse the order so latest requests appear at the end
+ for request in filtered_requests.iter().rev() {
+ print_request_summary(
+ request,
+ !full_body, // Don't show body preview in full body mode
+ config.get_body_preview_length(),
+ );#!/usr/bin/env bash
set -e
# Show how filtered_requests is produced in commands.rs
rg -n 'filtered_requests' src/commands.rs
# Find the definition of get_requests across the codebase
rg -nP 'fn\s+get_requests' -C3 .🏁 Script executed:
#!/usr/bin/env bash
set -e
# Locate filtered_requests in commands.rs
rg -n 'filtered_requests' src/commands.rs
# Find get_requests definition and its sorting logic
rg -nP 'fn\s+get_requests\b' -C5 .Length of output: 1067
🏁 Script executed:
#!/usr/bin/env bash
set -e
# Show the full get_requests implementation to check for sorting
rg -nP -C20 'fn\s+get_requests\b' src/client.rsLength of output: 1358
🏁 Script executed:
#!/usr/bin/env bash
set -e
# Display the full implementation of get_requests to check for sorting
sed -n '26,200p' src/client.rsLength of output: 1517
Reflow print_request_summary call & enforce correct ordering
- Reflow the
print_request_summaryinvocation into a multi-line call to satisfy rustfmt. - Ensure
filtered_requestsis sorted newest-first before applying.iter().rev()—either confirm the server returns requests in descending order or explicitly sort (e.g. by timestamp) client-side.
🤖 Prompt for AI Agents
In src/commands.rs around lines 88 to 91, reflow the print_request_summary call
into a multi-line invocation to satisfy rustfmt (each argument on its own line)
and, before the for loop, explicitly sort filtered_requests so it is
newest-first (e.g., sort_by_key or sort_by on the request timestamp in
descending order) so that using .iter().rev() still yields requests with the
latest appearing at the end; then keep the loop using .iter().rev() and the
reflowed print_request_summary call inside it.
There was a problem hiding this comment.
@coderabbitai yes, server returns records in descending order
There was a problem hiding this comment.
@SpeedSX, thank you for confirming! Since the server returns records in descending order (newest first), the .iter().rev() logic is correct and will display the latest requests at the end as intended. No explicit sorting is needed.
However, the rustfmt formatting issue still needs to be addressed. Please reflow the print_request_summary call to satisfy the formatter:
// Reverse the order so latest requests appear at the end
for request in filtered_requests.iter().rev() {
print_request_summary(
request,
!full_body, // Don't show body preview in full body mode
config.get_body_preview_length(),
);
if show_headers {This multi-line formatting will resolve the CI failure while preserving the correct ordering logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/config.rs (1)
70-83: Runcargo fmtto resolve formatting issues.The logic for joining URL segments is correct, but
cargo fmt --checkis failing on this block. Based on learnings from the past review comments.src/commands.rs (1)
88-94: Verify request ordering and fix formatting.The
.iter().rev()assumesfiltered_requestsis ordered newest-first from the API. If the server returns oldest-first, this would display them newest-last (intended), but if the server already returns newest-last, this would invert that to oldest-last.Additionally,
cargo fmt --checkis failing on this call. Based on learnings from the past review comments.Run the following script to verify the ordering returned by
get_requests:#!/bin/bash # Description: Check the implementation of get_requests to determine sort order # Find get_requests implementation and check for sorting logic rg -nP 'fn\s+get_requests\b' -A 30 src/client.rs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands.rs(8 hunks)src/config.rs(3 hunks)src/display.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands.rs (2)
src/display.rs (4)
print_full_request_body(50-83)print_request_details(85-159)print_request_headers(39-48)print_request_summary(10-37)src/config.rs (1)
join_url_segments(71-83)
🪛 GitHub Actions: Rust
src/commands.rs
[error] 158-164: cargo fmt --check failed due to code formatting issues. The diff shows reflowed chaining in the filter; please run 'cargo fmt' to format the code.
🔇 Additional comments (18)
src/config.rs (5)
18-20: LGTM!The
body_preview_lengthfield is correctly configured with a serde default, providing backward compatibility for existing config files.
22-26: LGTM!The default value of 80 characters is reasonable for terminal output and aligns with common CLI tool conventions.
53-53: LGTM!Consistent use of
default_body_preview_length()ensures the default config matches the serde default.
65-68: LGTM!The
normalize_base_urlhelper correctly removes trailing slashes, preventing double-slash issues in URL construction.
89-91: LGTM!Clean accessor pattern for exposing the body preview length to other modules.
src/display.rs (7)
1-1: LGTM!The
Localimport is required for the local time formatting informat_date.
19-36: LGTM with a minor note.The conditional preview logic correctly handles the two display modes. However, ensure the formatting satisfies
rustfmt.
200-200: LGTM!The
bright_bluecolor improves visual distinction for POST requests.
210-214: LGTM!Displaying both local and UTC times improves usability for users in different timezones. The format is clear and intuitive.
232-232: LGTM!The parameterized signature enables configurable preview length. Verify all call sites have been updated.
236-240: LGTM!The truncation logic correctly uses the
max_lengthparameter and appends an ellipsis when needed.
10-14: All call sites updated to use the new 3-argument signature.src/commands.rs (6)
16-16: LGTM!Using
Config::join_url_segmentsensures consistent URL construction throughout the codebase and avoids potential double-slash issues.
41-41: LGTM!Threading
configthroughmonitor_requestsenables access to configuration-driven display settings like body preview length.
70-70: LGTM!Using
eq_ignore_ascii_caseis more idiomatic and efficient than allocating withto_lowercase().
114-118: LGTM!The call correctly uses the updated
print_request_summarysignature with the new parameters.
141-141: LGTM!Threading
configthroughshow_logsmaintains consistency with themonitor_requestssignature change.
162-162: LGTM!Consistent use of
eq_ignore_ascii_caseimproves code quality.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands.rs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands.rs (2)
src/display.rs (4)
print_full_request_body(50-83)print_request_details(85-159)print_request_headers(39-48)print_request_summary(10-37)src/config.rs (1)
join_url_segments(71-83)
🔇 Additional comments (6)
src/commands.rs (6)
11-11: LGTM!The removal of the unused
print_request_bodyimport is appropriate.
16-16: LGTM!Using
Config::join_url_segmentscentralizes URL construction and provides consistent base URL normalization across the codebase.
39-49: LGTM!The
#[allow(clippy::too_many_arguments)]attribute appropriately addresses the clippy lint, and adding theconfigparameter enables the new body preview length configuration.
71-71: LGTM!Using
eq_ignore_ascii_casefor method filtering improves user experience by allowing case-insensitive method matching (e.g., "post", "POST", "Post").
89-119: LGTM!The multi-line formatting of
print_request_summarycalls now satisfies rustfmt requirements, and the logic correctly displays latest requests at the end given the server returns records in descending order (as confirmed).
142-164: LGTM!Adding the
configparameter and using case-insensitive method filtering are consistent improvements matching the changes inmonitor_requests.
| // Reverse the order so latest requests appear at the end | ||
| for request in filtered_requests.iter().rev() { | ||
| print_request_summary(request, !full_body, config.get_body_preview_length()); // Don't show body preview in full body mode |
There was a problem hiding this comment.
Format inconsistency: reflow to multi-line.
The print_request_summary call here uses single-line formatting, whereas lines 89-95 and 115-119 use multi-line formatting with each argument on a separate line. This inconsistency may fail rustfmt checks.
Apply this diff to match the formatting style used elsewhere in the file:
// Reverse the order so latest requests appear at the end
for request in filtered_requests.iter().rev() {
- print_request_summary(request, !full_body, config.get_body_preview_length()); // Don't show body preview in full body mode
+ print_request_summary(
+ request,
+ !full_body,
+ config.get_body_preview_length(),
+ ); // Don't show body preview in full body mode
if show_headers {📝 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.
| // Reverse the order so latest requests appear at the end | |
| for request in filtered_requests.iter().rev() { | |
| print_request_summary(request, !full_body, config.get_body_preview_length()); // Don't show body preview in full body mode | |
| // Reverse the order so latest requests appear at the end | |
| for request in filtered_requests.iter().rev() { | |
| print_request_summary( | |
| request, | |
| !full_body, | |
| config.get_body_preview_length(), | |
| ); // Don't show body preview in full body mode | |
| if show_headers { |
🤖 Prompt for AI Agents
In src/commands.rs around lines 187 to 189, the call to print_request_summary is
written on a single line which is inconsistent with the file's multi-line
argument formatting and may fail rustfmt; change the call to use the same
multi-line style used elsewhere by placing each argument on its own line (one
line for the function name and opening paren, one line per argument, and a
closing paren/semicolon on its own line) while preserving the boolean negation
and the config.get_body_preview_length() call and keeping the trailing comment.
No description provided.