Render the Charts catalog as native site content - #1068
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Charts Catalog system with validated manifests, server-side publication and asset handling, catalog pages, dynamically mounted charts, iframe embedding, cache invalidation, sitemap integration, and generated TanStack Router routes. ChangesCharts Catalog
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CatalogPage
participant CatalogFunctions
participant CatalogServer
participant AssetRoute
participant ChartRuntime
CatalogPage->>CatalogFunctions: request catalog metadata
CatalogFunctions->>CatalogServer: load and validate publication
CatalogServer-->>CatalogFunctions: return catalog case and module data
CatalogPage->>AssetRoute: request immutable chart asset
AssetRoute->>CatalogServer: verify asset bytes and digest
CatalogServer-->>AssetRoute: return verified asset
AssetRoute-->>ChartRuntime: provide runtime module
CatalogPage->>ChartRuntime: mount and resize chart
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
tanstack-com | c82d391 | Commit Preview URL Branch Preview URL |
Jul 29 2026, 06:17 AM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/utils/charts-catalog.ts (2)
49-56: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider restricting source URLs to
https:.
sourceUrlSchemaacceptshttp:, and these URLs are rendered as outbound links from catalog pages. Since the manifest is generated from a known GitHub repo,https:-only costs nothing and avoids mixed-content/downgrade links.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/charts-catalog.ts` around lines 49 - 56, Update sourceUrlSchema to accept only URLs whose protocol is https:, removing the http: allowance while preserving the existing URL validation and error message.
444-454: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
chartsCatalogBasePathinstead of hardcoded literals.♻️ Proposed tweak
- { path: '/charts/catalog/' }, - { path: '/charts/catalog/all/' }, + { path: chartsCatalogBasePath }, + { path: `${chartsCatalogBasePath}all/` },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/charts-catalog.ts` around lines 444 - 454, Update getChartsCatalogSitemapEntries to build the catalog sitemap paths from the existing chartsCatalogBasePath constant instead of hardcoded '/charts/catalog/' literals, including the base and all-cases entries while preserving the manifest case routes.src/utils/charts-catalog.functions.ts (2)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
caseIdSchema.The same case-ID regex is defined privately in
src/utils/charts-catalog.ts(lines 20-23). Export it there and import here so the ID contract can't drift between validation sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/charts-catalog.functions.ts` at line 12, Remove the private duplicate caseIdSchema definition from charts-catalog.functions.ts and reuse the exported caseIdSchema from charts-catalog.ts. Export the existing schema in charts-catalog.ts and import it into the functions module, preserving the current validation contract.
66-75: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFetch the two sources concurrently in comparison mode.
⚡ Proposed tweak
- const tanstackSource = await getChartsCatalogSource( - publication.manifest.revision, - catalogCase.code.tanstack, - ) - const comparisonSource = data.comparison - ? await getChartsCatalogSource( - publication.manifest.revision, - catalogCase.code.reference, - ) - : undefined + const [tanstackSource, comparisonSource] = await Promise.all([ + getChartsCatalogSource( + publication.manifest.revision, + catalogCase.code.tanstack, + ), + data.comparison + ? getChartsCatalogSource( + publication.manifest.revision, + catalogCase.code.reference, + ) + : Promise.resolve(undefined), + ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/charts-catalog.functions.ts` around lines 66 - 75, Update the source-fetching flow around getChartsCatalogSource so tanstackSource and comparisonSource are requested concurrently when data.comparison is present, while retaining undefined for comparisonSource when it is absent. Preserve the existing revision and catalogCase codes for each request.src/routes/charts.catalog_.catalog[.]json.ts (1)
12-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the cache-header values with
setCatalogResponseHeaders.The same three cache directives are literal-duplicated in
src/utils/charts-catalog.functions.ts(lines 150-157). Export a single constant object from~/utils/charts-catalogand spread it here so the catalog JSON and the server functions can't drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/charts.catalog_.catalog`[.]json.ts around lines 12 - 20, The cache directives in the catalog JSON response are duplicated instead of shared. Export a single cache-header constants object from the charts-catalog utility, update setCatalogResponseHeaders to use it, and spread that object into the headers of the route’s Response.json call while preserving the existing non-cache headers.src/utils/charts-catalog-embed.ts (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication: embed path prefix hardcoded in two places.
chartsCatalogEmbedPrefixand the literal/charts/catalog/embed/insideisChartsCatalogEmbedPath's regex encode the same path independently. If the prefix ever changes, one could be updated without the other, silently breaking either the path grammar check or the caseId slice inparseChartsCatalogEmbed.♻️ Optional: derive the regex from the shared constant
-export function isChartsCatalogEmbedPath(pathname: string) { - return /^\/charts\/catalog\/embed\/[a-z0-9]+(?:-[a-z0-9]+)*\/$/.test(pathname) -} +const embedCaseIdPattern = /^[a-z0-9]+(?:-[a-z0-9]+)*$/ + +export function isChartsCatalogEmbedPath(pathname: string) { + if ( + !pathname.startsWith(chartsCatalogEmbedPrefix) || + !pathname.endsWith('/') + ) { + return false + } + const caseId = pathname.slice( + chartsCatalogEmbedPrefix.length, + -1, + ) + return embedCaseIdPattern.test(caseId) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/charts-catalog-embed.ts` around lines 1 - 12, Update isChartsCatalogEmbedPath to derive its path-matching pattern from chartsCatalogEmbedPrefix instead of hardcoding /charts/catalog/embed/. Preserve the existing caseId grammar and trailing-slash requirements while ensuring the shared prefix remains the single source of truth for both validation and parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/charts/ChartsCatalogChart.tsx`:
- Around line 69-145: Split the chart lifecycle in the effect containing the
dynamic import: keep mounting, preload management, and cleanup dependent only on
artifactRevision, caseId, module, and visible, while storing the mounted
ChartMountHandle in a ref. Add a separate effect that invokes handle.update()
when height, revision, or interactive changes, reusing the existing
ChartMountInput values and preserving ResizeObserver updates; avoid remounting
for these non-structural changes.
In `@src/components/charts/ChartsCatalogEmbed.tsx`:
- Around line 90-105: Add a restrictive sandbox attribute to the iframe rendered
by the ChartsCatalogEmbed component, without granting same-origin, popups,
top-level navigation, or form submission permissions; preserve the existing
postChartTheme message flow and other iframe props.
In `@src/components/charts/ChartsCatalogPages.tsx`:
- Around line 223-245: Update the width-controlled chart container in the
component rendering the compact/wide grid so the “compact” and “wide” options
enforce the widths advertised by the toggle labels: compact at 640px and wide at
960px. Replace the current max-w-2xl/no-constraint behavior while preserving the
existing width state selection and layout styling.
In `@src/routes/charts.catalog_.assets`.$artifactRevision.$.ts:
- Around line 50-52: Replace the blanket catch around asset retrieval with
error-aware handling: log the failure, map GitHubContentError network,
rate-limit, and server kinds to a 503 response with Cache-Control no-store, and
reserve notFound() for genuinely unlisted assets. Allow SHA-256 or
byte-integrity mismatches to propagate as server-side integrity failures rather
than becoming 404 responses.
In `@src/utils/documents.server.ts`:
- Around line 523-560: Validate inputs at the start of fetchRepoRawFile before
building the cache key or selecting local/remote fetching: require a well-formed
repoPair with exactly non-empty owner and repository segments, and reject
filepath values containing traversal segments, backslashes, leading slashes, or
empty segments. Mirror the validation behavior used by resolveGitHubRef and fail
immediately for invalid input; keep valid-path caching and fetch behavior
unchanged.
---
Nitpick comments:
In `@src/routes/charts.catalog_.catalog`[.]json.ts:
- Around line 12-20: The cache directives in the catalog JSON response are
duplicated instead of shared. Export a single cache-header constants object from
the charts-catalog utility, update setCatalogResponseHeaders to use it, and
spread that object into the headers of the route’s Response.json call while
preserving the existing non-cache headers.
In `@src/utils/charts-catalog-embed.ts`:
- Around line 1-12: Update isChartsCatalogEmbedPath to derive its path-matching
pattern from chartsCatalogEmbedPrefix instead of hardcoding
/charts/catalog/embed/. Preserve the existing caseId grammar and trailing-slash
requirements while ensuring the shared prefix remains the single source of truth
for both validation and parsing.
In `@src/utils/charts-catalog.functions.ts`:
- Line 12: Remove the private duplicate caseIdSchema definition from
charts-catalog.functions.ts and reuse the exported caseIdSchema from
charts-catalog.ts. Export the existing schema in charts-catalog.ts and import it
into the functions module, preserving the current validation contract.
- Around line 66-75: Update the source-fetching flow around
getChartsCatalogSource so tanstackSource and comparisonSource are requested
concurrently when data.comparison is present, while retaining undefined for
comparisonSource when it is absent. Preserve the existing revision and
catalogCase codes for each request.
In `@src/utils/charts-catalog.ts`:
- Around line 49-56: Update sourceUrlSchema to accept only URLs whose protocol
is https:, removing the http: allowance while preserving the existing URL
validation and error message.
- Around line 444-454: Update getChartsCatalogSitemapEntries to build the
catalog sitemap paths from the existing chartsCatalogBasePath constant instead
of hardcoded '/charts/catalog/' literals, including the base and all-cases
entries while preserving the manifest case routes.
🪄 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: d8a2e59a-58ba-4427-8851-268211c85565
📒 Files selected for processing (30)
src/components/charts/ChartsCatalogChart.tsxsrc/components/charts/ChartsCatalogEmbed.tsxsrc/components/charts/ChartsCatalogPages.tsxsrc/routeTree.gen.tssrc/routes/api/github/webhook.tssrc/routes/charts.catalog.all.tsxsrc/routes/charts.catalog.charts.$caseId.tsxsrc/routes/charts.catalog.index.tsxsrc/routes/charts.catalog.tsxsrc/routes/charts.catalog_.assets.$artifactRevision.$.tssrc/routes/charts.catalog_.catalog[.]json.tssrc/routes/charts.catalog_.embed.$caseId.tsxsrc/server.tssrc/styles/app.csssrc/utils/charts-catalog-assets.tssrc/utils/charts-catalog-embed.tssrc/utils/charts-catalog.functions.tssrc/utils/charts-catalog.server.tssrc/utils/charts-catalog.tssrc/utils/docs-webhook-sources.tssrc/utils/documents.server.tssrc/utils/frame-embedding.tssrc/utils/sitemap.tstests/charts-catalog-assets.test.tstests/charts-catalog-frame-embedding.test.tstests/charts-catalog-manifest.test.tstests/charts-catalog-raw-content.test.tstests/charts-catalog-search.test.tstests/charts-catalog-site-contracts.test.tstests/charts-catalog-test-fixture.ts
Comment on lines
+90
to
+105
| return ( | ||
| <iframe | ||
| title={iframeTitle} | ||
| {...iframeProps} | ||
| ref={frameRef} | ||
| src={shouldLoad ? src : undefined} | ||
| loading={loading} | ||
| referrerPolicy="strict-origin-when-cross-origin" | ||
| className={`block w-full ${className ?? ''}`.trim()} | ||
| data-chart-catalog-embed={chartEmbed.caseId} | ||
| onLoad={(event) => { | ||
| onLoad?.(event) | ||
| postChartTheme() | ||
| }} | ||
| /> | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a sandbox attribute to the embed iframe.
Static analysis flags the missing sandbox on this <iframe>. Even though src is restricted to tanstack.com catalog embed paths, sandboxing still adds defense-in-depth against a compromised chart artifact navigating the top-level page, opening popups, or submitting forms. postMessage works fine across sandboxed frames without allow-same-origin, so this shouldn't break the existing theme-sync protocol.
🔒 Proposed fix
<iframe
title={iframeTitle}
{...iframeProps}
ref={frameRef}
src={shouldLoad ? src : undefined}
loading={loading}
referrerPolicy="strict-origin-when-cross-origin"
+ sandbox="allow-scripts"
className={`block w-full ${className ?? ''}`.trim()}
data-chart-catalog-embed={chartEmbed.caseId}
onLoad={(event) => {
onLoad?.(event)
postChartTheme()
}}
/>📝 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
| return ( | |
| <iframe | |
| title={iframeTitle} | |
| {...iframeProps} | |
| ref={frameRef} | |
| src={shouldLoad ? src : undefined} | |
| loading={loading} | |
| referrerPolicy="strict-origin-when-cross-origin" | |
| className={`block w-full ${className ?? ''}`.trim()} | |
| data-chart-catalog-embed={chartEmbed.caseId} | |
| onLoad={(event) => { | |
| onLoad?.(event) | |
| postChartTheme() | |
| }} | |
| /> | |
| ) | |
| return ( | |
| <iframe | |
| title={iframeTitle} | |
| {...iframeProps} | |
| ref={frameRef} | |
| src={shouldLoad ? src : undefined} | |
| loading={loading} | |
| referrerPolicy="strict-origin-when-cross-origin" | |
| sandbox="allow-scripts" | |
| className={`block w-full ${className ?? ''}`.trim()} | |
| data-chart-catalog-embed={chartEmbed.caseId} | |
| onLoad={(event) => { | |
| onLoad?.(event) | |
| postChartTheme() | |
| }} | |
| /> | |
| ) |
🧰 Tools
🪛 React Doctor (0.9.1)
[error] 91-91: An <iframe> with no sandbox is a security hole: the embedded page gets full access to your site.
Add sandbox="" or a curated value so embedded pages cannot get full access to your site by default.
(iframe-missing-sandbox)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/charts/ChartsCatalogEmbed.tsx` around lines 90 - 105, Add a
restrictive sandbox attribute to the iframe rendered by the ChartsCatalogEmbed
component, without granting same-origin, popups, top-level navigation, or form
submission permissions; preserve the existing postChartTheme message flow and
other iframe props.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/charts/ChartsCatalogChart.tsx`:
- Around line 51-52: Move the inputRef and onStatusRef assignments out of render
in ChartsCatalogChart and synchronize them in committed effects that run before
the lifecycle effect, ensuring mount, update, and status callbacks only observe
committed props.
🪄 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: de466e3d-e74d-4d8f-85c2-2767bd214c77
📒 Files selected for processing (15)
src/components/charts/ChartsCatalogChart.tsxsrc/components/charts/ChartsCatalogPages.tsxsrc/routes/charts.catalog_.assets.$artifactRevision.$.tssrc/routes/charts.catalog_.catalog[.]json.tssrc/utils/charts-catalog-embed.tssrc/utils/charts-catalog.functions.tssrc/utils/charts-catalog.server.tssrc/utils/charts-catalog.tssrc/utils/documents.server.tstests/charts-catalog-asset-route.test.tstests/charts-catalog-assets.test.tstests/charts-catalog-manifest.test.tstests/charts-catalog-raw-content.test.tstests/charts-catalog-search.test.tstests/charts-catalog-site-contracts.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/routes/charts.catalog_.catalog[.]json.ts
- src/components/charts/ChartsCatalogPages.tsx
- src/utils/charts-catalog.functions.ts
- tests/charts-catalog-manifest.test.ts
- src/utils/charts-catalog-embed.ts
- src/utils/charts-catalog.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Renders the generated TanStack Charts catalog through the existing site content pipeline.
catalog-distto an exact artifact commit?compare=1opt-inSummary by CodeRabbit