Skip to content

Conversation

@snomiao
Copy link
Member

@snomiao snomiao commented Sep 2, 2025

Summary

  • Merged pr-playwright-deploy.yaml and pr-playwright-comment.yaml into a single workflow file
  • Fixed race condition where comment job didn't wait for deployment to complete
  • Ensures deployment URLs are available when generating PR comments

Test plan

  • Verify CI runs correctly on PR
  • Confirm deployment URLs appear in PR comments
  • Check both "tests starting" and "tests completed" comments work

🤖 Generated with Claude Code

┆Issue is synchronized with this Notion page by Unito

Combines pr-playwright-deploy.yaml and pr-playwright-comment.yaml into a single workflow to ensure proper dependency ordering. The comment job now waits for deployments to complete before generating comments with deployment URLs.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Sep 2, 2025

🎭 Playwright Test Results

All tests passed across all browsers!

⏰ Completed at: 09/02/2025, 04:05:03 AM UTC

📊 Test Reports by Browser


🎉 Your tests are passing across all browsers!

@snomiao snomiao marked this pull request as ready for review September 2, 2025 03:44
@snomiao snomiao requested a review from a team as a code owner September 2, 2025 03:44
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 2, 2025
@snomiao
Copy link
Member Author

snomiao commented Sep 2, 2025

@christian-byrne
(note) This change only works after merged to main, so review flow is: just review codes and merge, and revert if not working

@christian-byrne christian-byrne merged commit f215872 into main Sep 2, 2025
17 checks passed
@christian-byrne christian-byrne deleted the sno-fix-playwright-comment branch September 2, 2025 04:34
@benceruleanlu benceruleanlu mentioned this pull request Sep 3, 2025
@snomiao
Copy link
Member Author

snomiao commented Sep 3, 2025

@christian-byrne (note) This change only works after merged to main, so review flow is: just review codes and merge, and revert if not working

seems still not working yet....

unlucky

snomiao added a commit that referenced this pull request Sep 3, 2025
…links

The issue in PR #5298 was caused by missing deployment-info artifact
creation. The deploy-reports job was deploying to Cloudflare but wasn't
creating the deployment-info-* artifacts that comment-tests-completed
job expected to download.

This change consolidates the deployment and commenting into a single job,
eliminating the artifact dependency and ensuring links are always available.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
snomiao added a commit that referenced this pull request Sep 8, 2025
…links

The issue in PR #5298 was caused by missing deployment-info artifact
creation. The deploy-reports job was deploying to Cloudflare but wasn't
creating the deployment-info-* artifacts that comment-tests-completed
job expected to download.

This change consolidates the deployment and commenting into a single job,
eliminating the artifact dependency and ensuring links are always available.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
christian-byrne pushed a commit that referenced this pull request Sep 9, 2025
* [fix] Consolidate Playwright workflow jobs to fix missing deployment links

The issue in PR #5298 was caused by missing deployment-info artifact
creation. The deploy-reports job was deploying to Cloudflare but wasn't
creating the deployment-info-* artifacts that comment-tests-completed
job expected to download.

This change consolidates the deployment and commenting into a single job,
eliminating the artifact dependency and ensuring links are always available.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: Split PR deployment workflow for forked vs non-forked repos

- Extract deployment logic to reusable script (scripts/cicd/pr-playwright-deploy-and-comment.sh)
- Non-forked PRs: Use direct pull_request event in test-ui.yaml (faster)
- Forked PRs: Use workflow_run in pr-playwright-deploy.yaml (handles permissions)
- Add starting comment for both forked and non-forked PRs
- Make Cloudflare tokens optional for starting status comments

* refactor: Simplify PR deployment workflow and script

- Consolidate workflow into single job with clearer structure
- Reduce script from 200+ to ~140 lines
- Simplify deployment retry logic and comment generation
- Remove redundant checks and unnecessary complexity

* fix: Add debugging and wrangler installation to deployment script

- Add debug output to identify missing reports
- Install wrangler if not available
- Show deployment attempts and failures
- Log available reports before deployment

* chore: Trigger CI to test deployment workflow

* fix: Fix browser artifact name mismatch in deployment script

- Use dot notation (0.5x) for artifact names as Playwright creates them
- Convert to dash notation (0-5x) for Cloudflare project names
- Properly handle browser name display in comments

* refactor: Convert deployment script to POSIX sh for better compatibility

- Replace bash arrays with space-separated strings
- Use while loops instead of bash-specific for syntax
- Remove bash-specific string manipulation features
- Replace local variables (not required in functions)
- Ensure compatibility with standard /bin/sh

* fix: Fix deployment script output to properly capture URLs

- Redirect debug messages to stderr
- Only output URL to stdout for proper capture
- This fixes the missing deployment links in PR comments

* fix: Add input validation to prevent command injection

- Validate PR number is numeric only
- Sanitize branch name at script start
- Validate status parameter values
- Use pre-sanitized branch throughout script
- Addresses high-severity security issue from PR review

