Skip to content

Commit 03c81e6

Browse files
mfaferek93bburda
authored andcommitted
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.
1 parent 85394d4 commit 03c81e6

8 files changed

Lines changed: 387 additions & 154 deletions

File tree

e2e/docker-compose.rosbag.yml

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,12 @@
33
# runs a manifest-only gateway with no fault manager at all, which cannot
44
# produce a single bag; the two scenarios are kept apart rather than merged so
55
# neither has to carry the other's configuration.
6+
7+
# Its own project: both compose files live in e2e/, so without this they share
8+
# the default project name "e2e" and their `gateway` services replace each other
9+
# instead of running side by side.
10+
name: e2e-rosbag
11+
612
services:
713
gateway:
814
# Overridable because the recording-id contract these specs assert on
@@ -18,15 +24,44 @@ services:
1824
- ./gateway/rosbag-params.yaml:/e2e/params.yaml:ro
1925
- ./gateway/seed_recordings.py:/e2e/seed_recordings.py:ro
2026
- e2e-bags:/e2e-bags
27+
# PID 1 reaps children and forwards signals; without it bash -lc keeps
28+
# PID 1 for itself and `docker compose down` waits out the whole grace
29+
# period before SIGKILLing a fault manager mid-write.
30+
init: true
31+
# Overriding the entrypoint skips /entrypoint.sh, which is what sources
32+
# ROS and exports the RMW default - both have to be restored here.
33+
environment:
34+
RMW_IMPLEMENTATION: ${RMW_IMPLEMENTATION:-rmw_fastrtps_cpp}
2135
entrypoint: ['/bin/bash', '-lc']
36+
# Fault manager, the seeder and the gateway in one container. Not three
37+
# services sharing a network: the default DDS transport uses /dev/shm,
38+
# which is per container, so the seeder's service calls would never
39+
# complete even though discovery says the service is there.
40+
#
41+
# Sourced ONCE in the parent shell, then every process runs as a WATCHED
42+
# background job: `&` binds looser than `&&`, so the earlier
43+
# `source && source && fault_manager & gateway` form left the gateway in
44+
# an unsourced shell ("ros2: command not found", exit 127). `wait -n`
45+
# returns when the FIRST job dies, so a fault manager that cannot open
46+
# its DB or a seeder that raises SystemExit takes the container down
47+
# with its exit code instead of leaving a healthy-looking stack whose
48+
# specs skip. The trap makes SIGTERM stop the children before the shell
49+
# exits.
2250
command:
2351
- >
2452
source /opt/ros/jazzy/setup.bash &&
2553
source /home/medkit/ws/install/setup.bash &&
2654
ros2 run ros2_medkit_fault_manager fault_manager_node
2755
--ros-args --params-file /e2e/params.yaml &
56+
FM=$!;
57+
python3 /e2e/seed_recordings.py &
58+
SEED=$!;
2859
ros2 run ros2_medkit_gateway gateway_node
29-
--ros-args --params-file /e2e/params.yaml
60+
--ros-args --params-file /e2e/params.yaml &
61+
GW=$!;
62+
trap 'kill $FM $SEED $GW 2>/dev/null' TERM INT;
63+
wait -n $FM $SEED $GW;
64+
exit $?
3065
depends_on:
3166
init-bags:
3267
condition: service_completed_successfully
@@ -39,5 +74,10 @@ services:
3974
volumes:
4075
- e2e-bags:/e2e-bags
4176
entrypoint: ['chown', '-R', '999:999', '/e2e-bags']
77+
4278
volumes:
79+
# Holds the bags AND faults.db, and it outlives `docker compose down`.
80+
# Re-seed from a clean slate with `down -v` first: on a reused volume the
81+
# fault is already CONFIRMED, the first confirm captures nothing, and the
82+
# suite sees three recordings instead of two.
4383
e2e-bags:

e2e/gateway/seed_recordings.py

Lines changed: 51 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,12 @@
3838
from std_msgs.msg import Float32
3939

4040
FAULT_CODE = 'E2E_FLAPPING_SENSOR'
41-
SOURCE_ID = '/e2e/probe_publisher'
41+
# The node's own fully qualified name. The gateway attributes a fault to the app
42+
# whose FQN matches its reporting source, so a source that belongs to no live
43+
# node leaves the fault owned by nobody and invisible under any /apps/{id} -
44+
# which is also why this node stays up afterwards instead of exiting.
45+
NODE_NAME = 'e2e_rosbag_seeder'
46+
SOURCE_ID = f'/{NODE_NAME}'
4247
PROBE_TOPIC = '/e2e/probe'
4348
# Must exceed the configured duration_sec so the ring buffer holds a full window
4449
# before each confirmation; a bag flushed from an empty buffer has no content.
@@ -47,7 +52,7 @@
4752

4853
class Seeder(Node):
4954
def __init__(self):
50-
super().__init__('e2e_rosbag_seeder')
55+
super().__init__(NODE_NAME)
5156
qos = QoSProfile(
5257
reliability=ReliabilityPolicy.BEST_EFFORT,
5358
history=HistoryPolicy.KEEP_LAST,
@@ -72,12 +77,24 @@ def publish_for(self, seconds, rate_hz=20.0):
7277
rclpy.spin_once(self, timeout_sec=0.0)
7378
time.sleep(period)
7479

75-
def call(self, client, request):
76-
future = client.call_async(request)
77-
rclpy.spin_until_future_complete(self, future, timeout_sec=20.0)
78-
if future.result() is None:
79-
raise SystemExit('service call timed out')
80-
return future.result()
80+
def call(self, client, request, attempts=5):
81+
# Retried rather than one-shot: wait_for_service returns as soon as the
82+
# service is advertised, which under DDS is before the fault manager has
83+
# finished coming up, so the very first call can time out on a server
84+
# that is seconds away from being fine.
85+
for _ in range(attempts):
86+
future = client.call_async(request)
87+
rclpy.spin_until_future_complete(self, future, timeout_sec=20.0)
88+
result = future.result()
89+
if result is not None:
90+
return result
91+
# A future that outlived its timeout must not stay in flight: the
92+
# request is not idempotent, and a late completion next to the retry
93+
# would hand the fault manager two EVENT_FAILED reports for one
94+
# occurrence.
95+
future.cancel()
96+
time.sleep(2.0)
97+
raise SystemExit('service call timed out after retries')
8198

8299
def confirm(self):
83100
request = ReportFault.Request()
@@ -86,18 +103,31 @@ def confirm(self):
86103
request.severity = Fault.SEVERITY_ERROR
87104
request.description = 'Intermittent sensor dropout seen twice'
88105
request.source_id = SOURCE_ID
89-
return self.call(self.report, request)
106+
response = self.call(self.report, request)
107+
if not response.accepted:
108+
# ReportFault's response carries no message field; accepted=False
109+
# means the request itself was invalid.
110+
raise SystemExit('ReportFault rejected the request as invalid')
111+
return response
90112

91113
def acknowledge(self):
92114
request = ClearFault.Request()
93115
request.fault_code = FAULT_CODE
94-
return self.call(self.clear, request)
116+
response = self.call(self.clear, request)
117+
# A silent "Fault not found" here would leave one bag on disk and the
118+
# whole suite skipping, with only a DEBUG log line to say why.
119+
if not response.success:
120+
raise SystemExit(f'ClearFault failed: {response.message}')
121+
return response
95122

96123

97124
def main():
98125
rclpy.init()
99126
node = Seeder()
100127
node.wait_for_services()
128+
# Let discovery settle before the first report; the gateway is coming up in
129+
# the same window and a confirmation raced against it produces no bag.
130+
time.sleep(5.0)
101131

102132
# First occurrence.
103133
node.publish_for(FILL_SECONDS)
@@ -115,8 +145,17 @@ def main():
115145
# CONFIRMED-only listing, so acknowledging this one too would leave the specs
116146
# with two bags on disk and no fault on screen pointing at them.
117147
print('SEEDED', flush=True)
118-
node.destroy_node()
119-
rclpy.shutdown()
148+
149+
# Stay on the graph. The fault is attributed to this node, so letting it
150+
# exit would take the owning app entity with it and the fault would stop
151+
# being reachable under any /apps/{id}.
152+
try:
153+
rclpy.spin(node)
154+
except KeyboardInterrupt:
155+
pass
156+
finally:
157+
node.destroy_node()
158+
rclpy.shutdown()
120159
return 0
121160

122161

e2e/rosbag-recordings.spec.ts

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -174,23 +174,25 @@ test('every recording downloads as its own bag', async ({ page }) => {
174174
}
175175
});
176176

