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

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

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon
โ€ฆstructure.
@jrmccannon jrmccannon force-pushed the jmccannon/ac/pm-21691-nested-tree branch from d1e534f to b0dbb1c Compare May 27, 2025 21:15
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 and others added 20 commits May 28, 2025 08:10

Partially verified

This commit was created on GitHub.com and signed with GitHubโ€™s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
Co-authored-by: Andreas Coroiu <[email protected]>

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon
# Conflicts:
#	crates/bitwarden-vault/src/lib.rs

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
coroiu Andreas Coroiu

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon
โ€ฆdings to attempt to solve compile issue.

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon
# Conflicts:
#	crates/bitwarden-collections/src/collection.rs

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon
@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

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon
# Conflicts:
#	crates/bitwarden-collections/src/collection.rs

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon
@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

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon
@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

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon

Verified

This commit was signed with the committerโ€™s verified signature.
jrmccannon Jared McCannon
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

Verified

This commit was created on GitHub.com and signed with GitHubโ€™s verified signature.
## ๐ŸŽŸ๏ธ 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 <[email protected]>
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.

None yet

4 participants