Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -397,11 +397,13 @@ class CornerstoneViewportService extends PubSubService implements IViewportServi
presentations?: Presentations
): void {
const renderingEngine = this.getRenderingEngine();
const { viewportGridService } = this.servicesManager.services;

// if not valid viewportData then return early
if (viewportData.viewportType === csEnums.ViewportType.STACK) {
// check if imageIds is valid
if (!viewportData.data[0].imageIds?.length) {
viewportGridService?.notifyViewportUpdateCompleted?.(viewportId);
return;
}
}
Expand Down Expand Up @@ -490,6 +492,7 @@ class CornerstoneViewportService extends PubSubService implements IViewportServi
viewportData,
viewportId,
});
viewportGridService?.notifyViewportUpdateCompleted?.(viewportId);
Copy link

Choose a reason for hiding this comment

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

Bug: Promise rejection prevents grid state change notification

The notifyViewportUpdateCompleted call is only placed inside .then() callbacks without any .catch() or .finally() handlers. If displaySetPromise rejects due to an error during viewport data loading, notifyViewportUpdateCompleted will never be called. This causes pendingGridStateChanges to wait indefinitely for that viewport, preventing GRID_STATE_CHANGED from ever being broadcast. Before this change, GRID_STATE_CHANGED would fire regardless of loading success (via setTimeout), but now the pending queue mechanism creates a dependency on successful completion.

Additional Locations (1)

Fix in Cursor Fix in Web

});
}

Expand Down Expand Up @@ -1107,6 +1110,7 @@ class CornerstoneViewportService extends PubSubService implements IViewportServi
const viewportInfo = this.getViewportInfo(viewportId);
const viewport = this.getCornerstoneViewport(viewportId);
const viewportCamera = viewport.getCamera();
const { viewportGridService } = this.servicesManager.services;

let displaySetPromise;

Expand All @@ -1128,6 +1132,7 @@ class CornerstoneViewportService extends PubSubService implements IViewportServi
viewportData,
viewportId,
});
viewportGridService?.notifyViewportUpdateCompleted?.(viewportId);
});
}

