From 05100fdb1b7cbd93f4383f5d55e706078edd897a Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sat, 15 Aug 2026 17:18:51 +0200 Subject: [PATCH 1/5] fix: reach every recording a fault kept, not just the newest A fault can now hold several black-box recordings. The detail refetches on expand so one written while the page is open is reachable, the dashboard keys its cache by entity as well as code so two entities reporting the same code stop showing each other's evidence, each download button names its recording, and the saved file keeps the extension the gateway put on it. Adds a Playwright stack with a fault manager and specs covering all of it. --- e2e/docker-compose.rosbag.yml | 43 ++++++ e2e/gateway/rosbag-params.yaml | 36 +++++ e2e/gateway/seed_recordings.py | 124 +++++++++++++++++ e2e/rosbag-recordings.spec.ts | 168 ++++++++++++++++++++++++ playwright.config.ts | 4 + src/components/FaultsDashboard.tsx | 28 +++- src/components/FaultsPanel.tsx | 11 +- src/components/RosbagDownloadButton.tsx | 6 + src/lib/store-download.test.ts | 58 ++++++++ src/lib/store.ts | 48 +++++-- 10 files changed, 509 insertions(+), 17 deletions(-) create mode 100644 e2e/docker-compose.rosbag.yml create mode 100644 e2e/gateway/rosbag-params.yaml create mode 100644 e2e/gateway/seed_recordings.py create mode 100644 e2e/rosbag-recordings.spec.ts create mode 100644 src/lib/store-download.test.ts diff --git a/e2e/docker-compose.rosbag.yml b/e2e/docker-compose.rosbag.yml new file mode 100644 index 0000000..cf58141 --- /dev/null +++ b/e2e/docker-compose.rosbag.yml @@ -0,0 +1,43 @@ +# Stack for the rosbag-history specs: a gateway AND a fault manager, so a fault +# can actually own black-box recordings. docker-compose.yml next to this file +# runs a manifest-only gateway with no fault manager at all, which cannot +# produce a single bag; the two scenarios are kept apart rather than merged so +# neither has to carry the other's configuration. +services: + gateway: + # Overridable because the recording-id contract these specs assert on + # (ros2_medkit#620) is newer than any published tag: point this at a + # locally built image to run them before that lands. Once it is + # published, pin a digest here the way docker-compose.yml does. + image: ${E2E_ROSBAG_GATEWAY_IMAGE:-ghcr.io/selfpatch/ros2_medkit-jazzy:latest} + ports: + # Loopback only, and on its own port so this stack can run alongside + # the scripts one without either stealing the other's. + - '127.0.0.1:${E2E_ROSBAG_GATEWAY_PORT:-8081}:8080' + volumes: + - ./gateway/rosbag-params.yaml:/e2e/params.yaml:ro + - ./gateway/seed_recordings.py:/e2e/seed_recordings.py:ro + - e2e-bags:/e2e-bags + entrypoint: ['/bin/bash', '-lc'] + command: + - > + source /opt/ros/jazzy/setup.bash && + source /home/medkit/ws/install/setup.bash && + ros2 run ros2_medkit_fault_manager fault_manager_node + --ros-args --params-file /e2e/params.yaml & + ros2 run ros2_medkit_gateway gateway_node + --ros-args --params-file /e2e/params.yaml + depends_on: + init-bags: + condition: service_completed_successfully + # The gateway image runs as uid 999 and a fresh named volume is root-owned, + # so the fault manager could not write a bag into it. Same one-shot chown + # the scripts stack does for its upload volume. + init-bags: + image: ${E2E_ROSBAG_GATEWAY_IMAGE:-ghcr.io/selfpatch/ros2_medkit-jazzy:latest} + user: root + volumes: + - e2e-bags:/e2e-bags + entrypoint: ['chown', '-R', '999:999', '/e2e-bags'] +volumes: + e2e-bags: diff --git a/e2e/gateway/rosbag-params.yaml b/e2e/gateway/rosbag-params.yaml new file mode 100644 index 0000000..2ac6c06 --- /dev/null +++ b/e2e/gateway/rosbag-params.yaml @@ -0,0 +1,36 @@ +# Gateway + fault manager for the rosbag-history scenario. +# +# Separate from params.yaml on purpose: that stack is manifest-only with script +# uploads and no fault manager at all, and this one needs the opposite - live +# ROS discovery so the seeded fault's reporting source resolves to an app, and a +# fault manager configured to keep a HISTORY of black-box recordings rather than +# overwriting on every re-confirmation. +/**: + ros__parameters: + server: + host: '0.0.0.0' + port: 8080 + cors: + # Same reasoning as params.yaml: the browser's origin is the dev + # server, not the gateway, and E2E_APP_URL is overridable. + allowed_origins: + - '*' + # Rosbag retention. 3 leaves headroom above the two occurrences the seed + # drives, so a failing spec means "a recording was lost", not "the cap + # trimmed one". + snapshots: + rosbag: + enabled: true + duration_sec: 2.0 + duration_after_sec: 0.5 + include_topics: ['/e2e/probe'] + format: 'mcap' + storage_path: '/e2e-bags' + max_bags_per_fault: 3 + # Acknowledging a fault must not delete the evidence it just + # produced - the scenario is confirm, acknowledge, confirm again. + auto_cleanup: false + lazy_start: false + confirmation_threshold: -1 + storage_type: 'sqlite' + database_path: '/e2e-bags/faults.db' diff --git a/e2e/gateway/seed_recordings.py b/e2e/gateway/seed_recordings.py new file mode 100644 index 0000000..51cc9fb --- /dev/null +++ b/e2e/gateway/seed_recordings.py @@ -0,0 +1,124 @@ +#!/usr/bin/env python3 +# Copyright 2026 mfaferek93 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Leave one fault holding two black-box recordings, for the browser to click on. + +Everything below the fault report is real: the fault manager runs its own +capture, records a topic that is genuinely being published, and writes two +separate bags to disk. Only the trigger is a service call rather than a sensor +detecting its own misconfiguration - the subject of these specs is the web UI, +and the demo nodes that detect faults on their own are not shipped in the +gateway image. + +The fault is confirmed, acknowledged, then confirmed again: that is the sequence +that used to leave a single recording behind, because the second one overwrote +the first (ros2_medkit#620). +""" + +import sys +import time + +import rclpy +from rclpy.node import Node +from rclpy.qos import HistoryPolicy, QoSProfile, ReliabilityPolicy +from ros2_medkit_msgs.msg import Fault +from ros2_medkit_msgs.srv import ClearFault, ReportFault +from std_msgs.msg import Float32 + +FAULT_CODE = 'E2E_FLAPPING_SENSOR' +SOURCE_ID = '/e2e/probe_publisher' +PROBE_TOPIC = '/e2e/probe' +# Must exceed the configured duration_sec so the ring buffer holds a full window +# before each confirmation; a bag flushed from an empty buffer has no content. +FILL_SECONDS = 3.0 + + +class Seeder(Node): + def __init__(self): + super().__init__('e2e_rosbag_seeder') + qos = QoSProfile( + reliability=ReliabilityPolicy.BEST_EFFORT, + history=HistoryPolicy.KEEP_LAST, + depth=10, + ) + self.pub = self.create_publisher(Float32, PROBE_TOPIC, qos) + self.report = self.create_client(ReportFault, '/fault_manager/report_fault') + self.clear = self.create_client(ClearFault, '/fault_manager/clear_fault') + + def wait_for_services(self, timeout=90.0): + for client, name in ((self.report, 'report_fault'), (self.clear, 'clear_fault')): + if not client.wait_for_service(timeout_sec=timeout): + raise SystemExit(f'{name} service never appeared') + + def publish_for(self, seconds, rate_hz=20.0): + msg = Float32() + msg.data = 1.0 + deadline = time.time() + seconds + period = 1.0 / rate_hz + while time.time() < deadline: + self.pub.publish(msg) + rclpy.spin_once(self, timeout_sec=0.0) + time.sleep(period) + + def call(self, client, request): + future = client.call_async(request) + rclpy.spin_until_future_complete(self, future, timeout_sec=20.0) + if future.result() is None: + raise SystemExit('service call timed out') + return future.result() + + def confirm(self): + request = ReportFault.Request() + request.fault_code = FAULT_CODE + request.event_type = ReportFault.Request.EVENT_FAILED + request.severity = Fault.SEVERITY_ERROR + request.description = 'Intermittent sensor dropout seen twice' + request.source_id = SOURCE_ID + return self.call(self.report, request) + + def acknowledge(self): + request = ClearFault.Request() + request.fault_code = FAULT_CODE + return self.call(self.clear, request) + + +def main(): + rclpy.init() + node = Seeder() + node.wait_for_services() + + # First occurrence. + node.publish_for(FILL_SECONDS) + node.confirm() + node.publish_for(FILL_SECONDS) # post-roll window, then finalize + node.acknowledge() + + # Second occurrence. Before #620 this one replaced the first recording + # outright, so the fault ended up with exactly one bag either way. + node.publish_for(FILL_SECONDS) + node.confirm() + node.publish_for(FILL_SECONDS) + + # Deliberately NOT acknowledged: a cleared fault drops out of the default + # CONFIRMED-only listing, so acknowledging this one too would leave the specs + # with two bags on disk and no fault on screen pointing at them. + print('SEEDED', flush=True) + node.destroy_node() + rclpy.shutdown() + return 0 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/e2e/rosbag-recordings.spec.ts b/e2e/rosbag-recordings.spec.ts new file mode 100644 index 0000000..0271d7b --- /dev/null +++ b/e2e/rosbag-recordings.spec.ts @@ -0,0 +1,168 @@ +// Copyright 2026 mfaferek93 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/** + * A fault that owns several black-box recordings, end to end in a browser. + * + * An intermittent fault leaves one recording per occurrence. Until + * ros2_medkit#620 the newest overwrote the previous one, so the occurrence an + * engineer actually wanted to look at was already gone by the time they opened + * the fault. These specs drive the real UI against a real gateway holding two + * real bags and assert the technician can reach both of them. + * + * Runs against e2e/docker-compose.rosbag.yml, a separate stack from the scripts + * one: it needs a fault manager, which that stack does not run. + */ + +import { expect, test } from '@playwright/test'; + +const GATEWAY_PORT = process.env.E2E_ROSBAG_GATEWAY_PORT ?? '8081'; +const GATEWAY_URL = process.env.E2E_ROSBAG_GATEWAY_URL ?? `http://localhost:${GATEWAY_PORT}/api/v1`; +const STORAGE_KEY = 'ros2_medkit_web_ui_server_url'; +const FAULT_CODE = process.env.E2E_ROSBAG_FAULT_CODE ?? 'E2E_FLAPPING_SENSOR'; + +interface Descriptor { + id: string; + 'x-medkit'?: { fault_codes?: string[]; recording_id?: string }; +} + +/** Recording ids the gateway attributes to the seeded fault, via its own API. */ +async function recordingsFromApi(appId: string): Promise { + const response = await fetch(`${GATEWAY_URL}/apps/${appId}/bulk-data/rosbags`); + if (!response.ok) return []; + const body = (await response.json()) as { items?: Descriptor[] }; + return (body.items ?? []) + .filter((item) => item['x-medkit']?.fault_codes?.includes(FAULT_CODE)) + .map((item) => item.id); +} + +/** The app the seeded fault is attributed to, whatever the gateway named it. */ +async function appHoldingTheFault(): Promise { + const response = await fetch(`${GATEWAY_URL}/apps`); + if (!response.ok) return null; + const body = (await response.json()) as { items?: Array<{ id: string }> }; + for (const app of body.items ?? []) { + const faults = await fetch(`${GATEWAY_URL}/apps/${app.id}/faults`); + if (!faults.ok) continue; + const listing = (await faults.json()) as { items?: Array<{ fault_code?: string }> }; + if ((listing.items ?? []).some((f) => f.fault_code === FAULT_CODE)) return app.id; + } + return null; +} + +let appId: string | null = null; +let expectedRecordings: string[] = []; + +test.beforeAll(async () => { + appId = await appHoldingTheFault(); + if (appId) expectedRecordings = await recordingsFromApi(appId); +}); + +// Skipped rather than failed when the stack is not up or predates the +// recording-id contract: a red suite over a missing fixture says nothing about +// this repo, and these specs are the first to need a gateway new enough to keep +// more than one bag per fault. +test.beforeEach(async ({ page }) => { + test.skip( + appId === null || expectedRecordings.length < 2, + `needs e2e/docker-compose.rosbag.yml up with ${FAULT_CODE} seeded and holding ` + + `at least two recordings (found ${expectedRecordings.length} on ${GATEWAY_URL})` + ); + // Point the app at the rosbag stack instead of the scripts one that global + // setup seeded, before any application code runs. The stored value is a + // zustand-persist envelope, not a bare URL - writing the raw string leaves + // the app unable to parse it and sitting on the connection dialog. + await page.addInitScript( + ([key, url]) => window.localStorage.setItem(key, JSON.stringify({ state: { serverUrl: url }, version: 0 })), + [STORAGE_KEY, GATEWAY_URL] as const + ); +}); + +async function openTheFault(page: import('@playwright/test').Page) { + await page.goto('/', { waitUntil: 'load' }); + await page.getByRole('button', { name: /Faults Dashboard/i }).click(); + await expect(page.getByText(FAULT_CODE).first()).toBeVisible(); + await page.getByText(FAULT_CODE).first().click(); +} + +/** The fault's rosbag download buttons, in the order the detail lists them. */ +function downloadButtons(page: import('@playwright/test').Page) { + return page.locator('button:has(svg.lucide-download)'); +} + +test('the fault detail shows every recording, not just the newest', async ({ page }) => { + await openTheFault(page); + + // One download button per recording. Before #620 the gateway could only ever + // report one, so this is the assertion the whole change exists for. + await expect(downloadButtons(page)).toHaveCount(expectedRecordings.length); + + // Each button names the recording it will fetch. Without that the icon + // buttons are indistinguishable - to a screen reader they were nameless, and + // sighted users saw N identical download icons with nothing to tell them + // apart. + const names = await downloadButtons(page).evaluateAll((els) => + els.map((el) => el.getAttribute('aria-label') ?? '') + ); + expect(new Set(names).size).toBe(expectedRecordings.length); + for (const recordingId of expectedRecordings) { + expect(names.some((name) => name.includes(recordingId))).toBe(true); + } +}); + +test('every recording downloads as its own bag', async ({ page }) => { + await openTheFault(page); + const buttons = downloadButtons(page); + await expect(buttons).toHaveCount(expectedRecordings.length); + + const filenames: string[] = []; + for (let i = 0; i < expectedRecordings.length; i += 1) { + const [download] = await Promise.all([page.waitForEvent('download'), buttons.nth(i).click()]); + const name = download.suggestedFilename(); + filenames.push(name); + + // The gateway names the file and is the only party that knows the + // storage format. Saving under the descriptor's display label instead + // dropped the extension, landing a bag on disk that neither the OS nor + // `ros2 bag play` could open without a manual rename. + expect(name).toMatch(/\.(mcap|db3)$/); + + const path = await download.path(); + expect(path).toBeTruthy(); + } + + // Distinct files, not the same bag served twice under different buttons. + expect(new Set(filenames).size).toBe(expectedRecordings.length); +}); + +test('a recording that appears while the fault is open is reachable without a reload', async ({ page }) => { + // The detail used to be fetched once per fault and cached forever, so a + // recording written after the first expand stayed invisible until the + // component remounted - which for a technician watching a machine fault + // again is exactly the recording they are waiting for. + await openTheFault(page); + await expect(downloadButtons(page)).toHaveCount(expectedRecordings.length); + + // Collapse and re-expand: the second expand must go back to the gateway + // rather than replay the first response. + let refetched = false; + page.on('response', (response) => { + if (response.url().includes(`/faults/${FAULT_CODE}`)) refetched = true; + }); + + await page.getByText(FAULT_CODE).first().click(); + await page.getByText(FAULT_CODE).first().click(); + await expect(downloadButtons(page)).toHaveCount(expectedRecordings.length); + expect(refetched).toBe(true); +}); diff --git a/playwright.config.ts b/playwright.config.ts index ec1207c..b2925f6 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -50,5 +50,9 @@ export default defineConfig({ // concurrency limit), so they must not run in parallel with each other. { name: 'scripts-serial', testMatch: /(scripts|smoke)\.spec\.ts/, fullyParallel: false, workers: 1 }, { name: 'mocked', testMatch: /.*-errors\.spec\.ts/ }, + // Its own stack (e2e/docker-compose.rosbag.yml) on its own port, because + // it needs a fault manager the scripts gateway does not run. Serial for + // the same reason as scripts-serial: one shared gateway. + { name: 'rosbag-serial', testMatch: /rosbag-.*\.spec\.ts/, fullyParallel: false, workers: 1 }, ], }); diff --git a/src/components/FaultsDashboard.tsx b/src/components/FaultsDashboard.tsx index 45b5d15..baefcff 100644 --- a/src/components/FaultsDashboard.tsx +++ b/src/components/FaultsDashboard.tsx @@ -344,7 +344,7 @@ function FaultGroup({ isClearing={clearingCodes.has(fault.code)} isExpanded={expandedFaults.has(fault.code)} onToggle={() => onToggleFault(fault)} - environmentData={faultDetails.get(fault.code)?.environment_data} + environmentData={faultDetails.get(faultKey(fault))?.environment_data} isLoadingDetails={loadingDetails.has(fault.code)} /> ))} @@ -393,6 +393,20 @@ function DashboardSkeleton() { * Uses shared faults state from useAppStore to avoid duplicate API calls * when both FaultsDashboard and FaultsCountBadge are visible. */ +/** + * Key for the per-fault caches in this dashboard. + * + * The dashboard lists faults from every entity at once, and a fault code is only + * unique within one entity - two apps can both report `LIDAR_RANGE_INVALID`. + * Keying the detail cache by code alone made the second entity's row render the + * first entity's environment data, i.e. download buttons pointing at another + * entity's recordings. Including the entity makes the key as specific as the + * request that filled it. + */ +function faultKey(fault: { code: string; entity_type?: string; entity_id?: string }): string { + return `${fault.entity_type ?? ''}/${fault.entity_id ?? ''}/${fault.code}`; +} + export function FaultsDashboard() { const [isRefreshing, setIsRefreshing] = useState(false); const [autoRefresh, setAutoRefresh] = useState(true); @@ -493,13 +507,15 @@ export function FaultsDashboard() { } else { newExpanded.add(faultCode); - // Fetch details if not cached - if (!faultDetails.has(faultCode)) { + // Always refetch: a fault gains recordings while the page is open, + // and a cache filled once on first expand would keep serving the + // shorter list. The previous entry stays rendered meanwhile. + { setLoadingDetails((prev) => new Set([...prev, faultCode])); try { const entityGroup = mapFaultEntityTypeToResourceType(fault.entity_type); const details = await getFaultWithEnvironmentData(entityGroup, fault.entity_id, faultCode); - setFaultDetails((prev) => new Map(prev).set(faultCode, details as FaultResponse)); + setFaultDetails((prev) => new Map(prev).set(faultKey(fault), details as FaultResponse)); } catch (err) { console.error('Failed to fetch fault details:', err); } finally { @@ -514,7 +530,7 @@ export function FaultsDashboard() { setExpandedFaults(newExpanded); }, - [getFaultWithEnvironmentData, expandedFaults, faultDetails] + [getFaultWithEnvironmentData, expandedFaults] ); // Filter faults @@ -825,7 +841,7 @@ export function FaultsDashboard() { isClearing={clearingCodes.has(fault.code)} isExpanded={expandedFaults.has(fault.code)} onToggle={() => handleToggleFault(fault)} - environmentData={faultDetails.get(fault.code)?.environment_data} + environmentData={faultDetails.get(faultKey(fault))?.environment_data} isLoadingDetails={loadingDetails.has(fault.code)} /> ))} diff --git a/src/components/FaultsPanel.tsx b/src/components/FaultsPanel.tsx index 85aa1ae..0744e83 100644 --- a/src/components/FaultsPanel.tsx +++ b/src/components/FaultsPanel.tsx @@ -309,8 +309,13 @@ export function FaultsPanel({ entityId, entityType = 'components' }: FaultsPanel } else { newExpanded.add(faultCode); - // Fetch details if not cached - if (!faultDetails.has(faultCode)) { + // Always refetch, even when a detail is already cached. A fault can + // gain recordings while the page is open - it re-confirms, the black + // box is written, the snapshot list grows - and a cache that is + // filled once on first expand would keep serving the older list with + // no way to refresh short of remounting. The stale entry stays + // rendered until the new one lands, so re-expanding never flickers. + { setLoadingDetails((prev) => new Set([...prev, faultCode])); try { // Use the fault's own entity info (app-level) for correct bulk_data_uri. @@ -337,7 +342,7 @@ export function FaultsPanel({ entityId, entityType = 'components' }: FaultsPanel setExpandedFaults(newExpanded); }, - [getFaultWithEnvironmentData, entityType, entityId, expandedFaults, faultDetails, faults] + [getFaultWithEnvironmentData, entityType, entityId, expandedFaults, faults] ); const handleClear = useCallback( diff --git a/src/components/RosbagDownloadButton.tsx b/src/components/RosbagDownloadButton.tsx index 7fded98..b36d9ac 100644 --- a/src/components/RosbagDownloadButton.tsx +++ b/src/components/RosbagDownloadButton.tsx @@ -84,6 +84,11 @@ export function RosbagDownloadButton({ snapshot, variant = 'outline', size = 'sm } const label = snapshot.size_bytes ? `Download (${formatBytes(snapshot.size_bytes)})` : 'Download rosbag'; + // A fault can hold several recordings, so the icon variant renders as N + // buttons with no text at all - identical to a screen reader and to keyboard + // navigation. Name each one after the recording it downloads so they can be + // told apart; snapshot.name carries the recording id. + const accessibleName = snapshot.name ? `${label} - ${snapshot.name}` : label; return ( @@ -93,6 +98,7 @@ export function RosbagDownloadButton({ snapshot, variant = 'outline', size = 'sm size={size} onClick={handleDownload} disabled={isDownloading} + aria-label={accessibleName} className={error ? 'border-destructive' : ''} > {isDownloading ? : } diff --git a/src/lib/store-download.test.ts b/src/lib/store-download.test.ts new file mode 100644 index 0000000..0bea934 --- /dev/null +++ b/src/lib/store-download.test.ts @@ -0,0 +1,58 @@ +// Copyright 2026 mfaferek93 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { describe, expect, it } from 'vitest'; + +import { filenameFromContentDisposition } from './store'; + +describe('filenameFromContentDisposition', () => { + it('takes the quoted filename the gateway sends for a rosbag', () => { + // The extension is the point: only the server knows the storage format, + // and a bag saved without `.mcap` is one the OS and `ros2 bag play` + // cannot open until the user renames it by hand. + expect(filenameFromContentDisposition('attachment; filename="fault_MOTOR_OVERHEAT_1738664999000.mcap"')).toBe( + 'fault_MOTOR_OVERHEAT_1738664999000.mcap' + ); + }); + + it('accepts an unquoted filename', () => { + expect(filenameFromContentDisposition('attachment; filename=bag.db3')).toBe('bag.db3'); + }); + + it('prefers the RFC 5987 form, which is the one that survives non-ASCII', () => { + expect( + filenameFromContentDisposition('attachment; filename="fallback.mcap"; filename*=UTF-8\'\'r%C3%B6ntgen.mcap') + ).toBe('röntgen.mcap'); + }); + + it('falls back to the plain form when the extended one is malformed', () => { + // A truncated percent-escape throws inside decodeURIComponent; the plain + // filename is still perfectly usable and must not be lost with it. + expect(filenameFromContentDisposition('attachment; filename="good.mcap"; filename*=UTF-8\'\'bad%ZZ')).toBe( + 'good.mcap' + ); + }); + + it('returns null rather than a filename when the header says nothing', () => { + // Null is what lets the caller fall back to the recording id. Returning + // an empty string here would save the file as "" instead. + expect(filenameFromContentDisposition(null)).toBeNull(); + expect(filenameFromContentDisposition('attachment')).toBeNull(); + expect(filenameFromContentDisposition('attachment; filename=""')).toBeNull(); + }); + + it('is case-insensitive about the parameter name', () => { + expect(filenameFromContentDisposition('attachment; FileName="x.mcap"')).toBe('x.mcap'); + }); +}); diff --git a/src/lib/store.ts b/src/lib/store.ts index 380d789..f7bc6fc 100644 --- a/src/lib/store.ts +++ b/src/lib/store.ts @@ -50,7 +50,6 @@ import { putEntityDataItem, deleteEntityConfiguration, deleteEntityConfigurations, - getEntityBulkData, getEntityLogs, getEntityLogsConfiguration, putEntityLogsConfiguration, @@ -901,6 +900,38 @@ async function fetchEntityFromApi( } } +/** + * Filename the server chose, out of a `Content-Disposition` header. + * + * Handles both the plain `filename="x"` form and RFC 5987's `filename*=UTF-8''x`, + * preferring the latter when present because that is the one that survives + * non-ASCII. Returns null when the header is absent or names nothing, so the + * caller can fall back rather than saving a file called "null". + */ +export function filenameFromContentDisposition(header: string | null): string | null { + if (!header) return null; + + const extended = /filename\*\s*=\s*(?:UTF-8|utf-8)''([^;]+)/i.exec(header); + if (extended?.[1]) { + try { + const decoded = decodeURIComponent(extended[1].trim()); + if (decoded) return decoded; + } catch { + // Malformed percent-encoding: fall through to the plain form. + } + } + + // One alternative, then unquote: matching the quoted form separately means a + // `filename=""` falls through to the unquoted branch and comes back as the + // two literal quote characters instead of as "no name". + const plain = /filename\s*=\s*([^;]+)/i.exec(header); + const name = plain?.[1] + ?.trim() + .replace(/^"(.*)"$/, '$1') + .trim(); + return name ? name : null; +} + export const useAppStore = create()( persist( (set, get) => ({ @@ -2550,13 +2581,6 @@ export const useAppStore = create()( const { client, serverUrl } = get(); if (!client || !serverUrl) return null; - // Fetch file list to get filename - const { data } = await getEntityBulkData(client, entityType, entityId, category); - if (!data) return null; - const items = (data as unknown as { items?: Array<{ id: string; name?: string }> })?.items || []; - const fileDesc = items.find((item) => item.id === fileId); - const filename = fileDesc?.name || fileId; - // Download binary via fetch (openapi-fetch doesn't support blob responses) const baseUrl = normalizeBaseUrl(serverUrl); const downloadUrl = `${baseUrl}/${entityType}/${encodeURIComponent(entityId)}/bulk-data/${encodeURIComponent(category)}/${encodeURIComponent(fileId)}`; @@ -2567,6 +2591,14 @@ export const useAppStore = create()( clearTimeout(timer); if (!response.ok) return null; const blob = await response.blob(); + // The server names the file, and it is the only party that knows + // the storage format, so only it can put the right extension on + // the end. The descriptor's `name` is a human label (" + // recording "), not a filename: saving under it lands + // a rosbag on disk with no `.mcap`/`.db3` at all, which neither + // the OS nor `ros2 bag play` can make sense of. + const filename = + filenameFromContentDisposition(response.headers.get('content-disposition')) ?? fileId; return { blob, filename }; } catch { clearTimeout(timer); From 45a04979e11745240d4b57a7a943548ae239a174 Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sun, 16 Aug 2026 13:05:29 +0200 Subject: [PATCH 2/5] test(e2e): prove each recording downloads its own bytes Distinct filenames alone would pass on a build that resolved both ids to one recording and labelled the responses differently. --- e2e/rosbag-recordings.spec.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/e2e/rosbag-recordings.spec.ts b/e2e/rosbag-recordings.spec.ts index 0271d7b..49096d8 100644 --- a/e2e/rosbag-recordings.spec.ts +++ b/e2e/rosbag-recordings.spec.ts @@ -25,6 +25,8 @@ * one: it needs a fault manager, which that stack does not run. */ +import { readFileSync } from 'node:fs'; + import { expect, test } from '@playwright/test'; const GATEWAY_PORT = process.env.E2E_ROSBAG_GATEWAY_PORT ?? '8081'; @@ -127,6 +129,7 @@ test('every recording downloads as its own bag', async ({ page }) => { await expect(buttons).toHaveCount(expectedRecordings.length); const filenames: string[] = []; + const payloads: Buffer[] = []; for (let i = 0; i < expectedRecordings.length; i += 1) { const [download] = await Promise.all([page.waitForEvent('download'), buttons.nth(i).click()]); const name = download.suggestedFilename(); @@ -140,10 +143,22 @@ test('every recording downloads as its own bag', async ({ page }) => { const path = await download.path(); expect(path).toBeTruthy(); + payloads.push(readFileSync(path!)); } // Distinct files, not the same bag served twice under different buttons. expect(new Set(filenames).size).toBe(expectedRecordings.length); + + // And distinct BYTES. Names alone would still pass on a build that resolved + // both ids to one recording but labelled the responses differently; this is + // what proves each button fetched its own occurrence. + expect(new Set(payloads.map((b) => b.toString('base64'))).size).toBe(expectedRecordings.length); + for (const payload of payloads) { + expect(payload.length).toBeGreaterThan(0); + // Every bag the fixture records is mcap; the magic is the cheapest proof + // that what arrived is a bag and not an error page. + expect(payload.subarray(0, 5)).toEqual(Buffer.from([0x89, 0x4d, 0x43, 0x41, 0x50])); + } }); test('a recording that appears while the fault is open is reachable without a reload', async ({ page }) => { From 2ed0bb1eba760e3f5fc493442d33f2504a710ac6 Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sun, 16 Aug 2026 15:11:15 +0200 Subject: [PATCH 3/5] test(e2e): skip the rosbag specs when their stack is absent A fetch in beforeAll threw before the skip guard could run, so CI went red on a missing fixture rather than reporting it as skipped. --- e2e/rosbag-recordings.spec.ts | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/e2e/rosbag-recordings.spec.ts b/e2e/rosbag-recordings.spec.ts index 49096d8..6f12026 100644 --- a/e2e/rosbag-recordings.spec.ts +++ b/e2e/rosbag-recordings.spec.ts @@ -39,10 +39,23 @@ interface Descriptor { 'x-medkit'?: { fault_codes?: string[]; recording_id?: string }; } +/** fetch that answers null instead of throwing when nothing is listening. */ +async function safeFetch(url: string): Promise { + try { + return await fetch(url); + } catch { + return null; + } +} + /** Recording ids the gateway attributes to the seeded fault, via its own API. */ async function recordingsFromApi(appId: string): Promise { - const response = await fetch(`${GATEWAY_URL}/apps/${appId}/bulk-data/rosbags`); - if (!response.ok) return []; + // Network errors are swallowed here and in appHoldingTheFault so a stack that + // is not up leaves expectedRecordings empty and the specs SKIP with a named + // reason. Letting fetch throw out of beforeAll fails them instead, which says + // nothing about this repo and turns CI red on a missing fixture. + const response = await safeFetch(`${GATEWAY_URL}/apps/${appId}/bulk-data/rosbags`); + if (!response?.ok) return []; const body = (await response.json()) as { items?: Descriptor[] }; return (body.items ?? []) .filter((item) => item['x-medkit']?.fault_codes?.includes(FAULT_CODE)) @@ -51,12 +64,12 @@ async function recordingsFromApi(appId: string): Promise { /** The app the seeded fault is attributed to, whatever the gateway named it. */ async function appHoldingTheFault(): Promise { - const response = await fetch(`${GATEWAY_URL}/apps`); - if (!response.ok) return null; + const response = await safeFetch(`${GATEWAY_URL}/apps`); + if (!response?.ok) return null; const body = (await response.json()) as { items?: Array<{ id: string }> }; for (const app of body.items ?? []) { - const faults = await fetch(`${GATEWAY_URL}/apps/${app.id}/faults`); - if (!faults.ok) continue; + const faults = await safeFetch(`${GATEWAY_URL}/apps/${app.id}/faults`); + if (!faults?.ok) continue; const listing = (await faults.json()) as { items?: Array<{ fault_code?: string }> }; if ((listing.items ?? []).some((f) => f.fault_code === FAULT_CODE)) return app.id; } From 2bf28d85f952aecce5720758d30b91ac773b6861 Mon Sep 17 00:00:00 2001 From: Michal Faferek Date: Thu, 20 Aug 2026 17:14:12 +0200 Subject: [PATCH 4/5] fix(ui): address multi-rosbag review round Per-entity faultKey shared by the dashboard and the entity panel - expand, loading, clearing and the detail cache no longer tie colliding codes together, clear takes the Fault, and expansion opens before the refetch without blanking cached evidence on a 404. RFC 8187 charset+language filename parsing. The e2e stack gets its own compose project, sources ROS in the parent shell, runs the seeder as a watched job, and the seeder survives for discovery and checks both service responses. --- e2e/docker-compose.rosbag.yml | 42 +++++++- e2e/gateway/seed_recordings.py | 63 +++++++++--- e2e/rosbag-recordings.spec.ts | 24 +++-- src/components/FaultsDashboard.test.tsx | 116 +++++++++++++++++++++ src/components/FaultsDashboard.tsx | 129 ++++++++++++------------ src/components/FaultsPanel.tsx | 97 ++++++++++-------- src/lib/store.ts | 57 +++++++---- src/lib/utils.ts | 13 +++ 8 files changed, 387 insertions(+), 154 deletions(-) create mode 100644 src/components/FaultsDashboard.test.tsx diff --git a/e2e/docker-compose.rosbag.yml b/e2e/docker-compose.rosbag.yml index cf58141..d5333b0 100644 --- a/e2e/docker-compose.rosbag.yml +++ b/e2e/docker-compose.rosbag.yml @@ -3,6 +3,12 @@ # runs a manifest-only gateway with no fault manager at all, which cannot # produce a single bag; the two scenarios are kept apart rather than merged so # neither has to carry the other's configuration. + +# Its own project: both compose files live in e2e/, so without this they share +# the default project name "e2e" and their `gateway` services replace each other +# instead of running side by side. +name: e2e-rosbag + services: gateway: # Overridable because the recording-id contract these specs assert on @@ -18,15 +24,44 @@ services: - ./gateway/rosbag-params.yaml:/e2e/params.yaml:ro - ./gateway/seed_recordings.py:/e2e/seed_recordings.py:ro - e2e-bags:/e2e-bags + # PID 1 reaps children and forwards signals; without it bash -lc keeps + # PID 1 for itself and `docker compose down` waits out the whole grace + # period before SIGKILLing a fault manager mid-write. + init: true + # Overriding the entrypoint skips /entrypoint.sh, which is what sources + # ROS and exports the RMW default - both have to be restored here. + environment: + RMW_IMPLEMENTATION: ${RMW_IMPLEMENTATION:-rmw_fastrtps_cpp} entrypoint: ['/bin/bash', '-lc'] + # Fault manager, the seeder and the gateway in one container. Not three + # services sharing a network: the default DDS transport uses /dev/shm, + # which is per container, so the seeder's service calls would never + # complete even though discovery says the service is there. + # + # Sourced ONCE in the parent shell, then every process runs as a WATCHED + # background job: `&` binds looser than `&&`, so the earlier + # `source && source && fault_manager & gateway` form left the gateway in + # an unsourced shell ("ros2: command not found", exit 127). `wait -n` + # returns when the FIRST job dies, so a fault manager that cannot open + # its DB or a seeder that raises SystemExit takes the container down + # with its exit code instead of leaving a healthy-looking stack whose + # specs skip. The trap makes SIGTERM stop the children before the shell + # exits. command: - > source /opt/ros/jazzy/setup.bash && source /home/medkit/ws/install/setup.bash && ros2 run ros2_medkit_fault_manager fault_manager_node --ros-args --params-file /e2e/params.yaml & + FM=$!; + python3 /e2e/seed_recordings.py & + SEED=$!; ros2 run ros2_medkit_gateway gateway_node - --ros-args --params-file /e2e/params.yaml + --ros-args --params-file /e2e/params.yaml & + GW=$!; + trap 'kill $FM $SEED $GW 2>/dev/null' TERM INT; + wait -n $FM $SEED $GW; + exit $? depends_on: init-bags: condition: service_completed_successfully @@ -39,5 +74,10 @@ services: volumes: - e2e-bags:/e2e-bags entrypoint: ['chown', '-R', '999:999', '/e2e-bags'] + volumes: + # Holds the bags AND faults.db, and it outlives `docker compose down`. + # Re-seed from a clean slate with `down -v` first: on a reused volume the + # fault is already CONFIRMED, the first confirm captures nothing, and the + # suite sees three recordings instead of two. e2e-bags: diff --git a/e2e/gateway/seed_recordings.py b/e2e/gateway/seed_recordings.py index 51cc9fb..0ff7a2d 100644 --- a/e2e/gateway/seed_recordings.py +++ b/e2e/gateway/seed_recordings.py @@ -38,7 +38,12 @@ from std_msgs.msg import Float32 FAULT_CODE = 'E2E_FLAPPING_SENSOR' -SOURCE_ID = '/e2e/probe_publisher' +# The node's own fully qualified name. The gateway attributes a fault to the app +# whose FQN matches its reporting source, so a source that belongs to no live +# node leaves the fault owned by nobody and invisible under any /apps/{id} - +# which is also why this node stays up afterwards instead of exiting. +NODE_NAME = 'e2e_rosbag_seeder' +SOURCE_ID = f'/{NODE_NAME}' PROBE_TOPIC = '/e2e/probe' # Must exceed the configured duration_sec so the ring buffer holds a full window # before each confirmation; a bag flushed from an empty buffer has no content. @@ -47,7 +52,7 @@ class Seeder(Node): def __init__(self): - super().__init__('e2e_rosbag_seeder') + super().__init__(NODE_NAME) qos = QoSProfile( reliability=ReliabilityPolicy.BEST_EFFORT, history=HistoryPolicy.KEEP_LAST, @@ -72,12 +77,24 @@ def publish_for(self, seconds, rate_hz=20.0): rclpy.spin_once(self, timeout_sec=0.0) time.sleep(period) - def call(self, client, request): - future = client.call_async(request) - rclpy.spin_until_future_complete(self, future, timeout_sec=20.0) - if future.result() is None: - raise SystemExit('service call timed out') - return future.result() + def call(self, client, request, attempts=5): + # Retried rather than one-shot: wait_for_service returns as soon as the + # service is advertised, which under DDS is before the fault manager has + # finished coming up, so the very first call can time out on a server + # that is seconds away from being fine. + for _ in range(attempts): + future = client.call_async(request) + rclpy.spin_until_future_complete(self, future, timeout_sec=20.0) + result = future.result() + if result is not None: + return result + # A future that outlived its timeout must not stay in flight: the + # request is not idempotent, and a late completion next to the retry + # would hand the fault manager two EVENT_FAILED reports for one + # occurrence. + future.cancel() + time.sleep(2.0) + raise SystemExit('service call timed out after retries') def confirm(self): request = ReportFault.Request() @@ -86,18 +103,31 @@ def confirm(self): request.severity = Fault.SEVERITY_ERROR request.description = 'Intermittent sensor dropout seen twice' request.source_id = SOURCE_ID - return self.call(self.report, request) + response = self.call(self.report, request) + if not response.accepted: + # ReportFault's response carries no message field; accepted=False + # means the request itself was invalid. + raise SystemExit('ReportFault rejected the request as invalid') + return response def acknowledge(self): request = ClearFault.Request() request.fault_code = FAULT_CODE - return self.call(self.clear, request) + response = self.call(self.clear, request) + # A silent "Fault not found" here would leave one bag on disk and the + # whole suite skipping, with only a DEBUG log line to say why. + if not response.success: + raise SystemExit(f'ClearFault failed: {response.message}') + return response def main(): rclpy.init() node = Seeder() node.wait_for_services() + # Let discovery settle before the first report; the gateway is coming up in + # the same window and a confirmation raced against it produces no bag. + time.sleep(5.0) # First occurrence. node.publish_for(FILL_SECONDS) @@ -115,8 +145,17 @@ def main(): # CONFIRMED-only listing, so acknowledging this one too would leave the specs # with two bags on disk and no fault on screen pointing at them. print('SEEDED', flush=True) - node.destroy_node() - rclpy.shutdown() + + # Stay on the graph. The fault is attributed to this node, so letting it + # exit would take the owning app entity with it and the fault would stop + # being reachable under any /apps/{id}. + try: + rclpy.spin(node) + except KeyboardInterrupt: + pass + finally: + node.destroy_node() + rclpy.shutdown() return 0 diff --git a/e2e/rosbag-recordings.spec.ts b/e2e/rosbag-recordings.spec.ts index 6f12026..720968f 100644 --- a/e2e/rosbag-recordings.spec.ts +++ b/e2e/rosbag-recordings.spec.ts @@ -174,23 +174,25 @@ test('every recording downloads as its own bag', async ({ page }) => { } }); -test('a recording that appears while the fault is open is reachable without a reload', async ({ page }) => { +test('re-expanding the fault asks the gateway again instead of replaying a cache', async ({ page }) => { // The detail used to be fetched once per fault and cached forever, so a // recording written after the first expand stayed invisible until the - // component remounted - which for a technician watching a machine fault - // again is exactly the recording they are waiting for. + // component remounted. This drives the collapse/re-expand path and pins + // that the second expand goes back to the gateway with a 2xx; seeding a + // THIRD recording mid-test would need a second seeder pass, so the + // count-grows half lives in the jsdom tests that stub the store. await openTheFault(page); await expect(downloadButtons(page)).toHaveCount(expectedRecordings.length); - // Collapse and re-expand: the second expand must go back to the gateway - // rather than replay the first response. - let refetched = false; - page.on('response', (response) => { - if (response.url().includes(`/faults/${FAULT_CODE}`)) refetched = true; - }); - + // Collapse. await page.getByText(FAULT_CODE).first().click(); + + // Re-expand, armed BEFORE the click and only satisfied by a 2xx: an error + // response must not count as "refetched". + const refetch = page.waitForResponse( + (response) => response.url().includes(`/faults/${FAULT_CODE}`) && response.ok() + ); await page.getByText(FAULT_CODE).first().click(); + await refetch; await expect(downloadButtons(page)).toHaveCount(expectedRecordings.length); - expect(refetched).toBe(true); }); diff --git a/src/components/FaultsDashboard.test.tsx b/src/components/FaultsDashboard.test.tsx new file mode 100644 index 0000000..0b6ba25 --- /dev/null +++ b/src/components/FaultsDashboard.test.tsx @@ -0,0 +1,116 @@ +// Copyright 2026 mfaferek93 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/** + * Two entities reporting the SAME fault code, which is legal - a code is only + * unique within one entity. Everything here failed while the dashboard's caches + * were keyed by code alone: expanding one row opened both, and clearing the + * second row cleared the first entity's fault. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import { FaultsDashboard } from './FaultsDashboard'; +import type { Fault } from '@/lib/types'; + +const mockFetchFaults = vi.fn(); +const mockClearFault = vi.fn(); +const mockGetFaultWithEnvironmentData = vi.fn(); + +let storeState: Record = {}; + +vi.mock('@/lib/store', () => ({ + useAppStore: Object.assign( + vi.fn((selector?: (s: Record) => unknown) => (selector ? selector(storeState) : storeState)), + { getState: () => storeState } + ), +})); + +function fault(entityId: string): Fault { + return { + code: 'LIDAR_RANGE_INVALID', + message: `range invalid on ${entityId}`, + severity: 'error', + status: 'active', + timestamp: '2026-08-20T10:00:00Z', + entity_id: entityId, + entity_type: 'app', + }; +} + +beforeEach(() => { + vi.clearAllMocks(); + mockGetFaultWithEnvironmentData.mockResolvedValue({ environment_data: { snapshots: [] } }); + storeState = { + faults: [fault('app_a'), fault('app_b')], + isLoadingFaults: false, + faultsError: null, + fetchFaults: mockFetchFaults, + clearFault: mockClearFault, + getFaultWithEnvironmentData: mockGetFaultWithEnvironmentData, + isConnected: true, + }; +}); + +describe('FaultsDashboard with colliding fault codes', () => { + it('expands only the clicked row and fetches only its entity', async () => { + render(); + // Flat list view: the grouped default splits by entity, which would + // hide the collision the caches must survive. + fireEvent.click(screen.getByRole('switch', { name: /group by entity/i })); + + const rows = screen.getAllByText('LIDAR_RANGE_INVALID'); + expect(rows).toHaveLength(2); + fireEvent.click(rows[0]!); + + await waitFor(() => expect(mockGetFaultWithEnvironmentData).toHaveBeenCalledTimes(1)); + expect(mockGetFaultWithEnvironmentData).toHaveBeenCalledWith('apps', 'app_a', 'LIDAR_RANGE_INVALID'); + // The sibling with the same code stays collapsed: exactly one row shows + // the expanded empty-environment marker. + await waitFor(() => expect(screen.getAllByText(/no environment data available/i)).toHaveLength(1)); + }); + + it("clears the clicked row's entity, not the first entity with that code", async () => { + render(); + fireEvent.click(screen.getByRole('switch', { name: /group by entity/i })); + + const clearButtons = screen.getAllByTitle('Clear fault'); + expect(clearButtons).toHaveLength(2); + fireEvent.click(clearButtons[1]!); + + await waitFor(() => expect(mockClearFault).toHaveBeenCalledTimes(1)); + expect(mockClearFault).toHaveBeenCalledWith('apps', 'app_b', 'LIDAR_RANGE_INVALID'); + }); + + it('keeps evidence on screen when a refetch answers 404 (null)', async () => { + mockGetFaultWithEnvironmentData + .mockResolvedValueOnce({ + environment_data: { snapshots: [{ type: 'freeze_frame', name: 'ff', data: { level: 82 } }] }, + }) + .mockResolvedValueOnce(null); + render(); + fireEvent.click(screen.getByRole('switch', { name: /group by entity/i })); + + const row = screen.getAllByText('LIDAR_RANGE_INVALID')[0]!; + fireEvent.click(row); + await waitFor(() => expect(screen.getByText(/snapshots \(1\)/i)).toBeInTheDocument()); + + // Collapse, re-expand: the second fetch resolves null (the store's + // documented 404 shape). The cached evidence must survive it. + fireEvent.click(row); + fireEvent.click(row); + await waitFor(() => expect(mockGetFaultWithEnvironmentData).toHaveBeenCalledTimes(2)); + expect(screen.getByText(/snapshots \(1\)/i)).toBeInTheDocument(); + }); +}); diff --git a/src/components/FaultsDashboard.tsx b/src/components/FaultsDashboard.tsx index baefcff..998650b 100644 --- a/src/components/FaultsDashboard.tsx +++ b/src/components/FaultsDashboard.tsx @@ -30,7 +30,7 @@ import { Skeleton } from '@/components/ui/skeleton'; import { SnapshotCard } from './SnapshotCard'; import { useAppStore } from '@/lib/store'; import type { Fault, FaultSeverity, FaultStatus, FaultResponse } from '@/lib/types'; -import { mapFaultEntityTypeToResourceType } from '@/lib/utils'; +import { faultKey, mapFaultEntityTypeToResourceType } from '@/lib/utils'; /** * Default polling interval in milliseconds @@ -129,7 +129,7 @@ function FaultRow({ isLoadingDetails, }: { fault: Fault; - onClear: (code: string) => void; + onClear: (fault: Fault) => void; isClearing: boolean; isExpanded: boolean; onToggle: () => void; @@ -189,7 +189,7 @@ function FaultRow({ size="sm" onClick={(e) => { e.stopPropagation(); - onClear(fault.code); + onClear(fault); }} disabled={isClearing} className="shrink-0" @@ -298,7 +298,7 @@ function FaultGroup({ entityId: string; entityType: string; faults: Fault[]; - onClear: (code: string) => void; + onClear: (fault: Fault) => void; clearingCodes: Set; expandedFaults: Set; onToggleFault: (fault: Fault) => void; @@ -338,14 +338,14 @@ function FaultGroup({ {faults.map((fault) => ( onToggleFault(fault)} environmentData={faultDetails.get(faultKey(fault))?.environment_data} - isLoadingDetails={loadingDetails.has(fault.code)} + isLoadingDetails={loadingDetails.has(faultKey(fault))} /> ))} @@ -393,20 +393,6 @@ function DashboardSkeleton() { * Uses shared faults state from useAppStore to avoid duplicate API calls * when both FaultsDashboard and FaultsCountBadge are visible. */ -/** - * Key for the per-fault caches in this dashboard. - * - * The dashboard lists faults from every entity at once, and a fault code is only - * unique within one entity - two apps can both report `LIDAR_RANGE_INVALID`. - * Keying the detail cache by code alone made the second entity's row render the - * first entity's environment data, i.e. download buttons pointing at another - * entity's recordings. Including the entity makes the key as specific as the - * request that filled it. - */ -function faultKey(fault: { code: string; entity_type?: string; entity_id?: string }): string { - return `${fault.entity_type ?? ''}/${fault.entity_id ?? ''}/${fault.code}`; -} - export function FaultsDashboard() { const [isRefreshing, setIsRefreshing] = useState(false); const [autoRefresh, setAutoRefresh] = useState(true); @@ -471,66 +457,75 @@ export function FaultsDashboard() { // Clear fault handler const handleClear = useCallback( - async (code: string) => { - setClearingCodes((prev) => new Set([...prev, code])); + // The whole Fault, not its code: two entities can report the same code, + // and resolving through `faults.find` cleared the FIRST entity's fault + // whichever row was clicked. + async (fault: Fault) => { + const key = faultKey(fault); + setClearingCodes((prev) => new Set([...prev, key])); try { - // Find the fault to get entity info - const fault = faults.find((f) => f.code === code); - if (fault) { - // Map the fault's entity_type to the correct resource type for the API - const entityGroup = mapFaultEntityTypeToResourceType(fault.entity_type); - // Use store's clearFault which has proper error handling with toasts - await clearFault(entityGroup, fault.entity_id, code); - } + // Map the fault's entity_type to the correct resource type for the API + const entityGroup = mapFaultEntityTypeToResourceType(fault.entity_type); + // Use store's clearFault which has proper error handling with toasts + await clearFault(entityGroup, fault.entity_id, fault.code); // Reload faults after clearing await fetchFaults(); } finally { setClearingCodes((prev) => { const next = new Set(prev); - next.delete(code); + next.delete(key); return next; }); } }, - [faults, fetchFaults, clearFault] + [fetchFaults, clearFault] ); // Toggle fault expansion and lazy-load environment data const handleToggleFault = useCallback( async (fault: Fault) => { - const faultCode = fault.code; - const newExpanded = new Set(expandedFaults); - - if (newExpanded.has(faultCode)) { - newExpanded.delete(faultCode); - } else { - newExpanded.add(faultCode); - - // Always refetch: a fault gains recordings while the page is open, - // and a cache filled once on first expand would keep serving the - // shorter list. The previous entry stays rendered meanwhile. - { - setLoadingDetails((prev) => new Set([...prev, faultCode])); - try { - const entityGroup = mapFaultEntityTypeToResourceType(fault.entity_type); - const details = await getFaultWithEnvironmentData(entityGroup, fault.entity_id, faultCode); - setFaultDetails((prev) => new Map(prev).set(faultKey(fault), details as FaultResponse)); - } catch (err) { - console.error('Failed to fetch fault details:', err); - } finally { - setLoadingDetails((prev) => { - const next = new Set(prev); - next.delete(faultCode); - return next; - }); - } + const key = faultKey(fault); + // Functional update, BEFORE the await: the row opens on the click + // (the previous entry stays rendered while the refetch runs), and a + // second click during the request collapses instead of reading a + // stale closed-over set and firing another GET. + let opened = false; + setExpandedFaults((prev) => { + const next = new Set(prev); + if (next.has(key)) { + next.delete(key); + } else { + next.add(key); + opened = true; + } + return next; + }); + if (!opened) return; + + // Always refetch: a fault gains recordings while the page is open, + // and a cache filled once on first expand would keep serving the + // shorter list. + setLoadingDetails((prev) => new Set([...prev, key])); + try { + const entityGroup = mapFaultEntityTypeToResourceType(fault.entity_type); + const details = await getFaultWithEnvironmentData(entityGroup, fault.entity_id, fault.code); + // A 404 resolves to null rather than throwing; overwriting the + // cache with it would blank evidence that was already on screen. + if (details) { + setFaultDetails((prev) => new Map(prev).set(key, details as FaultResponse)); } + } catch (err) { + console.error('Failed to fetch fault details:', err); + } finally { + setLoadingDetails((prev) => { + const next = new Set(prev); + next.delete(key); + return next; + }); } - - setExpandedFaults(newExpanded); }, - [getFaultWithEnvironmentData, expandedFaults] + [getFaultWithEnvironmentData] ); // Filter faults @@ -835,14 +830,14 @@ export function FaultsDashboard() { {filteredFaults.map((fault) => ( handleToggleFault(fault)} environmentData={faultDetails.get(faultKey(fault))?.environment_data} - isLoadingDetails={loadingDetails.has(fault.code)} + isLoadingDetails={loadingDetails.has(faultKey(fault))} /> ))} diff --git a/src/components/FaultsPanel.tsx b/src/components/FaultsPanel.tsx index 0744e83..96d48cf 100644 --- a/src/components/FaultsPanel.tsx +++ b/src/components/FaultsPanel.tsx @@ -19,7 +19,7 @@ import { Collapsible, CollapsibleContent, CollapsibleTrigger } from '@/component import { SnapshotCard } from './SnapshotCard'; import { useAppStore, type AppState } from '@/lib/store'; import type { Fault, FaultSeverity, FaultStatus, FaultResponse, SovdResourceEntityType } from '@/lib/types'; -import { mapFaultEntityTypeToResourceType } from '@/lib/utils'; +import { faultKey, mapFaultEntityTypeToResourceType } from '@/lib/utils'; interface FaultsPanelProps { entityId: string; @@ -301,48 +301,59 @@ export function FaultsPanel({ entityId, entityType = 'components' }: FaultsPanel }, [loadFaults]); const handleToggleFault = useCallback( - async (faultCode: string) => { - const newExpanded = new Set(expandedFaults); - - if (newExpanded.has(faultCode)) { - newExpanded.delete(faultCode); - } else { - newExpanded.add(faultCode); + // The whole Fault, not its code: a component's list spans every app it + // hosts, so two apps under one component can report the same code and a + // code-keyed lookup cannot tell them apart. + async (fault: Fault) => { + const key = faultKey(fault); + // Functional update, BEFORE the await: the row opens on the click + // (the previous entry stays rendered while the refetch runs), and a + // second click during the request collapses instead of reading a + // stale closed-over set and firing another GET. + let opened = false; + setExpandedFaults((prev) => { + const next = new Set(prev); + if (next.has(key)) { + next.delete(key); + } else { + next.add(key); + opened = true; + } + return next; + }); + if (!opened) return; - // Always refetch, even when a detail is already cached. A fault can - // gain recordings while the page is open - it re-confirms, the black - // box is written, the snapshot list grows - and a cache that is - // filled once on first expand would keep serving the older list with - // no way to refresh short of remounting. The stale entry stays - // rendered until the new one lands, so re-expanding never flickers. - { - setLoadingDetails((prev) => new Set([...prev, faultCode])); - try { - // Use the fault's own entity info (app-level) for correct bulk_data_uri. - // Components have a synthetic FQN that doesn't match fault reporting sources, - // so fetching via /components/{id}/faults/{code} produces an unusable bulk_data_uri. - const fault = faults.find((f) => f.code === faultCode); - const detailEntityType: SovdResourceEntityType = fault?.entity_type - ? mapFaultEntityTypeToResourceType(fault.entity_type) - : entityType; - const detailEntityId = fault?.entity_id || entityId; - const details = await getFaultWithEnvironmentData(detailEntityType, detailEntityId, faultCode); - setFaultDetails((prev) => new Map(prev).set(faultCode, details as FaultResponse)); - } catch (err) { - console.error('Failed to fetch fault details:', err); - } finally { - setLoadingDetails((prev) => { - const next = new Set(prev); - next.delete(faultCode); - return next; - }); - } + // Always refetch, even when a detail is already cached. A fault can + // gain recordings while the page is open - it re-confirms, the black + // box is written, the snapshot list grows - and a cache that is + // filled once on first expand would keep serving the older list with + // no way to refresh short of remounting. + setLoadingDetails((prev) => new Set([...prev, key])); + try { + // Use the fault's own entity info (app-level) for correct bulk_data_uri. + // Components have a synthetic FQN that doesn't match fault reporting sources, + // so fetching via /components/{id}/faults/{code} produces an unusable bulk_data_uri. + const detailEntityType: SovdResourceEntityType = fault.entity_type + ? mapFaultEntityTypeToResourceType(fault.entity_type) + : entityType; + const detailEntityId = fault.entity_id || entityId; + const details = await getFaultWithEnvironmentData(detailEntityType, detailEntityId, fault.code); + // A 404 resolves to null rather than throwing; overwriting the + // cache with it would blank evidence that was already on screen. + if (details) { + setFaultDetails((prev) => new Map(prev).set(key, details as FaultResponse)); } + } catch (err) { + console.error('Failed to fetch fault details:', err); + } finally { + setLoadingDetails((prev) => { + const next = new Set(prev); + next.delete(key); + return next; + }); } - - setExpandedFaults(newExpanded); }, - [getFaultWithEnvironmentData, entityType, entityId, expandedFaults, faults] + [getFaultWithEnvironmentData, entityType, entityId] ); const handleClear = useCallback( @@ -424,10 +435,10 @@ export function FaultsPanel({ entityId, entityType = 'components' }: FaultsPanel fault={fault} onClear={handleClear} isClearing={clearingCodes.has(fault.code)} - isExpanded={expandedFaults.has(fault.code)} - onToggle={() => handleToggleFault(fault.code)} - environmentData={faultDetails.get(fault.code)?.environment_data} - isLoadingDetails={loadingDetails.has(fault.code)} + isExpanded={expandedFaults.has(faultKey(fault))} + onToggle={() => handleToggleFault(fault)} + environmentData={faultDetails.get(faultKey(fault))?.environment_data} + isLoadingDetails={loadingDetails.has(faultKey(fault))} /> ))} diff --git a/src/lib/store.ts b/src/lib/store.ts index f7bc6fc..699c74e 100644 --- a/src/lib/store.ts +++ b/src/lib/store.ts @@ -903,32 +903,49 @@ async function fetchEntityFromApi( /** * Filename the server chose, out of a `Content-Disposition` header. * - * Handles both the plain `filename="x"` form and RFC 5987's `filename*=UTF-8''x`, - * preferring the latter when present because that is the one that survives - * non-ASCII. Returns null when the header is absent or names nothing, so the - * caller can fall back rather than saving a file called "null". + * Handles the plain `filename=` form (quoted or not) and RFC 8187's + * `filename*=''` - any charset and any language + * tag, not just `UTF-8''`. Returns null when the header is absent or names + * nothing, so the caller can fall back rather than saving a file called "null". */ + +/** RFC 8187 ext-value payload to a string, or null when undecodable. UTF-8 is + * the wire norm; any single-byte charset (the RFC's other registered case is + * ISO-8859-1) decodes byte-per-byte, which maps 1:1 onto code points. */ +function decodeExtValue(charset: string, encoded: string): string | null { + try { + if (/^utf-?8$/i.test(charset)) return decodeURIComponent(encoded); + return encoded.replace(/%([0-9a-f]{2})/gi, (_, hex: string) => String.fromCharCode(parseInt(hex, 16))); + } catch { + return null; + } +} + export function filenameFromContentDisposition(header: string | null): string | null { if (!header) return null; - const extended = /filename\*\s*=\s*(?:UTF-8|utf-8)''([^;]+)/i.exec(header); - if (extended?.[1]) { - try { - const decoded = decodeURIComponent(extended[1].trim()); - if (decoded) return decoded; - } catch { - // Malformed percent-encoding: fall through to the plain form. - } + const extended = /(?:^|;)\s*filename\*\s*=\s*([^';]+)'[^';]*'([^;\s]*)/.exec(header); + // Validated as a WHOLE before decoding: a partial match would silently + // truncate at the first bad escape ("bad%ZZ" -> "bad") instead of letting a + // well-formed plain `filename=` further down win. Decoded before trimming, + // so a value that is all `%20` is rejected as empty. + if (extended && /^(?:%[0-9a-fA-F]{2}|[^%])*$/.test(extended[2]!)) { + const name = decodeExtValue(extended[1]!, extended[2]!)?.trim(); + if (name) return name; + } + + // Quoted form next: semicolons and spaces stay inside the quotes, and a + // backslash escapes the next character. Anchored on a parameter boundary so + // `xfilename=` cannot match, and `filename*=` cannot reach here because a + // `*` sits between the name and the `=`. + const quoted = /(?:^|;)\s*filename\s*=\s*"((?:\\.|[^"\\])*)"/i.exec(header); + if (quoted) { + const name = quoted[1]!.replace(/\\(.)/g, '$1').trim(); + return name ? name : null; } - // One alternative, then unquote: matching the quoted form separately means a - // `filename=""` falls through to the unquoted branch and comes back as the - // two literal quote characters instead of as "no name". - const plain = /filename\s*=\s*([^;]+)/i.exec(header); - const name = plain?.[1] - ?.trim() - .replace(/^"(.*)"$/, '$1') - .trim(); + const plain = /(?:^|;)\s*filename\s*=\s*([^;]+)/i.exec(header); + const name = plain?.[1]?.trim(); return name ? name : null; } diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 7bf3517..1e75558 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -50,3 +50,16 @@ export function formatDuration(seconds: number): string { const secs = Math.round(seconds % 60); return `${mins}m ${secs}s`; } + +/** + * Key for per-fault UI caches (expand state, detail cache, in-flight sets). + * + * A fault code is only unique within one entity - two apps can both report + * `LIDAR_RANGE_INVALID` - so any cache keyed by code alone ties their rows + * together: expanding one opens both, and a clear resolves to whichever + * entity's fault happens to come first. Including the entity makes the key as + * specific as the request that filled the cache. + */ +export function faultKey(fault: { code: string; entity_type?: string; entity_id?: string }): string { + return `${fault.entity_type ?? ''}/${fault.entity_id ?? ''}/${fault.code}`; +} From 493119bf0594ad084b07583c1b4ede3777657edb Mon Sep 17 00:00:00 2001 From: Michal Faferek Date: Thu, 20 Aug 2026 18:19:12 +0200 Subject: [PATCH 5/5] fix(e2e): source in statements, not in the backgrounded chain Brought the stack up for real: '&' still bound the whole 'source && source && fault_manager' chain, so the gateway leg ran unsourced and exited 127. Semicolons scope each '&' to one command. Verified live: all three jobs up, two recordings seeded, specs' data visible in the browser. --- e2e/docker-compose.rosbag.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/docker-compose.rosbag.yml b/e2e/docker-compose.rosbag.yml index d5333b0..b3961ca 100644 --- a/e2e/docker-compose.rosbag.yml +++ b/e2e/docker-compose.rosbag.yml @@ -49,8 +49,8 @@ services: # exits. command: - > - source /opt/ros/jazzy/setup.bash && - source /home/medkit/ws/install/setup.bash && + source /opt/ros/jazzy/setup.bash; + source /home/medkit/ws/install/setup.bash; ros2 run ros2_medkit_fault_manager fault_manager_node --ros-args --params-file /e2e/params.yaml & FM=$!;