test(web): harden primary-action ownership regressions

Refs #247

Co-Authored-By: OpenAI Codex <codex@openai.com>

web/src/primaryAction.test.tsx

@@ -0,0 +1,152 @@
+import { cleanup, fireEvent, render, screen } from '@testing-library/react';
+import { afterEach, describe, expect, it, vi } from 'vitest';
+import { PrimaryActionProvider, usePrimaryAction, usePrimaryActionHandler } from './primaryAction';
+
+function Registration({ routeId, handler }: { routeId: string; handler: () => void }) {
+  usePrimaryAction(routeId, handler);
+  return null;
+}
+
+function Action({ routeId }: { routeId: string }) {
+  const handler = usePrimaryActionHandler(routeId);
+  return (
+    <button type="button" disabled={!handler} onClick={handler}>
+      Run {routeId}
+    </button>
+  );
+}
+
+describe('primary actions', () => {
+  afterEach(() => {
+    cleanup();
+  });
+
+  it('invokes the active handler for a matching route', () => {
+    const handler = vi.fn();
+    render(
+      <PrimaryActionProvider>
+        <Registration routeId="schedules" handler={handler} />
+        <Action routeId="schedules" />
+      </PrimaryActionProvider>
+    );
+
+    fireEvent.click(screen.getByRole('button', { name: 'Run schedules' }));
+
+    expect(handler).toHaveBeenCalledOnce();
+  });
+
+  it('returns no handler for a nonmatching route', () => {
+    const handler = vi.fn();
+    render(
+      <PrimaryActionProvider>
+        <Registration routeId="schedules" handler={handler} />
+        <Action routeId="channels" />
+      </PrimaryActionProvider>
+    );
+
+    const action = screen.getByRole('button', { name: 'Run channels' });
+    expect(action).toBeDisabled();
+    fireEvent.click(action);
+    expect(handler).not.toHaveBeenCalled();
+  });
+
+  it('invokes the latest handler without replacing the registration', () => {
+    const firstHandler = vi.fn();
+    const latestHandler = vi.fn();
+    const view = render(
+      <PrimaryActionProvider>
+        <Registration routeId="schedules" handler={firstHandler} />
+        <Action routeId="schedules" />
+      </PrimaryActionProvider>
+    );
+
+    view.rerender(
+      <PrimaryActionProvider>
+        <Registration routeId="schedules" handler={latestHandler} />
+        <Action routeId="schedules" />
+      </PrimaryActionProvider>
+    );
+    fireEvent.click(screen.getByRole('button', { name: 'Run schedules' }));
+
+    expect(firstHandler).not.toHaveBeenCalled();
+    expect(latestHandler).toHaveBeenCalledOnce();
+  });
+
+  it("moves the same owner's registration to a new route and clears it on unmount", () => {
+    const handler = vi.fn();
+    const view = render(
+      <PrimaryActionProvider>
+        <Registration routeId="schedules" handler={handler} />
+        <Action routeId="schedules" />
+        <Action routeId="channels" />
+      </PrimaryActionProvider>
+    );
+
+    view.rerender(
+      <PrimaryActionProvider>
+        <Registration routeId="channels" handler={handler} />
+        <Action routeId="schedules" />
+        <Action routeId="channels" />
+      </PrimaryActionProvider>
+    );
+
+    expect(screen.getByRole('button', { name: 'Run schedules' })).toBeDisabled();
+    expect(screen.getByRole('button', { name: 'Run channels' })).toBeEnabled();
+    fireEvent.click(screen.getByRole('button', { name: 'Run channels' }));
+    expect(handler).toHaveBeenCalledOnce();
+
+    view.rerender(
+      <PrimaryActionProvider>
+        <Action routeId="schedules" />
+        <Action routeId="channels" />
+      </PrimaryActionProvider>
+    );
+
+    expect(screen.getByRole('button', { name: 'Run channels' })).toBeDisabled();
+  });
+
+  it('does not let an older owner rerender or cleanup reclaim a newer registration', () => {
+    const olderHandler = vi.fn();
+    const updatedOlderHandler = vi.fn();
+    const newerHandler = vi.fn();
+    const view = render(
+      <PrimaryActionProvider>
+        <Registration key="older" routeId="schedules" handler={olderHandler} />
+        <Action routeId="schedules" />
+      </PrimaryActionProvider>
+    );
+
+    view.rerender(
+      <PrimaryActionProvider>
+        <Registration key="older" routeId="schedules" handler={olderHandler} />
+        <Registration key="newer" routeId="schedules" handler={newerHandler} />
+        <Action routeId="schedules" />
+      </PrimaryActionProvider>
+    );
+    view.rerender(
+      <PrimaryActionProvider>
+        <Registration key="older" routeId="schedules" handler={updatedOlderHandler} />
+        <Registration key="newer" routeId="schedules" handler={newerHandler} />
+        <Action routeId="schedules" />
+      </PrimaryActionProvider>
+    );
+    fireEvent.click(screen.getByRole('button', { name: 'Run schedules' }));
+    expect(newerHandler).toHaveBeenCalledOnce();
+
+    view.rerender(
+      <PrimaryActionProvider>
+        <Registration key="newer" routeId="schedules" handler={newerHandler} />
+        <Action routeId="schedules" />
+      </PrimaryActionProvider>
+    );
+    fireEvent.click(screen.getByRole('button', { name: 'Run schedules' }));
+
+    expect(olderHandler).not.toHaveBeenCalled();
+    expect(updatedOlderHandler).not.toHaveBeenCalled();
+    expect(newerHandler).toHaveBeenCalledTimes(2);
+  });
+
+  it('lets screens render harmlessly without a provider', () => {
+    expect(() => render(<Registration routeId="schedules" handler={vi.fn()} />)).not.toThrow();
+  });
+});

