Docs: rustdoc coverage; API changes for fragments and backoff#403
Docs: rustdoc coverage; API changes for fragments and backoff#403
Conversation
Provide richer API documentation and examples for public types, methods, and test helpers to improve clarity for users and contributors. Clarify semantics of error variants and important fields so callers understand expected values and failure conditions. Document queue priority semantics and backoff normalization (with examples) to prevent misconfiguration. Add descriptive comments to test worlds and harness state to make scenarios and assertions easier to follow.
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughSummarise the change: Added public PacketParts accessors, enriched fragmentation error variants with contextual fields, introduced BackoffConfig::normalised and invoked it in accept_loop, and expanded Rustdoc coverage across many modules and tests; also enabled deny for missing_docs in Cargo.toml. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (7)
🧰 Additional context used📓 Path-based instructions (4)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
docs/**/*.md📄 CodeRabbit inference engine (docs/contents.md)
Files:
docs/**/*.{rs,md}📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
Files:
🪛 LanguageToolCHANGELOG.md[typographical] ~53-~53: Two consecutive dots (DOUBLE_PUNCTUATION) 🔍 Remote MCP Deepwiki, RefSummary — additional repo facts relevant to reviewing PR #403 (concise):
Tools used:⏰ 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)
🔇 Additional comments (10)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/worlds/multi_packet.rs (1)
1-5: Module doc comment is split by the inner attribute.The
#![cfg(not(loom))]attribute at line 2 interrupts the module-level documentation. Other test world files in this PR (e.g.,tests/worlds/fragment/mod.rs,tests/worlds/stream_end.rs) place this attribute after the complete//!doc block.Move the attribute after the module documentation for consistency:
//! Test world for multi-packet channel scenarios. +//! +//! Provides [`MultiPacketWorld`] to verify message ordering, back-pressure +//! handling, and channel lifecycle in cucumber-based behaviour tests. #![cfg(not(loom))] -//! -//! Provides [`MultiPacketWorld`] to verify message ordering, back-pressure -//! handling, and channel lifecycle in cucumber-based behaviour tests.src/connection.rs (1)
1-6: File exceeds 400-line limit.This file is 992 lines, well above the 400-line threshold specified in your coding guidelines. Decompose this module into smaller, domain-focused submodules (e.g., separate
ActorState,MultiPacketContext, event handling) in a follow-up PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
Cargo.toml(1 hunks)src/app/envelope.rs(1 hunks)src/connection.rs(1 hunks)src/connection/test_support.rs(2 hunks)src/fragment/error.rs(2 hunks)src/push/queues/errors.rs(1 hunks)src/push/queues/mod.rs(1 hunks)src/server/runtime.rs(2 hunks)tests/advanced/concurrency_loom.rs(1 hunks)tests/common/mod.rs(1 hunks)tests/worlds/correlation.rs(1 hunks)tests/worlds/fragment/mod.rs(1 hunks)tests/worlds/multi_packet.rs(1 hunks)tests/worlds/panic.rs(1 hunks)tests/worlds/stream_end.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/fragment/error.rstests/worlds/multi_packet.rssrc/push/queues/errors.rstests/common/mod.rstests/worlds/fragment/mod.rstests/worlds/stream_end.rssrc/app/envelope.rstests/worlds/panic.rssrc/push/queues/mod.rssrc/connection.rstests/advanced/concurrency_loom.rstests/worlds/correlation.rssrc/connection/test_support.rssrc/server/runtime.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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 / -yse / -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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
src/fragment/error.rstests/worlds/multi_packet.rssrc/push/queues/errors.rstests/common/mod.rstests/worlds/fragment/mod.rstests/worlds/stream_end.rssrc/app/envelope.rstests/worlds/panic.rssrc/push/queues/mod.rssrc/connection.rstests/advanced/concurrency_loom.rstests/worlds/correlation.rssrc/connection/test_support.rssrc/server/runtime.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3"). Wildcard (*) or open-ended inequality (>=) version requirements are strictly forbidden. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🧬 Code graph analysis (1)
src/app/envelope.rs (5)
src/connection/test_support.rs (4)
id(24-24)id(36-36)correlation_id(26-26)correlation_id(38-38)tests/lifecycle.rs (2)
id(151-151)correlation_id(153-153)tests/routes.rs (2)
id(42-42)correlation_id(45-45)src/connection.rs (1)
correlation_id(236-241)src/correlation.rs (3)
correlation_id(11-11)correlation_id(18-18)correlation_id(24-24)
🔍 Remote MCP Deepwiki, Ref
Now I have sufficient context. Let me compile the findings:
Additional Context for PR #403 Review
Repository Context
The wireframe codebase is progressing toward version 1.0, which introduces significant public API expansions and breaking changes, including new traits (WireframeProtocol), enhanced Response enum variants (Vec, Stream, Empty), new PushHandle APIs for asynchronous server pushes, and generic WireframeError<E> for error handling.
BackoffConfig Changes
The PR adds initial_delay and max_delay fields to BackoffConfig and introduces the normalised() method. The normalised() method ensures critical invariants: it clamps both delays to a minimum of 1 millisecond and swaps the values if initial_delay exceeds max_delay. This configuration is used by the WireframeServer's accept_loop to implement exponential backoff during connection acceptance failures.
PacketParts Public API Changes
The PR adds three new public accessor methods to PacketParts:
pub const fn id(&self) -> u32pub const fn correlation_id(&self) -> Option<u64>pub fn payload(self) -> Vec<u8>(consuming function)
These complement existing struct fields and align with the library's recent introduction of correlation_id functionality for tying packets to requests in multi-packet streaming responses.
Documentation Lint Configuration
The missing_docs = "deny" lint configuration added to Cargo.toml enforces that all public items must have documentation. By default, the missing_docs lint is allowed in Rust; setting it to "deny" treats missing documentation as a hard error rather than a warning, supporting the PR's stated goal of preparing for stricter documentation requirements.
Breaking Changes Context
While the PR's stated objective focuses on documentation standardization, it's part of a broader trajectory toward version 1.0 that includes public API breaking changes. The error variant changes (adding fields to FragmentError::MessageMismatch, FragmentError::IndexMismatch, and FragmentationError::IndexOverflow) represent changes to public enum structures that may affect exhaustive pattern matching in downstream code.
⏰ 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 (19)
tests/worlds/panic.rs (1)
78-83: LGTM!The added doc comment accurately describes the test world's purpose and aligns with the PR's documentation standardization goals.
Cargo.toml (1)
118-122: LGTM!Adding
missing_docs = "deny"enforces documentation coverage for all public items, aligning with the coding guidelines requiring Rustdoc comments for public APIs. This is a sensible change ahead of stricter documentation requirements.src/push/queues/mod.rs (1)
50-57: LGTM!The doc comments for
PushPriorityvariants are concise and clearly describe the semantic difference between high and low priority queues.tests/worlds/correlation.rs (2)
19-24: LGTM!The doc comment for
CorrelationWorldclearly describes the test world's purpose.
26-34: LGTM!The doc comment and example for
set_expectedare clear and useful.src/connection/test_support.rs (2)
69-70: LGTM!The field-level doc comment clearly describes the purpose of the
outfield for test assertions.
177-186: LGTM!The doc comments on
ActorStateSnapshotfields provide clear, concise descriptions of each lifecycle flag and counter. This aligns well with the PR's documentation standardisation goals.tests/worlds/fragment/mod.rs (1)
32-33: LGTM!The doc comment succinctly describes the struct's role in behavioural testing. Good use of en-GB spelling.
tests/worlds/stream_end.rs (1)
24-25: LGTM!Clear and concise doc comment describing the struct's testing purpose.
tests/worlds/multi_packet.rs (1)
26-27: LGTM!The struct doc comment is well-placed and uses correct en-GB spelling ("behaviours").
src/push/queues/errors.rs (1)
28-33: LGTM!Field-level doc comments clearly describe what each capacity value represents. This aligns with the PR's documentation standardisation effort and supports the
missing_docs = "deny"lint configuration.src/connection.rs (1)
98-103: Documentation additions are appropriate and concise.The field-level doc comments accurately describe the purpose of
queuesandhandlewithinConnectionChannels<F>. This aligns with the PR's goal of standardizing rustdoc style.src/server/runtime.rs (1)
76-106: Well-designed normalisation method.The
normalised()method correctly handles edge cases (zero durations, inverted values) and uses British spelling per coding guidelines. The doctest clearly demonstrates the swap behaviour.src/app/envelope.rs (3)
125-136: Accessor method is well-formed.The
id()accessor is correctly markedconst fnand#[must_use]. The doctest is clear and demonstrates the expected behaviour.
138-149: Accessor aligns withCorrelatableFrametrait pattern.The
correlation_id()method mirrors the trait method signature seen insrc/correlation.rs(line 10). Usingconst fnis appropriate for simple field access.
151-162: Consuming accessor transfers ownership correctly.The
payload(self)method intentionally consumesPacketPartsto transfer ownership of theVec<u8>. This is used inFrom<PacketParts> for Envelopeat line 210, confirming the design is intentional.tests/common/mod.rs (1)
22-25: Type alias documentation is clear and appropriate.The doc comments succinctly describe the purpose of
TestAppandTestResultfor test scaffolding. This aligns with the PR's documentation standardisation goals.src/fragment/error.rs (2)
27-51: Field documentation is well-structured.The doc comments for
expected,found, andlastfields clearly describe their semantic meaning, aiding debugging and error handling.
46-50: The review comment is incorrect.IndexOverflowis already defined as a struct variant in bothFragmentErrorandFragmentationError, and all existing code already uses compatible pattern matching syntax. All references toIndexOverflowin the codebase usematches!(err, FragmentError::IndexOverflow { .. })or construct it asFragmentError::IndexOverflow { last: ... }. There are no exhaustive pattern matches likeFragmentError::IndexOverflow => ...that would fail to compile. If this variant structure changed, it was already completed and all downstream code has been updated.Likely an incorrect or invalid review comment.
- Converted block comments to doc comments in concurrency_loom.rs for better documentation. - Added #[must_use] attribute to the expected() method in correlation.rs to indicate return value should not be ignored. - Normalized backoff variable usage in runtime.rs for clarity without changing behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…ent index Changed `FragmentError::IndexOverflow` and `FragmentationError::IndexOverflow` from unit variants to struct variants carrying a `last: FragmentIndex` field. This records the final valid index before an overflow, providing richer diagnostic information. BREAKING CHANGE: Pattern matches against `IndexOverflow` must be updated to destructure or wildcard the new `last` field. See migration guide in CHANGELOG for details. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai please create a new GitHub issue for the following: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 46 @leynos: I've created GitHub issue #404 for refactoring The issue has been assigned to you and includes the context from this PR review. |
Moved the #![cfg(not(loom))] attribute below the module-level documentation in tests/worlds/multi_packet.rs to ensure it properly excludes the test when running with loom. This maintains correct test configuration without changing functionality. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Address code review feedback from PR #403: - Make ActorState fields private, exposing closed_sources() and total_sources() accessor methods to preserve lifecycle invariants - Make RunState private to state.rs module since it's only consumed within ActorState - Make MultiPacketStamp private to multi_packet.rs, adding is_stamping_enabled() method to avoid leaking implementation details - Extract helper functions into domain-focused submodules to bring mod.rs below the 400-line cap: - drain.rs: Queue drain operations and fairness-aware helpers - frame.rs: Frame processing and correlation stamping - response.rs: Streaming response handling - shutdown.rs: Shutdown and multi-packet closure handling - polling.rs: Async polling utilities - dispatch.rs: Event dispatching Module layout after refactoring: - mod.rs: 399 lines (core actor struct and public API) - drain.rs: 189 lines (queue processing) - frame.rs: 88 lines (frame emission) - response.rs: 77 lines (streaming response) - shutdown.rs: 83 lines (shutdown handling) - polling.rs: 70 lines (async helpers) - dispatch.rs: 34 lines (event dispatch) All existing tests pass and the public API is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* Decompose connection module into domain-focused submodules Extract ActorState, MultiPacketContext, and Event types into separate submodules to improve code organisation and maintainability. The module structure now follows the established pattern used by app/, fragment/, and push/ modules. Extracted components: - state.rs: RunState enum and ActorState lifecycle management - multi_packet.rs: MultiPacketStamp, MultiPacketTerminationReason, and MultiPacketContext for correlation stamping and channel handling - event.rs: Event enum for the actor select loop The ConnectionActor implementation remains in mod.rs as the core public API. All existing tests pass and the public API is unchanged. closes #404 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Tighten encapsulation and reduce module size per code review Address code review feedback from PR #403: - Make ActorState fields private, exposing closed_sources() and total_sources() accessor methods to preserve lifecycle invariants - Make RunState private to state.rs module since it's only consumed within ActorState - Make MultiPacketStamp private to multi_packet.rs, adding is_stamping_enabled() method to avoid leaking implementation details - Extract helper functions into domain-focused submodules to bring mod.rs below the 400-line cap: - drain.rs: Queue drain operations and fairness-aware helpers - frame.rs: Frame processing and correlation stamping - response.rs: Streaming response handling - shutdown.rs: Shutdown and multi-packet closure handling - polling.rs: Async polling utilities - dispatch.rs: Event dispatching Module layout after refactoring: - mod.rs: 399 lines (core actor struct and public API) - drain.rs: 189 lines (queue processing) - frame.rs: 88 lines (frame emission) - response.rs: 77 lines (streaming response) - shutdown.rs: 83 lines (shutdown handling) - polling.rs: 70 lines (async helpers) - dispatch.rs: 34 lines (event dispatch) All existing tests pass and the public API is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * refactor(connection): replace manual multi_packet install with set_multi_packet_with_correlation Replaced the direct call to `self.multi_packet.install(channel, None)` and debug assertion with a call to the helper method `set_multi_packet_with_correlation(channel, None)`. This centralizes correlation handling and reduces code duplication in the connection module. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com> * refactor(connection): clean up shutdown handling and clarify comments - Replaced 'await_shutdown' with 'wait_shutdown' for shutdown cancellation handling to simplify code. - Added Clippy exemption for unreachable code in 'try_opportunistic_drain' to maintain exhaustive matching. - Improved documentation comments regarding multi-packet channel stamping behavior. - Removed unused 'await_shutdown' function to reduce dead code. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com> * refactor(connection): replace unreachable!() with debug_assert!() in drain.rs Changed try_opportunistic_drain for QueueKind::High from unreachable!() call to a debug_assert!(false) and returning false. Removed the #[expect(clippy::unreachable)] attribute. This improves clarity and debugging while preserving behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Summary
FragmentError::IndexOverflowandFragmentationError::IndexOverflownow carry alast: FragmentIndexfield; tests updated accordinglyWhy
Task
https://www.terragonlabs.com/task/bab289c9-a99e-4663-aac8-ae73906a449b