[issues/356] File Picker — Secondary "More Files..." Picker#370
Open
[issues/356] File Picker — Secondary "More Files..." Picker#370
Conversation
…e method Lays the foundation for the secondary file picker. FilePickerHandlers<T> is the handler interface parallel to TerminalPickerHandlers<T> — same shape, typed for EligibleFile. getAllFileItems() runs the full pipeline (getEligibleFiles → markBoundFile → sortEligibleFiles → disambiguateFilenames → buildFileItem) for ALL files regardless of isCurrentInGroup, where getFileItems() only returns the current-in-group subset used by the main picker. The distinction matters: the main picker shows one file per tab group + "More files..." overflow; the secondary picker will show everything organized into sections. Benefits: - FilePickerHandlers<T> gives the secondary picker the same handler pattern as the terminal picker - getAllFileItems() gives DestinationPicker access to all files it needs to populate the secondary picker - 4 new tests covering empty state, non-current-in-group inclusion, bound state enrichment, and disambiguator application
The same file can be open in multiple tab groups simultaneously, each with a different viewColumn. Matching on URI alone would mark every copy as bound, but only the copy in the specific editor the user bound to should show the bound badge. Benefits: - Bound badge appears only on the correct editor instance when a file is open in multiple tab groups - getGroupedDestinationItems, getFileItems, and getAllFileItems all thread viewColumn precisely - Backward-compatible: callers without a viewColumn fall back to URI-only matching
…tions The secondary file picker needs to show all open files organized into sections, not a flat list. Active files (one per tab group, the visible tab) go first under "Active Files", and the remaining overflow files are grouped by their tab group under "Tab Group N" separators — matching how VSCode itself labels tab groups. Benefits: - Secondary picker presents files in a structured, scannable layout - Section separators omitted for empty sections (e.g., no per-group section when all files are active) - Per-group sections sort by viewColumn for deterministic ordering - Passes pre-built labels and descriptions through unchanged (disambiguation and bound badges survive the transform)
The secondary file picker needs a presenter function that validates inputs, builds sectioned items, and delegates outcomes to caller-provided handlers — the same pattern already established by showTerminalPicker for terminals. Benefits: - Callers own only what varies: onSelected, onDismissed, and placeholder text - Type guard on fileInfo ensures separators returned by QuickPick are handled safely - FILE_PICKER_EMPTY_ITEMS error on empty input matches terminal picker contract
…cker The file-more item was a stub returning cancelled — clicking "More files..." did nothing useful. Now it delegates to showSecondaryFilePicker(), mirroring the showSecondaryTerminalPicker() pattern already proven for terminals. Benefits: - Selecting "More files..." shows all open files organized into sections - Dismissing the secondary picker returns to the main destination picker (loop-back) - boundFileUriString and boundFileViewColumn thread through to getAllFileItems for correct bound-state badges when the same URI is open in multiple tab groups
Users need to know the file picker exists and how it works — without changelog and README coverage, the feature ships invisible. This closes out #356 documentation. Benefits: - CHANGELOG gives release notes the full multi-bullet detail the feature warrants - README helps users discover the inline items, "More files..." overflow, and bound-file badge
…M menu The destinations section (files, terminals, AI assistants) ran directly into the "Go to Link" command item with no visual boundary, making the menu harder to scan. A blank separator provides a clear break between "bind a destination" items and "navigate/utility" items. Benefits: - Clear visual boundary between the destinations block and command items in all menu states (bound, unbound, no destinations) - Consistent with how the bookmarks section already uses blank separators for its own boundaries
…r menu Selecting "More files..." silently dismissed the R-M menu because the handler was a TODO stub. This wires it up to the same showFilePicker infrastructure used by DestinationPicker, so the overflow flow works consistently in both contexts. Benefits: - "More files..." now opens the secondary file picker instead of silently closing the menu - Selecting a file from the overflow picker binds it directly - Dismissing the picker re-opens the R-M menu, matching the terminal overflow behavior
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (25)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. WalkthroughThis pull request implements the secondary file picker feature for the RangeLink VS Code extension, allowing users to select specific open files as text editor destinations from a "More files..." overflow menu, organized with active files section and per-tab-group sections. Changes
Sequence DiagramsequenceDiagram
actor User
participant DestinationPicker
participant showSecondaryFilePicker
participant showFilePicker
participant QuickPickProvider
participant Handlers as FilePickerHandlers
User->>DestinationPicker: Select "More files..." item
DestinationPicker->>showSecondaryFilePicker: Invoke with bound file context
showSecondaryFilePicker->>DestinationAvailabilityService: getAllFileItems(boundURI, boundViewColumn)
DestinationAvailabilityService-->>showSecondaryFilePicker: FileBindableQuickPickItem[]
showSecondaryFilePicker->>showFilePicker: Present items with handlers
showFilePicker->>showFilePicker: buildFilePickerItems() → organize by section
showFilePicker->>QuickPickProvider: Render with "Active Files" + per-group sections
alt User selects file
User->>QuickPickProvider: Choose eligible file
QuickPickProvider-->>showFilePicker: Selected item
showFilePicker->>Handlers: onSelected(file)
Handlers-->>showFilePicker: T | Promise<T>
showFilePicker-->>showSecondaryFilePicker: Result
showSecondaryFilePicker->>DestinationService: Bind selected file as destination
else User dismisses
User->>QuickPickProvider: Cancel
QuickPickProvider-->>showFilePicker: Dismissed
showFilePicker->>Handlers: onDismissed()?
Handlers-->>showFilePicker: Result | undefined
showFilePicker-->>showSecondaryFilePicker: undefined
showSecondaryFilePicker->>DestinationPicker: Return to main picker
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~30 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 |
Owner
Author
|
@coderabbitai review I hit rate limit on #370 (comment) |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/rangelink-vscode-extension/README.md (1)
136-136: Consider adding<sup>Unreleased</sup>to the "File Picker" paragraph.The File Picker is an unreleased feature documented within the released "Text Editor" section. While the coding guidelines require
<sup>Unreleased</sup>markers on new section headers and new table rows (and the [!IMPORTANT] banner already exists), this inline paragraph sits in a grey area. The analogous "Terminal Picker:" paragraph (Line 107) is a released feature, so they are currently visually equivalent but semantically different. Adding<sup>Unreleased</sup>on the bold lead would make the unreleased status visible to readers scanning the section.📝 Suggested change
-**File Picker:** In the destination picker and RangeLink Menu, open text editor files appear as individual items. +**File Picker:** <sup>Unreleased</sup> In the destination picker and RangeLink Menu, open text editor files appear as individual items.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/README.md` at line 136, The "File Picker" inline paragraph is an unreleased feature but lacks the required visual marker; update the bold lead "**File Picker:**" in the README.md so it reads "**File Picker: <sup>Unreleased</sup>**" (matching the convention used for new sections/rows) to clearly indicate unreleased status, keeping the rest of the paragraph text unchanged and consistent with the existing "[!IMPORTANT]" banner and the released "Terminal Picker" entry.packages/rangelink-vscode-extension/src/destinations/utils/showFilePicker.ts (1)
59-62: Separator guard is safely defensive but practically unreachable.VS Code's QuickPick API does not return separator items as a selection result, so the
!('fileInfo' in selected)branch (Line 59) will never execute in production. It's harmless as a safety net, but worth noting it cannot be exercised by tests either.🤖 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/showFilePicker.ts` around lines 59 - 62, The guard in showFilePicker that checks if (!('fileInfo' in selected)) is unreachable because VS Code QuickPick never returns separator items as selections; remove this dead branch (the logger.debug and return undefined) and rely on the existing type narrowing/path that handles real selections for the variable selected (and its fileInfo property), or if you prefer to keep a defensive note, replace the branch with a single-line comment explaining why the check is intentionally omitted rather than leaving unreachable code in the function.packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts (1)
1035-1057: Property-level assertions may hide regressions in the full item shape.Lines 1055–1056 only assert the
descriptionproperty on each item rather than the fullFileBindableQuickPickItemstructure. If other properties change unexpectedly (e.g.,bindOptions,boundState), the test will not catch it.♻️ Proposed change — assert full item shape
- expect(result[0].description).toBe('…/a'); - expect(result[1].description).toBe('…/b'); + expect(result).toStrictEqual([ + expect.objectContaining({ description: '…/a' }), + expect.objectContaining({ description: '…/b' }), + ]);Actually, per the guideline of no partial matchers, prefer the full exact structure:
- expect(result[0].description).toBe('…/a'); - expect(result[1].description).toBe('…/b'); + expect(result[0]).toStrictEqual({ + label: 'util.ts', + displayName: 'util.ts', + description: '…/a', + bindOptions: { kind: 'text-editor', uri: uri1, viewColumn: 1 }, + itemKind: 'bindable', + fileInfo: { + uri: uri1, + filename: 'util.ts', + displayPath: 'src/a/util.ts', + viewColumn: 1, + isCurrentInGroup: true, + isActiveEditor: false, + boundState: 'not-bound', + }, + boundState: 'not-bound', + }); + expect(result[1]).toStrictEqual({ + label: 'util.ts', + displayName: 'util.ts', + description: '…/b', + bindOptions: { kind: 'text-editor', uri: uri2, viewColumn: 1 }, + itemKind: 'bindable', + fileInfo: { + uri: uri2, + filename: 'util.ts', + displayPath: 'src/b/util.ts', + viewColumn: 1, + isCurrentInGroup: false, + isActiveEditor: false, + boundState: 'not-bound', + }, + boundState: 'not-bound', + });As per coding guidelines: "Never use partial matchers like
expect.objectContaining()orexpect.stringContaining()- assert exact values instead."🤖 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 1035 - 1057, The test in DestinationAvailabilityService.test.ts is only asserting the description property of items returned by service.getAllFileItems(), which can miss regressions in the full FileBindableQuickPickItem shape (e.g., bindOptions, boundState); update the assertions to compare the entire item objects exactly for result[0] and result[1] (not just description) by constructing the expected full FileBindableQuickPickItem values using the same fixtures (uris from createMockUri('/workspace/src/a/util.ts') and createMockUri('/workspace/src/b/util.ts'), tabs from createMockTab, and group via createMockTabGroup) and assert equality (strict/deep equals) against result[0] and result[1] so the test fails if any property changes.packages/rangelink-vscode-extension/src/__tests__/statusBar/RangeLinkStatusBar.test.ts (1)
694-706: Optional: extract repeatedFileBindableQuickPickItemfixture creation.The same object shape is duplicated across three tests; a small helper would reduce noise and simplify future updates.
Also applies to: 754-766, 819-831
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/statusBar/RangeLinkStatusBar.test.ts` around lines 694 - 706, Tests repeat construction of a FileBindableQuickPickItem fixture (object using FileBindableQuickPickItem, eligibleFile, bindOptions, itemKind, fileInfo, boundState) across multiple places; extract a small factory helper like createFileBindableQuickPickItem(eligibleFile) and replace the inline objects in the tests (including the occurrences around FileBindableQuickPickItem at lines referenced) to return the same shaped object so the tests reuse the fixture and reduce duplication.packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationPicker.test.ts (1)
158-250: Consider adding an empty-secondary-file-list regression test.A test where
getAllFileItems()returns[]and the flow returns cleanly to the main picker would lock down the race-condition behavior.As per coding guidelines
**/*.test.{ts,tsx}: 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 158 - 250, Add a regression test in DestinationPicker.test.ts that simulates the secondary file list being empty and ensures the picker returns to the main picker: set mockAvailabilityService.getGroupedDestinationItems to include a 'file-more' item, have mockAvailabilityService.getAllFileItems return [] (empty array), make showQuickPickMock return the 'file-more' item first then a main item on retry, and have showFilePickerSpy resolve undefined (or call handlers.onDismissed) to simulate dismissing the empty secondary list; then call picker.pick(...) and assert showQuickPickMock was called twice, showFilePickerSpy was invoked, and the final result is the main picker's selection and appropriate debug logs from DestinationPicker.showSecondaryFilePicker and DestinationPicker.pick were emitted.
🤖 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 938-975: The two tests "marks bound file with bound state" and
"marks only the file in the matching viewColumn as bound" call
DestinationAvailabilityService.getAllFileItems but omit assertions on the
logger; add assertions that mockLogger.debug was called with the message 'Built
all file items' and a payload containing fileCount: 1 for the "marks bound file
with bound state" test and fileCount: 2 for the two-tab-group test (use the
existing mockLogger instance used in the tests and assert the debug call after
calling getAllFileItems).
- Around line 674-734: Add assertions for the logger in this test to mirror
other getGroupedDestinationItems specs: after calling
service.getGroupedDestinationItems, assert mockLogger.debug was called with the
same debug messages/args used in the other tests (e.g., the messages emitted by
DestinationAvailabilityService.getGroupedDestinationItems when scanning tab
groups and building file items). Keep the assertions in this test (not a
separate logging-only test), referencing mockLogger.debug and the same
expectations used in the "returns file items when eligible files exist" test so
that the logger behavior for the case with duplicate URIs in different
viewColumns is validated alongside the result assertions.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/utils/showFilePicker.test.ts`:
- Around line 92-143: The tests for showFilePicker omit assertions about
logging; update each test that calls showFilePicker (the three shown plus the
ones at 211-227) to create and reuse the mock logger from createMockLogger and
assert its mock methods were called appropriately after the call: e.g., verify
logger received a selection/log entry when a file is chosen and a handler result
is produced (for the first two tests assert the logger mock was called with a
message containing the file name or handler result), and for the placeholder
test assert the logger mock recorded the placeholder usage or that a
showQuickPick invocation was logged; use the existing logger variable and its
mocked methods (from createMockLogger) and toHaveBeenCalled/
toHaveBeenCalledWith assertions to validate logging behavior.
In `@packages/rangelink-vscode-extension/src/destinations/DestinationPicker.ts`:
- Around line 156-163: The call in DestinationPicker that passes fileItems (from
availabilityService.getAllFileItems) into showFilePicker must guard against an
empty array because showFilePicker rejects empty input; before calling
showFilePicker check if fileItems is empty and, if so, skip calling
showFilePicker and return/continue the main picker flow (e.g., return undefined
or the appropriate fallback used by the surrounding method). Apply the same
empty-array guard to both places where fileItems is passed into showFilePicker
so you never call showFilePicker with an empty list.
In `@packages/rangelink-vscode-extension/src/statusBar/RangeLinkStatusBar.ts`:
- Around line 187-194: The call to availabilityService.getAllFileItems(...) can
return an empty array and showFilePicker will throw; before calling
showFilePicker check the returned fileItems (from getAllFileItems) for
empty/undefined and short-circuit (e.g., return or show a user message) to avoid
calling showFilePicker with no items; apply the same guard to the other similar
block(s) in RangeLinkStatusBar (the code around showFilePicker usages between
lines ~192-220) so every invocation verifies fileItems.length > 0 before calling
showFilePicker.
---
Nitpick comments:
In `@packages/rangelink-vscode-extension/README.md`:
- Line 136: The "File Picker" inline paragraph is an unreleased feature but
lacks the required visual marker; update the bold lead "**File Picker:**" in the
README.md so it reads "**File Picker: <sup>Unreleased</sup>**" (matching the
convention used for new sections/rows) to clearly indicate unreleased status,
keeping the rest of the paragraph text unchanged and consistent with the
existing "[!IMPORTANT]" banner and the released "Terminal Picker" entry.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts`:
- Around line 1035-1057: The test in DestinationAvailabilityService.test.ts is
only asserting the description property of items returned by
service.getAllFileItems(), which can miss regressions in the full
FileBindableQuickPickItem shape (e.g., bindOptions, boundState); update the
assertions to compare the entire item objects exactly for result[0] and
result[1] (not just description) by constructing the expected full
FileBindableQuickPickItem values using the same fixtures (uris from
createMockUri('/workspace/src/a/util.ts') and
createMockUri('/workspace/src/b/util.ts'), tabs from createMockTab, and group
via createMockTabGroup) and assert equality (strict/deep equals) against
result[0] and result[1] so the test fails if any property changes.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/DestinationPicker.test.ts`:
- Around line 158-250: Add a regression test in DestinationPicker.test.ts that
simulates the secondary file list being empty and ensures the picker returns to
the main picker: set mockAvailabilityService.getGroupedDestinationItems to
include a 'file-more' item, have mockAvailabilityService.getAllFileItems return
[] (empty array), make showQuickPickMock return the 'file-more' item first then
a main item on retry, and have showFilePickerSpy resolve undefined (or call
handlers.onDismissed) to simulate dismissing the empty secondary list; then call
picker.pick(...) and assert showQuickPickMock was called twice,
showFilePickerSpy was invoked, and the final result is the main picker's
selection and appropriate debug logs from
DestinationPicker.showSecondaryFilePicker and DestinationPicker.pick were
emitted.
In
`@packages/rangelink-vscode-extension/src/__tests__/statusBar/RangeLinkStatusBar.test.ts`:
- Around line 694-706: Tests repeat construction of a FileBindableQuickPickItem
fixture (object using FileBindableQuickPickItem, eligibleFile, bindOptions,
itemKind, fileInfo, boundState) across multiple places; extract a small factory
helper like createFileBindableQuickPickItem(eligibleFile) and replace the inline
objects in the tests (including the occurrences around FileBindableQuickPickItem
at lines referenced) to return the same shaped object so the tests reuse the
fixture and reduce duplication.
In
`@packages/rangelink-vscode-extension/src/destinations/utils/showFilePicker.ts`:
- Around line 59-62: The guard in showFilePicker that checks if (!('fileInfo' in
selected)) is unreachable because VS Code QuickPick never returns separator
items as selections; remove this dead branch (the logger.debug and return
undefined) and rely on the existing type narrowing/path that handles real
selections for the variable selected (and its fileInfo property), or if you
prefer to keep a defensive note, replace the branch with a single-line comment
explaining why the check is intentionally omitted rather than leaving
unreachable code in the function.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/rangelink-vscode-extension/CHANGELOG.mdpackages/rangelink-vscode-extension/README.mdpackages/rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/DestinationPicker.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/buildFilePickerItems.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/markBoundFile.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/showFilePicker.test.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockBindableQuickPickItem.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockDestinationAvailabilityService.tspackages/rangelink-vscode-extension/src/__tests__/helpers/index.tspackages/rangelink-vscode-extension/src/__tests__/helpers/spyOnShowFilePicker.tspackages/rangelink-vscode-extension/src/__tests__/statusBar/RangeLinkStatusBar.test.tspackages/rangelink-vscode-extension/src/destinations/DestinationAvailabilityService.tspackages/rangelink-vscode-extension/src/destinations/DestinationPicker.tspackages/rangelink-vscode-extension/src/destinations/types/FilePickerHandlers.tspackages/rangelink-vscode-extension/src/destinations/types/index.tspackages/rangelink-vscode-extension/src/destinations/utils/buildFilePickerItems.tspackages/rangelink-vscode-extension/src/destinations/utils/index.tspackages/rangelink-vscode-extension/src/destinations/utils/markBoundFile.tspackages/rangelink-vscode-extension/src/destinations/utils/showFilePicker.tspackages/rangelink-vscode-extension/src/statusBar/RangeLinkStatusBar.tspackages/rangelink-vscode-extension/src/types/GroupedDestinationKinds.ts
...rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts
Show resolved
Hide resolved
...rangelink-vscode-extension/src/__tests__/destinations/DestinationAvailabilityService.test.ts
Show resolved
Hide resolved
packages/rangelink-vscode-extension/src/__tests__/destinations/utils/showFilePicker.test.ts
Show resolved
Hide resolved
Comment on lines
156
to
163
| const fileItems = this.availabilityService.getAllFileItems( | ||
| boundFileUriString, | ||
| boundFileViewColumn, | ||
| ); | ||
|
|
||
| const result = await showFilePicker<InternalPickerResult>( | ||
| fileItems, | ||
| this.uiProvider, |
There was a problem hiding this comment.
Guard empty fileItems before invoking showFilePicker.
showFilePicker rejects empty input. If open files change between main pick and secondary pick, this can throw instead of returning to the main picker flow.
Suggested fix
const fileItems = this.availabilityService.getAllFileItems(
boundFileUriString,
boundFileViewColumn,
);
+ if (fileItems.length === 0) {
+ this.logger.debug(logCtx, 'No files available for secondary file picker');
+ return { outcome: 'returned-to-main-picker' };
+ }
const result = await showFilePicker<InternalPickerResult>(
fileItems,Also applies to: 161-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rangelink-vscode-extension/src/destinations/DestinationPicker.ts`
around lines 156 - 163, The call in DestinationPicker that passes fileItems
(from availabilityService.getAllFileItems) into showFilePicker must guard
against an empty array because showFilePicker rejects empty input; before
calling showFilePicker check if fileItems is empty and, if so, skip calling
showFilePicker and return/continue the main picker flow (e.g., return undefined
or the appropriate fallback used by the surrounding method). Apply the same
empty-array guard to both places where fileItems is passed into showFilePicker
so you never call showFilePicker with an empty list.
Without viewColumn context, a user with multiple side-by-side editor groups sees identical "active" descriptions for each group's representative file — there's no way to tell which item belongs to which group. This is most visible when viewColumns 2 and 3 each contain a single file: both appear under the "Files" separator as plain "active" entries.
The fix appends "Tab Group N" at render time inside `buildDestinationQuickPickItems`, which is only called for the R-M inline list and the destination picker — the secondary "More files…" picker is unaffected because it uses a separate rendering path (`buildFilePickerItems`).
Benefits:
- Users with multi-column workspaces can immediately identify which Tab Group each inline file belongs to
- The bound badge ("bound · Tab Group N") and active badge ("active · Tab Group N") now carry full location context
- Secondary file picker descriptions are unchanged — Tab Group separators there already provide context
…o foreground When a user selects a background-tab file during an R-V flow, the cursor lands at an unknown position — pasting immediately would write content somewhere unintended. The bind should succeed so the file is registered as the destination, but the auto-paste for that invocation should be skipped so the user can position their cursor first. Benefits: - R-V with a background tab now binds the file without blindly pasting at an unknown cursor position - suppressAutoPaste on BindSuccessInfo is a typed, explicit signal the caller can inspect — no hidden state or side channels - Normal bind paths are unaffected; the field is absent unless a background tab was opened
…to-paste When R-V selects a background-tab file, the bind should succeed but pasting immediately at an unknown cursor position is unsafe. The new outcome propagates the suppressAutoPaste signal from BindSuccessInfo through showPickerAndBindForPaste() so all three R-V callers abort without writing to the clipboard. Benefits: - R-V flow binds the background-tab file correctly and skips the unsafe auto-paste - All existing callers handle 'bound-no-paste' for free — their `outcome !== 'bound'` guard already returns early - Typed discriminated union makes the suppression path explicit and exhaustiveness-checked
Without the CTA, the toast only confirmed the file was opened — the user had no explicit prompt to move their cursor before issuing R-V again. The updated message tells them exactly what to do next. Benefits: - Users understand they must reposition the cursor before pasting - Consistent with the suppressed-auto-paste behavior introduced in the same flow
…() callers BindToDestinationCommand and showPickerAndBindForPaste() were calling pick() with no bound-state options, so the destination picker never highlighted which file or terminal was already bound. The DestinationPicker plumbing already supported boundFileUriString, boundFileViewColumn, and boundTerminalProcessId — the callers just weren't supplying them. Benefits: - The 'bound' badge surfaces on the currently-bound file or terminal in both R-D and the R-D secondary file picker - Consistent with RangeLinkStatusBar.showSecondaryFilePicker, which already follows this pattern - Conditional spread means callers with no bound destination emit no extra fields, keeping existing tests green
…ile workspaces The command palette 'Bind to Text Editor' action used a flat QuickPick fed by getFileItems(), which returned only one active tab per group. With multiple tab groups open, users had no way to distinguish files from different groups or reach non-active tabs. The single-file auto-bind shortcut is preserved. Benefits: - Multi-file case now shows Active Files + Tab Group N sections via showFilePicker, matching the R-M secondary picker and R-D - Bound badge appears on the currently-bound file when re-binding - Files from all tabs are reachable, not just the active one per group - getFileItems() removed — it was the only caller and is now replaced by getAllFileItems()
…s, and factory Five categories of reviewer feedback addressed: missing logger assertions (T007), two uncaught-throw race conditions, a misleading warn log on a TypeScript-only dead branch, incomplete test assertions (T002/T004), a missing README unreleased marker, and inconsistent fixture construction across 9 test files. Benefits: - Guards in DestinationPicker and RangeLinkStatusBar prevent showFilePicker from throwing FILE_PICKER_EMPTY_ITEMS when all tabs close between the "More files..." click and secondary picker opening; RangeLinkStatusBar now also shows an error toast so the user knows why the menu disappeared - Logger assertions added to getAllFileItems bound tests, getGroupedDestinationItems duplicate-URI test, and showFilePicker happy-path tests — logging is now part of the observable test contract - Dead-branch warn replaced with a clarifying comment explaining the TypeScript narrowing requirement; 100% branch coverage preserved - 'applies disambiguators' test upgraded from property-level checks to full toStrictEqual, catching any unintended shape changes - createMockTextEditorQuickPickItem factory updated to accept fileInfo and description params; 71 inline FileBindableQuickPickItem constructions across 6 test files migrated to the factory - README File Picker entry marked <sup>Unreleased</sup> per trunk-based documentation convention Ref: #370 (review)
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
Completes the file picker flow (#245) by wiring the "More files..." overflow item to a secondary QuickPick that shows all open files organized into sections. When users have more open files than fit inline in the main destination picker, they can now access them all without losing context — escaping the secondary picker returns them to the main one.
Changes
FilePickerHandlers<T>type, parallel toTerminalPickerHandlers<T>, definingonSelected,onDismissed, andgetPlaceholdercallbacksgetAllFileItems()toDestinationAvailabilityService— runs the full file pipeline (eligible → bound-mark → sort → disambiguate → build) for ALL open files, not just the current-in-group subset used by inline itemsmarkBoundFileto match on URI +viewColumntogether, preventing false "bound" badges when the same file is open in multiple tab groupsbuildFilePickerItems()— groupsFileBindableQuickPickItem[]into an "Active Files" section (current-in-group files) plus per-"Tab Group N" sections, usingvscode.QuickPickItemKind.Separatorbetween sectionsshowFilePicker<T>()presenter — validates items, builds sectioned list, shows QuickPick withFILE_PICKER_TITLEand delegated placeholder, routes selection/dismiss toFilePickerHandlersfile-morecase inDestinationPicker— replace stub withshowSecondaryFilePicker(), which callsgetAllFileItems()thenshowFilePicker(); dismiss returns{ outcome: 'returned-to-main-picker' }to loop back to main pickerboundFileUriStringandboundFileViewColumnthroughDestinationPickerOptionsto support correct bound-state marking when the same URI is open in multiple columnsTest Plan
markBoundFile— URI-only match (backward compat), URI+viewColumn match, same URI different column stays unboundDestinationAvailabilityService.getAllFileItems()— all files returned, bound state enriched, disambiguators applied, same-URI/different-column scenariosbuildFilePickerItems()— single group, multi-group, all-active, bound badge, disambiguated descriptions, empty active sectionshowFilePicker()— empty validation, selection, dismiss with/without handler, async handlers, separator/non-file item guard, placeholder delegationDestinationPicker— file-more selection returns correctbindOptions, dismiss returns to main picker loop,boundFileUriString/boundFileViewColumnpassed through togetAllFileItemsRelated
Summary by CodeRabbit
New Features
Documentation