From 45ebc346fa8bb34263faee27d8610e6591cdd80d Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 14 Apr 2026 21:36:15 +0200 Subject: [PATCH 1/2] fix: harden Data tab against sibling transform crashes and entity races Three independent failure modes that all surface as "No data available for this component." even when the data endpoint returns items: 1. transformFault now accepts status as either a string or an object with an aggregatedStatus field, accepts code/fault_name as fallbacks for fault_code/description, and tolerates an undefined severity. Without these guards a single malformed fault payload threw inside a sibling transform and rejected the whole Promise.all in prefetchResourceCounts. 2. prefetchResourceCounts now wraps each per-resource transform in its own try/catch via a small safeCount helper. One bad resource can no longer wipe out the others. 3. The EntityDetailPanel resource-counts effect now sets a cancelled flag in its cleanup so a late Promise.all from a previously-selected entity cannot overwrite the new entity's topicsData. Also adds a typed access field to ComponentTopic, plumbed from x-medkit.access. DataPanel uses it to hide the write section for access==='read' and to label the section "Write Value" instead of "Publish Message" for scalar writes. closes #67 --- src/components/DataPanel.tsx | 16 ++++-- src/components/EntityDetailPanel.tsx | 10 ++++ src/lib/store.ts | 21 ++++++-- src/lib/transforms.test.ts | 81 ++++++++++++++++++++++++++++ src/lib/transforms.ts | 47 +++++++++++----- src/lib/types.ts | 4 ++ 6 files changed, 158 insertions(+), 21 deletions(-) diff --git a/src/components/DataPanel.tsx b/src/components/DataPanel.tsx index 584eebc..7f5b4dd 100644 --- a/src/components/DataPanel.tsx +++ b/src/components/DataPanel.tsx @@ -201,7 +201,15 @@ export function DataPanel({ const isConnected = useAppStore((state) => state.isConnected); const hasData = topic.status === 'data' && topic.data !== null && topic.data !== undefined; - const canPublish = isConnected && !!(topic.type || topic.type_info || topic.data); + // `access` is the explicit per-item write capability; when present it + // overrides the legacy "any typed topic is publishable" heuristic so a + // read-only data item never surfaces a write form. + const canWrite = isConnected && topic.access !== 'read' && !!(topic.type || topic.type_info || topic.data); + // Use "Write Value" when the gateway told us this is a writable scalar + // (access === 'write' / 'readwrite'); fall back to "Publish Message" for + // streaming topics where the operation really is a publish. + const writeSectionLabel = + topic.access === 'write' || topic.access === 'readwrite' ? 'Write Value' : 'Publish Message'; const handleCopyFromLast = () => { if (topic.data) { @@ -270,10 +278,10 @@ export function DataPanel({ )} - {/* Publish Section */} - {canPublish && ( + {/* Write/Publish Section */} + {canWrite && (
- Publish Message + {writeSectionLabel} { // Mark topicsData as "not loaded yet for the current entity" so the // Data tab renders a skeleton instead of an empty-state flash while @@ -463,6 +467,8 @@ export function EntityDetailPanel({ onConnectClick, viewMode = 'entity', onEntit fetchEntityData(entityType, entityId).catch(() => [] as ComponentTopic[]), ]); + if (cancelled) return; + // Store the fetched data for the Data tab const fetchedData = Array.isArray(dataRes) ? dataRes : []; setTopicsData(fetchedData); @@ -470,6 +476,7 @@ export function EntityDetailPanel({ onConnectClick, viewMode = 'entity', onEntit // Use the already-fetched data length instead of a separate request setResourceCounts({ ...counts, data: fetchedData.length, logs: 0 }); } catch { + if (cancelled) return; // On unexpected failure fall back to "loaded empty" so the UI // doesn't get stuck showing the skeleton forever. setTopicsData([]); @@ -477,6 +484,9 @@ export function EntityDetailPanel({ onConnectClick, viewMode = 'entity', onEntit }; doFetchResourceCounts(); + return () => { + cancelled = true; + }; }, [selectedEntity, prefetchResourceCounts, fetchEntityData]); const handleCopyEntity = async () => { diff --git a/src/lib/store.ts b/src/lib/store.ts index cc8a462..8a63c4d 100644 --- a/src/lib/store.ts +++ b/src/lib/store.ts @@ -2151,13 +2151,28 @@ export const useAppStore = create()( getEntityFaults(client, entityType, entityId).catch(() => ({ data: undefined, error: undefined })), ]); + // Isolate each transform call: a malformed payload from one + // resource (e.g. UDS DTC faults with non-canonical schema) + // must not crash the others or the caller's Promise.all. + const safeCount = (fn: () => T, fallback: T): T => { + try { + return fn(); + } catch { + return fallback; + } + }; return { data: 0, - operations: opsRes.data ? unwrapItems(opsRes.data).length : 0, + operations: opsRes.data ? safeCount(() => unwrapItems(opsRes.data).length, 0) : 0, configurations: configRes.data - ? transformConfigurationsResponse(configRes.data, entityId).parameters.length + ? safeCount( + () => transformConfigurationsResponse(configRes.data, entityId).parameters.length, + 0 + ) + : 0, + faults: faultsRes.data + ? safeCount(() => transformFaultsResponse(faultsRes.data).items.length, 0) : 0, - faults: faultsRes.data ? transformFaultsResponse(faultsRes.data).items.length : 0, }; }, diff --git a/src/lib/transforms.test.ts b/src/lib/transforms.test.ts index 1aa86d5..67cdc8c 100644 --- a/src/lib/transforms.test.ts +++ b/src/lib/transforms.test.ts @@ -270,6 +270,63 @@ describe('transformFault', () => { const result = transformFault(makeFaultInput({ status: 'UNKNOWN_STATUS' })); expect(result.status).toBe('active'); }); + + it('reads aggregatedStatus when status is an object', () => { + const result = transformFault(makeFaultInput({ status: { aggregatedStatus: 'active', extra: '1' } })); + expect(result.status).toBe('active'); + }); + + it('maps object aggregatedStatus "passive" to pending', () => { + const result = transformFault(makeFaultInput({ status: { aggregatedStatus: 'passive' } })); + expect(result.status).toBe('pending'); + }); + + it('does not throw when status is null', () => { + const result = transformFault(makeFaultInput({ status: null })); + expect(result.status).toBe('active'); + }); + + it('does not throw when status is missing', () => { + const result = transformFault(makeFaultInput({ status: undefined })); + expect(result.status).toBe('active'); + }); + }); + + describe('field aliases', () => { + it('accepts code as a fallback for fault_code', () => { + const result = transformFault({ + code: 'ALT_CODE', + description: 'x', + severity: 1, + severity_label: 'warn', + status: 'CONFIRMED', + first_occurred: 1700000000, + } as never); + expect(result.code).toBe('ALT_CODE'); + }); + + it('accepts fault_name as a fallback for description', () => { + const result = transformFault({ + fault_code: 'F1', + fault_name: 'Alternative description', + severity: 1, + severity_label: 'warn', + status: 'CONFIRMED', + first_occurred: 1700000000, + } as never); + expect(result.message).toBe('Alternative description'); + }); + + it('does not throw when severity is undefined', () => { + const result = transformFault({ + fault_code: 'F1', + description: 'x', + severity_label: 'warn', + status: 'CONFIRMED', + first_occurred: 1700000000, + } as never); + expect(result.severity).toBe('warning'); + }); }); it('includes occurrence metadata in parameters', () => { @@ -558,6 +615,30 @@ describe('transformDataResponse', () => { const result = transformDataResponse({ items: [raw] }); expect(result[0]?.uniqueKey).toBe('x:publish'); }); + + it('passes through access="read"', () => { + const raw = { id: 'x', name: 'x', 'x-medkit': { access: 'read' } }; + const result = transformDataResponse({ items: [raw] }); + expect(result[0]?.access).toBe('read'); + }); + + it('passes through access="readwrite"', () => { + const raw = { id: 'x', name: 'x', 'x-medkit': { access: 'readwrite' } }; + const result = transformDataResponse({ items: [raw] }); + expect(result[0]?.access).toBe('readwrite'); + }); + + it('lowercases access for case-insensitive matching', () => { + const raw = { id: 'x', name: 'x', 'x-medkit': { access: 'WRITE' } }; + const result = transformDataResponse({ items: [raw] }); + expect(result[0]?.access).toBe('write'); + }); + + it('drops unrecognised access values', () => { + const raw = { id: 'x', name: 'x', 'x-medkit': { access: 'execute' } }; + const result = transformDataResponse({ items: [raw] }); + expect(result[0]?.access).toBeUndefined(); + }); }); }); diff --git a/src/lib/transforms.ts b/src/lib/transforms.ts index d8ff805..38dd9f5 100644 --- a/src/lib/transforms.ts +++ b/src/lib/transforms.ts @@ -59,14 +59,21 @@ export function unwrapItems(response: unknown): T[] { * Raw fault item shape returned by the gateway faults endpoints. */ export interface RawFaultItem { - fault_code: string; - description: string; - severity: number; - severity_label: string; - status: string; + /** Canonical SOVD identifier. `code` accepted as a fallback for backends + * that have not yet aligned on the SOVD field name. */ + fault_code?: string; + code?: string; + /** Human-readable description. `fault_name` accepted as a fallback. */ + description?: string; + fault_name?: string; + severity?: number; + severity_label?: string; + /** Canonical value is a string. An object form with `aggregatedStatus` + * is also accepted to keep the UI from crashing on payload drift. */ + status?: string | { aggregatedStatus?: string; [key: string]: unknown } | null; /** Accepted as unix seconds (number), ISO 8601 string, or missing/invalid; * `transformFault` normalises all of these to an ISO timestamp. */ - first_occurred: number | string | null | undefined; + first_occurred?: number | string | null; last_occurred?: number | string | null; occurrence_count?: number; reporting_sources?: string[]; @@ -91,21 +98,29 @@ export function transformFault(apiFault: RawFaultItem): Fault { // Map severity number/label to FaultSeverity. // Label check takes priority over numeric value; critical is checked first. let severity: FaultSeverity = 'info'; + const severityNum = typeof apiFault.severity === 'number' ? apiFault.severity : 0; const label = apiFault.severity_label?.toLowerCase() || ''; - if (label === 'critical' || apiFault.severity >= 3) { + if (label === 'critical' || severityNum >= 3) { severity = 'critical'; - } else if (label === 'error' || apiFault.severity === 2) { + } else if (label === 'error' || severityNum === 2) { severity = 'error'; - } else if (label === 'warn' || label === 'warning' || apiFault.severity === 1) { + } else if (label === 'warn' || label === 'warning' || severityNum === 1) { severity = 'warning'; } - // Map API status string to FaultStatusValue. + // Map API status to FaultStatusValue. Accept either a string (canonical SOVD) + // or an object with an `aggregatedStatus` field (UDS DTC-style backends). + let apiStatus = ''; + if (typeof apiFault.status === 'string') { + apiStatus = apiFault.status.toLowerCase(); + } else if (apiFault.status && typeof apiFault.status === 'object') { + const agg = (apiFault.status as { aggregatedStatus?: unknown }).aggregatedStatus; + if (typeof agg === 'string') apiStatus = agg.toLowerCase(); + } let status: FaultStatusValue = 'active'; - const apiStatus = apiFault.status?.toLowerCase() || ''; if (apiStatus === 'confirmed' || apiStatus === 'active') { status = 'active'; - } else if (apiStatus === 'pending' || apiStatus === 'prefailed') { + } else if (apiStatus === 'pending' || apiStatus === 'prefailed' || apiStatus === 'passive') { status = 'pending'; } else if (apiStatus === 'cleared' || apiStatus === 'resolved') { status = 'cleared'; @@ -125,8 +140,8 @@ export function transformFault(apiFault: RawFaultItem): Fault { const entity_type = apiFault.entity_type || 'app'; return { - code: apiFault.fault_code, - message: apiFault.description, + code: apiFault.fault_code ?? apiFault.code ?? 'unknown', + message: apiFault.description ?? apiFault.fault_name ?? '', severity, status, timestamp: (() => { @@ -328,6 +343,9 @@ export function transformDataResponse(rawData: unknown): ComponentTopic[] { // `direction` above. const typeLabel = xm?.ros2?.type ?? xm?.type; const hasValue = item.value !== undefined; + const rawAccess = xm?.access?.toLowerCase(); + const access: 'read' | 'write' | 'readwrite' | undefined = + rawAccess === 'read' || rawAccess === 'write' || rawAccess === 'readwrite' ? rawAccess : undefined; return { topic: topicName, timestamp: Date.now(), @@ -344,6 +362,7 @@ export function transformDataResponse(rawData: unknown): ComponentTopic[] { isPublisher: direction === 'publish' || direction === 'both' || direction === 'output', isSubscriber: direction === 'subscribe' || direction === 'both' || direction === 'input', uniqueKey: direction ? `${topicName}:${direction}` : topicName, + access, }; }); } diff --git a/src/lib/types.ts b/src/lib/types.ts index ab28f78..b874fba 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -214,6 +214,10 @@ export interface ComponentTopic { isSubscriber?: boolean; /** Unique key combining topic and direction for React key */ uniqueKey?: string; + /** Access mode from `x-medkit.access`. `'read'` hides the write section, + * `'write'` / `'readwrite'` enable it. Absent means "no constraint" (the + * legacy ROS 2 behaviour where any topic with a known type may publish). */ + access?: 'read' | 'write' | 'readwrite'; } /** From bc9a9e1d43cf12a966660e2222d636081d750a23 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 14 Apr 2026 22:29:36 +0200 Subject: [PATCH 2/2] fix: harden access parsing and abort stale resource-count fetches - Guard `xm?.access` with a `typeof === 'string'` check before lowercasing so non-string vendor payloads no longer throw inside the data transform. - Abort in-flight resource-count and entity-data fetches in the EntityDetailPanel cleanup via AbortController, so rapid entity navigation no longer piles up wasted requests. Threaded `signal` through prefetchResourceCounts to its three underlying GETs. - Replace `as never` casts in transformFault alias tests with `as unknown as RawFaultItem` to keep the cast intent typed. --- src/components/EntityDetailPanel.tsx | 11 +++++++---- src/lib/store.ts | 18 +++++++++++++----- src/lib/transforms.test.ts | 7 ++++--- src/lib/transforms.ts | 2 +- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/components/EntityDetailPanel.tsx b/src/components/EntityDetailPanel.tsx index 10e445f..12baed2 100644 --- a/src/components/EntityDetailPanel.tsx +++ b/src/components/EntityDetailPanel.tsx @@ -426,8 +426,10 @@ export function EntityDetailPanel({ onConnectClick, viewMode = 'entity', onEntit logs: 0, }; // Guard against late results from a previous entity overwriting the - // current entity's state. The cleanup flips `cancelled` so any setState - // calls after the entity changed become no-ops. + // current entity's state. The cleanup aborts in-flight requests AND + // flips `cancelled` so any setState calls after the entity changed + // become no-ops (covers transforms that ignore the abort signal). + const controller = new AbortController(); let cancelled = false; const doFetchResourceCounts = async () => { // Mark topicsData as "not loaded yet for the current entity" so the @@ -463,8 +465,8 @@ export function EntityDetailPanel({ onConnectClick, viewMode = 'entity', onEntit try { // Fetch resource counts and data in parallel const [counts, dataRes] = await Promise.all([ - prefetchResourceCounts(entityType, entityId), - fetchEntityData(entityType, entityId).catch(() => [] as ComponentTopic[]), + prefetchResourceCounts(entityType, entityId, controller.signal), + fetchEntityData(entityType, entityId, controller.signal).catch(() => [] as ComponentTopic[]), ]); if (cancelled) return; @@ -486,6 +488,7 @@ export function EntityDetailPanel({ onConnectClick, viewMode = 'entity', onEntit doFetchResourceCounts(); return () => { cancelled = true; + controller.abort(); }; }, [selectedEntity, prefetchResourceCounts, fetchEntityData]); diff --git a/src/lib/store.ts b/src/lib/store.ts index 8a63c4d..54f8264 100644 --- a/src/lib/store.ts +++ b/src/lib/store.ts @@ -213,7 +213,8 @@ export interface AppState { getFunctionHosts: (functionId: string) => Promise; prefetchResourceCounts: ( entityType: SovdResourceEntityType, - entityId: string + entityId: string, + signal?: AbortSignal ) => Promise<{ data: number; operations: number; configurations: number; faults: number }>; } @@ -2132,7 +2133,11 @@ export const useAppStore = create()( return data ? unwrapItems(data) : []; }, - prefetchResourceCounts: async (entityType: SovdResourceEntityType, entityId: string) => { + prefetchResourceCounts: async ( + entityType: SovdResourceEntityType, + entityId: string, + signal?: AbortSignal + ) => { const { client } = get(); if (!client) return { data: 0, operations: 0, configurations: 0, faults: 0 }; @@ -2140,15 +2145,18 @@ export const useAppStore = create()( // The caller (EntityDetailPanel) already fetches entity data via fetchEntityData // and overrides counts.data with the result length. const [opsRes, configRes, faultsRes] = await Promise.all([ - getEntityOperations(client, entityType, entityId).catch(() => ({ + getEntityOperations(client, entityType, entityId, signal).catch(() => ({ + data: undefined, + error: undefined, + })), + getEntityConfigurations(client, entityType, entityId, signal).catch(() => ({ data: undefined, error: undefined, })), - getEntityConfigurations(client, entityType, entityId).catch(() => ({ + getEntityFaults(client, entityType, entityId, signal).catch(() => ({ data: undefined, error: undefined, })), - getEntityFaults(client, entityType, entityId).catch(() => ({ data: undefined, error: undefined })), ]); // Isolate each transform call: a malformed payload from one diff --git a/src/lib/transforms.test.ts b/src/lib/transforms.test.ts index 67cdc8c..3fde215 100644 --- a/src/lib/transforms.test.ts +++ b/src/lib/transforms.test.ts @@ -21,6 +21,7 @@ import { transformDataResponse, transformConfigurationsResponse, transformBulkDataDescriptor, + type RawFaultItem, } from './transforms'; // ============================================================================= @@ -301,7 +302,7 @@ describe('transformFault', () => { severity_label: 'warn', status: 'CONFIRMED', first_occurred: 1700000000, - } as never); + } as unknown as RawFaultItem); expect(result.code).toBe('ALT_CODE'); }); @@ -313,7 +314,7 @@ describe('transformFault', () => { severity_label: 'warn', status: 'CONFIRMED', first_occurred: 1700000000, - } as never); + } as unknown as RawFaultItem); expect(result.message).toBe('Alternative description'); }); @@ -324,7 +325,7 @@ describe('transformFault', () => { severity_label: 'warn', status: 'CONFIRMED', first_occurred: 1700000000, - } as never); + } as unknown as RawFaultItem); expect(result.severity).toBe('warning'); }); }); diff --git a/src/lib/transforms.ts b/src/lib/transforms.ts index 38dd9f5..2e37777 100644 --- a/src/lib/transforms.ts +++ b/src/lib/transforms.ts @@ -343,7 +343,7 @@ export function transformDataResponse(rawData: unknown): ComponentTopic[] { // `direction` above. const typeLabel = xm?.ros2?.type ?? xm?.type; const hasValue = item.value !== undefined; - const rawAccess = xm?.access?.toLowerCase(); + const rawAccess = typeof xm?.access === 'string' ? xm.access.toLowerCase() : undefined; const access: 'read' | 'write' | 'readwrite' | undefined = rawAccess === 'read' || rawAccess === 'write' || rawAccess === 'readwrite' ? rawAccess : undefined; return {