feat: support collapsed CI card & support find the parent commits#22
feat: support collapsed CI card & support find the parent commits#22
Conversation
Rsdoctor Bundle Diff AnalysisFound 2 project(s) in monorepo. 📊 Quick Summary (Click to expand)
📋 Detailed Reports (Click to expand)📁 rsbuild-demoPath:
📦 Download Diff Report: rsbuild-demo Bundle Diff 📁 rsbuild-demo2Path:
📦 Download Diff Report: rsbuild-demo2 Bundle Diff Generated by Rsdoctor GitHub Action |
yifancong
changed the title
yifancong
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds support for collapsible sections in PR comments and implements an intelligent fallback mechanism for baseline artifact resolution. When the latest commit on the target branch lacks baseline artifacts, the system now searches through up to 5 parent commits to find a suitable baseline, improving the reliability of bundle analysis comparisons.
Key Changes:
- Adds fallback logic to search parent commits when baseline artifacts are missing from the latest commit
- Introduces collapsible UI sections for better organization of multi-project reports
- Exports utility functions (
formatBytesandcalculateDiff) for reuse across the codebase
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/report.ts | Exports formatBytes and calculateDiff functions for use in generating the Quick Summary table |
| src/github.ts | Adds hasArtifactsForCommit and getParentCommit methods; updates getTargetBranchLatestCommit to return an object with fallback information and implement parent commit searching |
| src/index.ts | Updates ProjectReport interface with fallback fields; adds Quick Summary collapsible section; propagates fallback information through the processing pipeline; displays warning notices when using fallback commits |
| src/tests/github.test.ts | Adds tests for the updated getTargetBranchLatestCommit return type, covering both normal and fallback scenarios |
| dist/index.js | Compiled JavaScript output reflecting all source code changes |
Comments suppressed due to low confidence (1)
src/index.ts:198
- Unused variable targetArtifactName.
const targetArtifactName = `${pathParts.join('-')}-${fileNameWithoutExt}-${targetCommitHash}${fileExt}`;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| function formatBytes(bytes: number): string { | ||
| export function formatBytes(bytes: number): string { |
There was a problem hiding this comment.
The newly exported functions formatBytes and calculateDiff are now being used directly in index.ts but lack dedicated test coverage. While these functions are indirectly tested through the existing integration tests, adding explicit unit tests would improve test coverage and ensure their behavior is properly validated, especially for edge cases like negative numbers, zero values, and very large/small numbers.
| } | ||
|
|
||
| function calculateDiff(current: number, baseline: number): { value: string; emoji: string } { | ||
| export function calculateDiff(current: number, baseline: number): { value: string; emoji: string } { |
There was a problem hiding this comment.
The newly exported function calculateDiff lacks dedicated test coverage. While it's indirectly tested through integration tests, adding explicit unit tests would improve test coverage and ensure proper behavior for edge cases such as when baseline is 0, NaN, or when dealing with very small percentage differences.
| async hasArtifactsForCommit(commitHash: string): Promise<boolean> { | ||
| try { | ||
| const workflowRuns = await this.findAllWorkflowRunsByCommit(commitHash); | ||
|
|
||
| for (const workflowRun of workflowRuns) { | ||
| try { | ||
| const runArtifacts = await this.listArtifactsForWorkflowRun(workflowRun.id); | ||
| if (runArtifacts.artifacts && runArtifacts.artifacts.length > 0) { | ||
| return true; | ||
| } | ||
| } catch (error) { | ||
| // Continue checking other workflow runs | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } catch (error) { | ||
| // If we can't check, assume no artifacts | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new method hasArtifactsForCommit lacks dedicated test coverage. While it's used within the tested getTargetBranchLatestCommit method, adding explicit unit tests would improve confidence in its behavior, especially for edge cases like when a commit has multiple workflow runs with varying artifact states, or when API calls fail.
| async getParentCommit(commitHash: string): Promise<string | null> { | ||
| const { owner, repo } = this.repository; | ||
|
|
||
| try { | ||
| const commitResponse = await this.octokit.rest.repos.getCommit({ | ||
| owner, | ||
| repo, | ||
| ref: commitHash | ||
| }); | ||
|
|
||
| if (commitResponse.data.parents && commitResponse.data.parents.length > 0) { | ||
| return commitResponse.data.parents[0].sha.substring(0, 10); | ||
| } | ||
|
|
||
| return null; | ||
| } catch (error) { | ||
| const apiError = error as ApiError; | ||
| console.warn(`⚠️ Failed to get parent commit for ${commitHash}: ${apiError.message}`); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new method getParentCommit lacks dedicated test coverage. While it's used within the tested getTargetBranchLatestCommit method, adding explicit unit tests would ensure proper handling of edge cases such as commits with no parents, commits with multiple parents (merge commits), and API failures.
|
|
||
| let targetCommitHash: string | null = null; | ||
| let baselineUsedFallback = false; | ||
| let baselineLatestCommitHash: string | undefined = undefined; |
There was a problem hiding this comment.
The parameter baselineLatestCommitHash is declared as string | undefined on line 286, but it's assigned undefined explicitly. For consistency with TypeScript conventions and to avoid confusion, consider initializing optional values with no assignment or explicitly use undefined consistently throughout the codebase. The current approach is correct but could be clearer by removing the explicit assignment.
| let baselineLatestCommitHash: string | undefined = undefined; | |
| let baselineLatestCommitHash: string | undefined; |
| if (reportsWithCurrent.length > 0) { | ||
| commentBody += '<details>\n<summary><b>📊 Quick Summary</b> (Click to expand)</summary>\n\n'; |
There was a problem hiding this comment.
The Quick Summary section is always shown inside a <details> tag, even when there's only a single project report. For single-project scenarios, this creates an unnecessary collapsible section that users must expand to see basic summary information. Consider only wrapping the Quick Summary in <details> when there are multiple projects, similar to the logic used for the Detailed Reports section.
| commentBody += '|---------|------------|--------|\n'; | ||
|
|
||
| for (const report of reportsWithCurrent) { | ||
| if (!report.current) continue; |
There was a problem hiding this comment.
There's a redundant check if (!report.current) continue; on line 431 that is unnecessary because the loop already iterates over reportsWithCurrent, which is filtered to only include reports where r.current is truthy. This check will never evaluate to true and can be removed.
| if (!report.current) continue; |
| @@ -2,7 +2,7 @@ import { setFailed, getInput, summary } from '@actions/core'; | |||
| import { uploadArtifact, hashPath } from './upload'; | |||
There was a problem hiding this comment.
Unused import hashPath.
| import { uploadArtifact, hashPath } from './upload'; | |
| import { uploadArtifact } from './upload'; |
feat: support collapsed CI card
optimize: support find the parent commits when the latest commit don't have the baseline.