Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughThis pull request removes the Algolia DocSearch integration and related assets (preconnect header, DocSearch styles, and module), adds FlexSearch and types, and introduces a build-time search index generator that writes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/tags/search-dialog/search-worker-client.ts (1)
24-24: Defer worker startup until search is actually used.Line 24 eagerly spins up the worker and starts the
/search-index.jsonfetch as soon as this module evaluates. Deferring that to the first open/query avoids front-loading the full index for visitors who never use search.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tags/search-dialog/search-worker-client.ts` at line 24, The module currently calls ensureWorker() at top-level which eagerly starts the worker and fetches /search-index.json; remove that top-level invocation and instead call ensureWorker() lazily from the user-facing entrypoints that open or query search (e.g., the functions that show the search dialog and run queries such as openSearchDialog, handleSearchQuery or performSearch—use the actual exported names in this file). Add a simple started/initializing guard around ensureWorker() so concurrent calls don't re-start the worker, and await ensureWorker() inside those entrypoints before triggering any fetches or worker messages so the index is only loaded when search is actually used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tags/app-header/app-header.marko`:
- Line 57: The header's two-way binding using search-dialog open:=searchOpen
breaks because search-dialog never calls input.openChange; update search-dialog
(src/tags/search-dialog/search-dialog.marko) to invoke input.openChange?.(false)
whenever the dialog closes internally (e.g., backdrop clicks, ESC/key handlers,
or any internal closeButton handlers) so searchOpen is kept in sync, or
alternatively stop using := and bind one-way (open=) plus an explicit close
handler in app-header if you prefer to avoid two-way binding until openChange
exists.
In `@src/tags/search-dialog/search-dialog.marko`:
- Around line 35-43: The current handler that awaits sendQuery(query) can apply
out-of-order responses; change the flow so only the latest query's response
updates results/activeIndex: either (A) modify sendQuery to accept and return a
unique requestId (or include the original query) and, in the script block, track
latestRequestId/latestQuery and only assign results/activeIndex when the
returned requestId/query matches the latest one, or (B) make sendQuery support
cancellation (e.g., via AbortController) and abort any previous pending request
before issuing a new sendQuery; reference the sendQuery function in
src/tags/search-dialog/search-worker-client.ts and the results/activeIndex
assignments in the script block to implement one of these defenses.
- Around line 63-72: The form currently uses method="dialog" which lets the
native dialog close before your onSubmit handler runs, causing state desync;
remove method="dialog" from the form element and update the onSubmit handler
(the anonymous onSubmit in the form tag that references results, activeIndex,
open, and query) to call event.preventDefault() at the top, then only perform
the navigation and set open = false and query = "" inside the existing if
(results && results[activeIndex]) block so the component state and the dialog
element remain in sync.
In `@src/tags/search-dialog/search-worker-client.ts`:
- Around line 26-38: sendQuery currently subscribes with { once: true } and
resolves on the next "results" message regardless of which query produced it;
change it to correlate responses by sending a unique request id (e.g., generate
a short uid inside sendQuery) with the postMessage and install a message
listener that checks e.data.type === "results" && e.data.requestId === id (or
e.data.query === q if you prefer matching query), resolving when matched and
then removing that listener; update sendQuery (and ensureWorker usage) to post {
type: "query", query: q, requestId: id } and make the listener remove itself
only after a matching response so overlapping queries don't interfere (note:
search-worker.ts already echoes query so mirror that behavior for requestId or
query matching).
In `@src/util/search-index-builder.ts`:
- Around line 49-65: The slug generation here must match the renderer's single
GithubSlugger sequence: seed and advance the same slugger for every heading in
document order instead of only calling slugger.slug when building sectionHref;
specifically, initialize GithubSlugger once (already present), call
slugger.slug(pageTitle) to account for the top-level heading and then, while
iterating the sections array, advance the same slugger for each heading
encountered (even if you don't use the returned value) so that when you compute
sectionHref you use the slug value returned for that heading from
slugger.slug(section.heading); keep references to GithubSlugger, slugger.slug,
sectionHref, sections and ensure the slugger state is advanced in the same order
the renderer visits headings.
In `@src/util/search-worker.ts`:
- Around line 154-158: The worker's "init" case must catch failures from
fetch()/res.json() and post an explicit error instead of never posting "ready";
wrap the fetch+json+init(blocks) sequence in a try/catch, only postMessage({
type: "ready" }) on success, and on error postMessage({ type: "init-error",
error: String(err) }) (include enough error text for debugging); update any
error paths in the "init" case that call init() to avoid throwing uncaught
exceptions so ensureWorker() (in search-worker-client.ts) can reject or recover
instead of hanging.
- Around line 83-84: The snippet clipping logic in src/util/search-worker.ts
incorrectly uses `content.indexOf(" ", end) || end`, which keeps -1 (truthy)
when no next space exists and thus trims the last character; change this to
explicitly check the indexOf result: call `content.indexOf(" ", end)` into a
temporary (e.g., nextSpace) and if nextSpace !== -1 set `end = nextSpace`,
otherwise set `end = content.length` (so the snippet runs to the end); do the
analogous explicit check for the `start` adjustment if needed to avoid relying
on `||` with -1.
---
Nitpick comments:
In `@src/tags/search-dialog/search-worker-client.ts`:
- Line 24: The module currently calls ensureWorker() at top-level which eagerly
starts the worker and fetches /search-index.json; remove that top-level
invocation and instead call ensureWorker() lazily from the user-facing
entrypoints that open or query search (e.g., the functions that show the search
dialog and run queries such as openSearchDialog, handleSearchQuery or
performSearch—use the actual exported names in this file). Add a simple
started/initializing guard around ensureWorker() so concurrent calls don't
re-start the worker, and await ensureWorker() inside those entrypoints before
triggering any fetches or worker messages so the index is only loaded when
search is actually used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b993727-04ec-4ea7-b34a-33237964d09e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (16)
.gitignorecspell.jsondocs/reference/core-tag.mdpackage.jsonskills-lock.jsonsrc/routes/+layout.markosrc/tags/app-header/app-header.markosrc/tags/app-header/app-header.style.module.scsssrc/tags/app-header/docsearch.scsssrc/tags/app-header/docsearch.tssrc/tags/search-dialog/search-dialog.markosrc/tags/search-dialog/search-dialog.style.module.scsssrc/tags/search-dialog/search-worker-client.tssrc/util/markodown.tssrc/util/search-index-builder.tssrc/util/search-worker.ts
💤 Files with no reviewable changes (3)
- src/routes/+layout.marko
- src/tags/app-header/docsearch.ts
- src/tags/app-header/docsearch.scss
Comment on lines
+26
to
+38
| export async function sendQuery(q: string): Promise<SearchHit[]> { | ||
| await ensureWorker(); | ||
| const result = new Promise<SearchHit[]>((resolve) => { | ||
| worker!.addEventListener( | ||
| "message", | ||
| (e: MessageEvent) => { | ||
| if (e.data.type === "results") resolve(e.data.results); | ||
| }, | ||
| { once: true }, | ||
| ); | ||
| }); | ||
| worker!.postMessage({ type: "query", query: q }); | ||
| return result; |
Contributor
There was a problem hiding this comment.
Correlate "results" messages to the query that sent them.
Every sendQuery() call resolves on the next "results" message, not its own response. If keystrokes fire overlapping queries, all pending promises can settle with the first result set and later responses are dropped. src/util/search-worker.ts already echoes query, but { once: true } means even a non-matching response would still remove the listener, so this really needs request ids or manual unsubscription after a match.
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 28-34: Message event listeners should validate the origin to prevent XSS attacks. Always check the event origin before processing the message.
Context: worker!.addEventListener(
"message",
(e: MessageEvent) => {
if (e.data.type === "results") resolve(e.data.results);
},
{ once: true },
)
Note: [CWE-346] Origin Validation Error [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html
- https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage
(event-origin-validation)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tags/search-dialog/search-worker-client.ts` around lines 26 - 38,
sendQuery currently subscribes with { once: true } and resolves on the next
"results" message regardless of which query produced it; change it to correlate
responses by sending a unique request id (e.g., generate a short uid inside
sendQuery) with the postMessage and install a message listener that checks
e.data.type === "results" && e.data.requestId === id (or e.data.query === q if
you prefer matching query), resolving when matched and then removing that
listener; update sendQuery (and ensureWorker usage) to post { type: "query",
query: q, requestId: id } and make the listener remove itself only after a
matching response so overlapping queries don't interfere (note: search-worker.ts
already echoes query so mirror that behavior for requestId or query matching).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Abandon Algolia in favor of flexsearch (this is what svelte.dev uses).
Also changed the styles for the search dialog