close
Skip to content

12714/bug/frozen carousels - #12812

Merged
jimchamp merged 3 commits into
internetarchive:masterfrom
LUC4SWM:12714/bug/frozen-carousels
May 29, 2026
Merged

12714/bug/frozen carousels#12812
jimchamp merged 3 commits into
internetarchive:masterfrom
LUC4SWM:12714/bug/frozen-carousels

Conversation

@LUC4SWM

@LUC4SWM LUC4SWM commented May 27, 2026

Copy link
Copy Markdown
Contributor

Closes #12714

Bug fix

Technical

Makes carousel loading-state handling more defensive in two failure cases:

  • If the template-provided carousel-i18n-strings input is missing, the carousel defaults the loading label to an empty string instead of throwing while attempting to load more cards.
  • If a CarouselLoadMore request fails, the temporary loading slide is removed and the carousel is unlocked so users can continue navigating without refreshing the page.

Adds JavaScript regression coverage for both failure paths.

Testing

  • Ran carousel.test.js
  • Ran eslint
  • Reproduced the missing-i18n console exception locally before the fallback was added, then rebuilt assets and verified the carousel no longer throws during load-more navigation.
  • Manually rejected one /partials/CarouselLoadMore.json request in DevTools and verified previously loaded slides remain navigable and subsequent navigation can recover without refreshing.

Screenshot

Stakeholders

@mekarpeles

Copy link
Copy Markdown
Member

Thank you for submitting this PR, @LUC4SWM!

🤖 Copilot has been assigned for an initial review.

@jimchamp is assigned to this PR and currently has:

  • 8 open PR(s) of equal or higher priority to review first
PR triage checklist (maintainers / Pam)
  • PR description — not empty; explains what the change does and how to verify it
  • References an issue — PR body contains a #NNN reference
    • Linked issue is triaged — has a Priority: * label (not just Needs: Triage)
    • Linked issue is assigned — has at least one assignee
  • Commit history clean — no WIP/fixup/conflict noise; commit messages are meaningful
  • CI passing — no failing check-runs
  • Test cases present — if the change touches substantive logic, test coverage exists or is explained
  • Proof of testing — PR body includes a description of what was tested, a screenshot, or a video

Note

This comment was automatically generated by Pam, Open Library's Project AI Manager, on behalf of @mekarpeles. Pam is designed to provide status visibility, perform basic project management functions and relevant codebase research, and provide actionable feedback so contributors aren't left waiting.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves the resilience of the Slick-based carousel “load more” behavior to prevent UI freezes when required i18n inputs are missing or when load-more network requests fail.

Changes:

  • Default carousel i18n loading label to '' when the carousel-i18n-strings input is absent.
  • On CarouselLoadMore request failure, remove the temporary loading slide and unlock the carousel.
  • Add Jest regression tests covering both failure paths.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/unit/js/carousel.test.js Adds unit tests for missing-i18n and failed load-more behavior.
openlibrary/plugins/openlibrary/js/carousel/Carousel.js Adds defensive i18n default and unlock/cleanup on AJAX failure.

Comment on lines +173 to +176
.fail(() => {
this.removeLoadingSlide();
loadMore.locked = false;
})

test('unlocks and removes the loading slide when loading more cards fails', async() => {
const request = $.Deferred();
$.ajax = jest.fn(() => request.promise());

@jimchamp jimchamp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LUC4SWM!

@jimchamp
jimchamp merged commit 08f6bfc into internetarchive:master May 29, 2026
4 checks passed
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.

Carousel navigation freezes after reaching initial loaded results

4 participants