Skip to content

Commit 796f20e

Browse files
tannerlinsleytannerlinsley
committed
Address Charts catalog review findings
1 parent cf3baa8 commit 796f20e

13 files changed

Lines changed: 336 additions & 104 deletions

‎src/components/charts/ChartsCatalogChart.tsx‎

Lines changed: 35 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,15 @@ export function ChartsCatalogChart({
4242
revision?: number
4343
}) {
4444
const containerRef = React.useRef<HTMLDivElement>(null)
45+
const handleRef = React.useRef<ChartMountHandle | undefined>(undefined)
46+
const inputRef = React.useRef({ height, interactive, revision })
47+
const onStatusRef = React.useRef(onStatus)
4548
const [visible, setVisible] = React.useState(!defer)
4649
const [failed, setFailed] = React.useState(false)
4750

51+
inputRef.current = { height, interactive, revision }
52+
onStatusRef.current = onStatus
53+
4854
React.useEffect(() => {
4955
const container = containerRef.current
5056
if (!defer || visible || !container) return
@@ -71,7 +77,7 @@ export function ChartsCatalogChart({
7177
if (!container || !visible) return
7278

7379
let cancelled = false
74-
let handle: ChartMountHandle | undefined
80+
let mountedHandle: ChartMountHandle | undefined
7581
let width = measureWidth(container)
7682
const preloadLinks = module.preload.map((assetPath) => {
7783
const link = document.createElement('link')
@@ -95,54 +101,60 @@ export function ChartsCatalogChart({
95101

96102
const mounted = loaded.mount(container, {
97103
width,
98-
height,
99-
revision,
100-
interactive,
104+
...inputRef.current,
101105
})
102106
if (!isChartMountHandle(mounted)) {
103107
throw new TypeError('Invalid Charts catalog mount handle')
104108
}
105-
handle = mounted
106-
requestAnimationFrame(() => onStatus?.('ready'))
109+
mountedHandle = mounted
110+
handleRef.current = mounted
111+
requestAnimationFrame(() => {
112+
if (!cancelled) onStatusRef.current?.('ready')
113+
})
107114
})
108115
.catch((error: unknown) => {
109116
if (cancelled) return
110117
console.error(`Unable to mount Charts catalog case ${caseId}`, error)
111118
setFailed(true)
112-
onStatus?.('error')
119+
onStatusRef.current?.('error')
113120
})
114121

115122
const resizeObserver = new ResizeObserver(() => {
116123
const nextWidth = measureWidth(container)
117124
if (nextWidth === width || nextWidth < 1) return
118125
width = nextWidth
119-
handle?.update({
126+
handleRef.current?.update({
120127
width,
121-
height,
122-
revision,
123-
interactive,
128+
...inputRef.current,
124129
})
125-
onStatus?.('resize')
130+
onStatusRef.current?.('resize')
126131
})
127132
resizeObserver.observe(container)
128133

129134
return () => {
130135
cancelled = true
131136
resizeObserver.disconnect()
132-
handle?.destroy()
137+
mountedHandle?.destroy()
138+
if (handleRef.current === mountedHandle) {
139+
handleRef.current = undefined
140+
}
133141
for (const link of preloadLinks) link.remove()
134142
container.replaceChildren()
135143
}
136-
}, [
137-
artifactRevision,
138-
caseId,
139-
height,
140-
interactive,
141-
module,
142-
onStatus,
143-
revision,
144-
visible,
145-
])
144+
}, [artifactRevision, caseId, module, visible])
145+
146+
React.useEffect(() => {
147+
const container = containerRef.current
148+
const handle = handleRef.current
149+
if (!container || !handle) return
150+
151+
handle.update({
152+
width: measureWidth(container),
153+
height,
154+
interactive,
155+
revision,
156+
})
157+
}, [height, interactive, revision])
146158

147159
return (
148160
<div

‎src/components/charts/ChartsCatalogPages.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ export function ChartsCatalogDetail({
247247

248248
<div
249249
className={`mx-auto grid gap-6 ${comparison ? 'xl:grid-cols-2' : ''} ${
250-
width === 'compact' ? 'max-w-2xl' : ''
250+
width === 'compact' ? 'max-w-[640px]' : 'max-w-[960px]'
251251
}`}
252252
>
253253
<ChartPanel label="TanStack">

‎src/routes/charts.catalog_.assets.$artifactRevision.$.ts‎

Lines changed: 65 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,35 +19,83 @@ async function serveCatalogAsset({
1919
request: Request
2020
params: { artifactRevision: string; _splat: string }
2121
}) {
22+
const {
23+
classifyChartsCatalogAssetError,
24+
getChartsCatalogManifestAtRevision,
25+
getVerifiedChartsCatalogAssetSource,
26+
} = await import('~/utils/charts-catalog.server')
27+
28+
let manifest: Awaited<ReturnType<typeof getChartsCatalogManifestAtRevision>>
2229
try {
23-
const {
24-
getChartsCatalogManifestAtRevision,
25-
getVerifiedChartsCatalogAssetSource,
26-
} = await import('~/utils/charts-catalog.server')
27-
const manifest = await getChartsCatalogManifestAtRevision(
28-
params.artifactRevision,
30+
manifest = await getChartsCatalogManifestAtRevision(params.artifactRevision)
31+
} catch (error) {
32+
return handleCatalogAssetError(
33+
error,
34+
classifyChartsCatalogAssetError,
35+
request.method,
2936
)
30-
const asset = parseChartsCatalogAssetRequest({
37+
}
38+
39+
let asset: ReturnType<typeof parseChartsCatalogAssetRequest>
40+
try {
41+
asset = parseChartsCatalogAssetRequest({
3142
artifactRevision: params.artifactRevision,
3243
assetPath: params._splat,
3344
manifest,
3445
})
35-
const descriptor = manifest.assets[asset.repoPath]
36-
if (!descriptor) throw new TypeError('Unlisted Charts catalog asset')
46+
} catch (error) {
47+
if (error instanceof TypeError) throw notFound()
48+
throw error
49+
}
50+
51+
const descriptor = manifest.assets[asset.repoPath]
52+
if (!descriptor) throw notFound()
3753

38-
const source = await getVerifiedChartsCatalogAssetSource(
54+
let source: string
55+
try {
56+
source = await getVerifiedChartsCatalogAssetSource(
3957
params.artifactRevision,
4058
asset.repoPath,
4159
descriptor,
4260
)
61+
} catch (error) {
62+
return handleCatalogAssetError(
63+
error,
64+
classifyChartsCatalogAssetError,
65+
request.method,
66+
)
67+
}
68+
69+
return new Response(request.method === 'HEAD' ? null : source, {
70+
headers: {
71+
...asset.headers,
72+
'Content-Length': String(descriptor.bytes),
73+
},
74+
})
75+
}
4376

44-
return new Response(request.method === 'HEAD' ? null : source, {
45-
headers: {
46-
...asset.headers,
47-
'Content-Length': String(descriptor.bytes),
77+
function handleCatalogAssetError(
78+
error: unknown,
79+
classify: (error: unknown) => 'not-found' | 'unavailable' | 'internal',
80+
method: string,
81+
) {
82+
const classification = classify(error)
83+
if (classification === 'not-found') throw notFound()
84+
85+
console.error('[Charts catalog asset] Failed to serve asset', error)
86+
if (classification === 'unavailable') {
87+
return new Response(
88+
method === 'HEAD' ? null : 'Charts catalog asset temporarily unavailable',
89+
{
90+
status: 503,
91+
headers: {
92+
'Cache-Control': 'no-store',
93+
'Cloudflare-CDN-Cache-Control': 'no-store',
94+
'Retry-After': '60',
95+
},
4896
},
49-
})
50-
} catch {
51-
throw notFound()
97+
)
5298
}
99+
100+
throw error
53101
}

‎src/routes/charts.catalog_.catalog[.]json.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { createFileRoute } from '@tanstack/react-router'
2-
import { chartsCatalogPublicationCacheTag } from '~/utils/charts-catalog'
2+
import { chartsCatalogPublicationCacheHeaders } from '~/utils/charts-catalog'
33

44
export const Route = createFileRoute('/charts/catalog_/catalog.json')({
55
server: {
@@ -11,10 +11,7 @@ export const Route = createFileRoute('/charts/catalog_/catalog.json')({
1111

1212
return Response.json(publication.manifest, {
1313
headers: {
14-
'Cache-Control': 'public, max-age=60, must-revalidate',
15-
'Cloudflare-CDN-Cache-Control':
16-
'public, max-age=300, stale-while-revalidate=300',
17-
'Cache-Tag': chartsCatalogPublicationCacheTag,
14+
...chartsCatalogPublicationCacheHeaders,
1815
'X-Charts-Catalog-Artifact-Revision': publication.artifactRevision,
1916
},
2017
})

‎src/utils/charts-catalog-embed.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { isChartsCatalogCaseId } from './charts-catalog'
2+
13
export const chartsCatalogEmbedPrefix = '/charts/catalog/embed/'
24

35
export type ChartsCatalogEmbed = {
@@ -20,7 +22,15 @@ export type ChartsCatalogEmbedRouteSearch = {
2022
}
2123

2224
export function isChartsCatalogEmbedPath(pathname: string) {
23-
return /^\/charts\/catalog\/embed\/[a-z0-9]+(?:-[a-z0-9]+)*\/$/.test(pathname)
25+
if (
26+
!pathname.startsWith(chartsCatalogEmbedPrefix) ||
27+
!pathname.endsWith('/')
28+
) {
29+
return false
30+
}
31+
return isChartsCatalogCaseId(
32+
pathname.slice(chartsCatalogEmbedPrefix.length, -1),
33+
)
2434
}
2535

2636
export function parseChartsCatalogEmbed(

‎src/utils/charts-catalog.functions.ts‎

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,23 +5,24 @@ import type {
55
ChartsCatalogCase,
66
ChartsCatalogPublication,
77
} from './charts-catalog'
8-
import { chartsCatalogPublicationCacheTag } from './charts-catalog'
8+
import {
9+
chartsCatalogCaseIdSchema,
10+
chartsCatalogPublicationCacheHeaders,
11+
} from './charts-catalog'
912

1013
const defaultReferenceRenderer = 'observable-plot'
1114

12-
const caseIdSchema = v.pipe(v.string(), v.regex(/^[a-z0-9]+(?:-[a-z0-9]+)*$/))
13-
1415
const comparisonInputSchema = v.strictObject({
1516
comparison: v.boolean(),
1617
})
1718

1819
const caseInputSchema = v.strictObject({
19-
caseId: caseIdSchema,
20+
caseId: chartsCatalogCaseIdSchema,
2021
comparison: v.boolean(),
2122
})
2223

2324
const embedCaseInputSchema = v.strictObject({
24-
caseId: caseIdSchema,
25+
caseId: chartsCatalogCaseIdSchema,
2526
})
2627

2728
export const getChartsCatalogIndex = createServerFn({
@@ -63,16 +64,18 @@ export const getChartsCatalogCase = createServerFn({ method: 'GET' })
6364
if (!catalogCase) return null
6465

6566
const { getChartsCatalogSource } = await import('./charts-catalog.server')
66-
const tanstackSource = await getChartsCatalogSource(
67-
publication.manifest.revision,
68-
catalogCase.code.tanstack,
69-
)
70-
const comparisonSource = data.comparison
71-
? await getChartsCatalogSource(
72-
publication.manifest.revision,
73-
catalogCase.code.reference,
74-
)
75-
: undefined
67+
const [tanstackSource, comparisonSource] = await Promise.all([
68+
getChartsCatalogSource(
69+
publication.manifest.revision,
70+
catalogCase.code.tanstack,
71+
),
72+
data.comparison
73+
? getChartsCatalogSource(
74+
publication.manifest.revision,
75+
catalogCase.code.reference,
76+
)
77+
: Promise.resolve(undefined),
78+
])
7679

7780
setCatalogResponseHeaders()
7881
return {
@@ -148,10 +151,9 @@ function getCaseMetadata(catalogCase: ChartsCatalogCase) {
148151
}
149152

150153
function setCatalogResponseHeaders() {
151-
setResponseHeader('Cache-Control', 'public, max-age=60, must-revalidate')
152-
setResponseHeader(
153-
'Cloudflare-CDN-Cache-Control',
154-
'public, max-age=300, stale-while-revalidate=300',
155-
)
156-
setResponseHeader('Cache-Tag', chartsCatalogPublicationCacheTag)
154+
for (const [name, value] of Object.entries(
155+
chartsCatalogPublicationCacheHeaders,
156+
)) {
157+
setResponseHeader(name, value)
158+
}
157159
}

0 commit comments

Comments
 (0)