From 1f9f8b0b6996c2b07491dac4ed48c89039ffdde8 Mon Sep 17 00:00:00 2001 From: Ross Cadogan Date: Sat, 4 Jul 2026 18:27:21 +0100 Subject: [PATCH 1/6] docs(voices): implementation plan for the voice-menu row/model refactor Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Y1oVsdBKdATjojxQprFhR4 --- .../2026-07-04-voice-menu-row-refactor.md | 1064 +++++++++++++++++ 1 file changed, 1064 insertions(+) create mode 100644 doc/plans/2026-07-04-voice-menu-row-refactor.md diff --git a/doc/plans/2026-07-04-voice-menu-row-refactor.md b/doc/plans/2026-07-04-voice-menu-row-refactor.md new file mode 100644 index 0000000000..22ed10ac76 --- /dev/null +++ b/doc/plans/2026-07-04-voice-menu-row-refactor.md @@ -0,0 +1,1064 @@ +# Voice Menu Row/Model Refactor Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Restructure the `VoiceSelector` base (src/tts/VoiceMenu.ts) and its host subclasses so a voice row is a first-class unit with positive classification, selection/pin state is derived from the stored preference (not recovered from the DOM), and there is one idempotent render path — making the next menu affordance (#483's ▶ on Pi, per-voice delete, etc.) a JSDOM-unit-testable afternoon instead of a real-host-verified half-day. + +**Architecture:** Split the current three-jobs-in-one base into (1) a slim generic `VoiceSelector` (auth/preference wiring + a gather-then-render `refreshMenu`), (2) a new `GridVoiceSelector` layer owning the button-grid rendering for Pi's two surfaces (row factory, reconciler, host-row adoption, selection render), and (3) a tiny `VoiceMenuControls` taxonomy module (positive, exhaustive classification of every interactive element). `ClaudeVoiceMenu` migrates to the slim base contract (renderMenu receives the stored voice; the pin field and the read-selection-from-button-label fallback die) but its rendering is NOT redesigned. `VoiceCuration.ts` is untouched. No Preact — host-injected widgets stay imperative per doc/preact-component-conventions.md. + +**Tech Stack:** TypeScript, Vitest + JSDOM (existing harness style: `vi.mock` for ConfigModule/JwtManager/i18n/EventBus/SpeechSynthesisModule; `Object.create(Proto)` to bypass heavy constructors). + +**Worktree:** `.worktrees/voice-menu-row-refactor` (branch `refactor/voice-menu-rows`). ALL file paths below are relative to the worktree root — never edit the main checkout (memory: worktree-edit-path-trap). + +--- + +## Design invariants this refactor must not drop (test-pinned) + +1. **Current-voice-never-vanishes:** the stored voice always renders on a capped surface. Mechanism changes from async pin-recursion to passing the stored id into `curateShortlist` (which already pins current first) at every render. +2. **Never a silent swap / stale mark:** selection marks are a pure render of the stored preference. *Deliberate improvement:* when the stored voice is not visible in the menu, NO row is marked (today a stale row can keep its highlight). +3. **Deprecated grandfathering:** `curateShortlist`/`visibleCatalog` behavior unchanged (file untouched); assert through the render. +4. **Unknown elements are inert:** a nested ▶ (or any future control) is never adopted, never counted as a Pi voice, never given a select handler, never torn down by a re-render (#485, the ▶-deselects trap). +5. **Pi7/Pi8 extras survive re-renders; pins are not wiped** (the Patch A regression class). +6. **Door: exactly one, after the SayPi block, always present on surfaces that show it (#472).** +7. **Preserved quirks (behavior-preserving on purpose):** extras render with today's appearance (custom-voice class + flair, inserted at top); the uncapped Pi settings grid does not filter deprecated voices (pre-existing gap, not this refactor's job); `unmarkRowSelected` still wipes inline styles (`setAttribute("style","")`) exactly as today. + +## Element taxonomy (the positive classification) + +| Kind | Marker | Meaning | +|---|---|---| +| `custom-voice` | `.saypi-custom-voice` | SayPi catalog voice row (select target) | +| `restored-voice` | `.saypi-restored-voice` | default-flagged catalog voice row SayPi re-adds | +| `door` | `.saypi-more-voices` | "More voices" → settings catalog | +| `preview` | `.saypi-voice-preview` | ▶ free-sample playback (#482) | +| `host-voice` | `data-saypi-host-voice` | host-native button ADOPTED at the single adoption site | +| `unknown` | none of the above | **inert** — SayPi code never acts on it | + +Adoption (the only place unmarked elements are touched): direct-child ` - button.type = "button"; - const additionalClasses = ["saypi-voice-button", "saypi-restored-voice"]; - const combinedClasses = [ - ...this.getButtonClasses(), - ...additionalClasses, - voice.name.toLowerCase().replace(" ", "-"), - ]; - button.classList.add(...combinedClasses); - button.innerText = voice.name; - button.addEventListener("click", () => { - this.userPreferences.setVoice(voice, this.chatbot).then(() => { - console.log(`Selected voice: ${voice.name}`); - defaultVoiceButtons.forEach((button) => { - this.unmarkButtonAsSelectedVoice(button); - }); - const voiceButtons = voiceSelector.querySelectorAll("button"); - voiceButtons.forEach((button) => { - this.unmarkButtonAsSelectedVoice(button as HTMLButtonElement); - }); - this.markButtonAsSelectedVoice(button); - this.introduceVoice(voice); - }); - }); - button.dataset.voiceId = voice.id; - defaultVoiceButtons.push(button); - }); - - defaultVoiceButtons.forEach((button) => { - voiceSelector.appendChild(button); - }); - } - - populateCustomVoices( - customVoices: SpeechSynthesisVoiceRemote[], - voiceSelector: HTMLElement, - showTier: boolean = false - ): void { - const customVoiceButtons = Array(customVoices.length); - - customVoices.forEach((voice) => { - // if not already in the menu, add the voice - if (voiceSelector.querySelector(`button[data-voice-id="${voice.id}"]`)) { - // voice already in menu, skip to next voice - return; - } - const button = document.createElement("button"); - // template: - button.type = "button"; - const additionalClasses = ["saypi-voice-button", "saypi-custom-voice"]; - const combinedClasses = [ - ...this.getButtonClasses(), - ...additionalClasses, - voice.name.toLowerCase().replace(" ", "-"), - ]; - button.classList.add(...combinedClasses); - const name = document.createElement("span"); - name.classList.add("voice-name"); - name.innerText = voice.name; - button.appendChild(name); - // Quiet tier suffix on premium rows, only while tiers coexist - // (doc/plans/2026-07-02-voice-selection-ux.md §3 — no chips, no prices on Pi). - if (showTier && getVoiceTier(voice) === "hd") { - const tier = document.createElement("span"); - tier.classList.add("voice-tier"); - tier.textContent = "HD"; - tier.title = getMessage("hdVoicesAllowanceNote"); - button.appendChild(tier); - } - const flair = document.createElement("img"); - flair.classList.add("flair"); - flair.src = getResourceUrl("icons/logos/saypi.png"); - flair.alt = "Say, Pi logo"; - flair.title = getMessage("enhancedVoice", ["Say, Pi"]); - button.appendChild(flair); - button.addEventListener("click", () => { - this.userPreferences.setVoice(voice, this.chatbot).then(() => { - console.log(`Selected voice: ${voice.name}`); - customVoiceButtons.forEach((button) => { - this.unmarkButtonAsSelectedVoice(button); - }); - const voiceButtons = voiceSelector.querySelectorAll("button"); - voiceButtons.forEach((button) => { - if (this.isBuiltInVoiceButton(button as HTMLButtonElement)) { - this.unmarkButtonAsSelectedVoice(button as HTMLButtonElement); - } - }); - this.markButtonAsSelectedVoice(button); - this.introduceVoice(voice); - }); - }); - button.dataset.voiceId = voice.id; - customVoiceButtons.push(button); - }); - - customVoiceButtons.reverse().forEach((button) => { - voiceSelector.insertBefore(button, voiceSelector.firstChild); - }); - } introduceVoice(voice: SpeechSynthesisVoiceRemote): void { // Prefer the free, server-served canned clip (design §4) whenever the catalog @@ -557,71 +222,6 @@ export abstract class VoiceSelector { }); } - // Listen for additions of custom voice buttons and update selections - addVoiceButtonAdditionListener(voiceMenu: HTMLElement): void { - const observerCallback = ( - mutationsList: MutationRecord[], - observer: MutationObserver - ) => { - for (let mutation of mutationsList) { - if (mutation.type === "childList") { - for (let node of mutation.addedNodes) { - if ( - node.nodeName === "BUTTON" && - node instanceof HTMLButtonElement - ) { - const button = node as HTMLButtonElement; - this.handleButtonAddition(button); - } - } - } - } - }; - const observer = new MutationObserver(observerCallback); - observer.observe(voiceMenu, { childList: true }); - } - - handleButtonAddition(button: HTMLButtonElement): void { - // a voice button was added to the menu that is not a custom voice button - // if a voice is selected, mark the button as selected - this.userPreferences.getVoice(this.chatbot).then((voice) => { - const customVoiceIsSelected = voice !== null; - if (customVoiceIsSelected) { - if (this.isBuiltInVoiceButton(button)) { - this.unmarkButtonAsSelectedVoice(button); - } else if (button.dataset.voiceId === voice.id) { - // unmark all other buttons and mark this one as selected - const voiceButtons = Array.from( - this.element.querySelectorAll("button") - ); - voiceButtons.forEach((btn) => { - this.unmarkButtonAsSelectedVoice(btn as HTMLButtonElement); - }); - this.markButtonAsSelectedVoice(button); - } - } - }); - } - - addMissingPiVoices(voiceSelector: HTMLElement) { - // only for chatbots that ship their own built-in voices (Pi.ai) - if (!isBuiltInVoiceProvider(this.chatbot)) { - return; - } - // count the number of original Pi voices in the menu - let piVoices = 0; - const voiceButtons = Array.from(voiceSelector.querySelectorAll("button")); - voiceButtons.forEach((button) => { - if (this.isBuiltInVoiceButton(button as HTMLButtonElement)) { - piVoices++; - } - }); - // if fewer than 8 Pi voices, add the missing Pi voices to the menu - if (piVoices < 8) { - this.populateVoices(this.chatbot.getExtraVoices(), voiceSelector); - } - } - /** * Returns the position from the end where the voice menu should be inserted in its parent container * @returns 0: append at the end of the container diff --git a/test/tts/GridVoiceSelector.spec.ts b/test/tts/GridVoiceSelector.spec.ts new file mode 100644 index 0000000000..04b3f8908d --- /dev/null +++ b/test/tts/GridVoiceSelector.spec.ts @@ -0,0 +1,276 @@ +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: () => null, + }), +})); +const openSettingsMock = vi.fn(); +vi.mock("../../src/popup/popupopener", () => ({ + openSettings: (...args: unknown[]) => openSettingsMock(...args), +})); +vi.mock("../../src/i18n", () => ({ default: (key: string) => key })); +vi.mock("../../src/events/EventBus", () => ({ + default: { emit: vi.fn(), on: vi.fn(), off: vi.fn() }, +})); + +import { GridVoiceSelector } from "../../src/tts/GridVoiceSelector"; +import { HOST_VOICE_ATTR } from "../../src/tts/VoiceMenuControls"; +import { ElevenLabsVoice, OpenAIVoice, openAiMockVoices } from "../data/Voices"; +import { SpeechSynthesisVoiceRemote } from "../../src/tts/SpeechModel"; + +class TestGrid extends GridVoiceSelector { + cap: number | null = 5; + getId() { + return "test-grid"; + } + getButtonClasses() { + return ["mb-1"]; + } + protected override getCustomVoiceCap() { + return this.cap; + } +} + +const piCustoms = [ + new ElevenLabsVoice("ig1TeITnnNlsJtfHxJlW", "Paola"), + new ElevenLabsVoice("bWJPewAagbymiJXZcxnh", "Joey"), + new ElevenLabsVoice("paola-v3", "Paola", "F"), + ...openAiMockVoices, +]; // 13 custom voices, HD + everyday tiers coexist + +function extraVoice(id: string, name: string): SpeechSynthesisVoiceRemote { + return new OpenAIVoice(id, name); +} + +function makeGrid( + opts: { + stored?: SpeechSynthesisVoiceRemote | null; + provider?: boolean; + cap?: number | null; + } = {} +): { grid: any; el: HTMLElement } { + const grid: any = Object.create(TestGrid.prototype); + grid.cap = opts.cap === undefined ? 5 : opts.cap; + grid.chatbot = opts.provider + ? { + getID: () => "pi", + getExtraVoices: () => [ + extraVoice("voice7", "Pi 7"), + extraVoice("voice8", "Pi 8"), + ], + getVoiceIntroductionUrl: () => "https://pi.ai/intro.mp3", + } + : { getID: () => "pi" }; + grid.userPreferences = { + getVoice: vi.fn(async () => opts.stored ?? null), + setVoice: vi.fn(async () => {}), + unsetVoice: vi.fn(async () => {}), + }; + grid.element = document.createElement("div"); + grid.introduceVoice = vi.fn(); + return { grid, el: grid.element }; +} + +const rowsOf = (el: HTMLElement) => + Array.from(el.querySelectorAll(".saypi-custom-voice")); +const idsOf = (el: HTMLElement) => rowsOf(el).map((r) => r.dataset.voiceId); + +beforeEach(() => { + openSettingsMock.mockReset(); + document.body.innerHTML = ""; +}); + +describe("GridVoiceSelector.renderMenu — one idempotent render path", () => { + it("caps SayPi rows and always renders exactly one door", () => { + const { grid, el } = makeGrid(); + grid.renderMenu(piCustoms, null); + expect(rowsOf(el).length).toBe(5); + expect(el.querySelectorAll(".saypi-more-voices").length).toBe(1); + }); + + it("is idempotent: rendering the same inputs twice yields identical DOM", () => { + const { grid, el } = makeGrid(); + grid.renderMenu(piCustoms, null); + const first = el.innerHTML; + grid.renderMenu(piCustoms, null); + expect(el.innerHTML).toBe(first); + expect(el.querySelectorAll(".saypi-more-voices").length).toBe(1); + }); + + it("keeps DOM identity of surviving rows across re-renders (no listener churn)", () => { + const { grid, el } = makeGrid(); + grid.renderMenu(piCustoms, null); + const before = rowsOf(el)[0]; + grid.renderMenu(piCustoms, null); + expect(rowsOf(el)[0]).toBe(before); + }); + + it("always shows the stored voice, synchronously, even when the cap would hide it", () => { + const shimmer = openAiMockVoices.find((v) => v.name === "Shimmer")!; + const { grid, el } = makeGrid({ stored: shimmer }); + grid.renderMenu(piCustoms, shimmer); // no flushAsync, no pin recursion + expect(idsOf(el)).toContain("shimmer"); + expect(rowsOf(el).length).toBe(5); + expect( + (el.querySelector('[data-voice-id="shimmer"]') as HTMLButtonElement) + .disabled + ).toBe(true); + }); + + it("renders the stored voice's row selected and no other", () => { + const coral = openAiMockVoices.find((v) => v.name === "Coral")!; + const { grid, el } = makeGrid({ stored: coral }); + grid.renderMenu(piCustoms, coral); + const selected = el.querySelectorAll(".selected"); + expect(selected.length).toBe(1); + expect((selected[0] as HTMLElement).dataset.voiceId).toBe("coral"); + }); + + it("switching the stored voice on re-render moves the mark and un-hides the new voice (no stale mark, no vanish)", () => { + const shimmer = openAiMockVoices.find((v) => v.name === "Shimmer")!; + const coral = openAiMockVoices.find((v) => v.name === "Coral")!; + const { grid, el } = makeGrid(); + grid.renderMenu(piCustoms, shimmer); + grid.renderMenu(piCustoms, coral); + expect(idsOf(el)).toContain("coral"); + expect(idsOf(el)).not.toContain("shimmer"); // stale pin gone, stateless + const selected = el.querySelectorAll(".selected"); + expect(selected.length).toBe(1); + expect((selected[0] as HTMLElement).dataset.voiceId).toBe("coral"); + }); + + it("uncapped surfaces render the whole catalog with no tier badges and no door by default", () => { + const { grid, el } = makeGrid({ cap: null }); + grid.renderMenu(piCustoms, null); + expect(rowsOf(el).length).toBe(piCustoms.length); + expect(el.querySelector(".voice-tier")).toBeNull(); + expect(el.querySelector(".saypi-more-voices")).toBeNull(); + }); +}); + +describe("GridVoiceSelector — host-row adoption (positive classification)", () => { + it("adopts pre-existing host buttons: marks them and wires unset-on-click exactly once across renders", async () => { + const { grid, el } = makeGrid(); + const hostBtn = document.createElement("button"); + hostBtn.textContent = "Pi 1"; + el.appendChild(hostBtn); + grid.renderMenu(piCustoms, null); + grid.renderMenu(piCustoms, null); // second render must not double-bind + expect(hostBtn.getAttribute(HOST_VOICE_ATTR)).toBe("true"); + hostBtn.click(); + await new Promise((r) => setTimeout(r, 0)); + expect(grid.userPreferences.unsetVoice).toHaveBeenCalledTimes(1); + expect(hostBtn.classList.contains("selected")).toBe(true); + }); + + it("voice-off selection leaves host rows untouched but clears SayPi rows", () => { + const coral = openAiMockVoices.find((v) => v.name === "Coral")!; + const { grid, el } = makeGrid(); + const hostBtn = document.createElement("button"); + el.appendChild(hostBtn); + grid.renderMenu(piCustoms, coral); + grid.renderMenu(piCustoms, null); + expect(el.querySelectorAll(".saypi-custom-voice.selected").length).toBe(0); + }); +}); + +describe("GridVoiceSelector — Pi extras top-up (was addMissingPiVoices)", () => { + it("tops up Pi's account-gated extras for provider chatbots when fewer than 8 host rows", () => { + const { grid, el } = makeGrid({ provider: true }); + grid.renderMenu(piCustoms, null); + expect(idsOf(el)).toContain("voice7"); + expect(idsOf(el)).toContain("voice8"); + }); + + it("extras survive re-renders alongside a shortlist-hidden stored voice (the Patch A regression)", () => { + const shimmer = openAiMockVoices.find((v) => v.name === "Shimmer")!; + const { grid, el } = makeGrid({ provider: true }); + grid.renderMenu(piCustoms, shimmer); + grid.renderMenu(piCustoms, shimmer); + grid.renderMenu(piCustoms, shimmer); + const ids = idsOf(el); + expect(ids).toContain("shimmer"); + expect(ids).toContain("voice7"); + expect(ids).toContain("voice8"); + }); + + it("does not top up for chatbots without built-in voices", () => { + const { grid, el } = makeGrid({ provider: false }); + grid.renderMenu(piCustoms, null); + expect(idsOf(el)).not.toContain("voice7"); + }); +}); + +describe("GridVoiceSelector — unknown elements are inert (#485 / ▶-deselects trap)", () => { + function nestedPreviewFixture(el: HTMLElement): HTMLButtonElement { + // A future Pi row: wrapper div holding a ▶ — the #483 shape. + const rowWrapper = document.createElement("div"); + const preview = document.createElement("button"); + preview.classList.add("saypi-voice-preview"); + rowWrapper.appendChild(preview); + el.appendChild(rowWrapper); + return preview; + } + + it("re-render never throws on, removes, adopts, or counts a nested ▶", () => { + const { grid, el } = makeGrid({ provider: true }); + const preview = nestedPreviewFixture(el); + expect(() => grid.renderMenu(piCustoms, null)).not.toThrow(); + expect(el.contains(preview)).toBe(true); + expect(preview.hasAttribute(HOST_VOICE_ATTR)).toBe(false); + expect(idsOf(el)).toContain("voice7"); // ▶ not miscounted as a host voice + }); + + it("clicking a ▶ never unsets or sets the voice", async () => { + const { grid, el } = makeGrid(); + const preview = nestedPreviewFixture(el); + grid.renderMenu(piCustoms, null); + preview.click(); + await new Promise((r) => setTimeout(r, 0)); + expect(grid.userPreferences.unsetVoice).not.toHaveBeenCalled(); + expect(grid.userPreferences.setVoice).not.toHaveBeenCalled(); + }); + + it("a DIRECT-CHILD ▶ (SayPi-marked control) is still not adopted — positive marks win over position", () => { + const { grid, el } = makeGrid(); + const preview = document.createElement("button"); + preview.classList.add("saypi-voice-preview"); + el.appendChild(preview); + grid.renderMenu(piCustoms, null); + expect(preview.hasAttribute(HOST_VOICE_ATTR)).toBe(false); + }); +}); + +describe("GridVoiceSelector — selection via rows (click paths)", () => { + it("clicking a SayPi row persists the voice, marks exactly that row, and introduces the voice", async () => { + const { grid, el } = makeGrid(); + grid.renderMenu(piCustoms, null); + const row = el.querySelector('[data-voice-id="coral"]') as HTMLButtonElement; + row.click(); + await new Promise((r) => setTimeout(r, 0)); + expect(grid.userPreferences.setVoice).toHaveBeenCalled(); + expect(row.classList.contains("selected")).toBe(true); + expect(el.querySelectorAll(".selected").length).toBe(1); + expect(grid.introduceVoice).toHaveBeenCalled(); + }); + + it("tier badge appears on HD rows only while tiers coexist", () => { + const { grid, el } = makeGrid(); + grid.renderMenu(piCustoms, null); + const paola = el.querySelector('[data-voice-id="ig1TeITnnNlsJtfHxJlW"]'); + // Paola is HD and featured, so present under the cap; badge because tiers coexist. + expect(paola?.querySelector(".voice-tier")?.textContent).toBe("HD"); + }); +}); diff --git a/test/vitest.setup.js b/test/vitest.setup.js index e0407c313f..f4f104f9ec 100644 --- a/test/vitest.setup.js +++ b/test/vitest.setup.js @@ -35,6 +35,7 @@ Object.defineProperties(global, { HTMLInputElement: { value: dom.window.HTMLInputElement }, HTMLTextAreaElement: { value: dom.window.HTMLTextAreaElement }, HTMLDivElement: { value: dom.window.HTMLDivElement }, + HTMLButtonElement: { value: dom.window.HTMLButtonElement }, DOMParser: { value: dom.window.DOMParser }, SVGElement: { value: dom.window.SVGElement }, Event: { value: dom.window.Event }, From fdcfe914cf43cf187e86eddca3d180cc36b7c225 Mon Sep 17 00:00:00 2001 From: Ross Cadogan Date: Sat, 4 Jul 2026 18:36:40 +0100 Subject: [PATCH 4/6] refactor(voices): Pi surfaces render through GridVoiceSelector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The expand listener collapses to one gather-then-render (TTS-off still adopts host rows, tops up Pi extras, and keeps the door); PiVoiceSettings' four-call constructor sequence becomes refreshMenu(). Existing specs are transformed to drive renderMenu(voices, stored) directly — every product invariant assertion (cap, door, uncapped grid, tier badge, hidden-stored- voice visibility, Pi7/8 survival, stale-pin clearance) is preserved, now without flushAsync pin-recursion choreography. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Y1oVsdBKdATjojxQprFhR4 --- src/chatbots/PiVoiceMenu.ts | 44 +++--- test/chatbots/PiVoiceMenu-curation.spec.ts | 143 ++++++++---------- .../VoiceMenu-preference-sync.spec.ts | 2 +- test/tts/VoiceMenu.spec.ts | 57 ++++--- test/tts/VoiceMenuRefresh.spec.ts | 99 ++++++------ 5 files changed, 172 insertions(+), 173 deletions(-) diff --git a/src/chatbots/PiVoiceMenu.ts b/src/chatbots/PiVoiceMenu.ts index 218f6d30bd..956ebda86c 100644 --- a/src/chatbots/PiVoiceMenu.ts +++ b/src/chatbots/PiVoiceMenu.ts @@ -1,13 +1,12 @@ import { Observation } from "../dom/Observation"; import EventBus from "../events/EventBus"; import { audioProviders } from "../tts/SpeechModel"; -import { SpeechSynthesisModule } from "../tts/SpeechSynthesisModule"; -import { VoiceSelector } from "../tts/VoiceMenu"; +import { GridVoiceSelector } from "../tts/GridVoiceSelector"; import { PI_MENU_CAP } from "../tts/VoiceCuration"; import { Chatbot } from "./Chatbot"; import { UserPreferenceModule } from "../prefs/PreferenceModule"; -export class PiVoiceMenu extends VoiceSelector { +export class PiVoiceMenu extends GridVoiceSelector { constructor( chatbot: Chatbot, userPreferences: UserPreferenceModule, @@ -132,12 +131,17 @@ export class PiVoiceMenu extends VoiceSelector { node === audioControlsContainer.firstChild ) { voiceMenu.classList.add("expanded"); - // mark the selected voice each time the menu is expanded (because pi.ai recreates the menu each time) + // Pi recreates its native menu on every expand: re-adopt the + // fresh host rows and re-render SayPi's block from state. When + // TTS is off we still adopt + top up Pi's extras (and the + // door), just with no SayPi catalog rows. this.userPreferences.getTextToSpeechEnabled().then((enabled) => { - if (enabled) this.addVoicesToSelector(voiceMenu); + if (enabled) { + this.refreshMenu(); + } else { + this.renderMenu([], null); + } }); - this.addMissingPiVoices(voiceMenu); - this.registerVoiceChangeHandler(voiceMenu); } } for (let node of mutation.removedNodes) { @@ -160,7 +164,7 @@ export class PiVoiceMenu extends VoiceSelector { } } -export class PiVoiceSettings extends VoiceSelector { +export class PiVoiceSettings extends GridVoiceSelector { constructor( chatbot: Chatbot, userPreferences: UserPreferenceModule, @@ -168,14 +172,10 @@ export class PiVoiceSettings extends VoiceSelector { ) { super(chatbot, userPreferences, element); this.addIdVoiceMenu(element); - SpeechSynthesisModule.getInstance() - .getVoices(chatbot) - .then((multilingualVoices) => { - this.populateVoices(multilingualVoices, element); - this.addMissingPiVoices(element); - this.handleExistingVoiceButtons(element); - this.registerVoiceChangeHandler(element); - }); + // One gather-then-render covers what four calls did before: catalog rows, + // Pi's extra-voice top-up, adoption of Pi's pre-existing native buttons, + // and selection marking. + this.refreshMenu(); } getId(): string { @@ -203,14 +203,4 @@ export class PiVoiceSettings extends VoiceSelector { "border-neutral-500", ]; } - - handleExistingVoiceButtons(voiceMenu: HTMLElement): void { - const voiceButtons = Array.from(voiceMenu.querySelectorAll("button")); - if (!voiceButtons || voiceButtons.length === 0) { - return; - } - voiceButtons.forEach((button) => { - this.handleButtonAddition(button as HTMLButtonElement); - }); - } -} \ No newline at end of file +} diff --git a/test/chatbots/PiVoiceMenu-curation.spec.ts b/test/chatbots/PiVoiceMenu-curation.spec.ts index 0c064b057b..2473e8ebf3 100644 --- a/test/chatbots/PiVoiceMenu-curation.spec.ts +++ b/test/chatbots/PiVoiceMenu-curation.spec.ts @@ -50,17 +50,20 @@ function builtInVoice(id: string, name: string): SpeechSynthesisVoiceRemote { } const piBuiltIns = [builtInVoice("voice1", "Pi 1"), builtInVoice("voice2", "Pi 2")]; -// Bypass the heavy constructor (DOM observers); populateVoices only needs -// prototype methods plus the fields it reads. -function makeMenu(currentVoice: SpeechSynthesisVoiceRemote | null = null): any { +// Bypass the heavy constructor (DOM observers); renderMenu only needs +// prototype methods plus the fields it reads. renderMenu targets +// menu.element — the stored voice is passed in directly (gather-then-render), +// so no getVoice mock choreography or flushAsync is needed. +function makeMenu(): any { const menu = Object.create(PiVoiceMenu.prototype); - menu.chatbot = {} as any; + menu.chatbot = { getID: () => "pi" } as any; menu.userPreferences = { - getVoice: vi.fn(async () => currentVoice), + getVoice: vi.fn(async () => null), setVoice: vi.fn(async () => {}), unsetVoice: vi.fn(async () => {}), }; menu.element = document.createElement("div"); + menu.introduceVoice = vi.fn(); return menu; } @@ -68,10 +71,6 @@ function customRows(selector: HTMLElement): HTMLButtonElement[] { return Array.from(selector.querySelectorAll("button.saypi-custom-voice")); } -function flushAsync(): Promise { - return new Promise((resolve) => setTimeout(resolve, 0)); -} - beforeEach(() => { openSettingsMock.mockReset(); document.body.innerHTML = ""; @@ -80,26 +79,23 @@ beforeEach(() => { describe("PiVoiceMenu shortlist cap + door", () => { it("caps SayPi (custom) rows at PI_MENU_CAP on the flip-day catalog", () => { const menu = makeMenu(); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - expect(customRows(selector).length).toBe(PI_MENU_CAP); + menu.renderMenu([...piBuiltIns, ...piFlipDay], null); + expect(customRows(menu.element).length).toBe(PI_MENU_CAP); }); it("never caps Pi's own built-in voice rows", () => { const menu = makeMenu(); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); + menu.renderMenu([...piBuiltIns, ...piFlipDay], null); const builtInRows = Array.from( - selector.querySelectorAll("button.saypi-restored-voice") + menu.element.querySelectorAll("button.saypi-restored-voice") ); expect(builtInRows.length).toBe(piBuiltIns.length); }); it("adds a muted 'More voices' button after the SayPi block when voices are hidden", () => { const menu = makeMenu(); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - const door = selector.querySelector( + menu.renderMenu([...piBuiltIns, ...piFlipDay], null); + const door = menu.element.querySelector( "button.saypi-more-voices" ) as HTMLButtonElement; expect(door).not.toBeNull(); @@ -109,10 +105,9 @@ describe("PiVoiceMenu shortlist cap + door", () => { it("still shows the door when the catalog fits the cap — it's the path to the catalog, not an overflow marker (#472)", () => { const menu = makeMenu(); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piElevenLabs], selector); - expect(customRows(selector).length).toBe(piElevenLabs.length); - const door = selector.querySelector( + menu.renderMenu([...piBuiltIns, ...piElevenLabs], null); + expect(customRows(menu.element).length).toBe(piElevenLabs.length); + const door = menu.element.querySelector( "button.saypi-more-voices" ) as HTMLButtonElement; expect(door).not.toBeNull(); @@ -124,22 +119,22 @@ describe("PiVoiceMenu shortlist cap + door", () => { describe("PiVoiceSettings door (#472)", () => { function makeSettings(): any { const settings = Object.create(PiVoiceSettings.prototype); - settings.chatbot = {} as any; + settings.chatbot = { getID: () => "pi" } as any; settings.userPreferences = { getVoice: vi.fn(async () => null), setVoice: vi.fn(async () => {}), unsetVoice: vi.fn(async () => {}), }; settings.element = document.createElement("div"); + settings.introduceVoice = vi.fn(); return settings; } it("appends a 'More voices' door to the uncapped settings grid", () => { const settings = makeSettings(); - const grid = document.createElement("div"); - settings.populateVoices([...piElevenLabs], grid); - expect(customRows(grid).length).toBe(piElevenLabs.length); // grid stays uncapped - const door = grid.querySelector( + settings.renderMenu([...piElevenLabs], null); + expect(customRows(settings.element).length).toBe(piElevenLabs.length); // grid stays uncapped + const door = settings.element.querySelector( "button.saypi-more-voices" ) as HTMLButtonElement; expect(door).not.toBeNull(); @@ -147,21 +142,21 @@ describe("PiVoiceSettings door (#472)", () => { expect(openSettingsMock).toHaveBeenCalledWith("voices"); }); - it("does not duplicate the door on repeated populates", () => { + it("does not duplicate the door on repeated renders", () => { const settings = makeSettings(); - const grid = document.createElement("div"); - settings.populateVoices([...piElevenLabs], grid); - settings.populateVoices([...piElevenLabs], grid); - expect(grid.querySelectorAll("button.saypi-more-voices").length).toBe(1); + settings.renderMenu([...piElevenLabs], null); + settings.renderMenu([...piElevenLabs], null); + expect( + settings.element.querySelectorAll("button.saypi-more-voices").length + ).toBe(1); }); }); describe("PiVoiceMenu tier badge", () => { it("suffixes premium rows with a quiet HD badge only when tiers coexist", () => { const menu = makeMenu(); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - const rows = customRows(selector); + menu.renderMenu([...piBuiltIns, ...piFlipDay], null); + const rows = customRows(menu.element); const paola = rows.find( (r) => r.dataset.voiceId === "ig1TeITnnNlsJtfHxJlW" )!; @@ -172,72 +167,60 @@ describe("PiVoiceMenu tier badge", () => { it("shows no badge for today's single-tier catalog", () => { const menu = makeMenu(); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piElevenLabs], selector); - expect(selector.querySelector(".voice-tier")).toBeNull(); + menu.renderMenu([...piBuiltIns, ...piElevenLabs], null); + expect(menu.element.querySelector(".voice-tier")).toBeNull(); }); }); -describe("PiVoiceMenu current-voice pinning", () => { - it("re-renders with the stored voice visible when the cap would have hidden it", async () => { +describe("PiVoiceMenu current-voice visibility", () => { + it("renders the stored voice visible when the cap would have hidden it — synchronously, no pin recursion", () => { const shimmer = openAiMockVoices.find((v) => v.name === "Shimmer")!; - const menu = makeMenu(shimmer); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - await flushAsync(); - const rows = customRows(selector); + const menu = makeMenu(); + menu.renderMenu([...piBuiltIns, ...piFlipDay], shimmer); + const rows = customRows(menu.element); const ids = rows.map((r) => r.dataset.voiceId); expect(ids).toContain("shimmer"); expect(rows.length).toBe(PI_MENU_CAP); }); - it("does not duplicate the door row when populateVoices runs again on the same menu", async () => { + it("does not duplicate the door row when renderMenu runs again on the same menu", () => { const menu = makeMenu(); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - await flushAsync(); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - await flushAsync(); - expect(selector.querySelectorAll("button.saypi-more-voices").length).toBe(1); + menu.renderMenu([...piBuiltIns, ...piFlipDay], null); + menu.renderMenu([...piBuiltIns, ...piFlipDay], null); + expect( + menu.element.querySelectorAll("button.saypi-more-voices").length + ).toBe(1); }); - it("keeps the pin when a partial built-ins-only populate runs (addMissingPiVoices path)", async () => { + it("keeps Pi's extra voices AND the hidden stored voice across re-renders (was: partial-populate pin wipe / Pi7-8 deletion)", () => { const shimmer = openAiMockVoices.find((v) => v.name === "Shimmer")!; - const menu = makeMenu(shimmer); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - await flushAsync(); // pin applied, Shimmer visible - // Pi tops the menu up with its own extra voices (voice7/voice8 are - // default=false, so they arrive as a customs-only partial list). - const extras = [builtInVoice("voice7", "Pi 7"), builtInVoice("voice8", "Pi 8")]; - extras.forEach((v) => ((v as any).default = false)); - menu.populateVoices(extras, selector); - await flushAsync(); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - await flushAsync(); - const ids = customRows(selector).map((r) => r.dataset.voiceId); + const menu = makeMenu(); + // Pi tops the menu up with its own account-gated extra voices; these are + // rendered by the same pass now, never via a separate partial populate. + menu.chatbot = { + getID: () => "pi", + getExtraVoices: () => [ + new OpenAIVoice("voice7", "Pi 7"), + new OpenAIVoice("voice8", "Pi 8"), + ], + getVoiceIntroductionUrl: () => "https://pi.ai/intro.mp3", + } as any; + menu.renderMenu([...piBuiltIns, ...piFlipDay], shimmer); + menu.renderMenu([...piBuiltIns, ...piFlipDay], shimmer); + const ids = customRows(menu.element).map((r) => r.dataset.voiceId); expect(ids).toContain("shimmer"); - // ...and the pin re-render must not have deleted the extra built-ins. + // ...and the re-render must not have deleted the extras. expect(ids).toContain("voice7"); expect(ids).toContain("voice8"); }); - it("clears a stale pin after the user switches to a featured voice", async () => { + it("stops showing a previously-pinned voice once the user switches to a featured voice (was: stale pin)", () => { const shimmer = openAiMockVoices.find((v) => v.name === "Shimmer")!; const coral = openAiMockVoices.find((v) => v.name === "Coral")!; - let stored: any = shimmer; const menu = makeMenu(); - menu.userPreferences.getVoice = vi.fn(async () => stored); - const selector = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - await flushAsync(); // pin = shimmer - stored = coral; // user picked a featured voice - menu.populateVoices([...piBuiltIns, ...piFlipDay], selector); - await flushAsync(); // stale pin detected and cleared - const rebuilt = document.createElement("div"); - menu.populateVoices([...piBuiltIns, ...piFlipDay], rebuilt); - await flushAsync(); - const ids = customRows(rebuilt).map((r) => r.dataset.voiceId); + menu.renderMenu([...piBuiltIns, ...piFlipDay], shimmer); + menu.renderMenu([...piBuiltIns, ...piFlipDay], coral); + const ids = customRows(menu.element).map((r) => r.dataset.voiceId); expect(ids).not.toContain("shimmer"); expect(ids).toContain("coral"); }); diff --git a/test/chatbots/VoiceMenu-preference-sync.spec.ts b/test/chatbots/VoiceMenu-preference-sync.spec.ts index c38a110eba..153de4faf6 100644 --- a/test/chatbots/VoiceMenu-preference-sync.spec.ts +++ b/test/chatbots/VoiceMenu-preference-sync.spec.ts @@ -152,7 +152,7 @@ describe("PiVoiceMenu reacts to settings-page voice changes (#475)", () => { const joey = piVoices[1]; let stored: SpeechSynthesisVoiceRemote | null = paola; const menu = makePiMenu(() => stored); - menu.populateVoices(piVoices, menu.element); + menu.renderMenu(piVoices, stored); await flushAsync(); stored = joey; diff --git a/test/tts/VoiceMenu.spec.ts b/test/tts/VoiceMenu.spec.ts index ab3823518a..0c02eb5d44 100644 --- a/test/tts/VoiceMenu.spec.ts +++ b/test/tts/VoiceMenu.spec.ts @@ -20,6 +20,8 @@ vi.mock("../../src/JwtManager", () => ({ })); import { VoiceSelector } from "../../src/tts/VoiceMenu"; +import { GridVoiceSelector } from "../../src/tts/GridVoiceSelector"; +import { SpeechSynthesisVoiceRemote } from "../../src/tts/SpeechModel"; import { ClaudeVoiceMenu } from "../../src/chatbots/ClaudeVoiceMenu"; // Minimal concrete selector to exercise base-class logic. @@ -30,6 +32,8 @@ class TestVoiceSelector extends VoiceSelector { getButtonClasses(): string[] { return []; } + protected renderMenu(): void {} + protected applySelectedVoice(): void {} } describe("VoiceSelector.ttsRequiresSignIn", () => { @@ -57,39 +61,52 @@ describe("VoiceSelector.ttsRequiresSignIn", () => { }); }); -describe("VoiceSelector built-in-voice-provider capability detection", () => { - // The base class adds a chatbot's own built-in voices (e.g. Pi.ai's native - // voices + introduction audio) when the chatbot can provide them. This used - // to be gated on `instanceof PiAIChatbot`, which forced VoiceMenu to import - // the concrete Pi chatbot and created a VoiceMenu -> Pi -> PiVoiceMenu -> - // VoiceMenu import cycle. The gate is now a structural capability check, so +describe("GridVoiceSelector built-in-voice-provider capability detection", () => { + // The grid render tops up a chatbot's own built-in voices (e.g. Pi.ai's + // account-gated extras) when the chatbot can provide them. This used to be + // gated on `instanceof PiAIChatbot`, which forced VoiceMenu to import the + // concrete Pi chatbot and created a VoiceMenu -> Pi -> PiVoiceMenu -> + // VoiceMenu import cycle. The gate is a structural capability check, so // any chatbot exposing getExtraVoices()/getVoiceIntroductionUrl() qualifies. - function makeSelector(chatbot: unknown): TestVoiceSelector { - return new TestVoiceSelector( - chatbot as any, - {} as any, - document.createElement("div"), - ); + class TestGrid extends GridVoiceSelector { + getId(): string { + return "test-grid-capability"; + } + getButtonClasses(): string[] { + return []; + } + } + + function makeGrid(chatbot: unknown): any { + const grid: any = Object.create(TestGrid.prototype); + grid.chatbot = chatbot; + grid.userPreferences = { + getVoice: vi.fn(async () => null), + setVoice: vi.fn(async () => {}), + unsetVoice: vi.fn(async () => {}), + }; + grid.element = document.createElement("div"); + return grid; } it("requests extra voices from a chatbot that provides its own built-in voices", () => { - const getExtraVoices = vi.fn(() => []); + const getExtraVoices = vi.fn((): SpeechSynthesisVoiceRemote[] => []); const chatbot = { getExtraVoices, getVoiceIntroductionUrl: vi.fn(() => ""), }; - const selector = makeSelector(chatbot); - // An empty menu has 0 built-in voices, i.e. below the 8-voice threshold. - selector.addMissingPiVoices(document.createElement("div")); + const grid = makeGrid(chatbot); + // An empty menu has 0 adopted host rows, i.e. below the 8-voice threshold. + grid.renderMenu([], null); expect(getExtraVoices).toHaveBeenCalled(); }); it("does nothing for a chatbot without built-in voices (e.g. Claude)", () => { const chatbot = { getName: () => "Claude" }; - const selector = makeSelector(chatbot); - const populateSpy = vi.spyOn(selector as any, "populateVoices"); - selector.addMissingPiVoices(document.createElement("div")); - expect(populateSpy).not.toHaveBeenCalled(); + const grid = makeGrid(chatbot); + grid.renderMenu([], null); + // No extras requested, so no rows appear in the empty render. + expect(grid.element.querySelectorAll("[data-voice-id]").length).toBe(0); }); }); diff --git a/test/tts/VoiceMenuRefresh.spec.ts b/test/tts/VoiceMenuRefresh.spec.ts index 7bb0841d92..9a86845a88 100644 --- a/test/tts/VoiceMenuRefresh.spec.ts +++ b/test/tts/VoiceMenuRefresh.spec.ts @@ -4,9 +4,6 @@ vi.mock("../../src/ConfigModule", () => ({ config: { appServerUrl: "https://app.example.com", apiServerUrl: "https://api.saypi.ai" }, })); -// Break the VoiceMenu -> Pi -> PiVoiceMenu -> VoiceMenu import cycle. -vi.mock("../../src/chatbots/Pi", () => ({ PiAIChatbot: class {} })); - vi.mock("../../src/JwtManager", () => ({ getJwtManagerSync: () => ({ isAuthenticated: () => true, getClaims: () => null }), })); @@ -14,8 +11,8 @@ vi.mock("../../src/JwtManager", () => ({ vi.mock("../../src/dom/ChatHistory", () => ({ getMostRecentAssistantMessage: () => undefined })); vi.mock("../../src/i18n", () => ({ default: (key: string) => key })); -// refreshMenu re-populates via getVoices(); return [] so the test isolates the -// TEARDOWN half (the crash) — re-population is exercised elsewhere. +// refreshMenu gathers via getVoices() + getVoice(); return [] so the test +// isolates the render-with-nested-buttons-present path. const getVoicesMock = vi.fn().mockResolvedValue([]); vi.mock("../../src/tts/SpeechSynthesisModule", () => ({ SpeechSynthesisModule: { @@ -32,11 +29,11 @@ vi.mock("../../src/events/EventBus", () => ({ default: { emit: vi.fn(), on: vi.fn(), off: vi.fn() }, })); -import { VoiceSelector } from "../../src/tts/VoiceMenu"; +import { GridVoiceSelector } from "../../src/tts/GridVoiceSelector"; -class TestVoiceSelector extends VoiceSelector { +class TestGrid extends GridVoiceSelector { getId(): string { - return "test-voice-selector"; + return "test-grid-refresh"; } getButtonClasses(): string[] { return []; @@ -44,27 +41,33 @@ class TestVoiceSelector extends VoiceSelector { } /** - * Regression (#482 lit up live by saypi-api's `sample_url`): the ▶ preview is a - * `