-
Notifications
You must be signed in to change notification settings - Fork 5k
FEAT: support bulk add/remove labels for selected artifacts; keep “Add Labels” enabled on multi-select #22553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…abels enabled on multi-select, Fixes goharbor#19422 Signed-off-by: falconlee236 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances label management functionality to support bulk operations when multiple artifacts are selected. Previously, labels could only be added/removed for a single artifact at a time, and label selection was limited when multiple items were selected.
- Introduces
selectedCommonLabelsgetter to compute labels shared across all selected artifacts - Refactors
addLabel()andremoveLabel()to process multiple artifacts usingforkJoin - Simplifies
canAddLabel()logic to allow label operations on multiple selections
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| artifact-list-tab.component.ts | Adds common label computation logic, refactors label add/remove methods to support bulk operations via forkJoin, and simplifies canAddLabel check |
| artifact-list-tab.component.html | Updates label selector to use computed common labels instead of first selected item's labels |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| forkJoin(ops) | ||
| .pipe(finalize(() => (this.inprogress = false))) | ||
| .subscribe({ | ||
| next: () => this.refresh(), | ||
| error: err => { | ||
| this.errorHandlerService.error(err); | ||
| this.refresh(); | ||
| }, | ||
| }); |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When forkJoin receives an empty array, it completes immediately with an empty array. However, the guard at line 547 prevents this. The real issue is that forkJoin will fail if ANY of the operations fail, meaning only the first error will be reported and refresh() will be called once. If multiple operations fail, users won't see all errors. Consider using forkJoin with individual error handling via catchError on each observable, or switch to a pattern that reports all failures.
| forkJoin(ops) | ||
| .pipe(finalize(() => (this.inprogress = false))) | ||
| .subscribe({ | ||
| next: () => this.refresh(), | ||
| error: err => { | ||
| this.errorHandlerService.error(err); | ||
| this.refresh(); | ||
| }, | ||
| }); |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as in addLabel: forkJoin will fail fast on the first error, meaning if multiple remove operations fail, only the first error will be reported to users. Consider using forkJoin with individual error handling via catchError on each observable to report all failures, or use a different pattern that doesn't fail fast.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #22553 +/- ##
===========================================
+ Coverage 45.36% 68.55% +23.18%
===========================================
Files 244 821 +577
Lines 13333 101822 +88489
Branches 2719 0 -2719
===========================================
+ Hits 6049 69799 +63750
- Misses 6983 28140 +21157
- Partials 301 3883 +3582
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Hi @bupd , Could you please help to review this PR^ Thanks. |
Signed-off-by: falconlee236 <[email protected]>
|
Hi @MinerYang, |
What this PR does
Why
How (high level)
Issue being fixed
Fixes #19422
Please indicate you've done the following: