Skip to content

[MINOR] chore(zeppelin-web-angular): remove @antv/g2plot and migrate to chart.js to fix npm audit#5257

Open
jongyoul wants to merge 1 commit into
apache:masterfrom
jongyoul:fix-npm-audit-issues
Open

[MINOR] chore(zeppelin-web-angular): remove @antv/g2plot and migrate to chart.js to fix npm audit#5257
jongyoul wants to merge 1 commit into
apache:masterfrom
jongyoul:fix-npm-audit-issues

Conversation

@jongyoul
Copy link
Copy Markdown
Member

@jongyoul jongyoul commented May 24, 2026

What is this PR for?

This PR completely removes the @antv/g2plot dependency and migrates the charting visualization to chart.js in order to resolve the npm-audit failures caused by the critical malware advisories for @antv/color-util and @antv/adjust (GHSA-rh6v-hwr4-6jcp / GHSA-qcp2-qp9h-qprg).

What changes are proposed?

  1. Remove @antv/g2plot: Uninstalled @antv/g2plot and its transitive dependencies to clean up the security alerts.
  2. Install chart.js: Installed chart.js (and upgraded webpack-dev-server to ^5.2.4 to remediate other moderate vulnerabilities).
  3. Migrate Visualization: Rewrote TableVisualization.tsx using chart.js to draw the 5 types of charts (Bar, Line, Pie, Scatter, Area) on a HTML5 Canvas context. The visualization layout and color scheme (#1890ff) are preserved.

How should this be tested?

Verify that the npm-audit job passes in GitHub Actions.

Copilot AI review requested due to automatic review settings May 24, 2026 06:40
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the zeppelin-react frontend audit flow to remediate current npm audit failures while allowing known @antv malware false-positive advisories to be filtered.

Changes:

  • Upgrades webpack-dev-server to 5.2.4.
  • Adds audit-filter.js to run and filter npm audit JSON output.
  • Updates the frontend CI workflow to use the custom audit filter.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.

File Description
zeppelin-web-angular/projects/zeppelin-react/package.json Updates the webpack-dev-server dev dependency range.
zeppelin-web-angular/projects/zeppelin-react/package-lock.json Locks webpack-dev-server to 5.2.4.
zeppelin-web-angular/projects/zeppelin-react/audit-filter.js Adds custom npm audit filtering for selected advisories.
.github/workflows/frontend.yml Runs the new audit filter in the zeppelin-react audit job.
Files not reviewed (1)
  • zeppelin-web-angular/projects/zeppelin-react/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread zeppelin-web-angular/projects/zeppelin-react/audit-filter.js Outdated
Comment thread zeppelin-web-angular/projects/zeppelin-react/audit-filter.js Outdated
Comment thread zeppelin-web-angular/projects/zeppelin-react/audit-filter.js Outdated
Comment thread zeppelin-web-angular/projects/zeppelin-react/audit-filter.js Outdated
@jongyoul jongyoul force-pushed the fix-npm-audit-issues branch from db5a970 to fd888bf Compare May 24, 2026 06:52
@jongyoul
Copy link
Copy Markdown
Member Author

@tbonelee @ParkGyeongTae @voidmatcha I heard that @AntV had a security issue, so it's been fully deprecated. So npm-audit failed, so I removed them and used the vanilla one. Could you please check it?

Copy link
Copy Markdown
Contributor

@voidmatcha voidmatcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • npm audit on master: 21 advisories (10 moderate / 5 high / 6 critical, incl. the @antv/adjust & @antv/color-util malware advisories)
  • Diff review: no new dangerouslySetInnerHTML / eval / dynamic Function introduced, and chart labels/data are drawn on a 2D canvas context. So the migration doesn't add any XSS surface.

LGTM from a supply-chain perspective.

labels: data.map(d => d.category),
datasets: [{
label: 'Value',
data: data.map(d => data.value),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
data: data.map(d => data.value),
data: data.map(d => d.value),

Original(master)

Image

As-Is

Image

To-Be

Image

The callback references the outer data array instead of the element d, so data.value is undefined for every row and the Area Chart renders empty.

let chart: Column | Line | Pie | Scatter | null = null;
chartRef.current.innerHTML = '';

let chart: any = null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: let chart: any and let chartConfig: any violate @typescript-eslint/no-explicit-any (set to error in this project)

npm run lint fails on both lines. Beyond the lint rule, the migration also loses the previous Column | Line | Pie | Scatter discrimination. Chart.js 4 ships full generics (ChartConfiguration<TType>, Chart<TType>), so we can satisfy the rule and tighten the original type safety in one go.

Concretely: drop the chartConfig: any intermediate variable and call new Chart<TType>(ctx, {...}) inline per case. The literal type: 'bar' narrows data.datasets[N] and options to the bar-specific shape, so typos like data.map(d => data.value) would be caught at compile time.

import type { Chart as ChartJS } from 'chart.js';

// ...

let chart: ChartJS | null = null;

import('chart.js/auto').then(module => {
  // ...
  switch (currentMode) {
    case 'multiBarChart':
      chart = new Chart<'bar'>(ctx, {
        type: 'bar',
        data: {
          labels: data.map(d => d.category),
          datasets: [{ label: 'Value', data: data.map(d => d.value), backgroundColor: '#1890ff' }]
        },
        options: { responsive: true, maintainAspectRatio: false }
      });
      break;
    case 'lineChart':
      chart = new Chart<'line'>(ctx, { /* ... */ });
      break;
    // pieChart -> Chart<'pie'>, scatterChart -> Chart<'scatter'>,
    // stackedAreaChart -> Chart<'line'>
  }
});

Same pattern for the remaining four cases. Tested locally: no runtime change, both any lint warnings go away, and the per-case dataset shape is now type-checked.

Comment on lines +200 to +202
if (chartRef.current) {
chartRef.current.innerHTML = '';
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: cleanup references chartRef.current, which violates react-hooks/exhaustive-deps (set to error in this project)

npm run lint fails on the cleanup line. In this specific component the ref is unlikely to actually diverge between effect runs (the chart <div> is identity-stable across chart-mode switches, and on switching to the table mode the ref simply goes to null, which the existing if (chartRef.current) guard handles), so this is not a runtime bug. But the project has the rule at error, so it still needs to be fixed.

The canonical fix is to capture the ref to a local const at effect start and use it everywhere (initial guard, the dynamic import().then() callback, and the cleanup return). This satisfies the rule without an eslint-disable and makes the lifecycle binding explicit.

useEffect(() => {
  const container = chartRef.current;
  if (!container || !tableData || tableData.rows.length === 0 || currentMode === 'table') return;

  container.innerHTML = '';

  let chart: ChartJS | null = null;
  let cancelled = false;

  import('chart.js/auto').then(module => {
    if (cancelled) return;
    // ... use `container.appendChild(canvas)` instead of `chartRef.current.appendChild(...)`
  });

  return () => {
    cancelled = true;
    if (chart) chart.destroy();
    container.innerHTML = '';
  };
}, [currentMode, tableData]);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I've captured the chartRef.current reference into a local container variable at the beginning of the effect and updated the effect and cleanup function to use it. This resolves the react-hooks/exhaustive-deps warning.

@jongyoul jongyoul force-pushed the fix-npm-audit-issues branch from fd888bf to 0509a0f Compare May 25, 2026 01:03
@jongyoul jongyoul changed the title chore(zeppelin-web-angular): remediate npm audit findings in zeppelin-react chore(zeppelin-web-angular): remove @antv/g2plot and migrate to chart.js to fix npm audit May 25, 2026
@jongyoul jongyoul closed this May 25, 2026
@jongyoul jongyoul reopened this May 25, 2026
@jongyoul jongyoul force-pushed the fix-npm-audit-issues branch from 0509a0f to 23f6f2a Compare May 25, 2026 02:47
@jongyoul jongyoul changed the title chore(zeppelin-web-angular): remove @antv/g2plot and migrate to chart.js to fix npm audit [MINOR] chore(zeppelin-web-angular): remove @antv/g2plot and migrate to chart.js to fix npm audit May 25, 2026
@tbonelee
Copy link
Copy Markdown
Contributor

@jongyoul Could you rebase this onto master?

@jongyoul jongyoul force-pushed the fix-npm-audit-issues branch from 23f6f2a to 917820b Compare May 25, 2026 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants