Skip to content

fix(web): preserve drafts across inactive session resume - #1378

Merged
tiann merged 40 commits into
tiann:mainfrom
techotaku39:fix/inactive-session-attachments
Aug 8, 2026
Merged

fix(web): preserve drafts across inactive session resume#1378
tiann merged 40 commits into
tiann:mainfrom
techotaku39:fix/inactive-session-attachments

Conversation

@techotaku39

Copy link
Copy Markdown
Contributor

Summary

  • allow attachments to be selected while a resumable session is inactive
  • resume the session before uploading, then hand the pending composer draft to the resolved session
  • preserve text and attachment drafts across archive, session switches, and every reopen entry point
  • navigate to the resolved session without waiting on stale-session refetches, preventing transient Session unavailable 404 pages

Problem

Composer text was already persisted per session after #231/#438, but attachment drafts and inactive-session transitions were incomplete:

  • inactive sessions could not accept attachments
  • archiving a session and switching away dropped pending attachments
  • reopening could return a different session ID without transferring its text and attachment draft
  • the old session route could refetch after a successful resume and render HTTP 404: Session not found

Implementation

  • extend the attachment adapter with an optional session resolver and resolved-session handoff
  • persist attachment upload metadata so restored drafts do not upload the same file twice
  • add a shared composer-draft transfer helper backed by live snapshots with IndexedDB/session-storage fallback
  • use the transfer helper from the header, session-list, and inline reopen flows
  • optimistically seed and navigate to the resolved session before background query synchronization
  • keep attachment restore enabled for resumable inactive sessions

Testing

  • bun run typecheck — passed
  • bun run test:web — passed
  • bun run test:shared — passed
  • targeted web tests — 12 passed
  • live Playwright test against an isolated Hub and Runner on port 3016:
    • attach a real file and enter text
    • archive the real Codex session
    • switch sessions and return
    • reopen the session
    • verify text and attachment drafts survive
    • verify no session API 404 occurs

AI disclosure

Code and test preparation used OpenAI Codex (GPT-5.6). The changes were reviewed and exercised with unit, type, build, and live browser tests.

Related: #231

@techotaku39
techotaku39 force-pushed the fix/inactive-session-attachments branch from 23676f8 to 9f802f8 Compare August 5, 2026 04:39

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [MAJOR] Reusing session-scoped upload paths after archive/resume — web/src/lib/attachmentAdapter.ts:52, web/src/lib/composer-draft-transfer.ts:37-39
    The inactive-session restore now keeps a persisted path and returns without uploading. The CLI removes its upload directory on session end (cli/src/api/apiSession.ts:1130) and authorizes files by the active session id (cli/src/pi/runPi.ts:765-766). After archive/reopen the path can be deleted; when resume merges into a new id, the target cannot authorize the source path, so the subsequent message contains an unreadable attachment.
    Suggested fix:

    const restored = resolveSessionId ? undefined : getRestoredUploadMetadata(file)
    const transferable = sourceSessionId === targetSessionId
        ? attachments
        : attachments.map((attachment) => ({
            ...attachment,
            path: undefined,
            previewUrl: undefined,
            uploadSessionId: undefined,
        }))

    Force a fresh upload for inactive/changed-id drafts, or migrate the blob server-side.

  • [MAJOR] Empty live snapshots can overwrite hydrated drafts — web/src/components/AssistantChat/HappyComposer.tsx:605, web/src/lib/composer-draft-transfer.ts:28-31
    useComposerDraft restores text after a RAF and attachments after an async IndexedDB read (web/src/hooks/useComposerDraft.ts:62-99), but this effect records the initial empty composer immediately. transferComposerDraft always prefers that live snapshot over persisted storage, so a quick Header/List reopen during hydration saves ''/[] to the new id and drops the draft.
    Suggested fix:

    useEffect(() => {
        if (!draftHydration.complete) return
        if (sessionId) setComposerDraftSnapshot(sessionId, composerText, attachmentDrafts)
        props.onUploadDraftSnapshot?.(composerText, attachmentDrafts)
    }, [draftHydration.complete, attachmentDrafts, composerText, props.onUploadDraftSnapshot, sessionId])
  • [MAJOR] Suppressed retry errors no longer follow the resolved session — web/src/router.tsx:588
    HappyComposer marks the existing error restoreSuppressed before a retry (web/src/components/AssistantChat/HappyComposer.tsx:1080), and useSendMessage calls onSessionResolved before the mutation (web/src/hooks/mutations/useSendMessage.ts:271-274). The old callback migrated this record with migrateSuppressedSendError; the new shared callback only navigates, leaving state keyed to the old id while success/error callbacks use the new id. Failed resumed retries can therefore lose the intended recovery state or leave a stale suppressed record.
    Suggested fix:

    onSessionResolved: (resolvedSessionId) => {
        setSendErrors((prev) => migrateSuppressedSendError(prev, sessionId, resolvedSessionId))
        handleSessionResolved(resolvedSessionId)
    }
  • [MINOR] Live snapshot map retains every attachment blob — web/src/lib/composer-draft-transfer.ts:14-21
    The new map stores File objects for every visited session but has no eviction or unmount cleanup. The IndexedDB/cache path is capped at 50 drafts, while this map is not; switching sessions can retain multiple 50 MB files (and their contents) until a full page reload.
    Suggested fix:

    export function clearComposerDraftSnapshot(sessionId: string): void {
        liveSnapshots.delete(sessionId)
    }

    Call it from the composer cleanup and after a transfer, or cap the map and avoid retaining blobs.

Questions

  • None.

Summary
Review mode: initial
The diff introduces two draft data-loss paths around asynchronous hydration and session-scoped upload lifetimes, plus a regression in failed-send recovery state. Attachment/reopen integration coverage is missing for these races and for a resume that returns a different session id.

Testing

  • Not run (automation): bun is unavailable in this environment.

HAPI Bot

Comment thread web/src/lib/attachmentAdapter.ts
Comment thread web/src/components/AssistantChat/HappyComposer.tsx Outdated
Comment thread web/src/router.tsx Outdated
Comment thread web/src/lib/composer-draft-transfer.ts

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [MAJOR] Hydration-triggered resume can erase the transferred draft - web/src/components/SessionChat.tsx:1631
    useComposerDraft awaits each restored addAttachment before marking hydration complete (web/src/hooks/useComposerDraft.ts:78), while HappyComposer now withholds this snapshot until completion (web/src/components/AssistantChat/HappyComposer.tsx:605). For an inactive session, the restored attachment itself reaches this callback first, so the ref is still its initial ''/[]; saving it to the resolved id loses the persisted text and files and also bypasses transferComposerDraft's upload-metadata stripping.
    Suggested fix:

    async (resolvedSessionId) => {
        await transferComposerDraft(props.session.id, resolvedSessionId)
        props.onUploadSessionResolved?.(resolvedSessionId)
    },
  • [MAJOR] Updated hook contract leaves the required test suite red - web/src/components/AssistantChat/HappyComposer.tsx:605
    The new dereference is valid for the production hook, but HappyComposer.expandSelection.test.tsx:21 still mocks useComposerDraft as returning undefined. The current GitHub test check deterministically fails here with TypeError: Cannot read properties of undefined (reading 'complete'), so the PR cannot pass its required web suite.
    Suggested fix:

    vi.mock('@/hooks/useComposerDraft', () => ({
        useComposerDraft: () => ({
            sessionId: undefined,
            complete: true,
            restoredAny: false,
        }),
    }))

Summary

Review mode: follow-up after new commits

The latest commits address the prior upload-path, snapshot-retention, and suppressed-error findings, but the inactive hydration handoff still has a draft-loss path. Coverage is missing for restoration itself triggering a resume into a different session id.

Testing

  • Not run locally (review automation does not execute PR code).
  • GitHub test: failed; 1 failed / 2,123 passed.

HAPI Bot

Comment thread web/src/components/SessionChat.tsx Outdated
Comment thread web/src/components/AssistantChat/HappyComposer.tsx Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [MAJOR] Successful resume cache survives a later archive — resolvedSessionRef is cleared only on rejection. When resume returns the same ID, SessionPage stays mounted; after the session becomes active and is archived again, the cached branch skips the required new resume and the next attachment/send targets an inactive session. Evidence web/src/router.tsx:493.
    Suggested fix:
    useEffect(() => {
        resolvedSessionRef.current = null
    }, [session?.id, session?.active])

Summary
Review mode: follow-up after new commits

The prior two findings are no longer present. One resume-cache lifecycle regression remains.

Testing

  • Current GitHub test check passed, including typecheck and repository tests.
  • Not run locally: review automation did not execute PR code.
  • Missing coverage: resume to the same ID, transition active, archive again, then verify the next resolve calls resumeSession again.

HAPI Bot

Comment thread web/src/router.tsx

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] Gate inactive attachments on resumability — SessionChat now creates an attachment adapter for every inactive session because the route always supplies resolveSessionIdForUpload; however, that resolver rejects when inactiveSessionCanResume(...) is false (web/src/router.tsx:502). The changed drop-zone line is also enabled in that state, so unsupported/missing-history inactive sessions advertise attachment selection, begin processing the file, and then leave an error attachment that cannot be sent. Evidence web/src/components/SessionChat.tsx:1622 and web/src/components/SessionChat.tsx:1702.
    Suggested fix:
    const canAttach = props.session.active || inactiveCanResume
    // after the active-session branch
    if (!inactiveCanResume || !props.resolveSessionIdForUpload) {
        return undefined
    }
    
    <DragDropZone disabled={!canAttach || props.isSending || pendingSchedule != null || isScratchlistParking}>
    Include inactiveCanResume in the adapter memo dependencies.

Questions

  • None.

Summary

  • Review mode: follow-up after new commits
  • The new commit resolves the prior resume-cache lifecycle finding. One attachment-affordance regression remains for inactive sessions that cannot be resumed.

Testing

  • Not run locally (review automation; PR code was not executed).
  • GitHub test: in progress at review time.
  • Add coverage asserting a non-resumable inactive session has no attachment adapter and ignores file drops.

HAPI Bot

Comment thread web/src/components/SessionChat.tsx Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Reopen can render the removed source before draft handoff — reopenSession() starts invalidateSession() for the source detail in web/src/hooks/mutations/useSessionActions.ts:104, while this new awaited transfer keeps the browser on that source route. When reopen merges into a different ID and removes the source, the refetch can return 404 during the IndexedDB transfer; SessionPage then renders Session unavailable at web/src/router.tsx:711 before navigation runs. Evidence web/src/components/SessionChat.tsx:1675.
    Suggested fix:
    onSuccess: (result) => {
        void (async () => {
            if (result.sessionId === sessionId) {
                await invalidateSession()
            } else {
                await queryClient.invalidateQueries({ queryKey: queryKeys.sessions })
            }
            markSessionActiveInCache(result.sessionId)
        })()
    }

Questions

  • None.

Summary
Review mode: follow-up after new commits

The current head resolves the prior attachment-gating finding. The full diff still leaves a source-session 404 window in the header reopen path when the resumed session receives a different ID.

Testing

  • Not run (automation; PR code was not executed).
  • GitHub test: passed.
  • Missing coverage: delay draft attachment lookup, return a different reopen ID, make the source detail refetch return 404, and assert the old route never renders Session unavailable before target navigation.

HAPI Bot

Comment thread web/src/components/SessionChat.tsx Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Newly selected inactive-session attachment can be omitted from handoff — createAttachmentAdapter.add() invokes the resume handoff and returns, but the callback transfers only a previously published live snapshot or persisted storage. The current file is never passed into that transfer, and uploadDraftSnapshotRef is populated but never read. A fast resume, or the first item in an inactive multi-file drop, can navigate before the effect publishes that attachment, so the resumed composer restores without the selected file. Evidence web/src/lib/attachmentAdapter.ts:102.
    Suggested fix:
    onSessionResolved?: (
        sessionId: string,
        pending: AttachmentDraftInput,
    ) => Promise<void>
    
    // Merge the in-flight file with the selected live/persisted snapshot.
    await onSessionResolved(uploadSessionId, { id, file, previewUrl })
    await transferComposerDraft(sourceSessionId, uploadSessionId, pending)

Questions

  • None.

Summary
Review mode: follow-up after new commits

One Major draft-loss race remains in the inactive attachment handoff. The prior source-session invalidation finding is resolved at this head.

Testing

  • Not run (automation; PR content was not executed).
  • GitHub test: passed.
  • Missing coverage: resolve resume immediately while adding a new file, and drop multiple files on an inactive session; assert every file exists in the target-session draft after navigation.

HAPI Bot

Comment thread web/src/lib/attachmentAdapter.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Cancellation is lost while an inactive-session resume is pending — remove() can add the emitted attachment id to cancelledAttachmentIds while resolveSessionId() is awaiting the hub, but the different-id branch hands the file to the target without checking the set again. A file the user removed can therefore reappear in the resumed composer and be sent. Evidence web/src/lib/attachmentAdapter.ts:101.
    Suggested fix:
    const uploadSessionId = resolveSessionId ? await resolveSessionId() : sessionId
    if (cancelledAttachmentIds.has(id)) return
    if (uploadSessionId !== sessionId && onSessionResolved) {
        await onSessionResolved(uploadSessionId, { id, file, previewUrl })
        return
    }
  • [Major] A previously visited target can shadow the source draft — when reopening from the session list, the source composer is unmounted, so sourceLive is absent. If the resolved target is still in the 50-entry live snapshot map, this branch selects targetLive and never loads the source persisted text/attachments. The handoff then lands on the target with its old draft instead of the draft belonging to the reopened source. Evidence web/src/lib/composer-draft-transfer.ts:96.
    Suggested fix:
    if (sourceLive) {
        text = sourceLive.text
        baseAttachments = sourceLive.attachments
    } else {
        text = getDraft(sourceSessionId)
        baseAttachments = await loadPersistedAttachments(sourceSessionId)
    }
    
    // Late handoff files should append from the known target, not trigger source fallback.
    await transferComposerDraft(targetSessionId, targetSessionId, late)
  • [Major] The optimistic target seed prevents the intended REST reconciliation — setQueryData() marks the cloned source session fresh immediately before prefetchQuery(). With the repo default staleTime of 5 seconds and useSession using 30 seconds, the prefetch returns cached data without calling getSession; becoming stale later does not itself trigger a refetch. Target-specific metadata can remain the cloned source state until an SSE/reconnect/manual refresh happens. Evidence web/src/router.tsx:548.
    Suggested fix:
    void queryClient.fetchQuery({
        queryKey: queryKeys.session(resolvedSessionId),
        queryFn: () => api.getSession(resolvedSessionId),
        staleTime: 0,
    }).catch(() => {})

Questions

  • None.

Summary
Review mode: follow-up after new commits

Three Major correctness risks remain in cancellation, source-draft selection, and exact target-session reconciliation. The prior bot finding about including the in-flight attachment is addressed at this head.

Testing

  • Not run (automation; PR code was not executed).
  • GitHub test: passed.
  • Missing coverage: cancel while the resume promise is pending; transfer persisted source data when the target already has a live snapshot; assert getSession(target) runs after optimistic seeding.

HAPI Bot

Comment thread web/src/lib/attachmentAdapter.ts
Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Comment thread web/src/router.tsx Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Passive attachment hydration resumes an inactive session — canRestoreAttachments is now true whenever the inactive resolver adapter exists. On mount, useComposerDraft restores every stored file by calling api.composer().addAttachment(file); that adapter immediately calls resolveSessionId(), so merely revisiting an archived session with an attachment draft resumes/navigates it without a new pick or explicit Reopen. Evidence web/src/components/SessionChat.tsx:1814 (restore call context web/src/components/AssistantChat/HappyComposer.tsx:595).
    Suggested fix:
    <HappyComposer
        // Keep persisted files stored while inactive; explicit reopen/handoff
        // transfers them before the active target hydrates.
        canRestoreAttachments={props.session.active}
    />
  • [Major] A staggered multi-file handoff can overwrite files already transferred — the zero-delay batch only coalesces callbacks that reach handoffComposerDraft immediately. After the first handoff deletes activeHandoffs, a slower sibling (for example, an image still generating its preview) starts a new source-to-target transfer. The first transfer already deleted the source live snapshot, so the second reloads the old persisted source draft, which does not contain the first in-flight file, and replaces the target draft with only the later file. Evidence web/src/lib/composer-draft-transfer.ts:182.
    Suggested fix:
    const completedTarget = completedHandoffs.get(sourceSessionId)
    if (completedTarget === targetSessionId) {
        await transferComposerDraft(targetSessionId, targetSessionId, [pendingItem])
        return
    }
    
    await transferComposerDraft(sourceSessionId, targetSessionId, batch)
    completedHandoffs.set(sourceSessionId, targetSessionId)
    // Clear this entry when a fresh source-session snapshot/lifecycle is published.

Questions

  • None.

Summary
Review mode: follow-up after new commits

Two Major correctness issues remain: passive draft restoration can mutate session state, and staggered multi-file additions can lose an earlier attachment. The three findings from the prior HAPI Bot review are addressed at this head.

Testing

  • Not run (automation; PR code was not executed).
  • GitHub test: in progress when reviewed.
  • Missing coverage: mounting a resumable inactive composer with persisted attachments must not call resumeSession; a second handoff arriving after the first completes must preserve both files.

HAPI Bot

Comment thread web/src/components/SessionChat.tsx Outdated
Comment thread web/src/lib/composer-draft-transfer.ts

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Inactive hydration shadows the persisted attachment draft - canRestoreAttachments={props.session.active} intentionally skips IndexedDB attachment restoration on an inactive session, but the new effect still records the visible empty attachment list as the authoritative live snapshot. transferComposerDraft prefers that snapshot over IndexedDB, so switching away from an archived session, returning, and clicking Reopen transfers no attachments. Evidence web/src/components/AssistantChat/HappyComposer.tsx:606 (read precedence: web/src/lib/composer-draft-transfer.ts:99).
    Suggested fix:
    useEffect(() => {
        if (draftHydration.sessionId !== sessionId || !draftHydration.complete || !sessionId) return
        const canHydrateAttachments = props.canRestoreAttachments ?? active
        if (canHydrateAttachments) {
            setComposerDraftSnapshot(sessionId, composerText, attachmentDrafts)
        } else {
            saveDraft(sessionId, composerText)
            clearComposerDraftSnapshot(sessionId)
        }
    }, [active, attachmentDrafts, composerText, draftHydration.complete, draftHydration.sessionId, props.canRestoreAttachments, sessionId])

Questions

  • None.

Summary
Review mode: follow-up after new commits

One Major attachment-draft loss path remains. The prior passive-resume and staggered-file findings no longer reproduce in the updated lines, but the passive-resume fix now lets an empty inactive live snapshot override the persisted files.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub test: in progress when reviewed.
  • Missing coverage: persist an attachment, remount the source as inactive with attachment restoration disabled, then reopen to a new id and assert that the target draft still contains the file.

HAPI Bot

Comment thread web/src/components/AssistantChat/HappyComposer.tsx Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • None.

Questions

  • None.

Summary
Review mode: follow-up after new commits

No issues found above the 80% confidence threshold in the latest full diff. The prior Major finding is resolved. Residual risk: the inactive-composer remount and multi-file resume handoff are covered primarily at helper/adapter level; no committed component-level test exercises the complete archive → switch → reopen lifecycle.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • Static diff check: passed.
  • GitHub test: in progress when reviewed.

HAPI Bot

@techotaku39
techotaku39 force-pushed the fix/inactive-session-attachments branch from 8f9c1c1 to 76e7fa6 Compare August 6, 2026 13:53

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Blocker] Resolve the committed merge conflict — raw conflict markers make the TypeScript file unparsable, and choosing the HEAD side alone would also leave handoffComposerDraft undefined where it is called. Evidence web/src/components/SessionChat.tsx:79
    Suggested fix:
    import type { SendMessageAcceptance, SendMessageSettlement } from '@/hooks/mutations/useSendMessage'
    import { handoffComposerDraft, transferComposerDraft } from '@/lib/composer-draft-transfer'

Questions

  • None.

Summary
Review mode: follow-up after new commits

The latest commits introduce an unresolved merge conflict that blocks typechecking/building. No additional issues above the 80% confidence threshold were found in the latest full diff; functional validation remains blocked until the conflict is resolved.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • Static inspection: unresolved conflict markers found.

HAPI Bot

Comment thread web/src/components/SessionChat.tsx Outdated
@heavygee

heavygee commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Temporarily closing to retrigger Actions after conflict-marker fix (tip 629db29).

@heavygee heavygee closed this Aug 6, 2026
@heavygee heavygee reopened this Aug 6, 2026
heavygee added a commit to heavygee/hapi that referenced this pull request Aug 6, 2026
Add fix/inactive-session-drafts-1378 (tip 629db29) so :3006 can dogfood draft resume across inactive archive/switch/reopen.

Co-authored-by: Cursor <cursoragent@cursor.com>
heavygee added a commit to heavygee/hapi that referenced this pull request Aug 6, 2026
…n-drafts union

Thin PR tip conflicted with live soup share-retarget/cursor gates; remat absorbs the union tip instead.

Co-authored-by: Cursor <cursoragent@cursor.com>
heavygee added a commit to heavygee/hapi that referenced this pull request Aug 6, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
heavygee added a commit to heavygee/hapi that referenced this pull request Aug 6, 2026
Point remat at driver/settings-provider-onboard-heal (union of remat tip
+ feat tip) so tiann#1378 tip-forward can FF-absorb without conflicting the
upstream-clean feat/settings-provider-onboard PR branch.

Co-authored-by: Cursor <cursoragent@cursor.com>
heavygee added a commit to heavygee/hapi that referenced this pull request Aug 6, 2026
…by Actions outage

Co-authored-by: Cursor <cursoragent@cursor.com>
@heavygee

heavygee commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

CI retry after Actions outage — tip 9223f11 (conflict markers already fixed). Reopening to requeue workflows.

@heavygee heavygee closed this Aug 6, 2026
@heavygee heavygee reopened this Aug 6, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Preserve hidden stored attachments when an inactive add cannot hand off — the inactive composer deliberately does not restore the existing attachment draft, but the new inactive adapter can add one visible file. If the 50 MB check, resume, or upload fails, the existing unmount cleanup treats that partial visible list as authoritative and overwrites IndexedDB, deleting every previously stored attachment. Evidence web/src/components/SessionChat.tsx:1818; related cleanup web/src/hooks/useComposerDraft.ts:131.
    Suggested fix:
    // An inactive composer does not own the complete stored attachment list.
    if (canRestoreAttachments && (attachmentsRef.current.length > 0 || attachmentsReadyRef.current)) {
        saveDraftAttachments(sessionId, [...attachmentsRef.current])
    }

Questions

  • None.

Summary
Review mode: follow-up after new commits

The prior merge-conflict blocker is resolved. One Major data-loss path remains: a failed attachment selection on an inactive session can replace hidden persisted attachments with the partial visible list.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub test check was pending at review time.
  • Add a regression test: persist attachment A, mount inactive, select an oversized file or reject resume, unmount/switch sessions, then verify attachment A is still stored.

HAPI Bot

Comment thread web/src/components/SessionChat.tsx

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Preserve failed inactive selections through the next reopen — when automatic resume rejects, the adapter leaves the selected file visible as an incomplete attachment, but the inactive composer clears its live snapshot and no longer persists visible attachments. A later session switch or successful Reopen therefore transfers only the older IndexedDB draft and drops the newly selected file. Evidence web/src/components/AssistantChat/HappyComposer.tsx:646; related web/src/lib/attachmentAdapter.ts:161 and web/src/hooks/useComposerDraft.ts:133.
    Suggested fix:
    // In the inactive branch, merge hidden persisted inputs with visible pending inputs.
    const stored = await loadPersistedAttachments(sessionId)
    const merged = mergeAttachmentsById(stored, attachmentDrafts)
    saveDraftAttachments(sessionId, merged)
    setComposerDraftSnapshot(sessionId, composerText, merged)

Questions

  • None.

Summary
Review mode: follow-up after new commits

The prior hidden-draft overwrite is prevented, but the follow-up introduces the complementary loss path: a newly selected inactive file disappears after a transient resume failure followed by a switch or successful Reopen.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub test check passed at review time.
  • Add a regression test: persist attachment A, select valid attachment B while inactive, reject automatic resume, then successfully Reopen to a new session ID and verify both A and B restore.

HAPI Bot

Comment thread web/src/components/AssistantChat/HappyComposer.tsx Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Preserve stable IDs for pathless pending files — loadPersistedAttachments merges by ID, but a failed-resume pick has no upload path and getDraftAttachments does not restore its stored ID unless path exists. The next persistence pass therefore sees the stored file under a synthetic ID and the visible file under its original ID, duplicating it; repeated renders can compound the draft. Evidence web/src/lib/composer-draft-transfer.ts:101; related context web/src/lib/composer-attachment-drafts.ts:89.
    Suggested fix:
    // Restore identity for every stored file; only reuse an upload when path exists.
    restoredUploadMetadata.set(restored, { id: file.id, path: file.path, previewUrl: file.previewUrl, uploadSessionId: file.uploadSessionId })
    const restored = getRestoredUploadMetadata(file)
    if (!resolveSessionId && restored?.path) { /* reuse completed upload */ }
    const id = restored?.id ?? randomId()
  • [Major] Persist removal of a failed inactive selection — after a failed resume, the helper writes the visible pick into IndexedDB. Removing that attachment makes visibleAttachments empty, but this branch leaves IndexedDB untouched, so the removed file returns after switching sessions or reopening. Evidence web/src/lib/composer-draft-transfer.ts:95.
    Suggested fix:
    const previousVisibleIds = inactiveVisibleIds.get(sessionId) ?? new Set<string>()
    const stored = await loadPersistedAttachments(sessionId)
    const retained = stored.filter((item) => !previousVisibleIds.has(item.id))
    const merged = mergeAttachmentsById(retained, visibleAttachments)
    inactiveVisibleIds.set(sessionId, new Set(visibleAttachments.map((item) => item.id)))
    saveDraftAttachments(sessionId, merged)
  • [Major] Serialize inactive persistence and await it before transfer — each composer change starts an unawaited read/merge/write. An older read can finish after a newer one and replace the latest live snapshot, while an immediate Reopen can transfer before the selected file has been merged at all. Both paths can still drop the newest text or attachment. Evidence web/src/lib/composer-draft-transfer.ts:100; caller web/src/components/AssistantChat/HappyComposer.tsx:647.
    Suggested fix:
    const previous = inactivePersistQueue.get(sessionId) ?? Promise.resolve([])
    const next = previous.catch(() => []).then(() => persistInactiveComposerAttachmentsNow(sessionId, text, visibleAttachments))
    inactivePersistQueue.set(sessionId, next)
    // At the start of transferComposerDraft:
    await inactivePersistQueue.get(sourceSessionId)

Questions

  • None.

Summary
Review mode: follow-up after new commits

The current follow-up still has three draft-integrity paths: pathless picks can duplicate, removed picks can reappear, and overlapping persistence can transfer stale state.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub test check passed at review time.
  • Add regressions for repeated persistence of one pathless pick, removal after failed resume, and Reopen while a deferred persistence read is pending.

HAPI Bot

Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Comment thread web/src/lib/composer-draft-transfer.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Keep navigation after cancelling an in-flight inactive attachment — resolveSessionId() may already have resumed into a different session and the hub then merges away the source row, but the cancellation check returns before onSessionResolved, leaving the browser on the deleted source route and skipping draft transfer. Evidence web/src/lib/attachmentAdapter.ts:103; related context hub/src/sync/syncEngine.ts:2963.
    Suggested fix:
    const cancelled = cancelledAttachmentIds.has(id)
    if (uploadSessionId !== sessionId && onSessionResolved) {
        await onSessionResolved(
            uploadSessionId,
            cancelled ? undefined : { id, file, previewUrl },
        )
        return
    }
    if (cancelled) return
    Make the pending argument optional; when absent, transfer the remaining draft and navigate without restoring the removed file.

Questions

  • None.

Summary
Review mode: follow-up after new commits

The three prior draft-integrity findings are no longer present. One cancellation path can still strand the UI on a source session that the successful resume has deleted.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub test check passed at review time.
  • Add a regression where removal happens after resume starts, resume returns a different ID, the removed file stays removed, and navigation/draft transfer still target the resolved session.

HAPI Bot

