Type preamble handler test cases and stabilise runtime readiness#301
Type preamble handler test cases and stabilise runtime readiness#301
Conversation
Reviewer's GuideRefactors preamble handler tests to use a typed enum for success/failure scenarios, and stabilises runtime readiness signal logic by avoiding let-chains and centralising BackoffConfig initialization. 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. Summary by CodeRabbit
WalkthroughReplace the two-argument accept_backoff API with a single exported BackoffConfig; thread BackoffConfig through WireframeServer state, builders, binding and preamble transitions; pass backoff_config into accept_loop and normalise it; update tests to use BackoffConfig and a PreambleHandlerKind enum exercising both success and failure paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Runtime as Server Runtime
participant Wireframe as WireframeServer
participant Accept as accept_loop
Runtime->>Wireframe: read backoff_config
Runtime->>Accept: spawn worker with backoff_config
Note right of Accept: accept_loop uses provided BackoffConfig per worker
sequenceDiagram
participant Test as Preamble Test
participant Listener as TcpListener
participant Handler as Preamble Handler
Test->>Listener: bind & connect (success case)
Test->>Handler: call on_preamble_decode_success
Handler-->>Test: increment counter
alt Failure case
Test->>Handler: call on_preamble_decode_failure(DecodeError)
Handler-->>Test: increment counter
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 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 (
|
![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.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
|
@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 introduces two key improvements to enhance code quality and type safety in the Wireframe server runtime. The changes replace string-based test parameters with a typed enum approach and refactor runtime initialization patterns for better performance and clarity.
The most significant change is in the test suite where string-based preamble handler identification ("success", "failure") has been replaced with a strongly-typed HandlerKind enum. This eliminates the need for runtime string matching and the associated panic!("Invalid handler type") fallback, moving error detection from runtime to compile-time. The enum approach integrates seamlessly with the existing rstest parameterized testing framework while providing better type safety.
In the server runtime module, two complementary improvements were made: the readiness signaling logic was refactored from a let-chain to nested if-statements (with a clippy expectation to maintain the desired structure), and BackoffConfig initialization was moved outside the retry loop to avoid repeated default() calls. These changes improve code clarity and eliminate minor performance overhead without altering any functional behavior.
These modifications align well with the codebase's emphasis on type safety and performance optimization, particularly given Wireframe's focus on high-performance asynchronous binary protocol handling.
Important Files Changed
Click to expand file changes
| Filename | Score | Overview |
|---|---|---|
| src/server/config/tests.rs | 5/5 | Replaced string-based test parameters with typed HandlerKind enum for better type safety |
| src/server/runtime.rs | 5/5 | Refactored readiness signaling and pre-initialized BackoffConfig for improved performance |
Confidence score: 5/5
- This PR is safe to merge with minimal risk as it only improves type safety and code clarity
- Score reflects straightforward refactoring with no functional behavior changes and clear performance benefits
- No files require special attention as both changes are well-contained improvements
2 files reviewed, no comments
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/server/config/tests.rs (1)
125-129: Assert handler invocation to close the loop on behaviour.Add assertions that the counter increments after invoking the registered handler to validate execution, not just registration.
I can wire a minimal invocation path or propose a helper in test_util to trigger success/failure preamble flows; say the word and I’ll draft it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/server/config/tests.rs(2 hunks)src/server/runtime.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/server/runtime.rssrc/server/config/tests.rs
🔇 Additional comments (1)
src/server/config/tests.rs (1)
112-123: LGTM: Replace string matching with a typed enum.Strengthen type-safety and remove unreachable arms. Matching on HandlerKind is clear and robust.
There was a problem hiding this comment.
Greptile Summary
This review covers only changes made since commit 6fcc9e2, not the entire PR.
The recent changes implement the improvements suggested in previous reviews while maintaining the core functionality. The key modifications include:
-
Backoff Configuration Persistence: The
backoff_configfield is now properly included in allWireframeServerstruct constructions across the codebase (inbinding.rs,preamble.rs,mod.rs), ensuring that custom backoff configurations are preserved during server state transitions and type conversions. -
Runtime Optimization: The server runtime now destructures
backoff_configdirectly from theWireframeServerstruct and passes it to the accept loop, eliminating the need for inlineBackoffConfig::default()calls during runtime. -
Test Improvements: The test suite has been enhanced with better type safety by using a properly derived
PreambleHandlerKindenum instead of string-based matching, and includes comprehensive handler execution testing with real TCP connections. -
Readiness Signal Refinement: The readiness signaling logic has been simplified using the more idiomatic
is_some_and()pattern.
These changes integrate well with the existing Wireframe architecture by preserving the builder pattern's configuration chain, improving performance through reduced allocations, and enhancing test robustness without breaking existing APIs.
Important Files Changed
Files Modified
| Filename | Score | Overview |
|---|---|---|
src/server/config/preamble.rs |
5/5 | Adds backoff_config field preservation in with_preamble method |
src/server/runtime.rs |
4/5 | Refactors to use pre-initialized BackoffConfig and simplifies readiness logic |
src/server/config/binding.rs |
5/5 | Ensures backoff_config field is copied in bind methods |
src/server/config/mod.rs |
5/5 | Pre-initializes BackoffConfig in constructor and adds required imports |
src/server/mod.rs |
4/5 | Adds backoff_config field to WireframeServer struct |
src/server/config/tests.rs |
4/5 | Replaces string-based tests with typed enum and adds handler execution tests |
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it addresses configuration bugs and improves code quality
- Score reflects solid implementation of previous review feedback with proper configuration persistence and test improvements
- Pay close attention to
src/server/runtime.rsandsrc/server/config/tests.rsfor the more complex logic changes
6 files reviewed, no comments
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/server/config/binding.rs(2 hunks)src/server/config/mod.rs(2 hunks)src/server/config/preamble.rs(1 hunks)src/server/config/tests.rs(2 hunks)src/server/mod.rs(1 hunks)src/server/runtime.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/server/mod.rssrc/server/config/preamble.rssrc/server/config/binding.rssrc/server/config/mod.rssrc/server/runtime.rssrc/server/config/tests.rs
🧬 Code Graph Analysis (2)
src/server/config/mod.rs (1)
src/server/runtime.rs (1)
default(46-51)
src/server/config/tests.rs (4)
src/server/test_util.rs (4)
bind_server(36-43)factory(23-25)free_port(28-34)server_with_preamble(50-55)tests/preamble.rs (2)
tokio(132-132)tokio(133-133)src/server/config/mod.rs (1)
new(67-79)src/server/config/binding.rs (2)
bind(57-60)bind(144-147)
🔍 Additional research (Context7, Deepwiki)
Summary of additional context found (relevant to this PR):
-
Repository docs/wiki confirm WireframeServer behavior, preamble flow, ready-signal, and backoff/accept loop design; preamble callbacks (on_preamble_decode_success/failure), RewindStream, and tests drive preamble behavior via bincode and local TCP (matches test changes) — see server/preamble/run sections.,
-
Testing infrastructure: the project uses rstest, wireframe_testing helpers (drive_with_bincode/duplex), and tokio-based in-memory TCP utilities; tests commonly spawn local TcpListener/TcpStream for integration-style callbacks (supports the new Success path added in tests).,
-
Backoff/accept-loop: docs describe accept-loop backoff for accept errors and worker-per-app factory pattern; propagating a BackoffConfig from server state into accept_loop (as PR changes do) aligns with documented design. Ready-signal is documented as oneshot sent after workers spawn (matches runtime change to send readiness).,
-
Related PRs and overlap: docs/wiki list of related changes around backoff_config, binding paths, and preamble tests — multiple related PRs referenced in the PR metadata are consistent with repo history and design notes.,
Files/areas to focus in review (based on docs):
- src/server/config/tests.rs (preamble tests): new typed enum, TCP listener usage, bincode DecodeError handling.
- src/server/{runtime.rs, config/{mod.rs,binding.rs,preamble.rs}, mod.rs}: propagation of BackoffConfig and readiness signal handling.
- Tests & wireframe_testing helpers (ensure test scaffolding matches project patterns and no flakiness introduced).
Tools used:
- functions.read_wiki_structure (Deepwiki) — repo wiki/pages listing and structure.
- functions.read_wiki_contents (Context7) — detailed docs/wiki contents referenced above.
🔇 Additional comments (14)
src/server/mod.rs (1)
122-123: Plumb backoff configuration through server state. LGTM.Retain the server’s BackoffConfig on the struct; downstream code can now use the configured timings instead of reinitialising defaults.
src/server/config/preamble.rs (1)
49-50: Preserve backoff_config across preamble typestate transitions. LGTM.Carry the BackoffConfig when switching preamble types to avoid losing server runtime configuration.
src/server/config/binding.rs (1)
87-98: Forward backoff_config on bind/rebind. LGTM.Propagate the stored BackoffConfig when constructing the Bound server in both bind paths; this keeps accept-loop behaviour consistent post-bind.
Also applies to: 172-184
src/server/config/mod.rs (1)
15-15: Initialise and expose BackoffConfig via builder state. LGTM.Import BackoffConfig and default-initialise it in new(); this removes ad-hoc defaults elsewhere and centralises configuration.
Also applies to: 75-76
src/server/runtime.rs (2)
130-131: Use the stored BackoffConfig instead of reinitialising. LGTM.Destructure backoff_config from WireframeServer and plumb it through; this respects builder-time configuration.
148-156: Pass BackoffConfig into accept_loop. LGTM.Stop constructing per-loop defaults and use the configured timings for retries.
src/server/config/tests.rs (8)
20-20: LGTM!The import of
DecodeErroris appropriate for the failure path testing in the preamble handler registration test.
28-28: LGTM!The import of Tokio's TCP primitives is necessary for the success path testing that establishes a local TCP connection.
30-34: LGTM!The typed enum provides excellent type safety compared to string-based matching. The derives are comprehensive and appropriate for test usage.
103-104: LGTM!The named rstest cases improve test output readability significantly.
108-108: LGTM!The function signature change to accept the typed enum enhances type safety.
114-125: LGTM!The match statement properly handles both enum variants with appropriate handler registration. The pattern matching provides clear, type-safe dispatch.
128-155: Excellent test implementation with proper TCP scaffolding.The success path correctly establishes a local TCP listener and client connection to exercise the preamble handler with actual network streams. The failure path appropriately uses
DecodeError::UnexpectedEndto trigger the failure handler. Both paths properly verify handler invocation through counter increments.
156-156: LGTM!The assertion confirms that exactly one handler invocation occurs, addressing the previous review comment about verifying handler execution.
|
@coderabbitai have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll check the current state of the code to verify if those issues have been resolved. 🏁 Script executed: Length of output: 3075 Let me check specifically for the clippy collapsible_if attribute in the runtime file, since that's where the readiness signal changes were made: 🏁 Script executed: Length of output: 935 @leynos Yes, all three issues have been resolved: ✅ Debug/PartialEq/Eq derives: ✅ Clippy attribute: No ✅ Descriptive naming: The enum has been renamed from The code is now more type-safe, follows Rust idioms better, and has improved readability for test failures. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/server/configuration.md(2 hunks)src/server/config/mod.rs(3 hunks)src/server/config/tests.rs(7 hunks)src/server/runtime.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/server/configuration.md
**/*.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/server/config/mod.rssrc/server/runtime.rssrc/server/config/tests.rs
🧬 Code Graph Analysis (2)
src/server/config/mod.rs (1)
src/server/runtime.rs (2)
default(46-51)normalised(56-63)
src/server/config/tests.rs (5)
src/server/test_util.rs (4)
bind_server(36-43)factory(23-25)server_with_preamble(50-55)WireframeServer(54-54)tests/preamble.rs (4)
tokio(132-132)tokio(133-133)WireframeServer(56-58)WireframeServer(268-272)src/server/config/mod.rs (1)
new(67-79)src/server/config/binding.rs (2)
bind(57-60)bind(144-147)src/server/runtime.rs (1)
default(46-51)
🔍 Additional research (Deepwiki)
Summary of relevant context for reviewing the PR
-
Backoff config & API changes
- BackoffConfig introduced and propagated through WireframeServer (new field backoff_config); builder gains accept_backoff(cfg: BackoffConfig) that stores cfg.normalised(); accept_backoff call sites in tests updated to pass BackoffConfig instead of two Durations.,
-
Runtime/accept loop
- accept_loop now accepts a BackoffConfig parameter; runtime now passes server.backoff_config into worker accept_loop; BackoffConfig derives PartialEq/Eq and has method normalised; debug_asserts added to validate invariants; readiness signalling simplified to use is_some_and.
-
Binding / with_preamble propagation
- Binding code paths and with_preamble now propagate backoff_config when creating/rebinding WireframeServer instances so backoff_config persists across builder transformations.
-
Preamble test refactor
- Tests replaced string-based handler selection with test-only enum PreambleHandlerKind { Success, Failure }; tests now exercise on_preamble_decode_success by spinning up TcpListener/TcpStream and on_preamble_decode_failure by invoking with DecodeError::UnexpectedEnd; assertions check shared counter increments.
-
Related areas to inspect during review
- Ensure public API change (accept_backoff signature) is consistent across crate boundaries and docs/examples (docs updated).
- Confirm BackoffConfig::normalised enforces invariants (initial_delay >= 1ms and <= max_delay) and review debug_asserts for intended release behavior.
- Verify readiness signalling timing (sent after workers spawned) and no regressions in tests relying on ready signal.
- Check tests that now use Tokio TcpListener/TcpStream for flakiness or additional async runtime requirements.
Tools used: Deepwiki.read_wiki_structure, Deepwiki.read_wiki_contents
🔇 Additional comments (18)
src/server/config/mod.rs (2)
15-15: Wire in BackoffConfig at the config layer.Import BackoffConfig at this level to keep the builder API cohesive with runtime types. This aligns the configuration module with the runtime’s backoff handling.
75-75: Initialise a sane default backoff.Defaulting to BackoffConfig::default() gives predictable behaviour out of the box and keeps invariants intact without additional normalisation.
docs/server/configuration.md (1)
26-35: Update example to the new BackoffConfig-based API.The example correctly demonstrates constructing BackoffConfig and passing it via accept_backoff. This keeps the docs aligned with the public API.
src/server/runtime.rs (6)
39-39: Derive Debug and equality for BackoffConfig.Deriving PartialEq/Eq is appropriate for config comparisons in tests and Debug aids diagnostics.
54-64: Provide a must_use normalised() constructor-style helper.Implement normalised() to enforce invariants consistently. This prevents invalid configs propagating.
142-163: Plumb the stored BackoffConfig into workers.Destructure backoff_config from the server and pass it into accept_loop. This removes the previous silent defaulting and honours the builder configuration.
166-170: Signal readiness after spawning workers.Send the ready signal only after all accept loops are spawned to avoid racey readiness. Using is_some_and keeps the code concise without lint suppressions.
194-201: Assert backoff invariants in debug builds.Guard the accept loop against invalid timing in debug builds. This is low-overhead and catches misconfigurations early.
166-169: Ensure MSRV supports Option::is_some_andNo
rust-versionsetting orrust-toolchainfile detected. Manually verify that the project’s minimum supported Rust version is ≥ 1.70. If the MSRV is lower, replace this pattern:// Signal readiness after all workers have been spawned. if ready_tx.is_some_and(|tx| tx.send(()).is_err()) { tracing::warn!("Failed to send readiness signal: receiver dropped"); }with an equivalent construct compatible with older Rust versions, for example:
if let Some(tx) = ready_tx { if tx.send(()).is_err() { tracing::warn!("Failed to send readiness signal: receiver dropped"); } }
- File:
src/server/runtime.rs- Lines: 166–169
src/server/config/tests.rs (9)
20-31: Adopt BackoffConfig in tests and import test utilities.Importing BackoffConfig and TCP primitives prepares tests for the typed backoff and preamble handler execution paths.
33-37: Use a typed enum for handlers in tests.Replace string matching with PreambleHandlerKind to eliminate brittle comparisons and improve readability.
106-112: Name rstest cases and type the parameter.Label cases for clear test output and accept a typed enum argument for robust branching.
117-128: Exercise preamble success/failure handlers with real IO and direct error.Drive the success path via a local TCP round-trip and the failure path via a DecodeError to assert handler invocation exactly once. This gives strong signal without flakiness.
Also applies to: 131-158, 159-159
239-245: Assert end-to-end assignment of BackoffConfig via builder.Validate that accept_backoff stores the provided configuration, aligning with the new API.
300-302: Validate initial_delay override via BackoffConfig.Ensure per-field overrides propagate when using struct update syntax.
310-312: Validate max_delay override via BackoffConfig.Ensure max_delay updates persist through accept_backoff.
317-329: Validate normalisation for zero and swapped values.Assert clamping to 1 ms and max/inital swap on inverted inputs. This matches BackoffConfig::normalised.
359-363: Keep the parameter-swapping tests aligned with normalised().Assert swapped and clamped values after passing inverted and zero durations. These cases provide good coverage of the invariants.
Also applies to: 369-377
✅ Actions performedReview triggered.
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Sorry @leynos, you have reached your 24-hour rate limit for Sourcery. Please try again later
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Sorry @leynos, you have reached your 24-hour rate limit for Sourcery. Please try again later
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/server/configuration.md(2 hunks)src/server/config/mod.rs(3 hunks)src/server/config/tests.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/server/config/mod.rssrc/server/config/tests.rs
**/*.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/server/configuration.md
🧬 Code Graph Analysis (2)
src/server/config/mod.rs (1)
src/server/runtime.rs (2)
default(46-51)normalised(56-63)
src/server/config/tests.rs (5)
src/server/test_util.rs (5)
bind_server(36-43)factory(23-25)free_port(28-34)server_with_preamble(50-55)WireframeServer(54-54)tests/preamble.rs (4)
tokio(132-132)tokio(133-133)WireframeServer(56-58)WireframeServer(268-272)src/server/config/mod.rs (1)
new(67-79)src/server/config/binding.rs (2)
bind(57-60)bind(144-147)src/server/runtime.rs (1)
default(46-51)
🪛 LanguageTool
docs/server/configuration.md
[style] ~11-~11: Would you like to use the Oxford spelling “normalizes”? The spelling ‘normalises’ is also correct.
Context: ...ng a BackoffConfig value. The builder normalises the supplied configuration via `Backoff...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~11-~11: Would you like to use the Oxford spelling “normalized”? The spelling ‘normalised’ is also correct.
Context: ...rmalises the supplied configuration via BackoffConfig::normalised, so out-of-range values are adjusted r...
(OXFORD_SPELLING_Z_NOT_S)
🔍 Remote MCP (Deepwiki)
Relevant facts found in the repository docs/wiki that affect review (concise):
-
BackoffConfig & accept_backoff API
- BackoffConfig is documented as a public type (initial_delay, max_delay) and accept_backoff(cfg: BackoffConfig) replaces the former two-duration builder; BackoffConfig::normalised enforces initial_delay >= 1ms and initial_delay <= max_delay (clamping/swapping rules).
-
Runtime/accept loop & readiness
- Runtime now threads server.backoff_config into worker accept_loop; accept_loop debug_asserts initial_delay <= max_delay and initial_delay >= 1ms. Ready signal is sent after all workers are spawned (oneshot sent post-spawn). This aligns with the PR change to use is_some_and for readiness sending.
-
Preamble handling & tests
- Preamble flow is bincode-based; with_preamble() + on_preamble_decode_success / _failure callbacks are core APIs. Tests use tokio::TcpListener/TcpStream and bincode decoding for preamble; the repo docs describe adaptive read and DecodeError paths used by tests. (Impl detail: read_preamble may return UnexpectedEnd.)
-
Wiring propagation
- Builder transformations (with_preamble, bind, binding.rs) are expected to propagate backoff_config across WireframeServer instances — the docs/wiki describe preserving fields across builder changes.
-
Tests / tooling expectations
- Test infra uses tokio, wireframe_testing helpers (drive_with_bincode, duplex), rstest fixtures, and requires running make lint / make test in CI. Tests use time control (tokio::time::pause/advance) and may rely on deterministic timing. Network-based preamble tests spawn real TcpListener/TcpStream locally (docs & testing pages).
-
CI & quality gates
- CI enforces cargo fmt, clippy -D warnings, and full test runs; contributor workflow expects make lint / make test prior to merge.,
Sources/tools used:
- Repository wiki/pages listing and content retrieval via Deepwiki API:,
🔇 Additional comments (16)
src/server/config/mod.rs (2)
15-15: Scope BackoffConfig import with server types — LGTM.Importing
BackoffConfigalongsideServerState,Unbound, andWireframeServerkeeps the builder module cohesive.
75-75: Initialise backoff_config with sensible defaults — LGTM.Defaulting to
BackoffConfig::default()aligns with the runtime defaults (10 ms initial, 1 s max) and keeps behaviour predictable.docs/server/configuration.md (2)
10-12: Fix standalone Markdown type formatting — LGTM.Use of backticks for
BackoffConfigin standalone Markdown is correct. Retain en-GB spelling (“normalises”/“normalised”) per project guidelines.
28-37: BackoffConfig example and API usage — LGTM.The example compiles as written and clearly demonstrates constructing a config and passing it to
accept_backoff(cfg). This matches the builder’s normalisation semantics described above.src/server/config/tests.rs (12)
20-31: Introduce precise imports and test utilities — LGTM.Importing
DecodeError, Tokio TCP types, and test utilities keeps the test self-contained and expressive.
33-37: Replace stringly selectors with a typed enum — LGTM.
PreambleHandlerKindwithClone, Copy, Debug, PartialEq, Eqimproves readability and diagnostics in assertions.
105-107: Name rstest cases for clarity — LGTM.Labelling cases as
success/failureimproves test report readability.
111-111: Use typed enum in parameterised test — LGTM.Typed
#[case] handler: PreambleHandlerKindremoves brittle string matching.
117-128: Register preamble handlers via typed dispatch — LGTM.The success handler increments asynchronously; the failure handler increments synchronously. Both paths are exercised in the test.
160-160: Assert handler invocation count — LGTM.Verifying a single increment robustly proves only the selected handler ran once.
239-245: Backoff configuration equality test — LGTM.Equality holds because the supplied config already satisfies normalisation invariants.
300-303: Initial delay override test — LGTM.Using struct update syntax with
..BackoffConfig::default()is idiomatic and clear.
310-313: Max delay override test — LGTM.The assertion targets only the field under test, avoiding over-coupling to defaults.
317-329: Normalisation behaviour tests — LGTM.
- Clamp of zero initial delay to 1 ms is asserted.
- Raising
max_delayto matchinitial_delaywhen needed is asserted.
346-353: Swap expectation when initial > max — LGTM.Asserting
initial_delay = 1sandmax_delay = 2smatchesnormalised()semantics.
359-363: Parameter swapping and zero handling — LGTM.
- Swap to maintain
initial <= maxis correct.- Both zero values normalise to 1 ms as expected.
Also applies to: 369-372
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/server/config/mod.rs (1)
122-137: Document normalisation and swap behaviour; add a usage example for parity with other setters.The docstring correctly explains normalisation and swap semantics. Add an Examples section (as with workers/ready_signal) to show expected outcomes and reduce ambiguity.
Apply this diff to extend the docs:
builder_setter!( /// Configure accept-loop backoff behaviour. /// /// The supplied configuration is passed to /// [`BackoffConfig::normalised`] (`cfg.normalised()`) before being /// stored. Normalisation clamps `initial_delay` to at least 1 ms and no /// greater than `max_delay`. If `initial_delay` exceeds `max_delay`, /// the values are swapped. Normalisation applies any other adjustments /// `BackoffConfig::normalised` defines so out-of-range values are /// corrected rather than preserved. /// + /// # Examples + /// + /// ``` + /// use std::time::Duration; + /// use wireframe::{app::WireframeApp, server::{BackoffConfig, WireframeServer}}; + /// + /// let cfg = BackoffConfig { + /// initial_delay: Duration::from_millis(5), + /// max_delay: Duration::from_millis(500), + /// }; + /// let server = WireframeServer::new(|| WireframeApp::default()).accept_backoff(cfg); + /// assert_eq!(server.backoff_config.initial_delay, Duration::from_millis(5)); + /// assert_eq!(server.backoff_config.max_delay, Duration::from_millis(500)); + /// + /// // Values are swapped if initial_delay > max_delay: + /// let cfg = BackoffConfig { + /// initial_delay: Duration::from_secs(2), + /// max_delay: Duration::from_secs(1), + /// }; + /// let server = WireframeServer::new(|| WireframeApp::default()).accept_backoff(cfg); + /// assert_eq!(server.backoff_config.initial_delay, Duration::from_secs(1)); + /// assert_eq!(server.backoff_config.max_delay, Duration::from_secs(2)); + /// ``` + /// /// Invariants: /// - `initial_delay` must be >= 1 ms /// - `initial_delay` must be <= `max_delay` accept_backoff, backoff_config, cfg: BackoffConfig => cfg.normalised() );src/server/config/tests.rs (3)
33-37: Strengthen enum derives for ergonomics and diagnostics.Derive Debug, PartialEq, and Eq to improve assertion output and enable direct comparisons in rstest cases.
105-107: Name rstest cases for readable reports.Label cases to make failures self-explanatory in CI output. Good fix.
346-353: Assert swapped values when initial_delay exceeds max_delay.Match BackoffConfig::normalised semantics by asserting initial=1s and max=2s. Good correction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/server/config/mod.rs(3 hunks)src/server/config/tests.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/server/config/mod.rssrc/server/config/tests.rs
🧬 Code Graph Analysis (2)
src/server/config/mod.rs (1)
src/server/runtime.rs (2)
default(46-51)normalised(56-63)
src/server/config/tests.rs (5)
src/server/test_util.rs (5)
bind_server(36-43)factory(23-25)free_port(28-34)server_with_preamble(50-55)WireframeServer(54-54)tests/preamble.rs (4)
tokio(132-132)tokio(133-133)WireframeServer(56-58)WireframeServer(268-272)src/server/config/mod.rs (1)
new(67-79)src/server/config/binding.rs (2)
bind(57-60)bind(144-147)src/server/runtime.rs (1)
default(46-51)
🔍 Remote MCP (Deepwiki)
Relevant facts from repository docs useful for reviewing this PR:
-
BackoffConfig API & invariants
- BackoffConfig is a public type (initial_delay: Duration, max_delay: Duration). It must be normalised so initial_delay >= 1 ms and initial_delay <= max_delay (clamp/swap rules enforced by BackoffConfig::normalised).
-
Server runtime / accept loop behavior
- WireframeServer threads a single backoff_config into per-worker accept_loop; accept_loop contains debug_asserts enforcing BackoffConfig invariants. Ready signal (oneshot) is sent after all workers are spawned.
-
API surface changes to check
- accept_backoff(builder) changed from two-duration args to accept_backoff(cfg: BackoffConfig); BackoffConfig exported under wireframe::server; builder setters for accept_initial_delay/accept_max_delay removed in docs. WireframeServer stores backoff_config pub(crate).
-
Builder / binding / with_preamble propagation
- Builders (bind, with_preamble, binding.rs) must preserve backoff_config when producing new WireframeServer instances.
-
Tests & test infra expectations
- Tests use tokio, wireframe_testing helpers (drive_with_bincode, duplex), sometimes spawn real TcpListener/TcpStream for preamble tests; tests use tokio::time::pause/advance for deterministic timing. CI requires make lint / make test; clippy warnings are -D warnings.
-
Documentation updates
- docs/server/configuration.md updated to show BackoffConfig usage and normalisation behavior; examples switch to BackoffConfig.
🔇 Additional comments (10)
src/server/config/mod.rs (2)
15-15: Pull BackoffConfig into scope at the right level.Import BackoffConfig alongside ServerState/Unbound/WireframeServer to keep intra-module links resolvable and avoid fully-qualified paths in docs.
75-75: Initialise backoff_config to a sane default.Defaulting to BackoffConfig::default() matches runtime defaults and avoids uninitialised state.
src/server/config/tests.rs (8)
20-31: Import DecodeError, test utilities, and Tokio TCP types.Use the concrete DecodeError and test_util helpers to make tests precise and focused. Bringing TcpListener/TcpStream into scope enables the preamble handler exercise over a real socket.
111-111: Type the handler selector parameter.Use the typed enum in place of string matching for robustness and compile-time coverage.
117-128: Register the appropriate preamble handler based on the enum.Set the success/failure handlers in a single match; keep closures small and side-effect–free except for the counter increment.
239-245: Assert equality against the provided cfg after normalisation.Provide in-range values so the stored config equals the input. Good direct equality assertion.
300-303: Verify initial_delay setter via BackoffConfig.Set initial_delay through cfg and assert the stored field matches.
310-313: Verify max_delay setter via BackoffConfig.Set max_delay through cfg and assert the stored field matches.
359-363: Assert swap when initial <-> max are provided in reverse order.Verify initial_delay is clamped to 1 ms and max to 5 ms.
369-378: Clamp zero delays to the minimum.Assert both ends clamp to 1 ms when ZERO is supplied, matching documented invariants.
Await the client connection before calling accept to avoid hanging in the preamble handler test. Also assert the swapped initial delay in backoff validation.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/server/config/tests.rs (1)
249-295: Avoid wall-clock sleeps in unit tests; assert the backoff schedule deterministically.Wall-clock sleeps introduce flakiness and slow the suite. Validate the doubling-and-capping logic without sleeping.
Example refactor:
- Extract a pure function that computes the backoff sequence.
- Assert the sequence equals the expected values.
-/// Behaviour test verifying exponential delay doubling and capping. -#[test] -fn test_accept_exponential_backoff_doubles_and_caps() { - use std::{ - thread, - time::{Duration, Instant}, - }; - - let initial = Duration::from_millis(10); - let max = Duration::from_millis(80); - let mut backoff = initial; - let mut delays = Vec::new(); - let attempts = 5; - - let start = Instant::now(); - let mut last = start; - - for _i in 0..attempts { - thread::sleep(backoff); - let now = Instant::now(); - let elapsed = now.duration_since(last); - delays.push(elapsed); - last = now; - - backoff = std::cmp::min(backoff * 2, max); - } - - let expected_delays = [ - initial, - std::cmp::min(initial * 2, max), - std::cmp::min(initial * 4, max), - std::cmp::min(initial * 8, max), - max, - ]; - - for (i, (actual, expected)) in delays.iter().zip(expected_delays.iter()).enumerate() { - assert!( - *actual >= *expected, - "Delay {i} was {actual:?}, expected at least {expected:?}" - ); - let max_expected = *expected + Duration::from_millis(20); - assert!( - *actual < max_expected, - "Delay {i} was {actual:?}, expected less than {max_expected:?}" - ); - } -} +/// Behaviour test verifying exponential delay doubling and capping (pure logic). +#[test] +fn test_accept_exponential_backoff_doubles_and_caps() { + use std::time::Duration; + + let initial = Duration::from_millis(10); + let max = Duration::from_millis(80); + let attempts = 5usize; + + let mut backoff = initial; + let mut sequence = Vec::with_capacity(attempts); + for _ in 0..attempts { + sequence.push(backoff); + backoff = std::cmp::min(backoff.saturating_mul(2), max); + } + + let expected = [ + initial, + std::cmp::min(initial * 2, max), + std::cmp::min(initial * 4, max), + std::cmp::min(initial * 8, max), + max, + ]; + assert_eq!(&sequence[..], &expected); +}
♻️ Duplicate comments (1)
docs/server/configuration.md (1)
23-23: Acknowledge fix: remove Rustdoc-style link brackets around BackoffConfig.The type name is now correctly formatted as inline code in standalone Markdown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (7)
docs/server/configuration.md(2 hunks)src/server/config/binding.rs(2 hunks)src/server/config/mod.rs(3 hunks)src/server/config/preamble.rs(1 hunks)src/server/config/tests.rs(6 hunks)src/server/mod.rs(1 hunks)src/server/runtime.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
src/server/config/binding.rssrc/server/mod.rssrc/server/config/mod.rssrc/server/config/preamble.rssrc/server/runtime.rssrc/server/config/tests.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/server/config/binding.rssrc/server/mod.rssrc/server/config/mod.rssrc/server/config/preamble.rssrc/server/runtime.rssrc/server/config/tests.rs
{src,tests}/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/server/config/binding.rssrc/server/mod.rssrc/server/config/mod.rssrc/server/config/preamble.rssrc/server/runtime.rssrc/server/config/tests.rs
docs/**/*.md
📄 CodeRabbit Inference Engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation text.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation text.
Treat company names as collective nouns in documentation (e.g., "Lille Industries are expanding").
Write headings in sentence case in documentation.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations for Markdown files.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL; avoid reference-style links like[foo][bar].
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown.
Ensure tables have a delimiter line below the header row in Markdown.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns in documentation.
Wrap code at 120 columns in documentation.
Do not wrap tables in documentation.
Use sequentially numbered footnotes referenced with[^1]and place definitions at the end of the file.
Where it adds clarity, include Mermaid diagrams in documentation.
When embedding figures, useand provide concise alt text describing the content.
Add a brief description before each Mermaid diagram in documentation for screen readers.Document examples showing how to deprecate old message versions gracefully
Write the official documentation for the new features. Create separate guides for "Duplex Messaging & Pushes", "Streaming Responses", and "Message Fragmentation". Each guide must include runnable examples and explain the relevant concepts and APIs.
docs/**/*.md: Use docs/ markdown ...
Files:
docs/server/configuration.md
docs/**/*.{md,rs}
📄 CodeRabbit Inference Engine (docs/multi-packet-and-streaming-responses-design.md)
docs/**/*.{md,rs}: The official documentation and examples must exclusively use the declarativeResponsemodel for handler responses.
The async-stream pattern must be documented as the canonical approach for dynamic stream generation.
Files:
docs/server/configuration.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns
Markdown code blocks must be wrapped at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/server/configuration.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/server/configuration.md
🧬 Code Graph Analysis (2)
src/server/config/mod.rs (1)
src/server/runtime.rs (2)
default(64-69)normalised(74-81)
src/server/config/tests.rs (4)
src/server/test_util.rs (5)
factory(23-25)free_listener(32-35)listener_addr(74-78)server_with_preamble(90-95)WireframeServer(94-94)src/server/config/mod.rs (1)
new(71-83)src/server/config/binding.rs (2)
bind(56-59)bind(143-146)src/server/runtime.rs (1)
default(64-69)
🪛 LanguageTool
docs/server/configuration.md
[style] ~24-~24: Would you like to use the Oxford spelling “normalizes”? The spelling ‘normalises’ is also correct.
Context: ...ng a BackoffConfig value. The builder normalises the supplied configuration via `Backoff...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~24-~24: Would you like to use the Oxford spelling “normalized”? The spelling ‘normalised’ is also correct.
Context: ...rmalises the supplied configuration via BackoffConfig::normalised, so out-of-range values are adjusted r...
(OXFORD_SPELLING_Z_NOT_S)
🔍 Remote MCP (Deepwiki)
Relevant additional facts for reviewing this PR
-
BackoffConfig behavior & invariants:
- BackoffConfig is public, holds initial_delay and max_delay, adds normalised() to enforce initial_delay >= 1ms and initial_delay <= max_delay (swap/clamp semantics). Tests assert defaults and validation/clamping behavior.
-
API surface & propagation:
- WireframeServer now stores backoff_config (pub(crate)) and exposes builder accept_backoff(cfg: BackoffConfig) which stores cfg.normalised(). accept_loop signature updated to accept backoff_config and worker construction/with_preamble/binding paths propagate the server.backoff_config.
-
Runtime/readiness changes:
- run_with_shutdown/worker startup now send the ready signal after spawning all workers (ready_tx.is_some_and(|tx| tx.send(()).is_err()) used) and accept_loop uses the provided backoff_config (with debug_asserts on invariants).
-
Tests & test infra changes:
- Preamble tests converted to a strongly-typed PreambleHandlerKind enum; test_preamble_handler_registration now uses a real local TcpListener/TcpStream to exercise on_preamble_decode_success/failure paths and increments a shared counter exactly once. Many tests updated to construct and pass BackoffConfig objects (tokio time control and wireframe_testing helpers used).
-
Areas to focus in review (based on changes and docs):
- Ensure BackoffConfig::normalised() semantics match documentation/tests (clamping/swap) and accept_backoff builder applies normalised() before storage.
- Verify accept_loop uses the propagated backoff_config everywhere (no remaining Default() calls).
- Confirm ready-signal ordering: sent after all workers spawned and that warning-on-send-failure behavior is correct.
- Check propagation of backoff_config through with_preamble and bind_existing_listener codepaths to avoid mismatches.
Tools/sources used
- Repository wiki pages (structure + full contents) via read_wiki_structure and read_wiki_contents.,
⏰ 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)
src/server/runtime.rs (3)
57-61: Derive traits on BackoffConfig improve test diagnostics and ergonomicsDeriving Clone, Copy, Debug, PartialEq, Eq is appropriate for a small config struct and helps with assertions/logging.
189-198: Plumb and pass BackoffConfig through runtimeDestructure and pass the stored backoff_config into accept_loop. This removes the prior defaulting and preserves configured values.
Also applies to: 216-217
298-304: Validate backoff invariants in debug buildsThe debug assertions are precise and match the documented invariants. Keep them to catch misconfigurations early.
src/server/mod.rs (1)
121-127: Persist backoff_config on WireframeServerAdd the field to the server state so configuration survives binding and typestate transitions. This aligns with the runtime wiring.
src/server/config/preamble.rs (1)
43-51: Carry backoff_config across preamble type transitionsPreserve the existing backoff configuration when switching preamble type to avoid surprising resets.
src/server/config/binding.rs (2)
86-96: Propagate backoff_config when transitioning Unbound -> BoundRetain the configured backoff when binding to a listener. This avoids silent defaults after binding.
171-183: Propagate backoff_config when rebinding Bound -> BoundMaintain backoff configuration during rebinds as well. This keeps behaviour consistent across state changes.
src/server/config/mod.rs (3)
19-21: Import BackoffConfig into builder module scopeExpose the type in this module to support the new builder method.
71-83: Initialise backoff_config with sensible defaultsDefaulting at construction is correct and keeps behaviour predictable before callers override it.
126-141: Add builder for accept-loop backoff with explicit normalisationProvide a fluent API and document the normalisation behaviour. The assignment uses cfg.normalised(), which enforces invariants at the edge.
docs/server/configuration.md (1)
41-50: LGTM: update example to import BackoffConfig and use accept_backoff(cfg).The example correctly demonstrates constructing BackoffConfig and passing it to the builder, aligning with the new API surface and normalisation semantics.
src/server/config/tests.rs (8)
32-36: LGTM: derive traits on test enum for clearer diagnostics.Deriving Clone, Copy, Debug, PartialEq, Eq on PreambleHandlerKind improves assertion messages and ergonomics.
109-164: LGTM: stabilise preamble handler tests and avoid async hangs.
- Name rstest cases for readable reports.
- Await TcpStream::connect before accept, preventing a potential deadlock.
- Assert handler presence and ensure the counter increments exactly once.
All align with robust async testing practices.
241-247: LGTM: assert BackoffConfig is stored unchanged when already valid.Asserting equality with the provided cfg validates that the builder stores the normalised value, which here is identical to input.
301-305: LGTM: set and assert initial_delay via BackoffConfig.This verifies the single-struct builder path replaces the previous two-argument API cleanly.
312-315: LGTM: set and assert max_delay via BackoffConfig.The test covers the upper bound configuration path correctly.
319-336: LGTM: exercise clamping and swapping invariants.
- Clamp zero initial_delay to 1 ms.
- Swap when initial_delay > max_delay and assert both fields.
Good coverage of BackoffConfig::normalised semantics.
352-359: LGTM: validate initial_delay > default max swaps to maintain invariants.The assertions match the documented behaviour.
365-384: LGTM: cover both swap and double-zero clamping paths.The tests correctly assert the resulting initial/max delays after normalisation.
| `accept_backoff(initial_delay, max_delay)` sets both bounds in one call. These | ||
| values are stored in `BackoffConfig`: | ||
| `accept_backoff(cfg)` sets both bounds using a `BackoffConfig` value. The | ||
| builder normalises the supplied configuration via `BackoffConfig::normalised`, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use -ize spelling per en-GB-oxendict style.
Adopt -ize spelling in prose while keeping the method name as-is.
- builder normalises the supplied configuration via `BackoffConfig::normalised`,
+ builder normalizes the supplied configuration via `BackoffConfig::normalised`,📝 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.
| builder normalises the supplied configuration via `BackoffConfig::normalised`, | |
| -builder normalises the supplied configuration via `BackoffConfig::normalised`, | |
| +builder normalizes the supplied configuration via `BackoffConfig::normalised`, |
🧰 Tools
🪛 LanguageTool
[style] ~24-~24: Would you like to use the Oxford spelling “normalizes”? The spelling ‘normalises’ is also correct.
Context: ...ng a BackoffConfig value. The builder normalises the supplied configuration via `Backoff...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~24-~24: Would you like to use the Oxford spelling “normalized”? The spelling ‘normalised’ is also correct.
Context: ...rmalises the supplied configuration via BackoffConfig::normalised, so out-of-range values are adjusted r...
(OXFORD_SPELLING_Z_NOT_S)
🤖 Prompt for AI Agents
In docs/server/configuration.md around line 24, the prose uses en-GB -ise
spelling ("normalises") but the project prefers -ize; update the sentence to use
"normalizes the supplied configuration via" while leaving the code/method
reference `BackoffConfig::normalised` exactly as-is (do not rename the method),
so the text reads something like: "builder normalizes the supplied configuration
via `BackoffConfig::normalised`".
There was a problem hiding this comment.
🧩 Analysis chain
@leynos I'll create a GitHub issue to track the spelling consistency fix.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "Fix spelling consistency: use -ize instead of -ise in documentation prose" \
--body "## Description
Fix spelling inconsistency in \`docs/server/configuration.md\` to follow en-GB-oxendict style (-ize spelling).
## Details
On line 24, change:
- \"builder normalises the supplied configuration via\"
to:
- \"builder normalizes the supplied configuration via\"
The method name \`BackoffConfig::normalised\` should remain unchanged - only update the prose text.
## Context
Identified during PR review: https://github.com/leynos/wireframe/pull/301#discussion_r2277529340
## Reference
- PR: https://github.com/leynos/wireframe/pull/301
- Comment: https://github.com/leynos/wireframe/pull/301#discussion_r2277529340
- Requested by: leynos" \
--assignee leynos
Length of output: 822
Perfect! I've created GitHub issue #313 to track the spelling consistency fix. The issue documents the need to change "normalises" to "normalizes" in the documentation prose while keeping the method name BackoffConfig::normalised unchanged.
The issue has been assigned to you and includes all the relevant context and references to this PR discussion.
🐰
/ \
| o.o |
\___/
| |
/ \
| use crate::server::{ | ||
| test_util::{ | ||
| TestPreamble, | ||
| bind_server, | ||
| factory, | ||
| free_listener, | ||
| listener_addr, | ||
| server_with_preamble, | ||
| }, | ||
| BackoffConfig, | ||
| use tokio::net::{TcpListener, TcpStream}; | ||
|
|
There was a problem hiding this comment.
Fix syntax error: close the grouped use import.
The grouped import starting at Line 20 is not closed, causing a compile error. Terminate the block before the next use.
use crate::server::{
test_util::{
TestPreamble,
bind_server,
factory,
free_listener,
listener_addr,
server_with_preamble,
},
- BackoffConfig,
- use tokio::net::{TcpListener, TcpStream};
+ BackoffConfig,
+};
+use tokio::net::{TcpListener, TcpStream};📝 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.
| use crate::server::{ | |
| test_util::{ | |
| TestPreamble, | |
| bind_server, | |
| factory, | |
| free_listener, | |
| listener_addr, | |
| server_with_preamble, | |
| }, | |
| BackoffConfig, | |
| use tokio::net::{TcpListener, TcpStream}; | |
| use crate::server::{ | |
| test_util::{ | |
| TestPreamble, | |
| bind_server, | |
| factory, | |
| free_listener, | |
| listener_addr, | |
| server_with_preamble, | |
| }, | |
| BackoffConfig, | |
| }; | |
| use tokio::net::{TcpListener, TcpStream}; |
🤖 Prompt for AI Agents
In src/server/config/tests.rs around lines 20 to 31, the grouped use import
beginning with use crate::server::{... is not closed before the next use
statement, causing a syntax error; close the brace and terminate the statement
(add the closing '}' and a semicolon) immediately after server items so the
subsequent use tokio::net::... line stands alone.
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue for the syntax error in the grouped use import.
@leynos I've created a GitHub issue for the syntax error in the grouped use import. The issue has been assigned to you and includes the details of the problem along with the expected fix.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "Fix syntax error in grouped use import in src/server/config/tests.rs" \
--body "## Problem
There is a syntax error in the grouped use import block in \`src/server/config/tests.rs\` around lines 20-31. The grouped import starting with \`use crate::server::{\` is not properly closed before the next use statement, causing a compile error.
## Location
File: \`src/server/config/tests.rs\`
Lines: 20-31
## Expected Fix
Close the grouped import block with \`};\` before the \`use tokio::net::\` statement:
\`\`\`rust
use crate::server::{
test_util::{
TestPreamble,
bind_server,
factory,
free_listener,
listener_addr,
server_with_preamble,
},
BackoffConfig,
};
use tokio::net::{TcpListener, TcpStream};
\`\`\`
## Backlinks
- PR: https://github.com/leynos/wireframe/pull/301
- Comment: https://github.com/leynos/wireframe/pull/301#discussion_r2277529347
- Requested by: leynos" \
--assignee leynos
Length of output: 1013
@leynos Perfect! I've successfully created GitHub issue #312 for the syntax error in the grouped use import. The issue has been assigned to you and includes:
- Clear problem description
- Specific file location and line numbers
- The exact fix needed to resolve the compilation error
- Backlinks to this PR and comment thread
The issue is now ready for you to address when convenient.
🧬 Code Graph Analysis Results
- File: src/server/config/mod.rs (71-83)
pub fn new(factory: F) -> Self {
let workers = std::thread::available_parallelism().map_or(1, std::num::NonZeroUsize::get);
Self {
factory,
workers,
on_preamble_success: None,
on_preamble_failure: None,
ready_tx: None,
backoff_config: BackoffConfig::default(),
state: Unbound,
_preamble: PhantomData,
}
}
- File: src/server/config/mod.rs (155-155)
pub const fn worker_count(&self) -> usize { self.workers }
- File: src/server/config/binding.rs (36-36)
pub const fn local_addr(&self) -> Option<SocketAddr> { None }
- File: src/server/config/binding.rs (121-121)
pub fn local_addr(&self) -> SocketAddr { self.state.listener.local_addr().ok() }
- File: src/server/config/binding.rs (56-59)
pub fn bind(self, addr: SocketAddr) -> Result<WireframeServer<F, T, Bound>, ServerError> {
let std = StdTcpListener::bind(addr).map_err(ServerError::Bind)?;
self.bind_existing_listener(std)
}
- File: src/server/config/binding.rs (143-146)
pub fn bind(self, addr: SocketAddr) -> Result<Self, ServerError> {
let std = StdTcpListener::bind(addr).map_err(ServerError::Bind)?;
self.bind_existing_listener(std)
}
- File: src/server/test_util.rs (23-25)
pub fn factory() -> impl Fn() -> WireframeApp + Send + Sync + Clone + 'static {
|| WireframeApp::default()
}
- File: src/server/test_util.rs (32-35)
pub fn free_listener() -> StdTcpListener {
let addr = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 0);
StdTcpListener::bind(addr).expect("Failed to bind free port listener")
}
- File: src/server/test_util.rs (74-78)
pub fn listener_addr(listener: &StdTcpListener) -> SocketAddr {
listener
.local_addr()
.expect("failed to get listener address")
}
- File: src/server/test_util.rs (80-87)
pub fn bind_server<F>(factory: F, listener: StdTcpListener) -> WireframeServer<F, (), Bound>
where
F: Fn() -> WireframeApp + Send + Sync + Clone + 'static,
{
WireframeServer::new(factory)
.bind_existing_listener(listener)
.expect("Failed to bind")
}
- File: src/server/test_util.rs (90-95)
pub fn server_with_preamble<F>(factory: F) -> WireframeServer<F, TestPreamble>
where
F: Fn() -> WireframeApp + Send + Sync + Clone + 'static,
{
WireframeServer::new(factory).with_preamble::<TestPreamble>()
}
- File: src/server/runtime.rs (33-33)
fn local_addr(&self) -> io::Result<SocketAddr>;
- File: src/server/runtime.rs (42-42)
fn local_addr(&self) -> io::Result<SocketAddr> { TcpListener::local_addr(self) }
- File: src/server/runtime.rs (64-69)
fn default() -> Self {
Self {
initial_delay: Duration::from_millis(10),
max_delay: Duration::from_secs(1),
}
}
- File: tests/preamble.rs (56-58)
WireframeServer::new(factory)
.workers(1)
.with_preamble::<HotlinePreamble>
- File: tests/preamble.rs (132-132)
tokio::sync::oneshot::channel::<HotlinePreamble>
- File: tests/preamble.rs (133-133)
tokio::sync::oneshot::channel::<()>
- File: tests/preamble.rs (268-272)
WireframeServer::new(factory.clone())
.with_preamble::<HotlinePreamble>()
.on_preamble_decode_success(success_cb::<HotlinePreamble>(hotline_success.clone()))
.on_preamble_decode_failure(failure_cb(hotline_failure.clone()))
.with_preamble::<OtherPreamble>
|
@coderabbitai please create a new GitHub issue for the following: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create a GitHub issue for the test improvement suggestion. @leynos I've created a GitHub issue for removing the wall-clock sleeps from the exponential backoff unit test. The issue includes the problem description, your suggested solution with the complete code refactor, and the benefits of making this change. 🏁 Script executed: Length of output: 1669 Perfect! I've successfully created GitHub issue #310 for removing wall-clock sleeps from the exponential backoff unit test. The issue is available at: #310 It includes the complete problem description, your suggested refactor code, the benefits of the change, and has been assigned to you as requested. |
2 participants
Summary
BackoffConfigand avoid let-chain for readiness signalTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_6897964c80f48322b0928bdbf71b492e
Summary by Sourcery
Use a typed enum for preamble handler tests and refine server runtime readiness signalling and backoff initialization
Enhancements: