[PM-26687] send skeleton #17333

Jingo88 · 2025-11-11T17:52:41Z

🎟️ Tracking

Note branch name shows incorrect ticket. it should be the one below
PM-26687

📔 Objective

Adding the new vault loading skeleton to the browser send page.

📸 Screen Recording (slowed down to see loading)

Empty Send Does not show skeleton (Skeleton Feature Flag On)

PM-26687-send-empty-no-skeleton.mov

Has Items (Skeleton Feature Flag On)

PM-27523-send-skeleton-items.mov

Show Loading Spinner (Skeleton Feature Flag Off)

PM-27523-loading-spinner.mov

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

claude · 2025-11-11T17:54:11Z

Claude finished @Jingo88's task —— View job

Code Review Complete

The implementation looks solid overall. The PR successfully adds skeleton loading to the browser send page with a feature flag toggle. Previous review feedback has been addressed - the shareReplay operator was added and the unused translation keys have been removed.

Findings

Finding 1: Missing startWith(true) could cause brief empty state flash before initial load

💭 The sendsLoading$ observable (send-v2.component.ts:84-87) doesn't include startWith(true) like the vault implementation does (vault-popup-loading.service.ts:25). This means the skeleton won't display during the brief moment between component initialization and the first emission from sendItemsService.loading$.

However, given that send data loads very quickly (as noted in review discussion), and there's a separate ticket (PM-27710) to address loading flicker, this may be intentional. The vault implementation has more complex dependencies that justify the startWith(true).

Finding 2: Test coverage missing for new loading state logic

⚠️ The codecov report shows 4 lines missing coverage (77.78% patch coverage). The new loading state observables (showSpinnerLoaders$, showSkeletonsLoaders$, sendsLoading$) lack test coverage. Consider adding tests similar to the vault implementation that verify:

Feature flag toggles between spinner and skeleton
Loading state transitions correctly
The shareReplay operator prevents duplicate subscriptions

Example test structure:

it("should show spinner loaders when loading and skeleton flag is disabled", () => {
  configService.getFeatureFlag$.mockReturnValue(of(false));
  sendItemsService.loading$ = of(true);
  // Assert showSpinnerLoaders$ emits true
});

Finding 3: Mixed control flow syntax in template

💭 The template mixes modern control flow (@if on line 49) with legacy structural directives (*ngIf on lines 4, 10, 13, 20, 28, 30, 37, 39). While both are valid, consistent use of one pattern improves maintainability. This is common during Angular modernization efforts, but worth noting for future cleanup.

Summary

✅ Strengths:

Proper use of shareReplay to prevent multiple subscriptions
Clean feature flag implementation following established patterns
Previous review feedback addressed (translation keys removed, shareReplay added)
Correctly hides content when skeleton is showing (line 37)

📝 Notes:

Missing test coverage is the main concern - would help catch regressions
startWith(true) omission appears intentional given sends load quickly
Pattern follows vault implementation closely, which is good for consistency

github-actions · 2025-11-11T18:00:41Z

Checkmarx One – Scan Summary & Details – c74727f7-1ce3-4011-afd8-b2688f76765c

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CVE-2025-12429	Npm-electron-37.7.0	details Description: Inappropriate implementation in V8 in Google Chrome prior to 142.0.7444.59 allowed a remote attacker to perform arbitrary read/write via a crafted ... Attack Vector: NETWORK Attack Complexity: LOW `ID: vEtSmal6GP37yZzyalQ5elcNrwFoHK71hWBDIL0v6Gs%3D` Vulnerable Package

codecov · 2025-11-11T18:04:16Z

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.93%. Comparing base (b2682a4) to head (fb400c7).
⚠️ Report is 31 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...owser/src/tools/popup/send-v2/send-v2.component.ts	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17333      +/-   ##
==========================================
+ Coverage   40.74%   40.93%   +0.18%     
==========================================
  Files        3541     3544       +3     
  Lines      101415   101725     +310     
  Branches    15188    15240      +52     
==========================================
+ Hits        41321    41637     +316     
+ Misses      58353    58334      -19     
- Partials     1741     1754      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The base branch was changed.

claude · 2025-11-12T20:36:14Z

apps/browser/src/tools/popup/send-v2/send-v2.component.ts

+  protected sendsLoading$ = this.sendItemsService.loading$.pipe(
+    distinctUntilChanged(),
+    tap((loading) => {
+      const key = loading ? "loadingSendPage" : "sendPageLoaded";
+      void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite");
+    }),
+  );


