Threads 8: Add externally owned thread state - #262
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. |
Reviewer's GuideIntroduce an AbstractThread controller that orchestrates runs against a pluggable ThreadState, keep Thread as the default in-memory implementation via MemoryThreadState, and update docs/tests/types so useThread can subscribe to externally owned thread state safely. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/thread/test/use-thread.test.ts" line_range="183" />
<code_context>
+ describe("useThread", () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a runtime test that `useThread` can work with an externally owned Thread/state instance.
The types guarantee that `useThread` accepts an externally owned `Thread` with a custom `ThreadState`, but there’s no runtime test covering this. Please add a test that:
- Constructs a `Thread` with a custom `ThreadState` (e.g. `new Thread({ state: new MemoryThreadState({ messages: [...] }) })`).
- Passes that thread into `renderUseThread({ thread })`.
- Verifies the hook snapshot reflects the external messages/cursor, and that changes via `thread.setCursor(...)` / `thread.sendMessage(...)` are observed by the hook.
This confirms the hook correctly subscribes to and reacts to externally owned thread state, not just at the type level.
</issue_to_address>
### Comment 2
<location path="packages/thread/ARCHITECTURE.md" line_range="187" />
<code_context>
- the selected cursor
-- run records and per-run status
+- public run status and errors
+- the selected-path and aggregate projections
+- the immutable snapshot consumed by React
+
</code_context>
<issue_to_address>
**nitpick (typo):** The phrase "selected-path" would read more naturally as "selected path".
Unless "selected-path" is a defined term that must be hyphenated, please change it to "selected path" for standard usage and to match the parallel structure with "aggregate projections."
```suggestion
- the selected path and aggregate projections
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| transport, | ||
| }); | ||
| const hook = renderUseThread({ thread, resume: true }); | ||
| const hook = renderUseThread({ resume: true, thread }); |
There was a problem hiding this comment.
suggestion (testing): Add a runtime test that useThread can work with an externally owned Thread/state instance.
The types guarantee that useThread accepts an externally owned Thread with a custom ThreadState, but there’s no runtime test covering this. Please add a test that:
- Constructs a
Threadwith a customThreadState(e.g.new Thread({ state: new MemoryThreadState({ messages: [...] }) })). - Passes that thread into
renderUseThread({ thread }). - Verifies the hook snapshot reflects the external messages/cursor, and that changes via
thread.setCursor(...)/thread.sendMessage(...)are observed by the hook.
This confirms the hook correctly subscribes to and reacts to externally owned thread state, not just at the type level.
| - the selected cursor | ||
| - run records and per-run status | ||
| - public run status and errors | ||
| - the selected-path and aggregate projections |
There was a problem hiding this comment.
nitpick (typo): The phrase "selected-path" would read more naturally as "selected path".
Unless "selected-path" is a defined term that must be hyphenated, please change it to "selected path" for standard usage and to match the parallel structure with "aggregate projections."
| - the selected-path and aggregate projections | |
| - the selected path and aggregate projections |
Greptile SummaryThe PR separates thread orchestration from observable state ownership.
Confidence Score: 3/5The PR is not yet safe to merge because creating another controller for a shared ThreadState can make subscribers lose the first controller's active-run state. A new Thread still initializes an empty controller-local run registry and immediately publishes its values into the injected state, replacing active run metadata and reporting ready while another controller using that state may still be processing. Files Needing Attention: packages/thread/src/abstract-thread.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Hook["useThread"] --> Controller["Thread"]
Controller --> Abstract["AbstractThread"]
Abstract --> State["ThreadState"]
Abstract --> Registry["Controller-local RunRegistry"]
State --> Snapshot["Observable tree and run snapshot"]
State --> Subscribers["React and external subscribers"]
Registry --> Snapshot
Reviews (2): Last reviewed commit: "feat(thread): support externally owned t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| this.#runs = new RunRegistry(options.concurrency); | ||
| this.#state = options.state; | ||
| this.#runHost = this.createRunHost(); | ||
| this.publish(); |
There was a problem hiding this comment.
When a second Thread is created with the same external ThreadState, its fresh empty RunRegistry is immediately published over the shared snapshot, causing subscribers to see no active runs and a ready status while the original controller is still processing.
Knowledge Base Used: @chatjs/thread Package
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
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="packages/thread/src/abstract-thread.ts">
<violation number="1" location="packages/thread/src/abstract-thread.ts:277">
P1: After `restore`, persisted tool calls and approval requests cannot accept `addToolOutput` or `addToolApprovalResponse`: clearing the registry drops ownership and restored messages never reconstruct it. Rebuild ownership and lazily create the owning branch adapter before routing these helpers.
(Based on your team's feedback about restored tool ownership.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/thread/src/abstract-thread.ts:571">
P2: Selecting a persisted root assistant makes `resumeStream()` silently return rather than reconnect because root nodes have `parentMessageId: null`. Allow null parent IDs when creating the reconstructed run; `ThreadRunSpec` already supports them.</violation>
</file>
<file name="packages/thread/ARCHITECTURE.md">
<violation number="1" location="packages/thread/ARCHITECTURE.md:187">
P3: "selected-path" reads as a hyphenated compound modifier, but here it's used as a noun phrase parallel to "aggregate projections." Removing the hyphen ("selected path") would match the parallel structure.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| restore(snapshot: MessageTreeSnapshot<TMessage>) { | ||
| this.assertCanResetTree(); | ||
| this.#runs.clear(); |
There was a problem hiding this comment.
P1: After restore, persisted tool calls and approval requests cannot accept addToolOutput or addToolApprovalResponse: clearing the registry drops ownership and restored messages never reconstruct it. Rebuild ownership and lazily create the owning branch adapter before routing these helpers.
(Based on your team's feedback about restored tool ownership.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/thread/src/abstract-thread.ts, line 277:
<comment>After `restore`, persisted tool calls and approval requests cannot accept `addToolOutput` or `addToolApprovalResponse`: clearing the registry drops ownership and restored messages never reconstruct it. Rebuild ownership and lazily create the owning branch adapter before routing these helpers.
(Based on your team's feedback about restored tool ownership.) </comment>
<file context>
@@ -0,0 +1,673 @@
+
+ restore(snapshot: MessageTreeSnapshot<TMessage>) {
+ this.assertCanResetTree();
+ this.#runs.clear();
+ this.updateTree((tree) => tree.restore(snapshot));
+ }
</file context>
| if (!message || message.role !== "assistant") { | ||
| return; | ||
| } | ||
| const parentMessageId = tree.getParentId(messageId); |
There was a problem hiding this comment.
P2: Selecting a persisted root assistant makes resumeStream() silently return rather than reconnect because root nodes have parentMessageId: null. Allow null parent IDs when creating the reconstructed run; ThreadRunSpec already supports them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/thread/src/abstract-thread.ts, line 571:
<comment>Selecting a persisted root assistant makes `resumeStream()` silently return rather than reconnect because root nodes have `parentMessageId: null`. Allow null parent IDs when creating the reconstructed run; `ThreadRunSpec` already supports them.</comment>
<file context>
@@ -0,0 +1,673 @@
+ if (!message || message.role !== "assistant") {
+ return;
+ }
+ const parentMessageId = tree.getParentId(messageId);
+ if (!parentMessageId) return;
+
</file context>
| - the selected cursor | ||
| - run records and per-run status | ||
| - public run status and errors | ||
| - the selected-path and aggregate projections |
There was a problem hiding this comment.
P3: "selected-path" reads as a hyphenated compound modifier, but here it's used as a noun phrase parallel to "aggregate projections." Removing the hyphen ("selected path") would match the parallel structure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/thread/ARCHITECTURE.md, line 187:
<comment>"selected-path" reads as a hyphenated compound modifier, but here it's used as a noun phrase parallel to "aggregate projections." Removing the hyphen ("selected path") would match the parallel structure.</comment>
<file context>
@@ -133,22 +138,67 @@ React hooks or components.
- the selected cursor
-- run records and per-run status
+- public run status and errors
+- the selected-path and aggregate projections
+- the immutable snapshot consumed by React
+
</file context>
| - the selected-path and aggregate projections | |
| - the selected path and aggregate projections |
7f8f27a to
2963466
Compare
9dff780 to
9b89d72
Compare
2963466 to
a7aabe5
Compare
1 participant
Summary
AbstractThreadas the tree-aware equivalent of AI SDKAbstractChatThreadStatesubscription/update contract and provideMemoryThreadStateThreadas the default in-memory implementationuseThread({ thread })to subscribe to externally owned thread instancesVerification
bun --cwd packages/thread testbun --cwd packages/thread buildbun --cwd packages/thread test:typesbun --cwd packages/thread lintSummary by Sourcery
Introduce an abstract, state-agnostic thread controller and make Thread use pluggable thread state to support externally owned thread instances.
New Features:
Enhancements:
Documentation:
Tests:
Summary by cubic
Adds externally owned thread state with
ThreadStateandMemoryThreadState, and refactorsThreadto extend a newAbstractThread. Apps can keep the message tree in their own store;useThreadnow reads from the thread’s external state.New Features
AbstractThreadfor framework-agnostic orchestration;Threadis the default adapter.ThreadStateinterface and defaultMemoryThreadState; updates are atomic and must be synchronous.AbstractThread,MemoryThreadState,ThreadState, andAbstractThreadInit; updated docs and types.Migration
statetonew Thread({ state }); do not also passmessagesorinitialTree.ThreadState.updatemust call its updater exactly once, synchronously.Written for commit a7aabe5. Summary will update on new commits.
Stack