Skip to content

011 fix discrete legend filter after update spec#4567

Open
xuefei1313 wants to merge 2 commits intodevelopfrom
011-fix-discrete-legend-filter-after-update-spec
Open

011 fix discrete legend filter after update spec#4567
xuefei1313 wants to merge 2 commits intodevelopfrom
011-fix-discrete-legend-filter-after-update-spec

Conversation

@xuefei1313
Copy link
Copy Markdown
Contributor

@xuefei1313 xuefei1313 commented Apr 21, 2026

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Refactoring
  • Update dependency
  • Code style optimization
  • Test Case
  • Branch merge
  • Release
  • Site / documentation update
  • Demo update
  • Workflow
  • Other (about what?)

🔗 Related issue link

close #4566

🔗 Related PR link

🐞 Bugserver case id

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English
🇨🇳 Chinese

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

🚀 Summary

copilot:summary

🔍 Walkthrough

copilot:walkthrough
@xuefei1313
fix(vchart): preserve legend filtering after updateSpec recompiles
9fbcc32 
Discrete legends with custom data callbacks stopped filtering after
`updateSpec(..., { reMake: false })`.
`BaseLegend.clear()` released the vrender legend component but kept
its stale instance reference. The next compile pass reused that dead
instance and skipped the event-binding path that forwards legend
clicks into VChart selection and data filtering.

Null the cached legend component during cleanup, add a focused
regression test for the callback-backed legend update path, and
record the issue spec/plan/tasks alongside the code change.

Constraint: Preserve `reMake: false` update path and discrete legend filtering semantics
Rejected: Force `reMake` for updated legends | broader rebuild cost and masks lifecycle bug
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: If legend cleanup changes, verify recompile recreates vrender components
Tested: jest __tests__/unit/core/vchart-event.test.ts --runInBand (packages/vchart)
Tested: npm run compile (packages/vchart)
Not-tested: Manual browser reproduction against the original issue sandbox
Related: #4566
@xuefei1313
chore(vchart): record release note for legend updateSpec fix
6847417 
Add the required Rush changefile for the discrete legend filtering fix
so release automation can emit the patch note without manual
changelog reconstruction later.

Constraint: Submit a Rush change file with source changes in this repository
Rejected: Fold into previous commit | fix commit already passed hooks intact
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Keep changefile comments aligned with the shipped bug-fix description
Tested: Added JSON under common/changes/@visactor/vchart and staged it cleanly
Not-tested: Release workflow generation from this changefile
Related: #4566
@xuefei1313
Copy link
Copy Markdown
Contributor Author

xuefei1313 commented Apr 21, 2026

Summary

This looks like a solid, minimal fix for the regression in #4566. The root cause analysis makes sense: BaseComponent.clear() releases/removes the old VRender legend instance, but BaseLegend keeps _legendComponent populated, so the next layout pass reuses a released instance and (critically) skips the creation path where _initEvent() is bound.

Nulling _legendComponent in BaseLegend.clear() forces the getBoundsInRect() path to recreate the legend and rebind events, which should restore discrete-legend filtering after updateSpecSync(..., { reMake: false }).

What looks good

  • The code change is very small and scoped to lifecycle cleanup (packages/vchart/src/component/legend/base-legend.ts).
  • The regression test reproduces the exact flow (legends.data as a callback + updateSpecSync without remake) and asserts both legend selection state and filtered series view data.

Suggestions / potential follow-ups

  1. More defensive stale-instance handling: consider treating a legend instance without stage/parent as stale in getBoundsInRect() (e.g., if (this._legendComponent && !this._legendComponent.stage) this._legendComponent = null;). This makes the behavior robust even if the clear/recompile path changes in the future.
  2. Type correctness: _legendComponent is declared as IGroup but assigned null in several places (including this PR). If feasible, change it to IGroup | null to avoid hiding potential issues when strictNullChecks is enabled.
  3. Test brittleness (non-blocking): the new test reaches into private VRender fields (_itemsContainer, _onClick). If there is any existing helper/util to simulate legend clicks at the VChart layer, using that would reduce coupling to VRender internals.
  4. Repo hygiene (non-blocking):
    • common/changes/...fix-legend-filter-after-updatespec_20260421.json uses openai@example.com as email (looks like a placeholder).
    • specs/.../plan.md contains an absolute local path (/Users/bytedance/...), which is probably unintended for a public repo.

Suggested manual verification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore vchart

2 participants

@xuefei1313 @xile611