Conversation
|
Cursor Agent can help with this pull request. Just |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds server-side semantic “possible matches” for 404s: builds a search query from the pathname, runs semantic search, normalizes/deduplicates/sorts results into NotFoundMatch objects, returns them in loader 404 payloads, and renders them via FourOhFour/ErrorPage with an in-page Possible Matches section and optional remote fetch. Changes
Sequence DiagramsequenceDiagram
participant Browser
participant Router
participant SemanticSearch as SemanticSearchServer
participant ErrorBoundary
participant UI as FourOhFour
Browser->>Router: GET /non/existent/path
Router->>Router: notFoundQueryFromPathname(pathname)
Router->>SemanticSearch: getNotFoundSuggestions({request, pathname})
SemanticSearch-->>Router: { query, matches[] } or null
Router-->>Browser: throw 404 with possibleMatches & possibleMatchesQuery
Browser->>ErrorBoundary: receive 404 payload
ErrorBoundary->>UI: render FourOhFour(possibleMatches, possibleMatchesQuery)
UI-->>Browser: render 404 page + Possible Matches section (or placeholder)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cursor
bot
force-pushed
the
cursor/404-content-matching-284b
branch
from
cd6f0df to
95b7599
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/errors.tsx`:
- Around line 250-274: The component currently treats an empty
possibleMatchesProp array as valid data and prevents the client fetch/fallback;
update the guard in the useEffect and the possibleMatches selection so empty
arrays are treated as “no data”: in the useEffect that references
possibleMatchesProp, change the early-return to only short-circuit when
possibleMatchesProp is non-empty (e.g. if (possibleMatchesProp != null &&
possibleMatchesProp.length > 0) return) so fetcher.load runs for empty arrays,
and change the possibleMatches assignment (now possibleMatches =
possibleMatchesProp ?? fetchedMatches) to prefer fetchedMatches when
possibleMatchesProp is empty (e.g. possibleMatches = (possibleMatchesProp &&
possibleMatchesProp.length > 0) ? possibleMatchesProp : fetchedMatches),
touching symbols: possibleMatchesProp, useEffect, requestedQueryRef, fetcher,
fetchedMatches, possibleMatches.
In `@app/utils/not-found-suggestions.server.ts`:
- Around line 68-75: The self-suggestion check currently compares
normalizePathname(url) to resolvedPathname but normalizePathname does not remove
an origin, so absolute URLs slip through; update the logic in the loop that uses
normalizePathname(url), resolvedPathname, toUrlKey, and byUrl so you first
produce a stripped pathname for absolute URLs (e.g., detect if url is absolute
and use its URL.pathname, otherwise use the url as-is) then pass that stripped
pathname into normalizePathname and compare that normalizedPathname to
resolvedPathname before continuing; use the same normalized pathname when
creating the key/entry so absolute URLs are not suggested back.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/components/errors.tsxapp/root.tsxapp/routes/$.tsxapp/routes/$slug.tsxapp/routes/blog_/$slug.tsxapp/utils/not-found-matches.tsapp/utils/not-found-query.tsapp/utils/not-found-suggestions.server.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/errors.tsx (1)
263-281:⚠️ Potential issue | 🟡 MinorAllow client fallback when
possibleMatchesPropis empty.Line 264 short-circuits even when
possibleMatchesPropis an empty array, so the client fetch never runs andpossibleMatchesis pinned to[](Line 281). Treat empty arrays as “no data.”🛠️ Proposed fix
React.useEffect(() => { - if (possibleMatchesProp != null) return + if (Array.isArray(possibleMatchesProp) && possibleMatchesProp.length > 0) { + return + } if (!effectiveQuery) return if (requestedQueryRef.current === effectiveQuery) return requestedQueryRef.current = effectiveQuery void fetcher.load( `/resources/search?query=${encodeURIComponent(effectiveQuery)}`, ) }, [effectiveQuery, fetcher, possibleMatchesProp]) -const possibleMatches = possibleMatchesProp ?? fetchedMatches +const possibleMatches = + possibleMatchesProp && possibleMatchesProp.length > 0 + ? possibleMatchesProp + : fetchedMatches🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 263 - 281, The effect currently bails out when possibleMatchesProp is any non-null value, which prevents the client fetch when possibleMatchesProp is an empty array; change the guard in the React.useEffect to treat empty arrays as "no data" by only returning early when possibleMatchesProp is provided and non-empty — e.g. replace the line using possibleMatchesProp != null with a check that returns early only if possibleMatchesProp is not null/undefined and not an empty array (referencing possibleMatchesProp, effectiveQuery, requestedQueryRef, and fetcher.load), so the fetcher.load(...) runs when possibleMatchesProp === [].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/errors.tsx`:
- Around line 209-238: normalizeNotFoundUrl currently returns any trimmed string
when the value is neither relative nor http(s), which allows unsafe schemes
(e.g. javascript:) to pass through; update normalizeNotFoundUrl so it only
accepts relative paths (startsWith('/')) or http/https URLs (startsWith('http')
— parse and return pathname+search+hash) and returns an empty string for any
other scheme/value, and ensure callers like asNotFoundMatchFromResourceSearch
use the sanitized result (function names: normalizeNotFoundUrl,
asNotFoundMatchFromResourceSearch).
---
Duplicate comments:
In `@app/components/errors.tsx`:
- Around line 263-281: The effect currently bails out when possibleMatchesProp
is any non-null value, which prevents the client fetch when possibleMatchesProp
is an empty array; change the guard in the React.useEffect to treat empty arrays
as "no data" by only returning early when possibleMatchesProp is provided and
non-empty — e.g. replace the line using possibleMatchesProp != null with a check
that returns early only if possibleMatchesProp is not null/undefined and not an
empty array (referencing possibleMatchesProp, effectiveQuery, requestedQueryRef,
and fetcher.load), so the fetcher.load(...) runs when possibleMatchesProp ===
[].
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
bot
force-pushed
the
cursor/404-content-matching-284b
branch
from
0385bb7 to
fecc65d
Compare
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
app/utils/not-found-suggestions.server.ts (1)
68-69: Self-suggestion check still broken for absolute URLs (unresolved from prior review).
normalizePathnameonly strips query/fragment; it does not remove the origin. An absolute URL likehttps://kentcdodds.com/blog/missing-postcompares as-is againstresolvedPathname(/blog/missing-post) and will never match, so the missing URL can be suggested back to the visitor.The fix proposed in the previous review (using
new URL(url, 'https://kentcdodds.com').pathnamebefore normalizing) still applies here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/not-found-suggestions.server.ts` around lines 68 - 69, The self-suggestion check incorrectly compares absolute URLs because normalizePathname(url) doesn't strip the origin, so an absolute URL like "https://kentcdodds.com/..." won't equal resolvedPathname ("/..."); update the comparison in the loop (where normalizePathname and resolvedPathname are used) to first convert url to its pathname via new URL(url, 'https://kentcdodds.com').pathname, then pass that pathname into normalizePathname and compare to resolvedPathname so the missing URL is correctly recognized and skipped (preserve the existing continue behavior).app/components/errors.tsx (2)
209-227:urlextracted byasNotFoundMatchFromResourceSearchis passed to<a href>without scheme validation (unresolved from prior review).
asNotFoundMatchFromResourceSearchaccepts any trimmed string as a URL (line 212). Ajavascript:ordata:URL returned by the/resources/searchendpoint would flow directly intohrefon line 178 — the same XSS vector flagged in the previous review fornormalizeNotFoundUrl, which no longer exists.The previous fix proposal (allowing only relative paths and
http(s)://URLs; returning''for everything else) should be applied toasNotFoundMatchFromResourceSearch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 209 - 227, asNotFoundMatchFromResourceSearch currently accepts any trimmed string for url and returns it into href (XSS risk); update the url validation so only relative paths (starting with '/') or absolute http(s) URLs (matching /^https?:\/\//i) are allowed and return null if the url doesn't match those patterns. In practice, change the url extraction in asNotFoundMatchFromResourceSearch to check the scheme after trimming and only proceed when it is a safe relative or http(s) URL; for all other values set url empty/return null so the unsafe value never flows into anchor href.
250-258: EmptypossibleMatchesProparray pinspossibleMatchesto[], permanently blocking the client fallback (unresolved from prior review).
possibleMatchesProp != nullis truthy for[], so the effect short-circuits andfetcher.loadis never called. Combined with the??on line 268 (which only coalescesnull/undefined), an empty server-supplied array silently suppresses the client search entirely.The fix proposed in the previous review — treating empty arrays like
nullin both the effect guard and the merge — still applies here.Also applies to: 268-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 250 - 258, The effect currently bails out whenever possibleMatchesProp is non-null, which treats an empty array as a valid server result and prevents client fallback; update the guard in the React.useEffect so it only returns when possibleMatchesProp is a non-empty array (e.g. check possibleMatchesProp != null && possibleMatchesProp.length !== 0 before returning), and update the merge/selection logic that currently uses the nullish coalescing (the code referencing possibleMatches and the ?? fallback) to prefer possibleMatchesProp only when it is a non-empty array (e.g. use a length check rather than ??) so empty arrays are treated like null and allow client fetcher.load to run.
🧹 Nitpick comments (2)
app/components/errors.tsx (1)
72-82:Object.assign(heroProps, …)mutates the prop object during render.Mutating a prop in-place is non-idiomatic and fragile — any caller that reuses or memoizes the
heroPropsobject literal would see unexpected side-effects. Derive a new object instead.♻️ Proposed refactor
- if (possibleMatches?.length) { - Object.assign(heroProps, { - arrowUrl: '#possible-matches', - arrowLabel: 'Possible matches', - }) - } else if (articles?.length) { - Object.assign(heroProps, { - arrowUrl: '#articles', - arrowLabel: 'But wait, there is more!', - }) - } + const resolvedHeroProps: HeroSectionProps = possibleMatches?.length + ? { ...heroProps, arrowUrl: '#possible-matches', arrowLabel: 'Possible matches' } + : articles?.length + ? { ...heroProps, arrowUrl: '#articles', arrowLabel: 'But wait, there is more!' } + : heroPropsThen replace
{...heroProps}with{...resolvedHeroProps}in the JSX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 72 - 82, The code mutates the incoming heroProps via Object.assign during render; instead, derive a new object (e.g., resolvedHeroProps) that spreads heroProps and conditionally adds arrowUrl/arrowLabel based on possibleMatches and articles, then update the JSX to spread {...resolvedHeroProps} instead of {...heroProps}; locate the Object.assign calls on heroProps and replace them with a single creation of resolvedHeroProps and update any JSX using heroProps to use resolvedHeroProps.app/routes/blog_/$slug.tsx (1)
89-98: Unbounded semantic-search latency on the 404 hot path.
getNotFoundSuggestionsmakes two sequential outbound calls (getEmbedding+queryVectorize). Neither has an explicit timeout. For the first request to any new bad slug the 60 s private cache is cold, so a slow or stuck search service would delay the entire 404 response indefinitely — there is no deadline after which the suggestion fetch is abandoned and the page renders without it.Consider wrapping the call with a
Promise.racetimeout insidegetNotFoundSuggestions, or passing anAbortSignalderived from the request's signal:// In getNotFoundSuggestions, replace the bare call: - const results = await semanticSearchKCD({ query, topK }) + const timeout = new Promise<never>((_, reject) => + setTimeout(() => reject(new Error('suggestions timeout')), 1500), + ) + const results = await Promise.race([ + semanticSearchKCD({ query, topK }), + timeout, + ])The existing
catchblock already returnsnullon failure, so this requires no other changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/blog_/`$slug.tsx around lines 89 - 98, The 404 suggestion path can hang because getNotFoundSuggestions calls getEmbedding/queryVectorize (semanticSearchKCD) without any deadline; wrap the outbound semantic search in a bounded operation (either use Promise.race with a short timeout e.g. ~1500ms or pass an AbortSignal derived from the incoming request.signal) so the function returns quickly on slow services; specifically modify getNotFoundSuggestions to replace the direct call to semanticSearchKCD (or the chained getEmbedding/queryVectorize calls) with a timed/abortable invocation and keep the existing catch that returns null on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/components/errors.tsx`:
- Around line 209-227: asNotFoundMatchFromResourceSearch currently accepts any
trimmed string for url and returns it into href (XSS risk); update the url
validation so only relative paths (starting with '/') or absolute http(s) URLs
(matching /^https?:\/\//i) are allowed and return null if the url doesn't match
those patterns. In practice, change the url extraction in
asNotFoundMatchFromResourceSearch to check the scheme after trimming and only
proceed when it is a safe relative or http(s) URL; for all other values set url
empty/return null so the unsafe value never flows into anchor href.
- Around line 250-258: The effect currently bails out whenever
possibleMatchesProp is non-null, which treats an empty array as a valid server
result and prevents client fallback; update the guard in the React.useEffect so
it only returns when possibleMatchesProp is a non-empty array (e.g. check
possibleMatchesProp != null && possibleMatchesProp.length !== 0 before
returning), and update the merge/selection logic that currently uses the nullish
coalescing (the code referencing possibleMatches and the ?? fallback) to prefer
possibleMatchesProp only when it is a non-empty array (e.g. use a length check
rather than ??) so empty arrays are treated like null and allow client
fetcher.load to run.
In `@app/utils/not-found-suggestions.server.ts`:
- Around line 68-69: The self-suggestion check incorrectly compares absolute
URLs because normalizePathname(url) doesn't strip the origin, so an absolute URL
like "https://kentcdodds.com/..." won't equal resolvedPathname ("/..."); update
the comparison in the loop (where normalizePathname and resolvedPathname are
used) to first convert url to its pathname via new URL(url,
'https://kentcdodds.com').pathname, then pass that pathname into
normalizePathname and compare to resolvedPathname so the missing URL is
correctly recognized and skipped (preserve the existing continue behavior).
---
Nitpick comments:
In `@app/components/errors.tsx`:
- Around line 72-82: The code mutates the incoming heroProps via Object.assign
during render; instead, derive a new object (e.g., resolvedHeroProps) that
spreads heroProps and conditionally adds arrowUrl/arrowLabel based on
possibleMatches and articles, then update the JSX to spread
{...resolvedHeroProps} instead of {...heroProps}; locate the Object.assign calls
on heroProps and replace them with a single creation of resolvedHeroProps and
update any JSX using heroProps to use resolvedHeroProps.
In `@app/routes/blog_/`$slug.tsx:
- Around line 89-98: The 404 suggestion path can hang because
getNotFoundSuggestions calls getEmbedding/queryVectorize (semanticSearchKCD)
without any deadline; wrap the outbound semantic search in a bounded operation
(either use Promise.race with a short timeout e.g. ~1500ms or pass an
AbortSignal derived from the incoming request.signal) so the function returns
quickly on slow services; specifically modify getNotFoundSuggestions to replace
the direct call to semanticSearchKCD (or the chained getEmbedding/queryVectorize
calls) with a timed/abortable invocation and keep the existing catch that
returns null on failure.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/components/errors.tsxapp/root.tsxapp/routes/$.tsxapp/routes/$slug.tsxapp/routes/blog_/$slug.tsxapp/utils/not-found-matches.tsapp/utils/not-found-query.tsapp/utils/not-found-suggestions.server.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- app/utils/not-found-query.ts
- app/routes/$.tsx
- app/routes/$slug.tsx
- app/root.tsx
- app/utils/not-found-matches.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/errors.tsx (1)
72-82:Object.assign(heroProps, …)mutates the caller's argument and bypasses TypeScript's union narrowing.
HeroSectionPropshas a discriminated union onarrowUrl/arrowLabel, but adding them viaObject.assignat runtime side-steps the constraint. All current callers pass inline object literals so there's no observable bug today, but it's a footgun if a shared object is ever passed.♻️ Proposed refactor (spread instead of mutate)
- if (possibleMatches?.length) { - Object.assign(heroProps, { - arrowUrl: '#possible-matches', - arrowLabel: 'Possible matches', - }) - } else if (articles?.length) { - Object.assign(heroProps, { - arrowUrl: '#articles', - arrowLabel: 'But wait, there is more!', - }) - } + const resolvedHeroProps: HeroSectionProps = possibleMatches?.length + ? { ...heroProps, arrowUrl: '#possible-matches', arrowLabel: 'Possible matches' } + : articles?.length + ? { ...heroProps, arrowUrl: '#articles', arrowLabel: 'But wait, there is more!' } + : heroPropsThen replace
<HeroSection {...heroProps} />with<HeroSection {...resolvedHeroProps} />.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 72 - 82, The code mutates heroProps with Object.assign which bypasses TypeScript's discriminated-union narrowing on HeroSectionProps; instead, construct a new resolvedHeroProps using spread (e.g. const resolvedHeroProps = { ...heroProps, arrowUrl: '...', arrowLabel: '...' }) in each branch so you don't mutate the original object and preserve type safety, then pass resolvedHeroProps to <HeroSection {...resolvedHeroProps} /> (referencing heroProps, Object.assign, and HeroSectionProps/HeroSection in your changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/errors.tsx`:
- Around line 230-245: normalizeNotFoundUrl allows protocol-relative URLs like
"//evil.com" because the check if (url.startsWith('/')) returns true for both
"/path" and "//host"; update normalizeNotFoundUrl to reject protocol-relative
paths by only accepting a single-leading-slash path (e.g., require
startsWith('/') and not startsWith('//') or use a regex like /^\/(?!\/)/),
return '' for "//..." inputs, and mirror the same one-line guard in the other
copy (not-found-suggestions.server.ts) or move this logic into a shared function
(e.g., not-found-matches.ts) to remove duplication.
In `@app/utils/not-found-suggestions.server.ts`:
- Around line 31-44: normalizeNotFoundUrl currently treats any string starting
with '/' as safe, which lets protocol-relative URLs like "//evil.com/path"
through; update normalizeNotFoundUrl to reject protocol-relative URLs by
requiring a single leading slash (e.g. use check url.startsWith('/') &&
!url.startsWith('//') or equivalent regex like /^\/(?!\/)/), keep the existing
handling for absolute http(s) URLs, and ensure invalid or malformed inputs
return ''. Remove the duplicate copy in errors.tsx (the same
normalizeNotFoundUrl) and consolidate a single exported function in
not-found-matches.ts (export normalizeNotFoundUrl) and update callers to import
it instead of using their local copies.
---
Nitpick comments:
In `@app/components/errors.tsx`:
- Around line 72-82: The code mutates heroProps with Object.assign which
bypasses TypeScript's discriminated-union narrowing on HeroSectionProps;
instead, construct a new resolvedHeroProps using spread (e.g. const
resolvedHeroProps = { ...heroProps, arrowUrl: '...', arrowLabel: '...' }) in
each branch so you don't mutate the original object and preserve type safety,
then pass resolvedHeroProps to <HeroSection {...resolvedHeroProps} />
(referencing heroProps, Object.assign, and HeroSectionProps/HeroSection in your
changes).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/utils/not-found-suggestions.server.ts (1)
62-65: Prefer a dedicated sentinel to detect the local timeout rather than matching by message string.
error.message === 'Timeout'will silently suppress any error thrown bysemanticSearchKCDitself if it happens to use the same message (e.g., the underlying fetch library surfacing its own timeout). That swallowed error never reachesconsole.error, making production failures invisible.♻️ Proposed fix — use a class to distinguish the local timeout
+class LocalTimeoutError extends Error { + override name = 'LocalTimeoutError' +} + const timeoutPromise = new Promise<never>((_, reject) => { - timeoutId = setTimeout(() => reject(new Error('Timeout')), timeoutMs) + timeoutId = setTimeout(() => reject(new LocalTimeoutError('Timeout')), timeoutMs) })} catch (error: unknown) { - if (error instanceof Error && error.message === 'Timeout') return null + if (error instanceof LocalTimeoutError) return null console.error('Semantic search failed while rendering 404 suggestions', error) return null }Also applies to: 121-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/not-found-suggestions.server.ts` around lines 62 - 65, Replace the string-match timeout detection with a distinct sentinel error class or value so the local timeout can be reliably detected without clobbering real errors from semanticSearchKCD; create a LocalTimeoutError (or unique sentinel object) and use it when rejecting timeoutPromise (where timeoutId and timeoutPromise are defined) and then check for that sentinel (via instanceof or identity) instead of comparing error.message === 'Timeout' (also update the identical check referenced around the second occurrence at the other location).app/components/errors.tsx (1)
139-141:PossibleMatchesSectionunnecessarily re-sorts server data that is already sorted upstream.
getNotFoundSuggestionscallssortNotFoundMatches([...byUrl.values()], { priorities })before returning (line 127 ofnot-found-suggestions.server.ts), so server-provided matches are already sorted. However,PossibleMatchesSectioncallssortNotFoundMatches(matches)again on line 141 with no priorities argument, applying hardcoded defaults. This is a no-op for server data when all callers use default priorities (which they currently do), but it creates a fragile design: client-side data from/resources/searchgenuinely needs sorting since that endpoint doesn't sort, yet server data gets redundantly re-sorted. If a caller ever passes customprioritiestogetNotFoundSuggestions, they would be silently discarded here.Pass
prioritiesthrough from parent loaders when available to make the intent explicit and avoid redundant sorting:♻️ Proposed fix — forward priorities as an optional prop
function PossibleMatchesSection({ matches, query, + priorities, }: { matches: Array<NotFoundMatch> query?: string + priorities?: ReadonlyArray<string> }) { const q = typeof query === 'string' ? query.trim() : '' const searchUrl = q ? `/search?q=${encodeURIComponent(q)}` : '/search' - const sorted = sortNotFoundMatches(matches) + const sorted = sortNotFoundMatches(matches, { priorities })Pass
prioritiesthrough from error boundary when available (from loaders that callgetNotFoundSuggestions).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 139 - 141, PossibleMatchesSection is re-sorting matches with sortNotFoundMatches(matches) and discarding any upstream priorities from getNotFoundSuggestions; instead make PossibleMatchesSection accept an optional priorities prop (or read it from the parent loader/error boundary) and only call sortNotFoundMatches(matches, priorities) when priorities are present or when matches come from an unsorted client endpoint; update callers (error boundary / parent loader that calls getNotFoundSuggestions) to forward their priorities into PossibleMatchesSection so server-sorted data keeps its original ordering and client-side data still gets sorted appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/errors.tsx`:
- Around line 216-217: The imageUrl value is not being normalized like v.url;
update the code that computes imageUrlRaw to first coerce and trim v.imageUrl
and then pass it through normalizeNotFoundUrl (same as is done for v.url) so
imageUrlRaw uses normalizeNotFoundUrl(v.imageUrl?.trim() ?? '') before use; keep
imageAltRaw handling unchanged and ensure you reference normalizeNotFoundUrl,
v.imageUrl, and imageUrlRaw when making the change.
---
Nitpick comments:
In `@app/components/errors.tsx`:
- Around line 139-141: PossibleMatchesSection is re-sorting matches with
sortNotFoundMatches(matches) and discarding any upstream priorities from
getNotFoundSuggestions; instead make PossibleMatchesSection accept an optional
priorities prop (or read it from the parent loader/error boundary) and only call
sortNotFoundMatches(matches, priorities) when priorities are present or when
matches come from an unsorted client endpoint; update callers (error boundary /
parent loader that calls getNotFoundSuggestions) to forward their priorities
into PossibleMatchesSection so server-sorted data keeps its original ordering
and client-side data still gets sorted appropriately.
In `@app/utils/not-found-suggestions.server.ts`:
- Around line 62-65: Replace the string-match timeout detection with a distinct
sentinel error class or value so the local timeout can be reliably detected
without clobbering real errors from semanticSearchKCD; create a
LocalTimeoutError (or unique sentinel object) and use it when rejecting
timeoutPromise (where timeoutId and timeoutPromise are defined) and then check
for that sentinel (via instanceof or identity) instead of comparing
error.message === 'Timeout' (also update the identical check referenced around
the second occurrence at the other location).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/errors.tsxapp/utils/not-found-matches.tsapp/utils/not-found-suggestions.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils/not-found-matches.ts
app/components/errors.tsx
Outdated
| const imageUrlRaw = typeof v.imageUrl === 'string' ? v.imageUrl.trim() : '' | ||
| const imageAltRaw = typeof v.imageAlt === 'string' ? v.imageAlt.trim() : '' |
There was a problem hiding this comment.
imageUrl is not normalized through normalizeNotFoundUrl, unlike url.
Line 211 correctly pipes v.url through normalizeNotFoundUrl, but imageUrlRaw at line 216 is used as-is. While javascript: in <img src> doesn't execute in modern browsers, an unexpected scheme or absolute external URL from a compromised search response can still leak the user's IP/referrer via image load. Consistent sanitization keeps the surface predictable.
🛡️ Proposed fix
- const imageUrlRaw = typeof v.imageUrl === 'string' ? v.imageUrl.trim() : ''
+ const imageUrlRaw =
+ typeof v.imageUrl === 'string'
+ ? normalizeNotFoundUrl(v.imageUrl.trim())
+ : ''📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const imageUrlRaw = typeof v.imageUrl === 'string' ? v.imageUrl.trim() : '' | |
| const imageAltRaw = typeof v.imageAlt === 'string' ? v.imageAlt.trim() : '' | |
| const imageUrlRaw = | |
| typeof v.imageUrl === 'string' | |
| ? normalizeNotFoundUrl(v.imageUrl.trim()) | |
| : '' | |
| const imageAltRaw = typeof v.imageAlt === 'string' ? v.imageAlt.trim() : '' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/errors.tsx` around lines 216 - 217, The imageUrl value is not
being normalized like v.url; update the code that computes imageUrlRaw to first
coerce and trim v.imageUrl and then pass it through normalizeNotFoundUrl (same
as is done for v.url) so imageUrlRaw uses
normalizeNotFoundUrl(v.imageUrl?.trim() ?? '') before use; keep imageAltRaw
handling unchanged and ensure you reference normalizeNotFoundUrl, v.imageUrl,
and imageUrlRaw when making the change.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
2 participants
Add semantic search "Possible matches" to 404 pages, prioritizing blog posts, to improve user experience.
Note
Medium Risk
Touches multiple 404 loader/error-boundary paths and introduces a new server-side semantic search call (network/perf dependency), though it’s gated by request type, short timeouts, and fails open to the existing 404 UX.
Overview
404 pages now optionally show a new “Possible matches” section populated via server-side semantic search, with results sorted (blog/pages prioritized), deduped, and linked to an on-site search query derived from the missing pathname.
Not-found loaders (
routes/$.tsx,routes/$slug.tsx,routes/blog_/$slug.tsx) now fetch suggestions for HTML GET requests only, return them in 404 JSON payloads with short-lived private caching, and render them through an enhancedFourOhFour/ErrorPageflow (including hero CTA/anchor behavior).Written by Cursor Bugbot for commit 42e006f. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements