feat(storymap): PDF handout generator for the Story Map plugin#856
Conversation
Add a PDF Handout Generator to the Story Map panel (#830). A new "Handout (PDF)" dialog lets the user select which chapter views to include, choose the paper size and orientation, and set a document title and footer. Generating flies the live map to each selected chapter, captures the rendered view, and assembles a clean multi-page PDF (one chapter per page) saved through the existing Tauri/browser save bridge. The map returns to its original view when done. The PDF builder (storymap-pdf.ts) is pure and unit tested; map capture reuses the Print Layout captureMapImage helper. Fixes #830
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
More reviews will be available in 23 minutes and 59 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a Story Map handout export flow. It introduces a PDF builder, a dialog to choose chapters and page settings, panel wiring to open the dialog, localized strings, and tests for the PDF utilities. ChangesStory Map handout export
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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 |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Chapters with an image now show that photo next to the captured map view on their handout page (map left, photo right). The photo is loaded into a canvas with CORS so it can be embedded; if it fails to load or would taint the canvas the page falls back to the map alone. Fixes #830
![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 `@apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsx`:
- Around line 316-359: The StoryMap handout form fields in StoryMapHandoutDialog
are missing accessible label associations because Field only renders a visual
label. Update each Field/Select/Input pair to pass matching htmlFor and id
values so the controls get an accessible name, and apply the same fix to the
other affected fields later in the component. Use the existing
StoryMapHandoutDialog, Field, Select, and Input elements to locate the places
where the paper size, orientation, document title, and footer text controls are
rendered.
- Around line 76-103: `loadChapterPhoto` can hang forever on a pending remote
image request, blocking the handout export flow. Add a timeout inside
`loadChapterPhoto()` in `StoryMapHandoutDialog` so the Promise resolves to null
if `img.onload`/`img.onerror` never fires, and make sure the timeout is cleared
on success or failure; keep the existing fallback behavior by returning null
from the timed-out path.
In `@apps/geolibre-desktop/src/lib/storymap-pdf.ts`:
- Around line 117-120: The drawImageInBox helper is hard-coding the image format
passed to pdf.addImage, which breaks JPEG exports when HandoutImage.data
contains a JPEG data URL. Update drawImageInBox in storymap-pdf.ts to derive the
format from image.data (or otherwise pass the correct format dynamically)
instead of always using "PNG", so jsPDF uses the right parser for both PNG and
JPEG inputs.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 7b993b0f-9318-448b-aee2-e2e990c96e1f
📒 Files selected for processing (5)
apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsxapps/geolibre-desktop/src/components/storymap/StoryMapPanel.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/storymap-pdf.tstests/storymap-pdf.test.ts
Code reviewBugs
Performance
Quality
SecurityNothing found. CLAUDE.mdAll i18n strings are added to Overall: The feature is well-structured — pure builder separated from orchestration, solid error recovery in the |
- StoryMapHandoutDialog: seed the footer field with singleLine(story.footer) so the input shows readable text instead of raw HTML (Claude). - StoryMapHandoutDialog: add a Stop button that aborts an in-progress export via an abort ref checked in the capture loop; the loop abandons the export when nothing was captured (Claude B1). - StoryMapHandoutDialog: add a timeout to loadChapterPhoto so a hung remote image can no longer stall the whole export (CodeRabbit). - StoryMapHandoutDialog: associate each field Label with its control via htmlFor/id for accessibility (CodeRabbit). - storymap-pdf: decode extended named entities (mdash, ndash, hellip, smart quotes, etc.) and numeric (decimal/hex) entities (Claude). - storymap-pdf: append an ellipsis when a description is truncated so the cut reads as intentional (Claude). - storymap-pdf: derive the jsPDF image format from a data URL's MIME type so a JPEG is not handed to the PNG parser (CodeRabbit). - storymap-pdf: correct the footer comment to describe first-line-only behaviour rather than width clipping (Claude nit).
- storymap-pdf: strip <script>/<style> blocks (with their text content) before the generic tag pass so imported HTML descriptions can't leak CSS/JS text into the handout. - StoryMapHandoutDialog: NFD-normalize the slug so accented Latin titles (e.g. "São Paulo") keep their skeleton instead of dropping characters. - StoryMapHandoutDialog: discard the export entirely when Stop is pressed rather than saving a partial PDF (the finally block still restores the map view).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsx (1)
213-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor Stop before building or saving a partial PDF.
If the user clicks Stop after one chapter is captured, the next loop check breaks, but
captures.length > 0lets the code continue into PDF generation and the save prompt. TreatabortRef.currentas a full cancellation before building/saving.Proposed fix
for (let i = 0; i < chosen.length; i++) { if (abortRef.current) break; const chapter = chosen[i]; setProgress({ current: i + 1, total: chosen.length }); await jumpAndWaitIdle(map, chapter.location); + if (abortRef.current) break; const shot = captureMapImage(map); // Load the chapter's own photo (if any) so it appears beside the map. const photo = chapter.image ? await loadChapterPhoto(chapter.image) : null; + if (abortRef.current) break; captures.push({ title: chapter.title, description: chapter.description, map: { data: shot.image, width: shot.width, height: shot.height }, ...(photo ? { photo } : {}), }); } - // Stopped before capturing anything: abandon the export rather than - // writing an empty PDF. - if (captures.length === 0) { + // Stopped or captured nothing: abandon the export rather than writing a + // partial/empty PDF. + if (abortRef.current || captures.length === 0) { return; }🤖 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 `@apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsx` around lines 213 - 234, The export flow in StoryMapHandoutDialog should treat abortRef.current as a full cancellation, not just a loop break; after the capture loop, check for cancellation before any PDF building or save prompt, so a Stop click after one chapter prevents partial exports. Update the logic around the captures array and the PDF generation/save path to exit early when abortRef.current is set, using the existing jumpAndWaitIdle, captureMapImage, and loadChapterPhoto flow as the reference point.apps/geolibre-desktop/src/lib/storymap-pdf.ts (1)
248-268: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLet the image band collapse when the title already used the page.
If
chapter.titlewraps to many lines,bottomLimit - y - reservedForTextcan go negative here, but theMath.max(20, ...)floor still forces a 20 mm image block. That guarantees the image overprints the footer area on long-title/small-page exports.Suggested fix
const reservedForText = chapter.description ? 24 : 4; - const imageBandHeight = Math.max(20, bottomLimit - y - reservedForText); + const imageBandHeight = Math.max(0, bottomLimit - y - reservedForText); const gap = 5; - if (chapter.photo) { + if (imageBandHeight > 0 && chapter.photo) { // Map on the left, the chapter photo on the right, each fit into its own // half-width column and vertically centered within the band. const colWidth = (contentWidth - gap) / 2; drawImageInBox(pdf, chapter.map, MARGIN_MM, y, colWidth, imageBandHeight); @@ - } else { + } else if (imageBandHeight > 0) { // No photo: the map view spans the full content width. drawImageInBox(pdf, chapter.map, MARGIN_MM, y, contentWidth, imageBandHeight); } - y += imageBandHeight + 5; + if (imageBandHeight > 0) y += imageBandHeight + 5;🤖 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 `@apps/geolibre-desktop/src/lib/storymap-pdf.ts` around lines 248 - 268, The image band sizing in storymap-pdf should not force a minimum height when the remaining page space is already exhausted by a wrapped title. Update the image layout logic around the imageBandHeight calculation in storymap-pdf so it collapses or skips drawing when bottomLimit - y - reservedForText is non-positive, rather than always using Math.max(20, ...). Keep the behavior in the chapter.photo branch and the no-photo drawImageInBox path consistent so images never encroach on the footer area.
🤖 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 `@apps/geolibre-desktop/src/lib/storymap-pdf.ts`:
- Around line 91-97: The numeric entity handling in the text replacement
callback can still throw when a malformed code point like a very large reference
is parsed by the entity decoder in storymap-pdf’s replace logic. Update the
callback that processes matched entities so it validates the parsed numeric
value before calling String.fromCodePoint, and return the original match for
invalid or out-of-range values. Keep the fix localized to the entity-decoding
path in the text replacement function.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsx`:
- Around line 213-234: The export flow in StoryMapHandoutDialog should treat
abortRef.current as a full cancellation, not just a loop break; after the
capture loop, check for cancellation before any PDF building or save prompt, so
a Stop click after one chapter prevents partial exports. Update the logic around
the captures array and the PDF generation/save path to exit early when
abortRef.current is set, using the existing jumpAndWaitIdle, captureMapImage,
and loadChapterPhoto flow as the reference point.
In `@apps/geolibre-desktop/src/lib/storymap-pdf.ts`:
- Around line 248-268: The image band sizing in storymap-pdf should not force a
minimum height when the remaining page space is already exhausted by a wrapped
title. Update the image layout logic around the imageBandHeight calculation in
storymap-pdf so it collapses or skips drawing when bottomLimit - y -
reservedForText is non-positive, rather than always using Math.max(20, ...).
Keep the behavior in the chapter.photo branch and the no-photo drawImageInBox
path consistent so images never encroach on the footer area.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 5b49b393-c331-4cc1-97ec-22d8f50d8283
📒 Files selected for processing (4)
apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/storymap-pdf.tstests/storymap-pdf.test.ts
- storymap-pdf: guard numeric HTML entity decoding against out-of-range code points so a malformed reference (e.g. �) is left untouched instead of throwing and aborting the export.
Code reviewReviewed Bugs
Performance
Quality
CLAUDE.mdNo violations. i18n strings are added to Overall: The feature is well-structured and the majority of the code is solid. The two behavioural issues worth fixing before merge are the silent partial PDF on Stop (#1) and the Stop-button latency (#2) — both are easy one-liners once the design intent (cancel-all vs. keep-partial) is decided. |
- StoryMapHandoutDialog: resolve jumpAndWaitIdle on the next frame when a chapter shares the previous chapter's exact location (a no-op jumpTo emits no idle event), instead of waiting the full timeout per such page. - StoryMapHandoutDialog: skip crossOrigin for data: URI chapter photos so browsers that apply CORS to data images don't spuriously drop them.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsx (1)
321-420: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDisable export settings while generation is running.
The chapter checkboxes and title/footer/layout controls remain editable during export, but
handleGenerate()uses the values captured when Generate was clicked. Edits made mid-export look accepted in the dialog but won’t affect the saved PDF, which can produce surprising output. Disable these controls whilegeneratingis true.Proposed fix
<Button size="sm" variant="ghost" className="h-7 px-2 text-xs" + disabled={generating} onClick={toggleAll} > @@ <input type="checkbox" + disabled={generating} checked={selected[chapter.id] ?? false} @@ <Select id="storymap-handout-paper-size" + disabled={generating} value={paperSize} @@ <Select id="storymap-handout-orientation" + disabled={generating} value={orientation} @@ <Input id="storymap-handout-document-title" + disabled={generating} value={title} @@ <Input id="storymap-handout-footer" + disabled={generating} value={footer}🤖 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 `@apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsx` around lines 321 - 420, The export form in StoryMapHandoutDialog still allows edits while handleGenerate() is running, which can mislead users because the PDF uses the values captured at click time. In StoryMapHandoutDialog, disable the chapter checkboxes, the toggleAll button, and the paperSize/orientation/title/footer inputs whenever generating is true so the current settings stay locked during export.
🤖 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.
Outside diff comments:
In `@apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsx`:
- Around line 321-420: The export form in StoryMapHandoutDialog still allows
edits while handleGenerate() is running, which can mislead users because the PDF
uses the values captured at click time. In StoryMapHandoutDialog, disable the
chapter checkboxes, the toggleAll button, and the
paperSize/orientation/title/footer inputs whenever generating is true so the
current settings stay locked during export.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6bf7a3b0-7336-4b5c-ba3e-146c6284623a
📒 Files selected for processing (3)
apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsxapps/geolibre-desktop/src/lib/storymap-pdf.tstests/storymap-pdf.test.ts
Code reviewOverall this is a well-structured addition: the PDF builder is cleanly separated from the dialog, the abort-and-restore pattern is correct, the HTML→plain-text conversion handles the common entity set, tests cover the main paths, and i18n strings are complete. The items below range from a real (if edge-case) layout bug to a missing import. Bugs
Performance
Quality
CLAUDE.mdNo violations found. New i18n strings are present in |
Code reviewReviewed BugsImage band overruns the footer for long chapter titles — medium confidence, inline comment on
QualitySilent abort — no feedback that export was cancelled — high confidence, inline comment on When the user clicks Stop and the early-return fires,
The same 5-second constant covers both map tile-settle waits (where the full timeout is reasonable) and chapter photo loads (where a repeated 5-second DNS hang multiplied by every chapter is a noticeable stall). Separate constants would let each be tuned independently. PerformanceNo significant issues beyond the photo-timeout concern above. The SecurityNo issues found. CLAUDE.md
|
- storymap-pdf: exclude code point 0 from numeric entity decoding so � no longer inserts a null byte into the PDF text stream. - storymap-pdf: skip the image band entirely when a long title has already consumed the page, instead of drawing a fixed 20mm band over the footer. - storymap-pdf: drop the unreachable "?? footerText" fallback. - StoryMapHandoutDialog: import maplibregl explicitly (matching sibling components) rather than relying on the ambient global. - StoryMapHandoutDialog: give chapter-photo loading its own shorter timeout (3s) so a slow photo host doesn't double the per-chapter stall.
- StoryMapHandoutDialog: show an "Export cancelled" notice when the user stops an in-progress export, instead of silently returning to idle.
| }); | ||
| const saved = await saveBinaryFileWithFallback(bytes, { | ||
| defaultName: `${slugify(title || story.title)}-handout.pdf`, | ||
| filters: [{ name: t("storymap.handout.pdfFile"), extensions: ["pdf"] }], |
There was a problem hiding this comment.
Bug (medium confidence): raw story.title HTML leaks into the filename when the document-title field is cleared.
story.title is a raw HTML string (story titles can contain markup like <em>Europe Tour</em>). When the user empties the "Document title" field and triggers the fallback, slugify converts the HTML delimiters to dashes — e.g. "<em>Paris</em>" → "em-paris-em-handout.pdf".
The title state is already stripped of HTML by singleLine() in the useEffect, so only the raw fallback is affected.
| filters: [{ name: t("storymap.handout.pdfFile"), extensions: ["pdf"] }], | |
| defaultName: `${slugify(title || singleLine(story.title))}-handout.pdf`, |
| // (the finally block still restores the map view). | ||
| if (abortRef.current || captures.length === 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
UX (low confidence / design question): no feedback to the user after a manual stop.
When the user clicks "Stop" the loop breaks here and the finally block clears both generating and progress, leaving the dialog in its pre-export idle state with no indication that the export was aborted. After watching "Capturing view 3 of 5…" for several seconds, a silent reset is a bit jarring.
If this is intentional (the user clearly chose to stop), it's fine as-is. If you want to surface confirmation, a brief setError(t("storymap.handout.stopped")) (with a muted colour rather than destructive red) here would be one option.
| }; | ||
| const timer = setTimeout(() => finish(null), PHOTO_TIMEOUT_MS); | ||
| // Only request CORS for real remote URLs. A data: URI has no origin, and | ||
| // some browsers fire `onerror` for `crossOrigin` data images, which would |
There was a problem hiding this comment.
Nit: IDLE_TIMEOUT_MS (named for the map-idle wait) is reused here for the photo-load timeout.
Both are 5 s, which is a reasonable value for each, but the constant name ("map idle timeout") is misleading in this context. A separate constant or an inline comment would clarify the intent:
| // some browsers fire `onerror` for `crossOrigin` data images, which would | |
| const timer = setTimeout(() => finish(null), 5000 /* photo load timeout */); |
(Or introduce a PHOTO_LOAD_TIMEOUT_MS = 5000 constant near IDLE_TIMEOUT_MS.)
Code reviewReviewed Bugs
SecurityNothing flagged. PerformanceNothing flagged. The sequential chapter-capture loop (one Quality
CLAUDE.md
What I verified was correct
|
| }); | ||
| // A no-op jump (an adjacent chapter sharing this exact location) changes | ||
| // nothing, so MapLibre fires no `idle` and the wait would hit the full | ||
| // timeout. The current frame is already rendered with tiles loaded, so | ||
| // resolve on the next frame instead. | ||
| const after = map.getCenter(); | ||
| if ( | ||
| before.lng === after.lng && | ||
| before.lat === after.lat && | ||
| map.areTilesLoaded() | ||
| ) { | ||
| requestAnimationFrame(finish); | ||
| } | ||
| // Register after jumpTo so a pre-existing idle event isn't consumed before | ||
| // the new camera has started rendering. | ||
| map.on("idle", finish); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Load a chapter image URL (or data URI) into a canvas for embedding in the | ||
| * PDF. Resolves to null when the image fails to load or is cross-origin without | ||
| * CORS headers (so it would taint the canvas), letting the export proceed with |
There was a problem hiding this comment.
Bug (medium confidence) — no-op detection ignores zoom, pitch, and bearing
The comment says this path is for "a no-op jump (an adjacent chapter sharing this exact location)", but the condition only checks lng/lat, not zoom, pitch, or bearing. When two consecutive chapters share the same centre but differ in zoom (e.g. the classic "zoom in on a city across chapters" pattern), the condition fires requestAnimationFrame(finish) and the idle listener is removed before MapLibre has a chance to mark new-zoom tiles as pending. captureMapImage forces a redraw() but that renders with whatever tiles are in the buffer — tiles for the previous zoom — producing a blurry or tiled-pattern capture for that page.
The fix is to capture the full camera state before the jump and compare all four parameters:
| }); | |
| // A no-op jump (an adjacent chapter sharing this exact location) changes | |
| // nothing, so MapLibre fires no `idle` and the wait would hit the full | |
| // timeout. The current frame is already rendered with tiles loaded, so | |
| // resolve on the next frame instead. | |
| const after = map.getCenter(); | |
| if ( | |
| before.lng === after.lng && | |
| before.lat === after.lat && | |
| map.areTilesLoaded() | |
| ) { | |
| requestAnimationFrame(finish); | |
| } | |
| // Register after jumpTo so a pre-existing idle event isn't consumed before | |
| // the new camera has started rendering. | |
| map.on("idle", finish); | |
| }); | |
| } | |
| /** | |
| * Load a chapter image URL (or data URI) into a canvas for embedding in the | |
| * PDF. Resolves to null when the image fails to load or is cross-origin without | |
| * CORS headers (so it would taint the canvas), letting the export proceed with | |
| const before = map.getCenter(); | |
| const beforeZoom = map.getZoom(); | |
| const beforePitch = map.getPitch(); | |
| const beforeBearing = map.getBearing(); | |
| map.jumpTo({ | |
| center: location.center, | |
| zoom: location.zoom, | |
| pitch: location.pitch, | |
| bearing: location.bearing, | |
| }); | |
| // A no-op jump (an adjacent chapter sharing this exact location) changes | |
| // nothing, so MapLibre fires no `idle` and the wait would hit the full | |
| // timeout. The current frame is already rendered with tiles loaded, so | |
| // resolve on the next frame instead. | |
| const after = map.getCenter(); | |
| if ( | |
| before.lng === after.lng && | |
| before.lat === after.lat && | |
| beforeZoom === location.zoom && | |
| beforePitch === location.pitch && | |
| beforeBearing === location.bearing && | |
| map.areTilesLoaded() | |
| ) { | |
| requestAnimationFrame(finish); | |
| } | |
| // Register after jumpTo so a pre-existing idle event isn't consumed before | |
| // the new camera has started rendering. | |
| map.on("idle", finish); |
| // wrongly drop an embedded photo. | ||
| if (!url.startsWith("data:")) img.crossOrigin = "anonymous"; | ||
| img.onload = () => { | ||
| try { |
There was a problem hiding this comment.
Nit — in-flight image request is not cancelled on timeout
When PHOTO_TIMEOUT_MS fires, finish(null) correctly nulls out the handlers and resolves the promise, but the browser continues the network request in the background until it finishes or errors. For a story with many chapters all having slow remote photos, several concurrent fetches linger after each per-chapter timeout. Adding img.src = "" inside the timeout callback (before finish) aborts the request immediately:
| try { | |
| const timer = setTimeout(() => { img.src = ""; finish(null); }, PHOTO_TIMEOUT_MS); |
| }); | ||
| // A no-op jump (an adjacent chapter sharing this exact location) changes | ||
| // nothing, so MapLibre fires no `idle` and the wait would hit the full | ||
| // timeout. The current frame is already rendered with tiles loaded, so | ||
| // resolve on the next frame instead. | ||
| const after = map.getCenter(); | ||
| if ( | ||
| before.lng === after.lng && | ||
| before.lat === after.lat && | ||
| map.areTilesLoaded() | ||
| ) { | ||
| requestAnimationFrame(finish); | ||
| } | ||
| // Register after jumpTo so a pre-existing idle event isn't consumed before | ||
| // the new camera has started rendering. | ||
| map.on("idle", finish); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug (medium-high confidence): No-op detection ignores zoom, bearing, and pitch
The fast-path only checks whether the map centre changed, so if two adjacent chapters share the same lat/lng but have different zoom levels (a common authoring pattern — e.g. zoom-in "chapter 1 overview → chapter 2 detail"), jumpTo triggers a tile load for the new zoom level, but the guard evaluates to true and resolves after a single animation frame instead of waiting for idle. The capture then fires before the new tile set is loaded, producing a blurry or partially-empty screenshot.
| }); | |
| // A no-op jump (an adjacent chapter sharing this exact location) changes | |
| // nothing, so MapLibre fires no `idle` and the wait would hit the full | |
| // timeout. The current frame is already rendered with tiles loaded, so | |
| // resolve on the next frame instead. | |
| const after = map.getCenter(); | |
| if ( | |
| before.lng === after.lng && | |
| before.lat === after.lat && | |
| map.areTilesLoaded() | |
| ) { | |
| requestAnimationFrame(finish); | |
| } | |
| // Register after jumpTo so a pre-existing idle event isn't consumed before | |
| // the new camera has started rendering. | |
| map.on("idle", finish); | |
| }); | |
| } | |
| const beforeZoom = map.getZoom(); | |
| const beforeBearing = map.getBearing(); | |
| const beforePitch = map.getPitch(); | |
| const before = map.getCenter(); | |
| map.jumpTo({ | |
| center: location.center, | |
| zoom: location.zoom, | |
| pitch: location.pitch, | |
| bearing: location.bearing, | |
| }); | |
| // A no-op jump (adjacent chapters sharing the exact same view) changes | |
| // nothing, so MapLibre fires no `idle` and the wait would hit the full | |
| // timeout. The current frame is already rendered with tiles loaded, so | |
| // resolve on the next frame instead. | |
| const after = map.getCenter(); | |
| if ( | |
| before.lng === after.lng && | |
| before.lat === after.lat && | |
| map.getZoom() === beforeZoom && | |
| map.getBearing() === beforeBearing && | |
| map.getPitch() === beforePitch && | |
| map.areTilesLoaded() | |
| ) { | |
| requestAnimationFrame(finish); | |
| } |
| // Always return the map to where the user left it, even on failure. | ||
| if (original) { | ||
| map.jumpTo({ |
There was a problem hiding this comment.
Nit: dialog stays open on save cancellation
saveBinaryFileWithFallback returns null when the user dismisses the native save dialog without choosing a file (desktop) or cancels the File System Access picker (browser). In that case saved !== null is false, so the dialog stays open — which is intentional — but the user sees no feedback: the progress and error messages are both cleared by the finally block, and no notice is set on a cancelled save.
Consider setting a neutral notice (similar to the abort case) so the user knows the PDF was generated but not saved:
| // Always return the map to where the user left it, even on failure. | |
| if (original) { | |
| map.jumpTo({ | |
| if (saved !== null) { | |
| onOpenChange(false); | |
| } else { | |
| setNotice(t("storymap.handout.saveSkipped")); | |
| } |
(Requires a new i18n key, e.g. "saveSkipped": "PDF generated but not saved.")
| boxW: number, | ||
| boxH: number, | ||
| ): { width: number; height: number } { | ||
| if (w <= 0 || h <= 0) return { width: boxW, height: boxH }; |
There was a problem hiding this comment.
Nit: zero-dimension fallback in fitInto is misleading
When w ≤ 0 || h ≤ 0, the function returns { width: boxW, height: boxH }, which tells drawImageInBox to stretch the image to fill the box instead of skipping it. The path is unreachable in practice — loadChapterPhoto already returns null for zero-sized canvases and the map canvas always has positive dimensions — but as a standalone utility the contract is surprising.
A clearer fallback (no-op draw rather than a full-box stretch):
| if (w <= 0 || h <= 0) return { width: boxW, height: boxH }; | |
| if (w <= 0 || h <= 0) return { width: 0, height: 0 }; |
| assert.equal(count(one), 1); | ||
| assert.equal(count(three), 3); | ||
| }); | ||
|
|
||
| it("throws when given no chapters", () => { | ||
| assert.throws( |
There was a problem hiding this comment.
Nit: page-count heuristic matches the first /Count N in the byte stream, not necessarily the page tree
PDF objects frequently contain /Count in other contexts (e.g. font glyph count, form field count). A regex scan over the raw bytes finds the first match, which could be the wrong one if jsPDF ever reorders its output. The test passes today because jsPDF happens to emit the page tree /Count early, but it is brittle.
A slightly more robust approach is to count occurrences of /Page\n (the page type entry) or /Type /Page instead:
| assert.equal(count(one), 1); | |
| assert.equal(count(three), 3); | |
| }); | |
| it("throws when given no chapters", () => { | |
| assert.throws( | |
| const count = (bytes: Uint8Array): number => { | |
| const text = Buffer.from(bytes).toString("latin1"); | |
| // Count leaf page objects (/Type /Page) rather than scanning for /Count, | |
| // which can appear in font or other dictionaries. | |
| return (text.match(/\/Type\s*\/Page[^s]/g) ?? []).length; | |
| }; |
| const docTitle = singleLine(options.title); | ||
| const footerText = singleLine(options.footer); |
There was a problem hiding this comment.
Nit — redundant singleLine calls
By the time buildStoryMapHandoutPdf is called, options.title and options.footer have already been processed through singleLine in the dialog's useEffect (lines 217–218 of StoryMapHandoutDialog.tsx) and then potentially edited as plain text by the user. Calling singleLine a second time here is idempotent so it never corrupts output, but it is silent overhead on every page. If the intent is to keep drawChapterPage independently safe for callers that supply raw HTML, a brief comment to that effect would clarify the design decision.
| } | ||
|
|
||
| /** Reduce HTML/multi-line text to a single line of plain text for headers. */ | ||
| export function singleLine(value: string): string { |
There was a problem hiding this comment.
Nit — singleLine is exported but has no unit test
htmlToPlainText is well covered in tests/storymap-pdf.test.ts, but the additional step in singleLine — collapsing embedded newlines to spaces — is untested. Since the function is part of the public API (imported by the dialog to pre-populate the title/footer fields), adding one or two cases to the test suite (e.g. multi-line HTML titles) would complete the coverage.
Code reviewBugs
Quality
Tests
What was checked but found clean
|
Code reviewBugs
Performance
Quality
Security / CLAUDE.mdNo issues found. HTML-to-text stripping is for PDF text output only (never browser rendering), so incomplete entity handling carries no XSS risk. What was checked
|
1 participant
Summary
Adds a PDF Handout Generator to the Story Map plugin, as requested in #830. Presenters can now export the exact map views in a presentation as a clean, multi-page PDF handout instead of capturing screenshots by hand.
A new Handout (PDF) button in the Story Map panel opens a generator dialog with the three stages the request described:
Implementation
apps/geolibre-desktop/src/lib/storymap-pdf.ts- pure, unit-tested PDF builder (jsPDF). One page per chapter; HTML in descriptions/footer is reduced to plain text; images are fit to the content box while preserving aspect ratio.apps/geolibre-desktop/src/components/storymap/StoryMapHandoutDialog.tsx- the generator dialog and the capture orchestration (jump to each chapter, wait for the map to settle, then capture). Map capture reuses the Print LayoutcaptureMapImagehelper.StoryMapPanel.tsx- new Handout (PDF) button wired to the dialog.en.json.Testing
tests/storymap-pdf.test.ts- unit tests for the PDF builder (valid PDF output, one page per chapter, no-chapter guard, title/footer-less render) and the HTML-to-text helper.Fixes #830
Summary by CodeRabbit