Studio: Fix empty chat threads on navigation and stabilize new chat flow#10
Studio: Fix empty chat threads on navigation and stabilize new chat flow#10danielhanchen wants to merge 8 commits into
Conversation
|
/gemini review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors chat thread management to prevent the creation of empty threads in the database. Key changes include initializing the chat view from the existing active thread, adding a check for messages before starting a new thread, and filtering the sidebar to only show non-empty, non-archived threads. Review feedback indicates that the synchronization logic for the active thread ID in ChatRuntimeProvider should be simplified; specifically, ActiveThreadSync should remain enabled for all single-chat views to maintain global state consistency, and a redundant useEffect in ThreadAutoSwitch should be removed to prevent potential UI flickering.
| <AssistantRuntimeProvider runtime={runtime} aui={aui}> | ||
| <ActiveThreadSync enabled={modelType === "base" && !pairId && !newThreadNonce} /> | ||
| <ActiveThreadSync | ||
| enabled={modelType === "base" && !pairId && !newThreadNonce && !initialThreadId} |
There was a problem hiding this comment.
Disabling ActiveThreadSync when newThreadNonce or initialThreadId is present prevents the global store from syncing the activeThreadId once the thread is actually initialized or switched to. For a 'New Chat', the thread is only persisted after the first message is sent; at that point, mainThreadId will be set by the runtime, but the store will remain at null because sync is disabled here. This breaks features that rely on the store's activeThreadId, such as the context usage bar. Keeping sync enabled for all base single-chat views ensures the store stays updated as soon as the runtime identifies the active thread.
| enabled={modelType === "base" && !pairId && !newThreadNonce && !initialThreadId} | |
| enabled={modelType === "base" && !pairId} |
| useEffect(() => { | ||
| if (isLoading || mainThreadId !== threadId) { | ||
| return; | ||
| } | ||
| useChatRuntimeStore.getState().setActiveThreadId(threadId); | ||
| }, [isLoading, mainThreadId, threadId]); |
There was a problem hiding this comment.
This useEffect in ThreadAutoSwitch is redundant if ActiveThreadSync is enabled for all single-chat views. Additionally, calling setActiveThreadId unconditionally whenever isLoading or mainThreadId changes can be problematic because setActiveThreadId in the store (line 290 of chat-runtime-store.ts) clears contextUsage. This might lead to the token usage bar flickering or disappearing during state transitions even if the thread hasn't changed. It is recommended to rely on a single, properly configured ActiveThreadSync component and remove this redundant effect.
|
/gemini review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request refactors chat thread initialization and persistence to prevent the creation of empty database entries until a message is sent. It introduces logic to promote in-memory chat views to persisted threads and updates the sidebar to filter out empty or archived threads. Feedback suggests broadening the state promotion logic to ensure the UI correctly highlights the active thread even when a conversation starts without an explicit "New Chat" action.
| if (view.mode !== "single" || !view.newThreadNonce || !activeThreadId) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The promotion logic for the chat view is currently restricted to cases where a newThreadNonce is present. This means if a user starts a conversation from the initial empty state (e.g., by navigating to /chat and typing a message without clicking "New Chat" first), the view state won't be updated with the new threadId. This will prevent the sidebar from highlighting the active thread because activeId (derived from view.threadId) will remain undefined.
Updating the condition to allow promotion whenever activeThreadId is set and differs from the current view.threadId ensures the UI stays in sync regardless of how the chat was initiated.
| if (view.mode !== "single" || !view.newThreadNonce || !activeThreadId) { | |
| return; | |
| } | |
| if (view.mode !== "single" || !activeThreadId || activeThreadId === view.threadId) { | |
| return; | |
| } |
|
/gemini review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request refactors the chat thread initialization process to ensure that empty threads are not persisted in the database until the first message is sent. It introduces logic to manage transient chat views, updates the runtime provider to delay thread creation, and modifies the thread sidebar to filter out empty or archived threads. The review feedback recommends using functional state updates in the chat page to preserve nonces and prevent UI remounts, and suggests optimizing the thread sidebar's database query by using indexed lookups to improve performance.
| if (activeThreadId.startsWith("__LOCALID_")) return; | ||
| // Already tracking the correct thread. | ||
| if (view.threadId === activeThreadId) return; | ||
| setView({ mode: "single", threadId: activeThreadId }); |
There was a problem hiding this comment.
When promoting a transient view to a persisted one, it is recommended to use a functional update and preserve the existing state (specifically the newThreadNonce). Although the key for the SingleContent component is not visible in this diff hunk, it typically relies on either the threadId or the newThreadNonce for stability. If the nonce is lost here while the threadId is being assigned, it could trigger an unnecessary remount of the entire chat interface during the first message generation, which can interrupt the assistant's response stream.
| setView({ mode: "single", threadId: activeThreadId }); | |
| setView((prev) => (prev.mode === "single" ? { ...prev, threadId: activeThreadId } : prev)); |
| const allThreads = useLiveQuery(async () => { | ||
| const threadIdsWithMessage = new Set( | ||
| (await db.messages.orderBy("threadId").uniqueKeys()) as string[], | ||
| ); | ||
| const rows = await db.threads.orderBy("createdAt").reverse().toArray(); | ||
| return rows.filter((t) => !t.archived && threadIdsWithMessage.has(t.id)); | ||
| }, []); |
There was a problem hiding this comment.
The current implementation fetches all threads from the database and then filters them in memory. This can become a performance bottleneck as the number of threads grows. A more efficient approach is to use the threadIdsWithMessage set to fetch only the relevant threads using the anyOf operator on the primary key index, and then apply the archived filter and sorting.
| const allThreads = useLiveQuery(async () => { | |
| const threadIdsWithMessage = new Set( | |
| (await db.messages.orderBy("threadId").uniqueKeys()) as string[], | |
| ); | |
| const rows = await db.threads.orderBy("createdAt").reverse().toArray(); | |
| return rows.filter((t) => !t.archived && threadIdsWithMessage.has(t.id)); | |
| }, []); | |
| const allThreads = useLiveQuery(async () => { | |
| const threadIds = (await db.messages.orderBy("threadId").uniqueKeys()) as string[]; | |
| const rows = await db.threads.where("id").anyOf(threadIds).toArray(); | |
| return rows | |
| .filter((t) => !t.archived) | |
| .sort((a, b) => b.createdAt - a.createdAt); | |
| }, []); |
|
/gemini review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request refactors the chat thread initialization and persistence logic to prevent the creation of empty "New Chat" records in the database. Key changes include delaying thread persistence until the first message is sent, updating the sidebar to filter out threads without messages, and refining state management to avoid spurious thread creation during navigation. Feedback suggests that when deleting the currently active thread, the implementation should trigger the full 'New Chat' flow to ensure the UI and runtime are properly reset via a new nonce.
| if (activeId === item.id) { | ||
| useChatRuntimeStore.getState().setActiveThreadId(null); | ||
| onSelect({ mode: "single" }); | ||
| } |
There was a problem hiding this comment.
When deleting the currently active thread, it is recommended to trigger the full 'New Chat' flow by calling onNewThread(). This ensures a new newThreadNonce is generated, which correctly triggers the ThreadNewChatSwitch component to reset the chat runtime and clear the composer. The current implementation only resets the view mode, which might leave the UI in a stale state if the runtime isn't explicitly told to switch to a new thread.
if (activeId === item.id) {
onNewThread();
}
…er99/unsloth into fix/new-chat-page-switch
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors chat thread initialization and synchronization to prevent the creation of empty "New Chat" entries and improve state management. Key changes include delaying thread persistence until the first message is sent, introducing a context for active thread synchronization, and filtering the thread sidebar to only show non-archived threads with messages. Feedback was provided regarding a potential performance bottleneck in the thread sidebar's database query, which currently performs in-memory filtering.
| const allThreads = useLiveQuery(async () => { | ||
| const threadIdsWithMessage = new Set( | ||
| (await db.messages.orderBy("threadId").uniqueKeys()) as string[], | ||
| ); | ||
| const rows = await db.threads.orderBy("createdAt").reverse().toArray(); | ||
| return rows.filter((t) => !t.archived && threadIdsWithMessage.has(t.id)); | ||
| }, []); |
There was a problem hiding this comment.
The current implementation of useLiveQuery performs two separate database queries and then filters the results in memory. While functional, this could become a performance bottleneck as the number of threads and messages grows. Consider optimizing this by using a more targeted Dexie query or by adding a messageCount field to the threads table to allow for direct filtering at the database level.
danielhanchen
force-pushed
the
pr-4872-head
branch
from
deedaa5 to
cb41fc0
Compare
|
/gemini review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request refactors chat thread initialization and synchronization, introducing a syncActiveThreadId flag to prevent compare panes from overwriting the global active thread state. It also defers thread persistence until the first message is sent and filters archived threads in the sidebar. Feedback indicates that the message-check logic in handleNewThread is redundant and may cause a UX regression by preventing the composer from resetting. Additionally, filtering archived threads at the query level in the sidebar makes existing checks in the groupThreads function redundant.
| const handleNewThread = useCallback(() => { | ||
| useChatRuntimeStore.getState().setActiveThreadId(null); | ||
| setView({ mode: "single", newThreadNonce: crypto.randomUUID() }); | ||
| }, []); | ||
| void (async () => { | ||
| if (view.mode === "single") { | ||
| const currentThreadId = view.threadId ?? activeThreadId; | ||
| if (!currentThreadId) { | ||
| // Already at a fresh/unsaved chat state. | ||
| return; | ||
| } | ||
| try { | ||
| const hasMessages = !!(await db.messages | ||
| .where("threadId") | ||
| .equals(currentThreadId) | ||
| .first()); | ||
| if (!hasMessages) { | ||
| return; | ||
| } | ||
| } catch { | ||
| // allow explicit new chat if Dexie fails | ||
| } | ||
| } | ||
| useChatRuntimeStore.getState().setActiveThreadId(null); | ||
| setView({ mode: "single", newThreadNonce: crypto.randomUUID() }); | ||
| })(); | ||
| }, [activeThreadId, view]); |
There was a problem hiding this comment.
The added logic in handleNewThread to check for existing messages is redundant and introduces a UX regression.
- Redundancy: The goal of preventing empty threads in the database is already addressed by the changes in
ThreadNewChatSwitch(inruntime-provider.tsx), which now avoids callinginitialize()until the first message is appended. - UX Regression: This check prevents the "New Chat" button from resetting the composer state (clearing unsent text) when the user is in an unpersisted or empty thread. Users often rely on "New Chat" to quickly clear their current input and start fresh.
Since empty threads are no longer persisted automatically, it is better to allow the "New Chat" action to always proceed, ensuring the UI can be reset consistently.
const handleNewThread = useCallback(() => {
useChatRuntimeStore.getState().setActiveThreadId(null);
setView({ mode: "single", newThreadNonce: crypto.randomUUID() });
}, []);
| () => db.threads.orderBy("createdAt").reverse().toArray(), | ||
| async () => { | ||
| const rows = await db.threads.orderBy("createdAt").reverse().toArray(); | ||
| return rows.filter((t) => !t.archived); |
There was a problem hiding this comment.
Filtering archived threads here makes the identical check inside the groupThreads function (line 39) redundant. While filtering at the query level is more efficient, you should remove the redundant check from groupThreads to maintain code clarity and avoid confusion about where the filtering logic resides.
…lete flow - Remove __LOCALID_ filter from getInitialSingleChatView: in this Dexie-backed adapter, AUI's __LOCALID_ prefixed IDs ARE the real persistent thread IDs stored by initialize(). Filtering them out breaks thread restoration on navigation. - Simplify handleNewThread to synchronous: the async Dexie message check is redundant (persistence is already deferred to first append) and strands users on legacy empty threads. Use a simple guard that checks the store's activeThreadId to detect unsent drafts. - Add message-count filter to sidebar: filter threads to only show those with at least one message, hiding legacy empty threads. - Add store-based sidebar highlighting fallback: use activeThreadId from the store when view.threadId is not set (nonce-backed chats). - Fix handleDelete to call onNewThread() instead of onSelect(), and clear activeThreadId, so the runtime properly resets after deleting the active thread.
|
/gemini review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request refactors chat thread management and synchronization. Key changes include syncing the active thread ID via a global store, delaying thread persistence until the first message is sent, and filtering empty threads from the sidebar. Feedback was provided regarding the performance of the sidebar's thread filtering logic, suggesting a more scalable approach to identify threads with messages.
| const allThreads = useLiveQuery(async () => { | ||
| const threadIdsWithMessage = new Set( | ||
| (await db.messages.orderBy("threadId").uniqueKeys()) as string[], | ||
| ); | ||
| const rows = await db.threads.orderBy("createdAt").reverse().toArray(); | ||
| return rows.filter((t) => !t.archived && threadIdsWithMessage.has(t.id)); | ||
| }, []); |
There was a problem hiding this comment.
The useLiveQuery implementation for filtering empty threads uses db.messages.orderBy("threadId").uniqueKeys(). While efficient for small to medium datasets due to the index on threadId, this operation's performance will degrade as the total number of messages in the database grows, as it requires an index scan to find unique keys.
Consider adding a messageCount or hasMessages boolean field to the threads table and updating it during the first message append. This would allow for a much more scalable query directly on the threads table without involving the messages table at all.
handleDelete was calling onNewThread() after clearing activeThreadId, but the handleNewThread guard sees !view.threadId && !activeThreadId and returns early, leaving the UI stuck on the deleted thread. Fix by directly calling onSelect with a new nonce instead. Restore __LOCALID_ filter in getInitialSingleChatView to prevent restoring unpersisted AUI local thread IDs on navigation. Without this filter, navigating away from /chat before sending a message would restore a non-existent thread that Dexie cannot fetch.
|
/gemini review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request refines thread management and state synchronization within the chat interface. Key improvements include delaying thread persistence until the first message is sent to prevent empty threads, synchronizing the active thread ID via a central store, and filtering the sidebar to display only non-archived threads with messages. Feedback was provided regarding the efficiency of the database query used in the sidebar, suggesting a more performant approach for filtering threads directly within the query.
| const allThreads = useLiveQuery(async () => { | ||
| const threadIdsWithMessage = new Set( | ||
| (await db.messages.orderBy("threadId").uniqueKeys()) as string[], | ||
| ); | ||
| const rows = await db.threads.orderBy("createdAt").reverse().toArray(); | ||
| return rows.filter((t) => !t.archived && threadIdsWithMessage.has(t.id)); | ||
| }, []); |
There was a problem hiding this comment.
The current implementation of useLiveQuery to fetch threads could be inefficient if there are many threads stored in the database. It fetches all threads and then filters them in JavaScript. A more performant approach would be to fetch only the threads that have messages directly from the database using where...anyOf.
| const allThreads = useLiveQuery(async () => { | |
| const threadIdsWithMessage = new Set( | |
| (await db.messages.orderBy("threadId").uniqueKeys()) as string[], | |
| ); | |
| const rows = await db.threads.orderBy("createdAt").reverse().toArray(); | |
| return rows.filter((t) => !t.archived && threadIdsWithMessage.has(t.id)); | |
| }, []); | |
| const allThreads = useLiveQuery(async () => { | |
| const threadIdsWithMessage = (await db.messages | |
| .orderBy("threadId") | |
| .uniqueKeys()) as string[]; | |
| const threads = await db.threads | |
| .where("id") | |
| .anyOf(threadIdsWithMessage) | |
| .and((t) => !t.archived) | |
| .toArray(); | |
| return threads.sort((a, b) => b.createdAt - a.createdAt); | |
| }, []); |
|
/gemini review |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request refactors chat thread management to prevent the creation of empty threads by delaying persistence until the first message is sent and syncing the active thread state through a global store. The sidebar now filters for threads containing messages. Feedback indicates that a new guard in the 'New Chat' handler causes a UX regression by preventing the composer from clearing when a user is on an unsaved draft; it is recommended to remove this guard to ensure the button always provides a fresh start.
| const handleNewThread = useCallback(() => { | ||
| // Skip if we are already on a fresh unsaved draft with no messages sent. | ||
| // Once the user sends a message, append() sets activeThreadId in the store, | ||
| // so we check the store to know whether the current draft has been sent. | ||
| if ( | ||
| view.mode === "single" && | ||
| !view.threadId && | ||
| !useChatRuntimeStore.getState().activeThreadId | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| useChatRuntimeStore.getState().setActiveThreadId(null); | ||
| setView({ mode: "single", newThreadNonce: crypto.randomUUID() }); | ||
| }, []); | ||
| }, [view]); |
There was a problem hiding this comment.
The guard added to handleNewThread prevents the "New Chat" action from resetting the composer state if the user is already on an unsaved draft (i.e., no messages sent yet). This means clicking "New Chat" won't clear any typed text or attachments in the current draft, which is a regression in user experience.
Since new threads are no longer persisted until the first message is sent (as per the changes in runtime-provider.tsx), there is no risk of cluttering the database with empty threads by allowing the state to reset. Removing this guard ensures that the "New Chat" button always provides a fresh start as expected.
const handleNewThread = useCallback(() => {
useChatRuntimeStore.getState().setActiveThreadId(null);
setView({ mode: "single", newThreadNonce: crypto.randomUUID() });
}, []);
…params + cancel race
Round 5 of parallel-reviewer aggregation surfaced six additional
findings; five are real and fixed here:
1. Hyphenated MCP parameter names (`<parameter=issue-number>`) were
dropped by the XML parser's `\w+` regex. Extended to `[\w-]+` in
both core/inference/tool_call_parser.py and core/tool_healing.py.
The latter is GGUF's own copy of the parser/strip patterns and was
missed by round 4.
2. core/tool_healing.py's `strip_tool_call_markup` still used
`<function=\w+>` so hyphenated MCP tool-call XML leaked into the
GGUF visible content even after round 4 fixed the shared parser.
3+4. `mcp_enabled` re-opened the tool loop even when the operator
passed `unsloth run --disable-tools` (CLI policy False). Round 2's
`(_tools_on or payload.mcp_enabled)` gate ignored the raw process
policy. Now reads `state.tool_policy.get_tool_policy()` and gates
mcp_enabled on `_cli_policy is not False`. Applied to both GGUF
and safetensors paths.
5. GGUF's agentic loop called `execute_tool(tool_name, ...)` without
checking the model-emitted name against the per-request tool list,
while the safetensors loop already enforces this. Added the same
allow-list check so a model that hallucinates a filtered MCP name
or a built-in the caller opted out of returns "not enabled" instead
of executing.
Bonus P2 fixes:
- `call_tool_sync` now checks `cancel_event.is_set()` BEFORE
creating the call task, so a pre-set cancellation does not open
the HTTP transport.
- `clear_oauth_tokens_async` moved the OAuth import + construction
inside the protected try block; a fastmcp.client.auth load error
used to escape and 500 the delete / update route.
NOT fixed (verified false or out of scope):
- finding #10 "structured_content vs structuredContent": fastmcp's
CallToolResult dataclass uses snake_case (verified live against
structured-only tool result; fields are
`dict_keys(['content', 'structured_content', 'meta', 'data', 'is_error'])`).
- finding #11 "asyncio.run from running loop": call_tool_sync is
invoked from `asyncio.to_thread` worker threads which have no
event loop; asyncio.run() is safe there.
Tests added (37 total): hyphenated param names, tool_healing strip,
GGUF allow-list gate, cancel pre-set short-circuit, OAuth cleanup
constructor-error swallowing. 1721 passed locally, no regressions.
2 participants
Staging mirror of unslothai#4872
Original PR: unslothai#4872
Author: Imagineer99
This is a staging copy for review and editing. Once finalized, changes will be pushed back to the original PR.
Original description
Switching tabs could create extra empty “New Chat” entries, cluttering the thread list and confusing thread state.
New chat creation now aligns with standard chat UI behaviour: a new conversation is only persisted once the user actually sends a message, not on navigation or empty state transitions.