* fix: Add null checks and logging to workflow condition

- Add explicit null checks for head_repository and repository
- Add debug logging to help diagnose workflow trigger issues
- Prevents potential failures from undefined repository objects
- Addresses medium-severity issue from PR review

* fix: Pin wrangler to major version 4 with error handling

- Pin wrangler to major version 4 (^4.0.0) for stability
- Add error handling if wrangler installation fails
- Return 'failed' status if installation fails
- Addresses dependency management issue from PR review

* perf: Implement parallel deployments to reduce CI time

- Deploy all browser reports in parallel using background processes
- Use temporary directory to collect deployment results
- Wait for all deployments to complete before generating comment
- Maintains result order for consistent output
- Significantly reduces deployment time from sequential to parallel execution

* fix: Use specific comment ID for updates instead of edit-last

- Use GitHub API to find exact comment ID
- Update specific comment by ID to avoid editing wrong comment
- Prevents race conditions if user posts between finding and editing
- More reliable comment updates

* fix(workflows/test-ui.yaml): change condition to always run deploy job for pull requests to ensure deployment consistency

* fix(workflows/test-ui.yaml): change condition to always run deploy job for pull requests to ensure deployment consistency

* fix(pr-playwright-deploy-and-comment.sh): remove npx prefix from wrangler command for consistency and simplicity

* fix(pr-playwright-deploy-and-comment.sh): remove npx prefix from wrangler command for consistency and simplicity

* Update scripts/cicd/pr-playwright-deploy-and-comment.sh

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix(pr-playwright-deploy-and-comment.sh): improve regex for URL extraction to include valid characters and ensure correct URL format

* chore(pr-playwright-deploy-and-comment.sh): move wrangler installation to the beginning of the script to avoid redundancy and improve efficiency

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
arjansingh pushed a commit that referenced this pull request Sep 11, 2025
* [fix] Consolidate Playwright workflow jobs to fix missing deployment links

The issue in PR #5298 was caused by missing deployment-info artifact
creation. The deploy-reports job was deploying to Cloudflare but wasn't
creating the deployment-info-* artifacts that comment-tests-completed
job expected to download.

This change consolidates the deployment and commenting into a single job,
eliminating the artifact dependency and ensuring links are always available.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: Split PR deployment workflow for forked vs non-forked repos

- Extract deployment logic to reusable script (scripts/cicd/pr-playwright-deploy-and-comment.sh)
- Non-forked PRs: Use direct pull_request event in test-ui.yaml (faster)
- Forked PRs: Use workflow_run in pr-playwright-deploy.yaml (handles permissions)
- Add starting comment for both forked and non-forked PRs
- Make Cloudflare tokens optional for starting status comments

* refactor: Simplify PR deployment workflow and script

- Consolidate workflow into single job with clearer structure
- Reduce script from 200+ to ~140 lines
- Simplify deployment retry logic and comment generation
- Remove redundant checks and unnecessary complexity

* fix: Add debugging and wrangler installation to deployment script

- Add debug output to identify missing reports
- Install wrangler if not available
- Show deployment attempts and failures
- Log available reports before deployment

* chore: Trigger CI to test deployment workflow

* fix: Fix browser artifact name mismatch in deployment script

- Use dot notation (0.5x) for artifact names as Playwright creates them
- Convert to dash notation (0-5x) for Cloudflare project names
- Properly handle browser name display in comments

* refactor: Convert deployment script to POSIX sh for better compatibility

- Replace bash arrays with space-separated strings
- Use while loops instead of bash-specific for syntax
- Remove bash-specific string manipulation features
- Replace local variables (not required in functions)
- Ensure compatibility with standard /bin/sh

* fix: Fix deployment script output to properly capture URLs

- Redirect debug messages to stderr
- Only output URL to stdout for proper capture
- This fixes the missing deployment links in PR comments

* fix: Add input validation to prevent command injection

- Validate PR number is numeric only
- Sanitize branch name at script start
- Validate status parameter values
- Use pre-sanitized branch throughout script
- Addresses high-severity security issue from PR review

* fix: Add null checks and logging to workflow condition

- Add explicit null checks for head_repository and repository
- Add debug logging to help diagnose workflow trigger issues
- Prevents potential failures from undefined repository objects
- Addresses medium-severity issue from PR review

* fix: Pin wrangler to major version 4 with error handling

- Pin wrangler to major version 4 (^4.0.0) for stability
- Add error handling if wrangler installation fails
- Return 'failed' status if installation fails
- Addresses dependency management issue from PR review

* perf: Implement parallel deployments to reduce CI time

- Deploy all browser reports in parallel using background processes
- Use temporary directory to collect deployment results
- Wait for all deployments to complete before generating comment
- Maintains result order for consistent output
- Significantly reduces deployment time from sequential to parallel execution

* fix: Use specific comment ID for updates instead of edit-last

- Use GitHub API to find exact comment ID
- Update specific comment by ID to avoid editing wrong comment
- Prevents race conditions if user posts between finding and editing
- More reliable comment updates

* fix(workflows/test-ui.yaml): change condition to always run deploy job for pull requests to ensure deployment consistency

* fix(workflows/test-ui.yaml): change condition to always run deploy job for pull requests to ensure deployment consistency

* fix(pr-playwright-deploy-and-comment.sh): remove npx prefix from wrangler command for consistency and simplicity

* fix(pr-playwright-deploy-and-comment.sh): remove npx prefix from wrangler command for consistency and simplicity

* Update scripts/cicd/pr-playwright-deploy-and-comment.sh

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix(pr-playwright-deploy-and-comment.sh): improve regex for URL extraction to include valid characters and ensure correct URL format

* chore(pr-playwright-deploy-and-comment.sh): move wrangler installation to the beginning of the script to avoid redundancy and improve efficiency

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
snomiao added a commit that referenced this pull request Sep 12, 2025
* [fix] Consolidate Playwright workflow jobs to fix missing deployment links

The issue in PR #5298 was caused by missing deployment-info artifact
creation. The deploy-reports job was deploying to Cloudflare but wasn't
creating the deployment-info-* artifacts that comment-tests-completed
job expected to download.

This change consolidates the deployment and commenting into a single job,
eliminating the artifact dependency and ensuring links are always available.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: Split PR deployment workflow for forked vs non-forked repos

- Extract deployment logic to reusable script (scripts/cicd/pr-playwright-deploy-and-comment.sh)
- Non-forked PRs: Use direct pull_request event in test-ui.yaml (faster)
- Forked PRs: Use workflow_run in pr-playwright-deploy.yaml (handles permissions)
- Add starting comment for both forked and non-forked PRs
- Make Cloudflare tokens optional for starting status comments

* refactor: Simplify PR deployment workflow and script

- Consolidate workflow into single job with clearer structure
- Reduce script from 200+ to ~140 lines
- Simplify deployment retry logic and comment generation
- Remove redundant checks and unnecessary complexity

* fix: Add debugging and wrangler installation to deployment script

- Add debug output to identify missing reports
- Install wrangler if not available
- Show deployment attempts and failures
- Log available reports before deployment

* chore: Trigger CI to test deployment workflow

* fix: Fix browser artifact name mismatch in deployment script

- Use dot notation (0.5x) for artifact names as Playwright creates them
- Convert to dash notation (0-5x) for Cloudflare project names
- Properly handle browser name display in comments

* refactor: Convert deployment script to POSIX sh for better compatibility

- Replace bash arrays with space-separated strings
- Use while loops instead of bash-specific for syntax
- Remove bash-specific string manipulation features
- Replace local variables (not required in functions)
- Ensure compatibility with standard /bin/sh

* fix: Fix deployment script output to properly capture URLs

- Redirect debug messages to stderr
- Only output URL to stdout for proper capture
- This fixes the missing deployment links in PR comments

* fix: Add input validation to prevent command injection

- Validate PR number is numeric only
- Sanitize branch name at script start
- Validate status parameter values
- Use pre-sanitized branch throughout script
- Addresses high-severity security issue from PR review

* fix: Add null checks and logging to workflow condition

- Add explicit null checks for head_repository and repository
- Add debug logging to help diagnose workflow trigger issues
- Prevents potential failures from undefined repository objects
- Addresses medium-severity issue from PR review

* fix: Pin wrangler to major version 4 with error handling

- Pin wrangler to major version 4 (^4.0.0) for stability
- Add error handling if wrangler installation fails
- Return 'failed' status if installation fails
- Addresses dependency management issue from PR review

* perf: Implement parallel deployments to reduce CI time

- Deploy all browser reports in parallel using background processes
- Use temporary directory to collect deployment results
- Wait for all deployments to complete before generating comment
- Maintains result order for consistent output
- Significantly reduces deployment time from sequential to parallel execution

* fix: Use specific comment ID for updates instead of edit-last

- Use GitHub API to find exact comment ID
- Update specific comment by ID to avoid editing wrong comment
- Prevents race conditions if user posts between finding and editing
- More reliable comment updates

* fix(workflows/test-ui.yaml): change condition to always run deploy job for pull requests to ensure deployment consistency

* fix(workflows/test-ui.yaml): change condition to always run deploy job for pull requests to ensure deployment consistency

* fix(pr-playwright-deploy-and-comment.sh): remove npx prefix from wrangler command for consistency and simplicity

* fix(pr-playwright-deploy-and-comment.sh): remove npx prefix from wrangler command for consistency and simplicity

* Update scripts/cicd/pr-playwright-deploy-and-comment.sh

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix(pr-playwright-deploy-and-comment.sh): improve regex for URL extraction to include valid characters and ensure correct URL format

* chore(pr-playwright-deploy-and-comment.sh): move wrangler installation to the beginning of the script to avoid redundancy and improve efficiency

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CI/CD size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants