Skip to content

fix(loader): default shared loader to spinner-only - #119

Open
drugoi wants to merge 2 commits into
Techinterview-space:mainfrom
drugoi:codex/loader-fix-lighthouse-followup
Open

drugoi wants to merge 2 commits into
Techinterview-space:mainfrom
drugoi:codex/loader-fix-lighthouse-followup

Conversation

@drugoi

@drugoi drugoi commented Feb 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Make shared loader spinner-only by default to avoid odd GIF rendering on /salaries and other route-level loaders.
  • Keep GIF loader available as an opt-in mode via [showGif]="true".
  • Keep spinner visible if GIF load fails.
  • Add maintainer-facing changelog section with production Lighthouse snapshot and validation notes.

Changelog

  • Added: CHANGELOG.md section 2026-02-11 (Loader Follow-up) with maintainer notes and Lighthouse scores.

Why

  • Recent loader behavior introduced delayed multi-MB GIF loading in shared route loaders.
  • This caused visibly strange loader UX and contributed to mobile performance risk.

Lighthouse Snapshot (production, 2026-02-11)

  • /salaries mobile: Performance 47, Accessibility 100, Best Practices 93, SEO 93
  • /salaries desktop: Performance 99, Accessibility 100, Best Practices 100, SEO 90
  • / mobile: Performance 60, Accessibility 97, Best Practices 93, SEO 100
  • / desktop: Performance 74, Accessibility 98, Best Practices 93, SEO 100

Validation

  • npx ng test --no-watch --browsers ChromiumNoSandbox --include=src/app/shared/components/loading-spinner/loading-spinner.component.spec.ts
  • npm run build-prod

Summary by CodeRabbit

  • New Features

    • Added configurable GIF display option for the loading spinner
    • Improved error handling for failed GIF loads
  • Bug Fixes

    • Fixed error event handling to properly manage GIF load failures
  • Tests

    • Updated test suite to cover conditional GIF loading behavior and timing
  • Documentation

    • Added changelog entry documenting loader behavior follow-up and performance observations

Disable GIF loading by default in shared loader and make GIF mode opt-in via showGif input.

Also add maintainer changelog entry with production Lighthouse snapshots and validation commands.
@coderabbitai

coderabbitai Bot commented Feb 11, 2026 •

Copy link
Copy Markdown

Walkthrough

This pull request adds conditional GIF loading to the shared loader component with a new showGif input property that controls whether the loading GIF is displayed. When disabled, GIF loading is skipped entirely. A new error handler method (onGifLoadFailed) resets the GIF state if image loading fails. Tests are updated to validate both default behavior and delayed loading scenarios.

Changes

Cohort / File(s) Summary
Loading Spinner Component
src/app/shared/components/loading-spinner/loading-spinner.component.ts, loading-spinner.component.html, loading-spinner.component.spec.ts
Added showGif input property to conditionally enable GIF loading. Introduced onGifLoadFailed() error handler to reset GIF state on load failure. Updated template to call error handler on GIF load errors. Modified tests to verify GIF is not loaded by default and is delayed when enabled.
Documentation
CHANGELOG.md
Added changelog entry dated 2026-02-11 documenting the loader follow-up with maintainer notes on spinner defaults, GIF behavior, test updates, and validation commands.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: defaulting the shared loader to spinner-only mode instead of GIF-based loading.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
src/app/shared/components/loading-spinner/loading-spinner.component.ts (1)

42-46: Consider ChangeDetectionStrategy.OnPush for this component.

Since setTimeout mutates component state (loadingUrl, loadingAlt) outside Angular's zone awareness under OnPush, this currently works only because the component uses the default change detection strategy. This is fine for now, but if you ever switch to OnPush, the delayed GIF assignment on Line 44-45 won't trigger change detection without a manual markForCheck() call.

src/app/shared/components/loading-spinner/loading-spinner.component.spec.ts (1)

27-53: Missing test coverage for onGifLoadFailed().

The new onGifLoadFailed() method is untested. Consider adding a test that verifies loadingUrl is reset to "" and showPreloader is set back to true after a GIF load error. This is a new code path introduced by this PR and exercised by the template's (error) binding.

💡 Suggested test
it("resets to spinner on GIF load failure", fakeAsync(() => {
  component.ngOnDestroy();
  component.showGif = true;
  component.delayMs = 0;
  component.loadingUrl = "";

  component.ngOnInit();
  tick(0);
  expect(component.loadingUrl).not.toBe("");

  component.onGifLoadFailed();
  expect(component.loadingUrl).toBe("");
  expect(component.showPreloader).toBe(true);

  component.ngOnDestroy();
}));

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@drugoi

drugoi commented Feb 11, 2026

Copy link
Copy Markdown
Contributor Author
Screenshot 2026-02-11 at 22 52 00

Такое поймал

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant