CLIM-1322 (2): Map popup name resolution — server-side rewrite + dispatchMapClick refactor#682
Draft
renoirb wants to merge 11 commits into
Draft
CLIM-1322 (2): Map popup name resolution — server-side rewrite + dispatchMapClick refactor#682renoirb wants to merge 11 commits into
renoirb wants to merge 11 commits into
Conversation
…coords() The function selected "province_fr" (or "province") via $term_append but the result row was always read as $result['province']. In FR this produced "Undefined array key" warnings and a null province_short, which in turn yielded titles like "Sainte-Adèle, " (trailing comma). Aliases the column back to the "province" key, matching the pattern already used by cdc_get_location_by_id() at the same file.
…autocomplete
cdc_get_location_by_coords() previously ran a two-pass loop that only
accepted rows whose gen_term was in ('Community','Metropolitan Area'),
falling through to a generic nearest-neighbour query when none was found.
The two paths used different gen_term exclusion lists than
cdc_location_search() — so search and click returned different name
spaces for the same coordinates (e.g. searching "Victoria" landed on
"Fairfield" via the snap; the autocomplete row was filtered out).
Per client clarification 2026-04-17 (Carrington Pomeroy / ECCC), search
and click must return the same names. This commit:
- Removes the $preferred_terms / $found_community two-pass loop entirely.
- Replaces it with a single nearest-neighbour query (range 0.2°,
ORDER BY distance, LIMIT 1).
- Aligns the gen_term NOT IN exclusion with cdc_location_search()'s
('Administrative Region','Province','Territory'), removing
asymmetry between the two endpoints.
In the locationNotFound branch (raw lat,lng paste), showLocation()'s title argument was the opaque 5-char geo_id (e.g. "EPYTF", "EJK8O"), producing a popup title users could not read. The fetched response already carries a human-readable title field built server-side; pass that instead.
…ion_by_coords() Verification at Sainte-Adèle (autocomplete row EQMGV / Town at 45.95,-74.1333334) showed the reverse-lookup at the exact same coord returned EPYTF / Les Pays-d'en-Haut / Census Division at distance 0 — tied distance with the Town row, Census Division winning. Census Division rows are administrative groupings; users never type their names into the search box (autocomplete LIKE matches city-level names), so they never appear in autocomplete suggestions. They should not appear in reverse-lookup either. Note: this change does NOT fully resolve Sainte-Adèle. With Census Division excluded, the next admin row at the same coord wins — EKTUS / Les Pays-d'en-Haut / County Regional Municipality. The asymmetry between LIKE-driven autocomplete and lat/lon-driven reverse-lookup at densely-populated centroids needs broader treatment (see follow-up commit / open question).
Mirrors the Census Division exclusion from the previous commit on the same reasoning: census-level administrative groupings are never selectable through autocomplete (their geo_name doesn't match typed place names) and should not be returnable from reverse-lookup either. Open question (not resolved by this commit): the underlying asymmetry between autocomplete (geo_name LIKE) and reverse-lookup (lat/lon range) allows multiple administrative-grouping rows to coexist at a city's centroid coordinates. Sainte-Adèle (45.95,-74.1333334) currently still returns EKTUS / County Regional Municipality after this change. A broader treatment is needed: either an inclusion list of "user-pickable" gen_terms, or a tiebreaker on the all_areas.scale column so smaller units win at tied distances. Decision deferred — surfacing for review.
… lat/lng The old function had three problems: it dropped its latlng argument (literally `void latlng`) and dispatched the synthetic click at the map container's centre, it required every caller to call map.setView() just before it, and its name described the implementation detail (dispatch a click event) rather than the purpose (drive the map to where the user asked to go and trigger the location-selection cascade there). Renames the file and symbol to moveAndPointAt and folds the setView call inside the function. The synthetic click is now dispatched at map.latLngToContainerPoint(latlng) instead of the container centre, so the click — and therefore the downstream popup-title and data-fetch — lands at exactly the typed coordinates rather than at whatever the map view tile-snapped to. Both call sites updated: - handleLocationChange in search-control.tsx (search and locate-me) - moveToLocation in recent-locations.tsx (recent-locations panel) Removes the now-redundant SEARCH_DEFAULT_ZOOM import from both callers.
…ping rows Atoms 4 and 5 added Census Division and Census Subdivision to the exclusion but Sainte-Adèle still fell through to other admin-grouping rows that share the city's stored centroid coords (Les Pays-d'en-Haut, gen_term = "County Regional Municipality" — an MRC, encompassing many cities; users searching "Sainte-Adèle" do not want that as the popup result). This commit excludes the broader family of administrative-grouping gen_term values that exist in all_areas: counties, regional districts and municipalities, county regional municipalities, district municipalities, generic municipalities, regions, subdivisions, plus the various flavours of districts and unorganised-territory rows. None of these are places a user types into a search bar to find a specific spot on the map; they are containers naming areas that encompass many cities. Verified on dev: Sainte-Adèle, Toronto, Montréal, Québec, Ottawa, Vancouver, Halifax, Calgary, Banff — autocomplete row and reverse- lookup at autocomplete coords now return the same place name in every case. Known limitation: if a user explicitly types the name of an administrative grouping (e.g. "Les Pays-d'en-Haut"), the popup at its centroid will show the nearest non-excluded place instead of the grouping itself. Honouring that case requires passing the selected autocomplete row's id_code through to a fetch-by-id, an architectural change descoped per the analysis plan's D1c.
… its site
The original dispatch-map-click.ts top-of-file JSDoc carried a useful
note explaining why the function enumerates gridPane canvases and
filters by bounding rect rather than calling document.elementFromPoint.
The note was dropped during the rename to moveAndPointAt because the
docblock was reorganised around purpose ("move and point at") and the
implementation detail no longer fit there.
Putting it back as a code comment immediately above the canvas
enumeration where it actually applies — a future reader of that block
needs to know why elementFromPoint is not used (Leaflet's pane wrapper
divs have pointer-events: none, so elementFromPoint returns the
outermost container instead of the canvas tile underneath).
Contributor
Author
|
I'm sharing this but it's still a draft of sorts. This should be about functionality and making sure it solves what we want to solve, and confirming our hypothesis of the improvements we've assumed such changes would give. Once we're settled with all examples of inputs and comparing what this Change-Set does, I'll clean up any excess edits. What I'd like to adjust if applicable:
Code clarifications:
|
…c click The synthetic PointerEvent dispatched by moveAndPointAt forces Leaflet's downstream click handler to rebuild `latlng` via containerPointToLatLng on the integer client pixel coords we just computed via latLngToContainerPoint. That round-trip drops sub-pixel precision (~80 m drift at typical zooms), and for InteractiveRegionOption.GRIDDED_DATA the drifted coord is what fetchLocationByCoords resolves into the popup title — landing on the wrong gridded row at places like Montreal under the current broadened gen_term exclusion list. Restore the caller-supplied precise latlng via a per-map WeakMap channel inside move-and-point-at.ts: the producer (the only path that creates the synthetic click) stashes `latlng` keyed by the L.Map instance immediately before dispatch; the consumer (useMapInteractions.handleClick) reads-and- deletes via consumeIntendedLatlng. One-shot read so a stale entry cannot bleed into a later real user click. WeakMap keying lets entries get GC'd with the map. The channel lives in move-and-point-at.ts (not threaded through React) because the producers — SearchControl and RecentLocationsPanel — and the consumer (useMapInteractions, mounted in Map) sit in three different React subtrees that share only the Leaflet L.Map instance. This way every moveAndPointAt caller benefits with no caller-side opt-in: search, locate- me, and recent-locations all get the precise coordinate. Polygon modes (CENSUS / HEALTH / WATERSHED) are unaffected behaviorally — they override locationTitle from layer.properties.label_en/label_fr — but the latlng now reaching setSelectedLocation is more precise, which feeds R4 downstream consumers more accurately too.
renoirb
added a commit
that referenced
this pull request
May 19, 2026
Edited: 2026-05-19 This branch was originally written BEFORE creating `dispatchMapClick` and has been now merged to main in PR #676. During the rebase that followed 676 the important aspect of `CLIM-1322_Popup-Title-Name-Resolution` was to pass the title. Passing the title was an illustrative "band aid" and we've kept this branch to return back to it later. After Passes 2 supplemental passes (...): - #682 branch `CLIM-1322_2_Popup-Title-Name-Resolution` - #684 branch `CLIM-1322_3_Popup-Title-Name-Resolution` We've decided to return to what this branch initially suggested. Simply to pass the title from the click handler. This commit is there to illustrate what was lost during the iterations to set the record straight. Commits that had gotten lost and were part of this branch this commit is replacing: - 72a0df4 - f9169bb Earlier commit message: Map search popup was showing neighboring Community names (e.g. "Toronto" → "North York") because handleLocationChange fired a synthetic click that discarded the search item's pre-resolved title, forcing handleClick to re-derive from lat/lng via fetchLocationByCoords, which snaps to the nearest 'Community' / 'Metropolitan Area' and filters out 'City' entries server-side. D1a from the Analysis Plan: plumb buildLocationTitle(item) through handleLocationChange into the synthetic click payload as an optional searchProvidedTitle field. handleClick prefers it over fetchLocationByCoords when the mode is GRIDDED_DATA, skipping the network call on the happy path. This is the third optional field on the synthetic click payload (joining layer.properties from CLIM-1223). Submitted as a draft PR for team discussion on whether to continue the bandaid pattern or escalate to the structural refactor in D1c. See [[LLM-Context-ClimateData-Ticket-CLIM-1322-Analysis-Plan]] for the full design trade-off.
renoirb
added a commit
that referenced
this pull request
May 19, 2026
Edited: 2026-05-19 This branch was originally written BEFORE creating `dispatchMapClick` and has been now merged to main in PR #676. During the rebase that followed 676 the important aspect of `CLIM-1322_Popup-Title-Name-Resolution` was to pass the title. Passing the title was an illustrative "band aid" and we've kept this branch to return back to it later. After Passes 2 supplemental passes (...): - #682 branch `CLIM-1322_2_Popup-Title-Name-Resolution` - #684 branch `CLIM-1322_3_Popup-Title-Name-Resolution` We've decided to return to what this branch initially suggested. Simply to pass the title from the click handler. This commit is there to illustrate what was lost during the iterations to set the record straight. Commits that had gotten lost and were part of this branch this commit is replacing: - 72a0df4 - f9169bb Earlier commit message: Map search popup was showing neighboring Community names (e.g. "Toronto" → "North York") because handleLocationChange fired a synthetic click that discarded the search item's pre-resolved title, forcing handleClick to re-derive from lat/lng via fetchLocationByCoords, which snaps to the nearest 'Community' / 'Metropolitan Area' and filters out 'City' entries server-side. D1a from the Analysis Plan: plumb buildLocationTitle(item) through handleLocationChange into the synthetic click payload as an optional searchProvidedTitle field. handleClick prefers it over fetchLocationByCoords when the mode is GRIDDED_DATA, skipping the network call on the happy path. This is the third optional field on the synthetic click payload (joining layer.properties from CLIM-1223). Submitted as a draft PR for team discussion on whether to continue the bandaid pattern or escalate to the structural refactor in D1c. See [[LLM-Context-ClimateData-Ticket-CLIM-1322-Analysis-Plan]] for the full design trade-off.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Popup Title Display Discrepancy When InteractiveRegionOption.GRIDDED_DATA
Related:
WIP
Summary
Rewrites
cdc_get_location_by_coords()and refactorsdispatchMapClickso the map popup title and pin position match what the user asked for, across all four entry paths (autocomplete selection, rawlat,lngpaste, locate-me, manual map click) and both languages.What was wrong
Per the original report and clarifications we've established back around 2026-04-17:
cdc_get_location_by_coords()two-pass loop: it first filtered candidate rows togen_term IN ('Community','Metropolitan Area')and only fell through to a broader query when that failed. Search and click ran through the same endpoint and produced different name spaces because the click-side filter excluded rows autocomplete returned.lat,lngpaste snaps to the nearest DB row: typing44.646445,-63.619798(slightly West of Halifax) sent the marker to44.647650,-63.590240instead. Two distinct user inputs collapsed to the same response."Sainte-Adèle, ","Victoria, ". The PHP function selectedprovince_frvia column-name concatenation but never aliased the returned column back to theprovincekey, so$result['province']was undefined in FR.province_shortcame outnulland thetitleslot for the province abbreviation was empty.dispatchMapClick(map, latlng)dropped itslatlngargument (literallyvoid latlng;) and dispatched the synthetic click at the container's centre. AftersetView(latlng)tile-snapped, the centre's actual lat/lng was off the typed value by 30–120 m — observable as the Session 8 DoD-2 coord-drift hypothesis.EJK8O,EPYTF, …) instead of a place name. ThelocationNotFoundbranch passedlocationByCoords.geo_idtoshowLocationas the title argument; the server already returned a human-readabletitlefield, but it wasn't being used.What this changes
Eight atomic commits on
CLIM-1322_2_Popup-Title-Name-Resolution, one per concrete change:province_frback toprovinceincdc_get_location_by_coords()SELECT — matches the pattern already used bycdc_get_location_by_id()in the same file. Fixes the trailing-comma title.preferred_termstwo-pass; harmonisegen_termexclusion withcdc_location_search()(('Administrative Region','Province','Territory')). Single nearest-neighbour query at range 0.2°. Fixes the Vancouver/Toronto and Victoria search/click name divergence.locationByCoords.titlefor raw-coord popup, notgeo_idinapps/src/components/map-layers/search-control.tsx'slocationNotFoundbranch. Fixes the opaque 5-char popup title.Census Divisionto thegen_termexclusion — first attempt at the Sainte-Adèle case where the Town and the Census Division share centroid coords.Census Subdivisionto thegen_termexclusion — same family.dispatchMapClick→moveAndPointAt; honour the typedlat/lng— the function now takes responsibility forsetView(folded in) and computes the synthetic click'sclientX/clientYviamap.latLngToContainerPoint(latlng)so the click fires at the typed coord, not the tile-snapped centre. Both call sites updated. The "dispatch click" framing was the implementation detail; the new name describes the purpose.gen_termexclusion to drop all administrative-grouping rows — addsCounty,County Regional Municipality, all variants of*Municipality,*District,Region,Regional District,Subdivision,Unorganized Territory, plusAdministrative Sector(lost from the original 4-term exclusion in the previous harmonisation step). None of these are place names a user types into a search bar to find a specific spot. Resolves Sainte-Adèle.elementFromPointworkaround comment as an inline code comment near the canvas-enumeration block — the original JSDoc carried the explanation; it was dropped during the rename refactor and restored where it actually applies.Why these specific changes
Carrington's reply collapsed the (mode × entry path) ambiguity matrix — search and click must return the same names; raw
lat,lngmust keep the typed coords. The fix needed to be at the server, not in the client's title plumbing. An earlier illustration (PR #674, draft) tried passing the autocomplete title through to the popup via asearchProvidedTitleargument; the present PR replaces that approach. The server-side rewrite reaches the click-on-map path that no client-side title plumbing could reach, and matches behaviour across the autocomplete, raw-coord-paste, and locate-me paths because they all go through the same endpoint.The exclusion list grew through three iterations because the data has multiple administrative-grouping rows at city centroids — Census Division (Atom 4), Census Subdivision (Atom 5), then County Regional Municipality and others (Atom 7). Each step was driven by what the data actually returned at the verification anchors. The autocomplete-only filter (Atom 2's exclusion of just
Administrative Region/Province/Territory) was sufficient for major cities (Toronto, Vancouver, Halifax) but not for towns whose centroid coincides with their administrative parent (Sainte-Adèle).The
dispatchMapClickrefactor was on the same path: the function existed to bridge "the user said they want to go here" with "trigger the same cascade as a manual click would", but it had been dropping thelatlngargument and approximating the destination via the container centre. The Session 8 coord-drift was one symptom; Renoir flagged it independently this session as a "the name is a misnomer" observation about the function's purpose vs its name.Verification
API-level probes against
dev-enanddev-fr(raw HTTP, JSON parse, no Xdebug pollution observed in any probe):Mount Pleasant, BCVancouver, BCNorth York, ONToronto, ONLes Pays-d'en-Haut,Sainte-Adèle, QCFairfield, BCVictoria, BCVictoria,(trailing comma)Victoria, BCHalifax, NS(unchanged)Halifax, NSHalifax, NS(snap — original client complaint)Chocolate Lake, NS(no snap;coordscarries typed coords back)Click-precision spot-checks (5–10 km offsets from city centroids) confirm clicks far from a city return locally-appropriate places, not the city — exactly the behaviour the wide 0.2° bounding box was a theoretical concern about. The bounding box is a SQL optimisation;
ORDER BY DISTANCE LIMIT 1finds the truly-nearest allowed row.Known limitations / not done
Urban Community/Railway Point/Railway Junctionrows are now reachable through the click path (the original 4-term exclusion no longer applies). Empirically: a click in downtown Toronto can land onCabbagetown, ON(Urban Community) instead ofToronto, ONbecause Cabbagetown's stored centroid is closer to the click. Within Carrington's "match autocomplete" directive, but flagged for domain-expert review on whether it's the desired behaviour forInteractiveRegionOption.GRIDmap clicks.cdc/v2endpoints (Slice C, separable) — not addressed. Tracked as T23.id_codethrough to a fetch-by-id, which is the architectural change descoped in the analysis plan as D1c (preserved as TechDebt FI1).What this does not touch
Frozen historical artefact: branch
CLIM-1322_Popup-Title-Name-Resolutionand its draft PR #674. Not amended, not pushed, not closed — preserved per Renoir's instruction.Documentation
all_areasschema audit), CI-19 (Sainte-Adèle admin-grouping coord-tie), CI-20 (Halifax raw-coord post-fix behaviour).