refactor(app): Reduce unnecessary API health requests #5979
Conversation
📝 WalkthroughWalkthroughThis PR refactors API health monitoring from a custom hook pattern to a React Context-based pattern. It introduces Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes This refactor involves architectural changes across multiple file types—import migrations (repetitive, low complexity), a new context implementation with polling and AbortController management (moderate logic density), a comprehensive test suite for the context (higher density), and component simplifications (straightforward removals). While many changes follow consistent patterns, the new context implementation, its error-handling logic, and the expanded test coverage require careful review to validate the architectural shift and ensure no regressions. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/src/pages/launcher/page.tsx (1)
137-153: Remove redundant polling logic.This component implements its own polling mechanism (every 2 seconds), but the
ApiHealthProvideralready polls every 3 seconds globally. This creates duplicate API health check requests and could lead to race conditions or unexpected behavior.Since the component already receives
statusfrom the context (line 98), the polling effect should be removed and the component should rely entirely on the context's automatic polling.Apply this diff to remove the redundant polling:
- useEffect(() => { - // Simple delay to allow server startup, then start health checks - let interval: NodeJS.Timeout; - const timeout = setTimeout(() => { - checkHealth(); - interval = setInterval(() => { - checkHealth(); - }, 2000); - }, 3000); - - return () => { - clearTimeout(timeout); - if (interval) { - clearInterval(interval); - } - }; - }, [checkHealth]);The
ApiHealthProviderwill handle polling automatically, and the status updates will flow through the context to this component.
🧹 Nitpick comments (3)
src/app/src/pages/redteam/setup/components/Targets/CustomPoliciesSection.tsx (1)
161-162: Surface "checking" state in tooltip to improve UXRight now, when status is not connected, the tooltip always says connection is required. Consider showing a “Checking connection...” message while health is being checked.
Apply:
- const { status: apiHealthStatus } = useApiHealth(); + const { status: apiHealthStatus, isChecking } = useApiHealth();- <TestCaseGenerateButton + <TestCaseGenerateButton onClick={() => handleGenerateTestCase(params.row)} disabled={ apiHealthStatus !== 'connected' || (generatingTestCase && generatingPolicyIndex === params.row.id) } isGenerating={generatingTestCase && generatingPolicyIndex === params.row.id} - tooltipTitle={ - apiHealthStatus === 'connected' - ? undefined - : 'Promptfoo Cloud connection is required for test generation' - } + tooltipTitle={ + apiHealthStatus === 'connected' + ? undefined + : isChecking + ? 'Checking connection to Promptfoo Cloud...' + : 'Promptfoo Cloud connection is required for test generation' + } />Also applies to: 492-504
src/app/src/contexts/ApiHealthContext.test.tsx (1)
64-72: Optional: add a test for an initial health check on mountIf we make
usePollingtrigger a leading-edge call, add a test to assertcheckHealthruns once on mount. This protects UX regressions where status staysunknownfor the first interval.Example addition:
describe('ApiHealthProvider', () => { it('should initialize with default state', () => { @@ }); + it('should invoke a health check immediately on mount', async () => { + // Make usePolling call the provided function right away + vi.mocked(usePolling).mockImplementation((fn: any) => { fn(); }); + const wrapper = ({ children }: { children: ReactNode }) => ( + <ApiHealthProvider>{children}</ApiHealthProvider> + ); + const mockResponse = { json: vi.fn().mockResolvedValue({ status: 'OK', message: '' }) }; + vi.mocked(callApi).mockResolvedValue(mockResponse as any); + const { result } = renderHook(() => useApiHealth(), { wrapper }); + await waitFor(() => expect(result.current.status).toBe('connected')); + });src/app/src/contexts/ApiHealthContext.tsx (1)
77-77: Consider invokingcheckHealthimmediately on mount.The
usePollinghook only sets up the interval without an initial invocation, so the first health check happens after 3 seconds. This means users see an 'unknown' status for 3 seconds after the app loads.Add an effect to call
checkHealthimmediately:+import { createContext, useContext, useState, useRef, useCallback, useEffect } from 'react'; + export function ApiHealthProvider({ children }: { children: React.ReactNode }) { // ... existing state and checkHealth code ... + // Check health immediately on mount + useEffect(() => { + checkHealth(); + }, [checkHealth]); + usePolling(() => checkHealth(), API_POLLING_INTERVAL, []);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/app/src/App.tsx(2 hunks)src/app/src/components/ApiSettingsModal.test.tsx(4 hunks)src/app/src/components/ApiSettingsModal.tsx(1 hunks)src/app/src/contexts/ApiHealthContext.test.tsx(1 hunks)src/app/src/contexts/ApiHealthContext.tsx(1 hunks)src/app/src/hooks/useApiHealth.test.tsx(0 hunks)src/app/src/hooks/useApiHealth.tsx(0 hunks)src/app/src/hooks/usePolling.ts(1 hunks)src/app/src/pages/launcher/page.test.tsx(2 hunks)src/app/src/pages/launcher/page.tsx(1 hunks)src/app/src/pages/redteam/setup/components/Plugins.tsx(2 hunks)src/app/src/pages/redteam/setup/components/PluginsTab.tsx(3 hunks)src/app/src/pages/redteam/setup/components/Purpose.test.tsx(1 hunks)src/app/src/pages/redteam/setup/components/Purpose.tsx(1 hunks)src/app/src/pages/redteam/setup/components/Review.test.tsx(4 hunks)src/app/src/pages/redteam/setup/components/Review.tsx(2 hunks)src/app/src/pages/redteam/setup/components/Targets/CustomPoliciesSection.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- src/app/src/hooks/useApiHealth.test.tsx
- src/app/src/hooks/useApiHealth.tsx
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/gh-cli-workflow.mdc)
Prefer not to introduce new TypeScript types; use existing interfaces whenever possible
**/*.{ts,tsx}: Use TypeScript with strict type checking
Follow consistent import order (Biome will handle import sorting)
Use consistent curly braces for all control statements
Prefer const over let; avoid var
Use object shorthand syntax whenever possible
Use async/await for asynchronous code
Use consistent error handling with proper type checks
Always sanitize sensitive data before logging
Use structured logger methods (debug/info/warn/error) with a context object instead of interpolating secrets into log strings
Use sanitizeObject for manual sanitization in non-logging contexts before persisting or further processing data
Files:
src/app/src/App.tsxsrc/app/src/components/ApiSettingsModal.tsxsrc/app/src/pages/redteam/setup/components/Purpose.tsxsrc/app/src/contexts/ApiHealthContext.test.tsxsrc/app/src/pages/redteam/setup/components/Purpose.test.tsxsrc/app/src/components/ApiSettingsModal.test.tsxsrc/app/src/pages/launcher/page.tsxsrc/app/src/contexts/ApiHealthContext.tsxsrc/app/src/pages/launcher/page.test.tsxsrc/app/src/pages/redteam/setup/components/Plugins.tsxsrc/app/src/pages/redteam/setup/components/Targets/CustomPoliciesSection.tsxsrc/app/src/pages/redteam/setup/components/Review.test.tsxsrc/app/src/hooks/usePolling.tssrc/app/src/pages/redteam/setup/components/Review.tsxsrc/app/src/pages/redteam/setup/components/PluginsTab.tsx
src/app/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/app/CLAUDE.md)
src/app/src/**/*.{ts,tsx}: Never use fetch() directly; always use callApi() from @app/utils/api for all HTTP requests
Access Zustand state outside React components via store.getState(); do not call hooks outside components
Use the @app/* path alias for internal imports as configured in Vite
Files:
src/app/src/App.tsxsrc/app/src/components/ApiSettingsModal.tsxsrc/app/src/pages/redteam/setup/components/Purpose.tsxsrc/app/src/contexts/ApiHealthContext.test.tsxsrc/app/src/pages/redteam/setup/components/Purpose.test.tsxsrc/app/src/components/ApiSettingsModal.test.tsxsrc/app/src/pages/launcher/page.tsxsrc/app/src/contexts/ApiHealthContext.tsxsrc/app/src/pages/launcher/page.test.tsxsrc/app/src/pages/redteam/setup/components/Plugins.tsxsrc/app/src/pages/redteam/setup/components/Targets/CustomPoliciesSection.tsxsrc/app/src/pages/redteam/setup/components/Review.test.tsxsrc/app/src/hooks/usePolling.tssrc/app/src/pages/redteam/setup/components/Review.tsxsrc/app/src/pages/redteam/setup/components/PluginsTab.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-components.mdc)
**/*.{tsx,jsx}: Use icons from @mui/icons-material
Prefer commonly used icons from @mui/icons-material for intuitive experience
Files:
src/app/src/App.tsxsrc/app/src/components/ApiSettingsModal.tsxsrc/app/src/pages/redteam/setup/components/Purpose.tsxsrc/app/src/contexts/ApiHealthContext.test.tsxsrc/app/src/pages/redteam/setup/components/Purpose.test.tsxsrc/app/src/components/ApiSettingsModal.test.tsxsrc/app/src/pages/launcher/page.tsxsrc/app/src/contexts/ApiHealthContext.tsxsrc/app/src/pages/launcher/page.test.tsxsrc/app/src/pages/redteam/setup/components/Plugins.tsxsrc/app/src/pages/redteam/setup/components/Targets/CustomPoliciesSection.tsxsrc/app/src/pages/redteam/setup/components/Review.test.tsxsrc/app/src/pages/redteam/setup/components/Review.tsxsrc/app/src/pages/redteam/setup/components/PluginsTab.tsx
src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/app/**/*.{ts,tsx}: In the React app (src/app), always use callApi from @app/utils/api for API calls instead of fetch()
React hooks: use useMemo for computed values and useCallback for functions that accept arguments
Files:
src/app/src/App.tsxsrc/app/src/components/ApiSettingsModal.tsxsrc/app/src/pages/redteam/setup/components/Purpose.tsxsrc/app/src/contexts/ApiHealthContext.test.tsxsrc/app/src/pages/redteam/setup/components/Purpose.test.tsxsrc/app/src/components/ApiSettingsModal.test.tsxsrc/app/src/pages/launcher/page.tsxsrc/app/src/contexts/ApiHealthContext.tsxsrc/app/src/pages/launcher/page.test.tsxsrc/app/src/pages/redteam/setup/components/Plugins.tsxsrc/app/src/pages/redteam/setup/components/Targets/CustomPoliciesSection.tsxsrc/app/src/pages/redteam/setup/components/Review.test.tsxsrc/app/src/hooks/usePolling.tssrc/app/src/pages/redteam/setup/components/Review.tsxsrc/app/src/pages/redteam/setup/components/PluginsTab.tsx
src/app/src/components/**/*.tsx
📄 CodeRabbit inference engine (src/app/CLAUDE.md)
src/app/src/components/**/*.tsx: Create MUI styled components using @mui/material/styles styled API; use shouldForwardProp when adding custom props
Define strict TypeScript interfaces for all component props and provide defaults for optional props where appropriate
Add a displayName to components wrapped with React.memo for better debugging
Files:
src/app/src/components/ApiSettingsModal.tsxsrc/app/src/components/ApiSettingsModal.test.tsx
src/app/src/{components,pages}/**/*.tsx
📄 CodeRabbit inference engine (src/app/CLAUDE.md)
src/app/src/{components,pages}/**/*.tsx: Use the class-based ErrorBoundary component (@app/components/ErrorBoundary) to wrap error-prone UI
Access theme via useTheme() from @mui/material/styles instead of hardcoding theme values
Use useMemo/useCallback only when profiling indicates benefit; avoid unnecessary memoization
Implement explicit loading and error states for components performing async operations
Prefer MUI composition and the sx prop for styling over ad-hoc inline styles
Files:
src/app/src/components/ApiSettingsModal.tsxsrc/app/src/pages/redteam/setup/components/Purpose.tsxsrc/app/src/pages/redteam/setup/components/Purpose.test.tsxsrc/app/src/components/ApiSettingsModal.test.tsxsrc/app/src/pages/launcher/page.tsxsrc/app/src/pages/launcher/page.test.tsxsrc/app/src/pages/redteam/setup/components/Plugins.tsxsrc/app/src/pages/redteam/setup/components/Targets/CustomPoliciesSection.tsxsrc/app/src/pages/redteam/setup/components/Review.test.tsxsrc/app/src/pages/redteam/setup/components/Review.tsxsrc/app/src/pages/redteam/setup/components/PluginsTab.tsx
**/*.{test,spec}.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/gh-cli-workflow.mdc)
Avoid disabling or skipping tests unless absolutely necessary and documented
Files:
src/app/src/contexts/ApiHealthContext.test.tsxsrc/app/src/pages/redteam/setup/components/Purpose.test.tsxsrc/app/src/components/ApiSettingsModal.test.tsxsrc/app/src/pages/launcher/page.test.tsxsrc/app/src/pages/redteam/setup/components/Review.test.tsx
src/app/src/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (src/app/CLAUDE.md)
src/app/src/**/*.test.{ts,tsx}: In tests, import testing APIs from vitest (e.g., describe, it, expect, vi) and not from @jest/globals
Mock API calls in tests; do not make real network requests (e.g., vi.mock('@app/utils/api'))
Wrap MUI components in a ThemeProvider when testing components that depend on theme
Prefer Testing Library queries that reflect user behavior (getByRole, getByLabelText, etc.)
Clean up after tests as needed (e.g., clear/reset mocks) despite Vitest auto-cleanup
When testing React Router components, render under a MemoryRouter
Use Vitest as the test runner for the frontend app; do not use Jest APIs
Files:
src/app/src/contexts/ApiHealthContext.test.tsxsrc/app/src/pages/redteam/setup/components/Purpose.test.tsxsrc/app/src/components/ApiSettingsModal.test.tsxsrc/app/src/pages/launcher/page.test.tsxsrc/app/src/pages/redteam/setup/components/Review.test.tsx
src/app/src/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/app/CLAUDE.md)
Custom hooks that read context must validate the context and throw a clear error if undefined
Files:
src/app/src/hooks/usePolling.ts
🧠 Learnings (8)
📚 Learning: 2025-10-11T15:29:11.784Z
Learnt from: CR
PR: promptfoo/promptfoo#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-11T15:29:11.784Z
Learning: Applies to src/app/**/*.{ts,tsx} : React hooks: use useMemo for computed values and useCallback for functions that accept arguments
Applied to files:
src/app/src/pages/redteam/setup/components/Purpose.tsxsrc/app/src/pages/redteam/setup/components/PluginsTab.tsx
📚 Learning: 2025-10-05T17:00:56.424Z
Learnt from: CR
PR: promptfoo/promptfoo#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-05T17:00:56.424Z
Learning: Applies to test/providers/**/*.test.ts : Provider tests must cover: success case, error cases (4xx, 5xx, rate limits), configuration validation, and token usage tracking
Applied to files:
src/app/src/contexts/ApiHealthContext.test.tsx
📚 Learning: 2025-10-05T16:58:47.598Z
Learnt from: CR
PR: promptfoo/promptfoo#0
File: src/providers/CLAUDE.md:0-0
Timestamp: 2025-10-05T16:58:47.598Z
Learning: Applies to src/providers/test/providers/**/*.test.{ts,tsx,js,jsx} : Provider tests must cover rate limits, timeouts, and invalid configuration scenarios
Applied to files:
src/app/src/contexts/ApiHealthContext.test.tsx
📚 Learning: 2025-10-05T16:58:47.598Z
Learnt from: CR
PR: promptfoo/promptfoo#0
File: src/providers/CLAUDE.md:0-0
Timestamp: 2025-10-05T16:58:47.598Z
Learning: Applies to src/providers/test/providers/**/*.test.{ts,tsx,js,jsx} : Provider tests must mock API responses and never call real external services
Applied to files:
src/app/src/contexts/ApiHealthContext.test.tsx
📚 Learning: 2025-07-18T17:26:23.087Z
Learnt from: CR
PR: promptfoo/promptfoo#0
File: .cursor/rules/jest.mdc:0-0
Timestamp: 2025-07-18T17:26:23.087Z
Learning: Applies to test/**/*.{test,spec}.ts : Mock API responses to avoid external dependencies in tests
Applied to files:
src/app/src/pages/redteam/setup/components/Purpose.test.tsxsrc/app/src/components/ApiSettingsModal.test.tsx
📚 Learning: 2025-10-05T16:56:39.114Z
Learnt from: CR
PR: promptfoo/promptfoo#0
File: src/app/CLAUDE.md:0-0
Timestamp: 2025-10-05T16:56:39.114Z
Learning: Applies to src/app/src/**/*.test.{ts,tsx} : Mock API calls in tests; do not make real network requests (e.g., vi.mock('app/utils/api'))
Applied to files:
src/app/src/pages/redteam/setup/components/Purpose.test.tsxsrc/app/src/components/ApiSettingsModal.test.tsxsrc/app/src/pages/launcher/page.test.tsxsrc/app/src/pages/redteam/setup/components/Review.test.tsx
📚 Learning: 2025-10-05T16:56:39.114Z
Learnt from: CR
PR: promptfoo/promptfoo#0
File: src/app/CLAUDE.md:0-0
Timestamp: 2025-10-05T16:56:39.114Z
Learning: Applies to src/app/src/**/*.test.{ts,tsx} : In tests, import testing APIs from vitest (e.g., describe, it, expect, vi) and not from jest/globals
Applied to files:
src/app/src/components/ApiSettingsModal.test.tsxsrc/app/src/pages/launcher/page.test.tsx
📚 Learning: 2025-10-05T17:00:56.424Z
Learnt from: CR
PR: promptfoo/promptfoo#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-05T17:00:56.424Z
Learning: Applies to test/**/*.test.ts : Use Jest (not Vitest) APIs in this suite; avoid importing vitest
Applied to files:
src/app/src/pages/launcher/page.test.tsx
🧬 Code graph analysis (7)
src/app/src/App.tsx (1)
src/app/src/contexts/ApiHealthContext.tsx (1)
ApiHealthProvider(33-84)
src/app/src/contexts/ApiHealthContext.test.tsx (2)
src/app/src/contexts/ApiHealthContext.tsx (2)
useApiHealth(25-31)ApiHealthProvider(33-84)src/app/src/hooks/usePolling.ts (1)
usePolling(3-8)
src/app/src/contexts/ApiHealthContext.tsx (1)
src/app/src/hooks/usePolling.ts (1)
usePolling(3-8)
src/app/src/pages/redteam/setup/components/Plugins.tsx (1)
src/app/src/contexts/ApiHealthContext.tsx (1)
useApiHealth(25-31)
src/app/src/pages/redteam/setup/components/Targets/CustomPoliciesSection.tsx (1)
src/app/src/contexts/ApiHealthContext.tsx (1)
useApiHealth(25-31)
src/app/src/pages/redteam/setup/components/Review.tsx (1)
src/app/src/contexts/ApiHealthContext.tsx (1)
useApiHealth(25-31)
src/app/src/pages/redteam/setup/components/PluginsTab.tsx (1)
src/app/src/contexts/ApiHealthContext.tsx (1)
useApiHealth(25-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Tusk Test Runner (4)
- GitHub Check: Tusk Test Runner (2)
- GitHub Check: Tusk Test Runner (1)
- GitHub Check: Tusk Test Runner (3)
- GitHub Check: Redteam (Staging API)
- GitHub Check: Test on Node 22.x and windows-latest
- GitHub Check: Test on Node 24.x and windows-latest
- GitHub Check: Share Test
- GitHub Check: Build Docs
- GitHub Check: webui tests
- GitHub Check: Style Check
- GitHub Check: Test on Node 24.x and ubuntu-latest
- GitHub Check: Test on Node 20.x and macOS-latest
- GitHub Check: Redteam (Production API)
- GitHub Check: Test on Node 20.x and windows-latest
- GitHub Check: Test on Node 20.x and ubuntu-latest
- GitHub Check: Build on Node 24.x
- GitHub Check: Tusk Tester
🔇 Additional comments (17)
src/app/src/pages/redteam/setup/components/Purpose.tsx (2)
4-4: LGTM - Import path updated correctly.The import path has been updated to use the new context-based API health system.
276-280: The on-demand health check is justified. Verification confirms the ApiHealthProvider usesusePollingwith a 3000ms (3-second) interval. ThecheckHealth()call in Purpose.tsx is complementary, not redundant—it provides immediate feedback when the user configures a target, while the background polling continues for ongoing monitoring. The dependency array[hasTargetConfigured, checkHealth]is properly configured to prevent infinite loops. This is sound UX design: instant user feedback on action + continuous monitoring.src/app/src/pages/redteam/setup/components/Plugins.tsx (2)
3-3: LGTM - Import path migrated to context.The import has been correctly updated to use the new
ApiHealthContext.
141-141: LGTM - Simplified to use context-provided status.The component now relies on the
ApiHealthProviderfor automatic polling instead of triggering health checks on mount. This reduces redundant API calls.src/app/src/components/ApiSettingsModal.tsx (2)
3-3: LGTM - Import updated with type export.The import correctly uses the new context path and includes the
ApiHealthStatustype.
48-52: LGTM - Intentional health check on modal open.Calling
checkHealth()when the settings modal opens provides immediate, up-to-date status information to the user. This is appropriate for a settings UI.src/app/src/pages/launcher/page.test.tsx (1)
7-7: LGTM - Test imports and mocks updated correctly.The test has been updated to import and mock the new
ApiHealthContextpath. The mock structure remains compatible with the existing tests.Also applies to: 45-52
src/app/src/App.tsx (1)
27-27: LGTM - Context provider correctly integrated.The
ApiHealthProvideris properly wrapped around theRouterProvider, making API health status available to all routes. This establishes the context boundary for the entire application.Also applies to: 89-91
src/app/src/pages/launcher/page.tsx (1)
19-19: LGTM - Import path updated.src/app/src/pages/redteam/setup/components/PluginsTab.tsx (2)
1-1: LGTM - Clean migration to context-based health status.The component correctly removes
useEffectfrom imports and updates to use theApiHealthContext. It now relies solely on the context-providedstatuswithout triggering its own health checks.Also applies to: 55-55
110-110: LGTM - Simplified to consume status from context.The component no longer calls
checkHealth()on mount, relying on theApiHealthProvider's automatic polling instead. This eliminates redundant API calls.src/app/src/components/ApiSettingsModal.test.tsx (2)
1-1: LGTM - Test updated to mock new context path.The test correctly imports and mocks the
ApiHealthContextmodule path.Also applies to: 7-9
26-26: Message type isstring, notnull | string—test changes are correct.The verification confirms that
ApiHealthContextdefinesmessageas typestringwith a default value of''(empty string), notnull. The test's use ofmessage: ''is correct and consistent with the actual implementation.Likely an incorrect or invalid review comment.
src/app/src/pages/redteam/setup/components/Targets/CustomPoliciesSection.tsx (1)
3-3: Import swap to context looks goodSwitching to
@app/contexts/ApiHealthContextaligns with the new provider pattern. No issues.src/app/src/pages/redteam/setup/components/Purpose.test.tsx (1)
31-33: Mock path update is correctMocking
useApiHealthfrom@app/contexts/ApiHealthContextaligns with the refactor. Matches testing guidelines to mock API/state sources.src/app/src/pages/redteam/setup/components/Review.tsx (1)
4-4: Import swap to context is correctMatches the new centralized provider approach.
src/app/src/pages/redteam/setup/components/Review.test.tsx (1)
6-6: LGTM! Clean migration to context-based API health.The test file has been correctly updated to use the new
ApiHealthContextmodule instead of the old hook. The mock structure is preserved, and the test descriptions now accurately reflect that the component reads from context rather than performing mount-time health checks.Also applies to: 47-49, 305-320
| export const useApiHealth = () => { | ||
| // Ensure that the context is initialized | ||
| if (!ApiHealthContext) { | ||
| throw new Error('useApiHealth must be used within an ApiHealthProvider'); | ||
| } | ||
| return useContext(ApiHealthContext); | ||
| }; |
There was a problem hiding this comment.
Fix the context validation check.
The condition if (!ApiHealthContext) will always be false because createContext() returns an object, which is always truthy. This check doesn't actually validate whether the hook is used within a provider.
If you want to enforce provider usage, you could check the context value instead, but React's default behavior (returning the default context value) is typically sufficient. Consider removing this check or using a sentinel value in the default context to detect missing providers.
Apply this diff to remove the ineffective check:
export const useApiHealth = () => {
- // Ensure that the context is initialized
- if (!ApiHealthContext) {
- throw new Error('useApiHealth must be used within an ApiHealthProvider');
- }
return useContext(ApiHealthContext);
};🤖 Prompt for AI Agents
In src/app/src/contexts/ApiHealthContext.tsx around lines 25 to 31, the current
validation uses `if (!ApiHealthContext)` which is ineffective because
createContext returns a truthy object; remove that conditional check and simply
return `useContext(ApiHealthContext)` (or, if you want provider enforcement,
change the default context value to a sentinel like `null` and type the context
accordingly so you can check the returned value for null and throw a clear
error).
|
⏩ PR identified as a refactor (6f48d4d) View output ↗ View check history
|
|
Please consider adding the tusk tests! |
There was a problem hiding this comment.
I think we could use react query to do this instead of re-implementing the logic ourselves.
export function useApiHealth() {
return useQuery({
queryKey: ['apiHealth'],
queryFn: async () => {
const response = await callApi('/remote-health', {
cache: 'no-store',
});
const data = await response.json() as { status: string; message: string };
let status: ApiHealthStatus;
if (data.status === 'DISABLED') {
status = 'disabled';
} else {
status = data.status === 'OK' ? 'connected' : 'blocked';
}
return { status, message: data.message };
},
refetchInterval: 3000, // Poll every 3 seconds
retry: false,
staleTime: 2000,
});
}
|
@sklein12 react-query is great idea for the future. This is an improvement that could land today as is. |
|
React query is a 10 line drop in, it's a 10 minute refactor. I think it's worth it. This change is super complex and a lot of code to handle a single http call. This is the same thing we talked about two weeks ago with the other PR. |
will-holley
changed the title
Regenerated package-lock.json to include missing transitive dependencies (gaxios@5.1.3, https-proxy-agent@5.0.1, agent-base@6.0.2, node-fetch@2.7.0) that were causing npm ci to fail in CI.
Overrides gcp-metadata, gaxios, https-proxy-agent, node-fetch, and agent-base to versions required by mongoose/mongodb dependencies after tanstack/react-query addition caused version conflicts and removed nested package installations.
Summary
This PR is a minor refactor of the
useApiHealthhook, backing it with an app-scopedReact ContextTanStack Query instance that polls for status updates.Motivation
While working on #5869, I observed that we're making unnecessarily redundant
checkHealthcalls.