PM-24535 Web premium upgrade path for archive #16854
Conversation
| premiumOptions?: { | ||
| /** When true, the premium badge will show on the filter for non-premium users. */ | ||
| showPremiumBadge?: true; | ||
| /** Action to be called instead of applying the filter. */ | ||
| blockFilterAction?: () => Promise<void>; | ||
| }; |
There was a problem hiding this comment.
I tried to make this somewhat reusable if it were to ever apply to other filters, but I'm not supper happy with the configuration options here. I'm open to ideas!
|
Great job! No new security vulnerabilities introduced in this pull request |
nick-livefront
force-pushed
the
vault/pm-24535/web-premium-archive
branch
from
1c1f901 to
759126f
Compare
|
Claude finished @nick-livefront's task —— View job Code Review CompleteSummary of ChangesThis PR implements a premium upgrade path for the archive feature in the web client. It adds:
Critical Issues1. Type Safety Violation in Filter ConfigurationFile: options?: {
component: any; // ❌ Using 'any' type
};Issue: Using Recommendation: options?: {
component: Type<unknown>; // Import Type from @angular/core
};2. Conditional Logic InconsistencyFile: const premiumPromptOnFilter = !userHasPremium && !hasArchivedCiphers;Issue: The logic determines when to block the filter action, but the condition is confusing. If a user has no premium AND no archived ciphers, they're blocked. However, if they have archived ciphers but no premium, they're NOT blocked (they can view). This works correctly but could be clearer. Recommendation: Add a comment explaining the business logic: // Block archive filter if user lacks premium AND has no archived items
// Users who lost premium but have archived items can still view them
const premiumPromptOnFilter = !userHasPremium && !hasArchivedCiphers;3. Missing Error Handling in Archive ServiceFile: async archiveWithServer(ids: CipherId | CipherId[], userId: UserId): Promise<void> {
const request = new CipherBulkArchiveRequest(Array.isArray(ids) ? ids : [ids]);
const r = await this.apiService.send("PUT", "/ciphers/archive", request, true, true);
// No try-catch, errors propagate to callerIssue: No error handling for API failures. If the API call fails, the local cipher state won't be updated, leading to inconsistency. Recommendation: Add error handling or document that callers must handle errors: async archiveWithServer(ids: CipherId | CipherId[], userId: UserId): Promise<void> {
try {
const request = new CipherBulkArchiveRequest(Array.isArray(ids) ? ids : [ids]);
const r = await this.apiService.send("PUT", "/ciphers/archive", request, true, true);
const response = new ListResponse(r, CipherResponse);
const currentCiphers = await firstValueFrom(this.cipherService.ciphers$(userId));
// ... update logic
await this.cipherService.replace(currentCiphers, userId);
} catch (error) {
// Re-throw with context or handle appropriately
throw new Error(`Failed to archive ciphers: ${error.message}`);
}
}4. Premium Badge Template Reference Without Safety CheckFile: <button bitMenuItem (click)="badge.promptForPremium()" type="button">
<i class="bwi bwi-fw bwi-archive" aria-hidden="true"></i>
{{ "archiveVerb" | i18n }}
<div slot="end" class="-tw-mt-0.5" aria-hidden>
<app-premium-badge #badge></app-premium-badge>
</div>
</button>Issue: The template reference Recommendation: Add safe navigation or ensure the component is always rendered: <button bitMenuItem (click)="badge?.promptForPremium()" type="button">Or better, handle this in the component TypeScript: @ViewChild('badge') badgeComponent: PremiumBadgeComponent;
protected promptForPremium() {
this.badgeComponent?.promptForPremium();
}Suggested Improvements5. Configuration Design Concern (Author's Question)File: The author asked about the reusability of premiumOptions?: {
/** When true, the premium badge will show on the filter for non-premium users. */
showPremiumBadge?: true; // ❌ Type is literally `true`, not boolean
/** Action to be called instead of applying the filter. */
blockFilterAction?: () => Promise<void>;
};Issues:
Recommendation: Make it more generic and flexible: featureGate?: {
/** Badge to display when feature is gated (e.g., premium, enterprise) */
showBadge?: boolean;
/** Badge type/variant if needed */
badgeType?: 'premium' | 'enterprise' | 'custom';
/** Action to execute instead of the default filter action when feature is gated */
gatedAction?: () => Promise<void>;
/** Optional predicate to determine if feature should be gated */
shouldGate?: () => boolean | Observable<boolean>;
};This would support:
6. Observable Subscription ManagementFile: async onFilterSelect(filterNode: TreeNode<VaultFilterType>) {
if (this.section?.premiumOptions?.blockFilterAction) {
await this.section.premiumOptions.blockFilterAction();
return;
}
await this.section?.action(filterNode);
}Observation: Good implementation, but consider adding loading state to prevent double-clicks during async operations. Recommendation: protected isProcessing = false;
async onFilterSelect(filterNode: TreeNode<VaultFilterType>) {
if (this.isProcessing) return;
this.isProcessing = true;
try {
if (this.section?.premiumOptions?.blockFilterAction) {
await this.section.premiumOptions.blockFilterAction();
return;
}
await this.section?.action(filterNode);
} finally {
this.isProcessing = false;
}
}7. Duplicate Observable Pipe OperationsFile: const hasArchivedCiphers = await firstValueFrom(
this.cipherArchiveService
.archivedCiphers$(userId)
.pipe(map((archivedCiphers) => archivedCiphers.length > 0)),
);
const userHasPremium = await firstValueFrom(this.cipherArchiveService.userHasPremium$(userId));Issue: Both of these observables are re-evaluated every time Recommendation: Create a combined observable at the service level: archiveFilterConfig$(userId: UserId): Observable<{hasArchivedCiphers: boolean, hasPremium: boolean}> {
return combineLatest([
this.archivedCiphers$(userId).pipe(map(c => c.length > 0)),
this.userHasPremium$(userId)
]).pipe(
map(([hasArchived, hasPremium]) => ({ hasArchivedCiphers: hasArchived, hasPremium })),
shareReplay({ refCount: true, bufferSize: 1 })
);
}8. Accessibility Improvement NeededFile: <ng-container *ngIf="premiumFeature">
<app-premium-badge></app-premium-badge>
</ng-container>Issue: The premium badge appears in the filter UI without screen reader context. Recommendation: Add aria-label or accessible text: <ng-container *ngIf="premiumFeature">
<app-premium-badgepremiumFeatureRequired' | i18n }}"></app-premium-badge>
</ng-container>9. Inconsistent Naming ConventionsFile: Multiple locations The PR uses both "Archive" (noun) and "archive" (verb) but the distinction isn't always clear in variable names:
Recommendation: Use consistent naming:
10. Missing Test Coverage for Edge CasesFile: Good: Comprehensive tests exist for main flows Missing: Tests for edge cases:
Recommendation: Add edge case tests: it("should handle empty array gracefully", async () => {
await service.archiveWithServer([], userId);
expect(mockApiService.send).toHaveBeenCalledWith(
"PUT",
"/ciphers/archive",
expect.objectContaining({ ids: [] }),
true,
true
);
});
it("should skip ciphers not found locally", async () => {
const unknownCipherId = "unknown-id" as CipherId;
mockApiService.send.mockResolvedValue({
data: [{ id: unknownCipherId, archivedDate: "2024-01-15" }]
});
mockCipherService.ciphers$.mockReturnValue(of({}));
await service.archiveWithServer(unknownCipherId, userId);
// Should not throw, should complete successfully
expect(mockCipherService.replace).toHaveBeenCalled();
});11. Performance ConsiderationFile: if (await firstValueFrom(this.cipherArchiveService.showArchiveFeatures$())) {
builderFilter.archiveFilter = await this.addArchiveFilter(userId);
}Issue: This check happens on every filter rebuild. The feature flag likely doesn't change during a session, so this could be cached. Recommendation: Store the feature flag result once at initialization: private archiveFeatureEnabled$: Observable<boolean>;
async ngOnInit(): Promise<void> {
this.archiveFeatureEnabled$ = this.cipherArchiveService
.showArchiveFeatures$()
.pipe(shareReplay({ refCount: false, bufferSize: 1 }));
// ... rest of init
}
async buildAllFilters(): Promise<VaultFilterList> {
// ...
if (await firstValueFrom(this.archiveFeatureEnabled$)) {
builderFilter.archiveFilter = await this.addArchiveFilter(userId);
}
// ...
}12. Potential Memory Leak in Browser ComponentFile: protected userCanArchive = toSignal(
this.userId$.pipe(switchMap((userId) => this.cipherArchiveService.userCanArchive$(userId))),
);
protected showArchiveFilter = toSignal(
this.userId$.pipe(
switchMap((userId) =>
this.cipherArchiveService
.archivedCiphers$(userId)
.pipe(map((ciphers) => ciphers.length > 0)),
),
),
);Issue: Recommendation: Use protected userCanArchive = toSignal(
this.userId$.pipe(
switchMap((userId) => this.cipherArchiveService.userCanArchive$(userId)),
takeUntilDestroyed()
),
{ requireSync: false }
);Good Practices Observed
Action Items for AuthorMust Fix (Blocking)
Should Fix (High Priority)
Nice to Have (Low Priority)
Regarding Configuration Design (Author's Concern)Your concern about the
However, if this is the only use case for now, the current implementation is acceptable with the type fixes mentioned in Must Fix #3. |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## PM-19152-web-archive #16854 +/- ##
========================================================
- Coverage 38.92% 38.91% -0.02%
========================================================
Files 3437 3437
Lines 97593 97622 +29
Branches 14681 14686 +5
========================================================
+ Hits 37989 37990 +1
- Misses 57944 57972 +28
Partials 1660 1660 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
🎟️ Tracking
PM-24535
📔 Objective
Add steps for the user to upgrade to if they do not have access to premium:
📸 Screenshots
no-premium.mov
lost-premium.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