From 129e3509a3f24bd3e9b2fb2117a88b1fe5801fc9 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 1 Apr 2026 15:40:10 +0100 Subject: [PATCH] fix(site): address post-merge review comments on kyleosophy chimes (#23896) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes issues found in post-merge review of #23891 and #23892. - **P2:** Export `_resetForTesting()` from `chime.ts` to break cross-test cache dependency; call in `beforeEach` - **P2:** Add `KylesophyToggle` and `TogglesKyleosophy` Storybook stories - **P3:** Fix JSDoc on `maybePlayChime` — terminal states are `waiting|pending`, not `waiting|error` - **P3:** Rename `setKylesophyLocal` back to `setLocalKyleosophy` to match `setLocal*` convention - Preserve original `location` property descriptor in `isKylesophyForced` tests to avoid leaking mutated descriptors across test suites (#23892 review) > 🤖 Written by a Coder Agent. Reviewed by a human. --- .../AgentSettingsBehaviorPageView.stories.tsx | 30 +++++++++++++++++++ .../AgentSettingsBehaviorPageView.tsx | 4 +-- site/src/pages/AgentsPage/utils/chime.test.ts | 22 +++++++------- site/src/pages/AgentsPage/utils/chime.ts | 24 ++++++++++----- 4 files changed, 60 insertions(+), 20 deletions(-) diff --git a/site/src/pages/AgentsPage/AgentSettingsBehaviorPageView.stories.tsx b/site/src/pages/AgentsPage/AgentSettingsBehaviorPageView.stories.tsx index c6ebfdf913..e2520a40ba 100644 --- a/site/src/pages/AgentsPage/AgentSettingsBehaviorPageView.stories.tsx +++ b/site/src/pages/AgentsPage/AgentSettingsBehaviorPageView.stories.tsx @@ -460,3 +460,33 @@ export const NoWarningForCleanPrompt: Story = { expect(canvas.queryByText(/invisible Unicode/)).toBeNull(); }, }; + +// ── Kyleosophy ───────────────────────────────────────────────── + +export const KylesophyToggle: Story = { + play: async ({ canvasElement }) => { + localStorage.removeItem("agents.kyleosophy"); + const canvas = within(canvasElement); + await canvas.findByText("Kyleosophy"); + await canvas.findByText(/Replace the standard completion chime/i); + const toggle = await canvas.findByRole("switch", { + name: "Enable Kyleosophy", + }); + expect(toggle).not.toBeChecked(); + }, +}; + +export const TogglesKyleosophy: Story = { + play: async ({ canvasElement }) => { + localStorage.removeItem("agents.kyleosophy"); + const canvas = within(canvasElement); + const toggle = await canvas.findByRole("switch", { + name: "Enable Kyleosophy", + }); + + await userEvent.click(toggle); + await waitFor(() => { + expect(localStorage.getItem("agents.kyleosophy")).toBe("true"); + }); + }, +}; diff --git a/site/src/pages/AgentsPage/AgentSettingsBehaviorPageView.tsx b/site/src/pages/AgentsPage/AgentSettingsBehaviorPageView.tsx index 0bc3592a9f..b5fc9a1838 100644 --- a/site/src/pages/AgentsPage/AgentSettingsBehaviorPageView.tsx +++ b/site/src/pages/AgentsPage/AgentSettingsBehaviorPageView.tsx @@ -130,7 +130,7 @@ export const AgentSettingsBehaviorPageView: FC< const [isSystemPromptOverflowing, setIsSystemPromptOverflowing] = useState(false); const kylesophyForced = isKylesophyForced(); - const [kylesophyEnabled, setKylesophyLocal] = useState(getKylesophyEnabled); + const [kylesophyEnabled, setLocalKylesophy] = useState(getKylesophyEnabled); // ── Derived state ── const hasLoadedSystemPrompt = systemPromptData !== undefined; @@ -536,7 +536,7 @@ export const AgentSettingsBehaviorPageView: FC< checked={kylesophyEnabled} onCheckedChange={(checked) => { setKylesophyEnabled(checked); - setKylesophyLocal(checked); + setLocalKylesophy(checked); }} aria-label="Enable Kyleosophy" disabled={kylesophyForced} diff --git a/site/src/pages/AgentsPage/utils/chime.test.ts b/site/src/pages/AgentsPage/utils/chime.test.ts index e9a56d27f8..fd81c58dcb 100644 --- a/site/src/pages/AgentsPage/utils/chime.test.ts +++ b/site/src/pages/AgentsPage/utils/chime.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { + _resetForTesting, getChimeEnabled, getKylesophyEnabled, isKylesophyForced, @@ -118,6 +119,7 @@ describe("maybePlayChime", () => { beforeEach(() => { vi.useFakeTimers(); localStorage.clear(); + _resetForTesting(); // Explicitly enable the chime — the default is now disabled. setChimeEnabled(true); @@ -299,10 +301,6 @@ describe("maybePlayChime", () => { setKylesophyEnabled(false); vi.spyOn(document, "hidden", "get").mockReturnValue(true); - // Force a fresh Audio element by spying on the constructor - // before any call in this test. The previous test left - // lastSoundUrl pointing at a kyleosophy URL, so switching - // back to /chime.mp3 will always trigger a new Audio(). const audioSpy = vi.spyOn(globalThis, "Audio" as never); await triggerAndSettle("running", "waiting", "chat-1", "chat-2"); @@ -320,14 +318,18 @@ describe("maybePlayChime", () => { // --------------------------------------------------------------------------- describe("isKylesophyForced", () => { - const originalLocation = globalThis.location; + const originalLocationDescriptor = Object.getOwnPropertyDescriptor( + globalThis, + "location", + ); afterEach(() => { - Object.defineProperty(globalThis, "location", { - value: originalLocation, - writable: true, - configurable: true, - }); + if (originalLocationDescriptor) { + Object.defineProperty(globalThis, "location", originalLocationDescriptor); + } else { + // If location did not originally exist, remove the stub. + delete (globalThis as Record).location; + } }); it("returns true on dev.coder.com", () => { diff --git a/site/src/pages/AgentsPage/utils/chime.ts b/site/src/pages/AgentsPage/utils/chime.ts index 35665ef92c..1e1de17d7c 100644 --- a/site/src/pages/AgentsPage/utils/chime.ts +++ b/site/src/pages/AgentsPage/utils/chime.ts @@ -81,6 +81,12 @@ export const KYLEOSOPHY_SOUNDS: readonly string[] = [ let chimeAudio: HTMLAudioElement | null = null; let lastSoundUrl: string | null = null; +/** @internal Reset cached Audio state between tests. */ +export function _resetForTesting(): void { + chimeAudio = null; + lastSoundUrl = null; +} + function playChimeAudio(soundUrl = "/chime.mp3"): void { try { if (!chimeAudio || soundUrl !== lastSoundUrl) { @@ -160,14 +166,16 @@ function playChime(chatID: string, soundUrl?: string): void { /** * Check whether a chat status transition should trigger a chime - * and play it if so. A chime fires when a chat reaches a - * terminal state ("waiting" or "error") from a non-terminal - * state, meaning the agent just finished work. The previous - * status may be "running" (seen via the per-chat WebSocket) or - * "pending" (when only the watchChats WebSocket is active and - * the intermediate "running" status was never pushed to the - * chat list). The chime is suppressed when the chat is - * currently visible to the user. + * and play it if so. The chime fires on these transitions: + * + * running → waiting (normal completion via per-chat WS) + * running → pending (normal completion via per-chat WS) + * pending → waiting (watchChats WS skipped "running") + * + * Note that "pending" appears as both a source and a target: + * it is an active state when the agent is queued, and a resting + * state after the agent finishes. The chime is suppressed when + * the chat is currently visible to the user. */ export function maybePlayChime( prevStatus: string | undefined,