ci: fix backport workflow not cleaning up branch on failure and not able to update existing PRs/branches on re-run #6620
Conversation
dosubot
bot
added
the
size:M
🎭 Playwright Test Results⏰ Completed at: 11/06/2025, 07:44:05 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/06/2025, 07:07:49 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
christian-byrne
changed the title
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.25 MB (baseline 3.25 MB) • ⚪ 0 BMain entry bundles and manifests
Graph Workspace — 792 kB (baseline 792 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Views & Navigation — 8.18 kB (baseline 8.18 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Panels & Settings — 293 kB (baseline 293 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
UI Components — 12.6 kB (baseline 12.6 kB) • ⚪ 0 BReusable component library chunks
Data & Services — 10.4 kB (baseline 10.4 kB) • ⚪ 0 BStores, services, APIs, and repositories
Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 2.55 MB (baseline 2.55 MB) • ⚪ 0 BBundles that do not match a named category
|
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| PENDING=() | ||
| SKIPPED=() | ||
| REUSED=() |
There was a problem hiding this comment.
[quality] low Priority
Issue: The REUSED array is initialized but variable naming could be clearer
Context: Array tracks backport branches being reused but the name doesn't indicate these are branch names
Suggestion: Consider renaming to REUSED_BRANCHES for clarity
| window['__COMFYUI_FRONTEND_VERSION__'] = config.app_version | ||
| if (isElectron()) { | ||
| document.addEventListener('contextmenu', showContextMenu) |
There was a problem hiding this comment.
[architecture] high Priority
Issue: Complete removal of Vite preload error handling without explanation
Context: This removes the entire error handling mechanism for deployment asset issues, which could lead to broken app states after updates
Suggestion: Either provide a replacement error handling mechanism or explain why this critical error handling is no longer needed
| import { useConflictDetection } from '@/workbench/extensions/manager/composables/useConflictDetection' | ||
| import { electronAPI, isElectron } from './utils/envUtil' | ||
There was a problem hiding this comment.
[quality] low Priority
Issue: Multiple unused imports removed
Context: Removing unused imports is good for bundle size and code cleanliness
Suggestion: Good cleanup! This improves bundle optimization
| "noImportedFiles": "No imported files found", | ||
| "noGeneratedFiles": "No generated files found", | ||
| "noFilesFoundMessage": "Upload files or generate content to see them here", | ||
| "templates": "Templates" |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Multiple locale keys removed without clear context
Context: Removing locale keys for Vite preload errors is consistent with App.vue changes, but other removals (3D viewer, assets, etc.) seem unrelated to the PR scope
Suggestion: Either provide context for why these UI features are being removed or split into separate PR
| isSelected(option) && !disabled, | ||
| 'hover:bg-interface-menu-component-surface-hovered': | ||
| !isSelected(option) && !disabled, | ||
| 'bg-white': isSelected(option) && !disabled, |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Hardcoded color classes replacing semantic theme classes
Context: Changes from semantic classes like 'bg-interface-menu-component-surface-selected' to hardcoded 'bg-white' breaks theme consistency
Suggestion: Use semantic theme classes from the CSS design system instead of hardcoded colors for better theme support
| <i | ||
| v-if="!isPlaying" | ||
| class="text-secondary icon-[lucide--play] size-4" | ||
| class="icon-[lucide--play] size-4 text-smoke-600 dark-theme:text-smoke-800" |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Introduction of non-standard color classes and hardcoded theme-specific styles
Context: Using classes like 'text-smoke-600' and 'dark-theme:text-smoke-800' instead of semantic theme variables breaks the established design system
Suggestion: Use semantic theme classes from style.css instead of custom color schemes that bypass the theming system
| 'bg-interface-menu-component-surface-selected' | ||
| ) | ||
| expect(unselectedButton.classes()).toContain('text-secondary') | ||
| expect(selectedButton.classes()).toContain('bg-white') |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Tests updated to match hardcoded color implementation
Context: While tests are correctly updated to match the new hardcoded colors, this locks in the architectural violation
Suggestion: Consider testing against semantic class names or data attributes instead of specific color classes for better maintainability
| while IFS= read -r branch; do | ||
| [ -z "$branch" ] && continue | ||
| printf 'Deleting branch %s\n' "${branch}" | ||
| if ! git push origin --delete "$branch"; then |
There was a problem hiding this comment.
[security] low Priority
Issue: Branch name passed to git push --delete without shell escaping
Context: While unlikely to be exploited in this context, branch names could theoretically contain shell metacharacters
Suggestion: Consider using -- to separate options from branch names: git push origin --delete -- "$branch"
There was a problem hiding this comment.
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: ci: fix backport workflow not cleaning up branch on failure and not able to update existing PRs/branches on re-run (#6620)
Impact: 66 additions, 7 deletions across 9 files
Issue Distribution
- Critical: 0
- High: 1
- Medium: 4
- Low: 4
Category Breakdown
- Architecture: 3 issues
- Security: 1 issues
- Performance: 0 issues
- Code Quality: 2 issues
Key Findings
Architecture & Design
The main concern is the systematic replacement of semantic theme classes with hardcoded colors throughout Vue components. This breaks the established design system that uses semantic CSS variables for theming. Components are moving from classes like 'bg-interface-menu-component-surface-selected' to hardcoded 'bg-white' and custom color schemes like 'text-smoke-600'.
Security Considerations
The workflow improvements include good security practices like using --force-with-lease for safer forced pushes. There is one minor shell safety concern around branch name handling in the cleanup section.
Performance Impact
The removal of unused imports in App.vue is positive for bundle optimization. No significant performance concerns identified.
Integration Points
The removal of Vite preload error handling could impact user experience during deployments when assets change. The locale key removals suggest features are being removed without clear justification in this PR scope.
Positive Observations
- Good use of --force-with-lease for safer git operations
- Proper cleanup mechanism for stranded branches
- Consistent test updates to match implementation changes
- Unused import cleanup improves bundle size
References
Next Steps
- Address the removal of critical error handling (Vite preload errors)
- Consider reverting to semantic theme classes for better theming support
- Provide justification for removed locale keys and features
- Consider splitting this PR into focused changes (CI workflow vs. UI changes)
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
Fixes issue in which a failed backport runs would not cleanup the branch (issue 1) and then on the next backport attempt, it would bail out early because it checks if a branch with that name already exists (issue 2).
The workflow now treats existing backport branches as reusable unless an open PR already references them (issue 2 solution), force-updates any reused branch with the latest cherry-pick, and records them so a new cleanup step can delete the branch if the run fails (issue 1 solution). That prevents stranded refs from blocking future backport runs while keeping active backport PRs intact.
┆Issue is synchronized with this Notion page by Unito