Conversation
There was a problem hiding this comment.
Pull request overview
Adds a site-wide note search experience to the Gatsby notes site, including a header search input with preview results and a dedicated /search results page.
Changes:
- Introduces a search page (
/search) that reads theqquery param and renders matching notes. - Adds a
useNoteSearchhook backed byjs-searchindexing over note metadata and excerpts. - Replaces the header’s static search UI with an interactive
SearchInputpreview popper and adds required dependencies.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/search.tsx | New search results page rendering matches from the search hook. |
| src/hooks/use-note-search.ts | New hook that builds a js-search index over note documents and returns matches. |
| src/components/search-input.tsx | New interactive header search input with preview results popper and navigation to /search. |
| src/components/header.tsx | Integrates SearchInput into the header and simplifies drawer toggle handler. |
| package.json | Adds js-search and TypeScript types (and updates devDependencies). |
| package-lock.json | Locks the added dependencies. |
| CLAUDE.md | Updates contributor/tooling guidance documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Re: filter in — 🤖 Claude |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
package.json:50
- Adding
@eslint/js@10.0.1introduces a Node engine requirement of^20.19.0 || ^22.13.0 || >=24(per the lockfile), which conflicts with this repo’s declared engine ofnode >=18.16.1. This can cause warnings or installation failures in environments pinned to Node 18. Consider either bumping the repo’sengines.noderange to match the new dependency requirements, or selecting dependency versions that still support Node 18.
"devDependencies": {
"@emotion/cache": "^11.14.0",
"@eslint/js": "^10.0.1",
"@graphql-eslint/eslint-plugin": "^4.0.0",
"@types/js-search": "^1.4.4",
"@types/node": "^24.0.0",
"@types/react": "^19.0.0",
"@types/react-dom": "^19.0.0",
"@typescript-eslint/eslint-plugin": "^8.0.0",
"@typescript-eslint/parser": "^8.0.0",
"eslint": "^10.0.0",
"gh-pages": "^6.0.0",
"prettier": "^3.0.0",
"typescript": "^6.0.0"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return useMemo(() => { | ||
| if (!isSearchEnabled) return []; | ||
|
|
||
| const documents = notes | ||
| .map(({ node }) => node as unknown as Note) | ||
| .filter( | ||
| (note) => | ||
| typeof note.frontmatter?.title === "string" && | ||
| note.frontmatter.title.trim() !== "" && | ||
| typeof note.fields?.slug === "string" && | ||
| note.fields.slug.trim() !== "", | ||
| ); | ||
|
|
||
| const search = new JsSearch.Search("id"); | ||
| search.addIndex(["frontmatter", "title"]); | ||
| search.addIndex(["frontmatter", "tags"]); | ||
| search.addIndex("excerpt"); | ||
| search.addDocuments(documents); | ||
|
|
||
| return search.search(query) as Note[]; | ||
| }, [isSearchEnabled, notes, query]); |
There was a problem hiding this comment.
useNoteSearch rebuilds the full js-search index whenever query changes because the memo depends on query and the index is created inside the memo. This can become noticeably expensive on each keystroke as the note list grows. Consider memoizing the Search instance (and addDocuments) based only on notes, then running search.search(query) in a separate memo derived from query (or just calling it directly) so indexing is not repeated per query update.
| return useMemo(() => { | |
| if (!isSearchEnabled) return []; | |
| const documents = notes | |
| .map(({ node }) => node as unknown as Note) | |
| .filter( | |
| (note) => | |
| typeof note.frontmatter?.title === "string" && | |
| note.frontmatter.title.trim() !== "" && | |
| typeof note.fields?.slug === "string" && | |
| note.fields.slug.trim() !== "", | |
| ); | |
| const search = new JsSearch.Search("id"); | |
| search.addIndex(["frontmatter", "title"]); | |
| search.addIndex(["frontmatter", "tags"]); | |
| search.addIndex("excerpt"); | |
| search.addDocuments(documents); | |
| return search.search(query) as Note[]; | |
| }, [isSearchEnabled, notes, query]); | |
| const documents = useMemo( | |
| () => | |
| notes | |
| .map(({ node }) => node as unknown as Note) | |
| .filter( | |
| (note) => | |
| typeof note.frontmatter?.title === "string" && | |
| note.frontmatter.title.trim() !== "" && | |
| typeof note.fields?.slug === "string" && | |
| note.fields.slug.trim() !== "", | |
| ), | |
| [notes], | |
| ); | |
| const search = useMemo(() => { | |
| const nextSearch = new JsSearch.Search("id"); | |
| nextSearch.addIndex(["frontmatter", "title"]); | |
| nextSearch.addIndex(["frontmatter", "tags"]); | |
| nextSearch.addIndex("excerpt"); | |
| nextSearch.addDocuments(documents); | |
| return nextSearch; | |
| }, [documents]); | |
| return useMemo(() => { | |
| if (!isSearchEnabled) return []; | |
| return search.search(query) as Note[]; | |
| }, [isSearchEnabled, query, search]); |
| if (!isoDateStr) return ""; | ||
| const date = new Date(isoDateStr); | ||
| if (isNaN(date.getTime())) return ""; | ||
| const diffDays = Math.floor((Date.now() - date.getTime()) / 86400000); |
There was a problem hiding this comment.
relativeDate treats future dates as "today" because diffDays becomes negative and hits the < 1 branch. If rawDate can be in the future (typos, scheduled posts, timezone parsing), this will display incorrect metadata; handle diffDays < 0 explicitly (e.g., return ""/"in Xd" or clamp to 0).
| const diffDays = Math.floor((Date.now() - date.getTime()) / 86400000); | |
| const diffDays = Math.floor((Date.now() - date.getTime()) / 86400000); | |
| if (diffDays < 0) return ""; |
| /> | ||
| </SearchContainer> | ||
| <ResultsPopper | ||
| open={open} |
There was a problem hiding this comment.
Popper is opened based on open, but anchorEl can be null during the initial render(s) (before the ref is set), which can lead to warnings/positioning issues. Consider gating open with Boolean(anchorRef.current) (or rendering the Popper only once the anchor is available).
| open={open} | |
| open={open && Boolean(anchorRef.current)} |
| "author": "Kayla Altepeter", | ||
| "engines": { | ||
| "node": ">=18.16.1", | ||
| "node": ">=24.15.0", |
There was a problem hiding this comment.
Bumping the supported Node engine from >=18.16.1 to >=24.15.0 is a major operational change and is significantly stricter than what Gatsby 5 itself declares (it supports >=18 <26 per the lockfile). If the project truly requires Node 24.15 specifically (vs 24.x / >=24), it would help to justify this in the PR or relax it to the minimum actually required so CI/deploy environments don’t break unnecessarily.
| "node": ">=24.15.0", | |
| "node": ">=18.16.1", |
Uh oh!
There was an error while loading. Please reload this page.