Skip to content

Commit 9a0258b

Browse files
milanmajchrakclaude
andcommitted
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>
1 parent dce52f0 commit 9a0258b

2 files changed

Lines changed: 103 additions & 54 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: 86 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -94,30 +94,33 @@ export class AppComponent implements OnInit, AfterViewInit {
9494

9595
/**
9696
* 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:
97+
* routed CSR page has actually finished rendering. Picking the right moment is the whole problem,
98+
* and the two earlier attempts each fixed one symptom and reintroduced the other:
9999
*
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.
100+
* - PR #1288 waited for `ApplicationRef.isStable`. No flicker, but isStable is held hostage by ANY
101+
* ongoing zone async — after an admin login the app keeps the zone busy (authz/widgets, periodic
102+
* polling, AAI/discojuice scripts) so isStable fires many seconds late (or hits the 15s
103+
* fallback). The inert snapshot then masks the live page -> "looks rendered but not interactive"
104+
* (issue #725).
105+
* - PR #1317 switched to the loader-swap gate `!isAuthenticationBlocking && !isThemeLoading` plus a
106+
* single requestAnimationFrame. Prompt, but that gate only un-hides `<router-outlet>`; the home
107+
* page then renders piecewise (navbar, search box, community list, recent submissions) as each
108+
* section's data arrives. Dropping the snapshot at the gate — or, as an earlier revision of this
109+
* method did, as soon as *some* content exists — exposes a half-built page that visibly pops
110+
* into place on a hard reload -> the flicker.
110111
*
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}.
112+
* The signal we actually need is "the routed page has stopped changing". So, after the gate opens,
113+
* we keep the snapshot until the real <ds-app> DOM has SETTLED: no element added or removed for a
114+
* short quiet window. This stays decoupled from isStable (DOM-settle ignores non-rendering
115+
* background async, so admin reveals in a few seconds rather than ~15s) while waiting for the page
116+
* the user is actually looking at to be fully built. See {@link removeSsrOverlayWhenDomSettles}.
114117
*/
115118
private removeSsrOverlayWhenContentVisible(): void {
116119
const w: Window | undefined = this._window?.nativeWindow;
117120
if (!w || typeof w.__dspaceRemoveSsrOverlay !== 'function') {
118121
return;
119122
}
120-
// run outside Angular so the subscription does not keep change detection alive
123+
// run outside Angular so the subscription/observer does not keep change detection alive
121124
this.ngZone.runOutsideAngular(() => {
122125
combineLatest([
123126
this.store.pipe(select(isAuthenticationBlocking), distinctUntilChanged()),
@@ -126,65 +129,99 @@ export class AppComponent implements OnInit, AfterViewInit {
126129
filter(([blocking, themeLoading]: [boolean, boolean]) => !blocking && !themeLoading),
127130
first(),
128131
).subscribe(() => {
129-
this.removeSsrOverlayAfterContentPainted(w);
132+
this.removeSsrOverlayWhenDomSettles(w);
130133
});
131134
});
132135
}
133136

134137
/**
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.
138+
* Removes the SSR snapshot overlay once the real <ds-app> subtree has stopped mutating for
139+
* SETTLE_QUIET_MS (the routed page finished rendering its sections), requiring a minimum amount of
140+
* content first so we never settle on the empty shell the overlay script left behind. A
141+
* MutationObserver tracks element add/remove inside <ds-app>; every such change rearms the quiet
142+
* timer. Capped at SETTLE_MAX_MS so a page that never goes quiet (e.g. constant background DOM
143+
* updates) still reveals; the 15s fallback in index.html remains the ultimate net.
141144
*/
142-
private removeSsrOverlayAfterContentPainted(w: Window): void {
145+
private removeSsrOverlayWhenDomSettles(w: Window): void {
143146
const doc: Document = this.document;
144-
const raf: ((cb: FrameRequestCallback) => number) | null =
145-
typeof w.requestAnimationFrame === 'function' ? w.requestAnimationFrame.bind(w) : null;
147+
const SETTLE_QUIET_MS = 600; // no DOM change for this long => the routed page has finished rendering
148+
const SETTLE_MAX_MS = 10000; // hard cap so a never-quiet page still reveals (below index.html's 15s net)
149+
const MIN_CONTENT_HEIGHT = 200; // px: proves <ds-app> is no longer the empty shell
150+
151+
const app: Element | null = doc.querySelector('ds-app');
146152
const remove = () => {
147153
if (typeof w.__dspaceRemoveSsrOverlay === 'function') {
148154
w.__dspaceRemoveSsrOverlay();
149155
}
150156
};
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+
const hasMinContent = (): boolean => {
158+
const el: Element | null = doc.querySelector('ds-app');
159+
if (!el) {
157160
return false;
158161
}
159162
let height = 0;
160163
try {
161-
height = app.getBoundingClientRect().height;
164+
height = el.getBoundingClientRect().height;
162165
} catch (e) {
163166
height = 0;
164167
}
165-
return height >= MIN_CONTENT_HEIGHT && app.querySelector('#main-content') !== null;
168+
return height >= MIN_CONTENT_HEIGHT && el.querySelector('#main-content') !== null;
166169
};
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();
174-
}
170+
171+
let done = false;
172+
let quietTimer: ReturnType<typeof setTimeout> | null = null;
173+
let capTimer: ReturnType<typeof setTimeout> | null = null;
174+
175+
const finish = () => {
176+
if (done) {
175177
return;
176178
}
177-
if (raf) {
178-
raf(tick);
179+
done = true;
180+
if (quietTimer !== null) { clearTimeout(quietTimer); }
181+
if (capTimer !== null) { clearTimeout(capTimer); }
182+
try { observer.disconnect(); } catch (e) { /* noop */ }
183+
// one rAF so the final rendered frame is committed to screen before the snapshot fades
184+
if (typeof w.requestAnimationFrame === 'function') {
185+
w.requestAnimationFrame(remove);
179186
} else {
180-
setTimeout(tick, 16);
187+
remove();
181188
}
182189
};
183-
if (raf) {
184-
raf(tick);
185-
} else {
186-
setTimeout(tick, 16);
190+
191+
const armQuietTimer = () => {
192+
if (done) {
193+
return;
194+
}
195+
if (quietTimer !== null) { clearTimeout(quietTimer); }
196+
quietTimer = setTimeout(() => {
197+
// DOM has been quiet for SETTLE_QUIET_MS; reveal once real content is there, else keep waiting
198+
if (hasMinContent()) {
199+
finish();
200+
} else {
201+
armQuietTimer();
202+
}
203+
}, SETTLE_QUIET_MS);
204+
};
205+
206+
const observer = new MutationObserver((mutations: MutationRecord[]) => {
207+
for (const m of mutations) {
208+
if (m.type === 'childList' && (m.addedNodes.length > 0 || m.removedNodes.length > 0)) {
209+
let elementChanged = false;
210+
m.addedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } });
211+
m.removedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } });
212+
if (elementChanged) {
213+
armQuietTimer(); // a section rendered -> reset the quiet window
214+
return;
215+
}
216+
}
217+
}
218+
});
219+
220+
if (app) {
221+
observer.observe(app, { childList: true, subtree: true });
187222
}
223+
capTimer = setTimeout(finish, SETTLE_MAX_MS);
224+
armQuietTimer();
188225
}
189226

190227
ngOnInit() {

0 commit comments

Comments
 (0)