Optimize: collapse the useless info #23
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for collapsible sections in bundle analysis reports and implements a fallback mechanism to find baseline artifacts from parent commits when the latest commit lacks them. The changes improve the user experience by automatically expanding reports with significant changes while keeping minor changes collapsed, and by providing clearer feedback when baseline comparisons use fallback commits.
Key Changes
- Exports
formatBytesandcalculateDiffutility functions for reuse across modules - Implements fallback logic to search up to 5 parent commits when the latest commit doesn't have baseline artifacts
- Adds collapsible Quick Summary and Detailed Reports sections in PR comments, with automatic expansion based on change significance
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/report.ts | Exports utility functions (formatBytes, calculateDiff) and changes 'N/A' to '0' for missing baseline comparisons; removes emoji for changes < 1% |
| src/github.ts | Adds hasArtifactsForCommit and getParentCommit methods; modifies getTargetBranchLatestCommit to return object with fallback information and traverse up to 5 parent commits |
| src/index.ts | Adds fallback tracking fields to ProjectReport; implements collapsible sections with conditional expansion based on change significance (>= 1%); adds Quick Summary table for monorepo overview |
| src/tests/github.test.ts | Updates test to expect object return type from getTargetBranchLatestCommit; adds test case for fallback commit behavior when latest commit has no artifacts |
| dist/index.js | Compiled output reflecting source changes, including new methods and collapsible section logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Generate summary table for quick overview | ||
| if (reportsWithCurrent.length > 0) { | ||
| // Check if any project has significant changes (> 1% or no baseline) | ||
| let hasChanges = false; | ||
| for (const report of reportsWithCurrent) { | ||
| if (!report.current) continue; | ||
| if (!report.baseline) { | ||
| hasChanges = true; // No baseline means we can't compare, show it | ||
| break; | ||
| } | ||
| const currentSize = report.current.totalSize; | ||
| const baselineSize = report.baseline.totalSize; | ||
| if (baselineSize === 0 || isNaN(baselineSize)) continue; | ||
| const diff = currentSize - baselineSize; | ||
| const percent = Math.abs((diff / baselineSize) * 100); | ||
| if (percent >= 1) { | ||
| hasChanges = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| commentBody += generateProjectMarkdown(report.projectName, report.filePath, report.current, report.baseline || undefined, report.baselineCommitHash, report.baselinePRs); | ||
| // Use 'open' attribute if there are changes, otherwise keep it collapsed | ||
| const detailsTag = hasChanges ? '<details open>\n' : '<details>\n'; | ||
| commentBody += `${detailsTag}<summary><b>📊 Quick Summary</b></summary>\n\n`; | ||
| commentBody += '| Project | Total Size | Change |\n'; | ||
| commentBody += '|---------|------------|--------|\n'; | ||
|
|
||
| for (const report of reportsWithCurrent) { | ||
| if (!report.current) continue; | ||
| const currentSize = report.current.totalSize; | ||
| const baselineSize = report.baseline?.totalSize || 0; | ||
| const diff = report.baseline ? calculateDiff(currentSize, baselineSize) : { value: '-', emoji: '' }; | ||
| const sizeStr = formatBytes(currentSize); | ||
| commentBody += `| ${report.projectName} | ${sizeStr} | ${diff.emoji} ${diff.value} |\n`; | ||
| } | ||
|
|
||
| commentBody += '\n</details>\n\n'; |
There was a problem hiding this comment.
The logic for determining when to show the Quick Summary as open versus collapsed is duplicated. The hasChanges variable is calculated here (lines 427-443) to decide whether to use 'details open' or 'details', but the exact same logic is duplicated in the hasSignificantChanges helper function (lines 464-473). This code duplication makes the code harder to maintain and could lead to inconsistencies if one is updated without the other. Consider extracting this logic to use the hasSignificantChanges function instead.
| export function calculateDiff(current: number, baseline: number): { value: string; emoji: string } { | ||
| if (!baseline || baseline === 0 || isNaN(baseline)) { | ||
| return { value: 'N/A', emoji: '❓' }; | ||
| return { value: '0', emoji: '❓' }; | ||
| } | ||
|
|
||
| if (isNaN(current)) { | ||
| return { value: 'N/A', emoji: '❓' }; | ||
| return { value: '0', emoji: '❓' }; | ||
| } |
There was a problem hiding this comment.
When the baseline is 0 or NaN, the function returns '0' with a question mark emoji. However, this is semantically incorrect: a value of '0' suggests no change, but the question mark emoji suggests uncertainty or missing data. When baseline data is truly invalid or missing, returning 'N/A' (as before) would be more accurate. The '0' value could be misleading to users who might interpret it as "no change" rather than "unable to calculate".
| const hasSignificantChanges = (report: ProjectReport): boolean => { | ||
| if (!report.current) return false; | ||
| if (!report.baseline) return true; // No baseline means we can't compare, show it | ||
| const currentSize = report.current.totalSize; | ||
| const baselineSize = report.baseline.totalSize; | ||
| if (baselineSize === 0 || isNaN(baselineSize)) return false; | ||
| const diff = currentSize - baselineSize; | ||
| const percent = Math.abs((diff / baselineSize) * 100); | ||
| return percent >= 1; |
There was a problem hiding this comment.
The hasSignificantChanges function returns false when baselineSize is 0 or NaN (line 469), but this means that if a project goes from 0 bytes to having actual content, it won't be shown as a significant change. This seems counterintuitive - going from nothing to something should likely be considered a significant change and be displayed to users.
| if (commitResponse.data.parents && commitResponse.data.parents.length > 0) { | ||
| return commitResponse.data.parents[0].sha.substring(0, 10); | ||
| } |
There was a problem hiding this comment.
The getParentCommit method always returns the first parent (index 0) of a commit. For merge commits with multiple parents, this may not always be the desired parent to check for artifacts. In a typical Git workflow, merge commits have two parents - the first is usually the target branch, and the second is the merged branch. Depending on the branching strategy, you might want to consider checking both parents or documenting why only the first parent is used.
yifancong
force-pushed
the
feat/support-collapsed
branch
from
4310ddc to
45dda4e
Compare
Rsdoctor Bundle Diff AnalysisFound 2 project(s) in monorepo. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
No description provided.