feat: [performance improvement]#251
Conversation
Replaced sequential `for...of` loops with `Promise.all(years.map(...))` in `generateStaticParams` across dynamic routes to improve Next.js build performance, properly un-nesting arrays using `.flat()` while preserving `try/catch` fallbacks. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 55 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughRefactored Next.js static parameter generation across four dynamic routes from sequential year iteration to concurrent ChangesNext.js Static Generation Parallelization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 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 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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes Next.js static generation builds by replacing sequential loops with parallel data fetching using Promise.all in generateStaticParams across multiple routes, and documents this practice in .jules/bolt.md. The review feedback points out that the added try/catch blocks are redundant because the data fetching functions do not throw unless strict mode is enabled. It recommends enabling strict mode to ensure build failures on data fetching errors, removing the try/catch blocks, simplifying the array mapping using .map(), and updating the documentation to reflect this strict mode requirement.
| const nestedParams = await Promise.all( | ||
| years.map(async (year) => { | ||
| const params: { year: string; speakerId: string }[] = []; | ||
| try { | ||
| const speakers = await getSpeakers(year); | ||
| for (const speaker of speakers) { | ||
| params.push({ year, speakerId: speaker.id }); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch speakers for year ${year}:`, error); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch speakers for year ${year}:`, error); | ||
| } | ||
| } | ||
| return params; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
The try/catch block is currently dead code because getSpeakers(year) does not throw errors unless { strict: true } is passed. Furthermore, according to the repository's general rules, data fetching functions used during the build process should run in strict mode to ensure that failures cause the build to fail, preventing the deployment of incomplete pages.
We should pass { strict: true } to getSpeakers and let the error propagate (removing the try/catch block). We can also simplify the array mapping using .map() instead of a manual loop and .push().
const nestedParams = await Promise.all(
years.map(async (year) => {
const speakers = await getSpeakers(year, { strict: true });
return speakers.map((speaker) => ({
year,
speakerId: speaker.id,
}));
})
);
References
- Data fetching functions used during the build process (e.g., for sitemap generation) should include a 'strict' mode that throws an error on failure. This ensures that data fetching problems cause the build to fail, preventing the deployment of incomplete pages.
| const nestedParams = await Promise.all( | ||
| years.map(async (year) => { | ||
| const params: { year: string; tag: string }[] = []; | ||
| try { | ||
| const sessionGroups = await getTalks(year); | ||
| const allTalks = sessionGroups.flatMap((group) => group.sessions); | ||
| const allTags = new Set<string>(); | ||
|
|
||
| for (const talk of allTalks) { | ||
| getTagsFromTalk(talk).forEach((tag) => allTags.add(tag)); | ||
| } | ||
|
|
||
| for (const tag of allTags) { | ||
| params.push({ year, tag: tag.replaceAll(" ", "-").toLowerCase() }); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch talks for year ${year}:`, error); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch talks for year ${year}:`, error); | ||
| } | ||
| } | ||
| return params; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
The try/catch block is currently dead code because getTalks(year) does not throw errors unless { strict: true } is passed. Furthermore, according to the repository's general rules, data fetching functions used during the build process should run in strict mode to ensure that failures cause the build to fail, preventing the deployment of incomplete pages.
We should pass { strict: true } to getTalks and let the error propagate (removing the try/catch block). We can also simplify the array mapping using .map() instead of a manual loop and .push().
const nestedParams = await Promise.all(
years.map(async (year) => {
const sessionGroups = await getTalks(year, { strict: true });
const allTalks = sessionGroups.flatMap((group) => group.sessions);
const allTags = new Set<string>();
for (const talk of allTalks) {
getTagsFromTalk(talk).forEach((tag) => allTags.add(tag));
}
return Array.from(allTags).map((tag) => ({
year,
tag: tag.replaceAll(" ", "-").toLowerCase(),
}));
})
);
References
- Data fetching functions used during the build process (e.g., for sitemap generation) should include a 'strict' mode that throws an error on failure. This ensures that data fetching problems cause the build to fail, preventing the deployment of incomplete pages.
| const nestedParams = await Promise.all( | ||
| years.map(async (year) => { | ||
| const params: { year: string; talkId: string }[] = []; | ||
| try { | ||
| const sessionGroups = await getTalks(year); | ||
| const allTalks = sessionGroups.flatMap((group) => group.sessions); | ||
| for (const talk of allTalks) { | ||
| params.push({ year, talkId: talk.id }); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch talks for year ${year}:`, error); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch talks for year ${year}:`, error); | ||
| } | ||
| } | ||
| return params; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
The try/catch block is currently dead code because getTalks(year) does not throw errors unless { strict: true } is passed. Furthermore, according to the repository's general rules, data fetching functions used during the build process should run in strict mode to ensure that failures cause the build to fail, preventing the deployment of incomplete pages.
We should pass { strict: true } to getTalks and let the error propagate (removing the try/catch block). We can also simplify the array mapping using .map() instead of a manual loop and .push().
const nestedParams = await Promise.all(
years.map(async (year) => {
const sessionGroups = await getTalks(year, { strict: true });
const allTalks = sessionGroups.flatMap((group) => group.sessions);
return allTalks.map((talk) => ({
year,
talkId: talk.id,
}));
})
);
References
- Data fetching functions used during the build process (e.g., for sitemap generation) should include a 'strict' mode that throws an error on failure. This ensures that data fetching problems cause the build to fail, preventing the deployment of incomplete pages.
|
|
||
| ## 2024-05-31 - Optimize Next.js static generation builds with parallel data fetching | ||
| **Learning:** Sequential `for...of` loops used for fetching dynamic page dependencies inside Next.js build-time functions like `generateStaticParams` (or `sitemap.ts`) block the thread and significantly increase total site build duration. | ||
| **Action:** When gathering parameters for static routes across large array sets (such as mapping years or categories), replace sequential `for...of` iteration with parallel execution using `Promise.all(array.map(...))`. Then, correctly aggregate nested outputs utilizing `.flat()` or `.flatMap()`, and preserve per-item `try/catch` fallbacks to prevent a single item failure from crashing the entire batch build. |
There was a problem hiding this comment.
The recommendation to 'preserve per-item try/catch fallbacks to prevent a single item failure from crashing the entire batch build' contradicts the repository's general rule. We want data fetching failures during the build process to fail the build so that we do not deploy incomplete pages. Let's update this documentation to reflect the correct practice.
| **Action:** When gathering parameters for static routes across large array sets (such as mapping years or categories), replace sequential `for...of` iteration with parallel execution using `Promise.all(array.map(...))`. Then, correctly aggregate nested outputs utilizing `.flat()` or `.flatMap()`, and preserve per-item `try/catch` fallbacks to prevent a single item failure from crashing the entire batch build. | |
| **Action:** When gathering parameters for static routes across large array sets (such as mapping years or categories), replace sequential `for...of` iteration with parallel execution using `Promise.all(array.map(...))`. Then, correctly aggregate nested outputs utilizing `.flat()` or `.flatMap()`, and ensure data fetching functions run in strict mode without swallowing errors to let failures correctly fail the build and prevent deploying incomplete pages. |
References
- Data fetching functions used during the build process (e.g., for sitemap generation) should include a 'strict' mode that throws an error on failure. This ensures that data fetching problems cause the build to fail, preventing the deployment of incomplete pages.
| const nestedParams = await Promise.all( | ||
| years.map(async (year) => { | ||
| const params: { year: string; companyName: string }[] = []; | ||
| const companies = await getJobOffersForEdition(year); | ||
| for (const company of companies) { | ||
| params.push({ | ||
| year, | ||
| companyName: company.id, | ||
| }); | ||
| } | ||
| return params; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Simplify the nested mapping using .map() instead of initializing an empty array and manually pushing to it. Additionally, to fully align with the repository's general rules, getJobOffersForEdition should ideally be updated to support a strict mode that throws on failure, though that can be addressed in a separate PR since public-queries.ts is not modified here.
const nestedParams = await Promise.all(
years.map(async (year) => {
const companies = await getJobOffersForEdition(year);
return companies.map((company) => ({
year,
companyName: company.id,
}));
})
);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/[year]/job-offers/[companyName]/page.tsx (1)
24-38: ⚡ Quick winAdd per-year error isolation in
Promise.allmapping.A single unexpected rejection currently aborts all static params generation. Catch per year and return
[]to keep the build resilient.Proposed patch
const nestedParams = await Promise.all( years.map(async (year) => { const params: { year: string; companyName: string }[] = []; - const companies = await getJobOffersForEdition(year); - for (const company of companies) { - params.push({ - year, - companyName: company.id, - }); + try { + const companies = await getJobOffersForEdition(year); + for (const company of companies) { + params.push({ + year, + companyName: company.id, + }); + } + } catch (error) { + console.warn(`Failed to fetch job offers for year ${year}:`, error); } return params; }) );🤖 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 `@app/`[year]/job-offers/[companyName]/page.tsx around lines 24 - 38, The Promise.all over years currently lets any rejection abort the whole static params generation; wrap the per-year async mapper used in years.map (the function that calls getJobOffersForEdition and builds params, assigned to nestedParams) in a try/catch so that errors for a single year are caught and the mapper returns an empty array [] on failure; keep logging or process the error inside the catch for visibility but ensure the mapper never throws so nestedParams.flat() completes resiliently.
🤖 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.
Nitpick comments:
In `@app/`[year]/job-offers/[companyName]/page.tsx:
- Around line 24-38: The Promise.all over years currently lets any rejection
abort the whole static params generation; wrap the per-year async mapper used in
years.map (the function that calls getJobOffersForEdition and builds params,
assigned to nestedParams) in a try/catch so that errors for a single year are
caught and the mapper returns an empty array [] on failure; keep logging or
process the error inside the catch for visibility but ensure the mapper never
throws so nestedParams.flat() completes resiliently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53276fbf-81a1-4e11-a2f0-970d12528306
📒 Files selected for processing (5)
.jules/bolt.mdapp/[year]/job-offers/[companyName]/page.tsxapp/[year]/speakers/[speakerId]/page.tsxapp/[year]/tags/[tag]/page.tsxapp/[year]/talks/[talkId]/page.tsx
Replaced sequential `for...of` loops with `Promise.all(years.map(...))` in `generateStaticParams` across dynamic routes to improve Next.js build performance, properly un-nesting arrays using `.flat()` while preserving `try/catch` fallbacks. Fixed Prettier formatting on `.jules/bolt.md` to pass CI. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
1 participant
💡 What: Replaced the sequential
for...ofloops used for gathering dynamic data ingenerateStaticParamswith parallel concurrent fetches usingPromise.all(years.map(...))and.flat()acrossapp/[year]/job-offers/[companyName]/page.tsx,app/[year]/speakers/[speakerId]/page.tsx,app/[year]/tags/[tag]/page.tsx, andapp/[year]/talks/[talkId]/page.tsx.🎯 Why: Sequential data fetching inside Next.js build-time functions severely bottlenecked build generation, blocking the thread while waiting for individual years' HTTP requests to resolve one by one.
📊 Impact: Significantly reduces total Next.js static build execution time and duration by parallelizing the collection of parameters necessary to pre-render dynamic routes across all available edition years.
🔬 Measurement: Build completion duration can be accurately measured locally or via CI comparing
npm run buildtotal duration prior to and following this branch integration.PR created automatically by Jules for task 4612543585120995632 started by @anyulled
Summary by CodeRabbit
Performance
Documentation