HTML constraints validation rises like a phoenix

bitwarden · Oct 31, 2024 · 6832b43 · 6832b43
1 parent 42be5db
commit 6832b43
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 65 deletions.
diff --git a/libs/tools/generator/components/src/credential-generator.component.ts b/libs/tools/generator/components/src/credential-generator.component.ts
@@ -511,7 +511,7 @@ export class CredentialGeneratorComponent implements OnInit, OnDestroy {
    */
   protected async generate(requestor: string) {
     if (this.passphraseSettings) {
-      await this.passphraseSettings.reloadSettings("credential generator");
+      await this.passphraseSettings.save("credential generator");
     }
 
     this.generate$.next(requestor);

diff --git a/libs/tools/generator/components/src/passphrase-settings.component.html b/libs/tools/generator/components/src/passphrase-settings.component.html
@@ -12,7 +12,9 @@ <h6 bitTypography="h6">{{ "options" | i18n }}</h6>
             formControlName="numWords"
             id="num-words"
             type="number"
-            (focusout)="reloadSettings('numWords')"
+            [min]="minNumWords"
+            [max]="maxNumWords"
+            (change)="save('numWords')"
           />
           <bit-hint>{{ numWordsBoundariesHint$ | async }}</bit-hint>
         </bit-form-field>
@@ -27,15 +29,18 @@ <h6 bitTypography="h6">{{ "options" | i18n }}</h6>
             formControlName="wordSeparator"
             id="word-separator"
             type="text"
-            (focusout)="reloadSettings('wordSeparator')"
+            [maxlength]="wordSeparatorMaxLength"
+            (change)="save('wordSeparator')"
           />
         </bit-form-field>
         <bit-form-control>
-          <input bitCheckbox formControlName="capitalize" id="capitalize" type="checkbox" />
+          <input bitCheckbox formControlName="capitalize" id="capitalize" type="checkbox"
+            (change)="save('capitalize')" />
           <bit-label>{{ "capitalize" | i18n }}</bit-label>
         </bit-form-control>
         <bit-form-control [disableMargin]="!policyInEffect">
-          <input bitCheckbox formControlName="includeNumber" id="include-number" type="checkbox" />
+          <input bitCheckbox formControlName="includeNumber" id="include-number" type="checkbox"
+          (change)="save('includeNumber')" />
           <bit-label>{{ "includeNumber" | i18n }}</bit-label>
         </bit-form-control>
         <p *ngIf="policyInEffect" bitTypography="helper">{{ "generatorPolicyInEffect" | i18n }}</p>

diff --git a/libs/tools/generator/components/src/passphrase-settings.component.ts b/libs/tools/generator/components/src/passphrase-settings.component.ts
@@ -9,9 +9,6 @@ import {
   filter,
   map,
   withLatestFrom,
-  Observable,
-  merge,
-  firstValueFrom,
   ReplaySubject,
   tap,
 } from "rxjs";
@@ -25,7 +22,7 @@ import {
   PassphraseGenerationOptions,
 } from "@bitwarden/generator-core";
 
-import { completeOnAccountSwitch, toValidators } from "./util";
+import { completeOnAccountSwitch } from "./util";
 
 const Controls = Object.freeze({
   numWords: "numWords",
@@ -106,16 +103,14 @@ export class PassphraseSettingsComponent implements OnInit, OnDestroy {
       .policy$(Generators.passphrase, { userId$: singleUserId$ })
       .pipe(takeUntil(this.destroyed$))
       .subscribe(({ constraints }) => {
-        this.settings
-          .get(Controls.numWords)
-          .setValidators(toValidators(Controls.numWords, Generators.passphrase, constraints));
-
-        this.settings
-          .get(Controls.wordSeparator)
-          .setValidators(toValidators(Controls.wordSeparator, Generators.passphrase, constraints));
-
-        this.settings.updateValueAndValidity({ emitEvent: false });
-
+        // reactive form validation doesn't work well with the generator's
+        // "auto-fix invalid data" feature. HTML constraints are used to
+        // improve usability. This approach causes `valueChanges` to fire
+        // *every time* this subscription fires. Take care not to leak these
+        // false emissions from the `onUpdated` event.
+        this.minNumWords = constraints.numWords.min;
+        this.maxNumWords = constraints.numWords.max;
+        this.wordSeparatorMaxLength = constraints.wordSeparator.maxLength;
         this.policyInEffect = constraints.policyInEffect;
 
         this.toggleEnabled(Controls.capitalize, !constraints.capitalize?.readonly);
@@ -130,64 +125,33 @@ export class PassphraseSettingsComponent implements OnInit, OnDestroy {
       });
 
     // now that outputs are set up, connect inputs
-    this.settings$().pipe(takeUntil(this.destroyed$)).subscribe(settings);
-  }
-
-  protected settings$(): Observable<Partial<PassphraseGenerationOptions>> {
-    // save valid changes
-    const validChanges$ = this.settings.statusChanges.pipe(
-      filter((status) => status === "VALID"),
+    this.saveSettings.pipe(
       withLatestFrom(this.settings.valueChanges),
+      tap(([requestor, value]) => console.log(`save request from ${requestor}: ${JSON.stringify(value)}`)),
       map(([, settings]) => settings),
-      tap((value) => console.log(`valid change: ${JSON.stringify(value)}`))
-    );
+      takeUntil(this.destroyed$)
+    ).subscribe(settings);
+  }
 
-    // discards changes but keep the override setting that changed
-    const overrides = [Controls.capitalize, Controls.includeNumber];
-    const overrideChanges$ = this.settings.valueChanges.pipe(
-      filter((settings) => !!settings),
-      withLatestFrom(this.okSettings$),
-      filter(([current, ok]) => overrides.some((c) => (current[c] ?? ok[c]) !== ok[c])),
-      map(([current, ok]) => {
-        const copy = { ...ok };
-        for (const override of overrides) {
-          copy[override] = current[override];
-        }
-        return copy;
-      }),
-      tap((value) => console.log(`override: ${JSON.stringify(value)}`))
-    );
+  /** attribute binding for numWords[min] */
+  protected minNumWords: number;
 
-    // save reloaded settings when requested
-    const reloadChanges$ = this.reloadSettings$.pipe(
-      withLatestFrom(this.okSettings$),
-      map(([, settings]) => settings),
-      tap((value) => console.log(`reload: ${JSON.stringify(value)}`))
-    );
+  /** attribute binding for numWords[max] */
+  protected maxNumWords: number;
 
-    return merge(validChanges$, overrideChanges$, reloadChanges$);
+  /** attribute binding for wordSeparator[maxlength] */
+  protected wordSeparatorMaxLength: number;
+
+  private saveSettings = new Subject<string>();
+  save(site: string = "component api call") {
+    this.saveSettings.next(site);
   }
 
   /** display binding for enterprise policy notice */
   protected policyInEffect: boolean;
 
   private okSettings$ = new ReplaySubject<PassphraseGenerationOptions>(1);
 
-  private reloadSettings$ = new Subject<string>();
-
-  /** triggers a reload of the users' settings
-   *  @param site labels the invocation site so that an operation
-   *   can be traced back to its origin. Useful for debugging rxjs.
-   *  @returns a promise that completes once a reload occurs.
-   */
-  async reloadSettings(site: string = "component api call") {
-    const reloadComplete = firstValueFrom(this.okSettings$);
-    if (this.settings.invalid) {
-      this.reloadSettings$.next(site);
-      await reloadComplete;
-    }
-  }
-
   private numWordsBoundariesHint = new ReplaySubject<string>(1);
 
   /** display binding for min/max constraints of `numWords` */