Skip to content

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Oct 3, 2025

Summary by CodeRabbit

  • Chores
    • PR workflow now runs on pull requests targeting the master branch, in addition to existing maintenance branches.
    • Expanded CI coverage provides consistent build and test checks across primary branches.
    • End users may see more uniform build statuses and earlier detection of issues on master-bound changes.
    • No user-facing functionality changes.

@haslinghuis haslinghuis added this to the 2025.12 milestone Oct 3, 2025
@haslinghuis haslinghuis self-assigned this Oct 3, 2025
Copy link
Contributor

coderabbitai bot commented Oct 3, 2025

Walkthrough

The PR updates the GitHub Actions PR workflow configuration to include the master branch in the pull_request trigger alongside existing branches (e.g., *-maintenance). No code or API surface changes.

Changes

Cohort / File(s) Change Summary
CI Workflow Triggers
.github/workflows/pr.yml
Added master to the branches list for the pull_request trigger in the workflow.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The changes in this pull request update the GitHub Actions workflow triggers by adding the master branch, but the title “Fix missing preview URL in pull requests” refers to an unrelated issue, making it misleading and not representative of the actual modifications. Please update the title to accurately describe the main change, for example “Add master branch to GitHub Actions PR trigger list.”
Description Check ⚠️ Warning The pull request description is entirely missing and does not include any of the required sections from the repository’s PR template, such as a summary of changes, motivation, or testing instructions. Please provide a description following the template, including an overview of the change, why it’s needed, and any steps to test or verify the update.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a87c9ca and 6b4c656.

📒 Files selected for processing (1)
  • .github/workflows/pr.yml (1 hunks)
🔇 Additional comments (1)
.github/workflows/pr.yml (1)

6-7: Coverage fix looks good.

Including master ensures the PR workflow fires for default-branch targets, which should restore the preview URL publishing.


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.

@haslinghuis haslinghuis moved this to App in 2025.12.0 Oct 3, 2025
@haslinghuis
Copy link
Member Author

@coderabbitai check for breaking changes made in #4639

Copy link
Contributor

coderabbitai bot commented Oct 3, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch details about PR #4639
echo "=== PR #4639 Details ==="
gh pr view 4639 --json title,body,author,state,files,additions,deletions,changedFiles

echo -e "\n=== Files Changed in PR #4639 ==="
gh pr diff 4639 --patch

Length of output: 7548


🌐 Web query:

GitHub Actions checkout v5 breaking changes from v4

💡 Result:

Short summary of breaking changes from actions/checkout v4 → v5

  • The action now runs on Node.js 24 (node24 runtime). This is the main change. [1]
  • Minimum GitHub Actions Runner required: v2.327.1 (update self-hosted runners before upgrading). [1][2]
  • No other API/inputs removed in the release notes — usage and inputs remain the same as v4, so workflows should mostly be a drop-in upgrade once runner compatibility is satisfied. [1][2]

Sources:
[1] actions/checkout README (v5 notes).
[2] actions/checkout v5 release / changelog.


🌐 Web query:

GitHub Actions setup-node v5 breaking changes from v4

💡 Result:

Key breaking changes in actions/setup-node v5 vs v4

  • Automatic package-manager caching is enabled by default when a valid packageManager field is present in package.json. You can disable it with the new input package-manager-cache: false. [1][2]
  • The action runtime was upgraded to Node 24; runners must be at least GitHub Actions runner v2.327.1 for compatibility. [1][2]

Quick migration actions

  • If you don’t want automatic caching, add: package-manager-cache: false to your setup-node step. [1]
  • Ensure your runner is updated to v2.327.1+ (or use GitHub-hosted runners) before switching to setup-node@v5. [1][2]

Sources: GitHub actions/setup-node release notes and README. [1] [2]


🌐 Web query:

GitHub Actions setup-java v5 breaking changes from v4

💡 Result:

Short answer — main breaking change in v5:

  • The action runtime was upgraded from Node.js 20 to Node.js 24. You must run on GitHub Actions runner v2.327.1 or newer for compatibility. [1][2]

Sources:

  • actions/setup-java release v5.0.0. [1]
  • actions/setup-java README (Breaking changes / upgrade note). [2]

🌐 Web query:

GitHub Actions download-artifact v5 breaking changes from v4

💡 Result:

Short summary of breaking changes from actions/download-artifact v4 → v5

  • Single-artifact extraction by ID changed: artifacts downloaded with artifact-ids used to be extracted into path// (nested); in v5 they are extracted directly into path/ (same as downloads by name). If you relied on the nested folder, update workflows to explicitly include the artifact name in path. [1][2]

  • No other behavioural incompatibilities introduced in v5 are listed; the other notable breaking changes were introduced in v4 (v4 is not cross-compatible with v3, GHES support notes, firewall/self-hosted runner requirements, and v4 immutability/merge-multiple behavior). Those v4 breaking changes remain relevant when moving to v4+ but are not new in v5. [2][3]

Migration tip: if you previously used artifact-ids and expected path//, change:

  • from: path: dist
  • to: path: dist/my-artifact
    (or switch to using name: my-artifact) to preserve the nested layout. [1]

Sources: GitHub release/migration notes and docs. [1][2][3]


🌐 Web query:

Crowdin GitHub Action v2 breaking changes from v1

💡 Result:

Key breaking changes when upgrading crowdin/github-action from v1 → v2:

  • Action now uses Crowdin CLI v4 (behaviour/commands follow CLI v4). [1]
  • Removed deprecated action inputs — these were removed in v2 and should be replaced with equivalent CLI commands or env vars:
    • add_crowdin_branch
    • new_branch_title
    • new_branch_export_pattern
    • new_branch_priority
    • delete_crowdin_branch
    • identity
      Use commands like command: branch add <name> instead. [1][2]

Sources:

  • Crowdin Action v2 release notes / changelog. [1]
  • GitHub repo v2 release / compare (v1.20.4...v2.0.0). [2]

🏁 Script executed:

#!/bin/bash
# Check if any workflow uses artifact-ids parameter
echo "=== Checking for artifact-ids usage ==="
rg -n "artifact-ids" .github/workflows/

# Check if package.json has packageManager field
echo -e "\n=== Checking package.json for packageManager field ==="
if [ -f package.json ]; then
  jq -r '.packageManager // "not present"' package.json
else
  echo "package.json not found"
fi

# Check if any workflows use the removed Crowdin inputs
echo -e "\n=== Checking for removed Crowdin action inputs ==="
rg -n "add_crowdin_branch|new_branch_title|new_branch_export_pattern|new_branch_priority|delete_crowdin_branch|identity:" .github/workflows/translations*.yml

Length of output: 159


@haslinghuis, I've analyzed PR #4639 for breaking changes. Here's what I found:

Summary of PR #4639

The PR updates GitHub Actions to their latest major versions:

  • actions/checkout: v4 → v5
  • actions/setup-node: v4 → v5
  • actions/setup-java: v4 → v5
  • actions/download-artifact: v4 → v5
  • crowdin/github-action: v1 → v2

Potential Breaking Changes (General)

  1. All actions now use Node.js 24 runtime

    • Requires GitHub Actions runner v2.327.1+
    • Not an issue: GitHub-hosted runners already meet this requirement
  2. setup-node v5: Automatic package-manager caching

    • Enables automatic caching when packageManager field exists in package.json
    • Not an issue: Your package.json doesn't have a packageManager field
  3. download-artifact v5: Single-artifact extraction behavior

    • Changed how artifacts downloaded by ID are extracted (no longer nested)
    • Not an issue: Your workflows don't use the artifact-ids parameter
  4. crowdin/github-action v2: CLI v4 and removed deprecated inputs

    • Removed: add_crowdin_branch, new_branch_title, identity, etc.
    • Not an issue: Your workflows don't use any of the removed inputs

Conclusion

