feat(DockView): add onPanelVisibleChanged event#981
Merged
Conversation
Reviewer's GuideAdds a new panel visibility change handling flow for DockView, refactors panel lifecycle logic to be driven by visibility instead of active state, hardens dockview initialization and disposal handling, and adjusts panel toggling to merge options safely without overwriting existing values with undefined. Sequence diagram for DockView panel visibility change handlingsequenceDiagram
participant Panel
participant PanelApi as PanelApi
participant Group
participant Dockview
participant LoadTabsEvent as LoadTabs
Panel->>PanelApi: onDidVisibilityChange(handler)
Note over PanelApi,Dockview: Visibility change event fires
PanelApi->>Dockview: handler({ isVisible })
Dockview->>Dockview: if _isDisposed return
Dockview->>Dockview: renderer = params.options.renderer
alt renderer is onlyWhenVisible and _inited
alt isVisible
Dockview->>Dockview: saveConfig(accessor)
Dockview->>Dockview: visiblePanels = groups.map(...)
Dockview->>LoadTabs: _loadTabs.fire(visiblePanelKeys)
else not isVisible
Dockview->>Panel: appendTemplatePanelEle(panel)
end
end
alt isVisible and group.floatType is drawer
Dockview->>Group: setDrawerTitle(group)
end
Dockview->>Dockview: handler = setTimeout(..., 0)
Dockview->>Dockview: clearTimeout(handler)
Dockview->>Dockview: if _isDisposed return
Dockview->>Panel: moveAlwaysRenderPanel(panel)
State diagram for Dockview initialization and disposalstateDiagram-v2
[*] --> Constructed
Constructed --> InitializingTimeoutPending: initDockview called
state InitializingTimeoutPending {
[*] --> Waiting
Waiting --> FinishedInit: timeout handler executes
Waiting --> Aborted: dockview._isDisposed becomes true before handler
}
InitializingTimeoutPending --> Disposed: dockview._isDisposed set before handler
FinishedInit --> Inited: _inited = true and _initialized.fire()
Aborted --> Disposed: handler sees _isDisposed, sets dockview = null
Inited --> Disposed: later disposal sets _isDisposed = true
Disposed --> Disposed: Any visibility or init handler checks _isDisposed and returns
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 - I've found 1 issue, and left some high level feedback:
- The
cleanUndefinedhelper filters out bothnullandundefined(v != null), which changes semantics if callers intentionally passnullto clear a value; consider restricting this toundefinedonly (e.g.,v !== undefined) or documenting the null-stripping behavior explicitly. - In
initDockview, settingdockview = nullafter checkingdockview._isDisposedonly reassigns the local variable and has no effect on the actual instance; you can drop this assignment to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `cleanUndefined` helper filters out both `null` and `undefined` (`v != null`), which changes semantics if callers intentionally pass `null` to clear a value; consider restricting this to `undefined` only (e.g., `v !== undefined`) or documenting the null-stripping behavior explicitly.
- In `initDockview`, setting `dockview = null` after checking `dockview._isDisposed` only reassigns the local variable and has no effect on the actual instance; you can drop this assignment to avoid confusion.
## Individual Comments
### Comment 1
<location path="src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-utils.js" line_range="230-231" />
<code_context>
}
}
-
+const cleanUndefined = (obj) => Object.fromEntries(
+ Object.entries(obj).filter(([, v]) => v != null)
+);
const toggleComponent = (dockview, options) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Filtering with `v != null` drops both `undefined` and `null`, which changes semantics when `null` is intentional.
Using `v != null` here also strips intentional `null` values, so callers can’t explicitly reset a property to `null`. If `null` is meaningful and only `undefined` should be removed, use `v !== undefined` instead, or make this behavior explicit in the function’s name/contract.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+230
to
+231
| const cleanUndefined = (obj) => Object.fromEntries( | ||
| Object.entries(obj).filter(([, v]) => v != null) |
There was a problem hiding this comment.
issue (bug_risk): Filtering with v != null drops both undefined and null, which changes semantics when null is intentional.
Using v != null here also strips intentional null values, so callers can’t explicitly reset a property to null. If null is meaningful and only undefined should be removed, use v !== undefined instead, or make this behavior explicit in the function’s name/contract.
There was a problem hiding this comment.
Pull request overview
This PR enhances the DockView JavaScript integration to better react to panel visibility changes (supporting an onPanelVisibleChanged-style event flow) and hardens lifecycle behaviors to avoid running callbacks after a DockView instance is disposed.
Changes:
- Switch panel event handling to rely on visibility changes (and consolidate related side effects like template moves / tab loading).
- Add disposal guards and make some DOM event wiring more resilient.
- Adjust panel add/remove/update behaviors (key-based removal, merging option updates, adding panels as inactive) and bump DockView package version.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-utils.js | Adds disposed guard during init, adjusts init timing/click handler wiring, and introduces cleanUndefined for option merging. |
| src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-panel.js | Consolidates panel visibility handling, adjusts when config/tabs are updated, and fixes panel deletion to be key-based. |
| src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-group.js | Adds panels as inactive when inserting into existing/new groups. |
| src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-extensions.js | Removes debug logging and simplifies closePanel template move logic. |
| src/components/BootstrapBlazor.DockView/BootstrapBlazor.DockView.csproj | Updates package version from beta to stable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+140
to
145
| dockview.element.querySelector('&>.dv-dockview>.dv-branch-node')?.addEventListener('click', function(e) { | ||
| this.parentElement.querySelectorAll('&>.dv-resize-container-drawer, &>.dv-render-overlay-float-drawer')?.forEach(item => { | ||
| item.classList.remove('active') | ||
| }) | ||
| this.closest('.bb-dockview').querySelectorAll('&>.bb-dockview-aside>.bb-dockview-aside-button').forEach(item => { | ||
| this.closest('.bb-dockview').querySelectorAll('&>.bb-dockview-aside>.bb-dockview-aside-button')?.forEach(item => { | ||
| item.classList.remove('active') |
There was a problem hiding this comment.
The optional chaining here is applied to the result of querySelectorAll, but querySelectorAll always returns a (non-null) NodeList; the real null/undefined risks are this.parentElement and this.closest('.bb-dockview'). As written, this can still throw if parentElement or closest(...) is null. Consider optional-chaining those objects instead (and drop the redundant ?. on querySelectorAll).
| } | ||
|
|
||
| const cleanUndefined = (obj) => Object.fromEntries( | ||
| Object.entries(obj).filter(([, v]) => v != null) |
There was a problem hiding this comment.
cleanUndefined currently filters out both undefined and null values (v != null). If callers use null intentionally (e.g., to reset a param to its default), this merge will silently ignore that update and keep the previous value. If the intent is only to avoid overwriting with undefined, filter with v !== undefined (or rename the helper to reflect “nullish” behavior).
Suggested change
| Object.entries(obj).filter(([, v]) => v != null) | |
| Object.entries(obj).filter(([, v]) => v !== undefined) |
Comment on lines
+15
to
+21
| if (renderer === 'onlyWhenVisible' && dockview._inited) { | ||
| if (isVisible) { | ||
| saveConfig(panel.accessor) | ||
| const visiblePanels = dockview.groups.map(g => g.panels.find(p => p.params.isActive) || g.panels.find(p => p.api.isVisible)) | ||
| dockview._loadTabs?.fire(visiblePanels.filter(p => Boolean(p)).map(p => p.params.key)); | ||
| } | ||
| else { |
There was a problem hiding this comment.
saveConfig is now only triggered on visibility changes when renderer === 'onlyWhenVisible'. For renderer === 'partial' or 'always', switching tabs will still change which panel is active/visible, but config won’t be persisted to localStorage anymore (active panel can be lost on reload). Consider saving config on the relevant active/visibility transition regardless of renderer (keeping any maximized/initialization guards as needed).
Suggested change
| if (renderer === 'onlyWhenVisible' && dockview._inited) { | |
| if (isVisible) { | |
| saveConfig(panel.accessor) | |
| const visiblePanels = dockview.groups.map(g => g.panels.find(p => p.params.isActive) || g.panels.find(p => p.api.isVisible)) | |
| dockview._loadTabs?.fire(visiblePanels.filter(p => Boolean(p)).map(p => p.params.key)); | |
| } | |
| else { | |
| if (dockview._inited) { | |
| if (isVisible) { | |
| saveConfig(panel.accessor) | |
| const visiblePanels = dockview.groups.map(g => g.panels.find(p => p.params.isActive) || g.panels.find(p => p.api.isVisible)) | |
| dockview._loadTabs?.fire(visiblePanels.filter(p => Boolean(p)).map(p => p.params.key)); | |
| } | |
| else if (renderer === 'onlyWhenVisible') { |
Comment on lines
8
to
10
| observePanelActiveChange(panel) | ||
| } | ||
| const observePanelActiveChange = panel => { |
There was a problem hiding this comment.
observePanelActiveChange no longer observes “active” changes (it now subscribes to onDidVisibilityChange). Renaming this helper (and its call site) to reflect visibility semantics would make the code easier to follow and reduce confusion for future maintenance.
Suggested change
| observePanelActiveChange(panel) | |
| } | |
| const observePanelActiveChange = panel => { | |
| observePanelVisibilityChange(panel) | |
| } | |
| const observePanelVisibilityChange = panel => { |
|
|
||
| <PropertyGroup> | ||
| <Version>10.0.8-beta06</Version> | ||
| <Version>10.0.8</Version> |
There was a problem hiding this comment.
PR metadata is inconsistent (description says fixes #980, while the referenced PR text says fixes #979), and this diff also changes the package version from 10.0.8-beta06 to 10.0.8. If a stable release is intended, please align the issue reference(s) and call out the version bump explicitly; otherwise consider keeping the pre-release suffix for a feature PR.
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
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 #979
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Update DockView panel visibility handling and panel synchronization for more robust lifecycle management and state updates.
New Features:
Bug Fixes:
Enhancements: