From 74e1853c39f4b08c317319670227d4e8b0f2b0b5 Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 9 Oct 2026 11:01:14 +0200 Subject: [PATCH] refactor(docsync): share the scope picker, lane switch and folder name between desktop and phone useScopePicker holds the provider's containers, binding one, and making and binding a new one under the suggested name; the desktop dialog keeps its search filter. docSyncModel holds the direction a lane switch leads to and the folder name made from the trip title, which the panel and the phone sheet each carried a copy of. --- .../components/Files/docsync/DocSyncFlow.tsx | 7 +- .../components/Files/docsync/DocSyncPanel.tsx | 17 +-- .../Files/docsync/DocSyncScopeModal.tsx | 65 ++------- .../Files/docsync/docSyncModel.test.ts | 32 +++++ .../components/Files/docsync/docSyncModel.ts | 32 +++++ .../Files/docsync/useScopePicker.test.ts | 135 ++++++++++++++++++ .../Files/docsync/useScopePicker.ts | 79 ++++++++++ .../screens/trip/tabs/MDocSyncSheet.tsx | 75 ++-------- 8 files changed, 304 insertions(+), 138 deletions(-) create mode 100644 client/src/components/Files/docsync/docSyncModel.test.ts create mode 100644 client/src/components/Files/docsync/docSyncModel.ts create mode 100644 client/src/components/Files/docsync/useScopePicker.test.ts create mode 100644 client/src/components/Files/docsync/useScopePicker.ts diff --git a/client/src/components/Files/docsync/DocSyncFlow.tsx b/client/src/components/Files/docsync/DocSyncFlow.tsx index 012a78e3f..ec9fe128e 100644 --- a/client/src/components/Files/docsync/DocSyncFlow.tsx +++ b/client/src/components/Files/docsync/DocSyncFlow.tsx @@ -3,6 +3,7 @@ import { useTranslation } from '../../../i18n/TranslationContext' import { DOCUMENT_PROVIDER_ICONS } from '../../shared/DocumentProviderIcons' import TrekIcon from '../../shared/TrekIcon' import { Badge } from './DocSyncBits' +import { toggledDirection } from './docSyncModel' export type SyncDirection = 'both' | 'pull' | 'push' @@ -58,10 +59,8 @@ export default function DocSyncFlow({ /** Turning a lane off leaves the other one; turning the last one off is refused. */ const toggle = (lane: 'push' | 'pull') => { if (disabled) return - const nextPush = lane === 'push' ? !pushOn : pushOn - const nextPull = lane === 'pull' ? !pullOn : pullOn - if (!nextPush && !nextPull) return - onChange(nextPush && nextPull ? 'both' : nextPush ? 'push' : 'pull') + const next = toggledDirection(direction, lane) + if (next) onChange(next) } return ( diff --git a/client/src/components/Files/docsync/DocSyncPanel.tsx b/client/src/components/Files/docsync/DocSyncPanel.tsx index 546b6cab4..2027bb808 100644 --- a/client/src/components/Files/docsync/DocSyncPanel.tsx +++ b/client/src/components/Files/docsync/DocSyncPanel.tsx @@ -4,6 +4,7 @@ import { DialogHeader, DialogShell, DialogTile, NEUTRAL_TINT } from '../../share import { useTranslation } from '../../../i18n/TranslationContext' import { DOCUMENT_PROVIDER_ICONS } from '../../shared/DocumentProviderIcons' import { StateBadge } from './DocSyncBits' +import { slugFor } from './docSyncModel' import { useConflicts } from './useConflicts' import { storeName, useDocSync, type DocSyncLink, type DocSyncProvider } from './useDocSync' import DocSyncBinding from './DocSyncBinding' @@ -416,19 +417,3 @@ function Empty() { ) } - -/** - * A folder name from the trip's own title, so nobody has to invent one. - * - * The id is appended because two trips can share a title and a folder cannot. - */ -function slugFor(title: string | undefined, tripId: number | string): string { - const base = (title || 'trek') - .toLowerCase() - .normalize('NFD') - .replace(/[̀-ͯ]/g, '') - .replace(/[^a-z0-9]+/g, '-') - .replace(/^-+|-+$/g, '') - .slice(0, 40) - return `${base || 'trek'}-${tripId}` -} diff --git a/client/src/components/Files/docsync/DocSyncScopeModal.tsx b/client/src/components/Files/docsync/DocSyncScopeModal.tsx index e4a4af406..55f1c0df1 100644 --- a/client/src/components/Files/docsync/DocSyncScopeModal.tsx +++ b/client/src/components/Files/docsync/DocSyncScopeModal.tsx @@ -1,8 +1,9 @@ -import { useEffect, useId, useMemo, useState } from 'react' +import { useId, useMemo, useState } from 'react' import { FolderPlus, Loader2, Search } from 'lucide-react' import { DialogHeader, DialogShell, DialogTile, NEUTRAL_TINT } from '../../shared/DialogShell' import { useTranslation } from '../../../i18n/TranslationContext' -import type { DocSyncConnection, DocSyncScope, useDocSync } from './useDocSync' +import type { DocSyncConnection, useDocSync } from './useDocSync' +import { NEW_SCOPE, useScopePicker } from './useScopePicker' /** * Picking the container a trip lives in. @@ -32,28 +33,13 @@ export default function DocSyncScopeModal({ }) { const { t } = useTranslation() const labelId = useId() - const [scopes, setScopes] = useState(null) - const [error, setError] = useState(null) const [query, setQuery] = useState('') - const [newName, setNewName] = useState(suggestedName) - const [working, setWorking] = useState(null) - - // `loadScopes`, not `sync`: the hook hands back a fresh object on every - // render of the panel above, so depending on it re-listed the provider's - // folders each time anything up there changed. The callback itself is - // stable. Taken out of `sync` first, because calling it as `sync.loadScopes` - // inside the effect makes the whole object a dependency again. - const { loadScopes } = sync - useEffect(() => { - let cancelled = false - void (async () => { - const res = await loadScopes(connection.id) - if (cancelled) return - setScopes(res.scopes) - setError(res.error ?? null) - })() - return () => { cancelled = true } - }, [connection.id, loadScopes]) + const { scopes, error, name: newName, setName: setNewName, working, bind, createAndBind } = useScopePicker({ + connectionId: connection.id, + suggestedName, + sync, + onBound, + }) const shown = useMemo(() => { const needle = query.trim().toLowerCase() @@ -61,37 +47,6 @@ export default function DocSyncScopeModal({ return scopes.filter(s => s.label.toLowerCase().includes(needle)) }, [scopes, query]) - const bind = async (scope: DocSyncScope) => { - setWorking(scope.scopeKey) - const ok = await sync.createLink({ - connectionId: connection.id, - scopeKey: scope.scopeKey, - remoteRootId: scope.remoteRootId, - remoteRootPath: scope.remoteRootPath, - remoteLabel: scope.label, - direction: 'both', - deletePolicy: 'unlink', - conflictPolicy: 'manual', - syncEnabled: true, - }) - setWorking(null) - if (ok) onBound() - } - - const createAndBind = async () => { - const name = newName.trim() - if (!name) return - setWorking('__new__') - try { - // A refused create leaves nothing to bind; the hook has already put the - // reason where the dialog shows it. - const scope = await sync.createScope(connection.id, name) - if (scope) await bind(scope) - } finally { - setWorking(null) - } - } - return ( - {working === '__new__' ? : } + {working === NEW_SCOPE ? : } {t('docsync.scope.createAction')} diff --git a/client/src/components/Files/docsync/docSyncModel.test.ts b/client/src/components/Files/docsync/docSyncModel.test.ts new file mode 100644 index 000000000..35fb3fab5 --- /dev/null +++ b/client/src/components/Files/docsync/docSyncModel.test.ts @@ -0,0 +1,32 @@ +// FE-DOCSYNC-MODEL-001 to FE-DOCSYNC-MODEL-004: the lane switch and the folder name the +// desktop document sync dialog and the phone sheet share. +import { describe, expect, it } from 'vitest'; + +import { slugFor, toggledDirection } from './docSyncModel'; + +describe('toggledDirection', () => { + it('FE-DOCSYNC-MODEL-001: switching a lane off on a two way binding leaves the other one', () => { + expect(toggledDirection('both', 'push')).toBe('pull'); + expect(toggledDirection('both', 'pull')).toBe('push'); + }); + + it('FE-DOCSYNC-MODEL-002: switching the missing lane on makes it two way, the last lane cannot go', () => { + expect(toggledDirection('push', 'pull')).toBe('both'); + expect(toggledDirection('pull', 'push')).toBe('both'); + expect(toggledDirection('push', 'push')).toBeNull(); + expect(toggledDirection('pull', 'pull')).toBeNull(); + }); +}); + +describe('slugFor', () => { + it('FE-DOCSYNC-MODEL-003: folds the title to lower case ASCII words joined by dashes, with the trip id', () => { + expect(slugFor('Côte d’Azur 2026!', 7)).toBe('cote-d-azur-2026-7'); + expect(slugFor(' --Tokyo-- ', '12')).toBe('tokyo-12'); + }); + + it('FE-DOCSYNC-MODEL-004: no usable title falls back to trek, and long titles are cut at 40', () => { + expect(slugFor(undefined, 3)).toBe('trek-3'); + expect(slugFor('東京', 3)).toBe('trek-3'); + expect(slugFor('a'.repeat(60), 1)).toBe(`${'a'.repeat(40)}-1`); + }); +}); diff --git a/client/src/components/Files/docsync/docSyncModel.ts b/client/src/components/Files/docsync/docSyncModel.ts new file mode 100644 index 000000000..db037cca1 --- /dev/null +++ b/client/src/components/Files/docsync/docSyncModel.ts @@ -0,0 +1,32 @@ +import type { SyncDirection } from './DocSyncFlow'; + +/** + * The direction after switching one lane of a binding on or off, or null when that would + * switch off the last one. The desktop flow bar and the phone's two lane buttons both + * ask this. + */ +export function toggledDirection(direction: string, lane: 'push' | 'pull'): SyncDirection | null { + const pushOn = direction === 'both' || direction === 'push'; + const pullOn = direction === 'both' || direction === 'pull'; + const nextPush = lane === 'push' ? !pushOn : pushOn; + const nextPull = lane === 'pull' ? !pullOn : pullOn; + if (!nextPush && !nextPull) return null; + return nextPush && nextPull ? 'both' : nextPush ? 'push' : 'pull'; +} + +/** + * A folder name from the trip's own title, so nobody has to invent one. + * + * The id is appended because two trips can share a title and a folder cannot. + */ +export function slugFor(title: string | undefined, tripId: number | string): string { + const base = (title || 'trek') + .toLowerCase() + .normalize('NFD') + .replace(/[̀-ͯ]/g, '') + .replace(/[^a-z0-9]+/g, '-') + // The step above leaves at most one dash at either end. + .replace(/^-|-$/g, '') + .slice(0, 40); + return `${base || 'trek'}-${tripId}`; +} diff --git a/client/src/components/Files/docsync/useScopePicker.test.ts b/client/src/components/Files/docsync/useScopePicker.test.ts new file mode 100644 index 000000000..0503536e3 --- /dev/null +++ b/client/src/components/Files/docsync/useScopePicker.test.ts @@ -0,0 +1,135 @@ +// FE-DOCSYNC-SCOPE-001 to FE-DOCSYNC-SCOPE-008: picking the container a trip syncs with, +// behind the desktop scope dialog and the phone sheet's scope step. +import { act, renderHook, waitFor } from '@testing-library/react'; +import { describe, expect, it, vi } from 'vitest'; + +import type { DocSyncScope, useDocSync } from './useDocSync'; +import { NEW_SCOPE, useScopePicker } from './useScopePicker'; + +const FOLDER: DocSyncScope = { scopeKey: 'f1', label: 'Japan', remoteRootId: 'r1', remoteRootPath: '/Japan' }; + +function fakeSync(over: Record = {}) { + return { + loadScopes: vi.fn(async () => ({ scopes: [FOLDER] as DocSyncScope[], error: undefined as string | undefined })), + createScope: vi.fn(async (_id: number, name: string) => ({ ...FOLDER, scopeKey: 'new', label: name })), + createLink: vi.fn(async () => true), + ...over, + } as unknown as ReturnType; +} + +function setup(sync = fakeSync(), connectionId = 4) { + const onBound = vi.fn(); + const hook = renderHook( + (props: { connectionId: number }) => + useScopePicker({ connectionId: props.connectionId, suggestedName: 'japan-1', sync, onBound }), + { + initialProps: { connectionId }, + } + ); + return { ...hook, sync, onBound }; +} + +describe('useScopePicker', () => { + it('FE-DOCSYNC-SCOPE-001: starts with the suggested name and lists the containers', async () => { + const { result, sync } = setup(); + expect(result.current.name).toBe('japan-1'); + expect(result.current.scopes).toBeNull(); + await waitFor(() => expect(result.current.scopes).toEqual([FOLDER])); + expect(sync.loadScopes).toHaveBeenCalledWith(4); + expect(result.current.error).toBeNull(); + expect(result.current.working).toBeNull(); + }); + + it('FE-DOCSYNC-SCOPE-002: a listing the store refused shows its error', async () => { + const sync = fakeSync({ loadScopes: vi.fn(async () => ({ scopes: [], error: 'unreachable' })) }); + const { result } = setup(sync); + await waitFor(() => expect(result.current.error).toBe('unreachable')); + expect(result.current.scopes).toEqual([]); + }); + + it('FE-DOCSYNC-SCOPE-003: a listing for a connection left behind is dropped', async () => { + let resolveFirst: (v: { scopes: DocSyncScope[] }) => void = () => {}; + const other: DocSyncScope = { ...FOLDER, scopeKey: 'f2', label: 'Other' }; + const loadScopes = vi + .fn() + .mockReturnValueOnce(new Promise((r) => (resolveFirst = r))) + .mockResolvedValueOnce({ scopes: [other] }); + const { result, rerender } = setup(fakeSync({ loadScopes }), 4); + rerender({ connectionId: 5 }); + await waitFor(() => expect(result.current.scopes).toEqual([other])); + await act(async () => { + resolveFirst({ scopes: [FOLDER] }); + }); + expect(result.current.scopes).toEqual([other]); + }); + + it('FE-DOCSYNC-SCOPE-004: binding a container links it two way and hands over', async () => { + const { result, sync, onBound } = setup(); + await act(async () => { + await result.current.bind(FOLDER); + }); + expect(sync.createLink).toHaveBeenCalledWith({ + connectionId: 4, + scopeKey: 'f1', + remoteRootId: 'r1', + remoteRootPath: '/Japan', + remoteLabel: 'Japan', + direction: 'both', + deletePolicy: 'unlink', + conflictPolicy: 'manual', + syncEnabled: true, + }); + expect(onBound).toHaveBeenCalledTimes(1); + expect(result.current.working).toBeNull(); + }); + + it('FE-DOCSYNC-SCOPE-005: a refused link stays on the picker', async () => { + const { result, onBound } = setup(fakeSync({ createLink: vi.fn(async () => false) })); + await act(async () => { + await result.current.bind(FOLDER); + }); + expect(onBound).not.toHaveBeenCalled(); + expect(result.current.working).toBeNull(); + }); + + it('FE-DOCSYNC-SCOPE-006: a new container is made under the trimmed name and bound', async () => { + let resolveCreate: (v: DocSyncScope) => void = () => {}; + const createScope = vi.fn(() => new Promise((r) => (resolveCreate = r))); + const { result, sync, onBound } = setup(fakeSync({ createScope })); + act(() => result.current.setName(' Kyoto ')); + + let pending: Promise | undefined; + act(() => { + pending = result.current.createAndBind(); + }); + expect(result.current.working).toBe(NEW_SCOPE); + await act(async () => { + resolveCreate({ ...FOLDER, scopeKey: 'new', label: 'Kyoto' }); + await pending; + }); + expect(createScope).toHaveBeenCalledWith(4, 'Kyoto'); + expect(sync.createLink).toHaveBeenCalledWith(expect.objectContaining({ scopeKey: 'new', remoteLabel: 'Kyoto' })); + expect(onBound).toHaveBeenCalledTimes(1); + expect(result.current.working).toBeNull(); + }); + + it('FE-DOCSYNC-SCOPE-007: a blank name makes nothing', async () => { + const { result, sync } = setup(); + act(() => result.current.setName(' ')); + await act(async () => { + await result.current.createAndBind(); + }); + expect(sync.createScope).not.toHaveBeenCalled(); + expect(result.current.working).toBeNull(); + }); + + it('FE-DOCSYNC-SCOPE-008: a refused create binds nothing and clears the marker', async () => { + const { result, sync, onBound } = setup(fakeSync({ createScope: vi.fn(async () => null) })); + await act(async () => { + await result.current.createAndBind(); + }); + expect(sync.createLink).not.toHaveBeenCalled(); + expect(onBound).not.toHaveBeenCalled(); + expect(result.current.working).toBeNull(); + }); +}); diff --git a/client/src/components/Files/docsync/useScopePicker.ts b/client/src/components/Files/docsync/useScopePicker.ts new file mode 100644 index 000000000..cc3af30c6 --- /dev/null +++ b/client/src/components/Files/docsync/useScopePicker.ts @@ -0,0 +1,79 @@ +import { useEffect, useState } from 'react'; + +import type { DocSyncScope, useDocSync } from './useDocSync'; + +/** The `working` marker while a new container is being made. */ +export const NEW_SCOPE = '__new__'; + +export interface ScopePickerOptions { + connectionId: number; + /** Pre-filled name for a new container, from the trip's title. */ + suggestedName: string; + sync: ReturnType; + onBound: () => void; +} + +/** + * Picking the folder, tag or space a trip syncs with, behind the desktop scope dialog and + * the phone sheet's scope step: the provider's containers, binding one of them, or making + * a new one under the suggested name and binding that. `working` holds the scope key being + * bound, or NEW_SCOPE while a new container is made. + */ +export function useScopePicker({ connectionId, suggestedName, sync, onBound }: ScopePickerOptions) { + const [scopes, setScopes] = useState(null); + const [error, setError] = useState(null); + const [name, setName] = useState(suggestedName); + const [working, setWorking] = useState(null); + + // `loadScopes`, not `sync`: the hook hands back a fresh object on every + // render of the panel above, so depending on it re-listed the provider's + // folders each time anything up there changed. The callback itself is + // stable. Taken out of `sync` first, because calling it as `sync.loadScopes` + // inside the effect makes the whole object a dependency again. + const { loadScopes } = sync; + useEffect(() => { + let cancelled = false; + void (async () => { + const res = await loadScopes(connectionId); + if (cancelled) return; + setScopes(res.scopes); + setError(res.error ?? null); + })(); + return () => { + cancelled = true; + }; + }, [connectionId, loadScopes]); + + const bind = async (scope: Pick) => { + setWorking(scope.scopeKey); + const ok = await sync.createLink({ + connectionId, + scopeKey: scope.scopeKey, + remoteRootId: scope.remoteRootId, + remoteRootPath: scope.remoteRootPath, + remoteLabel: scope.label, + direction: 'both', + deletePolicy: 'unlink', + conflictPolicy: 'manual', + syncEnabled: true, + }); + setWorking(null); + if (ok) onBound(); + }; + + const createAndBind = async () => { + const trimmed = name.trim(); + if (!trimmed) return; + setWorking(NEW_SCOPE); + try { + // A refused create leaves nothing to bind; the hook has already put the + // reason where the picker shows it. + const scope = await sync.createScope(connectionId, trimmed); + if (scope) await bind(scope); + } finally { + setWorking(null); + } + }; + + return { scopes, error, name, setName, working, bind, createAndBind }; +} diff --git a/client/src/mobile/screens/trip/tabs/MDocSyncSheet.tsx b/client/src/mobile/screens/trip/tabs/MDocSyncSheet.tsx index df2c29aff..fd239d0aa 100644 --- a/client/src/mobile/screens/trip/tabs/MDocSyncSheet.tsx +++ b/client/src/mobile/screens/trip/tabs/MDocSyncSheet.tsx @@ -16,6 +16,8 @@ import { } from '../../../../components/Files/docsync/useDocSync' import { useConnectForm } from '../../../../components/Files/docsync/useConnectForm' import { conflictPolicyKey, nextConflictPolicy } from '../../../../components/Files/docsync/DocSyncBits' +import { slugFor, toggledDirection } from '../../../../components/Files/docsync/docSyncModel' +import { NEW_SCOPE, useScopePicker } from '../../../../components/Files/docsync/useScopePicker' import { useConflicts } from '../../../../components/Files/docsync/useConflicts' import { relativeTime } from '../../../../utils/relativeTime' @@ -294,10 +296,8 @@ function DetailView({ /** The last remaining direction cannot be switched off. */ const toggle = (lane: 'push' | 'pull') => { if (!canManage) return - const nextPush = lane === 'push' ? !pushOn : pushOn - const nextPull = lane === 'pull' ? !pullOn : pullOn - if (!nextPush && !nextPull) return - void sync.updateLink(link.id, { direction: nextPush && nextPull ? 'both' : nextPush ? 'push' : 'pull' }) + const next = toggledDirection(link.direction, lane) + if (next) void sync.updateLink(link.id, { direction: next }) } const ranAt = link.lastSyncAt ? Date.parse(link.lastSyncAt) : NaN @@ -547,44 +547,12 @@ function ScopeView({ onBound: () => void }) { const { t } = useTranslation() - const [scopes, setScopes] = useState>['scopes'] | null>(null) - const [error, setError] = useState(null) - const [name, setName] = useState(suggestedName) - const [working, setWorking] = useState(null) - - // `loadScopes`, not `sync`: the hook hands back a fresh object on every - // render of the panel above, so depending on it re-listed the provider's - // folders each time anything up there changed. The callback itself is - // stable. Taken out of `sync` first, because calling it as `sync.loadScopes` - // inside the effect makes the whole object a dependency again. - const { loadScopes } = sync - useEffect(() => { - let cancelled = false - void (async () => { - const res = await loadScopes(connectionId) - if (cancelled) return - setScopes(res.scopes) - setError(res.error ?? null) - })() - return () => { cancelled = true } - }, [connectionId, loadScopes]) - - const bind = async (scope: { scopeKey: string; label: string; remoteRootId: string | null; remoteRootPath: string | null }) => { - setWorking(scope.scopeKey) - const ok = await sync.createLink({ - connectionId, - scopeKey: scope.scopeKey, - remoteRootId: scope.remoteRootId, - remoteRootPath: scope.remoteRootPath, - remoteLabel: scope.label, - direction: 'both', - deletePolicy: 'unlink', - conflictPolicy: 'manual', - syncEnabled: true, - }) - setWorking(null) - if (ok) onBound() - } + const { scopes, error, name, setName, working, bind, createAndBind } = useScopePicker({ + connectionId, + suggestedName, + sync, + onBound, + }) return (
@@ -599,18 +567,11 @@ function ScopeView({ />
@@ -811,18 +772,6 @@ function Empty({ text, hint }: { text: string; hint?: string }) { ) } -/** A folder name from the trip's own title, so nobody has to invent one. */ -function slugFor(title: string | undefined, tripId: number | string): string { - const base = (title || 'trek') - .toLowerCase() - .normalize('NFD') - .replace(/[̀-ͯ]/g, '') - .replace(/[^a-z0-9]+/g, '-') - .replace(/^-+|-+$/g, '') - .slice(0, 40) - return `${base || 'trek'}-${tripId}` -} - /** * The documents that changed in both places, with the way out. *