Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR implements client deletion functionality in the CLI. Previously unimplemented, the ChangesClient Deletion Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 49 minutes and 15 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libs/ferriskey-cli-core/src/client.rs (2)
111-111: 💤 Low valueUnnecessary
.clone()onargs.realm.
args.realmis not read after line 111, so it can be moved directly intoresolve_realminstead of cloned.♻️ Proposed fix
- let realm = resolve_realm(&context, args.realm.clone())?; + let realm = resolve_realm(&context, args.realm)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ferriskey-cli-core/src/client.rs` at line 111, The call to resolve_realm is cloning args.realm unnecessarily; change the call to move the value instead by passing args.realm (by value) rather than args.realm.clone() so ownership of the realm is transferred into resolve_realm; ensure any later uses of args.realm are removed or adjusted (e.g., update references in the surrounding function) and keep the function signature of resolve_realm as resolve_realm(&context, realm) to accept the moved String/value.
459-601: 🏗️ Heavy liftNo tests for
delete_clientorrender_message.The existing test suite covers
select_context,resolve_realm, andbuild_create_client_request, but has no coverage for the newly introduceddelete_clientandrender_message. At minimum,render_messageis straightforward to unit-test with the three output formats, and theuuid-missing error path indelete_clientshould be exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ferriskey-cli-core/src/client.rs` around lines 459 - 601, Add unit tests under the existing #[cfg(test)] mod to cover render_message and the error path in delete_client: write three tests that call render_message with MessageFormat::Json, MessageFormat::Yaml, and MessageFormat::Text and assert the output format/content matches expectations; and add a test that constructs a ClientDeleteArgs (or calls delete_client helper) with a missing/None uuid to assert it returns the expected error (ClientCommandError::MissingUuid or whichever variant delete_client uses). Reference the render_message function and delete_client (or the ClientDeleteArgs type) when adding these tests so they live alongside the other tests in the same file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/ferriskey-cli-core/src/client.rs`:
- Around line 119-123: The code currently uses found.id.unwrap_or_default()
which silently yields an empty string and then calls client.delete_client with
that empty UUID; change this to explicitly handle the Option returned by
found.id and return a clear error instead of an empty string. For example, after
obtaining found from client.get_client, check found.id (or use
found.id.ok_or_else(...)) and return a ClientCommandError variant (e.g.
ClientCommandError::MissingClientId or ClientCommandError::ClientHasNoId with
args.client_id.clone()) when id is None, only calling
client.delete_client(&realm, &uuid)? when a non-empty UUID is present; update
the call sites around get_client, found.id, and delete_client accordingly.
- Around line 411-413: The JSON branch currently prints compact JSON with
println!("{}", serde_json::json!({ "message": message })), causing inconsistent
formatting versus other renderers (render_client_detail, render_client_list,
render_created_client); change this match arm to produce pretty-printed JSON
using serde_json::to_string_pretty(&serde_json::json!({ "message": message }))
and then println! the resulting String (or propagate/unwrap the Result in the
same style used by the other render functions) so the delete output matches the
pretty formatting used elsewhere.
---
Nitpick comments:
In `@libs/ferriskey-cli-core/src/client.rs`:
- Line 111: The call to resolve_realm is cloning args.realm unnecessarily;
change the call to move the value instead by passing args.realm (by value)
rather than args.realm.clone() so ownership of the realm is transferred into
resolve_realm; ensure any later uses of args.realm are removed or adjusted
(e.g., update references in the surrounding function) and keep the function
signature of resolve_realm as resolve_realm(&context, realm) to accept the moved
String/value.
- Around line 459-601: Add unit tests under the existing #[cfg(test)] mod to
cover render_message and the error path in delete_client: write three tests that
call render_message with MessageFormat::Json, MessageFormat::Yaml, and
MessageFormat::Text and assert the output format/content matches expectations;
and add a test that constructs a ClientDeleteArgs (or calls delete_client
helper) with a missing/None uuid to assert it returns the expected error
(ClientCommandError::MissingUuid or whichever variant delete_client uses).
Reference the render_message function and delete_client (or the ClientDeleteArgs
type) when adding these tests so they live alongside the other tests in the same
file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c5f0264-8f0d-4ff8-97e9-ee1763aca09b
📒 Files selected for processing (2)
libs/ferriskey-cli-core/src/client.rslibs/ferriskey-commands/src/lib.rs
1 participant
Summary by CodeRabbit