Open
Conversation
fix: update ChartTypeSelect prop from dropdownMatchSelectWidth to popupMatchSelectWidth fix: add type annotation for onRow function in SeriesSettings refactor: remove TypeScript error suppression in ColumnsSettings fix: remove TypeScript error suppression in Renderer chore: update webpack config for React 19 compatibility with react-dom
There was a problem hiding this comment.
Pull request overview
This PR is a WIP refactor to move the frontend codebase to React v19 and Ant Design v5, including the related ecosystem changes (ReactDOM root API, antd prop/classname changes) and test framework updates.
Changes:
- Update ReactDOM usage to
createRootand add areact-domcompatibility shim for dependencies. - Migrate many components away from
defaultPropsto parameter defaults (React 19-friendly) and update AntD v5 API usages. - Replace Enzyme-based unit tests with React Testing Library.
Reviewed changes
Copilot reviewed 215 out of 220 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.config.js | Adds react-dom aliasing to support React 19 expectations from dependencies. |
| client/app/lib/react-dom-compat.js | Implements a compat shim that re-exports createRoot/hydrateRoot. |
| client/app/index.js | Switches app bootstrap from ReactDOM.render to createRoot().render. |
| viz-lib/package.json | Moves test deps from Enzyme to Testing Library; updates peer deps to React/AntD 5+. |
| viz-lib/tests/enzyme_setup.js + client/app/tests/enzyme_setup.js | Removes Enzyme setup (test migration). |
| client/app/pages/dashboards/hooks/useEditModeHandler.js | Refactors debounced dashboard layout saving logic. |
| client/app/pages/dashboards/DashboardPage.jsx | Migrates defaults to parameter defaults (but introduces a bad default). |
| client/app/components/queries/QueryEditor/QueryEditorControls.jsx | Migrates defaults to parameter defaults (but introduces a runtime crash). |
| viz-lib/src/visualizations/shared/components/ColumnEditor.tsx | Removes TS suppression and adjusts props; label regression risk on Description field. |
Comments suppressed due to low confidence (1)
client/app/components/queries/QueryEditor/QueryEditorControls.jsx:44
- The
filterpredicate assumes each button-props entry is an object (b.shortcut), but defaults arefalse. If any of these props are omitted/false, this will throw at runtime (Cannot read properties of false). Guard the predicate (e.g.,b && b.shortcut && ...) or normalize tonull/undefinedbefore filtering.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 220 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/app/components/EditVisualizationButton/index.jsx">
<violation number="1" location="client/app/components/EditVisualizationButton/index.jsx:7">
P2: This defaulting pattern changes behavior: `selectedTab` no longer defaults for explicit `undefined` values. Preserve the old semantics when migrating off `defaultProps`.</violation>
</file>
<file name="client/app/components/QuerySelector.jsx">
<violation number="1" location="client/app/components/QuerySelector.jsx:118">
P2: `popupMatchSelectWidth` is a v5 Select prop; with `antd@4.4.3` this setting won’t apply. Use `dropdownMatchSelectWidth` to preserve the intended dropdown width behavior.</violation>
</file>
<file name="client/app/components/ApplicationArea/Router.jsx">
<violation number="1" location="client/app/components/ApplicationArea/Router.jsx:39">
P1: Defaulting `routes` to `[]` in the parameter creates a new array each render, which makes the `[routes]` effect run repeatedly when the prop is omitted.</violation>
</file>
<file name="client/app/components/EditVisualizationButton/QueryResultsLink.jsx">
<violation number="1" location="client/app/components/EditVisualizationButton/QueryResultsLink.jsx:6">
P2: This defaulting pattern changes behavior: explicit `undefined` props now override defaults. Preserve defaults for undefined values (as `defaultProps` did) to avoid broken href values.</violation>
</file>
<file name="client/app/components/visualizations/VisualizationRenderer.jsx">
<violation number="1" location="client/app/components/visualizations/VisualizationRenderer.jsx:42">
P2: Using `filters: []` in the in-function default object creates a new array every render, which makes `props.filters` unstable and retriggers effects on every render when the prop is omitted.</violation>
</file>
<file name="client/app/components/queries/QueryEditor/index.jsx">
<violation number="1" location="client/app/components/queries/QueryEditor/index.jsx:16">
P2: Avoid inline array/function defaults in props destructuring here; they create new references every render and invalidate hook dependencies unnecessarily.</violation>
</file>
<file name="client/app/components/dynamic-parameters/DateRangeParameter.jsx">
<violation number="1" location="client/app/components/dynamic-parameters/DateRangeParameter.jsx:112">
P2: This spread-based fallback changes semantics: explicit `undefined` now overrides defaults, unlike the previous `defaultProps` behavior. Use destructuring defaults (or explicit `=== undefined` checks) to preserve prior behavior.</violation>
</file>
<file name="client/app/components/dashboards/DashboardGrid.jsx">
<violation number="1" location="client/app/components/dashboards/DashboardGrid.jsx:172">
P2: Merging with `existingByKey[item.i] || item` discards updated layout fields from props for existing widgets; preserve only runtime position/size from existing items and keep fresh constraints from `normalizeFrom`.</violation>
</file>
<file name="client/app/components/dynamic-parameters/DateParameter.jsx">
<violation number="1" location="client/app/components/dynamic-parameters/DateParameter.jsx:26">
P2: This spread-based fallback changes `defaultProps` semantics: explicitly `undefined` props now override defaults, which can reintroduce `undefined` values (e.g., `onSelect`) and break downstream assumptions.</violation>
</file>
<file name="client/app/components/Tooltip.tsx">
<violation number="1" location="client/app/components/Tooltip.tsx:8">
P2: Unsafe type assertion hides `title`'s function variant; handle function titles explicitly instead of casting to `ReactNode`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
client/app/components/EditVisualizationButton/QueryResultsLink.jsx
Outdated
Show resolved
Hide resolved
client/app/components/dynamic-parameters/DateRangeParameter.jsx
Outdated
Show resolved
Hide resolved
…e useEditModeHandler with improved layout saving logic and tests
Contributor
There was a problem hiding this comment.
4 issues found across 387 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/app/components/empty-state/EmptyState.jsx">
<violation number="1" location="client/app/components/empty-state/EmptyState.jsx:15">
P3: `urlTarget` is introduced in the `Step` parameters but never used, adding dead code in this change.</violation>
</file>
<file name="client/app/components/HelpTrigger.jsx">
<violation number="1" location="client/app/components/HelpTrigger.jsx:272">
P2: The new spread-based defaults do not preserve `defaultProps` semantics: `undefined` values in incoming props override defaults. This can drop defaults like `children` icon and `showTooltip` when props are spread from partially-defined objects.</violation>
</file>
<file name="client/app/components/dynamic-parameters/DateParameter.jsx">
<violation number="1" location="client/app/components/dynamic-parameters/DateParameter.jsx:26">
P2: Spreading `props` after inline defaults changes `defaultProps` semantics: explicitly `undefined` values now override defaults.</violation>
</file>
<file name="client/app/components/ApplicationArea/Router.jsx">
<violation number="1" location="client/app/components/ApplicationArea/Router.jsx:37">
P1: Inline `routes = []` default creates a new array each render, which makes the `[routes]` effect run repeatedly and can cause route re-resolution/re-render loops when `routes` prop is omitted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…c.js`, `dashboard/grid_compliant_widgets_spec.js`
Contributor
There was a problem hiding this comment.
6 issues found across 44 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/cypress/integration/query/create_query_spec.js">
<violation number="1" location="client/cypress/integration/query/create_query_spec.js:23">
P1: This no longer selects the configured Cypress data source, so the query runs against whichever source is preselected or first in the dropdown.</violation>
</file>
<file name="client/cypress/integration/alert/view_alert_spec.js">
<violation number="1" location="client/cypress/integration/alert/view_alert_spec.js:64">
P2: This change stops testing the non-author permission behavior and instead accepts an error page, so a regression that blocks non-authors from viewing alerts would now pass.</violation>
</file>
<file name="client/cypress/support/dashboard/index.js">
<violation number="1" location="client/cypress/support/dashboard/index.js:44">
P2: Compute the drag coordinates after `scrollIntoView()`. Right now the resize events use stale positions if scrolling moves the handle.</violation>
</file>
<file name="client/cypress/integration/dashboard/widget_spec.js">
<violation number="1" location="client/cypress/integration/dashboard/widget_spec.js:112">
P2: The refreshed 5-row height assertion is inconsistent with the other 5-row expectations in this spec and is likely a wrong expected value.</violation>
</file>
<file name="client/cypress/support/parameters.js">
<violation number="1" location="client/cypress/support/parameters.js:70">
P2: `offsetLeft` magnitude is ignored, so horizontal drag distance no longer matches the function API and prior behavior.</violation>
</file>
<file name="client/app/lib/dateTimeUtils.js">
<violation number="1" location="client/app/lib/dateTimeUtils.js:15">
P2: Invalid moment instances should be normalized to `null` here; returning them unchanged makes `toMoment` and `toMomentRange` accept invalid dates as valid values.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ormat retrieval in parameter_spec
Contributor
There was a problem hiding this comment.
6 issues found across 46 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/cypress/support/commands.js">
<violation number="1" location="client/cypress/support/commands.js:154">
P2: Avoid calling `cy.wrap()` inside the returned Promise chain in this custom command; it can trigger Cypress promise/command-mixing errors.</violation>
<violation number="2" location="client/cypress/support/commands.js:160">
P3: Use nullish coalescing for `delay` so an explicit `0` is respected.</violation>
</file>
<file name="client/cypress/integration/dashboard/widget_spec.js">
<violation number="1" location="client/cypress/integration/dashboard/widget_spec.js:112">
P2: The dynamic auto-height assertion is inconsistent with the file’s 5-row baseline and likely expects the wrong height (535 instead of 435).</violation>
</file>
<file name="client/cypress/integration/query/create_query_spec.js">
<violation number="1" location="client/cypress/integration/query/create_query_spec.js:13">
P1: Datasource selection is now non-deterministic; selecting the first visible option can execute against the wrong datasource and make this test flaky.</violation>
</file>
<file name="client/cypress/support/dashboard/index.js">
<violation number="1" location="client/cypress/support/dashboard/index.js:46">
P1: Compute drag coordinates after `scrollIntoView()`. Using pre-scroll `getBoundingClientRect()` values can target stale coordinates and cause flaky resize interactions.</violation>
</file>
<file name="client/app/components/DateTimeInput.jsx">
<violation number="1" location="client/app/components/DateTimeInput.jsx:26">
P2: `showTime.defaultOpenValue` is being set with `moment()`, which is incompatible with Ant Design v5 DatePicker's default Day.js date type. This can cause picker state/value bugs at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…angeInput components
…tion - Upgraded @typescript-eslint/eslint-plugin and @typescript-eslint/parser from version 5.62.0 to 8.57.1 in package.json and pnpm-lock.yaml. - Disabled the rule "@typescript-eslint/no-empty-object-type" in .eslintrc.js to allow empty object types.
Member
Author
|
@arikfr Could you look please? This PR requires a more thorough visual review than the PR for Node 24, because updating the libraries necessitated changes to the business logic files. |
Member
|
@denisov-vlad do you think there are specific areas of functionality that require a visual review? |
Member
Author
|
quick look: sortable elements (i.e. params), dashboard layout, popup messages - these were the elements I had the most trouble with, but that’s why I’m even more confident that everything is fine here. I think we need to spend more time testing the visualizations. |
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="viz-lib/src/visualizations/pivot/PlotlyComponent.tsx">
<violation number="1" location="viz-lib/src/visualizations/pivot/PlotlyComponent.tsx:191">
P1: Resize listener cleanup is skipped on unmount when `useResizeHandler` is true, leaking a window event listener.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
Author
|
Found that antd v6 has native react v19 support. Will update it too. |
Contributor
There was a problem hiding this comment.
1 issue found across 40 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="package.json">
<violation number="1" location="package.json:48">
P1: Version specifier targets antd **v6**, but the PR description and prior team convention both specify Ant Design **v5** as the migration target. If v6 is intentional, update the PR description; otherwise revert to `^5.24.7`. A major-version jump carries breaking-change risk that should be explicitly called out.
(Based on your team's feedback about treating this repository as using Ant Design v5.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
2 issues found across 40 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="package.json">
<violation number="1" location="package.json:48">
P1: The PR description says this is an "Ant Design 5 migration", but the diff bumps `antd` to `^6.3.3` and `@ant-design/icons` to `^6.1.0` — both major version upgrades from the v5 that was already in the base branch. If the intent was to stay on v5, these should remain at their previous `^5.x` ranges. If v6 is intentional, the PR description should be updated and the breaking changes from antd v6 should be called out for reviewers.</violation>
</file>
<file name="client/app/components/ApplicationArea/ApplicationLayout/DesktopNavbar.jsx">
<violation number="1" location="client/app/components/ApplicationArea/ApplicationLayout/DesktopNavbar.jsx:83">
P2: The icon is now outside the `<Link>`, so clicking the icon won’t navigate. Wrap the icon in the link (and drop the `icon` prop) to keep the entire item clickable, matching the previous behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
client/app/components/ApplicationArea/ApplicationLayout/DesktopNavbar.jsx
Outdated
Show resolved
Hide resolved
…apping and improve session handling
…e resize handling
… and improve dateTimeUtils with validation for moment instances
Contributor
There was a problem hiding this comment.
4 issues found across 31 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/cypress/support/commands.js">
<violation number="1" location="client/cypress/support/commands.js:103">
P2: The `data-test` option lookup uses a non-retrying jQuery snapshot, which can make option selection flaky while the AntD dropdown is still rendering.</violation>
</file>
<file name="test-support/jest/jsdom-shims.js">
<violation number="1" location="test-support/jest/jsdom-shims.js:3">
P2: Use an explicit timezone in the default mocked date to keep test timestamps deterministic across environments.</violation>
</file>
<file name="client/cypress/support/visualizations/chart.js">
<violation number="1" location="client/cypress/support/visualizations/chart.js:15">
P2: Use exact label matching instead of substring matching when deciding whether to remap the chart column.</violation>
<violation number="2" location="client/cypress/support/visualizations/chart.js:23">
P2: Compute missing labels with exact token matching, not `.includes`, to avoid skipping required multi-select mappings.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…mprove chart label retrieval logic
… improved Ant Design interactions and SQL normalization
Member
Author
|
@arikfr All done. I've updated |
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
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.
What type of PR is this?
Description
This branch updates the frontend package manifests to complete the React 19 / Ant Design 6 migration and align the supporting toolchain.
package.json: upgradesreact/react-domto 19, movesantdand@ant-design/iconsto v6, refreshes related React packages (react-grid-layout,react-resizable,react-refresh,eslint-plugin-react-hooks, React type packages), replaces the old Enzyme-based test setup with Testing Library/JSDOM packages, and updates the TypeScript lint stack to@typescript-eslintv8 with pnpm overrides.viz-lib/package.json: aligns peer dependencies with Ant Design 6 and React 19, updates React type packages, replaces Enzyme test dependencies with Testing Library/JSDOM, adds the@dnd-kit/*packages for drag-and-drop behavior, removesreact-sortable-hoc, and drops the old Enzyme-specific Jest setup/serializer.How is this tested?
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)