Conversation
|
Thanks for your PR, @zhaijunlei955. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Reviewer's GuideThis PR upgrades dockview-core to v4.4.0, introduces a new popout failure event, enhances popout handling and configuration persistence, adds a dropdown display observer for floating panels, reorders layout reload logic, and includes related CSS adjustments. Sequence diagram for popout window creation and failure handlingsequenceDiagram
participant User as actor User
participant DockviewComponent
participant PopoutWindow
participant Group
User->>DockviewComponent: Request to pop out a group
DockviewComponent->>PopoutWindow: Attempt to create popout window
alt Popout window creation fails
DockviewComponent->>DockviewComponent: Fire onDidOpenPopoutWindowFail
DockviewComponent->>Group: Move group back to reference group
DockviewComponent->>Group: Set group visible
else Popout window creation succeeds
DockviewComponent->>Group: Create floating group
end
Sequence diagram for group dropdown display observer in floating panelssequenceDiagram
participant Group
participant Icon
participant MutationObserver
Group->>Icon: Add dropdown icon
Icon->>MutationObserver: Observe class changes on dropdown menu
MutationObserver-->>Group: On 'show' class, move panel content to dv-content-container
MutationObserver-->>Group: On class removal, move content back to wrapper
Group->>MutationObserver: Disconnect on group dispose
Class diagram for DockviewComponent and new popout failure eventclassDiagram
class DockviewComponent {
+onDidOpenPopoutWindowFail
+_onDidOpenPopoutWindowFail
}
class DockviewApi {
+onDidOpenPopoutWindowFail
}
DockviewApi --> DockviewComponent : uses
DockviewComponent : +_onDidOpenPopoutWindowFail = new Emitter()
DockviewComponent : +onDidOpenPopoutWindowFail = this._onDidOpenPopoutWindowFail.event
DockviewApi : +get onDidOpenPopoutWindowFail()
Class diagram for group dropdown display observer logicclassDiagram
class Group {
+mutationObserver
+panels
+element
+activePanel
}
class observeDisplayChange {
+MutationObserver
}
Group --> observeDisplayChange : uses
observeDisplayChange : +observe(icon, group)
Group : +mutationObserver
Flow diagram for saveConfig floating group position persistenceflowchart TD
A[saveConfig called] --> B{floatingGroups exist?}
B -- No --> C[Save config to localStorage]
B -- Yes --> D[For each floatingGroup]
D --> E{fg.position.width > 0?}
E -- Yes --> F[Set panel.params.currentPosition.width]
E -- No --> G[Set fg.position.width from group.params.currentPosition.width or 500]
D --> H{fg.position.height > 0?}
H -- Yes --> I[Set panel.params.currentPosition.height]
H -- No --> J[Set fg.position.height from group.params.currentPosition.height or 350]
F & G & I & J --> C
File-Level Changes
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 @zhaijunlei955 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-group.js:190` </location>
<code_context>
}
+const observeDisplayChange = (icon, group) => {
+ const dockview = group.api.accessor
+ const element = icon.querySelector('.dropdown-menu')
+ const mutationObserver = new MutationObserver((mutations) => {
+ mutations.forEach(mutation => {
</code_context>
<issue_to_address>
Potential null reference if '.dropdown-menu' is not found.
Add a null check for 'element' before creating the MutationObserver to prevent runtime errors.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-group.js:202` </location>
<code_context>
+ childEle.wrapperEle = currentPanelEle
+ }
+ else {
+ const panelEleList = [...group.element.querySelector('&>.dv-content-container').children].map(item => {
+ const wrapperEle = item.wrapperEle
+ delete item.wrapperEle
</code_context>
<issue_to_address>
Repeated use of non-standard selector '&>.dv-content-container'.
This selector may not function as expected, which could cause 'panelEleList' to be empty or undefined and lead to errors.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-group.js:196` </location>
<code_context>
+ if (mutation.attributeName == 'class') {
+ if(mutation.target.classList.contains('show')) {
+ const currentPanelEle = group.activePanel.view.content.element.parentElement
+ const childEle = currentPanelEle.children[0]
+ group.element.querySelector('&>.dv-content-container').append(childEle)
+ currentPanelEle.style.zIndex = -1
</code_context>
<issue_to_address>
No check for existence of 'currentPanelEle' or its children.
Accessing properties of a null 'currentPanelEle' or missing children will cause a runtime error. Please add null and existence checks before using these elements.
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-group.js:199` </location>
<code_context>
+ const childEle = currentPanelEle.children[0]
+ group.element.querySelector('&>.dv-content-container').append(childEle)
+ currentPanelEle.style.zIndex = -1
+ childEle.wrapperEle = currentPanelEle
+ }
+ else {
</code_context>
<issue_to_address>
Attaching custom properties to DOM elements can cause memory leaks.
Instead of assigning 'wrapperEle' directly to the DOM node, use a WeakMap or similar structure to store metadata and avoid potential memory leaks.
</issue_to_address>
### Comment 5
<location> `src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-group.js:213` </location>
<code_context>
+ }
+ })
+ });
+ group.mutationObserver = mutationObserver
+ mutationObserver.observe(element, {
+ attributes: true,
</code_context>
<issue_to_address>
Potential for multiple observers if 'observeDisplayChange' is called multiple times.
Disconnect any existing observer before assigning a new one to prevent resource leaks.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
group.mutationObserver = mutationObserver
mutationObserver.observe(element, {
attributes: true,
attributeFilter: ["class"],
});
}
=======
if (group.mutationObserver) {
group.mutationObserver.disconnect();
}
group.mutationObserver = mutationObserver
mutationObserver.observe(element, {
attributes: true,
attributeFilter: ["class"],
});
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-config.js:321` </location>
<code_context>
- fg.position.width = fg.position.width || (width ? width + 2 : 300)
- fg.position.height = fg.position.height || dockview.floatingGroups[index].group.height
+ const group = dockview.floatingGroups[index].group
+ if (fg.position.width > 0) {
+ group.panels.forEach(panel => {
+ !panel.params.currentPosition && (panel.params.currentPosition = {})
+ panel.params.currentPosition.width = fg.position.width
+ })
+ }
+ else {
+ fg.position.width = group.params.currentPosition.width || 500
+ }
+ if (fg.position.height > 0) {
</code_context>
<issue_to_address>
Possible undefined 'group.params.currentPosition' access.
Accessing 'width' or 'height' on an undefined 'currentPosition' will cause a runtime error. Please add a check to ensure 'currentPosition' is defined before accessing its properties.
</issue_to_address>
### Comment 7
<location> `src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-group.js:188` </location>
<code_context>
}, 0)
addActionEvent(group, actionContainer);
}
+const observeDisplayChange = (icon, group) => {
+ const dockview = group.api.accessor
+ const element = icon.querySelector('.dropdown-menu')
</code_context>
<issue_to_address>
Consider replacing the MutationObserver logic with Bootstrap dropdown event listeners and extracting the panel move logic into two helper functions.
```markdown
You can simplify/remove the MutationObserver boilerplate by listening to the dropdown’s built-in events (e.g. Bootstrap’s `show.bs.dropdown`/`hide.bs.dropdown`) and extracting the “portal” logic into two small helpers. This keeps the same functionality but is far more declarative and easier to follow.
```js
// 1) In createGroupActions, replace observeDisplayChange(…) with two event listeners:
if (icon.classList.contains('bb-dockview-control-icon-dropdown')) {
// bootstrap dropdown events
icon.addEventListener('show.bs.dropdown', () => portalPanelIntoDropdown(group));
icon.addEventListener('hide.bs.dropdown', () => restorePanelFromDropdown(group));
}
```
```js
// 2) Move all the move/unwrap logic into two focused functions:
function portalPanelIntoDropdown(group) {
const panelEle = group.activePanel.view.content.element;
const wrapper = panelEle.parentElement;
const container = group.element.querySelector(':scope > .dv-content-container');
// “portal” the panel into dropdown
wrapper.style.zIndex = '-1';
panelEle.__wrapper = wrapper; // temp link back to wrapper
container.appendChild(panelEle);
}
function restorePanelFromDropdown(group) {
const container = group.element.querySelector(':scope > .dv-content-container');
Array.from(container.children).forEach(child => {
const wrapper = child.__wrapper;
delete child.__wrapper;
// move child back into its original wrapper, reset z-index
wrapper.style.zIndex = '';
wrapper.appendChild(child);
});
// re-attach wrappers back into the dockview DOM
const panels = Array.from(group.element.querySelector(':scope > .dv-content-container').children)
.map(c => c.__wrapper);
group.element.parentElement.parentElement.append(...panels);
}
```
Benefits:
- No manual `MutationObserver` or `setTimeout`
- Clear semantic hooks (`show.bs.dropdown` / `hide.bs.dropdown`)
- Portal logic lives in two tiny, testable functions
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| const observeDisplayChange = (icon, group) => { | ||
| const dockview = group.api.accessor | ||
| const element = icon.querySelector('.dropdown-menu') |
There was a problem hiding this comment.
issue (bug_risk): Potential null reference if '.dropdown-menu' is not found.
Add a null check for 'element' before creating the MutationObserver to prevent runtime errors.
| childEle.wrapperEle = currentPanelEle | ||
| } | ||
| else { | ||
| const panelEleList = [...group.element.querySelector('&>.dv-content-container').children].map(item => { |
There was a problem hiding this comment.
issue (bug_risk): Repeated use of non-standard selector '&>.dv-content-container'.
This selector may not function as expected, which could cause 'panelEleList' to be empty or undefined and lead to errors.
| if (mutation.attributeName == 'class') { | ||
| if(mutation.target.classList.contains('show')) { | ||
| const currentPanelEle = group.activePanel.view.content.element.parentElement | ||
| const childEle = currentPanelEle.children[0] |
There was a problem hiding this comment.
issue (bug_risk): No check for existence of 'currentPanelEle' or its children.
Accessing properties of a null 'currentPanelEle' or missing children will cause a runtime error. Please add null and existence checks before using these elements.
| const childEle = currentPanelEle.children[0] | ||
| group.element.querySelector('&>.dv-content-container').append(childEle) | ||
| currentPanelEle.style.zIndex = -1 | ||
| childEle.wrapperEle = currentPanelEle |
There was a problem hiding this comment.
suggestion (bug_risk): Attaching custom properties to DOM elements can cause memory leaks.
Instead of assigning 'wrapperEle' directly to the DOM node, use a WeakMap or similar structure to store metadata and avoid potential memory leaks.
Comment on lines
+213
to
+218
| group.mutationObserver = mutationObserver | ||
| mutationObserver.observe(element, { | ||
| attributes: true, | ||
| attributeFilter: ["class"], | ||
| }); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Potential for multiple observers if 'observeDisplayChange' is called multiple times.
Disconnect any existing observer before assigning a new one to prevent resource leaks.
Suggested change
| group.mutationObserver = mutationObserver | |
| mutationObserver.observe(element, { | |
| attributes: true, | |
| attributeFilter: ["class"], | |
| }); | |
| } | |
| if (group.mutationObserver) { | |
| group.mutationObserver.disconnect(); | |
| } | |
| group.mutationObserver = mutationObserver | |
| mutationObserver.observe(element, { | |
| attributes: true, | |
| attributeFilter: ["class"], | |
| }); | |
| } |
Comment on lines
+321
to
+328
| if (fg.position.width > 0) { | ||
| group.panels.forEach(panel => { | ||
| !panel.params.currentPosition && (panel.params.currentPosition = {}) | ||
| panel.params.currentPosition.width = fg.position.width | ||
| }) | ||
| } | ||
| else { | ||
| fg.position.width = group.params.currentPosition.width || 500 |
There was a problem hiding this comment.
issue (bug_risk): Possible undefined 'group.params.currentPosition' access.
Accessing 'width' or 'height' on an undefined 'currentPosition' will cause a runtime error. Please add a check to ensure 'currentPosition' is defined before accessing its properties.
| }, 0) | ||
| addActionEvent(group, actionContainer); | ||
| } | ||
| const observeDisplayChange = (icon, group) => { |
There was a problem hiding this comment.
issue (complexity): Consider replacing the MutationObserver logic with Bootstrap dropdown event listeners and extracting the panel move logic into two helper functions.
You can simplify/remove the MutationObserver boilerplate by listening to the dropdown’s built-in events (e.g. Bootstrap’s `show.bs.dropdown`/`hide.bs.dropdown`) and extracting the “portal” logic into two small helpers. This keeps the same functionality but is far more declarative and easier to follow.
```js
// 1) In createGroupActions, replace observeDisplayChange(…) with two event listeners:
if (icon.classList.contains('bb-dockview-control-icon-dropdown')) {
// bootstrap dropdown events
icon.addEventListener('show.bs.dropdown', () => portalPanelIntoDropdown(group));
icon.addEventListener('hide.bs.dropdown', () => restorePanelFromDropdown(group));
}// 2) Move all the move/unwrap logic into two focused functions:
function portalPanelIntoDropdown(group) {
const panelEle = group.activePanel.view.content.element;
const wrapper = panelEle.parentElement;
const container = group.element.querySelector(':scope > .dv-content-container');
// “portal” the panel into dropdown
wrapper.style.zIndex = '-1';
panelEle.__wrapper = wrapper; // temp link back to wrapper
container.appendChild(panelEle);
}
function restorePanelFromDropdown(group) {
const container = group.element.querySelector(':scope > .dv-content-container');
Array.from(container.children).forEach(child => {
const wrapper = child.__wrapper;
delete child.__wrapper;
// move child back into its original wrapper, reset z-index
wrapper.style.zIndex = '';
wrapper.appendChild(child);
});
// re-attach wrappers back into the dockview DOM
const panels = Array.from(group.element.querySelector(':scope > .dv-content-container').children)
.map(c => c.__wrapper);
group.element.parentElement.parentElement.append(...panels);
}Benefits:
- No manual
MutationObserverorsetTimeout - Clear semantic hooks (
show.bs.dropdown/hide.bs.dropdown) - Portal logic lives in two tiny, testable functions
| const width = dockview.floatingGroups[index].group.width | ||
| fg.position.width = fg.position.width || (width ? width + 2 : 300) | ||
| fg.position.height = fg.position.height || dockview.floatingGroups[index].group.height | ||
| const group = dockview.floatingGroups[index].group |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const group = dockview.floatingGroups[index].group | |
| const {group} = dockview.floatingGroups[index] |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
| } | ||
| else { | ||
| const panelEleList = [...group.element.querySelector('&>.dv-content-container').children].map(item => { | ||
| const wrapperEle = item.wrapperEle |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const wrapperEle = item.wrapperEle | |
| const {wrapperEle} = item |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
ArgoZhang
previously approved these changes
Jul 6, 2025
Co-Authored-By: chengKun <49547008+zhaijunlei955@users.noreply.github.com>
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
2 participants
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 #482
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Improve pop-out window handling by adding failure events and automatic repositioning, enhance floating group persistence and dropdown rendering, bump version, and clean up observers and configuration reload ordering.
New Features:
Bug Fixes:
Enhancements:
Chores: