react-viewer: firmware update proposal + one-click update#15391
react-viewer: firmware update proposal + one-click update#15391sareluzi wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds firmware-update recommendations and a one-click “download + install” flow to the React viewer by pulling recommended versions from the online versions DB (mirroring the C++ viewer), surfacing the proposal UI, and extending progress reporting to include a download phase.
Changes:
- Backend: fetch & cache
rs_versions_db.json, compute firmware status + download link, and add anupdate_from_recommendedendpoint that downloads (cached) and reuses the existing DFU flashing path. - Frontend: show an “outdated” firmware proposal with dismiss/remind behaviors, add one-click Update, and display a “downloading” phase in the progress modal.
- Tests: add Python unit tests for version comparison / DB matching and extend React unit tests for proposal + dismissal behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wrappers/rest-api/tools/react-viewer/tests/unit/store/store.test.ts | Adds unit tests for firmware-dismissal revival behavior in the store. |
| wrappers/rest-api/tools/react-viewer/tests/unit/components/DevicePanel.test.tsx | Adds UI tests for firmware update proposal visibility and dismissal flows. |
| wrappers/rest-api/tools/react-viewer/src/store/index.ts | Adds shared firmware-update driver, recommended-update action, and explicit firmware-check semantics. |
| wrappers/rest-api/tools/react-viewer/src/components/FirmwareProgressModal.tsx | Extends progress modal to distinguish downloading vs installing and only mark done after install. |
| wrappers/rest-api/tools/react-viewer/src/components/DevicePanel.tsx | Implements proposal banner UI, dismissal persistence, and one-click recommended update trigger. |
| wrappers/rest-api/tools/react-viewer/src/api/types.ts | Extends firmware state with link and phase. |
| wrappers/rest-api/tools/react-viewer/src/api/socket.ts | Forces a full device re-enumeration on devices_changed events. |
| wrappers/rest-api/tools/react-viewer/src/api/client.ts | Extends firmware progress callback with phase, adds firmware-status link, and adds recommended-update endpoint call. |
| wrappers/rest-api/tests/test_firmware_status.py | Adds backend tests for version comparison and versions DB matching rules. |
| wrappers/rest-api/app/services/rs_manager.py | Implements versions DB fetch/cache, recommendation matching, firmware download/cache, and recommended-update flashing path. |
| wrappers/rest-api/app/api/endpoints/firmware.py | Adds REST endpoint for “update from recommended”. |
| SERVER_VERSIONS_DB_URL = "https://librealsense.realsenseai.com/Releases/rs_versions_db.json" | ||
| _VERSIONS_DB_TTL = 3600.0 # seconds | ||
| _versions_db_cache: Dict[str, Any] = {"ts": 0.0, "entries": None} | ||
|
|
There was a problem hiding this comment.
Fixed — the versions-DB cache is now guarded by a module-level threading.Lock in firmware.py _fetch_versions_db.
| def _fetch_versions_db(): | ||
| """Return the DB 'versions' list (cached ~1h), or None on any network/parse failure.""" | ||
| now = time.monotonic() | ||
| cached = _versions_db_cache["entries"] | ||
| if cached is not None and now - _versions_db_cache["ts"] < _VERSIONS_DB_TTL: | ||
| return cached | ||
| try: | ||
| with urllib.request.urlopen(SERVER_VERSIONS_DB_URL, timeout=10) as resp: | ||
| data = json.loads(resp.read().decode("utf-8")) | ||
| entries = data.get("versions", []) | ||
| _versions_db_cache.update(ts=now, entries=entries) | ||
| return entries | ||
| except Exception as e: # network down, timeout, bad JSON — degrade gracefully | ||
| logging.warning("Could not fetch firmware versions DB: %s", e) | ||
| return None |
There was a problem hiding this comment.
Fixed — the same lock guards both read and populate, so no partially-updated state is observed. Also dropped the TTL (fetched once per process).
| os.makedirs(_FW_DOWNLOAD_DIR, exist_ok=True) | ||
| name = os.path.basename(urllib.parse.urlparse(url).path) or "firmware.bin" | ||
| path = os.path.join(_FW_DOWNLOAD_DIR, name) | ||
| if os.path.isfile(path) and os.path.getsize(path) > 0: | ||
| if on_progress: | ||
| on_progress(1.0) | ||
| with open(path, "rb") as f: | ||
| return f.read() | ||
| try: | ||
| buf = bytearray() | ||
| with urllib.request.urlopen(url, timeout=30) as resp: | ||
| total = int(resp.headers.get("Content-Length") or 0) |
There was a problem hiding this comment.
Fixed — download_firmware validates the scheme is http(s) with a netloc and rejects anything else (file://, relative).
| resetStore() | ||
| }) | ||
|
|
||
| describe('Firmware dismissal', () => { |
There was a problem hiding this comment.
Fixed — added an afterEach in the dismissal describe that clears both localStorage and sessionStorage.
| // Check for a firmware-update proposal (online versions DB; best-effort, non-blocking) | ||
| get().checkFirmwareUpdates(device.device_id) |
There was a problem hiding this comment.
Fixed — the on-activate check is now best-effort: it no longer raises the global error banner; only an explicit user-triggered check surfaces the error.
| } | ||
|
|
||
| function FirmwareBanner({ device: _device }: FirmwareBannerProps) { | ||
| function FirmwareBanner({ deviceId, firmware, onUpdate }: FirmwareBannerProps) { |
There was a problem hiding this comment.
Design wise, I think it'd be better to reuse the Toast popups we already have - their action would be 'update' or the built-in 'x' to dismiss - on load read /status per device and add toast if needed. Same pattern as the "Enable metadata" prompt.
Making this change will allow us to revert other changes like checkFirmwareUpdates, or alternatively make it re-show that toast.
When clicking the 'check for updates' button, indication of 'Up to date' in a Toast when relevant is also nice-to-have
There was a problem hiding this comment.
Agreed this is a nicer pattern. It's a larger UX redesign though — it would touch/revert the banner and the checkFirmwareUpdates/retry logic — so I'd rather not fold it into this PR. Deferring to a separate follow-up PR to keep this one focused.
There was a problem hiding this comment.
It'd revert the banner, yes — but the toast is a smaller net diff than the banner + dual-storage dismissal + checkFirmwareUpdates it replaces (~−110 lines; it reuses the existing "Enable metadata" toast). Splitting it into a follow-up is more total work, not less: that PR has to revert code this one just added, then go through review again. Cheaper to fold it in here while the code's fresh.
There was a problem hiding this comment.
@AviaAv - I implemented the banner as you suggested but I'm not sure it's good. If you have multiple cameras connected the banner pops at the global level making it unclear which camera should be updated. We can add the S/N to the toast pop-up, but it feels a bit cumbersome.
@Nir-Az - what do you think?
There was a problem hiding this comment.
If we add the SN, both flows are OK IMO, I am used to the C++ viewer pop up, but if it's hard to implement it's not a must.
| // Shared driver for both firmware-update paths (user file + recommended download): | ||
| // flips is_updating, runs the API call (progress arrives via Socket.IO), refreshes | ||
| // device info, and records any failure on the device's firmware state. | ||
| async function performFirmwareUpdate( |
There was a problem hiding this comment.
Great idea to extract logic into this function, but i don't think the 'attempt' logic is needed - previous version would await get().fetchDevices() - seems more suitable here too
This function will also be simplified if we use a Toast to prompt for updating
There was a problem hiding this comment.
Simplified, but kept a short bounded retry: a single await fetchDevices(true) can be dropped by the isLoadingDevices guard when a devices_changed fetch races, leaving the UI stale. Also added post-flash auto-initialize of a lone camera (same as first-load) + a firmware re-check so the proposal reflects the new FW.
There was a problem hiding this comment.
The toggle-active / FW re-check on the returned device is good. But the attempt loop's still there, and it worked fine without it before. Is there a specific case you think needs the retry? The backend doesn't return until the device is back, so a single fetchDevices(true) should always catch it — the UI shouldn't be left stale.
… validation, best-effort check) + auto-init single camera and re-check FW after flash
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| raise RealSenseError(status_code=400, detail="Refusing to download firmware from a non-http(s) URL") | ||
| try: | ||
| buf = bytearray() | ||
| with urllib.request.urlopen(url, timeout=30) as resp: |
There was a problem hiding this comment.
Potential user input in HTTP request may allow SSRF attack - low severity
If an attacker can control the URL input leading into this HTTP request, the attack might be able to perform an SSRF attack. This kind of attack is even more dangerous if the application returns the response of the request to the user. It could allow them to retrieve information from higher privileged services within the network (such as the metadata service, which is commonly available in cloud services, and could allow them to retrieve credentials).
Show fix
Remediation: If possible, only allow requests to allowlisting domains. If not, consult the article linked above to learn about other mitigating techniques such as disabling redirects, blocking private IPs and making sure private services have internal authentication. If you return data coming from the request to the user, validate the data before returning it to make sure you don't return random data.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| def recommended_firmware(device_name): | ||
| """Recommended FIRMWARE (version, link) for a device from the online DB, or (None, None).""" | ||
| return _pick_recommended_fw(_fetch_versions_db(), device_name, platform.system()) |
| Pure logic with no pyrealsense2 dependency, so tests can import it directly without | ||
| loading the SDK. The SDK no longer bundles firmware, so the recommended version is | ||
| looked up from the online versions DB (mirrors the C++ viewer's server_versions_db_url), | ||
| and the image is downloaded into memory and flashed from there (like the C++ viewer's | ||
| download_to_bytes_vector) — no temp files. |
| import asyncio | ||
| import os | ||
| import platform | ||
| import struct |
| with urllib.request.urlopen(SERVER_VERSIONS_DB_URL, timeout=10) as resp: | ||
| data = json.loads(resp.read().decode("utf-8")) | ||
| entries = data.get("versions", []) | ||
| _versions_db_cache["entries"] = entries |
There was a problem hiding this comment.
🤖 [rs-agentic] BUG — empty DB cached permanently
If the server returns a well-formed JSON with an empty versions array (e.g. CDN hiccup, wrong endpoint), entries = [] is stored here. Because the guard above checks is not None, an empty list passes it and will be returned forever — silently suppressing all firmware proposals for the process lifetime.
Consider only caching when entries is non-empty:
if entries:
_versions_db_cache["entries"] = entriesOr distinguish between "not yet fetched" and "fetched empty" with an explicit sentinel.
| if not data: | ||
| raise RealSenseError(status_code=502, detail="Downloaded firmware image is empty") | ||
| if on_progress: | ||
| on_progress(1.0) |
There was a problem hiding this comment.
🤖 [rs-agentic] Minor — on_progress(1.0) called twice
When Content-Length is known and the last chunk fills the buffer exactly, on_progress(min(len(buf)/total, 1.0)) inside the loop already fires 1.0. The unconditional call here emits it again. Rate-limiting in _make_fw_progress_callback lets p == 1.0 through unconditionally, so two 100% events are emitted. Harmless but noisy on the socket.
Review round addressed (commit 7f7d8f8)Replied inline on each thread. Summary: Security / correctness
Refactor (AviaAv)
Tests
Also fixed two behaviors found while testing: after a flash, a lone camera now auto-initializes (like first load) and the firmware proposal is re-checked. Deferred: the Toast-based proposal UX (AviaAv) — larger redesign, tracked for a separate follow-up PR. |
Overview: The react-viewer now proposes a firmware update when a newer version is recommended, and offers one-click download-and-install — restoring update notifications after the SDK stopped bundling firmware.
Tracked on [RSDEV-12698]
Why
When the SDK dropped the bundled firmware,
RS2_CAMERA_INFO_RECOMMENDED_FIRMWARE_VERSIONis no longer reported by devices, so the viewer could never propose an update. Recommendations must instead come from the online versions DB (mirroring the C++ viewer).Summary
get_firmware_statusfetches the onliners_versions_db.json(cached), matches the device (prefix-agnostic name +*wildcard, most-specific wins, mirrors the C++versions_db_manager), and returns the recommended version, status (outdated/up_to_date/unknown), and download link.update_from_recommendedendpoint downloads the recommended.bin(cached under the OS temp dir, size-capped) and reuses the existing DFU flash flow.downloadingphase with progress beforeinstalling, shown in the progress modal.devices_changednow forces a refresh) so the camera reappears without a manual Refresh.outdated; hidden whenup_to_date. Dismiss via X (permanent until a newer FW) or Remind me later (session). An explicit "Check for Firmware Updates" revives a dismissed proposal.Test plan
"Intel "-prefix regression and non-FIRMWARE/non-RECOMMENDED exclusion (12 tests)./statusreturns recommended/outdated for a D455; download fetched a real 5.17.3.10 image with monotonic progress to a temp cache.🤖 Generated by AI