From 99278fd3058f9be53c72c7654f5d5106ed1fcbc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ullrich=20Sch=C3=A4fer?= Date: Tue, 14 Jul 2026 00:06:35 +0200 Subject: [PATCH 1/3] feat(gpx): lenient point parsing + route () support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task groups 1–2 of gpx-parser-robustness. The parser trusted its input: `parseFloat(attr ?? "0")` turned a missing lat/lon into a 0,0 Null Island point (which passes range validation) and garbage into NaN that poisoned distance and gain/loss totals; ``/`rtept` files (Garmin courses, many exporters) parsed to zero track points and were rejected. - `parsePoint`: skip a trkpt/rtept whose lat/lon is missing or non-finite (no more 0,0 default); a non-finite `` becomes `undefined` so it never leaks NaN into totals. Parsing stays parseFloat-lenient (trailing junk like `471.0m` still accepted), gated by Number.isFinite. - Drop segments left with fewer than 2 points (render nothing / break distance math). - Parse `` as track segments appended after `` segments, rtept handled identically — route-only files now import. No GpxData shape change; well-formed files parse identically. Updated the geom single-point test to the new drop-invariant. Verified: gpx typecheck + lint clean, 76/76 tests pass, journal + planner typecheck unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../changes/gpx-parser-robustness/tasks.md | 10 +- packages/gpx/src/geom.test.ts | 7 +- packages/gpx/src/parse.test.ts | 129 ++++++++++++++++++ packages/gpx/src/parse.ts | 64 ++++++--- 4 files changed, 184 insertions(+), 26 deletions(-) diff --git a/openspec/changes/gpx-parser-robustness/tasks.md b/openspec/changes/gpx-parser-robustness/tasks.md index e053b89..0eaaba4 100644 --- a/openspec/changes/gpx-parser-robustness/tasks.md +++ b/openspec/changes/gpx-parser-robustness/tasks.md @@ -1,13 +1,13 @@ ## 1. Parser lenience -- [ ] 1.1 Add a finite-number coordinate gate in `packages/gpx/src/parse.ts`: skip `trkpt`/`rtept` with missing or non-finite `lat`/`lon`; treat non-finite `` as undefined -- [ ] 1.2 Drop segments with fewer than 2 surviving points -- [ ] 1.3 Unit tests: missing-lat point skipped, garbage lat skipped with finite stats, garbage ele → undefined with finite gain/loss, single-point segment dropped, well-formed files byte-identical output to before +- [x] 1.1 Add a finite-number coordinate gate in `packages/gpx/src/parse.ts`: skip `trkpt`/`rtept` with missing or non-finite `lat`/`lon`; treat non-finite `` as undefined +- [x] 1.2 Drop segments with fewer than 2 surviving points +- [x] 1.3 Unit tests: missing-lat point skipped, garbage lat skipped with finite stats, garbage ele → undefined with finite gain/loss, single-point segment dropped, well-formed files byte-identical output to before ## 2. Route (``) support -- [ ] 2.1 Parse ``/`rtept` as track segments (appended after `` segments), reusing the same point parsing and lenience -- [ ] 2.2 Unit tests: route-only file yields one segment and passes `validateGpx`; mixed trk+rte ordering; rtept with ele/time preserved +- [x] 2.1 Parse ``/`rtept` as track segments (appended after `` segments), reusing the same point parsing and lenience +- [x] 2.2 Unit tests: route-only file yields one segment and passes `validateGpx`; mixed trk+rte ordering; rtept with ele/time preserved ## 3. Timestamp repair diff --git a/packages/gpx/src/geom.test.ts b/packages/gpx/src/geom.test.ts index a1521ed..149e88d 100644 --- a/packages/gpx/src/geom.test.ts +++ b/packages/gpx/src/geom.test.ts @@ -56,7 +56,10 @@ describe("GPX to geometry coordinates", () => { const gpxData = await parseGpxAsync(singlePointGpx); const coords = gpxData.tracks.flat().map((p) => [p.lon, p.lat] as [number, number]); - expect(coords).toHaveLength(1); - // Caller should check coords.length >= 2 before creating LineString + // The parser now drops segments left with fewer than 2 points + // (gpx-parser-robustness "Invalid point handling"), so a lone point + // yields no track segment at all — the "insufficient for LineString" + // guard is enforced at the parser boundary rather than left to callers. + expect(coords).toHaveLength(0); }); }); diff --git a/packages/gpx/src/parse.test.ts b/packages/gpx/src/parse.test.ts index 3a1c1b1..b4a2963 100644 --- a/packages/gpx/src/parse.test.ts +++ b/packages/gpx/src/parse.test.ts @@ -68,3 +68,132 @@ describe("parseGpxAsync", () => { await expect(parseGpxAsync("not xml at all <<<<")).rejects.toThrow(); }); }); + +describe("parseGpxAsync — invalid point handling", () => { + const gpx = (body: string) => + `\n${body}`; + + it("skips a point with a missing lat/lon instead of defaulting to Null Island", async () => { + const result = await parseGpxAsync( + gpx(` + 34 + 113 + 519 + `), + ); + expect(result.tracks[0]).toHaveLength(2); + // No 0,0 point leaked in. + expect(result.tracks[0]!.some((p) => p.lat === 0 && p.lon === 0)).toBe(false); + }); + + it("skips a point with garbage coords and keeps distance finite", async () => { + const result = await parseGpxAsync( + gpx(` + + + + `), + ); + expect(result.tracks[0]).toHaveLength(2); + expect(Number.isFinite(result.distance)).toBe(true); + expect(result.distance).toBeGreaterThan(0); + }); + + it("treats unparseable as undefined so gain/loss stay finite", async () => { + const result = await parseGpxAsync( + gpx(` + 34 + NaN + 519 + `), + ); + expect(result.tracks[0]![1]!.ele).toBeUndefined(); + expect(Number.isFinite(result.elevation.gain)).toBe(true); + expect(Number.isFinite(result.elevation.loss)).toBe(true); + }); + + it("tolerates trailing junk on a numeric value (parseFloat lenience)", async () => { + const result = await parseGpxAsync( + gpx(` + 471.0m + 519 + `), + ); + expect(result.tracks[0]![0]!.ele).toBe(471); + }); + + it("drops a segment left with fewer than 2 points", async () => { + const result = await parseGpxAsync( + gpx(` + + + + + + `), + ); + expect(result.tracks).toHaveLength(1); + expect(result.tracks[0]).toHaveLength(2); + }); + + it("leaves a well-formed file's output unchanged", async () => { + const result = await parseGpxAsync(sampleGpx); + expect(result.tracks).toEqual([ + [ + { lat: 52.52, lon: 13.405, ele: 34, time: undefined }, + { lat: 51.05, lon: 13.74, ele: 113, time: undefined }, + { lat: 48.137, lon: 11.576, ele: 519, time: undefined }, + ], + ]); + }); +}); + +describe("parseGpxAsync — route () support", () => { + const gpx = (body: string) => + `\n${body}`; + + it("parses a route-only file into one segment", async () => { + const result = await parseGpxAsync( + gpx(`My Course + 34 + 113 + 519 + `), + ); + expect(result.tracks).toHaveLength(1); + expect(result.tracks[0]).toHaveLength(3); + expect(result.distance).toBeGreaterThan(0); + }); + + it("preserves rtept ele and time", async () => { + const result = await parseGpxAsync( + gpx(` + 34 + 519 + `), + ); + expect(result.tracks[0]![0]).toEqual({ + lat: 52.52, + lon: 13.405, + ele: 34, + time: "2026-01-01T10:00:00Z", + }); + }); + + it("appends route segments after track segments", async () => { + const result = await parseGpxAsync( + gpx(` + + + + + + + `), + ); + expect(result.tracks).toHaveLength(2); + // Track first, route second. + expect(result.tracks[0]![0]!.lat).toBe(52.52); + expect(result.tracks[1]![0]!.lat).toBe(10.0); + }); +}); diff --git a/packages/gpx/src/parse.ts b/packages/gpx/src/parse.ts index 1ea7faa..18f21e6 100644 --- a/packages/gpx/src/parse.ts +++ b/packages/gpx/src/parse.ts @@ -69,28 +69,54 @@ function parseWaypoints(doc: Document): Waypoint[] { }); } -function parseTracks(doc: Document): TrackPoint[][] { - const tracks: TrackPoint[][] = []; - const trksegs = doc.querySelectorAll("trk > trkseg"); +/** + * Parse one `trkpt`/`rtept` element into a TrackPoint, or null if it is + * unusable. Parsing stays `parseFloat`-lenient (accepts leading `+`, + * tolerates trailing junk like `471.0m` that real exporters emit), but a + * point whose `lat`/`lon` is missing or does not parse to a finite number + * is skipped rather than defaulted to `0,0` (which would land on Null + * Island and pass range validation) — spec: gpx-parser-robustness + * "Invalid point handling". A non-finite `` becomes `undefined` (the + * existing "no elevation" representation) so it never poisons gain/loss + * totals with `NaN`. + */ +function parsePoint(pt: Element): TrackPoint | null { + const lat = parseFloat(pt.getAttribute("lat") ?? ""); + const lon = parseFloat(pt.getAttribute("lon") ?? ""); + if (!Number.isFinite(lat) || !Number.isFinite(lon)) return null; + const eleText = pt.querySelector("ele")?.textContent; + const ele = eleText != null ? parseFloat(eleText) : NaN; + const time = pt.querySelector("time")?.textContent ?? undefined; + return { lat, lon, ele: Number.isFinite(ele) ? ele : undefined, time }; +} - for (const seg of trksegs) { - const points: TrackPoint[] = []; - for (const pt of seg.querySelectorAll("trkpt")) { - const lat = parseFloat(pt.getAttribute("lat") ?? "0"); - const lon = parseFloat(pt.getAttribute("lon") ?? "0"); - const eleText = pt.querySelector("ele")?.textContent; - const time = pt.querySelector("time")?.textContent ?? undefined; - points.push({ - lat, - lon, - ele: eleText ? parseFloat(eleText) : undefined, - time, - }); - } - tracks.push(points); +function parseSegmentPoints(pts: ArrayLike): TrackPoint[] { + const points: TrackPoint[] = []; + for (const pt of Array.from(pts)) { + const parsed = parsePoint(pt); + if (parsed) points.push(parsed); + } + return points; +} + +function parseTracks(doc: Document): TrackPoint[][] { + const segments: TrackPoint[][] = []; + + // Standard tracks: . + for (const seg of Array.from(doc.querySelectorAll("trk > trkseg"))) { + segments.push(parseSegmentPoints(seg.querySelectorAll("trkpt"))); + } + // Routes: . Many exporters (Garmin Connect courses, + // gpx.studio, planner exports) emit only routes; each becomes one + // segment appended after the track segments, with rtept handled + // identically to trkpt — spec: gpx-parser-robustness "Route support". + for (const rte of Array.from(doc.querySelectorAll("rte"))) { + segments.push(parseSegmentPoints(rte.querySelectorAll("rtept"))); } - return tracks; + // Drop empty or single-point segments: they render nothing and break + // distance-math assumptions (spec: "Invalid point handling"). + return segments.filter((seg) => seg.length >= 2); } function parseNoGoAreas(doc: Document): NoGoArea[] { From 4536470eb92b72622e2050607a10f0c4450ad220 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ullrich=20Sch=C3=A4fer?= Date: Tue, 14 Jul 2026 00:10:57 +0200 Subject: [PATCH 2/3] feat(gpx): post-parse timestamp repair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task group 3 of gpx-parser-robustness. Timestamps were passed through raw; a partially-broken time channel from a flaky recorder silently shrank moving time and skewed start-time derivation. New pure `timestamp-repair.ts` applied per segment after parsing: - validity = Date.parse yields a finite epoch; - a segment with no valid timestamps is left untouched (untimed track); - >50% invalid → drop all the segment's timestamps (noise, not signal); - otherwise → linearly interpolate invalid runs between valid neighbours (by point index), leading/trailing runs clamp to the nearest valid timestamp; only repaired points are rewritten as ISO 8601, valid points keep their original string. Monotonicity intentionally not enforced (movingTime already skips non-positive intervals; reordering would be fabrication). No GpxData shape change. Wired into parseTracks output in parse.ts. Verified: gpx typecheck + lint clean, 81/81 tests pass, journal + planner typecheck unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../changes/gpx-parser-robustness/tasks.md | 6 +- packages/gpx/src/parse.ts | 3 +- packages/gpx/src/timestamp-repair.test.ts | 56 ++++++++++++++++ packages/gpx/src/timestamp-repair.ts | 67 +++++++++++++++++++ 4 files changed, 128 insertions(+), 4 deletions(-) create mode 100644 packages/gpx/src/timestamp-repair.test.ts create mode 100644 packages/gpx/src/timestamp-repair.ts diff --git a/openspec/changes/gpx-parser-robustness/tasks.md b/openspec/changes/gpx-parser-robustness/tasks.md index 0eaaba4..65b827f 100644 --- a/openspec/changes/gpx-parser-robustness/tasks.md +++ b/openspec/changes/gpx-parser-robustness/tasks.md @@ -11,9 +11,9 @@ ## 3. Timestamp repair -- [ ] 3.1 Create `packages/gpx/src/timestamp-repair.ts`: per-segment pass — >50% invalid → drop all; else linear interpolation between valid neighbors with leading/trailing clamp; output ISO 8601 strings -- [ ] 3.2 Wire the repair pass into `parseTracks` output in `parse.ts` -- [ ] 3.3 Unit tests (`timestamp-repair.test.ts`): sparse invalid interpolated, >50% invalid dropped (and `movingTime` returns null), leading/trailing clamp, all-valid untouched, no-timestamps untouched +- [x] 3.1 Create `packages/gpx/src/timestamp-repair.ts`: per-segment pass — >50% invalid → drop all; else linear interpolation between valid neighbors with leading/trailing clamp; output ISO 8601 strings +- [x] 3.2 Wire the repair pass into `parseTracks` output in `parse.ts` +- [x] 3.3 Unit tests (`timestamp-repair.test.ts`): sparse invalid interpolated, >50% invalid dropped (and `movingTime` returns null), leading/trailing clamp, all-valid untouched, no-timestamps untouched ## 4. Metadata fallback diff --git a/packages/gpx/src/parse.ts b/packages/gpx/src/parse.ts index 18f21e6..fecb2e2 100644 --- a/packages/gpx/src/parse.ts +++ b/packages/gpx/src/parse.ts @@ -1,5 +1,6 @@ import type { Waypoint } from "@trails-cool/types"; import type { GpxData, TrackPoint, ElevationProfile, NoGoArea } from "./types.ts"; +import { repairTimestamps } from "./timestamp-repair.ts"; /** * Parse a GPX XML string into structured data. @@ -31,7 +32,7 @@ function parseGpxWithParser(parser: DOMParser, xml: string): GpxData { const name = doc.querySelector("metadata > name")?.textContent ?? undefined; const description = doc.querySelector("metadata > desc")?.textContent ?? undefined; const waypoints = parseWaypoints(doc); - const tracks = parseTracks(doc); + const tracks = repairTimestamps(parseTracks(doc)); const noGoAreas = parseNoGoAreas(doc); const { totalDistance, ...elevation } = computeElevation(tracks); diff --git a/packages/gpx/src/timestamp-repair.test.ts b/packages/gpx/src/timestamp-repair.test.ts new file mode 100644 index 0000000..df31c5e --- /dev/null +++ b/packages/gpx/src/timestamp-repair.test.ts @@ -0,0 +1,56 @@ +import { describe, it, expect } from "vitest"; +import { repairSegmentTimestamps } from "./timestamp-repair.ts"; +import { movingTime } from "./moving-time.ts"; +import type { TrackPoint } from "./types.ts"; + +const pt = (time?: string, lat = 52.5, lon = 13.4): TrackPoint => ({ lat, lon, time }); + +describe("repairSegmentTimestamps", () => { + it("interpolates a sparse invalid timestamp between valid neighbours", () => { + const out = repairSegmentTimestamps([ + pt("2026-01-01T10:00:00Z"), + pt("garbage"), + pt("2026-01-01T10:02:00Z"), + pt("2026-01-01T10:03:00Z"), + ]); + // index 1 is midway (by index) between valid index 0 and index 2. + expect(out[1]!.time).toBe("2026-01-01T10:01:00.000Z"); + // Valid points keep their original strings untouched. + expect(out[0]!.time).toBe("2026-01-01T10:00:00Z"); + expect(out[3]!.time).toBe("2026-01-01T10:03:00Z"); + }); + + it("drops all timestamps when more than half the segment is invalid", () => { + const out = repairSegmentTimestamps([ + pt("2026-01-01T10:00:00Z"), + pt("garbage"), + pt(undefined), + pt("nope"), + ]); + expect(out.every((p) => p.time === undefined)).toBe(true); + // A dropped time channel means the segment reads as untimed. + expect(movingTime([out])).toBeNull(); + }); + + it("clamps leading and trailing invalid runs to the nearest valid timestamp", () => { + const out = repairSegmentTimestamps([ + pt("garbage"), + pt("2026-01-01T10:01:00Z"), + pt("2026-01-01T10:02:00Z"), + pt(undefined), + ]); + expect(out[0]!.time).toBe("2026-01-01T10:01:00.000Z"); // leading clamp + expect(out[3]!.time).toBe("2026-01-01T10:02:00.000Z"); // trailing clamp + }); + + it("leaves an all-valid segment untouched", () => { + const input = [pt("2026-01-01T10:00:00Z"), pt("2026-01-01T10:01:00Z")]; + const out = repairSegmentTimestamps(input); + expect(out.map((p) => p.time)).toEqual(["2026-01-01T10:00:00Z", "2026-01-01T10:01:00Z"]); + }); + + it("leaves an untimed segment (no timestamps) untouched", () => { + const out = repairSegmentTimestamps([pt(undefined), pt(undefined), pt(undefined)]); + expect(out.every((p) => p.time === undefined)).toBe(true); + }); +}); diff --git a/packages/gpx/src/timestamp-repair.ts b/packages/gpx/src/timestamp-repair.ts new file mode 100644 index 0000000..4386601 --- /dev/null +++ b/packages/gpx/src/timestamp-repair.ts @@ -0,0 +1,67 @@ +import type { TrackPoint } from "./types.ts"; + +// Post-parse timestamp repair (spec: gpx-parser-robustness "Timestamp +// repair"). Recorders from flaky devices emit runs of missing or garbage +// `