feat(core,template): <GeoQuery> — GeoParquet + DuckDB-Wasm spatial query (po-6sr.2)#1647
Merged
Merged
Conversation
…ery with live map overlay (po-6sr.2) Phase 2 of the GIS epic (po-6sr §6): the QUERY tier for geometry. Where Phase 1's <MapPreview> renders any-size geo (PMTiles), <GeoQuery> lets you run spatial SQL over a remote GeoParquet entirely in the browser — no server, no download — and draws the results as a live map overlay. The geospatial analog of the existing CSV→Parquet + DuckDB-Wasm query view. - @portaljs/core: new <GeoQuery> (maplibre-gl + @duckdb/duckdb-wasm + spatial), exported from the public API. Reads a remote GeoParquet IN PLACE over HTTP range requests; INSTALL/LOAD spatial; a bbox-first query (covering-bbox WHERE prunes Parquet row groups via column stats, THEN ST_Intersects refines the exact predicate); results → ST_AsGeoJSON → MapLibre GeoJSON overlay + table + bar chart; a "Query viewport" button rewrites the WHERE to the current map bounds; click-to-inspect does a DuckDB point-in-polygon lookup for full attributes. @duckdb/duckdb-wasm is an optional peer dep. SSR-safe: maplibre + duckdb load via dynamic import() in the browser only. - catalog template: 'geoparquet' DataFormat + local components/GeoQuery.tsx wired into the showcase (next/dynamic ssr:false, chunk loads only for a geoparquet resource). lib/query/duckdb.ts gains a `spatial` flag (LOAD spatial + treat geoparquet as the range-read Parquet path). The @reference/world-boundaries demo is now dual-tier: a PMTiles resource (render) + a GeoParquet twin (query) derived from the same Natural Earth source (Hilbert-sorted, GeoParquet 1.1 covering bbox column), hosted on R2 via Giftless — the two compose on one dataset page. - docs: <GeoQuery> usage + the bbox-first query pattern + GeoJSON-vs-WKB decode guidance in the core README; a "Geo query tier" section (duckdb GeoParquet build recipe) in the template README. Note: chose a MapLibre-native GeoJSON overlay over deck.gl — deck.gl's MapboxOverlay broke the maplibre render (collapsed the map canvas) and added a heavy dependency; the native overlay is proven by <MapPreview>, keeps @portaljs/core lean, and the README documents WKB/GeoArrow + a GPU layer as the path for very large result sets. Verified in a real browser (headed, WebGL): GeoParquet fetched from data.portaljs.com via 206 range reads; the bbox-first spatial query returns the correct rows (49 European countries) and renders as a map overlay; table + chart views work; click-to-inspect returns full point-in-polygon attributes (Czechia); no console errors. Both @portaljs/core and the catalog template build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
7 Skipped Deployments
|
Contributor
📝 WalkthroughWalkthroughThis PR adds a GeoQuery React component (both in ChangesCore GeoQuery component
Catalog example: GeoQuery integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/portaljs-catalog/components/GeoQuery.tsx`:
- Around line 377-380: The inline style on the main container in GeoQuery should
be replaced with Tailwind classes because the height, background color, and map
visibility are all static. Update the JSX in GeoQuery to use Tailwind for the
fixed sizing and background, and switch the view === 'map' toggle to conditional
hidden/block classes, while leaving only truly dynamic inline styles elsewhere
in the component.
- Around line 28-32: The click-to-inspect path in GeoQuery uses hardcoded
geometry/bbox column names, so it can diverge from the viewport query when a
dataset overrides resource.query. Update the GeoQuery click handler to reuse the
same dataset-specific geometry and bbox columns used by the viewport SQL, or
derive both from the same query config, so inspect works with custom GeoParquet
schemas.
- Around line 182-188: The engine initialization in GeoQuery’s open flow is
leaving a rejected promise on openedRef.current because
engine.open(...).then(...) has no immediate rejection handler. Update the open
sequence to attach a catch (or equivalent rejection path) right where
openedRef.current is assigned so failures from open() are handled synchronously,
logged or stored consistently, and the rejection is still propagated for the
later await in run(). Use the existing open/ run flow and openedRef.current as
the key places to fix.
In `@packages/core/README.md`:
- Around line 66-73: The README bbox-first SQL example is using named
placeholders that do not match the implementation. Update the example in the
documentation to show literal values, consistent with buildViewportSql and the
ST_MakeEnvelope / bbox filtering logic, so readers see the same interpolation
style used by the actual query builder.
In `@packages/core/src/ui/GeoQuery/GeoQuery.tsx`:
- Around line 374-416: The async initialization in GeoQuery can still assign
workerRef.current, dbRef.current, and connRef.current after unmount cleanup has
already run, leaving the Worker and AsyncDuckDB orphaned. Add a cancelled check
immediately before those ref assignments in GeoQuery.tsx’s async setup block,
and if cancelled is true, stop without storing the instances so the cleanup path
can fully terminate them.
- Around line 88-120: The SQL built in buildViewportSql and the click-handler
query interpolates column/identifier values directly, so add a dedicated
identifier-escaping helper (separate from sqlStr, which only escapes string
literals) and use it for geometryColumn, bboxColumn, and columns before
inserting them into quoted identifiers. Update all identifier interpolations in
buildViewportSql and the related query construction in GeoQuery to route through
that helper so embedded double quotes cannot alter SQL structure.
- Line 108: The default SELECT built in GeoQuery should exclude both the
geometry column and the bbox column when columns is omitted. Update the select
expression in GeoQuery so the fallback branch in GeoQuery.tsx includes bbox
alongside geom, matching the behavior used by the example component and keeping
the table output clean.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c6f2f61-0009-4776-b4e1-3e164637c657
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
examples/portaljs-catalog/README.mdexamples/portaljs-catalog/components/GeoQuery.tsxexamples/portaljs-catalog/datasets.jsonexamples/portaljs-catalog/lib/providers/types.tsexamples/portaljs-catalog/lib/query/duckdb.tsexamples/portaljs-catalog/lib/query/types.tsexamples/portaljs-catalog/pages/[owner]/[slug].tsxpackages/core/README.mdpackages/core/package.jsonpackages/core/src/ui/GeoQuery/GeoQuery.tsxpackages/core/src/ui/GeoQuery/index.tspackages/core/src/ui/index.ts
Comment on lines
+28
to
+32
| // Default geometry / covering-bbox column names for the demo GeoParquet. A | ||
| // dataset with different names ships its own `resource.query` (which overrides | ||
| // the generated SQL entirely). | ||
| const GEOM = 'geometry' | ||
| const BBOX = 'bbox' |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='examples/portaljs-catalog/components/GeoQuery.tsx'
echo '--- outline ---'
ast-grep outline "$FILE" --view expanded || true
echo '--- relevant lines ---'
sed -n '1,340p' "$FILE" | cat -n | sed -n '1,340p'
echo '--- search for GEOM/BBOX/resource.query usage ---'
rg -n "GEOM|BBOX|resource\.query|buildViewportSql|click|inspect|catch" "$FILE"Repository: datopian/portaljs
Length of output: 18817
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- resource.query references ---'
rg -n "resource\.query|query:" examples/portaljs-catalog components lib -g '!**/node_modules/**' || true
echo '--- GeoParquet / geometry column references ---'
rg -n '"geometry"|"bbox"|ST_Intersects|ST_AsGeoJSON|geoparquet|GeoParquet' examples/portaljs-catalog components lib -g '!**/node_modules/**' || true
echo '--- Resource type definition ---'
fd -a "datasets.*ts" examples/portaljs-catalog components lib || true
fd -a "datasets.ts" examples/portaljs-catalog components lib || trueRepository: datopian/portaljs
Length of output: 8622
Click-to-inspect needs the same geometry/bbox columns as the viewport query.
resource.query can override the viewport SQL, but this click handler still hardcodes "geometry"/"bbox", so custom GeoParquet column names will make inspect fail silently. Thread the same column names through both paths or route the click lookup through the dataset-specific query config.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/portaljs-catalog/components/GeoQuery.tsx` around lines 28 - 32, The
click-to-inspect path in GeoQuery uses hardcoded geometry/bbox column names, so
it can diverge from the viewport query when a dataset overrides resource.query.
Update the GeoQuery click handler to reuse the same dataset-specific geometry
and bbox columns used by the viewport SQL, or derive both from the same query
config, so inspect works with custom GeoParquet schemas.
Comment on lines
+182
to
+188
| // Open the engine once (LOAD spatial + register the remote GeoParquet as `data`). | ||
| openedRef.current = engine | ||
| .open({ url, format: 'geoparquet', spatial: true }) | ||
| .then(() => { | ||
| if (!cancelled) setRanged(engine.ranged) | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Engine-open failure is an unhandled promise rejection.
engine.open(...).then(...) has no rejection handler, so if open() throws (network failure fetching the remote GeoParquet, extension-install failure, etc.) the promise stored in openedRef.current rejects with nobody attached synchronously — this surfaces as an unhandled promise rejection in the browser. The failure is only ever surfaced later, indirectly, when run() awaits openedRef.current inside its own try/catch (e.g., from map.once('load', ... void run(initialSql))), which is not guaranteed to attach before the runtime flags the rejection as unhandled.
🔧 Proposed fix: attach an immediate catch to surface + preserve the error
openedRef.current = engine
.open({ url, format: 'geoparquet', spatial: true })
.then(() => {
if (!cancelled) setRanged(engine.ranged)
})
+ .catch((e) => {
+ if (!cancelled) {
+ setError(e instanceof Error ? e.message : String(e))
+ setLoading(false)
+ }
+ throw e
+ })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/portaljs-catalog/components/GeoQuery.tsx` around lines 182 - 188,
The engine initialization in GeoQuery’s open flow is leaving a rejected promise
on openedRef.current because engine.open(...).then(...) has no immediate
rejection handler. Update the open sequence to attach a catch (or equivalent
rejection path) right where openedRef.current is assigned so failures from
open() are handled synchronously, logged or stored consistently, and the
rejection is still propagated for the later await in run(). Use the existing
open/ run flow and openedRef.current as the key places to fix.
Comment on lines
+377
to
+380
| <div | ||
| className="relative mt-3 w-full overflow-hidden" | ||
| style={{ height: 480, background: '#ece7db', display: view === 'map' ? 'block' : 'none' }} | ||
| > |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Static inline styles should be Tailwind classes.
height: 480, background: '#ece7db', and the view === 'map' block/none toggle are all statically known and don't need style={{}} — they can be expressed with Tailwind (h-[480px] bg-[#ece7db] and a conditional hidden/block class), keeping only the genuinely dynamic values (anchor position at Line 410, bar width at Line 524) as legitimate inline-style exceptions.
♻️ Proposed refactor
<div
- className="relative mt-3 w-full overflow-hidden"
- style={{ height: 480, background: '`#ece7db`', display: view === 'map' ? 'block' : 'none' }}
+ className={`relative mt-3 h-[480px] w-full overflow-hidden bg-[`#ece7db`] ${
+ view === 'map' ? 'block' : 'hidden'
+ }`}
>📝 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
| <div | |
| className="relative mt-3 w-full overflow-hidden" | |
| style={{ height: 480, background: '#ece7db', display: view === 'map' ? 'block' : 'none' }} | |
| > | |
| <div | |
| className={`relative mt-3 h-[480px] w-full overflow-hidden bg-[`#ece7db`] ${ | |
| view === 'map' ? 'block' : 'hidden' | |
| }`} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/portaljs-catalog/components/GeoQuery.tsx` around lines 377 - 380,
The inline style on the main container in GeoQuery should be replaced with
Tailwind classes because the height, background color, and map visibility are
all static. Update the JSX in GeoQuery to use Tailwind for the fixed sizing and
background, and switch the view === 'map' toggle to conditional hidden/block
classes, while leaving only truly dynamic inline styles elsewhere in the
component.
Source: Coding guidelines
Comment on lines
+66
to
+73
| ```sql | ||
| SELECT name, ST_AsGeoJSON(geometry) AS geojson | ||
| FROM data | ||
| WHERE bbox.xmin <= :maxx AND bbox.xmax >= :minx -- 1. prune row groups (Parquet stats) | ||
| AND bbox.ymin <= :maxy AND bbox.ymax >= :miny | ||
| AND ST_Intersects(geometry, ST_MakeEnvelope(:minx,:miny,:maxx,:maxy)) -- 2. exact | ||
| LIMIT 5000 | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
README SQL example uses parameter placeholder syntax not used in the implementation.
The bbox-first query example uses :minx, :maxx etc. as named parameter placeholders, but the actual buildViewportSql function interpolates values directly (e.g., ST_MakeEnvelope(${b.minX}, ...)). This could mislead users into thinking the component uses parameterized queries.
💚 Proposed fix: use literal values to match the implementation
```sql
SELECT name, ST_AsGeoJSON(geometry) AS geojson
FROM data
-WHERE bbox.xmin <= :maxx AND bbox.xmax >= :minx -- 1. prune row groups (Parquet stats)
- AND bbox.ymin <= :maxy AND bbox.ymax >= :miny
- AND ST_Intersects(geometry, ST_MakeEnvelope(:minx,:miny,:maxx,:maxy)) -- 2. exact
+WHERE bbox.xmin <= 180 AND bbox.xmax >= -180 -- 1. prune row groups (Parquet stats)
+ AND bbox.ymin <= 85 AND bbox.ymax >= -85
+ AND ST_Intersects(geometry, ST_MakeEnvelope(-180, -85, 180, 85)) -- 2. exact
LIMIT 5000📝 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
| ```sql | |
| SELECT name, ST_AsGeoJSON(geometry) AS geojson | |
| FROM data | |
| WHERE bbox.xmin <= :maxx AND bbox.xmax >= :minx -- 1. prune row groups (Parquet stats) | |
| AND bbox.ymin <= :maxy AND bbox.ymax >= :miny | |
| AND ST_Intersects(geometry, ST_MakeEnvelope(:minx,:miny,:maxx,:maxy)) -- 2. exact | |
| LIMIT 5000 | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/README.md` around lines 66 - 73, The README bbox-first SQL
example is using named placeholders that do not match the implementation. Update
the example in the documentation to show literal values, consistent with
buildViewportSql and the ST_MakeEnvelope / bbox filtering logic, so readers see
the same interpolation style used by the actual query builder.
Comment on lines
+88
to
+120
| // SQL string literal (single-quote escaped). URLs/identifiers passed to DuckDB. | ||
| function sqlStr(s: string): string { | ||
| return `'${s.replace(/'/g, "''")}'`; | ||
| } | ||
|
|
||
| interface Bounds { | ||
| minX: number; | ||
| minY: number; | ||
| maxX: number; | ||
| maxY: number; | ||
| } | ||
|
|
||
| // The generated bbox-first viewport query: prune row groups on the covering bbox | ||
| // (cheap, uses Parquet stats), THEN refine with the exact ST_Intersects predicate | ||
| // (expensive, only on survivors), and emit GeoJSON for the overlay. | ||
| function buildViewportSql( | ||
| b: Bounds, | ||
| opts: { geom: string; bbox: string; columns: string[]; limit: number } | ||
| ): string { | ||
| const { geom, bbox, columns, limit } = opts; | ||
| const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : "* EXCLUDE (" + `"${geom}"` + ")"; | ||
| const env = `ST_MakeEnvelope(${b.minX}, ${b.minY}, ${b.maxX}, ${b.maxY})`; | ||
| const bboxFilter = bbox | ||
| ? `"${bbox}".xmin <= ${b.maxX} AND "${bbox}".xmax >= ${b.minX} ` + | ||
| `AND "${bbox}".ymin <= ${b.maxY} AND "${bbox}".ymax >= ${b.minY} AND ` | ||
| : ""; | ||
| return ( | ||
| `SELECT ${select}, ST_AsGeoJSON("${geom}") AS ${GEOJSON_COL}\n` + | ||
| `FROM data\n` + | ||
| `WHERE ${bboxFilter}ST_Intersects("${geom}", ${env})\n` + | ||
| `LIMIT ${limit}` | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
SQL identifiers interpolated without escaping — defense-in-depth gap.
Column names from geometryColumn, bboxColumn, and columns props are interpolated directly into SQL quoted identifiers (e.g., "${geom}", "${bbox}".xmin) without escaping embedded double quotes. While these are developer-controlled props (not end-user input), a consumer passing user-derived column names could allow SQL structure alteration. The sqlStr helper escapes single quotes for string literals but there is no equivalent for identifiers.
The static analyzer also flagged the read_parquet call at lines 406–408, though that specific instance uses a hardcoded string and is safe.
🔒 Proposed fix: add an identifier escaper
// SQL string literal (single-quote escaped). URLs/identifiers passed to DuckDB.
function sqlStr(s: string): string {
return `'${s.replace(/'/g, "''")}'`;
}
+
+// SQL quoted identifier (double-quote escaped).
+function sqlIdent(s: string): string {
+ return `"${s.replace(/"/g, '""')}"`;
+}Then use sqlIdent for all identifier interpolations in buildViewportSql and the click handler:
function buildViewportSql(
b: Bounds,
opts: { geom: string; bbox: string; columns: string[]; limit: number }
): string {
const { geom, bbox, columns, limit } = opts;
- const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : "* EXCLUDE (" + `"${geom}"` + ")";
+ const select = columns.length ? columns.map((c) => sqlIdent(c)).join(", ") : `* EXCLUDE (${sqlIdent(geom)})`;
const env = `ST_MakeEnvelope(${b.minX}, ${b.minY}, ${b.maxX}, ${b.maxY})`;
const bboxFilter = bbox
- ? `"${bbox}".xmin <= ${b.maxX} AND "${bbox}".xmax >= ${b.minX} ` +
- `AND "${bbox}".ymin <= ${b.maxY} AND "${bbox}".ymax >= ${b.minY} AND `
+ ? `${sqlIdent(bbox)}.xmin <= ${b.maxX} AND ${sqlIdent(bbox)}.xmax >= ${b.minX} ` +
+ `AND ${sqlIdent(bbox)}.ymin <= ${b.maxY} AND ${sqlIdent(bbox)}.ymax >= ${b.minY} AND `
: "";
return (
- `SELECT ${select}, ST_AsGeoJSON("${geom}") AS ${GEOJSON_COL}\n` +
+ `SELECT ${select}, ST_AsGeoJSON(${sqlIdent(geom)}) AS ${GEOJSON_COL}\n` +
`FROM data\n` +
- `WHERE ${bboxFilter}ST_Intersects("${geom}", ${env})\n` +
+ `WHERE ${bboxFilter}ST_Intersects(${sqlIdent(geom)}, ${env})\n` +
`LIMIT ${limit}`
);
}And in the click handler (lines 426–427):
- `SELECT * EXCLUDE ("${geometryColumn}"${bboxColumn ? `, "${bboxColumn}"` : ""}) ` +
- `FROM data WHERE ST_Intersects("${geometryColumn}", ST_Point(${e.lngLat.lng}, ${e.lngLat.lat})) LIMIT 1`;
+ `SELECT * EXCLUDE (${sqlIdent(geometryColumn)}${bboxColumn ? `, ${sqlIdent(bboxColumn)}` : ""}) ` +
+ `FROM data WHERE ST_Intersects(${sqlIdent(geometryColumn)}, ST_Point(${e.lngLat.lng}, ${e.lngLat.lat})) LIMIT 1`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/ui/GeoQuery/GeoQuery.tsx` around lines 88 - 120, The SQL
built in buildViewportSql and the click-handler query interpolates
column/identifier values directly, so add a dedicated identifier-escaping helper
(separate from sqlStr, which only escapes string literals) and use it for
geometryColumn, bboxColumn, and columns before inserting them into quoted
identifiers. Update all identifier interpolations in buildViewportSql and the
related query construction in GeoQuery to route through that helper so embedded
double quotes cannot alter SQL structure.
Source: Linters/SAST tools
| opts: { geom: string; bbox: string; columns: string[]; limit: number } | ||
| ): string { | ||
| const { geom, bbox, columns, limit } = opts; | ||
| const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : "* EXCLUDE (" + `"${geom}"` + ")"; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude the bbox column from the default SELECT.
When columns is omitted, the generated SELECT is * EXCLUDE ("geometry"), leaving the bbox STRUCT in the output. The example component excludes both geometry and bbox. The bbox struct is not useful in the table view and clutters the output.
💚 Proposed fix
- const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : "* EXCLUDE (" + `"${geom}"` + ")";
+ const select = columns.length
+ ? columns.map((c) => `"${c}"`).join(", ")
+ : `* EXCLUDE ("${geom}"${bbox ? `, "${bbox}"` : ""})`;📝 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
| const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : "* EXCLUDE (" + `"${geom}"` + ")"; | |
| const select = columns.length | |
| ? columns.map((c) => `"${c}"`).join(", ") | |
| : `* EXCLUDE ("${geom}"${bbox ? `, "${bbox}"` : ""})`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/ui/GeoQuery/GeoQuery.tsx` at line 108, The default SELECT
built in GeoQuery should exclude both the geometry column and the bbox column
when columns is omitted. Update the select expression in GeoQuery so the
fallback branch in GeoQuery.tsx includes bbox alongside geom, matching the
behavior used by the example component and keeping the table output clean.
Comment on lines
+374
to
+416
| let workerUrl: string | null = null; | ||
| try { | ||
| const bundles = duckdbMod.getJsDelivrBundles(); | ||
| const bundle = await duckdbMod.selectBundle(bundles); | ||
| workerUrl = URL.createObjectURL( | ||
| new Blob([`importScripts("${bundle.mainWorker}");`], { type: "text/javascript" }) | ||
| ); | ||
| const worker = new Worker(workerUrl); | ||
| const logger = new duckdbMod.ConsoleLogger(duckdbMod.LogLevel.WARNING); | ||
| const db = new duckdbMod.AsyncDuckDB(logger, worker); | ||
| await db.instantiate(bundle.mainModule, bundle.pthreadWorker); | ||
| const conn = await db.connect(); | ||
| workerRef.current = worker; | ||
| dbRef.current = db; | ||
| connRef.current = conn; | ||
|
|
||
| // The spatial extension gives ST_*; it loads in WASM from the extension | ||
| // repository. Once loaded, read_parquet auto-decodes the GeoParquet | ||
| // geometry column to the GEOMETRY type. | ||
| await conn.query("INSTALL spatial; LOAD spatial;"); | ||
|
|
||
| // Register the remote file over HTTP and expose it as the VIEW `data` so | ||
| // projection + predicate pushdown reach the file over range requests — | ||
| // directIO=true reads it in place (footer + touched row groups only), | ||
| // never downloading the whole file. | ||
| const absoluteUrl = new URL(url, window.location.href).toString(); | ||
| await db.registerFileURL( | ||
| "geo.parquet", | ||
| absoluteUrl, | ||
| duckdbMod.DuckDBDataProtocol.HTTP, | ||
| true | ||
| ); | ||
| await conn.query( | ||
| `CREATE OR REPLACE VIEW data AS SELECT * FROM read_parquet(${sqlStr("geo.parquet")})` | ||
| ); | ||
| } catch (e) { | ||
| if (workerUrl) URL.revokeObjectURL(workerUrl); | ||
| if (!cancelled) setError(e instanceof Error ? e.message : "Could not open the query engine."); | ||
| return; | ||
| } finally { | ||
| if (workerUrl) URL.revokeObjectURL(workerUrl); | ||
| } | ||
| if (cancelled) return; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Worker/DB can be orphaned if the component unmounts during async init.
If cleanup runs between the cancelled check at line 327 and the ref assignments at lines 386–388, the async function will set workerRef.current, dbRef.current, and connRef.current after cleanup has already nullified them. The worker and DB instances are then never terminated. Adding a cancelled check before the ref assignments closes this window.
🔒 Proposed fix
await db.instantiate(bundle.mainModule, bundle.pthreadWorker);
const conn = await db.connect();
+ if (cancelled) {
+ void conn.close?.();
+ void db.terminate?.();
+ worker.terminate();
+ return;
+ }
workerRef.current = worker;
dbRef.current = db;
connRef.current = conn;📝 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
| let workerUrl: string | null = null; | |
| try { | |
| const bundles = duckdbMod.getJsDelivrBundles(); | |
| const bundle = await duckdbMod.selectBundle(bundles); | |
| workerUrl = URL.createObjectURL( | |
| new Blob([`importScripts("${bundle.mainWorker}");`], { type: "text/javascript" }) | |
| ); | |
| const worker = new Worker(workerUrl); | |
| const logger = new duckdbMod.ConsoleLogger(duckdbMod.LogLevel.WARNING); | |
| const db = new duckdbMod.AsyncDuckDB(logger, worker); | |
| await db.instantiate(bundle.mainModule, bundle.pthreadWorker); | |
| const conn = await db.connect(); | |
| workerRef.current = worker; | |
| dbRef.current = db; | |
| connRef.current = conn; | |
| // The spatial extension gives ST_*; it loads in WASM from the extension | |
| // repository. Once loaded, read_parquet auto-decodes the GeoParquet | |
| // geometry column to the GEOMETRY type. | |
| await conn.query("INSTALL spatial; LOAD spatial;"); | |
| // Register the remote file over HTTP and expose it as the VIEW `data` so | |
| // projection + predicate pushdown reach the file over range requests — | |
| // directIO=true reads it in place (footer + touched row groups only), | |
| // never downloading the whole file. | |
| const absoluteUrl = new URL(url, window.location.href).toString(); | |
| await db.registerFileURL( | |
| "geo.parquet", | |
| absoluteUrl, | |
| duckdbMod.DuckDBDataProtocol.HTTP, | |
| true | |
| ); | |
| await conn.query( | |
| `CREATE OR REPLACE VIEW data AS SELECT * FROM read_parquet(${sqlStr("geo.parquet")})` | |
| ); | |
| } catch (e) { | |
| if (workerUrl) URL.revokeObjectURL(workerUrl); | |
| if (!cancelled) setError(e instanceof Error ? e.message : "Could not open the query engine."); | |
| return; | |
| } finally { | |
| if (workerUrl) URL.revokeObjectURL(workerUrl); | |
| } | |
| if (cancelled) return; | |
| let workerUrl: string | null = null; | |
| try { | |
| const bundles = duckdbMod.getJsDelivrBundles(); | |
| const bundle = await duckdbMod.selectBundle(bundles); | |
| workerUrl = URL.createObjectURL( | |
| new Blob([`importScripts("${bundle.mainWorker}");`], { type: "text/javascript" }) | |
| ); | |
| const worker = new Worker(workerUrl); | |
| const logger = new duckdbMod.ConsoleLogger(duckdbMod.LogLevel.WARNING); | |
| const db = new duckdbMod.AsyncDuckDB(logger, worker); | |
| await db.instantiate(bundle.mainModule, bundle.pthreadWorker); | |
| const conn = await db.connect(); | |
| if (cancelled) { | |
| void conn.close?.(); | |
| void db.terminate?.(); | |
| worker.terminate(); | |
| return; | |
| } | |
| workerRef.current = worker; | |
| dbRef.current = db; | |
| connRef.current = conn; | |
| // The spatial extension gives ST_*; it loads in WASM from the extension | |
| // repository. Once loaded, read_parquet auto-decodes the GeoParquet | |
| // geometry column to the GEOMETRY type. | |
| await conn.query("INSTALL spatial; LOAD spatial;"); | |
| // Register the remote file over HTTP and expose it as the VIEW `data` so | |
| // projection + predicate pushdown reach the file over range requests — | |
| // directIO=true reads it in place (footer + touched row groups only), | |
| // never downloading the whole file. | |
| const absoluteUrl = new URL(url, window.location.href).toString(); | |
| await db.registerFileURL( | |
| "geo.parquet", | |
| absoluteUrl, | |
| duckdbMod.DuckDBDataProtocol.HTTP, | |
| true | |
| ); | |
| await conn.query( | |
| `CREATE OR REPLACE VIEW data AS SELECT * FROM read_parquet(${sqlStr("geo.parquet")})` | |
| ); | |
| } catch (e) { | |
| if (workerUrl) URL.revokeObjectURL(workerUrl); | |
| if (!cancelled) setError(e instanceof Error ? e.message : "Could not open the query engine."); | |
| return; | |
| } finally { | |
| if (workerUrl) URL.revokeObjectURL(workerUrl); | |
| } | |
| if (cancelled) return; |
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 406-408: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/ui/GeoQuery/GeoQuery.tsx` around lines 374 - 416, The async
initialization in GeoQuery can still assign workerRef.current, dbRef.current,
and connRef.current after unmount cleanup has already run, leaving the Worker
and AsyncDuckDB orphaned. Add a cancelled check immediately before those ref
assignments in GeoQuery.tsx’s async setup block, and if cancelled is true, stop
without storing the instances so the cleanup path can fully terminate them.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
examples/portaljs-catalog/components/GeoQuery.tsx (1)
376-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Tailwind classes instead of inline styles for static values.
The
height,background, anddisplaytoggle on this container are static or simply-conditional values that can be expressed as Tailwind classes. The coding guidelines require Tailwind CSS exclusively for styling. The dynamic inline styles elsewhere (popup position at line 410, chart bar width at line 524) are fine since they depend on runtime values.♻️ Proposed Tailwind refactor
<div - className="relative mt-3 w-full overflow-hidden" - style={{ height: 480, background: '`#ece7db`', display: view === 'map' ? 'block' : 'none' }} + className={`relative mt-3 w-full overflow-hidden h-[480px] bg-[`#ece7db`] ${view === 'map' ? '' : 'hidden'}`} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/portaljs-catalog/components/GeoQuery.tsx` around lines 376 - 380, The container in GeoQuery’s map view uses inline styles for static styling, so replace the `style` on the main map wrapper with Tailwind classes in the same JSX block. Keep the conditional visibility logic in `GeoQuery` but express the height and background with Tailwind utilities and use a conditional class for the `view === 'map'` toggle instead of `display`, leaving runtime-dependent inline styles elsewhere unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/ui/GeoQuery/GeoQuery.tsx`:
- Around line 723-763: The custom ResultTable in GeoQuery.tsx should be replaced
with the local components/Table.tsx table implementation to follow the project’s
tabular-data guideline. Update the GeoQuery UI to render the existing data
through the local Table component instead of the inline HTML table, and wire it
to the same columns/rows inputs so formatting and behavior remain consistent.
Make sure to use the package-local components/Table.tsx import path and not
`@portaljs/components`, and keep formatCell/data shaping only where needed before
passing data into Table.
- Around line 493-706: The GeoQuery render tree still relies on many inline
style props, which violates the Tailwind-only styling guideline. Update the
component’s JSX in GeoQuery.tsx to replace the current panel, button, textarea,
map overlay, popup, table, and chart styling with Tailwind utility classes,
using the existing structure in GeoQuery, ResultTable, and ResultChart to locate
the affected elements. Keep the MapLibre base CSS injection as-is if it is
required for the map canvas, but remove component-level inline styling and raw
style usage wherever possible.
- Around line 103-120: The default SELECT in buildViewportSql still includes the
bbox STRUCT column when columns is omitted, so update the SELECT fallback to
exclude both geom and bbox instead of only geom. Use the existing
buildViewportSql helper and its opts fields (geom, bbox, columns, limit) to
mirror the click-to-inspect query behavior, ensuring the generated viewport SQL
does not return the raw bbox object in table output.
- Around line 374-416: The GeoQuery async setup leaks the Web Worker when
initialization fails or when unmount happens mid-setup. In the initialization
flow inside GeoQuery, add cancelled checks immediately after each awaited step
such as selectBundle, instantiate, and connect so the code bails out before
creating or storing a Worker after cleanup has started. Also update the
catch/finally cleanup to explicitly terminate the created Worker instance if it
exists, not just revoke the blob URL, and keep the existing
workerRef/dbRef/connRef assignments only after the setup is still active.
---
Nitpick comments:
In `@examples/portaljs-catalog/components/GeoQuery.tsx`:
- Around line 376-380: The container in GeoQuery’s map view uses inline styles
for static styling, so replace the `style` on the main map wrapper with Tailwind
classes in the same JSX block. Keep the conditional visibility logic in
`GeoQuery` but express the height and background with Tailwind utilities and use
a conditional class for the `view === 'map'` toggle instead of `display`,
leaving runtime-dependent inline styles elsewhere unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c6f2f61-0009-4776-b4e1-3e164637c657
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
examples/portaljs-catalog/README.mdexamples/portaljs-catalog/components/GeoQuery.tsxexamples/portaljs-catalog/datasets.jsonexamples/portaljs-catalog/lib/providers/types.tsexamples/portaljs-catalog/lib/query/duckdb.tsexamples/portaljs-catalog/lib/query/types.tsexamples/portaljs-catalog/pages/[owner]/[slug].tsxpackages/core/README.mdpackages/core/package.jsonpackages/core/src/ui/GeoQuery/GeoQuery.tsxpackages/core/src/ui/GeoQuery/index.tspackages/core/src/ui/index.ts
Comment on lines
+103
to
+120
| function buildViewportSql( | ||
| b: Bounds, | ||
| opts: { geom: string; bbox: string; columns: string[]; limit: number } | ||
| ): string { | ||
| const { geom, bbox, columns, limit } = opts; | ||
| const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : "* EXCLUDE (" + `"${geom}"` + ")"; | ||
| const env = `ST_MakeEnvelope(${b.minX}, ${b.minY}, ${b.maxX}, ${b.maxY})`; | ||
| const bboxFilter = bbox | ||
| ? `"${bbox}".xmin <= ${b.maxX} AND "${bbox}".xmax >= ${b.minX} ` + | ||
| `AND "${bbox}".ymin <= ${b.maxY} AND "${bbox}".ymax >= ${b.minY} AND ` | ||
| : ""; | ||
| return ( | ||
| `SELECT ${select}, ST_AsGeoJSON("${geom}") AS ${GEOJSON_COL}\n` + | ||
| `FROM data\n` + | ||
| `WHERE ${bboxFilter}ST_Intersects("${geom}", ${env})\n` + | ||
| `LIMIT ${limit}` | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bbox column leaks into table output when columns is omitted
When columns is not provided, buildViewportSql generates SELECT * EXCLUDE ("geometry"), ST_AsGeoJSON("geometry") AS geojson .... The bbox STRUCT column (default "bbox") is not excluded, so it appears in the table output as a raw object. The click-to-inspect query (line 426) correctly excludes both geometry and bbox — the viewport query should do the same.
🔧 Proposed fix: exclude bbox column from default SELECT
const { geom, bbox, columns, limit } = opts;
- const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : "* EXCLUDE (" + `"${geom}"` + ")";
+ const exclude = [geom, ...(bbox ? [bbox] : [])].map((c) => `"${c}"`).join(", ");
+ const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : `* EXCLUDE (${exclude})`;
const env = `ST_MakeEnvelope(${b.minX}, ${b.minY}, ${b.maxX}, ${b.maxY})`;📝 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
| function buildViewportSql( | |
| b: Bounds, | |
| opts: { geom: string; bbox: string; columns: string[]; limit: number } | |
| ): string { | |
| const { geom, bbox, columns, limit } = opts; | |
| const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : "* EXCLUDE (" + `"${geom}"` + ")"; | |
| const env = `ST_MakeEnvelope(${b.minX}, ${b.minY}, ${b.maxX}, ${b.maxY})`; | |
| const bboxFilter = bbox | |
| ? `"${bbox}".xmin <= ${b.maxX} AND "${bbox}".xmax >= ${b.minX} ` + | |
| `AND "${bbox}".ymin <= ${b.maxY} AND "${bbox}".ymax >= ${b.minY} AND ` | |
| : ""; | |
| return ( | |
| `SELECT ${select}, ST_AsGeoJSON("${geom}") AS ${GEOJSON_COL}\n` + | |
| `FROM data\n` + | |
| `WHERE ${bboxFilter}ST_Intersects("${geom}", ${env})\n` + | |
| `LIMIT ${limit}` | |
| ); | |
| } | |
| function buildViewportSql( | |
| b: Bounds, | |
| opts: { geom: string; bbox: string; columns: string[]; limit: number } | |
| ): string { | |
| const { geom, bbox, columns, limit } = opts; | |
| const exclude = [geom, ...(bbox ? [bbox] : [])].map((c) => `"${c}"`).join(", "); | |
| const select = columns.length ? columns.map((c) => `"${c}"`).join(", ") : `* EXCLUDE (${exclude})`; | |
| const env = `ST_MakeEnvelope(${b.minX}, ${b.minY}, ${b.maxX}, ${b.maxY})`; | |
| const bboxFilter = bbox | |
| ? `"${bbox}".xmin <= ${b.maxX} AND "${bbox}".xmax >= ${b.minX} ` + | |
| `AND "${bbox}".ymin <= ${b.maxY} AND "${bbox}".ymax >= ${b.minY} AND ` | |
| : ""; | |
| return ( | |
| `SELECT ${select}, ST_AsGeoJSON("${geom}") AS ${GEOJSON_COL}\n` + | |
| `FROM data\n` + | |
| `WHERE ${bboxFilter}ST_Intersects("${geom}", ${env})\n` + | |
| `LIMIT ${limit}` | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/ui/GeoQuery/GeoQuery.tsx` around lines 103 - 120, The
default SELECT in buildViewportSql still includes the bbox STRUCT column when
columns is omitted, so update the SELECT fallback to exclude both geom and bbox
instead of only geom. Use the existing buildViewportSql helper and its opts
fields (geom, bbox, columns, limit) to mirror the click-to-inspect query
behavior, ensuring the generated viewport SQL does not return the raw bbox
object in table output.
Comment on lines
+374
to
+416
| let workerUrl: string | null = null; | ||
| try { | ||
| const bundles = duckdbMod.getJsDelivrBundles(); | ||
| const bundle = await duckdbMod.selectBundle(bundles); | ||
| workerUrl = URL.createObjectURL( | ||
| new Blob([`importScripts("${bundle.mainWorker}");`], { type: "text/javascript" }) | ||
| ); | ||
| const worker = new Worker(workerUrl); | ||
| const logger = new duckdbMod.ConsoleLogger(duckdbMod.LogLevel.WARNING); | ||
| const db = new duckdbMod.AsyncDuckDB(logger, worker); | ||
| await db.instantiate(bundle.mainModule, bundle.pthreadWorker); | ||
| const conn = await db.connect(); | ||
| workerRef.current = worker; | ||
| dbRef.current = db; | ||
| connRef.current = conn; | ||
|
|
||
| // The spatial extension gives ST_*; it loads in WASM from the extension | ||
| // repository. Once loaded, read_parquet auto-decodes the GeoParquet | ||
| // geometry column to the GEOMETRY type. | ||
| await conn.query("INSTALL spatial; LOAD spatial;"); | ||
|
|
||
| // Register the remote file over HTTP and expose it as the VIEW `data` so | ||
| // projection + predicate pushdown reach the file over range requests — | ||
| // directIO=true reads it in place (footer + touched row groups only), | ||
| // never downloading the whole file. | ||
| const absoluteUrl = new URL(url, window.location.href).toString(); | ||
| await db.registerFileURL( | ||
| "geo.parquet", | ||
| absoluteUrl, | ||
| duckdbMod.DuckDBDataProtocol.HTTP, | ||
| true | ||
| ); | ||
| await conn.query( | ||
| `CREATE OR REPLACE VIEW data AS SELECT * FROM read_parquet(${sqlStr("geo.parquet")})` | ||
| ); | ||
| } catch (e) { | ||
| if (workerUrl) URL.revokeObjectURL(workerUrl); | ||
| if (!cancelled) setError(e instanceof Error ? e.message : "Could not open the query engine."); | ||
| return; | ||
| } finally { | ||
| if (workerUrl) URL.revokeObjectURL(workerUrl); | ||
| } | ||
| if (cancelled) return; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Resource leak: Worker not terminated if init fails or component unmounts mid-setup
The async init creates a Worker at line 381 before any cancelled check or ref assignment. Two leak paths exist:
-
Failure path: If
db.instantiate()(line 384) ordb.connect()(line 385) throws, the catch block (lines 409–412) revokes the blob URL but never terminates the worker. SinceworkerRef.currentisn't set yet (line 386 hasn't run), the cleanup function can't reach it either. -
Unmount race: If the component unmounts during
selectBundle(line 377) orinstantiate(line 384), cleanup runs and setscancelled = true, but the async IIFE continues past the onlycancelledcheck at line 327, creates the worker, and setsworkerRef.current = workerat line 386 — after cleanup already nulled the ref. The worker is now unreachable and will never be terminated.
🔧 Proposed fix: add cancelled checks after awaits and terminate worker on failure
let workerUrl: string | null = null;
+ let worker: Worker | null = null;
try {
const bundles = duckdbMod.getJsDelivrBundles();
const bundle = await duckdbMod.selectBundle(bundles);
+ if (cancelled) return;
workerUrl = URL.createObjectURL(
new Blob([`importScripts("${bundle.mainWorker}");`], { type: "text/javascript" })
);
- const worker = new Worker(workerUrl);
+ worker = new Worker(workerUrl);
const logger = new duckdbMod.ConsoleLogger(duckdbMod.LogLevel.WARNING);
const db = new duckdbMod.AsyncDuckDB(logger, worker);
await db.instantiate(bundle.mainModule, bundle.pthreadWorker);
+ if (cancelled) { worker.terminate(); void db.terminate?.(); return; }
const conn = await db.connect();
+ if (cancelled) { worker.terminate(); void db.terminate?.(); return; }
workerRef.current = worker;
dbRef.current = db;
connRef.current = conn;
// ...
} catch (e) {
+ if (worker) worker.terminate();
if (workerUrl) URL.revokeObjectURL(workerUrl);
if (!cancelled) setError(e instanceof Error ? e.message : "Could not open the query engine.");
return;📝 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
| let workerUrl: string | null = null; | |
| try { | |
| const bundles = duckdbMod.getJsDelivrBundles(); | |
| const bundle = await duckdbMod.selectBundle(bundles); | |
| workerUrl = URL.createObjectURL( | |
| new Blob([`importScripts("${bundle.mainWorker}");`], { type: "text/javascript" }) | |
| ); | |
| const worker = new Worker(workerUrl); | |
| const logger = new duckdbMod.ConsoleLogger(duckdbMod.LogLevel.WARNING); | |
| const db = new duckdbMod.AsyncDuckDB(logger, worker); | |
| await db.instantiate(bundle.mainModule, bundle.pthreadWorker); | |
| const conn = await db.connect(); | |
| workerRef.current = worker; | |
| dbRef.current = db; | |
| connRef.current = conn; | |
| // The spatial extension gives ST_*; it loads in WASM from the extension | |
| // repository. Once loaded, read_parquet auto-decodes the GeoParquet | |
| // geometry column to the GEOMETRY type. | |
| await conn.query("INSTALL spatial; LOAD spatial;"); | |
| // Register the remote file over HTTP and expose it as the VIEW `data` so | |
| // projection + predicate pushdown reach the file over range requests — | |
| // directIO=true reads it in place (footer + touched row groups only), | |
| // never downloading the whole file. | |
| const absoluteUrl = new URL(url, window.location.href).toString(); | |
| await db.registerFileURL( | |
| "geo.parquet", | |
| absoluteUrl, | |
| duckdbMod.DuckDBDataProtocol.HTTP, | |
| true | |
| ); | |
| await conn.query( | |
| `CREATE OR REPLACE VIEW data AS SELECT * FROM read_parquet(${sqlStr("geo.parquet")})` | |
| ); | |
| } catch (e) { | |
| if (workerUrl) URL.revokeObjectURL(workerUrl); | |
| if (!cancelled) setError(e instanceof Error ? e.message : "Could not open the query engine."); | |
| return; | |
| } finally { | |
| if (workerUrl) URL.revokeObjectURL(workerUrl); | |
| } | |
| if (cancelled) return; | |
| let workerUrl: string | null = null; | |
| let worker: Worker | null = null; | |
| try { | |
| const bundles = duckdbMod.getJsDelivrBundles(); | |
| const bundle = await duckdbMod.selectBundle(bundles); | |
| if (cancelled) return; | |
| workerUrl = URL.createObjectURL( | |
| new Blob([`importScripts("${bundle.mainWorker}");`], { type: "text/javascript" }) | |
| ); | |
| worker = new Worker(workerUrl); | |
| const logger = new duckdbMod.ConsoleLogger(duckdbMod.LogLevel.WARNING); | |
| const db = new duckdbMod.AsyncDuckDB(logger, worker); | |
| await db.instantiate(bundle.mainModule, bundle.pthreadWorker); | |
| if (cancelled) { worker.terminate(); void db.terminate?.(); return; } | |
| const conn = await db.connect(); | |
| if (cancelled) { worker.terminate(); void db.terminate?.(); return; } | |
| workerRef.current = worker; | |
| dbRef.current = db; | |
| connRef.current = conn; | |
| // The spatial extension gives ST_*; it loads in WASM from the extension | |
| // repository. Once loaded, read_parquet auto-decodes the GeoParquet | |
| // geometry column to the GEOMETRY type. | |
| await conn.query("INSTALL spatial; LOAD spatial;"); | |
| // Register the remote file over HTTP and expose it as the VIEW `data` so | |
| // projection + predicate pushdown reach the file over range requests — | |
| // directIO=true reads it in place (footer + touched row groups only), | |
| // never downloading the whole file. | |
| const absoluteUrl = new URL(url, window.location.href).toString(); | |
| await db.registerFileURL( | |
| "geo.parquet", | |
| absoluteUrl, | |
| duckdbMod.DuckDBDataProtocol.HTTP, | |
| true | |
| ); | |
| await conn.query( | |
| `CREATE OR REPLACE VIEW data AS SELECT * FROM read_parquet(${sqlStr("geo.parquet")})` | |
| ); | |
| } catch (e) { | |
| if (worker) worker.terminate(); | |
| if (workerUrl) URL.revokeObjectURL(workerUrl); | |
| if (!cancelled) setError(e instanceof Error ? e.message : "Could not open the query engine."); | |
| return; | |
| } finally { | |
| if (workerUrl) URL.revokeObjectURL(workerUrl); | |
| } | |
| if (cancelled) return; |
🧰 Tools
🪛 ESLint
[error] 378-378: 'URL' is not defined.
(no-undef)
[error] 379-379: 'Blob' is not defined.
(no-undef)
[error] 381-381: 'Worker' is not defined.
(no-undef)
[error] 399-399: 'URL' is not defined.
(no-undef)
[error] 399-399: 'window' is not defined.
(no-undef)
[error] 410-410: 'URL' is not defined.
(no-undef)
[error] 414-414: 'URL' is not defined.
(no-undef)
🪛 OpenGrep (1.23.0)
[ERROR] 406-408: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/ui/GeoQuery/GeoQuery.tsx` around lines 374 - 416, The
GeoQuery async setup leaks the Web Worker when initialization fails or when
unmount happens mid-setup. In the initialization flow inside GeoQuery, add
cancelled checks immediately after each awaited step such as selectBundle,
instantiate, and connect so the code bails out before creating or storing a
Worker after cleanup has started. Also update the catch/finally cleanup to
explicitly terminate the created Worker instance if it exists, not just revoke
the blob URL, and keep the existing workerRef/dbRef/connRef assignments only
after the setup is still active.
Comment on lines
+493
to
+706
| return ( | ||
| <div style={{ fontFamily: "ui-sans-serif, system-ui, -apple-system, 'Segoe UI', sans-serif" }}> | ||
| {/* SQL panel. */} | ||
| <div style={{ border: "1px solid rgba(0,0,0,0.18)", marginBottom: 10 }}> | ||
| <textarea | ||
| value={draft} | ||
| onChange={(e) => setDraft(e.target.value)} | ||
| onKeyDown={(e) => { | ||
| if ((e.metaKey || e.ctrlKey) && e.key === "Enter") void run(); | ||
| }} | ||
| rows={5} | ||
| spellCheck={false} | ||
| disabled={!ready} | ||
| style={{ | ||
| display: "block", | ||
| width: "100%", | ||
| resize: "vertical", | ||
| border: 0, | ||
| boxSizing: "border-box", | ||
| padding: "12px 14px", | ||
| fontFamily: "ui-monospace, SFMono-Regular, Menlo, monospace", | ||
| fontSize: 13, | ||
| lineHeight: 1.5, | ||
| color: "#1c1c1c", | ||
| background: "#f6f4ee", | ||
| outline: "none", | ||
| }} | ||
| /> | ||
| <div | ||
| style={{ | ||
| display: "flex", | ||
| alignItems: "center", | ||
| justifyContent: "space-between", | ||
| gap: 8, | ||
| borderTop: "1px solid rgba(0,0,0,0.15)", | ||
| padding: "8px 12px", | ||
| flexWrap: "wrap", | ||
| }} | ||
| > | ||
| <span style={{ fontSize: 11, color: "rgba(0,0,0,0.45)", fontFamily: "ui-monospace, monospace" }}> | ||
| DuckDB-Wasm + spatial · queried in place over HTTP range requests · ⌘/Ctrl+Enter | ||
| </span> | ||
| <span style={{ display: "flex", gap: 8 }}> | ||
| <button | ||
| type="button" | ||
| onClick={queryViewport} | ||
| disabled={!ready || loading} | ||
| style={btnStyle(false, color)} | ||
| title="Rewrite the query to the current map viewport (bbox pre-filter → ST_Intersects)" | ||
| > | ||
| Query viewport | ||
| </button> | ||
| <button type="button" onClick={() => void run()} disabled={!ready || loading} style={btnStyle(true, color)}> | ||
| {loading ? "Running…" : "Run ▶"} | ||
| </button> | ||
| </span> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* View switcher. */} | ||
| <div style={{ display: "flex", gap: 4, marginBottom: 8 }}> | ||
| {(["map", "table", "chart"] as ViewMode[]).map((m) => ( | ||
| <button | ||
| key={m} | ||
| type="button" | ||
| onClick={() => setView(m)} | ||
| style={{ | ||
| border: "1px solid rgba(0,0,0,0.2)", | ||
| background: view === m ? color : "#fff", | ||
| color: view === m ? "#fff" : "#333", | ||
| padding: "4px 12px", | ||
| fontSize: 11, | ||
| textTransform: "uppercase", | ||
| letterSpacing: "0.05em", | ||
| cursor: "pointer", | ||
| }} | ||
| > | ||
| {m} | ||
| </button> | ||
| ))} | ||
| <span style={{ marginLeft: "auto", alignSelf: "center", fontSize: 11, color: "rgba(0,0,0,0.4)", fontFamily: "ui-monospace, monospace" }}> | ||
| {output.rows.length} rows | ||
| </span> | ||
| </div> | ||
|
|
||
| {error && ( | ||
| <pre | ||
| style={{ | ||
| overflowX: "auto", | ||
| border: "1px solid #f0b4b4", | ||
| background: "#fdf1f1", | ||
| color: "#a33", | ||
| padding: 12, | ||
| fontSize: 12, | ||
| fontFamily: "ui-monospace, monospace", | ||
| whiteSpace: "pre-wrap", | ||
| }} | ||
| > | ||
| {error} | ||
| </pre> | ||
| )} | ||
|
|
||
| {/* Map view. */} | ||
| <div | ||
| style={{ | ||
| position: "relative", | ||
| height, | ||
| width: "100%", | ||
| overflow: "hidden", | ||
| background: "#e6e2d8", | ||
| display: view === "map" ? "block" : "none", | ||
| }} | ||
| > | ||
| <div ref={containerRef} style={{ position: "absolute", inset: 0 }} /> | ||
|
|
||
| <div style={{ position: "absolute", top: 10, right: 10, display: "flex", flexDirection: "column", gap: 1, zIndex: 2 }}> | ||
| {[ | ||
| { label: "+", title: "Zoom in", delta: 1 }, | ||
| { label: "−", title: "Zoom out", delta: -1 }, | ||
| ].map((b) => ( | ||
| <button | ||
| key={b.label} | ||
| type="button" | ||
| aria-label={b.title} | ||
| title={b.title} | ||
| onClick={() => zoomBy(b.delta)} | ||
| style={{ | ||
| width: 28, | ||
| height: 28, | ||
| border: "1px solid rgba(0,0,0,0.25)", | ||
| background: "#fff", | ||
| color: "#222", | ||
| fontSize: 16, | ||
| lineHeight: "26px", | ||
| cursor: "pointer", | ||
| padding: 0, | ||
| }} | ||
| > | ||
| {b.label} | ||
| </button> | ||
| ))} | ||
| </div> | ||
|
|
||
| {inspected && anchor && ( | ||
| <div | ||
| style={{ | ||
| position: "absolute", | ||
| left: anchor.x, | ||
| top: anchor.y, | ||
| transform: "translate(-50%, calc(-100% - 10px))", | ||
| zIndex: 3, | ||
| maxWidth: 280, | ||
| maxHeight: 220, | ||
| overflow: "auto", | ||
| background: "#fff", | ||
| border: "1px solid rgba(0,0,0,0.2)", | ||
| boxShadow: "0 2px 8px rgba(0,0,0,0.15)", | ||
| padding: "8px 10px", | ||
| fontSize: 12, | ||
| }} | ||
| > | ||
| <div style={{ display: "flex", justifyContent: "space-between", alignItems: "baseline", gap: 8, marginBottom: 4 }}> | ||
| <strong style={{ fontSize: 11, textTransform: "uppercase", letterSpacing: "0.05em" }}>Feature</strong> | ||
| <button | ||
| type="button" | ||
| aria-label="Close" | ||
| onClick={() => setInspected(null)} | ||
| style={{ border: "none", background: "none", cursor: "pointer", fontSize: 14, lineHeight: 1, padding: 0, color: "#666" }} | ||
| > | ||
| × | ||
| </button> | ||
| </div> | ||
| {inspectEntries.length === 0 ? ( | ||
| <div style={{ color: "#666", fontStyle: "italic" }}>No properties</div> | ||
| ) : ( | ||
| <table style={{ borderCollapse: "collapse", width: "100%" }}> | ||
| <tbody> | ||
| {inspectEntries.map(([k, v]) => ( | ||
| <tr key={k}> | ||
| <td style={{ padding: "2px 8px 2px 0", color: "#666", verticalAlign: "top", whiteSpace: "nowrap" }}>{k}</td> | ||
| <td style={{ padding: "2px 0", wordBreak: "break-word" }}>{String(v)}</td> | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> | ||
| )} | ||
| </div> | ||
| )} | ||
|
|
||
| {attribution && ( | ||
| <div | ||
| style={{ | ||
| position: "absolute", | ||
| right: 0, | ||
| bottom: 0, | ||
| zIndex: 2, | ||
| background: "rgba(255,255,255,0.75)", | ||
| padding: "2px 6px", | ||
| fontSize: 10, | ||
| color: "#333", | ||
| }} | ||
| > | ||
| {attribution} | ||
| </div> | ||
| )} | ||
| </div> | ||
|
|
||
| {/* Table view. */} | ||
| {view === "table" && <ResultTable columns={output.columns} rows={output.rows} />} | ||
|
|
||
| {/* Chart view. */} | ||
| {view === "chart" && <ResultChart columns={output.columns} rows={output.rows} color={color} />} | ||
| </div> | ||
| ); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use Tailwind CSS classes instead of inline styles
Per coding guidelines, **/*.{ts,tsx,css} files should use Tailwind CSS exclusively for styling. The component uses extensive inline style props throughout the render tree (panels, buttons, textarea, table, chart, popup) and injects raw CSS via a <style> element (lines 70–86). The MapLibre base CSS injection is necessary for the map canvas, but all component-level styling should use Tailwind utility classes.
As per coding guidelines: "Use Tailwind CSS exclusively for styling; do not use inline styles or CSS modules unless the project already uses them."
Run the following script to verify whether the project already uses inline styles or has Tailwind configured:
#!/bin/bash
# Check for Tailwind config and inline style usage in existing components
fd -e "config.js" -e "config.ts" -e "config.mjs" --hidden -t f | rg -l "tailwind\|content:\|theme:" || echo "No Tailwind config found"
fd -e "tsx" -e "jsx" -t f packages/core/src/ui | head -5 | xargs rg -l "style={{" || echo "No inline styles found in sampled components"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/ui/GeoQuery/GeoQuery.tsx` around lines 493 - 706, The
GeoQuery render tree still relies on many inline style props, which violates the
Tailwind-only styling guideline. Update the component’s JSX in GeoQuery.tsx to
replace the current panel, button, textarea, map overlay, popup, table, and
chart styling with Tailwind utility classes, using the existing structure in
GeoQuery, ResultTable, and ResultChart to locate the affected elements. Keep the
MapLibre base CSS injection as-is if it is required for the map canvas, but
remove component-level inline styling and raw style usage wherever possible.
Source: Coding guidelines
Comment on lines
+723
to
+763
| function ResultTable({ columns, rows }: { columns: string[]; rows: Record<string, unknown>[] }) { | ||
| if (!rows.length) return <p style={{ fontStyle: "italic", color: "rgba(0,0,0,0.55)" }}>No rows.</p>; | ||
| return ( | ||
| <div style={{ overflowX: "auto", border: "1px solid rgba(0,0,0,0.18)" }}> | ||
| <table style={{ minWidth: "100%", borderCollapse: "collapse", fontSize: 13 }}> | ||
| <thead> | ||
| <tr> | ||
| {columns.map((c) => ( | ||
| <th | ||
| key={c} | ||
| style={{ | ||
| whiteSpace: "nowrap", | ||
| background: "#f0ede5", | ||
| padding: "10px 14px", | ||
| textAlign: "left", | ||
| fontSize: 11, | ||
| textTransform: "uppercase", | ||
| letterSpacing: "0.05em", | ||
| color: "rgba(0,0,0,0.6)", | ||
| }} | ||
| > | ||
| {c} | ||
| </th> | ||
| ))} | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| {rows.map((row, i) => ( | ||
| <tr key={i} style={{ borderTop: "1px solid rgba(0,0,0,0.1)" }}> | ||
| {columns.map((c) => ( | ||
| <td key={c} style={{ whiteSpace: "nowrap", padding: "8px 14px", fontFamily: "ui-monospace, monospace", fontSize: 12.5 }}> | ||
| {formatCell(row[c])} | ||
| </td> | ||
| ))} | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use the local components/Table.tsx component for tabular data
Per coding guidelines, **/*.{ts,tsx} files should use the local components/Table.tsx component (with papaparse + @tanstack/react-table) for displaying tabular data. The custom ResultTable function implements a basic HTML table instead of using the required component, duplicating functionality the project already standardizes.
As per coding guidelines: "Use the local components/Table.tsx component (with papaparse + @tanstack/react-table) for displaying tabular data; do not import from @portaljs/components."
Run the following script to verify whether components/Table.tsx exists and is importable from this package:
#!/bin/bash
# Check for the local Table component
fd "Table.tsx" --type f | head -10
rg -l "papaparse\|`@tanstack/react-table`" --type ts --type tsx | head -10🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/ui/GeoQuery/GeoQuery.tsx` around lines 723 - 763, The
custom ResultTable in GeoQuery.tsx should be replaced with the local
components/Table.tsx table implementation to follow the project’s tabular-data
guideline. Update the GeoQuery UI to render the existing data through the local
Table component instead of the inline HTML table, and wire it to the same
columns/rows inputs so formatting and behavior remain consistent. Make sure to
use the package-local components/Table.tsx import path and not
`@portaljs/components`, and keep formatCell/data shaping only where needed before
passing data into Table.
Source: Coding guidelines
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
1 participant
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.
Phase 2 of the GIS epic (po-6sr §6) — the query tier for geometry. Phase 1 (
<MapPreview>, PMTiles rendering) shipped in #1644; this adds spatial SQL over a remote GeoParquet entirely in the browser (DuckDB-Wasm +spatial), rendered as a live map overlay. Pure frontend, no infra dependency — same shippable-slice pattern as Phase 1.What's in it
@portaljs/core<GeoQuery>(exported): reads a remote GeoParquet in place over HTTP range requests;INSTALL/LOAD spatial; a bbox-first query (covering-bboxWHEREprunes Parquet row groups via column stats, thenST_Intersectsrefines the exact predicate); results →ST_AsGeoJSON→ MapLibre GeoJSON overlay + table + bar chart; a Query viewport button rewrites theWHEREto the current map bounds; click-to-inspect does a DuckDB point-in-polygon lookup for full attributes.@duckdb/duckdb-wasmis an optional peer dep; SSR-safe (dynamicimport()).geoparquetDataFormat + localcomponents/GeoQuery.tsxwired into the showcase (next/dynamicssr:false).lib/query/duckdb.tsgains aspatialflag. The@reference/world-boundariesdemo is now dual-tier — a PMTiles resource (render) + a GeoParquet twin (query) from the same Natural Earth source (Hilbert-sorted, GeoParquet 1.1 covering bbox column), hosted on R2 via Giftless — so<MapPreview>+<GeoQuery>compose on one page.<GeoQuery>usage + bbox-first pattern + GeoJSON-vs-WKB decode guidance (core README); "Geo query tier" duckdb build recipe (template README).Design note
Chose a MapLibre-native GeoJSON overlay over deck.gl: deck.gl's
MapboxOverlaybroke the maplibre render (collapsed the map canvas) and added a heavy dependency. The native overlay is proven by<MapPreview>, keeps@portaljs/corelean, and the README documents WKB/GeoArrow + a GPU layer as the path for very large result sets.Verification (real browser, headed/WebGL)
data.portaljs.comvia 206 range reads@portaljs/coreand the catalog template build cleanDo not merge — leave on the feature branch; mayor lands after CI is green.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation