Move every panel and plugin caller onto the request bus - #325
Merged
Conversation
sandesh-sp
force-pushed
the
refactor/panel-plugin-command-cutover
branch
from
August 19, 2026 14:14
90eb9ac to
f24efa9
Compare
sandesh-sp
force-pushed
the
refactor/panel-plugin-command-cutover
branch
2 times, most recently
from
August 19, 2026 15:57
b547423 to
8b62c90
Compare
sandesh-sp
force-pushed
the
refactor/panel-plugin-command-cutover
branch
from
August 20, 2026 19:57
a45b708 to
6daf668
Compare
sandesh-sp
force-pushed
the
refactor/panel-plugin-command-cutover
branch
from
August 25, 2026 19:20
6daf668 to
b77156b
Compare
CarsonDavis
approved these changes
Aug 26, 2026
The core:* command events and the direct mmgisAPI panel and plugin methods were a second way to do what the request bus now does, and the weaker one: events are fire-and-forget, so a refusal had nowhere to go, and the direct methods cannot cross a sandbox boundary at all. Both are removed. The panel header and icon-tray controls issue requests and log a refusal rather than clicking dead, and draw themselves from canSetState instead of reading allowedStates and missing the float restriction. Config action strings resolve through one shared resolver, so a mission author reads the same request names the API documents. core: is reserved rather than reused: its verbs have no expressible successor, so a config still carrying core:showPlugin:<id> gets a warning naming the supported actions instead of emitting an event nothing listens for. Chart, AOI and AddTempLayer move to the plugin state requests. AddTempLayer was reaching the removed methods through a window cast, which optional chaining turned into a silent no-op.
panels:changed was documented as firing when a panel gains a tool. It doesn't — only losing one notifies — so a reader following the docs would wait for an event that never comes. Both call sites run before anything subscribes, so the listing is right in practice; the sentence was the only thing wrong. plugins:changed now names the batch broadcast, and both events say plainly that teardown is silent.
A config action carrying no namespace is emitted exactly as written. Every tool publishes under plugin:<toolId>:, so a bare name lands on a channel nothing subscribes to and the button that triggered it does nothing at all — no error, no clue. It is still emitted, since a bare event name is legal, but it is reported with the fully qualified form the author most likely wanted. This is the same reasoning the reserved core namespace already follows: a config mistake is worth surfacing rather than swallowing.
resolveAction requested first and caught afterwards, so a core that never registered the action surfaced as "failed: Error: No handler for…" — the shape of an internal fault rather than of an action this core does not offer. Asking first gives both that case and a wholly absent bus the same plain "no handler", which is what the surrounding comment already claimed happened. Also covers the float-panel header. Deciding from allowedStates rather than canSetState draws a Minimize button on a panel whose position forbids the state it targets, stranding it; the rule was tested at the manager and the provider but never through the control that had the bug.
Teardown was the one layout change that went unannounced, on the reasoning that it destroys the subscribers along with the state they follow. The bus does not work that way: it outlives the layout, so a plugin that subscribed to panels:changed or plugins:changed is still subscribed after the panels and plugins it was following have been dismantled, holding a roster of things that no longer exist. An empty listing is what tells it otherwise, and it agrees with what the providers report from that point on — empty listings from getAll, layout-inactive from every command. PanelManager gains clear(), so the empty listing goes out once instead of the per-panel stream unregisterPanel produced when modern.js looped over the layout. destroyAllTools broadcasts the same way, and both stay silent when there was nothing to tear down.
The resolver named the four request strings a second time and issued them itself, next to a client whose comment claims the bus names live there and nowhere else. Two spellings of a name with nothing holding them together drift into a layout-inactive refusal that logs nothing, and five typed wrappers sat unused beside the duplicate. The action table maps a name to the wrapper that runs it, so each name is written once and the wrappers are the only way across. It still asks whether the handler exists before calling. A wrapper reports a core that lacks the handler as layout-inactive, which reads to a config author as a layout that is not up rather than a core too old to offer the action. The panel header's Maximize button and the icon tray had no coverage: rendering a button and wiring it to the right request look identical until it is clicked, and the tray is where a click means focus or un-focus depending on panel state. The rejected-request arm is covered too, without which a failed command escapes as an unhandled rejection nothing surfaces. Docs describe what the commands actually answer: an unrecognised state name as bad-request rather than state-not-allowed, hasHandler as a question about the core build rather than about what it currently holds, and panels:show as lifting a panel out of collapsed only — which leaves the iconified left panel every stock layout starts with exactly where it is, so revealing it is two steps and the result says which one you are on.
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
force-pushed
the
refactor/panel-plugin-command-cutover
branch
from
August 27, 2026 01:26
b77156b to
39baa7f
Compare
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
With the request surface in place (#322), there were three ways to collapse a panel: a direct
PanelManager_call, anmmgisAPI.hidePanelmethod, and acore:hidePanelevent. 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 directmmgisAPImethods cannot cross a sandbox boundary, sincepostMessagecannot carry a function reference. Both are removed here.What
Removed:
_initCoreCommandDispatcherand the sevencore:*command events;showPanel,hidePanel,togglePanel,showPlugin,hidePlugin,loadPlugin,unloadPlugin,isPluginLoaded,isPluginHiddenfrommmgisAPI;togglePanelCollapsedfromPanelManager_.Panel controls issue requests and log the refusal reason rather than clicking dead, and decide whether to draw themselves from
canSetStateinstead of readingallowedStatesdirectly — 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 (togglePanelis gone;unloadPluginneeds a state argument a colon-delimited string cannot carry), so a config still carryingcore:showPlugin:LayersToolgets a warning naming the supported actions instead of emitting an event nothing listens for.Consumers migrated: Chart (
plugins:showon analysis-ready,plugins:setStateon close), AOI (unload on close), Title (through the resolver), AddTempLayer.Teardown announces itself.
panels:changedandplugins:changedwere 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_gainsclear()so that goes out once rather than once per panel.Regression fixed
AddTempLayer was calling
window.mmgisAPI.showPlugin/hidePluginthrough 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:showshrinking an already-visible panel, and a failed plugin transition reportingnot-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 --noEmitclean.Related
Third of a three-part stack: #324 → #322 → this. Merge in order.