[issues/592] Prevent extension-managed pty terminals from being offered as bindable destinations#595
Merged
Conversation
…bility bug Locks the post-fix contract for the issue #592 bug as a red integration test before any production code changes. The pty terminal is spawned via an inline minimal Pseudoterminal alongside a shell terminal that takes active focus, so the assertion covers both the pty case AND a negative-control regression on the shell case (catches a future over-filter blocking legitimate shells). Today the test fails on `description: undefined` and `nonBindableReason: undefined` for the pty entry. The shell entry validates as expected. Subsequent steps (S1 classification + S2 badge) flip the pty fields to `'not bindable'` and `'extension-managed'` and turn the test green.
…nd arg typed as `Omit<...TerminalOptions, 'name'> | Omit<...ExtensionTerminalOptions, 'name'>`, spread into the call. All existing one-arg call sites compile unchanged and the existing automated `terminal-picker-*` tests pass. The pty terminal now flows through the standard tracking + teardown path, eliminating the try/finally + manual dispose the test would otherwise need.
…rned during this spike: the `:grep` script does not accept `--` before the pattern, and it does not filter assisted tests on its own. The targeted-command guidance now spells out `pnpm test:release:grep "<pattern>" --exclude-assisted` and explains why the flag is required.
`ss.createCapturingTerminal` already enrolls the underlying terminal in the suite-level `tmpTerminals` array, which `SsContextImpl` disposes on `teardown` after each test. The `try { ... } finally { capturing.terminal.dispose(); }` wrapper around 16 dirty-buffer-warning tests was therefore double-cleanup. Removing the wrapper flattens each test body and eliminates a misleading hint that the dispose was load-bearing.
One try/finally is preserved because it restores the `files.trimTrailingWhitespace` workspace config — that is real cleanup that cannot be deferred to teardown without leaking state into the next test.
Verified: the four automated dirty-buffer-warning TCs (004, 006, 007, 019) still pass under `--exclude-assisted`. The 13 assisted TCs in the same file weren't run as part of this verification but the change is mechanical (no logic touched).
Introduces `classifyTerminalForBinding(terminal)` returning `{ visible: false }` (exited, hideFromUser, null/undefined) or `{ visible: true, nonBindableReason? }`. Extension-managed pty terminals (Jest task runner, debug consoles, etc.) now get `nonBindableReason: 'extension-managed'` instead of being silently treated as bindable. The single boolean `isTerminalEligible` is removed in favour of the richer classification — only one caller existed, so no compatibility shim is kept.
`EligibleTerminal` and `TerminalBindableQuickPickItem` both grow an optional `nonBindableReason: NonBindableReason` field that flows from `getEligibleTerminals` through `DestinationAvailabilityService.buildTerminalItem` and `buildTerminalPickerItems` to the final QuickPick item. `VscodeAdapter.showQuickPick`'s log projection is extended to surface the field so integration tests can assert on it.
The structural signal for "extension-managed" is the presence of `creationOptions.pty` (i.e. the terminal was created via `ExtensionTerminalOptions`). VS Code exposes no `isReadOnly` / `acceptsInput` property, and `isTransient` is not reliably set by every task runner, so `pty` is the most stable distinction available. Capturing pty terminals used in integration tests are also caught by this rule — S4 will neutralize that by marking them `hideFromUser: true`.
`nonBindableReason` is also surfaced at the top level of `TerminalBindableQuickPickItem`, mirroring the existing pattern for `isActive` and `boundState`. This is duplication that `VscodeAdapter.showQuickPick`'s log projection currently depends on. Two TODO comments (at the type definition and at the log projection) point at #594, which tracks dropping the duplication holistically across both terminal and file picker items.
Effect on the S0 spike (`terminal-picker-014`): the assertion mismatch on `nonBindableReason` is now satisfied. The remaining red — `description: 'not bindable'` — is S2's responsibility (badge in description).
…cker
Adds a `not bindable` badge to the terminal QuickPick description whenever the upstream `EligibleTerminal.nonBindableReason` is set. The badge composes with the existing `bound` / `active` badges via the same ` · ` separator and shows last in the sequence. `bound` and `not bindable` cannot co-occur in practice because the bind path will reject non-bindable terminals once S3 lands — `active` and `not bindable` can co-occur and are emitted in that order.
A single new `MessageCode.TERMINAL_PICKER_NOT_BINDABLE_DESCRIPTION` is added with the catalog string `"not bindable"`. VS Code's QuickPick API has no per-item disabled state and no hover tooltip, so a description badge is the closest match to the A002 design decision ("minimal tooltip we'll tightly control"). The selection-rejection guard that completes the disabled-style UX is S3 (`showTerminalPicker`).
Turns the S0 spike `terminal-picker-014` fully green: the pty entry now reports `description: 'not bindable'` alongside the `nonBindableReason: 'extension-managed'` that S1 already supplies, and the shell entry continues to validate as `description: 'active'`. The negative-control assertion catches any future over-filter that would block legitimate shell terminals.
…ckers VS Code's QuickPick has no per-item disabled state, so clicking the "not bindable" entry from S2 still resolves the picker with that item. Without a guard, the bind path would happily call `vscode.window.activeTerminal.sendText` on an extension-managed pty terminal, which is exactly the Jest-terminal bug that #592 exists to fix. `showTerminalPicker` and `DestinationPicker.handleQuickPickSelection` now check `terminalInfo.nonBindableReason` (and the equivalent top-level `nonBindableReason` for the destination picker, which sees a wider `DestinationQuickPickItem` union) before invoking the success path. When set: log the rejection, route through `handlers.onDismissed` if provided, otherwise return undefined / `{ outcome: 'cancelled' }`. The badge from S2 explains to the user why nothing happened. No notification is shown at this layer — the picker badge is the user-visible signal. S3.5 will add a notification for the parallel `CMD_BIND_TO_TERMINAL_HERE` palette / keybinding entry points where there is no equivalent visual cue. Tests: showTerminalPicker gets two new cases (default-dismiss returns undefined and skips onSelected; explicit onDismissed routes through). DestinationPicker gets one new case (clicking a pty entry returns `{ outcome: 'cancelled' }` and logs the reason). The `createMockTerminalQuickPickItem` helper now accepts an optional `nonBindableReason` argument so tests can construct disabled entries.
…ty terminals Context menus, command palette, and keybindings for "Bind Here" / "Bind to Terminal" now all reject extension-managed terminals (pty). A new context key `rangelink.isActiveTerminalBindable` hides the terminal bind menu entries when the active terminal is non-bindable, catching the paths the command handler can't (context menus render before the command runs). The `wireActiveTerminalBindabilityContext` helper maintains this key by subscribing to terminal lifecycle events.
…LOG for pty bindability (#592)
…message vocabulary The "not bindable" badge approach was leaking an implementation detail (extension-managed pty) into the UI. VS Code's own pickers (file open, command palette) filter out non-applicable items rather than rendering them disabled; aligning with that convention removes a learning surface and makes the picker faster to scan. Extension-managed pty terminals are now excluded at `getEligibleTerminals`, so the entire picker pipeline downstream sees only bindable terminals. The R-D unified picker already skipped empty sections, so a pty-only setup now silently omits the terminals section. Error messages now use "bindable" instead of "active" for eligibility failures. `ERROR_NO_ACTIVE_TEXT_EDITOR` previously fired for image viewers, webviews, and settings tabs — the message was misleading because those ARE active editors, just not text editors. The "bindable" framing matches the `isActiveTerminalBindable` context key and the `classifyTerminalForBinding` helper already in use, keeping vocabulary coherent end-to-end. `ERROR_NO_ACTIVE_TERMINAL` is preserved for `TerminalSelectionService` since that path is genuinely about focus, not eligibility. A new integration test `terminal-picker-016` asserts the absence invariant directly: a single pty terminal yields a picker with no terminal section and no overflow item. This catches the failure mode where filtering breaks and non-bindable items leak back into the UI without anything else regressing. Benefits: - Picker UX matches VS Code conventions (filter, not disable) - Smaller surface area: badge rendering, selection short-circuits, and the unreachable single-non-bindable branch all removed - Error toasts now correctly describe the PNG-viewer / pty-only failure modes - `terminal-picker-016` locks in the filtering invariant against future regressions
The `when: rangelink.isActiveTerminalBindable` clauses on `terminal/title/context` and `terminal/context` shipped in commit `6bac5adf` without any TC verifying the menu items actually hide on pty terminals in a real VS Code host. VS Code exposes no API to query rendered context-menu items, so coverage requires assisted TCs with `waitForHumanVerdict`. These four TCs close that gap: 006 and 007 prove "Bind Here" is absent on the pty tab and content-area menus respectively; 008 proves the `when` clause is not overly broad by checking that a bindable shell tab still shows the entry when a pty is also open; 009 proves "Unbind" rides on `rangelink.isBound` (not bindability) by verifying it remains reachable from the pty content-area menu when a different terminal is bound. Benefits: - Closes the safety-net gap left by the original pty-hardening commit - Catches both over-hiding (008) and under-hiding (006/007) regressions - 009 documents that Unbind reachability is independent of bindability
|
The PR introduces a new filtering behavior for extension-managed pty terminals in the terminal picker, which is a user-visible change that requires additional test coverage. Suggested test cases:
Generated by QA Gap Check (GPT-4o-mini via GitHub Models) |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR classifies terminals to identify extension-managed PTY terminals as non-bindable, wires a VS Code context key that reflects active-terminal bindability, gates terminal-bind menu visibility, rejects direct-bind attempts to non-bindable terminals, refactors eligibility, updates messages, and adds integration/unit tests and test-fixture helpers. ChangesExtension-Managed PTY Terminal Filtering and Bindability Context
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/rangelink-vscode-extension/src/__integration-tests__/suite/dirtyBufferWarning.test.ts (1)
733-762: ⚡ Quick winAdd log assertions for these assisted dirty-buffer send flows.
These tests validate behavior but don’t assert the expected
handleDirtyBufferWarninglog path. This weakens failure diagnostics (and in Line 740 / Line 775 / Line 847,logCapture.mark(...)is currently not validated).Proposed patch pattern (apply similarly to 013/014/016/017)
const logCapture = getLogCapture(); logCapture.mark('before-rf-save'); capturing.clearCaptured(); @@ await ss.settle(); + const lines = logCapture.getLinesSince('before-rf-save'); + const showingWarningLog = lines.find( + (l) => + l.includes('handleDirtyBufferWarning') && + l.includes('Document has unsaved changes, showing warning'), + ); + assert.ok( + showingWarningLog, + 'Expected handleDirtyBufferWarning to log "showing warning" — dialog must actually fire', + );As per coding guidelines, "Include logger assertions in tests that verify method behavior — logging provides critical visibility for debugging; never create separate tests just for logging, consolidate with behavior tests".
Also applies to: 768-797, 836-870, 876-907
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/src/__integration-tests__/suite/dirtyBufferWarning.test.ts` around lines 733 - 762, The test currently marks logs with logCapture.mark('before-rf-save') but never asserts the expected handleDirtyBufferWarning logging path; update this test (and the sibling tests for 013/014/016/017) to capture and assert the logger output after the R-F Save & Send flow: use getLogCapture()/logCapture to retrieve logs after the save, verify a log entry indicating the handleDirtyBufferWarning code path (e.g., a message or tag produced by handleDirtyBufferWarning), and assert that the mark 'before-rf-save' was advanced/recorded as expected so the log marker is validated alongside the existing behavior assertions.packages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.ts (1)
419-439: ⚡ Quick winAdd a logger assertion for the exited-terminal rejection path.
This test verifies behavior but currently skips the debug-log contract asserted in the PTY variant.
Suggested addition
expect(mockDestinationManager.bind).not.toHaveBeenCalled(); expect(mockAdapter.__getVscodeInstance().window.showErrorMessage).toHaveBeenCalledWith( 'Cannot bind to "dead-shell": it is managed by an extension and does not accept input.', ); + expect(mockLogger.debug).toHaveBeenCalledWith( + { + fn: 'BindToTerminalCommand.execute', + terminalName: 'dead-shell', + source: 'context-menu', + nonBindableReason: 'not-visible', + }, + 'Rejected bind to non-bindable terminal from context menu', + );As per coding guidelines
**/*.test.ts: Include logger assertions in tests that verify method behavior — logging provides critical visibility for debugging; never create separate tests just for logging, consolidate with behavior tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.ts` around lines 419 - 439, Add a logger assertion to the existing exited-terminal test: after calling command.execute(exitedTerminal) assert that mockLogger.debug (or the logger method used by BindToTerminalCommand) was called with a message indicating the bind was rejected for the exited terminal and includes the terminal name "dead-shell"; this should be added alongside the existing expectations so the test verifies both behavior and the debug log contract of BindToTerminalCommand.execute when given an exited terminal created via createMockTerminal.packages/rangelink-vscode-extension/src/__tests__/wireSubscriptions.test.ts (1)
152-157: ⚡ Quick winStrengthen the 4th disposable assertion.
The test title says active-terminal bindability context is pushed, but the body only checks call count. Please also assert the 4th pushed value is a disposable from that wiring path (not just “some 4th item”).
Suggested assertion tightening
it('pushes disposables: delimiterCache, statusBar, destinationManager, and active terminal bindability context', () => { expect(registrar.pushDisposable).toHaveBeenNthCalledWith(1, services.delimiterCache); expect(registrar.pushDisposable).toHaveBeenNthCalledWith(2, services.statusBar); expect(registrar.pushDisposable).toHaveBeenNthCalledWith(3, services.destinationManager); expect(registrar.pushDisposable).toHaveBeenCalledTimes(4); + const fourthDisposable = registrar.pushDisposable.mock.calls[3][0] as { + dispose: () => void; + }; + expect(typeof fourthDisposable.dispose).toBe('function'); });As per coding guidelines, “When testing that a list contains items, assert on specific item labels/fields, not just array.length.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/src/__tests__/wireSubscriptions.test.ts` around lines 152 - 157, The fourth disposable assertion is weak — instead of only checking call count, assert the exact fourth pushed value; update the test that currently checks registrar.pushDisposable calls so the 4th expectation verifies the active-terminal bindability disposable (e.g., add an assertion like expect(registrar.pushDisposable).toHaveBeenNthCalledWith(4, services.activeTerminalBindabilityContext) or equivalent using registrar.pushDisposable.mock.calls[3][0] to match the specific disposable from the wiring path) while keeping the existing first three assertions for services.delimiterCache, services.statusBar, and services.destinationManager.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml`:
- Around line 270-274: Update the expected_result for test id
terminal-picker-015 to use the new "bindable" terminology (or make the assertion
less string-literal brittle) instead of the old sentence that ends with "does
not accept input"; specifically edit the expected_result value for
terminal-picker-015 so it expects an error notification referencing "bindable"
(e.g., 'Cannot bind to "<name>": it is not bindable' or change to a
pattern/contains check) so the QA spec matches the new standardized messaging.
In `@packages/rangelink-vscode-extension/src/i18n/messages.en.ts`:
- Around line 14-16: The rejection message keyed by
MessageCode.BIND_TO_TERMINAL_NOT_BINDABLE_REJECT uses wording that mentions
"managed by an extension" which can be incorrect for exited or non-visible
terminals; update the string value for
MessageCode.BIND_TO_TERMINAL_NOT_BINDABLE_REJECT to a generic rejection copy
(e.g., "Cannot bind to \"{name}\": terminal does not accept input.") so it
applies to all non-bindable cases without implying extension management.
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/dirtyBufferWarning.test.ts`:
- Around line 733-762: The test currently marks logs with
logCapture.mark('before-rf-save') but never asserts the expected
handleDirtyBufferWarning logging path; update this test (and the sibling tests
for 013/014/016/017) to capture and assert the logger output after the R-F Save
& Send flow: use getLogCapture()/logCapture to retrieve logs after the save,
verify a log entry indicating the handleDirtyBufferWarning code path (e.g., a
message or tag produced by handleDirtyBufferWarning), and assert that the mark
'before-rf-save' was advanced/recorded as expected so the log marker is
validated alongside the existing behavior assertions.
In
`@packages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.ts`:
- Around line 419-439: Add a logger assertion to the existing exited-terminal
test: after calling command.execute(exitedTerminal) assert that mockLogger.debug
(or the logger method used by BindToTerminalCommand) was called with a message
indicating the bind was rejected for the exited terminal and includes the
terminal name "dead-shell"; this should be added alongside the existing
expectations so the test verifies both behavior and the debug log contract of
BindToTerminalCommand.execute when given an exited terminal created via
createMockTerminal.
In `@packages/rangelink-vscode-extension/src/__tests__/wireSubscriptions.test.ts`:
- Around line 152-157: The fourth disposable assertion is weak — instead of only
checking call count, assert the exact fourth pushed value; update the test that
currently checks registrar.pushDisposable calls so the 4th expectation verifies
the active-terminal bindability disposable (e.g., add an assertion like
expect(registrar.pushDisposable).toHaveBeenNthCalledWith(4,
services.activeTerminalBindabilityContext) or equivalent using
registrar.pushDisposable.mock.calls[3][0] to match the specific disposable from
the wiring path) while keeping the existing first three assertions for
services.delimiterCache, services.statusBar, and services.destinationManager.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea1fd1ca-84b3-4841-8b6e-3250922fe389
📒 Files selected for processing (40)
CLAUDE.mdpackages/rangelink-vscode-extension/CHANGELOG.mdpackages/rangelink-vscode-extension/package.jsonpackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yamlpackages/rangelink-vscode-extension/src/__integration-tests__/helpers/ssContext.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuTerminal.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/dirtyBufferWarning.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/terminalPicker.test.tspackages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.tspackages/rangelink-vscode-extension/src/__tests__/commands/BindToTextEditorCommand.test.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/classifyTerminalForBinding.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/getEligibleTerminals.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/isTerminalEligible.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/wireActiveTerminalBindabilityContext.test.tspackages/rangelink-vscode-extension/src/__tests__/extension.test.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockBindableQuickPickItem.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockWindow.tspackages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.tspackages/rangelink-vscode-extension/src/__tests__/statusBar/RangeLinkStatusBar.test.tspackages/rangelink-vscode-extension/src/__tests__/wireSubscriptions.test.tspackages/rangelink-vscode-extension/src/commands/BindToTerminalCommand.tspackages/rangelink-vscode-extension/src/commands/BindToTextEditorCommand.tspackages/rangelink-vscode-extension/src/constants/contextKeys.tspackages/rangelink-vscode-extension/src/destinations/DestinationPicker.tspackages/rangelink-vscode-extension/src/destinations/index.tspackages/rangelink-vscode-extension/src/destinations/utils/classifyTerminalForBinding.tspackages/rangelink-vscode-extension/src/destinations/utils/getEligibleTerminals.tspackages/rangelink-vscode-extension/src/destinations/utils/index.tspackages/rangelink-vscode-extension/src/destinations/utils/isTerminalEligible.tspackages/rangelink-vscode-extension/src/destinations/wireActiveTerminalBindabilityContext.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/EligibleTerminal.tspackages/rangelink-vscode-extension/src/types/MessageCode.tspackages/rangelink-vscode-extension/src/types/NonBindableReason.tspackages/rangelink-vscode-extension/src/types/QuickPickTypes.tspackages/rangelink-vscode-extension/src/types/index.tspackages/rangelink-vscode-extension/src/wireSubscriptions.ts
💤 Files with no reviewable changes (2)
- packages/rangelink-vscode-extension/src/tests/destinations/utils/isTerminalEligible.test.ts
- packages/rangelink-vscode-extension/src/destinations/utils/isTerminalEligible.ts
❌ CI / Test & Validate — run summary
To re-run failed tests: |
The pty terminal filtering introduced a bindability classifier that correctly excluded extension-managed terminals from the picker, but it also broke every integration test that uses a capturing pty fixture. Those tests call createAndBindCapturingTerminal which now failed because the pty terminal was classified as non-bindable, causing the bind step to show an error toast instead of binding, and subsequent paste commands to open a picker that no one was there to dismiss, timing out after 20s. The fix uses a property marker on the terminal object — not a WeakSet — because esbuild bundles the extension into dist/extension.js while integration test helpers load from out/, creating two separate module graphs that cannot share a WeakSet instance. A property on the vscode.Terminal object itself bridges that gap since both sides hold a reference to the same JS object. Also addresses CodeRabbit feedback: generic wording for the non-bindable rejection message (covers extension-managed, exited, and hideFromUser cases equally), QA YAML sync for the same message, a missing logger assertion in the exited-terminal rejection test, and a count-only disposable assertion upgraded to a spy-based identity check on the wireActiveTerminalBindabilityContext return value. Backfills handleDirtyBufferWarning log assertions in four assisted dirty-buffer-warning tests that marked the log but never read it. Benefits: - 20 integration tests that were timing out now pass - Non-bindable rejection message accurately describes all three rejection cases, not just extension-managed - Disposable assertion catches regressions where wireActiveTerminalBindabilityContext returns the wrong object - Logger assertions close a visibility gap for assisted dirty-buffer tests Ref: #595 (review)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__tests__/wireSubscriptions.test.ts (1)
164-169: ⚡ Quick winPrefer call matchers over direct
.mock.calls/.mock.resultsindexing.Lines 164–169 are more brittle than necessary. Use a stable mocked disposable and assert with
toHaveBeenNthCalledWith/toHaveReturnedWith.Proposed refactor
let services: ReturnType<typeof createMockWiringServices>; let wireActiveTerminalBindabilityContextSpy: jest.SpyInstance; + let activeTerminalBindabilityDisposable: { dispose: jest.Mock }; beforeEach(() => { registrar = createMockSubscriptionRegistrar(); services = createMockWiringServices(); + activeTerminalBindabilityDisposable = { dispose: jest.fn() }; wireActiveTerminalBindabilityContextSpy = jest.spyOn( wireActiveTerminalBindabilityContextModule, 'wireActiveTerminalBindabilityContext', - ); + ).mockReturnValue(activeTerminalBindabilityDisposable); wireSubscriptions(registrar, services); }); @@ - const fourthDisposable = registrar.pushDisposable.mock.calls[3][0]; - expect(fourthDisposable).toHaveProperty('dispose'); - expect(typeof fourthDisposable.dispose).toBe('function'); + expect(registrar.pushDisposable).toHaveBeenNthCalledWith( + 4, + activeTerminalBindabilityDisposable, + ); expect(wireActiveTerminalBindabilityContextSpy).toHaveBeenCalledTimes(1); - expect(wireActiveTerminalBindabilityContextSpy.mock.results[0].value).toBe(fourthDisposable); + expect(wireActiveTerminalBindabilityContextSpy).toHaveReturnedWith( + activeTerminalBindabilityDisposable, + );As per coding guidelines: "Use toHaveBeenCalledWith for mock assertions - Use
.toHaveBeenCalledWith(param1, param2, ...)instead of accessing.mock.calls[0]."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/src/__tests__/wireSubscriptions.test.ts` around lines 164 - 169, Replace brittle direct mock indexing with stable call-match assertions: create a mocked disposable instance and assert registrar.pushDisposable was called with it using toHaveBeenNthCalledWith(4, mockedDisposable) (or toHaveBeenCalledWith if order not important), then assert wireActiveTerminalBindabilityContextSpy returned that same disposable using toHaveReturnedWith(mockedDisposable) or toHaveNthReturnedWith(1, mockedDisposable); refer to the existing symbols registrar.pushDisposable, wireActiveTerminalBindabilityContextSpy, and the current fourthDisposable variable when implementing the stable mockedDisposable replacement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/helpers/capturingPtyHelpers.ts`:
- Around line 52-54: The terminal created by vscode.window.createTerminal is not
disposed if markRangeLinkTestFixture(terminal) throws; wrap the calls that use
the newly created terminal (markRangeLinkTestFixture(terminal) and
trackingArray?.push(terminal)) in a try/catch, and in the catch call
terminal.dispose() before rethrowing the error so the terminal is not leaked;
ensure trackingArray?.push(terminal) only runs after markRangeLinkTestFixture
succeeds.
---
Nitpick comments:
In `@packages/rangelink-vscode-extension/src/__tests__/wireSubscriptions.test.ts`:
- Around line 164-169: Replace brittle direct mock indexing with stable
call-match assertions: create a mocked disposable instance and assert
registrar.pushDisposable was called with it using toHaveBeenNthCalledWith(4,
mockedDisposable) (or toHaveBeenCalledWith if order not important), then assert
wireActiveTerminalBindabilityContextSpy returned that same disposable using
toHaveReturnedWith(mockedDisposable) or toHaveNthReturnedWith(1,
mockedDisposable); refer to the existing symbols registrar.pushDisposable,
wireActiveTerminalBindabilityContextSpy, and the current fourthDisposable
variable when implementing the stable mockedDisposable replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75c105c6-7a3c-4a23-be4c-039e734051bb
📒 Files selected for processing (14)
packages/rangelink-vscode-extension/.vscode-test.base.mjspackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yamlpackages/rangelink-vscode-extension/src/__integration-tests__/helpers/capturingPtyHelpers.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/dirtyBufferWarning.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/terminalPicker.test.tspackages/rangelink-vscode-extension/src/__tests__/commands/BindToTerminalCommand.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/classifyTerminalForBinding.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/utils/testFixtureRegistry.test.tspackages/rangelink-vscode-extension/src/__tests__/wireSubscriptions.test.tspackages/rangelink-vscode-extension/src/destinations/utils/classifyTerminalForBinding.tspackages/rangelink-vscode-extension/src/destinations/utils/index.tspackages/rangelink-vscode-extension/src/destinations/utils/testFixtureRegistry.tspackages/rangelink-vscode-extension/src/errors/RangeLinkExtensionErrorCodes.tspackages/rangelink-vscode-extension/src/i18n/messages.en.ts
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
❌ CI / Integration Tests (with extensions) — run summary
To re-run failed tests: |
If markRangeLinkTestFixture throws (env var not set), the capturing-pty terminal was never disposed. The wireSubscriptions disposable test accessed mock.calls and mock.results directly, violating T006. Ref: #595 (review)
✅ CI / Integration Tests (with extensions) — run summary
|
✅ CI / Test & Validate — run summary
|
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
The R-D bind picker, the terminal context menus, and the "Bind to Terminal" command all used to treat Jest watchers, task runners, and other extension-managed pty terminals as valid bind destinations. Sending text to them is meaningless because they only accept structured
Pseudoterminaloutput, not arbitrary shell input. This branch hardens every entry point: pty terminals are filtered out of the picker, the context-menu "Bind Here" entries are hidden via a new context key, and the command handler rejects direct invocations with a clear error message.Late iteration replaced the original "not bindable" badge with VS Code-native filtering (matching how the file picker omits non-text files), and harmonized the error messages around the "bindable" vocabulary that the codebase already uses for context keys and helpers.
Changes
classifyTerminalForBinding(terminal)returns a tagged union ({ visible: false } | { visible: true, nonBindableReason? }). Drives both the picker filter and the menu-visibility context key from one place.getEligibleTerminalsexcludes any terminal where the classification carriesnonBindableReason. Downstream code paths (R-D unified picker, "Bind to Terminal" command, R-D secondary picker) see only bindable terminals, so the terminal section is omitted entirely when no shell terminals are open.rangelink.isActiveTerminalBindablemaintained bywireActiveTerminalBindabilityContext, subscribed toonDidOpenTerminal/onDidCloseTerminal/onDidChangeActiveTerminal. Theterminal/title/contextandterminal/contextbind entries gain awhenclause that reads this key.BindToTerminalCommandrejects non-bindable terminals on the context-menu (preferredTerminal) path withBIND_TO_TERMINAL_NOT_BINDABLE_REJECT. The picker-side single-non-bindable branch is unreachable after filtering and was deleted.ERROR_NO_BINDABLE_TERMINAL("No bindable terminal. Open a new terminal and try again.") used byBindToTerminalCommandwhen no bindable terminals remain. RenamedERROR_NO_ACTIVE_TEXT_EDITOR→ERROR_NO_BINDABLE_TEXT_EDITOR("No bindable text editor. Open a file and try again.") so the toast is accurate when an image viewer or webview is focused.ERROR_NO_ACTIVE_TERMINALis preserved forTerminalSelectionService, which is genuinely about focus, not eligibility.terminal-picker-014(filtered out of R-D),terminal-picker-015(context-menu reject),terminal-picker-016(R-D terminal section absent when only a pty is open). Four assisted TCscontext-menus-terminal-006–009cover the menu-visibilitywhenclause: 006/007 verify "Bind Here" is absent on pty tab and content menus (verdict-based); 008 verifies thewhenclause is not overly broad by having the human bind via the shell's tab and checking log-side that the shell — not the pty — was bound (action-based, mirroring 005's pattern); 009 verifies the pty content menu shows "Unbind" but not "Bind Here" when something else is bound, proving Unbind rides onrangelink.isBoundand not on bindability.Key discoveries
isReadOnly/acceptsInputproperty onTerminal. The only structurally stable signal for "extension-managed" is the presence ofcreationOptions.pty(i.e., the terminal was created viaExtensionTerminalOptions).isTransientis not reliably set by task runners;shellIntegrationis positive but incomplete.whenclauses cannot readcreationOptions.ptydirectly, so menu visibility required a custom context key wired through terminal lifecycle events.ERROR_NO_ACTIVE_TEXT_EDITORwas misleading for image viewers: a PNG IS an active editor, just not a text editor. The "bindable" framing matches the existingisActiveTerminalBindablecontext key and theclassifyTerminalForBindinghelper, making vocabulary coherent end-to-end.Test plan
terminal-picker-014,terminal-picker-015,terminal-picker-016,bind-to-destination-010,bind-to-destination-013— all greenpnpm test:release:automatedpnpm test:release:grep "context-menus-terminal-006|context-menus-terminal-007|context-menus-terminal-008|context-menus-terminal-009" --assistedRelated
Summary by CodeRabbit
Bug Fixes
New Features