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/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..17dfdea2 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 } = {} @@ -429,6 +429,11 @@ 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() } @@ -436,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. @@ -499,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( @@ -954,6 +966,89 @@ 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. 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, + ): boolean => { + 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 false + } + if (!describesSpecimenSubject(report)) { + logger.debug( + `ignore SR document "${report.SOPInstanceUID}" ` + + 'because it does not describe a specimen subject', + ) + return false + } + if (!containsROIAnnotations(report)) { + logger.debug( + `ignore SR document "${report.SOPInstanceUID}" ` + + 'because it does not contain any suitable ROI annotations', + ) + return false + } + + 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', + ) + } + }) + return true + } + /** * Retrieve Structured Report instances that contain regions of interests * with 3D spatial coordinates defined in the same frame of reference as the @@ -990,81 +1085,9 @@ 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 (this.addRetrievedSrRois(retrievedInstance)) { + resolve() } - 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', - ) - } - }) - resolve() }) .catch((error) => { // eslint-disable-next-line @typescript-eslint/no-floating-promises @@ -1111,6 +1134,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 @@ -1160,52 +1237,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) => { @@ -1247,6 +1279,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') @@ -1292,40 +1367,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) => { @@ -1366,6 +1408,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') @@ -1409,44 +1496,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/__tests__/xhrRetryHook.test.ts b/src/utils/__tests__/xhrRetryHook.test.ts new file mode 100644 index 00000000..0f1176ec --- /dev/null +++ b/src/utils/__tests__/xhrRetryHook.test.ts @@ -0,0 +1,187 @@ +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(() => undefined) + }) + + 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) + }) + + 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 6c295b69..057fcd00 100644 --- a/src/utils/xhrRetryHook.ts +++ b/src/utils/xhrRetryHook.ts @@ -10,60 +10,45 @@ 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 = new Set(['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, 502, 503, 504, + ], } /** @@ -81,41 +66,72 @@ export const getXHRRetryHook = ( ): XMLHttpRequest => { const { url, method } = metadata + if (!RETRYABLE_METHODS.has(method.toUpperCase())) { + return request + } + + const headers = metadata.headers ?? {} + const originalRequestSend = request.send + /** Captured at send(); re-applied after retry open() for safety. */ + let responseType: XMLHttpRequestResponseType = request.responseType + function faultTolerantRequestSend( ...args: Parameters ): void { - const operation = retry.operation(retryOptions) + 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, + 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() empties author request headers; re-apply those + responseType. */ + 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 (downstreamOnReadyStateChange != null) { + downstreamOnReadyStateChange.call(request, ev) + } } - }) - originalRequestSend.apply(request, args) + originalRequestSend.apply(request, args) + }) } - /** Overriding/extending XHR function */ - const originalRequestSend = request.send request.send = faultTolerantRequestSend return request