From 4d04dd5651f4cf8dcaf4732baee5e218e92bd3c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20K=C3=B6hler?= Date: Tue, 7 Oct 2025 09:52:56 -0300 Subject: [PATCH] fix(toolbar): prevent duplicate command execution on repeated interactions (#5456) --- .../src/hooks/__tests__/useToolbar.test.ts | 188 ++++++++++++++++++ platform/core/src/hooks/useToolbar.tsx | 45 +---- .../core/src/services/ToolBarService/types.ts | 3 +- platform/core/src/types/index.ts | 1 + .../core/src/utils/buildButtonCommands.ts | 40 ++++ platform/core/src/utils/index.ts | 3 + 6 files changed, 238 insertions(+), 42 deletions(-) create mode 100644 platform/core/src/hooks/__tests__/useToolbar.test.ts create mode 100644 platform/core/src/utils/buildButtonCommands.ts diff --git a/platform/core/src/hooks/__tests__/useToolbar.test.ts b/platform/core/src/hooks/__tests__/useToolbar.test.ts new file mode 100644 index 000000000..01d85b94d --- /dev/null +++ b/platform/core/src/hooks/__tests__/useToolbar.test.ts @@ -0,0 +1,188 @@ +import { act } from 'react'; +import { renderHook } from '@testing-library/react'; +import { useToolbar } from '../useToolbar'; +import { useSystem } from '../../contextProviders/SystemProvider'; + +type MockSubscription = { unsubscribe: jest.Mock }; + +type MockToolbarService = { + EVENTS: Record; + getButtonSection: jest.Mock; + getButtonProps: jest.Mock; + recordInteraction: jest.Mock; + subscribe: jest.Mock void]>; + getButton: jest.Mock; + refreshToolbarState: jest.Mock; +}; + +type MockViewportGridService = { + EVENTS: Record; + subscribe: jest.Mock void]>; + getActiveViewportId: jest.Mock; +}; + +jest.mock('../../contextProviders/SystemProvider', () => ({ + useSystem: jest.fn(), +})); + +const mockedUseSystem = useSystem as jest.MockedFunction; + +const createToolbarService = (): MockToolbarService => ({ + EVENTS: { + TOOL_BAR_MODIFIED: 'TOOL_BAR_MODIFIED', + TOOL_BAR_STATE_MODIFIED: 'TOOL_BAR_STATE_MODIFIED', + }, + getButtonSection: jest.fn().mockReturnValue([]), + getButtonProps: jest.fn().mockReturnValue({}), + recordInteraction: jest.fn(), + subscribe: jest.fn(() => ({ unsubscribe: jest.fn() })), + getButton: jest.fn(), + refreshToolbarState: jest.fn(), +}); + +const createViewportGridService = (): MockViewportGridService => ({ + EVENTS: { + ACTIVE_VIEWPORT_ID_CHANGED: 'ACTIVE_VIEWPORT_ID_CHANGED', + VIEWPORTS_READY: 'VIEWPORTS_READY', + LAYOUT_CHANGED: 'LAYOUT_CHANGED', + }, + subscribe: jest.fn(() => ({ unsubscribe: jest.fn() })), + getActiveViewportId: jest.fn().mockReturnValue('ACTIVE_VIEWPORT'), +}); + +describe('useToolbar', () => { + let toolbarService: MockToolbarService; + let viewportGridService: MockViewportGridService; + let commandsManager: { run: jest.Mock }; + let servicesManager: { + services: { toolbarService: MockToolbarService; viewportGridService: MockViewportGridService }; + }; + + const renderToolbarHook = () => renderHook(() => useToolbar({ buttonSection: 'primary' } as any)); + + beforeEach(() => { + toolbarService = createToolbarService(); + viewportGridService = createViewportGridService(); + commandsManager = { run: jest.fn() }; + servicesManager = { services: { toolbarService, viewportGridService } }; + + mockedUseSystem.mockReset(); + mockedUseSystem.mockReturnValue({ + commandsManager, + servicesManager, + } as any); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('aggregates commands on interaction and defers execution to the commands manager', () => { + const stopPropagation = jest.fn(); + + const option = { + id: 'option-1', + value: 'opt-1', + commands: 'secondaryCommand', + label: 'Option 1', + }; + const ignoredOption = { + id: 'option-ignored', + value: 'ignored', + explicitRunOnly: true, + commands: ['ignoredCommand'], + }; + + toolbarService.getButtonProps.mockReturnValue({ + commands: ['primaryCommand'], + options: [option, ignoredOption], + }); + + const { result } = renderToolbarHook(); + + act(() => { + result.current.onInteraction({ + itemId: 'test-button', + viewportId: 'custom-viewport', + event: { stopPropagation }, + }); + }); + + expect(stopPropagation).toHaveBeenCalledTimes(1); + expect(toolbarService.getButtonProps).toHaveBeenCalledWith('test-button'); + + const [interactionArgs, interactionMeta] = toolbarService.recordInteraction.mock.calls[0]; + const { commands } = interactionArgs; + + expect(commands).toHaveLength(2); + commands.forEach(commandFn => expect(typeof commandFn).toBe('function')); + + (commands[0] as () => void)(); + expect(commandsManager.run).toHaveBeenLastCalledWith( + 'primaryCommand', + expect.objectContaining({ + itemId: 'test-button', + viewportId: 'custom-viewport', + event: expect.any(Object), + }) + ); + + (commands[1] as () => void)(); + expect(commandsManager.run).toHaveBeenLastCalledWith( + 'secondaryCommand', + expect.objectContaining({ + id: 'option-1', + value: 'opt-1', + options: expect.any(Array), + servicesManager, + commandsManager, + }) + ); + + expect(interactionMeta).toEqual({ refreshProps: { viewportId: 'custom-viewport' } }); + }); + + it('keeps button props immutable when aggregating commands', () => { + const stopPropagation = jest.fn(); + const option = { + id: 'option-1', + value: 'opt-1', + commands: 'secondaryCommand', + label: 'Option 1', + }; + const ignoredOption = { + id: 'option-ignored', + value: 'ignored', + explicitRunOnly: true, + commands: ['ignoredCommand'], + }; + const buttonProps = { + commands: ['primaryCommand'], + options: [option, ignoredOption], + }; + + toolbarService.getButtonProps.mockReturnValue(buttonProps); + + const { result } = renderToolbarHook(); + + act(() => { + result.current.onInteraction({ + itemId: 'test-button', + viewportId: 'custom-viewport', + event: { stopPropagation }, + }); + }); + + act(() => { + result.current.onInteraction({ + itemId: 'test-button', + viewportId: 'custom-viewport', + event: { stopPropagation }, + }); + }); + + expect(stopPropagation).toHaveBeenCalledTimes(2); + expect(buttonProps.commands).toHaveLength(1); + expect(toolbarService.recordInteraction).toHaveBeenCalledTimes(2); + }); +}); diff --git a/platform/core/src/hooks/useToolbar.tsx b/platform/core/src/hooks/useToolbar.tsx index 33c2fe011..f4a8c4bff 100644 --- a/platform/core/src/hooks/useToolbar.tsx +++ b/platform/core/src/hooks/useToolbar.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, useState, useMemo } from 'react'; import { useSystem } from '../contextProviders/SystemProvider'; import { ToolbarHookReturn } from './types'; +import { buildButtonCommands } from '../utils'; export function useToolbar({ buttonSection = 'primary' }: withAppTypes): ToolbarHookReturn { const { commandsManager, servicesManager } = useSystem(); @@ -24,48 +25,10 @@ export function useToolbar({ buttonSection = 'primary' }: withAppTypes): Toolbar const refreshProps = { viewportId }; const buttonProps = toolbarService.getButtonProps(args.itemId); + const commands = buildButtonCommands(buttonProps, args, { servicesManager, commandsManager }); + const computedProps = { ...buttonProps, commands }; - if (buttonProps.commands || buttonProps.options) { - const allCommands = []; - const options = buttonProps.options || []; - const itemCommands = buttonProps.commands || []; - - // Process item commands - if (itemCommands) { - Array.isArray(itemCommands) - ? allCommands.push(...itemCommands) - : allCommands.push(itemCommands); - } - - // Process commands from options - if (options.length > 0) { - options.forEach(option => { - if (!option.commands) { - return; - } - - const valueToUse = option.value; - const commands = Array.isArray(option.commands) ? option.commands : [option.commands]; - - commands.forEach(command => { - const commandOptions = { - ...option, - value: valueToUse, - options: buttonProps.options, - servicesManager: servicesManager, - commandsManager: commandsManager, - }; - - const processedCommand = () => commandsManager.run(command, commandOptions); - allCommands.push(processedCommand); - }); - }); - } - - buttonProps.commands = allCommands; - } - - toolbarService.recordInteraction({ ...args, ...buttonProps }, { refreshProps }); + toolbarService.recordInteraction({ ...args, ...computedProps }, { refreshProps }); }, [toolbarService, viewportGridService] ); diff --git a/platform/core/src/services/ToolBarService/types.ts b/platform/core/src/services/ToolBarService/types.ts index 6e091f1f9..d684172b5 100644 --- a/platform/core/src/services/ToolBarService/types.ts +++ b/platform/core/src/services/ToolBarService/types.ts @@ -40,10 +40,11 @@ export type ButtonOptions = { max?: number; step?: number; value?: number | number[] | string; - commands?: (value: unknown) => void; + commands?: RunCommand; condition?: (props: Record) => boolean; children?: React.ReactNode | (() => React.ReactNode); options?: Array<{ value: string; label: string }>; + explicitRunOnly?: boolean; }; export type ButtonProps = { diff --git a/platform/core/src/types/index.ts b/platform/core/src/types/index.ts index e01e64f71..42a9a3565 100644 --- a/platform/core/src/types/index.ts +++ b/platform/core/src/types/index.ts @@ -8,6 +8,7 @@ import type { BaseDataSourceConfigurationAPIItem, } from './DataSourceConfigurationAPI'; +export type * from '../services/ToolBarService/types'; export type * from '../services/ViewportGridService'; export type * from '../services/CustomizationService/types'; // Separate out some generic types diff --git a/platform/core/src/utils/buildButtonCommands.ts b/platform/core/src/utils/buildButtonCommands.ts new file mode 100644 index 000000000..033dfdb82 --- /dev/null +++ b/platform/core/src/utils/buildButtonCommands.ts @@ -0,0 +1,40 @@ +import { ButtonProps, RunCommand } from '../types'; + +const toArray = (value?: T | T[]): T[] => + Array.isArray(value) ? value : value != null ? [value] : []; + +export const buildButtonCommands = ( + buttonProps: ButtonProps, + baseArgs: Record, + { servicesManager, commandsManager }: AppTypes.Managers +): Array<() => unknown> => { + const allCommands: Array<() => unknown> = []; + + // 1) normalize item-level commands + for (const command of toArray(buttonProps.commands as RunCommand)) { + allCommands.push(() => commandsManager.run(command, baseArgs)); + } + + // 2) normalize option-level commands + for (const option of toArray(buttonProps.options)) { + const shouldSkip = !option?.commands || option.explicitRunOnly; + if (shouldSkip) { + continue; + } + + const valueToUse = option.value; + for (const command of toArray(option.commands)) { + const commandOptions = { + ...option, + value: valueToUse, + options: buttonProps.options, + servicesManager, + commandsManager, + }; + + allCommands.push(() => commandsManager.run(command, commandOptions)); + } + } + + return allCommands; +}; diff --git a/platform/core/src/utils/index.ts b/platform/core/src/utils/index.ts index 6b80a21ec..e624dacee 100644 --- a/platform/core/src/utils/index.ts +++ b/platform/core/src/utils/index.ts @@ -46,6 +46,8 @@ import getClosestOrientationFromIOP from './getClosestOrientationFromIOP'; import calculateScanAxisNormal from './calculateScanAxisNormal'; import areAllImageOrientationsEqual from './areAllImageOrientationsEqual'; import { structuredCloneWithFunctions } from './structuredCloneWithFunctions'; +import { buildButtonCommands } from './buildButtonCommands'; + // Commented out unused functionality. // Need to implement new mechanism for derived displaySets using the displaySetManager. @@ -134,6 +136,7 @@ export { createStudyBrowserTabs, MeasurementFilters, getClosestOrientationFromIOP, + buildButtonCommands, }; export default utils;