Skip to content

PM-40380: Add New Item button to Vault Toolbar - #22437

Closed
nikwithak wants to merge 2 commits into
mainfrom
vault/pm-40380
Closed

PM-40380: Add New Item button to Vault Toolbar#22437
nikwithak wants to merge 2 commits into
mainfrom
vault/pm-40380

Conversation

@nikwithak

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-40380

📔 Objective

Moves the Add Item button into the new vault table toolbar. This also replaces the old vault list component with the new vault table, gated behind the vfo1-foundation feature flag.

📸 Screenshots

image

@nikwithak
nikwithak requested a review from a team as a code owner August 14, 2026 17:40
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

Reviewed the move of the Add Item button into the new vault table toolbar and the vfo1-foundation-gated swap from app-vault-items to vault-items-table in the web individual vault. The feature flag defaults to FALSE, the header correctly stops rendering its own vault-new-cipher-menu (and its coachmark) when the flag is on, so there is no duplicate addItem coachmark anchor, and the spec's getFeatureFlag$ mock keeps the legacy branch under test. Two findings concern the flag-on path: a lost free-organization collection limit guard and incomplete host wiring of the new table.

Code Review Details
  • ⚠️ : Free-organization collection limit / upgrade dialog guard is bypassed when (collectionAdded) binds directly to VaultComponent.addCollection()
    • apps/web/src/app/vault/individual-vault/vault.component.html:74
  • ⚠️ : Table hosted without itemAction, folders, rowActions, or (selectedChange), so with the flag on items cannot be opened, the My folders column/filter is inert, row actions are gone, and the batch bar receives no selection
    • apps/web/src/app/vault/individual-vault/vault.component.html:55

[coachmarkPosition]="coachmarkService.getStepPosition('addItem')"
(cipherAdded)="addCipher($event)"
(folderAdded)="addFolder()"
(collectionAdded)="addCollection()"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: The free-organization collection limit check is skipped on this path.

Details and fix

In the header, the menu's collection option went through VaultHeaderComponent.addCollection(), which guards before emitting onAddCollection: for a user with a single free organization already at maxCollections, it opens the upgrade dialog and returns (vault-header.component.ts lines 319-340).

Binding (collectionAdded) directly to VaultComponent.addCollection() drops that guard, so with vfo1-foundation enabled a free-org user at the limit now gets the collection dialog and a server-side failure on save instead of the upgrade prompt.

Moving the limit check into VaultComponent.addCollection() would restore it here and also cover openAddItemDialog(), which calls the same method.

Comment on lines +55 to +60
<vault-items-table
[ciphers]="ciphers"
[loading]="performingInitialLoad || refreshing"
[collections]="allCollections"
[organizations]="allOrganizations"
>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: With itemAction unset, no vault item can be opened while the flag is on.

Details and fix

VaultItemsTableComponent.itemAction is documented as: "Runs when a row's name is activated. Omit to render the name as plain text rather than a button." With vfo1-foundation enabled, the list therefore offers no way to view or edit an item, since app-vault-items (which handled that) is no longer rendered.

Compared with the desktop host (apps/desktop/src/vault/app/vault-v3/vault-list-table/vault-list-table.component.html lines 12-22), three other inputs are also unbound here:

  • foldersfolderChips() resolves names from it, so the always-visible My folders column renders "None" for every row and the My folders filter chip stays disabled with the "no folders" tooltip even when the user has folders.
  • rowActions — the overflow menu only renders when visibleActions(row).length > 0, so there are no per-row actions (clone, move, delete, restore).
  • (selectedChange) — the old app-vault-items fed VaultBatchBarService; the table's selection is not forwarded, so with PM37785_VaultBatchBar also on the batch bar never receives a selection.

If these are intentionally deferred to follow-up work, a short comment or ticket reference here would help.

@nick-livefront nick-livefront left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this overlap with @shane-melton's #22425?

@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.97%. Comparing base (25d5c69) to head (06005bf).
⚠️ Report is 9 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../src/app/vault/individual-vault/vault.component.ts 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #22437      +/-   ##
==========================================
+ Coverage   53.93%   53.97%   +0.04%     
==========================================
  Files        4297     4304       +7     
  Lines      136500   136670     +170     
  Branches    21569    21586      +17     
==========================================
+ Hits        73616    73769     +153     
- Misses      57518    57522       +4     
- Partials     5366     5379      +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nikwithak

Copy link
Copy Markdown
Contributor Author

@nick-livefront Thanks for catching that - I synced up with @shane-melton and it seems we ended up with some dupe tickets - closing this one out in favor of his #22425

@nikwithak nikwithak closed this Aug 14, 2026
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.

2 participants