Fix TUI dashboard readiness races#261
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a dashboard ready stamp for reuse and respawn checks, defers selected local navigation during guarded states, updates runtime-guard probing and key handling, and stamps tmux runtime contracts earlier during new session creation. ChangesDashboard Readiness Stamp, Navigation Queuing & Runtime-Guard Improvements
Runtime Contract Stamped Before Session Configuration
Possibly Related PRs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 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/multiplexer/dashboard-control.ts`:
- Around line 228-233: The queued navigation payload currently stores the full
buffer in queuePendingDashboardLocalNavigation, which can replay extra coalesced
input later; update this helper to queue only the first parsed event’s raw bytes
(events[0].raw) instead of Buffer.from(data). Make the same change in the other
affected queueing path referenced by the review, and keep the pending.key logic
unchanged so it still validates only the first parsed event.
In `@src/multiplexer/runtime-guard.ts`:
- Around line 34-44: Update isProjectServiceConnectionError in runtime-guard.ts
to treat connect timeouts as connection failures. Add ETIMEDOUT handling
alongside the existing ECONNREFUSED/ECONNRESET/EPIPE checks, including matching
it in both the error.code and message-based branches. This should prevent
getProjectServiceManifest from falling through to the local-manifest path when a
socket connect timeout occurs.
In `@src/tmux/control-script.test.ts`:
- Around line 114-118: The fake tmux ready-option logic in the control script
test is diverging from scripts/tmux-control.sh because the
`@aimux-dashboard-ready` fallback currently substitutes the build stamp when the
value is missing. Update the option lookup path in control-script.test.ts so
`@aimux-dashboard-ready` stays unset/false when absent instead of inheriting
`@aimux-dashboard-build`, and keep the behavior aligned with the control script’s
readiness check in the same helper that reads state.windowOptions.
🪄 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: fc7ce326-eeef-499f-a376-9cd6139d3cab
📒 Files selected for processing (14)
scripts/tmux-control.shsrc/dashboard/targets.test.tssrc/dashboard/targets.tssrc/metadata-server.test.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/runtime-guard.test.tssrc/multiplexer/runtime-guard.tssrc/multiplexer/session-launch.test.tssrc/multiplexer/session-launch.tssrc/runtime-owner.tssrc/tmux/control-script.test.tssrc/tmux/runtime-manager.test.tssrc/tmux/runtime-manager.ts
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 `@src/metadata-server.test.ts`:
- Around line 2885-2894: The stale-ready test in metadata-server.test.ts is
currently satisfied by earlier calls, so it doesn’t prove this request path
still resets state correctly. Before the stale-ready scenario, clear the
relevant mocks used in the open-dashboard flow, especially setWindowOptionMock
and respawnWindowMock, or switch the expectation in this block to assert only
the new call(s) made by this request. Use the existing open-dashboard test setup
and the TMUX_DASHBOARD_READY_OPTION assertion to keep the check scoped to the
stale-ready case.
🪄 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: 4e36be46-431d-44f2-9d50-f5984a0c20eb
📒 Files selected for processing (2)
src/metadata-server.test.tssrc/metadata-server.ts
Owner
Author
|
Independent review follow-up fixed in 123617d:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tui/screens/dashboard-renderers.ts (1)
389-428: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the footer hint aligned with the actual activation key.
handleDashboardNavigationKey()still accepts"right"alongside"enter"in every dashboard navigation state, so these hints now hide a supported shortcut. Please either renderEnter/→here too, or drop"right"from the handler if the alias is intentionally gone.🤖 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/tui/screens/dashboard-renderers.ts` around lines 389 - 428, The footer hints in the dashboard renderer are out of sync with the supported activation keys in handleDashboardNavigationKey(), since the handler still accepts both Enter and right-arrow in these states. Update the hint labels in the dashboard footer rendering to show the actual activation shortcut (for example Enter/→) wherever that alias is still supported, or remove the right-arrow alias from handleDashboardNavigationKey() if it is no longer intended.src/multiplexer/runtime-guard.ts (1)
172-193: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't swallow local processing failures as
unreachable.This
catchnow covers both the HTTP probe and the local response-processing branch. IfgetProjectServiceManifest()orgetProjectStateDirFor(projectRoot)throws, we silently downgrade that bug/config failure into a disconnected state, and downstream dashboard control will treat it like a recoverable guard miss. Please narrow thetrytorequestJson(...)or rethrow non-transport errors.🤖 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/multiplexer/runtime-guard.ts` around lines 172 - 193, The runtime guard in runtime-guard.ts is swallowing local processing errors and turning them into "unreachable," which hides real bugs; narrow the inner try/catch around requestJson in the health probe path or rethrow any non-transport failures. Keep getProjectServiceManifest and getProjectStateDirFor(projectRoot) outside the catch that maps network failures to serviceManifest = "unreachable", and preserve the existing endpointPresent/serviceIdentityMismatch flow in the same block.
🤖 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/metadata-server.test.ts`:
- Around line 2906-2911: The test in the metadata-server suite currently
verifies that `setWindowOptionMock` and `respawnWindowMock` are both called, but
it does not assert their order. Update this assertion block to pin the contract
that `TMUX_DASHBOARD_READY_OPTION` is cleared before `respawnWindowMock` runs,
using the same mocked calls (`setWindowOptionMock`, `respawnWindowMock`, and
`expect.objectContaining({ windowId: "`@99`" })`) so the sequence is enforced even
if implementation details change.
---
Outside diff comments:
In `@src/multiplexer/runtime-guard.ts`:
- Around line 172-193: The runtime guard in runtime-guard.ts is swallowing local
processing errors and turning them into "unreachable," which hides real bugs;
narrow the inner try/catch around requestJson in the health probe path or
rethrow any non-transport failures. Keep getProjectServiceManifest and
getProjectStateDirFor(projectRoot) outside the catch that maps network failures
to serviceManifest = "unreachable", and preserve the existing
endpointPresent/serviceIdentityMismatch flow in the same block.
In `@src/tui/screens/dashboard-renderers.ts`:
- Around line 389-428: The footer hints in the dashboard renderer are out of
sync with the supported activation keys in handleDashboardNavigationKey(), since
the handler still accepts both Enter and right-arrow in these states. Update the
hint labels in the dashboard footer rendering to show the actual activation
shortcut (for example Enter/→) wherever that alias is still supported, or remove
the right-arrow alias from handleDashboardNavigationKey() if it is no longer
intended.
🪄 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: b610548a-ff7e-4d51-8574-d087301c0bd0
📒 Files selected for processing (12)
src/metadata-server.test.tssrc/metadata-server.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/dashboard-interaction.test.tssrc/multiplexer/dashboard-interaction.tssrc/multiplexer/runtime-guard.test.tssrc/multiplexer/runtime-guard.tssrc/tmux/control-script.test.tssrc/tui/screens/dashboard-renderers.test.tssrc/tui/screens/dashboard-renderers.tssrc/tui/screens/overlay-renderers.ts
✅ Files skipped from review due to trivial changes (1)
- src/tui/screens/dashboard-renderers.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/metadata-server.ts
- src/multiplexer/dashboard-control.ts
- src/multiplexer/runtime-guard.test.ts
- src/multiplexer/dashboard-control.test.ts
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
Verification
Summary by CodeRabbit
New Features
Enter/→and library access vial).Bug Fixes
Tests