From 8869ea8e55766fd621d612e869efb7aa5ae7c66c Mon Sep 17 00:00:00 2001 From: Jacob Hagstedt Date: Mon, 14 Sep 2026 18:54:50 +0200 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01RXMhc9nT6sNz1SL4j8dznL --- frontend/src/hooks/useLeaveWishes.test.ts | 87 +++++++++++++++++++- frontend/src/hooks/useLeaveWishes.ts | 58 ++++++++----- frontend/src/pages/Plan.test.tsx | 99 ++++++++++++++++++++++- 3 files changed, 222 insertions(+), 22 deletions(-) diff --git a/frontend/src/hooks/useLeaveWishes.test.ts b/frontend/src/hooks/useLeaveWishes.test.ts index 1586df5..e0931c5 100644 --- a/frontend/src/hooks/useLeaveWishes.test.ts +++ b/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 = 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) + }) }) diff --git a/frontend/src/hooks/useLeaveWishes.ts b/frontend/src/hooks/useLeaveWishes.ts index a5f02cc..074b921 100644 --- a/frontend/src/hooks/useLeaveWishes.ts +++ b/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 add: (req: LeaveWishCreateRequest) => Promise @@ -12,25 +19,40 @@ interface UseLeaveWishesResult { remove: (id: string) => Promise } -/** 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([]) const [isLoading, setIsLoading] = useState(false) + const [isRefreshing, setIsRefreshing] = useState(false) const [error, setError] = useState(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 } } diff --git a/frontend/src/pages/Plan.test.tsx b/frontend/src/pages/Plan.test.tsx index 2e16f92..3478ad3 100644 --- a/frontend/src/pages/Plan.test.tsx +++ b/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,