Skip to content

Feat/clean remove panel #6056

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
theoc0702 wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Feat/clean remove panel #6056

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d89a27e
fgh
Nov 5, 2025
c622b95
doc
Nov 24, 2025
3207ce1
Merge branch 'master' into theo
Nov 24, 2025
6d6998d
Merge pull request #1 from theoc0702/master
theoc0702 Dec 10, 2025
abdc962
update config
Dec 22, 2025
8da5807
Merge pull request #2 from theoc0702/master
theoc0702 Dec 22, 2025
25c8bd0
Merge pull request #3 from theoc0702/master
theoc0702 Jan 13, 2026
4ba15c1
Merge pull request #4 from theoc0702/master
theoc0702 Jan 14, 2026
5af9bef
cleaning
Jan 14, 2026
08f24f1
Merge pull request #5 from theoc0702/master
theoc0702 Feb 11, 2026
87bde30
fix
Feb 20, 2026
f659d32
Merge pull request #6 from theoc0702/master
theoc0702 Feb 20, 2026
6a0f626
Merge pull request #7 from theoc0702/master
theoc0702 Apr 17, 2026
5731401
feat(core): add removePanel method to PanelService
Jun 3, 2026
1bcb167
chore: revert custom pixilib paths from webpack config
theoc0702 Jun 25, 2026
5aa847e
chore: revert local pixilib configurations
theoc0702 Jun 26, 2026
3d781cc
fix(PanelService): target panel reset to only clear updated positions
Jul 1, 2026
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
3 changes: 2 additions & 1 deletion .webpack/webpack.base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ module.exports = (env, argv, { SRC_DIR, ENTRY }) => {

const config = {
mode: isProdBuild ? 'production' : 'development',
devtool: isProdBuild ? 'source-map' : 'cheap-module-source-map',
devtool: isProdBuild ? false /*'source-map'*/ : 'cheap-module-source-map',

@greptile-apps greptile-apps Bot Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Disabling source maps for production builds (false) removes the ability to debug minified production errors. The commented-out value suggests this was a temporary local tweak that was accidentally committed. Restore the original value.

Suggested change
devtool: isProdBuild ? false /*'source-map'*/ : 'cheap-module-source-map',
devtool: isProdBuild ? 'source-map' : 'cheap-module-source-map',
Prompt To Fix With AI 
This is a comment left during a code review.
Path: .webpack/webpack.base.js
Line: 68

Comment:
Disabling source maps for production builds (`false`) removes the ability to debug minified production errors. The commented-out value suggests this was a temporary local tweak that was accidentally committed. Restore the original value.

```suggestion
    devtool: isProdBuild ? 'source-map' : 'cheap-module-source-map',
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

entry: ENTRY,
optimization: {
// splitChunks: {
Expand Down Expand Up @@ -210,6 +210,7 @@ module.exports = (env, argv, { SRC_DIR, ENTRY }) => {
path.resolve(__dirname, '../../../node_modules'),
path.resolve(__dirname, '../platform/app/node_modules'),
path.resolve(__dirname, '../platform/ui/node_modules'),
path.resolve(__dirname, '../../pixilib/node_modules'),
SRC_DIR,
],
// Attempt to resolve these extensions in order.
Expand Down
8 changes: 8 additions & 0 deletions platform/app/.webpack/webpack.pwa.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ module.exports = (env, argv) => {
// Hoisted Yarn Workspace Modules
path.resolve(__dirname, '../../../node_modules'),
SRC_DIR,
path.resolve(__dirname, '../pixilib/modes/gaelo-mode/node_modules'),

@wayfarer3130 wayfarer3130 Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a custom change for your paths/setup - please don't push changes like this not for merge into master.
Also, the right way to add dependencies like this is to add them to pluginConfig.json as references so that you don't need to update webpack.pwa.js

path.resolve(__dirname, '../pixilib/modes/learning-mode/node_modules'),
path.resolve(__dirname, '../pixilib/extensions/custom-tools/node_modules'),
path.resolve(__dirname, '../pixilib/extensions/e-learning/node_modules'),
path.resolve(__dirname, '../pixilib/extensions/gaelo-ohif-forms/node_modules'),
path.resolve(__dirname, '../pixilib/extensions/gaelo-panels-viewport/node_modules'),
path.resolve(__dirname, '../pixilib/extensions/gaelo-tmtv/node_modules'),
path.resolve(__dirname, '../pixilib/extensions/hanging-protocol-manager/node_modules'),
],
},
plugins: [
Expand Down
36 changes: 36 additions & 0 deletions platform/app/pluginConfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,34 @@
{
"packageName": "@ohif/extension-ultrasound-pleura-bline",
"version": "3.0.0"
},
{
"packageName": "@pixilib/custom-tools",
"version": "0.0.1"
},
{
"packageName": "@pixilib/e-learning",
"version": "0.0.1"
},
{
"packageName": "@pixilib/gaelo-ohif-forms",
"version": "0.0.1"
},
{
"packageName": "@pixilib/gaelo-panels-viewport",
"version": "0.0.1"
},
{
"packageName": "@pixilib/gaelo-tmtv",
"version": "0.0.1"
},
{
"packageName": "@pixilib/hanging-protocol-manager",
"version": "0.0.1"
},
{
"packageName": "@pixilib/volume-registration",
"version": "0.0.1"
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
],
"modes": [
Expand Down Expand Up @@ -99,6 +127,14 @@
{
"packageName": "@ohif/mode-ultrasound-pleura-bline",
"version": "3.0.0"
},
{
"packageName": "@pixilib/gaelo-mode",
"version": "0.0.1"
},
{
"packageName": "@pixilib/learning-mode",
"version": "0.0.1"
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
],
"public": [
Expand Down
6 changes: 5 additions & 1 deletion platform/app/public/config/default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ window.config = {
// whiteLabeling: {},
extensions: [],
modes: [],
customizationService: {},
customizationService: [
"@pixilib/gaelo-panels-viewport.customizationModule.default",
"@pixilib/gaelo-ohif-forms.customizationModule.loginRoute",
],
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
disableConfirmationPrompts: true,
showStudyList: true,
// some windows systems have issues with more than 3 web workers
maxNumberOfWebWorkers: 3,
Expand Down
4 changes: 3 additions & 1 deletion platform/app/tailwind.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/** @type {import('tailwindcss').Config} */
// Todo: we probably should add external installed extensions/modes here as well.

module.exports = {
// Note: in Tailwind 3.0, JIT will purge unused styles by default
// but in development, it is often useful to disable this to see
Expand All @@ -16,6 +17,7 @@ module.exports = {
'../../node_modules/@ohif/ui/src/**/*.{js,jsx,ts,tsx,css}',
'../../node_modules/@ohif/ui-next/src/**/*.{js,jsx,ts,tsx,css}',
'../../node_modules/@ohif/extension-*/src/**/*.{js,jsx,css, ts,tsx}',
'../../node_modules/@pixilib/**/*.{jsx,js,ts,tsx,css}',
],
theme: {
fontFamily: {
Expand Down
14 changes: 14 additions & 0 deletions platform/core/src/services/PanelService/PanelService.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,20 @@ export default class PanelService extends PubSubService {
this._broadcastEvent(EVENTS.PANELS_CHANGED, { position, options });
}

public removePanel(panelId: string, position: PanelPosition, options): void {

@wayfarer3130 wayfarer3130 Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Address the greptile comment about panelPosition - suggest just removing it.


const leftPanels = this._panelsGroups.get(PanelPosition.Left)?.map((p) => p.id) ?? [];
const rightPanels = this._panelsGroups.get(PanelPosition.Right)?.map((p) => p.id) ?? [];

const newLeftPanels = leftPanels.filter((id) => id !== panelId);
const newRightPanels = rightPanels.filter((id) => id !== panelId);

this.setPanels({

@wayfarer3130 wayfarer3130 Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably need to fix setPanels so that it only updates panels actually provided. That will need an update to the reset to work.

[PanelPosition.Left]: newLeftPanels,
[PanelPosition.Right]: newRightPanels,
} as any, options);
}

@greptile-apps greptile-apps Bot Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 position parameter is declared but never used

The method signature declares a position: PanelPosition argument, but the body ignores it entirely — it always reads and filters both Left and Right regardless. Any caller who passes a specific position expecting targeted removal will be confused by this silent discard. Either remove the parameter (matching the PR description's removePanel(panelId, options = {}) signature) or use it to scope the removal to the specified position.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: platform/core/src/services/PanelService/PanelService.tsx
Line: 146-158

Comment:
**`position` parameter is declared but never used**

The method signature declares a `position: PanelPosition` argument, but the body ignores it entirely — it always reads and filters both `Left` and `Right` regardless. Any caller who passes a specific position expecting targeted removal will be confused by this silent discard. Either remove the parameter (matching the PR description's `removePanel(panelId, options = {})` signature) or use it to scope the removal to the specified position.

How can I resolve this? If you propose a fix, please make it concise.

@greptile-apps greptile-apps Bot Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Bottom panels are silently wiped on every removePanel call

setPanels internally calls this.reset(), which clears all positions — including Bottom. The removePanel implementation only reconstructs Left and Right panels before passing them back to setPanels; Bottom panels are never read and therefore never restored. Any panel currently registered at PanelPosition.Bottom will be irreversibly dropped as a side-effect of removing an unrelated Left or Right panel. The fix is to include Bottom in the filter-and-restore logic (or avoid setPanels/reset() altogether and instead mutate only the target position directly).

Prompt To Fix With AI 
This is a comment left during a code review.
Path: platform/core/src/services/PanelService/PanelService.tsx
Line: 146-158

Comment:
**Bottom panels are silently wiped on every `removePanel` call**

`setPanels` internally calls `this.reset()`, which clears all positions — including `Bottom`. The `removePanel` implementation only reconstructs `Left` and `Right` panels before passing them back to `setPanels`; `Bottom` panels are never read and therefore never restored. Any panel currently registered at `PanelPosition.Bottom` will be irreversibly dropped as a side-effect of removing an unrelated Left or Right panel. The fix is to include Bottom in the filter-and-restore logic (or avoid `setPanels`/`reset()` altogether and instead mutate only the target position directly).

How can I resolve this? If you propose a fix, please make it concise.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

public addPanels(position: PanelPosition, panelsIds: string[], options): void {
if (!Array.isArray(panelsIds)) {
throw new Error('Invalid "panelsIds" array');
Expand Down