Fix unit-inconsistent adoption metric: count drifted components, not findings#5
Merged
Merged
Conversation
…findings
The "Design system adoption: NN%" headline divided canonical DS usages
(import specifiers) by the raw drift FINDING count. But one drifting local
component emits 3-4 findings at once (token-fingerprint + prop-match +
subcomponent), so a single drifted component depressed adoption 3-4x harder
than one correct import raised it — the ratio wasn't comparing like with like.
Redefine the metric at component granularity:
canonicalUsages / (canonicalUsages + driftedComponents)
`countDriftedComponents` dedups findings by (file, key): every inline rule
keys on the local component name, so its multiple signals fold back into one
component; import-drift keys on the wrongly-sourced specifier, already one
drifted DS component per entry (the counterpart to one canonical import).
analyze.ts now uses it for both head adoption and the base-branch delta.
Documents the definition in adoption.ts / findings.ts and adds tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces ChangesComponent-level adoption metric
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
tests/comment-analyze.test.ts (1)
32-52: ⚡ Quick winAdd a regression case for
readBase+ suppressions affecting adoption delta.This suite now validates component-level head adoption well; please add one case asserting base/head parity when suppress rules are active, so
adoptionDeltaPctcan’t regress silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/comment-analyze.test.ts` around lines 32 - 52, Add a new test case after the existing test that validates adoption delta calculation when suppressions or rules are active. Create a scenario using both readBase and readCurrent options in the base() configuration where one version has suppressions applied and the other doesn't, then assert that adoptionDeltaPct is correctly calculated and doesn't regress. Reference the analyzePr function call pattern and adoption percentage expectations from the existing test to structure the new regression case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/comment/analyze.ts`:
- Around line 66-77: The baseAdopt calculation uses unsuppressed findings from
the baseFindings array, while headAdopt uses suppressed findings, causing
inconsistent adoption percentage calculations. Apply the same suppression logic
to baseFindings (similar to how suppressions are applied to the head findings)
before passing the suppressed findings to countDriftedComponents when computing
baseAdopt. This ensures that adoptionDeltaPct is calculated consistently with
the same suppression rules applied to both base and head findings.
---
Nitpick comments:
In `@tests/comment-analyze.test.ts`:
- Around line 32-52: Add a new test case after the existing test that validates
adoption delta calculation when suppressions or rules are active. Create a
scenario using both readBase and readCurrent options in the base() configuration
where one version has suppressions applied and the other doesn't, then assert
that adoptionDeltaPct is correctly calculated and doesn't regress. Reference the
analyzePr function call pattern and adoption percentage expectations from the
existing test to structure the new regression case.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c59e383c-7735-4ea7-8866-94d8832d8b97
⛔ Files ignored due to path filters (2)
dist/cli/index.jsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (5)
src/comment/adoption.tssrc/comment/analyze.tssrc/comment/findings.tstests/comment-analyze.test.tstests/comment-core.test.ts
Comment on lines
+66
to
+77
| const baseFindings: Finding[] = []; | ||
| for (const file of p.files) { | ||
| const base = p.readBase(file); | ||
| if (base == null) continue; | ||
| const res = checkDriftFull(base, p.dsExports, p.canonicalPkgs, p.allowlist, file); | ||
| const baseFindings = flattenFindings(file, res); | ||
| for (const f of baseFindings) preexistingIds.add(f.id); | ||
| baseDrift += baseFindings.length; | ||
| const ff = flattenFindings(file, res); | ||
| for (const f of ff) preexistingIds.add(f.id); | ||
| baseFindings.push(...ff); | ||
| baseCanonical += countCanonicalUsages(base, p.dsExports, p.canonicalPkgs); | ||
| } | ||
| const baseAdopt = adoptionPct(baseCanonical, baseDrift); | ||
| const headAdopt = adoptionPct(canonicalUsages, findings.length); | ||
| const baseAdopt = adoptionPct(baseCanonical, countDriftedComponents(baseFindings)); | ||
| const headAdopt = adoptionPct(canonicalUsages, driftedComponents); |
There was a problem hiding this comment.
Apply suppressions to base findings before computing base adoption.
headAdopt uses suppressed findings, but baseAdopt currently uses unsuppressed findings. With readBase + active suppress rules, adoptionDeltaPct can be materially incorrect even when no real drift changed.
Suggested patch
const baseAdopt = adoptionPct(baseCanonical, countDriftedComponents(baseFindings));
+ const baseFindingsForAdoption = applySuppressions(baseFindings, p.suppress);
+ const baseAdopt = adoptionPct(baseCanonical, countDriftedComponents(baseFindingsForAdoption));
const headAdopt = adoptionPct(canonicalUsages, driftedComponents);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/comment/analyze.ts` around lines 66 - 77, The baseAdopt calculation uses
unsuppressed findings from the baseFindings array, while headAdopt uses
suppressed findings, causing inconsistent adoption percentage calculations.
Apply the same suppression logic to baseFindings (similar to how suppressions
are applied to the head findings) before passing the suppressed findings to
countDriftedComponents when computing baseAdopt. This ensures that
adoptionDeltaPct is calculated consistently with the same suppression rules
applied to both base and head findings.
…omponent-level # Conflicts: # dist/cli/index.js # dist/index.js # src/comment/analyze.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (review finding #10)
The Design system adoption: NN% headline in the PR comment was unit-inconsistent. In
src/comment/adoption.ts+src/comment/analyze.tsit was computed as:canonicalUsagescounts import specifiers (one per DS component used correctly, fromcountCanonicalUsages), butdriftSignalscounted findings. One drifting local component emits 3-4 findings at once (token-fingerprint + prop-match + subcomponent), so a single drifted component depressed adoption 3-4× harder than one correct import raised it. The headline wasn't a meaningful ratio.Fix
Redefine the metric at component granularity so numerator and denominator compare like with like:
New
countDriftedComponents(findings)(infindings.ts) dedups findings by(file, key):Button from './ui'), which is already one drifted DS component per entry — the natural counterpart to one canonical import specifier.analyze.tsuses it for both the head adoption headline and the base-branch delta. The definition is documented inadoption.tsandfindings.ts.Example
A
ProductCardthat trips 3 inline signals against 1 canonical (Tile) import now reads 50% (1 drifted component vs 1 canonical), not 25% (1 / (1+3)) as before.Tests
tests/comment-core.test.ts— newcountDriftedComponentsblock (folds inline signals to one component, counts each import-drift specifier, file-scoped, empty case).tests/comment-analyze.test.ts— end-to-end: one multi-signal local component → 3 findings but 50% adoption.npm run typecheck && npm test && npm run build:allall pass (122 tests);dist/rebuilt and committed.🤖 Generated with Claude Code
Summary by CodeRabbit