ui select dimensions#665
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new multi-dimensional slicing feature by adding the DimSlicer component and its subcomponents, replacing the older MetaDataInfo with MetaDimSelector in Variables.tsx. The code review identified several critical issues, including a potential runtime crash in Variables.tsx due to unchecked props, a missing metadata prop regression on mobile, and potential NaN operations in DimSlicer.tsx when handling empty arrays. Additionally, there are opportunities to improve performance and stability by avoiding unstable references that risk infinite render loops, removing global mutable state and side effects from state updaters, and moving static helper functions outside of component definitions.
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.
| > | ||
| {meta && ( | ||
| <MetaDataInfo | ||
| {metadata && ( |
There was a problem hiding this comment.
🔴 Potential Runtime Crash
Only metadata is checked for truthiness before rendering <MetaDimSelector />, but meta is passed as a required prop. If meta is null (e.g., during state transitions or resets), the application will crash with a TypeError when trying to access meta.long_name inside MetaDimSelector.
Ensure both meta and metadata are truthy before rendering.
| {metadata && ( | |
| {meta && metadata && ( |
| 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.
🔴 Unstable References & Infinite Render Loop Risk
Using inline nullish coalescing fallbacks like meta?.dimInfo?.dimArrays ?? [] creates a new array reference on every render when the value is undefined. This causes the useMemo hooks for dimArrays and dimUnits to re-evaluate on every render, which in turn triggers the useEffect that updates the global store, potentially causing infinite render loops.
Additionally, dimNames is not memoized and will also change reference on every render.
Fix: Avoid inline fallbacks for raw variables and handle the fallback array inside the useMemo hooks to keep references stable.
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 ? rawDimArrays.map((a) => Array.from(a)) : []),
[rawDimArrays],
);
const dimUnits: string[] = useMemo(
() => (rawDimUnits ? rawDimUnits.map((u) => u ?? '') : []),
[rawDimUnits],
);
const dimNames: string[] = useMemo(
() => rawDimNames ?? [],
[rawDimNames],
);
| <MetaDimSelector | ||
| meta={meta} |
There was a problem hiding this comment.
⚠️ Missing metadata Prop (Regression)
The metadata prop is omitted when rendering <MetaDimSelector /> inside the mobile dialog. This is a regression from the previous <MetaDataInfo /> implementation and will prevent metadata attributes from being displayed on mobile screens.
Pass the metadata prop to ensure attributes are rendered correctly.
<MetaDimSelector
meta={meta}
metadata={metadata}
| const sel = lockMode ? { ...rawSel, mode: lockMode } : rawSel; | ||
|
|
||
| const getIndexFromValue = (val: number): number => { | ||
| if (!values) return val; |
There was a problem hiding this comment.
⚠️ Potential NaN Math Operations
If values is an empty array ([]), values[0] will be undefined. This leads to NaN in Math.abs(values[0] - val) and subsequent comparison operations.
Add a guard to return early if values is empty.
| if (!values) return val; | |
| if (!values || values.length === 0) return val; |
| const addRow = () => { | ||
| setRows((prev) => { | ||
| if (prev.length >= MAX_ACTIVE_DIMS) return prev; | ||
| const dimName = firstUnusedDim(prev); | ||
| if (!dimName) return prev; | ||
| const dim = availableDims.find((d) => d.name === dimName)!; | ||
| const newRow: SlicerRow = { | ||
| id: nextId(), | ||
| dimName, | ||
| sel: { ...defaultSelection(dim.size), mode: 'slice' }, | ||
| axis: defaultAxisForIndex(prev.length, prev.length + 1), | ||
| }; | ||
| return [...prev, newRow]; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
⚠️ Global Mutable State & Side Effects in State Updater
Using a global mutable variable _nextId for generating row IDs can cause Next.js SSR hydration mismatches (since the server and client counters can get out of sync). Furthermore, calling nextId() inside the setRows functional updater is a side effect, which is discouraged because React can run updater functions multiple times in Strict Mode.
Fix: Generate a unique ID (e.g., using Math.random()) directly in the event handler before updating the state, and remove the global _nextId and nextId helper functions.
const addRow = () => {
if (rows.length >= MAX_ACTIVE_DIMS) return;
const dimName = firstUnusedDim(rows);
if (!dimName) return;
const dim = availableDims.find((d) => d.name === dimName)!;
const newRow: SlicerRow = {
id: Math.random(),
dimName,
sel: { ...defaultSelection(dim.size), mode: 'slice' },
axis: defaultAxisForIndex(rows.length, rows.length + 1),
};
setRows((prev) => [...prev, newRow]);
};
| const makeInitialCollapsedSels = (dims: DimOption[]): Record<string, SliceSelectionState> => | ||
| Object.fromEntries(dims.map((d) => [d.name, { ...defaultSelection(d.size), mode: 'scalar' as const }])); |
There was a problem hiding this comment.
⚙️ Move Helper Function Outside Component
makeInitialCollapsedSels is defined inside the component body but does not depend on any component state or props. Defining it inside the component causes it to be recreated on every render.
Move makeInitialCollapsedSels outside the MetaDimSelector component to improve maintainability and performance.
This one adds most of the features and layout we discussed.
Test with
@TheJeran
Edit: All feedback on UI is done. Please take whatever works and optimise where needed. Zero effort has been made on making things performant.