clean selectors#672
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new interactive DimSlicer component suite and integrates it via MetaDimSelector in Variables.tsx to replace the old metadata view with advanced dimension slicing controls. The review feedback highlights several critical issues, including an infinite re-render loop in MetaDimSelector caused by unstable array references, potential runtime crashes from unsafe property access on meta, potential NaN calculations in DimSlicer when values is empty, input flickering in the numeric stepper, and missing prop passing or guards in Variables.tsx.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const rawDimArrays = meta?.dimInfo?.dimArrays ?? []; | ||
| const rawDimNames = meta?.dimInfo?.dimNames ?? []; | ||
| const rawDimUnits = meta?.dimInfo?.dimUnits ?? []; | ||
| const dataShape = meta?.shape; | ||
| const chunkShape = meta?.chunks; | ||
|
|
||
| const dimArrays: number[][] = useMemo( | ||
| () => rawDimArrays.map((a) => Array.from(a)), | ||
| [rawDimArrays], | ||
| ); | ||
| const dimUnits: string[] = useMemo( | ||
| () => rawDimUnits.map((u) => u ?? ''), | ||
| [rawDimUnits], | ||
| ); | ||
| const dimNames: string[] = rawDimNames; |
There was a problem hiding this comment.
The fallback arrays [] in rawDimArrays, rawDimNames, and rawDimUnits create new array references on every render when meta is null or undefined. Since dimNames is not memoized and dimArrays/dimUnits depend on these unstable references, the useEffect that updates the global store will run on every render, causing an infinite re-render loop. Memoizing these values directly using optional chaining on meta resolves this issue.
| const rawDimArrays = meta?.dimInfo?.dimArrays ?? []; | |
| const rawDimNames = meta?.dimInfo?.dimNames ?? []; | |
| const rawDimUnits = meta?.dimInfo?.dimUnits ?? []; | |
| const dataShape = meta?.shape; | |
| const chunkShape = meta?.chunks; | |
| const dimArrays: number[][] = useMemo( | |
| () => rawDimArrays.map((a) => Array.from(a)), | |
| [rawDimArrays], | |
| ); | |
| const dimUnits: string[] = useMemo( | |
| () => rawDimUnits.map((u) => u ?? ''), | |
| [rawDimUnits], | |
| ); | |
| const dimNames: string[] = rawDimNames; | |
| const dimArrays = useMemo( | |
| () => (meta?.dimInfo?.dimArrays ?? []).map((a) => Array.from(a)), | |
| [meta?.dimInfo?.dimArrays] | |
| ); | |
| const dimUnits = useMemo( | |
| () => (meta?.dimInfo?.dimUnits ?? []).map((u) => u ?? ''), | |
| [meta?.dimInfo?.dimUnits] | |
| ); | |
| const dimNames = useMemo( | |
| () => meta?.dimInfo?.dimNames ?? [], | |
| [meta?.dimInfo?.dimNames] | |
| ); | |
| const dataShape = meta?.shape; | |
| const chunkShape = meta?.chunks; |
|
|
||
| return ( | ||
| <> | ||
| <b>{`${meta.long_name} `}</b> |
There was a problem hiding this comment.
Accessing meta.long_name directly can throw a TypeError if meta is null or undefined (for example, if the metadata is loaded but the dimension info is still being fetched or failed). Using optional chaining with a fallback ensures the component renders safely.
| <b>{`${meta.long_name} `}</b> | |
| <b>{`${meta?.long_name ?? ''} `}</b> |
| const getIndexFromValue = (val: number): number => { | ||
| if (!values) { | ||
| return clamp(Math.round(val / step) * step, 0, maxIndex); | ||
| } |
There was a problem hiding this comment.
If values is an empty array, values[0] will be undefined, leading to NaN calculations in Math.abs(values[0] - val). Adding a guard clause for empty or undefined values prevents potential runtime errors and incorrect index clamping.
| const getIndexFromValue = (val: number): number => { | |
| if (!values) { | |
| return clamp(Math.round(val / step) * step, 0, maxIndex); | |
| } | |
| const getIndexFromValue = (val: number): number => { | |
| if (!values || values.length === 0) { | |
| return clamp(Math.round(val / step) * step, 0, maxIndex); | |
| } |
| const commitValue = () => { | ||
| onValueChange(localValue); | ||
| setLocalValue(value); | ||
| }; |
There was a problem hiding this comment.
Calling setLocalValue(value) synchronously right after onValueChange(localValue) will reset the input to the old prop value before the parent has a chance to re-render with the new value. This causes a visible flickering effect. Removing the synchronous reset allows the prop sync in the useEffect to handle the update smoothly.
const commitValue = () => {
onValueChange(localValue);
};
| > | ||
| {meta && ( | ||
| <MetaDataInfo | ||
| {metadata && ( |
There was a problem hiding this comment.
Rendering MetaDimSelector when only metadata is loaded but meta is still null can cause runtime crashes (e.g., when accessing meta.long_name) and displays an incomplete UI. Ensuring both meta and metadata are available before rendering prevents these issues.
| {metadata && ( | |
| {meta && metadata && ( |
| <MetaDimSelector | ||
| key={selectedVar || "none"} | ||
| meta={meta} | ||
| metadata={metadata ?? {}} | ||
| setShowMeta={setShowMeta} | ||
| setOpenVariables={setOpenVariables} | ||
| popoverSide={"top"} | ||
| onApply={(sels, axes) => { |
There was a problem hiding this comment.
In the dialog view (when popoverSide === "top"), the metadata prop is not passed to MetaDimSelector. This will cause the 'Attributes' popover inside the selector to be empty. Passing metadata ensures attributes are displayed correctly in both views.
| <MetaDimSelector | |
| key={selectedVar || "none"} | |
| meta={meta} | |
| metadata={metadata ?? {}} | |
| setShowMeta={setShowMeta} | |
| setOpenVariables={setOpenVariables} | |
| popoverSide={"top"} | |
| onApply={(sels, axes) => { | |
| <MetaDimSelector | |
| key={selectedVar || "none"} | |
| meta={meta} | |
| metadata={metadata ?? undefined} | |
| onApply={(sels, axes) => { |
This is not supposed to be merge directly, but to be merged into a new branch just to bring in the DimSlicer components.