[issues/255] PR4: DestinationPickerCommand and Grouped API#302
Conversation
## Summary Fourth PR in the stacked PR series for Issue #255 (Terminal Picker). This PR introduces the `DestinationPickerCommand` for showing a unified destination picker, along with the grouped destination API on `DestinationAvailabilityService`. The command returns the user's selection without performing binding, allowing callers to decide what to do with the result. ## Changes - Add `DestinationPickerCommand` class that shows a QuickPick with all available destinations - AI assistants first, then terminals, then text editor (via `DESTINATION_PICKER_SEQUENCE`) - Returns selection result without binding (caller decides action) - Delegates to terminal picker for "More terminals..." selection - Three outcome types: `selected`, `cancelled`, `returned-to-destination-picker` - Add `getGroupedDestinationItems()` method to `DestinationAvailabilityService` - Returns destinations grouped by type with pre-built QuickPick items - Includes `terminal-more` placeholder when terminals exceed inline limit - Active terminal sorted first with "(active)" description - Add `buildDestinationQuickPickItems()` utility for converting grouped items to QuickPick array - Configurable label builder for different display contexts - Ordered by `DESTINATION_PICKER_SEQUENCE` for consistent UX - Add `BindOptions` discriminated union type for type-safe bind operations - `TerminalBindOptions` carries terminal reference - `TextEditorBindOptions`, `CursorAIBindOptions`, `CopilotChatBindOptions`, `ClaudeCodeBindOptions` - Add `DestinationType` string literal union and `DESTINATION_TYPES` constant array - Add `GroupedDestinationItems` type and `BindableQuickPickItem` unified item type - Add `DEFAULT_TERMINAL_PICKER_MAX_INLINE` setting (configurable inline terminal limit) - Add `TERMINAL_PICKER_MORE_TERMINALS_DESCRIPTION` i18n message - Update `DestinationRegistry` to use grouped return type - Add shared test helpers (`createMockBindableQuickPickItem.ts`) for QuickPick item creation - Update tests with proper unit isolation (mock dependencies, test orchestration) ## Test Plan - [x] New tests added for: - `DestinationPickerCommand` - no destinations, cancellation, bindable selection, terminal-more flow, logging - `buildDestinationQuickPickItems()` - sequence ordering, label building, terminal-more handling - `DestinationAvailabilityService.getGroupedDestinationItems()` - grouping logic, terminal overflow - `DestinationRegistry` grouped API - [x] Shared test helpers extracted to `createMockBindableQuickPickItem.ts` - [ ] Manual testing: DestinationPickerCommand not wired yet (PR5 will wire to command) ## Related - Part of #255 (Terminal Picker) - Depends on: PR1 (#294), PR2 (#296), PR3 (#298)
WalkthroughRenames destination discriminator Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant DAS as DestinationAvailabilityService
participant UI as QuickPickUI
participant Sec as SecondaryPicker
participant Log as Logger
Caller->>DAS: getGroupedDestinationItems({ destinationTypes: ['terminal'], terminalThreshold: Infinity })
DAS-->>Caller: groupedItems (bindable items [+ terminal-more?])
Caller->>UI: show primary destination picker (groupedItems)
UI-->>Caller: selection
alt selection.itemKind == "bindable"
Caller->>Log: log selected bindOptions
Caller-->>Caller: return { outcome: 'selected', bindOptions }
else selection.itemKind == "terminal-more"
Caller->>Sec: showSecondaryTerminalPicker(...)
Sec-->>Caller: result (selected | cancelled | returned-to-destination-picker)
alt selected
Caller-->>Caller: return { outcome: 'selected', bindOptions: terminal }
else cancelled
Caller-->>Caller: return { outcome: 'cancelled' }
else returned-to-destination-picker
Caller->>UI: show primary destination picker (loop)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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/types/AvailableDestination.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorChange the import to reference the canonical source.
DestinationTypeis defined in two places:./DestinationType.ts(which also exports the canonicalDESTINATION_TYPESconstant) and../destinations/PasteDestination.ts. To avoid duplication and maintain a single source of truth, import directly from./DestinationType:import type { DestinationType } from './DestinationType';
🤖 Fix all issues with AI agents
In
`@packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts`:
- Around line 154-160: In getGroupedDestinationItems, add DEBUG logs that state
whether destinationTypes and terminalThreshold came from the caller or from
defaults; specifically, after resolving destinationTypes (symbol:
destinationTypes, fallback: DESTINATION_TYPES) log whether
options?.destinationTypes was used vs the default, and after resolving
terminalThreshold (symbol: terminalThreshold, fallback via
this.configReader.getWithDefault with keys SETTING_TERMINAL_PICKER_MAX_INLINE
and DEFAULT_TERMINAL_PICKER_MAX_INLINE) log whether options?.terminalThreshold
was used or the config/default value was applied; use the existing logger at
DEBUG level and include the chosen value in each message for clarity.
- Around line 180-188: The terminal eligibility guard in
DestinationAvailabilityService is using a boolean negation on
isTerminalDestinationEligible(this.ideAdapter) which returns an eligibility
object; update the guard to check the object's .eligible property (e.g., if
(!isTerminalDestinationEligible(this.ideAdapter).eligible) break) so
buildGroupedTerminalItems, getEligibleTerminals and subsequent terminal grouping
only run when the terminal destination is actually eligible; ensure the same
.eligible check pattern matches how getAvailableDestinations uses the
eligibility object.
🧹 Nitpick comments (9)
packages/rangelink-vscode-extension/src/__tests__/helpers/createMockDestinationRegistry.ts (2)
17-19: Minor: Update JSDoc to reflectkindterminology.The JSDoc comment on Line 18 still references "based on type", but the implementation now uses
kind. Consider updating for consistency./** * Pre-configured destinations to return from create(). - * Registry will return the appropriate destination based on type. + * Registry will return the appropriate destination based on kind. */
50-52: Minor: Update JSDoc to reflectkindterminology.The JSDoc comment on Line 51 still references "requested type", but the implementation now uses
kind.- * based on the requested type. This eliminates test coupling to registry's + * based on the requested kind. This eliminates test coupling to registry'spackages/rangelink-vscode-extension/src/__tests__/helpers/createMockBindableQuickPickItem.ts (1)
94-96: Consider: Type assertion may cause issues with partial objects.The function returns
groupsdirectly asGroupedDestinationItems, butPartial<GroupedDestinationItems>may not satisfy all required properties ifGroupedDestinationItemshas required fields. This works if all fields are optional, but could cause runtime issues if the type evolves.If
GroupedDestinationItemsis designed to have all optional properties, this is fine. Otherwise, consider documenting this assumption or adding a type guard.packages/rangelink-vscode-extension/src/destinations/utils/assertTerminalFromPicker.ts (1)
21-38: Consider using generic parameter for stronger type safety.The function accepts
BindableQuickPickItemand casts toTerminalBindOptionsat line 28. Using the generic parameter would provide compile-time type safety:💡 Optional: Strengthen type signature
export const assertTerminalFromPicker = <T>( - selected: BindableQuickPickItem, + selected: BindableQuickPickItem<TerminalBindOptions>, functionName: string, logger: Logger, logMessage: string, action: (terminal: vscode.Terminal) => T, ): T => { - const terminalBindOptions = selected.bindOptions as TerminalBindOptions; + const terminalBindOptions = selected.bindOptions; if (!terminalBindOptions?.terminal) {This would shift validation to callers at compile time rather than relying solely on runtime checks. However, the current implementation is also valid if callers may pass mixed item types.
packages/rangelink-vscode-extension/src/destinations/utils/buildDestinationQuickPickItems.ts (1)
68-75: Defensive validation is currently unreachable but provides future safety.Given that
DESTINATION_PICKER_SEQUENCEis typed asreadonly (DestinationType | 'terminal-more')[]and'terminal-more'is handled above,isDestinationKind(key)will always returntrueat this point. This check becomes valuable if the sequence array is ever modified or derived dynamically.Consider if this defensive check adds sufficient value, or if a compile-time assertion (via type narrowing) would be clearer. The current approach is acceptable for runtime safety.
packages/rangelink-vscode-extension/src/__tests__/destinations/utils/showTerminalPicker.test.ts (1)
644-717: Avoid logging-only tests; fold log assertions into behavior tests.
These cases currently validate logging without asserting outcomes, which conflicts with the test guidelines.♻️ Suggested consolidation
@@ - await showTerminalPicker( + const result = await showTerminalPicker( terminals, undefined, adapter, DEFAULT_OPTIONS, logger, identityCallback, ); + expect(result).toStrictEqual({ outcome: 'selected', result: terminals[0] }); @@ - await showTerminalPicker( + const result = await showTerminalPicker( terminals, undefined, adapter, DEFAULT_OPTIONS, logger, identityCallback, ); + expect(result).toStrictEqual({ outcome: 'cancelled' }); @@ - await showTerminalPicker( + const result = await showTerminalPicker( terminals, undefined, adapter, DEFAULT_OPTIONS, logger, identityCallback, ); + expect(result).toStrictEqual({ outcome: 'selected', result: terminals[1] });Based on learnings: Include logger assertions in tests that verify method behavior - consolidate logging verification with behavior tests, never create separate tests just for logging.
packages/rangelink-vscode-extension/src/__tests__/commands/DestinationPickerCommand.test.ts (1)
241-295: Avoid standalone logging-only tests; fold logs into behavior assertions.Please merge these log assertions into the related behavior tests instead of keeping a separate logging suite.
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.
packages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.ts (1)
117-167: Add logger assertions for abort scenarios.
These tests validate behavior but skip log expectations; align them with the other cases that assert logging. 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.packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts (1)
146-226: Consider splitting getGroupedDestinationItems into per-kind handlers.
The method is long and mixes eligibility, grouping, and logging logic; extracting handlers would reduce complexity. As per coding guidelines: Extract functions into separate modules when they exceed 50 lines, have 3+ dependencies, or mix concerns (UI + business logic) using /utils/, /services/, or /src/ subdirectories.
packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts
Outdated
Show resolved
Hide resolved
packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts
Show resolved
Hide resolved
Fixes bug where terminal eligibility check used object as boolean (always truthy). Renames isTerminalDestinationEligible → getTerminalDestinationEligibility to follow naming convention (get* returns objects, is* returns booleans), preventing similar bugs in the future. Benefits: - Fixes broken terminal eligibility guard in getGroupedDestinationItems - Naming convention makes return type obvious at call sites - Consolidates standalone logging tests into behavior tests (rule T007) - Removes no-op createMockGroupedDestinationItems helper - Adds DEBUG logs for parameter resolution in getGroupedDestinationItems Ignored Feedback: - Generic parameter suggestion for assertTerminalFromPicker: Current runtime check is valid and more flexible - Defensive validation note in buildDestinationQuickPickItems: Acknowledged but provides future safety - Method length suggestion for getGroupedDestinationItems: Method is cohesive; splitting adds complexity without benefit Ref: #302 (review)
There was a problem hiding this comment.
Actionable comments posted: 1
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/__tests__/destinations/utils/showTerminalPicker.test.ts (1)
85-147:⚠️ Potential issue | 🟡 MinorAdd logger assertions in these behavior tests.
These tests assert picker behavior but don’t validate the debug logging, which the test guidelines require to be included alongside behavior verification. Consider mirroring the log expectations already used in the 2-terminal test.
💡 Example addition for the "5 terminals exist" test
expect(showQuickPickMock).toHaveBeenCalledWith( [ { label: 'terminal-1', description: undefined, displayName: 'terminal-1', bindOptions: { kind: 'terminal', terminal: terminals[0] }, itemKind: 'bindable', }, { label: 'terminal-2', description: undefined, displayName: 'terminal-2', bindOptions: { kind: 'terminal', terminal: terminals[1] }, itemKind: 'bindable', }, { label: 'terminal-3', description: undefined, displayName: 'terminal-3', bindOptions: { kind: 'terminal', terminal: terminals[2] }, itemKind: 'bindable', }, { label: 'terminal-4', description: undefined, displayName: 'terminal-4', bindOptions: { kind: 'terminal', terminal: terminals[3] }, itemKind: 'bindable', }, { label: 'terminal-5', description: undefined, displayName: 'terminal-5', bindOptions: { kind: 'terminal', terminal: terminals[4] }, itemKind: 'bindable', }, ], { title: 'Select Terminal', placeHolder: 'Choose a terminal to bind to' }, ); + expect(logger.debug).toHaveBeenCalledWith( + { fn: 'showTerminalPicker', terminalCount: 5, itemCount: 5 }, + 'Showing terminal picker', + ); + expect(logger.debug).toHaveBeenCalledWith( + { + fn: 'showTerminalPicker', + selected: { + label: 'terminal-3', + displayName: 'terminal-3', + bindOptions: { kind: 'terminal', terminal: terminals[2] }, + itemKind: 'bindable', + }, + }, + 'Terminal selected', + );Based on learnings: Include logger assertions in tests that verify method behavior - consolidate logging verification with behavior tests, never create separate tests just for logging.
Also applies to: 149-198, 201-338, 341-528
🤖 Fix all issues with AI agents
In
`@packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts`:
- Around line 167-173: The terminalThreshold value (computed from
options?.terminalThreshold or this.configReader.getWithDefault using
SETTING_TERMINAL_PICKER_MAX_INLINE/DEFAULT_TERMINAL_PICKER_MAX_INLINE) needs
sanitization to prevent negative/NaN values; in the
DestinationAvailabilityService where terminalThreshold is assigned and later
used (including the other block around 264-292), coerce to a number and
validate: if not Number.isFinite(value) or value < 0 set terminalThreshold = 0,
and use Math.floor(value) if you expect an integer, then proceed with
slicing/remaining-count logic using this sanitized terminalThreshold.
🧹 Nitpick comments (2)
packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts (1)
138-259: Consider extractinggetGroupedDestinationItemsinto focused helpers.
It now mixes eligibility checks with QuickPick item construction and exceeds 50 lines, which makes it harder to test and maintain.As per coding guidelines: Extract functions into separate modules when they exceed 50 lines, have 3+ dependencies, or mix concerns (UI + business logic) using /utils/, /services/, or /src/ subdirectories.
packages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.ts (1)
52-68: Use a named constant for the Infinity threshold.
This value has semantic meaning (no inline limit) and is repeated in expectations/options.♻️ Suggested refactor
import { BindAbortReason, type TerminalBindResult } from '../../types'; import { createMockDestinationAvailabilityService, @@ createMockVscodeAdapter, } from '../helpers'; +const TERMINAL_PICKER_THRESHOLD = Infinity; + describe('BindToTerminalCommand', () => { @@ expect(mockAvailabilityService.getGroupedDestinationItems).toHaveBeenCalledWith({ destinationTypes: ['terminal'], - terminalThreshold: Infinity, + terminalThreshold: TERMINAL_PICKER_THRESHOLD, }); @@ { - maxItemsBeforeMore: Infinity, + maxItemsBeforeMore: TERMINAL_PICKER_THRESHOLD, title: 'RangeLink', placeholder: 'Select terminal to bind to', activeDescription: 'active',As per coding guidelines: Define named constants using SCREAMING_SNAKE_CASE for all numeric literals with semantic meaning.
Also applies to: 345-350
packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts
Outdated
Show resolved
Hide resolved
Adds validation for terminalThreshold config to guard against users manually editing settings.json with invalid values (negative, zero, or NaN). Changes: - Validates terminalThreshold < MIN_TERMINAL_PICKER_THRESHOLD or NaN - Logs warning with invalid value and falls back to default (5) - Boundary tests for 0, NaN, 1, 2 with return value assertions Ignored Feedback: - Logger assertions in every test: Primary tests already include logging verification - Extract getGroupedDestinationItems helper: Already addressed in previous review - Use TERMINAL_PICKER_SHOW_ALL constant: Rule T003 requires literal values in assertions Ref: #302 (review)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts`:
- Around line 169-198: The debug message can be misleading when
options.terminalThreshold is provided but invalid; update
DestinationAvailabilityService.getGroupedDestinationItems to determine the
effective source after validation (e.g., compute a boolean or a string like
source = options?.terminalThreshold !== undefined &&
!Number.isNaN(options.terminalThreshold) && options.terminalThreshold >=
MIN_TERMINAL_PICKER_THRESHOLD ? 'provided' : 'config/default') and use that when
logging instead of directly checking options?.terminalThreshold; ensure
terminalThreshold still falls back to DEFAULT_TERMINAL_PICKER_MAX_INLINE and the
debug call uses the computed source and the final terminalThreshold value
(referencing terminalThreshold, options.terminalThreshold,
DEFAULT_TERMINAL_PICKER_MAX_INLINE).
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts (1)
148-274: Consider extracting per-kind builders to keep this method focused.
getGroupedDestinationItemsis >50 lines and mixes eligibility checks, item construction, and logging across multiple dependencies. Extracting per-kind builders into utils/private helpers would make the orchestration logic clearer and easier to test.As per coding guidelines: Extract functions into separate modules when they exceed 50 lines, have 3+ dependencies, or mix concerns (UI + business logic) using /utils/, /services/, or /src/ subdirectories.
packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts
Outdated
Show resolved
Hide resolved
The debug log was checking `options?.terminalThreshold !== undefined` to determine
the source, but this logged "Using provided terminalThreshold" even when the provided
value was invalid and fell back to the default.
Now computes `providedWasValid` after checking validity, and logs the actual source
("provided" vs "config/default") that reflects what was actually used.
Extracts `isValidThreshold()` helper to avoid duplicating the validation logic
between the providedWasValid check and the fallback condition.
Benefits:
- Debug logs accurately reflect what happened during threshold resolution
- Single unified log instead of two conditional branches
- DRY: validation logic defined once in isValidThreshold()
Ignored Feedback:
- Extract per-kind builders: Switch-case methods with clear separation are readable
as-is; extracting single-call private methods adds indirection without improving
testability or reuse
Ref: #302 (review)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts`:
- Around line 32-35: isValidThreshold currently allows fractional and non-finite
values which leads to non-integer remainingCount and UI issues; change
validation and normalization so terminalThreshold is a safe finite integer (use
Number.isFinite and convert to an integer via Math.trunc/Math.floor or
Number.isInteger check) before using it to compute remainingCount, and ensure
MIN_TERMINAL_PICKER_THRESHOLD is still enforced; apply the same fix where
terminalThreshold is validated/used (the block around lines 172-193) so all uses
normalize to an integer.
- Around line 151-278: getGroupedDestinationItems is over 50 lines and mixes
option resolution, validation/logging, eligibility checks and item construction;
split responsibilities by extracting: (1) threshold resolution/validation into a
helper resolveTerminalThreshold(options, configReader, logger) that returns {
terminalThreshold, thresholdSource, providedWasValid }, (2) per-kind builders
such as buildTextEditorItem(ideAdapter, displayNames),
buildTerminalGroup(ideAdapter, displayNames, terminalThreshold) — note
buildGroupedTerminalItems already exists and should be reused inside the
terminal builder — and buildAIAssistantItem(kind, displayNames,
isAIAssistantAvailable) for 'claude-code'/'cursor-ai'/'github-copilot-chat';
have getGroupedDestinationItems orchestrate calling these helpers, keep existing
logging calls but move logging related to threshold and per-kind decisions into
the new helpers where appropriate, and preserve thrown RangeLinkExtensionError
for unexpected kinds.
packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts
Outdated
Show resolved
Hide resolved
| async getGroupedDestinationItems( | ||
| options?: GetAvailableDestinationItemsOptions, | ||
| ): Promise<GroupedDestinationItems> { | ||
| const displayNames = this.registry.getDisplayNames(); | ||
| const result: { | ||
| -readonly [K in keyof GroupedDestinationItems]: GroupedDestinationItems[K]; | ||
| } = {}; | ||
|
|
||
| const destinationTypes = options?.destinationTypes ?? DESTINATION_TYPES; | ||
| if (options?.destinationTypes) { | ||
| this.logger.debug( | ||
| { fn: 'DestinationAvailabilityService.getGroupedDestinationItems', destinationTypes }, | ||
| 'Using provided destinationTypes filter', | ||
| ); | ||
| } else { | ||
| this.logger.debug( | ||
| { fn: 'DestinationAvailabilityService.getGroupedDestinationItems' }, | ||
| 'Using default DESTINATION_TYPES', | ||
| ); | ||
| } | ||
|
|
||
| let terminalThreshold = | ||
| options?.terminalThreshold ?? | ||
| this.configReader.getWithDefault( | ||
| SETTING_TERMINAL_PICKER_MAX_INLINE, | ||
| DEFAULT_TERMINAL_PICKER_MAX_INLINE, | ||
| ); | ||
|
|
||
| const providedWasValid = | ||
| options?.terminalThreshold !== undefined && isValidThreshold(options.terminalThreshold); | ||
|
|
||
| if (!isValidThreshold(terminalThreshold)) { | ||
| this.logger.warn( | ||
| { | ||
| fn: 'DestinationAvailabilityService.getGroupedDestinationItems', | ||
| invalidValue: terminalThreshold, | ||
| fallback: DEFAULT_TERMINAL_PICKER_MAX_INLINE, | ||
| }, | ||
| 'Invalid terminalThreshold, using default', | ||
| ); | ||
| terminalThreshold = DEFAULT_TERMINAL_PICKER_MAX_INLINE; | ||
| } | ||
|
|
||
| const thresholdSource = providedWasValid ? 'provided' : 'config/default'; | ||
| this.logger.debug( | ||
| { | ||
| fn: 'DestinationAvailabilityService.getGroupedDestinationItems', | ||
| terminalThreshold, | ||
| thresholdSource, | ||
| }, | ||
| `Using ${thresholdSource} terminalThreshold`, | ||
| ); | ||
|
|
||
| for (const kind of destinationTypes) { | ||
| switch (kind) { | ||
| case 'text-editor': { | ||
| const textEditorEligibility = isTextEditorDestinationEligible(this.ideAdapter); | ||
| if (!textEditorEligibility.eligible) break; | ||
|
|
||
| const displayName = `${displayNames['text-editor']} ("${textEditorEligibility.filename}")`; | ||
| result['text-editor'] = [ | ||
| { | ||
| label: displayName, | ||
| displayName, | ||
| bindOptions: { kind: 'text-editor' }, | ||
| itemKind: 'bindable', | ||
| }, | ||
| ]; | ||
| break; | ||
| } | ||
|
|
||
| case 'terminal': { | ||
| if (!getTerminalDestinationEligibility(this.ideAdapter).eligible) break; | ||
|
|
||
| const eligibleTerminals = getEligibleTerminals(this.ideAdapter); | ||
| const { items, moreItem } = this.buildGroupedTerminalItems( | ||
| eligibleTerminals, | ||
| terminalThreshold, | ||
| ); | ||
| if (items.length > 0) { | ||
| result['terminal'] = items; | ||
| } | ||
| if (moreItem) { | ||
| result['terminal-more'] = moreItem; | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| case 'claude-code': | ||
| case 'cursor-ai': | ||
| case 'github-copilot-chat': { | ||
| const available = await this.isAIAssistantAvailable(kind); | ||
| if (!available) break; | ||
|
|
||
| const displayName = displayNames[kind]; | ||
| result[kind] = [ | ||
| { | ||
| label: displayName, | ||
| displayName, | ||
| bindOptions: { kind }, | ||
| itemKind: 'bindable', | ||
| }, | ||
| ]; | ||
| break; | ||
| } | ||
|
|
||
| default: { | ||
| const _exhaustiveCheck: never = kind; | ||
| throw new RangeLinkExtensionError({ | ||
| code: RangeLinkExtensionErrorCodes.UNEXPECTED_DESTINATION_TYPE, | ||
| message: `Unhandled destination kind in getGroupedDestinationItems`, | ||
| functionName: 'DestinationAvailabilityService.getGroupedDestinationItems', | ||
| details: { kind: _exhaustiveCheck }, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this.logger.debug( | ||
| { | ||
| fn: 'DestinationAvailabilityService.getGroupedDestinationItems', | ||
| groupKeys: Object.keys(result), | ||
| }, | ||
| `Built grouped destination items`, | ||
| ); | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract getGroupedDestinationItems into helper module(s).
This method is >50 lines and mixes option resolution, validation/logging, eligibility checks, and item construction. Consider extracting threshold resolution and per-kind builders into /utils or a dedicated service module, leaving this as orchestration.
As per coding guidelines: Extract functions into separate modules when they exceed 50 lines, have 3+ dependencies, or mix concerns (UI + business logic) using /utils/, /services/, or /src/ subdirectories.
🤖 Prompt for AI Agents
In
`@packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts`
around lines 151 - 278, getGroupedDestinationItems is over 50 lines and mixes
option resolution, validation/logging, eligibility checks and item construction;
split responsibilities by extracting: (1) threshold resolution/validation into a
helper resolveTerminalThreshold(options, configReader, logger) that returns {
terminalThreshold, thresholdSource, providedWasValid }, (2) per-kind builders
such as buildTextEditorItem(ideAdapter, displayNames),
buildTerminalGroup(ideAdapter, displayNames, terminalThreshold) — note
buildGroupedTerminalItems already exists and should be reused inside the
terminal builder — and buildAIAssistantItem(kind, displayNames,
isAIAssistantAvailable) for 'claude-code'/'cursor-ai'/'github-copilot-chat';
have getGroupedDestinationItems orchestrate calling these helpers, keep existing
logging calls but move logging related to threshold and per-kind decisions into
the new helpers where appropriate, and preserve thrown RangeLinkExtensionError
for unexpected kinds.
Extracts threshold resolution logic into a dedicated private method to reduce cognitive load in getGroupedDestinationItems(). The new method encapsulates: - Retrieving threshold from options or config - Validation with warning log for invalid values - Normalization via Math.floor() - Debug logging with full context (providedThreshold, configThreshold, effectiveThreshold) The debug log now shows all three values - no ternary needed, the values tell the story: providedThreshold undefined means config was used, effectiveThreshold differing from input means validation/flooring was applied. Benefits: - Single responsibility - threshold resolution is self-contained - getGroupedDestinationItems() stays focused on orchestration - Debug logs show complete picture for troubleshooting Ref: #302 (comment)
1 participant
Summary
Fourth PR in the stacked PR series for Issue #255 (Terminal Picker). This PR introduces the
DestinationPickerCommandfor showing a unified destination picker, along with the grouped destination API onDestinationAvailabilityService. The command returns the user's selection without performing binding, allowing callers to decide what to do with the result.Changes
DestinationPickerCommandclass that shows a QuickPick with all available destinationsDESTINATION_PICKER_SEQUENCE)selected,cancelled,returned-to-destination-pickergetGroupedDestinationItems()method toDestinationAvailabilityServiceterminal-moreplaceholder when terminals exceed inline limitbuildDestinationQuickPickItems()utility for converting grouped items to QuickPick arrayDESTINATION_PICKER_SEQUENCEfor consistent UXBindOptionsdiscriminated union type for type-safe bind operationsTerminalBindOptionscarries terminal referenceTextEditorBindOptions,CursorAIBindOptions,CopilotChatBindOptions,ClaudeCodeBindOptionsDestinationTypestring literal union andDESTINATION_TYPESconstant arrayGroupedDestinationItemstype andBindableQuickPickItemunified item typeDEFAULT_TERMINAL_PICKER_MAX_INLINEsetting (configurable inline terminal limit)TERMINAL_PICKER_MORE_TERMINALS_DESCRIPTIONi18n messageDestinationRegistryto use grouped return typecreateMockBindableQuickPickItem.ts) for QuickPick item creationTest Plan
DestinationPickerCommand- no destinations, cancellation, bindable selection, terminal-more flow, loggingbuildDestinationQuickPickItems()- sequence ordering, label building, terminal-more handlingDestinationAvailabilityService.getGroupedDestinationItems()- grouping logic, terminal overflowDestinationRegistrygrouped APIcreateMockBindableQuickPickItem.tsRelated
Summary by CodeRabbit