fix: receipts, cached places, chat previews and the Google-route tool

Saving an expense twice while its receipt is also attached to a place and a
booking hit the new unique index on (file, expense): the row kept from the
first save was skipped, the next spare link row was adopted into a duplicate
pair, and the constraint threw inside the transaction. The whole edit rolled
back with a 500, every time, so that expense could never be changed again. The
adopt now skips a link the expense already holds, and a receipt named twice in
one save is linked once.

The offline place cache was keyed on the place id alone while it is owned and
evicted per trip, so two trips over the same city took each other's rows: the
second write claimed them, switching the second trip off deleted them for both,
and the first trip's fingerprint still matched so nothing ever refilled them.
Offline search then answered nothing for a trip whose switch read "on". Each
trip keeps its own copies now, and the upgrade clears the fingerprints so the
next sync refills what the rebuild dropped.

The chat composer minted its object URLs inside a state updater, which React
runs twice in development, so one picked image drew two thumbnails and removing
one left a preview for a picture that would not be sent. File and URL are one
record now and every mutator commits a finished list.

import_google_maps_route wrote up to thirty places and their day assignments
under trips:write, which does not imply the places:write that create_place is
gated on, and it was the one non-admin write tool with no demo gate and the
only tool in the repo with no annotations. All three corrected.
This commit is contained in:
Maurice
2026-09-13 22:55:15 +02:00
parent f0e4c9a33d
commit 9deedb80ea
9 changed files with 292 additions and 54 deletions
@@ -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'])
})
})
+44 -38
View File
@@ -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<File[]>([])
const [previews, setPreviews] = useState<string[]>([])
const urls = useRef<string[]>([])
const [items, setItems] = useState<PickedImage[]>([])
const itemsRef = useRef<PickedImage[]>([])
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 }
}
+20 -1
View File
@@ -178,7 +178,7 @@ class TrekOfflineDb extends Dexie {
syncMeta!: Table<SyncMeta, number>;
blobCache!: Table<BlobCacheEntry, string>;
importFiles!: Table<ImportSourceFile, [string, string]>;
areaPlaces!: Table<CachedAreaPlace, string>;
areaPlaces!: Table<CachedAreaPlace, [string, number]>;
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;
});
});
}
}
+31
View File
@@ -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)])
+26 -7
View File
@@ -183,14 +183,30 @@ export async function searchCachedPlaces(query: string, limit = 10): Promise<Cac
// Prefix hits come off the index; the substring pass is the fallback for
// "osteria" matching "L'Osteria Rostock", which no prefix index can answer.
const byPrefix = await offlineDb.areaPlaces.where('searchName').startsWith(needle).limit(limit).toArray()
// Deduplicated on the GERS id: a place inside two cached trips' areas is
// stored once per trip, and a search that spans every trip would otherwise
// hand the same restaurant back twice.
const seen = new Set<string>()
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<Cac
*/
export async function getCachedPlace(placeId: string): Promise<CachedAreaPlace | null> {
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<string, unknown> {
+14 -3
View File
@@ -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.
+14 -4
View File
@@ -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));
}
}
@@ -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);
@@ -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) };