Conversation
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 51 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 (1)
Summary by CodeRabbit
WalkthroughSummarise: Replace many owned String and large error payloads with boxed variants (Box, Box), add a BoxedStr utility trait, update error construction sites to use .boxed()/.into(), and adjust one function to return boxed OrthoError. Control flow remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Module
participant VkError
Caller->>Module: Call function returning Result<T, VkError>
alt Ok
Module-->>Caller: Ok(T)
else Err
Module->>VkError: Construct variant with boxed payload (Box<str>/Box<Error>)
VkError-->>Caller: Err(boxed VkError)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Possibly related issues
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 (
|
Reviewer's GuideThe PR refactors VkError and related modules to reduce enum size by boxing large error and string payloads, removes obsolete clippy result_large_err suppressions, and updates the configuration loader to return boxed errors. Class diagram for updated config loader error typeclassDiagram
class OrthoError {
+Gathering(FigmentError)
...
}
class FigmentError
OrthoError <.. FigmentError: Gathering variant
class ConfigLoader {
+load_with_reference_fallback(cli_args: T): Result<T, Box<OrthoError>>
}
ConfigLoader --> OrthoError: returns boxed error
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a memory optimization strategy for the VkError enum by boxing large variants to reduce the overall size of Result<T, VkError> types throughout the codebase. The changes address Clippy's result_large_err lint warnings that occur when enum variants contain large types, causing all Result instances to allocate memory for the largest variant.
The key changes include:
-
Error Enum Optimization: Modified the
VkErrorenum insrc/main.rsto box external error types (reqwest::Error,std::io::Error,ortho_config::OrthoError) and convertStringfields toBox<str>. CustomFromimplementations maintain ergonomic error conversion while wrapping sources inBox. -
Configuration Module Update: Updated
src/config.rsto returnResult<T, Box<OrthoError>>instead of bareOrthoError, ensuring consistency with the boxed error handling pattern used throughout the application. -
API Module Refactoring: Systematically updated
src/api/mod.rsto useBox<str>when constructingVkErrorvariants, utilizing.into_boxed_str()for efficient string-to-boxed-str conversion and introducing intermediate variables to prevent repeated cloning. -
Parser and Review Thread Updates: Modified error construction sites in
src/ref_parser.rsandsrc/review_threads.rsto use the new boxed string types, changing from.to_owned()to.into()for better conversion flexibility. -
Lint Suppression Cleanup: Removed
#[allow(clippy::result_large_err)]suppressions from multiple files now that the underlying memory efficiency issue has been resolved.
This optimization reduces stack allocation sizes for Result types, improves cache performance, and eliminates the need for lint suppressions while maintaining full API compatibility. The Box<str> type is particularly well-suited for error messages since they are typically created once and not modified, making this a memory-efficient choice over String.
Important Files Changed
Click to expand
| Filename | Score | Overview |
|---|---|---|
| src/main.rs | 5/5 | Modified VkError enum to box large variants and added custom From implementations |
| src/api/mod.rs | 5/5 | Updated error construction to use Box and boxed external errors throughout |
| src/config.rs | 5/5 | Changed return type to Box and removed clippy suppression |
| src/ref_parser.rs | 5/5 | Removed clippy suppressions and updated string conversion from .to_owned() to .into() |
| src/review_threads.rs | 5/5 | Updated error construction to use boxed strings with .into_boxed_str() |
Confidence score: 5/5
- This PR is safe to merge with minimal risk as it implements a well-established Rust optimization pattern
- Score reflects systematic application of boxing pattern across all relevant files with proper From implementations maintaining API compatibility
- No files require special attention as all changes follow consistent patterns and maintain existing error handling semantics
5 files reviewed, no comments
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit a49f130), not the entire PR.
This PR implements a memory optimization for the VkError enum by boxing large variants to reduce overall enum size and eliminate clippy::result_large_err warnings throughout the codebase. The changes introduce several key components:
-
New
BoxedStrtrait (src/boxed.rs): A convenience trait that provides a unified.boxed()method for converting string-like types (String,&str,Cow<'_, str>) toBox<str>. This eliminates verbose.into_boxed_str()calls throughout the codebase. -
VkError enum optimization (
src/main.rs): The core error enum has been restructured to box large variants including external error types (reqwest::Error,std::io::Error,ortho_config::OrthoError) and string payloads (converted fromStringtoBox<str>). A macro-based approach usingboxed_error_from!provides cleanFromimplementations. -
Configuration loader updates (
src/config.rs): The configuration loader now returnsResult<T, Box<OrthoError>>instead of the unboxed version, maintaining consistency with the new boxed error pattern. -
API and parser integration (
src/api/mod.rs,src/review_threads.rs): Error construction throughout these modules has been updated to use the new.boxed()method and leverage theFromtrait implementations.
The optimization reduces the stack allocation size for Result<T, VkError> instances across the application, improving memory efficiency and cache performance. All previously required #[allow(clippy::result_large_err)] suppressions have been successfully removed. The changes maintain identical error semantics while providing a cleaner, more maintainable error handling approach.
Important Files Changed
Files Changed
| Filename | Score | Overview |
|---|---|---|
| src/boxed.rs | 5/5 | Introduces new BoxedStr trait for ergonomic string-to-Box<str> conversions |
| src/main.rs | 5/5 | Core VkError enum optimization with boxed variants and macro-based From implementations |
| src/api/mod.rs | 4/5 | Updates error construction to use new .boxed() method and From trait implementations |
| src/config.rs | 4/5 | Modifies configuration loader to return Box<OrthoError> for consistency |
| src/review_threads.rs | 5/5 | Simple update to use .boxed() instead of .into_boxed_str() for consistency |
Confidence score: 5/5
- This PR is safe to merge with minimal risk as it focuses on memory optimization without changing error semantics
- Score reflects well-structured refactoring with comprehensive documentation and consistent application across the codebase
- No files require special attention as all changes follow the same optimization pattern systematically
5 files reviewed, no comments
![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
📒 Files selected for processing (6)
src/api/mod.rs(8 hunks)src/boxed.rs(1 hunks)src/config.rs(2 hunks)src/main.rs(2 hunks)src/ref_parser.rs(1 hunks)src/review_threads.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/review_threads.rssrc/api/mod.rssrc/config.rssrc/boxed.rssrc/ref_parser.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.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.
Files:
src/review_threads.rssrc/api/mod.rssrc/config.rssrc/boxed.rssrc/ref_parser.rssrc/main.rs
🧬 Code Graph Analysis (3)
src/review_threads.rs (1)
src/boxed.rs (4)
boxed(12-12)boxed(16-18)boxed(22-24)boxed(28-33)
src/api/mod.rs (1)
src/boxed.rs (4)
boxed(12-12)boxed(16-18)boxed(22-24)boxed(28-33)
src/main.rs (2)
src/boxed.rs (4)
boxed(12-12)boxed(16-18)boxed(22-24)boxed(28-33)src/api/mod.rs (1)
new(91-98)
🔍 MCP Research (2 servers)
Deepwiki:
-
src/main.rs: A macro
boxed_error_from!was introduced to implementFromconversions for boxed error variants ofVkError, replacing the previous direct#[from]attribute usage with implementations that box the source error. This macro is applied toreqwest::Error,std::io::Error, andortho_config::OrthoError. The#[expect(clippy::result_large_err)]attributes on functions were removed as the error enum is now semantically small due to boxing. A new moduleboxedwas added to the crate root. (src/main.rs) -
src/boxed.rs: A new Rust module
src/boxed.rswas introduced providing a traitBoxedStrwith methodboxed()that converts various string-like types (String,&str,Cow<'_, str>) intoBox<str>. This abstraction simplifies boxing strings, particularly useful for keeping error enums concise without verbose calls. (src/boxed.rs) -
src/api/mod.rs: Error messages in
VkErrorvariantsApiErrors,BadResponseSerde, andBadResponseare now boxed using theboxed()method fromBoxedStr. The context string inVkError::RequestContextis cloned once, boxed, and reused to avoid repeated cloning. The error source inRequestContextis converted into a boxed error type via.into(). In tests, error strings passed toApiErrorsare changed from.to_string()to.into(), aligning with boxed string usage. (src/api/mod.rs) -
src/review_threads.rs: In the
fetch_comment_pageasync function, when thenodefield is missing in the response, the error message string passed toVkError::BadResponseis now wrapped with.boxed()before passing, introducing boxing of the error message string. No other logic or control flow changes. (src/review_threads.rs)
Context7:
-
The
VkErrorenum insrc/main.rswas refactored to box all string and external error payloads, replacing ownedStringand direct error types withBox<str>andBox<ErrorType>. A macroboxed_error_from!was introduced to implementFromtrait conversions for boxed error variants, applied toreqwest::Error,std::io::Error, andortho_config::OrthoError. This enabled removal of#[expect(clippy::result_large_err)]attributes. (src/main.rs) -
The new module
src/boxed.rsadds theBoxedStrtrait with methodboxed()to convertString,&str, andCow<'_, str>intoBox<str>, simplifying boxing string payloads in error construction. (src/boxed.rs) -
In
src/api/mod.rs, error message strings inVkError::ApiErrors,VkError::BadResponseSerde, andVkError::BadResponseare converted to boxed strings using.boxed(). TheRequestContextvariant clones and boxes its context string once and boxes its error source with.into(). (src/api/mod.rs) -
The function
load_with_reference_fallbackinsrc/config.rswas changed to returnResult<T, Box<OrthoError>>instead ofResult<T, OrthoError>, boxing error returns with.into(). (src/config.rs)
🔇 Additional comments (14)
src/boxed.rs (1)
1-6: Document the module succinctly and keep it.Module-level docs meet the guideline and clearly state purpose. Retain this.
src/review_threads.rs (1)
100-107: Box the formatted error message correctly.Use of
.boxed()here aligns the error payload with the refactoredVkErrorvariant. Good change.src/api/mod.rs (6)
50-50: Box GraphQL errors succinctly.Create
VkError::ApiErrors(msg.boxed())as done here. This aligns with the boxed error strategy.
149-156: Keep cloning ofctx_boxlocal to the error path.Cloning the boxed context only occurs when an error is produced, which is acceptable.
176-176: Box the serde error with a body snippet.The
.boxed()call on the formatted message matches the refactor and keepsVkErrorsmall.
185-186: Box the “missing data” error message.The
.boxed()call is correct and consistent.
196-198: Box the path-aware deserialisation error.Formatting and boxing the message is spot-on.
281-282: Use.into()for boxed string literal in tests.
"boom".into()resolves toBox<str>for theApiErrorsvariant, matching the new type.src/ref_parser.rs (1)
53-55: Convert the segment to a boxed string via.into().This matches the boxed payload type of
VkError::WrongResourceType::found.src/main.rs (5)
72-79: LGTM: Document the error type and the boxing rationale.The enum-level rustdoc is clear and aligned with the architectural intent to minimise enum size.
83-89: Retain #[from] on Box and provide From via the macro; this is the right split.
- #[from] on
Box<reqwest::Error>enablesFrom<Box<reqwest::Error>>without extra boilerplate.- The macro supplies
From<reqwest::Error>, so call-sites can use?with owned errors directly.
104-107: LGTM: Box external error sources.Boxing
std::io::Errorandortho_config::OrthoErrorkeepsVkErrorsmall and removes the need forresult_large_errallowances.
93-103: Approve removal of.to_string()calls inVkErrorvariantsNo occurrences of
.to_string()remain when constructingVkError::BadResponse,VkError::BadResponseSerde,VkError::ApiErrors, orWrongResourceType { found }. IntermediateStringallocations have been eliminated.
119-122: Merge with confidenceNo
clippy::result_large_error#[allow(...)]references remain repository-wide. Cleanup verified under warnings-denied configuration.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/api/mod.rs (2)
140-155: Avoid allocating context on the success path and drop clones; prefer expect over unwrap_or_defaultBuild the context lazily inside error handlers to avoid the cost on the happy path and eliminate deep clones of Box. Also use
.expect(...)per guidelines.- let ctx_box = serde_json::to_string(&payload).unwrap_or_default().boxed(); let response = self .client .post(&self.endpoint) .headers(self.headers.clone()) .json(&payload) .send() .await - .map_err(|e| VkError::RequestContext { - context: ctx_box.clone(), - source: e.into(), - })?; + .map_err(|e| VkError::RequestContext { + context: serde_json::to_string(&payload) + .expect("serialising GraphQL request payload") + .boxed(), + source: e.into(), + })?; let body = response.text().await.map_err(|e| VkError::RequestContext { - context: ctx_box.clone(), + context: serde_json::to_string(&payload) + .expect("serialising GraphQL request payload") + .boxed(), source: e.into(), })?;
186-198: Prefer expect for pretty-print serialisation in error pathUse
.expect(...)with serde_json::to_string_pretty since serialising a Value cannot fail here; dropunwrap_or_default.- let snippet = snippet( - &serde_json::to_string_pretty(&value).unwrap_or_default(), - VALUE_SNIPPET_LEN, - ); + let snippet = snippet( + &serde_json::to_string_pretty(&value) + .expect("serialising JSON snippet for error"), + VALUE_SNIPPET_LEN, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/api/mod.rs(8 hunks)src/boxed.rs(1 hunks)src/main.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/boxed.rssrc/main.rssrc/api/mod.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.
Files:
src/boxed.rssrc/main.rssrc/api/mod.rs
🧬 Code Graph Analysis (2)
src/main.rs (2)
src/boxed.rs (5)
boxed(11-11)boxed(15-17)boxed(21-23)boxed(27-32)boxed(36-38)src/api/mod.rs (1)
new(91-98)
src/api/mod.rs (1)
src/boxed.rs (5)
boxed(11-11)boxed(15-17)boxed(21-23)boxed(27-32)boxed(36-38)
🔍 MCP Research (1 server)
Deepwiki:
-
"VkError is the central error type defined in src/main.rs and is returned by multiple functions across the codebase, including GraphQLClient::run_query, paginate, fetch_comment_page, fetch_issue, and fetch_thread_page (these functions return Result<..., VkError>)." (https://deepwiki.com/search/box-large-vkerror-variants-pr_01c5162b-edac-443d-96d7-91dd31284e68)
-
"Several functions previously had #[allow(clippy::result_large_err, reason = "...")] attributes — examples called out include run_pr, run_issue, main, and parse_reference." (https://deepwiki.com/search/box-large-vkerror-variants-pr_01c5162b-edac-443d-96d7-91dd31284e68)
🔇 Additional comments (12)
src/boxed.rs (2)
1-5: Module-level docs comply with guidelinesKeep module docs at the top for all modules. This matches the repository standards.
68-74: Identity test correctly verifies zero-allocation pass-throughThe pointer identity assertion proves no re-boxing occurs for Box.
src/api/mod.rs (5)
16-16: Import the BoxedStr helperWire in the trait to enable
.boxed()without boilerplate. Good.
45-51: Box aggregated API error messagesConstruct
VkError::ApiErrorswith a boxed payload to keep the enum lean. Good.
173-176: Box serde error messageBox the formatted serde error to keep
VkErrorcompact. Correct.
183-185: Box missing-data error messageBox the message for
BadResponse. Correct and consistent with the new enum layout.
280-281: Update tests to construct boxed string errorUse
"boom".into()to feedBox<str>payload. Correct.src/main.rs (5)
14-14: Declare the boxed module in the crate rootMake the helper available crate-wide without exporting it publicly. Correct.
72-107: Box all large payloads in VkError to reduce enum sizeBox
reqwest::Error,std::io::Error,ortho_config::OrthoError, and string payloads. This meets the objective to removeresult_large_errsuppressions.
109-118: Document and use the macro to implement From by boxingDocument the intent and generate concise
Fromimpls for unboxed error sources. Good for maintainability.
120-123: Apply macro to all external error sourcesRegister
Fromconversions forreqwest::Error,std::io::Error,ortho_config::OrthoError. Good coverage.
1-383: Clean-up audit passed
- No
#[allow(clippy::result_large_err)]attributes remain.- No
to_string()invocations inVkErrorconstructors.- No raw string literals used in
VkErrorvariants.- All dynamic payloads in
VkErrorare boxed (Box<…>) or static references.No further clean-up required.
|
@coderabbitai Please create new GitHub issues for the following: 186-198: Prefer expect for pretty-print serialisation in error path Use .expect(...) with serde_json::to_string_pretty since serialising a Value cannot fail here; drop unwrap_or_default. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create GitHub issues for these optimization suggestions. 🏁 Script executed: Length of output: 3213 @leynos I've successfully created two GitHub issues for your optimization suggestions:
Both issues include detailed descriptions, the current problems, proposed solutions, benefits, and references back to this PR. They've been assigned to you for follow-up implementation. 🐰
|
Add #[inline] to BoxedStr implementations to reduce call overhead and consolidate tests using rstest to remove duplication.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/boxed.rs (1)
45-49: Declare rstest as a dev-dependency in Cargo.toml
Add the following under[dev-dependencies]in your Cargo.toml to enable the rstest tests:[dev-dependencies] rstest = "<latest compatible version>"Replace
<latest compatible version>with the current rstest release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/boxed.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/boxed.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.
Files:
src/boxed.rs
🔍 Additional research (Context7)
Summary of additional repo context relevant to reviewing this PR:
- Repo docs/wiki confirm project architecture, error handling, and VkError centrality — boxing large VkError variants aligns with intent to reduce Result<T, VkError> size and improve performance (§Error Handling Architecture).
- The new boxed.rs utility (BoxedStr trait) fits existing patterns: many modules construct error messages and request/response snippets before rendering; switching to Box reduces enum footprint where Strings were previously stored (multiple places in main.rs, api client, parsers).
- Config change (load_with_reference_fallback now returns Result<T, Box>) is consistent with project-wide use of ortho_config and the configuration merging flow; ortho_config errors are already propagated in main flow and GraphQL client initialization.
- Tests and CI expectations: repo enforces strict formatting/linting/coverage (make fmt, make lint, make test; clippy warns treated as errors). Any public API/type changes (e.g., changing VkError variant payload types) can be breaking for external users and must pass CI; review should verify test updates and ensure no public API breakage without versioning notes.
- Related reviewer suggestions already filed as issues (#67, #68) proposing lazy ctx allocation and replacing unwrap_or_default with expect in error paths — these are relevant follow-ups rather than blockers for this PR.
Tools used:,
🔇 Additional comments (3)
src/boxed.rs (3)
1-5: Keep the succinct module docs; LGTM.The module-level docs are clear and meet the repo’s documentation requirements.
14-43: Inline trivial adapters; LGTM.The #[inline] annotations on the tiny adapters are appropriate, and the identity impl for Box avoids needless re-boxing.
60-66: Approve pointer-identity test.The identity test correctly verifies no reallocation occurs when boxing an existing Box.
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos Let me check the current implementation to see which of these concerns have been resolved. Looking at the codebase: ✅ RESOLVED - String boxing utility: The ✅ RESOLVED - Macro for From implementations: The macro_rules! boxed_error_from {
($source:ty, $variant:ident) => {
impl From<$source> for VkError {
fn from(value: $source) -> Self {
VkError::$variant(Box::new(value))
}
}
};
}
The first two concerns have been well addressed with clean, reusable abstractions. The third point about performance trade-offs remains a design decision that could benefit from benchmarking if performance becomes a concern in practice. 🐰
🏁 Script executed: Length of output: 5269 @leynos Based on my examination of the current codebase, here's the status of those three concerns: ✅ FULLY RESOLVED: Macro for DRY error boxingThe macro_rules! boxed_error_from {
($source:ty, $variant:ident) => {
impl From<$source> for VkError {
fn from(source: $source) -> Self {
Self::$variant(Box::new(source))
}
}
};
}Used consistently for: ✅ FULLY RESOLVED: BoxedStr utility traitThe
The
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create GitHub issues for these optimization suggestions. 🏁 Script executed: Length of output: 2419 @leynos Looking at the current PR changes, here's the status of those optimization suggestions: 🏁 Script executed: Length of output: 3400 Yes, these optimization suggestions have been well-addressed in this PR: ✅ Extract macro for DRY Box::new() calls - RESOLVEDThe macro_rules! boxed_error_from {
($source:ty, $variant:ident) => {
impl From<$source> for VkError {
fn from(source: $source) -> Self {
Self::$variant(Box::new(source))
}
}
};
}Used for all external error types: ✅ Utility for Box conversion - RESOLVEDThe pub trait BoxedStr {
fn boxed(self) -> Box<str>;
}Comprehensive implementations for
|
1 participant
Summary
VkErrorvariants and removeresult_large_errsuppressionsBox<OrthoError>from config loader to avoid large error resultsStringpayloads when constructingVkErrorin API and parser helpersTesting
make fmtmake lintmake testcloses #58
https://chatgpt.com/codex/tasks/task_e_689797186e688322851b9c66dc429d37
Summary by Sourcery
Box large VkError variants and streamline error handling by boxing external errors and string payloads across the codebase, removing obsolete Clippy suppressions, and updating the configuration loader to return boxed errors.
Enhancements: