Threads 2: Add the canonical message tree - #235
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. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @FranciscoMoretti, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThread package updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 490c5c20b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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
f7c7b6f to
cc47844
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/thread/src/message-tree.ts (2)
177-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated "cannot move" check; inconsistent privacy style on
validatePath.The existing-parent conflict check is duplicated verbatim between
upsertMessage(177-182) andvalidatePath(270-277) — a future edit to one message/format is likely to miss the other. Also,validatePathuses TSprivatewhile every other member uses true#privatefields; for consistency it should use#validatePath.♻️ Extract shared check
+ `#assertNotReparenting`(messageId: string, parentId: string | null) { + const existingParentId = this.#parentById.get(messageId); + if (existingParentId !== undefined && existingParentId !== parentId) { + throw new Error( + `Cannot move message ${messageId} from ${existingParentId ?? "root"} to ${parentId ?? "root"}`, + ); + } + }Then call
this.#assertNotReparenting(message.id, parentId)in bothupsertMessageandvalidatePath(renaming it#validatePathfor consistency with the other private fields).Also applies to: 262-280
🤖 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 `@packages/thread/src/message-tree.ts` around lines 177 - 182, Extract the duplicated existing-parent conflict validation into a shared `#assertNotReparenting` method, preserving the current error message and behavior, then call it from both upsertMessage and validatePath. Rename validatePath to `#validatePath` and update all references to use the true private method syntax.
63-83: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRecursive traversal risks stack overflow on deep chains.
getLeavesandgetSnapshot'svisitrecurse once per tree depth level. For very long single-branch conversations (long-running threads without forks), this can approach the call-stack limit. Converting to an explicit-stack iterative traversal removes this risk entirely.♻️ Example iterative rewrite for getLeaves
getLeaves(messageId: string | null = null) { const leaves: TMessage[] = []; - const visit = (id: string) => { - const children = this.#childrenByParentId.get(id) ?? []; - if (children.length === 0) { - const message = this.#messagesById.get(id); - if (message) { - leaves.push(clone(message)); - } - return; - } - for (const childId of children) { - visit(childId); - } - }; - - for (const childId of this.#childrenByParentId.get(messageId) ?? []) { - visit(childId); - } + const stack = [...(this.#childrenByParentId.get(messageId) ?? [])]; + while (stack.length > 0) { + const id = stack.pop() as string; + const children = this.#childrenByParentId.get(id) ?? []; + if (children.length === 0) { + const message = this.#messagesById.get(id); + if (message) leaves.push(clone(message)); + } else { + stack.push(...children); + } + } return leaves; }The same pattern (explicit stack, preserving pre-order for
getSnapshot) applies to thevisitingetSnapshot.Also applies to: 108-127
🤖 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 `@packages/thread/src/message-tree.ts` around lines 63 - 83, Replace the recursive visit traversal in getLeaves with an explicit stack, preserving leaf collection order and cloning behavior. Apply the same iterative traversal pattern to getSnapshot’s visit, pushing children in reverse order where needed to preserve its existing pre-order output, and remove recursion so deep message chains cannot overflow the call stack.
🤖 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.
Nitpick comments:
In `@packages/thread/src/message-tree.ts`:
- Around line 177-182: Extract the duplicated existing-parent conflict
validation into a shared `#assertNotReparenting` method, preserving the current
error message and behavior, then call it from both upsertMessage and
validatePath. Rename validatePath to `#validatePath` and update all references to
use the true private method syntax.
- Around line 63-83: Replace the recursive visit traversal in getLeaves with an
explicit stack, preserving leaf collection order and cloning behavior. Apply the
same iterative traversal pattern to getSnapshot’s visit, pushing children in
reverse order where needed to preserve its existing pre-order output, and remove
recursion so deep message chains cannot overflow the call stack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44b2b814-f34b-4820-a41a-f2d16fab8975
📒 Files selected for processing (5)
packages/thread/package.jsonpackages/thread/src/index.tspackages/thread/src/message-tree.tspackages/thread/src/message-utils.tspackages/thread/test/message-tree.test.ts
cc47844 to
267e6cb
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| mergePath(messages: TMessage[], options: { moveCursor?: boolean } = {}) { | ||
| this.validatePath(messages, true); | ||
| let parentId: string | null = null; | ||
| for (const message of messages) { | ||
| this.upsertMessage(message, parentId); | ||
| parentId = message.id; | ||
| } | ||
| if (options.moveCursor ?? true) { | ||
| this.#cursorId = messages.at(-1)?.id ?? null; | ||
| } |
There was a problem hiding this comment.
mergePath([]) silently resets cursor to null
When messages is an empty array, validatePath and the insertion loop are both no-ops, but messages.at(-1)?.id ?? null evaluates to null, so this.#cursorId is unconditionally overwritten with null. Any caller that passes a dynamically-computed, possibly-empty path will silently lose the active cursor position without any structural change to the tree.
Concretely: if the cursor is "u3" and mergePath([]) is called (e.g. a filtered path returned no messages), the cursor becomes null and getPath() returns [] on the next call, even though the tree is intact.
267e6cb to
30c82db
Compare
30c82db to
51f7c89
Compare
51f7c89 to
ac50cdf
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/thread/src/message-tree.ts (2)
44-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle empty-string message IDs explicitly.
upsertMessageaccepts anystringID, including"", but these truthiness checks treat an empty ID as absent. Such a message can be stored and selected, yetgetParent()andgetPath()return incomplete results. Use explicitnull/undefinedchecks and add an empty-ID test.Proposed fix
- return parentId ? this.getMessage(parentId) : undefined; + return parentId == null ? undefined : this.getMessage(parentId); - if (!messageId) { + if (messageId == null) { return []; } - while (currentId) { + while (currentId !== null) {Also applies to: 85-97
🤖 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 `@packages/thread/src/message-tree.ts` around lines 44 - 47, Update getParent and the related getPath logic to check parent/message IDs explicitly for null or undefined rather than by truthiness, so an empty-string ID is preserved and resolved like any other valid string ID. Add a test covering an empty-ID message through both parent lookup and path traversal.
234-242: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject unsupported snapshot versions before restoration.
getSnapshot()writesversion: 1, butrestore()ignores the version entirely. A future or incompatible persisted snapshot can therefore be interpreted as v1 and restore incorrect state instead of failing clearly.Proposed fix
restore(snapshot: MessageTreeSnapshot<TMessage>) { + if (snapshot.version !== 1) { + throw new Error(`Unsupported message tree snapshot version ${snapshot.version}`); + } const restored = new MessageTree<TMessage>();🤖 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 `@packages/thread/src/message-tree.ts` around lines 234 - 242, Update MessageTree.restore to validate snapshot.version before iterating snapshot.nodes or applying snapshot.cursorId. Accept the currently supported version 1 and throw a clear error for any unsupported or future version, preventing incompatible snapshots from being restored.
🤖 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.
Outside diff comments:
In `@packages/thread/src/message-tree.ts`:
- Around line 44-47: Update getParent and the related getPath logic to check
parent/message IDs explicitly for null or undefined rather than by truthiness,
so an empty-string ID is preserved and resolved like any other valid string ID.
Add a test covering an empty-ID message through both parent lookup and path
traversal.
- Around line 234-242: Update MessageTree.restore to validate snapshot.version
before iterating snapshot.nodes or applying snapshot.cursorId. Accept the
currently supported version 1 and throw a clear error for any unsupported or
future version, preventing incompatible snapshots from being restored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 376972f2-cf1d-40d5-8f35-bfc473e84ac4
📒 Files selected for processing (2)
packages/thread/src/message-tree.tspackages/thread/test/message-tree.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/thread/test/message-tree.test.ts
1 participant
Summary
Behavior
Adds the headless topology layer; no network or React behavior.
Verification
Review focus
Tree invariants, snapshot round trips, and path reconciliation.
Summary by cubic
Adds a canonical, headless message tree with navigation, safe mutations, and v1 snapshot save/restore using ordered traversal. Separates path updates from selection and removes root-sentinel collisions; no network or React changes.
New Features
nullparent andcursorId;getPath/getPathIds,getSiblings/getLeaves,getParent/getChildren.upsertMessage(optional sibling index, no cross-parent moves),removeLeaf(moves cursor), cursor setters,setPath(update + select),updatePath(update only).getSnapshot()using a single tree traversal and strictrestore(rejects unordered/duplicate nodes).getMessageText,getIndexes().Refactors & Bug Fixes
__root__work;getIndexes().rootIdsshows real roots.testinformat/lint; exportedgetMessageText.Written for commit 81c2cb5. Summary will update on new commits.
Summary by CodeRabbit
getMessageTextto extract combined text from message parts, and re-exported it from the thread package.setPath/updatePathfor selected-path management.removeLeafto leaf nodes.Stack