[issues/342] Forgiving filename navigation — resolve bare filenames via workspace search#497
Merged
Conversation
added 5 commits
March 23, 2026 23:48
Callers had no visibility into which resolution strategy succeeded — the function returned a bare URI. This made it impossible for handlers to log or adjust UI feedback for the filename-fallback (Issue #342) vs standard resolution. The new ResolvedPath interface carries `uri` + `resolvedVia` ('absolute' | 'workspace-relative' | 'filename-fallback'). Both navigation handlers now log the strategy at DEBUG level, giving output channel visibility into how each link was resolved. Benefits: - Handlers can distinguish fallback navigation from standard resolution in logs - Opens the door for strategy-aware toast messages without further refactoring - No behavioral change — existing callers just destructure `.uri` from the result
…ent drift The 'absolute' and 'workspace-relative' string literals were duplicated between PathFormat (input) and PathResolutionStrategy (output). Using a template literal type ties them together so changes to PathFormat automatically propagate.
…back Ensures the fallback behavior from Issue #342 is covered by both the QA framework and automated integration tests. Four TCs added under slug `filename-fallback-navigation`: two automated (unique match navigates, relative path uses standard resolution) and two manual (ambiguous/missing filenames show toast — blocked by showWarningMessage in extension host). The QA validator confirms all 57 `automated: true` entries match integration test IDs.
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 (2)
WalkthroughAdds a filename-only navigation fallback: when a RangeLink contains only a bare filename (no directory separators), the resolver globs the workspace; a single match returns a ResolvedPath and navigation proceeds, multiple matches return an ambiguity sentinel and trigger a warning, and zero matches remain a not-found result. New types and tests added. Changes
Sequence DiagramsequenceDiagram
participant User
participant Handler as RangeLinkNavigationHandler
participant Adapter as VscodeAdapter
participant Utils as resolveWorkspacePath
participant Workspace as vscode.workspace
participant Editor as vscode.window
User->>Handler: Click RangeLink (bare filename)
Handler->>Adapter: resolveWorkspacePath(filename)
Adapter->>Utils: delegate resolution
Utils->>Workspace: try absolute fs.stat / workspace-relative stat
Workspace-->>Utils: not found
alt input has no separators
Utils->>Workspace: findFiles("**/<escaped-filename>", maxResults=2)
Workspace-->>Utils: [matches]
alt exactly 1 match
Utils-->>Adapter: ResolvedPath{ uri, resolvedVia: 'filename-fallback' }
Adapter-->>Handler: ResolvedPath
Handler->>Editor: showTextDocument(resolved.uri)
Editor-->>User: navigated
else multiple matches
Utils-->>Adapter: FILENAME_AMBIGUOUS
Adapter-->>Handler: FILENAME_AMBIGUOUS
Handler-->>User: show warning (Multiple files match)
else 0 matches
Utils-->>Adapter: undefined
Adapter-->>Handler: undefined
Handler-->>User: show not-found warning
end
else contains separators
Utils-->>Adapter: undefined
Adapter-->>Handler: undefined
Handler-->>User: show not-found warning
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 |
|
The PR introduces a new feature for forgiving filename navigation, which allows links with bare filenames to navigate correctly when there is a unique match. This includes new scenarios for both successful navigation and handling of ambiguous or nonexistent filenames. 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: 2
🧹 Nitpick comments (3)
packages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.ts (1)
573-575: StandardizeresolvedViamocks to enum values across updated tests.These updated mock blocks should use the project enum member(s) instead of
'workspace-relative'string literals to keep setup type-safe and resilient to enum renames.As per coding guidelines: "Use literal values in expect() assertions for project enums, user-facing text, and config keys - Use enum values in test setup/mocks for type safety".
Also applies to: 780-782, 828-830, 1099-1101, 1211-1213
🤖 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 573 - 575, The mock setup uses the string 'workspace-relative' for the resolvedVia field; replace these with the project's enum member (import the project's ResolvedVia/ResolvedViaKind enum) and use the appropriate enum value instead of the literal string in the mockResolvedValue for mockAdapter.resolveWorkspacePath (and the other identical mocks at the indicated locations). Update imports to include the enum, change resolvedVia: 'workspace-relative' to resolvedVia: ResolvedVia.<WORKSPACE_RELATIVE_MEMBER>, and run tests to ensure type-safety and consistency.packages/rangelink-vscode-extension/src/__tests__/navigation/FilePathNavigationHandler.test.ts (1)
130-130: Use enum members forresolvedViain test mocks.At Line 130, Line 171, and Line 183, prefer
PathFormatenum values instead of raw strings so mocks stay type-safe if enum contracts evolve.As per coding guidelines: "Use literal values in expect() assertions for project enums, user-facing text, and config keys - Use enum values in test setup/mocks for type safety".
Also applies to: 171-171, 181-183
🤖 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/FilePathNavigationHandler.test.ts` at line 130, Replace the raw string literals used for resolvedVia in test mocks with the corresponding PathFormat enum members (e.g., use PathFormat.<member> that maps to 'workspace-relative') in the .mockResolvedValue({... resolvedVia: ...}) calls and any related expect() assertions in FilePathNavigationHandler.test (locations around the shown .mockResolvedValue and the other mentions at lines ~171 and ~183); import PathFormat if needed and use the enum members instead of raw strings so the mocks remain type-safe if the enum changes.packages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.ts (1)
412-416: Prefer enum-backedresolvedViamock value.At Line 414, switch from the raw string literal to the corresponding enum member for stronger type safety in test setup.
As per coding guidelines: "Use literal values in expect() assertions for project enums, user-facing text, and config keys - Use enum values in test setup/mocks for type safety".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.ts` around lines 412 - 416, Replace the raw string literal used for mockResolved.resolvedVia with the appropriate enum member from the codebase (rather than 'workspace-relative'); import the enum and set resolvedVia to that enum member (e.g., ResolvedVia.WorkspaceRelative) in the mockResolved object used by spyOnResolveWorkspacePath(), so the test uses the enum for type-safe mocking.
🤖 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/qa/qa-test-cases-v1.1.0-2026-03-18.yaml`:
- Around line 2377-2386: Test case filename-fallback-navigation-002 expects the
generic "Cannot find file" message for ambiguous filename matches; update both
the YAML expected_result and the code that shows the toast so ambiguity is
reported instead of "not found". Change the QA entry (id
filename-fallback-navigation-002, feature 'Filename-Only Navigation Fallback')
expected_result to assert a warning like "RangeLink: Multiple files found for
filename: index.ts" and modify the file-resolution path in the code (the
function that resolves RangeLink targets — e.g., resolveRangeLinkTarget /
findFilesByName / handleRangeLinkClick) so when more than one match is returned
it emits a distinct toast message indicating multiple matches and does not
navigate. Ensure the toast text exactly matches the updated expected_result.
In `@packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts`:
- Around line 64-69: The glob pattern built in resolveWorkspacePath (where
pattern = `**/${linkPath}`) directly interpolates linkPath and fails for
filenames containing glob metacharacters; update the pattern construction in
resolveWorkspacePath to escape glob metacharacters using VS Code bracket-range
escaping (e.g., `[` → `[[]`, `]` → `[]]`, `*` → `[*]`, `?` → `[?]`, `{`/`}`
similarly) before inserting into `**/${escaped}` so filenames like `[id].ts`,
`foo?.ts`, `file*.ts` are treated as literals; also add unit tests to the
filename-fallback test suite that cover bare filenames containing `*`, `?`, `[`
and `]` to prevent regressions.
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.ts`:
- Around line 412-416: Replace the raw string literal used for
mockResolved.resolvedVia with the appropriate enum member from the codebase
(rather than 'workspace-relative'); import the enum and set resolvedVia to that
enum member (e.g., ResolvedVia.WorkspaceRelative) in the mockResolved object
used by spyOnResolveWorkspacePath(), so the test uses the enum for type-safe
mocking.
In
`@packages/rangelink-vscode-extension/src/__tests__/navigation/FilePathNavigationHandler.test.ts`:
- Line 130: Replace the raw string literals used for resolvedVia in test mocks
with the corresponding PathFormat enum members (e.g., use PathFormat.<member>
that maps to 'workspace-relative') in the .mockResolvedValue({... resolvedVia:
...}) calls and any related expect() assertions in
FilePathNavigationHandler.test (locations around the shown .mockResolvedValue
and the other mentions at lines ~171 and ~183); import PathFormat if needed and
use the enum members instead of raw strings so the mocks remain type-safe if the
enum changes.
In
`@packages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.ts`:
- Around line 573-575: The mock setup uses the string 'workspace-relative' for
the resolvedVia field; replace these with the project's enum member (import the
project's ResolvedVia/ResolvedViaKind enum) and use the appropriate enum value
instead of the literal string in the mockResolvedValue for
mockAdapter.resolveWorkspacePath (and the other identical mocks at the indicated
locations). Update imports to include the enum, change resolvedVia:
'workspace-relative' to resolvedVia: ResolvedVia.<WORKSPACE_RELATIVE_MEMBER>,
and run tests to ensure type-safety and consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75ccd31d-79f8-45cb-8552-25f14acf161a
📒 Files selected for processing (13)
packages/rangelink-vscode-extension/CHANGELOG.mdpackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0-2026-03-18.yamlpackages/rangelink-vscode-extension/src/__integration-tests__/suite/filenameOnlyNavigation.test.tspackages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.tspackages/rangelink-vscode-extension/src/__tests__/navigation/FilePathNavigationHandler.test.tspackages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.tspackages/rangelink-vscode-extension/src/__tests__/utils/resolveWorkspacePath.test.tspackages/rangelink-vscode-extension/src/ide/vscode/VscodeAdapter.tspackages/rangelink-vscode-extension/src/navigation/FilePathNavigationHandler.tspackages/rangelink-vscode-extension/src/navigation/RangeLinkNavigationHandler.tspackages/rangelink-vscode-extension/src/types/ResolvedPath.tspackages/rangelink-vscode-extension/src/types/index.tspackages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts
Comment on lines
+2377
to
+2386
| - id: filename-fallback-navigation-002 | ||
| feature: 'Filename-Only Navigation Fallback' | ||
| scenario: 'Bare filename with multiple matches shows file-not-found warning' | ||
| preconditions: | ||
| - 'Extension installed from .vsix build' | ||
| - 'Two or more files with the same name exist in different directories (e.g., index.ts in src/ and lib/)' | ||
| steps: | ||
| - 'Click a RangeLink using only the ambiguous filename (e.g., index.ts#L1)' | ||
| expected_result: 'A warning toast is shown: "RangeLink: Cannot find file: index.ts". No navigation occurs.' | ||
| automated: false |
Contributor
There was a problem hiding this comment.
Distinguish ambiguous filenames from missing files.
This case currently expects the same Cannot find file warning as the no-match path. For the new fallback that is misleading—the actionable problem is ambiguity, not absence. Please make the warning, and this QA expectation, say that multiple matches were found.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0-2026-03-18.yaml`
around lines 2377 - 2386, Test case filename-fallback-navigation-002 expects the
generic "Cannot find file" message for ambiguous filename matches; update both
the YAML expected_result and the code that shows the toast so ambiguity is
reported instead of "not found". Change the QA entry (id
filename-fallback-navigation-002, feature 'Filename-Only Navigation Fallback')
expected_result to assert a warning like "RangeLink: Multiple files found for
filename: index.ts" and modify the file-resolution path in the code (the
function that resolves RangeLink targets — e.g., resolveRangeLinkTarget /
findFilesByName / handleRangeLinkClick) so when more than one match is returned
it emits a distinct toast message indicating multiple matches and does not
navigate. Ensure the toast text exactly matches the updated expected_result.
added 4 commits
March 24, 2026 09:24
…lenames
Bare filenames containing glob metacharacters ([id].ts, foo?.ts) were interpreted as patterns by findFiles, making real files unresolvable. Now escaped using VS Code's bracket-range convention before building the glob.
Ambiguous filenames (2+ matches) previously showed the generic "Cannot find file" warning — misleading when the file exists in multiple locations. Now returns a distinct FILENAME_AMBIGUOUS sentinel so handlers show "Multiple files match: {filename}" instead, giving users actionable feedback to add a directory prefix.
Benefits:
- Next.js/SvelteKit [slug].ts files are now resolvable via bare filename
- Users know when to disambiguate vs when the file is truly missing
- resolveWorkspacePath return type cleanly signals three outcomes: resolved, ambiguous, not found
Ref: #497 (review)
…vedVia
Test setup was using string literals ('workspace-relative', 'absolute') for the resolvedVia field in mock objects. Project rule T003 requires enum values in setup for type safety — literals are reserved for assertions where they freeze the contract.
Ref: #497 (review)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/utils/resolveWorkspacePath.ts (1)
65-98:⚠️ Potential issue | 🟠 MajorBare filenames can still bypass the ambiguity check.
Because the workspace-relative loop runs first,
index.ts#L1will immediately resolve to<workspace>/index.tseven ifsrc/index.tsalso exists. That still guesses on ambiguous bare filenames, which is the case this feature is supposed to reject.💡 One straightforward way to fix it
const workspaceFolders = ideInstance.workspace.workspaceFolders; if (!workspaceFolders || workspaceFolders.length === 0) { return undefined; } + const isBareFilename = !linkPath.includes('/') && !linkPath.includes('\\'); + if (isBareFilename) { + const pattern = `**/${escapeGlobPattern(linkPath)}`; + try { + const matches = await ideInstance.workspace.findFiles( + pattern, + undefined, + AMBIGUITY_THRESHOLD, + ); + if (matches.length === 1) { + return { uri: matches[0], resolvedVia: 'filename-fallback' }; + } + if (matches.length >= AMBIGUITY_THRESHOLD) { + return FILENAME_AMBIGUOUS; + } + } catch { + // findFiles failed (e.g., no workspace open) — fall through to undefined + } + return undefined; + } + for (const folder of workspaceFolders) { const absolutePath = path.join(folder.uri.fsPath, linkPath); const uri = ideInstance.Uri.file(absolutePath); try { await ideInstance.workspace.fs.stat(uri); return { uri, resolvedVia: 'workspace-relative' }; } catch { // File doesn't exist in this workspace folder, try next continue; } } - - const isBareFilename = !linkPath.includes('/') && !linkPath.includes('\\'); - if (isBareFilename) { - const pattern = `**/${escapeGlobPattern(linkPath)}`; - ... - }Please also add a regression case for a workspace-root
index.tsplus anothersrc/index.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts` around lines 65 - 98, The current logic lets a bare filename like "index.ts#L1" resolve immediately via the workspace-relative loop, bypassing the ambiguity check; change resolve behavior so bare filenames are disambiguated before accepting a single workspace match: either (A) if isBareFilename run the findFiles(...)/AMBIGUITY_THRESHOLD check first and return FILENAME_AMBIGUOUS if ambiguous, or (B) when isBareFilename, collect matches across all workspaceFolders instead of returning on the first stat success and only return a single match when exactly one unique path is found (otherwise return FILENAME_AMBIGUOUS); update the resolvedVia values accordingly and keep using escapeGlobPattern, ideInstance.workspace.findFiles, AMBIGUITY_THRESHOLD and FILENAME_AMBIGUOUS; add a regression test covering a workspace-root index.ts plus src/index.ts to assert ambiguity is reported.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts (1)
44-47: Keep the new workspace search behind the adapter boundary.This PR adds another direct VS Code behavior call in
src/utils. IfresolveWorkspacePathkeeps growing here, theideAdapterabstraction stops buying much in testability or IDE portability. I’d move thefindFilesbranch intoVscodeAdapter.resolveWorkspacePath()(or a helper undersrc/ide/vscode/) and keep this utility API-neutral.As per coding guidelines, "No direct vscode.* behavior calls — use ideAdapter - Call behavior methods through ideAdapter (e.g., ideAdapter.showTextDocument()); import constants/enums directly from vscode".
Also applies to: 83-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts` around lines 44 - 47, The resolveWorkspacePath util currently calls vscode.findFiles directly; move the VS Code–specific workspace search logic into the IDE adapter boundary (e.g., implement a VscodeAdapter.resolveWorkspacePath() or helper under src/ide/vscode/) and have resolveWorkspacePath call ideAdapter.resolveWorkspacePath() instead so the util remains IDE-agnostic; update the code paths that perform the findFiles behavior (also referenced around the other branch at lines 83–88) to use the adapter method and remove any direct vscode.* calls from src/utils/resolveWorkspacePath.ts.
🤖 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/__tests__/navigation/RangeLinkNavigationHandler.test.ts`:
- Around line 738-774: Add assertions to confirm the ambiguous case
short-circuits: after calling RangeLinkNavigationHandler.navigateToLink with
resolveWorkspacePath mocked to FILENAME_AMBIGUOUS, assert that the adapter's
navigation methods were not invoked (e.g.,
expect(mockAdapter.showTextDocument).not.toHaveBeenCalled() and
expect(mockAdapter.openTextDocument).not.toHaveBeenCalled() or whichever
navigation/open-file methods exist on mockAdapter). Ensure you create
spies/mocks for mockAdapter.showTextDocument and mockAdapter.openTextDocument
before invoking handler.navigateToLink so the test fails if those are called.
---
Outside diff comments:
In `@packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts`:
- Around line 65-98: The current logic lets a bare filename like "index.ts#L1"
resolve immediately via the workspace-relative loop, bypassing the ambiguity
check; change resolve behavior so bare filenames are disambiguated before
accepting a single workspace match: either (A) if isBareFilename run the
findFiles(...)/AMBIGUITY_THRESHOLD check first and return FILENAME_AMBIGUOUS if
ambiguous, or (B) when isBareFilename, collect matches across all
workspaceFolders instead of returning on the first stat success and only return
a single match when exactly one unique path is found (otherwise return
FILENAME_AMBIGUOUS); update the resolvedVia values accordingly and keep using
escapeGlobPattern, ideInstance.workspace.findFiles, AMBIGUITY_THRESHOLD and
FILENAME_AMBIGUOUS; add a regression test covering a workspace-root index.ts
plus src/index.ts to assert ambiguity is reported.
---
Nitpick comments:
In `@packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts`:
- Around line 44-47: The resolveWorkspacePath util currently calls
vscode.findFiles directly; move the VS Code–specific workspace search logic into
the IDE adapter boundary (e.g., implement a VscodeAdapter.resolveWorkspacePath()
or helper under src/ide/vscode/) and have resolveWorkspacePath call
ideAdapter.resolveWorkspacePath() instead so the util remains IDE-agnostic;
update the code paths that perform the findFiles behavior (also referenced
around the other branch at lines 83–88) to use the adapter method and remove any
direct vscode.* calls from src/utils/resolveWorkspacePath.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fe1f21c-bacd-43a4-9ae4-87887ead2a9c
📒 Files selected for processing (15)
.prettierignoreeslint.config.mjspackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0-2026-03-18.yamlpackages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.tspackages/rangelink-vscode-extension/src/__tests__/navigation/FilePathNavigationHandler.test.tspackages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.tspackages/rangelink-vscode-extension/src/__tests__/utils/resolveWorkspacePath.test.tspackages/rangelink-vscode-extension/src/i18n/messages.en.tspackages/rangelink-vscode-extension/src/ide/vscode/VscodeAdapter.tspackages/rangelink-vscode-extension/src/navigation/FilePathNavigationHandler.tspackages/rangelink-vscode-extension/src/navigation/RangeLinkNavigationHandler.tspackages/rangelink-vscode-extension/src/types/MessageCode.tspackages/rangelink-vscode-extension/src/types/ResolvedPath.tspackages/rangelink-vscode-extension/src/types/index.tspackages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts
✅ Files skipped from review due to trivial changes (4)
- .prettierignore
- eslint.config.mjs
- packages/rangelink-vscode-extension/src/i18n/messages.en.ts
- packages/rangelink-vscode-extension/src/tests/navigation/FilePathNavigationHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rangelink-vscode-extension/src/types/index.ts
- packages/rangelink-vscode-extension/src/tests/ide/vscode/VscodeAdapter.test.ts
- packages/rangelink-vscode-extension/src/navigation/FilePathNavigationHandler.ts
- packages/rangelink-vscode-extension/src/tests/utils/resolveWorkspacePath.test.ts
- packages/rangelink-vscode-extension/src/types/ResolvedPath.ts
added 2 commits
March 24, 2026 23:47
Bare filenames like `index.ts` bypassed the ambiguity check because the workspace-relative loop ran first — resolving `<workspace>/index.ts` immediately even when `src/index.ts` also existed. Moving the `findFiles` check before the workspace-relative loop ensures bare filenames always go through ambiguity detection. The bare-filename block now terminates with `return undefined` so bare filenames never fall through to the workspace-relative loop. Paths with directory separators are unaffected. Benefits: - `index.ts#L1` correctly reports "Multiple files match" when ambiguous - Bare filenames that exist only at workspace root resolve via filename-fallback (not workspace-relative) - 4 new regression tests verify the reorder's behavioral effects Ignored Feedback: - Move findFiles behind adapter boundary: `resolveWorkspacePath` is already called *by* `VscodeAdapter.resolveWorkspacePath()` — it's the adapter's implementation, not handler code. P004 targets direct vscode.* calls in business logic. Ref: #497 (review)
The existing test verified the warning message but didn't prove that navigation was actually prevented. Added `not.toHaveBeenCalled()` assertions for `findOpenUntitledFile` and `showTextDocument` to lock down the early return. Ref: #497 (review)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__tests__/utils/resolveWorkspacePath.test.ts (1)
184-184: Extract the fallback limit into a named constant.These
2literals are the ambiguity threshold, so the suite will drift if that cap changes in the resolver. A localAMBIGUITY_THRESHOLDkeeps the intent clear and avoids repeating the same semantic value through the block.As per coding guidelines "No magic numbers - Define named constants with SCREAMING_SNAKE_CASE for all numeric literals with semantic meaning."Suggested cleanup
+const AMBIGUITY_THRESHOLD = 2; + ... - expect(mockFindFiles).toHaveBeenCalledWith('**/auth.ts', undefined, 2); + expect(mockFindFiles).toHaveBeenCalledWith( + '**/auth.ts', + undefined, + AMBIGUITY_THRESHOLD, + );Also applies to: 195-195, 204-204, 227-227, 239-239, 250-250, 261-261, 272-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/utils/resolveWorkspacePath.test.ts` at line 184, Replace the repeated literal 2 used as the ambiguity/fallback limit in resolveWorkspacePath tests with a named constant (e.g. AMBIGUITY_THRESHOLD) declared at the top of the test file; update every expect(mockFindFiles).toHaveBeenCalledWith(..., 2) occurrence (and any other places using that semantic 2) to use AMBIGUITY_THRESHOLD so the intent is clear and the value is centralized for future changes.
🤖 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/__tests__/helpers/createMockWorkspace.ts`:
- Line 49: The shared mock currently returns a non-empty default for findFiles
(jest.fn().mockResolvedValue([{ fsPath: '/workspace/mock-file', scheme: 'file'
}])) which causes unrelated tests to hit the filename-fallback path; change the
default in the createMockWorkspace mock so findFiles resolves to an empty array
([]) and make tests that need a positive match explicitly override findFiles to
return the desired result; update references to the mock's findFiles behavior
where necessary so fallback-specific tests set findFiles on the returned mock
object.
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/utils/resolveWorkspacePath.test.ts`:
- Line 184: Replace the repeated literal 2 used as the ambiguity/fallback limit
in resolveWorkspacePath tests with a named constant (e.g. AMBIGUITY_THRESHOLD)
declared at the top of the test file; update every
expect(mockFindFiles).toHaveBeenCalledWith(..., 2) occurrence (and any other
places using that semantic 2) to use AMBIGUITY_THRESHOLD so the intent is clear
and the value is centralized for future changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d608e328-89b3-40c2-889d-df101a6a5a0a
📒 Files selected for processing (4)
packages/rangelink-vscode-extension/src/__tests__/helpers/createMockWorkspace.tspackages/rangelink-vscode-extension/src/__tests__/navigation/RangeLinkNavigationHandler.test.tspackages/rangelink-vscode-extension/src/__tests__/utils/resolveWorkspacePath.test.tspackages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rangelink-vscode-extension/src/utils/resolveWorkspacePath.ts
- packages/rangelink-vscode-extension/src/tests/navigation/RangeLinkNavigationHandler.test.ts
…ck resolveWorkspacePath The shared createMockWorkspace returned a single-match findFiles by default, causing unrelated tests to silently succeed via the filename-fallback path. Defaulting to [] makes fallback behavior opt-in. Three handler test describe blocks that use bare filenames now explicitly spy on resolveWorkspacePath. Ignored Feedback: - Extract AMBIGUITY_THRESHOLD constant in tests: the literal `2` in assertions is acceptable as a contract-freezing value per T003 spirit. Ref: #497 (review)
couimet
pushed a commit
that referenced
this pull request
Mar 25, 2026
…lenameOnlyNavigation Addressed CodeRabbit nitpicks and migrated the new PR #497 test file: - `clipboardPreservation.test.ts`: replaced 5 inline clipboard read+assert blocks with `assertClipboardChanged`/`assertClipboardRestored` helpers (kept separate `#L` assertion where needed) - `logBasedUiValidation.test.ts`: replaced `fs.unlinkSync` with `cleanupFiles`, removed `fs` import - `navigationToastSettings.test.ts`: replaced manual file creation with `createWorkspaceFile`, derive `testFilename` via `path.basename(uri.fsPath)` - `filePathNavigation.test.ts`: extracted `settle(1000)` into `NON_EXISTENT_PATH_SETTLE_MS` constant - `filenameOnlyNavigation.test.ts` (from PR #497): replaced inline `getWorkspaceRoot`, `navigateViaHandleLinkClick`, activation boilerplate with shared helpers (kept `fs.mkdirSync`/`fs.writeFileSync` for subdirectory file creation — not supported by `createWorkspaceFile`) Ignored Feedback: - dirtyBufferWarning spy assertion: can't spy on extension internals from integration tests - resetRangelinkSettings scope: behavioral change beyond refactor scope Ref: #499 (review)
4 tasks
couimet
added a commit
that referenced
this pull request
Mar 25, 2026
* [refactor] Create testEnv.ts — shared workspace root, activation, and timing constants First step in extracting shared utilities from the 14 integration test files. `testEnv.ts` provides `getWorkspaceRoot()` (duplicated in 11 files), `activateExtension()` (duplicated in 11 files), `settle()` (inline setTimeout pattern in 10+ files), and timing constants (`SETTLE_MS`, `TERMINAL_READY_MS`, `POLL_INTERVAL_MS`, `POLL_TIMEOUT_MS`) that were inconsistently named and valued across files. * [refactor] Create shared helper modules for integration tests Created 7 helper modules that extract duplicated patterns from the 14 integration test files: - `fileHelpers.ts`: createWorkspaceFile, openEditor, cleanupFiles, closeAllEditors - `clipboardHelpers.ts`: sentinel write/read/assert pattern (used in 3 files) - `terminalHelpers.ts`: createAndBindTerminal with settle wait - `editorHelpers.ts`: waitForActiveEditor (polling), selectAll - `logHelpers.ts`: createLogger with suite-scoped prefix - `settingsHelpers.ts`: loadSettingsProfile, resetRangelinkSettings (extracted from smartPadding) - `navigationHelpers.ts`: navigateViaHandleLinkClick debounce pattern (duplicated in navigationPrecision + navigationClamping) All exported from helpers/index.ts. No test files modified yet — migration happens in subsequent commits. * [refactor] Migrate smartPadding.test.ts to shared integration test helpers First test file migration — validates the helper API works in the VS Code host. Replaced 6 inline utility functions and 3 inline constants with imports from the shared helpers: `activateExtension()`, `getWorkspaceRoot()`, `settle()`, `createLogger()`, `loadSettingsProfile()`, `resetRangelinkSettings()`, `waitForActiveEditor()`, `cleanupFiles()`, `closeAllEditors()`. The test-specific `setupEditorPair` and `setupUntitledEditorPair` functions remain inline — they're coupled to the editor-to-editor binding flow. * [refactor] Migrate clipboardPreservation.test.ts and unbind.test.ts to shared helpers Replaced inline `getWorkspaceRoot`, activation boilerplate, file creation/cleanup, and timing constants with imports from the shared helpers. `clipboardPreservation` now uses `createAndBindTerminal`, `createWorkspaceFile`, `openEditor`, `writeClipboardSentinel`, `CLIPBOARD_SENTINEL`, `cleanupFiles`, `closeAllEditors`. `unbind` now uses `createWorkspaceFile`, `settle(TERMINAL_READY_MS)`, `cleanupFiles`, `closeAllEditors`. Both files dropped `fs` and `path` imports entirely. * [refactor] Migrate dirtyBufferWarning.test.ts and linkGeneration.test.ts to shared helpers Both files replaced inline `getWorkspaceRoot`, activation boilerplate, and file creation/cleanup with shared helper imports. `linkGeneration` switched from `string[]` file tracking to `Uri[]` with `cleanupFiles()`. Both files dropped `fs` and `path` imports entirely. * [refactor] Migrate 5 navigation test files to shared helpers Replaced inline `getWorkspaceRoot`, activation boilerplate, file creation/cleanup, timing constants, and the duplicated `navigateViaHandleLinkClick` function across all 5 navigation test files: - `navigationPrecision.test.ts`: replaced 78-line inline `navigateViaHandleLinkClick` + boilerplate with imports - `navigationClamping.test.ts`: same — identical `navigateViaHandleLinkClick` eliminated - `navigationToastSettings.test.ts`: replaced both `navigateViaHandleLinkClick` and inline `settle`/`SETTLE_MS` - `filePathNavigation.test.ts`: replaced file creation with `createWorkspaceFile`, cleanup with `cleanupFiles` - `untitledNavigation.test.ts`: replaced activation, `settle`, `closeAllEditors` (kept `navigateToUntitledLink` per-file — different pattern matching by URI) * [refactor] Migrate logBasedUiValidation.test.ts to shared helpers Replaced inline `getWorkspaceRoot`, activation boilerplate, `SETTLE_MS`/`settle`, and file creation with shared helper imports. Kept the test-specific `bindTerminal` and `openAndSelectLines` functions per-file — they have unique lifecycle management (terminal disposed in teardown). * Ran `fix` * Ran `fix` * Ignore for coverage * Handle `eslint-disable-line no-undef` * [fix] Restore dropped tests, add QA entries, fix eslint no-undef CI caught three issues from the navigation test migration: - `navigationClamping.test.ts` was missing tests 002–004 (accidentally rewritten with different names/assertions instead of preserving originals) - `navigationPrecision.test.ts` had two new `char-navigation` tests without matching QA YAML entries — added `char-navigation-001` and `char-navigation-002` entries with `automated: true` - `logHelpers.ts` was missing `eslint-disable-line no-undef` on the `console.log` call (present in the original inline version but dropped during extraction) - `jest.config.js` coverage exclusion widened from single `getLogCapture.ts` to all VS Code-dependent helpers except `logBasedUiAssertions.ts` * [PR feedback] Harden integration test helpers — collision, console, terminal leak, command constant Addressed 4 items from CodeRabbit review on PR #499: - `fileHelpers.ts`: added incrementing counter to `createWorkspaceFile` filename to prevent collision on fast repeated calls - `logHelpers.ts`: replaced `console.log` with `node:console` import — removes eslint-disable workaround - `terminalHelpers.ts`: wrapped `bindToTerminalHere` in try/catch to dispose terminal on failure — prevents leaks in test failures - `navigationHelpers.ts`: replaced string literal `'rangelink.handleDocumentLinkClick'` with `CMD_HANDLE_DOCUMENT_LINK_CLICK` constant Ignored Feedback: - resetRangelinkSettings scope change: the function was extracted exactly as it existed in smartPadding.test.ts — changing its semantics to enumerate all rangelink keys is a behavioral change beyond the refactor scope Ref: #499 (review) * [PR feedback] Use shared helpers in remaining test files + migrate filenameOnlyNavigation Addressed CodeRabbit nitpicks and migrated the new PR #497 test file: - `clipboardPreservation.test.ts`: replaced 5 inline clipboard read+assert blocks with `assertClipboardChanged`/`assertClipboardRestored` helpers (kept separate `#L` assertion where needed) - `logBasedUiValidation.test.ts`: replaced `fs.unlinkSync` with `cleanupFiles`, removed `fs` import - `navigationToastSettings.test.ts`: replaced manual file creation with `createWorkspaceFile`, derive `testFilename` via `path.basename(uri.fsPath)` - `filePathNavigation.test.ts`: extracted `settle(1000)` into `NON_EXISTENT_PATH_SETTLE_MS` constant - `filenameOnlyNavigation.test.ts` (from PR #497): replaced inline `getWorkspaceRoot`, `navigateViaHandleLinkClick`, activation boilerplate with shared helpers (kept `fs.mkdirSync`/`fs.writeFileSync` for subdirectory file creation — not supported by `createWorkspaceFile`) Ignored Feedback: - dirtyBufferWarning spy assertion: can't spy on extension internals from integration tests - resetRangelinkSettings scope: behavioral change beyond refactor scope Ref: #499 (review) * Remove unused import
3 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
RangeLink now resolves bare filenames (without directory paths) when exactly one matching file exists in the workspace. This covers a common scenario where AI tools generate code references like
RangeLinkNavigationHandler.ts#L10without the directory prefix — previously these showed "Cannot find file", now they navigate directly.Changes
resolveWorkspacePath: when standard resolution fails and the path has no/or\separators, usesvscode.workspace.findFileswith a**/{filename}glob (capped at 2 results) to find a unique matchResolvedPathinterface withuriandresolvedViastrategy field ('absolute' | 'workspace-relative' | 'filename-fallback'), derived from the existingPathFormatenum to prevent string literal driftRangeLinkNavigationHandlerandFilePathNavigationHandlernow log the resolution strategy at DEBUG levelfilename-fallback-navigationslug (2 automated, 2 manual)filenameOnlyNavigation.test.tscovering unique-match navigation and standard-resolution control caseRelated
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests
Documentation