Upgrade frontend deps #278
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR removes the visx visualization dependency and why-did-you-render integration, upgrades React and its types to v19, updates several dependencies (lucide-react, mobx, mobx-react-lite), widens multiple internal ref/nullability types, and comments out visx-based axis rendering with warning logs. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / Entry
participant WDYR_old as dynamic import('why-did-you-render')
participant WDYR_new as (commented out)
rect rgb(220,235,255)
note right of App: Previous runtime flow
App->>WDYR_old: dynamic import & init on URL param
WDYR_old-->>App: whyDidYouRender enabled
end
rect rgb(255,235,220)
note right of App: New runtime flow (this PR)
App->>WDYR_new: (no-op, code commented out)
App-->>App: console.warn('WDYR disabled / React 19 incompatible')
end
sequenceDiagram
participant Chart as Chart components
participant visx_old as visx Axis/Scale
participant visx_new as (removed)
rect rgb(220,235,255)
note right of Chart: Previous: axis rendered
Chart->>visx_old: create scales & AxisBottom/AxisLeft
visx_old-->>Chart: axis elements
end
rect rgb(255,235,220)
note right of Chart: New: axis disabled
Chart->>visx_new: (imports and axis calls commented)
Chart-->>Chart: console.warn('visx removed - axis disabled')
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/AxisComponent.tsx (1)
122-159: Axis rendering completely disabled.All axis rendering is commented out, leaving scatter plots without axes. This is related to the visx removal issue flagged in package.json.
This should be tracked as part of the visx migration effort. The component needs a complete reimplementation of axis rendering using an alternative approach.
🧹 Nitpick comments (1)
src/react/components/AxisComponent.tsx (1)
17-28: Remove or document unused helper function.The
getLabelPropsfunction is defined but never called since all axis rendering is commented out. Consider either removing it or adding a TODO comment indicating it will be needed when axis rendering is restored.Apply this diff to add context:
//! todo - get label options working... for now, we ignore rotate_labels // not generally relevant to scatterplot, but will be for other charts +//! Currently unused due to visx removal - will be needed when axis rendering is restored function getLabelProps(axisConfig?: AxisConfig) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(2 hunks)src/modules/wdyr.ts(1 hunks)src/react/components/AxisComponent.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T21:07:38.683Z
Learning: When building interactive data visualizations in React that require D3.js functionality (like brushing/zooming), using a dedicated bridging library like visx (visx/brush) is preferable to manual DOM manipulation. It eliminates circular update issues and complex state management by providing React components that abstract D3's imperative approach.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T20:29:56.245Z
Learning: When working with D3.js in React components, it's appropriate to intentionally exclude certain dependencies from useEffect dependency arrays to prevent unnecessary recreation of D3 elements. This approach should be documented with explanatory comments and ESLint suppression comments like `// eslint-disable-next-line react-hooks/exhaustive-deps` to clarify the intentional exclusion.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The project uses MobX for state management in chart configurations, with `mobxAutorun` and reaction disposers to manage reactive updates based on changes to linked data sources.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:58-67
Timestamp: 2025-04-23T09:25:51.968Z
Learning: When implementing deck.gl components in React, axis scales can appear to be a frame behind due to synchronization issues between deck.gl's render cycle and React's component updates.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:46-46
Timestamp: 2025-04-23T09:26:52.252Z
Learning: In React dependency arrays, conditional expressions (e.g., `is2d && config.axis.x.size`) can be necessary for type safety when some properties might not exist on all possible types. In these cases, extracting safe values before useMemo or adding a lint exception with explanation is preferred.
📚 Learning: 2025-04-09T20:29:56.245Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T20:29:56.245Z
Learning: When working with D3.js in React components, it's appropriate to intentionally exclude certain dependencies from useEffect dependency arrays to prevent unnecessary recreation of D3 elements. This approach should be documented with explanatory comments and ESLint suppression comments like `// eslint-disable-next-line react-hooks/exhaustive-deps` to clarify the intentional exclusion.
Applied to files:
src/modules/wdyr.tssrc/react/components/AxisComponent.tsx
📚 Learning: 2025-04-09T20:54:14.593Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T20:54:14.593Z
Learning: In React components that use D3.js for visualization, preventing infinite update loops requires strategic flags (like updatingProgrammatically.current) to track the source of updates, minimal debouncing for store updates, and setTimeout(0) to reset flags asynchronously after programmatic UI changes.
Applied to files:
src/modules/wdyr.tssrc/react/components/AxisComponent.tsx
📚 Learning: 2025-05-30T11:08:52.145Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 208
File: src/react/components/PopoutWindow.tsx:11-11
Timestamp: 2025-05-30T11:08:52.145Z
Learning: For PopoutWindow components in React with MUI, use async/await with Promise.all to fetch and inline CSS content from link tags in parallel, combined with a loading state that hides content until all CSS operations complete, to avoid blank screens and provide better user experience.
Applied to files:
src/modules/wdyr.ts
📚 Learning: 2025-04-23T09:26:52.252Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:46-46
Timestamp: 2025-04-23T09:26:52.252Z
Learning: In React dependency arrays, conditional expressions (e.g., `is2d && config.axis.x.size`) can be necessary for type safety when some properties might not exist on all possible types. In these cases, extracting safe values before useMemo or adding a lint exception with explanation is preferred.
Applied to files:
src/modules/wdyr.tssrc/react/components/AxisComponent.tsx
📚 Learning: 2025-05-30T10:51:04.022Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 208
File: src/react/components/PopoutWindow.tsx:11-11
Timestamp: 2025-05-30T10:51:04.022Z
Learning: For PopoutWindow components in React with MUI, avoid async CSS fetching operations as they cause blank screens. Instead, copy link tags directly and rely on Emotion CacheProvider for CSS-in-JS styling to work properly in popout windows.
Applied to files:
src/modules/wdyr.ts
📚 Learning: 2025-04-09T21:07:38.683Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T21:07:38.683Z
Learning: When building interactive data visualizations in React that require D3.js functionality (like brushing/zooming), using a dedicated bridging library like visx (visx/brush) is preferable to manual DOM manipulation. It eliminates circular update issues and complex state management by providing React components that abstract D3's imperative approach.
Applied to files:
package.jsonsrc/react/components/AxisComponent.tsx
📚 Learning: 2025-04-23T09:25:51.968Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:58-67
Timestamp: 2025-04-23T09:25:51.968Z
Learning: When implementing deck.gl components in React, axis scales can appear to be a frame behind due to synchronization issues between deck.gl's render cycle and React's component updates.
Applied to files:
src/react/components/AxisComponent.tsx
📚 Learning: 2025-05-22T11:21:43.866Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 202
File: src/charts/TableChart.js:18-19
Timestamp: 2025-05-22T11:21:43.866Z
Learning: In the TableChart component, the condition for including the index column was changed from `config.include_index !== false` to `config?.include_index`, making it opt-in rather than opt-out. This means the index column is only included when explicitly enabled rather than being included by default.
Applied to files:
src/react/components/AxisComponent.tsx
📚 Learning: 2025-03-17T15:08:26.492Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The project uses MobX for state management in chart configurations, with `mobxAutorun` and reaction disposers to manage reactive updates based on changes to linked data sources.
Applied to files:
src/react/components/AxisComponent.tsx
📚 Learning: 2025-06-04T10:03:08.161Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:22-22
Timestamp: 2025-06-04T10:03:08.161Z
Learning: In the MDV project, React hooks provide a more natural and simpler pattern for MobX integration compared to class-based charts, as hooks automatically handle lifecycle management and cleanup in a way that aligns better with reactive programming patterns.
Applied to files:
src/react/components/AxisComponent.tsx
📚 Learning: 2025-04-23T08:48:20.916Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/DeckScatterReactWrapper.tsx:42-44
Timestamp: 2025-04-23T08:48:20.916Z
Learning: In DeckScatterReactWrapper.tsx, properties like maxZoom and minZoom in viewState are currently treated as constants rather than user-configurable settings, and are not preserved during serialization in getConfig().
Applied to files:
src/react/components/AxisComponent.tsx
📚 Learning: 2025-04-23T15:23:31.175Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/contour_state.ts:173-175
Timestamp: 2025-04-23T15:23:31.175Z
Learning: The `useContour` hook in `src/react/contour_state.ts` doesn't handle undefined parameters gracefully, which can cause issues when used with conditional returns in parent hooks.
Applied to files:
src/react/components/AxisComponent.tsx
📚 Learning: 2025-06-04T10:19:55.042Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:0-0
Timestamp: 2025-06-04T10:19:55.042Z
Learning: In the MDV codebase, BaseChart.ts provides automatic disposer management through the `mobxAutorun()` method which tracks disposers in a `reactionDisposers` array and automatically disposes them in the `remove()` method. When using direct `reaction()` calls instead of `mobxAutorun()`, manual disposer management is required.
Applied to files:
src/react/components/AxisComponent.tsx
📚 Learning: 2025-06-04T09:41:18.914Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:17-17
Timestamp: 2025-06-04T09:41:18.914Z
Learning: In the MDV project, BaseChart.mobxAutorun automatically stores the disposer and runs it when the chart is destroyed, eliminating the need for manual cleanup of MobX autorun reactions in derived chart classes.
Applied to files:
src/react/components/AxisComponent.tsx
🔇 Additional comments (3)
src/modules/wdyr.ts (1)
5-22: Appropriate approach for dev-only tool.Disabling Why Did You Render due to React 19 incompatibility is reasonable since it's a development-only debugging tool that doesn't affect production functionality.
package.json (2)
95-96: Critical: visx removal breaks axis rendering with no alternative provided.The upgrade to React 19 requires removing visx, but this breaks core visualization functionality in AxisComponent. According to project learnings, visx was the preferred solution for bridging React and D3.js for interactive visualizations. Based on learnings.
This PR leaves charts without functional axes and provides no migration path or alternative implementation.
Possible solutions:
- Wait for visx to release React 19 compatible version
- Implement custom D3-based axis rendering (would require careful handling of React/D3 integration patterns)
- Explore alternative React-compatible visualization libraries (recharts, nivo, victory)
Would you like me to:
- Search for React 19 compatible visualization libraries?
- Generate a custom D3 axis implementation following React best practices?
- Check visx's roadmap for React 19 support?
Also applies to: 135-136
⛔ Skipped due to learnings
Learnt from: xinaesthete Repo: Taylor-CCB-Group/MDV PR: 0 File: :0-0 Timestamp: 2025-04-09T21:07:38.683Z Learning: When building interactive data visualizations in React that require D3.js functionality (like brushing/zooming), using a dedicated bridging library like visx (visx/brush) is preferable to manual DOM manipulation. It eliminates circular update issues and complex state management by providing React components that abstract D3's imperative approach.Learnt from: xinaesthete Repo: Taylor-CCB-Group/MDV PR: 162 File: src/react/components/AxisComponent.tsx:58-67 Timestamp: 2025-04-23T09:25:51.968Z Learning: When implementing deck.gl components in React, axis scales can appear to be a frame behind due to synchronization issues between deck.gl's render cycle and React's component updates.Learnt from: xinaesthete Repo: Taylor-CCB-Group/MDV PR: 0 File: :0-0 Timestamp: 2025-04-03T10:26:40.958Z Learning: In the MDV repository, there's a cross-browser issue where react-based viv charts don't have proper mouse events attached when combining popout and fullscreen functionality, even though they work correctly with either popout or fullscreen individually.Learnt from: xinaesthete Repo: Taylor-CCB-Group/MDV PR: 0 File: :0-0 Timestamp: 2025-04-09T20:29:56.245Z Learning: When working with D3.js in React components, it's appropriate to intentionally exclude certain dependencies from useEffect dependency arrays to prevent unnecessary recreation of D3 elements. This approach should be documented with explanatory comments and ESLint suppression comments like `// eslint-disable-next-line react-hooks/exhaustive-deps` to clarify the intentional exclusion.Learnt from: xinaesthete Repo: Taylor-CCB-Group/MDV PR: 0 File: :0-0 Timestamp: 2025-04-03T10:26:40.958Z Learning: In the MDV repository, there's a Safari-specific bug when popping out vanilla-js charts to a separate window and then making them fullscreen. The axes often only occupy the width of the popped-out window, while GL content is sometimes clipped to that width.
87-87: Lucide-react may lack official React 19 support; verify compatibility in your environment.Current versions: lucide-react ^0.552.0, mobx ^6.13.7, mobx-react-lite ^4.1.1
The web search indicates lucide-react has no released version officially declaring React 19 support, though maintainers are tracking it. The mobx (6.13.7) and mobx-react-lite (4.1.1) versions align with the latest stable releases compatible with React 19.
Test lucide-react components thoroughly in your React 19 setup to ensure they function as expected before merging.
| // import * as Axis from "@visx/axis"; | ||
| // import * as Scale from "@visx/scale"; | ||
|
|
||
| //! This component is broken as the visx dependency is uninstalled due to incompatibility with React 19 version |
There was a problem hiding this comment.
Track the broken component with a GitHub issue.
The comment indicates this component is broken, but there's no tracking issue linked. This critical functionality loss should be tracked and prioritized.
Would you like me to create a GitHub issue to track the restoration of axis rendering functionality?
🤖 Prompt for AI Agents
In src/react/components/AxisComponent.tsx around line 9 the file contains a note
that the component is broken because the visx dependency was removed for React
19; create a GitHub issue in the repo to track restoring axis rendering (title,
description with reproduction steps, affected versions, and suggested options:
reintroduce compatible visx, find alternative lib, or implement custom axis),
then add a one-line TODO comment in this file referencing the new issue number
(e.g. "TODO: track restore of AxisComponent - GH#123") and update the PR
description to link the new issue so the loss of functionality is triaged and
prioritized.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue to track AxisComponent restoration
gh issue create \
--repo Taylor-CCB-Group/MDV \
--title "AxisComponent broken after visx removal for React 19 compatibility" \
--body "## Problem
The \`AxisComponent\` in \`src/react/components/AxisComponent.tsx\` is currently non-functional after the visx dependency was removed during the React 19 upgrade (PR #278).
## Context
- **Affected PR**: #278
- **Cause**: visx library is incompatible with React 19
- **Impact**: Axis rendering (x-axis and y-axis) is completely disabled in scatter plots
## Current State
All axis rendering functionality is commented out:
- \`@visx/axis\` imports are commented
- \`@visx/scale\` imports are commented
- Scale creation (\`scaleX\`, \`scaleY\`) is disabled
- Axis components (\`AxisBottom\`, \`AxisLeft\`) are not rendered
- A warning is logged: \"This is a broken chart, visx dependency has been removed\"
## Reproduction
1. Open any 2D scatter plot in the application
2. Observe that axis labels, tick marks, and scales are not rendered
3. Check browser console for the warning message
## Suggested Solutions
1. **Wait for visx React 19 compatibility**: Monitor visx repository for React 19 support and reintroduce when available
2. **Alternative library**: Evaluate other axis/scale libraries compatible with React 19 (e.g., recharts, victory, custom D3.js wrapper)
3. **Custom implementation**: Implement custom axis rendering using SVG elements and D3.js scales directly
## Files Affected
- \`src/react/components/AxisComponent.tsx\`
## Related
- PR: https://github.com/Taylor-CCB-Group/MDV/pull/278
- Comment: https://github.com/Taylor-CCB-Group/MDV/pull/278#discussion_r2499948979" \
--assignee nawabfurquan
Length of output: 50
@nawabfurquan, I've created GitHub issue #280 to track the broken AxisComponent functionality:
🔗 #280
The issue includes:
- Detailed description of the problem (axis rendering disabled due to visx removal)
- Context about React 19 incompatibility
- Reproduction steps
- Three suggested solutions:
- Wait for visx React 19 compatibility
- Evaluate alternative libraries (recharts, victory, custom D3.js wrapper)
- Implement custom axis rendering
- Links back to PR Upgrade frontend deps #278 and this comment thread
The issue has been assigned to you for tracking and prioritization.
/)/)
(^.^)
/>📋 Issue created!
🧬 Code Graph Analysis Results
[src/react/scatter_state.ts] (lines 77-85)
export type ScatterPlotConfig2D = ScatterPlotConfig & {
dimension: "2d";
axis: AxisConfig2D;
viewState: OrthographicViewState;
};
[src/react/scatter_state.ts] (lines 82-85)
export type ScatterPlotConfig3D = ScatterPlotConfig & {
dimension: "3d";
viewState: OrbitViewState;
};
[src/react/hooks.ts] (lines 100-126)
export function useParamColumns(): LoadedDataColumn<DataType>[] {
const chart = useChart();
const { columnIndex } = chart.dataStore;
const columns = useMemo(() => {
const param = chart.config.param;
if (!param) return [];
if (typeof param === "string")
return [columnIndex[param]];
// we could make sure they are loaded as well...
if (!isArray(param)) {
//must be a query object...
//!no - in that case, it should be a one-element array with a query object
// return param.columns;
throw new Error("config.param should always be an array");
}
// const param = chart.config.param as FieldName[]; // up for review with query objects etc.
//@ts-expect-error non-string 'name' as index; if we had 'concrete fieldName' version of config.param?
return param.map((name) => columnIndex[name]);
}, [chart.config.param, columnIndex]) as DataColumn<DataType>[];
// note that columns is 'any' here as of this writing
// - so this isn't an exhaustive check and ts will have limited capacity to help us.
// but we should be fairly safe to assume that once we get past here, we have `LoadedDataColumn`s
if (!allColumnsLoaded(columns)) {
throw new Error("we always expect that param columns are loaded by the time we try to use them... this shouldn't happen");
}
return columns;
}
[src/react/hooks.ts] (lines 36-50)
export function useChartSize() {
const chart = useChart();
// return chart.config.size; // not so well behaved?
const div = chart.contentDiv;
const [size, setSize] = useState([div.clientWidth, div.clientHeight]);
useLayoutEffect(() => {
const resize = () => {
setSize([div.clientWidth, div.clientHeight]);
};
const observer = new ResizeObserver(resize);
observer.observe(div);
return () => observer.unobserve(div);
}, [div]);
return size;
}
[src/charts/charts.d.ts] (lines 68-116)
export type DataColumn<T extends DataType> = {
/** human-readable column name, to be displayed in GUI etc */
name: ColumnName; //nb - we should check use of 'name' vs 'field'
/** id of the column, used internally */
field: FieldName;
/** the datatype- can be one of
* - `"double"` - any floating point data
* - `"integer"` - any integer data
* - `"text"` - data containing strings but with no more than 256 categories
* - `"text16"` - data containing strings with up to 65536 categories
* - `"unique"` - data containing strings but with many categories
* - `"multitext"` -
*/
datatype: T;
/** whether the column's data can be changed */
editable?: boolean;
/**In the case of a double/integer (number) column, the array
* buffer should be the appropriate size to contain float32s. For text it should be Uint8
* and contain numbers corresponding to the indexes in the values parameter. For a column of
* type unique it should be a JavaScript array. This parameter is optional as the data can
* be added later see {@link DataStore#setColumnData}.
* A {@link LoadedDataColumn<T>} can be used to represent a column that is known to have loaded data.
*/
data?: DataStructureTypes[T];
values: T extends CategoricalDataType ? string[] : never; //probably wrong for 'unique'
/** An array of rgb hex colors. In the case of a `"text"` column the `colors` should match the `values`. For number columns, the list represents
* colors that will be interpolated. If not specified, default color pallettes will be supplied.
*/
colors?: Colors;
/** if `true` then the colors will be displayed on a log scale- useful if the dataset contains outliers.
* Because a symlog scale is used the data can contain 0 and negative values */
colorLogScale?: boolean;
/** the column's values will be displayed as links (text and unique columns only).
* not sure if this is strictly boolean or can be undefined */
is_url?: T extends CategoricalDataType ? boolean : never;
/** the min max values in the column's values (integer/double only) */
minMax: T extends NumberDataType ? [number, number] : never;
/** an object describing the 0.05,0.01 and 0,001 qunatile ranges (integer/double only) */
quantiles: T extends NumberDataType ? Quantiles : never;
/** if `true` then the store will keep a record that this column has been added and is not permanently stored in the backend */
dirty?: boolean;
/** return the value corresponding to a given row index `i`. If the data is categorical, this will be the appropriate value from `values` */
getValue: (i: number) => T extends CategoricalDataType ? string : number;
stringLength: T extends "unique" ? number : never;
delimiter?: T extends "multitext" ? string : never;
subgroup?: SubgroupName; //not attempting to descriminate the other sg properties being related to this for now
sgindex?: number;
sgtype?: "dense" | "sparse"; //?? any other options?
};
🧠 Learnings used
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:58-67
Timestamp: 2025-04-23T09:25:51.968Z
Learning: When implementing deck.gl components in React, axis scales can appear to be a frame behind due to synchronization issues between deck.gl's render cycle and React's component updates.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:46-46
Timestamp: 2025-04-23T09:26:52.252Z
Learning: In React dependency arrays, conditional expressions (e.g., `is2d && config.axis.x.size`) can be necessary for type safety when some properties might not exist on all possible types. In these cases, extracting safe values before useMemo or adding a lint exception with explanation is preferred.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The project uses MobX for state management in chart configurations, with `mobxAutorun` and reaction disposers to manage reactive updates based on changes to linked data sources.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T20:29:56.245Z
Learning: When working with D3.js in React components, it's appropriate to intentionally exclude certain dependencies from useEffect dependency arrays to prevent unnecessary recreation of D3 elements. This approach should be documented with explanatory comments and ESLint suppression comments like `// eslint-disable-next-line react-hooks/exhaustive-deps` to clarify the intentional exclusion.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:22-22
Timestamp: 2025-06-04T10:03:08.161Z
Learning: In the MDV project, React hooks provide a more natural and simpler pattern for MobX integration compared to class-based charts, as hooks automatically handle lifecycle management and cleanup in a way that aligns better with reactive programming patterns.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T14:32:53.708Z
Learning: When implementing React components, the MobX `observer()` wrapper is only necessary for components that directly access MobX observables or stores. Components that exclusively use props and local React state (useState, useRef, etc.) don't need to be wrapped with observer() as they will re-render appropriately when props change through normal React mechanisms.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T14:32:53.708Z
Learning: In React components that don't directly access MobX observables or stores, the `observer()` wrapper is unnecessary. Components that only receive data via props and use standard React hooks for internal state don't need to be MobX observers, as they will re-render appropriately when props change.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T21:07:38.683Z
Learning: When building interactive data visualizations in React that require D3.js functionality (like brushing/zooming), using a dedicated bridging library like visx (visx/brush) is preferable to manual DOM manipulation. It eliminates circular update issues and complex state management by providing React components that abstract D3's imperative approach.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:0-0
Timestamp: 2025-06-04T10:19:55.042Z
Learning: In the MDV codebase, BaseChart.ts provides automatic disposer management through the `mobxAutorun()` method which tracks disposers in a `reactionDisposers` array and automatically disposes them in the `remove()` method. When using direct `reaction()` calls instead of `mobxAutorun()`, manual disposer management is required.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 115
File: src/react/components/ColumnSelectionComponent.tsx:165-165
Timestamp: 2025-03-03T15:53:04.505Z
Learning: In complex MobX-React components like those in the MDV codebase, creating intermediate objects (like `props2 = ...`) without making them observable can lead to read-only props in child components. This can be intentional to control the flow of state updates and avoid circular reactions.
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 202
File: src/charts/TableChart.js:18-19
Timestamp: 2025-05-22T11:21:43.866Z
Learning: In the TableChart component, the condition for including the index column was changed from `config.include_index !== false` to `config?.include_index`, making it opt-in rather than opt-out. This means the index column is only included when explicitly enabled rather than being included by default.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| useEffect(() => { | ||
| console.warn("This is a broken chart, visx dependency has been removed"); | ||
| }, []); |
There was a problem hiding this comment.
Display visible warning to users, not just console.
A console warning is insufficient for a broken chart. Users won't see it, leading to confusion about missing axes. Render a visible overlay or error message in the UI.
Apply this diff to show a user-visible warning:
} as const), [chartWidth, chartHeight, margin.top, margin.left]);
- // useEffect(() => {
- // if (is2d && (config.axis.x.rotate_labels || config.axis.y.rotate_labels)) {
- // console.warn("Axis rotation not implemented for react charts");
- // }
- // }, [is2d]);
- useEffect(() => {
- console.warn("This is a broken chart, visx dependency has been removed");
- }, []);
+
return (
<>
+ {is2d && (
+ <div style={{
+ position: "absolute",
+ top: 0,
+ left: 0,
+ right: 0,
+ padding: "8px",
+ backgroundColor: "rgba(255, 165, 0, 0.9)",
+ color: "black",
+ fontSize: "12px",
+ zIndex: 1000,
+ textAlign: "center"
+ }}>
+ ⚠️ Axes temporarily disabled due to React 19 upgrade (visx incompatibility)
+ </div>
+ )}
<div style={deckStyle}>{children}</div>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/react/components/AxisComponent.tsx around lines 115 to 117, replace the
console.warn-only effect with rendering a visible user-facing warning: add
component state (e.g., hasBrokenChartWarning) or a prop-controlled flag and
render an overlay/banner inside the component DOM when true that clearly informs
users the chart axes are unavailable and the visx dependency was removed; ensure
the warning is styled to be prominent, accessible (role="alert"), and
dismissible if appropriate, and remove or keep the console.warn for developer
logs as secondary output.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
At some point we accidentally indirectly installed another version of We should only have one version, which should currently be 9.11.9. It appears that this change happened with 00ffb68 when we installed React 19, not sure why yet. |
|
@coderabbitai can you see why 00ffb68 caused another version of |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1529 🏁 Script executed: Length of output: 2733 🏁 Script executed: Length of output: 871 🏁 Script executed: Length of output: 348 🏁 Script executed: Length of output: 630 🏁 Script executed: Length of output: 610 🏁 Script executed: Length of output: 1396 🏁 Script executed: Length of output: 858 @xinaesthete, I've investigated the issue. Here's what's happening: Root CauseWhen React 19 was installed, npm v7+'s automatic peer dependency resolution re-evaluated the entire dependency tree, and this caused a change in how dependencies were resolved. Before React 19 (commit 00ffb68^):
After React 19 (commit 00ffb68):
Why This HappenedThe issue stems from version range conflicts:
When React was upgraded, npm's resolution algorithm re-evaluated peer dependencies throughout the tree. Since npm favors updates by default, even when doing so causes duplication, and the peer dependency chain with React 19 triggered this behavior. SolutionUse npm's {
"overrides": {
"@luma.gl/core": "9.1.9"
}
}Add this to your rm -rf node_modules package-lock.json
npm installThis will force all packages to use the same version of Would you like me to create a script to verify this fix resolves the duplicate installation issue? |
|
@coderabbitai lots of other issues reported by I also have another pnpm based repo with similar dependencies -including react-19, but not |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1534 🏁 Script executed: Length of output: 784 @xinaesthete, I've analyzed the issue. The The ProblemWhen React 19 was installed, npm's resolution algorithm decided to install deck.gl 9.2.x packages under
Why pnpm Works Betterpnpm uses a different peer dependency resolution strategy:
This is why your pnpm repo doesn't have this issue. SolutionsOption 1: Upgrade deck.gl to 9.2.2 (Recommended)This aligns your top-level version with what npm wants to install: {
"dependencies": {
"deck.gl": "~9.2.2"
}
}Then: rm -rf node_modules package-lock.json
npm installBoth Option 2: Switch to pnpmSince you've verified it works in another repo: npm install -g pnpm
rm -rf node_modules package-lock.json
pnpm installUpdate your scripts to use pnpm instead of npm. Option 3: Accept the DuplicationIf the application runs without issues, you could keep the duplicate versions. Multiple Which approach would you prefer? I'd recommend upgrading to deck.gl 9.2.2 as the cleanest solution. |
|
@coderabbitai I will update the deck.gl version, but this is a much more involved process of making a PR for viv and publishing a new version. I intend to do that soon anyway, but it is out-of-scope here. There may be other benefits of switching to pnpm anyway as it seems generally better at handling monorepos, and it may be a good idea for us to move more towards that kind of structure in future. |
|
Tip For best results, initiate chat on the files or code changes. @xinaesthete, that sounds like a solid plan! Upgrading deck.gl when it involves coordinating with viv definitely makes sense as a separate effort. For this PR, the duplicate Regarding the pnpm migration - you're right that it handles monorepos much better. A few considerations when you decide to make that switch:
Good luck with the viv PR! Let me know if you need any help when you tackle the deck.gl upgrade. |
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@xinaesthete, you're absolutely right - I apologize for the incorrect suggestion. If luma.gl throws runtime errors on version conflicts, this must be resolved. Let me investigate solutions that work within your constraints (no deck.gl upgrade yet, no pnpm migration yet): 🏁 Script executed: Length of output: 1436 @xinaesthete, now I understand the severity. Here's the solution using comprehensive npm Solution: Comprehensive npm OverridesAdd this {
"overrides": {
"@deck.gl/core": "9.1.11",
"@deck.gl/layers": "9.1.11",
"@deck.gl/geo-layers": "9.1.11",
"@deck.gl/react": "9.1.11",
"@deck.gl/widgets": "9.1.11",
"@luma.gl/core": "9.1.9",
"@luma.gl/engine": "9.1.9",
"@luma.gl/shadertools": "9.1.9",
"@luma.gl/webgl": "9.1.9",
"@luma.gl/constants": "9.1.9"
}
}Then run: rm -rf node_modules package-lock.json
npm installWhat This DoesThis forces npm to use only the 9.1.x versions throughout the entire dependency tree, preventing the 9.2.x versions from being installed under VerificationAfter running the install, verify with: npm why @luma.gl/coreYou should see only the 9.1.9 version listed. This is a temporary solution until you can upgrade deck.gl to 9.2.x in the separate viv PR. The overrides can be removed once deck.gl is upgraded. |
|
@coderabbitai with the more extensive As for whether we move to |
|
Tip For best results, initiate chat on the files or code changes. @xinaesthete, excellent progress! The override solution is working as intended. Here's what's happening: The Warnings Are Expected and SafeYou can ignore these warnings as npm is overriding the peer dependency. The "masses of warnings" you're seeing are npm informing you that it's forcing version resolutions that don't match what some packages declared in their peer dependencies. This is exactly what the Since you've verified that:
The approach is working correctly. The warnings won't cause runtime issues - they're just npm's way of saying "heads up, we're using version X even though package Y requested version Z." Next StepsCritical: Test the build and runtime behavior, as this is what matters. luma.gl will throw runtime errors if multiple versions exist, so if the application runs without luma.gl version conflicts, you're good to go. Run: npm run vite-build
# Then test the application runtimeRegarding pnpmYou're right that pnpm requires users to have it installed. A few options:
The override approach is a solid interim solution until you can upgrade deck.gl and remove the overrides entirely. Would you like me to help verify anything else about the setup, or shall we consider this issue resolved pending your build/runtime tests? |
|
@coderabbitai when I try applying the change to add those overrides on top of the latest commit here, I still end up with multiple luma versions. I can go back to the pre-react-19 revision, add the overrides and then install the updated react version. May be simpler to branch from there and apply changes, or maybe I'll get on with updating deck.gl and see how how smoothly that goes. |
|
Tip For best results, initiate chat on the files or code changes. @xinaesthete, you've identified a key issue - npm's dependency resolution is influenced by the existing lockfile state, and simply adding overrides after React 19 is already installed with the problematic nested structure may not fully re-resolve the tree. Quick Fix to TryBefore branching or upgrading deck.gl, try this complete reset procedure on the current branch: # Ensure overrides are in package.json (with React 19 still present)
# Then completely reset:
rm -rf node_modules package-lock.json
npm cache clean --force
npm installThe If That Doesn't WorkYou have two solid options: Option 1: Rebase approach
Option 2: Upgrade deck.gl now (Recommended if feasible)
Since you mentioned you were planning to upgrade deck.gl soon anyway, it might be worth just doing that now rather than fighting with npm's resolution algorithm. The override approach is a workaround - upgrading is the actual fix. Which path would you prefer? I'm happy to help with either approach - debugging the override application, helping with the rebase, or assisting with the deck.gl upgrade preparation. |
2 participants
wdyrandvisxdue to incompatibility with React 19mobx,mobx-react-liteandlucide-reactdependenciesSummary by CodeRabbit