[PM-9111] Extension: persist add/edit form #12236

nick-livefront · 2024-12-04T02:39:10Z

🎟️ Tracking

PM-9111
Server PR (add feature flag - no logic)

📔 Objective

Use ViewCacheService to store the cipher that is being added/edited to repopulate the form when the extension loses focus and regains focus.

This stores the cipher in the signal as it is updated in the form.
- The cache is discarded when a navigation occurs which keeps users from seeing stale values.
- CipherFormCacheService is an in-between consuming components and the ViewCacheService
The main logic occurs in CipherFormComponent:
- The updatedCipherView is set to the cached cipher if it is available.
- getInitialCipherView allows each of the child components to get the cached cipher to populate their own forms with either the cached cipher or the updatedCipherView. This is a change from using originalCipherView
- Cloning also needed a flag to check so - Clone is only appended on the initial load of the cloned cipher.
Both the PM9111ExtensionPersistAddEditForm and PersistPopupView feature flag should be enabled to use.

`PopupViewCacheService.formGroup`

PopupViewCacheService has a formGroup method that will store/create a formGroup directly and update the value as the form updates. This didn't work for the cipher form as each child component registers their own individual form as they are initialized in registerChildForm. I wasn't able to successfully get this to work with the dynamic & nested form.
Along the same lines, trying to store each child form in PopupViewCacheService.formGroup would require a cache entry for each individual form group alongside repopulating which strays from how the cipher form has a singular form.
Initialization with PopupViewCacheService.formGroup didn't support FormArrays we definitely could overcome that with some logic change

📸 Screenshots

Adding Ciphers

Login	Card	Identity	Note
new-login-cached.mov	new-card-cached.mov	new-identity-cached.mov	new-note-cached.mov

Other scenarios

Editing a cipher	Cloning a cipher
edit-cipher.mov	cloned-cipher.mov

🦮 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

…ccur on non-cached values

…t/pm-9111/persist-add-edit

github-actions · 2024-12-04T02:47:30Z

Checkmarx One – Scan Summary & Details – 337f29e9-f84f-4a9b-97dc-f02b2f2907bd

No New Or Fixed Issues Found

codecov · 2024-12-04T02:49:03Z

Codecov Report

Attention: Patch coverage is 59.25926% with 33 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@997d40f). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...rc/cipher-form/components/cipher-form.component.ts	5.88%	16 Missing ⚠️
libs/vault/src/cipher-form/cipher-form.stories.ts	0.00%	7 Missing ⚠️
...her-form/components/identity/identity.component.ts	0.00%	4 Missing ⚠️
...nts/item-details/item-details-section.component.ts	69.23%	0 Missing and 4 partials ⚠️
...rm/services/popup-view-cache-background.service.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #12236   +/-   ##
=======================================
  Coverage        ?   33.58%           
=======================================
  Files           ?     2927           
  Lines           ?    91586           
  Branches        ?    17405           
=======================================
  Hits            ?    30760           
  Misses          ?    58410           
  Partials        ?     2416

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

shane-melton

I think this approach worked out nicely, great work! I just noticed a few issues and had a few small suggestions.

shane-melton · 2024-12-11T20:22:22Z

libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts

+import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
+import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
+
+const POPUP_CIPHER_CACHE_KEY = "popup-cipher-cache";


🎨 This service is used in the shared cipher form component and will be used in other clients besides Browser. I'm thinking we should make these keys client agnostic.

e.g. const CIPHER_FORM_CACHE_KEY = "cipher-form-cache";

👍 cb5d689

shane-melton · 2024-12-11T20:22:44Z

libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts

+
+@Injectable()
+export class CipherFormCacheService {
+  private popupViewCacheService: ViewCacheService = inject(ViewCacheService);


🎨 Similarly, we should just keep this as viewCacheService

👍 cb5d689

libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts

shane-melton · 2024-12-11T20:44:19Z

libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts

+      name: name ? name : (this.initialValues?.name ?? ""),
+      organizationId: prefillCipher.organizationId, // We do not allow changing ownership of an existing cipher.
+      folderId: folderId ? folderId : (this.initialValues?.folderId ?? null),
+      collectionIds: prefillCipher.collectionIds,


⚠️ The collectionIds form control is actually the type SelectItemView[] so this will cause collectionsIds to have a map of undefined values (due to the missing .id property that is referenced in the itemDetailsForm.valueChanges subscription.

We can keep this with an empty [] and let the updateCollectionOptions() call down below (which you already updated).

Updated! 7ef3a7b

shane-melton · 2024-12-11T20:46:05Z

libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts

-      this.initialValues?.collectionIds ??
-        (this.originalCipherView.collectionIds as CollectionId[]),
-    );
+    const prefillCollections = prefillCipher.collectionIds?.length


⚠️ I noticed when running this locally, the call above to itemDetailsForm.setValue() will actually update the reference to prefillCipher resulting in the loss of the prefillCipher.collectionIds value. If we pull out the collectionIds at the top of this method (with name and folderId) that should get us an unchanging copy of the ids.

Nice! Catch! This scenario totally slipped by me.

Updated! 7ef3a7b

shane-melton · 2024-12-11T20:50:53Z

libs/vault/src/cipher-form/components/cipher-form.component.ts

+      return cachedCipherView;
+    }
+
+    return this.updatedCipherView;


