diff --git a/crates/legolas-core/src/action_plan.rs b/crates/legolas-core/src/action_plan.rs index af050a6..9c23b40 100644 --- a/crates/legolas-core/src/action_plan.rs +++ b/crates/legolas-core/src/action_plan.rs @@ -7,8 +7,8 @@ use crate::{ subpath_import_fix_hint, }, models::{ - ActionDifficulty, ActionPlanItem, Analysis, DuplicatePackage, HeavyDependency, - LazyLoadCandidate, RecommendedFix, TreeShakingWarning, + ActionDifficulty, ActionPlanItem, Analysis, DuplicateImpactScope, DuplicatePackage, + HeavyDependency, LazyLoadCandidate, RecommendedFix, TreeShakingWarning, }, }; @@ -18,6 +18,7 @@ pub fn rank_actions(analysis: &Analysis) -> Vec { for item in &analysis.heavy_dependencies { push_action( &mut actions, + ActionRankBucket::SourceOrArtifactPerformance, &item.finding, item.estimated_kb, ActionDifficulty::Hard, @@ -28,6 +29,7 @@ pub fn rank_actions(analysis: &Analysis) -> Vec { for item in &analysis.lazy_load_candidates { push_action( &mut actions, + ActionRankBucket::SourceOrArtifactPerformance, &item.finding, item.estimated_savings_kb, ActionDifficulty::Medium, @@ -38,6 +40,7 @@ pub fn rank_actions(analysis: &Analysis) -> Vec { for item in &analysis.tree_shaking_warnings { push_action( &mut actions, + ActionRankBucket::SourceOrArtifactPerformance, &item.finding, item.estimated_kb, ActionDifficulty::Easy, @@ -48,6 +51,7 @@ pub fn rank_actions(analysis: &Analysis) -> Vec { for item in &analysis.duplicate_packages { push_action( &mut actions, + duplicate_action_bucket(item.impact_scope), &item.finding, item.estimated_extra_kb, ActionDifficulty::Medium, @@ -56,17 +60,23 @@ pub fn rank_actions(analysis: &Analysis) -> Vec { } actions.sort_by(|left, right| { - right - .estimated_savings_kb - .cmp(&left.estimated_savings_kb) - .then(right.confidence.cmp(&left.confidence)) - .then(left.difficulty.cmp(&right.difficulty)) - .then(left.finding_id.cmp(&right.finding_id)) + left.bucket + .cmp(&right.bucket) + .then( + right + .item + .estimated_savings_kb + .cmp(&left.item.estimated_savings_kb), + ) + .then(right.item.confidence.cmp(&left.item.confidence)) + .then(left.item.difficulty.cmp(&right.item.difficulty)) + .then(left.item.finding_id.cmp(&right.item.finding_id)) }); let mut seen = BTreeSet::new(); let mut ranked = actions .into_iter() + .map(|ranked| ranked.item) .filter(|item| seen.insert(item.finding_id.clone())) .collect::>(); @@ -77,6 +87,28 @@ pub fn rank_actions(analysis: &Analysis) -> Vec { ranked } +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +enum ActionRankBucket { + SourceOrArtifactPerformance, + ProductionLikelyDuplicate, + UnknownDuplicate, + DevOnlyDuplicate, +} + +#[derive(Debug, Clone)] +struct RankedAction { + bucket: ActionRankBucket, + item: ActionPlanItem, +} + +fn duplicate_action_bucket(impact_scope: DuplicateImpactScope) -> ActionRankBucket { + match impact_scope { + DuplicateImpactScope::ProductionLikely => ActionRankBucket::ProductionLikelyDuplicate, + DuplicateImpactScope::Unknown => ActionRankBucket::UnknownDuplicate, + DuplicateImpactScope::DevOnly => ActionRankBucket::DevOnlyDuplicate, + } +} + pub fn apply_action_plan(analysis: &mut Analysis) -> Vec { let actions = rank_actions(analysis); let action_by_id = actions @@ -101,7 +133,8 @@ pub fn apply_action_plan(analysis: &mut Analysis) -> Vec { } fn push_action( - actions: &mut Vec, + actions: &mut Vec, + bucket: ActionRankBucket, finding: &FindingMetadata, estimated_savings_kb: usize, difficulty: ActionDifficulty, @@ -111,13 +144,16 @@ fn push_action( return; }; - actions.push(ActionPlanItem { - action_priority: 0, - finding_id, - estimated_savings_kb, - confidence: finding.confidence.unwrap_or(FindingConfidence::Low), - difficulty, - recommended_fix, + actions.push(RankedAction { + bucket, + item: ActionPlanItem { + action_priority: 0, + finding_id, + estimated_savings_kb, + confidence: finding.confidence.unwrap_or(FindingConfidence::Low), + difficulty, + recommended_fix, + }, }); } @@ -180,12 +216,42 @@ fn tree_shaking_fix(item: &TreeShakingWarning) -> Option { } fn duplicate_package_fix(item: &DuplicatePackage) -> Option { + if item.impact_scope == DuplicateImpactScope::DevOnly || !versions_share_major(&item.versions) { + return None; + } + dedupe_resolution_fix_hint( &item.finding, format!("Deduplicate {} to one installed version.", item.name), ) } +fn versions_share_major(versions: &[String]) -> bool { + if versions.len() < 2 { + return false; + } + + let Some(first_major) = versions.first().and_then(|version| major_version(version)) else { + return false; + }; + + versions + .iter() + .skip(1) + .all(|version| major_version(version) == Some(first_major)) +} + +fn major_version(version: &str) -> Option { + let normalized = version.trim().trim_start_matches('v'); + let major = normalized.split(['.', '-', '+']).next()?; + + if major.is_empty() || !major.chars().all(|character| character.is_ascii_digit()) { + return None; + } + + major.parse().ok() +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum SupportedFixHintKind { DynamicImport, diff --git a/crates/legolas-core/tests/action_plan.rs b/crates/legolas-core/tests/action_plan.rs index 6c7945d..90cfba5 100644 --- a/crates/legolas-core/tests/action_plan.rs +++ b/crates/legolas-core/tests/action_plan.rs @@ -1,7 +1,7 @@ use legolas_core::{ - apply_action_plan, rank_actions, ActionDifficulty, Analysis, DuplicatePackage, - FindingAnalysisSource, FindingConfidence, FindingMetadata, HeavyDependency, LazyLoadCandidate, - RecommendedFix, TreeShakingWarning, + apply_action_plan, rank_actions, ActionDifficulty, Analysis, DuplicateImpactScope, + DuplicatePackage, FindingAnalysisSource, FindingConfidence, FindingMetadata, HeavyDependency, + LazyLoadCandidate, RecommendedFix, TreeShakingWarning, }; use serde_json::json; @@ -51,32 +51,83 @@ fn rank_actions_sorts_by_savings_confidence_and_difficulty() { vec![ ( 1, - "duplicate-package:low-medium", - 150, - FindingConfidence::Low, - ActionDifficulty::Medium, - ), - ( - 2, "tree-shaking:high-easy", 100, FindingConfidence::High, ActionDifficulty::Easy, ), ( - 3, + 2, "lazy-load:high-medium", 100, FindingConfidence::High, ActionDifficulty::Medium, ), ( - 4, + 3, "heavy-dependency:medium-hard", 100, FindingConfidence::Medium, ActionDifficulty::Hard, ), + ( + 4, + "duplicate-package:low-medium", + 150, + FindingConfidence::Low, + ActionDifficulty::Medium, + ), + ] + ); +} + +#[test] +fn rank_actions_prioritizes_source_work_before_duplicate_hygiene_buckets() { + let analysis = Analysis { + tree_shaking_warnings: vec![tree_shaking_warning( + "tree-shaking:lodash-root-import", + "lodash", + 100, + FindingConfidence::High, + )], + duplicate_packages: vec![ + duplicate_package_with_scope( + "duplicate-package:dev-only", + "eslint-visitor-keys", + 150, + FindingConfidence::High, + DuplicateImpactScope::DevOnly, + ), + duplicate_package_with_scope( + "duplicate-package:unknown", + "ansi-styles", + 120, + FindingConfidence::High, + DuplicateImpactScope::Unknown, + ), + duplicate_package_with_scope( + "duplicate-package:production", + "react", + 40, + FindingConfidence::Low, + DuplicateImpactScope::ProductionLikely, + ), + ], + ..Default::default() + }; + + let actions = rank_actions(&analysis); + + assert_eq!( + actions + .iter() + .map(|item| item.finding_id.as_str()) + .collect::>(), + vec![ + "tree-shaking:lodash-root-import", + "duplicate-package:production", + "duplicate-package:unknown", + "duplicate-package:dev-only", ] ); } @@ -308,6 +359,62 @@ fn apply_action_plan_only_exposes_safe_fix_hints_for_high_confidence_findings() ); } +#[test] +fn duplicate_package_fix_hint_requires_same_major_and_not_dev_only() { + let mut analysis = Analysis { + duplicate_packages: vec![ + duplicate_package_with_versions_and_scope( + "duplicate-package:major-mismatch", + "react", + 36, + FindingConfidence::High, + &["17.0.2", "18.2.0"], + DuplicateImpactScope::ProductionLikely, + ), + duplicate_package_with_versions_and_scope( + "duplicate-package:same-major", + "lodash", + 18, + FindingConfidence::High, + &["4.17.20", "4.17.21"], + DuplicateImpactScope::Unknown, + ), + duplicate_package_with_versions_and_scope( + "duplicate-package:dev-only", + "eslint-visitor-keys", + 18, + FindingConfidence::High, + &["3.3.0", "3.4.0"], + DuplicateImpactScope::DevOnly, + ), + ], + ..Default::default() + }; + + apply_action_plan(&mut analysis); + + let major_mismatch = &analysis.duplicate_packages[0].finding.recommended_fix; + assert_ne!( + major_mismatch.as_ref().map(|fix| fix.kind.as_str()), + Some("dedupe-package") + ); + + assert_eq!( + analysis.duplicate_packages[1] + .finding + .recommended_fix + .as_ref() + .map(|fix| fix.kind.as_str()), + Some("dedupe-package") + ); + + let dev_only = &analysis.duplicate_packages[2].finding.recommended_fix; + assert_ne!( + dev_only.as_ref().map(|fix| fix.kind.as_str()), + Some("dedupe-package") + ); +} + #[test] fn apply_action_plan_clears_stale_metadata_before_reapplying() { let mut analysis = Analysis { @@ -447,10 +554,50 @@ fn duplicate_package( name: &str, estimated_extra_kb: usize, confidence: FindingConfidence, +) -> DuplicatePackage { + duplicate_package_with_scope( + finding_id, + name, + estimated_extra_kb, + confidence, + DuplicateImpactScope::Unknown, + ) +} + +fn duplicate_package_with_scope( + finding_id: &str, + name: &str, + estimated_extra_kb: usize, + confidence: FindingConfidence, + impact_scope: DuplicateImpactScope, +) -> DuplicatePackage { + duplicate_package_with_versions_and_scope( + finding_id, + name, + estimated_extra_kb, + confidence, + &["1.0.0", "1.1.0"], + impact_scope, + ) +} + +fn duplicate_package_with_versions_and_scope( + finding_id: &str, + name: &str, + estimated_extra_kb: usize, + confidence: FindingConfidence, + versions: &[&str], + impact_scope: DuplicateImpactScope, ) -> DuplicatePackage { DuplicatePackage { name: name.to_string(), + versions: versions + .iter() + .map(|version| (*version).to_string()) + .collect(), + count: versions.len(), estimated_extra_kb, + impact_scope, finding: finding(finding_id, confidence), ..Default::default() }