diff --git a/AGENTS.md b/AGENTS.md index eff9a964..081c3481 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,6 +148,43 @@ Before commit/push/PR: use the **`pre-push-review`** skill (`~/.cursor/skills/pr - **Permission modes**: `default`, `acceptEdits`, `bypassPermissions`, `plan` - **Namespaces**: Multi-user isolation via `CLI_API_TOKEN:` suffix +## Adding new web features — consider an FUE + +When you ship a non-essential feature (the 20% of sessions, not the 80%), consider wrapping its affordance in the generic First-User-Experience primitive so existing users discover it without a giant always-visible UI block. + +- **Hook**: `web/src/lib/use-fue.ts` — `useFue(featureId)` returns `{ status, engage, dismiss }`. Storage namespace `hapi.fue.v1.` (one localStorage key per feature, isolated from any upstream onboarding flow). +- **Components**: `web/src/components/Fue.tsx` — `` (small pulsing badge for the affordance) and `` (portal-rendered popover with title/body + "Got it" affirmative-action dismiss). + +Pattern (~10 lines around the affordance): + +```tsx +const fue = useFue('my-feature') +const buttonRef = useRef(null) +return ( + <> + + {fue.status === 'engaging' ? ( + + ) : null} + +) +``` + +Rules: +- Affirmative action only: there is no auto-timeout — user dismisses by clicking "Got it" (reading speed varies). +- The FUE dot and any feature-specific badge (e.g. an entry counter) should be **mutually exclusive**: onboarding signal beats inventory signal until acknowledged. +- Storage is opt-in per-feature; if upstream ships its own onboarding for a feature, just don't wrap that affordance. + +Canonical example: scratchlist toggle in `web/src/components/AssistantChat/ComposerButtons.tsx` (`ScratchlistToggleButton`). + ## Critical Thinking 1. Fix root cause (not band-aid). diff --git a/web/src/components/AssistantChat/ComposerButtons.test.tsx b/web/src/components/AssistantChat/ComposerButtons.test.tsx new file mode 100644 index 00000000..90ed25be --- /dev/null +++ b/web/src/components/AssistantChat/ComposerButtons.test.tsx @@ -0,0 +1,85 @@ +import type { ReactElement } from 'react' +import { cleanup, render, screen } from '@testing-library/react' +import { afterEach, describe, expect, it } from 'vitest' +import { I18nProvider } from '@/lib/i18n-context' +import { UnifiedButton } from './ComposerButtons' + +function renderInProviders(ui: ReactElement) { + return render({ui}) +} + +/** + * Regression tests for upstream review on PR #798 + * (github-actions[bot] [Major]: "Send button advertises scratchlist + * routing even when the submit will go to chat"). + * + * UnifiedButton's visible state (amber + "Send to scratchlist" label + * vs. black + "Send message" label) MUST reflect the actual routing + * decision rather than the raw scratchlist toggle. Callers are + * responsible for computing routesToScratchlist from + * (mode, attachments, schedule); these tests pin the contract that + * routesToScratchlist=false drives the chat-style render. + */ + +function getButton(label: RegExp | string): HTMLButtonElement { + return screen.getByRole('button', { name: label }) as HTMLButtonElement +} + +describe('UnifiedButton — routesToScratchlist visual state', () => { + const noop = () => {} + + afterEach(() => { + cleanup() + }) + + it('paints amber + announces "Send to scratchlist" when routesToScratchlist=true', () => { + renderInProviders( + , + ) + const btn = getButton(/scratchlist/i) + expect(btn.className).toContain('bg-amber-500') + }) + + it('paints chat black + announces "Send" when routesToScratchlist=false even if scratchlist toggle conceptually on', () => { + // Caller computed routesToScratchlist=false because the payload + // would carry attachments or a pending schedule. The button must + // therefore look like a normal chat send. + renderInProviders( + , + ) + const btn = getButton('Send') + expect(btn.className).not.toContain('bg-amber-500') + expect(btn.className).toContain('bg-black') + }) + + it('defaults routesToScratchlist to false when omitted', () => { + renderInProviders( + , + ) + const btn = getButton('Send') + expect(btn.className).not.toContain('bg-amber-500') + }) +}) diff --git a/web/src/components/AssistantChat/ComposerButtons.tsx b/web/src/components/AssistantChat/ComposerButtons.tsx index 28d1fcc8..5b0325a6 100644 --- a/web/src/components/AssistantChat/ComposerButtons.tsx +++ b/web/src/components/AssistantChat/ComposerButtons.tsx @@ -4,6 +4,8 @@ import { useTranslation } from '@/lib/use-translation' import { ScheduleIcon } from '@/components/icons' import { ScheduleTimePicker } from './ScheduleTimePicker' import type { PendingSchedule } from './ScheduleTimePicker' +import { useFue } from '@/lib/use-fue' +import { FueCallout, FueDot } from '@/components/Fue' import { useRef, useState } from 'react' function VoiceAssistantIcon() { @@ -198,6 +200,101 @@ function SendIcon() { ) } +function ScratchlistToggleIcon() { + return ( + + + + + ) +} + +/** + * ScratchlistToggleButton — composer affordance for toggling scratchlist mode, + * wrapped in the generic FUE (First-User Experience) primitive so a new + * operator sees a pulsing dot + a one-time explainer popover the first time + * they encounter the feature; once they engage with it, the dot disappears + * for good and the entry counter takes over. + * + * The FUE wiring here is the canonical example for future features: the + * pattern is "wrap the affordance in useFue + FueDot, conditionally render + * FueCallout while engaging". See web/src/lib/use-fue.ts for the contract. + */ +function ScratchlistToggleButton(props: { + scratchlistMode: boolean + scratchlistCount: number + onScratchlistToggle: () => void + controlsDisabled?: boolean +}) { + const { t } = useTranslation() + const fue = useFue('scratchlist-toggle') + const buttonRef = useRef(null) + + const showFueDot = fue.status !== 'acknowledged' + // Counter and FUE dot are mutually exclusive (see FueDot doc comment). + // Onboarding signal beats inventory signal: the user can't read the + // counter as "you have N items" until they understand the feature. + const showCounter = !showFueDot && props.scratchlistCount > 0 + + return ( + <> + + {fue.status === 'engaging' ? ( + + ) : null} + + ) +} + function StopIcon() { return ( void onVoiceToggle: () => void + /** + * When true, the send button repaints amber and the aria-label + * announces "Send to scratchlist" instead of "Send message". The + * actual routing happens in SessionChat's wrapped onSend - the + * button itself is content-agnostic. + * + * Caller MUST compute this from the actual routing decision (mode + * AND no-attachments AND no-pending-schedule), not the raw + * scratchlist toggle. If the toggle is on but the submission would + * fall back to chat (because the scratchlist can't represent the + * payload), the button must look like a normal chat send. Per + * upstream review on PR #798: [Major] "Send button advertises + * scratchlist routing even when the submit will go to chat". + */ + routesToScratchlist?: boolean }) { const { t } = useTranslation() - // Determine button state const isConnecting = props.voiceStatus === 'connecting' const isConnected = props.voiceStatus === 'connected' const isVoiceActive = isConnecting || isConnected const hasText = props.canSend + const routesToScratchlist = props.routesToScratchlist ?? false - // Determine button behavior const handleClick = () => { if (isVoiceActive) { props.onVoiceToggle() // Stop voice } else if (hasText) { - props.onSend() // Send message - } else if (props.voiceEnabled) { - props.onVoiceToggle() // Start voice + props.onSend() // Send message (or scratchlist add — wrapper decides) + } else if (props.voiceEnabled && !routesToScratchlist) { + props.onVoiceToggle() // Start voice (suppressed in scratchlist mode) } } - // Determine button style and icon let icon: React.ReactNode let className: string let ariaLabel: string @@ -270,6 +380,13 @@ function UnifiedButton(props: { icon = className = 'bg-black text-white' ariaLabel = t('composer.stop') + } else if (routesToScratchlist) { + // Amber send button - matches the scratchlist drawer accent. + // Single visual signal carries the "this goes to the scratchlist" + // contract; without it, the modal state is invisible to the user. + icon = + className = 'bg-amber-500 text-white hover:bg-amber-600' + ariaLabel = t('scratchlist.sendToScratchlist') } else if (hasText) { icon = className = 'bg-black text-white' @@ -284,7 +401,16 @@ function UnifiedButton(props: { ariaLabel = t('composer.send') } - const isDisabled = props.controlsDisabled || (!hasText && !props.voiceEnabled && !isVoiceActive) + // When the submission routes to scratchlist the send button is the + // only path that does anything useful, so it must be enabled whenever + // there is text - we deliberately do NOT fall back to voice-toggle-on- + // empty-text. (When attachments / schedule force a chat fallback the + // normal chat-send disable rules apply.) + const isDisabled = props.controlsDisabled || ( + routesToScratchlist + ? !hasText + : !hasText && !props.voiceEnabled && !isVoiceActive + ) return ( ) : null} + {/* + * Scratchlist toggle - prototype of the composer-controlled + * drawer (replaces the always-visible orange band). Counter + * shown only when entries exist (>0); empty-state shows just + * the icon to avoid the "you have 0 things" guilt UI. + * + * Clicking enters scratchlist mode: the send button repaints + * amber and SessionChat's wrapped onSend routes the next + * submission to addScratchlistEntry() instead of the chat. + * Mode is sticky - operator clicks the icon again to exit. + */} + {props.onScratchlistToggle ? ( + + ) : null} + {/* Schedule button — only shown when onSchedule handler is provided */} {props.onSchedule ? ( <> @@ -466,6 +619,21 @@ export function ComposerButtons(props: { controlsDisabled={props.controlsDisabled} onSend={props.onSend} onVoiceToggle={props.onVoiceToggle} + /* + * Derived, NOT raw scratchlistMode. Mirror SessionChat's + * shouldRouteToScratchlist so the visible send-button state + * matches the actual routing decision: amber + "Send to + * scratchlist" only when mode is on AND the payload would + * be a pure-text scratchlist add. Attachments or a pending + * schedule force a chat fallback in onSendForComposer; the + * button must reflect that, otherwise the UI lies about + * where the user's content is going. + */ + routesToScratchlist={ + (props.scratchlistMode ?? false) + && !hasAttachments + && props.pendingSchedule == null + } /> ) diff --git a/web/src/components/AssistantChat/HappyComposer.tsx b/web/src/components/AssistantChat/HappyComposer.tsx index c47dcdae..96ae5753 100644 --- a/web/src/components/AssistantChat/HappyComposer.tsx +++ b/web/src/components/AssistantChat/HappyComposer.tsx @@ -40,6 +40,31 @@ export interface TextInputState { selection: { start: number; end: number } } +/** + * One rejected send. `id` is bumped per failure so two failures with the + * same `text` still trigger a fresh restore (the dedupe key is the id, not + * the text). + * + * - `text` is the original input that should be put back into the composer. + * - `message` is the user-facing error string we render inline. + * - `scheduledAt` is the absolute epoch-ms the rejected send was bound for, + * or null for an immediate send. When non-null, the composer also + * restores the schedule via `onSchedule` so the operator can edit and + * retry without silently downgrading a scheduled send to immediate. + * + * Owned by the route component (`router.tsx`); the composer is a pure + * consumer that: + * 1. restores the text once per `id` via `api.composer().setText`, + * 2. restores the schedule (if any) via `onSchedule`, and + * 3. shows a red ring + inline message until the user types or sends. + */ +export type ComposerSendError = { + id: number + text: string + message: string + scheduledAt: number | null +} + const defaultSuggestionHandler = async (): Promise => [] export function HappyComposer(props: { @@ -89,6 +114,17 @@ export function HappyComposer(props: { pendingSchedule?: PendingSchedule | null onSchedule?: (pending: PendingSchedule) => void onClearSchedule?: () => void + // Scratchlist drawer props - SessionChat owns the state. Threaded + // straight through to ComposerButtons. When undefined, the toggle + // button doesn't render (back-compat for any other consumer). + scratchlistMode?: boolean + scratchlistCount?: number + onScratchlistToggle?: () => void + // Set when the most recent send failed (4xx/5xx/network). The composer + // restores the original text once per `sendError.id` and renders an + // inline error affordance until the user dismisses or starts editing. + sendError?: ComposerSendError | null + onClearSendError?: () => void }) { const { t } = useTranslation() const { @@ -131,7 +167,9 @@ export function HappyComposer(props: { onVoiceMicToggle, pendingSchedule: pendingScheduleProp, onSchedule: onScheduleProp, - onClearSchedule: onClearScheduleProp + onClearSchedule: onClearScheduleProp, + sendError = null, + onClearSendError } = props // Use ?? so missing values fall back to default (destructuring defaults only handle undefined) @@ -183,6 +221,40 @@ export function HappyComposer(props: { useComposerDraft(sessionId, composerText, (text) => api.composer().setText(text)) + // assistant-ui clears `composer.text` synchronously the moment a send is + // invoked AND `SessionChat.handleSend` clears `pendingSchedule` the + // moment the mutation is accepted, so by the time the mutation's + // onError fires both the typed text and the schedule are gone. When + // the route hands us a `sendError`, splice both back in -- once per + // `sendError.id` so a second failure with the same text still triggers + // a fresh restore. + const restoredErrorIdRef = useRef(null) + useEffect(() => { + if (!sendError) { + return + } + if (restoredErrorIdRef.current === sendError.id) { + return + } + restoredErrorIdRef.current = sendError.id + // Only restore when the composer is empty. If the user has already + // typed something new (rare -- composer is `disabled` during send, + // but possible if isSending toggles before this effect runs), we + // would otherwise stomp on their fresh input. + if (composerText.length === 0 && sendError.text.length > 0) { + api.composer().setText(sendError.text) + } + // Restore the pending schedule too. `scheduledAt` was already + // resolved to an absolute epoch-ms before the failed send (presets + // are computed at send time -- see `resolvePendingSchedule`), so + // we feed it back as an 'absolute' PendingSchedule. The existing + // shouldAutoClearPendingSchedule effect in SessionChat handles the + // case where the absolute time has passed by the time we restore. + if (sendError.scheduledAt !== null && onScheduleProp) { + onScheduleProp({ type: 'absolute', ms: sendError.scheduledAt }) + } + }, [sendError, api, composerText, onScheduleProp]) + useEffect(() => { setInputState((prev) => { if (prev.text === composerText) return prev @@ -438,7 +510,13 @@ export function HappyComposer(props: { end: e.target.selectionEnd } setInputState({ text: e.target.value, selection }) - }, []) + // Editing the restored text is the operator's "I'm handling it" + // signal -- drop the inline error so the affordance doesn't shout + // at them while they fix the message. + if (sendError && onClearSendError) { + onClearSendError() + } + }, [sendError, onClearSendError]) const handleSelect = useCallback((e: ReactSyntheticEvent) => { const target = e.target as HTMLTextAreaElement @@ -559,6 +637,11 @@ export function HappyComposer(props: { // and async inactive-session resume failure. Clearing here unconditionally // would race ahead of that check and drop the user's schedule on every // rejected send path. + // + // The inline send-error affordance is intentionally NOT cleared here: + // the route-level state (`onSuccess`/`onError` in router.tsx) replaces + // or clears it based on the actual mutation result, so the user keeps + // the error context while the new attempt is in flight. }, [api]) const overlays = useMemo(() => { @@ -890,7 +973,21 @@ export function HappyComposer(props: { /> ) : null} -
+ {sendError ? ( +
+ {sendError.message} +
+ ) : null} + +
{attachments.length > 0 ? (
@@ -941,6 +1038,9 @@ export function HappyComposer(props: { onSchedule={setPendingSchedule} onClearSchedule={isControlled ? onClearScheduleProp : () => setPendingScheduleLocal(null)} hasAttachments={hasAttachments} + scratchlistMode={props.scratchlistMode} + scratchlistCount={props.scratchlistCount} + onScratchlistToggle={props.onScratchlistToggle} />
diff --git a/web/src/components/AssistantChat/ScratchlistPanel.test.tsx b/web/src/components/AssistantChat/ScratchlistPanel.test.tsx index 06bb4de3..79cd55c6 100644 --- a/web/src/components/AssistantChat/ScratchlistPanel.test.tsx +++ b/web/src/components/AssistantChat/ScratchlistPanel.test.tsx @@ -61,6 +61,21 @@ describe('ScratchlistPanel', () => { expect(toggle.textContent).toContain('held') }) + it('uses the chat user surface for the panel background and keeps a subtle amber border (regression guard for #812)', () => { + // The amber chrome was too loud as an always-visible scroll element + // (#812). The fix swaps the warning *fill* for the chat-user-surface + // tone but keeps the warning *border* as a soft accent so the panel + // still reads as a different destination from a normal user message. + // The strong amber destination signal lives on the composer Send + // button, not here. See PR 827 (swear01) for the styling note this + // test guards. + renderPanel() + const panel = screen.getByTestId('scratchlist-panel') + expect(panel.className).toContain('bg-[var(--app-chat-user-surface-bg)]') + expect(panel.className).not.toContain('bg-[var(--app-badge-warning-bg)]') + expect(panel.className).toContain('border-[var(--app-badge-warning-border)]') + }) + it('starts collapsed by default; clicking the header expands it', () => { renderPanel() const toggle = screen.getByRole('button', { name: /Scratchlist/ }) @@ -208,6 +223,63 @@ describe('ScratchlistPanel', () => { expect(readScratchlist(SID).map((e) => e.id)).toEqual(['a']) }) + it('copy button writes the entry text to clipboard and shows briefly the "Copied" tooltip', async () => { + // Clipboard API isn't implemented in jsdom; install a mock that + // captures the writeText call. (web/src/lib/clipboard.ts already + // tries navigator.clipboard first, then falls back to execCommand.) + const writeText = vi.fn().mockResolvedValue(undefined) + Object.defineProperty(navigator, 'clipboard', { + value: { writeText }, + configurable: true, + }) + + persistScratchlist(SID, [makeEntry({ id: 'a', text: 'copy me' })]) + renderPanel() + expandPanel() + + const copyBtn = screen.getByRole('button', { name: 'Copy to clipboard' }) + fireEvent.click(copyBtn) + + await waitFor(() => expect(writeText).toHaveBeenCalledWith('copy me')) + + // After the async copy resolves, the same button (it stays in the + // DOM, only its label/icon flip) should advertise the success. + await waitFor(() => + expect(screen.getByRole('button', { name: 'Copied!' })).toBeTruthy(), + ) + // Entry is preserved — copy is non-destructive. + expect(readScratchlist(SID).map((e) => e.id)).toEqual(['a']) + }) + + it('clipboard write failure leaves the icon in the default state (no false success)', async () => { + // Force navigator.clipboard.writeText to reject AND make the + // execCommand fallback fail too, so safeCopyToClipboard throws. + const writeText = vi.fn().mockRejectedValue(new Error('denied')) + Object.defineProperty(navigator, 'clipboard', { + value: { writeText }, + configurable: true, + }) + // jsdom doesn't implement document.execCommand. Define a stub + // that returns false so safeCopyToClipboard's fallback path + // also fails (covering the "everything failed" branch). + Object.defineProperty(document, 'execCommand', { + value: () => false, + configurable: true, + writable: true, + }) + + persistScratchlist(SID, [makeEntry({ id: 'a', text: 'try copy' })]) + renderPanel() + expandPanel() + + fireEvent.click(screen.getByRole('button', { name: 'Copy to clipboard' })) + + await waitFor(() => expect(writeText).toHaveBeenCalled()) + // Should NOT flip to "Copied!" because the copy failed. + expect(screen.queryByRole('button', { name: 'Copied!' })).toBeNull() + expect(screen.getByRole('button', { name: 'Copy to clipboard' })).toBeTruthy() + }) + it('persists collapse state across mounts for the same session', () => { const { unmount } = renderPanel() expandPanel() diff --git a/web/src/components/AssistantChat/ScratchlistPanel.tsx b/web/src/components/AssistantChat/ScratchlistPanel.tsx index b9f27451..f684b692 100644 --- a/web/src/components/AssistantChat/ScratchlistPanel.tsx +++ b/web/src/components/AssistantChat/ScratchlistPanel.tsx @@ -18,6 +18,7 @@ import { shouldConfirmDelete, type ScratchlistEntry, } from '@/lib/scratchlist' +import { safeCopyToClipboard } from '@/lib/clipboard' import { useTranslation } from '@/lib/use-translation' const STORAGE_KEY_PREFIX = 'hapi.scratchlist-collapsed.v1.' @@ -134,6 +135,290 @@ function TrashIcon() { ) } +function CopyIcon() { + return ( + + ) +} + +function ClipboardCheckIcon() { + return ( + + ) +} + +/** + * Tracks which entry was most-recently copied to the clipboard so the UI + * can briefly swap the copy icon to a check + the tooltip to "Copied". + * Auto-clears after `clearAfterMs` (default 1500ms). Pure state machine - + * the caller wires `safeCopyToClipboard` separately so the hook stays + * easy to test and free of jsdom clipboard quirks. + */ +const COPIED_FEEDBACK_MS = 1500 +function useCopiedFeedback(clearAfterMs: number = COPIED_FEEDBACK_MS) { + const [copiedEntryId, setCopiedEntryId] = useState(null) + const timerRef = useRef | null>(null) + const signalCopied = useCallback((entryId: string) => { + setCopiedEntryId(entryId) + if (timerRef.current) clearTimeout(timerRef.current) + timerRef.current = setTimeout(() => setCopiedEntryId(null), clearAfterMs) + }, [clearAfterMs]) + useEffect(() => () => { + if (timerRef.current) clearTimeout(timerRef.current) + }, []) + return { copiedEntryId, signalCopied } +} + +/** + * Inventory list with per-entry action buttons. Pure presentational - takes + * entries + callbacks. Used by both the always-visible ScratchlistPanel + * and the composer-controlled drawer below. + */ +function ScratchlistInventory({ + entries, + busyEntryId, + onPromoteToComposer, + onPromoteToQueue, + onDelete, + onMove, +}: { + entries: ScratchlistEntry[] + busyEntryId: string | null + onPromoteToComposer: (entry: ScratchlistEntry) => void + onPromoteToQueue: (entry: ScratchlistEntry) => void + onDelete: (entry: ScratchlistEntry) => void + onMove: (entry: ScratchlistEntry, direction: 'up' | 'down') => void +}) { + const { t } = useTranslation() + const { copiedEntryId, signalCopied } = useCopiedFeedback() + const handleCopy = useCallback(async (entry: ScratchlistEntry) => { + try { + await safeCopyToClipboard(entry.text) + signalCopied(entry.id) + } catch { + // safeCopyToClipboard exhausted both the navigator.clipboard + // path and the execCommand fallback; nothing useful left to do. + // Silently no-op rather than throw at the click handler. + } + }, [signalCopied]) + if (entries.length === 0) { + return ( +

+ {t('scratchlist.emptyHint')} +

+ ) + } + return ( +
    + {entries.map((entry, index) => { + const isFirst = index === 0 + const isLast = index === entries.length - 1 + const isBusy = busyEntryId === entry.id + return ( +
  • + + {entry.text} + +
    + + + + + + +
    +
    • + ) + })} +
+ ) +} + +/** + * Composer-controlled drawer. No own header / no own textarea: the composer + * is the input source (composerSendsToScratchlist toggle in SessionChat). + * + * State is owned by the caller via useScratchlist(). The drawer is purely + * presentational + behavior glue around the inventory list. + */ +export function ScratchlistDrawer({ + entries, + onMove, + onDelete, + onPromoteToComposer, + onPromoteToQueue, +}: { + entries: ScratchlistEntry[] + onMove: (id: string, direction: 'up' | 'down') => void + onDelete: (id: string) => void + onPromoteToComposer: (text: string) => void + onPromoteToQueue: (text: string) => Promise +}) { + const { t } = useTranslation() + const [busyEntryId, setBusyEntryId] = useState(null) + + const summary = useMemo(() => { + if (entries.length === 0) return t('scratchlist.empty') + if (entries.length === 1) return t('scratchlist.count.one') + return t('scratchlist.count.other', { n: entries.length }) + }, [entries.length, t]) + + const handleDelete = useCallback((entry: ScratchlistEntry) => { + if (shouldConfirmDelete(entry)) { + const confirmed = typeof window !== 'undefined' + ? window.confirm(t('scratchlist.confirmDelete')) + : true + if (!confirmed) return + } + onDelete(entry.id) + }, [onDelete, t]) + + const handleMove = useCallback((entry: ScratchlistEntry, direction: 'up' | 'down') => { + onMove(entry.id, direction) + }, [onMove]) + + const handlePromoteToComposer = useCallback((entry: ScratchlistEntry) => { + onPromoteToComposer(entry.text) + }, [onPromoteToComposer]) + + const handlePromoteToQueue = useCallback(async (entry: ScratchlistEntry) => { + if (busyEntryId) return + setBusyEntryId(entry.id) + try { + const accepted = await onPromoteToQueue(entry.text) + if (accepted) onDelete(entry.id) + } finally { + setBusyEntryId(null) + } + }, [busyEntryId, onDelete, onPromoteToQueue]) + + return ( +
+
+
+ + + {t('scratchlist.title')} + + + + {summary} + +
+ +
+

+ {t('scratchlist.drawerHint')} +

+ +
+
+
+ ) +} + /** * Per-session scratchlist (issue #11) -- the operator's "workbench". * @@ -142,8 +427,11 @@ function TrashIcon() { * - Scratchlist = workbench: notes / drafts / parking-lot ideas held until the * operator explicitly promotes them (to the composer or into the queue). * - * The "held -- not sent" pill plus the amber accent is the visual signal - * that nothing here is being sent without an explicit action. + * The "held -- not sent" pill plus a subtle amber border is the visual + * signal that nothing here is being sent without an explicit action. The + * panel surface mirrors the user-message chat surface so it stays calm in + * the scroll; the strong amber destination signal lives on the composer + * Send button (which only goes amber while scratchlist mode is routing). */ export function ScratchlistPanel({ sessionId, @@ -172,6 +460,15 @@ export function ScratchlistPanel({ const [draft, setDraft] = useState('') const [busyEntryId, setBusyEntryId] = useState(null) const inputRef = useRef(null) + const { copiedEntryId, signalCopied } = useCopiedFeedback() + const handleCopy = useCallback(async (entry: ScratchlistEntry) => { + try { + await safeCopyToClipboard(entry.text) + signalCopied(entry.id) + } catch { + // see ScratchlistInventory.handleCopy for rationale + } + }, [signalCopied]) // Re-hydrate when the session id changes (route navigation between sessions). useEffect(() => { @@ -278,7 +575,7 @@ export function ScratchlistPanel({ return (
@@ -408,6 +705,25 @@ export function ScratchlistPanel({ > + +
+ {/* Affirmative-action dismiss. No auto-timeout: reading speed + varies, and a popover that disappears on its own undercuts + the "user is in control" model. */} +
+ +
+
+ ) + + return createPortal(node, document.body) +} diff --git a/web/src/components/SessionChat.exit-mode.test.tsx b/web/src/components/SessionChat.exit-mode.test.tsx new file mode 100644 index 00000000..f0e0f145 --- /dev/null +++ b/web/src/components/SessionChat.exit-mode.test.tsx @@ -0,0 +1,146 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { I18nProvider } from '@/lib/i18n-context' +import type { ScratchlistEntry } from '@/lib/scratchlist' + +/** + * Regression test for upstream review on PR #798 (HAPI Bot follow-up + * after b256fe5): + * + * > Found one major issue: promoting a scratchlist item to the + * > composer keeps scratchlist mode enabled, so the next send re-adds + * > it to the scratchlist instead of sending to chat. + * + * The fix is for ScratchlistDrawerHost to call `onExitScratchlistMode` + * whenever it promotes an entry to the composer (since promoting means + * "I want to send this for real now"). This test mocks the assistant-ui + * runtime hook and asserts both the setText call AND the exit-mode call + * fire when the operator clicks promote-to-composer. + * + * Promote-to-queue does NOT exit the mode - the queue path bypasses the + * scratchlist-mode wrapper entirely, and the operator may still want to + * capture related notes. + */ + +const setText = vi.fn() +vi.mock('@assistant-ui/react', () => ({ + useAssistantApi: () => ({ + composer: () => ({ setText }), + }), +})) + +import { ScratchlistDrawerHost } from './SessionChat' + +function makeEntry(overrides: Partial & { id: string }): ScratchlistEntry { + return { text: 'note', createdAt: 1000, ...overrides } +} + +afterEach(() => { + cleanup() + setText.mockReset() +}) + +describe('ScratchlistDrawerHost.onPromoteToComposer', () => { + it('exits scratchlist mode AND sets composer text when an entry is promoted to composer', () => { + const onExitScratchlistMode = vi.fn() + const onSend = vi.fn(async () => true) + const onMove = vi.fn() + const onDelete = vi.fn() + + render( + + + , + ) + + // The drawer renders a "promote to composer" button per entry. + // Match by aria-label so we do not depend on icon/glyph copy. + const promoteButtons = screen.getAllByRole('button', { name: /composer|edit/i }) + expect(promoteButtons.length).toBeGreaterThan(0) + fireEvent.click(promoteButtons[0]!) + + expect(setText).toHaveBeenCalledWith('queued thought') + expect(onExitScratchlistMode).toHaveBeenCalledTimes(1) + // Promote-to-composer must NOT call onSend (that's promote-to-queue). + expect(onSend).not.toHaveBeenCalled() + }) + + it('does NOT exit scratchlist mode when an entry is promoted to queue', async () => { + const onExitScratchlistMode = vi.fn() + const onSend = vi.fn(async () => true) + const onMove = vi.fn() + const onDelete = vi.fn() + + render( + + + , + ) + + const queueButtons = screen.getAllByRole('button', { name: /queue|send/i }) + expect(queueButtons.length).toBeGreaterThan(0) + fireEvent.click(queueButtons[0]!) + + // Allow the async onSend to settle + await Promise.resolve() + await Promise.resolve() + + expect(onSend).toHaveBeenCalledWith('send-to-queue text') + expect(onExitScratchlistMode).not.toHaveBeenCalled() + expect(setText).not.toHaveBeenCalled() + }) +}) + +describe('ScratchlistDrawer copy-to-clipboard action', () => { + it('writes the entry text to the clipboard and flips the button label to "Copied!" briefly', async () => { + // Mock navigator.clipboard so safeCopyToClipboard's primary path + // resolves successfully (it tries this before the execCommand fallback). + const writeText = vi.fn().mockResolvedValue(undefined) + Object.defineProperty(navigator, 'clipboard', { + value: { writeText }, + configurable: true, + }) + + const onExitScratchlistMode = vi.fn() + const onSend = vi.fn(async () => true) + const onMove = vi.fn() + const onDelete = vi.fn() + + render( + + + , + ) + + fireEvent.click(screen.getByRole('button', { name: 'Copy to clipboard' })) + + await waitFor(() => expect(writeText).toHaveBeenCalledWith('copy this')) + await waitFor(() => + expect(screen.getByRole('button', { name: 'Copied!' })).toBeTruthy(), + ) + + // Copy must NOT mutate the list — entry stays, no other handlers fire. + expect(onDelete).not.toHaveBeenCalled() + expect(onSend).not.toHaveBeenCalled() + expect(setText).not.toHaveBeenCalled() + expect(onExitScratchlistMode).not.toHaveBeenCalled() + }) +}) diff --git a/web/src/components/SessionChat.test.ts b/web/src/components/SessionChat.test.ts index d3618bd8..2e92dab6 100644 --- a/web/src/components/SessionChat.test.ts +++ b/web/src/components/SessionChat.test.ts @@ -1,7 +1,13 @@ import { describe, expect, it } from 'vitest' -import { buildGoalStateMessages, shouldAutoClearPendingSchedule } from './SessionChat' +import { + buildGoalStateMessages, + isScratchlistHotkeyBlockedTarget, + isScratchlistToggleHotkey, + shouldAutoClearPendingSchedule, + shouldRouteToScratchlist, +} from './SessionChat' import type { PendingSchedule } from '@/components/AssistantChat/ScheduleTimePicker' -import type { DecryptedMessage } from '@/types/api' +import type { AttachmentMetadata, DecryptedMessage } from '@/types/api' function userMessage(props: { id: string @@ -67,6 +73,173 @@ describe('shouldAutoClearPendingSchedule', () => { }) }) +/** + * Unit tests for shouldRouteToScratchlist. + * + * Regression cover for upstream review on PR #798 (github-actions[bot] + * [Major]): scratchlist-mode submissions used to silently drop + * attachments and scheduledAt because the wrapper short-circuited to + * scratchlist.add(text) regardless of payload. The fix is to fall + * through to the regular chat send whenever the submission can't be + * represented as a pure-text scratchlist entry. + */ +describe('shouldRouteToScratchlist', () => { + function attachment(): AttachmentMetadata { + return { + id: 'attach-1', + filename: 'attach-1.png', + mimeType: 'image/png', + size: 1024, + path: '/tmp/attach-1.png', + } + } + + it('returns false when scratchlist mode is off, regardless of payload', () => { + expect(shouldRouteToScratchlist(false, undefined, null)).toBe(false) + expect(shouldRouteToScratchlist(false, [attachment()], null)).toBe(false) + expect(shouldRouteToScratchlist(false, undefined, Date.now() + 60_000)).toBe(false) + }) + + it('returns true when scratchlist mode is on and the payload is pure text', () => { + expect(shouldRouteToScratchlist(true, undefined, null)).toBe(true) + expect(shouldRouteToScratchlist(true, undefined, undefined)).toBe(true) + expect(shouldRouteToScratchlist(true, [], null)).toBe(true) + }) + + it('returns false when scratchlist mode is on but attachments are present', () => { + expect(shouldRouteToScratchlist(true, [attachment()], null)).toBe(false) + expect(shouldRouteToScratchlist(true, [attachment(), attachment()], null)).toBe(false) + }) + + it('returns false when scratchlist mode is on but a scheduled-send is set', () => { + expect(shouldRouteToScratchlist(true, undefined, Date.now() + 60_000)).toBe(false) + expect(shouldRouteToScratchlist(true, [], 0)).toBe(false) + }) + + it('returns false when both attachments and scheduledAt are set', () => { + expect(shouldRouteToScratchlist(true, [attachment()], Date.now() + 60_000)).toBe(false) + }) + + /** + * Bot follow-up on PR #798: handleSend gates pendingSchedule cleanup on + * routedToScratchlist, not scratchlistMode. So a scheduled chat send made + * while the scratchlist toggle is on (which falls through to chat per + * the previous tests) MUST also trigger schedule clear + scroll bump. + * This test pins the decision matrix that handleSend depends on. + */ + it('cleanup gate: scheduled chat send while scratchlist toggle is on still clears schedule', () => { + const scheduledAt = Date.now() + 60_000 + // Scenario: mode on, no attachments, scheduled. shouldRouteToScratchlist + // must return false so handleSend's `if (!routedToScratchlist)` runs + // setPendingSchedule(null). + const routed = shouldRouteToScratchlist(true, undefined, scheduledAt) + expect(routed).toBe(false) + const shouldClearAfterAccepted = !routed + expect(shouldClearAfterAccepted).toBe(true) + }) + + it('cleanup gate: pure-text scratchlist add does NOT clear schedule', () => { + const routed = shouldRouteToScratchlist(true, undefined, null) + expect(routed).toBe(true) + const shouldClearAfterAccepted = !routed + expect(shouldClearAfterAccepted).toBe(false) + }) +}) + +describe('isScratchlistToggleHotkey', () => { + function k(over: Partial<{ + metaKey: boolean; ctrlKey: boolean; shiftKey: boolean; altKey: boolean; key: string + }>): { metaKey: boolean; ctrlKey: boolean; shiftKey: boolean; altKey: boolean; key: string } { + return { metaKey: false, ctrlKey: false, shiftKey: false, altKey: false, key: '', ...over } + } + + it('matches Ctrl+Shift+S (Linux/Windows)', () => { + expect(isScratchlistToggleHotkey(k({ ctrlKey: true, shiftKey: true, key: 'S' }))).toBe(true) + expect(isScratchlistToggleHotkey(k({ ctrlKey: true, shiftKey: true, key: 's' }))).toBe(true) + }) + + it('matches Cmd+Shift+S (macOS)', () => { + expect(isScratchlistToggleHotkey(k({ metaKey: true, shiftKey: true, key: 'S' }))).toBe(true) + }) + + it('rejects Cmd/Ctrl + S without shift (browser Save)', () => { + // Browsers reserve Ctrl-S / Cmd-S for "Save Page". The toggle MUST + // require shift so the user's save-page muscle memory keeps working. + expect(isScratchlistToggleHotkey(k({ ctrlKey: true, key: 's' }))).toBe(false) + expect(isScratchlistToggleHotkey(k({ metaKey: true, key: 's' }))).toBe(false) + }) + + it('rejects bare S / Shift+S (literal typing)', () => { + expect(isScratchlistToggleHotkey(k({ key: 's' }))).toBe(false) + expect(isScratchlistToggleHotkey(k({ shiftKey: true, key: 'S' }))).toBe(false) + }) + + it('rejects when Alt is also held (avoid clashes with OS shortcuts)', () => { + expect(isScratchlistToggleHotkey(k({ + ctrlKey: true, shiftKey: true, altKey: true, key: 'S', + }))).toBe(false) + }) + + it('rejects unrelated keys', () => { + expect(isScratchlistToggleHotkey(k({ ctrlKey: true, shiftKey: true, key: 'A' }))).toBe(false) + expect(isScratchlistToggleHotkey(k({ ctrlKey: true, shiftKey: true, key: 'Tab' }))).toBe(false) + }) +}) + +describe('isScratchlistHotkeyBlockedTarget', () => { + // Note: tests run under jsdom, so HTMLElement / HTMLInputElement etc. + // are real constructors that we can construct via document.createElement. + + it('blocks hotkey when focus is in a single-line input', () => { + const input = document.createElement('input') + expect(isScratchlistHotkeyBlockedTarget(input)).toBe(true) + }) + + it('blocks hotkey when focus is in a select element', () => { + const select = document.createElement('select') + expect(isScratchlistHotkeyBlockedTarget(select)).toBe(true) + }) + + it('blocks hotkey when focus is on a contentEditable host', () => { + const div = document.createElement('div') + div.setAttribute('contenteditable', 'true') + expect(isScratchlistHotkeyBlockedTarget(div)).toBe(true) + }) + + it('blocks hotkey when focus is anywhere inside a [role=dialog]', () => { + const dialog = document.createElement('div') + dialog.setAttribute('role', 'dialog') + const inner = document.createElement('button') + dialog.appendChild(inner) + document.body.appendChild(dialog) + expect(isScratchlistHotkeyBlockedTarget(inner)).toBe(true) + document.body.removeChild(dialog) + }) + + it('does NOT block hotkey when focus is on the composer textarea', () => { + // The composer textarea is the EXPECTED focus target when the + // operator presses the shortcut. Blocking it would defeat the + // shortcut entirely. + const textarea = document.createElement('textarea') + expect(isScratchlistHotkeyBlockedTarget(textarea)).toBe(false) + }) + + it('does NOT block hotkey when focus is on a regular button', () => { + const button = document.createElement('button') + expect(isScratchlistHotkeyBlockedTarget(button)).toBe(false) + }) + + it('does NOT block hotkey when target is null (unfocused)', () => { + expect(isScratchlistHotkeyBlockedTarget(null)).toBe(false) + }) + + it('does NOT block hotkey when target is non-Element (e.g. window)', () => { + // Some keyboard events come with a non-Element target (e.g. window + // before focus settles). Should fall through. + expect(isScratchlistHotkeyBlockedTarget(window as unknown as EventTarget)).toBe(false) + }) +}) + describe('buildGoalStateMessages', () => { it('keeps immediate queued user messages so completed goal status can clear before timeline render', () => { const now = 1_700_000_000_000 diff --git a/web/src/components/SessionChat.tsx b/web/src/components/SessionChat.tsx index 9694c5cb..3390ee7b 100644 --- a/web/src/components/SessionChat.tsx +++ b/web/src/components/SessionChat.tsx @@ -19,12 +19,13 @@ import { buildConversationOutline } from '@/chat/outline' import { buildVisibleChatBlocks, isToolGroupBlock, type ToolGroupBlock } from '@/chat/toolGroups' import { isQueuedForInvocation, mergeMessages } from '@/lib/messages' import { inactiveSessionCanResume } from '@/lib/sessionResume' -import { HappyComposer } from '@/components/AssistantChat/HappyComposer' +import { HappyComposer, type ComposerSendError } from '@/components/AssistantChat/HappyComposer' import type { PendingSchedule } from '@/components/AssistantChat/ScheduleTimePicker' import { resolvePendingSchedule } from '@/components/AssistantChat/ScheduleTimePicker' import { HappyThread } from '@/components/AssistantChat/HappyThread' import { QueuedMessagesBar } from '@/components/AssistantChat/QueuedMessagesBar' -import { ScratchlistPanel } from '@/components/AssistantChat/ScratchlistPanel' +import { ScratchlistDrawer } from '@/components/AssistantChat/ScratchlistPanel' +import { useScratchlist } from '@/lib/use-scratchlist' import { useHappyRuntime } from '@/lib/assistant-runtime' import { createAttachmentAdapter } from '@/lib/attachmentAdapter' import { useTranslation } from '@/lib/use-translation' @@ -67,33 +68,135 @@ export function shouldAutoClearPendingSchedule(pending: PendingSchedule | null): return pending !== null && pending.type === 'absolute' } +/** + * True if the keystroke matches the scratchlist-mode toggle shortcut + * (Ctrl/Cmd + Shift + S, no Alt). Pure / exported for unit tests. + * + * Convention: matches the v1 always-visible panel's shortcut so muscle + * memory carries over. Sibling globals follow the same modifier shape + * (Ctrl/Cmd-m cycles agent model in HappyComposer). + */ +export function isScratchlistToggleHotkey(e: { + metaKey: boolean + ctrlKey: boolean + shiftKey: boolean + altKey: boolean + key: string +}): boolean { + if (!(e.metaKey || e.ctrlKey)) return false + if (!e.shiftKey) return false + if (e.altKey) return false + return e.key === 'S' || e.key === 's' +} + +/** + * True when the global scratchlist hotkey should be SKIPPED for the + * given event target. Window-level shortcuts that fire regardless of + * focus can quietly toggle modes "behind" modal dialogs (rename, + * schedule picker, FUE callout) and that's the kind of UX bug the bot + * caught on PR #798. + * + * Block targets: + * - any descendant of an open dialog (Radix UI's DialogContent renders + * role="dialog", as do FueCallout / ScheduleTimePicker / ImagePreview) + * - HTMLInputElement (single-line inputs) + * - HTMLSelectElement + * - any contentEditable host + * + * NOT blocked: + * - HTMLTextAreaElement (the composer textarea is the normal focus + * target when the operator presses the hotkey - blocking it would + * defeat the shortcut) + * - the document body / unfocused targets + * + * Pure / exported for unit tests. + */ +export function isScratchlistHotkeyBlockedTarget(target: EventTarget | null): boolean { + if (!(target instanceof HTMLElement)) return false + if (target.closest('[role="dialog"]') !== null) return true + if (target instanceof HTMLInputElement) return true + if (target instanceof HTMLSelectElement) return true + // isContentEditable is the authoritative check in real browsers but + // jsdom doesn't implement it; the attribute fallback covers both. + if (target.isContentEditable === true) return true + return target.getAttribute('contenteditable') === 'true' +} + +/** + * Decide whether a submit should be routed to the per-session scratchlist + * or to the regular chat send. Scratchlist entries are pure text - they + * don't carry attachments or schedules - so any submit that includes + * either of those MUST fall through to the normal chat path even if the + * scratchlist toggle is on. Otherwise the wrapper would silently drop + * attachments / scheduled-send metadata while telling the composer the + * submission succeeded (which then clears the composer state, losing + * the user's data). + * + * Per upstream review on PR #798 (github-actions[bot] [Major]). + * + * Pure / exported so it can be unit tested without mounting SessionChat. + */ +export function shouldRouteToScratchlist( + scratchlistMode: boolean, + attachments: AttachmentMetadata[] | undefined, + scheduledAt: number | null | undefined, +): boolean { + if (!scratchlistMode) return false + if (attachments && attachments.length > 0) return false + if (scheduledAt != null) return false + return true +} + function isUninvokedScheduledMessage(message: DecryptedMessage): boolean { return message.invokedAt == null && message.scheduledAt != null } /** - * Mounts the per-session scratchlist (issue #11) inside the AssistantUI - * runtime so promote-to-composer can call `composer().setText(...)`. - * Promote-to-queue routes to the same `onSend` path as a normal composer - * send, so a promoted entry shows up immediately in `QueuedMessagesBar`. + * Mounts the per-session scratchlist DRAWER (composer-controlled). + * + * The drawer renders only when the operator toggles into "scratchlist + * mode" via the notepad icon in the composer toolbar. While in that mode: + * - drawer (this component) is visible above the composer + * - composer's send button repaints amber (handled in ComposerButtons) + * - SessionChat's wrapped onSend routes adds into the scratchlist + * + * Entries state is owned by SessionChat's useScratchlist() so the + * composer-toolbar counter and the drawer share one source of truth. */ -function ScratchlistHost({ - sessionId, - onSend, -}: { - sessionId: string +export function ScratchlistDrawerHost(props: { + entries: ReturnType['entries'] + onMove: ReturnType['move'] + onDelete: ReturnType['remove'] onSend: (text: string, attachments?: AttachmentMetadata[], scheduledAt?: number | null) => Promise + /** + * Called when the operator promotes an entry to the composer. + * + * Promoting means "I want to send this for real now" - so the host + * MUST exit scratchlist mode, otherwise the next composer submit + * routes back to scratchlist (per the v1.1 modal-mode contract) and + * the user re-adds the same text instead of sending it to chat. + * Per upstream review on PR #798 (HAPI Bot, v6 follow-up). + */ + onExitScratchlistMode: () => void }) { const assistantApi = useAssistantApi() const handlePromoteToComposer = useCallback((text: string) => { assistantApi.composer().setText(text) - }, [assistantApi]) + props.onExitScratchlistMode() + }, [assistantApi, props.onExitScratchlistMode]) const handlePromoteToQueue = useCallback(async (text: string) => { - return await onSend(text) - }, [onSend]) + // Promote-to-queue bypasses the scratchlist-mode wrapper by + // calling props.onSend directly (the chat send), so the queue + // entry lands in the conversation regardless of scratchlist + // mode. Mode itself stays on - the operator may still be + // capturing related notes. + return await props.onSend(text) + }, [props.onSend]) return ( - @@ -141,7 +244,7 @@ function hasAbortableAgentRun(blocks: readonly ChatBlock[]): boolean { return false } -export function SessionChat(props: { +type SessionChatProps = { api: ApiClient session: Session messages: DecryptedMessage[] @@ -166,7 +269,35 @@ export function SessionChat(props: { onRetryMessage?: (localId: string) => void autocompleteSuggestions?: (query: string) => Promise availableSlashCommands?: readonly SlashCommand[] -}) { + // The latest send the hub rejected (4xx/5xx/network). When set, the + // composer is asked to restore the typed text and surface an inline + // error -- see HappyComposer. Cleared by `onClearSendError` once the + // user dismisses or starts editing. + sendError?: ComposerSendError | null + onClearSendError?: () => void +} + +/** + * Public entry point. Thin wrapper around `SessionChatInner` keyed by + * the session id so that ALL inner state - including the scratchlist + * (entries + mode) and the assistant-ui runtime - resets atomically + * when the operator navigates between sessions on the same route + * (e.g. /sessions/A -> /sessions/B). + * + * Without the key, React reuses the same component instance, and + * effects run AFTER the first paint of the new session. That window + * briefly renders the new session with the previous session's + * scratchlist entries / drawer-open state, which is the bot finding + * on PR #798 (PRRT_kwDOQuQOSc6HHOsa). The keyed wrapper is the + * canonical React pattern for "fully reset state on prop change"; it + * supersedes the effect-based mode-reset that previously lived in + * SessionChatInner. + */ +export function SessionChat(props: SessionChatProps) { + return +} + +function SessionChatInner(props: SessionChatProps) { const { haptic } = usePlatform() const { t } = useTranslation() const navigate = useNavigate() @@ -180,6 +311,79 @@ export function SessionChat(props: { const [outlineOpen, setOutlineOpen] = useState(false) const [cursorSelectedBase, setCursorSelectedBase] = useState('auto') const lastSyncedCursorModelRef = useRef(undefined) + const scratchlist = useScratchlist(props.session.id) + const [scratchlistMode, setScratchlistMode] = useState(false) + // Mode resets across sessions implicitly: SessionChat is keyed by + // session.id at the public-export boundary, so a session switch + // remounts SessionChatInner from scratch and `scratchlistMode` + // initializes to false again. (Previous effect-based reset was + // racy on first paint - see public-export comment for context.) + const handleScratchlistToggle = useCallback(() => { + setScratchlistMode((m) => !m) + }, []) + /** + * Global keyboard shortcut: Ctrl/Cmd + Shift + S toggles scratchlist + * mode (open/close drawer + flip composer routing). + * + * Convention matches the v1 always-visible panel's shortcut so muscle + * memory carries over. Other composer-adjacent globals in the app use + * the same modifier shape: Ctrl/Cmd-m cycles agent model in + * HappyComposer. Ctrl/Cmd-Shift-S is unreserved by Chrome / Firefox / + * Safari at the app level (browser Save As is Ctrl-S / Cmd-S, no + * Shift), so requiring Shift keeps the user's save-page muscle memory + * working. Bound at SessionChat scope (not the drawer) because the + * drawer is unmounted while mode is off — a drawer-scoped listener + * couldn't reopen it. + * + * Skipped when focus is inside an open dialog or single-line input + * (see isScratchlistHotkeyBlockedTarget). Otherwise fires for any + * focus target - composer textarea is the expected case so it's + * deliberately allowed. Window-level shortcut without target + * filtering would silently toggle mode "behind" modal dialogs + * (rename, schedule picker, FUE callout); the bot caught this on + * PR #798. + */ + useEffect(() => { + const onKeyDown = (e: globalThis.KeyboardEvent) => { + if (!isScratchlistToggleHotkey(e)) return + if (isScratchlistHotkeyBlockedTarget(e.target)) return + e.preventDefault() + setScratchlistMode((m) => !m) + } + window.addEventListener('keydown', onKeyDown) + return () => window.removeEventListener('keydown', onKeyDown) + }, []) + /** + * onSend wrapper: when scratchlist mode is on AND the submission is + * pure text (no attachments, no scheduledAt), the operator's submit + * is treated as "add to scratchlist" instead of "send to chat". + * + * If the submission carries attachments or a scheduledAt value, + * scratchlist can't represent it (entries are text-only), so we + * fall through to the normal chat send. Silently dropping + * attachments / schedule while reporting success to the composer + * caused PR #798 review's [Major] data-loss finding. + * + * The composer (HappyComposer) uses the boolean return value to + * decide whether to clear text/attachments/schedule, so we resolve + * true on a successful add - the operator's text gets cleared and + * they can keep adding entries while sticky-mode is on. If add() + * returns false (empty after trim, at-cap), we resolve false so + * the composer keeps its text and the operator can fix it. + */ + const onSendForComposer = useCallback( + async ( + text: string, + attachments?: AttachmentMetadata[], + scheduledAt?: number | null, + ): Promise => { + if (shouldRouteToScratchlist(scratchlistMode, attachments, scheduledAt)) { + return scratchlist.add(text) + } + return props.onSend(text, attachments, scheduledAt) + }, + [props.onSend, scratchlist, scratchlistMode], + ) const agentFlavor = props.session.metadata?.flavor ?? null const controlledByUser = props.session.agentState?.controlledByUser === true const codexCollaborationModeSupported = agentFlavor === 'codex' && !controlledByUser @@ -708,15 +912,32 @@ export function SessionChat(props: { }, [pendingSchedule]) const handleSend = useCallback(async (text: string, attachments?: AttachmentMetadata[], scheduledAt?: number | null) => { - const accepted = await props.onSend(text, attachments, scheduledAt) + // Route through the scratchlist-aware wrapper. When scratchlistMode + // is on AND the payload is pure text, this turns into + // addScratchlistEntry; otherwise it goes to props.onSend (the chat + // send path). The wrapper resolves true on success either way so + // the composer-clear is shared, but the schedule-clear / scroll + // dance below must gate on the actual route taken (not just + // scratchlistMode), or a scheduled chat send made while the + // scratchlist toggle is on will leave pendingSchedule sticky and + // the next normal send would reuse the same schedule. (Per + // upstream review on PR #798: [Major] "Clear accepted scheduled + // chat sends after scratchlist fallback".) + const routedToScratchlist = shouldRouteToScratchlist(scratchlistMode, attachments, scheduledAt) + const accepted = await onSendForComposer(text, attachments, scheduledAt) if (!accepted) return - // Clear pendingSchedule only after the mutation is actually accepted — - // covers both pre-mutation guards AND async inactive-session resume - // failure. SessionChat is the single owner of schedule clear (HappyComposer - // no longer clears on its own send path). - setPendingSchedule(null) - setForceScrollToken((token) => token + 1) - }, [props.onSend]) + if (!routedToScratchlist) { + // Clear pendingSchedule only after the mutation is actually + // accepted - covers both pre-mutation guards AND async + // inactive-session resume failure. SessionChat is the single + // owner of schedule clear (HappyComposer no longer clears on + // its own send path). Schedule clear / forced scroll only + // matter for chat sends; scratchlist adds don't have a + // schedule and shouldn't move the chat viewport. + setPendingSchedule(null) + setForceScrollToken((token) => token + 1) + } + }, [onSendForComposer, scratchlistMode]) const attachmentAdapter = useMemo(() => { if (!props.session.active) { @@ -807,23 +1028,21 @@ export function SessionChat(props: {
{/* - * Key by session id so React unmounts/remounts when - * the operator switches sessions without remounting - * SessionChat (e.g. same-route navigation A -> B). - * Without this, ScratchlistPanel's useState - * initializer reads sessionId once at mount; the - * useEffect rehydrate then races against the persist - * effect, briefly rendering A's entries under B and - * writing them into B's localStorage before - * correcting. Keying makes the first render for B - * read B's storage directly. Cleaner than chasing - * the race inside the panel. + * Scratchlist drawer - composer-controlled. Only + * mounted when the operator clicks the notepad icon + * in the composer toolbar. State lives in the + * useScratchlist hook above (so the toolbar counter + * and the drawer share one source of truth). */} - + {scratchlistMode ? ( + setScratchlistMode(false)} + /> + ) : null}
diff --git a/web/src/hooks/mutations/useSendMessage.test.tsx b/web/src/hooks/mutations/useSendMessage.test.tsx index 20d57440..9187bce4 100644 --- a/web/src/hooks/mutations/useSendMessage.test.tsx +++ b/web/src/hooks/mutations/useSendMessage.test.tsx @@ -9,6 +9,7 @@ vi.mock('@/lib/message-window-store', () => ({ appendOptimisticMessage: vi.fn(), getMessageWindowState: vi.fn(() => ({ messages: [], pending: [] })), updateMessageStatus: vi.fn(), + removeOptimisticMessage: vi.fn(), })) vi.mock('@/hooks/usePlatform', () => ({ @@ -101,6 +102,337 @@ describe('useSendMessage', () => { expect(onSuccess).not.toHaveBeenCalled() }) + // assistant-ui clears the composer eagerly when send is invoked, so to + // retain the typed text on failure we hand the original input back + // through the `onError` callback. The three branches below cover the + // acceptance criteria: 5xx/network, 4xx, and 2xx. + describe('composer text retention on send failure', () => { + it('5xx/network: onError fires with the original text so the composer can restore it', async () => { + const onError = vi.fn() + const onSuccess = vi.fn() + const api = createMockApi(async () => { + // request() throws plain Error for 5xx with this shape. + throw new Error('HTTP 503 Service Unavailable: hub down') + }) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError, onSuccess }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('keep this text on 503') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + const info = onError.mock.calls[0][0] as { text: string; error: unknown } + expect(info.text).toBe('keep this text on 503') + expect(info.error).toBeInstanceOf(Error) + expect((info.error as Error).message).toContain('503') + expect(onSuccess).not.toHaveBeenCalled() + }) + + it('network: onError fires with the original text on a fetch-level rejection', async () => { + const onError = vi.fn() + // Simulates a TypeError surfaced by fetch() when the hub socket + // dies mid-request (e.g. daily-rebuild restart blip). + const api = createMockApi(async () => { + throw new TypeError('Failed to fetch') + }) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('keep this on a dropped fetch') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + const info = onError.mock.calls[0][0] as { text: string; error: unknown } + expect(info.text).toBe('keep this on a dropped fetch') + expect(info.error).toBeInstanceOf(TypeError) + }) + + it('4xx: onError fires with the original text so the inline affordance can render', async () => { + const onError = vi.fn() + const api = createMockApi(async () => { + // request() throws plain Error for 4xx (e.g. 400/403). + throw new Error('HTTP 400 Bad Request: invalid payload') + }) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('keep this text on 400') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + const info = onError.mock.calls[0][0] as { text: string; error: unknown } + expect(info.text).toBe('keep this text on 400') + expect((info.error as Error).message).toContain('400') + }) + + it('2xx: onError is not called and onSuccess fires (composer clears as today)', async () => { + const onError = vi.fn() + const onSuccess = vi.fn() + const api = createMockApi() + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError, onSuccess }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('clean send') + }) + + await waitFor(() => { + expect(onSuccess).toHaveBeenCalledWith('session-A') + }) + expect(onError).not.toHaveBeenCalled() + }) + + it('non-Error throws still surface text; the consumer falls back to its default message', async () => { + const onError = vi.fn() + const api = createMockApi(async () => { + // Defensive case — some providers throw bare strings/objects. + // We must not swallow these or the composer would silently + // eat the user's text again. + throw 'opaque failure' + }) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('keep this on opaque failure') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + const info = onError.mock.calls[0][0] as { text: string; error: unknown; scheduledAt: number | null } + expect(info.text).toBe('keep this on opaque failure') + expect(info.error).toBe('opaque failure') + expect(info.scheduledAt).toBeNull() + }) + + it('carries scheduledAt through onError so the composer can restore a failed scheduled send as scheduled', async () => { + // Without this, SessionChat clears pendingSchedule on accept and the + // subsequent failure's restore would silently downgrade a scheduled + // send to immediate -- the operator hits send again and the message + // dispatches now instead of at the chosen time. + const onError = vi.fn() + const api = createMockApi(async () => { + throw new Error('HTTP 503 Service Unavailable') + }) + const scheduledAt = Date.now() + 5 * 60 * 1000 + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('see you in 5', undefined, scheduledAt) + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + const info = onError.mock.calls[0][0] as { text: string; scheduledAt: number | null } + expect(info.text).toBe('see you in 5') + expect(info.scheduledAt).toBe(scheduledAt) + }) + + it('immediate send: scheduledAt is null in onError', async () => { + const onError = vi.fn() + const api = createMockApi(async () => { + throw new Error('boom') + }) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('immediate') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + const info = onError.mock.calls[0][0] as { scheduledAt: number | null } + expect(info.scheduledAt).toBeNull() + }) + + it('removes the optimistic row on failure so the composer-restore path is the single retry surface', async () => { + // Without this, the thread keeps a stale `failed` bubble next to + // the restored composer text, and the operator can stack a + // duplicate by retrying from either surface. + const onError = vi.fn() + const api = createMockApi(async () => { + throw new Error('HTTP 503') + }) + + const { removeOptimisticMessage, updateMessageStatus } = await import('@/lib/message-window-store') + const removeMock = vi.mocked(removeOptimisticMessage) + const updateMock = vi.mocked(updateMessageStatus) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('hello') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + // The optimistic row is removed instead of being kept as failed. + expect(removeMock).toHaveBeenCalledWith('session-A', 'local-id-1') + // Defensive: nothing else should have transitioned the row to + // 'failed' on this path -- we removed it outright. + expect(updateMock.mock.calls.some((call) => call[2] === 'failed')).toBe(false) + }) + + it('carries sessionId through onError so a resumed-session POST that fails restores into the right composer', async () => { + // Inactive-session resume: useSendMessage resolves a target id, + // kicks off async navigation, and then the POST can fail. The + // route component keys sendError state by sessionId so the + // restore lands on the resumed session, not the old one whose + // composer the operator has already navigated away from. + const onError = vi.fn() + const api = createMockApi(async () => { + throw new Error('HTTP 500') + }) + + const { result } = renderHook( + () => useSendMessage(api, 'session-original', { + onError, + resolveSessionId: async () => 'session-resolved', + onSessionResolved: vi.fn(), + }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('hi from resumed') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + const info = onError.mock.calls[0][0] as { sessionId: string; text: string } + expect(info.sessionId).toBe('session-resolved') + expect(info.text).toBe('hi from resumed') + }) + + it('attachment send: keeps the failed row in the thread and skips composer-restore', async () => { + // The composer-restore path can't reinstate uploaded attachment + // metadata, so for sends with attachments we fall back to the + // legacy failed-bubble UX (operator retries via the in-thread + // retry button, which re-fires the send WITH attachments). + const onError = vi.fn() + const api = createMockApi(async () => { + throw new Error('HTTP 503') + }) + + const { removeOptimisticMessage, updateMessageStatus } = await import('@/lib/message-window-store') + const removeMock = vi.mocked(removeOptimisticMessage) + const updateMock = vi.mocked(updateMessageStatus) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('see this image', [ + { id: 'att-1', filename: 'x.png', mimeType: 'image/png', size: 1, path: '/x.png' } + ]) + }) + + await waitFor(() => { + expect(updateMock).toHaveBeenCalledWith('session-A', 'local-id-1', 'failed') + }) + // No composer-restore: onError is NOT fired and the optimistic + // row is NOT removed -- both would destroy the attachment UX. + expect(onError).not.toHaveBeenCalled() + expect(removeMock).not.toHaveBeenCalled() + }) + + it('retryMessage: passes attachments through so failed-bubble retry of an attachment send keeps its files', async () => { + // Without this, the failed-bubble retry path silently drops the + // attachments and re-fires as a text-only send. + const sendMock = vi.fn<(...args: unknown[]) => Promise>(async () => {}) + const api = { sendMessage: sendMock } as unknown as ApiClient + + const { getMessageWindowState } = await import('@/lib/message-window-store') + const stateMock = vi.mocked(getMessageWindowState) + const failedAttachmentMessage = { + id: 'local-att-1', + seq: null, + localId: 'local-att-1', + content: { + role: 'user' as const, + content: { + type: 'text' as const, + text: 'photo + text', + attachments: [ + { id: 'att-1', filename: 'x.png', mimeType: 'image/png', size: 1, path: '/x.png' } + ] + } + }, + createdAt: 1000, + invokedAt: null, + scheduledAt: null, + status: 'failed' as const, + originalText: 'photo + text', + } + stateMock.mockReturnValue({ + messages: [failedAttachmentMessage], + pending: [] + } as unknown as ReturnType) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A'), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.retryMessage('local-att-1') + }) + + await waitFor(() => { + expect(sendMock).toHaveBeenCalled() + }) + const args = sendMock.mock.calls[0] + expect(args[0]).toBe('session-A') + expect(args[1]).toBe('photo + text') + expect(args[2]).toBe('local-att-1') + expect(args[3]).toEqual([ + { id: 'att-1', filename: 'x.png', mimeType: 'image/png', size: 1, path: '/x.png' } + ]) + }) + }) + it('does not call onSuccess when blocked', () => { const onSuccess = vi.fn() const onBlocked = vi.fn() diff --git a/web/src/hooks/mutations/useSendMessage.ts b/web/src/hooks/mutations/useSendMessage.ts index dc54c83f..2f6c77ff 100644 --- a/web/src/hooks/mutations/useSendMessage.ts +++ b/web/src/hooks/mutations/useSendMessage.ts @@ -6,6 +6,7 @@ import { makeClientSideId } from '@/lib/messages' import { appendOptimisticMessage, getMessageWindowState, + removeOptimisticMessage, updateMessageStatus, } from '@/lib/message-window-store' import { usePlatform } from '@/hooks/usePlatform' @@ -21,11 +22,49 @@ type SendMessageInput = { type BlockedReason = 'no-api' | 'no-session' | 'pending' +/** + * Information about a send that the underlying mutation rejected. + * + * Surfaced via the `onError` option so the consumer can keep the typed + * text in the composer (composer must NOT clear on 4xx/5xx or network + * failure) and render an inline affordance. + * + * - `sessionId` is the session the failed send was actually targeting + * (post-`resolveSessionId`). Inactive-session resume can resolve a + * target id, kick off async navigation, and then have the POST fail + * before navigation completes; without this id the consumer would + * restore the text into the wrong composer (the old session) and the + * sessionId-change effect would clear it again. + * - `text` is the original input the user typed, captured before the + * mutation cleared the composer. + * - `error` is the raw thrown value (typically `Error`) so the consumer + * can inspect status / message. + * - `scheduledAt` is the absolute epoch-ms the send was bound for, or + * null for an immediate send. Carried through so a failed scheduled + * send can be restored as a scheduled send instead of silently + * downgrading to immediate -- `SessionChat.handleSend` clears the + * pendingSchedule the moment the mutation is accepted, so without + * this the schedule is gone by the time onError fires. + * + * Only fired for text-only sends. Sends with attachments fall back to + * the legacy failed-bubble UX (the optimistic row stays as `failed` and + * the user retries via the in-thread retry button); the composer-restore + * path can't reinstate uploaded attachment metadata, so doing the swap + * for attachment sends would silently drop the attachments. + */ +export type SendErrorInfo = { + sessionId: string + text: string + error: unknown + scheduledAt: number | null +} + type UseSendMessageOptions = { resolveSessionId?: (sessionId: string) => Promise onSessionResolved?: (sessionId: string) => void onBlocked?: (reason: BlockedReason) => void onSuccess?: (sessionId: string) => void + onError?: (info: SendErrorInfo) => void isSessionThinking?: boolean } @@ -69,6 +108,30 @@ function findMessageByLocalId( return null } +/** Pull attachments off a stored optimistic user message. The schema types + * `content` as `unknown`, so this is a defensive narrow: we accept only the + * exact shape `createOptimisticMessage` produces (`role: 'user'`, text-typed + * content, attachments array) and return undefined otherwise. Used by + * retryMessage so an attachment send retried from the failed-bubble button + * re-fires with its attachments instead of becoming a text-only send. */ +function getMessageAttachments(message: DecryptedMessage): AttachmentMetadata[] | undefined { + const content = message.content as unknown + if ( + typeof content !== 'object' || + content === null + ) { + return undefined + } + const outer = content as { role?: unknown; content?: unknown } + if (outer.role !== 'user') return undefined + const inner = outer.content as { type?: unknown; attachments?: unknown } | null + if (!inner || inner.type !== 'text') return undefined + if (!Array.isArray(inner.attachments) || inner.attachments.length === 0) { + return undefined + } + return inner.attachments as AttachmentMetadata[] +} + export function useSendMessage( api: ApiClient | null, sessionId: string | null, @@ -111,9 +174,34 @@ export function useSendMessage( haptic.notification('success') options?.onSuccess?.(input.sessionId) }, - onError: (_, input) => { - updateMessageStatus(input.sessionId, input.localId, 'failed') + onError: (error, input) => { + // Attachment sends keep the legacy failed-bubble UX: the + // composer-restore path can only re-seat text + scheduledAt, + // not the uploaded attachment metadata. Removing the row + // would destroy the attachment preview AND leave the operator + // with no retry surface for it. Keep the row as `failed` so + // the in-thread retry button can re-fire the send (with + // attachments) via retryMessage. + if (input.attachments && input.attachments.length > 0) { + updateMessageStatus(input.sessionId, input.localId, 'failed') + haptic.notification('error') + return + } + // Text-only sends use the composer-restore path: drop the + // optimistic row from the thread (otherwise the failed bubble + // would visually duplicate the same text the composer is + // about to restore, and the operator could stack a stale + // failed turn next to a fresh send) and hand the text + + // scheduledAt + sessionId back so the route can put both + // back into the composer keyed to the right session. + removeOptimisticMessage(input.sessionId, input.localId) haptic.notification('error') + options?.onError?.({ + sessionId: input.sessionId, + text: input.text, + error, + scheduledAt: input.scheduledAt ?? null + }) }, }) @@ -190,6 +278,7 @@ export function useSendMessage( text: message.originalText, localId, createdAt: message.createdAt, + attachments: getMessageAttachments(message), scheduledAt: message.scheduledAt ?? null, }) return true diff --git a/web/src/lib/locales/en.ts b/web/src/lib/locales/en.ts index 812b4e00..10e3abf3 100644 --- a/web/src/lib/locales/en.ts +++ b/web/src/lib/locales/en.ts @@ -231,6 +231,7 @@ export default { 'chat.settings': 'Settings', 'chat.terminal': 'Terminal', 'chat.switchRemote': 'Switch to remote mode', + 'chat.sendError.fallback': "Couldn't send your message. Edit and try again.", // Codex review 'codexReview.title': 'Codex review', @@ -404,6 +405,12 @@ export default { 'scratchlist.count.one': '1 item', 'scratchlist.count.other': '{n} items', 'scratchlist.emptyHint': 'Park notes, drafts, or ideas here. Nothing is sent until you promote it.', + 'scratchlist.drawerHint': 'Type below — Send adds the next message to the scratchlist instead of the chat. Click the note icon again to leave.', + 'scratchlist.toggleAriaLabel': 'Scratchlist drawer', + 'scratchlist.toggleTooltip': 'Scratchlist — park notes & drafts (Ctrl/Cmd+Shift+S)', + 'scratchlist.sendToScratchlist': 'Send to scratchlist', + 'scratchlist.fueTitle': 'New: Scratchlist', + 'scratchlist.fueBody': 'Park notes & drafts here without sending. The Send button glows amber while you stash; click the icon (or Ctrl/Cmd+Shift+S) again to leave.', 'scratchlist.addPlaceholder': 'Note, draft, or idea — Enter to add', 'scratchlist.addAriaLabel': 'Add scratchlist entry', 'scratchlist.add': 'Add', @@ -414,7 +421,12 @@ export default { 'scratchlist.action.moveDown': 'Move entry down', 'scratchlist.action.promoteToComposer': 'Copy into composer', 'scratchlist.action.promoteToQueue': 'Send to queue', + 'scratchlist.action.copy': 'Copy to clipboard', + 'scratchlist.action.copied': 'Copied!', 'scratchlist.action.delete': 'Delete entry', + 'fue.newFeatureDot': 'New feature available', + 'fue.gotIt': 'Got it', + 'fue.closeAriaLabel': 'Close explainer', 'composer.codexSlashUnsupported.title': 'Codex command unavailable', 'composer.codexSlashUnsupported.body': 'HAPI remote mode does not yet run built-in Codex slash commands like {command}. Use natural language instead, or run it in the local Codex TUI.', diff --git a/web/src/lib/locales/zh-CN.ts b/web/src/lib/locales/zh-CN.ts index c91029f1..7c52ba47 100644 --- a/web/src/lib/locales/zh-CN.ts +++ b/web/src/lib/locales/zh-CN.ts @@ -234,6 +234,7 @@ export default { 'chat.settings': '设置', 'chat.terminal': '终端', 'chat.switchRemote': '切换到远程模式', + 'chat.sendError.fallback': '消息未能发送。请修改后重试。', // Codex review 'codexReview.title': 'Codex review', @@ -407,6 +408,12 @@ export default { 'scratchlist.count.one': '1 条', 'scratchlist.count.other': '{n} 条', 'scratchlist.emptyHint': '在此暂存笔记、草稿或想法。需点击发送或编辑后才会真正发出。', + 'scratchlist.drawerHint': '在下方输入 — 发送会把内容存入此暂存清单，而不是发到对话。再次点击便签图标可退出。', + 'scratchlist.toggleAriaLabel': '暂存清单抽屉', + 'scratchlist.toggleTooltip': '暂存清单 — 暂存笔记与草稿（Ctrl/Cmd+Shift+S）', + 'scratchlist.sendToScratchlist': '存入暂存清单', + 'scratchlist.fueTitle': '新功能：暂存清单', + 'scratchlist.fueBody': '在此暂存笔记和草稿，不会被发送。暂存模式下发送按钮会显示琥珀色；再次点击图标（或 Ctrl/Cmd+Shift+S）可退出。', 'scratchlist.addPlaceholder': '笔记、草稿或想法 — 回车键添加', 'scratchlist.addAriaLabel': '添加草稿夹条目', 'scratchlist.add': '添加', @@ -417,7 +424,12 @@ export default { 'scratchlist.action.moveDown': '下移', 'scratchlist.action.promoteToComposer': '复制到输入框', 'scratchlist.action.promoteToQueue': '加入发送队列', + 'scratchlist.action.copy': '复制到剪贴板', + 'scratchlist.action.copied': '已复制！', 'scratchlist.action.delete': '删除条目', + 'fue.newFeatureDot': '新功能可用', + 'fue.gotIt': '知道了', + 'fue.closeAriaLabel': '关闭说明', 'composer.codexSlashUnsupported.title': '无法执行 Codex 命令', 'composer.codexSlashUnsupported.body': 'HAPI 远程模式暂不支持 {command} 这类 Codex 内建 slash command，请改用自然语言，或在本地 Codex TUI 中执行。', diff --git a/web/src/lib/use-fue.test.ts b/web/src/lib/use-fue.test.ts new file mode 100644 index 00000000..2110b35a --- /dev/null +++ b/web/src/lib/use-fue.test.ts @@ -0,0 +1,103 @@ +import { act, renderHook } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { resetFue, useFue } from './use-fue' + +const FEATURE = 'test-feature' +const STORAGE_KEY = `hapi.fue.v1.${FEATURE}` + +describe('useFue', () => { + beforeEach(() => { + localStorage.clear() + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('starts unseen for new features', () => { + const { result } = renderHook(() => useFue(FEATURE)) + expect(result.current.status).toBe('unseen') + }) + + it('reads acknowledged state from localStorage on mount', () => { + localStorage.setItem(STORAGE_KEY, '1') + const { result } = renderHook(() => useFue(FEATURE)) + expect(result.current.status).toBe('acknowledged') + }) + + it('engage() flips status to engaging once', () => { + const { result } = renderHook(() => useFue(FEATURE)) + act(() => { + result.current.engage() + }) + expect(result.current.status).toBe('engaging') + // Re-engage is a no-op (does not flip back). + act(() => { + result.current.engage() + }) + expect(result.current.status).toBe('engaging') + }) + + it('does NOT auto-acknowledge — engaging persists until dismiss is called', () => { + const { result } = renderHook(() => useFue(FEATURE)) + act(() => { + result.current.engage() + }) + // Advance the clock far past any plausible timeout. + act(() => { + vi.advanceTimersByTime(60_000) + }) + expect(result.current.status).toBe('engaging') + expect(localStorage.getItem(STORAGE_KEY)).toBeNull() + }) + + it('dismiss() acknowledges and persists', () => { + const { result } = renderHook(() => useFue(FEATURE)) + act(() => { + result.current.engage() + }) + act(() => { + result.current.dismiss() + }) + expect(result.current.status).toBe('acknowledged') + expect(localStorage.getItem(STORAGE_KEY)).toBe('1') + }) + + it('dismiss() also works directly from unseen (caller may skip the engaging step)', () => { + const { result } = renderHook(() => useFue(FEATURE)) + act(() => { + result.current.dismiss() + }) + expect(result.current.status).toBe('acknowledged') + expect(localStorage.getItem(STORAGE_KEY)).toBe('1') + }) + + it('engage() is a no-op once acknowledged', () => { + localStorage.setItem(STORAGE_KEY, '1') + const { result } = renderHook(() => useFue(FEATURE)) + act(() => { + result.current.engage() + }) + expect(result.current.status).toBe('acknowledged') + }) + + it('resetFue() clears storage so the badge re-appears', () => { + localStorage.setItem(STORAGE_KEY, '1') + resetFue(FEATURE) + expect(localStorage.getItem(STORAGE_KEY)).toBeNull() + const { result } = renderHook(() => useFue(FEATURE)) + expect(result.current.status).toBe('unseen') + }) + + it('switches state when featureId changes', () => { + localStorage.setItem('hapi.fue.v1.feature-a', '1') + const { result, rerender } = renderHook( + ({ id }: { id: string }) => useFue(id), + { initialProps: { id: 'feature-a' } } + ) + expect(result.current.status).toBe('acknowledged') + rerender({ id: 'feature-b' }) + expect(result.current.status).toBe('unseen') + }) +}) diff --git a/web/src/lib/use-fue.ts b/web/src/lib/use-fue.ts new file mode 100644 index 00000000..8dee0ceb --- /dev/null +++ b/web/src/lib/use-fue.ts @@ -0,0 +1,98 @@ +import { useCallback, useEffect, useState } from 'react' + +/** + * useFue — generic First-User-Experience badge / callout state machine. + * + * Goal: any new feature can advertise itself with a small dot on its + * affordance; the dot disappears for good once the user has engaged + * with it AND explicitly acknowledged the explainer. Hover surfaces a + * tooltip (caller's responsibility); click triggers `engage()`, which + * flips status to 'engaging' and renders the callout. Auto-timeout is + * deliberately not provided — reading speed varies, and a popover that + * disappears on its own undercuts the affirmative-action model. + * + * Storage: hapi.fue.v1. ('1' once acknowledged, absent otherwise) + * + * Status machine: + * unseen — initial. Badge visible, callout primed. + * engaging — operator has clicked the affordance for the first time. + * Callout is showing; awaiting explicit dismiss. + * acknowledged — terminal. Persisted to localStorage. Badge + callout + * suppressed forever (until storage is cleared). + * + * Independence from any upstream FUE: the storage namespace is + * `hapi.fue.v1.*` and feature IDs are caller-defined. If upstream/tiann + * adds a different onboarding flow that uses other keys / mechanisms, + * this system stays out of the way (caller decides whether to wrap a + * given affordance with FUE or not). + */ + +const STORAGE_PREFIX = 'hapi.fue.v1.' + +export type FueStatus = 'unseen' | 'engaging' | 'acknowledged' + +function readAcknowledged(featureId: string): boolean { + if (typeof window === 'undefined') return false + try { + return window.localStorage.getItem(STORAGE_PREFIX + featureId) === '1' + } catch { + return false + } +} + +function writeAcknowledged(featureId: string): void { + if (typeof window === 'undefined') return + try { + window.localStorage.setItem(STORAGE_PREFIX + featureId, '1') + } catch { + // localStorage may be unavailable (private mode, quota). Non-fatal: + // worst case the user sees the badge again next session. + } +} + +export function useFue(featureId: string): { + status: FueStatus + /** Call this on the first user-initiated engagement (typically the + * affordance's onClick). No-op if already engaging or acknowledged. */ + engage: () => void + /** Acknowledge permanently (call from the callout's "Got it" button or + * any other explicit affirmative action). */ + dismiss: () => void +} { + const [status, setStatus] = useState(() => + readAcknowledged(featureId) ? 'acknowledged' : 'unseen' + ) + + // Re-read on featureId change (different feature, different state). + useEffect(() => { + setStatus(readAcknowledged(featureId) ? 'acknowledged' : 'unseen') + }, [featureId]) + + const engage = useCallback(() => { + setStatus((prev) => (prev === 'unseen' ? 'engaging' : prev)) + }, []) + + const dismiss = useCallback(() => { + writeAcknowledged(featureId) + setStatus('acknowledged') + }, [featureId]) + + return { status, engage, dismiss } +} + +/** + * Test / dev-tool helper: clear acknowledgement for a feature so the FUE + * badge re-appears. Not used at runtime; expose via window for manual QA: + * + * localStorage.removeItem('hapi.fue.v1.scratchlist-toggle') + * + * Or call from a dev console: resetFue('scratchlist-toggle'). + */ +export function resetFue(featureId: string): void { + if (typeof window === 'undefined') return + try { + window.localStorage.removeItem(STORAGE_PREFIX + featureId) + } catch { + // ignore + } +} diff --git a/web/src/lib/use-scratchlist.test.ts b/web/src/lib/use-scratchlist.test.ts new file mode 100644 index 00000000..b6555ffb --- /dev/null +++ b/web/src/lib/use-scratchlist.test.ts @@ -0,0 +1,136 @@ +import { act, renderHook } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { addScratchlistEntry, persistScratchlist, readScratchlist } from './scratchlist' +import { useScratchlist } from './use-scratchlist' + +const SESSION_A = 'session-a' +const SESSION_B = 'session-b' + +describe('useScratchlist', () => { + beforeEach(() => { + localStorage.clear() + }) + + afterEach(() => { + localStorage.clear() + }) + + it('hydrates from localStorage on mount', () => { + const { entries: seeded } = addScratchlistEntry([], 'a-only', 1000) + persistScratchlist(SESSION_A, seeded) + const { result } = renderHook(({ id }: { id: string }) => useScratchlist(id), { + initialProps: { id: SESSION_A }, + }) + expect(result.current.entries.map((e) => e.text)).toEqual(['a-only']) + }) + + it('add() persists to the current sessions storage', () => { + const { result } = renderHook(({ id }: { id: string }) => useScratchlist(id), { + initialProps: { id: SESSION_A }, + }) + act(() => { + result.current.add('first') + }) + expect(readScratchlist(SESSION_A).map((e) => e.text)).toEqual(['first']) + expect(readScratchlist(SESSION_B)).toEqual([]) + }) + + it('switching sessions does NOT overwrite the new sessions storage with stale entries', () => { + // Regression test for the cross-session leak found by upstream review on PR #798. + // Seed both sessions distinctly; mount with A; rerender with B. + // The persist effect must not write A's entries into B's localStorage + // key during the brief render where the prop has changed but the + // rehydrate effect hasn't run yet. + const { entries: aEntries } = addScratchlistEntry([], 'a-original', 1000) + const { entries: bEntries } = addScratchlistEntry([], 'b-original', 2000) + persistScratchlist(SESSION_A, aEntries) + persistScratchlist(SESSION_B, bEntries) + + const { rerender } = renderHook(({ id }: { id: string }) => useScratchlist(id), { + initialProps: { id: SESSION_A }, + }) + + rerender({ id: SESSION_B }) + + // After the session switch, B's storage must still contain B's + // entry (not A's). Reading from disk because that's what the next + // mount of any other component would see. + expect(readScratchlist(SESSION_B).map((e) => e.text)).toEqual(['b-original']) + // A's storage stays intact too. + expect(readScratchlist(SESSION_A).map((e) => e.text)).toEqual(['a-original']) + }) + + it('after switching sessions, add() targets the new session', () => { + const { entries: aEntries } = addScratchlistEntry([], 'a-original', 1000) + persistScratchlist(SESSION_A, aEntries) + + const { result, rerender } = renderHook( + ({ id }: { id: string }) => useScratchlist(id), + { initialProps: { id: SESSION_A } } + ) + rerender({ id: SESSION_B }) + act(() => { + result.current.add('b-only') + }) + expect(readScratchlist(SESSION_B).map((e) => e.text)).toEqual(['b-only']) + expect(readScratchlist(SESSION_A).map((e) => e.text)).toEqual(['a-original']) + }) + + it('switching sessions never writes the previous sessions entries to the new sessions storage key', () => { + // The bot's review on PR #798 specifically called out the write + // window: between commit-with-new-id and the rehydrate effect + // running, the persist effect can fire one corrupting write + // (sessionId=B, entries=A's). That write self-heals on the next + // render once the rehydrate completes, so a "read after rerender" + // assertion would falsely pass. This test inspects every setItem + // call that happens during the rerender lifecycle and asserts no + // call wrote A's entries to B's storage key. + const { entries: aEntries } = addScratchlistEntry([], 'a-original', 1000) + const { entries: bEntries } = addScratchlistEntry([], 'b-original', 2000) + persistScratchlist(SESSION_A, aEntries) + persistScratchlist(SESSION_B, bEntries) + + const { rerender } = renderHook(({ id }: { id: string }) => useScratchlist(id), { + initialProps: { id: SESSION_A }, + }) + + const setItemSpy = vi.spyOn(Storage.prototype, 'setItem') + rerender({ id: SESSION_B }) + + // Storage format is a top-level array of entries (see writeScratchlist + // in scratchlist.ts), so unpack and inspect each entry directly. + const corruptingWrites = setItemSpy.mock.calls.filter(([key, value]) => { + if (typeof key !== 'string' || typeof value !== 'string') return false + if (!key.endsWith(SESSION_B)) return false + try { + const parsed = JSON.parse(value) + if (!Array.isArray(parsed)) return false + return parsed.some( + (e: { text?: string }) => e?.text === 'a-original' + ) + } catch { + return false + } + }) + setItemSpy.mockRestore() + + expect(corruptingWrites).toEqual([]) + }) + + it('remove() and move() use the loaded sessionId', () => { + const { entries: seeded } = addScratchlistEntry([], 'first', 1000) + const { entries: seeded2 } = addScratchlistEntry(seeded, 'second', 2000) + persistScratchlist(SESSION_A, seeded2) + + const { result } = renderHook(({ id }: { id: string }) => useScratchlist(id), { + initialProps: { id: SESSION_A }, + }) + + const firstId = result.current.entries[0]!.id + act(() => { + result.current.remove(firstId) + }) + expect(result.current.entries.map((e) => e.text)).toEqual(['first']) + expect(readScratchlist(SESSION_A).map((e) => e.text)).toEqual(['first']) + }) +}) diff --git a/web/src/lib/use-scratchlist.ts b/web/src/lib/use-scratchlist.ts new file mode 100644 index 00000000..9ce37590 --- /dev/null +++ b/web/src/lib/use-scratchlist.ts @@ -0,0 +1,84 @@ +import { useCallback, useEffect, useState } from 'react' +import { + addScratchlistEntry, + deleteScratchlistEntry, + moveScratchlistEntry, + persistScratchlist, + readScratchlist, + type ScratchlistEntry, +} from '@/lib/scratchlist' + +/** + * useScratchlist - per-session scratchlist state hook. + * + * Originally the entries lived inside ScratchlistPanel's useState. The + * composer-controlled drawer (v1.1) needs the same data exposed in two + * places (the drawer + the composer-toolbar counter), so the state is + * lifted here. localStorage stays the source of truth; this hook is the + * React mirror. + * + * Cross-session race protection + * ----------------------------- + * The naive shape (entries: useState, sessionId: prop, two useEffects) + * leaks across session navigation: + * + * 1. Mount with sessionId=A → entries = readScratchlist(A) = [a1, a2] + * 2. Parent rerenders with sessionId=B (same component instance — the + * v1 panel sidestepped this with key={props.session.id}; the v1.1 + * lifted hook can't, because its parent SessionChat *isn't* + * remounted on session switch). + * 3. React commits with sessionId=B but `entries` is still A's data. + * 4. Persist effect fires: persistScratchlist(B, [a1, a2]) — + * OVERWRITES B's storage with A's entries before the rehydrate + * effect has a chance to run. + * + * Fix (per upstream review on PR #798): keep the loaded sessionId in + * state alongside the entries so they can swap atomically, and persist + * against the LOADED sessionId, not the current prop. After step 2 the + * loaded sessionId is still A (until the rehydrate effect runs), so a + * spurious persist re-writes A's storage with A's entries — a no-op + * instead of a corruption. + */ +export function useScratchlist(sessionId: string) { + const [{ sessionId: loadedSessionId, entries }, setScratchlist] = useState<{ + sessionId: string + entries: ScratchlistEntry[] + }>(() => ({ sessionId, entries: readScratchlist(sessionId) })) + + // Rehydrate when the parent navigates to a different session. This + // atomically swaps both the loaded sessionId and the entries, so the + // persist effect below sees a consistent (sessionId, entries) pair. + useEffect(() => { + setScratchlist({ sessionId, entries: readScratchlist(sessionId) }) + }, [sessionId]) + + // Persist using the LOADED sessionId, not the prop. If the prop has + // moved ahead of the rehydrate effect, this still writes back to the + // session whose entries we currently hold — no cross-session leak. + useEffect(() => { + persistScratchlist(loadedSessionId, entries) + }, [loadedSessionId, entries]) + + const add = useCallback((rawText: string): boolean => { + const result = addScratchlistEntry(entries, rawText) + if (result.entries === entries) return false + setScratchlist({ sessionId: loadedSessionId, entries: result.entries }) + return true + }, [entries, loadedSessionId]) + + const remove = useCallback((id: string) => { + setScratchlist((prev) => ({ + sessionId: prev.sessionId, + entries: deleteScratchlistEntry(prev.entries, id), + })) + }, []) + + const move = useCallback((id: string, direction: 'up' | 'down') => { + setScratchlist((prev) => ({ + sessionId: prev.sessionId, + entries: moveScratchlistEntry(prev.entries, id, direction), + })) + }, []) + + return { entries, add, remove, move } +} diff --git a/web/src/router.tsx b/web/src/router.tsx index 64da9fbf..84e030a5 100644 --- a/web/src/router.tsx +++ b/web/src/router.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useMemo, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { useQueryClient } from '@tanstack/react-query' import { Navigate, @@ -30,7 +30,8 @@ import { useSession } from '@/hooks/queries/useSession' import { useSessions } from '@/hooks/queries/useSessions' import { useSlashCommands } from '@/hooks/queries/useSlashCommands' import { useSkills } from '@/hooks/queries/useSkills' -import { useSendMessage } from '@/hooks/mutations/useSendMessage' +import { useSendMessage, type SendErrorInfo } from '@/hooks/mutations/useSendMessage' +import type { ComposerSendError } from '@/components/AssistantChat/HappyComposer' import { queryKeys } from '@/lib/query-keys' import { useToast } from '@/lib/toast-context' import { useTranslation } from '@/lib/use-translation' @@ -572,6 +573,23 @@ function SessionsIndexPage() { return null } +/** + * Extract a user-facing message from a thrown send error. + * `request` in the api client throws plain `Error` for !res.ok, with the + * format `"HTTP : "` -- we surface the message as + * a single line and fall back to a localized default when nothing usable is + * present (e.g. an aborted fetch that resolved with no message). + */ +function deriveSendErrorMessage( + error: unknown, + t: (key: string) => string, +): string { + if (error instanceof Error && error.message) { + return error.message + } + return t('chat.sendError.fallback') +} + function SessionPage() { const { api } = useAppContext() const { t } = useTranslation() @@ -599,6 +617,30 @@ function SessionPage() { flushPending, setAtBottom, } = useMessages(api, sessionId) + + // Tracks the most recent send the hub rejected (4xx/5xx/network), keyed + // by the session the failed POST actually targeted (post-resolveSessionId). + // assistant-ui clears the composer eagerly when a send is invoked, so to + // retain the typed text on error we keep it here and hand it back to the + // composer for restore + visual error affordance. Keying by sessionId + // covers the inactive-session resume race: useSendMessage can resolve + // the target id, kick off async navigation to it, and then have the POST + // fail before navigation completes. Without keying, we'd restore the + // text into the OLD session's composer and the next render would clear + // it. The bumped `id` still lets the composer dedupe restorations of + // identical text. + const [sendErrors, setSendErrors] = useState>({}) + const sendErrorIdRef = useRef(0) + const sendError = sendErrors[sessionId] ?? null + const clearSendError = useCallback(() => { + setSendErrors((prev) => { + if (!(sessionId in prev)) return prev + const next = { ...prev } + delete next[sessionId] + return next + }) + }, [sessionId]) + const { sendMessage, retryMessage, @@ -607,8 +649,28 @@ function SessionPage() { isSessionThinking: session?.thinking ?? false, onSuccess: (sentSessionId) => { clearDraftsAfterSend(sentSessionId, sessionId) - // 中文注释：一旦用户已经在 Hapi 内继续这个 Codex 会话，就清除“刚从 Codex 导入”的标记。 + // 中文注释：一旦用户已经在 Hapi 内继续这个 Codex 会话，就清除"刚从 Codex 导入"的标记。 clearCodexImportedSession(session?.metadata?.codexSessionId) + // A successful send supersedes any previously-rendered error + // for that session. Other sessions' errors stay put. + setSendErrors((prev) => { + if (!(sentSessionId in prev)) return prev + const next = { ...prev } + delete next[sentSessionId] + return next + }) + }, + onError: (info: SendErrorInfo) => { + sendErrorIdRef.current += 1 + setSendErrors((prev) => ({ + ...prev, + [info.sessionId]: { + id: sendErrorIdRef.current, + text: info.text, + message: deriveSendErrorMessage(info.error, t), + scheduledAt: info.scheduledAt + } + })) }, resolveSessionId: async (currentSessionId) => { if (!api || !session || session.active) { @@ -746,6 +808,8 @@ function SessionPage() { onRetryMessage={retryMessage} autocompleteSuggestions={getAutocompleteSuggestions} availableSlashCommands={slashCommands} + sendError={sendError} + onClearSendError={clearSendError} /> ) }