Conversation
Feat/search fuctionality
There was a problem hiding this comment.
Pull request overview
Adds a Pagefind-backed documentation search UI to the template and wires the build pipeline to generate a searchable index, along with a patch version bump + changelog entry.
Changes:
- Add a new
Searchmodal component and mount it in the header. - Update
buildscripts to run Pagefind indexing afterastro build, and addpagefindas a dev dependency. - Bump package version to
1.1.2and document the change inCHANGELOG.md.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| template/src/components/Search.astro | New search trigger + modal UI, client-side Pagefind init/search logic. |
| template/src/components/Header.astro | Replaces placeholder search input with the new Search component. |
| template/package.json | Adds pagefind devDependency and runs Pagefind after Astro build. |
| template/pnpm-lock.yaml | Adds lockfile for the template including Pagefind dependencies. |
| pnpm-lock.yaml | Adds Pagefind to the root lockfile. |
| package.json | Bumps version and updates build script; adds Pagefind devDependency. |
| CHANGELOG.md | Adds 1.1.2 entry for search. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let searchTimeout = null; | ||
|
|
||
| async function initPagefind() { | ||
| if (pagefind) return pagefind; | ||
| try { | ||
| pagefind = await import('/pagefind/pagefind.js'); | ||
| await pagefind.init(); | ||
| return pagefind; | ||
| } catch (e) { | ||
| console.warn('Pagefind not available. Run "pnpm build" to generate the search index.'); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
initPagefind() doesn’t cache an in-flight dynamic import/init. If the modal is opened multiple times quickly (or Cmd/Ctrl+K is pressed repeatedly) before the first import resolves, this can trigger multiple concurrent imports/inits. Consider caching a promise (e.g., pagefindPromise) so all callers await the same initialization work.
| let searchTimeout = null; | |
| async function initPagefind() { | |
| if (pagefind) return pagefind; | |
| try { | |
| pagefind = await import('/pagefind/pagefind.js'); | |
| await pagefind.init(); | |
| return pagefind; | |
| } catch (e) { | |
| console.warn('Pagefind not available. Run "pnpm build" to generate the search index.'); | |
| return null; | |
| } | |
| let pagefindPromise = null; | |
| let searchTimeout = null; | |
| async function initPagefind() { | |
| if (pagefind) return pagefind; | |
| if (!pagefindPromise) { | |
| pagefindPromise = (async () => { | |
| try { | |
| const module = await import('/pagefind/pagefind.js'); | |
| await module.init(); | |
| pagefind = module; | |
| return pagefind; | |
| } catch (e) { | |
| console.warn('Pagefind not available. Run "pnpm build" to generate the search index.'); | |
| pagefind = null; | |
| pagefindPromise = null; | |
| return null; | |
| } | |
| })(); | |
| } | |
| return pagefindPromise; |
| await pagefind.init(); | ||
| return pagefind; | ||
| } catch (e) { | ||
| console.warn('Pagefind not available. Run "pnpm build" to generate the search index.'); |
There was a problem hiding this comment.
The warning message hard-codes pnpm ("Run "pnpm build"..."). The README supports npm/yarn as well, so this message is misleading for non-pnpm users. Consider making the message package-manager-agnostic (e.g., "Run the build command to generate the search index") or referencing npm run build.
| console.warn('Pagefind not available. Run "pnpm build" to generate the search index.'); | |
| console.warn('Pagefind not available. Run the build command (e.g., "npm run build") to generate the search index.'); |
| function openModal() { | ||
| const modal = document.getElementById('search-modal'); | ||
| const input = document.getElementById('search-input'); | ||
| if (!modal || !input) return; | ||
|
|
||
| modal.classList.remove('hidden'); | ||
| document.body.style.overflow = 'hidden'; | ||
| input.focus(); | ||
| initPagefind(); | ||
| } | ||
|
|
||
| function closeModal() { | ||
| const modal = document.getElementById('search-modal'); | ||
| const input = document.getElementById('search-input'); | ||
| const resultsList = document.getElementById('search-results-list'); | ||
|
|
||
| if (!modal) return; | ||
|
|
||
| if (searchTimeout) clearTimeout(searchTimeout); | ||
|
|
||
| modal.classList.add('hidden'); | ||
| document.body.style.overflow = ''; | ||
|
|
There was a problem hiding this comment.
The modal sets aria-modal="true" and moves focus to the input on open, but it doesn’t restore focus to the trigger on close or trap focus within the dialog while open. This can strand keyboard/screen-reader users. Consider storing the previously focused element before opening and restoring it on close, and implementing a simple focus trap (Tab/Shift+Tab cycling) while the modal is visible.
|
|
||
| ### Patch Changes | ||
|
|
||
| - 820c898: Search fuctionality |
There was a problem hiding this comment.
Spelling: "Search fuctionality" should be "Search functionality".
| - 820c898: Search fuctionality | |
| - 820c898: Search functionality |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Main → Prod