clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-28222] delay progress bar

Open AlexRubik opened this issue 1 month ago â€ĸ 3 comments

đŸŽŸī¸ 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

AlexRubik avatar Nov 20 '25 02:11 AlexRubik

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

  1. All Applications Tab Shows Static Progress - The component at all-applications.component.html:2 doesn't pass the progressStep input, causing static progress display instead of animated steps.

  2. Activity Tab Has Same Issue - The component at all-activity.component.html:2 also lacks the progressStep input.

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

claude[bot] avatar Nov 20 '25 02:11 claude[bot]

Logo Checkmarx One – Scan Summary & Details – 0334133c-c4d4-4894-83f0-be7ab1d5d25e

Great job! No new security vulnerabilities introduced in this pull request

github-actions[bot] avatar Nov 20 '25 02:11 github-actions[bot]

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.

Files with missing lines Patch % Lines
...irt/access-intelligence/risk-insights.component.ts 0.00% 12 Missing :warning:
...ss-intelligence/shared/report-loading.component.ts 0.00% 9 Missing :warning:
...ss-intelligence/activity/all-activity.component.ts 0.00% 1 Missing :warning:
...nce/all-applications/all-applications.component.ts 0.00% 1 Missing :warning:
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.

codecov[bot] avatar Nov 20 '25 02:11 codecov[bot]

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.

add minimum display time for each progress step

AlexRubik avatar Dec 15 '25 20:12 AlexRubik

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.

claude[bot] avatar Dec 18 '25 15:12 claude[bot]