[PM-27465] Fixing cancel button on Send and Vault export #17138
Conversation
|
Claude finished @itsadrago's task —— View job PR Review: Cancel Button FixesSummary of ChangesThis PR fixes non-functional cancel buttons in two browser extension dialogs by changing the
The issue was introduced when the ✅ Code QualityThe fix is correct and minimal - it properly converts the directive usage to match Angular's signal-based model input requirements. 📋 Review FindingsCritical IssuesNone found. Suggestions for Improvement💭 Consider adding template tests to catch similar regressionsThese templates lack unit tests that could have caught this issue earlier. While not blocking for this PR, consider adding tests that verify:
This would prevent similar issues when directives are refactored in the future. Files lacking test coverage:
🌱 Migration gap: Other templates may have similar issuesThe signal migration in PR #15738 (commit 827c4c0) changed Recommendation: Search for other directives that were migrated to signal models with aliases in that same PR to verify all their usages were updated. This could be a systematic issue. # Check if other signal model directives might have similar issues
git diff 827c4c0301^..827c4c0301 -- '*.directive.ts' | grep -A5 "model.*alias"👍 Good Practices Observed
Action ItemsFor this PR:
For future consideration:
Technical ContextWhy this fix works: In Angular, when a directive uses readonly handler = model<FunctionReturningAwaitable>(
() => this.routerCacheService.back(),
{ alias: "popupBackAction" }
);The template must use property binding syntax Reference:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17138 +/- ##
==========================================
- Coverage 40.41% 40.40% -0.01%
==========================================
Files 3504 3504
Lines 100148 100148
Branches 15022 15022
==========================================
- Hits 40473 40467 -6
- Misses 57957 57964 +7
+ Partials 1718 1717 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| {{ "save" | i18n }} | ||
| </button> | ||
| <button bitButton type="button" buttonType="secondary" popupBackAction> | ||
| <button bitButton type="button" buttonType="secondary" [popupBackAction]> |
There was a problem hiding this comment.
@bitwarden/team-ui-foundation Should the selector for this be tweaked to allow missed []?
There was a problem hiding this comment.
I think these are the only two instances of this directive being used, so not sure if it is worth it at this point?
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27465
📔 Objective
During the creation of a new Text Send on the browser extension, you have the option to
SaveorCancel. Hitting theCancelbutton currently has no effect. It should close the "New Text Send" dialog window, similar to if you hit the back arrow at the top left of the dialog window.During the creation of a new Export Vault operation on the browser extension, you have the option to
Export vaultorCancel. Hitting theCancelbutton currently has no effect. It should close the "Export vault" dialog window, similar to if you hit the back arrow at the top left of the dialog window.📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes