Add @workkit/chat-react debug frame hook#118
Conversation
Why: - implement help-wanted chat-react debug frames package for beeeku#84 - share DebugFrame types from @workkit/chat and provide focused hook tests Verified: - bun run --filter @workkit/chat-react test - bun run --filter @workkit/chat-react typecheck - bun run --filter @workkit/chat-react build - bun run --filter @workkit/chat test - bun run --filter @workkit/chat typecheck - bun x biome check packages/chat/src/index.ts packages/chat/src/types.ts packages/chat-react .changeset/quiet-frames-chat-react.md
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 1 minute and 7 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds DebugFrame types to ChangesDebug frames capture and inspection
Sequence DiagramsequenceDiagram
participant Component
participant useChatDebugFrames
participant ChatDebugSocket
participant FrameBuffer
Component->>useChatDebugFrames: mount(socket, options)
useChatDebugFrames->>ChatDebugSocket: addEventListener("message"/"open"/"close"/"error")
useChatDebugFrames->>ChatDebugSocket: installSendRecorder(socket)
ChatDebugSocket->>useChatDebugFrames: message event (data)
useChatDebugFrames->>useChatDebugFrames: parse → makeFrame
useChatDebugFrames->>FrameBuffer: append(inbound DebugFrame)
Component->>ChatDebugSocket: call send(data)
ChatDebugSocket->>useChatDebugFrames: wrapped send intercept (outbound)
useChatDebugFrames->>FrameBuffer: append(outbound DebugFrame)
useChatDebugFrames->>ChatDebugSocket: call original send(data)
ChatDebugSocket->>useChatDebugFrames: open/close/error events
useChatDebugFrames->>Component: update connectionState
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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}
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 `@packages/chat-react/src/index.ts`:
- Around line 83-87: The toMessage return currently casts wire.role directly to
ChatMessage["role"], allowing invalid runtime values; update to validate
wire.role against the allowed roles union before assigning (e.g., check if
typeof wire.role === "string" and wire.role is one of the ChatMessage role
values) and then set role to that validated value or fallback to "user" (or
throw) — locate the logic in the toMessage function where id/type/role/content
are returned and replace the direct cast of wire.role with an explicit union
check using the ChatMessage role set.
🪄 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: 112f87fa-e871-4d94-ba62-124810d06975
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.changeset/quiet-frames-chat-react.mdpackages/chat-react/CHANGELOG.mdpackages/chat-react/README.mdpackages/chat-react/bunup.config.tspackages/chat-react/package.jsonpackages/chat-react/src/index.tspackages/chat-react/tests/use-chat-debug-frames.test.tspackages/chat-react/tsconfig.jsonpackages/chat-react/vitest.config.tspackages/chat/src/index.tspackages/chat/src/types.ts
Why: - Address CodeRabbit feedback on PR beeeku#118 by preventing invalid wire roles from entering parsed debug messages. Verified: - bun run --filter @workkit/chat-react test - bun run --filter @workkit/chat-react typecheck - bun run --filter @workkit/chat-react build - bun run --filter @workkit/chat test - bun run --filter @workkit/chat typecheck - bun x biome check packages/chat-react/src/index.ts packages/chat-react/tests/use-chat-debug-frames.test.ts packages/chat/src/index.ts packages/chat/src/types.ts - bun run constitution:check --diff-only --base=origin/master - git diff --check
There was a problem hiding this comment.
Pull request overview
Adds a client-side debugging primitive for @workkit/chat by introducing shared DebugFrame types and a new @workkit/chat-react package exposing a headless useChatDebugFrames hook for capturing inbound message events and outbound socket.send calls.
Changes:
- Add
DebugFrame/DebugFrameDirectiontypes to@workkit/chatand re-export them. - Introduce
@workkit/chat-reactwithuseChatDebugFrames(buffering + include-filtering + clear + connection state). - Add focused hook tests and publish tooling/docs (README, changelog, changeset).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/chat/src/types.ts | Adds shared DebugFrame + direction types for client-side frame inspection. |
| packages/chat/src/index.ts | Re-exports the new debug frame types from @workkit/chat. |
| packages/chat-react/src/index.ts | Implements useChatDebugFrames hook (capture/parse/filter/buffer/clear/state). |
| packages/chat-react/tests/use-chat-debug-frames.test.ts | Adds tests for capturing frames, filtering/capping, role validation, and clear/state. |
| packages/chat-react/package.json | New package manifest (exports, peers, scripts, deps). |
| packages/chat-react/tsconfig.json | TS config for DOM + library build output. |
| packages/chat-react/vitest.config.ts | Vitest configuration for the new package. |
| packages/chat-react/bunup.config.ts | Build configuration for bundling + DTS output. |
| packages/chat-react/README.md | Basic usage documentation for the hook. |
| packages/chat-react/CHANGELOG.md | Initial changelog entry for v0.1.0. |
| .changeset/quiet-frames-chat-react.md | Changeset for releasing @workkit/chat (patch) and @workkit/chat-react (minor). |
| bun.lock | Lockfile updates to include the new package and dependency graph changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const role = isChatMessageRole(wire.role) ? wire.role : "user"; | ||
| return { | ||
| id: typeof wire.id === "string" ? wire.id : "", | ||
| type: wire.type as ChatMessageType, | ||
| role, | ||
| content: wire.content, | ||
| metadata: | ||
| typeof wire.metadata === "object" && wire.metadata !== null && !Array.isArray(wire.metadata) | ||
| ? (wire.metadata as Record<string, unknown>) | ||
| : undefined, | ||
| timestamp: Date.now(), | ||
| }; |
| let nextFrameId = 0; | ||
|
|
| function bytesFor(data: unknown): number { | ||
| if (typeof data === "string") { | ||
| return new TextEncoder().encode(data).byteLength; | ||
| } |
| socket: ChatDebugSocket | null | undefined, | ||
| options: UseChatDebugFramesOptions = {}, | ||
| ): UseChatDebugFramesResult { | ||
| const bufferSize = Math.max(1, Math.floor(options.bufferSize ?? DEFAULT_BUFFER_SIZE)); |
|
Great PR @Chris79OG ! Appreciate the help . The code looks great for first time contribution , please fix the copilot review comments and we need to update the readme for the new packages and docs site too . If you can attach visual verification proofs that would be great too . |
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 (1)
packages/chat-react/src/index.ts (1)
195-210:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid clobbering another hook's
sendwrapper on cleanup.Line 195 binds whatever
socket.sendis currently installed, so twouseChatDebugFramesinstances on the same socket stack wrappers. If the earlier hook unmounts first, Line 208 can disable the later hook; if the later hook unmounts last, it restores the earlier wrapper and leaves a patchedsendbehind after both hooks are gone. This needs shared per-socket patching/ref-counting instead of per-instance replacement. A same-socket double-mount regression test would catch it.🤖 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/chat-react/src/index.ts` around lines 195 - 210, The current per-instance replacement of socket.send (using originalSendRef and setting socket.send directly in useChatDebugFrames) can clobber other hook instances; replace this with a shared per-socket registry (e.g. a WeakMap keyed by the socket) that stores {refCount, originalSend, wrapperSend} so mounting increments refCount and installs a single wrapper that calls recordFrame("out", data) then delegates to originalSend, and unmounting decrements refCount and only restores socket.send to originalSend when refCount reaches 0; update references to originalSendRef/current behavior to use the registry for both install and cleanup so multiple useChatDebugFrames on the same socket stack cleanly.
🤖 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/chat-react/src/index.ts`:
- Around line 195-210: The current per-instance replacement of socket.send
(using originalSendRef and setting socket.send directly in useChatDebugFrames)
can clobber other hook instances; replace this with a shared per-socket registry
(e.g. a WeakMap keyed by the socket) that stores {refCount, originalSend,
wrapperSend} so mounting increments refCount and installs a single wrapper that
calls recordFrame("out", data) then delegates to originalSend, and unmounting
decrements refCount and only restores socket.send to originalSend when refCount
reaches 0; update references to originalSendRef/current behavior to use the
registry for both install and cleanup so multiple useChatDebugFrames on the same
socket stack cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11bf1c4b-7a16-469c-9e55-4d7b4d80ab97
📒 Files selected for processing (2)
packages/chat-react/src/index.tspackages/chat-react/tests/use-chat-debug-frames.test.ts
|
Addressed the latest review/maintainer feedback in commit 22ac9c2.\n\nWhat changed:\n- replaced per-hook send replacement with a shared per-socket send recorder registry, so multiple hook instances on the same socket no longer clobber each other on cleanup\n- added a same-socket double-mount regression test covering recorder isolation and original send restoration\n- added @workkit/chat and @workkit/chat-react to the root README plus docs index, getting-started install list, and API reference\n\nVerification proofs:\n- bun run --filter @workkit/chat build\n- bun run --filter @workkit/chat-react test: 7 passed\n- bun run --filter @workkit/chat-react typecheck\n- bun run --filter @workkit/chat-react build\n- bun x biome check packages/chat-react/src/index.ts packages/chat-react/tests/use-chat-debug-frames.test.ts\n- git diff --check\n\nThe package is headless, so there is no styled visual surface to screenshot; the docs updates are the visible proof surface for this PR. |
|
Status check against the latest commit (22ac9c2): Previously flagged review comments — already addressed in 22ac9c2, but the inline threads weren't auto-resolved:
Those Copilot/CodeRabbit threads are stale and can be resolved. Still outstanding from the maintainer ask — docs site (not just the The maintainer asked for "the readme for the new packages and docs site too" —
That's the last thing standing between this PR and merge as far as I can see — code quality reads clean. Generated by Claude Code |
|
Added the missing docs-site page in commit 20e3b9c.\n\nWhat changed:\n- added apps/docs/src/content/docs/guides/chat-react.md as a generated Starlight guide for @workkit/chat-react\n- covered install, basic hook usage, DebugFrame shape, options, same-socket multi-panel behavior, connection state, and production gating\n\nVerification:\n- bun run --filter @workkit/docs typecheck\n- bun run --filter @workkit/docs build\n- git diff --check\n\nThe docs build now emits /guides/chat-react/index.html. |
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 `@packages/chat-react/src/index.ts`:
- Around line 167-176: The socket.send override currently calls each
activeRecorder(data) before invoking the underlying send, which can record an
"out" frame even if activePatch.originalSend.call(socket, data) throws; modify
the override in installSendRecorder so it first calls
activePatch.originalSend.call(socket, data) and only after that succeeds (or
after awaiting its result if it can be async) iterate through
activePatch.recorders to invoke them, and ensure any synchronous exception from
originalSend is re-thrown (or propagated) so recorders are not executed on
failure; keep references to sendPatches, activePatch.originalSend, and
activePatch.recorders when making this change.
🪄 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: 63b1b189-e700-4823-960f-8fa0517fd2e9
📒 Files selected for processing (7)
README.mdapps/docs/src/content/docs/guides/chat-react.mddocs/README.mddocs/api-reference.mddocs/getting-started.mdpackages/chat-react/src/index.tspackages/chat-react/tests/use-chat-debug-frames.test.ts
✅ Files skipped from review due to trivial changes (4)
- docs/api-reference.md
- docs/getting-started.md
- README.md
- docs/README.md
|
Addressed the latest review note in commit c185255. What changed:
Verification:
|
|
Verified c185255 against the latest CodeRabbit ask:
Cross-checked all 5 still-unresolved review threads against the latest source — every one is genuinely addressed in the current commit (the threads just never got auto-marked resolved):
Constitution gates: changeset present ( Closes #84. Nothing structural left on my end — over to you for the merge call. Generated by Claude Code |
Summary
Verification
Closes #84
Summary by CodeRabbit
New Features
Documentation
Tests