From 39113e12de9a2cfdd8d471735dd0eac39eea561d Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 31 May 2026 22:36:15 +0200 Subject: [PATCH] Name GPX routes and tracks after their source file so multiple imports stick (#1054) Unnamed routes and tracks all fell back to the same generic 'GPX Route' / 'GPX Track' label, so the name-based import dedup dropped every one after the first - importing several files (or one file with several tracks) only kept a single place. Derive the default name from the source filename with an index suffix when a file holds more than one geometry, thread the filename down through the controller, and let the import modal take more than one file at a time. Adds PLACE-SVC-037/038. --- .../components/Planner/FileImportModal.tsx | 164 +++++++++++------- server/src/nest/places/places.controller.ts | 2 +- server/src/nest/places/places.service.ts | 6 +- server/src/services/placeService.ts | 22 ++- .../tests/unit/services/placeService.test.ts | 33 ++++ 5 files changed, 156 insertions(+), 71 deletions(-) diff --git a/client/src/components/Planner/FileImportModal.tsx b/client/src/components/Planner/FileImportModal.tsx index e5b452f9..2e752afa 100644 --- a/client/src/components/Planner/FileImportModal.tsx +++ b/client/src/components/Planner/FileImportModal.tsx @@ -31,7 +31,7 @@ export default function FileImportModal({ isOpen, onClose, tripId, pushUndo, ini const loadTrip = useTripStore((s) => s.loadTrip) const fileInputRef = useRef(null) - const [file, setFile] = useState(null) + const [files, setFiles] = useState([]) const [isDragOver, setIsDragOver] = useState(false) const [loading, setLoading] = useState(false) const [error, setError] = useState('') @@ -51,7 +51,7 @@ export default function FileImportModal({ isOpen, onClose, tripId, pushUndo, ini } const reset = () => { - setFile(null) + setFiles([]) setIsDragOver(false) setLoading(false) setError('') @@ -67,14 +67,14 @@ export default function FileImportModal({ isOpen, onClose, tripId, pushUndo, ini if (initialFile) { const err = validateFile(initialFile) if (err) { - setFile(null) + setFiles([]) setError(err) } else { - setFile(initialFile) + setFiles([initialFile]) setError('') } } else { - setFile(null) + setFiles([]) setError('') } // validateFile uses t() which is stable — intentionally omitted from deps @@ -86,22 +86,32 @@ export default function FileImportModal({ isOpen, onClose, tripId, pushUndo, ini onClose() } - const selectFile = (f: File) => { - const validationError = validateFile(f) - if (validationError) { - setError(validationError) - setFile(null) + const selectFiles = (incoming: File[]) => { + if (incoming.length === 0) return + const valid: File[] = [] + let firstError: string | null = null + for (const f of incoming) { + const validationError = validateFile(f) + if (validationError) { + firstError = firstError ?? validationError + continue + } + valid.push(f) + } + if (valid.length === 0) { + setError(firstError ?? '') + setFiles([]) return } - setFile(f) - setError('') + setFiles(valid) + setError(firstError ?? '') setSummary(null) } const handleInputChange = (e: React.ChangeEvent) => { - const f = e.target.files?.[0] + const list = e.target.files ? Array.from(e.target.files) : [] e.target.value = '' - if (f) selectFile(f) + if (list.length) selectFiles(list) } const handleDragOver = (e: React.DragEvent) => { @@ -116,71 +126,92 @@ export default function FileImportModal({ isOpen, onClose, tripId, pushUndo, ini const handleDrop = (e: React.DragEvent) => { e.preventDefault() setIsDragOver(false) - const f = e.dataTransfer.files[0] - if (f) selectFile(f) + const list = Array.from(e.dataTransfer.files) + if (list.length) selectFiles(list) } const handleImport = async () => { - if (!file || loading) return - const ext = file.name.toLowerCase().split('.').pop() + if (files.length === 0 || loading) return setLoading(true) setError('') setSummary(null) - try { - if (ext === 'gpx') { - const result = await placesApi.importGpx(tripId, file, gpxOpts) - await loadTrip(tripId) - if (result.count === 0 && result.skipped > 0) { - toast.warning(t('places.importAllSkipped')) + let totalCreated = 0 + let totalSkipped = 0 + const createdIds: number[] = [] + const errors: string[] = [] + let mergedSummary: PlacesImportSummary | null = null + let importedGpx = false + let importedKml = false + + for (const f of files) { + const ext = f.name.toLowerCase().split('.').pop() + try { + if (ext === 'gpx') { + importedGpx = true + const result = await placesApi.importGpx(tripId, f, gpxOpts) + totalCreated += result.count ?? 0 + totalSkipped += result.skipped ?? 0 + if (result.places?.length > 0) createdIds.push(...result.places.map((p: { id: number }) => p.id)) } else { - toast.success(t('places.gpxImported', { count: result.count })) - } - if (result.places?.length > 0) { - const importedIds: number[] = result.places.map((p: { id: number }) => p.id) - pushUndo?.(t('undo.importGpx'), async () => { - try { await placesApi.bulkDelete(tripId, importedIds) } catch {} - await loadTrip(tripId) - }) - } - handleClose() - } else { - const result = await placesApi.importMapFile(tripId, file, kmlOpts) - await loadTrip(tripId) - setSummary(result.summary || null) - if (result.count === 0 && (result.summary?.skippedCount ?? 0) > 0) { - toast.warning(t('places.importAllSkipped')) - } else { - toast.success(t('places.kmlKmzImported', { count: result.count })) - } - if (result.summary?.errors?.length > 0) { - setError(result.summary.errors.join('\n')) - } - if (result.places?.length > 0) { - const importedIds: number[] = result.places.map((p: { id: number }) => p.id) - pushUndo?.(t('undo.importKeyholeMarkup'), async () => { - try { await placesApi.bulkDelete(tripId, importedIds) } catch {} - await loadTrip(tripId) - }) + importedKml = true + const result = await placesApi.importMapFile(tripId, f, kmlOpts) + totalCreated += result.count ?? 0 + if (result.places?.length > 0) createdIds.push(...result.places.map((p: { id: number }) => p.id)) + const s = result.summary as PlacesImportSummary | undefined + if (s) { + mergedSummary = mergedSummary + ? { + totalPlacemarks: mergedSummary.totalPlacemarks + s.totalPlacemarks, + createdCount: mergedSummary.createdCount + s.createdCount, + skippedCount: mergedSummary.skippedCount + s.skippedCount, + warnings: [...mergedSummary.warnings, ...(s.warnings ?? [])], + errors: [...mergedSummary.errors, ...(s.errors ?? [])], + } + : s + totalSkipped += s.skippedCount ?? 0 + } } + } catch (err: any) { + const message = err?.response?.data?.error || t('places.importFileError') + errors.push(files.length > 1 ? `${f.name}: ${message}` : message) } - } catch (err: any) { - const responseSummary = err?.response?.data?.summary as PlacesImportSummary | undefined - if (responseSummary) setSummary(responseSummary) - const message = err?.response?.data?.error || t('places.importFileError') - setError(message) - toast.error(message) - } finally { - setLoading(false) } + + await loadTrip(tripId) + + if (createdIds.length > 0) { + pushUndo?.(importedGpx && !importedKml ? t('undo.importGpx') : t('undo.importKeyholeMarkup'), async () => { + try { await placesApi.bulkDelete(tripId, createdIds) } catch {} + await loadTrip(tripId) + }) + } + + if (totalCreated > 0) { + const key = importedKml && !importedGpx ? 'places.kmlKmzImported' : 'places.gpxImported' + toast.success(t(key, { count: totalCreated })) + } else if (totalSkipped > 0 && errors.length === 0) { + toast.warning(t('places.importAllSkipped')) + } + + if (mergedSummary) setSummary(mergedSummary) + if (errors.length > 0) { + setError(errors.join('\n')) + toast.error(errors[0]) + } + + setLoading(false) + + // Close once everything succeeded and there's no KML summary left to surface. + if (errors.length === 0 && !mergedSummary) handleClose() } - const fileExt = file?.name.toLowerCase().split('.').pop() ?? '' - const isGpx = fileExt === 'gpx' - const isKml = fileExt === 'kml' || fileExt === 'kmz' + const exts = files.map(f => f.name.toLowerCase().split('.').pop() ?? '') + const isGpx = exts.includes('gpx') + const isKml = exts.some(e => e === 'kml' || e === 'kmz') const gpxNoneSelected = isGpx && !gpxOpts.waypoints && !gpxOpts.routes && !gpxOpts.tracks const kmlNoneSelected = isKml && !kmlOpts.points && !kmlOpts.paths - const canImport = !!file && !loading && !gpxNoneSelected && !kmlNoneSelected + const canImport = files.length > 0 && !loading && !gpxNoneSelected && !kmlNoneSelected if (!isOpen) return null @@ -206,6 +237,7 @@ export default function FileImportModal({ isOpen, onClose, tripId, pushUndo, ini ref={fileInputRef} type="file" accept=".gpx,.kml,.kmz" + multiple style={{ display: 'none' }} onChange={handleInputChange} /> @@ -240,8 +272,8 @@ export default function FileImportModal({ isOpen, onClose, tripId, pushUndo, ini {isDragOver ? ( {t('places.importFileDropActive')} - ) : file ? ( - {file.name} + ) : files.length > 0 ? ( + {files.map(f => f.name).join(', ')} ) : ( {t('places.importFileDropHere')} )} diff --git a/server/src/nest/places/places.controller.ts b/server/src/nest/places/places.controller.ts index 744815c6..7e930e3e 100644 --- a/server/src/nest/places/places.controller.ts +++ b/server/src/nest/places/places.controller.ts @@ -117,7 +117,7 @@ export class PlacesController { if (!importWaypoints && !importRoutes && !importTracks) { throw new HttpException({ error: 'No import types selected' }, 400); } - const result = this.places.importGpx(tripId, file.buffer, { importWaypoints, importRoutes, importTracks }); + const result = this.places.importGpx(tripId, file.buffer, { importWaypoints, importRoutes, importTracks, defaultName: file.originalname }); if (!result) { throw new HttpException({ error: 'No matching places found in GPX file' }, 400); } diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 909f8483..eedf767d 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -52,7 +52,11 @@ export class PlacesService { return svc.deletePlacesMany(tripId, ids); } - importGpx(tripId: string, buffer: Buffer, opts: { importWaypoints: boolean; importRoutes: boolean; importTracks: boolean }) { + importGpx( + tripId: string, + buffer: Buffer, + opts: { importWaypoints: boolean; importRoutes: boolean; importTracks: boolean; defaultName?: string }, + ) { return svc.importGpx(tripId, buffer, opts); } diff --git a/server/src/services/placeService.ts b/server/src/services/placeService.ts index d5dd7810..ed91c3eb 100644 --- a/server/src/services/placeService.ts +++ b/server/src/services/placeService.ts @@ -346,6 +346,8 @@ export interface GpxImportOptions { importWaypoints?: boolean; importRoutes?: boolean; importTracks?: boolean; + /** Source filename used to name unnamed routes/tracks (keeps multiple imports distinct). */ + defaultName?: string; } export interface KmlImportOptions { @@ -354,7 +356,7 @@ export interface KmlImportOptions { } export function importGpx(tripId: string, fileBuffer: Buffer, opts: GpxImportOptions = {}) { - const { importWaypoints = true, importRoutes = true, importTracks = true } = opts; + const { importWaypoints = true, importRoutes = true, importTracks = true, defaultName } = opts; const parsed = gpxParser.parse(fileBuffer.toString('utf-8')); const gpx = parsed?.gpx; @@ -363,6 +365,20 @@ export function importGpx(tripId: string, fileBuffer: Buffer, opts: GpxImportOpt const str = (v: unknown) => (v != null ? String(v).trim() : null); const num = (v: unknown) => { const n = parseFloat(String(v)); return isNaN(n) ? null : n; }; + // Routes and tracks rarely carry their own . Without one they all fall back to the + // same generic label, so name-based dedup drops every import after the first. Derive a + // base from the source filename (the requested behaviour) and suffix an index so multiple + // geometries from one file stay distinct. + const rawName = str(defaultName); + const baseName = rawName ? rawName.replace(/\.[^.]+$/, '').trim() || rawName : null; + let geoSeq = 0; + const geoName = (explicit: string | null, fallback: string): string => { + if (explicit) return explicit; + geoSeq++; + const base = baseName || fallback; + return geoSeq === 1 ? base : `${base} ${geoSeq}`; + }; + type WaypointEntry = { name: string; lat: number; lng: number; description: string | null; routeGeometry?: string }; const waypoints: WaypointEntry[] = []; @@ -385,7 +401,7 @@ export function importGpx(tripId: string, fileBuffer: Buffer, opts: GpxImportOpt if (pts.length === 0) continue; const hasAllEle = pts.every(p => p.ele !== null); const routeGeometry = pts.map(p => hasAllEle ? [p.lat, p.lng, p.ele] : [p.lat, p.lng]); - waypoints.push({ lat: pts[0].lat, lng: pts[0].lng, name: str(rte.name) || 'GPX Route', description: str(rte.desc), routeGeometry: JSON.stringify(routeGeometry) }); + waypoints.push({ lat: pts[0].lat, lng: pts[0].lng, name: geoName(str(rte.name), 'GPX Route'), description: str(rte.desc), routeGeometry: JSON.stringify(routeGeometry) }); } } @@ -405,7 +421,7 @@ export function importGpx(tripId: string, fileBuffer: Buffer, opts: GpxImportOpt const start = trackPoints[0]; const hasAllEle = trackPoints.every(p => p.ele !== null); const routeGeometry = trackPoints.map(p => hasAllEle ? [p.lat, p.lng, p.ele] : [p.lat, p.lng]); - waypoints.push({ lat: start.lat, lng: start.lng, name: str(trk.name) || 'GPX Track', description: str(trk.desc), routeGeometry: JSON.stringify(routeGeometry) }); + waypoints.push({ lat: start.lat, lng: start.lng, name: geoName(str(trk.name), 'GPX Track'), description: str(trk.desc), routeGeometry: JSON.stringify(routeGeometry) }); } } diff --git a/server/tests/unit/services/placeService.test.ts b/server/tests/unit/services/placeService.test.ts index 00fb6920..5bec43d7 100644 --- a/server/tests/unit/services/placeService.test.ts +++ b/server/tests/unit/services/placeService.test.ts @@ -346,6 +346,39 @@ describe('importGpx', () => { const result = importGpx(String(trip.id), gpx); expect(result).toBeNull(); }); + + it('PLACE-SVC-037 — multiple unnamed tracks in one file get distinct names instead of collapsing to one', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const gpx = Buffer.from(` + + + + + + + + + `); + const result = importGpx(String(trip.id), gpx) as any; + expect(result.places).toHaveLength(2); + const names = result.places.map((p: any) => p.name); + expect(new Set(names).size).toBe(2); + }); + + it('PLACE-SVC-038 — unnamed tracks fall back to the source filename', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const gpx = Buffer.from(` + + + + + `); + const result = importGpx(String(trip.id), gpx, { defaultName: 'morning-hike.gpx' }) as any; + expect(result.places).toHaveLength(1); + expect(result.places[0].name).toBe('morning-hike'); + }); }); // ── importGoogleList ──────────────────────────────────────────────────────────