feat: add --remote-only flag to run TUI as pure remote client - #389
Merged
Conversation
- Skips local engine, session, and lock acquisition - InstanceManager accepts remoteOnly option to omit local tab - TabBar shows placeholder when no tabs are configured - Guards against --listen and --headless combinations - Fixes isViewingRemote check to use tab.isLocal instead of index > 0
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces a ChangesRemote-Only Mode
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
==========================================
- Coverage 48.27% 48.25% -0.02%
==========================================
Files 117 117
Lines 38511 38723 +212
==========================================
+ Hits 18590 18685 +95
- Misses 19921 20038 +117
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tui/components/RunApp.tsx (1)
3406-3411:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRender gate hides the new “No remotes configured” state
TabBarnow handlestabs.length === 0, but this parent only renders it whenlength > 1, so the empty-state hint never appears. Please relax this condition (and matching height calc) so zero-tab remote-only mode can display the guidance.Proposed fix
- const tabBarHeight = instanceTabs && instanceTabs.length > 1 ? layout.tabBar.height : 0; + const shouldShowTabBar = Boolean(instanceTabs && (instanceTabs.length === 0 || instanceTabs.length > 1)); + const tabBarHeight = shouldShowTabBar ? layout.tabBar.height : 0; @@ - {instanceTabs && instanceTabs.length > 1 && ( + {shouldShowTabBar && instanceTabs && ( <TabBar tabs={instanceTabs} selectedIndex={selectedTabIndex} /> )}🤖 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/components/RunApp.tsx` around lines 3406 - 3411, The parent currently only renders TabBar when instanceTabs && instanceTabs.length > 1, which prevents TabBar's zero-tab empty state from showing; change the render gate to allow an empty array (e.g., instanceTabs != null or instanceTabs && instanceTabs.length >= 0) so TabBar can render when instanceTabs.length === 0, and update the matching height calculation that currently checks instanceTabs.length > 1 to use the same new condition (or check instanceTabs.length > 0) so the layout reserves space for the TabBar's "No remotes configured" hint; references: instanceTabs, TabBar, selectedTabIndex.
🧹 Nitpick comments (2)
tests/remote/remote.test.ts (1)
649-670: ⚡ Quick winAdd regression tests for remote-only empty-tab safety
Good coverage on local-tab inclusion/exclusion. Please also assert that remote-only with zero remotes does not break navigation helpers (
selectNextTab,selectPreviousTab,selectTab(NaN)) and thatisViewingRemote()followstab.isLocal.🤖 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 `@tests/remote/remote.test.ts` around lines 649 - 670, Add assertions to the Remote-only test that when constructing InstanceManager with { remoteOnly: true } and no remotes, navigation helpers don't throw and behave sensibly: after await manager.initialize() call manager.selectNextTab(), manager.selectPreviousTab(), and manager.selectTab(NaN) (assert they do not throw and that selected tab index/state remains stable), and assert manager.isViewingRemote() mirrors the current tab's isLocal flag (false for remote-only). Update the test block that creates new InstanceManager({ remoteOnly: true }) and uses initialize/getTabs to include these checks referencing InstanceManager, initialize, getTabs, selectNextTab, selectPreviousTab, selectTab, and isViewingRemote.tests/commands/run.test.ts (1)
304-314: ⚡ Quick winAdd command-level tests for remote-only guard paths.
These tests cover parsing/help, but not runtime guard behavior (
--remote-onlywith--listen,--headless, and empty remotes). A smallexecuteRunCommandsuite with mockedlistRemotes/process.exitwould lock down the new safety checks.Also applies to: 395-395
🤖 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 `@tests/commands/run.test.ts` around lines 304 - 314, Add tests for runtime guard behavior by creating an executeRunCommand test suite that invokes executeRunCommand with combinations of flags: remoteOnly true/false together with --listen and --headless, and with listRemotes mocked to return empty or non-empty arrays; mock process.exit (and restore) to assert it is called for invalid combinations (e.g., --remote-only with --listen or --headless, or --remote-only with no remotes) and not called for valid cases; use the existing parseRunArgs tests as a guide and reference executeRunCommand, listRemotes, process.exit, and the remoteOnly flag to locate the code under test and set up appropriate mocks and expectations.
🤖 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/commands/run.tsx`:
- Around line 2230-2301: Create and assign the quit Promise (set
resolveQuitPromise) before any listeners/rendering so gracefulShutdown can
always resolve it; specifically, move the await new Promise<void>((resolve) => {
resolveQuitPromise = resolve; }); to before creating the interrupt handler /
calling process.on('SIGTERM') and before root.render (so resolveQuitPromise is
set prior to the definitions/uses of gracefulShutdown, interruptHandler,
process.on, and root.render).
In `@src/remote/instance-manager.ts`:
- Around line 72-78: isViewingRemote() currently infers remote view by
selectedIndex > 0 which breaks when remoteOnly is true; change the check to be
tab-based instead: inside the InstanceManager class update isViewingRemote() to
consult the current tab object (e.g., this.tabs[this.selectedIndex]) or
this.remoteOnly flag and return true if that tab is a remote tab (e.g.,
tab.isRemote) or if remoteOnly is set, ensuring you reference isViewingRemote(),
isRemoteOnly(), this.selectedIndex and this.tabs to locate and fix the logic.
- Around line 84-87: initialize() currently sets this.tabs = [] in remote-only
mode which is valid, but navigation methods can compute out-of-range indices and
cause selectTab to dereference undefined; update selectTab to defensively
validate the incoming index (ensure it's a finite integer, clamp to
0..this.tabs.length-1, and return early when this.tabs.length === 0), and update
any tab-navigation helpers (e.g., next/prev/tab index calculators or methods
that call selectTab) to check this.tabs.length before computing or passing
indices so no code ever indexes this.tabs when it's empty; reference
initialize(), this.tabs, remoteOnly, createLocalTab(), and selectTab when making
the changes.
In `@src/tui/components/TabBar.tsx`:
- Around line 196-198: The empty-state hint in the TabBar component incorrectly
instructs "Press R" but the actual keybinding to open remote management is 'a';
update the displayed string in the TabBar render where the <text
fg={colors.fg.dim}> node shows "No remotes configured. Press R to manage
remotes." to instead reference the correct key ("Press A") so the UI matches the
actual binding used by the remote-management handler.
---
Outside diff comments:
In `@src/tui/components/RunApp.tsx`:
- Around line 3406-3411: The parent currently only renders TabBar when
instanceTabs && instanceTabs.length > 1, which prevents TabBar's zero-tab empty
state from showing; change the render gate to allow an empty array (e.g.,
instanceTabs != null or instanceTabs && instanceTabs.length >= 0) so TabBar can
render when instanceTabs.length === 0, and update the matching height
calculation that currently checks instanceTabs.length > 1 to use the same new
condition (or check instanceTabs.length > 0) so the layout reserves space for
the TabBar's "No remotes configured" hint; references: instanceTabs, TabBar,
selectedTabIndex.
---
Nitpick comments:
In `@tests/commands/run.test.ts`:
- Around line 304-314: Add tests for runtime guard behavior by creating an
executeRunCommand test suite that invokes executeRunCommand with combinations of
flags: remoteOnly true/false together with --listen and --headless, and with
listRemotes mocked to return empty or non-empty arrays; mock process.exit (and
restore) to assert it is called for invalid combinations (e.g., --remote-only
with --listen or --headless, or --remote-only with no remotes) and not called
for valid cases; use the existing parseRunArgs tests as a guide and reference
executeRunCommand, listRemotes, process.exit, and the remoteOnly flag to locate
the code under test and set up appropriate mocks and expectations.
In `@tests/remote/remote.test.ts`:
- Around line 649-670: Add assertions to the Remote-only test that when
constructing InstanceManager with { remoteOnly: true } and no remotes,
navigation helpers don't throw and behave sensibly: after await
manager.initialize() call manager.selectNextTab(), manager.selectPreviousTab(),
and manager.selectTab(NaN) (assert they do not throw and that selected tab
index/state remains stable), and assert manager.isViewingRemote() mirrors the
current tab's isLocal flag (false for remote-only). Update the test block that
creates new InstanceManager({ remoteOnly: true }) and uses initialize/getTabs to
include these checks referencing InstanceManager, initialize, getTabs,
selectNextTab, selectPreviousTab, selectTab, and isViewingRemote.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 33450430-a694-4334-9e36-925c4ced0490
📒 Files selected for processing (6)
src/commands/run.tsxsrc/remote/instance-manager.tssrc/tui/components/RunApp.tsxsrc/tui/components/TabBar.tsxtests/commands/run.test.tstests/remote/remote.test.ts
- Resolve quit-promise race in runRemoteOnlyTui (SIGTERM before await) - Guard selectTab/selectNextTab/selectPreviousTab against zero tabs and NaN/non-integer indices - Show TabBar in remote-only mode even with zero tabs so empty-state hint is visible - Update empty-tab hint text to "Press A to add a remote" - Add integration tests for --remote-only flag conflicts and zero-tab navigation safety
Contributor
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 `@tests/commands/run-remote-only.test.ts`:
- Around line 152-174: Replace brittle console.log assertions with direct checks
of the remote-only behavior: remove the expect(logOutput).toContain(...) lines
and instead assert the renderer invocation/error for the remote-only path by
checking the caught error from executeRunCommand (e.g.,
expect(caught?.message).toContain('test-mock: createCliRenderer disabled')) and,
where relevant, assert that mockedRemotes was provided (use the mockedRemotes
array length or the createCliRenderer mock invocation to verify 1 vs 3 remotes)
so the tests rely on the renderer invocation/error and mockedRemotes rather than
console output.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e34a820b-9534-4db2-83e0-847cfc36f5c1
📒 Files selected for processing (7)
src/commands/run.tsxsrc/remote/instance-manager.tssrc/tui/components/RunApp.tsxsrc/tui/components/TabBar.tsxtests/commands/run-remote-only.test.tstests/commands/run.test.tstests/remote/remote.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/tui/components/TabBar.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tui/components/RunApp.tsx
- tests/remote/remote.test.ts
- src/commands/run.tsx
Replace brittle console.log assertions with direct checks on the createCliRenderer mock counter and the caught error. The earlier assertions failed in CI because mock.module pollution from tests/engine/execution-engine.test.ts (which stubs getAgentRegistry without registerBuiltin) caused initializePlugins to throw before the success-path console.log ever ran. Isolate tests/commands/run-remote-only.test.ts in its own CI batch so the mocked @OpenTui modules + remote/index don't leak to other tests and the agent-registry pollution from other batches doesn't leak in. Same pattern already used for info.test.ts and beads-rust-bv-tracker. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/commands/run-remote-only.test.ts (1)
207-208: ⚡ Quick winAvoid fixed sleep for signal-handler readiness.
The fixed
setTimeout(..., 50)can make this test flaky on slower CI runs. Prefer waiting on an observable condition before emittingSIGTERM.Proposed diff
- // Give the TUI a tick to install the SIGTERM handler. - await new Promise((resolve) => setTimeout(resolve, 50)); + // Wait until startup has progressed before emitting SIGTERM. + for (let i = 0; i < 100 && createCliRendererCallCount === 0; i++) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + expect(createCliRendererCallCount).toBe(1);🤖 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 `@tests/commands/run-remote-only.test.ts` around lines 207 - 208, Replace the fixed 50ms sleep used to "Give the TUI a tick to install the SIGTERM handler" by actively waiting for a readiness condition: remove the await new Promise((resolve) => setTimeout(resolve, 50)) and instead poll or await an observable that verifies the TUI has installed its SIGTERM handler (for example use a helper like waitForCondition that checks process.listenerCount('SIGTERM') > 0 on the spawned TUI process or waits for a TUI-emitted "ready" event). Ensure the test fails with a clear timeout if the condition isn't met to avoid flakiness; update references around the TUI/spawn logic where the sleep was used (the existing new Promise usage).
🤖 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.
Nitpick comments:
In `@tests/commands/run-remote-only.test.ts`:
- Around line 207-208: Replace the fixed 50ms sleep used to "Give the TUI a tick
to install the SIGTERM handler" by actively waiting for a readiness condition:
remove the await new Promise((resolve) => setTimeout(resolve, 50)) and instead
poll or await an observable that verifies the TUI has installed its SIGTERM
handler (for example use a helper like waitForCondition that checks
process.listenerCount('SIGTERM') > 0 on the spawned TUI process or waits for a
TUI-emitted "ready" event). Ensure the test fails with a clear timeout if the
condition isn't met to avoid flakiness; update references around the TUI/spawn
logic where the sleep was used (the existing new Promise usage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1de126d7-2360-4047-a3d1-73d41c54e306
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/commands/run-remote-only.test.ts
Add a Remote-Only Mode section to docs/cli/run.mdx covering the new client-only TUI mode (no local engine, no local tab), including flag-conflict errors, the empty-remotes fail-fast, and the natural pairing with --listen on the server side. Also cross-reference --remote-only from docs/cli/remote.mdx and surface it in the Remote Control section of docs/cli/overview.mdx so it's discoverable from the main CLI index. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
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 `@website/content/docs/cli/run.mdx`:
- Line 280: The docs currently contradict: update the empty-state sentence that
references the TUI message "No remotes configured. Press A to add a remote." to
state that this in-TUI message appears only for interactive runs, and that when
using the --remote-only flag the CLI will fail fast and exit with a
non-interactive error instead of opening the TUI; mention the flag name
(--remote-only) and the exact TUI message so readers understand the conditional
behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8fa2806f-e928-4646-9133-b8c9ce57af96
📒 Files selected for processing (3)
website/content/docs/cli/overview.mdxwebsite/content/docs/cli/remote.mdxwebsite/content/docs/cli/run.mdx
✅ Files skipped from review due to trivial changes (1)
- website/content/docs/cli/remote.mdx
The "No remotes configured. Press A to add a remote." TUI hint only appears for an interactive session where the user removes the last remote. At startup, --remote-only with zero remotes fails fast with a non-interactive error (exit 1) and never opens the TUI. Update the Behavior table to spell this out and link to the Fail-fast section. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closed
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
--remote-onlyCLI flag that launches the TUI without starting a local execution engine, acting purely as a client for configured remotesrunRemoteOnlyTuifunction handles the remote-only lifecycle with the same interrupt/shutdown plumbing as the normal TUIInstanceManageraccepts anoptionsobject withremoteOnly: boolean; when true,initialize()skips creating the local tab--remote-onlywith--listenor--headlessexits with a clear error messageremotes.tomlbefore proceeding, with actionable error outputisViewingRemoteto usetab.isLocal === falseinstead ofselectedTabIndex > 0so remote-only mode (where index 0 is already a remote) works correctlyTabBarrenders a "No remotes configured" hint when the tab list is emptye) and delete (d) remote key bindings no longer requireselectedTabIndex > 0Testing
tests/commands/run.test.ts: verifies--remote-onlyparses toremoteOnly: true, default isundefined, and help text includes the flagtests/remote/remote.test.ts: verifies default constructor produces a local tab on init;remoteOnly: trueskips the local tab andisRemoteOnly()returns the correct valueralph-tui run --remote-onlywith no remotes configured — should print actionable error and exitralph-tui run --remote-onlywith a configured remote — TUI should open showing only remote tabs--remote-only --listenand--remote-only --headlessshould each exit with a conflict errorSummary by CodeRabbit
New Features
--remote-onlyCLI flag to run the TUI as a remote-only client (TUI-only, requires at least one configured remote). Help/examples updated; command fails fast with guidance if no remotes exist.Bug Fixes
Tests
--remote-only, conflict validations, initialization, and tab navigation.Chores
Documentation