Include the title of the file in the simple search #187
Conversation
coddingtonbear
added
the
needs more discussion
|
Hi @coddingtonbear, what do you think? Could we merge this by time? Do you need anything from my side? :) |
src/requestHandler.ts
Outdated
| 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.
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.
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.
Update: I fixed all copilot comments in the MR. It found a bug, which is nice!
Also I added lot's of test cases. Tested: works like a charm:
$ curl -X 'POST' \
'http://127.0.0.1:27123/search/simple/?query=Master%20Plan' \
-H 'accept: application/json' \
-H 'Authorization: Bearer 320fac56a929e907fd1f724b12eb55327b38a363e8c8a8cbc290c9c5f1c30631'
[
[...],
{
"filename": "5 - Finanzen/1 - Master Plan.md",
"score": -1.2066,
"matches": [
{
"match": {
"start": 4,
"end": 10,
"source": "filename"
},
"context": "1 - Master Plan"
},
{
"match": {
"start": 11,
"end": 15,
"source": "filename"
},
"context": "1 - Master Plan"
}
]
}
]There was a problem hiding this comment.
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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Changed condition from match[0] === 0 to match[0] < positionOffset to correctly detect all matches within the filename, not just at position 0 - Added comprehensive test suite for searchSimplePost with 12 test cases covering filename matches, content matches, edge cases, and position calculations - Implemented functional prepareSimpleSearch mock for testing Fixes issue where files like '1 - Master Plan.md' were not found when searching for 'Master Plan' because the match didn't start at position 0. Addresses PR feedback: - Tests now cover off-by-one errors and boundary conditions - Match detection logic correctly identifies all filename matches - Position calculations verified through tests
2 participants
... also some smaller formatting fixes that the IDE did.
See #172