diff --git a/client/src/components/Collab/useChatImages.test.ts b/client/src/components/Collab/useChatImages.test.ts new file mode 100644 index 000000000..6f309a7e2 --- /dev/null +++ b/client/src/components/Collab/useChatImages.test.ts @@ -0,0 +1,95 @@ +/** + * FE-CHATIMG-001..006 — the pictures pinned to a chat message before it is sent. + * + * The reason this file exists: the hook used to create object URLs and enqueue + * the preview state from inside a `setFiles` updater. React invokes an updater + * an extra time in development, so one picked image produced two preview URLs + * against one file, the composer strip drew two thumbnails for one attachment, + * and removing one of them left a preview for a picture that would not be sent. + * `renderHook` under StrictMode is what reproduces it. + */ +import { describe, it, expect, beforeEach, vi } from 'vitest' +import { StrictMode } from 'react' +import { act, renderHook } from '@testing-library/react' +import { useChatImages, MAX_CHAT_IMAGES } from './useChatImages' + +let nextUrl = 0 +const revoked: string[] = [] + +beforeEach(() => { + nextUrl = 0 + revoked.length = 0 + vi.stubGlobal('URL', { + ...URL, + createObjectURL: vi.fn(() => `blob:mock/${++nextUrl}`), + revokeObjectURL: vi.fn((url: string) => { revoked.push(url) }), + }) +}) + +const img = (name: string, size = 1024) => + new File([new Uint8Array(size)], name, { type: 'image/png' }) + +const strict = () => renderHook(() => useChatImages(), { wrapper: StrictMode }) + +describe('useChatImages', () => { + it('FE-CHATIMG-001: one picked image is one file and one preview, in StrictMode', () => { + const { result } = strict() + + act(() => { result.current.add([img('a.png')]) }) + + expect(result.current.files).toHaveLength(1) + expect(result.current.previews).toHaveLength(1) + expect(result.current.previews[0]).toBe('blob:mock/1') + }) + + it('FE-CHATIMG-002: removing the only image empties both lists and revokes its URL', () => { + const { result } = strict() + act(() => { result.current.add([img('a.png')]) }) + + act(() => { result.current.remove(0) }) + + expect(result.current.files).toEqual([]) + expect(result.current.previews).toEqual([]) + expect(revoked).toEqual(['blob:mock/1']) + }) + + it('FE-CHATIMG-003: files and previews stay in step across several adds', () => { + const { result } = strict() + + act(() => { result.current.add([img('a.png')]) }) + act(() => { result.current.add([img('b.png'), img('c.png')]) }) + + expect(result.current.files.map(f => f.name)).toEqual(['a.png', 'b.png', 'c.png']) + expect(result.current.previews).toEqual(['blob:mock/1', 'blob:mock/2', 'blob:mock/3']) + }) + + it('FE-CHATIMG-004: a rejected type is reported and nothing is pinned for it', () => { + const { result } = strict() + let ok = true + + act(() => { ok = result.current.add([new File(['x'], 'notes.pdf', { type: 'application/pdf' })]) }) + + expect(ok).toBe(false) + expect(result.current.files).toEqual([]) + expect(result.current.previews).toEqual([]) + }) + + it('FE-CHATIMG-005: the cap holds and no URL is minted for what it turns away', () => { + const { result } = strict() + + act(() => { result.current.add(Array.from({ length: MAX_CHAT_IMAGES + 2 }, (_, i) => img(`${i}.png`))) }) + + expect(result.current.files).toHaveLength(MAX_CHAT_IMAGES) + expect(result.current.previews).toHaveLength(MAX_CHAT_IMAGES) + }) + + it('FE-CHATIMG-006: clear revokes every URL it handed out', () => { + const { result } = strict() + act(() => { result.current.add([img('a.png'), img('b.png')]) }) + + act(() => { result.current.clear() }) + + expect(result.current.files).toEqual([]) + expect(revoked.sort()).toEqual(['blob:mock/1', 'blob:mock/2']) + }) +}) diff --git a/client/src/components/Collab/useChatImages.ts b/client/src/components/Collab/useChatImages.ts index 2cca6a19c..29ac30411 100644 --- a/client/src/components/Collab/useChatImages.ts +++ b/client/src/components/Collab/useChatImages.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useRef, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' /** * The images pinned to a chat message before it is sent. @@ -7,16 +7,27 @@ import { useCallback, useEffect, useRef, useState } from 'react' * their own copy of the same picking, previewing and clearing, which is two * places for the same leak and a duplication bill on every line. * - * Object URLs are held in a ref and revoked when the file they belong to goes, - * when the list is cleared and on unmount. Rebuilding every preview on each - * change, which is what both copies did, leaked one URL per image per keystroke - * of picking, and did it from inside a setState updater, so React ran it twice - * in development and leaked twice as fast. + * A file and the object URL that previews it are one thing, so they are stored + * as one thing and the two arrays the callers read are derived from it. Every + * mutator computes the next list first and then commits it: nothing is created + * or revoked inside a state updater, because React invokes those an extra time + * in development and the second run would mint a second URL for the same file. + * That is what the strip showed as two thumbnails for one attachment, and + * removing one of them left a preview for a picture that was never sent. + * + * The ref is the list, the state is the render of it. Written together so two + * calls in the same tick see each other, which a functional updater would give + * for free and a plain array would not. */ export const CHAT_IMAGE_TYPES = ['image/jpeg', 'image/png', 'image/gif', 'image/webp'] export const MAX_CHAT_IMAGES = 4 export const MAX_CHAT_IMAGE_BYTES = 10 * 1024 * 1024 +interface PickedImage { + file: File + url: string +} + export interface ChatImages { files: File[] previews: string[] @@ -27,51 +38,46 @@ export interface ChatImages { } export function useChatImages(): ChatImages { - const [files, setFiles] = useState([]) - const [previews, setPreviews] = useState([]) - const urls = useRef([]) + const [items, setItems] = useState([]) + const itemsRef = useRef([]) - const revokeAll = useCallback(() => { - urls.current.forEach(URL.revokeObjectURL) - urls.current = [] + const commit = useCallback((next: PickedImage[]) => { + itemsRef.current = next + setItems(next) }, []) - useEffect(() => revokeAll, [revokeAll]) + // Unmount with pictures still pinned: the URLs outlive the component otherwise. + useEffect(() => () => { + itemsRef.current.forEach(item => URL.revokeObjectURL(item.url)) + itemsRef.current = [] + }, []) const add = useCallback((incoming: FileList | File[]) => { const all = Array.from(incoming) const valid = all.filter(f => CHAT_IMAGE_TYPES.includes(f.type) && f.size <= MAX_CHAT_IMAGE_BYTES) - if (valid.length) { - setFiles(prev => { - const room = MAX_CHAT_IMAGES - prev.length - const added = valid.slice(0, Math.max(0, room)) - // Only the new files get a URL; the ones already in the list keep theirs. - const fresh = added.map(URL.createObjectURL) - urls.current = [...urls.current, ...fresh] - setPreviews(p => [...p, ...fresh]) - return [...prev, ...added] - }) + const room = MAX_CHAT_IMAGES - itemsRef.current.length + const added = valid.slice(0, Math.max(0, room)) + if (added.length) { + // Only the new files get a URL; the ones already in the list keep theirs. + commit([...itemsRef.current, ...added.map(file => ({ file, url: URL.createObjectURL(file) }))]) } return valid.length === all.length - }, []) + }, [commit]) const remove = useCallback((index: number) => { - setFiles(prev => prev.filter((_, i) => i !== index)) - setPreviews(prev => { - const gone = prev[index] - if (gone) { - URL.revokeObjectURL(gone) - urls.current = urls.current.filter(u => u !== gone) - } - return prev.filter((_, i) => i !== index) - }) - }, []) + const gone = itemsRef.current[index] + if (!gone) return + URL.revokeObjectURL(gone.url) + commit(itemsRef.current.filter((_, i) => i !== index)) + }, [commit]) const clear = useCallback(() => { - revokeAll() - setFiles([]) - setPreviews([]) - }, [revokeAll]) + itemsRef.current.forEach(item => URL.revokeObjectURL(item.url)) + commit([]) + }, [commit]) + + const files = useMemo(() => items.map(item => item.file), [items]) + const previews = useMemo(() => items.map(item => item.url), [items]) return { files, previews, add, remove, clear } } diff --git a/client/src/db/offlineDb.ts b/client/src/db/offlineDb.ts index 3bce5333f..97f641278 100644 --- a/client/src/db/offlineDb.ts +++ b/client/src/db/offlineDb.ts @@ -178,7 +178,7 @@ class TrekOfflineDb extends Dexie { syncMeta!: Table; blobCache!: Table; importFiles!: Table; - areaPlaces!: Table; + areaPlaces!: Table; constructor(name: string = ANON_DB_NAME) { super(name); @@ -227,6 +227,25 @@ class TrekOfflineDb extends Dexie { this.version(5).stores({ areaPlaces: 'gers, tripId, searchName', }); + + // v7/v8: the same place near two trips is two rows now. + // + // Keyed on the GERS id alone, one row could only ever name one trip, and a + // bulkPut for the second trip rewrote the first trip's rows to point at it. + // Switching the second trip off then deleted the shared set by tripId, and + // the first trip's areaPlacesKey still matched its bbox, so nothing ever + // downloaded them again: a trip whose switch read "on" with no offline + // search behind it. Dexie cannot change a primary key in place, so the + // table is dropped and rebuilt, and every stored fingerprint is cleared so + // the next sync refills what the drop took. + this.version(7).stores({ areaPlaces: null }); + this.version(8).stores({ + areaPlaces: '[gers+tripId], gers, tripId, searchName', + }).upgrade(async (tx) => { + await tx.table('syncMeta').toCollection().modify((row: { areaPlacesKey?: string }) => { + delete row.areaPlacesKey; + }); + }); } } diff --git a/client/src/sync/placePrefetcher.test.ts b/client/src/sync/placePrefetcher.test.ts index d69f9b84b..299b62697 100644 --- a/client/src/sync/placePrefetcher.test.ts +++ b/client/src/sync/placePrefetcher.test.ts @@ -92,6 +92,37 @@ describe('prefetchPlacesForTrip', () => { expect(areaMock).toHaveBeenCalledTimes(1) }) + it('FE-PLACEPRE-005b: two trips over the same city keep their own copies', async () => { + // Keyed on the GERS id alone, the second trip's write rewrote the first + // trip's rows to point at itself, and switching the second trip off then + // deleted them for both. + await offlineDb.syncMeta.put({ + tripId: 8, lastSyncedAt: null, status: 'idle', tilesBbox: null, filesCachedCount: 0, + }) + await prefetchPlacesForTrip(7, TRIP_PLACES) + await prefetchPlacesForTrip(8, TRIP_PLACES) + + const rows = await offlineDb.areaPlaces.toArray() + expect(rows).toHaveLength(4) + expect(rows.filter((r) => r.tripId === 7)).toHaveLength(2) + expect(rows.filter((r) => r.tripId === 8)).toHaveLength(2) + }); + + it('FE-PLACEPRE-005c: dropping one trip leaves the other trip searchable', async () => { + await offlineDb.syncMeta.put({ + tripId: 8, lastSyncedAt: null, status: 'idle', tilesBbox: null, filesCachedCount: 0, + }) + await prefetchPlacesForTrip(7, TRIP_PLACES) + await prefetchPlacesForTrip(8, TRIP_PLACES) + + await offlineDb.areaPlaces.where('tripId').equals(8).delete() + + // Offline search spans every cached trip, and trip 7 never asked to lose + // anything. Its fingerprint also still matches, so nothing would refill it. + const hits = await searchCachedPlaces('osteria') + expect(hits.map((h) => h.name)).toEqual(["L'Osteria"]) + }); + it('FE-PLACEPRE-005: re-downloads when the trip moves', async () => { await prefetchPlacesForTrip(7, TRIP_PLACES) await prefetchPlacesForTrip(7, [place(41.9, 12.5), place(41.92, 12.52)]) diff --git a/client/src/sync/placePrefetcher.ts b/client/src/sync/placePrefetcher.ts index 6ac66ef28..c8b0f9b98 100644 --- a/client/src/sync/placePrefetcher.ts +++ b/client/src/sync/placePrefetcher.ts @@ -183,14 +183,30 @@ export async function searchCachedPlaces(query: string, limit = 10): Promise() + const take = (rows: CachedAreaPlace[]) => { + const out: CachedAreaPlace[] = [] + for (const row of rows) { + if (seen.has(row.gers)) continue + seen.add(row.gers) + out.push(row) + if (seen.size >= limit) break + } + return out + } + + const byPrefix = take(await offlineDb.areaPlaces.where('searchName').startsWith(needle).limit(limit * 2).toArray()) if (byPrefix.length >= limit) return byPrefix - const seen = new Set(byPrefix.map((p) => p.gers)) - const rest = await offlineDb.areaPlaces - .filter((p) => !seen.has(p.gers) && p.searchName.includes(needle)) - .limit(limit - byPrefix.length) - .toArray() + const rest = take( + await offlineDb.areaPlaces + .filter((p) => !seen.has(p.gers) && p.searchName.includes(needle)) + .limit((limit - byPrefix.length) * 2) + .toArray(), + ) return [...byPrefix, ...rest] } @@ -206,7 +222,10 @@ export async function searchCachedPlaces(query: string, limit = 10): Promise { if (!placeId.startsWith('gers:')) return null - return (await offlineDb.areaPlaces.get(placeId.slice('gers:'.length))) ?? null + // Through the `gers` index rather than the primary key: the key is compound + // now, and which trip cached the row is not something the caller knows or + // should have to know. + return (await offlineDb.areaPlaces.where('gers').equals(placeId.slice('gers:'.length)).first()) ?? null } export function cachedToPlaceRecord(p: CachedAreaPlace): Record { diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index 186810b91..b2a6272f1 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -639,14 +639,25 @@ export class BudgetService { if (data.receipt_file_ids !== undefined) { // The ids that survive are left linked rather than dropped and re-added, // so a receipt the request keeps never loses its row for an instant. - const keep = new Set(data.receipt_file_ids.map(Number)); + // Deduplicated: the same id twice would make the second pass adopt a + // second spare row into the pair the first one just wrote. + const wanted = [...new Set(data.receipt_file_ids.map(Number))]; + const keep = new Set(wanted); this.unlinkReceipts(id, keep); - if (data.receipt_file_ids.length > 0) { + if (wanted.length > 0) { const insertLink = this.db.prepare('INSERT OR IGNORE INTO file_links (file_id, budget_item_id) VALUES (?, ?)'); const adopt = this.db.prepare('UPDATE file_links SET budget_item_id = ? WHERE id = ?'); - for (const fid of data.receipt_file_ids) { + for (const fid of wanted) { const belongs = this.db.get('SELECT id FROM trip_files WHERE id = ? AND trip_id = ? AND deleted_at IS NULL', fid, tripId); if (!belongs) continue; + // Already this item's receipt: nothing to do. A file can carry several + // link rows (one per place, one per booking), so on a second save of + // the same expense the row kept from last time is skipped by + // unlinkReceipts and the next spare would be adopted into a duplicate + // (file, item) pair, which the unique index refuses. That threw inside + // the transaction and rolled the whole expense edit back, every time. + const already = this.db.get('SELECT 1 FROM file_links WHERE file_id = ? AND budget_item_id = ?', fid, id); + if (already) continue; // A file already tied to a place or a booking gets the receipt link // written onto that row, because the unique index is per file and // item and a second row for the same pair would be refused anyway. diff --git a/server/src/nest/roadtrip/google-route.mcp.ts b/server/src/nest/roadtrip/google-route.mcp.ts index 5198f6dc5..6bd0a7c5a 100644 --- a/server/src/nest/roadtrip/google-route.mcp.ts +++ b/server/src/nest/roadtrip/google-route.mcp.ts @@ -1,14 +1,16 @@ import { z } from 'zod'; import { googleRouteImportSchema, googleRoutePreviewRequestSchema, type GoogleRouteImport } from '@trek/shared'; -import { McpController, Tool, TOOL_ANNOTATIONS_READONLY, ok, type McpContext } from '../../nest-mcp'; +import { McpController, Tool, TOOL_ANNOTATIONS_READONLY, TOOL_ANNOTATIONS_NON_IDEMPOTENT, ok, type McpContext } from '../../nest-mcp'; import { ADDON_IDS } from '../../addons'; import { AddonsService } from '../addons/addons.service'; import { addonGate } from '../addons/addon-gate'; import { GoogleRouteService } from './google-route.service'; +import { AuthService } from '../auth/auth.service'; +import { demoDenied } from '../../mcp/tools/_shared'; @McpController() export class GoogleRouteMcp { - constructor(private readonly routes: GoogleRouteService, readonly addons: AddonsService) {} + constructor(private readonly routes: GoogleRouteService, private readonly auth: AuthService, readonly addons: AddonsService) {} @Tool({ name: 'preview_google_maps_route', description: 'Read ordered stops from a Google Maps directions link. No changes are saved. Review geocoded positions and unresolved stops before importing. The exact Google road geometry is not imported.', inputSchema: googleRoutePreviewRequestSchema.shape, annotations: TOOL_ANNOTATIONS_READONLY, access: { group: 'trips', mode: 'read' }, when: addonGate(ADDON_IDS.ROADTRIP) }) @@ -16,6 +18,14 @@ export class GoogleRouteMcp { @Tool({ name: 'import_google_maps_route', description: 'Append reviewed Google Maps stops to an existing trip day in supplied order. Creates places and visits atomically. Requires place and day editing permissions. Existing visits remain. TREK calculates the road geometry; no Google route geometry is preserved.', inputSchema: { tripId: z.number().int().positive(), ...googleRouteImportSchema.shape }, - access: { group: 'trips', mode: 'write' }, when: addonGate(ADDON_IDS.ROADTRIP) }) - import(input: GoogleRouteImport & { tripId: number }, ctx: McpContext) { return ok(this.routes.import(input.tripId, ctx.userId, input)); } + annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, + // What the tool writes, not where the link came from: this inserts up to + // thirty places and their day assignments, which is exactly what create_place + // and create_and_assign_place are gated on. trips:write does not imply it, and + // a client that withheld places:write was getting an itinerary written anyway. + access: { group: 'places', mode: 'write' }, when: addonGate(ADDON_IDS.ROADTRIP) }) + import(input: GoogleRouteImport & { tripId: number }, ctx: McpContext) { + if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); + return ok(this.routes.import(input.tripId, ctx.userId, input)); + } } diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index b5950cc76..97ad73ae5 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -1046,6 +1046,39 @@ describe('an expense whose split leaves a remainder', () => { expect(testDb.prepare('SELECT COUNT(*) c FROM file_links WHERE file_id = ?').get(f4)).toEqual({ c: 1 }); }); + it('re-saving an expense whose receipt is also linked elsewhere twice does not 500', () => { + // A file may carry one link row per place and one per booking, and the receipt + // link is written onto a spare one of those. On the second save the row kept + // from last time is skipped, so the next spare used to be adopted into a + // second (file, item) pair — refused by the unique index, thrown inside the + // transaction, and the whole expense edit rolled back. Every time, for good. + const { user: alice } = createUser(testDb); + const trip = createTrip(testDb, alice.id); + const file = Number(testDb.prepare('INSERT INTO trip_files (trip_id, filename, original_name) VALUES (?, ?, ?)').run(trip.id, 'r.jpg', 'r.jpg').lastInsertRowid); + const place = testDb.prepare('INSERT INTO places (trip_id, name) VALUES (?, ?)').run(trip.id, 'Osteria').lastInsertRowid; + const reservation = testDb.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Table', 'restaurant')").run(trip.id).lastInsertRowid; + testDb.prepare('INSERT INTO file_links (file_id, place_id) VALUES (?, ?)').run(file, place); + testDb.prepare('INSERT INTO file_links (file_id, reservation_id) VALUES (?, ?)').run(file, reservation); + + const item = budget.createBudgetItem(trip.id, { name: 'Dinner', total_price: 40, receipt_file_ids: [file] }); + const again = budget.updateBudgetItem(item.id, trip.id, { total_price: 42, receipt_file_ids: [file] }); + + expect(again?.total_price).toBe(42); + expect(again?.receipts?.map(r => r.id)).toEqual([file]); + expect(testDb.prepare('SELECT COUNT(*) c FROM file_links WHERE file_id = ? AND budget_item_id = ?').get(file, item.id)).toEqual({ c: 1 }); + }); + + it('a receipt named twice in one save is linked once', () => { + const { user: alice } = createUser(testDb); + const trip = createTrip(testDb, alice.id); + const file = Number(testDb.prepare('INSERT INTO trip_files (trip_id, filename, original_name) VALUES (?, ?, ?)').run(trip.id, 'dup.jpg', 'dup.jpg').lastInsertRowid); + const item = budget.createBudgetItem(trip.id, { name: 'Taxi' }); + + const updated = budget.updateBudgetItem(item.id, trip.id, { receipt_file_ids: [file, file] }); + + expect(updated?.receipts?.map(r => r.id)).toEqual([file]); + }); + it('unlinks receipts on deleteBudgetItem and leaves every file in place', () => { const { user: alice } = createUser(testDb); const trip = createTrip(testDb, alice.id); diff --git a/server/tests/unit/nest/roadtrip-read-tools.test.ts b/server/tests/unit/nest/roadtrip-read-tools.test.ts index cc953d5c9..df3b4a4ca 100644 --- a/server/tests/unit/nest/roadtrip-read-tools.test.ts +++ b/server/tests/unit/nest/roadtrip-read-tools.test.ts @@ -22,13 +22,27 @@ describe('roadtrip read tools', () => { }); it('passes reviewed Google stops and the authenticated user to the importer', async () => { const routes = { preview: vi.fn(async () => ({ stops: [] })), import: vi.fn(() => ({ imported: 2 })) }; - const tool = new GoogleRouteMcp(routes as never, {} as never); + const auth = { isDemoUser: vi.fn(() => false) }; + const tool = new GoogleRouteMcp(routes as never, auth as never, {} as never); await tool.preview({ url: 'https://www.google.com/maps/dir/A/B' }); expect(routes.preview).toHaveBeenCalledWith('https://www.google.com/maps/dir/A/B'); const input = { tripId: 10, dayId: 1, stops: [{ name: 'A', lat: 1, lng: 2 }] }; expect(JSON.stringify(tool.import(input, ctx))).toContain('2'); expect(routes.import).toHaveBeenCalledWith(10, 5, input); }); + + it('refuses the Google import for the demo account, like every other write tool', () => { + // The one non-admin write tool that had no gate: a demo session could write + // thirty places and their assignments onto the shared demo trip. + const routes = { preview: vi.fn(), import: vi.fn() }; + const auth = { isDemoUser: vi.fn(() => true) }; + const tool = new GoogleRouteMcp(routes as never, auth as never, {} as never); + + const res = tool.import({ tripId: 10, dayId: 1, stops: [{ name: 'A', lat: 1, lng: 2 }] } as never, ctx); + + expect(res.isError).toBe(true); + expect(routes.import).not.toHaveBeenCalled(); + }); it('does not expose manual boundaries to nonmembers', async () => { const boundaries = { list: vi.fn(() => [{ day_number: 1 }]) }; const db = { canAccessTrip: vi.fn(() => false) };