crates/config/src/combined.rs (1)

15-21: Consider clarifying the inner type or adding docs for future maintenance.

ScanResultInner uses indexes referencing RuleConfig. It is unclear if there's any plan to handle invalid indexes. Documenting or enforcing index validity could help prevent potential panics when converting to ScanResult.

crates/cli/src/scan.rs (1)

154-157: Refactor for consistency between diffs and matches processing.

Here, match_rule_diff_on_file is invoked only for scanned.diffs. For clarity, consider grouping or naming the calls in a similar manner when handling scanned.matches.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Searching for usage of `get_rule` calls to check index references
rg --context 3 "get_rule\s*\(" 

#!/bin/bash
# Search for where diffs and matches are populated with indices
ast-grep --pattern 'struct ScanResultInner {
  $$$
}'

# Also check the implementation of CombinedScan
ast-grep --pattern 'struct CombinedScan {
  $$$
}'

#!/bin/bash
# Search for struct definitions and their fields
rg --type rust -A 10 "struct (ScanResultInner|CombinedScan)" 

# Look for where indices might be populated
rg --type rust -B 3 -A 3 "diffs.*=|matches.*="

crates/config/src/combined.rs (2)

9-13: Improved type safety with direct rule references!

The restructured ScanResult with direct references to RuleConfig instead of indices is a significant improvement:

• Eliminates potential index-out-of-bounds errors
• Provides better compile-time guarantees
• Makes the relationship between rules and matches more explicit

Consider documenting the lifetime parameters 't and 'r to clarify their purposes, as they play a crucial role in ensuring reference validity.

---

24-54: Consider simplifying unused suppression handling

The unused suppression handling logic could be extracted into a separate method for better readability and maintainability.

Consider refactoring like this:

 impl<'t, D: Doc> ScanResultInner<'t, D> {
+  fn handle_unused_suppressions<'r, L: Language>(
+    unused: Vec<NodeMatch<'t, D>>,
+    rule: &'r RuleConfig<L>,
+    separate_fix: bool,
+  ) -> (Vec<(&'r RuleConfig<L>, NodeMatch<'t, D>)>, Vec<(&'r RuleConfig<L>, Vec<NodeMatch<'t, D>>)>) {
+    if separate_fix {
+      (unused.into_iter().map(|nm| (rule, nm)).collect(), vec![])
+    } else {
+      let mut supprs = unused;
+      supprs.sort_unstable_by_key(|nm| nm.range().start);
+      (vec![], vec![(rule, supprs)])
+    }
+  }
+
   pub fn into_result<'r, L: Language>(
     self,
     combined: &CombinedScan<'r, L>,
     separate_fix: bool,
   ) -> ScanResult<'t, 'r, D, L> {
     let mut diffs: Vec<_> = self
       .diffs
       .into_iter()
       .map(|(idx, nm)| (combined.get_rule(idx), nm))
       .collect();
     let mut matches: Vec<_> = self
       .matches
       .into_iter()
       .map(|(idx, nms)| (combined.get_rule(idx), nms))
       .collect();
     if let Some(rule) = combined.unused_suppression_rule {
-      if separate_fix {
-        diffs.extend(self.unused_suppressions.into_iter().map(|nm| (rule, nm)));
-        diffs.sort_unstable_by_key(|(_, nm)| nm.range().start);
-      } else {
-        let mut supprs = self.unused_suppressions;
-        supprs.sort_unstable_by_key(|nm| nm.range().start);
-        matches.push((rule, supprs));
-      }
+      let (mut new_diffs, mut new_matches) = 
+        Self::handle_unused_suppressions(self.unused_suppressions, rule, separate_fix);
+      diffs.append(&mut new_diffs);
+      matches.append(&mut new_matches);
     }
     ScanResult {
       diffs,
       matches,
       unused_suppressions: vec![],
     }
   }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Check test coverage for unused suppression scenarios
rg -l 'unused.*suppression' crates/cli/src/scan.rs crates/config/src/combined.rs | xargs -I {} rg -A 5 '#\[test\]' {}

crates/config/src/combined.rs (2)

29-43: Consider consistent sorting for all diffs

Currently, sorting by range start is only applied to unused suppressions. Consider applying consistent sorting to all diffs to maintain a predictable order in the output.

 let mut diffs: Vec<_> = self
   .diffs
   .into_iter()
   .map(|(idx, nm)| (combined.get_rule(idx), nm))
   .collect();
+diffs.sort_unstable_by_key(|(_, nm)| nm.range().start);
 if let Some(rule) = combined.unused_suppression_rule {
   if separate_fix {
     diffs.extend(self.unused_suppressions.into_iter().map(|nm| (rule, nm)));
-    diffs.sort_unstable_by_key(|(_, nm)| nm.range().start);
   } else {

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 41-42: crates/config/src/combined.rs#L41-L42
Added lines #L41 - L42 were not covered by tests

---

362-370: Enhance test coverage for edge cases

The test suite could be strengthened by adding the following test cases:

1. Verify the order of results when multiple matches are found
2. Test the behavior when separate_fix=true
3. Test the interaction between fixes and unused suppressions

Here's a suggested test case structure:

#[test]
fn test_separate_fix_with_unused_suppression() {
    let source = r#"
    console.log('first') // ast-grep-ignore: test
    console.log('second')
    console.log('third') // ast-grep-ignore: not-test
    "#;
    let root = TypeScript::Tsx.ast_grep(source);
    let rule = create_rule();
    let mut scan = CombinedScan::new(vec![&rule]);
    scan.set_unused_suppression_rule(&rule);
    let pre = scan.find(&root);
    let scanned = scan.scan(&root, pre, true);
    
    // Verify diffs are sorted by position
    assert_eq!(scanned.diffs.len(), 3);
    let texts: Vec<_> = scanned.diffs
        .iter()
        .map(|(_, nm)| nm.text())
        .collect();
    assert_eq!(texts, vec![
        "console.log('first')",
        "console.log('second')",
        "console.log('third')",
    ]);
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro