fix(site): address post-merge review comments on kyleosophy chimes (#23896)

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.
This commit is contained in:
Cian Johnston
2026-04-01 15:40:10 +01:00
committed by GitHub
parent 53a1b6d67e
commit 129e3509a3
4 changed files with 60 additions and 20 deletions
@@ -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");
});
},
};
@@ -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}
+12 -10
View File
@@ -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<string, unknown>).location;
}
});
it("returns true on dev.coder.com", () => {
+16 -8
View File
@@ -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,