SCC-5375: Multiselect lazy loading by KaseyW31 · Pull Request #2007 · NYPL/nypl-design-system

KaseyW31 · 2026-07-10T14:12:29Z

Fixes JIRA ticket SCC-5375

This PR does the following:

Adds the lazy-load option to the listOverflow prop of the MultiSelect component

How has this been tested?

Local build of storybook - "Lazy loading items" story

Accessibility concerns or updates

Accessibility Checklist

Checked Storybook's "Accessibility" tab for color contrast and other issues.
The feature works with keyboard inputs including tabbing back and forward and pressing space, enter, arrow, and esc keys.
For hidden text or when aria-live is used, a screenreader was used to verify the text is read.
For features that involve UI updates and focusing on DOM refs, focus management was reviewed.
The feature works when the page is zoomed in to 200% and 400%.

Open Questions

Checklist:

I have updated the Storybook documentation and changelog accordingly.
I have added relevant accessibility documentation for this pull request.
All new and existing tests passed.

Front End Review:

Review the Vercel preview deployment once it is ready.

vercel · 2026-07-10T14:12:36Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
nypl-design-system	Ready	Preview, Comment	Jul 21, 2026 3:15pm

7emansell

overall looks good to me, and it's working great in Storybook, just a couple notes/questions on the implementation.
I do have to ask my favorite question though... have you tried this in RC advanced search and if so is it performant

7emansell · 2026-07-10T18:58:42Z

        }, 1); // Ensure focus logic runs after state update
      };

+      const onItemsListScroll = () => {


I think this chunk can be replaced by setting rootMargin on the IntersectionObserver– i'm new to that API but it seems like if it's set (to 200px or something) it would trigger the same anticipatory load?

This chunk allows for a more gradual and scaled loading - without it, scrolling down just a bit loads a lot of items (even with rootMargin at 0px). But that would work too, any thoughts on which is preferred?
I'm also quite new to the API so I could definitely be missing something else, and I can continue to explore it more

No i see what you're saying here! I think it's worth the workaround to have a better scrolling experience

Yes, the scrolling-loading could use some polishing. I'd love to have a chat to learn more about the API and the config.

I put these console.logs right before the loadMoreLazyItems() call in the observer and right before loadMoreLazyItems() call in the onItemsListScroll function.

The scroll handler keeps moving the sentinel away before the observer can reach it. So the observer fires only once, while the scroll handler takes over and it's running a lot.

I tried removing onItemsListScroll and I personally did not observe much of a difference, so would love to see the difference you're seeing.

Also with regards to, "scrolling down just a bit loads a lot of items" -- isn't the number of items related to the setState in loadMoreLazyItems, rather than anything to do with the rootMargin?

setLazyItemsVisible((previousVisibleItems) => Math.min( previousVisibleItems + defaultItemsVisible + previousVisibleItems / defaultItemsVisible, // Scales with further scrolling itemsList.length ) );

@oliviawongnyc I didn't realize that, thanks for pointing that out! When I was testing in research catalog using just the observer, scrolling fast would cause stuttering. After scrolling all the way to the bottom, it took some time to load more items, and by then the user would be scrolling past the panel entirely. So I think I overcorrected for that and messed up the observer in the process without realizing it.

As for the difference, I misinterpreted the comment and thought the chunk was referring to the scaling part of it. Removing the scroll handler changes the behavior slightly but doesn't make the difference I mentioned, sorry for the confusion.

I'll take a look at reducing stuttering while only using the IntersectionObserver.

I removed the scroll handler and tweaked some of the scaling. I tried calculating the rootMargin using pixel heights in the scroll panel but that didn't work out great, so the numbers I have now are from trial and error. Any thoughts on that or the scrolling experience now @7emansell @oliviawongnyc ?

Changes looking good to me!

As for the scaling, what happens with a fixed root margin? rootMargin: "0px 0px 100px 0px", for example? I would think we just want the observer to trigger loading before someone reaches the bottom of the visible list and not need to scale, but lmk your thoughts.

Same question about the batching. Does the increment size need to scale?

7emansell · 2026-07-10T19:01:05Z

    );
  });

+  it("should lazily load more list items while scrolling in lazy load mode", async () => {


Clever test!! also wondering: did you try a more involved mock of the intersection observer and it got too messy ? It seems like it would be difficult to mock

I did not 🤔 I could try it though if that would be preferred

No worries just wondering

Agreed on the good test and good mock.

7emansell · 2026-07-10T19:02:09Z

 export type MultiSelectWidths = typeof multiSelectWidthsArray[number];
-export const multiSelectListOverflowArray = ["scroll", "expand"] as const;
+export const multiSelectListOverflowArray = [
+  "scroll",


Interested in marty/edwin thoughts but maybe lazy-load should be a flag you can set if you've picked scroll, not a standalone type

Ah that does make sense

Let me review before you make this update. Without having looked too closely at the PR, Emma's comment sounds reasonable. Perhaps it can be an independent flag that works for both scroll and expand?

this shouldn't be possible on expand right?

Would it be wrong to always have lazy load be enabled for the scroll option? It works well and it doesn't seem like it has a big overhead.

Yes, agreed. lazy-load should not be an option for listOverflow. It should be a boolean flag.

Here's another idea. Although, I am not sure if the timing of this is possible. Can lazy load be automatically enabled if the total options is greater than x? Otherwise, scrolling functions as is always has?

Do we know the threshold when performance takes a hit?

I'm not sure about the threshold. I'm open to logging performance/render times compared to the number of items, but I imagine that performance could also vary based on how it's used in the consuming app?

Right now 20 + defaultItemsVisible are loaded initially per @bigfishdesign13 's suggestion (although defaultItemsVisible is only used for expand since the default appears to be ~7 for scrolling, but that might be a separate concern). Maybe this could be the threshold for when lazy loading is enabled?

Yes, let's keep it at 20 + defaultItemVisible as the threshold for now and we can gauge its performance once it's live.

KaseyW31 · 2026-07-10T19:59:42Z

have you tried this in RC advanced search and if so is it performant

I did, without any other changes to RC ~~it doesn't seem to make a much of a difference in prod~~, but it does in dev mode... I haven't tried it with my changes in this PR yet, but I imagine it would make less of a difference since it wouldn't be rerendered as often
Do you have any thoughts on whether this is worth trying?

Actually on second look there is a slight performance improvement in the language checkbox and input fields (if I squint). Previously I was still on DS 4.3.0 which might have slowed it down a bit

7emansell · 2026-07-13T14:04:55Z

have you tried this in RC advanced search and if so is it performant

I did, without any other changes to RC ~~it doesn't seem to make a much of a difference in prod~~, but it does in dev mode... I haven't tried it with my changes in this PR yet, but I imagine it would make less of a difference since it wouldn't be rerendered as often Do you have any thoughts on whether this is worth trying?

Actually on second look there is a slight performance improvement in the language checkbox and input fields (if I squint). Previously I was still on DS 4.3.0 which might have slowed it down a bit

if it makes a difference in dev mode that's awesome (thinking along the lines of our unit test duration..) Let me know if you have a branch that has this + 4.2.0 + no other changes in advanced search, i'd be interested to see. I like the idea of adding this as a non-breaking change, totally optional, to the Multiselect on the DS level and then handling the useMemo on the consuming client level, that seems like a fair separation of concerns to me

KaseyW31 · 2026-07-13T14:46:48Z

Let me know if you have a branch that has this + 4.2.0 + no other changes in advanced search, i'd be interested to see.

Yes I pushed my changes to the no-ref/test-multiselect-lazy branch! I do think the unit tests run slightly faster, now around 40-50s

EdwinGuzman

Some comments. Before I review the main file, we should decide on the approach and whether the lazy loading should always be enabled. If so, it might change the code.

EdwinGuzman · 2026-07-14T14:39:46Z

    );
  });

+  it("should lazily load more list items while scrolling in lazy load mode", async () => {


Agreed on the good test and good mock.

EdwinGuzman · 2026-07-14T14:42:26Z

+  observe: () => null,
+  disconnect: () => null,
+});
+window.IntersectionObserver = jest


Emma already asked if you tested the rc npm release in Research Catalog. Did you also run tests in RC?

I noticed that whenever we mock global APIs in the DS, unit tests in consuming apps need to be updated as well. It's not bad, but we would just have to let teams know about the dependency.

Yes, the IntersectionObserver mock is needed when lazy-load is selected. Would this be a factor at all in deciding whether lazy loading should be always enabled for scroll?

Not a big factor but just something to call out when devs import the release version. Did you have to add it in a global pre-test jest file in RC?

Seems like it's only needed if the number of items in the MultiSelect > defaultItemsVisible + 20, when lazy loading is triggered. Otherwise putting this block of code

const intersectionObserverMock = () => ({ observe: () => null, disconnect: () => null, unobserve: () => null, }) window.IntersectionObserver = jest .fn() .mockImplementation(intersectionObserverMock)

either in the test files with the MultiSelect, or the global pre-test file is needed.

EdwinGuzman · 2026-07-14T14:45:17Z

 export type MultiSelectWidths = typeof multiSelectWidthsArray[number];
-export const multiSelectListOverflowArray = ["scroll", "expand"] as const;
+export const multiSelectListOverflowArray = [
+  "scroll",


Would it be wrong to always have lazy load be enabled for the scroll option? It works well and it doesn't seem like it has a big overhead.

oliviawongnyc · 2026-07-14T20:18:55Z

+
+        const { scrollTop, clientHeight, scrollHeight } = itemsListRef.current;
+        const scrollThreshold = scrollHeight / 2; // Scales with further scrolling
+        const isAtBottom =


What do you think about isHalfwayThroughLoadedContent rather than isAtBottom? Then someone reading this can easily see that when isHalfwayThroughLoadedContent, loadMoreLazyItems() and I think it would act like a little bit of documentation.

Scratch this. After looking at this longer, I don't think we should use two triggers for the loading, and think we should use the observer over this onScroll.

oliviawongnyc · 2026-07-15T17:42:37Z

+          Math.min(
+            previousVisibleItems +
+              defaultItemsVisible +
+              previousVisibleItems / defaultItemsVisible, // Scales with further scrolling


What are the benefits to scaling the batch?

When I was experimenting with scrolling quickly, the scrolling speed can increase as I scroll further down. The scaling was to prevent too many items from loading at the beginning, while preventing the stuttering that I mentioned above if the user is scrolling quickly to the bottom (which I saw more on research catalog than storybook). Maybe I could set a constant higher amount to load, though @bigfishdesign13 suggested continuing with the scaling, which I can try adapting to the IntersectionObserver.

The current configuration works well. No matter how fast I try to scroll, I never hit the bottom. I would prefer to load a constant number of options during the scrolling, but I also don't want users to hit the bottom.

The data for all of the options is always available, right? No API call to get more data from the server. So any lag that happens as the options are added is due to rendering, which could be impacted by the power of a users computer or device. If there are concerns about the scaling, let's try a static number and try to find the sweet spot.

I realized that a lot of the slowness issues I was having without scaling was because I was testing in research catalog dev mode earlier, my mistake. I removed the scaling and set the rootMargin to 300px which seems to work well enough, let me know if there are any issues with that

EdwinGuzman · 2026-07-20T16:01:30Z

 <Banner
  content={
    <>
      <strong>IMPORTANT:</strong> This prop can only be used with{" "}


Minor: update "with" to "when"?

EdwinGuzman

This is looking great. Just have a few comments and questions.

I think what's left to do is to always enable lazy loading when scrolling is selected. @bigfishdesign13 just want to make sure we agree on this; you suggested an additional boolean flag.

EdwinGuzman · 2026-07-20T16:16:28Z

+    return {
+      observe: observeMock,
+      disconnect: disconnectMock,
+      unobserve: jest.fn(),


This doesn't also need to be cleared?

EdwinGuzman · 2026-07-20T16:22:30Z

+  });
+});
+
+const triggerIntersection = () => {


Nice update. How did you catch that the mock scroll behavior was not actually working as expected?

The test was failing because no more items were being loaded on scroll - seems like the scroll handler was taking over. I also didn't realize earlier that the IntersectionObserver doesn't work in the test environment because the environment doesn't have the browser layout engine.

EdwinGuzman · 2026-07-20T16:26:33Z

+
+        setLazyItemsVisible((previousVisibleItems) =>
+          Math.min(
+            previousVisibleItems * 1.2 + lazyLoadIncrementNum,


Can you create a CONSTANT_VARIABLE for 1.2? This is to reduce magic numbers and better understand when reading this why it's set to 1.2.

EdwinGuzman · 2026-07-20T16:27:03Z

+          {
+            root: itemsListRef.current,
+            threshold: 0,
+            rootMargin: `${12 * lazyItemsVisible}px`,


Can you also create another constant var for 12?

EdwinGuzman · 2026-07-20T16:32:10Z

                  ))}
                </CheckboxGroup>
                {isOverflowExpand && <ExpandToggleButton />}
+                {isOverflowLazy &&


It makes sense why we need this, but can you add a short comment describing why? When we revisit this component in the future, it'll be clear why this is needed in the DOM.

bigfishdesign13 · 2026-07-20T16:43:29Z

I think what's left to do is to always enable lazy loading when scrolling is selected. @bigfishdesign13 just want to make sure we agree on this; you suggested an additional boolean flag.

@EdwinGuzman Yes, always enable lazy loading when scrolling is selected. Consistency. There's no need to add an additional boolean to toggle this feature on and off.

KaseyW31 added 6 commits July 9, 2026 16:50

lazy loading

6b10163

test

7deaab1

change name, changelog

3c7a344

reset modified story

6a1802f

changelog

344828f

revert other stories

71b2031

KaseyW31 mentioned this pull request Jul 10, 2026

SCC-5375: MultiSelect performance #2001

Closed

9 tasks

KaseyW31 requested review from 7emansell, EdwinGuzman and bigfishdesign13 July 10, 2026 14:17

7emansell requested changes Jul 10, 2026

View reviewed changes

KaseyW31 requested a review from 7emansell July 10, 2026 20:00

EdwinGuzman requested a review from oliviawongnyc July 13, 2026 20:22

EdwinGuzman reviewed Jul 14, 2026

View reviewed changes

Comment thread src/components/MultiSelect/MultiSelect.tsx Outdated

EdwinGuzman reviewed Jul 14, 2026

View reviewed changes

Comment thread src/components/MultiSelect/MultiSelect.tsx Outdated

KaseyW31 added 2 commits July 14, 2026 12:58

remove unused data-testid

ee5a4dc

move withLazyItems to new file

0ca1936

vercel Bot deployed to Preview July 14, 2026 17:07 View deployment

oliviawongnyc reviewed Jul 14, 2026

View reviewed changes

oliviawongnyc reviewed Jul 15, 2026

View reviewed changes

remove scroll handler; scaling lazy-load

d257c93

vercel Bot deployed to Preview July 17, 2026 15:03 View deployment

change test, rename file

5da33c4

vercel Bot deployed to Preview July 17, 2026 16:03 View deployment

EdwinGuzman reviewed Jul 20, 2026

View reviewed changes

KaseyW31 added 6 commits July 20, 2026 15:31

make lazy loading default for scroll

9babab3

clear mock

56e243e

target dom comment

5f98c62

remove scaling

0dd92ad

fix comments

88e8857

grammar

831cde6

vercel Bot deployed to Preview July 21, 2026 15:15 View deployment

KaseyW31 requested review from EdwinGuzman and oliviawongnyc July 21, 2026 15:19

Uh oh!

Conversation

KaseyW31 commented Jul 10, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR does the following:

How has this been tested?

Accessibility concerns or updates

Accessibility Checklist

Open Questions

Checklist:

Front End Review:

Uh oh!

vercel Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

7emansell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KaseyW31 commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

7emansell commented Jul 13, 2026

Uh oh!

KaseyW31 commented Jul 13, 2026

Uh oh!

EdwinGuzman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

KaseyW31 commented Jul 10, 2026 •

edited by atlassian Bot

Loading

vercel Bot commented Jul 10, 2026 •

edited

Loading

KaseyW31 commented Jul 10, 2026 •

edited

Loading