🧯 fix: Remove Revoked Agents from User Favorites (#12296)

* 🧯 fix: Remove revoked agents from user favorites

When agent access is revoked, the agent remained in the user's favorites
causing repeated 403 errors on page load. Backend now cleans up favorites
on permission revocation; frontend treats 403 like 404 and auto-removes
stale agent references.

* 🧪 fix: Address review findings for stale agent favorites cleanup

- Guard cleanup effect with ref to prevent infinite loop on mutation
  failure (Finding 1)
- Use validated results.revoked instead of raw request payload for
  revokedUserIds (Finding 3)
- Stabilize staleAgentIds memo with string key to avoid spurious
  re-evaluation during drag-drop (Finding 5)
- Add JSDoc with param types to removeRevokedAgentFromFavorites
  (Finding 7)
- Return promise from removeRevokedAgentFromFavorites for testability
- Add 7 backend tests covering revocation cleanup paths
- Add 3 frontend tests for 403 handling and stale cleanup persistence
This commit is contained in:
Danny Avila 2026-03-19 15:15:10 -04:00 committed by GitHub
parent b189972381
commit 93952f06b4
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 409 additions and 3 deletions

View file

@ -198,7 +198,8 @@ export default function FavoritesList({
} catch (error) {
if (error && typeof error === 'object' && 'response' in error) {
const axiosError = error as { response?: { status?: number } };
if (axiosError.response?.status === 404) {
const status = axiosError.response?.status;
if (status === 404 || status === 403) {
return { found: false };
}
}
@ -206,10 +207,34 @@ export default function FavoritesList({
}
},
staleTime: 1000 * 60 * 5,
enabled: missingAgentIds.length > 0,
})),
});
const staleAgentIdsKey = useMemo(() => {
const ids: string[] = [];
for (let i = 0; i < missingAgentIds.length; i++) {
const query = missingAgentQueries[i];
if (query.data && !query.data.found) {
ids.push(missingAgentIds[i]);
}
}
return ids.sort().join(',');
}, [missingAgentIds, missingAgentQueries]);
const cleanupAttemptedRef = useRef('');
useEffect(() => {
if (!staleAgentIdsKey || cleanupAttemptedRef.current === staleAgentIdsKey) {
return;
}
const staleSet = new Set(staleAgentIdsKey.split(','));
const cleaned = safeFavorites.filter((f) => !f.agentId || !staleSet.has(f.agentId));
if (cleaned.length < safeFavorites.length) {
cleanupAttemptedRef.current = staleAgentIdsKey;
reorderFavorites(cleaned, true);
}
}, [staleAgentIdsKey, safeFavorites, reorderFavorites]);
const combinedAgentsMap = useMemo(() => {
if (agentsMap === undefined) {
return undefined;

View file

@ -188,5 +188,86 @@ describe('FavoritesList', () => {
// No favorite items should be rendered (deleted agent is filtered out)
expect(queryAllByTestId('favorite-item')).toHaveLength(0);
});
it('should treat 403 the same as 404 — agent not rendered', async () => {
const validAgent: t.Agent = {
id: 'valid-agent',
name: 'Valid Agent',
author: 'test-author',
} as t.Agent;
mockFavorites.push({ agentId: 'valid-agent' }, { agentId: 'revoked-agent' });
(dataService.getAgentById as jest.Mock).mockImplementation(
({ agent_id }: { agent_id: string }) => {
if (agent_id === 'valid-agent') {
return Promise.resolve(validAgent);
}
if (agent_id === 'revoked-agent') {
return Promise.reject({ response: { status: 403 } });
}
return Promise.reject(new Error('Unknown agent'));
},
);
const { findAllByTestId } = renderWithProviders(<FavoritesList />);
const favoriteItems = await findAllByTestId('favorite-item');
expect(favoriteItems).toHaveLength(1);
expect(favoriteItems[0]).toHaveTextContent('Valid Agent');
});
it('should call reorderFavorites to persist removal of stale agents', async () => {
const mockReorderFavorites = jest.fn().mockResolvedValue(undefined);
mockUseFavorites.mockReturnValue({
favorites: [{ agentId: 'revoked-agent' }],
reorderFavorites: mockReorderFavorites,
isLoading: false,
});
(dataService.getAgentById as jest.Mock).mockRejectedValue({ response: { status: 403 } });
renderWithProviders(<FavoritesList />);
await waitFor(() => {
expect(mockReorderFavorites).toHaveBeenCalledWith([], true);
});
});
it('should only attempt cleanup once even when favorites revert to stale state', async () => {
const mockReorderFavorites = jest.fn().mockResolvedValue(undefined);
mockUseFavorites.mockReturnValue({
favorites: [{ agentId: 'revoked-agent' }],
reorderFavorites: mockReorderFavorites,
isLoading: false,
});
(dataService.getAgentById as jest.Mock).mockRejectedValue({ response: { status: 403 } });
const { rerender } = renderWithProviders(<FavoritesList />);
await waitFor(() => {
expect(mockReorderFavorites).toHaveBeenCalledWith([], true);
});
expect(mockReorderFavorites).toHaveBeenCalledTimes(1);
rerender(
<QueryClientProvider client={createTestQueryClient()}>
<RecoilRoot>
<BrowserRouter>
<DndProvider backend={HTML5Backend}>
<FavoritesList />
</DndProvider>
</BrowserRouter>
</RecoilRoot>
</QueryClientProvider>,
);
await new Promise((r) => setTimeout(r, 50));
expect(mockReorderFavorites).toHaveBeenCalledTimes(1);
});
});
});