diff --git a/openspec/changes/gpx-parser-robustness/tasks.md b/openspec/changes/gpx-parser-robustness/tasks.md index e053b89..65b827f 100644 --- a/openspec/changes/gpx-parser-robustness/tasks.md +++ b/openspec/changes/gpx-parser-robustness/tasks.md @@ -1,19 +1,19 @@ ## 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 -- [ ] 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/fit/src/gpx-to-fit-course.test.ts b/packages/fit/src/gpx-to-fit-course.test.ts index 6f938ea..d887bff 100644 --- a/packages/fit/src/gpx-to-fit-course.test.ts +++ b/packages/fit/src/gpx-to-fit-course.test.ts @@ -33,7 +33,11 @@ function decode(bytes: Uint8Array): Promise { }); } -const FIXTURES = ["short-flat.gpx", "alpine.gpx", "multi-day.gpx", "single-point.gpx"] as const; +// single-point.gpx is intentionally excluded from the round-trip loop: the +// GPX parser now drops segments left with fewer than 2 points +// (gpx-parser-robustness), so a lone point yields zero track points and +// can't round-trip. That degenerate case is covered on its own below. +const FIXTURES = ["short-flat.gpx", "alpine.gpx", "multi-day.gpx"] as const; describe("gpxToFitCourse", () => { for (const fixture of FIXTURES) { @@ -67,6 +71,14 @@ describe("gpxToFitCourse", () => { await expect(gpxToFitCourse({ gpx, name: "Empty" })).rejects.toThrow(/zero track points/); }); + it("drops a single-point GPX to zero points, so it is refused", async () => { + // The parser discards <2-point segments (gpx-parser-robustness), so a + // lone point can't form a FIT course — the zero-points guard fires. + const gpx = await loadFixture("single-point.gpx"); + expect((await parseGpxAsync(gpx)).tracks.flat()).toHaveLength(0); + await expect(gpxToFitCourse({ gpx, name: "Single" })).rejects.toThrow(/zero track points/); + }); + it("advertises position+distance course capabilities (not time-only)", async () => { const gpx = await loadFixture("short-flat.gpx"); const bytes = await gpxToFitCourse({ gpx, name: "Caps", sport: "cycling" }); 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..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); @@ -69,28 +70,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[] { 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 +// `