[issues/255] PR4.5: Type→Kind Naming Consistency + Test Type Patterns#309
[issues/255] PR4.5: Type→Kind Naming Consistency + Test Type Patterns#309
Conversation
Comprehensive naming cleanup that aligns type names with the `kind` property convention used throughout the codebase, plus improves test type safety by replacing `jest.Mocked<T>` with `ReturnType<typeof createMockT>` patterns. This is a code quality improvement with no user-facing changes. - Renamed `DestinationType` → `DestinationKind` across all source and test files - Renamed `AIAssistantDestinationType` → `AIAssistantDestinationKind` - Renamed `DESTINATION_TYPES` → `DESTINATION_KINDS` constant - Renamed `AI_ASSISTANT_TYPES` → `AI_ASSISTANT_KINDS` constant - Renamed method `getSupportedTypes()` → `getSupportedKinds()` in DestinationRegistry - Renamed function `isPasteDestinationType()` → `isPasteDestinationKind()` in type guards - Renamed option `destinationTypes` → `destinationKinds` in GetAvailableDestinationItemsOptions - Renamed property `destinationType` → `destinationKind` in QuickPickItems and logging contexts - File renamed: `DestinationType.ts` → `DestinationKind.ts` - Replaced `jest.Mocked<DestinationAvailabilityService>` with `ReturnType<typeof createMockDestinationAvailabilityService>` - Replaced `jest.Mocked<PasteDestination>` with appropriate `ReturnType<typeof createMock*Destination>` patterns - Removed now-unused type imports that were only needed for jest.Mocked declarations - Applied pattern consistently across BindToTerminalCommand.test.ts, RangeLinkStatusBar.test.ts, and PasteDestinationManager.test.ts - Extracted `WithDestinationKind` interface to `types/WithDestinationKind.ts` - Extracted `WithDisplayName` interface to `types/WithDisplayName.ts` - `BindOptions.ts` now extends `WithDestinationKind` instead of local `BaseBindOptions` - `AvailableDestination` now composes `WithDestinationKind` and `WithDisplayName` - `QuickPickTypes.ts` imports and uses `WithDisplayName` from the shared location - Added barrel exports for both new interfaces in `types/index.ts` - Removed re-export of `DestinationKind` and `AIAssistantDestinationKind` from `PasteDestination.ts` - Updated all imports to use destination kind types from `types/` barrel instead of direct file imports - Enforces clean separation: types defined in `types/`, interfaces defined where used - Standardized all imports to use barrel exports instead of direct file imports - Source files: Commands, StatusBar, BookmarkService, navigation handlers, utils - Test files: All test helpers and test files in __tests__/ - Types imported from `../types` or `../../types` (not `../types/FileName.ts`) - Destinations imported from `../destinations` or `../../destinations` - i18n imports consolidated through `../i18n` barrel - errors imports consolidated through `../errors` barrel - [x] All existing 1396 tests pass - [x] No new tests added (refactoring only) - [x] Verified no remaining `DestinationType` references in codebase - [x] Verified `jest.Mocked<T>` replaced with factory return types where createMock* helpers exist - [x] Verified no re-export violations (import then export same symbol) - [x] Verified all imports use barrel exports (no direct file imports) - Part of #255 (Terminal Picker feature stack) - Follows PR4: DestinationPickerCommand and Grouped API
WalkthroughThis pull request refactors destination terminology and import structure throughout the VSCode extension. It renames "type" to "kind" across destination-related types and methods, consolidates imports using barrel exports, introduces new composition-based type interfaces, and moves type definitions to a centralized location. Changes
Sequence Diagram(s)The conditions for generating sequence diagrams are not met. While the changes involve multiple components and import restructuring, they consist primarily of terminology renaming, import consolidation, and type system updates without introducing new feature flows or significantly altering control flow sequences. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/rangelink-vscode-extension/src/__tests__/helpers/createMockDestinationRegistry.ts (1)
90-95:⚠️ Potential issue | 🟠 Major
getSupportedTypesshould begetSupportedKindsto match the interface.The
DestinationRegistryclass defines the method asgetSupportedKinds()(line 165 of DestinationRegistry.ts), but this mock still uses the old namegetSupportedTypes. Any test callingmockRegistry.getSupportedKinds()will getundefinedinstead of the mocked function.Fix
return { register: jest.fn(), create: jest.fn().mockImplementation(createImpl), - getSupportedTypes: jest.fn().mockReturnValue([]), + getSupportedKinds: jest.fn().mockReturnValue([]), getDisplayNames: jest.fn().mockReturnValue(DEFAULT_DISPLAY_NAMES), } as unknown as jest.Mocked<DestinationRegistry>;packages/rangelink-vscode-extension/src/utils/__tests__/destinationKindGuards.test.ts (1)
21-41:⚠️ Potential issue | 🟡 MinorTest descriptions still reference "type" instead of "kind".
Three
itdescriptions were not updated to match the rename:
- Line 21:
"does not match type"→"does not match kind"- Line 27:
"matches type"→"matches kind"- Line 33:
"all destination types"→"all destination kinds"Suggested fix
- it('should return false when destination.id does not match type', () => { + it('should return false when destination.id does not match kind', () => {- it('should return true when destination.id matches type', () => { + it('should return true when destination.id matches kind', () => {- it('should work with all destination types', () => { + it('should work with all destination kinds', () => {packages/rangelink-vscode-extension/src/destinations/utils/buildDestinationQuickPickItems.ts (1)
68-85:⚠️ Potential issue | 🟡 MinorUpdate error message strings to use "kind" terminology for consistency.
The error messages at lines 71 and 81 still reference "Invalid destination type" while the rest of this file has been migrated to "kind" terminology (isDestinationKind, DESTINATION_KINDS, DestinationKind). Update the message strings to match:
- Line 71:
"Invalid destination kind in picker sequence: ${key}"- Line 81:
"Invalid destination kind in bindOptions"Suggested message string updates
if (!isDestinationKind(key)) { throw new RangeLinkExtensionError({ code: RangeLinkExtensionErrorCodes.UNEXPECTED_DESTINATION_TYPE, - message: `Invalid destination type in picker sequence: ${key}`, + message: `Invalid destination kind in picker sequence: ${key}`, functionName: 'buildDestinationQuickPickItems', details: { key }, }); } for (const item of groupItems as BindableQuickPickItem[]) { if (!isDestinationKind(item?.bindOptions?.kind)) { throw new RangeLinkExtensionError({ code: RangeLinkExtensionErrorCodes.UNEXPECTED_DESTINATION_TYPE, - message: `Invalid destination type in bindOptions`, + message: `Invalid destination kind in bindOptions`, functionName: 'buildDestinationQuickPickItems', details: { item, key }, }); }packages/rangelink-vscode-extension/src/destinations/DestinationRegistry.ts (1)
84-88:⚠️ Potential issue | 🟡 MinorStale comment: still references "destination type" instead of "destination kind".
Line 85 says "Registry stores builders by destination type" but the rest of the file has been renamed to "kind".
Proposed fix
- * - Registry stores builders by destination type + * - Registry stores builders by destination kindpackages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.ts (3)
74-76:⚠️ Potential issue | 🟡 MinorStale JSDoc: "destination type" should be "destination kind".
Line 75 still reads "Bind to a destination type".
Proposed fix
- * Bind to a destination type + * Bind to a destination kind
920-927:⚠️ Potential issue | 🟡 MinorStale log key:
currentTypeshould becurrentKind.Line 924 uses
currentTypeas a logging context key, inconsistent with the rest of the rename to "kind".Proposed fix
fn: 'PasteDestinationManager.confirmReplaceBinding', - currentType: currentDestination.id, + currentKind: currentDestination.id, newKind,
596-606:⚠️ Potential issue | 🟡 MinorStale error message: "destination type" should be "destination kind" (lines 602, 871).
Two error messages still reference "type":
- Line 602:
Unhandled AI assistant destination type: ${kind}- Line 871/874:
Unknown destination type '${destination.id}'These appear in developer-facing error output and should be updated for consistency. Note that changing line 874's message will require updating the corresponding test assertion on line 1260.
Proposed fix
- message: `Unhandled AI assistant destination type: ${kind}`, + message: `Unhandled AI assistant destination kind: ${kind}`,- message: `Unknown destination type '${destination.id}' - missing case in buildPasteFailureMessage()`, + message: `Unknown destination kind '${destination.id}' - missing case in buildPasteFailureMessage()`,
🧹 Nitpick comments (6)
packages/rangelink-vscode-extension/src/commands/ManageBookmarksCommand.ts (1)
4-5: Nit:BookmarkandBookmarkServiceimports could be consolidated.These two type imports from
../bookmarkscould be merged into a single line, mirroring the consolidation pattern applied on line 7.-import type { Bookmark } from '../bookmarks'; -import type { BookmarkService } from '../bookmarks'; +import type { Bookmark, BookmarkService } from '../bookmarks';packages/rangelink-vscode-extension/src/__tests__/commands/DestinationPickerCommand.test.ts (1)
24-24: Consider aligningjest.Mocked<DestinationAvailabilityService>with theReturnType<typeof createMock...>pattern.The PR replaced
jest.Mocked<T>withReturnType<typeof createMockXxx>inBindToTerminalCommand.test.ts,RangeLinkStatusBar.test.ts, andPasteDestinationManager.test.ts, but this file still uses the old pattern on line 24. Applying the same change here would also make the type import on line 8 unnecessary.♻️ Suggested diff
-import type { DestinationAvailabilityService } from '../../destinations'; import type { TerminalPickerResult } from '../../destinations/utils';- let mockAvailabilityService: jest.Mocked<DestinationAvailabilityService>; + let mockAvailabilityService: ReturnType<typeof createMockDestinationAvailabilityService>;packages/rangelink-vscode-extension/src/types/DestinationKind.ts (1)
27-32: ConstrainAI_ASSISTANT_KINDStoDestinationKindvalues withsatisfiesfor compile-time safety.Currently
AI_ASSISTANT_KINDSis independently typed, so a typo (e.g.,'cursor-ais') would silently create an invalid kind. Add asatisfiesconstraint to catch such errors at compile time:♻️ Proposed refinement
-export const AI_ASSISTANT_KINDS = ['claude-code', 'cursor-ai', 'github-copilot-chat'] as const; +export const AI_ASSISTANT_KINDS = [ + 'claude-code', + 'cursor-ai', + 'github-copilot-chat', +] as const satisfies readonly DestinationKind[];packages/rangelink-vscode-extension/src/commands/DestinationPickerCommand.ts (1)
3-11: Consolidate error imports from barrel export.Both
RangeLinkExtensionErrorandRangeLinkExtensionErrorCodesare exported from the../errorsbarrel. Combine them into a single import statement for consistency:Import consolidation
-import { RangeLinkExtensionError } from '../errors'; -import { RangeLinkExtensionErrorCodes } from '../errors/RangeLinkExtensionErrorCodes'; +import { RangeLinkExtensionError, RangeLinkExtensionErrorCodes } from '../errors';packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationRegistry.test.ts (1)
43-43: Test descriptions still reference "types" instead of "kinds".Several test description strings still say "types" (e.g.,
'should allow registering multiple destination types','should overwrite previous builder when registering same type','should return registered types', etc.). Since this PR's objective is naming consistency, consider updating these to say "kinds" for alignment.Also applies to: 53-53, 169-169, 181-181, 202-202, 209-209
packages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.ts (1)
227-234: Inconsistency noted: error code name uses "TYPE" while messages reference "kind"—consider revisiting if a naming convention was established for this pattern.The error code
UNEXPECTED_DESTINATION_TYPEis consistently used across 8 locations for destination-level exhaustive checks (all with messages saying "destination kind"), while a separate error codeUNEXPECTED_ITEM_KINDexists for item-level checks. The enum defines both as distinct codes with noUNEXPECTED_DESTINATION_KINDvariant. If this naming distinction (TYPE vs KIND) reflects an intentional semantic separation, it may warrant a comment explaining the rationale; otherwise, consider aligning the code name to match the message terminology.
Addresses CodeRabbit review feedback on PR #309 by completing the rename from "type" to "kind" throughout the codebase. This includes mock method names, test descriptions, error messages, error codes, JSDoc comments, log keys, and import consolidations. Benefits: - Mock method `getSupportedTypes` now matches interface `getSupportedKinds` - Error code `UNEXPECTED_DESTINATION_TYPE` → `UNEXPECTED_DESTINATION_KIND` for consistency - All test descriptions now use "kind" terminology - Added `satisfies` constraint to AI_ASSISTANT_KINDS for compile-time type safety - Consolidated barrel imports in ManageBookmarksCommand and DestinationPickerCommand - Standardized test type patterns using `ReturnType<typeof createMock...>` Ref: #309 (review)
1 participant
Comprehensive naming cleanup that aligns type names with the
kindproperty convention used throughout the codebase, plus improves test type safety by replacingjest.Mocked<T>withReturnType<typeof createMockT>patterns. This is a code quality improvement with no user-facing changes.Renamed
DestinationType→DestinationKindacross all source and test filesRenamed
AIAssistantDestinationType→AIAssistantDestinationKindRenamed
DESTINATION_TYPES→DESTINATION_KINDSconstantRenamed
AI_ASSISTANT_TYPES→AI_ASSISTANT_KINDSconstantRenamed method
getSupportedTypes()→getSupportedKinds()in DestinationRegistryRenamed function
isPasteDestinationType()→isPasteDestinationKind()in type guardsRenamed option
destinationTypes→destinationKindsin GetAvailableDestinationItemsOptionsRenamed property
destinationType→destinationKindin QuickPickItems and logging contextsFile renamed:
DestinationType.ts→DestinationKind.tsReplaced
jest.Mocked<DestinationAvailabilityService>withReturnType<typeof createMockDestinationAvailabilityService>Replaced
jest.Mocked<PasteDestination>with appropriateReturnType<typeof createMock*Destination>patternsRemoved now-unused type imports that were only needed for jest.Mocked declarations
Applied pattern consistently across BindToTerminalCommand.test.ts, RangeLinkStatusBar.test.ts, and PasteDestinationManager.test.ts
Extracted
WithDestinationKindinterface totypes/WithDestinationKind.tsExtracted
WithDisplayNameinterface totypes/WithDisplayName.tsBindOptions.tsnow extendsWithDestinationKindinstead of localBaseBindOptionsAvailableDestinationnow composesWithDestinationKindandWithDisplayNameQuickPickTypes.tsimports and usesWithDisplayNamefrom the shared locationAdded barrel exports for both new interfaces in
types/index.tsRemoved re-export of
DestinationKindandAIAssistantDestinationKindfromPasteDestination.tsUpdated all imports to use destination kind types from
types/barrel instead of direct file importsEnforces clean separation: types defined in
types/, interfaces defined where usedStandardized all imports to use barrel exports instead of direct file imports
Source files: Commands, StatusBar, BookmarkService, navigation handlers, utils
Test files: All test helpers and test files in tests/
Types imported from
../typesor../../types(not../types/FileName.ts)Destinations imported from
../destinationsor../../destinationsi18n imports consolidated through
../i18nbarrelerrors imports consolidated through
../errorsbarrelAll existing 1396 tests pass
No new tests added (refactoring only)
Verified no remaining
DestinationTypereferences in codebaseVerified
jest.Mocked<T>replaced with factory return types where createMock* helpers existVerified no re-export violations (import then export same symbol)
Verified all imports use barrel exports (no direct file imports)
Part of Terminal Binding - Terminal Picker #255 (Terminal Picker feature stack)
Summary by CodeRabbit