⚠️ I think this should be this.originalCipherView?

This one is tripping me but I agree with you. I recall switching from originalCipherView to updatedCipherView during development but I can't for the life of me remember why.

I should write better commit messages

I'm betting it was necessary for one of my earlier solutions that didn't pan out. After testing originalCipherView works just fine and makes more sense. We're rolling.

77c0c19

shane-melton · 2024-12-11T21:03:46Z

libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts

+    }
+
+    // Create a new shallow reference to force the signal to update
+    this.cipherCache.set({ ...cipherView } as CipherView);


💭 I'm slightly worried this may not be a "deep" enough / separate clone.

For example the item details and auto fill option child forms update the form values during initialization that ends up making changes to the updatedCipherView which we're caching here which in turn may be affecting subsequent child forms during initialization.

I did see the item details section having an impact which I described in my other comment about pulling the collectionIds reference out. However, when I tried stepping through the auto fill options form with multiple URIs everything appeared to be working, which makes me suspicious that we may just be getting "lucky".

I did some digging, Signals use Object.is by default, any new object would fail that equality check update the signal value. Docs.

I think your concern is legitimate and I can't really find an answer to it. If we have a deeply nested object and there is a comparison done against and updated value would it still hold the same reference.

Do you think it's worth creating a deep clone for it? structuredClone is available but the browser support may be too narrow for our liking.

For now, I added a comment about the default equality check but I'll update depending on which you think is a good path forward.

5c0a399

shane-melton · 2024-12-11T21:06:16Z

libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts

💭 We should double check that this behaves as expected in the Web client that uses a NoopViewCacheService.

I haven't seen any preserved values or anything unexpected in the web 🚀

…t/pm-9111/persist-add-edit

nick-livefront added 9 commits December 2, 2024 22:58

remove todo

83b8a0b

Retrieve cache cipher for add-edit form

69753d2

user prefilled cipher for add-edit form

7823c00

add listener for clearing view cache

feba423

clear local cache when clearing global state

fd38a8c

track initial value of cache for down stream logic that should only o…

fed1991

…ccur on non-cached values

add feature flag for edit form persistence

d85f7f4

add tests for cipher form cache service

388b1f2

Merge branch 'main' of https://github.com/bitwarden/clients into vaul…

2f59a3d

…t/pm-9111/persist-add-edit

nick-livefront requested review from a team as code owners December 4, 2024 02:39

nick-livefront requested review from justindbaur, cd-bitwarden, Jingo88, shane-melton and gbubemismith December 4, 2024 02:39

nick-livefront mentioned this pull request Dec 4, 2024

[PM-9111] Add PM9111ExtensionPersistAddEditForm feature flag bitwarden/server#5106

Merged

nick-livefront added 2 commits December 3, 2024 21:05

fix optional initialValues

3f6374f

add services to cipher form storybook

7b12559

shane-melton requested changes Dec 11, 2024

View reviewed changes

nick-livefront added 6 commits December 12, 2024 14:35

Merge branch 'main' of https://github.com/bitwarden/clients into vaul…

0720a9d

…t/pm-9111/persist-add-edit

fix strict types

00266fd

rename variables to be platform agnostic

cb5d689

use deconstructed collectionIds variable to avoid them be overwritten

7ef3a7b

use the originalCipherView for initial values

77c0c19

add comment about signal equality

5c0a399

nick-livefront requested a review from shane-melton December 13, 2024 02:16

Merge branch 'main' of https://github.com/bitwarden/clients into vaul…

6736292

…t/pm-9111/persist-add-edit

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-9111] Extension: persist add/edit form #12236

[PM-9111] Extension: persist add/edit form #12236

nick-livefront commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

shane-melton left a comment

shane-melton Dec 11, 2024

nick-livefront Dec 13, 2024

shane-melton Dec 11, 2024

nick-livefront Dec 13, 2024

shane-melton Dec 11, 2024

nick-livefront Dec 13, 2024

shane-melton Dec 11, 2024

nick-livefront Dec 13, 2024

shane-melton Dec 11, 2024

nick-livefront Dec 13, 2024

shane-melton Dec 11, 2024

nick-livefront Dec 13, 2024

nick-livefront Dec 13, 2024

shane-melton Dec 11, 2024

nick-livefront Dec 13, 2024

[PM-9111] Extension: persist add/edit form #12236

Are you sure you want to change the base?

[PM-9111] Extension: persist add/edit form #12236

Conversation

nick-livefront commented Dec 4, 2024 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

Adding Ciphers

Other scenarios

🦮 Reviewer guidelines

github-actions bot commented Dec 4, 2024 • edited Loading

No New Or Fixed Issues Found

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

shane-melton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-livefront commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading