store composer drafts in session storage until sent

[maxwellpeterson]

This PR stores prompt drafts in session storage until they are sent to the agent. This means page refreshes don't wipe composer state, which helps mitigate workspace and conversation creation failures. Prompts typed into the new workspace, new conversation, and existing conversation composers are all covered. Capsules are serialized to their corresponding resource URL for storage, and format specifiers (docs, slides, etc.) are fully preserved.

[ask-bonk]

Review: store composer drafts in local storage until sent

I read all 8 changed files, ran the affected tests (composerDraft.test.ts, homePromptFlow.test.tsx, messageFormatRefs.test.ts — 14 pass), tsc --noEmit for workshop-frontend (clean), and pnpm lint:check (no new errors). The design is careful and the edge cases I probed mostly hold up. A few things worth addressing:

1. Title/body say "local storage" but the code uses sessionStorage

composerDraft.ts reads/writes window.sessionStorage exclusively. sessionStorage is the better choice here (per-tab, auto-cleared on close, which limits how long capability-bearing capsule URLs linger — see #2), but the PR title and description both say "local storage", and the JSDoc on draftStorageKey correctly says "Session-storage key". Please reconcile the title/body with the implementation so future readers aren't misled.

2. Capsule resource URLs are persisted to browser storage

serializeComposerDraft downgrades each capsule to description.url and stores it (composerDraft.ts:52). Capsule URLs are capability-bearing resource references, so this writes capabilities into sessionStorage. The exposure is same-origin, per-tab, and short-lived, and the URL already lives in the composer DOM, so I don't consider it blocking — but it's a deliberate widening of where capability URLs are at rest and deserves an explicit mention in the PR description (and ideally a one-line comment near the serialization noting the tradeoff).

Note also this is a one-way transform: a restored capsule comes back as plain URL text, not a re-hydrated capsule chip (it'll be re-detected as a link). The body says "Capsules are serialized to their corresponding resource URL for storage", which covers it, but the loss of the chip on restore is worth stating plainly.

3. Refresh recovery depends on the key-change effect, not the mount path

Because currentUser is null until whoami() resolves (AuthContext.tsx:23-32), draftStorageKey is undefined on first render, so initialDraft is always undefined on a cold load and the mount effect (ChatInterface.tsx:1978) is a no-op. Actual restore happens later when the key flips undefined -> defined and the second effect (:1991) reads storage and calls restoreDraftPresentation. This works, and the preserveLocalDraft branch (:1998) correctly protects text the user typed before auth resolved. It's just subtle enough that a comment on the mount effect explaining "the key is usually undefined on first render; the key-change effect below performs the real restore once currentUser resolves" would save the next reader some time.

Things I verified as correct

• Effect ordering: the key-change effect (:1991) runs before the write effect (:2023), so skipDraftWriteRef is set before it's consumed; the subsequent idempotent re-write on the state updates is harmless.
• Send flow: draft is cleared only on the success path after await onSend (:2598), and the loadedDraftKeyRef.current !== sendingDraftKey guard (:2599) correctly avoids wiping a composer that navigated to a different destination mid-send. Failure preserves the draft — the stated goal.
• readComposerDraft validation is strict (monotonic non-overlapping positions, length === noun.length, and text.slice(...) === noun), so a tampered/stale entry is rejected rather than mis-rendered.
• serializeComposerDraft strips the logo slot and records positions against normalized text; decorateComposerDraft re-inserts slots and shifts later tokens. Round-trip is covered by tests.
• The messageFormatRefs refactor (offer -> Pick<MessageFormatRef,'noun'|'icon'>) and FormatToken flattening (format.output.noun -> noun) are consistent across call sites and the test.

Overall a solid, well-tested change. My only requested change is #1 (title/body vs. sessionStorage); #2 and #3 are documentation/comment asks.

[ask-bonk]

Review posted to PR #125.

Summary

I reviewed all 8 changed files, ran the affected tests (14 pass), tsc --noEmit (clean), and pnpm lint:check (no new errors). The change is well-structured and carefully handles the tricky effect-ordering and send-flow edge cases.

Findings I raised:

1. Title/body vs. implementation mismatch (requested change) — Title and body say "local storage" but the code uses sessionStorage throughout. sessionStorage is actually the better choice, but the wording should be reconciled.

2. Capsule capability URLs persisted to storage (doc ask) — serializeComposerDraft writes capsule resource URLs (capability-bearing) into sessionStorage. Low risk (same-origin, per-tab, short-lived, already in DOM) but worth an explicit note. Also, restored capsules come back as plain URL text, not chips.

3. Refresh recovery relies on the key-change effect, not mount (comment ask) — Because currentUser is null until whoami() resolves, draftStorageKey is undefined on first render, so the mount effect is a no-op and the real restore happens via the key-change effect. A clarifying comment would help.

Verified correct: effect ordering vs. skipDraftWriteRef, the send-success-only clear with the mid-send navigation guard, strict readComposerDraft validation, the logo-slot strip/restore round-trip, and the messageFormatRefs/FormatToken refactor consistency.

The only requested change is #1; #2 and #3 are documentation improvements.

github run

[ndisidore]

Are you sure you don't want localStorage? Session storage goes away when the tab is closed; we did localstorage in CFOSv1 so that you could recover from that.
Possibly overkill: I like the session storage api much more

[ask-bonk]

Re @ndisidore's question about localStorage vs sessionStorage:

I want to keep sessionStorage here, for two reasons:

1. Scope matches the goal. The stated aim is to keep composer state alive across a page refresh that would otherwise wipe an in-flight workspace/conversation creation. That failure mode is within a single tab session, which sessionStorage covers exactly. Surviving a tab close (the CFOSv1 localStorage behavior) is a broader "resume where I left off later" feature — nice, but not what this PR is trying to fix, and it comes with the tradeoffs below.

2. Capsule URLs are capability-bearing. serializeComposerDraft downgrades each capsule to its description.url, and those resource URLs carry capability. sessionStorage keeps them same-origin, per-tab, and auto-cleared on tab close, which bounds how long those URLs sit at rest. localStorage would persist them indefinitely across all tabs until explicitly cleared — a meaningfully wider exposure window for capability references, for a recovery scenario the PR isn't targeting.

So this is deliberate rather than an oversight. If we later decide cross-session draft recovery is worth it, I'd rather do it as a follow-up that also thinks through the capability-URL lifetime (e.g. TTL/expiry, or not persisting capsule URLs at rest at all) rather than widen the storage now.

On the title/body wording (@ask-bonk's requested change #1): both the PR title and body now say "session storage" to match the implementation.

The comment asks from @ask-bonk (#2 the capsule-URL tradeoff, #3 the mount-effect vs key-change-effect restore path) are addressed in the "address bonk comments" commit — see the note at composerDraft.ts:52 and the explanatory comment on the mount effect at ChatInterface.tsx:1978.

[ask-bonk]

Posted a response to PR #125 addressing the outstanding review feedback.

Summary

The PR is code-complete and well-tested; the last commit ("address bonk comments") already handled ask-bonk's code-level asks. The two remaining open items were both design/wording questions rather than code changes, so I responded on the PR rather than editing code:

1. ndisidore's question — localStorage vs sessionStorage: Recommended keeping sessionStorage, with two reasons:

   • The PR's goal is refresh recovery within a tab session, which sessionStorage covers exactly; tab-close survival (the CFOSv1 behavior) is a broader feature this PR isn't targeting.
   • Capsule URLs are capability-bearing. sessionStorage bounds their at-rest lifetime (per-tab, auto-cleared on close); localStorage would persist those capability references indefinitely across all tabs. Any future cross-session recovery should be a follow-up that also addresses capability-URL lifetime.

2. ask-bonk's requested change Bump vitest from 3.2.4 to 4.1.0 #1 (title/body says "local storage"): Confirmed the PR title and body now both say "session storage", matching the implementation.

I also noted that ask-bonk's comment asks #2 (capsule-URL tradeoff note at composerDraft.ts:52) and #3 (mount-effect explanation at ChatInterface.tsx:1978) are already resolved in the latest commit.

No code changes were required — the implementation already reflects the recommended decision.

github run