Skip to content

Conversation

@kumawatkaran523
Copy link
Contributor

@kumawatkaran523 kumawatkaran523 commented Sep 20, 2025

Fixes GitHub Pages deployment for Chainvoice by updating the GitHub Actions workflow to handle our Vite React frontend located in the frontend/ subfolder. Resolves package manager detection errors, corrects build output paths from Next.js (out) to Vite (dist).

Key Changes:

  • Updated package detection to look in frontend/ directory
  • Changed build output from ./out to ./frontend/dist
  • Fixed cache configuration to avoid "paths not resolved" errors
  • Added proper working-directory for all npm/yarn commands

Summary by CodeRabbit

  • New Features

    • Added batch invoice creation with encryption, token selection (including custom ERC‑20), and comprehensive UI.
    • Introduced batch payments for multiple invoices with token-aware grouping, approval handling, and detailed error messaging.
    • Added smart batch suggestions, selection controls, and a summary panel with per‑token totals.
    • Enhanced invoice views with a detail drawer, printable/downloadable invoices, and improved token logos.
    • Updated navigation and routing to include Batch Create and Batch Payment pages.
    • Token carousel now uses a dynamic on‑chain token list.
  • Chores

    • Replaced legacy deployment workflow with a new GitHub Pages pipeline for the frontend.

@coderabbitai
Copy link

coderabbitai bot commented Sep 20, 2025

Walkthrough

Adds a new GitHub Pages deploy workflow for a Vite React frontend and removes the old Next.js workflow. Introduces batch invoice creation and payment in the Chainvoice smart contract and frontend. Updates routing and navigation, switches token carousel to dynamic token list, and enhances received invoices with batch payment UI and flows.

Changes

Cohort / File(s) Summary
GitHub Actions: Deploy Workflows
.github/workflows/deploy.yml, .github/workflows/nextjs.yml
Added a Vite-based Pages deploy workflow targeting frontend/; removed the previous Next.js Pages workflow. New workflow handles package manager detection, caching, build, artifact upload, and deploy.
Smart Contract: Chainvoice Batch Ops
contracts/src/Chainvoice.sol
Added batch create/pay functions, new errors, events, constants, and fee handling; updated single-invoice payment to CEI pattern; introduced nonReentrant guard and token (native/ERC20) flows; adjusted view helpers.
Frontend: Routing & Navigation
frontend/src/App.jsx, frontend/src/page/Home.jsx
Added dashboard route batch-invoice and a “Batch Create” nav item; imported relevant components/icons; minor styling string change.
Frontend: Batch Pages
frontend/src/page/CreateInvoicesBatch.jsx, frontend/src/page/BatchPayment.jsx
New pages: batch invoice creation with Lit encryption and contract submission; batch payment with decryption, token-aware approvals, grouping, suggestions, and processing.
Frontend: Token Carousel Data Source
frontend/src/components/TokenCrousel.jsx
Replaced static presets with useTokenList('1'); updated image fallback handling; minor render adjustments and debug logging.
Frontend: Received Invoices Batch UX
frontend/src/page/ReceivedInvoice.jsx
Added batch selection, suggestions, grouped totals, per-token checks, approvals, and batch payment; enhanced error handling, toasts, and UI indicators.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant UI as Frontend (CreateInvoicesBatch)
  participant Lit as Lit Protocol
  participant Wallet as Wallet/Signer
  participant Chain as Chainvoice Contract

  User->>UI: Enter batch invoices + select token
  UI->>Lit: Encrypt each invoice payload
  Lit-->>UI: Ciphertexts + hashes
  UI->>Wallet: Request transaction (createInvoicesBatch)
  Wallet->>Chain: createInvoicesBatch(tos, amounts, token, payloads, hashes)
  Chain-->>Wallet: Tx receipt (InvoiceBatchCreated)
  UI-->>User: Batch created (ids)
Loading
sequenceDiagram
  autonumber
  actor User
  participant UI as Frontend (BatchPayment/Received)
  participant Lit as Lit Protocol
  participant ERC20 as ERC-20 Token
  participant Wallet as Wallet/Signer
  participant Chain as Chainvoice Contract

  User->>UI: Select invoices for batch
  UI->>Lit: Decrypt selected invoice details
  alt Token = ERC-20
    UI->>ERC20: Check allowance/balance
    opt Need approval
      UI->>Wallet: Approve token spend
      Wallet->>ERC20: approve(spender, amount)
    end
    UI->>Wallet: payInvoicesBatch(ids)
    Wallet->>Chain: payInvoicesBatch(ids)
  else Token = Native
    UI->>Wallet: payInvoicesBatch(ids, value)
    Wallet->>Chain: payInvoicesBatch(ids) with ETH
  end
  Chain-->>Wallet: Tx receipt (InvoiceBatchPaid)
  UI-->>User: Status updated (paid)
Loading
sequenceDiagram
  autonumber
  participant GH as GitHub Actions
  participant Repo as Repo (frontend/)
  participant Node as Node Setup
  participant Pages as GitHub Pages

  GH->>Repo: On push to main / dispatch
  GH->>Node: Setup Node 20 + cache deps
  GH->>Repo: Install + build (Vite) in frontend/
  GH->>Pages: Upload artifact
  GH->>Pages: Deploy (deploy-pages)
  Pages-->>GH: Page URL output
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Possibly related PRs

Suggested reviewers

  • adityabhattad2021

Poem

I hop through bills in tidy rows,
Stack by stack, the ledger grows.
A twitch, a tap—batch paid in flight,
Carrots count, the gas feels light.
Encrypt, dispatch, deploy the page—
Thump-thump! Another balanced stage. 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately and concisely captures the primary change: fixing the GitHub Actions workflow to deploy a Vite React frontend located in a subfolder. It mentions the key aspects (Vite, React, deployment, subfolder) and aligns with the PR objectives and modified workflow file, so a reviewer scanning history will understand the main intent.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
.github/workflows/deploy.yml (1)

1-105: Fix package-manager detection & cache key in deploy workflow

  • Findings: frontend/package.json contains "build" (vite build) but there is NO lockfile (no package-lock.json, no yarn.lock, no pnpm-lock.yaml). Current workflow will fall back to npm ci and uses hashFiles('frontend/package-lock.json') for the cache key — npm ci will fail without a lockfile and the cache key is ineffective.
  • Action (file: .github/workflows/deploy.yml): detect lockfiles (frontend/package-lock.json, frontend/yarn.lock, frontend/pnpm-lock.yaml) and package.json.packageManager; use npm ci only when package-lock.json exists, otherwise use npm install (and use the correct install command for yarn/pnpm); set runner accordingly. Update cache key to hash the available lockfile(s) with a fallback to package.json (e.g. hashFiles(...lockfiles..., 'frontend/package.json')). Add yarn/pnpm cache paths when relevant.
  • Verify frontend/vite.config.js base if the site will be served from a repo subpath (GitHub Pages).
frontend/src/components/TokenCrousel.jsx (1)

53-54: React keys use missing field; will cause unstable list

Tokens from useTokenList expose contract_address not address. Use a stable key.

-              key={`${token.address}-${index}`}
+              key={`${token.contract_address ?? index}-${index}`}
frontend/src/page/ReceivedInvoice.jsx (1)

631-635: Fix BigInt network check; current comparison can throw and block loading

ethers v6 returns bigint for chainId. Compare to 11155111n.

-        if (network.chainId != 11155111) {
+        if (network.chainId !== 11155111n) {
🧹 Nitpick comments (19)
.github/workflows/deploy.yml (6)

55-60: Use setup-node built-in caching tied to the detected lockfile.

Leverage actions/setup-node@v4 caching instead of a manual cache step; it’s simpler and avoids key mismatches.

       - name: Setup Node
         uses: actions/setup-node@v4
         with:
-          node-version: "20"
+          node-version: "20"
+          cache: ${{ steps.detect-package-manager.outputs.manager }}
+          cache-dependency-path: ${{ steps.detect-package-manager.outputs.lockfile_path }}

78-81: Install step OK; optional: enforce immutability for reproducible installs.

If you expect lockfiles to be authoritative, consider yarn install --frozen-lockfile (Yarn v1) or yarn install --immutable (Yarn Berry). You can branch on the Yarn version if needed.

Would you like me to add a small version check to pick the right Yarn flag?


83-86: Optional: Add SPA fallback for client-side routing on Pages.

If the app uses React Router, copy index.html to 404.html so deep links work on GitHub Pages.

       - name: Build with Vite
         run: ${{ steps.detect-package-manager.outputs.runner }} run build
         working-directory: ./frontend  # Run command in frontend subfolder
+
+      # Ensure SPA routing works on GitHub Pages
+      - name: Add 404.html fallback
+        if: ${{ always() }}  # harmless noop if files missing
+        run: cp dist/index.html dist/404.html || true
+        working-directory: ./frontend

18-23: Optionally scope concurrency by ref.

Not required since you deploy only from main, but using the ref avoids blocking other branches if triggers expand later.

 concurrency:
-  group: "pages"
+  group: pages-${{ github.ref }}
   cancel-in-progress: false

5-11: Trigger set is fine; consider adding a non-deploy PR check.

If you want CI to verify builds on PRs without deploying, add a pull_request trigger that runs only the build (guard deploy with if: github.event_name != 'pull_request').


33-92: One-shot consolidated patch (optional).

If you prefer a single change set adopting setup-node caching and SPA fallback:

@@
       - name: Detect package manager
         id: detect-package-manager
         run: |
-          # Check for yarn.lock first in frontend directory
-          if [ -f "${{ github.workspace }}/frontend/yarn.lock" ]; then
+          # Check for yarn/npm in frontend directory
+          if [ -f "${{ github.workspace }}/frontend/yarn.lock" ]; then
             echo "manager=yarn" >> $GITHUB_OUTPUT
             echo "command=install" >> $GITHUB_OUTPUT
             echo "runner=yarn" >> $GITHUB_OUTPUT
+            echo "lockfile_path=frontend/yarn.lock" >> $GITHUB_OUTPUT
             exit 0
-          # Check for package.json in frontend directory (npm)
-          elif [ -f "${{ github.workspace }}/frontend/package.json" ]; then
+          elif [ -f "${{ github.workspace }}/frontend/package-lock.json" ]; then
             echo "manager=npm" >> $GITHUB_OUTPUT
             echo "command=ci" >> $GITHUB_OUTPUT
             echo "runner=npm" >> $GITHUB_OUTPUT
+            echo "lockfile_path=frontend/package-lock.json" >> $GITHUB_OUTPUT
             exit 0
+          elif [ -f "${{ github.workspace }}/frontend/package.json" ]; then
+            echo "manager=npm" >> $GITHUB_OUTPUT
+            echo "command=install" >> $GITHUB_OUTPUT
+            echo "runner=npm" >> $GITHUB_OUTPUT
+            echo "lockfile_path=frontend/package-lock.json" >> $GITHUB_OUTPUT
+            exit 0
           else
             # Neither found - fail the build
             echo "Unable to determine package manager"
             exit 1
           fi
@@
       - name: Setup Node
         uses: actions/setup-node@v4
         with:
-          node-version: "20"
+          node-version: "20"
+          cache: ${{ steps.detect-package-manager.outputs.manager }}
+          cache-dependency-path: ${{ steps.detect-package-manager.outputs.lockfile_path }}
@@
-      - name: Cache node modules
-        uses: actions/cache@v4
-        with:
-          path: |
-            frontend/node_modules  # Cache installed packages
-            ~/.npm                 # Cache npm's global cache
-          # Create unique cache key based on package-lock.json content
-          key: ${{ runner.os }}-node-${{ hashFiles('frontend/package-lock.json') }}
-          restore-keys: |
-            ${{ runner.os }}-node-  # Fallback to any cached version
+      # Caching handled by setup-node above
@@
       - name: Build with Vite
         run: ${{ steps.detect-package-manager.outputs.runner }} run build
         working-directory: ./frontend  # Run command in frontend subfolder
+      - name: Add 404.html fallback
+        if: ${{ always() }}
+        run: cp dist/index.html dist/404.html || true
+        working-directory: ./frontend
frontend/src/components/TokenCrousel.jsx (2)

60-69: Harden image fallback

Use currentTarget to avoid bubbling issues and ensure a predictable element reference.

-                    onError={(e) => {
-                      e.target.src = "/tokenImages/generic.png";
-                    }}
+                    onError={(e) => {
+                      e.currentTarget.src = "/tokenImages/generic.png";
+                    }}

71-76: Zero‑address check uses non‑existent field

Hook returns contract_address; your ETH badge condition will never trigger.

-                  {token.address ===
+                  {token.contract_address ===
                     "0x0000000000000000000000000000000000000000" && (
frontend/src/page/CreateInvoicesBatch.jsx (2)

329-339: Validate recipient addresses client‑side before submit

Add ethers.isAddress check to avoid revert noise from invalid inputs.

-      const validInvoices = invoiceRows.filter(
-        (row) => row.clientAddress && parseFloat(row.totalAmountDue) > 0
-      );
+      const validInvoices = invoiceRows.filter(
+        (row) =>
+          ethers.isAddress(row.clientAddress) &&
+          parseFloat(row.totalAmountDue) > 0
+      );

535-541: Prop mismatch: WalletConnectionAlert ignores message

The component signature is { show, onDismiss }. Either pass supported props or extend the component.

frontend/src/page/ReceivedInvoice.jsx (2)

1365-1368: Harden image fallback

Use currentTarget to avoid React synthetic event pitfalls.

-                                    onError={(e) => {
-                                      e.target.src = "/tokenImages/generic.png";
-                                    }}
+                                    onError={(e) => {
+                                      e.currentTarget.src = "/tokenImages/generic.png";
+                                    }}

Also applies to: 1124-1127


890-896: Prop mismatch: WalletConnectionAlert ignores message

Same as CreateInvoicesBatch; either wire the prop or remove it.

contracts/src/Chainvoice.sol (1)

84-101: Require non‑zero amount on single‑invoice create (batch already checks)

Prevents useless/costly zero‑amount entries and aligns with batch path.

     function createInvoice(
         address to,
         uint256 amountDue,
         address tokenAddress,
         string memory encryptedInvoiceData,
         string memory encryptedHash
     ) external {
         require(to != address(0), "Recipient address is zero");
         require(to != msg.sender, "Self-invoicing not allowed");
+        require(amountDue > 0, "Amount zero");
frontend/src/App.jsx (1)

36-38: Remove unused BatchPayment import or add its route

Currently imported but not routed/used.

-import BatchPayment from "./page/BatchPayment"; // New import needed

Also applies to: 88-92

frontend/src/page/Home.jsx (2)

97-104: Tighten selected-state matching to avoid false positives

includes() can mis-highlight when routes are substrings of others. Match on the trailing segment.

Apply this diff:

-                      selected={location.pathname.includes(item.route)}
+                      selected={location.pathname.endsWith("/" + item.route)}

1-1: Header comment vs nav contents

Comment mentions Batch Payment, but the nav only adds “Batch Create”. Either add the Batch Payment link (if intended) or update the comment for accuracy.

Is Batch Payment intentionally accessed from elsewhere (e.g., Received Invoices) rather than from this menu?

frontend/src/page/BatchPayment.jsx (3)

728-731: Unify notifications: replace alert() with toasts

Keep UX consistent with react-toastify.

Apply this diff:

-          alert(
-            `Approval for ${tokenSymbol} completed! Now processing payment...`
-          );
+          toast.success(`Approval for ${tokenSymbol} completed! Now processing payment...`);
...
-        alert("Payment successful in ETH!");
+        toast.success("Payment successful in ETH!");

Also applies to: 748-749


800-814: Guard against missing injected provider and avoid alert()

Handle non-EIP‑1193 environments gracefully; use toasts.

Apply this diff:

   const switchNetwork = async () => {
     try {
       setNetworkLoading(true);
+      if (!window?.ethereum) {
+        toast.error("No injected wallet found. Please install MetaMask or a compatible wallet.");
+        return;
+      }
       await window.ethereum.request({
         method: "wallet_switchEthereumChain",
         params: [{ chainId: "0xaa36a7" }], // Sepolia chain ID
       });
       setError(null);
     } catch (error) {
       console.error("Network switch failed:", error);
-      alert("Failed to switch network. Please switch to Sepolia manually.");
+      toast.error("Failed to switch network. Please switch to Sepolia manually.");
     } finally {
       setNetworkLoading(false);
     }
   };

526-528: Fee state type and display

contract.fee() returns a BigInt in ethers v6, while fee is initialized as number (0). It works, but mixing types can be brittle. Consider initializing fee as 0n and always treating it as BigInt when stored/displayed.

Apply this diff:

-  const [fee, setFee] = useState(0);
+  const [fee, setFee] = useState(0n);
...
-        const fee = await contract.fee();
-        setFee(fee);
+        const onChainFee = await contract.fee();
+        setFee(onChainFee);

The existing ethers.formatUnits(fee) will keep working with BigInt.

Also applies to: 1558-1575

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4bbd83a and 61a3615.

📒 Files selected for processing (9)
  • .github/workflows/deploy.yml (1 hunks)
  • .github/workflows/nextjs.yml (0 hunks)
  • contracts/src/Chainvoice.sol (5 hunks)
  • frontend/src/App.jsx (2 hunks)
  • frontend/src/components/TokenCrousel.jsx (2 hunks)
  • frontend/src/page/BatchPayment.jsx (1 hunks)
  • frontend/src/page/CreateInvoicesBatch.jsx (1 hunks)
  • frontend/src/page/Home.jsx (4 hunks)
  • frontend/src/page/ReceivedInvoice.jsx (19 hunks)
💤 Files with no reviewable changes (1)
  • .github/workflows/nextjs.yml
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/components/TokenCrousel.jsx (1)
frontend/src/hooks/useTokenList.js (2)
  • useTokenList (18-86)
  • tokens (19-19)
frontend/src/page/CreateInvoicesBatch.jsx (10)
frontend/src/page/BatchPayment.jsx (6)
  • walletClient (39-39)
  • useAccount (40-40)
  • loading (41-41)
  • litClientRef (48-48)
  • showWalletAlert (51-51)
  • error (46-46)
frontend/src/page/ReceivedInvoice.jsx (6)
  • walletClient (74-74)
  • useAccount (75-75)
  • loading (76-76)
  • litClientRef (81-81)
  • showWalletAlert (84-84)
  • error (79-79)
frontend/src/components/WalletConnectionAlert.jsx (1)
  • WalletConnectionAlert (7-56)
frontend/src/components/ui/label.jsx (1)
  • Label (11-13)
frontend/src/components/ui/button.jsx (1)
  • Button (37-45)
frontend/src/components/ui/popover.jsx (3)
  • Popover (6-6)
  • PopoverTrigger (8-8)
  • PopoverContent (12-24)
frontend/src/components/ui/input.jsx (1)
  • Input (5-16)
frontend/src/components/TokenPicker.jsx (1)
  • TokenPicker (175-382)
frontend/src/components/ui/copyButton.jsx (1)
  • CopyButton (5-39)
frontend/src/lib/utils.js (1)
  • cn (4-6)
frontend/src/page/ReceivedInvoice.jsx (3)
frontend/src/page/SentInvoice.jsx (10)
  • columns (51-58)
  • drawerState (346-349)
  • error (69-69)
  • getTokenInfo (82-90)
  • fee (68-68)
  • walletClient (63-63)
  • switchNetwork (406-420)
  • paymentLoading (72-72)
  • toggleDrawer (351-363)
  • handlePrint (365-376)
frontend/src/page/BatchPayment.jsx (26)
  • selectedInvoices (43-43)
  • batchLoading (44-44)
  • batchSuggestions (53-53)
  • drawerState (56-59)
  • error (46-46)
  • getTokenInfo (74-81)
  • getTokenSymbol (84-87)
  • detectBatchFromMetadata (90-100)
  • findBatchSuggestions (103-167)
  • fee (45-45)
  • getGroupedInvoices (277-305)
  • grouped (831-831)
  • receivedInvoices (42-42)
  • handleSelectInvoice (250-263)
  • handleSelectAll (265-270)
  • unpaidInvoices (827-829)
  • handleClearAll (272-274)
  • selectBatchSuggestion (220-224)
  • payEntireBatch (227-247)
  • handleBatchPayment (540-678)
  • payInvoice (681-770)
  • walletClient (39-39)
  • switchNetwork (800-814)
  • paymentLoading (49-49)
  • toggleDrawer (773-785)
  • handlePrint (787-798)
frontend/src/hooks/useTokenList.js (2)
  • error (21-21)
  • tokens (19-19)
frontend/src/page/BatchPayment.jsx (1)
frontend/src/page/ReceivedInvoice.jsx (24)
  • page (72-72)
  • walletClient (74-74)
  • receivedInvoices (77-77)
  • selectedInvoices (91-91)
  • fee (78-78)
  • error (79-79)
  • drawerState (96-99)
  • getTokenInfo (175-182)
  • getTokenSymbol (184-187)
  • detectBatchFromMetadata (189-199)
  • findBatchSuggestions (201-234)
  • selectBatchSuggestion (339-343)
  • payEntireBatch (345-364)
  • handleBatchPayment (469-590)
  • handleSelectInvoice (313-326)
  • handleSelectAll (328-333)
  • unpaidInvoices (882-884)
  • handleClearAll (335-337)
  • getGroupedInvoices (276-302)
  • grouped (886-886)
  • payInvoice (367-466)
  • toggleDrawer (819-831)
  • handlePrint (833-850)
  • switchNetwork (852-869)
🔇 Additional comments (7)
.github/workflows/deploy.yml (4)

88-92: Artifact path looks correct for Vite.

./frontend/dist is the right upload path. No changes needed.


94-105: Deploy job wiring LGTM.

  • needs: build, actions/deploy-pages@v4, and environment URL exposure are correct.

If you see the environment URL not set on the run, it’s a transient Actions quirk; re-run should fix. Otherwise ping and I’ll adjust.


73-76: Pages configuration placement is correct.

Configuring Pages before uploading the artifact matches the recommended sequence.


78-86: Verified: frontend/package.json contains a Vite build script.
scripts.build = "vite build"

contracts/src/Chainvoice.sol (1)

246-318: Batch pay CEI looks good

Effects-before-interactions with nonReentrant and explicit allowance/value checks minimize reentrancy and partial‑settlement risks.

Please confirm MAX_BATCH aligns with frontend limits (both use 50).

frontend/src/page/Home.jsx (1)

111-112: Border-left styling change looks good

String concatenation is fine here; no functional issue.

frontend/src/page/BatchPayment.jsx (1)

336-353: Network check UX

You set error and return when not on Sepolia. Consider showing the “Switch to Sepolia” CTA even when loading finishes (you already do), and optionally auto-invoking switchNetwork upon user confirmation.

Would you like this screen to offer an automatic “Switch and retry” action?

Comment on lines 33 to 54
# Detect whether project uses npm or yarn by checking lock files
- name: Detect package manager
id: detect-package-manager
run: |
# Check for yarn.lock first in frontend directory
if [ -f "${{ github.workspace }}/frontend/yarn.lock" ]; then
echo "manager=yarn" >> $GITHUB_OUTPUT
echo "command=install" >> $GITHUB_OUTPUT
echo "runner=yarn" >> $GITHUB_OUTPUT
exit 0
# Check for package.json in frontend directory (npm)
elif [ -f "${{ github.workspace }}/frontend/package.json" ]; then
echo "manager=npm" >> $GITHUB_OUTPUT
echo "command=ci" >> $GITHUB_OUTPUT
echo "runner=npm" >> $GITHUB_OUTPUT
exit 0
else
# Neither found - fail the build
echo "Unable to determine package manager"
exit 1
fi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Make npm detection robust and expose lockfile for caching.

If frontend/package.json exists but package-lock.json doesn’t, npm ci will fail. Also, you’re not exposing the lockfile path, which is useful for cache keys. Recommend:

  • Prefer npm ci only when package-lock.json exists; otherwise fall back to npm install.
  • Output lockfile_path for use by caching.
-      - name: Detect package manager
+      - name: Detect package manager
         id: detect-package-manager
         run: |
-          # Check for yarn.lock first in frontend directory
-          if [ -f "${{ github.workspace }}/frontend/yarn.lock" ]; then
-            echo "manager=yarn" >> $GITHUB_OUTPUT
-            echo "command=install" >> $GITHUB_OUTPUT
-            echo "runner=yarn" >> $GITHUB_OUTPUT
-            exit 0
-          # Check for package.json in frontend directory (npm)
-          elif [ -f "${{ github.workspace }}/frontend/package.json" ]; then
-            echo "manager=npm" >> $GITHUB_OUTPUT
-            echo "command=ci" >> $GITHUB_OUTPUT
-            echo "runner=npm" >> $GITHUB_OUTPUT
-            exit 0
-          else
-            # Neither found - fail the build
-            echo "Unable to determine package manager"
-            exit 1
-          fi
+          FRONTEND="${{ github.workspace }}/frontend"
+          if [ -f "$FRONTEND/yarn.lock" ]; then
+            echo "manager=yarn" >> $GITHUB_OUTPUT
+            echo "command=install" >> $GITHUB_OUTPUT
+            echo "runner=yarn" >> $GITHUB_OUTPUT
+            echo "lockfile_path=frontend/yarn.lock" >> $GITHUB_OUTPUT
+            exit 0
+          elif [ -f "$FRONTEND/package-lock.json" ]; then
+            echo "manager=npm" >> $GITHUB_OUTPUT
+            echo "command=ci" >> $GITHUB_OUTPUT
+            echo "runner=npm" >> $GITHUB_OUTPUT
+            echo "lockfile_path=frontend/package-lock.json" >> $GITHUB_OUTPUT
+            exit 0
+          elif [ -f "$FRONTEND/package.json" ]; then
+            echo "manager=npm" >> $GITHUB_OUTPUT
+            echo "command=install" >> $GITHUB_OUTPUT
+            echo "runner=npm" >> $GITHUB_OUTPUT
+            echo "lockfile_path=frontend/package-lock.json" >> $GITHUB_OUTPUT
+            exit 0
+          else
+            echo "Unable to determine package manager (missing frontend/)"
+            exit 1
+          fi
📝 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.

Suggested change
# Detect whether project uses npm or yarn by checking lock files
- name: Detect package manager
id: detect-package-manager
run: |
# Check for yarn.lock first in frontend directory
if [ -f "${{ github.workspace }}/frontend/yarn.lock" ]; then
echo "manager=yarn" >> $GITHUB_OUTPUT
echo "command=install" >> $GITHUB_OUTPUT
echo "runner=yarn" >> $GITHUB_OUTPUT
exit 0
# Check for package.json in frontend directory (npm)
elif [ -f "${{ github.workspace }}/frontend/package.json" ]; then
echo "manager=npm" >> $GITHUB_OUTPUT
echo "command=ci" >> $GITHUB_OUTPUT
echo "runner=npm" >> $GITHUB_OUTPUT
exit 0
else
# Neither found - fail the build
echo "Unable to determine package manager"
exit 1
fi
# Detect whether project uses npm or yarn by checking lock files
- name: Detect package manager
id: detect-package-manager
run: |
FRONTEND="${{ github.workspace }}/frontend"
if [ -f "$FRONTEND/yarn.lock" ]; then
echo "manager=yarn" >> $GITHUB_OUTPUT
echo "command=install" >> $GITHUB_OUTPUT
echo "runner=yarn" >> $GITHUB_OUTPUT
echo "lockfile_path=frontend/yarn.lock" >> $GITHUB_OUTPUT
exit 0
elif [ -f "$FRONTEND/package-lock.json" ]; then
echo "manager=npm" >> $GITHUB_OUTPUT
echo "command=ci" >> $GITHUB_OUTPUT
echo "runner=npm" >> $GITHUB_OUTPUT
echo "lockfile_path=frontend/package-lock.json" >> $GITHUB_OUTPUT
exit 0
elif [ -f "$FRONTEND/package.json" ]; then
echo "manager=npm" >> $GITHUB_OUTPUT
echo "command=install" >> $GITHUB_OUTPUT
echo "runner=npm" >> $GITHUB_OUTPUT
echo "lockfile_path=frontend/package-lock.json" >> $GITHUB_OUTPUT
exit 0
else
echo "Unable to determine package manager (missing frontend/)"
exit 1
fi
🤖 Prompt for AI Agents
In .github/workflows/deploy.yml around lines 33 to 54, the package manager
detection sets npm to use "ci" whenever package.json exists (which will fail if
package-lock.json is missing) and doesn't expose a lockfile path for caching;
change the logic so that if frontend/yarn.lock exists you emit manager=yarn,
command=install, runner=yarn and lockfile_path=frontend/yarn.lock; else if
frontend/package.json exists and frontend/package-lock.json exists emit
manager=npm, command=ci, runner=npm and
lockfile_path=frontend/package-lock.json; else if frontend/package.json exists
but no package-lock.json emit manager=npm, command=install, runner=npm and set
lockfile_path to an empty value (or omit it) so callers know no lockfile is
available; ensure each branch writes the lockfile_path to $GITHUB_OUTPUT before
exiting and keep the existing exit status behavior.

Comment on lines 61 to 72
# Cache node_modules and npm cache for faster builds
- name: Cache node modules
uses: actions/cache@v4
with:
path: |
frontend/node_modules # Cache installed packages
~/.npm # Cache npm's global cache
# Create unique cache key based on package-lock.json content
key: ${{ runner.os }}-node-${{ hashFiles('frontend/package-lock.json') }}
restore-keys: |
${{ runner.os }}-node- # Fallback to any cached version
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix cache key and paths (or remove in favor of setup-node caching).

Current cache always keys on frontend/package-lock.json and caches ~/.npm, which is wrong for Yarn projects and yields low hit rates. If you keep a manual cache, condition it on the detected manager and use the correct lockfile and cache paths. Otherwise, drop this step entirely when using setup-node caching.

-      - name: Cache node modules
-        uses: actions/cache@v4
-        with:
-          path: |
-            frontend/node_modules  # Cache installed packages
-            ~/.npm                 # Cache npm's global cache
-          # Create unique cache key based on package-lock.json content
-          key: ${{ runner.os }}-node-${{ hashFiles('frontend/package-lock.json') }}
-          restore-keys: |
-            ${{ runner.os }}-node-  # Fallback to any cached version
+      # (Optional) Remove this block when using setup-node caching above.
📝 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.

Suggested change
# Cache node_modules and npm cache for faster builds
- name: Cache node modules
uses: actions/cache@v4
with:
path: |
frontend/node_modules # Cache installed packages
~/.npm # Cache npm's global cache
# Create unique cache key based on package-lock.json content
key: ${{ runner.os }}-node-${{ hashFiles('frontend/package-lock.json') }}
restore-keys: |
${{ runner.os }}-node- # Fallback to any cached version
# Cache node_modules and npm cache for faster builds
# (Optional) Remove this block when using setup-node caching above.
🤖 Prompt for AI Agents
.github/workflows/deploy.yml lines 61-72: the cache step always uses
frontend/package-lock.json and ~/.npm which is incorrect for Yarn/PNPM and
causes poor cache hits; either remove this manual cache step and rely on
actions/setup-node's built-in caching (set cache: 'npm'/'yarn'/'pnpm'), or make
the step manager-aware: detect the package manager (npm/yarn/pnpm), use the
correct lockfile in the key (package-lock.json OR yarn.lock OR pnpm-lock.yaml),
and set the appropriate cache paths (~/.npm for npm, ~/.cache/yarn OR
frontend/.yarn/cache for Yarn, ~/.pnpm-store for pnpm); ensure the cache key
includes runner.os and the hashFiles of the chosen lockfile and provide sensible
restore-keys fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant