refactor: deduplicate shared utilities into utils module#30
Conversation
Move constant_time_eq, is_vx2_file, format_size, and HumanBytes from cli.rs and gui.rs into src/utils.rs. Export from lib.rs and update both callers to import from crate::utils.
Reviewer's GuideRefactors several duplicated helper functions into a new shared utils module and updates cli and gui to consume the shared APIs, unifying constant-time comparison, VX2 file detection, and human-readable size formatting across the crate. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
More reviews will be available in 59 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="neuron-encrypt/src/utils.rs" line_range="1-20" />
<code_context>
+pub fn constant_time_eq(a: &[u8], b: &[u8]) -> bool {
+ let len_a = a.len();
+ let len_b = b.len();
+ let max_len = len_a.max(len_b);
+
+ let mut byte_comparison_result = 0;
+
+ for i in 0..max_len {
+ let byte_a = a.get(i).unwrap_or(&0);
+ let byte_b = b.get(i).unwrap_or(&0);
+ byte_comparison_result |= byte_a ^ byte_b;
+ }
+
+ let len_diff = len_a ^ len_b;
+ let len_mismatch_flag = (((len_diff | len_diff.wrapping_neg()) >> (usize::BITS - 1)) & 1) as u8;
+
+ let final_result = byte_comparison_result | len_mismatch_flag;
+
+ final_result == 0
</code_context>
<issue_to_address>
**suggestion:** Consider a more flexible signature for `constant_time_eq` to avoid repeated `as_bytes()` at call sites.
Now that this utility is shared between CLI and GUI, consider making it generic over `AsRef<[u8]>`, e.g. `pub fn constant_time_eq<A: AsRef<[u8]>, B: AsRef<[u8]>>(a: A, b: B) -> bool`. That would allow direct use with `String`, `&str`, `Vec<u8>`, etc., and remove the need for repeated `as_bytes()` calls while keeping the implementation unchanged.
```suggestion
pub fn constant_time_eq<A: AsRef<[u8]>, B: AsRef<[u8]>>(a: A, b: B) -> bool {
let a = a.as_ref();
let b = b.as_ref();
let len_a = a.len();
let len_b = b.len();
let max_len = len_a.max(len_b);
let mut byte_comparison_result = 0;
for i in 0..max_len {
let byte_a = a.get(i).unwrap_or(&0);
let byte_b = b.get(i).unwrap_or(&0);
byte_comparison_result |= byte_a ^ byte_b;
}
let len_diff = len_a ^ len_b;
let len_mismatch_flag = (((len_diff | len_diff.wrapping_neg()) >> (usize::BITS - 1)) & 1) as u8;
let final_result = byte_comparison_result | len_mismatch_flag;
final_result == 0
}
```
</issue_to_address>
### Comment 2
<location path="neuron-encrypt/src/gui.rs" line_range="413-414" />
<code_context>
}
- if self.mode == Mode::Encrypt && !constant_time_eq(&self.password, &self.confirm_password) {
+ if self.mode == Mode::Encrypt && !constant_time_eq(self.password.as_bytes(), self.confirm_password.as_bytes()) {
self.status = Some("Passphrases do not match.".to_owned());
return;
}
</code_context>
<issue_to_address>
**suggestion:** The repeated `password`/`confirm_password` comparisons could be centralized to reduce duplication and potential drift.
Since this comparison appears in multiple branches with similar conditions, consider extracting a helper like `fn passwords_match(&self) -> bool` that performs the mode check and constant-time comparison. This avoids duplicated logic and keeps any future change to the comparison behavior in a single place.
Suggested implementation:
```rust
if !self.passwords_match() {
self.status = Some("Passphrases do not match.".to_owned());
return;
}
```
To fully implement the suggestion and centralize the comparison logic, you should:
1. Add a helper method on the relevant GUI/app struct (the one that owns `mode`, `password`, and `confirm_password`) inside its `impl` block, for example:
```rust
impl App { // or whatever the struct is named
fn passwords_match(&self) -> bool {
if self.mode != Mode::Encrypt {
return true;
}
constant_time_eq(self.password.as_bytes(), self.confirm_password.as_bytes())
}
}
```
2. Replace any other occurrences of repeated comparison logic that look like:
```rust
if self.mode == Mode::Encrypt && !constant_time_eq(self.password.as_bytes(), self.confirm_password.as_bytes()) { ... }
```
or similar direct comparisons, with:
```rust
if !self.passwords_match() { ... }
```
so that all passphrase comparison behavior is centralized in `passwords_match`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pub fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { | ||
| let len_a = a.len(); | ||
| let len_b = b.len(); | ||
| let max_len = len_a.max(len_b); | ||
|
|
||
| let mut byte_comparison_result = 0; | ||
|
|
||
| for i in 0..max_len { | ||
| let byte_a = a.get(i).unwrap_or(&0); | ||
| let byte_b = b.get(i).unwrap_or(&0); | ||
| byte_comparison_result |= byte_a ^ byte_b; | ||
| } | ||
|
|
||
| let len_diff = len_a ^ len_b; | ||
| let len_mismatch_flag = (((len_diff | len_diff.wrapping_neg()) >> (usize::BITS - 1)) & 1) as u8; | ||
|
|
||
| let final_result = byte_comparison_result | len_mismatch_flag; | ||
|
|
||
| final_result == 0 | ||
| } |
There was a problem hiding this comment.
suggestion: Consider a more flexible signature for constant_time_eq to avoid repeated as_bytes() at call sites.
Now that this utility is shared between CLI and GUI, consider making it generic over AsRef<[u8]>, e.g. pub fn constant_time_eq<A: AsRef<[u8]>, B: AsRef<[u8]>>(a: A, b: B) -> bool. That would allow direct use with String, &str, Vec<u8>, etc., and remove the need for repeated as_bytes() calls while keeping the implementation unchanged.
| pub fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { | |
| let len_a = a.len(); | |
| let len_b = b.len(); | |
| let max_len = len_a.max(len_b); | |
| let mut byte_comparison_result = 0; | |
| for i in 0..max_len { | |
| let byte_a = a.get(i).unwrap_or(&0); | |
| let byte_b = b.get(i).unwrap_or(&0); | |
| byte_comparison_result |= byte_a ^ byte_b; | |
| } | |
| let len_diff = len_a ^ len_b; | |
| let len_mismatch_flag = (((len_diff | len_diff.wrapping_neg()) >> (usize::BITS - 1)) & 1) as u8; | |
| let final_result = byte_comparison_result | len_mismatch_flag; | |
| final_result == 0 | |
| } | |
| pub fn constant_time_eq<A: AsRef<[u8]>, B: AsRef<[u8]>>(a: A, b: B) -> bool { | |
| let a = a.as_ref(); | |
| let b = b.as_ref(); | |
| let len_a = a.len(); | |
| let len_b = b.len(); | |
| let max_len = len_a.max(len_b); | |
| let mut byte_comparison_result = 0; | |
| for i in 0..max_len { | |
| let byte_a = a.get(i).unwrap_or(&0); | |
| let byte_b = b.get(i).unwrap_or(&0); | |
| byte_comparison_result |= byte_a ^ byte_b; | |
| } | |
| let len_diff = len_a ^ len_b; | |
| let len_mismatch_flag = (((len_diff | len_diff.wrapping_neg()) >> (usize::BITS - 1)) & 1) as u8; | |
| let final_result = byte_comparison_result | len_mismatch_flag; | |
| final_result == 0 | |
| } |
| if self.mode == Mode::Encrypt && !constant_time_eq(self.password.as_bytes(), self.confirm_password.as_bytes()) { | ||
| self.status = Some("Passphrases do not match.".to_owned()); |
There was a problem hiding this comment.
suggestion: The repeated password/confirm_password comparisons could be centralized to reduce duplication and potential drift.
Since this comparison appears in multiple branches with similar conditions, consider extracting a helper like fn passwords_match(&self) -> bool that performs the mode check and constant-time comparison. This avoids duplicated logic and keeps any future change to the comparison behavior in a single place.
Suggested implementation:
if !self.passwords_match() {
self.status = Some("Passphrases do not match.".to_owned());
return;
}To fully implement the suggestion and centralize the comparison logic, you should:
-
Add a helper method on the relevant GUI/app struct (the one that owns
mode,password, andconfirm_password) inside itsimplblock, for example:impl App { // or whatever the struct is named fn passwords_match(&self) -> bool { if self.mode != Mode::Encrypt { return true; } constant_time_eq(self.password.as_bytes(), self.confirm_password.as_bytes()) } }
-
Replace any other occurrences of repeated comparison logic that look like:
if self.mode == Mode::Encrypt && !constant_time_eq(self.password.as_bytes(), self.confirm_password.as_bytes()) { ... }
or similar direct comparisons, with:
if !self.passwords_match() { ... }
so that all passphrase comparison behavior is centralized in
passwords_match.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -2 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| pub struct HumanBytes(pub u64); |
There was a problem hiding this comment.
🔥 The Roast: You moved two byte-size formatters into the same module and let them keep different GB precision (format_size uses {:.1}, HumanBytes uses {:.2}). That's not deduplication so much as putting twins in matching outfits with different shoe sizes.
🩹 The Fix: Pick one source of truth: either have HumanBytes delegate to format_size(self.0) if one decimal is acceptable, or factor a shared formatter that takes precision and use it from both paths. Add a small boundary-size test/table so future formatting tweaks don't quietly diverge.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: 1 New Issue Found (2 Existing Active Comments) | Recommendation: Address before merge OverviewActive issue count includes 2 pre-existing Sourcery comments plus 1 new Kilo comment.
Issue Details (click to expand)
Fix these issues in Kilo Cloud 🏆 Best part: Oh wait, extracting duplicated helpers into a shared module is actually the right instinct; I had my flamethrower warmed up and found a refactor with a spine. 💀 Worst part: The byte-size formatter twins living in the same module with different GB precision is the biggest new concern — tiny, but it smells like future drift wearing a fake mustache. 📊 Overall: Mostly a clean utility extraction; fix the formatter drift and the existing active comments, then this PR can stop looking over its shoulder. Files Reviewed (4 files)
Reviewed by nex-n2-pro:free · Input: 829K · Output: 12.9K · Cached: 0 |
1 participant
Move constant_time_eq, is_vx2_file, format_size, and HumanBytes from cli.rs and gui.rs into src/utils.rs. Export from lib.rs and update both callers to import from crate::utils.
Summary by Sourcery
Deduplicate shared helper functionality into a reusable utils module and update CLI and GUI to use it.
Enhancements: