Skip to content

Commit 0843e07

Browse files
bburdabburda
committed
fix: abort in-flight entity fetches and dedupe peer /apps requests
Addresses review feedback on PR #62: 1. EntityResourceTabs: propagate an AbortSignal to all resource fetches (data/operations/configurations/faults) and abort them whenever the entity changes or the component unmounts. Prevents stale responses from overwriting a newer entity's data on slow connections. Extend fetchEntityData, fetchEntityOperations, fetchConfigurations, listEntityFaults, and the underlying api-dispatch helpers to accept an optional signal parameter that threads through to openapi-fetch. 2. Deduplicate concurrent GET /apps requests when multiple peer-sourced components are expanded in quick succession. Extract the fallback into fetchAllAppsDeduped, which holds a module-level in-flight promise and shares it across concurrent callers. The promise is cleared on settlement so later expansions fetch fresh data. Add tests for both behaviors (stale-fetch discard + concurrent-request dedupe + dedupe-cache reset + fetch-failure fallback).
1 parent cfa8a54 commit 0843e07

5 files changed

Lines changed: 251 additions & 38 deletions

File tree

‎src/components/EntityResourceTabs.test.tsx‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ describe('EntityResourceTabs', () => {
6969
render(<EntityResourceTabs entityId="ecu-primary" entityType="components" />);
7070

7171
await waitFor(() => {
72-
expect(mockFetchEntityData).toHaveBeenCalledWith('components', 'ecu-primary');
72+
expect(mockFetchEntityData).toHaveBeenCalledWith('components', 'ecu-primary', expect.anything());
7373
});
7474

7575
await waitFor(() => {
@@ -104,7 +104,7 @@ describe('EntityResourceTabs', () => {
104104
rerender(<EntityResourceTabs entityId="ecu-mcu" entityType="components" />);
105105

106106
await waitFor(() => {
107-
expect(mockFetchEntityData).toHaveBeenCalledWith('components', 'ecu-mcu');
107+
expect(mockFetchEntityData).toHaveBeenCalledWith('components', 'ecu-mcu', expect.anything());
108108
});
109109

110110
await waitFor(() => {
@@ -114,4 +114,44 @@ describe('EntityResourceTabs', () => {
114114
// Old data should be gone
115115
expect(screen.queryByText('/engine/temperature')).not.toBeInTheDocument();
116116
});
117+
118+
it('does not apply stale fetch result when entity changes mid-flight', async () => {
119+
// First fetch returns a promise we control, so we can switch entities
120+
// while it is still in-flight and verify the old result is discarded.
121+
let resolveFirst: (value: ComponentTopic[]) => void = () => {};
122+
const firstPromise = new Promise<ComponentTopic[]>((resolve) => {
123+
resolveFirst = resolve;
124+
});
125+
mockFetchEntityData.mockReturnValueOnce(firstPromise);
126+
127+
const { rerender } = render(<EntityResourceTabs entityId="ecu-primary" entityType="components" />);
128+
129+
// Switch entity while the first fetch is still pending
130+
const secondTopics: ComponentTopic[] = [
131+
{
132+
topic: '/brake/pressure',
133+
timestamp: Date.now(),
134+
data: null,
135+
status: 'metadata_only',
136+
type: 'sensor_msgs/msg/FluidPressure',
137+
},
138+
];
139+
mockFetchEntityData.mockResolvedValueOnce(secondTopics);
140+
rerender(<EntityResourceTabs entityId="ecu-mcu" entityType="components" />);
141+
142+
// New entity's fetch resolves and renders
143+
await waitFor(() => {
144+
expect(screen.getByText('/brake/pressure')).toBeInTheDocument();
145+
});
146+
147+
// Late-resolve the first (aborted) fetch - it must NOT overwrite the
148+
// current entity's data.
149+
resolveFirst(sampleTopics());
150+
// Give the microtask queue a chance to run the (hopefully discarded) setData.
151+
await Promise.resolve();
152+
await Promise.resolve();
153+
154+
expect(screen.queryByText('/engine/temperature')).not.toBeInTheDocument();
155+
expect(screen.getByText('/brake/pressure')).toBeInTheDocument();
156+
});
117157
});

‎src/components/EntityResourceTabs.tsx‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,34 +65,47 @@ export function EntityResourceTabs({ entityId, entityType, basePath, onNavigate
6565
}))
6666
);
6767

68+
// AbortController for in-flight fetches. Aborted (and replaced) whenever
69+
// the entity changes so stale responses never overwrite fresh ones.
70+
const abortRef = useRef<AbortController | null>(null);
71+
6872
// Lazy load resources for the active tab
6973
const loadTabResources = useCallback(
7074
async (tab: ResourceTabId) => {
7175
if (loadedTabsRef.current[tab]) return;
7276

77+
const signal = abortRef.current?.signal;
7378
setIsLoading(true);
7479
try {
7580
switch (tab) {
7681
case 'data': {
77-
const dataRes = await fetchEntityData(entityType, entityId).catch(() => [] as ComponentTopic[]);
82+
const dataRes = await fetchEntityData(entityType, entityId, signal).catch(
83+
() => [] as ComponentTopic[]
84+
);
85+
if (signal?.aborted) return;
7886
setData(dataRes);
7987
break;
8088
}
8189
case 'operations': {
82-
const opsRes = await fetchEntityOperations(entityType, entityId).catch(() => [] as Operation[]);
90+
const opsRes = await fetchEntityOperations(entityType, entityId, signal).catch(
91+
() => [] as Operation[]
92+
);
93+
if (signal?.aborted) return;
8394
setOperations(opsRes);
8495
break;
8596
}
8697
case 'configurations': {
87-
await fetchConfigurations(entityId, entityType);
98+
await fetchConfigurations(entityId, entityType, signal);
99+
if (signal?.aborted) return;
88100
// Configurations are stored in the store's configurations map
89101
break;
90102
}
91103
case 'faults': {
92-
const faultsRes = await listEntityFaults(entityType, entityId).catch(() => ({
104+
const faultsRes = await listEntityFaults(entityType, entityId, signal).catch(() => ({
93105
items: [] as Fault[],
94106
count: 0,
95107
}));
108+
if (signal?.aborted) return;
96109
setFaults(faultsRes.items || []);
97110
break;
98111
}
@@ -101,11 +114,13 @@ export function EntityResourceTabs({ entityId, entityType, basePath, onNavigate
101114
break;
102115
}
103116
}
117+
if (signal?.aborted) return;
104118
setLoadedTabs((prev) => ({ ...prev, [tab]: true }));
105119
} catch (error) {
120+
if (signal?.aborted) return;
106121
console.error(`Failed to load ${tab} resources:`, error);
107122
} finally {
108-
setIsLoading(false);
123+
if (!signal?.aborted) setIsLoading(false);
109124
}
110125
},
111126

@@ -115,6 +130,11 @@ export function EntityResourceTabs({ entityId, entityType, basePath, onNavigate
115130
// Reset tab state when the entity changes so stale data from the
116131
// previous entity does not leak into the new one.
117132
useEffect(() => {
133+
// Abort any fetches still running for the previous entity. The
134+
// next load effect will spin up a fresh controller.
135+
abortRef.current?.abort();
136+
abortRef.current = new AbortController();
137+
118138
const reset: LoadedResources = {
119139
data: false,
120140
operations: false,
@@ -133,6 +153,11 @@ export function EntityResourceTabs({ entityId, entityType, basePath, onNavigate
133153
setFaults([]);
134154
}, [entityId, entityType]);
135155

156+
// Abort in-flight fetches on unmount.
157+
useEffect(() => {
158+
return () => abortRef.current?.abort();
159+
}, []);
160+
136161
// Load resources when tab changes
137162
useEffect(() => {
138163
loadTabResources(activeTab);

‎src/lib/api-dispatch.ts‎

Lines changed: 52 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -46,19 +46,29 @@ export function getEntityDetail(client: MedkitClient, entityType: SovdResourceEn
4646
// Configurations
4747
// =============================================================================
4848

49-
export function getEntityConfigurations(client: MedkitClient, entityType: SovdResourceEntityType, entityId: string) {
49+
export function getEntityConfigurations(
50+
client: MedkitClient,
51+
entityType: SovdResourceEntityType,
52+
entityId: string,
53+
signal?: AbortSignal
54+
) {
5055
switch (entityType) {
5156
case 'apps':
52-
return client.GET('/apps/{app_id}/configurations', { params: { path: { app_id: entityId } } });
57+
return client.GET('/apps/{app_id}/configurations', { params: { path: { app_id: entityId } }, signal });
5358
case 'components':
5459
return client.GET('/components/{component_id}/configurations', {
5560
params: { path: { component_id: entityId } },
61+
signal,
5662
});
5763
case 'areas':
58-
return client.GET('/areas/{area_id}/configurations', { params: { path: { area_id: entityId } } });
64+
return client.GET('/areas/{area_id}/configurations', {
65+
params: { path: { area_id: entityId } },
66+
signal,
67+
});
5968
case 'functions':
6069
return client.GET('/functions/{function_id}/configurations', {
6170
params: { path: { function_id: entityId } },
71+
signal,
6272
});
6373
}
6474
}
@@ -167,16 +177,27 @@ export function deleteEntityConfigurations(client: MedkitClient, entityType: Sov
167177
// Data
168178
// =============================================================================
169179

170-
export function getEntityData(client: MedkitClient, entityType: SovdResourceEntityType, entityId: string) {
180+
export function getEntityData(
181+
client: MedkitClient,
182+
entityType: SovdResourceEntityType,
183+
entityId: string,
184+
signal?: AbortSignal
185+
) {
171186
switch (entityType) {
172187
case 'apps':
173-
return client.GET('/apps/{app_id}/data', { params: { path: { app_id: entityId } } });
188+
return client.GET('/apps/{app_id}/data', { params: { path: { app_id: entityId } }, signal });
174189
case 'components':
175-
return client.GET('/components/{component_id}/data', { params: { path: { component_id: entityId } } });
190+
return client.GET('/components/{component_id}/data', {
191+
params: { path: { component_id: entityId } },
192+
signal,
193+
});
176194
case 'areas':
177-
return client.GET('/areas/{area_id}/data', { params: { path: { area_id: entityId } } });
195+
return client.GET('/areas/{area_id}/data', { params: { path: { area_id: entityId } }, signal });
178196
case 'functions':
179-
return client.GET('/functions/{function_id}/data', { params: { path: { function_id: entityId } } });
197+
return client.GET('/functions/{function_id}/data', {
198+
params: { path: { function_id: entityId } },
199+
signal,
200+
});
180201
}
181202
}
182203

@@ -241,19 +262,29 @@ export function putEntityDataItem(
241262
// Operations
242263
// =============================================================================
243264

244-
export function getEntityOperations(client: MedkitClient, entityType: SovdResourceEntityType, entityId: string) {
265+
export function getEntityOperations(
266+
client: MedkitClient,
267+
entityType: SovdResourceEntityType,
268+
entityId: string,
269+
signal?: AbortSignal
270+
) {
245271
switch (entityType) {
246272
case 'apps':
247-
return client.GET('/apps/{app_id}/operations', { params: { path: { app_id: entityId } } });
273+
return client.GET('/apps/{app_id}/operations', { params: { path: { app_id: entityId } }, signal });
248274
case 'components':
249275
return client.GET('/components/{component_id}/operations', {
250276
params: { path: { component_id: entityId } },
277+
signal,
251278
});
252279
case 'areas':
253-
return client.GET('/areas/{area_id}/operations', { params: { path: { area_id: entityId } } });
280+
return client.GET('/areas/{area_id}/operations', {
281+
params: { path: { area_id: entityId } },
282+
signal,
283+
});
254284
case 'functions':
255285
return client.GET('/functions/{function_id}/operations', {
256286
params: { path: { function_id: entityId } },
287+
signal,
257288
});
258289
}
259290
}
@@ -359,19 +390,26 @@ export function deleteEntityExecution(
359390
// Faults
360391
// =============================================================================
361392

362-
export function getEntityFaults(client: MedkitClient, entityType: SovdResourceEntityType, entityId: string) {
393+
export function getEntityFaults(
394+
client: MedkitClient,
395+
entityType: SovdResourceEntityType,
396+
entityId: string,
397+
signal?: AbortSignal
398+
) {
363399
switch (entityType) {
364400
case 'apps':
365-
return client.GET('/apps/{app_id}/faults', { params: { path: { app_id: entityId } } });
401+
return client.GET('/apps/{app_id}/faults', { params: { path: { app_id: entityId } }, signal });
366402
case 'components':
367403
return client.GET('/components/{component_id}/faults', {
368404
params: { path: { component_id: entityId } },
405+
signal,
369406
});
370407
case 'areas':
371-
return client.GET('/areas/{area_id}/faults', { params: { path: { area_id: entityId } } });
408+
return client.GET('/areas/{area_id}/faults', { params: { path: { area_id: entityId } }, signal });
372409
case 'functions':
373410
return client.GET('/functions/{function_id}/faults', {
374411
params: { path: { function_id: entityId } },
412+
signal,
375413
});
376414
}
377415
}

‎src/lib/store-helpers.test.ts‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
import { describe, it, expect } from 'vitest';
15+
import { describe, it, expect, vi, beforeEach } from 'vitest';
1616
import {
1717
toTreeNode,
1818
updateNodeInTree,
@@ -21,8 +21,11 @@ import {
2121
parseTreePath,
2222
filterAppsByComponent,
2323
isPeerSourcedComponent,
24+
fetchAllAppsDeduped,
25+
__resetAppsRequestCache,
2426
} from './store';
2527
import type { SovdEntity, EntityTreeNode } from './types';
28+
import type { MedkitClient } from '@selfpatch/ros2-medkit-client-ts';
2629

2730
// =============================================================================
2831
// Helper factories
@@ -465,6 +468,59 @@ describe('filterAppsByComponent', () => {
465468
});
466469
});
467470

471+
// =============================================================================
472+
// fetchAllAppsDeduped
473+
// =============================================================================
474+
475+
describe('fetchAllAppsDeduped', () => {
476+
beforeEach(() => {
477+
__resetAppsRequestCache();
478+
});
479+
480+
it('shares one in-flight request across concurrent calls', async () => {
481+
let resolveGet: (value: { data: { items: Record<string, unknown>[] } }) => void = () => {};
482+
const getSpy = vi.fn(
483+
() =>
484+
new Promise<{ data: { items: Record<string, unknown>[] } }>((resolve) => {
485+
resolveGet = resolve;
486+
})
487+
);
488+
const client = { GET: getSpy } as unknown as MedkitClient;
489+
490+
// Fire three concurrent callers
491+
const p1 = fetchAllAppsDeduped(client);
492+
const p2 = fetchAllAppsDeduped(client);
493+
const p3 = fetchAllAppsDeduped(client);
494+
495+
// Exactly one underlying HTTP call
496+
expect(getSpy).toHaveBeenCalledTimes(1);
497+
498+
// Resolve and verify all three callers receive the same data
499+
const apps = [{ id: 'one' }, { id: 'two' }];
500+
resolveGet({ data: { items: apps } });
501+
await expect(p1).resolves.toEqual(apps);
502+
await expect(p2).resolves.toEqual(apps);
503+
await expect(p3).resolves.toEqual(apps);
504+
});
505+
506+
it('clears cache after settle so the next caller refetches', async () => {
507+
const getSpy = vi.fn().mockResolvedValue({ data: { items: [{ id: 'a' }] } });
508+
const client = { GET: getSpy } as unknown as MedkitClient;
509+
510+
await fetchAllAppsDeduped(client);
511+
await fetchAllAppsDeduped(client);
512+
513+
expect(getSpy).toHaveBeenCalledTimes(2);
514+
});
515+
516+
it('returns empty array on fetch failure', async () => {
517+
const getSpy = vi.fn().mockRejectedValue(new Error('network down'));
518+
const client = { GET: getSpy } as unknown as MedkitClient;
519+
520+
await expect(fetchAllAppsDeduped(client)).resolves.toEqual([]);
521+
});
522+
});
523+
468524
// =============================================================================
469525
// isPeerSourcedComponent
470526
// =============================================================================

0 commit comments

Comments
 (0)