perf: load markdown source code on copy instead of on main request #1386
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR centralises README resolution into a new server utility and adds a dedicated markdown API route. The existing HTML readme route now delegates to the utility and returns an updated ReadmeResponse shape using Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/package/[[org]]/[name].vue (1)
103-111:⚠️ Potential issue | 🟠 MajorUpdate the README default to the new mdExists shape.
The default still includesmd, which no longer exists on ReadmeResponse and should fail type checking.🐛 Suggested fix
- { default: () => ({ html: '', md: '', playgroundLinks: [], toc: [] }) }, + { default: () => ({ html: '', mdExists: false, playgroundLinks: [], toc: [] }) },
🧹 Nitpick comments (3)
server/utils/readme.ts (1)
321-323: Include mdExists in the empty-content short‑circuit.
Returning a consistent shape avoids callers handlingundefinedand keeps the response aligned with ReadmeResponse.♻️ Suggested tweak
- if (!content) return { html: '', playgroundLinks: [], toc: [] } + if (!content) return { html: '', mdExists: false, playgroundLinks: [], toc: [] }server/api/registry/readme/markdown/[...pkg].get.ts (1)
5-12: Align the markdown endpoint payload with ReadmeMarkdownResponse.
Either return onlymarkdownhere or expand the type to match the full resolver output, so the contract and payload stay tight.♻️ Suggested tweak
export default async function getMarkdownReadme(event: H3Event) { try { - return await resolvePackageReadmeSource(event) + const { markdown } = await resolvePackageReadmeSource(event) + return { markdown } } catch (error: unknown) {app/pages/package/[[org]]/[name].vue (1)
2-9: Skip re-fetching markdown if already loaded or fetch is pending.
When the markdown is already cached inreadmeMarkdownData, reuse it instead of fetching again. Also avoid re-fetching if a request is already in flight, sinceuseLazyFetch().execute()always triggers a new request in Nuxt 3/4.♻️ Suggested refactor
async function copyReadmeHandler() { - await fetchReadmeMarkdown() - - const markdown = readmeMarkdownData.value?.markdown + if (!readmeMarkdownData.value?.markdown && readmeMarkdownStatus.value !== 'pending') { + await fetchReadmeMarkdown() + } + const markdown = readmeMarkdownData.value?.markdown if (!markdown) return await copyReadme(markdown) }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/unit/server/utils/readme.spec.ts (1)
349-356: AssertmdExists: falsein the empty-state contract.This keeps the response shape contract explicit and prevents regressions.
Proposed fix
expect(result).toMatchObject({ html: '', + mdExists: false, playgroundLinks: [], toc: [], })
| const packagePath = getRouterParam(event, 'pkg') ?? '' | ||
| const { packageName, markdown, repoInfo } = await resolvePackageReadmeSource(packagePath) | ||
|
|
There was a problem hiding this comment.
Normalise packagePath before resolving.
Keep resolver input consistent with cache key normalisation to avoid duplicate cache entries and edge-case validation errors.
Proposed fix
- const packagePath = getRouterParam(event, 'pkg') ?? ''
- const { packageName, markdown, repoInfo } = await resolvePackageReadmeSource(packagePath)
+ const packagePath = (getRouterParam(event, 'pkg') ?? '').replace(/\/+$/, '').trim()
+ const { packageName, markdown, repoInfo } = await resolvePackageReadmeSource(packagePath)📝 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 packagePath = getRouterParam(event, 'pkg') ?? '' | |
| const { packageName, markdown, repoInfo } = await resolvePackageReadmeSource(packagePath) | |
| const packagePath = (getRouterParam(event, 'pkg') ?? '').replace(/\/+$/, '').trim() | |
| const { packageName, markdown, repoInfo } = await resolvePackageReadmeSource(packagePath) |
| const packagePath = getRouterParam(event, 'pkg') ?? '' | ||
| return await resolvePackageReadmeSource(packagePath) |
There was a problem hiding this comment.
Normalise packagePath before resolving.
Trailing slashes or whitespace can create duplicate cache entries and may lead to validation failures. Normalise once before calling the resolver.
Proposed fix
- const packagePath = getRouterParam(event, 'pkg') ?? ''
- return await resolvePackageReadmeSource(packagePath)
+ const packagePath = (getRouterParam(event, 'pkg') ?? '').replace(/\/+$/, '').trim()
+ return await resolvePackageReadmeSource(packagePath)| // Mock Nitro globals before importing the module | ||
| vi.stubGlobal('defineCachedFunction', (fn: Function) => fn) | ||
| const $fetchMock = vi.fn() | ||
| vi.stubGlobal('$fetch', $fetchMock) | ||
| vi.stubGlobal('parsePackageParams', parsePackageParams) | ||
|
|
||
| const fetchNpmPackageMock = vi.fn() | ||
| vi.stubGlobal('fetchNpmPackage', fetchNpmPackageMock) | ||
|
|
||
| const parseRepositoryInfoMock = vi.fn() | ||
| vi.stubGlobal('parseRepositoryInfo', parseRepositoryInfoMock) | ||
|
|
||
| const { fetchReadmeFromJsdelivr, isStandardReadme, resolvePackageReadmeSource } = | ||
| await import('../../../../server/utils/readme-loaders') |
There was a problem hiding this comment.
Avoid the broad Function type in the stub.
Use a generic function type to keep the stub type-safe.
Proposed fix
-vi.stubGlobal('defineCachedFunction', (fn: Function) => fn)
+const defineCachedFunctionMock = <T extends (...args: unknown[]) => unknown>(fn: T) => fn
+vi.stubGlobal('defineCachedFunction', defineCachedFunctionMock)As per coding guidelines Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index.
📝 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.
| // Mock Nitro globals before importing the module | |
| vi.stubGlobal('defineCachedFunction', (fn: Function) => fn) | |
| const $fetchMock = vi.fn() | |
| vi.stubGlobal('$fetch', $fetchMock) | |
| vi.stubGlobal('parsePackageParams', parsePackageParams) | |
| const fetchNpmPackageMock = vi.fn() | |
| vi.stubGlobal('fetchNpmPackage', fetchNpmPackageMock) | |
| const parseRepositoryInfoMock = vi.fn() | |
| vi.stubGlobal('parseRepositoryInfo', parseRepositoryInfoMock) | |
| const { fetchReadmeFromJsdelivr, isStandardReadme, resolvePackageReadmeSource } = | |
| await import('../../../../server/utils/readme-loaders') | |
| // Mock Nitro globals before importing the module | |
| const defineCachedFunctionMock = <T extends (...args: unknown[]) => unknown>(fn: T) => fn | |
| vi.stubGlobal('defineCachedFunction', defineCachedFunctionMock) | |
| const $fetchMock = vi.fn() | |
| vi.stubGlobal('$fetch', $fetchMock) | |
| vi.stubGlobal('parsePackageParams', parsePackageParams) | |
| const fetchNpmPackageMock = vi.fn() | |
| vi.stubGlobal('fetchNpmPackage', fetchNpmPackageMock) | |
| const parseRepositoryInfoMock = vi.fn() | |
| vi.stubGlobal('parseRepositoryInfo', parseRepositoryInfoMock) | |
| const { fetchReadmeFromJsdelivr, isStandardReadme, resolvePackageReadmeSource } = | |
| await import('../../../../server/utils/readme-loaders') |
2 participants
Currently, we transfer all Markdown content in the main request, which increases the response size by 1.5-2 times and adds unnecessary weight to the finished pages.
I've refactored this logic so that Markdown is loaded separately and cached. The main route and Markdown-source route then work with this loader. Because the user has just loaded the page, the markdown cache will already be ready. The main route simply returns mdExists to reduce the request and avoid duplicate entries in the cache.
I also made it so that loading starts on hover to make the behavior even less noticeable for users