-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix search for files starting with periods (e.g., .rooignore, .gitignore) #6510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest" | ||
| import { Fzf } from "fzf" | ||
|
|
||
| // Test the fallback search logic for period-prefixed queries | ||
| describe("searchWorkspaceFiles period handling", () => { | ||
| it("should handle period-prefixed queries with fallback search", () => { | ||
| // Mock file data that would come from ripgrep | ||
| const mockFiles = [ | ||
| { path: ".rooignore", type: "file" as const, label: ".rooignore" }, | ||
| { path: ".gitignore", type: "file" as const, label: ".gitignore" }, | ||
| { path: ".env", type: "file" as const, label: ".env" }, | ||
| { path: "src/app.ts", type: "file" as const, label: "app.ts" }, | ||
| { path: "package.json", type: "file" as const, label: "package.json" }, | ||
| ] | ||
|
|
||
| // Test the fallback search logic directly | ||
| const query = ".rooignore" | ||
|
|
||
| // Create search items like the real function does | ||
| const searchItems = mockFiles.map((item) => ({ | ||
| original: item, | ||
| searchStr: `${item.path} ${item.label || ""}`, | ||
| })) | ||
|
|
||
| // Test fzf search first | ||
| const fzf = new Fzf(searchItems, { | ||
| selector: (item) => item.searchStr, | ||
| }) | ||
|
|
||
| let fzfResults = fzf.find(query).map((result) => result.item.original) | ||
|
|
||
| // If fzf doesn't return results for period-prefixed queries, use fallback | ||
| if (fzfResults.length === 0 && query.startsWith(".")) { | ||
| // Fallback: exact substring matching | ||
| const exactMatches = mockFiles.filter((item) => { | ||
| const searchStr = `${item.path} ${item.label || ""}` | ||
| return searchStr.toLowerCase().includes(query.toLowerCase()) | ||
| }) | ||
|
|
||
| // Sort by relevance | ||
| exactMatches.sort((a, b) => { | ||
| const aLabel = (a.label || "").toLowerCase() | ||
| const bLabel = (b.label || "").toLowerCase() | ||
| const queryLower = query.toLowerCase() | ||
|
|
||
| // Prioritize exact filename matches | ||
| if (aLabel === queryLower && bLabel !== queryLower) return -1 | ||
| if (bLabel === queryLower && aLabel !== queryLower) return 1 | ||
|
|
||
| // Then prioritize filename starts with query | ||
| if (aLabel.startsWith(queryLower) && !bLabel.startsWith(queryLower)) return -1 | ||
| if (bLabel.startsWith(queryLower) && !aLabel.startsWith(queryLower)) return 1 | ||
|
|
||
| // Finally sort by path length | ||
| return a.path.length - b.path.length | ||
| }) | ||
|
|
||
| fzfResults = exactMatches | ||
| } | ||
|
|
||
| // Should find the .rooignore file | ||
| expect(fzfResults.length).toBeGreaterThan(0) | ||
| expect(fzfResults[0].path).toBe(".rooignore") | ||
| expect(fzfResults[0].label).toBe(".rooignore") | ||
| }) | ||
|
|
||
| it("should prioritize exact matches over partial matches", () => { | ||
| const mockFiles = [ | ||
| { path: ".rooignore", type: "file" as const, label: ".rooignore" }, | ||
| { path: "src/.rooignore.backup", type: "file" as const, label: ".rooignore.backup" }, | ||
| { path: "docs/rooignore-guide.md", type: "file" as const, label: "rooignore-guide.md" }, | ||
| ] | ||
|
|
||
| const query = ".rooignore" | ||
|
|
||
| // Simulate fallback search | ||
| const exactMatches = mockFiles.filter((item) => { | ||
| const searchStr = `${item.path} ${item.label || ""}` | ||
| return searchStr.toLowerCase().includes(query.toLowerCase()) | ||
| }) | ||
|
|
||
| // Sort by relevance | ||
| exactMatches.sort((a, b) => { | ||
| const aLabel = (a.label || "").toLowerCase() | ||
| const bLabel = (b.label || "").toLowerCase() | ||
| const queryLower = query.toLowerCase() | ||
|
|
||
| // Prioritize exact filename matches | ||
| if (aLabel === queryLower && bLabel !== queryLower) return -1 | ||
| if (bLabel === queryLower && aLabel !== queryLower) return 1 | ||
|
|
||
| // Then prioritize filename starts with query | ||
| if (aLabel.startsWith(queryLower) && !bLabel.startsWith(queryLower)) return -1 | ||
| if (bLabel.startsWith(queryLower) && !aLabel.startsWith(queryLower)) return 1 | ||
|
|
||
| // Finally sort by path length | ||
| return a.path.length - b.path.length | ||
| }) | ||
|
|
||
| // The exact match should be first | ||
| expect(exactMatches.length).toBeGreaterThan(0) | ||
| expect(exactMatches[0].label).toBe(".rooignore") | ||
| }) | ||
|
|
||
| it("should handle .gitignore searches correctly", () => { | ||
| const mockFiles = [ | ||
| { path: ".gitignore", type: "file" as const, label: ".gitignore" }, | ||
| { path: "src/.gitignore", type: "file" as const, label: ".gitignore" }, | ||
| { path: "docs/gitignore-examples.md", type: "file" as const, label: "gitignore-examples.md" }, | ||
| ] | ||
|
|
||
| const query = ".gitignore" | ||
|
|
||
| // Simulate fallback search | ||
| const exactMatches = mockFiles.filter((item) => { | ||
| const searchStr = `${item.path} ${item.label || ""}` | ||
| return searchStr.toLowerCase().includes(query.toLowerCase()) | ||
| }) | ||
|
|
||
| // Sort by relevance | ||
| exactMatches.sort((a, b) => { | ||
| const aLabel = (a.label || "").toLowerCase() | ||
| const bLabel = (b.label || "").toLowerCase() | ||
| const queryLower = query.toLowerCase() | ||
|
|
||
| // Prioritize exact filename matches | ||
| if (aLabel === queryLower && bLabel !== queryLower) return -1 | ||
| if (bLabel === queryLower && aLabel !== queryLower) return 1 | ||
|
|
||
| // Then prioritize filename starts with query | ||
| if (aLabel.startsWith(queryLower) && !bLabel.startsWith(queryLower)) return -1 | ||
| if (bLabel.startsWith(queryLower) && !aLabel.startsWith(queryLower)) return 1 | ||
|
|
||
| // Finally sort by path length | ||
| return a.path.length - b.path.length | ||
| }) | ||
|
|
||
| // Should find .gitignore files | ||
| expect(exactMatches.length).toBeGreaterThan(0) | ||
| expect(exactMatches.some((result) => result.label === ".gitignore")).toBe(true) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -135,7 +135,40 @@ export async function searchWorkspaceFiles( | |||||||||||||||
| }) | ||||||||||||||||
|
|
||||||||||||||||
| // Get all matching results from fzf | ||||||||||||||||
| const fzfResults = fzf.find(query).map((result) => result.item.original) | ||||||||||||||||
| // The fzf library supports exact matching with quotes, but for queries starting | ||||||||||||||||
| // with special characters like periods, we need to handle them specially. | ||||||||||||||||
| // The issue is that fzf may not handle leading periods well in fuzzy matching. | ||||||||||||||||
| let fzfResults = fzf.find(query).map((result) => result.item.original) | ||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great implementation! The fallback logic is clean and only activates when needed. Consider adding a comment here explaining why this fallback is necessary (fzf's limitation with period-prefixed queries) to help future maintainers understand the rationale:
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| // If the original query didn't return results and starts with a period, | ||||||||||||||||
| // try alternative search strategies | ||||||||||||||||
| if (fzfResults.length === 0 && query.startsWith(".")) { | ||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered edge cases like "..." or ".."? These might need special handling to avoid unexpected behavior. You could add a check like:
Suggested change
|
||||||||||||||||
| // Try exact substring matching as a fallback for period-prefixed queries | ||||||||||||||||
| const exactMatches = allItems.filter((item) => { | ||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For large workspaces with thousands of files, this fallback search could potentially be optimized. Since it only triggers for period-prefixed queries that return no fzf results, the performance impact should be minimal. However, if performance becomes a concern in the future, you might consider:
Is the current performance acceptable for your use cases? |
||||||||||||||||
| const searchStr = `${item.path} ${item.label || ""}` | ||||||||||||||||
| return searchStr.toLowerCase().includes(query.toLowerCase()) | ||||||||||||||||
| }) | ||||||||||||||||
|
|
||||||||||||||||
| // Sort by relevance (exact filename matches first, then path matches) | ||||||||||||||||
| exactMatches.sort((a, b) => { | ||||||||||||||||
| const aLabel = (a.label || "").toLowerCase() | ||||||||||||||||
| const bLabel = (b.label || "").toLowerCase() | ||||||||||||||||
| const queryLower = query.toLowerCase() | ||||||||||||||||
|
|
||||||||||||||||
| // Prioritize exact filename matches | ||||||||||||||||
| if (aLabel === queryLower && bLabel !== queryLower) return -1 | ||||||||||||||||
| if (bLabel === queryLower && aLabel !== queryLower) return 1 | ||||||||||||||||
|
|
||||||||||||||||
| // Then prioritize filename starts with query | ||||||||||||||||
| if (aLabel.startsWith(queryLower) && !bLabel.startsWith(queryLower)) return -1 | ||||||||||||||||
| if (bLabel.startsWith(queryLower) && !aLabel.startsWith(queryLower)) return 1 | ||||||||||||||||
|
|
||||||||||||||||
| // Finally sort by path length (shorter paths first) | ||||||||||||||||
| return a.path.length - b.path.length | ||||||||||||||||
| }) | ||||||||||||||||
|
|
||||||||||||||||
| fzfResults = exactMatches.slice(0, limit) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Verify types of the shortest results | ||||||||||||||||
| const verifiedResults = await Promise.all( | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are comprehensive and cover the important scenarios well! However, I noticed that the test duplicates the implementation logic rather than testing the actual
searchWorkspaceFilesfunction. This approach works but has a maintenance risk - if the implementation changes, the tests might not catch regressions.Would it be possible to refactor these tests to mock the dependencies (like
executeRipgrepForFilesandfs.existsSync) and test the actualsearchWorkspaceFilesfunction? This would ensure the tests stay in sync with the implementation and provide better coverage.