TUL/Backport: Fix home-page SSR->CSR flicker#1310
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/index.html (1)
10-29: 💤 Low valueHardcoded white background may cause flash on dark themes.
The overlay uses
background:#fff`` which will cause a brief white flash if the site theme has a dark background. Consider using a CSS variable orinheritto match the theme.Suggested approach
`#__dspace_ssr_overlay` { position: absolute; top: 0; left: 0; right: 0; width: 100%; z-index: 10000; - background: `#fff`; + background: var(--ds-background-color, `#fff`); pointer-events: none; }This requires the CSS variable to be defined, or you could use
background: inheritif the body background is set appropriately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.html` around lines 10 - 29, The `#__dspace_ssr_overlay` element in the style block with id __dspace-ssr-overlay-style uses a hardcoded white background (background: `#fff`) which causes a flash on dark themes. Replace the hardcoded background property with either a CSS variable that respects the site theme or use background: inherit to match the parent element's background. Ensure the chosen solution works with your theme's color scheme so the overlay appears seamless when the page loads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/index.html`:
- Around line 10-29: The `#__dspace_ssr_overlay` element in the style block with
id __dspace-ssr-overlay-style uses a hardcoded white background (background:
`#fff`) which causes a flash on dark themes. Replace the hardcoded background
property with either a CSS variable that respects the site theme or use
background: inherit to match the parent element's background. Ensure the chosen
solution works with your theme's color scheme so the overlay appears seamless
when the page loads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ea61586-ce7b-4669-8f57-1c3ed501a790
📒 Files selected for processing (6)
angular.jsonsrc/app/app.component.spec.tssrc/app/app.component.tssrc/index.htmlsrc/themes/eager-themes.module.tssrc/typings.d.ts
There was a problem hiding this comment.
Pull request overview
Backports the “SSR → CSR flicker” mitigation by keeping the SSR-rendered DOM visible during Angular 15’s client bootstrap rebuild, then removing the mask once the app becomes stable.
Changes:
- Re-enabled eager loading of the custom theme module to avoid wrapper lazy-load gaps during bootstrap.
- Added an inline anti-flicker overlay script + CSS in
index.html, with a correspondingAppComponenthook to remove the overlay onApplicationRef.isStable. - Added typings + unit tests for the new
window.__dspaceRemoveSsrOverlayintegration and raised production bundle budgets.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/typings.d.ts | Adds a typed Window.__dspaceRemoveSsrOverlay global used by the anti-flicker flow. |
| src/themes/eager-themes.module.ts | Enables CustomEagerThemeModule eager import to prevent theme wrapper lazy-load gaps. |
| src/index.html | Introduces the SSR snapshot overlay CSS + pre-bootstrap script that installs/removes the overlay. |
| src/app/app.component.ts | Removes the SSR overlay after first stable state (with a small rAF + timeout pad). |
| src/app/app.component.spec.ts | Adds tests asserting overlay removal is called once on stability and is a no-op when absent. |
| angular.json | Increases production initial bundle size budgets to accommodate the eager theme import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The overlay remover bailed out via `if (!el) return;` before unhiding <ds-app>, so if the overlay node went missing (browser extension, race, external script) the app stayed visibility:hidden forever -> blank page, plus the kept SSR styles leaked. Unhide the app and clean up the kept styles unconditionally, before checking for the overlay node. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3d34499 to
87dc1aa
Compare
Make the ApplicationRef.isStable override in the removeSsrOverlayWhenStable suite configurable and restore the original descriptor in afterEach, so the patched observable can't leak onto the shared TestBed instance. Add a test for the requestAnimationFrame-absent fallback branch of the remover. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Propagates VSB-TUO's fix #1317 to this instance. The overlay was removed when ApplicationRef.isStable settled, but isStable can be delayed for seconds by post-login admin zone activity (auth work, background polling, third-party scripts) - during which the live app stays hidden under the SSR mask and the page renders but is non-interactive (dataquest-dev/dspace-customers#725). Switch removal to the same condition root.component.html uses to show real content: !isAuthenticationBlocking && !isThemeLoading. Drop the now-unused ApplicationRef injection and the 50ms pad; keep the 15s hard fallback as a catastrophic safety net. Tests and the theme-service mock updated to match. Ref: #1317 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Switched the SSR overlay-removal trigger from
|
Supersedes the #1317 content-visible trigger backported earlier. That gate (!isAuthenticationBlocking && !isThemeLoading) still revealed a half-built page on hard reload, so VSB-TUO's #1318/#1321 keep the snapshot until the routed <ds-app> DOM has SETTLED (MutationObserver + quiet window, with a content height / #main-content check and a 10s cap). The overlay is now a purely visual mask, so the live app stays interactive underneath while it rebuilds (closes dspace-customers#725 - "looks rendered but not clickable"). index.html, app.component.ts, spec and typings are synced to VSB-TUO's final version; the VSB-only ngAfterViewInit delay(0) is omitted (these instances don't carry it). Ref: #1318, #1321 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Problem description
Backport from #1287
Analysis
(Write here, if there is needed describe some specific problem. Erase it, when it is not needed.)
Problems
(Write here, if some unexpected problems occur during solving issues. Erase it, when it is not needed.)
Sync verification
If en.json5 or cs.json5 translation files were updated:
yarn run sync-i18n -t src/assets/i18n/cs.json5 -ito synchronize messages, and changes are included in this PR.Manual Testing (if applicable)
Copilot review
Summary by CodeRabbit
New Features
Improvements
Tests
Chores