Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,32 @@ describe('consoleFetch', () => {
).rejects.not.toBeInstanceOf(RetryError);
});

it('does not console.warn when fetch is aborted via AbortController', async () => {
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
const controller = new AbortController();
window.fetch = jest.fn(() =>
Promise.reject(new DOMException('The operation was aborted.', 'AbortError')),
);

await expect(
coFetch('/api/lifecycle/test', { signal: controller.signal }),
).rejects.toMatchObject({ name: 'AbortError' });
expect(consoleSpy).not.toHaveBeenCalled();
consoleSpy.mockRestore();
});

it('does console.warn when fetch fails with a real network error', async () => {
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
window.fetch = jest.fn(() => Promise.reject(new TypeError('Network failure')));

await expect(coFetch('/api/lifecycle/test')).rejects.toThrow('Network failure');
expect(consoleSpy).toHaveBeenCalledWith(
expect.stringContaining('/api/lifecycle/test'),
expect.any(TypeError),
);
consoleSpy.mockRestore();
});

it('should retry up to 3 times when RetryError is thrown', async () => {
window.fetch = jest.fn(() =>
Promise.resolve({ status: 404, headers: emptyHeaders } as Response),
Expand Down
2 changes: 2 additions & 0 deletions frontend/packages/console-shared/src/utils/console-fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ export const coFetch: ConsoleFetch = async (url, options = {}, timeout = 60000)
} catch (e) {
if (e instanceof RetryError) {
retry = true;
} else if (e instanceof DOMException && e.name === 'AbortError') {
throw e; // expected cancellation — don't warn
} else {
// eslint-disable-next-line no-console
console.warn(`consoleFetch failed for url ${url}`, e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,8 @@ const managedNamespacesColumnClass = css('pf-m-hidden', 'pf-m-visible-on-sm');
const statusColumnClass = css('pf-m-hidden', 'pf-m-visible-on-lg');
const lastUpdatedColumnClass = css('pf-m-hidden', 'pf-m-visible-on-2xl');
const providedAPIsColumnClass = css('pf-m-hidden', 'pf-m-visible-on-xl');
const clusterCompatibilityColumnClass = css('pf-m-hidden', 'pf-m-visible-on-xl');
const supportPhaseColumnClass = css('pf-m-hidden', 'pf-m-visible-on-xl');
const clusterCompatibilityColumnClass = css('pf-m-hidden', 'pf-m-visible-on-xl', 'pf-m-width-15');
const supportPhaseColumnClass = css('pf-m-hidden', 'pf-m-visible-on-xl', 'pf-m-width-15');

const SubscriptionStatus: FC<{ muted?: boolean; subscription: SubscriptionKind }> = ({
muted = false,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import { act, renderHook } from '@testing-library/react';
import { coFetchJSON } from '@console/shared/src/utils/console-fetch';
import { useOperatorLifecycle } from '../useOperatorLifecycle';

jest.mock('@console/shared/src/utils/console-fetch', () => ({
coFetchJSON: jest.fn(),
}));
const mockCoFetchJSON = (coFetchJSON as unknown) as jest.Mock;

// Each test uses a unique packageName to avoid sharing module-level cache entries.
const CATALOG = 'redhat-operators';
const NS = 'openshift-marketplace';

describe('useOperatorLifecycle', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('returns data when fetch succeeds', async () => {
const mockData = { lifecycleSchema: 'test', properties: '{}' };
mockCoFetchJSON.mockResolvedValueOnce(mockData);

const { result } = renderHook(() => useOperatorLifecycle('pkg-success', CATALOG, NS));

expect(result.current[1]).toBe(true); // loading initially

await act(async () => {
await Promise.resolve();
});

const [data, loading, error] = result.current;
expect(data).toEqual(mockData);
expect(loading).toBe(false);
expect(error).toBeNull();
});

it('surfaces real network errors as errors', async () => {
const networkError = new Error('Network failure');
mockCoFetchJSON.mockRejectedValueOnce(networkError);

const { result } = renderHook(() => useOperatorLifecycle('pkg-network-error', CATALOG, NS));

await act(async () => {
await Promise.resolve();
await Promise.resolve();
});

const [data, loading, error] = result.current;
expect(error).toBe(networkError);
expect(data).toBeNull();
expect(loading).toBe(false);
});

it('skips fetch when packageName is missing', () => {
renderHook(() => useOperatorLifecycle(undefined, CATALOG, NS));

expect(mockCoFetchJSON).not.toHaveBeenCalled();
const [data, loading, error] = renderHook(() =>
useOperatorLifecycle(undefined, CATALOG, NS),
).result.current;
expect(data).toBeNull();
expect(loading).toBe(false);
expect(error).toBeNull();
});

it('multiple callers sharing the same cache key all receive data from a single fetch', async () => {
// Core deduplication invariant: N components for the same operator should never
// issue more than one in-flight request.
const sharedData = { lifecycleSchema: 'shared', properties: '{}' };
mockCoFetchJSON.mockResolvedValueOnce(sharedData);

const PKG = 'pkg-shared-dedup';

// All three mount and subscribe to the same in-flight promise
const { result: resultA } = renderHook(() => useOperatorLifecycle(PKG, CATALOG, NS));
const { result: resultB } = renderHook(() => useOperatorLifecycle(PKG, CATALOG, NS));
const { result: resultC } = renderHook(() => useOperatorLifecycle(PKG, CATALOG, NS));

await act(async () => {
await Promise.resolve();
await Promise.resolve();
});

expect(resultA.current[0]).toEqual(sharedData);
expect(resultB.current[0]).toEqual(sharedData);
expect(resultC.current[0]).toEqual(sharedData);
// Only one fetch regardless of how many components share the key
expect(mockCoFetchJSON).toHaveBeenCalledTimes(1);
});

it('a component mounting after the cache is populated serves data immediately without fetching', async () => {
const cachedData = { lifecycleSchema: 'cached', properties: '{}' };
mockCoFetchJSON.mockResolvedValueOnce(cachedData);

const PKG = 'pkg-cache-hit';

// First component populates the cache
const { result: result1 } = renderHook(() => useOperatorLifecycle(PKG, CATALOG, NS));

await act(async () => {
await Promise.resolve();
});

expect(result1.current[0]).toEqual(cachedData);
expect(mockCoFetchJSON).toHaveBeenCalledTimes(1);

// Second component — cache hit, no new fetch
const { result: result2 } = renderHook(() => useOperatorLifecycle(PKG, CATALOG, NS));
expect(result2.current[0]).toEqual(cachedData);
expect(result2.current[1]).toBe(false); // not loading
expect(result2.current[2]).toBeNull();
expect(mockCoFetchJSON).toHaveBeenCalledTimes(1); // still just one fetch
});
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect, useRef, useState } from 'react';
import { useEffect, useState } from 'react';
import { coFetchJSON } from '@console/shared/src/utils/console-fetch';
import type { LifecycleData } from '../components/operator-lifecycle-status';
import { DEFAULT_SOURCE_NAMESPACE } from '../const';
Expand Down Expand Up @@ -41,46 +41,28 @@ const extractPackageName = (olmProperties: string): string | undefined => {
}
};

const fetchLifecycleData = (
cacheKey: string,
url: string,
signal: AbortSignal,
): Promise<LifecycleData> => {
// Fetch lifecycle data and deduplicate concurrent requests via a module-level cache.
// Requests are intentionally not cancelled on component unmount: the response
// populates the cache so any later render of the same operator avoids a re-fetch,
// and React silently ignores state updates after unmount.
const fetchLifecycleData = (cacheKey: string, url: string): Promise<LifecycleData> => {
const existing = lifecycleCache.get(cacheKey);
if (existing?.promise) {
return existing.promise;
return existing.promise; // deduplicate: all callers share the one in-flight request
}

const promise = coFetchJSON(url, 'GET', { signal }).then(
const promise = coFetchJSON(url, 'GET', {}).then(
(result: LifecycleData) => {
lifecycleCache.set(cacheKey, {
data: result,
error: null,
timestamp: Date.now(),
});
lifecycleCache.set(cacheKey, { data: result, error: null, timestamp: Date.now() });
return result;
},
(err: Error) => {
if (err.name === 'AbortError') {
lifecycleCache.delete(cacheKey);
} else {
lifecycleCache.set(cacheKey, {
data: null,
error: err,
timestamp: Date.now(),
});
}
lifecycleCache.set(cacheKey, { data: null, error: err, timestamp: Date.now() });
throw err;
},
);

lifecycleCache.set(cacheKey, {
data: null,
error: null,
timestamp: Date.now(),
promise,
});

lifecycleCache.set(cacheKey, { data: null, error: null, timestamp: Date.now(), promise });
return promise;
};

Expand All @@ -92,7 +74,6 @@ export const useOperatorLifecycle = (
const [data, setData] = useState<LifecycleData | null>(null);
const [loading, setLoading] = useState(false);
const [error, setError] = useState<Error | null>(null);
const abortRef = useRef<AbortController>();

useEffect(() => {
if (!packageName || !catalogName || !catalogNamespace) {
Expand All @@ -110,35 +91,25 @@ export const useOperatorLifecycle = (
return undefined;
}

abortRef.current?.abort();
const controller = new AbortController();
abortRef.current = controller;

setLoading(true);

const url = `/api/olm/lifecycle/${encodeURIComponent(catalogNamespace)}/${encodeURIComponent(
catalogName,
)}/${encodeURIComponent(packageName)}`;

fetchLifecycleData(cacheKey, url, controller.signal)
fetchLifecycleData(cacheKey, url)
.then((result) => {
if (!controller.signal.aborted) {
setData(result);
setError(null);
setLoading(false);
}
setData(result);
setError(null);
setLoading(false);
})
.catch((err: Error) => {
if (!controller.signal.aborted) {
setData(null);
setError(err);
setLoading(false);
}
setData(null);
setError(err);
setLoading(false);
});

return () => {
controller.abort();
};
return undefined; // no cleanup: requests complete naturally
Comment on lines +100 to +112

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Prevent superseded requests from updating this hook.

When the inputs change while request A is pending, request B starts; if B resolves first and A resolves later, A unconditionally overwrites B’s state. Keep the request cacheable, but ignore callbacks from a cleaned-up effect. Add a regression test that rerenders from A to B, resolves B then A, and verifies B remains displayed.

Proposed fix
   useEffect(() => {
+    let active = true;
+
     // ...
     fetchLifecycleData(cacheKey, url)
       .then((result) => {
+        if (!active) return;
         setData(result);
         setError(null);
         setLoading(false);
       })
       .catch((err: Error) => {
+        if (!active) return;
         setData(null);
         setError(err);
         setLoading(false);
       });
 
-    return undefined; // no cleanup: requests complete naturally
+    return () => {
+      active = false;
+    };
   }, [packageName, catalogName, catalogNamespace]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fetchLifecycleData(cacheKey, url)
.then((result) => {
if (!controller.signal.aborted) {
setData(result);
setError(null);
setLoading(false);
}
setData(result);
setError(null);
setLoading(false);
})
.catch((err: Error) => {
if (!controller.signal.aborted) {
setData(null);
setError(err);
setLoading(false);
}
setData(null);
setError(err);
setLoading(false);
});
return () => {
controller.abort();
};
return undefined; // no cleanup: requests complete naturally
useEffect(() => {
let active = true;
// ...
fetchLifecycleData(cacheKey, url)
.then((result) => {
if (!active) return;
setData(result);
setError(null);
setLoading(false);
})
.catch((err: Error) => {
if (!active) return;
setData(null);
setError(err);
setLoading(false);
});
return () => {
active = false;
};
}, [packageName, catalogName, catalogNamespace]);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.ts`
around lines 100 - 112, Guard the fetchLifecycleData callbacks in the
useOperatorLifecycle effect with an active/cancelled flag, and mark the effect
inactive during cleanup so superseded requests cannot update data, error, or
loading state. Preserve request caching and return the cleanup function from the
effect. Add a regression test that rerenders from A to B, resolves B before A,
and verifies B remains displayed.

}, [packageName, catalogName, catalogNamespace]);

return [data, loading, error];
Expand Down