Conversation
|
Warning Rate limit exceeded@twangodev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughThe PR refactors the SVG generator API to accept two new structs— Changes
Sequence Diagram(s)sequenceDiagram
participant Route as Route Handler
participant Generator as Generator::svg
rect rgb(248, 250, 252)
Note over Route,Generator: Former call (flattened params)
Route->>Generator: generate_svg(title, description, subtitle, logo, image, template_name, ...)
Generator->>Generator: use title/description/subtitle/logo/image directly
end
rect rgb(240, 255, 245)
Note over Route,Generator: New call (grouped wrappers)
Route->>Route: text = TextContent{title, description, subtitle}
Route->>Route: images = Images{logo, image}
Route->>Generator: generate_svg(text, images, template_name, ...)
Generator->>Generator: use text.title/text.description/text.subtitle/images.logo/images.image
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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.
Pull Request Overview
This PR refactors the codebase to use let-chains (a Rust 1.80+ feature) for cleaner nested conditional logic and introduces better API design through structured parameters in the SVG generation function. The changes include consolidating multiple string and image parameters into dedicated TextContent and Images structs, and replacing nested if let statements with more concise let-chain syntax throughout the codebase.
Key changes:
- Introduced
TextContentandImagesstructs to group related parameters in the SVG generator - Replaced nested
if letstatements with let-chains across multiple files for improved readability - Simplified error handling in the image resolver using
io::Error::other()
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/templates.rs | Adds Default implementation for TextWidthConstraints and refactors parse_width_constraints to use let-chains |
| src/routes/index.rs | Updates to use new TextContent and Images structs when calling generate_svg |
| src/params.rs | Refactors validation and color extraction logic to use let-chains |
| src/image/resolver.rs | Simplifies error mapping to use io::Error::other() |
| src/image/parse.rs | Refactors SSRF protection checks to use let-chains |
| src/image/fetch.rs | Refactors Content-Length validation to use let-chains |
| src/generator/svg.rs | Introduces TextContent and Images structs and updates generate_svg signature to use them |
| src/generator/mod.rs | Exports new TextContent and Images structs |
Comments suppressed due to low confidence (1)
src/templates.rs:33
- This creates infinite recursion. The struct derives
Defaulton line 9 (via#[derive(Default)]), but then manually implementsDefault::default()which callsnew(), andnew()callsSelf::default(). This will cause a stack overflow at runtime.
Either remove the manual Default implementation (since it's already derived), or remove Default from the derive attribute and implement it properly. If you want new() to use custom logic, have default() implement that logic directly and have new() call default().
impl Default for TextWidthConstraints {
fn default() -> Self {
Self::new()
}
}
impl TextWidthConstraints {
pub fn new() -> Self {
Self::default()
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (8)
src/generator/mod.rs(1 hunks)src/generator/svg.rs(4 hunks)src/image/fetch.rs(1 hunks)src/image/parse.rs(1 hunks)src/image/resolver.rs(2 hunks)src/params.rs(2 hunks)src/routes/index.rs(2 hunks)src/templates.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/image/resolver.rs (1)
src/image/mod.rs (1)
new(25-61)
src/generator/mod.rs (1)
src/generator/svg.rs (1)
generate_svg(122-184)
src/routes/index.rs (1)
src/generator/svg.rs (1)
generate_svg(122-184)
🪛 GitHub Check: Format Check
src/routes/index.rs
[warning] 1-1:
Diff in /home/runner/work/ogis/ogis/src/routes/index.rs
⏰ 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: Agent
- GitHub Check: Code Coverage
- GitHub Check: Code Coverage
🔇 Additional comments (14)
src/image/fetch.rs (1)
36-37: LGTM! Cleaner conditional with let-chains.The refactored conditional using let-chains (
&&) is more idiomatic and readable than nested if statements, while preserving the same logic.src/params.rs (2)
69-70: LGTM! Improved readability with let-chains.The combined conditional reduces nesting and makes the validation logic more concise.
89-90: LGTM! Cleaner guard condition.Combining the template color map existence check with the key presence check into a single conditional improves readability.
src/image/parse.rs (1)
41-43: LGTM! Excellent SSRF protection refactor.The let-chain syntax makes the multi-step IP validation more readable while maintaining the security logic.
src/templates.rs (1)
114-114: LGTM! Cleaner conditional with let-chains.The combined pattern matching reduces nesting and improves readability.
src/image/resolver.rs (2)
16-16: LGTM! Modern error handling.Using
io::Error::otheris the idiomatic way to convert errors in modern Rust (1.74+).
33-33: LGTM! Simplified error construction.The
io::Error::otherhelper is more concise than manually constructing errors withErrorKind::Other.src/generator/mod.rs (1)
9-9: LGTM! Public API expansion for wrapper structs.Exporting
ImagesandTextContentallows consumers to use the new grouped parameter API, which is cleaner than passing individual text and image parameters.src/routes/index.rs (2)
1-1: LGTM! Updated imports for new API.The import correctly includes the new
TextContentandImageswrapper structs.
166-175: LGTM! Cleaner API with grouped parameters.Grouping related text fields into
TextContentand images intoImagessignificantly improves the clarity of thegenerate_svgcall by reducing the parameter count and making the relationship between parameters explicit.src/generator/svg.rs (4)
15-24: LGTM! Well-designed wrapper structs.The new
TextContentandImagesstructs effectively group related parameters. Using lifetimes inTextContentavoids unnecessary string copies, whileImagesappropriately owns the image data.
122-129: LGTM! Improved function signature.Refactoring the signature from 9 individual parameters to use
TextContentandImagesstructs makes the function more maintainable and easier to use. The grouping makes the relationship between parameters explicit.
137-137: LGTM! Consistent usage of new structs.The code correctly accesses text fields through the
TextContentstruct.
152-159: LGTM! Proper handling of image wrappers.The image replacement logic correctly extracts data from the
Imagesstruct fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/rust.yml (1)
23-34: Optional: Consider adding Rust cache for faster CI runs.The clippy job could benefit from cargo dependency caching to reduce build time on subsequent runs, similar to how other projects structure their workflows.
You could optionally add caching by incorporating the
Swatinem/rust-cacheaction:- uses: actions-rust-lang/setup-rust-toolchain@v1 with: components: clippy + - uses: Swatinem/rust-cache@v2 - name: Run Clippy
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/rust.yml(2 hunks)
⏰ 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). (1)
- GitHub Check: Code Coverage
🔇 Additional comments (2)
.github/workflows/rust.yml (2)
23-34: Clippy job structure looks good.The job follows the established pattern from the
formatjob and is well-configured. The use of--all-targets --all-features -- -D warningsis appropriate for comprehensive linting and will catch issues across all targets and feature combinations.
72-72: Docker job dependency appropriately updated.Adding
clippyto the dependency list ensures clippy checks pass before Docker images are built, which aligns with the PR objective to integrate clippy into the CI pipeline.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores