feat(launch): add herdr session launcher - #16
Conversation
📝 WalkthroughWalkthroughOpenTab now supports Herdr tab and split launches through a unified backend API. Backend selection handles nested multiplexers and launcher hooks. The TUI exposes backend-specific targets, and documentation and tests cover local, remote, and error paths. ChangesHerdr launch integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant OpenTabTUI
participant launch_command
participant HerdrCLI
User->>OpenTabTUI: select a tab or split target
OpenTabTUI->>launch_command: dispatch launch with selected backend
launch_command->>HerdrCLI: create tab or split
HerdrCLI-->>launch_command: return JSON pane ID
launch_command->>HerdrCLI: run command in pane
HerdrCLI-->>OpenTabTUI: return launch result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 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 `@README.md`:
- Line 232: Update the README table entry for the L key to qualify the SSH
reopening behavior: sessions pulled from another machine should reopen over SSH
only when an SSH target exists in remotes.json, while URL-based pulls should be
described as providing only the copied command. Preserve the existing launcher
examples and key behavior.
In `@tests/test_tui_app.py`:
- Around line 2081-2086: Pin backend detection in the affected UI tests by
stubbing ot.util.launch_backend: return "tmux" for the tmux phase and None for
the copy-only phase at tests/test_tui_app.py lines 2081-2086, and return "tmux"
at lines 2158-2163 and 2352-2357. Save and restore the original launch_backend
in each test’s finally block, while leaving
test_launch_menu_uses_the_innermost_nested_multiplexer environment-based.
In `@tests/test_util.py`:
- Around line 476-496: Make the no-hook assertions in
test_launch_backend_prefers_hook_and_selects_innermost_multiplexer independent
of the real config directory by temporarily patching ot.util.launcher_hook to
return None before the TMUX and HERDR_ENV checks. Restore the original
launcher_hook in the test’s existing finally block, then run the
OPENTAB_LAUNCHER hook-precedence assertion against the real function.
🪄 Autofix
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 Plus
Run ID: a595e4c0-48b2-4364-ad12-aefff6a95f72
📒 Files selected for processing (12)
README.mddocs/architecture.mddocs/keys.mddocs/privacy.mdsrc/opentab/__init__.pysrc/opentab/tui/app.pysrc/opentab/tui/renderer.pysrc/opentab/util.pytests/__init__.pytests/test_doctor.pytests/test_tui_app.pytests/test_util.py
|
Looks good — I checked it against herdr 0.7.5's source and the flags, JSON paths, create-then-run sequence and nesting detection all hold up. One thing to fix first. herdr pane split --current resolves via $HERDR_PANE_ID (src/cli/pane.rs:517); when that's unset herdr silently falls through to the UI-focused pane (src/app/api/panes.rs:40), and popup panes strip the variable on purpose (src/app/popup.rs:154). So L → s from a popup can type claude --resume … into someone else's pane. Pass --pane "$HERDR_PANE_ID" so it fails visibly, or hide the split rows when it's unset. Two doc fixes: herdr pane run is pane.send_input (src/cli/pane.rs:930), and the socket API isn't deprecated. And let palette_writes_ignored() call your new in_herdr() — its comment says there should only ever be one copy of that check. Also the popup wording, in launch_targets() and docs/keys.md: herdr does have popups, they're just only openable from inside herdr — a [[keys.command]] type = "popup" binding, or a plugin pane entrypoint. Nothing an external caller can drive, so leaving p off the menu is right; "no native popup" isn't. Rest is minor: stale launch_menu_backend, raw JSON in error toasts, an unreachable ValueError path, and the docstring blank lines from a newer ruff than CI's 0.1.15. |
d31eae7 to
226cae6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/opentab/util.py (1)
786-792: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep one validation site for Herdr launch kinds.
herdr_launchrejects unsupported kinds before it calls_herdr_cli_launch, so theValueErrorraises inherdr_create_argvare unreachable on this path. Theexcept ValueErrorblock inlaunch_command(Line 889) is also unreachable, becauseherdr_launchreturns a message instead of raising. Keep the raises inherdr_create_argvfor direct callers, and drop the duplicated guard plus the dead handler.♻️ Proposed refactor
def herdr_launch(kind: str, directory: str, command: str) -> str | None: """Create a Herdr tab or split through its CLI, then run the command in it.""" - if kind not in ("window", "hsplit", "vsplit"): - if kind == "popup": - return "herdr does not support popups" - return f"unknown Herdr launch kind: {kind}" - return _herdr_cli_launch(kind, directory, command) + try: + return _herdr_cli_launch(kind, directory, command) + except ValueError as exc: + return str(exc)Then simplify
launch_command:if backend == "herdr": - try: - return herdr_launch(kind, directory, command) - except ValueError as exc: - return str(exc) + return herdr_launch(kind, directory, command)The existing tests keep passing:
herdr_launch("popup", ...)still returns"herdr does not support popups".🤖 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/opentab/util.py` around lines 786 - 792, Remove the unsupported-kind validation from herdr_launch so it delegates all kinds to _herdr_cli_launch, preserving herdr_create_argv as the single validation site while keeping direct herdr_launch("popup", ...) behavior unchanged. In launch_command, remove the now-dead ValueError exception handler around the Herdr launch path.
🤖 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/architecture.md`:
- Around line 146-149: Update the OpenTab description to state that it uses the
CLI and never opens a Herdr socket, while retaining the unchanged visible and
copyable command behavior. Remove pane.send_input from the excluded operations,
leaving only layout.apply and pane.current.
In `@src/opentab/util.py`:
- Around line 866-868: Update the nonzero-return handling in the launch-result
helper to strip proc.stderr before selecting the fallback, so whitespace-only
stderr produces the "{label} failed" message instead of an empty string.
Preserve returning None for successful processes and ensure _do_launch receives
a non-empty failure result for every failed launch.
In `@tests/test_tui_app.py`:
- Around line 2296-2304: Update the test setup and cleanup around the tmux/Herdr
assertions to isolate launch_backend from launcher hooks: save the existing
OPENTAB_LAUNCHER value, clear it during the test, and restore or remove it in
the finally block. Also stub ot.util.launcher_hook if needed so real launcher
configuration cannot affect the backend assertions.
In `@tests/test_util.py`:
- Line 400: Update the assertion in the test covering captured calls so it
checks that no invocation contains the argv sequence representing the “pane
current” subcommand, rather than searching for the bare string “current”.
Preserve validation across every call and assert against the actual split argv
elements.
---
Nitpick comments:
In `@src/opentab/util.py`:
- Around line 786-792: Remove the unsupported-kind validation from herdr_launch
so it delegates all kinds to _herdr_cli_launch, preserving herdr_create_argv as
the single validation site while keeping direct herdr_launch("popup", ...)
behavior unchanged. In launch_command, remove the now-dead ValueError exception
handler around the Herdr launch path.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05088bbc-82b0-4426-b31c-f28453b80a25
📒 Files selected for processing (12)
README.mddocs/architecture.mddocs/keys.mddocs/privacy.mdsrc/opentab/__init__.pysrc/opentab/tui/app.pysrc/opentab/tui/renderer.pysrc/opentab/util.pytests/__init__.pytests/test_doctor.pytests/test_tui_app.pytests/test_util.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/init.py
- tests/test_doctor.py
- docs/keys.md
- README.md
- docs/privacy.md
- src/opentab/tui/renderer.py
- src/opentab/init.py
- src/opentab/tui/app.py
226cae6 to
9941835
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/opentab/util.py (2)
759-768: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a type annotation for
proc.
_herdr_failureusesgetattron an untyped parameter. Annotate it assubprocess.CompletedProcess[str]and readproc.stderrandproc.returncodedirectly, since both call sites pass a completed process.🤖 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/opentab/util.py` around lines 759 - 768, Update _herdr_failure to annotate proc as subprocess.CompletedProcess[str], then access proc.stderr and proc.returncode directly instead of using getattr. Preserve the existing diagnostic formatting and fallback exit-status behavior.
722-753: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the two split branches.
hsplitandvsplitdiffer only in the--directionvalue. One branch with a direction map removes the duplicated pane lookup and argv literal.♻️ Proposed refactor
- if kind == "hsplit": - pane = herdr_pane_id() - if pane is None: - raise ValueError("HERDR_PANE_ID is required for Herdr splits") - return [ - herdr, - "pane", - "split", - "--pane", - pane, - "--direction", - "right", - "--cwd", - directory, - "--focus", - ] - if kind == "vsplit": - pane = herdr_pane_id() - if pane is None: - raise ValueError("HERDR_PANE_ID is required for Herdr splits") - return [ - herdr, - "pane", - "split", - "--pane", - pane, - "--direction", - "down", - "--cwd", - directory, - "--focus", - ] + direction = {"hsplit": "right", "vsplit": "down"}.get(kind) + if direction is not None: + pane = herdr_pane_id() + if pane is None: + raise ValueError("HERDR_PANE_ID is required for Herdr splits") + return [herdr, "pane", "split", "--pane", pane, + "--direction", direction, "--cwd", directory, "--focus"]🤖 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/opentab/util.py` around lines 722 - 753, Collapse the duplicated hsplit and vsplit branches in the split-command builder by mapping each kind to its direction and sharing the Herdr pane lookup, validation, and argument construction. Preserve “right” for hsplit and “down” for vsplit, including the existing missing HERDR_PANE_ID error.
🤖 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 `@src/opentab/util.py`:
- Around line 759-768: Update _herdr_failure to annotate proc as
subprocess.CompletedProcess[str], then access proc.stderr and proc.returncode
directly instead of using getattr. Preserve the existing diagnostic formatting
and fallback exit-status behavior.
- Around line 722-753: Collapse the duplicated hsplit and vsplit branches in the
split-command builder by mapping each kind to its direction and sharing the
Herdr pane lookup, validation, and argument construction. Preserve “right” for
hsplit and “down” for vsplit, including the existing missing HERDR_PANE_ID
error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 405eada6-40c6-4283-8a2e-6c842f30984c
📒 Files selected for processing (12)
README.mddocs/architecture.mddocs/keys.mddocs/privacy.mdsrc/opentab/__init__.pysrc/opentab/tui/app.pysrc/opentab/tui/renderer.pysrc/opentab/util.pytests/__init__.pytests/test_doctor.pytests/test_tui_app.pytests/test_util.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/init.py
- README.md
- docs/privacy.md
- src/opentab/init.py
- docs/keys.md
- tests/test_doctor.py
- src/opentab/tui/renderer.py
- src/opentab/tui/app.py
|
thank you for this feature! |
2 participants
Summary
Validation
Summary by CodeRabbit
Lmenu.