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:
📝 WalkthroughWalkthroughCentralize route-error handling in the error boundary; adjust error UI and 404 hero actions and visuals; reword not-found query comments; replace semantic-search 404 suggestions with a deterministic, cached filesystem-driven index and enrichment pipeline; change getNotFoundSuggestions to an options-object API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NotFound as getNotFoundSuggestions
participant IndexBuilder as getNotFoundDeterministicIndex
participant FS as Filesystem/MDX Parser
participant QueryGen as getQueryCandidates
participant Filter as filterIndexItems
participant Enricher as toNotFoundMatch
Client->>NotFound: call({ request, pathname, limit, priorities })
NotFound->>IndexBuilder: ensure cached index
IndexBuilder->>FS: read routes, blog posts, pages
FS-->>IndexBuilder: raw items (paths, frontmatter, thumbnails)
IndexBuilder-->>NotFound: deduplicated index
NotFound->>QueryGen: derive query candidates from pathname
QueryGen-->>NotFound: candidate queries
NotFound->>Filter: filter & rank index by candidates
Filter->>Enricher: enrich top candidates
Enricher->>FS: parse MDX/frontmatter, build thumbs if present
FS-->>Enricher: enriched metadata
Enricher-->>Filter: enriched matches
Filter-->>NotFound: final matches
NotFound-->>Client: { query, matches } or null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/error-boundary.tsx (1)
1-6:⚠️ Potential issue | 🟡 MinorImport ordering:
clsxshould precedereact-router.ESLint flags that the
clsximport on Line 6 should appear before thereact-routerimport block (Lines 1-5). Move it above to satisfy the project's import-order rule.Proposed fix
+import { clsx } from 'clsx' import { type ErrorResponse, isRouteErrorResponse, useParams, } from 'react-router' -import { clsx } from 'clsx'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/error-boundary.tsx` around lines 1 - 6, The import ordering is wrong: move the `clsx` import so it appears before the `react-router` import block (the lines importing `ErrorResponse`, `isRouteErrorResponse`, and `useParams`) to satisfy the project's import-order rule; update the top of `error-boundary.tsx` so `import { clsx } from 'clsx'` comes above the `import { type ErrorResponse, isRouteErrorResponse, useParams } from 'react-router'` statement and run the linter to confirm the fix.
🧹 Nitpick comments (6)
app/components/errors.tsx (1)
230-235: Theas anyCSS custom property cast is a known TS limitation — consider a narrower type assertion.The
['--spacing-10vw' as any]pattern works but silences all type checking on the key. A slightly tighter alternative:Optional refinement
const notFoundGutterStyle = { - ['--spacing-10vw' as any]: 'clamp(0.75rem, 3vw, 3rem)', - } as React.CSSProperties + '--spacing-10vw': 'clamp(0.75rem, 3vw, 3rem)', + } as React.CSSProperties & Record<`--${string}`, string>This preserves type safety for standard CSS properties while allowing custom properties.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 230 - 235, The code uses a broad cast for the custom CSS property when building notFoundGutterStyle; replace the `['--spacing-10vw' as any]` cast with a narrower type assertion so TypeScript still types CSS properties but permits custom properties — for example make notFoundGutterStyle typed as React.CSSProperties & Record<string, string> and assign the custom property key without `any`, keeping the value as a string; update the declaration of notFoundGutterStyle accordingly (referencing the notFoundGutterStyle variable) so custom properties are allowed without disabling type checking.app/utils/not-found-suggestions.server.ts (5)
395-432:filterIndexItems: the multi-word intersection path is well-designed.One note: the
individualWordResults.filter(r => searchResult.includes(r))on Line 426-428 is O(n·m) per word iteration. This is fine for the expected index size (hundreds of items), but if the index ever grows substantially, switching to aSetforsearchResultwould make it O(n).Optional optimization
for (const word of restWords) { - const searchResult = matchSorter( + const searchResultSet = new Set(matchSorter( individualWordResults, word, individualWordOptions, - ) - individualWordResults = individualWordResults.filter((r) => - searchResult.includes(r), - ) + )) + individualWordResults = individualWordResults.filter((r) => + searchResultSet.has(r), + ) }🤖 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 395 - 432, The multi-word intersection loop in filterIndexItems uses individualWordResults.filter(r => searchResult.includes(r)) which is O(n·m); to optimize, convert the searchResult array to a Set once per iteration (e.g., const searchSet = new Set(searchResult)) and then filter using searchSet.has(r) so lookups are O(1); update the loop that iterates over restWords and replace the includes-based test with a .has check on the Set while keeping the rest of the logic (individualWordResults, individualWordOptions, restWords) unchanged.
297-328: Module-level cache has no invalidation — intentional but worth documenting.Both
cachedIndex(Line 297) andmdxMetaCache(Line 106) persist for the lifetime of the server process. New or edited content won't appear in 404 suggestions until the next deploy/restart. This is likely fine for production but could surprise during local development if content files are added or modified without an HMR reload of this module.A brief inline comment (e.g.,
// Invalidated on process restart / deploy) would document the intent.🤖 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 297 - 328, The module-level caches cachedIndex and cachedIndexPromise in getNotFoundDeterministicIndex (and the related mdxMetaCache) are persistent for the process lifetime and lack documentation of that intent; add a concise inline comment near the declarations of cachedIndex, cachedIndexPromise, and mdxMetaCache (and optionally above function getNotFoundDeterministicIndex) stating that these caches are intentionally invalidated only on process restart/deploy (e.g., "// Invalidated on process restart / deploy; no HMR invalidation") so future maintainers know this is intentional and not a bug.
496-512: Sequentialawait toNotFoundMatch(item)in a tight loop.Each candidate item is enriched one-at-a-time via
await toNotFoundMatch(item)which reads from the filesystem. After the first request,mdxMetaCachemakes this cheap, but the cold-start path for the first 404 hit could do up tosafeLimitsequential file reads per query candidate.This is acceptable given the small limit (default 8) and the cache warming on first hit, but if latency on the first 404 becomes noticeable, batching enrichment with
Promise.allon the filtered slice would help.🤖 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 496 - 512, The code currently calls await toNotFoundMatch(item) sequentially inside the nested loops (over queryCandidates and filtered results), causing up to safeLimit serial filesystem reads on cold-start; change to batch enrichment: for each candidate, build a slice of filtered items up to the remaining slots (safeLimit - matches.length), map them to toNotFoundMatch promises, await Promise.all on that slice, then iterate the resolved results to add non-null matches (adding seen keys via toUrlKey and skipping normalizePathname matches) until matches reaches safeLimit; keep using filterIndexItems, queryCandidates, toNotFoundMatch, safeLimit, matches and seen to locate the code.
46-56: Frontmatter regex assumes\nline endings.The regex
^---\n([\s\S]*?)\n---\nwon't match files with\r\n(Windows-style) line endings. If any contributor uses Windows without git'sautocrlfnormalization, frontmatter parsing will silently fail (returningnull, so no crash).If you want to be defensive:
Optional fix
- const match = source.match(/^---\n([\s\S]*?)\n---\n/u) + const match = source.match(/^---\r?\n([\s\S]*?)\r?\n---\r?\n/u)🤖 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 46 - 56, The frontmatter parser in parseYamlFrontmatter uses a regex that only matches LF line endings; update the regex to accept both LF and CRLF (use \r?\n) and also allow the closing --- to be followed by optional newline or end-of-file so Windows-formatted files still match. Locate parseYamlFrontmatter and replace the pattern /^---\n([\s\S]*?)\n---\n/u with a variant that uses \r?\n (for example /^---\r?\n([\s\S]*?)\r?\n---(?:\r?\n|$)/u) so the YAML.parse/try-catch logic continues to run unchanged on matched frontmatter.
166-170: CachegetRepoRootDir()result to avoid repeatedprocess.cwd()calls.The function is called multiple times across modules (5+ calls in
semantic-search-presentation.server.tsalone) without caching. Whileprocess.cwd()works under the stated assumptions, caching prevents unnecessary calls and guards against potential working directory changes.The
import.meta.dirnamealternative is technically available (Node 24 supports it), but it would anchor to the utils file's directory, requiring directory traversal to reach the repo root—less robust than the current approach unless paired with additional logic.🤖 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 166 - 170, getRepoRootDir() currently calls process.cwd() every time; cache its result in a module-level variable (e.g., repoRoot or cachedRepoRoot) when first invoked and return that on subsequent calls to avoid repeated process.cwd() calls and guard against working-directory churn; update the getRepoRootDir() function to check the cache, set it on first call, and return the cached value thereafter (referencing the getRepoRootDir symbol to locate and modify the implementation).
🤖 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`:
- Line 142: Update the user-facing copy in the errors component: replace the
subTitle prop value that currently uses `Deterministic matches for "${q}"` /
`Deterministic matches.` with friendlier text like `Possible matches for "${q}"`
and `Possible matches.` (use the `subTitle` prop and `q` variable), and
similarly change the "Try semantic search" string (found near the "Try semantic
search" usage) to a clearer phrase such as "Try a full-text search" or "Try
searching the full text" so the UI shows non-technical language to visitors.
---
Outside diff comments:
In `@app/components/error-boundary.tsx`:
- Around line 1-6: The import ordering is wrong: move the `clsx` import so it
appears before the `react-router` import block (the lines importing
`ErrorResponse`, `isRouteErrorResponse`, and `useParams`) to satisfy the
project's import-order rule; update the top of `error-boundary.tsx` so `import {
clsx } from 'clsx'` comes above the `import { type ErrorResponse,
isRouteErrorResponse, useParams } from 'react-router'` statement and run the
linter to confirm the fix.
---
Nitpick comments:
In `@app/components/errors.tsx`:
- Around line 230-235: The code uses a broad cast for the custom CSS property
when building notFoundGutterStyle; replace the `['--spacing-10vw' as any]` cast
with a narrower type assertion so TypeScript still types CSS properties but
permits custom properties — for example make notFoundGutterStyle typed as
React.CSSProperties & Record<string, string> and assign the custom property key
without `any`, keeping the value as a string; update the declaration of
notFoundGutterStyle accordingly (referencing the notFoundGutterStyle variable)
so custom properties are allowed without disabling type checking.
In `@app/utils/not-found-suggestions.server.ts`:
- Around line 395-432: The multi-word intersection loop in filterIndexItems uses
individualWordResults.filter(r => searchResult.includes(r)) which is O(n·m); to
optimize, convert the searchResult array to a Set once per iteration (e.g.,
const searchSet = new Set(searchResult)) and then filter using searchSet.has(r)
so lookups are O(1); update the loop that iterates over restWords and replace
the includes-based test with a .has check on the Set while keeping the rest of
the logic (individualWordResults, individualWordOptions, restWords) unchanged.
- Around line 297-328: The module-level caches cachedIndex and
cachedIndexPromise in getNotFoundDeterministicIndex (and the related
mdxMetaCache) are persistent for the process lifetime and lack documentation of
that intent; add a concise inline comment near the declarations of cachedIndex,
cachedIndexPromise, and mdxMetaCache (and optionally above function
getNotFoundDeterministicIndex) stating that these caches are intentionally
invalidated only on process restart/deploy (e.g., "// Invalidated on process
restart / deploy; no HMR invalidation") so future maintainers know this is
intentional and not a bug.
- Around line 496-512: The code currently calls await toNotFoundMatch(item)
sequentially inside the nested loops (over queryCandidates and filtered
results), causing up to safeLimit serial filesystem reads on cold-start; change
to batch enrichment: for each candidate, build a slice of filtered items up to
the remaining slots (safeLimit - matches.length), map them to toNotFoundMatch
promises, await Promise.all on that slice, then iterate the resolved results to
add non-null matches (adding seen keys via toUrlKey and skipping
normalizePathname matches) until matches reaches safeLimit; keep using
filterIndexItems, queryCandidates, toNotFoundMatch, safeLimit, matches and seen
to locate the code.
- Around line 46-56: The frontmatter parser in parseYamlFrontmatter uses a regex
that only matches LF line endings; update the regex to accept both LF and CRLF
(use \r?\n) and also allow the closing --- to be followed by optional newline or
end-of-file so Windows-formatted files still match. Locate parseYamlFrontmatter
and replace the pattern /^---\n([\s\S]*?)\n---\n/u with a variant that uses
\r?\n (for example /^---\r?\n([\s\S]*?)\r?\n---(?:\r?\n|$)/u) so the
YAML.parse/try-catch logic continues to run unchanged on matched frontmatter.
- Around line 166-170: getRepoRootDir() currently calls process.cwd() every
time; cache its result in a module-level variable (e.g., repoRoot or
cachedRepoRoot) when first invoked and return that on subsequent calls to avoid
repeated process.cwd() calls and guard against working-directory churn; update
the getRepoRootDir() function to check the cache, set it on first call, and
return the cached value thereafter (referencing the getRepoRootDir symbol to
locate and modify the implementation).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/components/error-boundary.tsxapp/components/errors.tsxapp/components/sections/header-section.tsxapp/utils/not-found-query.tsapp/utils/not-found-suggestions.server.ts
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/components/errors.tsx (2)
149-151: Already flagged: implementation-oriented subtitle copy.
"Deterministic matches for "${q}""/"Deterministic matches."are still technical, implementation-oriented labels for site visitors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 149 - 151, The subtitle text is implementation-oriented; update the JSX subTitle prop in the component where subTitle is set (the expression using q) to use user-facing wording instead of "Deterministic matches…". Replace both branches (the q truthy branch and the fallback) to something like an understandable label — e.g., use `Exact matches for "q"` when q is present and `Exact matches` (or another plain-language phrase) when q is absent — by editing the subTitle expression that references q in this component.
195-201: Already flagged: "Try semantic search" link text.Same user-facing copy concern as previously raised — "semantic search" is implementation language not meaningful to visitors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 195 - 201, The anchor text "Try semantic search" is implementation-centric; update the JSX anchor in app/components/errors.tsx (the <a href={searchUrl} className="underlined"> element using the searchUrl variable) to use user-facing copy (e.g., "Try a broader search" or "Try searching by meaning") and, if your app uses i18n, replace the hardcoded string with the appropriate translation key (or add one) so the visible link text is meaningful to visitors.
🤖 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 234-236: heroActionLabel exposes implementation-oriented copy
("Semantic search") to users; update the user-facing label used by the hero
ArrowLink so when hasPossibleMatches is false it shows a friendly phrase like
"Search the site" or "Find it with search" instead of "Semantic search". Locate
the heroActionLabel definition and the component that renders the ArrowLink and
change the false branch from 'Semantic search' to the new copy, ensuring any
tests or i18n keys referencing heroActionLabel are updated accordingly.
In `@app/utils/not-found-suggestions.server.ts`:
- Around line 45-55: The frontmatter regex in parseYamlFrontmatter only matches
LF endings so CRLF files fail and draft/unlisted flags get lost; update the
regex in parseYamlFrontmatter to accept both LF and CRLF by replacing literal
"\n" delimiters with "\r?\n" (and make the closing delimiter allow end-of-file
like (?:\r?\n|$)) so files with CRLF line endings correctly parse and preserve
draft/unlisted metadata.
---
Duplicate comments:
In `@app/components/errors.tsx`:
- Around line 149-151: The subtitle text is implementation-oriented; update the
JSX subTitle prop in the component where subTitle is set (the expression using
q) to use user-facing wording instead of "Deterministic matches…". Replace both
branches (the q truthy branch and the fallback) to something like an
understandable label — e.g., use `Exact matches for "q"` when q is present and
`Exact matches` (or another plain-language phrase) when q is absent — by editing
the subTitle expression that references q in this component.
- Around line 195-201: The anchor text "Try semantic search" is
implementation-centric; update the JSX anchor in app/components/errors.tsx (the
<a href={searchUrl} className="underlined"> element using the searchUrl
variable) to use user-facing copy (e.g., "Try a broader search" or "Try
searching by meaning") and, if your app uses i18n, replace the hardcoded string
with the appropriate translation key (or add one) so the visible link text is
meaningful to visitors.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/error-boundary.tsxapp/components/errors.tsxapp/utils/not-found-suggestions.server.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/utils/not-found-suggestions.server.ts (1)
37-39:⚠️ Potential issue | 🟡 MinorMake frontmatter parsing CRLF/EOF safe to avoid leaking drafts.
The regex only matches LF delimiters, so CRLF files won’t parse and
draft/unlistedflags will be missed. This can surface drafts in suggestions. Consider accepting CRLF and allowing end-of-file after the closing---.🛡️ Proposed fix
- const match = source.match(/^---\n([\s\S]*?)\n---\n/u) + const match = source.match(/^---\r?\n([\s\S]*?)\r?\n---(?:\r?\n|$)/u)🤖 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 37 - 39, The parseYamlFrontmatter function currently only matches LF line endings and will miss frontmatter in CRLF files and when closing --- is followed by EOF; update the regex used in parseYamlFrontmatter to accept both LF and CRLF by using an optional carriage return before newlines (e.g., \r?\n semantics) for the opening, capture, and closing delimiter, and allow the closing delimiter to be followed by either a newline or end-of-file so frontmatter is recognized at EOF; keep the Unicode flag and existing capture group behavior intact.
🤖 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/utils/not-found-suggestions.server.ts`:
- Around line 37-39: The parseYamlFrontmatter function currently only matches LF
line endings and will miss frontmatter in CRLF files and when closing --- is
followed by EOF; update the regex used in parseYamlFrontmatter to accept both LF
and CRLF by using an optional carriage return before newlines (e.g., \r?\n
semantics) for the opening, capture, and closing delimiter, and allow the
closing delimiter to be followed by either a newline or end-of-file so
frontmatter is recognized at EOF; keep the Unicode flag and existing capture
group behavior intact.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/components/errors.tsx (3)
235-237:heroActionLabelstill uses "Semantic search" — implementation-oriented copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 235 - 237, The label variable heroActionLabel currently falls back to the implementation-oriented string "Semantic search"; change it to a user-facing phrase (e.g., "Search" or "Find results") so the UI copy is customer-focused. Update the ternary that assigns heroActionLabel (the expression using hasPossibleMatches) to use the chosen user-facing string instead of "Semantic search" and ensure any related tests or translations referencing heroActionLabel are updated to match the new copy.
196-202: "Try semantic search" link text is still implementation-oriented copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 196 - 202, The link text "Try semantic search" is implementation-oriented; update the user-facing copy in the paragraph that uses hasMatches and searchUrl (in app/components/errors.tsx) to something clearer like "Search more results" or "Try a broader search" (choose consistent wording), keeping the same anchor element using searchUrl and preserving conditional prefix based on hasMatches; ensure only the visible link text is changed and not the variable names or URL.
150-152:subTitlestill exposes "Deterministic matches" — implementation-oriented copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 150 - 152, The subTitle prop in the errors component currently exposes implementation-oriented copy "Deterministic matches" (see subTitle assignment in app/components/errors.tsx); update the string to user-facing language (e.g., "Exact matches" or "Filtered matches") and ensure the q interpolation remains for the query case (q ? `Exact matches for "${q}"` : 'Exact matches.'); keep the same subTitle property and formatting so only the displayed text changes.
🧹 Nitpick comments (1)
app/components/errors.tsx (1)
242-244:as anyon the CSS variable key can be dropped—React.CSSPropertiesis already augmented globally.The project defines a global augmentation in
types/index.d.ts(lines 180–181) that extendsReact.CSSPropertieswith[key: \--${string}`]: string | number`, so the computed key cast is unnecessary.♻️ Cleaner alternative
- const notFoundGutterStyle = { - ['--spacing-10vw' as any]: 'clamp(0.75rem, 3vw, 3rem)', - } as React.CSSProperties + const notFoundGutterStyle: React.CSSProperties = { + '--spacing-10vw': 'clamp(0.75rem, 3vw, 3rem)', + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 242 - 244, Remove the unnecessary "as any" cast on the computed CSS variable key in the notFoundGutterStyle declaration: React.CSSProperties is already augmented to accept CSS custom properties, so change the object so the key is declared without the ['--spacing-10vw' as any] cast and keep the type assertion to React.CSSProperties; update the notFoundGutterStyle constant accordingly (referencing the notFoundGutterStyle symbol and React.CSSProperties) to rely on the global augmentation instead of forcing any.
🤖 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 75-87: ErrorPage's resolvedHeroProps currently always adds
arrowUrl/arrowLabel which duplicates or conflicts with an explicit action
injected by FourOhFour; update the logic in ErrorPage.resolvedHeroProps so it
only sets arrowUrl and arrowLabel when heroProps.action is falsy (i.e., guard
the ternary that builds the object to check !heroProps.action before spreading
in arrowUrl/arrowLabel), ensuring HeroSection receives either an action OR an
arrow link but not both.
---
Duplicate comments:
In `@app/components/errors.tsx`:
- Around line 235-237: The label variable heroActionLabel currently falls back
to the implementation-oriented string "Semantic search"; change it to a
user-facing phrase (e.g., "Search" or "Find results") so the UI copy is
customer-focused. Update the ternary that assigns heroActionLabel (the
expression using hasPossibleMatches) to use the chosen user-facing string
instead of "Semantic search" and ensure any related tests or translations
referencing heroActionLabel are updated to match the new copy.
- Around line 196-202: The link text "Try semantic search" is
implementation-oriented; update the user-facing copy in the paragraph that uses
hasMatches and searchUrl (in app/components/errors.tsx) to something clearer
like "Search more results" or "Try a broader search" (choose consistent
wording), keeping the same anchor element using searchUrl and preserving
conditional prefix based on hasMatches; ensure only the visible link text is
changed and not the variable names or URL.
- Around line 150-152: The subTitle prop in the errors component currently
exposes implementation-oriented copy "Deterministic matches" (see subTitle
assignment in app/components/errors.tsx); update the string to user-facing
language (e.g., "Exact matches" or "Filtered matches") and ensure the q
interpolation remains for the query case (q ? `Exact matches for "${q}"` :
'Exact matches.'); keep the same subTitle property and formatting so only the
displayed text changes.
---
Nitpick comments:
In `@app/components/errors.tsx`:
- Around line 242-244: Remove the unnecessary "as any" cast on the computed CSS
variable key in the notFoundGutterStyle declaration: React.CSSProperties is
already augmented to accept CSS custom properties, so change the object so the
key is declared without the ['--spacing-10vw' as any] cast and keep the type
assertion to React.CSSProperties; update the notFoundGutterStyle constant
accordingly (referencing the notFoundGutterStyle symbol and React.CSSProperties)
to rely on the global augmentation instead of forcing any.
app/components/errors.tsx
Outdated
| const resolvedHeroProps: HeroSectionProps = possibleMatches?.length | ||
| ? { ...heroProps, arrowUrl: '#possible-matches', arrowLabel: 'Possible matches' } | ||
| ? { | ||
| ...heroProps, | ||
| arrowUrl: '#possible-matches', | ||
| arrowLabel: 'Possible matches', | ||
| } | ||
| : articles?.length | ||
| ? { ...heroProps, arrowUrl: '#articles', arrowLabel: 'But wait, there is more!' } | ||
| ? { | ||
| ...heroProps, | ||
| arrowUrl: '#articles', | ||
| arrowLabel: 'But wait, there is more!', | ||
| } | ||
| : heroProps |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the HeroSection component
fd -type f -name "*hero*" \( -name "*.tsx" -o -name "*.ts" \)Repository: kentcdodds/kentcdodds.com
Length of output: 239
🏁 Script executed:
# Also check the errors.tsx file structure
cat -n app/components/errors.tsx | head -100Repository: kentcdodds/kentcdodds.com
Length of output: 3498
🏁 Script executed:
# Check around line 75-87 for resolvedHeroProps
cat -n app/components/errors.tsx | sed -n '70,90p'Repository: kentcdodds/kentcdodds.com
Length of output: 690
🏁 Script executed:
# Check around line 257-260 for FourOhFour action
cat -n app/components/errors.tsx | sed -n '250,270p'Repository: kentcdodds/kentcdodds.com
Length of output: 795
🏁 Script executed:
# Find and examine HeroSection component
fd -path "*/hero-section*" -name "*.tsx"Repository: kentcdodds/kentcdodds.com
Length of output: 237
🏁 Script executed:
# Also try a broader search
find . -type f -name "hero-section.tsx" -o -name "hero*.tsx" | head -20Repository: kentcdodds/kentcdodds.com
Length of output: 111
🏁 Script executed:
# Examine HeroSection component
cat -n ./app/components/sections/hero-section.tsxRepository: kentcdodds/kentcdodds.com
Length of output: 6259
ErrorPage.resolvedHeroProps adds arrowUrl/arrowLabel simultaneously with FourOhFour's explicit action prop, causing duplication or conflicting CTAs.
HeroSection renders both action and arrowUrl/arrowLabel independently in separate DOM elements. Since FourOhFour (lines 256–260) injects an explicit action prop into heroProps, and ErrorPage.resolvedHeroProps (lines 75–87) then spreads heroProps and adds arrowUrl/arrowLabel overrides, both elements end up in the component simultaneously:
-
Case A —
possibleMatchespresent: Bothactionand the arrow link render on larger screens pointing to#possible-matcheswith identical labels ("Possible matches"). The same CTA appears twice. -
Case B —
articlesonly (nopossibleMatches):actionlinks tosearchUrl("Semantic search") while the arrow links to#articles("But wait, there is more!"). Two distinct CTAs lead users to different destinations.
Fix by guarding the resolvedHeroProps override — only inject arrowUrl/arrowLabel if the caller hasn't already provided action:
Proposed guard
const resolvedHeroProps: HeroSectionProps = possibleMatches?.length
- ? {
- ...heroProps,
- arrowUrl: '#possible-matches',
- arrowLabel: 'Possible matches',
- }
+ ? heroProps.action
+ ? heroProps
+ : { ...heroProps, arrowUrl: '#possible-matches', arrowLabel: 'Possible matches' }
: articles?.length
- ? {
- ...heroProps,
- arrowUrl: '#articles',
- arrowLabel: 'But wait, there is more!',
- }
+ ? heroProps.action
+ ? heroProps
+ : { ...heroProps, arrowUrl: '#articles', arrowLabel: 'But wait, there is more!' }
: heroProps📝 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 resolvedHeroProps: HeroSectionProps = possibleMatches?.length | |
| ? { ...heroProps, arrowUrl: '#possible-matches', arrowLabel: 'Possible matches' } | |
| ? { | |
| ...heroProps, | |
| arrowUrl: '#possible-matches', | |
| arrowLabel: 'Possible matches', | |
| } | |
| : articles?.length | |
| ? { ...heroProps, arrowUrl: '#articles', arrowLabel: 'But wait, there is more!' } | |
| ? { | |
| ...heroProps, | |
| arrowUrl: '#articles', | |
| arrowLabel: 'But wait, there is more!', | |
| } | |
| : heroProps | |
| const resolvedHeroProps: HeroSectionProps = possibleMatches?.length | |
| ? heroProps.action | |
| ? heroProps | |
| : { ...heroProps, arrowUrl: '#possible-matches', arrowLabel: 'Possible matches' } | |
| : articles?.length | |
| ? heroProps.action | |
| ? heroProps | |
| : { ...heroProps, arrowUrl: '#articles', arrowLabel: 'But wait, there is more!' } | |
| : heroProps |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/errors.tsx` around lines 75 - 87, ErrorPage's
resolvedHeroProps currently always adds arrowUrl/arrowLabel which duplicates or
conflicts with an explicit action injected by FourOhFour; update the logic in
ErrorPage.resolvedHeroProps so it only sets arrowUrl and arrowLabel when
heroProps.action is falsy (i.e., guard the ternary that builds the object to
check !heroProps.action before spreading in arrowUrl/arrowLabel), ensuring
HeroSection receives either an action OR an arrow link but not both.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/components/errors.tsx (2)
75-87:⚠️ Potential issue | 🟡 Minor
resolvedHeroPropsstill injectsarrowUrl/arrowLabeleven whenheroProps.actionis already set.
FourOhFouralways provides an explicitactionprop (lines 256–260).ErrorPage.resolvedHeroPropsthen unconditionally addsarrowUrl/arrowLabelon top, soHeroSectionreceives both UI elements simultaneously:
- With
possibleMatches: both theactionbutton and the arrow link point to#possible-matcheswith the identical label "Possible matches" — redundant duplicate CTAs.- With
articlesonly:actiontargetssearchUrl("Search the site") whilearrowUrltargets#articles("But wait, there is more!") — the hero presents two simultaneous navigational choices that stem from different sources in the tree.Guard the arrow-link injection so it only applies when the caller hasn't supplied an
action:🛡️ Proposed fix
const resolvedHeroProps: HeroSectionProps = possibleMatches?.length - ? { - ...heroProps, - arrowUrl: '#possible-matches', - arrowLabel: 'Possible matches', - } + ? heroProps.action + ? heroProps + : { ...heroProps, arrowUrl: '#possible-matches', arrowLabel: 'Possible matches' } : articles?.length - ? { - ...heroProps, - arrowUrl: '#articles', - arrowLabel: 'But wait, there is more!', - } + ? heroProps.action + ? heroProps + : { ...heroProps, arrowUrl: '#articles', arrowLabel: 'But wait, there is more!' } : heroProps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 75 - 87, resolvedHeroProps currently injects arrowUrl/arrowLabel even when the caller provided a heroProps.action (causing duplicate CTAs); update the construction of resolvedHeroProps to only add arrowUrl and arrowLabel when heroProps.action is not present (e.g., check heroProps.action or its equivalent truthiness) so that when FourOhFour supplies an explicit action the HeroSection receives only that action and not the injected arrow link.
143-202:⚠️ Potential issue | 🟡 Minor
hasMatchesis alwaystruehere, making the "No deterministic matches found." branch dead code; also, "Try semantic search" copy is still implementation-oriented.Two issues in this region:
1. Dead code (Line 143 / Line 197):
PossibleMatchesSectionis only rendered byErrorPagewhenpossibleMatches?.length > 0(line 111).sortNotFoundMatchesonly sorts — it never filters — sosorted.length === matches.length >= 1always holds.hasMatchesis therefore alwaystruein practice, and the'No deterministic matches found.'branch (line 197) is unreachable. Either remove the guard inErrorPageso the component handles empty arrays itself, or drophasMatchesand hard-code'None of these match? '.2. "Try semantic search" (Line 199): The link label is still implementation-oriented phrasing that visitors won't understand, as previously flagged. Consider "Try a full-text search" or simply "Search the site".
🛠️ Proposed fix
- const hasMatches = sorted.length > 0 + // PossibleMatchesSection is only rendered when matches is non-empty, + // so sorted.length is always > 0 here. ... - <p className="mt-4 text-sm text-slate-500"> - {hasMatches ? 'None of these match? ' : 'No deterministic matches found. '} - <a href={searchUrl} className="underlined"> - Try semantic search - </a> - . - </p> + <p className="mt-4 text-sm text-slate-500"> + {'None of these match? '} + <a href={searchUrl} className="underlined"> + Try a full-text search + </a> + . + </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/errors.tsx` around lines 143 - 202, The hasMatches flag is effectively always true because ErrorPage only renders PossibleMatchesSection when possibleMatches?.length > 0 and sortNotFoundMatches only sorts, so drop hasMatches logic: remove the hasMatches variable and always render the "None of these match? " copy before the search link (i.e., hard-code that text in the paragraph inside PossibleMatchesSection). Also update the link label from "Try semantic search" to a user-facing phrase like "Search the site" or "Try a full-text search" by changing the anchor text that uses searchUrl. Ensure edits target PossibleMatchesSection, ErrorPage, sortNotFoundMatches, hasMatches and searchUrl references.
🧹 Nitpick comments (3)
app/utils/not-found-suggestions.server.ts (3)
407-416:maxRanking: CASE_SENSITIVE_EQUALis a no-op inindividualWordOptions.
CASE_SENSITIVE_EQUALis the highest ranking in match-sorter (value7) — settingmaxRankingto it imposes no cap whatsoever, since no ranking can exceed it. The actual filtering is done entirely bythreshold: WORD_STARTS_WITH. If the intent was to prevent an overly-boosted full-match from dominating the per-word results, use a lower cap (e.g.,WORD_STARTS_WITHorCONTAINS); otherwise, removemaxRankingto avoid misleading future readers.🤖 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 407 - 416, The current individualWordOptions sets maxRanking: matchSorterRankings.CASE_SENSITIVE_EQUAL which is a no-op (highest rank); either remove maxRanking entirely or set it to a lower cap (e.g., matchSorterRankings.WORD_STARTS_WITH or matchSorterRankings.CONTAINS) so per-word results aren’t dominated by full-match boosts — update the keys mapping in individualWordOptions where matchSorterOptions.keys is mapped to replace the maxRanking value accordingly.
423-434: O(n²) array intersection; swap to aSetfor O(n).
searchResult.includes(r)is O(n) per element, making the filter O(n²) per word. Since all items share object references from the same source array, aSetworks correctly.♻️ Proposed fix
for (const word of restWords) { const searchResult = matchSorter( individualWordResults, word, individualWordOptions, ) - individualWordResults = individualWordResults.filter((r) => - searchResult.includes(r), - ) + const searchResultSet = new Set(searchResult) + individualWordResults = individualWordResults.filter((r) => + searchResultSet.has(r), + ) }🤖 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 423 - 434, The current loop over restWords repeatedly does individualWordResults.filter(...) with searchResult.includes(r), causing O(n²) behavior; replace the per-word filter with a Set-based intersection: for each word call matchSorter(individualWordResults, word, individualWordOptions), build a Set from that searchResult, then reassign individualWordResults to only those items present in the Set (O(n) per word). Update the block that references restWords, individualWordResults, matchSorter, and individualWordOptions to use a Set lookup rather than Array.prototype.includes to improve performance.
149-153: Redundant ENOENT branch in catch — both pathscontinue.The
if (e?.code === 'ENOENT') continueand the subsequent barecontinueare identical. The check adds no differentiation.♻️ Proposed simplification
- } catch (e: any) { - if (e?.code === 'ENOENT') continue - // If the filesystem is unhappy, skip enrichment rather than failing 404s. - continue - } + } catch { + // ENOENT or any other fs error — skip enrichment rather than failing 404s. + continue + }🤖 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 149 - 153, In the catch block that handles errors for enrichment (the block using the exception variable `e` and checking `e?.code === 'ENOENT'`), remove the redundant `if (e?.code === 'ENOENT') continue` branch since both branches call `continue`; leave a single `continue` to skip on any caught error (or, if you prefer, collapse the whole catch to just `continue`) so the behavior is unchanged but the code is simplified.
🤖 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 75-87: resolvedHeroProps currently injects arrowUrl/arrowLabel
even when the caller provided a heroProps.action (causing duplicate CTAs);
update the construction of resolvedHeroProps to only add arrowUrl and arrowLabel
when heroProps.action is not present (e.g., check heroProps.action or its
equivalent truthiness) so that when FourOhFour supplies an explicit action the
HeroSection receives only that action and not the injected arrow link.
- Around line 143-202: The hasMatches flag is effectively always true because
ErrorPage only renders PossibleMatchesSection when possibleMatches?.length > 0
and sortNotFoundMatches only sorts, so drop hasMatches logic: remove the
hasMatches variable and always render the "None of these match? " copy before
the search link (i.e., hard-code that text in the paragraph inside
PossibleMatchesSection). Also update the link label from "Try semantic search"
to a user-facing phrase like "Search the site" or "Try a full-text search" by
changing the anchor text that uses searchUrl. Ensure edits target
PossibleMatchesSection, ErrorPage, sortNotFoundMatches, hasMatches and searchUrl
references.
---
Nitpick comments:
In `@app/utils/not-found-suggestions.server.ts`:
- Around line 407-416: The current individualWordOptions sets maxRanking:
matchSorterRankings.CASE_SENSITIVE_EQUAL which is a no-op (highest rank); either
remove maxRanking entirely or set it to a lower cap (e.g.,
matchSorterRankings.WORD_STARTS_WITH or matchSorterRankings.CONTAINS) so
per-word results aren’t dominated by full-match boosts — update the keys mapping
in individualWordOptions where matchSorterOptions.keys is mapped to replace the
maxRanking value accordingly.
- Around line 423-434: The current loop over restWords repeatedly does
individualWordResults.filter(...) with searchResult.includes(r), causing O(n²)
behavior; replace the per-word filter with a Set-based intersection: for each
word call matchSorter(individualWordResults, word, individualWordOptions), build
a Set from that searchResult, then reassign individualWordResults to only those
items present in the Set (O(n) per word). Update the block that references
restWords, individualWordResults, matchSorter, and individualWordOptions to use
a Set lookup rather than Array.prototype.includes to improve performance.
- Around line 149-153: In the catch block that handles errors for enrichment
(the block using the exception variable `e` and checking `e?.code ===
'ENOENT'`), remove the redundant `if (e?.code === 'ENOENT') continue` branch
since both branches call `continue`; leave a single `continue` to skip on any
caught error (or, if you prefer, collapse the whole catch to just `continue`) so
the behavior is unchanged but the code is simplified.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/error-boundary.tsxapp/components/errors.tsxapp/utils/not-found-suggestions.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/error-boundary.tsx
cursor
bot
force-pushed
the
cursor/deterministic-404-search-ef47
branch
from
b4b9682 to
8ba4bb1
Compare
| possibleMatchesQuery={effectiveQuery} | ||
| heroProps={{ | ||
| title: "404 - Oh no, you found a page that's missing stuff.", | ||
| subtitle: `"${pathname}" is not a page on kentcdodds.com. So sorry.`, |
There was a problem hiding this comment.
Missing pathname fallback in 404 subtitle
Medium Severity
The 404 subtitle uses pathname directly in the template string. When pathnameProp is not passed and last?.pathname from useMatches() is undefined (e.g. empty route matches or edge cases), the subtitle renders the literal string "undefined" instead of the missing URL.
cursor
bot
force-pushed
the
cursor/deterministic-404-search-ef47
branch
from
c0143df to
744e0e5
Compare
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>
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>
This reverts commit ce52af7.
This reverts commit c0fee95.
This reverts commit 827cdfb.
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>
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>
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/deterministic-404-search-ef47
branch
from
1ade3cd to
0cae7ff
Compare
| threshold: matchSorterRankings.WORD_STARTS_WITH, | ||
| } | ||
| }), | ||
| } |
There was a problem hiding this comment.
Individual word matching broken by maxRanking below threshold
Medium Severity
The individualWordOptions spreads the original keys and only overrides threshold to WORD_STARTS_WITH, but inherits the original maxRanking: CONTAINS on all keys except title. In match-sorter's ranking system, CONTAINS has a lower numeric value than WORD_STARTS_WITH, so the capped rank can never meet the threshold — effectively disabling URL, slug, and type keys for individual word matching. The existing blog.ts filter handles this correctly by explicitly setting maxRanking: CASE_SENSITIVE_EQUAL on individual word keys.
2 participants
Replace 404 semantic search with deterministic
match-sorterresults and improve mobile layout.Note
Medium Risk
Introduces a new server-side indexing flow that reads the repo filesystem and parses YAML frontmatter on 404s; while guarded to fail closed, it can affect performance and suggestion correctness across environments.
Overview
Replaces 404 “possible matches” generation from networked semantic search to a deterministic, filesystem-backed
match-sorterindex that scanscontent/blogandcontent/pages, dedupes URLs, filters drafts/unlisted via MDX frontmatter, and enriches results with title/summary and Cloudinary thumbnails.Improves error/404 UI:
GeneralErrorBoundarynow renders route-error vs unexpected-error branches with consistent responsive padding; the 404 page avoids duplicate hero CTAs, adjusts hero action label/target based on match availability, tweaks mobile gutters, and updates the possible-matches section copy/layout to always show a “Try semantic search” link (including when there are zero matches).Written by Cursor Bugbot for commit 0cae7ff. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation