SAV/Backport: Fix home-page SSR->CSR flicker#1311
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:
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.
Pull request overview
Backports an SSR→CSR anti-flicker mechanism for the home page by keeping the SSR-rendered DOM visible during Angular 15’s client bootstrap rebuild, then removing the mask once the app becomes stable.
Changes:
- Add an inline “SSR overlay” script + CSS in
index.htmland a corresponding teardown hook inAppComponentusingApplicationRef.isStable. - Re-enable
CustomEagerThemeModuleto avoid lazy-loading themed wrappers that exacerbate visible gaps. - Add typings + unit tests for the new
window.__dspaceRemoveSsrOverlayglobal and adjust bundle budgets.
Reviewed changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/typings.d.ts | Declares the window.__dspaceRemoveSsrOverlay global for TS type safety. |
| src/themes/eager-themes.module.ts | Imports CustomEagerThemeModule eagerly to reduce flicker caused by lazy wrapper loading. |
| src/index.html | Adds SSR overlay CSS + inline bootstrap script to preserve SSR DOM/styles until CSR is stable. |
| src/app/app.component.ts | Calls overlay teardown once ApplicationRef.isStable emits true (outside Angular zone). |
| src/app/app.component.spec.ts | Adds unit tests validating overlay teardown behavior. |
| angular.json | Raises initial bundle size budgets to accommodate eager theme inclusion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/app.component.spec.ts (1)
132-188: ⚡ Quick winAdd coverage for the no-
requestAnimationFramefallback branch.The new tests only exercise the rAF-present path. Please add one case with
window.requestAnimationFrameunset and assert the remover still runs after the 50ms delay, so both scheduler branches are protected.Proposed test addition
it('is a no-op when the global is not injected (e.g. CSR-only route, SSR skipped)', fakeAsync(() => { @@ expect(window.__dspaceRemoveSsrOverlay).toBeUndefined(); })); + + it('removes the overlay when requestAnimationFrame is unavailable', fakeAsync(() => { + const spy = jasmine.createSpy('__dspaceRemoveSsrOverlay'); + window.__dspaceRemoveSsrOverlay = spy; + (window as any).requestAnimationFrame = undefined; + + const f = TestBed.createComponent(AppComponent); + f.detectChanges(); + + isStable$.next(true); + tick(50); + flush(); + + expect(spy).toHaveBeenCalledTimes(1); + }));As per coding guidelines, “New features must include new unit tests”.
🤖 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/app/app.component.spec.ts` around lines 132 - 188, Add a new test case in the removeSsrOverlayWhenStable describe block to cover the fallback path when requestAnimationFrame is not available. In this test, delete window.requestAnimationFrame (or set it to undefined) before constructing the AppComponent, then verify that the __dspaceRemoveSsrOverlay spy is still called exactly once after the 50ms delay using fakeAsync, tick, and flush. This ensures the removeSsrOverlayWhenStable implementation correctly handles both the rAF-present path and the fallback scheduler branch.Source: Coding guidelines
🤖 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.
Inline comments:
In `@angular.json`:
- Around line 103-104: The bundle budget thresholds for the initial bundle
(maximumWarning at 5.5mb and maximumError at 6mb) are set too high with
excessive headroom, reducing the effectiveness of regression detection. Reduce
both the maximumWarning and maximumError values to be closer to the actual
current post-change bundle size with only minimal headroom (approximately
50-100kb above the actual size), so that future bundle size regressions will
fail the build quickly and be detected early.
---
Nitpick comments:
In `@src/app/app.component.spec.ts`:
- Around line 132-188: Add a new test case in the removeSsrOverlayWhenStable
describe block to cover the fallback path when requestAnimationFrame is not
available. In this test, delete window.requestAnimationFrame (or set it to
undefined) before constructing the AppComponent, then verify that the
__dspaceRemoveSsrOverlay spy is still called exactly once after the 50ms delay
using fakeAsync, tick, and flush. This ensures the removeSsrOverlayWhenStable
implementation correctly handles both the rAF-present path and the fallback
scheduler branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4b08381-9c4f-419d-bf0e-0e4c67e443ce
⛔ Files ignored due to path filters (3)
_build.logis excluded by!**/*.log_install.logis excluded by!**/*.log_spec.logis excluded by!**/*.log
📒 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
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>
_build.log, _install.log and _spec.log are local deploy-tooling output that should never have been tracked. Remove them and gitignore /_*.log. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
|
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
Bug Fixes
Chores