Skip to content

Conversation

@jrmccannon
Copy link
Contributor

@jrmccannon jrmccannon commented May 19, 2025

๐ŸŽŸ๏ธ Tracking

PM-21691

๐Ÿ“” Objective

This splits the collections code out of the vault crate and adds a Tree implementation for representing the parent child relationship between nested collections.

This ideally will then be used as a unified representation for the nested collection structure across all clients, instead of each one having to implement their own.

โฐ 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

@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2025

Logo
Checkmarx One โ€“ Scan Summary & Details โ€“ ab5a157d-8484-42d8-a917-00f0551cb98a

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

@jrmccannon jrmccannon force-pushed the jmccannon/ac/pm-21691-nested-tree branch from d1e534f to b0dbb1c Compare May 27, 2025 21:15
@codecov
Copy link

codecov bot commented May 27, 2025

Codecov Report

โŒ Patch coverage is 68.27795% with 105 lines in your changes missing coverage. Please review.
โœ… Project coverage is 73.34%. Comparing base (49d027c) to head (b2a8db9).
โš ๏ธ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/bitwarden-vault/src/collection_client.rs 0.00% 45 Missing โš ๏ธ
crates/bitwarden-uniffi/src/vault/collections.rs 0.00% 32 Missing โš ๏ธ
crates/bitwarden-collections/src/tree.rs 94.37% 13 Missing โš ๏ธ
crates/bitwarden-collections/src/collection.rs 20.00% 12 Missing โš ๏ธ
crates/bitwarden-collections/src/error.rs 0.00% 1 Missing โš ๏ธ
crates/bitwarden-collections/src/lib.rs 0.00% 1 Missing โš ๏ธ
crates/bitwarden-collections/src/uniffi_support.rs 0.00% 1 Missing โš ๏ธ
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #279      +/-   ##
==========================================
- Coverage   73.43%   73.34%   -0.10%     
==========================================
  Files         248      252       +4     
  Lines       20962    21283     +321     
==========================================
+ Hits        15393    15609     +216     
- Misses       5569     5674     +105     

โ˜” View full report in Codecov by Sentry.
๐Ÿ“ข Have feedback on the report? Share it here.

๐Ÿš€ New features to boost your workflow:
  • โ„๏ธ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • ๐Ÿ“ฆ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coroiu coroiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments, but very nice overall, +1 for the tests ๐Ÿ‘

@jrmccannon jrmccannon changed the title Added tree and test with implementation for CollectionView [PM-21691] - Added tree and test with implementation for CollectionView Jun 6, 2025
@jrmccannon jrmccannon marked this pull request as ready for review July 15, 2025 16:18
@jrmccannon jrmccannon requested review from a team as code owners July 15, 2025 16:18
Copy link
Contributor

@gbubemismith gbubemismith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just noticed a few things

@jrmccannon jrmccannon requested a review from gbubemismith July 25, 2025 13:49
gbubemismith
gbubemismith previously approved these changes Jul 25, 2025
@gbubemismith gbubemismith self-requested a review July 25, 2025 14:09
Copy link
Contributor

@gbubemismith gbubemismith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one last comment

@sonarqubecloud
Copy link

@jrmccannon jrmccannon merged commit 58a9bf9 into main Jul 30, 2025
49 checks passed
@jrmccannon jrmccannon deleted the jmccannon/ac/pm-21691-nested-tree branch July 30, 2025 20:12
quexten added a commit that referenced this pull request Aug 14, 2025
## ๐ŸŽŸ๏ธ Tracking

https://bitwarden.slack.com/archives/C054ZQSBS49/p1755188403409309

## ๐Ÿ“” Objective

It seems #279 accidentally
required wasm in all uniffi builds via `bitwarden-vault` ->
`bitwarden-collections` -> `bitwarden-core`. Since it is only required
on core, this leads to build issues on anything where an impl is tagged
with wasm-bindgen, but uses a struct from a third package - such as
`bitwarden-crypto` that does not have the wasm feature enabled.

Attempts to fix the build errors of:
#383

Note: This flags out a function from the collections client. This has to
be fixed by the owning team and updated, this PR just unblocks builds
and fixes the imports.

## โฐ 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

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- ๐Ÿ‘ (`:+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

---------

Co-authored-by: Oscar Hinton <Hinton@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants