[PM-40318] Introduce Web's VaultNextComponet for VFO1 - #22425
[PM-40318] Introduce Web's VaultNextComponet for VFO1#22425shane-melton wants to merge 2 commits into
Conversation
|
@nick-livefront Tagging you since you've just done similar work in Desktop. I have a few minor things to clean up still, but should be ready for an initial review. |
shane-melton
added
ai-review
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return false; | ||
| } | ||
|
|
||
| return this.organizations().length > 0 && collections.some((c) => !c.readOnly); |
There was a problem hiding this comment.
❓ QUESTION: Should the writable-collection check be scoped to the cipher's own organization?
Details
The gate is global: any org membership plus any writable collection anywhere. For a cipher that already belongs to org A, the destination is fixed — WebVaultItemActionsService.assignToCollections narrows availableCollections to c.organizationId === cipher.organizationId, and AssignCollectionsComponent.handleOrganizationCiphers narrows further with canEditItems(org).
So a user in org A (read-only collections only) and org B (writable) sees "Add to shared folder" on an org A item and lands in a dialog with an empty picker.
A per-org scope for org ciphers would keep the global check for personal items, where the dialog does let the user choose the org:
const assignable = cipher.organizationId
? collections.filter((c) => c.organizationId === cipher.organizationId && !c.readOnly)
: collections.filter((c) => !c.readOnly);
return this.organizations().length > 0 && assignable.length > 0;Not a regression — the previous gate (!!cipher.organizationId && canAssignToCollections) had the same dead end — so happy to leave it if the multi-org case is out of scope for VFO1.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #22425 +/- ##
==========================================
- Coverage 53.93% 53.92% -0.01%
==========================================
Files 4297 4299 +2
Lines 136500 136639 +139
Branches 21569 21590 +21
==========================================
+ Hits 73616 73685 +69
- Misses 57518 57584 +66
- Partials 5366 5370 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nick-livefront
left a comment
nick-livefront
left a comment
There was a problem hiding this comment.
Nothing major to me, looks great!
| this.accountService.activeAccount$.pipe( | ||
| getUserId, |
There was a problem hiding this comment.
⛏️ (non-blocking) This is the second usage of activeAccount$ -> getUserId. Should we make a single reference to active account. Minor benefit of one emission rather than two, Angular might optimize that already 🤷
| const isDeleted = CipherViewLikeUtils.isDeleted(cipher); | ||
| const confirmed = await this.dialogService.openSimpleDialog({ | ||
| title: { key: "deleteItem" }, | ||
| title: { key: isDeleted ? "permanentlyDeleteItem" : "deleteItem" }, |
| * The web individual vault's cipher actions that open a web-specific dialog. | ||
| */ | ||
| @Injectable() | ||
| export class WebVaultItemActionsService { |
There was a problem hiding this comment.
👏 A separate service rather than keeping these in the vault, 10/10
| * Not yet wired: the typed filter adapter that syncs the | ||
| * table's chips to the URL, the redirect that rewrites legacy filter query params, and the | ||
| * `?itemId=&action=` deep link that opens an item on load. Until the chips are wired there is no | ||
| * route to trash or the archive from this page, so both are excluded from the list. |
There was a problem hiding this comment.
FYI: I should have a utility method up for review this morning that handles this.
2 participants
🎟️ Tracking
PM-40318
📔 Objective
Introduce the
VaultNextComponentthat uses the new, sharedVaultItemsTableComponentfor VFO1.Router query parameter mapping and legacy query param handling coming in a followup PR.
📸 Screenshots