-
Notifications
You must be signed in to change notification settings - Fork 163
Include the title of the file in the simple search #187
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
base: main
Are you sure you want to change the base?
Conversation
src/requestHandler.ts
Outdated
| for (const file of this.app.vault.getMarkdownFiles()) { | ||
| const cachedContents = await this.app.vault.cachedRead(file); | ||
| const result = search(cachedContents); | ||
| const result = search(file.basename + "\n\n" + cachedContents); |
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.
I believe the filename is already included in the response; see line 1017 below. Am I misunderstanding the result shape, or was there a reason that wasn't sufficient?
Nevermind, I see that this is instead adding this to the searched text. I'll think about this a little more when I have more time.
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.
Yes, see my last comments in the issue above :)
The problem is that searching for the file name doesn't yield matching files with that name. However this is important.
My change makes sure the filename is also respected in the search.
Thanks, take your time and let me know when you need anything :)
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.
I think it's a great idea to make /search/simple/ also match filenames, but there are a couple things we'll need to work together to fix before this could land in main:
- If you look below around line 1006,1007, you'll see that we include the
matchposition within the document in our response. Before these changes, the document we're searching within is the content of the markdown file, and folks could be using those returnedstart/endpositions to figure out where to make changes to the document. After your changes, though, that returned position no longer matches the position of the match within the document, and if they have some kind of script for making changes based on where the match is found, suddenly their tooling will be modifying the wrong position in the document. - Related to that, there isn't really a match range to give if you've matched the file's basename. Maybe instead of just returning
match: {start, end}, you could change the API such that it returned something likematch: {start, end, source}wheresourcecould be eithercontentorbasename?
I'm curious to hear what you think about either of those
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.
Thanks for pointing this out!
- Should be easy to fix by just adding 3 to the start and end position (first line is headline, then 2 linebreaks)
- That's a good idea, or we return 0 for start and end and document that this means, the match was in the headline.
I will look into this as soon as I have some spare time :)
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.
Sorry for the delay, busy times :)
I made the changes, could you please take a look when you have some time? Thanks!
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.
I also discovered a bug with the contextLength, which caused the search result often to contain no context. That bug also exists in your main branch.
Details: When parseInt() fails, it returns NaN (instead of null). But the Nullish Coalescing Operator (??) only respects null or undefined. Thus contextLength was null most of the times and therefore the context field empty. I fixed that.
|
Hi @coddingtonbear, what do you think? Could we merge this by time? Do you need anything from my side? :) |
| if (match[0] == 0) { | ||
| // When start position is between 0 and positionOffset, that means the search term matched within the headline. | ||
| contextMatches.push({ | ||
| match: { | ||
| start: match[0], | ||
| end: match[1], | ||
| source: "filename" | ||
| }, | ||
| context: file.basename, | ||
| }); | ||
| } else { | ||
| // Otherwise, the match was in the content | ||
| contextMatches.push({ | ||
| match: { | ||
| start: match[0] - positionOffset, | ||
| end: match[1] - positionOffset, | ||
| source: "content" | ||
| }, | ||
| context: cachedContents.slice( | ||
| Math.max(match[0] - contextLength, positionOffset), | ||
| match[1] + contextLength | ||
| ), | ||
| }); | ||
| } |
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.
There's a lot of possibility for off-by-one types of errors and other kinds of bugs in this kind of logic; could I trouble you to writing some tests covering this?
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.
Thanks, I really appreciate your in-depth review. I will add more tests in the next days. Have a great sunday!
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.
Pull Request Overview
This PR enhances the search functionality to include file names in the search scope and distinguish between filename and content matches. The implementation prepends file basenames to the search text and handles match position offsets accordingly.
- Added a
sourcefield toSearchContextto distinguish between filename and content matches - Modified search logic to prepend file basename to searchable text and adjust match positions
- Fixed several formatting issues (whitespace, spacing) in the mock files
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/types.ts | Added source field to SearchContext interface to distinguish match origin |
| src/requestHandler.ts | Enhanced search to include filenames and differentiate match sources; fixed contextLength parsing bug |
| mocks/obsidian.ts | Fixed spacing/formatting inconsistencies; added basename property to TFile mock |
| docs/openapi.yaml | Updated API documentation with descriptions and new source field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match[1] + contextLength | ||
| ), | ||
| }); | ||
| if (match[0] == 0) { |
Copilot
AI
Nov 4, 2025
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 condition match[0] == 0 only checks if the match starts exactly at position 0, but the comment states 'between 0 and positionOffset'. This logic is incorrect for matches that occur partially within the filename. A match starting at position 1 through (positionOffset - 1) would be incorrectly classified as content matches. The condition should be match[0] < positionOffset to correctly identify all filename matches.
| if (match[0] == 0) { | |
| if (match[0] < positionOffset) { |
| // We added the headline to the search text with 2 line breaks. | ||
| // That causes the start and end position numbers to be wrong with an offset | ||
| // of the char length of the headline line breaks. | ||
| // This is fixed by substracting the positionOffset from the start and end position. |
Copilot
AI
Nov 4, 2025
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.
Corrected spelling of 'substracting' to 'subtracting'.
| // This is fixed by substracting the positionOffset from the start and end position. | |
| // This is fixed by subtracting the positionOffset from the start and end position. |
| match[1] + contextLength | ||
| ), | ||
| }); | ||
| if (match[0] == 0) { |
Copilot
AI
Nov 4, 2025
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.
Use strict equality operator === instead of == for type-safe comparison.
| if (match[0] == 0) { | |
| if (match[0] === 0) { |
| contextMatches.push({ | ||
| match: { | ||
| start: match[0], | ||
| end: match[1], |
Copilot
AI
Nov 4, 2025
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.
When a match occurs within the filename, the start and end positions should be adjusted to reflect positions within the basename only. Currently, start is always 0 for filename matches, but the end position (match[1]) includes the offset and should be adjusted to match[1] to represent the actual match extent within the basename string.
| end: match[1], | |
| end: Math.min(match[1], file.basename.length), |
... also some smaller formatting fixes that the IDE did.
See #172