optimize: find the parent commits when no latest commit's baseline#19
optimize: find the parent commits when no latest commit's baseline#19
Conversation
Rsdoctor Bundle Diff AnalysisFound 2 project(s) in monorepo. 📁 rsbuild-demoPath:
📦 Download Diff Report: rsbuild-demo Bundle Diff 📁 rsbuild-demo2Path:
📦 Download Diff Report: rsbuild-demo2 Bundle Diff Generated by Rsdoctor GitHub Action |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the baseline commit selection logic by implementing a parent commit traversal mechanism when the latest commit lacks baseline artifacts. Instead of immediately using a commit without artifacts, the code now traverses up to 5 parent commits to find one with available baseline artifacts.
Key changes:
- Added helper methods to check for artifacts and retrieve parent commits
- Refactored commit retrieval to defer early returns and enable artifact validation
- Implemented parent commit traversal with depth limiting and circular reference protection
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/github.ts | Added hasArtifactsForCommit and getParentCommit helper methods; refactored getTargetBranchLatestCommit to check for artifacts and traverse parent commits when needed |
| dist/index.js | Compiled JavaScript output matching the TypeScript source changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } catch (error) { | ||
| // Continue checking other workflow runs | ||
| continue; |
There was a problem hiding this comment.
The continue statement is redundant here. Since continue is the default behavior at the end of a for loop iteration, it can be removed without changing functionality.
| continue; |
| 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 only retrieves the first parent commit. In merge commits with multiple parents, this could lead to unexpected behavior. Consider documenting this assumption or adding logic to handle merge commits appropriately. The method should clarify whether it's intended to follow the first parent (main branch) or if there's specific merge commit handling needed.
|
|
||
| let currentCommit = latestCommitHash; | ||
| let checkedCommits: string[] = [currentCommit]; | ||
| const maxDepth = 5; |
There was a problem hiding this comment.
The magic number 5 for maxDepth lacks explanation. Consider adding a comment explaining why 5 commits is an appropriate depth limit, or making this configurable. This helps future maintainers understand the rationale behind this value.
|
|
||
| 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; |
There was a problem hiding this comment.
The hasArtifactsForCommit method could potentially make many API calls sequentially, which may impact performance and could hit rate limits. For a commit with many workflow runs, this could be slow. Consider adding pagination limits, caching results, or implementing parallel checks with Promise.all() for better performance.
| 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; | |
| // Check all workflow runs in parallel for artifacts | |
| const artifactChecks = await Promise.all( | |
| workflowRuns.map(async (workflowRun) => { | |
| try { | |
| const runArtifacts = await this.listArtifactsForWorkflowRun(workflowRun.id); | |
| return runArtifacts.artifacts && runArtifacts.artifacts.length > 0; | |
| } catch (error) { | |
| // Treat errors as no artifacts for this run | |
| return false; | |
| } | |
| }) | |
| ); | |
| return artifactChecks.some(Boolean); |
| for (let depth = 0; depth < maxDepth; depth++) { | ||
| const parentCommit = await this.getParentCommit(currentCommit); | ||
|
|
||
| console.log(`📥 Running: git rev-parse --short=10 origin/${targetBranch}`); | ||
| const commitHash = execSync(`git rev-parse --short=10 origin/${targetBranch}`, { encoding: 'utf8' }).trim(); | ||
| console.log(`✅ Found commit hash from git: ${commitHash}`); | ||
| return commitHash; | ||
| } catch (gitError) { | ||
| console.warn(`❌ Git fetch failed: ${gitError}`); | ||
| if (!parentCommit) { | ||
| console.log(`⚠️ Reached the beginning of the branch, no more parent commits`); | ||
| break; | ||
| } | ||
|
|
||
| try { | ||
| console.log(`📥 Trying alternative: git ls-remote origin ${targetBranch}`); | ||
| const remoteRef = execSync(`git ls-remote origin ${targetBranch}`, { encoding: 'utf8' }).trim(); | ||
| if (remoteRef) { | ||
| const commitHash = remoteRef.split('\t')[0].substring(0, 10); | ||
| console.log(`✅ Found commit hash from git ls-remote: ${commitHash}`); | ||
| return commitHash; | ||
| } | ||
| } catch (altError) { | ||
| console.warn(`❌ Alternative git command failed: ${altError}`); | ||
| if (checkedCommits.includes(parentCommit)) { | ||
| console.log(`⚠️ Detected circular reference, stopping search`); | ||
| break; | ||
| } | ||
|
|
||
| checkedCommits.push(parentCommit); | ||
| console.log(`🔍 Checking parent commit ${parentCommit}...`); | ||
|
|
||
| const parentHasArtifacts = await this.hasArtifactsForCommit(parentCommit); | ||
|
|
||
| if (parentHasArtifacts) { | ||
| console.log(`✅ Found commit ${parentCommit} with baseline artifacts`); | ||
| console.log(`\n⚠️ Note: The latest commit (${latestCommitHash}) does not have baseline artifacts.`); | ||
| console.log(` Using commit ${parentCommit} for baseline comparison instead.`); | ||
| console.log(` If this seems incorrect, please wait a few minutes and try rerunning the workflow.`); | ||
| return parentCommit; | ||
| } | ||
|
|
||
| currentCommit = parentCommit; | ||
| } |
There was a problem hiding this comment.
The parent commit traversal loop could make up to 5 sequential API calls (getParentCommit) followed by potentially expensive hasArtifactsForCommit checks. This sequential approach could be slow and may hit API rate limits. Consider adding delays between checks, implementing exponential backoff, or optimizing the artifact checking logic to reduce the number of API calls.
No description provided.