Fix docs demo React externalization and local dev workflow - #25
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDev/server scripts and Vite configs now bind dev servers to 127.0.0.1 and stricter ports; vis adds a coordinated dev script that spawns a build watcher and demo dev server. Library externals expanded to regexes. avivatorish adds raster-source resolution helpers and updates callers to use them. (45 words) Changes
Sequence DiagramsequenceDiagram
participant Parent as dev.mjs (Parent Process)
participant ViteBuild as Vite Build (watch)
participant ViteDemo as Vite Demo (dev server)
participant System as System Signals (SIGINT/SIGTERM)
Parent->>ViteBuild: spawn "pnpm vite build --watch"
Parent->>ViteDemo: spawn "pnpm vite --config vite.config.demo.ts --host 127.0.0.1 --port 5173 --strictPort"
ViteBuild-->>Parent: 'error' / 'exit' (non-zero)
ViteDemo-->>Parent: 'error' / 'exit' (non-zero)
Note over Parent: On first failure or non-successful exit\nset shuttingDown = true, kill children
alt System signal received
System->>Parent: SIGINT / SIGTERM
Parent->>Parent: map to exit code (130 / 143), set shuttingDown = true
Parent->>ViteBuild: kill()
Parent->>ViteDemo: kill()
end
Parent->>Parent: wait 500ms
Parent->>System: process.exit(code)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/avivatorish/src/hooks.ts (2)
56-57: Consider consolidating state updates.Lines 56-57 make two separate
viewerStore.setStatecalls in sequence. These could be merged into a single call for clarity and minor performance benefit:♻️ Proposed consolidation
- viewerStore.setState({ isChannelLoading: [true] }); - viewerStore.setState({ isViewerLoading: true, metadata: null }); + viewerStore.setState({ isChannelLoading: [true], isViewerLoading: true, metadata: null });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/avivatorish/src/hooks.ts` around lines 56 - 57, The two sequential state updates should be consolidated into one setState call: replace the separate viewerStore.setState({ isChannelLoading: [true] }) and viewerStore.setState({ isViewerLoading: true, metadata: null }) with a single viewerStore.setState that sets isChannelLoading, isViewerLoading, and metadata together (i.e., viewerStore.setState({ isChannelLoading: [true], isViewerLoading: true, metadata: null })) to avoid redundant updates and potential extra re-renders.
113-116: Silent early return may hide loader resolution failures.When
resolveRasterSourcereturnsundefined, the function silently exits without updating the loading state or notifying the user. The viewer may remain in a stale or loading state indefinitely.Consider logging a warning or setting an error state:
🔊 Proposed fix to add warning
const rasterSource = resolveRasterSource(loader); if (!rasterSource) { + console.warn("Unable to resolve raster source from loader; skipping settings update"); + viewerStore.setState({ isViewerLoading: false }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/avivatorish/src/hooks.ts` around lines 113 - 116, The early return after calling resolveRasterSource(loader) silently hides failures; instead of just returning, update the component/viewer state and surface a warning: when rasterSource is falsy, call the appropriate state setters (e.g., setLoading(false) and setError(...) or dispatch an error action) and emit a warning (e.g., processLogger.warn or console.warn) mentioning the loader and that resolveRasterSource failed so consumers aren’t left in a stale loading state; update the block that currently reads "const rasterSource = resolveRasterSource(loader); if (!rasterSource) { return; }" to perform these logging/state updates before returning.packages/avivatorish/src/utils.ts (1)
408-416: Type guard does not verifylabelsandshapeare arrays.
isRasterSourceLikechecks thatlabelsandshapeproperties exist, but doesn't verify they are arrays. If a malformed loader haslabels: "foo"orshape: 42, downstream code callinglabels.lengthor iterating overshapecould fail unexpectedly.Consider strengthening the guard:
🛡️ Proposed fix to validate array types
export function isRasterSourceLike(value: unknown): value is RasterSourceLike { if (!value || typeof value !== "object") return false; - return "getRaster" in value && typeof value.getRaster === "function" && "labels" in value && "shape" in value; + return ( + "getRaster" in value && + typeof value.getRaster === "function" && + "labels" in value && + Array.isArray(value.labels) && + "shape" in value && + Array.isArray(value.shape) + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/avivatorish/src/utils.ts` around lines 408 - 416, The type guard isRasterSourceLike currently only checks existence of labels and shape but not that they are arrays; update isRasterSourceLike to additionally verify Array.isArray(value.labels) and Array.isArray(value.shape) and also that value.labels.every(item => typeof item === "string") and value.shape.every(item => typeof item === "number") while retaining the existing check that value.getRaster is a function (reference symbols: isRasterSourceLike, RasterSourceLike, getRaster, labels, shape) so downstream code can safely use labels.length and iterate shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/avivatorish/src/utils.ts`:
- Around line 580-582: getBoundingCube calls getPhysicalSizeScalingMatrix with
`source` (a RasterSourceLike) which lacks `meta.physicalSizes`, so scaling falls
back to identity; change the argument to the original `loader` (PixelSource) so
getPhysicalSizeScalingMatrix can read `meta.physicalSizes` and return the
correct scaling matrix — update the call in getBoundingCube from using `source`
to using `loader`.
---
Nitpick comments:
In `@packages/avivatorish/src/hooks.ts`:
- Around line 56-57: The two sequential state updates should be consolidated
into one setState call: replace the separate viewerStore.setState({
isChannelLoading: [true] }) and viewerStore.setState({ isViewerLoading: true,
metadata: null }) with a single viewerStore.setState that sets isChannelLoading,
isViewerLoading, and metadata together (i.e., viewerStore.setState({
isChannelLoading: [true], isViewerLoading: true, metadata: null })) to avoid
redundant updates and potential extra re-renders.
- Around line 113-116: The early return after calling
resolveRasterSource(loader) silently hides failures; instead of just returning,
update the component/viewer state and surface a warning: when rasterSource is
falsy, call the appropriate state setters (e.g., setLoading(false) and
setError(...) or dispatch an error action) and emit a warning (e.g.,
processLogger.warn or console.warn) mentioning the loader and that
resolveRasterSource failed so consumers aren’t left in a stale loading state;
update the block that currently reads "const rasterSource =
resolveRasterSource(loader); if (!rasterSource) { return; }" to perform these
logging/state updates before returning.
In `@packages/avivatorish/src/utils.ts`:
- Around line 408-416: The type guard isRasterSourceLike currently only checks
existence of labels and shape but not that they are arrays; update
isRasterSourceLike to additionally verify Array.isArray(value.labels) and
Array.isArray(value.shape) and also that value.labels.every(item => typeof item
=== "string") and value.shape.every(item => typeof item === "number") while
retaining the existing check that value.getRaster is a function (reference
symbols: isRasterSourceLike, RasterSourceLike, getRaster, labels, shape) so
downstream code can safely use labels.length and iterate shape.
🪄 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: d6c3ec21-40b4-4169-9f22-13b2f59f4150
📒 Files selected for processing (4)
docs/docusaurus.config.tspackages/avivatorish/src/hooks.tspackages/avivatorish/src/utils.tspackages/vis/src/ImageView/index.tsx
This was referenced Jun 29, 2026
This was referenced Jul 31, 2026
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
react/jsx-runtime,react/jsx-dev-runtime, andzustandsubpaths to avoid the demo bundle requiring React at runtime@spatialdata/visexternals to keep workspace package boundaries intact in docs and CI buildsvisrebuildsdistwhile the demo server runsTesting
pnpm buildpnpm --filter docs buildpnpm devSummary by CodeRabbit
Chores
Refactor
Bug Fixes