Implement Client-Side Caching for Calculations#287
Conversation
🦋 Changeset detectedLatest commit: 59832c1 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 client-side caching for calculator results (norms, nitrogen balance, nutrient advice) using object-hash and persistent Zustand stores, wires cache-aware loaders/components and client middleware, updates some breadcrumb NavLink usage, and exposes PUBLIC_FDM_CALCULATOR_VERSION from an async Vite config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser
participant Router
participant Loader as Route Loader
participant Server as Calculation Service
participant Cache as Zustand Cache
User->>Browser: Navigate to /.../route?cacheHash=Hc
Browser->>Router: dispatch route
Router->>Loader: call loader({ request })
Loader->>Server: compute inputHash Hi (hash inputs)
alt Hc == Hi
Loader-->>Router: return { useCache: true, inputHash: Hi }
else
Loader->>Server: run calculation -> result R
Server-->>Loader: return R
Loader-->>Router: return { result: R, inputHash: Hi }
end
Router->>Cache: get cached[scope][id] -> Rc
alt useCache == true and Rc exists
Router-->>Browser: render Rc (cached)
Browser->>Router: client clears `cacheHash` from URL
else
Router-->>Browser: render R (fresh)
Browser->>Cache: set cached[scope][id] = { data: R, inputHash: Hi }
end
sequenceDiagram
autonumber
actor User
participant Router
participant ClientMW as Client Middleware
participant Cache as Cache Store
User->>Router: Open /farm/:id/calendar/...
Router->>ClientMW: run clientMiddleware()
alt route is balance root
ClientMW-->>Router: redirect to /.../balance/nitrogen
else route matches cache predicate
ClientMW->>Cache: get cached inputHash Hi for scope
alt cached exists and cacheHash != Hi
ClientMW-->>Router: redirect to same URL with cacheHash=Hi
else
ClientMW-->>Router: continue navigation
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 #287 +/- ##
============================================
Coverage 93.07% 93.07%
============================================
Files 81 81
Lines 13118 13118
Branches 1326 1326
============================================
Hits 12209 12209
Misses 907 907
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 can you help me with getting the npm package version for fdm-calculator or the entire project in createCache() in calculation-cache.ts |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 17724 @BoraIneviNMI, I can help you access the npm package version in ImplementationSince import { create } from "zustand"
import { persist } from "zustand/middleware"
import packageJson from "../../package.json"
export interface DataWithInputHash {
inputHash?: string | undefined
}
export interface CacheStore<T extends DataWithInputHash> {
db: Record<string, T>
get: (id: string) => T | undefined
set: (id: string, val: T) => void
}
function createCache<T extends DataWithInputHash>(name: string) {
return create(
persist<CacheStore<T>>(
(_set, _get) => ({
db: {},
get: (id) => _get().db[id],
set: (id, val) => _set({ db: { ..._get().db, [id]: val } }),
}),
{
name,
version: packageJson.version, // This will be "0.23.0"
},
),
)
}
export const useFarmNormsCache = createCache("farm-norms-cache")
export const useFarmNitrogenBalanceCache = createCache(
"farm-nitrogen-balance-cache",
)
export const useFieldNitrogenBalanceCache = createCache(
"field-nitrogen-balance-cache",
)How it works
TypeScript ConfigurationMake sure your {
"compilerOptions": {
"resolveJsonModule": true,
"esModuleInterop": true
}
}This approach ensures that every time you release a new version of fdm-app, all cached calculations will be automatically invalidated, which aligns perfectly with your PR objectives! |
|
@coderabbitai what if only fdm-calculator version is bumped? It can alter calculation results even if fdm-app is unchanged. Does fdm-app get bumped too when fdm-calculator gets bumped, since fdm-app depends on it? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6183 🏁 Script executed: Length of output: 3405 @BoraIneviNMI, excellent question! You've identified a critical issue with the caching strategy. The ProblemLooking at the versions:
The project uses changesets for independent package versioning. This means This means if we only use The SolutionUse fdm-calculator's version for cache invalidation instead, since it's the actual engine performing the calculations: import { create } from "zustand"
import { persist } from "zustand/middleware"
import calculatorPackage from "@svenvw/fdm-calculator/package.json"
export interface DataWithInputHash {
inputHash?: string | undefined
}
export interface CacheStore<T extends DataWithInputHash> {
db: Record<string, T>
get: (id: string) => T | undefined
set: (id: string, val: T) => void
}
function createCache<T extends DataWithInputHash>(name: string) {
return create(
persist<CacheStore<T>>(
(_set, _get) => ({
db: {},
get: (id) => _get().db[id],
set: (id, val) => _set({ db: { ..._get().db, [id]: val } }),
}),
{
name,
version: calculatorPackage.version, // Currently "0.7.0"
},
),
)
}
export const useFarmNormsCache = createCache("farm-norms-cache")
export const useFarmNitrogenBalanceCache = createCache(
"farm-nitrogen-balance-cache",
)
export const useFieldNitrogenBalanceCache = createCache(
"field-nitrogen-balance-cache",
)Why This Works
This ensures calculation results are always fresh when the calculation engine changes, regardless of whether 🧠 Learnings used |
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
176-189: Prevent crash when cacheHash exists but cache is emptyDirectly visited URLs can carry
cacheHash. On a fresh browser (or SSR) there’s no persisted entry yet, socachedDataisundefinedwhiledata.useCache === true. The ternary falls back todata, leavingresolvedNitrogenBalanceResultundefined, and the very next use (resolvedNitrogenBalanceResult.fields) throws at runtime. Guard this case (e.g. detectdata.useCache && !cachedData, clear the cacheHash and re-run the loader, or have the loader fall back to recomputing) before destructuring so the page can render instead of crashing.
🧹 Nitpick comments (4)
fdm-app/package.json (1)
55-55: Pin object-hash for deterministic cache keysHash output is part of your cache key contract. Pin to 3.0.0 to avoid accidental changes.
- "object-hash": "^3.0.0", + "object-hash": "3.0.0",If you rely on a lockfile for determinism across environments, you may keep the caret. Otherwise, pin.
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.tsx (1)
125-149: Redirect picks arbitrary cached field; consider MRU selectionUsing Object.keys(db)[0] is arbitrary. Prefer the most recently used/updated field id for a better UX.
- Track last-used field id in the cache store and use it here.
- Fallback to any key when MRU is absent.
Also verify the persist key for this cache: calculation-cache.ts shows
"field-norms-cache"for useFieldNutrientAdviceCache — that likely collides with norms. Use a distinct key (e.g., "field-nutrient-advice-cache"). Based on learningsfdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx (2)
80-82: Minor: include inputHash in the short‑circuit pathReturning
{ useCache: true }omitsinputHash. Including it can simplify client logic/telemetry without recomputation.- if (inputHash === cacheHash) { - return { useCache: true } - } + if (inputHash === cacheHash) { + return { useCache: true, inputHash } + }
234-247: Add asyncData to useEffect deps and destructure set for stability
- Move the
const asyncData = …declaration above the effect and includeasyncDatain its dependency array.- Destructure
setfromuseFieldNutrientAdviceCache()and useset(field.b_id, asyncData)instead offieldNutrientAdviceCache.set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
fdm-app/app/lib/middleware.tsis excluded by!fdm-app/app/lib/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.changeset/rotten-rocks-argue.md(1 hunks)fdm-app/app/components/blocks/header/balance.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance._index.tsx(0 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx(6 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx(6 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx(6 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx(6 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.tsx(3 hunks)fdm-app/app/store/calculation-cache.ts(1 hunks)fdm-app/package.json(2 hunks)fdm-app/vite.config.ts(3 hunks)
💤 Files with no reviewable changes (1)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.balance._index.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-25T12:42:32.783Z
Learnt from: SvenVw
PR: SvenVw/fdm#6
File: fdm-app/vite.config.ts:5-9
Timestamp: 2024-11-25T12:42:32.783Z
Learning: In the `fdm-app` project, SvenVw is preparing for migration to Remix v3 and may include type declarations or configurations for v3 features in advance, such as in `vite.config.ts`.
Applied to files:
fdm-app/vite.config.ts
📚 Learning: 2024-12-16T10:56:07.561Z
Learnt from: SvenVw
PR: SvenVw/fdm#16
File: fdm-app/app/routes/app.addfarm.$b_id_farm.cultivations.$b_lu_catalogue.fertilizers.tsx:1-1
Timestamp: 2024-12-16T10:56:07.561Z
Learning: The project uses `react-router` v7, and the `data` function is exported and used for error handling in loaders and actions.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.tsx
📚 Learning: 2025-01-14T16:06:24.294Z
Learnt from: SvenVw
PR: SvenVw/fdm#45
File: fdm-app/app/routes/farm.$b_id_farm._index.tsx:1-1
Timestamp: 2025-01-14T16:06:24.294Z
Learning: In the fdm-app codebase, the `redirect` function should be imported from `react-router`, not `react-router-dom`.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.tsx
📚 Learning: 2025-09-23T10:02:32.123Z
Learnt from: BoraIneviNMI
PR: SvenVw/fdm#272
File: fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fertilizers.$b_lu_catalogue.manage.$p_id.tsx:151-164
Timestamp: 2025-09-23T10:02:32.123Z
Learning: The getFertilizer function from svenvw/fdm-core throws an exception if the fertilizer doesn't exist, rather than returning null or undefined.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx
📚 Learning: 2025-09-23T12:37:58.711Z
Learnt from: SvenVw
PR: SvenVw/fdm#274
File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsx:113-148
Timestamp: 2025-09-23T12:37:58.711Z
Learning: In the FDM application, the current field data fetching implementation using Promise.all with individual API calls (getCultivations, getFertilizerApplications, getCurrentSoilData) performs acceptably even with farms containing 90+ fields. No performance issues have been observed in practice with this approach.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx
🧬 Code graph analysis (6)
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.tsx (3)
fdm-app/app/store/calculation-cache.ts (1)
useFieldNutrientAdviceCache(37-37)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.tsx (1)
clientMiddleware(35-50)fdm-app/app/lib/middleware.ts (1)
splatCacheMiddleware(14-47)
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx (1)
fdm-app/app/store/calculation-cache.ts (1)
useFieldNutrientAdviceCache(37-37)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.tsx (5)
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.tsx (2)
loader(49-123)clientMiddleware(151-160)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
loader(55-121)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (1)
loader(57-136)fdm-app/app/lib/middleware.ts (1)
splatCacheMiddleware(14-47)fdm-app/app/store/calculation-cache.ts (2)
useFarmNitrogenBalanceCache(31-33)useFieldNitrogenBalanceCache(34-36)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (2)
fdm-app/app/store/field-filter.ts (1)
useFieldFilterStore(10-24)fdm-app/app/store/calculation-cache.ts (1)
useFarmNitrogenBalanceCache(31-33)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (2)
fdm-app/app/store/calendar.tsx (1)
useCalendarStore(11-15)fdm-app/app/store/calculation-cache.ts (1)
useFieldNitrogenBalanceCache(34-36)
fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx (3)
fdm-calculator/src/norms/index.ts (1)
createFunctionsForNorms(7-21)fdm-app/app/store/calculation-cache.ts (1)
useFarmNormsCache(30-30)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.tsx (1)
clientMiddleware(35-50)
🪛 Biome (2.1.2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx
[error] 234-234: This hook does not specify its dependency on asyncData.
This dependency is being used here, but is not specified in the hook dependency list.
Unsafe fix: Add the missing dependency to the list.
(lint/correctness/useExhaustiveDependencies)
🔇 Additional comments (5)
fdm-app/app/components/blocks/header/balance.tsx (2)
33-37: Well-implemented client-side navigation with asChild pattern.The change correctly uses the
asChildprop to composeBreadcrumbLinkwithNavLink, enabling React Router's client-side navigation while preserving the breadcrumb's semantic structure and styling. This pattern is the recommended approach for integrating React Router with Radix UI/shadcn components.
34-34: Calendar state never undefined Initialized to current year and setter falls back to it, socalendaris always defined.fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.tsx (1)
151-160: Regex guard looks correct for field-level routeThe splatCacheMiddleware matcher //nutrient_advice/.+/?$/ correctly targets the field variant.
fdm-app/package.json (1)
97-97: No changes needed for @types/object-hash import
Version ^3.0.6 is published and esModuleInterop is enabled, soimport hash from "object-hash"works as expected.fdm-app/app/store/calculation-cache.ts (1)
14-27: Make persist storage SSR-safe
persistdefaults tolocalStorage; during SSR (all loaders import this module)localStorageis undefined and the store creation throws. Mirror the approach inuseFieldFilterStore: importcreateJSONStorageandssrSafeJSONStorage, and passstorage: createJSONStorage(() => ssrSafeJSONStorage)(or an equivalent noop storage) so the cache initializes safely on the server.-import { persist } from "zustand/middleware" +import { createJSONStorage, persist } from "zustand/middleware" +import { ssrSafeJSONStorage } from "~/lib/ssr-safe-storage" @@ persist<CacheStore<T>>( (_set, _get) => ({ db: {}, get: (id) => _get().db[id], set: (id, val) => _set({ db: { ..._get().db, [id]: val } }), }), { name, version: `fdm-calculator:${PUBLIC_FDM_CALCULATOR_VERSION}`, + storage: createJSONStorage(() => ssrSafeJSONStorage), }, ), )⛔ Skipped due to learnings
Learnt from: SvenVw PR: SvenVw/fdm#272 File: fdm-app/app/store/field-fertilizer-form.tsx:45-47 Timestamp: 2025-09-25T15:10:59.708Z Learning: In the FDM application, Zustand stores with persist middleware using sessionStorage/localStorage don't require SSR hardening guards. The existing store patterns in fdm-app work without typeof window checks or memory storage fallbacks, as evidenced by the changelog store using createJSONStorage(() => localStorage) directly.Learnt from: SvenVw PR: SvenVw/fdm#272 File: fdm-app/app/store/field-fertilizer-form.tsx:45-47 Timestamp: 2025-09-25T15:10:59.708Z Learning: In the FDM application, Zustand stores with persist middleware using sessionStorage/localStorage don't require SSR hardening guards. The existing store patterns in fdm-app work without typeof window checks or memory storage fallbacks.
| export default defineConfig(async (config) => { | ||
| // We need to go one directory up since package.json is not inside the dist folder | ||
| const fdmCalculatorPackageJsonPath = new URL( | ||
| "../package.json", | ||
| import.meta.resolve("@svenvw/fdm-calculator"), | ||
| ) | ||
| const fdmCalculatorPackage = JSON.parse( | ||
| await fs.readFile(fdmCalculatorPackageJsonPath, { encoding: "utf-8" }), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Guard package.json resolution/read with fallback
A failure in import.meta.resolve/readFile will crash the build. Wrap in try/catch and fall back to a default string.
-export default defineConfig(async (config) => {
- // We need to go one directory up since package.json is not inside the dist folder
- const fdmCalculatorPackageJsonPath = new URL(
- "../package.json",
- import.meta.resolve("@svenvw/fdm-calculator"),
- )
- const fdmCalculatorPackage = JSON.parse(
- await fs.readFile(fdmCalculatorPackageJsonPath, { encoding: "utf-8" }),
- )
+export default defineConfig(async (config) => {
+ // Resolve calculator version from its package.json; fall back safely
+ let calculatorVersion = "0.7.0"
+ try {
+ const pkgUrl = new URL(
+ "../package.json",
+ import.meta.resolve("@svenvw/fdm-calculator"),
+ )
+ const pkgJson = JSON.parse(
+ await fs.readFile(pkgUrl, { encoding: "utf-8" }),
+ )
+ if (typeof pkgJson?.version === "string") calculatorVersion = pkgJson.version
+ } catch {
+ // keep default fallback
+ }📝 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 default defineConfig(async (config) => { | |
| // We need to go one directory up since package.json is not inside the dist folder | |
| const fdmCalculatorPackageJsonPath = new URL( | |
| "../package.json", | |
| import.meta.resolve("@svenvw/fdm-calculator"), | |
| ) | |
| const fdmCalculatorPackage = JSON.parse( | |
| await fs.readFile(fdmCalculatorPackageJsonPath, { encoding: "utf-8" }), | |
| ) | |
| export default defineConfig(async (config) => { | |
| // Resolve calculator version from its package.json; fall back safely | |
| let calculatorVersion = "0.7.0" | |
| try { | |
| const pkgUrl = new URL( | |
| "../package.json", | |
| import.meta.resolve("@svenvw/fdm-calculator"), | |
| ) | |
| const pkgJson = JSON.parse( | |
| await fs.readFile(pkgUrl, { encoding: "utf-8" }), | |
| ) | |
| if (typeof pkgJson?.version === "string") calculatorVersion = pkgJson.version | |
| } catch { | |
| // keep default fallback | |
| } | |
| // …rest of your config, now using calculatorVersion… |
🤖 Prompt for AI Agents
fdm-app/vite.config.ts around lines 12 to 21: the import.meta.resolve and
fs.readFile calls can throw and currently will crash the build; wrap the
resolution and file-read in a try/catch, and on error assign a safe fallback
(e.g. a default package JSON string/object or "{}" parsed) to
fdmCalculatorPackage so the build continues; ensure both import.meta.resolve and
the await fs.readFile are inside the try block and that any caught error is
silently handled or logged, then parse/use the fallback value.
| PUBLIC_FDM_CALCULATOR_VERSION: JSON.stringify( | ||
| fdmCalculatorPackage.version || "0.7.0", | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Prefer defining an env var instead of a bare global
If app code reads import.meta.env.PUBLIC_FDM_CALCULATOR_VERSION, define that key to avoid TS/global typing issues.
- PUBLIC_FDM_CALCULATOR_VERSION: JSON.stringify(
- fdmCalculatorPackage.version || "0.7.0",
- ),
+ "import.meta.env.PUBLIC_FDM_CALCULATOR_VERSION": JSON.stringify(
+ calculatorVersion
+ ),Please confirm the code references this as import.meta.env.PUBLIC_FDM_CALCULATOR_VERSION (not a bare identifier). If it uses the bare name, keep both defines for a transitional period.
🏁 Script executed:
#!/bin/bash
# Find usages of the calculator version in app code
rg -n -C2 'PUBLIC_FDM_CALCULATOR_VERSION|import\.meta\.env\.PUBLIC_FDM_CALCULATOR_VERSION' --type=ts --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
rg -n -C2 'PUBLIC_FDM_CALCULATOR_VERSION|import\.meta\.env\.PUBLIC_FDM_CALCULATOR_VERSION' -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx'Length of output: 653
Preserve both env var definitions for transition
The app code in fdm-app/app/store/calculation-cache.ts (line 24) uses the bare PUBLIC_FDM_CALCULATOR_VERSION. To avoid runtime errors, retain the existing PUBLIC_FDM_CALCULATOR_VERSION define alongside the new import.meta.env.PUBLIC_FDM_CALCULATOR_VERSION (or update all references to use import.meta.env.PUBLIC_FDM_CALCULATOR_VERSION).
🤖 Prompt for AI Agents
In fdm-app/vite.config.ts around lines 49 to 51, preserve backward compatibility
by defining both PUBLIC_FDM_CALCULATOR_VERSION and
import.meta.env.PUBLIC_FDM_CALCULATOR_VERSION in the Vite define map so existing
code that references PUBLIC_FDM_CALCULATOR_VERSION continues to work while new
code can use import.meta.env.PUBLIC_FDM_CALCULATOR_VERSION; set both to the same
value (e.g., JSON.stringify(fdmCalculatorPackage.version || "0.7.0")).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx (1)
369-375: Stale cacheHash guard handled correctly.This addresses the earlier concern about
{ useCache: true }without a local cache by removing the hash and short‑circuiting the render.fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (1)
189-195: Resolved: handle{ useCache: true }without cached payload.Removing
cacheHashand returning null avoids the incorrect “invalid field” branch. This addresses the earlier concern.
🧹 Nitpick comments (6)
fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx (3)
153-158: Pinobject-hashoptions for stability (algorithm/encoding, optional unordered).Defaults can change across versions; make hashes explicit and consistent across routes.
Based on learnings
- const inputs = await Promise.all(inputPromises) - inputHash = hash(inputs) + const inputs = await Promise.all(inputPromises) + inputHash = hash(inputs, { algorithm: "sha1", encoding: "hex" }) if (inputHash === cacheHash) { return { useCache: true } }
159-166: Destructurefieldonly after error check.Avoid reading from potentially undefined shapes and reduce confusion.
- const fieldNormPromises = inputs.map(async (input) => { - const { field } = input - if (input.errorMessage) { - return input - } + const fieldNormPromises = inputs.map(async (input) => { + if (input.errorMessage) { + return input + } + const { field } = input
276-310: Client middleware looks good; consider reusing the shared helper.Logic is solid and fixes stale cacheHash. Prefer
splatCacheMiddlewarefromapp/lib/middleware.tsfor consistency/DRY if it fits this route’s shape.fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx (1)
128-136: Make hash deterministic by setting options explicitly.Set
algorithmandencoding(and keep consistent across routes).Based on learnings
- const inputHash = hash([ + const inputHash = hash([ resolvedCurrentSoilData, resolvedFertilizerApplications, resolvedFertilizers, b_lu_catalogue, - ]) + ], { algorithm: "sha1", encoding: "hex" })fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
96-100: Explicitly configureobject-hashfor consistent cache keys.Align with other routes; set algorithm/encoding.
Based on learnings
- const inputHash = hash(nitrogenBalanceInput) + const inputHash = hash(nitrogenBalanceInput, { + algorithm: "sha1", + encoding: "hex", + })fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (1)
109-113: Stabilize hash function options.Pin
algorithmandencodingto avoid future default changes.Based on learnings
- const inputHash = hash(input) + const inputHash = hash(input, { algorithm: "sha1", encoding: "hex" }) if (inputHash === cacheHash) { return { useCache: true } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx(6 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx(6 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx(6 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-23T12:37:58.711Z
Learnt from: SvenVw
PR: SvenVw/fdm#274
File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsx:113-148
Timestamp: 2025-09-23T12:37:58.711Z
Learning: In the FDM application, the current field data fetching implementation using Promise.all with individual API calls (getCultivations, getFertilizerApplications, getCurrentSoilData) performs acceptably even with farms containing 90+ fields. No performance issues have been observed in practice with this approach.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx
🧬 Code graph analysis (4)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
fdm-app/app/store/calculation-cache.ts (1)
useFarmNitrogenBalanceCache(31-33)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (2)
fdm-app/app/store/calendar.tsx (1)
useCalendarStore(11-15)fdm-app/app/store/calculation-cache.ts (1)
useFieldNitrogenBalanceCache(34-36)
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx (1)
fdm-app/app/store/calculation-cache.ts (1)
useFieldNutrientAdviceCache(37-37)
fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx (3)
fdm-calculator/src/norms/index.ts (1)
createFunctionsForNorms(7-21)fdm-app/app/store/calculation-cache.ts (1)
useFarmNormsCache(30-30)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.tsx (1)
clientMiddleware(34-49)
🪛 Biome (2.1.2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx
[error] 236-236: This hook does not specify its dependency on asyncData.
This dependency is being used here, but is not specified in the hook dependency list.
Unsafe fix: Add the missing dependency to the list.
(lint/correctness/useExhaustiveDependencies)
🔇 Additional comments (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx (1)
251-261: Good defensive handling of stale cacheHash.Clearing the param and short‑circuiting avoids rendering with incomplete data.
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
178-185: Stale cacheHash guard is correct.Clears the query param and avoids rendering stale state.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx (2)
81-82: Caching logic is sound; consider SHA-256 for future-proofing.The implementation correctly extracts
cacheHashfrom the URL, computes aninputHashfrom the resolved input data, and performs an early return when they match, avoiding redundant calculations. This pattern effectively leverages client-side caching to improve performance.Optional enhancement: The
object-hashlibrary defaults to SHA-1. While SHA-1 is acceptable for cache invalidation (non-cryptographic use), you could specify SHA-256 for better collision resistance and future-proofing:const inputHash = hash([ resolvedCurrentSoilData, resolvedFertilizerApplications, resolvedFertilizers, b_lu_catalogue, -]) +], { algorithm: 'sha256' })Based on learnings.
Also applies to: 128-136, 156-156
263-265: Type cast is pragmatic but could be more explicit.The type cast
as typeof serverAsyncDataassumescachedDatahas a compatible shape with the full server response. While this works in practice (the cache stores previously received server data), TypeScript can't verify this structural compatibility at compile time.For better type safety, consider defining explicit union types for the different response shapes:
type AsyncDataResponse = | { useCache: true } | { nutrientAdvice: ..., doses: ..., inputHash: string, errorMessage?: undefined } | { errorMessage: string } // Then in the loader: const asyncData: Promise<AsyncDataResponse> = (async () => { ... })() // And in the component: const asyncData: AsyncDataResponse = ( serverAsyncData.useCache && cachedData ? cachedData : serverAsyncData )This makes the different response shapes explicit and allows TypeScript to catch shape mismatches.
fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx (1)
136-151: Consider whether error messages should affect the inputHash.The current implementation includes the entire
inputobject (includingerrorMessagewhen errors occur) in theinputHashcomputation at line 154. This means that if the same field fails with a slightly different error message on subsequent visits, theinputHashwill differ and the cache will be invalidated, even though the underlying field data might be identical.If error messages are deterministic and stable (e.g., always "Missing soil type"), this is fine. However, if they include timestamps, request IDs, or other volatile information, consider excluding them from the hash computation or normalizing errors before hashing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fdm-app/app/components/blocks/header/nutrient-advice.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx(6 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-23T12:37:58.711Z
Learnt from: SvenVw
PR: SvenVw/fdm#274
File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsx:113-148
Timestamp: 2025-09-23T12:37:58.711Z
Learning: In the FDM application, the current field data fetching implementation using Promise.all with individual API calls (getCultivations, getFertilizerApplications, getCurrentSoilData) performs acceptably even with farms containing 90+ fields. No performance issues have been observed in practice with this approach.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx
🧬 Code graph analysis (3)
fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx (1)
fdm-app/app/store/calculation-cache.ts (1)
useFieldNutrientAdviceCache(37-37)
fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx (3)
fdm-calculator/src/norms/index.ts (1)
createFunctionsForNorms(7-21)fdm-app/app/store/calculation-cache.ts (1)
useFarmNormsCache(30-30)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.tsx (1)
clientMiddleware(34-49)
fdm-app/app/components/blocks/header/nutrient-advice.tsx (1)
fdm-app/app/components/ui/breadcrumb.tsx (1)
BreadcrumbLink(111-111)
🔇 Additional comments (7)
fdm-app/app/components/blocks/header/nutrient-advice.tsx (1)
33-39: LGTM! Client-side navigation enables cache optimization.The refactor to use
NavLinkwithasChildcomposition is correct and aligns well with the PR's client-side caching objectives. Client-side navigation allows the cache middleware to intercept requests without full page reloads, improving UX and enabling the hash-based cache checks.The
asChildpattern properly delegates rendering to theNavLinkcomponent, which is the standard Radix UI composition approach for custom navigation components.fdm-app/app/routes/farm.$b_id_farm.$calendar.nutrient_advice.$b_id.tsx (2)
230-261: Defensive cache handling is well-implemented.The defensive check on lines 251-261 correctly handles the edge case where
serverAsyncData.useCacheis true butcachedDatais missing (e.g., browser history link with stalecacheHash, cleared localStorage). Removing thecacheHashparameter and returningnulltriggers a re-render with fresh data fetched from the server.The
useEffect(lines 236-249) appropriately caches fresh server data when the server didn't use the cache, ensuring future requests can benefit from the cached result.Note: Past review comments suggested moving the
asyncDatacomputation before theuseEffectand including it in the dependency array. However, since the currentuseEffectusesserverAsyncData(line 242) rather thanasyncData, there's no temporal dead zone (TDZ) risk, and the implementation works correctly within the middleware contract. The current approach is acceptable.
9-10: LGTM! Imports and overall structure are well-organized.The imports are appropriate for the client-side caching implementation:
object-hashfor deterministic input hashinguseEffectfor cache lifecycle managementuseSearchParamsfor defensive cache parameter handlinguseFieldNutrientAdviceCachefrom the shared calculation-cache storeThe overall structure cleanly separates concerns: the loader computes the input hash and decides whether to use cached data, while the component manages the cache lifecycle and fallback scenarios.
Also applies to: 16-16, 33-33
fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx (4)
7-7: LGTM!The new imports support the caching implementation correctly.
Also applies to: 14-14, 17-17, 36-36, 38-38
276-310: LGTM!The client middleware correctly synchronizes the
cacheHashURL parameter with the cached data. The SSR check, null safety, and redirect logic are all appropriate.
369-375: LGTM!This correctly handles the case where the server instructs the client to use cache (
data.useCache) but the client has no cached data. Removing thecacheHashparameter and returningnulltriggers a re-render, and the client middleware will then redirect to a clean URL without the hash, prompting the server to compute fresh data.✅ This addresses the previous review comment about guarding against
cacheHashwithout local cache.
377-386: Verify data selection logic is safe after fixing the missing inputHash issue.The data selection logic correctly chooses cached data when
data.useCache && cachedData, and falls back to fresh data otherwise. However, this assumes that whendata.useCacheis true, eithercachedDataexists or lines 369-375 have already handled the missing cache case.After applying the fix for the missing
inputHashin the loader's early return (see review comment on lines 155-157), this logic should work correctly.
| if (inputHash === cacheHash) { | ||
| return { useCache: true } | ||
| } |
There was a problem hiding this comment.
Include inputHash in the early return payload.
When returning early due to a cache hit, the payload only contains { useCache: true }. However, the Norms component at line 363 checks data.inputHash, and the useEffect at line 361 also references it. Without inputHash in the payload, these checks may behave unexpectedly.
Apply this diff to include inputHash in the early return:
inputHash = hash(inputs)
if (inputHash === cacheHash) {
- return { useCache: true }
+ return { useCache: true, inputHash }
}📝 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.
| if (inputHash === cacheHash) { | |
| return { useCache: true } | |
| } | |
| inputHash = hash(inputs) | |
| if (inputHash === cacheHash) { | |
| return { useCache: true, inputHash } | |
| } |
🤖 Prompt for AI Agents
In fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx around lines 155 to
157, the early return on cache hit only returns { useCache: true } but the
component expects data.inputHash; update the return to include inputHash (e.g.,
return { useCache: true, inputHash }) so the Norms component and its useEffect
can read the hash; ensure the inputHash variable is in scope and returned
whenever you short-circuit on cache.
| useEffect(() => { | ||
| if ( | ||
| (!data.useCache || !cachedData?.inputHash) && | ||
| !data.errorMessage && | ||
| data.inputHash | ||
| ) { | ||
| farmNormsCache.set(loaderData.b_id_farm, data) | ||
| } | ||
| }, [loaderData.b_id_farm, data, cachedData?.inputHash, farmNormsCache.set]) |
There was a problem hiding this comment.
Refine the useEffect condition to prevent redundant cache updates.
The current condition (!data.useCache || !cachedData?.inputHash) will always be true when data.useCache is false (fresh data), causing the effect to run repeatedly even after the cache has been populated. This could lead to unnecessary re-renders and potential infinite loops if the cache setter triggers a component update.
Apply this diff to only save when the inputHash differs from the cached inputHash:
useEffect(() => {
if (
- (!data.useCache || !cachedData?.inputHash) &&
+ !data.useCache &&
+ data.inputHash &&
+ data.inputHash !== cachedData?.inputHash &&
- !data.errorMessage &&
- data.inputHash
+ !data.errorMessage
) {
farmNormsCache.set(loaderData.b_id_farm, data)
}
- }, [loaderData.b_id_farm, data, cachedData?.inputHash, farmNormsCache.set])
+ }, [loaderData.b_id_farm, data.inputHash, cachedData?.inputHash, farmNormsCache.set, data.errorMessage])This ensures the effect only runs when:
- Data is fresh from the server (
!data.useCache) - Data has an inputHash
- The inputHash differs from what's cached (or cache is empty)
- No error occurred
Additionally, refine the dependency array to include only the specific properties of data that are checked in the condition, rather than the entire data object, to avoid unnecessary re-runs.
📝 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.
| useEffect(() => { | |
| if ( | |
| (!data.useCache || !cachedData?.inputHash) && | |
| !data.errorMessage && | |
| data.inputHash | |
| ) { | |
| farmNormsCache.set(loaderData.b_id_farm, data) | |
| } | |
| }, [loaderData.b_id_farm, data, cachedData?.inputHash, farmNormsCache.set]) | |
| useEffect(() => { | |
| if ( | |
| !data.useCache && | |
| data.inputHash && | |
| data.inputHash !== cachedData?.inputHash && | |
| !data.errorMessage | |
| ) { | |
| farmNormsCache.set(loaderData.b_id_farm, data) | |
| } | |
| }, [loaderData.b_id_farm, data.inputHash, cachedData?.inputHash, farmNormsCache.set, data.errorMessage]) |
🤖 Prompt for AI Agents
In fdm-app/app/routes/farm.$b_id_farm.$calendar.norms.tsx around lines 359 to
367, the useEffect saves to cache unconditionally when data.useCache is false
which can cause redundant updates; change the condition to only call
farmNormsCache.set(loaderData.b_id_farm, data) when data.inputHash exists and
differs from cachedData?.inputHash (i.e., !data.useCache && data.inputHash &&
data.inputHash !== cachedData?.inputHash && !data.errorMessage), and tighten the
dependency array to only include loaderData.b_id_farm, data.useCache,
data.inputHash, data.errorMessage, and cachedData?.inputHash (remove the whole
data object and farmNormsCache.set) so the effect only runs when those specific
values change.
There was a problem hiding this comment.
Can the data object will become a different one between renders with the same async data? If this is not the case, then b_id_farm, inputHash, farmNormsCacheset, and errorMessage won't be different during the second render.
There was a problem hiding this comment.
@BoraIneviNMI, excellent question! You're absolutely right to scrutinize this.
With React Router's loader data and the use() hook, the data object should indeed be referentially stable across renders as long as the underlying loaderData.asyncData promise hasn't changed. React's use() hook caches the resolved value based on the promise's identity, so the same promise will return the same data object reference.
Given this stability, you're correct that including individual properties like data.inputHash versus the whole data object in the dependency array would have similar behavior in terms of preventing unnecessary effect runs - they'll only trigger when the loader runs again with new data.
The primary issue is actually the condition logic, not the dependency array.
The original condition (!data.useCache || !cachedData?.inputHash) will remain true even after caching completes (because !data.useCache stays true for fresh data), which could cause the effect to run on every render if something else triggers re-renders. The fix to check data.inputHash !== cachedData?.inputHash prevents this by ensuring we only cache when the input has actually changed.
So a simpler fix focusing on the condition would be sufficient:
useEffect(() => {
if (
!data.useCache &&
data.inputHash &&
data.inputHash !== cachedData?.inputHash &&
!data.errorMessage
) {
farmNormsCache.set(loaderData.b_id_farm, data)
}
}, [loaderData.b_id_farm, data, cachedData?.inputHash, farmNormsCache.set])The dependency array can remain with data since it's stable. The refined dependencies I suggested earlier were following the general React best practice of "include only what you use," but with stable data, it's not strictly necessary for correctness here.
Does this clarify the situation?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
fdm-app/app/components/blocks/header/nutrient-advice.tsx(1 hunks)
| <BreadcrumbLink onClick={() => typeof window !== "undefined" && window.location.reload()}> | ||
| Bemestingsadvies | ||
| </BreadcrumbLink> |
There was a problem hiding this comment.
Replace page reload with proper router navigation.
The onClick handler forces a full page reload instead of using React Router's client-side navigation. This causes several issues:
- Incorrect navigation:
window.location.reload()reloads the current page rather than navigating to the base nutrient advice path (/farm/{b_id_farm}/{calendar}/nutrient_advice). If a user is viewing a specific field, clicking this breadcrumb should take them to the overview, not reload the field page. - Performance: Full page reloads are significantly slower than client-side navigation.
- Inconsistency: Field selection (lines 56-60) uses
NavLinkfor proper routing, making this reload approach inconsistent. - Lost state: Reloading discards browser state, scroll position, and other ephemeral UI state.
Given that this PR introduces cache-aware routing with client middleware (per PR objectives), integrate this breadcrumb with the new routing system instead of forcing reloads.
Apply this diff to use proper navigation:
- <BreadcrumbLink onClick={() => typeof window !== "undefined" && window.location.reload()}>
+ <BreadcrumbLink asChild>
+ <NavLink to={`/farm/${b_id_farm}/${calendar}/nutrient_advice`}>
- Bemestingsadvies
+ Bemestingsadvies
+ </NavLink>
</BreadcrumbLink>📝 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.
| <BreadcrumbLink onClick={() => typeof window !== "undefined" && window.location.reload()}> | |
| Bemestingsadvies | |
| </BreadcrumbLink> | |
| <BreadcrumbLink asChild> | |
| <NavLink to={`/farm/${b_id_farm}/${calendar}/nutrient_advice`}> | |
| Bemestingsadvies | |
| </NavLink> | |
| </BreadcrumbLink> |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
fdm-app/app/components/blocks/header/nutrient-advice.tsx(1 hunks)
| <BreadcrumbLink onClick={() => typeof window !== "undefined" && window.location.reload()}> | ||
| Bemestingsadvies | ||
| </BreadcrumbLink> |
There was a problem hiding this comment.
🧩 Analysis chain
Restore proper navigation instead of forcing page reload.
This change breaks accessibility and navigation:
- Accessibility: The link has no
hrefattribute, making it non-keyboard accessible and invisible to screen readers and search engines. - Navigation:
window.location.reload()reloads the current page instead of navigating to/farm/${b_id_farm}/${calendar}/nutrient_advice. If the user is on a field-specific page (e.g.,/farm/123/2025/nutrient_advice/field456), clicking this breadcrumb keeps them on the same page rather than navigating up to the parent nutrient advice route. - UX degradation: Full page reloads are slower and more disruptive than React Router's client-side navigation.
- Inconsistency: The dropdown menu items (lines 56-58) use proper
NavLinknavigation, but the parent breadcrumb uses a reload.
Given that the PR implements automatic cache versioning (as stated in the PR description), cache invalidation should be handled by the versioning system rather than forcing manual page reloads in navigation links.
Replace the reload with proper navigation:
-<BreadcrumbLink onClick={() => typeof window !== "undefined" && window.location.reload()}>
+<BreadcrumbLink asChild>
+ <NavLink to={`/farm/${b_id_farm}/${calendar}/nutrient_advice`}>
Bemestingsadvies
+ </NavLink>
</BreadcrumbLink>If cache invalidation is required on navigation, implement it in the route component's loader or use React Router's reloadDocument prop on the NavLink, but avoid window.location.reload() which doesn't navigate to the intended route.
Can you clarify the intent behind forcing a page reload here? If the goal is cache invalidation, please verify that:
🏁 Script executed:
#!/bin/bash
# Search for cache invalidation logic in route loaders and stores
rg -nP -C3 "nutrient_advice|calculation-cache" --type=ts --type=tsx -g '!**/node_modules/**'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for cache invalidation logic in route loaders and stores
rg -nP -C3 "nutrient_advice|calculation-cache" -g '*.ts' -g '*.tsx' -g '!**/node_modules/**'Length of output: 9613
Restore proper navigation on the breadcrumb link.
- The link has no
href, breaking keyboard and screen-reader access. window.location.reload()reloads the current page instead of navigating to/farm/${b_id_farm}/${calendar}/nutrient_advice.- Full-page reloads are slower and more disruptive than client-side routing.
- Dropdown menu items use
<NavLink>correctly; this breadcrumb should too.
-<BreadcrumbLink onClick={() => typeof window !== "undefined" && window.location.reload()}>
+<BreadcrumbLink asChild>
+ <NavLink reloadDocument to={`/farm/${b_id_farm}/${calendar}/nutrient_advice`}>
Bemestingsadvies
+ </NavLink>
</BreadcrumbLink>
SvenVw
left a comment
SvenVw
left a comment
There was a problem hiding this comment.
Hmm, I think I have made a faulty assumption in the original issue that the server knows the state of the localstorage to check whether it should calculate the values or not. Let's discuss it first if there are other ways to implement it
| @@ -0,0 +1,47 @@ | |||
| import { redirect } from "react-router" | |||
There was a problem hiding this comment.
React router supports middleware as a stable feature since v7.9.0. Have you considered using those functions? https://reactrouter.com/how-to/middleware
|
|
||
| export async function loader() { | ||
| // Redirect to nitrogen page | ||
| return redirect("./nitrogen") |
There was a problem hiding this comment.
Why is this redirect removed?
| const page = location.pathname | ||
| const calendar = useCalendarStore((state) => state.calendar) | ||
|
|
||
| const fieldNitrogenBalanceCache = useFieldNitrogenBalanceCache() |
There was a problem hiding this comment.
I would prefer to move this into a separate function. The idea is that we can obtain the results of this calculation on various pages with a single function that handles the input collection, cache checking, calculation and cache storage. Otherwise we have to repeat this code on every page that will use the results of the calculation
| datasetsUrl, | ||
| ) | ||
| .then(async (input) => { | ||
| const inputHash = hash(input) |
There was a problem hiding this comment.
There are various typescript warnings with this block of code. Please check
| envPrefix: "PUBLIC_", | ||
| define: { | ||
| global: {}, | ||
| PUBLIC_FDM_CALCULATOR_VERSION: JSON.stringify( |
There was a problem hiding this comment.
I am not sure if this works in production, as build times variables are not present at runtime always. Maybe better to use at the store the clientConfig
2 participants
Summary by CodeRabbit
New Features
Performance
Navigation