177-
test('a recording that appears while the fault is open is reachable without a reload', async ({ page }) => {
177+
test('re-expanding the fault asks the gateway again instead of replaying a cache', async ({ page }) => {
178178
// The detail used to be fetched once per fault and cached forever, so a
179179
// recording written after the first expand stayed invisible until the
180-
// component remounted - which for a technician watching a machine fault
181-
// again is exactly the recording they are waiting for.
180+
// component remounted. This drives the collapse/re-expand path and pins
181+
// that the second expand goes back to the gateway with a 2xx; seeding a
182+
// THIRD recording mid-test would need a second seeder pass, so the
183+
// count-grows half lives in the jsdom tests that stub the store.
182184
await openTheFault(page);
183185
await expect(downloadButtons(page)).toHaveCount(expectedRecordings.length);
184186

185-
// Collapse and re-expand: the second expand must go back to the gateway
186-
// rather than replay the first response.
187-
let refetched = false;
188-
page.on('response', (response) => {
189-
if (response.url().includes(`/faults/${FAULT_CODE}`)) refetched = true;
190-
});
191-
187+
// Collapse.
192188
await page.getByText(FAULT_CODE).first().click();
189+
190+
// Re-expand, armed BEFORE the click and only satisfied by a 2xx: an error
191+
// response must not count as "refetched".
192+
const refetch = page.waitForResponse(
193+
(response) => response.url().includes(`/faults/${FAULT_CODE}`) && response.ok()
194+
);
193195
await page.getByText(FAULT_CODE).first().click();
196+
await refetch;
194197
await expect(downloadButtons(page)).toHaveCount(expectedRecordings.length);
195-
expect(refetched).toBe(true);
196198
});
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
// Copyright 2026 mfaferek93
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
/**
16+
* Two entities reporting the SAME fault code, which is legal - a code is only
17+
* unique within one entity. Everything here failed while the dashboard's caches
18+
* were keyed by code alone: expanding one row opened both, and clearing the
19+
* second row cleared the first entity's fault.
20+
*/
21+
22+
import { describe, it, expect, vi, beforeEach } from 'vitest';
23+
import { render, screen, waitFor, fireEvent } from '@testing-library/react';
24+
import { FaultsDashboard } from './FaultsDashboard';
25+
import type { Fault } from '@/lib/types';
26+
27+
const mockFetchFaults = vi.fn();
28+
const mockClearFault = vi.fn();
29+
const mockGetFaultWithEnvironmentData = vi.fn();
30+
31+
let storeState: Record<string, unknown> = {};
32+
33+
vi.mock('@/lib/store', () => ({
34+
useAppStore: Object.assign(
35+
vi.fn((selector?: (s: Record<string, unknown>) => unknown) => (selector ? selector(storeState) : storeState)),
36+
{ getState: () => storeState }
37+
),
38+
}));
39+
40+
function fault(entityId: string): Fault {
41+
return {
42+
code: 'LIDAR_RANGE_INVALID',
43+
message: `range invalid on ${entityId}`,
44+
severity: 'error',
45+
status: 'active',
46+
timestamp: '2026-08-20T10:00:00Z',
47+
entity_id: entityId,
48+
entity_type: 'app',
49+
};
50+
}
51+
52+
beforeEach(() => {
53+
vi.clearAllMocks();
54+
mockGetFaultWithEnvironmentData.mockResolvedValue({ environment_data: { snapshots: [] } });
55+
storeState = {
56+
faults: [fault('app_a'), fault('app_b')],
57+
isLoadingFaults: false,
58+
faultsError: null,
59+
fetchFaults: mockFetchFaults,
60+
clearFault: mockClearFault,
61+
getFaultWithEnvironmentData: mockGetFaultWithEnvironmentData,
62+
isConnected: true,
63+
};
64+
});
65+
66+
describe('FaultsDashboard with colliding fault codes', () => {
67+
it('expands only the clicked row and fetches only its entity', async () => {
68+
render(<FaultsDashboard />);
69+
// Flat list view: the grouped default splits by entity, which would
70+
// hide the collision the caches must survive.
71+
fireEvent.click(screen.getByRole('switch', { name: /group by entity/i }));
72+
73+
const rows = screen.getAllByText('LIDAR_RANGE_INVALID');
74+
expect(rows).toHaveLength(2);
75+
fireEvent.click(rows[0]!);
76+
77+
await waitFor(() => expect(mockGetFaultWithEnvironmentData).toHaveBeenCalledTimes(1));
78+
expect(mockGetFaultWithEnvironmentData).toHaveBeenCalledWith('apps', 'app_a', 'LIDAR_RANGE_INVALID');
79+
// The sibling with the same code stays collapsed: exactly one row shows
80+
// the expanded empty-environment marker.
81+
await waitFor(() => expect(screen.getAllByText(/no environment data available/i)).toHaveLength(1));
82+
});
83+
84+
it("clears the clicked row's entity, not the first entity with that code", async () => {
85+
render(<FaultsDashboard />);
86+
fireEvent.click(screen.getByRole('switch', { name: /group by entity/i }));
87+
88+
const clearButtons = screen.getAllByTitle('Clear fault');
89+
expect(clearButtons).toHaveLength(2);
90+
fireEvent.click(clearButtons[1]!);
91+
92+
await waitFor(() => expect(mockClearFault).toHaveBeenCalledTimes(1));
93+
expect(mockClearFault).toHaveBeenCalledWith('apps', 'app_b', 'LIDAR_RANGE_INVALID');
94+
});
95+
96+
it('keeps evidence on screen when a refetch answers 404 (null)', async () => {
97+
mockGetFaultWithEnvironmentData
98+
.mockResolvedValueOnce({
99+
environment_data: { snapshots: [{ type: 'freeze_frame', name: 'ff', data: { level: 82 } }] },
100+
})
101+
.mockResolvedValueOnce(null);
102+
render(<FaultsDashboard />);
103+
fireEvent.click(screen.getByRole('switch', { name: /group by entity/i }));
104+
105+
const row = screen.getAllByText('LIDAR_RANGE_INVALID')[0]!;
106+
fireEvent.click(row);
107+
await waitFor(() => expect(screen.getByText(/snapshots \(1\)/i)).toBeInTheDocument());
108+
109+
// Collapse, re-expand: the second fetch resolves null (the store's
110+
// documented 404 shape). The cached evidence must survive it.
111+
fireEvent.click(row);
112+
fireEvent.click(row);
113+
await waitFor(() => expect(mockGetFaultWithEnvironmentData).toHaveBeenCalledTimes(2));
114+
expect(screen.getByText(/snapshots \(1\)/i)).toBeInTheDocument();
115+
});
116+
});

0 commit comments

Comments
 (0)