Conversation
WalkthroughAdds terminal context-menu bind/unbind entries and a new VS Code context key constant; centralizes bound destination state management in PasteDestinationManager to set/clear the context key; updates package.json, tests, and docs to reflect the new terminal binding workflow. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant VSCode
participant PasteManager as PasteDestinationManager
participant Terminal
User->>VSCode: Right-click terminal/tab and select Bind or Unbind
VSCode->>PasteManager: executeCommand('rangelink.bindToTerminalHere' | 'rangelink.unbindDestination')
PasteManager->>Terminal: perform bind or unbind operation (assign/clear boundTerminal)
PasteManager->>VSCode: executeCommand('setContext', 'rangelink.isBound', true/false)
VSCode->>VSCode: update terminal menu visibility based on `rangelink.isBound`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In
@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts:
- Around line 32-42: Prettier is failing on the long type annotation in the
helper expectTerminalContextNotSet; run prettier --write and simplify the
annotation in that function by extracting a named type alias for
ReturnType<VscodeAdapterWithTestHooks['__getVscodeInstance']> or split the
function signature across lines so it formats cleanly, and (optionally) replace
the partial matcher expect.anything() with a more explicit value if you want
stricter negative assertions; ensure you update the expect call on
mockVscode.commands.executeCommand inside expectTerminalContextNotSet
accordingly.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/constants/contextKeys.ts (1)
1-12: Clean implementation of context key constant.The constant follows SCREAMING_SNAKE_CASE naming convention and the JSDoc clearly documents the usage pattern with
setContextand package.jsonwhenclauses.Minor nit: The alphabetical sorting instruction appears twice (line 7 in JSDoc and line 12 as standalone comment). Consider keeping just the JSDoc mention.
🧹 Optional: Remove redundant comment
export const CONTEXT_TERMINAL_IS_BOUND = 'rangelink.terminalIsBound'; - -// Keep entries sorted alphabetically by constant name.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/rangelink-vscode-extension/CHANGELOG.mdpackages/rangelink-vscode-extension/README.mdpackages/rangelink-vscode-extension/package.jsonpackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/constants/contextKeys.tspackages/rangelink-vscode-extension/src/constants/index.tspackages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,ts,tsx}: Never add comments that simply describe what the code does - the code should be self-documenting. Only add comments for non-obvious behavior, gotchas, or 'why' explanations
Use arrow functions for all new code:const fn = (param: T): R => { ... }with exception that class methods use method syntax
Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE for constant names
Files:
packages/rangelink-vscode-extension/src/constants/index.tspackages/rangelink-vscode-extension/src/constants/contextKeys.tspackages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Useundefinedfor absent values instead ofnull- TypeScript convention with lighter syntax for optional properties
Wrap external dependency behaviors through adapters (e.g.,ideAdapter.showTextDocument()) but import constants/enums directly (e.g.,vscode.TextEditorRevealType.InCenterIfOutsideViewport) - facades wrap behaviors, not types/constants
Log at DEBUG level when using default vs provided values for optional parameters, noting in the log whether defaults are being used or provided values are being used
Focus JSDoc comments on WHAT and WHY - parameters, return types, behavioral notes - never include @example blocks with code snippets as unit tests are the single source of truth for usage examples
Files:
packages/rangelink-vscode-extension/src/constants/index.tspackages/rangelink-vscode-extension/src/constants/contextKeys.tspackages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Call function directly in tests for happy path scenarios instead of usingexpect(() => fn()).not.toThrow()- Jest fails automatically on unexpected exceptions, making the extra assertion unnecessary
Use.toStrictEqual()when asserting objects or arrays in tests, never use.toContainEqual()as it's a partial matcher that hides unexpected properties
Use string literals for contract assertions in tests - use literal values for OUR enums ('Regular'notLinkType.Regular), user-facing text ('RangeLink Menu'notmessagesEn[MessageCode.X]), and configuration keys ('delimiterLine'notSETTING_DELIMITER_LINE). Exception: external library enums should use actual constants
Never use partial matchers likeexpect.objectContaining()orexpect.stringContaining()in tests - assert exact values instead, extracting relevant fields first if the full object is too verbose
Do not manually clean up mocks in afterEach blocks usingjest.clearAllMocks()or similar - Jest config already hasclearMocks,resetMocks,restoreMocksset totrue
Use.toHaveBeenCalledWith(param1, param2, ...)to verify mock call parameters in tests, never access.mock.calls[0]to extract and assert parameters separately
Always include logger assertions in tests that verify method behavior - consolidate logging tests with behavior tests, never create separate tests just for logging, as logging provides critical visibility for debugging
Use descriptive test names that explain the scenario being tested
Extract error objects as test-scoped constants when testing error scenarios, asserting on type + message + reference equality
Files:
packages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
🧠 Learnings (6)
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.{ts,tsx} : Wrap external dependency behaviors through adapters (e.g., `ideAdapter.showTextDocument()`) but import constants/enums directly (e.g., `vscode.TextEditorRevealType.InCenterIfOutsideViewport`) - facades wrap behaviors, not types/constants
Applied to files:
packages/rangelink-vscode-extension/src/constants/index.tspackages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.{js,ts,tsx} : Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE for constant names
Applied to files:
packages/rangelink-vscode-extension/src/constants/contextKeys.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use string literals for contract assertions in tests - use literal values for OUR enums (`'Regular'` not `LinkType.Regular`), user-facing text (`'RangeLink Menu'` not `messagesEn[MessageCode.X]`), and configuration keys (`'delimiterLine'` not `SETTING_DELIMITER_LINE`). Exception: external library enums should use actual constants
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Never use partial matchers like `expect.objectContaining()` or `expect.stringContaining()` in tests - assert exact values instead, extracting relevant fields first if the full object is too verbose
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Call function directly in tests for happy path scenarios instead of using `expect(() => fn()).not.toThrow()` - Jest fails automatically on unexpected exceptions, making the extra assertion unnecessary
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Do not manually clean up mocks in afterEach blocks using `jest.clearAllMocks()` or similar - Jest config already has `clearMocks`, `resetMocks`, `restoreMocks` set to `true`
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
🧬 Code graph analysis (2)
packages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.ts (1)
packages/rangelink-vscode-extension/src/constants/contextKeys.ts (1)
CONTEXT_TERMINAL_IS_BOUND(10-10)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (7)
packages/rangelink-vscode-extension/src/__tests__/helpers/createMockVscodeAdapter.ts (1)
VscodeAdapterWithTestHooks(103-111)packages/rangelink-vscode-extension/src/__tests__/helpers/createMockClaudeCodeDestination.ts (1)
createMockClaudeCodeDestination(15-20)packages/rangelink-vscode-extension/src/__tests__/helpers/createMockGitHubCopilotChatDestination.ts (1)
createMockGitHubCopilotChatDestination(15-22)packages/rangelink-vscode-extension/src/__tests__/helpers/createMockUri.ts (1)
createMockUri(25-62)packages/rangelink-vscode-extension/src/__tests__/helpers/createMockDocument.ts (1)
createMockDocument(20-26)packages/rangelink-vscode-extension/src/__tests__/helpers/createMockEditor.ts (1)
createMockEditor(142-149)packages/rangelink-vscode-extension/src/__tests__/helpers/configureEmptyTabGroups.ts (1)
configureEmptyTabGroups(20-25)
🪛 GitHub Actions: CI
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🔇 Additional comments (12)
packages/rangelink-vscode-extension/CHANGELOG.md (1)
12-15: Well-documented changelog entry.The entry clearly describes the new terminal context menu feature, the menu item names, and correctly references issue #73. Good practice to note it complements existing Command Palette commands.
packages/rangelink-vscode-extension/src/constants/index.ts (1)
3-3: LGTM!The new re-export maintains alphabetical ordering and follows the established barrel export pattern. This enables clean imports like
import { CONTEXT_TERMINAL_IS_BOUND } from '../constants'.packages/rangelink-vscode-extension/README.md (1)
90-92: Clear documentation of the new binding methods.The "How to use" section now accurately documents both the Command Palette and context menu approaches, giving users flexibility in how they bind terminals.
packages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.ts (3)
5-5: LGTM!Import follows coding guidelines: constants are imported directly rather than wrapped through adapters.
114-122: Correct context cleanup on unbind.The logic correctly:
- Captures
wasTerminalBoundbefore clearing state- Only clears the context key when a terminal was actually bound (not for other destination types)
- Uses
voidfor fire-and-forget since menu visibility updates are non-critical
458-459: Context key correctly set on terminal bind.The context key is set after the binding state is established, ensuring UI consistency. The comment clearly documents the purpose.
Note:
awaitis used here whileunbind()usesvoid. This inconsistency is minor since both work correctly —awaitensures the context is set before the success toast, which is a reasonable choice for the bind flow.packages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.ts (1)
482-532: LGTM!The new tests correctly freeze the contract for terminal context menu contributions. String literals are properly used for all assertions, and the test structure follows the established patterns in this file.
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (4)
223-228: LGTM!Correctly verifies that terminal binding sets the context key to
true. String literals used for contract assertions as per guidelines.
358-393: LGTM!Tests correctly verify that chat destination bindings (claude-code, github-copilot-chat) do not affect terminal context state. Logger assertions are included for debugging visibility as required by guidelines.
714-719: LGTM!Correctly verifies that terminal unbinding clears the context key by setting it to
false.
756-795: LGTM!Good test coverage added for unbinding claude-code and text-editor destinations. Both correctly verify that non-terminal destinations do not affect the terminal context state, and include proper status bar message assertions.
packages/rangelink-vscode-extension/package.json (1)
344-368: LGTM!Menu contributions are well-structured and correctly use the
rangelink.terminalIsBoundcontext key for conditional visibility. Bothterminal/title/context(tab context menu) andterminal/context(terminal text area context menu) are covered, providing consistent UX.The documented limitation (unbind appears for all terminals when any is bound due to VS Code lacking terminal-ID when-clauses) is an acceptable trade-off given VS Code's constraints.
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts:
- Around line 32-45: The current assertion in expectTerminalContextNotSet misses
calls where the third arg is undefined or omitted; replace the
toHaveBeenCalledWith check with an assertion that inspects
mockVscode.commands.executeCommand.mock.calls and verifies no call has args[0]
=== 'setContext' and args[1] === 'rangelink.terminalIsBound' (e.g., assert
calls.some(c => c[0] === 'setContext' && c[1] === 'rangelink.terminalIsBound')
is false) so any 2- or 3-argument invocation is caught.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,ts,tsx}: Never add comments that simply describe what the code does - the code should be self-documenting. Only add comments for non-obvious behavior, gotchas, or 'why' explanations
Use arrow functions for all new code:const fn = (param: T): R => { ... }with exception that class methods use method syntax
Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE for constant names
Files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Call function directly in tests for happy path scenarios instead of usingexpect(() => fn()).not.toThrow()- Jest fails automatically on unexpected exceptions, making the extra assertion unnecessary
Use.toStrictEqual()when asserting objects or arrays in tests, never use.toContainEqual()as it's a partial matcher that hides unexpected properties
Use string literals for contract assertions in tests - use literal values for OUR enums ('Regular'notLinkType.Regular), user-facing text ('RangeLink Menu'notmessagesEn[MessageCode.X]), and configuration keys ('delimiterLine'notSETTING_DELIMITER_LINE). Exception: external library enums should use actual constants
Never use partial matchers likeexpect.objectContaining()orexpect.stringContaining()in tests - assert exact values instead, extracting relevant fields first if the full object is too verbose
Do not manually clean up mocks in afterEach blocks usingjest.clearAllMocks()or similar - Jest config already hasclearMocks,resetMocks,restoreMocksset totrue
Use.toHaveBeenCalledWith(param1, param2, ...)to verify mock call parameters in tests, never access.mock.calls[0]to extract and assert parameters separately
Always include logger assertions in tests that verify method behavior - consolidate logging tests with behavior tests, never create separate tests just for logging, as logging provides critical visibility for debugging
Use descriptive test names that explain the scenario being tested
Extract error objects as test-scoped constants when testing error scenarios, asserting on type + message + reference equality
Files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Useundefinedfor absent values instead ofnull- TypeScript convention with lighter syntax for optional properties
Wrap external dependency behaviors through adapters (e.g.,ideAdapter.showTextDocument()) but import constants/enums directly (e.g.,vscode.TextEditorRevealType.InCenterIfOutsideViewport) - facades wrap behaviors, not types/constants
Log at DEBUG level when using default vs provided values for optional parameters, noting in the log whether defaults are being used or provided values are being used
Focus JSDoc comments on WHAT and WHY - parameters, return types, behavioral notes - never include @example blocks with code snippets as unit tests are the single source of truth for usage examples
Files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
🧠 Learnings (9)
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use descriptive test names that explain the scenario being tested
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use string literals for contract assertions in tests - use literal values for OUR enums (`'Regular'` not `LinkType.Regular`), user-facing text (`'RangeLink Menu'` not `messagesEn[MessageCode.X]`), and configuration keys (`'delimiterLine'` not `SETTING_DELIMITER_LINE`). Exception: external library enums should use actual constants
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Never use partial matchers like `expect.objectContaining()` or `expect.stringContaining()` in tests - assert exact values instead, extracting relevant fields first if the full object is too verbose
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Call function directly in tests for happy path scenarios instead of using `expect(() => fn()).not.toThrow()` - Jest fails automatically on unexpected exceptions, making the extra assertion unnecessary
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Do not manually clean up mocks in afterEach blocks using `jest.clearAllMocks()` or similar - Jest config already has `clearMocks`, `resetMocks`, `restoreMocks` set to `true`
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use `.toHaveBeenCalledWith(param1, param2, ...)` to verify mock call parameters in tests, never access `.mock.calls[0]` to extract and assert parameters separately
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Extract error objects as test-scoped constants when testing error scenarios, asserting on type + message + reference equality
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use `.toStrictEqual()` when asserting objects or arrays in tests, never use `.toContainEqual()` as it's a partial matcher that hides unexpected properties
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.{ts,tsx} : Wrap external dependency behaviors through adapters (e.g., `ideAdapter.showTextDocument()`) but import constants/enums directly (e.g., `vscode.TextEditorRevealType.InCenterIfOutsideViewport`) - facades wrap behaviors, not types/constants
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
🔇 Additional comments (2)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (2)
225-229: Nice coverage: explicitsetContext('rangelink.terminalIsBound', true/false)assertions for terminal bind/unbind.
These assertions directly protect the new context-key-driven menu visibility.Also applies to: 716-720
327-327: Good regression guard: non-terminal binds/unbinds assert terminal context isn’t touched.
Once the helper is tightened (see earlier comment), these call sites become a strong safety net for the new context key behavior. (Based on learnings / test guidelines.)Also applies to: 372-373, 394-395, 484-485, 736-737, 755-797
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (1)
32-44: Consider avoidingexpect.anything()partial matcher.Per coding guidelines, partial matchers should be avoided. The
expect.anything()on line 42 is a partial matcher. Since this helper verifies thatsetContextwas NOT called with the terminal binding key, consider restructuring to checkexecuteCommandcalls more explicitly.♻️ Alternative approach
const expectTerminalContextNotSet = ( mockVscode: ReturnType<VscodeAdapterWithTestHooks['__getVscodeInstance']>, ): void => { - expect(mockVscode.commands.executeCommand).not.toHaveBeenCalledWith( - 'setContext', - 'rangelink.terminalIsBound', - expect.anything(), - ); + const executeCommandMock = mockVscode.commands.executeCommand as jest.Mock; + const setContextCalls = executeCommandMock.mock.calls.filter( + (call) => call[0] === 'setContext' && call[1] === 'rangelink.terminalIsBound' + ); + expect(setContextCalls).toHaveLength(0); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/rangelink-vscode-extension/CHANGELOG.mdpackages/rangelink-vscode-extension/README.mdpackages/rangelink-vscode-extension/package.jsonpackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/constants/contextKeys.tspackages/rangelink-vscode-extension/src/constants/index.tspackages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.ts
- packages/rangelink-vscode-extension/CHANGELOG.md
- packages/rangelink-vscode-extension/README.md
- packages/rangelink-vscode-extension/package.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,ts,tsx}: Never add comments that simply describe what the code does - the code should be self-documenting. Only add comments for non-obvious behavior, gotchas, or 'why' explanations
Use arrow functions for all new code:const fn = (param: T): R => { ... }with exception that class methods use method syntax
Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE for constant names
Files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/constants/index.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/constants/contextKeys.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Call function directly in tests for happy path scenarios instead of usingexpect(() => fn()).not.toThrow()- Jest fails automatically on unexpected exceptions, making the extra assertion unnecessary
Use.toStrictEqual()when asserting objects or arrays in tests, never use.toContainEqual()as it's a partial matcher that hides unexpected properties
Use string literals for contract assertions in tests - use literal values for OUR enums ('Regular'notLinkType.Regular), user-facing text ('RangeLink Menu'notmessagesEn[MessageCode.X]), and configuration keys ('delimiterLine'notSETTING_DELIMITER_LINE). Exception: external library enums should use actual constants
Never use partial matchers likeexpect.objectContaining()orexpect.stringContaining()in tests - assert exact values instead, extracting relevant fields first if the full object is too verbose
Do not manually clean up mocks in afterEach blocks usingjest.clearAllMocks()or similar - Jest config already hasclearMocks,resetMocks,restoreMocksset totrue
Use.toHaveBeenCalledWith(param1, param2, ...)to verify mock call parameters in tests, never access.mock.calls[0]to extract and assert parameters separately
Always include logger assertions in tests that verify method behavior - consolidate logging tests with behavior tests, never create separate tests just for logging, as logging provides critical visibility for debugging
Use descriptive test names that explain the scenario being tested
Extract error objects as test-scoped constants when testing error scenarios, asserting on type + message + reference equality
Files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Useundefinedfor absent values instead ofnull- TypeScript convention with lighter syntax for optional properties
Wrap external dependency behaviors through adapters (e.g.,ideAdapter.showTextDocument()) but import constants/enums directly (e.g.,vscode.TextEditorRevealType.InCenterIfOutsideViewport) - facades wrap behaviors, not types/constants
Log at DEBUG level when using default vs provided values for optional parameters, noting in the log whether defaults are being used or provided values are being used
Focus JSDoc comments on WHAT and WHY - parameters, return types, behavioral notes - never include @example blocks with code snippets as unit tests are the single source of truth for usage examples
Files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/constants/index.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/constants/contextKeys.ts
🧠 Learnings (10)
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use descriptive test names that explain the scenario being tested
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Never use partial matchers like `expect.objectContaining()` or `expect.stringContaining()` in tests - assert exact values instead, extracting relevant fields first if the full object is too verbose
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use string literals for contract assertions in tests - use literal values for OUR enums (`'Regular'` not `LinkType.Regular`), user-facing text (`'RangeLink Menu'` not `messagesEn[MessageCode.X]`), and configuration keys (`'delimiterLine'` not `SETTING_DELIMITER_LINE`). Exception: external library enums should use actual constants
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/constants/contextKeys.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Call function directly in tests for happy path scenarios instead of using `expect(() => fn()).not.toThrow()` - Jest fails automatically on unexpected exceptions, making the extra assertion unnecessary
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Do not manually clean up mocks in afterEach blocks using `jest.clearAllMocks()` or similar - Jest config already has `clearMocks`, `resetMocks`, `restoreMocks` set to `true`
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use `.toHaveBeenCalledWith(param1, param2, ...)` to verify mock call parameters in tests, never access `.mock.calls[0]` to extract and assert parameters separately
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Extract error objects as test-scoped constants when testing error scenarios, asserting on type + message + reference equality
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use `.toStrictEqual()` when asserting objects or arrays in tests, never use `.toContainEqual()` as it's a partial matcher that hides unexpected properties
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.{ts,tsx} : Wrap external dependency behaviors through adapters (e.g., `ideAdapter.showTextDocument()`) but import constants/enums directly (e.g., `vscode.TextEditorRevealType.InCenterIfOutsideViewport`) - facades wrap behaviors, not types/constants
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/constants/index.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.{js,ts,tsx} : Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE for constant names
Applied to files:
packages/rangelink-vscode-extension/src/constants/contextKeys.ts
🔇 Additional comments (7)
packages/rangelink-vscode-extension/src/constants/contextKeys.ts (1)
1-12: LGTM! Clean constant definition with proper documentation.The JSDoc explains the purpose well, and the constant follows SCREAMING_SNAKE_CASE naming convention per coding guidelines. Minor observation: the sorting instruction appears both in the JSDoc (line 7) and as a trailing comment (line 12) — consider keeping only one to avoid duplication.
packages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.ts (1)
481-532: LGTM! Well-structured contract tests for terminal context menus.The tests correctly:
- Use string literals for context keys (
'rangelink.terminalIsBound') and commands per coding guidelines- Verify exact structure with
.toStrictEqual()- Have descriptive test names explaining the scenario
- Test both menu item count and individual properties
packages/rangelink-vscode-extension/src/constants/index.ts (1)
1-5: LGTM!The re-export follows the established pattern and maintains alphabetical ordering.
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (4)
225-229: LGTM! Correct assertion for terminal context binding.The test properly verifies that
setContextis called with the exact key and value (true) when binding to a terminal.
716-720: LGTM! Correct assertion for terminal context unbinding.The test properly verifies that
setContextis called withfalsewhen unbinding from a terminal.
360-395: LGTM! Test coverage for chat destination bindings.Tests correctly verify that:
- Claude Code and GitHub Copilot Chat bind successfully when available
- Terminal context is not affected for non-terminal destinations
- Logger assertions are included per coding guidelines
757-797: LGTM! Comprehensive unbind tests for non-terminal destinations.The new tests for unbinding
claude-codeandtext-editorproperly:
- Verify the bound state is cleared
- Check the correct status bar message is shown
- Assert terminal context is not affected via
expectTerminalContextNotSet
…values `expect.anything()` doesn't match `undefined`/`null`, so the helper could miss edge cases. Replaced with explicit checks for `true` and `false` - the only values the production code uses. Benefits: - More robust assertion that explicitly covers both possible context values - Follows existing test patterns (toHaveBeenCalledWith over manual mock.calls inspection) Ref: #233 (comment)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (1)
55-76: Consider refactoring helper to avoid.mock.calls[0]access.The
expectQuickPickConfirmationhelper accesses.mock.calls[0]to extract call arguments for detailed inspection. While this works for complex QuickPick structure validation, the coding guidelines prefer using.toHaveBeenCalledWith()directly.For this particular case, since you need to verify partial string content within nested objects, the current approach is pragmatic. However, if the helper is frequently used, consider whether a custom matcher could provide the same functionality while adhering more closely to guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,ts,tsx}: Never add comments that simply describe what the code does - the code should be self-documenting. Only add comments for non-obvious behavior, gotchas, or 'why' explanations
Use arrow functions for all new code:const fn = (param: T): R => { ... }with exception that class methods use method syntax
Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE for constant names
Files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Call function directly in tests for happy path scenarios instead of usingexpect(() => fn()).not.toThrow()- Jest fails automatically on unexpected exceptions, making the extra assertion unnecessary
Use.toStrictEqual()when asserting objects or arrays in tests, never use.toContainEqual()as it's a partial matcher that hides unexpected properties
Use string literals for contract assertions in tests - use literal values for OUR enums ('Regular'notLinkType.Regular), user-facing text ('RangeLink Menu'notmessagesEn[MessageCode.X]), and configuration keys ('delimiterLine'notSETTING_DELIMITER_LINE). Exception: external library enums should use actual constants
Never use partial matchers likeexpect.objectContaining()orexpect.stringContaining()in tests - assert exact values instead, extracting relevant fields first if the full object is too verbose
Do not manually clean up mocks in afterEach blocks usingjest.clearAllMocks()or similar - Jest config already hasclearMocks,resetMocks,restoreMocksset totrue
Use.toHaveBeenCalledWith(param1, param2, ...)to verify mock call parameters in tests, never access.mock.calls[0]to extract and assert parameters separately
Always include logger assertions in tests that verify method behavior - consolidate logging tests with behavior tests, never create separate tests just for logging, as logging provides critical visibility for debugging
Use descriptive test names that explain the scenario being tested
Extract error objects as test-scoped constants when testing error scenarios, asserting on type + message + reference equality
Files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Useundefinedfor absent values instead ofnull- TypeScript convention with lighter syntax for optional properties
Wrap external dependency behaviors through adapters (e.g.,ideAdapter.showTextDocument()) but import constants/enums directly (e.g.,vscode.TextEditorRevealType.InCenterIfOutsideViewport) - facades wrap behaviors, not types/constants
Log at DEBUG level when using default vs provided values for optional parameters, noting in the log whether defaults are being used or provided values are being used
Focus JSDoc comments on WHAT and WHY - parameters, return types, behavioral notes - never include @example blocks with code snippets as unit tests are the single source of truth for usage examples
Files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
🧠 Learnings (8)
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use descriptive test names that explain the scenario being tested
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Never use partial matchers like `expect.objectContaining()` or `expect.stringContaining()` in tests - assert exact values instead, extracting relevant fields first if the full object is too verbose
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use string literals for contract assertions in tests - use literal values for OUR enums (`'Regular'` not `LinkType.Regular`), user-facing text (`'RangeLink Menu'` not `messagesEn[MessageCode.X]`), and configuration keys (`'delimiterLine'` not `SETTING_DELIMITER_LINE`). Exception: external library enums should use actual constants
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Call function directly in tests for happy path scenarios instead of using `expect(() => fn()).not.toThrow()` - Jest fails automatically on unexpected exceptions, making the extra assertion unnecessary
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Do not manually clean up mocks in afterEach blocks using `jest.clearAllMocks()` or similar - Jest config already has `clearMocks`, `resetMocks`, `restoreMocks` set to `true`
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use `.toHaveBeenCalledWith(param1, param2, ...)` to verify mock call parameters in tests, never access `.mock.calls[0]` to extract and assert parameters separately
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Extract error objects as test-scoped constants when testing error scenarios, asserting on type + message + reference equality
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
📚 Learning: 2026-01-10T17:19:31.670Z
Learnt from: CR
Repo: couimet/rangeLink PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:19:31.670Z
Learning: Applies to **/*.test.{ts,tsx} : Use `.toStrictEqual()` when asserting objects or arrays in tests, never use `.toContainEqual()` as it's a partial matcher that hides unexpected properties
Applied to files:
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts
🧬 Code graph analysis (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (6)
packages/rangelink-vscode-extension/src/__tests__/helpers/createMockClaudeCodeDestination.ts (1)
createMockClaudeCodeDestination(15-20)packages/rangelink-vscode-extension/src/__tests__/helpers/createMockGitHubCopilotChatDestination.ts (1)
createMockGitHubCopilotChatDestination(15-22)packages/rangelink-vscode-extension/src/__tests__/helpers/createMockUri.ts (1)
createMockUri(25-62)packages/rangelink-vscode-extension/src/__tests__/helpers/createMockDocument.ts (1)
createMockDocument(20-26)packages/rangelink-vscode-extension/src/__tests__/helpers/createMockEditor.ts (1)
createMockEditor(142-149)packages/rangelink-vscode-extension/src/__tests__/helpers/configureEmptyTabGroups.ts (1)
configureEmptyTabGroups(20-25)
🔇 Additional comments (6)
packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (6)
32-49: LGTM!Well-designed helper function with clear JSDoc explaining its purpose. Correctly uses string literals for the context key contract assertion per coding guidelines.
211-235: LGTM!Test correctly verifies the terminal context is set to
trueupon successful binding, with logger assertions consolidated in the same test per coding guidelines.
365-400: LGTM!Tests correctly verify that non-terminal destinations (Claude Code, GitHub Copilot Chat) don't affect terminal context state. Logger assertions are consolidated with behavior tests per coding guidelines.
459-490: LGTM!Test correctly verifies text-editor binding doesn't affect terminal context state.
707-802: LGTM!Comprehensive unbind tests that correctly verify:
- Terminal unbind triggers
setContext(false)- Non-terminal destinations (Cursor AI, GitHub Copilot Chat, Claude Code, Text Editor) don't affect terminal context
The new tests for Claude Code and Text Editor unbinding follow the established pattern and include proper assertions.
1933-1936: Clarification onjest.clearAllMocks()usage within tests.While the coding guidelines state "Do not manually clean up mocks in afterEach blocks", the usage here within a test body to isolate logger assertions after
bind()is a valid pattern. This is different from cleanup between tests (which Jest handles automatically).The comment "Clear logger calls from bind()" makes the intent clear. No change needed.
Adds CONTEXT_TERMINAL_IS_BOUND constant to support `when` clause conditionals in package.json menus. This enables showing/hiding context menu items based on whether a terminal is currently bound as a RangeLink destination. Part of #73 - Terminal Binding Context Menu
…values `expect.anything()` doesn't match `undefined`/`null`, so the helper could miss edge cases. Replaced with explicit checks for `true` and `false` - the only values the production code uses. Benefits: - More robust assertion that explicitly covers both possible context values - Follows existing test patterns (toHaveBeenCalledWith over manual mock.calls inspection) Ref: #233 (comment)
Removes misleading checkmark indicator since VSCode's context menus can't differentiate which terminal triggered them. Status bar remains the single source of truth for binding state. Changes: - Remove `rangelink.terminalBoundIndicator` command and checkmark indicator - Remove `rangelink.terminalIsBound` context key (no longer needed) - Add `rangelink.isBound` context key (true when ANY destination is bound) - Add `rangelink.bindToTerminalHere` command with short label for context menus - Keep `rangelink.bindToTerminal` with full title for command palette discoverability - Terminal menus now show: "Bind RangeLink Here" (always) + "Unbind RangeLink" (when bound) - Centralize binding state management via `setBoundDestination()` and `clearBoundDestination()` helpers - Add `expectContextKeys()` test helper Benefits: - Cleaner menus without misleading per-terminal indicators - "Bind RangeLink Here" always visible, allowing terminal rebinding - Unbind option works for all destination types via `rangelink.isBound` - Single source of truth for binding state mutations
|
@CodeRabbit I'm done with the iterations, review again |
|
✅ Actions performedReview triggered.
|
|
@CodeRabbit I pushed new changes this morning on this branch, I read #233 (comment), but I was expecting you would pick up the changes as incremental reviews ? Is there a different command I should have used ? |
🧩 Analysis chain🏁 Script executed: Length of output: 354 🏁 Script executed: Length of output: 484
It appears the automatic review didn't trigger as expected for your commits this morning. This can occasionally happen if:
You don't need to use a different command. The behavior you expected (automatic incremental review) is the correct default behavior. Your manual Let me review the new changes you pushed this morning now. ✅ Actions performedReview triggered.
|
Add context menu entries when right-clicking inside the editor text area, matching the terminal UX established in PR #233. Changes: - Add `rangelink.bindToTextEditorHere` command (hidden from Command Palette) - Add "Bind RangeLink Here" to editor/context menu (group 8_rangelink@0) - Add "Unbind RangeLink" to editor/context menu when bound (group 8_rangelink@7) - Update contract tests for new command and menu entries Benefits: - Consistent UX across terminal and editor context menus - Quick binding without Command Palette lookup - Menu items grouped with other RangeLink actions
* [issues/246] add text editor context menu for bind/unbind (#246) Add context menu entries when right-clicking inside the editor text area, matching the terminal UX established in PR #233. Changes: - Add `rangelink.bindToTextEditorHere` command (hidden from Command Palette) - Add "Bind RangeLink Here" to editor/context menu (group 8_rangelink@0) - Add "Unbind RangeLink" to editor/context menu when bound (group 8_rangelink@7) - Update contract tests for new command and menu entries Benefits: - Consistent UX across terminal and editor context menus - Quick binding without Command Palette lookup - Menu items grouped with other RangeLink actions * [issues/246] register bindToTextEditorHere command handler Wire up the new context menu command by registering it in extension.ts, following the same shared handler pattern established for terminal binding. Changes: - Import CMD_BIND_TO_TEXT_EDITOR_HERE constant - Extract bindToTextEditorHandler (reuses existing text-editor bind logic) - Register both CMD_BIND_TO_TEXT_EDITOR and CMD_BIND_TO_TEXT_EDITOR_HERE with the shared handler - Update extension.test.ts command registration count (21 → 22) - Add 'rangelink.bindToTextEditorHere' to expectedCommands array * [issues/246] update documentation for text editor context menu Add documentation for the new "Bind RangeLink Here" context menu entry that appears when right-clicking inside the editor text area. Changes: - CHANGELOG.md: Add entry under [Unreleased] → Added - README.md: Update "How to use" section with right-click option * Caught a small miss from before as I was reviewing the `CHANGELOG` * [issues/246] support `untitled` files in editor context menu The "Bind RangeLink Here" context menu entry should appear for untitled/scratch files, not just saved files. This aligns with the documented workflow where users create scratch files with Cmd+N for drafting AI prompts. Changes: - Update `when` clause: `resourceScheme == file || resourceScheme == untitled` - Update contract test to match
1 participant
Summary
Test plan
pnpm test- all 1183 tests passImplementation Details
UX Decision: No Per-Terminal Checkmark
Originally planned checkmark indicator was removed because VSCode's context menu
whenclauses evaluate against global context - there's no way to know which terminal triggered the menu. Showing a checkmark on ALL terminals when only one is bound would be misleading.Final UX:
rangelink.isBound)Changes
rangelink.bindToTerminalHere(short label for menus)rangelink.isBound(true when any destination bound)setBoundDestination()/clearBoundDestination()expectContextKeys()helper validates keys + valuesCode Quality Improvements
boundDestinationand context keysexpectContextKeys({ 'rangelink.isBound': true/false })replaces two separate assertionsCloses #73