diff --git a/client/src/components/Tours/paddle/ObstacleDecisionPanel.test.tsx b/client/src/components/Tours/paddle/ObstacleDecisionPanel.test.tsx index ae264f8df..3fea251ae 100644 --- a/client/src/components/Tours/paddle/ObstacleDecisionPanel.test.tsx +++ b/client/src/components/Tours/paddle/ObstacleDecisionPanel.test.tsx @@ -7,7 +7,7 @@ import type { ObstacleDecision, ObstacleDecisionRecord, TourObstacleCheck, TourO import { buildSettings } from '../../../../tests/helpers/factories'; import { earlierEntry, obstacleCheck, obstacleEntry } from '../../../../tests/helpers/paddleCheck'; -import { fireEvent, render, screen, within } from '../../../../tests/helpers/render'; +import { fireEvent, render, screen, waitFor, within } from '../../../../tests/helpers/render'; import { seedStore } from '../../../../tests/helpers/store'; import { useSettingsStore } from '../../../store/settingsStore'; import { ChecklistObstaclePanel, ObstacleDecisionPanel } from './ObstacleDecisionPanel'; @@ -52,7 +52,7 @@ function decidedEntry(decision: ObstacleDecision = 'pass', note: string | null = type PanelProps = ComponentProps; const onDecide = vi.fn(); -const onNote = vi.fn(); +const onNote = vi.fn(() => Promise.resolve(true)); const onClear = vi.fn(); const onKeep = vi.fn(); /** Trip member 1 is Anna; anyone else has left the trip. */ @@ -98,6 +98,7 @@ function checklistOf( stale: false, dirty: false, canDecide: true, + canPlanPortage: true, savingKey: null, selectedKey: null, dialogKey: null, @@ -108,10 +109,10 @@ function checklistOf( nextOf: () => null, next: vi.fn(), planPortage: vi.fn(), - decide: vi.fn(() => Promise.resolve()), - setNote: vi.fn(() => Promise.resolve()), - clear: vi.fn(() => Promise.resolve()), - keep: vi.fn(() => Promise.resolve()), + decide: vi.fn(() => Promise.resolve(true)), + setNote: vi.fn(() => Promise.resolve(true)), + clear: vi.fn(() => Promise.resolve(true)), + keep: vi.fn(() => Promise.resolve(true)), memberName, ...overrides, }; @@ -166,7 +167,8 @@ describe('ObstacleDecisionPanel', () => { it('FE-PADDLE-DECIDE-003: in review the earlier choice keeps a dashed ring, nothing is checked, the reason shows', () => { renderPanel(obstacleEntry({ status: 'review', changed: ['route'], decision: record('pass') })); - const earlier = screen.getByRole('radio', { name: 'Paddle through' }); + // Named with its review state too, so it is told apart by more than its ring. + const earlier = screen.getByRole('radio', { name: 'Paddle through, Check again' }); expect(earlier).toHaveAttribute('aria-checked', 'false'); expect(earlier).toHaveClass('border-dashed'); expect(earlier).not.toHaveClass('bg-accent'); @@ -251,7 +253,7 @@ describe('ObstacleDecisionPanel', () => { expect(screen.getByTestId('obstacle-on-site')).toHaveTextContent('unknown'); }); - it('FE-PADDLE-DECIDE-009: a decision takes a note of two rows and at most 500 characters, saved trimmed', () => { + it('FE-PADDLE-DECIDE-009: a decision takes a note of two rows and at most 500 characters, saved trimmed', async () => { renderPanel(decidedEntry()); fireEvent.click(screen.getByRole('button', { name: 'Note' })); @@ -264,7 +266,18 @@ describe('ObstacleDecisionPanel', () => { fireEvent.click(screen.getByRole('button', { name: 'Save' })); expect(onNote).toHaveBeenCalledTimes(1); expect(onNote).toHaveBeenCalledWith('Links an der Bootsrutsche'); - expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + await waitFor(() => expect(screen.queryByRole('textbox')).not.toBeInTheDocument()); + }); + + it('FE-PADDLE-DECIDE-021: a note whose save failed stays in the editor with its text', async () => { + onNote.mockResolvedValueOnce(false); + renderPanel(decidedEntry()); + fireEvent.click(screen.getByRole('button', { name: 'Note' })); + fireEvent.change(screen.getByRole('textbox', { name: 'Note' }), { target: { value: 'Rechts umtragen' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + await waitFor(() => expect(onNote).toHaveBeenCalledWith('Rechts umtragen')); + await Promise.resolve(); + expect(screen.getByRole('textbox', { name: 'Note' })).toHaveValue('Rechts umtragen'); }); it('FE-PADDLE-DECIDE-010: a saved note reads clamped; Cancel and an unchanged save keep it, emptying it removes it', () => { @@ -385,20 +398,28 @@ describe('ObstacleDecisionPanel', () => { }); describe('ChecklistObstaclePanel', () => { - it('FE-PADDLE-DECIDE-017: shows nothing for an object the checklist does not list or while it is not ready', () => { + it('FE-PADDLE-DECIDE-017: shows nothing without a checklist or for an object it does not list, and stays while it reads again', () => { const { container, rerender } = render(); expect(container).toBeEmptyDOMElement(); - rerender( - - ); + rerender(); expect(container).toBeEmptyDOMElement(); + // Reading again keeps the panel open, locked, so a note being written is not lost. + rerender( + + ); + expect(screen.getByTestId('obstacle-decision-panel')).toBeInTheDocument(); + expect(screen.getByRole('radio', { name: 'Paddle through' })).toBeDisabled(); + rerender(); expect(screen.getByTestId('obstacle-decision-panel')).toBeInTheDocument(); }); - it('FE-PADDLE-DECIDE-018: decides, plans the portage, notes, removes and moves on through the checklist', () => { + it('FE-PADDLE-DECIDE-018: decides, plans the portage, notes, removes and moves on through the checklist', async () => { const entry = obstacleEntry({ status: 'todo', decision: record('portage') }); const following = obstacleEntry({ key: 'node/2', refs: ['node/2'], name: 'Wehr Pielenhofen', along_m: 3400 }); const checklist = checklistOf(obstacleCheck({ entries: [entry, following] }), { @@ -421,6 +442,37 @@ describe('ChecklistObstaclePanel', () => { fireEvent.click(screen.getByRole('button', { name: 'Next' })); expect(checklist.next).toHaveBeenCalledWith(entry); expect(screen.getByText(`Decided ${DECIDED_ON} by Anna`)).toBeInTheDocument(); + await waitFor(() => expect(screen.queryByRole('textbox')).not.toBeInTheDocument()); + }); + + it('FE-PADDLE-DECIDE-022: someone who may not edit the tour is not offered to plan the portage', () => { + const entry = obstacleEntry({ status: 'todo', decision: record('portage') }); + render( + + ); + expect(screen.queryByRole('button', { name: 'Plan the portage' })).not.toBeInTheDocument(); + }); + + it('FE-PADDLE-DECIDE-023: moving on keeps the focus in the panel when the next entry has no Next of its own', () => { + const entry = obstacleEntry({ status: 'todo', decision: record('portage') }); + const following = obstacleEntry({ key: 'node/2', refs: ['node/2'], name: 'Wehr Pielenhofen', along_m: 3400 }); + const check = obstacleCheck({ entries: [entry, following] }); + let featureRef = 'node/1'; + const checklist = checklistOf(check, { + nextOf: (item) => (item.key === entry.key ? following : null), + next: vi.fn(() => { + featureRef = 'node/2'; + }), + }); + const { rerender } = render(); + const next = screen.getByRole('button', { name: 'Next' }); + next.focus(); + fireEvent.click(next); + rerender(); + expect(screen.getByTestId('obstacle-decision-panel')).toHaveFocus(); }); it('FE-PADDLE-DECIDE-019: a decision about an object the data no longer holds is kept through the checklist', () => { diff --git a/client/src/components/Tours/paddle/ObstacleDecisionPanel.tsx b/client/src/components/Tours/paddle/ObstacleDecisionPanel.tsx index 517dcf5fe..f2cfa6281 100644 --- a/client/src/components/Tours/paddle/ObstacleDecisionPanel.tsx +++ b/client/src/components/Tours/paddle/ObstacleDecisionPanel.tsx @@ -18,7 +18,7 @@ import { Unlink, X, } from 'lucide-react'; -import { useState, type KeyboardEvent, type ReactNode } from 'react'; +import { useLayoutEffect, useRef, useState, type KeyboardEvent, type ReactNode } from 'react'; import { useTranslation } from '../../../i18n'; import { fs } from '../../shared/DialogShell'; import { Tooltip } from '../../shared/Tooltip'; @@ -104,6 +104,8 @@ function DecisionChoices({ > {CHOICE_ICON[choice]} {t(`tours.paddle.check.decision.${choice}`)} + {/* The earlier choice is told apart by more than its dashed ring. */} + {look === 'earlier' && {`, ${t('tours.paddle.check.status.review')}`}} ); })} @@ -236,15 +238,21 @@ function NoteBlock({ }: { note: string | null; disabled: boolean; - onNote: (note: string | null) => void; + /** Resolves to whether the note was saved; a failed save keeps the text being written. */ + onNote: (note: string | null) => Promise; }) { const { t } = useTranslation(); // The text being written, or null while the note is only read. const [draft, setDraft] = useState(null); const save = (text: string) => { const next = text.trim() || null; - if (next !== note) onNote(next); - setDraft(null); + if (next === note) { + setDraft(null); + return; + } + void onNote(next).then((saved) => { + if (saved) setDraft(null); + }); }; if (draft !== null) return ( @@ -364,7 +372,7 @@ export function ObstacleDecisionPanel({ saving: boolean; memberName: (id: number | null) => string | null; onDecide: (decision: ObstacleDecision) => void; - onNote: (note: string | null) => void; + onNote: (note: string | null) => Promise; onClear: () => void; onKeep: () => void; /** Moves on to the next entry still asking for a look; null when there is none. */ @@ -376,8 +384,28 @@ export function ObstacleDecisionPanel({ const locked = !canDecide || saving; const gone = entry.status === 'gone'; const label = entry.name ?? t(`tours.paddle.obstacle.kind.${entry.kind}`); + const root = useRef(null); + const movedOn = useRef(false); + // Moving on keeps the focus in the panel, also when the next entry has no Next button of its own. + useLayoutEffect(() => { + if (!movedOn.current) return; + movedOn.current = false; + if (!root.current?.contains(document.activeElement)) root.current?.focus({ preventScroll: true }); + }); + const next = onNext + ? () => { + movedOn.current = true; + onNext(); + } + : null; return ( -
+
@@ -391,14 +419,14 @@ export function ObstacleDecisionPanel({ value={entry.local_check ? formatCheckDate(entry.local_check, locale) : t('tours.paddle.check.unknown')} />
- {entry.decision && } + {entry.decision && }
); @@ -408,17 +436,16 @@ export function ObstacleDecisionPanel({ export function ChecklistEntryPanel({ checklist, entry }: { checklist: PaddleChecklist; entry: TourObstacleEntry }) { return ( void checklist.decide(entry, decision)} - onNote={(note) => void checklist.setNote(entry, note)} + onNote={(note) => checklist.setNote(entry, note)} onClear={() => void checklist.clear(entry)} onKeep={() => void checklist.keep(entry)} onNext={checklist.nextOf(entry) ? () => checklist.next(entry) : null} - onPlanPortage={() => checklist.planPortage(entry)} + onPlanPortage={checklist.canPlanPortage ? () => checklist.planPortage(entry) : null} /> ); } @@ -426,10 +453,11 @@ export function ChecklistEntryPanel({ checklist, entry }: { checklist: PaddleChe /** * The checklist's part of the map card of a paddle object (PAD-06): the * decision panel of the entry the object belongs to, below its facts, or - * nothing while the checklist is not ready or does not list the object. + * nothing while there is no checklist or it does not list the object. */ export function ChecklistObstaclePanel({ checklist, featureRef }: { checklist: PaddleChecklist; featureRef: string }) { - const entry = checklist.status === 'ready' ? checklist.entryForRef(featureRef) : null; + // While the checklist reads again it keeps showing, locked, so the panel open on an object stays open. + const entry = checklist.check ? checklist.entryForRef(featureRef) : null; if (!entry) return null; return (
diff --git a/client/src/components/Tours/paddle/ObstacleEntryDialog.test.tsx b/client/src/components/Tours/paddle/ObstacleEntryDialog.test.tsx index feff5ab6a..8e7c8b01b 100644 --- a/client/src/components/Tours/paddle/ObstacleEntryDialog.test.tsx +++ b/client/src/components/Tours/paddle/ObstacleEntryDialog.test.tsx @@ -29,6 +29,7 @@ function checklistOf( stale: false, dirty: false, canDecide: true, + canPlanPortage: true, savingKey: null, selectedKey: null, dialogKey: null, @@ -39,10 +40,10 @@ function checklistOf( nextOf: () => null, next: vi.fn(), planPortage: vi.fn(), - decide: vi.fn(() => Promise.resolve()), - setNote: vi.fn(() => Promise.resolve()), - clear: vi.fn(() => Promise.resolve()), - keep: vi.fn(() => Promise.resolve()), + decide: vi.fn(() => Promise.resolve(true)), + setNote: vi.fn(() => Promise.resolve(true)), + clear: vi.fn(() => Promise.resolve(true)), + keep: vi.fn(() => Promise.resolve(true)), memberName: (id) => (id === 1 ? 'Anna' : null), ...overrides, }; diff --git a/client/src/components/Tours/paddle/PaddleCheckCard.test.tsx b/client/src/components/Tours/paddle/PaddleCheckCard.test.tsx index a65770ecc..f5f60946b 100644 --- a/client/src/components/Tours/paddle/PaddleCheckCard.test.tsx +++ b/client/src/components/Tours/paddle/PaddleCheckCard.test.tsx @@ -110,10 +110,12 @@ const ADVISED_LOCK = ownPortage({ const BYPASSED_LOCK = ownPortage({ kind: 'lock', index: 5, from_m: 5600, to_m: 5600, covers: [], bypassed: true }); /** All of it out of route order, as the server may list it. */ const ROUTE = obstacleCheck({ entries: [NEAR_LOCK, WEIR, BANNED_WEIR], own: [ADVISED_LOCK, OWN_PORTAGE] }); -/** The weir, and an earlier decision about a sill the data no longer holds. */ -const GONE = earlierEntry(); +/** The weir, and an earlier decision about a sill the route no longer meets. */ +const GONE = earlierEntry({ status: 'off_route' }); const GONE_NAME = 'Alte Sohlschwelle'; const WITH_EARLIER = obstacleCheck({ entries: [WEIR, GONE] }); +/** The weir, and a decision about a sill the complete data no longer holds: it counts, so it stays in view. */ +const VANISHED = earlierEntry({ key: 'node/8', refs: ['node/8'], name: 'Verschwundenes Wehr' }); /** What usePaddleChecklist hands the card: a ready list matching the saved route, nothing selected, every action a spy. */ function checklistOf(overrides: Partial = {}): PaddleChecklist { @@ -125,6 +127,7 @@ function checklistOf(overrides: Partial = {}): PaddleChecklist stale: false, dirty: false, canDecide: true, + canPlanPortage: true, savingKey: null, selectedKey: null, dialogKey: null, @@ -132,10 +135,10 @@ function checklistOf(overrides: Partial = {}): PaddleChecklist select: vi.fn(), selectOwn: vi.fn(), closeDialog: vi.fn(), - decide: vi.fn(async () => undefined), - setNote: vi.fn(async () => undefined), - clear: vi.fn(async () => undefined), - keep: vi.fn(async () => undefined), + decide: vi.fn(async () => true), + setNote: vi.fn(async () => true), + clear: vi.fn(async () => true), + keep: vi.fn(async () => true), nextOf: vi.fn(() => null), next: vi.fn(), planPortage: vi.fn(), @@ -366,7 +369,7 @@ describe('PaddleCheckCard', () => { const earlier = screen.getByRole('button', { name: /^Alte Sohlschwelle/ }); // No longer on the route, so no place along it. expect(earlier).not.toHaveTextContent('km'); - expect(earlier).toHaveTextContent('status gone'); + expect(earlier).toHaveTextContent('status off_route'); fireEvent.click(earlier); expect(select).toHaveBeenCalledWith(GONE); }); @@ -389,6 +392,14 @@ describe('PaddleCheckCard', () => { expect(screen.queryByText(GONE_NAME)).not.toBeInTheDocument(); }); + it('FE-PADDLE-CHECKCARD-018: a decision about an object the data no longer holds stays among the rows, after the route', () => { + renderCard(checklistOf({ check: obstacleCheck({ entries: [VANISHED, WEIR] }) })); + const rows = rowButtons(); + expect(rows).toHaveLength(2); + expect(rows[1]).toHaveTextContent('Verschwundenes Wehr'); + expect(screen.queryByRole('button', { name: SAYS.earlier })).not.toBeInTheDocument(); + }); + it('FE-PADDLE-CHECKCARD-015: without rows it says nothing is known on covered data, and that there is none where none is', () => { const empty = obstacleCheck({ entries: [], own: [] }); const bench = paddleBench({ entries: 0, open: 0 }); diff --git a/client/src/components/Tours/paddle/PaddleCheckCard.tsx b/client/src/components/Tours/paddle/PaddleCheckCard.tsx index 118b0b30e..33a85776e 100644 --- a/client/src/components/Tours/paddle/PaddleCheckCard.tsx +++ b/client/src/components/Tours/paddle/PaddleCheckCard.tsx @@ -330,7 +330,8 @@ function EmptyLine({ summary }: { summary: PaddleCheck | null }) { /** The rows along the route, then the earlier decisions; dimmed, never hidden, while they may not match the route. */ function CheckList({ checklist, check }: { checklist: PaddleChecklist; check: TourObstacleCheck }) { const { rows, earlier } = useMemo(() => checklistRows(check), [check]); - const dimmed = checklist.dirty || checklist.stale; + // Dimmed while it may not match the route, and while the list it came from reads again. + const dimmed = checklist.dirty || checklist.stale || checklist.status === 'loading'; return (
{rows.length > 0 ? ( diff --git a/client/src/components/Tours/paddle/PaddleObstacleCard.tsx b/client/src/components/Tours/paddle/PaddleObstacleCard.tsx index d27156bc4..569797808 100644 --- a/client/src/components/Tours/paddle/PaddleObstacleCard.tsx +++ b/client/src/components/Tours/paddle/PaddleObstacleCard.tsx @@ -134,7 +134,7 @@ export default function PaddleObstacleCard({ const color = paddleKindCss(feature.kind); return (
diff --git a/client/src/components/Tours/paddle/paddleCheckModel.test.ts b/client/src/components/Tours/paddle/paddleCheckModel.test.ts index a23757563..ce146c7ac 100644 --- a/client/src/components/Tours/paddle/paddleCheckModel.test.ts +++ b/client/src/components/Tours/paddle/paddleCheckModel.test.ts @@ -188,7 +188,7 @@ describe('focusPointsOf', () => { }); describe('checklistRows', () => { - it('PCM-014: lists the obstacles and the own portages and locks in route order, the earlier decisions apart', () => { + it('PCM-014: lists the obstacles and the own portages and locks in route order, then the vanished ones, the earlier decisions apart', () => { const weir = obstacleEntry(); const rapids = obstacleEntry({ key: 'node/2', @@ -200,9 +200,10 @@ describe('checklistRows', () => { }); const dam = obstacleEntry({ key: 'node/3', refs: ['node/3'], kind: 'dam', along_m: 2600 }); const gone = earlierEntry(); + const offRoute = earlierEntry({ key: 'node/10', refs: ['node/10'], status: 'off_route' }); const portage = ownPortage(); const lock = ownPortage({ kind: 'lock', index: 2, from_m: 2000, to_m: 2000, covers: [] }); - const check = obstacleCheck({ entries: [dam, weir, gone, rapids], own: [lock, portage] }); + const check = obstacleCheck({ entries: [dam, weir, gone, offRoute, rapids], own: [lock, portage] }); expect(checklistRows(check)).toEqual({ rows: [ @@ -211,8 +212,9 @@ describe('checklistRows', () => { { type: 'obstacle', entry: weir }, { type: 'own', own: lock }, { type: 'obstacle', entry: dam }, + { type: 'obstacle', entry: gone }, ], - earlier: [gone], + earlier: [offRoute], }); }); @@ -261,3 +263,13 @@ describe('nextPendingEntry', () => { expect(nextPendingEntry(obstacleCheck({ entries: [first] }), first)).toBeNull(); }); }); + +describe('nextPendingEntry at one metre', () => { + it('PCM-019: two pending entries at the same metre follow each other in list order', () => { + const first = obstacleEntry({ key: 'node/1', along_m: 1200 }); + const second = obstacleEntry({ key: 'node/2', refs: ['node/2'], along_m: 1200 }); + const check = obstacleCheck({ entries: [first, second] }); + expect(nextPendingEntry(check, first)?.key).toBe('node/2'); + expect(nextPendingEntry(check, second)).toBeNull(); + }); +}); diff --git a/client/src/components/Tours/paddle/paddleCheckModel.ts b/client/src/components/Tours/paddle/paddleCheckModel.ts index 4a6064870..b1b0eadd0 100644 --- a/client/src/components/Tours/paddle/paddleCheckModel.ts +++ b/client/src/components/Tours/paddle/paddleCheckModel.ts @@ -89,27 +89,36 @@ export type ChecklistRow = { type: 'obstacle'; entry: TourObstacleEntry } | { ty /** * The rows of a checklist in route order, the paddler's own portages and - * locks among the obstacles at their metre, and apart from them the earlier - * decisions the route or the data no longer carries. + * locks among the obstacles at their metre, then the decisions about objects + * the complete data no longer holds (they count as to check again, so they + * stay in view). Apart from them the earlier decisions the route no longer + * meets or the data cannot tell. */ export function checklistRows(check: TourObstacleCheck): { rows: ChecklistRow[]; earlier: TourObstacleEntry[] } { const metreOf = (row: ChecklistRow) => (row.type === 'obstacle' ? (row.entry.along_m ?? 0) : row.own.from_m); - const rows: ChecklistRow[] = [ + const onRoute: ChecklistRow[] = [ ...check.entries.filter((entry) => entry.relation !== null).map((entry) => ({ type: 'obstacle' as const, entry })), ...check.own.map((own) => ({ type: 'own' as const, own })), ].sort((a, b) => metreOf(a) - metreOf(b)); - return { rows, earlier: check.entries.filter((entry) => entry.relation === null) }; + const earlier = check.entries.filter((entry) => entry.relation === null); + const gone = earlier.filter((entry) => entry.status === 'gone'); + return { + rows: [...onRoute, ...gone.map((entry) => ({ type: 'obstacle' as const, entry }))], + earlier: earlier.filter((entry) => entry.status !== 'gone'), + }; } const PENDING = new Set(['open', 'todo', 'review']); -/** The next entry along the route still asking for a look; from an earlier decision, the first one. */ +/** + * The next entry still asking for a look, in the order the checklist lists + * them (route order, entries at one metre in list order); from an earlier + * decision, the first one. + */ export function nextPendingEntry(check: TourObstacleCheck, entry: TourObstacleEntry): TourObstacleEntry | null { - const from = entry.along_m ?? -1; - return ( - check.entries - .filter((candidate) => candidate.relation !== null && PENDING.has(candidate.status)) - .filter((candidate) => candidate.key !== entry.key && (candidate.along_m ?? 0) > from) - .sort((a, b) => (a.along_m ?? 0) - (b.along_m ?? 0))[0] ?? null - ); + const current = check.entries + .filter((candidate) => candidate.relation !== null) + .sort((a, b) => (a.along_m ?? 0) - (b.along_m ?? 0)); + const from = current.findIndex((candidate) => candidate.key === entry.key); + return current.slice(from + 1).find((candidate) => PENDING.has(candidate.status)) ?? null; } diff --git a/client/src/components/Tours/paddle/usePaddleChecklist.test.tsx b/client/src/components/Tours/paddle/usePaddleChecklist.test.tsx index 67be4ff4d..94fd7cf66 100644 --- a/client/src/components/Tours/paddle/usePaddleChecklist.test.tsx +++ b/client/src/components/Tours/paddle/usePaddleChecklist.test.tsx @@ -1,5 +1,5 @@ /** - * UPC-001 to UPC-023: the obstacle checklist of the paddle tour open in the + * UPC-001 to UPC-030: the obstacle checklist of the paddle tour open in the * tours workspace (PAD-06, PAD-09), as its card and the map share it. * * Pinned: off for anything but an open paddle tour in an active workspace, @@ -219,7 +219,7 @@ beforeEach(() => { describe('usePaddleChecklist: what it reads', () => { it('UPC-001: is off for a hike, a neutral planner and a new tour that is no paddle tour, and reads nothing', () => { const hike = paddleTour({ tour_type: 'hike', boat_type: null, water_context: null, paddle_check: null }); - const { result, rerender } = renderChecklist(options({ tours: [hike] })); + const { result, rerender } = renderChecklist(options({ tours: [hike], planner: planner({ isPaddle: false }) })); expect(result.current).toMatchObject({ status: 'off', tour: null, check: null, summary: null, canDecide: false }); rerender(options({ planner: planner({ mode: { type: 'neutral' } }) })); @@ -375,7 +375,7 @@ describe('usePaddleChecklist: deciding', () => { const onSaved = vi.fn<(tour: TourListItem) => void>(); const { result, rerender } = await renderReady(options({ onSaved })); - let saving: Promise = Promise.resolve(); + let saving: Promise = Promise.resolve(true); act(() => { saving = result.current.decide(ENTRY, 'pass'); }); @@ -436,7 +436,7 @@ describe('usePaddleChecklist: deciding', () => { const onSaved = vi.fn<(tour: TourListItem) => void>(); const { result } = await renderReady(options({ onSaved })); - let saving: Promise = Promise.resolve(); + let saving: Promise = Promise.resolve(true); act(() => { saving = result.current.decide(ENTRY, 'portage'); }); @@ -662,3 +662,102 @@ describe('usePaddleChecklist: the map and the route', () => { expect(setSelectedWaypointId).toHaveBeenCalledTimes(1); }); }); + +describe('usePaddleChecklist after review', () => { + const answer = (check: typeof CHECK, revision: string, place = 7): TourDetailResponse => ({ + tour: paddleTour({ place_id: place, paddle_check: paddleBench({ revision }) }), + waypoints: [], + obstacle_check: check, + }); + + it('UPC-024: a save answered after another tour opened leaves that tour as it read itself', async () => { + const other = paddleTour({ place_id: 8, paddle_check: paddleBench({ revision: 'rev-8' }) }); + const save = deferred(); + repo.saveObstacleReviews.mockReturnValueOnce(save.promise); + const props = options({ tours: [TOUR, other] }); + const { result, rerender } = await renderReady(props); + act(() => { + void result.current.decide(ENTRY, 'pass'); + }); + const otherCheck = obstacleCheck({ + revision: 'rev-8', + entries: [obstacleEntry({ key: 'node/8', refs: ['node/8'] })], + }); + repo.obstacleCheck.mockResolvedValue({ check: otherCheck, stale: false }); + rerender({ ...props, planner: planner({ mode: { type: 'edit-saved', placeId: 8 } }) }); + await waitFor(() => expect(result.current.check?.entries[0]?.key).toBe('node/8')); + expect(result.current.savingKey).toBeNull(); + + await act(async () => { + save.resolve( + answer(obstacleCheck({ revision: 'rev-2', entries: [obstacleEntry({ status: 'decided' })] }), 'rev-2') + ); + await save.promise; + }); + expect(result.current.status).toBe('ready'); + expect(result.current.check?.entries[0]?.key).toBe('node/8'); + expect(props.onSaved).toHaveBeenCalledWith(expect.objectContaining({ place_id: 7 })); + }); + + it('UPC-025: a save whose checklist is of another revision than its bench stays marked stale', async () => { + const { result, rerender } = await renderReady(); + repo.saveObstacleReviews.mockResolvedValueOnce(answer(obstacleCheck({ revision: 'rev-1' }), 'local:u1')); + await act(async () => { + await result.current.decide(ENTRY, 'pass'); + }); + rerender(options({ tours: [paddleTour({ paddle_check: paddleBench({ revision: 'local:u1' }) })] })); + expect(result.current.stale).toBe(true); + }); + + it('UPC-026: a read that finds a newer bench on the server hands the tour on and is not stale', async () => { + const newer = paddleTour({ paddle_check: paddleBench({ revision: 'rev-9' }) }); + repo.obstacleCheck.mockResolvedValue({ check: obstacleCheck({ revision: 'rev-9' }), stale: true, tour: newer }); + const props = options({ tours: [paddleTour({ paddle_check: paddleBench({ revision: 'local:u1' }) })] }); + const { result, rerender } = renderChecklist(props); + await waitFor(() => expect(props.onSaved).toHaveBeenCalledWith(newer)); + rerender({ ...props, tours: [newer] }); + expect(result.current).toMatchObject({ status: 'ready', stale: false }); + }); + + it('UPC-027: while the same tour reads again its last checklist stays, locked', async () => { + const { result, rerender } = await renderReady(); + const again = deferred(); + repo.obstacleCheck.mockReturnValueOnce(again.promise); + rerender(options({ tours: [paddleTour({ paddle_check: paddleBench({ revision: 'rev-3' }) })] })); + expect(result.current.status).toBe('loading'); + expect(result.current.check).toEqual(CHECK); + expect(result.current.canDecide).toBe(false); + await act(async () => { + again.resolve({ check: obstacleCheck({ revision: 'rev-3' }), stale: false }); + await again.promise; + }); + expect(result.current.status).toBe('ready'); + }); + + it('UPC-028: a saved tour turned into a paddle tour and not saved yet asks to be saved first', () => { + const hike = paddleTour({ tour_type: 'hike', boat_type: null, water_context: null, paddle_check: null }); + const { result } = renderChecklist(options({ tours: [hike], planner: planner({ isPaddle: true }) })); + expect(result.current).toMatchObject({ status: 'new', tour: null, canDecide: false }); + expect(repo.obstacleCheck).not.toHaveBeenCalled(); + }); + + it('UPC-029: planning a portage is offered to editors only', async () => { + expect((await renderReady()).result.current.canPlanPortage).toBe(true); + expect((await renderReady(options({ canEdit: false }))).result.current.canPlanPortage).toBe(false); + }); + + it('UPC-030: a note whose save failed answers false, one that went through true', async () => { + const { result } = await renderReady(); + repo.saveObstacleReviews.mockRejectedValueOnce(new Error('offline')); + let saved: boolean | undefined; + await act(async () => { + saved = await result.current.setNote(ENTRY, 'Links'); + }); + expect(saved).toBe(false); + repo.saveObstacleReviews.mockResolvedValueOnce(answer(CHECK, 'rev-1')); + await act(async () => { + saved = await result.current.setNote(ENTRY, 'Links'); + }); + expect(saved).toBe(true); + }); +}); diff --git a/client/src/components/Tours/paddle/usePaddleChecklist.ts b/client/src/components/Tours/paddle/usePaddleChecklist.ts index 1cc9c44a7..04c8e3249 100644 --- a/client/src/components/Tours/paddle/usePaddleChecklist.ts +++ b/client/src/components/Tours/paddle/usePaddleChecklist.ts @@ -10,7 +10,7 @@ import { type TourOwnEntry, type TripMember, } from '@trek/shared'; -import { useCallback, useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useEffectEvent, useMemo, useState } from 'react'; import { useNetworkMode } from '../../../hooks/useNetworkMode'; import { useTranslation } from '../../../i18n'; import { applyObstacleOps } from '../../../repo/obstacleCheckLocal'; @@ -43,6 +43,7 @@ interface PaddleChecklistOptions { } interface Loaded { + placeId: number; key: string; check: TourObstacleCheck | null; stale: boolean; @@ -57,27 +58,59 @@ function openPaddleTour(mode: TourPlannerController['mode'], tours: readonly Tou } /** Reads the checklist for the bench the list shows, again whenever its revision or the connection changes. */ -function useChecklistRead(tripId: number, tour: TourListItem | null, active: boolean) { +/** The read key of a tour's checklist: a new bench revision or a change of the connection reads it again. */ +const readKey = (placeId: number | null, revision: string | null, offline: boolean) => + `${placeId}:${revision}:${offline}`; + +/** + * Reads the checklist for the bench the list shows, again whenever its + * revision or the connection changes. A read that finds the server further + * than the list (a queued write replayed meanwhile) hands the newer tour on, + * so the list and its bench catch up. + */ +function useChecklistRead( + tripId: number, + tour: TourListItem | null, + active: boolean, + onNewer: (tour: TourListItem) => void +) { const { offline } = useNetworkMode(); const placeId = tour?.place_id ?? null; const revision = tour?.paddle_check?.revision ?? null; - const key = `${placeId}:${revision}:${offline}`; + const key = readKey(placeId, revision, offline); const [loaded, setLoaded] = useState(null); + // Handing the newer tour on is no reason to read again. + const takeNewer = useEffectEvent(onNewer); useEffect(() => { if (!active || placeId === null) return; const controller = new AbortController(); tourRepo .obstacleCheck(tripId, placeId, revision, controller.signal) - .then(({ check, stale }) => { - if (!controller.signal.aborted) setLoaded({ key, check, stale, failed: false }); + .then(({ check, stale, tour: answered }) => { + if (controller.signal.aborted) return; + const newer = answered?.paddle_check?.revision; + if (answered && newer && newer !== revision) { + setLoaded({ + placeId, + key: readKey(placeId, newer, offline), + check, + stale: check?.revision !== newer, + failed: false, + }); + takeNewer(answered); + return; + } + setLoaded({ placeId, key, check, stale, failed: false }); }) .catch(() => { - if (!controller.signal.aborted) setLoaded({ key, check: null, stale: false, failed: true }); + if (!controller.signal.aborted) setLoaded({ placeId, key, check: null, stale: false, failed: true }); }); return () => controller.abort(); - }, [active, key, placeId, revision, tripId]); + }, [active, key, offline, placeId, revision, tripId]); const current = loaded?.key === key ? loaded : null; - return { current, offline, key, setLoaded }; + // While the same tour reads again, what it read before stays in view, locked, so a panel open on it stays open. + const previous = !current && loaded?.placeId === placeId && loaded.check ? loaded : null; + return { current, previous, offline, key, setLoaded }; } /** Where a portage for an obstacle starts on a planned tour: the control point whose leg passes it. */ @@ -194,14 +227,16 @@ function useChecklistNavigation( export function usePaddleChecklist(options: PaddleChecklistOptions) { const { tripId, planner, tours, canEdit, active, trackPaddlePlan } = options; const tour = useMemo(() => openPaddleTour(planner.mode, tours), [planner.mode, tours]); - const read = useChecklistRead(tripId, tour, active); + const read = useChecklistRead(tripId, tour, active, options.onSaved); const writes = useChecklistWrites(options, tour, read); - const check = writes.optimistic ?? read.current?.check ?? null; + const check = writes.optimistic ?? read.current?.check ?? read.previous?.check ?? null; const navigation = useChecklistNavigation(planner, check, trackPaddlePlan); const dirty = planner.hasUnsavedChanges || !!trackPaddlePlan?.dirty; const status = checklistStatus({ active, - newPaddleDraft: planner.mode.type === 'new-draft' && planner.isPaddle, + // A new paddle draft, or a saved tour turned into one and not saved yet: no server check exists for either. + newPaddleDraft: + planner.isPaddle && (planner.mode.type === 'new-draft' || (planner.mode.type === 'edit-saved' && !tour)), tour, current: read.current, offline: read.offline, @@ -216,6 +251,8 @@ export function usePaddleChecklist(options: PaddleChecklistOptions) { stale: !!read.current?.stale, dirty, canDecide: canEdit && status === 'ready' && !dirty && saved && writes.savingKey === null, + /** Planning a portage edits the tour, so only someone who may edit it is offered it. */ + canPlanPortage: canEdit, savingKey: writes.savingKey, ...navigation, ...writes.actions, @@ -235,30 +272,38 @@ function useChecklistWrites( const { t } = useTranslation(); const toast = useToast(); const userId = useAuthStore((state) => state.user?.id ?? null); - const [pending, setPending] = useState<{ key: string; check: TourObstacleCheck } | null>(null); + const [pending, setPending] = useState<{ placeId: number; key: string; check: TourObstacleCheck } | null>(null); const base = read.current?.check ?? null; const placeId = tour?.place_id ?? null; const { setLoaded, offline } = read; const save = useCallback( async (key: string, op: ObstacleReviewOp) => { - if (!base || placeId === null) return; + if (!base || placeId === null) return false; setPending({ + placeId, key, check: applyObstacleOps(base, [op], { now: new Date().toISOString(), userId, revision: 'pending' }), }); try { const result = await tourRepo.saveObstacleReviews(tripId, placeId, { ops: [op] }, userId); const revision = result.tour.paddle_check?.revision ?? null; - setLoaded({ - key: `${placeId}:${revision}:${offline}`, - check: result.obstacle_check ?? base, - stale: false, + const check = result.obstacle_check ?? base; + const next: Loaded = { + placeId, + key: readKey(placeId, revision, offline), + check, + // A checklist still judged on an older route than the bench stays marked as such. + stale: revision !== null && check.revision !== revision, failed: false, - }); + }; + // Another tour opened meanwhile keeps what it read; this one's answer reaches the list and the cache. + setLoaded((current) => (current && !current.key.startsWith(`${placeId}:`) ? current : next)); onSaved(result.tour); + return true; } catch { toast.error(t('tours.paddle.marks.failed')); + return false; } finally { setPending(null); } @@ -285,5 +330,7 @@ function useChecklistWrites( }), [save] ); - return { optimistic: pending?.check ?? null, savingKey: pending?.key ?? null, actions }; + // A save still running for a tour left meanwhile shows nothing on the one open now. + const mine = pending && pending.placeId === placeId ? pending : null; + return { optimistic: mine?.check ?? null, savingKey: mine?.key ?? null, actions }; } diff --git a/client/src/components/shared/Tooltip.test.tsx b/client/src/components/shared/Tooltip.test.tsx index 9522bda3c..16b19954b 100644 --- a/client/src/components/shared/Tooltip.test.tsx +++ b/client/src/components/shared/Tooltip.test.tsx @@ -1,4 +1,4 @@ -// FE-W4TIP-001 to FE-W4TIP-014 +// FE-W4TIP-001 to FE-W4TIP-015 import { createRef, useRef } from 'react' import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import { render, screen, fireEvent, act } from '../../../tests/helpers/render' @@ -181,4 +181,16 @@ describe('Tooltip — inside a component that owns the ref', () => { expect(screen.getByRole('button')).toHaveAttribute('data-clicked', '1') }) + + it('FE-W4TIP-015: a label of several lines breaks at them and stays within the window, a single line does not wrap', () => { + const { unmount } = render() + hover(screen.getByRole('button')) + expect(screen.getByRole('tooltip')).toHaveStyle({ whiteSpace: 'pre-line' }) + expect(screen.getByRole('tooltip').style.maxWidth).toContain('100vw') + unmount() + + render() + hover(screen.getByRole('button')) + expect(screen.getByRole('tooltip')).toHaveStyle({ whiteSpace: 'nowrap' }) + }) }) diff --git a/client/src/components/shared/Tooltip.tsx b/client/src/components/shared/Tooltip.tsx index 83b51dfa4..2b44c50db 100644 --- a/client/src/components/shared/Tooltip.tsx +++ b/client/src/components/shared/Tooltip.tsx @@ -92,7 +92,9 @@ export function Tooltip({ label, placement = 'bottom', delay = 250, disabled, ch fontWeight: 500, padding: '5px 10px', borderRadius: 8, - whiteSpace: 'nowrap', + // A label of several lines (the paddle check bench) breaks at its lines and wraps within the window. + whiteSpace: label.includes('\n') ? 'pre-line' : 'nowrap', + maxWidth: label.includes('\n') ? 'min(22rem, calc(100vw - 12px))' : undefined, boxShadow: '0 4px 12px rgba(0,0,0,0.15)', fontFamily: "var(--font-system)", transformOrigin: placement === 'top' ? 'bottom center' : placement === 'bottom' ? 'top center' : placement === 'left' ? 'center right' : 'center left', diff --git a/client/src/pages/TripPlannerPage.wiring.test.tsx b/client/src/pages/TripPlannerPage.wiring.test.tsx index 4be95bf13..1b94a716e 100644 --- a/client/src/pages/TripPlannerPage.wiring.test.tsx +++ b/client/src/pages/TripPlannerPage.wiring.test.tsx @@ -308,7 +308,8 @@ describe('TripPlannerPage — shell', () => { expect(await screen.findByText('Plan a tour')).toBeInTheDocument() expect(screen.getByRole('button', { name: 'Plan new tour' })).toBeInTheDocument() expect(screen.queryByLabelText('Tour name')).not.toBeInTheDocument() - expect(screen.getByRole('option', { name: /Editable ridge tour/i })).not.toHaveAttribute('aria-selected', 'true') + // The tours list fills in after the rail: wait for its row instead of reading it at once. + expect(await screen.findByRole('option', { name: /Editable ridge tour/i })).not.toHaveAttribute('aria-selected', 'true') expect(plannerActions.openTour).not.toHaveBeenCalled() }) diff --git a/client/src/repo/tourRepo.obstacles.test.ts b/client/src/repo/tourRepo.obstacles.test.ts index 88017ffcb..7443632d7 100644 --- a/client/src/repo/tourRepo.obstacles.test.ts +++ b/client/src/repo/tourRepo.obstacles.test.ts @@ -1,4 +1,4 @@ -// FE-REPO-TOUR-030 to FE-REPO-TOUR-038 +// FE-REPO-TOUR-030 to FE-REPO-TOUR-040 // A paddle tour's checklist through the offline core (PAD-06, PAD-09): read // from the cache while its revision matches the bench, decisions saved online // and cached, saved offline under a local revision and queued, and a route @@ -92,14 +92,14 @@ describe('tourRepo.obstacleCheck', () => { }) ) ); - expect(await tourRepo.obstacleCheck(3, 40, 'rev-2')).toEqual({ check: fresh, stale: false }); + expect(await tourRepo.obstacleCheck(3, 40, 'rev-2')).toMatchObject({ check: fresh, stale: false }); expect((await offlineDb.tours.get(40))?.obstacle_check).toEqual(fresh); }); it('FE-REPO-TOUR-032: offline, an older cached checklist stands in and reads as stale', async () => { setOnline(false); await offlineDb.tours.put({ ...paddleTour(), trip_id: 3, waypoints, obstacle_check: obstacleCheck() }); - expect(await tourRepo.obstacleCheck(3, 40, 'local:abc')).toEqual({ check: obstacleCheck(), stale: true }); + expect(await tourRepo.obstacleCheck(3, 40, 'local:abc')).toMatchObject({ check: obstacleCheck(), stale: true }); }); }); @@ -204,3 +204,34 @@ describe('a route written offline and the list cache', () => { expect(tours[0]).not.toHaveProperty('obstacle_check'); }); }); + +describe('after review', () => { + it('FE-REPO-TOUR-039: a decision queued on a checklist already stale keeps it stale against the bench', async () => { + setOnline(false); + vi.spyOn(mutationQueue, 'sendSoon').mockImplementation(() => undefined); + await offlineDb.tours.put({ + ...paddleTour({ paddle_check: paddleBench({ revision: 'local:route' }) }), + trip_id: 3, + waypoints, + obstacle_check: obstacleCheck(), + }); + const result = await tourRepo.saveObstacleReviews(3, 40, decide, 4); + expect(result.tour.paddle_check?.revision).toBe('local:route'); + expect(result.obstacle_check?.revision).not.toBe('local:route'); + expect((await tourRepo.obstacleCheck(3, 40, 'local:route')).stale).toBe(true); + }); + + it('FE-REPO-TOUR-040: online, a bench of a write made offline waits for the queue, then reads the server and hands its tour on', async () => { + await offlineDb.tours.put({ ...paddleTour(), trip_id: 3, waypoints, obstacle_check: obstacleCheck() }); + const flush = vi.spyOn(mutationQueue, 'flush').mockResolvedValue(); + const server_ = paddleTour({ paddle_check: paddleBench({ revision: 'rev-5' }) }); + server.use( + http.get('/api/trips/3/tours/40', () => + HttpResponse.json({ tour: server_, waypoints, obstacle_check: obstacleCheck({ revision: 'rev-5' }) }) + ) + ); + const read = await tourRepo.obstacleCheck(3, 40, 'local:abc'); + expect(flush).toHaveBeenCalled(); + expect(read).toMatchObject({ check: { revision: 'rev-5' }, tour: { paddle_check: { revision: 'rev-5' } } }); + }); +}); diff --git a/client/src/repo/tourRepo.ts b/client/src/repo/tourRepo.ts index bf486c3e9..1b6bb8661 100644 --- a/client/src/repo/tourRepo.ts +++ b/client/src/repo/tourRepo.ts @@ -268,18 +268,22 @@ export const tourRepo = { * PAD-06: a paddle tour's checklist for the bench the list shows. The * cached one serves while its revision matches; otherwise the detail is read * (the cache standing in offline), and a checklist of another revision than - * the bench's reads as stale. + * the bench's reads as stale. A bench of a write made here offline waits for + * the queue to replay once online, and the tour read with the detail comes + * back with it, so the list can take in the server's bench. */ async obstacleCheck( tripId: number | string, placeId: number, revision: string | null, signal?: AbortSignal, - ): Promise<{ check: TourObstacleCheck | null; stale: boolean }> { + ): Promise<{ check: TourObstacleCheck | null; stale: boolean; tour?: TourListItem }> { + if (revision?.startsWith('local:') && !isEffectivelyOffline()) await mutationQueue.flush() const cached = await offlineDb.tours.get(placeId) if (revision !== null && cached?.obstacle_check?.revision === revision) return { check: cached.obstacle_check, stale: false } - const check = (await tourRepo.detail(tripId, placeId, signal)).obstacle_check ?? null - return { check, stale: check !== null && revision !== null && check.revision !== revision } + const detail = await tourRepo.detail(tripId, placeId, signal) + const check = detail.obstacle_check ?? null + return { check, stale: check !== null && revision !== null && check.revision !== revision, tour: detail.tour } }, /** @@ -300,9 +304,14 @@ export const tourRepo = { if (!existing?.obstacle_check) throw new Error('Checklist not available offline') const id = generateUUID() const revision = `local:${id}` + // A checklist still judged on an older route than the bench keeps that gap, so it stays marked stale. + const routeStale = !!existing.paddle_check && existing.paddle_check.revision !== existing.obstacle_check.revision const check = applyObstacleOps(existing.obstacle_check, body.ops, { now: new Date().toISOString(), userId, revision }) const bench = existing.paddle_check - ? { ...paddleCheckSummaryOf(check.entries, check.own, existing.paddle_check), revision } + ? { + ...paddleCheckSummaryOf(check.entries, check.own, existing.paddle_check), + revision: routeStale ? existing.paddle_check.revision : revision, + } : existing.paddle_check const tour: CachedTour = { ...existing, obstacle_check: check, paddle_check: bench } await offlineDb.tours.put(tour)