[issues/547] Single clipboard write per R-* operation#556
Merged
Conversation
## Summary Architectural refactoring that gives ClipboardRouter sole ownership of the clipboard write. Insert factories now only execute paste commands — never touch the clipboard. Eliminates double clipboard writes, nested ClipboardPreservers, and double padding risk. No user-visible behavior changes. ## Changes - ClipboardRouter owns the sole clipboard write per operation; insert factories (AIAssistantInsertFactory, TerminalInsertFactory) only execute paste commands — their constructors drop from 4→2 and 3→2 params respectively - Two-delay model replaces the old single delay: FOCUS_TO_PASTE_DELAY_MS (pre-paste, 200ms) for focus readiness, CLIPBOARD_POST_PASTE_DELAY_MS (post-paste, 200ms) for webview async clipboard read across Electron IPC boundary - Padding pre-applied at all 4 call sites (LinkGenerator, TextSelectionPaster, FilePathPaster, TerminalSelectionService) — performPaste() removed from PasteDestinationManager - chatPasteConstants.ts renamed to pasteTimingConstants.ts; multi-command paste array replaced with single editor.action.clipboardPasteAction - focusAndPasteCommands → focusCommands in BuiltinAiAssistantDef type (backward-compatible: CustomAiAssistantConfig.focusAndPasteCommands preserved) - New ADR-0003 with MermaidJS sequence diagram documenting single clipboard write, two-delay model, and webview race ## QA & Tests - 15 new QA test cases: built-in AI assistant binding/sending (claude-code-001–005, cursor-ai-001–004, github-copilot-chat-001–004), clipboard-preservation-010, ubuntu-ctrl-keybindings-001 - New builtInAiAssistants.test.ts [assisted] integration tests for cold/warm paste verification across Claude Code, Cursor, and Copilot Chat - New platformKeybindings.test.ts integration test for Ubuntu Ctrl keybinding parity - QA YAML precondition cleanup: removed redundant "Extension installed from .vsix build" lines; added labels (requires-extensions, clipboard, ubuntu, cursor) to relevant TCs - Removed logBasedUiValidation.test.ts — its assertions now live in individual suite tests - Test infrastructure: new standardSuite.ts helper, settingsHelpers improvements - Coverage maintained at 98.2% statements, 95.2% branches ## Test Plan - [x] All existing tests pass (pnpm test — 98.2%/95.2%/95.8%/98.3% coverage) - [x] New tests added for: AIAssistantInsertFactory simplified constructor, FocusCapabilityFactory, ComposablePasteDestination paste flow, VscodeAdapter paste methods, padding call sites, dest build pipeline - [x] pnpm test:release (VS Code host integration tests) — standard suite passes Closes #547
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)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCentralizes clipboard writes in ClipboardRouter (single write per operation); removes clipboard writes from insert factories; pre-applies smart padding at callers; introduces focus-to-paste and post-paste delays; simplifies paste APIs and wiring; updates ADR, tests, and QA cases accordingly. ChangesSingle Clipboard Write & Padding Refactor
Sequence Diagram (new flow): sequenceDiagram
participant User
participant Router as ClipboardRouter
participant Preserver as ClipboardPreserver
participant Dest as PasteDestination
participant Adapter as VscodeAdapter
User->>Router: copyAndSend(paddedText)
Router->>Preserver: preserve(currentClipboard)
Router->>Router: write paddedText to clipboard
Router->>Dest: sendFn(paddedText)
Dest->>Dest: focus via focusCommands
Dest->>Adapter: pasteTextFromClipboard()
Adapter->>Adapter: execute workbench.action.clipboard.paste (pre/post delays)
Adapter-->>Dest: pasteSucceeded (boolean)
Dest-->>Router: return pasteSucceeded
Router->>Preserver: maybe restore(previousClipboard) based on pasteSucceeded
Preserver-->>Router: restored
Router-->>User: done
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Possibly related PRs
🚥 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 |
|
The PR introduces significant changes to clipboard handling and paste operations, which affect user experience and functionality. These changes require additional QA test cases to ensure proper behavior. Suggested test cases:
Generated by QA Gap Check (GPT-4o-mini via GitHub Models) |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.ts (1)
30-413:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd logger assertions to these behavior tests.
These tests validate core behavior paths (focus + paste success/failure), but they don’t assert emitted logs. Please add explicit logger assertions in the same test cases so behavior and observability contracts are both covered.
As per coding guidelines, "Always include logger assertions in tests that verify method behavior—logging provides critical visibility for debugging; consolidate logging assertions with behavior tests rather than creating separate logging-only 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__/destinations/ComposablePasteDestination.integration.test.ts` around lines 30 - 413, Tests exercise ComposablePasteDestination.pasteLink with TerminalFocusCapability, AIAssistantFocusCapability and EditorFocusCapability but never assert logging; update each relevant test (those calling ComposablePasteDestination.createForTesting and destination.pasteLink) to assert mockLogger methods (e.g., mockLogger.info/error/debug) were called with expected messages such as the configured jumpSuccessMessage on success and an error message on failure, and verify call counts where appropriate (e.g., success path -> one info log with 'Focused ...', failure paths -> one error log); use the existing mockLogger instance passed into TerminalInsertFactory/AIAssistantInsertFactory/EditorInsertFactory to locate and add these assertions adjacent to existing behavior expects.
🧹 Nitpick comments (4)
packages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.ts (1)
740-762: ⚡ Quick winMerge the logging assertion into the main behavior test.
The logging check is currently isolated in a logging-only test; please fold it into the behavior test so behavior and observability are verified together.
As per coding guidelines, “Always include logger assertions in tests that verify method behavior… consolidate logging assertions with behavior tests rather than creating separate logging-only 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__/ide/vscode/VscodeAdapter.test.ts` around lines 740 - 762, Combine the logging assertion into the existing behavior test for pasteIntoTerminal: in the "should show terminal and execute paste command" test (which uses createMockTerminal and spies on adapter.executeCommand), add the expect for mockLogger.debug to assert it was called with { fn: 'VscodeAdapter.pasteIntoTerminal', terminalName: 'my-terminal' } and message 'Executing terminal paste command' (use a mockTerminal created with name 'my-terminal'), then remove the separate "should log terminal name before executing command" test; keep references to pasteIntoTerminal, adapter.executeCommand, mockTerminal, and mockLogger.debug when updating the test.packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/terminalInsertFactory.test.ts (1)
52-69: ⚡ Quick winInclude a logger assertion in the closure behavior test.
This behavior test verifies functional routing but omits logging assertions. Add at least one logger expectation (e.g., no error log) to keep behavior+observability covered together.
Proposed update
expect(pasteIntoTerminalSpy).toHaveBeenNthCalledWith(1, terminal1); expect(pasteIntoTerminalSpy).toHaveBeenNthCalledWith(2, terminal2); + expect(mockLogger.error).not.toHaveBeenCalled();As per coding guidelines, "Always include logger assertions in tests that verify method behavior—logging provides critical visibility for debugging; consolidate logging assertions with behavior tests rather than creating separate logging-only 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__/destinations/capabilities/insertFactories/terminalInsertFactory.test.ts` around lines 52 - 69, The test for TerminalInsertFactory closure behavior should also assert logging: after calling insertFn1 and insertFn2 verify the mockLogger was used appropriately (for example assert mockLogger.error was not called, or assert mockLogger.info/debug was called with the expected message), so update the test that creates TerminalInsertFactory and uses forTarget/insertFn1/insertFn2 to include at least one expectation on mockLogger (referencing mockLogger, TerminalInsertFactory, forTarget, pasteIntoTerminal, insertFn1/insertFn2) to cover observability alongside the existing routing assertions.packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/aiAssistantInsertFactory.test.ts (1)
13-24: ⚡ Quick winAdd logger assertion in the success-path behavior test.
This test validates method behavior but does not assert logging on the success path. Add a negative warning assertion (or explicit success-level assertion) to align with the test contract.
[Suggest patch]
Proposed update
expect(result).toBe(true); expect(pasteSpy).toHaveBeenCalled(); + expect(mockLogger.warn).not.toHaveBeenCalled();As per coding guidelines, "Always include logger assertions in tests that verify method behavior—logging provides critical visibility for debugging; consolidate logging assertions with behavior tests rather than creating separate logging-only 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__/destinations/capabilities/insertFactories/aiAssistantInsertFactory.test.ts` around lines 13 - 24, The test for AIAssistantInsertFactory.forTarget currently verifies pasteTextFromClipboard was called and returned true but omits logger assertions; update the test to assert logging on the success path by verifying the mockLogger used in the test (mockLogger) either received an explicit success/info call (e.g., mockLogger.info called with the expected success message) or that mockLogger.warn was not called, ensuring you reference AIAssistantInsertFactory, forTarget, and pasteTextFromClipboard in the change so the test both validates behavior and the expected logging outcome.packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts (1)
2103-2115: ⚡ Quick winUse the literal auto-paste result in the assertion.
toHaveBeenCalledWith(AutoPasteResult.Failure)ties this check to the enum symbol instead of the contract value the test is meant to freeze. Please assert the literal value here instead.As per coding guidelines, "In Jest expect() assertions (not test setup/mocks), use string literals for our own enum values, user-facing text, and config keys to freeze contracts; enum values ARE allowed in test setup for type safety".
🤖 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__/destinations/PasteDestinationManager.test.ts` around lines 2103 - 2115, The test currently asserts mockDest.getUserInstruction was called with the enum symbol AutoPasteResult.Failure; change that assertion to use the literal contract value the enum represents (i.e., replace toHaveBeenCalledWith(AutoPasteResult.Failure) with toHaveBeenCalledWith('<literal-value>') where '<literal-value>' is the exact string/primitive that AutoPasteResult.Failure maps to), keeping the rest of the test (mock setup of getUserInstruction, manager.boundDestination, and the isClipboardRestorationApplicable call) unchanged so the assertion freezes the contract rather than the enum symbol.
🤖 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 `@docs/ADR/0003-single-clipboard-write.md`:
- Around line 37-47: The fenced diagram block using triple backticks that begins
with the ASCII diagram should include a language tag to satisfy MD040; update
the opening fence from ``` to ```text (i.e., add the language specifier "text")
so the block becomes ```text ... ``` and leave the diagram content unchanged.
In `@packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml`:
- Line 708: Replace the unsupported key name focusPasteCommands in the Dummy AI
precondition with the suite's canonical custom-assistant config key (the same
key used throughout the YAML for custom assistants) so the "'Custom AI assistant
\"Dummy AI (Focus-Fail)\" configured..." line matches the actual config
contract; update the entry referenced by the clipboard-preservation-010
precondition to use that canonical key name.
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/sendFilePath.test.ts`:
- Around line 282-285: The test updates the global setting via
vscode.workspace.getConfiguration('rangelink').update('smartPadding.pasteFilePath',
'both', vscode.ConfigurationTarget.Global) but never restores it; capture the
previous value before calling update (const prev =
vscode.workspace.getConfiguration('rangelink').get('smartPadding.pasteFilePath'))
and restore it in the test teardown (use afterEach/after or a finally block) by
calling
vscode.workspace.getConfiguration('rangelink').update('smartPadding.pasteFilePath',
prev, vscode.ConfigurationTarget.Global) (or undefined to remove) so the global
state is not leaked to other tests.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/manualPasteInsertFactory.test.ts`:
- Around line 17-28: The test creates spies on createMockVscodeAdapter
(clipboardSpy, executeCommandSpy) but never injects that mock into the system
under test, so the assertions are non-proving; instead, instantiate or configure
ManualPasteInsertFactory to use the mocked adapter (e.g., pass mockAdapter into
the factory constructor or into ManualPasteInsertFactory.forTarget) so that the
spies on writeTextToClipboard and executeCommand are attached to the actual
adapter used by insertFn; target the symbols ManualPasteInsertFactory,
forTarget, createMockVscodeAdapter, writeTextToClipboard, and executeCommand so
the test calls insertFn(LINK_TEXT) using the injected mockAdapter and then
assert clipboardSpy/executeCommandSpy expectations.
In
`@packages/rangelink-vscode-extension/src/destinations/capabilities/insertFactories/terminalInsertFactory.ts`:
- Around line 3-5: Reorder the import statements to satisfy import/order: place
external packages first, then parent/internal paths, then sibling imports;
specifically import Logger from 'barebone-logger' first, then VscodeAdapter from
'../../../ide/vscode/VscodeAdapter', and finally InsertFactory from
'./InsertFactory' (keep all as type imports).
In `@packages/rangelink-vscode-extension/src/destinations/PasteDestination.ts`:
- Around line 73-103: The JSDoc for the paste contract is inconsistent: callers
now pre-populate the clipboard and destinations must focus before inserting, but
the implementation requirements still say "Focus destination after paste";
update the docs for both pasteLink (method pasteLink) and the corresponding
paste text method (e.g., pasteText) to require focusing the destination before
performing the paste (e.g., "Focus destination before paste (terminal.show(),
chat.open(), etc.)"), keep the other bullets (eligibility check, logging, return
false on failure/no throwing) unchanged, and ensure the wording clearly states
the focus-then-paste sequencing to avoid regressions.
In `@packages/rangelink-vscode-extension/src/services/ClipboardRouter.ts`:
- Around line 41-44: The code always passes true into
destinationManager.isClipboardRestorationApplicable, incorrectly assuming the
send succeeded; instead capture the actual boolean result from
executeCopyAndSend and use it when deciding restoration. Change the preserve
call so the first action runs executeCopyAndSend and returns its boolean result
(via the existing executeCopyAndSend function), and make the second predicate
call destinationManager.isClipboardRestorationApplicable(result) (or a closure
that references that result) so restoration policy reflects the real outcome.
---
Outside diff comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.ts`:
- Around line 30-413: Tests exercise ComposablePasteDestination.pasteLink with
TerminalFocusCapability, AIAssistantFocusCapability and EditorFocusCapability
but never assert logging; update each relevant test (those calling
ComposablePasteDestination.createForTesting and destination.pasteLink) to assert
mockLogger methods (e.g., mockLogger.info/error/debug) were called with expected
messages such as the configured jumpSuccessMessage on success and an error
message on failure, and verify call counts where appropriate (e.g., success path
-> one info log with 'Focused ...', failure paths -> one error log); use the
existing mockLogger instance passed into
TerminalInsertFactory/AIAssistantInsertFactory/EditorInsertFactory to locate and
add these assertions adjacent to existing behavior expects.
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/aiAssistantInsertFactory.test.ts`:
- Around line 13-24: The test for AIAssistantInsertFactory.forTarget currently
verifies pasteTextFromClipboard was called and returned true but omits logger
assertions; update the test to assert logging on the success path by verifying
the mockLogger used in the test (mockLogger) either received an explicit
success/info call (e.g., mockLogger.info called with the expected success
message) or that mockLogger.warn was not called, ensuring you reference
AIAssistantInsertFactory, forTarget, and pasteTextFromClipboard in the change so
the test both validates behavior and the expected logging outcome.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/terminalInsertFactory.test.ts`:
- Around line 52-69: The test for TerminalInsertFactory closure behavior should
also assert logging: after calling insertFn1 and insertFn2 verify the mockLogger
was used appropriately (for example assert mockLogger.error was not called, or
assert mockLogger.info/debug was called with the expected message), so update
the test that creates TerminalInsertFactory and uses
forTarget/insertFn1/insertFn2 to include at least one expectation on mockLogger
(referencing mockLogger, TerminalInsertFactory, forTarget, pasteIntoTerminal,
insertFn1/insertFn2) to cover observability alongside the existing routing
assertions.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.ts`:
- Around line 2103-2115: The test currently asserts mockDest.getUserInstruction
was called with the enum symbol AutoPasteResult.Failure; change that assertion
to use the literal contract value the enum represents (i.e., replace
toHaveBeenCalledWith(AutoPasteResult.Failure) with
toHaveBeenCalledWith('<literal-value>') where '<literal-value>' is the exact
string/primitive that AutoPasteResult.Failure maps to), keeping the rest of the
test (mock setup of getUserInstruction, manager.boundDestination, and the
isClipboardRestorationApplicable call) unchanged so the assertion freezes the
contract rather than the enum symbol.
In
`@packages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.ts`:
- Around line 740-762: Combine the logging assertion into the existing behavior
test for pasteIntoTerminal: in the "should show terminal and execute paste
command" test (which uses createMockTerminal and spies on
adapter.executeCommand), add the expect for mockLogger.debug to assert it was
called with { fn: 'VscodeAdapter.pasteIntoTerminal', terminalName: 'my-terminal'
} and message 'Executing terminal paste command' (use a mockTerminal created
with name 'my-terminal'), then remove the separate "should log terminal name
before executing command" test; keep references to pasteIntoTerminal,
adapter.executeCommand, mockTerminal, and mockLogger.debug when updating the
test.
🪄 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: ac5bd727-a9fc-479a-bff1-c83514598741
📒 Files selected for processing (36)
docs/ADR/0003-single-clipboard-write.mddocs/ADR/README.mdpackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yamlpackages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/customAiAssistants.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/sendFilePath.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/DestinationRegistry.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/FocusCapabilityFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/aiAssistantInsertFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/manualPasteInsertFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/terminalInsertFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/helpers/destinationTestHelpers.tspackages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.tspackages/rangelink-vscode-extension/src/bookmarks/BookmarkService.tspackages/rangelink-vscode-extension/src/bookmarks/__tests__/BookmarkService.test.tspackages/rangelink-vscode-extension/src/constants/chatPasteConstants.tspackages/rangelink-vscode-extension/src/constants/index.tspackages/rangelink-vscode-extension/src/constants/pasteTimingConstants.tspackages/rangelink-vscode-extension/src/createWiringServices.tspackages/rangelink-vscode-extension/src/destinations/ComposablePasteDestination.tspackages/rangelink-vscode-extension/src/destinations/PasteDestination.tspackages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.tspackages/rangelink-vscode-extension/src/destinations/capabilities/FocusCapabilityFactory.tspackages/rangelink-vscode-extension/src/destinations/capabilities/insertFactories/aiAssistantInsertFactory.tspackages/rangelink-vscode-extension/src/destinations/capabilities/insertFactories/manualPasteInsertFactory.tspackages/rangelink-vscode-extension/src/destinations/capabilities/insertFactories/terminalInsertFactory.tspackages/rangelink-vscode-extension/src/destinations/destinationBuilders.tspackages/rangelink-vscode-extension/src/ide/vscode/VscodeAdapter.tspackages/rangelink-vscode-extension/src/services/ClipboardRouter.tspackages/rangelink-vscode-extension/src/services/FilePathPaster.tspackages/rangelink-vscode-extension/src/services/LinkGenerator.tspackages/rangelink-vscode-extension/src/services/TerminalSelectionService.tspackages/rangelink-vscode-extension/src/services/TextSelectionPaster.ts
💤 Files with no reviewable changes (3)
- packages/rangelink-vscode-extension/src/constants/chatPasteConstants.ts
- packages/rangelink-vscode-extension/src/bookmarks/BookmarkService.ts
- packages/rangelink-vscode-extension/src/bookmarks/tests/BookmarkService.test.ts
Comment on lines
+282
to
285
| await vscode.workspace | ||
| .getConfiguration('rangelink') | ||
| .update('smartPadding.pasteFilePath', 'both', vscode.ConfigurationTarget.Global); | ||
|
|
Contributor
There was a problem hiding this comment.
Reset smartPadding.pasteFilePath after this test to avoid global-state leakage.
Line 282 updates a global setting but the suite teardown doesn’t restore it, so later tests can inherit 'both' and become flaky/order-dependent.
🤖 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/sendFilePath.test.ts`
around lines 282 - 285, The test updates the global setting via
vscode.workspace.getConfiguration('rangelink').update('smartPadding.pasteFilePath',
'both', vscode.ConfigurationTarget.Global) but never restores it; capture the
previous value before calling update (const prev =
vscode.workspace.getConfiguration('rangelink').get('smartPadding.pasteFilePath'))
and restore it in the test teardown (use afterEach/after or a finally block) by
calling
vscode.workspace.getConfiguration('rangelink').update('smartPadding.pasteFilePath',
prev, vscode.ConfigurationTarget.Global) (or undefined to remove) so the global
state is not leaked to other tests.
…ts, correct docs Fixed the clipboard preservation guard so it receives the actual paste outcome instead of hardcoded `true`, preventing stale clipboard restoration on failed pastes. Added logger assertions to integration tests, updated enum references to string literals per T003, and fixed a minor docs issue. Benefits: - Clipboard is only restored when paste actually succeeded — failed pastes no longer inadvertently overwrite user clipboard - Integration test logger assertions provide regression protection for diagnostic logging - ADR code fence now has correct language tag for proper rendering at docs.rangelink.io - QA YAML uses renamed `focusCommands` field, matching current code Ignored Feedback: - Adding `settingsHelpers.resetRangelinkSettings` to send-file-path integration test: the `standardSuite.setup()` wrapper already calls `resetRangelinkSettings()` before every test in the suite, so the explicit call is redundant Ref: #556 (review)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/aiAssistantInsertFactory.test.ts (1)
23-23: ⚡ Quick winPrefer
.toHaveBeenCalledWith()to explicitly assert no parameters.Using
.toHaveBeenCalledWith()instead of.toHaveBeenCalled()makes the expectation explicit thatpasteTextFromClipboardis called with no arguments, improving clarity and robustness to signature changes.♻️ Suggested improvement
- expect(pasteSpy).toHaveBeenCalled(); + expect(pasteSpy).toHaveBeenCalledWith();🤖 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__/destinations/capabilities/insertFactories/aiAssistantInsertFactory.test.ts` at line 23, Replace the generic call assertion in the test with an explicit no-arguments assertion: update the expectation that verifies pasteTextFromClipboard (the spy referenced as pasteSpy in aiAssistantInsertFactory.test.ts) was invoked to use toHaveBeenCalledWith() with no parameters so the test asserts the function was called with an empty argument list rather than just being called.
🤖 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.
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/aiAssistantInsertFactory.test.ts`:
- Line 23: Replace the generic call assertion in the test with an explicit
no-arguments assertion: update the expectation that verifies
pasteTextFromClipboard (the spy referenced as pasteSpy in
aiAssistantInsertFactory.test.ts) was invoked to use toHaveBeenCalledWith() with
no parameters so the test asserts the function was called with an empty argument
list rather than just being called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04bd6eac-618d-42e1-83a1-6b01ba1d9ec0
📒 Files selected for processing (11)
docs/ADR/0003-single-clipboard-write.mdpackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yamlpackages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/aiAssistantInsertFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/manualPasteInsertFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/terminalInsertFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.tspackages/rangelink-vscode-extension/src/destinations/PasteDestination.tspackages/rangelink-vscode-extension/src/destinations/capabilities/insertFactories/terminalInsertFactory.tspackages/rangelink-vscode-extension/src/services/ClipboardRouter.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rangelink-vscode-extension/src/destinations/PasteDestination.ts
- packages/rangelink-vscode-extension/src/tests/ide/vscode/VscodeAdapter.test.ts
- packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml
- packages/rangelink-vscode-extension/src/tests/destinations/PasteDestinationManager.test.ts
- packages/rangelink-vscode-extension/src/tests/destinations/capabilities/insertFactories/terminalInsertFactory.test.ts
- packages/rangelink-vscode-extension/src/tests/destinations/ComposablePasteDestination.integration.test.ts
…istant paste regression The single-clipboard-write refactoring accidentally dropped FOCUS_TO_PASTE_DELAY_MS from the AI assistant paste path. The old aiAssistantInsertFactory had this 200ms delay between clipboard write and paste command execution — it gave the Electron clipboard write time to propagate from the extension host process through the main process to the webview's renderer. Without it, editor.action.clipboardPasteAction fires before the webview can read the clipboard content, and the paste lands empty even though all code-side signals report success. Extracted a delay(ms) method in VscodeAdapter that wraps setTimeout. pasteTextFromClipboard calls this.delay(FOCUS_TO_PASTE_DELAY_MS) before executing the paste command, then this.delay(postPasteDelayMs) after a successful paste. Tests spy on delay() via jest.spyOn to verify the delays are enforced in the correct order without fighting fake timers. Benefits: - Claude Code, Cursor AI, GitHub Copilot Chat, and custom AI assistants now reliably receive pasted content - Pre-paste delay (FOCUS_TO_PASTE_DELAY_MS, 200ms) ensures clipboard propagation across Electron IPC boundary - Post-paste delay (CLIPBOARD_POST_PASTE_DELAY_MS, 200ms) remains intact for clipboard restoration safety - delay() method keeps production code clean (no testing parameters); tests spy on it for deterministic verification
…ion in AIAssistantInsertFactory test .toHaveBeenCalled() only asserts the spy was invoked. .toHaveBeenCalledWith() asserts it was called with an empty argument list, which is stricter and catches regressions where the signature changes to pass unintended arguments. Benefits: - Explicit no-arguments contract for pasteTextFromClipboard call site - Stricter assertion catches unintended signature drift Ref: #556 (review)
Owner
Author
|
@coderabbitai review - |
Contributor
|
The Comment image shows the GitHub PR checks panel with "CodeRabbit — Review skipped" listed alongside two passing CI checks ("CI / Test & Validate" and "QA Coverage Gap Check"). All checks have passed and there are no merge conflicts.
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai Do you see #556 (comment) as actionable ? If so, can you create a new GitHub issue on this repo with a detailed plan on how you'd proceed with the implementation of those detected gaps ? |
5 tasks
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me first fetch the content of that comment. 🏁 Script executed: Length of output: 923 Yes, the comment is actionable. It was posted by the QA Gap Check bot and identifies four concrete test gaps stemming from the single-clipboard-write refactor in this PR. Let me create the issue now. 🏁 Script executed: Length of output: 48
I've created issue
|
5 tasks
couimet
added a commit
that referenced
this pull request
May 14, 2026
…llback for AI assistant webview delivery ## Summary The "single clipboard write per R-* operation" refactor (PR #556, `issues/547`) inadvertently reduced a three-command paste fallback chain to a single `editor.action.clipboardPasteAction`. That command only reaches VS Code text editors — for webview-hosted chat inputs (Claude Code, Cursor AI, GitHub Copilot Chat, custom AI Tier 1/2 webview targets) the system-level fallbacks `execPaste` / `paste` were what actually delivered content. Without them the dispatch returned success, but no content arrived in the chat input. This PR restores the fallback chain, logs the successful command for diagnostics, and converts the silent-pass `claude-code-004/005` tests to verdict-based assertions so the same class of regression cannot land green in the future. ## Changes - Renamed `src/constants/pasteTimingConstants.ts` → `aiAssistantPasteConstants.ts` and added `AI_ASSISTANT_PASTE_COMMANDS = ['editor.action.clipboardPasteAction', 'execPaste', 'paste']` alongside the existing delay constants. The file now correctly reflects its scope (was a name-vs-content drift from #556 that removed the commands). - `VscodeAdapter.pasteTextFromClipboard()` now iterates `AI_ASSISTANT_PASTE_COMMANDS` and stops on the first success. The `command` field is included in the `Clipboard paste succeeded` log so tests can assert which command delivered the paste. Per-command failures log at `debug` with `Paste command failed, trying next`; an all-failed path logs at `warn` with `allCommandsFailed: true`. - Inline JSDoc on `pasteTextFromClipboard` explains WHY the fallback chain exists — non-obvious enough that the next refactor would otherwise repeat the same mistake. - Added `assertPasteCommandLogged(lines, { command })` helper to `logBasedUiAssertions.ts` for integration-test assertions on which paste command succeeded. - Updated `VscodeAdapter.test.ts` `pasteTextFromClipboard` describe block: 5 tests now cover the fallback chain (first-success, fall-through-to-second, all-fail, default post-paste delay, custom post-paste delay). All assertions include the `command` field per the test contract. - Converted `claude-code-004` and `claude-code-005` from `waitForHuman` to `waitForHumanVerdict`. The silent-pass trap is closed for these specific TCs (broader audit tracked in #560). Setup is now programmatic (file open + selection via `vscode.workspace.openTextDocument` + `editor.selection`) and the send fires via `executeCommand(CMD_COPY_LINK_RELATIVE)` rather than asking the human to press the chord. - Added the wave-1 Claude Code TCs from #483: `claude-code-001` (automated picker-inspection in `standardSuite`, uses `openAndDismiss` from #561), `claude-code-002` (assisted send-delivery with `waitForHumanVerdict`), and the deletion of `claude-code-003` (superseded by `claude-code-005`'s richer warm-path scenario). YAML flips for 001 → `automated: true` and 002 → `automated: assisted`. ## Key Discoveries - The 547 refactor's "Test Plan" claimed `pnpm test:release passes` — technically true, but the with-extensions tests were not in that baseline AND `claude-code-004/005` used `waitForHuman` (Cancel-to-resolve) rather than `waitForHumanVerdict` (PASS/FAIL-to-resolve). Code-side log assertions stayed green even though the webview received nothing. The regression therefore landed undetected. The verdict-helper conversion in this PR closes that specific silent-pass trap; the broader audit lives in #560. - The `dummy-ai-extension` fixture is already webview-based and exposes `dummyAi.getText` for reading what landed in the webview textareas — the missing piece for a fully automated webview paste-delivery test. Adding one is blocked on a new programmatic bind command for custom AI assistants (the current bind path requires a human picker selection); that command is out of scope here and will be filed as a follow-up issue. - The 547 paste pipeline was audited beyond the bug-fix change: padding is correctly pre-applied at all 4 call sites (`LinkGenerator`, `TextSelectionPaster`, `FilePathPaster`, `TerminalSelectionService`); the two-delay model is used consistently in the single `pasteTextFromClipboard` chokepoint; the `focusCommands` fallback iteration in `AIAssistantFocusCapability` is intact. No additional regressions found. ## Test Plan - [x] All existing unit tests pass — 107 suites, 228 tests in `VscodeAdapter.test.ts` (including 5 new fallback tests), 98.42% statement coverage. - [x] `pnpm test:release:automated` — 165 of 175 passing. The 10 failures are pre-existing flakes (`langswitch-binding-*`, `smart-padding-*`, `stale-viewcolumn-001`) on non-AI-assistant code paths, same as noted in PR #561's description. My changes don't touch terminal/editor paste paths. - [x] QA coverage validator passes — 121 automated / 115 assisted / 12 false markers all align with integration tests. - [ ] **Assisted verification (definitive bug-fix proof):** `pnpm test:release:with-extensions --grep "claude-code-00[245]"` runs `claude-code-002` (cleanest reproduction), `claude-code-004` (cold panel paste), and `claude-code-005` (cold→warm sequence). Each prompts PASS/FAIL on whether the RangeLink actually arrived in Claude Code's chat input. All three must produce PASS verdicts. - [ ] CI `test-with-extensions` job (from #564) runs green on the PR. ## Related - Closes #559 - Resolves the regression introduced by #556 - Companion to #560 (silent-pass-trap audit) — this PR fixes the 547-introduced subset (`claude-code-004/005`); the broader audit remains open. - Folds in the Wave-1 Claude Code work from #483 (`claude-code-001` / `claude-code-002` integration tests + the `claude-code-003` deletion). - Complements #563 — this PR fixes the bug; #563 backfills coverage that would have caught it. They land independently; whichever merges first, the other rebases. - Follow-up issue to be filed: a programmatic bind command for custom AI assistants (currently the bind path requires a human picker selection), which would unblock a fully automated webview paste-delivery test using the dummy-ai-extension's `dummyAi.getText` reader.
couimet
added a commit
that referenced
this pull request
May 14, 2026
…llback for AI assistant webview delivery (#566) ## Summary The "single clipboard write per R-* operation" refactor (PR #556, `issues/547`) inadvertently reduced a three-command paste fallback chain to a single `editor.action.clipboardPasteAction`. That command only reaches VS Code text editors — for webview-hosted chat inputs (Claude Code, Cursor AI, GitHub Copilot Chat, custom AI Tier 1/2 webview targets) the system-level fallbacks `execPaste` / `paste` were what actually delivered content. Without them the dispatch returned success, but no content arrived in the chat input. This PR restores the fallback chain, logs the successful command for diagnostics, and converts the silent-pass `claude-code-004/005` tests to verdict-based assertions so the same class of regression cannot land green in the future. ## Changes - Renamed `src/constants/pasteTimingConstants.ts` → `aiAssistantPasteConstants.ts` and added `AI_ASSISTANT_PASTE_COMMANDS = ['editor.action.clipboardPasteAction', 'execPaste', 'paste']` alongside the existing delay constants. The file now correctly reflects its scope (was a name-vs-content drift from #556 that removed the commands). - `VscodeAdapter.pasteTextFromClipboard()` now iterates `AI_ASSISTANT_PASTE_COMMANDS` and stops on the first success. The `command` field is included in the `Clipboard paste succeeded` log so tests can assert which command delivered the paste. Per-command failures log at `debug` with `Paste command failed, trying next`; an all-failed path logs at `warn` with `allCommandsFailed: true`. - Inline JSDoc on `pasteTextFromClipboard` explains WHY the fallback chain exists — non-obvious enough that the next refactor would otherwise repeat the same mistake. - Added `assertPasteCommandLogged(lines, { command })` helper to `logBasedUiAssertions.ts` for integration-test assertions on which paste command succeeded. - Updated `VscodeAdapter.test.ts` `pasteTextFromClipboard` describe block: 5 tests now cover the fallback chain (first-success, fall-through-to-second, all-fail, default post-paste delay, custom post-paste delay). All assertions include the `command` field per the test contract. - Converted `claude-code-004` and `claude-code-005` from `waitForHuman` to `waitForHumanVerdict`. The silent-pass trap is closed for these specific TCs (broader audit tracked in #560). Setup is now programmatic (file open + selection via `vscode.workspace.openTextDocument` + `editor.selection`) and the send fires via `executeCommand(CMD_COPY_LINK_RELATIVE)` rather than asking the human to press the chord. - Added the wave-1 Claude Code TCs from #483: `claude-code-001` (automated picker-inspection in `standardSuite`, uses `openAndDismiss` from #561), `claude-code-002` (assisted send-delivery with `waitForHumanVerdict`), and the deletion of `claude-code-003` (superseded by `claude-code-005`'s richer warm-path scenario). YAML flips for 001 → `automated: true` and 002 → `automated: assisted`. ## Key Discoveries - The 547 refactor's "Test Plan" claimed `pnpm test:release passes` — technically true, but the with-extensions tests were not in that baseline AND `claude-code-004/005` used `waitForHuman` (Cancel-to-resolve) rather than `waitForHumanVerdict` (PASS/FAIL-to-resolve). Code-side log assertions stayed green even though the webview received nothing. The regression therefore landed undetected. The verdict-helper conversion in this PR closes that specific silent-pass trap; the broader audit lives in #560. - The `dummy-ai-extension` fixture is already webview-based and exposes `dummyAi.getText` for reading what landed in the webview textareas — the missing piece for a fully automated webview paste-delivery test. Adding one is blocked on a new programmatic bind command for custom AI assistants (the current bind path requires a human picker selection); that command is out of scope here and will be filed as a follow-up issue. - The 547 paste pipeline was audited beyond the bug-fix change: padding is correctly pre-applied at all 4 call sites (`LinkGenerator`, `TextSelectionPaster`, `FilePathPaster`, `TerminalSelectionService`); the two-delay model is used consistently in the single `pasteTextFromClipboard` chokepoint; the `focusCommands` fallback iteration in `AIAssistantFocusCapability` is intact. No additional regressions found. ## Test Plan - [x] All existing unit tests pass — 107 suites, 228 tests in `VscodeAdapter.test.ts` (including 5 new fallback tests), 98.42% statement coverage. - [x] `pnpm test:release:automated` — 165 of 175 passing. The 10 failures are pre-existing flakes (`langswitch-binding-*`, `smart-padding-*`, `stale-viewcolumn-001`) on non-AI-assistant code paths, same as noted in PR #561's description. My changes don't touch terminal/editor paste paths. - [x] QA coverage validator passes — 121 automated / 115 assisted / 12 false markers all align with integration tests. - [ ] **Assisted verification (definitive bug-fix proof):** `pnpm test:release:with-extensions --grep "claude-code-00[245]"` runs `claude-code-002` (cleanest reproduction), `claude-code-004` (cold panel paste), and `claude-code-005` (cold→warm sequence). Each prompts PASS/FAIL on whether the RangeLink actually arrived in Claude Code's chat input. All three must produce PASS verdicts. - [ ] CI `test-with-extensions` job (from #564) runs green on the PR. ## Related - Closes #559 - Resolves the regression introduced by #556 - Companion to #560 (silent-pass-trap audit) — this PR fixes the 547-introduced subset (`claude-code-004/005`); the broader audit remains open. - Folds in the Wave-1 Claude Code work from #483 (`claude-code-001` / `claude-code-002` integration tests + the `claude-code-003` deletion). - Complements #563 — this PR fixes the bug; #563 backfills coverage that would have caught it. They land independently; whichever merges first, the other rebases. - Follow-up issue to be filed: a programmatic bind command for custom AI assistants (currently the bind path requires a human picker selection), which would unblock a fully automated webview paste-delivery test using the dummy-ai-extension's `dummyAi.getText` reader.
5 tasks
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
Architectural refactoring that gives ClipboardRouter sole ownership of the clipboard write. Insert factories now only execute paste commands — never touch the clipboard. Eliminates double clipboard writes, nested ClipboardPreservers, and double padding risk. No user-visible behavior changes.
Changes
QA & Tests
Test Plan
Closes #547
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests