[WIP] Refactor chat sidebar UI and fix thread ID generation #61
Conversation
- Simplify ThreadItem component with cleaner hover states - Reset editValue on double-click instead of useEffect sync - Reduce visual noise with smaller icons and tighter spacing - Fix thread ID generation to include counter for uniqueness - Add deduplication logic when loading threads from localStorage Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces premium chat thread persistence with server synchronization. It adds isPremium gating to article chat and sidebar components, implements a new chat threads hook with Redis persistence, creates server API routes for thread CRUD operations, and integrates thread history management into the proxy-content component with automatic message syncing. Changes
Sequence DiagramsequenceDiagram
participant User as User (Client)
participant ProxyContent as ProxyContent Component
participant ArticleChat as ArticleChat Component
participant ChatSidebar as ChatSidebar Component
participant useChatThreads as useChatThreads Hook
participant Server as Server API
participant Redis as Redis
User->>ProxyContent: View premium chat interface
ProxyContent->>ChatSidebar: Render with isPremium=true
ChatSidebar->>useChatThreads: useChatThreads(isPremium=true)
useChatThreads->>Server: GET /api/chat-threads
Server->>Redis: Load compressed threads
Redis-->>Server: Return compressed data
Server-->>useChatThreads: Return threads list
useChatThreads-->>ChatSidebar: Display threads
User->>ChatSidebar: Select existing thread
ChatSidebar->>ProxyContent: handleSelectThread(threadId)
ProxyContent->>ArticleChat: Load thread messages via ref
ArticleChat-->>ProxyContent: Display messages
User->>ArticleChat: Send new message
ArticleChat->>ProxyContent: onMessagesChange(updatedMessages)
ProxyContent->>useChatThreads: Update active thread
useChatThreads-->>useChatThreads: Debounce (1s)
useChatThreads->>Server: POST /api/chat-threads (compressed)
Server->>Redis: Save compressed threads
Redis-->>Server: Confirmed
Server-->>useChatThreads: Success response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the SMRY-pr-61 environment in smry
|
Greptile OverviewGreptile SummaryRefactored chat sidebar UI with cleaner styling and added chat thread persistence for premium users. Thread IDs now include a counter for better uniqueness, and deduplication logic prevents duplicate threads in localStorage. Major changes:
Issues found:
Previous review issues remain unaddressed:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ProxyContent
participant ChatSidebar
participant useChatThreads
participant ArticleChat
participant API
User->>ProxyContent: Opens page (isPremium=true)
ProxyContent->>useChatThreads: Initialize hook (isPremium)
useChatThreads->>API: GET /api/chat-threads
API-->>useChatThreads: Return server threads
useChatThreads->>useChatThreads: Sync to localStorage
useChatThreads-->>ChatSidebar: Display threads
User->>ChatSidebar: Click "New Chat"
ChatSidebar->>ProxyContent: handleNewChat()
ProxyContent->>useChatThreads: createThread()
useChatThreads->>useChatThreads: Generate ID with counter
useChatThreads->>API: POST /api/chat-threads (debounced)
ProxyContent->>ArticleChat: clearMessages()
User->>ArticleChat: Send message
ArticleChat->>ProxyContent: onMessagesChange(messages)
ProxyContent->>useChatThreads: updateThread() or createThread()
useChatThreads->>API: POST /api/chat-threads (debounced)
User->>ChatSidebar: Select existing thread
ChatSidebar->>ProxyContent: handleSelectThread(threadId)
ProxyContent->>ArticleChat: setMessages(thread.messages)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Notify parent when messages change (for thread syncing) | ||
| useEffect(() => { | ||
| if (messages.length > 0) { | ||
| onMessagesChange?.(messages); | ||
| } | ||
| }, [messages, onMessagesChange]); |
There was a problem hiding this comment.
Messages never sync cleared
onMessagesChange only fires when messages.length > 0, so clearing the chat (or switching to an empty thread) never propagates to the parent. Since ProxyContent uses this callback to persist thread messages, the thread store can retain stale messages even after the UI clears. Consider calling onMessagesChange on every change (including empty arrays) so thread persistence stays consistent.
| // Handle new chat from history sidebar | ||
| const handleNewChat = React.useCallback(() => { | ||
| createThread(); | ||
| // Close history sidebar after creating new chat | ||
| // Clear current chat messages for the new thread | ||
| articleChatRef.current?.clearMessages(); | ||
| setHistoryOpen(false); | ||
| }, [createThread]); | ||
|
|
||
| // Handle thread selection from history sidebar |
There was a problem hiding this comment.
Free users still create threads
handleNewChat calls createThread() even when useChatThreads(isPremium) is invoked with isPremium=false. In that mode saveThreads no-ops, but createThread still mutates in-memory state, so free users can accumulate thread state during a session despite the UI hiding history; if premium status flips, those unexpected threads can appear. If the intent is “no threads for free users,” guard createThread calls (and other thread mutations) behind isPremium.
| .post( | ||
| "/chat-threads", | ||
| async ({ body, request, set }) => { | ||
| const { threads } = body; | ||
|
|
||
| const authInfo = await getAuthInfo(request); | ||
| if (!authInfo.userId || !authInfo.isPremium) { | ||
| set.status = 401; | ||
| return { error: "Unauthorized", success: false }; | ||
| } | ||
|
|
||
| const key = getThreadsKey(authInfo.userId); | ||
|
|
||
| try { | ||
| const compressed = await compressAsync(threads); | ||
| await redis.set(key, compressed, { ex: CHAT_THREADS_TTL }); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| console.error("[chat-threads] Error saving:", error); | ||
| set.status = 500; | ||
| return { error: "Failed to save", success: false }; | ||
| } | ||
| }, | ||
| { | ||
| body: t.Object({ | ||
| threads: t.Array(t.Any()), | ||
| }), | ||
| } |
There was a problem hiding this comment.
Unbounded threads payload
POST /api/chat-threads accepts threads: t.Array(t.Any()) and then JSON.stringify + gzip-compresses it before storing in Redis. This makes it trivial for a client to send an extremely large payload that spikes CPU/memory (stringify+gzip) and writes huge Redis values. This endpoint needs a bounded schema (thread/message shape + max counts/size) or explicit request size limits to avoid resource exhaustion.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 ResizableChatLayout doesn't pass isPremium to ChatSidebar, breaking chat page for premium users
ResizableChatLayout destructures from ResizableChatSidebarProps extends ChatSidebarProps but omits isPremium from its destructuring and doesn't pass it to the inner ChatSidebar component.
Detailed Explanation
At components/features/chat-sidebar.tsx:346-353, ResizableChatLayout destructures only isOpen, onOpenChange, onNewChat, onSelectThread, activeThreadId, and children — but not isPremium. At line 388-394, ChatSidebar is rendered without the isPremium prop, so it defaults to false.
This component is used in components/features/chat-page-content.tsx:244 for the standalone chat page. Since isPremium is never passed through, premium users on the /chat page will:
- See the "Upgrade to Pro" prompt instead of their thread list (because
ChatSidebargates thread list display behindisPremiumat line 276) - Not see the "New Chat" button or search bar (gated behind
isPremiumat line 249) - Have
useChatThreads(false)called, which skips loading threads and server sync
Impact: The entire chat page sidebar is broken for premium users — they cannot see or manage their threads.
(Refers to lines 388-394)
Prompt for agents
Two changes are needed:
1. In components/features/chat-sidebar.tsx, update the ResizableChatLayout function (around line 346) to destructure isPremium from props and pass it to ChatSidebar. Change the destructuring to include isPremium, and add isPremium={isPremium} to the ChatSidebar JSX at line 388.
2. In components/features/chat-page-content.tsx, the useChatThreads() call at line 52 needs to receive isPremium. Import useIsPremium hook and pass the value: useChatThreads(isPremium). Also pass isPremium to ResizableChatLayout at line 244.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (currentThreadId) { | ||
| updateThread(currentThreadId, { messages: threadMessages, title }); |
There was a problem hiding this comment.
🔴 handleMessagesChange overwrites user-renamed thread titles on every message sync
Every time messages are synced to a thread, handleMessagesChange unconditionally auto-generates a title from the first user message and passes it to updateThread, overwriting any custom title the user set via the rename feature.
Root Cause
At components/features/proxy-content.tsx:506-512, the title is always computed from the first user message:
const title = firstUserMsg
? firstUserMsg.content.slice(0, 50) + ...
: "New Chat";
updateThread(currentThreadId, { messages: threadMessages, title });This runs every time messages change (triggered by the useEffect at components/features/article-chat.tsx:224-228). If a user has renamed a thread to "My Research", the next message sync will overwrite it with the auto-generated title from the first message content.
Notably, the standalone chat page (chat-page-content.tsx:143-151) handles this correctly by only auto-titling when currentThread.title === "New Chat". The proxy-content implementation lacks this guard.
Impact: Users cannot persistently rename threads — any custom title is lost as soon as a new message is sent or the thread is loaded.
Prompt for agents
In components/features/proxy-content.tsx, the handleMessagesChange callback (around line 495-517) should only auto-generate a title when the current thread's title is still the default "New Chat". To do this:
1. Add threads to the dependency array of handleMessagesChange
2. Look up the current thread: const currentThread = threads.find(t => t.id === currentThreadId)
3. Only include title in the updateThread call when currentThread?.title === "New Chat" or when creating a new thread. For example:
if (currentThreadId) {
const updates: any = { messages: threadMessages };
const currentThread = threads.find(t => t.id === currentThreadId);
if (currentThread?.title === "New Chat" && firstUserMsg) {
updates.title = title;
}
updateThread(currentThreadId, updates);
}
This matches the pattern used in chat-page-content.tsx at lines 143-151.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } else { | ||
| // Auto-create thread on first message | ||
| createThread(title); |
There was a problem hiding this comment.
🔴 Auto-created thread on first message has no messages saved
When currentThreadId is null and handleMessagesChange fires, createThread(title) is called but the actual messages are not passed to the new thread, resulting in an empty thread being persisted.
Root Cause
At components/features/proxy-content.tsx:513-515:
} else {
// Auto-create thread on first message
createThread(title);
}createThread (defined at lib/hooks/use-chat-threads.ts:228-250) only accepts a title parameter and creates the thread with messages: []. The actual chat messages from the handleMessagesChange argument are discarded.
While the onMessagesChange effect will re-fire on the next render (because handleMessagesChange is recreated with the new currentThreadId), there's a window where:
- The thread is saved to localStorage and potentially synced to the server with empty messages
- If the user navigates away or the component unmounts before the next render cycle, the thread remains permanently empty
Impact: Newly auto-created threads may be saved with no messages, losing the user's conversation.
Prompt for agents
In components/features/proxy-content.tsx at line 513-515, when auto-creating a thread on first message, the messages should be saved to the thread immediately. The createThread function only accepts a title, so after creating the thread, immediately call updateThread with the messages. For example:
} else {
// Auto-create thread on first message
const newThread = createThread(title);
updateThread(newThread.id, { messages: threadMessages });
}
This ensures the thread is created with the actual messages rather than an empty array.
Was this helpful? React with 👍 or 👎 to provide feedback.
# Conflicts: # components/features/proxy-content.tsx
| let idCounter = 0; | ||
| function generateId(): string { | ||
| return `${Date.now()}-${Math.random().toString(36).substring(2, 11)}`; | ||
| idCounter++; | ||
| return `${Date.now()}-${idCounter}-${Math.random().toString(36).substring(2, 9)}`; | ||
| } |
There was a problem hiding this comment.
idCounter resets to 0 on page refresh since it's a module-level variable, potentially causing ID collisions if two users open tabs at nearly the same timestamp.
| useEffect(() => { | ||
| if (!isSignedIn || !isPremium || !serverFetchComplete || hasSyncedRef.current) return; | ||
|
|
||
| hasSyncedRef.current = true; | ||
|
|
||
| const timer = setTimeout(() => { | ||
| if (serverThreads && serverThreads.length > 0) { | ||
| setThreads(serverThreads); | ||
| try { | ||
| window.localStorage.setItem(THREADS_KEY, JSON.stringify(serverThreads)); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| } | ||
| setIsLoaded(true); | ||
| }, 0); | ||
| return () => clearTimeout(timer); | ||
| }, [serverThreads, serverFetchComplete, isSignedIn, isPremium]); |
There was a problem hiding this comment.
Server threads overwrite local state only when serverThreads.length > 0, so if a premium user deletes all threads on another device, this tab retains stale local threads instead of clearing them.
| useEffect(() => { | ||
| if (!isPremium) { | ||
| const t = setTimeout(() => { | ||
| setThreads([]); | ||
| setIsLoaded(true); | ||
| }, 0); | ||
| return () => clearTimeout(t); | ||
| } | ||
|
|
||
| const timer = setTimeout(() => { | ||
| try { | ||
| const item = window.localStorage.getItem(THREADS_KEY); | ||
| if (item) { | ||
| const parsed = JSON.parse(item) as ChatThread[]; | ||
| setThreads(parsed); | ||
| // Deduplicate by ID (keep first occurrence) | ||
| const seen = new Set<string>(); | ||
| const deduped = parsed.filter((thread) => { | ||
| if (seen.has(thread.id)) return false; | ||
| seen.add(thread.id); | ||
| return true; | ||
| }); | ||
| // Save deduped list if there were duplicates | ||
| if (deduped.length !== parsed.length) { | ||
| window.localStorage.setItem(THREADS_KEY, JSON.stringify(deduped)); | ||
| } | ||
| setThreads(deduped); | ||
| } | ||
| } catch (error) { | ||
| console.warn("Error reading threads from localStorage:", error); | ||
| } | ||
| setIsLoaded(true); | ||
| // Only mark loaded if server sync isn't going to override | ||
| if (!isSignedIn || !isPremium) { | ||
| setIsLoaded(true); | ||
| } | ||
| }, 0); | ||
| return () => clearTimeout(timer); | ||
| }, []); | ||
| }, [isPremium, isSignedIn]); |
There was a problem hiding this comment.
Race condition: localStorage loads and sets isLoaded=true before server sync completes (line 169), but server sync effect (line 103-120) only runs after serverFetchComplete. If server is slow, UI shows stale local threads then jumps to server state.
Additional Comments (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/features/chat-sidebar.tsx (2)
53-60:⚠️ Potential issue | 🟡 MinorDouble rename call on Enter due to subsequent
onBlurfiring.When the user presses Enter,
handleSubmitRenameruns and setsisEditingtofalse, which unmounts the<input>. The unmount triggersonBlur, callinghandleSubmitRenamea second time. Sincethread.title(a prop) hasn't updated yet, theeditValue !== thread.titleguard passes again, producing a duplicateonRenamecall.Suggested fix — guard with a ref
+ const isSubmittingRef = useRef(false); + const handleSubmitRename = () => { + if (isSubmittingRef.current) return; + isSubmittingRef.current = true; if (editValue.trim() && editValue !== thread.title) { onRename(editValue.trim()); } else { setEditValue(thread.title); } setIsEditing(false); + // Reset after a tick so future edits work + setTimeout(() => { isSubmittingRef.current = false; }, 0); };Also applies to: 77-93
346-394:⚠️ Potential issue | 🟡 Minor
ResizableChatLayoutdoesn't forwardisPremiumtoChatSidebar.
ResizableChatSidebarPropsextendsChatSidebarProps(which includesisPremium), but the prop is neither destructured nor passed to the innerChatSidebaron line 388. This means the sidebar will always render in non-premium mode when used via this wrapper.If
ResizableChatLayoutis currently unused (proxy-content.tsx rendersChatSidebardirectly), consider either removing this dead component or fixing the forwarding.Suggested fix
export function ResizableChatLayout({ isOpen, onOpenChange, onNewChat, onSelectThread, activeThreadId, + isPremium, children, }: ResizableChatSidebarProps) {<ChatSidebar isOpen={isOpen} onOpenChange={onOpenChange} onNewChat={onNewChat} onSelectThread={onSelectThread} activeThreadId={activeThreadId} + isPremium={isPremium} />
🤖 Fix all issues with AI agents
In `@components/features/article-chat.tsx`:
- Around line 223-228: The effect that calls onMessagesChange on every messages
update is causing excessive localStorage writes; modify the useEffect (the
effect using messages and onMessagesChange) so it only calls onMessagesChange
when streaming has finished (e.g., when chat.status === 'idle' or similar
finished state) or wrap the call in a debounce before invoking onMessagesChange;
locate the effect around useEffect(...) that references messages and
onMessagesChange in components/features/article-chat.tsx and either add a
condition checking chat.status (or response streaming flag) before calling
onMessagesChange?.(messages) or debounce the onMessagesChange invocation to
limit frequency.
In `@server/routes/chat-threads.ts`:
- Around line 83-112: The DELETE handler for removing a thread (the async route
using params.threadId) does a non-atomic redis.get → modify → redis.set on the
same key produced by getThreadsKey(authInfo.userId), causing a TOCTOU race with
the POST/save-all path that can overwrite newer state; change the delete logic
to perform the update atomically using Redis optimistic locking or a Lua script:
use redis.WATCH on the key, re-read/decompress with decompressAsync, filter out
the threadId, then MULTI/SET the recompressed value (compressAsync) with
CHAT_THREADS_TTL and EXEC, retry a few times on WATCH failures, or replace the
whole sequence with a single EVAL Lua script that reads, filters, and writes
atomically; ensure errors still set status/return like existing catch.
🧹 Nitpick comments (9)
lib/hooks/use-chat.ts (1)
226-243: Sync effect doesn't gate onisPremium, which may cause a subtle edge case.If a premium user fetches server messages and then downgrades, the React Query cache may still hold
serverMessageswithserverFetchComplete === true. This effect would still sync those stale messages into chat state since it only checksisSignedIn, notisPremium. Additionally, the reset effect (lines 241-243) resetshasSyncedRefonisSignedInchange but not onisPremiumchange.In practice this is unlikely to be a real problem (sync runs once, and showing prior messages after downgrade is arguably reasonable UX), but adding
isPremiumto the guard on line 228 and the reset deps on line 243 would make the intent explicit.Suggested fix
- if (!isSignedIn || !serverFetchComplete || hasSyncedRef.current || isLoadingHistory) return; + if (!isSignedIn || !isPremium || !serverFetchComplete || hasSyncedRef.current || isLoadingHistory) return;- }, [serverMessages, serverFetchComplete, isSignedIn, isLoadingHistory, chat]); + }, [serverMessages, serverFetchComplete, isSignedIn, isPremium, isLoadingHistory, chat]);- }, [articleHash, isSignedIn]); + }, [articleHash, isSignedIn, isPremium]);server/routes/chat-threads.ts (2)
74-78: Loose body validation —t.Array(t.Any())allows arbitrary data to be persisted.This means any JSON array will be accepted and stored in Redis, including malformed or oversized payloads. Consider adding at least a basic shape validation (e.g.,
t.Object({ id: t.String(), title: t.String(), ... })) and/or a max array length to prevent abuse.
23-46: GET error handler returns 200 with empty threads, masking failures.On line 44, a Redis error returns
{ threads: [] }with a 200 status. While this prevents client-side breakage, it makes it impossible for the caller to distinguish between "no threads" and "load failed." Consider logging distinctly and optionally returning a different status or an error flag so the client can show a retry prompt.lib/hooks/use-chat-threads.ts (3)
267-285: Redundant server operations on delete — bothsaveThreads(debounced POST) anddeleteMutation(DELETE) fire.
saveThreads(updated)inside the updater queues a debounced POST of the full thread list (already without the deleted thread), anddeleteMutation.mutate(id)sends a targeted DELETE. Both achieve the same result on the server. The DELETE endpoint also does its own read-modify-write, which adds another Redis round-trip.Consider removing either the targeted DELETE call or skipping the server save in
saveThreadswhen a delete is in flight. This would reduce unnecessary server load and eliminate the race window between the two operations noted in the server route review.
228-250: Side effects insidesetStateupdater function.
saveThreads(updated)is called insidesetThreads((prev) => { ... }). This writes to localStorage, dispatches custom events, and may trigger async server mutations — all side effects that shouldn't be in a state updater (which React may invoke more than once in StrictMode or concurrent features).A safer pattern is to compute the new state in the updater and persist after:
Illustrative refactor for createThread
const createThread = useCallback( (title?: string): ChatThread => { const now = new Date().toISOString(); const newThread: ChatThread = { ... }; - setThreads((prev) => { - const updated = [newThread, ...prev]; - saveThreads(updated); - return updated; - }); + setThreads((prev) => [newThread, ...prev]); + // Persist outside the updater + // (use a ref or effect to get the latest threads for saving) setActiveThreadId(newThread.id); return newThread; }, [saveThreads] );This pattern applies to
updateThread,deleteThread, andtogglePinas well.
136-173: Misleading condition —!isPremiumis always false at line 168.At line 138,
if (!isPremium)returns early, so code below executes only whenisPremiumistrue. The condition on line 168 (if (!isSignedIn || !isPremium)) could be simplified toif (!isSignedIn)for clarity.Suggested fix
- if (!isSignedIn || !isPremium) { + if (!isSignedIn) { setIsLoaded(true); }components/features/article-chat.tsx (1)
107-108: Two distinctisPremiumvalues coexist — naming may confuse future readers.
isPremiumProp(from parent billing state) controls hook persistence behavior, whileisPremium(line 123, from server usage response) drives UI elements. Consider renaming the local one to something likeisPremiumFromUsageorserverIsPremiumto make the distinction clearer.Also applies to: 123-123
components/features/proxy-content.tsx (2)
482-497: Synthetic message IDs on thread load will trigger a redundant save cycle.When loading a thread, messages are reconstructed with IDs like
${threadId}-${i}, which differ from the original IDs. This causes React to see "new" messages, firing theonMessagesChangeeffect inarticle-chat.tsx, which callshandleMessagesChange→updateThread— re-saving identical content to localStorage and queuing a debounced server POST.This is not harmful (same data) but wasteful. To avoid it, you could either:
- Store original message IDs in the thread and restore them, or
- Add a guard in
handleMessagesChangeto skip updates when content hasn't actually changed (e.g., compare serialized messages against the current thread's messages).
499-522: Auto-create thread on first message works but creates an initially empty thread.When
currentThreadIdis null,createThread(title)is called, which stores a thread with emptymessages: []. The actual messages are only associated on the next render cycle whencurrentThreadIdupdates andupdateThreadis called. This brief inconsistency is functionally benign but means the thread briefly exists without its messages in both localStorage and the debounced server queue.Consider passing the messages into the thread at creation time to avoid this:
Suggested improvement
if (currentThreadId) { updateThread(currentThreadId, { messages: threadMessages, title }); } else { // Auto-create thread on first message - createThread(title); + const newThread = createThread(title); + updateThread(newThread.id, { messages: threadMessages }); }
| // Notify parent when messages change (for thread syncing) | ||
| useEffect(() => { | ||
| if (messages.length > 0) { | ||
| onMessagesChange?.(messages); | ||
| } | ||
| }, [messages, onMessagesChange]); |
There was a problem hiding this comment.
onMessagesChange fires on every streaming token, causing high-frequency localStorage writes.
During AI response streaming, messages changes with every token (~20 times/sec given the 50ms throttle). Each change triggers this effect, which calls onMessagesChange → handleMessagesChange in proxy-content → updateThread → saveThreads, which writes to localStorage synchronously on each call. The server save is debounced (1s), but the localStorage writes are not.
Consider debouncing the onMessagesChange callback here or in the parent, or only calling it when chat.status is idle (i.e., after streaming completes).
Example: notify only after streaming completes
+ const isLoading = chat.status === "streaming" || chat.status === "submitted";
// Notify parent when messages change (for thread syncing)
useEffect(() => {
- if (messages.length > 0) {
+ if (messages.length > 0 && !isLoading) {
onMessagesChange?.(messages);
}
- }, [messages, onMessagesChange]);
+ }, [messages, onMessagesChange, isLoading]);🤖 Prompt for AI Agents
In `@components/features/article-chat.tsx` around lines 223 - 228, The effect that
calls onMessagesChange on every messages update is causing excessive
localStorage writes; modify the useEffect (the effect using messages and
onMessagesChange) so it only calls onMessagesChange when streaming has finished
(e.g., when chat.status === 'idle' or similar finished state) or wrap the call
in a debounce before invoking onMessagesChange; locate the effect around
useEffect(...) that references messages and onMessagesChange in
components/features/article-chat.tsx and either add a condition checking
chat.status (or response streaming flag) before calling
onMessagesChange?.(messages) or debounce the onMessagesChange invocation to
limit frequency.
| .delete( | ||
| "/chat-threads/:threadId", | ||
| async ({ params, request, set }) => { | ||
| const { threadId } = params; | ||
|
|
||
| const authInfo = await getAuthInfo(request); | ||
| if (!authInfo.userId || !authInfo.isPremium) { | ||
| set.status = 401; | ||
| return { error: "Unauthorized", success: false }; | ||
| } | ||
|
|
||
| const key = getThreadsKey(authInfo.userId); | ||
|
|
||
| try { | ||
| const compressed = await redis.get(key); | ||
| if (!compressed) { | ||
| return { success: true }; | ||
| } | ||
|
|
||
| const threads = (await decompressAsync(compressed)) as Array<{ id: string }>; | ||
| const filtered = (threads || []).filter((t) => t.id !== threadId); | ||
| const recompressed = await compressAsync(filtered); | ||
| await redis.set(key, recompressed, { ex: CHAT_THREADS_TTL }); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| console.error("[chat-threads] Error deleting:", error); | ||
| set.status = 500; | ||
| return { error: "Failed to delete", success: false }; | ||
| } | ||
| }, |
There was a problem hiding this comment.
TOCTOU race between DELETE and concurrent POST (save-all).
The DELETE handler performs a non-atomic read → filter → write cycle on the same Redis key that POST overwrites wholesale. If a debounced POST lands between the redis.get and the redis.set in DELETE, the POST's full thread list is silently overwritten with stale data minus the deleted thread — losing any thread updates from the POST.
In practice, the 1-second client-side debounce and typical single-user access make this unlikely, but it's worth being aware of. A Redis transaction (WATCH/MULTI) or Lua script would eliminate the window.
🤖 Prompt for AI Agents
In `@server/routes/chat-threads.ts` around lines 83 - 112, The DELETE handler for
removing a thread (the async route using params.threadId) does a non-atomic
redis.get → modify → redis.set on the same key produced by
getThreadsKey(authInfo.userId), causing a TOCTOU race with the POST/save-all
path that can overwrite newer state; change the delete logic to perform the
update atomically using Redis optimistic locking or a Lua script: use
redis.WATCH on the key, re-read/decompress with decompressAsync, filter out the
threadId, then MULTI/SET the recompressed value (compressAsync) with
CHAT_THREADS_TTL and EXEC, retry a few times on WATCH failures, or replace the
whole sequence with a single EVAL Lua script that reads, filters, and writes
atomically; ensure errors still set status/return like existing catch.
2 participants
This PR is a draft and needs further review/testing before merging.
Summary
TODO before merge
🤖 Generated with Claude Code
Summary by CodeRabbit