Add spatial parquet points loading - #51
Closed
xinaesthete wants to merge 38 commits into
Closed
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vis/src/SpatialCanvas/useLayerData.ts (1)
1525-1550:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
reloadElementinvalidates refs but does not trigger a reload/update cycle.This callback only mutates refs. Without a state bump, the load effect and UI selectors may not re-run immediately after manual reload.
🐛 Proposed fix
- const reloadElement = useCallback((type: string, key: string) => { + const reloadElement = useCallback((type: string, key: string) => { const loaded = loadedDataRef.current; ... // The useEffect will pick up the missing data and reload - }, []); + notifyLoadedDataChanged(); + setPointsFeatureCatalogRevision((revision) => revision + 1); + setPointsRowFeatureCodesRevision((revision) => revision + 1); + }, [notifyLoadedDataChanged]);🤖 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/vis/src/SpatialCanvas/useLayerData.ts` around lines 1525 - 1550, The reloadElement callback clears multiple ref caches (pointsRenderResourceCacheRef, pointsFeatureCatalogRef, pointsFeatureCatalogInFlightRef, pointsRowFeatureCodesRef, pointsRowFeatureCodesInFlightRef) but only mutates refs without triggering a React state update. Since refs do not cause re-renders, the useEffect will not re-run to reload the data. Add a state update (such as toggling a reload trigger state or incrementing a counter) within the reloadElement callback alongside the ref mutations to ensure the component re-renders and the load effect picks up the cleared cache and reloads the data as intended.
♻️ Duplicate comments (1)
packages/core/tests/pointsFeatures.spec.ts (1)
17-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
spawnorexecFileinstead ofexecSyncfor external commands.Same command injection risk as in
mortonPointsTiling.spec.ts. AllexecSynccalls should be refactored to usespawnorexecFilewith argument arrays to eliminate shell invocation.Also applies to: 129-149, 187-210, 238-257, 286-299, 324-358
🤖 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/tests/pointsFeatures.spec.ts` around lines 17 - 45, The execSync calls throughout the file (at lines 17-45, 129-149, 187-210, 238-257, 286-299, 324-358) are vulnerable to command injection because they execute shell strings with interpolated variables. Replace all execSync calls with spawn or execFile by writing the Python code to a temporary file first, then executing it with the file path passed as a separate argument array. This eliminates shell invocation and removes the injection risk while maintaining the same functionality of creating the parquet test data files.Source: Linters/SAST tools
🟠 Major comments (26)
packages/layers/src/pointsFeatureCodes.ts-28-39 (1)
28-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
preloadedFeatureCodesSignaturecan miss real row-code changesUsing only
length:first:lastallows collisions (same size/endpoints, different interior values), sofilterBatchSignaturecan remain unchanged while row-feature codes actually changed.💡 Suggested fix
export function preloadedFeatureCodesSignature( featureCodes: ArrayLike<number> | undefined ): string { if (!featureCodes) { return 'nocodes'; } const length = featureCodes.length; if (length === 0) { return 'len:0'; } - return `len:${length}:${featureCodes[0]}:${featureCodes[length - 1]}`; + let hash = 2166136261 >>> 0; // FNV-1a 32-bit + for (let i = 0; i < length; i += 1) { + hash ^= (featureCodes[i] ?? 0) | 0; + hash = Math.imul(hash, 16777619); + } + return `len:${length}:h:${hash >>> 0}`; }Also applies to: 41-48
🤖 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/layers/src/pointsFeatureCodes.ts` around lines 28 - 39, The preloadedFeatureCodesSignature function creates a signature using only the array length and the first and last elements, which can produce identical signatures for arrays with the same length and endpoints but different interior values. This causes filterBatchSignature to miss actual changes in row-feature codes. Modify the function to incorporate all values from the featureCodes array into the signature calculation, such as by computing a hash or checksum of the entire array content, rather than relying solely on length and boundary values.packages/layers/src/PointsLayer.ts-149-176 (1)
149-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against stale async preloads when
resourcechanges
ensurePreloadedBatchcan commit an oldloadAll()result after props switched to a new resource, because there’s no post-await identity check beforesetState. This can briefly render the wrong dataset.💡 Suggested fix
private async ensurePreloadedBatch(): Promise<void> { const { resource } = this.props; + const expectedLoader = resource.loader; + const expectedElement = resource.element; if (resource.loader.capabilities.kind !== 'preloaded-columnar') { return; } const existing = (this.state as PointsLayerState).preloadedBatch; if (existing) { return; } - const batch = await resource.loader.loadAll?.(); + const batch = await expectedLoader.loadAll?.(); + if ( + this.props.resource.loader !== expectedLoader || + this.props.resource.element !== expectedElement + ) { + return; + } if (batch?.format === 'columnar-ndarray') { this.setState({ preloadedBatch: batch });🤖 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/layers/src/PointsLayer.ts` around lines 149 - 176, The ensurePreloadedBatch method has a race condition where an old loadAll() result can be committed after the resource prop has changed. Capture a reference to this.props.resource at the start of the method before awaiting the loader.loadAll() call, then after the await completes and before calling setState with the batch, verify that this.props.resource is still the same resource object that was captured at the start. Only proceed with setState if the resource identity hasn't changed, otherwise return early to prevent stale data from being rendered.packages/vis/demo/src/enableDemoPointsWorker.ts-11-13 (1)
11-13:⚠️ Potential issue | 🟠 MajorRemove the explicit
workerUrloverride and use the default worker resolution.The demo hard-codes a source
.tsfile path, which breaks outside the dev environment. TheenablePointsWorkerfunction already provides a correct default that resolves to the built./points-worker.js(line 112 of pointsWorkerClient.ts). In production builds or distributions, the source.tsfile won't be available, causing worker initialization to fail and forcing point decoding/filtering onto the main thread, degrading responsiveness.Suggested fix
- enablePointsWorker({ - workerUrl: new URL('../../../core/src/workers/points-worker.ts', import.meta.url), - }); + enablePointsWorker();🤖 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/vis/demo/src/enableDemoPointsWorker.ts` around lines 11 - 13, Remove the explicit workerUrl override from the enablePointsWorker function call in the demo file. The enablePointsWorker function has a built-in default that correctly resolves to the compiled points-worker.js file for production environments. Delete the entire workerUrl configuration object parameter so the function uses its default resolution logic, which will prevent worker initialization failures in production builds where the source .ts file is not available.packages/layers/src/pointsTileLoadCallbacks.ts-15-15 (1)
15-15:⚠️ Potential issue | 🟠 MajorNarrow
loadModeto a concrete union of actual runtime values.
loadMode?: stringallows typos and unreachable branches. Based on actual usage across the codebase, the only values emitted are'row-groups','full-filter', and'clipped'. Use a narrowed union type that reflects these constraints, either inline or exported as a shared constant:loadMode?: 'row-groups' | 'full-filter' | 'clipped';As per coding guidelines, types should match runtime behavior. A broad
stringtype defeats static guarantees and allows branches that can never execute under the actual implementation.🤖 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/layers/src/pointsTileLoadCallbacks.ts` at line 15, The `loadMode` property in the pointsTileLoadCallbacks.ts file is typed as a generic string, which allows invalid values and defeats static type checking. Replace the `loadMode?: string;` type annotation with a narrowed union type that only includes the three actual runtime values: the literal types for 'row-groups', 'full-filter', and 'clipped'. This ensures type safety and prevents typos or unreachable code branches at compile time.Source: Coding guidelines
packages/vis/src/SpatialCanvas/renderers/pointsRenderer.ts-105-105 (1)
105-105: 🛠️ Refactor suggestion | 🟠 MajorRemove the unnecessary
as Layerassertion.The
PointsLayerclass extendsCompositeLayerfrom deck.gl, which is a subclass ofLayer. SincePointsLayeris already assignable to the declared return typeLayer, the assertion is redundant and violates the coding guideline to avoid type assertions when proper inheritance proves assignability.Proposed change
export function renderPointsLayer(config: PointsLayerRenderConfig): Layer | null { return new PointsLayer({ id, resource, modelMatrix, opacity, visible, pointSize, pointRadiusMinPixels, pointRadiusMaxPixels, pointMinSizeScale, viewZoom, color, featureCodes, preloadedFeatureCodes, renderCap, showTileDebugOverlay: showTileDebugOverlay ?? true, tileDebugStore, tileDebugSignature, use3d, - }) as Layer; + }); }🤖 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/vis/src/SpatialCanvas/renderers/pointsRenderer.ts` at line 105, The type assertion `as Layer` at line 105 in the pointsRenderer.ts file is unnecessary and should be removed. Since PointsLayer extends CompositeLayer which is already a subclass of Layer, the class is already assignable to the Layer return type without requiring an explicit assertion. Simply remove the `as Layer` portion from the end of the statement, leaving only the closing bracket and semicolon.Source: Coding guidelines
packages/vis/src/SpatialCanvas/useLayerData.ts-1184-1187 (1)
1184-1187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError handling clears
geometryErrorinstead of setting it.Both points error paths call
setLayerGeometryError(layerId, undefined), so the new user-facing geometry error is never populated.🐛 Proposed fix
} catch (error) { setLayerResourceStatus(layerId, 'geometry', 'error'); setLayerGeometryNotice(layerId, undefined); - setLayerGeometryError(layerId, undefined); + setLayerGeometryError( + layerId, + error instanceof Error ? error.message : String(error) + ); console.error(`Failed to load points for ${layerId}:`, error); notifyLoadedDataChanged(); } ... } catch (error) { loadedDataRef.current.pointTilingMetadata.set(element.key, null); setLayerResourceStatus(layerId, 'geometry', 'error'); - setLayerGeometryError(layerId, undefined); + setLayerGeometryError( + layerId, + error instanceof Error ? error.message : String(error) + ); console.error(`Failed to inspect point tiling metadata for ${layerId}:`, error);Also applies to: 1222-1224
🤖 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/vis/src/SpatialCanvas/useLayerData.ts` around lines 1184 - 1187, In the error handling for points loading failures in the useLayerData hook, the code is clearing the geometry error instead of capturing it. In both error paths where setLayerResourceStatus is called with 'geometry' and 'error' status (around lines 1184 and 1222), change the setLayerGeometryError calls from passing undefined to passing the actual error object that is available in scope, so that the user-facing geometry error is properly populated with the error details.packages/vis/src/SpatialCanvas/useLayerData.ts-1749-1764 (1)
1749-1764:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRender-resource cache key is too coarse for multi-layer points setups.
The cache is keyed by
elem.key, but signature varies per layer/config (preloadCacheKey, optimization mode). Two visible layers sharing one element will repeatedly invalidate each other and recreate resources.⚙️ Suggested direction
- let cachedResource = pointsRenderResourceCacheRef.current.get(elem.key); + let cachedResource = pointsRenderResourceCacheRef.current.get(layerId); ... - pointsRenderResourceCacheRef.current.set(elem.key, cachedResource); + pointsRenderResourceCacheRef.current.set(layerId, cachedResource); ... - const cached = pointsRenderResourceCacheRef.current.get(elem.key); + const cached = pointsRenderResourceCacheRef.current.get(layerId);Also clear per-layer cache entries in
reloadElementfor layers targeting the same points element.Also applies to: 2211-2212
🤖 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/vis/src/SpatialCanvas/useLayerData.ts` around lines 1749 - 1764, The cache key for points render resources is keyed only by elem.key, but the signature varies per layer and configuration (preloadCacheKey, optimization mode), causing multiple layers sharing one element to repeatedly invalidate each other's cached resources. Modify the cache keying strategy to be layer-specific so that different layers can maintain independent cached resources for the same element. Additionally, update the reloadElement function to clear per-layer cache entries for all layers targeting the same points element, ensuring that invalidating one layer's cache does not affect other layers' cached resources. The changes should affect the pointsRenderResourceCacheRef cache management and the reloadElement function logic.packages/core/src/models/VPointsSource.ts-253-264 (1)
253-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize worker before enforcing worker-only full-dataset scan.
The full-dataset filtered path can throw even with default worker enablement configured, because it returns before worker initialization runs.
Proposed fix
private async loadPointsMatchingFeatureCodes( elementPath: string, options: { memoryCap: number; featureCodes: readonly number[]; onProgress?: (progress: PointsLoadProgress) => void; } ): Promise<PointsLoadResult> { + ensurePointsWorker(); const parquetPath = getParquetPath(elementPath); const zattrs = await this.loadSpatialDataElementAttrs(elementPath); @@ - if (!isPointsWorkerEnabled()) { + if (!isPointsWorkerEnabled()) { throw new Error( 'Feature-filtered points loading requires the points worker and parquet part bytes.' ); }Also applies to: 349-353
🤖 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/models/VPointsSource.ts` around lines 253 - 264, The loadPoints method has an early return path through loadPointsMatchingFeatureCodes when fullDatasetFeatureScan is true, but worker initialization code runs after this conditional check. Move the worker initialization code to execute before the condition that checks for options.fullDatasetFeatureScan === true, ensuring the worker is properly initialized regardless of which code path is taken through loadPointsMatchingFeatureCodes.packages/core/src/models/VPointsSource.ts-923-925 (1)
923-925:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check abort after fallback full-load resolves.
If the signal aborts while full-data fallback is in flight, execution still proceeds to bounds filtering and returns stale work.
Proposed fix
checkAbort(options.signal); const full = await this.loadPointsWithOptionalFeatureCodes(elementPath, metadata, options); + checkAbort(options.signal); return filterPointsToBounds( full.data, options.bounds,🤖 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/models/VPointsSource.ts` around lines 923 - 925, After the await on loadPointsWithOptionalFeatureCodes completes on line 924, add another checkAbort call before proceeding to the filterPointsToBounds call. The signal could be aborted while the async loadPointsWithOptionalFeatureCodes operation is in flight, so you need to re-check for abort immediately after it resolves and before performing the bounds filtering work to prevent stale results from being returned.packages/core/src/models/VPointsSource.ts-841-848 (1)
841-848:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid caching rejected tiling-metadata promises.
A failed metadata load is cached permanently, so subsequent calls for the same element keep failing without retry.
Proposed fix
async getPointsTilingMetadata(elementPath: string): Promise<PointsTilingMetadata | null> { if (this.pointTilingMetadataCache.has(elementPath)) { return this.pointTilingMetadataCache.get(elementPath) ?? null; } - const promise = this.loadPointsTilingMetadataUncached(elementPath); - this.pointTilingMetadataCache.set(elementPath, promise); - return promise; + const promise = this.loadPointsTilingMetadataUncached(elementPath).catch((error) => { + this.pointTilingMetadataCache.delete(elementPath); + throw error; + }); + this.pointTilingMetadataCache.set(elementPath, promise); + return promise; }🤖 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/models/VPointsSource.ts` around lines 841 - 848, The issue is that rejected promises from loadPointsTilingMetadataUncached are being cached in pointTilingMetadataCache, causing permanent failures for subsequent calls to getPointsTilingMetadata with the same elementPath. To fix this, modify the caching logic so that only successfully resolved metadata is cached, not rejected promises. You can achieve this by adding error handling to the promise before caching it, or by only caching after the promise resolves. Ensure that if the promise rejects, it is either not cached at all or the cache is cleared so that subsequent calls can retry the metadata load operation.python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.py-42-45 (1)
42-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate points keys before using them in filesystem and metadata paths.
At Line 43,
read_points_element_attrs()uses rawpoints_keyinstead ofvalidate_points_key(). At Line 137, consolidated metadata keys are written from unvalidatedelement_keys. This allows unsafe path-like keys and corrupted metadata entries.Suggested fix
def read_points_element_attrs(zarr_path: str | Path, points_key: str) -> dict[str, Any]: - element_json = _points_root(Path(zarr_path)) / points_key / "zarr.json" + safe_key = validate_points_key(points_key) + element_json = _points_root(Path(zarr_path)) / safe_key / "zarr.json" @@ def register_points_elements_in_consolidated_metadata( @@ - template_entry = _points_element_consolidated_entry( + safe_template_key = validate_points_key(template_key) + template_entry = _points_element_consolidated_entry( store_path, - template_key, + safe_template_key, metadata=metadata, ) for key in element_keys: - metadata[f"points/{key}"] = json.loads(json.dumps(template_entry)) + safe_key = validate_points_key(key) + metadata[f"points/{safe_key}"] = json.loads(json.dumps(template_entry))Also applies to: 132-139
🤖 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 `@python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.py` around lines 42 - 45, The `read_points_element_attrs()` function uses the raw `points_key` parameter directly to construct filesystem paths without validation, and the code around line 137 writes consolidated metadata keys from unvalidated `element_keys`. Validate the `points_key` parameter at the beginning of `read_points_element_attrs()` using the `validate_points_key()` function before it is used in the `_points_root(Path(zarr_path)) / points_key / "zarr.json"` path construction. Additionally, validate each key in the `element_keys` collection before writing them to consolidated metadata around line 137 to prevent unsafe path-like keys and corrupted metadata entries.packages/core/src/models/VTableSource.ts-829-833 (1)
829-833:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing parquet parts are silently dropped, causing partial reads.
At Line 831 and Line 860, missing part bytes are skipped with
continue. This can return an incompletepartspayload without surfacing an error, which risks downstream partial decode/data loss.Suggested fix
for (const part of dataset.parts) { const bytes = await this.loadParquetFileBytesAtPath(part.path); - if (bytes) { - parts.push(bytes); - } + if (!bytes) { + throw new Error(`Missing parquet part bytes at ${part.path}`); + } + parts.push(bytes); } return { parts, totalRows, truncated: false }; @@ const partPath = partPaths[partIndex]; const bytes = await this.loadParquetFileBytesAtPath(partPath); - if (!bytes) { - continue; - } + if (!bytes) { + throw new Error(`Missing parquet part bytes at ${partPath}`); + } parts.push(bytes);Also applies to: 859-863
🤖 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/models/VTableSource.ts` around lines 829 - 833, The code silently skips missing parquet parts when loadParquetFileBytesAtPath returns falsy bytes, causing incomplete data to be returned without error. In the loop iterating through dataset.parts, instead of silently continuing when bytes is falsy from the loadParquetFileBytesAtPath call for part.path, throw an error with details about which part failed to load. This should also be applied to the second location mentioned at lines 859-863 where the same pattern occurs. Ensure that missing parquet parts are treated as errors rather than being silently dropped from the parts array.packages/core/src/models/VTableSource.ts-427-435 (1)
427-435:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRow-count fallback is incorrectly coupled to an
xcolumn.At Line 433, the fallback count path projects
columns: ['x']. Any valid parquet file without anxcolumn will fail row counting in this path and break capped loading logic upstream.Suggested fix
private async countRowsFromFullParquetFile(path: string): Promise<number> { const fileBytes = await this.loadParquetFileBytesAtPath(path); if (!fileBytes) { return 0; } const { readParquet } = await SpatialDataTableSource.parquetModulePromise; - const table = await tableFromIPC(readParquet(fileBytes, { columns: ['x'] }).intoIPCStream()); + // Keep fallback schema-agnostic: row counting must not depend on points columns. + const table = await tableFromIPC(readParquet(fileBytes).intoIPCStream()); return table.numRows; }🤖 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/models/VTableSource.ts` around lines 427 - 435, The countRowsFromFullParquetFile method hardcodes columns: ['x'] when reading the parquet file, which will fail for any valid parquet file that doesn't contain an 'x' column. Since the purpose of this method is only to count rows and doesn't require reading specific column data, remove the columns projection from the readParquet call options or pass an empty columns array if the API requires it. This will allow the method to work with parquet files regardless of their schema.python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/verify.py-292-306 (1)
292-306:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDirectory-backed Morton outputs bypass deep verification.
Line 292 accepts both file and directory outputs, but Line 305 runs
verify_morton_parquet()only whenparquet_path.is_file(). Multipart directory outputs therefore skip morton checks and can be reported as valid on path existence alone.Suggested fix
- tiling_kind = condition.get("tiling_kind") - if tiling_kind == "morton-points" and parquet_path.is_file(): - for morton_check in verify_morton_parquet(parquet_path): + tiling_kind = condition.get("tiling_kind") + if tiling_kind == "morton-points": + candidate_paths = ( + [parquet_path] + if parquet_path.is_file() + else sorted(parquet_path.glob("part.*.parquet")) + ) + for candidate in candidate_paths: + for morton_check in verify_morton_parquet(candidate): checks.append( VerifyCheck( id=f"{condition_id}_{morton_check.id}", passed=morton_check.passed, detail=morton_check.detail, ) )🤖 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 `@python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/verify.py` around lines 292 - 306, The Morton verification logic at line 305 only executes when parquet_path is a file, causing directory-backed outputs to skip deep verification entirely. Modify the condition that checks for tiling_kind being "morton-points" to also handle directory outputs, not just files. Instead of only calling verify_morton_parquet(parquet_path) when parquet_path.is_file() is true, extend the logic to perform Morton verification for both file and directory outputs by removing or modifying the is_file() check so that the verify_morton_parquet function is invoked for directory-backed multipart outputs as well.packages/core/src/parquetWasmLoader.ts-114-119 (1)
114-119:⚠️ Potential issue | 🟠 MajorAwait CDN fallback calls so the intended catch paths actually run.
At lines 118 and 131, returning the fallback promise directly without
awaitcauses rejected CDN imports to bypass the surroundingcatchblocks. At line 131, this specifically prevents the composite error message (combining local and CDN failures) from being constructed and thrown as intended.Suggested fix
- return loadParquetModuleFromCdn(); + return await loadParquetModuleFromCdn(); @@ - return loadParquetModuleFromCdn(); + return await loadParquetModuleFromCdn();🤖 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/parquetWasmLoader.ts` around lines 114 - 119, The return statements for loadParquetModuleFromCdn() at lines 118 and 131 are not awaiting the promise, causing rejected CDN imports to bypass the surrounding catch blocks and prevent the composite error message from being constructed. Add the await keyword before both return loadParquetModuleFromCdn() calls so that promise rejections are properly caught by the try-catch block and the intended error handling logic executes.packages/core/vite.config.ts-14-19 (1)
14-19:⚠️ Potential issue | 🟠 MajorGenerate format-specific filenames for non-index entries to avoid ES/CJS collisions.
At line 18, both
esandcjsbuilds forworkersandpoints-workerentries produce the same*.jsfilename. When Vite builds with multiple formats, the CJS output overwrites the ES output, leaving only one format available at runtime.Suggested fix
fileName: (format, entryName) => { - if (entryName === 'index') { - return `index.${format === 'es' ? 'js' : 'cjs'}`; - } - return `${entryName}.js`; + const ext = format === 'es' ? 'js' : 'cjs'; + return `${entryName}.${ext}`; },🤖 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/vite.config.ts` around lines 14 - 19, In the fileName function within the vite.config.ts build configuration, the return statement for non-index entries (line 18) generates the same filename for both ES and CJS formats, causing one format to overwrite the other. Modify the return statement to conditionally append the appropriate file extension based on the format parameter, similar to how the index entry is handled - use `.js` extension when format is 'es', and use `.cjs` extension for other formats. This ensures each entry produces distinct output files for each build format.packages/core/src/pointsLoader.ts-145-153 (1)
145-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the row dimension when inferring preloaded bounds.
At Line 145 and Line 152,
shape[0]is treated as row count, but the rest of the core path uses[axisCount, rowCount](row count atshape[1]). This computes bounds from only the first 2/3 points and can produce incorrectcapabilities.bounds.💡 Suggested fix
function inferBoundsFromColumnar(preloaded: PreloadedColumnarInput) { const xs = preloaded.data[0]; const ys = preloaded.data[1]; - if (!xs || !ys || preloaded.shape[0] === 0) { + if (!xs || !ys) { return undefined; } + const count = Number.isFinite(preloaded.shape[1]) + ? preloaded.shape[1] + : Math.min(xs.length, ys.length); + if (count === 0) { + return undefined; + } let minX = Number.POSITIVE_INFINITY; let maxX = Number.NEGATIVE_INFINITY; let minY = Number.POSITIVE_INFINITY; let maxY = Number.NEGATIVE_INFINITY; - const count = preloaded.shape[0]; for (let index = 0; index < count; index += 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/pointsLoader.ts` around lines 145 - 153, The preloaded shape array uses a format of [axisCount, rowCount] where the row count is at index 1, not index 0. Update the code in the pointsLoader.ts file to use preloaded.shape[1] instead of preloaded.shape[0] in both the validation condition (the check for preloaded.shape[0] === 0) and the count variable assignment (where count = preloaded.shape[0]). This ensures the bounds calculation iterates through all points rather than only the first subset, producing correct capabilities.bounds.python/spatialdata-experimental-writer/scripts/benchmark_points_index.py-15-35 (1)
15-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnify default scenario selection for bounds and feature codes.
When no
--scenariois passed, Line 22–23 picks the first scenario bounds, but Line 29–30 drops feature filters. This can benchmark a different query than the scenario definition.💡 Suggested fix
-def _load_bounds(manifest: dict, scenario_id: str | None) -> dict[str, float]: +def _select_scenario(manifest: dict, scenario_id: str | None) -> dict: scenarios = manifest.get("benchmark_scenarios") or [] if scenario_id: for scenario in scenarios: if scenario.get("id") == scenario_id: - return scenario["bounds"] + return scenario raise SystemExit(f"Unknown scenario id: {scenario_id}") if scenarios: - return scenarios[0]["bounds"] + return scenarios[0] raise SystemExit("Manifest has no benchmark_scenarios") -def _feature_codes(manifest: dict, scenario_id: str | None) -> list[int] | None: - scenarios = manifest.get("benchmark_scenarios") or [] - if not scenario_id: - return None - for scenario in scenarios: - if scenario.get("id") == scenario_id: - codes = scenario.get("feature_codes") - return list(codes) if codes is not None else None - return None +def _load_bounds(manifest: dict, scenario_id: str | None) -> dict[str, float]: + return _select_scenario(manifest, scenario_id)["bounds"] + + +def _feature_codes(manifest: dict, scenario_id: str | None) -> list[int] | None: + scenario = _select_scenario(manifest, scenario_id) + codes = scenario.get("feature_codes") + return list(codes) if codes is not None else None🤖 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 `@python/spatialdata-experimental-writer/scripts/benchmark_points_index.py` around lines 15 - 35, The functions _load_bounds and _feature_codes handle default scenario selection inconsistently. When no scenario_id is provided, _load_bounds returns bounds from the first scenario (lines 22-23) but _feature_codes returns None (line 29-30), causing feature filters to be dropped. Fix this by modifying _feature_codes to return feature codes from the first scenario when no scenario_id is provided, matching the behavior of _load_bounds and ensuring both functions use the same default scenario for consistent benchmarking.packages/core/src/workers/points-worker.ts-387-390 (1)
387-390:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSelect the feature-code column deterministically from
featureKey.Line 387 currently picks the first column ending in
_codes. If the table has multiple coded attributes, catalog building can use the wrong code column and return incorrect mappings.Proposed fix
- const codeColumnName = table.schema.fields - .map((field) => field.name) - .find((name): name is string => typeof name === 'string' && name.endsWith('_codes')); - const codeColumn = codeColumnName ? table.getChild(codeColumnName) : null; + const codeColumnName = `${request.featureKey}_codes`; + const codeColumn = table.getChild(codeColumnName);🤖 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/workers/points-worker.ts` around lines 387 - 390, The code at lines 387-390 uses find() to select the first column ending in '_codes', which is non-deterministic when multiple code columns exist. Instead of searching for any column ending with '_codes', construct the specific column name deterministically by appending '_codes' to the featureKey variable. Replace the map and find logic with a direct construction of the expected column name based on featureKey, then retrieve that column using table.getChild() with the constructed name.packages/core/src/workers/pointsWorkerScan.ts-227-253 (1)
227-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not silently bypass feature filtering when the code column is missing.
Line 249 only enforces filtering when
featureCodeColumnis truthy. If filtering is requested but that column is absent, all rows pass, returning incorrect unfiltered data.Proposed fix
const featureCodeColumn = input.featureCodeColumnName ? input.table.getChild(input.featureCodeColumnName) : null; + if (filterByFeature && !featureCodeColumn) { + throw new Error( + `Feature filtering requested but column "${input.featureCodeColumnName}" is missing` + ); + } @@ - if ( - filterByFeature && - featureCodeColumn && - !rowMatchesFeatureCode(featureCodeColumn.get(rowIndex), allowedFeatureCodes) - ) { + if (filterByFeature && !rowMatchesFeatureCode(featureCodeColumn.get(rowIndex), allowedFeatureCodes)) { continue; }🤖 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/workers/pointsWorkerScan.ts` around lines 227 - 253, The feature code filtering check at line 246 silently bypasses all filtering when filterByFeature is true but featureCodeColumn is null, allowing unfiltered data to be returned. Add an early validation after featureCodeColumn is retrieved (similar to the existing xColumn and yColumn validation) to check that if filterByFeature is true, featureCodeColumn must exist and not be null, returning early if this condition is violated to prevent rows from being processed without the required feature code filtering.packages/core/src/workers/pointsWorkerProtocol.ts-47-56 (1)
47-56: 🛠️ Refactor suggestion | 🟠 MajorTighten payload request types to match runtime XOR behavior.
Several request variants allow both
partsandrowGroupsto be optional, but runtime checks enforce mutual exclusivity—requiring exactly one. Using discriminated unions will prevent invalid states at the type level and eliminate runtime guards.At minimum, apply this pattern to:
decodeParquetRowFeatureCodes(lines 47–56)decodeParquetGeometryCapped(lines 78–87)scanParquetFeatureCountsandscanParquetByFeatureCodes(same pattern)Proposed direction
-type DecodeParquetGeometryCappedRequest = { - parts?: Uint8Array[]; - rowGroups?: ParquetRowGroupBytesChunk[]; +type DecodeParquetGeometryCappedRequest = + | { parts: Uint8Array[]; rowGroups?: never } + | { parts?: never; rowGroups: ParquetRowGroupBytesChunk[] };Per coding guidelines: prefer types that match runtime behavior and avoid branches that can never run under the declared types.
🤖 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/workers/pointsWorkerProtocol.ts` around lines 47 - 56, Refactor the request types to use discriminated unions that enforce mutual exclusivity between `parts` and `rowGroups`. For `decodeParquetRowFeatureCodes`, `decodeParquetGeometryCapped`, `scanParquetFeatureCounts`, and `scanParquetByFeatureCodes`, create two separate type variants: one with `parts` as a required Uint8Array array (and no rowGroups property), and another with `rowGroups` as a required ParquetRowGroupBytesChunk array (and no parts property). Union these two variants together so the type system enforces that exactly one of these properties must be present, eliminating the need for runtime mutual exclusivity checks. This applies to all four request types mentioned in the comment, each following the same discriminated union pattern.Source: Coding guidelines
packages/core/tests/mortonPointsTiling.spec.ts-175-175 (1)
175-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
rm -rfvia shell with Node.jsrmfrom fs/promises.Using
execSyncwithrm -rfis unnecessary and introduces shell injection risk.🔄 Refactor to use fs/promises
afterAll(async () => { - execSync(`rm -rf ${JSON.stringify(fixtureRoot)}`, { stdio: 'pipe' }); + await rm(fixtureRoot, { recursive: true, force: true }); });Note:
rmis already imported fromnode:fs/promisesat line 2.Apply the same change at line 278 in the nested test case.
Also applies to: 278-278
🤖 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/tests/mortonPointsTiling.spec.ts` at line 175, Replace the execSync call with rm -rf at line 175 with the Node.js rm function from fs/promises which is already imported at the top of the file. Instead of using execSync to execute a shell command, directly call the rm function with the fixtureRoot path and appropriate options to remove the directory. Apply the same change at line 278 in the nested test case. This eliminates the shell injection risk and removes unnecessary shell execution overhead.Source: Linters/SAST tools
packages/core/tests/mortonPointsTiling.spec.ts-36-60 (1)
36-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
spawnorexecFileinstead ofexecSyncfor external commands.While
JSON.stringifyprovides some path escaping,execSyncinvokes a shell by default, which introduces command injection risk. Even in test code, this matters because tests run in CI and developer environments.Refactor to use
child_process.spawnorexecFilewith argument arrays to eliminate shell invocation:import { spawnSync } from 'node:child_process'; // Example refactor for the uv python call: const result = spawnSync('uv', ['run', 'python', '-c', pythonScript], { cwd: writerRoot, stdio: 'pipe', encoding: 'utf-8', env: { ...process.env, ELEMENT_DIR: elementDir } }); if (result.error) throw result.error;For the spatialdata-experimental-writer CLI:
spawnSync('uv', [ 'run', 'spatialdata-experimental-writer', 'morton-points-from-zarr', root, '--points-key', 'transcripts', '--row-group-size', '100' ], { cwd: writerRoot, stdio: 'pipe' });Also applies to: 86-112
🤖 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/tests/mortonPointsTiling.spec.ts` around lines 36 - 60, Replace both execSync calls with spawnSync from node:child_process to eliminate shell invocation risks. For the uv python command execution, pass arguments as an array to spawnSync (uv, run, python, -c, etc.) instead of a shell string with template literals, and consider passing the elementDir via environment variables. Similarly, refactor the spatialdata-experimental-writer command to use spawnSync with arguments split into an array (uv, run, spatialdata-experimental-writer, morton-points-from-zarr, root, --points-key, transcripts, etc.) rather than as a single shell command string. Ensure all path variables are passed as separate arguments rather than interpolated into command strings.Source: Linters/SAST tools
python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/index_permutations.py-165-170 (1)
165-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winManifest
sort_ordershould reflect the resolved columns actually used.Line 169 writes
condition.sort_order(placeholder values likefeature_name_codes) even when Line 155 resolves to a different column (e.g.,gene_codes). This breaks manifest/data consistency.Suggested fix
- if condition.sort_order is None: + resolved_sort_order = _condition_sort_order(condition, feature_key) + + if resolved_sort_order is None: if max_rows is not None: output_parquet.parent.mkdir(parents=True, exist_ok=True) if output_parquet.exists(): @@ else: - sort_order = _condition_sort_order(condition, feature_key) write_morton_points_parquet( df, output_parquet, feature_key=feature_key, - sort_order=sort_order, + sort_order=resolved_sort_order, row_group_size=row_group_size, compression=compression, ) @@ - "sort_order": list(condition.sort_order) if condition.sort_order else None, + "sort_order": list(resolved_sort_order) if resolved_sort_order else None,🤖 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 `@python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/index_permutations.py` around lines 165 - 170, The manifest is writing the original placeholder values from condition.sort_order (like feature_name_codes) instead of the actually resolved column names (like gene_codes) that are determined earlier in the code. Identify where the actual column resolution happens (around line 155) and capture the resolved column names. Then when appending to manifest_conditions, replace the condition.sort_order reference with the resolved column names to ensure the manifest accurately reflects the actual columns being used in the data.python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.py-122-127 (1)
122-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not mask internal TUI import failures as dependency-missing errors.
Line 124 catches every
ImportError, so unrelated import bugs insidespatialdata_experimental_writer.tui.appget misreported as “install TUI deps”. Re-raise non-Textual import failures.Suggested fix
def _tui(args: argparse.Namespace) -> None: try: from .tui.app import run_tui - except ImportError as exc: - raise SystemExit( - "TUI dependencies are not installed. Run: uv sync --group tui" - ) from exc + except ModuleNotFoundError as exc: + if exc.name and exc.name.startswith("textual"): + raise SystemExit( + "TUI dependencies are not installed. Run: uv sync --group tui" + ) from exc + raise run_tui(initial_zarr=args.zarr)🤖 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 `@python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.py` around lines 122 - 127, The except ImportError block in the run_tui import catches all ImportError exceptions, including those from internal failures within the .tui.app module itself, and reports them all as missing TUI dependencies. Modify the exception handler to differentiate between ImportError caused by .tui.app not being found versus ImportError from within that module. Check the error's name attribute or message to determine if it's a missing module error (e.g., name equals 'spatialdata_experimental_writer.tui.app' or similar) and only report the "TUI dependencies not installed" message for that case, while re-raising all other ImportError exceptions to expose the actual internal import failures.python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/index_permutations.py-132-163 (1)
132-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard feature-dependent conditions before writing permutations.
With
DEFAULT_CONDITIONS, feature-based variants are always selected. Iffeature_keyis missing (or not present in the dataframe), Line 156→163 eventually sorts by a non-existent*_codescolumn and fails at runtime.Suggested fix
selected = tuple(conditions or DEFAULT_CONDITIONS) manifest_conditions: list[dict[str, Any]] = [] + + needs_feature_codes = any( + condition.sort_order and "feature_name_codes" in condition.sort_order + for condition in selected + ) + if needs_feature_codes and (not feature_key or feature_key not in df.columns): + raise ValueError( + "Selected conditions require a valid feature_key column, but it is missing." + ) for condition in selected:🤖 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 `@python/spatialdata-experimental-writer/src/spatialdata_experimental_writer/index_permutations.py` around lines 132 - 163, The code currently processes all selected conditions including feature-dependent ones regardless of whether the feature_key exists in the dataframe. Before the loop over selected conditions, add a guard check to verify that feature_key is present in the dataframe (df). For conditions where condition.sort_order is not None (the else block at line 156 that calls write_morton_points_parquet), either skip these feature-dependent conditions when feature_key is missing from df, or add a conditional check within that else block to only call write_morton_points_parquet when the feature_key exists. This prevents the code from attempting to sort by a non-existent *_codes column derived from a missing feature_key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 367dc296-23c7-4d56-8a0e-4a9a00e8443f
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpython/spatialdata-experimental-writer/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (92)
.gitignoreCONTEXT.mddocs/adr/0002-spatially-aware-vector-loading.mddocs/adr/0003-points-render-resource.mddocs/plans/points-preload-feature-filter-status.mdpackages/core/package.jsonpackages/core/src/index.tspackages/core/src/models/VPointsSource.tspackages/core/src/models/VShapesSource.tspackages/core/src/models/VTableSource.tspackages/core/src/models/index.tspackages/core/src/parquetWasmLoader.tspackages/core/src/pointsFeatures.tspackages/core/src/pointsLimits.tspackages/core/src/pointsLoadOptions.tspackages/core/src/pointsLoader.tspackages/core/src/pointsTiling.tspackages/core/src/spatialViewFit.tspackages/core/src/workers/index.tspackages/core/src/workers/points-worker.tspackages/core/src/workers/pointsWorkerClient.tspackages/core/src/workers/pointsWorkerProtocol.tspackages/core/src/workers/pointsWorkerScan.tspackages/core/tests/mortonPointsTiling.spec.tspackages/core/tests/pointsFeatures.spec.tspackages/core/tests/pointsLoader.spec.tspackages/core/tests/pointsPreloadGuard.spec.tspackages/core/tests/pointsPreloadReadStrategy.spec.tspackages/core/tests/pointsTiling.spec.tspackages/core/tests/pointsWorker.spec.tspackages/core/tests/pointsWorkerScan.spec.tspackages/core/tests/vtableMultipart.spec.tspackages/core/tsconfig.jsonpackages/core/vite.config.tspackages/layers/package.jsonpackages/layers/src/PointsLayer.tspackages/layers/src/geoArrowStrategies.tspackages/layers/src/index.tspackages/layers/src/mortonTiledStrategy.tspackages/layers/src/pointsBbox.tspackages/layers/src/pointsFeatureCodes.tspackages/layers/src/pointsLoader.tspackages/layers/src/pointsLoaderAdapter.tspackages/layers/src/pointsRenderStrategies.tspackages/layers/src/pointsScatterLayer.tspackages/layers/src/pointsTileDebug.tspackages/layers/src/pointsTileLoadCallbacks.tspackages/layers/src/pointsTiledDebugHooks.tspackages/layers/src/preloadedScatterStrategy.tspackages/layers/tests/pointsLayerFilter.spec.tspackages/layers/tests/pointsRenderStrategies.spec.tspackages/layers/tests/pointsTileDebug.spec.tspackages/layers/vite.config.tspackages/vis/demo/src/enableDemoPointsWorker.tspackages/vis/demo/src/main.tsxpackages/vis/src/SpatialCanvas/PointsFeatureFilterPanel.tsxpackages/vis/src/SpatialCanvas/PointsStylePanel.tsxpackages/vis/src/SpatialCanvas/ShapesStylePanel.tsxpackages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsxpackages/vis/src/SpatialCanvas/geometryLoadStats.tsxpackages/vis/src/SpatialCanvas/index.tsxpackages/vis/src/SpatialCanvas/pointsLoadPlan.tspackages/vis/src/SpatialCanvas/pointsTileProgress.tspackages/vis/src/SpatialCanvas/renderers/pointsRenderer.tspackages/vis/src/SpatialCanvas/resolvePointsRenderResource.tspackages/vis/src/SpatialCanvas/types.tspackages/vis/src/SpatialCanvas/useLayerData.tspackages/vis/tests/formatLoadDuration.spec.tspackages/vis/tests/pointsLoadPlan.spec.tspackages/vis/tests/pointsRenderer.spec.tspackages/vis/tests/pointsTileProgress.spec.tspackages/vis/tests/resolvePointsRenderResource.spec.tspackages/vis/tests/shapesStylePanel.spec.tspython/spatialdata-experimental-writer/README.mdpython/spatialdata-experimental-writer/pyproject.tomlpython/spatialdata-experimental-writer/scripts/benchmark_points_index.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/__init__.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/cli.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/errors.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/index_permutations.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/points.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/runners.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/tui/__init__.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/tui/app.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/tui/models.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/tui/screens.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/verify.pypython/spatialdata-experimental-writer/src/spatialdata_experimental_writer/zarr.pypython/spatialdata-experimental-writer/tests/test_integration.pypython/spatialdata-experimental-writer/tests/test_points.pypython/spatialdata-experimental-writer/tests/test_verify.pypython/spatialdata-experimental-writer/tests/test_zarr.py
Comment on lines
+163
to
+166
| stack.push([(prefix << 2) | 0, level + 1, xmin, ymin, midx, midy]); | ||
| stack.push([(prefix << 2) | 1, level + 1, midx + 1, ymin, xmax, midy]); | ||
| stack.push([(prefix << 2) | 2, level + 1, xmin, midy + 1, midx, ymax]); | ||
| stack.push([(prefix << 2) | 3, level + 1, midx + 1, midy + 1, xmax, ymax]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import ctypes
prefix = (1 << 30) - 1 # reachable near final expansion with 16 bits/axis
js_like_shift = ctypes.c_int32(prefix << 2).value
arith = prefix * 4
print("js_like_shift:", js_like_shift)
print("arith:", arith)
if js_like_shift >= 0:
raise SystemExit("Expected signed int32 wrap to negative value")
PY
rg -n "prefix << 2" packages/core/src/pointsTiling.tsRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 431
🏁 Script executed:
cat -n packages/core/src/pointsTiling.ts | sed -n '120,180p'Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 2491
🏁 Script executed:
rg -n "MORTON_CODE_BITS_PER_AXIS" packages/core/src/pointsTiling.ts | head -5Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 236
🏁 Script executed:
rg -n "function cellRange|const cellRange" packages/core/src/pointsTiling.ts -A 5Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 303
🏁 Script executed:
rg -n "prefix" packages/core/src/pointsTiling.ts | grep -E "(Array|type|interface)" | head -10Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 57
🏁 Script executed:
cat -n packages/core/src/pointsTiling.ts | sed -n '142,152p'Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 485
🏁 Script executed:
rg -n "prefix" packages/core/src/pointsTiling.ts | grep -E "(<<|>>|&|\|)"Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 395
Replace bitwise prefix expansion with arithmetic to avoid int32 overflow.
At lines 163–166, (prefix << 2) | quadrant forces signed 32-bit math. With bits = 16, recursion reaches level 15 where prefix has 30 bits; shifting left by 2 overflows and wraps to negative. This corrupts cellRange(), which returns invalid negative Morton interval bounds.
Suggested fix: replace with arithmetic (prefix * 4 + quadrant), which avoids bitwise overflow and is mathematically equivalent.
Suggested fix
- stack.push([(prefix << 2) | 0, level + 1, xmin, ymin, midx, midy]);
- stack.push([(prefix << 2) | 1, level + 1, midx + 1, ymin, xmax, midy]);
- stack.push([(prefix << 2) | 2, level + 1, xmin, midy + 1, midx, ymax]);
- stack.push([(prefix << 2) | 3, level + 1, midx + 1, midy + 1, xmax, ymax]);
+ const nextPrefix = prefix * 4;
+ stack.push([nextPrefix + 0, level + 1, xmin, ymin, midx, midy]);
+ stack.push([nextPrefix + 1, level + 1, midx + 1, ymin, xmax, midy]);
+ stack.push([nextPrefix + 2, level + 1, xmin, midy + 1, midx, ymax]);
+ stack.push([nextPrefix + 3, level + 1, midx + 1, midy + 1, xmax, ymax]);🤖 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/pointsTiling.ts` around lines 163 - 166, The four
consecutive stack.push calls that compute the prefix using bitwise operations
(prefix << 2) | quadrant for quadrant values 0, 1, 2, and 3 are causing signed
32-bit integer overflow at deep recursion levels. Replace the bitwise prefix
expansion (prefix << 2) | quadrant with the arithmetic equivalent prefix * 4 +
quadrant in all four stack.push calls to avoid the overflow while maintaining
the same mathematical result.
- Introduced `pointsTiling.ts` to handle Morton code-based tiling for point data. - Updated `index.ts` to export the new points tiling module. - Enhanced `VPointsSource` and `VShapesSource` to support loading points tiling metadata and filtering points within specified bounds. - Modified `PointsColumnarData` type to use `ArrayLike<number>[]` for better compatibility. - Added tests for points tiling functions to ensure correctness. - Updated relevant components in the visualization layer to utilize the new points tiling features.
- Added `pyproject.toml` for package configuration, including dependencies and build system. - Created `README.md` with an overview of the experimental vector optimization writers for SpatialData. - Implemented core functionality in `src/spatialdata_experimental_writer`, including Morton sorting and multiscale Parquet writing. - Developed command-line interface in `cli.py` for various operations on SpatialData. - Introduced Zarr integration for reading and writing points data. - Added tests for key functionalities in `tests/test_points.py` and `tests/test_zarr.py`. - Established package structure with necessary modules and scripts.
…egration - Added `featureCodes` option to `PointsInBoundsOptions` for filtering points based on feature codes. - Introduced utility functions `featureCodeAllowSet` and `rowMatchesFeatureCode` for feature code management. - Updated `filterPointsToBounds` to incorporate feature code filtering logic. - Enhanced `VPointsSource` to support loading points with optional feature codes and improved row group reading. - Implemented tests for new functionality in `mortonPointsTiling.spec.ts` and `pointsTiling.spec.ts`. - Added `PointsStylePanel` and loading message handling in the visualization layer for better user feedback during point loading.
- Updated README.md to include detailed installation instructions and command usage for the experimental writer. - Added a new script for benchmarking points index permutations, allowing users to evaluate performance based on index-manifest.json. - Implemented a new CLI command for writing derivative Zarr stores with transcript index sort permutations. - Introduced consolidated metadata registration for points elements in Zarr stores. - Enhanced morton sorting functionality to support custom sort orders and ensure uint columns are not persisted in output Parquet files. - Added integration tests to validate new features and ensure correct functionality across the package.
…nd add loading debug info - Added detailed documentation for the Points Render Resource, including its structure and usage. - Introduced `PointsLoader` capabilities and encoding strategies for handling point data. - Implemented `createMortonTiledPointsLoader` and `createPreloadedColumnarPointsLoader` functions for efficient data loading. - Developed a new `PointsLayer` class to manage rendering strategies and integrate with deck.gl. - Added support for tile debugging and enhanced point visualization features. - Created tests for the new points loader functionality to ensure reliability and performance. - Updated package exports to include new points-related modules and types.
- Introduced `columnarPointCount` function to accurately determine point counts from columnar data. - Updated `toColumnarBatch` to utilize the new point count logic. - Added `createTileDebugStore` and `createTiledPointsDebugHooks` for improved tile debugging capabilities. - Enhanced `PointsLayer` to support tile debugging and updated state management. - Modified `pointsTileProgress` to track loaded points and improve loading messages. - Updated tests to cover new debugging features and ensure correct functionality across layers. - Adjusted visualization components to integrate tile debugging options seamlessly.
Add points load planning functionality and integrate with layer data management - Introduced `pointsLoadPlan.ts` to define interfaces and functions for scheduling point loads based on optimization and metadata status. - Integrated `planPointsLoads` and `shouldPreloadAfterMetadataProbe` into `useLayerData` for improved loading logic. - Updated state management to handle loading conditions more effectively. - Added unit tests for the new loading plan functions to ensure correct behavior and coverage.
- Integrated preloaded point count and loading duration into the PointsStylePanel for improved user feedback. - Updated SpatialCanvasInner to retrieve and display loading state and preloaded point count for points layers. - Added formatLoadDurationMs function to format loading durations in a user-friendly manner. - Introduced unit tests for formatLoadDurationMs to ensure accurate duration formatting. - Enhanced useLayerData to manage loading states and durations for points layers effectively.
…s Style Panels - Introduced `GeometryLoadStats` component to display loading state and duration for geometry layers. - Updated `PointsStylePanel` to utilize `GeometryLoadStats` for improved loading feedback. - Created `ShapesStylePanel` to manage shapes layer styling and integrate geometry loading statistics. - Enhanced `useLayerData` to support retrieval of shapes layer loading summaries. - Added unit tests for `formatShapesGeometryKindLabel` to ensure correct mapping of geometry kinds.
- Added new `pointsLimits.ts` and `pointsFeatures.ts` modules to manage point preload limits and feature cataloging. - Implemented `filterColumnarByFeatureCodes` function for efficient filtering of point data based on feature codes. - Integrated worker support for offloading heavy computations related to point data processing. - Updated `VPointsSource` to include methods for listing features and retrieving parquet row counts. - Enhanced `points-worker.ts` to handle requests for filtering and building feature catalogs in a web worker context. - Added unit tests for new functionalities to ensure reliability and performance across point data operations.
…ows past initial limit - Updated `pointsFeatures.ts` to introduce `resolveRowFeatureCodesFromTable` for deriving row feature codes from tables and added `dictionaryIndexArray` for handling dictionary-encoded columns. - Modified `VPointsSource` to include `loadPointsRowFeatureCodes` for loading feature codes aligned with point data. - Enhanced `PointsElement` class to support loading row feature codes. - Improved `useLayerData` to manage loading of row feature codes and integrated it with existing layer data management. - Added unit tests to validate new functionalities and ensure correct behavior in feature code loading and filtering.
…refactor and type review
- Updated `pointsFeatures.ts` to introduce `featureCodeMapFromCatalog` for mapping feature names to codes, improving row feature code resolution. - Modified `VPointsSource` to utilize the new feature code mapping and delegate row feature code decoding to the points worker when enabled, enhancing performance for large datasets. - Refactored `loadPointsRowFeatureCodes` to support both direct feature code columns and dictionary-based feature codes, ensuring accurate code extraction. - Enhanced worker communication to handle row group data efficiently, allowing for better scalability in processing point data. - Added unit tests to validate new functionalities and ensure correct behavior in feature code loading and decoding.
- Updated `VPointsSource` to enable geometry preloading and feature catalog scanning in the points worker, improving performance for large datasets. - Introduced new methods for decoding parquet geometry and scanning feature catalogs directly in the worker, allowing for efficient data processing. - Refactored worker communication to handle both row groups and parts, ensuring flexibility in data handling. - Enhanced error handling for worker operations, providing fallbacks to the main thread when necessary. - Added unit tests to validate the new worker functionalities and ensure correct behavior in feature catalog scanning and geometry decoding.
- Added a new Textual User Interface (TUI) for an interactive workflow, allowing users to execute commands in a guided manner. - Updated `pyproject.toml` to include `textual` as a dependency for TUI functionality. - Update the README with instructions for using the TUI, including command execution and verification checks. - Refactored CLI commands to utilize new runner functions for improved organization and maintainability. - Introduced verification checks for Morton and multiscale parquet outputs to ensure data integrity. - Added unit tests for verification functions to validate their correctness and reliability.
Add InputFormScreen and refactor Existing Screens - Introduced InputFormScreen as a base class for form handling, enabling consistent input navigation and submission across multiple screens. - Refactored ZarrPathScreen, MortonFromZarrScreen, MortonFileScreen, MultiscaleScreen, and IndexPermutationsScreen to inherit from InputFormScreen, streamlining input management. - Implemented action handling for back navigation and primary button submission in the new InputFormScreen. - Enhanced user experience by ensuring focus transitions between input fields based on user input submission.
PointsLayer and VPointsSource support feature catalog in loadRowFeatureCodes; add utility functions for preloaded feature code handling and update related tests.
xinaesthete
force-pushed
the
codex/spatial-parquet
branch
from
June 22, 2026 13:52
0ca7dc3 to
fea53f9
Compare
- Introduced a new page for Browser Workers detailing their setup and benefits for offloading CPU-heavy tasks in browser applications. - Updated the Core Package Overview to include instructions for enabling the points worker at startup. - Enhanced existing documentation to reference the new Browser Workers guide for Vite integration and codec handling. - Adjusted various files to ensure consistency in referencing the new worker setup.
- Added `ensurePointsWorker` call to ensure the points worker is initialized. - Improved error handling in `loadPointsTilingMetadata` to clear cache on failure. - Added additional abort checks in `loadPointsWithOptionalFeatureCodes`. - Updated return values to return `null` instead of empty structures when no data is available. - Refactored file reading logic in tests to handle directory checks and improve error handling.
…Result to PointsInBoundsResponse
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.
Summary
Work-in-progress improvements to points rendering, as well as python scripts / TUI for re-writing points with new indexing (more options anticipated in future, as well as merging with codec writer for making a more general utility).
The scope has crept way too far here for a single PR, there are some things that are still fundamentally not working as they should, and also some other improvements that could have been isolated (including some general minor tweaks like making the demo UI default to single available CS).
Before merging (not going to squash this one considering huge diff), I should check that there are no actual significant regressions or blockers and verify that the current status and anticipated future-work is reasonably well understood and documented.