fix: build check #31
fix: build check #31
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCI workflow updated to trigger on pull_request events with concurrency control, split into install and build jobs. Adds Node.js 22 setup, npm/node_modules caching keyed by package-lock.json, conditional npm ci, build depending on install, and build-time NEXT_PUBLIC_* environment variables. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor PR as GitHub PR Event
participant GH as GitHub Actions
participant Install as Job: Install Dependencies
participant Cache as actions/cache
participant Build as Job: Build
participant NPM as npm
PR->>GH: pull_request (opened/reopened/synchronize/ready_for_review)
GH->>GH: Concurrency group (cancel in-progress same ref)
GH->>Install: start
Install->>Install: checkout
Install->>Install: setup-node v22 (npm cache)
Install->>Cache: restore node_modules (key: hashFiles(package-lock.json))
alt cache hit
Install->>Install: skip npm ci
else cache miss
Install->>NPM: npm ci
NPM-->>Install: node_modules
Install->>Cache: save node_modules
end
Install-->>GH: complete
GH->>Build: start (needs: install)
Build->>Build: checkout
Build->>Build: setup-node v22 (npm cache)
Build->>Cache: restore node_modules
Build->>Build: set NEXT_PUBLIC_* env vars
Build->>NPM: npm run build
NPM-->>Build: build artifacts
Build-->>GH: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a GitHub Actions workflow issue where build checks were not automatically running on pull requests. The solution replaces the existing manual-trigger-only build workflow with a comprehensive build-and-lint workflow that includes automatic PR triggers.
- Removed the old
build.ymlworkflow that only supported manual triggering - Added a new
build-and-lint.ymlworkflow with automatic PR triggers and dependency caching - Enhanced the workflow with concurrency controls and environment variables for the build process
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/build.yml |
Removed manual-only workflow file |
.github/workflows/build-and-lint.yml |
Added comprehensive workflow with PR triggers, caching, and build configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![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
🧹 Nitpick comments (6)
.github/workflows/build.yml (6)
12-14: Refine concurrency key for PRs.Using github.ref is OK, but for pull_request it’s safer to key by PR number to avoid oddities across merge vs head refs.
Apply:
-concurrency: - group: ${{ github.workflow }}-${{ github.ref }} +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true
21-28: Verify Node 22 compatibility; prefer repo-standard version.Ensure Node 22 matches package.json "engines" and any .nvmrc/.node-version. If the repo specifies a version, have Actions read it directly.
Option A (use version file if present):
- - name: Setup Node.js - uses: actions/setup-node@v4 - with: - node-version: "22" - cache: "npm" + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version-file: '.nvmrc' + cache: 'npm'Option B (if no version file): align to LTS (e.g., 20) or engines field.
29-41: Avoid caching node_modules; rely on npm cache + npm ci.Caching node_modules is brittle (binary/addon drift, platform coupling) and redundant with setup-node’s npm cache. Run npm ci every time; keep only npm cache.
Apply:
- - name: Cache node_modules - uses: actions/cache@v4 - id: node-modules-cache - with: - path: | - node_modules - ~/.npm - key: ${{ runner.os }}-node-modules-${{ hashFiles('**/package-lock.json') }} - restore-keys: | - ${{ runner.os }}-node-modules- - - - name: Install dependencies - if: steps.node-modules-cache.outputs.cache-hit != 'true' - run: npm ci + - name: Install dependencies + run: npm ci --no-audit --prefer-offline
51-55: Apply the same Node version strategy in build job.Mirror the install job change (node-version-file or aligned version).
- - name: Setup Node.js - uses: actions/setup-node@v4 - with: - node-version: "22" - cache: "npm" + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version-file: '.nvmrc' + cache: 'npm'
1-3: Set least-privilege token permissions.For a build-only workflow, lock down GITHUB_TOKEN.
name: Build + +permissions: + contents: read + actions: read + checks: writeIf checks: write isn’t needed, drop it.
67-69: Consider a job timeout.Add a conservative timeout to avoid hung builds consuming minutes.
build: name: Build needs: install - runs-on: ubuntu-latest + runs-on: ubuntu-latest + timeout-minutes: 15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.27.2)
.github/workflows/build.yml
[high] 77-77: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
.github/workflows/build.yml (4)
5-10: PR builds now auto-trigger on pull_request — good fix.This restores CI on PR events (opened/reopened/synchronize/ready_for_review). Matches the PR objective.
45-48: Job dependency wiring looks good.needs: install ensures build runs after deps are installed/cached.
67-79: Hard-coded NEXT_PUBLIC envs — confirm RN client ID is non-secret and stop embedding in the workflow.
- Evidence: NEXT_PUBLIC_RN_API_CLIENT_ID is hard-coded in .github/workflows/build.yml (lines 70–78) and duplicated in .env.example (line 8); it's consumed client-side in src/components/PaymentStep.tsx (line 38).
- Action: If sensitive → rotate and remove from client bundle, store in GitHub Secrets (do not expose via NEXT_PUBLIC). If non-sensitive/public → move values to repo variables (use ${{ vars.NAME }}) instead of hard-coding in the workflow; ensure PR/fork builds tolerate empty defaults.
67-79: Confirmed — build tolerates empty NEXT_PUBLIC_ placeholders.*
- RN_API_URL falls back to a default in src/components/payment-widget/constants.ts (process.env.NEXT_PUBLIC_REQUEST_API_URL || "https://api.request.network").
- PaymentWidget is gated by NEXT_PUBLIC_RN_API_CLIENT_ID in src/components/PaymentStep.tsx — empty string prevents rendering (no runtime error).
- DOCK links, Navbar hrefs and GoogleTagManager receive NEXT_PUBLIC_* directly (src/app/layout.tsx, src/components/Navbar.tsx); empty/placeholder values won't fail the build but will produce empty links or a non-functional/placeholder GTM tag (GTM-XXXXXXXX) at runtime.
- No usages of string methods (e.g. .length/.split) on NEXT_PUBLIC_* were found, so empty values are unlikely to throw during build.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Problem
Build check didn't run on PRs. Required manual trigger.
Changes
Copy
build-and-lint.ymlfrom https://github.com/RequestNetwork/easy-invoiceSummary by CodeRabbit