Conversation
A user can turn off a misbehaving plugin and have it stay off across reloads. Disable is a HOST/RUNTIME concern, kept out of #167's frozen structural core: loadFeaturePlugins still returns every activatable descriptor; the render-time consumers filter the disabled ones. - lib/plugins/pluginPrefs.js: localStorage-backed disabled-id set with theme.js-style reactivity (CustomEvent + cross-tab storage listener), node-unit-testable behind typeof guards. Documents the honest limit: disable ejects from activation/render, the module is still imported. - lib/plugins/useDisabledPlugins.js: shared reactive hook, one subscription for both consumers. - PluginOverlayHost: filter launchers/overlays to enabled plugins, close an overlay whose plugin is disabled, and a "⚙ Plugins" manager popover (the only re-enable path) that lists the full loaded set. - PluginErrorBoundary: optional onDisable → "Disable plugin" quick-eject on a render crash. - TakeoffCanvas: filter export slots by the disabled set; the shared action-error notice now carries the offending pluginId so the banner offers a "Disable plugin" eject. - test/pluginPrefs.test.ts: round-trip, isPluginDisabled, malformed-JSON tolerance, persistence, subscribe/unsubscribe.
The overlay-host launcher/manager column sat at left:14 bottom:14 — the exact coords of the canvas's native zoom/fit/dark-mode control column (TakeoffCanvas 5813) — so ✎ Notes / ⚙ Plugins (and the Notes panel + crash notice at left:14 bottom:60) covered the dark-mode toggle and zoom buttons. Shift all plugin UI to left:58, clear of the 34px-wide native column: launcher/manager (host), the crash notice (error boundary), and the reference Notes panel. Pure layout; no behavior change.
Plugins no longer self-position their overlays. All plugin UI now lives in ONE host-positioned column (left:58, clear of the canvas's native zoom/dark-mode controls); the overlay renders as RELATIVE content into a host-sized, scroll-capped panel slot, so a plugin can't land its UI over app controls. The manager and an open overlay share that slot and are MUTUALLY EXCLUSIVE, so they can no longer cover each other (the overlap seen when both were open). - PluginOverlayHost: single flex column; panel slot (overlay|manager) above the launchers; openOverlay/toggleManager enforce mutual exclusion; PANEL_WIDTH is host-owned; overlay wrapped in a width-bounded maxHeight:60vh scroll box. - takeoff-notes reference plugin: drop self-positioning; render relative content. - PluginErrorBoundary: notice is relative content in the host slot, not absolute. - PLUGIN-AUTHORING: document that overlays render relative content; the host owns placement; don't self-position (position:fixed/absolute escapes the safe zone). #167 frozen core untouched; canvas untouched. Gate green (861 tests).
…, honest docs Apply adversarial-review findings (no blockers; all minor): - Browser MINOR: a tall relative overlay grew to maxHeight:60vh (viewport) and could reach the TOP toolbar. Bound both the overlay slot and the manager to the canvas STAGE via maxHeight:calc(100% - 28px) + overflowY:auto — matching the native panels (TakeoffCanvas:5925) — so a tall overlay scrolls within the stage instead of spilling over the top chrome. The zoom/dark-mode column was never at risk (relative content grows rightward, away from it) — confirmed by review. - Static MINOR: the slot's overflow clipped the inner panel's drop-shadow. Put the shadow on the host wrapper (its own box-shadow isn't self-clipped) and drop the now-redundant shadow from the reference panel + crash notice. - Soften the overstated 'can't cover the canvas's own controls' comment + doc: the honest guarantee is 'clear of the zoom/dark-mode column, bounded to the stage'; position:fixed still escapes (documented convention, not a sandbox). Gate green (861 tests). Core + canvas untouched.
…t cap works Re-verification showed the calc(100% - 28px) overlay cap was inert: PluginOverlay Host was mounted at the 100vh root (sibling of the stage), whose auto-height host column gave the percentage nothing definite to bind to — a tall overlay grew unbounded and covered the top toolbar. Move the mount INSIDE the canvas stage (TakeoffCanvas:5340 — flex:1, position:relative, overflow:hidden, below the chrome), beside the native zoom column. Now the stage is the definite-height positioning ancestor: the cap clamps, a tall overlay scrolls within the stage, and overflow:hidden clips any spillover — so plugin UI can't reach the top toolbar. Launchers stay visually put (stage bottom = viewport bottom). Pure relocation of one element; behavior otherwise unchanged. Gate green (861).
Round-2 fix was still inert: the slot's maxHeight:calc(100%) bound to the auto- height host COLUMN (its containing block), not the stage — so it clamped nothing and a tall overlay was clipped (data loss, no scrollbar) by the stage's overflow:hidden. Correct mechanism: put the stage-relative cap on the host COLUMN (a direct child of the definite-height stage, like the native panel at :5925), then flex-distribute inside it — launchers flexShrink:0, the panel flex:0 1 auto + minHeight:0 + overflowY:auto. A tall overlay now shrinks to the space above the launchers and SCROLLS; a short one keeps its natural size; neither reaches the top toolbar. Gate green (861).
Owner
Author
Evidence — disable / eject toggleManager + eject, persists across reload, re-enable Active (⚙ manager open, each plugin has a Disable button): Disabled via the manager — launcher gone, row flips to "Enable": After a full page reload — still gone (persisted to Re-enabled — launcher back live, no reload: Eject a crashing plugin from its own notice The render-crash boundary now carries a "Disable plugin" button: After clicking it — notice and launcher both gone (and it stays gone): Export-only plugins respect disable too Scope Summary present in After disabling it in the manager — gone from the menu: |
Owner
Author
Evidence — Option A: host owns plugin-UI placementOverlay open — relative content in the host slot; native zoom/dark-mode column clear: Manager open — same slot, native column clear: Mutual exclusion — opening the manager closes the overlay (proven never both at once): Crash notice renders relative in-slot (not floating over the canvas): Honest limitation — a Many launchers — column stays on-screen: |
Owner
Author
Evidence — the reported bug fixed + tall-overlay region protectionThe reported bug: plugin UI no longer covers the zoom/dark-mode controls Native With the overlay + manager open, the native column stays clear: Tall-overlay region protection (final, empirically verified) A 2000px overlay clamps to the canvas stage and SCROLLS (rows 43–60 shown) — the top toolbar, launchers, and zoom/dark-mode column are all uncovered. Measured: A short overlay keeps its natural size + drop shadow (not stretched to fill): Manager / disabled / re-enabled all intact under the new layout: |
There was a problem hiding this comment.
Pull request overview
Adds host-side “disable/eject” preferences for plugins (persisted + reactive) and moves plugin UI into a host-owned, stage-bounded placement column (“Option A”) to prevent well-behaved plugins from covering native canvas controls.
Changes:
- Introduces a localStorage-backed disabled-plugin id set with same-tab + cross-tab reactivity, plus a React hook for reactive reads.
- Filters plugin contributions (overlays + report export items) by the disabled set, and adds “Disable plugin” eject affordances (action-error banner + render error boundary) plus a Plugins manager.
- Converts plugin overlay UI to render relative content into a host-positioned, stage-height-capped panel; updates authoring docs accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/test/pluginPrefs.test.ts | Adds node tests for disabled-plugin persistence + reactivity behavior. |
| web/src/pages/TakeoffCanvas.jsx | Filters export plugin items by disabled set; adds eject button on action-error banner; relocates overlay host into stage. |
| web/src/lib/plugins/useDisabledPlugins.js | Adds a React hook to subscribe to disabled-plugin changes. |
| web/src/lib/plugins/pluginPrefs.js | Implements localStorage-backed disabled-plugin set + event/storage reactivity. |
| web/src/features/takeoff-notes/plugin.jsx | Updates overlay UI to be host-placed relative content (no self-positioning). |
| web/src/components/PluginOverlayHost.jsx | Adds host-owned placement column, Plugins manager, disabled filtering, and overlay close-on-disable behavior. |
| web/src/components/PluginErrorBoundary.jsx | Updates error UI to be relative-content and adds “Disable plugin” eject button. |
| docs/plugin-foundation/PLUGIN-AUTHORING.md | Documents host-owned overlay placement conventions for plugin authors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+77
to
+89
| export function onDisabledPluginsChange(fn) { | ||
| if (typeof window === "undefined") return () => {}; | ||
| const onEvt = () => fn(getDisabledPluginIds()); | ||
| const onStorage = (e) => { | ||
| if (e.key === KEY) fn(getDisabledPluginIds()); | ||
| }; | ||
| window.addEventListener(EVT, onEvt); | ||
| window.addEventListener("storage", onStorage); | ||
| return () => { | ||
| window.removeEventListener(EVT, onEvt); | ||
| window.removeEventListener("storage", onStorage); | ||
| }; | ||
| } |
Comment on lines
+64
to
+66
| useEffect(() => { | ||
| if (openKey && disabled.has(openKey.split("::")[0])) setOpenKey(null); | ||
| }, [disabled, openKey]); |
Owner
Author
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
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.
Plugin disable/eject toggle + host-owned UI placement
Stacked on
feat/169-export-slot(the plugin foundation: #167 contract core → #168 overlay host → #169 export slot). Adds a per-user disable/eject toggle and Option A host-owned placement (region protection). Branch-only for now — a reviewable showcase, not queued to merge.What's here
Disable / "eject" a misbehaving plugin
pluginPrefsmodule persists a device-global disabled-id set tolocalStorage(opentakeoff_plugins_disabled), with the app's standardCustomEvent+ cross-tabstoragereactivity — so an ejected plugin stays ejected across reloads (critical if it crashes on mount).⚙ Pluginsmanager lists every loaded plugin with Enable/Disable toggles (always reachable, even if all are disabled).Option A — the host owns plugin-UI placement (region protection)
position:fixedplugin can still escape — documented as convention, not a sandbox.<PluginOverlayHost/>was relocated into the stage so the height cap binds to a definite-height ancestor.Verification
Every claim below was browser-verified via a repeatable Playwright harness (production build, seeded state); the evidence screenshots are posted as comments on this PR. The tall-overlay bound specifically went through three empirical rounds — two plausible-but-inert fixes were caught in-browser before the working one landed.
Gate green on each commit (
npm run typecheck && lint && test && build && axis-a— 861 tests).