[CL-761] Enable strict template typechecking #17334

BryanCunningham · 2025-11-11T18:17:22Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/CL-761

📔 Objective

Enable strict type checking in extension templates

Without a ton of context fixing the existing type errors was a bit of a guessing game. I had Claude help out here so please let me know which fixes are correct and which may have unintended side effects.

📸 Screenshots

⏰ 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

github-actions · 2025-11-11T18:27:28Z

Checkmarx One – Scan Summary & Details – bbbaea61-a21d-4907-aa40-fe13a038b94f

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

codecov · 2025-11-11T18:29:41Z

Codecov Report

❌ Patch coverage is 23.80952% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.25%. Comparing base (b009871) to head (1b7daca).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...-container/trash-list-items-container.component.ts	0.00%	10 Missing ⚠️
...wser/src/vault/popup/settings/archive.component.ts	0.00%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #17334   +/-   ##
=======================================
  Coverage   41.25%   41.25%           
=======================================
  Files        3545     3545           
  Lines      102007   102019   +12     
  Branches    15302    15305    +3     
=======================================
+ Hits        42080    42090   +10     
- Misses      58162    58164    +2     
  Partials     1765     1765

☔ 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.

…wser-strict-template-type-check

nick-livefront

Only some 🎨 to utilize our CipherViewLikeUtils which will handle the logic between a full CipherView and CipherListView for you!

nick-livefront · 2025-11-12T19:14:54Z

apps/browser/src/vault/popup/settings/archive.component.ts

  protected archivedCiphers$ = this.userId$.pipe(
    switchMap((userId) => this.cipherArchiveService.archivedCiphers$(userId)),
-  );
+  ) as Observable<CipherView[]>;


❓ Was this needed? archivedCiphers$ has a return type of Observable<CipherView[]>, I would expect that the type could be inferred.

@nick-livefront For some reason, yes. The return type wasn't being inferred correctly here. I thought the same thing. Here were the original type errors. Several related to the 'cipher' argument being passed into the handlers like this one. Very open to other possible fixes for this. This solution seemed like the least intrusive but, I don't love it either TBH

Ah, I had to restart my TypeScript and Eslint locally to see the same errors as in CI. archivedCiphers$ has a return type of Observable<CipherListView[]>. The handlers are only accepting of CipherListView, they'll need to be updated to accept CipherListView and you can then refactor to use the respective helper CipherListViewUtils method for the logic.

For the template related errors, you could assign CipherListViewUtils to the component so it can be used directly in the template:

/// class protected CipherListViewUtils = CipherListViewUtils; // template @if (CipherViewLikeUtils.hasAttachments(cipher)) { <i class="bwi bwi-paperclip bwi-sm" [appA11yTitle]="'attachments' | i18n"></i> }

Cool. I'll look into that approach instead. Will hit you up if I run into anything weird. Thanks

nick-livefront · 2025-11-12T19:18:23Z

.../src/vault/popup/settings/trash-list-items-container/trash-list-items-container.component.ts

+    if ("hasAttachments" in cipher) {
+      return cipher.hasAttachments;
+    }
+    return cipher.attachments > 0;


🎨 The CipherViewLikeUtils has a hasAttachments helper method that handles the logic between CipherListView and CipherView that can be utilized here.

hasAttachments(cipher: PopupCipherViewLike): boolean { return CipherViewLikeUtils.hasAttachments(cipher); }

@nick-livefront I'm finishing up this ticket for Bryan since he's OOO this week -- are the current changes sufficient or are you suggesting to entirely replace the contents of these methods with the helpers from CipherViewLikeUtils?

nick-livefront · 2025-11-12T19:19:45Z

.../src/vault/popup/settings/trash-list-items-container/trash-list-items-container.component.ts

