[issues/355] File picker — per-file items in destination picker#369
Merged
[issues/355] File picker — per-file items in destination picker#369
Conversation
…Options workaround TextEditorBindOptions gains an optional editor field. When provided (e.g., from file picker selection), bindTextEditor() uses it directly. When omitted (e.g., from keybinding commands), it falls back to activeTextEditor — preserving backward compatibility. This eliminates the CreateOptions type workaround in DestinationRegistry (TODO #245) since BindOptions now covers both shapes. DestinationBuilder and create() use BindOptions directly. Benefits: - Single BindOptions type replaces the temporary CreateOptions union - File picker can pass a resolved editor; commands can omit it - buildTextEditorDestination guards against missing editor defensively
…types Introduces file-specific QuickPick types parallel to the terminal picker pattern: - FileBindableQuickPickItem carries fileInfo: EligibleFile alongside bind options - FileMoreQuickPickItem for "More files..." overflow (itemKind: 'file-more') Updates GroupedDestinationItems to use FileBindableQuickPickItem[] for the text-editor bucket and adds 'file-more' key. Adds 'file-more' to PICKER_ITEM_KINDS and handles the new item kind in DestinationPicker and RangeLinkStatusBar exhaustive switches (placeholder for #356 secondary picker). Temporary type casts in DestinationAvailabilityService and buildDestinationQuickPickItems tests bridge the old plain-item format until S004/S005 rewrite them with proper file items.
…iptions Builds description strings for file picker items combining disambiguator and bound/active badges separated by " · ". Tab group labels are rendered as QuickPick separators (not part of the item description), parallel to how destination groups use separators. Returns undefined when there's nothing to show (no disambiguator, no badges). Format examples: "…/components", "bound · active", "./ · bound · active"
Replaces the single text-editor item in DestinationAvailabilityService with the full file discovery pipeline: getEligibleFiles → markBoundFile → sortEligibleFiles → disambiguateFilenames → buildGroupedFileItems. Each eligible file becomes a FileBindableQuickPickItem with filename as label, disambiguator + badges as description, and EligibleFile domain info. Inline items are files with isCurrentInGroup=true (one per tab group). Non-current files appear behind "More files..." — no configurable threshold needed since tab group count is the natural limit. TextEditorBindOptions now requires uri + viewColumn (editor field removed). Every caller explicitly identifies the target file and tab group — no implicit activeTextEditor fallback. resolveEditor() uses findVisibleEditorsByUri + viewColumn match as the single resolution path. The builder (buildTextEditorDestination) also resolves via the same path. Enriches EligibleFile with displayPath (workspace-relative or absolute for outside-workspace files) and viewColumn (from TabGroup). disambiguateFilenames() takes EligibleFile[] directly, eliminating the separate FileEntry interface. Extracts BindToTextEditorCommand (parallel to BindToTerminalCommand) with two modes: - With URI (explorer context menu): detects multi-group, prompts if ambiguous, focuses existing tab when single match (explicit viewColumn), opens in active group when not found - Without URI (keybinding/editor context menu): 0/1/2+ file picker flow Consolidates BindToTerminalCommand constructor — removes redundant quickPickProvider param since VscodeAdapter already implements QuickPickProvider. Adds getFileItems() convenience method parallel to getTerminalItems(). Removes isTextEditorDestinationEligible dependency, filePicker.maxTabGroups setting, and related constants.
The destination picker rendered a single generic "Text Editor" item regardless of how many files were open. Now it renders individual file items with pre-built descriptions (bound/active badges, disambiguators) and a "More files..." overflow item -- parallel to the existing terminal/terminal-more pattern. Benefits: - Users see actual filenames in the destination picker instead of a generic label - "More files..." shows remaining count, matching terminal overflow UX - File descriptions (badges, disambiguators) flow through from the availability service
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors editor bindings from editor objects to uri+viewColumn; removes the rangelink.filePicker.maxTabGroups setting and default; adds per-file picker items with a "file-more" overflow and supporting types/utilities; consolidates BindToTerminalCommand dependencies; introduces BindToTextEditorCommand; updates focus resolution to use bound view columns; adapts many tests and helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BindCmd as BindToTextEditorCommand
participant Vscode as VscodeAdapter
participant Avail as DestinationAvailabilityService
participant Picker as DestinationPicker
participant Manager as PasteDestinationManager
User->>BindCmd: execute(explorerUri?)
alt explorerUri provided
BindCmd->>Vscode: locate tab groups for uri
Vscode-->>BindCmd: viewColumns[]
alt 0 or 1 match
BindCmd->>Vscode: showTextDocument(uri, viewColumn?)
BindCmd->>Manager: bind({ kind:'text-editor', uri, viewColumn })
else multiple matches
BindCmd->>Picker: show viewColumn quickpick
Picker-->>BindCmd: selected viewColumn
BindCmd->>Manager: bind({ kind:'text-editor', uri, viewColumn })
end
else picker mode
BindCmd->>Avail: getFileItems()
Avail-->>BindCmd: FileBindableQuickPickItem[]
alt 0 files
BindCmd->>Vscode: showErrorMessage()
BindCmd-->>User: { outcome: 'no-resource' }
else 1 file
BindCmd->>Manager: bind({ kind:'text-editor', uri, viewColumn })
else multiple files
BindCmd->>Picker: show file quickpick (includes file-more)
Picker-->>BindCmd: selected file
BindCmd->>Manager: bind({ kind:'text-editor', uri, viewColumn })
end
end
Manager-->>BindCmd: bind result
BindCmd-->>User: QuickPickBindResult
sequenceDiagram
participant Availability as DestinationAvailabilityService
participant Eligible as getEligibleFiles
participant Mark as markBoundFile
participant Sort as sortEligibleFiles
participant Disamb as disambiguateFilenames
participant Builder as buildGroupedFileItems
Availability->>Eligible: getEligibleFiles()
Eligible-->>Availability: EligibleFile[]
Availability->>Mark: markBoundFile(files, boundUri?)
Mark-->>Availability: files(with boundState)
Availability->>Sort: sortEligibleFiles(files)
Sort-->>Availability: sortedFiles
Availability->>Disamb: disambiguateFilenames(sortedFiles)
Disamb-->>Availability: disambiguators[]
Availability->>Builder: buildGroupedFileItems(sortedFiles, disambiguators)
Builder-->>Availability: FileBindableQuickPickItem[], maybe FileMoreQuickPickItem
Availability-->>Caller: grouped items (text-editor group includes file items)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
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__/commands/BindToTerminalCommand.test.ts (1)
14-14:⚠️ Potential issue | 🔴 CriticalRemove the unused
mockQuickPickProvidervariable — this causes the ESLintno-unused-varsCI failure.The variable is declared (line 26) and assigned in
beforeEach(line 34) but never referenced in any test after theBindToTerminalCommandconstructor was simplified to four parameters.🔧 Proposed fix
import { createMockDestinationAvailabilityService, createMockDestinationManager, createMockEligibleTerminal, - createMockQuickPickProvider, createMockTerminal, createMockTerminalQuickPickItem, createMockVscodeAdapter, spyOnShowTerminalPicker, } from '../helpers';let mockAdapter: ReturnType<typeof createMockVscodeAdapter>; - let mockQuickPickProvider: ReturnType<typeof createMockQuickPickProvider>; let command: BindToTerminalCommand;beforeEach(() => { mockLogger = createMockLogger(); mockDestinationManager = createMockDestinationManager(); mockAvailabilityService = createMockDestinationAvailabilityService(); - mockQuickPickProvider = createMockQuickPickProvider(); showTerminalPickerSpy = spyOnShowTerminalPicker(); });Also applies to: 26-26, 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.ts` at line 14, Remove the unused mockQuickPickProvider from the test: delete the variable declaration and its assignment in the beforeEach, and remove the unused import createMockQuickPickProvider so ESLint no-unused-vars stops failing; check references around the BindToTerminalCommand constructor usage to ensure no other tests rely on mockQuickPickProvider before committing.
🧹 Nitpick comments (11)
packages/rangelink-vscode-extension/src/statusBar/RangeLinkStatusBar.ts (1)
120-123: Stub forfile-moreis appropriate for now.The case ensures exhaustive switch coverage and the TODO correctly references the follow-up issue (
#356). Consider logging atwarnorinfolevel so it's easier to notice if users hit this path before#356lands, thoughdebugis also fine if this isn't expected to be reachable yet.Do you want me to open an issue or remind about implementing the secondary file picker from the status bar in
#356?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/statusBar/RangeLinkStatusBar.ts` around lines 120 - 123, The 'file-more' switch branch in RangeLinkStatusBar.ts currently logs at debug level; update the logger call in the case 'file-more' branch within the RangeLinkStatusBar class (the handler that contains "File more item selected") to use a more visible level (info or warn) so hits are noticeable before issue `#356` is implemented, retain the TODO comment referencing `#356`, and keep the exhaustive switch coverage intact.packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts (2)
109-114: Minor inconsistency: inlinevscode.Uriobject instead ofcreateMockUrihelper.All other untitled URI tests in the suite (e.g.,
compareEditorsByUri.test.ts) usecreateMockUri('untitled:Untitled-1', { scheme: 'untitled' }). The inline object here also omits standardUriproperties (authority,fragment,query), which could silently break ifbuildTextEditorDestinationor its dependencies access them.♻️ Suggested replacement
- const mockUri = { - scheme: 'untitled', - fsPath: 'Untitled-1', - path: 'Untitled-1', - toString: () => 'untitled:Untitled-1', - } as vscode.Uri; + const mockUri = createMockUri('untitled:Untitled-1', { scheme: 'untitled' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts` around lines 109 - 114, Replace the inline mock Uri object in the test with the shared helper to ensure all standard Uri fields are present: use createMockUri('untitled:Untitled-1', { scheme: 'untitled' }) instead of the ad-hoc object so tests that call buildTextEditorDestination or any Uri-dependent utilities get a full mock (including authority, fragment, query, etc.); update the reference in destinationBuilders.test.ts to use createMockUri to match other tests like compareEditorsByUri.test.ts.
67-143: Missing logger assertions inbuildTextEditorDestinationtests.The three happy-path
buildTextEditorDestinationtests verifyid/displayNamebut never assert onmockLoggercalls. As per coding guidelines, logging behavior should be consolidated into the same behavior tests rather than omitted entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts` around lines 67 - 143, The tests for buildTextEditorDestination are missing assertions that the mockLogger was called; update each happy-path test (the ones creating destination for workspace file, external file, and untitled file) to assert that the provided mockLogger (mockLogger on the createMockContext result) received the expected logging call when buildTextEditorDestination(...) runs, e.g. verify the logger method used (debug/info) was called once with a message/context that includes the resulting destination.displayName or the source uri; modify the three tests to include these mockLogger assertions after constructing destination so logging behavior is validated alongside id/displayName checks.packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationPicker.test.ts (1)
148-226: Missing test coverage for the newfile-morecase.The
terminal-morehandling has two tests (selected → secondary picker; cancelled → returns to main picker) but there's no equivalent test forfile-more, which was added in this PR and currently stubs back to'cancelled'. Per coding guidelines, all new features should have corresponding tests.♻️ Suggested test to add alongside the terminal-more describe block
+ describe('file-more item selection', () => { + it('returns cancelled when user selects "More files..."', async () => { + const fileMoreItem = createMockFileMoreQuickPickItem(3); + mockAvailabilityService.getGroupedDestinationItems.mockResolvedValue({ + 'file-more': fileMoreItem, + }); + showQuickPickMock.mockResolvedValue(fileMoreItem); + + const result = await picker.pick(defaultOptions); + + expect(result).toStrictEqual({ outcome: 'cancelled' }); + expect(mockLogger.debug).toHaveBeenCalledWith( + { fn: 'DestinationPicker.handleQuickPickSelection' }, + 'User selected "More files...", showing secondary picker', + ); + }); + });Based on learnings: "Add tests for all new features and bug fixes with descriptive test names that explain the scenario."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationPicker.test.ts` around lines 148 - 226, Add two tests mirroring the terminal-more cases for the new "file-more" path: (1) "shows secondary file picker when user selects 'More files...'" — stub mockAvailabilityService.getGroupedDestinationItems to return { file: [createMockFileQuickPickItem(file1)], 'file-more': moreItem }, set showQuickPickMock to resolve moreItem, mockAvailabilityService.getFileItems to return file items, and implement showFilePickerSpy to invoke handlers.onSelected({ file: file2, name: 'File 2' }) then assert showFilePickerSpy was called and picker.pick returned outcome 'selected' with bindOptions { kind: 'file', file: file2 }; (2) "returns to main picker when user cancels secondary file picker" — similar setup but have showFilePickerSpy call handlers.onDismissed?.(), have showQuickPickMock return moreItem first then a file item, assert showQuickPickMock called twice, mockLogger.debug called with { fn: 'DestinationPicker.showSecondaryFilePicker' } and { fn: 'DestinationPicker.pick' } messages, and picker.pick returns the original selected file bindOptions { kind: 'file', file }.packages/rangelink-vscode-extension/src/ide/vscode/VscodeAdapter.ts (1)
700-708: Optional: add@param/@returnsto JSDoc for consistency with neighbouring methods.Every other method in this section (e.g.
findVisibleEditorsByUri) documents its parameters and return value.hasVisibleEditorAthas only a single-line description.📝 Proposed JSDoc expansion
- /** - * Check if a visible editor exists at the given URI and viewColumn. - */ + /** + * Check if a visible editor exists at the given URI and viewColumn. + * + * `@param` uri - Document URI to match against + * `@param` viewColumn - Editor column number to match against + * `@returns` True if a matching visible editor is found + */ hasVisibleEditorAt(uri: vscode.Uri, viewColumn: number): boolean {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/ide/vscode/VscodeAdapter.ts` around lines 700 - 708, Add JSDoc tags to the hasVisibleEditorAt method to match neighboring methods: document the uri parameter with `@param` {vscode.Uri} uri - the resource URI to check, the viewColumn parameter with `@param` {number} viewColumn - the view column to match, and add an `@returns` {boolean} describing that the method returns true if a visible text editor exists at the given URI and viewColumn; update the block above the hasVisibleEditorAt function (which queries this.visibleTextEditors and uses editor.document.uri.toString() and editor.viewColumn) to include these tags for consistency with findVisibleEditorsByUri.packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.ts (1)
263-275: Consider extracting a named constant for the repeatedviewColumn: 1literal.The literal
1(VSCodeViewColumn.One) appears six times across the editor test cases. Per coding guidelines, numeric literals with semantic meaning should use SCREAMING_SNAKE_CASE named constants.♻️ Proposed refactor
Add a test-scoped constant near the top of the
describe('Editor-like destination …')block:+ const TEST_VIEW_COLUMN = 1; // vscode.ViewColumn.One + it('should complete end-to-end paste flow with EditorFocusCapability', async () => { ... const focusCapability = new EditorFocusCapability( mockAdapter, mockUri, - 1, + TEST_VIEW_COLUMN, insertFactory, mockLogger, ); ... const destination = ComposablePasteDestination.createForTesting({ ... - resource: { kind: 'editor', uri: mockUri, viewColumn: 1 }, + resource: { kind: 'editor', uri: mockUri, viewColumn: TEST_VIEW_COLUMN },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.ts` around lines 263 - 275, Extract the repeated numeric literal for the editor view column into a test-scoped SCREAMING_SNAKE_CASE constant (e.g., VIEW_COLUMN_ONE = 1) and replace all occurrences of the literal `1` used as `viewColumn: 1` in this test suite; update places where EditorFocusCapability is constructed and where ComposablePasteDestination.createForTesting({ resource: { ..., viewColumn: 1 } }) (and the other editor test cases that also use viewColumn) to reference the new constant instead of the raw `1`.packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts (2)
578-580: Both tab groups default toviewColumn: 1— consider using distinct view columns for realism.
createMockTabGroupdefaults toviewColumn: 1, so bothgroup1andgroup2produceEligibleFileobjects withviewColumn: 1. While the test outcome is unaffected (the assertion only checks count and absence offile-more), this is unrealistic mock data since VS Code assigns distinctviewColumnvalues per tab group.♻️ Suggested fix
- const group1 = createMockTabGroup([tab1]); - const group2 = createMockTabGroup([tab2]); + const group1 = createMockTabGroup([tab1], { viewColumn: 1 }); + const group2 = createMockTabGroup([tab2], { viewColumn: 2 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts` around lines 578 - 580, The two mock tab groups are created with the same default viewColumn, making both EligibleFile objects have viewColumn: 1; update the test setup so createMockTabGroup is called for group1 and group2 with distinct viewColumn values (e.g., pass viewColumn: 1 for group1 and viewColumn: 2 for group2) so the resulting EligibleFile objects reflect realistic, different VS Code view columns; modify the calls that construct group1 and group2 (references: createMockTabGroup, group1, group2, EligibleFile) accordingly.
463-599: Missing logger assertions in the new file-item tests.None of the five new "file items via text-editor kind" tests assert the
debugcalls emitted bygetGroupedDestinationItems(the "Using provided destinationKinds filter" and "Built grouped destination items" log lines). The guideline requires logger assertions to be consolidated into behavior tests.As per coding guidelines: "Always test logging behavior by including logger assertions in tests that verify method behavior — consolidate with behavior tests rather than creating separate logging tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts` around lines 463 - 599, Add assertions for the logger in each of the five "file items via text-editor kind" tests so they also verify debug logs emitted by DestinationAvailabilityService.getGroupedDestinationItems; specifically assert that mockLogger.debug was called with (or matching) the "Using provided destinationKinds filter" message when a destinationKinds filter is passed and the "Built grouped destination items" message after results are built. Locate the tests that create the service (new DestinationAvailabilityService(...)) and, after calling getGroupedDestinationItems, add expectations against mockLogger.debug (e.g., toHaveBeenCalledWith or toHaveBeenCalledTimes + matching args) to consolidate logging verification into these behavior tests.packages/rangelink-vscode-extension/src/destinations/utils/buildDestinationQuickPickItems.ts (1)
143-147: File item descriptions are pre-built; consider a brief inline comment explaining why.Terminal descriptions are built here via
buildTerminalDescription, but file descriptions useitem.descriptiondirectly (pre-built bybuildFileDescriptionupstream). The asymmetry is intentional — file descriptions include disambiguators and badges that are computed during the file discovery pipeline — but a brief "why" comment would help future readers understand this design choice.📝 Optional: Add a brief "why" comment
const description = isTerminalItem(item) ? buildTerminalDescription(item.terminalInfo) : isFileItem(item) - ? item.description + ? item.description // Pre-built by buildFileDescription (includes disambiguators + badges) : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/destinations/utils/buildDestinationQuickPickItems.ts` around lines 143 - 147, The branching in buildDestinationQuickPickItems treats terminal items by calling buildTerminalDescription(item.terminalInfo) but uses item.description directly for file items because file descriptions are pre-built upstream by buildFileDescription (they include disambiguators and badges); add a brief inline comment next to that ternary branch explaining this asymmetry and referencing isTerminalItem, isFileItem, buildTerminalDescription, and buildFileDescription so future readers understand why file descriptions are not recomputed here.packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (1)
1496-1533: Inline URI mocks withtoString()are fine for these tests, but consider reusingcreateMockUri.Lines 1498–1502 and 1536–1540 construct manual URI-like objects with
toString(), while the test file already imports and usescreateMockUrielsewhere (e.g., Lines 528, 572). Using the helper consistently would reduce duplication and ensure any future URI shape changes are centralized.♻️ Use createMockUri for consistency
- const mockUri = { - scheme: 'file', - fsPath: '/test/file.ts', - toString: () => 'file:///test/file.ts', - } as vscode.Uri; - const mockDocument = { uri: mockUri } as vscode.TextDocument; - const mockEditor = { document: mockDocument, viewColumn: 1 } as vscode.TextEditor; + const mockUri = createMockUri('/test/file.ts'); + const mockDocument = createMockDocument({ uri: mockUri }); + const mockEditor = createMockEditor({ document: mockDocument, viewColumn: 1 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts` around lines 1496 - 1533, Replace the inline manual URI mocks with the shared helper createMockUri to reduce duplication and centralize URI shape changes: find tests using the manual mockUri objects (the one used with documentCloseListener and the bind call in the "Text editor closure auto-unbind" spec) and replace their construction with createMockUri(...); ensure the resulting mockUri is used for mockDocument, mockEditor and any other assertions that reference the URI string so existing expectations on status bar messages and manager.bind/documentCloseListener behavior remain unchanged.packages/rangelink-vscode-extension/src/commands/BindToTextEditorCommand.ts (1)
148-153:mapBindResultis duplicated withBindToTerminalCommand.This 4-line method is identical in both command classes. Could be extracted to a shared utility, but given the small size and that both classes are command handlers with the same result type, this is a low-priority improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/commands/BindToTextEditorCommand.ts` around lines 148 - 153, mapBindResult is duplicated across BindToTextEditorCommand and BindToTerminalCommand; extract the logic into a shared helper function (e.g., export function mapBindResult<T extends BindSuccessInfo>(bindResult: ExtensionResult<T>): QuickPickBindResult) in a small utilities module and import it in both command classes, replacing the existing private method bodies with calls to the shared mapBindResult helper; ensure the helper types reference ExtensionResult, BindSuccessInfo and return QuickPickBindResult so existing callers compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts`:
- Around line 484-492: The assertions in DestinationAvailabilityService.test.ts
use partial checks for the 'text-editor' result (e.g.,
expect(result['text-editor'])[...].label/bindOptions/itemKind/fileInfo.filename)
instead of asserting the full item shape; update the "returns file items when
eligible files exist", "applies bound state", and "shows file-more" tests to
replace these partial expects with a single
expect(result['text-editor'])[0].toStrictEqual(...) that contains the complete
expected object (include displayName, description, bindOptions, itemKind,
boundState, and the full fileInfo shape such as filename, path, and any other
properties asserted elsewhere) so the test verifies the entire item structure
exactly.
In
`@packages/rangelink-vscode-extension/src/__tests__/helpers/createMockBindableQuickPickItem.ts`:
- Around line 11-13: Remove the blank line between the consecutive local imports
in createMockBindableQuickPickItem.ts so the imports are adjacent; specifically,
adjust the import statements for createMockEligibleFile and
createMockEligibleTerminal to be consecutive (no empty line) to satisfy ESLint
import/order rules in the module that defines createMockBindableQuickPickItem.
In `@packages/rangelink-vscode-extension/src/destinations/DestinationPicker.ts`:
- Around line 112-115: The debug message in the DestinationPicker handling for
case 'file-more' is misleading because the code returns { outcome: 'cancelled' }
instead of showing a secondary picker; update the log in the 'file-more' branch
(where logger.debug is called) to accurately reflect that the user selected
"More files..." but the picker is being cancelled/closed (e.g., "User selected
'More files...' — cancelling destination picker"), so logs match the actual
behavior of the case 'file-more' return; ensure you update the log string used
by logger.debug in DestinationPicker to avoid implying a secondary picker will
appear.
---
Outside diff comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.ts`:
- Line 14: Remove the unused mockQuickPickProvider from the test: delete the
variable declaration and its assignment in the beforeEach, and remove the unused
import createMockQuickPickProvider so ESLint no-unused-vars stops failing; check
references around the BindToTerminalCommand constructor usage to ensure no other
tests rely on mockQuickPickProvider before committing.
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.ts`:
- Around line 263-275: Extract the repeated numeric literal for the editor view
column into a test-scoped SCREAMING_SNAKE_CASE constant (e.g., VIEW_COLUMN_ONE =
1) and replace all occurrences of the literal `1` used as `viewColumn: 1` in
this test suite; update places where EditorFocusCapability is constructed and
where ComposablePasteDestination.createForTesting({ resource: { ..., viewColumn:
1 } }) (and the other editor test cases that also use viewColumn) to reference
the new constant instead of the raw `1`.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts`:
- Around line 578-580: The two mock tab groups are created with the same default
viewColumn, making both EligibleFile objects have viewColumn: 1; update the test
setup so createMockTabGroup is called for group1 and group2 with distinct
viewColumn values (e.g., pass viewColumn: 1 for group1 and viewColumn: 2 for
group2) so the resulting EligibleFile objects reflect realistic, different VS
Code view columns; modify the calls that construct group1 and group2
(references: createMockTabGroup, group1, group2, EligibleFile) accordingly.
- Around line 463-599: Add assertions for the logger in each of the five "file
items via text-editor kind" tests so they also verify debug logs emitted by
DestinationAvailabilityService.getGroupedDestinationItems; specifically assert
that mockLogger.debug was called with (or matching) the "Using provided
destinationKinds filter" message when a destinationKinds filter is passed and
the "Built grouped destination items" message after results are built. Locate
the tests that create the service (new DestinationAvailabilityService(...)) and,
after calling getGroupedDestinationItems, add expectations against
mockLogger.debug (e.g., toHaveBeenCalledWith or toHaveBeenCalledTimes + matching
args) to consolidate logging verification into these behavior tests.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts`:
- Around line 109-114: Replace the inline mock Uri object in the test with the
shared helper to ensure all standard Uri fields are present: use
createMockUri('untitled:Untitled-1', { scheme: 'untitled' }) instead of the
ad-hoc object so tests that call buildTextEditorDestination or any Uri-dependent
utilities get a full mock (including authority, fragment, query, etc.); update
the reference in destinationBuilders.test.ts to use createMockUri to match other
tests like compareEditorsByUri.test.ts.
- Around line 67-143: The tests for buildTextEditorDestination are missing
assertions that the mockLogger was called; update each happy-path test (the ones
creating destination for workspace file, external file, and untitled file) to
assert that the provided mockLogger (mockLogger on the createMockContext result)
received the expected logging call when buildTextEditorDestination(...) runs,
e.g. verify the logger method used (debug/info) was called once with a
message/context that includes the resulting destination.displayName or the
source uri; modify the three tests to include these mockLogger assertions after
constructing destination so logging behavior is validated alongside
id/displayName checks.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationPicker.test.ts`:
- Around line 148-226: Add two tests mirroring the terminal-more cases for the
new "file-more" path: (1) "shows secondary file picker when user selects 'More
files...'" — stub mockAvailabilityService.getGroupedDestinationItems to return {
file: [createMockFileQuickPickItem(file1)], 'file-more': moreItem }, set
showQuickPickMock to resolve moreItem, mockAvailabilityService.getFileItems to
return file items, and implement showFilePickerSpy to invoke
handlers.onSelected({ file: file2, name: 'File 2' }) then assert
showFilePickerSpy was called and picker.pick returned outcome 'selected' with
bindOptions { kind: 'file', file: file2 }; (2) "returns to main picker when user
cancels secondary file picker" — similar setup but have showFilePickerSpy call
handlers.onDismissed?.(), have showQuickPickMock return moreItem first then a
file item, assert showQuickPickMock called twice, mockLogger.debug called with {
fn: 'DestinationPicker.showSecondaryFilePicker' } and { fn:
'DestinationPicker.pick' } messages, and picker.pick returns the original
selected file bindOptions { kind: 'file', file }.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts`:
- Around line 1496-1533: Replace the inline manual URI mocks with the shared
helper createMockUri to reduce duplication and centralize URI shape changes:
find tests using the manual mockUri objects (the one used with
documentCloseListener and the bind call in the "Text editor closure auto-unbind"
spec) and replace their construction with createMockUri(...); ensure the
resulting mockUri is used for mockDocument, mockEditor and any other assertions
that reference the URI string so existing expectations on status bar messages
and manager.bind/documentCloseListener behavior remain unchanged.
In `@packages/rangelink-vscode-extension/src/commands/BindToTextEditorCommand.ts`:
- Around line 148-153: mapBindResult is duplicated across
BindToTextEditorCommand and BindToTerminalCommand; extract the logic into a
shared helper function (e.g., export function mapBindResult<T extends
BindSuccessInfo>(bindResult: ExtensionResult<T>): QuickPickBindResult) in a
small utilities module and import it in both command classes, replacing the
existing private method bodies with calls to the shared mapBindResult helper;
ensure the helper types reference ExtensionResult, BindSuccessInfo and return
QuickPickBindResult so existing callers compile.
In
`@packages/rangelink-vscode-extension/src/destinations/utils/buildDestinationQuickPickItems.ts`:
- Around line 143-147: The branching in buildDestinationQuickPickItems treats
terminal items by calling buildTerminalDescription(item.terminalInfo) but uses
item.description directly for file items because file descriptions are pre-built
upstream by buildFileDescription (they include disambiguators and badges); add a
brief inline comment next to that ternary branch explaining this asymmetry and
referencing isTerminalItem, isFileItem, buildTerminalDescription, and
buildFileDescription so future readers understand why file descriptions are not
recomputed here.
In `@packages/rangelink-vscode-extension/src/ide/vscode/VscodeAdapter.ts`:
- Around line 700-708: Add JSDoc tags to the hasVisibleEditorAt method to match
neighboring methods: document the uri parameter with `@param` {vscode.Uri} uri -
the resource URI to check, the viewColumn parameter with `@param` {number}
viewColumn - the view column to match, and add an `@returns` {boolean} describing
that the method returns true if a visible text editor exists at the given URI
and viewColumn; update the block above the hasVisibleEditorAt function (which
queries this.visibleTextEditors and uses editor.document.uri.toString() and
editor.viewColumn) to include these tags for consistency with
findVisibleEditorsByUri.
In `@packages/rangelink-vscode-extension/src/statusBar/RangeLinkStatusBar.ts`:
- Around line 120-123: The 'file-more' switch branch in RangeLinkStatusBar.ts
currently logs at debug level; update the logger call in the case 'file-more'
branch within the RangeLinkStatusBar class (the handler that contains "File more
item selected") to use a more visible level (info or warn) so hits are
noticeable before issue `#356` is implemented, retain the TODO comment referencing
`#356`, and keep the exhaustive switch coverage intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
packages/rangelink-vscode-extension/package.jsonpackages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/DestinationPicker.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/DestinationRegistry.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/EditorFocusCapability.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/equality/compareEditorsByUri.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/buildDestinationQuickPickItems.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/buildFileDescription.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/disambiguateFilenames.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/getEligibleFiles.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/isTextEditorDestinationEligible.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/markBoundFile.test.tspackages/rangelink-vscode-extension/src/__tests__/extension.test.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockBindableQuickPickItem.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockDestinationRegistry.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockEditorComposablePasteDestination.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockEligibleFile.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockTabGroup.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockWorkspace.tspackages/rangelink-vscode-extension/src/commands/BindToTerminalCommand.tspackages/rangelink-vscode-extension/src/commands/BindToTextEditorCommand.tspackages/rangelink-vscode-extension/src/commands/index.tspackages/rangelink-vscode-extension/src/constants/settingDefaults.tspackages/rangelink-vscode-extension/src/constants/settingKeys.tspackages/rangelink-vscode-extension/src/destinations/ComposablePasteDestination.tspackages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.tspackages/rangelink-vscode-extension/src/destinations/DestinationPicker.tspackages/rangelink-vscode-extension/src/destinations/DestinationRegistry.tspackages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.tspackages/rangelink-vscode-extension/src/destinations/capabilities/EditorFocusCapability.tspackages/rangelink-vscode-extension/src/destinations/capabilities/FocusCapability.tspackages/rangelink-vscode-extension/src/destinations/capabilities/FocusCapabilityFactory.tspackages/rangelink-vscode-extension/src/destinations/destinationBuilders.tspackages/rangelink-vscode-extension/src/destinations/equality/compareEditorsByUri.tspackages/rangelink-vscode-extension/src/destinations/utils/buildDestinationQuickPickItems.tspackages/rangelink-vscode-extension/src/destinations/utils/buildFileDescription.tspackages/rangelink-vscode-extension/src/destinations/utils/disambiguateFilenames.tspackages/rangelink-vscode-extension/src/destinations/utils/getEligibleFiles.tspackages/rangelink-vscode-extension/src/destinations/utils/index.tspackages/rangelink-vscode-extension/src/destinations/utils/isTextEditorDestinationEligible.tspackages/rangelink-vscode-extension/src/extension.tspackages/rangelink-vscode-extension/src/i18n/messages.en.tspackages/rangelink-vscode-extension/src/ide/vscode/VscodeAdapter.tspackages/rangelink-vscode-extension/src/statusBar/RangeLinkStatusBar.tspackages/rangelink-vscode-extension/src/types/BindOptions.tspackages/rangelink-vscode-extension/src/types/EligibleFile.tspackages/rangelink-vscode-extension/src/types/GroupedDestinationKinds.tspackages/rangelink-vscode-extension/src/types/MessageCode.tspackages/rangelink-vscode-extension/src/types/QuickPickTypes.tspackages/rangelink-vscode-extension/src/types/index.tspackages/rangelink-vscode-extension/src/utils/__tests__/destinationKindGuards.test.tspackages/rangelink-vscode-extension/src/utils/destinationKindGuards.ts
💤 Files with no reviewable changes (8)
- packages/rangelink-vscode-extension/src/destinations/capabilities/FocusCapability.ts
- packages/rangelink-vscode-extension/src/types/MessageCode.ts
- packages/rangelink-vscode-extension/package.json
- packages/rangelink-vscode-extension/src/i18n/messages.en.ts
- packages/rangelink-vscode-extension/src/constants/settingKeys.ts
- packages/rangelink-vscode-extension/src/destinations/utils/isTextEditorDestinationEligible.ts
- packages/rangelink-vscode-extension/src/tests/destinations/utils/isTextEditorDestinationEligible.test.ts
- packages/rangelink-vscode-extension/src/constants/settingDefaults.ts
...rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts
Outdated
Show resolved
Hide resolved
packages/rangelink-vscode-extension/src/__tests__/helpers/createMockBindableQuickPickItem.ts
Outdated
Show resolved
Hide resolved
packages/rangelink-vscode-extension/src/destinations/DestinationPicker.ts
Show resolved
Hide resolved
… log Addresses CodeRabbit review feedback on PR #369. Replaces partial property checks with full toStrictEqual assertions to catch regressions in the file item pipeline. Fixes a misleading log that implied a secondary picker was being shown when the file-more stub actually returns cancelled. Replaces inline URI casts with the shared createMockUri helper for consistency across test files. Benefits: - Full-object assertions catch unexpected property changes (T002/T004 compliance) - Logger assertions verify debug visibility in file-item tests (T007 compliance) - Distinct viewColumns in multi-group test produce realistic mock data - Consistent use of createMockUri across all destination test files Ignored Feedback: - Missing logger assertions in buildTextEditorDestination tests: builders don't emit logs — they delegate to factories that receive the logger - Missing file-more test in DestinationPicker: stub is a TODO for #356 — test will be rewritten with the real implementation - JSDoc expansion for hasVisibleEditorAt: method signature is self-documenting (C002) - Extract VIEW_COLUMN_ONE constant: vscode.ViewColumn.One is a well-known constant, not a magic number - Extract mapBindResult to shared utility: two 4-line private methods don't warrant premature abstraction - Add inline comment for file description asymmetry: code is self-documenting (C002) - Change file-more stub log level to warn/info: stub returns cancelled immediately — warn for expected placeholder behavior would pollute logs Ref: #369 (review)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts (2)
593-600: Missing the'Built grouped destination items'logger assertion.Every other test in this
describeblock verifies both debug calls (lines 510–516, 544–550, 651–657, 685–691), but this test only checks'Using provided destinationKinds filter'and omits the second call. Per coding guidelines: "Always test logging behavior by including logger assertions in tests that verify method behavior."♻️ Proposed fix
expect(mockLogger.debug).toHaveBeenCalledWith( { fn: 'DestinationAvailabilityService.getGroupedDestinationItems', destinationKinds: ['text-editor'], }, 'Using provided destinationKinds filter', ); + expect(mockLogger.debug).toHaveBeenCalledWith( + { + fn: 'DestinationAvailabilityService.getGroupedDestinationItems', + groupKeys: ['text-editor'], + }, + 'Built grouped destination items', + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts` around lines 593 - 600, Add the missing logger assertion: after the existing expectation for 'Using provided destinationKinds filter' in the test for DestinationAvailabilityService.getGroupedDestinationItems, add an assertion that mockLogger.debug was called with the context indicating fn: 'DestinationAvailabilityService.getGroupedDestinationItems' and the appropriate payload (e.g., grouped items/destinationKinds info) and the message 'Built grouped destination items' so the test verifies both debug calls like the other tests in this describe block.
697-724: Missing logger assertions in bothgetFileItems()tests.Neither
'returns empty array when no files exist'nor'delegates to getGroupedDestinationItems…'includes anymockLoggerassertions, despite the guideline: "Always test logging behavior by including logger assertions in tests that verify method behavior." IfgetFileItems()emits debug/info log entries, they should be verified here in the same way thegetGroupedDestinationItemscall-site behavior is verified in thegetTerminalItemsdelegation test pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts` around lines 697 - 724, Add logger assertions to both tests for DestinationAvailabilityService.getFileItems: after calling service.getFileItems() in the 'returns empty array when no files exist' test assert that mockLogger (the injected logger) was called appropriately (e.g., expect(mockLogger.debug/info).toHaveBeenCalled() or toHaveBeenCalledWith(expect.stringContaining(...)) to validate the log emitted when no files are found), and in the 'delegates to getGroupedDestinationItems…' test add an assertion that mockLogger recorded the delegation/log message (e.g., expect(mockLogger.debug/info).toHaveBeenCalled() or toHaveBeenCalledWith(expect.stringContaining('getGroupedDestinationItems') or similar) in addition to the existing spy on service.getGroupedDestinationItems; use the existing mockLogger instance used to construct DestinationAvailabilityService to locate where to add these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts`:
- Around line 683-684: Replace the partial assertion
expect(result['text-editor']).toHaveLength(2) with a full equality assertion
that checks the exact array structure for the two text-editor items; assert
expect(result['text-editor']).toStrictEqual([...]) using the exact objects
produced by the test setup (group1 with viewColumn 1 mapping to "a.ts" and
group2 with viewColumn 2 mapping to "b.ts") so the test verifies the full item
shapes rather than only the count.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts`:
- Line 582: Replace the enum constant in the test assertion with its string
literal value: update the expect call that references formatMessageSpy and
MessageCode.ERROR_TEXT_EDITOR_NOT_VISIBLE so it asserts with the actual string
(e.g., "ERROR_TEXT_EDITOR_NOT_VISIBLE") instead of the enum symbol; locate the
assertion using formatMessageSpy and MessageCode.ERROR_TEXT_EDITOR_NOT_VISIBLE
in PasteDestinationManager.test.ts and change the second argument of
toHaveBeenCalledWith to the corresponding string literal.
- Around line 146-155: The cache key for editor destinations uses only fsPath
which can collide for the same file opened in different view columns; update the
key generation in the branch where options.kind === 'text-editor' and
options.uri exists to include options.viewColumn (e.g., incorporate viewColumn
into cacheKey alongside fsPath) so destinationCache entries are unique per
editor column and createMockEditorComposablePasteDestination receives the
correct per-column instance.
- Around line 540-541: The tests repeatedly use the numeric literal 1 for
viewColumn (e.g., calls like manager.bind({ kind: 'text-editor', uri: mockUri,
viewColumn: 1 })); define a single SCREAMING_SNAKE_CASE constant (e.g.,
VIEW_COLUMN = 1) near the top of the test file and replace all literal uses (all
manager.bind and similar assertions/inputs listed in the comment) with that
constant to centralize the semantic value and improve maintainability.
- Around line 568-585: The tests exercising PasteDestinationManager.bind (which
calls PasteDestinationManager.bindTextEditor) are missing assertions that the
logger warned for each error path; update the three tests around the
failing-bind, read-only, and binary-file cases to assert that mockLogger.warn
was called with the expected message(s) (use mockLogger.warn and the same
message strings produced by bindTextEditor), e.g., add
expect(mockLogger.warn).toHaveBeenCalledWith(...) after the existing
expect(result)... and before/after the existing side-effect checks so each test
verifies the warning log alongside the error result and calls to
formatMessageSpy and window.showErrorMessage.
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts`:
- Around line 593-600: Add the missing logger assertion: after the existing
expectation for 'Using provided destinationKinds filter' in the test for
DestinationAvailabilityService.getGroupedDestinationItems, add an assertion that
mockLogger.debug was called with the context indicating fn:
'DestinationAvailabilityService.getGroupedDestinationItems' and the appropriate
payload (e.g., grouped items/destinationKinds info) and the message 'Built
grouped destination items' so the test verifies both debug calls like the other
tests in this describe block.
- Around line 697-724: Add logger assertions to both tests for
DestinationAvailabilityService.getFileItems: after calling
service.getFileItems() in the 'returns empty array when no files exist' test
assert that mockLogger (the injected logger) was called appropriately (e.g.,
expect(mockLogger.debug/info).toHaveBeenCalled() or
toHaveBeenCalledWith(expect.stringContaining(...)) to validate the log emitted
when no files are found), and in the 'delegates to getGroupedDestinationItems…'
test add an assertion that mockLogger recorded the delegation/log message (e.g.,
expect(mockLogger.debug/info).toHaveBeenCalled() or
toHaveBeenCalledWith(expect.stringContaining('getGroupedDestinationItems') or
similar) in addition to the existing spy on service.getGroupedDestinationItems;
use the existing mockLogger instance used to construct
DestinationAvailabilityService to locate where to add these assertions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tspackages/rangelink-vscode-extension/src/destinations/DestinationPicker.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rangelink-vscode-extension/src/destinations/DestinationPicker.ts
- packages/rangelink-vscode-extension/src/tests/destinations/destinationBuilders.test.ts
...rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+146
to
+155
| if (options.kind === 'text-editor' && options.uri) { | ||
| const fsPath = options.uri.fsPath; | ||
| const cacheKey = `text-editor:${fsPath}`; | ||
| if (!destinationCache.has(cacheKey)) { | ||
| destinationCache.set( | ||
| cacheKey, | ||
| createMockEditorComposablePasteDestination({ editor: options.editor }), | ||
| createMockEditorComposablePasteDestination({ | ||
| uri: options.uri, | ||
| viewColumn: options.viewColumn, | ||
| }), |
There was a problem hiding this comment.
Include viewColumn in the editor cache key to avoid collisions.
Using only fsPath can collapse multiple editors of the same file opened in different columns into one cached destination, which can mask view-column-specific behavior in tests.
🔧 Suggested fix
- const cacheKey = `text-editor:${fsPath}`;
+ const cacheKey = `text-editor:${fsPath}:${options.viewColumn ?? 'unknown'}`;📝 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.
Suggested change
| if (options.kind === 'text-editor' && options.uri) { | |
| const fsPath = options.uri.fsPath; | |
| const cacheKey = `text-editor:${fsPath}`; | |
| if (!destinationCache.has(cacheKey)) { | |
| destinationCache.set( | |
| cacheKey, | |
| createMockEditorComposablePasteDestination({ editor: options.editor }), | |
| createMockEditorComposablePasteDestination({ | |
| uri: options.uri, | |
| viewColumn: options.viewColumn, | |
| }), | |
| if (options.kind === 'text-editor' && options.uri) { | |
| const fsPath = options.uri.fsPath; | |
| const cacheKey = `text-editor:${fsPath}:${options.viewColumn ?? 'unknown'}`; | |
| if (!destinationCache.has(cacheKey)) { | |
| destinationCache.set( | |
| cacheKey, | |
| createMockEditorComposablePasteDestination({ | |
| uri: options.uri, | |
| viewColumn: options.viewColumn, | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts`
around lines 146 - 155, The cache key for editor destinations uses only fsPath
which can collide for the same file opened in different view columns; update the
key generation in the branch where options.kind === 'text-editor' and
options.uri exists to include options.viewColumn (e.g., incorporate viewColumn
into cacheKey alongside fsPath) so destinationCache entries are unique per
editor column and createMockEditorComposablePasteDestination receives the
correct per-column instance.
Comment on lines
+540
to
541
| const result = await manager.bind({ kind: 'text-editor', uri: mockUri, viewColumn: 1 }); | ||
|
|
There was a problem hiding this comment.
Replace repeated viewColumn numeric literals with a named constant.
The repeated viewColumn: 1 literals carry semantic meaning and should be centralized for consistency and easy updates.
🔧 Suggested fix
+const VIEW_COLUMN_ONE = 1;- const result = await manager.bind({ kind: 'text-editor', uri: mockUri, viewColumn: 1 });
+ const result = await manager.bind({
+ kind: 'text-editor',
+ uri: mockUri,
+ viewColumn: VIEW_COLUMN_ONE,
+ });Also applies to: 574-575, 601-602, 631-632, 958-962, 1498-1511, 1536-1552, 1793-1806, 1889-1902, 2040-2053
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts`
around lines 540 - 541, The tests repeatedly use the numeric literal 1 for
viewColumn (e.g., calls like manager.bind({ kind: 'text-editor', uri: mockUri,
viewColumn: 1 })); define a single SCREAMING_SNAKE_CASE constant (e.g.,
VIEW_COLUMN = 1) near the top of the test file and replace all literal uses (all
manager.bind and similar assertions/inputs listed in the comment) with that
constant to centralize the semantic value and improve maintainability.
Owner
Author
There was a problem hiding this comment.
Ignore this one; I will not fix it as you expect it
There was a problem hiding this comment.
@couimet, understood – I'll skip this one. Thanks for the feedback!
🧠 Learnings used
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-16T12:18:04.041Z
Learning: Applies to **/*.{ts,tsx} : Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE naming
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
Show resolved
Hide resolved
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
Outdated
Show resolved
Hide resolved
…gger coverage Addresses CodeRabbit follow-up review on PR #369. Replaces the last partial multi-group assertion with full toStrictEqual, converts MessageCode enum references to string literals in expect() calls, and adds missing logger assertions for bindTextEditor error paths and the bound-state grouped items flow. Benefits: - Multi-group test now catches regressions in both file items (T002/T004 compliance) - String literals in assertions freeze the MessageCode contract (T003 compliance) - bindTextEditor error paths verify warn logging for all three rejection reasons (T007 compliance) - Bound-state test now asserts both logger calls matching the pattern in sibling tests Ignored Feedback: - Include viewColumn in editor cache key: no test opens the same file in two columns — premature optimization - Replace viewColumn: 1 with VIEW_COLUMN_ONE constant: vscode.ViewColumn.One is a well-known constant, not a magic number - Missing logger assertions in getFileItems() tests: thin passthrough that doesn't emit its own logs Ref: #369 (review)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (1)
145-147: Cache key still omitsviewColumn— same file in two columns maps to one cached destination.Line 147 still uses only
fsPathas the discriminator. A file opened in column 1 and column 2 would resolve to the same cached mock, masking column-specific bind behavior in tests.🔧 Suggested fix
- const cacheKey = `text-editor:${fsPath}`; + const cacheKey = `text-editor:${fsPath}:${options.viewColumn ?? 'unknown'}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts` around lines 145 - 147, The cache key for text-editor destinations only uses the file system path (cacheKey = `text-editor:${fsPath}`) which conflates the same file opened in different view columns; update the key construction in the PasteDestinationManager test to also include options.viewColumn (e.g., incorporate `viewColumn` into cacheKey) so mocks for the same fsPath but different viewColumn are distinct; locate the branch checking options.kind === 'text-editor' and options.uri and change the cacheKey formation to include the viewColumn identifier from options.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (1)
1513-1519: PrefercreateMockUriover inline URI literals for consistency.Both auto-unbind tests construct the URI by hand (missing the
pathproperty thatcreateMockUrisets) whilecreateMockUriis already imported and used everywhere else in the file.♻️ Proposed refactor
- const mockUri = { - scheme: 'file', - fsPath: '/test/file.ts', - toString: () => 'file:///test/file.ts', - } as vscode.Uri; + const mockUri = createMockUri('/test/file.ts');Same substitution applies to
boundUriat lines 1551–1555.Also applies to: 1551-1560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts` around lines 1513 - 1519, Replace the inline URI objects used in the auto-unbind tests with the shared helper createMockUri to ensure consistency and include the missing path property; specifically, stop creating mockUri, mockDocument, and mockEditor by hand and instead call createMockUri(...) for the URI used to build mockDocument/mockEditor, and do the same substitution for boundUri at the later occurrence (the variables named mockUri/mockDocument/mockEditor and boundUri in these tests should be replaced to use createMockUri).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts`:
- Around line 629-664: Add the missing logger assertion in the two tests that
call DestinationAvailabilityService.getGroupedDestinationItems with
destinationKinds (the "shows file-more" and "shows no file-more" tests): after
calling getGroupedDestinationItems, assert mockLogger.debug was called with the
debug payload for the "Using provided destinationKinds filter" entry (i.e.,
verify mockLogger.debug was called with { fn:
'DestinationAvailabilityService.getGroupedDestinationItems', destinationKinds:
['text-editor'] } and the message 'Using provided destinationKinds filter')
before asserting the 'Built grouped destination items' log; use the same
mockLogger.debug matcher pattern as other tests in the describe block to keep
consistency with existing assertions.
---
Duplicate comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts`:
- Around line 145-147: The cache key for text-editor destinations only uses the
file system path (cacheKey = `text-editor:${fsPath}`) which conflates the same
file opened in different view columns; update the key construction in the
PasteDestinationManager test to also include options.viewColumn (e.g.,
incorporate `viewColumn` into cacheKey) so mocks for the same fsPath but
different viewColumn are distinct; locate the branch checking options.kind ===
'text-editor' and options.uri and change the cacheKey formation to include the
viewColumn identifier from options.
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts`:
- Around line 1513-1519: Replace the inline URI objects used in the auto-unbind
tests with the shared helper createMockUri to ensure consistency and include the
missing path property; specifically, stop creating mockUri, mockDocument, and
mockEditor by hand and instead call createMockUri(...) for the URI used to build
mockDocument/mockEditor, and do the same substitution for boundUri at the later
occurrence (the variables named mockUri/mockDocument/mockEditor and boundUri in
these tests should be replaced to use createMockUri).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
...rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts
Show resolved
Hide resolved
…ace inline URIs with createMockUri Addresses CodeRabbit third follow-up review on PR #369. Adds the "Using provided destinationKinds filter" logger assertion to the two file-more tests that were missing it, and replaces inline URI object literals in auto-unbind tests with the shared createMockUri helper for consistency. Benefits: - All 5 tests that pass destinationKinds now assert the filter log (T007 compliance) - Auto-unbind tests use createMockUri consistently with the rest of the file Ignored Feedback: - Include viewColumn in editor cache key: third time raised — no test opens the same file in two columns, premature optimization Ref: #369 (review)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (1)
537-540: Extract a named constant forviewColumn: 1literals.Centralize the semantic value to comply with the numeric-literal guideline and keep the tests consistent.
As per coding guidelines, "Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE naming".♻️ Suggested pattern
+const VIEW_COLUMN_ONE = 1;- const result = await manager.bind({ kind: 'text-editor', uri: mockUri, viewColumn: 1 }); + const result = await manager.bind({ + kind: 'text-editor', + uri: mockUri, + viewColumn: VIEW_COLUMN_ONE, + });Also applies to: 567-589, 598-620, 630-654, 972-976, 1513-1527, 1547-1559, 1799-1813, 1896-1909, 2047-2060
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts` around lines 537 - 540, Replace literal viewColumn: 1 occurrences in the PasteDestinationManager tests with a named SCREAMING_SNAKE_CASE constant (e.g., DEFAULT_VIEW_COLUMN or VIEW_COLUMN_ONE) and use that constant wherever viewColumn: 1 is passed (examples: the call to manager.bind in PasteDestinationManager.test.ts and the other listed ranges); update imports or test-scope const declarations so the tests reference the constant instead of the numeric literal to satisfy the numeric-literal guideline.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts (1)
463-749: Extract semantic numeric literals into named constants.
viewColumnvalues andremainingCountare meaningful and repeated; define SCREAMING_SNAKE_CASE constants to align with guidelines.♻️ Suggested refactor
describe('file items via text-editor kind', () => { + const VIEW_COLUMN_PRIMARY = 1; + const VIEW_COLUMN_SECONDARY = 2; + const REMAINING_FILES_COUNT = 2; + it('returns file items when eligible files exist', async () => { const uri = createMockUri('/workspace/src/app.ts'); const tab = createMockTab(uri); const group = createMockTabGroup([tab]); @@ - bindOptions: { kind: 'text-editor', uri, viewColumn: 1 }, + bindOptions: { kind: 'text-editor', uri, viewColumn: VIEW_COLUMN_PRIMARY }, @@ - viewColumn: 1, + viewColumn: VIEW_COLUMN_PRIMARY,Apply consistently across this block (e.g.,
viewColumn: 2andremainingCount: 2).</details> As per coding guidelines, "Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE naming". <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts
around lines 463 - 749, The tests in DestinationAvailabilityService.test use
repeated semantic numeric literals (e.g., viewColumn: 1 and 2, remainingCount:
2) — extract these into SCREAMING_SNAKE_CASE named constants (e.g.,
VIEW_COLUMN_1, VIEW_COLUMN_2, FILE_MORE_REMAINING_COUNT) and replace inline
numbers in the test cases that create tabs/groups (createMockTab,
createMockTabGroup) and assertions for 'file-more' so the meaning is clear and
consistent across the describe('file items via text-editor kind') block and
related expectations.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In
@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts:
- Around line 752-779: Tests for DestinationAvailabilityService.getFileItems()
are missing assertions that the mockLogger was called as required; update both
test cases to assert logging behavior on mockLogger (e.g., verify
mockLogger.info/debug/error called with expected messages/contexts) after
invoking service.getFileItems(), and for the delegating test also assert that
the logger recorded the delegation call; locate references to getFileItems(),
getGroupedDestinationItems, DestinationAvailabilityService and mockLogger in the
test file and add the appropriate
expect(mockLogger.xxx).toHaveBeenCalledWith(...) checks matching the existing
logging conventions used elsewhere in tests.
Duplicate comments:
In
@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts:
- Around line 537-540: Replace literal viewColumn: 1 occurrences in the
PasteDestinationManager tests with a named SCREAMING_SNAKE_CASE constant (e.g.,
DEFAULT_VIEW_COLUMN or VIEW_COLUMN_ONE) and use that constant wherever
viewColumn: 1 is passed (examples: the call to manager.bind in
PasteDestinationManager.test.ts and the other listed ranges); update imports or
test-scope const declarations so the tests reference the constant instead of the
numeric literal to satisfy the numeric-literal guideline.
Nitpick comments:
In
@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts:
- Around line 463-749: The tests in DestinationAvailabilityService.test use
repeated semantic numeric literals (e.g., viewColumn: 1 and 2, remainingCount:
- — extract these into SCREAMING_SNAKE_CASE named constants (e.g.,
VIEW_COLUMN_1, VIEW_COLUMN_2, FILE_MORE_REMAINING_COUNT) and replace inline
numbers in the test cases that create tabs/groups (createMockTab,
createMockTabGroup) and assertions for 'file-more' so the meaning is clear and
consistent across the describe('file items via text-editor kind') block and
related expectations.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c83d92fd581846f9466de285aeeeff46645f359a and 2c8b2b61a09a566f2cfa663f84fcb41802e9106e. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts` * `packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+752
to
+779
| describe('getFileItems()', () => { | ||
| it('returns empty array when no files exist', async () => { | ||
| ideAdapter = createMockVscodeAdapter({ | ||
| windowOptions: { | ||
| tabGroups: { all: [] }, | ||
| }, | ||
| }); | ||
| service = new DestinationAvailabilityService( | ||
| mockRegistry, | ||
| ideAdapter, | ||
| mockConfigReader, | ||
| mockLogger, | ||
| ); | ||
|
|
||
| const result = await service.getFileItems(); | ||
|
|
||
| expect(result).toStrictEqual([]); | ||
| }); | ||
|
|
||
| it('delegates to getGroupedDestinationItems with text-editor filter and boundFileUriString', async () => { | ||
| const spy = jest.spyOn(service, 'getGroupedDestinationItems'); | ||
|
|
||
| await service.getFileItems('file:///workspace/src/app.ts'); | ||
|
|
||
| expect(spy).toHaveBeenCalledWith({ | ||
| destinationKinds: ['text-editor'], | ||
| boundFileUriString: 'file:///workspace/src/app.ts', | ||
| }); |
There was a problem hiding this comment.
Add logger assertions in getFileItems() tests.
These tests verify behavior but omit required logging checks.
🔧 Proposed fix
it('returns empty array when no files exist', async () => {
@@
const result = await service.getFileItems();
expect(result).toStrictEqual([]);
+ expect(mockLogger.debug).toHaveBeenCalledWith(
+ {
+ fn: 'DestinationAvailabilityService.getGroupedDestinationItems',
+ destinationKinds: ['text-editor'],
+ },
+ 'Using provided destinationKinds filter',
+ );
+ expect(mockLogger.debug).toHaveBeenCalledWith(
+ {
+ fn: 'DestinationAvailabilityService.getGroupedDestinationItems',
+ groupKeys: [],
+ },
+ 'Built grouped destination items',
+ );
});
it('delegates to getGroupedDestinationItems with text-editor filter and boundFileUriString', async () => {
const spy = jest.spyOn(service, 'getGroupedDestinationItems');
await service.getFileItems('file:///workspace/src/app.ts');
expect(spy).toHaveBeenCalledWith({
destinationKinds: ['text-editor'],
boundFileUriString: 'file:///workspace/src/app.ts',
});
+ expect(mockLogger.debug).toHaveBeenCalledWith(
+ {
+ fn: 'DestinationAvailabilityService.getGroupedDestinationItems',
+ destinationKinds: ['text-editor'],
+ },
+ 'Using provided destinationKinds filter',
+ );
+ expect(mockLogger.debug).toHaveBeenCalledWith(
+ {
+ fn: 'DestinationAvailabilityService.getGroupedDestinationItems',
+ groupKeys: [],
+ },
+ 'Built grouped destination items',
+ );
});As per coding guidelines, "Always test logging behavior by including logger assertions in tests that verify method behavior - consolidate with behavior tests rather than creating separate logging tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts`
around lines 752 - 779, Tests for DestinationAvailabilityService.getFileItems()
are missing assertions that the mockLogger was called as required; update both
test cases to assert logging behavior on mockLogger (e.g., verify
mockLogger.info/debug/error called with expected messages/contexts) after
invoking service.getFileItems(), and for the delegating test also assert that
the logger recorded the delegation call; locate references to getFileItems(),
getGroupedDestinationItems, DestinationAvailabilityService and mockLogger in the
test file and add the appropriate
expect(mockLogger.xxx).toHaveBeenCalledWith(...) checks matching the existing
logging conventions used elsewhere in tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the single generic "Text Editor" item in the destination picker with individual file items showing actual filenames, disambiguators, and bound/active badges. Adds "More files..." overflow for non-current tab group files, parallel to the existing terminal picker pattern. Extracts
BindToTextEditorCommandwith two modes: URI-based (explorer/tab context menu) and picker-based (keybinding).Changes
uri+viewColumn(explicit file targeting, no implicit activeTextEditor fallback).CreateOptionsworkaround removed fromDestinationRegistry.GroupedDestinationItemsupdated with file-specific buckets.getEligibleFiles -> markBoundFile -> sortEligibleFiles -> disambiguateFilenames -> buildGroupedFileItems. AddedgetFileItems()convenience method.BindToTerminalCommand): with-URI mode detects multi-group and prompts; without-URI mode runs 0/1/2+ file picker flow.quickPickProviderparam sinceVscodeAdapteralready implementsQuickPickProvider.filePicker.maxTabGroupssetting removed — replaced by the file discovery pipeline.context,documentUriString) since there's a single caller and values derive from instance fields.EligibleFile[]directly, eliminating the intermediateFileEntryinterface.displayPath(workspace-relative or absolute) andviewColumnfrom tab group.Test Plan
Related
Summary by CodeRabbit
New Features
Bug Fixes
Chores