Replace WKT with GeoJSON as geometry format #80
Conversation
🦋 Changeset detectedLatest commit: 157f210 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
Warning Rate limit exceeded@SvenVw has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThe changes update how geometric data is handled throughout the codebase. The representation of the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Request
participant R as Route Loader
participant D as Database
participant G as Geometry Handler
participant UI as UI Component
U->>R: Request field data
R->>D: Query field with GeoJSON geometry
D-->>R: Return field data (GeoJSON)
alt Parsing/Conversion
R->>G: Validate/transform geometry data (no wkx conversion)
G-->>R: Return valid GeoJSON geometry
else Direct Assignment
Note over R: Geometry assigned directly as GeoJSON
end
R->>UI: Render field with GeoJSON geometry
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
==========================================
- Coverage 94.19% 93.26% -0.93%
==========================================
Files 27 27
Lines 3563 3830 +267
Branches 259 274 +15
==========================================
+ Hits 3356 3572 +216
- Misses 207 258 +51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
fdm-core/src/db/schema-custom-types.ts (1)
83-131: Remove unused performance timestamp and redundant try/catch.
The variablestartMsis never used, and the baretry { ... } catch(e) { throw e }rethrows without alteration. Simplify as follows:export const parseHexToGeometry = (hex: string): GeoJSON.Geometry => { - const startMs = performance.now() try { ... } catch (e) { - throw e + throw new Error("Failed to parse hex geometry", { cause: e }) } }fdm-core/src/field.ts (1)
199-295: Consider including geometry in error context.While the error handling structure has been improved by encapsulating the transaction logic, commenting out
b_geometryin the error context might make debugging geometry-related issues more difficult.Apply this diff to include geometry in error context:
- // b_geometry, + b_geometry,fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx (1)
599-608: Simplify updateField call by removing unnecessary undefined parameters.Based on the retrieved learnings, only
fdmandb_idare required parameters forupdateField. The remaining parameters are optional and don't need to be passed as undefined.- await updateField( - fdm, - b_id, - formValues.b_name, - undefined, - undefined, - undefined, - undefined, - undefined, - ) + await updateField(fdm, b_id, formValues.b_name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
fdm-core/src/db/migrations/0001_loud_fallen_one.sqlis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/0002_clammy_kylun.sqlis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/0003_thick_stature.sqlis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/0001_snapshot.jsonis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/0002_snapshot.jsonis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/0003_snapshot.jsonis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/_journal.jsonis excluded by!fdm-core/src/db/migrations/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.changeset/calm-swans-turn.md(1 hunks).changeset/curly-mayflies-invite.md(1 hunks).changeset/unlucky-eyes-doubt.md(1 hunks)fdm-app/app/routes/farm.$b_id_farm.atlas.elevation.tsx(0 hunks)fdm-app/app/routes/farm.$b_id_farm.atlas.fields.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.atlas.soil.tsx(0 hunks)fdm-app/app/routes/farm.$b_id_farm.field.$b_id.atlas.tsx(1 hunks)fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx(2 hunks)fdm-app/app/routes/farm.create.$b_id_farm.cultivations.tsx(0 hunks)fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx(1 hunks)fdm-app/app/routes/farm.create.$b_id_farm.fields.tsx(0 hunks)fdm-app/package.json(0 hunks)fdm-core/package.json(2 hunks)fdm-core/src/cultivation.test.ts(2 hunks)fdm-core/src/db/schema-custom-types.ts(2 hunks)fdm-core/src/db/schema.ts(3 hunks)fdm-core/src/fertilizer.test.ts(1 hunks)fdm-core/src/field.d.ts(1 hunks)fdm-core/src/field.test.ts(17 hunks)fdm-core/src/field.ts(7 hunks)fdm-core/src/soil.test.ts(1 hunks)
💤 Files with no reviewable changes (5)
- fdm-app/app/routes/farm.$b_id_farm.atlas.soil.tsx
- fdm-app/package.json
- fdm-app/app/routes/farm.create.$b_id_farm.cultivations.tsx
- fdm-app/app/routes/farm.$b_id_farm.atlas.elevation.tsx
- fdm-app/app/routes/farm.create.$b_id_farm.fields.tsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/unlucky-eyes-doubt.md
🧰 Additional context used
🧠 Learnings (1)
fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx (5)
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core has optional parameters after `fdm` and `b_id`. The TypeScript definitions might show 8 required parameters due to a potential version mismatch.
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx:208-208
Timestamp: 2025-01-23T15:17:23.027Z
Learning: The `addField` function in fdm-core should verify field creation within the same transaction by checking the existence of the field and all its required relations (field data, acquiring info, geometry) before resolving its promise.
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: When using `updateField` from fdm-core, all 8 parameters must be provided in order: fdm, b_id, b_name, b_geometry, b_area, b_id_source, b_id_farm, and b_id_farm_source.
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:34:20.850Z
Learning: The `updateField` function in fdm-core has optional parameters that don't need to be passed as undefined. Only `fdm` and `b_id` are required.
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core requires 8 parameters: fdm, b_id (required), and 6 optional parameters (b_name, b_id_source, b_geometry, b_acquiring_date, b_acquiring_method, b_discarding_date).
🔇 Additional comments (22)
fdm-core/package.json (2)
30-32: Enhanced "files" Array Formatting Improves ReadabilityThe reformatting of the
"files"array (now clearly listing"dist") increases clarity and maintainability. This clean structure will help future developers quickly understand which build artifacts are being included in the package.
64-70: Successful Removal of thewkxDependency and Dependency CleanupThe removal of the
"wkx": "^0.5.0"dependency aligns perfectly with the shift to using GeoJSON for geometry. The remaining dependency list (including"postgres": "^3.4.5") correctly reflects the new architectural direction. Please ensure no residual references towkxexist elsewhere in the codebase.fdm-core/src/db/schema-custom-types.ts (4)
2-2: Import statement looks good.
No issues found with the new import forcustomTypeandCustomTypeValues.
73-81: GeometryType enum is well-defined.
No logical issues found. Each geometric type is enumerated clearly.
159-180: Polygon reading logic looks consistent.
ThereadPolygonimplementation respectably uses the passed-inoffsetat all steps. No immediate issues found.
26-71:❓ Verification inconclusive
Verify parameter binding for ST_GeomFromGeoJSON.
The functiongeometry()correctly leveragesST_GeomFromGeoJSON(JSON.stringify(value)). However, confirm that Drizzle ORM handles parameter binding here to avoid injection risks if untrusted data is ever passed in.Generate a script to search for all references to
geometry()so we can confirm usage is always trusted:
🏁 Script executed:
#!/bin/bash # Description: Locate all references to the geometry custom type for usage review. fd --type f --extension ts --exec rg 'geometry\(' {}Length of output: 56
Action Required: Verify Parameter Binding for ST_GeomFromGeoJSON
The current implementation in
fdm-core/src/db/schema-custom-types.tsusesST_GeomFromGeoJSON(JSON.stringify(value))within thetoDrivermethod, relying on Drizzle ORM’s SQL tagged template to handle parameter binding. However, our automated search for references togeometry(in TypeScript files produced no output, so we couldn’t definitively confirm that all invocations are made with trusted data.Please manually verify that:
- All calls to
geometry()in the codebase inject only trusted data.- The Drizzle ORM’s handling of SQL template literals correctly applies parameter binding for the
ST_GeomFromGeoJSONinvocation.Once you’ve reviewed these points, update or confirm the usage accordingly.
fdm-core/src/field.d.ts (1)
4-13: Consistent schema-based typing.
Referencing the type definitions directly from the schema helps maintain alignment with database fields. No issues found.fdm-app/app/routes/farm.$b_id_farm.atlas.fields.tsx (1)
44-44: Handle potential null or invalid GeoJSON geometry.
Currently,field.b_geometryis used directly as the feature’s geometry. If the database stores null or invalid geometry, it might crash the map. Consider a fallback or validation.fdm-app/app/routes/farm.$b_id_farm.field.$b_id.atlas.tsx (1)
51-51: LGTM! Direct GeoJSON assignment.The change aligns with the PR objective of replacing WKT with GeoJSON format. The geometry is now directly assigned without any conversion, which simplifies the code and improves maintainability.
fdm-core/src/soil.test.ts (1)
44-55: LGTM! Well-structured GeoJSON test data.The change improves test readability by using a proper GeoJSON object with explicit type and coordinates, making the geometry structure clearer than the previous WKT string format.
fdm-core/src/field.ts (1)
15-15: LGTM! Updated parameter documentation.The parameter descriptions have been correctly updated to reflect the change from WKT to GeoJSON format.
Also applies to: 182-182
fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx (1)
51-51: LGTM! Simplified geometry handling.The changes improve code maintainability by:
- Removing unused imports
- Directly using GeoJSON geometry without conversion
Also applies to: 237-237
fdm-core/src/db/schema.ts (2)
73-75: LGTM! Explicit geometry type definition improves type safety.The change from
geometryPolygon()togeometry("b_geometry", { type: "Polygon" })enhances type safety and aligns with the transition to GeoJSON format. The comment about PGLite support provides valuable context for future maintenance.
512-514: LGTM! Consistent geometry type definition for sampling points.The change from
geometryMultipoint()togeometry("b_sampling_geometry", { type: "MultiPoint" })maintains consistency with the new geometry type definition approach.fdm-core/src/field.test.ts (2)
8-8: LGTM! Type alias improves code maintainability.The
Polygontype alias derived from the schema ensures type consistency between the schema and tests.
38-49: LGTM! Test assertions properly validate GeoJSON structure.The change to use
toStrictEqualfor geometry comparison is correct as it performs deep equality checks on the GeoJSON object structure.Also applies to: 69-69
fdm-core/src/cultivation.test.ts (2)
57-68: LGTM! Consistent GeoJSON format in cultivation tests.The field geometry is correctly defined using GeoJSON format, maintaining consistency with other test files.
385-396: LGTM! Consistent geometry definition in cultivation plan tests.The field geometry in the cultivation plan tests maintains the same GeoJSON structure and coordinates.
fdm-core/src/fertilizer.test.ts (1)
407-418: LGTM! Consistent GeoJSON format in fertilizer tests.The field geometry is correctly defined using GeoJSON format, maintaining consistency with other test files.
fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx (1)
163-163:❓ Verification inconclusive
Verify GeoJSON format validation.
With the removal of WKT parsing, ensure that
field.b_geometryis always provided in valid GeoJSON format to prevent runtime errors.
🏁 Script executed:
#!/bin/bash # Description: Check if b_geometry is consistently in GeoJSON format across the codebase # Search for any remaining WKT format geometry assignments rg -A 5 'b_geometry.*=.*POINT|b_geometry.*=.*LINESTRING|b_geometry.*=.*POLYGON' # Search for GeoJSON type validations rg -A 5 'b_geometry.*type.*Feature|b_geometry.*type.*Point|b_geometry.*type.*LineString|b_geometry.*type.*Polygon'Length of output: 198
Verify GeoJSON Geometry Format in Loader Function
The direct assignment of
field.b_geometrytofeature.geometry(line 163) now bypasses previous WKT parsing. Please manually verify thatfield.b_geometryis always a well-formed GeoJSON object (including properties liketypeand proper coordinates) to avoid runtime issues. The automated search did not return any GeoJSON validation logic or remaining WKT parsing, so extra caution is advised.
- File:
fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx(line 163)- Confirm that the loader or upstream processing enforces/validates that
b_geometrymeets the GeoJSON specification..changeset/curly-mayflies-invite.md (1)
1-6: LGTM! Appropriate version bump for dependency removal.The patch version bump is appropriate for removing the
wkxdependency, as this is a non-breaking internal implementation change..changeset/calm-swans-turn.md (1)
1-6: LGTM! Appropriate version bump for format change.The minor version bump is appropriate for changing the
b_geometryformat from WKT to GeoJSON, as this is a new feature that maintains backward compatibility.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
fdm-core/src/db/schema-custom-types.ts (4)
26-29: Update documentation to reflect write support.The comment states "only supports reads" but the implementation includes write support via
toDriver. Please update the documentation to accurately reflect the capabilities.
65-67: Enhance error message with value details.The error message could be more helpful by including the actual value that failed to parse.
- throw new Error("Failed to parse geometry", { + throw new Error(`Failed to parse geometry: ${value}`, { cause: e, })
84-84: Remove unused performance measurement.The
startMsvariable is initialized but never used. Either use it to log/monitor parsing performance or remove it.- const startMs = performance.now() try { // ... parsing logic ... - } catch (e) { - throw e - } + } catch (e) { + throw new Error(`Failed to parse hex geometry: ${hex}`, { cause: e }) + }Also applies to: 128-130
219-235: Improve clarity of MultiPoint and MultiLineString parsing.The code reuses
readLineStringfor MultiPoint andreadPolygonfor MultiLineString. While this works due to similar binary structures, it makes the code harder to understand and maintain.Consider creating dedicated readers for clarity:
+function readMultiPoint( + dataView: DataView, + littleEndian: boolean, + offset: number, +): GeoJSON.Position[] { + if (offset + 4 > dataView.byteLength) { + throw new Error('Buffer too small to read MultiPoint') + } + const numPoints = dataView.getUint32(offset, littleEndian) + offset += 4 + const points: GeoJSON.Position[] = [] + for (let i = 0; i < numPoints; i++) { + points.push(readPoint(dataView, littleEndian, offset + (i * 16))) + } + return points +} case GeometryType.MultiPoint: return { type: "MultiPoint", - coordinates: readLineString( + coordinates: readMultiPoint( dataView, littleEndian, offset, ) as GeoJSON.MultiPoint["coordinates"], }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fdm-core/src/db/schema-custom-types.ts(2 hunks)
🔇 Additional comments (2)
fdm-core/src/db/schema-custom-types.ts (2)
73-81: Implement or remove incomplete MultiPolygon and GeometryCollection handling.The enum includes
MultiPolygonandGeometryCollection, but theparseGeometryfunction lacks support for these types.
241-249: LGTM!The type definitions are well-structured and correctly map all geometry types to their GeoJSON equivalents.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
fdm-core/src/db/schema-custom-types.ts (4)
27-72: Add input validation for GeoJSON data.The
toDrivermethod should validate the GeoJSON object before stringifying to ensure data integrity.toDriver(value) { + // Validate GeoJSON structure + if (!value || typeof value !== 'object' || !value.type || !value.coordinates) { + throw new Error('Invalid GeoJSON object') + } return sql`ST_GeomFromGeoJSON(${JSON.stringify(value)})` },
74-131: Add JSDoc comments for better documentation.The geometry parsing functions would benefit from detailed documentation explaining the binary format structure and SRID handling.
+/** + * Parses a PostGIS EWKB (Extended Well-Known Binary) hex string into a GeoJSON geometry. + * The binary format follows the PostGIS EWKB specification: + * - Byte order (1 byte) + * - Geometry type (4 bytes, including optional SRID flag) + * - SRID (4 bytes, optional) + * - Geometry data (variable length) + * @param hex - The EWKB hex string to parse + * @returns A GeoJSON geometry object + * @throws Error if parsing fails + */ export const parseHexToGeometry = (hex: string): GeoJSON.Geometry => {
133-241: Optimize array allocations in geometry reading functions.Consider pre-allocating arrays with known sizes to reduce memory allocations and improve performance.
function readMultiPoint( dataView: DataView, littleEndian: boolean, offset: number, ): GeoJSON.Position[] { if (offset + 4 > dataView.byteLength) { throw new Error("Buffer too small to read MultiPoint") } const numPoints = dataView.getUint32(offset, littleEndian) offset += 4 - const points: GeoJSON.Position[] = [] + const points: GeoJSON.Position[] = new Array(numPoints) for (let i = 0; i < numPoints; i++) { - points.push(readPoint(dataView, littleEndian, offset + i * 16)) + points[i] = readPoint(dataView, littleEndian, offset + i * 16) } return points }
243-319: Improve error messages in geometry parsing.The error messages could be more descriptive to help with debugging.
- throw new Error("GeometryCollection is not supported yet") + throw new Error("GeometryCollection type is not implemented. Please use individual geometry types instead.") - throw new Error("Unsupported geometry type") + throw new Error(`Unknown geometry type: ${type}. Supported types are: ${Object.keys(GeometryType).join(', ')}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fdm-core/codecov.yml(1 hunks)fdm-core/src/db/schema-custom-types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- fdm-core/codecov.yml
🔇 Additional comments (1)
fdm-core/src/db/schema-custom-types.ts (1)
1-320: Great implementation of the GeoJSON geometry handling!The code successfully addresses all previous review comments and provides a robust, type-safe implementation for handling PostGIS geometries. The implementation includes proper error handling, bounds checking, and support for multiple geometry types.
closes #78
Summary by CodeRabbit
wkxlibrary, and removed unused imports to streamline the codebase.These changes improve compatibility and maintainability while ensuring that geometric data is handled more clearly and directly.