Option to show scatterplot density layers as grid rather than layers on same plot#459
Open
xinaesthete wants to merge 16 commits into
Open
Option to show scatterplot density layers as grid rather than layers on same plot#459xinaesthete wants to merge 16 commits into
xinaesthete wants to merge 16 commits into
Conversation
✅ Deploy Preview for mdv-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a density-grid small-multiple rendering mode for scatter charts with chart-array layout, view-id/view-state routing, contour-state numeric-field refactor and a density_mode toggle, DeckGL/Viv grid renderers, layout/measurement/visibility hooks, layer viewport-scope tagging, and comprehensive unit and Playwright tests. ChangesDensity Grid View Feature
Estimated code review effort: Possibly related PRs
Suggested reviewers:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/react/contour_state.ts (1)
562-645:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBroaden the density-field picker to match the new numeric-field support.
isLoadedNumericContourField()now acceptsintegerandint32, but theDensity Fieldscontrol in this settings block still filters tocolumnType: "double"on Line 615. That means the new numeric-field support is not actually reachable from the UI.Suggested fix
g({ type: "multicolumn", label: "Density Fields", //@ts-expect-error - pending optional columns current_value: c.densityFields, - columnType: "double", + columnType: ["double", "integer", "int32"], func: (x) => { c.densityFields = x; }, }),🤖 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 `@src/react/contour_state.ts` around lines 562 - 645, The Density Fields control in getDensitySettings limits selectable fields to columnType "double", preventing integer/int32 numeric fields from being picked; update the Density Fields gui spec (the g call labeled "Density Fields" in getDensitySettings) to accept the broader numeric types used by isLoadedNumericContourField (e.g. change columnType from the single "double" to a more permissive type or list such as "number" or ["double","integer","int32"] or remove the restriction so integer/int32 fields become selectable).
🤖 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 `@src/react/components/DeckScatterComponent.tsx`:
- Around line 42-50: paramKey currently only includes cx.field and cy.field so
changes to the z-axis (cz.field) won't trigger the recenter logic; update the
key construction used by useEffect (paramKey and lastParamKeyRef comparisons) to
include cz.field (e.g., `${cx.field}\u0000${cy.field}\u0000${cz.field}`) so that
the effect sees z-axis changes and forces recenter when cz changes, preserving
the existing checks for pendingRecenter, config.zoom_on_filter, and
chart.ignoreStateUpdate.
---
Outside diff comments:
In `@src/react/contour_state.ts`:
- Around line 562-645: The Density Fields control in getDensitySettings limits
selectable fields to columnType "double", preventing integer/int32 numeric
fields from being picked; update the Density Fields gui spec (the g call labeled
"Density Fields" in getDensitySettings) to accept the broader numeric types used
by isLoadedNumericContourField (e.g. change columnType from the single "double"
to a more permissive type or list such as "number" or
["double","integer","int32"] or remove the restriction so integer/int32 fields
become selectable).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa4ab950-be73-4aed-8a00-20dfa062f0e3
📒 Files selected for processing (21)
src/charts/css/charts.csssrc/charts/schemas/ChartConfigSchema.tssrc/react/components/ChartArrayLayout.tsxsrc/react/components/DeckDensityGridComponent.tsxsrc/react/components/DeckScatterComponent.tsxsrc/react/components/DeckScatterReactWrapper.tsxsrc/react/components/SelectionOverlay.tsxsrc/react/components/chartArrayUtils.test.tssrc/react/components/chartArrayUtils.tssrc/react/components/densityGridUtils.test.tssrc/react/components/densityGridUtils.tssrc/react/components/sharedScatterSettings.tssrc/react/contour_state.test.tssrc/react/contour_state.tssrc/react/hooks/useChartArrayMetrics.tssrc/react/hooks/useChartArrayVisibleIndices.tssrc/react/scatter_state.tssrc/tests/chartConfigSchema.spec.tssrc/webgl/SpatialLayer.tstests_playwright/project/charts/density_grid_selection.spec.tstests_playwright/utils/helpers.ts
Comment on lines
+42
to
+50
| const paramKey = `${cx.field}\u0000${cy.field}`; | ||
| const lastParamKeyRef = useRef(""); | ||
| useEffect(() => { | ||
| if (!enabled) return; | ||
| if (chart.ignoreStateUpdate) return; | ||
| if (data.length === 0) return; // [0, 0, 1, 1]; | ||
| if (!pendingRecenter && !config.zoom_on_filter) return; | ||
| const paramChanged = lastParamKeyRef.current !== paramKey; | ||
| lastParamKeyRef.current = paramKey; | ||
| if (!pendingRecenter && !config.zoom_on_filter && !paramChanged) return; |
Contributor
There was a problem hiding this comment.
Include the z-axis in the recenter key for 3D charts.
paramKey only tracks cx and cy, but this effect also recomputes the 3D viewState.target from cz. After this change, switching only the z-axis can skip the recenter path entirely, so 3D plots stay centered on the previous z column.
Suggested fix
- const paramKey = `${cx.field}\u0000${cy.field}`;
+ const paramKey =
+ config.dimension === "3d" && cz
+ ? `${cx.field}\u0000${cy.field}\u0000${cz.field}`
+ : `${cx.field}\u0000${cy.field}`;🤖 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 `@src/react/components/DeckScatterComponent.tsx` around lines 42 - 50, paramKey
currently only includes cx.field and cy.field so changes to the z-axis
(cz.field) won't trigger the recenter logic; update the key construction used by
useEffect (paramKey and lastParamKeyRef comparisons) to include cz.field (e.g.,
`${cx.field}\u0000${cy.field}\u0000${cz.field}`) so that the effect sees z-axis
changes and forces recenter when cz changes, preserving the existing checks for
pendingRecenter, config.zoom_on_filter, and chart.ignoreStateUpdate.
Comment on lines
+217
to
+235
| useEffect(() => { | ||
| if (!isDensityGrid) return; | ||
| if (editingGateId) { | ||
| onCancelEditGate(); | ||
| } | ||
| if (selectedTool !== "Pan") { | ||
| setSelectedTool("Pan"); | ||
| } | ||
| }, [isDensityGrid, editingGateId, onCancelEditGate, selectedTool, setSelectedTool]); | ||
|
|
||
| const toggleDensityGrid = useCallback(() => { | ||
| action(() => { | ||
| const nextIsGrid = contourConfig.density_mode !== "grid"; | ||
| contourConfig.density_mode = nextIsGrid ? "grid" : "overlay"; | ||
| if (nextIsGrid) { | ||
| contourConfig.contour_fill = true; | ||
| } | ||
| })(); | ||
| }, [contourConfig]); |
Contributor
There was a problem hiding this comment.
Dismiss gate dialogs when switching into grid mode.
This effect cancels in-progress edits, but it leaves GateNameDialog and any already-open ManageGateDialogWrapper interactive after the UI switches to the advertised "view only" grid mode.
Suggested fix
useEffect(() => {
if (!isDensityGrid) return;
+ setGateDialogOpen(false);
+ const manageDialog = manageGateByChartRef.current.get(chartId);
+ manageDialog?.close();
+ manageGateByChartRef.current.delete(chartId);
if (editingGateId) {
onCancelEditGate();
}
if (selectedTool !== "Pan") {
setSelectedTool("Pan");
}
- }, [isDensityGrid, editingGateId, onCancelEditGate, selectedTool, setSelectedTool]);
+ }, [chartId, isDensityGrid, editingGateId, onCancelEditGate, selectedTool, setSelectedTool]);
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/react/components/ChartArrayLayout.tsx (1)
81-86: ⚡ Quick winAvoid the
as stringcast for CSS custom property assignment.You can keep this type-safe without casting by defining a local style type that includes
"--mdv-chart-array-columns".Suggested change
+type ChartArrayStyle = CSSProperties & { + "--mdv-chart-array-columns"?: string; +}; ... - style={ - layoutMode === "grid" - ? { - ...style, - ["--mdv-chart-array-columns" as string]: String(gridColumns), - } - : style - } + style={ + layoutMode === "grid" + ? ({ + ...style, + "--mdv-chart-array-columns": String(gridColumns), + } satisfies ChartArrayStyle) + : style + }As per coding guidelines: "
**/*.{ts,tsx}: Avoidascasts where possible in TypeScript".🤖 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 `@src/react/components/ChartArrayLayout.tsx` around lines 81 - 86, The code uses an "as string" cast when setting the CSS custom property in ChartArrayLayout.tsx; replace that cast by creating a local typed style object (e.g., define an interface extending React.CSSProperties with "--mdv-chart-array-columns"?: string) and use that type for the object you spread/return so you can assign ["--mdv-chart-array-columns"]: String(gridColumns) without casting; update the branch where layoutMode === "grid" (and the variable style / gridColumns) to build/return this strongly-typed style object instead of using the inline "as string" cast.src/react/components/VivDensityGridComponent.tsx (1)
26-26: ⚡ Quick winReplace the
as OME_TIFF["data"]assertion with a typed narrow/helper.This cast bypasses compile-time guarantees and conflicts with the TS guideline for this repo.
As per coding guidelines: "
**/*.{ts,tsx}: Avoidascasts where possible in TypeScript".🤖 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 `@src/react/components/VivDensityGridComponent.tsx` at line 26, Replace the unsafe "as OME_TIFF[\"data\"]" cast on the value returned from useLoader() by either calling useLoader with an explicit generic (e.g. useLoader<OME_TIFF[\"data\"]>()) or by performing a local type-narrow using a type guard helper (create an isOmeTiffData(value): value is OME_TIFF[\"data\"] and check it before assigning to ome); update the variable assignment currently named ome and any downstream consumers to use the narrowed/typed value so you keep compile-time guarantees without using an "as" assertion.src/react/hooks/useVivDensityGridViewState.ts (1)
18-18: ⚡ Quick winPrefer updating the chart type over using a type assertion.
The
ascast here adds apendingRecenterproperty to the chart type. As per coding guidelines,ascasts should be avoided where possible. Consider addingpendingRecenter?: booleanto the chart type definition (or using a more specific hook/type) to eliminate the need for this assertion and improve type safety.🤖 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 `@src/react/hooks/useVivDensityGridViewState.ts` at line 18, The code currently uses a type assertion on the result of useChart() to add a pendingRecenter?: boolean property; remove the unsafe "as" cast by extending the chart type or creating a more specific hook/type. Update the chart type/interface used by useChart (or create a new hook like useChartWithPendingRecenter) to include pendingRecenter?: boolean, then replace the assertion in useVivDensityGridViewState with the correctly typed return value from useChart (or the new hook) so pendingRecenter is available without "as".src/react/hooks/useDensityGridCells.ts (1)
130-130: ⚡ Quick winPrefer updating the chart type over using a type assertion.
The
ascast here adds apendingRecenterproperty to the chart type. As per coding guidelines,ascasts should be avoided where possible. Consider addingpendingRecenter?: booleanto the chart type definition (or using a more specific hook/type) to eliminate the need for this assertion and improve type safety.🤖 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 `@src/react/hooks/useDensityGridCells.ts` at line 130, Replace the type assertion on useChart with a proper chart type that includes the optional pendingRecenter property: update the chart/interface/type used by the useChart hook (or create a more specific hook-return type) to declare pendingRecenter?: boolean, then remove the "as { pendingRecenter?: boolean }" cast in useDensityGridCells (where useChart() is called) so the code uses the typed return instead of an assertion.
🤖 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 `@src/react/components/avivatorish/MDVivViewer.tsx`:
- Around line 131-143: The layerFilter can accept layers from other viewports
because matchesDensityGridView(layer.id, layerViewId) is not scoped to the
active viewport; to fix, restrict calls that use layerViewId so they only run
when layerViewId refers to the active viewport: inside the block handling
layerViewId (string) first check that layerViewId === viewportId || layerViewId
=== getVivId(viewportId), and only then call matchesDensityGridView using the
active viewport identifiers (e.g., matchesDensityGridView(layer.id,
getVivId(viewportId)) or matchesDensityGridView(layer.id, viewportId)); update
the logic in layerFilter (the code using viewportId, layerViewId,
matchesDensityGridView, and getVivId) accordingly so matchesDensityGridView is
always evaluated with the active viewport scope.
In `@src/react/components/chartArrayLayout.css`:
- Around line 51-59: The calc() expressions for the flex basis and max-width are
splitting the division operator across lines which violates
scss/operator-no-newline-after; update the two calc() usages (the flex: 1 1 ...
and max-width: calc(...) lines) so the '/' operator is not followed by a newline
— for example move the '/' onto the same line as the numerator or collapse the
calc into a single line so the division reads "... ) /
var(--mdv-chart-array-columns)" using the existing CSS custom properties
(--mdv-chart-array-gap, --mdv-chart-array-columns).
In `@src/react/components/ChartArrayLayout.tsx`:
- Around line 56-60: cellRefs.current can retain stale entries when the number
of cells shrinks; add logic to truncate cellRefs.current to the current
cellCount whenever cellCount decreases (e.g., in a useEffect watching cellCount)
so layoutRef.cells won't expose DOM nodes past the active range, and update
setCellRef to guard against writing refs at indices >= cellCount (and similarly
apply the same truncation/guard to the other identical ref array usage around
the 91-100 block); reference cellRefs, setCellRef, layoutRef.cells and the
cellCount prop/state when implementing this change.
In `@src/react/components/DeckDensityGridComponent.tsx`:
- Around line 159-163: The onViewStateChange handler currently ignores the
viewId and replaces config.viewState with a single view state; modify the
handler (onViewStateChange) to destructure both viewId and viewState from its
parameter, call getSerializableViewState(viewState) for the next state, and set
config.viewState[viewId] = that serializable state (leaving other entries
intact) instead of overwriting the whole config.viewState map; ensure this
update is done inside the existing action() call so MobX state stays consistent.
---
Nitpick comments:
In `@src/react/components/ChartArrayLayout.tsx`:
- Around line 81-86: The code uses an "as string" cast when setting the CSS
custom property in ChartArrayLayout.tsx; replace that cast by creating a local
typed style object (e.g., define an interface extending React.CSSProperties with
"--mdv-chart-array-columns"?: string) and use that type for the object you
spread/return so you can assign ["--mdv-chart-array-columns"]:
String(gridColumns) without casting; update the branch where layoutMode ===
"grid" (and the variable style / gridColumns) to build/return this
strongly-typed style object instead of using the inline "as string" cast.
In `@src/react/components/VivDensityGridComponent.tsx`:
- Line 26: Replace the unsafe "as OME_TIFF[\"data\"]" cast on the value returned
from useLoader() by either calling useLoader with an explicit generic (e.g.
useLoader<OME_TIFF[\"data\"]>()) or by performing a local type-narrow using a
type guard helper (create an isOmeTiffData(value): value is OME_TIFF[\"data\"]
and check it before assigning to ome); update the variable assignment currently
named ome and any downstream consumers to use the narrowed/typed value so you
keep compile-time guarantees without using an "as" assertion.
In `@src/react/hooks/useDensityGridCells.ts`:
- Line 130: Replace the type assertion on useChart with a proper chart type that
includes the optional pendingRecenter property: update the chart/interface/type
used by the useChart hook (or create a more specific hook-return type) to
declare pendingRecenter?: boolean, then remove the "as { pendingRecenter?:
boolean }" cast in useDensityGridCells (where useChart() is called) so the code
uses the typed return instead of an assertion.
In `@src/react/hooks/useVivDensityGridViewState.ts`:
- Line 18: The code currently uses a type assertion on the result of useChart()
to add a pendingRecenter?: boolean property; remove the unsafe "as" cast by
extending the chart type or creating a more specific hook/type. Update the chart
type/interface used by useChart (or create a new hook like
useChartWithPendingRecenter) to include pendingRecenter?: boolean, then replace
the assertion in useVivDensityGridViewState with the correctly typed return
value from useChart (or the new hook) so pendingRecenter is available without
"as".
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f30aa34-1a8b-40f4-ad71-40369627ac57
📒 Files selected for processing (18)
src/charts/css/charts.csssrc/react/components/ChartArrayLayout.tsxsrc/react/components/DeckDensityGridComponent.tsxsrc/react/components/VivDensityGridComponent.tsxsrc/react/components/VivMDVReact.tsxsrc/react/components/VivScatterComponent.tsxsrc/react/components/avivatorish/MDVivViewer.tsxsrc/react/components/chartArrayGridUtils.tssrc/react/components/chartArrayLayout.csssrc/react/components/chartArrayLayoutUtils.test.tssrc/react/components/chartArrayLayoutUtils.tssrc/react/components/densityGridUtils.test.tssrc/react/components/densityGridUtils.tssrc/react/hooks/useChartArrayGrid.tssrc/react/hooks/useChartArrayLayoutGeometry.tssrc/react/hooks/useDensityGridCells.tssrc/react/hooks/useVivDensityGridViewState.tssrc/types/external-modules.d.ts
✅ Files skipped from review due to trivial changes (3)
- src/react/components/VivMDVReact.tsx
- src/types/external-modules.d.ts
- src/charts/css/charts.css
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/components/densityGridUtils.test.ts
Comment on lines
+51
to
+59
| flex: 1 1 | ||
| calc( | ||
| (100% - var(--mdv-chart-array-gap) * (var(--mdv-chart-array-columns) - 1)) / | ||
| var(--mdv-chart-array-columns) | ||
| ); | ||
| max-width: calc( | ||
| (100% - var(--mdv-chart-array-gap) * (var(--mdv-chart-array-columns) - 1)) / | ||
| var(--mdv-chart-array-columns) | ||
| ); |
Contributor
There was a problem hiding this comment.
Fix calc() formatting to satisfy Stylelint.
The division operator is split across lines, triggering scss/operator-no-newline-after (lines with / in the calc() expressions).
Suggested fix
.mdv-chart-array[data-layout="grid"] .mdv-chart-array__cell {
box-sizing: border-box;
flex: 1 1
calc(
- (100% - var(--mdv-chart-array-gap) * (var(--mdv-chart-array-columns) - 1)) /
- var(--mdv-chart-array-columns)
+ (100% - var(--mdv-chart-array-gap) * (var(--mdv-chart-array-columns) - 1)) / var(--mdv-chart-array-columns)
);
max-width: calc(
- (100% - var(--mdv-chart-array-gap) * (var(--mdv-chart-array-columns) - 1)) /
- var(--mdv-chart-array-columns)
+ (100% - var(--mdv-chart-array-gap) * (var(--mdv-chart-array-columns) - 1)) / var(--mdv-chart-array-columns)
);🧰 Tools
🪛 Stylelint (17.11.0)
[error] 53-53: Unexpected newline after "/" (scss/operator-no-newline-after)
(scss/operator-no-newline-after)
[error] 57-57: Unexpected newline after "/" (scss/operator-no-newline-after)
(scss/operator-no-newline-after)
🤖 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 `@src/react/components/chartArrayLayout.css` around lines 51 - 59, The calc()
expressions for the flex basis and max-width are splitting the division operator
across lines which violates scss/operator-no-newline-after; update the two
calc() usages (the flex: 1 1 ... and max-width: calc(...) lines) so the '/'
operator is not followed by a newline — for example move the '/' onto the same
line as the numerator or collapse the calc into a single line so the division
reads "... ) / var(--mdv-chart-array-columns)" using the existing CSS custom
properties (--mdv-chart-array-gap, --mdv-chart-array-columns).
Comment on lines
+56
to
+60
| const cellRefs = useRef<(HTMLDivElement | null)[]>([]); | ||
|
|
||
| const setCellRef = useCallback((index: number, element: HTMLDivElement | null) => { | ||
| cellRefs.current[index] = element; | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Truncate stale cell refs when cellCount decreases.
cellRefs.current can keep old entries after a shrink, so layoutRef.cells may expose stale DOM refs beyond current cells.
Suggested change
import {
useCallback,
+ useEffect,
useImperativeHandle,
useRef,
...
const cellRefs = useRef<(HTMLDivElement | null)[]>([]);
+ useEffect(() => {
+ cellRefs.current.length = cellCount;
+ }, [cellCount]);Also applies to: 91-100
🤖 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 `@src/react/components/ChartArrayLayout.tsx` around lines 56 - 60,
cellRefs.current can retain stale entries when the number of cells shrinks; add
logic to truncate cellRefs.current to the current cellCount whenever cellCount
decreases (e.g., in a useEffect watching cellCount) so layoutRef.cells won't
expose DOM nodes past the active range, and update setCellRef to guard against
writing refs at indices >= cellCount (and similarly apply the same
truncation/guard to the other identical ref array usage around the 91-100
block); reference cellRefs, setCellRef, layoutRef.cells and the cellCount
prop/state when implementing this change.
Comment on lines
+159
to
+163
| onViewStateChange={({ viewState: nextViewState }: { viewState: OrthographicViewState }) => { | ||
| action(() => { | ||
| config.viewState = getSerializableViewState(nextViewState); | ||
| })(); | ||
| }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For deck.gl v9.1.11, what is the exact onViewStateChangecallback payload shape whenviewState is provided as an object keyed by view id (multi-view setup)?
💡 Result:
In deck.gl v9.1.11, when using a multi-view setup where the viewState prop is provided as an object keyed by view ID, the onViewStateChange callback receives a single parameter object containing the following properties: { viewId: string, viewState: object, interactionState: object, newViewState: object } Specifically, for a multi-view setup: 1. The viewId property identifies which specific view triggered the change (e.g., 'main' or 'minimap') [1]. 2. The viewState property contains the updated state object for that specific view [1]. 3. The callback is triggered individually for each view that undergoes a state change [1]. When managing state in a multi-view setup, you typically use the viewId to determine which part of your state object to update before passing the new, merged state back to the Deck instance via setProps [1]. Example usage: onViewStateChange: ({viewId, viewState}) => { // Update your state object based on the viewId const newViewState = {...currentViewStates, [viewId]: viewState }; deckInstance.setProps({viewState: newViewState}); } [1]
Citations:
🏁 Script executed:
# Locate and examine the DeckDensityGridComponent.tsx file
fd -p "DeckDensityGridComponent.tsx" --type fRepository: Taylor-CCB-Group/MDV
Length of output: 114
🏁 Script executed:
# Read the complete DeckDensityGridComponent.tsx file
cat -n src/react/components/DeckDensityGridComponent.tsxRepository: Taylor-CCB-Group/MDV
Length of output: 9406
🏁 Script executed:
# Check the densityGridUtils to understand getDensityGridViewStates and getSerializableViewState
fd -p "densityGridUtils" --type fRepository: Taylor-CCB-Group/MDV
Length of output: 151
🏁 Script executed:
# Read the densityGridUtils.ts file to understand the utility functions
cat -n src/react/components/densityGridUtils.tsRepository: Taylor-CCB-Group/MDV
Length of output: 2134
🏁 Script executed:
# Check chartArrayGridUtils to understand the full context of getDensityGridViewStates
fd -p "chartArrayGridUtils" --type fRepository: Taylor-CCB-Group/MDV
Length of output: 108
🏁 Script executed:
# Read chartArrayGridUtils.ts to understand getDensityGridViewStates
cat -n src/react/components/chartArrayGridUtils.tsRepository: Taylor-CCB-Group/MDV
Length of output: 1611
Update onViewStateChange handler to use viewId for multi-view state management.
The handler receives both viewId and viewState from deck.gl's multi-view callback but ignores viewId. This causes single-view state updates to overwrite the entire config.viewState map instead of updating only the affected view's entry.
Update the handler to:
- Destructure
viewIdfrom the callback parameter - Update only the specific view's state in the
config.viewStatemap
Suggested fix
- onViewStateChange={({ viewState: nextViewState }: { viewState: OrthographicViewState }) => {
+ onViewStateChange={({ viewId, viewState: nextViewState }: { viewId: string; viewState: OrthographicViewState }) => {
action(() => {
- config.viewState = getSerializableViewState(nextViewState);
+ if (viewId && config.viewState) {
+ config.viewState[viewId] = getSerializableViewState(nextViewState);
+ }
})();
}}🤖 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 `@src/react/components/DeckDensityGridComponent.tsx` around lines 159 - 163,
The onViewStateChange handler currently ignores the viewId and replaces
config.viewState with a single view state; modify the handler
(onViewStateChange) to destructure both viewId and viewState from its parameter,
call getSerializableViewState(viewState) for the next state, and set
config.viewState[viewId] = that serializable state (leaving other entries
intact) instead of overwriting the whole config.viewState map; ensure this
update is done inside the existing action() call so MobX state stays consistent.
xinaesthete
force-pushed
the
codex/chart-array
branch
from
May 18, 2026 09:44
07554d4 to
23d8140
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/react/hooks/useChartArrayVisibleIndices.ts (1)
52-53: ⚡ Quick winReplace the
as HTMLElementcast with a runtime type guard.The
as HTMLElementcast on line 52 violates the coding guideline to avoidascasts in TypeScript. Useinstanceof HTMLElementto check the type before accessingdataset.Suggested change
- const index = Number((entry.target as HTMLElement).dataset.chartArrayIndex); + const target = entry.target; + if (!(target instanceof HTMLElement)) continue; + const index = Number(target.dataset.chartArrayIndex);This improves type safety and eliminates the unsafe cast.
🤖 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 `@src/react/hooks/useChartArrayVisibleIndices.ts` around lines 52 - 53, In useChartArrayVisibleIndices replace the unsafe TypeScript cast on entry.target with a runtime type guard: check that entry.target instanceof HTMLElement before accessing entry.target.dataset.chartArrayIndex; if the guard fails, continue the loop. Update the code path that computes index (using Number(...)) to only run inside the HTMLElement branch so you no longer need the "as HTMLElement" cast.
🤖 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.
Nitpick comments:
In `@src/react/hooks/useChartArrayVisibleIndices.ts`:
- Around line 52-53: In useChartArrayVisibleIndices replace the unsafe
TypeScript cast on entry.target with a runtime type guard: check that
entry.target instanceof HTMLElement before accessing
entry.target.dataset.chartArrayIndex; if the guard fails, continue the loop.
Update the code path that computes index (using Number(...)) to only run inside
the HTMLElement branch so you no longer need the "as HTMLElement" cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b77a139-e79d-47fa-bbc3-5a340c949a38
📒 Files selected for processing (34)
src/charts/css/charts.csssrc/charts/schemas/ChartConfigSchema.tssrc/react/components/ChartArrayLayout.tsxsrc/react/components/DeckDensityGridComponent.tsxsrc/react/components/DeckScatterComponent.tsxsrc/react/components/DeckScatterReactWrapper.tsxsrc/react/components/SelectionOverlay.tsxsrc/react/components/VivDensityGridComponent.tsxsrc/react/components/VivMDVReact.tsxsrc/react/components/VivScatterComponent.tsxsrc/react/components/avivatorish/MDVivViewer.tsxsrc/react/components/chartArrayGridUtils.tssrc/react/components/chartArrayLayout.csssrc/react/components/chartArrayLayoutUtils.test.tssrc/react/components/chartArrayLayoutUtils.tssrc/react/components/chartArrayUtils.test.tssrc/react/components/chartArrayUtils.tssrc/react/components/densityGridUtils.test.tssrc/react/components/densityGridUtils.tssrc/react/components/sharedScatterSettings.tssrc/react/contour_state.test.tssrc/react/contour_state.tssrc/react/hooks/useChartArrayGrid.tssrc/react/hooks/useChartArrayLayoutGeometry.tssrc/react/hooks/useChartArrayMetrics.tssrc/react/hooks/useChartArrayVisibleIndices.tssrc/react/hooks/useDensityGridCells.tssrc/react/hooks/useVivDensityGridViewState.tssrc/react/scatter_state.tssrc/tests/chartConfigSchema.spec.tssrc/types/external-modules.d.tssrc/webgl/SpatialLayer.tstests_playwright/project/charts/density_grid_selection.spec.tstests_playwright/utils/helpers.ts
✅ Files skipped from review due to trivial changes (2)
- src/react/scatter_state.ts
- src/charts/css/charts.css
🚧 Files skipped from review as they are similar to previous changes (27)
- src/tests/chartConfigSchema.spec.ts
- src/react/components/VivMDVReact.tsx
- src/charts/schemas/ChartConfigSchema.ts
- src/react/components/DeckScatterReactWrapper.tsx
- src/react/components/chartArrayUtils.test.ts
- src/webgl/SpatialLayer.ts
- src/react/components/VivScatterComponent.tsx
- src/react/components/sharedScatterSettings.ts
- src/types/external-modules.d.ts
- src/react/components/chartArrayLayoutUtils.ts
- src/react/components/ChartArrayLayout.tsx
- tests_playwright/utils/helpers.ts
- src/react/components/chartArrayGridUtils.ts
- src/react/components/chartArrayLayoutUtils.test.ts
- src/react/hooks/useChartArrayMetrics.ts
- src/react/contour_state.test.ts
- src/react/components/avivatorish/MDVivViewer.tsx
- src/react/components/SelectionOverlay.tsx
- src/react/hooks/useChartArrayGrid.ts
- src/react/components/densityGridUtils.ts
- src/react/components/DeckScatterComponent.tsx
- src/react/components/chartArrayUtils.ts
- src/react/components/densityGridUtils.test.ts
- src/react/components/VivDensityGridComponent.tsx
- src/react/contour_state.ts
- src/react/hooks/useDensityGridCells.ts
- src/react/hooks/useVivDensityGridViewState.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/react/components/avivatorish/MDVivViewer.tsx (1)
402-408:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid inserting
ScaleBarLayertwice in the no-deckProps.layerspath.At Line 403 and Line 407, the fallback uses
vivLayers(which already includes the scale bar), then appendsscaleBarLayeragain.Suggested fix
- const layers = deckProps?.layers === undefined - ? [vivLayers] - : [otherLayers, ...deckProps.layers]; + const layers = deckProps?.layers === undefined + ? [otherLayers] + : [otherLayers, ...deckProps.layers]; // XXX: including an undefined scaleBarLayer above causes some other layers to not render if (scaleBarLayer) { layers.push(scaleBarLayer); }🤖 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 `@src/react/components/avivatorish/MDVivViewer.tsx` around lines 402 - 408, The code can add scaleBarLayer twice because vivLayers already contains the ScaleBarLayer; when deckProps?.layers is undefined you should not append scaleBarLayer again. Change the logic around the layers array (the variable layers, and the branch using deckProps, vivLayers and scaleBarLayer) so that you only push scaleBarLayer when it exists AND you're in the branch that uses deckProps.layers (i.e., when deckProps?.layers !== undefined), or alternatively check that vivLayers does not already contain the scale bar before pushing; update the push condition for scaleBarLayer to avoid duplicating it.
♻️ Duplicate comments (1)
src/react/components/densityGridUtils.ts (1)
105-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
layerViewIdmatching to the active viewport only.At Line 108,
matchesDensityGridView(layer.id, layerViewId)can returntrueeven whenlayerViewId !== viewportId, so layers can render in unrelated grid cells.Suggested fix
const layerViewId = layer.props?.viewId; if (typeof layerViewId === "string") { return ( - matchesDensityGridView(layer.id, layerViewId) || matchesDensityGridView(layer.id, viewportId) || layerViewId === viewportId ); }🤖 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 `@src/react/components/densityGridUtils.ts` around lines 105 - 111, The condition currently allows matchesDensityGridView(layer.id, layerViewId) to gate rendering even when layerViewId !== viewportId; restrict that check so it only applies when the layerViewId actually targets the active viewport. Update the return expression in the block that references layerViewId so the first term is guarded (e.g., change matchesDensityGridView(layer.id, layerViewId) to matchesDensityGridView(layer.id, layerViewId) && layerViewId === viewportId) or remove that term entirely, leaving only matchesDensityGridView(layer.id, viewportId) || layerViewId === viewportId; this ensures matchesDensityGridView is evaluated only for the active viewport (functions/variables: matchesDensityGridView, layerViewId, viewportId, layer.id).
🤖 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 `@src/react/components/SelectionOverlay.tsx`:
- Around line 319-320: The aria-label for the density toggle is inverted and
should reflect the action the control will perform (the state it will switch
to), so update the JSX where the attribute is set (the element using
"aria-label": isDensityGrid ? "Density layer view" : "Density grid view") to use
labels that describe the resulting view when clicked (e.g., when isDensityGrid
is true the label should be "Single scatter view", otherwise "Density grid
view") so screen readers announce the correct transition; locate the toggle in
SelectionOverlay (reference isDensityGrid and the aria-label attribute) and
replace the labels accordingly.
In `@src/react/spatial_context.tsx`:
- Around line 193-199: The effect in useEffect that updates
selectedFeatureIndexes based on editingGateId and
selectionFeatureCollection.features.length can leave stale indexes when
editingGateId is set but features.length becomes 0; update the logic in the
effect (the callback registered with useEffect) to explicitly clear
selectedFeatureIndexes (call setSelectedFeatureIndexes([])) when editingGateId
is truthy but selectionFeatureCollection.features.length === 0, while keeping
the existing branches that set [0] when length > 0 and clear when editingGateId
is falsy; reference the useEffect, editingGateId,
selectionFeatureCollection.features.length, setSelectedFeatureIndexes and
selectedFeatureIndexes symbols when making the change.
---
Outside diff comments:
In `@src/react/components/avivatorish/MDVivViewer.tsx`:
- Around line 402-408: The code can add scaleBarLayer twice because vivLayers
already contains the ScaleBarLayer; when deckProps?.layers is undefined you
should not append scaleBarLayer again. Change the logic around the layers array
(the variable layers, and the branch using deckProps, vivLayers and
scaleBarLayer) so that you only push scaleBarLayer when it exists AND you're in
the branch that uses deckProps.layers (i.e., when deckProps?.layers !==
undefined), or alternatively check that vivLayers does not already contain the
scale bar before pushing; update the push condition for scaleBarLayer to avoid
duplicating it.
---
Duplicate comments:
In `@src/react/components/densityGridUtils.ts`:
- Around line 105-111: The condition currently allows
matchesDensityGridView(layer.id, layerViewId) to gate rendering even when
layerViewId !== viewportId; restrict that check so it only applies when the
layerViewId actually targets the active viewport. Update the return expression
in the block that references layerViewId so the first term is guarded (e.g.,
change matchesDensityGridView(layer.id, layerViewId) to
matchesDensityGridView(layer.id, layerViewId) && layerViewId === viewportId) or
remove that term entirely, leaving only matchesDensityGridView(layer.id,
viewportId) || layerViewId === viewportId; this ensures matchesDensityGridView
is evaluated only for the active viewport (functions/variables:
matchesDensityGridView, layerViewId, viewportId, layer.id).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cdc2aeba-5676-4099-8ff8-b3df9a900d8e
📒 Files selected for processing (14)
AGENTS.mdpackage.jsonsrc/lib/deckMonkeypatch.tssrc/react/components/DeckDensityGridComponent.tsxsrc/react/components/DeckScatterComponent.tsxsrc/react/components/SelectionOverlay.tsxsrc/react/components/VivDensityGridComponent.tsxsrc/react/components/avivatorish/MDVivViewer.tsxsrc/react/components/densityGridUtils.test.tssrc/react/components/densityGridUtils.tssrc/react/hooks/useDeckSelectionMouseRebind.tssrc/react/hooks/useGateLayers.tssrc/react/spatial_context.tsxtests_playwright/project/charts/density_grid_selection.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/react/components/DeckScatterComponent.tsx
- src/react/components/DeckDensityGridComponent.tsx
- src/react/components/VivDensityGridComponent.tsx
Comment on lines
+319
to
+320
| "aria-label": isDensityGrid ? "Density layer view" : "Density grid view", | ||
| }} |
Contributor
There was a problem hiding this comment.
Align toggle accessible name with the actual action/state transition.
Line 319 uses "Density layer view" while the UI action/expectation is “Single scatter view” in grid mode, which makes screen-reader labeling inconsistent and can break role-name queries.
Suggested fix
- "aria-label": isDensityGrid ? "Density layer view" : "Density grid view",
+ "aria-label": isDensityGrid ? "Single scatter view" : "Density grid view",📝 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
| "aria-label": isDensityGrid ? "Density layer view" : "Density grid view", | |
| }} | |
| "aria-label": isDensityGrid ? "Single scatter view" : "Density grid view", | |
| }} |
🤖 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 `@src/react/components/SelectionOverlay.tsx` around lines 319 - 320, The
aria-label for the density toggle is inverted and should reflect the action the
control will perform (the state it will switch to), so update the JSX where the
attribute is set (the element using "aria-label": isDensityGrid ? "Density layer
view" : "Density grid view") to use labels that describe the resulting view when
clicked (e.g., when isDensityGrid is true the label should be "Single scatter
view", otherwise "Density grid view") so screen readers announce the correct
transition; locate the toggle in SelectionOverlay (reference isDensityGrid and
the aria-label attribute) and replace the labels accordingly.
Collaborator
Author
|
I think there is currently a bug with the gate layers which means that it is appearing more intense, probably because it's making a clone for each view and then not filtering the ones from outside the view (or something). You can see this change as you scroll and a different number of views are active... |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/react/components/deckLayerViewportScope.ts (1)
42-55: ⚡ Quick winAvoid the
as Lcast by tightening the generic constraint.
tagDeckLayerViewportScopecan be typed soclonereturnsL, which removes the unsafe cast.As per coding guidelines: `**/*.{ts,tsx}`: Avoid `as` casts where possible.Suggested fix
-type CloneableDeckLayer = { - clone: (props: Record<string, unknown>) => unknown; -}; - -export function tagDeckLayerViewportScope<L extends CloneableDeckLayer>( +export function tagDeckLayerViewportScope<L extends { clone: (props: Record<string, unknown>) => L }>( layer: L, scope: DeckLayerViewportScope, extraProps?: Record<string, unknown>, ): L { return layer.clone({ ...extraProps, [MDV_DECK_LAYER_VIEWPORT_SCOPE]: scope, - }) as L; + }); }🤖 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 `@src/react/components/deckLayerViewportScope.ts` around lines 42 - 55, The clone signature is too loose causing the unsafe "as L" cast in tagDeckLayerViewportScope; tighten CloneableDeckLayer so clone returns the same concrete type and update the generic accordingly. Change CloneableDeckLayer to a self-referential generic (e.g., CloneableDeckLayer<T>) where clone returns T, make tagDeckLayerViewportScope generic as L extends CloneableDeckLayer<L>, call layer.clone(...) and remove the "as L" cast; keep MDV_DECK_LAYER_VIEWPORT_SCOPE and extraProps usage unchanged.
🤖 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 `@src/react/components/densityGridUtils.ts`:
- Around line 64-65: getViewportAtCanvasPoint currently returns viewports[0] as
a fallback which can cause unprojectCanvasPoint to use the wrong cell; change
getViewportAtCanvasPoint to return undefined when no viewport contains the point
(so callers like unprojectCanvasPoint can exercise their explicit fallback path)
and make the analogous change in the other block referenced (lines ~73-77) where
a default viewport is returned — replace that default return with undefined and
ensure callers handle the undefined result where appropriate.
---
Nitpick comments:
In `@src/react/components/deckLayerViewportScope.ts`:
- Around line 42-55: The clone signature is too loose causing the unsafe "as L"
cast in tagDeckLayerViewportScope; tighten CloneableDeckLayer so clone returns
the same concrete type and update the generic accordingly. Change
CloneableDeckLayer to a self-referential generic (e.g., CloneableDeckLayer<T>)
where clone returns T, make tagDeckLayerViewportScope generic as L extends
CloneableDeckLayer<L>, call layer.clone(...) and remove the "as L" cast; keep
MDV_DECK_LAYER_VIEWPORT_SCOPE and extraProps usage 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ef4fef5-c227-471f-86d2-83385b78d96d
📒 Files selected for processing (9)
src/react/components/DeckDensityGridComponent.tsxsrc/react/components/VivDensityGridComponent.tsxsrc/react/components/VivScatterComponent.tsxsrc/react/components/deckLayerViewportScope.test.tssrc/react/components/deckLayerViewportScope.tssrc/react/components/densityGridUtils.test.tssrc/react/components/densityGridUtils.tssrc/react/hooks/useGateLayers.tssrc/react/spatial_context.tsx
✅ Files skipped from review due to trivial changes (1)
- src/react/components/deckLayerViewportScope.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/react/components/DeckDensityGridComponent.tsx
- src/react/components/VivDensityGridComponent.tsx
- src/react/components/densityGridUtils.test.ts
Comment on lines
+64
to
+65
| return viewports[0]; | ||
| } |
Contributor
There was a problem hiding this comment.
Return undefined when no viewport contains the point.
getViewportAtCanvasPoint currently falls back to viewports[0], so unprojectCanvasPoint can unproject against the wrong cell instead of using its explicit fallback path.
Suggested fix
export function getViewportAtCanvasPoint(
viewports: readonly DeckCanvasViewport[],
canvasX: number,
canvasY: number,
): DeckCanvasViewport | undefined {
@@
- return viewports[0];
+ return undefined;
}Also applies to: 73-77
🤖 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 `@src/react/components/densityGridUtils.ts` around lines 64 - 65,
getViewportAtCanvasPoint currently returns viewports[0] as a fallback which can
cause unprojectCanvasPoint to use the wrong cell; change
getViewportAtCanvasPoint to return undefined when no viewport contains the point
(so callers like unprojectCanvasPoint can exercise their explicit fallback path)
and make the analogous change in the other block referenced (lines ~73-77) where
a default viewport is returned — replace that default return with undefined and
ensure callers handle the undefined result where appropriate.
…mponents - Introduced `supportsDensityGridMode` utility to determine if the current chart type supports density grid mode. - Updated `SelectionOverlay` to conditionally show/hide shape-drawing tools based on the density grid mode. - Enhanced `DeckDensityGridComponent` to integrate gate layers and adjust layout calculations for viewport dimensions. - Modified `DeckScatterComponent` to check for density grid mode compatibility. - Added Playwright tests to verify the behavior of the selection overlay in grid mode. - Refactored density grid layout calculations to use a consistent cell size and content dimensions. interactive rebase in progress; onto acd62cf Conflicts: tests_playwright/utils/helpers.ts
…ensityGridComponent - Implemented the `useFieldContour` hook to build contours based on specified visible field indices. - Updated `FieldContourProps` to include `visibleFieldIndices` for selective contour rendering. - Enhanced `DeckDensityGridComponent` to utilize virtualized cell indices for improved performance and rendering of density grid layers. - Added utility function `getDensityGridVisibleCellIndices` to calculate visible cell indices based on virtualized row and column ranges. - Updated tests to verify the functionality of the new visible cell index calculations.
…hart rendering - Introduced the `ChartArrayLayout` component to manage a grid layout for charts, allowing for reusable and responsive chart arrangements. - Added CSS styles for the new grid layout, including customizable gaps and padding. - Implemented utility functions in `chartArrayUtils` for measuring cell dimensions and root size, enhancing layout calculations. - Created hooks `useChartArrayMetrics` and `useChartArrayVisibleIndices` to manage metrics and visibility of chart cells within the layout. - Developed tests for utility functions to ensure correct behavior in various scenarios. - Refactored `DeckDensityGridComponent` to utilize the new layout and metrics for improved rendering performance and responsiveness.
- Added a new `canvasOverlay` prop to the `ChartArrayLayout` component to support rendering overlays like DeckGL. - Introduced CSS styles for the canvas overlay to ensure proper positioning and responsiveness. - Implemented the `snapRectToDevicePixels` utility function to align chart dimensions with the device pixel grid, enhancing visual clarity. - Updated `DeckDensityGridComponent` to utilize the new canvas overlay for better rendering of density grid layers. - Enhanced `useChartArrayMetrics` to manage resize events more effectively, ensuring layout updates on window resize.
…n grid but also underlying density-field update logic. - Added update triggers for contour layers in `createBaseContourLayerProps` and `buildFieldContourLayerProps` to ensure proper reactivity to position changes. - Refactored `DeckDensityGridComponent` to utilize new hooks for managing chart dimensions and visibility, improving responsiveness and layout fitting. - Implemented a new utility function `fitViewStateToFilteredRows` to dynamically adjust the view state based on filtered data, enhancing user experience. - Updated `useZoomOnFilter` to conditionally apply zoom behavior based on the enabled state, allowing for more flexible chart interactions. - Added tests for the new utility functions to ensure correct behavior in various scenarios.
- Introduced `chartArrayGridUtils` with functions for generating view IDs and managing orthographic view states. - Added `VivDensityGridComponent` for rendering density grids in Viv charts, utilizing new hooks for grid management. - Updated `DeckDensityGridComponent` to leverage new density grid utilities and improve responsiveness. - Enhanced `densityGridUtils` to support additional chart types and refactored related functions for better modularity. - Added tests for new utility functions and components to ensure correct behavior and integration.
…e grid rendering - Removed legacy CSS styles for chart arrays and added a new `chartArrayLayout.css` for improved layout management. - Implemented the `ChartArrayLayout` component with enhanced layout modes (single, pair, grid) and responsive behavior. - Created utility functions for calculating grid dimensions and viewport fitting, ensuring optimal rendering of chart arrays. - Added a new hook, `useChartArrayLayoutGeometry`, to manage layout geometry based on cell count and viewport size. - Introduced tests for layout utilities to validate functionality and responsiveness across different scenarios.
- Introduced `getMapCoords` and `getPicks` methods in `MonkeyPatchEditableGeoJsonLayer` to improve coordinate mapping and object picking in multi-viewport scenarios. - Added `useDeckSelectionMouseRebind` hook to streamline mouse event rebinding between overlay and density grid modes. - Updated `DeckDensityGridComponent` and `DeckScatterComponent` to utilize the new mouse rebind functionality, ensuring consistent interaction behavior. - Enhanced utility functions in `densityGridUtils` for better viewport management and selection layer identification. - Refactored `SelectionOverlay` to maintain tool visibility based on grid mode and editing state. - Improved tests to validate new functionalities and ensure correct behavior across components.
xinaesthete
force-pushed
the
codex/chart-array
branch
from
May 28, 2026 14:59
672f67e to
750eccd
Compare
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
2 participants
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.
Intention in future is to have more general ways of showing several plots in a single chart (with lazy-loading).
For now, this is a first-version which just adds a new mode in "scatterplot with density".
Summary by CodeRabbit
New Features
Bug Fixes
Tests