From 47b399805eab1da5ba1841f3cfcee60efd9ee844 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 28 Jul 2026 17:09:47 +0200 Subject: [PATCH 01/11] test(e2e): add a Playwright harness and cover the Scripts tab Runs the suite against a real gateway in a container: manifest-defined scripts, uploads enabled, and a named volume for uploaded files so the checkout stays clean. Seven scenarios drive the live gateway, six more use route mocking for states a healthy gateway will not produce on demand, and one smoke test proves the stack itself. The suite runs on a single worker because the mocked project still reaches the gateway for entity discovery. --- .gitignore | 5 + e2e/docker-compose.yml | 29 ++++ e2e/fixtures/uploaded-script.sh | 16 +++ e2e/gateway/manifest.yaml | 34 +++++ e2e/gateway/params.yaml | 14 ++ e2e/gateway/scripts/fail.sh | 16 +++ e2e/gateway/scripts/hello.sh | 17 +++ e2e/gateway/scripts/sleep.sh | 15 ++ e2e/global-setup.ts | 60 ++++++++ e2e/scripts-errors.spec.ts | 201 ++++++++++++++++++++++++++ e2e/scripts.spec.ts | 245 ++++++++++++++++++++++++++++++++ e2e/smoke.spec.ts | 20 +++ e2e/tsconfig.json | 8 ++ playwright.config.ts | 47 ++++++ tsconfig.json | 6 +- 15 files changed, 732 insertions(+), 1 deletion(-) create mode 100644 e2e/docker-compose.yml create mode 100755 e2e/fixtures/uploaded-script.sh create mode 100644 e2e/gateway/manifest.yaml create mode 100644 e2e/gateway/params.yaml create mode 100644 e2e/gateway/scripts/fail.sh create mode 100644 e2e/gateway/scripts/hello.sh create mode 100644 e2e/gateway/scripts/sleep.sh create mode 100644 e2e/global-setup.ts create mode 100644 e2e/scripts-errors.spec.ts create mode 100644 e2e/scripts.spec.ts create mode 100644 e2e/smoke.spec.ts create mode 100644 e2e/tsconfig.json create mode 100644 playwright.config.ts diff --git a/.gitignore b/.gitignore index fd85165..d10094d 100644 --- a/.gitignore +++ b/.gitignore @@ -25,3 +25,8 @@ dist-ssr # Serena .serena/ + +# Playwright +playwright-report/ +test-results/ +e2e/.auth/ diff --git a/e2e/docker-compose.yml b/e2e/docker-compose.yml new file mode 100644 index 0000000..2763068 --- /dev/null +++ b/e2e/docker-compose.yml @@ -0,0 +1,29 @@ +services: + # Docker creates a fresh named volume owned by root:root, but the gateway + # image runs as the unprivileged `medkit` user (uid 999) and cannot create + # script subdirectories under a root-owned mount. This one-shot service + # chowns the volume before the gateway starts; it reuses the pinned + # gateway image (which already has chown) instead of pulling another one. + init-uploads: + image: ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 + user: root + volumes: + - e2e-uploads:/e2e-uploads + entrypoint: ['chown', '-R', '999:999', '/e2e-uploads'] + gateway: + # Pinned on purpose: :latest is overwritten on every push to the gateway + # main branch, which would let unrelated changes turn this repo CI red. + image: ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 + ports: + - '${E2E_GATEWAY_PORT:-8080}:8080' + volumes: + - ./gateway/params.yaml:/e2e/params.yaml:ro + - ./gateway/manifest.yaml:/e2e/manifest.yaml:ro + - ./gateway/scripts:/e2e-scripts:ro + - e2e-uploads:/e2e-uploads + command: ['--ros-args', '--params-file', '/e2e/params.yaml'] + depends_on: + init-uploads: + condition: service_completed_successfully +volumes: + e2e-uploads: diff --git a/e2e/fixtures/uploaded-script.sh b/e2e/fixtures/uploaded-script.sh new file mode 100755 index 0000000..3d4f653 --- /dev/null +++ b/e2e/fixtures/uploaded-script.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash +# Copyright 2026 bburda +# +# 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. +set -eu +echo "uploaded script executed" diff --git a/e2e/gateway/manifest.yaml b/e2e/gateway/manifest.yaml new file mode 100644 index 0000000..edf673a --- /dev/null +++ b/e2e/gateway/manifest.yaml @@ -0,0 +1,34 @@ +manifest_version: '1.0' +components: + - id: 'ecu' + name: 'Test ECU' +apps: + - id: 'talker' + name: 'Talker' + is_located_on: 'ecu' +scripts: + - id: 'hello' + name: 'Hello' + description: 'Echoes the parameters it receives on stdin' + path: '/e2e-scripts/hello.sh' + format: 'bash' + timeout_sec: 30 + entity_filter: + - 'ecu' + - 'talker' + - id: 'failing' + name: 'Failing' + description: 'Exits with a non-zero code' + path: '/e2e-scripts/fail.sh' + format: 'bash' + timeout_sec: 30 + entity_filter: + - 'ecu' + - id: 'sleeper' + name: 'Sleeper' + description: 'Runs long enough to be stopped' + path: '/e2e-scripts/sleep.sh' + format: 'bash' + timeout_sec: 300 + entity_filter: + - 'ecu' diff --git a/e2e/gateway/params.yaml b/e2e/gateway/params.yaml new file mode 100644 index 0000000..6a94272 --- /dev/null +++ b/e2e/gateway/params.yaml @@ -0,0 +1,14 @@ +/**: + ros__parameters: + server: + host: '0.0.0.0' + port: 8080 + cors: + allowed_origins: + - 'http://localhost:5173' + discovery: + mode: 'manifest_only' + manifest_path: '/e2e/manifest.yaml' + scripts: + scripts_dir: '/e2e-uploads' + allow_uploads: true diff --git a/e2e/gateway/scripts/fail.sh b/e2e/gateway/scripts/fail.sh new file mode 100644 index 0000000..22c0893 --- /dev/null +++ b/e2e/gateway/scripts/fail.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash +# Copyright 2026 bburda +# +# 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. +echo "diagnostics failed: sensor unreachable" >&2 +exit 3 diff --git a/e2e/gateway/scripts/hello.sh b/e2e/gateway/scripts/hello.sh new file mode 100644 index 0000000..9e55a6a --- /dev/null +++ b/e2e/gateway/scripts/hello.sh @@ -0,0 +1,17 @@ +#!/usr/bin/env bash +# Copyright 2026 bburda +# +# 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. +set -eu +params="$(cat)" +echo "hello ${params}" diff --git a/e2e/gateway/scripts/sleep.sh b/e2e/gateway/scripts/sleep.sh new file mode 100644 index 0000000..87dfd87 --- /dev/null +++ b/e2e/gateway/scripts/sleep.sh @@ -0,0 +1,15 @@ +#!/usr/bin/env bash +# Copyright 2026 bburda +# +# 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. +sleep 300 diff --git a/e2e/global-setup.ts b/e2e/global-setup.ts new file mode 100644 index 0000000..2b14e09 --- /dev/null +++ b/e2e/global-setup.ts @@ -0,0 +1,60 @@ +// Copyright 2026 bburda +// +// 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 { chromium } from '@playwright/test'; +import { mkdirSync } from 'node:fs'; + +const GATEWAY_URL = process.env.E2E_GATEWAY_URL ?? 'http://localhost:8080/api/v1'; +const APP_URL = process.env.E2E_APP_URL ?? 'http://localhost:5173'; +const STORAGE_KEY = 'ros2_medkit_web_ui_server_url'; + +async function waitForGateway(): Promise { + const deadline = Date.now() + 120_000; + let lastError = 'no attempt made'; + while (Date.now() < deadline) { + try { + const res = await fetch(`${GATEWAY_URL}/health`); + if (res.ok) return; + lastError = `HTTP ${res.status}`; + } catch (err) { + lastError = err instanceof Error ? err.message : String(err); + } + await new Promise((resolve) => setTimeout(resolve, 1000)); + } + throw new Error(`Gateway did not become healthy at ${GATEWAY_URL}: ${lastError}`); +} + +export default async function globalSetup(): Promise { + await waitForGateway(); + + const browser = await chromium.launch(); + const context = await browser.newContext(); + // The app auto-connects to the persisted URL on start, so seeding the store + // is enough and we never have to drive the connection dialog. + await context.addInitScript( + ([key, url]) => { + window.localStorage.setItem(key, JSON.stringify({ state: { serverUrl: url }, version: 0 })); + }, + [STORAGE_KEY, GATEWAY_URL] + ); + const page = await context.newPage(); + // goto() already waits for 'load'. Not 'networkidle': once connected, the + // app opens a long-lived SSE fault stream that never completes, so the + // network would never go idle and this would hang until the action timeout. + await page.goto(APP_URL); + + mkdirSync('e2e/.auth', { recursive: true }); + await context.storageState({ path: 'e2e/.auth/state.json' }); + await browser.close(); +} diff --git a/e2e/scripts-errors.spec.ts b/e2e/scripts-errors.spec.ts new file mode 100644 index 0000000..567dfb1 --- /dev/null +++ b/e2e/scripts-errors.spec.ts @@ -0,0 +1,201 @@ +// Copyright 2026 bburda +// +// 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 { test, expect, type Page } from '@playwright/test'; + +async function selectTestEcu(page: Page): Promise { + await page.goto('/'); + await page.getByText('Test ECU').click(); +} + +async function openScripts(page: Page): Promise { + await selectTestEcu(page); + await page.getByRole('button', { name: /scripts/i }).click(); +} + +test('hides the Scripts tab when the gateway does not report the capability', async ({ page }) => { + await page.route('**/api/v1/', async (route) => { + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + api_base: '/api/v1', + endpoints: [], + name: 'ROS 2 Medkit Gateway', + version: '0.6.0', + capabilities: { + aggregation: false, + async_actions: true, + authentication: false, + bulk_data: true, + configurations: true, + cyclic_subscriptions: true, + data_access: true, + discovery: true, + faults: true, + locking: true, + logs: true, + operations: true, + scripts: false, + tls: false, + triggers: true, + updates: false, + vendor_extensions: false, + }, + }), + }); + }); + + await selectTestEcu(page); + await expect(page.getByRole('button', { name: /scripts/i })).toHaveCount(0); +}); + +test('shows the not-configured state when listing returns 501', async ({ page }) => { + await page.route('**/api/v1/components/*/scripts', async (route) => { + await route.fulfill({ + status: 501, + contentType: 'application/json', + body: JSON.stringify({ error_code: 'not-implemented', message: 'Scripts backend not configured' }), + }); + }); + + await openScripts(page); + await expect(page.getByText('Scripts are not configured on this gateway')).toBeVisible({ timeout: 30_000 }); +}); + +test('reports disabled uploads from the gateway message', async ({ page }) => { + await page.route('**/api/v1/components/*/scripts', async (route) => { + if (route.request().method() !== 'POST') { + await route.continue(); + return; + } + await route.fulfill({ + status: 400, + contentType: 'application/json', + body: JSON.stringify({ + error_code: 'invalid-request', + message: 'Script uploads are disabled on this gateway', + }), + }); + }); + + await openScripts(page); + await page.getByRole('button', { name: 'Upload' }).click(); + const dialog = page.getByRole('dialog'); + await dialog.getByLabel('File').setInputFiles({ + name: 'probe.sh', + mimeType: 'text/x-shellscript', + buffer: Buffer.from('#!/usr/bin/env bash\necho probe\n'), + }); + await dialog.getByRole('button', { name: 'Upload' }).click(); + await expect(dialog.getByRole('alert')).toHaveText('Script uploads are disabled on this gateway', { + timeout: 30_000, + }); +}); + +test('reports that a managed script cannot be deleted', async ({ page }) => { + // The live gateway would never let a managed script's Delete button + // render at all, so the only way to exercise the gateway's rejection + // message is to mock a script that claims not to be managed and have the + // delete call fail anyway - exactly what a stale client cache would see. + await page.route('**/api/v1/components/*/scripts', async (route) => { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + items: [{ id: 'hello', name: 'Hello', description: 'Mocked managed script', managed: false }], + }), + }); + }); + await page.route('**/api/v1/components/*/scripts/*', async (route) => { + if (route.request().method() !== 'DELETE') { + await route.continue(); + return; + } + await route.fulfill({ + status: 409, + contentType: 'application/json', + body: JSON.stringify({ + error_code: 'x-medkit-managed-script', + message: 'Cannot delete managed script: hello', + }), + }); + }); + + await openScripts(page); + await page.getByRole('button', { name: 'Hello' }).click(); + await page.getByRole('button', { name: 'Delete' }).click(); + await expect(page.getByText('Cannot delete managed script: hello')).toBeVisible({ timeout: 30_000 }); +}); + +test('reports the concurrency limit when starting an execution', async ({ page }) => { + await page.route('**/api/v1/components/*/scripts/*/executions', async (route) => { + if (route.request().method() !== 'POST') { + await route.continue(); + return; + } + await route.fulfill({ + status: 429, + contentType: 'application/json', + body: JSON.stringify({ + error_code: 'x-medkit-concurrency-limit', + message: 'Maximum concurrent executions reached (5)', + }), + }); + }); + + await openScripts(page); + await page.getByRole('button', { name: 'Hello' }).click(); + await page.getByRole('button', { name: 'Run' }).click(); + await expect(page.getByText('Maximum concurrent executions reached (5)')).toBeVisible({ timeout: 30_000 }); +}); + +test('marks an execution as no longer tracked when polling returns 404', async ({ page }) => { + await page.route('**/api/v1/components/*/scripts/*/executions', async (route) => { + if (route.request().method() !== 'POST') { + await route.continue(); + return; + } + await route.fulfill({ + status: 202, + contentType: 'application/json', + body: JSON.stringify({ id: 'exec_mocked_1', status: 'running', started_at: new Date().toISOString() }), + }); + }); + await page.route('**/api/v1/components/*/scripts/*/executions/*', async (route) => { + await route.fulfill({ + status: 404, + contentType: 'application/json', + body: JSON.stringify({ error_code: 'resource-not-found', message: 'Execution not found' }), + }); + }); + + await openScripts(page); + await page.getByRole('button', { name: 'Hello' }).click(); + await page.getByRole('button', { name: 'Run' }).click(); + const status = page.getByTestId('execution-status'); + await expect(status).toBeVisible({ timeout: 30_000 }); + // Scope to the card that owns this status badge - other Refresh/Remove + // buttons exist elsewhere on the page (the panel's list reload, other + // execution cards). Do not click Refresh: the assertion below must be + // satisfied by the store's own poll loop picking up the 404 on its next + // tick, not by the manual rescue action. + const card = page.locator('[data-slot="card"]', { has: status }).last(); + await expect(page.getByText('The gateway no longer tracks this execution')).toBeVisible({ timeout: 30_000 }); + await expect(card.getByRole('button', { name: 'Remove' })).toBeVisible({ timeout: 30_000 }); +}); diff --git a/e2e/scripts.spec.ts b/e2e/scripts.spec.ts new file mode 100644 index 0000000..5f90d5e --- /dev/null +++ b/e2e/scripts.spec.ts @@ -0,0 +1,245 @@ +// Copyright 2026 bburda +// +// 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 path from 'node:path'; +import { test, expect, type Locator, type Page, type TestInfo } from '@playwright/test'; + +async function openScripts(page: Page, entity: 'Test ECU' | 'Talker'): Promise { + await page.goto('/'); + if (entity === 'Talker') { + // Talker only appears in the tree once its parent component has been + // selected and its children have loaded, so it must be expanded first. + await page.getByText('Test ECU').click(); + await page.getByText('Talker').waitFor({ state: 'visible', timeout: 30_000 }); + } + await page.getByText(entity).click(); + await page.getByRole('button', { name: /scripts/i }).click(); +} + +/** + * A stable, unique-per-worker upload name. `testInfo` (rather than the + * module-scope `test.info()`, which throws outside a running test) is what + * lets the afterEach hook below compute the exact same name the upload test + * used, since both run in the same worker with the same repeat index. + */ +function uploadedNameFor(testInfo: TestInfo): string { + return `uploaded_${testInfo.workerIndex}_${testInfo.repeatEachIndex}`; +} + +function writtenNameFor(testInfo: TestInfo, language: 'bash' | 'python'): string { + return `written_${language}_${testInfo.workerIndex}_${testInfo.repeatEachIndex}`; +} + +test.afterEach(async ({ page }, testInfo) => { + // Uploads persist in the gateway's volume between runs, so any script left + // over from this test (including a previous, unfinished run of it) must be + // removed - otherwise the next run would find a duplicate row and a + // getByRole('button', { name: uploadedName }) lookup would no longer be unique. + await openScripts(page, 'Test ECU'); + const names = [uploadedNameFor(testInfo), writtenNameFor(testInfo, 'bash'), writtenNameFor(testInfo, 'python')]; + for (const name of names) { + const row = page.getByRole('button', { name }); + if (await row.isVisible().catch(() => false)) { + await row.click(); + await page.getByRole('button', { name: 'Delete' }).click(); + await expect(row).toBeHidden({ timeout: 30_000 }); + } + } +}); + +/** + * CodeMirror's editable region is a `contenteditable` div, not a form + * control, so it has to be driven with real key events rather than a + * Locator.fill() - select-all, delete, then type the new content, the same + * sequence a person at the keyboard would use to replace the starter + * template. Takes the resolved editable locator rather than looking it up + * itself, so callers can choose how they reach it (by test id or, in one + * scenario below, by role and accessible name). + */ +async function writeInEditor(editable: Locator, content: string): Promise { + await editable.click(); + await editable.press('ControlOrMeta+a'); + await editable.press('Delete'); + await editable.pressSequentially(content); +} + +test('lists manifest scripts and marks them managed', async ({ page }) => { + await openScripts(page, 'Test ECU'); + for (const name of ['Hello', 'Failing', 'Sleeper']) { + const row = page.getByRole('button', { name }); + await expect(row).toBeVisible({ timeout: 30_000 }); + await expect(row).toContainText('managed'); + } +}); + +test('runs a script and shows the parameters it received on stdin', async ({ page }) => { + await openScripts(page, 'Test ECU'); + await page.getByRole('button', { name: 'Hello' }).click(); + // Sending parameters is what makes the gateway open the stdin pipe at all; + // hello.sh echoes whatever JSON it reads back on stdout. + await page.getByPlaceholder('{}').fill('{"greeting":"e2e-hello"}'); + await page.getByRole('button', { name: 'Run' }).click(); + + const status = page.getByTestId('execution-status'); + await expect(status).toHaveText('completed', { timeout: 30_000 }); + await expect(status).toHaveAttribute('data-tone', 'ok'); + await expect(page.locator('pre')).toContainText('"greeting":"e2e-hello"'); +}); + +test('shows exit code and stderr for a failing script', async ({ page }) => { + await openScripts(page, 'Test ECU'); + await page.getByRole('button', { name: 'Failing' }).click(); + await page.getByRole('button', { name: 'Run' }).click(); + + const status = page.getByTestId('execution-status'); + await expect(status).toHaveText('failed', { timeout: 30_000 }); + await expect(status).toHaveAttribute('data-tone', 'error'); + // The gateway discards stdout on a non-zero exit: only the stderr message + // and the exit code are ever available to assert on. + await expect(page.getByText(/sensor unreachable/)).toBeVisible(); + await expect(page.getByText(/exit code 3/)).toBeVisible(); +}); + +test('stops a running script and reports it as stopped, not failed', async ({ page }) => { + await openScripts(page, 'Test ECU'); + await page.getByRole('button', { name: 'Sleeper' }).click(); + await page.getByRole('button', { name: 'Run' }).click(); + const status = page.getByTestId('execution-status'); + await expect(status).toHaveText('running', { timeout: 30_000 }); + await page.getByRole('button', { name: 'Stop' }).click(); + await expect(status).toHaveText('terminated', { timeout: 30_000 }); + // A successful stop must render as stopped, never as an error tone. + await expect(status).toHaveAttribute('data-tone', 'stopped'); +}); + +test('uploads, runs and deletes a script', async ({ page }, testInfo) => { + const uploadedName = uploadedNameFor(testInfo); + await openScripts(page, 'Test ECU'); + await page.getByRole('button', { name: 'Upload' }).click(); + const dialog = page.getByRole('dialog'); + await dialog.getByLabel('File').setInputFiles(path.join(import.meta.dirname, 'fixtures', 'uploaded-script.sh')); + await dialog.getByLabel('Name').fill(uploadedName); + await dialog.getByRole('button', { name: 'Upload' }).click(); + await expect(dialog).toBeHidden({ timeout: 30_000 }); + + const row = page.getByRole('button', { name: uploadedName }); + await expect(row).toBeVisible({ timeout: 30_000 }); + // An uploaded script is not managed, so it must expose the Delete control. + await expect(row).not.toContainText('managed'); + await row.click(); + + await page.getByRole('button', { name: 'Run' }).click(); + const status = page.getByTestId('execution-status'); + await expect(status).toHaveText('completed', { timeout: 30_000 }); + + await page.getByRole('button', { name: 'Delete' }).click(); + await expect(row).toBeHidden({ timeout: 30_000 }); +}); + +test('writes a bash script in the UI, runs it and shows its output', async ({ page }, testInfo) => { + const scriptName = writtenNameFor(testInfo, 'bash'); + await openScripts(page, 'Test ECU'); + await page.getByRole('button', { name: 'Upload' }).click(); + const dialog = page.getByRole('dialog'); + await dialog.getByRole('button', { name: 'Write script' }).click(); + await dialog.getByLabel('File name').fill(`${scriptName}.sh`); + // Reachable by role and accessible name against the real CodeMirror + // instance, not just the mocked editor the unit tests exercise - this is + // the only place that would catch ScriptEditor's aria-label regressing. + const editor = dialog.getByRole('textbox', { name: 'Script content' }); + await expect(editor).toBeVisible(); + // Reads stdin and prints a recognisable, greppable line - the same + // contract the starter template teaches, just without the placeholder text. + await writeInEditor(editor, ['read -r params', 'echo e2e-write-bash-ok $params'].join('\n')); + // Exact match: "File name" also contains the substring "Name", and + // Playwright's getByLabel matches substrings by default. + await dialog.getByLabel('Name', { exact: true }).fill(scriptName); + await dialog.getByRole('button', { name: 'Upload' }).click(); + await expect(dialog).toBeHidden({ timeout: 30_000 }); + + const row = page.getByRole('button', { name: scriptName }); + await expect(row).toBeVisible({ timeout: 30_000 }); + await row.click(); + + await page.getByPlaceholder('{}').fill('{"greeting":"e2e-write-bash"}'); + await page.getByRole('button', { name: 'Run' }).click(); + + const status = page.getByTestId('execution-status'); + await expect(status).toHaveText('completed', { timeout: 30_000 }); + await expect(page.locator('pre')).toContainText('e2e-write-bash-ok'); + await expect(page.locator('pre')).toContainText('"greeting":"e2e-write-bash"'); + + await page.getByRole('button', { name: 'Delete' }).click(); + await expect(row).toBeHidden({ timeout: 30_000 }); +}); + +test('writes a python script in the UI, runs it and shows its output', async ({ page }, testInfo) => { + const scriptName = writtenNameFor(testInfo, 'python'); + await openScripts(page, 'Test ECU'); + await page.getByRole('button', { name: 'Upload' }).click(); + const dialog = page.getByRole('dialog'); + await dialog.getByRole('button', { name: 'Write script' }).click(); + await dialog.getByLabel('File name').fill(`${scriptName}.py`); + // The only coverage in this repository for the python3 interpreter path: + // the gateway only picks python3 when the uploaded file's name ends in .py. + await writeInEditor( + dialog.locator('[data-testid="script-editor"] .cm-content'), + [ + 'import sys', + 'import json', + '', + 'raw = sys.stdin.read()', + 'params = json.loads(raw) if raw.strip() else {}', + 'print("e2e-write-python-ok:", json.dumps(params, separators=(",", ":")))', + ].join('\n') + ); + // Exact match: "File name" also contains the substring "Name", and + // Playwright's getByLabel matches substrings by default. + await dialog.getByLabel('Name', { exact: true }).fill(scriptName); + await dialog.getByRole('button', { name: 'Upload' }).click(); + await expect(dialog).toBeHidden({ timeout: 30_000 }); + + const row = page.getByRole('button', { name: scriptName }); + await expect(row).toBeVisible({ timeout: 30_000 }); + await row.click(); + + await page.getByPlaceholder('{}').fill('{"greeting":"e2e-write-python"}'); + await page.getByRole('button', { name: 'Run' }).click(); + + const status = page.getByTestId('execution-status'); + await expect(status).toHaveText('completed', { timeout: 30_000 }); + await expect(page.locator('pre')).toContainText('e2e-write-python-ok'); + await expect(page.locator('pre')).toContainText('"greeting":"e2e-write-python"'); + + await page.getByRole('button', { name: 'Delete' }).click(); + await expect(row).toBeHidden({ timeout: 30_000 }); +}); + +test('removes a finished execution record', async ({ page }) => { + await openScripts(page, 'Test ECU'); + await page.getByRole('button', { name: 'Hello' }).click(); + await page.getByRole('button', { name: 'Run' }).click(); + + const status = page.getByTestId('execution-status'); + await expect(status).toHaveText('completed', { timeout: 30_000 }); + await page.getByRole('button', { name: 'Remove' }).click(); + await expect(status).toHaveCount(0, { timeout: 30_000 }); +}); + +test('shows the Scripts tab and the shared script on an app as well', async ({ page }) => { + await openScripts(page, 'Talker'); + const row = page.getByRole('button', { name: 'Hello' }); + await expect(row).toBeVisible({ timeout: 30_000 }); + await expect(row).toContainText('managed'); +}); diff --git a/e2e/smoke.spec.ts b/e2e/smoke.spec.ts new file mode 100644 index 0000000..cfb0b3a --- /dev/null +++ b/e2e/smoke.spec.ts @@ -0,0 +1,20 @@ +// Copyright 2026 bburda +// +// 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 { test, expect } from '@playwright/test'; + +test('connects to the gateway and shows the manifest component', async ({ page }) => { + await page.goto('/'); + await expect(page.getByText('Test ECU')).toBeVisible({ timeout: 30_000 }); +}); diff --git a/e2e/tsconfig.json b/e2e/tsconfig.json new file mode 100644 index 0000000..2f2944c --- /dev/null +++ b/e2e/tsconfig.json @@ -0,0 +1,8 @@ +{ + "extends": "../tsconfig.node.json", + "compilerOptions": { + "lib": ["ES2023", "DOM"], + "tsBuildInfoFile": "./node_modules/.tmp/tsconfig.e2e.tsbuildinfo" + }, + "include": ["../playwright.config.ts", "**/*.ts"] +} diff --git a/playwright.config.ts b/playwright.config.ts new file mode 100644 index 0000000..b506302 --- /dev/null +++ b/playwright.config.ts @@ -0,0 +1,47 @@ +// Copyright 2026 bburda +// +// 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 { defineConfig } from '@playwright/test'; + +const BASE_URL = 'http://localhost:5173'; + +export default defineConfig({ + testDir: './e2e', + globalSetup: './e2e/global-setup.ts', + timeout: 60_000, + expect: { timeout: 30_000 }, + reporter: [['html', { open: 'never' }], ['list']], + // Both projects below hit the same single containerised gateway for + // everything they do not explicitly mock (entity discovery, health, + // faults). Left to Playwright's default per-project parallelism, the + // 'mocked' project's own worker runs concurrently with 'scripts-serial' + // and the resulting burst of simultaneous full-app connections can push + // the gateway's response time past the client's health-check timeout, + // aborting an unrelated connect() and failing an unrelated test. A single + // global worker keeps every test's gateway traffic strictly sequential. + workers: 1, + use: { baseURL: BASE_URL, storageState: 'e2e/.auth/state.json', trace: 'retain-on-failure' }, + webServer: { + command: 'npm run dev -- --port 5173 --strictPort', + url: BASE_URL, + reuseExistingServer: !process.env.CI, + timeout: 120_000, + }, + projects: [ + // These specs mutate shared gateway state (uploads, executions, the global + // 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/ }, + ], +}); diff --git a/tsconfig.json b/tsconfig.json index 04f1a75..070fb00 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -1,6 +1,10 @@ { "files": [], - "references": [{ "path": "./tsconfig.app.json" }, { "path": "./tsconfig.node.json" }], + "references": [ + { "path": "./tsconfig.app.json" }, + { "path": "./tsconfig.node.json" }, + { "path": "./e2e/tsconfig.json" } + ], "compilerOptions": { "baseUrl": ".", "paths": { From d031d22e988c003f705d1dbf5b77e8fb8ff9b553 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 28 Jul 2026 17:09:48 +0200 Subject: [PATCH 02/11] ci: run the Playwright suite against a containerised gateway Adds the end-to-end job alongside the existing checks, dumping the gateway's container logs when a run fails so a container that dies during startup can be diagnosed from the run alone. Documents the Scripts tab and how to run the suite locally. --- .github/workflows/ci.yml | 43 ++++++++++++++++++++++++++++++++++++++++ CONTRIBUTING.md | 28 ++++++++++++++++++++++++++ README.md | 13 +++++++++++- 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d92807e..6d0813f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,49 @@ jobs: - name: Build project run: npm run build + e2e: + runs-on: ubuntu-latest + timeout-minutes: 20 + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: '22' + cache: 'npm' + + - name: Install dependencies + run: npm ci + + - name: Start gateway + run: docker compose -f e2e/docker-compose.yml up -d + + - name: Install Playwright browsers + run: npx playwright install --with-deps chromium + + - name: Run E2E tests + run: npm run test:e2e + + - name: Upload Playwright artifacts + if: failure() + uses: actions/upload-artifact@v4 + with: + name: playwright-report + path: | + playwright-report/ + test-results/ + + - name: Dump gateway logs on failure + if: failure() + run: docker compose -f e2e/docker-compose.yml logs + + - name: Stop gateway + if: always() + run: docker compose -f e2e/docker-compose.yml down -v + docker-build: runs-on: ubuntu-latest timeout-minutes: 15 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 63b50e5..5ebf28c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -87,6 +87,34 @@ Before opening or updating a Pull Request, you **must**: npm run dev ``` +> **Note:** `npm run typecheck` runs `tsc --noEmit` against the root `tsconfig.json`, which has no `files` of its own and therefore checks nothing. Type errors are actually caught by `npm run build` (`tsc -b`, which builds the referenced app, node and e2e project configs). Do not trust a green `typecheck` on its own; run `build` before opening a PR. + +### Running the End-to-End Suite Locally + +The Playwright suite in `e2e/` runs the real UI against a containerised gateway instead of mocks, so it needs Docker. + +1. Start the gateway: + + ```bash + docker compose -f e2e/docker-compose.yml up -d + ``` + +2. Run the suite: + + ```bash + npm run test:e2e + ``` + + Use `npm run test:e2e:ui` instead to step through the tests with the Playwright UI. + +3. Stop the gateway once you are done, dropping the uploads volume along with it: + + ```bash + docker compose -f e2e/docker-compose.yml down -v + ``` + +`e2e/scripts.spec.ts` uploads, runs and deletes scripts against the shared gateway container, mutating its state as it goes, so it and the other specs that touch the live gateway are pinned to a single Playwright worker (see `playwright.config.ts`). Do not attempt to parallelize these specs or run them against a gateway instance you care about keeping in a known state. + ### Pull Request Checklist Before submitting your PR, ensure: diff --git a/README.md b/README.md index b60bc51..ef7259a 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,9 @@ ros2_medkit_web_ui is a lightweight single-page application that connects to a S - **Entity Tree Sidebar** - Browse the hierarchical structure of SOVD entities with lazy-loading, with a readiness lamp on app and component nodes (a green disc for ready, an amber ring for not ready, a grey square for a readiness the UI has not established). The lamp is re-read while the branch is open, so it tracks an entity that stops or comes back - **Entity Detail Panel** - View raw JSON details of any selected entity - **Entity Lifecycle Status Control** - View readiness and request lifecycle transitions (start, restart, force-restart, shutdown, force-shutdown) for apps and components, degrading gracefully on an entity with no lifecycle provider, without taking the entities that have one with it. Actions are gated by the current status (a transition the current status does not allow is marked unavailable and rejected, and stays focusable so the tooltip explaining why reaches a screen reader), and every destructive transition (all but Start) asks for confirmation before dispatch. A transition is only reported as requested when the gateway accepts it; because acceptance is not completion, the readiness is dropped and re-established by the refresh rather than read back straight away +- **Scripts Tab** - List the scripts available on an entity, run one with optional parameters, watch its live status while it executes, see the output once it completes, stop or force-kill a running execution, upload a new script (from a file or written directly in the browser), and delete scripts you no longer need + +> **Note:** The Scripts tab only appears for entities whose gateway reports `capabilities.scripts` in `GET /`, and even then only for apps and components - areas and functions never show it regardless of the capability. The gateway sets this when either a script provider plugin is loaded or `scripts.scripts_dir` is configured; a plugin takes precedence over `scripts_dir`, and when one is loaded `scripts_dir` is ignored. This tool is designed for developers and integrators working with SOVD-compatible systems who need a quick way to explore and debug the entity structure. @@ -84,6 +87,13 @@ npm run test:ui # Run tests with coverage npm run test:coverage +# Run the end-to-end suite against a containerised gateway +docker compose -f e2e/docker-compose.yml up -d +npm run test:e2e + +# Run the end-to-end suite with the Playwright UI +npm run test:e2e:ui + # Format code npm run format @@ -114,7 +124,8 @@ npm run lint - **shadcn/ui** - UI components - **Zustand** - State management - **lucide-react** - Icons -- **Vitest** - Testing framework +- **Vitest** - Unit and component testing framework +- **Playwright** - End-to-end testing against a containerised gateway - **Prettier** - Code formatting - **Husky** - Git hooks From dfb572ccbe91a25ab38a2158f7270d312dd1ed47 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 28 Jul 2026 21:02:54 +0200 Subject: [PATCH 03/11] fix(e2e): harden gateway startup wait, config and cleanup, pin image by digest - Give each gateway health-check attempt in global setup its own timeout via AbortSignal.timeout, since a stalled connect (as opposed to a refused one) would otherwise hang past the overall deadline with no informative error. - Read E2E_APP_URL in playwright.config.ts and derive the dev server's port from it, matching global setup, so the two cannot end up pointed at different addresses. - Scope the "no longer tracks this execution" assertion in the polling-404 scenario to the card that owns the status badge, matching every other assertion in that test. - Make the scripts spec's afterEach cleanup best-effort: a failure opening the panel or removing one leftover script no longer masks the test's own failure or stops the rest of the cleanup from being attempted. - Pin the e2e gateway image by immutable digest instead of a mutable tag, so a later re-publish of the same tag cannot change what CI pulls, keeping the human-readable tag in a comment for reference. --- e2e/docker-compose.yml | 8 ++++++-- e2e/global-setup.ts | 10 +++++++++- e2e/scripts-errors.spec.ts | 2 +- e2e/scripts.spec.ts | 27 +++++++++++++++++++++------ playwright.config.ts | 8 ++++++-- 5 files changed, 43 insertions(+), 12 deletions(-) diff --git a/e2e/docker-compose.yml b/e2e/docker-compose.yml index 2763068..746d6ce 100644 --- a/e2e/docker-compose.yml +++ b/e2e/docker-compose.yml @@ -5,7 +5,8 @@ services: # chowns the volume before the gateway starts; it reuses the pinned # gateway image (which already has chown) instead of pulling another one. init-uploads: - image: ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 + # ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 + image: ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:565db07e1e972b31684bf864fbaad7e8a70aacabf2ef0cd4510fdbd8e3281831 user: root volumes: - e2e-uploads:/e2e-uploads @@ -13,7 +14,10 @@ services: gateway: # Pinned on purpose: :latest is overwritten on every push to the gateway # main branch, which would let unrelated changes turn this repo CI red. - image: ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 + # Pinned by digest, not by the sha-7939c94 tag alone: tags on this + # registry are mutable and a re-run of the publishing workflow on the + # same commit would move one. ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 + image: ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:565db07e1e972b31684bf864fbaad7e8a70aacabf2ef0cd4510fdbd8e3281831 ports: - '${E2E_GATEWAY_PORT:-8080}:8080' volumes: diff --git a/e2e/global-setup.ts b/e2e/global-setup.ts index 2b14e09..8d35ba9 100644 --- a/e2e/global-setup.ts +++ b/e2e/global-setup.ts @@ -19,12 +19,20 @@ const GATEWAY_URL = process.env.E2E_GATEWAY_URL ?? 'http://localhost:8080/api/v1 const APP_URL = process.env.E2E_APP_URL ?? 'http://localhost:5173'; const STORAGE_KEY = 'ros2_medkit_web_ui_server_url'; +// Node's fetch has no default timeout, so a stalled TCP connect (as opposed +// to an outright refused one) would otherwise hang the await forever - the +// deadline below would never get re-checked and global setup would just sit +// there until Playwright's own timeout kills it, looking like a mysterious +// hang instead of "the gateway did not start". Each attempt gets its own +// short timeout so the loop always keeps making progress toward the deadline. +const ATTEMPT_TIMEOUT_MS = 5_000; + async function waitForGateway(): Promise { const deadline = Date.now() + 120_000; let lastError = 'no attempt made'; while (Date.now() < deadline) { try { - const res = await fetch(`${GATEWAY_URL}/health`); + const res = await fetch(`${GATEWAY_URL}/health`, { signal: AbortSignal.timeout(ATTEMPT_TIMEOUT_MS) }); if (res.ok) return; lastError = `HTTP ${res.status}`; } catch (err) { diff --git a/e2e/scripts-errors.spec.ts b/e2e/scripts-errors.spec.ts index 567dfb1..0443d60 100644 --- a/e2e/scripts-errors.spec.ts +++ b/e2e/scripts-errors.spec.ts @@ -196,6 +196,6 @@ test('marks an execution as no longer tracked when polling returns 404', async ( // satisfied by the store's own poll loop picking up the 404 on its next // tick, not by the manual rescue action. const card = page.locator('[data-slot="card"]', { has: status }).last(); - await expect(page.getByText('The gateway no longer tracks this execution')).toBeVisible({ timeout: 30_000 }); + await expect(card.getByText('The gateway no longer tracks this execution')).toBeVisible({ timeout: 30_000 }); await expect(card.getByRole('button', { name: 'Remove' })).toBeVisible({ timeout: 30_000 }); }); diff --git a/e2e/scripts.spec.ts b/e2e/scripts.spec.ts index 5f90d5e..d4de36c 100644 --- a/e2e/scripts.spec.ts +++ b/e2e/scripts.spec.ts @@ -46,14 +46,29 @@ test.afterEach(async ({ page }, testInfo) => { // over from this test (including a previous, unfinished run of it) must be // removed - otherwise the next run would find a duplicate row and a // getByRole('button', { name: uploadedName }) lookup would no longer be unique. - await openScripts(page, 'Test ECU'); + // + // This is best-effort cleanup, not part of the test: if the page is + // already in a broken state because the test itself failed, a throw here + // must not replace that failure in the report, and one leftover failing + // to delete must not stop the others from being attempted. + try { + await openScripts(page, 'Test ECU'); + } catch (err) { + console.warn('afterEach cleanup: could not open the Scripts panel', err); + return; + } + const names = [uploadedNameFor(testInfo), writtenNameFor(testInfo, 'bash'), writtenNameFor(testInfo, 'python')]; for (const name of names) { - const row = page.getByRole('button', { name }); - if (await row.isVisible().catch(() => false)) { - await row.click(); - await page.getByRole('button', { name: 'Delete' }).click(); - await expect(row).toBeHidden({ timeout: 30_000 }); + try { + const row = page.getByRole('button', { name }); + if (await row.isVisible().catch(() => false)) { + await row.click(); + await page.getByRole('button', { name: 'Delete' }).click(); + await expect(row).toBeHidden({ timeout: 30_000 }); + } + } catch (err) { + console.warn(`afterEach cleanup: failed to remove leftover script "${name}"`, err); } } }); diff --git a/playwright.config.ts b/playwright.config.ts index b506302..34ae764 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -14,7 +14,11 @@ import { defineConfig } from '@playwright/test'; -const BASE_URL = 'http://localhost:5173'; +// Read from the same E2E_APP_URL that e2e/global-setup.ts honours, and derive +// the dev server's port from it, so config and setup cannot end up visiting +// two different addresses if only one of them is overridden. +const BASE_URL = process.env.E2E_APP_URL ?? 'http://localhost:5173'; +const APP_PORT = new URL(BASE_URL).port || '5173'; export default defineConfig({ testDir: './e2e', @@ -33,7 +37,7 @@ export default defineConfig({ workers: 1, use: { baseURL: BASE_URL, storageState: 'e2e/.auth/state.json', trace: 'retain-on-failure' }, webServer: { - command: 'npm run dev -- --port 5173 --strictPort', + command: `npm run dev -- --port ${APP_PORT} --strictPort`, url: BASE_URL, reuseExistingServer: !process.env.CI, timeout: 120_000, From b1b3cbc91735ce69c8c8f7c5844c3f87e53989de Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 29 Jul 2026 08:55:58 +0200 Subject: [PATCH 04/11] fix(e2e): keep the gateway loopback-only and harden test isolation The e2e gateway container ran with uploads enabled and executes uploaded shell scripts without authentication, yet published its port on every interface - on a shared or untrusted network that is remote code execution for as long as the container is left running. Bind it to 127.0.0.1 instead, keeping the port override working. Several other rough edges in the harness made it unreliable or misleading: - The toolbar "Upload" button lookup matched by substring, so it also resolved to leftover rows named uploaded__ (which contains "upload"). Match it by exact name instead. - afterEach cleanup checked isVisible() once per leftover name; once two runs left duplicates behind, that check itself threw and nothing got deleted, compounding the problem. Loop on the locator's count and delete .first() until none remain. - The gateway's CORS config only allowed the default dev server origin, so overriding E2E_APP_URL to dodge a busy port failed every request with no CORS error to explain why. Allow any origin - this is a throwaway local/CI fixture with allow_credentials left at its default false, so a wildcard carries none of the risk it would in production. - global-setup only read E2E_GATEWAY_URL, while docker-compose reads E2E_GATEWAY_PORT, so overriding just the port (the natural move) left setup polling the wrong address for its full deadline. Derive the URL from the port when the URL itself is not set. - playwright.config.ts had no forbidOnly, so a committed test.only would pass CI quietly instead of failing it. - The sleeper script and its manifest timeout ran for 300s against a global concurrency cap of 5, so a handful of interrupted runs could exhaust every execution slot with no reset short of destroying the stack. Both are now 30s, still ample for the scenario that stops it mid-run. - The "writes a bash script" scenario uploaded a .sh file, but the gateway only runs .bash under bash - .sh runs under sh like everything else - so it never covered the branch its name claims to. Give it a .bash extension. Documented the E2E_GATEWAY_PORT/E2E_APP_URL override in CONTRIBUTING. --- CONTRIBUTING.md | 9 +++++++++ e2e/docker-compose.yml | 9 ++++++++- e2e/gateway/manifest.yaml | 4 +++- e2e/gateway/params.yaml | 11 ++++++++++- e2e/gateway/scripts/sleep.sh | 10 +++++++++- e2e/global-setup.ts | 10 +++++++++- e2e/scripts.spec.ts | 32 +++++++++++++++++++++++++------- playwright.config.ts | 3 +++ 8 files changed, 76 insertions(+), 12 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5ebf28c..8f44930 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -115,6 +115,15 @@ The Playwright suite in `e2e/` runs the real UI against a containerised gateway `e2e/scripts.spec.ts` uploads, runs and deletes scripts against the shared gateway container, mutating its state as it goes, so it and the other specs that touch the live gateway are pinned to a single Playwright worker (see `playwright.config.ts`). Do not attempt to parallelize these specs or run them against a gateway instance you care about keeping in a known state. +If port 8080 or 5173 is already taken on your machine, override the gateway port and/or the dev server URL before starting the stack: + +```bash +E2E_GATEWAY_PORT=8081 docker compose -f e2e/docker-compose.yml up -d +E2E_GATEWAY_PORT=8081 npm run test:e2e +``` + +`E2E_GATEWAY_PORT` is the only variable you need for the gateway side: `e2e/global-setup.ts` derives the full gateway URL from it, and the gateway's CORS configuration allows any origin so an overridden dev server port is never rejected. Set `E2E_APP_URL` instead (e.g. `E2E_APP_URL=http://localhost:5174`) if the dev server port needs to change; `playwright.config.ts` derives the dev server's port from it. The gateway container stays bound to `127.0.0.1` regardless of the port chosen. + ### Pull Request Checklist Before submitting your PR, ensure: diff --git a/e2e/docker-compose.yml b/e2e/docker-compose.yml index 746d6ce..650b2f7 100644 --- a/e2e/docker-compose.yml +++ b/e2e/docker-compose.yml @@ -19,7 +19,14 @@ services: # same commit would move one. ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 image: ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:565db07e1e972b31684bf864fbaad7e8a70aacabf2ef0cd4510fdbd8e3281831 ports: - - '${E2E_GATEWAY_PORT:-8080}:8080' + # Bound to loopback only, on purpose: this gateway has uploads enabled + # and executes uploaded shell scripts without authentication. + # CONTRIBUTING has developers bring this stack up and leave it running, + # so publishing it on every interface would let anyone else on the same + # network or Wi-Fi execute arbitrary shell on this machine for as long + # as the container is up. Do not drop the `127.0.0.1:` prefix to + # "simplify" this - that reintroduces the exposure. + - '127.0.0.1:${E2E_GATEWAY_PORT:-8080}:8080' volumes: - ./gateway/params.yaml:/e2e/params.yaml:ro - ./gateway/manifest.yaml:/e2e/manifest.yaml:ro diff --git a/e2e/gateway/manifest.yaml b/e2e/gateway/manifest.yaml index edf673a..4a26d4b 100644 --- a/e2e/gateway/manifest.yaml +++ b/e2e/gateway/manifest.yaml @@ -29,6 +29,8 @@ scripts: description: 'Runs long enough to be stopped' path: '/e2e-scripts/sleep.sh' format: 'bash' - timeout_sec: 300 + # Matches sleep.sh's own sleep duration - see the comment there for why + # this is kept short rather than generously long. + timeout_sec: 30 entity_filter: - 'ecu' diff --git a/e2e/gateway/params.yaml b/e2e/gateway/params.yaml index 6a94272..9569153 100644 --- a/e2e/gateway/params.yaml +++ b/e2e/gateway/params.yaml @@ -4,8 +4,17 @@ host: '0.0.0.0' port: 8080 cors: + # '*' rather than a hardcoded 'http://localhost:5173': playwright.config.ts + # and e2e/global-setup.ts both derive the dev server origin from + # E2E_APP_URL, so overriding that variable (e.g. to dodge a busy port) + # would otherwise leave the browser talking to an origin this gateway + # never allowed, failing every request with no + # Access-Control-Allow-Origin header and no mention of CORS anywhere + # in the symptom. This is a throwaway local/CI fixture with + # allow_credentials left at its default false, so a wildcard origin + # carries none of the risk it would in a real deployment. allowed_origins: - - 'http://localhost:5173' + - '*' discovery: mode: 'manifest_only' manifest_path: '/e2e/manifest.yaml' diff --git a/e2e/gateway/scripts/sleep.sh b/e2e/gateway/scripts/sleep.sh index 87dfd87..19befce 100644 --- a/e2e/gateway/scripts/sleep.sh +++ b/e2e/gateway/scripts/sleep.sh @@ -12,4 +12,12 @@ # 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. -sleep 300 +# Kept short on purpose: the gateway's concurrency cap is 5 and it is global, +# not per script. A stopped-but-still-running execution (e.g. from a test run +# interrupted mid-scenario) holds its slot until this sleep exits naturally, +# and a running execution cannot be deleted - so a handful of interrupted runs +# inside a long sleep window would block every execution the suite tries to +# start afterwards, with no reset short of destroying the stack. 30 seconds is +# still ample for the "stop a running script" scenario, which stops it within +# a second or two of starting. +sleep 30 diff --git a/e2e/global-setup.ts b/e2e/global-setup.ts index 8d35ba9..34a1a55 100644 --- a/e2e/global-setup.ts +++ b/e2e/global-setup.ts @@ -15,7 +15,15 @@ import { chromium } from '@playwright/test'; import { mkdirSync } from 'node:fs'; -const GATEWAY_URL = process.env.E2E_GATEWAY_URL ?? 'http://localhost:8080/api/v1'; +// e2e/docker-compose.yml publishes the gateway container on E2E_GATEWAY_PORT, +// not E2E_GATEWAY_URL - deriving the URL's port from it here means overriding +// the one variable that actually changes (the port, e.g. because 8080 is +// already taken) is enough. Without this, setting only E2E_GATEWAY_PORT would +// leave global setup polling the old default port for the full two-minute +// deadline before failing. An explicit E2E_GATEWAY_URL still wins outright, +// for the rarer case where the host part needs to change too. +const GATEWAY_PORT = process.env.E2E_GATEWAY_PORT ?? '8080'; +const GATEWAY_URL = process.env.E2E_GATEWAY_URL ?? `http://localhost:${GATEWAY_PORT}/api/v1`; const APP_URL = process.env.E2E_APP_URL ?? 'http://localhost:5173'; const STORAGE_KEY = 'ros2_medkit_web_ui_server_url'; diff --git a/e2e/scripts.spec.ts b/e2e/scripts.spec.ts index d4de36c..2322fab 100644 --- a/e2e/scripts.spec.ts +++ b/e2e/scripts.spec.ts @@ -61,11 +61,21 @@ test.afterEach(async ({ page }, testInfo) => { const names = [uploadedNameFor(testInfo), writtenNameFor(testInfo, 'bash'), writtenNameFor(testInfo, 'python')]; for (const name of names) { try { + // A substring, case-insensitive match on `name` can resolve to more + // than one row: workerIndex and repeatEachIndex are both 0 on an + // ordinary run, so uploadedNameFor/writtenNameFor produce the same + // name across separate runs, and the gateway happily accepts + // duplicate script names as separate entities. Loop on the + // locator's count instead of a single isVisible() check, deleting + // .first() each time, so every leftover is removed rather than + // only a uniquely-named one. const row = page.getByRole('button', { name }); - if (await row.isVisible().catch(() => false)) { - await row.click(); + let remaining = await row.count(); + while (remaining > 0) { + await row.first().click(); await page.getByRole('button', { name: 'Delete' }).click(); - await expect(row).toBeHidden({ timeout: 30_000 }); + await expect(row).toHaveCount(remaining - 1, { timeout: 30_000 }); + remaining = await row.count(); } } catch (err) { console.warn(`afterEach cleanup: failed to remove leftover script "${name}"`, err); @@ -141,7 +151,12 @@ test('stops a running script and reports it as stopped, not failed', async ({ pa test('uploads, runs and deletes a script', async ({ page }, testInfo) => { const uploadedName = uploadedNameFor(testInfo); await openScripts(page, 'Test ECU'); - await page.getByRole('button', { name: 'Upload' }).click(); + // exact: true - Playwright's role/name matching is a case-insensitive + // substring match by default, and script rows carry the script name as + // their accessible name. Leftover fixtures from this file are named + // `uploaded__`, which contains "upload", so a non-exact + // lookup for the toolbar button also resolves to every leftover row. + await page.getByRole('button', { name: 'Upload', exact: true }).click(); const dialog = page.getByRole('dialog'); await dialog.getByLabel('File').setInputFiles(path.join(import.meta.dirname, 'fixtures', 'uploaded-script.sh')); await dialog.getByLabel('Name').fill(uploadedName); @@ -165,10 +180,13 @@ test('uploads, runs and deletes a script', async ({ page }, testInfo) => { test('writes a bash script in the UI, runs it and shows its output', async ({ page }, testInfo) => { const scriptName = writtenNameFor(testInfo, 'bash'); await openScripts(page, 'Test ECU'); - await page.getByRole('button', { name: 'Upload' }).click(); + await page.getByRole('button', { name: 'Upload', exact: true }).click(); const dialog = page.getByRole('dialog'); await dialog.getByRole('button', { name: 'Write script' }).click(); - await dialog.getByLabel('File name').fill(`${scriptName}.sh`); + // .bash, not .sh: the gateway only runs a script under bash when its name + // ends in .bash - .sh, like everything else, runs under sh - so this is + // the only extension that actually exercises the bash interpreter path. + await dialog.getByLabel('File name').fill(`${scriptName}.bash`); // Reachable by role and accessible name against the real CodeMirror // instance, not just the mocked editor the unit tests exercise - this is // the only place that would catch ScriptEditor's aria-label regressing. @@ -202,7 +220,7 @@ test('writes a bash script in the UI, runs it and shows its output', async ({ pa test('writes a python script in the UI, runs it and shows its output', async ({ page }, testInfo) => { const scriptName = writtenNameFor(testInfo, 'python'); await openScripts(page, 'Test ECU'); - await page.getByRole('button', { name: 'Upload' }).click(); + await page.getByRole('button', { name: 'Upload', exact: true }).click(); const dialog = page.getByRole('dialog'); await dialog.getByRole('button', { name: 'Write script' }).click(); await dialog.getByLabel('File name').fill(`${scriptName}.py`); diff --git a/playwright.config.ts b/playwright.config.ts index 34ae764..ec1207c 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -23,6 +23,9 @@ const APP_PORT = new URL(BASE_URL).port || '5173'; export default defineConfig({ testDir: './e2e', globalSetup: './e2e/global-setup.ts', + // Fails the run in CI if a `test.only` was committed, instead of silently + // running just that one test and reporting a spuriously green suite. + forbidOnly: !!process.env.CI, timeout: 60_000, expect: { timeout: 30_000 }, reporter: [['html', { open: 'never' }], ['list']], From 977120286544d3dbd6ebf6d9ff1fd52977628b6b Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 29 Jul 2026 08:56:07 +0200 Subject: [PATCH 05/11] ci: run pull_request checks regardless of target branch pull_request previously triggered only for PRs targeting main, so a PR targeting any other branch got no CI signal at all. Drop the branch filter on pull_request (push stays scoped to main) so every pull request runs the check, e2e and docker-build jobs. --- .github/workflows/ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6d0813f..c4dd126 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,7 +2,6 @@ name: CI on: pull_request: - branches: [main] push: branches: [main] From ebc9c5b1e78850c2886a0d670c72e7c6bda8d481 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 29 Jul 2026 09:51:15 +0200 Subject: [PATCH 06/11] fix(e2e): accept the delete confirmation dialog in scripts specs ScriptRow's handleDelete now calls window.confirm before deleting a script. Playwright auto-dismisses a native dialog with no handler, which returns false and turns every delete into a silent no-op, so every spec that clicks Delete needs to accept the dialog explicitly. Add clickDeleteAndConfirm, a shared helper that registers the dialog listener before the click (accepting after the click would deadlock, since window.confirm blocks the page's JS until the dialog is resolved) and asserts the dialog actually appeared with the expected message, so a removed confirmation guard would fail the test rather than pass silently. Use it everywhere a spec deletes a script: the upload/run/delete scenario, both write-script scenarios, the managed-script-rejection scenario, and the shared afterEach cleanup that removes leftovers between runs. --- e2e/dialog-helpers.ts | 44 ++++++++++++++++++++++++++++++++++++++ e2e/scripts-errors.spec.ts | 3 ++- e2e/scripts.spec.ts | 9 ++++---- 3 files changed, 51 insertions(+), 5 deletions(-) create mode 100644 e2e/dialog-helpers.ts diff --git a/e2e/dialog-helpers.ts b/e2e/dialog-helpers.ts new file mode 100644 index 0000000..794bd67 --- /dev/null +++ b/e2e/dialog-helpers.ts @@ -0,0 +1,44 @@ +// Copyright 2026 bburda +// +// 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 { expect, type Dialog, type Page } from '@playwright/test'; + +/** + * Clicks the (already-visible, uniquely-matched) Delete button for + * `scriptName` and accepts the native `confirm()` dialog that ScriptRow's + * handleDelete requires before it calls deleteScript. + * + * The listener is registered before the click and accepts inline as soon as + * the dialog opens, rather than after awaiting the click: `window.confirm` + * blocks the page's JS (and, with it, the click action itself) until the + * dialog is resolved, so anything that awaits the click before calling + * `dialog.accept()` would deadlock - the click can never settle first. + * + * Asserts the dialog actually appeared, with the expected message, instead + * of accepting whatever dialog (if any) shows up - a bare accept-everything + * handler would still pass the day someone removes the confirmation guard by + * accident. + */ +export async function clickDeleteAndConfirm(page: Page, scriptName: string): Promise { + let seenDialog: Dialog | undefined; + page.once('dialog', async (dialog) => { + seenDialog = dialog; + await dialog.accept(); + }); + + await page.getByRole('button', { name: 'Delete' }).click(); + + expect(seenDialog?.type()).toBe('confirm'); + expect(seenDialog?.message()).toBe(`Delete script "${scriptName}"? This cannot be undone.`); +} diff --git a/e2e/scripts-errors.spec.ts b/e2e/scripts-errors.spec.ts index 0443d60..af77375 100644 --- a/e2e/scripts-errors.spec.ts +++ b/e2e/scripts-errors.spec.ts @@ -13,6 +13,7 @@ // limitations under the License. import { test, expect, type Page } from '@playwright/test'; +import { clickDeleteAndConfirm } from './dialog-helpers'; async function selectTestEcu(page: Page): Promise { await page.goto('/'); @@ -139,7 +140,7 @@ test('reports that a managed script cannot be deleted', async ({ page }) => { await openScripts(page); await page.getByRole('button', { name: 'Hello' }).click(); - await page.getByRole('button', { name: 'Delete' }).click(); + await clickDeleteAndConfirm(page, 'Hello'); await expect(page.getByText('Cannot delete managed script: hello')).toBeVisible({ timeout: 30_000 }); }); diff --git a/e2e/scripts.spec.ts b/e2e/scripts.spec.ts index 2322fab..5931ace 100644 --- a/e2e/scripts.spec.ts +++ b/e2e/scripts.spec.ts @@ -14,6 +14,7 @@ import path from 'node:path'; import { test, expect, type Locator, type Page, type TestInfo } from '@playwright/test'; +import { clickDeleteAndConfirm } from './dialog-helpers'; async function openScripts(page: Page, entity: 'Test ECU' | 'Talker'): Promise { await page.goto('/'); @@ -73,7 +74,7 @@ test.afterEach(async ({ page }, testInfo) => { let remaining = await row.count(); while (remaining > 0) { await row.first().click(); - await page.getByRole('button', { name: 'Delete' }).click(); + await clickDeleteAndConfirm(page, name); await expect(row).toHaveCount(remaining - 1, { timeout: 30_000 }); remaining = await row.count(); } @@ -173,7 +174,7 @@ test('uploads, runs and deletes a script', async ({ page }, testInfo) => { const status = page.getByTestId('execution-status'); await expect(status).toHaveText('completed', { timeout: 30_000 }); - await page.getByRole('button', { name: 'Delete' }).click(); + await clickDeleteAndConfirm(page, uploadedName); await expect(row).toBeHidden({ timeout: 30_000 }); }); @@ -213,7 +214,7 @@ test('writes a bash script in the UI, runs it and shows its output', async ({ pa await expect(page.locator('pre')).toContainText('e2e-write-bash-ok'); await expect(page.locator('pre')).toContainText('"greeting":"e2e-write-bash"'); - await page.getByRole('button', { name: 'Delete' }).click(); + await clickDeleteAndConfirm(page, scriptName); await expect(row).toBeHidden({ timeout: 30_000 }); }); @@ -255,7 +256,7 @@ test('writes a python script in the UI, runs it and shows its output', async ({ await expect(page.locator('pre')).toContainText('e2e-write-python-ok'); await expect(page.locator('pre')).toContainText('"greeting":"e2e-write-python"'); - await page.getByRole('button', { name: 'Delete' }).click(); + await clickDeleteAndConfirm(page, scriptName); await expect(row).toBeHidden({ timeout: 30_000 }); }); From 1058fe0e0eb1e375f9d8bb89c188cbd78abcb3ed Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sat, 15 Aug 2026 17:18:51 +0200 Subject: [PATCH 07/11] 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 c6bbc5e..80bf2e3 100644 --- a/src/lib/store.ts +++ b/src/lib/store.ts @@ -51,7 +51,6 @@ import { putEntityDataItem, deleteEntityConfiguration, deleteEntityConfigurations, - getEntityBulkData, getEntityLogs, getEntityLogsConfiguration, putEntityLogsConfiguration, @@ -979,6 +978,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) => ({ @@ -2759,13 +2790,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)}`; @@ -2776,6 +2800,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 00711451741c1951ac9a85ec31d22788d3e36b9f Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sun, 16 Aug 2026 13:05:29 +0200 Subject: [PATCH 08/11] 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 4a0dbb7a088838467896815660f771e857d313c2 Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sun, 16 Aug 2026 15:11:15 +0200 Subject: [PATCH 09/11] 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 17c91a6105a3823a4734023a5c1ce2c510d10704 Mon Sep 17 00:00:00 2001 From: Michal Faferek Date: Thu, 20 Aug 2026 17:14:12 +0200 Subject: [PATCH 10/11] 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 80bf2e3..9931b72 100644 --- a/src/lib/store.ts +++ b/src/lib/store.ts @@ -981,32 +981,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 d39dc0642556e4b1789b545750d598b793313e37 Mon Sep 17 00:00:00 2001 From: Michal Faferek Date: Thu, 20 Aug 2026 18:19:12 +0200 Subject: [PATCH 11/11] 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=$!;