feat: [performance improvement] Parallelize static param generation#250
feat: [performance improvement] Parallelize static param generation#250anyulled wants to merge 1 commit into
Conversation
Refactor generateStaticParams in speakers, tags, and talks routes to fetch parameters concurrently using Promise.all instead of sequential for...of loops. 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? |
📝 WalkthroughWalkthroughThree Next.js dynamic routes—speakers, tags, and talks—refactor their ChangesStatic Route Parameter Generation Parallelization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 refactors the sequential fetching logic in generateStaticParams across the speakers, tags, and talks pages to use Promise.all for parallel execution, which improves build performance. The reviewer feedback suggests utilizing strict mode (e.g., { strict: true }) for the data fetching functions (getSpeakers and getTalks) and allowing errors to propagate to fail the build rather than catching them and returning empty arrays, preventing the deployment of incomplete pages. Additionally, the documentation in .jules/bolt.md should be updated to reflect this strict mode recommendation.
| const paramsArrays = await Promise.all( | ||
| years.map(async (year) => { | ||
| try { | ||
| const speakers = await getSpeakers(year); | ||
| return speakers.map((speaker) => ({ year, speakerId: speaker.id })); | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch speakers for year ${year}:`, error); | ||
| return []; | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch speakers for year ${year}:`, error); | ||
| } | ||
| } | ||
|
|
||
| return params; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
To prevent the deployment of incomplete pages, data fetching during the build process should use the strict mode of getSpeakers (i.e., { strict: true }) and allow the error to propagate. This ensures that any data fetching failure causes the build to fail rather than silently returning an empty array.
const paramsArrays = 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 paramsArrays = await Promise.all( | ||
| years.map(async (year) => { | ||
| 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)); | ||
| } | ||
|
|
||
| return Array.from(allTags).map((tag) => ({ year, tag: tag.replaceAll(" ", "-").toLowerCase() })); | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch talks for year ${year}:`, error); | ||
| return []; | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch talks for year ${year}:`, error); | ||
| } | ||
| } | ||
|
|
||
| return params; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
To prevent the deployment of incomplete pages, data fetching during the build process should use the strict mode of getTalks (i.e., { strict: true }) and allow the error to propagate. This ensures that any data fetching failure causes the build to fail rather than silently returning an empty array.
const paramsArrays = 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 paramsArrays = await Promise.all( | ||
| years.map(async (year) => { | ||
| try { | ||
| const sessionGroups = await getTalks(year); | ||
| const allTalks = sessionGroups.flatMap((group) => group.sessions); | ||
| return allTalks.map((talk) => ({ year, talkId: talk.id })); | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch talks for year ${year}:`, error); | ||
| return []; | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to fetch talks for year ${year}:`, error); | ||
| } | ||
| } | ||
|
|
||
| return params; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
To prevent the deployment of incomplete pages, data fetching during the build process should use the strict mode of getTalks (i.e., { strict: true }) and allow the error to propagate. This ensures that any data fetching failure causes the build to fail rather than silently returning an empty array.
const paramsArrays = 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-18 - Use Promise.all to parallelize generateStaticParams fetching | ||
|
|
||
| **Learning:** Next.js build performance is highly sensitive to synchronous I/O operations inside `generateStaticParams`. A sequential `for...of` loop over multiple archived editions for fetching talks, speakers, or tags can significantly slow down static generation and sitemap rendering times. | ||
| **Action:** Always refactor sequential iteration logic over external/database queries in build steps to parallel promises using `Promise.all(array.map(async (item) => { ... }))`. Make sure to retain per-item internal `try/catch` fallbacks to avoid premature build crashes on a single missing parameter item. Ensure to `.flat()` the arrays before returning. |
There was a problem hiding this comment.
Update the action guideline to recommend using strict mode (e.g., { strict: true }) and propagating errors to fail the build on data fetching failures, in alignment with the repository's general rules.
| **Action:** Always refactor sequential iteration logic over external/database queries in build steps to parallel promises using `Promise.all(array.map(async (item) => { ... }))`. Make sure to retain per-item internal `try/catch` fallbacks to avoid premature build crashes on a single missing parameter item. Ensure to `.flat()` the arrays before returning. | |
| **Action:** Always refactor sequential iteration logic over external/database queries in build steps to parallel promises using `Promise.all(array.map(async (item) => { ... }))`. Ensure data fetching functions use strict mode (e.g., `{ strict: true }`) and propagate errors to fail the build on failure, preventing the deployment of incomplete pages. Ensure to `.flat()` the arrays before returning. |
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.
![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
🤖 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 `@app/`[year]/speakers/[speakerId]/page.tsx:
- Line 28: The catch clause currently declares an untyped error variable; change
the catch parameter to be explicitly typed as unknown (catch (error: unknown))
in the page component's try/catch block in
app/[year]/speakers/[speakerId]/page.tsx, and if the code uses the error value
later, narrow it before use (e.g., guard with typeof or instanceOf checks or
extract a message with a helper that handles unknown) to satisfy strict
TypeScript rules and avoid any usage of any.
In `@app/`[year]/tags/[tag]/page.tsx:
- Line 33: Update the catch clause to explicitly type the exception variable as
unknown (change "catch (error)" to "catch (error: unknown)"), then narrow the
type before usage (e.g., check instanceof Error or typeof) wherever "error" is
used inside the catch so no implicit any remains; target the catch block in
page.tsx where "error" is currently untyped.
In `@app/`[year]/talks/[talkId]/page.tsx:
- Line 38: The catch clause currently uses an untyped identifier "error" in the
try/catch block (catch (error)); change it to explicitly type the variable as
unknown (catch (error: unknown)) and, if the code reads properties from error
later, narrow or assert its type safely (e.g., using isError checks or
extracting message with: const msg = error instanceof Error ? error.message :
String(error)) to satisfy strict TypeScript rules and avoid any usage of any.
🪄 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: 155f0bcc-1e67-4922-9ee6-0845a313dc14
📒 Files selected for processing (4)
.jules/bolt.mdapp/[year]/speakers/[speakerId]/page.tsxapp/[year]/tags/[tag]/page.tsxapp/[year]/talks/[talkId]/page.tsx
| try { | ||
| const speakers = await getSpeakers(year); | ||
| return speakers.map((speaker) => ({ year, speakerId: speaker.id })); | ||
| } catch (error) { |
There was a problem hiding this comment.
Type the catch clause variable explicitly.
The error parameter should be explicitly typed as unknown for strict TypeScript compliance. As per coding guidelines, avoid any at all costs and use strict TypeScript.
🔧 Suggested fix
- } catch (error) {
+ } catch (error: unknown) {
console.warn(`Failed to fetch speakers for year ${year}:`, error);📝 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.
| } catch (error) { | |
| } catch (error: unknown) { | |
| console.warn(`Failed to fetch speakers for year ${year}:`, error); |
🤖 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]/speakers/[speakerId]/page.tsx at line 28, The catch clause
currently declares an untyped error variable; change the catch parameter to be
explicitly typed as unknown (catch (error: unknown)) in the page component's
try/catch block in app/[year]/speakers/[speakerId]/page.tsx, and if the code
uses the error value later, narrow it before use (e.g., guard with typeof or
instanceOf checks or extract a message with a helper that handles unknown) to
satisfy strict TypeScript rules and avoid any usage of any.
| } | ||
|
|
||
| return Array.from(allTags).map((tag) => ({ year, tag: tag.replaceAll(" ", "-").toLowerCase() })); | ||
| } catch (error) { |
There was a problem hiding this comment.
Type the catch clause variable explicitly.
The error parameter should be explicitly typed as unknown for strict TypeScript compliance. As per coding guidelines, avoid any at all costs and use strict TypeScript.
🔧 Suggested fix
- } catch (error) {
+ } catch (error: unknown) {
console.warn(`Failed to fetch talks for year ${year}:`, error);📝 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.
| } catch (error) { | |
| } catch (error: unknown) { | |
| console.warn(`Failed to fetch talks for year ${year}:`, error); | |
| } |
🤖 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]/tags/[tag]/page.tsx at line 33, Update the catch clause to
explicitly type the exception variable as unknown (change "catch (error)" to
"catch (error: unknown)"), then narrow the type before usage (e.g., check
instanceof Error or typeof) wherever "error" is used inside the catch so no
implicit any remains; target the catch block in page.tsx where "error" is
currently untyped.
| const sessionGroups = await getTalks(year); | ||
| const allTalks = sessionGroups.flatMap((group) => group.sessions); | ||
| return allTalks.map((talk) => ({ year, talkId: talk.id })); | ||
| } catch (error) { |
There was a problem hiding this comment.
Type the catch clause variable explicitly.
The error parameter should be explicitly typed as unknown for strict TypeScript compliance. As per coding guidelines, avoid any at all costs and use strict TypeScript.
🔧 Suggested fix
- } catch (error) {
+ } catch (error: unknown) {
console.warn(`Failed to fetch talks for year ${year}:`, error);🤖 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]/talks/[talkId]/page.tsx at line 38, The catch clause currently
uses an untyped identifier "error" in the try/catch block (catch (error));
change it to explicitly type the variable as unknown (catch (error: unknown))
and, if the code reads properties from error later, narrow or assert its type
safely (e.g., using isError checks or extracting message with: const msg = error
instanceof Error ? error.message : String(error)) to satisfy strict TypeScript
rules and avoid any usage of any.
1 participant
💡 What: Refactored
generateStaticParamsinapp/[year]/talks/[talkId]/page.tsx,app/[year]/speakers/[speakerId]/page.tsx, andapp/[year]/tags/[tag]/page.tsx. Replaced sequentialfor...ofloops with parallel execution usingPromise.allandArray.map.🎯 Why: Next.js evaluates
generateStaticParamsduring the build process. Sequentially awaiting data (such as fetching talks or speakers for multiple archived editions) artificially inflates build times by blocking on sequential network/database I/O.📊 Impact: Reduces static site generation times and build latency linearly based on the number of editions being generated.
🔬 Measurement: Verify by running
npm run buildand checking the "Generating static pages" stage timing. Tested successfully withnpm run buildlocally showing a slight reduction in generation latency.PR created automatically by Jules for task 359695072073289939 started by @anyulled
Summary by CodeRabbit
Performance
Documentation