Skip to content

Fix/issues#2151 #2339

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.

Sign up for GitHub

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

Open
Wang-Yi-Teng wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix/issues#2151 #2339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8bc0b84
fix: 修复小地图在vue自定义节点删除一个小地图中全部vue节点消失问题
Sep 15, 2025
eed6773
Merge branch 'didi:master' into master
Wang-Yi-Teng Oct 16, 2025
ac8dbba
Merge branch 'didi:master' into master
Wang-Yi-Teng Oct 28, 2025
902284a
fix: 修复extension包引入vue包问题
Oct 30, 2025
15e51a5
Merge branch 'didi:master' into master
Wang-Yi-Teng Dec 17, 2025
9f43069
fix: (GraphModel)添加DOM存在性和可见性检查防止resize错误
Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions packages/core/src/model/GraphModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ export class GraphModel {
((entries) => {
for (const entry of entries) {
if (entry.target === this.rootEl) {
// 检查元素是否仍在DOM中
const isElementInDOM = document.body.contains(this.rootEl)
if (!isElementInDOM) return
Comment on lines +198 to +200
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The ResizeObserver callback has duplicate DOM existence check logic. The same check document.body.contains(this.rootEl) is performed both in the ResizeObserver callback (lines 199-200) and at the start of the resize method (lines 1599-1600). This creates redundant validation. Consider removing the check from the callback since the resize method will handle it, or document why both checks are necessary.

Suggested change
// 检查元素是否仍在DOM中
const isElementInDOM = document.body.contains(this.rootEl)
if (!isElementInDOM) return

Copilot uses AI. Check for mistakes.
this.resize()
this.eventCenter.emit('graph:resize', {
target: this.rootEl,
Expand Down Expand Up @@ -1589,15 +1592,32 @@ export class GraphModel {
* 重新设置画布的宽高
*/
@action resize(width?: number, height?: number): void {
this.width = width ?? this.rootEl.getBoundingClientRect().width
this.isContainerWidth = isNil(width)
this.height = height ?? this.rootEl.getBoundingClientRect().height
this.isContainerHeight = isNil(height)

if (!this.width || !this.height) {
console.warn(
'渲染画布的时候无法获取画布宽高，请确认在container已挂载到DOM。@see https://github.com/didi/LogicFlow/issues/675',
)
// 检查当前实例是否已被销毁或rootEl不存在
if (!this.rootEl) return

// 检查元素是否仍在DOM中
const isElementInDOM = document.body.contains(this.rootEl)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The check document.body.contains(this.rootEl) may not be comprehensive enough. If this.rootEl is in a shadow DOM or a detached fragment that's not directly under document.body, this check would incorrectly return false even though the element might still be in a valid DOM tree. Consider using this.rootEl.isConnected instead, which is a standard DOM property that correctly identifies whether an element is connected to the document, regardless of its position in the DOM tree.

Suggested change
const isElementInDOM = document.body.contains(this.rootEl)
const isElementInDOM = this.rootEl.isConnected

Copilot uses AI. Check for mistakes.
if (!isElementInDOM) return

// 检查元素是否可见
const isVisible = this.rootEl.offsetParent !== null
Comment on lines +1602 to +1603
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The visibility check using offsetParent !== null will return false for elements with position: fixed, even when they are visible on the screen. This could prevent legitimate resize operations from executing when the canvas uses fixed positioning. Consider using getComputedStyle(this.rootEl).display !== 'none' or checking both offsetParent and the element's fixed positioning status to handle this edge case.

Suggested change
// 检查元素是否可见
const isVisible = this.rootEl.offsetParent !== null
// 检查元素是否可见：兼容 position: fixed 的情况
const computedStyle = window.getComputedStyle(this.rootEl)
const isVisible =
computedStyle.display !== 'none' &&
computedStyle.visibility !== 'hidden' &&
(this.rootEl.offsetParent !== null || computedStyle.position === 'fixed')

Copilot uses AI. Check for mistakes.
if (!isVisible) return

try {
this.width = width ?? this.rootEl.getBoundingClientRect().width
this.isContainerWidth = isNil(width)
this.height = height ?? this.rootEl.getBoundingClientRect().height
this.isContainerHeight = isNil(height)

// 只有在元素可见且应该有宽高的情况下才显示警告
if (isVisible && (!this.width || !this.height)) {
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The condition if (isVisible && (!this.width || !this.height)) is redundant because the method already returns early when isVisible is false (line 1604). Since this code is only reached when isVisible is true, the check can be simplified to just if (!this.width || !this.height).

Suggested change
if (isVisible && (!this.width || !this.height)) {
if (!this.width || !this.height) {

Copilot uses AI. Check for mistakes.
Comment on lines +1602 to +1613
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The visibility check using offsetParent !== null has limitations. It returns null for elements with position: fixed, even though they may be visible. Additionally, this check causes an early return that prevents the resize method from updating width and height when the element is temporarily hidden (e.g., in a hidden tab or collapsed panel), which may not be the intended behavior. Consider if resizing should still update internal state even when the element is not visible, or use a more robust visibility check that accounts for different CSS positioning.

Suggested change
// 检查元素是否可见
const isVisible = this.rootEl.offsetParent !== null
if (!isVisible) return
try {
this.width = width ?? this.rootEl.getBoundingClientRect().width
this.isContainerWidth = isNil(width)
this.height = height ?? this.rootEl.getBoundingClientRect().height
this.isContainerHeight = isNil(height)
// 只有在元素可见且应该有宽高的情况下才显示警告
if (isVisible && (!this.width || !this.height)) {
try {
this.width = width ?? this.rootEl.getBoundingClientRect().width
this.isContainerWidth = isNil(width)
this.height = height ?? this.rootEl.getBoundingClientRect().height
this.isContainerHeight = isNil(height)
// 当无法获取画布宽高时显示警告
if (!this.width || !this.height) {

Copilot uses AI. Check for mistakes.
console.warn(
'渲染画布的时候无法获取画布宽高，请确认在container已挂载到DOM。@see https://github.com/didi/LogicFlow/issues/675',
)
}
} catch (error) {
// 捕获可能的DOM操作错误
console.warn('获取画布宽高时发生错误:', error)
Comment on lines +1619 to +1620
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The error message is too generic and doesn't provide actionable information. It only states that an error occurred but doesn't explain what specific operation failed or what the developer should do to resolve it. Consider providing more specific information about what DOM operation failed and potential causes.

Suggested change
// 捕获可能的DOM操作错误
console.warn('获取画布宽高时发生错误:', error)
// 捕获可能的DOM操作错误，并提供更详细的上下文信息
const rootElInfo = this.rootEl
? `<${this.rootEl.tagName.toLowerCase()} id="${this.rootEl.id}"> (inDOM=${document.body.contains(
this.rootEl,
)}, visible=${this.rootEl.offsetParent !== null})`
: 'rootEl is null/undefined'
console.warn(
'获取画布宽高时发生错误：无法通过 container.getBoundingClientRect() 计算画布尺寸。' +
' 这通常意味着 container 尚未挂载到 DOM、被 display:none 隐藏，或其尺寸为 0。' +
` container: ${rootElInfo}。原始错误:`,
error,
)

Copilot uses AI. Check for mistakes.
}
Comment on lines +1606 to 1621
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The try-catch block catches all errors but doesn't handle them meaningfully. The catch block only logs a warning, which means the method will silently fail to update width and height if getBoundingClientRect() throws an error. This could leave the component in an inconsistent state. Consider either re-throwing critical errors or implementing proper error recovery logic.

Copilot uses AI. Check for mistakes.
Comment on lines +1595 to 1621
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The new safety checks in the resize method (DOM existence check, visibility check, and error handling) lack test coverage. Consider adding tests that verify the behavior when: 1) rootEl is detached from DOM, 2) rootEl is hidden (offsetParent is null), and 3) getBoundingClientRect() operations might fail. This will ensure these defensive checks work as intended and prevent regressions.

Copilot uses AI. Check for mistakes.
}

Expand Down
52 changes: 52 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.