⚡ Bolt: optimize pdf export to render sheets concurrently#303
⚡ Bolt: optimize pdf export to render sheets concurrently#303guitarbeat wants to merge 1 commit into
Conversation
|
👋 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. |
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? |
|
Deployment failed with the following error: Learn More: https://vercel.com/guitarbeats-projects?upgradeToPro=build-rate-limit |
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @guitarbeat, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![pullpal-ai[bot]](https://avatars.githubusercontent.com/in/1237299?s=60&v=4){kind=small}
|
|
||
| for (let sheetIndex = 0; sheetIndex < sheetCount; sheetIndex++) { | ||
| // ⚡ Bolt: Process sheet canvases concurrently to improve export speed | ||
| const sheetPromises = Array.from({ length: sheetCount }, async (_, sheetIndex) => { |
There was a problem hiding this comment.
Great use of Promise.all for concurrent canvas processing! However, the use of Array.from({ length: sheetCount }, async ...) is a bit misleading: the second argument to Array.from is not an async callback, so this will create an array of Promises, but the callback does not receive the index as the first argument. Consider using Array.from({ length: sheetCount }).map((_, sheetIndex) => async ...) for clarity and correctness.
| const pageIndex = (sheetIndex * slotsPerSheet) + (pageNum - 1); | ||
| const url = this.state.allPageImages[pageIndex]; | ||
| if (!url) continue; | ||
| if (!url) { continue; } |
There was a problem hiding this comment.
The inner if (!url) { continue; } inside the async function will not skip the current iteration in the context of Promise.all; it will just return undefined for that sheet. This could result in an array of undefineds in sheetImages, which may cause doc.addImage to fail or insert blank pages. Consider returning a placeholder image or filtering out undefineds, and ensure the mapping between sheet index and images remains correct.
There was a problem hiding this comment.
If the number of sheets is very large, running all canvas renderings in parallel may cause high memory usage and potential browser tab crashes. Consider adding a concurrency limit (e.g., using a sliding window pool pattern as described in your documentation) if you anticipate very large exports.
There was a problem hiding this comment.
Good update to the documentation to reflect the new concurrency approach. However, consider keeping previous learnings (such as the sliding window pattern and memory management) in the doc for future reference, rather than deleting them. They may still be relevant for other parts of the codebase or future optimizations.
💡 What: Refactored the
handleExportloop inExportService.jsto process sheet canvas generation concurrently usingPromise.all()over an array of promises, before sequentially adding the resulting image data to thejsPDFdocument. Also documented learning in.jules/bolt.md.🎯 Why: Previously, rendering each sheet's underlying canvas (filling background, clipping, rotating, placing the page image, and extracting to data URL) occurred sequentially in a
forloop. This created a performance bottleneck during the export of documents, especially large ones. Concurrent rendering allows the browser to utilize its resources more effectively without breaking jsPDF's strict sequential page insertion order.📊 Measured Improvement:
performance.now()instrumentation.PR created automatically by Jules for task 14714827244503765498 started by @guitarbeat