refactor: add native transport seam under IRC client#3
Open
louzt wants to merge 2 commits into
Open
Conversation
Replace the hardcoded NodeTCPSocket construction with an injectable socket transport factory so future native transport work can be integrated without making the IRC/UI layers depend on an external engine repository.\n\nAlso adds ARCHITECTURE.md with Mermaid diagrams and focused tests covering the new seam.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces a transport-abstraction for sockets: extracts Changes
Sequence DiagramsequenceDiagram
participant IRC as IRCClient
participant Factory as SocketTransportFactory
participant NodeTCP as NodeTCPSocket
participant ISocket as ISocket
IRC->>Factory: createSocketTransport(url)
Factory->>NodeTCP: instantiate with parsed host/port (default)
Factory-->>IRC: returns ISocket
IRC->>ISocket: attach onopen/onmessage/onerror/onclose
IRC->>ISocket: send(data) / close()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
🧹 Nitpick comments (1)
src/lib/socketTransport.ts (1)
1-20: LGTM — simple seam with proper lifecycle controls.Default factory, override, and reset cover the immediate needs, and the re-exports keep consumer imports consolidated. One optional thought for later: the mutable module-level
socketTransportFactoryis a fine seam today, but if you ever need per-client isolation (e.g., one IRC connection with a fake transport while another keeps TCP, or parallelized integration tests withoutafterEachdiscipline), promoting this to a small class or a per-IRCClientinjection point would avoid cross-cutting state. Not necessary for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/socketTransport.ts` around lines 1 - 20, The current module-level mutable socketTransportFactory (used by createSocketTransport, setSocketTransportFactory, resetSocketTransportFactory and defaultSocketTransportFactory/NodeTCPSocket) prevents per-client isolation; to support per-client injection, add an optional SocketTransportFactory parameter to the public API that needs it (e.g., the IRCClient constructor or createSocketTransport signature) and thread that factory through to where sockets are created instead of reading the module-level socketTransportFactory, leaving the existing setters as test/global conveniences; ensure the new consumer-facing API accepts a SocketTransportFactory and falls back to defaultSocketTransportFactory when undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/socketTransport.test.ts`:
- Around line 34-44: The test currently triggers a real TCP connect via
NodeTCPSocket's constructor; instead stub out net.connect (and tls.connect if
needed) before calling createSocketTransport so the constructor doesn't dial
out: use jest.spyOn(require('net'), 'connect').mockImplementation(() => ({ on:
() => {}, once: () => {}, end: () => {}, destroy: () => {} })) (and similarly
stub tls.connect if TLS paths are exercised), call
resetSocketTransportFactory(), run
createSocketTransport('irc://127.0.0.1:65535') to assert it returns a
NodeTCPSocket without real network I/O, then restore the spies; reference
setSocketTransportFactory, resetSocketTransportFactory, createSocketTransport,
and NodeTCPSocket to locate code.
---
Nitpick comments:
In `@src/lib/socketTransport.ts`:
- Around line 1-20: The current module-level mutable socketTransportFactory
(used by createSocketTransport, setSocketTransportFactory,
resetSocketTransportFactory and defaultSocketTransportFactory/NodeTCPSocket)
prevents per-client isolation; to support per-client injection, add an optional
SocketTransportFactory parameter to the public API that needs it (e.g., the
IRCClient constructor or createSocketTransport signature) and thread that
factory through to where sockets are created instead of reading the module-level
socketTransportFactory, leaving the existing setters as test/global
conveniences; ensure the new consumer-facing API accepts a
SocketTransportFactory and falls back to defaultSocketTransportFactory when
undefined.
🪄 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: f59c0416-e400-4206-be21-8c6a28e9b9d8
📒 Files selected for processing (7)
ARCHITECTURE.mdREADME.mdsrc/lib/nodeTcpSocket.tssrc/lib/socketTransport.tssrc/lib/socketTypes.tssrc/utils/ircClient.tstests/unit/socketTransport.test.ts
Stub net.connect in the reset test so the unit test verifies the default factory without performing real loopback network I/O.
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
This PR introduces a native socket transport seam under tobby's IRC client so future non-TCP transport work can be integrated without coupling the UI/store/IRC layers to any external transport engine repository.
What this changes
NodeTCPSocketconstruction insideIRCClient.connect()with an injectableSocketTransportFactoryARCHITECTURE.mdwith Mermaid diagrams showing the current layering and the intended native transport evolution pathWhy this matters
Today tobby is hard-wired to raw TCP/TLS at the point where the IRC client opens its byte transport.
That makes experimentation with a native overlay transport unnecessarily invasive.
This PR keeps the existing behavior intact while creating a narrow integration seam for future transport implementations.
Non-goals
Verification
bun x vitest run tests/unit/socketTransport.test.ts tests/nodeTcpSocket.test.tsbun run typecheckNote
The existing Husky hook in this repo currently calls
bunx, which is not available in this environment. The code in this PR was validated explicitly with the commands above.Summary by CodeRabbit
Documentation
Refactor
Tests