Skip to content

Conversation

christian-byrne
Copy link
Contributor

@christian-byrne christian-byrne commented Sep 19, 2025

Enables manual backport triggering for scenarios where labels are added after PR merge.

Adds workflow_dispatch trigger to the backport workflow with support for:

  • Specifying PR number to backport post-merge
  • Force rerun option to override duplicate detection
  • Proper handling of multi-version backport scenarios

Solves the issue where adding version labels (e.g., 1.27) after a PR is already merged and backported (e.g., to 1.26) would not trigger additional backports.

┆Issue is synchronized with this Notion page by Unito

- Add workflow_dispatch trigger for post-merge backports
- Support force_rerun option to override duplicate detection
- Handle PR data fetching for manual triggers
- Enables backporting when labels are added after PR merge

Fixes: Labels added after merge (1.26 → add 1.27 later)
Usage: gh workflow run backport.yaml -f pr_number=5637
@christian-byrne christian-byrne requested a review from a team as a code owner September 19, 2025 03:04
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 19, 2025
Copy link

github-actions bot commented Sep 19, 2025

🎭 Playwright Test Results

All tests passed!

⏰ Completed at: 09/19/2025, 03:54:13 AM UTC

📈 Summary

  • Total Tests: 450
  • Passed: 421 ✅
  • Failed: 0
  • Flaky: 0
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 414 / ❌ 0 / ⚠️ 0 / ⏭️ 29
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

@christian-byrne christian-byrne added area:CI/CD claude-review Add to trigger a PR code review from Claude Code labels Sep 19, 2025
if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then
# For manual triggers, get labels from the PR
LABELS=$(gh pr view ${{ inputs.pr_number }} --json labels | jq -r '.labels[].name')
Copy link

Choose a reason for hiding this comment

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

[quality] medium Priority

Issue: Missing GH_TOKEN environment variable in step that calls GitHub API
Context: Line 78 uses gh pr view but the step doesn't declare GH_TOKEN env var, potentially causing authentication errors
Suggestion: Add env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} to this step

pr_number:
description: 'PR number to backport'
required: true
type: string
Copy link

Choose a reason for hiding this comment

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

[security] medium Priority

Issue: No input validation for pr_number parameter
Context: Workflow accepts pr_number as string input but doesn't validate it's a valid PR number, could cause unexpected behavior or command injection
Suggestion: Add validation to ensure pr_number is numeric and corresponds to an existing PR before using it

- name: Create PR for each successful backport
if: steps.check-existing.outputs.skip != 'true' && steps.backport.outputs.success
env:
GH_TOKEN: ${{ secrets.PR_GH_TOKEN }}
Copy link

Choose a reason for hiding this comment

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

[quality] medium Priority

Issue: Inconsistent GitHub token usage between steps
Context: This step uses secrets.PR_GH_TOKEN while others use secrets.GITHUB_TOKEN. Different tokens may have different permissions
Suggestion: Document why PR_GH_TOKEN is needed here or standardize on one token for consistency

jobs:
backport:
if: github.event.pull_request.merged == true && contains(github.event.pull_request.labels.*.name, 'needs-backport')
if: >
Copy link

Choose a reason for hiding this comment

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

[quality] low Priority

Issue: Complex multiline conditional could be hard to maintain
Context: The if condition spans multiple lines with complex boolean logic, making it harder to read and debug
Suggestion: Consider extracting to separate steps or using environment variables to make the logic clearer

// Watch for completion and close dialog
watch(allMissingNodesInstalled, async (allInstalled) => {
if (allInstalled && showInstallAllButton.value) {
if (allInstalled) {
Copy link

Choose a reason for hiding this comment

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

[quality] medium Priority

Issue: Behavioral change without clear explanation or test coverage
Context: Removing the showInstallAllButton.value condition changes when the dialog auto-closes. This could affect UX
Suggestion: Add comment explaining why this change was needed, and add test to verify new behavior works correctly

entryLocale: 'en',
output: 'src/locales',
outputLocales: ['zh', 'zh-TW', 'ru', 'ja', 'ko', 'fr', 'es', 'ar', 'tr'],
outputLocales: ['zh', 'zh-TW', 'ru', 'ja', 'ko', 'fr', 'es', 'ar'],
Copy link

Choose a reason for hiding this comment

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

[architecture] high Priority

Issue: Breaking change removes Turkish language support without deprecation notice
Context: Removing 'tr' from outputLocales and i18n configuration is a breaking change for Turkish users
Suggestion: Consider deprecation strategy or migration path. Document why Turkish support was removed in PR description

const connType = fromSlot.type

const colour = resolveConnectingLinkColor(connType)
const colour =
Copy link

Choose a reason for hiding this comment

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

[quality] low Priority

Issue: Replaced utility function with inline logic, potential duplication
Context: The deleted resolveConnectingLinkColor function is now inlined. If this logic is used elsewhere, it creates duplication
Suggestion: Check if this color resolution logic is needed elsewhere in the codebase to avoid duplication

// the connection being dragged by the mouse
if (this.linkRenderer) {
this.linkRenderer.renderDraggingLink(
this.linkRenderer.renderLinkDirect(
Copy link

Choose a reason for hiding this comment

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

[architecture] medium Priority

Issue: Changed linkRenderer API call from renderDraggingLink to renderLinkDirect with different parameters
Context: This changes the rendering API which could break extensions or custom renderers that depend on renderDraggingLink
Suggestion: Verify this doesn't break existing extensions. Consider keeping backward compatibility or documenting the breaking change

import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore'
import { useWorkflowAutoSave } from '@/platform/workflow/persistence/composables/useWorkflowAutoSave'
import { useWorkflowPersistence } from '@/platform/workflow/persistence/composables/useWorkflowPersistence'
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
Copy link

Choose a reason for hiding this comment

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

[architecture] medium Priority

Issue: Removed attachSlotLinkPreviewRenderer call without explanation
Context: This appears to remove preview functionality for slot links, which could affect user experience
Suggestion: Document why this was removed and ensure the functionality is replaced elsewhere or no longer needed

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Comprehensive PR Review

This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.

Review Summary

PR: feat: add manual dispatch to backport workflow (#5651)
Impact: 67 additions, 13 deletions across 26 files

Issue Distribution

  • Critical: 0
  • High: 1
  • Medium: 6
  • Low: 2

Category Breakdown

  • Architecture: 3 issues
  • Security: 1 issues
  • Performance: 0 issues
  • Code Quality: 5 issues

Key Findings

Architecture & Design

The PR introduces manual workflow dispatch capability to the backport system, which is a good architectural enhancement. However, there are some concerns:

  • Breaking change removing Turkish language support without deprecation strategy
  • API changes to linkRenderer that could affect extension compatibility
  • Removed functionality (attachSlotLinkPreviewRenderer) without clear documentation

Security Considerations

  • Input validation is missing for the pr_number parameter, which could potentially be exploited
  • Inconsistent use of GitHub tokens between workflow steps could cause permission issues

Performance Impact

No significant performance impacts were identified in this review.

Integration Points

The changes affect multiple integration points:

  • GitHub workflow automation system
  • Internationalization system (Turkish removal)
  • Link rendering system in the canvas
  • Dialog management system

Positive Observations

  • Well-structured workflow with proper error handling and cleanup
  • Good use of GitHub Actions conditional logic
  • Comprehensive handling of both automatic and manual trigger scenarios
  • Clear separation of concerns between workflow steps

References

Next Steps

  1. Address security issue with input validation (medium priority)
  2. Consider architectural feedback for breaking changes
  3. Document reasons for removed functionality
  4. Verify extension compatibility with API changes

This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.

- Validate PR number format (must be positive integer)
- Verify PR exists and is accessible
- Ensure PR is merged before allowing backport
- Check for required needs-backport label

Addresses security concern from automated review
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 19, 2025
@christian-byrne
Copy link
Contributor Author

The automated Claude review appears to be faulty - it references changes that are not in this PR (Turkish language removal, linkRenderer changes, etc.).

This PR only modifies to add manual dispatch capability.

However, I've addressed the one valid concern about input validation by adding:

  • ✅ PR number format validation
  • ✅ PR existence and accessibility checks
  • ✅ Merged status verification
  • ✅ Required label validation

The workflow now properly validates all inputs before proceeding with backport operations.

@christian-byrne christian-byrne merged commit 726a2fb into main Sep 19, 2025
25 checks passed
@christian-byrne christian-byrne deleted the feat/add-manual-dispatch-workflows branch September 19, 2025 04:01
christian-byrne pushed a commit that referenced this pull request Sep 19, 2025
…rison (#5654)

## Summary
- Fixes the Claude automated PR review comparing against wrong commits
- Updates the comprehensive-pr-review.md command to use `$BASE_SHA`
instead of `origin/$BASE_BRANCH`
- Resolves issue where Claude was reviewing unrelated changes from other
PRs

## Problem
As identified in #5651 (comment
#5651 (comment)),
the Claude automated review was incorrectly analyzing changes that
weren't part of the PR being reviewed. The review was mentioning Turkish
language removal, linkRenderer changes, and other modifications that
weren't in the actual PR diff.

## Root Cause Analysis

### The Issue Explained (from Discord discussion)
When Christian Byrne noticed Claude was referencing things from previous
reviews on other PRs, we investigated and found:

1. **The backport branch was created from origin/main BEFORE Turkish
language support was merged**
   - Branch state: `main.A`
   - Backport changes committed: `main.A.Backport`

2. **Turkish language support was then merged into origin/main**
   - Main branch updated to: `main.A.Turkish`

3. **Claude review workflow checked out `main.A.Backport` and ran git
diff against `origin/main`**
   - This compared: `main.A.Backport <> main.A.Turkish`
   - The diff showed: `+++Backport` changes and `---Turkish` removal
   - Because the common parent of both branches was `main.A`

### Why This Happens
When using `origin/$BASE_BRANCH`, git resolves to the latest commit on
that branch. The diff includes:
1. The PR's actual changes (+++Backport)
2. The reverse of all commits merged to main since the PR was created
(---Turkish)

This causes Claude to review changes that appear as "removals" of code
from other merged PRs, leading to confusing comments about unrelated
code.

## Solution
Changed the git diff commands to use `$BASE_SHA` directly, which GitHub
Actions provides as the exact commit SHA that represents the merge base.
This ensures Claude only reviews the actual changes introduced by the
PR.

### Before (incorrect):
```bash
git diff --name-only "origin/$BASE_BRANCH"  # Compares against latest main
git diff "origin/$BASE_BRANCH"
git diff --name-status "origin/$BASE_BRANCH"
```

### After (correct):
```bash
git diff --name-only "$BASE_SHA"  # Compares against merge base
git diff "$BASE_SHA"
git diff --name-status "$BASE_SHA"
```

## Technical Details

### GitHub Actions Environment Variables
- `BASE_SHA`: The commit SHA of the merge base (where PR branched from
main)
- `BASE_BRANCH`: Not provided by GitHub Actions (this was the bug)
- Using `origin/$BASE_BRANCH` was falling back to comparing against the
latest main commit

### Alternative Approaches Considered
1. **Approach 1**: Rebase/update branch before running Claude review
   - Downside: Changes the PR's commits, not always desirable
2. **Approach 2**: Use BASE_SHA to diff against the merge base ✅
   - This is what GitHub's PR diff view does
   - Shows only the changes introduced by the PR

## Testing
The BASE_SHA environment variable is already correctly set in the
claude-pr-review.yml workflow (line 88), so this change will work
immediately once merged.

## Impact
- Claude reviews will now be accurate and only analyze the actual PR
changes
- No false positives about "removed" code from other PRs
- More reliable automated PR review process
- Developers won't be confused by comments about code they didn't change

## Verification
You can verify this fix by:
1. Creating a PR from an older branch
2. Merging another PR to main
3. Triggering Claude review with the label
4. Claude should only review the PR's changes, not show removals from
the newly merged commits

## Credits
Thanks to @christian-byrne for reporting the issue and @snomiao for the
root cause analysis.

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

Co-authored-by: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CI/CD claude-review Add to trigger a PR code review from Claude Code size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant