fix(export): PNG/SVG export leaks + wire Export PNG action (#872) - #873
Merged
Conversation
- Fix dispose() leak in exportChartToSvg: declare svgInstance outside try, move dispose() into finally with null-check so it runs on every throw path - Round SVG dimensions before passing to echarts.init (avoids fractional width/height that some SVG viewers handle inconsistently) - Add exportChartToPng helper using getDataURL with pixelRatio 2 and theme- matched background (#0a0f1e dark / #ffffff light) - Wire Export PNG widget action alongside existing Export SVG, gated by capabilities.isECharts - 8 unit tests covering both helpers: success / no-instance / setOption- throws / no-svg / dimension rounding / PNG light + dark backgrounds - E2E test: ECharts widget exposes both Export PNG and Export SVG actions and PNG click triggers a .png download - Refresh package-lock.json to match release/1.0 versions (1.0.0) Closes #872 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alfredo1996
added
bug
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds PNG export support to ECharts widgets by fixing an SVG instance disposal leak, implementing a new PNG export function that reads from the existing canvas renderer, wiring the PNG action into the widget menu, and validating both export paths with unit and E2E tests. ChangesChart PNG Export Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/e2e/chart-export.spec.ts (1)
12-13: ⚡ Quick winAssert dashboard API response success before parsing JSON.
Add an
expect(res.ok()).toBeTruthy()check beforeres.json()to fail fast with a clearer cause when the request is unauthorized or fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/e2e/chart-export.spec.ts` around lines 12 - 13, Before parsing the dashboard response, assert the HTTP success on the Response object returned by page.request.get("/api/dashboards"); add an expectation like expect(res.ok()).toBeTruthy() immediately after obtaining res so the test fails fast with a clear authorization/error message before calling res.json() and assigning to dashboards.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/e2e/chart-export.spec.ts`:
- Around line 12-13: Before parsing the dashboard response, assert the HTTP
success on the Response object returned by page.request.get("/api/dashboards");
add an expectation like expect(res.ok()).toBeTruthy() immediately after
obtaining res so the test fails fast with a clear authorization/error message
before calling res.json() and assigning to dashboards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6d91fbb-d99e-465b-89b2-a4898b5e8aa4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
app/e2e/chart-export.spec.tsapp/src/components/dashboard-container.tsxcomponent/src/charts/__tests__/chart-export.test.tscomponent/src/charts/base-chart.tsxcomponent/src/charts/index.ts
…7778) SonarCloud flagged sequential actions.push() calls in dashboard-container.tsx as a MINOR maintainability issue (Array#push called multiple times). Combine the PNG + SVG action pushes into a single call with both args. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
7 tasks
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
2 participants
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.
Summary
dispose()leak inexportChartToSvg—svgInstancedeclared outsidetry,dispose()moved intofinallywith null-check so it runs on every throw path (HIGH)getDataURL({ type: "png", pixelRatio: 2, backgroundColor })with#0a0f1e(dark) /#ffffff(light) to match the on-screen theme (HIGH)Math.roundbefore passing toecharts.init(MEDIUM)exportChartToPnghelper incomponent/src/charts/base-chart.tsx, exported via@neoboard/components.pngdownloadDefers LOW items (font-embed tooltip, generic
triggerDownloadmime tightening) — out of scope per drill.Why
exportChartToSvgleaked the offscreen ECharts instance (canvas, ResizeObserver, animation frames) wheneversetOptionorserializeToStringthrew — thefinallyonly removed the offscreen DOM node. Repeated failed exports slowly leaked memory.PNG is the format users reach for first (paste into doc/email/Slack).
triggerPngDownloadwas exported but no UI wired it up. Now it does.Test plan
component/src/charts/__tests__/chart-export.test.tsapp/e2e/chart-export.spec.tspassesCloses #872
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Summary by CodeRabbit
Release Notes
New Features
Tests