Unmatched OSM - #164
Unmatched OSM#164
Conversation
…nused A new view re-projects each stop's marker onto the OSM feature the matcher anchored it to, instead of where its feed puts it -- the categories stay as they are, and a stop the anchoring refused keeps its feed position. The switch is shared between the report and the selection panel via OsmMatchesOptionsContext, and gated where a region has no anchors. Beside it, an 'unmatched OSM stops' dataset draws the stops and stations the matcher was offered but did not use (osm-index-stops.tsv.gz, read once per region on demand), and a click on one opens its tags. The old standalone preview panel is removed in favour of this re-projection. Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
8.0.1 pulled in an atypical dependency set; ^7.29.7 restores the standard Babel 7.x graph that the build expects. Signed-off-by: dkiselev <dmitry.v.kiselev@gmail.com>
biodranik
left a comment
There was a problem hiding this comment.
Re-review of eec901b (merged — these are follow-ups). Reviewed as architect, frontend, performance, OSM and GTFS: the two-collection setData design, the revision counter and the on-demand gzipped index are all sound; findings are inline.
The one that matters: clicking an unmatched OSM circle opens the GTFS stop panel, which offers Add OSM Stop and mints a node tagged ref:gtfs=undefined. The rest are comment/doc drift, dead fields in OsmIndexRow, a cached rejected promise, and layer order lost on a base-style switch.
Alternative worth weighing — the unmatched-OSM dataset is a 3.3 MB gzipped TSV parsed into a 67k-feature GeoJSON source held whole in memory (germany-local). The pipeline already publishes pt-structure.pmtiles; putting this layer in a PMTiles archive would make it viewport- and zoom-lazy, with no DecompressionStream, no full-region parse and no per-region memory cost. Cons: pipeline work, a second format for one layer, and the current path is already lazy behind a checkbox. Probably not worth it for one country — worth it if this grows to the whole planet or gains per-feature styling.
Two things I checked and found fine: manualChunks: { pmtiles: [...] } does only name the chunk that await import('pmtiles') in map.ts already splits out (the comment is accurate), and MatchReport is keyed by region in report-selector.tsx, so unassignedRows cannot survive a region switch.
Generated by Claude Code
| setActionError(null); | ||
| updateSelection({ | ||
| feature: stringifyProperties({ | ||
| type: 'Feature', | ||
| geometry: { type: 'Point', coordinates: [lon, lat] }, | ||
| properties: { | ||
| name, lon, lat, flavour, seenBy, nearestM, | ||
| osmFeatures: [{ id: osmId, lon, lat, tags: {} }], | ||
| }, | ||
| }), | ||
| // No dataset: this is not one of the report's stop categories, and naming one | ||
| // would have the panel describe a match that does not exist. | ||
| reportRegion, idTags, | ||
| }, 'map-click'); |
There was a problem hiding this comment.
An OSM-only selection goes through MatchInfo, which assumes a GTFS stop behind it:
getGtfsFeatures(selection-info.tsx:359) falls back to[{id: properties.gtfsStopId, …}], sogtfsFeatures.length === 1and the panel printsGTFS stop ID: **(empty) andGTFS stop code: N/A.AddOsmStopController id={properties.gtfsStopId}is rendered withid/codeboth undefined. Pressing Add OSM Stop and clicking the map creates a node with{[gtfsIdTag]: code || id}→ref:gtfs=undefined(add-stop-controller.tsx:36-40), which then goes into the exported.osm.
Worth gating the GTFS blocks and the editor action in MatchInfo on an actual stop, e.g. const hasGtfs = !!(properties.gtfsStopId || properties.gtfsFeatures).
Related: flavour, seenBy and nearestM are put into the properties here but nothing renders them — they are the only facts that make the circle worth clicking ("offered to 3 stops, nearest 12 m"). Either show them in the panel or drop them from the payload.
Generated by Claude Code
| // What the matcher looked at and did not use, drawn beside the stops it placed. Fetched | ||
| // only while osm matches is on: it is the largest file the report publishes, and a | ||
| // session that never looks at the osm-matches view should never pay for it. |
There was a problem hiding this comment.
Outdated comment: the fetch is driven by showUnmatchedOsm — its own checkbox — not by the osm-matches switch. The dataset is deliberately independent of it (docs/component-tree.md says so), so this reads as a constraint that isn't there.
Same claim in two more places: the UnassignedOsmLayer doc ("while osm matches is on", report.tsx:693) and osmIndex.ts:4.
Generated by Claude Code
| .catch(e => { | ||
| console.error('Could not read osm-index.tsv.gz for', reportRegion, e); | ||
| setActionError(`Could not read osm-index.tsv.gz: ${e.message}`); | ||
| }); |
There was a problem hiding this comment.
Two small things:
- The
.thenabove checkscancelled, this doesn't — untick the checkbox while the fetch is in flight and a failure still raises a banner for a layer that is no longer on. - The file fetched is
osm-index-stops.tsv.gz, notosm-index.tsv.gz; the error names the wrong one (also osmIndex.ts:80).
| .catch(e => { | |
| console.error('Could not read osm-index.tsv.gz for', reportRegion, e); | |
| setActionError(`Could not read osm-index.tsv.gz: ${e.message}`); | |
| }); | |
| .catch(e => { | |
| console.error('Could not read osm-index-stops.tsv.gz for', reportRegion, e); | |
| if (!cancelled) setActionError(`Could not read osm-index-stops.tsv.gz: ${e.message}`); | |
| }); |
Generated by Claude Code
| matched: 0, | ||
| anchored: parseInt(c[at['gtfs_anchored']], 10) || 0, |
There was a problem hiding this comment.
matched is hard-coded to 0 although gtfs_matched is listed in REQUIRED — the column is demanded and never read, and the field's doc ("How many GTFS stops matched it, by any tier") is not what the code produces. anchored is parsed but no consumer reads it either (report.tsx only takes osmId, lon, lat, flavour, name, seenBy, nearestM).
This file is by definition the subset nothing matched, so the simplest fix is to drop both fields from OsmIndexRow and gtfs_matched/gtfs_anchored from REQUIRED. Also a stray double blank line at :87-88.
Generated by Claude Code
| * and says so with an empty count. | ||
| */ | ||
| export async function loadUnmatchedOsmStops(region: string): Promise<OsmIndexRow[] | null> { | ||
| return cache[region] ??= fetchRegion(region); |
There was a problem hiding this comment.
A failed fetch is cached as a rejected promise for the rest of the session, so unticking and re-ticking the checkbox never retries — the one recovery a user has. routeVariants.ts evicts on failure for exactly this reason; worth matching it:
| return cache[region] ??= fetchRegion(region); | |
| return cache[region] ??= fetchRegion(region).catch(e => { | |
| delete cache[region]; | |
| throw e; | |
| }); |
Generated by Claude Code
| function useOsmFeatures() { | ||
| return useSyncExternalStore( | ||
| const revision = useSyncExternalStore( | ||
| (sub) => OSM_DATA.subscribe(sub), | ||
| () => OSM_DATA.elements | ||
| () => OSM_DATA.revision | ||
| ); | ||
| return { elements: OSM_DATA.elements, revision }; |
There was a problem hiding this comment.
Simplification: the hook leaks revision to every caller, who must then remember to put it in each dep list — and the one caller already gets it half wrong, listing both allOsmFeatures (identity never changes, so it contributes nothing) and osmRevision at :473. Returning a snapshot whose identity does change makes the deps correct by construction:
| function useOsmFeatures() { | |
| return useSyncExternalStore( | |
| const revision = useSyncExternalStore( | |
| (sub) => OSM_DATA.subscribe(sub), | |
| () => OSM_DATA.elements | |
| () => OSM_DATA.revision | |
| ); | |
| return { elements: OSM_DATA.elements, revision }; | |
| function useOsmFeatures() { | |
| const revision = useSyncExternalStore( | |
| (sub) => OSM_DATA.subscribe(sub), | |
| () => OSM_DATA.revision | |
| ); | |
| return useMemo(() => OSM_DATA.elements.slice(), [revision]); | |
| } |
Callers go back to const allOsmFeatures = useOsmFeatures() with plain [allOsmFeatures, …] deps (:421 and :473). A shallow copy of pointers per Overpass response is nothing beside the round-trip that produced it, and no future memo can forget the revision.
Generated by Claude Code
| // A region with no anchors, or the report list, leaves nothing to show on osm matches -- and | ||
| // the switch outlives both, so it is cleared here rather than by whichever component noticed. | ||
| useEffect(() => { | ||
| if (!osmMatchesAvailable && osmMatchesOn) setOsmMatchesOn(false); | ||
| }, [osmMatchesAvailable, osmMatchesOn]); |
There was a problem hiding this comment.
Three mechanisms guard the same thing: this effect, showingOsmMatches = osmMatchesOn && anchoredTotal > 0 (report.tsx:238), and OsmMatchesSwitch returning null when unavailable. The last two already make the flag unreadable and the control unreachable, so this one only costs an extra render pass on every region switch — and dropping it is arguably nicer behaviour: moving between two regions that both have anchors would keep the switch where you left it instead of resetting it.
Generated by Claude Code
| import { useContext } from "preact/hooks"; | ||
|
|
||
| import "./switch.css"; | ||
| import { OsmMatchesOptionsContext } from "../app"; |
There was a problem hiding this comment.
This makes a cycle: app.tsx → selection-info.tsx → switch.tsx → app.tsx. It works only because the context is read inside the component body rather than at module scope. Keeping switch.tsx to the generic Switch and putting OsmMatchesSwitch next to the context (in app.tsx, or its own file) breaks it and leaves a reusable control behind.
Generated by Claude Code
| title={'OSM stops and stations the matcher was offered for some GTFS stop' | ||
| + ' and nothing matched, and that nothing else in the pipeline claims.' | ||
| + ' Shown beside any of the categories above; it moves nothing.'}> | ||
| Unmatched OSM stops |
There was a problem hiding this comment.
Proofreading: "the matcher was offered for some GTFS stop and nothing matched" reads as a garden path — offered wants as a candidate for, and the two clauses switch subject mid-sentence. "that nothing else in the pipeline claims" is also something the reader cannot check from the map.
| title={'OSM stops and stations the matcher was offered for some GTFS stop' | |
| + ' and nothing matched, and that nothing else in the pipeline claims.' | |
| + ' Shown beside any of the categories above; it moves nothing.'}> | |
| Unmatched OSM stops | |
| <span className={'match-group-title'} | |
| title={'OSM stops and stations the matcher considered as candidates for' | |
| + ' some GTFS stop and matched to none. Drawn beside any of the' | |
| + ' categories above; it moves nothing.'}> |
Generated by Claude Code
| - `LayerControls` (`src/map/layers-controls.ts`) swaps base styles (`cycleBaseStyle`) while re-applying registered overlays; everything map-drawn goes through `addOverlayImmediate`/`removeOverlayImmediate` so it survives style switches. | ||
| - `StopsLayer` (`report.tsx`) is **one** source/symbol layer for all of a region's stops; category toggles use `map.setFilter` on `subcategory` (never rebuild the source), icons are data-driven `stop-{code}` images recolored from `/stop-var.svg` by `loadSvgWithColors`. It also mutates the stored overlay spec's `filter` in place so base-style switches re-apply the current filter. | ||
| - Render-less overlay components follow one idiom: build the overlay spec, `mapLoaded.then(...)` guarded by a `subscription = { canceled, promiseFulfiled }` object, clean up in the effect teardown. See `StopsLayer`, `DatasetMapLayer` (preview), `RegionMarkersLayer`, `MatchArrowLayer`, `RoutesMap` (which instead keeps a persistent `routes` source and `setData`s into it), `HtmlMapMarker` (`editor/map-marker.tsx`). | ||
| - `StopsLayer` (`report.tsx`) is **one** source/symbol layer for all of a region's stops; category toggles use `map.setFilter` on `subcategory`, and the osm-matches view swaps the geometry with `setData` from one of two collections built per region and kept — the source is created once per region, never per toggle. Icons are data-driven `stop-{code}` images recolored from `/stop-var.svg` by `loadSvgWithColors`, with `icon-opacity` fading the stops the anchoring refused. It also mutates the stored overlay spec's `filter` and `data` in place so base-style switches re-apply what is on screen. |
There was a problem hiding this comment.
"with icon-opacity fading the stops the anchoring refused" — there is no icon-opacity anywhere in src/; StopsLayer's layout has only icon-image, icon-size and icon-allow-overlap, and an unanchored stop is drawn at full opacity at its feed position. Either the fade was dropped or this sentence should go.
(The fade would be worth having, incidentally: with the switch on, the unanchored stops are the ones that did not move, and nothing on the map distinguishes them from stops anchored exactly where their feed puts them — only the panel says which, via notAnchored.)
Generated by Claude Code
Add an option to show GTFS stop at its matched location.
Add OSM Matches view, which shows matches from the OSM elements point of view. In other words Shows unmatched OSM stops in the vicinity of GTFS stop.