Remove auto-hold typing detection from terminals#144
Conversation
The auto-HOLD feature (switch to hold mode on first keystroke when multiple agents are running, release on Enter/blur) shipped in the #119 era (682fb80) but is buggy in practice. Remove it entirely: - use-terminal: drop autoHold/onAutoHoldStart/onAutoHoldRelease params, the typing-active state machine, the held-input send path and its input serialization queue, and the blur release handler. sendInput is a plain synchronous fast-path again. - TerminalInstance: drop the pass-through props. - TerminalPane: drop makeAutoHoldHandlers, the runningAgents>1 autoHold computation, and all prop threading through TerminalProject, SplitTerminalTile, and SplitTerminalPage. Manual HOLD/LIVE control via the pending-messages delivery-mode menu is unchanged; agent typing indicators (typing-store) are unrelated and unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR removes auto-hold functionality from terminal rendering. The useTerminal hook signature is simplified by removing autoHold and related callbacks; input handling is refactored to use direct sendInputFast calls instead of queued async logic. Blur event listeners and handleBlur cleanup code are deleted. This change cascades through TerminalInstance, which drops auto-hold props, and TerminalPane, which removes makeAutoHoldHandlers and stops wiring auto-hold callbacks in split and tabbed layouts. ChangesTerminal Auto-Hold Removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the "auto-hold" feature and its associated props and handlers (autoHold, onAutoHoldStart, onAutoHoldRelease, makeAutoHoldHandlers) across the terminal components (TerminalInstance, TerminalPane, TerminalProject, SplitTerminalTile, SplitTerminalPage) and the useTerminal hook. The sendInput logic in useTerminal has been simplified to directly invoke pear.broker.sendInputFast without input queuing or auto-hold transitions. The review feedback suggests adding error handling to the pear.broker.sendInputFast IPC call to prevent unhandled promise rejections or uncaught exceptions in the renderer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| typingActiveRef.current = false | ||
| await onAutoHoldReleaseRef.current?.(true) | ||
| } | ||
| pear.broker.sendInputFast(projectId, agentName, data) |
There was a problem hiding this comment.
Since pear.broker.sendInputFast is an IPC call that may return a Promise (or throw synchronously if the broker is disconnected), it is safer to handle potential errors or rejections to avoid unhandled promise rejections or uncaught exceptions in the renderer.
| pear.broker.sendInputFast(projectId, agentName, data) | |
| pear.broker.sendInputFast(projectId, agentName, data).catch((err) => { console.warn('[terminal] sendInputFast failed:', err); }) |
There was a problem hiding this comment.
Not applicable: pear.broker.sendInputFast is typed => void (fire-and-forget ipcRenderer.send in the preload, src/shared/types/ipc.ts:869) — chaining .catch() on it would be a type error. This line restores the exact pre-auto-hold call shape; broker-disconnect handling lives on the main-process side of the channel.
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Reviewed the PR diff and traced the changed terminal input path across the renderer hook, terminal components, IPC typing, and related delivery-mode code. I did not find a current checkout defect or stale auto-hold call site that needed a code fix. Validation run: I did not verify remote GitHub CI status, mergeability, or pending checks from this sandbox, so I’m not marking this as READY. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Summary
useTerminalloses theautoHold/onAutoHoldStart/onAutoHoldReleaseparams, the typing-active state machine, the held-input send path + input serialization queue, and the blur release listener —sendInputis a plain synchronous fast-path againTerminalPanelosesmakeAutoHoldHandlers, therunningAgents > 1computation, and all prop threading throughTerminalProject/SplitTerminalTile/SplitTerminalPage;TerminalInstanceloses the pass-through propsWhy
The detection is buggy in practice (mode flapping on keystrokes). Removing it entirely; manual HOLD/LIVE control via the pending-messages delivery-mode menu is unchanged, and agent typing indicators (
typing-store) are unrelated and untouched.Validation
npm run build(electron-vite) ✓npx tsc --noEmit✓npm test(88/88) ✓npx vitest run— 138/139; the 1 failure (broker.test.tspersona clone-safe payload) is a pre-existing flake that fails identically on cleanorigin/mainunder full-suite load and passes in isolation🤖 Generated with Claude Code