diff --git a/e2e/docker-compose.rosbag.yml b/e2e/docker-compose.rosbag.yml new file mode 100644 index 0000000..b3961ca --- /dev/null +++ b/e2e/docker-compose.rosbag.yml @@ -0,0 +1,83 @@ +# 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. + +# 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 + # (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 + # 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 & + 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 + # 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: + # 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/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..0ff7a2d --- /dev/null +++ b/e2e/gateway/seed_recordings.py @@ -0,0 +1,163 @@ +#!/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' +# 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. +FILL_SECONDS = 3.0 + + +class Seeder(Node): + def __init__(self): + super().__init__(NODE_NAME) + 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, 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() + 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 + 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 + 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) + 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) + + # 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 + + +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..720968f --- /dev/null +++ b/e2e/rosbag-recordings.spec.ts @@ -0,0 +1,198 @@ +// 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 { readFileSync } from 'node:fs'; + +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 }; +} + +/** 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 { + // 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)) + .map((item) => item.id); +} + +/** The app the seeded fault is attributed to, whatever the gateway named it. */ +async function appHoldingTheFault(): Promise { + 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 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; + } + 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[] = []; + 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(); + 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(); + 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('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. 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. + 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); +}); 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.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 45b5d15..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(fault.code)?.environment_data} - isLoadingDetails={loadingDetails.has(fault.code)} + environmentData={faultDetails.get(faultKey(fault))?.environment_data} + isLoadingDetails={loadingDetails.has(faultKey(fault))} /> ))} @@ -457,64 +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); - - // Fetch details if not cached - if (!faultDetails.has(faultCode)) { - 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)); - } 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, faultDetails] + [getFaultWithEnvironmentData] ); // Filter faults @@ -819,14 +830,14 @@ export function FaultsDashboard() { {filteredFaults.map((fault) => ( handleToggleFault(fault)} - environmentData={faultDetails.get(fault.code)?.environment_data} - isLoadingDetails={loadingDetails.has(fault.code)} + environmentData={faultDetails.get(faultKey(fault))?.environment_data} + isLoadingDetails={loadingDetails.has(faultKey(fault))} /> ))} diff --git a/src/components/FaultsPanel.tsx b/src/components/FaultsPanel.tsx index 85aa1ae..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,43 +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; - // Fetch details if not cached - if (!faultDetails.has(faultCode)) { - 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, faultDetails, faults] + [getFaultWithEnvironmentData, entityType, entityId] ); const handleClear = useCallback( @@ -419,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/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..699c74e 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,55 @@ async function fetchEntityFromApi( } } +/** + * Filename the server chose, out of a `Content-Disposition` header. + * + * 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 = /(?:^|;)\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; + } + + const plain = /(?:^|;)\s*filename\s*=\s*([^;]+)/i.exec(header); + const name = plain?.[1]?.trim(); + return name ? name : null; +} + export const useAppStore = create()( persist( (set, get) => ({ @@ -2550,13 +2598,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 +2608,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); 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}`; +}