diff --git a/ts/packages/dispatcher/dispatcher/src/helpers/completion/session.ts b/ts/packages/dispatcher/dispatcher/src/helpers/completion/session.ts index 2ca2efec7..706e39752 100644 --- a/ts/packages/dispatcher/dispatcher/src/helpers/completion/session.ts +++ b/ts/packages/dispatcher/dispatcher/src/helpers/completion/session.ts @@ -245,6 +245,20 @@ export class PartialCompletionSession implements CompletionController { input: string, direction: CompletionDirection = "forward", ): void { + // Deduplicate: if the input and direction haven't changed and + // the session is already active or pending, there is no new + // work to do. The shell's previousInput guard prevents + // selectionchange echoes from reaching here, so direction is + // safe to compare. + if ( + input === this.lastInput && + direction === this.lastDirection && + (this.completionState !== undefined || + this.completionP !== undefined) + ) { + debug(`update skipped: input unchanged ('${input}')`); + return; + } this.lastInput = input; this.lastDirection = direction; if (this.reuseSession(input, direction)) { diff --git a/ts/packages/shell/src/renderer/src/partial.ts b/ts/packages/shell/src/renderer/src/partial.ts index 54d3788f1..8f2f1b316 100644 --- a/ts/packages/shell/src/renderer/src/partial.ts +++ b/ts/packages/shell/src/renderer/src/partial.ts @@ -65,10 +65,6 @@ export class PartialCompletion { // items) should be called on the SearchMenu. private lastGeneration: number = -1; private lastPrefix: string = ""; - // Set when update(contentChanged=true) runs so the selectionchange - // handler can skip the redundant re-update that the browser fires - // after every content mutation. - private contentUpdated: boolean = false; private readonly cleanupEventListeners: () => void; constructor( @@ -96,6 +92,9 @@ export class PartialCompletion { // pass them directly to avoid a redundant trie query in render(). this.controller.setOnUpdate(() => { const state = this.controller.getCompletionState(); + debug( + `onUpdate: ${state ? `prefix='${state.prefix}' items=${state.items.length}` : "hidden"}`, + ); if (state) { if ( state.generation !== this.lastGeneration || @@ -119,10 +118,6 @@ export class PartialCompletion { }); const selectionChangeHandler = () => { - if (this.contentUpdated) { - this.contentUpdated = false; - return; - } debug("Partial completion update on selection changed"); this.update(false); }; @@ -144,20 +139,32 @@ export class PartialCompletion { if (this.closed) { throw new Error("Using a closed PartialCompletion"); } - debug(`update(contentChanged=${contentChanged})`); + debug(`update entry: contentChanged=${contentChanged}`); if (contentChanged) { // Normalize the input text to ensure selection at end is correct. this.input.getTextEntry().normalize(); - // Suppress the selectionchange event that the browser fires - // after every content mutation — this update already covers it. - this.contentUpdated = true; } if (!this.isSelectionAtEnd(contentChanged)) { + this.previousInput = ""; this.controller.hide(); + debug("update: selection not at end, hiding"); return; } const input = this.getCurrentInputForCompletion(); - debug(`Partial completion input: '${input}'`); + + // Skip if input hasn't changed since the last call we forwarded + // to the controller. This prevents selectionchange echoes from + // recomputing direction against the already-mutated previousInput + // (which would flip "backward" to "forward"). + // Reset to "" on hide (cursor moved away) so re-focus always + // re-activates the controller. + if (input === this.previousInput) { + debug(`update skipped: input unchanged ('${input}')`); + return; + } + debug( + `Partial completion input: '${input}' (${contentChanged ? "content changed" : "selection changed"})`, + ); // Only use "backward" when the user is genuinely backspacing: // the new input must be a strict prefix of the previous input. diff --git a/ts/packages/shell/test/completionUpdate.spec.ts b/ts/packages/shell/test/completionUpdate.spec.ts index 13efebabb..dd4ecb4bb 100644 --- a/ts/packages/shell/test/completionUpdate.spec.ts +++ b/ts/packages/shell/test/completionUpdate.spec.ts @@ -40,17 +40,25 @@ function collectConsoleMessages(page: Page, substring: string) { } test.describe("Partial completion update suppression", () => { - test("typing triggers exactly one update per keystroke", async () => { + test("typing triggers exactly one content-change update per keystroke", async () => { await runTestCallback(async (mainWindow: Page) => { await enablePartialDebug(mainWindow); const contentTrue = collectConsoleMessages( mainWindow, - "update(contentChanged=true)", + "content changed", ); - const contentFalse = collectConsoleMessages( + const updateEntries = collectConsoleMessages( mainWindow, - "update(contentChanged=false)", + "update entry:", + ); + const updateSkipped = collectConsoleMessages( + mainWindow, + "update skipped:", + ); + const updateHidden = collectConsoleMessages( + mainWindow, + "selection not at end", ); const input = mainWindow.locator(inputSelector); @@ -61,13 +69,23 @@ test.describe("Partial completion update suppression", () => { await expect(async () => { expect(contentTrue.length).toBe(1); }).toPass({ timeout: 2000 }); - expect(contentFalse.length).toBe(0); + + // Wait a beat for any straggling selectionchange echoes. + await mainWindow.waitForTimeout(500); + + // Every update() call must either be the content-change + // path, deduped by the previousInput guard, or hidden + // because selection moved away from the end. No call + // may slip through to recompute direction. + expect(updateEntries.length).toBe( + contentTrue.length + updateSkipped.length + updateHidden.length, + ); await mainWindow.keyboard.press("Escape"); }); }); - test("second keystroke also triggers exactly one update", async () => { + test("second keystroke also triggers exactly one content-change update", async () => { await runTestCallback(async (mainWindow: Page) => { await enablePartialDebug(mainWindow); @@ -90,11 +108,19 @@ test.describe("Partial completion update suppression", () => { // Start collecting after the first keystroke has settled. const contentTrue = collectConsoleMessages( mainWindow, - "update(contentChanged=true)", + "content changed", + ); + const updateEntries = collectConsoleMessages( + mainWindow, + "update entry:", + ); + const updateSkipped = collectConsoleMessages( + mainWindow, + "update skipped:", ); - const contentFalse = collectConsoleMessages( + const updateHidden = collectConsoleMessages( mainWindow, - "update(contentChanged=false)", + "selection not at end", ); await mainWindow.keyboard.type("l", { delay: 0 }); @@ -102,13 +128,21 @@ test.describe("Partial completion update suppression", () => { await expect(async () => { expect(contentTrue.length).toBe(1); }).toPass({ timeout: 2000 }); - expect(contentFalse.length).toBe(0); + + // Wait a beat for any straggling selectionchange echoes. + await mainWindow.waitForTimeout(500); + + // Every update() call must either be the content-change + // path, deduped by the previousInput guard, or hidden. + expect(updateEntries.length).toBe( + contentTrue.length + updateSkipped.length + updateHidden.length, + ); await mainWindow.keyboard.press("Escape"); }); }); - test("backspace triggers exactly one update", async () => { + test("backspace triggers exactly one content-change update", async () => { await runTestCallback(async (mainWindow: Page) => { await enablePartialDebug(mainWindow); @@ -131,11 +165,19 @@ test.describe("Partial completion update suppression", () => { // Start collecting after the initial typing has settled. const contentTrue = collectConsoleMessages( mainWindow, - "update(contentChanged=true)", + "content changed", + ); + const updateEntries = collectConsoleMessages( + mainWindow, + "update entry:", ); - const contentFalse = collectConsoleMessages( + const updateSkipped = collectConsoleMessages( mainWindow, - "update(contentChanged=false)", + "update skipped:", + ); + const updateHidden = collectConsoleMessages( + mainWindow, + "selection not at end", ); await mainWindow.keyboard.press("Backspace"); @@ -143,7 +185,15 @@ test.describe("Partial completion update suppression", () => { await expect(async () => { expect(contentTrue.length).toBe(1); }).toPass({ timeout: 2000 }); - expect(contentFalse.length).toBe(0); + + // Wait a beat for any straggling selectionchange echoes. + await mainWindow.waitForTimeout(500); + + // Every update() call must either be the content-change + // path, deduped by the previousInput guard, or hidden. + expect(updateEntries.length).toBe( + contentTrue.length + updateSkipped.length + updateHidden.length, + ); await mainWindow.keyboard.press("Escape"); }); diff --git a/ts/packages/shell/test/partialCompletion/renderDispatch.spec.ts b/ts/packages/shell/test/partialCompletion/renderDispatch.spec.ts index 7b43fca60..46d2e25a2 100644 --- a/ts/packages/shell/test/partialCompletion/renderDispatch.spec.ts +++ b/ts/packages/shell/test/partialCompletion/renderDispatch.spec.ts @@ -86,7 +86,7 @@ describe("render vs updatePosition dispatch", () => { expect(menu.updatePositionCalls.length).toBe(0); }); - test("same generation + same prefix calls updatePosition", async () => { + test("same input + direction is deduplicated (no callback)", async () => { const result = makeCompletionResult(["song", "shuffle"], 4, { separatorMode: "optionalSpace", }); @@ -99,10 +99,32 @@ describe("render vs updatePosition dispatch", () => { await Promise.resolve(); const rendersBefore = menu.renderCalls.length; + const updatePosBefore = menu.updatePositionCalls.length; - // Same input — same generation, same prefix. + // Same input, same direction — controller skips entirely. controller.update("play", "forward"); + expect(menu.renderCalls.length).toBe(rendersBefore); + expect(menu.updatePositionCalls.length).toBe(updatePosBefore); + }); + + test("same input + different direction calls updatePosition", async () => { + const result = makeCompletionResult(["song", "shuffle"], 4, { + separatorMode: "optionalSpace", + }); + const dispatcher = makeDispatcher(result); + const controller = createCompletionController(dispatcher); + const menu = new MockSearchMenu(); + wireController(controller, menu); + + controller.update("play", "forward"); + await Promise.resolve(); + + const rendersBefore = menu.renderCalls.length; + + // Same input, different direction — passes dedup, reuse triggers updatePosition. + controller.update("play", "backward"); + expect(menu.renderCalls.length).toBe(rendersBefore); expect(menu.updatePositionCalls.length).toBe(1); });