diff --git a/.agents/skills/custom-codereview-guide.md b/.agents/skills/custom-codereview-guide.md new file mode 100644 index 0000000000..6add03c6ed --- /dev/null +++ b/.agents/skills/custom-codereview-guide.md @@ -0,0 +1,47 @@ +--- +name: custom-codereview-guide +description: Repo-specific code review guidelines for All-Hands-AI/OpenHands. Provides frontend and backend review rules in addition to the default code review skill. +triggers: +- /codereview +--- + +# All-Hands-AI/OpenHands Code Review Guidelines + +You are an expert code reviewer for the **All-Hands-AI/OpenHands** repository. This skill provides repo-specific review guidelines. + +## Frontend: i18n / Translation Key Usage + +**Never dynamically construct i18n keys via string interpolation or template literals.** + +All translation keys must come from the `I18nKey` enum (`frontend/src/i18n/declaration.ts`) or from canonical mapping objects like `AGENT_STATUS_MAP` (`frontend/src/utils/status.ts`). Dynamically constructed keys (e.g., `` t(`STATUS$${value.toUpperCase()}`) ``) will silently fall back to the raw key string at runtime because `i18next` returns the key itself when a translation is missing — this produces broken UI text with no build-time or test-time error. + +### What to flag + +- Any call to `t(...)` or `i18next.t(...)` where the key is built at runtime via template literals, string concatenation, or helper functions rather than referencing `I18nKey` or a known mapping +- Any new i18n key referenced in code that does not exist in `frontend/src/i18n/translation.json` + +### Correct pattern + +```ts +import { AGENT_STATUS_MAP } from "#/utils/status"; + +const i18nKey = AGENT_STATUS_MAP[agentState]; +const message = i18nKey ? t(i18nKey) : fallback; +``` + +### Incorrect pattern + +```ts +// BAD: constructs a key that may not exist in translation.json +const message = t(`STATUS$${agentState.toUpperCase()}`); +``` + +## Frontend: Data Fetching Architecture + +UI components must never call API client methods (`frontend/src/api/`) directly. All data access must go through TanStack Query hooks: + +``` +UI components → TanStack Query hooks (frontend/src/hooks/query/ or mutation/) → API client (frontend/src/api/) → API endpoints +``` + +Flag any component that imports directly from `#/api/` and calls fetch/mutation functions without a TanStack Query wrapper. diff --git a/frontend/__tests__/hooks/use-agent-notification.test.ts b/frontend/__tests__/hooks/use-agent-notification.test.ts new file mode 100644 index 0000000000..28cf36b88d --- /dev/null +++ b/frontend/__tests__/hooks/use-agent-notification.test.ts @@ -0,0 +1,182 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { useAgentNotification } from "#/hooks/use-agent-notification"; +import { AgentState } from "#/types/agent-state"; +import * as browserTabModule from "#/utils/browser-tab"; + +// Mock useSettings to control the sound notification setting +vi.mock("#/hooks/query/use-settings", () => ({ + useSettings: vi.fn().mockReturnValue({ + data: { enable_sound_notifications: true }, + }), +})); + +// Spy on browserTab methods +vi.spyOn(browserTabModule.browserTab, "startNotification"); +vi.spyOn(browserTabModule.browserTab, "stopNotification"); + +// Mock Audio +const mockPlay = vi.fn().mockResolvedValue(undefined); +const mockAudio = { + play: mockPlay, + currentTime: 0, + volume: 0.5, +}; + +class MockAudio { + play = mockPlay; + currentTime = 0; + volume = 0.5; + constructor() { + Object.assign(this, mockAudio); + return mockAudio as unknown as MockAudio; + } +} +vi.stubGlobal("Audio", MockAudio); + +describe("useAgentNotification", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.clearAllMocks(); + // Simulate tab not focused + Object.defineProperty(document, "hasFocus", { + value: () => false, + configurable: true, + }); + }); + + afterEach(() => { + vi.useRealTimers(); + // Restore hasFocus + Object.defineProperty(document, "hasFocus", { + value: () => true, + configurable: true, + }); + }); + + it("starts browser tab notification when agent reaches FINISHED state and tab is not focused", () => { + const { rerender } = renderHook( + ({ state }) => useAgentNotification(state), + { initialProps: { state: AgentState.RUNNING } }, + ); + + // Transition to FINISHED + rerender({ state: AgentState.FINISHED }); + + expect( + browserTabModule.browserTab.startNotification, + ).toHaveBeenCalledTimes(1); + }); + + it("plays notification sound when agent reaches FINISHED state and sound is enabled", () => { + const { rerender } = renderHook( + ({ state }) => useAgentNotification(state), + { initialProps: { state: AgentState.RUNNING } }, + ); + + // Transition to FINISHED + rerender({ state: AgentState.FINISHED }); + + expect(mockPlay).toHaveBeenCalledTimes(1); + }); + + it("starts notification when agent reaches AWAITING_USER_INPUT state", () => { + const { rerender } = renderHook( + ({ state }) => useAgentNotification(state), + { initialProps: { state: AgentState.RUNNING } }, + ); + + // Transition to AWAITING_USER_INPUT + rerender({ state: AgentState.AWAITING_USER_INPUT }); + + expect( + browserTabModule.browserTab.startNotification, + ).toHaveBeenCalledTimes(1); + }); + + it("starts notification when agent reaches AWAITING_USER_CONFIRMATION state", () => { + const { rerender } = renderHook( + ({ state }) => useAgentNotification(state), + { initialProps: { state: AgentState.RUNNING } }, + ); + + rerender({ state: AgentState.AWAITING_USER_CONFIRMATION }); + + expect( + browserTabModule.browserTab.startNotification, + ).toHaveBeenCalledTimes(1); + }); + + it("stops browser tab notification when window gains focus", () => { + const { rerender } = renderHook( + ({ state }) => useAgentNotification(state), + { initialProps: { state: AgentState.RUNNING } }, + ); + + rerender({ state: AgentState.FINISHED }); + + // Simulate window focus + act(() => { + window.dispatchEvent(new Event("focus")); + }); + + expect( + browserTabModule.browserTab.stopNotification, + ).toHaveBeenCalledTimes(1); + }); + + it("does not start tab flash when focused, but still plays sound", () => { + Object.defineProperty(document, "hasFocus", { + value: () => true, + configurable: true, + }); + + const { rerender } = renderHook( + ({ state }) => useAgentNotification(state), + { initialProps: { state: AgentState.RUNNING } }, + ); + + rerender({ state: AgentState.FINISHED }); + + expect( + browserTabModule.browserTab.startNotification, + ).not.toHaveBeenCalled(); + // Sound still plays when focused (completion chime UX pattern) + expect(mockPlay).toHaveBeenCalledTimes(1); + }); + + it("does not play sound when sound notifications are disabled", async () => { + const { useSettings } = await import("#/hooks/query/use-settings"); + vi.mocked(useSettings).mockReturnValue({ + data: { enable_sound_notifications: false }, + } as ReturnType); + + const { rerender } = renderHook( + ({ state }) => useAgentNotification(state), + { initialProps: { state: AgentState.RUNNING } }, + ); + + rerender({ state: AgentState.FINISHED }); + + expect(mockPlay).not.toHaveBeenCalled(); + + // Restore + vi.mocked(useSettings).mockReturnValue({ + data: { enable_sound_notifications: true }, + } as ReturnType); + }); + + it("does not trigger for non-notification states like RUNNING", () => { + const { rerender } = renderHook( + ({ state }) => useAgentNotification(state), + { initialProps: { state: AgentState.LOADING } }, + ); + + rerender({ state: AgentState.RUNNING }); + + expect( + browserTabModule.browserTab.startNotification, + ).not.toHaveBeenCalled(); + expect(mockPlay).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/__tests__/utils/browser-tab.test.ts b/frontend/__tests__/utils/browser-tab.test.ts new file mode 100644 index 0000000000..f133e431cf --- /dev/null +++ b/frontend/__tests__/utils/browser-tab.test.ts @@ -0,0 +1,77 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; + +import { browserTab } from "#/utils/browser-tab"; + +// These tests exercise the browser-tab notification flasher behavior. +// Specifically we verify that when the document title changes externally +// while a notification is active, the flasher updates its internal +// baseline so it restores/toggles to the new title instead of an old one. + +describe("browserTab notifications", () => { + const MESSAGE = "Agent ready"; + const INITIAL = "Conversation 123 | OpenHands"; + const RENAMED = "My renamed title | OpenHands"; + + beforeEach(() => { + vi.useFakeTimers(); + // reset title for each test + document.title = INITIAL; + }); + + afterEach(() => { + browserTab.stopNotification(); + vi.runOnlyPendingTimers(); + vi.useRealTimers(); + }); + + it("flashes the browser tab title between the original title and the notification message", () => { + browserTab.startNotification(MESSAGE); + + // First tick: should switch to the notification message + vi.advanceTimersByTime(1000); + expect(document.title).toBe(MESSAGE); + + // Next tick: should switch back to original + vi.advanceTimersByTime(1000); + expect(document.title).toBe(INITIAL); + + // Next tick: should switch to message again + vi.advanceTimersByTime(1000); + expect(document.title).toBe(MESSAGE); + }); + + it("stops flashing and restores original title when stopNotification is called", () => { + browserTab.startNotification(MESSAGE); + + vi.advanceTimersByTime(1000); + expect(document.title).toBe(MESSAGE); + + browserTab.stopNotification(); + expect(document.title).toBe(INITIAL); + }); + + it("updates baseline when title changes during an active notification and restores to the new title", () => { + // Start flashing + browserTab.startNotification(MESSAGE); + + // Tick once: should switch to the message + vi.advanceTimersByTime(1000); + expect(document.title).toBe(MESSAGE); + + // Simulate an external rename while flashing (e.g., user edits title) + document.title = RENAMED; + + // Next tick: flasher observes the external change and updates baseline + vi.advanceTimersByTime(1000); + // On this tick, we toggle back to the message + expect(document.title).toBe(MESSAGE); + + // Next tick should toggle to the updated baseline (renamed title) + vi.advanceTimersByTime(1000); + expect(document.title).toBe(RENAMED); + + // Stop flashing: title should remain the updated baseline + browserTab.stopNotification(); + expect(document.title).toBe(RENAMED); + }); +}); diff --git a/frontend/src/assets/notification.mp3 b/frontend/src/assets/notification.mp3 new file mode 100644 index 0000000000..7937b7c566 Binary files /dev/null and b/frontend/src/assets/notification.mp3 differ diff --git a/frontend/src/components/features/controls/agent-status.tsx b/frontend/src/components/features/controls/agent-status.tsx index b0dba1fa25..176bffea23 100644 --- a/frontend/src/components/features/controls/agent-status.tsx +++ b/frontend/src/components/features/controls/agent-status.tsx @@ -14,6 +14,7 @@ import { useAgentState } from "#/hooks/use-agent-state"; import { useUnifiedWebSocketStatus } from "#/hooks/use-unified-websocket-status"; import { useTaskPolling } from "#/hooks/query/use-task-polling"; import { useSubConversationTaskPolling } from "#/hooks/query/use-sub-conversation-task-polling"; +import { useAgentNotification } from "#/hooks/use-agent-notification"; export interface AgentStatusProps { className?: string; @@ -33,6 +34,9 @@ export function AgentStatus({ const { t } = useTranslation(); const { setShouldShownAgentLoading } = useConversationStore(); const { curAgentState, executionStatus } = useAgentState(); + + // Trigger browser tab flash and notification sound on state changes + useAgentNotification(curAgentState); const webSocketStatus = useUnifiedWebSocketStatus(); const { data: conversation } = useActiveConversation(); const { taskStatus } = useTaskPolling(); diff --git a/frontend/src/hooks/use-agent-notification.ts b/frontend/src/hooks/use-agent-notification.ts new file mode 100644 index 0000000000..bef6190811 --- /dev/null +++ b/frontend/src/hooks/use-agent-notification.ts @@ -0,0 +1,76 @@ +import { useEffect, useRef } from "react"; +import { useTranslation } from "react-i18next"; +import { AgentState } from "#/types/agent-state"; +import { browserTab } from "#/utils/browser-tab"; +import { useSettings } from "#/hooks/query/use-settings"; +import { AGENT_STATUS_MAP } from "#/utils/status"; +import notificationSound from "#/assets/notification.mp3"; + +const NOTIFICATION_STATES: AgentState[] = [ + AgentState.AWAITING_USER_INPUT, + AgentState.FINISHED, + AgentState.AWAITING_USER_CONFIRMATION, +]; + +/** + * Hook that triggers browser tab flashing and notification sound + * when the agent transitions into a state that requires user attention. + * + * - Flashes the browser tab title when the tab is not focused. + * - Plays a notification sound if enabled in settings. + * - Stops flashing when the user focuses the tab. + */ +export function useAgentNotification(curAgentState: AgentState) { + const { data: settings } = useSettings(); + const { t } = useTranslation(); + const audioRef = useRef(undefined); + const prevStateRef = useRef(undefined); + + // Initialize audio only in browser environment, inside useEffect to + // avoid side effects during render (React 18 strict mode, SSR safety). + useEffect(() => { + if (typeof window !== "undefined" && !audioRef.current) { + audioRef.current = new Audio(notificationSound); + audioRef.current.volume = 0.5; + } + }, []); + + const isSoundEnabled = settings?.enable_sound_notifications ?? false; + + // Trigger notification only on actual state transitions into a + // notification-worthy state — not when unrelated deps (e.g. settings) change. + useEffect(() => { + if (prevStateRef.current === curAgentState) return; + prevStateRef.current = curAgentState; + + if (!NOTIFICATION_STATES.includes(curAgentState)) return; + + if (isSoundEnabled && audioRef.current) { + audioRef.current.currentTime = 0; + audioRef.current.play().catch(() => { + // Ignore autoplay errors (browsers may block autoplay) + }); + } + + if (typeof document !== "undefined" && !document.hasFocus()) { + const i18nKey = AGENT_STATUS_MAP[curAgentState]; + const message = i18nKey ? t(i18nKey) : curAgentState; + browserTab.startNotification(message); + } + }, [curAgentState, isSoundEnabled, t]); + + // Stop tab notification when window gains focus + useEffect(() => { + if (typeof window === "undefined") return undefined; + + const handleFocus = () => { + browserTab.stopNotification(); + }; + + window.addEventListener("focus", handleFocus); + return () => { + window.removeEventListener("focus", handleFocus); + browserTab.stopNotification(); + }; + }, []); +} diff --git a/frontend/src/utils/browser-tab.ts b/frontend/src/utils/browser-tab.ts new file mode 100644 index 0000000000..f08d9656ee --- /dev/null +++ b/frontend/src/utils/browser-tab.ts @@ -0,0 +1,42 @@ +let originalTitle = ""; +let titleInterval: number | undefined; + +const isBrowser = + typeof window !== "undefined" && typeof document !== "undefined"; + +export const browserTab = { + startNotification(message: string) { + if (!isBrowser) return; + + // Always capture the current title as the baseline to restore to + originalTitle = document.title; + + // Clear any existing interval + if (titleInterval) { + this.stopNotification(); + } + + // Alternate between the latest baseline title and the notification message. + // If the title changes externally (e.g., user renames conversation), + // update the baseline so we restore to the new value when stopping. + titleInterval = window.setInterval(() => { + const current = document.title; + if (current !== originalTitle && current !== message) { + originalTitle = current; + } + document.title = current === message ? originalTitle : message; + }, 1000); + }, + + stopNotification() { + if (!isBrowser) return; + + if (titleInterval) { + window.clearInterval(titleInterval); + titleInterval = undefined; + } + if (originalTitle) { + document.title = originalTitle; + } + }, +};