mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(plugins): keep plugin side panel from rendering empty (#8394)
The plugin iframe (srcdoc, v18.10.0) drove its visible UI through tokenless PLUGIN_MESSAGE round-trips that the host accepted only when `event.source === this.iframeRef.contentWindow`. Under zoneless change detection the @ViewChild `iframeRef` is assigned on a later rAF/timeout CD pass, which can land after the iframe has already posted its first messages. Those messages were then dropped forever (postMessage is not redelivered) and useTranslate has no timeout, so the panel stayed blank — intermittently, depending on how the timing landed. - Resolve the iframe window from the live DOM (host-scoped querySelector) instead of the cached @ViewChild. The iframe must be in the DOM to have posted a message, so this always finds it at message time, while staying scoped to this component so sibling plugin iframes are not answered. - Drop the redundant _cleanupIframeCommunication() on fresh mount: it set an empty-document srcdoc that forced an extra iframe load + CD pass before the real document, widening the race. - Add PluginIndexComponent spec covering the dropped-message regression and cross-iframe isolation.
This commit is contained in:
Johannes Millan
parent
80bdd08177
commit
4699499dc3
2 changed files with 173 additions and 3 deletions
142
src/app/plugins/ui/plugin-index/plugin-index.component.spec.ts
Normal file
142
src/app/plugins/ui/plugin-index/plugin-index.component.spec.ts
Normal file
|
|
@ -0,0 +1,142 @@
|
|||
import { ComponentFixture, TestBed } from '@angular/core/testing';
|
||||
import { NoopAnimationsModule } from '@angular/platform-browser/animations';
|
||||
import { TranslateModule } from '@ngx-translate/core';
|
||||
import { ActivatedRoute, Router } from '@angular/router';
|
||||
import { EMPTY } from 'rxjs';
|
||||
import { signal } from '@angular/core';
|
||||
import { PluginIframeMessageType } from '@super-productivity/plugin-api';
|
||||
import { PluginIndexComponent } from './plugin-index.component';
|
||||
import { PluginService } from '../../plugin.service';
|
||||
import { PluginBridgeService } from '../../plugin-bridge.service';
|
||||
import { PluginCleanupService } from '../../plugin-cleanup.service';
|
||||
import { LayoutService } from '../../../core-ui/layout/layout.service';
|
||||
|
||||
const PLUGIN_ID = 'test-plugin';
|
||||
|
||||
describe('PluginIndexComponent', () => {
|
||||
let fixture: ComponentFixture<PluginIndexComponent>;
|
||||
let component: PluginIndexComponent;
|
||||
let pluginService: jasmine.SpyObj<PluginService>;
|
||||
let pluginBridge: jasmine.SpyObj<PluginBridgeService>;
|
||||
|
||||
const setup = async (): Promise<void> => {
|
||||
pluginService = jasmine.createSpyObj<PluginService>('PluginService', [
|
||||
'isInitialized',
|
||||
'initializePlugins',
|
||||
'getPluginIndexHtml',
|
||||
'getAllPlugins',
|
||||
'getBaseCfg',
|
||||
'getPluginIframeGeneration',
|
||||
]);
|
||||
pluginService.isInitialized.and.returnValue(true);
|
||||
pluginService.getPluginIndexHtml.and.returnValue(
|
||||
'<!doctype html><html><head></head><body><div id="root"></div></body></html>',
|
||||
);
|
||||
pluginService.getAllPlugins.and.resolveTo([
|
||||
{ manifest: { id: PLUGIN_ID, iFrame: true }, error: null } as never,
|
||||
]);
|
||||
pluginService.getBaseCfg.and.resolveTo({} as never);
|
||||
pluginService.getPluginIframeGeneration.and.returnValue(0);
|
||||
|
||||
pluginBridge = jasmine.createSpyObj<PluginBridgeService>('PluginBridgeService', [
|
||||
'createBoundMethods',
|
||||
'sendMessageToPlugin',
|
||||
]);
|
||||
pluginBridge.createBoundMethods.and.returnValue({} as never);
|
||||
pluginBridge.sendMessageToPlugin.and.resolveTo('translated');
|
||||
|
||||
const cleanupService = jasmine.createSpyObj<PluginCleanupService>(
|
||||
'PluginCleanupService',
|
||||
['registerIframe', 'cleanupPlugin'],
|
||||
);
|
||||
|
||||
await TestBed.configureTestingModule({
|
||||
imports: [PluginIndexComponent, NoopAnimationsModule, TranslateModule.forRoot()],
|
||||
providers: [
|
||||
{ provide: PluginService, useValue: pluginService },
|
||||
{ provide: PluginBridgeService, useValue: pluginBridge },
|
||||
{ provide: PluginCleanupService, useValue: cleanupService },
|
||||
{ provide: LayoutService, useValue: { isPanelResizing: signal(false) } },
|
||||
{ provide: ActivatedRoute, useValue: { paramMap: EMPTY } },
|
||||
{ provide: Router, useValue: jasmine.createSpyObj('Router', ['navigate']) },
|
||||
],
|
||||
}).compileComponents();
|
||||
|
||||
fixture = TestBed.createComponent(PluginIndexComponent);
|
||||
component = fixture.componentInstance;
|
||||
// The side-panel container drives the component via this input.
|
||||
fixture.componentRef.setInput('directPluginId', PLUGIN_ID);
|
||||
fixture.componentRef.setInput('showFullUI', false);
|
||||
fixture.componentRef.setInput('useSidePanelConfig', true);
|
||||
// contentWindow only exists for a connected iframe.
|
||||
document.body.appendChild(fixture.nativeElement);
|
||||
|
||||
await component.ngOnInit();
|
||||
fixture.detectChanges();
|
||||
};
|
||||
|
||||
afterEach(() => {
|
||||
fixture?.destroy();
|
||||
fixture?.nativeElement?.remove();
|
||||
});
|
||||
|
||||
const getIframe = (): HTMLIFrameElement =>
|
||||
fixture.nativeElement.querySelector('iframe[data-plugin-iframe]');
|
||||
|
||||
it('renders the plugin iframe after loading', async () => {
|
||||
await setup();
|
||||
expect(getIframe()).toBeTruthy();
|
||||
expect(component.isLoading()).toBe(false);
|
||||
});
|
||||
|
||||
// Regression for #8394: with zoneless change detection the @ViewChild can be
|
||||
// assigned only AFTER the srcdoc iframe posts its first (tokenless)
|
||||
// PLUGIN_MESSAGE. The host must still attribute that message to the iframe via
|
||||
// the live DOM, or the message is dropped forever and the panel stays blank.
|
||||
it('handles a plugin message even when the @ViewChild iframeRef is not set', async () => {
|
||||
await setup();
|
||||
const iframe = getIframe();
|
||||
expect(iframe.contentWindow).toBeTruthy();
|
||||
|
||||
// Stand in for the race: under zoneless CD the @ViewChild can still be
|
||||
// unassigned when the iframe posts its first message. Clearing it proves
|
||||
// message handling no longer depends on iframeRef (the old code read it
|
||||
// here and dropped the message); attribution comes from the live DOM.
|
||||
(component as unknown as { iframeRef?: unknown }).iframeRef = undefined;
|
||||
|
||||
window.dispatchEvent(
|
||||
new MessageEvent('message', {
|
||||
source: iframe.contentWindow,
|
||||
data: {
|
||||
type: PluginIframeMessageType.MESSAGE,
|
||||
messageId: 'm1',
|
||||
message: { type: 'translate', payload: { key: 'HOME.TITLE' } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
expect(pluginBridge.sendMessageToPlugin).toHaveBeenCalledWith(PLUGIN_ID, {
|
||||
type: 'translate',
|
||||
payload: { key: 'HOME.TITLE' },
|
||||
});
|
||||
});
|
||||
|
||||
it('ignores messages whose source is not this plugin iframe', async () => {
|
||||
await setup();
|
||||
|
||||
// A message from a foreign window (e.g. another plugin's iframe) must not be
|
||||
// answered with this plugin's bridge — isolation is preserved.
|
||||
window.dispatchEvent(
|
||||
new MessageEvent('message', {
|
||||
source: window,
|
||||
data: {
|
||||
type: PluginIframeMessageType.MESSAGE,
|
||||
messageId: 'm2',
|
||||
message: { type: 'translate', payload: { key: 'HOME.TITLE' } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
expect(pluginBridge.sendMessageToPlugin).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
@ -80,6 +80,7 @@ export class PluginIndexComponent implements OnInit, OnDestroy {
|
|||
|
||||
private readonly _route = inject(ActivatedRoute);
|
||||
private readonly _router = inject(Router);
|
||||
private readonly _elRef = inject<ElementRef<HTMLElement>>(ElementRef);
|
||||
private readonly _sanitizer = inject(DomSanitizer);
|
||||
private readonly _pluginService = inject(PluginService);
|
||||
private readonly _pluginBridge = inject(PluginBridgeService);
|
||||
|
|
@ -102,7 +103,10 @@ export class PluginIndexComponent implements OnInit, OnDestroy {
|
|||
// If directPluginId is provided, load that plugin directly
|
||||
if (this.directPluginId) {
|
||||
this.pluginId.set(this.directPluginId);
|
||||
this._cleanupIframeCommunication();
|
||||
// NOTE: no _cleanupIframeCommunication() here. On a fresh mount there is
|
||||
// nothing to tear down, and the empty-document srcdoc it would set just
|
||||
// forces an extra iframe load + change-detection pass before the real
|
||||
// document — widening the load-timing race that blanked the panel (#8394).
|
||||
await this._waitForPluginSystem();
|
||||
|
||||
try {
|
||||
|
|
@ -241,7 +245,7 @@ export class PluginIndexComponent implements OnInit, OnDestroy {
|
|||
// other embed slots) don't get answered with this plugin's bound methods.
|
||||
this._messageListener = async (event: Event) => {
|
||||
const msgEvent = event as MessageEvent;
|
||||
const iframeWin = this.iframeRef?.nativeElement?.contentWindow;
|
||||
const iframeWin = this._getPluginIframeWindow();
|
||||
if (!iframeWin || msgEvent.source !== iframeWin) {
|
||||
return;
|
||||
}
|
||||
|
|
@ -274,6 +278,27 @@ export class PluginIndexComponent implements OnInit, OnDestroy {
|
|||
return Array.from(bytes, (byte) => byte.toString(16).padStart(2, '0')).join('');
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the live window of THIS component's plugin iframe.
|
||||
*
|
||||
* Reads the rendered DOM rather than the cached `@ViewChild`: with zoneless
|
||||
* change detection the ViewChild property is only assigned on the next CD
|
||||
* pass (rAF/timeout), which can land AFTER the srcdoc iframe has executed its
|
||||
* scripts and posted its first (tokenless) PLUGIN_MESSAGE. Because a message
|
||||
* can only exist once its iframe is in the DOM, querying this component's
|
||||
* host subtree at message time always finds our iframe — so we no longer
|
||||
* silently drop the plugin's own messages and leave the panel blank (#8394).
|
||||
*
|
||||
* Still scoped to this component's host, so messages from sibling plugin
|
||||
* iframes (side panel vs. embed slot) are not answered with our bound methods.
|
||||
*/
|
||||
private _getPluginIframeWindow(): Window | null {
|
||||
const iframeEl = this._elRef.nativeElement.querySelector<HTMLIFrameElement>(
|
||||
'iframe[data-plugin-iframe]',
|
||||
);
|
||||
return iframeEl?.contentWindow ?? null;
|
||||
}
|
||||
|
||||
private _cleanupIframeCommunication(): void {
|
||||
const currentPluginId = this.pluginId();
|
||||
PluginLog.log(`Cleaning up iframe communication for plugin: ${currentPluginId}`);
|
||||
|
|
@ -303,7 +328,10 @@ export class PluginIndexComponent implements OnInit, OnDestroy {
|
|||
onIframeLoad(): void {
|
||||
PluginLog.log('Plugin iframe loaded for plugin:', this.pluginId());
|
||||
|
||||
// Register iframe with cleanup service
|
||||
// Reading the cached @ViewChild is fine HERE (unlike the message listener,
|
||||
// which uses _getPluginIframeWindow()): this fires on the iframe's `load`
|
||||
// event, by which point the ViewChild is assigned, and registration is not
|
||||
// race-sensitive. Do not "unify" this with the listener's DOM lookup.
|
||||
if (this.iframeRef?.nativeElement && this.pluginId()) {
|
||||
this._cleanupService.registerIframe(this.pluginId(), this.iframeRef.nativeElement);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue