Conversation
added 4 commits
March 22, 2026 17:15
Added two user-configurable settings that allow power users to mute navigation toasts without losing the navigation functionality itself: - `rangelink.navigation.showNavigatedToast` (default: true) — controls the info toast shown after successful exact navigation - `rangelink.navigation.showClampingWarning` (default: true) — controls the warning toast shown when navigation is clamped to file boundaries Injected `ConfigReader` into `RangeLinkNavigationHandler` as a 4th constructor parameter, following the same pattern used in `RangeLinkService`. Error toasts remain always-on — errors need visibility regardless of user preference. The issue's proposed `toastDelay` setting was dropped because VS Code's `showInformationMessage`/`showWarningMessage` API does not support duration parameters. Benefits: - Power users can silence navigated/clamping toasts for a quieter workflow - Error toasts always shown for visibility - Default behavior unchanged (both settings default to true)
…tings Added 3 integration tests that verify setting-controlled toast suppression in a real VS Code host: - showNavigatedToast=false: navigation succeeds silently (no info toast) - showClampingWarning=false: clamped navigation succeeds silently (no warning toast) - Default settings: info toast shown after successful navigation (baseline) Each test uses log-based toast assertions and verifies navigation still works (correct selection position) even when toasts are suppressed.
Added CHANGELOG entry under [Unreleased] Added for the two navigation toast settings with descriptions of what each controls. Added "Navigation Settings" section to README Configuration with a table documenting both settings, defaults, and a note that error toasts always appear. Section has <sup>Unreleased</sup> marker per trunk-based documentation convention.
|
✅ QA Coverage OK New settings for controlling navigation toast notifications were introduced, allowing users to suppress specific toasts after navigation actions. Generated by QA Gap Check (GPT-4o-mini via GitHub Models) |
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds two new boolean settings— Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (click link)"
participant Cmd as "handleDocumentLinkClick"
participant Handler as "RangeLinkNavigationHandler"
participant Config as "ConfigReader"
participant IDE as "IDE Adapter (show message)"
participant Logger as "Logger"
User->>Cmd: trigger link click
Cmd->>Handler: navigateTo(target)
Handler->>Config: getBoolean(showClampingWarning / showNavigatedToast)
Handler->>IDE: apply navigation (open editor / set selection)
alt clamping occurred
Handler->>Config: getBoolean(showClampingWarning, default:true)
alt true
Handler->>IDE: showWarningMessage("clamped...")
else false
Handler->>Logger: debug("suppressed clamping warning")
end
else exact navigation
Handler->>Config: getBoolean(showNavigatedToast, default:true)
alt true
Handler->>IDE: showInformationMessage("Navigated to ...")
else false
Handler->>Logger: debug("suppressed navigated toast")
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/navigationToastSettings.test.ts`:
- Around line 128-132: The test uses assertNoToastLogged with a partial message
'RangeLink: Navigated to' which will never equal the actual toast string and
makes the check ineffective; update the test to pass the exact expected message
into assertNoToastLogged (or derive it using the same message-formatting helper
used when toasts are created) so the assertion compares the full toast text
produced by the extension (e.g., the fully formatted "RangeLink: Navigated to
<filename> @ <line>" string), referencing the assertNoToastLogged call and the
logCapture.getLinesSince usage to locate and correct the expected message.
- Around line 154-159: The current negative assertion uses the partial string
'clamped' which won't detect the actual toast text format; update the check that
calls assertNoToastLogged(lines, { type: 'warning', message: 'clamped' }) to use
the exact expected warning string produced by the code (e.g. "RangeLink:
Navigated to ${filename} @ 50 (clamped: line exceeded file length)") — locate
the test around the variables lastLine and sel.anchor.line and the log capture
call logCapture.getLinesSince('before-toast-settings-002') and replace the
partial message with the precise formatted message (using the test's filename
variable) so the assertion accurately fails when the clamping warning is
emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5029e05-1b7a-4e08-ba8f-d5dd1a842615
📒 Files selected for processing (11)
packages/rangelink-vscode-extension/CHANGELOG.mdpackages/rangelink-vscode-extension/README.mdpackages/rangelink-vscode-extension/package.jsonpackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0-2026-03-18.yamlpackages/rangelink-vscode-extension/src/__integration-tests__/suite/navigationToastSettings.test.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.tspackages/rangelink-vscode-extension/src/constants/settingDefaults.tspackages/rangelink-vscode-extension/src/constants/settingKeys.tspackages/rangelink-vscode-extension/src/extension.tspackages/rangelink-vscode-extension/src/navigation/RangeLinkNavigationHandler.ts
added 2 commits
March 22, 2026 21:48
The negative toast assertions used partial strings that could never match via `findLogEntry`'s exact `===` comparison, making both assertions no-ops. Fixing the message strings alone still leaves a fragile single-point-of-failure — if `assertNoToastLogged` has a bug, the test silently passes. The suppression debug logs now include a `suppressedMessage` attribute carrying the exact formatted message that *would* have been shown. A new `assertSuppressionLogged` helper matches on `fn` + `suppressedMessage` in the log JSON context. Integration tests now have a two-sided proof: the suppression log IS present with the exact constructed message AND the toast is NOT present. Changes: - `RangeLinkNavigationHandler`: build toast/clamping message before the setting branch; log it as `suppressedMessage` attribute in else-branch - `logBasedUiAssertions`: added `parseLogContextFull` (extracts arbitrary JSON attrs) and `assertSuppressionLogged` helper - Integration tests (TC 001, TC 002): use `assertSuppressionLogged` with exact `suppressedMessage` + `assertNoToastLogged` - Unit tests: verify `suppressedMessage` attribute in both suppression debug log assertions Ref: #491 (review)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__integration-tests__/helpers/logBasedUiAssertions.ts (1)
46-61: Minor observation: shared JSON extraction logic.There's some duplication between
parseLogContextandparseLogContextFullfor the JSON extraction. Given this is test infrastructure with minimal duplication and slightly different validation requirements, the current approach is acceptable. No action needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__integration-tests__/helpers/logBasedUiAssertions.ts` around lines 46 - 61, No code change required: the duplication between parseLogContext and parseLogContextFull is acceptable for test infra, so leave both functions (parseLogContextFull and parseLogContext) as-is and do not consolidate their JSON extraction logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/helpers/logBasedUiAssertions.ts`:
- Around line 46-61: No code change required: the duplication between
parseLogContext and parseLogContextFull is acceptable for test infra, so leave
both functions (parseLogContextFull and parseLogContext) as-is and do not
consolidate their JSON extraction logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 530e658a-af39-4803-9eed-559f4fd0077c
📒 Files selected for processing (5)
packages/rangelink-vscode-extension/src/__integration-tests__/helpers/index.tspackages/rangelink-vscode-extension/src/__integration-tests__/helpers/logBasedUiAssertions.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/navigationToastSettings.test.tspackages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.tspackages/rangelink-vscode-extension/src/navigation/RangeLinkNavigationHandler.ts
✅ Files skipped from review due to trivial changes (1)
- packages/rangelink-vscode-extension/src/integration-tests/helpers/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rangelink-vscode-extension/src/integration-tests/suite/navigationToastSettings.test.ts
- packages/rangelink-vscode-extension/src/tests/navigation/RangeLinkNavigationHandler.test.ts
…thod with richer return type
couimet
pushed a commit
that referenced
this pull request
Mar 24, 2026
…s for logBasedUiAssertions `getLogCapture.ts` depends on `vscode.extensions` (VS Code host only) and was dragging down Jest coverage metrics with 0% across all metrics. Excluded it from `collectCoverageFrom` — targeted exclusion, not the entire `__integration-tests__/` directory. `logBasedUiAssertions.ts` is the assertion safety net for all log-based integration tests. Extended the existing test file with tests for `assertSuppressionLogged` (new function from PR #491 feedback) and `parseLogContext` edge cases (invalid JSON, missing fn, non-string fn, non-object parse results). The file is now at 100% coverage across all metrics.
couimet
added a commit
that referenced
this pull request
Mar 24, 2026
…extension` (#495) * [issues/488] Exclude getLogCapture.ts from coverage and add full tests for logBasedUiAssertions `getLogCapture.ts` depends on `vscode.extensions` (VS Code host only) and was dragging down Jest coverage metrics with 0% across all metrics. Excluded it from `collectCoverageFrom` — targeted exclusion, not the entire `__integration-tests__/` directory. `logBasedUiAssertions.ts` is the assertion safety net for all log-based integration tests. Extended the existing test file with tests for `assertSuppressionLogged` (new function from PR #491 feedback) and `parseLogContext` edge cases (invalid JSON, missing fn, non-string fn, non-object parse results). The file is now at 100% coverage across all metrics. * [test] Cover uncovered callbacks in destinationBuilders The builder functions passed `compareWith`, `isAvailable`, and `getUserInstruction` callbacks into `ComposablePasteDestination` but no test ever invoked them — leaving 6 branches and 7 functions uncovered despite high line coverage. Added tests that invoke each callback through the built destination's public interface: `equals()` for compareWith, `isAvailable()` for availability delegation, and `getUserInstruction()` for both Success (returns undefined) and Failure (returns instruction string) branches. All three AI assistant builders (Cursor, Claude Code, GitHub Copilot) now have availability spy tests verifying delegation to the detection utilities. `destinationBuilders.ts` is now at 100% across all coverage metrics. * [test] Cover BookmarksStore error handling branches BookmarksStore had 65.78% branch coverage — all the `catch` branches for save failures and unexpected errors were untested. These branches are the safety net that prevents unhandled rejections from propagating to the extension host. Added tests for both error paths: `globalState.update` rejection (triggers the `save()` catch which wraps as `BOOKMARK_SAVE_FAILED` with operation details and cause), and non-`RangeLinkExtensionError` throws (triggers the generic catch in each CRUD method). Also covered the `error instanceof Error` ternary edge case where a non-Error value is thrown. BookmarksStore is now at 100/92/100/100 (branches at 92.1%, up from 65.78%). * [test] Cover BindToTextEditorCommand executeWithUri and getPlaceholder paths The entire `executeWithUri` path (explorer context menu "Bind Here") was untested — the tab group scanning loop, multi-group picker, and single-group focus were all uncovered. The `getPlaceholder` callback in the file picker was also never invoked. Added 5 tests covering all `executeWithUri` branches (no matching groups, single group, multi-group picker selection, picker cancellation) and the `getPlaceholder` callback invocation. `BindToTextEditorCommand.ts` is now at 100/93.75/100/100 (up from 76.78/62.5/75/78.18). * [test] Cover focus capability resolution branches and factory methods `FocusCapabilityFactory` had 25% function coverage (only `createAIAssistantCapability` called), `TerminalFocusCapability` had no test for its error catch branch, and `EditorFocusCapability.resolveViewColumn` had an untested fallback when `editor.viewColumn` is undefined. Added `FocusCapabilityFactory.test.ts` (verifies all 3 factory methods return correct capability types), `TerminalFocusCapability.test.ts` (success + error catch branch), and the undefined-viewColumn fallback test in `EditorFocusCapability.test.ts`. All capabilities files now at 100% across all individual coverage metrics. * [test] Add unit tests for SelectionValidator `SelectionValidator` was extracted from `RangeLinkService` in #493 but had no dedicated test file — 0% branches, 16.66% functions. Added tests covering all three public methods: `validateSelectionsAndShowError` (valid selections, no editor, empty selections), `mapSelectionsForLogging` (populated and empty arrays), and `getLineContentAtSelectionBoundaries` (in-bounds, out-of-bounds, lineAt throws, negative line numbers). `SelectionValidator.ts` is now at 100% across all coverage metrics. * [test] Cover getPlaceholder() callbacks in StatusBar and DestinationPicker overflow pickers The `getPlaceholder` callbacks in secondary terminal/file pickers were never invoked during tests — they were passed to `showTerminalPicker`/`showFilePicker` but the mock implementations only called `onSelected`. Added `getPlaceholder()` invocation and literal placeholder assertions to existing overflow picker tests in both `RangeLinkStatusBar` and `DestinationPicker`. Both files now at 100% function coverage. Only the exhaustive `never` type guards remain uncovered (unreachable by design). * [refactor] Move inline command object construction to bootstrap section Preparation for extracting registration wiring into a separate file. Command objects (ShowVersionCommand, BindToDestinationCommand, JumpToDestinationCommand, GoToRangeLinkCommand, ListBookmarksCommand, ManageBookmarksCommand) were created inline within the registration block — moved them to the bootstrap section so the registration block contains only `context.subscriptions.push(...)` calls. * [refactor] Extract registration wiring from extension.ts into wireSubscriptions.ts extension.ts was 649 lines mixing bootstrap, dependency construction, and registration wiring. This made it hard to navigate and impossible to unit test without mocking the full activation lifecycle. Moved the entire object graph (config, services, commands, providers) and all `context.subscriptions.push(...)` calls into `wireSubscriptions()`. The `ExtensionDependencies` interface has only 3 fields — `ideAdapter`, `logger`, `versionInfo` — everything else is built internally. extension.ts is now 67 lines: logger init, version info, adapter creation, locale setup, and the single `wireSubscriptions()` call. Benefits: - extension.ts is a minimal bootstrap file — reads top to bottom in seconds - wireSubscriptions.ts is independently unit-testable with just 3 mock dependencies - New commands/services are added in one place without touching bootstrap * [test] Add unit tests for wireSubscriptions command and provider registration Verifies that `wireSubscriptions` registers the complete set of 50 commands and 4 link providers. Uses a real `VscodeAdapter` backed by mock VSCode — calls `wireSubscriptions` with the 3-field `ExtensionDependencies` and asserts against spy calls. Provider tests go beyond counts: they verify exact provider types via `instanceof` (FilePathTerminalProvider, RangeLinkTerminalProvider, FilePathDocumentProvider, RangeLinkDocumentProvider) and document selectors (`file` + `untitled` schemes). Command count is frozen as a literal to catch accidental deletions from the EXPECTED_COMMANDS array. * Raise thresholds — pass #1 * [refactor] Introduce SubscriptionRegistrar and WiringServices factory for testable wiring `wireSubscriptions` built the entire object graph and registered closures in one function — making closures untestable (services constructed internally, registrations coupled to `context`). This kept function coverage at 85% with no path to 90%. Separated concerns into three layers following the VscodeAdapter pattern: - `SubscriptionRegistrar`: wraps `context.subscriptions.push(ideAdapter.registerCommand(...))` behind a testable interface - `createWiringServices`: factory that builds the full service object graph from 3 foundational deps - `wireSubscriptions`: pure wiring — receives a registrar and pre-built services, registers closures Tests inject a mock registrar (captures handlers for invocation) and mock services (verifiable delegation). Added 30 closure delegation tests covering all service methods, URI ternary branches, and representative commands from each pattern. Functions coverage crossed the 90% target. Coverage thresholds raised to 97/95/90/97 — all acceptance criteria met. Benefits: - Every command closure is independently testable via `registrar.getHandler(CMD_X)()` - `wireSubscriptions` is now 100% branches (pure wiring, no construction logic) - `extension.ts` stays 67 lines — unchanged from the previous extraction * [test] Cover all wireSubscriptions closures and exclude barrel files from coverage Added the remaining 13 closure delegation tests — every command handler now verifies the correct service method is called with the correct arguments. `wireSubscriptions.ts` is at 100% across all metrics. Excluded barrel `index.ts` re-export files from coverage collection — Istanbul counts each `export {}` as a function, inflating the denominator with phantom uncovered functions despite 100% line coverage. This removed ~85 false-negative functions from the count. Coverage thresholds raised to 98/95/95/98 (up from 97/95/90/97). * Ran `fix` * [PR feedback] Address CodeRabbit review — test isolation, spy typing, logger assertions, exact matchers Addressed 4 accepted items from the CodeRabbit review: - FocusCapabilityFactory.test.ts: moved mock creation into `beforeEach` for test isolation - spyOnIsGitHubCopilotChatAvailable.ts: added generic type params (`jest.SpyInstance<Promise<boolean>, [VscodeAdapter, Logger]>`) so `mockResolvedValue` infers correctly — removed `as never` cast from destinationBuilders.test.ts - BookmarksStore.test.ts: added `mockLogger.error` assertions to update, remove, and recordAccess save failure tests (T007 compliance) - SelectionValidator.test.ts: replaced 3 `expect.objectContaining()` calls with exact object literals (T004 compliance) Ignored Feedback: - spyOnIsCursorIDEDetected interception concern: false positive — CommonJS module identity ensures the spy intercepts calls through barrel re-exports. All 3 spy tests pass and verify `toHaveBeenCalledTimes(1)`. Ref: #495 (review)
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
Adds two user-configurable settings that let power users control which toasts appear after clicking a RangeLink. This completes Phase 3 of the Enhanced Navigation Feedback epic (#47), following Phase 1 (clamping detection) and Phase 2 (toast classification).
The issue's proposed
toastDelaysetting was dropped because VS Code'sshowInformationMessage/showWarningMessageAPI does not support duration parameters — toasts remain until dismissed by the user.Changes
rangelink.navigation.showNavigatedToast(boolean, default: true) — controls the info toast after successful navigationrangelink.navigation.showClampingWarning(boolean, default: true) — controls the warning toast when navigation is clamped to file boundariesConfigReaderintoRangeLinkNavigationHandlerto read settings, following the same pattern asRangeLinkServicesettingDefaults.tsfor consistency with other settingsTest Plan
test:releasepasses — 110 integration tests in VS Code host, 0 failuresnavigation-toast-settings-001through003(all automated)Related
Summary by CodeRabbit
New Features
Documentation
Tests