Conversation
When a RangeLink points beyond file boundaries (e.g., #L100 in a 50-line file), the position is silently clamped with no way for callers to know. Adding lineClamped and characterClamped booleans to ConvertedPosition lets the navigation handler (Phase 2) provide user feedback when positions are adjusted. Benefits: - Callers can distinguish exact vs adjusted navigation without re-deriving bounds - Per-position detection (start/end independently) enables granular feedback - Zero overhead for existing callers — new fields are simply available
…tests All existing assertions now verify the new lineClamped/characterClamped fields, and a dedicated "Clamping detection" describe block covers the matrix: no clamping, line-only, char-only, both, undefined character, and exact boundary conditions.
…bounds Without feedback, users navigating to stale links (e.g., file shortened since the link was created) see the cursor land at an unexpected location with no explanation. A warn-level log now fires when any position is clamped, including requested vs actual values for both start and end — giving visibility in the output channel ahead of Phase 2's user-facing toasts.
Covers the three scenarios: line clamped (requested line beyond EOF), character clamped (requested column beyond line length), and no clamping (position within bounds produces no warning).
|
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)
WalkthroughconvertRangeLinkPosition now reports per-axis clamping (lineClamped, characterClamped). RangeLinkNavigationHandler detects any clamping and emits a structured warning via logger before creating the selection. Tests and integration tests were added to cover vertical, horizontal, both, and no-clamp scenarios. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Handler as RangeLinkNavigationHandler
participant Converter as convertRangeLinkPosition
participant Logger as Logger
participant Editor as VSCode Editor
User->>Handler: navigateToLink(requestedPosition)
Handler->>Converter: convertRangeLinkPosition(position, document)
Converter->>Converter: Compute clamping flags (lineClamped, characterClamped)
Converter-->>Handler: { line, character, lineClamped, characterClamped }
alt Any Clamping Detected
Handler->>Logger: warn(structured payload with requested vs actual and flags)
Logger-->>Handler: logged
end
Handler->>Editor: create selection & reveal
Editor-->>User: position revealed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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)
📝 Coding Plan
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 New test cases were added for navigation clamping feedback, which includes user-visible changes in how the extension handles navigation to out-of-bounds positions. Generated by QA Gap Check (GPT-4o-mini via GitHub Models) |
|
@coderabbitai review I was rate limited in #479 (comment) |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.ts (1)
952-968: Replace partial matcher with stricter assertion.The assertion at lines 964-967 uses
expect.anything()which violates coding guidelines prohibiting partial matchers. Since positions are within bounds and path resolution succeeds, nologger.warncalls should occur at all.Proposed fix: use stricter negative assertion
- expect(mockLogger.warn).not.toHaveBeenCalledWith( - expect.anything(), - 'Position clamped to document bounds', - ); + expect(mockLogger.warn).not.toHaveBeenCalled();As per coding guidelines: "Do not use partial matchers like
expect.objectContaining()orexpect.stringContaining()in Jest; assert exact values instead"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.ts` around lines 952 - 968, The test currently uses a partial matcher against mockLogger.warn which violates the no-partial-matchers rule; update the assertion in RangeLinkNavigationHandler.test.ts (the it block calling handler.navigateToLink) to assert that mockLogger.warn was not called at all—e.g., replace the expect(mockLogger.warn).not.toHaveBeenCalledWith(expect.anything(), 'Position clamped to document bounds') with a stricter negative assertion such as expect(mockLogger.warn).not.toHaveBeenCalled() so the test ensures no warnings were logged when positions are within bounds and path resolution succeeds.
🤖 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/__tests__/navigation/RangeLinkNavigationHandler.test.ts`:
- Around line 952-968: The test currently uses a partial matcher against
mockLogger.warn which violates the no-partial-matchers rule; update the
assertion in RangeLinkNavigationHandler.test.ts (the it block calling
handler.navigateToLink) to assert that mockLogger.warn was not called at
all—e.g., replace the
expect(mockLogger.warn).not.toHaveBeenCalledWith(expect.anything(), 'Position
clamped to document bounds') with a stricter negative assertion such as
expect(mockLogger.warn).not.toHaveBeenCalled() so the test ensures no warnings
were logged when positions are within bounds and path resolution succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 92b01c2e-24f7-4c25-a6a2-178166789acd
📒 Files selected for processing (4)
packages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.tspackages/rangelink-vscode-extension/src/__tests__/utils/convertRangeLinkPosition.test.tspackages/rangelink-vscode-extension/src/navigation/RangeLinkNavigationHandler.tspackages/rangelink-vscode-extension/src/utils/convertRangeLinkPosition.ts
The clamping-negative test used expect.anything() which violates rule T004. Since path resolution succeeds and positions are within bounds in this test, logger.warn should not be called at all — .not.toHaveBeenCalled() is both stricter and simpler. Ref: #479 (review)
Appends 4 new TCs to the v1.1.0 QA YAML under "Navigation Clamping Feedback": line beyond EOF, column beyond line length, valid position (no clamping), and both axes clamped. All marked automated: true — integration tests follow in the next commit.
…cation Validates end-to-end that clamped positions land at the correct document boundaries: line beyond EOF clamps to last line, column beyond line length clamps to end of line, valid position lands exactly, and both-axes clamping. Toast type/content verification deferred to #481.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__integration-tests__/suite/navigationClamping.test.ts (1)
148-157: Consider verifying both anchor and active positions for completeness.This test only asserts on
sel.anchor, but for single-position selections with explicit character (#L5C10), the handler extends the selection by 1 character when possible. Verifyingsel.activewould ensure the extension behavior is correctly applied end-to-end.💡 Suggested addition
assert.strictEqual(sel.anchor.line, 4, 'Expected anchor at line 4 (0-indexed)'); assert.strictEqual(sel.anchor.character, 9, 'Expected anchor char 9 (0-indexed)'); + assert.strictEqual(sel.active.line, 4, 'Expected active at line 4 (0-indexed)'); + assert.strictEqual(sel.active.character, 10, 'Expected active char 10 (extended by 1)'); });🤖 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__/suite/navigationClamping.test.ts` around lines 148 - 157, The test navigation-clamping-003 currently only asserts sel.anchor but should also assert sel.active to ensure the handler's one-character extension behavior for explicit char links (e.g., "#L5C10") is validated; update the test that calls parseLink and navigateViaHandleLinkClick to add an assertion that sel.active.line equals 4 and sel.active.character equals 10 (0-indexed) to confirm the selection was extended by one character when possible.
🤖 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__/suite/navigationClamping.test.ts`:
- Around line 148-157: The test navigation-clamping-003 currently only asserts
sel.anchor but should also assert sel.active to ensure the handler's
one-character extension behavior for explicit char links (e.g., "#L5C10") is
validated; update the test that calls parseLink and navigateViaHandleLinkClick
to add an assertion that sel.active.line equals 4 and sel.active.character
equals 10 (0-indexed) to confirm the selection was extended by one character
when possible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bda855dc-c11c-425b-8819-c4bc1068f545
📒 Files selected for processing (3)
packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0-2026-03-18.yamlpackages/rangelink-vscode-extension/src/__integration-tests__/suite/navigationClamping.test.tspackages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.ts
…n test The test only verified the anchor position but not the active end of the selection. For single-position links with explicit character (#L5C10), the handler extends the selection by 1 character for visibility — verifying sel.active catches regressions in that behavior. Ref: #479 (review)
1 participant
Summary
Adds clamping detection to
convertRangeLinkPositionso the navigation system knows when a RangeLink points beyond file boundaries. When a user navigates to a stale link (e.g.,#L100in a file that now has only 50 lines), the position is silently clamped — this phase makes that clamping observable via boolean fields onConvertedPositionand warn-level logging in the navigation handler.Changes
ConvertedPositioninterface withlineClampedandcharacterClampedboolean fieldsconvertRangeLinkPositionto detect when requested positions exceed document/line bounds (only MAX bound — minimum is impossible per parser guarantees)RangeLinkNavigationHandler.navigateToLink()with requested vs actual position detailsTest Plan
Related
Summary by CodeRabbit
New Features
Tests
QA