feat(web): preserve new-chat text and context across navigation and reloads by Max-Samson · Pull Request #1034 · rome-os/rome

Max-Samson · 2026-10-14T15:33:58Z

What this PR does

Leaving New chat discards an unfinished message. This PR keeps that
draft available when the guardian returns.

Preserve exact text, selected context, and pending attachments.
Restore the draft through New chat and its keyboard shortcut.
Place the caret at the end on re-entry without interrupting normal editing.
Keep explicit deletions empty.
Clear the draft on logout or identity changes.

Design & Invariants

Keep one in-memory draft above the route tree.
Keep existing-session composers on their current state and send paths.
Retain a recoverable submission snapshot until server acceptance.
Preserve retry identities and duplicate-submit protection across remounts.
Keep newer edits separate from an earlier pending submission.
Preserve fix(web): keep one composer from the empty chat into the new chat #910's mounted composer, focus, and early session navigation.
Keep consumed prefill intents invalid after an identity reset.

Conflicting prefill does not replace an existing draft. This follows the
updated silent-retention scope instead of the confirmation dialog proposed
in the issue. No dialogs or translated strings are added.

Test plan

Run the four targeted chat suites on aa07876: 116 tests passed.
Suites: NewChatDraftProvider, ChatComponent, ChatComposer, and Chat.
Verify regressions for a new draft entered during a pending first POST
and for consumed prefill intents across identity resets.
Run scripts/check-pr-title.sh with this PR title.
Run git diff --check main...HEAD.
Verify the creation-failure E2E cannot forward writes to a real backend.
Record final-branch typecheck, full unit, and fullstack startup results.
Recheck the final branch in the browser.
Attach desktop (1280×900) and touch-phone (390×844) screenshots.

Not in this PR

Draft restoration after reload, tab closure, or browser restart.
Multiple drafts or cross-device synchronization.
General draft retention for existing sessions.
Backend APIs, database changes, or composer layout changes.

Keep unsent text, composer context and attachments in memory when navigating away from New chat. Restore the draft with the caret at the end, and clear it on logout or identity changes. Fixes rome-os#1029

Jessie-QingYu

🔁 This review has been superseded. See the latest review.

yunfanye

🔁 This review has been superseded. See the latest review.

yunfanye · 2026-10-14T17:50:36Z

Suggestion: shrink the scope and keep the draft in `sessionStorage`

Most of this PR's complexity comes from #1029's spec, not from the problem itself. Users lose typed text when they leave New chat. A smaller design covers that:

Proposed design

Composer-owned draft. The composer keeps local state. A small hook loads the draft when the composer mounts, saves it to sessionStorage on each change, and deletes it on logout or identity change. Key it by guardian identity. There's already a precedent in use-app-remix-resume.ts.
Attachments in memory. PendingUpload holds a real File, which can't be stored as JSON. Keep attachments in a plain in-memory variable outside React. That survives navigation, which is all web: preserve the new-chat draft across navigation #1029 needs. IndexedDB isn't worth adding for this.
Simple send rule. Clear the draft as soon as the user sends. If the send fails, write it back, unless the user has already typed something new. Storage can be written after the composer unmounts, so this still works when the user has left the page.
Only text and attachments count as a draft. A project, agent, persona or skill that an earlier link filled in does not. That fixes the P1 where "New chat in project B" or "chat with agent Y" silently does nothing.

What this removes

NewChatDraftProvider above App, and the full-App remount on reset
Re-rendering every mounted composer (including pinned side chats) on each keystroke
The controlled/uncontrolled double send-and-upload path in ChatComposer

Suggested cuts to #1029

The confirm-before-overwrite prefill flow
Handing a pending first send to its new session
Keeping the pending copy and retry IDs alive across unmounts

Tradeoffs

Drafts are stored on disk in plain text. sessionStorage limits this: each tab has its own copy, which is deleted when the tab closes, and tabs can't overwrite each other. localStorage would not give us that, so don't use it here.
Drafts survive a reload. web: preserve the new-chat draft across navigation #1029 listed this as out of scope, but I think it's a bonus.

I'd expect roughly 150 lines plus tests, compared with about 700 net non-test lines now. This doesn't replace the other open items: the composer locking after an upload, and the failure e2e not intercepting its POST (it needs page.route). Those still need fixing either way.

