🧹 Refactor search context arguments in working_set.rs#3118
Conversation
This commit addresses a code health issue where `walk_always_discoverable_dirs`, `walk_for_completions`, and `add_local_reference_completions` had too many arguments and required `#[allow(clippy::too_many_arguments)]`. A new struct `SearchContext<'a>` is introduced to encapsulate the frequently passed search state (`prefix_hits`, `substring_hits`, `seen`, and `limit`). This simplifies function signatures, improves readability, and resolves the clippy warnings. Co-authored-by: Hmbown <101357273+Hmbown@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a SearchContext struct in crates/tui/src/working_set.rs to group search-related parameters, simplifying function signatures and allowing the removal of several #[allow(clippy::too_many_arguments)] attributes. The reviewer suggests further encapsulating the limit checking and hit-adding logic as helper methods on the new SearchContext struct to reduce code duplication and improve maintainability across the walk functions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| struct SearchContext<'a> { | ||
| prefix_hits: &'a mut Vec<String>, | ||
| substring_hits: &'a mut Vec<String>, | ||
| seen: &'a mut HashSet<PathBuf>, | ||
| limit: usize, | ||
| } |
There was a problem hiding this comment.
Since we are introducing a SearchContext struct to group these search parameters, we can encapsulate the limit checking and hit-adding logic as helper methods on the struct. This reduces code duplication across the different walk functions and improves maintainability.
struct SearchContext<'a> {
prefix_hits: &'a mut Vec<String>,
substring_hits: &'a mut Vec<String>,
seen: &'a mut HashSet<PathBuf>,
limit: usize,
}
impl<'a> SearchContext<'a> {
fn is_limit_reached(&self) -> bool {
self.prefix_hits.len() + self.substring_hits.len() >= self.limit
}
fn add_hit(&mut self, candidate: String, needle: &str) {
let lower = candidate.to_lowercase();
if needle.is_empty() || lower.starts_with(needle) {
self.prefix_hits.push(candidate);
} else if lower.contains(needle) {
self.substring_hits.push(candidate);
}
}
}| } | ||
| for entry in builder.build().flatten() { | ||
| if prefix_hits.len() + substring_hits.len() >= limit { | ||
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { |
There was a problem hiding this comment.
Use the encapsulated is_limit_reached helper method on SearchContext to simplify the limit check.
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { | |
| if ctx.is_limit_reached() { |
| let lower = candidate.to_lowercase(); | ||
| if needle.is_empty() || lower.starts_with(needle) { | ||
| prefix_hits.push(candidate); | ||
| ctx.prefix_hits.push(candidate); | ||
| } else if lower.contains(needle) { | ||
| substring_hits.push(candidate); | ||
| ctx.substring_hits.push(candidate); | ||
| } |
There was a problem hiding this comment.
Use the encapsulated add_hit helper method on SearchContext to simplify adding prefix or substring hits.
| let lower = candidate.to_lowercase(); | |
| if needle.is_empty() || lower.starts_with(needle) { | |
| prefix_hits.push(candidate); | |
| ctx.prefix_hits.push(candidate); | |
| } else if lower.contains(needle) { | |
| substring_hits.push(candidate); | |
| ctx.substring_hits.push(candidate); | |
| } | |
| ctx.add_hit(candidate, needle); |
|
|
||
| for entry in builder.build().flatten() { | ||
| if prefix_hits.len() + substring_hits.len() >= limit { | ||
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { |
There was a problem hiding this comment.
Use the encapsulated is_limit_reached helper method on SearchContext to simplify the limit check.
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { | |
| if ctx.is_limit_reached() { |
| let lower = candidate.to_lowercase(); | ||
| if needle.is_empty() || lower.starts_with(needle) { | ||
| prefix_hits.push(candidate); | ||
| ctx.prefix_hits.push(candidate); | ||
| } else if lower.contains(needle) { | ||
| substring_hits.push(candidate); | ||
| ctx.substring_hits.push(candidate); | ||
| } |
There was a problem hiding this comment.
Use the encapsulated add_hit helper method on SearchContext to simplify adding prefix or substring hits.
| let lower = candidate.to_lowercase(); | |
| if needle.is_empty() || lower.starts_with(needle) { | |
| prefix_hits.push(candidate); | |
| ctx.prefix_hits.push(candidate); | |
| } else if lower.contains(needle) { | |
| substring_hits.push(candidate); | |
| ctx.substring_hits.push(candidate); | |
| } | |
| ctx.add_hit(candidate, needle); |
|
|
||
| for path in local_reference_paths(root, LOCAL_REFERENCE_SCAN_LIMIT, max_depth) { | ||
| if prefix_hits.len() + substring_hits.len() >= limit { | ||
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { |
There was a problem hiding this comment.
Use the encapsulated is_limit_reached helper method on SearchContext to simplify the limit check.
| if ctx.prefix_hits.len() + ctx.substring_hits.len() >= ctx.limit { | |
| if ctx.is_limit_reached() { |
| let lower = rel_str.to_lowercase(); | ||
| if needle.is_empty() || lower.starts_with(needle) { | ||
| prefix_hits.push(rel_str); | ||
| ctx.prefix_hits.push(rel_str); | ||
| } else if lower.contains(needle) { | ||
| substring_hits.push(rel_str); | ||
| ctx.substring_hits.push(rel_str); | ||
| } |
There was a problem hiding this comment.
Use the encapsulated add_hit helper method on SearchContext to simplify adding prefix or substring hits.
| let lower = rel_str.to_lowercase(); | |
| if needle.is_empty() || lower.starts_with(needle) { | |
| prefix_hits.push(rel_str); | |
| ctx.prefix_hits.push(rel_str); | |
| } else if lower.contains(needle) { | |
| substring_hits.push(rel_str); | |
| ctx.substring_hits.push(rel_str); | |
| } | |
| ctx.add_hit(rel_str, needle); |
|
Closing as part of cleaning up stale auto-generated PRs from the v0.8.64/v0.8.65 milestones. If any of this work is still wanted it can be reopened or re-landed. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
🎯 What: Grouped related search parameters (
prefix_hits,substring_hits,seen, andlimit) into a newSearchContextstruct incrates/tui/src/working_set.rs. Applied this refactoring towalk_always_discoverable_dirs,walk_for_completions, andadd_local_reference_completions.💡 Why: This reduces the number of arguments passed to these helper functions, making the code much easier to read and maintain. It also allows the removal of the
#[allow(clippy::too_many_arguments)]attribute.✅ Verification: Verified by running
cargo clippy -p codewhale-tui(no warnings) andcargo test -p codewhale-tui(all related tests passed).✨ Result: Improved maintainability and reduced clippy suppressions without altering functionality.
PR created automatically by Jules for task 3644023353456815746 started by @Hmbown