From e10c8cedca9449a8644aa1ac31b8736177d12d66 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 4 Jul 2026 17:54:54 +0200 Subject: [PATCH 1/4] fix: reconcile routeselected handlers and persist selected alternative Merge two separate lrmControl.on('routeselected') listeners into a single handler in index.js. The state.js listener only set this.options.alternative but never called this.update(), so the selected alternative was never persisted to the URL. Changes: - Remove routeselected listener from state.js - Add alternative tracking + state.update() to the remaining handler - Switch to URL-specified alternative on initial load (LRM always selects route[0] first), with fallback if the alternative no longer exists - Extract alternative-resolution logic into pure resolveInitialAlternative() in src/route_alternative.js - Add 18 unit tests covering no-switch, switch, fallback, and edge cases Closes #455 Co-Authored-By: Claude --- src/index.js | 18 ++++ src/route_alternative.js | 30 +++++++ src/state.js | 4 - test/route_alternative.test.js | 158 +++++++++++++++++++++++++++++++++ 4 files changed, 206 insertions(+), 4 deletions(-) create mode 100644 src/route_alternative.js create mode 100644 test/route_alternative.test.js diff --git a/src/index.js b/src/index.js index 04fbcd9c..a9e9fa6c 100644 --- a/src/index.js +++ b/src/index.js @@ -43,6 +43,7 @@ var localization = require('./localization'); var initialLayers = require('./initial_layers'); var layerUtils = require('./layer_utils'); var routeZoom = require('./route_zoom'); +var resolveInitialAlternative = require('./route_alternative'); require('./polyfill'); var parsedOptions = urlState.parse(window.location.search.slice(1)); @@ -721,6 +722,23 @@ lrmControl.on('alternateChosen', function(e) { lrmControl.on('routeselected', function(e) { var route = e.route || {}; + // On the initial route load, the LRM always selects route[0] first. + // If the URL requested a different alternative, switch to it now. + // Falls back to route[0] when the requested alternative no longer exists. + if (shouldFitRoute) { + var switchTo = resolveInitialAlternative(route, e.alternatives, state.options.alternative); + if (switchTo) { + // Re-fire on lrmControl so the LRM-internal listeners + // (_routeSelected, _selectAlt) also process the correct route. + lrmControl.fire('routeselected', switchTo); + return; + } + } + + // Track selected alternative in URL state and persist to URL + state.options.alternative = route.routesIndex; + state.update(); + // Route export: build GeoJSON and hand it to the tools control for GPX download var routeGeoJSON = { type: 'Feature', diff --git a/src/route_alternative.js b/src/route_alternative.js new file mode 100644 index 00000000..4df10649 --- /dev/null +++ b/src/route_alternative.js @@ -0,0 +1,30 @@ +'use strict'; + +/** + * Decides whether to switch from the current route to a different alternative + * based on a desired index (e.g. read from URL state on initial page load). + * + * The LRM always selects route[0] first. If the caller wants a different + * alternative, this function returns the re-arranged event payload so the + * caller can re-fire `routeselected` with the correct route. + * + * @param {Object} route – the currently selected route (must have .routesIndex) + * @param {Array} alternatives – other route objects (may be empty / undefined) + * @param {*} desiredAlt – the 0-based index of the desired alternative + * @returns {Object|null} – {route, alternatives} to re-fire, or null when + * no switch is needed (desiredAlt is 0, missing, + * already matches, or out of bounds) + */ +module.exports = function resolveInitialAlternative(route, alternatives, desiredAlt) { + var index = Number(desiredAlt); + if (!index || index < 0) return null; // 0, NaN, undefined, null, "", negative + if (route.routesIndex === index) return null; // already on the desired route + + var allRoutes = [route].concat(alternatives || []); + if (index >= allRoutes.length) return null; // desiredAlt no longer exists + + return { + route: allRoutes[index], + alternatives: allRoutes.filter(function(_, i) { return i !== index; }) + }; +}; diff --git a/src/state.js b/src/state.js index 97a8660e..58186fe7 100644 --- a/src/state.js +++ b/src/state.js @@ -18,10 +18,6 @@ var State = L.Class.extend({ this.set(default_options); - this._lrm.on('routeselected', function(e) { - this.options.alternative = e.route.routesIndex; - }, this); - this._lrm.getPlan().on('waypointschanged', function() { this.options.waypoints = this._lrm.getWaypoints(); this.update({ push: true }); }.bind(this)); diff --git a/test/route_alternative.test.js b/test/route_alternative.test.js new file mode 100644 index 00000000..cf41635a --- /dev/null +++ b/test/route_alternative.test.js @@ -0,0 +1,158 @@ +'use strict'; + +var resolveInitialAlternative = require('../src/route_alternative'); + +function makeRoute(routesIndex) { + return { name: 'Route ' + routesIndex, coordinates: [], routesIndex: routesIndex }; +} + +describe('resolveInitialAlternative', function() { + + describe('returns null (no switch) when', function() { + + test('desiredAlt is 0', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], 0); + expect(result).toBeNull(); + }); + + test('desiredAlt is undefined', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], undefined); + expect(result).toBeNull(); + }); + + test('desiredAlt is null', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], null); + expect(result).toBeNull(); + }); + + test('desiredAlt is NaN (unparseable string)', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], 'abc'); + expect(result).toBeNull(); + }); + + test('desiredAlt is an empty string', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], ''); + expect(result).toBeNull(); + }); + + test('route.routesIndex already matches desiredAlt', function() { + var route = makeRoute(1); + var result = resolveInitialAlternative(route, [makeRoute(0), makeRoute(2)], 1); + expect(result).toBeNull(); + }); + + test('desiredAlt exceeds available routes (out of bounds)', function() { + var route = makeRoute(0); + // Only 3 routes total (indices 0,1,2) — desiredAlt 5 doesn't exist + var result = resolveInitialAlternative(route, [makeRoute(1), makeRoute(2)], 5); + expect(result).toBeNull(); + }); + + test('desiredAlt equals length (off by one)', function() { + var route = makeRoute(0); + // 3 routes, index 3 is past the end + var result = resolveInitialAlternative(route, [makeRoute(1), makeRoute(2)], 3); + expect(result).toBeNull(); + }); + + test('there are no alternatives and desiredAlt > 0', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [], 1); + expect(result).toBeNull(); + }); + + test('alternatives is undefined', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, undefined, 1); + expect(result).toBeNull(); + }); + + test('alternatives is null', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, null, 1); + expect(result).toBeNull(); + }); + + test('desiredAlt is negative', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], -1); + expect(result).toBeNull(); + }); + }); + + describe('returns a switch payload when', function() { + + test('desiredAlt matches a middle alternative', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var route2 = makeRoute(2); + var result = resolveInitialAlternative(route0, [route1, route2], 1); + + expect(result).not.toBeNull(); + expect(result.route).toBe(route1); + expect(result.alternatives).toHaveLength(2); + expect(result.alternatives).toContain(route0); + expect(result.alternatives).toContain(route2); + }); + + test('desiredAlt is the last alternative', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var route2 = makeRoute(2); + var result = resolveInitialAlternative(route0, [route1, route2], 2); + + expect(result).not.toBeNull(); + expect(result.route).toBe(route2); + expect(result.alternatives).toEqual([route0, route1]); + }); + + test('desiredAlt is a string ("1") that coerces to number', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var route2 = makeRoute(2); + // This mimics the URL parser returning strings + var result = resolveInitialAlternative(route0, [route1, route2], '1'); + + expect(result).not.toBeNull(); + expect(result.route).toBe(route1); + expect(result.alternatives).toContain(route0); + expect(result.alternatives).toContain(route2); + }); + + test('there is exactly one alternative and it is the target', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var result = resolveInitialAlternative(route0, [route1], 1); + + expect(result).not.toBeNull(); + expect(result.route).toBe(route1); + expect(result.alternatives).toEqual([route0]); + }); + }); + + describe('returns referentially correct objects', function() { + + test('the returned route is the exact same object from the input', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var result = resolveInitialAlternative(route0, [route1], 1); + + expect(result.route).toBe(route1); + }); + + test('returned alternatives are the exact same objects', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var route2 = makeRoute(2); + var result = resolveInitialAlternative(route0, [route1, route2], 1); + + expect(result.alternatives[0]).toBe(route0); + expect(result.alternatives[1]).toBe(route2); + }); + }); +}); From 776aee461ae63997467ecda5e1b58d2638553ceb Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 4 Jul 2026 18:00:22 +0200 Subject: [PATCH 2/4] style: fix brace-style lint errors in route_alternative.js --- src/route_alternative.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/route_alternative.js b/src/route_alternative.js index 4df10649..40ee7966 100644 --- a/src/route_alternative.js +++ b/src/route_alternative.js @@ -25,6 +25,8 @@ module.exports = function resolveInitialAlternative(route, alternatives, desired return { route: allRoutes[index], - alternatives: allRoutes.filter(function(_, i) { return i !== index; }) + alternatives: allRoutes.filter(function(_, i) { + return i !== index; + }) }; }; From 4e7d7858119ed4a99871382f69e866d59ad6ffcd Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 4 Jul 2026 18:06:23 +0200 Subject: [PATCH 3/4] fix(routing): guard against non-integer desiredAlt in route alternative resolution Non-integer values like 1.5 (which could come from URL query strings) would produce allRoutes[1.5] = undefined, causing an undefined route payload and potential re-entrant routeselected firing. Add index % 1 !== 0 guard and regression tests for float and float-string inputs. --- src/route_alternative.js | 2 +- test/route_alternative.test.js | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/route_alternative.js b/src/route_alternative.js index 40ee7966..2d766c89 100644 --- a/src/route_alternative.js +++ b/src/route_alternative.js @@ -17,7 +17,7 @@ */ module.exports = function resolveInitialAlternative(route, alternatives, desiredAlt) { var index = Number(desiredAlt); - if (!index || index < 0) return null; // 0, NaN, undefined, null, "", negative + if (!index || index < 0 || index % 1 !== 0) return null; // 0, NaN, undefined, null, "", negative, non-integer if (route.routesIndex === index) return null; // already on the desired route var allRoutes = [route].concat(alternatives || []); diff --git a/test/route_alternative.test.js b/test/route_alternative.test.js index cf41635a..b19ad47f 100644 --- a/test/route_alternative.test.js +++ b/test/route_alternative.test.js @@ -83,6 +83,18 @@ describe('resolveInitialAlternative', function() { var result = resolveInitialAlternative(route, [makeRoute(1)], -1); expect(result).toBeNull(); }); + + test('desiredAlt is a non-integer float (e.g. "1.5")', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1), makeRoute(2)], 1.5); + expect(result).toBeNull(); + }); + + test('desiredAlt is a float string ("1.5")', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1), makeRoute(2)], '1.5'); + expect(result).toBeNull(); + }); }); describe('returns a switch payload when', function() { From dee57a6ddcd20ff56908c61626b4f7ec774be725 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 4 Jul 2026 18:08:52 +0200 Subject: [PATCH 4/4] docs: add naming convention to AGENTS.md Prohibit cute/whimsical names; require descriptive, functional names for variables and functions. --- AGENTS.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index fd558628..cb710e3c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,6 +20,10 @@ OSRM Frontend is the browser-based routing interface served at https://map.proje - All new functionality must have unit test coverage. +### Naming + +- **Use descriptive, functional names for variables and functions.** Avoid cute, playful, or whimsical names (e.g. no `makeRoute`, `switchTo`, `handleIt`). Names should describe what the thing is or does, not how the author feels about it. + ### Notes - Commit generated artifacts (`bundle.js`, `bundle.js.map`, `dist/`) only when necessary.