web/src/primaryAction.ts

@@ -1,39 +1,68 @@
-import { useEffect, useRef } from 'react';
+import {
+  createContext,
+  createElement,
+  type ReactNode,
+  useCallback,
+  useContext,
+  useEffect,
+  useMemo,
+  useRef,
+  useState
+} from 'react';
 
-// The TopBar renders one "primary action" button per screen (top-right). Because the
-// TopBar and the screens are decoupled (the TopBar has no reference to the active
-// screen component), the click is delivered as a window CustomEvent keyed on the
-// active route id; a screen opts in with `usePrimaryAction(routeId, handler)`.
-//
-// A screen that does NOT call usePrimaryAction gets NO button — App.tsx's route table
-// declares an empty `primaryAction` for such screens and the TopBar suppresses the
-// button (see issue #238: the old code rendered a dead button for every unwired route).
-export const PRIMARY_ACTION_EVENT = 'ctv:primary-action';
+type PrimaryActionHandler = () => void;
 
-export function dispatchPrimaryAction(routeId: string): void {
-  window.dispatchEvent(new CustomEvent(PRIMARY_ACTION_EVENT, { detail: routeId }));
+interface PrimaryActionRegistration {
+  owner: symbol;
+  routeId: string;
+  handler: PrimaryActionHandler;
 }
 
-/**
- * Subscribe a screen to its TopBar primary-action button. The handler runs whenever the
- * TopBar dispatches `ctv:primary-action` with a detail matching `routeId`. The latest
- * handler is held in a ref so re-renders don't churn the window listener.
- */
-export function usePrimaryAction(routeId: string, handler: () => void): void {
+interface PrimaryActionContextValue {
+  registration: PrimaryActionRegistration | undefined;
+  register: (owner: symbol, routeId: string, handler: PrimaryActionHandler) => () => void;
+}
+
+const PrimaryActionContext = createContext<PrimaryActionContextValue | undefined>(undefined);
+
+/** Owns the single primary-action registration shared by the shell and active screen. */
+export function PrimaryActionProvider({ children }: { children: ReactNode }) {
+  const [registration, setRegistration] = useState<PrimaryActionRegistration>();
+
+  const register = useCallback((owner: symbol, routeId: string, handler: PrimaryActionHandler) => {
+    setRegistration({ owner, routeId, handler });
+
+    return () => {
+      setRegistration((current) => (current?.owner === owner ? undefined : current));
+    };
+  }, []);
+
+  const value = useMemo(() => ({ registration, register }), [register, registration]);
+
+  return createElement(PrimaryActionContext.Provider, { value }, children);
+}
+
+/** Register a screen's primary action. Rendering outside the provider is intentionally harmless. */
+export function usePrimaryAction(routeId: string, handler: PrimaryActionHandler): void {
   const handlerRef = useRef(handler);
-  // Keep the ref pointing at the latest handler without re-subscribing the window listener
-  // on every render. Updating a ref during render trips react-hooks/refs, so do it in an effect.
+  const [owner] = useState(() => Symbol('primary-action-owner'));
+  const register = useContext(PrimaryActionContext)?.register;
+
   useEffect(() => {
     handlerRef.current = handler;
   });
 
   useEffect(() => {
-    const listener = (event: Event) => {
-      if ((event as CustomEvent<string>).detail === routeId) {
-        handlerRef.current();
-      }
-    };
-    window.addEventListener(PRIMARY_ACTION_EVENT, listener);
-    return () => window.removeEventListener(PRIMARY_ACTION_EVENT, listener);
-  }, [routeId]);
+    if (!register) {
+      return;
+    }
+
+    return register(owner, routeId, () => handlerRef.current());
+  }, [owner, register, routeId]);
+}
+
+/** Return the active handler only when its registration belongs to the requested route. */
+export function usePrimaryActionHandler(routeId: string): PrimaryActionHandler | undefined {
+  const registration = useContext(PrimaryActionContext)?.registration;
+  return registration?.routeId === routeId ? registration.handler : undefined;
 }