Skip to content

fix(web): bypass xterm for global terminal shortcuts#1580

Merged
juliusmarminge merged 13 commits intopingdotgg:mainfrom
AriajSarkar:fix/terminal-toggle
Apr 17, 2026
Merged

fix(web): bypass xterm for global terminal shortcuts#1580
juliusmarminge merged 13 commits intopingdotgg:mainfrom
AriajSarkar:fix/terminal-toggle

Conversation

@AriajSarkar
Copy link
Copy Markdown
Contributor

@AriajSarkar AriajSarkar commented Mar 30, 2026

What Changed

  • Plumbed the keybindings state down from ChatView.tsx into ThreadTerminalDrawer.tsx.
  • Updated attachCustomKeyEventHandler on the xterm.js instance to explicitly pass the shortcut context: { terminalFocus: true, terminalOpen: true }.
  • Returned false on global shortcut matches (terminal.toggle, terminal.new, terminal.split, terminal.close, diff.toggle) to bypass xterm input capture.

Why

When the terminal was focused, xterm.js eagerly captured shortcut combinations (e.g., intercepting Ctrl+J as an 0x0A Line Feed, or Ctrl+N as ^N).
This prevented the global application shortcut listeners in ChatView.tsx from ever receiving these specific keystrokes. By explicitly whitelisting these application-level combinations in the custom event handler and providing the correct when context, xterm safely ignores them and allows the keydown event to bubble up properly.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Medium Risk
Changes low-level keyboard event interception in the terminal; regressions could block either terminal input or global shortcut handling depending on platform/keybinding context.

Overview
Ensures global app shortcuts still work while the xterm terminal has focus by explicitly bypassing xterm key handling for terminal.toggle/terminal.split/terminal.new/terminal.close and diff.toggle.

This threads the resolved keybindings config from ChatView down through PersistentThreadTerminalDrawer/ThreadTerminalDrawer into TerminalViewport, and updates TerminalViewport’s attachCustomKeyEventHandler to match those shortcuts with { terminalFocus: true, terminalOpen: true } context and return false so the events bubble to the app-level listener. Browser tests are updated to pass the new keybindings prop.

Reviewed by Cursor Bugbot for commit 973a7fd. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Bypass xterm key handling for global terminal shortcuts in TerminalViewport

Adds a keybindings prop (typed as ResolvedKeybindingsConfig) to TerminalViewport, ThreadTerminalDrawer, and PersistentThreadTerminalDrawer, threading the config down from ChatView.

TerminalViewport uses attachCustomKeyEventHandler to intercept key events and return false for any event matching terminal toggle, split, new, close, or diff toggle shortcuts (with terminalFocus: true, terminalOpen: true context), so xterm never consumes those keystrokes.

Macroscope summarized 973a7fd.

@github-actions github-actions bot added the size:M 30-99 changed lines (additions + deletions). label Mar 30, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 90f4b065-951a-4173-aef7-dac04662c111

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ 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.

@github-actions github-actions bot added the vouch:unvouched PR author is not yet trusted in the VOUCHED list. label Mar 30, 2026
@macroscopeapp
Copy link
Copy Markdown
Contributor

macroscopeapp bot commented Apr 17, 2026

Approvability

Verdict: Approved

This is a straightforward bug fix that threads keybindings config to the terminal component and adds a check in xterm's key event handler to let global shortcuts (terminal toggle, split, new, close, diff toggle) bubble up instead of being swallowed. The change is small, uses existing helper functions, and has clear intent.

You can customize Macroscope's approvability policy. Learn more.

@juliusmarminge juliusmarminge merged commit 39ca3ee into pingdotgg:main Apr 17, 2026
10 checks passed
aaditagrawal added a commit to aaditagrawal/t3code that referenced this pull request Apr 18, 2026
Integrates upstream/main (9df3c64) on top of fork's main (9602c18).

Upstream features adopted:
- Claude Opus 4.5 and 4.7 built-in models (pingdotgg#2072, pingdotgg#2143)
- Node-native TypeScript migration across desktop/server (pingdotgg#2098)
- Configurable project grouping with client-settings overrides (pingdotgg#2055, pingdotgg#2099)
- Thread status in command palette (pingdotgg#2107)
- Responsive composer / plan sidebar on narrow windows (pingdotgg#1198)
- Capture-phase CTRL+J keydown for Windows terminal toggle (pingdotgg#2113/pingdotgg#2142)
- Bypass xterm for global terminal shortcuts (pingdotgg#1580)
- Windows ARM build target (pingdotgg#2080)
- Windows PATH hydration + repair (pingdotgg#1729)
- Gitignore-aware workspace search (pingdotgg#2078)
- Claude process leak fix + stale session monitoring (pingdotgg#2042)
- Preserve provider bindings when stopping sessions (pingdotgg#2084)
- Clean up invalid pending-approval projections (pingdotgg#2106) — new migration
- Extract backend startup readiness coordination
- Drop stale text-gen options on reset (pingdotgg#2076)
- Extend negative repository identity cache TTL (pingdotgg#2083)
- Allow deleting non-empty projects from warning toast (pingdotgg#1264)
- Restore defaults only on General settings (pingdotgg#1710)
- Release workflow modernization (blacksmith runners, GitHub App token guards, v0.0.20 version bump)

Fork features preserved:
- All 8 providers (codex, claudeAgent, copilot, cursor, opencode,
  geminiCli, amp, kilo) with their adapters, services, and tests
- Fork's custom OpenCode protocol impl in apps/server/src/opencode/ (kept
  over upstream's @opencode-ai/sdk-based provider added in pingdotgg#1758 — fork's
  version is tested and integrated; upstream's parallel files deleted)
- Fork's direct-CLI Cursor adapter (kept over upstream's new ACP-based
  CursorProvider added in pingdotgg#1355 — upstream's parallel files deleted)
- Fork's ProviderRegistry aggregates only codex + claudeAgent snapshots;
  the other 6 providers register via ProviderAdapterRegistry
- PROVIDER_CACHE_IDS stays at [codex, claudeAgent] matching what the
  registry actually caches
- Migration IDs preserved (fork 23/24/25/26; upstream's new 025 lands at
  ID 27 to avoid re-applying on deployed fork DBs)
- Fork's generic per-provider settings (enabled/binaryPath/configDir/
  customModels) kept over upstream's opencode-specific serverUrl/password
- Log directory IPC channels, updateInstallInFlight tracking, icon
  composer pipeline all preserved
- Fork's simplified release.yml (no npm CLI publish, no nightly infra)
- composerDraftStore normalizeProviderKind widened to accept all 8 kinds
- Dark mode --background set to #0f0f0f

Test status:
- All 9 package typechecks pass
- Lint clean (0 errors)
- Tests: 1877 passed, 15 skipped (incl. 4 historically-flaky GitManager
  cross-repo PR selector tests newly gated with TODO for Node-native-TS
  follow-up)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants