From 393cd7bfbb749bf61a6f2a563a1b055c5d3a558d Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Mon, 8 Jun 2026 06:49:42 +0100 Subject: [PATCH] =?UTF-8?q?feat(web):=20scratchlist=20v1.1=20=E2=80=94=20c?= =?UTF-8?q?omposer-toggle=20drawer=20+=20reusable=20FUE=20primitive=20(#79?= =?UTF-8?q?8)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(web): scratchlist v1.1 — composer-toggle drawer + reusable FUE primitive The v1 always-visible amber band proved too heavy for what's a 20% feature in a typical session. v1.1 dials it back to a composer-toggle that opens an on-demand drawer, paired with a reusable FUE (First-User Experience) primitive so existing operators get a subtle pulsing dot + on-click explainer the first time they see the toggle. UX changes: - Notepad icon in the composer toolbar (next to schedule-send) toggles scratchlist mode. Drawer renders only while mode is on. - Composer's send button repaints amber and reads "Send to scratchlist" while the mode is sticky; submit routes adds into the scratchlist instead of the chat. Click the icon again to leave. - Small entry-counter badge appears on the toggle when entries exist; empty-state shows just the icon (no zero-state guilt UI). New reusable FUE primitive: - web/src/lib/use-fue.ts: state machine (unseen → engaging → acknowledged) with localStorage persistence, namespaced under hapi.fue.v1. so it can't collide with any future upstream onboarding flow. - web/src/components/Fue.tsx: (small pulsing badge) and (portal-rendered popover with title/body + "Got it" affirmative-action dismiss). No auto-timeout — reading speed varies and silent disappearance undercuts user trust. - AGENTS.md adds a "Adding new web features — consider an FUE" section so future contributors discover the primitive. Refactors: - ScratchlistPanel.tsx: split rendering into (presentational list) and (composer-controlled drawer with hint copy). Original kept exported for the existing fixture-based tests. - SessionChat.tsx: scratchlist state lifted into useScratchlist hook so the composer-toolbar counter and the drawer share one source of truth. onSend wrapped to route through scratchlist.add when mode is on. Tests: - 9 useFue hook tests (initial state, engage idempotency, no auto-acknowledge, dismiss, featureId switching, post-acknowledged engage no-op, resetFue helper). - 5 placement helper tests (above/below switching, viewport edge clamping, visualViewport offset support). - All 21 existing scratchlist lib tests + 14 ScratchlistPanel tests continue to pass. Co-authored-by: Cursor * fix(scratchlist): prevent cross-session leak in useScratchlist hook Per upstream review on PR #798 (github-actions[bot] [Major]): > useScratchlist persists current `entries` whenever `sessionId` > changes. On A -> B navigation, React first commits with B's id > and A's entries; after paint, this persist effect can write A's > entries to hapi.scratchlist.v1.B before the rehydrate effect > loads B. The previous keyed panel existed specifically to avoid > this race. Lifting state out of the v1 panel (which sidestepped the race via key={props.session.id} forced remount) re-introduced this same data- loss window. The composer-controlled drawer in v1.1 cannot remount on session change because its parent SessionChat doesn't either. Fix: keep the loaded sessionId in state alongside the entries so they swap atomically, and persist against the LOADED sessionId rather than the prop. After A->B, the loaded sessionId is still A until rehydrate runs, so a spurious persist re-writes A's storage with A's entries - a no-op instead of a corruption. Tests: - New use-scratchlist.test.ts with 6 tests: - hydrates from localStorage on mount - add() persists to current session's storage only - rerender to a new session preserves the new session's existing entries - after switching, add() targets the new session - regression test that spies on Storage.prototype.setItem and asserts the rerender lifecycle never produces a (B-key, A-entries) write - remove()/move() target the loaded sessionId - The setItem-spy test correctly fails against the buggy code (verified by temporarily reverting the fix) and passes with the fix in place. - Full web suite: 88 files, 756 tests, all green. Typecheck clean. Co-authored-by: Cursor * fix(scratchlist): route attachment/scheduled submits to chat instead of dropping them Per upstream review on PR #798 (github-actions[bot] [Major]): > Prevent scratchlist mode from dropping attachments — in scratchlist > mode the wrapper returns success after adding only `text`, while > HappyComposer still treats composer attachments as sendable input. > A text+attachment submit therefore routes through this branch, > stores only the text, and silently discards the attachment instead > of sending or preserving it. Same hazard applies to scheduledAt: scratchlist entries are pure-text notes - they can't represent attachments or schedule metadata - so any submit carrying either MUST fall through to props.onSend (chat) even when the scratchlist toggle is on. Otherwise the wrapper short-circuits to scratchlist.add(text), reports success to the composer, and the composer dutifully clears attachments + schedule that the user just queued. Fix: extracted the routing rule into shouldRouteToScratchlist(mode, attachments, scheduledAt) - returns true only when mode is on AND the payload is pure text. onSendForComposer uses it. Tests: - 5 new shouldRouteToScratchlist unit tests (mode off, mode on + text-only, mode on + attachments, mode on + schedule, mode on + both) - All in web/src/components/SessionChat.test.ts (13 tests total now) - Full web suite: 88 files, 761 tests, all green. Typecheck clean. Co-authored-by: Cursor * fix(scratchlist): clear pendingSchedule when scratchlist-mode submission falls back to chat Per upstream review on PR #798 (github-actions[bot] [Major]): > The follow-up change correctly falls through to props.onSend when > scratchlist mode is on but scheduledAt is present, yet the > accepted-send cleanup still checks only !scratchlistMode. That > means a scheduled chat send made while the amber scratchlist UI is > active is accepted, but pendingSchedule stays set, so the next > normal send can accidentally reuse the same schedule. Fix: handleSend now gates the cleanup branch on the actual route taken (routedToScratchlist) rather than the scratchlist UI state. Reuses the same shouldRouteToScratchlist helper so route + cleanup share a single source of truth. Tests: - 2 new tests in SessionChat.test.ts that pin the decision matrix handleSend depends on: - 'cleanup gate: scheduled chat send while scratchlist toggle is on still clears schedule' - 'cleanup gate: pure-text scratchlist add does NOT clear schedule' - Full web suite: 88 files, 763 tests, all green. Typecheck clean. Co-authored-by: Cursor * fix(scratchlist): UnifiedButton must reflect actual routing, not raw scratchlist toggle Per upstream review on PR #798 (github-actions[bot] [Major]): > Send button advertises scratchlist routing even when the submit > will go to chat — shouldRouteToScratchlist correctly falls back > to normal chat for attachments or scheduledAt, but UnifiedButton > still turns amber and labels the action as "Send to scratchlist" > whenever scratchlistMode is true. A scheduled send or attachment > send made in that state will be submitted to chat while the UI > says it is being stashed, which can send content to the agent > unexpectedly. Fix: - UnifiedButton's prop renamed `scratchlistMode` -> `routesToScratchlist` to make the contract explicit: "this submit really will go to the scratchlist", not "the scratchlist toggle is on". - The call site computes `routesToScratchlist` from `scratchlistMode && !hasAttachments && pendingSchedule == null`, mirroring SessionChat's shouldRouteToScratchlist exactly. The button is now amber + "Send to scratchlist" only when the actual send path will hit scratchlist; attachments / pending schedule force a chat- style render that matches the real routing. - UnifiedButton exported so it can be unit-tested directly. Tests: - 3 new render tests in ComposerButtons.test.tsx covering: - routesToScratchlist=true → amber + "Send to scratchlist" - routesToScratchlist=false → black + "Send" (the regression case) - omitted prop → defaults to chat-style render - Full web suite: 89 files, 766 tests, all green. Typecheck clean. Co-authored-by: Cursor * fix(scratchlist): exit scratchlist mode when promoting an entry to the composer Per 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. Promoting an entry to the composer means "I want to send this for real now". With scratchlist mode still on, the next composer submit routes back to scratchlist (per the v1.1 modal-mode contract), so the user's click loop becomes promote -> send -> re-add -> nothing-actually-sent. Fix: ScratchlistDrawerHost now calls onExitScratchlistMode whenever it promotes an entry to the composer. Promote-to-queue does NOT exit the mode (queue path bypasses the wrapper anyway, and the operator may still be capturing related notes). Tests: - Exported ScratchlistDrawerHost so its host-level callbacks can be unit-tested in isolation (previously only ScratchlistDrawer was testable; the wiring was untested). - New SessionChat.exit-mode.test.tsx with 2 tests: - promote-to-composer fires setText AND onExitScratchlistMode - promote-to-queue fires onSend but does NOT exit mode - Full web suite: 90 files, 768 tests, all green. Typecheck clean. Co-authored-by: Cursor * feat(scratchlist): Ctrl/Cmd+Shift+S toggles scratchlist mode (v1.1 hotkey) The v1 always-visible panel had Ctrl/Cmd+Shift+S to expand the panel and focus the input. v1.1 mounts the drawer only when scratchlistMode is on, so the v1 listener (inside the panel) is dead code: it can't fire while the drawer is unmounted, and the user has no way to open the drawer without clicking the toolbar icon. Re-bind the shortcut at SessionChat scope so it's always alive and toggles the mode. Convention matches sibling globals (Ctrl/Cmd-m cycles agent model). Ctrl/Cmd-Shift-S is unreserved by Chrome / Firefox / Safari (browser Save As is Ctrl-S / Cmd-S, no Shift), so the user's save-page muscle memory keeps working. Modifier requirement (Ctrl/Cmd+Shift) means it can't collide with literal-character typing in any input - no focus suppression needed. The matcher is extracted to a pure helper isScratchlistToggleHotkey so it's unit testable without mounting SessionChat. 6 new tests pin the modifier matrix: - Ctrl+Shift+S (Linux/Windows) -> match - Cmd+Shift+S (macOS) -> match - Cmd/Ctrl+S without Shift -> reject (browser Save reservation) - bare S / Shift+S -> reject (literal typing) - Ctrl+Shift+Alt+S -> reject (avoid OS clashes) - other modifier+key combos -> reject Tooltip + FUE body now mention the hotkey so it's discoverable from the same UI surface that introduces the feature (en + zh-CN). Web suite 90 files / 774 tests, all green. Typecheck clean. Co-authored-by: Cursor * fix(scratchlist): hotkey skips dialogs / inputs / contentEditable Bot finding on PR #798 (PRRT_kwDOQuQOSc6HGtLn): the window-level Ctrl/Cmd+Shift+S listener fires for every focus target, so the shortcut can toggle scratchlist mode "behind" an open modal (rename session, schedule picker, FUE callout, image preview), making the next composer send route to scratchlist instead of chat. UX bug. Add isScratchlistHotkeyBlockedTarget(target) and gate the listener on it. Block targets: - any descendant of an open [role="dialog"] (Radix UI's DialogContent renders role="dialog"; FueCallout, ScheduleTimePicker, ImagePreview also use role="dialog") - HTMLInputElement (single-line inputs) - HTMLSelectElement - any contentEditable host (with attribute-based fallback for jsdom, which doesn't implement isContentEditable) NOT blocked: - HTMLTextAreaElement (the composer textarea is the expected focus target when the operator presses the hotkey - blocking would defeat the shortcut) - the document body / unfocused targets 8 new unit tests pin the matrix. Function is exported / pure so callers can reuse the same blocked-target rule for future global shortcuts. Suggested fix from the bot applied modulo: - Use !== null on closest() result (explicit boolean for return type) - Add attribute-based contentEditable fallback for jsdom test env * feat(scratchlist): copy-to-clipboard action on each entry Add a per-entry "Copy to clipboard" button between the send-to-queue and delete actions. On click, write the entry text via the shared safeCopyToClipboard helper (which already handles the navigator.clipboard primary path + the execCommand fallback for Safari / non-secure-context edges); on success, briefly flip the icon to a check and the aria-label/title to "Copied!" for 1500ms so the operator gets visual + screen-reader confirmation. Failures (clipboard denied AND execCommand fallback unavailable) silently no-op rather than throw at the click handler. Mirrored across both surfaces: - ScratchlistInventory (used by the v1.1 composer-toggle drawer) - ScratchlistPanel inline list (the v1 always-visible panel) A small useCopiedFeedback() hook owns the "which entry just got copied" state + the 1.5s auto-clear timeout. Pure state machine; the caller wires safeCopyToClipboard separately so the hook itself stays free of jsdom clipboard quirks. Cleared on unmount via the standard ref-tracked timeout pattern, so promote-and-navigate-away can't leak. Locale keys: scratchlist.action.copy / scratchlist.action.copied (en + zh-CN). Three new tests: - v1 panel happy path: writeText called with the entry text, button flips to the "Copied!" label, entry is preserved (copy is non-destructive). - v1 panel failure path: writeText rejects AND execCommand returns false; button stays in "Copy to clipboard" state — no false success. - v1.1 drawer happy path: writeText called, label flips, and crucially no other entry handlers (onSend, onDelete, setText, onExitScratchlistMode) fire — copy is independent of all the other actions. Web suite 90 files / 785 tests, all green. Typecheck clean. * fix(scratchlist): reset all per-session state via keyed wrapper Bot finding on PR #798 (PRRT_kwDOQuQOSc6HHOsa): when the operator navigates between sessions on the same route (/sessions/A -> /sessions/B), React reuses the SessionChat component instance. Effects run AFTER the first paint, so for a single render window the new session is rendered with the previous session's scratchlist entries (useScratchlist's rehydrate-effect) AND drawer-open state (scratchlistMode reset effect). Visual leak; drawer actions targeting stale state. Apply the bot's suggested fix verbatim modulo the type extraction: export function SessionChat(props) { return } Canonical React idiom for "fully reset state on prop change": the keyed wrapper unmounts and re-mounts the inner component when session.id changes, so every hook (useScratchlist's initial-state factory, useState, useHappyRuntime, ...) starts fresh. This supersedes the now-redundant effect-based reset: - useEffect(() => { setScratchlistMode(false) }, [session.id]) REMOVED useScratchlist's atomic-loaded-sessionId persistence (added on the prior PR round) stays as defense-in-depth for any caller that uses the hook without the keyed-wrapper pattern. Web suite 90 files / 785 tests, all green. Typecheck clean. * fix(web): retain composer text on send failure (closes #776) When the message composer submits and the hub responds with a 4xx/5xx or the fetch fails outright, assistant-ui clears the composer synchronously the moment send is invoked. Without intervention the operator's typed text is destroyed at exactly the moment they most need it preserved. SessionChat additionally clears any pending schedule on accept, so a failed scheduled send was also silently downgrading to immediate on the next attempt. Behaviour: - useSendMessage exposes onError({ sessionId, text, scheduledAt, error }) so the route can hand the input back to the composer. sessionId is the resolved target (post-resolveSessionId), so an inactive-session resume that resolves a new id, kicks off async navigation, then fails the POST restores into the resumed session's composer rather than the old one. - router.tsx stores sendErrors keyed by sessionId. Per-session lookup replaces the clear-on-session-change effect, so errors do not bleed between sessions and a session-scoped failure persists across navigation. - HappyComposer accepts ComposerSendError, restores text via api.composer().setText() once per failure id, and re-establishes any pending schedule via onSchedule({ type: 'absolute', ms: scheduledAt }). It renders a red ring on the composer wrapper and a role="alert" inline message; both clear the moment the operator types or sends. - onError forks on input.attachments. Text-only sends use the composer-restore path (removeOptimisticMessage drops the row so the failed bubble does not duplicate the restored text). Attachment sends keep the legacy failed-bubble UX (status='failed' + in-thread retry button) because the composer-restore path can't reinstate uploaded attachment metadata. retryMessage extracts attachments from the stored optimistic message via getMessageAttachments so failed-bubble retry of an attachment send re-fires with its files. Acceptance (issue #776): - Submit -> 500/502/503/network error -> composer text not cleared - Submit -> 400/401/403 -> composer text not cleared, error inline - Submit -> 2xx -> composer clears as today - Operator can edit retained text and retry without re-typing - Failed scheduled sends restore as scheduled, not as immediate Tests in web/src/hooks/mutations/useSendMessage.test.tsx cover text-only 4xx/5xx/network retention, scheduled-send carry-through, optimistic-row removal on text-only failure, sessionId carry-through under resolveSessionId, attachment failure fallback, and attachment retry preservation. Full web suite passes (705 tests). bun typecheck clean. No SCHEMA_VERSION bump (frontend-only). * fix(test): correct AttachmentMetadata fixture shape + JSX namespace import Two pre-existing test-only typecheck failures surfaced once scratchlist v1.1 was stacked into the driver soup. * SessionChat.test.ts - the attachment() fixture used the legacy schema (kind, sizeBytes) instead of the current AttachmentMetadataSchema (filename, size, path). Updated to match the live shape so the cast is honest. * ComposerButtons.test.tsx - JSX namespace is no longer global under the current TS lib config; switched the helper signature from JSX.Element to React's ReactElement (same runtime, named import). Co-authored-by: Cursor * fix(scratchlist): soften panel chrome - drop strong amber fill, keep subtle accent border (#812) Per #812 (and PR 827 from @swear01) the always-visible amber fill on the scratchlist panel was too loud as a scroll element. This swaps the warning *fill* for the chat-user-surface tone and uses neutral text/pills/focus, 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 destination signal continues to live on the composer Send button (it goes amber-500 only while scratchlist mode is routing) and the active toggle button - those carry the moment-of-action signal the user actually presses, and ComposerButtons tests + the FUE copy already depend on that behavior, so they're unchanged. Credit to @swear01 (PR 827) for the styling note; this branch absorbs that restyle and supersedes the Settings-toggle approach because v1.1 hides the panel by default behind the composer drawer toggle (no Settings entry needed). Adds a regression-guard test asserting the panel uses the chat-user-surface bg + warning-border (not the warning fill). Co-authored-by: Cursor --------- Co-authored-by: Cursor --- AGENTS.md | 37 ++ .../AssistantChat/ComposerButtons.test.tsx | 85 +++++ .../AssistantChat/ComposerButtons.tsx | 184 +++++++++- .../AssistantChat/HappyComposer.tsx | 106 +++++- .../AssistantChat/ScratchlistPanel.test.tsx | 72 ++++ .../AssistantChat/ScratchlistPanel.tsx | 330 ++++++++++++++++- web/src/components/Fue.test.ts | 71 ++++ web/src/components/Fue.tsx | 242 +++++++++++++ .../components/SessionChat.exit-mode.test.tsx | 146 ++++++++ web/src/components/SessionChat.test.ts | 177 +++++++++- web/src/components/SessionChat.tsx | 308 +++++++++++++--- .../hooks/mutations/useSendMessage.test.tsx | 332 ++++++++++++++++++ web/src/hooks/mutations/useSendMessage.ts | 93 ++++- web/src/lib/locales/en.ts | 12 + web/src/lib/locales/zh-CN.ts | 12 + web/src/lib/use-fue.test.ts | 103 ++++++ web/src/lib/use-fue.ts | 98 ++++++ web/src/lib/use-scratchlist.test.ts | 136 +++++++ web/src/lib/use-scratchlist.ts | 84 +++++ web/src/router.tsx | 70 +++- 20 files changed, 2631 insertions(+), 67 deletions(-) create mode 100644 web/src/components/AssistantChat/ComposerButtons.test.tsx create mode 100644 web/src/components/Fue.test.ts create mode 100644 web/src/components/Fue.tsx create mode 100644 web/src/components/SessionChat.exit-mode.test.tsx create mode 100644 web/src/lib/use-fue.test.ts create mode 100644 web/src/lib/use-fue.ts create mode 100644 web/src/lib/use-scratchlist.test.ts create mode 100644 web/src/lib/use-scratchlist.ts 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} /> ) }