From dd7ee44e36636e1a7518c335fca93e1f293e7b41 Mon Sep 17 00:00:00 2001 From: MareStare Date: Wed, 19 Mar 2025 00:58:16 +0000 Subject: [PATCH 1/4] Abort the server-side completions when the autocomplete popup needs to be hidden --- assets/js/autocomplete/index.ts | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/assets/js/autocomplete/index.ts b/assets/js/autocomplete/index.ts index 3c0a885e..5dbe24a2 100644 --- a/assets/js/autocomplete/index.ts +++ b/assets/js/autocomplete/index.ts @@ -229,18 +229,23 @@ class Autocomplete { // We use this method instead of the `focusout` event because this way it's // easier to work in the developer tools when you want to inspect the element. // When you inspect it, a `focusout` happens. - this.popup.hide(); + this.hidePopup('The user clicked away'); this.input = null; } } + hidePopup(reason: string) { + this.serverSideTagSuggestions.abortLastSchedule(`[Autocomplete] Popup was hidden. ${reason}`); + this.popup.hide(); + } + onKeyDown(event: KeyboardEvent) { if (!this.isActive() || this.input.element !== event.target) { return; } if ((event.key === ',' || event.code === 'Enter') && this.input.type === 'single-tag') { - // Coma means the end of input for the current tag in single-tag mode. - this.popup.hide(); + // Comma/Enter mean the end of input for the current tag in single-tag mode. + this.hidePopup(`User accepted the existing input via key: '${event.key}', code: '${event.code}'`); return; } @@ -265,7 +270,7 @@ class Autocomplete { return; } case 'Escape': { - this.popup.hide(); + this.hidePopup('User pressed "Escape"'); return; } case 'ArrowLeft': @@ -340,7 +345,14 @@ class Autocomplete { // brief moment of silence for the user without the popup before they type // something else, otherwise we'd show some more completions for the current term. this.input.element.focus(); - this.popup.hide(); + + // Technically no server-side suggestion request can be in flight at this point. + // If the user managed to accept a suggestion, it means the user already got + // presented with the results of auto-completions, so there is nothing in-flight. + // + // Although, we don't make this a hard assertion just in case, to make sure this + // code is tolerant to any bugs in the described assumption. + this.hidePopup('The user accepted the existing suggestion'); } updateInputWithSelectedValue(this: ActiveAutocomplete, suggestion: Suggestion) { From 96d32d863de7b26a93bd4f69cdb9a5ca874bae3c Mon Sep 17 00:00:00 2001 From: MareStare Date: Wed, 19 Mar 2025 01:37:52 +0000 Subject: [PATCH 2/4] Add an integration test for aborting the completions on Escape --- .../server-side-completions-aborting.spec.ts | 34 +++++++++++++++++++ ... => server-side-completions-smoke.spec.ts} | 0 2 files changed, 34 insertions(+) create mode 100644 assets/js/autocomplete/__tests__/server-side-completions-aborting.spec.ts rename assets/js/autocomplete/__tests__/{server-side-completions.spec.ts => server-side-completions-smoke.spec.ts} (100%) diff --git a/assets/js/autocomplete/__tests__/server-side-completions-aborting.spec.ts b/assets/js/autocomplete/__tests__/server-side-completions-aborting.spec.ts new file mode 100644 index 00000000..4bf8617e --- /dev/null +++ b/assets/js/autocomplete/__tests__/server-side-completions-aborting.spec.ts @@ -0,0 +1,34 @@ +import { init } from './context'; + +it('ignores the autocompletion results if Escape was pressed', async () => { + const ctx = await init(); + + await Promise.all([ctx.setInput('mar'), ctx.keyDown('Escape')]); + + // The input must be empty because the user typed `mar` and pressed `Escape` right after that + ctx.expectUi().toMatchInlineSnapshot(` + { + "input": "mar<>", + "suggestions": [], + } + `); + + // First request for the local autocomplete index. + expect(fetch).toHaveBeenCalledTimes(1); + + await ctx.setInput('mar'); + + ctx.expectUi().toMatchInlineSnapshot(` + { + "input": "mar<>", + "suggestions": [ + "marvelous → beautiful 30", + "mare 20", + "market 10", + ], + } + `); + + // Second request for the server-side suggestions. + expect(fetch).toHaveBeenCalledTimes(2); +}); diff --git a/assets/js/autocomplete/__tests__/server-side-completions.spec.ts b/assets/js/autocomplete/__tests__/server-side-completions-smoke.spec.ts similarity index 100% rename from assets/js/autocomplete/__tests__/server-side-completions.spec.ts rename to assets/js/autocomplete/__tests__/server-side-completions-smoke.spec.ts From 57781a1aa06280e793d53ddf11148fb98aa8e75d Mon Sep 17 00:00:00 2001 From: MareStare Date: Wed, 19 Mar 2025 01:52:19 +0000 Subject: [PATCH 3/4] Add integration tests for server-side completions cancellation with Escape --- assets/js/autocomplete/__tests__/context.ts | 43 +++++------ .../server-side-completions-aborting.spec.ts | 34 --------- .../debouncing.spec.ts | 71 +++++++++++++++++++ .../smoke.spec.ts} | 2 +- 4 files changed, 95 insertions(+), 55 deletions(-) delete mode 100644 assets/js/autocomplete/__tests__/server-side-completions-aborting.spec.ts create mode 100644 assets/js/autocomplete/__tests__/server-side-completions/debouncing.spec.ts rename assets/js/autocomplete/__tests__/{server-side-completions-smoke.spec.ts => server-side-completions/smoke.spec.ts} (96%) diff --git a/assets/js/autocomplete/__tests__/context.ts b/assets/js/autocomplete/__tests__/context.ts index fabfab23..89e949eb 100644 --- a/assets/js/autocomplete/__tests__/context.ts +++ b/assets/js/autocomplete/__tests__/context.ts @@ -6,7 +6,7 @@ import { fireEvent } from '@testing-library/dom'; import { assertNotNull } from '../../utils/assert'; import { TextInputElement } from '../input'; import store from '../../utils/store'; -import { GetTagSuggestionsResponse } from 'autocomplete/client'; +import { GetTagSuggestionsResponse, TagSuggestion } from 'autocomplete/client'; /** * A reusable test environment for autocompletion tests. Note that it does no @@ -44,27 +44,30 @@ export class TestContext { } const url = new URL(request.url); - if (url.searchParams.get('term')?.toLowerCase() !== 'mar') { - const suggestions: GetTagSuggestionsResponse = { suggestions: [] }; - return JSON.stringify(suggestions); - } + const term = url.searchParams.get('term'); + + const termLower = assertNotNull(term).toLowerCase(); + + const fakeSuggestions: TagSuggestion[] = [ + { + alias: 'marvelous', + canonical: 'beautiful', + images: 30, + }, + { + canonical: 'mare', + images: 20, + }, + { + canonical: 'market', + images: 10, + }, + ]; const suggestions: GetTagSuggestionsResponse = { - suggestions: [ - { - alias: 'marvelous', - canonical: 'beautiful', - images: 30, - }, - { - canonical: 'mare', - images: 20, - }, - { - canonical: 'market', - images: 10, - }, - ], + suggestions: fakeSuggestions.filter(suggestion => + (suggestion.alias || suggestion.canonical).startsWith(termLower), + ), }; return JSON.stringify(suggestions); diff --git a/assets/js/autocomplete/__tests__/server-side-completions-aborting.spec.ts b/assets/js/autocomplete/__tests__/server-side-completions-aborting.spec.ts deleted file mode 100644 index 4bf8617e..00000000 --- a/assets/js/autocomplete/__tests__/server-side-completions-aborting.spec.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { init } from './context'; - -it('ignores the autocompletion results if Escape was pressed', async () => { - const ctx = await init(); - - await Promise.all([ctx.setInput('mar'), ctx.keyDown('Escape')]); - - // The input must be empty because the user typed `mar` and pressed `Escape` right after that - ctx.expectUi().toMatchInlineSnapshot(` - { - "input": "mar<>", - "suggestions": [], - } - `); - - // First request for the local autocomplete index. - expect(fetch).toHaveBeenCalledTimes(1); - - await ctx.setInput('mar'); - - ctx.expectUi().toMatchInlineSnapshot(` - { - "input": "mar<>", - "suggestions": [ - "marvelous → beautiful 30", - "mare 20", - "market 10", - ], - } - `); - - // Second request for the server-side suggestions. - expect(fetch).toHaveBeenCalledTimes(2); -}); diff --git a/assets/js/autocomplete/__tests__/server-side-completions/debouncing.spec.ts b/assets/js/autocomplete/__tests__/server-side-completions/debouncing.spec.ts new file mode 100644 index 00000000..f3bd94a4 --- /dev/null +++ b/assets/js/autocomplete/__tests__/server-side-completions/debouncing.spec.ts @@ -0,0 +1,71 @@ +import { init } from '../context'; + +it('ignores the autocompletion results if Escape was pressed', async () => { + const ctx = await init(); + + // First request for the local autocomplete index was done + expect(fetch).toHaveBeenCalledTimes(1); + + await Promise.all([ctx.setInput('mar'), ctx.keyDown('Escape')]); + + // The input must be empty because the user typed `mar` and pressed `Escape` right after that + ctx.expectUi().toMatchInlineSnapshot(` + { + "input": "mar<>", + "suggestions": [], + } + `); + + // No new requests must've been sent because the input was debounced early + expect(fetch).toHaveBeenCalledTimes(1); + + await ctx.setInput('mar'); + + ctx.expectUi().toMatchInlineSnapshot(` + { + "input": "mar<>", + "suggestions": [ + "marvelous → beautiful 30", + "mare 20", + "market 10", + ], + } + `); + + // Second request for the server-side suggestions. + expect(fetch).toHaveBeenCalledTimes(2); + + ctx.setInput('mare'); + + // After 300 milliseconds the debounce threshold is over, and the server-side + // completions request is issued. + vi.advanceTimersByTime(300); + + await ctx.keyDown('Escape'); + + expect(fetch).toHaveBeenCalledTimes(3); + + ctx.expectUi().toMatchInlineSnapshot(` + { + "input": "mare<>", + "suggestions": [], + } + `); + + ctx.setInput('mare'); + + // Make sure that the user gets the results immediately without any debouncing (0 ms) + await vi.advanceTimersByTimeAsync(0); + + ctx.expectUi().toMatchInlineSnapshot(` + { + "input": "mare<>", + "suggestions": [ + "mare 20", + ], + } + `); + + // The results must come from the cache, so no new fetch calls must have been made + expect(fetch).toHaveBeenCalledTimes(3); +}); diff --git a/assets/js/autocomplete/__tests__/server-side-completions-smoke.spec.ts b/assets/js/autocomplete/__tests__/server-side-completions/smoke.spec.ts similarity index 96% rename from assets/js/autocomplete/__tests__/server-side-completions-smoke.spec.ts rename to assets/js/autocomplete/__tests__/server-side-completions/smoke.spec.ts index fcf53eb7..e487b281 100644 --- a/assets/js/autocomplete/__tests__/server-side-completions-smoke.spec.ts +++ b/assets/js/autocomplete/__tests__/server-side-completions/smoke.spec.ts @@ -1,4 +1,4 @@ -import { init } from './context'; +import { init } from '../context'; it('requests server-side autocomplete if local autocomplete returns no results', async () => { const ctx = await init(); From 11c9534dbc271858215f41412a0ee7582d22e8c5 Mon Sep 17 00:00:00 2001 From: MareStare Date: Wed, 19 Mar 2025 01:59:09 +0000 Subject: [PATCH 4/4] Fix grammar --- assets/js/autocomplete/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/js/autocomplete/index.ts b/assets/js/autocomplete/index.ts index 5dbe24a2..8bffa7d0 100644 --- a/assets/js/autocomplete/index.ts +++ b/assets/js/autocomplete/index.ts @@ -245,7 +245,7 @@ class Autocomplete { } if ((event.key === ',' || event.code === 'Enter') && this.input.type === 'single-tag') { // Comma/Enter mean the end of input for the current tag in single-tag mode. - this.hidePopup(`User accepted the existing input via key: '${event.key}', code: '${event.code}'`); + this.hidePopup(`The user accepted the existing input via key: '${event.key}', code: '${event.code}'`); return; }