fix: make XHR retry hook actually retry and keep ICC switch state in sync - #402
Open
igoroctaviano wants to merge 10 commits into
Open
fix: make XHR retry hook actually retry and keep ICC switch state in sync#402igoroctaviano wants to merge 10 commits into
igoroctaviano wants to merge 10 commits into
Conversation
…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.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Aug 1, 2026 12:18a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
|
Visit the preview URL for this PR (updated for commit d767215): https://idc-external-006--pr402-fix-code-review-mast-1xjjfq21.web.app (expires Sat, 08 Aug 2026 00:20:13 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 88aacecd98ba54d2f9c8d201a9444e43d1ad8307 |
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.
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.
open() does not clear responseType; we re-apply it after retry open() for safety. Align the JSDoc with that.
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.
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.
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.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two fixes for pre-existing bugs on
master, found during a code review of thefeat/viv-loaderwork:src/utils/xhrRetryHook.ts— the retry hook never actually retried. On a retryable status it re-open()ed the XHR but never re-send()ed it, and the client'sonreadystatechangewas invoked on the first failed attempt anyway, so the promise settled with the first failure and the retry machinery was dead weight. The hook now re-opens, restores headers/responseType(both cleared byopen()), and re-sends, only surfacing the outcome todicomweb-clientonce retries are exhausted or the request succeeds. Retries are skipped for non-idempotent methods (e.g. STOWPOST, which could duplicate partially stored instances), and default backoff is tuned for interactive use (3 retries, factor 2, 10s max timeout) so persistent failures surface in seconds rather than minutes.retryableStatusCodesis now optional inRetryRequestSettingssince the hook provides a default, and the hook metadata type documents theheadersfield thatdicomweb-clientalready passes.src/components/SlideViewer.tsx— ICC profiles switch desynced after a slide switch.componentDidUpdatedestroys and recreatesvolumeViewerwhen the route/series changes, and a fresh viewer always starts with ICC profiles enabled, butisICCProfilesEnabledwas not reset. After toggling ICC off and switching slides, the viewer rendered with ICC on while the settings switch showed off — and the switch then acted inverted. The state is now reset alongside the other per-viewer state.Test plan
tsc --noEmitpassesbiome check .passesretryconfigured, verify a 429/500 endpoint is retried with backoff and that errors surface after backoff is exhausted