Stop the Plan page unmounting on every wish mutation

useLeaveWishes.fetch() runs again after every add/update/remove to
refresh the list, and it flipped the same isLoading flag the initial load
uses. Plan.tsx swaps the whole page for a spinner on that flag, so every
mutation unmounted and remounted WishEditor -- wiping which rows were
expanded and any save status.

This predates auto-save (the old manual Save button did it too), but a
debounce that fires on every edit turned an occasional flicker into the
dominant experience, collapsing the row the user was editing.

A background refetch now toggles a separate isRefreshing flag; isLoading
stays true only for the initial load. Plan.tsx needed no change once the
hook was right.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXMhc9nT6sNz1SL4j8dznL

frontend/src/hooks/useLeaveWishes.test.ts

@@ -1,7 +1,8 @@
 import { describe, it, expect, beforeEach, vi } from 'vitest'
-import { renderHook, act } from '@testing-library/react'
+import { renderHook, act, waitFor } from '@testing-library/react'
 import { useLeaveWishes } from './useLeaveWishes'
 import * as api from '@/services/api'
+import type { LeaveWish } from '@/types'
 
 vi.mock('@/services/api')
 
@@ -69,6 +70,18 @@ describe('useLeaveWishes', () => {
     expect(result.current.error).toContain('boom')
   })
 
+  it('falls back to a generic message when a non-Error value is thrown', async () => {
+    vi.mocked(api.getLeaveWishes).mockRejectedValue('boom')
+
+    const { result } = renderHook(() => useLeaveWishes('ws1'))
+
+    await act(async () => {
+      await new Promise((resolve) => setTimeout(resolve, 0))
+    })
+
+    expect(result.current.error).toBe('Failed to load leave wishes')
+  })
+
   it('add() creates a wish and refreshes the list', async () => {
     vi.mocked(api.getLeaveWishes).mockResolvedValue([])
     vi.mocked(api.createLeaveWish).mockResolvedValue(mockWish)
@@ -127,4 +140,76 @@ describe('useLeaveWishes', () => {
 
     expect(api.deleteLeaveWish).toHaveBeenCalledWith('lw1')
   })
+
+  it('sets isLoading (not isRefreshing) during the initial load', async () => {
+    let resolveInitial: (wishes: LeaveWish[]) => void = () => {}
+    vi.mocked(api.getLeaveWishes).mockImplementationOnce(
+      () =>
+        new Promise((resolve) => {
+          resolveInitial = resolve
+        }),
+    )
+
+    const { result } = renderHook(() => useLeaveWishes('ws1'))
+
+    await waitFor(() => expect(result.current.isLoading).toBe(true))
+    expect(result.current.isRefreshing).toBe(false)
+
+    await act(async () => {
+      resolveInitial([mockWish])
+      await new Promise((resolve) => setTimeout(resolve, 0))
+    })
+
+    expect(result.current.isLoading).toBe(false)
+    expect(result.current.isRefreshing).toBe(false)
+  })
+
+  it('sets isRefreshing (not isLoading) while a mutation background-refreshes the list', async () => {
+    vi.mocked(api.getLeaveWishes).mockResolvedValueOnce([])
+    vi.mocked(api.createLeaveWish).mockResolvedValue(mockWish)
+
+    const { result } = renderHook(() => useLeaveWishes('ws1'))
+    await act(async () => {
+      await new Promise((resolve) => setTimeout(resolve, 0))
+    })
+    expect(result.current.isLoading).toBe(false)
+
+    let resolveRefresh: (wishes: LeaveWish[]) => void = () => {}
+    vi.mocked(api.getLeaveWishes).mockImplementationOnce(
+      () =>
+        new Promise((resolve) => {
+          resolveRefresh = resolve
+        }),
+    )
+
+    let addPromise: Promise<LeaveWish> = Promise.resolve(mockWish)
+    act(() => {
+      addPromise = result.current.add({
+        workspaceId: 'ws1',
+        name: 'Sommar',
+        windowStartMonth: 6,
+        windowStartDay: 15,
+        windowEndMonth: 7,
+        windowEndDay: 26,
+        targetWeeks: 6,
+        recurrence: 'yearly',
+        firstYear: 2026,
+      })
+    })
+
+    await waitFor(() => expect(result.current.isRefreshing).toBe(true))
+    // The whole point of isRefreshing existing separately: a mutation's
+    // background refetch must never flip the initial-load flag, or a
+    // consumer gating a full-page spinner on isLoading would unmount its
+    // page during every save.
+    expect(result.current.isLoading).toBe(false)
+
+    await act(async () => {
+      resolveRefresh([mockWish])
+      await addPromise
+    })
+
+    expect(result.current.isRefreshing).toBe(false)
+    expect(result.current.isLoading).toBe(false)
+  })
 })

frontend/src/hooks/useLeaveWishes.ts

@@ -4,7 +4,14 @@ import type { LeaveWish, LeaveWishCreateRequest, LeaveWishUpdateRequest } from '
 
 interface UseLeaveWishesResult {
   data: LeaveWish[]
+  /** True only while the initial (or workspace-switch) load is in flight —
+   * safe to gate a full-page blocking spinner on. */
   isLoading: boolean
+  /** True while a post-mutation background refresh is in flight. Never true
+   * at the same time as `isLoading`; a consumer that renders a blocking
+   * spinner should NOT key off this — it exists so a mutation's list refetch
+   * doesn't unmount whatever is currently showing `data`. */
+  isRefreshing: boolean
   error: string | null
   refresh: () => Promise<void>
   add: (req: LeaveWishCreateRequest) => Promise<LeaveWish>
@@ -12,25 +19,40 @@ interface UseLeaveWishesResult {
   remove: (id: string) => Promise<void>
 }
 
-/** CRUD over a workspace's recurring leave wishes (`/api/leave-wishes`). */
+/** CRUD over a workspace's recurring leave wishes (`/api/leave-wishes`).
+ *
+ * `fetch` is called both on initial mount/workspace-change and after every
+ * mutation (to keep `data` in sync with the server). Those two cases need
+ * different loading semantics: the initial load should block the page, but
+ * a post-mutation refresh must not — it happens while the user is mid-edit
+ * of a form built from `data`, and flipping `isLoading` would unmount that
+ * form. `background: true` routes the fetch through `isRefreshing` instead
+ * of `isLoading` so callers can tell the two apart.
+ */
 export function useLeaveWishes(workspaceId: string | null): UseLeaveWishesResult {
   const [data, setData] = useState<LeaveWish[]>([])
   const [isLoading, setIsLoading] = useState(false)
+  const [isRefreshing, setIsRefreshing] = useState(false)
   const [error, setError] = useState<string | null>(null)
 
-  const fetch = useCallback(async () => {
-    if (!workspaceId) return
-    setIsLoading(true)
-    setError(null)
-    try {
-      const result = await api.getLeaveWishes(workspaceId)
-      setData(result)
-    } catch (err: unknown) {
-      setError(err instanceof Error ? err.message : 'Failed to load leave wishes')
-    } finally {
-      setIsLoading(false)
-    }
-  }, [workspaceId])
+  const fetch = useCallback(
+    async (options?: { background?: boolean }) => {
+      if (!workspaceId) return
+      const background = options?.background ?? false
+      const setBusy = background ? setIsRefreshing : setIsLoading
+      setBusy(true)
+      setError(null)
+      try {
+        const result = await api.getLeaveWishes(workspaceId)
+        setData(result)
+      } catch (err: unknown) {
+        setError(err instanceof Error ? err.message : 'Failed to load leave wishes')
+      } finally {
+        setBusy(false)
+      }
+    },
+    [workspaceId],
+  )
 
   useEffect(() => {
     void fetch()
@@ -39,7 +61,7 @@ export function useLeaveWishes(workspaceId: string | null): UseLeaveWishesResult
   const add = useCallback(
     async (req: LeaveWishCreateRequest) => {
       const created = await api.createLeaveWish(req)
-      await fetch()
+      await fetch({ background: true })
       return created
     },
     [fetch],
@@ -48,7 +70,7 @@ export function useLeaveWishes(workspaceId: string | null): UseLeaveWishesResult
   const update = useCallback(
     async (id: string, req: LeaveWishUpdateRequest) => {
       const updated = await api.updateLeaveWish(id, req)
-      await fetch()
+      await fetch({ background: true })
       return updated
     },
     [fetch],
@@ -57,10 +79,10 @@ export function useLeaveWishes(workspaceId: string | null): UseLeaveWishesResult
   const remove = useCallback(
     async (id: string) => {
       await api.deleteLeaveWish(id)
-      await fetch()
+      await fetch({ background: true })
     },
     [fetch],
   )
 
-  return { data, isLoading, error, refresh: fetch, add, update, remove }
+  return { data, isLoading, isRefreshing, error, refresh: fetch, add, update, remove }
 }

frontend/src/pages/Plan.test.tsx

@@ -1,5 +1,5 @@
 import { describe, it, expect, beforeEach, vi } from 'vitest'
-import { render, screen, fireEvent, waitFor } from '@testing-library/react'
+import { render, screen, fireEvent, waitFor, act } from '@testing-library/react'
 import { Plan } from './Plan'
 import * as hooks from '@/hooks'
 import { useWorkspace } from '@/context'
@@ -156,6 +156,7 @@ describe('Plan', () => {
     vi.mocked(hooks.useLeaveWishes).mockReturnValue({
       data: [mockWish],
       isLoading: false,
+      isRefreshing: false,
       error: null,
       refresh: vi.fn(),
       add: mockAddWish,
@@ -188,6 +189,42 @@ describe('Plan', () => {
     expect(screen.getByText(/Laddar planering/)).toBeInTheDocument()
   })
 
+  it('renders the loading spinner during the initial wishes load, before any data has arrived', () => {
+    vi.mocked(hooks.useLeaveWishes).mockReturnValue({
+      data: [],
+      isLoading: true,
+      isRefreshing: false,
+      error: null,
+      refresh: vi.fn(),
+      add: mockAddWish,
+      update: mockUpdateWish,
+      remove: mockRemoveWish,
+    } as any)
+    renderPlan()
+    expect(screen.getByText(/Laddar planering/)).toBeInTheDocument()
+    expect(screen.queryByTestId('wish-editor')).not.toBeInTheDocument()
+  })
+
+  it('does not show the full-page spinner or unmount the wish editor during a mutation background refresh', () => {
+    // useLeaveWishes flips isRefreshing (not isLoading) while it refetches
+    // after add/update/remove. Plan must keep rendering the wish editor
+    // (and whatever expanded/save-status state it holds) through that.
+    vi.mocked(hooks.useLeaveWishes).mockReturnValue({
+      data: [mockWish],
+      isLoading: false,
+      isRefreshing: true,
+      error: null,
+      refresh: vi.fn(),
+      add: mockAddWish,
+      update: mockUpdateWish,
+      remove: mockRemoveWish,
+    } as any)
+    renderPlan()
+    expect(screen.queryByText(/Laddar planering/)).not.toBeInTheDocument()
+    expect(screen.getByTestId('wish-editor')).toBeInTheDocument()
+    expect(screen.getByTestId('wish-card-lw1')).toBeInTheDocument()
+  })
+
   it('renders the page title and the wish editor', () => {
     renderPlan()
     expect(screen.getByRole('heading', { name: 'Planering', level: 1 })).toBeInTheDocument()
@@ -246,9 +283,20 @@ describe('Plan', () => {
   it('edits and removes a wish through the wish editor', async () => {
     renderPlan()
 
+    // WishEditor auto-saves edits after an 800ms debounce (no per-row Save
+    // button anymore) — advance fake timers to let it fire.
+    vi.useFakeTimers()
     fireEvent.change(screen.getByTestId('wish-weeks-lw1'), { target: { value: '8' } })
-    fireEvent.click(screen.getByTestId('wish-save-lw1'))
-    await waitFor(() => expect(mockUpdateWish).toHaveBeenCalledWith('lw1', expect.objectContaining({ targetWeeks: 8 })))
+    act(() => {
+      vi.advanceTimersByTime(800)
+    })
+    expect(mockUpdateWish).toHaveBeenCalledWith('lw1', expect.objectContaining({ targetWeeks: 8 }))
+    // Flush the microtask queue so Plan's `.finally(() => setIsSaving(false))`
+    // runs and re-enables the remove button before it's clicked below.
+    await act(async () => {
+      await Promise.resolve()
+    })
+    vi.useRealTimers()
 
     fireEvent.click(screen.getByTestId('wish-remove-lw1'))
     await waitFor(() => expect(mockRemoveWish).toHaveBeenCalledWith('lw1'))
@@ -263,6 +311,51 @@ describe('Plan', () => {
     await waitFor(() => expect(screen.getByTestId('wish-error')).toHaveTextContent('nätverksfel'))
   })
 
+  it('shows a generic error banner when adding a wish fails with a non-Error value', async () => {
+    mockAddWish.mockRejectedValue('boom')
+    renderPlan()
+
+    fireEvent.click(screen.getByTestId('wish-preset-sommar'))
+
+    await waitFor(() =>
+      expect(screen.getByTestId('wish-error')).toHaveTextContent('Kunde inte lägga till önskemål'),
+    )
+  })
+
+  it('shows a calculating label on the propose button while a proposal is in flight', () => {
+    vi.mocked(hooks.usePlanProposal).mockReturnValue({
+      data: null,
+      isLoading: true,
+      error: null,
+      propose: mockPropose,
+    } as any)
+    renderPlan()
+    expect(screen.getByTestId('propose-plan-button')).toHaveTextContent('Beräknar')
+  })
+
+  it('treats a missing workspace as having no workspace id: skips the wish editor and refuses to apply', async () => {
+    vi.mocked(useWorkspace).mockReturnValue({ ...mockWorkspaceContext, workspace: null } as any)
+    vi.mocked(hooks.usePlanProposal).mockReturnValue({
+      data: mockProposal,
+      isLoading: false,
+      error: null,
+      propose: mockPropose,
+    } as any)
+    renderPlan()
+
+    // workspace?.id ?? null falls back to null, so the wish editor (guarded
+    // on workspaceId) must not render even though wishes/members data exist.
+    expect(screen.queryByTestId('wish-editor')).not.toBeInTheDocument()
+
+    fireEvent.click(screen.getByTestId('plan-apply-button'))
+
+    await waitFor(() => {
+      // handleApplyPlan's `if (!workspaceId) throw` fires before ever
+      // reaching the real apply() call.
+      expect(mockApply).not.toHaveBeenCalled()
+    })
+  })
+
   it('shows an error banner when the proposal fails', () => {
     vi.mocked(hooks.usePlanProposal).mockReturnValue({
       data: null,