fix: resolve /clear command and ESC key lag caused by hooks system#2656
fix: resolve /clear command and ESC key lag caused by hooks system#2656
Conversation
- Make hook events fire-and-forget in clearCommand to avoid blocking UI - Move context.ui.clear() before resetChat for immediate responsiveness - Add hasHooksForEvent() fast-path check to HookSystem and Config - Skip MessageBus round-trips in client.ts when no hooks are registered - Add comprehensive unit tests for all changes Fixes #2651
📋 Review SummaryThis PR addresses performance lag in the 🔍 General Feedback
🎯 Specific Feedback🟡 High Priority
🟢 Medium Priority
🔵 Low Priority
✅ Highlights
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
TLDR
Fix performance lag when using
/clearcommand and pressing ESC to stop conversations. The hooks system was introducing unnecessary blockingawaitcalls and MessageBus round-trips even when no hooks were registered.Screenshots / Video Demo
N/A — no user-facing visual change. This is a performance fix that eliminates noticeable lag/delay.
Dive Deeper
Root Cause
Two main sources of lag were identified:
/clearcommand (clearCommand.ts): Two sequentialawaitcalls tofireSessionEndEvent()andfireSessionStartEvent()blocked beforecontext.ui.clear()was called. Even with no hooks registered, these async calls introduced latency.ESC key (
client.ts):sendMessageStream()always performedmessageBus.request()forUserPromptSubmitandStophook events whenenableHookswastrue, regardless of whether any hooks were actually registered. EachmessageBus.request()involvesrandomUUID(), Promise creation, EventEmitter subscription, and timeout setup.Fix
Fire-and-forget hooks in
/clear: Changed hook event calls fromawaitto fire-and-forget (.catch()for error handling), and movedcontext.ui.clear()beforeresetChat()for immediate UI responsiveness.Fast-path check: Added
hasHooksForEvent(eventName)method toHookSystemandConfigthat checks theHookRegistryfor registered hooks. This is an O(1) check that completely skips the expensive MessageBus round-trip when no hooks are configured for a given event.Files Changed
packages/cli/src/ui/commands/clearCommand.tspackages/core/src/hooks/hookSystem.tshasHooksForEvent()methodpackages/core/src/config/config.tshasHooksForEvent()proxy methodpackages/core/src/core/client.ts**/**.test.ts(4 files)Reviewer Test Plan
npm start/clear— should clear instantly with no lag--experimental-hooks) and repeat steps 2-3 — behavior should be the samenpm run testto verify all tests passTesting Matrix
Linked issues / bugs
Fixes #2651