bug(DockView): fix title incorrect when switch lang#955
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFixes DockView panel deletion logic when toggling components to ensure the correct panel instance is removed, which resolves incorrect titles after language switches. Sequence diagram for DockView toggleComponent panel deletion fixsequenceDiagram
actor User
participant DockViewUI
participant dockview_utils_js
participant dockview_group_js
participant DockviewInstance
User->>DockViewUI: Switch language / toggle component
DockViewUI->>dockview_utils_js: toggleComponent(dockview, options)
dockview_utils_js->>DockviewInstance: getPanels()
DockviewInstance-->>dockview_utils_js: panels
loop For each panel p in panels
dockview_utils_js->>dockview_group_js: addGroupWithPanel(dockview, p_or_panel, localPanel, panels, index)
alt Panel has groupId
dockview_group_js->>DockviewInstance: addPanelWidthGroupId(dockview, panel, index)
else Panel has no groupId
dockview_group_js->>DockviewInstance: addPanelWidthCreatGroup(dockview, panel, panels)
end
note over dockview_group_js,DockviewInstance: Corrected behavior: deletePanel now uses localPanel
dockview_group_js->>DockviewInstance: deletePanel(dockview, localPanel)
end
DockviewInstance-->>DockViewUI: Panels updated with correct titles after lang switch
File-Level Changes
Assessment against linked issues
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 change to
addGroupWithPanelnow preferspoverpanel(p || panelinstead ofpanel || p); if both can be defined this may alter behavior in subtle ways and is worth confirming aligns with the intended source of truth. - Since
localPanelcan now beundefinedwhen passed intodeletePanel, consider adding a guard or clarifying the expected invariants to make the deletion logic more robust and self-documenting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change to `addGroupWithPanel` now prefers `p` over `panel` (`p || panel` instead of `panel || p`); if both can be defined this may alter behavior in subtle ways and is worth confirming aligns with the intended source of truth.
- Since `localPanel` can now be `undefined` when passed into `deletePanel`, consider adding a guard or clarifying the expected invariants to make the deletion logic more robust and self-documenting.
## Individual Comments
### Comment 1
<location path="src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-group.js" line_range="29-36" />
<code_context>
}
-const addGroupWithPanel = (dockview, panel, panels, index) => {
+const addGroupWithPanel = (dockview, panel, localPanel, panels, index) => {
if (panel.groupId) {
addPanelWidthGroupId(dockview, panel, index)
}
else {
addPanelWidthCreatGroup(dockview, panel, panels)
}
- deletePanel(dockview, panel)
+ deletePanel(dockview, localPanel)
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `localPanel` being undefined before calling `deletePanel`.
With the new signature, `localPanel` may be `undefined` when `panels.find(...)` in the caller fails, leading to `deletePanel(dockview, undefined)`. Please either guard before calling (e.g. `if (localPanel) deletePanel(...)`) or ensure the caller always provides a valid panel reference.
</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
+29
to
+36
| const addGroupWithPanel = (dockview, panel, localPanel, panels, index) => { | ||
| if (panel.groupId) { | ||
| addPanelWidthGroupId(dockview, panel, index) | ||
| } | ||
| else { | ||
| addPanelWidthCreatGroup(dockview, panel, panels) | ||
| } | ||
| deletePanel(dockview, panel) | ||
| deletePanel(dockview, localPanel) |
There was a problem hiding this comment.
issue (bug_risk): Guard against localPanel being undefined before calling deletePanel.
With the new signature, localPanel may be undefined when panels.find(...) in the caller fails, leading to deletePanel(dockview, undefined). Please either guard before calling (e.g. if (localPanel) deletePanel(...)) or ensure the caller always provides a valid panel reference.
There was a problem hiding this comment.
Pull request overview
Fixes an issue in the DockView JavaScript integration where panel titles could remain incorrect after switching languages, by preferring the latest panel definition from options when re-adding panels and cleaning up stale stored panel metadata.
Changes:
- Update
toggleComponentto re-add panels using the current option panel (with updated title) instead of a cached/stored panel instance. - Adjust
addGroupWithPanelto accept both the panel-to-add and the stored panel instance to delete fromdockview.params.panels. - Bump
BootstrapBlazor.DockViewpackage version to10.0.4-beta01.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-utils.js | Prefer option-derived panel when adding a panel during toggle (helps ensure updated titles are used). |
| src/components/BootstrapBlazor.DockView/wwwroot/js/dockview-group.js | Extend addGroupWithPanel to delete the correct stored panel instance after re-adding. |
| src/components/BootstrapBlazor.DockView/BootstrapBlazor.DockView.csproj | Update package version for the DockView component. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| <PropertyGroup> | ||
| <Version>10.0.3</Version> | ||
| <Version>10.0.4-beta01</Version> |
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 #954
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Bug Fixes: