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
1 change: 1 addition & 0 deletions .pip-audit-ignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@
# Format: CVE-ID # reason — added YYYY-MM-DD, remove by YYYY-MM-DD

CVE-2026-4539 # pygments ReDoS in AdlLexer (archetype.py) — local-only, no upstream fix yet — added 2026-03-24, remove by 2026-04-24
CVE-2026-3219 # pip handles concatenated tar/ZIP files as ZIP — install-time only, no fix released yet — added 2026-04-25, remove by 2026-05-25
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)

// 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 @@ -46,14 +46,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 @@ -232,6 +239,8 @@ export const ScenarioProvider: FC<ScenarioProviderProps> = ({ children }) => {
isProductionMode,
scenarios,
loading,
isLoading,
isMutating,
error,
loadScenarios,
createScenario,
Expand Down
22 changes: 21 additions & 1 deletion frontend/src/hooks/useSavedScenariosMutation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ vi.mock('../lib/pocketbase', () => {
})

import { pb } from '../lib/pocketbase'
import { useCreateScenario } from './useSavedScenariosMutation'
import { useCreateScenario, useDeleteScenario } from './useSavedScenariosMutation'
import type { SavedScenario } from '../types/app-types'

function createWrapper() {
Expand Down Expand Up @@ -219,3 +219,23 @@ describe('Bug B: copyScenarioToScenario copies ALL source assignments without lo
})
})
})

describe('useDeleteScenario: relies on server-side cascade', () => {
it('deletes only the saved_scenarios row — does not pre-delete draft assignments', async () => {
const savedScenarios = getCollection('saved_scenarios')
const drafts = getCollection('bunk_assignments_draft')
savedScenarios.delete.mockResolvedValue(true)

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

await act(async () => {
await result.current.mutateAsync('scenario-to-delete')
})

// Single server call — no N+1 pre-delete loop.
expect(savedScenarios.delete).toHaveBeenCalledTimes(1)
expect(savedScenarios.delete).toHaveBeenCalledWith('scenario-to-delete')
expect(drafts.getFullList).not.toHaveBeenCalled()
expect(drafts.delete).not.toHaveBeenCalled()
})
})
16 changes: 5 additions & 11 deletions frontend/src/hooks/useSavedScenariosMutation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,17 +201,11 @@ export function useDeleteScenario() {

return useMutation({
mutationFn: async (scenarioId: string) => {
// Must delete all draft assignments first (PocketBase enforces referential integrity)
const draftAssignments = await pb.collection('bunk_assignments_draft').getFullList({
filter: `scenario = "${scenarioId}"`,
})

// Delete in batches to avoid overwhelming the server
for (const assignment of draftAssignments) {
await pb.collection('bunk_assignments_draft').delete(assignment.id)
}

// Now delete the scenario
// PocketBase cascades bunk_assignments_draft rows via
// cascadeDelete: true on the scenario relation (migration
// 1500000098). One server-side call replaces the previous N+1
// client-side pre-delete loop that made scenario deletion take
// several seconds on real sessions.
return await pb.collection<SavedScenario>('saved_scenarios').delete(scenarioId)
},
onSuccess: () => {
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
Loading