Implement PDF export for Bemestingsplan#425
Conversation
🦋 Changeset detectedLatest commit: a9b466d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a full Bemestingsplan PDF export: new React-PDF components, styles and types; server loader that gathers farm/ and field-level data and computes norms/advice/planned doses; client UI to download the PDF; dependency and Docker asset inclusion. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant Client as Farm Dashboard (Client)
participant Server as PDF Route Loader
participant DB as Data Sources (fdm-core / integrations)
participant PDF as BemestingsplanPDF
User->>Client: Click "Download bemestingsplan"
Client->>Client: set isGeneratingPdf = true
Client->>Server: GET /farm/:b_id_farm/:calendar/bemestingsplan.pdf
Server->>DB: Fetch farm, fields, cultivations, soil, applications, fertilizers
Server->>Server: Compute norms, norm fillings, nutrient advice, planned doses per field
Server->>Server: Aggregate farm-level totals and OM balance
Server->>Server: Resolve public assets -> data URIs
Server->>PDF: Render BemestingsplanPDF with assembled data
PDF->>Server: Stream PDF bytes
Server->>Client: Stream PDF response (blob)
Client->>Client: Create temporary link and trigger download
Client->>User: Save file and show success toast
Client->>Client: set isGeneratingPdf = false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #425 +/- ##
============================================
Coverage 88.07% 88.07%
============================================
Files 91 91
Lines 4621 4621
Branches 1492 1492
============================================
Hits 4070 4070
Misses 551 551
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
changed the title
coderabbitai
Bot
added
branch:development
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx`:
- Around line 433-444: In the BemestingsplanPDF React component (look for the
Text block that contains the Dutch disclaimer and references data.config.name),
fix the two typos where the English word "for" is used: change "for informatieve
doeleinden" to "voor informatieve doeleinden" and "for de naleving" to "voor de
naleving" so the disclaimer is correct Dutch; update the strings directly inside
the Text JSX where the current Dutch paragraph is rendered.
- Around line 295-333: The Table of Contents in BemestingsplanPDF.tsx is using
hardcoded page numbers inside the Link/Text blocks (see Link and Text usages for
"#farm-summary", "#fields-overview", "#fertilizer-totals"), which will become
incorrect as content changes; remove the hardcoded numeric <Text> nodes or
replace them with either empty placeholders or an indicator like "pagina
(approx.)" and/or a short note that numbers are approximate, and ensure the Link
components keep only the section labels (e.g., "Samenvatting bedrijf",
"Overzicht percelen", "Benodigde meststoffen") so navigation anchors remain but
no stale page numbers are shown.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsx:
- Around line 265-291: The current mapping in the applications mapping block
assumes index alignment between applications and dosesResult.applications (using
idx), which can misalign data; instead, look up the corresponding dose by a
stable key (e.g., p_id or p_app_id) when building each application object — use
fertilizers.find as an example and create a doses lookup (map by p_id or the
application id present in dosesResult.applications) and then pull p_dose_*
values from that lookup, falling back to 0 when no matching dose exists; update
the code in the applications: applications.map(...) block to replace
dosesResult.applications[idx] with a safe lookup into that map.
- Around line 484-492: The Content-Disposition header builds a filename from
farm.b_name_farm which may contain non-ASCII or special characters; update the
response to emit both a safe ASCII filename and an RFC 5987 encoded filename*
parameter: generate a sanitized fallback (e.g., replace quotes/whitespace and
strip/normalize accents) for filename, produce a UTF-8 percent-encoded value for
filename* (using encodeURIComponent-like RFC5987 encoding), and set the header
to include both "filename" and "filename*" so Content-Disposition uses the
encoded name; adjust the code that constructs the header where new Response(...)
sets "Content-Disposition" and reference farm.b_name_farm, b_id_farm and
calendar when creating these two filename variants.
- Around line 444-457: The path construction currently assumes a top-level
"fdm-app" directory by using path.resolve(process.cwd(), "fdm-app", "public")
which breaks in Docker (WORKDIR is already /app/fdm-app) and also fails if
public isn't copied into the image; change the publicDir resolution to point to
the runtime app public folder (e.g., path.resolve(process.cwd(), "public") or
derive the app root dynamically) and update usages of publicDir, logoPath,
logoInverted and clientConfig.logo to use that corrected path; additionally add
a graceful fallback that checks file existence (fs.existsSync) and continues
without a logo if missing, and/or ensure the Docker build copies the public
assets into the image so logos are available in production.
🧹 Nitpick comments (5)
fdm-app/app/components/blocks/pdf/PdfBadge.tsx (1)
4-14: Consider stronger typing for thestyleprop.The
anytype forstylereduces type safety. The@react-pdf/rendererpackage exports aStyletype that could be used here. However, I note this matches the pattern used in other PDF components (PdfCard, PdfTable) in this PR.♻️ Optional type improvement
+import type { Style } from "@react-pdf/renderer" import { Text, View } from "@react-pdf/renderer" import { pdfStyles } from "./bemestingsplan/styles" export const PdfBadge = ({ children, style, }: { children: string - style?: any + style?: Style }) => ( <View style={[pdfStyles.badge, style]}> <Text>{children}</Text> </View> )fdm-app/app/components/blocks/pdf/PdfCard.tsx (1)
4-10: Consider stronger typing for the style prop.Using
anyfor the style prop bypasses type checking. While react-pdf styles can be complex, you could improve type safety by using the library's type.♻️ Suggested improvement
+import type { Style } from "@react-pdf/types" import { View } from "@react-pdf/renderer" import { pdfStyles } from "./bemestingsplan/styles" export const PdfCard = ({ children, style, }: { children: React.ReactNode - style?: any + style?: Style }) => <View style={[pdfStyles.card, style]}>{children}</View>fdm-app/app/components/blocks/pdf/bemestingsplan/styles.ts (1)
160-172: Hardcoded footer positioning may cause layout issues.The
top: 780value assumes a specific page height (A4 = 842pt). This absolute positioning could cause the footer to overlap with content on pages with more data, or appear incorrectly if page size changes.Consider using
bottompositioning instead for more reliable footer placement.♻️ Suggested fix
footer: { position: "absolute", - top: 780, + bottom: 20, left: 40, right: 40, flexDirection: "row", justifyContent: "space-between", borderTopWidth: 1, borderTopColor: "#e2e8f0", paddingTop: 10, fontSize: 8, color: "#64748b", },fdm-app/app/routes/farm.$b_id_farm._index.tsx (1)
132-164: Improve error handling and sanitize filename.Two concerns with the download handler:
- The error handling discards the server's error details which would help debugging.
- The filename could contain special characters from the farm name that may cause issues.
♻️ Suggested improvements
const handleDownloadPdf = async (e: React.MouseEvent) => { e.preventDefault() if (isGeneratingPdf) return setIsGeneratingPdf(true) toast.info("Bemestingsplan wordt gegenereerd", { description: "Dit kan enkele seconden duren...", }) try { const response = await fetch( `/farm/${loaderData.b_id_farm}/${calendar}/bemestingsplan.pdf`, ) - if (!response.ok) throw new Error("Download failed") + if (!response.ok) { + const errorText = await response.text().catch(() => "") + throw new Error(`Download failed: ${response.status} ${errorText}`) + } const blob = await response.blob() const url = window.URL.createObjectURL(blob) const a = document.createElement("a") a.href = url - a.download = `Bemestingsplan_${loaderData.b_name_farm}_${calendar}.pdf` + // Sanitize filename by removing/replacing problematic characters + const safeFarmName = loaderData.b_name_farm.replace(/[<>:"/\\|?*]/g, "_") + a.download = `Bemestingsplan_${safeFarmName}_${calendar}.pdf` document.body.appendChild(a) a.click() window.URL.revokeObjectURL(url) document.body.removeChild(a) toast.success("Download voltooid") } catch (error) { console.error(error) toast.error("Er ging iets mis bij het genereren van de PDF") } finally { setIsGeneratingPdf(false) } }fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx (1)
86-101: External URL creates unnecessary availability risk; consider bundling a local image asset for resilience.Unsplash images are licensed for use in generated PDFs without attribution or permission requirements. However, relying on an external URL introduces an availability dependency—if the image becomes unavailable or the connection fails, PDF generation could be impacted. Bundling a local asset eliminates this external dependency.
…nPDF.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx`:
- Around line 87-90: The Image component is using a hardcoded external Unsplash
URL which can break PDF generation; update the component (BemestingsplanPDF ->
the Image usage) to use a local bundled asset or a configurable image from
data.config (e.g., use data.config.bannerImage if provided, otherwise fall back
to a static local import/asset served from the app). Ensure the chosen image is
bundled or served from your app (import the file or reference /public) so PDF
generation doesn't rely on external HTTP, and keep the same style props
(width/height/objectFit) when replacing the src.
- Around line 1623-1631: The "p_dose_eoc" plannedKey used in BemestingsplanPDF
(the nutrient entry with key "om" / label "Organische koolstof (EOC)") does not
exist on field.planned per types.d.ts, causing field.planned["p_dose_eoc"] to be
undefined and planned values to show 0; fix by either (A) adding p_dose_eoc to
the field.planned type in types.d.ts and ensuring the data loader populates
field.planned.p_dose_eoc for each field, or (B) remove/omit the EOC entry from
the secondary nutrients list in BemestingsplanPDF (or switch it to use a
farm-level aggregate property) so you no longer access a non-existent key;
update any code referencing plannedKey "p_dose_eoc" and corresponding UI
rendering logic accordingly.
♻️ Duplicate comments (1)
fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx (1)
295-333: Table of Contents has hardcoded page numbers.This issue was previously flagged. The hardcoded page numbers (3, 4, 3) and the formula-based field page numbers (
5 + ion line 369) won't adjust dynamically as content changes.
🧹 Nitpick comments (2)
fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx (2)
668-820: Consider extracting crop overview logic to a separate component.The IIFE computing
productiveFields,bufferFields,getCrops, and renderingrenderCropListis complex. Extracting this to a dedicatedCropOverviewcomponent would improve readability and testability.
1016-1170: Potentially redundant Footer rendering.The
Footercomponent hasfixedpositioning (renders at absolute position on every page). Having<Footer />at both line 1017 and line 1169 may cause duplicate rendering. Other pages in this document only include a single Footer.♻️ Remove redundant Footer
</PdfTable> - <Footer config={data.config} /> </Page>
coderabbitai
Bot
added
dependencies
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx`:
- Around line 1202-1203: Remove the duplicated Footer render inside the Page
start: in BemestingsplanPDF remove the earlier <Footer config={data.config} />
that sits immediately after <Page key={field.id} size="A4"
style={pdfStyles.page}> so only the existing fixed Footer (the one intended to
be rendered across pages) remains; locate usages of the Footer component within
the Page block and delete the initial/duplicate instance while keeping the fixed
Footer at the end.
- Around line 1042-1043: The Footer component is rendered twice in
BemestingsplanPDF (inside the Page with pdfStyles.page and again later), causing
duplicate fixed-position footers; remove the earlier Footer invocation located
immediately after <Page size="A4" orientation="landscape"
style={pdfStyles.page}> so only the single Footer rendered after the content
remains, and verify no other duplicate Footer calls exist in the
BemestingsplanPDF component.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsx:
- Around line 172-176: The return type of getNutrientAdviceForField must be
explicit so callers can access properties safely; update its signature in
integrations/calculator.ts to return Promise<ReturnType<typeof
getNutrientAdvice>> (or the specific exported interface from
`@svenvw/fdm-calculator` if available) instead of an implicit any, then adjust the
implementation to respect that type; lastly remove the defensive any casts and
fallback chain in the caller (where yearAdvice is computed) and access the typed
fields directly (e.g., result.data.year or result.year) relying on the declared
return type.
🧹 Nitpick comments (5)
fdm-app/app/components/blocks/pdf/bemestingsplan/styles.ts (2)
63-63: Remove leftover development comment.The comment
// ... rest of frontInfoappears to be a leftover from development and should be removed for cleaner code.♻️ Suggested fix
frontFarmName: { fontSize: 24, fontWeight: "bold", marginBottom: 8, color: "#FFFFFF", }, - // ... rest of frontInfo - frontInfo: {
161-173: Hardcoded footer position assumes A4 dimensions.The
top: 780value is hardcoded based on A4 page height (~842pt) minus margins. This works for portrait A4 but will break for landscape orientation used in the Fields Overview page.Consider using
bottompositioning instead oftopfor more robust placement across different page orientations.♻️ Suggested fix
footer: { position: "absolute", - top: 780, + bottom: 20, left: 40, right: 40, flexDirection: "row", justifyContent: "space-between", borderTopWidth: 1, borderTopColor: "#e2e8f0", paddingTop: 10, fontSize: 8, color: "#64748b", },fdm-app/app/routes/farm.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsx (1)
73-361: Consider chunking for very large farms.For farms with 300+ fields (as mentioned in requirements), executing all field data gathering in a single
Promise.allcould cause memory pressure. The current per-field error handling is good, but consider adding chunking for very large datasets.Would you like me to generate a chunked implementation that processes fields in batches?
fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx (2)
23-40: Redundant condition check in Footer component.The
showPageNumbersis checked at line 23 to return the entire footer, but then checked again at line 30 for the page number text. Since line 23 already guards the entire block, the inner check at line 30 is redundant.♻️ Suggested fix
const Footer = ({ config, style, showPageNumbers = true, }: { config: { name: string } style?: any showPageNumbers?: boolean }) => { - if (showPageNumbers) { - return ( - <View style={[pdfStyles.footer, style]} fixed> - <Text> - {config.name} - Gegenereerd op{" "} - {format(new Date(), "d MMMM yyyy", { locale: nl })} - </Text> - {showPageNumbers && ( - <Text - render={({ pageNumber, totalPages }) => - `Pagina ${pageNumber} / ${totalPages}` - } - /> - )} - </View> - ) - } - return null + return ( + <View style={[pdfStyles.footer, style]} fixed> + <Text> + {config.name} - Gegenereerd op{" "} + {format(new Date(), "d MMMM yyyy", { locale: nl })} + </Text> + {showPageNumbers && ( + <Text + render={({ pageNumber, totalPages }) => + `Pagina ${pageNumber} / ${totalPages}` + } + /> + )} + </View> + ) }
140-147: Remove commented-out logo code.This commented-out code block should either be removed or properly implemented. Leaving commented code in production reduces readability.
♻️ Suggested fix
<View style={{ marginBottom: 40, alignItems: "center" }}> - {/* {data.config.logo ? ( - <Image src={data.config.logo} style={pdfStyles.frontLogo} /> - ) : data.config.logoInverted ? ( - <Image - src={data.config.logoInverted} - style={pdfStyles.frontLogo} - /> - ) : null} */} + {data.config.logoInverted && ( + <Image + src={data.config.logoInverted} + style={pdfStyles.frontLogo} + /> + )} <Text
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsx:
- Around line 164-167: Remove the redundant type assertion on b_centroid: since
field is typed as Field (returned from getFields) and Field.b_centroid is
already [number, number], delete the "as [number, number]" cast in the object
literal where b_centroid is assigned (the code referencing b_centroid for the
field variable). Ensure the property remains simply b_centroid: field.b_centroid
and that no other callers rely on that cast.
🧹 Nitpick comments (2)
fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx (2)
140-147: Remove commented-out code or add a TODO explaining its purpose.This commented-out logo rendering block appears to be dead code. If it's intended for future use, add a TODO comment explaining when/why it should be enabled. Otherwise, remove it to keep the codebase clean.
694-846: Consider extracting the crop aggregation logic into a helper function.The IIFE containing crop aggregation logic (
getCrops,renderCropList) spans 150+ lines inside JSX. While functional, extracting this to a separate component or helper would improve readability and testability.
…pdf.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsx:
- Around line 539-555: The ASCII fallback filename in the Content-Disposition
header is inconsistent with the encoded filename; update the `filename` value to
include the same "Bemestingsplan_${safeName}_${calendar}.pdf" prefix used when
building `encodedName` so both `safeName`/`encodedName` align; locate the header
construction around `safeName`, `encodedName` and the Response creation and
replace the current `filename="${safeName}.pdf"` with a filename string that
matches the encoded name pattern (using `safeName` and `calendar`).
- Around line 356-452: The farm OM average currently divides totalOmBalance by
totalArea, which treats missing f.omBalance as zero; update the aggregation to
compute omArea = sum of f.area for fields where f.omBalance exists (using
pdfFieldsData), use omArea as the divisor and in the conditional (replace checks
against totalArea for OM averaging), and keep totalOmBalance accumulation logic
in totalOmBalance; adjust farmOmBalance to use omArea > 0 ? totalOmBalance.* /
omArea : 0 so only fields with omBalance contribute to the per-hectare averages.
…pdf.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
There was a problem hiding this comment.
It seems to work fine and looks great. I have tested some things and here is what I found:
- The PDF renders good on the latest iOS.
- It handles the calendar year properly.
- If some error is thrown when an error occurs, the section that has given an error and other sections might show 0 instead of showing an error. The teeltplan shows an error but it might not be clear that it applies to the rest of the field's section.
- Line numbers include the cover page. This is probably fine.
2 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Closes #413