Skip to content

fix(desktop): focus xterm when v2 pane becomes active via FOCUS_PANE_* hotkey#3465

Open
manoffortune wants to merge 1 commit into
superset-sh:mainfrom
manoffortune:fix/shortcut-20260415
Open

fix(desktop): focus xterm when v2 pane becomes active via FOCUS_PANE_* hotkey#3465
manoffortune wants to merge 1 commit into
superset-sh:mainfrom
manoffortune:fix/shortcut-20260415

Conversation

@manoffortune
Copy link
Copy Markdown

@manoffortune manoffortune commented Apr 15, 2026

Summary

  • FOCUS_PANE_* hotkeys (Cmd+Alt+Arrow, added in feat(desktop/hotkeys): v1 directional pane focus + best-effort v1 override migrator #3460) visually moved the active pane indicator in v2 workspaces but never transferred actual keyboard focus to the new terminal
  • Root cause: setActivePane updates the store (ctx.isActive) but there was no effect to call xterm.focus() — unlike v1, which has this in useTerminalHotkeys
  • Fix: add a useEffect in TerminalPane that calls terminal.focus() when ctx.isActive becomes true, mirroring the v1 pattern

Context

  • PREV_WORKSPACE / NEXT_WORKSPACE are also reported as broken — this is intentional: their Cmd+Alt+Up/Down bindings were freed for FOCUS_PANE_UP/DOWN in c925f4d. Users can rebind them in Settings → Keyboard.

Test plan

  • Open a v2 workspace with 2+ split terminal panes
  • Focus one terminal (click into it, type something)
  • Press Cmd+Alt+→ — confirm keyboard focus moves to the right pane (cursor blinks there, typing goes there)
  • Press Cmd+Alt+← — confirm focus returns

Summary by cubic

Fixes v2 pane focus hotkeys so keyboard focus moves to the selected terminal when using Cmd+Alt+Arrow, not just the visual highlight.

  • Bug Fixes
    • Root cause: setActivePane updated ctx.isActive but never called xterm.focus().
    • Fix: added a useEffect in TerminalPane to call terminal.focus() when ctx.isActive becomes true, matching v1 behavior.

Written for commit 1da365f. Summary will update on new commits.

Summary by CodeRabbit

  • New Features
    • Terminal panes now automatically receive keyboard focus when activated via hotkeys, improving workflow efficiency.

…* hotkey

setActivePane updates the store (ctx.isActive) but never called xterm.focus(),
so Cmd+Alt+Arrow moved the visual focus indicator without transferring actual
keyboard focus. Add a useEffect mirroring v1's useTerminalHotkeys pattern.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0d0681c7-e61e-4c76-85e9-6367aa670850

📥 Commits

Reviewing files that changed from the base of the PR and between 4ee2e61 and 1da365f.

📒 Files selected for processing (1)
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx

📝 Walkthrough

Walkthrough

A useEffect hook was added to the TerminalPane component that automatically focuses the terminal when the pane becomes active. The effect monitors ctx.isActive and the terminal instance, calling terminal.focus() when conditions are met.

Changes

Cohort / File(s) Summary
Terminal Focus Enhancement
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Added a useEffect hook that focuses the terminal pane when it becomes active via hotkey activation (e.g., FOCUS_PANE_* commands).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A terminal wakes with a gentle call,
Focus flows in, answering the pall,
When hotkeys dance and panes align,
The cursor finds its rightful line!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing focus behavior for xterm when v2 panes become active via the FOCUS_PANE_* hotkey.
Description check ✅ Passed The description includes all required template sections: Summary (with root cause and fix), Context (explaining other hotkey changes), and Test plan with clear verification steps.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 15, 2026

Greptile Summary

This PR fixes FOCUS_PANE_* hotkeys (Cmd+Alt+Arrow) in v2 workspaces: previously, triggering these shortcuts updated the active pane indicator in the store (ctx.isActive) but never transferred actual keyboard focus to the new terminal's xterm instance, so typing would still go to the previously-focused pane.

Changes:

  • Adds a single useEffect in TerminalPane.tsx that calls terminal.focus() whenever ctx.isActive becomes true or terminal transitions from null to a live xterm instance, directly mirroring the v1 behavior from useTerminalHotkeys.ts
  • The fix is minimal and correct: the terminal ref is stable after initial connection (same xterm instance is reused across reconnects), so focus is only stolen on actual pane activation or first connect — not on reconnection events
  • The guard if (!ctx.isActive || !terminal) return correctly handles the race where ctx.isActive becomes true before ensureSession resolves and attach is called; the effect re-runs once terminal becomes non-null

Confidence Score: 5/5

Safe to merge — targeted, minimal one-file fix with no side-effects beyond the intended focus transfer.

The change adds a single useEffect that is logically equivalent to the already-proven v1 pattern in useTerminalHotkeys.ts. The dependency array [ctx.isActive, terminal] is correct: terminal is a stable object reference after first connection (same xterm instance reused on reconnect), so the effect only fires on genuine isActive transitions and on initial terminal attach — not spuriously on reconnects. The early-return guard handles the async attach race cleanly. No security, data-loss, or reliability concerns.

No files require special attention.

Important Files Changed

Filename Overview
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx Adds a useEffect that calls terminal.focus() when ctx.isActive becomes true, fixing keyboard focus not following FOCUS_PANE_* hotkey navigation in v2 workspaces; implementation correctly mirrors the v1 pattern in useTerminalHotkeys.

Sequence Diagram

sequenceDiagram
    participant User
    participant HotkeySystem
    participant PaneStore
    participant TerminalPane
    participant TerminalRegistry
    participant XTerm

    User->>HotkeySystem: Press Cmd+Alt+→ (FOCUS_PANE_RIGHT)
    HotkeySystem->>PaneStore: setActivePane(newPaneId)
    PaneStore-->>TerminalPane: ctx.isActive changes to true
    Note over TerminalPane: useEffect fires<br/>[ctx.isActive, terminal]
    TerminalPane->>TerminalRegistry: getTerminal(terminalId)
    TerminalRegistry-->>TerminalPane: xterm instance
    TerminalPane->>XTerm: terminal.focus()
    Note over XTerm: Keyboard events now<br/>route to this terminal
Loading

Reviews (1): Last reviewed commit: "fix(desktop): focus xterm when v2 pane b..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant