[PM-15065] Vault Loading on empty tabs

nick-livefront commented 6 days ago

🎟️ Tracking

📔 Objective

When on a non-standard page or empty tab is focused when opening the extension, the vault was taking longer than expected to load. This was caused from a message never being received in the collectPageDetailsFromTab$ observable.

The underlying sendMessage fails in Chrome and Firefox. That error was ignored in the tabSendMessage, I now rejected the promise to surface the error.
Safari does not throw an error but a new tab does not have a url, I added a check for that as there wouldn't be autofill information (to my knowledge)

Rejecting the promise

This approach falls in line with what is happening but it is also introducing a new set of logic where tabSendMessage is called. Scanning the codebase there are places that have error handling and some that do not. If there is a more idiomatic approach, let me know!

📸 Screenshots

Chrome	FireFox	Safari

🦮 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

codecov[bot] commented 6 days ago

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 33.44%. Comparing base (2df8643) to head (72ec9be). Report is 41 commits behind head on main.

:white_check_mark: All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
apps/browser/src/platform/browser/browser-api.ts	50.00%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #12059 +/- ## ========================================== - Coverage 33.52% 33.44% -0.09% ========================================== Files 2859 2863 +4 Lines 89434 89659 +225 Branches 17022 17066 +44 ========================================== + Hits 29986 29988 +2 - Misses 57087 57310 +223 Partials 2361 2361 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚨 Try these New Features:

JS Bundle Analysis - Avoid shipping oversized bundles

github-actions[bot] commented 6 days ago

Checkmarx One – Scan Summary & Details – 43062958-dc1d-4f5f-aa05-5d2bf1052f95

No New Or Fixed Issues Found

nick-livefront commented 5 days ago

@jprusik @dani-garcia @gbubemismith I added a commit here to make rejecting on an error configurable. That way existing implementations that don't have error handling in place won't throw for uncaught errors.

72ec9be

Relevant comment from Jon in slack

bitwarden / clients