fix(ci): handle filtered size-limit results correctly#715
Conversation
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughReplace previous hasErrors-only decision with multi-branch logic that derives Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Action Runner
participant SizeLimit as size-limit process
participant Index as index.ts (action logic)
participant Output as output.ts (setGitHubOutputs)
participant GitHub as GitHub Outputs / Actions
Runner->>SizeLimit: execute size-limit
SizeLimit-->>Index: return results (+ errors info)
Index->>Index: compute filteredResults, hasErrors, hasRelevantErrors, shouldFail
Index->>Output: setGitHubOutputs(result, hasRelevantErrors, packagesChanged)
Index->>GitHub: emit logs (info/warn/fail)
Index-->>Runner: exit (0 if !shouldFail, non-zero if shouldFail)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
- Handle case where size-limit results are filtered out due to package changes - Don't fail when results exist but don't match changed packages - Only fail when size-limit truly fails or limits are exceeded for changed packages - Add clearer logging to distinguish different "no results" scenarios This fixes failures in PRs where size-limit runs on packages but those packages weren't actually changed, causing filtered results to be empty. Co-authored-by: yamcodes <2014360+yamcodes@users.noreply.github.com>
- Rename finalHasErrors to hasRelevantErrors for clarity - Add detailed comments explaining the error handling logic - Distinguish between "should we fail" and "should we report errors" - Make it clear that errors are only relevant if they apply to filtered results Addresses code review feedback about variable naming and semantics. Co-authored-by: yamcodes <2014360+yamcodes@users.noreply.github.com>
github-actions
bot
added
the
github actions
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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
🤖 Fix all issues with AI agents
In @.github/actions/size-limit/src/index.ts:
- Around line 83-125: The logic when filteredResults.length > 0 incorrectly uses
the global hasErrors flag to decide failure reporting; change it to inspect the
filteredResults array for per-result failures by setting hasRelevantErrors =
filteredResults.some(r => r.status === "❌") and then shouldFail =
hasRelevantErrors so only failures among the filtered set cause the workflow to
fail; update the block that currently assigns hasRelevantErrors = hasErrors and
shouldFail = hasErrors to use filteredResults instead (refer to symbols
filteredResults, hasErrors, hasRelevantErrors, shouldFail).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/actions/size-limit/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
.github/actions/size-limit/src/index.ts (1)
.github/actions/size-limit/src/output.ts (1)
setGitHubOutputs(24-37)
🪛 GitHub Actions: autofix.ci
.github/actions/size-limit/src/index.ts
[error] 1-1: The autofix.ci action is not allowed to modify the .github directory.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-e2e (a11y)
- GitHub Check: test-e2e (e2e)
- GitHub Check: Deploy-Preview
🔇 Additional comments (2)
.github/actions/size-limit/src/index.ts (2)
127-129: LGTM!The change correctly propagates
hasRelevantErrorsto GitHub outputs, ensuring downstream consumers (PR comments, status checks) only see errors relevant to the changed packages.
131-137: LGTM!The exit logic now correctly uses
shouldFailto determine workflow outcome, decoupling the exit decision from the rawhasErrorsflag.
- Changed failure logic to check for explicit failures (❌) - Ensures CI fails only when size-limit has actual errors - Handles cases where size-limit runs but finds no configs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/actions/size-limit/src/index.ts (2)
98-132: Comment says “don’t report”, but outputs are still written unconditionally.
The branches explicitly say “don’t report” (Lines 102-103, 114-116), but you still always compute/print the table (Lines 81-83) and always callsetGitHubOutputs(...)(Line 131). If downstream steps use outputs to decide PR comments, you may still emit an empty/irrelevant comment (especially whenfilteredResultsis empty butpackagesChanged === true).
Consider either:
- early-returning before table/output in the “no relevant results” branches, or
- adding a
shouldReportflag and gatingsetGitHubOutputs(and possiblyconsole.log(result)).Proposed minimal gating refactor (keeps current logic intact)
@@ -// Create the table -const result = createTable(filteredResults); -console.log(result); +// Create the table (but only report it when we actually have relevant results) +const result = createTable(filteredResults); @@ let shouldFail = false; let hasRelevantErrors = false; +let shouldReport = true; if (filteredResults.length === 0) { if (results.length > 0) { @@ console.log( "ℹ️ Size checks ran but no results matched changed packages. This is expected when unchanged packages have size-limit configs.", ); hasRelevantErrors = false; shouldFail = false; + shouldReport = false; } else if (hasErrors) { @@ hasRelevantErrors = true; shouldFail = true; + shouldReport = true; } else { @@ console.log( "ℹ️ No size-limit results found. Packages may not have size-limit configured.", ); hasRelevantErrors = false; shouldFail = false; + shouldReport = false; } } else { @@ hasRelevantErrors = filteredResults.some((r) => r.status === "❌"); shouldFail = hasRelevantErrors; } + +if (shouldReport) { + console.log(result); +} // Set GitHub outputs const packagesChanged = changedPackages === null || changedPackages.size > 0; -await setGitHubOutputs(result, hasRelevantErrors, packagesChanged); +if (shouldReport) { + await setGitHubOutputs(result, hasRelevantErrors, packagesChanged); +}
123-127: Brittle error detection viastatus === "❌"string.
Ifstatusis user-facing (emoji) rather than a stable enum, this can silently break failure detection. Prefer a typed status enum/value (e.g.,"ok" | "fail") and render emoji at the presentation layer (createTable).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/actions/size-limit/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
.github/actions/size-limit/src/index.ts (1)
.github/actions/size-limit/src/output.ts (1)
setGitHubOutputs(24-37)
🔇 Additional comments (1)
.github/actions/size-limit/src/index.ts (1)
83-132: No issues found. The flagged critical issue does not exist.The call
setGitHubOutputs(result, hasRelevantErrors, packagesChanged)on line 131 correctly matches the function signature inoutput.tslines 24-27: parameters are passed in the correct order with matching types (string, boolean, boolean). Parameter names do not enforce type matching in TypeScript—only order and type do.The semantic use of
hasRelevantErrorsas the second argument (which the function parameter nameshasErrors) is intentional and correct. The GitHub action outputhas_errorsis set to the value ofhasRelevantErrors, accurately reflecting whether there are errors relevant to the PR being checked.The decision logic (lines 98–127) correctly distinguishes between execution failures (
hasErrorsfromrunSizeLimit) and business-logic failures (status checks in filtered results), ensuring that irrelevant errors from unchanged packages don't cause false positives.
- Report config errors even if no limits are breached - Clarify when to fail CI based on errors - Improve message for filtered results
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/actions/size-limit/src/index.ts (1)
134-139: Consider detecting config errors when filtered results exist.This branch only checks for ❌ in
filteredResultsand ignores the globalhasErrorsflag. If size-limit had a config/execution error but the checked packages all passed, the error would go unreported.You could apply the same heuristic used above for consistency:
} else { // We have filtered results - check if any failed hasRelevantErrors = filteredResults.some((r) => r.status === "❌"); // Also detect config errors: hasErrors is true but no limits breached const allResultsPassed = results.every((r) => r.status !== "❌"); if (!hasRelevantErrors && hasErrors && allResultsPassed) { console.log("⚠️ Size limit had a configuration or execution error."); hasRelevantErrors = true; } shouldFail = hasRelevantErrors; }This matches the PR's stated objectives ("fail only when filtered results contain errors OR when size-limit execution failed with no results"), so this is optional. The current code is correct per those objectives.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/actions/size-limit/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
.github/actions/size-limit/src/index.ts (1)
.github/actions/size-limit/src/output.ts (1)
setGitHubOutputs(24-37)
🔇 Additional comments (3)
.github/actions/size-limit/src/index.ts (3)
83-96: Clear documentation of the decision logic.The inline comments thoroughly explain the multi-branch decision flow. The variable names
shouldFailandhasRelevantErrorsclearly distinguish between workflow exit behavior and error reporting state.
98-133: Logic correctly handles the filtered-to-empty scenarios.The heuristic at line 103-104 is sound: when
hasErrorsis true but all results passed, the error must stem from a configuration or execution issue rather than a limit breach. This correctly distinguishes:
- Config/execution errors → fail (lines 104-109)
- Limit breaches in unchanged packages filtered out → pass (lines 110-119)
- No results + execution failure → fail (lines 120-124)
- No results + success (no configs) → pass (lines 125-133)
143-151: Correctly propagates the new error semantics.Passing
hasRelevantErrorstosetGitHubOutputsensures the GitHub workflow outputs reflect only errors relevant to the PR's changed packages. UsingshouldFailfor the exit condition correctly decouples the workflow exit status from the rawhasErrorsflag.
- Detects when size limit had errors but no limits breached - Marks action as failed if config/execution errors exist - Improves error reporting for size limit action
The size-limit action fails when results are filtered out due to package changes, treating filtered-empty results as a failure even when size-limit executed successfully on unchanged packages.
Changes
hasRelevantErrors: Separate "size-limit had errors" from "errors relevant to this PR's changed packages"Scenarios Fixed
Before:
@arkenv/vite-pluginarkenv(unchanged package)After:
Still fails correctly:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.