[PM-28222] delay progress bar
đī¸ Tracking
https://bitwarden.atlassian.net/browse/PM-28222
đ Objective
When a report is loaded quickly, the progress bar jumps through the steps and creates a jarring visual for the user.
Add a delay in between events in the progress pipeline.
đ¸ Screenshots
https://github.com/user-attachments/assets/a24c4fdb-3040-4bec-979e-b48aa0c5b619
â° 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 finished @AlexRubik's task in 4m 0s ââ View job
Code Review Complete
I've completed a thorough review of this PR. The implementation is generally solid with proper use of RxJS delay operators and adherence to Bitwarden's architectural patterns.
Key Findings
â ī¸ Important Issues
-
All Applications Tab Shows Static Progress - The component at
all-applications.component.html:2doesn't pass theprogressStepinput, causing static progress display instead of animated steps. -
Activity Tab Has Same Issue - The component at
all-activity.component.html:2also lacks theprogressStepinput.
Impact: Users on these tabs won't see accurate progress feedback during report generation.
đ¨ Suggestion
Consider extracting the concatMap + delay pattern from risk-insights.component.ts:192-222 to a shared utility if it needs to be reused in the All Applications and Activity tabs.
Naming Convention Clarification
The latest review comment about "selector and file name not matching component name" appears to be a misunderstanding. The current naming is correct per Angular conventions:
- Component:
ReportLoadingComponentâ - Files:
report-loading.component.ts/htmlâ - Selector:
dirt-report-loadingâ
Angular selectors use kebab-case with a prefix, so this is proper.
Architecture Review â
- Correctly follows ADR-0025 (const object instead of enum for
PROGRESS_STEPS) - Proper Observable data service patterns (ADR-0003)
- Good separation of concerns (delay logic in parent, presentational loading component)
- Security: No issues found
Checkmarx One â Scan Summary & Details â 0334133c-c4d4-4894-83f0-be7ab1d5d25e
Great job! No new security vulnerabilities introduced in this pull request
Codecov Report
:x: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 42.07%. Comparing base (9db6321) to head (ee7a1f0).
:warning: Report is 7 commits behind head on main.
:white_check_mark: All tests successful. No failed tests found.
Additional details and impacted files
@@ Coverage Diff @@
## main #17507 +/- ##
=======================================
Coverage 42.06% 42.07%
=======================================
Files 3595 3595
Lines 104422 104427 +5
Branches 15744 15748 +4
=======================================
+ Hits 43930 43934 +4
- Misses 58625 58626 +1
Partials 1867 1867
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Needed to make these loading component changes so there would be a min delay for each step. Without this, all the steps complete almost instantly (jarring animation) when there isn't a lot of data to process and would stall on the last step dependent on LOADING_DISPLAY_DELAY_MS.
Overall Assessment: APPROVE
Reviewed progress bar delay implementation across loading component refactoring. The architecture is sound with proper separation of concerns - delay logic in parent component, presentational loading component accepting progress as input. ADR-0025 compliance is correct (const object with type alias instead of enum). RxJS patterns follow Observable data service conventions.
The naming convention is correct per Angular standards (ReportLoadingComponent class, report-loading files, dirt-report-loading selector - all properly aligned).
See inline comments for findings.