Skip to content

CLIM-1322: Map search popup title (Illustration Keep As-Is, or Rework?)#674

Closed
renoirb wants to merge 4 commits into
mainfrom
CLIM-1322_Popup-Title-Name-Resolution
Closed

CLIM-1322: Map search popup title (Illustration Keep As-Is, or Rework?)#674
renoirb wants to merge 4 commits into
mainfrom
CLIM-1322_Popup-Title-Name-Resolution

Conversation

@renoirb
Copy link
Copy Markdown
Contributor

@renoirb renoirb commented Apr 14, 2026

Popup Title Display Discrepancy When InteractiveRegionOption.GRIDDED_DATA

Related:

CLIM-1322 — Map search popup title matches the user's selection

⚠️ This is a draft PR submitted as an illustration for a design discussion. It closes a user-visible bug, but its scope is narrower than the root cause. The team is asked to decide between merging it as-is (D1a), fixing the server-side root cause (D1b), or escalating to a structural refactor that eliminates the synthetic click pattern (D1c). See the "What the team is asked to decide" section below.

User story

When a user searches a place in the map search bar and selects a match from the autocomplete, the map navigates to that place, drops a pin, and the popup title shows the exact words the user selected.

That is the behaviour the submitter (Carington Pomeroy, Environment and Climate Change Canada) reported as broken: searching "Vancouver" returned "Mount Pleasant" in the popup; searching "Toronto" returned "North York". The map pin was in roughly the right place, but the name was a neighbouring community — whatever happened to be closest in the all_areas location database.

This PR makes the search path honour the user's selection. That is probably what the client wants and expects.

Before / After

Before (from the original ticket screenshot — included as CLIM-1322_Original-Ticket-Illustration-1.png in the ticket attachments):

Caption — Before. Search bar reads Toronto, (City), York, Ontario. Popup title reads North York, ON. Map pin visibly south of the "TORONTO" label. Two sources of truth on one popup.

After:

Caption — After. Same search, same selection. Popup title now reads Toronto, (City), York, Ontario — byte-for-byte what the user picked from the dropdown.

Why it was broken

The search autocomplete queries WordPress endpoint cdc/v2/location_search, which returns rows from the all_areas database filtered by geo_name LIKE ... and excluding ('Administrative Region', 'Province', 'Territory'). The client formats each row as:

{text}, ({term}), {location}, {province}

…and displays the list in the dropdown. A single place name appears as multiple rows — for example, Toronto appears as five distinct rows: (City), (Island), (Lake) (Thunder Bay), (Community), (Locality) (PEI):

Caption — Five distinct "Toronto" rows exist in all_areas, each with a different gen_term and different lat/lng. The user picks one from the dropdown and the map is supposed to reflect that exact choice.

When the user picks a row and clicks it, Leaflet's L.Control.Search fires its moveToLocation(latlng, title, map) callback. The second argument — title — is the user's selected text. Prior to this PR, the code was silently discarding it:

// Before (on main, post-CLIM-1223)
moveToLocation: (latlng: SearchLatLng) => {
    handleLocationChange(latlng);
},

With only latlng surviving, handleLocationChange() fires a synthetic click event on the vector layer carrying only { latlng, layer: { properties } } (the properties part came from CLIM-1223). handleClick() in use-map-interactions.tsx then re-derives the title by calling fetchLocationByCoords(latlng) → WordPress endpoint cdc/v2/get_location_by_coords, which in fw-child/resources/functions/rest.php:

$preferred_terms = [ 'Community', 'Metropolitan Area' ];
$ranges = [ 0.05, 0.1, 0.2 ];
// ... progressive-radius search, ORDER BY DISTANCE, pick first row matching preferred_terms

The filter excludes 'City', 'Island', 'Lake', 'Locality', etc. The only accepted gen_terms are 'Community' and 'Metropolitan Area'. So:

  • Toronto's row is gen_term = 'City' — filtered out
  • The progressive radius search picks the nearest 'Community' — typically North York
  • Popup title becomes "North York, ON"

This is a filter mismatch between two sibling endpoints (cdc_location_search and cdc_get_location_by_coords) that both query the same all_areas table with different gen_term rules. The autocomplete is willing to return a City; the coordinate lookup is not.

The fix

Forward the title argument Leaflet is already providing to us. Attach it to the synthetic click event payload as searchProvidedTitle. In handleClick(), prefer it over fetchLocationByCoords() when the mode is GRIDDED_DATA.

-moveToLocation: (latlng: SearchLatLng) => {
-    handleLocationChange(latlng);
+moveToLocation: (latlng: SearchLatLng, title: string) => {
+    handleLocationChange(latlng, title);
 },
 vectorLayer.fire('click', {
   latlng: { lat: latlng.lat, lng: latlng.lng },
+  ...(searchProvidedTitle ? { searchProvidedTitle } : {}),
   ...(properties ? { layer: { properties } } : {}),
 });
-const locationByCoords = await fetchLocationByCoords(latlng);
-const locationId = locationByCoords?.geo_id ?? `${locationByCoords?.lat}|${locationByCoords?.lng}`;
-let locationTitle = locationByCoords.title;
+let locationId: string;
+let locationTitle: string;
+
+if (searchProvidedTitle && interactiveRegion === InteractiveRegionOption.GRIDDED_DATA) {
+  locationTitle = searchProvidedTitle;
+  locationId = `${latlng.lat}|${latlng.lng}`;
+} else {
+  const locationByCoords = await fetchLocationByCoords(latlng);
+  locationId = locationByCoords?.geo_id ?? `${locationByCoords?.lat}|${locationByCoords?.lng}`;
+  locationTitle = locationByCoords.title;
+  // CLIM-1223 non-gridded branch preserved byte-for-byte below
+  ...
+}

Diff scope: 2 files, 17 insertions / 8 deletions.

  • apps/src/components/map-layers/search-control.tsx
  • apps/src/hooks/use-map-interactions.tsx

Not touched: services.ts, interactive-regions-layer.tsx, rest.php, any WordPress code, any GeoServer code. The real VectorGrid click listener and the CLIM-1223 non-gridded branch are preserved byte-for-byte.

Base commit: branched from 4b6c49bf — the CLIM-1223 merge commit — so the diff reads as a clean delta over the prior fix in the same code area.

Bonus: when the search path provides a title, fetchLocationByCoords() is skipped entirely — one fewer network round-trip on the happy path.

Verification

Manual test matrix

All 7 rows of the planned matrix green, plus bonus coverage across eight distinct gen_term categories. Screenshots attached.

# gen_term Example Mode Was broken on main? Result Screenshot
1 (City) Toronto gridded ✅ yes (→ "North York") ✅ fixed Toronto-1
2 (City) Toronto repeat gridded Toronto-2
3 (Island) Toronto Island gridded ✅ yes (filter excludes) ✅ fixed Toronto-3
4 (Community) Toronto, (Community) gridded ❌ already worked ✅ no regression Toronto-4
5 (Islands) — French Îles de Toronto gridded ✅ yes (filter + accented Î) ✅ fixed Toronto-5
6 (Lake) Prout Lake, MB gridded ✅ yes (filter excludes) ✅ fixed Prout-Lake-1
7 (City) Ottawa Watersheds N/A (non-gridded path) ✅ watershed label wins Ottawa-Watersheds
8 (Town) — French é Montréal Watersheds N/A (non-gridded path) ✅ watershed label wins Watersheds-Montreal-1
9 N/A (no search) Three downtown Toronto clicks gridded, real click N/A — unchanged ✅ click path untouched, DI1 visible Toronto-7-Mouse-Click-1/2/3

Caption — (Island) variant. Toronto Island, (Island), York, Ontario — popup title matches. On main, (Island) is filtered out by preferred_terms, and the search would snap to a neighboring Community.

Caption — (Community) non-regression. Toronto, (Community), York, Ontario — popup title matches. (Community) is in preferred_terms, so this variant was already working on main. Still works. No regression introduced.

Caption — Noms de lieux en français / French place names. Îles de Toronto, (Islands), York, Ontario is the French name for Toronto Islands — stored in all_areas as a distinct row from the English Toronto Island. The popup title preserves the selection byte-for-byte, accented Î included. It also implicitly rules out any encoding or escape issues in the searchProvidedTitle propagation path.

Caption — (Lake) — Manitoba wilderness. Prout Lake, (Lake), Manitoba — popup title matches. Prout Lake is in a blank-grid wilderness area with no nearby cities; on main, the search would have snapped to whatever (Community) happens to be closest within 0.2°, typically something miles away and completely unrelated. This exercises the failure mode more cleanly than Toronto because (Lake) is in a gen_term category the server-side filter explicitly excludes from the preferred match loop.

Interactive region mode — CLIM-1223 preservation

The new D1a branch in handleClick() is explicitly guarded on interactiveRegion === GRIDDED_DATA. In Watersheds / Census / Health modes, the guard fails, the D1a code is skipped entirely, and control falls through to the existing CLIM-1223 layer.properties.label_en override. The watershed / region label wins — which is the correct product behaviour for non-gridded interactive region modes (users want the region name, not the populated place).

Caption — Watersheds mode + Ottawa search. Search bar: Ottawa, (City), Carleton; Russell, Ontario. Popup title: Lower Ottawa – South Nation. The watershed label wins because the D1a guard is GRIDDED_DATA-only; CLIM-1223's layer.properties.label_en override path is preserved byte-for-byte.

Caption — Watersheds mode + Montréal search. Search bar: Montréal, (Town), Montréal, Quebec. Popup title: St. Lawrence Drainage Area. Same product behaviour — in Watersheds mode, the watershed label is what the user wants, not the populated place name. French é in Montréal is preserved cleanly through the search flow, matching the Î in Îles de Toronto. Note the (Town) gen_term — also not in preferred_terms, so Montréal, (Town) in gridded mode on main would also have snapped to a neighbouring Community.

Click path (unchanged) — and the DI1 demonstration

The search path is fixed; the real click path is intentionally untouched. This PR does not change what happens when a user clicks directly on a gridded cell without using the search bar. That path still routes through fetchLocationByCoords() and still exhibits the underlying filter snap. The three screenshots below are three direct map clicks, no search, in downtown Toronto:

Caption — Click 1 (no search). Popup title: East York, ON.

Caption — Click 2 (no search). Popup title: Toronto, ON. This click happened to land close enough to Toronto's own (Community) row to win the distance race.

Caption — Click 3 (no search). Popup title: Gibraltar Point, ON. Gibraltar Point is a place on Toronto Islands.

Three clicks within a few grid cells of each other in downtown Toronto return three different neighbour names. Sub-degree click precision determines which (Community) row wins the preferred_terms + distance race on the server. This is the same filter mismatch described at the top of this PR — cdc_get_location_by_coords() excludes 'City' / 'Island' / 'Lake' / etc. from the preferred match loop — but triggered by raw click position rather than search selection. This PR deliberately does not address the click path (scope boundary: CLIM-1322 is a search-bar ticket). The screenshots make the asymmetry visible: search is fixed, click is not. That is the honest state of the codebase after this PR, and it is part of the team discussion — see "What the team is asked to decide" below.

This is also why the submitter's additional comment ("if I search by raw lat, lon, I'm taken somewhere nearby instead of where I typed") is not closed by this PR. The locationNotFound path also routes through fetchLocationByCoords().

Defensive properties

  • No XSS vector introduced. searchProvidedTitle is sourced from buildLocationTitle(dbRow) — a formatted string built from all_areas row fields (text, term, location, province). It is never raw user input. Invalid or injection-style strings typed into the search bar are rejected by L.Control.Search's autocomplete filter before moveToLocation fires; the dropdown returns no matches and no title is ever constructed.
  • CLIM-1223 preservation. The non-gridded branch in handleClick() (lines 79–87 of the post-CLIM-1223 file) is byte-for-byte unchanged. Only its outer indentation level shifted because it is now nested inside a new else branch. Verify by applying the diff hunk against 4b6c49bf — the inner statements match character-for-character.

Visual aid — the combined CLIM-1223 + CLIM-1322 changeset

For the team discussion, here is the combined diff of both PRs in one view, starting from the commit immediately before CLIM-1223 was merged:

Combined CLIM-1223 + CLIM-1322 compare view

4b6c49b...72a0df4

This shows the layering in one screen:

  1. Baseline — synthetic click carries only { latlng }
  2. + CLIM-1223 — adds layer.properties to the payload when the search path pre-fetches the region feature
  3. + CLIM-1322 (this PR) — adds searchProvidedTitle to the payload when the search path has an autocomplete selection

Each of the two tickets extends the synthetic click event with one more optional field, stacked on top of the baseline latlng. The compare view makes the "three spreads" architectural point visible at a glance — it is the concrete substrate for the team discussion below. Looking at the combined diff, the vectorLayer.fire('click', { latlng, ...(properties ? { layer: { properties } } : {}), ...(searchProvidedTitle ? { searchProvidedTitle } : {}) }) call site tells the story of the pattern more clearly than any prose.

What the team is asked to decide

This PR is submitted as a draft to surface three candidate approaches for closing CLIM-1322. D1a (this PR) is the minimum-blast-radius choice. The team should review all three and decide deliberately:

D1a — attach searchProvidedTitle to synthetic click (this PR)

  • Files touched: 2 (search-control.tsx, use-map-interactions.tsx)
  • Blast radius: search path only. Real click path, all interactive region modes, locate-me, raw-coord input all untouched.
  • Architectural cost: this is the third optional field on the synthetic click event payload (joining latlng baseline + layer.properties from CLIM-1223). The synthetic click vectorLayer.fire('click', fakeEventObject) is already not a real event — it is a procedure call disguised as an event, and every new ticket in this code area bolts one more field onto the fake object. The diff you are reading in this PR is the concrete illustration of what the next bandaid looks like. If the team accepts this as-is, there is every reason to expect a CLIM-13XX next year that needs a fourth field.

D1b — fix the PHP endpoint server-side

  • Files touched: 1 (fw-child/resources/functions/rest.php)
  • Mechanism: widen preferred_terms to include 'City' / 'Island' / etc., or prefer the row with smallest distance regardless of gen_term (excluding the existing Railway/Administrative blacklist).
  • Blast radius: broader — fixes the bug for every caller (search, locate-me, raw-coord input, recent locations, real click path), but also changes behaviour for the real map-click flow in ways that may surprise users. Requires a PHP deploy and a broader test matrix. Carington's raw lat, lon comment is closed by this path.

D1c — eliminate the synthetic click pattern entirely

  • Files touched: 3–5 (search-control.tsx, use-map-interactions.tsx, interactive-regions-layer.tsx or wherever the real VectorGrid click listener lives, possibly a new lib/map-selection.ts)
  • Mechanism: extract a shared function selectLocation({ latlng, interactiveRegion, title?, featureProperties? }). Both the search path and the real click listener call it directly. vectorLayer.fire('click', ...) is deleted. Title priority cascade (search-provided → feature properties → fetchLocationByCoords) lives in one place.
  • Blast radius: larger, but kills the recurrence class. Full click-path regression matrix across all region modes (gridded, watershed, health, census, shapefile). Aligns with the project's backporting philosophy — replacing a legacy pattern properly while fixing a user-visible bug.
  • Closes: the architectural observation first raised during CLIM-1223's review (that vectorLayer.fire('click', fakeObject) is a procedure call masquerading as an event), and the same observation re-surfaced by this ticket's analysis. Also closes the click-path asymmetry demonstrated in the DI1 screenshots above.

The implementation finding that is load-bearing for this decision

One surprising thing came out of implementing this PR. When I went to figure out how to capture the user's selected search item from L.Control.Search, I expected to need a useRef or a closure over formatData's _recordsCache. In reality:

L.Control.Search.moveToLocation callback signature is (latlng, title, map). The title — the user's selected autocomplete text — was already being handed to us as the second argument. The original code was discarding it silently.

This is load-bearing for the D1a / D1b / D1c decision because it cuts both ways:

  1. It makes D1a very cheap. One parameter forwarded, one optional field on the synthetic click. Symmetric with CLIM-1223's layer.properties pattern.
  2. It makes the architectural critique sharper. The synthetic click pattern is not just dropping data the search API "knows." It is dropping data that Leaflet is handing directly to the hook point, on the wire, as a positional parameter. That is the most concrete possible instance of the "procedure call disguised as an event" critique first surfaced during CLIM-1223's review.

Reviewers should weigh both sides.

What this PR deliberately accepts

  • The PHP server-side root cause remains. cdc_get_location_by_coords() still filters preferred_terms and still snaps to a neighbouring Community. Locate-me and raw-coord input (Carington's Halifax example) still exhibit the bug. Demonstrated live in the three click screenshots above: three clicks near each other in downtown Toronto return three different neighbour names.
  • The synthetic click payload gets a third optional field. Joining latlng baseline and layer.properties from CLIM-1223, this PR adds searchProvidedTitle. The bandaid layer is visibly thicker — that is the point of the illustration, not an accident.
  • locationId divergence in addRecentLocation. Search entries use a ${lat}|${lng} composite as locationId; click entries use locationByCoords.geo_id. A user who searches for a place and later clicks near it may see both as separate recent-location entries. Observable during manual testing; not visibly broken in this round of verification.

Out of scope (deliberate)

  • Locate-me title fix — no user-provided name to substitute. Any "fix" there is a product decision, not a bug fix.
  • Raw lat, lon input snapping — Carington's Halifax example. Same reason as locate-me; needs D1b or D1c.
  • PHP filter fix — that is D1b, a separate ticket.
  • Synthetic click elimination — that is D1c, a separate decision.
  • Shapefile mode — user-uploaded shapefiles have no name properties; out of scope from the ticket's scope boundary.
  • Mixed tab/space indentation in the CLIM-1223 inner block preserved byte-for-byte. A focused T8 cleanup is possible if the team wants it in this PR.

Related

@renoirb renoirb self-assigned this Apr 14, 2026
@renoirb renoirb changed the title CLIM-1322: Map search popup title (D1a draft for team discussion) CLIM-1322: Map search popup title (Illustration Keep As-Is, or Rework?) Apr 14, 2026
Comment thread apps/src/hooks/use-map-interactions.tsx Outdated
Comment on lines +74 to +94
const locationByCoords = await fetchLocationByCoords(latlng);
const locationId = locationByCoords?.geo_id ?? `${locationByCoords?.lat}|${locationByCoords?.lng}`;
let locationTitle = locationByCoords.title;
let locationId: string;
let locationTitle: string;

if (searchProvidedTitle && interactiveRegion === InteractiveRegionOption.GRIDDED_DATA) {
locationTitle = searchProvidedTitle;
locationId = `${latlng.lat}|${latlng.lng}`;
} else {
const locationByCoords = await fetchLocationByCoords(latlng);
locationId = locationByCoords?.geo_id ?? `${locationByCoords?.lat}|${locationByCoords?.lng}`;
locationTitle = locationByCoords.title;

// For non-gridded data, try to get the title from the layer properties, if available.
if (interactiveRegion !== InteractiveRegionOption.GRIDDED_DATA) {
if (interactiveRegion !== InteractiveRegionOption.GRIDDED_DATA) {
if (layer) {
if (locale === 'en') {
locationTitle = layer.properties.label_en ?? '';
} else if (locale === 'fr') {
locationTitle = layer.properties.label_fr ?? '';
}
}
}
Copy link
Copy Markdown
Contributor Author

@renoirb renoirb Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dates back from about a year ago (2025-06) #558. There's a few things to clean up and simplify.

... including TAB over spaces (hence the strange indentation)

@renoirb renoirb force-pushed the CLIM-1322_Popup-Title-Name-Resolution branch 2 times, most recently from 72a0df4 to f9169bb Compare April 14, 2026 16:10
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 renoirb force-pushed the CLIM-1322_Popup-Title-Name-Resolution branch from 45f1a00 to 904d0dc Compare May 19, 2026 17:03
@renoirb
Copy link
Copy Markdown
Contributor Author

renoirb commented May 19, 2026

Keeping Pass 1 for historical purposes.

Resolved by #685

@renoirb renoirb closed this May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant