refactor(web): extract useDraftInput hook from god-hook (LUM-1737)#31456
Conversation
Extract per-conversation draft persistence into a focused useDraftInput hook owned by ChatPage, backed by localStorage keyed by assistantId. Fixes bug #5 (LUM-1516): onDraftRestored no longer misfires on pull-to-refresh or feature flag re-runs — the hook uses a prev/next key ref to detect genuine conversation switches vs. same-key effect re-runs. Removes draftsRef, inputRef, setInput, and onDraftRestored from: - useConversationHistory params and deps array - useConversationLoader params and call site - ChatRouteRefs interface ChatRouteContent now receives saveDraft/clearDraft callbacks instead of a mutable draftsRef. Closes LUM-1737 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27be355105
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (assistantIdRef.current) { | ||
| persistDrafts(assistantIdRef.current, draftsRef.current); |
There was a problem hiding this comment.
When assistantId changes, the earlier effect has already swapped draftsRef/assistantIdRef to the next assistant before the conversation-switch effect runs, so this persist call writes the outgoing draft into the wrong assistant namespace. In a real assistant switch (A → B) with unsent text, A’s latest draft is not saved under A and can later reappear (or be lost) under B’s storage, which breaks per-assistant draft isolation and causes draft loss when returning to A.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this is a real bug. When both assistantId and activeConversationKey change in the same render, effects run in declaration order: the assistantId effect swaps assistantIdRef and draftsRef to the new assistant before the switch effect runs, so the outgoing draft would be persisted under the wrong namespace.
Fixed in 5d79577: the assistantId effect now flushes outgoing drafts (including any in-progress text) under the previous assistantId before swapping to the new one. This ensures per-assistant draft isolation is maintained even during simultaneous assistant + conversation changes.
Resolved.
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE
Value: Users who type a message, switch conversations, and switch back now have their draft reliably restored — and the spurious "Draft restored" notice that fired on pull-to-refresh (LUM-1516 bug #5) is fixed. Every engineer touching the chat pipeline gets a simpler useConversationHistory interface with ~80 fewer lines to reason about.
What this does: Extracts draft persistence (input state + saveDraft/clearDraft) out of the ~530-line useConversationHistory god-hook into a new focused useDraftInput hook. Replaces the draftsRef that was manually threaded through ChatPage → useConversationLoader → useConversationHistory. Adds localStorage backing so drafts survive explicit conversation switches (consistent with vellum:lastViewedConversation: and vellum:contextWindow: storage patterns).
use-draft-input.ts — switch detection logic
The previousKeyRef guard is the right fix for LUM-1516:
const isSwitch = prevKey !== null && prevKey !== activeConversationKey;Same-key re-runs (pull-to-refresh incrementing refreshEpoch, feature-flag dep changes) are now no-ops inside this hook — no stale overwrite of the live composer input, no spurious onDraftRestored call. This is cleaner than the old in-useConversationHistory approach which had a long comment explaining why the guard was needed and still got it wrong.
setInput wrapper
const setInput = useCallback((action) => {
setInputState((prev) => {
const next = typeof action === "function" ? action(prev) : action;
inputValueRef.current = next;
return next;
});
}, []);inputValueRef staying in sync via the functional updater is the correct pattern — reads inputValueRef.current inside the switch effect are always fresh, not a stale closure. useCallback([]) here is for correctness (stable identity for dep tracking), not just a performance hint. ✅
saveDraft / clearDraft empty deps
Both only mutate draftsRef.current and assistantIdRef.current (refs, not closed-over state), so empty dep arrays are correct. React Compiler won't miscache these. ✅
Anti-patterns check — clean
- No
MutableRefObjectidentity in dep arrays (onlyactiveConversationKey,setInput,onDraftRestored) - No
ref.currentreads in render - No multi-field Zustand selector concerns (hook doesn't touch Zustand)
setInputwrapper handles both value-style and function-style updates correctly
Minor observations (non-blocking)
-
Draft not auto-restored on page load. On mount,
loadDrafts()populatesdraftsReffrom localStorage, but the current conversation's saved draft is only surfaced when the user switches AWAY and back. If they reload the page while on conversation A, the draft is in localStorage but won't appear in the composer until they switch. This is strictly better than before (was always lost on reload), but worth noting as a fast-follow if Boss wants full restoration on mount. -
inputRef+inputValueRefdual tracking.handleRefreshConversationinchat-route-content.tsxstill reads from the DOM textareainputRef.current?.value, whileuseDraftInputtracks the same value ininputValueRef. They should always be in sync, but it's two sources of truth. IfinputRefever lags (e.g., IME composition in-flight),saveDraftwould get the wrong value. A future pass could replace theinputRef.current?.valueread inhandleRefreshConversationwith the exposedinputstate fromuseDraftInput, but this is fine as-is. -
use-draft-input.tslives inchat-composer/butChatPageimports it from the domain root. Makes sense as placement (it's composer state), and the import path is explicit, so no concern.
Tests
The localStorage shim pattern is correct for Bun's environment (no jsdom). Installing on globalThis.window before the dynamic import ensures the module under test sees it. The 13 tests cover the serialization contract well: round-trips, malformed JSON, non-string values, and assistantId scoping. The switch-detection logic isn't exercised (no hook test harness), but that's an acknowledged workspace constraint.
Merge gate
- ✅ Vex APPROVED (reviewed at
27be3551) - ⏳ Codex / Devin second approval pending
Vellum Constitution — Yours: composer state now belongs to the Composer — users get their drafts back where they left them, and the system is honest about it.
When both assistantId and activeConversationKey change in the same render, effects run in declaration order. The assistantId effect now persists outgoing drafts under the previous assistantId before loading the new assistant's drafts, preventing cross-assistant draft leakage. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
devin-ai-integration
Bot
dismissed
vex-assistant-bot[bot]’s stale review
via
5d79577
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| draftsRef.current.set(prevConvKey, currentInput); | ||
| } else { | ||
| draftsRef.current.delete(prevConvKey); | ||
| } | ||
| } | ||
| persistDrafts(prevId, draftsRef.current); | ||
| } | ||
|
|
||
| if (!assistantId) { | ||
| draftsRef.current = new Map(); | ||
| assistantIdRef.current = null; | ||
| return; | ||
| } | ||
| draftsRef.current = loadDrafts(assistantId); | ||
| assistantIdRef.current = assistantId; | ||
| }, [assistantId]); | ||
|
|
||
| // ----------------------------------------------------------------------- | ||
| // Conversation switch: save outgoing draft, restore incoming draft | ||
| // ----------------------------------------------------------------------- | ||
| useEffect(() => { | ||
| const prevKey = previousKeyRef.current; | ||
| const isSwitch = prevKey !== null && prevKey !== activeConversationKey; | ||
|
|
||
| if (isSwitch && prevKey) { | ||
| // Save outgoing conversation's draft. | ||
| const currentInput = inputValueRef.current; | ||
| if (currentInput.trim()) { | ||
| draftsRef.current.set(prevKey, currentInput); | ||
| } else { | ||
| draftsRef.current.delete(prevKey); | ||
| } | ||
|
|
||
| // Restore incoming conversation's draft (or clear). | ||
| const savedDraft = | ||
| (activeConversationKey && |
There was a problem hiding this comment.
🔴 useDraftInput misidentifies draft-key resolution as a conversation switch, clearing user input
When a user sends a message in a draft conversation, useSendMessage resolves the draft key to a server-assigned key by setting draftKeyResolutionRef.current = true and previousConversationKeyRef.current = newKey before calling setActiveKey(newKey) (use-send-message.ts:558-562). The old code in useConversationHistory checked draftKeyResolutionRef and returned early (a no-op), preserving the composer state. The new useDraftInput hook has no knowledge of draftKeyResolutionRef — it only tracks its own internal previousKeyRef. When activeConversationKey changes from draft-xxx to conv-yyy, the effect at line 148 evaluates isSwitch = true and calls setInput(savedDraft) (line 164), where savedDraft is typically "" since no draft exists for the newly-minted server key. If the user typed follow-up text in the composer after clicking Send but before the API response resolves the key, that text is silently discarded. It also spuriously saves a draft under the now-orphaned draft key and may fire the onDraftRestored callback incorrectly.
Prompt for agents
The useDraftInput hook needs to be aware of draft-key-resolution events so it can skip treating them as conversation switches.
The existing pattern uses draftKeyResolutionRef (owned by ChatPage, written by useSendMessage) as a guard flag. useConversationHistory checks this ref and returns early when it is true.
Approach 1: Pass draftKeyResolutionRef into useDraftInput as a parameter. At the top of the conversation-switch effect (line 146), check if draftKeyResolutionRef.current is true. If so, skip the save/restore logic — only update previousKeyRef.current to the new activeConversationKey so future switches work correctly. Do NOT reset draftKeyResolutionRef here (useConversationHistory already handles that).
Approach 2: Have useSendMessage also synchronously update useDraftInput's previousKeyRef when resolving a draft key, similar to how it already updates previousConversationKeyRef (use-send-message.ts:560). This would prevent useDraftInput from seeing a stale previous key. However, this couples useSendMessage to useDraftInput internals.
Approach 1 is recommended because it mirrors the existing guard pattern and keeps the hooks decoupled.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — this is a real bug. When useSendMessage resolves draft-xxx → conv-yyy, it sets draftKeyResolutionRef.current = true as a guard. useConversationHistory checks this and returns early, but useDraftInput had no knowledge of it — it would see prevKey !== activeConversationKey, treat it as a switch, and clear the composer.
Fixed in e7f81b1: draftKeyResolutionRef is now passed into useDraftInput as a parameter. At the top of the switch effect, if the ref is true, we skip save/restore and only update previousKeyRef so future switches work correctly. We deliberately don't reset the ref here — useConversationHistory already handles that (line 204).
Went with Approach 1 (pass the ref) as recommended — mirrors the existing guard pattern and keeps the hooks decoupled.
| draftsRef.current.set(prevConvKey, currentInput); | ||
| } else { | ||
| draftsRef.current.delete(prevConvKey); | ||
| } | ||
| } | ||
| persistDrafts(prevId, draftsRef.current); | ||
| } | ||
|
|
||
| if (!assistantId) { | ||
| draftsRef.current = new Map(); | ||
| assistantIdRef.current = null; | ||
| return; | ||
| } | ||
| draftsRef.current = loadDrafts(assistantId); | ||
| assistantIdRef.current = assistantId; | ||
| }, [assistantId]); | ||
|
|
||
| // ----------------------------------------------------------------------- | ||
| // Conversation switch: save outgoing draft, restore incoming draft | ||
| // ----------------------------------------------------------------------- | ||
| useEffect(() => { | ||
| const prevKey = previousKeyRef.current; | ||
| const isSwitch = prevKey !== null && prevKey !== activeConversationKey; | ||
|
|
||
| if (isSwitch && prevKey) { | ||
| // Save outgoing conversation's draft. | ||
| const currentInput = inputValueRef.current; | ||
| if (currentInput.trim()) { | ||
| draftsRef.current.set(prevKey, currentInput); | ||
| } else { | ||
| draftsRef.current.delete(prevKey); | ||
| } | ||
|
|
||
| // Restore incoming conversation's draft (or clear). | ||
| const savedDraft = | ||
| (activeConversationKey && |
There was a problem hiding this comment.
🚩 Drafts persisted to localStorage are not restored on initial page load
The new useDraftInput hook loads drafts from localStorage when assistantId changes (line 134-141), but the conversation-switch effect (line 146-181) only restores a draft when previousKeyRef.current !== null — which is null on initial mount. This means if a user has a saved draft, closes the browser, and reopens the same conversation, the draft is not restored into the composer. However, this matches the old behavior: the old draftsRef was in-memory only and always started empty on mount. The localStorage persistence is a net improvement for cross-switch survival. Full page-reload restoration could be added as a follow-up by checking draftsRef.current.get(activeConversationKey) when previousKeyRef is null, but this is a feature enhancement, not a regression.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct analysis — this matches old behavior (in-memory draftsRef was always empty on mount). The localStorage persistence is a net improvement for cross-switch survival. Full page-reload restoration would be a feature enhancement for a follow-up.
When useSendMessage resolves a draft key to a server-assigned key (draft-xxx → conv-yyy), the key change is not a real conversation switch. Pass draftKeyResolutionRef into useDraftInput so it can skip save/restore and only update previousKeyRef, matching the existing guard in useConversationHistory. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
There was a problem hiding this comment.
✦ APPROVE — re-review at e7f81b19 (HEAD, full PR state)
Value: Users who type a draft, switch conversations, and switch back now reliably see their text — and the logic that does it no longer travels through four unrelated files.
What this does: Extracts per-conversation draft persistence from the ~530-line useConversationHistory god-hook into a dedicated useDraftInput hook. Replaces the mutable draftsRef (threaded through ChatPage → useConversationLoader → useConversationHistory) with localStorage-backed persistence and a clean { input, setInput, saveDraft, clearDraft } surface.
use-draft-input.ts — The new hook
The implementation is correct and well-layered:
localStorage helpers (loadDrafts / persistDrafts) — proper defensive coding. typeof window === "undefined" guard for SSR-safety, JSON validation rejects null/array/non-object shapes, non-string entries filtered with a type predicate, try/catch on both read and write (private browsing / quota exceeded). No issues.
setInput wrapper — correctly wraps setInputState to keep inputValueRef.current in sync, handles both value and updater-function forms. Empty useCallback deps are correct since setInputState is stable.
Effect ordering — assistantId effect is declared before the switch effect. React fires effects in declaration order, so the assistantId effect flushes the outgoing draft under the previous assistantId before the switch effect runs. This is the exact fix for the cross-assistant draft leakage Codex caught — and it's structurally enforced, not just documented.
draftKeyResolutionRef guard — reads the ref and early-returns without save/restore when true, then unconditionally updates previousKeyRef. This correctly handles the draft-xxx → conv-yyy resolution case without clearing the composer. The ref is owned/reset by useSendMessage (pre-existing contract).
previousKeyRef gate — isSwitch requires prevKey !== null && prevKey !== activeConversationKey. This is the bug #5 fix: same-key effect re-runs (feature-flag re-evaluations, pull-to-refresh) no longer fire onDraftRestored.
One non-blocking observation: initial page-load draft restoration is not attempted — on first mount prevKey is null so isSwitch = false and no draft is restored. This matches the old draftsRef behavior (empty on mount) and Devin's inline comment confirms this is intentional. localStorage persistence is still a net win — cross-switch survival and cross-tab survival both improve. Follow-on enhancement as needed.
One non-blocking observation: draftsRef entries for deleted/archived conversations are never pruned. Low concern — localStorage quotas bound total storage naturally — but worth a future cleanup ticket if users with large conversation histories start hitting quota issues.
use-conversation-history.ts and use-conversation-loader.ts
Clean deletions. inputRef, draftsRef, setInput, onDraftRestored removed from both the params interface and the call site. The docstring update in useConversationHistory correctly reflects the narrowed scope. No regressions visible in the dep arrays or effect bodies.
chat-page.tsx
draftsRef and useState("") for input replaced by a single useDraftInput() call. draftKeyResolutionRef is threaded in correctly (it was already present). saveDraft / clearDraft passed down to ChatRouteContent. Import placement is consistent with other hook imports in the block.
chat-route-content.tsx
saveDraft(key, text) replaces the inline if (currentInput.trim()) { draftsRef.current.set(...) } else { draftsRef.current.delete(...) } in handleRefreshConversation. clearDraft(key) replaces draftsRef.current.delete(key) on send. Both preserve the same conditional trim-before-save semantics (the logic lives in saveDraft now). Dep array updated correctly in handleRefreshConversation (draftsRef → saveDraft).
use-draft-input.test.ts and use-draft-input-test-helpers.ts
13 tests cover the localStorage serialization contract: round-trip, non-string value filtering, malformed JSON, empty values, and per-assistantId key namespacing. The localStorage shim is clean. Hook behavioral tests (switch detection, assistantId flush) aren't feasible without a JSDOM renderer in this environment — the existing coverage is appropriate for what's testable.
Anti-pattern cross-check
- ✅
useEffectwithout cleanup: no subscriptions, listeners, or timers — no cleanup needed - ✅ Ref reads inside effects (not in render):
inputValueRef.current,assistantIdRef.current, etc. all read inside effects/callbacks only - ✅ MutableRefObject identity not in dep arrays:
draftKeyResolutionRefcorrectly excluded - ✅ No multi-field Zustand selector issues introduced
- ✅ No
globalThis.fetchspy in tests - ✅ Error handling: localStorage ops wrapped in try/catch, no silently dropped errors
Merge gate
- ✅ Vex approved (this review,
e7f81b19) - ✅ CI 7/7 passing
- ✅ LUM-1737 in Done state
- ⏳ Awaiting second approval (Codex 👍 or Devin APPROVE on current HEAD)
Vellum Constitution — Yours: this extraction gives the composer actual ownership of its own state, rather than borrowing it from a page-level hook that doesn't know it exists.
Post-Merge Test ReportRan shell-based verification on main after merge. All draft-related checks pass. This is a state-management refactor — runtime draft behavior requires Django backend (not testable on Devin VMs). Test Results
Not Tested (Requires Django Backend)
|
1 participant
Prompt / plan
Phase 2 of LUM-1734 chat redesign — extract per-conversation draft persistence from the ~530-line
useConversationHistorygod-hook into a new, focuseduseDraftInputhook.Why this change is needed:
useConversationHistorymixes draft persistence (a pure UI concern) with cache management, history fetching, and state reconciliation. This extraction moves draft state to where it belongs — near the Composer.setup.shscript #5):onDraftRestoredmisfired on pull-to-refresh / feature-flag re-runs because the old code couldn't distinguish genuine conversation switches from same-key effect re-runs. The new hook uses apreviousKeyRefto compare prev vs. next conversation key — same-key re-renders are no-ops.draftsRef(threaded through 4 files) with localStorage-backed persistence, consistent with other chat storage patterns (vellum:lastViewedConversation:,vellum:contextWindow:, etc.).What changed:
use-draft-input.tsinputstate,saveDraft(),clearDraft(), localStorage round-trip, prev/next key switch detection, anddraftKeyResolutionRefguarduse-draft-input.test.tsuse-draft-input-test-helpers.tsuse-conversation-history.tsdraftsRef,inputRef,setInput,onDraftRestoredfrom params, deps array, and effect body. Updated docstring.use-conversation-loader.tsdraftsRef,inputRef,setInput,onDraftRestoredfrom params interface and call sitechat-page.tsxuseState("")+draftsRefwithuseDraftInput()call; passessaveDraft/clearDraftto ChatRouteContent; passesdraftKeyResolutionRefto new hookchat-route-content.tsxdraftsReffromChatRouteRefs; usessaveDraft()/clearDraft()callbacks instead ofdraftsRef.current.set()/.delete()Bugs found and fixed during review:
assistantIdandactiveConversationKeychanged in the same render, effects ran in declaration order — the assistantId effect swappedassistantIdRefbefore the switch effect persisted, writing the outgoing draft under the wrong assistant. Fix: the assistantId effect now flushes outgoing drafts under the previous assistantId before swapping.useSendMessageresolvesdraft-xxx→conv-yyy, the key change looked like a conversation switch touseDraftInput, clearing the composer. Fix:draftKeyResolutionRefis now passed into the hook — when true, the switch effect skips save/restore and only updatespreviousKeyRef.Why it's safe:
setup.shscript #5 and the newdraftKeyResolutionRefguard{ input, setInput, saveDraft, clearDraft }which maps 1:1 to what ChatPage and ChatRouteContent already consumedvellum:chatDrafts:{assistantId}) follows existing conventionsTest plan
bun test src/domains/chat/components/chat-composer/use-draft-input.test.tsbunx tsc --noEmit— no new type errorsbun run lint— no new warningsbun run test:ci— 92/92 tests passCloses LUM-1737
Link to Devin session: https://app.devin.ai/sessions/c4696ace57fe43649f2475d7661ac273
Requested by: @ashleeradka