Fix Terminal Jump Focus + FocusCapability Refactor #308
Conversation
The R-J command (cmd+r cmd+j) was not actually jumping to the terminal - it showed the terminal panel but kept focus in the editor. This was due to `terminal.show(true)` which preserves focus instead of stealing it. Changed to use `ideAdapter.showTerminal(terminal, TerminalFocusType.StealFocus)` for consistency with other terminal operations and correct focus behavior. Benefits: - R-J command now actually moves focus to the bound terminal - Consistent use of adapter pattern for terminal focus operations - Uses existing TerminalFocusType enum for explicit intent
## Summary This side-quest addresses two related issues: 1. **Bug Fix**: Terminal jump (R-J) was not actually moving focus to the terminal - it showed the terminal panel but kept focus in the editor 2. **Refactor**: Renamed `PasteExecutor` → `FocusCapability` with symmetric `InsertFactory` injection pattern ## Changes ### Bug Fix - For unreleased version - Changed `terminal.show(true)` (preserveFocus) to `ideAdapter.showTerminal(terminal, StealFocus)` - Now R-J properly moves focus to the terminal ### Architecture Refactor - Renamed types for clarity: - `PasteExecutor` → `FocusCapability` (emphasizes what it does: focus destinations) - `FocusSuccess` → `FocusedDestination` (a handle to something focused) - `CommandPasteExecutor` → `AIAssistantFocusCapability` (domain concept, not implementation) - Extracted `InsertFactory<T>` interface with class implementations: - `TerminalInsertFactory` - pastes via clipboard to terminal - `EditorInsertFactory` - inserts text at cursor - `AIAssistantInsertFactory` - clipboard paste with command execution - Made implementations symmetric: - Both Editor and Terminal capabilities now receive `InsertFactory` as constructor dependency - Both call `factory.forTarget(target)` to create insert functions - AI assistant uses `forTarget()` with no argument (void parameter) ### File Structure ``` src/destinations/capabilities/ ├── FocusCapability.ts (interface + FocusedDestination + FocusResult) ├── TerminalFocusCapability.ts ├── EditorFocusCapability.ts ├── AIAssistantFocusCapability.ts ├── FocusCapabilityFactory.ts ├── insertFactories/ │ ├── InsertFactory.ts (type definition) │ ├── terminalInsertFactory.ts │ ├── editorInsertFactory.ts │ └── aiAssistantInsertFactory.ts └── index.ts ``` ## Benefits 1. **Clear naming** - Types describe what they do, not how they do it 2. **Symmetry** - All implementations follow same pattern 3. **Testability** - InsertFactory can be mocked in unit tests 4. **Decoupling** - Focus logic separated from insert logic
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis change refactors the VSCode extension's destination architecture by replacing the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rangelink-vscode-extension/src/destinations/ComposablePasteDestination.ts (1)
538-540:⚠️ Potential issue | 🟡 MinorStale JSDoc reference to
TextInserter.Line 539 mentions "Inject mocked capabilities (TextInserter, EligibilityChecker, etc.)" but
TextInserterno longer exists — it should referenceFocusCapabilityorInsertFactory.📝 Suggested fix
* This method is exposed for unit tests that need to: - * - Inject mocked capabilities (TextInserter, EligibilityChecker, etc.) + * - Inject mocked capabilities (FocusCapability, EligibilityChecker, etc.) * - Test specific orchestration scenarios
🤖 Fix all issues with AI agents
In
`@packages/rangelink-vscode-extension/src/destinations/capabilities/insertFactories/aiAssistantInsertFactory.ts`:
- Line 28: Replace the Node-specific global.setTimeout call in
aiAssistantInsertFactory.ts with the universal setTimeout to fix the no-undef CI
error: inside the async pause awaiting the paste delay (the line using
FOCUS_TO_PASTE_DELAY_MS), change the Promise constructor to call
setTimeout(resolve, FOCUS_TO_PASTE_DELAY_MS) instead of global.setTimeout so the
code runs in all JS environments.
- Around line 22-43: The forTarget() async function currently calls
this.ideAdapter.writeTextToClipboard(text) without handling exceptions, so a
clipboard write failure will throw and bypass the pasteCommands loop; wrap the
entire function body (inside the returned async (text) => { ... }) in a
try/catch so any error from writeTextToClipboard (and any other unexpected
error) is caught, log the error via this.logger (include fn and error) and
return false; keep the existing try/catch around
this.ideAdapter.executeCommand(command) intact and ensure you still await the
FOCUS_TO_PASTE_DELAY_MS before attempting pasteCommands.
🧹 Nitpick comments (8)
packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/terminalInsertFactory.test.ts (2)
9-37: Test at lines 26-37 is redundant with the first test.The "returns true on successful paste" test (lines 26-37) is a subset of the first test (lines 9-24), which already asserts
result === trueand verifies the paste call. Consider removing it or consolidating into a single test.
54-85: Consolidate logging assertions into behavior tests instead of separate tests.The "logs success on successful paste" and "logs warning on paste failure" tests are standalone logging-only tests. Per coding guidelines, logger assertions should be included in the tests that verify the corresponding behavior (e.g., the success path test and the failure path test). As per coding guidelines: "Include logger assertions in tests that verify method behavior - consolidate logging verification with behavior tests, never create separate tests just for logging."
♻️ Suggested consolidation
it('creates an insert function that pastes text to terminal via clipboard', async () => { const mockAdapter = createMockVscodeAdapter(); - const mockTerminal = createMockTerminal({ name: 'Test Terminal' }); + const mockTerminal = createMockTerminal({ name: 'My Terminal' }); const pasteTextSpy = jest .spyOn(mockAdapter, 'pasteTextToTerminalViaClipboard') .mockResolvedValue(undefined); const factory = new TerminalInsertFactory(mockAdapter, mockLogger); const insertFn = factory.forTarget(mockTerminal); const result = await insertFn('test content'); expect(result).toBe(true); expect(pasteTextSpy).toHaveBeenCalledTimes(1); expect(pasteTextSpy).toHaveBeenCalledWith(mockTerminal, 'test content'); + expect(mockLogger.info).toHaveBeenCalledWith( + { fn: 'TerminalInsertFactory.insert', terminalName: 'My Terminal' }, + 'Terminal paste succeeded', + ); }); it('returns false when paste throws an error', async () => { const mockAdapter = createMockVscodeAdapter(); - const mockTerminal = createMockTerminal({ name: 'Test Terminal' }); - jest + const mockTerminal = createMockTerminal({ name: 'My Terminal' }); + const testError = new Error('Paste failed'); + jest .spyOn(mockAdapter, 'pasteTextToTerminalViaClipboard') - .mockRejectedValue(new Error('Paste failed')); + .mockRejectedValue(testError); const factory = new TerminalInsertFactory(mockAdapter, mockLogger); const insertFn = factory.forTarget(mockTerminal); const result = await insertFn('content'); expect(result).toBe(false); + expect(mockLogger.warn).toHaveBeenCalledWith( + { fn: 'TerminalInsertFactory.insert', terminalName: 'My Terminal', error: testError }, + 'Terminal paste failed', + ); });Then remove the standalone logging tests at lines 54-85.
packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/editorInsertFactory.test.ts (2)
21-47: Test at lines 36-47 is redundant with the first test."returns true when insertTextAtCursor succeeds" (lines 36-47) duplicates the assertion already in the first test (line 31). Consider removing it.
75-122: Consolidate logging assertions into behavior tests.Same pattern as in
terminalInsertFactory.test.ts— these three tests (lines 75-89, 91-105, 107-122) exist solely to verify logging. Per coding guidelines, fold logger assertions into the corresponding behavior tests (success, failure, and exception paths). As per coding guidelines: "Include logger assertions in tests that verify method behavior - consolidate logging verification with behavior tests, never create separate tests just for logging."♻️ Suggested consolidation
it('creates an insert function that inserts text at cursor', async () => { const mockAdapter = createMockVscodeAdapter(); const mockEditor = createTestEditor('/path/to/file.ts'); const insertSpy = jest.spyOn(mockAdapter, 'insertTextAtCursor').mockResolvedValue(true); const factory = new EditorInsertFactory(mockAdapter, mockLogger); const insertFn = factory.forTarget(mockEditor); const result = await insertFn('test content'); expect(result).toBe(true); expect(insertSpy).toHaveBeenCalledTimes(1); expect(insertSpy).toHaveBeenCalledWith(mockEditor, 'test content'); + expect(mockLogger.info).toHaveBeenCalledWith( + { fn: 'EditorInsertFactory.insert', editorUri: 'file:///path/to/file.ts' }, + 'Editor insert succeeded', + ); }); it('returns false when insertTextAtCursor returns false', async () => { const mockAdapter = createMockVscodeAdapter(); const mockEditor = createTestEditor('/path/to/file.ts'); jest.spyOn(mockAdapter, 'insertTextAtCursor').mockResolvedValue(false); const factory = new EditorInsertFactory(mockAdapter, mockLogger); const insertFn = factory.forTarget(mockEditor); const result = await insertFn('content'); expect(result).toBe(false); + expect(mockLogger.info).toHaveBeenCalledWith( + { fn: 'EditorInsertFactory.insert', editorUri: 'file:///path/to/file.ts' }, + 'Editor insert failed', + ); }); it('returns false when insertTextAtCursor throws an error', async () => { const mockAdapter = createMockVscodeAdapter(); const mockEditor = createTestEditor('/path/to/file.ts'); - jest.spyOn(mockAdapter, 'insertTextAtCursor').mockRejectedValue(new Error('Insert failed')); + const testError = new Error('Insert threw'); + jest.spyOn(mockAdapter, 'insertTextAtCursor').mockRejectedValue(testError); const factory = new EditorInsertFactory(mockAdapter, mockLogger); const insertFn = factory.forTarget(mockEditor); const result = await insertFn('content'); expect(result).toBe(false); + expect(mockLogger.warn).toHaveBeenCalledWith( + { fn: 'EditorInsertFactory.insert', editorUri: 'file:///path/to/file.ts', error: testError }, + 'Editor insert threw exception', + ); });Then remove the standalone logging tests at lines 75-122.
packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/aiAssistantInsertFactory.test.ts (2)
114-174: Consolidate logging assertions into the corresponding behavior tests.The guideline states to "never create separate tests just for logging." The clipboard-copy debug log (line 130), success info log (line 152), and all-fail info log (line 170) should be asserted within the existing behavior tests at lines 18, 67, and 92 respectively.
As per coding guidelines,
**/*.test.{ts,tsx}: "Include logger assertions in tests that verify method behavior - consolidate logging verification with behavior tests, never create separate tests just for logging."
176-194: This test is redundant.Every other test already calls
factory.forTarget()with no arguments. This test doesn't exercise any unique path beyond what's covered in the first happy-path test (line 18).packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.test.ts (1)
57-72: Dead_mockInsertassignment on Line 60.Line 60 sets
_mockInserton thefocusCapabilityobject, but line 61 immediately overridesfocusto return a differentResult.ok({ insert: mockInsert }). The_mockInsertproperty is never read in this test — the assertion on line 71 checksmockInsertdirectly.♻️ Suggested cleanup
it('should not double-pad already padded text', async () => { const mockInsert = jest.fn().mockResolvedValue(true); const focusCapability = createMockFocusCapability(); - (focusCapability as unknown as { _mockInsert: jest.Mock })._mockInsert = mockInsert; focusCapability.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));packages/rangelink-vscode-extension/src/destinations/destinationBuilders.ts (1)
212-221: Minor inconsistency:CURSOR_AI_FOCUS_COMMANDSandCLAUDE_CODE_FOCUS_COMMANDSare local constants, butGITHUB_COPILOT_CHAT_FOCUS_COMMANDSis imported from../utils.Consider colocating all three in the same place — either all local to this file or all in a shared constants module — to reduce surprise when maintaining these lists.
| forTarget(): (text: string) => Promise<boolean> { | ||
| return async (text: string): Promise<boolean> => { | ||
| const fn = 'AIAssistantInsertFactory.insert'; | ||
| await this.ideAdapter.writeTextToClipboard(text); | ||
| this.logger.debug({ fn, textLength: text.length }, 'Copied text to clipboard'); | ||
|
|
||
| await new Promise<void>((resolve) => global.setTimeout(resolve, FOCUS_TO_PASTE_DELAY_MS)); | ||
|
|
||
| for (const command of this.pasteCommands) { | ||
| try { | ||
| await this.ideAdapter.executeCommand(command); | ||
| this.logger.info({ fn, command }, 'Clipboard paste succeeded'); | ||
| return true; | ||
| } catch (error) { | ||
| this.logger.debug({ fn, command, error }, 'Paste command failed, trying next'); | ||
| } | ||
| } | ||
|
|
||
| this.logger.info({ fn, allCommandsFailed: true }, 'All clipboard paste commands failed'); | ||
| return false; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Clipboard write failure is unhandled.
If writeTextToClipboard (line 25) throws, the promise rejects with an unhandled error. The paste-command loop has try/catch, but the clipboard write does not — a failure there will propagate as an unhandled rejection rather than returning false.
🛡️ Proposed fix — wrap the entire body in try/catch
forTarget(): (text: string) => Promise<boolean> {
return async (text: string): Promise<boolean> => {
const fn = 'AIAssistantInsertFactory.insert';
- await this.ideAdapter.writeTextToClipboard(text);
- this.logger.debug({ fn, textLength: text.length }, 'Copied text to clipboard');
+ try {
+ await this.ideAdapter.writeTextToClipboard(text);
+ } catch (error) {
+ this.logger.warn({ fn, error }, 'Failed to write to clipboard');
+ return false;
+ }
+ this.logger.debug({ fn, textLength: text.length }, 'Copied text to clipboard');
- await new Promise<void>((resolve) => global.setTimeout(resolve, FOCUS_TO_PASTE_DELAY_MS));
+ await new Promise<void>((resolve) => setTimeout(resolve, FOCUS_TO_PASTE_DELAY_MS));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| forTarget(): (text: string) => Promise<boolean> { | |
| return async (text: string): Promise<boolean> => { | |
| const fn = 'AIAssistantInsertFactory.insert'; | |
| await this.ideAdapter.writeTextToClipboard(text); | |
| this.logger.debug({ fn, textLength: text.length }, 'Copied text to clipboard'); | |
| await new Promise<void>((resolve) => global.setTimeout(resolve, FOCUS_TO_PASTE_DELAY_MS)); | |
| for (const command of this.pasteCommands) { | |
| try { | |
| await this.ideAdapter.executeCommand(command); | |
| this.logger.info({ fn, command }, 'Clipboard paste succeeded'); | |
| return true; | |
| } catch (error) { | |
| this.logger.debug({ fn, command, error }, 'Paste command failed, trying next'); | |
| } | |
| } | |
| this.logger.info({ fn, allCommandsFailed: true }, 'All clipboard paste commands failed'); | |
| return false; | |
| }; | |
| } | |
| forTarget(): (text: string) => Promise<boolean> { | |
| return async (text: string): Promise<boolean> => { | |
| const fn = 'AIAssistantInsertFactory.insert'; | |
| try { | |
| await this.ideAdapter.writeTextToClipboard(text); | |
| } catch (error) { | |
| this.logger.warn({ fn, error }, 'Failed to write to clipboard'); | |
| return false; | |
| } | |
| this.logger.debug({ fn, textLength: text.length }, 'Copied text to clipboard'); | |
| await new Promise<void>((resolve) => setTimeout(resolve, FOCUS_TO_PASTE_DELAY_MS)); | |
| for (const command of this.pasteCommands) { | |
| try { | |
| await this.ideAdapter.executeCommand(command); | |
| this.logger.info({ fn, command }, 'Clipboard paste succeeded'); | |
| return true; | |
| } catch (error) { | |
| this.logger.debug({ fn, command, error }, 'Paste command failed, trying next'); | |
| } | |
| } | |
| this.logger.info({ fn, allCommandsFailed: true }, 'All clipboard paste commands failed'); | |
| return false; | |
| }; | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 28-28: ESLint: 'global' is not defined. (no-undef)
🪛 GitHub Check: Test & Validate
[failure] 28-28:
'global' is not defined
🤖 Prompt for AI Agents
In
`@packages/rangelink-vscode-extension/src/destinations/capabilities/insertFactories/aiAssistantInsertFactory.ts`
around lines 22 - 43, The forTarget() async function currently calls
this.ideAdapter.writeTextToClipboard(text) without handling exceptions, so a
clipboard write failure will throw and bypass the pasteCommands loop; wrap the
entire function body (inside the returned async (text) => { ... }) in a
try/catch so any error from writeTextToClipboard (and any other unexpected
error) is caught, log the error via this.logger (include fn and error) and
return false; keep the existing try/catch around
this.ideAdapter.executeCommand(command) intact and ensure you still await the
FOCUS_TO_PASTE_DELAY_MS before attempting pasteCommands.
...k-vscode-extension/src/destinations/capabilities/insertFactories/aiAssistantInsertFactory.ts
Outdated
Show resolved
Hide resolved
1 participant
Summary
This side-quest addresses two related issues:
PasteExecutor→FocusCapabilitywith symmetricInsertFactoryinjection patternChanges
Bug Fix - For unreleased version
terminal.show(true)(preserveFocus) toideAdapter.showTerminal(terminal, StealFocus)Architecture Refactor
Renamed types for clarity:
PasteExecutor→FocusCapability(emphasizes what it does: focus destinations)FocusSuccess→FocusedDestination(a handle to something focused)CommandPasteExecutor→AIAssistantFocusCapability(domain concept, not implementation)Extracted
InsertFactory<T>interface with class implementations:TerminalInsertFactory- pastes via clipboard to terminalEditorInsertFactory- inserts text at cursorAIAssistantInsertFactory- clipboard paste with command executionMade implementations symmetric:
InsertFactoryas constructor dependencyfactory.forTarget(target)to create insert functionsforTarget()with no argument (void parameter)File Structure
Benefits
Summary by CodeRabbit
Refactor
Tests