From eb9e8bd7c8d2cbe3f509344d915f5f9ca57dac0d Mon Sep 17 00:00:00 2001 From: Igor Octaviano Date: Fri, 31 Jul 2026 16:21:32 -0300 Subject: [PATCH 1/9] fix: make XHR retry hook actually retry and keep ICC switch state in sync The retry request hook re-opened the XHR on retry but never re-sent it, and the client's onreadystatechange fired on the first failed attempt, so retries never happened. Rewrite the hook to re-open, restore headers and responseType, and re-send on retryable statuses, deferring the client callback until the final outcome. Retries are skipped for non-idempotent methods (e.g. STOW POST) and default backoff is capped (3 retries, 10s max) so failures surface promptly. Also reset isICCProfilesEnabled when SlideViewer recreates the viewer on slide/series switch: a fresh viewer starts with ICC on, so the settings switch desynced (and inverted) after an in-session toggle. --- src/AppConfig.d.ts | 4 +- src/components/SlideViewer.tsx | 3 + src/utils/xhrRetryHook.ts | 124 ++++++++++++++++++--------------- 3 files changed, 73 insertions(+), 58 deletions(-) diff --git a/src/AppConfig.d.ts b/src/AppConfig.d.ts index be2859b7..fbad6bc8 100644 --- a/src/AppConfig.d.ts +++ b/src/AppConfig.d.ts @@ -9,6 +9,8 @@ export type DicomWebManagerErrorHandler = ( export interface DICOMwebClientRequestHookMetadata { url: string method: string + /** Combined request headers from dicomweb-client (needed to re-apply after retry open()). */ + headers?: Record } export interface RetryRequestSettings { @@ -17,7 +19,7 @@ export interface RetryRequestSettings { minTimeout?: number maxTimeout?: number randomize?: boolean - retryableStatusCodes: number[] + retryableStatusCodes?: number[] } export interface EvaluationSetting { diff --git a/src/components/SlideViewer.tsx b/src/components/SlideViewer.tsx index ff766773..cf5232e0 100644 --- a/src/components/SlideViewer.tsx +++ b/src/components/SlideViewer.tsx @@ -429,6 +429,9 @@ class SlideViewer extends React.Component { selectedSeriesInstanceUID: undefined, validXCoordinateRange: [offset[0], offset[0] + size[0]], validYCoordinateRange: [offset[1], offset[1] + size[1]], + // A freshly constructed viewer always starts with ICC profiles + // enabled; reset the flag so the settings switch stays in sync. + isICCProfilesEnabled: true, }) this.populateViewports() } diff --git a/src/utils/xhrRetryHook.ts b/src/utils/xhrRetryHook.ts index 6c295b69..1c87a152 100644 --- a/src/utils/xhrRetryHook.ts +++ b/src/utils/xhrRetryHook.ts @@ -10,60 +10,43 @@ type RequestHook = ( metadata: DICOMwebClientRequestHookMetadata, ) => XMLHttpRequest +/** + * HTTP methods that are safe to retry automatically. Non-idempotent methods + * (e.g. STOW POST) are excluded because re-sending them can duplicate + * partially stored data on a server that failed mid-request. + */ +const RETRYABLE_METHODS = ['GET', 'HEAD', 'OPTIONS'] + /** * Returns a configured retry request hook function * that can be used to add retry functionality to XHR request. * * Default options: - * retries: 5 - * factor: 3 + * retries: 3 + * factor: 2 * minTimeout: 1 * 1000 - * maxTimeout: 60 * 1000 + * maxTimeout: 10 * 1000 * randomize: true * * @param options - * @param options.retires - Number of retries - * @param options.factor - Factor - * @param options.minTimeout - Min number of seconds to wait before next retry - * @param options.maxTimeout - Max number of seconds to wait before next retry + * @param options.retries - Number of retries + * @param options.factor - Exponential backoff factor + * @param options.minTimeout - Min number of milliseconds to wait before next retry + * @param options.maxTimeout - Max number of milliseconds to wait before next retry * @param options.randomize - Whether randomization should be applied * @param options.retryableStatusCodes HTTP status codes that can trigger a retry * @returns Configured retry request function */ export const getXHRRetryHook = ( - options: RetryRequestSettings = { - retries: 5, - factor: 3, - minTimeout: 1 * 1000, - maxTimeout: 60 * 1000, - randomize: true, - retryableStatusCodes: [429, 500], - }, + options: RetryRequestSettings = {}, ): RequestHook => { - const retryOptions = options - - if (options.retries != null) { - retryOptions.retries = options.retries - } - - if (options.factor != null) { - retryOptions.factor = options.factor - } - - if (options.minTimeout != null) { - retryOptions.minTimeout = options.minTimeout - } - - if (options.maxTimeout != null) { - retryOptions.maxTimeout = options.maxTimeout - } - - if (options.randomize != null) { - retryOptions.randomize = options.randomize - } - - if (options.retryableStatusCodes != null) { - retryOptions.retryableStatusCodes = options.retryableStatusCodes + const retryOptions = { + retries: options.retries ?? 3, + factor: options.factor ?? 2, + minTimeout: options.minTimeout ?? 1 * 1000, + maxTimeout: options.maxTimeout ?? 10 * 1000, + randomize: options.randomize ?? true, + retryableStatusCodes: options.retryableStatusCodes ?? [429, 500], } /** @@ -81,41 +64,68 @@ export const getXHRRetryHook = ( ): XMLHttpRequest => { const { url, method } = metadata + if (!RETRYABLE_METHODS.includes(method.toUpperCase())) { + return request + } + + const headers = metadata.headers ?? {} + const originalRequestSend = request.send + /** Captured before open() resets it on retry. */ + let responseType: XMLHttpRequestResponseType = request.responseType + /** dicomweb-client handler installed before this hook runs. */ + const clientOnReadyStateChange = request.onreadystatechange + function faultTolerantRequestSend( ...args: Parameters ): void { - const operation = retry.operation(retryOptions) + responseType = request.responseType + const operation = retry.operation({ + retries: retryOptions.retries, + factor: retryOptions.factor, + minTimeout: retryOptions.minTimeout, + maxTimeout: retryOptions.maxTimeout, + randomize: retryOptions.randomize, + }) operation.attempt(function operationAttempt(currentAttempt) { - const originalOnReadyStateChange = request.onreadystatechange + if (currentAttempt > 1) { + console.warn(`Requesting ${url}... (attempt: ${currentAttempt})`) + // open() clears headers / responseType — restore what dicomweb-client set. + request.open(method, url, true) + request.responseType = responseType + for (const key of Object.keys(headers)) { + request.setRequestHeader(key, headers[key]) + } + } - /** Overriding/extending XHR function */ request.onreadystatechange = function onReadyStateChange( ev: Event, ): void { - if (originalOnReadyStateChange != null) { - originalOnReadyStateChange.call(request, ev) + if (request.readyState !== XMLHttpRequest.DONE) { + return } - if (retryOptions.retryableStatusCodes.includes(request.status)) { - const errorMessage = `Attempt to request ${url} failed.` - const attemptFailedError = new Error(errorMessage) - operation.retry(attemptFailedError) + if ( + retryOptions.retryableStatusCodes.includes(request.status) && + operation.retry( + new Error( + `Attempt to request ${url} failed (${request.status}).`, + ), + ) + ) { + // Schedule another attempt; do not surface failure to dicomweb-client yet. + return } - } - /** Call open only on retry (after headers and other things were set in the xhr instance) */ - if (currentAttempt > 1) { - console.warn(`Requesting ${url}... (attempt: ${currentAttempt})`) - request.open(method, url, true) + if (clientOnReadyStateChange != null) { + clientOnReadyStateChange.call(request, ev) + } } - }) - originalRequestSend.apply(request, args) + originalRequestSend.apply(request, args) + }) } - /** Overriding/extending XHR function */ - const originalRequestSend = request.send request.send = faultTolerantRequestSend return request From 0a262f7013fc62bbeaecc7c6d4026cb28190593e Mon Sep 17 00:00:00 2001 From: Igor Octaviano Date: Fri, 31 Jul 2026 16:46:43 -0300 Subject: [PATCH 2/9] fix: retry proxy 5xx responses and clarify open() comment Include 502/503/504 in the default retryable status codes (common transient failures behind reverse proxies) and correct the open() comment: headers are cleared, responseType is not. --- src/utils/xhrRetryHook.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/utils/xhrRetryHook.ts b/src/utils/xhrRetryHook.ts index 1c87a152..f94201e9 100644 --- a/src/utils/xhrRetryHook.ts +++ b/src/utils/xhrRetryHook.ts @@ -46,7 +46,9 @@ export const getXHRRetryHook = ( minTimeout: options.minTimeout ?? 1 * 1000, maxTimeout: options.maxTimeout ?? 10 * 1000, randomize: options.randomize ?? true, - retryableStatusCodes: options.retryableStatusCodes ?? [429, 500], + retryableStatusCodes: options.retryableStatusCodes ?? [ + 429, 500, 502, 503, 504, + ], } /** @@ -90,7 +92,7 @@ export const getXHRRetryHook = ( operation.attempt(function operationAttempt(currentAttempt) { if (currentAttempt > 1) { console.warn(`Requesting ${url}... (attempt: ${currentAttempt})`) - // open() clears headers / responseType — restore what dicomweb-client set. + // open() empties author request headers; re-apply those + responseType. request.open(method, url, true) request.responseType = responseType for (const key of Object.keys(headers)) { From 7ed1014816c87f98804bd8adaba96e2e3c4ee428 Mon Sep 17 00:00:00 2001 From: Igor Octaviano Date: Fri, 31 Jul 2026 17:40:31 -0300 Subject: [PATCH 3/9] docs: prefer JSDoc block comments for explanations Add a Cursor rule and CONTRIBUTING note that explanatory comments use /** */ rather than // (tooling directives excepted). Convert the new comments in the retry hook and ICC reset to that style. --- .cursor/rules/jsdoc-comments.mdc | 35 ++++++++++++++++++++++++++++++++ CONTRIBUTING.md | 2 ++ src/components/SlideViewer.tsx | 6 ++++-- src/utils/xhrRetryHook.ts | 4 ++-- 4 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 .cursor/rules/jsdoc-comments.mdc diff --git a/.cursor/rules/jsdoc-comments.mdc b/.cursor/rules/jsdoc-comments.mdc new file mode 100644 index 00000000..2c59193c --- /dev/null +++ b/.cursor/rules/jsdoc-comments.mdc @@ -0,0 +1,35 @@ +--- +description: Prefer JSDoc block comments over line comments +globs: "**/*.{js,jsx,ts,tsx,mjs,cjs}" +alwaysApply: false +--- + +# Comment style + +Use JSDoc-style block comments (`/** */`) for explanatory comments in application and library source. Do not use `//` line comments for explanations. + +```js +// ❌ BAD +// Re-open the XHR and restore headers before retrying. +request.open(method, url, true) + +// ✅ GOOD +/** Re-open the XHR and restore headers before retrying. */ +request.open(method, url, true) +``` + +Multi-line: + +```js +/** + * When Range is ignored, complete from the buffered body instead of + * appending — otherwise coordinates duplicate and the loop never ends. + */ +``` + +Exceptions (keep `//`): +- Tooling directives: `eslint-disable`, `@ts-expect-error`, `biome-ignore`, `prettier-ignore` +- Temporarily commented-out code +- Shebang lines + +Public APIs should still use proper JSDoc tags (`@param`, `@returns`, etc.) inside `/** */`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bffdf40f..d6b857ea 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,6 +37,8 @@ The [pnpm](https://pnpm.io/) package manager is used to manage dependencies and Source code is linted and formatted using [Biome](https://biomejs.dev/). TypeScript is used with [strict type checking compiler options](https://www.typescriptlang.org/tsconfig#Strict_Type_Checking_Options_6173) enabled. Semicolons are not used at the end of statements (Biome uses `asNeeded`). +Explanatory comments use JSDoc-style block comments (`/** … */`), not `//` line comments. Keep `//` only for tooling directives (`eslint-disable`, `@ts-expect-error`, `biome-ignore`), temporarily commented-out code, and shebang lines. + Use the following commands to check and fix style: $ pnpm run lint # check for issues diff --git a/src/components/SlideViewer.tsx b/src/components/SlideViewer.tsx index cf5232e0..a0dddca8 100644 --- a/src/components/SlideViewer.tsx +++ b/src/components/SlideViewer.tsx @@ -429,8 +429,10 @@ class SlideViewer extends React.Component { selectedSeriesInstanceUID: undefined, validXCoordinateRange: [offset[0], offset[0] + size[0]], validYCoordinateRange: [offset[1], offset[1] + size[1]], - // A freshly constructed viewer always starts with ICC profiles - // enabled; reset the flag so the settings switch stays in sync. + /** + * A freshly constructed viewer always starts with ICC profiles + * enabled; reset the flag so the settings switch stays in sync. + */ isICCProfilesEnabled: true, }) this.populateViewports() diff --git a/src/utils/xhrRetryHook.ts b/src/utils/xhrRetryHook.ts index f94201e9..a16487bc 100644 --- a/src/utils/xhrRetryHook.ts +++ b/src/utils/xhrRetryHook.ts @@ -92,7 +92,7 @@ export const getXHRRetryHook = ( operation.attempt(function operationAttempt(currentAttempt) { if (currentAttempt > 1) { console.warn(`Requesting ${url}... (attempt: ${currentAttempt})`) - // open() empties author request headers; re-apply those + responseType. + /** open() empties author request headers; re-apply those + responseType. */ request.open(method, url, true) request.responseType = responseType for (const key of Object.keys(headers)) { @@ -115,7 +115,7 @@ export const getXHRRetryHook = ( ), ) ) { - // Schedule another attempt; do not surface failure to dicomweb-client yet. + /** Schedule another attempt; do not surface failure to dicomweb-client yet. */ return } From af9db2348821ad4ab14f216d501a29f418a1d587 Mon Sep 17 00:00:00 2001 From: Igor Octaviano Date: Fri, 31 Jul 2026 17:42:31 -0300 Subject: [PATCH 4/9] docs: clarify responseType capture comment in retry hook open() does not clear responseType; we re-apply it after retry open() for safety. Align the JSDoc with that. --- src/utils/xhrRetryHook.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/xhrRetryHook.ts b/src/utils/xhrRetryHook.ts index a16487bc..2caa7f76 100644 --- a/src/utils/xhrRetryHook.ts +++ b/src/utils/xhrRetryHook.ts @@ -72,7 +72,7 @@ export const getXHRRetryHook = ( const headers = metadata.headers ?? {} const originalRequestSend = request.send - /** Captured before open() resets it on retry. */ + /** Captured at send(); re-applied after retry open() for safety. */ let responseType: XMLHttpRequestResponseType = request.responseType /** dicomweb-client handler installed before this hook runs. */ const clientOnReadyStateChange = request.onreadystatechange From 17b0abcf6cdcb45555cbc0af6a511c616292a026 Mon Sep 17 00:00:00 2001 From: Igor Octaviano Date: Fri, 31 Jul 2026 19:55:22 -0300 Subject: [PATCH 5/9] refactor: address SonarCloud maintainability findings Extract the deeply nested per-series retrieval handlers in SlideViewer (SR ROIs, annotation groups, segmentations, parametric maps) and the presentation-state upsert into class methods, resolving the >5-level function nesting findings. Mark never-reassigned members readonly and use a Set for the retryable HTTP methods lookup in the XHR retry hook. Pure code movement; no behavior change. --- src/components/SlideViewer.tsx | 454 ++++++++++++++++++--------------- src/utils/xhrRetryHook.ts | 4 +- 2 files changed, 249 insertions(+), 209 deletions(-) diff --git a/src/components/SlideViewer.tsx b/src/components/SlideViewer.tsx index a0dddca8..770a4729 100644 --- a/src/components/SlideViewer.tsx +++ b/src/components/SlideViewer.tsx @@ -162,9 +162,9 @@ class SlideViewer extends React.Component { }, } - private roiStyles: { [key: string]: dmv.viewer.ROIStyleOptions } = {} + private readonly roiStyles: { [key: string]: dmv.viewer.ROIStyleOptions } = {} - private defaultAnnotationStyles: { + private readonly defaultAnnotationStyles: { [annotationUID: string]: StyleOptions } = {} @@ -441,6 +441,25 @@ class SlideViewer extends React.Component { this.publishActiveSeriesToService() } + /** + * Merge a presentation state into component state, replacing any previously + * stored instance with the same SOP Instance UID. + */ + private readonly upsertPresentationState = ( + presentationState: dmv.metadata.AdvancedBlendingPresentationState, + ): void => { + this.setState((state) => { + const mapping: { + [sopInstanceUID: string]: dmv.metadata.AdvancedBlendingPresentationState + } = {} + state.presentationStates.forEach((instance) => { + mapping[instance.SOPInstanceUID] = instance + }) + mapping[presentationState.SOPInstanceUID] = presentationState + return { presentationStates: Object.values(mapping) } + }) + } + /** * Retrieve Presentation State instances that reference the any images of * the currently selected series. @@ -504,19 +523,7 @@ class SlideViewer extends React.Component { this.setPresentationState(presentationState) } } - this.setState((state) => { - const mapping: { - [ - sopInstanceUID: string - ]: dmv.metadata.AdvancedBlendingPresentationState - } = {} - state.presentationStates.forEach((instance) => { - mapping[instance.SOPInstanceUID] = instance - }) - mapping[presentationState.SOPInstanceUID] = - presentationState - return { presentationStates: Object.values(mapping) } - }) + this.upsertPresentationState(presentationState) } } else { logger.log( @@ -959,6 +966,85 @@ class SlideViewer extends React.Component { } } + /** + * Parse a retrieved Comprehensive 3D SR instance and add the ROIs of a + * suitable measurement report to the volume viewer. + */ + private readonly addRetrievedSrRois = ( + retrievedInstance: dwc.api.Dataset, + ): void => { + const data = dcmjs.data.DicomMessage.readFile(retrievedInstance) + const { dataset } = dmv.metadata.formatMetadata(data.dict) + const report = dataset as unknown as dmv.metadata.Comprehensive3DSR + /* + * Perform a couple of checks to ensure the document content of the + * report fullfils the requirements of the application. + */ + if (!implementsTID1500(report)) { + logger.debug( + `ignore SR document "${report.SOPInstanceUID}" ` + + 'because it is not structured according to template ' + + 'TID 1500 "MeasurementReport"', + ) + return + } + if (!describesSpecimenSubject(report)) { + logger.debug( + `ignore SR document "${report.SOPInstanceUID}" ` + + 'because it does not describe a specimen subject', + ) + return + } + if (!containsROIAnnotations(report)) { + logger.debug( + `ignore SR document "${report.SOPInstanceUID}" ` + + 'because it does not contain any suitable ROI annotations', + ) + return + } + + const content = new MeasurementReport(report) + content.ROIs.forEach((roi) => { + logger.log(`add ROI "${roi.uid}"`) + const scoord3d = roi.scoord3d + const image = this.props.slide.volumeImages[0] + if (scoord3d.frameOfReferenceUID === image.FrameOfReferenceUID) { + /* + * ROIs may get assigned new UIDs upon re-rendering of the + * page and we need to ensure that we don't add them twice. + * The same ROI may be stored in multiple SR documents and + * we don't want them to show up twice. + * TODO: We should probably either "merge" measurements and + * quantitative evaluations or pick the ROI from the "best" + * available report (COMPLETE and VERIFIED). + */ + const doesROIExist = this.volumeViewer + .getAllROIs() + .some((otherROI: dmv.roi.ROI): boolean => { + return areROIsEqual(otherROI, roi) + }) + if (!doesROIExist) { + try { + // Add ROI without style such that it won't be visible. + this.volumeViewer.addROI(roi, {}) + const roiAsAnnotation = adaptRoiToAnnotation(roi) + this.formatAnnotation(roiAsAnnotation) + } catch { + logger.error(`could not add ROI "${roi.uid}"`) + } + } else { + logger.debug(`skip already existing ROI "${roi.uid}"`) + } + } else { + logger.debug( + `skip ROI "${roi.uid}" ` + + `of SR document "${report.SOPInstanceUID}"` + + 'because it is defined in another frame of reference', + ) + } + }) + } + /** * Retrieve Structured Report instances that contain regions of interests * with 3D spatial coordinates defined in the same frame of reference as the @@ -995,80 +1081,7 @@ class SlideViewer extends React.Component { sopInstanceUID: instance.SOPInstanceUID, }) .then((retrievedInstance): void => { - const data = - dcmjs.data.DicomMessage.readFile(retrievedInstance) - const { dataset } = dmv.metadata.formatMetadata(data.dict) - const report = - dataset as unknown as dmv.metadata.Comprehensive3DSR - /* - * Perform a couple of checks to ensure the document content of the - * report fullfils the requirements of the application. - */ - if (!implementsTID1500(report)) { - logger.debug( - `ignore SR document "${report.SOPInstanceUID}" ` + - 'because it is not structured according to template ' + - 'TID 1500 "MeasurementReport"', - ) - return - } - if (!describesSpecimenSubject(report)) { - logger.debug( - `ignore SR document "${report.SOPInstanceUID}" ` + - 'because it does not describe a specimen subject', - ) - return - } - if (!containsROIAnnotations(report)) { - logger.debug( - `ignore SR document "${report.SOPInstanceUID}" ` + - 'because it does not contain any suitable ROI annotations', - ) - return - } - - const content = new MeasurementReport(report) - content.ROIs.forEach((roi) => { - logger.log(`add ROI "${roi.uid}"`) - const scoord3d = roi.scoord3d - const image = this.props.slide.volumeImages[0] - if ( - scoord3d.frameOfReferenceUID === image.FrameOfReferenceUID - ) { - /* - * ROIs may get assigned new UIDs upon re-rendering of the - * page and we need to ensure that we don't add them twice. - * The same ROI may be stored in multiple SR documents and - * we don't want them to show up twice. - * TODO: We should probably either "merge" measurements and - * quantitative evaluations or pick the ROI from the "best" - * available report (COMPLETE and VERIFIED). - */ - const doesROIExist = this.volumeViewer - .getAllROIs() - .some((otherROI: dmv.roi.ROI): boolean => { - return areROIsEqual(otherROI, roi) - }) - if (!doesROIExist) { - try { - // Add ROI without style such that it won't be visible. - this.volumeViewer.addROI(roi, {}) - const roiAsAnnotation = adaptRoiToAnnotation(roi) - this.formatAnnotation(roiAsAnnotation) - } catch { - logger.error(`could not add ROI "${roi.uid}"`) - } - } else { - logger.debug(`skip already existing ROI "${roi.uid}"`) - } - } else { - logger.debug( - `skip ROI "${roi.uid}" ` + - `of SR document "${report.SOPInstanceUID}"` + - 'because it is defined in another frame of reference', - ) - } - }) + this.addRetrievedSrRois(retrievedInstance) resolve() }) .catch((error) => { @@ -1116,6 +1129,60 @@ class SlideViewer extends React.Component { }) } + /** + * Add retrieved Microscopy Bulk Simple Annotations metadata to the volume + * viewer and apply configured styles per annotation group. + */ + private readonly addRetrievedAnnotationGroups = ( + retrievedMetadata: dwc.api.Metadata[], + ): void => { + const annotations: dmv.metadata.MicroscopyBulkSimpleAnnotations[] = + retrievedMetadata.map((metadata) => { + return new dmv.metadata.MicroscopyBulkSimpleAnnotations({ + metadata, + }) + }) + annotations.forEach((ann) => { + try { + this.volumeViewer.addAnnotationGroups(ann) + } catch (error: unknown) { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + NotificationMiddleware.onError( + NotificationMiddlewareContext.SLIM, + new CustomError( + errorTypes.VISUALIZATION, + 'Microscopy Bulk Simple Annotations cannot be displayed.', + ), + ) + logger.error('failed to add annotation groups:', error) + } + ann.AnnotationGroupSequence.forEach((item) => { + const annotationGroupUID = item.AnnotationGroupUID + const finding = item.AnnotationPropertyTypeCodeSequence[0] + const key = buildKey(finding) + const style = this.roiStyles[key] + // eslint-disable-next-line @typescript-eslint/prefer-optional-chain + if ( + style !== null && + style !== undefined && + style.fill !== null && + style.fill !== undefined + ) { + this.volumeViewer.setAnnotationGroupStyle(annotationGroupUID, { + color: style.fill.color, + }) + } + }) + }) + /* + * React is not aware of the fact that annotation groups have been + * added via the viewer (the underlying HTML viewport element is a + * ref object) and won't show the annotation groups in the user + * interface unless an update is forced. + */ + this.forceUpdate() + } + /** * Retrieve Microscopy Bulk Simple Annotations instances that contain * annotation groups defined in the same frame of reference as the currently @@ -1165,52 +1232,7 @@ class SlideViewer extends React.Component { seriesInstanceUID: series.SeriesInstanceUID, }) .then((retrievedMetadata): void => { - const annotations: dmv.metadata.MicroscopyBulkSimpleAnnotations[] = - retrievedMetadata.map((metadata) => { - return new dmv.metadata.MicroscopyBulkSimpleAnnotations({ - metadata, - }) - }) - annotations.forEach((ann) => { - try { - this.volumeViewer.addAnnotationGroups(ann) - } catch (error: unknown) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - NotificationMiddleware.onError( - NotificationMiddlewareContext.SLIM, - new CustomError( - errorTypes.VISUALIZATION, - 'Microscopy Bulk Simple Annotations cannot be displayed.', - ), - ) - logger.error('failed to add annotation groups:', error) - } - ann.AnnotationGroupSequence.forEach((item) => { - const annotationGroupUID = item.AnnotationGroupUID - const finding = item.AnnotationPropertyTypeCodeSequence[0] - const key = buildKey(finding) - const style = this.roiStyles[key] - // eslint-disable-next-line @typescript-eslint/prefer-optional-chain - if ( - style !== null && - style !== undefined && - style.fill !== null && - style.fill !== undefined - ) { - this.volumeViewer.setAnnotationGroupStyle( - annotationGroupUID, - { color: style.fill.color }, - ) - } - }) - }) - /* - * React is not aware of the fact that annotation groups have been - * added via the viewer (the underlying HTML viewport element is a - * ref object) and won't show the annotation groups in the user - * interface unless an update is forced. - */ - this.forceUpdate() + this.addRetrievedAnnotationGroups(retrievedMetadata) finishOne() }) .catch((error) => { @@ -1252,6 +1274,49 @@ class SlideViewer extends React.Component { * frame of reference as the currently selected series and add them to the * VOLUME image viewer. */ + /** + * Add retrieved Segmentation metadata matching the current slide's frame of + * reference and container to the volume viewer. + */ + private readonly addRetrievedSegmentations = ( + retrievedMetadata: dwc.api.Metadata[], + ): void => { + const segmentations: dmv.metadata.Segmentation[] = [] + retrievedMetadata.forEach((metadata) => { + const seg = new dmv.metadata.Segmentation({ metadata }) + const refImage = this.props.slide.volumeImages[0] + if ( + seg.FrameOfReferenceUID === refImage.FrameOfReferenceUID && + seg.ContainerIdentifier === refImage.ContainerIdentifier + ) { + segmentations.push(seg) + } + }) + if (segmentations.length > 0) { + try { + this.volumeViewer.addSegments(segmentations) + applyDistinctFractionalSegmentPalettes(this.volumeViewer) + } catch (error: unknown) { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + NotificationMiddleware.onError( + NotificationMiddlewareContext.SLIM, + new CustomError( + errorTypes.VISUALIZATION, + 'Segmentations cannot be displayed', + ), + ) + console.error('failed to add segments: ', error) + } + /* + * React is not aware of the fact that segments have been added via + * the viewer (the underlying HTML viewport element is a ref object) + * and won't show the segments in the user interface unless an update + * is forced. + */ + this.forceUpdate() + } + } + addSegmentations = async (): Promise => { return await new Promise((resolve, reject) => { console.info('search for Segmentation instances') @@ -1297,40 +1362,7 @@ class SlideViewer extends React.Component { seriesInstanceUID: series.SeriesInstanceUID, }) .then((retrievedMetadata): void => { - const segmentations: dmv.metadata.Segmentation[] = [] - retrievedMetadata.forEach((metadata) => { - const seg = new dmv.metadata.Segmentation({ metadata }) - const refImage = this.props.slide.volumeImages[0] - if ( - seg.FrameOfReferenceUID === refImage.FrameOfReferenceUID && - seg.ContainerIdentifier === refImage.ContainerIdentifier - ) { - segmentations.push(seg) - } - }) - if (segmentations.length > 0) { - try { - this.volumeViewer.addSegments(segmentations) - applyDistinctFractionalSegmentPalettes(this.volumeViewer) - } catch (error: unknown) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - NotificationMiddleware.onError( - NotificationMiddlewareContext.SLIM, - new CustomError( - errorTypes.VISUALIZATION, - 'Segmentations cannot be displayed', - ), - ) - console.error('failed to add segments: ', error) - } - /* - * React is not aware of the fact that segments have been added via - * the viewer (the underlying HTML viewport element is a ref object) - * and won't show the segments in the user interface unless an update - * is forced. - */ - this.forceUpdate() - } + this.addRetrievedSegmentations(retrievedMetadata) finishOne() }) .catch((error) => { @@ -1371,6 +1403,51 @@ class SlideViewer extends React.Component { * frame of reference as the currently selected series and add them to the * VOLUME image viewer. */ + /** + * Add retrieved Parametric Map metadata matching the current slide's frame + * of reference and container to the volume viewer. + */ + private readonly addRetrievedParametricMaps = ( + retrievedMetadata: dwc.api.Metadata[], + ): void => { + const parametricMaps: dmv.metadata.ParametricMap[] = [] + retrievedMetadata.forEach((metadata) => { + const pm = new dmv.metadata.ParametricMap({ metadata }) + const refImage = this.props.slide.volumeImages[0] + if ( + pm.FrameOfReferenceUID === refImage.FrameOfReferenceUID && + pm.ContainerIdentifier === refImage.ContainerIdentifier + ) { + parametricMaps.push(pm) + } else { + console.warn(`skip Parametric Map instance "${pm.SOPInstanceUID}"`) + } + }) + if (parametricMaps.length > 0) { + try { + this.volumeViewer.addParameterMappings(parametricMaps) + applyDistinctParametricMapPalettes(this.volumeViewer) + } catch (error: unknown) { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + NotificationMiddleware.onError( + NotificationMiddlewareContext.SLIM, + new CustomError( + errorTypes.VISUALIZATION, + 'Parametric Map cannot be displayed', + ), + ) + console.error('failed to add mappings: ', error) + } + /* + * React is not aware of the fact that mappings have been added via + * the viewer (the underlying HTML viewport element is a ref object) + * and won't show the mappings in the user interface unless an update + * is forced. + */ + this.forceUpdate() + } + } + addParametricMaps = async (): Promise => { return await new Promise((resolve, reject) => { console.info('search for Parametric Map instances') @@ -1414,44 +1491,7 @@ class SlideViewer extends React.Component { seriesInstanceUID: series.SeriesInstanceUID, }) .then((retrievedMetadata): void => { - const parametricMaps: dmv.metadata.ParametricMap[] = [] - retrievedMetadata.forEach((metadata) => { - const pm = new dmv.metadata.ParametricMap({ metadata }) - const refImage = this.props.slide.volumeImages[0] - if ( - pm.FrameOfReferenceUID === refImage.FrameOfReferenceUID && - pm.ContainerIdentifier === refImage.ContainerIdentifier - ) { - parametricMaps.push(pm) - } else { - console.warn( - `skip Parametric Map instance "${pm.SOPInstanceUID}"`, - ) - } - }) - if (parametricMaps.length > 0) { - try { - this.volumeViewer.addParameterMappings(parametricMaps) - applyDistinctParametricMapPalettes(this.volumeViewer) - } catch (error: unknown) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - NotificationMiddleware.onError( - NotificationMiddlewareContext.SLIM, - new CustomError( - errorTypes.VISUALIZATION, - 'Parametric Map cannot be displayed', - ), - ) - console.error('failed to add mappings: ', error) - } - /* - * React is not aware of the fact that mappings have been added via - * the viewer (the underlying HTML viewport element is a ref object) - * and won't show the mappings in the user interface unless an update - * is forced. - */ - this.forceUpdate() - } + this.addRetrievedParametricMaps(retrievedMetadata) finishOne() }) .catch((error) => { diff --git a/src/utils/xhrRetryHook.ts b/src/utils/xhrRetryHook.ts index 2caa7f76..7d398d72 100644 --- a/src/utils/xhrRetryHook.ts +++ b/src/utils/xhrRetryHook.ts @@ -15,7 +15,7 @@ type RequestHook = ( * (e.g. STOW POST) are excluded because re-sending them can duplicate * partially stored data on a server that failed mid-request. */ -const RETRYABLE_METHODS = ['GET', 'HEAD', 'OPTIONS'] +const RETRYABLE_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']) /** * Returns a configured retry request hook function @@ -66,7 +66,7 @@ export const getXHRRetryHook = ( ): XMLHttpRequest => { const { url, method } = metadata - if (!RETRYABLE_METHODS.includes(method.toUpperCase())) { + if (!RETRYABLE_METHODS.has(method.toUpperCase())) { return request } From b4a59a9a0d306ba875697ea1d4accd73a02fbf1b Mon Sep 17 00:00:00 2001 From: Igor Octaviano Date: Fri, 31 Jul 2026 20:03:22 -0300 Subject: [PATCH 6/9] fix: do not resolve addAnnotations for ignored SR documents The SonarCloud extraction changed control flow: the original early returns for ignored SR documents (not TID 1500, wrong subject, no ROIs) exited the retrieval handler before resolve(), but the extracted call site resolved unconditionally. That let addAnnotations settle on the first retrieved instance even when it was rejected, re-opening the derived-data load-order race. addRetrievedSrRois now reports whether the report was accepted and the promise only resolves when it was, matching the pre-refactoring behavior exactly. --- src/components/SlideViewer.tsx | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/components/SlideViewer.tsx b/src/components/SlideViewer.tsx index 770a4729..17dfdea2 100644 --- a/src/components/SlideViewer.tsx +++ b/src/components/SlideViewer.tsx @@ -968,11 +968,14 @@ class SlideViewer extends React.Component { /** * Parse a retrieved Comprehensive 3D SR instance and add the ROIs of a - * suitable measurement report to the volume viewer. + * suitable measurement report to the volume viewer. Returns whether the + * report was accepted: ignored documents must not settle the promise in + * addAnnotations (matching the pre-refactoring control flow, where the + * early returns skipped resolve()). */ private readonly addRetrievedSrRois = ( retrievedInstance: dwc.api.Dataset, - ): void => { + ): boolean => { const data = dcmjs.data.DicomMessage.readFile(retrievedInstance) const { dataset } = dmv.metadata.formatMetadata(data.dict) const report = dataset as unknown as dmv.metadata.Comprehensive3DSR @@ -986,21 +989,21 @@ class SlideViewer extends React.Component { 'because it is not structured according to template ' + 'TID 1500 "MeasurementReport"', ) - return + return false } if (!describesSpecimenSubject(report)) { logger.debug( `ignore SR document "${report.SOPInstanceUID}" ` + 'because it does not describe a specimen subject', ) - return + return false } if (!containsROIAnnotations(report)) { logger.debug( `ignore SR document "${report.SOPInstanceUID}" ` + 'because it does not contain any suitable ROI annotations', ) - return + return false } const content = new MeasurementReport(report) @@ -1043,6 +1046,7 @@ class SlideViewer extends React.Component { ) } }) + return true } /** @@ -1081,8 +1085,9 @@ class SlideViewer extends React.Component { sopInstanceUID: instance.SOPInstanceUID, }) .then((retrievedInstance): void => { - this.addRetrievedSrRois(retrievedInstance) - resolve() + if (this.addRetrievedSrRois(retrievedInstance)) { + resolve() + } }) .catch((error) => { // eslint-disable-next-line @typescript-eslint/no-floating-promises From 41c9b942cef2bf0da86f2c6c6d7881e1dc593fd0 Mon Sep 17 00:00:00 2001 From: Igor Octaviano Date: Fri, 31 Jul 2026 21:10:44 -0300 Subject: [PATCH 7/9] test: add unit tests for XHR retry hook --- src/utils/__tests__/xhrRetryHook.test.ts | 160 +++++++++++++++++++++++ 1 file changed, 160 insertions(+) create mode 100644 src/utils/__tests__/xhrRetryHook.test.ts diff --git a/src/utils/__tests__/xhrRetryHook.test.ts b/src/utils/__tests__/xhrRetryHook.test.ts new file mode 100644 index 00000000..10ea83db --- /dev/null +++ b/src/utils/__tests__/xhrRetryHook.test.ts @@ -0,0 +1,160 @@ +import type { RetryRequestSettings } from '../../AppConfig' +import getXHRRetryHook from '../xhrRetryHook' + +/** + * Minimal XHR stand-in exposing only the surface the retry hook touches. + * Each send() completes asynchronously with the next status from the queue, + * mirroring how a real XHR invokes onreadystatechange at DONE. + */ +class FakeXHR { + readyState = 0 + status = 0 + responseType: XMLHttpRequestResponseType = '' + onreadystatechange: ((ev: Event) => void) | null = null + statusQueue: number[] = [] + sendCount = 0 + openCalls: string[] = [] + headers: { [key: string]: string } = {} + + open(method: string, _url: string, _async: boolean): void { + this.openCalls.push(method) + /** A real XHR clears author request headers on open(). */ + this.headers = {} + } + + setRequestHeader(key: string, value: string): void { + this.headers[key] = value + } + + send(): void { + this.sendCount += 1 + const status = this.statusQueue.shift() ?? 200 + setTimeout(() => { + this.readyState = XMLHttpRequest.DONE + this.status = status + this.onreadystatechange?.(new Event('readystatechange')) + }, 0) + } +} + +const flush = async (ms = 50): Promise => { + return await new Promise((resolve) => setTimeout(resolve, ms)) +} + +const fastRetryOptions = { + retries: 2, + factor: 1, + minTimeout: 1, + maxTimeout: 1, + randomize: false, +} + +const applyHook = ( + xhr: FakeXHR, + method: string, + options: RetryRequestSettings = fastRetryOptions, +): void => { + getXHRRetryHook(options)(xhr as unknown as XMLHttpRequest, { + url: 'https://example.com/studies', + method, + headers: { Accept: 'application/dicom+json' }, + }) +} + +describe('getXHRRetryHook', () => { + beforeEach(() => { + jest.spyOn(console, 'warn').mockImplementation(() => {}) + }) + + afterEach(() => { + jest.restoreAllMocks() + }) + + it('does not wrap send for non-idempotent methods', () => { + const xhr = new FakeXHR() + applyHook(xhr, 'POST') + expect(Object.prototype.hasOwnProperty.call(xhr, 'send')).toBe(false) + }) + + it('passes a success straight through to the client handler', async () => { + const xhr = new FakeXHR() + xhr.statusQueue = [200] + const clientHandler = jest.fn() + xhr.onreadystatechange = clientHandler + applyHook(xhr, 'GET') + + xhr.send() + await flush() + + expect(xhr.sendCount).toBe(1) + expect(clientHandler).toHaveBeenCalledTimes(1) + expect(xhr.status).toBe(200) + }) + + it('does not retry non-retryable failure statuses', async () => { + const xhr = new FakeXHR() + xhr.statusQueue = [404] + const clientHandler = jest.fn() + xhr.onreadystatechange = clientHandler + applyHook(xhr, 'GET') + + xhr.send() + await flush() + + expect(xhr.sendCount).toBe(1) + expect(clientHandler).toHaveBeenCalledTimes(1) + expect(xhr.status).toBe(404) + }) + + it('re-sends on retryable statuses and only surfaces the final success', async () => { + const xhr = new FakeXHR() + xhr.statusQueue = [500, 429, 200] + xhr.responseType = 'arraybuffer' + const clientHandler = jest.fn() + xhr.onreadystatechange = clientHandler + applyHook(xhr, 'GET') + + xhr.send() + await flush() + + expect(xhr.sendCount).toBe(3) + /** The client handler must not observe the intermediate failures. */ + expect(clientHandler).toHaveBeenCalledTimes(1) + expect(xhr.status).toBe(200) + /** Retries re-open the request and restore headers and responseType. */ + expect(xhr.openCalls).toEqual(['GET', 'GET']) + expect(xhr.headers).toEqual({ Accept: 'application/dicom+json' }) + expect(xhr.responseType).toBe('arraybuffer') + }) + + it('surfaces the final failure once retries are exhausted', async () => { + const xhr = new FakeXHR() + xhr.statusQueue = [500, 500, 500] + const clientHandler = jest.fn() + xhr.onreadystatechange = clientHandler + applyHook(xhr, 'GET') + + xhr.send() + await flush() + + /** Initial attempt plus the two configured retries. */ + expect(xhr.sendCount).toBe(3) + expect(clientHandler).toHaveBeenCalledTimes(1) + expect(xhr.status).toBe(500) + }) + + it('honors custom retryable status codes', async () => { + const xhr = new FakeXHR() + xhr.statusQueue = [500] + const clientHandler = jest.fn() + xhr.onreadystatechange = clientHandler + applyHook(xhr, 'GET', { ...fastRetryOptions, retryableStatusCodes: [429] }) + + xhr.send() + await flush() + + /** 500 is not retryable under the custom configuration. */ + expect(xhr.sendCount).toBe(1) + expect(clientHandler).toHaveBeenCalledTimes(1) + }) +}) From c48f0dbb8d1ec42ecdb6e5997eb865a655e25ca3 Mon Sep 17 00:00:00 2001 From: Igor Octaviano Date: Fri, 31 Jul 2026 21:13:07 -0300 Subject: [PATCH 8/9] fix: capture retry downstream handler at send time Capturing onreadystatechange when the retry hook installs skipped wrappers from later requestHooks (e.g. Viv abort suppress). Read the handler at send() instead so the chain stays intact. --- src/utils/__tests__/xhrRetryHook.test.ts | 27 ++++++++++++++++++++++++ src/utils/xhrRetryHook.ts | 12 +++++++---- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/utils/__tests__/xhrRetryHook.test.ts b/src/utils/__tests__/xhrRetryHook.test.ts index 10ea83db..61257bb5 100644 --- a/src/utils/__tests__/xhrRetryHook.test.ts +++ b/src/utils/__tests__/xhrRetryHook.test.ts @@ -157,4 +157,31 @@ describe('getXHRRetryHook', () => { expect(xhr.sendCount).toBe(1) expect(clientHandler).toHaveBeenCalledTimes(1) }) + + it('preserves onreadystatechange wrappers installed after the retry hook', async () => { + const xhr = new FakeXHR() + xhr.statusQueue = [0] + const clientHandler = jest.fn() + xhr.onreadystatechange = clientHandler + applyHook(xhr, 'GET') + + /** + * Mimic a later requestHook (e.g. Viv abort suppress) that wraps the + * handler between retry-hook install and send(). + */ + const prev = xhr.onreadystatechange + const laterWrapper = jest.fn(function (this: FakeXHR, ev: Event) { + if (this.readyState === XMLHttpRequest.DONE && this.status === 0) { + return + } + prev?.call(this, ev) + }) + xhr.onreadystatechange = laterWrapper + + xhr.send() + await flush() + + expect(laterWrapper).toHaveBeenCalledTimes(1) + expect(clientHandler).not.toHaveBeenCalled() + }) }) diff --git a/src/utils/xhrRetryHook.ts b/src/utils/xhrRetryHook.ts index 7d398d72..057fcd00 100644 --- a/src/utils/xhrRetryHook.ts +++ b/src/utils/xhrRetryHook.ts @@ -74,13 +74,17 @@ export const getXHRRetryHook = ( const originalRequestSend = request.send /** Captured at send(); re-applied after retry open() for safety. */ let responseType: XMLHttpRequestResponseType = request.responseType - /** dicomweb-client handler installed before this hook runs. */ - const clientOnReadyStateChange = request.onreadystatechange function faultTolerantRequestSend( ...args: Parameters ): void { responseType = request.responseType + /** + * Capture at send() — not when this hook runs — so later requestHooks + * (e.g. Viv's abort suppress wrapper) that wrap onreadystatechange + * between hook install and send() stay in the call chain. + */ + const downstreamOnReadyStateChange = request.onreadystatechange const operation = retry.operation({ retries: retryOptions.retries, factor: retryOptions.factor, @@ -119,8 +123,8 @@ export const getXHRRetryHook = ( return } - if (clientOnReadyStateChange != null) { - clientOnReadyStateChange.call(request, ev) + if (downstreamOnReadyStateChange != null) { + downstreamOnReadyStateChange.call(request, ev) } } From d767215a8e7ea6ed13d19f84a13ec1463aa26794 Mon Sep 17 00:00:00 2001 From: Igor Octaviano Date: Fri, 31 Jul 2026 21:17:32 -0300 Subject: [PATCH 9/9] test: satisfy DeepSource on console.warn mock --- src/utils/__tests__/xhrRetryHook.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/__tests__/xhrRetryHook.test.ts b/src/utils/__tests__/xhrRetryHook.test.ts index 61257bb5..0f1176ec 100644 --- a/src/utils/__tests__/xhrRetryHook.test.ts +++ b/src/utils/__tests__/xhrRetryHook.test.ts @@ -63,7 +63,7 @@ const applyHook = ( describe('getXHRRetryHook', () => { beforeEach(() => { - jest.spyOn(console, 'warn').mockImplementation(() => {}) + jest.spyOn(console, 'warn').mockImplementation(() => undefined) }) afterEach(() => {