Bug/595 debounced project code validation is causing trouble (#608)

* The complicated async project-code validation * Revert "The complicated async project-code validation" This reverts commit 35a88fd. * Less complicated async project-code validation * track and verify value received in debounce tests * Use derived store to implement async-debouncing --------- Co-authored-by: Kevin Hahn <[email protected]>
sillsdev · Mar 4, 2024 · 0df31e7 · 0df31e7
1 parent 8afb5e9
commit 0df31e7
Show file tree

Hide file tree

Showing 8 changed files with 191 additions and 54 deletions.
diff --git a/frontend/src/lib/forms/Input.svelte b/frontend/src/lib/forms/Input.svelte
@@ -29,12 +29,12 @@
 
 <FormField {id} {error} {label} {autofocus} {description}>
   {#if (type == 'password')}
-  <div class="container">
-    <PlainInput {id} bind:value type={currentType} {autofocus} {readonly} {error} {placeholder} {autocomplete} style="w-full" />
-    <span class="eye"><IconButton variant="btn-ghost" icon={currentType == 'password' ? 'i-mdi-eye' : 'i-mdi-eye-off'} on:click={togglePasswordVisibility} /></span>
-  </div>
+    <div class="container">
+      <PlainInput {id} bind:value on:input type={currentType} {autofocus} {readonly} {error} {placeholder} {autocomplete} style="w-full" />
+      <span class="eye"><IconButton variant="btn-ghost" icon={currentType == 'password' ? 'i-mdi-eye' : 'i-mdi-eye-off'} on:click={togglePasswordVisibility} /></span>
+    </div>
   {:else}
-    <PlainInput {id} bind:value {type} {autofocus} {readonly} {error} {placeholder} {autocomplete} />
+    <PlainInput {id} bind:value on:input {type} {autofocus} {readonly} {error} {placeholder} {autocomplete} />
   {/if}
 </FormField>
 

diff --git a/frontend/src/lib/forms/PlainInput.svelte b/frontend/src/lib/forms/PlainInput.svelte
@@ -1,6 +1,11 @@
 <script lang="ts">
   import { randomFormId } from './utils';
-  import { debounce as _debounce } from '$lib/util/time';
+  import { makeDebouncer } from '$lib/util/time';
+  import { createEventDispatcher } from 'svelte';
+
+  const dispatch = createEventDispatcher<{
+    input: string | undefined;
+  }>();
 
   let input: HTMLInputElement;
 
@@ -17,16 +22,18 @@
   export let debounce: number | boolean = false;
   export let debouncing = false;
   export let undebouncedValue: string | undefined = undefined;
-  $: undebouncedValue = value;
   export let style: string | undefined = undefined;
 
+  $: undebouncedValue = value;
+  $: dispatch('input', value);
+
   export function clear(): void {
     debouncer.clear();
     input.value = ''; // if we cancel the debounce the input and the component can get out of sync
     undebouncedValue = value = undefined;
   }
 
-  $: debouncer = _debounce((newValue: string | undefined) => (value = newValue), debounce);
+  $: debouncer = makeDebouncer((newValue: string | undefined) => (value = newValue), debounce);
   $: debouncingStore = debouncer.debouncing;
   $: debouncing = $debouncingStore;
 
@@ -44,7 +51,7 @@
   {id}
   {type}
   {value}
-  class:input-error={error}
+  class:input-error={error && error.length}
   on:input={onInput}
   {placeholder}
   class="input input-bordered {style ?? ''}"

diff --git a/frontend/src/lib/forms/utils.ts b/frontend/src/lib/forms/utils.ts
@@ -1,4 +1,3 @@
-import { browser } from '$app/environment';
 import { type Translater } from '$lib/i18n';
 import { z, type ZodDefault, type ZodType } from 'zod';
 
@@ -21,30 +20,3 @@ export function passwordFormRules($t: Translater): z.ZodString {
 export function emptyString(): z.ZodString {
   return z.string().length(0);
 }
-
-/**
- * Debouncind a refine serves 2 purposes:
- * 1) Debouncing, of course, to avoid (e.g.) making too many requests
- * 2) Ensuring the validation result is consistent. It seems that the last resolve doesn't always "win".
- * By making them all resolve to the same value, it doesn't matter which one wins.
- */
-export function debouncedRefine<T>(refine: (value: T) => Promise<boolean>, debounceTime = 300): (value: T) => Promise<boolean> {
-  // consumers don't expect debounced refines to get called server-side, but they sometimes do,
-  // which can cause problems, e.g. calling fetch with a relative path kills the server
-  if (!browser) return () => Promise.resolve(true);
-
-  let timeout: ReturnType<typeof setTimeout>;
-  let openResolves: ((value: boolean) => void)[] = [];
-  return (value: T) => {
-    clearTimeout(timeout);
-    return new Promise((resolve) => {
-      openResolves.push(resolve);
-      timeout = setTimeout(() => {
-        void refine(value).then((result) => {
-          for (const openResolve of openResolves) openResolve(result);
-          openResolves = [];
-        });
-      }, debounceTime);
-    });
-  };
-}
diff --git a/frontend/src/lib/util/array.ts b/frontend/src/lib/util/array.ts
@@ -0,0 +1,12 @@
+/**
+ * @param values any number of arrays, values or undefined's
+ * @returns a single array containing any/all the defined values from the inputs
+ */
+export function concatAll<T>(...values: (T | T[] | undefined)[]): T[] {
+  const mergedResult = [];
+  for (const value of values) {
+    if (value === undefined) continue;
+    mergedResult.push(...(Array.isArray(value) ? value : [value]));
+  }
+  return mergedResult;
+}
diff --git a/frontend/src/lib/util/time.test.ts b/frontend/src/lib/util/time.test.ts
@@ -0,0 +1,111 @@
+import { delay, deriveAsync } from './time';
+import { describe, expect, it } from 'vitest';
+
+import { writable } from 'svelte/store';
+
+const debounceTime = 100;
+const promiseTime = 50;
+
+describe('deriveAsync', () => {
+  it('handles standard synchronous debouncing', async () => {
+    let reachedPromise = false;
+    let done = false;
+    let valueReceived: number | null = null;
+    const store = writable<number>();
+    const debouncedStore = deriveAsync(
+      store,
+      // the promise resolves immediately, so we're only testing the debounce that happens before awaiting the promise
+      (value: number) => {
+        reachedPromise = true;
+        return new Promise<number>(resolve => resolve(value));
+      },
+      undefined,
+      debounceTime);
+    debouncedStore.subscribe((value) => {
+      if (value !== undefined) {
+        done = true;
+        valueReceived = value;
+      }
+    });
+
+    store.set(1);
+    expect(reachedPromise).toBe(false);
+    expect(done).toBe(false);
+
+    await delay(debounceTime * 0.75);
+    expect(reachedPromise).toBe(false);
+    expect(done).toBe(false);
+
+    store.set(2); // restart the debounce
+    await delay(debounceTime * 0.75);
+    expect(reachedPromise).toBe(false);
+    expect(done).toBe(false);
+
+    store.set(3); // restart the debounce
+    await delay(debounceTime * 0.75);
+    expect(reachedPromise).toBe(false);
+    expect(done).toBe(false);
+
+    await delay(debounceTime * 0.25);
+    expect(reachedPromise).toBe(true);
+    expect(done).toBe(true);
+    expect(valueReceived).toBe(3);
+  });
+
+  it('handles asynchronous debouncing', async () => {
+    let reachedPromise = false;
+    let done = false;
+    let valueReceived: number | null = null;
+    const store = writable<number>();
+    const debouncedStore = deriveAsync(
+      store,
+      // the promise resolves after a delay, so it can get debounced before it hits the promise or before the promise has been resolved
+      (value: number) => {
+        reachedPromise = true;
+        return new Promise<number>(resolve => {
+          setTimeout(() => resolve(value), promiseTime);
+        });
+      },
+      undefined,
+      debounceTime);
+    debouncedStore.subscribe((value) => {
+      if (value !== undefined) {
+        done = true;
+        valueReceived = value;
+      }
+    });
+
+    store.set(1);
+    expect(reachedPromise).toBe(false);
+    expect(done).toBe(false);
+
+    await delay(debounceTime * 0.75);
+    expect(reachedPromise).toBe(false);
+    expect(done).toBe(false);
+
+    store.set(2); // restart the debounce
+    await delay(debounceTime * 0.75);
+    expect(reachedPromise).toBe(false);
+    expect(done).toBe(false);
+
+    store.set(3); // restart the debounce
+    await delay(debounceTime * 0.75);
+    expect(reachedPromise).toBe(false);
+    expect(done).toBe(false);
+
+    await delay(debounceTime * 0.25);
+    expect(reachedPromise).toBe(true); // we hit the promise
+    expect(done).toBe(false); // but it will only complete if it doesn't get debounced before the promise resolves
+
+    await delay(promiseTime * 0.5);
+    expect(done).toBe(false);
+
+    store.set(4); // restart the debounce
+    await delay(promiseTime * 0.5);
+    expect(done).toBe(false);
+
+    await delay(debounceTime + promiseTime);
+    expect(done).toBe(true);
+    expect(valueReceived).toBe(4);
+  });
+});
diff --git a/frontend/src/lib/util/time.ts b/frontend/src/lib/util/time.ts
@@ -1,12 +1,12 @@
-import { writable, type Readable } from 'svelte/store';
+import { writable, type Readable, derived } from 'svelte/store';
 
 export const enum Duration {
   Default = 5000,
   Medium = 10000,
   Long = 15000,
 }
 
-export async function delay<T>(ms = Duration.Default): Promise<T> {
+export async function delay<T>(ms: Duration | number = Duration.Default): Promise<T> {
   return new Promise<T>(resolve => setTimeout(resolve, ms));
 }
 
@@ -19,14 +19,18 @@ interface Debouncer<P extends any[]> {
   clear: () => void;
 }
 
+function pickDebounceTime(debounce: number | boolean): number {
+  return typeof debounce === 'number' ? debounce : DEFAULT_DEBOUNCE_TIME;
+}
+
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
-export function debounce<P extends any[]>(fn: (...args: P) => void, debounce: number | boolean = DEFAULT_DEBOUNCE_TIME): Debouncer<P> {
+export function makeDebouncer<P extends any[]>(fn: Debouncer<P>['debounce'], debounce: number | boolean = DEFAULT_DEBOUNCE_TIME): Debouncer<P> {
   const debouncing = writable(false);
 
   if (!debounce) {
     return { debounce: fn, debouncing, clear: () => { } };
   } else {
-    const debounceTime = typeof debounce === 'number' ? debounce : DEFAULT_DEBOUNCE_TIME;
+    const debounceTime = pickDebounceTime(debounce);
     let timeout: ReturnType<typeof setTimeout>;
     return {
       debounce: (...args: P) => {
@@ -48,3 +52,28 @@ export function debounce<P extends any[]>(fn: (...args: P) => void, debounce: nu
     };
   }
 }
+
+/**
+ * @param fn A function that maps the store value to an async result
+ * @returns A store that contains the result of the async function, optionally debounced
+ */
+export function deriveAsync<T, D>(
+  store: Readable<T>,
+  fn: (value: T) => Promise<D>,
+  initialValue?: D,
+  debounce: number | boolean = false): Readable<D> {
+
+  const debounceTime = pickDebounceTime(debounce);
+  let timeout: ReturnType<typeof setTimeout> | undefined;
+
+  return derived(store, (value, set) => {
+    clearTimeout(timeout);
+    timeout = setTimeout(() => {
+      const myTimeout = timeout;
+      void fn(value).then((result) => {
+        if (myTimeout !== timeout) return; // discard outdated results
+        set(result);
+      });
+    }, debounceTime);
+  }, initialValue);
+}
diff --git a/frontend/src/routes/(authenticated)/project/create/+page.svelte b/frontend/src/routes/(authenticated)/project/create/+page.svelte
@@ -1,33 +1,28 @@
 <script lang="ts">
   import { goto } from '$app/navigation';
-  import { Checkbox, Form, FormError, Input, ProjectTypeSelect, Select, SubmitButton, TextArea, debouncedRefine, lexSuperForm } from '$lib/forms';
+  import { Checkbox, Form, FormError, Input, ProjectTypeSelect, Select, SubmitButton, TextArea, lexSuperForm } from '$lib/forms';
   import { CreateProjectResult, DbErrorCode, ProjectRole, ProjectType, RetentionPolicy, type CreateProjectInput } from '$lib/gql/types';
   import t from '$lib/i18n';
   import { TitlePage } from '$lib/layout';
   import { z } from 'zod';
   import { _createProject, _projectCodeAvailable } from './+page';
   import AdminContent from '$lib/layout/AdminContent.svelte';
   import { useNotifications } from '$lib/notify';
-  import { Duration } from '$lib/util/time';
+  import { Duration, deriveAsync } from '$lib/util/time';
   import { getSearchParamValues } from '$lib/util/query-params';
   import { isAdmin } from '$lib/user';
   import { onMount } from 'svelte';
   import MemberBadge from '$lib/components/Badges/MemberBadge.svelte';
+  import { derived, writable } from 'svelte/store';
+  import { concatAll } from '$lib/util/array';
+  import { browser } from '$app/environment';
 
   export let data;
   $: user = data.user;
   let requestingUser : typeof data.requestingUser;
 
   const { notifySuccess } = useNotifications();
 
-  let codeValidation: z.ZodType = z.string().toLowerCase()
-    .min(4, $t('project.create.code_too_short'))
-    .refine(debouncedRefine((code) =>
-      // user is not available when defining the schema
-      // || isAdmin will be redundant soon after new JWTs roll out
-      user.canCreateProjects ? _projectCodeAvailable(code) : Promise.resolve(true)),
-      $t('project.create.code_exists'));
-
   const formSchema = z.object({
     name: z.string().min(1, $t('project.create.name_missing')),
     description: z.string().min(1, $t('project.create.description_missing')),
@@ -37,9 +32,10 @@
       .string()
       .min(2, $t('project.create.language_code_too_short'))
       .regex(/^[a-z-\d]+$/, $t('project.create.language_code_invalid')),
-    code: codeValidation,
+    code: z.string().toLowerCase().min(4, $t('project.create.code_too_short')),
     customCode: z.boolean().default(false),
   });
+
   //random guid
   const projectId = crypto.randomUUID();
   let { form, errors, message, enhance, submitting } = lexSuperForm(formSchema, async () => {
@@ -68,6 +64,16 @@
       await goto('/');
     }
   });
+
+  const asyncCodeError = writable<string | undefined>();
+  const codeStore = derived(form, ($form) => $form.code);
+  const codeIsAvailable = deriveAsync(codeStore, async (code) => {
+    if (!browser || !code || !user.canCreateProjects) return true;
+    return _projectCodeAvailable(code);
+  }, true, true);
+  $: $asyncCodeError = $codeIsAvailable ? undefined : $t('project.create.code_exists');
+  const codeErrors = derived([errors, asyncCodeError], () => [...new Set(concatAll($errors.code, $asyncCodeError))]);
+
   const typeCodeMap: Partial<Record<ProjectType, string | undefined>> = {
     [ProjectType.FlEx]: 'flex',
     [ProjectType.WeSay]: 'dictionary',
@@ -177,7 +183,7 @@
     <Input
       label={$t('project.create.code')}
       bind:value={$form.code}
-      error={$errors.code}
+      error={$codeErrors}
       readonly={!$form.customCode}
     />
 

diff --git a/frontend/src/routes/(authenticated)/project/create/+page.ts b/frontend/src/routes/(authenticated)/project/create/+page.ts
@@ -60,7 +60,7 @@ export async function _createProject(input: CreateProjectInput): $OpResult<Creat
 }
 
 export async function _projectCodeAvailable(code: string): Promise<boolean> {
-  const result = await fetch(`/api/project/projectCodeAvailable/${code}`);
+  const result = await fetch(`/api/project/projectCodeAvailable/${encodeURIComponent(code)}`);
   if (!result.ok) throw new Error('Failed to check project code availability');
   return await result.json() as boolean;
 }