Skip to content

[codex] Sync extension marks before hover frames render#4590

Draft
xuefei1313 wants to merge 1 commit into
developfrom
fix/extension-mark-sync-state-hover
Draft

[codex] Sync extension marks before hover frames render#4590
xuefei1313 wants to merge 1 commit into
developfrom
fix/extension-mark-sync-state-hover

Conversation

@xuefei1313
Copy link
Copy Markdown
Contributor

What changed

  • Move ExtensionMarkSyncStatePlugin state copying from the chart afterRender event to the VRender stage beforeRender hook.
  • Keep the chart afterRender subscription only to attach or rebind the stage hook after the stage is available or recreated.
  • Clean up both chart-event and stage-hook subscriptions on release or when syncState is no longer detected.

Why

syncState extension marks were copying the primary mark state after the frame had already rendered. For hover-driven outlines this made the outline appear one render late: hovering a bar did not show the outline, while pointerout could show the previous active state.

Copying states in beforeRender ensures extension marks receive the latest primary mark state before the current frame is drawn.

Validation

  • User confirmed the local runtime page interaction now behaves correctly.
  • Ran git diff --check.
  • Attempted rushx compile in packages/vchart-extension; it is currently blocked by existing unrelated @visactor/vchart export/type errors across the package, so it was not a valid signal for this change.

Extension mark syncState copied primary mark states in afterRender, so interactive state changes happened after the frame had already painted. Moving the copy to the stage beforeRender hook keeps hover-driven extension marks aligned with the primary mark in the same draw pass while afterRender only ensures the stage hook is attached or rebound.

Constraint: Hover-only bar outlines rely on extension marks syncing primary mark active state in the same frame
Rejected: Keep syncing in afterRender | state changes are visible one render late
Confidence: high
Scope-risk: narrow
Directive: Keep syncState state copying before drawing; do not move it back to afterRender without hover regression verification
Tested: Manual localhost page confirmation by user; git diff --check
Not-tested: Full package typecheck because existing unrelated @visactor/vchart export/type errors block rushx compile
Copy link
Copy Markdown
Contributor Author

@xuefei1313 xuefei1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦞 Aime Bot Review

改动摘要

本 PR 修复了 ExtensionMarkSyncStatePluginsyncState 扩展 mark 的状态同步时机问题。核心改动是将状态同步从 VChart 的 afterRender 事件移至 VRender stage 的 beforeRender hook,确保扩展 mark 在当前帧绘制就能获取到主 mark 的最新状态,从而修复 hover 轮廓延迟一帧显示的问题。

代码观察与建议

整体设计合理,通过引入 _unsubscribeBeforeRender_subscribedStage 两个私有字段,对 stage hook 的生命周期管理与已有的 event 订阅管理保持了一致的风格,值得肯定。

以下几点供参考:

  1. _ensureBeforeRenderSyncHook 中的 try/catchgetStage?.() 已使用可选链,通常不会抛出,但用 try/catch 兜底也无妨。可以考虑仅保留可选链而去掉 try/catch 以减少冗余,当然保留也不影响正确性。

  2. onAfterInitChart 中的提前清理

    if (!this._detectSyncState(chartSpec)) {
      this._unsubscribeAfterRender?.();
      this._unsubscribeBeforeRender?.();
      return;
    }

    这里在无 syncState 时主动取消订阅,逻辑是正确的。请确认是否存在 chart spec 动态变更(先有 syncState 后去除)的场景,若有,这段清理逻辑正好覆盖了该情况,👍。

  3. release() 中未清理 _subscribedEvent_subscribedStage:父类 super.release() 是否会处理这些引用?如果不会,建议在 release() 中手动将它们置为 undefined,避免潜在的内存泄漏。

  4. 文件末尾 patch 截断:diff 中 _detectSyncState 方法的检测逻辑被截断,从已有代码来看逻辑完整,问题不大。

合并建议

改动目标明确,修复逻辑清晰,代码整洁。建议关注第 3 条关于 release() 后引用清理的问题后,可以合并。感谢贡献! 🎉

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant