From 3c3a228737cc27d46dd417f7f30cd8d5ff5b0142 Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 21 Oct 2026 21:45:25 +0200 Subject: [PATCH] fix(assistant): lint, types and tests for the CI round - agent loop and decisions without await in loops: steps as recursion, calls and applied changes in order through one promise chain, the independent reads and claims in parallel; part mapping in its own helpers - place card, header and weather strip without refs or components created in render; long hooks and components split; state reset during render - trip notes only in a turn's tool session, never an undo's; map view reported only while the assistant can run - place strip scrolls through the strip's own smooth scroll, no physical side - tests follow the raised limits and the new card fields; toasts read through a mocked useToast instead of a window global; baselines lowered where the counts fell --- client/scripts/eslint-baseline.json | 4 +- client/scripts/strict-baseline.json | 1 - client/scripts/theme-baseline.json | 2 +- .../Admin/addons/AiAdminConfig.test.tsx | 4 +- .../Admin/addons/useLlmParsingConfig.ts | 136 +++--- .../Assistant/AssistantChangeCard.test.tsx | 21 +- .../Assistant/AssistantComposer.test.tsx | 2 +- .../components/Assistant/AssistantHeader.tsx | 7 +- .../Assistant/AssistantPlaceCard.tsx | 191 +++++---- .../Assistant/AssistantPlaceStrip.tsx | 9 +- .../Assistant/AssistantPowerParts.test.tsx | 6 +- .../Assistant/AssistantWeatherStrip.tsx | 5 +- .../components/Assistant/AssistantWindow.tsx | 6 +- .../Assistant/assistantHintRules.ts | 11 +- .../components/Assistant/assistantParts.ts | 21 +- .../Assistant/useAssistantHotkey.ts | 2 +- .../Assistant/useAssistantShare.test.tsx | 19 +- .../Assistant/useAssistantThread.test.tsx | 20 +- .../Assistant/useFoundPlacePhoto.ts | 9 +- .../src/components/Map/assistantViewport.ts | 14 +- .../Settings/ai/AiConnectionSection.tsx | 75 ++-- .../Settings/ai/useAiConnections.ts | 28 +- client/src/components/shared/Kbd.test.tsx | 3 +- client/src/components/shared/Kbd.tsx | 5 - .../tripPlanner/useAssistantMapLink.test.ts | 6 +- client/src/utils/applePlatform.ts | 4 + server/src/nest/assistant/assistant-agent.ts | 405 ++++++++++-------- .../nest/assistant/assistant-cards.helpers.ts | 14 +- .../assistant/assistant-decisions.helpers.ts | 44 ++ .../assistant-found-places.service.ts | 7 +- .../nest/assistant/assistant-tools.service.ts | 9 +- .../src/nest/assistant/assistant.service.ts | 192 ++++----- server/src/nest/oauth/oauth-cimd.ts | 6 +- server/tests/e2e/assistant.e2e.test.ts | 72 ++-- server/tests/helpers/factories/places.ts | 12 +- .../OauthClients.repository.test.ts | 11 +- .../assistant-attachments.controller.test.ts | 8 +- .../assistant-booking-import.service.test.ts | 12 +- .../assistant-found-places.service.test.ts | 4 +- .../nest/assistant/assistant-history.test.ts | 4 +- .../nest/assistant/assistant-memory.test.ts | 57 +++ .../nest/assistant/assistant-redact.test.ts | 5 +- .../assistant/assistant-tools.service.test.ts | 7 +- .../tool-call-preview.helpers.test.ts | 12 + server/tests/unit/nest/oauth.service.test.ts | 2 +- shared/src/i18n/de/assistant.ts | 4 +- shared/src/i18n/nl/assistant.ts | 4 +- 47 files changed, 892 insertions(+), 610 deletions(-) create mode 100644 client/src/utils/applePlatform.ts create mode 100644 server/src/nest/assistant/assistant-decisions.helpers.ts create mode 100644 server/tests/unit/nest/assistant/assistant-memory.test.ts diff --git a/client/scripts/eslint-baseline.json b/client/scripts/eslint-baseline.json index d0de5a06d..60488f868 100644 --- a/client/scripts/eslint-baseline.json +++ b/client/scripts/eslint-baseline.json @@ -1,7 +1,7 @@ { "src": { "@typescript-eslint/no-non-null-assertion": 404, - "@typescript-eslint/no-misused-promises": 874, + "@typescript-eslint/no-misused-promises": 870, "no-console": 88, "complexity": 230, "@typescript-eslint/no-explicit-any": 147, @@ -35,7 +35,7 @@ }, "tests": { "@typescript-eslint/no-explicit-any": 491, - "@typescript-eslint/no-non-null-assertion": 1946, + "@typescript-eslint/no-non-null-assertion": 1945, "@typescript-eslint/no-misused-promises": 22, "@typescript-eslint/no-unused-vars": 51, "@typescript-eslint/no-floating-promises": 33, diff --git a/client/scripts/strict-baseline.json b/client/scripts/strict-baseline.json index 31b55fa95..b08b5b08b 100644 --- a/client/scripts/strict-baseline.json +++ b/client/scripts/strict-baseline.json @@ -148,7 +148,6 @@ "tests/integration/api/client.test.ts": 1, "tests/integration/hooks/useRouteCalculation.test.ts": 1, "tests/unit/i18n/i18nKeysScript.test.ts": 3, - "tests/unit/mobile/admin/MAdminAddonManager.test.tsx": 2, "tests/unit/mobile/admin/MAdminGitHubPanel.test.tsx": 1, "tests/unit/mobile/admin/MAdminNotifyMatrix.test.tsx": 1, "tests/unit/mobile/admin/MAdminStoragePanel.test.tsx": 1, diff --git a/client/scripts/theme-baseline.json b/client/scripts/theme-baseline.json index 5c871f962..2a95337f5 100644 --- a/client/scripts/theme-baseline.json +++ b/client/scripts/theme-baseline.json @@ -156,7 +156,7 @@ "mobile/components/MToastHost.tsx": 2, "mobile/components/MUserPickerList.tsx": 1, "mobile/screens/admin/MAdmin.tsx": 9, - "mobile/screens/admin/MAdminAddonManager.tsx": 26, + "mobile/screens/admin/MAdminAddonManager.tsx": 13, "mobile/screens/admin/MAdminAuditLogPanel.tsx": 15, "mobile/screens/admin/MAdminBackupPanel.tsx": 8, "mobile/screens/admin/MAdminCategoryManager.tsx": 8, diff --git a/client/src/components/Admin/addons/AiAdminConfig.test.tsx b/client/src/components/Admin/addons/AiAdminConfig.test.tsx index f1b2ba6e7..7fe6b9a48 100644 --- a/client/src/components/Admin/addons/AiAdminConfig.test.tsx +++ b/client/src/components/Admin/addons/AiAdminConfig.test.tsx @@ -167,7 +167,9 @@ describe('AI parsing', () => { expect( await screen.findByText('No connection yet. Set one up for everyone, or give AI parsing its own.') ).toBeInTheDocument(); - await user.click(screen.getByRole('button', { name: 'Connection' })); + // The link in the hint, not the tab of the same name above it. + const link = screen.getAllByRole('button', { name: 'Connection' }).find((b) => !b.hasAttribute('aria-pressed')); + await user.click(link as HTMLElement); expect(await screen.findByRole('radio', { name: 'I pay, own allowed' })).toBeInTheDocument(); }); diff --git a/client/src/components/Admin/addons/useLlmParsingConfig.ts b/client/src/components/Admin/addons/useLlmParsingConfig.ts index 099651c31..97e59e6e1 100644 --- a/client/src/components/Admin/addons/useLlmParsingConfig.ts +++ b/client/src/components/Admin/addons/useLlmParsingConfig.ts @@ -26,6 +26,72 @@ function savedOf(cfg: Record): SavedParsing { }; } +/** + * The models a local (Ollama) server has, and pulling the recommended ones. + * The list loads while the form shows the local provider: a provider change or + * opening the form loads it, the address field's own blur reloads it. + */ +function useLocalModels(baseUrl: string, isLocal: boolean, formOpen: boolean, onPulled: (id: string) => void) { + const { t } = useTranslation(); + const toast = useToast(); + const [installed, setInstalled] = useState([]); + const [modelsErr, setModelsErr] = useState(''); + const [loadingModels, setLoadingModels] = useState(false); + const [pulling, setPulling] = useState(null); + const [pullPct, setPullPct] = useState(0); + const [pullStatus, setPullStatus] = useState(''); + const effectiveUrl = baseUrl.trim() || DEFAULT_OLLAMA_URL; + const isInstalled = (id: string) => installed.some((n) => n === id || n.startsWith(id + ':') || n.startsWith(id)); + + const loadModels = async () => { + if (!isLocal) return; + setLoadingModels(true); + setModelsErr(''); + try { + const res = await adminApi.llmLocalModels(effectiveUrl); + setInstalled(res.models.map((m) => m.name)); + } catch (e: unknown) { + // The server says why (unreachable, key refused); axios' own status line does not. + setModelsErr(getApiErrorMessage(e, t('admin.addons.ai.parsing.unreachable'))); + setInstalled([]); + } finally { + setLoadingModels(false); + } + }; + + const loadModelsForProvider = useEffectEvent(() => { + if (formOpen && isLocal) void loadModels(); + }); + useEffect(() => { + loadModelsForProvider(); + }, [isLocal, formOpen]); + + const pull = async (id: string) => { + if (pulling) return; + setPulling(id); + setPullPct(0); + setPullStatus(t('admin.addons.ai.parsing.pulling')); + try { + await adminApi.llmLocalPull(effectiveUrl, id, (p) => { + if (p.error) throw new Error(p.error); + if (p.status) setPullStatus(p.status); + if (p.total && p.completed != null) setPullPct(Math.round((p.completed / p.total) * 100)); + }); + toast.success(t('admin.addons.ai.parsing.pulled')); + onPulled(id); + await loadModels(); + } catch (e: unknown) { + toast.error(e instanceof Error && e.message ? e.message : t('admin.addons.ai.parsing.pullFailed')); + } finally { + setPulling(null); + setPullPct(0); + setPullStatus(''); + } + }; + + return { installed, modelsErr, loadingModels, pulling, pullPct, pullStatus, isInstalled, loadModels, pull }; +} + /** * AI parsing's admin settings behind both admin shells. Parsing reads with the * AI connection the assistant runs on unless the instance keeps one of its own @@ -45,14 +111,6 @@ export function useLlmParsingConfig(addon: Addon) { const [apiKey, setApiKey] = useState(saved.apiKey); const [saving, setSaving] = useState(false); - // Local-provider model management. - const [installed, setInstalled] = useState([]); - const [modelsErr, setModelsErr] = useState(''); - const [loadingModels, setLoadingModels] = useState(false); - const [pulling, setPulling] = useState(null); - const [pullPct, setPullPct] = useState(0); - const [pullStatus, setPullStatus] = useState(''); - /** A connection of its own, which wins over the shared one. */ const ownConnection = !!(saved.provider && saved.model); const formOpen = ownConnection || editing; @@ -68,57 +126,7 @@ export function useLlmParsingConfig(addon: Addon) { setProviderState(next); }; const baseUrlPlaceholder = LLM_BASE_URL_PLACEHOLDER[provider] ?? LLM_BASE_URL_PLACEHOLDER.openai; - - const effectiveUrl = baseUrl.trim() || DEFAULT_OLLAMA_URL; - const isInstalled = (id: string) => installed.some((n) => n === id || n.startsWith(id + ':') || n.startsWith(id)); - - const loadModels = async () => { - if (provider !== 'local') return; - setLoadingModels(true); - setModelsErr(''); - try { - const res = await adminApi.llmLocalModels(effectiveUrl); - setInstalled(res.models.map((m) => m.name)); - } catch (e: unknown) { - // The server says why (unreachable, key refused); axios' own status line does not. - setModelsErr(getApiErrorMessage(e, t('admin.addons.ai.parsing.unreachable'))); - setInstalled([]); - } finally { - setLoadingModels(false); - } - }; - - // Load installed models while the form shows the local provider. Only a provider - // change (or opening the form) triggers it; the base URL's own blur reloads. - const loadModelsForProvider = useEffectEvent(() => { - if (formOpen && provider === 'local') void loadModels(); - }); - useEffect(() => { - loadModelsForProvider(); - }, [provider, formOpen]); - - const pull = async (id: string) => { - if (pulling) return; - setPulling(id); - setPullPct(0); - setPullStatus(t('admin.addons.ai.parsing.pulling')); - try { - await adminApi.llmLocalPull(effectiveUrl, id, (p) => { - if (p.error) throw new Error(p.error); - if (p.status) setPullStatus(p.status); - if (p.total && p.completed != null) setPullPct(Math.round((p.completed / p.total) * 100)); - }); - toast.success(t('admin.addons.ai.parsing.pulled')); - setModel(id); - await loadModels(); - } catch (e: unknown) { - toast.error(e instanceof Error && e.message ? e.message : t('admin.addons.ai.parsing.pullFailed')); - } finally { - setPulling(null); - setPullPct(0); - setPullStatus(''); - } - }; + const local = useLocalModels(baseUrl, provider === 'local', formOpen, setModel); /** Writes the config; the stored key stays as long as the mask is sent in its place. */ const write = async (next: SavedParsing, config: Record): Promise => { @@ -186,15 +194,7 @@ export function useLlmParsingConfig(addon: Addon) { startOwn: () => setEditing(true), cancelOwn: () => setEditing(false), switchToShared, - installed, - modelsErr, - loadingModels, - pulling, - pullPct, - pullStatus, - isInstalled, - loadModels, - pull, + ...local, save, }; } diff --git a/client/src/components/Assistant/AssistantChangeCard.test.tsx b/client/src/components/Assistant/AssistantChangeCard.test.tsx index b3dd0f9af..52fa684c1 100644 --- a/client/src/components/Assistant/AssistantChangeCard.test.tsx +++ b/client/src/components/Assistant/AssistantChangeCard.test.tsx @@ -20,6 +20,18 @@ import AssistantCompareSwitch from './AssistantCompareSwitch'; import type { ChangeItem } from './assistantParts'; import { type AssistantView, AssistantViewContext } from './assistantViewContext'; +const { addToast } = vi.hoisted(() => ({ addToast: vi.fn() })); +// The toasts a component raises, read as (message, type) the way the toast host takes them. +vi.mock('../shared/Toast', async (importOriginal) => ({ + ...(await importOriginal()), + useToast: () => ({ + success: (message: string) => addToast(message, 'success', undefined), + error: (message: string) => addToast(message, 'error', undefined), + warning: (message: string) => addToast(message, 'warning', undefined), + info: (message: string) => addToast(message, 'info', undefined), + }), +})); + const TRIP = 4; const INITIAL = useAssistantStore.getState(); const TRIP_INITIAL = useTripStore.getState(); @@ -98,18 +110,15 @@ const undoBlock = const setCompare = vi.fn<(tripId: number, mode: 'before' | 'after') => void>(); const requestCamera = vi.fn(); const setMinimized = vi.fn(); -const addToast = vi.fn(); beforeEach(() => { decide.mockResolvedValue(undefined); useAssistantStore.setState({ decide, undo, undoBlock, setCompare, requestCamera, setMinimized }); - window.__addToast = addToast as unknown as typeof window.__addToast; }); afterEach(() => { useAssistantStore.setState(INITIAL, true); useTripStore.setState(TRIP_INITIAL, true); - delete window.__addToast; vi.clearAllMocks(); vi.useRealTimers(); }); @@ -119,9 +128,13 @@ describe('AssistantChangeCard: deciding', () => { renderCard([pending('c1', 'Kinkaku-ji')]); expect(screen.getByText('Kinkaku-ji')).toBeInTheDocument(); expect(screen.getByText('Add place')).toBeInTheDocument(); - expect(screen.getByText('Address')).toBeInTheDocument(); + // The time is a chip of its own, old to new; the address waits behind "Details". expect(screen.getByText('09:00')).toBeInTheDocument(); expect(screen.getByText('10:30')).toBeInTheDocument(); + expect(screen.queryByText('Kyoto')).toBeNull(); + fireEvent.click(screen.getByRole('button', { name: 'Details' })); + expect(screen.getByText('Address')).toBeInTheDocument(); + expect(screen.getByText('Kyoto')).toBeInTheDocument(); expect(screen.queryByRole('button', { name: 'Include this change' })).toBeNull(); expect(screen.getByRole('status')).toHaveTextContent('1 change is waiting for you'); }); diff --git a/client/src/components/Assistant/AssistantComposer.test.tsx b/client/src/components/Assistant/AssistantComposer.test.tsx index 384a12c09..c922fa8f9 100644 --- a/client/src/components/Assistant/AssistantComposer.test.tsx +++ b/client/src/components/Assistant/AssistantComposer.test.tsx @@ -80,7 +80,7 @@ describe('AssistantComposer mentions', () => { fireEvent.keyDown(field, { key: 'Enter' }); await waitFor(() => expect(field.value).toBe('Is @Louvre ')); expect(send).not.toHaveBeenCalled(); - expect(screen.queryByRole('listbox')).toBeNull(); + await waitFor(() => expect(screen.queryByRole('listbox')).toBeNull()); expect(mentions()).toEqual([{ kind: 'place', id: 21, label: 'Louvre' }]); expect(chatOf(useAssistantStore.getState(), TRIP).draft).toBe('Is @Louvre '); // The pick shows in the text only, never as a chip above it. diff --git a/client/src/components/Assistant/AssistantHeader.tsx b/client/src/components/Assistant/AssistantHeader.tsx index e97b13124..00e6b3a32 100644 --- a/client/src/components/Assistant/AssistantHeader.tsx +++ b/client/src/components/Assistant/AssistantHeader.tsx @@ -48,6 +48,7 @@ export default function AssistantHeader({ tripId, thread, tripTitle, onClose, dr const [confirming, setConfirming] = useState(false); const threads = thread.threads ?? []; const history = usePopoverList(threads.findIndex((th) => th.id === thread.threadId) + 1); + const { open: historyOpen, triggerRef: historyTrigger, listId: historyListId } = history; const hints = useAssistantHintsSetting(); const openHistory = () => { @@ -140,12 +141,12 @@ export default function AssistantHeader({ tripId, thread, tripTitle, onClose, dr + + ); + } + if (suggested) { + return ( + + + ); + } + return strip.canSuggest ? : null; +} + /** * One place a search found, as a card: its picture from TREK's photo proxy * with the category it most likely belongs in on it (or that category's icon * on its colour), its name, address and rating, whether it is open now while * the trip runs, how far it is from the last stop of the day in focus, and one - * way on: "+ Day 3" for a new - * place, "On map" for one the trip has. A click on the card shows it on the - * map, and resting the pointer on it pins it there for a moment. + * way on: "+ Day 3" for a new place, "On map" for one the trip has. A click on + * the card shows it on the map, and resting the pointer on it pins it there + * for a moment. */ export default function AssistantPlaceCard({ place, @@ -146,30 +246,8 @@ export default function AssistantPlaceCard({ const photo = useFoundPlacePhoto(place, ref); const category = strip.categoryOf(place); const distance = strip.distanceOf(place); - const Icon = category ? getCategoryIcon(category.icon) : MapPin; const tall = variant === 'fullscreen'; - let action: ReactNode = null; - if (place.inTripPlaceId !== null) { - action = ( - - - - ); - } else if (suggested) { - action = ( - - - ); - } else if (strip.canSuggest) { - action = ; - } - return (
  • strip.hover(null)} className="flex flex-col text-start outline-none focus-visible:bg-surface-hover" > - - {photo ? ( - - ) : ( - - - )} - {category && photo && ( - - - )} - {place.openNow !== null && ( - - - )} - + {place.name} {category && !photo && {category.name}} @@ -241,7 +282,9 @@ export default function AssistantPlaceCard({ )} -
    {action}
    +
    + +
    • ); } diff --git a/client/src/components/Assistant/AssistantPlaceStrip.tsx b/client/src/components/Assistant/AssistantPlaceStrip.tsx index a80c4b988..f50de2b48 100644 --- a/client/src/components/Assistant/AssistantPlaceStrip.tsx +++ b/client/src/components/Assistant/AssistantPlaceStrip.tsx @@ -3,7 +3,6 @@ import { ChevronLeft, ChevronRight } from 'lucide-react'; import { useCallback, useEffect, useRef, useState } from 'react'; import { useTranslation } from '../../i18n'; -import { prefersReducedMotion } from '../../utils/reducedMotion'; import AssistantPlaceCard from './AssistantPlaceCard'; import { suggestedFromStrip } from './assistantParts'; import { useAssistantView } from './assistantViewContext'; @@ -71,10 +70,8 @@ export default function AssistantPlaceStrip({ found, message }: { found: Assista const step = card.offsetWidth + gap; const cards = Math.max(1, Math.floor((el.clientWidth - 32) / step)); const rtl = getComputedStyle(el).direction === 'rtl'; - el.scrollBy({ - left: (rtl ? -towards : towards) * cards * step, - behavior: prefersReducedMotion() ? 'auto' : 'smooth', - }); + // Smooth through the strip's own scroll-behavior, which reduced motion turns off. + el.scrollBy((rtl ? -towards : towards) * cards * step, 0); }; const total = found.places.length + found.more; @@ -114,7 +111,7 @@ export default function AssistantPlaceStrip({ found, message }: { found: Assista onScroll={measure} data-fade-start={fade.start || undefined} data-fade-end={fade.end || undefined} - className="trek-assistant-strip flex snap-x snap-mandatory scroll-px-4 gap-2.5 overflow-x-auto overscroll-x-contain px-4 pb-2 pt-0.5" + className="trek-assistant-strip flex snap-x snap-mandatory scroll-px-4 gap-2.5 overflow-x-auto overscroll-x-contain px-4 pb-2 pt-0.5 motion-safe:scroll-smooth" > {found.places.slice(0, SHOWN).map((place) => ( { const openWith = vi.fn(); useAssistantStore.setState({ status: null, send, openWith }); const louvre = buildPlace({ id: 5, name: 'Louvre' }); - const off = renderHook(() => useAssistantMenu(TRIP)); + const translated = ({ children }: { children: ReactNode }) => {children}; + const off = renderHook(() => useAssistantMenu(TRIP), { wrapper: translated }); expect(off.result.current.place(louvre)).toEqual([]); useAssistantStore.setState({ status: { available: true } as never }); - const on = renderHook(() => useAssistantMenu(TRIP)); + const on = renderHook(() => useAssistantMenu(TRIP), { wrapper: translated }); const rows = on.result.current.place(louvre); const alternative = rows.find((r) => 'label' in r && r.label === 'Find an alternative'); act(() => (alternative && 'onClick' in alternative ? alternative.onClick() : undefined)); diff --git a/client/src/components/Assistant/AssistantWeatherStrip.tsx b/client/src/components/Assistant/AssistantWeatherStrip.tsx index 2434d1650..9ce44222a 100644 --- a/client/src/components/Assistant/AssistantWeatherStrip.tsx +++ b/client/src/components/Assistant/AssistantWeatherStrip.tsx @@ -1,5 +1,6 @@ import type { AssistantWeatherDay } from '@trek/shared'; import { Droplet } from 'lucide-react'; +import { createElement } from 'react'; import { useTranslation } from '../../i18n'; import { chatOf, useAssistantStore } from '../../store/assistantStore'; @@ -31,7 +32,7 @@ function WeatherChip({ onPick: ((dayId: number) => void) | null; }) { const { t, locale } = useTranslation(); - const Icon = weatherIconFor(day.main); + const icon = weatherIconFor(day.main); const { weekday, day: dayOfMonth } = dateParts(day.date, locale); // A climate figure is an average for the date, not a forecast; it says so with "≈". const approx = day.kind === 'climate' ? '≈' : ''; @@ -45,7 +46,7 @@ function WeatherChip({ {weekday} {dayOfMonth} -