feat(editor): add "Chart View" to display items as a chart #12677
Conversation
📝 (slash-menu): update slash menu to include "Chart View" entry with tooltip 📝 (tooltips): add tooltip for "Chart View" to enhance user experience ⬆️ (package.json): add chart.js dependency for chart rendering functionality ♻️ (view-manager): refactor ViewManager to support new chart view type 💡 (chart-view-manager): implement ChartSingleView to manage chart data 💡 (define): define ChartViewData type for chart view configuration 💡 (effect): create chartEffects function to define custom elements for chart view 📝 (index): export chart view related modules for better organization ✨ (chart): introduce Chart view UI and logic to enhance data visualization capabilities 📝 (chart): add styles for chart container to ensure proper layout and responsiveness ✅ (chart): integrate chart effects into view presets for consistent behavior 🔧 (keyboard-toolbar): add Chart view option to keyboard toolbar for easy access
|
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
WalkthroughThis change introduces a new "Chart View" feature to the data view and database modules. It adds chart view metadata, UI components, logic, and rendering using Chart.js. The chart view is integrated into view presets, slash menu, keyboard toolbar, and database block configurations, allowing users to create and interact with chart-based visualizations alongside existing table and kanban views. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Toolbar/Slash Menu)
participant Command Handler
participant DataView System
participant ChartViewManager
participant ChartViewUI
User->>UI (Toolbar/Slash Menu): Selects "Chart View"
UI (Toolbar/Slash Menu)->>Command Handler: Trigger chart view action
Command Handler->>DataView System: Insert database block with chartViewMeta
DataView System->>ChartViewManager: Initialize ChartSingleView
ChartViewManager->>ChartViewUI: Provide category counts data
ChartViewUI->>Chart.js: Render doughnut chart
ChartViewUI-->>User: Display interactive chart
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (6)
🪛 ESLintblocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts[error] 1-16: Run autofix to sort these imports! (simple-import-sort/imports) 🔇 Additional comments (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
blocksuite/affine/blocks/database/src/configs/tooltips.ts (1)
204-204: Missing newline at end of file.This file is missing a newline at the end, which can cause issues with some tools and version control systems.
🧹 Nitpick comments (11)
blocksuite/affine/blocks/database/src/configs/tooltips.ts (1)
191-198: Consider extracting inline styles to CSS classes for better maintainability.The inline styles work but could be moved to CSS classes to improve maintainability and consistency with the codebase styling approach.
- style=" - width: 20px; - height: 20px; - display: block; - fill: rgba(255, 255, 255, 0.81); - flex-shrink: 0; - margin-right: 6px; - " + class="chart-view-tooltip-icon"blocksuite/affine/data-view/src/view-presets/chart/index.ts (1)
3-7: Fix export sorting to address ESLint issue.The exports should be sorted alphabetically for consistency with the project's linting rules.
Apply this diff to sort the exports alphabetically:
-export * from './define.js'; -export * from './chart-view-manager.js'; -export * from './renderer.js'; -export * from './effect.js'; -export * from './styles.js'; +export * from './chart-view-manager.js'; +export * from './define.js'; +export * from './effect.js'; +export * from './renderer.js'; +export * from './styles.js';🧰 Tools
🪛 ESLint
[error] 3-7: Run autofix to sort these exports!
(simple-import-sort/exports)
blocksuite/affine/data-view/src/view-presets/index.ts (1)
8-8: Fix export sorting to comply with ESLint rules.The static analysis tool flagged an export sorting issue. Please run the autofix to maintain consistent code formatting.
-export * from './convert.js'; -export * from './kanban/index.js'; -export * from './table/index.js'; -export * from './chart'; +export * from './chart'; +export * from './convert.js'; +export * from './kanban/index.js'; +export * from './table/index.js';🧰 Tools
🪛 ESLint
[error] 5-8: Run autofix to sort these exports!
(simple-import-sort/exports)
blocksuite/affine/blocks/database/src/configs/slash-menu.ts (1)
12-12: Fix import sorting and organization.The ESLint rule indicates that imports should be sorted. Please run autofix to organize the imports properly.
🧰 Tools
🪛 ESLint
[error] 1-12: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/define.ts (1)
3-5: Fix import sorting.Please run autofix to sort the imports according to the ESLint rule.
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (2)
3-7: Fix import sorting.Please run autofix to sort the imports according to the ESLint rule.
🧰 Tools
🪛 ESLint
[error] 3-7: Run autofix to sort these imports!
(simple-import-sort/imports)
35-36: Improve type safety for cell value handling.The type assertion
as unknownis too broad and could mask type errors. Consider using a more specific type or proper type guard.- const raw = cell.jsonValue$.value as unknown; - const category = typeof raw === 'string' && raw.length > 0 ? raw : 'Undefined'; + const raw = cell.jsonValue$.value; + const category = typeof raw === 'string' && raw.length > 0 ? raw : 'Undefined';blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts (1)
3-7: Fix import sorting.Please run autofix to sort the imports according to the ESLint rule.
🧰 Tools
🪛 ESLint
[error] 3-7: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (3)
3-9: Fix import sorting per ESLint configuration.The imports are not properly sorted according to your project's ESLint rules.
Apply the ESLint autofix or manually sort the imports:
-import { DataViewUIBase } from '../../../core/view/data-view-base.js'; -import type { ChartViewUILogic } from './chart-view-ui-logic.js'; -import { html, css, LitElement } from 'lit'; -import { renderUniLit } from '../../../core/index.js'; -import Chart from 'chart.js/auto'; -import { styleMap } from 'lit/directives/style-map.js'; -import { chartContainerStyle } from '../styles.js'; +import Chart from 'chart.js/auto'; +import { css, html, LitElement } from 'lit'; +import { styleMap } from 'lit/directives/style-map.js'; + +import { renderUniLit } from '../../../core/index.js'; +import { DataViewUIBase } from '../../../core/view/data-view-base.js'; + +import { chartContainerStyle } from '../styles.js'; +import type { ChartViewUILogic } from './chart-view-ui-logic.js';🧰 Tools
🪛 ESLint
[error] 3-9: Run autofix to sort these imports!
(simple-import-sort/imports)
66-141: Solid chart creation logic with room for color palette improvement.The chart creation and data processing logic is well-implemented with proper memory management and error handling.
Consider expanding the color palette for better support of datasets with many categories:
// Pick a set of default colors (you can expand this array if you have more categories) const defaultColors = [ 'rgb(75, 192, 192)', // teal 'rgb(255, 205, 86)', // yellow 'rgb(54, 162, 235)', // blue 'rgb(255, 99, 132)', // red 'rgb(153, 102, 255)', // purple 'rgb(255, 159, 64)', // orange + 'rgb(199, 199, 199)', // grey + 'rgb(83, 102, 255)', // indigo + 'rgb(255, 159, 243)', // pink + 'rgb(159, 255, 64)', // lime + 'rgb(64, 159, 255)', // sky blue + 'rgb(255, 64, 159)', // magenta ];Or consider using a color generation function for unlimited categories:
// Alternative: Generate colors programmatically const generateColor = (index: number) => { const hue = (index * 137.508) % 360; // Golden angle approximation return `hsl(${hue}, 70%, 60%)`; }; const backgroundColor = labels.map((_, idx) => generateColor(idx));
147-151: Consider optimizing chart updates for better performance.The current implementation recreates the chart on every update, which may be inefficient for frequent property changes that don't affect chart data.
Consider checking if the relevant data has actually changed:
override updated(changedProps: Map<string, unknown>) { super.updated(changedProps); - // Whenever we know data has changed, rebuild the chart - this.createOrUpdateChart(); + // Only rebuild the chart if the data has actually changed + const currentCounts = this.logic.view.categoryCounts$.value; + const currentCountsString = JSON.stringify(currentCounts); + if (this.lastCountsString !== currentCountsString) { + this.lastCountsString = currentCountsString; + this.createOrUpdateChart(); + } }You would also need to add the tracking property:
export class ChartViewUI extends DataViewUIBase<ChartViewUILogic> { + private lastCountsString?: string; private canvasEl?: HTMLCanvasElement;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
blocksuite/affine/blocks/data-view/src/views/index.ts(1 hunks)blocksuite/affine/blocks/database/src/configs/slash-menu.ts(2 hunks)blocksuite/affine/blocks/database/src/configs/tooltips.ts(1 hunks)blocksuite/affine/blocks/database/src/views/index.ts(1 hunks)blocksuite/affine/data-view/package.json(1 hunks)blocksuite/affine/data-view/src/core/view-manager/view-manager.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/define.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/effect.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/index.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/renderer.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/styles.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/effect.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/index.ts(1 hunks)blocksuite/affine/widgets/keyboard-toolbar/src/config.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
blocksuite/affine/blocks/database/src/views/index.ts (1)
blocksuite/affine/data-view/src/view-presets/index.ts (1)
viewPresets(10-14)
blocksuite/affine/data-view/src/view-presets/effect.ts (1)
blocksuite/affine/data-view/src/view-presets/chart/effect.ts (1)
chartEffects(5-7)
blocksuite/affine/data-view/src/view-presets/chart/effect.ts (1)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (1)
ChartViewUI(17-152)
blocksuite/affine/data-view/src/view-presets/chart/renderer.ts (2)
blocksuite/affine/data-view/src/view-presets/chart/define.ts (1)
chartViewModel(17-28)blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts (1)
ChartViewUILogic(14-31)
blocksuite/affine/blocks/data-view/src/views/index.ts (1)
blocksuite/affine/data-view/src/view-presets/index.ts (1)
viewPresets(10-14)
blocksuite/affine/data-view/src/view-presets/chart/define.ts (2)
blocksuite/affine/data-view/src/core/view/data-view.ts (2)
viewType(65-80)BasicViewDataType(8-15)blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (1)
ChartSingleView(13-54)
blocksuite/affine/blocks/database/src/configs/slash-menu.ts (4)
blocksuite/affine/components/src/icons/text.ts (1)
DatabaseKanbanViewIcon(77-80)blocksuite/affine/blocks/database/src/configs/tooltips.ts (1)
ChartViewTooltip(185-204)blocksuite/affine/data-view/src/view-presets/index.ts (1)
viewPresets(10-14)blocksuite/affine/shared/src/services/telemetry-service/telemetry-service.ts (1)
TelemetryProvider(58-60)
blocksuite/affine/widgets/keyboard-toolbar/src/config.ts (2)
blocksuite/affine/components/src/icons/text.ts (1)
DatabaseKanbanViewIcon(77-80)blocksuite/affine/data-view/src/view-presets/index.ts (1)
viewPresets(10-14)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts (2)
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (1)
ChartSingleView(13-54)blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (1)
ChartViewUI(17-152)
🪛 ESLint
blocksuite/affine/data-view/src/view-presets/chart/index.ts
[error] 3-7: Run autofix to sort these exports!
(simple-import-sort/exports)
blocksuite/affine/data-view/src/view-presets/index.ts
[error] 5-8: Run autofix to sort these exports!
(simple-import-sort/exports)
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts
[error] 3-7: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/blocks/database/src/configs/slash-menu.ts
[error] 1-12: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts
[error] 3-9: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts
[error] 3-7: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (18)
blocksuite/affine/data-view/package.json (1)
26-26:✅ Verification successful
Verify Chart.js version for security and compatibility.
Please ensure that Chart.js version
^4.4.9is the latest stable version and free from known security vulnerabilities.
🌐 Web query:
What is the latest stable version of Chart.js and are there any known security vulnerabilities in version 4.4.9?💡 Result:
Latest Stable Version of Chart.js
The latest stable version of Chart.js as of June 2025 is 4.4.9[2][4][5][8]. This version was released on April 15, 2025, and is referenced as the most current in both official documentation and external lifecycle trackers[2][3][4][5][8].
Security Vulnerabilities in Version 4.4.9
There are no publicly documented or widely recognized security vulnerabilities affecting Chart.js version 4.4.9 as of the most recent updates:
- Security databases and vulnerability trackers do not list any CVEs or known issues specific to version 4.4.9[6][8][10].
- Prior Chart.js vulnerabilities, such as prototype pollution (CVE-2020-7746), affected only much older versions (notably those before 2.9.4), and there are no indications of unresolved critical security flaws in the 4.x branch[7][9].
- Lifecycle and security summaries confirm that version 4.4.9 is considered stable and fully supported[8].
If you are using 4.4.9, you are currently on the latest and most secure release. For the most up-to-date status, always check the project’s GitHub releases and security advisories.
Citations:
- 1: https://www.chartjs.org/docs/latest/getting-started/installation.html
- 2: https://github.com/chartjs/Chart.js/releases
- 3: https://www.chartjs.org/docs/
- 4: https://www.chartjs.org/docs/latest/getting-started/
- 5: https://en.wikipedia.org/wiki/Chart.js
- 6: https://probely.com/vulnerabilities/chartjs-library-with-known-vulnerabilities/
- 7: https://www.cvedetails.com/version/863974/Chartjs-Chart.js-2.9.4.html
- 8: https://www.versio.io/product-release-end-of-life-eol-Chart.js-Chart.js%20Team.html
- 9: https://www.versioneye.com/NodeJS/chart.js/2.7.3?page=1
- 10: https://security.snyk.io/package/npm/chart.js-latest
Chart.js ^4.4.9 is the latest stable release and has no known security issues
- File: blocksuite/affine/data-view/package.json (line 26)
- Version ^4.4.9 was released April 15, 2025 and is the current stable Chart.js
- No publicly documented CVEs or vulnerabilities affect 4.4.9
No further action needed.
blocksuite/affine/blocks/database/src/configs/tooltips.ts (1)
184-204: LGTM! Chart view tooltip implementation is solid.The SVG tooltip implementation is well-structured with proper accessibility attributes (
aria-hidden="true"androle="graphics-symbol"). The chart icon path data looks correct for representing a pie chart visualization.blocksuite/affine/blocks/database/src/views/index.ts (1)
7-7: LGTM! Clean integration of chart view into database views.The addition of
viewPresets.chartViewMetafollows the established pattern and properly integrates the new chart view option alongside existing table and kanban views. The change maintains consistency with the existing codebase structure.blocksuite/affine/data-view/src/view-presets/effect.ts (1)
3-3: LGTM! Clean integration following established patterns.The chart effects integration follows the exact same pattern as the existing kanban and table effects, maintaining consistency in the codebase architecture.
Also applies to: 9-9
🧰 Tools
🪛 ESLint
[error] 1-3: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/blocks/data-view/src/views/index.ts (1)
7-7: LGTM! Proper integration of chart view into block query views.The addition follows the established pattern perfectly, and the automatic map generation in
blockQueryViewMapwill correctly include the chart view.blocksuite/affine/data-view/src/view-presets/chart/styles.ts (1)
5-13: LGTM! Well-designed chart container styles.The flexbox layout with center alignment and appropriate padding creates a good foundation for chart display. The use of
border-boxsizing and descriptive naming follows best practices.blocksuite/affine/data-view/src/view-presets/chart/effect.ts (1)
1-7: LGTM! Clean custom element registration.The implementation correctly follows the established pattern for registering chart view UI as a custom element. The naming convention and structure are consistent with other view preset effects.
blocksuite/affine/data-view/src/view-presets/index.ts (2)
3-3: LGTM! Consistent import pattern.The import follows the same pattern as the existing table and kanban view imports.
🧰 Tools
🪛 ESLint
[error] 1-3: Run autofix to sort these imports!
(simple-import-sort/imports)
13-13: LGTM! Proper integration into view presets.The chart view metadata is correctly added to the viewPresets object following the established pattern.
blocksuite/affine/blocks/database/src/configs/slash-menu.ts (1)
86-117: LGTM! Chart View entry follows established patterns.The Chart View slash menu entry is well-structured and follows the same pattern as existing entries. The action logic, grouping, and conditional display are all implemented correctly.
blocksuite/affine/data-view/src/view-presets/chart/define.ts (1)
7-28: LGTM! Well-structured chart view preset definition.The chart view preset is properly defined with:
- Clear TypeScript types extending BasicViewDataType
- Sensible default behavior (selecting first property as category)
- Proper integration with the view system architecture
The implementation follows established patterns and provides good defaults for chart initialization.
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (1)
20-41: LGTM! Solid reactive category counting implementation.The
categoryCounts$computed signal effectively handles:
- Reactive data updates through signals
- Proper fallback when no category is selected
- Correct counting logic with default handling for undefined values
The implementation provides a clean interface for the chart UI to consume.
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts (1)
14-31: LGTM! Well-structured UI logic bridge.The ChartViewUILogic class provides a clean separation of concerns:
- Proper lifecycle management with Chart.js cleanup
- Reactive UI signal for component communication
- Clear documentation of purpose and responsibilities
The overall architecture follows established patterns in the data view system.
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (5)
17-32: Well-structured component definition and responsive styles.The class inheritance and CSS styles are well-implemented with proper responsive design considerations.
36-40: Proper lifecycle management.The connectedCallback implementation correctly establishes the connection between UI and logic layers.
42-54: Clean render method with proper conditional rendering.The template structure and conditional header widget rendering are well-implemented.
56-60: Correct use of firstUpdated lifecycle method.Properly queries the DOM after initial render and initializes the chart.
154-158: Proper TypeScript custom element declaration.The global interface extension correctly declares the custom element for type safety.
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts
Outdated
Show resolved
Hide resolved
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #12677 +/- ##
==========================================
- Coverage 55.87% 54.77% -1.11%
==========================================
Files 2652 2655 +3
Lines 125209 125205 -4
Branches 19806 19804 -2
==========================================
- Hits 69958 68577 -1381
- Misses 53124 54550 +1426
+ Partials 2127 2078 -49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This is a work in progress. |
…, effect.ts, index.ts, chart-view-ui-logic.ts): introduce "Chart View" functionality to display data visually ♻️ (chart-view-manager.ts, define.ts, chart-view-ui-logic.ts): refactor chart view logic for better maintainability and clarity 📝 (chart-view-manager.ts, define.ts): update comments to clarify chart view behavior and properties ✨ (chart-view-ui.ts): implement dynamic chart type selection based on view settings to enhance user experience ✨ (renderer.ts): add TODO for mobile-specific UI implementation to improve future development ✨ (view-options.ts): create chart menus for better chart configuration options in the UI 💡 (view-options.ts): add comments to clarify chart type and property selection logic for maintainability
| const type = chartType === 'pie' | ||
| ? 'doughnut' | ||
| : chartType === 'vertical-bar' || chartType === 'horizontal-bar' | ||
| ? 'bar' | ||
| : chartType === 'line' | ||
| ? 'line' | ||
| : 'bar'; |
There was a problem hiding this comment.
There's a type mismatch in the chart type mapping. The code checks for 'vertical-bar' and 'horizontal-bar' values, but the ChartType type definition in define.ts only includes 'pie', 'bar', 'stacked-bar', and 'line'. This mapping should be updated to use the correct type values to ensure type safety and prevent potential runtime errors.
| const type = chartType === 'pie' | |
| ? 'doughnut' | |
| : chartType === 'vertical-bar' || chartType === 'horizontal-bar' | |
| ? 'bar' | |
| : chartType === 'line' | |
| ? 'line' | |
| : 'bar'; | |
| const type = chartType === 'pie' | |
| ? 'doughnut' | |
| : chartType === 'bar' || chartType === 'stacked-bar' | |
| ? 'bar' | |
| : chartType === 'line' | |
| ? 'line' | |
| : 'bar'; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
blocksuite/affine/blocks/database/src/configs/slash-menu.ts (1)
86-86: Use appropriate icon for Chart View.The Chart View entry is using
DatabaseKanbanViewIcon()which is semantically incorrect. Consider creating or using a chart-specific icon to maintain visual consistency and user clarity.- icon: DatabaseKanbanViewIcon(), + icon: DatabaseChartViewIcon(), // or appropriate chart iconblocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (1)
45-51: 🛠️ Refactor suggestionRemove unnecessary method override.
This override simply calls the superclass method without adding any additional logic, making it redundant.
- /** - * Overrides propertyGetOrCreate only if you need custom Property handling. - * For Chart, we only group by an existing property; we do not need extra property logic. - */ - override propertyGetOrCreate(propertyId: string): ChartProperty { - return new ChartProperty(this, propertyId); - } + override propertyGetOrCreate(propertyId: string): ChartProperty { + return new ChartProperty(this, propertyId); + }
🧹 Nitpick comments (6)
blocksuite/affine/data-view/src/view-presets/chart/index.ts (1)
1-5: Fix export sorting order.The ESLint rule indicates the exports should be sorted. Apply the autofix to maintain consistency with project standards.
-export * from './define.js'; -export * from './chart-view-manager.js'; -export * from './renderer.js'; -export * from './effect.js'; -export * from './styles.js'; +export * from './chart-view-manager.js'; +export * from './define.js'; +export * from './effect.js'; +export * from './renderer.js'; +export * from './styles.js';🧰 Tools
🪛 ESLint
[error] 1-5: Run autofix to sort these exports!
(simple-import-sort/exports)
blocksuite/affine/blocks/database/src/configs/slash-menu.ts (1)
1-12: Fix import sorting order.The ESLint rule indicates the imports should be sorted. Apply the autofix to maintain consistency with project standards.
🧰 Tools
🪛 ESLint
[error] 1-12: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts (1)
1-6: Fix import sorting order.The ESLint rule indicates the imports should be sorted. Apply the autofix to maintain consistency with project standards.
🧰 Tools
🪛 ESLint
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (2)
1-7: Fix import order to comply with linting rules.The imports are not sorted according to the project's linting configuration.
+import Chart from 'chart.js/auto'; import { DataViewUIBase } from '../../../core/view/data-view-base.js'; +import { renderUniLit } from '../../../core/index.js'; +import { css, html, LitElement } from 'lit'; +import { styleMap } from 'lit/directives/style-map.js'; +import { chartContainerStyle } from '../styles.js'; import type { ChartViewUILogic } from './chart-view-ui-logic.js'; -import { html, css, LitElement } from 'lit'; -import { renderUniLit } from '../../../core/index.js'; -import Chart from 'chart.js/auto'; -import { styleMap } from 'lit/directives/style-map.js'; -import { chartContainerStyle } from '../styles.js';🧰 Tools
🪛 ESLint
[error] 1-7: Run autofix to sort these imports!
(simple-import-sort/imports)
88-100: Improve color palette scalability and accessibility.The hardcoded color palette has only 6 colors and may not provide sufficient contrast or accommodate users with color vision deficiencies.
Consider using a more extensive, accessible color palette:
- // Pick a set of default colors (you can expand this array if you have more categories) - const defaultColors = [ - 'rgb(75, 192, 192)', // teal - 'rgb(255, 205, 86)', // yellow - 'rgb(54, 162, 235)', // blue - 'rgb(255, 99, 132)', // red - 'rgb(153, 102, 255)', // purple - 'rgb(255, 159, 64)', // orange - ]; + // Use a more extensive, accessible color palette + const defaultColors = [ + '#1f77b4', '#ff7f0e', '#2ca02c', '#d62728', '#9467bd', '#8c564b', + '#e377c2', '#7f7f7f', '#bcbd22', '#17becf', '#aec7e8', '#ffbb78', + '#98df8a', '#ff9896', '#c5b0d5', '#c49c94', '#f7b6d3', '#c7c7c7', + '#dbdb8d', '#9edae5' + ];blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (1)
1-7: Fix import order to comply with linting rules.The imports are not sorted according to the project's linting configuration.
+import type { InsertToPosition } from '@blocksuite/affine-shared/utils'; +import { computed, type ReadonlySignal } from '@preact/signals-core'; +import type { Cell } from '../../core/view-manager/cell.js'; +import { PropertyBase } from '../../core/view-manager/property.js'; import { SingleViewBase } from '../../core/view-manager/single-view.js'; import type { ViewManager } from '../../core/view-manager/view-manager.js'; -import type { InsertToPosition } from '@blocksuite/affine-shared/utils'; -import { SingleViewBase } from '../../core/view-manager/single-view.js'; -import type { ViewManager } from '../../core/view-manager/view-manager.js'; import type { ChartViewData } from './define.js'; -import type { Cell } from '../../core/view-manager/cell.js'; -import { PropertyBase } from '../../core/view-manager/property.js';🧰 Tools
🪛 ESLint
[error] 1-7: Run autofix to sort these imports!
(simple-import-sort/imports)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
blocksuite/affine/blocks/database/src/configs/slash-menu.ts(2 hunks)blocksuite/affine/blocks/database/src/database-block.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/define.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/effect.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/index.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/renderer.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/styles.ts(1 hunks)blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- blocksuite/affine/blocks/database/src/database-block.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- blocksuite/affine/data-view/src/view-presets/chart/effect.ts
- blocksuite/affine/data-view/src/view-presets/chart/styles.ts
- blocksuite/affine/data-view/src/view-presets/chart/renderer.ts
- blocksuite/affine/data-view/src/view-presets/chart/define.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/blocks/database/src/configs/slash-menu.ts (4)
blocksuite/affine/components/src/icons/text.ts (1)
DatabaseKanbanViewIcon(77-80)blocksuite/affine/blocks/database/src/configs/tooltips.ts (1)
ChartViewTooltip(185-204)blocksuite/affine/data-view/src/view-presets/index.ts (1)
viewPresets(10-14)blocksuite/affine/shared/src/services/telemetry-service/telemetry-service.ts (1)
TelemetryProvider(60-62)
🪛 Biome (1.9.4)
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts
[error] 88-93: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🪛 ESLint
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts
[error] 1-7: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/blocks/database/src/configs/slash-menu.ts
[error] 1-12: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/index.ts
[error] 1-5: Run autofix to sort these exports!
(simple-import-sort/exports)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts
[error] 1-7: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (6)
blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts (2)
232-329: Chart menu implementation has incomplete functionality.The chart menu implementation looks well-structured, but several menu items have placeholder implementations that need to be completed:
- "Each slice represents" (lines 295-301)
- "Sort by" (lines 303-310)
- "Color" (lines 312-318)
- "More style options" (lines 320-326)
These empty
selectfunctions will provide no functionality to users.Are these placeholder implementations intentional for this PR, or should they be implemented before merging?
237-267: Well-implemented chart type and property selection.The chart type selection and "What to show" property selection logic is properly implemented with correct state management and UI updates.
Also applies to: 269-292
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui-logic.ts (2)
13-29: Excellent Chart.js lifecycle management.The class properly manages the Chart.js instance lifecycle with appropriate cleanup in
onHostDisconnected. The reactive signal pattern and renderer integration follow established patterns well.
31-52: Appropriate no-op implementations for chart view.The no-op implementations for selection, row operations, and navigation make sense since these concepts don't apply to chart visualizations.
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (2)
22-43: Excellent reactive data computation implementation.The
categoryCounts$computed signal correctly handles data aggregation with proper fallbacks for undefined values and efficient row processing.
94-114: Well-designed minimal property implementation.The
ChartPropertyclass appropriately disables column hiding and moving operations since they don't apply to chart views, with clear no-op implementations.
| override updated(changedProps: Map<string, unknown>) { | ||
| super.updated(changedProps); | ||
| // Whenever we know data has changed, rebuild the chart | ||
| this.createOrUpdateChart(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize chart updates to prevent unnecessary re-renders.
The updated method recreates the chart on every property change, which could be inefficient and cause flickering.
override updated(changedProps: Map<string, unknown>) {
super.updated(changedProps);
- // Whenever we know data has changed, rebuild the chart
- this.createOrUpdateChart();
+ // Only rebuild chart if category counts have actually changed
+ const currentCounts = this.logic.view.categoryCounts$.value;
+ if (JSON.stringify(currentCounts) !== JSON.stringify(this._lastCounts)) {
+ this._lastCounts = { ...currentCounts };
+ this.createOrUpdateChart();
+ }
}Also add the property to track previous state:
private canvasEl?: HTMLCanvasElement;
+ private _lastCounts: Record<string, number> = {};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override updated(changedProps: Map<string, unknown>) { | |
| super.updated(changedProps); | |
| // Whenever we know data has changed, rebuild the chart | |
| this.createOrUpdateChart(); | |
| } | |
| // add a field to remember the last seen counts | |
| private canvasEl?: HTMLCanvasElement; | |
| private _lastCounts: Record<string, number> = {}; | |
| override updated(changedProps: Map<string, unknown>) { | |
| super.updated(changedProps); | |
| // Only rebuild chart if category counts have actually changed | |
| const currentCounts = this.logic.view.categoryCounts$.value; | |
| if (JSON.stringify(currentCounts) !== JSON.stringify(this._lastCounts)) { | |
| this._lastCounts = { ...currentCounts }; | |
| this.createOrUpdateChart(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts around
lines 162 to 166, the updated method currently calls createOrUpdateChart on
every property change, causing unnecessary chart re-renders and potential
flickering. Modify the method to check if relevant data properties have actually
changed before calling createOrUpdateChart. Add a class property to track the
previous state of the data used for the chart, and only rebuild the chart when
this data differs from the new state.
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts
Outdated
Show resolved
Hide resolved
| constructor(viewManager: ViewManager, viewId: string) { | ||
| super(viewManager, viewId); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary constructor.
The constructor only calls the superclass constructor without any additional logic.
- constructor(viewManager: ViewManager, viewId: string) {
- super(viewManager, viewId);
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(viewManager: ViewManager, viewId: string) { | |
| super(viewManager, viewId); | |
| } | |
| // (constructor removed — no replacement needed) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 88-93: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🤖 Prompt for AI Agents
In blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts at
lines 88 to 90, the constructor is redundant as it only calls the superclass
constructor without adding any logic. Remove the entire constructor method to
simplify the class definition and rely on the default superclass constructor
behavior.
…consistency in documentation ✨ (chart-view-ui.ts): enhance ChartViewUI component with improved layout and functionality 📝 (chart-view-ui.ts): update comments for clarity and add file header documentation ♻️ (chart-view-ui.ts): refactor chart rendering logic to handle "Status" key removal and improve color assignment logic ✨ (chart-view-ui.ts): add custom Chart.js plugins for center text and outer callout lines to enhance chart visualization ♻️ (chart-view-ui.ts): refactor chart configuration to improve readability and maintainability 📝 (chart-view-ui.ts): update comments for clarity and to reflect new functionality in the chart rendering process
| : chartType === 'vertical-bar' || chartType === 'horizontal-bar' | ||
| ? 'bar' | ||
| : chartType === 'line' | ||
| ? 'line' | ||
| : 'bar'; |
There was a problem hiding this comment.
There appears to be a type mismatch in the chart type mapping logic. The ChartType union is defined as 'pie' | 'bar' | 'stacked-bar' | 'line', but this code checks for 'vertical-bar' and 'horizontal-bar' which aren't part of that union. Additionally, the fallback case always returns 'bar', which might not be the intended default behavior.
Consider updating either the ChartType union definition to include these additional types, or modify this conditional to only check for the types defined in the union. Also, review whether defaulting to 'bar' is the correct fallback behavior.
| : chartType === 'vertical-bar' || chartType === 'horizontal-bar' | |
| ? 'bar' | |
| : chartType === 'line' | |
| ? 'line' | |
| : 'bar'; | |
| : chartType === 'bar' || chartType === 'stacked-bar' | |
| ? 'bar' | |
| : chartType === 'line' | |
| ? 'line' | |
| : chartType === 'pie' | |
| ? 'pie' | |
| : 'bar'; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| import type { ChartSingleView } from '../../../view-presets/chart/chart-view-manager.js'; | ||
| import type { ChartType } from '../../../view-presets/chart/define.js'; |
There was a problem hiding this comment.
The import paths need to be corrected with an additional parent directory level. The current paths:
import type { ChartSingleView } from '../../../view-presets/chart/chart-view-manager.js';
import type { ChartType } from '../../../view-presets/chart/define.js';Should be updated to:
import type { ChartSingleView } from '../../../../view-presets/chart/chart-view-manager.js';
import type { ChartType } from '../../../../view-presets/chart/define.js';This ensures the imports correctly reference the files based on the relative directory structure.
| import type { ChartSingleView } from '../../../view-presets/chart/chart-view-manager.js'; | |
| import type { ChartType } from '../../../view-presets/chart/define.js'; | |
| import type { ChartSingleView } from '../../../../view-presets/chart/chart-view-manager.js'; | |
| import type { ChartType } from '../../../../view-presets/chart/define.js'; | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (2)
46-51: Remove unnecessary method override.The
propertyGetOrCreateoverride simply calls the superclass method without any additional logic. This override is unnecessary and should be removed.
88-90: Remove unnecessary constructor.The constructor only calls the superclass constructor without any additional logic.
🧰 Tools
🪛 Biome (1.9.4)
[error] 88-93: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (2)
83-138: Fix chart configuration issues for different chart types.Several issues with the chart configuration that could cause visual problems:
- The chart type mapping logic references
'vertical-bar'and'horizontal-bar'which may not exist in the type definition- The implementation could be simplified
312-316: Optimize chart updates to prevent unnecessary re-renders.The
updatedmethod recreates the chart on every property change, which could be inefficient and cause flickering.
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (1)
1-8: Fix import sorting to follow project conventions.The imports are not sorted according to the project's ESLint configuration.
-import { computed, type ReadonlySignal } from '@preact/signals-core'; -import type { InsertToPosition } from '@blocksuite/affine-shared/utils'; -import { SingleViewBase } from '../../core/view-manager/single-view.js'; -import type { ViewManager } from '../../core/view-manager/view-manager.js'; -import type { ChartViewData } from './define.js'; -import type { Cell } from '../../core/view-manager/cell.js'; -import { PropertyBase } from '../../core/view-manager/property.js'; +import { computed, type ReadonlySignal } from '@preact/signals-core'; + +import type { InsertToPosition } from '@blocksuite/affine-shared/utils'; + +import type { Cell } from '../../core/view-manager/cell.js'; +import { PropertyBase } from '../../core/view-manager/property.js'; +import { SingleViewBase } from '../../core/view-manager/single-view.js'; +import type { ViewManager } from '../../core/view-manager/view-manager.js'; +import type { ChartViewData } from './define.js';🧰 Tools
🪛 ESLint
[error] 1-7: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (1)
3-8: Fix import sorting to follow project conventions.The imports are not sorted according to the project's ESLint configuration.
-import { DataViewUIBase } from '../../../core/view/data-view-base.js'; -import type { ChartViewUILogic } from './chart-view-ui-logic.js'; -import { html, css, LitElement } from 'lit'; -import { renderUniLit } from '../../../core/index.js'; -import Chart from 'chart.js/auto'; -import { chartContainerStyle } from '../styles.js'; +import Chart from 'chart.js/auto'; +import { html, css, LitElement } from 'lit'; + +import { renderUniLit } from '../../../core/index.js'; +import { DataViewUIBase } from '../../../core/view/data-view-base.js'; +import { chartContainerStyle } from '../styles.js'; +import type { ChartViewUILogic } from './chart-view-ui-logic.js';🧰 Tools
🪛 ESLint
[error] 3-8: Run autofix to sort these imports!
(simple-import-sort/imports)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts
[error] 3-8: Run autofix to sort these imports!
(simple-import-sort/imports)
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts
[error] 1-7: Run autofix to sort these imports!
(simple-import-sort/imports)
🪛 Biome (1.9.4)
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts
[error] 88-93: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (4)
blocksuite/affine/data-view/src/view-presets/chart/chart-view-manager.ts (2)
22-43: LGTM! Solid reactive categorization logic.The
categoryCounts$computed signal correctly handles category counting with proper fallback to "Undefined" for missing values. The logic appropriately usesstringValue$to ensure Select/Multi-select properties display their tag names.
94-114: LGTM! Appropriate chart-specific property behavior.The
ChartPropertyclass correctly enforces that properties in chart view are always visible and cannot be hidden or reordered, which aligns with the chart view's data presentation requirements.blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (2)
23-46: LGTM! Well-structured component styling.The CSS styling provides a clean dark theme with proper layout for the chart container. The fixed height approach prevents chart distortion.
143-223: LGTM! Excellent custom Chart.js plugins.The center text and outer label plugins are well-implemented with proper Chart.js lifecycle hooks. The visual styling matches the design requirements with appropriate opacity and positioning.
| this.logic.chartInstance = new Chart(ctx, { | ||
| type, | ||
| data: { | ||
| // **IMPORTANT**: We explicitly set `labels: rawLabels` so that the bottom legend | ||
| // only shows “Complete”, “In Progress”, “TODO”, “DNF”, etc. (No “Status”). | ||
| labels: rawLabels, | ||
| datasets: [ | ||
| { | ||
| // Setting `label: ''` ensures Chart.js never auto-prepends “Status” anywhere | ||
| label: '', | ||
| data: dataValues, | ||
| backgroundColor, | ||
| borderWidth: 0, | ||
| hoverOffset: 4, | ||
| // Outer radius = 70% – 80% works well; here we choose 75% | ||
| radius: '75%', | ||
| // Inner cutout = 75% so that the ring is neither too thin nor too thick | ||
| cutout: '75%', | ||
| }, | ||
| ], | ||
| }, | ||
| options: { | ||
| responsive: true, | ||
| maintainAspectRatio: false, | ||
| indexAxis: horizontal ? 'y' : 'x', | ||
| layout: { | ||
| // Keep a little padding but not so much that it looks “squished” | ||
| padding: { | ||
| top: 12, | ||
| bottom: 12, | ||
| left: 12, | ||
| right: 12, | ||
| }, | ||
| }, | ||
| plugins: { | ||
| // ─── Legend ─────────────────────────────────────────────────────────────── | ||
| legend: { | ||
| position: 'bottom', | ||
| labels: { | ||
| // Tiny 8×8 rounded box (rx=2) | ||
| boxWidth: 8, | ||
| boxHeight: 8, | ||
| borderRadius: 2, | ||
| boxBorderColor: 'transparent', | ||
| // Legend text = 12px, rgba(255,255,255,0.46) | ||
| color: 'rgba(255, 255, 255, 0.46)', | ||
| font: { | ||
| size: 12, | ||
| weight: '400', | ||
| }, | ||
| padding: 16, // space between items | ||
| }, | ||
| }, | ||
|
|
||
| // ─── Tooltip ─────────────────────────────────────────────────────────────── | ||
| tooltip: { | ||
| enabled: true, | ||
| callbacks: { | ||
| label: (ctx) => { | ||
| const count = ctx.parsed as number; | ||
| const pct = | ||
| total > 0 | ||
| ? ((count / total) * 100).toFixed(1) | ||
| : '0.0'; | ||
| const rawLabel = rawLabels[ctx.dataIndex]; | ||
| return `${rawLabel}: ${count} (${pct}%)`; | ||
| }, | ||
| }, | ||
| backgroundColor: '#222', | ||
| titleColor: '#fff', | ||
| bodyColor: '#fff', | ||
| }, | ||
| }, | ||
|
|
||
| // ─── Disable ALL built-in “datalabels” (in case you had chartjs-plugin-datalabels) ─── | ||
| // This ensures no extra text (like “Status”) is ever rendered automatically on each slice. | ||
| datalabels: { | ||
| display: false, | ||
| } as any, // cast to any if you haven’t installed chartjs-plugin-datalabels | ||
| }, | ||
| plugins: [centerTextPlugin, outerLabelPlugin], | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix chart configuration for better type support.
The chart configuration has hardcoded properties that should be conditional:
cutoutproperty should only apply to doughnut chartsradiusproperty is specific to doughnut/pie charts
datasets: [
{
label: '',
data: dataValues,
backgroundColor,
borderWidth: 0,
hoverOffset: 4,
- radius: '75%',
- cutout: '75%',
+ ...(isDoughnut && {
+ radius: '75%',
+ cutout: '75%'
+ }),
},
],Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts
between lines 228 and 310, the chart configuration incorrectly applies the
'cutout' and 'radius' properties unconditionally. To fix this, modify the code
to set 'cutout' and 'radius' only when the chart type is 'doughnut' or 'pie'.
Implement conditional logic to include these properties only for those chart
types to ensure proper type support and avoid configuration errors.
NorkzYT
changed the title
style(chart-view-ui.ts): adjust chart height for better visual compactness style(chart-view-ui.ts): modify label formatting for improved readability style(chart-view-ui.ts): increase extension factor for better label positioning style(chart-view-ui.ts): update text alignment logic for dynamic labels style(chart-view-ui.ts): change border width and cutout for better aesthetics style(chart-view-ui.ts): adjust layout padding for improved spacing style(chart-view-ui.ts): change background color for better contrast style(styles.ts): increase padding in chart container for better layout
| :host { | ||
| //display: block; | ||
| //box-sizing: border-box; | ||
| //background-color: #121212; /* dark background like Notion */ | ||
| } |
There was a problem hiding this comment.
The CSS template literal contains JavaScript-style single-line comments (//) which are not valid in CSS. These should be replaced with proper CSS comment syntax (/* ... */) to ensure correct styling behavior. For example:
:host {
/* display: block; */
/* box-sizing: border-box; */
/* background-color: #121212; /* dark background like Notion */ */
}Alternatively, if these are just development notes that should be removed entirely before merging, consider cleaning up the unused style declarations.
Spotted by Diamond (based on custom rules)
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (3)
132-136:⚠️ Potential issueFix chart type mapping to match ChartType union definition.
The code checks for
'vertical-bar'and'horizontal-bar'values, but according to past reviews, theChartTypeunion only includes'pie','bar','stacked-bar', and'line'.- : chartType === 'vertical-bar' || chartType === 'horizontal-bar' - ? 'bar' + : chartType === 'bar' || chartType === 'stacked-bar' + ? 'bar' : chartType === 'line' ? 'line' : 'bar';
261-262:⚠️ Potential issueApply radius and cutout properties conditionally for doughnut charts only.
The
radiusandcutoutproperties are hardcoded and applied to all chart types, but they should only apply to doughnut/pie charts.backgroundColor, borderWidth: 1, hoverOffset: 4, - // Slightly smaller and thinner ring - radius: '75%', - cutout: '85%', + ...(isDoughnut && { + radius: '75%', + cutout: '85%' + }),
329-333: 🛠️ Refactor suggestionOptimize chart updates to prevent unnecessary re-renders.
The chart is recreated on every property change, which could cause performance issues and flickering.
Consider implementing change detection to only rebuild when data actually changes.
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (2)
25-27: Remove commented-out CSS properties.The commented-out styles should be removed to keep the code clean.
- //display: block; - //box-sizing: border-box; - //background-color: #121212; /* dark background like Notion */
321-323: Address type casting for datalabels configuration.The
datalabelsproperty is cast toanyto work around typing issues. Consider installing the proper types or using a more type-safe approach.- datalabels: { - display: false, - } as any, // cast to any if you haven't installed chartjs-plugin-datalabels + // Remove if chartjs-plugin-datalabels is not used, or install proper types + ...(typeof Chart.defaults.plugins?.datalabels !== 'undefined' && { + datalabels: { display: false } + }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/chart/styles.ts(1 hunks)
🔇 Additional comments (1)
blocksuite/affine/data-view/src/view-presets/chart/styles.ts (1)
1-11: LGTM! Clean and appropriate styling for chart container.The CSS-in-JS implementation is well-structured and the flexbox properties are appropriate for centering chart content within the container.
…actions to enhance user experience and data visibility feat(chart-view-ui.ts): implement external tooltip handler to display detailed data on hover and click events refactor(chart-view-ui.ts): restructure tooltip and dialog rendering logic for better maintainability and clarity
| options: { | ||
| responsive: true, | ||
| maintainAspectRatio: false, | ||
| indexAxis: horizontal ? 'y' : 'x', | ||
| onClick: this.handleChartClick, | ||
| layout: { | ||
| // Keep a little padding but not so much that it looks “squished” | ||
| padding: { | ||
| top: 80, | ||
| bottom: 12, | ||
| left: 12, | ||
| right: 12, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The stacked bar chart type is defined but missing the required Chart.js configuration. For proper stacking behavior, add the scales configuration to the options object:
options: {
responsive: true,
maintainAspectRatio: false,
indexAxis: horizontal ? 'y' : 'x',
onClick: this.handleChartClick,
// Add scales configuration for stacked bars
scales: chartType === 'stacked-bar' ? {
x: { stacked: true },
y: { stacked: true }
} : undefined,
layout: {
// ...Without this configuration, the 'stacked-bar' option will render as a regular bar chart with overlapping bars instead of properly stacked segments.
| options: { | |
| responsive: true, | |
| maintainAspectRatio: false, | |
| indexAxis: horizontal ? 'y' : 'x', | |
| onClick: this.handleChartClick, | |
| layout: { | |
| // Keep a little padding but not so much that it looks “squished” | |
| padding: { | |
| top: 80, | |
| bottom: 12, | |
| left: 12, | |
| right: 12, | |
| }, | |
| }, | |
| options: { | |
| responsive: true, | |
| maintainAspectRatio: false, | |
| indexAxis: horizontal ? 'y' : 'x', | |
| onClick: this.handleChartClick, | |
| scales: chartType === 'stacked-bar' ? { | |
| x: { stacked: true }, | |
| y: { stacked: true } | |
| } : undefined, | |
| layout: { | |
| // Keep a little padding but not so much that it looks "squished" | |
| padding: { | |
| top: 80, | |
| bottom: 12, | |
| left: 12, | |
| right: 12, | |
| }, | |
| }, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| override connectedCallback(): void { | ||
| super.connectedCallback(); | ||
| // Let our logic know we’re ready to receive updates | ||
| this.logic.ui$.value = this; | ||
| } |
There was a problem hiding this comment.
The tooltipEl element is created and appended to the DOM in getOrCreateTooltip() but never properly cleaned up when the component is removed. This could lead to memory leaks and orphaned DOM elements.
Consider adding cleanup in a disconnectedCallback method:
override disconnectedCallback(): void {
this.tooltipEl?.remove();
super.disconnectedCallback();
}This ensures proper resource management by removing the tooltip element when the component is disconnected from the DOM.
| override connectedCallback(): void { | |
| super.connectedCallback(); | |
| // Let our logic know we’re ready to receive updates | |
| this.logic.ui$.value = this; | |
| } | |
| override connectedCallback(): void { | |
| super.connectedCallback(); | |
| // Let our logic know we're ready to receive updates | |
| this.logic.ui$.value = this; | |
| } | |
| override disconnectedCallback(): void { | |
| this.tooltipEl?.remove(); | |
| super.disconnectedCallback(); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (4)
25-27: Fix invalid CSS comment syntax.The CSS template literal contains JavaScript-style single-line comments (
//) which are not valid in CSS. These should be replaced with proper CSS comment syntax (/* ... */) or removed entirely.:host { - //display: block; - //box-sizing: border-box; - //background-color: #121212; /* dark background like Notion */ + /* display: block; */ + /* box-sizing: border-box; */ + /* background-color: #121212; /* dark background like Notion */ */ }
224-228: Fix chart type mapping to use valid ChartType union values.There's a type mismatch in the chart type mapping. The code checks for
'vertical-bar'and'horizontal-bar'values, but these may not be part of theChartTypeunion definition. This mapping should be updated to use the correct type values to ensure type safety and prevent potential runtime errors.const type = isDoughnut ? 'doughnut' - : chartType === 'vertical-bar' || chartType === 'horizontal-bar' + : chartType === 'bar' || chartType === 'stacked-bar' ? 'bar' : chartType === 'line' ? 'line' : 'bar';
353-354: Apply chart-specific properties conditionally.The
radiusandcutoutproperties are hardcoded but should only apply to doughnut/pie charts. These properties should be applied conditionally to ensure proper type support and avoid configuration errors.hoverOffset: 4, - // Slightly smaller and thinner ring - radius: '75%', - cutout: '85%', + ...(isDoughnut && { + radius: '75%', + cutout: '85%' + }),
517-521: Optimize chart updates to prevent unnecessary re-renders.The
updatedmethod recreates the chart on every property change, which could be inefficient and cause flickering. This should only rebuild when category counts have actually changed.override updated(changedProps: Map<string, unknown>) { super.updated(changedProps); - // Whenever new data arrives, re-draw the chart - this.createOrUpdateChart(); + // Only rebuild chart if category counts have actually changed + const currentCounts = this.logic.view.categoryCounts$.value; + if (JSON.stringify(currentCounts) !== JSON.stringify(this._lastCounts)) { + this._lastCounts = { ...currentCounts }; + this.createOrUpdateChart(); + } }Also add the property to track previous state:
private canvasEl?: HTMLCanvasElement; + private _lastCounts: Record<string, number> = {};
🧹 Nitpick comments (1)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (1)
3-8: Fix import sorting to comply with ESLint rules.The static analysis tool indicates that imports need to be sorted according to the project's ESLint configuration.
Run
npm run lint:fixor manually reorder the imports to resolve this issue.🧰 Tools
🪛 ESLint
[error] 3-8: Run autofix to sort these imports!
(simple-import-sort/imports)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts
[error] 3-8: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (1)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (1)
173-407: Well-implemented Chart.js integration with excellent interactive features.The chart implementation demonstrates solid engineering with comprehensive Chart.js integration, custom plugins for enhanced visuals, and interactive features like tooltips and data dialogs. The separation of concerns and plugin architecture is well designed.
… display in ChartViewUI to enhance user interaction and data visualization refactor(chart-view-ui.ts): clean up and optimize data dialog rendering logic for better maintainability and performance
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (6)
56-60: Fix CSS comment syntax.CSS template literals should use proper CSS comment syntax (
/* */) instead of JavaScript-style comments (//).
258-262: Fix chart type mapping to match ChartType union.The code checks for
'vertical-bar'and'horizontal-bar'values, but these aren't part of theChartTypeunion definition which only includes'pie','bar','stacked-bar', and'line'.
387-388: Apply chart properties conditionally based on chart type.The
radiusandcutoutproperties are hardcoded but should only apply to doughnut/pie charts.
392-405: Add missing scales configuration for stacked bar charts.The stacked bar chart type is supported but lacks the required Chart.js scales configuration for proper stacking behavior.
168-172: Add cleanup for tooltip element to prevent memory leaks.The
tooltipElelement is created and appended to the DOM but never properly cleaned up when the component is removed.
574-578: Optimize chart updates to prevent unnecessary re-renders.The
updatedmethod recreates the chart on every property change, which could be inefficient and cause flickering.
🧹 Nitpick comments (1)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts (1)
502-517: Improve error handling in filter creation logic.The filter creation logic could fail if
prop.parseValueFromString()returns null or if the property type is unexpected, but there's no error handling for these cases.Add error handling to make the filter creation more robust:
const prop = this.logic.view.propertyGetOrCreate(categoryId); const parsed = prop.parseValueFromString(category); let filterFn = 'is'; let filterValue: unknown = category; const type = prop.type$.value; - if (parsed) { + if (parsed && parsed.value !== undefined) { filterValue = parsed.value; if (type === 'select') { filterFn = 'isOneOf'; filterValue = [parsed.value]; } else if (type === 'multi-select') { filterFn = 'containsOneOf'; filterValue = [parsed.value]; + } else { + // Handle other property types as needed + filterFn = 'is'; } + } else { + // Fallback to string comparison if parsing fails + filterFn = 'is'; + filterValue = category; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts
[error] 1-16: Run autofix to sort these imports!
(simple-import-sort/imports)
| import { DataViewUIBase } from '../../../core/view/data-view-base.js'; | ||
| import type { ChartViewUILogic } from './chart-view-ui-logic.js'; | ||
| import { html, css, LitElement } from 'lit'; | ||
| import { state } from 'lit/decorators.js'; | ||
| import { signal, computed } from '@preact/signals-core'; | ||
| import { renderUniLit } from '../../../core/index.js'; | ||
| import Chart from 'chart.js/auto'; | ||
| import { chartContainerStyle } from '../styles.js'; | ||
| import { tableViewStyle } from '../../table/pc/table-view-style.js'; | ||
| import { TableSingleView } from '../../table/table-view-manager.js'; | ||
| import { tableViewModel } from '../../table/define.js'; | ||
| import { TableViewUILogic } from '../../table/pc/table-view-ui-logic.js'; | ||
| import type { TableViewData } from '../../table/define.js'; | ||
| import { DEFAULT_COLUMN_WIDTH } from '../../table/consts.js'; | ||
| import type { FilterGroup } from '../../../core/filter/types.js'; | ||
| import type { ViewManager } from '../../../core/view-manager/view-manager.js'; |
There was a problem hiding this comment.
Fix import sorting per ESLint rules.
The imports are not sorted according to the project's ESLint configuration.
Apply the autofix suggested by ESLint to sort these imports properly.
🧰 Tools
🪛 ESLint
[error] 1-16: Run autofix to sort these imports!
(simple-import-sort/imports)
🤖 Prompt for AI Agents
In blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts lines
1 to 16, the import statements are not sorted according to the project's ESLint
rules. Reorder the import statements by grouping and sorting them alphabetically
as per ESLint autofix suggestions to ensure consistent and clean import
organization.
| datalabels: { | ||
| display: false, | ||
| } as any, // cast to any if you haven’t installed chartjs-plugin-datalabels |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove type assertion for better type safety.
The as any cast bypasses TypeScript's type checking and should be avoided. If chartjs-plugin-datalabels is not installed, consider using a proper interface or conditional typing.
Consider one of these approaches:
- datalabels: {
- display: false,
- } as any, // cast to any if you haven't installed chartjs-plugin-datalabels
+ // Option 1: Only include if plugin is available
+ ...(typeof Chart.defaults.plugins?.datalabels !== 'undefined' && {
+ datalabels: { display: false }
+ }),Or define a proper interface if this property is expected:
+interface ChartOptionsWithDatalabels extends ChartConfiguration['options'] {
+ datalabels?: { display: boolean };
+}
+
- datalabels: {
- display: false,
- } as any,
+ datalabels: {
+ display: false,
+ },Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In blocksuite/affine/data-view/src/view-presets/chart/pc/chart-view-ui.ts around
lines 435 to 437, remove the 'as any' type assertion on the datalabels property
to improve type safety. Instead, define a proper interface for the datalabels
configuration or use conditional typing based on whether
chartjs-plugin-datalabels is installed, ensuring TypeScript can properly check
the types without bypassing them.
…tent styles for better UI organization style(chart-view-ui.ts): refactor close button positioning and improve header structure for clarity and usability
This PR is to add the Layout View of a Chart to be able to view your data in different ways using
Chart.js.Example images from Notion: (Near expected functional outcome).
Summary by CodeRabbit
New Features
Enhancements
Dependencies