Skip to content

AOI: move the analyze/cancel popup onto the core popup service - #309

Draft
CarsonDavis wants to merge 8 commits into
feature/298-map-popup-servicefrom
feature/299-aoi-popup-migration
Draft

AOI: move the analyze/cancel popup onto the core popup service#309
CarsonDavis wants to merge 8 commits into
feature/298-map-popup-servicefrom
feature/299-aoi-popup-migration

Conversation

@CarsonDavis

@CarsonDavis CarsonDavis commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Migrates the AOI analyze/cancel popup onto the core popup service: the plugin now sends one serializable map:showPopup request and branches on the promise's resolved outcome — no popup rendering, no popup CSS, no popup event subscriptions left in the plugin. The legacy overlay API loses its last plugin consumer here but is not retired in this PR (MapControl's measure-distance label still uses it); retirement moves to a follow-up issue.

Closes #299

Stacked on #303 (feature/298-map-popup-service); retarget to development after #303 merges.

What's here

  • _showPopup sends one request and branches on the outcome: 'primary' → the existing analyze hand-off (FetchStats and Chart untouched), 'secondary'/'dismiss' → clear the selection, 'closed' → no-op (AOI replaced or retracted it itself). Rejections are caught and warned.
  • Deleted: AOITooltip.tsx, all .aoi-tooltip* SCSS, the dead tool:change subscription, and the consumer-less plugin:aoi:drawingCancelled event.
  • Orphan fix (the issue's marked gotcha): the moveend one-shot and 1500 ms fallback are tracked and cancelled on supersession and destroy(); map:hidePopup is called up-front in _applySelection and in destroy() — safe unconditionally, since core's ownership check means AOI can only ever close its own popup.
  • Camera restraint (Leave the camera alone unless a selection extends beyond the view #301): the pending-popup cancel token is armed before the async bounds read, so closing the tool or superseding the selection mid-read can't resurrect a popup; a selection already in view opens its card immediately.

Decisions to review

  1. X and click-away clear the selection, same as Cancel (owner-decided 2026-08-11).
  2. A rejected fitBounds opens the popup immediately against the unchanged view, identity-guarded so a late rejection can't resurrect a superseded selection's popup.
  3. tool:change subscription removed (issue-sanctioned; verified dead on the modern path). One incidental change: switching docked tools no longer clears AOI's separated-tool selection.

Gaps in the core service surfaced here (not this PR's scope)

  • A plugin deferring its popup past a camera move must retract the previous one itself, as AOI does — worth a future contract affordance.
  • Accessible-name regression: the old tooltip announced "Confirm analysis area"; the core card announces a generic "Map popup" — wants an ariaLabel field on the map:showPopup contract.

Verification

  • 1488 unit tests green, tsc --noEmit clean. The AOI spec — the plugin's first executed coverage — pins teardown, supersession, and every outcome path; the pending-popup guards are mutation-tested.
  • Manual smoke still needed on both engines: select via draw/search/boundary/upload → popup at the centroid; Analyze → stats/chart flow unchanged; Cancel/X/click-away → selection cleared; close the tool right after selecting → no orphan.

@CarsonDavis
CarsonDavis force-pushed the feature/299-aoi-popup-migration branch from b5e1c36 to e59635c Compare August 11, 2026 22:17
@CarsonDavis
CarsonDavis force-pushed the feature/298-map-popup-service branch from 9c72e45 to cb13772 Compare August 14, 2026 17:14
@CarsonDavis
CarsonDavis force-pushed the feature/299-aoi-popup-migration branch 5 times, most recently from f42e970 to a0216b2 Compare August 19, 2026 15:51
@CarsonDavis
[299] Move the AOI analyze/cancel popup onto map:showPopup
a8db42c 
AOI now asks core for its popup with one serializable request and listens
for the button events on the bus, so the plugin carries no popup DOM, no
popup CSS and no callback across the core boundary.

The Cancel button's event and the popup's dismissEvent are both
plugin:aoi:drawingCancelled, so core's broadcast replaces the plugin's own
emit and the X and a click away clear the selection like Cancel does.

A single _pendingPopup cancel replaces the untracked moveend one-shot and
its fallback timer, cancelled on a superseding selection and on destroy so
a closed tool can never leave a popup behind. It is armed before the camera
is read rather than after: reading the view is asynchronous, and a teardown
or a superseding selection during that hop has to drop this popup instead
of letting it open later. Every path that opens the popup first checks that
it is still the current pending show, so a moveend, the fallback timer and a
rejected fitBounds arbitrate to exactly one popup.

The tooltip's stylesheet goes with its component: the shared .aoi-tool /
.aoi-tooltip rules keep only the tool, and core's popup carries its own CSS.
@CarsonDavis
[299] Add the AOI popup unit spec
eb7d71a 
Covers the map:showPopup request shape, the deferred show, the
hide-before-show ordering, label escaping, the analyze and cancel
hand-offs, and teardown while a show is still pending.

Also pins the three pending-show defects the migration fixes: two rapid
selections leaving two live one-shots, a superseded selection's rejected
fitBounds opening its own popup, and a pending show surviving destroy.
@CarsonDavis
[299] Take the AOI popup outcome off the request promise
ef069bf 
`map:showPopup` now answers its own request with how the popup closed, so
AOI no longer names events for core to broadcast. The two popup event
constants and their subscriptions are gone; `_showPopup` awaits the
request and branches on the result: a primary press hands the feature to
the analysis consumers, a secondary press and a dismissal (the X or a
click on the map) clear the selection, and `closed` — a retract or a
replacement AOI itself caused — leaves the selection alone. A rejected
request, which is how core reports a malformed request or a mount
failure, is warned about instead of surfacing as an unhandled rejection.

Cancelling a pending show also joins `_clearSelection`, so a cleared
selection cannot leave a popup queued behind the camera.

The spec drives the popup slot the way core does — one popup at a time,
answered on its own promise — and covers each outcome, the rejection
path, and the teardown and supersession cases it already pinned.
@CarsonDavis
[299] Sharpen the AOI popup outcome handling
5875a07 
Take the up-front retract in `_applySelection` off the list of things that
look deletable: the comment now says outright that it is load-bearing. A
reader who knows `map:showPopup` replaces the open popup for itself can
otherwise conclude the extra `map:hidePopup` is redundant, and it is not —
this selection's popup waits for the camera to settle, so without the
retract the previous popup is still the open one when core's deferred
`map:click` dismissal runs, and it answers as a dismissal that clears the
selection just made.

Drop the `_cancelPendingPopup()` guard from `_clearSelection`. It is dead:
`_showPopup` only ever runs with `_pendingPopup` already null (from
`settled`, which cancels first, or from the branch that never arms one),
`_clearSelection` has a single call site on the popup's outcome, and a
selection that arms a new pending popup retracts the open one first, which
answers it `closed` and never reaches the clear. Its spec case went with
it — it exercised a state no path produces.

Split the outcome and rejection handlers into a two-arg `then`, so a throw
out of an `analysisAOIReady` consumer is no longer logged as a failure to
show the popup.

Also list the one-shot `map:moveend` subscription in the header contract
and drop two fake-api accessors the spec never reads.
@CarsonDavis
[299] Drive the popup spec through the camera read
655651b 
The selection path now reads `map:getBounds` before it decides whether to
move the camera, so the fake api answers that request and the flush helper
runs the chain of microtasks it adds instead of a single tick. Without both,
every case here asserts against a selection that has not reached its popup
yet.

Three cases follow from that read. A selection already inside the view opens
its popup immediately, with no fitBounds and nothing subscribed to moveend.
A rejected fitBounds opens the popup once, anchored against the view read a
moment earlier, and the fallback timer that follows adds none. And a torn
down or superseded selection is dropped mid-read, before it ever subscribes.

The teardown case now covers the whole contract rather than the popup alone:
closing the tool, switching away from it and collapsing its panel all reach
the plugin through `destroy()`, which has to leave no selection, no highlight
and no popup behind.
@CarsonDavis
[299] Keep AOI to its own popup and its own pending slot
59643b7 
`map:hidePopup` empties core's single popup slot whoever filled it, so AOI
retracting unconditionally would close another plugin's popup on teardown or
on the next selection. Track the popup AOI has on screen — the request that
goes unanswered for exactly as long as it is up — and ask for a hide only
while that is AOI's own. The slot is released however the request answers,
including the 'closed' another plugin's popup produces.

A camera step that threw also left `_pendingPopup` pointing at a token
nothing could settle, which claimed a popup was on its way when none was.
Release it from the failure path, and only while it is still that chain's, so
a selection that superseded it keeps its own.

The comment on the deferred show promised more than the code does: the
one-shot settles on the next `map:moveend`, whichever movement produces it.
Say so, and record why it is subscribed before the fit is requested —
`mmgisAPI.request` runs its provider synchronously and a fit with no
transition emits `moveend` from inside that call.
@CarsonDavis
[299] Let core decide whose popup a retract reaches
cc4ed63 
AOI tracked whether the popup on screen was its own so it could stay quiet
otherwise. Core answers that now — it retracts a popup only for the caller
that opened it — so the tracking is gone, and teardown and the up-front
retract in a new selection just ask.

Popup requests go through AOI's own bus handle, which stamps them with AOI's
id; that stamp is the whole reason core can tell. The handle's stand-in gained
a `request` to match, so a tool made without a bus still degrades to no-ops
rather than crashing the first time it reaches for one.

The up-front retract keeps its job of beating the deferred click-away, and is
race-free by construction now: it can only ever close AOI's own popup.
@CarsonDavis
CarsonDavis force-pushed the feature/299-aoi-popup-migration branch from a0216b2 to cc4ed63 Compare August 19, 2026 16:15
@CarsonDavis

CarsonDavis commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Follow-up carried over from #303's body (trimmed out of it during review):

Overlay retirement — map:addOverlay/removeOverlay are @deprecated in #303 and this PR removes their last plugin consumer (AOI). Full retirement needs its own follow-up issue once these land: MapControl's measure-distance label (merged after these branches were cut) still uses them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@CarsonDavis