refactor(threads): rust core single source of truth, drop redux thread slice#1199
Closed
senamakel wants to merge 1 commit into
Closed
refactor(threads): rust core single source of truth, drop redux thread slice#1199senamakel wants to merge 1 commit into
senamakel wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR removes the Redux ChangesFrontend: Thread State Migration (single DAG)
Backend: Agent/session transcript scoping (independent DAG)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/providers/__tests__/ChatRuntimeProvider.test.tsx (1)
80-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset
window.location.hashin the shared test setup.Multiple cases below mutate the global hash, and most of them leave it behind. Because proactive resolution reads
window.location.hashdirectly, that makes later tests depend on execution order.🧪 Minimal stabilization
-import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; ... beforeEach(() => { vi.clearAllMocks(); mockNotifyThreadMessagesRefresh.mockClear(); resetRuntimeState(); + window.location.hash = '#/conversations'; vi.mocked(threadApi.appendMessage).mockImplementation(async (_tid, msg) => msg); vi.mocked(threadApi.getThreads).mockResolvedValue({ threads: [], count: 0 }); vi.mocked(threadApi.generateTitleIfNeeded).mockResolvedValue({ id: 'tid', title: 'new', } as never); }); + + afterEach(() => { + window.location.hash = '#/conversations'; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/providers/__tests__/ChatRuntimeProvider.test.tsx` around lines 80 - 90, The test setup in beforeEach of ChatRuntimeProvider.test.tsx doesn't reset window.location.hash, causing test order dependencies; update the beforeEach (the block that calls vi.clearAllMocks(), resetRuntimeState(), and mocks threadApi methods) to explicitly reset window.location.hash (e.g., set to empty string or default value) so each test starts with a clean hash state and proactive resolution won't be affected by previous tests.
🧹 Nitpick comments (2)
app/src/pages/onboarding/__tests__/OnboardingLayout.test.tsx (1)
175-184: ⚡ Quick winDrop this duplicate “no welcome thread” test case.
This assertion is already covered by the test at Line 141-150, so keeping both adds churn without increasing behavior coverage.
Proposed cleanup
- // [`#1123`] Old test — welcome thread in Redux state — replaced: - // The welcome thread is no longer stored in Redux (threadSlice removed). - it('does NOT call threadApi.createNewThread on completeAndExit (Joyride flow has no welcome thread)', async () => { - await setupLayout(); - - await act(async () => { - fireEvent.click(screen.getByTestId('complete-btn')); - }); - - // threadApi.createNewThread is not called — welcome agent flow is gone. - expect(mockCreateNewThreadArg).not.toHaveBeenCalled(); - });As per coding guidelines, “Prefer testing behavior over implementation details; use existing helpers … before adding new harness code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/onboarding/__tests__/OnboardingLayout.test.tsx` around lines 175 - 184, Remove the duplicate test "does NOT call threadApi.createNewThread on completeAndExit (Joyride flow has no welcome thread)" — delete the entire it(...) block that calls setupLayout(), fires complete-btn, and asserts mockCreateNewThreadArg not.toHaveBeenCalled(); the behavior is already covered by the earlier test (which uses setupLayout and the same mockCreateNewThreadArg), so remove this redundant test to avoid churn.app/src/lib/threads/__tests__/useThreadMessages.test.tsx (1)
191-207: ⚡ Quick winAdd a regression test for “deselect while fetch is in-flight”.
The current null-state test covers immediate clearing, but not the race where the prior fetch resolves afterward. A deferred
getThreadMessagespromise plusrerender({ tid: null })would lock in protection against stale repopulation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/threads/__tests__/useThreadMessages.test.tsx` around lines 191 - 207, The test 'clears state when threadId becomes null' needs to also cover the race where a pending fetch resolves after deselecting: change the mock for threadApi.getThreadMessages to return a deferred promise (store resolve/reject and resolve it after rerender({ tid: null })), call rerender to set tid to null while the promise is still pending, then resolve the promise and assert that useThreadMessages still has messages.length === 0, isLoading === false, and error === null; update references in the test to the useThreadMessages hook and the mocked threadApi.getThreadMessages to ensure the deferred promise behavior is used for this regression check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/lib/threads/ThreadsContext.tsx`:
- Around line 42-45: The doFetch function currently catches and swallows errors
so callers like create/remove/updateLabels/generateTitleIfNeeded that await
refresh() cannot detect failures; change doFetch to log the error but rethrow it
(or remove the try/catch) so refresh() rejects on failure, and ensure the
mounted-check logic around state updates (used in refresh) is explicit so we
only set state when still mounted; update references to doFetch and refresh in
ThreadsContext.tsx so callers receive the propagated error and UI reconciliation
can fail fast.
- Around line 52-60: The current refresh() coalescing returns the in-flight
promise (refreshInFlightRef) and skips any trailing fetch, so mid-flight
mutations can be missed; change refresh() to, when refreshInFlightRef.current
exists, set a pendingRefreshRef (e.g., pendingRefreshRef.current = true) and
attach logic to the in-flight promise's finally (or then) to, after it clears
refreshInFlightRef, check pendingRefreshRef and run doFetch() once more
(resetting pendingRefreshRef) so one trailing refresh is queued; keep
setIsLoading semantics and ensure refresh() still returns the original in-flight
promise while scheduling the trailing fetch.
In `@app/src/lib/threads/useThreadMessages.ts`:
- Around line 59-66: When threadId becomes falsy in the useEffect block,
increment the fetch sequence token used to validate in-flight responses (the
same sequence checked when resolving async fetches) so any pending responses are
treated as stale; in the branch that calls setMessages([]), setError(null), and
setIsLoading(false) also bump the sequence (e.g., increment fetchSequence or
fetchSequenceRef.current) to ensure old fetch handlers do not repopulate
messages.
In `@app/src/pages/Conversations.tsx`:
- Around line 234-254: The one-shot guard using threadListSettledRef inside the
useEffect can leave selectedThreadId stale when threads later become empty or
change; update the logic in the effect that watches threads (and any related
startup effect) to always revalidate selectedThreadId on every threads change:
if threads.length > 0 ensure selectedThreadId exists in threads and call
setSelectedThreadId(threads[0].id) when missing, and if threads.length === 0
either programmatically create a new thread (invoke the existing
create/thread-creation function) or set a clear "no-thread" state that disables
the composer; also audit handleSendMessage to proactively create a thread when
selectedThreadId is null before returning (reference: threadListSettledRef,
selectedThreadId, setSelectedThreadId, useEffect watching threads, and
handleSendMessage).
In `@app/src/providers/ChatRuntimeProvider.tsx`:
- Around line 599-615: After successfully appending the proactiveMsg via
threadApi.appendMessage (use the same success path that calls rtLog and
notifyThreadMessagesRefresh), trigger the thread-list refresh so the sidebar
metadata (lastMessageAt, messageCount, ordering) updates; specifically, after
await threadApi.appendMessage(targetThreadId, proactiveMsg) add a call to the
existing thread-list refresh dispatcher (the same event/function used elsewhere
to refresh the thread list) so the sidebar re-fetches; apply the same change to
the other identical block noted around lines 713-731 to ensure both proactive
and error-path JSONL appends refresh the thread list.
---
Outside diff comments:
In `@app/src/providers/__tests__/ChatRuntimeProvider.test.tsx`:
- Around line 80-90: The test setup in beforeEach of
ChatRuntimeProvider.test.tsx doesn't reset window.location.hash, causing test
order dependencies; update the beforeEach (the block that calls
vi.clearAllMocks(), resetRuntimeState(), and mocks threadApi methods) to
explicitly reset window.location.hash (e.g., set to empty string or default
value) so each test starts with a clean hash state and proactive resolution
won't be affected by previous tests.
---
Nitpick comments:
In `@app/src/lib/threads/__tests__/useThreadMessages.test.tsx`:
- Around line 191-207: The test 'clears state when threadId becomes null' needs
to also cover the race where a pending fetch resolves after deselecting: change
the mock for threadApi.getThreadMessages to return a deferred promise (store
resolve/reject and resolve it after rerender({ tid: null })), call rerender to
set tid to null while the promise is still pending, then resolve the promise and
assert that useThreadMessages still has messages.length === 0, isLoading ===
false, and error === null; update references in the test to the
useThreadMessages hook and the mocked threadApi.getThreadMessages to ensure the
deferred promise behavior is used for this regression check.
In `@app/src/pages/onboarding/__tests__/OnboardingLayout.test.tsx`:
- Around line 175-184: Remove the duplicate test "does NOT call
threadApi.createNewThread on completeAndExit (Joyride flow has no welcome
thread)" — delete the entire it(...) block that calls setupLayout(), fires
complete-btn, and asserts mockCreateNewThreadArg not.toHaveBeenCalled(); the
behavior is already covered by the earlier test (which uses setupLayout and the
same mockCreateNewThreadArg), so remove this redundant test to avoid churn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a2d15a1-8eb2-497d-baee-930f0ea04259
📒 Files selected for processing (21)
app/src/App.tsxapp/src/lib/threads/ThreadsContext.tsxapp/src/lib/threads/__tests__/ThreadsContext.test.tsxapp/src/lib/threads/__tests__/messagesRefreshBus.test.tsapp/src/lib/threads/__tests__/useSelectedThread.test.tsxapp/src/lib/threads/__tests__/useThreadMessages.test.tsxapp/src/lib/threads/messagesRefreshBus.tsapp/src/lib/threads/useActiveThread.tsxapp/src/lib/threads/useSelectedThread.tsapp/src/lib/threads/useThreadMessages.tsapp/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.render.test.tsxapp/src/pages/conversations/components/WorkerThreadRefCard.tsxapp/src/pages/onboarding/__tests__/OnboardingLayout.test.tsxapp/src/providers/ChatRuntimeProvider.tsxapp/src/providers/CoreStateProvider.tsxapp/src/providers/__tests__/ChatRuntimeProvider.test.tsxapp/src/providers/__tests__/CoreStateProvider.identityFlip.test.tsxapp/src/store/__tests__/threadSlice.test.tsapp/src/store/index.tsapp/src/store/threadSlice.ts
💤 Files with no reviewable changes (3)
- app/src/store/threadSlice.ts
- app/src/store/tests/threadSlice.test.ts
- app/src/store/index.ts
Comment on lines
+42
to
+45
| } catch (err) { | ||
| log('[threads-ctx] fetch error %O', err); | ||
| console.warn('[threads-ctx] getThreads failed:', err); | ||
| } finally { |
Contributor
There was a problem hiding this comment.
doFetch swallows errors, so callers can’t detect failed reconciliation.
create/remove/updateLabels/generateTitleIfNeeded await refresh(), but doFetch catches and resolves. That can leave UI stale while mutation flows still appear successful.
💡 Proposed fix (propagate refresh failure; handle mount explicitly)
const doFetch = useCallback(async () => {
log('[threads-ctx] fetch start');
try {
const data = await threadApi.getThreads();
log('[threads-ctx] fetch done count=%d', data.threads.length);
setThreads(data.threads);
} catch (err) {
log('[threads-ctx] fetch error %O', err);
console.warn('[threads-ctx] getThreads failed:', err);
+ throw err;
} finally {
setIsLoading(false);
}
}, []);
useEffect(() => {
log('[threads-ctx] mount — initial fetch');
- void refresh();
+ void refresh().catch(() => {
+ // already logged in doFetch
+ });
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);Also applies to: 64-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/lib/threads/ThreadsContext.tsx` around lines 42 - 45, The doFetch
function currently catches and swallows errors so callers like
create/remove/updateLabels/generateTitleIfNeeded that await refresh() cannot
detect failures; change doFetch to log the error but rethrow it (or remove the
try/catch) so refresh() rejects on failure, and ensure the mounted-check logic
around state updates (used in refresh) is explicit so we only set state when
still mounted; update references to doFetch and refresh in ThreadsContext.tsx so
callers receive the propagated error and UI reconciliation can fail fast.
Comment on lines
+52
to
+60
| if (refreshInFlightRef.current) { | ||
| return refreshInFlightRef.current; | ||
| } | ||
| setIsLoading(true); | ||
| const promise = doFetch().finally(() => { | ||
| refreshInFlightRef.current = null; | ||
| }); | ||
| refreshInFlightRef.current = promise; | ||
| return promise; |
Contributor
There was a problem hiding this comment.
Coalescing can return stale thread lists after mid-flight mutations.
If a second refresh() is requested while one is in flight, you return the old promise and skip a trailing fetch. A mutation that completes during that window can be missed until some later trigger.
💡 Proposed fix (queue one trailing refresh)
const refreshInFlightRef = useRef<Promise<void> | null>(null);
+ const refreshQueuedRef = useRef(false);
const refresh = useCallback(async () => {
// Coalesce concurrent refresh calls into a single in-flight request.
if (refreshInFlightRef.current) {
+ refreshQueuedRef.current = true;
return refreshInFlightRef.current;
}
- setIsLoading(true);
- const promise = doFetch().finally(() => {
- refreshInFlightRef.current = null;
- });
- refreshInFlightRef.current = promise;
- return promise;
+ do {
+ refreshQueuedRef.current = false;
+ setIsLoading(true);
+ const promise = doFetch().finally(() => {
+ refreshInFlightRef.current = null;
+ });
+ refreshInFlightRef.current = promise;
+ await promise;
+ } while (refreshQueuedRef.current);
}, [doFetch]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/lib/threads/ThreadsContext.tsx` around lines 52 - 60, The current
refresh() coalescing returns the in-flight promise (refreshInFlightRef) and
skips any trailing fetch, so mid-flight mutations can be missed; change
refresh() to, when refreshInFlightRef.current exists, set a pendingRefreshRef
(e.g., pendingRefreshRef.current = true) and attach logic to the in-flight
promise's finally (or then) to, after it clears refreshInFlightRef, check
pendingRefreshRef and run doFetch() once more (resetting pendingRefreshRef) so
one trailing refresh is queued; keep setIsLoading semantics and ensure refresh()
still returns the original in-flight promise while scheduling the trailing
fetch.
Comment on lines
+59
to
+66
| useEffect(() => { | ||
| if (!threadId) { | ||
| // Clear state when no thread is selected. | ||
| setMessages([]); | ||
| setError(null); | ||
| setIsLoading(false); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Increment fetch sequence when threadId becomes null to prevent stale repopulation.
If deselection happens mid-fetch, the in-flight response still matches the current sequence and can repopulate messages after you've cleared state. Bump the sequence in this branch too so old responses are discarded.
💡 Suggested fix
useEffect(() => {
if (!threadId) {
+ // Invalidate any in-flight request for the previously selected thread.
+ fetchSeqRef.current += 1;
// Clear state when no thread is selected.
setMessages([]);
setError(null);
setIsLoading(false);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (!threadId) { | |
| // Clear state when no thread is selected. | |
| setMessages([]); | |
| setError(null); | |
| setIsLoading(false); | |
| return; | |
| } | |
| useEffect(() => { | |
| if (!threadId) { | |
| // Invalidate any in-flight request for the previously selected thread. | |
| fetchSeqRef.current += 1; | |
| // Clear state when no thread is selected. | |
| setMessages([]); | |
| setError(null); | |
| setIsLoading(false); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/lib/threads/useThreadMessages.ts` around lines 59 - 66, When threadId
becomes falsy in the useEffect block, increment the fetch sequence token used to
validate in-flight responses (the same sequence checked when resolving async
fetches) so any pending responses are treated as stale; in the branch that calls
setMessages([]), setError(null), and setIsLoading(false) also bump the sequence
(e.g., increment fetchSequence or fetchSequenceRef.current) to ensure old fetch
handlers do not repopulate messages.
Comment on lines
+234
to
+254
| // Cold-boot resume: once the thread list is loaded, ensure a thread is selected. | ||
| // Priority: URL param `t=` (already read by useSelectedThread) > most-recent thread. | ||
| // If the URL has a valid t= that exists in the list, keep it. Otherwise fall back | ||
| // to the first thread (most recent after sort). If list is empty, create a new thread. | ||
| // useThreadMessages auto-fetches whenever selectedThreadId changes. | ||
| // | ||
| // NOTE: We only run this once when `threads` first becomes non-empty. | ||
| // Subsequent thread list refreshes (e.g. after delete) should not override | ||
| // a user's explicit selection — those are handled inline in the delete handler. | ||
| const threadListSettledRef = useRef(false); | ||
| useEffect(() => { | ||
| let cancelled = false; | ||
|
|
||
| void dispatch(loadThreads()) | ||
| .unwrap() | ||
| .then(data => { | ||
| // [#1123] Commented out — welcome-agent onboarding replaced by Joyride walkthrough | ||
| // if (cancelled || skipInitialThreadSelectionRef.current) return; | ||
| if (cancelled) return; | ||
| // [#1123] Commented out — welcome-agent onboarding replaced by Joyride walkthrough | ||
| // Always prefer the welcome thread during lockdown regardless of | ||
| // whether the server list is empty or not. Without this guard the | ||
| // stale `.then` could select a pre-existing thread from a prior | ||
| // session and pull the user out of the welcome conversation. | ||
| // const snapForSelect = getCoreStateSnapshot().snapshot; | ||
| // const threadStateForSelect = store.getState().thread; | ||
| // if (isWelcomeLocked(snapForSelect) && threadStateForSelect.welcomeThreadId) { | ||
| // dispatch(setSelectedThread(threadStateForSelect.welcomeThreadId)); | ||
| // void dispatch(loadThreadMessages(threadStateForSelect.welcomeThreadId)); | ||
| // return; | ||
| // } | ||
| const threadStateForSelect = store.getState().thread; | ||
| if (data.threads.length > 0) { | ||
| // Prefer the thread the user was last viewing (persisted across | ||
| // reloads via redux-persist on the `thread` slice). Only fall | ||
| // through to "most recent" if that thread no longer exists | ||
| // server-side (deleted, purged, or different user). | ||
| const persistedId = threadStateForSelect.selectedThreadId; | ||
| const resumeId = | ||
| persistedId && data.threads.some(t => t.id === persistedId) | ||
| ? persistedId | ||
| : data.threads[0].id; | ||
| dispatch(setSelectedThread(resumeId)); | ||
| void dispatch(loadThreadMessages(resumeId)); | ||
| } else { | ||
| void handleCreateNewThread(); | ||
| } | ||
| }); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [dispatch]); | ||
|
|
||
| useEffect(() => { | ||
| if (selectedThreadId) { | ||
| void dispatch(loadThreadMessages(selectedThreadId)); | ||
| if (threadListSettledRef.current) return; | ||
| if (threads.length > 0) { | ||
| threadListSettledRef.current = true; | ||
| // selectedThreadId from URL is already restored on cold boot — validate it still exists. | ||
| if (selectedThreadId && threads.some(t => t.id === selectedThreadId)) return; | ||
| // URL param missing or points to a deleted/purged thread — pick the first available. | ||
| setSelectedThreadId(threads[0].id); | ||
| } | ||
| }, [selectedThreadId, dispatch]); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [threads]); |
Contributor
There was a problem hiding this comment.
Keep selectedThreadId valid after list refreshes and empty states.
This one-shot guard means a fresh account or any later refresh that drops the current thread can leave selectedThreadId null/stale forever. The composer still looks enabled, but handleSendMessage() just returns on Line 468. Revalidate the selection whenever threads changes, and either auto-create a thread or explicitly block the composer while no thread exists.
💡 One localized way to close the gap
- const threadListSettledRef = useRef(false);
useEffect(() => {
- if (threadListSettledRef.current) return;
- if (threads.length > 0) {
- threadListSettledRef.current = true;
- // selectedThreadId from URL is already restored on cold boot — validate it still exists.
- if (selectedThreadId && threads.some(t => t.id === selectedThreadId)) return;
- // URL param missing or points to a deleted/purged thread — pick the first available.
- setSelectedThreadId(threads[0].id);
- }
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [threads]);
+ if (threads.length === 0) {
+ setSelectedThreadId(null);
+ return;
+ }
+ if (selectedThreadId && threads.some(t => t.id === selectedThreadId)) return;
+ setSelectedThreadId(threads[0].id);
+ }, [threads, selectedThreadId, setSelectedThreadId]);
...
- const composerInteractionBlocked = isComposerInteractionBlocked({
- activeThreadId,
- welcomePending: false,
- rustChat,
- });
+ const composerInteractionBlocked =
+ !selectedThreadId ||
+ isComposerInteractionBlocked({
+ activeThreadId,
+ welcomePending: false,
+ rustChat,
+ });Also applies to: 464-468
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/pages/Conversations.tsx` around lines 234 - 254, The one-shot guard
using threadListSettledRef inside the useEffect can leave selectedThreadId stale
when threads later become empty or change; update the logic in the effect that
watches threads (and any related startup effect) to always revalidate
selectedThreadId on every threads change: if threads.length > 0 ensure
selectedThreadId exists in threads and call setSelectedThreadId(threads[0].id)
when missing, and if threads.length === 0 either programmatically create a new
thread (invoke the existing create/thread-creation function) or set a clear
"no-thread" state that disables the composer; also audit handleSendMessage to
proactively create a thread when selectedThreadId is null before returning
(reference: threadListSettledRef, selectedThreadId, setSelectedThreadId,
useEffect watching threads, and handleSendMessage).
… resume
Rust core (the real fix): web channel's build_session_agent was calling
set_agent_definition_name AFTER Agent::from_config_for_agent, but
session_key is frozen during .build() — so transcript files were written
as {ts}_orchestrator.jsonl regardless of thread, and the resume scan
(which looks for files matching the post-rename, thread-scoped name)
never found a match. Result: every cache-miss continuation (process
restart, mid-turn cancel, anything that evicts THREAD_SESSIONS) started
the agent with empty history, so the model "forgot" prior context even
though the JSONL was on disk.
Add Agent::from_config_for_agent_with_name so the per-thread name flows
to the builder before .build(); web.rs now uses it. Regression test
locks the contract that two definition_names in the same workspace
resume independently.
Frontend: pull threads/messages fresh from core on every selection and
re-pull after each chat_done event. selectedThreadId moves to a URL
hash query param so it survives reload without redux-persist.
chatRuntimeSlice (livestream preview / tool timeline) is unchanged.
The 370-line threadSlice and its persist config are deleted.
senamakel
force-pushed
the
refactor/threads-rust-source-of-truth
branch
from
3be500a to
948f055
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/src/lib/threads/useThreadMessages.ts (1)
59-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate in-flight fetches when
threadIdis cleared.This branch clears UI state but leaves
fetchSeqRefunchanged, so a response from the previously selected thread can still land afterward and repopulatemessagesfor a deselected thread. Bump the sequence before returning here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/threads/useThreadMessages.ts` around lines 59 - 65, When threadId is falsy the effect clears UI state but doesn't invalidate pending fetches; increment fetchSeqRef (the sequence used to discard stale fetch results) before returning so any in-flight responses for the previous thread are ignored — update the useEffect handling of threadId to bump fetchSeqRef.current (or call the existing bump method) prior to calling setMessages([]), setError(null), setIsLoading(false) and returning.app/src/providers/ChatRuntimeProvider.tsx (1)
599-615:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh the thread list after these direct JSONL appends.
Both success paths invalidate
useThreadMessages(), but neither invalidates the threads list. That leaves sidebar metadata and ordering stale after proactive/error messages until another code path happens to refresh threads. Trigger the existing thread-list refresh event (oruseThreads().refresh()) after each successfulappendMessage(...).Also applies to: 713-731
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/providers/ChatRuntimeProvider.tsx` around lines 599 - 615, The proactive/error message append paths call threadApi.appendMessage(...) and notifyThreadMessagesRefresh(...) but do not refresh the threads list; update both places (the appendMessage success handlers around the proactiveMsg and the block at lines 713-731) to trigger the threads-list refresh after a successful append — e.g., call the existing thread-list refresh mechanism (invoke useThreads().refresh() or the equivalent notifyThreadsRefresh/rtLog event that your app uses) using the targetThreadId so the sidebar metadata and ordering are updated immediately after appendMessage completes.app/src/pages/Conversations.tsx (1)
243-254:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRevalidate the selected thread on every list refresh.
threadListSettledRefstill makes this a one-shot check, so later refreshes can leaveselectedThreadIdnull or pointing at a deleted thread. On a fresh/emptied list that turns into a silent no-op send path becausehandleSendMessage()bails when no valid thread is selected. Handlethreads.length === 0explicitly and keep revalidating the URL selection wheneverthreadschanges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/Conversations.tsx` around lines 243 - 254, threadListSettledRef makes the validation a one-shot so later updates to threads can leave selectedThreadId stale; update the useEffect that depends on threads (the block referencing threadListSettledRef, threads, selectedThreadId, and setSelectedThreadId) to always revalidate on every threads change: explicitly handle threads.length === 0 by clearing selection (call setSelectedThreadId(null) or equivalent), and if threads.length > 0 check if selectedThreadId exists in threads and if not setSelectedThreadId(threads[0].id); remove or reset the one-time threadListSettledRef guard so the effect runs fully on each threads update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/lib/threads/useThreadMessages.ts`:
- Around line 72-75: In useThreadMessages, remove the synchronous calls to
setMessages([]), setError(null), and setIsLoading(true) from the top level of
the effect body and instead perform the "clear + set loading" transition inside
the async fetch task (e.g., an async IIFE or fetchMessages function) so the
state updates occur asynchronously (or dispatch a RESET action if you switch to
useReducer); specifically, delete the three setters from the effect start and
invoke them from within the async function just before the network call (or
dispatch RESET there) so the lint rule react-hooks/set-state-in-effect is not
violated.
---
Duplicate comments:
In `@app/src/lib/threads/useThreadMessages.ts`:
- Around line 59-65: When threadId is falsy the effect clears UI state but
doesn't invalidate pending fetches; increment fetchSeqRef (the sequence used to
discard stale fetch results) before returning so any in-flight responses for the
previous thread are ignored — update the useEffect handling of threadId to bump
fetchSeqRef.current (or call the existing bump method) prior to calling
setMessages([]), setError(null), setIsLoading(false) and returning.
In `@app/src/pages/Conversations.tsx`:
- Around line 243-254: threadListSettledRef makes the validation a one-shot so
later updates to threads can leave selectedThreadId stale; update the useEffect
that depends on threads (the block referencing threadListSettledRef, threads,
selectedThreadId, and setSelectedThreadId) to always revalidate on every threads
change: explicitly handle threads.length === 0 by clearing selection (call
setSelectedThreadId(null) or equivalent), and if threads.length > 0 check if
selectedThreadId exists in threads and if not
setSelectedThreadId(threads[0].id); remove or reset the one-time
threadListSettledRef guard so the effect runs fully on each threads update.
In `@app/src/providers/ChatRuntimeProvider.tsx`:
- Around line 599-615: The proactive/error message append paths call
threadApi.appendMessage(...) and notifyThreadMessagesRefresh(...) but do not
refresh the threads list; update both places (the appendMessage success handlers
around the proactiveMsg and the block at lines 713-731) to trigger the
threads-list refresh after a successful append — e.g., call the existing
thread-list refresh mechanism (invoke useThreads().refresh() or the equivalent
notifyThreadsRefresh/rtLog event that your app uses) using the targetThreadId so
the sidebar metadata and ordering are updated immediately after appendMessage
completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7423d129-ffe3-40e0-a68f-731a6a0e2672
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
app/src/App.tsxapp/src/lib/threads/ThreadsContext.tsxapp/src/lib/threads/__tests__/ThreadsContext.test.tsxapp/src/lib/threads/__tests__/messagesRefreshBus.test.tsapp/src/lib/threads/__tests__/useSelectedThread.test.tsxapp/src/lib/threads/__tests__/useThreadMessages.test.tsxapp/src/lib/threads/messagesRefreshBus.tsapp/src/lib/threads/useActiveThread.tsxapp/src/lib/threads/useSelectedThread.tsapp/src/lib/threads/useThreadMessages.tsapp/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.render.test.tsxapp/src/pages/conversations/components/WorkerThreadRefCard.tsxapp/src/pages/onboarding/__tests__/OnboardingLayout.test.tsxapp/src/providers/ChatRuntimeProvider.tsxapp/src/providers/CoreStateProvider.tsxapp/src/providers/__tests__/ChatRuntimeProvider.test.tsxapp/src/providers/__tests__/CoreStateProvider.identityFlip.test.tsxapp/src/store/__tests__/threadSlice.test.tsapp/src/store/index.tsapp/src/store/threadSlice.tssrc/openhuman/agent/harness/session/builder.rssrc/openhuman/agent/harness/session/turn_tests.rssrc/openhuman/channels/providers/web.rs
💤 Files with no reviewable changes (3)
- app/src/store/index.ts
- app/src/store/threadSlice.ts
- app/src/store/tests/threadSlice.test.ts
✅ Files skipped from review due to trivial changes (4)
- app/src/lib/threads/useActiveThread.tsx
- app/src/pages/onboarding/tests/OnboardingLayout.test.tsx
- app/src/providers/CoreStateProvider.tsx
- app/src/lib/threads/ThreadsContext.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- app/src/pages/conversations/components/WorkerThreadRefCard.tsx
- app/src/lib/threads/messagesRefreshBus.ts
- app/src/lib/threads/tests/useSelectedThread.test.tsx
- app/src/providers/tests/CoreStateProvider.identityFlip.test.tsx
- app/src/lib/threads/tests/useThreadMessages.test.tsx
- app/src/pages/tests/Conversations.render.test.tsx
Comment on lines
+72
to
+75
| // Clear immediately before fetching so no stale messages flash. | ||
| setMessages([]); | ||
| setError(null); | ||
| setIsLoading(true); |
Contributor
There was a problem hiding this comment.
This reset-before-fetch pattern will trip react-hooks/set-state-in-effect.
The effect synchronously calls setMessages, setError, and setIsLoading before kicking off the fetch. This repo explicitly enforces that lint rule, so this hook needs a different state shape/flow for the cleared-loading transition instead of setting state directly in the effect body. Based on learnings, “do not perform synchronous setState (or other state-updating calls) directly inside useEffect bodies… The lint rule react-hooks/set-state-in-effect (enforced in this codebase) disallows patterns such as resetting state flags at the top of an effect before returning.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/lib/threads/useThreadMessages.ts` around lines 72 - 75, In
useThreadMessages, remove the synchronous calls to setMessages([]),
setError(null), and setIsLoading(true) from the top level of the effect body and
instead perform the "clear + set loading" transition inside the async fetch task
(e.g., an async IIFE or fetchMessages function) so the state updates occur
asynchronously (or dispatch a RESET action if you switch to useReducer);
specifically, delete the three setters from the effect start and invoke them
from within the async function just before the network call (or dispatch RESET
there) so the lint rule react-hooks/set-state-in-effect is not violated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
web::build_session_agentwas setting the per-thread agent name AFTER.build(), butsession_keyis frozen during the build. Transcripts were written as{ts}_orchestrator.jsonlignoring the thread, and resume scans (looking for the post-rename, thread-scoped name) never matched — so every cache-miss continuation started with empty history.Agent::from_config_for_agent_with_name(config, agent_id, definition_name)plumbs the name into the builder before.build().web.rsuses it; the post-build setter call is gone.chat_done/chat_error/ proactive append (via a smallmessagesRefreshBus).selectedThreadIdmoves to a URL hash query param.threadSlice.ts(370 lines) + its persist config deleted.chatRuntimeSlice(livestream preview, tool timeline) untouched.Problem
Users reported that continuing a thread "forgot" prior context — the agent appeared to start a fresh session even though the UI showed the right history. Two compounding issues:
session_keywas frozen). So on cache miss the resume lookup couldn't find a matching file →cached_transcript_messages = None→ empty history at turn start.Solution
Rust: new public constructor that accepts a separate
definition_nameand passes it toAgentBuilderbefore.build()sosession_keycorrectly captures the per-thread suffix.web::build_session_agentcalls it. Regression test (transcript_resume_is_scoped_by_definition_name) locks the contract: twodefinition_names in the same workspace must resume independently, and matching names must resume.Frontend: new
app/src/lib/threads/:ThreadsContext— fetch threads on mount, exposerefresh/create/remove/updateLabels/purge/generateTitleIfNeeded.useThreadMessages(threadId)— auto-fetches onthreadIdchange AND onmessagesRefreshBus.notify(threadId). No cross-switch cache; truth = core.messagesRefreshBus— tiny module-level pub/sub soChatRuntimeProvidercan invalidate the mounted messages hook from outside the React tree.useSelectedThread— URL hash?t=<id>(replaces redux-persist).useActiveThread— in-memory turn target.ChatRuntimeProvider.onDone/onError(after error-row append) /onProactiveMessage(after proactive-row append) all callnotifyThreadMessagesRefresh(threadId).Submission Checklist
transcript_resume_is_scoped_by_definition_name; new TS unit tests forThreadsContext,useThreadMessages(incl. bus refetch),useSelectedThread,messagesRefreshBus; updatedChatRuntimeProvider/CoreStateProvider/Conversations/OnboardingLayouttests.Impact
selectedThreadIdis dropped; users land on the most-recent thread once, then the URL takes over.{ts}_orchestrator.jsonlstyle) won't be picked up by the new resume scan — they're effectively orphaned. Acceptable given the alternative (continuing to load some other thread's transcript) is worse.getThreadMessagesRPC per turn end (and per proactive append). Bounded by visible thread.Related
ChatRuntimeProvider's directthreadApi.appendMessagecalls (proactive + error message) should move into core so the UI never writes JSONL.Test plan
?t=restores the same thread.Summary by CodeRabbit
Refactor
Tests