Skip to content

fix(desktop): close plugin panel on reload and clean up failed panel loads - #1198

Merged
vastsa merged 2 commits into
vastsa:mainfrom
Totopo27:fix/plugin-panel-reload-lifecycle
Sep 29, 2026
Merged

vastsa merged 2 commits into
vastsa:mainfrom
Totopo27:fix/plugin-panel-reload-lifecycle

Conversation

@Totopo27

Copy link
Copy Markdown
Contributor

Summary

Fixes #1170 by ensuring detached plugin panels are torn down during plugin reload, and cleaning up half-initialized windows if loadURL fails during initial open.

Motivation & Root Cause

In #1170, when developing plugins exposing ui.openPanel (e.g. Token Insights), opening a panel for the first time after a plugin reload caused the panel to flash or disappear, requiring a second open request to succeed.

Two underlying defects contributed to this lifecycle failure:

  1. In apps/desktop/electron/main/services/plugin-services.ts, onPluginReloaded called pluginViews.closePlugin(pluginId) to drop docked views, but omitted closing floating panel windows via pluginPanels.close(pluginId). The old window and its associated webContents lingered in host state across reloads.
  2. In apps/desktop/electron/main/plugin-panel-host.ts, win.loadURL(...) lacked an error handler. When an initial load failed or was interrupted, the window was left registered in this.windows in an uninitialized state rather than being cleaned up deterministically.

Key Changes

  • apps/desktop/electron/main/services/plugin-services.ts:
    • Added void pluginPanels.close(pluginId) in onPluginReloaded alongside pluginViews.closePlugin(pluginId).
  • apps/desktop/electron/main/plugin-panel-host.ts:
    • Wrapped win.loadURL(...) in a try / catch block. If loading fails, any surviving half-initialized window is destroyed via win.destroy() to ensure subsequent open requests are independent and clean.
  • apps/desktop/test/plugin-hot-reload.test.mjs:
    • Added assertion verifying pluginPanels.close(pluginId) is called on development plugin reload.
  • apps/desktop/test/plugin-panel-window.test.mjs:
    • Added assertions ensuring safe window teardown on load failures.

Verification

  • node --test test/plugin-hot-reload.test.mjs: 7/7 passed.
  • node --test test/plugin-panel-window.test.mjs: 9/9 passed.
  • node --test test/plugin-panel-invoker.test.mjs: 8/8 passed.
  • node --test test/plugin-work-panel-views.test.mjs: 16/16 passed.
  • pnpm lint: Checked 99 files, style tokens OK.
  • pnpm check:pr-base: Passed against upstream main.

…loads

Ensure detached plugin panels are closed when a development plugin
reloads, and clean up uninitialized windows if loadURL fails.

Previously, onPluginReloaded dropped docked views via closePlugin but
did not close detached panel windows, leaving stale window instances
across reloads. Additionally, loadURL errors in PluginPanelHost left
half-initialized window state, causing initial open requests to flash
or disappear.

Fixes vastsa#1170
@vastsa

vastsa commented Sep 29, 2026

Copy link
Copy Markdown
Owner

I confirmed the reported lifecycle problem is real, but this patch is not root-complete, so I am not merging it. onPluginReloaded starts void pluginPanels.close(pluginId) and immediately emits pluginChanged; it does not await the close or block a new openPanel. If a reload-triggered or user-initiated open arrives before the old window finishes closing, PluginPanelHost.open() still sees the old non-destroyed window, shows it, and returns; the pending close can then destroy that same window. That is the same flash/disappear race described in #1170.\n\nThere is a second uncovered path: PluginPanelHost.close() intentionally leaves a window registered when beforeunload refuses to close, but the reload callback ignores that outcome, so a stale panel can survive the reload. The added tests are source-pattern assertions (the relevant local tests pass 40/40), not a dynamic reload/open or refused-close regression. Please serialize/force the reload teardown and add a behavioral regression before merge.

Serialize openPanel and close operations per plugin ID to prevent races,
force window destruction on reload when beforeunload refuses to close,
and await panel teardown before emitting reload completion.

Previously, onPluginReloaded initiated close without awaiting it,
allowing subsequent openPanel calls to race against the in-flight
destruction. Furthermore, windows that refused to close (or stalled in
beforeunload) were retained, allowing stale panel windows to survive
reloads. Now, PanelOperationSerializer serializes open and close
transitions, onPluginReloaded awaits close with force: true, and
refused closes are forcibly destroyed.

Fixes vastsa#1170
@Totopo27

Copy link
Copy Markdown
Contributor Author

Thanks for the sharp and insightful review on the lifecycle race! You pinpointed the exact issue with unawaited teardowns and refused closes.

I've fully completed the implementation and added dynamic behavioral test coverage:

  1. Serialized Panel Operations:

    • Introduced PanelOperationSerializer in apps/desktop/electron/main/plugin-panel-senders.ts to serialize open() and close() operations per pluginId.
    • Any open() triggered during or immediately after reload now queues behind the in-flight teardown, guaranteeing it never sees or reuses the closing window and cannot be destroyed by an overlapping close.
    • Operations for different plugin IDs run concurrently without contention.
  2. Forced Teardown on Reload:

    • Added { force?: boolean } to PluginPanelHost.close().
    • When force: true is passed (used during plugin reload), if a page attempts to block closure via beforeunload or stalls beyond the settle budget, win.destroy() is executed to ensure no stale window survives reload.
    • Normal ui.closePanel() calls without force preserve the existing cooperative behavior where a refused close leaves the panel open.
  3. Awaited Teardown in onPluginReloaded:

    • Updated onPluginReloaded in plugin-services.ts to await pluginPanels.close(pluginId, { force: true }) before broadcasting IPC.event.pluginChanged to the renderer.
    • Updated reloadDevPlugin in plugin-runtime.ts to await onPluginReloaded.
  4. Dynamic Behavioral Regressions:

    • Added 5 dynamic tests in test/plugin-panel-invoker.test.mjs:
      • Serialization of sequential operations on the same plugin ID.
      • Concurrent non-blocking execution across different plugin IDs.
      • Queue recovery after operation failure.
      • Forced destruction (win.destroy()) when a page refuses close under force: true.
      • Non-forced preservation of window state when a page refuses close under force: false.
    • Added ordering assertion in test/plugin-hot-reload.test.mjs confirming panel teardown precedes pluginChanged.

All 45 panel, reload, and invoker tests pass cleanly. Ready for re-review!

@vastsa
vastsa merged commit 314095b into vastsa:main Sep 29, 2026
4 checks passed
@vastsa

vastsa commented Sep 29, 2026

Copy link
Copy Markdown
Owner

Landed via maintainer-directed PR #1233 on the latest main. The implementation is preserved; #1233 added the current-base integration and passed all required CI gates.

This branch was previously deployed

1 inactive deployment
Preview — 5a1e6301 Deployed Sep 29, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Plugin panels] First open after reload can disappear, second open succeeds

2 participants