fix(ai-client): add missing methods to the no-op chat devtools bridge#752
Open
jan-kubica wants to merge 2 commits into
Open
fix(ai-client): add missing methods to the no-op chat devtools bridge#752jan-kubica wants to merge 2 commits into
jan-kubica wants to merge 2 commits into
Conversation
The default NoOpChatDevtoolsBridge was missing mountWithTools, notifyToolsChanged, and recordStreamId, so the first ChatClient.sendMessage() threw a TypeError before appending the user message whenever no devtoolsBridgeFactory was supplied. Subsequent sends appeared to work because mountDevtools() marks itself done before calling the missing method. Rework the compile-time parity check so drift between the real and no-op bridge surfaces fails the build: the previous form assigned 'undefined as never' to the missing-keys type, which always typechecks because never is assignable to every type. Add a unit test that unmocks the suite-wide real-bridge shim so the shipping default path is exercised.
…ridge The framework hooks always inject the real devtools bridge factory, so no existing E2E route exercised the no-op bridge that direct ChatClient consumers get by default.
Contributor
📝 WalkthroughWalkthroughThis PR fixes a runtime error in ChangesNo-op Devtools Bridge Fix and E2E Validation
Sequence DiagramsequenceDiagram
participant ChatClient
participant NoOpChatDevtoolsBridge
participant SSEFetcher
participant UI
ChatClient->>NoOpChatDevtoolsBridge: mountWithTools(initialCount)
ChatClient->>NoOpChatDevtoolsBridge: recordStreamId(streamId)
ChatClient->>SSEFetcher: fetch deterministic events
SSEFetcher-->>ChatClient: user message event
ChatClient->>NoOpChatDevtoolsBridge: notifyToolsChanged()
ChatClient->>UI: onMessagesChange callback
SSEFetcher-->>ChatClient: assistant message event
ChatClient->>UI: onMessagesChange callback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 1
🤖 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 `@testing/e2e/tests/chat-client-default-bridge.spec.ts`:
- Around line 3-5: Update the header comment above the test that exercises the
no-op bridge (referenced by ChatClient and the framework hooks that pass the
real devtools bridge factory) to explicitly document an aimock policy exception:
state that this spec uses a mock fetcher (a static SSE Response served from the
route file) and therefore never reaches the provider HTTP layer, so aimock is
intentionally not used; include brief rationale and link the exception to the
mock fetcher/SSE Response and the no-op bridge behavior so future readers
understand why aimock is skipped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a61c5231-635d-4ac9-aa2e-af0854eacc66
📒 Files selected for processing (6)
.changeset/noop-chat-devtools-bridge-parity.mdpackages/ai-client/src/devtools-noop.tspackages/ai-client/tests/chat-client-noop-devtools.test.tstesting/e2e/src/routeTree.gen.tstesting/e2e/src/routes/chat-client-default-bridge.tsxtesting/e2e/tests/chat-client-default-bridge.spec.ts
Comment on lines
+3
to
+5
| // The framework hooks always pass the real devtools bridge factory, so this | ||
| // is the only scenario in the suite that exercises the no-op bridge that | ||
| // vanilla `ChatClient` consumers get by default. |
Contributor
There was a problem hiding this comment.
Document the aimock policy exception in the header comment.
Based on learnings, E2E specs that don't reach the provider HTTP layer should document why aimock is not used. This test uses a mock fetcher (static SSE Response in the route file) that never reaches the provider HTTP layer, qualifying as an exception.
📝 Suggested header comment expansion
+// Policy exception: This test uses a mock fetcher (static SSE Response) that
+// never reaches the provider HTTP layer, so aimock is not needed. The route
+// component's inline fetcher returns deterministic SSE events to test the
+// default no-op bridge behavior without external HTTP dependencies.
+
// The framework hooks always pass the real devtools bridge factory, so this
// is the only scenario in the suite that exercises the no-op bridge that
// vanilla `ChatClient` consumers get by default.📝 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
| // The framework hooks always pass the real devtools bridge factory, so this | |
| // is the only scenario in the suite that exercises the no-op bridge that | |
| // vanilla `ChatClient` consumers get by default. | |
| // Policy exception: This test uses a mock fetcher (static SSE Response) that | |
| // never reaches the provider HTTP layer, so aimock is not needed. The route | |
| // component's inline fetcher returns deterministic SSE events to test the | |
| // default no-op bridge behavior without external HTTP dependencies. | |
| // The framework hooks always pass the real devtools bridge factory, so this | |
| // is the only scenario in the suite that exercises the no-op bridge that | |
| // vanilla `ChatClient` consumers get by default. |
🤖 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 `@testing/e2e/tests/chat-client-default-bridge.spec.ts` around lines 3 - 5,
Update the header comment above the test that exercises the no-op bridge
(referenced by ChatClient and the framework hooks that pass the real devtools
bridge factory) to explicitly document an aimock policy exception: state that
this spec uses a mock fetcher (a static SSE Response served from the route file)
and therefore never reaches the provider HTTP layer, so aimock is intentionally
not used; include brief rationale and link the exception to the mock fetcher/SSE
Response and the no-op bridge behavior so future readers understand why aimock
is skipped.
Source: Learnings
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.
🎯 Changes
Repro of the issue: https://stackblitz.com/github/jan-kubica/tanstack-ai-noop-bridge-repro?file=index.mjs
If you create a
ChatClientwithout adevtoolsBridgeFactoryyou get the bundled no-op bridge, and that class is missing three methods the client expects:mountWithTools,notifyToolsChangedandrecordStreamId.So the first
sendMessage()errors with:It throws before
addUserMessage, so the message is just gone. The second send works fine, becausemountDevtools()flipsdevtoolsMountedbefore calling the method that doesn't exist, so it may look like a transient issue.The parity check at the bottom of
devtools-noop.tswas supposed to catch this, but it assignsundefined as neverto the missing-keys type, and sinceneveris assignable to anything, that line can't fail no matter how out of sync the no-op gets. I suggest to change the direction: the missing-keys union now has to satisfy anextends neverconstraint, so a stale no-op fails tsc withType '"mountWithTools"' does not satisfy the constraint 'never'.Test plan
packages/ai-client:pnpm test:lib(the newchat-client-noop-devtools.test.tsfails on main with the TypeError above, passes here)pnpm test:pris green locallypnpm --filter @tanstack/ai-e2e exec playwright test chat-client-default-bridge✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
ChatClientthrowing an error when using the default configuration without a custom devtools bridge.sendMessage()orupdateOptions()on first use with the default configuration.Tests