[PM-40316] Restructure the desktop side navigation - #22585
Conversation
Register `/vault/:vaultId` alongside the unscoped `/vault` so the side nav's vault, trash and archive links resolve. Guarded by the VFO1Foundation flag and `vaultScopeGuard`, matching web's `VaultRoutingModule`.
Render the shared Vaults section and Manage links behind the VFO1Foundation flag, replacing the desktop-only vault filter. `VaultNavSectionComponent` and `VaultManageNavComponent` navigate through `routerLink`, so the layout no longer carries handlers for vault, archive and trash selection.
Filter rows with `cipherInScope` under the VFO1Foundation flag rather than the query-param filter, which the `:vaultId` path segment never reached. The table's chips and the create buttons read the same scope, so trash and archive suppress item creation and only in-scope vaults are offered. The legacy flag-off path keeps `RoutedVaultFilterService` and does not subscribe to `VaultNavService`.
gbubemismith
changed the title
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Re-reviewed the desktop side-nav restructure, the componentless Code Review Details
The "My folders" nav destination raised earlier is confirmed by the author as upcoming follow-up work and is not repeated. The archive "premium subscription ended" callout is likewise fed by the legacy filter and will not surface on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| </bit-nav-section> | ||
|
|
||
| <bit-nav-section [label]="'manage' | i18n"> | ||
| <vault-manage-nav /> |
There was a problem hiding this comment.
vault-manage-nav renders a "My folders" link, but desktop has no folders route.
Details and fix
VaultManageNavComponent's template hardcodes <bit-nav-item icon="bwi-folder" [text]="'myFolders' | i18n" route="folders">. The routerLink resolves against the layout route ({ path: "", component: DesktopLayoutComponent }), so it targets /folders, and apps/desktop/src/app/app-routing.module.ts registers no such path and no wildcard/fallback route — the click ends in NG04002: Cannot match any routes. (If it instead resolved under /vault, vaultScopeGuard would reject folders as neither a named scope nor a guid and bounce back to /vault, so the item is a dead end either way.)
Web wires this up in oss-routing.module.ts:
{
path: "folders",
component: MyFoldersComponent,
canActivate: [canAccessFeature(FeatureFlag.VFO1Foundation, true, "/vault")],
data: { titleId: "myFolders" } satisfies RouteDataProperties,
},This PR added /vault/:vaultId so the nav's Trash and Archive links resolve; the same treatment is needed for the folders link, otherwise every user with vfo1-foundation on gets a broken item in the Manage section. If a desktop folders page is planned for a later PR in the stack, worth confirming the ordering so the flag is never on with a dead link.
The new spec asserts "myFolders" in the expected nav text (desktop-layout.component.spec.ts), so the item is expected to render — only its destination is missing.
There was a problem hiding this comment.
Don't worry about this, there's an upcoming PR for this
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22585 +/- ##
==========================================
- Coverage 54.62% 54.61% -0.01%
==========================================
Files 4309 4309
Lines 137618 137632 +14
Branches 21797 21803 +6
==========================================
- Hits 75174 75171 -3
- Misses 56960 56977 +17
Partials 5484 5484 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /** The vault the `:vaultId` segment scopes this page to; always All items on the legacy nav. */ | ||
| private readonly vaultScope$ = this.vfo1Foundation$.pipe( | ||
| switchMap((vfo1Foundation) => | ||
| vfo1Foundation | ||
| ? combineLatest([ | ||
| this.route.paramMap.pipe(map((params) => params.get("vaultId"))), | ||
| this.vaultNavService.viewModel$, | ||
| ]).pipe(map(([vaultId, nav]) => resolveVaultScope(vaultId, nav) ?? ALL_ITEMS_SCOPE)) | ||
| : of(ALL_ITEMS_SCOPE), | ||
| ), | ||
| shareReplay({ refCount: true, bufferSize: 1 }), | ||
| ); |
There was a problem hiding this comment.
/vault/trash scope the batch bar still believes it is outside trash, so bulk Delete soft-deletes and Restore never appears.
Details and fix
VaultBatchBarService derives its trash state from the legacy query param, not from the scope:
readonly inTrash = toSignal(
this.routedVaultFilterService.filter$.pipe(map((f) => f.type === "trash")),
{ initialValue: false },
);Under vfo1-foundation trash is the /vault/trash route and vaultFilterLegacyRedirectGuard strips ?type=trash, so filter.type is never "trash" and inTrash() stays false. bit-vault-batch-action renders for both nav variants (vault.component.html gates it only on vaultBatchBarFeatureFlag()), and app-vault-list-table pushes its selection into the service via handleSelectionChange. With vfo1-foundation + PM37785_VaultBatchBar + PM37785_DesktopVaultBatchBar on, selecting rows in Trash gives:
- Delete labelled
deleteandbulkDeleterunning withpermanent = this.inTrash()→false, i.e. a soft delete of already-trashed items that appears to do nothing - No Restore action —
canRestoreANDs oninTrash - Archive / Add to folder / Add to shared folder offered on trashed items
Before this PR the trash scope was unreachable on desktop (as the description notes, the nav's /vault/trash link resolved to nothing), so the new route is what exposes this.
One option is to feed the scope in rather than have the service re-derive it: add a trash flag to VaultBatchBarConfig and set it from the existing setConfig({ isOrgVault: false, allCollections, hasCiphers }) call using vaultScope$, falling back to the routed filter when the flag is off.
There was a problem hiding this comment.
#22260 Is in the best position to fix this.
Register `/vault/:vaultId` alongside the unscoped `/vault` so the side nav's vault, trash and archive links resolve. Guarded by the VFO1Foundation flag and `vaultScopeGuard`, matching web's `VaultRoutingModule`.
Render the shared Vaults section and Manage links behind the VFO1Foundation flag, replacing the desktop-only vault filter. `VaultNavSectionComponent` and `VaultManageNavComponent` navigate through `routerLink`, so the layout no longer carries handlers for vault, archive and trash selection.
Filter rows with `cipherInScope` under the VFO1Foundation flag rather than the query-param filter, which the `:vaultId` path segment never reached. The table's chips and the create buttons read the same scope, so trash and archive suppress item creation and only in-scope vaults are offered. The legacy flag-off path keeps `RoutedVaultFilterService` and does not subscribe to `VaultNavService`.
9dd8471 to
5748db4
Compare
…itwarden/clients into vault/pm-40316-desktop-nav-restructure # Conflicts: # apps/web/src/app/vault/individual-vault/vault-routing.module.ts # libs/vault/src/routing/vault-scope.guard.spec.ts # libs/vault/src/routing/vault-scope.guard.ts
shane-melton
left a comment
shane-melton
left a comment
There was a problem hiding this comment.
Changes look good! Just a few non-blocking comments/suggestions.
|
|
||
| @if (!vfo1Foundation()) { | ||
| @if (vfo1Foundation()) { | ||
| <vault-nav-section /> |
There was a problem hiding this comment.
👍 Glad we get to reuse these! 🎉
| }, | ||
| }), | ||
| { | ||
| // The side nav's vault scopes: `my-vault` and an organization id. "All items" is the unscoped |
There was a problem hiding this comment.
⛏️ (non-blocking) I dropped these comments in my original PR as they were explained elsewhere better and sound very claude-like ("whole story here"...). Open to keeping if you find them beneficial.
| this.allOrganizations = vfo1Foundation | ||
| ? allOrganizations.filter((organization) => organizationInScope(organization, scope)) | ||
| : allOrganizations; | ||
| this.ciphers = ciphers; |
There was a problem hiding this comment.
❓ Would it make sense to do the same cipherInScope filtering here instead of the rowFilter change above? Just trying to think what would be easiest for eventual removal of the VFO1 feature flag.
There was a problem hiding this comment.
🎨 (non-blocking) The page header is still always "Vault" and doesn't update based on the scope. We can set the [title] on app-header in the template based on the scope. Though, I'm okay with leaving this alone since it'll be reworked in the breadcrumb epic.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40316
📔 Objective
The desktop counterpart to the web side-nav restructure in PM-40313. All changes sit behind the
vfo1-foundationflag; the legacy navigation and vault filtering are untouched when it is off.Navigation. The desktop-only vault filter is replaced by the shared
VaultNavSectionComponentandVaultManageNavComponent, so Vaults, Tools and Manage match web. Both navigate throughrouterLink, which let the layout drop its vault, archive and trash handlers along with four service injections.Routing.
/vault/:vaultIdis registered alongside the unscoped/vault, guarded bycanAccessFeature(VFO1Foundation, ...)andvaultScopeGuard— the same shape as web'sVaultRoutingModule. Without it the shared nav's links, and thevaultFilterLegacyRedirectGuardredirects to/vault/trashand/vault/archive, resolved to nothing on desktop.📸 Screenshots
Screen.Recording.2026-08-21.at.16.21.29.mov
Side Nav with vaults
Light mode