Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDependencies bumped in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/trp/src/methods.rs (1)
499-506:⚠️ Potential issue | 🔴 CriticalTest will fail: wrong JSON key for the
includePayloadfield.Line 500 sends
"include_payload"(snake_case) as the JSON key, butPeekPendingParamsmaps the field via#[serde(rename = "includePayload")]. Without#[serde(deny_unknown_fields)], serde silently discards the unrecognised key, leavinginclude_payloadasNone→false. The assertion on line 506 that all payloads areSometherefore fails.🐛 Proposed fix
- let req = json!({ "include_payload": true }).to_string(); + let req = json!({ "includePayload": true }).to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trp/src/methods.rs` around lines 499 - 506, The test is using the wrong JSON key name for PeekPendingParams' includePayload field; update the request payload sent to trp_peek_pending to use the exact serde name "includePayload" (not "include_payload") so PeekPendingParams (used by trp_peek_pending) receives includePayload=true and payloads are returned; locate the test block around trp_peek_pending, change the JSON key to "includePayload" when building req/Params, then re-run the test.
🧹 Nitpick comments (1)
crates/trp/src/methods.rs (1)
121-135:PeekPendingParamsandPeekInflightParamsare structurally identical — consider deduplicating.Both structs have the same two fields (
limit: Option<usize>,include_payload: Option<bool>). A single shared struct (e.g.,PeekParams) can serve both handlers.♻️ Proposed refactor
-// ── trp.peekPending ───────────────────────────────────────────────────── - -#[derive(Deserialize)] -struct PeekPendingParams { - limit: Option<usize>, - #[serde(rename = "includePayload")] - include_payload: Option<bool>, -} - -// ── trp.peekInflight ──────────────────────────────────────────────────── - -#[derive(Deserialize)] -struct PeekInflightParams { - limit: Option<usize>, - #[serde(rename = "includePayload")] - include_payload: Option<bool>, -} +// ── trp.peekPending / trp.peekInflight ────────────────────────────────── + +#[derive(Deserialize)] +struct PeekParams { + limit: Option<usize>, + #[serde(rename = "includePayload")] + include_payload: Option<bool>, +}Then update both handlers to use
PeekParams:- let params: PeekPendingParams = params.parse()?; + let params: PeekParams = params.parse()?;- let params: PeekInflightParams = params.parse()?; + let params: PeekParams = params.parse()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trp/src/methods.rs` around lines 121 - 135, The two structs PeekPendingParams and PeekInflightParams are identical; replace them with a single shared struct PeekParams containing limit: Option<usize> and #[serde(rename = "includePayload")] include_payload: Option<bool>, then update all places that referenced PeekPendingParams or PeekInflightParams (e.g., the peek-pending and peek-inflight handlers) to use PeekParams instead; keep the serde rename on include_payload so JSON compatibility is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/trp/src/methods.rs`:
- Around line 499-506: The test is using the wrong JSON key name for
PeekPendingParams' includePayload field; update the request payload sent to
trp_peek_pending to use the exact serde name "includePayload" (not
"include_payload") so PeekPendingParams (used by trp_peek_pending) receives
includePayload=true and payloads are returned; locate the test block around
trp_peek_pending, change the JSON key to "includePayload" when building
req/Params, then re-run the test.
---
Nitpick comments:
In `@crates/trp/src/methods.rs`:
- Around line 121-135: The two structs PeekPendingParams and PeekInflightParams
are identical; replace them with a single shared struct PeekParams containing
limit: Option<usize> and #[serde(rename = "includePayload")] include_payload:
Option<bool>, then update all places that referenced PeekPendingParams or
PeekInflightParams (e.g., the peek-pending and peek-inflight handlers) to use
PeekParams instead; keep the serde rename on include_payload so JSON
compatibility is preserved.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/trp/Cargo.tomlcrates/trp/src/methods.rs
1 participant
Summary by CodeRabbit
Chores
Refactor
Bug Fix / API