Fix search#875
Merged
tannerlinsley merged 10 commits intomainfrom Apr 30, 2026
Merged
Conversation
✅ Deploy Preview for tanstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis pull request adds framework-aware search and documentation features to the documentation portal. It introduces framework extraction from markdown, implements framework persistence in search results, refactors search filtering logic, updates sitemap configuration and properties across libraries, and adds framework-variant alternate links for SEO. Includes comprehensive test coverage for new utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SearchModal
participant FilterLogic as Filter Logic
participant SearchAPI as Search API
participant DataProcessor as Data Processor
participant DropdownContent as Dropdown Content
User->>SearchModal: Open search & type query
SearchModal->>FilterLogic: Build search filters via buildSearchFilters
FilterLogic->>FilterLogic: Compute available options from library visibility
FilterLogic->>SearchAPI: Configure.filters (internally computed)
SearchAPI-->>SearchModal: Return search results with framework/urlWithAnchor
SearchModal->>DataProcessor: Process hit (extract hit.framework, hit.urlWithAnchor)
DataProcessor->>DataProcessor: Check shouldPersistFrameworkForHit
alt Framework should persist
DataProcessor->>SearchModal: Preserve framework preference
else Framework should not persist
DataProcessor->>SearchModal: Clear selectedFramework
end
SearchModal->>DropdownContent: Render with portal={false}
DropdownContent-->>User: Display filtered options without portal
User->>SearchModal: Click search result
SearchModal->>SearchModal: Close modal (framework persisted if applicable)
sequenceDiagram
participant RouteHandler as Route Handler
participant DocsFunctions as Docs Functions
participant MarkdownParser as Markdown Parser
participant ManifestBuilder as Manifest Builder
participant HeadGenerator as Head Generator
RouteHandler->>DocsFunctions: fetchDocsPage(libraryId, version, path)
DocsFunctions->>ManifestBuilder: buildDocsManifest (if local mode)
ManifestBuilder->>MarkdownParser: Parse markdown files
MarkdownParser->>MarkdownParser: Extract frameworks via extractFrameworksFromMarkdown
MarkdownParser-->>ManifestBuilder: Return manifest with framework data
ManifestBuilder-->>DocsFunctions: Manifest ready
DocsFunctions->>DocsFunctions: Extract frameworks from markdown content
DocsFunctions-->>RouteHandler: Return docs page + frameworks array
RouteHandler->>HeadGenerator: Generate head with alternate links
HeadGenerator->>HeadGenerator: Create link rel=alternate for each framework variant
HeadGenerator-->>RouteHandler: Return meta + links for SEO
RouteHandler-->>RouteHandler: Render docs page with alternate links in head
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 3
🧹 Nitpick comments (1)
tests/filter-framework-content.test.ts (1)
3-9: 💤 Low valueConsider using Node.js built-in assert for consistency.
The sibling test file
tests/search-records.test.tsusesnode:assert/strict. Using the same assertion library would improve consistency across the test suite.Suggested change
-function assertEqual(actual: unknown, expected: unknown, message: string) { - const actualJson = JSON.stringify(actual) - const expectedJson = JSON.stringify(expected) - if (actualJson !== expectedJson) { - throw new Error(`${message}: expected ${expectedJson}, got ${actualJson}`) - } -} +import assert from 'node:assert/strict'Then use
assert.deepEqual(actual, expected, message)for assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/filter-framework-content.test.ts` around lines 3 - 9, The test defines a custom helper function assertEqual which duplicates Node's assertion behavior; replace it with the built-in strict assert module by importing from "node:assert/strict" and remove the custom function assertEqual, then update test usages to call assert.deepEqual(actual, expected, message) (or assert.deepStrictEqual if preferred) so the test suite matches the sibling file's assertion style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/`$libraryId/$version.docs.$.tsx:
- Around line 71-76: frameworkVariantLinks constructs alternate links that
append ".md" to the docs path, which causes the loader to resolve to ".md.md"
(or "/docs/.md") and break routes; update the href template in the
frameworkVariantLinks mapping to stop appending ".md" (use
`/\${libraryId}/\${version}/docs/\${docsPath}?framework=\${framework}` or ensure
docsPath is normalized so no ".md" is added) so the loader receives the raw
docsPath it expects and alternate links point to existing routes.
In `@src/utils/docs.functions.ts`:
- Around line 158-159: frontMatter.data.redirectFrom may be a string and the
current for...of loops over characters; coerce it to an array before iterating
(e.g., create a redirectFromList from frontMatter.data.redirectFrom using
Array.isArray check or wrapping non-arrays) and filter out falsy/non-string
entries, then iterate redirectFromList and call normalizeDocsRedirectPath for
each; update the loop that currently references redirectFrom,
normalizeDocsRedirectPath, and normalizedRedirect to use the new normalized list
so single-string frontmatter yields one redirect rather than per-character
redirects.
- Around line 150-154: The fetchRepoFile call can throw and should not abort the
whole manifest build; wrap the await fetchRepoFile(repo, branch, node.path) in a
try/catch inside the same loop (where you currently check if (!file) { continue
}) so that on any recoverable GitHub error you log or warn and continue to the
next node instead of letting the exception propagate; keep the existing
null/undefined check for file and reference fetchRepoFile, node.path and the
continue behavior when skipping the problematic file.
---
Nitpick comments:
In `@tests/filter-framework-content.test.ts`:
- Around line 3-9: The test defines a custom helper function assertEqual which
duplicates Node's assertion behavior; replace it with the built-in strict assert
module by importing from "node:assert/strict" and remove the custom function
assertEqual, then update test usages to call assert.deepEqual(actual, expected,
message) (or assert.deepStrictEqual if preferred) so the test suite matches the
sibling file's assertion style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98295c36-c9c9-4f77-9cd7-8a7b26d6ca74
📒 Files selected for processing (12)
src/components/Dropdown.tsxsrc/components/SearchModal.tsxsrc/libraries/libraries.tssrc/libraries/types.tssrc/routes/$libraryId/$version.docs.$.tsxsrc/utils/docs.functions.tssrc/utils/documents.server.tssrc/utils/markdown/filterFrameworkContent.tssrc/utils/searchRecords.tssrc/utils/sitemap.tstests/filter-framework-content.test.tstests/search-records.test.ts
Comment on lines
+71
to
+76
| const frameworkVariantLinks = (loaderData?.frameworks ?? []).map( | ||
| (framework) => ({ | ||
| rel: 'alternate', | ||
| type: 'text/markdown', | ||
| href: `/${libraryId}/${version}/docs/${docsPath}.md?framework=${framework}`, | ||
| }), |
There was a problem hiding this comment.
Alternate framework links currently point to non-existent docs routes.
Line 75 appends .md to the URL path, but this route’s loader resolves files by appending .md again. That can produce ${docsPath}.md.md (or /docs/.md at docs root) and break these alternates.
Proposed fix
- const frameworkVariantLinks = (loaderData?.frameworks ?? []).map(
+ const docsPathSuffix = docsPath ? `/${docsPath}` : ''
+ const frameworkVariantLinks = (loaderData?.frameworks ?? []).map(
(framework) => ({
rel: 'alternate',
type: 'text/markdown',
- href: `/${libraryId}/${version}/docs/${docsPath}.md?framework=${framework}`,
+ href: `/${libraryId}/${version}/docs${docsPathSuffix}?framework=${encodeURIComponent(framework)}`,
}),
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`$libraryId/$version.docs.$.tsx around lines 71 - 76,
frameworkVariantLinks constructs alternate links that append ".md" to the docs
path, which causes the loader to resolve to ".md.md" (or "/docs/.md") and break
routes; update the href template in the frameworkVariantLinks mapping to stop
appending ".md" (use
`/\${libraryId}/\${version}/docs/\${docsPath}?framework=\${framework}` or ensure
docsPath is normalized so no ".md" is added) so the loader receives the raw
docsPath it expects and alternate links point to existing routes.
Comment on lines
+150
to
+154
| const file = await fetchRepoFile(repo, branch, node.path) | ||
|
|
||
| if (!file) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Single-file fetch failures can abort the entire manifest build.
Line 150 can throw, and currently one recoverable GitHub error takes down the full manifest generation path. This should skip that file instead of failing the whole build.
Proposed fix
- const file = await fetchRepoFile(repo, branch, node.path)
+ let file: string | undefined
+ try {
+ file = await fetchRepoFile(repo, branch, node.path)
+ } catch (error) {
+ if (!isRecoverableGitHubContentError(error)) {
+ throw error
+ }
+ continue
+ }
if (!file) {
continue
}📝 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.
Suggested change
| const file = await fetchRepoFile(repo, branch, node.path) | |
| if (!file) { | |
| continue | |
| } | |
| let file: string | undefined | |
| try { | |
| file = await fetchRepoFile(repo, branch, node.path) | |
| } catch (error) { | |
| if (!isRecoverableGitHubContentError(error)) { | |
| throw error | |
| } | |
| continue | |
| } | |
| if (!file) { | |
| continue | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/docs.functions.ts` around lines 150 - 154, The fetchRepoFile call
can throw and should not abort the whole manifest build; wrap the await
fetchRepoFile(repo, branch, node.path) in a try/catch inside the same loop
(where you currently check if (!file) { continue }) so that on any recoverable
GitHub error you log or warn and continue to the next node instead of letting
the exception propagate; keep the existing null/undefined check for file and
reference fetchRepoFile, node.path and the continue behavior when skipping the
problematic file.
Comment on lines
+158
to
+159
| for (const redirectFrom of frontMatter.data.redirectFrom ?? []) { | ||
| const normalizedRedirect = normalizeDocsRedirectPath( |
There was a problem hiding this comment.
redirectFrom should be normalized before iteration.
Line 158 assumes redirectFrom is an array. If frontmatter provides a single string, the loop iterates characters and generates invalid redirects.
Proposed fix
- for (const redirectFrom of frontMatter.data.redirectFrom ?? []) {
+ const redirectFromValues = Array.isArray(frontMatter.data.redirectFrom)
+ ? frontMatter.data.redirectFrom
+ : typeof frontMatter.data.redirectFrom === 'string'
+ ? [frontMatter.data.redirectFrom]
+ : []
+
+ for (const redirectFrom of redirectFromValues) {📝 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.
Suggested change
| for (const redirectFrom of frontMatter.data.redirectFrom ?? []) { | |
| const normalizedRedirect = normalizeDocsRedirectPath( | |
| const redirectFromValues = Array.isArray(frontMatter.data.redirectFrom) | |
| ? frontMatter.data.redirectFrom | |
| : typeof frontMatter.data.redirectFrom === 'string' | |
| ? [frontMatter.data.redirectFrom] | |
| : [] | |
| for (const redirectFrom of redirectFromValues) { | |
| const normalizedRedirect = normalizeDocsRedirectPath( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/docs.functions.ts` around lines 158 - 159,
frontMatter.data.redirectFrom may be a string and the current for...of loops
over characters; coerce it to an array before iterating (e.g., create a
redirectFromList from frontMatter.data.redirectFrom using Array.isArray check or
wrapping non-arrays) and filter out falsy/non-string entries, then iterate
redirectFromList and call normalizeDocsRedirectPath for each; update the loop
that currently references redirectFrom, normalizeDocsRedirectPath, and
normalizedRedirect to use the new normalized list so single-string frontmatter
yields one redirect rather than per-character redirects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements