Refactor: Improve Build Workflow #576
Conversation
📝 WalkthroughWalkthroughRemoved the standalone build-size workflow and consolidated its behavior into an updated Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request (fork or internal)
participant GH as GitHub Actions (pull_request_target)
participant Comment as comment-external-pr job
participant CI as build-and-test-job
participant Size as Compressed Size Action
PR->>GH: PR opened / labeled / synchronized / reopened
alt PR head is from a fork
GH->>Comment: run comment-external-pr
Comment-->>PR: post guidance comment (size checks gated)
else Internal PR or has safe-to-build label
GH->>CI: run build-and-test-job
CI->>CI: checkout, setup-node, install, build, lint
CI->>Size: compare package sizes (packages/vechain-kit/dist/**)
Size-->>CI: report size diff
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Size Change: -17.9 kB (-0.31%) Total Size: 5.77 MB
ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/lint-build-test.yaml (2)
36-38: Minor formatting and style issues in the comment body.
- Line 36: "Eg." should be styled as "e.g.," (lowercase with comma) per standard English conventions.
- Line 38: Excessive indentation before "Next steps:" creates awkward spacing.
🔎 Proposed fix
Since this PR comes from a forked repository, the lint and build will only run for internal PRs for security reasons. - Please ensure that your PR is coming from a meaningful branch name. Eg. feature/my-feature **not main** + Please ensure that your PR is coming from a meaningful branch name, e.g., feature/my-feature **not main** - **Next steps:** + **Next steps:** 1. A maintainer will review your code
65-72: Consider enabling compression for more accurate size analysis.The
compression: nonesetting means the size comparison doesn't account for how files will actually be served (typically gzipped or brotli-compressed). Consider usingcompression: gziporcompression: brotlifor a more realistic size impact assessment.🔎 Proposed change
- name: Compare package sizes uses: preactjs/compressed-size-action@946a292cd35bd1088e0d7eb92b69d1a8d5b5d76a # v2 with: repo-token: ${{ secrets.GITHUB_TOKEN }} pattern: "packages/vechain-kit/dist/**/*" exclude: "{**/node_modules/**}" - compression: none + compression: gzip minimum-change-threshold: 1000
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-size.yaml.github/workflows/lint-build-test.yaml
💤 Files with no reviewable changes (1)
- .github/workflows/build-size.yaml
⏰ 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). (2)
- GitHub Check: build-size-check
- GitHub Check: e2e-test
🔇 Additional comments (1)
.github/workflows/lint-build-test.yaml (1)
52-58: Action version updates look good.The updates to
actions/checkout@v4andactions/setup-node@v4are appropriate. The Node.js setup correctly uses the.nvmrcfile for version management and enables yarn caching for faster builds.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/lint-build-test.yaml (1)
49-55: Checkout still doesn’t test PR changes underpull_request_target.With
pull_request_target,actions/checkout@v4without an explicitref/repositorychecks out the base branch, so this job currently builds and lintsmain, not the PR’s code—even for internal PRs and for external PRs withsafe-to-build. That makes the CI signal misleading and effectively useless for catching regressions.To both respect the gating you’ve added and actually exercise the PR code, explicitly check out the PR head commit from the PR’s repository (and avoid persisting credentials):
Proposed checkout fix
- name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@v4 + with: + repository: ${{ github.event.pull_request.head.repo.full_name }} + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + persist-credentials: falseThis keeps the
pull_request_targetsecurity posture (thanks to your gating + minimal permissions) while ensuring builds/lints are run against the actual PR code.In GitHub Actions, for workflows triggered by `pull_request_target`, what commit does `actions/checkout@v4` check out when `ref` is omitted, and what is the recommended pattern to safely build/test the PR’s head commit (including for forks) while minimizing token permissions?
🧹 Nitpick comments (1)
.github/workflows/lint-build-test.yaml (1)
33-41: Clarify external‑PR guidance text to match actual behavior.The message says “lint and build will only run for internal PRs,” but the job condition also allows external PRs once a maintainer adds the
safe-to-buildlabel. To avoid confusing contributors, consider rephrasing to something like “lint and build won’t run automatically for forked PRs; a maintainer can trigger them by adding thesafe-to-buildlabel,” while keeping the numbered steps.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/lint-build-test.yaml
⏰ 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). (1)
- GitHub Check: build-size-check
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/lint-build-test.yaml:
- Around line 32-43: The comment body under the "body:" YAML key contains excess
leading spaces on the "Next steps" block (lines containing "**Next steps:**",
the numbered list items and closing lines) which causes GitHub to render it as a
code block; remove the extra indentation so those lines align with the
surrounding paragraph (no more than 0–1 leading spaces) and ensure the numbered
list lines start at the same column as "## 👋 Thanks for your contribution!" to
restore proper Markdown list rendering.
🧹 Nitpick comments (2)
.github/workflows/lint-build-test.yaml (2)
49-51: Consider adding explicit parentheses for clarity.The condition works correctly due to operator precedence (
&&binds tighter than||), but the inconsistent parenthesization reduces readability. Wrapping the entire second condition makes the intent explicit.Proposed fix
if: | (github.event.label.name == 'safe-to-build') || - (github.event.pull_request.head.repo.full_name == github.repository) && github.event.pull_request.head.ref != 'main' + ((github.event.pull_request.head.repo.full_name == github.repository) && github.event.pull_request.head.ref != 'main')
53-56: Consider pinning all actions to SHA for consistency and supply-chain security.The workflow pins
peter-evans/create-or-update-commentandpreactjs/compressed-size-actionto SHA hashes but uses tag references foractions/checkout@v4andactions/setup-node@v4. SHA pinning provides stronger supply-chain security guarantees by preventing tag hijacking.Proposed fix with SHA-pinned versions
- name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: ref: ${{ github.event.pull_request.head.sha }} - name: Setup Node - uses: actions/setup-node@v4 + uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0 with: node-version-file: .nvmrc cache: yarn
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/lint-build-test.yaml
⏰ 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). (1)
- GitHub Check: build-size-check
🔇 Additional comments (2)
.github/workflows/lint-build-test.yaml (2)
4-8: Security setup forpull_request_targetis reasonable.The workflow correctly uses
pull_request_targetwith a label-gating mechanism for external PRs. Thecontents: readpermission at the workflow level is appropriate. The zizmor annotation documents the known risk and mitigation strategy.Be aware that this pattern still requires maintainers to carefully review PR code before adding the
safe-to-buildlabel, as the workflow will execute arbitrary code from the PR with access tosecrets.GITHUB_TOKEN.Also applies to: 14-15
64-77: Build and lint steps look good.The workflow correctly sequences dependency installation, build, size comparison, and linting. The compressed-size-action is properly configured with a reasonable threshold.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/lint-build-test.yaml:
- Around line 32-43: The comment body in the workflow step (the YAML "body"
string) contains extra leading spaces on the lines starting with "**Next
steps:**" through "3. **After each new commit**..." which makes Markdown treat
them as a code block; edit the "body" value to remove the additional indentation
on those lines so all lines align with the paragraph text (no leading spaces
before the bullets and phrases), preserving the same text but normalizing
indentation to avoid unintended code-block rendering.
- Around line 14-15: The workflow currently only sets top-level permissions to
"contents: read", preventing the compressed-size-action from posting PR
comments; update the permissions for the build-and-test-job (job name:
build-and-test-job) to include "pull-requests: write" (or add the same
permission at the workflow level) so the compressed-size-action can post size
comparison comments—modify the permissions block that references permissions:
contents: read to also include pull-requests: write for that job.
🧹 Nitpick comments (1)
.github/workflows/lint-build-test.yaml (1)
49-51: Add explicit parentheses to clarify operator precedence.The condition relies on
&&having higher precedence than||. While this likely produces the intended behavior, explicit parentheses would make the intent clearer and prevent future maintenance errors.Proposed fix
if: | (github.event.label.name == 'safe-to-build') || - (github.event.pull_request.head.repo.full_name == github.repository) && github.event.pull_request.head.ref != 'main' + ((github.event.pull_request.head.repo.full_name == github.repository) && (github.event.pull_request.head.ref != 'main'))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/lint-build-test.yaml
⏰ 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). (1)
- GitHub Check: build-size-check
🔇 Additional comments (4)
.github/workflows/lint-build-test.yaml (4)
3-8: Thepull_request_targetsecurity model is correctly implemented.The workflow properly gates the build job behind either the
safe-to-buildlabel (for external PRs) or same-repository origin checks. This follows the recommended pattern for safely handling fork PRs withpull_request_target.
64-78: Build, lint, and size comparison steps look good.The
yarn && yarn install:allpattern is appropriate for monorepos where root dependencies need to be installed before running workspace-specific install scripts. The step ordering (install → build → size compare → lint) is logical.
58-62: The setup-node action SHA is correctly pinned to v4.4.0.The SHA
49933ea5288caeca8642d1e84afbd3f7d6820020is a valid release commit corresponding to v4.4.0 ofactions/setup-node. The pinned SHA practice is sound.
53-56: No action needed. The code correctly usesactions/checkout@v6.0.1with a valid SHA. Version 6 is the current major release, and the specified commit8e8c483db84b4bee98b60c0593521ed34d9990e8is confirmed as a legitimate release commit dated December 2, 2025. The version and SHA are both accurate and secure.Likely an incorrect or invalid review comment.
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
Missing pull-requests: write permission for the build job.
The compressed-size-action (line 70) needs pull-requests: write permission to post size comparison comments on the PR. Currently, only contents: read is set at the workflow level, and the build-and-test-job doesn't override this.
Proposed fix
Add permissions to the build-and-test-job:
build-and-test-job:
runs-on: ubuntu-latest
name: Lint & Build & Compare package sizes
+ permissions:
+ contents: read
+ pull-requests: write
if: |🤖 Prompt for AI Agents
In @.github/workflows/lint-build-test.yaml around lines 14 - 15, The workflow
currently only sets top-level permissions to "contents: read", preventing the
compressed-size-action from posting PR comments; update the permissions for the
build-and-test-job (job name: build-and-test-job) to include "pull-requests:
write" (or add the same permission at the workflow level) so the
compressed-size-action can post size comparison comments—modify the permissions
block that references permissions: contents: read to also include pull-requests:
write for that job.
| body: | | ||
| ## 👋 Thanks for your contribution! | ||
|
|
||
| Since this PR comes from a forked repository, the lint and build will only run for internal PRs for security reasons. | ||
| Please ensure that your PR is coming from a meaningful branch name. Eg. feature/my-feature **not main** | ||
|
|
||
| **Next steps:** | ||
| 1. A maintainer will review your code | ||
| 2. If approved, they'll add the `safe-to-build` label to trigger build and test | ||
| 3. **After each new commit**, the maintainer will need to remove and re-add the label for security | ||
|
|
||
| Thank you for your patience! 🙏 |
There was a problem hiding this comment.
Inconsistent indentation in comment body will render poorly.
Lines 38-42 have extra leading spaces that will cause them to render as a code block in Markdown, breaking the visual consistency of the comment.
Proposed fix
body: |
## 👋 Thanks for your contribution!
Since this PR comes from a forked repository, the lint and build will only run for internal PRs for security reasons.
Please ensure that your PR is coming from a meaningful branch name. Eg. feature/my-feature **not main**
- **Next steps:**
- 1. A maintainer will review your code
- 2. If approved, they'll add the `safe-to-build` label to trigger build and test
- 3. **After each new commit**, the maintainer will need to remove and re-add the label for security
+ **Next steps:**
+ 1. A maintainer will review your code
+ 2. If approved, they'll add the `safe-to-build` label to trigger build and test
+ 3. **After each new commit**, the maintainer will need to remove and re-add the label for security
Thank you for your patience! 🙏📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| body: | | |
| ## 👋 Thanks for your contribution! | |
| Since this PR comes from a forked repository, the lint and build will only run for internal PRs for security reasons. | |
| Please ensure that your PR is coming from a meaningful branch name. Eg. feature/my-feature **not main** | |
| **Next steps:** | |
| 1. A maintainer will review your code | |
| 2. If approved, they'll add the `safe-to-build` label to trigger build and test | |
| 3. **After each new commit**, the maintainer will need to remove and re-add the label for security | |
| Thank you for your patience! 🙏 | |
| body: | | |
| ## 👋 Thanks for your contribution! | |
| Since this PR comes from a forked repository, the lint and build will only run for internal PRs for security reasons. | |
| Please ensure that your PR is coming from a meaningful branch name. Eg. feature/my-feature **not main** | |
| **Next steps:** | |
| 1. A maintainer will review your code | |
| 2. If approved, they'll add the `safe-to-build` label to trigger build and test | |
| 3. **After each new commit**, the maintainer will need to remove and re-add the label for security | |
| Thank you for your patience! 🙏 |
🤖 Prompt for AI Agents
In @.github/workflows/lint-build-test.yaml around lines 32 - 43, The comment
body in the workflow step (the YAML "body" string) contains extra leading spaces
on the lines starting with "**Next steps:**" through "3. **After each new
commit**..." which makes Markdown treat them as a code block; edit the "body"
value to remove the additional indentation on those lines so all lines align
with the paragraph text (no leading spaces before the bullets and phrases),
preserving the same text but normalizing indentation to avoid unintended
code-block rendering.
PR description
This PR updates the
Lint & BuildGitHub Actions workflow to run viapull_request_target, add an “external PR” guidance comment job, and adjust concurrency grouping.Also bumps
checkout/setup-nodefrom v3 to v4, switches build/lint commands toyarn build+yarn lint, and adds a compressed-size check onpackages/vechain-kit/dist.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.