Move dashboard runtime repairs into core routes - #323
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 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 (8)
📝 WalkthroughWalkthroughThis PR adds two new advanced recovery CLI commands, ChangesDashboard reload and runtime restart feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Shim as installed-aimux-shim.sh
participant Daemon as daemon.ts
participant Tmux as TmuxRuntimeManager
participant Statusline as statusline-artifacts.ts
Shim->>Daemon: POST /core/dashboard-reload-text
Daemon->>Tmux: resolve dashboard target
Daemon->>Statusline: rewriteDashboardStatuslineArtifacts
Daemon->>Tmux: attach caller session (if --open)
Daemon-->>Shim: rendered text/json payload
Shim->>Daemon: POST /core/runtime-restart-text
Daemon->>Tmux: stopProjectTmuxRuntime
Daemon->>Statusline: rewriteDashboardStatuslineArtifacts
Daemon-->>Shim: tmuxSessionsKilled + dashboard target
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: 5
🧹 Nitpick comments (6)
src/core-text.ts (1)
322-328: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider surfacing
tmuxSessionsKilledin the rendered text.The payload carries
tmuxSessionsKilled(per daemon.ts'sruntimeRestartTextRoute), but the rendered lines never mention it, so CLI text output gives no confirmation of how many tmux sessions were actually stopped during a runtime restart.♻️ Optional enhancement
export function renderCoreRuntimeRestartLines(payload: CoreRuntimeRestartTextPayload): string[] { return [ `Restarted project runtime for ${payload.projectRoot}`, + `Tmux sessions stopped: ${payload.tmuxSessionsKilled.length}`, `Dashboard: ${payload.dashboardSessionName}:${payload.dashboardTarget.windowIndex}`, ]; }🤖 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/core-text.ts` around lines 322 - 328, The rendered runtime restart text in renderCoreRuntimeRestartLines should include the tmuxSessionsKilled value from CoreRuntimeRestartTextPayload so CLI output confirms how many tmux sessions were stopped. Update the string array returned by renderCoreRuntimeRestartLines to add a line that surfaces tmuxSessionsKilled alongside the existing projectRoot and dashboard details, using the payload field directly so the output stays in sync with daemon.ts's runtimeRestartTextRoute.src/core-cli-routing.ts (1)
98-110: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEquals-form flag values aren't checked for a leading
-.The
--flag valuebranch rejects values starting with-(to avoid swallowing a following flag as the value), but the--flag=valuebranch only checks truthiness —--client-tty=-xwould be accepted while--client-tty -xwould not. Minor inconsistency; unlikely to matter in practice since=unambiguously delimits the value.🤖 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/core-cli-routing.ts` around lines 98 - 110, The consumeOptionalTextFlag helper handles flag values inconsistently between spaced and equals forms. Update the equals-form branch in consumeOptionalTextFlag so it applies the same leading-dash validation as the consumeRequiredValue path, and return null when the parsed value is empty or starts with a hyphen. Keep the behavior aligned for both forms of the same flag in src/core-cli-routing.ts.src/core-command-ownership.test.ts (1)
198-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
--client-tty/--current-client-sessionflag combos.
isCoreDashboardReloadCommandandisCoreRuntimeRestartCommand(src/core-cli-routing.ts) both accept--client-ttyand--current-client-sessionin addition to--open/--json, but these disposition fixtures only cover the bare and--open/--jsonvariants. Adding entries for the tty/session flag combos would keep this fixture in sync with the full CLI contract these commands accept.🤖 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/core-command-ownership.test.ts` around lines 198 - 221, The disposition fixture for core command ownership is missing coverage for the accepted `--client-tty` and `--current-client-session` flag combinations. Update the test cases in `src/core-command-ownership.test.ts` to add entries for `dashboard-reload` and `restart-runtime` using these flags, alongside the existing `isCoreDashboardReloadCommand` and `isCoreRuntimeRestartCommand` scenarios. Make sure the new fixtures use the same `shim-fast-path` disposition and the correct `shimNeedle` values so the test stays aligned with the CLI routing contract in `src/core-cli-routing.ts`.src/installed-shim.test.ts (1)
491-589: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the main fast-path flows.
Consider adding a case where the route response has no
dashboardTarget/sessionName(runtime-restart with nothing to attach to), to lock in the expectedError: command completed, but no dashboard target was available to openbehavior and catch the emptydashboard_session_nameformatting issue noted inscripts/installed-aimux-shim.sh.🤖 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/installed-shim.test.ts` around lines 491 - 589, Add a regression test in installed-shim.test.ts for the runtime-restart/dashboard-reload open path when the route response lacks both dashboardTarget and sessionName. Use the existing fixture helpers around fixture.run, dashboard-reload/restart-runtime, and the textRouteFile/daemonInfoPath setup to assert the command fails with the expected “no dashboard target was available to open” error and does not launch Node. This will also lock in the empty dashboard_session_name handling in the shim.scripts/installed-aimux-shim.sh (2)
310-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate of
aimux_post_project_restart_open.This function repeats the mktemp/trap/curl-status/sed-parsing/attach-session boilerplate already implemented in
aimux_post_project_restart_open(lines 255-308), differing only in the route path, success message, and error-message wording. Extracting a shared helper would reduce the surface area to maintain when this POST/parse/attach pattern needs to change.♻️ Sketch of a shared helper
+aimux_post_and_open_dashboard_target() { + route_path="$1" + timeout="$2" + on_success="$3" # callback name invoked with dashboard_session_name/project_root/window_index in scope + shift 3 + port="$(aimux_matching_daemon_port)" || return 1 + body_file="$(mktemp "${TMPDIR:-/tmp}/aimux-dashboard-open.XXXXXX")" || return 1 + trap 'rm -f "$body_file"' EXIT + trap 'rm -f "$body_file"; exit 130' INT TERM + status="$(curl -sS --max-time "$timeout" -o "$body_file" -w '%{http_code}' -X POST "$@" \ + "http://127.0.0.1:$port$route_path?json=1" 2>/dev/null || true)" + # ... shared status handling, sed extraction, attach-session logic ... +}🤖 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 `@scripts/installed-aimux-shim.sh` around lines 310 - 372, The logic in aimux_post_dashboard_open_route is a near-duplicate of aimux_post_project_restart_open, so factor the shared mktemp/trap/curl-status/sed-parsing/attach-session flow into a common helper. Keep only the route-specific pieces in aimux_post_dashboard_open_route: the route path, success_kind-specific output, and final error wording. Update both functions to call the shared helper so future changes to the POST/parse/attach behavior happen in one place.
767-890: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated open-detection/dispatch logic between the two new handlers.
The tmux client auto-detection and open-vs-direct-post branching (lines 805-820 and 872-890) is copy-pasted between
aimux_try_dashboard_reloadandaimux_try_runtime_restart, differing only by route path andsuccess_kind. Since both were introduced in this PR, extracting a shared dispatch helper now avoids two places to keep in sync going forward.♻️ Suggested shared dispatch helper
+aimux_dispatch_dashboard_action() { + route_path="$1" + success_kind="$2" + open="$3" + current_client_session="$4" + client_tty="$5" + shift 5 + if [ "$open" -eq 1 ]; then + if [ -z "$current_client_session" ] && [ -z "$client_tty" ] && [ -n "${TMUX:-}" ] && command -v tmux >/dev/null 2>&1; then + current_client_session="$(tmux display-message -p "#{client_session}" 2>/dev/null || true)" + client_tty="$(tmux display-message -p "#{client_tty}" 2>/dev/null || true)" + fi + if [ -n "$current_client_session" ] || [ -n "$client_tty" ]; then + set -- "$@" --data-urlencode "open=1" + [ -n "$current_client_session" ] && set -- "$@" --data-urlencode "currentClientSession=$current_client_session" + [ -n "$client_tty" ] && set -- "$@" --data-urlencode "clientTty=$client_tty" + aimux_post_query_text_route "$route_path" 120 "$@" + return $? + fi + aimux_post_dashboard_open_route "$route_path" "$success_kind" 120 "$@" + return $? + fi + aimux_post_query_text_route "$route_path" 120 "$@" +}Then both handlers would just build their
--data-urlencodeargs and call this helper.🤖 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 `@scripts/installed-aimux-shim.sh` around lines 767 - 890, The tmux auto-detection and open-vs-direct-post branching is duplicated in aimux_try_dashboard_reload and aimux_try_runtime_restart, so extract that shared logic into a helper and have both handlers call it. Keep the route-specific pieces as arguments to the helper (such as the text route and success_kind/open action), and let each handler only assemble its own query params like projectRoot and json before dispatching.
🤖 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 `@docs/command-ownership-inventory.md`:
- Around line 42-43: The command ownership inventory entry for restart-runtime
is missing the stale-daemon fallback behavior that is documented for
dashboard-reload. Update the restart-runtime row to explicitly mention that it
also falls back to a Node bootstrap when the daemon is stale, while keeping the
healthy path daemon-owned; use the existing “restart-runtime” row and its
`/core/runtime-restart-text` description as the anchor for the edit.
In `@scripts/installed-aimux-shim.sh`:
- Around line 345-356: The runtime-restart success path in the success_kind case
is logging a dashboard target even when none exists, which leads to misleading
output before the missing-target error path. Update the runtime-restart branch
to only print the Dashboard line after confirming session_name is set, or guard
it so it only logs when a real dashboard target is available; keep the change
localized to the success_kind switch handling and the session_name check flow.
In `@src/daemon.ts`:
- Around line 635-639: In explicitProjectRootTextParam, the projectRoot lookup
uses nullish coalescing so an explicit empty string prevents falling back to
project. Update the parameter selection logic to treat blank projectRoot as
absent and then use project as the fallback before validating. Keep the fix
localized to explicitProjectRootTextParam in src/daemon.ts and preserve the
existing resolveProjectRoot and textError behavior.
- Around line 697-709: The dashboard reload flow in dashboardReloadTextRoute
invalidates statusline artifacts before confirming tmux is available, which can
leave the dashboard broken if tmux is missing. Move the tmux availability check
on TmuxRuntimeManager ahead of invalidateTmuxStatuslineArtifacts(projectRoot),
mirroring the ordering used in runtimeRestartTextRoute, so artifacts are only
deleted once you know regeneration can proceed.
- Around line 732-758: The runtime restart flow in runtimeRestartTextRoute
recreates tmux sessions but does not refresh the statusline artifacts, leaving
the dashboard footer stale. Add the same statusline invalidation/rewriting step
used by dashboardReloadTextRoute after stopping/restarting the project and
before returning the payload, so restart-runtime keeps the tmux UI current. Use
the runtimeRestartTextRoute flow, stopProjectTmuxRuntime, and
resolveDashboardTarget as the nearby anchors when wiring in the refresh logic.
---
Nitpick comments:
In `@scripts/installed-aimux-shim.sh`:
- Around line 310-372: The logic in aimux_post_dashboard_open_route is a
near-duplicate of aimux_post_project_restart_open, so factor the shared
mktemp/trap/curl-status/sed-parsing/attach-session flow into a common helper.
Keep only the route-specific pieces in aimux_post_dashboard_open_route: the
route path, success_kind-specific output, and final error wording. Update both
functions to call the shared helper so future changes to the POST/parse/attach
behavior happen in one place.
- Around line 767-890: The tmux auto-detection and open-vs-direct-post branching
is duplicated in aimux_try_dashboard_reload and aimux_try_runtime_restart, so
extract that shared logic into a helper and have both handlers call it. Keep the
route-specific pieces as arguments to the helper (such as the text route and
success_kind/open action), and let each handler only assemble its own query
params like projectRoot and json before dispatching.
In `@src/core-cli-routing.ts`:
- Around line 98-110: The consumeOptionalTextFlag helper handles flag values
inconsistently between spaced and equals forms. Update the equals-form branch in
consumeOptionalTextFlag so it applies the same leading-dash validation as the
consumeRequiredValue path, and return null when the parsed value is empty or
starts with a hyphen. Keep the behavior aligned for both forms of the same flag
in src/core-cli-routing.ts.
In `@src/core-command-ownership.test.ts`:
- Around line 198-221: The disposition fixture for core command ownership is
missing coverage for the accepted `--client-tty` and `--current-client-session`
flag combinations. Update the test cases in `src/core-command-ownership.test.ts`
to add entries for `dashboard-reload` and `restart-runtime` using these flags,
alongside the existing `isCoreDashboardReloadCommand` and
`isCoreRuntimeRestartCommand` scenarios. Make sure the new fixtures use the same
`shim-fast-path` disposition and the correct `shimNeedle` values so the test
stays aligned with the CLI routing contract in `src/core-cli-routing.ts`.
In `@src/core-text.ts`:
- Around line 322-328: The rendered runtime restart text in
renderCoreRuntimeRestartLines should include the tmuxSessionsKilled value from
CoreRuntimeRestartTextPayload so CLI output confirms how many tmux sessions were
stopped. Update the string array returned by renderCoreRuntimeRestartLines to
add a line that surfaces tmuxSessionsKilled alongside the existing projectRoot
and dashboard details, using the payload field directly so the output stays in
sync with daemon.ts's runtimeRestartTextRoute.
In `@src/installed-shim.test.ts`:
- Around line 491-589: Add a regression test in installed-shim.test.ts for the
runtime-restart/dashboard-reload open path when the route response lacks both
dashboardTarget and sessionName. Use the existing fixture helpers around
fixture.run, dashboard-reload/restart-runtime, and the
textRouteFile/daemonInfoPath setup to assert the command fails with the expected
“no dashboard target was available to open” error and does not launch Node. This
will also lock in the empty dashboard_session_name handling in the shim.
🪄 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: 5e7c3f26-932f-47c2-9117-b8e1809616ad
📒 Files selected for processing (13)
docs/command-ownership-inventory.mddocs/core-sidecar-north-star.mdscripts/installed-aimux-shim.shsrc/core-cli-routing.tssrc/core-command-contract.tssrc/core-command-ownership.test.tssrc/core-text.tssrc/daemon.test.tssrc/daemon.tssrc/installed-shim.test.tssrc/main.tssrc/tmux/runtime-stop.tssrc/tmux/statusline-artifacts.ts
1 participant
Summary
dashboard-reloadandrestart-runtimethrough daemon-owned core text endpointsVerification
Summary by CodeRabbit
New Features
Bug Fixes
Tests