Isolate bus listeners so one handler cannot abort an emit - #324
Merged
Conversation
This was referenced Aug 19, 2026
sandesh-sp
force-pushed
the
fix/bus-emit-listener-isolation
branch
from
August 19, 2026 14:33
ff7f4ac to
92a96fd
Compare
Dispatching from events.all rather than through mitt's own emit is what lets a throwing listener be caught and skipped, and it puts three of mitt's rules in this file's hands: the subscriber list is copied before iteration, specific listeners run before wildcards, and wildcards are called with (type, data). Only the wildcard signature was tested. Removing the copy, reordering the two passes, or handing every listener the wrong arguments all left the suite green, and so did dropping the try/catch around the wildcard pass. Tests for each of those, plus the plainest promise of the lot — that an ordinary listener receives the payload it was emitted with. Cleanups move to afterEach. Run after the assertions, an unsubscribe is skipped by the first failure, and the next test in the file inherits a live listener on a bus that outlives it.
CarsonDavis
approved these changes
Aug 25, 2026
CarsonDavis
left a comment
CarsonDavis
left a comment
Collaborator
There was a problem hiding this comment.
Approving, but maybe worth a follow-up: this PR re-implements mitt's delivery, leaving mitt doing only the storage. That storage is just a Map and two functions we could own ourselves and drop the dependency (needs an off() miss-guard, and the bus in its own module so the test mock drops mitt too).
sandesh-sp
added a commit
that referenced
this pull request
Aug 27, 2026
## Why
Panels and plugins can only be driven from inside core. `PanelManager_`
and `ToolControllerModern_` are reachable by direct import, which a
sandboxed plugin cannot do, so a plugin that wants to collapse the panel
it lives in — or reveal a sibling plugin — has no way to read the state
or act on it.
The state owners were also awkward to drive even from inside core. They
signalled refusal by throwing, which does not survive a bus boundary,
and panels offered only `togglePanelCollapsed`: a toggle acts on
whatever it finds rather than on what the caller asked for, so a control
working from a listing it read a moment ago flips the wrong way, and a
retry undoes itself.
## What
**Results instead of exceptions.** `setPanelState` returns `{ ok: true,
state, changed }` or `{ ok: false, reason }`. `canSetState` exposes the
same constraints, so a control can decide whether to draw itself at all
rather than discovering the answer by being clicked. `showPanel`
replaces the toggle and names the state it resolves to.
**Plugin lifecycle as states.** `ToolControllerModern_`'s separate
loaded and hidden flags become one of `unloaded`, `hidden`, `visible`,
reachable through `setPluginState`. Asking for `visible` on an unloaded
plugin loads it.
**Eight handlers on the bus** plugins already use for everything else:
| | |
|---|---|
| `panels:getAll` / `plugins:getAll` | the current listing |
| `panels:setState` / `plugins:setState` | the primitive |
| `panels:show` / `panels:hide` | sugar, resolving a target state |
| `plugins:show` / `plugins:hide` | same |
Handlers register at module load and stay registered. An absent layout
is reported as a `layout-inactive` **result**, not a missing handler, so
a caller sees one failure vocabulary whichever layer refused. The
managers are read through getters rather than captured at registration,
since the UI clears them on teardown.
**Two events.** `panels:changed` and `plugins:changed` carry the same
listing the `getAll` handlers return, so a subscriber never has to
re-request. `panels:changed` replaces `mmgis-panel-layout-changed`.
Payloads are frozen projections holding no core references, so they
survive a structured clone — which is what a future sandbox bridge will
have to do to them.
Matching typed wrappers land in
`src/essence/Tools/_shared/adapters/mmgisAPI.ts`, the one place the bus
name strings appear.
## Scope
Mostly additive, with two breaking changes worth knowing about before
merge:
- `PanelManager_.togglePanelCollapsed` is removed, from both the class
and the interface. A toggle acts on whatever it finds, which is the
problem this PR exists to fix; `showPanel` and `setPanelState` name the
state they resolve to instead.
- `mmgis-panel-layout-changed` is renamed to `panels:changed`.
Everything else is additive. The existing `mmgisAPI.showPanel` /
`hidePanel` / `togglePanel` methods and the `core:*` command events all
still work — they are reimplemented on the new primitives here and
removed in #325.
## Defects fixed along the way
Two, both in the state owners this PR reworks:
- `panels:show` on an already-visible panel resolved a target state and
so could *shrink* it. Every stock layout gives the left panel an
`iconified` default, so showing an expanded panel iconified it. Show now
only ever lifts a panel out of `collapsed`.
- A failed plugin transition reported `not-found`, pointing a reader at
the plugin id — the one thing already known to be good, since the id was
resolved before the transition was attempted. It reports
`transition-failed`.
## Testing
Provider specs for all eight handlers (refusal vocabulary, malformed
payloads, absent layout, the float-position restriction),
`PanelManager_` command specs, plugin state-transition specs, and the
typed wrappers.
**1458 tests passing, `tsc --noEmit` clean.** Every commit in the stack
was verified green in isolation, not just the tip.
## Related
Second of a three-part stack: #324 → **this** → #325.
Note for #323: this replaces the API that branch was written against,
and the break lands when **this** PR merges — not #325.
`panels:toggleCollapsed` is not registered (toggle was dropped
deliberately, see above), `collapsible` is not on the listing, and the
event is now `panels:changed`. #323 needs a follow-up to move onto
`panels:show` / `panels:hide` driven off the `state` it already reads.
sandesh-sp
added a commit
that referenced
this pull request
Aug 27, 2026
## Why With the request surface in place (#322), there were three ways to collapse a panel: a direct `PanelManager_` call, an `mmgisAPI.hidePanel` method, and a `core:hidePanel` event. Two of them are strictly worse than the third. `core:*` events are fire-and-forget, so a refusal has nowhere to go — a config naming a panel that does not exist produced nothing at all. The direct `mmgisAPI` methods cannot cross a sandbox boundary, since `postMessage` cannot carry a function reference. Both are removed here. ## What **Removed:** `_initCoreCommandDispatcher` and the seven `core:*` command events; `showPanel`, `hidePanel`, `togglePanel`, `showPlugin`, `hidePlugin`, `loadPlugin`, `unloadPlugin`, `isPluginLoaded`, `isPluginHidden` from `mmgisAPI`; `togglePanelCollapsed` from `PanelManager_`. **Panel controls** issue requests and log the refusal reason rather than clicking dead, and decide whether to draw themselves from `canSetState` instead of reading `allowedStates` directly — which missed the float-position restriction, so a float panel offered a Minimize button that stranded it. **Config action strings** resolve through one shared resolver, so a mission author reads the same request names the API documents rather than a second tool-specific vocabulary. `core:` is reserved rather than reused: its verbs have no expressible successor here (`togglePanel` is gone; `unloadPlugin` needs a state argument a colon-delimited string cannot carry), so a config still carrying `core:showPlugin:LayersTool` gets a warning naming the supported actions instead of emitting an event nothing listens for. **Consumers migrated:** Chart (`plugins:show` on analysis-ready, `plugins:setState` on close), AOI (unload on close), Title (through the resolver), AddTempLayer. **Teardown announces itself.** `panels:changed` and `plugins:changed` were silent when a layout came down, on the reasoning that teardown destroys the subscribers with it. The bus outlives the layout, so a plugin that subscribed is still subscribed and still holding a roster of panels and plugins that no longer exist. Both events now fire once more with an empty listing, which agrees with what the providers report from that point on. `PanelManager_` gains `clear()` so that goes out once rather than once per panel. ## Regression fixed AddTempLayer was calling `window.mmgisAPI.showPlugin` / `hidePlugin` through a locally declared optional type. Those methods are removed here, and optional chaining would have turned both calls into silent no-ops. The tool starts hidden and ships no trigger of its own, so its only door would have stopped opening and its close button would have stopped closing — with no error anywhere. It goes through the shared client's plugin wrappers instead, which report a refusal. Two other defects turned up while tracing impact — `panels:show` shrinking an already-visible panel, and a failed plugin transition reporting `not-found`. Both are in the state owners #322 reworks, so they are fixed there and written up in that PR's body; nothing about them is in this diff. ## Testing Specs for the panel controls, the config resolver, and each migrated consumer, including a negative check run on every new test — the behaviour was broken to confirm the test goes red, then restored. **1485 tests passing, `tsc --noEmit` clean.** ## Related Third of a three-part stack: #324 → #322 → **this**. Merge in order.
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
2 participants
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.
Why
mmgisAPI.emitwas mitt'semitdirectly. mitt iterates its handler array and calls each one unguarded, so the first listener that throws takes the whole emit down with it — every listener registered after it is silently skipped, and the exception surfaces at the emitter, which has nothing to do with the fault.That is a poor property for a bus whose entire purpose is decoupling. A plugin subscribing to
layer:togglecan currently suppress core's own handling of it.What
emitwalks the handler list itself and runs each subscriber in a try/catch. A handler that throws is reported withconsole.errornaming the event, and the emit continues. Wildcard listeners get the same treatment.The list is copied before iterating, so a handler that subscribes or unsubscribes during the emit cannot corrupt the walk it is part of.
This reaches into
events.all, mitt's internal Map of event name to handler array. That is implementation, not documented API, and the comment says so — a mitt upgrade is a place to re-check this.Scope
One method. No caller changes, no signature change.
Testing
tests/unit/mmgisAPIBus.spec.jscovers a throwing listener not blocking the ones after it, the error being reported, and subscribe/unsubscribe during an emit.1413 tests passing,
tsc --noEmitclean.Related
First of a three-part stack. This one is independent of the rest and can merge on its own — it is a live bug fix. The panel and plugin command work builds on it in #322.