chore: refactor starters fetching to use dynamic source from starter repo - #492
chore: refactor starters fetching to use dynamic source from starter repo#492maximepvrt wants to merge 6 commits into
starter repo#492Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
@maximepvrt is attempting to deploy a commit to the Nuxt Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR replaces static starter metadata with runtime GitHub discovery and filtering, adds a JSON Schema and generated Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/components/StarterCard.vueESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. package.jsonESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. scripts/generate-types.mjsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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: 3
🤖 Fix all issues with AI agents
In `@server/routes/data/starters.json.ts`:
- Line 1: The handler exported from defineEventHandler declares an unused
parameter `event` and uses untyped fetch results causing implicit any for
`files` and `file`; rename the parameter to `_event` (or remove it), call $fetch
with a type parameter to declare the expected response shape (e.g.,
$fetch<YourResponseType>(...)) so `files` is strongly typed, and update the .map
callback to use a typed `file` parameter (e.g., (file: FileType) => ...)
matching that response type; adjust any related type aliases/interfaces used by
the handler to match the fetched data structure.
- Line 15: The current templates.push call sets slug: templateName before
spreading ...template which allows a slug from the fetched template object to
overwrite the filename-derived templateName; update the object literal in the
templates.push expression so that the spread comes first and then slug:
templateName appears after (e.g., change to { ...template, slug: templateName,
tar: ... }) to ensure the filename-derived slug wins; locate the
templates.push(...) expression and reorder the properties accordingly
(referencing templateName, template, and the Starter-shaped fetched JSON).
- Around line 6-17: Reformat the callback passed to Promise.all(files.map(...))
to use the project's 2-space indentation (instead of the current 6/8-space),
adjusting the entire block beginning with "async (file) => {" through the
closing "}))" so each nested line (checks for file.download_url/type/name, const
templateName, const template fetch, and the templates.push) is indented by 2
spaces per level; verify the async callback body, the if blocks, and the object
literal for templates.push maintain consistent 2-space indentation so the linter
stops failing.
🧹 Nitpick comments (2)
shared/types/index.ts (1)
8-9: Nit: Inconsistent separators in the interface.Lines 8–9 use trailing commas while every other property uses no separator. Consider removing them for consistency.
✏️ Suggested fix
- repo: string, - branch: string, + repo: string + branch: stringserver/routes/data/starters.json.ts (1)
4-4: Unauthenticated GitHub API calls are subject to strict rate limits.The GitHub Contents API allows only 60 requests/hour for unauthenticated callers. This handler makes 1 + N requests (directory listing + one per template file) on every cache miss. With ISR at 3600s that's likely fine under normal conditions, but any cache purge, deployment, or cold-start burst could exhaust the quota quickly.
Consider either:
- Using a
GITHUB_TOKENvia a server-side env variable to raise the limit to 5,000 req/hour.- Fetching the entire directory tree in a single call (e.g., Git Trees API with
?recursive=1) to reduce request count.
| export default defineEventHandler(async () => { | ||
| const templates: Starter[] = [] | ||
|
|
||
| const files = await $fetch('https://api.github.com/repos/maximepvrt/nuxt-starter/contents/templates?ref=update-template') |
There was a problem hiding this comment.
use https://api.github.com/repos/nuxt/starter/contents/templates?ref=templates after merge nuxt/starter#1774
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/routes/data/starters.json.ts`:
- Around line 1-24: Wrap the external GitHub calls inside robust error handling:
replace the Promise.all over files with Promise.allSettled and for each entry in
the map around the inner $fetch (the call that produces template in the
defineEventHandler) add a try/catch so one failed template is skipped and logged
(push only successful, non-deprecated templates into the templates array); also
wrap the initial $fetch that retrieves files in its own try/catch and return an
empty templates array or a safe fallback on failure; finally, when calling
$fetch to GitHub include an Authorization header using a server-side
GITHUB_TOKEN env var to increase rate limits.
- Line 4: The $fetch call that assigns files is pointing at a hardcoded fork and
feature branch; update the URL passed to $fetch (the string literal) to the
canonical upstream repository and branch (e.g., replace
'https://api.github.com/repos/maximepvrt/nuxt-starter/contents/templates?ref=update-template'
with the correct upstream repo and branch such as
'https://api.github.com/repos/nuxt/starter/contents/templates?ref=main'), or
better yet make the repo and ref configurable via an environment variable and
use that in the $fetch call so the files variable always targets the canonical
repo/branch in production.
🧹 Nitpick comments (1)
server/routes/data/starters.json.ts (1)
11-13: Unsafeas Startercast on unvalidated external data.The fetched JSON is cast to
Starterwithout any runtime validation. If the remote JSON schema changes or a file is malformed, the code will silently produce incorrectStarterobjects (e.g., missingrepoorbranchwould yield a brokentarURL on Line 15).Consider adding minimal runtime validation (e.g., check that
template.repoandtemplate.branchare truthy strings) before pushing.
| export default defineEventHandler(async () => { | ||
| const templates: Starter[] = [] | ||
|
|
||
| const files = await $fetch('https://api.github.com/repos/maximepvrt/nuxt-starter/contents/templates?ref=update-template') | ||
|
|
||
| await Promise.all(files.map(async (file) => { | ||
| if (!file.download_url || file.type !== 'file' || !file.name.endsWith('.json')) { | ||
| return | ||
| } | ||
| const templateName = file.name.replace('.json', '') | ||
| const template = await $fetch(file.download_url, { | ||
| responseType: 'json', | ||
| }) as Starter | ||
| if (!template.deprecated) { | ||
| templates.push({ slug: templateName, ...template, tar: `https://codeload.github.com/${template.repo}/tar.gz/refs/heads/${template.branch}` }) | ||
| } | ||
| })) | ||
|
|
||
| return templates.sort((a, b) => { | ||
| if (a.default && !b.default) return -1 | ||
| if (!a.default && b.default) return 1 | ||
| return 0 | ||
| }) | ||
| }) |
There was a problem hiding this comment.
No error handling for external GitHub API calls.
Both $fetch calls (directory listing on Line 4, individual template files on Line 11) can fail due to rate-limiting (unauthenticated GitHub API allows 60 req/hr), network errors, or malformed responses. A single failure inside Promise.all will reject the entire response with an unhandled error.
Consider:
- Adding a
try/catcharound each inner fetch so one bad template doesn't take down the whole endpoint. - Using
Promise.allSettledinstead ofPromise.all. - Authenticating the GitHub API request (e.g., via a server token) to raise the rate limit, especially given ISR revalidation every hour.
🧰 Tools
🪛 GitHub Check: test
[failure] 15-15:
'slug' is specified more than once, so this usage will be overwritten.
[failure] 6-6:
Parameter 'file' implicitly has an 'any' type.
[failure] 6-6:
'files' is of type 'unknown'.
[failure] 15-15:
'slug' is specified more than once, so this usage will be overwritten.
[failure] 6-6:
Parameter 'file' implicitly has an 'any' type.
[failure] 6-6:
'files' is of type 'unknown'.
🤖 Prompt for AI Agents
In `@server/routes/data/starters.json.ts` around lines 1 - 24, Wrap the external
GitHub calls inside robust error handling: replace the Promise.all over files
with Promise.allSettled and for each entry in the map around the inner $fetch
(the call that produces template in the defineEventHandler) add a try/catch so
one failed template is skipped and logged (push only successful, non-deprecated
templates into the templates array); also wrap the initial $fetch that retrieves
files in its own try/catch and return an empty templates array or a safe
fallback on failure; finally, when calling $fetch to GitHub include an
Authorization header using a server-side GITHUB_TOKEN env var to increase rate
limits.
starters repostarter repo
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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 `@server/routes/data/starters.json.ts`:
- Around line 10-14: Update the concurrent fetch logic in the templates
Promise.all flow to handle individual $fetch failures without rejecting the
entire route. Use Promise.allSettled or per-request try/catch, discard failed
template results, and continue rendering all successfully fetched Starter
entries.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc3c5b0d-3581-457b-b7f8-90d065681fd2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
app/components/StarterCard.vuepackage.jsonscripts/generate-types.mjsserver/api/lookup/[slug].get.tsserver/routes/data/starters.json.tsshared/types/index.tstemplate.schema.json
| const templates = await Promise.all(files.map(file => | ||
| $fetch<Starter>(`https://raw.githubusercontent.com/nuxt/starter/templates/${file.path}`, { | ||
| responseType: 'json', | ||
| }), | ||
| )) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing error handling for concurrent external API calls.
Using Promise.all means that if a single $fetch fails (e.g., due to GitHub's rate limits or a network blip), the entire promise rejects, and the route will fail for all templates. This was previously flagged and remains a risk.
Consider using Promise.allSettled or wrapping the inner $fetch in a try/catch block to skip failed items gracefully while successfully rendering the remaining templates.
🛠️ Proposed fix to handle individual failures
- const templates = await Promise.all(files.map(file =>
- $fetch<Starter>(`https://raw.githubusercontent.com/nuxt/starter/templates/${file.path}`, {
- responseType: 'json',
- }),
- ))
+ const templatesResult = await Promise.allSettled(files.map(file =>
+ $fetch<Starter>(`https://raw.githubusercontent.com/nuxt/starter/templates/${file.path}`, {
+ responseType: 'json',
+ })
+ ))
+
+ const templates = templatesResult
+ .filter((result): result is PromiseFulfilledResult<Starter> => result.status === 'fulfilled')
+ .map(result => result.value)🤖 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 `@server/routes/data/starters.json.ts` around lines 10 - 14, Update the
concurrent fetch logic in the templates Promise.all flow to handle individual
$fetch failures without rejecting the entire route. Use Promise.allSettled or
per-request try/catch, discard failed template results, and continue rendering
all successfully fetched Starter entries.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
❓ Type of change
📚 Description
I would like to propose replacing the static
starters.jsonfile in this repository with a dynamic version fetched directly from thenuxt/starterrepository, similar to how the Nuxt CLI operates.The current static approach leads to synchronization issues. I noticed this when @benjamincanac updated the UI templates:
npm create nuxt@latest -- -t uicorrectly uses the updated template.nuxt.newstill opens an outdated repository because the localstarters.jsonwas not updated.To facilitate this transition, I have already modified the repository definitions within
nuxt/starterand add properties to this repo with this PR : nuxt/starter#1774.Questions & Discussion Points
1. Template Recursion & Subdirectories
I noticed that subdirectories in the templates branch (e.g.,
uiandui-vue) are not currently integrated into the CLI because there is no recursion logic.Currently, @benjamincanac uses the following command for UI templates in the README:
npm create nuxt@latest -- -t github:nuxt-ui-templates/editor2. Centralizing the Source of Truth
To avoid maintaining starter lists in two places, should we move the
starters.jsondefinition to thenuxt.comrepository? This would allow both the CLI and the nuxt.new to pull from a single source.3. UX Consolidation
I am also questioning the placement of the starter cards. Would it make more sense to migrate/repatriate the
nuxt.newstarter cards to the top of the nuxt.com/templates page for a more unified user experience?