Conversation
Reviewer's GuideThis PR adds optional dead letter queue (DLQ) support to push queues by refactoring constructors to validate rates, introducing a PushConfigError type, extending PushHandle to route dropped frames to a DLQ with logging, updating the WireframeApp builder, and expanding tests and documentation accordingly. Sequence diagram for push queue DLQ routingsequenceDiagram
participant Producer
participant PushHandle
participant HighPrioQueue
participant DLQ as DeadLetterQueue
Producer->>PushHandle: try_push(frame, priority, DropIfFull)
PushHandle->>HighPrioQueue: try_send(frame)
alt Queue full
PushHandle->>DLQ: try_send(frame)
alt DLQ full
PushHandle->>PushHandle: log error (frame lost)
else DLQ has space
DLQ-->>PushHandle: frame accepted
end
else Queue has space
HighPrioQueue-->>PushHandle: frame accepted
end
PushHandle-->>Producer: Ok or logs
Class diagram for WireframeApp builder DLQ supportclassDiagram
class WireframeApp {
-push_dlq: Option<mpsc::Sender<Vec<u8>>>
+with_push_dlq(dlq: mpsc::Sender<Vec<u8>>): Self
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Summary by CodeRabbitNew Features
Bug Fixes
Documentation
Tests
Chores
Summary by CodeRabbit
WalkthroughThis update introduces an optional Dead Letter Queue (DLQ) feature to the push queue system, allowing dropped frames to be forwarded to a DLQ channel when the push queue is full. The change includes updates to the application struct, push queue constructors, and test coverage for the new DLQ behaviour, alongside documentation and formatting improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as PushHandle
participant Queue as PushQueues
participant DLQ as DeadLetterQueue (DLQ)
participant Consumer
Producer->>Queue: try_push(frame, priority, policy)
alt Queue not full
Queue-->>Consumer: frame accepted
else Queue full and DLQ configured
alt Policy is DropIfFull or WarnAndDropIfFull
Producer->>DLQ: send dropped frame
alt DLQ full
DLQ-->>Producer: log error (DLQ full)
else
DLQ-->>Producer: frame accepted by DLQ
end
else
Producer-->>Producer: frame dropped (no DLQ)
end
end
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The DLQ routing only logs on
TrySendError::Fullbut ignoresTrySendError::Closed; handle or log the closed case to avoid silently dropping frames when the DLQ is disconnected. - Consider extracting the DLQ routing logic into its own helper function to simplify the
try_pushmatch arm and improve readability by avoiding nestedif letpatterns. - The
bounded_with_rate_dlqbuilder panics on an invalid rate viaassert!; consider returning aResultor explicit error instead of panicking for safer configuration handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The DLQ routing only logs on `TrySendError::Full` but ignores `TrySendError::Closed`; handle or log the closed case to avoid silently dropping frames when the DLQ is disconnected.
- Consider extracting the DLQ routing logic into its own helper function to simplify the `try_push` match arm and improve readability by avoiding nested `if let` patterns.
- The `bounded_with_rate_dlq` builder panics on an invalid rate via `assert!`; consider returning a `Result` or explicit error instead of panicking for safer configuration handling.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/asynchronous-outbound-messaging-roadmap.md(1 hunks)docs/rust-testing-with-rstest-fixtures.md(3 hunks)docs/wireframe-1-0-detailed-development-roadmap.md(1 hunks)src/app.rs(6 hunks)src/push.rs(7 hunks)tests/push_policies.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
`docs/**/*.md`: Documentation must use en-GB-oxendict spelling and grammar (with the exception of "license" which is to be left unchanged for community consistency).
docs/**/*.md: Documentation must use en-GB-oxendict spelling and grammar (with the exception of "license" which is to be left unchanged for community consistency).
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.mddocs/wireframe-1-0-detailed-development-roadmap.mddocs/rust-testing-with-rstest-fixtures.md
`**/*.md`: Validate Markdown files using `markdownlint *.md **/*.md`. Run `mdfor...
**/*.md: Validate Markdown files usingmarkdownlint *.md **/*.md.
Runmdformat-allafter any documentation changes to format all Markdown files and fix table markup.
Validate Markdown Mermaid diagrams using thenixieCLI. The tool is already installed; runnixie *.md **/*.mddirectly instead of usingnpx.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.mddocs/wireframe-1-0-detailed-development-roadmap.mddocs/rust-testing-with-rstest-fixtures.md
`docs/**/*.md`: Provide user guides and examples demonstrating server-initiated messaging.
docs/**/*.md: Provide user guides and examples demonstrating server-initiated messaging.
📄 Source: CodeRabbit Inference Engine (docs/asynchronous-outbound-messaging-roadmap.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.mddocs/wireframe-1-0-detailed-development-roadmap.mddocs/rust-testing-with-rstest-fixtures.md
`docs/**/*.md`: Conventions for writing project documentation should follow the rules outlined in the documentation style guide.
docs/**/*.md: Conventions for writing project documentation should follow the rules outlined in the documentation style guide.
📄 Source: CodeRabbit Inference Engine (docs/contents.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.mddocs/wireframe-1-0-detailed-development-roadmap.mddocs/rust-testing-with-rstest-fixtures.md
`docs/**/*.md`: Use British English based on the Oxford English Dictionary (en-o...
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation.
Company names are treated as collective nouns (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 use fenced code blocks with a language identifier; 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 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 footnotes referenced with[^label]in documentation.
Include Mermaid diagrams in documentation where it adds clarity.
When embedding figures in documentation, useand provide concise alt text describing the content.
Add a short description before each Mermaid diagram in documentation so screen readers can understand it.
📄 Source: CodeRabbit Inference Engine (docs/documentation-style-guide.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.mddocs/wireframe-1-0-detailed-development-roadmap.mddocs/rust-testing-with-rstest-fixtures.md
`docs/**/*.md`: Write the official documentation for the new features. Create se...
docs/**/*.md: 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.
📄 Source: CodeRabbit Inference Engine (docs/wireframe-1-0-detailed-development-roadmap.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.mddocs/wireframe-1-0-detailed-development-roadmap.mddocs/rust-testing-with-rstest-fixtures.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.mddocs/wireframe-1-0-detailed-development-roadmap.mddocs/rust-testing-with-rstest-fixtures.md
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/push_policies.rssrc/app.rssrc/push.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.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 must use en-GB-oxendict 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/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/push_policies.rssrc/app.rssrc/push.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: leynos/wireframe#0
File: docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md:0-0
Timestamp: 2025-06-30T23:08:36.501Z
Learning: Applies to docs/src/**/*.rs : For push mechanisms, offer an optional Dead Letter Queue (DLQ) pattern: if a push fails due to a full queue, route the frame to a separate channel for later inspection or reprocessing.
docs/rust-testing-with-rstest-fixtures.md (11)
Learnt from: CR
PR: leynos/femtologging#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-25T00:05:36.858Z
Learning: Compared to standard Rust #[test], rstest provides declarative fixture injection and parameterization, reducing boilerplate and improving test clarity, especially for complex setups and input combinations.
Learnt from: CR
PR: leynos/mxd#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-25T23:40:09.111Z
Learning: In Rust, the rstest crate provides a declarative, macro-based approach to fixture-based and parameterized testing, reducing boilerplate and improving test readability.
Learnt from: CR
PR: leynos/ddlint#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-28T15:32:52.327Z
Learning: For small, simple unit tests with no shared setup or parameterization, standard #[test] may suffice; rstest is most beneficial for complex setups, parameterized testing, and DRY test code.
Learnt from: CR
PR: leynos/lille#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-24T18:32:30.955Z
Learning: rstest is most beneficial for complex setups, parameterized testing, and when aiming for readable, DRY, and maintainable test suites; for trivial tests, standard #[test] may suffice.
Learnt from: CR
PR: leynos/lille#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-07-07T22:05:30.492Z
Learning: Use `rstest` for tests with complex setup, parameterization, or where improved readability and reduced boilerplate are desired; for simple unit tests, standard `#[test]` may suffice.
Learnt from: CR
PR: leynos/femtologging#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-26T00:20:29.033Z
Learning: In Rust, the rstest crate enables declarative fixture-based and parameterized testing using procedural macros like #[rstest] and #[fixture], which inject dependencies as function arguments, improving readability and reducing boilerplate.
Learnt from: CR
PR: leynos/femtologging#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-25T00:05:37.557Z
Learning: rstest is most beneficial for complex setups, parameterized testing, and when aiming for readable, DRY, and maintainable test suites; for simple unit tests, standard #[test] may suffice.
Learnt from: CR
PR: leynos/mdtablefix#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-24T23:09:43.342Z
Learning: Standard Rust #[test] functions require manual setup and parameterization, leading to more boilerplate and less readable tests compared to rstest's declarative approach.
Learnt from: CR
PR: leynos/ddlint#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-28T15:32:52.327Z
Learning: In Rust, the rstest crate enables fixture-based and parameterized testing using procedural macros like #[rstest] and #[fixture], allowing dependencies to be injected as function arguments for improved readability and reduced boilerplate.
Learnt from: CR
PR: leynos/femtologging#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-25T00:05:36.857Z
Learning: In Rust, the rstest crate enables fixture-based and parameterized testing using procedural macros like #[rstest] and #[fixture], allowing dependencies to be injected as function arguments for improved readability and reduced boilerplate.
Learnt from: CR
PR: leynos/wireframe#0
File: docs/rust-testing-with-rstest-fixtures.md:0-0
Timestamp: 2025-06-30T23:08:10.890Z
Learning: Applies to docs/**/*.rs : Use the `#[rstest]` attribute to mark test functions that utilize fixture injection or parameterization.
🪛 LanguageTool
docs/wireframe-1-0-detailed-development-roadmap.md
[grammar] ~23-~23: The verb ‘depend’ requires the preposition ‘on’ (or ‘upon’).
Context: ... | Size | Depends | | ------ | --------------------------...
(DEPEND_ON)
[typographical] ~27-~27: To join two clauses or introduce examples, consider using an em dash.
Context: .... | Small | - | | 1.2 | Priority Push Channel...
(DASH_RULE)
[typographical] ~28-~28: To join two clauses or introduce examples, consider using an em dash.
Context: ... | Medium | - | | 1.3 | Connection Actor Writ...
(DASH_RULE)
[style] ~29-~29: Consider using the typographical ellipsis character here instead.
Context: ...stateful connection actors. Implement a select!(biased; ...) loop that polls for shutdown signals,...
(ELLIPSIS)
[uncategorized] ~29-~29: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...p that polls for shutdown signals, high/low priority pushes and the handler response stream ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[typographical] ~30-~30: To join two clauses or introduce examples, consider using an em dash.
Context: ... | Medium | - | | 1.5 | Basic FragmentAdapter...
(DASH_RULE)
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...for a single, non-multiplexed stream of fragments and the outbound logic for splitting a ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~81-~81: Do not mix variants of the same word (‘finalise’ and ‘finalize’) within a single text.
Context: ...Changelog & 1.0 Release | Finalise the CHANGELOG.md with a comprehensive...
(EN_WORD_COHERENCY)
docs/rust-testing-with-rstest-fixtures.md
[style] ~1174-~1174: Consider using the typographical ellipsis character here instead.
Context: ...le distinct #[test] functions. | #[case(...)] attributes on #[rstest] function. ...
(ELLIPSIS)
[style] ~1175-~1175: Consider using the typographical ellipsis character here instead.
Context: ...complex manual generation. | #[values(...)] attributes on arguments of #[rstest] ...
(ELLIPSIS)
[misspelling] ~1340-~1340: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ... | Marks a function as an rstest test; enables fixture injection ...
(EN_A_VS_AN)
[style] ~1342-~1342: Consider using the typographical ellipsis character here instead.
Context: ...ervices). | | #[case(...)] | Defines a single pa...
(ELLIPSIS)
[style] ~1343-~1343: Consider using the typographical ellipsis character here instead.
Context: ...s. | | #[values(...)] | Defines a list of val...
(ELLIPSIS)
[style] ~1348-~1348: Consider using the typographical ellipsis character here instead.
Context: ...n. | | #[with(...)] | Overrides default a...
(ELLIPSIS)
[style] ~1349-~1349: Consider using the typographical ellipsis character here instead.
Context: ... | | #[default(...)] | Provides default value...
(ELLIPSIS)
[style] ~1350-~1350: Consider using the typographical ellipsis character here instead.
Context: ... | | #[timeout(...)] | Sets a timeout for an ...
(ELLIPSIS)
[style] ~1351-~1351: Consider using the typographical ellipsis character here instead.
Context: ... | | #[files("glob_pattern",...)] | Injects file paths (or contents, wi...
(ELLIPSIS)
⏰ Context from checks skipped due to timeout of 240000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (18)
docs/rust-testing-with-rstest-fixtures.md (1)
543-549: Documentation formatting improvements look excellent.The numbering corrections and table alignment enhance readability whilst maintaining the technical accuracy of the
rstestdocumentation.docs/wireframe-1-0-detailed-development-roadmap.md (1)
23-82: Table formatting improvements enhance readability.The proper header separators and column alignment make the roadmap tables much more readable whilst preserving all content.
docs/asynchronous-outbound-messaging-roadmap.md (1)
47-48: Correctly marks the DLQ feature as implemented.The roadmap update accurately reflects the completion of the optional Dead Letter Queue functionality, maintaining consistency between documentation and implementation status.
src/app.rs (4)
17-20: Clean import addition for DLQ functionality.The
tokio::sync::mpscimport is appropriately added to support the DLQ sender type.
88-88: Well-designed DLQ field integration.The
push_dlqfield is properly typed and fits well within theWireframeAppstructure. Making it optional maintains backwards compatibility.
401-418: Excellent builder method implementation.The
with_push_dlqmethod follows the established builder pattern perfectly. The documentation includes a practical code example, and the method signature is clean and intuitive.
365-365: Consistent field propagation across builder methods.The
push_dlqfield is properly propagated through all builder transformation methods (on_connection_setupandserializer), ensuring the DLQ configuration is maintained throughout the builder chain.Also applies to: 467-467
tests/push_policies.rs (4)
7-7: Appropriate imports for enhanced testing.The
serial_test::serialandtokio::sync::mpscimports support the new DLQ testing functionality and prevent test interference.Also applies to: 10-10
63-66: Excellent test isolation improvements.Adding
#[serial]attributes and logger clearing prevents race conditions and ensures clean test state. This follows testing best practices for shared resources.Also applies to: 85-88
109-124: Comprehensive DLQ routing test.The
dropped_frame_goes_to_dlqtest excellently verifies that dropped frames are correctly routed to the DLQ when the main queue is full. The test setup and assertions are clear and thorough.
126-155: Thorough DLQ overflow error handling test.The
dlq_full_logs_errortest properly validates error logging when the DLQ itself is full. The test correctly pre-fills the DLQ and verifies both the error log content and that no additional frames are accepted.src/push.rs (7)
72-72: Well-structured field addition for DLQ support.The optional DLQ sender field is appropriately typed and positioned within the struct, maintaining consistency with the existing pattern of optional features.
155-157: Clear documentation update for DLQ behaviour.The documentation accurately describes the new DLQ routing behaviour for drop policies, enhancing API clarity for users.
162-176: Comprehensive example demonstrating DLQ usage.The updated example clearly illustrates how dropped frames are routed to the DLQ, providing practical guidance for users implementing this feature.
240-240: Excellent backward compatibility preservation.The delegation to
bounded_with_rate_dlqwithNonefor DLQ maintains complete backward compatibility whilst enabling the new feature.
258-258: Consistent delegation pattern maintained.The no-rate-limit constructor properly delegates to the new method, preserving existing behaviour whilst enabling DLQ support.
285-291: Clean refactoring of existing constructor.The
bounded_with_ratemethod now cleanly delegates to the newbounded_with_rate_dlqmethod, maintaining the same API whilst enabling DLQ functionality.
293-361: Well-implemented DLQ constructor with comprehensive documentation.The new constructor method is well-designed with:
- Proper parameter validation matching existing patterns
- Clear documentation explaining DLQ behaviour
- Comprehensive example demonstrating usage
- Consistent field initialisation
The implementation maintains the same validation logic as existing constructors and properly initialises the DLQ field.
There was a problem hiding this comment.
Gates Failed
(2 files with Complex Conditional, Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| push_policies.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
| push.rs | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
| if let Some(r) = rate | ||
| && (r == 0 || r > MAX_PUSH_RATE) |
There was a problem hiding this comment.
❌ New issue: Complex Conditional
PushQueues.bounded_with_rate_dlq has 1 complex conditionals with 2 branches, threshold = 2
There was a problem hiding this comment.
Gates Failed
(2 files with Complex Conditional, Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| push_policies.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
| push.rs | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@sourcery-ai review |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/asynchronous-outbound-messaging-design.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
`docs/**/*.md`: Documentation must use en-GB-oxendict spelling and grammar (with the exception of "license" which is to be left unchanged for community consistency).
docs/**/*.md: Documentation must use en-GB-oxendict spelling and grammar (with the exception of "license" which is to be left unchanged for community consistency).
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`**/*.md`: Validate Markdown files using `markdownlint *.md **/*.md`. Run `mdfor...
**/*.md: Validate Markdown files usingmarkdownlint *.md **/*.md.
Runmdformat-allafter any documentation changes to format all Markdown files and fix table markup.
Validate Markdown Mermaid diagrams using thenixieCLI. The tool is already installed; runnixie *.md **/*.mddirectly instead of usingnpx.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`docs/**/*.md`: Provide user guides and examples demonstrating server-initiated messaging.
docs/**/*.md: Provide user guides and examples demonstrating server-initiated messaging.
📄 Source: CodeRabbit Inference Engine (docs/asynchronous-outbound-messaging-roadmap.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`docs/**/*.md`: Conventions for writing project documentation should follow the rules outlined in the documentation style guide.
docs/**/*.md: Conventions for writing project documentation should follow the rules outlined in the documentation style guide.
📄 Source: CodeRabbit Inference Engine (docs/contents.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`docs/**/*.md`: Use British English based on the Oxford English Dictionary (en-o...
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation.
Company names are treated as collective nouns (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 use fenced code blocks with a language identifier; 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 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 footnotes referenced with[^label]in documentation.
Include Mermaid diagrams in documentation where it adds clarity.
When embedding figures in documentation, useand provide concise alt text describing the content.
Add a short description before each Mermaid diagram in documentation so screen readers can understand it.
📄 Source: CodeRabbit Inference Engine (docs/documentation-style-guide.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`docs/**/*.md`: Write the official documentation for the new features. Create se...
docs/**/*.md: 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.
📄 Source: CodeRabbit Inference Engine (docs/wireframe-1-0-detailed-development-roadmap.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: leynos/wireframe#0
File: docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md:0-0
Timestamp: 2025-06-30T23:08:36.501Z
Learning: Applies to docs/src/**/*.rs : For push mechanisms, offer an optional Dead Letter Queue (DLQ) pattern: if a push fails due to a full queue, route the frame to a separate channel for later inspection or reprocessing.
⏰ Context from checks skipped due to timeout of 240000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
| The following sequence diagram illustrates how frames are routed when a DLQ is | ||
| configured: | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap the intro line and add a <description> tag for accessibility compliance
Documentation guidelines require paragraphs to be wrapped at 80 columns and each
Mermaid diagram to be preceded by a short description wrapped in a <description>
tag for screen-reader support (see earlier diagrams in this file).
Suggested patch:
-The following sequence diagram illustrates how frames are routed when a DLQ is configured:
+<description>The diagram shows how a frame moves from the producer to the
+`PushHandle`, falls back to the Dead Letter Queue (DLQ) when the primary queue
+is full, and is ultimately logged if both queues are saturated.</description>
+
+The following sequence diagram illustrates how frames are routed when a DLQ is
+configured:📝 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.
| The following sequence diagram illustrates how frames are routed when a DLQ is | |
| configured: | |
| <description>The diagram shows how a frame moves from the producer to the | |
| `PushHandle`, falls back to the Dead Letter Queue (DLQ) when the primary queue | |
| is full, and is ultimately logged if both queues are saturated.</description> | |
| The following sequence diagram illustrates how frames are routed when a DLQ is | |
| configured: |
🤖 Prompt for AI Agents
In docs/asynchronous-outbound-messaging-design.md around lines 600 to 602, the
introductory line before the sequence diagram is not wrapped at 80 columns and
lacks a <description> tag for accessibility. Wrap the introductory text to
ensure no line exceeds 80 characters and add a <description> tag immediately
before the Mermaid diagram with a concise summary of the diagram's content to
support screen readers.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider adding #[must_use] to bounded_with_rate_dlq (and/or retain it on the rate-limited constructors) so callers don’t accidentally drop the returned queue and handle.
- The markdown tables in wireframe-1-0-detailed-development-roadmap.md look malformed—with extra separator rows—and the DLQ item is still unmarked; please clean up the separators and mark the DLQ row as completed.
- The let-chains in
if let Some(r) = rate && (r == 0 || r > MAX_PUSH_RATE)can be harder to parse; consider a nestedif let Some(r) = rate { if r == 0 || r > MAX_PUSH_RATE { … } }for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding #[must_use] to bounded_with_rate_dlq (and/or retain it on the rate-limited constructors) so callers don’t accidentally drop the returned queue and handle.
- The markdown tables in wireframe-1-0-detailed-development-roadmap.md look malformed—with extra separator rows—and the DLQ item is still unmarked; please clean up the separators and mark the DLQ row as completed.
- The let-chains in `if let Some(r) = rate && (r == 0 || r > MAX_PUSH_RATE)` can be harder to parse; consider a nested `if let Some(r) = rate { if r == 0 || r > MAX_PUSH_RATE { … } }` for clarity.
## Individual Comments
### Comment 1
<location> `tests/push.rs:59` </location>
<code_context>
#[tokio::test]
async fn rate_limiter_blocks_when_exceeded(#[case] priority: PushPriority) {
time::pause();
- let (mut queues, handle) = PushQueues::bounded_with_rate(2, 2, Some(1));
+ let (mut queues, handle) = PushQueues::bounded_with_rate(2, 2, Some(1)).unwrap();
match priority {
</code_context>
<issue_to_address>
Update to handle Result from new API is correct.
Please add a test that covers the error case for invalid rate values, such as zero or values above MAX_PUSH_RATE, to ensure error handling is tested.
Suggested implementation:
```rust
#[tokio::test]
async fn rate_limiter_blocks_when_exceeded(#[case] priority: PushPriority) {
time::pause();
let (mut queues, handle) = PushQueues::bounded_with_rate(2, 2, Some(1)).unwrap();
match priority {
PushPriority::High => handle.push_high_priority(1u8).await.unwrap(),
```
```rust
#[tokio::test]
async fn rate_limiter_allows_after_wait() {
time::pause();
let (mut queues, handle) = PushQueues::bounded_with_rate(2, 2, Some(1)).unwrap();
handle.push_high_priority(1u8).await.unwrap();
time::advance(Duration::from_secs(1)).await;
handle.push_high_priority(2u8).await.unwrap();
}
#[test]
fn rate_limiter_invalid_rate_returns_error() {
// Test zero rate
let result = PushQueues::bounded_with_rate(2, 2, Some(0));
assert!(result.is_err(), "Expected error for zero rate");
// Test rate above MAX_PUSH_RATE
let above_max = MAX_PUSH_RATE + 1;
let result = PushQueues::bounded_with_rate(2, 2, Some(above_max));
assert!(result.is_err(), "Expected error for rate above MAX_PUSH_RATE");
}
```
- Ensure that `MAX_PUSH_RATE` is accessible in this test module. If not, you may need to import or define it appropriately.
- If `PushQueues::bounded_with_rate` returns a custom error type, you may want to assert on the error variant for more precise testing.
</issue_to_address>
### Comment 2
<location> `src/push.rs:365` </location>
<code_context>
+ /// assert_eq!(dlq_rx.recv().await.unwrap(), 2);
+ /// }
+ /// ```
+ pub fn bounded_with_rate_dlq(
+ high_capacity: usize,
+ low_capacity: usize,
</code_context>
<issue_to_address>
Consider introducing a builder pattern to unify queue constructors and inline DLQ routing for simpler, more extensible code.
```markdown
You’ve correctly centralized all of the “real” work into `bounded_with_rate_dlq` and `try_push`, but we can collapse most of the surface‐level APIs and remove the extra `route_to_dlq` indirection—and make extension easier in the future—by introducing a small builder.
1) Define a `PushQueuesBuilder`, drop `route_to_dlq()`, and inline DLQ routing in `try_push`:
```rust
pub struct PushQueuesBuilder<F> {
high_capacity: usize,
low_capacity: usize,
rate: Option<usize>,
dlq: Option<mpsc::Sender<F>>,
}
impl<F: FrameLike> PushQueuesBuilder<F> {
pub fn new(high_capacity: usize, low_capacity: usize) -> Self {
Self { high_capacity, low_capacity, rate: None, dlq: None }
}
pub fn with_rate(mut self, rate: usize) -> Self {
self.rate = Some(rate);
self
}
pub fn with_dlq(mut self, dlq_tx: mpsc::Sender<F>) -> Self {
self.dlq = Some(dlq_tx);
self
}
pub fn build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> {
// reuse your existing bounded_with_rate_dlq logic:
PushQueues::bounded_with_rate_dlq(
self.high_capacity,
self.low_capacity,
self.rate,
self.dlq,
)
}
}
```
2) Replace your four public constructors with wrappers around the builder:
```rust
impl<F: FrameLike> PushQueues<F> {
/// no rate, no DLQ
pub fn bounded(high: usize, low: usize) -> (Self, PushHandle<F>) {
PushQueuesBuilder::new(high, low)
.with_rate(DEFAULT_PUSH_RATE)
.build()
.expect("DEFAULT_PUSH_RATE is valid")
}
pub fn bounded_no_rate_limit(high: usize, low: usize) -> (Self, PushHandle<F>) {
PushQueuesBuilder::new(high, low)
.build()
.unwrap()
}
pub fn bounded_with_rate(
high: usize,
low: usize,
rate: usize,
) -> Result<(Self, PushHandle<F>), PushConfigError> {
PushQueuesBuilder::new(high, low).with_rate(rate).build()
}
pub fn bounded_with_rate_dlq(
high: usize,
low: usize,
rate: Option<usize>,
dlq: Option<mpsc::Sender<F>>,
) -> Result<(Self, PushHandle<F>), PushConfigError> {
PushQueuesBuilder::new(high, low).with_rate_opt(rate)?.with_dlq_opt(dlq).build()
}
}
```
3) Inline the DLQ routing inside `try_push` and remove `route_to_dlq`:
```rust
pub fn try_push(
&self,
frame: F,
priority: PushPriority,
policy: PushPolicy,
) -> Result<(), PushError> {
let tx = match priority {
PushPriority::High => &self.0.high_prio_tx,
PushPriority::Low => &self.0.low_prio_tx,
};
match tx.try_send(frame) {
Ok(()) => Ok(()),
Err(mpsc::error::TrySendError::Full(f)) => {
if matches!(policy, PushPolicy::WarnAndDropIfFull) {
log::warn!("push queue full; dropping {:?}", priority);
}
if let Some(dlq) = &self.0.dlq_tx {
if let Err(e) = dlq.try_send(f) {
log::error!("DLQ send failed ({:?}); frame lost", e);
}
}
match policy {
PushPolicy::ReturnErrorIfFull => Err(PushError::QueueFull),
_ => Ok(()),
}
}
Err(mpsc::error::TrySendError::Closed(_)) => Err(PushError::Closed),
}
}
```
This:
- Collapses four entry points into a single builder + `build()`
- Removes `route_to_dlq()` and its nesting
- Keeps all existing behavior and adds one clear extension point for future options
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| #[tokio::test] | ||
| async fn rate_limiter_blocks_when_exceeded(#[case] priority: PushPriority) { | ||
| time::pause(); | ||
| let (mut queues, handle) = PushQueues::bounded_with_rate(2, 2, Some(1)); |
There was a problem hiding this comment.
suggestion (testing): Update to handle Result from new API is correct.
Please add a test that covers the error case for invalid rate values, such as zero or values above MAX_PUSH_RATE, to ensure error handling is tested.
Suggested implementation:
#[tokio::test]
async fn rate_limiter_blocks_when_exceeded(#[case] priority: PushPriority) {
time::pause();
let (mut queues, handle) = PushQueues::bounded_with_rate(2, 2, Some(1)).unwrap();
match priority {
PushPriority::High => handle.push_high_priority(1u8).await.unwrap(),#[tokio::test]
async fn rate_limiter_allows_after_wait() {
time::pause();
let (mut queues, handle) = PushQueues::bounded_with_rate(2, 2, Some(1)).unwrap();
handle.push_high_priority(1u8).await.unwrap();
time::advance(Duration::from_secs(1)).await;
handle.push_high_priority(2u8).await.unwrap();
}
#[test]
fn rate_limiter_invalid_rate_returns_error() {
// Test zero rate
let result = PushQueues::bounded_with_rate(2, 2, Some(0));
assert!(result.is_err(), "Expected error for zero rate");
// Test rate above MAX_PUSH_RATE
let above_max = MAX_PUSH_RATE + 1;
let result = PushQueues::bounded_with_rate(2, 2, Some(above_max));
assert!(result.is_err(), "Expected error for rate above MAX_PUSH_RATE");
}- Ensure that
MAX_PUSH_RATEis accessible in this test module. If not, you may need to import or define it appropriately. - If
PushQueues::bounded_with_ratereturns a custom error type, you may want to assert on the error variant for more precise testing.
| /// assert_eq!(dlq_rx.recv().await.unwrap(), 2); | ||
| /// } | ||
| /// ``` | ||
| pub fn bounded_with_rate_dlq( |
There was a problem hiding this comment.
issue (complexity): Consider introducing a builder pattern to unify queue constructors and inline DLQ routing for simpler, more extensible code.
You’ve correctly centralized all of the “real” work into `bounded_with_rate_dlq` and `try_push`, but we can collapse most of the surface‐level APIs and remove the extra `route_to_dlq` indirection—and make extension easier in the future—by introducing a small builder.
1) Define a `PushQueuesBuilder`, drop `route_to_dlq()`, and inline DLQ routing in `try_push`:
```rust
pub struct PushQueuesBuilder<F> {
high_capacity: usize,
low_capacity: usize,
rate: Option<usize>,
dlq: Option<mpsc::Sender<F>>,
}
impl<F: FrameLike> PushQueuesBuilder<F> {
pub fn new(high_capacity: usize, low_capacity: usize) -> Self {
Self { high_capacity, low_capacity, rate: None, dlq: None }
}
pub fn with_rate(mut self, rate: usize) -> Self {
self.rate = Some(rate);
self
}
pub fn with_dlq(mut self, dlq_tx: mpsc::Sender<F>) -> Self {
self.dlq = Some(dlq_tx);
self
}
pub fn build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> {
// reuse your existing bounded_with_rate_dlq logic:
PushQueues::bounded_with_rate_dlq(
self.high_capacity,
self.low_capacity,
self.rate,
self.dlq,
)
}
}- Replace your four public constructors with wrappers around the builder:
impl<F: FrameLike> PushQueues<F> {
/// no rate, no DLQ
pub fn bounded(high: usize, low: usize) -> (Self, PushHandle<F>) {
PushQueuesBuilder::new(high, low)
.with_rate(DEFAULT_PUSH_RATE)
.build()
.expect("DEFAULT_PUSH_RATE is valid")
}
pub fn bounded_no_rate_limit(high: usize, low: usize) -> (Self, PushHandle<F>) {
PushQueuesBuilder::new(high, low)
.build()
.unwrap()
}
pub fn bounded_with_rate(
high: usize,
low: usize,
rate: usize,
) -> Result<(Self, PushHandle<F>), PushConfigError> {
PushQueuesBuilder::new(high, low).with_rate(rate).build()
}
pub fn bounded_with_rate_dlq(
high: usize,
low: usize,
rate: Option<usize>,
dlq: Option<mpsc::Sender<F>>,
) -> Result<(Self, PushHandle<F>), PushConfigError> {
PushQueuesBuilder::new(high, low).with_rate_opt(rate)?.with_dlq_opt(dlq).build()
}
}- Inline the DLQ routing inside
try_pushand removeroute_to_dlq:
pub fn try_push(
&self,
frame: F,
priority: PushPriority,
policy: PushPolicy,
) -> Result<(), PushError> {
let tx = match priority {
PushPriority::High => &self.0.high_prio_tx,
PushPriority::Low => &self.0.low_prio_tx,
};
match tx.try_send(frame) {
Ok(()) => Ok(()),
Err(mpsc::error::TrySendError::Full(f)) => {
if matches!(policy, PushPolicy::WarnAndDropIfFull) {
log::warn!("push queue full; dropping {:?}", priority);
}
if let Some(dlq) = &self.0.dlq_tx {
if let Err(e) = dlq.try_send(f) {
log::error!("DLQ send failed ({:?}); frame lost", e);
}
}
match policy {
PushPolicy::ReturnErrorIfFull => Err(PushError::QueueFull),
_ => Ok(()),
}
}
Err(mpsc::error::TrySendError::Closed(_)) => Err(PushError::Closed),
}
}This:
- Collapses four entry points into a single builder +
build() - Removes
route_to_dlq()and its nesting - Keeps all existing behavior and adds one clear extension point for future options
There was a problem hiding this comment.
Gates Failed
(1 file with Complex Conditional)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| push.rs | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/push_policies.rs (1)
144-186: Excellent implementation addressing code duplication concerns.The parameterised test using
rstesteffectively eliminates the code duplication mentioned in the past review comments whilst maintaining comprehensive test coverage for both DLQ full and DLQ closed scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/push_policies.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/push_policies.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.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 must use en-GB-oxendict 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/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/push_policies.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: leynos/wireframe#0
File: docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md:0-0
Timestamp: 2025-06-30T23:08:36.501Z
Learning: Applies to docs/src/**/*.rs : For push mechanisms, offer an optional Dead Letter Queue (DLQ) pattern: if a push fails due to a full queue, route the frame to a separate channel for later inspection or reprocessing.
⏰ Context from checks skipped due to timeout of 240000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (5)
tests/push_policies.rs (5)
5-11: Import additions look appropriate for the new DLQ functionality.The new imports support the async helper functions (
BoxFuture) and test serialisation (serial), which are necessary for the DLQ testing implementation.
64-67: Good practice: Adding test serialisation and logger clearing.The
#[serial]attribute prevents race conditions with the shared logger state, and the logger clearing ensures clean test state between runs.
86-89: Consistent application of serialisation pattern.The same logger serialisation and clearing pattern is correctly applied to maintain test isolation.
128-142: Add documentation for helper functions.The helper functions lack documentation explaining their purpose and usage, which violates the coding guidelines requiring clear documentation for functions.
+/// Sets up a DLQ sender in a full state by pre-filling it with a test frame. +/// This simulates the condition where the DLQ cannot accept additional frames. fn setup_dlq_full(tx: &mpsc::Sender<u8>, _rx: &mut Option<mpsc::Receiver<u8>>) { tx.try_send(99).unwrap(); } +/// Sets up a DLQ in a closed state by dropping the receiver. +/// This simulates the condition where the DLQ is no longer accepting frames. fn setup_dlq_closed(_: &mpsc::Sender<u8>, rx: &mut Option<mpsc::Receiver<u8>>) { drop(rx.take()); } +/// Asserts that the DLQ received the pre-filled frame and cannot receive more. +/// Verifies the DLQ full condition by checking frame reception and queue state. fn assert_dlq_full(rx: &mut Option<mpsc::Receiver<u8>>) -> BoxFuture<'_, ()> { Box::pin(async move { let receiver = rx.as_mut().expect("receiver missing"); assert_eq!(receiver.recv().await.unwrap(), 99); assert!(receiver.try_recv().is_err()); }) } +/// No-op assertion for the DLQ closed scenario. +/// When the DLQ is closed, there are no additional assertions to perform. fn assert_dlq_closed(_: &mut Option<mpsc::Receiver<u8>>) -> BoxFuture<'_, ()> { Box::pin(async {}) }Likely an incorrect or invalid review comment.
110-126: Existing tests already coverWarnAndDropIfFullDLQ routingThe
WarnAndDropIfFullpolicy is exercised in tests/push_policies.rs, so no additional test is required:
- Line 93:
.try_push(4u8, PushPriority::Low, PushPolicy::WarnAndDropIfFull)- Line 145: rstest case
dlq_fullforPushPolicy::WarnAndDropIfFull, which asserts routing to the DLQPlease disregard the suggestion to add another DLQ test for
WarnAndDropIfFull.Likely an incorrect or invalid review comment.
| Setup: FnOnce(&mpsc::Sender<u8>, &mut Option<mpsc::Receiver<u8>>), | ||
| AssertFn: FnOnce(&mut Option<mpsc::Receiver<u8>>) -> BoxFuture<'_, ()>, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider simplifying the generic constraints.
The current approach using BoxFuture adds complexity. Consider using async closures or a simpler approach that doesn't require explicit lifetime management.
// Alternative approach using async closures
where
Setup: FnOnce(&mpsc::Sender<u8>, &mut Option<mpsc::Receiver<u8>>),
AssertFn: for<'a> FnOnce(&'a mut Option<mpsc::Receiver<u8>>) -> impl std::future::Future<Output = ()> + 'a,🤖 Prompt for AI Agents
In tests/push_policies.rs around lines 156 to 157, the generic constraint for
AssertFn uses BoxFuture which adds unnecessary complexity and explicit lifetime
management. Replace the BoxFuture return type with an async closure by changing
the constraint to use a higher-ranked trait bound with for<'a> and returning an
impl Future instead. This simplifies the code by leveraging async closures and
removes the need for boxing and explicit lifetimes.
There was a problem hiding this comment.
Please create a GitHub issue for this.
There was a problem hiding this comment.
🧩 Analysis chain
@leynos I'll create a GitHub issue for this suggestion to simplify the generic constraints.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "Simplify generic constraints in push policy tests" \
--body "A suggestion was made to simplify the generic constraints in the \`dlq_error_scenarios\` function in \`tests/push_policies.rs\` around lines 156-157.
The current approach using \`BoxFuture\` adds complexity and explicit lifetime management. The suggestion is to replace the \`BoxFuture\` return type with async closures using higher-ranked trait bounds (for<'a>) and returning an impl Future instead.
This would simplify the code by leveraging async closures and removing the need for boxing and explicit lifetimes.
**References:**
- PR: https://github.com/leynos/wireframe/pull/198
- Comment: https://github.com/leynos/wireframe/pull/198#discussion_r2195929726
- Requested by: @leynos" \
--assignee leynos
Length of output: 836
@leynos I've successfully created GitHub issue #199 for the suggestion to simplify the generic constraints in the push policy tests. The issue includes details about replacing BoxFuture with async closures and has been assigned to you.
Issue URL: #199
🧬 Code Graph Analysis Results
Relevant snippets from tests/connection_actor.rs (lines 372-381, 387, 391, 399)
fn new() -> Self {
static LOGGER: OnceLock<Mutex<Logger>> = OnceLock::new();
let logger = LOGGER.get_or_init(|| Mutex::new(Logger::start()));
let guard = logger
.lock()
.expect("failed to acquire global logger lock; a previous test may still hold it");
Self { guard }
}
fn deref(&self) -> &Self::Target { &self.guard }
fn deref_mut(&mut self) -> &mut Self::Target { &mut self.guard }
fn logger() -> LoggerHandle { LoggerHandle::new() }These snippets define a LoggerHandle struct with methods to create a global logger instance guarded by a mutex, and implement Deref and DerefMut traits for convenient access. They also provide a fixture function logger() returning a LoggerHandle. This is essentially the same pattern used in tests/push_policies.rs for managing the global logger.
Relevant snippets from src/push.rs (lines 274-277, 365-399, 419-425)
pub fn bounded(high_capacity: usize, low_capacity: usize) -> (Self, PushHandle<F>) {
Self::bounded_with_rate_dlq(high_capacity, low_capacity, Some(DEFAULT_PUSH_RATE), None)
.expect("DEFAULT_PUSH_RATE is always valid")
}
pub fn bounded_with_rate_dlq(
high_capacity: usize,
low_capacity: usize,
rate: Option<usize>,
dlq: Option<mpsc::Sender<F>>,
) -> Result<(Self, PushHandle<F>), PushConfigError> {
if let Some(r) = rate
&& (r == 0 || r > MAX_PUSH_RATE)
{
return Err(PushConfigError::InvalidRate(r));
}
let (high_tx, high_rx) = mpsc::channel(high_capacity);
let (low_tx, low_rx) = mpsc::channel(low_capacity);
let limiter = rate.map(|r| {
RateLimiter::builder()
.initial(r)
.refill(r)
.interval(Duration::from_secs(1))
.max(r)
.build()
});
let inner = PushHandleInner {
high_prio_tx: high_tx,
low_prio_tx: low_tx,
limiter,
dlq_tx: dlq,
};
Ok((
Self {
high_priority_rx: high_rx,
low_priority_rx: low_rx,
},
PushHandle(Arc::new(inner)),
))
}
pub async fn recv(&mut self) -> Option<(PushPriority, F)> {
tokio::select! {
biased;
res = self.high_priority_rx.recv() => res.map(|f| (PushPriority::High, f)),
res = self.low_priority_rx.recv() => res.map(|f| (PushPriority::Low, f)),
}
}These snippets define the PushQueues and PushHandle creation and behavior:
boundedcreates push queues with default rate limiting and no dead-letter queue (DLQ).bounded_with_rate_dlqcreates push queues with optional rate limiting and optional DLQ sender. It validates the rate and sets up channels for high and low priority.recvasynchronously receives from the high priority queue first, then low priority queue.
This is directly relevant to the tests in tests/push_policies.rs which create bounded push queues and test pushing behavior, including DLQ handling.
Summary
The relevant code snippets provide:
- The global logger setup and locking mechanism used in the tests.
- The construction and behavior of the push queues and handles, including bounded queues, rate limiting, and DLQ support.
- The asynchronous receiving of pushed items from the queues.
These are essential to understand the test file tests/push_policies.rs and the behavior it verifies regarding push policies and DLQ handling.
There was a problem hiding this comment.
Gates Failed
(2 files with Complex Conditional, Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| push_policies.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
| push.rs | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@sourcery-ai review |
There was a problem hiding this comment.
Gates Failed
(1 file with Complex Conditional)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| push.rs | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Add behavioural and unit tests for the new Dead Letter Queue (DLQ) feature in PushHandle and PushQueues. (link)
General comments:
- The
boundedandbounded_no_rate_limitconstructors still panic viaunwrap/expecton invalid defaults—consider making them return aResultor explicitly documenting these panics so callers aren’t surprised by hidden panics. - The new
route_to_dlqmethod swallows the DLQ send result; consider exposing aResultor flag in the public API so callers (or metrics) can know if a frame actually made it into the dead letter queue. - The
if let Some(r) = rate && (r == 0 || r > MAX_PUSH_RATE)let-chain requires Rust 1.65+—make sure the crate’srust-versionis bumped accordingly or revert to a more compatible pattern.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `bounded` and `bounded_no_rate_limit` constructors still panic via `unwrap`/`expect` on invalid defaults—consider making them return a `Result` or explicitly documenting these panics so callers aren’t surprised by hidden panics.
- The new `route_to_dlq` method swallows the DLQ send result; consider exposing a `Result` or flag in the public API so callers (or metrics) can know if a frame actually made it into the dead letter queue.
- The `if let Some(r) = rate && (r == 0 || r > MAX_PUSH_RATE)` let-chain requires Rust 1.65+—make sure the crate’s `rust-version` is bumped accordingly or revert to a more compatible pattern.
## Individual Comments
### Comment 1
<location> `src/push.rs:298` </location>
<code_context>
low_capacity: usize,
) -> (Self, PushHandle<F>) {
- Self::bounded_with_rate(high_capacity, low_capacity, None)
+ Self::bounded_with_rate_dlq(high_capacity, low_capacity, None, None).unwrap()
}
</code_context>
<issue_to_address>
Unwrapping here could panic if future changes introduce invalid parameters.
While safe now, future changes could make this unwrap unsafe. Please handle the error explicitly or document the required invariants.
Suggested implementation:
```rust
///
/// # Errors
///
/// Returns [`PushConfigError`] if the provided parameters are invalid.
#[must_use]
pub fn bounded_no_rate_limit(
high_capacity: usize,
low_capacity: usize,
) -> Result<(Self, PushHandle<F>), PushConfigError> {
Self::bounded_with_rate_dlq(high_capacity, low_capacity, None, None)
}
```
You will need to update all call sites of `bounded_no_rate_limit` to handle the new `Result` return type, either by propagating the error or handling it appropriately.
</issue_to_address>
### Comment 2
<location> `src/push.rs:274` </location>
<code_context>
+ ///
+ /// Panics if an internal invariant is violated. This should never occur.
#[must_use]
pub fn bounded(high_capacity: usize, low_capacity: usize) -> (Self, PushHandle<F>) {
- Self::bounded_with_rate(high_capacity, low_capacity, Some(DEFAULT_PUSH_RATE))
+ Self::bounded_with_rate_dlq(high_capacity, low_capacity, Some(DEFAULT_PUSH_RATE), None)
</code_context>
<issue_to_address>
Consider refactoring the four `bounded*` constructors into a single builder pattern with a rate validation helper to centralize logic and reduce duplication.
Consider collapsing the four `bounded*` constructors into a single builder + a tiny rate‐validator helper. That removes duplication, centralizes the `if let Some(r) && (r==0 || r>…)` logic, and keeps the DLQ wiring in one place. For example:
```rust
/// centralize rate checks
fn validate_rate(r: Option<usize>) -> Result<Option<usize>, PushConfigError> {
if let Some(n) = r {
if n == 0 || n > MAX_PUSH_RATE {
return Err(PushConfigError::InvalidRate(n));
}
}
Ok(r)
}
/// builder for all `bounded*` variants
pub struct PushQueuesBuilder<F> {
high: usize,
low: usize,
rate: Option<usize>,
dlq: Option<mpsc::Sender<F>>,
}
impl<F: FrameLike> PushQueuesBuilder<F> {
pub fn new(high: usize, low: usize) -> Self {
Self { high, low, rate: Some(DEFAULT_PUSH_RATE), dlq: None }
}
pub fn no_rate_limit(mut self) -> Self {
self.rate = None;
self
}
pub fn rate(mut self, r: usize) -> Self {
self.rate = Some(r);
self
}
pub fn dlq(mut self, dlq_tx: mpsc::Sender<F>) -> Self {
self.dlq = Some(dlq_tx);
self
}
pub fn build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> {
let rate = validate_rate(self.rate)?;
let (high_tx, high_rx) = mpsc::channel(self.high);
let (low_tx, low_rx) = mpsc::channel(self.low);
let limiter = rate.map(|r| {
RateLimiter::builder().initial(r).refill(r)
.interval(Duration::from_secs(1)).max(r).build()
});
let inner = PushHandleInner {
high_prio_tx: high_tx,
low_prio_tx: low_tx,
limiter,
dlq_tx: self.dlq,
};
Ok((
PushQueues { high_priority_rx: high_rx, low_priority_rx: low_rx },
PushHandle(Arc::new(inner)),
))
}
}
```
Then replace your four methods with thin wrappers:
```rust
impl<F: FrameLike> PushQueues<F> {
pub fn bounded(h: usize, l: usize)
-> (Self, PushHandle<F>)
{
PushQueuesBuilder::new(h, l)
.build().expect("DEFAULT_PUSH_RATE is valid")
}
pub fn bounded_no_rate_limit(h: usize, l: usize)
-> (Self, PushHandle<F>)
{
PushQueuesBuilder::new(h, l)
.no_rate_limit()
.build().unwrap()
}
pub fn bounded_with_rate(h: usize, l: usize, r: usize)
-> Result<(Self, PushHandle<F>), PushConfigError>
{
PushQueuesBuilder::new(h, l)
.rate(r)
.build()
}
pub fn bounded_with_rate_dlq(
h: usize, l: usize, r: Option<usize>, dlq: Option<mpsc::Sender<F>>
) -> Result<(Self, PushHandle<F>), PushConfigError> {
let mut b = PushQueuesBuilder::new(h, l);
if let Some(r) = r { b = b.rate(r) } else { b = b.no_rate_limit() }
if let Some(d) = dlq { b = b.dlq(d) }
b.build()
}
}
```
This keeps all public APIs, centralizes the “invalid rate” logic, and removes boilerplate from each overload.
</issue_to_address>
### Comment 3
<location> `src/push.rs:168` </location>
<code_context>
self.push_with_priority(frame, PushPriority::Low).await
}
+ /// Send a frame to the configured dead letter queue if available.
+ fn route_to_dlq(&self, frame: F) {
+ if let Some(dlq) = &self.0.dlq_tx {
</code_context>
<issue_to_address>
Add behavioural and unit tests for the new Dead Letter Queue (DLQ) feature in PushHandle and PushQueues.
The new DLQ routing logic in PushHandle and PushQueues is a significant feature addition. There must be both behavioural and unit tests demonstrating that frames are routed to the DLQ under the correct conditions (e.g., when queues are full and DropIfFull/WarnAndDropIfFull is used). Please add comprehensive tests covering these scenarios.
</issue_to_address>
### Comment 4
<location> `src/push.rs:71` </location>
<code_context>
+/// Errors returned when creating push queues.
+#[derive(Debug)]
+pub enum PushConfigError {
+ /// The provided rate was zero or exceeded [`MAX_PUSH_RATE`].
+ InvalidRate(usize),
+}
</code_context>
<issue_to_address>
Add tests for PushConfigError and error handling in PushQueues constructors.
The new PushConfigError type and error-returning constructors (e.g., bounded_with_rate, bounded_with_rate_dlq) require unit and behavioural tests to verify correct error handling for invalid rates. Please add tests that cover these error cases.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| low_capacity: usize, | ||
| ) -> (Self, PushHandle<F>) { | ||
| Self::bounded_with_rate(high_capacity, low_capacity, None) | ||
| Self::bounded_with_rate_dlq(high_capacity, low_capacity, None, None).unwrap() |
There was a problem hiding this comment.
suggestion (bug_risk): Unwrapping here could panic if future changes introduce invalid parameters.
While safe now, future changes could make this unwrap unsafe. Please handle the error explicitly or document the required invariants.
Suggested implementation:
///
/// # Errors
///
/// Returns [`PushConfigError`] if the provided parameters are invalid.
#[must_use]
pub fn bounded_no_rate_limit(
high_capacity: usize,
low_capacity: usize,
) -> Result<(Self, PushHandle<F>), PushConfigError> {
Self::bounded_with_rate_dlq(high_capacity, low_capacity, None, None)
}You will need to update all call sites of bounded_no_rate_limit to handle the new Result return type, either by propagating the error or handling it appropriately.
| /// | ||
| /// Panics if an internal invariant is violated. This should never occur. | ||
| #[must_use] | ||
| pub fn bounded(high_capacity: usize, low_capacity: usize) -> (Self, PushHandle<F>) { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the four bounded* constructors into a single builder pattern with a rate validation helper to centralize logic and reduce duplication.
Consider collapsing the four bounded* constructors into a single builder + a tiny rate‐validator helper. That removes duplication, centralizes the if let Some(r) && (r==0 || r>…) logic, and keeps the DLQ wiring in one place. For example:
/// centralize rate checks
fn validate_rate(r: Option<usize>) -> Result<Option<usize>, PushConfigError> {
if let Some(n) = r {
if n == 0 || n > MAX_PUSH_RATE {
return Err(PushConfigError::InvalidRate(n));
}
}
Ok(r)
}
/// builder for all `bounded*` variants
pub struct PushQueuesBuilder<F> {
high: usize,
low: usize,
rate: Option<usize>,
dlq: Option<mpsc::Sender<F>>,
}
impl<F: FrameLike> PushQueuesBuilder<F> {
pub fn new(high: usize, low: usize) -> Self {
Self { high, low, rate: Some(DEFAULT_PUSH_RATE), dlq: None }
}
pub fn no_rate_limit(mut self) -> Self {
self.rate = None;
self
}
pub fn rate(mut self, r: usize) -> Self {
self.rate = Some(r);
self
}
pub fn dlq(mut self, dlq_tx: mpsc::Sender<F>) -> Self {
self.dlq = Some(dlq_tx);
self
}
pub fn build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> {
let rate = validate_rate(self.rate)?;
let (high_tx, high_rx) = mpsc::channel(self.high);
let (low_tx, low_rx) = mpsc::channel(self.low);
let limiter = rate.map(|r| {
RateLimiter::builder().initial(r).refill(r)
.interval(Duration::from_secs(1)).max(r).build()
});
let inner = PushHandleInner {
high_prio_tx: high_tx,
low_prio_tx: low_tx,
limiter,
dlq_tx: self.dlq,
};
Ok((
PushQueues { high_priority_rx: high_rx, low_priority_rx: low_rx },
PushHandle(Arc::new(inner)),
))
}
}Then replace your four methods with thin wrappers:
impl<F: FrameLike> PushQueues<F> {
pub fn bounded(h: usize, l: usize)
-> (Self, PushHandle<F>)
{
PushQueuesBuilder::new(h, l)
.build().expect("DEFAULT_PUSH_RATE is valid")
}
pub fn bounded_no_rate_limit(h: usize, l: usize)
-> (Self, PushHandle<F>)
{
PushQueuesBuilder::new(h, l)
.no_rate_limit()
.build().unwrap()
}
pub fn bounded_with_rate(h: usize, l: usize, r: usize)
-> Result<(Self, PushHandle<F>), PushConfigError>
{
PushQueuesBuilder::new(h, l)
.rate(r)
.build()
}
pub fn bounded_with_rate_dlq(
h: usize, l: usize, r: Option<usize>, dlq: Option<mpsc::Sender<F>>
) -> Result<(Self, PushHandle<F>), PushConfigError> {
let mut b = PushQueuesBuilder::new(h, l);
if let Some(r) = r { b = b.rate(r) } else { b = b.no_rate_limit() }
if let Some(d) = dlq { b = b.dlq(d) }
b.build()
}
}This keeps all public APIs, centralizes the “invalid rate” logic, and removes boilerplate from each overload.
| self.push_with_priority(frame, PushPriority::Low).await | ||
| } | ||
|
|
||
| /// Send a frame to the configured dead letter queue if available. |
There was a problem hiding this comment.
issue (review_instructions): Add behavioural and unit tests for the new Dead Letter Queue (DLQ) feature in PushHandle and PushQueues.
The new DLQ routing logic in PushHandle and PushQueues is a significant feature addition. There must be both behavioural and unit tests demonstrating that frames are routed to the DLQ under the correct conditions (e.g., when queues are full and DropIfFull/WarnAndDropIfFull is used). Please add comprehensive tests covering these scenarios.
Review instructions:
Path patterns: **/*
Instructions:
For any new feature or change to an existing feature, both behavioural and unit tests are required.
| /// Errors returned when creating push queues. | ||
| #[derive(Debug)] | ||
| pub enum PushConfigError { | ||
| /// The provided rate was zero or exceeded [`MAX_PUSH_RATE`]. |
There was a problem hiding this comment.
issue (review_instructions): Add tests for PushConfigError and error handling in PushQueues constructors.
The new PushConfigError type and error-returning constructors (e.g., bounded_with_rate, bounded_with_rate_dlq) require unit and behavioural tests to verify correct error handling for invalid rates. Please add tests that cover these error cases.
Review instructions:
Path patterns: **/*
Instructions:
For any new feature or change to an existing feature, both behavioural and unit tests are required.
There was a problem hiding this comment.
Gates Failed
(1 file with Complex Conditional)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| push.rs | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/push_policies.rs (1)
91-99: DLQ assertion helpers complete the test infrastructure.These functions provide the necessary assertions for the DLQ error scenarios. Note that issue #199 tracks the suggestion to simplify the generic constraints using async closures instead of
BoxFuture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
tests/connection_actor.rs(1 hunks)tests/push_policies.rs(3 hunks)wireframe_testing/Cargo.toml(1 hunks)wireframe_testing/src/lib.rs(2 hunks)wireframe_testing/src/logging.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
wireframe_testing/src/lib.rstests/connection_actor.rswireframe_testing/src/logging.rstests/push_policies.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.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 must use en-GB-oxendict 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/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
wireframe_testing/src/lib.rstests/connection_actor.rswireframe_testing/src/logging.rstests/push_policies.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: leynos/wireframe#0
File: docs/wireframe-1-0-detailed-development-roadmap.md:0-0
Timestamp: 2025-06-29T18:04:58.493Z
Learning: An optional Dead Letter Queue (DLQ) mechanism should be provided, allowing failed pushes (due to a full queue) to be routed to a user-provided DLQ channel sender instead of being dropped.
Learnt from: CR
PR: leynos/wireframe#0
File: docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md:0-0
Timestamp: 2025-06-30T23:08:36.501Z
Learning: Applies to docs/src/**/*.rs : For push mechanisms, offer an optional Dead Letter Queue (DLQ) pattern: if a push fails due to a full queue, route the frame to a separate channel for later inspection or reprocessing.
🧬 Code Graph Analysis (3)
wireframe_testing/src/lib.rs (1)
wireframe_testing/src/logging.rs (1)
logger(41-41)
tests/connection_actor.rs (1)
wireframe_testing/src/logging.rs (1)
logger(41-41)
tests/push_policies.rs (2)
wireframe_testing/src/logging.rs (1)
logger(41-41)src/push.rs (3)
bounded(274-277)bounded_with_rate_dlq(365-399)recv(419-425)
🪛 GitHub Actions: CI
wireframe_testing/src/lib.rs
[warning] 32-32: Prettier formatting check failed: code style differences detected. Run 'cargo fmt --all -- --write' to fix code style issues.
wireframe_testing/src/logging.rs
[warning] 35-35: Prettier formatting check failed: code style differences detected. Run 'cargo fmt --all -- --write' to fix code style issues.
🪛 GitHub Check: build-test
wireframe_testing/src/logging.rs
[warning] 35-35:
Diff in /home/runner/work/wireframe/wireframe/wireframe_testing/src/logging.rs
🔇 Additional comments (9)
wireframe_testing/Cargo.toml (1)
12-12: LGTM!The addition of
logtestdependency is appropriate for the new logging test utilities.tests/connection_actor.rs (1)
362-362: Good refactoring!Removing the local logger implementation in favour of the centralized one from
wireframe_testingimproves maintainability and reduces code duplication.wireframe_testing/src/logging.rs (2)
6-24: Well-designed thread-safe logger implementation.The use of
OnceLockfor singleton initialization andMutexGuardfor exclusive access is appropriate for preventing test interference.
36-41: Clean fixture implementation.Good use of the
#[allow]attribute with a clear reason for the false positive.tests/push_policies.rs (5)
3-12: Imports aligned with DLQ functionality.The additional imports support the new DLQ tests and centralized logging.
26-65: Excellent refactoring using parameterized tests.The merged test eliminates duplication while maintaining comprehensive coverage of both push policies. Good use of
#[serial]to prevent log interference between tests.
67-83: Clear test for DLQ functionality.This test effectively validates that dropped frames are routed to the DLQ as expected.
85-89: Well-structured DLQ setup helpers.These functions clearly establish the DLQ error conditions needed for comprehensive testing.
101-143: Comprehensive DLQ error scenario testing.Excellent use of parameterized testing to cover both DLQ full and closed scenarios. The test validates error logging and maintains good separation of concerns with the helper functions.
There was a problem hiding this comment.
Gates Failed
(1 file with Complex Conditional)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| push.rs | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Gates Failed
(1 file with Complex Conditional)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| push.rs | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
Docstrings generation was requested by @leynos. * #198 (comment) The following files were modified: * `src/app.rs` * `src/push.rs` * `tests/push.rs` * `tests/push_policies.rs` * `wireframe_testing/src/logging.rs`
|
Note Generated docstrings for this pull request at #200 |
1 participant
Summary
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_686c5393d5bc8322b119c3a598ec70cf
Summary by Sourcery
Add optional dead letter queue (DLQ) support for push queues, including builder APIs, frame routing, error handling, documentation, and tests.
New Features:
Enhancements:
Build:
Documentation:
Tests: