Skip to content

fix: order of workflow actions—install deps before netlify step#16449

Merged
corwintines merged 1 commit into
devfrom
playwright-action
Oct 6, 2025
Merged

fix: order of workflow actions—install deps before netlify step#16449
corwintines merged 1 commit into
devfrom
playwright-action

Conversation

@wackerow
Copy link
Copy Markdown
Member

@wackerow wackerow commented Oct 6, 2025

Description

Reorders steps for playwright E2E Testing workflow to install dependencies prior to Netlify involvement

Related Issue

https://github.com/ethereum/ethereum-org-website/actions/runs/18140948786/job/51631376977#step:3:44

@netlify
Copy link
Copy Markdown

netlify Bot commented Oct 6, 2025

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit d1624b1
🔍 Latest deploy log https://app.netlify.com/projects/ethereumorg/deploys/68e4295f8f3c930008063e0f
😎 Deploy Preview https://deploy-preview-16449--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 51 (🟢 up 8 from production)
Accessibility: 94 (no change from production)
Best Practices: 92 (no change from production)
SEO: 92 (no change from production)
PWA: 59 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions Bot added the tooling 🔧 Changes related to tooling of the project label Oct 6, 2025
@wackerow
Copy link
Copy Markdown
Member Author

wackerow commented Oct 6, 2025

@claude please review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Oct 6, 2025

Claude finished @wackerow's task —— View job


Code Review

Todo List

  • Read and analyze the workflow file changes
  • Review the fix for dependency installation order
  • Provide comprehensive review feedback

Review Findings

✅ LGTM - This is a good fix that addresses a logical workflow dependency issue.

What Changed

The PR reorders the Playwright workflow steps to install Node.js, pnpm, and dependencies before attempting to get the Netlify deploy URL, instead of after.

Before:

- checkout
- Get Netlify Deploy URL (runs ts-node script)
- Setup Node.js
- Setup pnpm  
- Install dependencies
- Install Playwright

After:

- checkout
- Setup Node.js
- Setup pnpm
- Install dependencies  
- Get Netlify Deploy URL (runs ts-node script)
- Install Playwright

Why This Fix Makes Sense

  1. Dependency Resolution: The "Get Netlify Deploy URL" step runs a TypeScript script with ts-node:

    run: npx ts-node -O '{ "module": "commonjs" }' src/scripts/get-netlify-branch-deploy.ts
  2. Missing Prerequisites: This script requires:

    • Node.js runtime ✅ (now installed first)
    • ts-node package ✅ (now available via pnpm install)
    • TypeScript dependencies ✅ (now installed)
  3. Root Cause: The original failure was likely ts-node command not found, since dependencies weren't installed yet.

Code Quality Assessment

  • Logic: ✅ Correct dependency ordering
  • Consistency: ✅ Follows standard CI/CD patterns (setup → install → execute)
  • Performance: ✅ No negative impact, may be slightly faster since failing early
  • Readability: ✅ More intuitive step progression

Minor Suggestions

The workflow is well-structured overall. The only minor enhancement could be adding error handling, but that's outside the scope of this fix.

Conclusion

This fix correctly addresses the workflow failure by ensuring all required dependencies are available before executing the Netlify script. The reordering is logical and follows CI/CD best practices.

Recommendation: ✅ Approve and merge


@corwintines corwintines merged commit 843c3d2 into dev Oct 6, 2025
8 of 9 checks passed
@corwintines corwintines deleted the playwright-action branch October 6, 2025 23:50
This was referenced Oct 9, 2025
This was referenced Oct 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tooling 🔧 Changes related to tooling of the project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants