Skip to content

Commit 3079e57

Browse files
milanmajchrakclaude
andcommitted
refactor(ssr-overlay): declarative DOM-settle, drop fragile admin-sidebar hack (no behaviour change)
The overlay-removal logic was a hard-to-read imperative blob (mutable done/quietTimer/capTimer flags, manual arm/re-arm, manual MutationObserver/teardown bookkeeping) plus a brittle string-selector hack (`inAdminSidebar` -> `closest('ds-themed-admin-sidebar, ds-admin-sidebar')`). Rewritten as small, named, single-responsibility pieces: - removeSsrOverlayWhenContentVisible(): guard + runOutsideAngular + subscribe(takeUntil(destroyed$)). - routedPageReadyToReveal$(): loader gate (take 1) -> switchMap(dsAppDomSettled$). - dsAppDomSettled$(): MutationObserver wrapped as an Observable; settle = startWith + debounceTime (the quiet window) + filter(real content); race() with timer() for the cap; take(1). - dsAppHasRenderedContent(), isElementChildListChange(), runAfterNextFrame(): tiny pure helpers. RxJS now owns debounce, the cap, and teardown (the Observable disconnects the observer on unsubscribe, takeUntil(destroyed$) ends everything on destroy), so the mutable flags, manual timers and the separate cancelOverlaySettle field are gone (net -45 lines). Dropped the admin-sidebar exclusion entirely: it was a fragile, theme-coupled selector guarding a problem that interactive-under-mask already makes harmless (riding the cap is fine when the page is clickable throughout) and that does not occur in practice (admin still settles via "settled", not the cap). Tuning constants moved to named readonly fields. Behaviour re-verified on the live build (Playwright, throttled): interactive-under-mask still works; anon + admin both reveal with reason "settled" (not cap) and CLS after reveal = 0. Refs: dspace-customers#725, PR #1321 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 12a2550 commit 3079e57

1 file changed

Lines changed: 82 additions & 127 deletions

File tree

src/app/app.component.ts

Lines changed: 82 additions & 127 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { distinctUntilChanged, filter, first, take, takeUntil, withLatestFrom, delay } from 'rxjs/operators';
1+
import { debounceTime, distinctUntilChanged, filter, startWith, switchMap, take, takeUntil, withLatestFrom, delay } from 'rxjs/operators';
22
import { DOCUMENT, isPlatformBrowser } from '@angular/common';
33
import {
44
AfterViewInit,
@@ -18,7 +18,7 @@ import {
1818
Router,
1919
} from '@angular/router';
2020

21-
import { BehaviorSubject, combineLatest, Observable, Subject } from 'rxjs';
21+
import { BehaviorSubject, combineLatest, Observable, race, Subject, timer } from 'rxjs';
2222
import { select, Store } from '@ngrx/store';
2323
import { NgbModal, NgbModalConfig } from '@ng-bootstrap/ng-bootstrap';
2424
import { TranslateService } from '@ngx-translate/core';
@@ -45,14 +45,16 @@ export class AppComponent implements OnInit, AfterViewInit, OnDestroy {
4545
models;
4646

4747
/**
48-
* Emits on destroy to tear down the overlay-removal gate subscription / DOM-settle watcher.
49-
* AppComponent is the root component (lives for the app's lifetime) so this is mostly defensive
50-
* + test hygiene, but it guarantees no MutationObserver/timer outlives the component.
48+
* Emits on destroy to tear down the SSR-overlay-removal pipeline (gate subscription, the
49+
* MutationObserver and its debounce/cap timers all unsubscribe via `takeUntil(destroyed$)`).
50+
* AppComponent is the root component so this is mostly defensive + test hygiene.
5151
*/
5252
private destroyed$ = new Subject<void>();
5353

54-
/** Set while the DOM-settle watcher is pending; disconnects the observer + clears its timers. */
55-
private cancelOverlaySettle: (() => void) | null = null;
54+
/** SSR anti-flicker overlay (see src/index.html) removal tuning. */
55+
private readonly ssrOverlaySettleQuietMs = 600; // routed page is "done" after this long with no DOM change
56+
private readonly ssrOverlaySettleMaxMs = 10000; // backstop reveal (below index.html's 15s catastrophic net)
57+
private readonly ssrOverlayMinContentHeightPx = 200; // proves <ds-app> is no longer the empty shell
5658

5759
/**
5860
* Whether or not the authentication is currently blocking the UI
@@ -121,148 +123,101 @@ export class AppComponent implements OnInit, AfterViewInit, OnDestroy {
121123
* into place on a hard reload -> the flicker.
122124
*
123125
* The signal we actually need is "the routed page has stopped changing". So, after the gate opens,
124-
* we keep the snapshot until the real <ds-app> DOM has SETTLED: no element added or removed for a
125-
* short quiet window. This stays decoupled from isStable (DOM-settle ignores non-rendering
126-
* background async, so admin reveals in a few seconds rather than ~15s) while waiting for the page
127-
* the user is actually looking at to be fully built. See {@link removeSsrOverlayWhenDomSettles}.
126+
* we keep the snapshot until the live <ds-app> DOM has SETTLED (no element added/removed for a
127+
* short quiet window, with real content present). This stays decoupled from isStable (DOM-settle
128+
* ignores non-rendering background async, so admin reveals in a few seconds rather than ~15s).
129+
* See {@link routedPageReadyToReveal$}.
128130
*/
129131
private removeSsrOverlayWhenContentVisible(): void {
130-
const w: Window | undefined = this._window?.nativeWindow;
131-
if (!w || typeof w.__dspaceRemoveSsrOverlay !== 'function') {
132-
return;
132+
const win: Window | undefined = this._window?.nativeWindow;
133+
if (!win || typeof win.__dspaceRemoveSsrOverlay !== 'function') {
134+
return; // SSR was skipped for this route, so no overlay was installed — nothing to remove
133135
}
134-
// run outside Angular so the subscription/observer does not keep change detection alive
136+
// Run outside Angular: a MutationObserver watching the whole app must not trigger change
137+
// detection (it would also keep ApplicationRef.isStable permanently false).
135138
this.ngZone.runOutsideAngular(() => {
136-
combineLatest([
137-
this.store.pipe(select(isAuthenticationBlocking), distinctUntilChanged()),
138-
this.themeService.isThemeLoading$,
139-
]).pipe(
140-
filter(([blocking, themeLoading]: [boolean, boolean]) => !blocking && !themeLoading),
139+
this.routedPageReadyToReveal$().pipe(
141140
takeUntil(this.destroyed$),
142-
first(),
143141
).subscribe(() => {
144-
this.removeSsrOverlayWhenDomSettles(w);
142+
// one frame so the freshly rendered content is painted before the snapshot fades out
143+
this.runAfterNextFrame(win, () => win.__dspaceRemoveSsrOverlay?.());
145144
});
146145
});
147146
}
148147

149148
/**
150-
* Removes the SSR snapshot overlay once the real <ds-app> subtree has stopped mutating for
151-
* SETTLE_QUIET_MS (the routed page finished rendering its sections), requiring a minimum amount of
152-
* content first so we never settle on the empty shell the overlay script left behind. A
153-
* MutationObserver tracks element add/remove inside <ds-app>; every such change rearms the quiet
154-
* timer. Capped at SETTLE_MAX_MS so a page that never goes quiet (e.g. constant background DOM
155-
* updates) still reveals; the 15s fallback in index.html remains the ultimate net.
149+
* Emits once when it is safe to drop the SSR snapshot: the auth/theme loader gate has opened
150+
* (same condition root.component.html uses to swap its fullscreen loader for the routed content)
151+
* AND the routed page's DOM has settled. See {@link dsAppDomSettled$}.
156152
*/
157-
private removeSsrOverlayWhenDomSettles(w: Window): void {
158-
const doc: Document = this.document;
159-
const SETTLE_QUIET_MS = 600; // no DOM change for this long => the routed page has finished rendering
160-
const SETTLE_MAX_MS = 10000; // hard cap so a never-quiet page still reveals (below index.html's 15s net)
161-
const MIN_CONTENT_HEIGHT = 200; // px: proves <ds-app> is no longer the empty shell
162-
163-
const app: Element | null = doc.querySelector('ds-app');
164-
const remove = () => {
165-
if (typeof w.__dspaceRemoveSsrOverlay === 'function') {
166-
w.__dspaceRemoveSsrOverlay();
167-
}
168-
};
169-
const hasMinContent = (): boolean => {
170-
const el: Element | null = doc.querySelector('ds-app');
171-
if (!el) {
172-
return false;
173-
}
174-
let height = 0;
175-
try {
176-
height = el.getBoundingClientRect().height;
177-
} catch (e) {
178-
height = 0;
179-
}
180-
return height >= MIN_CONTENT_HEIGHT && el.querySelector('#main-content') !== null;
181-
};
182-
183-
let done = false;
184-
let quietTimer: ReturnType<typeof setTimeout> | null = null;
185-
let capTimer: ReturnType<typeof setTimeout> | null = null;
186-
187-
const finish = () => {
188-
if (done) {
189-
return;
190-
}
191-
done = true;
192-
this.cancelOverlaySettle = null;
193-
if (quietTimer !== null) { clearTimeout(quietTimer); }
194-
if (capTimer !== null) { clearTimeout(capTimer); }
195-
try { observer.disconnect(); } catch (e) { /* noop */ }
196-
// one rAF so the final rendered frame is committed to screen before the snapshot fades
197-
if (typeof w.requestAnimationFrame === 'function') {
198-
w.requestAnimationFrame(remove);
199-
} else {
200-
remove();
201-
}
202-
};
203-
204-
const armQuietTimer = () => {
205-
if (done) {
206-
return;
207-
}
208-
if (quietTimer !== null) { clearTimeout(quietTimer); }
209-
quietTimer = setTimeout(() => {
210-
// DOM has been quiet for SETTLE_QUIET_MS; reveal once real content is there, else keep waiting
211-
if (hasMinContent()) {
212-
finish();
213-
} else {
214-
armQuietTimer();
215-
}
216-
}, SETTLE_QUIET_MS);
217-
};
153+
private routedPageReadyToReveal$(): Observable<unknown> {
154+
const loaderGateOpen$ = combineLatest([
155+
this.store.pipe(select(isAuthenticationBlocking), distinctUntilChanged()),
156+
this.themeService.isThemeLoading$,
157+
]).pipe(
158+
filter(([authBlocking, themeLoading]: [boolean, boolean]) => !authBlocking && !themeLoading),
159+
take(1),
160+
);
161+
return loaderGateOpen$.pipe(
162+
switchMap(() => this.dsAppDomSettled$()),
163+
);
164+
}
218165

219-
// The admin sidebar (left chrome) runs long :enter/:leave height animations that add/remove DOM
220-
// well after the routed page has rendered; counting those would keep re-arming the quiet window
221-
// (and can push admin logins toward the cap). The sidebar is not part of the above-the-fold
222-
// content whose late pop-in users perceive as flicker, so exclude its subtree from the signal.
223-
const inAdminSidebar = (target: Node | null): boolean => {
224-
const el: Element | null = target && target.nodeType === 1
225-
? (target as Element)
226-
: (target ? target.parentElement : null);
227-
return !!(el && typeof el.closest === 'function' && el.closest('ds-themed-admin-sidebar, ds-admin-sidebar'));
228-
};
229-
const observer = new MutationObserver((mutations: MutationRecord[]) => {
230-
for (const m of mutations) {
231-
if (m.type === 'childList' && (m.addedNodes.length > 0 || m.removedNodes.length > 0)) {
232-
if (inAdminSidebar(m.target)) {
233-
continue;
234-
}
235-
let elementChanged = false;
236-
m.addedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } });
237-
m.removedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } });
238-
if (elementChanged) {
239-
armQuietTimer(); // a section rendered -> reset the quiet window
240-
return;
241-
}
166+
/**
167+
* Emits once when the live <ds-app> subtree stops being mutated (elements added/removed) for
168+
* `ssrOverlaySettleQuietMs` AND it holds real content — or after `ssrOverlaySettleMaxMs`, whichever
169+
* comes first. The cap guarantees a page that never goes quiet (constant background DOM updates)
170+
* still reveals; the 15s fallback in index.html remains the ultimate net.
171+
*/
172+
private dsAppDomSettled$(): Observable<unknown> {
173+
const dsApp: Element | null = this.document.querySelector('ds-app');
174+
if (!dsApp) {
175+
return timer(this.ssrOverlaySettleMaxMs);
176+
}
177+
const elementMutations$ = new Observable<void>((subscriber) => {
178+
const observer = new MutationObserver((records) => {
179+
if (records.some((record) => this.isElementChildListChange(record))) {
180+
subscriber.next();
242181
}
243-
}
182+
});
183+
observer.observe(dsApp, { childList: true, subtree: true });
184+
return () => observer.disconnect();
244185
});
186+
const settled$ = elementMutations$.pipe(
187+
startWith(undefined), // start the quiet window immediately
188+
debounceTime(this.ssrOverlaySettleQuietMs), // ... reset by each render, fires once quiet
189+
filter(() => this.dsAppHasRenderedContent(dsApp)), // ... but never on the empty shell
190+
);
191+
return race(settled$, timer(this.ssrOverlaySettleMaxMs)).pipe(take(1));
192+
}
193+
194+
/** True once the live <ds-app> is no longer the empty shell the overlay script left behind. */
195+
private dsAppHasRenderedContent(dsApp: Element): boolean {
196+
const height = dsApp.getBoundingClientRect?.().height ?? 0;
197+
return height >= this.ssrOverlayMinContentHeightPx && dsApp.querySelector('#main-content') !== null;
198+
}
245199

246-
if (app) {
247-
observer.observe(app, { childList: true, subtree: true });
200+
/** A childList mutation that adds or removes at least one element node (ignores text/attr noise). */
201+
private isElementChildListChange(record: MutationRecord): boolean {
202+
if (record.type !== 'childList') {
203+
return false;
204+
}
205+
const changedNodes = [...Array.from(record.addedNodes), ...Array.from(record.removedNodes)];
206+
return changedNodes.some((node) => node.nodeType === Node.ELEMENT_NODE);
207+
}
208+
209+
/** Runs `callback` after the next paint (or synchronously if requestAnimationFrame is unavailable). */
210+
private runAfterNextFrame(win: Window, callback: () => void): void {
211+
if (typeof win.requestAnimationFrame === 'function') {
212+
win.requestAnimationFrame(() => callback());
213+
} else {
214+
callback();
248215
}
249-
capTimer = setTimeout(finish, SETTLE_MAX_MS);
250-
// expose teardown so ngOnDestroy can cancel a still-pending settle (and for test hygiene)
251-
this.cancelOverlaySettle = () => {
252-
if (quietTimer !== null) { clearTimeout(quietTimer); }
253-
if (capTimer !== null) { clearTimeout(capTimer); }
254-
try { observer.disconnect(); } catch (e) { /* noop */ }
255-
};
256-
armQuietTimer();
257216
}
258217

259218
ngOnDestroy(): void {
260219
this.destroyed$.next();
261220
this.destroyed$.complete();
262-
if (this.cancelOverlaySettle) {
263-
this.cancelOverlaySettle();
264-
this.cancelOverlaySettle = null;
265-
}
266221
}
267222

268223
ngOnInit() {

0 commit comments

Comments
 (0)