[issues/621] Merge bind+send status bar messages into a single notification#667
Merged
Conversation
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 (31)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (22)
WalkthroughIntroduces a ChangesMerged bind+send status bar notification
Sequence Diagram(s)sequenceDiagram
participant User
participant Service as FilePathPaster / TextSelectionPaster / LinkGenerator
participant SendRouter
participant PasteDestinationManager
participant OperationFeedbackProvider
participant StatusBar
User->>Service: paste/send command (no destination bound)
Service->>SendRouter: resolveDestination(logCtx)
SendRouter->>SendRouter: showPickerAndBind() — user selects terminal
SendRouter->>PasteDestinationManager: bind(options, { skipMessage: true })
PasteDestinationManager-->>SendRouter: BindSuccessInfo (notifyBound suppressed)
SendRouter-->>Service: ResolveResult { canProceed: true, bindPerformed: true, destinationName }
Service->>Service: toBindContext(resolveResult) → BindContext
Service->>SendRouter: sendToDestination(payload, bindContext)
SendRouter->>OperationFeedbackProvider: provideSendFeedback(ctx, outcome, bindContext)
OperationFeedbackProvider->>StatusBar: "✓ RangeLink: Bound to Terminal (name) — File path sent"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ QA Coverage OK The PR introduces a new single status bar message format for binding and sending in one step, and fixes the R-F command to work with various active tabs. New test cases have been added to the QA YAML to cover these changes. Generated by QA Gap Check (GPT-4o-mini via GitHub Models) |
❌ CI / Integration Tests (with overrides) — run summaryDuration: 0m 48s QA TC IDs: 2 exercised across 0 features Report: View run & artifacts Reproduce locally: Feature breakdown
To re-run failed tests: |
❌ CI / Integration Tests (automated) — run summaryDuration: 1m 6s \342\232\240\357\270\217 Integration test report missing: Runner may have crashed QA TC IDs: 163 exercised across 0 features Report: View run & artifacts Reproduce locally: Feature breakdown
|
❌ CI / Integration Tests (with extensions) — run summaryDuration: 1m 5s Unit tests: Ran in separate Test & Validate job \342\232\240\357\270\217 Integration test report missing: Runner may have crashed QA TC IDs: 195 exercised across 0 features Report: View run & artifacts Reproduce locally: Feature breakdown
|
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rangelink-vscode-extension/CHANGELOG.md`:
- Line 14: Add QA test cases to the qa-test-cases-*.yaml files to cover the
merged bind+send status bar notification behavior. Create test cases with schema
fields (id, feature: status_bar_feedback, scenario, preconditions, steps,
expected_result, automated) for four scenarios: unbound picker send
(R-V/R-L/R-F) producing exactly one merged message in the format "Bound to
<destination> — <link> sent", already-bound send preserving the existing
send-only message behavior, bind-only action preserving the bind-only message,
and rebind paths ensuring no send text appears in bind-only contexts. Use id
naming like qa-bind-send-merged-001, qa-bind-send-merged-002, etc. for each test
case.
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuEditorTab.test.ts`:
- Around line 27-29: The test assertions for already-bound terminal flows are
expecting the wrong status bar message. In scenarios where the terminal is
pre-bound using CMD_BIND_TO_TERMINAL_HERE, the expectStatusBarMessages assertion
should expect send-only feedback messages, not combined bind+send messages.
Update the expectStatusBarMessages calls in both test cases (the one at lines
27-29 and the one at lines 76-78) to assert for the appropriate send-only
feedback message instead of the merged "Bound to Terminal... — File path sent"
message, which should only appear in fresh-bind scenarios.
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuExplorer.test.ts`:
- Around line 28-30: The expectStatusBarMessages assertion in the test is
validating the wrong status message. Since the terminal is already bound before
the send action occurs, the status bar should only display the send-only
message, not the combined bind-and-send message. Update the
expectStatusBarMessages call around lines 28-30 to assert only the send-only
status text instead of the combined "Bound to Terminal ... — File path sent"
message. Also apply the same fix to the similar assertion mentioned at lines
74-76.
🪄 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: b3ac3e4d-619f-4814-ba59-7edda64f7f2b
📒 Files selected for processing (31)
packages/rangelink-vscode-extension/CHANGELOG.mdpackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuEditorTab.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuExplorer.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuTerminal.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/customAiAssistants.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/dirtyBufferWarning.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/PasteDestinationManager.test.tspackages/rangelink-vscode-extension/src/__tests__/feedback/OperationFeedbackProvider.test.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockOperationFeedbackProvider.tspackages/rangelink-vscode-extension/src/__tests__/services/FilePathPaster.test.tspackages/rangelink-vscode-extension/src/__tests__/services/LinkGenerator.test.tspackages/rangelink-vscode-extension/src/__tests__/services/SendRouter.test.tspackages/rangelink-vscode-extension/src/__tests__/services/TerminalSelectionService.test.tspackages/rangelink-vscode-extension/src/__tests__/services/TextSelectionPaster.test.tspackages/rangelink-vscode-extension/src/__tests__/services/toBindContext.test.tspackages/rangelink-vscode-extension/src/destinations/DestinationBinder.tspackages/rangelink-vscode-extension/src/destinations/PasteDestinationManager.tspackages/rangelink-vscode-extension/src/feedback/BindingFeedback.tspackages/rangelink-vscode-extension/src/feedback/OperationFeedbackProvider.tspackages/rangelink-vscode-extension/src/i18n/messages.en.tspackages/rangelink-vscode-extension/src/services/FilePathPaster.tspackages/rangelink-vscode-extension/src/services/LinkGenerator.tspackages/rangelink-vscode-extension/src/services/SendRouter.tspackages/rangelink-vscode-extension/src/services/TerminalSelectionService.tspackages/rangelink-vscode-extension/src/services/TextSelectionPaster.tspackages/rangelink-vscode-extension/src/services/toBindContext.tspackages/rangelink-vscode-extension/src/services/types/ResolveResult.tspackages/rangelink-vscode-extension/src/services/types/index.tspackages/rangelink-vscode-extension/src/types/BindContext.tspackages/rangelink-vscode-extension/src/types/MessageCode.tspackages/rangelink-vscode-extension/src/types/index.ts
|
|
||
| ### Changed | ||
|
|
||
| - **Single status bar message when binding and sending in one step via the destination picker**, instead of two back-to-back messages. The merged message reads "Bound to <destination> — <link> sent". (#621) |
Contributor
There was a problem hiding this comment.
Potential QA coverage gap for the merged bind+send UX
This PR changes user-visible notification behavior, but no QA YAML updates are present in the reviewed changes. Please add/confirm QA cases in packages/rangelink-vscode-extension/qa/qa-test-cases-*.yaml for:
- unbound picker send (R-V/R-L/R-F) → exactly one merged status bar message,
- already-bound send → existing send-only behavior preserved,
- bind-only action → bind-only message preserved,
- rebind paths → no merged send text leakage.
Suggested schema examples:
id: qa-bind-send-merged-001,feature: status_bar_feedback,scenario: unbound picker send merges bind+send,preconditions,steps,expected_result,automated.id: qa-bind-send-merged-002,feature: status_bar_feedback,scenario: already-bound send remains send-only, ...
As per coding guidelines, "When reviewing changes in the VS Code extension source code... Flag a potential QA coverage gap if ... user-visible behavior changes and QA YAML is not updated."
🤖 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/CHANGELOG.md` at line 14, Add QA test
cases to the qa-test-cases-*.yaml files to cover the merged bind+send status bar
notification behavior. Create test cases with schema fields (id, feature:
status_bar_feedback, scenario, preconditions, steps, expected_result, automated)
for four scenarios: unbound picker send (R-V/R-L/R-F) producing exactly one
merged message in the format "Bound to <destination> — <link> sent",
already-bound send preserving the existing send-only message behavior, bind-only
action preserving the bind-only message, and rebind paths ensuring no send text
appears in bind-only contexts. Use id naming like qa-bind-send-merged-001,
qa-bind-send-merged-002, etc. for each test case.
Source: Coding guidelines
Comment on lines
27
to
29
| ss.expectStatusBarMessages([ | ||
| '✓ RangeLink: Bound to Terminal ("rl-ctxmenu-tab-001")', | ||
| '✓ RangeLink: File path sent to Terminal ("rl-ctxmenu-tab-001")', | ||
| '✓ RangeLink: Bound to Terminal ("rl-ctxmenu-tab-001") — File path sent', | ||
| ]); |
Contributor
There was a problem hiding this comment.
Already-bound scenarios should assert send-only feedback, not merged bind+send
Both tests pre-bind via CMD_BIND_TO_TERMINAL_HERE before triggering “Send File Path”. That makes these already-bound flows, so expecting Bound to ... — File path sent likely contradicts the intended contract for non-picker sends.
Suggested assertion adjustment
- ss.expectStatusBarMessages([
- '✓ RangeLink: Bound to Terminal ("rl-ctxmenu-tab-001") — File path sent',
- ]);
+ ss.expectStatusBarMessages([
+ '✓ RangeLink: File path sent to Terminal ("rl-ctxmenu-tab-001")',
+ ]);
- ss.expectStatusBarMessages([
- '✓ RangeLink: Bound to Terminal ("rl-ctxmenu-tab-002") — File path sent',
- ]);
+ ss.expectStatusBarMessages([
+ '✓ RangeLink: File path sent to Terminal ("rl-ctxmenu-tab-002")',
+ ]);Also applies to: 76-78
🤖 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/contextMenuEditorTab.test.ts`
around lines 27 - 29, The test assertions for already-bound terminal flows are
expecting the wrong status bar message. In scenarios where the terminal is
pre-bound using CMD_BIND_TO_TERMINAL_HERE, the expectStatusBarMessages assertion
should expect send-only feedback messages, not combined bind+send messages.
Update the expectStatusBarMessages calls in both test cases (the one at lines
27-29 and the one at lines 76-78) to assert for the appropriate send-only
feedback message instead of the merged "Bound to Terminal... — File path sent"
message, which should only appear in fresh-bind scenarios.
Comment on lines
28
to
30
| ss.expectStatusBarMessages([ | ||
| '✓ RangeLink: Bound to Terminal ("rl-ctxmenu-exp-001")', | ||
| '✓ RangeLink: File path sent to Terminal ("rl-ctxmenu-exp-001")', | ||
| '✓ RangeLink: Bound to Terminal ("rl-ctxmenu-exp-001") — File path sent', | ||
| ]); |
Contributor
There was a problem hiding this comment.
These Explorer tests also appear to assert the wrong message contract for pre-bound sends
Since the terminal is bound before the send action, these cases should validate send-only status text. The merged Bound to ... — File path sent expectation is generally for picker-bind+send in one flow.
Suggested assertion adjustment
- ss.expectStatusBarMessages([
- '✓ RangeLink: Bound to Terminal ("rl-ctxmenu-exp-001") — File path sent',
- ]);
+ ss.expectStatusBarMessages([
+ '✓ RangeLink: File path sent to Terminal ("rl-ctxmenu-exp-001")',
+ ]);
- ss.expectStatusBarMessages([
- '✓ RangeLink: Bound to Terminal ("rl-ctxmenu-exp-002") — File path sent',
- ]);
+ ss.expectStatusBarMessages([
+ '✓ RangeLink: File path sent to Terminal ("rl-ctxmenu-exp-002")',
+ ]);Also applies to: 74-76
🤖 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/contextMenuExplorer.test.ts`
around lines 28 - 30, The expectStatusBarMessages assertion in the test is
validating the wrong status message. Since the terminal is already bound before
the send action occurs, the status bar should only display the send-only
message, not the combined bind-and-send message. Update the
expectStatusBarMessages call around lines 28-30 to assert only the send-only
status text instead of the combined "Bound to Terminal ... — File path sent"
message. Also apply the same fix to the similar assertion mentioned at lines
74-76.
…cation
When no destination is bound and the picker resolves to a bind during a send operation (R-V, R-L, R-F), the user sees one status bar message instead of two back-to-back toasts. The merge is achieved by threading bind context through the feedback layer, introducing a `ResolveResult` discriminated union, and adding a `BindContext` type shared across senders.
- Destination picker send flows now produce a single merged status bar message ("Bound to Terminal — RangeLink sent") instead of separate bind and send toasts. The bind-only toast from `showPickerAndBind()` is suppressed via the existing `StatusBarOptions.skipMessage` flag.
- `ResolveResult` discriminated union replaces the boolean return of `resolveDestination()`, encoding whether a bind was performed and the destination name when applicable. Callers no longer need to infer bind state from a bare boolean.
- New `BindContext` type (`{ readonly destinationName: string }`) shared across `SendRouter`, `OperationFeedbackProvider`, `LinkGenerator`, and four sender services, replacing an inline `{ destinationName: string }` type duplicated at each call site.
- Standalone `toBindContext(result: ResolveResult)` utility extracts `BindContext` from the discriminated union, eliminating a 4× copy-pasted ternary.
- `BindingFeedback` interface gains `notifyRebound(newDestinationName, previousDestinationName)` as a first-class method, splitting the rebind notification from `notifyBound(destinationName)`. The `replacedName` optional param is removed.
- `commitBind` options restructured: `suppressAutoPaste` and `StatusBarOptions` are nested under a single `bindOptions` parameter, keeping bind behavior and UI feedback concerns separated.
- CI: CI action for install-deps added.
- The `replacedName` path in `notifyBound` was the only place where rebind and fresh-bind messages could be confused. Splitting into `notifyRebound` eliminates that ambiguity and structurally guarantees rebind messages never leak into the send-feedback path.
- The four senders (`TextSelectionPaster`, `LinkGenerator`, `FilePathPaster`, `TerminalSelectionService`) had nearly identical `resolveResult → bindContext` construction logic. Extracting `toBindContext` as a standalone utility with its own test file (100% branch coverage) cleaned up the duplication.
- [x] All 2099 existing tests pass
- [x] New unit tests: `toBindContext.test.ts` (3 tests, 100% branch), `OperationFeedbackProvider.test.ts` (+11 tests covering `notifyBound`, `notifyRebound`, `notifyAlreadyBound`, `notifyBindFailedEditor`, `notifyBindFailedNotAvailable`, `notifyBackgroundTabOpened`, `notifyUnbound`, `notifyNothingToUnbind`, `notifyJumpFocused`, `notifyJumpFailed`), `PasteDestinationManager.test.ts` (+5 tests for `buildCommitBindOptions` via as-any cast)
- [x] Existing unit tests updated: `SendRouter.test.ts`, `LinkGenerator.test.ts`, `FilePathPaster.test.ts`, `TextSelectionPaster.test.ts`, `TerminalSelectionService.test.ts` — all now assert exact `FormattedLink` objects and `ResolveResult` shapes instead of `expect.any(Object)` or bare booleans
- Closes #621
✅ CI / Integration Tests (with overrides) — run summaryDuration: 0m 51s QA TC IDs: 2 exercised across 0 features Report: View run & artifacts Reproduce locally: Feature breakdown
|
✅ CI / Integration Tests (automated) — run summaryDuration: 11m 24s QA TC IDs: 163 exercised across 0 features Report: View run & artifacts Reproduce locally: Feature breakdown
|
✅ CI / Integration Tests (with extensions) — run summaryDuration: 12m 37s Unit tests: Ran in separate Test & Validate job QA TC IDs: 195 exercised across 0 features Report: View run & artifacts Reproduce locally: Feature breakdown
|
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
When no destination is bound and the picker resolves to a bind during a send operation (R-V, R-L, R-F), the user sees one status bar message instead of two back-to-back toasts. The merge is achieved by threading bind context through the feedback layer, introducing a
ResolveResultdiscriminated union, and adding aBindContexttype shared across senders.Changes
showPickerAndBind()is suppressed via the existingStatusBarOptions.skipMessageflag.ResolveResultdiscriminated union replaces the boolean return ofresolveDestination(), encoding whether a bind was performed and the destination name when applicable. Callers no longer need to infer bind state from a bare boolean.BindContexttype ({ readonly destinationName: string }) shared acrossSendRouter,OperationFeedbackProvider,LinkGenerator, and four sender services, replacing an inline{ destinationName: string }type duplicated at each call site.toBindContext(result: ResolveResult)utility extractsBindContextfrom the discriminated union, eliminating a 4× copy-pasted ternary.BindingFeedbackinterface gainsnotifyRebound(newDestinationName, previousDestinationName)as a first-class method, splitting the rebind notification fromnotifyBound(destinationName). ThereplacedNameoptional param is removed.commitBindoptions restructured:suppressAutoPasteandStatusBarOptionsare nested under a singlebindOptionsparameter, keeping bind behavior and UI feedback concerns separated.Key Discoveries
replacedNamepath innotifyBoundwas the only place where rebind and fresh-bind messages could be confused. Splitting intonotifyReboundeliminates that ambiguity and structurally guarantees rebind messages never leak into the send-feedback path.TextSelectionPaster,LinkGenerator,FilePathPaster,TerminalSelectionService) had nearly identicalresolveResult → bindContextconstruction logic. ExtractingtoBindContextas a standalone utility with its own test file (100% branch coverage) cleaned up the duplication.Test Plan
toBindContext.test.ts(3 tests, 100% branch),OperationFeedbackProvider.test.ts(+11 tests coveringnotifyBound,notifyRebound,notifyAlreadyBound,notifyBindFailedEditor,notifyBindFailedNotAvailable,notifyBackgroundTabOpened,notifyUnbound,notifyNothingToUnbind,notifyJumpFocused,notifyJumpFailed),PasteDestinationManager.test.ts(+5 tests forbuildCommitBindOptionsvia as-any cast)SendRouter.test.ts,LinkGenerator.test.ts,FilePathPaster.test.ts,TextSelectionPaster.test.ts,TerminalSelectionService.test.ts— all now assert exactFormattedLinkobjects andResolveResultshapes instead ofexpect.any(Object)or bare booleansRelated
Summary by CodeRabbit
New Features
Tests