[PM-26513] Desktop Archive Upgrade #16964
Conversation
|
Claude finished @nick-livefront's task —— View job Code Review CompleteOverall Assessment: This PR successfully implements premium badge functionality for the archive filter. The implementation is solid with good test coverage. I found one minor issue that should be addressed and one inquiry for consideration. Summary of FindingsFinding 1: Template reference variable Finding 2: Observable subscriptions in template could cause multiple re-subscriptions (💭 inquiry) See inline comments for details and recommendations. Positive Observations✓ Well-structured test coverage with all three scenarios tested PR Title & Description✓ Clear title with Jira reference Test CoverageCodecov reports 76.92% patch coverage. The new test file provides good coverage of the core logic in status-filter.component.ts with three well-structured test cases covering all code paths. |
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16964 +/- ##
==========================================
+ Coverage 41.20% 41.27% +0.07%
==========================================
Files 3543 3546 +3
Lines 101912 102061 +149
Branches 15282 15310 +28
==========================================
+ Hits 41991 42125 +134
- Misses 58155 58172 +17
+ Partials 1766 1764 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| </button> | ||
| </span> | ||
| @if (!(canArchive$ | async)) { | ||
| <app-premium-badge #badge></app-premium-badge> |
There was a problem hiding this comment.
#badge is unused. Consider removing it:
| <app-premium-badge #badge></app-premium-badge> | |
| <app-premium-badge></app-premium-badge> |
| private readonly premiumBadgeComponent = viewChild(PremiumBadgeComponent); | ||
|
|
||
| private userId$ = this.accountService.activeAccount$.pipe(getUserId); | ||
| protected canArchive$ = this.userId$.pipe( |
There was a problem hiding this comment.
💭 The canArchive$ observable is used with the async pipe in the template. While this follows the existing codebase pattern (ADR-0003), be aware that without OnPush change detection, this could create multiple subscriptions on re-renders.
This is acceptable for now given the FIXME comment about migrating to OnPush change detection, but worth keeping in mind for the future migration.
🎟️ Tracking
PM-26513
📔 Objective
Adds premium badge to the archive filter for non-premium users.
📸 Screenshots
archive-from-menu.mov
no-archive-items.mov
past-archive-items.mov
🦮 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