Skip to content

Commit 0c858a5

Browse files
committed
fix: read the fault list when a view opens and when the stream stops
Sharing one refresh went too far: only the first view read on mount, and the sidebar badge is mounted for the whole session, so opening the dashboard showed the list as it stood when the session began. With the fault stream up there is no timer either, so nothing corrected it. Every view now reads when it opens, and again when the connection is replaced or the stream appears or dies - the requests that coincide still collapse into one. Connecting to another gateway also drops the previous one's faults instead of leaving rows whose clear button would address the new gateway. Two ways the list could stop refreshing entirely are gone. A request that never came back held the shared refresh for the life of the tab, so every later refresh waited behind it and the page kept claiming no faults; it now times out, and leaving the session releases it. A fault stream that ended without an error left the client believing it was still delivering, and the timer stayed off for good; the stream ending now hands refreshing back to polling however it ended. Clearing a fault re-read the list twice, once in the store and once in the dashboard.
1 parent 89fc7d1 commit 0c858a5

7 files changed

Lines changed: 233 additions & 8 deletions

File tree

e2e/faults-refresh.spec.ts

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,15 @@ async function watchForSkeleton(page: Page): Promise<() => Promise<boolean>> {
4545
});
4646
}
4747

48+
/**
49+
* The dashboard reads the gateway-wide fault list. Entity pages read their own
50+
* `/apps/<id>/faults`, which ends the same way, so the match has to be exact or a
51+
* detour through an entity inflates the count.
52+
*/
53+
function isFaultListRequest(url: string): boolean {
54+
return new URL(url).pathname === '/api/v1/faults';
55+
}
56+
4857
/** Opens the dashboard and waits for the gateway's (empty) fault list to be on screen. */
4958
async function openDashboard(page: Page): Promise<void> {
5059
await page.goto('/');
@@ -69,6 +78,26 @@ test.describe('faults dashboard refresh', () => {
6978
await expect(page.getByText('No faults to display')).toBeVisible();
7079
});
7180

81+
test('opening the dashboard reads the fault list', async ({ page }) => {
82+
await openDashboard(page);
83+
84+
const faultRequests: string[] = [];
85+
page.on('request', (request) => {
86+
if (isFaultListRequest(request.url())) {
87+
faultRequests.push(request.url());
88+
}
89+
});
90+
91+
// Leave the dashboard for an entity and come back. The sidebar badge stays
92+
// mounted the whole time, so the dashboard is never the session's first fault view.
93+
await page.getByText('Test ECU').first().click();
94+
await expect(page.getByText('No faults to display')).toBeHidden();
95+
await page.getByRole('button', { name: 'Faults Dashboard' }).click();
96+
await expect(page.getByText('No faults to display')).toBeVisible();
97+
98+
expect(faultRequests.length).toBeGreaterThan(0);
99+
});
100+
72101
test('the fallback poll asks once per interval, not once per mounted view', async ({ page }) => {
73102
// With no fault stream the app falls back to polling. The dashboard and the
74103
// sidebar badge are both on screen and read the same list. An HTTP status is
@@ -80,7 +109,7 @@ test.describe('faults dashboard refresh', () => {
80109

81110
const faultRequests: string[] = [];
82111
page.on('request', (request) => {
83-
if (new URL(request.url()).pathname.endsWith('/faults')) {
112+
if (isFaultListRequest(request.url())) {
84113
faultRequests.push(request.url());
85114
}
86115
});

src/components/FaultsDashboard.polling.test.tsx

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,78 @@ describe('FaultsDashboard refresh behaviour', () => {
185185
expect(client.GET.mock.calls.length).toBe(afterMount);
186186
});
187187

188+
it('reads the list when a second view opens on top of one already mounted', async () => {
189+
const client = clientReturning([RAW_FAULT]);
190+
connect(client, true);
191+
await mount(<FaultsCountBadge />);
192+
expect(client.GET).toHaveBeenCalledTimes(1);
193+
194+
// The badge lives in the sidebar for the whole session, so the dashboard is
195+
// always the second view. Opening it has to show the list as it is now.
196+
await mount(<FaultsDashboard />);
197+
198+
expect(client.GET).toHaveBeenCalledTimes(2);
199+
});
200+
201+
it('reads the list as soon as the fault stream stops delivering', async () => {
202+
const client = clientReturning([RAW_FAULT]);
203+
connect(client, true);
204+
await mount(
205+
<>
206+
<FaultsCountBadge />
207+
<FaultsDashboard />
208+
</>
209+
);
210+
const whileStreaming = client.GET.mock.calls.length;
211+
212+
// The stream dying is the one moment the client knows it has missed events.
213+
await act(async () => {
214+
useAppStore.setState({ faultStreamCleanup: null } as never);
215+
await vi.advanceTimersByTimeAsync(0);
216+
});
217+
218+
expect(client.GET.mock.calls.length).toBe(whileStreaming + 1);
219+
});
220+
221+
it('reads from the gateway it is now connected to', async () => {
222+
const gatewayA = clientReturning([RAW_FAULT]);
223+
connect(gatewayA, true);
224+
await mount(
225+
<>
226+
<FaultsCountBadge />
227+
<FaultsDashboard />
228+
</>
229+
);
230+
231+
// Connecting elsewhere never clears isConnected, so nothing else re-reads.
232+
const gatewayB = clientReturning([]);
233+
await act(async () => {
234+
useAppStore.setState({ client: gatewayB } as never);
235+
await vi.advanceTimersByTimeAsync(0);
236+
});
237+
238+
expect(gatewayB.GET).toHaveBeenCalledTimes(1);
239+
});
240+
241+
it('re-reads the list once when a fault is cleared, not twice', async () => {
242+
const client = {
243+
...clientReturning([RAW_FAULT]),
244+
DELETE: vi.fn(async () => ({ data: undefined, error: undefined })),
245+
};
246+
connect(client, true);
247+
const { container } = await mount(<FaultsDashboard />);
248+
const readsBeforeClear = client.GET.mock.calls.length;
249+
250+
const clearButton = container.querySelector('button[title="Clear fault"]') as HTMLElement;
251+
await act(async () => {
252+
clearButton.click();
253+
await vi.advanceTimersByTimeAsync(0);
254+
});
255+
256+
expect(client.DELETE).toHaveBeenCalledTimes(1);
257+
expect(client.GET.mock.calls.length).toBe(readsBeforeClear + 1);
258+
});
259+
188260
it('stops polling when the last fault view unmounts', async () => {
189261
const client = clientReturning([RAW_FAULT]);
190262
connect(client, false);

src/components/FaultsDashboard.tsx

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -441,9 +441,8 @@ export function FaultsDashboard() {
441441
// Map the fault's entity_type to the correct resource type for the API
442442
const entityGroup = mapFaultEntityTypeToResourceType(fault.entity_type);
443443
// Use store's clearFault which has proper error handling with toasts
444+
// clearFault refreshes the list itself once the delete succeeds.
444445
await clearFault(entityGroup, fault.entity_id, fault.code);
445-
// Reload faults after clearing
446-
await fetchFaults();
447446
} finally {
448447
setClearingCodes((prev) => {
449448
const next = new Set(prev);
@@ -452,7 +451,7 @@ export function FaultsDashboard() {
452451
});
453452
}
454453
},
455-
[fetchFaults, clearFault]
454+
[clearFault]
456455
);
457456

458457
// Toggle fault expansion and lazy-load environment data

src/hooks/useFaultPolling.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,22 +68,29 @@ export interface UseFaultPollingOptions {
6868
*/
6969
export function useFaultPolling(options: UseFaultPollingOptions = {}): void {
7070
const poll = options.poll ?? true;
71-
const { isConnected, hasFaultStream } = useAppStore(
71+
const { isConnected, client, hasFaultStream } = useAppStore(
7272
useShallow((state) => ({
7373
isConnected: state.isConnected,
74+
client: state.client,
7475
hasFaultStream: state.faultStreamCleanup !== null,
7576
}))
7677
);
7778

7879
useEffect(() => {
7980
if (!isConnected) return;
8081

82+
// Every view reads when it opens, not only the first one: the sidebar badge is
83+
// mounted for the whole session, so the dashboard is always a later subscriber
84+
// and would otherwise show the list as it stood when the session began. Views
85+
// opening together still cost one request - the store reuses the one in flight.
86+
// Re-runs when the connection is replaced (a new gateway has its own faults) and
87+
// when the fault stream appears or dies, which is when events may have been missed.
8188
subscribers += 1;
8289
if (subscribers === 1) {
8390
visibilityListener = refreshIfVisible;
8491
document.addEventListener('visibilitychange', visibilityListener);
85-
refreshFaults();
8692
}
93+
refreshFaults();
8794

8895
return () => {
8996
subscribers -= 1;
@@ -92,7 +99,7 @@ export function useFaultPolling(options: UseFaultPollingOptions = {}): void {
9299
visibilityListener = null;
93100
}
94101
};
95-
}, [isConnected]);
102+
}, [isConnected, client, hasFaultStream]);
96103

97104
useEffect(() => {
98105
if (!isConnected || !poll) return;

src/lib/store-connect.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,4 +97,33 @@ describe('connect', () => {
9797
await Promise.resolve();
9898
expect(useAppStore.getState().scriptsSupported).toBe(true);
9999
});
100+
101+
it('leaves no faults from the previous gateway on screen', async () => {
102+
const mockGet = vi.fn((path: string) => {
103+
if (path === '/health') return Promise.resolve({ error: undefined });
104+
return Promise.resolve({ data: undefined });
105+
});
106+
vi.mocked(createMedkitClient).mockReturnValue({ GET: mockGet } as unknown as MedkitClient);
107+
useAppStore.setState({
108+
loadRootEntities: vi.fn().mockResolvedValue(undefined),
109+
subscribeFaultStream: vi.fn(),
110+
faults: [
111+
{
112+
code: 'PREVIOUS_GATEWAY_FAULT',
113+
message: 'raised on the robot we just left',
114+
severity: 'error',
115+
status: 'active',
116+
timestamp: '2026-08-31T10:00:00.000Z',
117+
entity_id: 'lidar_front',
118+
entity_type: 'app',
119+
},
120+
],
121+
faultsLoaded: true,
122+
});
123+
124+
await useAppStore.getState().connect('http://other-gateway.local');
125+
126+
expect(useAppStore.getState().faults).toEqual([]);
127+
expect(useAppStore.getState().faultsLoaded).toBe(false);
128+
});
100129
});

src/lib/store-faults.test.ts

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
*/
2020

2121
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
22-
import { useAppStore } from './store';
22+
import { useAppStore, FAULTS_REQUEST_TIMEOUT_MS } from './store';
2323
import type { Fault } from './types';
2424

2525
vi.mock('react-toastify', () => ({
@@ -83,6 +83,26 @@ afterEach(() => {
8383
useAppStore.setState({ isConnected: false, client: null, faults: [] } as never);
8484
});
8585

86+
describe('subscribeFaultStream', () => {
87+
it('hands refreshing back to polling when the stream closes without an error', async () => {
88+
const streamClient = {
89+
GET: vi.fn(async () => ({ data: { items: [] }, error: undefined })),
90+
streams: {
91+
faults: () => ({
92+
close: vi.fn(),
93+
[Symbol.asyncIterator]: () => ({ next: async () => ({ done: true, value: undefined }) }),
94+
}),
95+
},
96+
};
97+
connected(streamClient);
98+
99+
useAppStore.getState().subscribeFaultStream();
100+
expect(useAppStore.getState().faultStreamCleanup).not.toBeNull();
101+
102+
await vi.waitFor(() => expect(useAppStore.getState().faultStreamCleanup).toBeNull());
103+
});
104+
});
105+
86106
describe('fetchFaults change detection', () => {
87107
it('takes up a fault that is now reported by a different entity', async () => {
88108
connected(clientReturning([raw()]));
@@ -153,6 +173,48 @@ describe('fetchFaults against a moving connection', () => {
153173
expect(client.GET).toHaveBeenCalledTimes(1);
154174
});
155175

176+
it('is not wedged by a request that never answers', async () => {
177+
const hung = { GET: vi.fn(() => new Promise(() => {})) };
178+
connected(hung);
179+
void useAppStore.getState().fetchFaults();
180+
181+
useAppStore.getState().disconnect();
182+
const healthy = clientReturning([raw()]);
183+
connected(healthy);
184+
await useAppStore.getState().fetchFaults();
185+
186+
expect(healthy.GET).toHaveBeenCalledTimes(1);
187+
expect(useAppStore.getState().faults).toHaveLength(1);
188+
});
189+
190+
it('gives up on a request the gateway never answers', async () => {
191+
// The abort is the behaviour under test, so its own log line is not a surprise.
192+
const logged = vi.spyOn(console, 'error').mockImplementation(() => {});
193+
vi.useFakeTimers();
194+
try {
195+
const hung = {
196+
GET: vi.fn((_path: string, init: { signal?: AbortSignal }) => {
197+
return new Promise((_resolve, reject) => {
198+
init.signal?.addEventListener('abort', () => reject(new Error('aborted')));
199+
});
200+
}),
201+
};
202+
connected(hung);
203+
const first = useAppStore.getState().fetchFaults();
204+
await vi.advanceTimersByTimeAsync(FAULTS_REQUEST_TIMEOUT_MS + 100);
205+
await first;
206+
207+
const healthy = clientReturning([raw()]);
208+
useAppStore.setState({ client: healthy } as never);
209+
await useAppStore.getState().fetchFaults();
210+
211+
expect(healthy.GET).toHaveBeenCalledTimes(1);
212+
} finally {
213+
vi.useRealTimers();
214+
logged.mockRestore();
215+
}
216+
});
217+
156218
it('lets a forced refresh through so a cleared fault is not read back from an older answer', async () => {
157219
const { client, release } = deferredClient();
158220
connected(client);

src/lib/store.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -892,6 +892,12 @@ function faultRowKey(fault: Fault): string {
892892
return `${faultKey(fault)}:${fault.status}:${fault.severity}:${fault.message}:${fault.timestamp}`;
893893
}
894894

895+
/**
896+
* How long a fault refresh may stay unanswered. A gateway that stops answering must
897+
* not hold the shared refresh open for good - every later refresh waits behind it.
898+
*/
899+
export const FAULTS_REQUEST_TIMEOUT_MS = 15000;
900+
895901
/**
896902
* The refresh currently on the wire, if any. Every fault view shares one list and
897903
* they refresh on the same events (a timer tick, a tab regaining focus), so without
@@ -1140,6 +1146,12 @@ export const useAppStore = create<AppState>()(
11401146
isConnecting: false,
11411147
connectionError: null,
11421148
client,
1149+
// Connecting elsewhere never goes through disconnect(), so without
1150+
// this the previous gateway's faults stay on screen - and the clear
1151+
// button on such a row would send its code to the new gateway.
1152+
faults: [],
1153+
isLoadingFaults: false,
1154+
faultsLoaded: false,
11431155
});
11441156

11451157
// A reconnect must not carry executions of the previous gateway:
@@ -1208,6 +1220,9 @@ export const useAppStore = create<AppState>()(
12081220

12091221
// Unsubscribe from fault stream
12101222
get().unsubscribeFaultStream();
1223+
// Whatever is on the wire belongs to the session being left, and a refresh
1224+
// for the next one must not be answered by it or queue behind it.
1225+
faultsRefreshInFlight = null;
12111226

12121227
set({
12131228
serverUrl: null,
@@ -2540,13 +2555,16 @@ export const useAppStore = create<AppState>()(
25402555
}
25412556

25422557
const run = async () => {
2558+
const controller = new AbortController();
2559+
const timeoutId = setTimeout(() => controller.abort(), FAULTS_REQUEST_TIMEOUT_MS);
25432560
try {
25442561
const { data: faultsData, error: faultsError } = await client.GET('/faults', {
25452562
params: {
25462563
// `status=all` is required to include cleared/healed faults
25472564
// (no param returns only active).
25482565
query: { status: 'all' },
25492566
},
2567+
signal: controller.signal,
25502568
});
25512569
// A refresh outlives the session that started it, so an answer from
25522570
// the gateway we just left must not populate the next one's list.
@@ -2578,6 +2596,8 @@ export const useAppStore = create<AppState>()(
25782596
// A failed attempt still ends the initial load: the error is reported
25792597
// through the toast, and the skeleton must not return on every retry.
25802598
set({ isLoadingFaults: false, faultsLoaded: true });
2599+
} finally {
2600+
clearTimeout(timeoutId);
25812601
}
25822602
};
25832603

@@ -2692,6 +2712,13 @@ export const useAppStore = create<AppState>()(
26922712
toast.warning(`Fault: ${fault.message}`, { autoClose: 5000 });
26932713
}
26942714
}
2715+
// The stream can also end without an error - a gateway closing the
2716+
// response cleanly looks like this. Updates have stopped either way,
2717+
// so refreshing has to go back to polling.
2718+
if (running) {
2719+
cleanup();
2720+
set({ faultStreamCleanup: null });
2721+
}
26952722
} catch (error) {
26962723
console.error('[store] subscribeFaultStream: error in consume loop', error);
26972724
if (running) {

0 commit comments

Comments
 (0)