Threads 10: Back threads with canonical Zustand state - #267
Open
FranciscoMoretti wants to merge 8 commits into
Open
Threads 10: Back threads with canonical Zustand state#267FranciscoMoretti wants to merge 8 commits into
FranciscoMoretti wants to merge 8 commits into
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideRefactors the ChatJS thread-aware store to use a canonical, ordered MessageTreeSnapshot as the source of truth for tree topology and navigation, deriving visible messages and sibling relationships from that snapshot while preserving existing behavior during streaming and server sync, and adds comprehensive tests for snapshot behavior and metadata handling. Sequence diagram for applying a MessageTreeSnapshot to the chat storesequenceDiagram
actor Thread
participant ChatStore
participant mergeTreeMessages
participant mergeTreeSnapshot
participant buildThreadFromSnapshot
Thread->>ChatStore: setTreeSnapshot(snapshot)
ChatStore->>mergeTreeMessages: mergeTreeMessages(snapshotMessages, allMessages, [], parentById)
mergeTreeMessages-->>ChatStore: mergedMessages
ChatStore->>mergeTreeSnapshot: mergeTreeSnapshot(snapshot, treeSnapshot, mergedMessages)
mergeTreeSnapshot-->>ChatStore: mergedSnapshot
ChatStore->>ChatStore: getSnapshotSignature(mergedSnapshot)
alt signature changed
ChatStore->>buildThreadFromSnapshot: buildThreadFromSnapshot(mergedMessages, mergedSnapshot, mergedSnapshot.cursorId)
buildThreadFromSnapshot-->>ChatStore: nextVisibleThread
ChatStore->>ChatStore: _messageIndex.update(nextVisibleThread)
ChatStore->>ChatStore: update messages, allMessages, treeSnapshot, treeSnapshotSignature, childrenMap
else signature unchanged
ChatStore->>ChatStore: return without state changes
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThe chat runtime replaces thread middleware with snapshot-backed ChangesApplication thread migration
Thread package vendoring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This was referenced Aug 2, 2026
This was referenced Aug 2, 2026
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
FranciscoMoretti
force-pushed
the
codex/threads-v2-07-store-tree-snapshot
branch
from
August 2, 2026 07:52
edf3932 to
b70c5e6
Compare
Base automatically changed from
codex/threads-v2-09-ai-sdk-compatibility
to
main
August 3, 2026 07:17
FranciscoMoretti
force-pushed
the
codex/threads-v2-07-store-tree-snapshot
branch
from
August 3, 2026 07:26
a14c895 to
b3fdab4
Compare
![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: 6
🧹 Nitpick comments (6)
apps/chat/lib/stores/with-threads.test.ts (4)
198-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrengthen the deep-path assertion.
The test asserts the path length and the first id only. A path that is reversed or reordered still passes. Assert the last id as well, because the test verifies that a deep path survives intact.
💚 Proposed assertion
assert.equal(store.getState().messages.length, messages.length); assert.equal(store.getState().messages.at(0)?.id, messages.at(0)?.id); + assert.equal(store.getState().messages.at(-1)?.id, messages.at(-1)?.id);🤖 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 `@apps/chat/lib/stores/with-threads.test.ts` around lines 198 - 199, Strengthen the deep-path verification in the test by adding an assertion for the last message ID alongside the existing length and first-ID checks. Use the final entries from store.getState().messages and messages so reordered or reversed paths fail.
493-495: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLook up the node by id instead of by position.
nodes.at(-1)depends on the DFS order thatbuildTreeSnapshotFromMessagesproduces. A change in traversal order breaks this test for a reason unrelated to metadata preservation.💚 Proposed change
const updatedAssistant = store .getState() - .treeSnapshot.nodes.at(-1)?.message; + .treeSnapshot.nodes.find(({ message }) => message.id === assistant.id) + ?.message;🤖 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 `@apps/chat/lib/stores/with-threads.test.ts` around lines 493 - 495, Update the test’s updatedAssistant lookup to retrieve the expected node by its stable node id rather than using treeSnapshot.nodes.at(-1). Use the id associated with the assistant message under test and preserve the existing message assertion.
629-639: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the cursor after the empty
setMessagescall.
setMessages([])leavesmessagesempty.addMessageToTreethen derives the cursor fromstate.messages.at(-1)?.id ?? null, so the cursor becomesnulleven thoughuserCandassistantCwere added. The test asserts only the root sibling ids, so this cursor behavior is not verified.Add an assertion for
treeSnapshot.cursorIdto record the intended result of this sequence.🤖 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 `@apps/chat/lib/stores/with-threads.test.ts` around lines 629 - 639, Extend the test around switchToSibling, setMessages, and addMessageToTree to assert treeSnapshot.cursorId after adding userC and assistantC. Record the intended cursor value for this sequence, ensuring the test verifies cursor state in addition to rootSiblingIds.
44-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe store double uses
as unknown asand leavesstatusundefined.Two points.
First, line 54 uses
as unknown as, which the coding guidelines prohibit. Build a typed partial factory, or add the few fields the tests need, so the double satisfiesBaseChatStoreState<ChatMessage>without a double assertion.Second, the double never sets
status.setAllMessagesbranches onstate.status === "streaming" || state.status === "submitted"at lines 560 and 572 ofapps/chat/lib/stores/with-threads.ts. Withstatusundefined, no test exercises the streaming branch, so the preservation of the live visible path during streaming is untested. Add a test that setsstatusto"streaming"and asserts thatmessagesdoes not change whiletreeSnapshotdoes.As per coding guidelines: "Avoid
anyand type assertions, includingas unknown as; fix types instead of suppressing them."🤖 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 `@apps/chat/lib/stores/with-threads.test.ts` around lines 44 - 57, Update createThreadStore to construct a fully typed BaseChatStoreState<ChatMessage> without any type assertions, adding the required fields or using a typed partial factory. Initialize status explicitly, then add coverage for setAllMessages with status set to "streaming", asserting messages remain unchanged while treeSnapshot updates.Source: Coding guidelines
apps/chat/lib/stores/with-threads.ts (2)
118-136: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRecursive DFS can overflow the stack on very deep threads.
visitrecurses once per depth level. A linear conversation of several thousand messages produces the same depth. The tests cover 125 messages, which is safe, but the limit is the JavaScript stack, not a defined bound. Convert the traversal to an explicit stack if long threads are expected. The same pattern exists inmergeTreeSnapshotat lines 244-269.🤖 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 `@apps/chat/lib/stores/with-threads.ts` around lines 118 - 136, Replace the recursive visit traversal in the snapshot-building logic with an explicit stack-based DFS so deeply nested threads cannot overflow the JavaScript call stack; preserve visited tracking, parentId assignment, child ordering, and root/message fallback behavior. Apply the same iterative traversal change to mergeTreeSnapshot, reusing its existing traversal semantics.
364-368: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSignature serializes full message content on every snapshot update.
getSnapshotSignaturecallsJSON.stringifyon the whole snapshot, including all message parts.setTreeSnapshotcalls it twice per invocation. For a long thread this serializes the complete conversation on each update.The comparison only needs to detect topology and cursor changes plus message identity. Build the signature from
cursorIdand theid/parentIdpairs instead.♻️ Cheaper signature
function getSnapshotSignature<UM extends UIMessage>( snapshot: MessageTreeSnapshot<UM> ) { - return JSON.stringify(snapshot); + return `${snapshot.cursorId ?? ""}|${snapshot.nodes + .map(({ message, parentId }) => `${message.id}>${parentId ?? ""}`) + .join(",")}`; }Note the behavior change: this signature ignores message content updates. If
setTreeSnapshotmust also propagate content-only changes, keep a content marker such as a per-message version orparts.length.🤖 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 `@apps/chat/lib/stores/with-threads.ts` around lines 364 - 368, Update getSnapshotSignature to avoid JSON.stringify of the full MessageTreeSnapshot; construct the signature from cursorId and each message’s id/parentId pairs, preserving detection of topology and cursor changes while ignoring message content updates unless setTreeSnapshot requires a lightweight content marker.
🤖 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 `@apps/chat/lib/stores/with-threads.ts`:
- Around line 207-242: Update mergeTreeSnapshot’s child-group construction
around childrenByParentId so each sibling list is sorted with
compareSiblingMessages, matching buildTreeSnapshotFromMessages before branch
traversal and sibling navigation consume it. Preserve the existing parent
validation and message collection logic, and do not rely on
uniqueOrderedMessageIds insertion order unless documenting that intentional
rule.
- Around line 535-541: Update the visible-thread handling in
apps/chat/lib/stores/with-threads.ts at lines 535-541 and 580-591: at the first
site, preserve the current path and return without writing state when non-null
mergedSnapshot.cursorId produces an empty buildThreadFromSnapshot result, while
retaining the empty result for a null cursor; at the second site, use
currentVisibleMessages when selectedLeafId is set but the built thread is empty
instead of writing the empty array.
- Around line 543-550: Keep the combined state update in setAllMessages
unchanged and do not add a preceding base-action call. Ensure setTreeSnapshot
remains responsible for updating _messageIndex, messages, _memoizedSelectors,
and _throttledMessages within the existing single set operation.
- Around line 187-195: Cache the result of getSnapshotIndexes per
MessageTreeSnapshot using a WeakMap, returning the cached index on repeated
lookups and storing newly built indexes before returning. Update
getSnapshotParentId, buildChildrenMapFromSnapshot, buildThreadFromSnapshot,
findLeafDfsToRightFromSnapshot, findRightmostLeafFromSnapshot, and
getMessageSiblingInfo to reuse the cached index or pass it through instead of
rebuilding it for each lookup.
- Around line 677-679: Update the parallel response rendering guard around the
parentId/parallelGroupId check to also require multiple messages, using
parallelGroupInfo.messages.length or the existing group-length guard. Ensure the
switcher returns null when responses are missing or contain only one message,
regardless of requested model slots.
- Around line 612-626: Replace the inline metadata-preservation logic in the
message-list update block with the existing mergeMessageIntoMap helper. Build
the next message list through that function while preserving the current
replacement ordering, so metadata merging is owned by a single implementation.
---
Nitpick comments:
In `@apps/chat/lib/stores/with-threads.test.ts`:
- Around line 198-199: Strengthen the deep-path verification in the test by
adding an assertion for the last message ID alongside the existing length and
first-ID checks. Use the final entries from store.getState().messages and
messages so reordered or reversed paths fail.
- Around line 493-495: Update the test’s updatedAssistant lookup to retrieve the
expected node by its stable node id rather than using treeSnapshot.nodes.at(-1).
Use the id associated with the assistant message under test and preserve the
existing message assertion.
- Around line 629-639: Extend the test around switchToSibling, setMessages, and
addMessageToTree to assert treeSnapshot.cursorId after adding userC and
assistantC. Record the intended cursor value for this sequence, ensuring the
test verifies cursor state in addition to rootSiblingIds.
- Around line 44-57: Update createThreadStore to construct a fully typed
BaseChatStoreState<ChatMessage> without any type assertions, adding the required
fields or using a typed partial factory. Initialize status explicitly, then add
coverage for setAllMessages with status set to "streaming", asserting messages
remain unchanged while treeSnapshot updates.
In `@apps/chat/lib/stores/with-threads.ts`:
- Around line 118-136: Replace the recursive visit traversal in the
snapshot-building logic with an explicit stack-based DFS so deeply nested
threads cannot overflow the JavaScript call stack; preserve visited tracking,
parentId assignment, child ordering, and root/message fallback behavior. Apply
the same iterative traversal change to mergeTreeSnapshot, reusing its existing
traversal semantics.
- Around line 364-368: Update getSnapshotSignature to avoid JSON.stringify of
the full MessageTreeSnapshot; construct the signature from cursorId and each
message’s id/parentId pairs, preserving detection of topology and cursor changes
while ignoring message content updates unless setTreeSnapshot requires a
lightweight content marker.
🪄 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 Plus
Run ID: 6fd9cbd9-251e-43b6-b736-859875a6b6e0
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/chat/lib/stores/hooks-threads.tsapps/chat/lib/stores/with-threads.test.tsapps/chat/lib/stores/with-threads.tsapps/chat/package.json
💤 Files with no reviewable changes (1)
- apps/chat/lib/stores/hooks-threads.ts
FranciscoMoretti
force-pushed
the
codex/threads-v2-07-store-tree-snapshot
branch
from
August 11, 2026 06:43
5c8527c to
09488f6
Compare
FranciscoMoretti
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/chat/lib/thread-utils.test.ts (1)
32-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the orphan and cycle fallback path.
buildTreeSnapshotFromMessagescontains a dedicated fallback loop for messages with missing or cyclic parent metadata (apps/chat/lib/thread-utils.tslines 124-126). The PR objectives list this behavior as intentional. No test exercises it. A regression would silently drop messages from the snapshot.Add one case with a message whose
parentMessageIdpoints to an absent ID, and one case with two messages that reference each other.🧪 Proposed additional test case
}); + + it("keeps messages with unresolvable parents", () => { + const orphan = message({ + id: "orphan", + parentMessageId: "missing", + role: "assistant", + }); + + const tree = buildTreeSnapshotFromMessages([orphan]); + + expect(tree.nodes.map(({ message: node }) => node.id)).toEqual([ + orphan.id, + ]); + expect(tree.nodes[0]?.parentId).toBeNull(); + }); });🤖 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 `@apps/chat/lib/thread-utils.test.ts` around lines 32 - 66, Add test coverage for the fallback behavior in buildTreeSnapshotFromMessages by adding one case with a message whose parentMessageId references a missing message and another case with two messages whose parentMessageId values form a cycle. Assert that both fallback scenarios retain all messages in the resulting tree or snapshot, alongside the existing sibling-branch assertions.apps/chat/lib/stores/hooks-threads.ts (1)
101-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an optimistic parallel-group regression test.
buildDraftChatSubmissioncopiesparallelGroupIdto the user metadata, andParallelResponseCardscreates model slots before assistant messages exist. Add coverage for this state with an emptyparallelGroupInfo.messagesarray.🤖 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 `@apps/chat/lib/stores/hooks-threads.ts` around lines 101 - 117, Add a regression test covering optimistic parallel-group state where buildDraftChatSubmission has set parallelGroupId on the user metadata before assistant messages exist. Verify useParallelGroupInfo returns a non-null group with an empty messages array while ParallelResponseCards has created the model slots.Source: Learnings
🤖 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 `@apps/chat/lib/stores/hooks-threads.ts`:
- Around line 62-74: Update the equality function in useMessageSiblingInfo to
compare sibling message IDs element-wise, matching the existing
useParallelGroupInfo behavior. Retain the null, siblingIndex, and
siblings.length checks, but ensure any changed sibling identity makes the values
unequal so replacements are returned to consumers.
In `@apps/chat/lib/stores/with-thread-state.ts`:
- Around line 28-32: Update createCustomChatStore initialization so when
options.initialTree is provided, base.messages, _throttledMessages, and
_messageIndex are derived from the created threadSnapshot rather than
initialMessages. Keep all related fields synchronized with the selected snapshot
path at construction, while preserving the existing messages-based
initialization when initialTree is absent.
---
Nitpick comments:
In `@apps/chat/lib/stores/hooks-threads.ts`:
- Around line 101-117: Add a regression test covering optimistic parallel-group
state where buildDraftChatSubmission has set parallelGroupId on the user
metadata before assistant messages exist. Verify useParallelGroupInfo returns a
non-null group with an empty messages array while ParallelResponseCards has
created the model slots.
In `@apps/chat/lib/thread-utils.test.ts`:
- Around line 32-66: Add test coverage for the fallback behavior in
buildTreeSnapshotFromMessages by adding one case with a message whose
parentMessageId references a missing message and another case with two messages
whose parentMessageId values form a cycle. Assert that both fallback scenarios
retain all messages in the resulting tree or snapshot, alongside the existing
sibling-branch assertions.
🪄 Autofix
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 Plus
Run ID: 0f30bebb-f1e1-486d-bdf7-787a6bbe2483
📒 Files selected for processing (20)
apps/chat/app/(chat)/chat-route-host.tsxapps/chat/app/(chat)/share/[id]/shared-chat-page.tsxapps/chat/components/chat-runtime-controller.tsxapps/chat/components/chat-sync.tsxapps/chat/components/chat-system.tsxapps/chat/hooks/use-chat-system-initial-state.tsapps/chat/lib/app-chat-runtime.tsapps/chat/lib/application-thread.tsapps/chat/lib/stores/custom-store-provider.tsxapps/chat/lib/stores/hooks-threads.tsapps/chat/lib/stores/with-thread-state.tsapps/chat/lib/stores/with-threads.test.tsapps/chat/lib/stores/with-threads.tsapps/chat/lib/stores/zustand-thread-state.test.tsapps/chat/lib/stores/zustand-thread-state.tsapps/chat/lib/thread-utils.test.tsapps/chat/lib/thread-utils.tspackages/thread/src/index.tspackages/thread/src/thread-state.tspackages/thread/test/thread.test.ts
💤 Files with no reviewable changes (2)
- apps/chat/lib/stores/with-threads.test.ts
- apps/chat/lib/stores/with-threads.ts
Comment on lines
62
to
74
| export function useMessageSiblingInfo( | ||
| messageId: string | ||
| ): MessageSiblingInfo<ChatMessage> | null { | ||
| ): MessageSiblingInfo | null { | ||
| return useThreadStore( | ||
| (state) => state.getMessageSiblingInfo(messageId), | ||
| (a, b) => { | ||
| if (a === null && b === null) { | ||
| return true; | ||
| } | ||
| if (a === null || b === null) { | ||
| return false; | ||
| } | ||
| return ( | ||
| (state) => getSiblingInfo(state, messageId), | ||
| (a, b) => | ||
| a === b || | ||
| (a !== null && | ||
| b !== null && | ||
| a.siblingIndex === b.siblingIndex && | ||
| a.siblings.length === b.siblings.length | ||
| ); | ||
| } | ||
| a.siblings.length === b.siblings.length) | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare sibling IDs in the equality function.
The equality function checks only siblingIndex and siblings.length. It never compares the sibling identities. If a sibling is replaced by a different message at the same index and the same count, the selector treats the new value as equal and returns the stale object. Consumers then render the previous message.
The PR objectives state that a temporary client placeholder can be replaced by a completed server response. That replacement keeps the index and the count, so this path is reachable.
useParallelGroupInfo at lines 148-153 already compares IDs element-wise. Apply the same comparison here.
🐛 Proposed fix
(a, b) =>
a === b ||
(a !== null &&
b !== null &&
a.siblingIndex === b.siblingIndex &&
- a.siblings.length === b.siblings.length)
+ a.siblings.length === b.siblings.length &&
+ a.siblings.every(
+ (sibling, index) => sibling.id === b.siblings[index]?.id
+ ))
);📝 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
| export function useMessageSiblingInfo( | |
| messageId: string | |
| ): MessageSiblingInfo<ChatMessage> | null { | |
| ): MessageSiblingInfo | null { | |
| return useThreadStore( | |
| (state) => state.getMessageSiblingInfo(messageId), | |
| (a, b) => { | |
| if (a === null && b === null) { | |
| return true; | |
| } | |
| if (a === null || b === null) { | |
| return false; | |
| } | |
| return ( | |
| (state) => getSiblingInfo(state, messageId), | |
| (a, b) => | |
| a === b || | |
| (a !== null && | |
| b !== null && | |
| a.siblingIndex === b.siblingIndex && | |
| a.siblings.length === b.siblings.length | |
| ); | |
| } | |
| a.siblings.length === b.siblings.length) | |
| ); | |
| } | |
| export function useMessageSiblingInfo( | |
| messageId: string | |
| ): MessageSiblingInfo | null { | |
| return useThreadStore( | |
| (state) => getSiblingInfo(state, messageId), | |
| (a, b) => | |
| a === b || | |
| (a !== null && | |
| b !== null && | |
| a.siblingIndex === b.siblingIndex && | |
| a.siblings.length === b.siblings.length && | |
| a.siblings.every( | |
| (sibling, index) => sibling.id === b.siblings[index]?.id | |
| )) | |
| ); | |
| } |
🤖 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 `@apps/chat/lib/stores/hooks-threads.ts` around lines 62 - 74, Update the
equality function in useMessageSiblingInfo to compare sibling message IDs
element-wise, matching the existing useParallelGroupInfo behavior. Retain the
null, siblingIndex, and siblings.length checks, but ensure any changed sibling
identity makes the values unequal so replacements are returned to consumers.
Comment on lines
+28
to
+32
| return { | ||
| ...base, | ||
| threadSnapshot: options.initialTree | ||
| ? createThreadStateSnapshot({ initialTree: options.initialTree }) | ||
| : createThreadStateSnapshot({ messages: base.messages }), |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect every createCustomChatStore call site and confirm initialMessages/initialTree pairing.
set -euo pipefail
rg -nP -C 12 '\bcreateCustomChatStore\s*\(' --type=ts --type=tsx apps packages || \
rg -nP -C 12 '\bcreateCustomChatStore\s*\(' -g '*.ts' -g '*.tsx' apps packages
# Show the provider wiring that forwards initialTree into withThreadState.
fd -t f 'custom-store-provider.tsx' | xargs -r rg -n -C 8 'initialTree|initialMessages|withThreadState'Repository: FranciscoMoretti/chat-js
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relevant files ---'
git ls-files | rg '(^|/)(with-thread-state\.ts|custom-store-provider\.tsx)$|createCustomChatStore'
echo '--- with-thread-state.ts ---'
sed -n '1,220p' apps/chat/lib/stores/with-thread-state.ts
echo '--- custom-store-provider.tsx ---'
sed -n '1,260p' apps/chat/lib/stores/custom-store-provider.tsx
echo '--- createCustomChatStore call sites ---'
rg -n -C 16 '\bcreateCustomChatStore\s*\(' apps packages -g '*.ts' -g '*.tsx' || trueRepository: FranciscoMoretti/chat-js
Length of output: 6252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- CustomStoreProvider usages ---'
rg -n -C 18 '\bCustomStoreProvider\b' apps packages -g '*.ts' -g '*.tsx' || true
echo '--- initialTree usages ---'
rg -n -C 14 '\binitialTree\b' apps packages -g '*.ts' -g '*.tsx' || true
echo '--- base store definitions and initialization ---'
fd -t f . apps/chat/lib/stores | sort
rg -n -C 20 'createChatStoreCreator|_messageIndex|_throttledMessages|messages:' apps/chat/lib/stores -g '*.ts' -g '*.tsx' || trueRepository: FranciscoMoretti/chat-js
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- base store file map ---'
fd -t f . apps/chat/lib/stores | sort
echo '--- base store creator ---'
rg -n -C 35 'createChatStoreCreator|function createChatStoreCreator|_messageIndex|_throttledMessages' apps/chat/lib/stores/base.ts apps/chat/lib/stores -g '*.ts' -g '*.tsx' || true
echo '--- initial state helper and tree utilities ---'
sed -n '1,110p' apps/chat/hooks/use-chat-system-initial-state.ts
rg -n -C 20 'function (getDefaultThread|buildTreeSnapshotFromMessages)|const (getDefaultThread|buildTreeSnapshotFromMessages)|export .*getDefaultThread|export .*buildTreeSnapshotFromMessages' apps/chat/lib apps/chat/hooks -g '*.ts' -g '*.tsx' || true
echo '--- runtime creation and route branches ---'
sed -n '88,155p' apps/chat/lib/app-chat-runtime.ts
sed -n '228,310p' 'apps/chat/app/(chat)/chat-route-host.tsx'Repository: FranciscoMoretti/chat-js
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- utility definitions ---'
rg -n -C 30 'getDefaultThread|buildTreeSnapshotFromMessages' apps/chat/lib apps/chat/hooks -g '*.ts' -g '*.tsx' | head -n 260
echo '--- all calls to initial-state hook ---'
rg -n -C 18 'useChatSystemInitialState\s*\(' apps/chat -g '*.ts' -g '*.tsx' || true
echo '--- runtime input construction ---'
rg -n -C 24 'createAppRuntimeInput|initialMessages:|initialTree:' 'apps/chat/app/(chat)/chat-route-host.tsx' apps/chat/lib -g '*.ts' -g '*.tsx' | head -n 360
echo '--- exports/imports for thread utilities ---'
rg -n -C 12 'thread-utils|getDefaultThread|buildTreeSnapshotFromMessages' apps/chat/lib apps/chat/hooks -g '*.ts' -g '*.tsx' | head -n 300Repository: FranciscoMoretti/chat-js
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- complete thread utility implementation ---'
sed -n '1,155p' apps/chat/lib/thread-utils.ts
echo '--- thread snapshot path implementation ---'
sed -n '1,95p' packages/thread/src/thread-state.ts
sed -n '1,190p' packages/thread/src/message-tree.ts
echo '--- standalone selected-path verifier ---'
python3 - <<'PY'
from collections import defaultdict
def default_leaf(messages):
# Mirrors the utility's default-leaf selection after inspecting its source.
return messages[-1]["id"] if messages else None
def default_thread(messages):
by_id = {message["id"]: message for message in messages}
leaf = default_leaf(messages)
path = []
while leaf is not None and leaf in by_id:
message = by_id[leaf]
path.insert(0, message)
parent = message.get("parent")
if parent == message["id"]:
break
leaf = parent
return path
def tree_path(messages, cursor):
return default_thread(messages) if cursor is not None else []
fixtures = [
[
{"id": "root", "parent": None},
{"id": "first", "parent": "root"},
{"id": "second", "parent": "root"},
],
[
{"id": "second", "parent": "root"},
{"id": "root", "parent": None},
{"id": "first", "parent": "root"},
],
[],
]
for messages in fixtures:
selected = default_thread(messages)
cursor = selected[-1]["id"] if selected else None
assert [m["id"] for m in selected] == [m["id"] for m in tree_path(messages, cursor)]
print({
"all_messages": [m["id"] for m in messages],
"initial_messages": [m["id"] for m in selected],
"cursor": cursor,
"snapshot_messages": [m["id"] for m in tree_path(messages, cursor)],
})
PYRepository: FranciscoMoretti/chat-js
Length of output: 11361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- complete thread utility implementation ---'
sed -n '1,155p' apps/chat/lib/thread-utils.ts
echo '--- thread snapshot path implementation ---'
sed -n '1,95p' packages/thread/src/thread-state.ts
sed -n '1,190p' packages/thread/src/message-tree.ts
echo '--- standalone selected-path verifier ---'
python3 - <<'PY'
from collections import defaultdict
def default_leaf(messages):
return messages[-1]["id"] if messages else None
def default_thread(messages):
by_id = {message["id"]: message for message in messages}
leaf = default_leaf(messages)
path = []
while leaf is not None and leaf in by_id:
message = by_id[leaf]
path.insert(0, message)
parent = message.get("parent")
if parent == message["id"]:
break
leaf = parent
return path
def tree_path(messages, cursor):
return default_thread(messages) if cursor is not None else []
fixtures = [
[
{"id": "root", "parent": None},
{"id": "first", "parent": "root"},
{"id": "second", "parent": "root"},
],
[
{"id": "second", "parent": "root"},
{"id": "root", "parent": None},
{"id": "first", "parent": "root"},
],
[],
]
for messages in fixtures:
selected = default_thread(messages)
cursor = selected[-1]["id"] if selected else None
assert [m["id"] for m in selected] == [m["id"] for m in tree_path(messages, cursor)]
print({
"all_messages": [m["id"] for m in messages],
"initial_messages": [m["id"] for m in selected],
"cursor": cursor,
"snapshot_messages": [m["id"] for m in tree_path(messages, cursor)],
})
PYRepository: FranciscoMoretti/chat-js
Length of output: 11361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from collections import defaultdict
def default_leaf(messages):
return max(messages, key=lambda message: message["created_at"], default=None)
def default_thread(messages):
by_id = {message["id"]: message for message in messages}
leaf = default_leaf(messages)
result = []
current = leaf["id"] if leaf else None
for _ in range(100):
if not current or current not in by_id:
break
message = by_id[current]
result.insert(0, message)
parent = message["parent"]
if parent == message["id"]:
break
current = parent
return result
def build_tree_snapshot(messages, cursor_id):
children = defaultdict(list)
for message in messages:
children[message["parent"]].append(message)
nodes = []
visited = set()
def visit(message, parent_id):
if message["id"] in visited:
return
visited.add(message["id"])
nodes.append((message, parent_id))
for child in children[message["id"]]:
visit(child, message["id"])
for root in children[None]:
visit(root, None)
for message in messages:
visit(message, None)
return {"nodes": nodes, "cursor_id": cursor_id}
def snapshot_path(snapshot):
by_id = {message["id"]: (message, parent_id) for message, parent_id in snapshot["nodes"]}
result = []
current = snapshot["cursor_id"]
while current:
if current not in by_id:
break
message, parent = by_id[current]
result.insert(0, message)
current = parent
return result
fixtures = [
[
{"id": "root", "parent": None, "created_at": 1},
{"id": "first", "parent": "root", "created_at": 2},
{"id": "second", "parent": "root", "created_at": 3},
],
[
{"id": "root", "parent": None, "created_at": 1},
{"id": "branch", "parent": "root", "created_at": 2},
{"id": "leaf", "parent": "branch", "created_at": 5},
{"id": "sibling", "parent": "root", "created_at": 4},
],
[
{"id": "root", "parent": None, "created_at": 10},
{"id": "child", "parent": "root", "created_at": 20},
],
]
for messages in fixtures:
initial_messages = default_thread(messages)
cursor = initial_messages[-1]["id"] if initial_messages else None
path = snapshot_path(build_tree_snapshot(messages, cursor))
assert [message["id"] for message in initial_messages] == [
message["id"] for message in path
]
print({
"initial_messages": [message["id"] for message in initial_messages],
"cursor": cursor,
"snapshot_messages": [message["id"] for message in path],
})
PYRepository: FranciscoMoretti/chat-js
Length of output: 479
Synchronize base messages when initialTree is supplied.
createCustomChatStore accepts initialTree independently of initialMessages. A tree-only or mismatched call leaves messages, _throttledMessages, and _messageIndex based on initialMessages until updateThreadSnapshot. Initialize these fields from the selected snapshot path at construction, or make both inputs one atomic initialization object.
🤖 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 `@apps/chat/lib/stores/with-thread-state.ts` around lines 28 - 32, Update
createCustomChatStore initialization so when options.initialTree is provided,
base.messages, _throttledMessages, and _messageIndex are derived from the
created threadSnapshot rather than initialMessages. Keep all related fields
synchronized with the selected snapshot path at construction, while preserving
the existing messages-based initialization when initialTree is absent.
Contributor
There was a problem hiding this comment.
7 issues found across 20 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/chat/components/chat-sync.tsx">
<violation number="1" location="apps/chat/components/chat-sync.tsx:103">
P1: ChatSync can hit a maximum-update-depth loop immediately after mounting: this effect writes the messages that it reads from the same store, and `setMessages` publishes a fresh cloned array on every write. The bridge should compare the canonical and Chat messages by value (or otherwise suppress its own synchronization update) before calling `thread.setMessages`.</violation>
<violation number="2" location="apps/chat/components/chat-sync.tsx:103">
P2: `thread.setMessages(chat.messages)` calls `MessageTree.setPath`, which rebuilds the entire tree as a linear path and sets `cursorId` to the last message on every update of `chat.messages` (which fires on each throttled stream tick). This resets the cursor to the newest message and rewrites the branch structure on every streaming/sync delta, which conflicts with the PR's stated goal of preserving sibling topology and navigation across syncs: a user browsing an earlier/alternate branch would get snapped back to the latest linear path while a stream is producing output. Previously `addMessageToTree` only inserted the finished message into the tree without reassigning the cursor or rewriting the whole path. If this bridge must sync the full array, consider preserving the cursor when the current branch is unaffected, or gate the `setMessages` call to when the path actually changed, so navigation isn't clobbered during streaming.</violation>
</file>
<file name="apps/chat/lib/stores/hooks-threads.ts">
<violation number="1" location="apps/chat/lib/stores/hooks-threads.ts:91">
P2: Sibling navigation wraps from the first/last branch to the opposite end, contrary to the UI's disabled boundary controls. Keeping out-of-range moves as no-ops would prevent a stale click or future caller from unexpectedly jumping to an unrelated branch.</violation>
</file>
<file name="apps/chat/lib/stores/with-thread-state.ts">
<violation number="1" location="apps/chat/lib/stores/with-thread-state.ts:26">
P1: Legacy store mutations can diverge the visible messages from the canonical thread snapshot: `_syncState`/`setMessages`/`reset` update `messages` without updating `threadSnapshot`. For example, reset clears the rendered list while `ApplicationThread` still reads the old tree, and a later thread mutation can resurrect those messages; routing these mutations through a snapshot update (or wrapping the base actions here) would keep both representations consistent.</violation>
<violation number="2" location="apps/chat/lib/stores/with-thread-state.ts:30">
P2: When initialTree is supplied but differs from initialMessages, base.messages/_throttledMessages/_messageIndex remain derived from initialMessages while threadSnapshot is built from initialTree, so the store's visible messages can be out of sync with the canonical snapshot until updateThreadSnapshot is first called. Consider deriving messages/_throttledMessages/_messageIndex from the same selected snapshot path at construction, or merging initialMessages/initialTree into a single atomic initialization step.</violation>
<violation number="3" location="apps/chat/lib/stores/with-thread-state.ts:41">
P2: `updateThreadSnapshot` writes `_throttledMessages` to the new snapshot's messages synchronously on every update, bypassing the throttle built into the base store (`MESSAGES_THROTTLE_MS = 16` and `throttledMessagesUpdater`). Every other mutation path (`setMessages`, `pushMessage`, `replaceMessage`, `_syncState`, …) defers `_throttledMessages` updates to a rAF/idle callback so streaming token updates render at ~60fps. Routing the controller's snapshot updates straight into `_throttledMessages` removes that guard, so `getThrottledMessages` consumers re-render on every snapshot commit instead of on the throttled cadence. If the controller emits many snapshot updates per frame during streaming, this can regress render performance versus the prior throttled behavior.</violation>
</file>
<file name="apps/chat/lib/thread-utils.ts">
<violation number="1" location="apps/chat/lib/thread-utils.ts:125">
P2: The fallback pass for missing/cyclic parents can detach a message from a parent that is actually present in `allMessages`, which contradicts the PR goal of preserving hidden branches and sibling topology. Because the pass iterates the original `allMessages` array order and re-roots each not-yet-visited message at `null`, a child can be visited (and permanently null-rooted) before its own parent. Example: message Q is present in the list but Q's own parent is missing (so Q is not reachable from a root in the first pass), and Q's child X appears earlier in the array. `visit(X, null)` runs first and pushes `{X, null}`, then `visit(Q, null)` runs later and skips X as already visited — so Q ends up a separate root and X loses its Q edge. After `createThreadStateSnapshot({ initialTree })` restores this, `getPath(cursorId)` truncates the visible thread at X, while `useChatSystemInitialState` computes `initialMessages` via `getDefaultThread`, which walks the real parent chain and still includes Q — so the tree-derived messages and `initialMessages` diverge for the exact broken-parent/hidden-branch case this code set out to stabilize. Consider repairing the parent chain (visiting a message's ancestor before the message) in the fallback rather than blindly re-rooting at null, so present parent edges are preserved regardless of array order.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // PR #241 replaces this compatibility bridge by mounting useThread on the | ||
| // same ApplicationThread controller. | ||
| useEffect(() => { | ||
| thread.setMessages(chat.messages); |
Contributor
There was a problem hiding this comment.
P1: ChatSync can hit a maximum-update-depth loop immediately after mounting: this effect writes the messages that it reads from the same store, and setMessages publishes a fresh cloned array on every write. The bridge should compare the canonical and Chat messages by value (or otherwise suppress its own synchronization update) before calling thread.setMessages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/chat/components/chat-sync.tsx, line 103:
<comment>ChatSync can hit a maximum-update-depth loop immediately after mounting: this effect writes the messages that it reads from the same store, and `setMessages` publishes a fresh cloned array on every write. The bridge should compare the canonical and Chat messages by value (or otherwise suppress its own synchronization update) before calling `thread.setMessages`.</comment>
<file context>
@@ -96,6 +97,12 @@ export function ChatSync({ id }: { id: string }) {
+ // PR #241 replaces this compatibility bridge by mounting useThread on the
+ // same ApplicationThread controller.
+ useEffect(() => {
+ thread.setMessages(chat.messages);
+ }, [chat.messages, thread]);
+
</file context>
| options: { initialTree?: MessageTreeSnapshot<TMessage> } = {} | ||
| ): StateCreator<TState & ThreadStateStore<TMessage>, [], []> => | ||
| (set, get, api) => { | ||
| const base = creator(set, get, api); |
Contributor
There was a problem hiding this comment.
P1: Legacy store mutations can diverge the visible messages from the canonical thread snapshot: _syncState/setMessages/reset update messages without updating threadSnapshot. For example, reset clears the rendered list while ApplicationThread still reads the old tree, and a later thread mutation can resurrect those messages; routing these mutations through a snapshot update (or wrapping the base actions here) would keep both representations consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/chat/lib/stores/with-thread-state.ts, line 26:
<comment>Legacy store mutations can diverge the visible messages from the canonical thread snapshot: `_syncState`/`setMessages`/`reset` update `messages` without updating `threadSnapshot`. For example, reset clears the rendered list while `ApplicationThread` still reads the old tree, and a later thread mutation can resurrect those messages; routing these mutations through a snapshot update (or wrapping the base actions here) would keep both representations consistent.</comment>
<file context>
@@ -0,0 +1,50 @@
+ options: { initialTree?: MessageTreeSnapshot<TMessage> } = {}
+ ): StateCreator<TState & ThreadStateStore<TMessage>, [], []> =>
+ (set, get, api) => {
+ const base = creator(set, get, api);
+
+ return {
</file context>
Comment on lines
+91
to
+92
| const target = | ||
| siblings[(currentIndex + offset + siblings.length) % siblings.length]; |
Contributor
There was a problem hiding this comment.
P2: Sibling navigation wraps from the first/last branch to the opposite end, contrary to the UI's disabled boundary controls. Keeping out-of-range moves as no-ops would prevent a stale click or future caller from unexpectedly jumping to an unrelated branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/chat/lib/stores/hooks-threads.ts, line 91:
<comment>Sibling navigation wraps from the first/last branch to the opposite end, contrary to the UI's disabled boundary controls. Keeping out-of-range moves as no-ops would prevent a stale click or future caller from unexpectedly jumping to an unrelated branch.</comment>
<file context>
@@ -1,131 +1,172 @@
+ return null;
+ }
+ const offset = direction === "next" ? 1 : -1;
+ const target =
+ siblings[(currentIndex + offset + siblings.length) % siblings.length];
+ const leaf = thread.getLeaves(target.id).at(-1) ?? target;
</file context>
Suggested change
| const target = | |
| siblings[(currentIndex + offset + siblings.length) % siblings.length]; | |
| const nextIndex = currentIndex + offset; | |
| if (nextIndex < 0 || nextIndex >= siblings.length) { | |
| return null; | |
| } | |
| const target = siblings[nextIndex]; |
|
|
||
| // Missing or cyclic parent metadata should not make messages disappear. | ||
| for (const message of allMessages) { | ||
| visit(message, null); |
Contributor
There was a problem hiding this comment.
P2: The fallback pass for missing/cyclic parents can detach a message from a parent that is actually present in allMessages, which contradicts the PR goal of preserving hidden branches and sibling topology. Because the pass iterates the original allMessages array order and re-roots each not-yet-visited message at null, a child can be visited (and permanently null-rooted) before its own parent. Example: message Q is present in the list but Q's own parent is missing (so Q is not reachable from a root in the first pass), and Q's child X appears earlier in the array. visit(X, null) runs first and pushes {X, null}, then visit(Q, null) runs later and skips X as already visited — so Q ends up a separate root and X loses its Q edge. After createThreadStateSnapshot({ initialTree }) restores this, getPath(cursorId) truncates the visible thread at X, while useChatSystemInitialState computes initialMessages via getDefaultThread, which walks the real parent chain and still includes Q — so the tree-derived messages and initialMessages diverge for the exact broken-parent/hidden-branch case this code set out to stabilize. Consider repairing the parent chain (visiting a message's ancestor before the message) in the fallback rather than blindly re-rooting at null, so present parent edges are preserved regardless of array order.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/chat/lib/thread-utils.ts, line 125:
<comment>The fallback pass for missing/cyclic parents can detach a message from a parent that is actually present in `allMessages`, which contradicts the PR goal of preserving hidden branches and sibling topology. Because the pass iterates the original `allMessages` array order and re-roots each not-yet-visited message at `null`, a child can be visited (and permanently null-rooted) before its own parent. Example: message Q is present in the list but Q's own parent is missing (so Q is not reachable from a root in the first pass), and Q's child X appears earlier in the array. `visit(X, null)` runs first and pushes `{X, null}`, then `visit(Q, null)` runs later and skips X as already visited — so Q ends up a separate root and X loses its Q edge. After `createThreadStateSnapshot({ initialTree })` restores this, `getPath(cursorId)` truncates the visible thread at X, while `useChatSystemInitialState` computes `initialMessages` via `getDefaultThread`, which walks the real parent chain and still includes Q — so the tree-derived messages and `initialMessages` diverge for the exact broken-parent/hidden-branch case this code set out to stabilize. Consider repairing the parent chain (visiting a message's ancestor before the message) in the fallback rather than blindly re-rooting at null, so present parent edges are preserved regardless of array order.</comment>
<file context>
@@ -91,6 +94,40 @@ export function getDefaultThread<T extends MessageNode>(allMessages: T[]): T[] {
+
+ // Missing or cyclic parent metadata should not make messages disappear.
+ for (const message of allMessages) {
+ visit(message, null);
+ }
+
</file context>
| // PR #241 replaces this compatibility bridge by mounting useThread on the | ||
| // same ApplicationThread controller. | ||
| useEffect(() => { | ||
| thread.setMessages(chat.messages); |
Contributor
There was a problem hiding this comment.
P2: thread.setMessages(chat.messages) calls MessageTree.setPath, which rebuilds the entire tree as a linear path and sets cursorId to the last message on every update of chat.messages (which fires on each throttled stream tick). This resets the cursor to the newest message and rewrites the branch structure on every streaming/sync delta, which conflicts with the PR's stated goal of preserving sibling topology and navigation across syncs: a user browsing an earlier/alternate branch would get snapped back to the latest linear path while a stream is producing output. Previously addMessageToTree only inserted the finished message into the tree without reassigning the cursor or rewriting the whole path. If this bridge must sync the full array, consider preserving the cursor when the current branch is unaffected, or gate the setMessages call to when the path actually changed, so navigation isn't clobbered during streaming.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/chat/components/chat-sync.tsx, line 103:
<comment>`thread.setMessages(chat.messages)` calls `MessageTree.setPath`, which rebuilds the entire tree as a linear path and sets `cursorId` to the last message on every update of `chat.messages` (which fires on each throttled stream tick). This resets the cursor to the newest message and rewrites the branch structure on every streaming/sync delta, which conflicts with the PR's stated goal of preserving sibling topology and navigation across syncs: a user browsing an earlier/alternate branch would get snapped back to the latest linear path while a stream is producing output. Previously `addMessageToTree` only inserted the finished message into the tree without reassigning the cursor or rewriting the whole path. If this bridge must sync the full array, consider preserving the cursor when the current branch is unaffected, or gate the `setMessages` call to when the path actually changed, so navigation isn't clobbered during streaming.</comment>
<file context>
@@ -96,6 +97,12 @@ export function ChatSync({ id }: { id: string }) {
+ // PR #241 replaces this compatibility bridge by mounting useThread on the
+ // same ApplicationThread controller.
+ useEffect(() => {
+ thread.setMessages(chat.messages);
+ }, [chat.messages, thread]);
+
</file context>
| return { | ||
| ...state, | ||
| _memoizedSelectors: new Map(), | ||
| _throttledMessages: threadSnapshot.messages, |
Contributor
There was a problem hiding this comment.
P2: updateThreadSnapshot writes _throttledMessages to the new snapshot's messages synchronously on every update, bypassing the throttle built into the base store (MESSAGES_THROTTLE_MS = 16 and throttledMessagesUpdater). Every other mutation path (setMessages, pushMessage, replaceMessage, _syncState, …) defers _throttledMessages updates to a rAF/idle callback so streaming token updates render at ~60fps. Routing the controller's snapshot updates straight into _throttledMessages removes that guard, so getThrottledMessages consumers re-render on every snapshot commit instead of on the throttled cadence. If the controller emits many snapshot updates per frame during streaming, this can regress render performance versus the prior throttled behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/chat/lib/stores/with-thread-state.ts, line 41:
<comment>`updateThreadSnapshot` writes `_throttledMessages` to the new snapshot's messages synchronously on every update, bypassing the throttle built into the base store (`MESSAGES_THROTTLE_MS = 16` and `throttledMessagesUpdater`). Every other mutation path (`setMessages`, `pushMessage`, `replaceMessage`, `_syncState`, …) defers `_throttledMessages` updates to a rAF/idle callback so streaming token updates render at ~60fps. Routing the controller's snapshot updates straight into `_throttledMessages` removes that guard, so `getThrottledMessages` consumers re-render on every snapshot commit instead of on the throttled cadence. If the controller emits many snapshot updates per frame during streaming, this can regress render performance versus the prior throttled behavior.</comment>
<file context>
@@ -0,0 +1,50 @@
+ return {
+ ...state,
+ _memoizedSelectors: new Map(),
+ _throttledMessages: threadSnapshot.messages,
+ error: threadSnapshot.error,
+ messages: threadSnapshot.messages,
</file context>
|
|
||
| return { | ||
| ...base, | ||
| threadSnapshot: options.initialTree |
Contributor
There was a problem hiding this comment.
P2: When initialTree is supplied but differs from initialMessages, base.messages/_throttledMessages/_messageIndex remain derived from initialMessages while threadSnapshot is built from initialTree, so the store's visible messages can be out of sync with the canonical snapshot until updateThreadSnapshot is first called. Consider deriving messages/_throttledMessages/_messageIndex from the same selected snapshot path at construction, or merging initialMessages/initialTree into a single atomic initialization step.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/chat/lib/stores/with-thread-state.ts, line 30:
<comment>When initialTree is supplied but differs from initialMessages, base.messages/_throttledMessages/_messageIndex remain derived from initialMessages while threadSnapshot is built from initialTree, so the store's visible messages can be out of sync with the canonical snapshot until updateThreadSnapshot is first called. Consider deriving messages/_throttledMessages/_messageIndex from the same selected snapshot path at construction, or merging initialMessages/initialTree into a single atomic initialization step.</comment>
<file context>
@@ -0,0 +1,50 @@
+
+ return {
+ ...base,
+ threadSnapshot: options.initialTree
+ ? createThreadStateSnapshot({ initialTree: options.initialTree })
+ : createThreadStateSnapshot({ messages: base.messages }),
</file context>
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\n- refactor the ChatJS store to retain ordered, canonical thread snapshots\n- derive visible messages, sibling order, and navigation indexes from that snapshot\n- preserve current store behavior ahead of mounting \n\n## Verification\n- Chat store unit tests pass at the stack tip\n\nSupersedes #240 after inserting Threads 9 beneath this layer; the original PR retains its review discussion history.
Summary by Sourcery
Refactor the chat thread store to use canonical message tree snapshots as the source of truth for navigation and visible threads.
New Features:
Bug Fixes:
Enhancements:
Build:
Summary by cubic
Backed ChatJS threads with a canonical snapshot state via
@chatjs/thread, adding anApplicationThreadcontroller and initializing threads from an orderedinitialTreebuilt from server messages. This unifies navigation, streaming, and sync around one snapshot shared between the controller and the store.Refactors
with-threadswith snapshot-drivenwith-thread-stateplusZustandThreadState; addApplicationThreadand a provider context.initialTreethrough route, runtime, andChatSystem; runtime now exposesthreadviagetAppRuntimeThread.ChatSyncand thread hooks to read/write throughApplicationThread(message upserts, cursor selection, sibling switching) and mirror visible messages back to the controller.buildTreeSnapshotFromMessagesfor ordered tree creation; exportcreateThreadStateSnapshotfrom@chatjs/threadand add tests for Zustand-backed thread state and tree utilities.Dependencies
@chatjs/thread; CLI templates vendor thread sources locally, rewrite imports to@/lib/thread, and strip the dependency and prebuild step.Written for commit f469048. Summary will update on new commits.
Stack
Summary by CodeRabbit
Bug Fixes
Refactor
New Features