diff --git a/src/chatbots/ClaudeVoiceMenu.ts b/src/chatbots/ClaudeVoiceMenu.ts index 5ae2a6b4b1..672153f09b 100644 --- a/src/chatbots/ClaudeVoiceMenu.ts +++ b/src/chatbots/ClaudeVoiceMenu.ts @@ -725,13 +725,19 @@ export class ClaudeVoiceMenu extends VoiceSelector { } /** - * The dropdown's single render path: rebuild the trigger button + menu + * The dropdown's single render path: paint the trigger button + menu * content from the given catalog and stored voice. The stored voice arrives * WITH the catalog (gather-then-render), so the trigger label is right on - * the first paint and curateShortlist pins the current voice synchronously - * — the old read-selection-from-the-button-label block and the async - * getVoice fallback (with its pin writeback + conditional re-populate) are - * gone. + * the first paint and curateShortlist pins the current voice synchronously. + * + * The trigger and menu container are STABLE NODES: created once, then + * updated/refilled in place. Rebuilding them here is not safe — every open + * runs this via refreshMenu, whose fetches resolve from warm in-memory + * caches in microtasks, i.e. (for a real, trusted click) BETWEEN the + * trigger's click listener and the document-level outside-click listener. + * A rebuild detaches the click's event.target, so the outside-click check + * `this.element.contains(event.target)` reads the opening click as + * "outside" and closes the menu in the same tick it opened (#494). */ protected override renderMenu( voices: SpeechSynthesisVoiceRemote[], @@ -763,21 +769,27 @@ export class ClaudeVoiceMenu extends VoiceSelector { ); this.showTier = curated.tiersCoexist; - // Comprehensive cleanup to prevent duplicates - this.cleanupExistingElements(voiceSelector); - // Check if we have any voices available besides "Voice off" const noVoicesAvailable = voices.length === 0; + const requiresSignIn = this.ttsRequiresSignIn(noVoicesAvailable); + + // First render creates the nodes; every later render reuses them. + // (Duplicate defense — cleanupExistingElements — runs once, from the + // constructor, where a fresh instance may inherit a prior instance's + // leftovers.) + if (!this.menuButton.isConnected) { + this.menuButton = this.createVoiceButton(storedVoice, requiresSignIn); + voiceSelector.appendChild(this.menuButton); + } + this.updateSignInAffordance(requiresSignIn); - // Recreate the menu button and content from scratch - this.menuButton = this.createVoiceButton( - storedVoice, - this.ttsRequiresSignIn(noVoicesAvailable) - ); - voiceSelector.appendChild(this.menuButton); - - this.menuContent = this.createVoiceMenu(); - voiceSelector.appendChild(this.menuContent); + if (!this.menuContent.isConnected) { + this.menuContent = this.createVoiceMenu(); + voiceSelector.appendChild(this.menuContent); + } + // Refill in place — the container keeps its identity and, when open, its + // place in document.body and its visibility. + this.menuContent.replaceChildren(); // Add "Voice off" option with appropriate messaging const voiceOffItem = this.createMenuItem(null, noVoicesAvailable); @@ -817,6 +829,22 @@ export class ClaudeVoiceMenu extends VoiceSelector { this.updateSelectedVoice(storedVoice); } + /** + * In-place counterpart of createVoiceButton's requiresSignIn branch, for + * re-renders that reuse the trigger node (see that branch for the + * accessibility rationale). + */ + private updateSignInAffordance(requiresSignIn: boolean): void { + this.menuButton.classList.toggle("saypi-voice-unavailable", requiresSignIn); + if (requiresSignIn) { + this.menuButton.setAttribute("aria-label", getMessage("signInForTTS")); + this.menuButton.setAttribute("title", getMessage("signInForTTS")); + } else { + this.menuButton.removeAttribute("aria-label"); + this.menuButton.removeAttribute("title"); + } + } + /** * The muted final row linking to the full voice catalog in the extension * settings (Voices tab). Always rendered — it is the menu's path to the diff --git a/test/chatbots/ClaudeVoiceMenu-open-race.spec.ts b/test/chatbots/ClaudeVoiceMenu-open-race.spec.ts new file mode 100644 index 0000000000..c8a043cd98 --- /dev/null +++ b/test/chatbots/ClaudeVoiceMenu-open-race.spec.ts @@ -0,0 +1,116 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// ConfigModule reads injected env at import time; stub it (mirrors other specs). +vi.mock("../../src/ConfigModule", () => ({ + config: { + appServerUrl: "https://app.example.com", + apiServerUrl: "https://api.saypi.ai", + GA_MEASUREMENT_ID: "x", + GA_API_SECRET: "x", + GA_ENDPOINT: "x", + }, +})); + +vi.mock("../../src/JwtManager", () => ({ + getJwtManagerSync: () => ({ + isAuthenticated: () => true, + getClaims: () => ({ ttsQuotaRemaining: 1000 }), + }), +})); + +vi.mock("../../src/popup/popupopener", () => ({ + openSettings: vi.fn(), +})); + +import { ClaudeVoiceMenu } from "../../src/chatbots/ClaudeVoiceMenu"; +import { claudeMockVoices } from "../data/Voices"; + +// Bypass the heavy constructor; renderMenu only needs prototype methods +// plus the fields it reads (pattern from ClaudeVoiceMenu-curation.spec.ts). +function makeMenu(): any { + const menu = Object.create(ClaudeVoiceMenu.prototype); + menu.chatbot = {} as any; + menu.userPreferences = { + getVoice: vi.fn(async () => null), + setVoice: vi.fn(async () => {}), + unsetVoice: vi.fn(async () => {}), + }; + menu.element = document.createElement("div"); + menu.element.id = "claude-voice-selector"; + document.body.appendChild(menu.element); + menu.menuButton = document.createElement("button"); + menu.menuContent = document.createElement("div"); + return menu; +} + +beforeEach(() => { + document.body.innerHTML = ""; +}); + +// Regression guard for the open-click self-close race (#494, introduced by +// #492): every open runs refreshMenu → renderMenu. With warm in-memory +// caches, that resolves in microtasks — which a REAL (trusted) click lets +// run BETWEEN the trigger's own click listener and the document-level +// outside-click listener. If renderMenu rebuilds the trigger, the click's +// event.target is a detached node by the time the document listener checks +// `this.element.contains(event.target)`, so it closes the menu in the same +// tick it opened. The invariant that makes the race harmless: renderMenu +// updates the existing trigger/menu nodes in place — it never replaces them. +describe("ClaudeVoiceMenu re-render node stability (open-click race)", () => { + it("keeps the same trigger button node across re-renders", () => { + const menu = makeMenu(); + menu.renderMenu(claudeMockVoices, null); + const trigger = menu.element.querySelector( + "button[data-voice-active]" + ) as HTMLButtonElement; + expect(trigger).not.toBeNull(); + + // A second render — exactly what an open-click triggers via refreshMenu. + menu.renderMenu(claudeMockVoices, null); + + expect(menu.menuButton).toBe(trigger); + expect(trigger.isConnected).toBe(true); + expect( + menu.element.querySelectorAll("button[data-voice-active]").length + ).toBe(1); + }); + + it("keeps the same menu container node, and an open menu stays open in body", () => { + const menu = makeMenu(); + menu.renderMenu(claudeMockVoices, null); + const content = menu.menuContent as HTMLDivElement; + + // Simulate the menu being open: parked in document.body, visible + // (positionMenuAboveButton's arrangement). + document.body.appendChild(content); + content.style.display = "block"; + + // A re-render lands while open (mid-click rebuild, or an auth/preference + // refresh). It must refill the SAME node, not tear it down. + const lucy = claudeMockVoices.find((v) => v.name === "Lucy")!; + menu.renderMenu(claudeMockVoices, lucy); + + expect(menu.menuContent).toBe(content); + expect(content.parentElement).toBe(document.body); + expect(content.style.display).toBe("block"); + // …and the refill really happened: the stored voice is now checked. + const lucyRow = content.querySelector( + "[data-voice-name='Lucy'] .checkmark-container" + ); + expect(lucyRow?.innerHTML).toContain("svg"); + }); + + it("updates the trigger label and sign-in affordance in place", () => { + const menu = makeMenu(); + menu.renderMenu(claudeMockVoices, null); + const trigger = menu.element.querySelector( + "button[data-voice-active]" + ) as HTMLButtonElement; + + const lucy = claudeMockVoices.find((v) => v.name === "Lucy")!; + menu.renderMenu(claudeMockVoices, lucy); + expect(menu.element.querySelector(".voice-name")?.textContent).toBe("Lucy"); + expect(trigger.getAttribute("data-voice-active")).toBe("true"); + expect(trigger.classList.contains("saypi-voice-unavailable")).toBe(false); + }); +});