ci: use trend images in bundle-size PR comments#7012
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR replaces inline ASCII sparklines with a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Script as Reporter Script
participant Service as Trend Graph Service
participant PR as PR Platform
Script->>Script: collect history -> build points [{x,y}...]
Script->>Service: request chart image URL with points
Service-->>Script: returns image URL (markdown)
Script->>PR: post report including "Trend chart" markdown image
PR-->>Script: stores/displays report
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 17f5e10
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/benchmarks/bundle-size/pr-report.mjs (1)
304-331:⚠️ Potential issue | 🟠 MajorDon't silently drop the current PR point when its timestamp is missing.
resolveCurrentTimestamp()can returnundefined, but Line 326 requires a finitex, so the current measurement is omitted from the trend instead of being shown as the final sample.Possible fix
- const currentPoint = { - x: currentTimestamp, - y: metric.gzipBytes, - } const lastPoint = historySeries[historySeries.length - 1] + const currentPoint = Number.isFinite(metric.gzipBytes) + ? { + x: + Number.isFinite(currentTimestamp) + ? currentTimestamp + : Number.isFinite(lastPoint?.x) + ? lastPoint.x + 1 + : 1, + y: Number(metric.gzipBytes), + } + : undefined if ( - Number.isFinite(currentPoint.x) && + currentPoint && (!lastPoint || lastPoint.x !== currentPoint.x || lastPoint.y !== currentPoint.y) ) { historySeries.push(currentPoint)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 304 - 331, The current check drops the currentPoint when resolveCurrentTimestamp(current) returns undefined because it requires Number.isFinite(currentPoint.x); update the conditional that guards pushing currentPoint so it allows a missing timestamp: replace the strict Number.isFinite(currentPoint.x) check with one that permits null/undefined timestamps (e.g., (currentPoint.x == null || Number.isFinite(currentPoint.x))) while keeping the duplicate-point avoidance logic that compares lastPoint.x/lastPoint.y; this ensures currentPoint (constructed in currentPoint) is appended to historySeries even when currentTimestamp is undefined.
🧹 Nitpick comments (1)
scripts/benchmarks/bundle-size/pr-report.mjs (1)
205-231: Sort each scenario by timestamp before trimming it.This function keeps whatever order the history feed arrives in, but Line 315 later slices from the end as if it were already chronological. With explicit
xvalues, a newest-first or partially reordered feed will draw backward in time and can trim the wrong points.Possible fix
function buildSeriesByScenario(historyEntries) { const map = new Map() @@ } + for (const points of map.values()) { + points.sort((a, b) => a.x - b.x) + } + return map }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 205 - 231, The buildSeriesByScenario function accumulates points per scenario but doesn't guarantee chronological order; after collecting points for each bench name (map entries), sort each series by the point timestamp (the x property) in ascending order so later slicing/trimming (which assumes chronological order) operates on correct data; locate buildSeriesByScenario and after the loop that pushes {x: timestamp, y: Number(bench.value)} into map.get(bench.name), iterate the map entries and sort each array by (a.x - b.x) (use resolveHistoryEntryTimestamp where relevant to ensure numeric timestamps) before returning the map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Around line 304-331: The current check drops the currentPoint when
resolveCurrentTimestamp(current) returns undefined because it requires
Number.isFinite(currentPoint.x); update the conditional that guards pushing
currentPoint so it allows a missing timestamp: replace the strict
Number.isFinite(currentPoint.x) check with one that permits null/undefined
timestamps (e.g., (currentPoint.x == null || Number.isFinite(currentPoint.x)))
while keeping the duplicate-point avoidance logic that compares
lastPoint.x/lastPoint.y; this ensures currentPoint (constructed in currentPoint)
is appended to historySeries even when currentTimestamp is undefined.
---
Nitpick comments:
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Around line 205-231: The buildSeriesByScenario function accumulates points per
scenario but doesn't guarantee chronological order; after collecting points for
each bench name (map entries), sort each series by the point timestamp (the x
property) in ascending order so later slicing/trimming (which assumes
chronological order) operates on correct data; locate buildSeriesByScenario and
after the loop that pushes {x: timestamp, y: Number(bench.value)} into
map.get(bench.name), iterate the map entries and sort each array by (a.x - b.x)
(use resolveHistoryEntryTimestamp where relevant to ensure numeric timestamps)
before returning the map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0284042-31be-4ef0-a4ea-8e80e5a4fa61
📒 Files selected for processing (1)
scripts/benchmarks/bundle-size/pr-report.mjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/benchmarks/bundle-size/pr-report.mjs (2)
375-375: The footer text is slightly stronger than the resolution logic.Line 375 says the series is plotted by “measurement date”, but the code can fall back to
commit.timestampandgeneratedAt. “Timestamp” would match the implemented sources better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/benchmarks/bundle-size/pr-report.mjs` at line 375, Update the footer string literal that currently reads "_Trend chart uses historical gzip bytes plotted by measurement date, ending with this PR measurement; lower is better._" to use "timestamp" (or explicitly mention the possible sources) so it matches the implemented fallback behavior (measurement.timestamp, commit.timestamp or generatedAt); e.g. replace "measurement date" with "timestamp (measurement.timestamp, commit.timestamp, or generatedAt)".
210-236: Sort each scenario series before trimming/appending.The chart path now compares against
lastPointand appends the PR measurement to the tail, so it implicitly assumeshistoryEntriesis already chronological. Sorting each series byxhere would make the graph and duplicate check deterministic even if the history feed arrives out of order.♻️ Suggested hardening
function buildSeriesByScenario(historyEntries) { const map = new Map() @@ map.get(bench.name).push({ x: timestamp, y: Number(bench.value), }) } } + + for (const points of map.values()) { + points.sort((a, b) => a.x - b.x) + } return map }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 210 - 236, In buildSeriesByScenario: the per-scenario arrays are not guaranteed to be chronological, which breaks the later duplicate check and appending logic; after populating each Map entry (i.e., after map.get(bench.name).push(...)) sort each series by the x timestamp (use a numeric comparator like (a,b) => a.x - b.x) so each series is deterministic and ordered before any trimming or comparison operations; do the sort once after the loop (iterate map.values()) to keep behavior stable even if historyEntries is out-of-order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Around line 320-337: The code trims history before checking whether to append
the current point, which causes the series to shrink when the current point is a
duplicate; fix by first getting the full series (const fullSeries =
seriesByScenario.get(metric.id) || []), compute currentPoint (using
currentTimestamp and metric.gzipBytes) and determine duplication against
fullSeries's last point (check Number.isFinite(currentPoint.x) and compare x/y);
then set historySeries = fullSeries.slice(willAppend ? -args.trendPoints + 1 :
-args.trendPoints) and only push currentPoint when willAppend is true (push to
historySeries when not duplicate). Ensure you reference historySeries,
seriesByScenario, currentPoint, currentTimestamp, metric.gzipBytes, and
args.trendPoints.
---
Nitpick comments:
In `@scripts/benchmarks/bundle-size/pr-report.mjs`:
- Line 375: Update the footer string literal that currently reads "_Trend chart
uses historical gzip bytes plotted by measurement date, ending with this PR
measurement; lower is better._" to use "timestamp" (or explicitly mention the
possible sources) so it matches the implemented fallback behavior
(measurement.timestamp, commit.timestamp or generatedAt); e.g. replace
"measurement date" with "timestamp (measurement.timestamp, commit.timestamp, or
generatedAt)".
- Around line 210-236: In buildSeriesByScenario: the per-scenario arrays are not
guaranteed to be chronological, which breaks the later duplicate check and
appending logic; after populating each Map entry (i.e., after
map.get(bench.name).push(...)) sort each series by the x timestamp (use a
numeric comparator like (a,b) => a.x - b.x) so each series is deterministic and
ordered before any trimming or comparison operations; do the sort once after the
loop (iterate map.values()) to keep behavior stable even if historyEntries is
out-of-order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c82a8c19-6b8f-4dc4-b534-aff172b83e65
📒 Files selected for processing (1)
scripts/benchmarks/bundle-size/pr-report.mjs
Comment on lines
320
to
337
| const historySeries = (seriesByScenario.get(metric.id) || []).slice( | ||
| // Reserve one slot for the current metric so the sparkline stays at trendPoints. | ||
| // Reserve one slot for the current metric so the trend stays at trendPoints. | ||
| -args.trendPoints + 1, | ||
| ) | ||
| const currentPoint = { | ||
| x: currentTimestamp, | ||
| y: metric.gzipBytes, | ||
| } | ||
| const lastPoint = historySeries[historySeries.length - 1] | ||
|
|
||
| if ( | ||
| !historySeries.length || | ||
| historySeries[historySeries.length - 1] !== metric.gzipBytes | ||
| Number.isFinite(currentPoint.x) && | ||
| (!lastPoint || | ||
| lastPoint.x !== currentPoint.x || | ||
| lastPoint.y !== currentPoint.y) | ||
| ) { | ||
| historySeries.push(metric.gzipBytes) | ||
| historySeries.push(currentPoint) | ||
| } |
Contributor
There was a problem hiding this comment.
Only reserve a slot when the current point is actually appended.
Lines 320-337 always pre-trim to trendPoints - 1, but the current point is skipped when it exactly matches the latest historical point. In that case the chart drops to 11 points instead of 12. Check duplication against the full scenario series first, then reserve a slot only when currentPoint will be added.
🐛 Suggested fix
- const historySeries = (seriesByScenario.get(metric.id) || []).slice(
- // Reserve one slot for the current metric so the trend stays at trendPoints.
- -args.trendPoints + 1,
- )
+ const scenarioSeries = seriesByScenario.get(metric.id) || []
const currentPoint = {
x: currentTimestamp,
y: metric.gzipBytes,
}
- const lastPoint = historySeries[historySeries.length - 1]
+ const lastPoint = scenarioSeries[scenarioSeries.length - 1]
+ const shouldAppendCurrent =
+ Number.isFinite(currentPoint.x) &&
+ Number.isFinite(currentPoint.y) &&
+ (!lastPoint ||
+ lastPoint.x !== currentPoint.x ||
+ lastPoint.y !== currentPoint.y)
+ const historySeries = scenarioSeries.slice(
+ -(args.trendPoints - (shouldAppendCurrent ? 1 : 0)),
+ )
- if (
- Number.isFinite(currentPoint.x) &&
- (!lastPoint ||
- lastPoint.x !== currentPoint.x ||
- lastPoint.y !== currentPoint.y)
- ) {
+ if (shouldAppendCurrent) {
historySeries.push(currentPoint)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/benchmarks/bundle-size/pr-report.mjs` around lines 320 - 337, The
code trims history before checking whether to append the current point, which
causes the series to shrink when the current point is a duplicate; fix by first
getting the full series (const fullSeries = seriesByScenario.get(metric.id) ||
[]), compute currentPoint (using currentTimestamp and metric.gzipBytes) and
determine duplication against fullSeries's last point (check
Number.isFinite(currentPoint.x) and compare x/y); then set historySeries =
fullSeries.slice(willAppend ? -args.trendPoints + 1 : -args.trendPoints) and
only push currentPoint when willAppend is true (push to historySeries when not
duplicate). Ensure you reference historySeries, seriesByScenario, currentPoint,
currentTimestamp, metric.gzipBytes, and args.trendPoints.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Example output
Bundle Size Benchmarks
6164816414562026-03-22T20:25:12+01:00history:b9f9ea15b696react-router.minimalreact-router.fullsolid-router.minimalsolid-router.fullvue-router.minimalvue-router.fullreact-start.minimalreact-start.fullsolid-start.minimalsolid-start.fullTrend chart uses historical gzip bytes plotted by measurement date, ending with this PR measurement; lower is better.
Summary by CodeRabbit