Expand Down
11 changes: 9 additions & 2 deletions platform/app/src/components/ViewportGrid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,15 @@ function ViewerViewportGrid(props: withAppTypes) {
const viewportId = Array.from(viewportMatchDetails.keys())[pos];
const details = viewportMatchDetails.get(viewportId);
if (!details) {
console.log('No match details for viewport', viewportId);
return;
return {
// we should create an empty viewport here, if the grid will not allocate
// a pane
displaySetInstanceUIDs: [],
displaySetOptions: [],
viewportOptions: {
viewportId,
},
};
}

const { displaySetsInfo, viewportOptions } = details;
Expand Down
166 changes: 130 additions & 36 deletions platform/core/src/services/ViewportGridService/ViewportGridService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ type PresentationIdProvider = (
{ viewport, viewports, isUpdatingSameViewport }
) => unknown;

type PendingGridStateChangePayload = {
state: AppTypes.ViewportGrid.State;
viewports?: AppTypes.ViewportGrid.Viewport[];
removedViewportIds: string[];
};

type PendingGridStateChange = {
payload: PendingGridStateChangePayload;
pendingViewportIds: Set<string>;
};

class ViewportGridService extends PubSubService {
public static readonly EVENTS = {
ACTIVE_VIEWPORT_ID_CHANGED: 'event::activeviewportidchanged',
Expand All @@ -26,12 +37,18 @@ class ViewportGridService extends PubSubService {
serviceImplementation = {};
servicesManager: AppTypes.ServicesManager;
presentationIdProviders: Map<string, PresentationIdProvider>;
pendingGridStateChanges: PendingGridStateChange[];

constructor({ servicesManager }) {
super(ViewportGridService.EVENTS);
this.servicesManager = servicesManager;
this.serviceImplementation = {};
this.presentationIdProviders = new Map();
/**
* Pending grid state changes waiting for associated viewports
* to signal that their data updates completed.
*/
this.pendingGridStateChanges = [];
}

public addPresentationIdProvider(id: string, provider: PresentationIdProvider): void {
Expand Down Expand Up @@ -148,14 +165,15 @@ class ViewportGridService extends PubSubService {
if (id === this.getActiveViewportId()) {
return;
}
this.serviceImplementation._setActiveViewport(id);
const state = this.serviceImplementation._setActiveViewport(id);

// Use queueMicrotask to delay the event broadcast
setTimeout(() => {
this._broadcastEvent(this.EVENTS.ACTIVE_VIEWPORT_ID_CHANGED, {
viewportId: id,
});
}, 0);
this._broadcastEvent(this.EVENTS.ACTIVE_VIEWPORT_ID_CHANGED, {
viewportId: id,
state,
});

return state;
}

public getState(): AppTypes.ViewportGrid.State {
Expand All @@ -182,14 +200,20 @@ class ViewportGridService extends PubSubService {
});
}

public setDisplaySetsForViewport(props) {
public setDisplaySetsForViewport(props, options: { preCallback?: () => void } = {}) {
// Just update a single viewport, but use the multi-viewport update for it.
this.setDisplaySetsForViewports([props]);
this.setDisplaySetsForViewports([props], { preCallback: options.preCallback });
}

public async setDisplaySetsForViewports(viewportsToUpdate) {
await this.serviceImplementation._setDisplaySetsForViewports(viewportsToUpdate);
const state = this.getState();
public async setDisplaySetsForViewports(
viewportsToUpdate,
{ preCallback }: { preCallback?: () => void } = {}
) {
if (preCallback) {
preCallback();
}

const state = await this.serviceImplementation._setDisplaySetsForViewports(viewportsToUpdate);
const updatedViewports = [];

const removedViewportIds = [];
Expand All @@ -212,13 +236,14 @@ class ViewportGridService extends PubSubService {
}
}

setTimeout(() => {
this._broadcastEvent(ViewportGridService.EVENTS.GRID_STATE_CHANGED, {
this._queueGridStateChanged(
{
state,
viewports: updatedViewports,
removedViewportIds,
});
});
},
updatedViewports.map(viewport => viewport.viewportId)
);
Copy link

Choose a reason for hiding this comment

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

Bug: Empty viewport pending IDs block GRID_STATE_CHANGED events

In setDisplaySetsForViewports, the pendingViewportIds on line 245 includes ALL updated viewports via updatedViewports.map(viewport => viewport.viewportId), even those with empty displaySetInstanceUIDs. However, viewports without display sets render EmptyViewport (which never calls notifyViewportUpdateCompleted), and setViewportData returns early without calling notifyViewportUpdateCompleted when imageIds are empty. This causes the pending queue to never clear for such viewports, blocking all subsequent GRID_STATE_CHANGED events. The fix should filter to only include viewports with displaySetInstanceUIDs, similar to how setLayout and set use _getViewportIdsWithDisplaySets.

Fix in Cursor Fix in Web

}

/**
Expand Down Expand Up @@ -253,7 +278,7 @@ class ViewportGridService extends PubSubService {
const prevState = this.getState();
const prevViewportIds = new Set(prevState.viewports.keys());

await this.serviceImplementation._setLayout({
const state = await this.serviceImplementation._setLayout({
numCols,
numRows,
layoutOptions,
Expand All @@ -263,29 +288,29 @@ class ViewportGridService extends PubSubService {
isHangingProtocolLayout,
});

// Use queueMicrotask to ensure the layout changed event is published after
setTimeout(() => {
// Get the new state after the layout change
const state = this.getState();
const currentViewportIds = new Set(state.viewports.keys());
const currentViewportIds = new Set(state.viewports.keys());

// Determine which viewport IDs have been removed
const removedViewportIds = [...prevViewportIds].filter(id => !currentViewportIds.has(id));
// Determine which viewport IDs have been removed
const removedViewportIds = [...prevViewportIds].filter(id => !currentViewportIds.has(id));

this._broadcastEvent(this.EVENTS.LAYOUT_CHANGED, {
numCols,
numRows,
});
// Use queueMicrotask to ensure the layout changed event is published after
Copy link
Contributor

Choose a reason for hiding this comment

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

is published after the state has been updated/is available.

this._broadcastEvent(this.EVENTS.LAYOUT_CHANGED, {
numCols,
numRows,
});

this._broadcastEvent(this.EVENTS.GRID_STATE_CHANGED, {
this._queueGridStateChanged(
{
state,
removedViewportIds,
});
}, 0);
},
this._getViewportIdsWithDisplaySets(state)
);
}

public reset() {
this.serviceImplementation._reset();
this.pendingGridStateChanges = [];
}

/**
Expand All @@ -296,31 +321,100 @@ class ViewportGridService extends PubSubService {
*/
public onModeExit(): void {
this.serviceImplementation._onModeExit();
this.pendingGridStateChanges = [];
}

public set(newState) {
const prevState = this.getState();
const prevViewportIds = new Set(prevState.viewports.keys());

this.serviceImplementation._set(newState);
const state = this.serviceImplementation._set(newState);

const state = this.getState();
const currentViewportIds = new Set(state.viewports.keys());

const removedViewportIds = [...prevViewportIds].filter(id => !currentViewportIds.has(id));

setTimeout(() => {
this._broadcastEvent(this.EVENTS.GRID_STATE_CHANGED, {
this._queueGridStateChanged(
{
state,
removedViewportIds,
});
}, 0);
},
this._getViewportIdsWithDisplaySets(state)
);
}

public getNumViewportPanes() {
return this.serviceImplementation._getNumViewportPanes();
}

/**
* Signals that a viewport has finished applying its pending data update so that queued
* grid state changes can be published when all dependencies are resolved.
*/
public notifyViewportUpdateCompleted(viewportId: string) {
if (!viewportId || !this.pendingGridStateChanges.length) {
return;
}

let didUpdatePendingChange = false;
for (let i = 0; i < this.pendingGridStateChanges.length; i++) {
const pendingChange = this.pendingGridStateChanges[i];
didUpdatePendingChange =
pendingChange.pendingViewportIds.delete(viewportId) || didUpdatePendingChange;
}

if (didUpdatePendingChange) {
this._flushPendingGridStateChanges();
}
}

private _getViewportIdsWithDisplaySets(state: AppTypes.ViewportGrid.State): string[] {
if (!state?.viewports?.size) {
return [];
}

const viewportIds: string[] = [];

state.viewports.forEach(viewport => {
if (viewport?.displaySetInstanceUIDs?.length && viewport.viewportId) {
viewportIds.push(viewport.viewportId);
}
});

return viewportIds;
}

private _queueGridStateChanged(
payload: PendingGridStateChangePayload,
pendingViewportIds: string[] = []
) {
const uniquePendingIds = Array.from(new Set(pendingViewportIds?.filter(Boolean)));

if (!uniquePendingIds.length) {
this._broadcastEvent(this.EVENTS.GRID_STATE_CHANGED, payload);
return;
}

const pendingChange: PendingGridStateChange = {
payload,
pendingViewportIds: new Set(uniquePendingIds),
};

this.pendingGridStateChanges.push(pendingChange);
}

private _flushPendingGridStateChanges() {
while (this.pendingGridStateChanges.length) {
const nextChange = this.pendingGridStateChanges[0];
if (nextChange.pendingViewportIds.size) {
break;
}

this._broadcastEvent(this.EVENTS.GRID_STATE_CHANGED, nextChange.payload);
this.pendingGridStateChanges.shift();
}
}

public getLayoutOptionsFromState(
state: any
): { x: number; y: number; width: number; height: number }[] {
Expand Down
17 changes: 17 additions & 0 deletions platform/docs/docs/migration-guide/3p11-to-3p12/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,20 @@ specific fields that have changed are:
If these fields need formatted versions, it is recommended to add a secondary/computed
metadata provider which simply gets the base metadata module and adds the
formatting. That way different metadata providers are all handled identically.

## Viewport grid state change timing

The `viewportGridService` now defers the `GRID_STATE_CHANGED` event until every
affected viewport finishes its `viewport data changed` work (this happens just
before Cornerstone renders). This guarantees listeners see the final display-set
assignments instead of intermediate state.

If you need to react earlier you can continue to use the other hooks that fire
immediately:

- `VIEWPORTS_READY` – panes are initialized, but their display sets may not be mounted
- `LAYOUT_CHANGED` – grid rows/columns changed, but viewport data may still be updating

Custom viewports must call `viewportGridService.notifyViewportUpdateCompleted(viewportId)`
after they dispatch their own viewport-data-change event so the grid service knows when it
can publish the queued state change.
Loading