ci(perf): Add benchmark history with JSON-in-comment storage#1289
ci(perf): Add benchmark history with JSON-in-comment storage#1289
Conversation
Store benchmark history as JSON in an HTML comment within the PR comment body, replacing the need for artifact-based or HTML regex parsing approaches. - History data stored as `<!-- bench-history-json [...] -->` - post-pending job archives completed results into JSON history - comment job reads JSON history and renders History section - Both jobs share the same renderHistory() logic - Capped at 5 history entries to prevent comment bloat - File-based comment passing to avoid env var escaping issues Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughModified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Performance Benchmark
Details
History
|
Deploying repomix with
|
| Latest commit: |
29e5b6e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8acfa38e.repomix.pages.dev |
| Branch Preview URL: | https://ci-perf-benchmark-json-histo.repomix.pages.dev |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
- Scope OS-row regex to main table only (exclude History section) - Wrap JSON.parse in try/catch to handle corrupted comment bodies - HTML-escape commit messages to prevent injection and comment breakage - Use start/end delimiters for JSON comment to avoid --> conflicts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Prevent duplicate history entry when a workflow is re-run on the same commit by checking prevSha !== shortSha before archiving. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Add shellcheck disable directives for SC2016 (expressions don't expand in single quotes) on node -e invocations where single quotes are intentionally used to pass JavaScript code without shell expansion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Ensure comment job waits for post-pending to finish before reading the PR comment body, preventing a race condition where post-pending could overwrite completed results with an in-progress status. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review - PR #1289 (3rd Review)Previous reviews covered HTML escaping, double-escaping, duplication, stale comments, and consistency risks. Here are new findings only: Issues1. False "Benchmark complete!" when all benchmarks fail The 2. No error gate between Node script and file read If the inline Node script crashes (syntax error, runtime exception), the shell continues to read the temp file which either has stale content from a previous run or does not exist. The actual Node error gets buried. Consider deleting the temp file before running Node so stale reads are impossible. 3. Silent JSON parse failure with no diagnostic output Both try/catch blocks around Suggestions (non-blocking)4. Redundant API call to re-fetch comment body Both jobs make two API calls: one paginated call to find the comment ID (which already returns the full body in the response), then a second call to fetch the body by ID. These could be combined into a single call by extracting both 5. Concurrent pushes can silently lose history If two commits are pushed to a PR in quick succession, both Reviewed with Claude Code |
| - name: Comment on PR | ||
| if: ${{ github.event.pull_request.head.repo.fork == false }} |
There was a problem hiding this comment.
🟡 Step summary no longer written for fork PRs due to merged steps
In the old code, GITHUB_STEP_SUMMARY was written in the "Generate benchmark report" step which had no if guard — it ran for all PRs, including forks. In the new code, the step summary write (fs.appendFileSync(summaryFile, ...) at line 352) is inside the "Comment on PR" step which is guarded by if: ${{ github.event.pull_request.head.repo.fork == false }} at line 243. This means fork PRs lose the step summary entirely. Previously, the step summary was fork PR authors' only way to view benchmark results (since the PR comment was also fork-guarded).
Prompt for agents
In .github/workflows/perf-benchmark.yml, the "comment" job (starting around line 229) currently has a single "Comment on PR" step (line 242) guarded by the fork check. To restore the old behavior where GITHUB_STEP_SUMMARY was written regardless of fork status, split this into two steps:
1. A "Generate benchmark report" step with NO `if` condition that runs the Node script to generate the comment body and write to GITHUB_STEP_SUMMARY. This step should write the body to $RUNNER_TEMP/new-comment.md.
2. A "Comment on PR" step with `if: ${{ github.event.pull_request.head.repo.fork == false }}` that reads $RUNNER_TEMP/new-comment.md and posts/updates the PR comment.
This matches the old code's pattern where report generation (including step summary) was unconditional, and only the PR comment posting was fork-guarded.
Was this helpful? React with 👍 or 👎 to provide feedback.
Code Review - PR 1289 (4th Review)Previous reviews (Claude 3rd review + CodeRabbit) comprehensively covered the major issues. After a fresh pass, most significant findings have already been raised. Here is what remains: Already-raised issues still open (summary for tracking)
New finding (minor)Asymmetric encoding of msg field between storage and rendering In post-pending, prevMsg is extracted from already-rendered HTML (so it is HTML-escaped), then stored raw in the JSON history blob. In renderHistory, h.msg is injected directly into HTML without escaping, which works today because the value is already HTML-escaped from the prior render. But commitMsg for the current entry is escaped via esc() before rendering, then the HTML-escaped version gets stored in JSON on the next cycle. This means history entries accumulate one layer of HTML escaping per cycle they survive. After 2 cycles, an ampersand becomes double-escaped. This is a concrete bug for commit messages containing special HTML characters and they will progressively double-escape in the history section. Fix: Store the plain-text commit message in JSON (not the HTML-escaped version). When extracting prevMsg, unescape HTML entities back to plain text before storing in the JSON blob. In renderHistory, always apply esc() to h.msg before inserting into HTML. Overall assessmentThis is a well-structured CI change. The JSON-in-comment approach is clever and the file-based passing avoids env var escaping issues. The main risks are around HTML/comment escaping edge cases. Once the injection and double-escaping issues are addressed, this looks good to merge. Reviewed with Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1289 +/- ##
=======================================
Coverage 87.18% 87.18%
=======================================
Files 115 115
Lines 4324 4324
Branches 1002 1002
=======================================
Hits 3770 3770
Misses 554 554 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Store benchmark history as JSON in an HTML comment within the PR comment body. Each benchmark run archives its results into a JSON array (
<!-- bench-history-json [...] -->), which is parsed withJSON.parseinstead of fragile HTML regex extraction.How it works
<details>section rendered from JSONKey design decisions
$RUNNER_TEMP/old-comment.txtto avoid env var size/escaping issuesgh api: Commit message fetching stays in shell to avoid escaping nightmares in NodeChecklist
npm run testnpm run lint🤖 Generated with Claude Code