fix: MemoryBusTui never upgrades + spawn lock #13#14
Merged
Conversation
Closes #13 Removes the automatic fallback to MemoryBusTui when the Go bus is not reachable. Callers that want in-process mode can now instantiate 'new MemoryBusTui()' explicitly. This prevents silent subscription failures where a TUI subscribes to a pattern via MemoryBusTui, never sees any messages (because the server-side plugin publishes via the real Go bus), and the user has no idea why. After this change: - BusTui.connect() throws a clear error if the Go bus is unreachable - MemoryBusTui is still exported for callers that want it explicitly - Both consumers (brain, tbguard) already have try/catch or .catch handlers, so they degrade gracefully and log a warning - Tests are not affected (integration test runs the real Go bus; scoped-close test also runs the real Go bus) Updates MemoryBusTui JSDoc to note opt-in usage and the integration test header comment to reflect the new contract.
|
No actionable comments were generated in the recent review. 🎉 Recent review infoRun configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Files ignored due to path filters (7)
Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughWalkthrough
ChangesBus lifecycle fixes, useServiceBus hook, and deriveProjectId
Sequence Diagram(s)sequenceDiagram
participant Component
participant useServiceBus
participant BusTui
participant BusChannel
Component->>useServiceBus: mount(service, sessionId, channel, onMessage)
useServiceBus->>BusTui: connect()
alt connect succeeds
BusTui-->>useServiceBus: bus handle stored in signal
else connect fails
useServiceBus->>useServiceBus: schedule retry in 5s
end
useServiceBus->>useServiceBus: createEffect waits for bus handle and sessionId()
useServiceBus->>BusChannel: subscribe(service/sessionId/channel)
useServiceBus->>BusChannel: publish ping {ts: Date.now()}
Note over useServiceBus,BusChannel: republish ping every 2s until first message
BusChannel-->>useServiceBus: envelope received
useServiceBus->>Component: onMessage(envelope.payload)
useServiceBus->>useServiceBus: stop ping interval (confirmed)
Component->>useServiceBus: unmount
useServiceBus->>BusChannel: unsubscribe
useServiceBus->>BusTui: close()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Tip: You can configure your own custom pre-merge checks in the settings. Finishing TouchesGenerate docstrings
Generate unit tests (beta)
Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/bus-tui.ts (1)
237-260:⚠️ Potential issue | 🔴 CriticalAdd
exportkeyword to MemoryBusTui class declaration.The class is not exported, making it inaccessible to users despite documentation instructing them to instantiate
MemoryBusTuidirectly. Change line 237 fromclass MemoryBusTui extends BusTui {toexport class MemoryBusTui extends BusTui {.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 `@src/bus-tui.ts` around lines 237 - 260, The MemoryBusTui class declaration is not exported, which prevents external code from accessing it even though it is documented for direct instantiation. Add the export keyword before the class declaration for MemoryBusTui to make it accessible to users of the module.
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 `@src/bus-tui.ts`:
- Around line 237-260: The MemoryBusTui class declaration is not exported, which
prevents external code from accessing it even though it is documented for direct
instantiation. Add the export keyword before the class declaration for
MemoryBusTui to make it accessible to users of the module.
Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b1ead0bc-b340-4dae-a3d3-3e841d86ea6b
Files ignored due to path filters (5)
dist/bus-tui.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/tui.jsis excluded by!**/dist/**dist/tui.js.mapis excluded by!**/dist/**,!**/*.map
Files selected for processing (2)
src/bus-tui.tstest/integration.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
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 `@src/tui-hooks.ts`:
- Around line 40-42: Remove the explicit `as BusCallback` type cast in the
subscribe method call to enable proper TypeScript type inference. The inline
callback type annotation currently declares only `{ payload: unknown }` but the
actual envelope object contains additional properties (channel, payload, ts).
Delete the type cast and update the inline callback parameter type to accurately
reflect the full BusEnvelope structure that the subscribe method signature
expects, allowing TypeScript to infer the correct types automatically rather
than masking the mismatch with a cast.
- Around line 23-29: Add a disposed flag to track whether the component has been
unmounted before the BusTui.connect() promise resolves. Create this flag within
the onMount hook and set it to true in an onCleanup function. Then in the
.then() callback of the BusTui.connect() promise chain, check the disposed flag
before calling setBusTui(); if disposed is true, explicitly close the bus
connection. This prevents the memory leak where a successfully resolved bus
connection has no cleanup handler if unmounting occurs before the promise
settles.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d434675b-9d96-4015-ba70-92dd3be75589
Files ignored due to path filters (4)
dist/tui-hooks.d.tsis excluded by!**/dist/**dist/tui.d.tsis excluded by!**/dist/**dist/tui.jsis excluded by!**/dist/**dist/tui.js.mapis excluded by!**/dist/**,!**/*.map
Files selected for processing (3)
package.jsonsrc/tui-hooks.tssrc/tui.ts
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/tui.ts
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
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="src/tui-hooks.ts">
<violation number="1" location="src/tui-hooks.ts:25">
P2: Async connect result is not cancellation-safe; unmount-before-resolve can leak an open bus connection.</violation>
<violation number="2" location="src/tui-hooks.ts:26">
P2: Initial connection failures are silently dropped with no retry, so the hook can stay disconnected for the component lifetime.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Add unmount guard and reconnect-with-backoff to useServiceBus hook. Prevents leaks when a TUI component unmounts while BusTui.connect() is still in-flight (the resulting handle is closed immediately). On connect failure, retries every 5s instead of silently giving up. Ref: #13
…move BusCallback cast
This was referenced Jun 15, 2026
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.
Throws clear error instead of silent fallback. Adds spawn lock.
Summary by cubic
Fail fast when the Go bus isn’t available and prevent duplicate spawns. Add SolidJS hooks for safe, reactive subscriptions (including project-scoped channels) and make TUI reconnect switch to new ports after restarts.
New Features
useServiceBusfor reactive, session-scoped subscriptions; cancellation-safe with 5s connect retry and 2s ping until the worker responds.useProjectBusfor project-scoped channels usingderiveProjectId(directory).MemoryBusTuiandderiveProjectId.Bug Fixes
BusTui.connect()now throws a clear error if the Go bus can’t be reached.BusClientwith a splitspawnBus()prevents concurrent spawns.solid-jsand@opentui/solidin the TUI build.Written for commit 335bf0e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation