From 45ebc346fa8bb34263faee27d8610e6591cdd80d Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 14 Apr 2026 21:36:15 +0200 Subject: [PATCH 1/5] 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/5] 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 { From 14e5e0042377054805810e2fcf72719eae950b42 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 19 Apr 2026 13:16:05 +0200 Subject: [PATCH 3/5] fix: prevent fault-code collisions and zero-value write-UI hiding - transformFault: guard severity_label with typeof before lowercasing so non-string payloads do not throw. - transformFault: when both fault_code and code are missing, generate a synthetic per-call id instead of the literal 'unknown'. A shared literal collapses store dedup (keyed by code+entity_id), duplicates React keys in the faults lists, and causes the expand-state Sets to toggle unrelated rows together. - DataPanel: split the write-form condition so an explicit access='write'/'readwrite' always enables the form and the fallback typed-topic heuristic checks data presence rather than truthiness. Fixes the write section disappearing when the last observed value is a falsy scalar (0, false, '') and no type hint is reported. - Add regression tests for synthetic fault codes and for the DataPanel canWrite logic across access modes. --- src/components/DataPanel.test.tsx | 71 +++++++++++++++++++++++++++++++ src/components/DataPanel.tsx | 12 +++++- src/lib/transforms.test.ts | 16 +++++++ src/lib/transforms.ts | 15 ++++++- 4 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 src/components/DataPanel.test.tsx diff --git a/src/components/DataPanel.test.tsx b/src/components/DataPanel.test.tsx new file mode 100644 index 0000000..616a1b7 --- /dev/null +++ b/src/components/DataPanel.test.tsx @@ -0,0 +1,71 @@ +// 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 + +import { describe, it, expect, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { DataPanel } from './DataPanel'; +import type { ComponentTopic } from '@/lib/types'; + +vi.mock('@/lib/store', () => ({ + useAppStore: vi.fn((selector: (s: { isConnected: boolean }) => unknown) => selector({ isConnected: true })), +})); + +vi.mock('@/components/TopicPublishForm', () => ({ + TopicPublishForm: () =>
, +})); + +vi.mock('@/components/JsonFormViewer', () => ({ + JsonFormViewer: () =>
, +})); + +function makeTopic(overrides: Partial = {}): ComponentTopic { + return { + topic: '/test/data', + timestamp: 0, + data: null, + status: 'data', + ...overrides, + }; +} + +describe('DataPanel canWrite', () => { + it('shows write form when access is write even if data is 0 and no type is present', () => { + // Regression for the falsy-scalar bug: a counter reading of exactly 0 + // with an explicit `access: 'write'` must not hide the write section. + render(); + expect(screen.getByText('Write Value')).toBeInTheDocument(); + expect(screen.getByTestId('topic-publish-form')).toBeInTheDocument(); + }); + + it('shows write form when access is readwrite even if data is false and no type is present', () => { + render(); + expect(screen.getByText('Write Value')).toBeInTheDocument(); + }); + + it('hides write form when access is read regardless of data or type', () => { + render( + + ); + expect(screen.queryByText('Write Value')).not.toBeInTheDocument(); + expect(screen.queryByText('Publish Message')).not.toBeInTheDocument(); + }); + + it('falls back to typed-topic heuristic when access is absent', () => { + render(); + expect(screen.getByText('Publish Message')).toBeInTheDocument(); + }); + + it('hides write form when access is absent and there is no type hint and no value', () => { + render(); + expect(screen.queryByText('Publish Message')).not.toBeInTheDocument(); + expect(screen.queryByText('Write Value')).not.toBeInTheDocument(); + }); +}); diff --git a/src/components/DataPanel.tsx b/src/components/DataPanel.tsx index 7f5b4dd..0edfab9 100644 --- a/src/components/DataPanel.tsx +++ b/src/components/DataPanel.tsx @@ -203,8 +203,16 @@ export function DataPanel({ const hasData = topic.status === 'data' && topic.data !== null && topic.data !== undefined; // `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); + // read-only data item never surfaces a write form. Falsy scalar values + // (0, false, '') count as present - checking truthiness of `topic.data` + // would incorrectly hide the write form for e.g. a counter reading 0. + const hasTypeHint = !!(topic.type || topic.type_info); + const hasValuePresent = topic.data !== null && topic.data !== undefined; + const canWrite = + isConnected && + (topic.access === 'write' || + topic.access === 'readwrite' || + (topic.access !== 'read' && (hasTypeHint || hasValuePresent))); // 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. diff --git a/src/lib/transforms.test.ts b/src/lib/transforms.test.ts index 3fde215..4961b62 100644 --- a/src/lib/transforms.test.ts +++ b/src/lib/transforms.test.ts @@ -382,6 +382,22 @@ describe('transformFaultsResponse', () => { expect(result.items).toEqual([]); expect(result.count).toBe(0); }); + + it('assigns distinct synthetic codes when fault_code and code are missing', () => { + // Collisions on a literal 'unknown' code would collapse store dedup + // (keyed by code+entity_id) and produce duplicate React keys in the + // faults lists. The synthetic fallback must keep each fault distinct. + const faultWithoutCode = { description: 'drift', severity: 2, reporting_sources: [] }; + const result = transformFaultsResponse({ + items: [faultWithoutCode, faultWithoutCode, faultWithoutCode], + }); + expect(result.items).toHaveLength(3); + const codes = result.items.map((f) => f.code); + expect(new Set(codes).size).toBe(3); + for (const code of codes) { + expect(code).toMatch(/^unknown-/); + } + }); }); // ============================================================================= diff --git a/src/lib/transforms.ts b/src/lib/transforms.ts index 2e37777..bd260d2 100644 --- a/src/lib/transforms.ts +++ b/src/lib/transforms.ts @@ -99,7 +99,7 @@ export function transformFault(apiFault: RawFaultItem): Fault { // 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() || ''; + const label = typeof apiFault.severity_label === 'string' ? apiFault.severity_label.toLowerCase() : ''; if (label === 'critical' || severityNum >= 3) { severity = 'critical'; } else if (label === 'error' || severityNum === 2) { @@ -139,8 +139,19 @@ export function transformFault(apiFault: RawFaultItem): Fault { // faults are always reported by ROS 2 nodes which map to apps. const entity_type = apiFault.entity_type || 'app'; + // When both canonical `fault_code` and the `code` alias are missing, a literal + // 'unknown' collides with every other code-less fault. That collapses the + // store's dedup (store.ts: dedup by `code + entity_id`), duplicates React + // keys in the faults lists, and ties the expand-state Sets together so + // clicking one row toggles them all. A synthetic per-call ID keeps each + // code-less fault distinct through the downstream identity pipeline. + const code = + apiFault.fault_code ?? + apiFault.code ?? + `unknown-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 7)}`; + return { - code: apiFault.fault_code ?? apiFault.code ?? 'unknown', + code, message: apiFault.description ?? apiFault.fault_name ?? '', severity, status, From 520df06176efe0a1ff9082a420f68a09f08a52ed Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 19 Apr 2026 13:31:11 +0200 Subject: [PATCH 4/5] fix: stabilise synthetic fault codes and preserve falsy scalar values - transformFault: accept an optional synthetic-id suffix and use the response array index (passed from transformFaultsResponse) instead of a timestamp+random token. The previous random suffix changed on every poll, so the store's code-based dedup saw the same code-less fault as new data on every refresh, churning React keys and resetting expand state. A per-index suffix keeps code-less faults distinct inside a response while remaining stable across polls. - DataPanel: switch publishValue initialization from `||` to `??` so a gateway-reported scalar of 0 / false / '' is preserved instead of collapsing to an empty object, and replace the truthy guard in handleCopyFromLast with an explicit null/undefined presence check so "Copy to Publish" works for legitimate falsy scalar readings. - Drop the truncated license header from DataPanel.test.tsx to match the header convention of the other component test files. - Extend DataPanel and transforms tests with the corresponding regression cases (scalar-zero initial value, scalar-zero copy, stable deterministic synthetic codes across repeated transform calls). --- src/components/DataPanel.test.tsx | 68 ++++++++++++++++++++++++++----- src/components/DataPanel.tsx | 8 +++- src/lib/transforms.test.ts | 16 +++++--- src/lib/transforms.ts | 25 ++++++------ 4 files changed, 88 insertions(+), 29 deletions(-) diff --git a/src/components/DataPanel.test.tsx b/src/components/DataPanel.test.tsx index 616a1b7..5994256 100644 --- a/src/components/DataPanel.test.tsx +++ b/src/components/DataPanel.test.tsx @@ -1,13 +1,6 @@ -// 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 - -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { DataPanel } from './DataPanel'; import type { ComponentTopic } from '@/lib/types'; @@ -15,14 +8,24 @@ vi.mock('@/lib/store', () => ({ useAppStore: vi.fn((selector: (s: { isConnected: boolean }) => unknown) => selector({ isConnected: true })), })); +// Capture the latest `initialValue` so tests can assert what the form would +// receive when the user clicks "Copy to Publish". +const publishFormInitialValues: unknown[] = []; vi.mock('@/components/TopicPublishForm', () => ({ - TopicPublishForm: () =>
, + TopicPublishForm: ({ initialValue }: { initialValue: unknown }) => { + publishFormInitialValues.push(initialValue); + return
; + }, })); vi.mock('@/components/JsonFormViewer', () => ({ JsonFormViewer: () =>
, })); +beforeEach(() => { + publishFormInitialValues.length = 0; +}); + function makeTopic(overrides: Partial = {}): ComponentTopic { return { topic: '/test/data', @@ -69,3 +72,48 @@ describe('DataPanel canWrite', () => { expect(screen.queryByText('Write Value')).not.toBeInTheDocument(); }); }); + +describe('DataPanel publishValue initialization', () => { + it('preserves a scalar zero data value as the initial publish value when no default is set', () => { + // `||` would have collapsed `0` to `{}`; `??` keeps the falsy scalar. + render(); + expect(publishFormInitialValues.at(-1)).toBe(0); + }); + + it('prefers type_info.default_value over data when both are present', () => { + render( + }, + })} + entityId="motor" + /> + ); + expect(publishFormInitialValues.at(-1)).toBe(42); + }); +}); + +describe('DataPanel Copy to Publish', () => { + it('copies a scalar zero value from the last received data into the publish form', async () => { + // With the previous truthy guard, clicking Copy did nothing for a + // valid reading of exactly 0 (or false / ''). Presence check fixes it. + render( + }, + })} + entityId="motor" + /> + ); + + expect(publishFormInitialValues.at(-1)).toBe(42); + + await userEvent.click(screen.getByRole('button', { name: /copy to publish/i })); + + expect(publishFormInitialValues.at(-1)).toBe(0); + }); +}); diff --git a/src/components/DataPanel.tsx b/src/components/DataPanel.tsx index 0edfab9..5c553e4 100644 --- a/src/components/DataPanel.tsx +++ b/src/components/DataPanel.tsx @@ -197,7 +197,9 @@ export function DataPanel({ isRefreshing = false, onRefresh, }: DataPanelProps) { - const [publishValue, setPublishValue] = useState(topic.type_info?.default_value || topic.data || {}); + // Use nullish coalescing so legitimate falsy scalars (0, false, '') are + // preserved as the initial publish value instead of collapsing to `{}`. + const [publishValue, setPublishValue] = useState(topic.type_info?.default_value ?? topic.data ?? {}); const isConnected = useAppStore((state) => state.isConnected); const hasData = topic.status === 'data' && topic.data !== null && topic.data !== undefined; @@ -220,7 +222,9 @@ export function DataPanel({ topic.access === 'write' || topic.access === 'readwrite' ? 'Write Value' : 'Publish Message'; const handleCopyFromLast = () => { - if (topic.data) { + // Presence check, not truthiness, so a reported value of exactly 0 + // (or false / empty string) still copies into the publish form. + if (topic.data !== null && topic.data !== undefined) { setPublishValue(JSON.parse(JSON.stringify(topic.data))); } }; diff --git a/src/lib/transforms.test.ts b/src/lib/transforms.test.ts index 4961b62..e8560f7 100644 --- a/src/lib/transforms.test.ts +++ b/src/lib/transforms.test.ts @@ -392,11 +392,17 @@ describe('transformFaultsResponse', () => { items: [faultWithoutCode, faultWithoutCode, faultWithoutCode], }); expect(result.items).toHaveLength(3); - const codes = result.items.map((f) => f.code); - expect(new Set(codes).size).toBe(3); - for (const code of codes) { - expect(code).toMatch(/^unknown-/); - } + expect(result.items.map((f) => f.code)).toEqual(['unknown-0', 'unknown-1', 'unknown-2']); + }); + + it('returns stable synthetic codes across repeated calls so store dedup can match', () => { + // A non-deterministic synthetic code would make the same code-less + // fault look "changed" on every poll, forcing re-render and resetting + // React keys and expand state each refresh. + const faultWithoutCode = { description: 'drift', severity: 2, reporting_sources: [] }; + const first = transformFaultsResponse({ items: [faultWithoutCode, faultWithoutCode] }); + const second = transformFaultsResponse({ items: [faultWithoutCode, faultWithoutCode] }); + expect(first.items.map((f) => f.code)).toEqual(second.items.map((f) => f.code)); }); }); diff --git a/src/lib/transforms.ts b/src/lib/transforms.ts index bd260d2..239e8a7 100644 --- a/src/lib/transforms.ts +++ b/src/lib/transforms.ts @@ -94,7 +94,7 @@ export interface RawFaultItem { * - `first_occurred` (unix seconds) → `timestamp` (ISO 8601) * - `reporting_sources[0]` last path segment → `entity_id` */ -export function transformFault(apiFault: RawFaultItem): Fault { +export function transformFault(apiFault: RawFaultItem, syntheticIdSuffix?: string): Fault { // Map severity number/label to FaultSeverity. // Label check takes priority over numeric value; critical is checked first. let severity: FaultSeverity = 'info'; @@ -139,16 +139,17 @@ export function transformFault(apiFault: RawFaultItem): Fault { // faults are always reported by ROS 2 nodes which map to apps. const entity_type = apiFault.entity_type || 'app'; - // When both canonical `fault_code` and the `code` alias are missing, a literal - // 'unknown' collides with every other code-less fault. That collapses the - // store's dedup (store.ts: dedup by `code + entity_id`), duplicates React - // keys in the faults lists, and ties the expand-state Sets together so - // clicking one row toggles them all. A synthetic per-call ID keeps each - // code-less fault distinct through the downstream identity pipeline. - const code = - apiFault.fault_code ?? - apiFault.code ?? - `unknown-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 7)}`; + // When both canonical `fault_code` and the `code` alias are missing, a bare + // literal 'unknown' collides with every other code-less fault in the same + // response: that collapses the store's dedup (keyed by code + entity_id), + // duplicates React keys in the faults lists, and ties the expand-state + // Sets together so clicking one row toggles them all. The optional + // `syntheticIdSuffix` (typically the array index supplied by + // `transformFaultsResponse`) makes each code-less fault distinct within a + // response while staying stable across polls, so the store can still dedup + // on the next refresh instead of churning on every fetch. + const syntheticCode = syntheticIdSuffix !== undefined ? `unknown-${syntheticIdSuffix}` : 'unknown'; + const code = apiFault.fault_code ?? apiFault.code ?? syntheticCode; return { code, @@ -203,7 +204,7 @@ interface RawFaultsResponse { export function transformFaultsResponse(rawData: unknown): ListFaultsResponse { if (!rawData || typeof rawData !== 'object') return { items: [], count: 0 }; const data = rawData as RawFaultsResponse; - const items = (data.items || []).map((f) => transformFault(f as RawFaultItem)); + const items = (data.items || []).map((f, idx) => transformFault(f as RawFaultItem, String(idx))); return { items, count: data['x-medkit']?.count ?? items.length }; } From d2dde56a08cc4d95cc9d8b15857bc2ffd9629f5e Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 19 Apr 2026 14:18:15 +0200 Subject: [PATCH 5/5] fix: guard transformFaultsResponse against non-array items field The `(data.items || []).map(...)` fallback only catches nullish values, so any truthy non-array payload (e.g. `{ items: {} }` or `{ items: 'x' }`) would crash the transform with "map is not a function". Switch to `Array.isArray` so malformed list payloads fall through to an empty result instead of breaking the Data tab for sibling resources. Covered by a new regression test that feeds a selection of non-array truthy values into the transform and asserts an empty, crash-free result. --- src/lib/transforms.test.ts | 10 ++++++++++ src/lib/transforms.ts | 6 +++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/lib/transforms.test.ts b/src/lib/transforms.test.ts index e8560f7..9df43e5 100644 --- a/src/lib/transforms.test.ts +++ b/src/lib/transforms.test.ts @@ -383,6 +383,16 @@ describe('transformFaultsResponse', () => { expect(result.count).toBe(0); }); + it('returns empty items when items field is a non-array truthy value', () => { + // `{ items: {} }` and similar would throw on `.map` if we trusted + // truthiness alone; transformFaultsResponse must accept unknown input. + for (const bad of [{}, 'string', 42, true]) { + const result = transformFaultsResponse({ items: bad }); + expect(result.items).toEqual([]); + expect(result.count).toBe(0); + } + }); + it('assigns distinct synthetic codes when fault_code and code are missing', () => { // Collisions on a literal 'unknown' code would collapse store dedup // (keyed by code+entity_id) and produce duplicate React keys in the diff --git a/src/lib/transforms.ts b/src/lib/transforms.ts index 239e8a7..103cb6d 100644 --- a/src/lib/transforms.ts +++ b/src/lib/transforms.ts @@ -204,7 +204,11 @@ interface RawFaultsResponse { export function transformFaultsResponse(rawData: unknown): ListFaultsResponse { if (!rawData || typeof rawData !== 'object') return { items: [], count: 0 }; const data = rawData as RawFaultsResponse; - const items = (data.items || []).map((f, idx) => transformFault(f as RawFaultItem, String(idx))); + // Guard against non-array `items` (e.g. `{}` or a string) - the `|| []` + // fallback only catches nullish values, so any other truthy non-array + // would crash `.map`. + const rawItems = Array.isArray(data.items) ? data.items : []; + const items = rawItems.map((f, idx) => transformFault(f as RawFaultItem, String(idx))); return { items, count: data['x-medkit']?.count ?? items.length }; }