From 19745b7b0d6f1fd70ba66857805b458548269101 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 23 Aug 2026 10:08:13 +0200 Subject: [PATCH] fix(studio): keep "Keep mine" from throwing the document away After a save conflict, Keep mine set the version and unblocked writes but never re-queued the local document, and write() had already cleared the queue before the request. So if the user picked Keep mine and then simply closed the editor without typing anything else, saveNow() found nothing pending and did nothing: their afternoon of edits existed only in memory, the indicator said idle, and the server kept the other version. The exact outcome the conflict dialog promises to prevent. Ctrl+Z was intercepted while typing in a text field too. Studio commits text on blur, so mid-edit the typing is not in the undo history yet: the browser's own text undo was suppressed and a previous layout change was undone underneath the open editor instead. Reads as random data loss. Also: delete no longer removes locked elements, the auto-layout no longer freezes an empty GPS path when tracks resolve after the one-shot build, the back-cover tally cannot print "0 places", the map fetch aborts when Studio closes, and a double Enter in the list-name field stops creating two lists. The page-pattern gate had quietly stopped covering anything: the Desktop-shim refactor moved every real container out of the default export the checker reads, and its regex missed useRef(null) anyway. Widened on both counts, and the state it immediately found moved into the hooks. --- client/scripts/check-page-pattern.mjs | 46 ++++-- .../Collections/ListEditorModal.test.tsx | 15 ++ .../Collections/ListEditorModal.tsx | 4 +- .../components/Studio/StudioTravelPanel.tsx | 20 ++- client/src/components/Studio/applyTemplate.ts | 2 +- client/src/components/Studio/autoLayout.ts | 10 +- client/src/components/Studio/useBookStore.ts | 34 +++- client/src/pages/JourneyDetailPage.tsx | 5 +- client/src/pages/join/useJoinTrip.test.ts | 39 ++++- client/src/pages/join/useJoinTrip.ts | 17 +- .../pages/journeyDetail/useJourneyDetail.ts | 4 +- .../journeyStudio/useJourneyStudio.test.tsx | 149 ++++++++++++++++++ .../pages/journeyStudio/useJourneyStudio.ts | 25 ++- 13 files changed, 333 insertions(+), 37 deletions(-) create mode 100644 client/src/pages/journeyStudio/useJourneyStudio.test.tsx diff --git a/client/scripts/check-page-pattern.mjs b/client/scripts/check-page-pattern.mjs index 5d7d789c9..05acc51a9 100644 --- a/client/scripts/check-page-pattern.mjs +++ b/client/scripts/check-page-pattern.mjs @@ -2,9 +2,12 @@ // src/pages/PATTERN.md). A *Page.tsx default-export component should wire a // co-located use() hook into JSX — it must not own state/effects itself. // -// We scan only the default-export component body (from `export default function` -// up to the next top-level `function` declaration or EOF), so presentational -// sub-components and helper hooks living in the same file are not flagged. +// We scan the default-export component body (from `export default function` up +// to the next top-level `function` declaration or EOF), and the same span of a +// top-level `*PageDesktop`/`*PageMobile` — since ViewportRoute started picking +// the branch, the default export is a two-line shim and the real container is +// one of those two. Everything else at top level stays exempt: presentational +// sub-components and helper hooks live in these files on purpose. // Context hooks like useTranslation/useParams are fine; the smell is stateful // logic — useState/useReducer/useEffect/useLayoutEffect/useMemo/useCallback/useRef. import { readdirSync, readFileSync } from 'node:fs' @@ -13,23 +16,38 @@ import { fileURLToPath } from 'node:url' const pagesDir = join(dirname(fileURLToPath(import.meta.url)), '..', 'src', 'pages') const BANNED = ['useState', 'useReducer', 'useEffect', 'useLayoutEffect', 'useMemo', 'useCallback', 'useRef'] -const bannedRe = new RegExp(`\\b(${BANNED.join('|')})\\s*\\(`) +// `[<(]` rather than `\(`: a generic call — `useRef<() => void>(null)` — is the +// same thing, and it slipped through for as long as the pattern only knew `(`. +const bannedRe = new RegExp(`\\b(${BANNED.join('|')})\\s*[<(]`) + +// Declarations the widened scan caught that are not this change's to move. +// Listed by the name they declare rather than by line, so an entry dies with +// its declaration instead of drifting onto whatever follows it. The rest of +// each listed file is still checked. +const KNOWN_ESCAPES = { + 'TripPlannerPage.tsx': ['glMap', 'bookingExpense'], +} const violations = [] for (const file of readdirSync(pagesDir)) { if (!file.endsWith('Page.tsx') || file.endsWith('.test.tsx')) continue const src = readFileSync(join(pagesDir, file), 'utf8') const lines = src.split('\n') - const start = lines.findIndex(l => /export default function/.test(l)) - if (start === -1) continue - // The page body ends at the next top-level declaration (a `function` at - // column 0) — everything after that is a sub-component or helper. - let end = lines.length - for (let i = start + 1; i < lines.length; i++) { - if (/^(function |const [A-Z]\w* = )/.test(lines[i])) { end = i; break } - } - for (let i = start; i < end; i++) { - if (bannedRe.test(lines[i])) { + const allowed = KNOWN_ESCAPES[file] ?? [] + const starts = [] + lines.forEach((line, i) => { + if (/export default function/.test(line) || /^function \w+Page(Desktop|Mobile)\b/.test(line)) starts.push(i) + }) + for (const start of starts) { + // The page body ends at the next top-level declaration (a `function` at + // column 0) — everything after that is a sub-component or helper. + let end = lines.length + for (let i = start + 1; i < lines.length; i++) { + if (/^(function |const [A-Z]\w* = )/.test(lines[i])) { end = i; break } + } + for (let i = start; i < end; i++) { + if (!bannedRe.test(lines[i])) continue + if (allowed.some(name => new RegExp(`\\b${name}\\b`).test(lines[i]))) continue violations.push(`${file}:${i + 1} ${lines[i].trim()}`) } } diff --git a/client/src/components/Collections/ListEditorModal.test.tsx b/client/src/components/Collections/ListEditorModal.test.tsx index bac602ebd..71e1907b5 100644 --- a/client/src/components/Collections/ListEditorModal.test.tsx +++ b/client/src/components/Collections/ListEditorModal.test.tsx @@ -172,6 +172,21 @@ describe('ListEditorModal', () => { expect(actions.createCollection).not.toHaveBeenCalled() }) + it('FE-COMP-LISTEDITOR-026: a second Enter while the create is in flight does not make a second list', async () => { + let release: (value: { id: number; owner_id: number; name: string }) => void = () => {} + actions.createCollection.mockReturnValueOnce(new Promise(resolve => { release = resolve })) + const { props } = setup() + const nameInput = screen.getByPlaceholderText('e.g. Tokyo 2025') + fireEvent.change(nameInput, { target: { value: 'Lisbon' } }) + fireEvent.keyDown(nameInput, { key: 'Enter' }) + await waitFor(() => expect(actions.createCollection).toHaveBeenCalledTimes(1)) + fireEvent.keyDown(nameInput, { key: 'Enter' }) + release({ id: 77, owner_id: 1, name: 'Lisbon' }) + await waitFor(() => expect(props.onClose).toHaveBeenCalled()) + expect(actions.createCollection).toHaveBeenCalledTimes(1) + expect(props.onCreated).toHaveBeenCalledTimes(1) + }) + it('FE-COMP-LISTEDITOR-011: picking a swatch changes the saved colour', async () => { setup() fireEvent.change(screen.getByPlaceholderText('e.g. Tokyo 2025'), { target: { value: 'Oslo' } }) diff --git a/client/src/components/Collections/ListEditorModal.tsx b/client/src/components/Collections/ListEditorModal.tsx index 9b0609cb1..ec59dbd46 100644 --- a/client/src/components/Collections/ListEditorModal.tsx +++ b/client/src/components/Collections/ListEditorModal.tsx @@ -122,7 +122,9 @@ export default function ListEditorModal({ target, onClose, onCreated, onRequestD const save = async () => { const trimmed = name.trim() - if (!trimmed) return + // Enter fires while the create is still in flight, and `createdId` is only + // set once it comes back — without this the second one creates a second list. + if (!trimmed || saving) return // Normalise + keep only links with a url; drop blank rows. const cleanLinks = links .map(l => ({ label: l.label?.trim() || undefined, url: normalizeLinkUrl(l.url) })) diff --git a/client/src/components/Studio/StudioTravelPanel.tsx b/client/src/components/Studio/StudioTravelPanel.tsx index 29c0594cb..e3a4c61c2 100644 --- a/client/src/components/Studio/StudioTravelPanel.tsx +++ b/client/src/components/Studio/StudioTravelPanel.tsx @@ -1,3 +1,4 @@ +import { useEffect, useRef } from 'react' import type { BookElement, BookMetric, BookPageSetup, JourneyStats } from '@trek/shared' import { BOOK_METRICS } from '@trek/shared' import { useStudioStore } from '../../store/studioStore' @@ -95,6 +96,19 @@ export function StudioTravelPanel({ const spread = doc?.spreads[active] const single = !!spread && spread.role !== 'inner' + /* + * The road lookups still running when the panel goes away. + * + * fetchRoads walks the legs at a deliberate pace, so a twenty-stop map keeps + * asking a public router for minutes after Studio is closed, and the answer + * lands in whatever document the store holds by then. + */ + const roadJobs = useRef(new Set()) + useEffect(() => () => { + roadJobs.current.forEach(c => c.abort()) + roadJobs.current.clear() + }, []) + const centre = (w: number, h: number) => { const W = single ? page.pageWidth : page.pageWidth * 2 return { x: (W - w) / 2, y: (page.pageHeight - h) / 2, w, h } @@ -304,13 +318,17 @@ export function StudioTravelPanel({ * free, and a failure leaves the leg exactly as it was. */ if (placed.kind === 'map' && placed.points.length > 1) { - void fetchRoads(placed.points.map(pt => ({ lat: pt.lat, lng: pt.lng }))) + const job = new AbortController() + roadJobs.current.add(job) + void fetchRoads(placed.points.map(pt => ({ lat: pt.lat, lng: pt.lng })), { signal: job.signal }) .then(roads => { + if (job.signal.aborted) return if (roads.some(r => r && r.length > 1)) { updateElement(active, placed.id, { roads } as Partial) } }) .catch(() => { /* No roads is a state the map already draws. */ }) + .finally(() => { roadJobs.current.delete(job) }) } } diff --git a/client/src/components/Studio/applyTemplate.ts b/client/src/components/Studio/applyTemplate.ts index 0472dba46..307079514 100644 --- a/client/src/components/Studio/applyTemplate.ts +++ b/client/src/components/Studio/applyTemplate.ts @@ -221,7 +221,7 @@ function fillBadge(el: BookElement & { kind: 'badge' }, entry: TemplateEntry, ct } } if (el.variant === 'distance' && ctx.stats?.distance) { - el.text = `${Math.round(ctx.stats.distance / 1000).toLocaleString('en-US')} km` + el.text = `${Math.round(ctx.stats.distance / 1000).toLocaleString(ctx.locale)} km` } } diff --git a/client/src/components/Studio/autoLayout.ts b/client/src/components/Studio/autoLayout.ts index 5d1889c13..15c45fc84 100644 --- a/client/src/components/Studio/autoLayout.ts +++ b/client/src/components/Studio/autoLayout.ts @@ -1369,7 +1369,7 @@ function summarySpread(input: AutoInput): BookSpread | null { els.push(badgeEl( { x: rx + 8 + lineW * 0.5 - 17, y: y - 11, w: 34, h: 8 }, 'distance', - { text: formatKm(stats.furthest) }, + { text: formatKm(stats.furthest, input.locale) }, { style: 'chip', textScale: 0.7 }, )) els.push(text({ x: rx + 4, y: y + 4, w: lineW * 0.5, h: 5 }, (home.label || '').toUpperCase(), { @@ -1396,9 +1396,9 @@ function furthestPoint(stats: JourneyStats) { return best } -/** Metres as the books print them: "1,189 km". */ -function formatKm(metres: number): string { - return `${Math.round(metres / 1000).toLocaleString('en-US')} km` +/** Metres as the books print them: "1,189 km", in the book's own language. */ +function formatKm(metres: number, locale: string): string { + return `${Math.round(metres / 1000).toLocaleString(locale)} km` } function countriesSpread(input: AutoInput, names: string[]): BookSpread | null { @@ -1477,7 +1477,7 @@ function backSpread(input: AutoInput): BookSpread { if (input.stats) { const closing: BookMetric[] = ([ ['days', input.stats.days], - ['places', input.stats.places || input.stats.steps], + ['places', input.stats.places], ['photos', input.stats.photos], ] as [BookMetric, number][]) .filter(([, v]) => v > 0) diff --git a/client/src/components/Studio/useBookStore.ts b/client/src/components/Studio/useBookStore.ts index 6358d7e66..7fc627946 100644 --- a/client/src/components/Studio/useBookStore.ts +++ b/client/src/components/Studio/useBookStore.ts @@ -55,6 +55,8 @@ export type SaveState = | { status: 'saving' } | { status: 'saved'; at: number } | { status: 'conflict'; current: BookRecord } + /** The journey is open to this user, but they may not write it (role 'viewer'). */ + | { status: 'readonly' } | { status: 'error' } export function useBookStore( @@ -73,6 +75,9 @@ export function useBookStore( const pending = useRef<{ document: BookDocument; title: string } | null>(null) const inFlight = useRef(false) + /** The title the last queued save carried, so a re-queue does not lose it. */ + const lastTitle = useRef('') + /** * The document the server and this client agree on, by identity. * @@ -173,6 +178,15 @@ export function useBookStore( if (res?.status === 409 && res.data?.current) { blocked.current = true setState({ status: 'conflict', current: res.data.current }) + } else if (res?.status === 403) { + /* + * A viewer can open Studio - the book is part of the journey they were + * invited to - but the server will not take their writes. Stop trying + * on the first refusal and say so: retrying forever would let someone + * lay out an entire book before finding out none of it was ever saved. + */ + blocked.current = true + setState({ status: 'readonly' }) } else { setState({ status: 'error' }) } @@ -193,6 +207,7 @@ export function useBookStore( */ const queueSave = useCallback((document: BookDocument, title: string) => { latest.current = document + lastTitle.current = title // The document we already agree on is not an edit — see `synced`. if (document === synced.current) return pending.current = { document, title } @@ -207,7 +222,10 @@ export function useBookStore( /** Write immediately — for closing the editor, or a save the user asked for. */ const saveNow = useCallback((document?: BookDocument, title?: string) => { - if (document) pending.current = { document, title: title ?? '' } + if (document) { + if (title != null) lastTitle.current = title + pending.current = { document, title: title ?? lastTitle.current } + } if (timer.current != null) window.clearTimeout(timer.current) return write() }, [write]) @@ -236,8 +254,20 @@ export function useBookStore( const keepMine = useCallback((current: BookRecord) => { version.current = current.version blocked.current = false + /* + * The refused document has to be queued again here, because write() cleared + * the queue before it made the request that came back 409. Without this, + * choosing "keep mine" left the work sitting in the tab: the autosave only + * fires on the next change, and closing the editor writes whatever is + * queued, which is nothing. + */ + if (latest.current && latest.current !== synced.current) { + pending.current = { document: latest.current, title: lastTitle.current } + void write() + return + } setState({ status: 'idle' }) - }, []) + }, [write]) /** * Someone else saved. diff --git a/client/src/pages/JourneyDetailPage.tsx b/client/src/pages/JourneyDetailPage.tsx index 0b95dce89..2feea2258 100644 --- a/client/src/pages/JourneyDetailPage.tsx +++ b/client/src/pages/JourneyDetailPage.tsx @@ -1,4 +1,3 @@ -import { useRef } from 'react' import { useAuthStore } from '../store/authStore' import { journeyApi } from '../api/client' import Navbar from '../components/Layout/Navbar' @@ -45,14 +44,12 @@ function JourneyDetailPageDesktop() { showInvite, setShowInvite, showAddTrip, setShowAddTrip, unlinkTrip, setUnlinkTrip, showSettings, setShowSettings, hideSkeletons, setHideSkeletons, - mapRef, fullMapRef, activeLocationId, handleMarkerClick, handleLocationClick, + mapRef, fullMapRef, galleryUploadRef, activeLocationId, handleMarkerClick, handleLocationClick, mapEntries, sidebarMapItems, tripDates, isMobile, tracks, feedEdge, scrollFeedTo, loadJourney, updateEntry, deleteEntry, reorderEntries, uploadPhotos, deletePhoto, } = useJourneyDetail() - const galleryUploadRef = useRef<(() => void) | null>(null) - if (loading || !current) { return (
diff --git a/client/src/pages/join/useJoinTrip.test.ts b/client/src/pages/join/useJoinTrip.test.ts index 788a325f3..59cb72a51 100644 --- a/client/src/pages/join/useJoinTrip.test.ts +++ b/client/src/pages/join/useJoinTrip.test.ts @@ -1,4 +1,4 @@ -// FE-PAGE-JOIN-001 to FE-PAGE-JOIN-008 +// FE-PAGE-JOIN-001 to FE-PAGE-JOIN-011 import { act, renderHook, waitFor } from '@testing-library/react'; import { tripInviteApi } from '../../api/client'; import { useJoinTrip } from './useJoinTrip'; @@ -11,8 +11,12 @@ vi.mock('react-router', () => ({ useParams: () => params, })); +const addToast = vi.fn(); + beforeEach(() => { navigate.mockClear(); + addToast.mockClear(); + window.__addToast = addToast; params = { token: 'inv-1' }; vi.spyOn(tripInviteApi, 'preview').mockResolvedValue({ title: 'Japan 2026' } as never); vi.spyOn(tripInviteApi, 'accept').mockResolvedValue({ trip_id: 42 } as never); @@ -20,6 +24,7 @@ beforeEach(() => { afterEach(() => { vi.restoreAllMocks(); + delete window.__addToast; }); describe('useJoinTrip', () => { @@ -74,8 +79,8 @@ describe('useJoinTrip', () => { await waitFor(() => expect(navigate).toHaveBeenCalledWith('/trips/7', { replace: true })); }); - it('FE-PAGE-JOIN-006: falls back to invalid when redeeming fails', async () => { - vi.mocked(tripInviteApi.accept).mockRejectedValue(new Error('already a member')); + it('FE-PAGE-JOIN-006: falls back to invalid when the invite itself is gone', async () => { + vi.mocked(tripInviteApi.accept).mockRejectedValue({ response: { status: 404, data: { error: 'Invalid or expired invite link' } } }); const { result } = renderHook(() => useJoinTrip()); await waitFor(() => expect(result.current.state).toBe('ready')); @@ -84,6 +89,34 @@ describe('useJoinTrip', () => { expect(navigate).not.toHaveBeenCalled(); }); + // A rate limit or a 500 is not an expired invite: saying so burns a link that + // still works, and the invalid screen offers no way back. + it('FE-PAGE-JOIN-010: a server failure reports itself and leaves the invite retryable', async () => { + vi.mocked(tripInviteApi.accept).mockRejectedValue({ response: { status: 429, data: { error: 'Too many attempts. Please try again later.' } } }); + const { result } = renderHook(() => useJoinTrip()); + await waitFor(() => expect(result.current.state).toBe('ready')); + + act(() => result.current.accept()); + await waitFor(() => expect(result.current.state).toBe('ready')); + expect(addToast).toHaveBeenCalledWith('Too many attempts. Please try again later.', 'error', undefined); + expect(navigate).not.toHaveBeenCalled(); + + // And the second press really does go out again. + vi.mocked(tripInviteApi.accept).mockResolvedValue({ trip_id: 42 } as never); + await act(() => { result.current.accept(); return Promise.resolve(); }); + await waitFor(() => expect(navigate).toHaveBeenCalledWith('/trips/42', { replace: true })); + }); + + it('FE-PAGE-JOIN-011: a network failure with no status is reported, not called invalid', async () => { + vi.mocked(tripInviteApi.accept).mockRejectedValue(new Error('Network Error')); + const { result } = renderHook(() => useJoinTrip()); + await waitFor(() => expect(result.current.state).toBe('ready')); + + act(() => result.current.accept()); + await waitFor(() => expect(addToast).toHaveBeenCalled()); + expect(result.current.state).toBe('ready'); + }); + it('FE-PAGE-JOIN-007: accept is a no-op without a token', () => { params = {}; const { result } = renderHook(() => useJoinTrip()); diff --git a/client/src/pages/join/useJoinTrip.ts b/client/src/pages/join/useJoinTrip.ts index 0a7b8f14d..22c575592 100644 --- a/client/src/pages/join/useJoinTrip.ts +++ b/client/src/pages/join/useJoinTrip.ts @@ -1,6 +1,9 @@ import { useEffect, useState } from 'react' import { useNavigate, useParams } from 'react-router' import { tripInviteApi } from '../../api/client' +import { useTranslation } from '../../i18n' +import { getApiErrorMessage } from '../../types' +import { useToast } from '../../components/shared/Toast' export type JoinTripState = 'loading' | 'ready' | 'joining' | 'invalid' @@ -12,6 +15,8 @@ export type JoinTripState = 'loading' | 'ready' | 'joining' | 'invalid' export function useJoinTrip() { const { token } = useParams<{ token: string }>() const navigate = useNavigate() + const { t } = useTranslation() + const toast = useToast() const [state, setState] = useState('loading') const [title, setTitle] = useState('') @@ -30,7 +35,17 @@ export function useJoinTrip() { setState('joining') tripInviteApi.accept(token) .then((data: { trip_id: number }) => navigate(`/trips/${data.trip_id}`, { replace: true })) - .catch(() => setState('invalid')) + .catch((err: unknown) => { + // Only a 404 means the invite itself is gone. A rate limit or a server + // having a bad minute used to be reported as an expired invite too, + // which sent the user back to the dashboard with no way to try again. + if ((err as { response?: { status?: number } }).response?.status === 404) { + setState('invalid') + return + } + setState('ready') + toast.error(getApiErrorMessage(err, t('common.error'))) + }) } const goToDashboard = () => navigate('/dashboard', { replace: true }) diff --git a/client/src/pages/journeyDetail/useJourneyDetail.ts b/client/src/pages/journeyDetail/useJourneyDetail.ts index e84ac47ad..d8c9edef9 100644 --- a/client/src/pages/journeyDetail/useJourneyDetail.ts +++ b/client/src/pages/journeyDetail/useJourneyDetail.ts @@ -29,6 +29,8 @@ export function useJourneyDetail() { const { current, loading, notFound, loadJourney, updateEntry, deleteEntry, reorderEntries, uploadPhotos, deletePhoto } = useJourneyStore() const mapRef = useRef(null) const fullMapRef = useRef(null) + /** The gallery hands its file picker up here, so the hero button can open it. */ + const galleryUploadRef = useRef<(() => void) | null>(null) const [activeLocationId, setActiveLocationId] = useState(null) const isMobile = useIsMobile() @@ -382,7 +384,7 @@ export function useJourneyDetail() { showInvite, setShowInvite, showAddTrip, setShowAddTrip, unlinkTrip, setUnlinkTrip, showSettings, setShowSettings, hideSkeletons, setHideSkeletons, - mapRef, fullMapRef, activeLocationId, handleMarkerClick, handleLocationClick, + mapRef, fullMapRef, galleryUploadRef, activeLocationId, handleMarkerClick, handleLocationClick, mapEntries, sidebarMapItems, tripDates, isMobile, tracks, feedEdge, scrollFeedTo, loadJourney, updateEntry, deleteEntry, reorderEntries, uploadPhotos, deletePhoto, diff --git a/client/src/pages/journeyStudio/useJourneyStudio.test.tsx b/client/src/pages/journeyStudio/useJourneyStudio.test.tsx new file mode 100644 index 000000000..f8e049964 --- /dev/null +++ b/client/src/pages/journeyStudio/useJourneyStudio.test.tsx @@ -0,0 +1,149 @@ +import { act, renderHook, waitFor } from '@testing-library/react' +import type { JourneyStats } from '@trek/shared' +import { useStudioStore } from '../../store/studioStore' +import { useJourneyStore } from '../../store/journeyStore' +import { journeyApi } from '../../api/client' + +/** + * Studio's shell state (#1973), for the two things that cost work rather than + * pixels: the keyboard reaching past an open text field, and the layout input + * being frozen before the track has arrived. + */ + +vi.mock('react-router', () => ({ + useParams: () => ({ id: '9' }), + useNavigate: () => vi.fn(), + useLocation: () => ({ state: null }), +})) + +vi.mock('../../api/websocket', async importOriginal => ({ + ...await importOriginal(), + addListener: vi.fn(), + removeListener: vi.fn(), + joinBook: vi.fn(), + leaveBook: vi.fn(), + sendBookCursor: vi.fn(), +})) + +const autoLayout = await vi.importActual( + '../../components/Studio/autoLayout', +) +const buildBook = vi.fn(autoLayout.buildBook) +vi.mock('../../components/Studio/autoLayout', async importOriginal => { + const actual = await importOriginal() + return { ...actual, buildBook: (...args: Parameters) => buildBook(...args) } +}) + +import { useJourneyStudio } from './useJourneyStudio' + +const TRACK = [[52.5, 13.4], [52.6, 13.5], [52.7, 13.6]] + +const stats = (): JourneyStats => ({ + journeyId: 9, distance: 1000, days: 2, steps: 2, photos: 0, places: 2, furthest: 0, + countries: [], trips: [], start: '2026-06-02', end: '2026-06-03', + points: [], +} as unknown as JourneyStats) + +/** A journey with one entry, which is all the auto layout needs to build. */ +function seedJourney() { + useJourneyStore.setState({ + current: { + id: 9, title: 'Iceland', entries: [ + { id: 1, type: 'entry', title: 'A day', story: 'Something happened.', entry_date: '2026-06-02', photos: [] }, + ], gallery: [], trips: [], + } as never, + loading: false, + }) +} + +beforeEach(() => { + seedJourney() + buildBook.mockClear() + useStudioStore.setState({ doc: null, selection: [], activeSpread: 0 }) + vi.spyOn(journeyApi, 'getBook').mockResolvedValue({ book: null } as never) + vi.spyOn(journeyApi, 'saveBook').mockResolvedValue({} as never) + vi.spyOn(journeyApi, 'stats').mockResolvedValue(stats() as never) + vi.spyOn(journeyApi, 'listTracks').mockResolvedValue({ tracks: [{ points: TRACK }] } as never) +}) + +afterEach(() => { + vi.restoreAllMocks() +}) + +/** Mount and wait until the one-shot build has run. */ +async function mountStudio() { + const view = renderHook(() => useJourneyStudio()) + await waitFor(() => expect(useStudioStore.getState().doc).not.toBeNull()) + return view +} + +describe('the keyboard', () => { + /* + * Studio text commits on blur, so the sentence being typed is not in the undo + * history yet: a Ctrl+Z inside a text box took back the change before it and + * left the mistyped word exactly where it was. + */ + it('leaves undo to the field somebody is typing in', async () => { + const undo = vi.fn() + useStudioStore.setState({ undo }) + await mountStudio() + + const field = document.createElement('textarea') + document.body.appendChild(field) + act(() => { + field.dispatchEvent(new KeyboardEvent('keydown', { key: 'z', ctrlKey: true, bubbles: true })) + }) + + expect(undo).not.toHaveBeenCalled() + field.remove() + }) + + it('undoes the document when nothing is being typed into', async () => { + const undo = vi.fn() + useStudioStore.setState({ undo }) + await mountStudio() + + act(() => { + window.dispatchEvent(new KeyboardEvent('keydown', { key: 'z', ctrlKey: true, bubbles: true })) + }) + + expect(undo).toHaveBeenCalledTimes(1) + }) +}) + +describe('the auto layout input', () => { + /* + * The build waits on the book and the figures, never on the track, so the + * input was frozen with an empty path — and a relayout afterwards drew the + * straight ruler line over a route somebody had walked. + */ + it('picks up the track that lands after the book was built', async () => { + // Held back on purpose: the tracks request is the slow one in practice, and + // the build does not wait for it. + let release: (value: unknown) => void = () => {} + vi.mocked(journeyApi.listTracks).mockReturnValue(new Promise(resolve => { release = resolve }) as never) + + const { result } = await mountStudio() + await act(async () => { + release({ tracks: [{ points: TRACK }] }) + await Promise.resolve() + }) + + act(() => { result.current.relayoutBook() }) + + expect(buildBook).toHaveBeenCalled() + const input = buildBook.mock.calls[buildBook.mock.calls.length - 1][0] + expect(input.path).toEqual([TRACK]) + }) + + it('leaves the path empty when the journey has no track', async () => { + vi.mocked(journeyApi.listTracks).mockResolvedValue({ tracks: [] } as never) + const { result } = await mountStudio() + await waitFor(() => expect(journeyApi.listTracks).toHaveBeenCalled()) + + act(() => { result.current.relayoutBook() }) + + const input = buildBook.mock.calls[buildBook.mock.calls.length - 1][0] + expect(input.path).toEqual([]) + }) +}) diff --git a/client/src/pages/journeyStudio/useJourneyStudio.ts b/client/src/pages/journeyStudio/useJourneyStudio.ts index 655fe6f55..38ddd841a 100644 --- a/client/src/pages/journeyStudio/useJourneyStudio.ts +++ b/client/src/pages/journeyStudio/useJourneyStudio.ts @@ -389,9 +389,21 @@ export function useJourneyStudio() { useEffect(() => { if (!Number.isFinite(journeyId)) return let cancelled = false + /* + * The frozen layout input is patched too, not just the state. + * + * The build effect runs as soon as the book and the figures are in, which is + * usually before this answers, and what it froze is what the Auto layout + * buttons hand to buildBook afterwards. Without the patch a relayout drew + * the straight line between stops over a route somebody had walked. + */ + const applyPath = (p: [number, number][][]) => { + setPath(p) + if (autoInput.current) autoInput.current = { ...autoInput.current, path: p } + } journeyApi.listTracks(journeyId) - .then(data => { if (!cancelled) setPath(bookPath(data.tracks)) }) - .catch(() => { if (!cancelled) setPath([]) }) + .then(data => { if (!cancelled) applyPath(bookPath(data.tracks)) }) + .catch(() => { if (!cancelled) applyPath([]) }) return () => { cancelled = true } }, [journeyId]) @@ -589,8 +601,13 @@ export function useJourneyStudio() { useEffect(() => { const onKey = (e: KeyboardEvent) => { - const tag = (e.target as HTMLElement)?.tagName - const typing = tag === 'INPUT' || tag === 'TEXTAREA' + const el = e.target as HTMLElement | null + const typing = el?.tagName === 'INPUT' || el?.tagName === 'TEXTAREA' || el?.isContentEditable === true + + // Undo belongs to the field while somebody is in one: Studio text commits + // on blur, so the sentence being typed is not in the history yet and the + // document-level undo would take back the change before it instead. + if (typing) return if ((e.ctrlKey || e.metaKey) && e.key.toLowerCase() === 'z') { e.preventDefault()