Adding bundle analyser for frontend #1080
Conversation
Summary by CodeRabbit
WalkthroughThe pull request updates the workflow for bundle analysis in the Changes
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
.github/workflows/bundle-analysis.yaml (4)
44-57:⚠️ Potential issueFix duplicated PR checkout section.
There appear to be two separate "Checkout PR branch" steps (lines 45 and 53). The workflow logic is confusing with duplicated steps and orphaned commands.
Consolidate these into a single, coherent sequence:
# 📌 Step 4: Switch back to PR branch and build - name: Checkout PR branch run: git checkout "${{ github.event.pull_request.head.ref }}" - name: Build PR branch run: | cd frontend pnpm build - name: Save PR branch bundle size run: | cd frontend du -sh dist > pr-bundle-size.txt🧰 Tools
🪛 actionlint (1.7.4)
54-54: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
58-68:⚠️ Potential issueFix duplicated comparison section.
There appears to be duplicated or incorrectly structured code in the bundle size comparison section.
Clean up this section to ensure proper flow:
# 📌 Step 5: Compare bundle sizes - name: Compare bundle sizes run: | cd frontend echo "**Bundle Size Comparison:**" > bundle-comparison.txt echo "\n**Main Branch:**" >> bundle-comparison.txt cat main-bundle-size.txt >> bundle-comparison.txt echo "\n**PR Branch:**" >> bundle-comparison.txt cat pr-bundle-size.txt >> bundle-comparison.txt echo "\n**Size Difference:**" >> bundle-comparison.txt diff main-bundle-size.txt pr-bundle-size.txt >> bundle-comparison.txt || true
69-89:⚠️ Potential issueFix artifact upload section.
There's misalignment in the artifact upload section with orphaned commands and duplicated artifacts.
Restructure this section:
# 📌 Step 6: Upload stats.html for review - name: Generate Visualizer Report + name: Upload bundle analysis artifact uses: actions/upload-artifact@v4 with: name: bundle-analysis path: frontend/dist/stats.html
90-101:⚠️ Potential issueFix comment posting section.
This section appears to be duplicated from an earlier part of the workflow.
Ensure this is properly structured:
# 📌 Step 7: Comment PR with bundle size change summary - name: Comment bundle size changes uses: marocchino/sticky-pull-request-comment@v2 with: repo-token: ${{ secrets.GITHUB_TOKEN }} number: ${{ github.event.pull_request.number }} header: bundle-analysis message: | 🔍 **Bundle Analysis Report** 📊 **Bundle Size Comparison:** ``` $(cat frontend/bundle-comparison.txt) ``` 📁 [Download the bundle analysis report](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})
🧹 Nitpick comments (1)
.github/workflows/bundle-analysis.yaml (1)
22-24: Good addition of an explicit pnpm installation step.Explicitly installing pnpm ensures it's available for the subsequent commands. However, consider using the official GitHub Action for pnpm instead of manual installation.
- name: Install pnpm - run: npm install -g pnpm # ✅ Fix: Explicitly install pnpm + name: Setup pnpm + uses: pnpm/action-setup@v2 + with: + version: 8
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.github/workflows/bundle-analysis.yaml(1 hunks)frontend/package.json(1 hunks)frontend/vite.config.ts(2 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run CI/CD
frontend/vite.config.ts
[warning] 6-6: rollup-plugin-visualizer import should occur before import of vite (import/order)
🪛 actionlint (1.7.4)
.github/workflows/bundle-analysis.yaml
54-54: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
🔇 Additional comments (3)
frontend/package.json (1)
93-93: Good addition of the rollup-plugin-visualizer dependency.Adding
rollup-plugin-visualizeras a development dependency is appropriate for implementing bundle analysis functionality. The version^5.14.0is recent, which ensures you'll have access to the latest features and bug fixes.frontend/vite.config.ts (1)
17-22: Well-configured visualizer plugin.The configuration for the rollup visualizer is appropriate:
- Setting
filenameto 'dist/stats.html' keeps the output in the build directory- Setting
open: falseis correct for CI environments- Enabling
gzipSizeandbrotliSizeprovides valuable information about the compressed bundle sizes, which is more relevant for production performanceThis setup will provide valuable insights into your bundle composition.
.github/workflows/bundle-analysis.yaml (1)
1-101:❓ Verification inconclusive
Critical issues with overall workflow structure.
The entire workflow file appears to have significant structural problems, including:
- Duplicated steps
- Misaligned commands
- Incorrect step numbering
- Orphaned commands without proper step names
This will likely lead to workflow failures.
I recommend running the following command to verify the YAML syntax:
Please reconstruct the entire workflow file from scratch, ensuring proper structure and step sequencing. The steps should generally follow this order:
- Checkout repository
- Setup Node.js and pnpm
- Install dependencies
- Checkout and build main branch
- Save main branch bundle size
- Checkout and build PR branch
- Save PR branch bundle size
- Compare bundle sizes
- Upload bundle analysis artifact
- Comment on PR with results
🏁 Script executed:
#!/bin/bash # Verify the YAML syntax of the workflow file yamllint .github/workflows/bundle-analysis.yamlLength of output: 164
Action Required: Verify and Streamline Workflow Steps
After running YAML linting, the workflow file passes syntax checks (aside from a minor warning regarding a truthy value at line 3). No critical YAML structural errors were detected. However, note the following:
- The build command for the PR branch appears to be executed twice (once as “Build PR branch” and again in “Generate Visualizer Report”). Please confirm whether this redundancy is intentional or if the second build can be removed.
- Review the overall step sequencing to ensure each step is necessary and that command duplication isn’t affecting performance or clarity.
- Consider explicitly setting the truthy value (if needed) on line 3 per linting suggestions.
Please verify that the intended workflow behavior is preserved after any restructuring.
🧰 Tools
🪛 actionlint (1.7.4)
54-54: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/bundle-analysis.yaml (3)
39-43: Main Branch Build with Fallback BehaviorUsing
pnpm build || trueensures that a build failure does not break the workflow. While this is acceptable for non-critical analysis, consider logging the build error explicitly for future debugging.
74-79: Visualizer Report Generation: Consider Caching BuildsRe-running
pnpm buildto generate the visualizer report is functionally correct; however, executing a second build might be inefficient. Investigate whether the build output from earlier steps can be reused to optimize the workflow runtime.
86-102: Duplicate Message in PR Comment StepThe step using
marocchino/sticky-pull-request-comment@v2contains a duplicated bundle size comparison block between lines 91–96 and 97–102. This redundancy could confuse readers. It is recommended to remove the duplicate block and keep a single, clear message.Suggested Diff:
- 📊 **Bundle Size Comparison:** - ``` - $(cat frontend/bundle-comparison.txt) - ``` - 📁 [Download the bundle analysis report](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/bundle-analysis.yaml
58-58: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
🔇 Additional comments (13)
.github/workflows/bundle-analysis.yaml (13)
1-2: Workflow Name is Clear and DescriptiveThe workflow is named "Bundle Analysis & Comparison," which clearly communicates its purpose.
3-8: Triggers are Configured CorrectlyThe workflow triggers on pull requests targeting the main branch and supports manual dispatch. This configuration meets the objective for analyzing bundle sizes on PRs.
9-12: Job Definition and Environment SetupThe job named
analyzeis correctly set to run on Ubuntu latest. The overall structure here is clear and follows GitHub Actions best practices.
13-15: Repository Checkout Step is Properly ConfiguredUsing
actions/checkout@v4to obtain the repository is standard and up-to-date.
16-21: Node.js Setup is Implemented WellThe usage of
actions/setup-node@v4with a specified Node version (22) and caching forpnpmensures efficient and repeatable dependency management.
22-26: Dependency Installation Step is ClearChanging into the
frontenddirectory before runningpnpm installis correct and maintains clarity in directory context.
27-30: Main Branch Checkout for Comparison is Handled ProperlyChecking out the
mainbranch usinggit fetchandgit checkout mainis appropriate for comparing bundle sizes.
31-38: Conditional Check for Required Dependency is a Good Safety MeasureThe step verifies whether
rollup-plugin-visualizeris present. Skipping analysis when the plugin isn’t found avoids unnecessary build steps.
44-48: Capturing Main Branch Bundle Size is RobustRunning
du -sh distto save the bundle size, with a fallback to "0 KB" if the command fails, is a practical approach for ensuring the file exists for comparison.
53-57: PR Branch Build Step is StraightforwardChanging directory to
frontendand executingpnpm buildis clear. Ensure that the build process produces the expected artifacts for analysis.
58-62: Saving PR Branch Bundle Size is ConsistentThe command to capture the bundle size for the PR branch mirrors the main branch approach, which is good for maintaining consistency in comparison.
🧰 Tools
🪛 actionlint (1.7.4)
58-58: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
63-73: Bundle Comparison Logic is SoundAppending sizes and using
diffto detect differences is a reasonable approach. The inclusion of|| trueensures the step doesn’t fail if there is a non-zero exit code fromdiff.
80-85: Artifact Upload is Configured CorrectlyUploading
frontend/dist/stats.htmlusingactions/upload-artifact@v4is implemented properly. Be sure the build consistently produces thestats.htmlfile in the expected location.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/bundle-analysis.yaml (1)
98-108: 🛠️ Refactor suggestionDuplicate PR Commenting Step Detected
There are two steps (lines 86–97 and 98–108) that serve the same purpose of commenting on the PR with the bundle size comparison report. It is advisable to remove the duplicate to avoid redundancy and potential confusion.
🧹 Nitpick comments (3)
.github/workflows/bundle-analysis.yaml (3)
27-48: Main Branch Preparation and Bundle Size RecordingThe steps to checkout the main branch, conditionally verify the presence of the rollup-plugin-visualizer, build the branch (with a fallback mechanism), and record the bundle size are well orchestrated.
However, static analysis and pipeline logs indicate trailing whitespace issues (notably around the build command area). Please remove any extra trailing spaces to comply with linting rules.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 45-45: trailing spaces
(trailing-spaces)
🪛 GitHub Actions: Run CI/CD
[error] 42-42: Trailing whitespace found. Pre-commit hook 'trailing-whitespace' failed.
86-97: PR Commenting for Bundle Analysis ReportThe step to post a comment on the pull request with the bundle analysis report is well constructed.
Note: There appears to be another similar step immediately following this one.
42-45: Remove Trailing WhitespaceStatic analysis reports trailing whitespace (e.g., near the run command at line 42–45). Please ensure that any extraneous spaces are removed to comply with YAML linting and pre-commit hooks.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 45-45: trailing spaces
(trailing-spaces)
🪛 GitHub Actions: Run CI/CD
[error] 42-42: Trailing whitespace found. Pre-commit hook 'trailing-whitespace' failed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/bundle-analysis.yaml
62-62: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
🪛 YAMLlint (1.35.1)
.github/workflows/bundle-analysis.yaml
[error] 45-45: trailing spaces
(trailing-spaces)
🪛 GitHub Actions: Run CI/CD
.github/workflows/bundle-analysis.yaml
[error] 42-42: Trailing whitespace found. Pre-commit hook 'trailing-whitespace' failed.
🔇 Additional comments (7)
.github/workflows/bundle-analysis.yaml (7)
1-8: Workflow Header and Trigger Configuration Looks SolidThe workflow’s name, pull request trigger (restricted to the main branch), and manual dispatch configuration are well defined.
9-26: Standard Repository Checkout and Environment SetupThe steps for checking out the repository, setting up Node.js with a specified version and pnpm cache, and installing dependencies are implemented correctly.
49-51: Sanitize PR Branch Checkout CommandThe checkout command for the PR branch directly uses the untrusted input:
git checkout "${{ github.event.pull_request.head.ref }}"This poses a security risk. Although similar feedback was provided previously (and noted as fixed in commit 6ca311c), please verify that the branch reference is properly sanitized before usage.
53-57: PR Branch Build Step is Configured CorrectlyThe PR branch build process mirrors the main branch build step, ensuring that the frontend is built for bundle size comparison.
58-61: Recording PR Bundle SizeThe process for saving the PR branch bundle size is clear and correct. Ensure any path or environment consistency is maintained.
63-73: Bundle Size Comparison Logic is EffectiveConcatenating the bundle sizes and using the
diffcommand (with a graceful failure using|| true) is a good approach for comparison.
74-84: Visualizer Report Generation and UploadGenerating the visualizer report by rebuilding the frontend and then uploading the resulting
stats.htmlartifact is implemented properly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/bundle-analysis.yaml (4)
31-38: Rollup Plugin Existence Check
The script checks for the presence ofrollup-plugin-visualizerin the main branch by runningpnpm listand gracefully exits if it’s not found. For improved debugging, consider logging additional details when the plugin is absent.
63-73: Bundle Size Comparison
The comparison step builds abundle-comparison.txtfile that aggregates the main and PR branch bundle sizes and runs adiffto highlight changes. While this offers a quick textual diff, consider whether formatting nuances (such as differing units or extra whitespace) might affect accuracy; using a more robust numerical comparison could be a good enhancement.
74-79: Generate Visualizer Report Step
The workflow re-runspnpm buildwithinfrontendto generate a visualizer report. Confirm that this rebuild is necessary—if the report is generated by a specific output (likestats.html), it might be more efficient to invoke a dedicated build command rather than duplicating the standard build process.
86-101: PR Comment Message Duplication
The message intended for the pull request comment appears to contain duplicated sections (the bundle size comparison and download link are repeated). Streamlining this message will improve clarity and reduce redundancy.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (11)
.github/workflows/bundle-analysis.yaml (11)
1-8: Workflow Trigger Definition
The workflow’s triggers—invoking on pull requests to themainbranch and via manual dispatch—are correctly defined. This clear configuration helps ensure that the bundle analysis runs at the appropriate times.
13-15: Repository Checkout Step
Usingactions/checkout@v4to obtain the codebase is appropriate and ensures that subsequent steps operate on the intended commit.
16-21: Node.js Setup
The Node.js setup with version22and caching forpnpmis configured correctly. Please verify that Node.js 22 is compatible with all project dependencies.
22-26: Dependency Installation
Changing the directory tofrontendand executingpnpm installis a clear and correct approach for installing project dependencies.
27-30: Main Branch Checkout
The step to switch to themainbranch for comparison usinggit fetch origin main && git checkout mainis implemented correctly.
39-43: Main Branch Build Step
Usingpnpm build || trueensures that the workflow doesn’t fail even if the build step has issues. However, bypassing build errors might mask critical issues with the bundle. Verify that this behavior is intentional—if not, consider handling build failures explicitly so that unexpected build errors are not silently ignored.
44-48: Save Main Bundle Size
Capturing the bundle size withdu -sh distand providing a fallback (echo "0 KB") is a robust approach. Ensure that the output format remains consistent to facilitate accurate diff comparisons later.
49-52: PR Branch Checkout and Security Consideration
The workflow switches back to the PR branch using:git checkout "${{ github.event.pull_request.head.ref }}"Ensure that this input is appropriately sanitized or validated per GitHub’s security guidelines. Since a similar issue was flagged earlier and marked as fixed in commit
6ca311c, please double-check that the security concern has been fully addressed.
53-57: PR Branch Build Step
The build process for the PR branch is straightforward and correctly navigates into thefrontenddirectory before executingpnpm build. Confirm that this step reliably captures the intended build outcomes.
58-62: Save PR Bundle Size
Saving the PR branch bundle size withdu -sh distis implemented consistently with the main branch. This approach should allow for an accurate side-by-side comparison later.
80-85: Artifact Upload
Uploading the generatedstats.htmlartifact fromfrontend/distviaactions/upload-artifact@v4is correctly implemented and will allow easy access to the bundle analysis report.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/bundle-analysis.yaml (1)
87-105:⚠️ Potential issueCritical Issue: Remove Duplicate 'message' Key in Comment Step
Within the "Comment bundle size changes" step, there are twomessagekeys defined underwith:. YAML requires unique keys within a mapping, so the duplicate key may lead to unexpected behavior or a workflow parsing error. Please remove one of the duplicatemessageentries. For example, use the diff below to remove the second occurrence:- message: | - 🔍 **Bundle Analysis Report** - 📊 **Bundle Size Comparison:** - ``` - $(cat frontend/bundle-comparison.txt) - ``` - 📁 [Download the bundle analysis report](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})This change will ensure that the comment is generated correctly.
🧹 Nitpick comments (2)
.github/workflows/bundle-analysis.yaml (2)
39-43: Suggestion: Revisit Error Handling in Main Branch Build Step
The use ofpnpm build || truemight hide build failures, making it harder to diagnose issues when the main branch build fails. Consider explicitly handling errors or at least logging the failure so that it won’t be silently swallowed.
75-79: Refactor Suggestion: Avoid Duplicate Build for Visualizer Report
The workflow builds the frontend in a prior step and then builds it again in the "Generate Visualizer Report" step. If the visualizer report depends on the same build output, consider caching or reusing the previous build artifacts to optimize the workflow runtime.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(3 hunks).github/workflows/bundle-analysis.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
.github/workflows/bundle-analysis.yaml (1)
50-51: Potential Security Concern: Validate PR Branch Checkout
The PR branch is checked out using an untrusted GitHub context variable:git checkout "${{ github.event.pull_request.head.ref }}"Ensure that this branch reference is sanitized or validated to comply with GitHub’s security guidelines. Note that a similar concern was flagged earlier and supposedly addressed in commit 6ca311c; please confirm the fix is effective.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/bundle-analysis.yaml (1)
86-106:⚠️ Potential issueCritical Issue: Duplicate
with:Blocks in Comment StepThe "Comment bundle size changes" step contains two separate
with:blocks (lines 87–106), which is invalid YAML syntax and may lead to misconfiguration of the step. Please merge these blocks into a singlewith:block that includes both theGITHUB_TOKENandmessageentries. For example:- - name: Comment bundle size changes - uses: marocchino/sticky-pull-request-comment@v2 - with: - message: | - 🔍 **Bundle Analysis Report** - 📊 **Bundle Size Comparison:** - ``` - $(cat frontend/bundle-comparison.txt) - ``` - 📁 [Download the bundle analysis report](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) - with: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # ✅ Now has proper write access - message: | - 🔍 **Bundle Analysis Report** - 📊 **Bundle Size Comparison:** - ``` - $(cat frontend/bundle-comparison.txt) - ``` - 📁 [Download the bundle analysis report](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) + - name: Comment bundle size changes + uses: marocchino/sticky-pull-request-comment@v2 + with: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # ✅ Now has proper write access + message: | + 🔍 **Bundle Analysis Report** + 📊 **Bundle Size Comparison:** + ``` + $(cat frontend/bundle-comparison.txt) + ``` + 📁 [Download the bundle analysis report](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})Merging these blocks ensures the artifact comment is configured correctly.
🧹 Nitpick comments (2)
.github/workflows/bundle-analysis.yaml (2)
41-43: Suggestion: Explicit Error Handling for Build FailuresUsing
pnpm build || truein the "Build main branch" step (lines 41–43) suppresses build errors, which might hide critical issues in the build process. Consider handling errors explicitly or logging failures so that any build issues are visible during troubleshooting.
74-79: Suggestion: Review Redundant Build for Visualizer ReportThe "Generate Visualizer Report" step (lines 74–79) runs
pnpm buildagain, even though a build has already been executed earlier for the PR branch. Please verify whether this rebuild is necessary or if the existing output can be reused for generating the report.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(3 hunks).github/workflows/bundle-analysis.yaml(5 hunks).github/workflows/bundle-analysis.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
🔇 Additional comments (1)
.github/workflows/bundle-analysis.yaml (1)
50-52: Security: Untrusted Branch CheckoutThe "Checkout PR branch" step uses the untrusted variable
${{ github.event.pull_request.head.ref }}directly. Although this issue was flagged in previous reviews, please confirm that its usage has been adequately sanitized or validated according to GitHub’s security guidelines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/bundle-analysis.yaml (4)
39-43: Main Branch Build with FallbackUsing
pnpm build || trueensures that the workflow continues even if the build fails. Be cautious with this approach, as it may mask build issues. It might be helpful to log build errors or verify that failing builds are acceptable in this context.
63-73: Bundle Size ComparisonThe comparison step concatenates the bundle sizes and uses
diffto highlight differences. Ensure that the output ofdu -shremains consistently formatted so that the diff produces meaningful results.
74-79: Visualizer Report GenerationA second build is triggered to generate the visualizer report. Verify whether this additional build is required, or if previous build artifacts could be reused to optimize the workflow and reduce redundant build operations.
86-97: Commenting Bundle Size ChangesThe step that comments on the pull request with the bundle size comparison summary is beneficial. However, there are still several commented-out fallback lines at the end of the step. Removing these outdated commented lines could help keep the workflow file clean and maintainable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(3 hunks).github/workflows/bundle-analysis.yaml(5 hunks).github/workflows/bundle-analysis.yaml(2 hunks).github/workflows/bundle-analysis.yaml(2 hunks).github/workflows/bundle-analysis.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (10)
.github/workflows/bundle-analysis.yaml (10)
1-8: Workflow Trigger and Naming ConfigurationThe workflow’s name and trigger configuration (pull_request on main and workflow_dispatch) are set up correctly and clearly reflect the purpose of the workflow.
13-21: Repository Checkout & Node.js SetupThe steps to check out the repository and set up Node.js (using version 22 with pnpm cache) are correctly implemented and well organized.
22-26: Dependency InstallationThe commands for changing to the
frontenddirectory and runningpnpm installare straightforward and appropriate.
27-30: Main Branch CheckoutThe main branch is fetched and checked out correctly with:
git fetch origin main && git checkout mainThis is a standard approach for preparing a baseline for bundle comparison.
31-38: Visualizer Existence CheckThe step to verify whether
rollup-plugin-visualizerexists in the main branch is a clever guard. It conditionally skips the analysis when the dependency is absent, which helps in avoiding unnecessary builds.
44-48: Saving Main Branch Bundle SizeThe command to capture the bundle size—with a fallback (
echo "0 KB") in case of failure—is a useful safeguard to ensure that the workflow always produces a valid output for comparison.
49-52: PR Branch Checkout – Security ConsiderationThe checkout command for the PR branch:
git checkout "${{ github.event.pull_request.head.ref }}"directly uses an unsanitized branch reference. Although previous revisions claim to have addressed this, please ensure that the branch name is validated or sanitized per GitHub’s security best practices to avoid potential injection risks.
53-57: PR Branch BuildBuilding the PR branch by changing directory to
frontendand runningpnpm buildis implemented correctly.
58-62: Saving PR Branch Bundle SizeThe method for saving the bundle size for the PR branch mirrors that of the main branch and is consistent and appropriate.
80-85: Artifact UploadThe step to upload the
stats.htmlartifact usingactions/upload-artifact@v4is clearly defined and correctly configured.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
.github/workflows/bundle-analysis.yaml (7)
1-2: Workflow Naming and Introduction
The workflow is clearly named as "Bundle Analysis & Comparison." Consider adding a brief YAML comment at the top to explain the purpose of the workflow for future maintainers.
22-26: Installation of Frontend Dependencies
The workflow changes to thefrontenddirectory and runspnpm install, which is appropriate.
Consider verifying that pnpm is available in the environment (or installing it explicitly) if not already provided by the Node.js setup step.
31-38: Conditional Check for rollup-plugin-visualizer
The script moves to thefrontenddirectory and usespnpm list rollup-plugin-visualizerto verify the presence of the visualizer tool.
If the package isn't found, the workflow exits gracefully.
Ensure that this check reliably detects the package—even if it’s a nested dependency—and that silently exiting is the desired behavior.
39-43: Main Branch Build Step and Error Handling
The commandpnpm build || trueis used to run the build and suppress errors. This may hide legitimate build failures. Confirm that suppressing errors here is intentional; otherwise, consider removing|| trueso that build issues can fail the workflow for immediate attention.
53-57: PR Branch Build Step
The build command for the PR branch (pnpm build) is straightforward. Consider whether the error handling strategy should mirror the main branch build step (i.e., deciding if the build failure should stop subsequent steps) and document this choice.
63-73: Bundle Size Comparison
The steps aggregate sizes from the main and PR branches into abundle-comparison.txtfile and usediffto highlight differences.
While functional, consider if the diff output format is sufficiently clear or if a more structured comparison might be beneficial for actionable insights.
86-102: PR Comment Step and Cleanup of Redundant Code
The step usesmarocchino/sticky-pull-request-comment@v2to comment on the PR with the bundle analysis summary.
The active comment block is clear; however, there is a trailing commented-out duplicate block (lines 97-102) that should be removed to clean up the workflow file and avoid confusion.
A suggested diff to remove the redundant commented code:- # 📊 **Bundle Size Comparison:** - # ``` - # $(cat frontend/bundle-comparison.txt) - # ``` - # 📁 [Download the bundle analysis report](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }})
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(4 hunks).github/workflows/bundle-analysis.yaml(3 hunks).github/workflows/bundle-analysis.yaml(5 hunks).github/workflows/bundle-analysis.yaml(2 hunks).github/workflows/bundle-analysis.yaml(2 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
- .github/workflows/bundle-analysis.yaml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
.github/workflows/bundle-analysis.yaml (8)
3-8: Event Triggers Configuration
The workflow is set to trigger on pull requests to the main branch and via manual dispatch. This configuration looks appropriate for the intended bundle analysis.
13-14: Repository Checkout Step
Using the standardactions/checkout@v4step for checking out the repository is correct.
16-21: Node.js Setup with PNPM Caching
The setup correctly usesactions/setup-node@v4with Node.js version 22 and enables caching for pnpm. Ensure that Node.js version 22 is compatible with your project.
27-30: Checkout Main Branch for Comparison
The step checking out the main branch withgit fetch origin main && git checkout mainis correctly targeting the main branch for a baseline comparison.
44-48: Saving Main Branch Bundle Size
Saving the bundle size withdu -sh distand using a fallback to output"0 KB"if the command fails is a pragmatic approach.
Ensure that the working directory and thedistfolder location are consistently correct across builds.
49-51: Checkout of PR Branch Using Untrusted Input
The checkout step for the PR branch uses:git checkout "${{ github.event.pull_request.head.ref }}"This pattern was flagged previously for potential security risks when using untrusted inputs. Please verify that the branch reference is sanitized or validated according to GitHub’s security guidelines.
(See earlier review comments – )
58-62: Saving PR Branch Bundle Size
The command to save the PR branch’s bundle size is consistent with the main branch step. This approach is acceptable.
80-85: Uploading the Bundle Analysis Report
Uploadingfrontend/dist/stats.htmlwithactions/upload-artifact@v4is a standard approach.
It might be useful to add a conditional check to verify thatstats.htmlexists to prevent potential issues if the file is missing.
yashgoyal0110
force-pushed
the
feat/bundle-analyser
branch
5 times, most recently
from
e8e4ec9 to
76fe9ff
Compare
yashgoyal0110
force-pushed
the
feat/bundle-analyser
branch
from
76fe9ff to
7249469
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/bundle-analysis.yaml (1)
85-91:⚠️ Potential issueIncomplete and Duplicated Artifact Upload Block
The block starting at line 85 begins an "Upload bundle analysis report" step but includes an emptywith:configuration and is immediately followed by a duplicated "Debug dist folder contents" step. Given that a correctly configured upload step already exists at lines 93–98, it is recommended to remove this entire block (lines 85–91) to prevent potential YAML parsing errors and avoid redundancy.Proposed diff:
- - name: Upload bundle analysis report - uses: actions/upload-artifact@v4 - with: - - name: Debug dist folder contents - working-directory: frontend - run: ls -R dist || echo "dist folder not found"
🧹 Nitpick comments (1)
.github/workflows/bundle-analysis.yaml (1)
80-83: Duplicate Debug Step Detected
The newly added "Debug dist folder contents" step appears to duplicate similar functionality already provided later in the file. Please verify whether both occurrences are necessary. Consolidating these steps can improve clarity and reduce redundant execution.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.github/workflows/bundle-analysis.yaml(1 hunks)frontend/package.json(1 hunks)frontend/vite.config.ts(2 hunks).github/workflows/bundle-analysis.yaml(0 hunks).github/workflows/bundle-analysis.yaml(1 hunks).github/workflows/bundle-analysis.yaml(2 hunks)frontend/vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/bundle-analysis.yaml
- frontend/package.json
- .github/workflows/bundle-analysis.yaml
- frontend/vite.config.ts
- .github/workflows/bundle-analysis.yaml
- frontend/vite.config.ts
|
|
This has been stale for a while, I'm closing this. Feel free to reopen when you feel it's ready for review. |
fixes: #1063