From ded443647d735f52b1ea96590a267b4d35faab8e Mon Sep 17 00:00:00 2001 From: James Date: Thu, 16 Jul 2026 14:44:07 -0400 Subject: [PATCH] fix(studio): address storyboard review feedback --- .../studio/src/components/StudioHeader.tsx | 5 +- .../AgentChatMessageButton.test.tsx | 47 ++++++ .../storyboard/AgentChatMessageButton.tsx | 28 +++- .../storyboard/StoryboardFrameFocus.tsx | 32 ++-- .../storyboard/StoryboardLoaded.tsx | 10 +- .../storyboard/StoryboardReviewGuide.tsx | 34 +++-- .../StoryboardViewModeGuard.test.tsx | 143 ++++++++++++++++++ .../storyboard/storyboardReviewStage.test.ts | 16 +- .../storyboard/storyboardReviewStage.ts | 6 +- .../studio/src/contexts/ViewModeContext.tsx | 23 ++- 10 files changed, 291 insertions(+), 53 deletions(-) create mode 100644 packages/studio/src/components/storyboard/AgentChatMessageButton.test.tsx create mode 100644 packages/studio/src/components/storyboard/StoryboardViewModeGuard.test.tsx diff --git a/packages/studio/src/components/StudioHeader.tsx b/packages/studio/src/components/StudioHeader.tsx index 138b65ce3..f6110cd9f 100644 --- a/packages/studio/src/components/StudioHeader.tsx +++ b/packages/studio/src/components/StudioHeader.tsx @@ -149,14 +149,13 @@ const VIEW_MODE_OPTIONS: Array<{ mode: StudioViewMode; label: string }> = [ ]; /** Segmented control switching the main stage between storyboard and preview. */ -function ViewModeToggle() { +export function ViewModeToggle() { const { viewMode, setViewMode } = useViewMode(); const tabRefs = useRef>([]); const selectMode = (mode: StudioViewMode) => { if (mode === viewMode) return; - trackStudioEvent("view_mode_toggle", { mode }); - setViewMode(mode); + if (setViewMode(mode)) trackStudioEvent("view_mode_toggle", { mode }); }; // Complete APG tabs pattern: roving tabIndex + arrow-key navigation. diff --git a/packages/studio/src/components/storyboard/AgentChatMessageButton.test.tsx b/packages/studio/src/components/storyboard/AgentChatMessageButton.test.tsx new file mode 100644 index 000000000..3c72fa21c --- /dev/null +++ b/packages/studio/src/components/storyboard/AgentChatMessageButton.test.tsx @@ -0,0 +1,47 @@ +// @vitest-environment happy-dom + +import React, { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { AgentChatMessageButton } from "./AgentChatMessageButton"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +afterEach(() => { + document.body.innerHTML = ""; + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +describe("AgentChatMessageButton", () => { + it("reports a successful copy and resets to an explicit re-copy action", async () => { + vi.useFakeTimers(); + const writeText = vi.fn().mockResolvedValue(undefined); + Object.defineProperty(navigator, "clipboard", { + configurable: true, + value: { writeText }, + }); + const onCopied = vi.fn(); + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + act(() => { + root.render(); + }); + + const button = host.querySelector("button"); + if (!button) throw new Error("copy button not rendered"); + await act(async () => { + button.dispatchEvent(new MouseEvent("click", { bubbles: true })); + await Promise.resolve(); + }); + + expect(writeText).toHaveBeenCalledWith("handoff"); + expect(onCopied).toHaveBeenCalledOnce(); + expect(button.textContent).toBe("Copied — paste in your agent chat"); + + act(() => vi.advanceTimersByTime(3000)); + expect(button.textContent).toBe("Copy again"); + act(() => root.unmount()); + }); +}); diff --git a/packages/studio/src/components/storyboard/AgentChatMessageButton.tsx b/packages/studio/src/components/storyboard/AgentChatMessageButton.tsx index 9aa56a95c..e00446fe3 100644 --- a/packages/studio/src/components/storyboard/AgentChatMessageButton.tsx +++ b/packages/studio/src/components/storyboard/AgentChatMessageButton.tsx @@ -1,21 +1,31 @@ -import { useState } from "react"; +import { useEffect, useState } from "react"; import { Button } from "../ui/Button"; -export const APPLY_STORYBOARD_FEEDBACK_MESSAGE = "Apply my saved storyboard feedback."; +export const APPLY_STORYBOARD_FEEDBACK_MESSAGE = + "Read the storyboard feedback I saved in .hyperframes/frame-comments.json and revise the frames."; export function AgentChatMessageButton({ message, - label = "Copy agent message", + label = "Copy prompt for agent", + onCopied, }: { message: string; label?: string; + onCopied?: () => void; }) { - const [copyState, setCopyState] = useState<"idle" | "copied" | "failed">("idle"); + const [copyState, setCopyState] = useState<"idle" | "copied" | "again" | "failed">("idle"); + + useEffect(() => { + if (copyState !== "copied") return; + const timeout = window.setTimeout(() => setCopyState("again"), 3000); + return () => window.clearTimeout(timeout); + }, [copyState]); const copyMessage = async () => { try { await navigator.clipboard.writeText(message); setCopyState("copied"); + onCopied?.(); } catch { setCopyState("failed"); } @@ -24,10 +34,12 @@ export function AgentChatMessageButton({ return ( ); } diff --git a/packages/studio/src/components/storyboard/StoryboardFrameFocus.tsx b/packages/studio/src/components/storyboard/StoryboardFrameFocus.tsx index 33f077ff5..aa1f3b583 100644 --- a/packages/studio/src/components/storyboard/StoryboardFrameFocus.tsx +++ b/packages/studio/src/components/storyboard/StoryboardFrameFocus.tsx @@ -35,6 +35,7 @@ export interface StoryboardFrameFocusProps { commentsSubmitState: CommentsSubmitState; commentsSubmitError: string | null; feedbackMessageCopied: boolean; + onFeedbackMessageCopied: () => void; onSaveFeedback: () => void; /** Project signature the board was loaded with (busts the poster cache). */ posterVersion?: string; @@ -67,11 +68,12 @@ export function StoryboardFrameFocus({ commentsSubmitState, commentsSubmitError, feedbackMessageCopied, + onFeedbackMessageCopied, onSaveFeedback, posterVersion, }: StoryboardFrameFocusProps) { const { readProjectFile, writeProjectFile } = useFileManagerContext(); - const { setViewMode } = useViewMode(); + const { setViewMode, registerViewModeGuard } = useViewMode(); const [draft, setDraft] = useState(frame.voiceover ?? ""); const [savedVoiceover, setSavedVoiceover] = useState(frame.voiceover ?? ""); const [busy, setBusy] = useState(false); @@ -121,7 +123,12 @@ export function StoryboardFrameFocus({ // dirty. An in-flight save does NOT count as safe: if it fails after unmount // the error lands on an unmounted component and the draft is silently lost, // so keep confirming until the save actually lands (dirty clears on success). - const confirmLeave = () => !dirty || window.confirm("Discard unsaved voiceover changes?"); + const confirmLeave = useCallback( + () => !dirty || window.confirm("Discard unsaved voiceover changes?"), + [dirty], + ); + useEffect(() => registerViewModeGuard(confirmLeave), [confirmLeave, registerViewModeGuard]); + const handleBack = () => { if (confirmLeave()) onBack(); }; @@ -144,8 +151,8 @@ export function StoryboardFrameFocus({ }); const openInPreview = () => { + if (!setViewMode("timeline")) return; if (frame.src) onSelectComposition(frame.src); - setViewMode("timeline"); }; return ( @@ -185,7 +192,7 @@ export function StoryboardFrameFocus({ {commentDraftCount > 0 ? "Save this batch and copy the message for your agent." : feedbackMessageCopied - ? "Message copied — paste it in agent chat to continue." + ? "Message copied — paste it in your terminal or IDE agent chat." : "The agent has not been notified yet."} @@ -201,7 +208,8 @@ export function StoryboardFrameFocus({ ) : ( )} @@ -235,7 +243,10 @@ export function StoryboardFrameFocus({
-

+

🎙 Voiceover guide

@@ -141,6 +143,7 @@ export function StoryboardLoaded({ frames={data.frames} draftCount={comments.draftCount} pendingCount={comments.pending?.length ?? 0} + onFeedbackMessageCopied={() => setFeedbackMessageCopied(true)} /> void; + onMessageCopied: () => void; }) { return (
@@ -191,12 +196,13 @@ function CommentsSubmitBar({ <> {messageCopied - ? "Feedback saved · Message copied — paste it in agent chat." + ? "Feedback saved · Message copied — paste it in your terminal or IDE agent chat." : "Feedback saved · Agent not notified."} )} diff --git a/packages/studio/src/components/storyboard/StoryboardReviewGuide.tsx b/packages/studio/src/components/storyboard/StoryboardReviewGuide.tsx index aad5b6419..ea2d9e4ed 100644 --- a/packages/studio/src/components/storyboard/StoryboardReviewGuide.tsx +++ b/packages/studio/src/components/storyboard/StoryboardReviewGuide.tsx @@ -21,7 +21,7 @@ const GUIDE_COPY: Record void; } /** Stage-aware instructions plus the explicit Studio → agent handoff. */ @@ -117,6 +118,7 @@ export function StoryboardReviewGuide({ frames, draftCount, pendingCount, + onFeedbackMessageCopied, }: StoryboardReviewGuideProps) { const summary = deriveStoryboardReviewStage(frames); const copy = GUIDE_COPY[summary.stage]; @@ -155,7 +157,12 @@ export function StoryboardReviewGuide({ {summary.frameCount > 0 && (
- +
)}
@@ -217,27 +224,34 @@ function NextAction({ stage, step, draftCount, + onFeedbackMessageCopied, }: { stage: StoryboardReviewStage; step: StoryboardHandoffStep; draftCount: number; + onFeedbackMessageCopied: () => void; }) { - if (step === 3) return ; + if (step === 3) { + return ; + } if (step === 2) return ; return ; } -function AgentHandoffAction() { +function AgentHandoffAction({ onFeedbackMessageCopied }: { onFeedbackMessageCopied: () => void }) { return (
-
Next: return to agent chat
+
Next: return to your agent chat

- Feedback is saved, but the agent has not been notified. Paste this message in chat: “ - {APPLY_STORYBOARD_FEEDBACK_MESSAGE}” + Feedback is saved, but the agent has not been notified. Paste this prompt in your terminal + or IDE agent chat.

- +
); } diff --git a/packages/studio/src/components/storyboard/StoryboardViewModeGuard.test.tsx b/packages/studio/src/components/storyboard/StoryboardViewModeGuard.test.tsx new file mode 100644 index 000000000..ed537d8f7 --- /dev/null +++ b/packages/studio/src/components/storyboard/StoryboardViewModeGuard.test.tsx @@ -0,0 +1,143 @@ +// @vitest-environment happy-dom + +import React, { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ViewModeProvider, useViewModeState } from "../../contexts/ViewModeContext"; +import { ViewModeToggle } from "../StudioHeader"; +import { StoryboardFrameFocus } from "./StoryboardFrameFocus"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +vi.mock("../../contexts/FileManagerContext", () => ({ + useFileManagerContext: () => ({ + readProjectFile: vi.fn(), + writeProjectFile: vi.fn(), + }), +})); + +vi.mock("../../utils/studioTelemetry", () => ({ + trackStudioEvent: vi.fn(), +})); + +vi.mock("./FramePoster", () => ({ + FramePoster: () =>
poster
, + posterTime: () => 0, +})); + +const onSelectComposition = vi.fn(); + +function TestApp() { + const viewMode = useViewModeState(); + return ( + + {viewMode.viewMode} + + {viewMode.viewMode === "storyboard" && ( + + )} + + ); +} + +function renderApp(): { host: HTMLDivElement; root: Root } { + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + act(() => root.render()); + return { host, root }; +} + +function makeVoiceoverDirty(host: HTMLElement): void { + const textarea = host.querySelector( + 'textarea[placeholder^="What the narrator says"]', + ); + if (!textarea) throw new Error("voiceover textarea not rendered"); + const valueSetter = Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, "value")?.set; + act(() => { + valueSetter?.call(textarea, "Changed voiceover"); + textarea.dispatchEvent(new Event("input", { bubbles: true })); + }); +} + +function clickButton(host: HTMLElement, label: string): void { + const button = [...host.querySelectorAll("button")].find( + (candidate) => candidate.textContent?.trim() === label, + ); + if (!button) throw new Error(`button not found: ${label}`); + act(() => button.dispatchEvent(new MouseEvent("click", { bubbles: true }))); +} + +beforeEach(() => { + window.history.replaceState({}, "", "/?view=storyboard"); + onSelectComposition.mockReset(); +}); + +afterEach(() => { + document.body.innerHTML = ""; + vi.restoreAllMocks(); +}); + +describe("dirty storyboard voiceover view-mode guard", () => { + it("guards the header Preview transition on decline and allows it on accept", () => { + const confirm = vi.spyOn(window, "confirm").mockReturnValue(false); + const { host, root } = renderApp(); + makeVoiceoverDirty(host); + + clickButton(host, "Preview"); + expect(host.querySelector("[data-view-mode]")?.textContent).toBe("storyboard"); + expect(confirm).toHaveBeenCalledWith("Discard unsaved voiceover changes?"); + + confirm.mockReturnValue(true); + clickButton(host, "Preview"); + expect(host.querySelector("[data-view-mode]")?.textContent).toBe("timeline"); + expect(onSelectComposition).not.toHaveBeenCalled(); + act(() => root.unmount()); + }); + + it("guards Open in Preview on decline and selects the frame only after accept", () => { + const confirm = vi.spyOn(window, "confirm").mockReturnValue(false); + const { host, root } = renderApp(); + makeVoiceoverDirty(host); + + clickButton(host, "Open in Preview →"); + expect(host.querySelector("[data-view-mode]")?.textContent).toBe("storyboard"); + expect(onSelectComposition).not.toHaveBeenCalled(); + + confirm.mockReturnValue(true); + clickButton(host, "Open in Preview →"); + expect(host.querySelector("[data-view-mode]")?.textContent).toBe("timeline"); + expect(onSelectComposition).toHaveBeenCalledWith("frames/01-opening.html"); + act(() => root.unmount()); + }); +}); diff --git a/packages/studio/src/components/storyboard/storyboardReviewStage.test.ts b/packages/studio/src/components/storyboard/storyboardReviewStage.test.ts index 6ae80079d..18982f5ea 100644 --- a/packages/studio/src/components/storyboard/storyboardReviewStage.test.ts +++ b/packages/studio/src/components/storyboard/storyboardReviewStage.test.ts @@ -1,9 +1,5 @@ import { describe, expect, it } from "vitest"; -import { - deriveStoryboardHandoffStep, - deriveStoryboardReviewStage, - isReviewReadyStage, -} from "./storyboardReviewStage"; +import { deriveStoryboardHandoffStep, deriveStoryboardReviewStage } from "./storyboardReviewStage"; describe("deriveStoryboardReviewStage", () => { it.each([ @@ -37,16 +33,8 @@ describe("storyboard review handoff", () => { [0, 0, 1], [2, 0, 2], [0, 3, 3], - [1, 3, 3], + [1, 3, 2], ] as const)("maps %i drafts and %i pending comments to step %i", (drafts, pending, step) => { expect(deriveStoryboardHandoffStep(drafts, pending)).toBe(step); }); - - it("only offers approval when a review pass is complete", () => { - expect(isReviewReadyStage("plan-review")).toBe(true); - expect(isReviewReadyStage("sketch-review")).toBe(true); - expect(isReviewReadyStage("final-review")).toBe(true); - expect(isReviewReadyStage("sketch-in-progress")).toBe(false); - expect(isReviewReadyStage("animation-in-progress")).toBe(false); - }); }); diff --git a/packages/studio/src/components/storyboard/storyboardReviewStage.ts b/packages/studio/src/components/storyboard/storyboardReviewStage.ts index 6fa83c5b8..6887d8012 100644 --- a/packages/studio/src/components/storyboard/storyboardReviewStage.ts +++ b/packages/studio/src/components/storyboard/storyboardReviewStage.ts @@ -21,15 +21,11 @@ export function deriveStoryboardHandoffStep( draftCount: number, pendingCount: number, ): StoryboardHandoffStep { - if (pendingCount > 0) return 3; if (draftCount > 0) return 2; + if (pendingCount > 0) return 3; return 1; } -export function isReviewReadyStage(stage: StoryboardReviewStage): boolean { - return stage === "plan-review" || stage === "sketch-review" || stage === "final-review"; -} - /** Derive the board-level review moment from agent-owned frame statuses. */ export function deriveStoryboardReviewStage( frames: ReadonlyArray<{ status: FrameStatus }>, diff --git a/packages/studio/src/contexts/ViewModeContext.tsx b/packages/studio/src/contexts/ViewModeContext.tsx index f9a17dfc3..365367e37 100644 --- a/packages/studio/src/contexts/ViewModeContext.tsx +++ b/packages/studio/src/contexts/ViewModeContext.tsx @@ -4,6 +4,7 @@ import { useContext, useEffect, useMemo, + useRef, useState, type ReactNode, } from "react"; @@ -17,6 +18,7 @@ import { * user straight into the storyboard by navigating the tab to `?view=storyboard`. */ export type StudioViewMode = "timeline" | "storyboard"; +export type ViewModeGuard = (nextMode: StudioViewMode) => boolean; const VIEW_QUERY_PARAM = "view"; @@ -40,7 +42,9 @@ function writeViewModeToUrl(mode: StudioViewMode): void { export interface ViewModeValue { viewMode: StudioViewMode; - setViewMode: (mode: StudioViewMode) => void; + /** Returns false when an active editor vetoes the transition. */ + setViewMode: (mode: StudioViewMode) => boolean; + registerViewModeGuard: (guard: ViewModeGuard) => () => void; } /** @@ -49,6 +53,7 @@ export interface ViewModeValue { */ export function useViewModeState(): ViewModeValue { const [viewMode, setMode] = useState(() => readViewModeFromUrl()); + const guardsRef = useRef(new Set()); // Reflect genuine browser back/forward between history entries with a different // `?view=`. Note: our own writes use `replaceState` (below), which does NOT fire @@ -63,11 +68,25 @@ export function useViewModeState(): ViewModeValue { }, []); const setViewMode = useCallback((mode: StudioViewMode) => { + for (const guard of guardsRef.current) { + if (!guard(mode)) return false; + } setMode(mode); writeViewModeToUrl(mode); + return true; }, []); - return useMemo(() => ({ viewMode, setViewMode }), [viewMode, setViewMode]); + const registerViewModeGuard = useCallback((guard: ViewModeGuard) => { + guardsRef.current.add(guard); + return () => { + guardsRef.current.delete(guard); + }; + }, []); + + return useMemo( + () => ({ viewMode, setViewMode, registerViewModeGuard }), + [viewMode, setViewMode, registerViewModeGuard], + ); } const ViewModeContext = createContext(null);