-
Notifications
You must be signed in to change notification settings - Fork 4
feat(charts): Style charts to match editor's color theme #76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(charts): Style charts to match editor's color theme #76
Conversation
GRN-4953 Dark mode support for chart block output
Currently we render charts on white background by default. Ideally, we'd want to detect whether current theme is dark or light and pick colors for chart appropriately. Maybe even use background color directly from the editor's theme to make output look seamless |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
=====================================
Coverage 71% 71%
=====================================
Files 527 527
Lines 39474 39474
Branches 4933 4933
=====================================
Hits 28277 28277
Misses 9563 9563
Partials 1634 1634 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR adds error boundary protection and dynamic theming to the Vega chart renderer. It installs Sequence Diagram(s)sequenceDiagram
participant App
participant ErrorBoundary
participant VegaRenderer
participant MutationObserver
participant VegaChart
App->>ErrorBoundary: Render with fallback
ErrorBoundary->>VegaRenderer: Mount
VegaRenderer->>MutationObserver: Initialize theme observer
VegaRenderer->>VegaRenderer: Get theme colors from CSS vars
VegaRenderer->>VegaRenderer: Apply colors to spec (Immer)
VegaRenderer->>VegaChart: Render with themedSpec
MutationObserver->>VegaRenderer: Detect theme change
VegaRenderer->>VegaRenderer: Recompute theme colors
VegaRenderer->>VegaChart: Re-render with updated spec
alt Rendering Error
VegaChart--xErrorBoundary: Error thrown
ErrorBoundary->>ErrorBoundary: onError callback
ErrorBoundary->>ErrorFallback: Render fallback UI
ErrorFallback->>App: Display error message & stack
end
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/webviews/webview-side/vega-renderer/index.ts (1)
15-44: Consider clarifying dual error handling strategy.The try/catch and ErrorBoundary handle different error phases:
- try/catch: JSON parsing and DOM setup errors (synchronous)
- ErrorBoundary: React render lifecycle errors (asynchronous)
A brief comment explaining this distinction would improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.eslintrc.js(1 hunks)package.json(1 hunks)src/webviews/webview-side/vega-renderer/ErrorBoundary.tsx(1 hunks)src/webviews/webview-side/vega-renderer/VegaRenderer.tsx(2 hunks)src/webviews/webview-side/vega-renderer/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/webviews/webview-side/vega-renderer/index.ts (3)
src/webviews/webview-side/react-common/errorBoundary.tsx (1)
ErrorBoundary(11-35)src/webviews/webview-side/vega-renderer/ErrorBoundary.tsx (1)
ErrorFallback(4-37)src/webviews/webview-side/vega-renderer/VegaRenderer.tsx (1)
VegaRenderer(50-121)
🔇 Additional comments (6)
package.json (1)
2198-2198: LGTM: Dependency addition aligns with usage.The react-error-boundary@^6.0.0 dependency is correctly added to support the new ErrorBoundary integration in the Vega renderer.
.eslintrc.js (1)
77-77: LGTM: ESLint configuration updated correctly.Adding react-error-boundary to the ignore list is necessary for unresolved import warnings.
src/webviews/webview-side/vega-renderer/index.ts (1)
4-6: LGTM: Imports are correct.ErrorBoundary from react-error-boundary and local ErrorFallback properly imported.
src/webviews/webview-side/vega-renderer/ErrorBoundary.tsx (1)
4-37: LGTM: Clean error UI with proper theming.ErrorFallback component correctly uses VSCode CSS variables and provides expandable stack trace. Inline styles are appropriate for this use case.
src/webviews/webview-side/vega-renderer/VegaRenderer.tsx (2)
21-48: LGTM: Theme detection implemented correctly.MutationObserver properly watches theme changes and cleanup prevents memory leaks. The approach is appropriate for VSCode webviews.
72-108: structuredClone is fully supported.VSCode 1.103.0 promotes Electron 37 to stable with Chromium 138.0.7204.100, which exceeds the Chrome 98+ requirement for
structuredClone. No compatibility concerns.
@coderabbitai ignore
Summary by CodeRabbit
Release Notes
New Features
Chores