Comment thread web/src/lib/attachmentAdapter.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Carry cancellation through the draft transfer — cancelled is sampled once before the async handoff. A removal during that handoff is invisible; when cancellation was already observed, passing undefined also loses the attachment ID and transferComposerDraft(source, target) can copy the same file from the live/persisted source snapshot. The removed file can therefore reappear after navigation. Evidence web/src/lib/attachmentAdapter.ts:108; related context web/src/components/SessionChat.tsx:1640, web/src/components/AssistantChat/HappyComposer.tsx:647.
    Suggested fix:
    type HandoffPending = AttachmentDraftInput & {
        isCancelled: () => boolean
    }
    
    await onSessionResolved(uploadSessionId, {
        id,
        file,
        previewUrl,
        isCancelled: () => cancelledAttachmentIds.has(id),
    })
    
    // Evaluate immediately before saveDraftAttachments.
    const cancelledIds = new Set(
        pendingAttachments.filter((item) => item.isCancelled()).map((item) => item.id),
    )
    const attachments = mergeAttachmentsById(baseAttachments, pendingAttachments)
        .filter((item) => !cancelledIds.has(item.id))
  • [Major] Stop rewriting attachment blobs on every inactive-composer render — attachmentDrafts is newly allocated each render and composerText is also a dependency, so every keystroke calls persistInactiveComposerAttachments. With a visible failed-resume file, that path reads/merges and queues another IndexedDB write of every blob; a 50 MB attachment can produce an unbounded transaction queue while typing. Evidence web/src/components/AssistantChat/HappyComposer.tsx:647; write context web/src/lib/composer-draft-transfer.ts:96 and web/src/lib/composer-draft-transfer.ts:113.
    Suggested fix:
    // Cheap text-only update; preserve the current in-memory attachment snapshot.
    updateComposerDraftTextSnapshot(sessionId, composerText)
    
    const attachmentRevision = attachmentDrafts
        .map(({ id, path, uploadSessionId }) => `${id}:${path ?? ""}:${uploadSessionId ?? ""}`)
        .join("\u0000")
    
    // Run blob persistence only when attachment membership/metadata changes.
    useEffect(() => {
        void persistInactiveComposerAttachments(sessionId, latestTextRef.current, attachmentDrafts)
    }, [sessionId, attachmentRevision])
  • [Minor] Preserve target upload metadata during same-session appends — transferComposerDraft(target, target, [pending]) is used for staggered files, but the unconditional base normalization clears path and uploadSessionId from attachments already uploaded by the target composer. A reload then uploads those files again and leaves the first uploads orphaned. Evidence web/src/lib/composer-draft-transfer.ts:177; same-session call sites web/src/lib/composer-draft-transfer.ts:218, web/src/lib/composer-draft-transfer.ts:230, web/src/lib/composer-draft-transfer.ts:257.
    Suggested fix:
    const normalizedBase = sourceSessionId === targetSessionId
        ? baseAttachments
        : baseAttachments.map(stripSessionScopedUploadFields)
    const attachments = mergeAttachmentsById(normalizedBase, normalizedPending)

Questions

  • None.

Summary
Review mode: follow-up after new commits

The prior cancellation/navigation finding was partially addressed, but cancellation is still not carried through the snapshot transfer. Inactive draft persistence also introduces per-keystroke blob writes, and staggered target appends can discard valid upload metadata.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub test check passed at review time.
  • Add regressions for: a cancelled ID already present in the source snapshot; cancellation while handoff awaits; typing with a visible inactive attachment without repeated blob writes; and a staggered append after the target snapshot contains an uploaded path.

HAPI Bot

Comment thread web/src/lib/attachmentAdapter.ts Outdated
Comment thread web/src/components/AssistantChat/HappyComposer.tsx Outdated
Comment thread web/src/lib/composer-draft-transfer.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Make the cross-session draft move durable and retire the source — transferComposerDraft only queues the target IndexedDB write, deletes the source live snapshot, and returns. The subsequent route change runs useComposerDraft cleanup, which saves the old text again and, when attachments are visible, re-persists the old blobs under the obsolete source ID. A fast reload can also abort the unawaited target write, leaving attachments stored only under a session ID that the resume merge removed. Evidence web/src/lib/composer-draft-transfer.ts:225; cleanup context web/src/hooks/useComposerDraft.ts:129; queued-write context web/src/lib/composer-attachment-drafts.ts:175.
    Suggested fix:
    // One awaited IndexedDB transaction: put target, delete source,
    // then update the target cache and source tombstone.
    await moveDraftAttachments(sourceSessionId, targetSessionId, attachments)
    completedHandoffs.set(sourceSessionId, targetSessionId)
    clearDraft(sourceSessionId)
    
    // The source composer is about to unmount; do not recreate a moved draft.
    if (composerDraftWasHandedOff(sessionId)) return

Questions

  • None.

Summary
Review mode: follow-up after new commits

The latest commit addresses the prior bot findings. One Major cross-session durability issue remains: the target attachment write is not awaited, and source-composer cleanup recreates the moved draft after navigation.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub test check passed at review time.
  • Add a regression that transfers a visible inactive attachment, unmounts the source composer, and verifies the target write completed while the source text/blob records remain cleared.

HAPI Bot

Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Re-read pendingTransfers.latest after awaits (TS2339), propagate real
IndexedDB write failures on corrective same-target moves, and restore only
missing stored attachment ids when same-id resume already has a visible pick.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] The final corrective write is not a stable handoff barrier — pendingLatest is sampled once, then an asynchronous IndexedDB write runs while the inactive composer remains editable. A text-only edit after an attachment edit updates only liveSnapshots, so pendingLatest.text wins with stale text; any attachment edit during the corrective write is also never rechecked before the source is cleared. This can discard draft changes made immediately before navigation. Evidence web/src/lib/composer-draft-transfer.ts:336, web/src/lib/composer-draft-transfer.ts:342, and web/src/lib/composer-draft-transfer.ts:350.
    Suggested fix:
    // Keep text edits on the same revision channel as attachment edits.
    const pending = pendingTransfers.get(sessionId)
    if (pending?.latest) {
        pending.latest = { ...pending.latest, text }
    }
    
    // Keep committing until no edit replaced the snapshot during the awaited write.
    for (;;) {
        const latest = pendingTransfers.get(sourceSessionId)?.latest
        if (!latest) break
        transferredText = latest.text
        attachments = buildTransferredAttachments()
        await moveDraftAttachments(targetSessionId, targetSessionId, () => attachments)
        if (pendingTransfers.get(sourceSessionId)?.latest === latest) break
    }
  • [Major] The resume cache can be cleared while old attachment generators are still running — each file may await preview generation before asking for the resolved session. The first file can resume and navigate, changing session.id/active and clearing resolvedSessionRef; a slower file from the old adapter then retries resume against the retired source id instead of joining the original promise. That file can end as an error or disappear from the resumed composer. Evidence web/src/lib/attachmentAdapter.ts:72, web/src/lib/attachmentAdapter.ts:106, and web/src/router.tsx:494.
    Suggested fix:
    let uploadResolution: Promise<string> | undefined
    const resolveUploadSession = () => {
        uploadResolution ??= props.resolveSessionIdForUpload!(props.session.id).catch((error) => {
            uploadResolution = undefined
            throw error
        })
        return uploadResolution
    }
    
    return createAttachmentAdapter(props.api, props.session.id, resolveUploadSession, onResolved)

Summary
Review mode: follow-up after new commits

Two Major draft-preservation races remain on the latest head. The previous strict-typecheck, corrective-write rejection, and same-id attachment restoration findings are addressed; the current GitHub test check passed.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test: passed on 68caeafdb4bd88ba93e27200ca935f87cd6e851f.
  • Add regressions for a text/attachment edit during the same-target corrective write, and for two inactive-session files where the second resolver starts after the first file navigates.

HAPI Bot

Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Comment thread web/src/router.tsx
Repeat corrective target writes until pending.latest is unchanged across
the await, sync keystrokes into the transfer barrier, and keep one resume
promise per inactive adapter instance so staggered files join the same
handoff after navigation.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Text-only edits can erase hidden persisted attachments during transfer - inactive composers intentionally keep stored files out of liveSnapshots. Once a cross-session handoff installs pendingTransfers, a keystroke records attachments: []; buildTransferredAttachments then prefers that empty list over the IndexedDB-backed baseAttachments, so reopen can silently delete every previously persisted attachment draft. Evidence web/src/lib/composer-draft-transfer.ts:74, web/src/lib/composer-draft-transfer.ts:76, and web/src/lib/composer-draft-transfer.ts:306.
    Suggested fix:
    type PendingTransfer = {
        targetSessionId: string
        latest?: { text: string; attachments: AttachmentDraftInput[] }
        latestText?: string
    }
    
    const pending = pendingTransfers.get(sessionId)
    if (pending?.latest) {
        pending.latest = { ...pending.latest, text }
    } else if (pending) {
        pending.latestText = text
    }
    
    // Use latestText as a text fallback, without replacing attachment state.
    const pendingState = pendingTransfers.get(sourceSessionId)
    transferredText = pendingState?.latest?.text
        ?? pendingState?.latestText
        ?? liveSnapshots.get(sourceSessionId)?.text
        ?? getDraft(sourceSessionId)

Summary

Review mode: follow-up after new commits

One Major data-loss race remains on the latest head. The two findings from the prior HAPI Bot pass are addressed by the new commits.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test: succeeded on 8d0ba194a89444c8a5640a71981307baeebe5ab8.
  • Add a regression with persisted attachments but no live snapshot: block the cross-session move, call updateComposerDraftTextSnapshot, then verify the target retains both the files and latest text.

HAPI Bot

Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Track inactive keystrokes on pending.latestText instead of inventing
pending.latest with an empty attachment list that would overwrite
hidden stored files during a cross-session move.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Do not treat an IndexedDB read failure as an empty draft before moving — the new transfer path calls loadPersistedAttachments, but its underlying getDraftAttachments catches every open/read error and returns []. If that transient read fails and the following move transaction succeeds, moveDraftAttachments deletes the source record and writes/deletes an empty target, silently losing all persisted attachments. Evidence web/src/lib/composer-draft-transfer.ts:314; related handling web/src/lib/composer-attachment-drafts.ts:182 and web/src/lib/composer-attachment-drafts.ts:269.
    Suggested fix:

export async function getDraftAttachments(
sessionId: string,
options: { throwOnError?: boolean } = {},
): Promise<File[]> {
try {
// existing cache / IndexedDB read
} catch (error) {
if (options.throwOnError) throw error
return []
}
}

const files = await getDraftAttachments(sessionId, { throwOnError: true })


**Summary**

Review mode: follow-up after new commits

One Major data-loss path remains on the current head.

**Testing**

- Not run (automation; PR code was not executed under the security constraints).
- GitHub Actions `test`: pending at review time.
- Add a regression where the source IndexedDB read rejects but a later write would succeed; transfer must reject/navigate without deleting or overwriting the source attachment draft.

*HAPI Bot*

Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Cross-session transfer now requests throwOnError for IndexedDB reads so a
transient empty result cannot delete the source draft during the durable move.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Make inactive read/merge persistence fail closed — both merge branches call the best-effort attachment reader. If that read fails while a newly selected inactive attachment is being persisted, the code publishes a live snapshot containing only the visible pick; transferComposerDraft then trusts that snapshot, skips its new strict read, and moveDraftAttachments deletes the source row. Existing hidden attachments are still permanently lost. Evidence web/src/lib/composer-draft-transfer.ts:176, web/src/lib/composer-draft-transfer.ts:193, and web/src/lib/composer-draft-transfer.ts:312.
    Suggested fix:

const stored = await loadPersistedAttachments(sessionId, { throwOnError: true })
// If this throws, do not publish liveSnapshots or write a partial merged draft.

