AOI: move the analyze/cancel popup onto the core popup service - #309
Draft
CarsonDavis wants to merge 17 commits into
Draft
AOI: move the analyze/cancel popup onto the core popup service#309CarsonDavis wants to merge 17 commits into
CarsonDavis wants to merge 17 commits into
Conversation
CarsonDavis
force-pushed
the
feature/299-aoi-popup-migration
branch
from
August 11, 2026 22:17
b5e1c36 to
e59635c
Compare
CarsonDavis
force-pushed
the
feature/298-map-popup-service
branch
from
August 14, 2026 17:14
9c72e45 to
cb13772
Compare
CarsonDavis
force-pushed
the
feature/299-aoi-popup-migration
branch
6 times, most recently
from
August 19, 2026 16:15
a0216b2 to
cc4ed63
Compare
Collaborator
Author
|
Follow-up carried over from #303's body (trimmed out of it during review): Overlay retirement — |
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.
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.
`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.
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.
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.
`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.
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.
The analyze/cancel card is a title over its two buttons, so the label travels in the request's `title` field and no html is sent at all. Core renders a title as text and names the card by it, which is what the tooltip this replaced announced.
CarsonDavis
force-pushed
the
feature/299-aoi-popup-migration
branch
from
August 26, 2026 19:37
d97bd31 to
b550739
Compare
…up-migration # Conflicts: # src/essence/Tools/AOI/AOITool.js
A `fitBounds` rejection cannot be cancelled once it is in flight, so it is the one route into the settle path that outlives the selection that started it. Left to run it disarms the current selection's pending show and opens the previous selection's card. The spec holds each fit open so that rejection can be fired after a second selection has taken over. The mocked handles stamp their caller the way the real one does, in the options object, rather than in the argument beside it that the bus now refuses.
Fold the payload permutations into the main request test, drop the camera-deferral scenarios aoiSelectionCamera.spec.js already covers, and keep one pin per ownership outcome.
7 tasks
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.
Moves the AOI analyze/cancel popup onto the core popup service. The plugin sends one serializable
map:showPopuprequest and branches on the promise's resolved outcome — no popup rendering, no popup CSS, and no popup event subscriptions remain in the plugin.Closes #299
What this changes
AOITooltip.tsxand all of its styling are deleted; AOI carries no popup code at all.Stacked on #303 (
feature/298-map-popup-service); retarget todevelopmentafter #303 merges.Decisions for review
1. The X and a click on the map clear the selection, the same as Cancel.
Only
'closed'— the popup going away without the user acting on it — leaves the selection standing.2. The selection label travels as
title; nohtmlis sent at all.The card is a title over its two buttons. AOI no longer builds markup or escapes anything, and the core names the card by its title for assistive technology. The label is whatever the selection produced — "Drawn polygon", a boundary name, an uploaded filename.
3. The
tool:changesubscription is removed.It was dead on the modern path. One incidental consequence: switching docked tools no longer clears a separated-tool selection.
4. AOI is the worked example for #303's teardown behaviour.
How core closes popups on tool teardown is #303's decision; AOI is where its consequences show. See #303 for the reasoning.
Known gaps
tool:changesubscription means switching tools no longer clears an AOI selection — see decision 3. Nothing pins it either way.