Repository navigation
Faster PDF report: one image fetch, a Web Worker, JPEG and a satellite cache - #65
Conversation
…e cache The report was slow, expensive in Google Static Maps calls and froze the browser. On the reference report (50 farms x 3 layers, production build): preview 34.3 s -> 8.7 s, "Download" 43.7 s -> 0.04 s, separated ZIP 395 s -> 7.5 s, Google calls 600 -> 50, longest main-thread freeze 1.7 s -> 0 ms, complete PDF 42 MB -> 17 MB. The PDF's content is unchanged (same pages, texts and links in es and en). API - AsyncTTLCache: the satellite image depends only on the farm, so it is fetched once for all layers (LRU 256, 10 min TTL, in-flight dedup, failures not cached) - One httpx client for the app's lifespan - RasterDatasetCache: rasters stay open between requests (LRU 16 keyed by the versioned path, one reader at a time per raster) - Overlay drawing, compositing and encoding run in threads - /generate-image returns JPEG (q85, 4:4:4): ~5x smaller than the PNG Web - ReportProvider fetches the images once per selection and shares them with the preview and both downloads (the downloads fetched them again; the ZIP made up to 300 requests) - Every PDF and the ZIP render in a Web Worker; the preview is an iframe over its blob instead of <PDFViewer>, which regenerated itself - The complete report (with links) is pre-rendered once the preview shows - The satellite limit counts distinct farms, not farm x layer pairs - Roboto served from public/fonts (OFL); lighter cover image - Preview errors show a snackbar instead of a blank page OpenSpec change: optimize-report-pdf-generation (all tasks done, with the measurements).
…e message CodeQL (js/unvalidated-dynamic-method-call, 37 alerts) traced the worker's message to the t() calls in the report: the message's locale picked the translations. Only the report page messages the worker, but it now uses the configured locale that matches (es, en) and rejects anything else.
BlancaMunizaga
left a comment
BlancaMunizaga
left a comment
There was a problem hiding this comment.
1. Summary of Findings
- 🔴 Critical: 0
- 🟠 High: 2
- 🟡 Medium: 1
- 🟢 Low: 1
2. Prioritized Findings
- 🟠 High · Reliability: A worker can start after the report page unmounts —
apps/web/src/context/ReportContext.tsx:148(inline). - 🟠 High · Reliability: A failed preview leaves a permanent loading spinner with no retry —
apps/web/src/context/ReportContext.tsx:199(inline). - 🟡 Medium · Performance: Reports with more than 256 farms can fetch every satellite image again for the next layer when the limit is unset —
apps/api/app/utils/image_generation/GoogleMapsAPIHelper.py:31(inline). - 🟢 Low · Documentation:
docs/onboarding.md:73still describes/generate-imageas PNG, although this PR changes it to JPEG. The onboarding guide says behavior changes must update it, and it is the canonical flow description for new contributors. Please update that report step to JPEG and the worker-based preview.
4. Positive Feedback
The version check still prevents reports from mixing raster versions. The new cache has focused tests for concurrent requests, failures, expiry, and eviction. Both API and frontend CI checks passed on this head commit.
If the user left the preview page while the images were still being fetched, the unmount cleanup found no worker to terminate, and the render started one once the fetch resolved: a worker rendering the whole PDF, holding the image blobs, with no one to stop it. The render now checks that the page is still mounted before creating the worker and fails with ReportPdfWorkerTerminatedError otherwise, which the preview already ignores; the download hook ignores it too, so leaving the page mid-download shows no error.
… fails A failed preview only showed a snackbar: the preview stayed null, so the spinner ran forever, and nothing re-ran the effect. The provider now records the failed selection, which stops the loading state and exposes previewFailed and retryPreview; the preview page replaces the spinner with an error and a Retry button (reportGeneration:preview:error and retry, en and es). The retry bumps an attempt counter so the effect fetches or renders again (the failed promise was already forgotten). The snackbar text moves out of common:snackbarAlerts.
…lite cache is hit for every farm The API caches the last 256 satellite images (one per farm). With a single map-major pass over the report, a farm's requests for the first and the last map were a whole report apart, so above 256 farms (the limit can be unset) every entry was evicted before its next use: 257 farms x 2 maps made 514 Google calls instead of 257. The requests now go out by groups of 64 farms, map-major within each group, through the same single pLimit(20) queue: at most 63 other farms come between a farm's first and last request, so the cache holds every farm's image until its last map, with room for other reports in flight. Up to 64 farms the order is unchanged from the measured one.
…iew in the onboarding guide The report steps still said /generate-image returns a PNG and didn't mention that the images are fetched once per selection, that the PDF renders in a Web Worker, or that the complete report is pre-rendered.
Fixed in 54176be. Step 7 now says the image is a 500×500 JPEG, that the API fetches each farm's satellite image once and caches it for the other maps, and that the images are fetched once per selection. Step 8 describes the Web Worker render, the |
Review triageAll four findings of the review were valid and are fixed; nothing was discarded or deferred. Addressed
Verification. Web only (no API change): Left open. Aborting the pending |
nivek0o0
left a comment
nivek0o0
left a comment
There was a problem hiding this comment.
Findings
- 🟢 Low · Reliability: The dataset handle leaks if
WarpedVRTfails —apps/api/app/utils/image_generation/RasterDatasetCache.py:13(inline). - 🟢 Low · Documentation: The PR description no longer matches the code. The "Web" section says preview errors "show a new snackbar (
errorGeneratingReportPreview, en/es)". Since f30a7fc they show an inlineAlertwith a Retry button, usingreportGeneration:preview:error/retry, and the keyerrorGeneratingReportPreviewno longer exists anywhere. Please update that bullet so the description stays an accurate record of the change.
Open Questions
- Re-seeding the share while the API runs.
RasterDatasetCachekeeps up to 16 raster handles open indefinitely on the SMB mount.layers-ops.sh seedempties and re-uploads the share, and its README says it "doesn't restart anything". If the seed writes a raster to the same path as one the API has open, the cache (keyed by path alone) would keep serving the old dataset. Report images would then be drawn from a different raster than the/analizeratios, which open the file fresh. Can the seed produce the same filenames? If so, either the seed docs should require an API restart, or the cache key should includest_mtime_ns/st_size. I couldn't verify how Azure Files and the filename scheme behave here (confidence: Low). - Google Maps Platform terms. The description says the 10-minute TTL was approved. Did that approval cover the Maps terms on caching Static Maps content?
satellite_image_cacheis server-side and shared across users, which is a different situation from embedding the image in one user's PDF.
| self.src: Any = rasterio_open(path) | ||
| self.vrt: Any = WarpedVRT(self.src, crs=target_crs) |
There was a problem hiding this comment.
🟢 Low · Reliability — The dataset handle leaks if WarpedVRT fails.
src is opened first, then WarpedVRT(self.src, ...) is built. If the VRT raises (a corrupt or unreadable COG, a transient SMB error on the Azure Files mount), _get never stores the entry and nothing closes src. Every retry for that raster leaks another GDAL handle on the share. RasterDataContext closed the handle in every case before.
Suggested fix:
self.src: Any = rasterio_open(path)
try:
self.vrt: Any = WarpedVRT(self.src, crs=target_crs)
except Exception:
self.src.close()
raiseThere was a problem hiding this comment.
Fixed in 54f0ca3.
_CachedRaster.__init__ now closes the dataset and re-raises if WarpedVRT fails, as you suggested (with except BaseException, so a cancellation doesn't leak either). Regression test test_the_dataset_is_closed_if_the_vrt_fails: a failing VRT leaves the cache empty and the opened dataset closed, and the next read opens the raster normally. Black, ruff, mypy and the suite (296) are clean.
nivek0o0
left a comment
nivek0o0
left a comment
There was a problem hiding this comment.
Review update
- 🟠 High · Reliability: Missing local italic Roboto faces breaks reports whose layer considerations use italics —
apps/web/src/utils/deforestationReport.tsx:34(inline).
| fonts: [ | ||
| { src: assetUrl("/fonts/roboto/Roboto-Regular.ttf"), fontWeight: 400 }, | ||
| { src: assetUrl("/fonts/roboto/Roboto-Medium.ttf"), fontWeight: 500 }, | ||
| { src: assetUrl("/fonts/roboto/Roboto-Bold.ttf"), fontWeight: 700 }, |
There was a problem hiding this comment.
🟠 High · Reliability — Restore the italic Roboto faces. The new local registration includes only normal styles, but sections.tsx still renders _..._ consideration text with fontStyle: "italic", and the enabled Colombia IDEAM layer contains that markup in both languages. React-pdf fails when an italic style is requested without a registered italic face, so selecting IDEAM prevents the preview and both downloads from rendering; the old registration included regular-italic and bold-italic faces. Please bundle/register Roboto-Italic.ttf and Roboto-BoldItalic.ttf (or otherwise remove the italic style deliberately), and add a render regression using the IDEAM considerations. The current reference benchmark uses maps 0–2, so it does not exercise IDEAM/map 3.
There was a problem hiding this comment.
Fixed in 9e08a1b.
registerReportFonts registers Roboto-Italic (400 italic, the same v2.137 file gstatic served) and Roboto-BoldItalic (700 italic, from the v2.138 release, subset like the others), and the licence file is the v2 files' Apache 2.0. Note that bold italic was already broken on dev: its gstatic URL answers 404.
On the regression: the web has no test harness, and the project convention is not to set one up inside a fix, so there is no committed test. I did check it outside the app, rendering a document with @react-pdf/renderer in Node using the real _…_ quote from considerations/en/ideam.md: with the three upright faces only it fails with Could not resolve font for Roboto, fontWeight 400, fontStyle italic, and with the five faces of 9e08a1b it renders both the italic and the bold-italic text. If you'd rather have that as a proper regression, I'd propose a follow-up that adds a minimal Node render test for sections.tsx (it needs a TSX runner the web doesn't have today).
The layers' considerations are markdown, and their _italic_ and **_bold italic_** render with fontStyle "italic". The italic faces were dropped when the fonts moved to public/fonts, so a layer with italics (Colombia's IDEAM) failed the whole render: "Could not resolve font for Roboto, fontWeight 400, fontStyle italic". - Roboto-Italic: the same v2.137 file the report used from gstatic - Roboto-BoldItalic: from the official v2.138 release, subset to the same characters as the other faces. Its old gstatic URL answers 404, so bold italic already failed before this branch - The v2 files are Apache 2.0, not OFL: LICENSE replaces OFL.txt, and the CHANGELOG, README and OpenSpec artifacts say so
_CachedRaster opened the dataset and then built the VRT; if the VRT raised (a corrupt raster, a transient error on the share) the entry was never stored and nothing closed the dataset, so every retry leaked a GDAL handle. The dataset is now closed before re-raising. With a regression test.
The raster cache was keyed by path alone, on the premise that a raster is never replaced in place. The admin honours it, but layers-ops.sh seed rewrites the same <stem>-v1.tif paths on a running API: a cached handle would keep drawing the old raster while /analize, which opens the file fresh, used the new one. Each entry now remembers the file's (st_mtime_ns, st_size), checked with one os.stat per read as LayerStore.read_index already does, and a changed file is closed and reopened. With a test.
Updated: the bullet now says the preview replaces the spinner with an error and a "Retry" button (
Yes.
I can't confirm that it did, so I'm leaving this open for the team rather than closing it. What I checked today:
What the cache does today: in-memory only, 256 entries, 10-minute TTL, never persisted, used to serve the same farm's image to the other layers of the same report. If the team decides the terms don't cover it, the smallest change that keeps most of the gain is dropping the TTL to the in-flight window (share concurrent requests, forget the bytes right after): with the grouped request order of ecd6065 the other layers' requests for a farm arrive within seconds. I'd rather have that decision made explicitly than guess at it here. |
Review triage (round 2, @nivek0o0's review)Addressed
Left open
Verification. API: black, ruff, mypy clean; 296 tests pass. Web: unchanged this round beyond 9e08a1b (tsc and lint were clean on it). |
- New spec `report-generation` (11 requirements): images fetched once per selection, one satellite call per farm, the satellite limit counts farms, JPEG images, rasters follow the file on disk, the API isn't blocked, the PDF renders off the main thread, the download is pre-rendered, a failed preview can be retried, no third-party requests, and every text style (italic and bold italic included) has a font face - The delta spec gains the three behaviours added by the fixes after review (preview retry, raster reopened when its file changes, italic faces), and states the image release as implemented (Blobs; the worker owns the URLs) - The change moves to openspec/changes/archive/2026-10-08-optimize-report-pdf-generation
2 participants
Implements the OpenSpec change
optimize-report-pdf-generation. The PDF report was slow, expensive in Google Static Maps calls and froze the browser; the PDF stays client-side (@react-pdf), moving it to the server was ruled out.Results
Reference report: 50 farms × 3 layers (GFW, TMF, MAATE 2020-2022), satellite background on, production Docker images of
208b7b8and of this branch, API cache cold, timed with Playwright over the real flow.fonts.gstatic.comThe content doesn't change: the complete PDF from both versions has the same 159 pages, the same text on every page and the same 153 links, in
esanden; the per-farm PDFs in the ZIP match too.Bugs fixed
TODOfor it).API
AsyncTTLCache(new): the satellite image depends only on the farm, soGoogleMapsAPIHelperfetches it once for all layers. LRU of 256, 10-minute TTL (approved), shares in-flight calls, doesn't cache failures, checks the bytes decode before caching.httpx.AsyncClientfor the app's lifespan.RasterDatasetCache(new): rasters stay open between requests, LRU of 16 keyed by the versioned path, one reader at a time per raster (GDAL handles aren't thread-safe). Rasters are never replaced in place, so an entry can't go stale./generate-imagereturnsimage/jpeg(quality 85, no chroma subsampling): 106 KB instead of 505 KB. The OpenAPI doesn't declare the media type, so the contract and the generated types don't change. Tiles stay PNG.Web
ReportProvider(src/context/ReportContext.tsx, new) fetches the images once per selection and shares them with the preview and both downloads.useDeforestationCompleteReportDocumentis removed;useDeforestationReportDownloadkeeps its API.src/workers/reportPdf.worker.tsx+reportPdfClient.ts): every PDF and the ZIP render off the main thread. The worker sets up its own i18next (same bundled locale files) and runtime config.iframeover the worker's blob instead of<PDFViewer>.public/fonts/roboto(Apache 2.0; the italics are needed by layers whose considerations use_italic_, like IDEAM, and bold italic's old gstatic URL was a 404), absolute URLs viaassetUrl(), cover image 211 KB → 32 KB.reportGeneration:preview:error/retry, en/es); the failed render is forgotten, so the retry fetches or renders again.Design deviation
The design proposed grouping each farm's image requests so they'd share the satellite call. Measured, it was twice as slow (5.2 s vs 2.5 s for 150 images): it cut the parallel Google calls from 20 to ~7. The current order already hits the cache for every farm while a report has ≤ 256 farms, which the satellite limit guarantees. Recorded in
design.md.Verified
openapi.jsondoesn't change.tscis clean; lint has 0 errors (18 warnings, all pre-existing).Dockerfile.prodbuilds. Turbopack bundles the worker, andentrypoint.shsubstitutes the variables in its chunks (no placeholder left).js/unvalidated-dynamic-method-callalerts: the worker's message reached the report'st()calls through the locale. The worker now takes the configured locale that matches (es, en) and rejects anything else (second commit); CodeQL reports no new alerts.Notes
static/media/reportPdf.worker.*.tsx. It isn't loaded and is just the repo's code.node .next/standalone/server.jsanswered every route with a 307 to itself; the Docker image doesn't, so it looks specific to that local setup. Not addressed here.