Skip to content

Commit 468a58a

Browse files
VSB-TUO/Fix home-page flicker AND close #725: interactive snapshot removed only after the routed DOM settles (#1321)
* fix(ssr-overlay): remove overlay only after the routed page DOM settles (real home-page flicker) Follow-up to #1318. On a real instance (VSB / dev-6.pc), an incognito Ctrl+Shift+R still flickered: the deployed code dropped the SSR snapshot ~3.2s in, while the home page was only half-rendered (search box, community list and several navbar items not yet present, content showing "Recent Submissions") and then everything popped into place ~600ms later. Captured frame-by-frame against the live instance. Root cause: the home page renders piecewise (each section fetches its own data), so the previous "<ds-app> has #main-content and height>=200" heuristic was satisfied while the page was still building -> snapshot removed too early -> visible flicker. Fix: after the auth/theme gate opens, keep the snapshot until the real <ds-app> subtree has SETTLED -- no element added/removed for SETTLE_QUIET_MS (600ms) -- via a MutationObserver, requiring minimum content first and capped at SETTLE_MAX_MS (10s). This stays decoupled from ApplicationRef.isStable (DOM-settle ignores non-rendering background async), so it does not bring back the post-login non-interactive page (#725): admin reveals in ~5s, not ~15s. Validated against the live dev-6.pc instance by intercepting the overlay-removal and driving it with this condition: - anon reload : drops @ ~4.8s, page COMPLETE (search + community list + full navbar) - admin reload: drops @ ~5.0s (reason "settled", not the cap), page complete vs the deployed code dropping @ ~3.2-3.6s on a half-built page. Refs: dspace-customers#725, PR #1288, PR #1317, PR #1318 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(ssr-overlay): make the masked page interactive + skip overlay under WebDriver (close #725) Addresses an expert review of the DOM-settle change: DOM-settle alone shortened but did not eliminate issue #725, because the masking window still left the page non-interactive (the opaque overlay sat over a visibility:hidden <ds-app>, so clicks landed on nothing) and still produced duplicate DOM that breaks strict-mode E2E locators. Changes: - index.html: drop the `ds-app[data-dspace-ssr-hidden]{visibility:hidden}` rule and give the overlay min-height:100vh. The overlay is already opaque + pointer-events:none, so it still masks the CSR rebuild visually, but clicks now pass THROUGH it to the real, still-visible <ds-app> underneath. The page is therefore interactive the entire time it is masked -> #725 ("looks rendered but nothing is clickable") cannot recur, even if removal rides the 10s cap. The snapshot is marked aria-hidden so AT (and the duplicate-node concern) target the live app, not the snapshot. - index.html: also bypass the overlay for WebDriver runners (navigator.webdriver), mirroring the existing Cypress guard, so Playwright/Selenium see no overlay and no duplicate DOM (fixes the #725 strict-mode locator failure). - app.component.ts: exclude the admin-sidebar subtree from the DOM-settle MutationObserver (its long :enter/:leave animations would keep re-arming the quiet window and push admin logins toward the cap); add OnDestroy + takeUntil + observer/timer teardown. - Fix stale `ApplicationRef.isStable` / `removeSsrOverlayWhenStable` comments (index.html, typings.d.ts). Verified against the live build (Playwright, CPU/network throttled): - WebDriver run: overlay absent (no duplicate DOM). - webdriver spoofed false (real-user path): a navbar click WHILE the snapshot is still shown reaches the live <ds-app> (interactive under mask). - anon reveal settled, CLS after reveal = 0; admin reveal settled (reason "settled", not cap), CLS after reveal = 0. Refs: dspace-customers#725, PR #1288, PR #1317, PR #1318, PR #1321 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 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> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent dce52f0 commit 468a58a

4 files changed

Lines changed: 167 additions & 108 deletions

File tree

src/app/app.component.spec.ts

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { Store, StoreModule } from '@ngrx/store';
2-
import { ComponentFixture, discardPeriodicTasks, fakeAsync, flush, inject, TestBed, waitForAsync } from '@angular/core/testing';
2+
import { ComponentFixture, discardPeriodicTasks, fakeAsync, inject, TestBed, tick, waitForAsync } from '@angular/core/testing';
33
import { CUSTOM_ELEMENTS_SCHEMA } from '@angular/core';
44
import { CommonModule } from '@angular/common';
55
import { ActivatedRoute, Router } from '@angular/router';
@@ -131,11 +131,13 @@ describe('App component', () => {
131131

132132
describe('removeSsrOverlayWhenContentVisible', () => {
133133
// The inline bootstrap script in src/index.html injects window.__dspaceRemoveSsrOverlay.
134-
// AppComponent should remove it once both auth blocking and theme loading are false.
134+
// Once auth blocking and theme loading are both false, AppComponent waits for the <ds-app> DOM
135+
// to settle (no element added/removed for the quiet window) and only then removes the overlay.
135136
let mockStore: MockStore;
136137
let themeLoading$: BehaviorSubject<boolean>;
137138
let themeService: ThemeService;
138139
let originalRaF: typeof window.requestAnimationFrame;
140+
let dsAppEl: HTMLElement;
139141

140142
beforeEach(() => {
141143
mockStore = TestBed.inject(MockStore);
@@ -144,6 +146,12 @@ describe('App component', () => {
144146
(themeService as any).isThemeLoading$ = themeLoading$.asObservable();
145147
mockStore.setState({ core: { auth: { loading: false, blocking: true } } });
146148

149+
// A settled <ds-app> with real content present, so the DOM-settle watcher can resolve.
150+
dsAppEl = document.createElement('ds-app');
151+
dsAppEl.setAttribute('style', 'display:block;height:800px');
152+
dsAppEl.innerHTML = '<main id="main-content" style="display:block;height:800px">home content</main>';
153+
document.body.appendChild(dsAppEl);
154+
147155
// Force rAF to a synchronous shim so assertions are deterministic.
148156
originalRaF = window.requestAnimationFrame;
149157
(window as any).requestAnimationFrame = (cb: FrameRequestCallback) => {
@@ -155,9 +163,10 @@ describe('App component', () => {
155163
afterEach(() => {
156164
(window as any).requestAnimationFrame = originalRaF;
157165
delete (window as any).__dspaceRemoveSsrOverlay;
166+
if (dsAppEl && dsAppEl.parentNode) { dsAppEl.parentNode.removeChild(dsAppEl); }
158167
});
159168

160-
it('removes the overlay once auth is unblocked and theme loading is finished', fakeAsync(() => {
169+
it('removes the overlay once auth/theme are ready AND the DOM has settled', fakeAsync(() => {
161170
const spy = jasmine.createSpy('__dspaceRemoveSsrOverlay');
162171
window.__dspaceRemoveSsrOverlay = spy;
163172

@@ -169,9 +178,12 @@ describe('App component', () => {
169178

170179
mockStore.setState({ core: { auth: { loading: false, blocking: false } } });
171180
themeLoading$.next(false);
172-
flush();
173181

182+
// Not removed at the gate: the DOM-settle quiet window must elapse first.
183+
expect(spy).not.toHaveBeenCalled();
184+
tick(700); // > SETTLE_QUIET_MS
174185
expect(spy).toHaveBeenCalledTimes(1);
186+
175187
discardPeriodicTasks();
176188
}));
177189

@@ -184,7 +196,7 @@ describe('App component', () => {
184196

185197
mockStore.setState({ core: { auth: { loading: false, blocking: false } } });
186198
themeLoading$.next(false);
187-
flush();
199+
tick(700);
188200

189201
expect(window.__dspaceRemoveSsrOverlay).toBeUndefined();
190202
discardPeriodicTasks();

src/app/app.component.ts

Lines changed: 111 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { distinctUntilChanged, filter, first, take, 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,
@@ -7,6 +7,7 @@ import {
77
HostListener,
88
Inject,
99
NgZone,
10+
OnDestroy,
1011
OnInit,
1112
PLATFORM_ID,
1213
} from '@angular/core';
@@ -17,7 +18,7 @@ import {
1718
Router,
1819
} from '@angular/router';
1920

20-
import { BehaviorSubject, combineLatest, Observable } from 'rxjs';
21+
import { BehaviorSubject, combineLatest, Observable, race, Subject, timer } from 'rxjs';
2122
import { select, Store } from '@ngrx/store';
2223
import { NgbModal, NgbModalConfig } from '@ng-bootstrap/ng-bootstrap';
2324
import { TranslateService } from '@ngx-translate/core';
@@ -39,10 +40,22 @@ import { distinctNext } from './core/shared/distinct-next';
3940
styleUrls: ['./app.component.scss'],
4041
changeDetection: ChangeDetectionStrategy.OnPush,
4142
})
42-
export class AppComponent implements OnInit, AfterViewInit {
43+
export class AppComponent implements OnInit, AfterViewInit, OnDestroy {
4344
notificationOptions;
4445
models;
4546

47+
/**
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.
51+
*/
52+
private destroyed$ = new Subject<void>();
53+
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
58+
4659
/**
4760
* Whether or not the authentication is currently blocking the UI
4861
*/
@@ -94,99 +107,119 @@ export class AppComponent implements OnInit, AfterViewInit {
94107

95108
/**
96109
* Drops the SSR mask overlay installed by the inline bootstrap script in src/index.html once the
97-
* real CSR content has actually been painted. This trigger has to thread a needle between the two
98-
* earlier approaches, each of which fixed one symptom and reintroduced the other:
110+
* routed CSR page has actually finished rendering. Picking the right moment is the whole problem,
111+
* and the two earlier attempts each fixed one symptom and reintroduced the other:
99112
*
100-
* - PR #1288 waited for `ApplicationRef.isStable`. That guaranteed the content was painted (no
101-
* flicker) but isStable is held hostage by ANY ongoing zone async — after an admin login the
102-
* app keeps the zone busy (authz/widgets, periodic polling, AAI/discojuice scripts) so isStable
103-
* fires many seconds late (or never, hitting the 15s fallback). The inert snapshot then masks
104-
* the live, already-rendered page -> "looks rendered but not interactive" (issue #725).
105-
* - PR #1317 switched to the loader-swap gate `!isAuthenticationBlocking && !isThemeLoading` plus
106-
* a single requestAnimationFrame. That fires promptly (fixing #725) but the gate only un-hides
107-
* `<router-outlet>`; Angular has NOT yet rendered the routed page at that instant and one rAF
108-
* runs before the browser paints, so the snapshot is dropped over an empty <ds-app> for a frame
109-
* or two -> the flicker came back.
113+
* - PR #1288 waited for `ApplicationRef.isStable`. No flicker, but isStable is held hostage by ANY
114+
* ongoing zone async — after an admin login the app keeps the zone busy (authz/widgets, periodic
115+
* polling, AAI/discojuice scripts) so isStable fires many seconds late (or hits the 15s
116+
* fallback). The inert snapshot then masks the live page -> "looks rendered but not interactive"
117+
* (issue #725).
118+
* - PR #1317 switched to the loader-swap gate `!isAuthenticationBlocking && !isThemeLoading` plus a
119+
* single requestAnimationFrame. Prompt, but that gate only un-hides `<router-outlet>`; the home
120+
* page then renders piecewise (navbar, search box, community list, recent submissions) as each
121+
* section's data arrives. Dropping the snapshot at the gate — or, as an earlier revision of this
122+
* method did, as soon as *some* content exists — exposes a half-built page that visibly pops
123+
* into place on a hard reload -> the flicker.
110124
*
111-
* We keep #1317's decoupling from isStable (so background async can never delay us) but, after the
112-
* gate opens, we wait across animation frames until the real <ds-app> is actually laid out before
113-
* removing the snapshot. See {@link removeSsrOverlayAfterContentPainted}.
125+
* The signal we actually need is "the routed page has stopped changing". So, after the gate opens,
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$}.
114130
*/
115131
private removeSsrOverlayWhenContentVisible(): void {
116-
const w: Window | undefined = this._window?.nativeWindow;
117-
if (!w || typeof w.__dspaceRemoveSsrOverlay !== 'function') {
118-
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
119135
}
120-
// run outside Angular so the subscription 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).
121138
this.ngZone.runOutsideAngular(() => {
122-
combineLatest([
123-
this.store.pipe(select(isAuthenticationBlocking), distinctUntilChanged()),
124-
this.themeService.isThemeLoading$,
125-
]).pipe(
126-
filter(([blocking, themeLoading]: [boolean, boolean]) => !blocking && !themeLoading),
127-
first(),
139+
this.routedPageReadyToReveal$().pipe(
140+
takeUntil(this.destroyed$),
128141
).subscribe(() => {
129-
this.removeSsrOverlayAfterContentPainted(w);
142+
// one frame so the freshly rendered content is painted before the snapshot fades out
143+
this.runAfterNextFrame(win, () => win.__dspaceRemoveSsrOverlay?.());
130144
});
131145
});
132146
}
133147

134148
/**
135-
* Waits until the routed CSR view has been committed to the DOM and painted, then removes the SSR
136-
* snapshot overlay. "Painted" is approximated by the real <ds-app> reaching a non-trivial height
137-
* AND containing its `#main-content` host (i.e. it is no longer the empty shell the overlay script
138-
* left behind). We poll this cheap layout signal once per animation frame, capped at MAX_FRAMES so
139-
* that — unlike isStable in #1288 — nothing can hold the overlay open indefinitely; the 15s hard
140-
* fallback in index.html stays as the catastrophic-error safety 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$}.
141152
*/
142-
private removeSsrOverlayAfterContentPainted(w: Window): void {
143-
const doc: Document = this.document;
144-
const raf: ((cb: FrameRequestCallback) => number) | null =
145-
typeof w.requestAnimationFrame === 'function' ? w.requestAnimationFrame.bind(w) : null;
146-
const remove = () => {
147-
if (typeof w.__dspaceRemoveSsrOverlay === 'function') {
148-
w.__dspaceRemoveSsrOverlay();
149-
}
150-
};
151-
const MAX_FRAMES = 180; // ~3s @60fps safety cap; the routed shell normally paints within a few frames
152-
const MIN_CONTENT_HEIGHT = 200; // px: enough to prove the real <ds-app> is no longer the empty shell
153-
let frames = 0;
154-
const contentPainted = (): boolean => {
155-
const app: Element | null = doc.querySelector('ds-app');
156-
if (!app) {
157-
return false;
158-
}
159-
let height = 0;
160-
try {
161-
height = app.getBoundingClientRect().height;
162-
} catch (e) {
163-
height = 0;
164-
}
165-
return height >= MIN_CONTENT_HEIGHT && app.querySelector('#main-content') !== null;
166-
};
167-
const tick = () => {
168-
if (contentPainted() || ++frames >= MAX_FRAMES) {
169-
// one more frame so the painted content is committed to screen before the snapshot fades
170-
if (raf) {
171-
raf(remove);
172-
} else {
173-
remove();
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+
}
165+
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();
174181
}
175-
return;
176-
}
177-
if (raf) {
178-
raf(tick);
179-
} else {
180-
setTimeout(tick, 16);
181-
}
182-
};
183-
if (raf) {
184-
raf(tick);
182+
});
183+
observer.observe(dsApp, { childList: true, subtree: true });
184+
return () => observer.disconnect();
185+
});
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+
}
199+
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());
185213
} else {
186-
setTimeout(tick, 16);
214+
callback();
187215
}
188216
}
189217

218+
ngOnDestroy(): void {
219+
this.destroyed$.next();
220+
this.destroyed$.complete();
221+
}
222+
190223
ngOnInit() {
191224
/** Implement behavior for interface {@link ModalBeforeDismiss} */
192225
this.modalConfig.beforeDismiss = async function () {

0 commit comments

Comments
 (0)