Fix/issues#2151 #2339
Fix/issues#2151 #2339
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #2151 by adding defensive checks to prevent errors when the canvas element is detached from the DOM or in an invalid state. The changes focus on improving the robustness of the resize operation in GraphModel.
- Adds DOM existence and visibility checks before performing resize operations
- Wraps getBoundingClientRect() calls in try-catch to handle potential DOM errors
- Updates pnpm-lock.yaml to add libc specifications for Linux platform-specific packages
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds libc field specifications (glibc/musl) for Linux platform-specific package entries to improve package resolution |
| packages/core/src/model/GraphModel.ts | Implements safety checks in resize method and ResizeObserver callback to prevent errors when DOM elements are detached or invalid |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.isContainerHeight = isNil(height) | ||
|
|
||
| // 只有在元素可见且应该有宽高的情况下才显示警告 | ||
| if (isVisible && (!this.width || !this.height)) { |
There was a problem hiding this comment.
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).
| if (isVisible && (!this.width || !this.height)) { | |
| if (!this.width || !this.height) { |
| // 捕获可能的DOM操作错误 | ||
| console.warn('获取画布宽高时发生错误:', error) |
There was a problem hiding this comment.
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.
| // 捕获可能的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, | |
| ) |
| // 检查元素是否仍在DOM中 | ||
| const isElementInDOM = document.body.contains(this.rootEl) | ||
| if (!isElementInDOM) return |
There was a problem hiding this comment.
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.
| // 检查元素是否仍在DOM中 | |
| const isElementInDOM = document.body.contains(this.rootEl) | |
| if (!isElementInDOM) 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)) { | ||
| console.warn( | ||
| '渲染画布的时候无法获取画布宽高，请确认在container已挂载到DOM。@see https://github.com/didi/LogicFlow/issues/675', | ||
| ) | ||
| } | ||
| } catch (error) { | ||
| // 捕获可能的DOM操作错误 | ||
| console.warn('获取画布宽高时发生错误:', error) | ||
| } |
There was a problem hiding this comment.
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.
| // 检查当前实例是否已被销毁或rootEl不存在 | ||
| if (!this.rootEl) return | ||
|
|
||
| // 检查元素是否仍在DOM中 | ||
| const isElementInDOM = document.body.contains(this.rootEl) | ||
| if (!isElementInDOM) return | ||
|
|
||
| // 检查元素是否可见 | ||
| 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)) { | ||
| console.warn( | ||
| '渲染画布的时候无法获取画布宽高，请确认在container已挂载到DOM。@see https://github.com/didi/LogicFlow/issues/675', | ||
| ) | ||
| } | ||
| } catch (error) { | ||
| // 捕获可能的DOM操作错误 | ||
| console.warn('获取画布宽高时发生错误:', error) | ||
| } |
There was a problem hiding this comment.
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.
| // 检查元素是否可见 | ||
| 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)) { |
There was a problem hiding this comment.
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.
| // 检查元素是否可见 | |
| 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) { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!this.rootEl) return | ||
|
|
||
| // 检查元素是否仍在DOM中 | ||
| const isElementInDOM = document.body.contains(this.rootEl) |
There was a problem hiding this comment.
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.
| const isElementInDOM = document.body.contains(this.rootEl) | |
| const isElementInDOM = this.rootEl.isConnected |
| // 检查元素是否可见 | ||
| const isVisible = this.rootEl.offsetParent !== null |
There was a problem hiding this comment.
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.
| // 检查元素是否可见 | |
| 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') |
1 participant
#2151