Chore/20250918#271
Conversation
|
WalkthroughVersion bumps across workflows and package manifests, replacing @mjackson storage/form-data-parser with @remix-run equivalents in three fdm-app routes, and updating pnpm to 10.17.0 throughout. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #271 +/- ##
============================================
Coverage 92.92% 92.92%
============================================
Files 79 79
Lines 12926 12926
Branches 1266 1266
============================================
Hits 12012 12012
Misses 912 912
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.new.upload.tsx (2)
129-133: Bug: fileTypeFromBuffer expects Uint8Array/Buffer, not ArrayBuffer.
This will mis-detect or throw at runtime.Apply:
- const fileBuffer = await fileUpload.arrayBuffer() - const fileType = await fileTypeFromBuffer(fileBuffer) + const fileBytes = new Uint8Array(await fileUpload.arrayBuffer()) + const fileType = await fileTypeFromBuffer(fileBytes)
123-146: Don’t gate on client-provided MIME; rely on magic bytes (+ add size limit).
Current check rejects valid PDFs if the browser sends a wrong type. Use magic‑bytes as the source of truth and cap size to avoid memory pressure.Apply:
- const uploadHandler = async (fileUpload: FileUpload) => { - if ( - fileUpload.fieldName === "soilAnalysisFile" && - fileUpload.type === "application/pdf" - ) { + const uploadHandler = async (fileUpload: FileUpload) => { + if (fileUpload.fieldName === "soilAnalysisFile") { + const MAX_BYTES = 10 * 1024 * 1024 // 10MB + if (typeof fileUpload.size === "number" && fileUpload.size > MAX_BYTES) { + throw new Error("File too large") + } // Check file type based on magic bytes - const fileBuffer = await fileUpload.arrayBuffer() - const fileType = await fileTypeFromBuffer(fileBuffer) + const fileBytes = new Uint8Array(await fileUpload.arrayBuffer()) + const fileType = await fileTypeFromBuffer(fileBytes) if (fileType?.mime !== "application/pdf") { throw new Error("Invalid file type (magic bytes check)") } // We need to create a new File object from the buffer - const file = new File([fileBuffer], fileUpload.name, { - type: fileUpload.type, + const file = new File([fileBytes], fileUpload.name, { + type: fileType.mime, })
🧹 Nitpick comments (9)
package.json (1)
28-32: Deps bump + pnpm 10.17.0: OK. Add Node engine to match CI?Since CI runs Node 22, consider declaring it to prevent local drift.
Add in root package.json (outside the changed lines):
{ "engines": { "node": ">=22" }, "packageManager": "pnpm@10.17.0" }.github/workflows/tests.yml (1)
229-229: Data job: PNPM 10.17.0 OK. Optional hardening.Consider pinning the container image to a digest to reduce supply‑chain drift:
- container: node:${{ matrix.node-version }}-bookworm-slim + container: node:${{ matrix.node-version }}-bookworm-slim@sha256:<digest>fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id.soil.analysis.upload.tsx (3)
122-147: Avoid buffering the whole file; stream to storage and verify header bytes.Current code reads the entire file into memory and then re-wraps it as a File before storing. Use LocalFileStorage with the FileUpload directly (streams) and only inspect a small header for magic bytes.
Apply this diff within the handler:
- const uploadHandler = async (fileUpload: FileUpload) => { + const uploadHandler = async (fileUpload: FileUpload) => { if ( fileUpload.fieldName === "soilAnalysisFile" && fileUpload.type === "application/pdf" ) { - // Check file type based on magic bytes - const fileBuffer = await fileUpload.arrayBuffer() - const fileType = await fileTypeFromBuffer(fileBuffer) + // Check magic bytes on a small header to validate PDF + const header = fileUpload.bytes.subarray(0, 4100) + const fileType = await fileTypeFromBuffer(header) if (fileType?.mime !== "application/pdf") { throw new Error("Invalid file type (magic bytes check)") } - // We need to create a new File object from the buffer - const file = new File([fileBuffer], fileUpload.name, { - type: fileUpload.type, - }) const storageKey = crypto.randomUUID() - await fileStorage.set(storageKey, file) + // Stream upload to storage without buffering + await fileStorage.set(storageKey, fileUpload) return fileStorage.get(storageKey) } throw new Error("Invalid file type (mime check)") }Please confirm your installed file-type version supports
Uint8Array/ArrayBufferinfileTypeFromBuffer(v19+ does). If older, we should adapt accordingly. See docs for accepted input types. (npmjs.com)
122-122: Make upload directory configurable.Consider reading the uploads base path from configuration/env for portability (tests, prod, ephemeral FS).
Example:
const uploadsDir = process.env.UPLOADS_DIR ?? "./uploads/soil_analyses"; const fileStorage = new LocalFileStorage(uploadsDir);
149-149: Add size/count limits to parseFormDataparseFormData supports an uploadHandler (2nd arg) and an options object (3rd arg) — pass conservative limits, e.g.:
const formData = await parseFormData(request, uploadHandler, { maxFiles: 1, maxFileSize: 20 * 1024 * 1024 })fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.new.upload.tsx (1)
121-144: Clarify retention: are stored PDFs intended to persist?
This route doesn’t remove files (unlike the shapefile route). Confirm retention/backup/rotation and access controls for ./uploads/soil_analyses.fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (2)
129-136: Make shapefile extension checks case‑insensitive.
Prevents false negatives on uppercase extensions.Apply:
- const shp_file = files.find((f) => f.name.endsWith(".shp")) - const shx_file = files.find((f) => f.name.endsWith(".shx")) - const dbf_file = files.find((f) => f.name.endsWith(".dbf")) - const prj_file = files.find((f) => f.name.endsWith(".prj")) + const shp_file = files.find((f) => f.name.toLowerCase().endsWith(".shp")) + const shx_file = files.find((f) => f.name.toLowerCase().endsWith(".shx")) + const dbf_file = files.find((f) => f.name.toLowerCase().endsWith(".dbf")) + const prj_file = files.find((f) => f.name.toLowerCase().endsWith(".prj"))
122-127: Optional: enforce per‑file size limits during upload.
Avoids large in‑memory buffers when parsing. You can reject files via FileUpload.size.fdm-docs/package.json (1)
56-57: Align packageManager with workspace (10.17.0).
Root and other packages use pnpm@10.17.0; docs still declare 10.14.0.Apply:
- "packageManager": "pnpm@10.14.0" + "packageManager": "pnpm@10.17.0"
📜 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 (14)
.github/workflows/deploy-docs-test.yml(1 hunks).github/workflows/deploy-docs.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/tests.yml(3 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.new.upload.tsx(1 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id.soil.analysis.upload.tsx(1 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx(1 hunks)fdm-app/package.json(3 hunks)fdm-calculator/package.json(1 hunks)fdm-core/package.json(2 hunks)fdm-data/package.json(1 hunks)fdm-docs/package.json(2 hunks)package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-13T10:33:05.313Z
Learnt from: SvenVw
PR: SvenVw/fdm#0
File: :0-0
Timestamp: 2025-08-13T10:33:05.313Z
Learning: In the fdm project, fdm-calculator integration for new features like b_lu_variety is handled in separate updates from the core data model changes. When fdm-core functions are updated to support new fields, fdm-calculator can consume these enhanced APIs without requiring changes in the same PR that introduces the core functionality.
Applied to files:
fdm-calculator/package.json
📚 Learning: 2025-05-28T07:57:19.217Z
Learnt from: SvenVw
PR: SvenVw/fdm#147
File: fdm-docs/package.json:39-39
Timestamp: 2025-05-28T07:57:19.217Z
Learning: In pnpm workspaces, the syntax `"typescript": "catalog:"` is valid and correct. It references the version defined in the workspace's catalog section in pnpm-workspace.yaml, allowing consistent dependency versions across packages in a monorepo.
Applied to files:
fdm-calculator/package.jsonfdm-data/package.jsonpnpm-workspace.yamlfdm-core/package.json
📚 Learning: 2025-05-28T07:57:19.217Z
Learnt from: SvenVw
PR: SvenVw/fdm#147
File: fdm-docs/package.json:39-39
Timestamp: 2025-05-28T07:57:19.217Z
Learning: In pnpm workspaces, the syntax `"typescript": "catalog:"` is valid and correct. It references the version defined in the workspace's catalog section in pnpm-workspace.yaml, allowing consistent dependency versions across packages in a monorepo. The catalog: syntax without a package name is the shorthand for referencing the default catalog.
Applied to files:
fdm-calculator/package.jsonfdm-data/package.jsonpnpm-workspace.yamlfdm-core/package.json
📚 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:
pnpm-workspace.yamlfdm-app/package.json
🔇 Additional comments (20)
.github/workflows/deploy-docs-test.yml (1)
38-38: PNPM bump looks good.Consistent with repo-wide 10.17.0 upgrade. No other workflow impacts.
.github/workflows/deploy-docs.yml (1)
41-41: Aligned PNPM version.Matches other workflows/packages; safe change.
fdm-data/package.json (1)
60-60: packageManager updated to pnpm@10.17.0.Consistent with the workspace. No concerns.
.github/workflows/tests.yml (2)
56-56: Core job: PNPM 10.17.0 OK.Change is mechanical; cache + setup-node remain intact.
149-149: Calculator job: PNPM 10.17.0 OK.Consistent with other jobs.
.github/workflows/release.yml (1)
40-40: Release workflow PNPM bump: OK.No functional changes; aligns with CI matrix.
fdm-calculator/package.json (1)
64-64: packageManager bump only.Looks good.
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id.soil.analysis.upload.tsx (1)
1-2: Import migration to @remix-run libs: LGTM.API parity retained; matches package changes.
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (1)
1-2: Remix import migration LGTM.
Interfaces match usage here.fdm-docs/package.json (3)
25-25: @mdx-js/react bump OK.
No breaking changes expected for this patch.
27-27: lucide-react bump OK.
Aligns with app package.
38-38: typedoc-plugin-markdown patch bump OK.pnpm-workspace.yaml (1)
9-24: Catalog bumps look consistent.
Rollup/Vite/typedoc updates align with package manifests.If CI is green, no action. If not, check plugin compat with Rollup 4.50.x.
fdm-core/package.json (3)
54-65: DevDependency bumps look safe.
Minor/patch updates only.
67-75: Dependency bumps OK.
Pglite and unique-username-generator patches are low risk.
76-76: pnpm 10.17.0 alignment OK.fdm-app/package.json (3)
28-31: Swapping to @remix-run/ libs is the right move.*
Matches route changes.
107-108: pnpm 10.17.0 alignment OK.
16-103: Approve — verification incomplete: ripgrep returned no files searchedVersion bumps look coherent with the workspace; nothing concerning stands out. Verification for leftover @mjackson imports failed because the ripgrep run returned "No files were searched". Re-run locally and share output:
rg -nP '@mjackson/(file-storage|form-data-parser)'
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.new.upload.tsx (1)
1-2: Remix import migration verified — no @mjackson imports in source files.
Only pnpm-lock.yaml contains '@mjackson/node-fetch-server' entries (transitive); all code files import @remix-run/file-storage and @remix-run/form-data-parser.
1 participant
Summary by CodeRabbit