Skip to content

Commit 4383639

Browse files
couimetcouimet
authored
Fix Terminal Jump Focus + FocusCapability Refactor (#308)
* [bug] terminal jump (R-J) now properly steals focus 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 * Fix Terminal Jump Focus + FocusCapability Refactor 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 - Changed `terminal.show(true)` (preserveFocus) to `ideAdapter.showTerminal(terminal, StealFocus)` - Now R-J properly moves focus to the terminal - 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) ``` 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 ``` 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 * Fix `setTimeout()` call * Add timer methods as global * [PR feedback] Consolidate test logging, centralize focus constants, fix clipboard error handling Addresses CodeRabbit review on PR #308. Consolidates standalone logging tests into behavior tests per T007 rule, removes redundant tests, fixes stale JSDoc, centralizes AI assistant focus command constants, and adds missing try/catch around clipboard write in AIAssistantInsertFactory. Benefits: - Tests are more focused: logging verified alongside behavior, not in isolation - Focus command constants colocated in one file instead of split across two - Clipboard write failure in AI assistant insert is now handled gracefully Ignored Feedback: - global.setTimeout replacement: Already fixed in commits 8a05197 and 7f3bc12 Ref: #308 (review)
1 parent 9639f28 commit 4383639

34 files changed

+947
-710
lines changed

‎eslint.config.mjs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,11 @@ export default [
3636
beforeAll: 'readonly',
3737
afterAll: 'readonly',
3838
jest: 'readonly',
39+
// Timer globals
40+
setTimeout: 'readonly',
41+
clearTimeout: 'readonly',
42+
setInterval: 'readonly',
43+
clearInterval: 'readonly',
3944
},
4045
parserOptions: {
4146
// Not enabling full type-checking to keep lint fast; can be enabled later

‎packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.ts‎

Lines changed: 156 additions & 145 deletions
Large diffs are not rendered by default.

‎packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.test.ts‎

Lines changed: 48 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
import { createMockLogger } from 'barebone-logger-testing';
22
import { Result } from 'rangelink-core-ts';
33

4-
import { FocusErrorReason } from '../../destinations/capabilities/PasteExecutor';
4+
import { FocusErrorReason } from '../../destinations/capabilities/FocusCapability';
55
import { AutoPasteResult, PasteContentType } from '../../types';
66
import type { PaddingMode } from '../../utils/applySmartPadding';
77
import {
88
createMockComposablePasteDestination,
99
createMockEligibilityChecker,
10+
createMockFocusCapability,
1011
createMockFormattedLink,
11-
createMockPasteExecutor,
1212
} from '../helpers';
1313

1414
const UNUSED_PADDING_MODE = 'parameter not used' as unknown as PaddingMode;
@@ -35,10 +35,10 @@ describe('ComposablePasteDestination', () => {
3535

3636
it('should return false when unavailable without focusing', async () => {
3737
const isAvailable = jest.fn().mockResolvedValue(false);
38-
const pasteExecutor = createMockPasteExecutor();
38+
const focusCapability = createMockFocusCapability();
3939
const destination = createMockComposablePasteDestination({
4040
isAvailable,
41-
pasteExecutor,
41+
focusCapability,
4242
logger: mockLogger,
4343
});
4444
const context = { fn: 'test', mock: true };
@@ -51,24 +51,23 @@ describe('ComposablePasteDestination', () => {
5151
);
5252

5353
expect(result).toBe(false);
54-
expect(pasteExecutor.focus).not.toHaveBeenCalled();
54+
expect(focusCapability.focus).not.toHaveBeenCalled();
5555
});
5656

5757
it('should not double-pad already padded text', async () => {
5858
const mockInsert = jest.fn().mockResolvedValue(true);
59-
const pasteExecutor = createMockPasteExecutor();
60-
(pasteExecutor as unknown as { _mockInsert: jest.Mock })._mockInsert = mockInsert;
61-
pasteExecutor.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
59+
const focusCapability = createMockFocusCapability();
60+
focusCapability.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
6261

6362
const destination = createMockComposablePasteDestination({
64-
pasteExecutor,
63+
focusCapability,
6564
logger: mockLogger,
6665
});
6766
const context = { fn: 'test', mock: true };
6867

6968
await destination['performPaste'](' already-padded ', context, PasteContentType.Link, 'both');
7069

71-
expect(mockInsert).toHaveBeenCalledWith(' already-padded ', context);
70+
expect(mockInsert).toHaveBeenCalledWith(' already-padded ');
7271
});
7372

7473
it('should focus before inserting text', async () => {
@@ -78,14 +77,14 @@ describe('ComposablePasteDestination', () => {
7877
return true;
7978
});
8079

81-
const pasteExecutor = createMockPasteExecutor();
82-
pasteExecutor.focus.mockImplementation(async () => {
80+
const focusCapability = createMockFocusCapability();
81+
focusCapability.focus.mockImplementation(async () => {
8382
callOrder.push('focus');
8483
return Result.ok({ insert: mockInsert });
8584
});
8685

8786
const destination = createMockComposablePasteDestination({
88-
pasteExecutor,
87+
focusCapability,
8988
logger: mockLogger,
9089
});
9190
const context = { fn: 'test', mock: true };
@@ -97,18 +96,18 @@ describe('ComposablePasteDestination', () => {
9796
ARBITRARY_PADDING_MODE,
9897
);
9998

100-
expect(pasteExecutor.focus).toHaveBeenCalledTimes(1);
101-
expect(pasteExecutor.focus).toHaveBeenCalledWith(context);
99+
expect(focusCapability.focus).toHaveBeenCalledTimes(1);
100+
expect(focusCapability.focus).toHaveBeenCalledWith(context);
102101
expect(callOrder).toStrictEqual(['focus', 'insert']);
103102
});
104103

105104
it('should return true when insertion succeeds', async () => {
106105
const mockInsert = jest.fn().mockResolvedValue(true);
107-
const pasteExecutor = createMockPasteExecutor();
108-
pasteExecutor.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
106+
const focusCapability = createMockFocusCapability();
107+
focusCapability.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
109108

110109
const destination = createMockComposablePasteDestination({
111-
pasteExecutor,
110+
focusCapability,
112111
logger: mockLogger,
113112
});
114113
const context = { fn: 'test', mock: true };
@@ -125,11 +124,11 @@ describe('ComposablePasteDestination', () => {
125124

126125
it('should return false when insertion fails', async () => {
127126
const mockInsert = jest.fn().mockResolvedValue(false);
128-
const pasteExecutor = createMockPasteExecutor();
129-
pasteExecutor.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
127+
const focusCapability = createMockFocusCapability();
128+
focusCapability.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
130129

131130
const destination = createMockComposablePasteDestination({
132-
pasteExecutor,
131+
focusCapability,
133132
logger: mockLogger,
134133
});
135134
const context = { fn: 'test', mock: true };
@@ -145,13 +144,13 @@ describe('ComposablePasteDestination', () => {
145144
});
146145

147146
it('should return false when focus fails', async () => {
148-
const pasteExecutor = createMockPasteExecutor();
149-
pasteExecutor.focus.mockResolvedValue(
147+
const focusCapability = createMockFocusCapability();
148+
focusCapability.focus.mockResolvedValue(
150149
Result.err({ reason: FocusErrorReason.SHOW_DOCUMENT_FAILED }),
151150
);
152151

153152
const destination = createMockComposablePasteDestination({
154-
pasteExecutor,
153+
focusCapability,
155154
logger: mockLogger,
156155
});
157156
const context = { fn: 'test', mock: true };
@@ -211,16 +210,16 @@ describe('ComposablePasteDestination', () => {
211210

212211
describe('pasteLink() delegation', () => {
213212
it('should build context with formattedLink, linkLength, and paddingMode', async () => {
214-
const pasteExecutor = createMockPasteExecutor();
213+
const focusCapability = createMockFocusCapability();
215214
const destination = createMockComposablePasteDestination({
216-
pasteExecutor,
215+
focusCapability,
217216
logger: mockLogger,
218217
});
219218
const formattedLink = createMockFormattedLink('test-link');
220219

221220
await destination.pasteLink(formattedLink, 'both');
222221

223-
expect(pasteExecutor.focus).toHaveBeenCalledWith({
222+
expect(focusCapability.focus).toHaveBeenCalledWith({
224223
fn: 'ComposablePasteDestination.pasteLink',
225224
formattedLink,
226225
linkLength: 9,
@@ -231,38 +230,32 @@ describe('ComposablePasteDestination', () => {
231230

232231
it('should pass link text to insert function with paddingMode applied', async () => {
233232
const mockInsert = jest.fn().mockResolvedValue(true);
234-
const pasteExecutor = createMockPasteExecutor();
235-
pasteExecutor.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
233+
const focusCapability = createMockFocusCapability();
234+
focusCapability.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
236235

237236
const destination = createMockComposablePasteDestination({
238-
pasteExecutor,
237+
focusCapability,
239238
logger: mockLogger,
240239
});
241240
const formattedLink = createMockFormattedLink('my-link');
242241

243242
await destination.pasteLink(formattedLink, 'both');
244243

245-
expect(mockInsert).toHaveBeenCalledWith(' my-link ', {
246-
fn: 'ComposablePasteDestination.pasteLink',
247-
formattedLink,
248-
linkLength: 7,
249-
paddingMode: 'both',
250-
mock: true,
251-
});
244+
expect(mockInsert).toHaveBeenCalledWith(' my-link ');
252245
});
253246
});
254247

255248
describe('pasteContent() delegation', () => {
256249
it('should build context with contentLength and paddingMode', async () => {
257-
const pasteExecutor = createMockPasteExecutor();
250+
const focusCapability = createMockFocusCapability();
258251
const destination = createMockComposablePasteDestination({
259-
pasteExecutor,
252+
focusCapability,
260253
logger: mockLogger,
261254
});
262255

263256
await destination.pasteContent('test content', 'none');
264257

265-
expect(pasteExecutor.focus).toHaveBeenCalledWith({
258+
expect(focusCapability.focus).toHaveBeenCalledWith({
266259
fn: 'ComposablePasteDestination.pasteContent',
267260
contentLength: 12,
268261
paddingMode: 'none',
@@ -272,22 +265,17 @@ describe('ComposablePasteDestination', () => {
272265

273266
it('should pass content text to insert function with paddingMode applied', async () => {
274267
const mockInsert = jest.fn().mockResolvedValue(true);
275-
const pasteExecutor = createMockPasteExecutor();
276-
pasteExecutor.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
268+
const focusCapability = createMockFocusCapability();
269+
focusCapability.focus.mockResolvedValue(Result.ok({ insert: mockInsert }));
277270

278271
const destination = createMockComposablePasteDestination({
279-
pasteExecutor,
272+
focusCapability,
280273
logger: mockLogger,
281274
});
282275

283276
await destination.pasteContent('my content', 'both');
284277

285-
expect(mockInsert).toHaveBeenCalledWith(' my content ', {
286-
fn: 'ComposablePasteDestination.pasteContent',
287-
contentLength: 10,
288-
paddingMode: 'both',
289-
mock: true,
290-
});
278+
expect(mockInsert).toHaveBeenCalledWith(' my content ');
291279
});
292280
});
293281

@@ -303,30 +291,30 @@ describe('ComposablePasteDestination', () => {
303291

304292
it('should return false when destination unavailable', async () => {
305293
const isAvailable = jest.fn().mockResolvedValue(false);
306-
const pasteExecutor = createMockPasteExecutor();
294+
const focusCapability = createMockFocusCapability();
307295
const destination = createMockComposablePasteDestination({
308296
isAvailable,
309-
pasteExecutor,
297+
focusCapability,
310298
logger: mockLogger,
311299
});
312300

313301
const result = await destination.focus();
314302

315303
expect(result).toBe(false);
316-
expect(pasteExecutor.focus).not.toHaveBeenCalled();
304+
expect(focusCapability.focus).not.toHaveBeenCalled();
317305
});
318306

319-
it('should delegate to pasteExecutor when available', async () => {
320-
const pasteExecutor = createMockPasteExecutor();
307+
it('should delegate to focusCapability when available', async () => {
308+
const focusCapability = createMockFocusCapability();
321309
const destination = createMockComposablePasteDestination({
322-
pasteExecutor,
310+
focusCapability,
323311
logger: mockLogger,
324312
});
325313

326314
await destination.focus();
327315

328-
expect(pasteExecutor.focus).toHaveBeenCalledTimes(1);
329-
expect(pasteExecutor.focus).toHaveBeenCalledWith({
316+
expect(focusCapability.focus).toHaveBeenCalledTimes(1);
317+
expect(focusCapability.focus).toHaveBeenCalledWith({
330318
fn: 'ComposablePasteDestination.focus',
331319
mock: true,
332320
});
@@ -341,13 +329,13 @@ describe('ComposablePasteDestination', () => {
341329
});
342330

343331
it('should return false when focus fails', async () => {
344-
const pasteExecutor = createMockPasteExecutor();
345-
pasteExecutor.focus.mockResolvedValue(
332+
const focusCapability = createMockFocusCapability();
333+
focusCapability.focus.mockResolvedValue(
346334
Result.err({ reason: FocusErrorReason.TERMINAL_FOCUS_FAILED }),
347335
);
348336

349337
const destination = createMockComposablePasteDestination({
350-
pasteExecutor,
338+
focusCapability,
351339
logger: mockLogger,
352340
});
353341

0 commit comments

Comments
 (0)