- [Major] Preserve text when the strict attachment read aborts handoff — the new strict read occurs before the move's recovery block. On failure, `transferComposerDraftThenNavigate` still navigates to the new session, but no text is saved under that target id; the composer therefore appears empty even though the source text remains under an obsolete session id. Evidence `web/src/lib/composer-draft-transfer.ts:319` and `web/src/lib/composer-draft-transfer.ts:430`.
Suggested fix:
```ts
text = getDraft(sourceSessionId)
try {
  baseAttachments = await loadPersistedAttachments(sourceSessionId, { throwOnError: true })
} catch (error) {
  saveDraft(targetSessionId, text)
  throw error
}

Summary

Review mode: follow-up after new commits

The direct destructive read now fails closed, but the inactive merge race can still erase stored attachments, and strict-read failure drops the text draft from the resumed route.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test: in progress at review time.
  • Add regressions for a failing best-effort read racing an inactive attachment handoff, and for text visibility on the target after a strict source read failure.

HAPI Bot

Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Strict-read every inactive attachment RMW merge so a transient IDB error
cannot publish a partial live snapshot, and copy composer text to the
resumed id before aborting when attachment transfer reads fail.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Reapply the optimistic active state after the background detail fetch — fetchQuery(..., staleTime: 0) writes its response into the same cache entry immediately after it was seeded as active. The resume path already acknowledges a REST/SSE race; if this fetch returns the stale inactive snapshot, the target route flips back to inactive and can replace the active attachment adapter until another update arrives. Evidence web/src/router.tsx:551.
    Suggested fix:
    void queryClient.fetchQuery({
        queryKey: queryKeys.session(resolvedSessionId),
        queryFn: () => api.getSession(resolvedSessionId),
        staleTime: 0,
    }).catch(() => undefined).finally(() => {
        queryClient.setQueryData(queryKeys.session(resolvedSessionId), (previous) => previous ? {
            ...previous,
            session: { ...previous.session, id: resolvedSessionId, active: true },
        } : previous)
    })
  • [Minor] Handle strict inactive-persist failures at the fire-and-forget call sites — the follow-up correctly makes persistInactiveComposerAttachments reject when IndexedDB cannot be read, but both new callers discard that rejecting promise. A storage/open failure therefore becomes an unhandled rejection during the effect or unmount cleanup. Evidence web/src/components/AssistantChat/HappyComposer.tsx:669 and web/src/hooks/useComposerDraft.ts:152.
    Suggested fix:
    void persistInactiveComposerAttachments(sessionId, text, attachments).catch((error) => {
        console.warn('[composer-draft] inactive persistence failed', error)
    })

Summary

Review mode: follow-up after new commits

The two prior Major findings are addressed. The forced post-navigation refetch can still undo the optimistic active state, and the newly strict persistence path needs rejection handling at its background call sites.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test and pr-review: pending at review time.
  • Add a deferred getSession regression returning active: false after optimistic seeding, plus coverage that IndexedDB read failures do not emit unhandled rejections from effect/unmount persistence.

HAPI Bot

Comment thread web/src/router.tsx Outdated
Comment thread web/src/components/AssistantChat/HappyComposer.tsx
Reapply optimistic active after background getSession settles so a lagging
inactive REST snapshot cannot flip the target composer back, and catch
strict inactive persist rejections at fire-and-forget call sites.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Re-check cancellation after the durable move - the initiating handoff samples isCancelled before moveDraftAttachments opens its IndexedDB transaction. If the operator removes that file while the transaction is pending, the committed target draft still contains it; navigation then restores it through the active adapter and uploads a file the operator already removed. The existing cancellation tests stop before resolveAttachments() is sampled, so they do not cover this window. Evidence web/src/lib/composer-draft-transfer.ts:516; transaction gap web/src/lib/composer-attachment-drafts.ts:230.
    Suggested fix:
    await transferComposerDraft(sourceSessionId, targetSessionId, batch)
    
    const cancelledAfterMove = batch.filter((item) => item.isCancelled?.())
    if (cancelledAfterMove.length > 0) {
        await transferComposerDraft(
            targetSessionId,
            targetSessionId,
            cancelledAfterMove,
        )
    }

Summary

Review mode: follow-up after new commits

The prior Major and Minor findings are addressed. One Major cancellation race remains: a file removed during the IndexedDB move can be restored and automatically uploaded on the resumed session.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test and pr-review: in progress at review time.
  • Add a regression that lets moveDraftAttachments sample the pending file, blocks transaction completion, cancels the file, then verifies the target draft excludes it before navigation.

HAPI Bot

Comment thread web/src/lib/composer-draft-transfer.ts
Always rewrite the target after the cross-session move so isCancelled
flips during the IndexedDB commit window still drop the file, and prune
again in handoff before navigation when the batch reports cancellation.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Stabilize cancellation across the corrective write — cancelledAfterMove snapshots only the files already cancelled before the same-target correction starts. If another file in the same multi-file batch is removed while that correction is awaiting IndexedDB, it is absent from the subset passed to transferComposerDraft; the target draft retains it and navigation restores/uploads a file the operator removed. Evidence web/src/lib/composer-draft-transfer.ts:518.
    Suggested fix:
    const cancellationRevision = () => JSON.stringify(
        batch.filter((item) => item.isCancelled?.()).map((item) => item.id).sort(),
    )
    let observedCancellationRevision = '[]'
    for (;;) {
        const nextCancellationRevision = cancellationRevision()
        if (nextCancellationRevision === observedCancellationRevision) break
        observedCancellationRevision = nextCancellationRevision
        await transferComposerDraft(targetSessionId, targetSessionId, batch)
    }

Summary

Review mode: follow-up after new commits

The prior single-file durable-move race is addressed, but the new corrective write opens one more cancellation window for multi-file batches. One Major finding remains.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test and pr-review: pending at review time.
  • Add a regression that cancels file A during the initial move, blocks the resulting same-target correction, cancels file B during that correction, then verifies both ids are absent before navigation.

HAPI Bot

Comment thread web/src/lib/composer-draft-transfer.ts Outdated
Treat the pending cancellation set as part of post-move stability so a
sibling remove mid-corrective rewrite is re-sampled before navigation,
and loop handoff same-target rewrites until that set stops changing.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Move the draft before text-only auto-resume navigation — the updated send-resolution callback navigates to the resumed id without using the new draft-transfer path. An inactive source can have persisted attachments that are intentionally hidden; after a text-only send resumes to a new id, the target mounts without those files. If the POST fails, only the text recovery follows the target and the attachments remain orphaned under the retired source id. Evidence web/src/router.tsx:607.
    Suggested fix:
    // useSendMessage.ts
    onSessionResolved?: (sessionId: string) => void | Promise<void>
    await options.onSessionResolved?.(resolved)
    
    // router.tsx
    await transferComposerDraftThenNavigate(
        sessionId,
        resolvedSessionId,
        () => handleSessionResolved(resolvedSessionId),
    )

Summary

Review mode: follow-up after new commits

The previous multi-file cancellation finding is addressed at the current head. One Major finding remains in the text-only auto-resume entry point.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test: passed.
  • Add a regression with a persisted attachment on an inactive source, a text-only resume to a new id, and a failed send; verify the target composer restores the attachment.

HAPI Bot

Comment thread web/src/router.tsx Outdated
Await onSessionResolved so composer draft handoff completes before the
send mutation, and route text-only auto-resume through
transferComposerDraftThenNavigate like upload/reopen paths.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Transfer drafts from the Files-page reopen callback — SessionHeader now awaits the callback, but the Files route still navigates directly. Leaving chat for Files unmounts and persists the composer under the source id; when reopen returns a new id, the hub merges/deletes that source, so the target chat cannot restore its text or attachments. Evidence web/src/components/SessionHeader.tsx:245, related caller web/src/routes/sessions/files.tsx:476.
    Suggested fix:
    onSessionReopened={async (newSessionId) => {
        await transferComposerDraftThenNavigate(
            session.id,
            newSessionId,
            () => navigate({
                to: "/sessions/$sessionId/files",
                params: { sessionId: newSessionId },
                replace: true,
            }),
        )
    }}
  • [Major] Preserve a newer inactive SSE transition during the background detail refresh — fetchQuery writes its response into the cache and the unconditional finally then forces active: true. If the resumed target ends while this request is pending, useSSE can correctly patch it inactive first; the later fetch/finally overwrites that newer state, with no subsequent event required to repair it. Evidence web/src/router.tsx:552.
    Suggested fix:
    void api.getSession(resolvedSessionId).then((response) => {
        queryClient.setQueryData(queryKeys.session(resolvedSessionId), (current) => {
            if (current?.session.active !== response.session.active) return current
            return response
        })
    }).catch(() => undefined)

Summary

Review mode: follow-up after new commits

The prior HAPI Bot Major finding is addressed at the current head. Two Major issues remain: the Files-page reopen path still orphans drafts on id changes, and the optimistic detail refresh can resurrect a target that has already become inactive.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test: passed.
  • Add a Files-page reopen regression with a persisted text/attachment draft and a changed session id.
  • Add a query-race regression where an inactive SSE cache patch lands while the post-resume detail request is pending; the inactive state must win.

HAPI Bot

Comment thread web/src/components/SessionHeader.tsx
Comment thread web/src/router.tsx Outdated
Wire the Files-page reopen path through transferComposerDraftThenNavigate,
and refresh resumed session detail only when the cached active flag still
matches so a mid-flight inactive SSE transition is not resurrected.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Do not send before hidden inactive attachments are available — inactive remounts restore text but intentionally keep attachment blobs only in IndexedDB. The added handoff waits for transfer/navigation, then the mutation still uses the old composer's empty attachment list; successful cleanup can erase the transferred draft. Evidence web/src/hooks/mutations/useSendMessage.ts:288, related context web/src/components/SessionChat.tsx:1827.
    Suggested fix:
    type SessionResolution = {
        attachments?: AttachmentMetadata[]
        deferUntilDraftHydrated?: boolean
    }
    
    const resolution = await options.onSessionResolved?.(resolved, { text, attachments })
    targetSessionId = resolved
    if (resolution?.deferUntilDraftHydrated) return false
    attachments = resolution?.attachments ?? attachments

Summary

Review mode: follow-up after new commits

The two prior Major findings are addressed at the current head. One Major data-loss regression remains in the inactive-session Send path: transferred hidden attachments are not included in the resumed POST and can be cleared after the text-only send succeeds.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test: passed.
  • Missing regression: archive with text + attachment, switch away/back, press Send, resume to a new id, then verify the POST includes attachment metadata or is deferred until the target composer restores it.

HAPI Bot

Comment thread web/src/hooks/mutations/useSendMessage.ts Outdated
After text-only resume transfers stored attachments, skip the mutation
when the target draft still has files the inactive composer never
exposed, so clearDraftsAfterSend cannot wipe them after a text-only POST.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Re-upload visible attachments after a cross-session resume — the source composer can remain mounted through archive, so its visible attachment metadata still contains paths authorized for the old session. The new branch skips deferral whenever that metadata is present, then sends it to the resolved session even though the transfer code strips those same session-scoped paths. Evidence web/src/router.tsx:612, related context web/src/lib/attachmentAdapter.ts:45 and web/src/lib/composer-draft-transfer.ts:350.
    Suggested fix:

    const stored = await getDraftAttachments(resolvedSessionId)
    if ((context.attachments?.length ?? 0) > 0 || stored.length > 0) {
        return { deferUntilDraftHydrated: true }
    }
  • [Major] Keep hidden attachment drafts in the schedule exclusion — inactive restoration hides stored files from the adapter, so the composer reports no attachments and enables scheduling. When Send later discovers a stored file, navigation occurs and the mutation is deferred; the session-keyed remount drops pendingSchedule, leaving the target draft ready for an unintended immediate send. Evidence web/src/components/SessionChat.tsx:1827, related context web/src/components/AssistantChat/HappyComposer.tsx:503, web/src/components/AssistantChat/ComposerButtons.tsx:862, and web/src/router.tsx:603.
    Suggested fix:

    const hasAnyAttachments =
        attachments.length > 0 || draftHydration.hasStoredAttachments
    
    <ComposerButtons hasAttachments={hasAnyAttachments} />

Summary

Review mode: follow-up after new commits

The previous hidden-draft send loss is only partially addressed. Two Major correctness paths remain: visible source-session upload metadata can be posted to a different resumed session, and hidden attachment drafts bypass the attachment/schedule mutual exclusion and lose the selected schedule during deferred navigation.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test: passed.
  • Missing regressions: archive without unmounting a visible uploaded attachment, then Send into a new session id; and inactive hidden attachment + scheduled Send, verifying the schedule cannot be silently converted to immediate delivery.

HAPI Bot

Comment thread web/src/router.tsx Outdated
Comment thread web/src/components/SessionChat.tsx
Always defer cross-session send when visible or stored attachments exist
so source-scoped upload paths are not POSTed, and surface IndexedDB
attachment presence to disable scheduling on inactive composers.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Run draft resolution after same-ID resumes — inactive composers deliberately keep persisted files out of the visible adapter. HAPI can resume PTY/Pi/Cursor sessions in place, but the new resolution callback only runs when the returned ID changes, so a text + hidden-file draft is posted as text-only after a same-ID resume. Evidence web/src/hooks/mutations/useSendMessage.ts:303, related context web/src/hooks/useComposerDraft.ts:93 and hub/src/sync/syncEngineReopenPreservesPtyId.test.ts:122.
    Suggested fix:

    type ResolvedSession = { sessionId: string; resumed: boolean }
    
    const result = await options.resolveSessionId(sessionId)
    targetSessionId = result.sessionId
    if (result.resumed) {
        const resolution = await options.onSessionResolved?.(
            targetSessionId,
            { text, attachments },
        )
        if (resolution?.deferUntilDraftHydrated) return false
        sendAttachments = resolution?.attachments ?? attachments
    }
  • [Major] Give hidden attachment-only drafts an explicit resume path — hasStoredAttachments enables the Send button even when the runtime contains neither text nor visible attachments. The only current path is api.composer().send(), and useHappyRuntime discards an empty append before resolveSessionId is called, so clicking Send does nothing. Evidence web/src/components/AssistantChat/HappyComposer.tsx:638, related context web/src/lib/assistant-runtime.ts:754.
    Suggested fix:

    if (!hasText && !hasAttachments && draftHydration.hasStoredAttachments) {
        await props.onResumeStoredDraft?.()
        return
    }
    
    <HappyComposer
        onResumeStoredDraft={() => handleSend('', undefined, null)}
    />
  • [Major] Block scheduling while inactive attachment hydration is pending — the IndexedDB presence check is asynchronous. Until it resolves, hasStoredAttachments is false, so the schedule picker can be opened; disabling the button later does not close the open picker. A text + hidden-file draft can therefore still acquire a schedule, then lose it when the deferred resume remounts the target composer, converting the next click to immediate delivery. Evidence web/src/hooks/useComposerDraft.ts:96, related context web/src/components/AssistantChat/HappyComposer.tsx:638 and web/src/components/AssistantChat/ComposerButtons.tsx:862.
    Suggested fix:

    const canHydrateAttachments = props.canRestoreAttachments ?? active
    const hiddenAttachmentStatePending =
        !canHydrateAttachments
        && (draftHydration.sessionId !== sessionId || !draftHydration.complete)
    const hasHiddenAttachments =
        !canHydrateAttachments && draftHydration.hasStoredAttachments
    const hasAnyAttachments = hasAttachments || hasHiddenAttachments
    const blocksScheduling =
        hasAttachments || hasHiddenAttachments || hiddenAttachmentStatePending
    <ComposerButtons hasAttachments={blocksScheduling} />

Summary

Review mode: follow-up after new commits

Three Major correctness paths remain. The latest commits address cross-session upload metadata and the steady-state hidden-attachment schedule exclusion, but same-ID resumes can still omit files, attachment-only inactive drafts cannot trigger resume, and scheduling remains possible during the asynchronous hidden-draft check.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test: passed.
  • Missing regressions: same-ID resume with text + hidden file; hidden attachment-only Send; and opening the schedule picker before inactive attachment hydration completes.

HAPI Bot

Comment thread web/src/hooks/mutations/useSendMessage.ts Outdated
Comment thread web/src/components/AssistantChat/HappyComposer.tsx Outdated
Comment thread web/src/hooks/useComposerDraft.ts
Return resumed independently of session id so same-row PTY/Pi resumes
still defer for hidden files, route attachment-only Send through an
explicit resume callback, and treat incomplete inactive hydration as
schedule-blocking.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Preserve the captured send text during deferred draft handoff — the resume callback transfers from mutable persisted state but ignores context.text. assistant-ui invokes the async send callback without awaiting it and clears the composer immediately; while resumeSession is in flight, the inactive draft effect can persist the cleared text. For a text + hidden-file draft, the transfer then moves the file with empty text, returns deferUntilDraftHydrated, and never starts a mutation/error-restore path, so the target composer reopens with the text lost. Evidence web/src/router.tsx:603, related context web/src/lib/assistant-runtime.ts:766, web/src/components/AssistantChat/HappyComposer.tsx:675, and web/src/lib/composer-draft-transfer.ts:318.
    Suggested fix:
    type TransferOptions = { textOverride?: string }
    
    await transferComposerDraftThenNavigate(
        sessionId,
        resolvedSessionId,
        () => handleSessionResolved(resolvedSessionId),
        [],
        { textOverride: context.text },
    )
    
    // In transferComposerDraft, prefer the immutable submitted snapshot:
    transferredText = options.textOverride ?? transferredText

Summary

Review mode: follow-up after new commits

One Major draft-loss path remains. The previous same-ID, attachment-only, and scheduling findings are addressed by the latest commits, but deferred hidden-attachment sends can still discard their accompanying text before navigation.

Testing

  • Not run (automation; PR code was not executed under the security constraints).
  • GitHub Actions test: passed.
  • Missing regression: mount an inactive composer with persisted text + a hidden attachment, delay resumeSession, trigger Send so assistant-ui clears the source composer, then assert the resolved composer restores both text and file before the send is retried.

HAPI Bot

Comment thread web/src/router.tsx
Pass the immutable submitted text into draft transfer so assistant-ui clearing the composer while resume is in flight cannot drop text from deferred hidden-attachment sends.

Co-authored-by: Cursor <cursoragent@cursor.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • None.

Questions

  • None.

Summary

Review mode: follow-up after new commits

No Blocker, Major, Minor, or Nit findings at the current head. The prior HAPI Bot finding about losing the captured send text during deferred draft handoff is addressed by the immutable textOverride path and its same-ID/cross-ID regression coverage. Residual risk remains in timing-sensitive browser integration across assistant-ui, IndexedDB, and in-place PTY/Pi/Cursor resumes; no full browser-level same-ID resume test was found in the changed tests.

Testing

  • Not run locally (review automation; PR code was not executed under the security constraints).
  • GitHub Actions test: passed at the reviewed head.
  • Static validation: complete PR diff inspected; git diff --check passed.

HAPI Bot

@tiann
tiann merged commit bdf8da1 into tiann:main Aug 8, 2026
3 checks passed
heavygee added a commit to heavygee/hapi that referenced this pull request Aug 8, 2026
Merged upstream as bdf8da1; leave DROPPED marker for remat wave.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants