Move page header to the same component as the page sidebar#273
Move page header to the same component as the page sidebar#273BoraIneviNMI wants to merge 14 commits into
Conversation
…itive error boundaries
…e overall farm page
🦋 Changeset detectedLatest commit: 7a08934 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 |
WalkthroughIntroduces HeaderAutomatic and farm/field options store; removes inline header compositions across many routes; centralizes error rendering via InlineErrorBoundary and replaces throw-based error handling with return-based patterns; revises several loaders to drop farm/fertilizer option payloads; adds fertilizers options loader route; updates ErrorBlock UI; minor type exports and Tailwind animation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router as Router (Remix)
participant Loader as Route Loader(s)
participant Store as FarmFieldOptionsStore
participant Header as HeaderAutomatic
participant UI as Page
User->>Router: Navigate to /farm/... route
Router->>Loader: Run loaders for matched routes
Loader-->>Router: Return data (e.g., fieldOptions, fertilizerOptions)
Router->>Store: Optionally set farm/field options (client-side effects)
Router->>Header: Render with route id + merged data
Header-->>UI: Render appropriate header variant
UI-->>User: Page content
sequenceDiagram
autonumber
participant Action as Action/Loader
participant Handler as handleActionError
participant Boundary as InlineErrorBoundary
participant Sentry as Sentry
Action->>Action: try { ... } catch (e)
Action->>Handler: return handleActionError(e)
note right of Action: Return-based error flow
Boundary->>Boundary: Inspect error/status
alt 401 RouteErrorResponse
Boundary-->>Router: Redirect to signin?redirectTo=...
else 4xx/5xx
Boundary->>Sentry: captureException(e) (non-4xx route/generic)
Boundary-->>User: Render ErrorBlock (Card + actions)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
coderabbitai
Bot
added
branch:development
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #273 +/- ##
============================================
Coverage 92.77% 92.77%
============================================
Files 87 87
Lines 13030 13030
Branches 1289 1289
============================================
Hits 12089 12089
Misses 939 939
Partials 2 2
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (30)
fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx (2)
73-81: Remove unused getFarms call (wasted query + unintended hard-fail).You fetch farms only to 404 if empty, but never use the result. This adds latency and can block the page even though the header is now self‑contained.
Apply this diff:
-import { getFarm, getFarms, getFields } from "@svenvw/fdm-core" +import { getFarm, getFields } from "@svenvw/fdm-core" @@ - // Get a list of possible farms of the user - const farms = await getFarms(fdm, session.principal_id) - if (!farms || farms.length === 0) { - throw data("not found: farms", { - status: 404, - statusText: "not found: farms", - }) - } + // Note: farm list not needed here; header handles its own options now.Also applies to: 6-6
101-105: Stabilize asyncData shape to avoid undefined destructuring.When calendar !== "2025" you return {}, but Norms destructures fields unconditionally. Return a stable object with nulls.
- // Currently only 2025 is supported - if (calendar !== "2025") { - return {} - } + // Currently only 2025 is supported + if (calendar !== "2025") { + return { + errorMessage: null, + fieldNorms: null, + farmNorms: null, + } + } @@ - return { - errorMessage: errorMessage, - fieldNorms: fieldNorms, - farmNorms: farmNorms, - } + return { + errorMessage, + fieldNorms, + farmNorms, + }Also applies to: 160-166, 213-215
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsx (2)
229-274: Non-POST paths fall through without a response (405 recommended)If the action receives anything other than POST, it returns
undefined. Add a 405 guard (or implement DELETE as documented).- if (request.method === "POST") { + if (request.method === "POST") { ... return dataWithSuccess( { result: "Data saved successfully" }, "Gewas is bijgewerkt! 🎉", ) - } + } else { + return data("Method Not Allowed", { + status: 405, + statusText: "Use POST", + }) + }
254-265: Updates to variety/residue are ignored unless start/end is setGating the update on only
b_lu_start/b_lu_endskips legitimate updates tom_cropresidueandb_lu_variety.- if (formValues.b_lu_start || formValues.b_lu_end) { + if ( + formValues.b_lu_start || + formValues.b_lu_end || + typeof formValues.m_cropresidue !== "undefined" || + formValues.b_lu_variety + ) {fdm-app/app/routes/farm.$b_id_farm.settings.derogation.tsx (1)
53-56: Validate and bound-check the posted year (trust-boundary input).
Number(formData.get("year"))may yieldNaNor an out-of-policy year; server-side validation prevents accidental writes and clearer errors.Apply:
- const formData = await request.formData() - const year = Number(formData.get("year")) + const formData = await request.formData() + const yearRaw = formData.get("year") + const year = Number(yearRaw) + if (!Number.isInteger(year) || year < EARLIEST_DEROGATION_YEAR || year > LATEST_DEROGATION_YEAR) { + return dataWithError({}, `Ongeldig jaar: ${String(yearRaw)}.`) + }fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.new.upload.tsx (2)
165-173: Bug: falsy check rejects 0 for a_depth_lower; validate numerics once
!soilAnalysis.a_depth_lowerwrongly rejects 0. Align checks witha_depth_upperand parse once to numbers.Apply:
- if (!soilAnalysis.a_depth_lower) { - throw new Error("Missing required a_depth_lower value") - } + const depthLower = Number(soilAnalysis.a_depth_lower) + if (soilAnalysis.a_depth_lower == null || Number.isNaN(depthLower)) { + throw new Error("Missing or invalid a_depth_lower value") + } if (!soilAnalysis.b_sampling_date) { throw new Error("Missing required b_sampling_date") } - if (soilAnalysis.a_depth_upper === undefined || soilAnalysis.a_depth_upper === null) { - throw new Error("Missing required a_depth_upper value") - } + const depthUpper = Number(soilAnalysis.a_depth_upper) + if (soilAnalysis.a_depth_upper == null || Number.isNaN(depthUpper)) { + throw new Error("Missing or invalid a_depth_upper value") + } @@ - Number(soilAnalysis.a_depth_lower), + depthLower, @@ - Number(soilAnalysis.a_depth_upper), + depthUpper,Also applies to: 176-186
123-146: Add upload size limit to prevent memory/DoS issuesYou read the full file into memory. Enforce a sane max (e.g., 10MB) before processing.
Apply:
const uploadHandler = async (fileUpload: FileUpload) => { if ( fileUpload.fieldName === "soilAnalysisFile" && fileUpload.type === "application/pdf" ) { + const MAX_PDF_BYTES = 10 * 1024 * 1024 // 10MB // Check file type based on magic bytes const fileBuffer = await fileUpload.arrayBuffer() + if (fileBuffer.byteLength > MAX_PDF_BYTES) { + throw new Error("File too large") + } const fileType = await fileTypeFromBuffer(fileBuffer)fdm-app/app/routes/farm.$b_id_farm.settings.properties.tsx (2)
283-285: Newline sanitization bug – regex matches the characters “\” and “n”, not actual newlines.Use
\n(and consider\r) and trim the result.Apply this diff:
- const b_address_farm = formValues.b_address_farm?.replace(/\\n/g, " ") + const b_address_farm = formValues.b_address_farm + ? formValues.b_address_farm.replace(/[\r\n]+/g, " ").trim() + : undefined
4-4: Replace incorrect Form import — use Form from "react-router".File: fdm-app/app/routes/farm.$b_id_farm.settings.properties.tsx
-import { Form } from "react-hook-form" +import { Form } from "react-router"fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (3)
141-147: Polygon-only assumption — MultiPolygon shapefiles will break.You cast to
FeatureCollection<Polygon,...>and map coordinates assumingPolygon. RVO/other sources can containMultiPolygon, which will cause runtime errors or invalid geometry.Minimal guard (return a clear warning):
try { shapefile = (await combine([ parseShp(shpBuffer, shxBuffer), parseDbf(dbfBuffer), ])) as FeatureCollection<Polygon, RvoProperties> } catch (_error) { return dataWithWarning({}, "Shapefile is ongeldig.") } + // Only Polygon geometries supported for now + const hasNonPolygon = shapefile.features.some( + (f: any) => f?.geometry?.type !== "Polygon", + ) + if (hasNonPolygon) { + return dataWithWarning( + {}, + "Shapefile bevat niet-Polygon geometrieën (bijv. MultiPolygon). Alleen Polygon wordt ondersteund.", + ) + }If preferred, I can provide a follow-up that expands MultiPolygons into individual Polygon features and imports each.
Also applies to: 158-169
190-207: Falsy checks may reject valid zero values; use nullish/empty checks.Properties like
VOLGNRcould be0, which your current!VOLGNRtest treats as missing.- if ( - !SECTORID || - !SECTORVER || - !NEN3610ID || - !VOLGNR || - !NAAM || - !BEGINDAT || - !EINDDAT || - !GEWASCODE || - !GEWASOMSCH || - !TITEL || - !TITELOMSCH - ) { + if ( + [ + SECTORID, + SECTORVER, + NEN3610ID, + VOLGNR, + NAAM, + BEGINDAT, + EINDDAT, + GEWASCODE, + GEWASOMSCH, + TITEL, + TITELOMSCH, + ].some((v) => v == null || (typeof v === "string" && v.trim() === "")) + ) {
115-120: Remove temporary uploads after parsing — call await fileStorage.remove(storageKey).
LocalFileStorage exposes remove(key); addawait fileStorage.remove(storageKey)after parsing and run it from a finally block (or equivalent) so files are deleted even on errors.
Location: fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (around lines 115–120)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop.harvest.$b_id_harvesting.tsx (3)
216-236: Bug: updates are scoped by the new date, not the original date — can no-op or partially update.When the user changes the harvest date, you filter by the newly submitted date. If no harvests yet exist on that new date, nothing gets updated (including the current record). You should anchor the group by the target harvest’s original date (same approach as your DELETE branch), then update all in that group to the new values/date.
Apply:
- // Update harvests for all cultivations that share the same harvest date for this cultivation - const targetHarvests = cultivation.fields.flatMap( - (field: { - harvests: { - b_id_harvesting: string - b_lu_harvest_date: Date - }[] - }) => { - return field.harvests.filter( - (harvest: { - b_id_harvesting: string - b_lu_harvest_date: Date - }) => { - return ( - harvest.b_lu_harvest_date.getTime() === - b_lu_harvest_date.getTime() - ) - }, - ) - }, - ) + // Determine original anchor date from the selected harvest, then update all linked ones + const harvestsForCultivation = + cultivation.fields.flatMap( + (field: { + harvests: { + b_id_harvesting: string + b_lu_harvest_date: Date + }[] + }) => field.harvests, + ) || [] + + const anchorHarvest = harvestsForCultivation.find( + (h) => h.b_id_harvesting === b_id_harvesting, + ) + if (!anchorHarvest) { + throw new Error("Target harvest not found") + } + const originalHarvestDate = anchorHarvest.b_lu_harvest_date + + const targetHarvests = harvestsForCultivation.filter( + (h) => + h.b_lu_harvest_date.getTime() === + originalHarvestDate.getTime(), + )
241-249: Use the normalized date when calling updateHarvest.Currently the call passes
b_lu_harvest_datedirectly. After normalizing (see below), useharvestDatefor consistency.- await updateHarvest( + await updateHarvest( fdm, session.principal_id, targetHarvest.b_id_harvesting, - b_lu_harvest_date, + harvestDate, b_lu_yield, b_lu_n_harvestable, )
199-201: Normalizeb_lu_harvest_dateto a Date to avoid.getTime()on strings.Depending on
FormSchema, this may arrive as a string. Normalize once and reuse.- const { b_lu_yield, b_lu_n_harvestable, b_lu_harvest_date } = - formValues + const { b_lu_yield, b_lu_n_harvestable, b_lu_harvest_date } = + formValues + const harvestDate = + b_lu_harvest_date instanceof Date + ? b_lu_harvest_date + : new Date(b_lu_harvest_date)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.tsx (1)
191-213: No 405 for unsupported methods.If the method is neither POST nor DELETE, the action returns
undefined. Return a 405.Apply:
if (request.method === "DELETE") { const formData = await request.formData() const b_lu = formData.get("b_lu") @@ return dataWithSuccess( { result: "Data deleted successfully" }, { message: "Gewas is verwijderd" } ) } + // Fallback for unsupported methods + return new Response("Method Not Allowed", { + status: 405, + headers: { Allow: "POST, DELETE" }, + })Also applies to: 230-233
fdm-app/app/routes/farm.$b_id_farm.fertilizers.new.custom.tsx (2)
9-14: Prefer json over data; align with RR/Remix helpers.Use
jsonfor 4xx responses and dropdata. This also simplifies downstream error handling.-import { - type ActionFunctionArgs, - data, - type LoaderFunctionArgs, - type MetaFunction, - useLoaderData, -} from "react-router" +import { + type ActionFunctionArgs, + json, + type LoaderFunctionArgs, + type MetaFunction, + useLoaderData, +} from "react-router"
41-46: Avoid double-handling thrown Responses in loader; pass-through Response.Throw a
jsonResponse for the param check and rethrow native Responses in catch to preserve status/body (important for localized boundaries).- if (!b_id_farm) { - throw data("invalid: b_id_farm", { - status: 400, - statusText: "invalid: b_id_farm", - }) - } + if (!b_id_farm) { + throw json("invalid: b_id_farm", { + status: 400, + statusText: "invalid: b_id_farm", + }) + }- } catch (error) { - throw handleLoaderError(error) - } + } catch (error) { + if (error instanceof Response) throw error + throw handleLoaderError(error) + }Also applies to: 109-111
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.harvest.$b_id_harvesting.tsx (1)
196-201: Add a 405 fallback to avoid undefined returns on unsupported methods.If a non-POST/DELETE request reaches the action, it currently falls through with no return.
Apply this diff:
// Get the action from the form + // Only allow supported methods; otherwise return 405 + if (request.method !== "POST" && request.method !== "DELETE") { + return data("Method Not Allowed", { + status: 405, + statusText: "Method Not Allowed", + }) + } if (request.method === "POST") {fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id._index.tsx (1)
492-511: POST flow can return undefined when no cultivations existIf cultivations is empty, the function reaches the end of the try without returning a response.
const cultivations = await getCultivations( fdm, session.principal_id, b_id, timeframe, ) - if (cultivations && cultivations.length > 0) { + if (cultivations && cultivations.length > 0) { await updateCultivation( fdm, session.principal_id, cultivations[0].b_lu, formValues.b_lu_catalogue, undefined, undefined, ) return dataWithSuccess("fields have been updated", { message: `${formValues.b_name} is bijgewerkt! 🎉`, }) - } + } + // No cultivations present — still return success for field update + return dataWithSuccess("field has been updated", { + message: `${formValues.b_name} is bijgewerkt! 🎉`, + })fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx (2)
78-79: Missing 404 when field not foundAdd a 404 like other routes to avoid downstream undefined usages.
- const field = await getField(fdm, session.principal_id, b_id) + const field = await getField(fdm, session.principal_id, b_id) + if (!field) { + throw data("Field not found", { + status: 404, + statusText: "Field not found", + }) + }
108-110: Don’t throw handleLoaderError inside asyncDatahandleLoaderError returns a Response; throwing it here ends up as “[object Response]” in your errorMessage path. Throw a normal Error and let the local catch produce the message.
- if (!cultivations.length) { - throw handleLoaderError("missing: cultivations") - } + if (!cultivations.length) { + throw new Error("Missing cultivations") + }fdm-app/app/routes/farm.create._index.tsx (1)
82-84: Hard-coded year 2025 will break flows in 2026Use the current year dynamically in validation, option lists, and derogation year range.
- .max(2025, { - message: "Startjaar mag maximaal 2025 zijn", - }) + .max(new Date().getFullYear(), { + message: `Startjaar mag maximaal ${new Date().getFullYear()} zijn`, + }) @@ - {Array.from( - { - length: - 2025 - + {Array.from( + { + length: + new Date().getFullYear() - 2006 + 1, }, @@ - const years = Array.from( - { length: 2025 - derogation_start_year + 1 }, - (_, i) => derogation_start_year + i, - ) + const currentYear = new Date().getFullYear() + const years = Array.from( + { length: currentYear - derogation_start_year + 1 }, + (_, i) => derogation_start_year + i, + )Also applies to: 227-230, 375-378
fdm-app/app/components/blocks/header/fertilizer.tsx (2)
62-79: Use the correct dropdown item type; avoid checkbox semantics for navigation.
DropdownMenuCheckboxItemimplies toggle state and expects onCheckedChange; here we only navigate. PreferDropdownMenuItem(or a radio group if you want selection semantics).- {fertilizerOptions.map((option) => ( - <DropdownMenuCheckboxItem - checked={p_id === option.p_id} - key={option.p_id} - > + {fertilizerOptions.map((option) => ( + <DropdownMenuItem key={option.p_id}> <NavLinkIf you need single‑selection semantics, switch to
DropdownMenuRadioGroup/DropdownMenuRadioItem.
67-74: Fragile path replacement.
currentPath.replace(p_id, option.p_id)can replace unintended substrings. Build the route explicitly.- p_id - ? currentPath.replace( - p_id, - option.p_id, - ) + p_id + ? `/farm/${b_id_farm}/fertilizers/${option.p_id}` : `/farm/${b_id_farm}/fertilizers/${option.p_id}/`Also consider normalizing trailing slashes.
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fertilizers.$b_lu_catalogue.tsx (2)
141-148: Prop mismatch:fertilizerOptionsis passed but never returned from loader.This will be
undefined(or a TS error). Either stop passing it or return it from the loader.Apply this diff to return options from the loader and wire them through:
@@ const fertilizers = await getFertilizers( fdm, session.principal_id, b_id_farm, ) + const fertilizerOptions = fertilizers.map((f) => ({ + p_id: f.p_id, + p_name_nl: f.p_name_nl ?? null, + })) @@ return { b_lu_catalogue: b_lu_catalogue, b_id_farm: b_id_farm, fertilizerApplications: fertilizerApplications, dose: dose.dose, applicationMethodOptions: applicationMethods.options, + fertilizerOptions, } @@ <FertilizerApplicationCard fertilizerApplications={loaderData.fertilizerApplications} applicationMethodOptions={loaderData.applicationMethodOptions} fertilizerOptions={loaderData.fertilizerOptions} dose={loaderData.dose} />
97-103: Date comparison may crash ifp_app_dateis a string.Guard by coercing to Date before calling
getTime().- app1.p_app_date.getTime() === app2.p_app_date.getTime() + new Date(app1.p_app_date).getTime() === + new Date(app2.p_app_date).getTime()fdm-app/app/routes/farm.$b_id_farm.fertilizers.$p_id.tsx (2)
71-73: Guard against missing fertilizer to avoid runtime crash in defaultValues.If
getFertilizerreturns null/undefined, the subsequentdefaultValuesaccess will throw. Add a 404 guard.- const fertilizer = await getFertilizer(fdm, p_id) - const fertilizerParameters = getFertilizerParametersDescription() + const fertilizer = await getFertilizer(fdm, p_id) + if (!fertilizer) { + throw data("not found: p_id", { status: 404, statusText: "not found: p_id" }) + } + const fertilizerParameters = getFertilizerParametersDescription()Optionally harden
defaultValuesfor nullable fields to avoid controlled/uncontrolled warnings if the API returnsnull:- p_name_nl: fertilizer.p_name_nl, + p_name_nl: fertilizer.p_name_nl ?? "", - p_density: fertilizer.p_density, + p_density: fertilizer.p_density ?? undefined, # apply similar `??` fallbacks only where the API can return nullAlso applies to: 101-151
194-203: Guard against missingp_id_cataloguebefore update.If a farm-sourced fertilizer lacks a catalogue link, the update may fail server-side. Fail fast with a clear error.
- const p_id_catalogue = fertilizer.p_id_catalogue + const p_id_catalogue = fertilizer.p_id_catalogue + if (!p_id_catalogue) { + throw new Error("missing: p_id_catalogue") + }fdm-app/app/routes/farm.$b_id_farm.fertilizers.new.$p_id.tsx (1)
69-72: Guard against missing fertilizer to avoid runtime crash in form defaults.If
getFertilizerreturns null/undefined,defaultValuesdereferences will throw. Add a 404 when not found.- const fertilizer = await getFertilizer(fdm, p_id) - const fertilizerParameters = getFertilizerParametersDescription() + const fertilizer = await getFertilizer(fdm, p_id) + if (!fertilizer) { + throw data("not found: p_id", { + status: 404, + statusText: "not found: p_id", + }) + } + const fertilizerParameters = getFertilizerParametersDescription()
| import { type UIMatch, useLocation, useMatches, useParams } from "react-router" | ||
| import { useCalendarStore } from "@/app/store/calendar" | ||
| import { useFarmFieldOptionsStore } from "@/app/store/farm-field-options" | ||
| import type { FertilizerOption } from "../farm/farm" |
There was a problem hiding this comment.
Fix fertilizer options typing/import and align with HeaderFertilizer.
- The import
FertilizerOptionpath looks suspicious and the localfertilizerOptionsis typed asunknown[], butHeaderFertilizerexpects a concrete option shape. This will either break TS or mask bugs.
Apply this diff to type things correctly (assuming HeaderFertilizerOption is exported from ./fertilizer; if not, export it there or declare a local type with p_id/p_name_nl):
-import type { FertilizerOption } from "../farm/farm"
+import type { HeaderFertilizerOption } from "./fertilizer"
@@
- let fertilizerOptions: unknown[] | undefined
+ let fertilizerOptions: HeaderFertilizerOption[] | undefined
@@
- fertilizerOptions={
+ fertilizerOptions={
/\/new(\/|$)/.test(location.pathname)
? []
: fertilizerOptions
}If HeaderFertilizerOption is not exported yet, add this to fdm-app/app/components/blocks/header/fertilizer.tsx:
+export type HeaderFertilizerOption = {
+ p_id: string
+ p_name_nl: string | undefined | null
+}Also applies to: 32-33, 114-122
🤖 Prompt for AI Agents
In fdm-app/app/components/blocks/header/automatic.tsx around lines 5 (and
likewise address lines 32-33 and 114-122), the import and typing for fertilizer
options are incorrect: replace the loose/unknown[] typing and the suspicious
import path with the concrete HeaderFertilizerOption type (imported from
./fertilizer); if HeaderFertilizerOption is not exported from
fdm-app/app/components/blocks/header/fertilizer.tsx, export it there (or declare
a local type with properties p_id and p_name_nl) and then update the import at
the top of automatic.tsx to import { HeaderFertilizerOption } from
"./fertilizer" and change fertilizerOptions' type from unknown[] to
HeaderFertilizerOption[] and ensure all usages match that shape so
HeaderFertilizer receives the correct typed options.
| if (error.status === 401) { | ||
| // Get the current path the user tried to access | ||
| const currentPath = | ||
| location.pathname + location.search + location.hash | ||
| // Construct the sign-in URL with the redirectTo parameter | ||
| const signInUrl = `./signin?redirectTo=${encodeURIComponent(currentPath)}` | ||
| // Throw the redirect response to be caught by React Router | ||
| throw redirect(signInUrl) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
401 redirect path should be absolute; relative “./signin” may 404 under nested routes.
Use /signin and consider navigation instead of throwing a redirect from a component.
- const signInUrl = `./signin?redirectTo=${encodeURIComponent(currentPath)}`
+ const signInUrl = `/signin?redirectTo=${encodeURIComponent(currentPath)}`
- // Throw the redirect response to be caught by React Router
- throw redirect(signInUrl)
+ // Prefer client navigation from components; throwing redirect is for loaders/actions.
+ // If you want to keep throw-behavior, verify it works with RRv7 in component boundaries.
+ throw redirect(signInUrl)Please verify the behavior with RR v7. If throws from component boundaries don’t navigate, switch to useNavigate:
// inside InlineErrorBoundary
const navigate = useNavigate()
useEffect(() => {
if (isRouteErrorResponse(error) && error.status === 401) {
const currentPath = location.pathname + location.search + location.hash
navigate(`/signin?redirectTo=${encodeURIComponent(currentPath)}`, { replace: true })
}
}, [error, location, navigate])
return nullUse absolute /signin URL and client‐side navigation in InlineErrorBoundary
Replace the relative ./signin with /signin to avoid nested‐route resolution errors(docs4dev.com), and handle 401 redirects in the component using useNavigate (rather than throwing redirect(), which is intended for loaders/actions)(reactrouter.com).
🤖 Prompt for AI Agents
In fdm-app/app/components/custom/inline-error-boundary.tsx around lines 27 to
35, the code currently throws a redirect to a relative "./signin" which can
resolve incorrectly in nested routes and uses redirect() (meant for
loaders/actions) instead of client navigation; change the URL to an absolute
"/signin" and replace the thrown redirect with client-side navigation via the
useNavigate hook (call
navigate(`/signin?redirectTo=${encodeURIComponent(currentPath)}`) when
error.status === 401), ensuring you call the hook at component scope and perform
the navigate side effect (and then return/stop rendering the error UI as
appropriate).
| } catch (error) { | ||
| throw handleActionError(error) | ||
| return handleActionError(error) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Throw from loader catch; returning breaks error boundaries and crashes the UI
Returning handleActionError(error) causes the route component to render with an error-shaped payload where loaderData.field is expected, leading to runtime failures in getViewState(fields) and MapGL props. To preserve ErrorBoundary flow (and match existing loader semantics), throw the response instead.
- return handleActionError(error)
+ throw handleActionError(error)If the new pattern truly requires returning structured errors, then the component must guard before using loaderData.field and render an inline error state. Verify ~/lib/error.handleActionError contract and repo-wide usage:
🏁 Script executed:
#!/bin/bash
# 1) Inspect implementation
rg -nC2 'export (async )?function handleActionError|export const handleActionError' --type=ts
# 2) Where is it thrown vs returned?
rg -nC1 'handleActionError\(' --type=ts | sed -n 's/.*:\([0-9]\+\):.*/\1/p' >/dev/null
rg -nP 'throw\s+handleActionError\(' --type=ts
rg -nP 'return\s+handleActionError\(' --type=ts
# 3) Check for error guards in this route component
rg -n 'InlineErrorBoundary|useRouteError|isRouteErrorResponse|useActionData' fdm-app/app/routes/farm.\$b_id_farm.\$calendar.field.\$b_id.atlas.tsx -nLength of output: 5271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) All occurrences of handleActionError:"
rg -n --hidden -S "handleActionError" || true
echo
echo "2) Look for explicit definitions (function/const/let/export default):"
rg -n --hidden -S "^[[:space:]]*(export )?(const|let|function)[[:space:]]+handleActionError" || true
rg -n --hidden -S "export default .*handleActionError" || true
echo
echo "3) If no explicit def found, list unique files that reference it:"
rg -n --hidden -S "handleActionError" | awk -F: '{print $1}' | sort -u > /tmp/ha_files || true
printf "%s\n" "$(cat /tmp/ha_files)" || true
echo
echo "4) Print the first 200 lines of each file that references handleActionError (to find its implementation/contract):"
while read -r f; do
echo "---- $f ----"
sed -n '1,200p' "$f" || true
done < /tmp/ha_files
echo
echo "5) Show the atlas route file around the relevant catches:"
atlas='fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx'
if [ -f "$atlas" ]; then
rg -n "handleActionError" "$atlas" || true
sed -n '1,260p' "$atlas"
else
echo "File not found: $atlas"
fiLength of output: 264280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the error handling file
error_file=$(rg -l "handleActionError" -g "fdm-app/lib/error.ts")
echo "Error file: $error_file"
# Print the implementation of handleActionError
sed -n '1,200p' "$error_file"Length of output: 148
Throw from loader catch; returning breaks error boundaries and crashes the UI
Returning handleActionError(error) in a loader places an error-shaped payload into loaderData (bypassing the route ErrorBoundary) — throw the response so the ErrorBoundary/loader semantics are preserved.
- return handleActionError(error)
+ throw handleActionError(error)Affected: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx (loader catch). Repo scan shows multiple occurrences of "return handleActionError(...)" — audit loaders and replace returns with throws where the loader should trigger the ErrorBoundary. Verify the ~/lib/error.handleActionError contract and repo-wide usage before mass changes.
📝 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) { | |
| throw handleActionError(error) | |
| return handleActionError(error) | |
| } | |
| } catch (error) { | |
| throw handleActionError(error) | |
| } |
🤖 Prompt for AI Agents
In fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx around
lines 90-92, the loader catch currently returns handleActionError(error) which
injects an error-shaped value into loaderData and bypasses the route
ErrorBoundary; change the catch to throw the response from
handleActionError(error) (i.e., throw await or throw the returned
Response/object per handleActionError contract) so the ErrorBoundary/loader
semantics are preserved. After fixing this instance, audit other loader
functions across the repo for "return handleActionError(" and replace returns
with throws where the loader is expected to trigger ErrorBoundary, and confirm
the exact return/throw shape by checking ~/lib/error.handleActionError to ensure
callers throw the correct Response/error type.
| }) | ||
| } catch (error) { | ||
| throw handleActionError(error) | ||
| return handleActionError(error) |
There was a problem hiding this comment.
🧩 Analysis chain
Returning handleActionError from the action is the right move—verify consumer and payload shape.
Good shift from throwing to returning; this enables inline form errors instead of tripping the route boundary. Please verify that:
handleActionErrorreturns serializable action data (not a thrownResponse) with appropriate status codes (400/422/500).SoilAnalysisFormon this route readsuseActionDataand surfaces these errors.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate handleActionError definition:"
rg -nP -C2 '(^export\s+(?:async\s+)?function\s+handleActionError\b)|(\bconst\s+handleActionError\s*=\s*\()' --type ts --type tsx
echo "Call sites of handleActionError:"
rg -nP -C2 '\bhandleActionError\s*\(' --type ts --type tsx -g '!**/node_modules/**'
echo "Confirm SoilAnalysisForm consumes action data:"
rg -nP -C3 'export (?:const|function) SoilAnalysisForm' --type tsx
rg -nP -C3 '\buseActionData\b' --type tsxLength of output: 64
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for handleActionError definition (ts/tsx files):"
rg -nP -C2 '(^export\s+(?:async\s+)?function\s+handleActionError\b)|(\bconst\s+handleActionError\s*=\s*\()' -g '!**/node_modules/**' -g '*.ts' -g '*.tsx' || true
echo "Searching for all call sites of handleActionError (ts/tsx):"
rg -nP -C2 '\bhandleActionError\s*\(' -g '!**/node_modules/**' -g '*.ts' -g '*.tsx' || true
echo "Searching for SoilAnalysisForm component definitions (tsx):"
rg -nP -C3 'export (?:const|function) SoilAnalysisForm' -g '!**/node_modules/**' -g '*.tsx' || true
echo "Searching for useActionData usages (tsx):"
rg -nP -C3 '\buseActionData\b' -g '!**/node_modules/**' -g '*.tsx' || true
file='fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.$a_id.tsx'
if [ -f "$file" ]; then
echo "Printing lines 195-215 from $file:"
sed -n '195,215p' "$file"
else
echo "File not found: $file"
fiLength of output: 27519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate all occurrences of handleActionError, dataWithError, redirectWithSuccess:"
rg -nP '\b(handleActionError|dataWithError|redirectWithSuccess)\b' -g '!**/node_modules/**' || true
echo; echo "Search for handleActionError definition (function/const/export):"
rg -nP '(^export\s+(?:async\s+)?function\s+handleActionError\b)|(\b(?:const|let|var)\s+handleActionError\s*=\s*\()|(\bexport\s+const\s+handleActionError\b)' -g '!**/node_modules/**' || true
echo; echo "Search for dataWithError / redirectWithSuccess definitions:"
rg -nP '(^export\s+(?:async\s+)?function\s+(dataWithError|redirectWithSuccess)\b)|(\b(?:const|let|var)\s+(dataWithError|redirectWithSuccess)\s*=\s*\()|(\bexport\s+const\s+(dataWithError|redirectWithSuccess)\b)' -g '!**/node_modules/**' || true
echo; echo "List helper files that reference these helpers:"
rg -n --hidden -S 'handleActionError|dataWithError|redirectWithSuccess' -g '!**/node_modules/**' -l || true
echo; echo "Print likely helper files (first 240 lines) to inspect return shapes:"
for f in $(rg -nP 'handleActionError|dataWithError|redirectWithSuccess' -g '!**/node_modules/**' -l || true); do
echo "---- $f ----"
sed -n '1,240p' "$f" || true
echo
done
echo "Print SoilAnalysisForm (component) for useActionData consumption:"
sed -n '1,240p' fdm-app/app/components/blocks/soil/form.tsx || echo "file not found"
echo
echo "Also print soil form-upload (already known to use useActionData) for reference:"
sed -n '1,240p' fdm-app/app/components/blocks/soil/form-upload.tsx || echo "file not found"Length of output: 27906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files containing 'handleActionError':"
rg -n --hidden -S "handleActionError" -g '!**/node_modules/**' -l || true
echo
echo "Print top of files that mention 'handleActionError' to find its definition (first 400 lines):"
for f in $(rg -n --hidden -S "handleActionError" -g '!**/node_modules/**' -l || true); do
echo "---- $f ----"
sed -n '1,400p' "$f" || true
echo
done
echo "Files containing 'dataWithError' or 'redirectWithSuccess':"
rg -n --hidden -S "dataWithError|redirectWithSuccess" -g '!**/node_modules/**' -l || true
echo
echo "Print top of files that mention 'dataWithError' or 'redirectWithSuccess' (first 400 lines):"
for f in $(rg -n --hidden -S "dataWithError|redirectWithSuccess" -g '!**/node_modules/**' -l || true); do
echo "---- $f ----"
sed -n '1,400p' "$f" || true
echo
done
route='fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.$a_id.tsx'
echo "Print full route file: $route"
if [ -f "$route" ]; then
sed -n '1,400p' "$route"
else
echo "File not found: $route"
fiLength of output: 7572
Action returns handleActionError — confirm payload and make the form consume it
- SoilAnalysisForm (fdm-app/app/components/blocks/soil/form.tsx) does not call useActionData, so returning handleActionError(error) from the action will not surface inline form errors.
- Ensure handleActionError returns a serializable action payload (example used elsewhere: { message?: string; fieldErrors?: Record<string,string[]>; formErrors?: string[] }) and uses appropriate status codes (400/422 for validation, 500 for server).
- Remediate by either updating SoilAnalysisForm to consume useActionData and map fieldErrors → form.setError / FormMessage, or by changing the action to throw/return a Response that matches the intended UX.
Files: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.$a_id.tsx (action), fdm-app/app/components/blocks/soil/form.tsx (consumer), fdm-app/app/components/blocks/soil/form-upload.tsx (example).
🤖 Prompt for AI Agents
In
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.$a_id.tsx
around line 205, the action currently returns handleActionError(error) but the
SoilAnalysisForm does not call useActionData so inline form errors never
surface; update the action to return a serializable payload shaped like {
message?: string; fieldErrors?: Record<string, string[]>; formErrors?: string[]
} and use appropriate status codes (400/422 for validation, 500 for server) OR
throw/return a Response with that JSON and status; then update
fdm-app/app/components/blocks/soil/form.tsx to call useActionData(), map
fieldErrors to form.setError or FormMessage (similar to form-upload.tsx), and
ensure the action and consumer agree on the payload shape so validation errors
display inline.
| const fertilizerOptions = fertilizers.map((fertilizer) => { | ||
| return { | ||
| p_id: fertilizer.p_id, | ||
| p_name_nl: fertilizer.p_name_nl || "", | ||
| } | ||
| }) |
There was a problem hiding this comment.
Prevent empty labels: don’t coerce missing names to empty string.
Mapping p_name_nl || "" suppresses your header’s fallback label. Use nullish coalescing and keep null/undefined.
- p_name_nl: fertilizer.p_name_nl || "",
+ p_name_nl: fertilizer.p_name_nl ?? null,📝 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.
| const fertilizerOptions = fertilizers.map((fertilizer) => { | |
| return { | |
| p_id: fertilizer.p_id, | |
| p_name_nl: fertilizer.p_name_nl || "", | |
| } | |
| }) | |
| const fertilizerOptions = fertilizers.map((fertilizer) => { | |
| return { | |
| p_id: fertilizer.p_id, | |
| p_name_nl: fertilizer.p_name_nl ?? null, | |
| } | |
| }) |
🤖 Prompt for AI Agents
In fdm-app/app/routes/farm.$b_id_farm.fertilizers.tsx around lines 28-33 the
mapping coerces missing fertilizer names to an empty string using `p_name_nl ||
""`, which hides the header’s fallback; remove the `|| ""` and preserve
null/undefined (use `p_name_nl` or `p_name_nl ?? undefined`) so the consumer can
trigger its fallback label, and update the mapped type/interface if necessary to
allow `string | undefined`/`null`.
| export async function loader({ request }: LoaderFunctionArgs) { | ||
| try { | ||
| // Get the farm id | ||
| const b_id_farm = params.b_id_farm | ||
| if (!b_id_farm) { | ||
| throw data("Farm ID is required", { | ||
| status: 400, | ||
| statusText: "Farm ID is required", | ||
| }) | ||
| } | ||
|
|
||
| // Get the session | ||
| const session = await getSession(request) | ||
|
|
||
| // Return the farm ID and session info | ||
| return { | ||
| farmId: b_id_farm, | ||
| session, | ||
| } | ||
| await getSession(request) | ||
| } catch (error) { | ||
| throw handleActionError(error) | ||
| return handleActionError(error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Throw loader errors instead of returning, and use the loader error helper.
Returning the error response prevents the ErrorBoundary flow (401 redirect logic won’t trigger). Use handleLoaderError and throw.
-import type { LoaderFunctionArgs, MetaFunction } from "react-router"
+import type { LoaderFunctionArgs, MetaFunction } from "react-router"
@@
-import { handleActionError } from "~/lib/error"
+import { handleLoaderError } from "~/lib/error"
@@
export async function loader({ request }: LoaderFunctionArgs) {
try {
// Get the session
await getSession(request)
} catch (error) {
- return handleActionError(error)
+ throw handleLoaderError(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.
| export async function loader({ request }: LoaderFunctionArgs) { | |
| try { | |
| // Get the farm id | |
| const b_id_farm = params.b_id_farm | |
| if (!b_id_farm) { | |
| throw data("Farm ID is required", { | |
| status: 400, | |
| statusText: "Farm ID is required", | |
| }) | |
| } | |
| // Get the session | |
| const session = await getSession(request) | |
| // Return the farm ID and session info | |
| return { | |
| farmId: b_id_farm, | |
| session, | |
| } | |
| await getSession(request) | |
| } catch (error) { | |
| throw handleActionError(error) | |
| return handleActionError(error) | |
| } | |
| } | |
| export async function loader({ request }: LoaderFunctionArgs) { | |
| try { | |
| // Get the session | |
| await getSession(request) | |
| } catch (error) { | |
| throw handleLoaderError(error) | |
| } | |
| } |
🤖 Prompt for AI Agents
In fdm-app/app/routes/farm.$b_id_farm.tsx around lines 31 to 38, the loader
currently catches errors and returns the result of handleActionError which
prevents the ErrorBoundary/loader flow (e.g., 401 redirect) from running;
replace the return with throwing the loader error helper by calling throw
handleLoaderError(error) inside the catch so the Remix loader error handling
runs; ensure the loader still returns the expected success value (e.g., the
session) on the happy path and import/use handleLoaderError instead of
handleActionError.
| import { Separator } from "~/components/ui/separator" | ||
| import { getSession } from "~/lib/auth.server" | ||
| import { handleLoaderError } from "~/lib/error" | ||
| import { handleActionError } from "~/lib/error" |
There was a problem hiding this comment.
Loader uses action error helper and inspects non-existent Response.init
handleActionError is for actions; Response doesn’t expose init. This path likely never redirects and may mask real errors.
-import { handleActionError } from "~/lib/error"
+import { handleLoaderError } from "~/lib/error"
@@
- } catch (error) {
- const response = await handleActionError(error)
- if (response.init) {
- return redirect(
- `/farm/create/${params.b_id_farm}/${params.calendar}/fields/${params.b_id}`,
- response.init,
- )
- }
- return response
- }
+ } catch (error) {
+ throw handleLoaderError(error)
+ }Also applies to: 58-66
| const response = await handleActionError(error) | ||
| if (response.init) { | ||
| return redirect( | ||
| `/farm/create/${params.b_id_farm}/${params.calendar}/fields/${params.b_id}/soil/analysis`, | ||
| response.init, | ||
| ) | ||
| } | ||
| return response | ||
| } |
There was a problem hiding this comment.
Use handleLoaderError in loader catch; current redirect-on-error path is brittle.
handleActionError is intended for actions; checking response.init is non-standard and likely incorrect. Prefer throwing loader-shaped errors so ErrorBoundaries handle them consistently.
Apply:
-import {
- type ActionFunctionArgs,
- data,
- type LoaderFunctionArgs,
- NavLink,
- redirect,
- useLoaderData,
-} from "react-router"
+import {
+ type ActionFunctionArgs,
+ data,
+ type LoaderFunctionArgs,
+ NavLink,
+ useLoaderData,
+} from "react-router"
@@
-import { handleActionError } from "~/lib/error"
+import { handleActionError, handleLoaderError } from "~/lib/error"
@@
- } catch (error) {
- const response = await handleActionError(error)
- if (response.init) {
- return redirect(
- `/farm/create/${params.b_id_farm}/${params.calendar}/fields/${params.b_id}/soil/analysis`,
- response.init,
- )
- }
- return response
- }
+ } catch (error) {
+ throw handleLoaderError(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.
| const response = await handleActionError(error) | |
| if (response.init) { | |
| return redirect( | |
| `/farm/create/${params.b_id_farm}/${params.calendar}/fields/${params.b_id}/soil/analysis`, | |
| response.init, | |
| ) | |
| } | |
| return response | |
| } | |
| import { | |
| type ActionFunctionArgs, | |
| data, | |
| type LoaderFunctionArgs, | |
| NavLink, | |
| useLoaderData, | |
| } from "react-router" | |
| import { handleActionError, handleLoaderError } from "~/lib/error" | |
| ... | |
| } catch (error) { | |
| throw handleLoaderError(error) | |
| } |
🤖 Prompt for AI Agents
In
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id.soil.analysis.$analysis_type.tsx
around lines 98 to 106, the catch currently calls handleActionError and inspects
response.init then redirects which is brittle; replace this with using
handleLoaderError (or throwing a loader-shaped error) so the loader error
boundary handles it consistently: call handleLoaderError(error) (or transform
the caught error into the loader error shape it expects) and then throw or
return its result instead of inspecting response.init and performing a manual
redirect.
2 participants
Enhancements
Summary by CodeRabbit
New Features
UX/Style
Refactor
Chores