+   * Get the subtitle for a cipher. CipherView has a subTitle getter,
+   * while CipherListView has a subtitle property.
+   */
+  getSubtitle(cipher: PopupCipherViewLike): string | undefined {


🎨 Parallel to the above, CipherViewLikeUtils has a getSubtitle helper method

nick-livefront · 2025-11-12T19:20:53Z

.../src/vault/popup/settings/trash-list-items-container/trash-list-items-container.component.ts

+   * Check if a cipher has a decryption failure. CipherView has this property,
+   * while CipherListView does not.
+   */
+  hasDecryptionFailure(cipher: PopupCipherViewLike): boolean {


🎨 And one more helper in CipherViewLikeUtils, decryptionFailure

nick-livefront · 2025-11-12T19:22:24Z

.../src/vault/popup/settings/trash-list-items-container/trash-list-items-container.component.ts

+    // CipherListView doesn't have decryptionFailure, so we use optional chaining
+    if ("decryptionFailure" in cipher && cipher.decryptionFailure) {


🎨 CipherViewLikeUtils.decryptionFailure can be utilized here as well.

coroiu

Not much to say from a platform perspective :)

Hinton · 2025-11-13T09:01:59Z

apps/browser/src/tools/popup/send-v2/add-edit/send-add-edit.component.html

      {{ "save" | i18n }}
    </button>
-    <button bitButton type="button" buttonType="secondary" popupBackAction>
+    <button bitButton type="button" buttonType="secondary" [popupBackAction]="undefined">


[popupBackAction]="undefined" seems very clunky. Can we just change the popupBackAction selector to not be [popupBackAction] and rather be popupBackAction which should let you use the original syntax.

@Hinton Agreed, don't love this either. I'll give that a try!

…wser-strict-template-type-check

…pe-check

claude · 2025-11-20T15:33:49Z

Claude finished @BryanCunningham's task —— View job

PR Review: Enable Strict Template Type Checking

This PR successfully enables strictTemplates in the browser extension's TypeScript config, which is a valuable improvement for type safety. I've reviewed the changes and have feedback on correctness, implementation approach, and addressing existing review comments.

Summary

The type fixes are generally correct but can be improved by consistently using the existing CipherViewLikeUtils helper methods instead of manual type checking. The PR addresses a valid need (strict template type checking) but the implementation can be more maintainable.

Findings

Finding 1: Inconsistent use of utility helpers between components (🎨)

archive.component.ts correctly uses CipherViewLikeUtils helpers in templates and component methods:

Uses CipherViewLikeUtils.hasAttachments(cipher) in template (archive.component.html:30)
Uses CipherViewLikeUtils.hasFido2Credentials(cipher) (archive.component.ts:155)
Uses CipherViewLikeUtils.decryptionFailure(cipher) (archive.component.ts:183)

However, trash-list-items-container.component.ts implements manual type checking logic (lines 91-115):

hasAttachments(cipher: PopupCipherViewLike): boolean {
  if ("hasAttachments" in cipher) {
    return cipher.hasAttachments;
  }
  return cipher.attachments > 0;
}

This duplicates logic already in CipherViewLikeUtils.hasAttachments() (libs/common/src/vault/utils/cipher-view-like-utils.ts:153-159).

Recommendation: Replace the three manual helper methods in trash-list-items-container.component.ts with direct calls to CipherViewLikeUtils:

hasAttachments() → CipherViewLikeUtils.hasAttachments()
getSubtitle() → CipherViewLikeUtils.subtitle()
hasDecryptionFailure() → CipherViewLikeUtils.decryptionFailure()

This addresses @nick-livefront's earlier review comments. Expose CipherViewLikeUtils as a protected property (like in archive.component.ts:77) to use it in templates.

Finding 2: Type assertions could be more precise (💭)

Multiple locations use as string or as unknown as CipherId for cipher IDs:

archive.component.ts:121: cipher.id as string
archive.component.ts:140: cipher.id as unknown as CipherId
trash-list-items-container.component.ts:119: cipher.id as string

The as unknown as CipherId double assertion is particularly concerning as it bypasses type safety entirely.

Question: Is there a type mismatch between CipherViewLike.id and the service methods? The CipherViewLike type appears to use string-based IDs while services expect typed GUIDs. This should be investigated to understand if there's a deeper architectural issue or if the types need alignment.

Finding 3: Addressing @vleague2's question from review comments (💭)

@vleague2 asked whether to "entirely replace the contents of these methods with the helpers from CipherViewLikeUtils."

Answer: Yes, that's exactly what should be done. The methods in trash-list-items-container.component.ts (lines 91-115) should be removed and replaced with direct calls to CipherViewLikeUtils static methods, following the pattern established in archive.component.ts.

Finding 4: Test coverage concerns (⚠️)

Codecov shows 23.8% patch coverage with 16 lines missing coverage, primarily in:

trash-list-items-container.component.ts: 10 missing lines
archive.component.ts: 6 missing lines

The new helper methods added to trash-list-items-container.component.ts lack test coverage. If these are replaced with CipherViewLikeUtils calls as recommended, the coverage concern is mitigated since those utilities should already have their own tests.

Finding 5: PR title and description quality (💭)

The PR title "CL-761 Enable strict template typechecking" is clear and specific. The objective section appropriately acknowledges uncertainty about correctness, which is good transparency. However, it would be helpful to add a brief test plan describing how the changes were verified (e.g., "Manually tested archive and trash views in browser extension to verify no runtime errors").

Additional Observations

Positive aspects:

Enabling strictTemplates is a valuable step toward better type safety
The use of CipherViewLike union type is the correct approach
Template changes in archive.component.html demonstrate proper use of utility helpers
Proper exports added to libs/vault/src/index.ts for CopyFieldAction type

No major concerns found in:

can-delete-cipher.directive.ts: Type change from CipherView to CipherViewLike is correct (line 18)
copy-cipher-field.directive.ts: Type change is appropriate (line 44)
item-copy-actions.component.ts: Return type annotation and CopyFieldAction type usage are correct improvements
copy-cipher-field.service.ts: The service already properly handles both CipherView and CipherListView via CipherViewLikeUtils

Recommendations

High Priority: Refactor trash-list-items-container.component.ts to use CipherViewLikeUtils helpers instead of manual type checking (addresses Finding 1 and @vleague2's question)
Medium Priority: Investigate the ID type assertions to understand if there's a type system issue that needs addressing (Finding 2)
Low Priority: Add brief test plan to PR description (Finding 5)

coroiu

No platform changes since last review 👍

enable strict template typechecking

ff07535

BryanCunningham changed the title ~~enable strict template typechecking~~ [CL-761] Enable strict template typechecking Nov 11, 2025

BryanCunningham added 7 commits November 11, 2025 16:10

add callout component to module

d592d24

fixing popup action types

9049cc1

fixing cipher item copy types

13da568

fix archive cipher type

aa7ef4a

fixing trash list items types

41c8892

fix remaining trash list item type errors

1b43b2e

Merge remote-tracking branch 'origin/main' into uif/CL-761/enable-bro…

8209db0

…wser-strict-template-type-check

harr1424 previously approved these changes Nov 12, 2025

View reviewed changes

nick-livefront reviewed Nov 12, 2025

View reviewed changes

coroiu previously approved these changes Nov 13, 2025

View reviewed changes

Hinton reviewed Nov 13, 2025

View reviewed changes

use CipherViewLike as correct type

d151c53

BryanCunningham dismissed stale reviews from coroiu and harr1424 via d151c53 November 13, 2025 17:17

BryanCunningham and others added 7 commits November 13, 2025 12:22

change popup back directive to attribute selector

b94c984

Merge remote-tracking branch 'origin/main' into uif/CL-761/enable-bro…

63ebe0d

…wser-strict-template-type-check

allow undefined in popupBackAction handler

0a523cf

Remove undefined from type

c69f39c

fix error with firefox commercial build

69d65ec

Merge branch 'main' into uif/CL-761/enable-browser-strict-template-ty…

e107cec

…pe-check

Merge branch 'main' into uif/CL-761/enable-browser-strict-template-ty…

1b7daca

…pe-check

vleague2 marked this pull request as ready for review November 20, 2025 15:32

vleague2 requested review from a team as code owners November 20, 2025 15:32

vleague2 requested review from dereknance and gbubemismith November 20, 2025 15:32

djsmith85 requested review from coroiu and removed request for dereknance November 20, 2025 15:32

vleague2 requested a review from nick-livefront November 20, 2025 15:37

nick-livefront approved these changes Nov 20, 2025

View reviewed changes

vleague2 removed the request for review from gbubemismith November 20, 2025 19:14

coroiu approved these changes Nov 21, 2025

View reviewed changes

		// CipherListView doesn't have decryptionFailure, so we use optional chaining
		if ("decryptionFailure" in cipher && cipher.decryptionFailure) {

[CL-761] Enable strict template typechecking #17334

Are you sure you want to change the base?

[CL-761] Enable strict template typechecking #17334

Conversation

BryanCunningham commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

codecov bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nick-livefront left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coroiu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Enable Strict Template Type Checking

Summary

Findings

Additional Observations

Recommendations

Uh oh!

coroiu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

BryanCunningham commented Nov 11, 2025 •

edited

Loading

github-actions bot commented Nov 11, 2025 •

edited

Loading

codecov bot commented Nov 11, 2025 •

edited

Loading

claude bot commented Nov 20, 2025 •

edited

Loading