diff --git a/web/src/components/AssistantChat/AttachmentItem.tsx b/web/src/components/AssistantChat/AttachmentItem.tsx index af08ff4f..36dc5187 100644 --- a/web/src/components/AssistantChat/AttachmentItem.tsx +++ b/web/src/components/AssistantChat/AttachmentItem.tsx @@ -1,5 +1,6 @@ import { AttachmentPrimitive, useThreadComposerAttachment } from '@assistant-ui/react' import { Spinner } from '@/components/Spinner' +import { useComposerParking } from '@/components/AssistantChat/composerParkingContext' function ErrorIcon() { return ( @@ -32,6 +33,7 @@ function RemoveIcon() { export function AttachmentItem() { const { name, status } = useThreadComposerAttachment() + const isParking = useComposerParking() const isUploading = status.type === 'running' const isError = status.type === 'incomplete' @@ -45,13 +47,15 @@ export function AttachmentItem() { ) : null} {name} {isError ? Upload failed : null} - - - + {!isParking ? ( + + + + ) : null} ) } diff --git a/web/src/components/AssistantChat/HappyComposer.parkSnapshot.test.ts b/web/src/components/AssistantChat/HappyComposer.parkSnapshot.test.ts new file mode 100644 index 00000000..0e483e86 --- /dev/null +++ b/web/src/components/AssistantChat/HappyComposer.parkSnapshot.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from 'vitest' +import { composerParkSnapshotUnchanged } from './HappyComposer' + +describe('composerParkSnapshotUnchanged', () => { + it('returns true when text and attachment ids match', () => { + const snapshot = { + text: 'hello', + attachments: [{ id: 'a1' }, { id: 'a2' }], + } + expect(composerParkSnapshotUnchanged(snapshot, { + text: 'hello', + attachments: [{ id: 'a1' }, { id: 'a2' }], + })).toBe(true) + }) + + it('returns false when text or attachments changed during park', () => { + const snapshot = { + text: 'hello', + attachments: [{ id: 'a1' }], + } + expect(composerParkSnapshotUnchanged(snapshot, { + text: 'hello world', + attachments: [{ id: 'a1' }], + })).toBe(false) + expect(composerParkSnapshotUnchanged(snapshot, { + text: 'hello', + attachments: [{ id: 'a1' }, { id: 'a2' }], + })).toBe(false) + expect(composerParkSnapshotUnchanged(snapshot, { + text: 'hello', + attachments: [{ id: 'a2' }], + })).toBe(false) + }) +}) diff --git a/web/src/components/AssistantChat/HappyComposer.tsx b/web/src/components/AssistantChat/HappyComposer.tsx index 7bc60f42..e6ac8dbe 100644 --- a/web/src/components/AssistantChat/HappyComposer.tsx +++ b/web/src/components/AssistantChat/HappyComposer.tsx @@ -38,6 +38,8 @@ import { StatusBar } from '@/components/AssistantChat/StatusBar' import { ComposerButtons } from '@/components/AssistantChat/ComposerButtons' import type { PendingSchedule } from '@/components/AssistantChat/ScheduleTimePicker' import { AttachmentItem } from '@/components/AssistantChat/AttachmentItem' +import { ComposerParkingContext } from '@/components/AssistantChat/composerParkingContext' +import type { ScratchlistParkResult } from '@/lib/scratchlistAttachmentFlow' import { useTranslation } from '@/lib/use-translation' import { getModelOptionsForFlavor, getNextModelForFlavor } from './modelOptions' import { getClaudeComposerEffortOptions } from './claudeEffortOptions' @@ -143,6 +145,18 @@ export function useRichComposerBridge( const defaultSuggestionHandler = async (): Promise => [] +/** True when composer text/attachment ids match a pre-park snapshot. */ +export function composerParkSnapshotUnchanged( + snapshot: { text: string; attachments: readonly { id: string }[] }, + current: { text: string; attachments: readonly { id: string }[] }, +): boolean { + return current.text === snapshot.text + && current.attachments.length === snapshot.attachments.length + && current.attachments.every( + (attachment, index) => attachment.id === snapshot.attachments[index]?.id, + ) +} + export function ModelEffortSettingsSection(props: { agentFlavor?: string | null options: Array<{ value: string; label: string }> @@ -260,6 +274,16 @@ export function HappyComposer(props: { scratchlistMode?: boolean scratchlistCount?: number onScratchlistToggle?: () => void + /** + * Prepare a scratchlist park (migrate only). Caller validates the + * composer snapshot, then commit()/abort()/beforeClear(). + */ + onParkScratchlist?: ( + text: string, + pending: readonly import('@assistant-ui/react').Attachment[], + ) => Promise + /** Parent disables DragDropZone / scratchlist promote while park is in flight. */ + onScratchlistParkingChange?: (parking: boolean) => 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. @@ -373,7 +397,15 @@ export function HappyComposer(props: { } }, [dictationActive, voiceInput.voiceMode, voiceStatus, onVoiceToggle, dictation.status, dictation.toggle]) - const controlsDisabled = disabled || (!active && !allowSendWhenInactive) || threadIsDisabled + const [isParkingScratchlist, setIsParkingScratchlist] = useState(false) + const parkInFlightRef = useRef(false) + const onScratchlistParkingChange = props.onScratchlistParkingChange + + useEffect(() => { + onScratchlistParkingChange?.(isParkingScratchlist) + }, [isParkingScratchlist, onScratchlistParkingChange]) + + const controlsDisabled = disabled || (!active && !allowSendWhenInactive) || threadIsDisabled || isParkingScratchlist const trimmed = composerText.trim() const hasText = trimmed.length > 0 const hasAttachments = attachments.length > 0 @@ -843,11 +875,49 @@ export function HappyComposer(props: { ? undefined : handleUserClearSchedule - /** Flush rich chips → `[title](/sessions/)` into composer.text, then send. */ - const flushAndSend = useCallback(() => { + const handleSend = useCallback(async () => { + // Rich chips must be serialized into composer.text before any send or + // scratchlist park snapshot (RichComposerInput contract). if (richMentionsEnabled && richInputRef.current) { richInputRef.current.flushSerializedText() } + + // Scratchlist parks must not go through assistant-ui's send(): it + // empties text/chips before onNew, so a rejected add cannot restore + // retryable composer state (#1226 Major). + if ( + props.scratchlistMode + && pendingSchedule == null + && props.onParkScratchlist + ) { + if (!canSend || parkInFlightRef.current) return + parkInFlightRef.current = true + setIsParkingScratchlist(true) + try { + const snapshot = api.composer().getState() + const prepared = await props.onParkScratchlist( + snapshot.text, + snapshot.attachments, + ) + if (!prepared) return + // Validate before irreversible add — otherwise a mid-flight + // composer edit leaves a parked duplicate while chips remain. + if (!composerParkSnapshotUnchanged(snapshot, api.composer().getState())) { + await prepared.abort() + return + } + if (!await prepared.commit()) { + return + } + await prepared.beforeClear() + api.composer().setText('') + await api.composer().clearAttachments() + } finally { + parkInFlightRef.current = false + setIsParkingScratchlist(false) + } + return + } // A retry intentionally clears composer state synchronously. It is // neither a replacement draft nor a dismissal: route onSuccess/onError // owns the inline-error transition for this new attempt. Drop the old @@ -860,7 +930,31 @@ export function HappyComposer(props: { userAttachmentGeneration: userAttachmentGenerationRef.current, } api.composer().send() - }, [api, attachments, onSuppressSendErrorRestore, richMentionsEnabled, sendError]) + // SessionChat owns clearing the schedule — it clears only after awaiting + // the send hook's accepted result, which covers both pre-mutation guards + // 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, + attachments, + canSend, + onSuppressSendErrorRestore, + pendingSchedule, + props.onParkScratchlist, + props.scratchlistMode, + richMentionsEnabled, + sendError, + ]) + + const flushAndSend = useCallback(() => { + void handleSend() + }, [handleSend]) const handleKeyDown = useCallback((e: ReactKeyboardEvent) => { const key = e.key @@ -956,7 +1050,7 @@ export function HappyComposer(props: { permissionMode, permissionModes, canSend, - api, + handleSend, haptic, composerEnterBehavior, richMentionsEnabled, @@ -1122,20 +1216,6 @@ export function HappyComposer(props: { const showAbortButton = true const voiceEnabled = Boolean(effectiveVoiceToggle) - const handleSend = useCallback(() => { - flushAndSend() - // SessionChat owns clearing the schedule — it clears only after awaiting - // the send hook's accepted result, which covers both pre-mutation guards - // 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. - }, [flushAndSend]) - // Pi: selected model info for UI labels and thinking level filtering const piModelLabel = agentFlavor === 'pi' ? (selectedPiModel?.name ?? selectedPiModel?.modelId ?? 'Model') @@ -1608,6 +1688,7 @@ export function HappyComposer(props: { : 'max-h-[7.5rem] min-h-[1.5rem] flex-1 overflow-y-auto whitespace-pre-wrap break-words bg-transparent text-base leading-snug text-[var(--app-fg)] focus:outline-none' return ( +
@@ -1784,5 +1865,6 @@ export function HappyComposer(props: {
+ ) } diff --git a/web/src/components/AssistantChat/composerParkingContext.ts b/web/src/components/AssistantChat/composerParkingContext.ts new file mode 100644 index 00000000..c777ae1c --- /dev/null +++ b/web/src/components/AssistantChat/composerParkingContext.ts @@ -0,0 +1,12 @@ +import { createContext, useContext } from 'react' + +/** + * True while HappyComposer is awaiting a scratchlist park. Attachment chips + * must not be removable mid-flight (would delete hub blobs the park still + * references). + */ +export const ComposerParkingContext = createContext(false) + +export function useComposerParking(): boolean { + return useContext(ComposerParkingContext) +} diff --git a/web/src/components/SessionChat.tsx b/web/src/components/SessionChat.tsx index a1c4918e..9f3c6367 100644 --- a/web/src/components/SessionChat.tsx +++ b/web/src/components/SessionChat.tsx @@ -45,10 +45,18 @@ import { ScratchlistMigrationBanner } from '@/components/AssistantChat/Scratchli import { assignThreadMessageIds, useHappyRuntime } from '@/lib/assistant-runtime' import type { OlderLoadOutcome } from '@/lib/message-window-store' import { createAttachmentAdapter } from '@/lib/attachmentAdapter' -import { createScratchlistAttachmentAdapter } from '@/lib/scratchlistAttachmentAdapter' import { + createScratchlistAttachmentAdapter, + type ScratchlistAttachmentAdapter, +} from '@/lib/scratchlistAttachmentAdapter' +import { + attachmentsNeedScratchlistMigration, + finalizeMigratedScratchlistParkCleanup, + prepareScratchlistParkAttachments, rehydrateScratchlistAttachmentsToComposer, - stageScratchlistAttachmentsForComposeSend + stageScratchlistAttachmentsForComposeSend, + type PendingParkAttachment, + type ScratchlistParkResult, } from '@/lib/scratchlistAttachmentFlow' import type { ScratchlistEntry } from '@/lib/scratchlist' import { isHubScratchlistAttachmentPath } from '@hapi/protocol' @@ -203,6 +211,11 @@ export function isScratchlistHotkeyBlockedTarget(target: EventTarget | null): bo * or to the regular chat send. Scratchlist entries support text and hub- * stored attachments; scheduled sends still fall through to chat. * + * Chat-path chips attached before scratchlist mode are migrated in the + * park-before-clear path (`prepareScratchlistParkAttachments`, #1226). + * This predicate still rejects non-hub paths as a fail-closed backstop + * for any leftover composer.send() route. + * * Pure / exported so it can be unit tested without mounting SessionChat. */ export function shouldRouteToScratchlist( @@ -548,14 +561,16 @@ function SessionChatInner(props: SessionChatProps) { } }, [allSessions, t]) const [scratchlistMode, setScratchlistMode] = useState(false) + const [isScratchlistParking, setIsScratchlistParking] = 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(() => { + if (isScratchlistParking) return setScratchlistMode((m) => !m) - }, []) + }, [isScratchlistParking]) /** * Global keyboard shortcut: Ctrl/Cmd + Shift + S toggles scratchlist * mode (open/close drawer + flip composer routing). @@ -580,6 +595,7 @@ function SessionChatInner(props: SessionChatProps) { */ useEffect(() => { const onKeyDown = (e: globalThis.KeyboardEvent) => { + if (isScratchlistParking) return if (!isScratchlistToggleHotkey(e)) return if (isScratchlistHotkeyBlockedTarget(e.target)) return e.preventDefault() @@ -587,7 +603,7 @@ function SessionChatInner(props: SessionChatProps) { } window.addEventListener('keydown', onKeyDown) return () => window.removeEventListener('keydown', onKeyDown) - }, []) + }, [isScratchlistParking]) /** * onSend wrapper: when scratchlist mode is on AND the submission is * not scheduled, route to scratchlist (text and/or hub attachments). @@ -598,15 +614,98 @@ function SessionChatInner(props: SessionChatProps) { * 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. + * + * Chat-path chips attached before scratchlist mode are migrated in + * the scratchlist attachment adapter's send() (#1226). If any + * non-hub path still reaches this wrapper, fail closed — never park + * text-only and clear chips. */ + // Stable handle so HappyComposer can release parked chips without + // deleting hub blobs when clearAttachments() calls adapter.remove(). + const scratchlistAdapterRef = useRef(null) + + /** + * Park from a live composer snapshot *before* assistant-ui's + * `composer.send()` empties text/chips. Returning false leaves the + * composer intact for retry (at-cap / hub error). + */ + const onParkScratchlist = useCallback( + async ( + text: string, + pending: readonly PendingParkAttachment[], + ): Promise => { + let prepared + try { + prepared = await prepareScratchlistParkAttachments( + props.api, + props.session.id, + pending, + ) + } catch { + return false + } + let aborted = false + const abort = async () => { + if (aborted) return + aborted = true + await finalizeMigratedScratchlistParkCleanup( + props.api, + props.session.id, + prepared, + false, + ) + } + return { + abort, + commit: async () => { + const accepted = await scratchlist.add(text, prepared) + if (!accepted) { + await abort() + return false + } + return true + }, + beforeClear: async () => { + await finalizeMigratedScratchlistParkCleanup( + props.api, + props.session.id, + prepared, + true, + ) + scratchlistAdapterRef.current?.releaseWithoutDelete( + pending.map((chip) => chip.id), + ) + }, + } + }, + [props.api, props.session.id, scratchlist], + ) + const onSendForComposer = useCallback( async ( text: string, attachments?: AttachmentMetadata[], scheduledAt?: number | null, ): Promise => { + if ( + scratchlistMode + && scheduledAt == null + && attachmentsNeedScratchlistMigration(attachments) + ) { + return false + } if (shouldRouteToScratchlist(scratchlistMode, attachments, scheduledAt)) { - return scratchlist.add(text, attachments) + // Legacy path if something still calls composer.send() while + // scratchlist mode is on. Prefer onParkScratchlist (clears + // only after accept). + const accepted = await scratchlist.add(text, attachments) + await finalizeMigratedScratchlistParkCleanup( + props.api, + props.session.id, + attachments, + accepted, + ) + return accepted } // If the user uploaded while scratchlist mode was on, then toggled // it off before send, pending items still carry hub paths. Stage @@ -1334,11 +1433,16 @@ function SessionChatInner(props: SessionChatProps) { const attachmentAdapter = useMemo(() => { if (!props.session.active) { + scratchlistAdapterRef.current = null return undefined } - return scratchlistMode - ? createScratchlistAttachmentAdapter(props.api, props.session.id) - : createAttachmentAdapter(props.api, props.session.id) + if (scratchlistMode) { + const adapter = createScratchlistAttachmentAdapter(props.api, props.session.id) + scratchlistAdapterRef.current = adapter + return adapter + } + scratchlistAdapterRef.current = null + return createAttachmentAdapter(props.api, props.session.id) }, [props.api, props.session.id, props.session.active, scratchlistMode]) const runtime = useHappyRuntime({ @@ -1396,7 +1500,7 @@ function SessionChatInner(props: SessionChatProps) { - + setScratchlistMode(false)} - disabled={props.isSending} + disabled={props.isSending || isScratchlistParking} /> ) : null} { expect(deleteScratchlistAttachment).toHaveBeenCalledWith('session-1', 'hub-race') }) }) + +describe('createScratchlistAttachmentAdapter.send migrates chat-path chips (#1226)', () => { + it('uploads pending chat-path file to hub scratchlist and returns hub metadata', async () => { + const hubAttachment = { + id: 'hub-migrated', + filename: 'before-mode.png', + mimeType: 'image/png', + size: 4, + path: 'hapi-hub:scratchlist/default/session-1/hub-migrated-before-mode.png', + } + const uploadScratchlistAttachment = vi.fn().mockResolvedValue({ + success: true, + attachment: hubAttachment, + }) + const deleteUploadFile = vi.fn().mockResolvedValue(undefined) + const api = { uploadScratchlistAttachment, deleteUploadFile } as never + const adapter = createScratchlistAttachmentAdapter(api, 'session-1') + const file = new File([new Uint8Array([137, 80, 78, 71])], 'before-mode.png', { type: 'image/png' }) + + const complete = await adapter.send({ + id: 'composer-chat-1', + type: 'file', + name: 'before-mode.png', + contentType: 'image/png', + file, + status: { type: 'requires-action', reason: 'composer-send' }, + path: '/tmp/hapi-blobs/session-1/before-mode.png', + previewUrl: 'data:image/png;base64,iVBORw0KGgo=', + } as never) + + expect(uploadScratchlistAttachment).toHaveBeenCalledWith( + 'session-1', + 'before-mode.png', + expect.any(String), + 'image/png', + ) + // Chat-path cleanup is deferred until scratchlist.add succeeds (#1226 review). + expect(deleteUploadFile).not.toHaveBeenCalled() + expect(complete.content).toEqual([ + { + type: 'text', + text: JSON.stringify({ + __attachmentMetadata: { + ...hubAttachment, + previewUrl: 'data:image/png;base64,iVBORw0KGgo=', + migratedFromPath: '/tmp/hapi-blobs/session-1/before-mode.png', + }, + }), + }, + ]) + }) + + it('throws when chat-path migrate upload fails so park does not silently drop', async () => { + const uploadScratchlistAttachment = vi.fn().mockResolvedValue({ + success: false, + error: 'quota', + }) + const api = { uploadScratchlistAttachment, deleteUploadFile: vi.fn() } as never + const adapter = createScratchlistAttachmentAdapter(api, 'session-1') + const file = new File([new Uint8Array([1])], 'x.png', { type: 'image/png' }) + + await expect(adapter.send({ + id: 'composer-chat-2', + type: 'file', + name: 'x.png', + contentType: 'image/png', + file, + status: { type: 'requires-action', reason: 'composer-send' }, + path: '/tmp/hapi-blobs/x.png', + } as never)).rejects.toThrow(/quota|Failed to migrate/i) + }) + + it('releaseWithoutDelete makes remove a no-op so clearAttachments keeps parked hubs', async () => { + const deleteScratchlistAttachment = vi.fn() + const deleteUploadFile = vi.fn() + const api = { deleteScratchlistAttachment, deleteUploadFile } as never + const adapter = createScratchlistAttachmentAdapter(api, 'session-1') + const pending = { + id: 'composer-hub-1', + type: 'file' as const, + name: 'a.png', + contentType: 'image/png', + status: { type: 'requires-action' as const, reason: 'composer-send' as const }, + path: 'hapi-hub:scratchlist/default/session-1/hub-1-a.png', + hubAttachment: { + id: 'hub-1', + filename: 'a.png', + mimeType: 'image/png', + size: 1, + path: 'hapi-hub:scratchlist/default/session-1/hub-1-a.png', + }, + } + + adapter.releaseWithoutDelete([pending.id]) + await adapter.remove(pending as never) + + expect(deleteScratchlistAttachment).not.toHaveBeenCalled() + expect(deleteUploadFile).not.toHaveBeenCalled() + }) +}) diff --git a/web/src/lib/scratchlistAttachmentAdapter.ts b/web/src/lib/scratchlistAttachmentAdapter.ts index cd40ff83..a427d87b 100644 --- a/web/src/lib/scratchlistAttachmentAdapter.ts +++ b/web/src/lib/scratchlistAttachmentAdapter.ts @@ -1,6 +1,9 @@ import type { AttachmentAdapter, Attachment, CompleteAttachment, PendingAttachment } from '@assistant-ui/react' import type { ScratchlistAttachmentMetadata } from '@hapi/protocol' -import { parseHubScratchlistAttachmentPath } from '@hapi/protocol' +import { + isHubScratchlistAttachmentPath, + parseHubScratchlistAttachmentPath, +} from '@hapi/protocol' import type { ApiClient } from '@/api/client' import { getRestoredUploadMetadata } from '@/lib/composer-attachment-drafts' import { isImageMimeType } from '@/lib/fileAttachments' @@ -40,8 +43,21 @@ export function hubAttachmentFromRestoredDraft( } } -export function createScratchlistAttachmentAdapter(api: ApiClient, sessionId: string): AttachmentAdapter { +export type ScratchlistAttachmentAdapter = AttachmentAdapter & { + /** + * After a successful park, `clearAttachments()` still calls `remove()`. + * Mark those chip ids so remove skips hub/chat deletes (blobs now live + * on the scratchlist entry). + */ + releaseWithoutDelete(ids: Iterable): void +} + +export function createScratchlistAttachmentAdapter( + api: ApiClient, + sessionId: string, +): ScratchlistAttachmentAdapter { const cancelledAttachmentIds = new Set() + const releasedWithoutDeleteIds = new Set() return { // assistant-ui uses the exact "*" sentinel for an allow-all adapter. @@ -49,6 +65,12 @@ export function createScratchlistAttachmentAdapter(api: ApiClient, sessionId: st // this adapter's add() method can run. accept: '*', + releaseWithoutDelete(ids: Iterable): void { + for (const id of ids) { + releasedWithoutDeleteIds.add(id) + } + }, + async *add({ file }): AsyncGenerator { const contentType = file.type || 'application/octet-stream' const restored = getRestoredUploadMetadata(file) @@ -163,17 +185,67 @@ export function createScratchlistAttachmentAdapter(api: ApiClient, sessionId: st }, async remove(attachment: Attachment): Promise { + if (releasedWithoutDeleteIds.delete(attachment.id)) { + return + } cancelledAttachmentIds.add(attachment.id) const pending = attachment as PendingScratchlistAttachment const hubId = pending.hubAttachment?.id if (hubId) { await api.deleteScratchlistAttachment(sessionId, hubId).catch(() => {}) + return + } + // Chat-path chip attached before scratchlist mode was enabled (#1226). + if (pending.path && !isHubScratchlistAttachmentPath(pending.path)) { + await api.deleteUploadFile(sessionId, pending.path).catch(() => {}) } }, async send(attachment: PendingAttachment): Promise { const pending = attachment as PendingScratchlistAttachment - const hubAttachment = pending.hubAttachment + let hubAttachment = pending.hubAttachment + let previewUrl = pending.previewUrl + let migratedFromPath: string | undefined + + // Attach-before-mode: composer still holds a chat-path pending from + // the normal upload adapter. Migrate into hub scratchlist storage + // so park keeps the image (#1226). Fail closed (throw) — empty + // content would park text-only and clear chips silently. + // + // Do NOT delete the chat-path upload here: send() runs before + // scratchlist.add succeeds. If park is rejected (at-cap / 409 / + // network), the composer keeps chips that still reference the + // chat path; deleting early poisons retry and orphans the hub + // blob. Cleanup is deferred to onSendForComposer. + if (!hubAttachment) { + const file = attachment.file + if (!file) { + throw new Error('Cannot park scratchlist attachment without file bytes') + } + const contentType = attachment.contentType || file.type || 'application/octet-stream' + const content = await fileToBase64(file) + const result = await api.uploadScratchlistAttachment( + sessionId, + attachment.name, + content, + contentType + ) + if (!result.success || !result.attachment) { + throw new Error( + result.error ?? 'Failed to migrate attachment to scratchlist storage' + ) + } + hubAttachment = result.attachment + if ( + pending.path + && !isHubScratchlistAttachmentPath(pending.path) + ) { + migratedFromPath = pending.path + } + if (!previewUrl && isImageMimeType(contentType) && file.size <= MAX_PREVIEW_BYTES) { + previewUrl = await fileToDataUrl(file) + } + } return { id: attachment.id, @@ -181,17 +253,16 @@ export function createScratchlistAttachmentAdapter(api: ApiClient, sessionId: st name: attachment.name, contentType: attachment.contentType, status: { type: 'complete' }, - content: hubAttachment - ? [{ - type: 'text', - text: JSON.stringify({ - __attachmentMetadata: { - ...hubAttachment, - previewUrl: pending.previewUrl - } - }) - }] - : [] + content: [{ + type: 'text', + text: JSON.stringify({ + __attachmentMetadata: { + ...hubAttachment, + previewUrl, + ...(migratedFromPath ? { migratedFromPath } : {}), + } + }) + }] } } } diff --git a/web/src/lib/scratchlistAttachmentFlow.test.ts b/web/src/lib/scratchlistAttachmentFlow.test.ts new file mode 100644 index 00000000..3f298199 --- /dev/null +++ b/web/src/lib/scratchlistAttachmentFlow.test.ts @@ -0,0 +1,300 @@ +import { describe, expect, it, vi } from 'vitest' +import type { AttachmentMetadata } from '@/types/api' +import { + attachmentsNeedScratchlistMigration, + finalizeMigratedScratchlistParkCleanup, + migrateChatPathAttachmentsToScratchlist, + prepareScratchlistParkAttachments, + type ParkAttachmentMetadata, +} from './scratchlistAttachmentFlow' + +function chatAttachment(path = '/tmp/hapi-blobs/a.png'): AttachmentMetadata { + return { + id: 'att-1', + filename: 'a.png', + mimeType: 'image/png', + size: 4, + path, + previewUrl: 'data:image/png;base64,aaaa', + } +} + +function hubAttachment(): AttachmentMetadata { + return { + id: 'hub-1', + filename: 'a.png', + mimeType: 'image/png', + size: 4, + path: 'hapi-hub:scratchlist/default/session-1/hub-1-a.png', + } +} + +describe('attachmentsNeedScratchlistMigration (#1226)', () => { + it('is false for empty / already-hub payloads', () => { + expect(attachmentsNeedScratchlistMigration(undefined)).toBe(false) + expect(attachmentsNeedScratchlistMigration([])).toBe(false) + expect(attachmentsNeedScratchlistMigration([hubAttachment()])).toBe(false) + }) + + it('is true when any attachment still has a chat upload path', () => { + expect(attachmentsNeedScratchlistMigration([chatAttachment()])).toBe(true) + expect(attachmentsNeedScratchlistMigration([hubAttachment(), chatAttachment()])).toBe(true) + }) +}) + +describe('migrateChatPathAttachmentsToScratchlist (#1226)', () => { + it('re-uploads chat-path items via scratchlist/upload and leaves hub items alone', async () => { + const migrated = { + id: 'hub-new', + filename: 'a.png', + mimeType: 'image/png', + size: 4, + path: 'hapi-hub:scratchlist/default/session-1/hub-new-a.png', + } + const uploadScratchlistAttachment = vi.fn().mockResolvedValue({ + success: true, + attachment: migrated, + }) + const deleteUploadFile = vi.fn().mockResolvedValue(undefined) + const api = { uploadScratchlistAttachment, deleteUploadFile } as never + + const hub = hubAttachment() + const chat = chatAttachment() + const contentBase64 = 'iVBORw0KGgo=' + const result = await migrateChatPathAttachmentsToScratchlist( + api, + 'session-1', + [hub, chat], + async (att) => { + expect(att.path).toBe(chat.path) + return contentBase64 + }, + ) + + expect(uploadScratchlistAttachment).toHaveBeenCalledTimes(1) + expect(uploadScratchlistAttachment).toHaveBeenCalledWith( + 'session-1', + 'a.png', + contentBase64, + 'image/png', + ) + // Chat-path delete is deferred until park succeeds. + expect(deleteUploadFile).not.toHaveBeenCalled() + expect(result).toEqual([ + hub, + { ...migrated, previewUrl: chat.previewUrl, migratedFromPath: chat.path }, + ]) + }) + + it('rolls back newly uploaded hub blobs and throws when one migrate fails', async () => { + const uploadScratchlistAttachment = vi.fn() + .mockResolvedValueOnce({ + success: true, + attachment: { + id: 'hub-ok', + filename: 'a.png', + mimeType: 'image/png', + size: 1, + path: 'hapi-hub:scratchlist/default/session-1/hub-ok-a.png', + }, + }) + .mockResolvedValueOnce({ success: false, error: 'too big' }) + const deleteScratchlistAttachment = vi.fn().mockResolvedValue(undefined) + const deleteUploadFile = vi.fn().mockResolvedValue(undefined) + const api = { + uploadScratchlistAttachment, + deleteScratchlistAttachment, + deleteUploadFile, + } as never + + await expect(migrateChatPathAttachmentsToScratchlist( + api, + 'session-1', + [chatAttachment('/tmp/a.png'), chatAttachment('/tmp/b.png')], + async () => 'YmFzZTY0', + )).rejects.toThrow(/too big|Failed to migrate/i) + + expect(deleteScratchlistAttachment).toHaveBeenCalledWith('session-1', 'hub-ok') + }) +}) + +describe('prepareScratchlistParkAttachments (#1226)', () => { + it('passes hub chips through and migrates chat-path chips without deleting the chat upload', async () => { + const hubMeta = { + id: 'hub-1', + filename: 'hub.png', + mimeType: 'image/png', + size: 2, + path: 'hapi-hub:scratchlist/default/session-1/hub-1-hub.png', + } + const migrated = { + id: 'hub-new', + filename: 'chat.png', + mimeType: 'image/png', + size: 1, + path: 'hapi-hub:scratchlist/default/session-1/hub-new-chat.png', + } + const uploadScratchlistAttachment = vi.fn().mockResolvedValue({ + success: true, + attachment: migrated, + }) + const deleteUploadFile = vi.fn() + const api = { uploadScratchlistAttachment, deleteUploadFile } as never + const chatFile = new File([new Uint8Array([1])], 'chat.png', { type: 'image/png' }) + + const result = await prepareScratchlistParkAttachments(api, 'session-1', [ + { + id: 'chip-hub', + name: 'hub.png', + contentType: 'image/png', + hubAttachment: hubMeta, + previewUrl: 'data:image/png;base64,hub', + }, + { + id: 'chip-chat', + name: 'chat.png', + contentType: 'image/png', + file: chatFile, + path: '/tmp/hapi-blobs/chat.png', + previewUrl: 'data:image/png;base64,chat', + }, + ]) + + expect(uploadScratchlistAttachment).toHaveBeenCalledTimes(1) + expect(deleteUploadFile).not.toHaveBeenCalled() + expect(result).toEqual([ + { ...hubMeta, previewUrl: 'data:image/png;base64,hub' }, + { + ...migrated, + previewUrl: 'data:image/png;base64,chat', + migratedFromPath: '/tmp/hapi-blobs/chat.png', + }, + ]) + }) + + it('reuses a restored hub path without re-uploading', async () => { + const uploadScratchlistAttachment = vi.fn() + const api = { uploadScratchlistAttachment } as never + const hubPath = 'hapi-hub:scratchlist/default/session-1/aaaaaaaa-bbbb-4ccc-8ddd-eeeeeeeeeeee-photo.png' + const file = new File([new Uint8Array([1, 2])], 'photo.png', { type: 'image/png' }) + + const result = await prepareScratchlistParkAttachments(api, 'session-1', [ + { + id: 'chip-restored', + name: 'photo.png', + contentType: 'image/png', + file, + path: hubPath, + previewUrl: 'data:image/png;base64,x', + }, + ]) + + expect(uploadScratchlistAttachment).not.toHaveBeenCalled() + expect(result).toEqual([{ + id: 'aaaaaaaa-bbbb-4ccc-8ddd-eeeeeeeeeeee', + filename: 'photo.png', + mimeType: 'image/png', + size: 2, + path: hubPath, + previewUrl: 'data:image/png;base64,x', + }]) + }) + + it('rolls back hub blobs when a later chip fails to migrate', async () => { + const uploadScratchlistAttachment = vi.fn() + .mockResolvedValueOnce({ + success: true, + attachment: { + id: 'hub-ok', + filename: 'a.png', + mimeType: 'image/png', + size: 1, + path: 'hapi-hub:scratchlist/default/session-1/hub-ok-a.png', + }, + }) + .mockResolvedValueOnce({ success: false, error: 'quota' }) + const deleteScratchlistAttachment = vi.fn().mockResolvedValue(undefined) + const api = { uploadScratchlistAttachment, deleteScratchlistAttachment } as never + const file = new File([new Uint8Array([1])], 'a.png', { type: 'image/png' }) + + await expect(prepareScratchlistParkAttachments(api, 'session-1', [ + { id: '1', name: 'a.png', contentType: 'image/png', file, path: '/tmp/a.png' }, + { id: '2', name: 'b.png', contentType: 'image/png', file, path: '/tmp/b.png' }, + ])).rejects.toThrow(/quota|Failed to migrate/i) + + expect(deleteScratchlistAttachment).toHaveBeenCalledWith('session-1', 'hub-ok') + }) +}) + +describe('finalizeMigratedScratchlistParkCleanup (#1226)', () => { + const hubPath = 'hapi-hub:scratchlist/default/session-1/hub-1-a.png' + const chatPath = '/tmp/hapi-blobs/a.png' + + it('deletes chat uploads after accepted park', async () => { + const deleteUploadFile = vi.fn().mockResolvedValue(undefined) + const deleteScratchlistAttachment = vi.fn() + const api = { deleteUploadFile, deleteScratchlistAttachment } as never + + await finalizeMigratedScratchlistParkCleanup( + api, + 'session-1', + [{ + id: 'hub-1', + filename: 'a.png', + mimeType: 'image/png', + size: 1, + path: hubPath, + migratedFromPath: chatPath, + } as ParkAttachmentMetadata], + true, + ) + + expect(deleteUploadFile).toHaveBeenCalledWith('session-1', chatPath) + expect(deleteScratchlistAttachment).not.toHaveBeenCalled() + }) + + it('deletes orphan hub blobs after rejected park so retry can remigrate', async () => { + const deleteUploadFile = vi.fn() + const deleteScratchlistAttachment = vi.fn().mockResolvedValue(undefined) + const api = { deleteUploadFile, deleteScratchlistAttachment } as never + + await finalizeMigratedScratchlistParkCleanup( + api, + 'session-1', + [{ + id: 'hub-1', + filename: 'a.png', + mimeType: 'image/png', + size: 1, + path: hubPath, + migratedFromPath: chatPath, + } as ParkAttachmentMetadata], + false, + ) + + expect(deleteScratchlistAttachment).toHaveBeenCalledWith('session-1', 'hub-1') + expect(deleteUploadFile).not.toHaveBeenCalled() + }) + + it('no-ops when nothing was migrated from a chat path', async () => { + const deleteUploadFile = vi.fn() + const deleteScratchlistAttachment = vi.fn() + const api = { deleteUploadFile, deleteScratchlistAttachment } as never + + await finalizeMigratedScratchlistParkCleanup( + api, + 'session-1', + [{ + id: 'hub-1', + filename: 'a.png', + mimeType: 'image/png', + size: 1, + path: hubPath, + }], + true, + ) + + expect(deleteUploadFile).not.toHaveBeenCalled() + expect(deleteScratchlistAttachment).not.toHaveBeenCalled() + }) +}) diff --git a/web/src/lib/scratchlistAttachmentFlow.ts b/web/src/lib/scratchlistAttachmentFlow.ts index 1d5cb2f8..1ca7199a 100644 --- a/web/src/lib/scratchlistAttachmentFlow.ts +++ b/web/src/lib/scratchlistAttachmentFlow.ts @@ -1,7 +1,11 @@ -import type { ScratchlistAttachmentMetadata } from '@hapi/protocol' +import { + isHubScratchlistAttachmentPath, + type ScratchlistAttachmentMetadata, +} from '@hapi/protocol' import type { ApiClient } from '@/api/client' import type { AttachmentMetadata } from '@/types/api' import { isImageMimeType } from '@/lib/fileAttachments' +import { hubAttachmentFromRestoredDraft } from '@/lib/scratchlistAttachmentAdapter' async function blobToBase64(blob: Blob): Promise { return new Promise((resolve, reject) => { @@ -20,6 +24,183 @@ async function blobToBase64(blob: Blob): Promise { }) } +/** Client-only marker stashed on park payloads after chat→hub migrate (#1226). */ +export type ParkAttachmentMetadata = AttachmentMetadata & { + migratedFromPath?: string +} + +/** True when any composer attachment still lives on the normal chat upload path. */ +export function attachmentsNeedScratchlistMigration( + attachments: AttachmentMetadata[] | undefined +): boolean { + return (attachments ?? []).some((att) => !isHubScratchlistAttachmentPath(att.path)) +} + +/** Result of preparing a scratchlist park (migrate only — add happens in commit). */ +export type ScratchlistParkPrepared = { + /** Drop orphan hub blobs created during prepare (snapshot changed / give up). */ + abort: () => Promise + /** Persist the entry. On false, orphans are already cleaned. */ + commit: () => Promise + /** After successful commit + snapshot still matches: delete chat uploads + release chips. */ + beforeClear: () => Promise +} + +export type ScratchlistParkResult = false | ScratchlistParkPrepared + +/** + * - accepted: drop the original chat uploads (hub blobs are on the entry) + * - rejected: drop the orphan hub blobs so retry can re-migrate from chat paths + */ +export async function finalizeMigratedScratchlistParkCleanup( + api: ApiClient, + sessionId: string, + attachments: AttachmentMetadata[] | undefined, + accepted: boolean, +): Promise { + const list = (attachments ?? []) as ParkAttachmentMetadata[] + const migrated = list.filter((att) => typeof att.migratedFromPath === 'string' && att.migratedFromPath.length > 0) + if (migrated.length === 0) return + + if (accepted) { + await Promise.allSettled( + migrated.map((att) => api.deleteUploadFile(sessionId, att.migratedFromPath!)) + ) + return + } + + await Promise.allSettled( + migrated.map((att) => api.deleteScratchlistAttachment(sessionId, att.id)) + ) +} + +/** + * Pending composer chip shape used when parking *before* assistant-ui + * `composer.send()` empties the UI (#1226 follow-up Major). + */ +export type PendingParkAttachment = { + id: string + name: string + contentType?: string + file?: File + path?: string + hubAttachment?: ScratchlistAttachmentMetadata + previewUrl?: string +} + +/** + * Build hub park metadata from live composer chips without clearing them. + * Chat-path items are migrated into hub storage and stamped with + * `migratedFromPath`; the original chat upload is left in place until + * `finalizeMigratedScratchlistParkCleanup(accepted=true)`. + */ +export async function prepareScratchlistParkAttachments( + api: ApiClient, + sessionId: string, + pending: readonly PendingParkAttachment[], +): Promise { + const prepared: ParkAttachmentMetadata[] = [] + const createdHubIds: string[] = [] + try { + for (const chip of pending) { + const contentType = chip.contentType || chip.file?.type || 'application/octet-stream' + const hubAttachment = chip.hubAttachment + ?? (chip.path && chip.file + ? hubAttachmentFromRestoredDraft(chip.path, chip.file, contentType) + : null) + if (hubAttachment) { + prepared.push({ + ...hubAttachment, + previewUrl: chip.previewUrl, + }) + continue + } + const file = chip.file + if (!file) { + throw new Error(`Cannot park attachment ${chip.name} without file bytes`) + } + const content = await blobToBase64(file) + const result = await api.uploadScratchlistAttachment( + sessionId, + chip.name, + content, + contentType, + ) + if (!result.success || !result.attachment) { + throw new Error( + result.error ?? `Failed to migrate attachment ${chip.name}` + ) + } + createdHubIds.push(result.attachment.id) + const migratedFromPath = + chip.path && !isHubScratchlistAttachmentPath(chip.path) + ? chip.path + : undefined + prepared.push({ + ...result.attachment, + previewUrl: chip.previewUrl, + ...(migratedFromPath ? { migratedFromPath } : {}), + }) + } + return prepared + } catch (error) { + await Promise.allSettled( + createdHubIds.map((id) => api.deleteScratchlistAttachment(sessionId, id)) + ) + throw error + } +} + +/** + * Re-upload chat-path attachments into hub scratchlist storage (#1226). + * + * Does **not** delete the original chat upload — callers must run + * `finalizeMigratedScratchlistParkCleanup` after the park attempt so a + * rejected add can retry from the chat path. On failure, newly created + * hub blobs are deleted so a partial migrate does not leak quota. + */ +export async function migrateChatPathAttachmentsToScratchlist( + api: ApiClient, + sessionId: string, + attachments: AttachmentMetadata[], + readContentBase64: (attachment: AttachmentMetadata) => Promise +): Promise { + const migrated: ParkAttachmentMetadata[] = [] + const createdHubIds: string[] = [] + try { + for (const attachment of attachments) { + if (isHubScratchlistAttachmentPath(attachment.path)) { + migrated.push(attachment) + continue + } + const content = await readContentBase64(attachment) + const result = await api.uploadScratchlistAttachment( + sessionId, + attachment.filename, + content, + attachment.mimeType + ) + if (!result.success || !result.attachment) { + throw new Error( + result.error ?? `Failed to migrate attachment ${attachment.filename}` + ) + } + createdHubIds.push(result.attachment.id) + migrated.push({ + ...result.attachment, + previewUrl: attachment.previewUrl, + migratedFromPath: attachment.path, + }) + } + return migrated + } catch (error) { + await Promise.allSettled( + createdHubIds.map((id) => api.deleteScratchlistAttachment(sessionId, id)) + ) + throw error + } +} + export async function stageScratchlistAttachmentsForComposeSend( api: ApiClient, sessionId: string,