Skip to content
Merged
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
96 changes: 96 additions & 0 deletions frontend/src/components/ScenarioManagementModal.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/**
* Regression test for the "list vanishes during delete" bug reported in
* staff testing (April 2026). See ScenarioContext.test.tsx for full context.
*
* The modal must keep rendering the scenario cards while a delete mutation
* is pending — only the initial query-fetch state should swap the list for
* a "Loading scenarios..." placeholder.
*/
import { describe, it, expect, vi } from 'vitest'
import { render, screen } from '@testing-library/react'
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
import type { ReactNode } from 'react'
import { Toaster } from 'react-hot-toast'

// Mock useSyncStatusAPI — modal uses it for the CampMinder "synced" line.
vi.mock('../hooks/useSyncStatusAPI', () => ({
useSyncStatusAPI: () => ({ data: undefined }),
}))

// Mock useYear — modal reads currentYear for clear-scenario calls.
vi.mock('../hooks/useCurrentYear', async () => {
const actual = await vi.importActual<object>('../hooks/useCurrentYear')
return { ...actual, useYear: () => 2026 }
})

// Render the modal without its child modals doing anything.
vi.mock('./ScenarioEditModal', () => ({ default: () => null }))
vi.mock('./NewScenarioModal', () => ({ default: () => null }))

import ScenarioManagementModal from './ScenarioManagementModal'
import { ScenarioContext } from '../hooks/useScenario'
import type { ScenarioContextType, Scenario } from '../hooks/useScenario'

function makeContext(overrides: Partial<ScenarioContextType>): ScenarioContextType {
const scenarios: Scenario[] = [
{
id: 'scenario-1',
name: 'Dorm Cabin Plan A',
session_cm_id: 1000001,
is_active: true,
description: '',
created: '2026-04-01T00:00:00Z',
updated: '2026-04-01T00:00:00Z',
},
]
return {
currentScenario: null,
isProductionMode: true,
scenarios,
loading: false,
isLoading: false,
isMutating: false,
error: null,
loadScenarios: vi.fn().mockResolvedValue(undefined),
createScenario: vi.fn(),
selectScenario: vi.fn(),
updateScenario: vi.fn().mockResolvedValue(undefined),
deleteScenario: vi.fn().mockResolvedValue(undefined),
clearScenario: vi.fn().mockResolvedValue(undefined),
...overrides,
}
}

function renderModal(ctx: ScenarioContextType) {
const qc = new QueryClient({
defaultOptions: { queries: { retry: false }, mutations: { retry: false } },
})
const wrapper = ({ children }: { children: ReactNode }) => (
<QueryClientProvider client={qc}>
<ScenarioContext.Provider value={ctx}>{children}</ScenarioContext.Provider>
<Toaster />
</QueryClientProvider>
)
return render(<ScenarioManagementModal sessionId={1000001} onClose={vi.fn()} />, { wrapper })
}

describe('ScenarioManagementModal loading state', () => {
it('shows "Loading scenarios..." during initial query fetch', () => {
// Matches what ScenarioContext produces when the query is first loading:
// loading=true AND isLoading=true (scenarios list not yet fetched).
renderModal(makeContext({ loading: true, isLoading: true, scenarios: [] }))
expect(screen.getByText('Loading scenarios...')).toBeInTheDocument()
})

it('keeps scenario cards visible while a delete mutation is pending', () => {
// Simulates real context state mid-delete: the combined loading flag
// is true (because isMutating is true), but the query has already
// resolved (isLoading=false). Prior to the fix, the modal consumed the
// combined `loading` and swapped the list for the placeholder — this
// test asserts the fix by requiring the cards to remain visible.
renderModal(makeContext({ loading: true, isLoading: false, isMutating: true }))

expect(screen.getByText('Dorm Cabin Plan A')).toBeInTheDocument()
expect(screen.queryByText('Loading scenarios...')).not.toBeInTheDocument()
})
})
8 changes: 6 additions & 2 deletions frontend/src/components/ScenarioManagementModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,18 @@ export default function ScenarioManagementModal({
onClose,
}: ScenarioManagementModalProps) {
const currentYear = useYear()
// Read `isLoading` (initial query fetch) rather than the combined
// `loading` flag — otherwise the scenario list is replaced with a
// "Loading scenarios..." placeholder while a delete/clear is in flight,
// making the list appear to vanish behind the confirmation dialog.
const {
scenarios,
currentScenario,
selectScenario,
updateScenario,
deleteScenario,
clearScenario,
loading,
isLoading,
} = useScenario()
const { data: syncStatus } = useSyncStatusAPI()

Expand Down Expand Up @@ -157,7 +161,7 @@ export default function ScenarioManagementModal({

{/* Scenarios List */}
<div className="flex-1 space-y-3 overflow-y-auto p-6 pt-4">
{loading ? (
{isLoading ? (
<div className="text-muted-foreground py-8 text-center">Loading scenarios...</div>
) : scenarios.length === 0 ? (
<div className="py-8 text-center">
Expand Down
152 changes: 152 additions & 0 deletions frontend/src/contexts/ScenarioContext.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
/**
* Tests for ScenarioContext loading/mutation state split.
*
* Regression: staff testing (April 2026) reported that clicking "Delete" on a
* scenario caused the entire list in ScenarioManagementModal to vanish — only
* the hardcoded "CampMinder" card remained visible behind the confirmation
* prompt. Root cause: the provider's single `loading` flag went true for any
* pending mutation (create / update / delete / clear), and the modal rendered
* a "Loading scenarios..." placeholder in that branch, replacing the scenario
* cards. The fix exposes `isLoading` (initial query fetch) separately from
* `isMutating` (any mutation pending) so consumers can distinguish the two.
*/
import { describe, it, expect, vi, beforeEach } from 'vitest'
import { renderHook, act, waitFor } from '@testing-library/react'
import type { ReactNode } from 'react'
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'

// Mock pocketbase before importing the context
vi.mock('../lib/pocketbase', () => {
const collections: Record<string, unknown> = {}
return {
pb: {
collection: vi.fn((name: string) => {
collections[name] ??= {
getFullList: vi.fn().mockResolvedValue([]),
getOne: vi.fn(),
create: vi.fn(),
update: vi.fn(),
delete: vi.fn(),
}
return collections[name]
}),
},
getCurrentUser: vi.fn(() => ({ id: 'user-1', email: 'user@example.com' })),
}
})

// Mock auth so useSavedScenarios doesn't gate on isLoading
vi.mock('./AuthContext', () => ({
useAuth: () => ({ isLoading: false }),
}))

// Mock useYear so provider gets a stable year
vi.mock('../hooks/useCurrentYear', async () => {
const actual = await vi.importActual<object>('../hooks/useCurrentYear')
return {
...actual,
useYear: () => 2026,
}
})

import { pb } from '../lib/pocketbase'
import type { Mock } from 'vitest'
import { ScenarioProvider } from './ScenarioContext'
import { useScenario } from '../hooks/useScenario'

interface CollectionMock {
getFullList: Mock
getOne: Mock
create: Mock
update: Mock
delete: Mock
}

function getCollection(name: string): CollectionMock {
return (pb.collection as Mock)(name) as CollectionMock
}

function createWrapper() {
const qc = new QueryClient({
defaultOptions: { mutations: { retry: false }, queries: { retry: false } },
})
return ({ children }: { children: ReactNode }) => (
<QueryClientProvider client={qc}>
<ScenarioProvider>{children}</ScenarioProvider>
</QueryClientProvider>
)
}

beforeEach(() => {
vi.clearAllMocks()
})

describe('ScenarioContext loading vs mutating', () => {
it('exposes isLoading (query fetch) separately from isMutating (mutation pending)', () => {
const { result } = renderHook(() => useScenario(), { wrapper: createWrapper() })
// Both flags must exist on the public context shape.
expect(result.current).toHaveProperty('isLoading')
expect(result.current).toHaveProperty('isMutating')
})

it('does not flip isLoading to true during a delete mutation', async () => {
const savedScenarios = getCollection('saved_scenarios')
const drafts = getCollection('bunk_assignments_draft')

savedScenarios.getFullList.mockResolvedValue([
{
id: 'scenario-1',
name: 'Test Scenario',
session: 'pb-session-1',
year: 2026,
is_active: true,
description: '',
created: '2026-04-01T00:00:00Z',
updated: '2026-04-01T00:00:00Z',
collectionId: 'c1',
collectionName: 'saved_scenarios',
expand: { session: { cm_id: 1000001 } },
},
])

// Delete path: getFullList for drafts, then delete scenario.
// Keep the delete promise pending so we can observe state mid-flight.
drafts.getFullList.mockResolvedValue([])
let resolveScenarioDelete: (value?: unknown) => void = () => {}
savedScenarios.delete.mockImplementation(
() =>
new Promise((resolve) => {
resolveScenarioDelete = resolve
})
)

const { result } = renderHook(() => useScenario(), { wrapper: createWrapper() })

// Let the initial query settle.
await act(async () => {
await result.current.loadScenarios(1000001)
})
await waitFor(() => expect(result.current.scenarios).toHaveLength(1))
expect(result.current.isLoading).toBe(false)

// Start the delete but do NOT await it yet — we want to observe the
// in-flight state.
let deletePromise: Promise<void> = Promise.resolve()
act(() => {
deletePromise = result.current.deleteScenario('scenario-1')
})

// While the delete is pending: isMutating true, isLoading stays false.
await waitFor(() => expect(result.current.isMutating).toBe(true))
expect(result.current.isLoading).toBe(false)
Comment on lines +132 to +141
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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

React Testing Library 16 act warning behavior with synchronous mutation kickoff and waitFor

💡 Result:

In React Testing Library (RTL), the “act warning” you typically see with React 18+ is a symptom of state updates that are scheduled asynchronously (microtasks / promises / effects) and occur after the test has already moved on to assertions or completed. Key points for “synchronous mutation kickoff + waitFor” scenarios: 1) Prefer async RTL utilities for waiting, not extra manual act - RTL’s guidance for async is: use waitFor or findBy* / waitForElementToBeRemoved so the test waits for the UI update that results from async work. This is explicitly recommended as the “approach 1” to fix “not wrapped in act” warnings: wait for the result using async utilities like waitFor or findBy*. - RTL also notes that render and fireEvent are wrapped in act by default; the remaining warnings usually mean updates happen after the test finishes (i.e., you didn’t wait for the update). 2) waitFor itself is designed to handle act internally, but there are edge cases - There is an open issue reporting that waitFor-style utilities may not wrap the entire final round of updates in act in some cases, meaning state updates from the last iteration might not be applied as expected to the DOM. This suggests that with certain async patterns (especially where updates are driven by queued microtasks/promises), you can still encounter act-related noise even if you “only used waitFor”. - A related upstream RTL change (“Require await act(...)”) explains the underlying motivation: tests used to “pass” because flushing microtasks happened in a way that masked some missing-act situations; later behavior became stricter and required awaiting async act boundaries to flush state updates on microtask queues at the right times. 3) What to do when your mutation kickoff is synchronous but the UI update is async - If your mutation is initiated synchronously (e.g., button click handler starts a mutation), but the UI update that you assert on happens later (promise resolution, effect, microtask), then you must wait for that UI change with RTL async utilities. - Practical pattern: a) trigger the mutation (usually via userEvent or fireEvent) b) await waitFor (or use findBy*) for the post-mutation UI state c) assert after it appears - Avoid this anti-pattern: - doing assertions immediately after kickoff (or ending the test) while the real state update is still pending; that’s exactly when the “not wrapped in act” warning is produced. 4) If warnings persist, the likely root cause is that something still updates after your awaited condition - The RTL waitFor/act edge-case issue indicates that updates from the “final iteration” of waitFor may not get fully flushed into React before your test completes in certain configurations. - Another cause is test environment/config mismatches (for example, using a different act implementation via different renderers, or mixing test utilities). But in the standard RTL + React DOM setup, the most common fix remains: wait for the UI state that corresponds to the async work. 5) If you truly need intermediate assertions between microtask turns - RTL discusses an escape hatch via globalThis.IS_REACT_ACT_ENVIRONMENT=false for asserting intermediate states, but it also warns you’ll need to manually wait for every state update because you’re opting out of the extra act behavior. Example structure that typically avoids the warning - Use findBy* for “wait until the element shows up” const modal = await screen.findByRole('dialog'); expect(modal).toBeInTheDocument; - Or use waitFor for “wait until expectation passes” await waitFor( => { expect(screen.getByRole('dialog')).toBeInTheDocument; }); If you want, paste your exact test (especially the part where you kickoff the mutation and your waitFor callback), plus your versions of: - react, react-dom, @testing-library/react - user-event (if used) - test runner (jest/vitest) and I can suggest the smallest change that removes the warning.

Citations:


🌐 Web query:

React Query useMutation RTL act wrapper async pattern best practices

💡 Result:

Best practices for testing React Query (TanStack Query) useMutation hooks with React Testing Library (RTL) involve creating isolated test environments with fresh QueryClient instances, mocking network requests, wrapping components/hooks in QueryClientProvider, using act for synchronous updates, and waitFor for asynchronous state changes. ## Core Setup: Test Wrapper Create a reusable wrapper for every test to ensure isolation: tsx import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import { render, type RenderOptions } from '@testing-library/react' function createTestQueryClient { return new QueryClient({ defaultOptions: { queries: { retry: false }, // Disable retries for predictable tests mutations: { retry: false } } }) } function createWrapper { const queryClient = createTestQueryClient return ({ children }: { children: React.ReactNode }) => ( {children} ) } export function renderWithClient(ui: React.ReactElement, options?: Omit<RenderOptions, 'wrapper'>) { return render(ui, { wrapper: createWrapper, ...options }) } export function renderHookWithClient<TResult, TProps>( hook: (props?: TProps) => TResult ) { return renderHook(hook, { wrapper: createWrapper }) } ## Testing Components with useMutation (Integration Tests) Test user interactions end-to-end, mocking the network: tsx import { renderWithClient } from './test-utils' import { screen, waitFor } from '@testing-library/react' import userEvent from '@testing-library/user-event' test('should create a post successfully', async => { // Mock network request global.fetch = vi.fn( => Promise.resolve({ ok: true, json: => Promise.resolve({ id: 1, title: 'New Post' }) } as Response) ) renderWithClient() const user = userEvent.setup await user.type(screen.getByPlaceholderText('Post title'), 'New Post') await user.click(screen.getByRole('button', { name: /create/i })) // Assert loading state expect(screen.getByText('Creating...')).toBeInTheDocument // Wait for success state await waitFor( => { expect(screen.getByText('Post created!')).toBeInTheDocument }) expect(global.fetch).toHaveBeenCalledWith( '/api/posts', expect.objectContaining({ method: 'POST', body: JSON.stringify({ title: 'New Post' }) }) ) }) test('should handle mutation error', async => { global.fetch = vi.fn( => Promise.reject(new Error('Network error'))) renderWithClient() await userEvent.click(screen.getByRole('button', { name: /create/i })) await waitFor( => { expect(screen.getByText(/error/i)).toBeInTheDocument }) }) ## Testing Custom useMutation Hooks (Unit Tests) For custom hooks wrapping useMutation: tsx import { renderHookWithClient, waitFor } from './test-utils' test('useCustomMutation should succeed', async => { const mockMutationFn = vi.fn.mockResolvedValue({ success: true }) const { result } = renderHookWithClient( => useCustomMutation(mockMutationFn) ) // Trigger mutation inside act for synchronous updates await act(async => { result.current.mutate({ id: 1 }) }) // Wait for async state change await waitFor( => expect(result.current.isSuccess).toBe(true)) expect(result.current.data).toEqual({ success: true }) expect(mockMutationFn).toHaveBeenCalledWith({ id: 1 }) }) test('useCustomMutation should handle error', async => { const mockMutationFn = vi.fn.mockRejectedValue(new Error('Failed')) const { result } = renderHookWithClient( => useCustomMutation(mockMutationFn) ) await act(async => { result.current.mutate({ id: 1 }) }) await waitFor( => expect(result.current.isError).toBe(true)) expect(result.current.error?.message).toBe('Failed') }) ## Key Patterns & Best Practices 1. Always use fresh QueryClient per test - Prevents test pollution 2. Disable retries - retry: false makes tests predictable and fast 3. act + mutate - Wrap mutate calls in act for React 18+ compatibility 4. waitFor for state changes - Never use fixed timeouts; wait for specific conditions 5. Mock at network boundary - Use MSW or global.fetch mocks, not the hook itself 6. Test all states - pending, success, error 7. Prefer userEvent over fireEvent - More realistic interactions ## Using MSW (Recommended for complex APIs) tsx import { setupServer } from 'msw/node' import { rest } from 'msw' const server = setupServer( rest.post('/api/posts', (req, res, ctx) => { return res(ctx.json({ id: 1, title: 'New Post' })) }) ) beforeAll( => server.listen) afterEach( => server.resetHandlers) afterAll( => server.close) ## Common Pitfalls to Avoid - ❌ setTimeout or fixed sleeps - Use waitFor - ❌ Shared QueryClient - Always fresh instance - ❌ Testing implementation details - Focus on user-visible outcomes - ❌ No act around mutate - Causes React warnings - ❌ Mocking useMutation directly - Mock the network instead Official TanStack Query docs emphasize testing via renderHook with QueryClient wrapper and waiting for status changes. GitHub discussions confirm using act( => mutate) followed by waitFor( => expect(isSuccess).toBe(true)).","grounding":[{"field":"content","citations":[1,2,3,4,6,9,10,13,20,24],"confidence":"high"}],"top_results":[1,2,3,4,6,9,10,13,20,24]}

Citations:


🌐 Web query:

React Testing Library 16 synchronous act vs waitFor for useMutation state updates

💡 Result:

In React Testing Library (RTL) v16, for testing useMutation state updates (e.g., from TanStack Query/react-query), use waitFor to asynchronously wait for DOM changes reflecting the mutation's state (isPending, isSuccess, isError). This polls until the expected UI appears or changes, handling React's async state updates and effects. Use synchronous act for immediate, non-async actions like triggering the mutation via user events (fireEvent.click or userEvent.click), ensuring synchronous React updates are flushed before proceeding. RTL's render and fireEvent are pre-wrapped in act, but explicit async act may be needed for promises. Key distinction: - act( => { mutate; }) or await act(async => { await mutateAsync; }) for firing mutations synchronously. - await waitFor( => { expect(screen.getByText('Success')).toBeInTheDocument; }) for waiting on resulting state updates in the DOM. In RTL 16 with React 18/19, waitFor may not always flush final state updates to DOM due to changes in async act wrapping (open issue #1416), sometimes requiring extra await act(async => {}) after waitFor for batched updates. Example for useMutation test: test('successful mutation', async => { renderWithQueryClient(); await userEvent.click(screen.getByRole('button', { name: /submit/i })); await waitFor( => { expect(screen.getByText('Creating...')).toBeInTheDocument; // isPending }); // Mock resolves, wait for success state await waitFor( => { expect(screen.getByText('Post created!')).toBeInTheDocument; // isSuccess }); }); Official TanStack Query docs recommend waitFor for mutation state assertions. Prefer findBy* queries over waitFor(getBy*) when possible for cleaner async element queries.

Citations:


Adopt async act wrapper for the mutation kickoff to match React Query testing best practices.

The mutation in line 134 should be wrapped in await act(async () => { ... }) rather than non-async act(). This is the pattern recommended by the official React Query testing guide and ensures proper handling of promise-based state updates from useMutation. The current approach works but does not follow the documented standard for testing React Query mutations with React Testing Library.

await act(async () => {
  deletePromise = result.current.deleteScenario('scenario-1')
})

This pattern is especially important for mutation state transitions like the isMutating check on the following line.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/contexts/ScenarioContext.test.tsx` around lines 132 - 141, Wrap
the mutation kickoff in an async act to properly handle the promise-based state
transitions: replace the non-async act block that sets deletePromise (where
deletePromise is initialized and assigned via
result.current.deleteScenario('scenario-1')) with an await act(async () => { ...
}) so the mutation start is awaited by React Testing Library; keep checking
result.current.isMutating and result.current.isLoading as before after the async
act to observe the in-flight state.


// Resolve and drain.
await act(async () => {
resolveScenarioDelete()
await deletePromise
})

await waitFor(() => expect(result.current.isMutating).toBe(false))
expect(result.current.isLoading).toBe(false)
})
})
15 changes: 12 additions & 3 deletions frontend/src/contexts/ScenarioContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,21 @@ export const ScenarioProvider: FC<ScenarioProviderProps> = ({ children }) => {
clearScenarioMutation.error?.message ??
null

// Combined loading state
const loading =
isLoading ||
// Any mutation in flight. Tracked separately from `isLoading` so UI that
// renders list content (e.g. ScenarioManagementModal) doesn't replace the
// list with a placeholder while a delete/clear is processing.
const isMutating =
createScenarioMutation.isPending ||
updateScenarioMutation.isPending ||
deleteScenarioMutation.isPending ||
clearScenarioMutation.isPending

// Combined loading state (initial fetch OR mutation pending). Kept for
// backward compatibility with any consumer that just needs a unified
// "busy" signal. Consumers that drive empty-state/placeholder UI should
// read `isLoading` instead.
const loading = isLoading || isMutating

// Load scenarios for a session
const loadScenarios = useCallback(async (sessionId: number) => {
setCurrentSessionId(sessionId)
Expand Down Expand Up @@ -220,6 +227,8 @@ export const ScenarioProvider: FC<ScenarioProviderProps> = ({ children }) => {
isProductionMode,
scenarios,
loading,
isLoading,
isMutating,
error,
loadScenarios,
createScenario,
Expand Down
8 changes: 8 additions & 0 deletions frontend/src/hooks/useScenario.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,15 @@ export interface ScenarioContextType {
scenarios: Scenario[]

// Loading states
// `isLoading` reflects the initial React Query fetch — use this for
// empty/placeholder UI. `isMutating` reflects any in-flight mutation
// (create/update/delete/clear) — don't use it to hide list content, since
// doing so causes the list to "vanish" mid-mutation. `loading` is the
// combined flag, retained for existing consumers that only care that
// *something* is happening.
loading: boolean
isLoading: boolean
isMutating: boolean
error: string | null

// Actions
Expand Down
Loading