Merge pull request #576 from trails-cool/feat/gpx-parser-robustness-lenience

feat(gpx): lenient point parsing + route (<rte>) support

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 `<ele>` 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 `<ele>` 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 (`<rte>`) support
 
-- [ ] 2.1 Parse `<rte>`/`rtept` as track segments (appended after `<trk>` 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 `<rte>`/`rtept` as track segments (appended after `<trk>` 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
 

packages/fit/src/gpx-to-fit-course.test.ts

@@ -33,7 +33,11 @@ function decode(bytes: Uint8Array): Promise<ParsedFit> {
   });
 }
 
-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" });

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);
   });
 });

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) =>
+    `<?xml version="1.0" encoding="UTF-8"?>\n<gpx version="1.1" xmlns="http://www.topografix.com/GPX/1/1">${body}</gpx>`;
+
+  it("skips a point with a missing lat/lon instead of defaulting to Null Island", async () => {
+    const result = await parseGpxAsync(
+      gpx(`<trk><trkseg>
+        <trkpt lat="52.52" lon="13.405"><ele>34</ele></trkpt>
+        <trkpt lon="13.74"><ele>113</ele></trkpt>
+        <trkpt lat="48.137" lon="11.576"><ele>519</ele></trkpt>
+      </trkseg></trk>`),
+    );
+    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(`<trk><trkseg>
+        <trkpt lat="52.52" lon="13.405"></trkpt>
+        <trkpt lat="abc" lon="13.74"></trkpt>
+        <trkpt lat="48.137" lon="11.576"></trkpt>
+      </trkseg></trk>`),
+    );
+    expect(result.tracks[0]).toHaveLength(2);
+    expect(Number.isFinite(result.distance)).toBe(true);
+    expect(result.distance).toBeGreaterThan(0);
+  });
+
+  it("treats unparseable <ele> as undefined so gain/loss stay finite", async () => {
+    const result = await parseGpxAsync(
+      gpx(`<trk><trkseg>
+        <trkpt lat="52.52" lon="13.405"><ele>34</ele></trkpt>
+        <trkpt lat="51.05" lon="13.74"><ele>NaN</ele></trkpt>
+        <trkpt lat="48.137" lon="11.576"><ele>519</ele></trkpt>
+      </trkseg></trk>`),
+    );
+    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(`<trk><trkseg>
+        <trkpt lat="52.52" lon="13.405"><ele>471.0m</ele></trkpt>
+        <trkpt lat="48.137" lon="11.576"><ele>519</ele></trkpt>
+      </trkseg></trk>`),
+    );
+    expect(result.tracks[0]![0]!.ele).toBe(471);
+  });
+
+  it("drops a segment left with fewer than 2 points", async () => {
+    const result = await parseGpxAsync(
+      gpx(`<trk>
+        <trkseg><trkpt lat="52.52" lon="13.405"></trkpt></trkseg>
+        <trkseg>
+          <trkpt lat="52.52" lon="13.405"></trkpt>
+          <trkpt lat="48.137" lon="11.576"></trkpt>
+        </trkseg>
+      </trk>`),
+    );
+    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 (<rte>) support", () => {
+  const gpx = (body: string) =>
+    `<?xml version="1.0" encoding="UTF-8"?>\n<gpx version="1.1" xmlns="http://www.topografix.com/GPX/1/1">${body}</gpx>`;
+
+  it("parses a route-only file into one segment", async () => {
+    const result = await parseGpxAsync(
+      gpx(`<rte><name>My Course</name>
+        <rtept lat="52.52" lon="13.405"><ele>34</ele></rtept>
+        <rtept lat="51.05" lon="13.74"><ele>113</ele></rtept>
+        <rtept lat="48.137" lon="11.576"><ele>519</ele></rtept>
+      </rte>`),
+    );
+    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(`<rte>
+        <rtept lat="52.52" lon="13.405"><ele>34</ele><time>2026-01-01T10:00:00Z</time></rtept>
+        <rtept lat="48.137" lon="11.576"><ele>519</ele><time>2026-01-01T11:00:00Z</time></rtept>
+      </rte>`),
+    );
+    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(`<trk><trkseg>
+          <trkpt lat="52.52" lon="13.405"></trkpt>
+          <trkpt lat="51.05" lon="13.74"></trkpt>
+        </trkseg></trk>
+        <rte>
+          <rtept lat="10.0" lon="10.0"></rtept>
+          <rtept lat="11.0" lon="11.0"></rtept>
+        </rte>`),
+    );
+    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);
+  });
+});

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 `<ele>` 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<Element>): 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: <trk><trkseg><trkpt>.
+  for (const seg of Array.from(doc.querySelectorAll("trk > trkseg"))) {
+    segments.push(parseSegmentPoints(seg.querySelectorAll("trkpt")));
+  }
+  // Routes: <rte><rtept>. 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[] {

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);
+  });
+});

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
+// `<time>` values; passed through raw they silently shrink moving time and
+// skew start-time derivation. This is a pure per-segment pass with an
+// explicit policy, so `movingTime`/`computeDays`/`startTime` stay correct
+// without each re-implementing (and drifting on) the same rules.
+//
+// Policy:
+//   - Validity = `Date.parse` yields a finite epoch.
+//   - A segment with NO valid timestamps is left untouched (an untimed
+//     track — downstream already handles that).
+//   - > 50% of a segment's points invalid → drop ALL of that segment's
+//     timestamps (a mostly-broken time channel is noise, not signal).
+//   - Otherwise → linearly interpolate invalid runs between their valid
+//     neighbours; a leading/trailing invalid run clamps to the nearest
+//     valid timestamp. Only repaired points are rewritten (as ISO 8601);
+//     valid points keep their original string untouched.
+//
+// Monotonicity is deliberately not enforced — `movingTime` already skips
+// non-positive intervals, and reordering recorded data would be fabrication.
+
+function epochOf(time: string | undefined): number {
+  return time == null ? NaN : Date.parse(time);
+}
+
+export function repairSegmentTimestamps(points: TrackPoint[]): TrackPoint[] {
+  const epochs = points.map((p) => epochOf(p.time));
+  const validIdx = epochs.map((e, i) => (Number.isFinite(e) ? i : -1)).filter((i) => i >= 0);
+
+  // Nothing to anchor to, or the time channel is mostly broken → treat the
+  // segment as untimed. (When there are zero valid timestamps this leaves
+  // the already-absent times as-is.)
+  if (validIdx.length === 0) return points;
+  const invalidCount = points.length - validIdx.length;
+  if (invalidCount === 0) return points; // all valid → untouched
+  if (invalidCount / points.length > 0.5) {
+    return points.map((p) => (p.time === undefined ? p : { ...p, time: undefined }));
+  }
+
+  const firstValid = validIdx[0]!;
+  const lastValid = validIdx[validIdx.length - 1]!;
+
+  return points.map((p, i) => {
+    if (Number.isFinite(epochs[i]!)) return p; // valid → keep original string
+    let t: number;
+    if (i < firstValid) {
+      t = epochs[firstValid]!; // leading run clamps forward
+    } else if (i > lastValid) {
+      t = epochs[lastValid]!; // trailing run clamps back
+    } else {
+      // Between two valid anchors: linear interpolation by point index.
+      let prev = i;
+      while (!Number.isFinite(epochs[prev]!)) prev--;
+      let next = i;
+      while (!Number.isFinite(epochs[next]!)) next++;
+      const frac = (i - prev) / (next - prev);
+      t = epochs[prev]! + (epochs[next]! - epochs[prev]!) * frac;
+    }
+    return { ...p, time: new Date(t).toISOString() };
+  });
+}
+
+export function repairTimestamps(segments: TrackPoint[][]): TrackPoint[][] {
+  return segments.map(repairSegmentTimestamps);
+}