Fix TUI transition smoke regressions#359
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR replaces the tmux exposé CLI invocation with a UNIX-socket daemon protocol (script, metadata-server, expose runtime), adds dashboard pending-action reconciliation against raw session/service state including resume-settle inactive handling, introduces sanitized user-facing error formatting used in several modules, and switches managed launch environment filtering to an allowlist. ChangesExpose UNIX-socket daemon protocol
Dashboard pending-action reconciliation and resume settlement
Sanitized user-facing error formatting
Managed launch environment allowlist
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant TmuxControlScript
participant ExposeSocket as "Expose Unix Socket"
participant MetadataServer
participant RunTmuxExpose
TmuxControlScript->>ExposeSocket: nc -U context payload (project/session/pane/size)
ExposeSocket->>MetadataServer: handleExposeSocket(header, stream)
MetadataServer->>MetadataServer: parse/validate launch header
MetadataServer->>RunTmuxExpose: runTmuxExpose(header params, input/output)
RunTmuxExpose-->>MetadataServer: exit code
MetadataServer-->>ExposeSocket: write code, end socket
ExposeSocket-->>TmuxControlScript: popup status (retry on 75)
sequenceDiagram
participant DashboardOps
participant ResumeAPI
participant DashboardModel
participant RuntimeState
DashboardOps->>ResumeAPI: resume offline session
ResumeAPI-->>DashboardOps: resumeRequestSettled=true (finally)
DashboardOps->>DashboardModel: waitForDashboardSessionResumeSettle(allowInactiveSettle)
DashboardModel->>RuntimeState: reconcileDashboardPendingActionsFromRawModel(raw sessions/services)
RuntimeState-->>DashboardModel: pending action cleared if settled
DashboardModel-->>DashboardOps: restored or stayed-offline result
DashboardOps->>DashboardOps: applyRestoreSettlementFlash(footerFlash)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 4
🧹 Nitpick comments (3)
src/multiplexer/runtime-state.test.ts (1)
2059-2090: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for the fresh-starting-without-pending-flag case.
This test only covers a stale
"starting"session with a matching"starting"pending flag. Add a companion test for a fresh"starting"session with no"starting"pending action set (or a different pending kind, e.g."creating") to confirm it is not demoted within the grace window — this would catch the condition issue raised onsrc/multiplexer/runtime-state.ts(lines 288-293).🤖 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-state.test.ts` around lines 2059 - 2090, Add a companion test around reconcileOrphanedTopologySessions and listTopologySessionStates for a fresh "starting" session that has no matching "starting" DashboardPendingActions entry (or only a different pending kind like "creating"); assert it stays in "starting" during the grace window and is not demoted to "offline". Reuse the existing helpers upsertTopologySession, noWindowsHost, and DashboardPendingActions so the new case specifically exercises the pending-action check in runtime-state handling.src/dashboard/pending-actions.ts (1)
164-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect implementation; minor duplication.
Both methods share identical shape (filter by
target, slice key prefix, map fields). Could be consolidated into a private generic helper, e.g.private listActionsByTarget<T>(target, prefix): T[], to avoid maintaining two near-identical blocks.🤖 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/dashboard/pending-actions.ts` around lines 164 - 185, The `listSessionActions` and `listServiceActions` methods in `PendingActions` are duplicated with only the target/prefix/type differing. Refactor them to use a single private helper such as `listActionsByTarget` that filters by `entry.target`, slices the appropriate key prefix, and maps the shared fields, while keeping the existing public method names and return types intact.src/multiplexer/dashboard-model.ts (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGrace-period constant duplicated across files without a shared source of truth.
PENDING_START_OFFLINE_SETTLE_MS(5000ms here) conceptually overlaps withSTARTING_ORPHAN_RECONCILE_GRACE_MSinsrc/multiplexer/runtime-state.tsand the inline>= 5_000threshold used inallowInactiveSettleinsrc/multiplexer/dashboard-ops.ts. All three independently encode "how long to wait before treating a starting session as settled/orphaned." Consider extracting a single shared constant to avoid future drift between these coordinated timing windows.🤖 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/dashboard-model.ts` at line 58, The grace-period timing is duplicated in dashboard-model, runtime-state, and dashboard-ops, so extract a single shared constant for the “starting session settle/orphan” window and use it everywhere. Update PENDING_START_OFFLINE_SETTLE_MS, STARTING_ORPHAN_RECONCILE_GRACE_MS, and the allowInactiveSettle threshold to reference the same source of truth so the coordinated timing stays aligned. Keep the new constant in a shared module that both dashboard-model and runtime-state can import without introducing local magic numbers.
🤖 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 `@scripts/tmux-control.sh`:
- Around line 528-530: Empty expose.sock.path can overwrite expose_socket with
an empty string and suppress the popup. In scripts/tmux-control.sh, update the
expose socket loading logic near the show_local_expose flow so head -n 1 from
expose_socket_file only replaces expose_socket when it returns a non-empty
value, otherwise keep the existing default socket path. Use the
expose_socket_file and expose_socket variables to locate the guard, and ensure
the later [ -e "$expose_socket" ] check still sees the default when the config
file is empty.
In `@src/metadata-server.ts`:
- Line 1342: The auxiliary expose socket startup in startExposeSocket is
currently awaited directly from start(), so any bind/listen failure can abort
the whole metadata server even though it is a secondary feature. Update the
start flow around startExposeSocket and the exposeServer error handling to
mirror the graceful fallback used by the main HTTP listen path: catch and log
expose-socket startup errors, allow start() to continue, and only fail the
server when the primary listener cannot start.
- Around line 1392-1396: The expose socket path is missing a local error
handler, so disconnects during handleExposeSocket can bubble to the
process-level uncaughtException flow and kill the daemon. Update the
createNetServer callback in exposeServer to register a socket.on("error", ...)
listener before invoking runInProjectContext/handleExposeSocket, and make it
safely destroy the socket there while keeping the existing catch-based cleanup
intact.
In `@src/multiplexer/dashboard-model.ts`:
- Around line 125-136: Add a time-based settle fallback to
servicePendingSettled, mirroring sessionPendingSettled, so "creating"/"starting"
service actions can clear even when the service never reaches "running". Update
servicePendingSettled to use the action.startedAt timestamp and a pending-start
timeout constant (similar to PENDING_START_OFFLINE_SETTLE_MS), and keep the
existing status-based checks for normal cases. Make sure the new logic is
applied in the dashboard-model service reconciliation path so persisted/reloaded
snapshots from persistence-methods.ts can also settle stuck pending service
actions.
---
Nitpick comments:
In `@src/dashboard/pending-actions.ts`:
- Around line 164-185: The `listSessionActions` and `listServiceActions` methods
in `PendingActions` are duplicated with only the target/prefix/type differing.
Refactor them to use a single private helper such as `listActionsByTarget` that
filters by `entry.target`, slices the appropriate key prefix, and maps the
shared fields, while keeping the existing public method names and return types
intact.
In `@src/multiplexer/dashboard-model.ts`:
- Line 58: The grace-period timing is duplicated in dashboard-model,
runtime-state, and dashboard-ops, so extract a single shared constant for the
“starting session settle/orphan” window and use it everywhere. Update
PENDING_START_OFFLINE_SETTLE_MS, STARTING_ORPHAN_RECONCILE_GRACE_MS, and the
allowInactiveSettle threshold to reference the same source of truth so the
coordinated timing stays aligned. Keep the new constant in a shared module that
both dashboard-model and runtime-state can import without introducing local
magic numbers.
In `@src/multiplexer/runtime-state.test.ts`:
- Around line 2059-2090: Add a companion test around
reconcileOrphanedTopologySessions and listTopologySessionStates for a fresh
"starting" session that has no matching "starting" DashboardPendingActions entry
(or only a different pending kind like "creating"); assert it stays in
"starting" during the grace window and is not demoted to "offline". Reuse the
existing helpers upsertTopologySession, noWindowsHost, and
DashboardPendingActions so the new case specifically exercises the
pending-action check in runtime-state handling.
🪄 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: e325b495-844d-4bb9-8868-e990ca1f447a
📒 Files selected for processing (19)
scripts/tmux-control.shsrc/dashboard/pending-actions.tssrc/debug.tssrc/error-display.test.tssrc/error-display.tssrc/managed-launch-env.test.tssrc/managed-launch-env.tssrc/metadata-server.tssrc/multiplexer/dashboard-model.test.tssrc/multiplexer/dashboard-model.tssrc/multiplexer/dashboard-ops.test.tssrc/multiplexer/dashboard-ops.tssrc/multiplexer/persistence-methods.tssrc/multiplexer/runtime-state.test.tssrc/multiplexer/runtime-state.tssrc/tmux/control-script.test.tssrc/tmux/expose.tssrc/tmux/runtime-manager.test.tssrc/tmux/runtime-manager.ts
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Security