[PM-27059] Browser: Retain vault filters when editing a cipher from the dropdown

[nikwithak]

🎟️ Tracking

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

📔 Objective

In the browser extension, when editing a cipher from the dropdown menu, it clears the filters on the main vault page. We want the filters to remain in place while the tab is active.

📸 Screenshots

pm-27059_vault_filters_dont_clear.mov

⏰ 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

[claude]

Claude finished @nikwithak's task —— View job

---

Code Review Complete ✅

---

Summary of Changes

This PR modifies the clearVaultStateGuard to preserve vault filters when navigating to cipher-related routes. The guard was renamed from isViewingCipher to isCipherOpen and now checks for four cipher-related routes instead of just one:

• view-cipher (existing)
• edit-cipher (new)
• clone-cipher (new)
• assign-collections (new)

A comprehensive test suite was added covering all scenarios.

---

🎨 Suggested Improvements

1. Missing Route: attachments

The attachments route (apps/browser/src/popup/app-routing.module.ts:220) should likely be included in isCipherOpen. When a user views/edits attachments for a cipher, they're still working with that cipher and would expect filters to be preserved.

Suggested fix:

const isCipherOpen = (url: string): boolean =>
  url.includes("view-cipher") ||
  url.includes("assign-collections") ||
  url.includes("edit-cipher") ||
  url.includes("clone-cipher") ||
  url.includes("attachments");

2. Missing Route: cipher-password-history

Similarly, cipher-password-history (apps/browser/src/popup/app-routing.module.ts:197) is another cipher detail view where users would expect filters to persist.

Suggested fix:

const isCipherOpen = (url: string): boolean =>
  url.includes("view-cipher") ||
  url.includes("assign-collections") ||
  url.includes("edit-cipher") ||
  url.includes("clone-cipher") ||
  url.includes("attachments") ||
  url.includes("cipher-password-history");

3. Potential False Positives with String Matching

The url.includes() approach could theoretically match unintended routes. For example, if a future route is named /my-edit-cipher-tool, it would incorrectly be treated as a cipher route.

Suggested improvements:

• Use more precise matching (e.g., check for /edit-cipher with a leading slash)
• Or use regex patterns
• Or check against a whitelist of known cipher route paths

Example with more precise matching

const isCipherOpen = (url: string): boolean => {
  const cipherRoutes = [
    "/view-cipher",
    "/edit-cipher", 
    "/clone-cipher",
    "/assign-collections",
    "/attachments",
    "/cipher-password-history"
  ];
  return cipherRoutes.some(route => url.includes(route));
};

Or with regex for exact segment matching:

const isCipherOpen = (url: string): boolean => {
  const cipherRoutePattern = /\/(view-cipher|edit-cipher|clone-cipher|assign-collections|attachments|cipher-password-history)(\?|$)/;
  return cipherRoutePattern.test(url);
};

4. Missing Route: add-cipher

The add-cipher route (apps/browser/src/popup/app-routing.module.ts:203) is not included. Consider whether filters should persist when adding a new cipher from the vault dropdown. This might be intentional (clearing filters when adding new items), but it's worth verifying the desired UX.

Question for review: Should add-cipher also preserve filters? If a user is filtered to a specific folder/collection and clicks "add," should the filter remain?

---

📝 Test Coverage Assessment

Strengths:

• ✅ Good use of it.each for parameterized tests
• ✅ Tests cover all four included cipher routes
• ✅ Tests cover the null state edge case
• ✅ Tests verify both positive (should not clear) and negative (should clear) cases
• ✅ Proper use of Jest mocks and TestBed

Gaps:

• ❌ Missing test cases for attachments route (if added per suggestion i18n #1)
• ❌ Missing test cases for cipher-password-history route (if added per suggestion Site Search #2)
• ❌ No tests for edge cases like:
  • URL with multiple query parameters: /edit-cipher?cipherId=123&organizationId=456
  • URL with hash fragments: /edit-cipher#section
  • Malformed URLs

Suggested additional test cases

it.each([
  "/attachments?cipherId=123",
  "/cipher-password-history?cipherId=123",
])("should not clear vault state when viewing cipher details: %s", (url) => {
  const nextState = { url } as RouterStateSnapshot;

  const result = TestBed.runInInjectionContext(() =>
    clearVaultStateGuard({} as VaultV2Component, null, null, nextState),
  );

  expect(result).toBe(true);
  expect(applyFilterSpy).not.toHaveBeenCalled();
  expect(resetFilterFormSpy).not.toHaveBeenCalled();
});

it("should handle URLs with multiple query parameters", () => {
  const nextState = { url: "/edit-cipher?cipherId=123&organizationId=456" } as RouterStateSnapshot;

  const result = TestBed.runInInjectionContext(() =>
    clearVaultStateGuard({} as VaultV2Component, null, null, nextState),
  );

  expect(result).toBe(true);
  expect(applyFilterSpy).not.toHaveBeenCalled();
  expect(resetFilterFormSpy).not.toHaveBeenCalled();
});

---

✅ Good Practices Observed

• Well-structured, readable test file using modern Angular testing patterns
• Clear documentation in JSDoc comment explaining guard purpose
• Proper use of Angular dependency injection with inject()
• Consistent code style matching repository standards

---

Security & Performance

• ✅ No security concerns: No sensitive data handling, encryption logic, or API calls
• ✅ Performance: String operations are fast; no performance concerns
• ✅ No unencrypted vault data exposure

---

Action Items

High Priority:

1. Consider adding attachments and cipher-password-history routes to isCipherOpen() per suggestions i18n #1 and Site Search #2
2. Add corresponding test cases for any new routes added

Medium Priority:

3. Clarify whether add-cipher should preserve filters (UX decision)
4. Consider more precise URL matching per suggestion Overlay Popup #3 to avoid false positives

Low Priority:

5. Add edge case tests for URLs with complex query parameters

---

| Branch: vault/pm-27059/dont-clear-vault-filter

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Checkmarx One – Scan Summary & Details – f3a15bbb-13cf-4ddf-9de8-1f7de4966353

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.17%. Comparing base (94cb1fe) to head (893f7e5).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16910      +/-   ##
==========================================
+ Coverage   39.15%   39.17%   +0.01%     
==========================================
  Files        3456     3456              
  Lines       97972    97969       -3     
  Branches    14730    14731       +1     
==========================================
+ Hits        38364    38380      +16     
+ Misses      57939    57921      -18     
+ Partials     1669     1668       -1     

☔ View full report in Codecov by Sentry.
📢 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.