Add PageFind beta V2 (Full Text Search across Site)#2857
Add PageFind beta V2 (Full Text Search across Site)#2857MoshiMoshiMochi wants to merge 36 commits intoMarkBind:masterfrom
Conversation
This added support for configuring Pagefind search indexing via site.json is aimed towards allowing users to declare glob patterns to customize what pages in which directory should be indexed by pagefind.
Initially, I felt that including the root_selector option and force_language option would be useful for pagefind configuration options. But perhaps for now it isn't the most pressing issue so I'm going to exlcude it from the current implementation of pagefind.
This helps resolve an issue with generate() test case within
Site.functional.test.ts. Namely, because Jest's cannot intercept
eval('import("pagefind")') runtime dynamic import used by pagefind.
Hence, for now at least, the error handling would catch this error and
allow the site generation to continue normally.
Extracted processResult into its own helper function. Added rel="noopener noreferrer" attribute for improved security and privacy when being redirected to pagefind's github repo.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2857 +/- ##
==========================================
+ Coverage 71.99% 72.45% +0.46%
==========================================
Files 132 134 +2
Lines 7352 7486 +134
Branches 1633 1635 +2
==========================================
+ Hits 5293 5424 +131
- Misses 2053 2056 +3
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi guys, sorry for the wait, marking this as ready to review! As mentioned in the PR description, while I believe there are still plenty of things that we can work on to enhancing the Pagefind feature in the future. Namely, we could take a look at
We can probably make separate issues for these things. The current implementation is far from perfect (e.g. a lot of css can be improved, clearly still an issue when using at different browser sizes, also a lot of z level issues with the navbar & nav components). So let me know of what yall think of the current implementation, anything to improve, anything pressing to fix, etc. @MarkBind/active-3281-members |
There was a problem hiding this comment.
Pull request overview
Adds Pagefind-based full-text search to MarkBind as a beta feature, including build-time indexing, asset injection into generated pages, a new Vue <search /> modal UI, and supporting configuration/docs/test updates.
Changes:
- Introduce a new Vue
Searchcomponent (Pagefind UI modal) and styling assets. - Generate and inject Pagefind search assets during site generation, with
site.jsonsupport forpagefind.exclude_selectorsandpagefind.glob. - Update unit/functional tests, expected HTML outputs, and tooling ignores to accommodate generated Pagefind artifacts.
Reviewed changes
Copilot reviewed 128 out of 130 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vue-components/src/pagefindSearchBar/assets/search.css | Adds global CSS for the Pagefind search modal styling. |
| packages/vue-components/src/pagefindSearchBar/Search.vue | New Pagefind-backed search modal component + keyboard handling and UI overrides. |
| packages/vue-components/src/pagefindSearchBar/LogoPagefind.vue | Adds Pagefind logo SVG for the modal footer. |
| packages/vue-components/src/index.js | Registers the new Search component for <search /> usage. |
| packages/vue-components/src/tests/Search.spec.js | Adds unit tests for the new Search component behavior. |
| packages/core/test/unit/utils/data.ts | Adds factories/helpers for mocking Pagefind and generating site.json with Pagefind config. |
| packages/core/test/unit/Site/SiteGenerationManager.test.ts | Adds unit tests for Pagefind glob normalization/validation and indexing behavior. |
| packages/core/src/Site/SitePagesManager.ts | Injects Pagefind UI CSS/JS asset paths into page rendering assets when search is enabled. |
| packages/core/src/Site/SiteGenerationManager.ts | Adds Pagefind indexing during site generation and glob filtering support. |
| packages/core/src/Site/SiteConfig.ts | Extends site config to include optional pagefind configuration. |
| packages/core/src/Page/page.njk | Injects Pagefind UI stylesheet/script into generated pages when available. |
| packages/core/src/Page/PageConfig.ts | Adds pagefindCss/pagefindJs to PageAssets type. |
| packages/core/package.json | Adds pagefind dependency for build-time indexing. |
| packages/cli/test/functional/test_site_templates/test_project/expected/userGuide/UserGuide.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/userGuide/QuickStart.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/userGuide/Features.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/userGuide/FAQ.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/team/johndoe.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/team/AboutUs.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/TracingCode.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/Testing.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/SettingUp.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/Requirements.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/Implementation.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/Documentation.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/DeveloperGuide.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/DevOps.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/Design.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_project/expected/developerGuide/Configuration.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_portfolio/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_minimal/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_default/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic3b.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic3a.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic2.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic1.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_templates/test_default/expected/404.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_table_plugin/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_special_tags/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/test_folder/extra_3.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/test_folder/extra_2.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/test_folder/extra_1.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/contents/topic3b.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/contents/topic3a.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/contents/topic2.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/contents/topic1.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/about.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/README.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/Page-1.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/Home.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_navigation_convert/expected/404.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/contents/topic3b.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/contents/topic3a.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/contents/topic2.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/contents/topic1.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/about.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/_Sidebar.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/_Footer.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/Page-1.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/Home.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_convert/test_basic_convert/expected/404.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site_algolia_plugin/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/test_md_fragment.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testWeb3FormPlugin.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testVariableContainsInclude.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testTree.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testTooltipSpacing.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testThumbnails.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testSourceContainScript.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testSingleAltFrontMatter.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPopovers.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPopoverTrigger.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPlantUML.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPanelsClosingTransition.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPanels.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPanelMarkdownParsing.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPageNavWithoutTitleAndNavHeadings.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPageNavWithOnlyTitle.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPageNavTarget.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPageNavPrint.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testPageNav.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testOcticonInPage.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testNunjucksPathResolving.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testModals.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testMermaid.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testMath.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testMaterialIconsInPage.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testList.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testLinks.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testLayoutsWithAltFrontMatter.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testLayoutsOverrideWithAltFrontmatter.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testLayoutsOverride.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testLayouts.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testIncludePluginsRendered.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testIncludeMultipleModals.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testIncludeBoilerplate.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testImages.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testIconsInSiteLayout.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testHr.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testGlyphiconInPage.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testFontAwesomeInPage.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testExternalScripts.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testEmptyFrontmatter.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testEmptyAltFrontMatter.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testDates.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testCodeBlocks.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testCenterText.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testBootstrapIconInPage.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testAntiFOUCStyles.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testAnnotate.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testAnchorGeneration.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testAltFrontMatterParsing.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/testAltFrontMatterInvalidKeyValue.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/sub_site/testNunjucksPathResolving.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/sub_site/nested_sub_site/testNunjucksPathResolving.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/sub_site/nested_sub_site/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/sub_site/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/test_site/expected/bugs/index.html | Updates expected HTML to include Pagefind UI assets. |
| packages/cli/test/functional/testUtil/compare.ts | Adds ignore support for generated Pagefind outputs in functional comparisons. |
| packages/cli/test/functional/test.ts | Uses new compare() signature to ignore generated pagefind/ directories. |
| package-lock.json | Locks Pagefind and platform-specific optional dependencies. |
| docs/userGuide/makingTheSiteSearchable.md | Documents Pagefind beta usage and new site.json options. |
| .stylelintrc.js | Adds stylelint override for Pagefind UI class naming patterns. |
| .gitignore | Ignores generated Pagefind directories and fragments. |
| .eslintrc.js | Adds ESLint ignore patterns for generated/build output directories. |
| .eslintignore | Ignores generated Pagefind JS under functional test outputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| watch(showModal, (val) => { | ||
| if (!val) observer.disconnect(); | ||
| }); |
There was a problem hiding this comment.
This watch(showModal, ...) is created inside the modal-open path and the stop handle is not saved/called. Opening the modal multiple times will register multiple watchers. Please store the stop function and call it (or use the outer watcher’s onCleanup) when the modal closes.
There was a problem hiding this comment.
Just adding on to say if this watcher is used only once (which it seems to be, correct me if I'm wrong) for each open, you can use a once watcher which vue should cleanup automatically once used.
| container.addEventListener('mouseover', (e) => { | ||
| const resultLink = e.target.closest('.pagefind-ui__result-link'); | ||
| if (resultLink) { | ||
| // Remove active class from all other results | ||
| container.querySelectorAll('.pagefind-ui__result.is-active').forEach((el) => { | ||
| el.classList.remove('is-active'); | ||
| }); | ||
|
|
||
| // Add active class to the hovered one | ||
| const resultElement = resultLink.closest('.pagefind-ui__result'); | ||
| if (resultElement) { | ||
| resultElement.classList.add('is-active'); | ||
| } | ||
|
|
||
| // Move browser focus to the link (optional, but keeps Enter key logic synced) | ||
| resultLink.focus({ preventScroll: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
container.addEventListener('mouseover', ...) is added whenever the modal opens, but it’s never removed. Reopening the modal will register duplicate handlers. Please remove this listener when the modal closes (or attach it once and guard by showModal).
| container.addEventListener('click', (e) => { | ||
| const anchor = e.target.closest('a'); | ||
| if (anchor) { | ||
| // Close the modal before the browser navigates | ||
| showModal.value = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
container.addEventListener('click', ...) is added whenever the modal opens, but it’s never removed. This can cause duplicate close calls and unexpected behavior after multiple open/close cycles. Please remove this listener on close/unmount (or add it once).
| /* Hide scrollbar for Chrome, Safari and Opera */ | ||
| &::-webkit-scrollbar { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
The scoped CSS uses nesting syntax (&::-webkit-scrollbar), which is not valid plain CSS unless a nesting preprocessor is configured. Rewrite this as a full selector (e.g., .pagefind-ui__drawer::-webkit-scrollbar) or use a preprocessed <style lang="..."> that supports nesting.
| pagefindCss: this.siteConfig.enableSearch | ||
| ? path.posix.join(baseAssetsPath || '/', 'pagefind', 'pagefind-ui.css') | ||
| : undefined, | ||
| pagefindJs: this.siteConfig.enableSearch | ||
| ? path.posix.join(baseAssetsPath || '/', 'pagefind', 'pagefind-ui.js') |
There was a problem hiding this comment.
pagefindCss / pagefindJs are injected whenever enableSearch is true, but Pagefind assets are only written if indexSiteWithPagefind() succeeds. If indexing is skipped (e.g. import/runtime failure), pages will still reference /.../pagefind/pagefind-ui.(css|js) and cause 404s. Consider only injecting these when indexing succeeded, or provide always-present fallback assets.
| if (window.PagefindUI) { | ||
| // eslint-disable-next-line no-unused-vars | ||
| const pagefind = new window.PagefindUI({ | ||
| element: '#pagefind-search-input', | ||
| showSubResults: true, |
There was a problem hiding this comment.
A new PagefindUI instance is created every time the modal opens, but there’s no teardown/cleanup when the modal closes. Reopening the modal can duplicate the Pagefind UI DOM and compound internal event handlers. Consider initializing PagefindUI only once per component instance (cache the instance) or clearing the container / invoking a destroy method (if available) on close.
There was a problem hiding this comment.
I was considering mentioning this too, but I realize it's probably not that big of a deal. Unless you open the search component a million times.
I tested it myself, and it didn't make a noticeable difference at all after spamming. Modern garbage collection should handle this anyway right.
Premature optimization is the root of all weevils or something
(Though if you want to fix this it's probably not that difficult to just wrap the instance in a ref that you re-use)
| const observer = new MutationObserver(() => { | ||
| container.querySelectorAll('.pagefind-ui__result').forEach((el) => { | ||
| el.classList.remove('is-active'); | ||
| }); | ||
|
|
||
| const firstResult = container.querySelector('.pagefind-ui__result'); | ||
| if (firstResult) { | ||
| firstResult.classList.add('is-active'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
A MutationObserver is created when the modal opens, but its lifecycle is tied to an inner watcher created later. Consider managing the observer lifecycle in the outer watch(showModal, ...) (e.g., store it in a ref and disconnect on close) to avoid leaking observers across repeated open/close cycles.
| --font-sans: "Inter", --apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Oxygen, Ubuntu, Cantarell, Fira Sans, | ||
| Droid Sans, Helvetica Neue, sans-serif; |
There was a problem hiding this comment.
In the --font-sans stack, --apple-system looks like a typo for -apple-system (the standard system font family name on Apple platforms). As written, the intended macOS/iOS fallback likely won’t be used.
| await close(); | ||
| } catch (error) { | ||
| logger.warn(`Pagefind indexing skipped: ${error}`); | ||
| } |
There was a problem hiding this comment.
close() is called only on the success path. If any error occurs after getPagefind() succeeds (e.g. during createIndex, addDirectory, or writeFiles), execution jumps to catch and close() is skipped. Consider moving close() into a finally block (with a guard) so resources are always released.
| div [command-dialog-mask] { | ||
| background-color: rgba(0, 0, 0, 0.3); | ||
| height: 100vh; | ||
| left: 0; | ||
| position: fixed; |
There was a problem hiding this comment.
Selectors like div [command-dialog-mask] are very broadly scoped (any descendant with that attribute under any div), and this stylesheet is imported globally. To reduce the risk of style collisions, scope these selectors under a component root (e.g. .algolia / .search-dialog) and/or remove the leading div combinator.
There was a problem hiding this comment.
I think think the overall risk of this is kinda low.
- The CSS is only used in Search.vue (confirmed - no other consumers)
- Custom attributes like command-dialog-* are unlikely to be used by users
Hence, the risk of actual collision is low in practice.
Anyways, since we are planning on doing an overhaul to the structure of the pagefind search results, we'll probably be updating a lot of the css classes to fit our case, and this custom css file will eventually be faded out.
Harjun751
left a comment
Harjun751
left a comment
There was a problem hiding this comment.
Huge effort, and it definitely paid off!
Requesting some changes/clarifications for the
- debug.log file
- empty glob parsing
- windows directory safety check
- spawned watcher
issues, but other than that it's all minor nits. After the above I think we'll be in a good state.
| To add the Pagefind search bar to your page, simply insert the following `div` where you want it to appear: | ||
|
|
||
| ```md | ||
| <search /> |
There was a problem hiding this comment.
nit: this code block and the description above (simply insert the following div...) doesn't seem to match?
To make it clearer, maybe we could re-phrase to "insert the following element"?
There was a problem hiding this comment.
Good spot haha, I copied it from gerteck's PR and forgot to change it when I switched 🫠
|
|
||
| The following UI will be rendered, which is provided by Pagefind: | ||
|
|
||
| <search /> |
There was a problem hiding this comment.
This looks super clean in the deploy preview 👍
|
|
||
| ## Using External Search Services | ||
|
|
||
| MarkBind sites can use Algolia Doc Search services easily via the Algolia plugin. Unlike the built-in search, Algolia provides full-text search. See the panel below for more info. |
There was a problem hiding this comment.
Reading this, my impression was that Pagefind does not include full-text search.
Is built-in search different from pagefind search? If so, maybe we can clarify that built-in search can't do full-text while pagefind/algolia can.
If you think this is obvious/given feel free to ignore.
There was a problem hiding this comment.
Valid point. While it's currently far from the final product, I believe the goal of the pagefind search is to replace our default built-in search.
But since we've yet to nuke the current built-in search, I think we should keep the text as is. I'll try to update the documentation to be a little clearer that pagefind search and the built-in search are 2 separate things
| : path.posix.join(baseAssetsPath, 'js', 'vue.global.prod.min.js'), | ||
| layoutUserScriptsAndStyles: [], | ||
| pagefindCss: this.siteConfig.enableSearch | ||
| ? path.posix.join(baseAssetsPath || '/', 'pagefind', 'pagefind-ui.css') |
There was a problem hiding this comment.
The other assets in this file don't use the root / directory and instead always use baseAssetsPath.
Is there a reason this is required? Perhaps we could document it if so.
| watch(showModal, (val) => { | ||
| if (!val) observer.disconnect(); | ||
| }); |
There was a problem hiding this comment.
Just adding on to say if this watcher is used only once (which it seems to be, correct me if I'm wrong) for each open, you can use a once watcher which vue should cleanup automatically once used.
| const createIndexOptions: Record<string, unknown> = { | ||
| keepIndexUrl: true, | ||
| verbose: true, | ||
| logfile: 'debug.log', |
There was a problem hiding this comment.
This now generates a debug.log in the same directory that markbind build is called. Might be more appropriate to use _markbind/logs?
| return false; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Comment from Claude:
isValidGlobPattern doesn't block Windows-style absolute paths (SiteGenerationManager.ts:875–887)
The check normalizedPattern.startsWith('/') catches Unix absolute paths after backslash normalization, but
Windows absolute paths like C:\Users\... → C:/Users/... won't be caught since they don't start with /.
if (normalizedPattern.startsWith('/')) {
return false; // catches /etc/passwd
// misses C:/Windows/...
}
Fix: Also check for drive letter + colon pattern: /^[a-zA-Z]:\//.test(normalizedPattern).
I think this is valid as we support Windows too?
| totalPageCount = await this.indexAllHtmlFiles(index); | ||
| } | ||
| } else { | ||
| totalPageCount = await this.indexAllHtmlFiles(index); |
There was a problem hiding this comment.
If a user passes an empty glob [] with the intention to explicitly not index any files, then the execution will reach here and index all files anyway, since the length of the array is zero.
Perhaps we should differentiate undefined globs and defined empty globs to ensure this doesn't happen?
(There is probably not many situations in which you'd want to pass an empty glob over not defining anything, but it still might be worth fixing to ensure consistency. Also maybe if the user is testing)
There was a problem hiding this comment.
Good idea, I didn't consider this as a potential factor. But if they really wanted to pass in an empty glob, then they could just site enableSearch in site.json to false. I believe I do log the results such that if a user chooses an empty glob [ ], it will inform the user that 'All glob patterns were invalid, falling back to indexing all HTML files'
Also as I'm typing this now, I think the users can technically just pass in glob pattern to a non-existent file and pagefind would only attempt to index that non-existent file, but since it doesn't exist, it ends up indexing no files at all. This is because, I'm not actually doing any validation on whether the glob pattern exists when indexing the site, rather I'm just checking if the glob pattern is a valid one.
Though that does kinda reminds me. Since the instantiation of pagefind search and our built-in search are both tied to enableSearch, the only way for users to stop pagefind from indexing would be to either pass in an empty glob or disable enableSearch which would also disable the built-in search. So I guess since pagefind is still considered a beta feature, perhaps instead of directly tying pagefind to enableSearch, how about I create another option in site.json for the user to enable beta features like pagefind. Or I could just have it so that users have to declare the pagefind option within site.json for it to be instantiated during the build. Curious to see what yall think. TBH, I don't think its a big issue, it might just add a few extra seconds to the build time for very large sites, but I think it would be nice for us to provide a way to toggle pagefind since it is still currently a beta feature
@MarkBind/active-3281-members curious to see what you think
| */ | ||
| // eslint-disable-next-line class-methods-use-this | ||
| protected async getPagefind() { | ||
| // TODO: Update this dynamic import to a static import when migrating to ESM |
Now indexSiteWithPagefind returns a boolean which is then determines if there is a need to inject the pagefindcss & pagefindJs.
Since isValidGlobPattern is only used by normalizeGlobPattern, changed it to private access. Now normalization of pattern happens solely in normalizeGlobPattern and validation is purely a validation method. Better separation of concerns. Also resolved a bug where the 404s required pages to be created before knowing if indexing succeeds by defaulting index success to true and only if it fails does the flag get set to false.
|
I tried dropping the default 
|
|
|
||
| MarkBind will automatically inject the necessary scripts and styles to render the search UI. | ||
|
|
||
| The following UI will be rendered, which is provided by Pagefind: |
There was a problem hiding this comment.
Yes, probably should update this comment since this new UI is not exxactly provided by PageFind but rather from your implementation 👍
|
Good first step introducing pagefind into MarkBind @MoshiMoshiMochi . The build-time integration (Node API, The main concern is in
This is workable for a first cut, but the cause-and-effect is opaque, where future contributors would need to understand both the PagefindUI internals and overrides to make changes safely. It also loads pagefind-ui.js (84.6KB) + pagefind-ui.css (14.5KB) on every page, when we only need pagefind.js (33.8KB). The long-term direction should be:
Suggest tracking this as follow-up issues:
I think it is okay to approve as a foundation if these follow-ups are tracked, since the build-time indexing is correct and the search does work end-to-end, just wanted to clarify on your architectural direction for the component moving forward! @MoshiMoshiMochi |
What is the purpose of this pull request?
Overview of changes:
Initial version of pagefind (based on #2771)
Original Search Issue is #205
No test cases yet.
Anything you'd like to highlight/discuss:
Search.vuecomponent<search />that renders the pagefind search modal.Heres how the modal currently looks vs Algolia's (CS2103 site)
Current implementation of pagefind.
Algolia's styling on CS2103 website
From the 2 images above, I believe that Algolia's styling is notably more structured in how it represents the search results and subresults, notably on how they group the search results.
The blog we used as reference for the styling formats the pagefindresults by processing the search results. More information on how to access the specific search results information can be found here.
pagefindconfiguration options withinsite.jsonto enable more flexible filtering options. Users can declareexclude_selectorsoption to exclude specific elements from being searchable.globoption to limit which pages are indexed by Pagefind.{ "pagefind": { "exclude_selectors": [".algolia-no-index", "[class*='algolia-no-index']"] "glob": ["**/devGuide/**", "**/userGuide/**"] } }exclude_selectors: tells Pagefind to exclude any element with thealgolia-no-indexclass from the searchable results. It works the same way as declaringdata-pagefind-ignore.glob: Pagefind will only index pages that are found insidedevGuideanduserGuidedirectories (including subdirectories).All in all, theres still a lot more things we can do to improve this pagefind feature (like page weighting, ranking, sorting etc). But this is all I've managed to conjure up thus far.
Let me know what yall think of these updates to the pagefind feature (there might be better ways to filter pages, this is just from my interpretation when reading through the pagefind docs). Let me know if I missed out on anything in particular or should be focusing my efforts more towards.
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Add base Pagefind with updated UI & extra filtering functionalities
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):