Build infrastructure optimization: Rollup to tsdown migration#559
Merged
Conversation
…ll in, so they are processed once rather than on every cold dev start
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
📝 WalkthroughWalkthroughMigrates per-package builds from Rollup+tsc to tsdown, replaces umbrella Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (pnpm dev)
participant Vite as Vite Dev Server
participant WS as Workspace Packages
participant Bundle as Pre-bundled Deps
rect rgba(100,150,240,0.5)
Dev->>Vite: start dev server
Vite->>Bundle: optimizeDeps.prebundle(include list)
Bundle-->>Vite: pre-bundled artifacts cached
Vite->>WS: load workspace package source (HMR enabled)
WS-->>Vite: source served (uses pre-bundled deps)
Vite-->>Dev: dev server ready (faster cold start)
end
sequenceDiagram
participant CI as Docker build (runner)
participant PNPM as pnpm
participant Image as Production image
rect rgba(100,200,150,0.5)
CI->>PNPM: RUN pnpm install --frozen-lockfile --prod
PNPM-->>Image: install production deps only
CI->>Image: build app (no devDeps)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-core/src/authorization.test.ts (1)
486-496:⚠️ Potential issue | 🔴 CriticalFix transaction mock typing to unblock TS2345 in tests.
mockTxis currently typed as a zero-arg function, which is incompatible with the expectedtransactionsignature. The real signature expects a callback that receives a transaction object as a parameter.Apply the following fix to all three test blocks at lines 486, 883, and 977:
Proposed fix
- const mockTx = async () => { + const mockTx = (async (..._args: unknown[]) => { throw new Error("Database transaction failed") - } + }) as FdmServerType["transaction"] const fdmMock = { ...fdm, transaction: mockTx, - } + } as FdmServerType🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.test.ts` around lines 486 - 496, The transaction mock is declared with zero args causing TS2345; change mockTx to match the real fdm.transaction signature by accepting a callback parameter (e.g., async (txCallback) => { throw new Error("Database transaction failed") } or call txCallback with a stub tx) and assign that to fdm.transaction in the test blocks where mockTx is used (updateRole tests referencing mockTx and fdm.transaction) so TypeScript recognizes the proper callback signature; apply the same change in the three test blocks at the referenced occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-agents/tsdown.config.ts`:
- Line 9: fdm-agents is inconsistent with the repo: add an engines.node entry in
fdm-agents/package.json and align tsdown.config.ts target to match the chosen
Node support; either set engines.node to ">=24.0.0" and change tsdown.config.ts
target from "node20" to "node24", or if you intend Node 20 support, add
engines.node ">=20.0.0" and keep target: "node20" while documenting the reason.
Update the package.json engines.node field and the target value in
tsdown.config.ts (the unique symbols to change are engines.node in package.json
and target in tsdown.config.ts).
In `@fdm-app/package.json`:
- Around line 40-45: Remove the unused dependency `@turf/boolean-intersects` from
package.json: locate the dependency entry "@turf/boolean-intersects": "^7.3.4"
and delete it, then run your package manager (npm/yarn/pnpm) to update
node_modules and regenerate the lockfile; also re-run a global import/search to
confirm no remaining usages of `@turf/boolean-intersects` in the codebase (if you
find any, either restore the dependency or replace those imports with the
appropriate granular package).
In `@fdm-core/tsdown.config.ts`:
- Around line 20-28: The try/catch around mkdirSync/cpSync currently logs errors
but swallows them so the build can continue; update the catch in the migration
copy block to fail the build by rethrowing the error (or calling
process.exit(1)) after logging so failures in mkdirSync/cpSync cause the build
hook to fail; locate the try/catch that wraps mkdirSync, cpSync and the
console.error and replace the swallow-with-log behavior with a rethrow or
explicit non-zero exit.
---
Outside diff comments:
In `@fdm-core/src/authorization.test.ts`:
- Around line 486-496: The transaction mock is declared with zero args causing
TS2345; change mockTx to match the real fdm.transaction signature by accepting a
callback parameter (e.g., async (txCallback) => { throw new Error("Database
transaction failed") } or call txCallback with a stub tx) and assign that to
fdm.transaction in the test blocks where mockTx is used (updateRole tests
referencing mockTx and fdm.transaction) so TypeScript recognizes the proper
callback signature; apply the same change in the three test blocks at the
referenced occurrences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0aac6c9d-474c-4832-81a7-f5ebc4890a6e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (80)
.changeset/dirty-teams-mate.md.changeset/lucky-terms-warn.md.changeset/olive-swans-roll.md.changeset/witty-towns-attack.mdfdm-agents/package.jsonfdm-agents/rollup.config.jsfdm-agents/tsdown.config.tsfdm-app/app/components/blocks/atlas-fields/query.tsxfdm-app/app/components/blocks/rvo/import-review-table.tsxfdm-app/app/components/blocks/soil/bulk-upload-match.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.elevation.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.fields._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.new._index.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsxfdm-app/dockerfilefdm-app/package.jsonfdm-app/vite.config.tsfdm-calculator/package.jsonfdm-calculator/rollup.config.jsfdm-calculator/tsconfig.build.jsonfdm-calculator/tsdown.config.tsfdm-core/package.jsonfdm-core/rollup.config.jsfdm-core/src/authentication.tsfdm-core/src/authorization.test.tsfdm-core/src/authorization.tsfdm-core/src/authorization.types.d.tsfdm-core/src/catalogues.tsfdm-core/src/cultivation.test.tsfdm-core/src/cultivation.tsfdm-core/src/cultivation.types.d.tsfdm-core/src/db/schema.tsfdm-core/src/derogation.test.tsfdm-core/src/derogation.tsfdm-core/src/error.tsfdm-core/src/error.types.d.tsfdm-core/src/farm.test.tsfdm-core/src/farm.tsfdm-core/src/fdm-local.tsfdm-core/src/fdm-local.types.d.tsfdm-core/src/fdm-server.test.tsfdm-core/src/fdm-server.tsfdm-core/src/fdm-server.types.d.tsfdm-core/src/fdm.d.tsfdm-core/src/fertilizer.test.tsfdm-core/src/fertilizer.tsfdm-core/src/fertilizer.types.d.tsfdm-core/src/field.test.tsfdm-core/src/field.tsfdm-core/src/field.types.d.tsfdm-core/src/grazing_intention.tsfdm-core/src/harvest-conversion.tsfdm-core/src/harvest-conversion.types.d.tsfdm-core/src/harvest.test.tsfdm-core/src/harvest.tsfdm-core/src/harvest.types.d.tsfdm-core/src/index.tsfdm-core/src/invitation.test.tsfdm-core/src/invitation.tsfdm-core/src/organic.test.tsfdm-core/src/organic.tsfdm-core/src/organic.types.d.tsfdm-core/src/principal.tsfdm-core/src/principal.types.d.tsfdm-core/src/soil.test.tsfdm-core/src/soil.tsfdm-core/src/soil.types.d.tsfdm-core/tsconfig.build.jsonfdm-core/tsdown.config.tsfdm-data/package.jsonfdm-data/rollup.config.jsfdm-data/tsconfig.build.jsonfdm-data/tsdown.config.tsfdm-rvo/package.jsonfdm-rvo/rollup.config.jsfdm-rvo/tsconfig.build.jsonfdm-rvo/tsdown.config.tspnpm-workspace.yaml
💤 Files with no reviewable changes (9)
- fdm-calculator/tsconfig.build.json
- fdm-calculator/rollup.config.js
- fdm-rvo/tsconfig.build.json
- fdm-core/tsconfig.build.json
- fdm-data/tsconfig.build.json
- fdm-agents/rollup.config.js
- fdm-rvo/rollup.config.js
- fdm-data/rollup.config.js
- fdm-core/rollup.config.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-core/src/cultivation.ts (1)
1077-1115:⚠️ Potential issue | 🟠 MajorAggregate nested plan data instead of appending once per joined row.
Lines 1079-1115 still create one
fertilizer_applicationsentry and oneharvestsentry per SQL row, but this query joins multiple one-to-many tables (fertilizerApplication,harvestableSampling,harvestableAnalyses). A single harvest with N analyses and M field applications will therefore fan out into M×N duplicate fertilizer entries and duplicate harvest objects, and a harvest with no analyses still produces a synthetic all-null analysis item. Please merge by stable IDs before pushing; otherwise the result does not matchfdm-core/src/cultivation.types:53-71, which models one harvest with an analyses array. You’ll likely need to project an analysis key as well to rebuild this safely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/cultivation.ts` around lines 1077 - 1115, The current logic pushes a fertilizer_applications entry and a harvests entry for every joined SQL row, causing M×N duplicates; instead, group rows by stable IDs: use existingField (found by field id), then for fertilizer entries deduplicate by p_app_id before pushing into existingField!.fertilizer_applications, and for harvests find or create a harvest object keyed by b_id_harvesting in existingField!.harvests and append analyses into that harvest’s harvestable.harvestable_analyses array (deduplicate by an analysis key you should project from the query, e.g., harvestable_analysis_id); also skip creating a synthetic all-null analysis by checking analysis columns are non-null before pushing. Ensure you reference and update fertilizer_applications, harvests, harvestable.harvestable_analyses, curr, and existingField in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-core/src/soil.ts`:
- Line 278: The function currently returns (soilAnalysis[0] as SoilAnalysis) ??
null which violates the declared Promise<SoilAnalysis> return type; instead
detect if soilAnalysis[0] is missing and throw an error (e.g., new Error("Soil
analysis not found")) so the function never returns null. Update the code around
the soilAnalysis result in the function (reference the soilAnalysis variable and
the checkPermission call) to throw when no record is found and then return
soilAnalysis[0] (typed as SoilAnalysis) otherwise, matching the pattern used in
getField and getFarm.
In `@issue-rvo-redirect-uri.md`:
- Line 66: Update the mismatched env var name: replace references to FDM_URL
with PUBLIC_FDM_URL so the docs/troubleshooting match how serverConfig.url is
actually read (process.env.PUBLIC_FDM_URL); search for occurrences of FDM_URL
(notably around the noted lines) and update text/examples to mention
PUBLIC_FDM_URL and confirm any guidance about request origin or misconfiguration
refers to serverConfig.url and process.env.PUBLIC_FDM_URL.
- Around line 13-15: The fenced code block that contains the redirect URI
currently has no language tag (the block that starts with ``` and contains the
https://minas2.nl/... URL); add a language identifier (e.g., use "text") to the
opening fence so it becomes ```text to satisfy markdownlint MD040 and improve
readability, leaving the block content unchanged.
---
Outside diff comments:
In `@fdm-core/src/cultivation.ts`:
- Around line 1077-1115: The current logic pushes a fertilizer_applications
entry and a harvests entry for every joined SQL row, causing M×N duplicates;
instead, group rows by stable IDs: use existingField (found by field id), then
for fertilizer entries deduplicate by p_app_id before pushing into
existingField!.fertilizer_applications, and for harvests find or create a
harvest object keyed by b_id_harvesting in existingField!.harvests and append
analyses into that harvest’s harvestable.harvestable_analyses array (deduplicate
by an analysis key you should project from the query, e.g.,
harvestable_analysis_id); also skip creating a synthetic all-null analysis by
checking analysis columns are non-null before pushing. Ensure you reference and
update fertilizer_applications, harvests, harvestable.harvestable_analyses,
curr, and existingField in your changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7d4708b5-8d6e-4880-8360-9081872d71e5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
.changeset/swift-suits-search.mdfdm-agents/package.jsonfdm-agents/tsdown.config.tsfdm-app/package.jsonfdm-core/package.jsonfdm-core/src/authentication.test.tsfdm-core/src/authentication.tsfdm-core/src/authorization.test.tsfdm-core/src/authorization.tsfdm-core/src/calculator.test.tsfdm-core/src/calculator.tsfdm-core/src/catalogues.test.tsfdm-core/src/catalogues.tsfdm-core/src/cultivation.test.tsfdm-core/src/cultivation.tsfdm-core/src/derogation.tsfdm-core/src/farm.test.tsfdm-core/src/farm.tsfdm-core/src/fdm-local.tsfdm-core/src/fdm-server.tsfdm-core/src/fdm-server.types.d.tsfdm-core/src/fdm.d.tsfdm-core/src/fdm.types.d.tsfdm-core/src/fertilizer.tsfdm-core/src/field.test.tsfdm-core/src/field.tsfdm-core/src/grazing_intention.test.tsfdm-core/src/grazing_intention.tsfdm-core/src/harvest.tsfdm-core/src/index.tsfdm-core/src/invitation.tsfdm-core/src/organic.tsfdm-core/src/principal.test.tsfdm-core/src/principal.tsfdm-core/src/soil.tsfdm-core/src/test-util.tsfdm-core/tsdown.config.tsissue-rvo-redirect-uri.md
💤 Files with no reviewable changes (2)
- fdm-core/src/fdm-local.ts
- fdm-core/src/fdm.d.ts
✅ Files skipped from review due to trivial changes (19)
- .changeset/swift-suits-search.md
- fdm-core/src/derogation.ts
- fdm-core/src/authentication.test.ts
- fdm-core/src/calculator.test.ts
- fdm-core/src/test-util.ts
- fdm-core/src/organic.ts
- fdm-core/src/grazing_intention.test.ts
- fdm-core/src/catalogues.ts
- fdm-core/package.json
- fdm-core/src/principal.test.ts
- fdm-agents/tsdown.config.ts
- fdm-core/src/grazing_intention.ts
- fdm-core/src/field.ts
- fdm-core/src/field.test.ts
- fdm-core/src/principal.ts
- fdm-core/src/cultivation.test.ts
- fdm-core/src/farm.ts
- fdm-core/src/invitation.ts
- fdm-core/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- fdm-core/src/fdm-server.ts
- fdm-agents/package.json
- fdm-app/package.json
- fdm-core/tsdown.config.ts
- fdm-core/src/authorization.ts
- fdm-core/src/fertilizer.ts
| .where(eq(schema.soilAnalysis.a_id, a_id)) | ||
|
|
||
| return soilAnalysis[0] || null | ||
| return (soilAnalysis[0] as SoilAnalysis) ?? null |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Definition of getSoilAnalysis:"
sed -n '209,280p' fdm-core/src/soil.ts
echo
echo "Call sites for getSoilAnalysis:"
rg -n -C2 '\bgetSoilAnalysis\s*\(' --type tsRepository: nmi-agro/fdm
Length of output: 4661
🏁 Script executed:
# Check how the call sites use the result from getSoilAnalysis
echo "=== Call site 1: test file ==="
sed -n '315,330p' fdm-core/src/soil.test.ts
echo
echo "=== Call site 2: routes file ==="
sed -n '82,100p' fdm-app/app/routes/farm.\$b_id_farm.\$calendar.field.\$b_id.soil.analysis.\$a_id.tsx
echo
echo "=== Check if other functions follow same pattern ==="
rg -A5 'async function get(Farm|Field|Fertilizer)\(' fdm-core/src --type ts | head -50Repository: nmi-agro/fdm
Length of output: 2284
🏁 Script executed:
# Check the return types of similar functions
echo "=== getField return type and implementation ==="
rg -A20 'async function getField\(' fdm-core/src/field.ts | head -40
echo
echo "=== getFarm return type and implementation ==="
rg -A20 'async function getFarm\(' fdm-core/src/farm.ts | head -40
echo
echo "=== getSoilAnalysis return type ==="
rg 'Promise<' fdm-core/src/soil.ts | grep getSoilAnalysisRepository: nmi-agro/fdm
Length of output: 1516
🏁 Script executed:
# Get return type from function signature
sed -n '209,215p' fdm-core/src/soil.ts
echo
echo "=== Check getField full implementation to see final return ==="
sed -n '110,185p' fdm-core/src/field.ts | tail -30
echo
echo "=== Check getFarm full implementation to see final return ==="
sed -n '15,100p' fdm-core/src/farm.ts | grep -A 30 "const farm ="Repository: nmi-agro/fdm
Length of output: 1339
🏁 Script executed:
# Find the actual return statements in these functions
echo "=== getField return statement ==="
rg -A 50 'async function getField\(' fdm-core/src/field.ts | grep -A 2 'return'
echo
echo "=== getFarm return statement ==="
rg -A 100 'async function getFarm\(' fdm-core/src/farm.ts | grep -A 2 'return' | head -20
echo
echo "=== getSoilAnalysis return statement ==="
rg -A 100 'async function getSoilAnalysis\(' fdm-core/src/soil.ts | grep -B 2 -A 2 'return.*soilAnalysis'Repository: nmi-agro/fdm
Length of output: 1191
🏁 Script executed:
# Check if widening the type is done elsewhere in the codebase
echo "=== Check if any get* functions return optional types ==="
rg 'async function get\w+\(' fdm-core/src --type ts -A 2 | grep 'Promise<.*\|'
echo
echo "=== Check the full routes implementation to see if null handling is essential ==="
sed -n '70,110p' fdm-app/app/routes/farm.\$b_id_farm.\$calendar.field.\$b_id.soil.analysis.\$a_id.tsx
echo
echo "=== Check test to understand null expectation ==="
sed -n '310,335p' fdm-core/src/soil.test.tsRepository: nmi-agro/fdm
Length of output: 9393
Align the return type with the function signature: throw instead of returning null.
Line 278 returns null despite the function declaring Promise<SoilAnalysis>. This creates a type contract violation—the as SoilAnalysis cast suppresses TypeScript warnings but cannot prevent null from being returned at runtime. Per the fdm-core pattern used in getField and getFarm, checkPermission should already throw if the soil analysis doesn't exist. If the query still returns empty, throw an error rather than returning null to maintain consistency.
Proposed fix
export async function getSoilAnalysis(
fdm: FdmType,
principal_id: PrincipalId,
a_id: schema.soilAnalysisTypeSelect["a_id"],
): Promise<SoilAnalysis> {
@@
const soilAnalysis = await fdm
.select({
a_id: schema.soilAnalysis.a_id,
@@
.leftJoin(
schema.soilSampling,
eq(schema.soilAnalysis.a_id, schema.soilSampling.a_id),
)
.where(eq(schema.soilAnalysis.a_id, a_id))
- return (soilAnalysis[0] as SoilAnalysis) ?? null
+ if (soilAnalysis.length === 0) {
+ throw new Error("Soil analysis does not exist")
+ }
+
+ return soilAnalysis[0]
} catch (err) {
throw handleError(err, "Exception for getSoilAnalysis", { a_id })
}
}📝 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.
Suggested change
| return (soilAnalysis[0] as SoilAnalysis) ?? null | |
| export async function getSoilAnalysis( | |
| fdm: FdmType, | |
| principal_id: PrincipalId, | |
| a_id: schema.soilAnalysisTypeSelect["a_id"], | |
| ): Promise<SoilAnalysis> { | |
| const soilAnalysis = await fdm | |
| .select({ | |
| a_id: schema.soilAnalysis.a_id, | |
| // ... other selected fields | |
| }) | |
| .from(schema.soilAnalysis) | |
| .leftJoin( | |
| schema.soilSampling, | |
| eq(schema.soilAnalysis.a_id, schema.soilSampling.a_id), | |
| ) | |
| .where(eq(schema.soilAnalysis.a_id, a_id)) | |
| if (soilAnalysis.length === 0) { | |
| throw new Error("Soil analysis does not exist") | |
| } | |
| return soilAnalysis[0] as SoilAnalysis | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-core/src/soil.ts` at line 278, The function currently returns
(soilAnalysis[0] as SoilAnalysis) ?? null which violates the declared
Promise<SoilAnalysis> return type; instead detect if soilAnalysis[0] is missing
and throw an error (e.g., new Error("Soil analysis not found")) so the function
never returns null. Update the code around the soilAnalysis result in the
function (reference the soilAnalysis variable and the checkPermission call) to
throw when no record is found and then return soilAnalysis[0] (typed as
SoilAnalysis) otherwise, matching the pattern used in getField and getFarm.
Comment on lines
+13
to
+15
| ``` | ||
| https://minas2.nl/?code=9wKFvFCqZ__gOUt8yBtslkFpJTobKuS-v1wDooDSI2s&state=eyJmYXJtSWQiOiJ0VFRQODk2bUxwN3drdGI4IiwicmV0dXJuVXJsIjoiLyIsIm5vbmNlIjoicDkzc08xTHdJVVEzbm9xSUFtb3NUIn0%3D | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language identifier to the fenced code block
Line 13 starts a fenced block without a language tag. This triggers markdownlint MD040 and reduces readability.
Suggested doc fix
-```
+```text
https://minas2.nl/?code=9wKFvFCqZ__gOUt8yBtslkFpJTobKuS-v1wDooDSI2s&state=eyJmYXJtSWQiOiJ0VFRQODk2bUxwN3drdGI4IiwicmV0dXJuVXJsIjoiLyIsIm5vbmNlIjoicDkzc08xTHdJVVEzbm9xSUFtb3NUIn0%3D</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @issue-rvo-redirect-uri.md around lines 13 - 15, The fenced code block that
contains the redirect URI currently has no language tag (the block that starts
with and contains the https://minas2.nl/... URL); add a language identifier (e.g., use "text") to the opening fence so it becomestext to satisfy
markdownlint MD040 and improve readability, leaving the block content unchanged.
</details>
<!-- fingerprinting:phantom:triton:hawk:829b1682-7df2-47d6-9154-8d419645ea1b -->
<!-- This is an auto-generated comment by CodeRabbit -->
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fdm-calculator/src/balance/nitrogen/input.ts (2)
346-367:⚠️ Potential issue | 🟡 MinorValidate
timeframebefore any reads start.This guard runs only after the catalogue fetches and after
collectInputForNitrogenBalanceForFarm()has already usedtimeframe, so invalid input can still trigger unnecessary work or surface a different error first. Move the check to the top of the function.Suggested fix
export async function collectInputForNitrogenBalance( fdm: FdmType, principal_id: PrincipalId, b_id_farm: fdmSchema.farmsTypeSelect["b_id_farm"], timeframe: Timeframe, b_id?: fdmSchema.fieldsTypeSelect["b_id"], ): Promise<NitrogenBalanceInput> { try { + if (!timeframe.start || !timeframe.end) { + throw new Error( + "Timeframe start and end must be provided for nitrogen balance calculation", + ) + } + return await fdm.transaction(async (tx) => { const cultivationDetails = await getCultivationsFromCatalogue( tx, principal_id, b_id_farm, @@ const fields = await collectInputForNitrogenBalanceForFarm( tx, principal_id, b_id_farm, timeframe, b_id, ) - if (!timeframe.start || !timeframe.end) { - throw new Error( - "Timeframe start and end must be provided for nitrogen balance calculation", - ) - } return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/balance/nitrogen/input.ts` around lines 346 - 367, The timeframe null/undefined check should be performed before any database reads so invalid input doesn't trigger unnecessary work; move the validation for timeframe.start and timeframe.end to the top of the transaction callback in the function that calls fdm.transaction, before calling getCultivationsFromCatalogue, getFertilizersFromCatalogue, or collectInputForNitrogenBalanceForFarm, and throw the same Error if either is missing to short-circuit execution early.
128-136:⚠️ Potential issue | 🟠 MajorDon't turn missing deposition data into
0.If
calculateAllFieldsNitrogenSupplyByDeposition()omits a field, these fallbacks now produce a valid-looking balance with understated nitrogen supply instead of surfacing incomplete input. Downstream, thattotalis added straight into the field supply, so this silently biases the calculation low.Also applies to: 165-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/balance/nitrogen/input.ts` around lines 128 - 136, The code is replacing missing deposition entries with a zero-value object which silently under-reports nitrogen; remove the fallback and preserve the absence so missing deposition is detectable. Replace the use of the nullish coalescing fallback (depositionByField.get(field.b_id) ?? { total: new Decimal(0) }) with just depositionByField.get(field.b_id) for both occurrences, leaving depositionSupply undefined when no entry exists; ensure any downstream consumers (e.g., where the value is added into field supply) handle or validate missing deposition data appropriately and surface an error or validation message instead of assuming zero.
♻️ Duplicate comments (1)
fdm-core/src/soil.ts (1)
278-278:⚠️ Potential issue | 🟠 MajorRemove nullable cast path from
getSoilAnalysisreturn.At Line 278,
(soilAnalysis[0] || null) as SoilAnalysiscan still returnnull(and mask nullable join fields) while the function contract isPromise<SoilAnalysis>.Proposed fix
- .leftJoin( + .innerJoin( schema.soilSampling, eq(schema.soilAnalysis.a_id, schema.soilSampling.a_id), ) .where(eq(schema.soilAnalysis.a_id, a_id)) - return (soilAnalysis[0] || null) as SoilAnalysis + const first = soilAnalysis[0] + if (!first) { + throw new Error("Soil analysis does not exist") + } + return firstBased on learnings,
checkPermissionin fdm-core throws when the resource does not exist, so a nullable return path is inconsistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/soil.ts` at line 278, The return currently allows a null path: "(soilAnalysis[0] || null) as SoilAnalysis" which contradicts the Promise<SoilAnalysis> contract; update getSoilAnalysis to remove the nullable cast and return a non-null value (e.g., return soilAnalysis[0] as SoilAnalysis or use a non-null assertion) so the function always returns a SoilAnalysis; you can rely on checkPermission's behavior (it throws if the resource is missing) and remove the null branch to avoid masking nullable join fields.
🧹 Nitpick comments (2)
fdm-core/src/farm.test.ts (1)
151-151: Optional: deduplicate repeatedas unknown as FdmTypecasts with a local helper.This is fine as-is, but a helper reduces noise and keeps future type migrations easier.
Refactor sketch
+const asFdmType = (mock: object): FdmType => mock as unknown as FdmType ... - return cb(tx as unknown as FdmType) + return cb(asFdmType(tx)) ... - getFarms(authorizationMock as unknown as FdmType, principal_id), + getFarms(asFdmType(authorizationMock), principal_id), ... - fdmMock as unknown as FdmType + asFdmType(fdmMock)Based on learnings: "When writing unit tests for the fdm project, avoid using Vitest's mocking utilities (vi) ... Instead, use simple object literals with methods that throw errors to test error handling."
Also applies to: 180-180, 252-252, 336-336, 445-445, 524-524, 599-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/farm.test.ts` at line 151, The repeated "as unknown as FdmType" casts (e.g., in the return cb(tx as unknown as FdmType) call) should be replaced with a small local helper to reduce noise and ease future type changes: add a helper like const asFdm = (t: unknown) => t as FdmType near the top of the test file and replace all occurrences (calls using cb, tx, etc.) with asFdm(tx); update every spot you saw the pattern (the instances around cb/tx and the other listed occurrences) so tests keep the same behavior but use the single helper cast.fdm-core/src/authorization.ts (1)
851-856: Extract the repeated bead-construction block.The same
Object.keys(...).filter(...).map(...)logic now exists in five branches ofgetResourceChain. Pulling that into a small helper would keep the null-filtering/casting rules in one place and make future resource-chain changes less error-prone.Also applies to: 887-892, 936-941, 966-971, 999-1004
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.ts` around lines 851 - 856, The repeated construction of "beads" inside getResourceChain should be extracted into a small helper (e.g., buildBeadsFromRow or extractBeads) that accepts a row/object (type any or a suitably typed record) and returns Array<{ resource: Resource; resource_id: string }>; implement the helper to run Object.keys(row).filter(k => row[k] !== null).map(k => ({ resource: k as Resource, resource_id: row[k] as string })), then replace each in-function block that currently builds beads (the occurrences around getResourceChain lines where beads is defined) with a single call to this helper so the null-filtering and casting logic is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@fdm-calculator/src/balance/nitrogen/input.ts`:
- Around line 346-367: The timeframe null/undefined check should be performed
before any database reads so invalid input doesn't trigger unnecessary work;
move the validation for timeframe.start and timeframe.end to the top of the
transaction callback in the function that calls fdm.transaction, before calling
getCultivationsFromCatalogue, getFertilizersFromCatalogue, or
collectInputForNitrogenBalanceForFarm, and throw the same Error if either is
missing to short-circuit execution early.
- Around line 128-136: The code is replacing missing deposition entries with a
zero-value object which silently under-reports nitrogen; remove the fallback and
preserve the absence so missing deposition is detectable. Replace the use of the
nullish coalescing fallback (depositionByField.get(field.b_id) ?? { total: new
Decimal(0) }) with just depositionByField.get(field.b_id) for both occurrences,
leaving depositionSupply undefined when no entry exists; ensure any downstream
consumers (e.g., where the value is added into field supply) handle or validate
missing deposition data appropriately and surface an error or validation message
instead of assuming zero.
---
Duplicate comments:
In `@fdm-core/src/soil.ts`:
- Line 278: The return currently allows a null path: "(soilAnalysis[0] || null)
as SoilAnalysis" which contradicts the Promise<SoilAnalysis> contract; update
getSoilAnalysis to remove the nullable cast and return a non-null value (e.g.,
return soilAnalysis[0] as SoilAnalysis or use a non-null assertion) so the
function always returns a SoilAnalysis; you can rely on checkPermission's
behavior (it throws if the resource is missing) and remove the null branch to
avoid masking nullable join fields.
---
Nitpick comments:
In `@fdm-core/src/authorization.ts`:
- Around line 851-856: The repeated construction of "beads" inside
getResourceChain should be extracted into a small helper (e.g.,
buildBeadsFromRow or extractBeads) that accepts a row/object (type any or a
suitably typed record) and returns Array<{ resource: Resource; resource_id:
string }>; implement the helper to run Object.keys(row).filter(k => row[k] !==
null).map(k => ({ resource: k as Resource, resource_id: row[k] as string })),
then replace each in-function block that currently builds beads (the occurrences
around getResourceChain lines where beads is defined) with a single call to this
helper so the null-filtering and casting logic is centralized.
In `@fdm-core/src/farm.test.ts`:
- Line 151: The repeated "as unknown as FdmType" casts (e.g., in the return
cb(tx as unknown as FdmType) call) should be replaced with a small local helper
to reduce noise and ease future type changes: add a helper like const asFdm =
(t: unknown) => t as FdmType near the top of the test file and replace all
occurrences (calls using cb, tx, etc.) with asFdm(tx); update every spot you saw
the pattern (the instances around cb/tx and the other listed occurrences) so
tests keep the same behavior but use the single helper cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c24efa74-67b5-474a-8adb-88ea3721d369
📒 Files selected for processing (22)
fdm-app/app/components/blocks/atlas-fields/query.tsxfdm-app/package.jsonfdm-calculator/src/balance/nitrogen/input.test.tsfdm-calculator/src/balance/nitrogen/input.tsfdm-core/src/authorization.tsfdm-core/src/calculator.tsfdm-core/src/catalogues.test.tsfdm-core/src/catalogues.tsfdm-core/src/cultivation.tsfdm-core/src/derogation.tsfdm-core/src/farm.test.tsfdm-core/src/farm.tsfdm-core/src/fdm-server.tsfdm-core/src/fdm-server.types.d.tsfdm-core/src/fertilizer.tsfdm-core/src/field.tsfdm-core/src/harvest.tsfdm-core/src/invitation.tsfdm-core/src/organic.tsfdm-core/src/principal.tsfdm-core/src/soil.tsfdm-rvo/package.json
✅ Files skipped from review due to trivial changes (7)
- fdm-app/app/components/blocks/atlas-fields/query.tsx
- fdm-rvo/package.json
- fdm-app/package.json
- fdm-core/src/catalogues.test.ts
- fdm-core/src/invitation.ts
- fdm-core/src/fdm-server.types.d.ts
- fdm-core/src/principal.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- fdm-core/src/fdm-server.ts
- fdm-core/src/organic.ts
- fdm-core/src/derogation.ts
- fdm-core/src/field.ts
- fdm-core/src/fertilizer.ts
- fdm-core/src/farm.ts
- fdm-core/src/cultivation.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
500-500: Clarify the optionality contract fordepositionSupply.Line 500 makes
depositionSupplyoptional, but calculation now hard-fails when it is absent. Add a short inline note here that omission is only valid before enrichment/preprocessing, not at calculation time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/balance/nitrogen/types.d.ts` at line 500, Add a short inline comment beside the optional property declaration "depositionSupply?: NitrogenSupplyDeposition" clarifying that while the field is optional on the type (e.g., during initial input/enrichment phases), it must be present before running calculations; state that omission is only allowed prior to enrichment/preprocessing and will cause a hard failure at calculation time. Mention the exact symbols "depositionSupply" and "NitrogenSupplyDeposition" in the note so readers know the contract applies to this field.fdm-calculator/src/balance/nitrogen/supply/index.ts (1)
60-65: Fail fast before running the other supply calculations.The missing-deposition guard is correct, but on Line 60 it runs after other costly calculations. Move it to the top of the
tryblock to avoid unnecessary work.♻️ Proposed refactor (fail fast)
export function calculateNitrogenSupply( cultivations: FieldInput["cultivations"], fertilizerApplications: FieldInput["fertilizerApplications"], soilAnalysis: SoilAnalysisPicked, cultivationDetailsMap: Map<string, CultivationDetail>, fertilizerDetailsMap: Map<string, FertilizerDetail>, depositionSupply: NitrogenSupply["deposition"] | undefined, timeFrame: NitrogenBalanceInput["timeFrame"], ): NitrogenSupply { try { + // Guard: deposition data must be present; silently defaulting to zero would mask missing data + if (depositionSupply === undefined) { + throw new Error( + "Missing deposition supply data for nitrogen balance calculation", + ) + } + // Calculate the amount of Nitrogen supplied by fertilizers const fertilizersSupply = calculateNitrogenSupplyByFertilizers( fertilizerApplications, fertilizerDetailsMap, ) @@ - // Guard: deposition data must be present; silently defaulting to zero would mask missing data - if (depositionSupply === undefined) { - throw new Error( - "Missing deposition supply data for nitrogen balance calculation", - ) - } - // Calculate the total amount of Nitrogen supplied const totalSupply = fertilizersSupply.total .add(fixationSupply.total) .add(depositionSupply.total) .add(mineralisationSupply.total)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/balance/nitrogen/supply/index.ts` around lines 60 - 65, Move the existing guard that throws when depositionSupply === undefined to the very start of the surrounding try block so it runs before any other expensive supply calculations; locate the check referencing depositionSupply in index.ts and relocate it above all other computation inside that try (keep the same Error message), ensuring the function/flow that performs the other supply calculations only executes after this guard passes.fdm-core/src/authorization.ts (3)
801-810: Clean refactoring with a minor robustness consideration.The helper consolidates repeated bead-building logic nicely. One consideration: the filter only excludes
nullvalues, notundefined. While Drizzle typically returnsnullfor NULL database values (making this safe), addingundefinedto the filter would make it more defensive.🛡️ Optional defensive improvement
return Object.keys(row) - .filter((k) => row[k] !== null) + .filter((k) => row[k] != null) // excludes both null and undefined .map((k) => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.ts` around lines 801 - 810, In buildBeadsFromRow, make the filter defensive against undefined as well as null so keys with undefined values are excluded; change the predicate from (k) => row[k] !== null to something like (k) => row[k] !== null && row[k] !== undefined (or typeof row[k] === 'string') and keep the map producing { resource: k as Resource, resource_id: row[k] as string } so resource/resource_id remain the same symbols used in the function.
351-396: Non-null assertion onas_organizationmay mask type issues.Line 354 uses
!(non-null assertion) on the result ofand(...). While this works around Drizzle's complex return types, it could mask legitimate issues if the expression were to returnundefined.Consider either:
- Keeping the assertion but adding a comment explaining why it's safe, or
- Using a type guard or conditional check if there's any possibility of
undefined.Also, minor formatting: missing space before
=on line 396.🔧 Formatting fix
- const deduped= new Map< + const deduped = new Map<🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.ts` around lines 351 - 396, The non-null assertion on as_organization (the and(...)! used in the select building for authZSchema.role/as_organization) can hide real type errors; either remove the trailing ! and add a runtime/type guard to ensure the and(...) expression is defined before using it, or keep the assertion but add a short inline comment explaining why the expression cannot be undefined in this context (referencing authZSchema.role, authNSchema.organization and the where() conditions). Also fix the minor formatting by adding a space in the declaration const deduped = new Map<...> so it reads "const deduped = new Map<".
187-192: Audit trail consideration: only first principal logged for arrays.When
principal_idis an array with multiple elements, only the first element is stored in the audit log. While this is likely acceptable (given audit table schema constraints), it means permission checks involving multiple principal IDs (e.g., user + their organizations) won't have complete attribution in the audit record.If full audit trail is needed, consider storing the array as JSON or logging multiple audit entries. Otherwise, this is fine as a pragmatic choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.ts` around lines 187 - 192, The current assignment for principal_id only preserves the first element when principal_id is an array, losing attribution for multi-principal permission checks; update the logic in authorization.ts where principal_id is prepared for the audit record (look for the principal_id ternary block) to either serialize the full array (e.g., JSON string) before storing so the audit column contains all principals, or alternatively emit separate audit entries by iterating over the array and calling the existing audit insert routine for each principal; pick one approach and adjust the principal_id handling and the call sites that build the audit payload accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-calculator/src/balance/nitrogen/supply/index.ts`:
- Around line 60-65: Move the existing guard that throws when depositionSupply
=== undefined to the very start of the surrounding try block so it runs before
any other expensive supply calculations; locate the check referencing
depositionSupply in index.ts and relocate it above all other computation inside
that try (keep the same Error message), ensuring the function/flow that performs
the other supply calculations only executes after this guard passes.
In `@fdm-calculator/src/balance/nitrogen/types.d.ts`:
- Line 500: Add a short inline comment beside the optional property declaration
"depositionSupply?: NitrogenSupplyDeposition" clarifying that while the field is
optional on the type (e.g., during initial input/enrichment phases), it must be
present before running calculations; state that omission is only allowed prior
to enrichment/preprocessing and will cause a hard failure at calculation time.
Mention the exact symbols "depositionSupply" and "NitrogenSupplyDeposition" in
the note so readers know the contract applies to this field.
In `@fdm-core/src/authorization.ts`:
- Around line 801-810: In buildBeadsFromRow, make the filter defensive against
undefined as well as null so keys with undefined values are excluded; change the
predicate from (k) => row[k] !== null to something like (k) => row[k] !== null
&& row[k] !== undefined (or typeof row[k] === 'string') and keep the map
producing { resource: k as Resource, resource_id: row[k] as string } so
resource/resource_id remain the same symbols used in the function.
- Around line 351-396: The non-null assertion on as_organization (the and(...)!
used in the select building for authZSchema.role/as_organization) can hide real
type errors; either remove the trailing ! and add a runtime/type guard to ensure
the and(...) expression is defined before using it, or keep the assertion but
add a short inline comment explaining why the expression cannot be undefined in
this context (referencing authZSchema.role, authNSchema.organization and the
where() conditions). Also fix the minor formatting by adding a space in the
declaration const deduped = new Map<...> so it reads "const deduped = new Map<".
- Around line 187-192: The current assignment for principal_id only preserves
the first element when principal_id is an array, losing attribution for
multi-principal permission checks; update the logic in authorization.ts where
principal_id is prepared for the audit record (look for the principal_id ternary
block) to either serialize the full array (e.g., JSON string) before storing so
the audit column contains all principals, or alternatively emit separate audit
entries by iterating over the array and calling the existing audit insert
routine for each principal; pick one approach and adjust the principal_id
handling and the call sites that build the audit payload accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e84bba86-2fd3-4601-93b5-bbea5cf81fb4
📒 Files selected for processing (6)
fdm-calculator/src/balance/nitrogen/input.tsfdm-calculator/src/balance/nitrogen/supply/index.tsfdm-calculator/src/balance/nitrogen/types.d.tsfdm-core/src/authorization.tsfdm-core/src/farm.test.tsfdm-core/src/soil.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- fdm-calculator/src/balance/nitrogen/input.ts
- fdm-core/src/soil.ts
- fdm-core/src/farm.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-core/src/authorization.ts`:
- Around line 187-192: The audit entry currently collapses an array principal_id
to principal_id[0], which can record the wrong actor; modify the audit
construction in authorization.ts so the audit's principal_id stores the matched
principal (the exact evaluated ID that granted access) when available, or the
full evaluated set otherwise instead of always taking the first element. Locate
where principal_id is used in the audit object (the principal_id: ...
expression) and change the logic to prefer a matchedPrincipal value produced by
the authorization check (or, if none, keep the entire principal_id array or the
original string) so the audit trail reflects the actual granting identity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c80d3aa8-e77f-4c9d-9f77-5d527d8af174
📒 Files selected for processing (2)
fdm-calculator/src/balance/nitrogen/supply/index.tsfdm-core/src/authorization.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-calculator/src/balance/nitrogen/supply/index.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
fdm-core/src/authorization.ts (2)
810-819: Helper centralizes bead construction - consider adding a comment for maintainability.The casts
k as Resourceandrow[k] as stringare safe given current usage where select columns exactly match Resource type names. A brief comment would help future maintainers understand this constraint.💡 Suggested documentation
+/** + * Builds resource beads from a query row. + * IMPORTANT: Row keys must be valid Resource type names (farm, field, etc.) + * as they are cast directly to Resource without runtime validation. + */ function buildBeadsFromRow( row: Record<string, unknown>, ): Array<{ resource: Resource; resource_id: string }> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.ts` around lines 810 - 819, Add a brief explanatory comment above the buildBeadsFromRow function stating the invariant that the input row's keys correspond exactly to Resource names and the values are non-null string IDs (hence the safe casts k as Resource and row[k] as string); mention that callers must ensure select columns match Resource names and non-null values so the casts remain valid and to prevent accidental misuse during future changes.
353-404: Type assertions bypass compile-time safety.The
as unknown as {...}[]double cast (lines 399-404) and non-null assertion!(line 363) work around Drizzle ORM's type inference limitations but disable TypeScript's ability to catch mismatches between the select shape and the expected type.Consider extracting the expected row type and using a type guard or schema validation for runtime safety, especially since this function is part of the authorization system.
♻️ Example: Extract type and add minimal validation
// Define the expected row shape interface RoleQueryRow { role: Role principal_id: string as_organization_member: boolean as_organization: boolean } // Then use a simple runtime check instead of double cast const result = await tx .select({ /* ... */ }) // ... // Validate first row has expected shape (optional runtime check) if (result.length > 0 && typeof result[0].role !== 'string') { throw new Error('Unexpected query result shape') } const typedResult = result as RoleQueryRow[]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.ts` around lines 353 - 404, The current double-cast "as unknown as { ... }[]" on the result of tx.select and the non-null assertion (!) bypass TypeScript safety; extract a named interface (e.g., RoleQueryRow) describing { principal_id: string; role: Role; as_organization_member: boolean; as_organization: boolean }, replace the "as unknown as" cast with a typed variable of that interface, remove the non-null assertion on the select expression, and add a small runtime validation or type guard after the query (check result length and primitive types of key fields) to throw if the row shape is unexpected; update usages that rely on authZSchema.role/authNSchema.member select results to use the validated RoleQueryRow[] instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-core/src/authorization.ts`:
- Around line 810-819: Add a brief explanatory comment above the
buildBeadsFromRow function stating the invariant that the input row's keys
correspond exactly to Resource names and the values are non-null string IDs
(hence the safe casts k as Resource and row[k] as string); mention that callers
must ensure select columns match Resource names and non-null values so the casts
remain valid and to prevent accidental misuse during future changes.
- Around line 353-404: The current double-cast "as unknown as { ... }[]" on the
result of tx.select and the non-null assertion (!) bypass TypeScript safety;
extract a named interface (e.g., RoleQueryRow) describing { principal_id:
string; role: Role; as_organization_member: boolean; as_organization: boolean },
replace the "as unknown as" cast with a typed variable of that interface, remove
the non-null assertion on the select expression, and add a small runtime
validation or type guard after the query (check result length and primitive
types of key fields) to throw if the row shape is unexpected; update usages that
rely on authZSchema.role/authNSchema.member select results to use the validated
RoleQueryRow[] instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: da385921-068c-4be6-885f-1cfbe7fbb2d2
📒 Files selected for processing (2)
fdm-calculator/src/balance/nitrogen/supply/index.tsfdm-core/src/authorization.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-calculator/src/balance/nitrogen/supply/index.ts
Collaborator
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-core/src/authorization.ts (1)
370-373:⚠️ Potential issue | 🔴 CriticalImport
sqlfrom drizzle-orm and wrap theand()expression to fix the TS2322 type error.
and(...)is typed asSQL<unknown> | undefinedin Drizzle, which cannot be directly projected in.select(). TheisNotNull()on the previous line works because it returnsSQL<boolean>.Wrap the
and()call withsql<boolean>`` or coalesce it with?? sql`false`` before projecting:Suggested fix
as_organization: sql<boolean>`${ and( isNotNull(authNSchema.organization.id), inArray(authZSchema.role.principal_id, principal_ids), ) ?? sql`false` }`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.ts` around lines 370 - 373, The projected field as_organization currently uses and(...) which is typed as SQL<unknown> and causes TS2322; import sql from drizzle-orm and wrap the and(...) expression so it yields SQL<boolean> before projecting in the select. Replace the raw and(...) usage inside the as_organization projection with a sql<boolean> template that embeds and(isNotNull(authNSchema.organization.id), inArray(authZSchema.role.principal_id, principal_ids)) and fallback to sql`false` (or coalesce) to ensure a boolean SQL type is returned for the select.
🧹 Nitpick comments (1)
fdm-core/src/authorization.ts (1)
411-423: The new runtime guard still accepts invalid role strings.
typeof row.role === "string"does not prove that the value is one of the allowedRoleliterals, so the subsequent cast can still let unexpected DB values through. If this block is meant to harden row shapes, validate membership againstroleshere too.♻️ Minimal hardening
const result: RoleQueryRow[] = rows.map((row) => { if ( typeof row.principal_id !== "string" || typeof row.role !== "string" || + !roles.includes(row.role as Role) || typeof row.as_organization_member !== "boolean" || typeof row.as_organization !== "boolean" ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.ts` around lines 411 - 423, The runtime guard in getRolesOfPrincipalForResource accepts any string for row.role; update the check to ensure row.role is one of the allowed Role literals (e.g., validate membership against the roles collection/enum) before casting to RoleQueryRow. Specifically, inside the map that builds RoleQueryRow validate row.role via roles.includes(row.role) or an equivalent Role-safe predicate, throw the same error if it fails, and only then return/ cast to RoleQueryRow so no unexpected DB role strings slip through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@fdm-core/src/authorization.ts`:
- Around line 370-373: The projected field as_organization currently uses
and(...) which is typed as SQL<unknown> and causes TS2322; import sql from
drizzle-orm and wrap the and(...) expression so it yields SQL<boolean> before
projecting in the select. Replace the raw and(...) usage inside the
as_organization projection with a sql<boolean> template that embeds
and(isNotNull(authNSchema.organization.id),
inArray(authZSchema.role.principal_id, principal_ids)) and fallback to
sql`false` (or coalesce) to ensure a boolean SQL type is returned for the
select.
---
Nitpick comments:
In `@fdm-core/src/authorization.ts`:
- Around line 411-423: The runtime guard in getRolesOfPrincipalForResource
accepts any string for row.role; update the check to ensure row.role is one of
the allowed Role literals (e.g., validate membership against the roles
collection/enum) before casting to RoleQueryRow. Specifically, inside the map
that builds RoleQueryRow validate row.role via roles.includes(row.role) or an
equivalent Role-safe predicate, throw the same error if it fails, and only then
return/ cast to RoleQueryRow so no unexpected DB role strings slip through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63003d2f-fb99-4f17-a709-2267def271f3
📒 Files selected for processing (1)
fdm-core/src/authorization.ts
BoraIneviNMI
approved these changes
Apr 9, 2026
Collaborator
BoraIneviNMI
left a comment
BoraIneviNMI
left a comment
There was a problem hiding this comment.
Everything seems to work still, both in the pipeline and locally.
I think this PR cleans up the codebase and gets rid of unused things so will improve the maintainability too.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fdm-core/src/authorization.ts (1)
852-870: Consider adding runtime validation for defensive consistency.The helper relies on the JSDoc invariant that callers pass correct column names matching
Resourcetypes. Given that this file adds runtime validation elsewhere (isRoleQueryRow,listPrincipalsForResource), consider validating that each key is a validResourcefor consistency:function buildBeadsFromRow( row: Record<string, unknown>, ): Array<{ resource: Resource; resource_id: string }> { return Object.keys(row) .filter((k) => row[k] !== null && row[k] !== undefined) - .map((k) => ({ - resource: k as Resource, - resource_id: row[k] as string, - })) + .map((k) => { + if (!resources.includes(k as Resource)) { + throw new Error(`Invalid resource key in buildBeadsFromRow: ${k}`) + } + return { + resource: k as Resource, + resource_id: row[k] as string, + } + }) }This would catch column name typos in the select statements during development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/authorization.ts` around lines 852 - 870, buildBeadsFromRow currently trusts the JSDoc invariant; add runtime validation to ensure each key in row is a known Resource and each value is a non-null string: iterate over Object.keys(row) and for each key verify it exists in the Resource set (use the same enum/union or helper used by isRoleQueryRow/listPrincipalsForResource) and that row[k] is a string and not empty; if a key is invalid or the value is not a string, throw a clear Error (or assert) identifying the bad key and value so SQL/select typos surface during development, otherwise continue to map to { resource: k as Resource, resource_id: row[k] as string }.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-core/src/authorization.ts`:
- Around line 852-870: buildBeadsFromRow currently trusts the JSDoc invariant;
add runtime validation to ensure each key in row is a known Resource and each
value is a non-null string: iterate over Object.keys(row) and for each key
verify it exists in the Resource set (use the same enum/union or helper used by
isRoleQueryRow/listPrincipalsForResource) and that row[k] is a string and not
empty; if a key is invalid or the value is not a string, throw a clear Error (or
assert) identifying the bad key and value so SQL/select typos surface during
development, otherwise continue to map to { resource: k as Resource,
resource_id: row[k] as string }.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a0b9ab1-0ac4-4cde-bf1c-6808fa82f45b
📒 Files selected for processing (1)
fdm-core/src/authorization.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Closes #558