yunfanye · 2026-10-14T17:53:03Z

Follow-up to my comment above. We've settled the scope:

Text and selected context: store in sessionStorage. This already survives a page refresh, because the data lasts as long as the tab is open.
Attachments: leave them out of this PR. Don't keep them in memory or in IndexedDB for now. If users need attachments to survive a refresh later, IndexedDB can store File objects. It would need loading before the composer renders, a separate copy for each tab, and deletion on logout, so it should be its own change.

Replace the root draft Provider and controlled composer paths with composer-local state and sessionStorage recovery. - Restore exact text and selected context across navigation and reloads - Clear drafts on logout or identity changes - Prevent stale failures and consumed prefill from overwriting newer edits - Preserve restored persona until settings are ready - Keep first-send focus, early navigation and ordinary uploads unchanged - Exclude attachment retention across navigation and reloads - Add storage, hydration and request-isolated regression coverage

Jessie-QingYu

Code Review: 🛑 REQUEST_CHANGES

This PR keeps the New chat composer's draft, meaning its text plus the selected project, agent, skill and persona. The draft is saved in a sessionStorage record per identity, managed by a new useNewChatDraft hook and new-chat-draft-storage helper. Editing state stays local to ChatComposer. Submission clears the saved record when the send starts, and fail() restores it only when nothing newer has happened. Prefill intents (keyed by location.key) are used once across remounts. Drafts are cleared on logout and identity change via useNewChatDraftCleanup and ProfileMenu. ChatComponent now remounts Chat when the location changes in draft mode, and Chat stops a first send (session creation plus first turn) when its composer unmounts. This matches the updated scope of #1029: text and context only, sessionStorage, no root Provider. CI checks pass except layout-invariants, which is still pending.

The PR description is out of date. The head commit (27f9ffe) replaced the in-memory root Provider with sessionStorage. As a result, drafts now survive reloads, and attachments are deliberately not kept. The description claims the opposite on both points and still cites NewChatDraftProvider tests that no longer exist. In the code, the main problem is the recovery path after a session is adopted early: a failed first turn is written back into New chat storage, and nothing on the existing-session send path ever clears it. A second, smaller design issue: leaving New chat while the session is being created leaves an empty server session behind. Storage isolation looks sound. Records are keyed by owner, cleared on identity switch and logout, and parsed with zod. The rest of the race handling (revision checks, late-failure protection, intent receipts) is careful and well tested.

Verdict: REQUEST_CHANGES — If a first send creates its session but the turn POST fails, the message is restored into New chat storage and never cleared. A retry that succeeds in the session therefore leaves an already-sent message to reappear as a New chat draft.

3 finding(s) posted as inline comments below.

Severity	Category	File	Title
P1	bug	`packages/web/src/hooks/use-new-chat-draft.ts`	Failure after session adoption leaves an already-sent message as a New chat draft
P2	design	`packages/web/src/components/chat/Chat.tsx`	Leaving New chat during creation orphans a server session
P3	documentation	`packages/web/src/lib/new-chat-draft-storage.ts`	PR description contradicts the implementation

Automated review by RomeOS Code Review · commit 27f9ffe

Jessie-QingYu · 2026-10-17T05:49:35Z

+          // they must still defeat this older recovery snapshot.
+          if (edits.current !== revision || (enabled && live.current.owner !== owner)) return false;
+          const restored =
+            owner && cleared ? restoreNewChatDraft(owner, cleared.revision, snapshot) : false;


[P1] bug — Failure after session adoption leaves an already-sent message as a New chat draft

fail() uses the enabled/owner values captured when the send began. Suppose createSession succeeds (so persistNewChatDraft becomes false once mainSessionId is set) but postTurn fails. restoreNewChatDraft then writes the snapshot back to New chat storage, and the session composer shows the same text. If the guardian retries in the session and it succeeds, nothing clears that record: beginSend gets cleared = null when disabled. The next visit to New chat shows the already-sent message as a draft, which invites a duplicate send. Fix: skip the storage restore when live.current.enabled is false at failure time, because the session composer already holds the text locally. Add a hook test for 'adopt, then fail without edits, then New chat is empty'.

Jessie-QingYu · 2026-10-17T05:49:35Z

          creatingSessionRef.current = false;
        }
+        if (!isMountedRef.current || controls.signal.aborted)
+          throw new DOMException("Cancelled draft", "AbortError");


[P2] design — Leaving New chat during creation orphans a server session

If the composer unmounts after createSession resolves, the send now throws AbortError and restores the draft. The session already exists on the server, though, and gets no turn, and the e2e test 'late create … never posts a turn' confirms this. Each time the guardian leaves during creation and later resends, another empty "New chat" session is created. Either delete the created session (best-effort) before throwing, or keep posting the turn without calling onSessionCreated and accept the submission instead of restoring it. The second option still meets the 'no redirect' requirement without leaving a blank session.

Jessie-QingYu · 2026-10-17T05:49:35Z

@@ -0,0 +1,118 @@
+import { z } from "zod";


[P3] documentation — PR description contradicts the implementation

The description says 'Keep one in-memory draft above the route tree', 'Preserve … pending attachments', and lists 'Draft restoration after reload' under 'Not in this PR'. Its test plan also cites NewChatDraftProvider suites. The head commit does the opposite: it persists to sessionStorage (so drafts survive reload), excludes attachments, and removes the Provider. Update the description to match the actual design (and #1029's current scope) so reviewers and the changelog aren't misled.

yunfanye

Code Review: 🛑 REQUEST_CHANGES

The implementation fits #1029’s updated sessionStorage scope, although the PR description still describes the earlier in-memory design. Two identity-loading races need correction.

Web typecheck, 138 targeted unit tests, and seven desktop E2Es pass. Temporary mounted-composer regression tests reproduce both findings below.

Verdict: REQUEST_CHANGES — Initial identity hydration can erase saved context and prevent recovery of failed submissions.

2 finding(s) posted as inline comments below.

Severity	Category	File	Title
P1	error-handling	`packages/web/src/hooks/use-new-chat-draft.ts`	Preserve failed sends across initial identity hydration
P1	data-integrity	`packages/web/src/hooks/use-new-chat-draft.ts`	Preserve untouched saved context during late hydration

Automated review by RomeOS Code Review · commit 27f9ffe

yunfanye · 2026-10-17T05:53:02Z

+        fail() {
+          // After early session adoption local edits no longer save to New chat;
+          // they must still defeat this older recovery snapshot.
+          if (edits.current !== revision || (enabled && live.current.owner !== owner)) return false;


[P1] error-handling — Preserve failed sends across initial identity hydration

The composer allows submission while /api/auth/me is pending, so beginSend can capture owner === null and optimistically clear the text. If identity resolves before session creation or the first POST rejects, this guard treats initial hydration as an identity change and skips recovery, leaving the composer and saved draft empty. Prevent submission until draft.ready, or distinguish initial identity resolution from an actual identity reset; add a regression covering this ordering.

yunfanye · 2026-10-17T05:53:02Z

+    const switching = hydrated.current !== null;
+    activateDraftOwner(owner);
+    const stored = readNewChatDraft(owner);
+    if (switching || edits.current === 0) replace(stored?.value ?? emptyNewChatDraft());


[P1] data-integrity — Preserve untouched saved context during late hydration

Typing before identity resolves increments edits.current, causing hydration to skip the entire saved record and then overwrite it with local defaults at line 65. Consequently, editing only text permanently erases the saved project, agent, skill, and persona; a mounted-composer regression confirms the saved project becomes default. Merge untouched context fields from storage, or finish hydration before allowing edits.

Max-Samson · 2026-10-17T05:53:34Z

Follow-up to my comment above. We've settled the scope:

Text and selected context: store in sessionStorage. This already survives a page refresh, because the data lasts as long as the tab is open.

Attachments: leave them out of this PR. Don't keep them in memory or in IndexedDB for now. If users need attachments to survive a refresh later, IndexedDB can store File objects. It would need loading before the composer renders, a separate copy for each tab, and deletion on logout, so it should be its own change.

Thanks @yunfanye for clarifying the scope and suggesting a simpler approach.

Scope updated following the review discussion.

Commit 27f9ffe replaces the in-memory Provider with composer-local
state and sessionStorage recovery for text and selected context.
Navigation and reload recovery are included.

Attachment retention, whether in memory or IndexedDB, and
cross-unmount retry handoff are excluded from this PR.
Attachment restoration can be a separate change if needed.

Issue #1029 has been updated to match this scope. I will also align
the PR description and test plan.

Max-Samson · 2026-10-17T06:02:51Z

What this PR does

Leaving New chat discards an unfinished message. This PR keeps that draft available when the guardian returns.

Preserve exact text, selected context, and pending attachments.

Restore the draft through New chat and its keyboard shortcut.

Place the caret at the end on re-entry without interrupting normal editing.

Keep explicit deletions empty.

Clear the draft on logout or identity changes.

Closes #1029

Design & Invariants

Keep one in-memory draft above the route tree.

Keep existing-session composers on their current state and send paths.

Retain a recoverable submission snapshot until server acceptance.

Preserve retry identities and duplicate-submit protection across remounts.

Keep newer edits separate from an earlier pending submission.

Preserve fix(web): keep one composer from the empty chat into the new chat #910's mounted composer, focus, and early session navigation.

Keep consumed prefill intents invalid after an identity reset.

Conflicting prefill does not replace an existing draft. This follows the updated silent-retention scope instead of the confirmation dialog proposed in the issue. No dialogs or translated strings are added.

Test plan

Run the four targeted chat suites on aa07876: 116 tests passed.
Suites: NewChatDraftProvider, ChatComponent, ChatComposer, and Chat.

Verify regressions for a new draft entered during a pending first POST
and for consumed prefill intents across identity resets.

Run scripts/check-pr-title.sh with this PR title.

Run git diff --check main...HEAD.

Verify the creation-failure E2E cannot forward writes to a real backend.

Record final-branch typecheck, full unit, and fullstack startup results.

Recheck the final branch in the browser.

Attach desktop (1280×900) and touch-phone (390×844) screenshots.

Not in this PR

Draft restoration after reload, tab closure, or browser restart.

Multiple drafts or cross-device synchronization.

General draft retention for existing sessions.

Backend APIs, database changes, or composer layout changes.

Scope updated following the review discussion.

Commit 27f9ffe replaces the in-memory Provider with composer-local
state and sessionStorage recovery for text and selected context.
Navigation and reload recovery are included. Attachment retention
and cross-unmount retry handoff are excluded.

feat(web): preserve new-chat drafts across navigation

aa07876

Keep unsent text, composer context and attachments in memory when navigating away from New chat. Restore the draft with the caret at the end, and clear it on logout or identity changes. Fixes rome-os#1029

Jessie-QingYu reviewed Oct 14, 2026

View reviewed changes

yunfanye reviewed Oct 14, 2026

View reviewed changes

Max-Samson mentioned this pull request Oct 16, 2026

web: preserve the new-chat draft across navigation #1029

Open

20 tasks

Max-Samson changed the title ~~feat(web): preserve new-chat drafts across navigation~~ feat(web): preserve new-chat text and context across navigation and reloads Oct 17, 2026

Jessie-QingYu reviewed Oct 17, 2026

View reviewed changes

yunfanye reviewed Oct 17, 2026

View reviewed changes

Conversation

Max-Samson commented Oct 14, 2026

What this PR does

Design & Invariants

Test plan

Not in this PR

Uh oh!

Jessie-QingYu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yunfanye left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yunfanye commented Oct 14, 2026

Suggestion: shrink the scope and keep the draft in sessionStorage

Uh oh!

yunfanye commented Oct 14, 2026

Uh oh!

Jessie-QingYu left a comment

Choose a reason for hiding this comment

Code Review: 🛑 REQUEST_CHANGES

Uh oh!

Jessie-QingYu Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

Jessie-QingYu Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

Jessie-QingYu Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

yunfanye left a comment

Choose a reason for hiding this comment

Code Review: 🛑 REQUEST_CHANGES

Uh oh!

yunfanye Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

yunfanye Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

Max-Samson commented Oct 17, 2026

Uh oh!

Max-Samson commented Oct 17, 2026

What this PR does

Design & Invariants

Test plan

Not in this PR

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Jessie-QingYu left a comment •

edited

Loading

yunfanye left a comment •

edited

Loading

Suggestion: shrink the scope and keep the draft in `sessionStorage`