Conversation
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 Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughThis PR extends semantic search results with enriched metadata (summary, imageUrl, imageAlt). A new presentation module generates type-specific metadata for various document types (blog, page, talk, etc.). Results are enriched via concurrent post-processing. UI components updated to display images and summaries. Test scripts configured for explicit run mode. Changes
Sequence DiagramsequenceDiagram
actor Client
participant NavSearch as NavSearch Component
participant SemanticSearch as semanticSearchKCD
participant PresentationModule as getSemanticSearchPresentation
participant FileSystem as MDX/YAML Files
participant Cloudinary as Cloudinary Service
Client->>NavSearch: Trigger search
NavSearch->>SemanticSearch: Execute semantic search
SemanticSearch->>SemanticSearch: Rank & slice results
SemanticSearch->>PresentationModule: Enrich each result (concurrent)
par Enrichment for each result
PresentationModule->>FileSystem: Read MDX/YAML (cached)
FileSystem-->>PresentationModule: Metadata (title, description, banner)
PresentationModule->>Cloudinary: Generate thumbnail URLs
Cloudinary-->>PresentationModule: Image URLs
PresentationModule-->>PresentationModule: Merge base + enriched data
end
PresentationModule-->>SemanticSearch: Enriched results
SemanticSearch-->>NavSearch: Results with summary, imageUrl, imageAlt
NavSearch->>Client: Display search suggestions with images & summaries
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 |
![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
🧹 Nitpick comments (9)
app/components/navbar.tsx (2)
543-543: Placeholder uses hardcoded gray colors instead of semantic theme tokens.The rest of the app uses semantic classes like
bg-primary,bg-secondary, etc. This hardcodedbg-gray-200 dark:bg-gray-700will not adapt if the theme palette changes. Consider using a semantic token or at least a shared utility class. The same pattern appears inapp/routes/search.tsxline 195.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/navbar.tsx` at line 543, The placeholder div in the Navbar component uses hardcoded colors ("bg-gray-200 dark:bg-gray-700"); replace that with a semantic theme token or shared utility class (for example use the app's semantic class like "bg-secondary" or a shared "placeholder-bg" utility) so it follows the app-wide palette and adapts to theme changes; apply the same replacement in the similar placeholder in app/routes/search.tsx (the two JSX placeholder divs) to ensure consistency across components.
553-559: Consider extracting the IIFE to a small helper or computing it before the JSX.An inline IIFE in JSX is functional but slightly harder to scan. A small helper like
getDisplayPath(url: string)or aconstbefore thereturnwould improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/navbar.tsx` around lines 553 - 559, Extract the inline IIFE that computes the display path from s.url into a small helper and use it in the JSX: create a function like getDisplayPath(url: string) (or compute const displayPath = getDisplayPath(s.url) just before the component's return) which returns new URL(url).pathname or falls back to the original url on error, then replace the IIFE in the JSX with that helper/const; reference the existing s.url and the new getDisplayPath/displayPath to locate and replace the logic in the Navbar component.app/routes/resources+/search.ts (1)
8-14:normalizeSummaryduplicates the normalize + truncate logic fromsemantic-search-presentation.server.ts.The presentation layer already truncates
summaryto 220 chars. This function is still useful as a safety net for ther.snippetfallback path, but the shared constants (220 / 217) and logic could be extracted to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/resources`+/search.ts around lines 8 - 14, normalizeSummary duplicates truncation constants and logic used in semantic-search-presentation.server.ts; extract the shared truncate/normalize logic and constants (e.g., MAX_SUMMARY_LENGTH = 220 and TRUNCATE_SLICE = 217 or a single truncate function) into a common helper used by both normalizeSummary in app/routes/resources+/search.ts and the presentation function in semantic-search-presentation.server.ts so both code paths use the same normalization + truncation implementation (keep normalizeSummary as a thin wrapper that calls the shared helper to preserve the r.snippet safety net).app/utils/semantic-search-presentation.server.ts (5)
221-257:mdxMetaCacheis an unbounded, never-evicted module-level Map.Every unique
(type, slug)pair that goes throughgetMdxDocMetais cached forever for the lifetime of the process. For a site with hundreds of blog posts, this will accumulate (though each entry is small). More importantly, cached data is never refreshed — if MDX frontmatter is updated, the stale value persists until restart.Consider using an LRU cache (you already depend on
lru-cache) with a reasonable max size and/or TTL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/semantic-search-presentation.server.ts` around lines 221 - 257, The module-level mdxMetaCache Map used by getMdxDocMeta is unbounded and never refreshed; replace it with an lru-cache instance (imported from 'lru-cache') configured with a sensible max entries and optional ttl so entries expire, then use that cache’s get/set methods instead of mdxMetaCache.get and mdxMetaCache.set; ensure the cache key remains `${type}:${slug}` and keep the try/catch flow the same so updates to MDX frontmatter are picked up after TTL or eviction.
24-31:ResultLikeis used in the exported function signature but not exported itself.Callers of
getSemanticSearchPresentationcannot reference this type by name. If any consumer needs to construct aResultLikeexplicitly, they'll need to duplicate the shape. Consider exporting it alongsideSemanticSearchPresentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/semantic-search-presentation.server.ts` around lines 24 - 31, The type alias ResultLike is referenced in the exported function getSemanticSearchPresentation but is not exported, preventing callers from using the type; export ResultLike (e.g., add export to the ResultLike declaration) so consumers can import it alongside SemanticSearchPresentation and avoid duplicating the shape — update the declaration of ResultLike and keep the existing names used by getSemanticSearchPresentation and SemanticSearchPresentation.
260-284: YAML-backed caches (talksBySlug,creditsBySlug,testimonialsBySlug,resumeMeta) are loaded once and never invalidated.These module-level singletons will serve stale data if the underlying YAML files change without a server restart. This may be acceptable if the deployment model always restarts on content changes, but it's worth calling out. The same LRU/TTL approach or a simple timestamp-based revalidation would add resilience.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/semantic-search-presentation.server.ts` around lines 260 - 284, The module-level caches (talksBySlug and similar) are never invalidated, causing stale data; update loadTalksBySlug to detect changes and reload: keep metadata alongside the cached Map (e.g., lastLoaded timestamp or the source file mtime) and in loadTalksBySlug call fs.stat on the YAML file to compare mtime or check TTL before returning the cached Map; if the file changed or TTL expired, re-read and reparse the file and update the cache. Apply the same pattern to creditsBySlug, testimonialsBySlug, and resumeMeta (or centralize into a small cache helper) so all YAML-backed singletons support revalidation.
73-135: Repetitive fallback image construction — consider a lookup table.Each branch in
getFallbackPresentationfollows the same pattern: pick an image preset, call it with identical options, and return{ imageUrl, imageAlt }. A data-driven lookup would reduce duplication and make adding new types trivial.Example data-driven approach
+const FALLBACK_IMAGE_MAP: Record<string, { image: typeof images.microphone; resize: 'pad' | 'fill' }> = { + ck: { image: images.microphone, resize: 'pad' }, + cwk: { image: images.kayak, resize: 'pad' }, + podcast: { image: images.microphone, resize: 'pad' }, + talk: { image: images.kentSpeakingAllThingsOpen, resize: 'fill' }, + resume: { image: images.kentProfile, resize: 'fill' }, +} + function getFallbackPresentation(type: string | undefined) { - if (type === 'ck') { - ... - } - ... + const entry = type ? FALLBACK_IMAGE_MAP[type] : undefined + const img = entry?.image ?? images.kodyProfileGray + const resize = entry?.resize ?? 'pad' + return { + imageUrl: img({ quality: 'auto', format: 'auto', resize: { type: resize, width: 96, height: 96 } }), + imageAlt: img.alt, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/semantic-search-presentation.server.ts` around lines 73 - 135, Refactor getFallbackPresentation to remove repetitive branches by creating a lookup map from the type keys ('ck', 'cwk', 'podcast', 'talk', 'resume', default) to the corresponding image factory (e.g., images.microphone, images.kayak, images.kentSpeakingAllThingsOpen, images.kentProfile, images.kodyProfileGray) and alt values, then call the selected image factory once with the shared options ({ quality: 'auto', format: 'auto', resize: { ... } }) and return { imageUrl, imageAlt }; keep the existing resize types (pad vs fill) per entry in the map so behavior stays identical and fall back to images.kodyProfileGray for unknown types.
43-47:asNonEmptyStringis duplicated verbatim inapp/utils/semantic-search.server.ts(line 20).Extract to a shared utility to avoid drift between the two copies.
Shared utility approach
Create a small shared helper (e.g., in
app/utils/misc.tsxor a newapp/utils/string-helpers.ts) and import it from both files:export function asNonEmptyString(value: unknown): string | undefined { if (typeof value !== 'string') return undefined const trimmed = value.trim() return trimmed || undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/semantic-search-presentation.server.ts` around lines 43 - 47, Duplicate function asNonEmptyString exists in two modules; extract it into a single shared utility (e.g., export function asNonEmptyString(...) in a new string-helpers module), remove the local copies from both consumers (the modules that currently define asNonEmptyString), and import the shared asNonEmptyString from that utility wherever it was used (update the import statements and ensure types remain string | undefined).app/utils/semantic-search.server.ts (1)
355-374: Enrichment viaPromise.allis clean, but consider error resilience for individual failures.
getSemanticSearchPresentationinternally catches per-branch errors, but if an unexpected error escapes (e.g., a filesystem permission issue), the entirePromise.allwill reject and the search request fails with no results. Wrapping each enrichment in a per-item catch would be more defensive:Defensive per-item fallback
const enriched = await Promise.all( baseResults.map(async (r) => { - const presentation = await getSemanticSearchPresentation(r) - return { ...r, ...presentation } + try { + const presentation = await getSemanticSearchPresentation(r) + return { ...r, ...presentation } + } catch { + return r + } }), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/semantic-search.server.ts` around lines 355 - 374, The enrichment step uses Promise.all over baseResults.map calling getSemanticSearchPresentation, but an unexpected thrown error will reject the whole Promise.all and drop results; wrap each item enrichment in a per-item try/catch inside the map so failures return a safe fallback (e.g., the original r or r with minimal presentation) and log the error instead of letting it propagate, ensuring enriched remains an array even if getSemanticSearchPresentation throws for some items.
🤖 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/routes/search.tsx`:
- Around line 199-205: The conditional rendering in the H4 always takes the Link
branch because href is always a non-empty string; update the condition to only
render the Link when href is a meaningful destination (e.g., href && href !==
'#') so the else branch (rendering r.id) can execute for placeholder
destinations—modify the JSX around H4/Link that uses the href variable (and
references r.title, r.url, r.id) to check for href !== '#' (or check r.url
directly) before rendering <Link>.
---
Nitpick comments:
In `@app/components/navbar.tsx`:
- Line 543: The placeholder div in the Navbar component uses hardcoded colors
("bg-gray-200 dark:bg-gray-700"); replace that with a semantic theme token or
shared utility class (for example use the app's semantic class like
"bg-secondary" or a shared "placeholder-bg" utility) so it follows the app-wide
palette and adapts to theme changes; apply the same replacement in the similar
placeholder in app/routes/search.tsx (the two JSX placeholder divs) to ensure
consistency across components.
- Around line 553-559: Extract the inline IIFE that computes the display path
from s.url into a small helper and use it in the JSX: create a function like
getDisplayPath(url: string) (or compute const displayPath =
getDisplayPath(s.url) just before the component's return) which returns new
URL(url).pathname or falls back to the original url on error, then replace the
IIFE in the JSX with that helper/const; reference the existing s.url and the new
getDisplayPath/displayPath to locate and replace the logic in the Navbar
component.
In `@app/routes/resources`+/search.ts:
- Around line 8-14: normalizeSummary duplicates truncation constants and logic
used in semantic-search-presentation.server.ts; extract the shared
truncate/normalize logic and constants (e.g., MAX_SUMMARY_LENGTH = 220 and
TRUNCATE_SLICE = 217 or a single truncate function) into a common helper used by
both normalizeSummary in app/routes/resources+/search.ts and the presentation
function in semantic-search-presentation.server.ts so both code paths use the
same normalization + truncation implementation (keep normalizeSummary as a thin
wrapper that calls the shared helper to preserve the r.snippet safety net).
In `@app/utils/semantic-search-presentation.server.ts`:
- Around line 221-257: The module-level mdxMetaCache Map used by getMdxDocMeta
is unbounded and never refreshed; replace it with an lru-cache instance
(imported from 'lru-cache') configured with a sensible max entries and optional
ttl so entries expire, then use that cache’s get/set methods instead of
mdxMetaCache.get and mdxMetaCache.set; ensure the cache key remains
`${type}:${slug}` and keep the try/catch flow the same so updates to MDX
frontmatter are picked up after TTL or eviction.
- Around line 24-31: The type alias ResultLike is referenced in the exported
function getSemanticSearchPresentation but is not exported, preventing callers
from using the type; export ResultLike (e.g., add export to the ResultLike
declaration) so consumers can import it alongside SemanticSearchPresentation and
avoid duplicating the shape — update the declaration of ResultLike and keep the
existing names used by getSemanticSearchPresentation and
SemanticSearchPresentation.
- Around line 260-284: The module-level caches (talksBySlug and similar) are
never invalidated, causing stale data; update loadTalksBySlug to detect changes
and reload: keep metadata alongside the cached Map (e.g., lastLoaded timestamp
or the source file mtime) and in loadTalksBySlug call fs.stat on the YAML file
to compare mtime or check TTL before returning the cached Map; if the file
changed or TTL expired, re-read and reparse the file and update the cache. Apply
the same pattern to creditsBySlug, testimonialsBySlug, and resumeMeta (or
centralize into a small cache helper) so all YAML-backed singletons support
revalidation.
- Around line 73-135: Refactor getFallbackPresentation to remove repetitive
branches by creating a lookup map from the type keys ('ck', 'cwk', 'podcast',
'talk', 'resume', default) to the corresponding image factory (e.g.,
images.microphone, images.kayak, images.kentSpeakingAllThingsOpen,
images.kentProfile, images.kodyProfileGray) and alt values, then call the
selected image factory once with the shared options ({ quality: 'auto', format:
'auto', resize: { ... } }) and return { imageUrl, imageAlt }; keep the existing
resize types (pad vs fill) per entry in the map so behavior stays identical and
fall back to images.kodyProfileGray for unknown types.
- Around line 43-47: Duplicate function asNonEmptyString exists in two modules;
extract it into a single shared utility (e.g., export function
asNonEmptyString(...) in a new string-helpers module), remove the local copies
from both consumers (the modules that currently define asNonEmptyString), and
import the shared asNonEmptyString from that utility wherever it was used
(update the import statements and ensure types remain string | undefined).
In `@app/utils/semantic-search.server.ts`:
- Around line 355-374: The enrichment step uses Promise.all over baseResults.map
calling getSemanticSearchPresentation, but an unexpected thrown error will
reject the whole Promise.all and drop results; wrap each item enrichment in a
per-item try/catch inside the map so failures return a safe fallback (e.g., the
original r or r with minimal presentation) and log the error instead of letting
it propagate, ensuring enriched remains an array even if
getSemanticSearchPresentation throws for some items.
| <H4 className="truncate"> | ||
| {href ? ( | ||
| <Link to={href}>{r.title ?? r.url ?? r.id}</Link> | ||
| ) : ( | ||
| r.id | ||
| )} | ||
| </H4> |
There was a problem hiding this comment.
The else branch (r.id) on line 203 is unreachable.
href on line 178 always resolves to a non-empty string (r.url, '/', or '#'), so the condition href ? is always truthy. The else branch can never execute.
If the intent is to avoid rendering a <Link> when there's no meaningful destination, the condition should exclude '#':
Suggested fix
<H4 className="truncate">
- {href ? (
+ {href && href !== '#' ? (
<Link to={href}>{r.title ?? r.url ?? r.id}</Link>
) : (
r.id
)}
</H4>📝 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.
| <H4 className="truncate"> | |
| {href ? ( | |
| <Link to={href}>{r.title ?? r.url ?? r.id}</Link> | |
| ) : ( | |
| r.id | |
| )} | |
| </H4> | |
| <H4 className="truncate"> | |
| {href && href !== '#' ? ( | |
| <Link to={href}>{r.title ?? r.url ?? r.id}</Link> | |
| ) : ( | |
| r.id | |
| )} | |
| </H4> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/search.tsx` around lines 199 - 205, The conditional rendering in
the H4 always takes the Link branch because href is always a non-empty string;
update the condition to only render the Link when href is a meaningful
destination (e.g., href && href !== '#') so the else branch (rendering r.id) can
execute for placeholder destinations—modify the JSX around H4/Link that uses the
href variable (and references r.title, r.url, r.id) to check for href !== '#'
(or check r.url directly) before rendering <Link>.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }) | ||
| : base.imageUrl | ||
| return { summary, imageUrl, imageAlt: alt } | ||
| } |
There was a problem hiding this comment.
Blog/page enrichment path missing try-catch unlike other paths
Low Severity
The blog/page enrichment path is the only type-specific branch in getSemanticSearchPresentation that lacks a try-catch block. All other enrichment paths (talk, credit, testimonial, resume) wrap their logic in try-catch and gracefully return base on error. Since this function runs inside Promise.all in semanticSearchKCD, an unexpected throw here (e.g. from buildThumbFromCloudinaryId) would reject the entire promise and break the search for all results, rather than gracefully degrading for just that one item.
Additional Locations (1)
| if (!text) return undefined | ||
| // Keep payloads small for consumers like Discord embeds/autocomplete UIs. | ||
| return text.length > 220 ? `${text.slice(0, 217)}...` : text | ||
| } |
There was a problem hiding this comment.
Redundant normalization duplicates existing truncation logic
Low Severity
The new normalizeSummary function duplicates the whitespace-normalization and truncation-to-220-chars logic already performed by normalizeText + truncate in semantic-search-presentation.server.ts. Since semanticSearchKCD now enriches results with already-normalized summaries via getSemanticSearchPresentation, applying normalizeSummary on r.summary is redundant. Having the same magic number 220 in two places risks divergence during future changes.
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
|
2 participants
Add image thumbnails and content summaries to semantic search results to improve their presentation across all consumers.
The
summary,imageUrl, andimageAltfields are dynamically derived at query time from existing content metadata (MDX frontmatter, YAML) and URL patterns, avoiding the need for re-indexing. Fallbacks are provided for content without explicit metadata.Note
Medium Risk
Introduces new server-side enrichment that reads repo content/YAML at query time, which could affect performance and result consistency across environments, though changes are additive and include fallbacks.
Overview
Semantic search results are now enriched at query time with
summary,imageUrl, andimageAlt, derived from local repo metadata (MDX frontmatter and YAML data) plus type-based fallbacks, and surfaced throughsemanticSearchKCD, the/resources/searchsuggestions endpoint, and the MCPfind_contenttool.Search UIs (navbar autocomplete and
/searchpage) are updated to render thumbnails, display the new summaries (with truncation for small payloads), and handle invalid suggestion URLs more safely when displaying the path. Test scripts are tweaked to usevitest --runby default and add atest:watchscript.Written by Cursor Bugbot for commit e33068c. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Improvements