Skip to content

fix(desktop): autofocus new terminal#262

Merged
AviPeltz merged 2 commits intomainfrom
influential-chipmunk-941b25
Dec 5, 2025
Merged

fix(desktop): autofocus new terminal#262
AviPeltz merged 2 commits intomainfrom
influential-chipmunk-941b25

Conversation

@AviPeltz
Copy link
Copy Markdown
Collaborator

@AviPeltz AviPeltz commented Dec 5, 2025

Description

Related Issues

Type of Change

  • [x ] Bug fix
  • New feature
  • Documentation
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Improved terminal focus handling to ensure the terminal automatically receives keyboard input when activated as the active pane.

✏️ Tip: You can customize this high-level summary in your review settings.

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 5, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
website Ready Ready Preview Comment Dec 5, 2025 8:52pm

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 5, 2025

Walkthrough

The changes introduce a ref to mirror the isFocused state and add autofocus effects for the xterm terminal instance. When the terminal becomes the focused pane or during initial render, it automatically receives focus to ensure keyboard input routes correctly.

Changes

Cohort / File(s) Summary
Terminal Component Autofocus Enhancement
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
Adds isFocusedRef to track isFocused state outside closures, prevents stale state issues. Introduces effects to autofocus xterm instance when terminal becomes focused pane or on initial render if already focused. Synchronizes isFocused to ref each render.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Single file modification with straightforward React patterns (ref and effect hooks)
  • Focus behavior logic is localized and self-contained
  • Verify that the ref synchronization occurs at the correct lifecycle point
  • Ensure autofocus effects don't conflict with other focus management logic

Possibly related PRs

  • xterm ui #115: Directly modifies the same Terminal.tsx component, likely addressing related focus or interaction behavior.

Poem

🐰 A terminal once lost in the blur,
Now focuses keen when you stir,
With refs held fast and effects so true,
Your keystrokes find their rightful due!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description follows the template structure but lacks specific implementation details in all sections - no description of changes, related issues, or testing information provided. Complete the Description section with implementation details, link related issues, and document testing steps performed to verify the autofocus fix works correctly.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: fixing autofocus functionality for newly created terminals in the desktop application.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch influential-chipmunk-941b25

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 996e53e and 5c420be.

📒 Files selected for processing (1)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)

For Electron interprocess communication, ALWAYS use tRPC as defined in src/lib/trpc

Files:

  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)

apps/desktop/**/*.{ts,tsx}: Please use alias as defined in tsconfig.json when possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary

Files:

  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Avoid using any type - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code

Files:

  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/components/**/*.tsx

📄 CodeRabbit inference engine (AGENTS.md)

**/components/**/*.tsx: Create one folder per component with structure: ComponentName/ComponentName.tsx + index.ts for barrel export
Co-locate tests next to the component file they test (e.g., ComponentName.test.tsx)
Co-locate dependencies (utils, hooks, constants, config, stories) next to the file using them
Use nested components/ subdirectory within a parent component only if a sub-component is used 2+ times within that parent; otherwise keep it in the parent's components/ folder
One component per file - avoid multi-component files

Files:

  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only

Files:

  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.tsx

📄 CodeRabbit inference engine (AGENTS.md)

Call IPC methods using window.ipcRenderer.invoke() with object parameters - TypeScript will infer the exact response type automatically

Files:

  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications

Applied to files:

  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build
🔇 Additional comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (3)

45-48: isFocusedRef correctly avoids stale focus state in the creation effect

Mirroring isFocused into isFocusedRef.current each render is a good pattern here: it lets the terminal-creation effect read the latest focus state without adding isFocused to that effect’s dependency array (and therefore without re-creating the xterm instance). No changes needed; this is a clean, minimal fix. Based on learnings, keeping this as-is avoids unnecessary diff.


115-120: Autofocus effect on focus changes is appropriate and minimal

This useEffect cleanly ensures the xterm instance gets focus whenever this pane becomes focused, without touching other dependencies or causing extra rerenders. It aligns with the goal of routing keyboard input to the active terminal and stays within a very small diff surface.


147-150: Initial-render autofocus in creation effect correctly covers mount-time race

Focusing xterm inside the creation effect when isFocusedRef.current is true is the right way to handle the initial render: the earlier [isFocused] effect runs before this effect and sees xtermRef.current as null, so this block closes that gap and guarantees a newly created, already-focused terminal gets keyboard focus. Implementation looks solid with no extra dependencies.


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.

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