feat(analyzer): improve fallback diff summarization - #13
Conversation
|
@Swathi-Chippa is attempting to deploy a commit to the nirvik34's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Warning
|
| Layer / File(s) | Summary |
|---|---|
FileChange type and path/diff classifiers src/analyzer/fileFilter.ts, src/analyzer/fileFilter.test.ts |
Adds FileChange type, isGeneratedFile, isSnapshotFile, diff parsing and isFormattingOnlyDiff, plus filterLowSignalFiles and tests. |
File scoring and prioritization src/analyzer/fileScorer.ts, src/analyzer/fileScorer.test.ts |
Adds scoring helpers (path normalization, type detectors), countFunctionSignals, scoreFile, scoreAll, and sortBySignal, with tests for scoring and sort order. |
Grouping and deduplication src/analyzer/fileDeduplicator.ts, src/analyzer/fileDeduplicator.test.ts |
Introduces FileGroup/DeduplicatedResult, getExtension, groupByDirectory, deduplicateFiles (extension-based groups, directory fallback, representative selection), and tests covering grouping scenarios and edge cases. |
Summary formatting and CLI integration src/analyzer/summarizer.ts, src/index.ts |
Updates generateSummary signature, adds generateSummaryFromResult, adds getDiffForFile, and wires the pipeline in run() (filter → sortBySignal → deduplicate → summarize) with fallback behavior. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
- Add Offline Summary Compression #6: Implements offline heuristics for filtering generated/formatting-only files, scoring/prioritizing changes, and deduplicating/grouping files as requested.
Poem
🐰 I hopped through diffs both near and far,
Sniffed out the noise and found each bright star,
I grouped same-typed files by folder and name,
Chose the loudest change to carry the claim,
Now commit summaries hum with tidy flame. ✨
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'feat(analyzer): improve fallback diff summarization' directly describes the primary change: enhancing the fallback summarizer component with filtering, scoring, and deduplication logic. |
| Linked Issues check | ✅ Passed | All core objectives from issue #6 are met: filtering low-signal files, scoring by importance, deduplicating repetitive patterns, and excluding generated files. |
| Out of Scope Changes check | ✅ Passed | All changes align with issue #6 objectives; no unrelated refactoring or modifications outside the scope of improving fallback summarization. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/index.ts (1)
76-76: ⚡ Quick winExtract magic number to a named constant.
The
2parameter passed todeduplicateFileslacks context. Consider extracting it to a named constant to clarify its meaning.📝 Proposed refactor
+const MIN_GROUP_SIZE = 2; + function getDiffForFile(path: string): string { ... } export async function run(options: CliOptions) { ... - const deduplicatedResult = deduplicateFiles(prioritizedFiles, 2); + const deduplicatedResult = deduplicateFiles(prioritizedFiles, MIN_GROUP_SIZE);🤖 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/index.ts` at line 76, The call to deduplicateFiles uses a magic number (2) with no context; replace it with a descriptive named constant (e.g., const MAX_DUPLICATES = 2 or const DEDUPLICATION_THRESHOLD = 2) declared near where prioritizedFiles is defined or at top-level, then pass that constant into deduplicateFiles (keep deduplicatedResult and prioritizedFiles unchanged); update any nearby comments to reflect the constant's intent so the meaning of the parameter is clear.
🤖 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/analyzer/fileFilter.ts`:
- Around line 68-71: The current call to childProcess.execSync builds a
shell-interpolated command using `path`, creating a command-injection risk;
replace it with `childProcess.execFileSync` (or `spawnSync` equivalent) and pass
the executable and arguments as an array (e.g., "git" and
["diff","--cached","-U0","--", path]) so the path is not shell-parsed; keep the
same options (`encoding: "utf8"`, `stdio: ["ignore","pipe","ignore"]`) and
return the result from the same location in src/analyzer/fileFilter.ts where
`childProcess.execSync` is used.
In `@src/index.ts`:
- Around line 31-40: Replace the vulnerable shell interpolation in
getDiffForFile by calling childProcess.execFileSync with "git" as the command
and an argument array like ["diff", "--cached", "-U0", "--", path] (preserve
options encoding: "utf8" and stdio as before) and keep the try/catch behavior;
apply the identical change to getStagedDiffForFile so both functions pass the
file path as an argument array instead of embedding it in a command string.
---
Nitpick comments:
In `@src/index.ts`:
- Line 76: The call to deduplicateFiles uses a magic number (2) with no context;
replace it with a descriptive named constant (e.g., const MAX_DUPLICATES = 2 or
const DEDUPLICATION_THRESHOLD = 2) declared near where prioritizedFiles is
defined or at top-level, then pass that constant into deduplicateFiles (keep
deduplicatedResult and prioritizedFiles unchanged); update any nearby comments
to reflect the constant's intent so the meaning of the parameter is clear.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1974776a-9d98-460b-9487-cf0f4db39e04
📒 Files selected for processing (8)
src/analyzer/fileDeduplicator.test.tssrc/analyzer/fileDeduplicator.tssrc/analyzer/fileFilter.test.tssrc/analyzer/fileFilter.tssrc/analyzer/fileScorer.test.tssrc/analyzer/fileScorer.tssrc/analyzer/summarizer.tssrc/index.ts
|
Addressed the two critical review comments from CodeRabbit — replaced execSync with execFileSync and an argument array in both src/analyzer/fileFilter.ts and src/index.ts to eliminate the command injection risk. All 21 tests still passing. Ready for review. |
|
@Swathi-Chippa Also mention the issue the PR is targeting or supposed to close. |
|
Hi @nirvik34 , just checking in — the PR is ready for review. I've addressed the CodeRabbit security comments and linked the issue. Let me know if anything else is needed! |
nirvik34
left a comment
nirvik34
left a comment
There was a problem hiding this comment.
Hey @Swathi-Chippa , sorry for the delay
Two things needed before this can merge:
- Magic number : comment on line 76 is still open. Please define
const MIN_GROUP_SIZE = 2above thededuplicateFilescall and pass that in instead of the literal2.
it should be
const MIN_GROUP_SIZE = 2;
const deduplicatedResult = deduplicateFiles(prioritizedFiles, MIN_GROUP_SIZE);
- Merge conflict —
src/index.tshas a conflict that needs to be resolved before GitHub will allow a merge.
Once those are addressed this should be ready to go!
| const prioritizedFiles = prioritizedCandidates.length > 0 | ||
| ? prioritizedCandidates | ||
| : enrichedFiles; | ||
| const deduplicatedResult = deduplicateFiles(prioritizedFiles, 2); |
There was a problem hiding this comment.
You should define a constant named MIN_GROUP_SIZE (or something similar) right above it, and then pass that constant into the function instead of the number 2 directly into the function.
There was a problem hiding this comment.
Fixed — defined MIN_GROUP_SIZE constant and resolved the merge conflict in src/index.ts. All 21 tests passing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/index.ts`:
- Around line 85-87: The call to generateCommitMessage is dropping
user-configured templates because it no longer passes config.format; update the
invocation (after loadConfig()) to pass config.format as the fourth argument so
generateCommitMessage(type, scope, prioritizedFiles, config.format) is used;
reference generateCommitMessage, loadConfig, and config.format when locating and
fixing the call.
- Around line 81-87: The non-AI fallback still builds commit text from
prioritizedFiles instead of using the deduplicated summary; change the logic
around generateCommitMessage so that when options.ai is false it uses the
summary produced by generateSummaryFromResult(deduplicatedResult) (variable
summary) as the input/context for generateCommitMessage (or otherwise
incorporate summary into the fallback message generation), ensuring the
deduplication/grouping (deduplicatedResult -> summary) affects the non-AI path;
update the conditional that checks options.ai to reference summary instead of
only prioritizedFiles and keep function names generateSummaryFromResult,
generateCommitMessage, deduplicatedResult and options.ai to locate the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const summary = generateSummaryFromResult(deduplicatedResult); | ||
|
|
||
|
|
||
| // Load config | ||
| const config = await loadConfig(); | ||
|
|
||
| let commitMessage = generateCommitMessage( | ||
| type, | ||
| scope, | ||
| enrichedFiles, | ||
| config.format | ||
| ); | ||
| let commitMessage = generateCommitMessage(type, scope, prioritizedFiles); |
There was a problem hiding this comment.
Use the deduplicated summary in the non-AI path.
generateSummaryFromResult(deduplicatedResult) is only used as AI context here. When options.ai is false, the commit message is still built from prioritizedFiles, so the new grouping/deduplication pipeline does not actually change the fallback commit text for large PRs.
🤖 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/index.ts` around lines 81 - 87, The non-AI fallback still builds commit
text from prioritizedFiles instead of using the deduplicated summary; change the
logic around generateCommitMessage so that when options.ai is false it uses the
summary produced by generateSummaryFromResult(deduplicatedResult) (variable
summary) as the input/context for generateCommitMessage (or otherwise
incorporate summary into the fallback message generation), ensuring the
deduplication/grouping (deduplicatedResult -> summary) affects the non-AI path;
update the conditional that checks options.ai to reference summary instead of
only prioritizedFiles and keep function names generateSummaryFromResult,
generateCommitMessage, deduplicatedResult and options.ai to locate the code.
|
Hi @nirvik34 , just checking in — the PR is ready for review. Let me know if anything else is needed! |
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
nirvik34
added
level: advanced
2 participants
feat(analyzer): smart fallback summarizer for offline mode
Problem
The fallback summarizer treated every file change equally. On large PRs this produced noisy, generic commit messages dominated by snapshots, formatting changes, lock files, and generated files.
Solution
Three new modular analyzers added to the fallback pipeline:
Pipeline:
enrichedFiles→filterLowSignalFiles→sortBySignal→deduplicateFiles→ commit messagefileFilter.tsfileScorer.tsfileDeduplicator.tssrc/components/*.tsxWhat's safe
generateSummary(files)signature preservedenrichedFiles— commit message is never emptyFileChangetypeKnown limitation (follow-up)
classifyCommitTypestill callsscanDiff()on the full staged diff, not the filtered file list. Behaviour is unchanged from before this PR — no regression — but aligning it with the new pipeline is a good follow-up issue.Summary by CodeRabbit
New Features
Refactor
Tests
Closes #6