⚠️ Multiple subscription issue: This observable is subscribed to 6+ times (4 direct template subscriptions + 2 via combineLatest in showSpinnerLoaders$/showSkeletonsLoaders$), which means the tap() side effect will fire multiple times per state change.

Recommendation: Add shareReplay({ bufferSize: 1, refCount: true }) to the pipe to prevent duplicate announcements:

protected sendsLoading$ = this.sendItemsService.loading$.pipe( distinctUntilChanged(), tap((loading) => { const key = loading ? "loadingSendPage" : "sendPageLoaded"; void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite"); }), shareReplay({ bufferSize: 1, refCount: true }), );

Also consider adding startWith(true) like vault-popup-loading.service.ts:25 to ensure skeleton displays immediately on init.

apps/browser/src/tools/popup/send-v2/send-v2.component.ts

claude · 2025-11-12T21:48:29Z

apps/browser/src/tools/popup/send-v2/send-v2.component.ts

+  private skeletonFeatureFlag$ = this.configService.getFeatureFlag$(
+    FeatureFlag.VaultLoadingSkeletons,
+  );
+  protected sendsLoading$ = this.sendItemsService.loading$.pipe(


⚠️ Missing startWith(true) for initial skeleton display

The observable doesn't include startWith(true), which means the skeleton won't display on initial component load before the first emission from sendItemsService.loading$.

Recommended addition:

protected sendsLoading$ = this.sendItemsService.loading$.pipe( startWith(true), // Add this distinctUntilChanged(), tap((loading) => { const key = loading ? "loadingSendPage" : "sendPageLoaded"; void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite"); }), shareReplay({ bufferSize: 1, refCount: true }), );

Reference: vault-popup-loading.service.ts:25 uses startWith(true) to ensure skeleton displays immediately on init.

claude · 2025-11-12T21:48:36Z

apps/browser/src/tools/popup/send-v2/send-v2.component.html

    </div>
    <app-send-list-items-container [headerText]="title | i18n" [sends]="sends$ | async" />
  </ng-container>
+  @if (sendsLoading$ | async) {


💭 Inconsistent control flow syntax

The template mixes modern control flow syntax (@if on line 49) with legacy structural directives (*ngIf on lines 4, 10, 13, 20, 28, 30, 37, 39).

While both are valid, consistent use of one pattern improves maintainability. Consider whether modernizing all to @if/@for is in scope for this PR or should be addressed separately.

claude · 2025-11-12T21:48:45Z

apps/browser/src/tools/popup/send-v2/send-v2.component.ts

+  private skeletonFeatureFlag$ = this.configService.getFeatureFlag$(
+    FeatureFlag.VaultLoadingSkeletons,
+  );
+  protected sendsLoading$ = this.sendItemsService.loading$.pipe(


🎨 Consider extracting to dedicated loading service

The vault implementation uses VaultPopupLoadingService (vault-popup-loading.service.ts:11-27) to centralize loading logic, improving testability and maintainability.

Consider creating a SendPopupLoadingService that:

Combines sendItemsService.loading$ with any future dependencies

Includes startWith(true) and shareReplay() for proper initialization and sharing

Can be easily mocked in tests (current tests don't verify loading behavior - see send-v2.component.spec.ts)

This would improve consistency with the vault implementation and make the component logic cleaner. However, this is an optional enhancement.

apps/browser/src/tools/popup/send-v2/send-v2.component.html

claude · 2025-11-13T01:33:10Z

apps/browser/src/tools/popup/send-v2/send-v2.component.ts

+  protected sendsLoading$ = this.sendItemsService.loading$.pipe(
+    distinctUntilChanged(),
+    tap((loading) => {
+      const key = loading ? "loadingSendPage" : "sendPageLoaded";
+      void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite");
+    }),
+    shareReplay({ bufferSize: 1, refCount: true }),
+  );


❌ Missing startWith(true) for initial loading state

Without startWith(true), the skeleton won't display during initial component load before the first emission from sendItemsService.loading$. This can cause a flash of empty content.

Add this line after line 86:

protected sendsLoading$ = this.sendItemsService.loading$.pipe( startWith(true), // Add this distinctUntilChanged(), tap((loading) => { const key = loading ? "loadingSendPage" : "sendPageLoaded"; void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite"); }), shareReplay({ bufferSize: 1, refCount: true }), );

Also import startWith on line 5.

Reference: vault-popup-loading.service.ts:25

Hinton · 2025-11-13T10:37:02Z

apps/browser/src/tools/popup/send-v2/send-v2.component.ts

+  protected sendsLoading$ = this.sendItemsService.loading$.pipe(
+    distinctUntilChanged(),
+    tap((loading) => {
+      const key = loading ? "loadingSendPage" : "sendPageLoaded";
+      void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite");
+    }),
+    shareReplay({ bufferSize: 1, refCount: true }),
+  );


question: Will this actually provide value? In most cases this will be very quick and might be quite annoying.

That's a fair point. I was following the skeleton loading patterns we're implementing in vault. But sends should be way quicker to load. I'll bring this up to design.

It seems the skeleton components have a 1s delay, the announce should probably mirror this.

Removed. Thanks!

Edit (did not see previous comment) - We're going to keep it removed for now. We have another ticket for avoiding a flicker where we can revisit this if needed.

apps/browser/src/_locales/en/messages.json

* adding skeleton to send

Jingo88 requested a review from a team as a code owner November 11, 2025 17:52

itsadrago previously approved these changes Nov 11, 2025

View reviewed changes

Jingo88 requested review from a team as code owners November 11, 2025 20:36

Jingo88 requested review from BTreston, addisonbeck, amorask-bitwarden, fntyler, mzieniukbw, nikwithak, rr-bw and vleague2 and removed request for a team November 11, 2025 20:36

Jingo88 changed the base branch from vault/pm-25081/vault-skeleton-loaders to main November 11, 2025 20:37

Jingo88 marked this pull request as draft November 11, 2025 20:37

Jingo88 removed request for a team November 11, 2025 20:38

Jingo88 added 2 commits November 12, 2025 15:14

adding flag check for skeleton in send

9a6ce54

update vault skeleton import in send

c0d1044

Jingo88 force-pushed the PM-27523-send-skeleton branch from 5878efd to c0d1044 Compare November 12, 2025 20:20

Jingo88 marked this pull request as ready for review November 12, 2025 20:32

Jingo88 requested review from itsadrago and removed request for a team November 12, 2025 20:32

claude bot reviewed Nov 12, 2025

View reviewed changes

Jingo88 changed the title ~~[PM-27523] send skeleton~~ [PM-26687] send skeleton Nov 12, 2025

empty state for send should show regardless of skeleton loading

dc84440

claude bot reviewed Nov 12, 2025

View reviewed changes

apps/browser/src/tools/popup/send-v2/send-v2.component.ts Show resolved Hide resolved

claude bot reviewed Nov 12, 2025

View reviewed changes

Merge branch 'main' into PM-27523-send-skeleton

96b4399

itsadrago reviewed Nov 12, 2025

View reviewed changes

apps/browser/src/tools/popup/send-v2/send-v2.component.html Show resolved Hide resolved

Jingo88 added 2 commits November 12, 2025 20:23

adding shareReplay

26e34fe

apply skeletonloading observable to dom

90c3ad0

claude bot reviewed Nov 13, 2025

View reviewed changes

Hinton reviewed Nov 13, 2025

View reviewed changes

remove liveannouncer

313fd45

Jingo88 requested review from Hinton and itsadrago November 13, 2025 15:56

itsadrago reviewed Nov 14, 2025

View reviewed changes

apps/browser/src/_locales/en/messages.json Outdated Show resolved Hide resolved

remove unused translation keys

fb400c7

Jingo88 requested a review from itsadrago November 14, 2025 18:50

itsadrago approved these changes Nov 14, 2025

View reviewed changes

Jingo88 merged commit 8a3f1ee into main Nov 14, 2025
60 checks passed

Jingo88 deleted the PM-27523-send-skeleton branch November 14, 2025 21:16

maxkpower pushed a commit that referenced this pull request Nov 17, 2025

[PM-26687] send skeleton (#17333)

e60579d

* adding skeleton to send

[PM-26687] send skeleton #17333

[PM-26687] send skeleton #17333

Uh oh!

Conversation

Jingo88 commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screen Recording (slowed down to see loading)

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

claude bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete

Findings

Summary

Uh oh!

github-actions bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude bot Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Hinton Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Jingo88 Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Hinton Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Jingo88 Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jingo88 commented Nov 11, 2025 •

edited

Loading

claude bot commented Nov 11, 2025 •

edited

Loading

github-actions bot commented Nov 11, 2025 •

edited

Loading

codecov bot commented Nov 11, 2025 •

edited

Loading

Jingo88 Nov 13, 2025 •

edited

Loading