No breaking changes affect this project. The updates in PR #4639 are safe and only modernize the infrastructure to Node.js 24. All potentially breaking changes either don't apply to your workflow configurations or are handled automatically by GitHub-hosted runners.


🧠 Learnings used
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4532
File: src/js/VirtualFC.js:234-234
Timestamp: 2025-06-27T22:06:49.210Z
Learning: In the betaflight-configurator codebase, the VirtualFC.js AUX_CONFIG array must stay synchronized with the betaflight firmware's msp_box.c definitions to ensure proper auxiliary mode functionality. Changes to mode names should follow the firmware source code even if it potentially breaks backward compatibility with saved presets.

Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4532
File: src/js/VirtualFC.js:234-234
Timestamp: 2025-06-27T22:06:49.210Z
Learning: In the betaflight-configurator codebase, the VirtualFC.js AUX_CONFIG array must stay synchronized with the betaflight firmware's msp_box.c definitions to ensure proper auxiliary mode functionality. Changes to mode names should follow the firmware source code even if it potentially breaks backward compatibility with saved presets.

Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.

@haslinghuis
Copy link
Member Author

@coderabbitai the issue seems to be here: https://github.com/betaflight/betaflight-configurator/blob/master/.github/workflows/deploy.yml#L94-L95

Copy link
Contributor

coderabbitai bot commented Oct 3, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the deploy.yml file and examine lines 94-95 with context
echo "=== deploy.yml lines 90-100 (context around 94-95) ==="
sed -n '90,100p' .github/workflows/deploy.yml

echo -e "\n=== Full section containing lines 94-95 ==="
sed -n '80,110p' .github/workflows/deploy.yml

# Also check the pr.yml file to compare preview URL generation
echo -e "\n=== Checking pr.yml for preview URL logic ==="
cat .github/workflows/pr.yml

Length of output: 1377


🏁 Script executed:

#!/bin/bash
# Check the triggers for deploy.yml workflow
echo "=== deploy.yml workflow triggers ==="
sed -n '1,30p' .github/workflows/deploy.yml

# Check if there are any other workflows that might be related
echo -e "\n=== List all workflow files ==="
ls -la .github/workflows/

# Check if ci.yml has any deployment steps
echo -e "\n=== Checking ci.yml for deployment or preview logic ==="
cat .github/workflows/ci.yml

Length of output: 4850


@haslinghuis, you're absolutely right! I found the issue.

The Problem

In .github/workflows/deploy.yml:

  • Line 3: The workflow triggers on pull_request_target
  • Line 90: The condition checks if: github.event_name == 'pull_request'

This is a mismatch! When the workflow is triggered by pull_request_target, the github.event_name will be 'pull_request_target', not 'pull_request'. Therefore, the condition at line 90 will always be false, and the preview URL comment will never be posted.

The Fix

The condition at line 90 in deploy.yml should be changed to:

if: github.event_name == 'pull_request_target'

About the Current PR

The current PR (#4649) modifies .github/workflows/pr.yml to add the master branch to PR triggers, but this won't fix the preview URL issue since:

  1. The pr.yml workflow only runs CI tests (via ci.yml)
  2. The deployment and preview URL posting happens in deploy.yml
  3. The actual bug is the event name check in deploy.yml

Would you like me to help identify the correct fix for deploy.yml?

Copy link

sonarqubecloud bot commented Oct 3, 2025

Copy link
Contributor

github-actions bot commented Oct 3, 2025

Preview URL: https://pr4649.betaflight-app.pages.dev

@haslinghuis
Copy link
Member Author

@haslinghuis haslinghuis closed this Oct 3, 2025
@github-project-automation github-project-automation bot moved this from App to Done in 2025.12.0 Oct 3, 2025
@blckmn
Copy link
Member

blckmn commented Oct 3, 2025

Closing as this was pre-mature (I was working on the update to the production deployment for the upcoming release).

@haslinghuis please be patient.

@haslinghuis haslinghuis deleted the fix-missing-preview branch October 3, 2025 00:57
@haslinghuis
Copy link
Member Author

@blckmn was not aware and thought it was related to my last PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants