feat(Chart): keep hidden of data value when update#878
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideSynchronizes dataset hidden state with the existing chart legend items during updates so that previously hidden series remain hidden, specifically for non-pie/doughnut charts, without altering other chart configuration behavior. Sequence diagram for chart update preserving hidden legend statesequenceDiagram
participant Caller
participant ChartInterop as ChartUpdateFunction
participant DataStore as DataStore
participant Chart as ChartInstance
participant Legend as LegendItems
Caller->>ChartInterop: update(id, option, method, angle)
ChartInterop->>DataStore: Data.get(id)
DataStore-->>ChartInterop: { invoke, chart }
loop ForEachDataset
ChartInterop->>Legend: find legendItem where legendItem.text == dataset.label
alt LegendItemFound
Legend-->>ChartInterop: legendItem.hidden
ChartInterop->>ChartInterop: dataset.hidden = legendItem.hidden
else LegendItemNotFound
ChartInterop->>ChartInterop: leave dataset.hidden unchanged
end
end
ChartInterop->>ChartInterop: op = getChartOption(option)
ChartInterop->>Caller: handlerClickData(invoke, op, option.options.onClickDataMethod)
ChartInterop->>Chart: apply updated options with preserved hidden flags
Chart-->>Caller: chart updated with consistent hidden state
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When syncing hidden states in update, guard against cases where
chart.legendorchart.legend.legendItemsmight be undefined to avoid runtime errors during initialization or non-standard configurations. - Matching legend items to datasets by
text === d.labelcan break when labels are duplicated or localized; consider using the dataset index (e.g., legend item index) or another stable identifier instead of the label string. - Defensively handle cases where
option.datais null/undefined or not an array before callingforEachto prevent unexpected runtime exceptions during update.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When syncing hidden states in update, guard against cases where `chart.legend` or `chart.legend.legendItems` might be undefined to avoid runtime errors during initialization or non-standard configurations.
- Matching legend items to datasets by `text === d.label` can break when labels are duplicated or localized; consider using the dataset index (e.g., legend item index) or another stable identifier instead of the label string.
- Defensively handle cases where `option.data` is null/undefined or not an array before calling `forEach` to prevent unexpected runtime exceptions during update.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.Chart/Components/Chart/Chart.razor.js:493-495` </location>
<code_context>
export function update(id, option, method, angle) {
- const { invoke, chart } = Data.get(id)
+ const { invoke, chart } = Data.get(id);
+ option.data.forEach(d => {
+ const l = chart.legend.legendItems.find(i => i.text === d.label);
+ if(l) {
+ d.hidden = l.hidden;
</code_context>
<issue_to_address>
**issue (bug_risk):** Access to `chart.legend.legendItems` should be guarded to avoid runtime errors when legend is disabled or unavailable.
When the legend is disabled or altered by plugins, `chart.legend` / `chart.legend.legendItems` can be `undefined`, causing `update` to throw. Add a defensive check, e.g.:
```js
const { invoke, chart } = Data.get(id);
const legendItems = chart.legend && chart.legend.legendItems;
if (legendItems) {
option.data.forEach(d => {
const l = legendItems.find(i => i.text === d.label);
if (l) d.hidden = l.hidden;
});
}
```
This preserves the current behavior when a legend exists while preventing runtime errors when it does not.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to preserve the hidden state of chart data items when updating charts. When a user hides a dataset via the legend and then updates the chart data, the hidden state is now maintained instead of being reset. The version is bumped from 10.0.1 to 10.0.2.
Key changes:
- Added logic in the
updatefunction to sync hidden states from existing legend items to incoming data - Removed a redundant Chinese comment that duplicated what the code clearly expresses
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/components/BootstrapBlazor.Chart/Components/Chart/Chart.razor.js | Added logic to preserve hidden state of datasets during chart updates by matching legend item states; removed redundant comment |
| src/components/BootstrapBlazor.Chart/BootstrapBlazor.Chart.csproj | Bumped version from 10.0.1 to 10.0.2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { invoke, chart } = Data.get(id); | ||
| option.data.forEach(d => { | ||
| const l = chart.legend.legendItems.find(i => i.text === d.label); | ||
| if(l) { |
There was a problem hiding this comment.
Missing space after 'if' keyword. JavaScript convention and most style guides recommend a space between 'if' and the opening parenthesis for better readability.
Suggested change
| if(l) { | |
| if (l) { |
Comment on lines
+494
to
+499
| option.data.forEach(d => { | ||
| const l = chart.legend.legendItems.find(i => i.text === d.label); | ||
| if(l) { | ||
| d.hidden = l.hidden; | ||
| } | ||
| }); |
There was a problem hiding this comment.
The code assumes option.data is iterable but doesn't check for null/undefined. If option.data is null or undefined, this will throw a runtime error. Add a defensive check to ensure option.data exists before calling forEach.
Suggested change
| option.data.forEach(d => { | |
| const l = chart.legend.legendItems.find(i => i.text === d.label); | |
| if(l) { | |
| d.hidden = l.hidden; | |
| } | |
| }); | |
| if (Array.isArray(option.data)) { | |
| option.data.forEach(d => { | |
| const l = chart.legend.legendItems.find(i => i.text === d.label); | |
| if (l) { | |
| d.hidden = l.hidden; | |
| } | |
| }); | |
| } |
Comment on lines
+494
to
+496
| option.data.forEach(d => { | ||
| const l = chart.legend.legendItems.find(i => i.text === d.label); | ||
| if(l) { |
There was a problem hiding this comment.
The nested iteration pattern has O(n*m) time complexity where n is the number of data items and m is the number of legend items. For large datasets, consider creating a Map from legendItems indexed by text before the forEach loop to improve performance to O(n+m).
Suggested change
| option.data.forEach(d => { | |
| const l = chart.legend.legendItems.find(i => i.text === d.label); | |
| if(l) { | |
| const legendItems = chart && chart.legend && Array.isArray(chart.legend.legendItems) | |
| ? chart.legend.legendItems | |
| : []; | |
| const legendItemMap = new Map(); | |
| legendItems.forEach(item => { | |
| legendItemMap.set(item.text, item); | |
| }); | |
| option.data.forEach(d => { | |
| const l = legendItemMap.get(d.label); | |
| if (l) { |
Comment on lines
+494
to
+499
| option.data.forEach(d => { | ||
| const l = chart.legend.legendItems.find(i => i.text === d.label); | ||
| if(l) { | ||
| d.hidden = l.hidden; | ||
| } | ||
| }); |
There was a problem hiding this comment.
The code lacks proper null/undefined safety checks. If chart.legend is null/undefined or chart.legend.legendItems is null/undefined, this code will throw a runtime error. This can happen when the legend is disabled (showLegend: false) or in certain chart configurations. Add defensive checks to ensure these properties exist before attempting to access them.
Suggested change
| option.data.forEach(d => { | |
| const l = chart.legend.legendItems.find(i => i.text === d.label); | |
| if(l) { | |
| d.hidden = l.hidden; | |
| } | |
| }); | |
| const legendItems = chart && chart.legend && chart.legend.legendItems; | |
| if (Array.isArray(option.data) && Array.isArray(legendItems)) { | |
| option.data.forEach(d => { | |
| const l = legendItems.find(i => i.text === d.label); | |
| if (l) { | |
| d.hidden = l.hidden; | |
| } | |
| }); | |
| } |
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.
Link issues
fixes #877
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enhancements: