Skip to content

fix(frontend): keep scenario list visible during delete mutations#992

Merged
adamflagg merged 2 commits intomainfrom
feature/fix-delete-scenario-render
Apr 25, 2026
Merged

fix(frontend): keep scenario list visible during delete mutations#992
adamflagg merged 2 commits intomainfrom
feature/fix-delete-scenario-render

Conversation

@adamflagg
Copy link
Copy Markdown
Owner

@adamflagg adamflagg commented Apr 24, 2026

Summary

Fixes #16 from the April 2026 staff-testing feedback round: deleting a scenario made the scenario list vanish behind the confirmation dialog, leaving only the hardcoded CampMinder card visible.

Root cause: ScenarioContext exposed one loading flag that combined isLoading (React Query initial fetch) with isPending from all four mutations (create/update/delete/clear). ScenarioManagementModal used that flag to swap the scenario cards for a "Loading scenarios..." placeholder, so any mutation replaced the whole list. The effect was especially visible on delete because the current delete mutation serially deletes every related draft assignment from the frontend (N+1 round-trips against PocketBase, cascadeDelete: false on bunk_assignments_draft.scenario).

Fix: Split the context's loading into isLoading (query only) and isMutating (mutations), keeping loading as the combined flag for backward compat. The modal now consumes isLoading only, so scenario cards stay in place while a delete/clear is in flight. The per-row "Deleting..." / "Clearing..." button state (already wired via local isProcessing) remains the correct in-progress affordance.

Follow-up PR (separate): switch bunk_assignments_draft.scenario to cascadeDelete: true and simplify useDeleteScenario to a single PB delete. That collapses the long serial delete into a server-side cascade, addressing the "long delete" part of the report independently.

Test plan

  • Failing tests written first (ScenarioContext.test.tsx, ScenarioManagementModal.test.tsx)
  • Red run: 3 failures confirmed — isLoading/isMutating not on context type, modal swaps list to placeholder during delete
  • Implementation: expose isLoading/isMutating separately; modal reads isLoading
  • Green run: 4/4 targeted tests pass
  • Full frontend suite: 2796 passing
  • prettier + tsc clean; 0 eslint errors (only pre-existing warnings)

🤖 Generated with Claude Code

Manual validation (dev/preview)

Scoreboard item covered: #16 — Scenario delete render glitch + list vanish

  • Open the Scenarios modal on a session with several scenarios.
  • Click Delete on one scenario, confirm.
  • While the delete is in flight: the rest of the scenario list REMAINS VISIBLE (no "Loading scenarios..." placeholder swap).
  • The deleting scenario's row shows its own "Deleting..." button state.
  • After delete completes, the list shows the remaining scenarios correctly.
  • Re-open the modal — list state is clean, no stale placeholder.
  • Repeat with Clear (if that mutation is exposed) — same expectations.

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Improved loading state behavior in scenario management—the "Loading scenarios" placeholder now displays only during initial data fetch, not during subsequent operations like deletions, keeping your scenario list visible and interactive throughout.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

Warning

Rate limit exceeded

@adamflagg has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 21 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 21 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 45d8529c-d758-4d83-ba01-80150e298bc2

📥 Commits

Reviewing files that changed from the base of the PR and between 1ff1744 and 1432cb7.

📒 Files selected for processing (5)
  • frontend/src/components/ScenarioManagementModal.test.tsx
  • frontend/src/components/ScenarioManagementModal.tsx
  • frontend/src/contexts/ScenarioContext.test.tsx
  • frontend/src/contexts/ScenarioContext.tsx
  • frontend/src/hooks/useScenario.ts
📝 Walkthrough

Walkthrough

The changes refactor loading-state tracking in ScenarioContext to separately expose isLoading (initial query fetch) and isMutating (in-flight mutations), while maintaining backward-compatible loading flag. New tests validate both context and modal behavior under different loading scenarios.

Changes

Cohort / File(s) Summary
Context & Hook Updates
frontend/src/contexts/ScenarioContext.tsx, frontend/src/hooks/useScenario.ts
Updated ScenarioContextType to expose isLoading and isMutating fields separately. Context provider now computes isMutating from pending mutation states and retains loading as a combined signal for backward compatibility.
Component Logic
frontend/src/components/ScenarioManagementModal.tsx
Modified to derive the "Loading scenarios..." placeholder from isLoading instead of loading, preventing the placeholder from appearing during mutations that don't represent initial fetch states.
Context Tests
frontend/src/contexts/ScenarioContext.test.tsx
New test suite validating that useScenario() context exposes both isLoading and isMutating fields and correctly distinguishes between initial fetch loading and mutation states during delete operations.
Modal Tests
frontend/src/components/ScenarioManagementModal.test.tsx
New test suite covering modal render-state transitions, asserting that the loading placeholder appears only during initial fetch (isLoading: true) and not during subsequent mutations (isMutating: true).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Once tangled loading flags now split in two,
One for fetches, one when mutations brew,
The modal breathes—no flashing on each change,
States separate, clean, and rearranged!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The pull request addresses staff-testing issue #16 about fixing scenario list visibility during deletions, but the linked issue #16 is about consolidating CLAUDE documentation files, which is unrelated. Link the correct issue related to the delete mutation visibility fix, or clarify why issue #16 appears to be mismatched with the PR objectives.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title "fix(frontend): keep scenario list visible during delete mutations" directly and clearly summarizes the main change: keeping the scenario list visible during delete operations.
Out of Scope Changes check ✅ Passed All changes are focused on fixing the scenario list visibility during mutations by refactoring the loading state logic and adding comprehensive regression tests.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/fix-delete-scenario-render

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
frontend/src/components/ScenarioManagementModal.test.tsx (1)

46-62: Nit: redundant as ScenarioContextType cast.

The function return type is already ScenarioContextType, so the trailing cast is dead weight. Removing it gives you stronger type checking on the object literal (e.g. catches accidental shape drift if ScenarioContextType evolves).

♻️ Suggested change
-    ...overrides,
-  } as ScenarioContextType
+    ...overrides,
+  }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/src/components/ScenarioManagementModal.test.tsx` around lines 46 -
62, Remove the redundant "as ScenarioContextType" cast on the returned object
literal; since the function already declares a ScenarioContextType return,
dropping the cast will enable stricter type checking and catch shape drift.
Locate the helper that returns the object (the return block containing
currentScenario/isProductionMode/scenarios/... and ...overrides) and delete the
trailing "as ScenarioContextType" cast; ensure the function signature itself
remains typed as ScenarioContextType so the compiler will validate the object
shape against ScenarioContextType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@frontend/src/contexts/ScenarioContext.test.tsx`:
- Around line 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.

---

Nitpick comments:
In `@frontend/src/components/ScenarioManagementModal.test.tsx`:
- Around line 46-62: Remove the redundant "as ScenarioContextType" cast on the
returned object literal; since the function already declares a
ScenarioContextType return, dropping the cast will enable stricter type checking
and catch shape drift. Locate the helper that returns the object (the return
block containing currentScenario/isProductionMode/scenarios/... and
...overrides) and delete the trailing "as ScenarioContextType" cast; ensure the
function signature itself remains typed as ScenarioContextType so the compiler
will validate the object shape against ScenarioContextType.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 67c1bf8d-19f3-4cc3-8efa-34cc599d8b03

📥 Commits

Reviewing files that changed from the base of the PR and between 1caaa33 and 1ff1744.

📒 Files selected for processing (5)
  • frontend/src/components/ScenarioManagementModal.test.tsx
  • frontend/src/components/ScenarioManagementModal.tsx
  • frontend/src/contexts/ScenarioContext.test.tsx
  • frontend/src/contexts/ScenarioContext.tsx
  • frontend/src/hooks/useScenario.ts

Comment on lines +132 to +141
// 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)
Copy link
Copy Markdown

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.

@adamflagg adamflagg force-pushed the feature/fix-delete-scenario-render branch from daad911 to 8acf8e1 Compare April 25, 2026 03:23
adamflagg added a commit that referenced this pull request Apr 25, 2026
…elete

Follow-up to #992. The scenario delete mutation previously fetched every
draft assignment for a scenario and deleted them one-by-one from the
frontend (N+1 serial HTTP DELETEs) before deleting the scenario itself,
because bunk_assignments_draft.scenario had cascadeDelete=false in
migration 1500000022. On real sessions this took several seconds and
was the visible component of the "list vanishes behind the confirm
modal" report — #992 made the list stop vanishing, and this PR makes the
delete near-instant.

Changes:
- New migration 1500000098 flips cascadeDelete to true on
  bunk_assignments_draft.scenario (relation to saved_scenarios). All other
  relation properties preserved exactly as created in 1500000022.
- useDeleteScenario now issues a single DELETE against saved_scenarios;
  PocketBase cascades the draft rows server-side.
- New test pins the single-call behavior: draft collection is never
  queried or written during a scenario delete.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
adamflagg and others added 2 commits April 24, 2026 22:39
The ScenarioContext exposed a single `loading` flag that went true whenever
either the initial React Query fetch or any mutation (create/update/delete/
clear) was pending. ScenarioManagementModal used that flag to swap the
scenario cards for a "Loading scenarios..." placeholder, so clicking Delete
made the entire list vanish behind the confirmation dialog — leaving only
the hardcoded CampMinder card visible. The effect was pronounced because
deleting a scenario serially deletes every related draft assignment from
the frontend (cascadeDelete=false on bunk_assignments_draft.scenario), so
the placeholder window lasts several seconds on real sessions.

Split the provider into `isLoading` (initial query fetch) and `isMutating`
(any mutation pending), retaining `loading` as the combined flag for
backward compatibility. Updated the modal to consume `isLoading` only, so
the cards stay in place during delete/clear and the per-row "Deleting..."
button state remains the correct in-progress affordance.

The slow-delete itself is a separate schema issue tracked for a follow-up
PR — switching the relation to cascadeDelete:true will collapse the N+1
serial deletes into a single server-side cascade.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@adamflagg adamflagg force-pushed the feature/fix-delete-scenario-render branch from 8acf8e1 to 1432cb7 Compare April 25, 2026 05:39
@adamflagg adamflagg enabled auto-merge (squash) April 25, 2026 05:39
adamflagg added a commit that referenced this pull request Apr 25, 2026
…elete

Follow-up to #992. The scenario delete mutation previously fetched every
draft assignment for a scenario and deleted them one-by-one from the
frontend (N+1 serial HTTP DELETEs) before deleting the scenario itself,
because bunk_assignments_draft.scenario had cascadeDelete=false in
migration 1500000022. On real sessions this took several seconds and
was the visible component of the "list vanishes behind the confirm
modal" report — #992 made the list stop vanishing, and this PR makes the
delete near-instant.

Changes:
- New migration 1500000098 flips cascadeDelete to true on
  bunk_assignments_draft.scenario (relation to saved_scenarios). All other
  relation properties preserved exactly as created in 1500000022.
- useDeleteScenario now issues a single DELETE against saved_scenarios;
  PocketBase cascades the draft rows server-side.
- New test pins the single-call behavior: draft collection is never
  queried or written during a scenario delete.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@adamflagg adamflagg merged commit c9d1081 into main Apr 25, 2026
19 checks passed
@adamflagg adamflagg deleted the feature/fix-delete-scenario-render branch April 25, 2026 05:45
adamflagg added a commit that referenced this pull request Apr 25, 2026
…elete (#994)

## Summary

Follow-up to **#992** (scoreboard item **#16b**). #992 fixed the visible
"list vanishes" bug; this PR kills the underlying "long delete" — the
several-second serial-delete loop that made the disappearance so
noticeable.

**Root cause:** `bunk_assignments_draft.scenario` was declared with
`cascadeDelete: false` in migration `1500000022`, so `useDeleteScenario`
had to fetch every draft assignment for the scenario and delete them
one-by-one from the frontend before deleting the scenario itself. For a
real session, that's N+1 round-trips against PocketBase on every delete.

**Fix:** New migration `1500000098` flips the relation to
`cascadeDelete: true` (all other properties preserved exactly as in
`1500000022`). `useDeleteScenario` collapses to a single
`pb.collection('saved_scenarios').delete(id)` — PocketBase cascades the
draft rows server-side.

## Base branch

This PR targets `feature/fix-delete-scenario-render` (PR #992) rather
than `main`. Once #992 merges, I'll retarget this at `main` or it'll
auto-rebase.

## Test plan

- [x] Failing TDD test first (`useDeleteScenario` asserted no calls to
`bunk_assignments_draft.getFullList` / `.delete` — red against the N+1
loop).
- [x] Implementation: single-call mutation; test goes green.
- [x] Full frontend suite: 2797 passing.
- [x] prettier / tsc / eslint (0 errors) clean; pre-push hooks green.
- [x] Migration symmetry: `down` restores `cascadeDelete: false` so a
rollback re-requires the frontend loop if paired with an older build.

## Manual validation (dev/preview)

Scoreboard item covered: **#16b** — cascadeDelete bunk_assignments_draft
(long-delete fix, follow-up to #16)

Requires PR #992 to be merged first, OR testing against the #994 branch
directly.

- [ ] Open a session with a saved scenario containing many draft bunk
assignments (ideally a populated session, not an empty one).
- [ ] Delete the scenario. Time it roughly.
- [ ] Expected: delete completes in ≲1 second (single server-side
cascade).
- [ ] Compare: before this PR, delete was multi-second due to N+1
frontend round-trips deleting each draft assignment individually.
- [ ] Verify scenario is gone from the modal list and no orphan
`bunk_assignments_draft` rows remain (spot-check via PocketBase admin or
a query if available).
- [ ] Create a new scenario and confirm it still works end-to-end
(migration didn't break upstream writes).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant