Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virtualized list with react window & throttled the request to prevent 429 errors // add list counts to list view #255

Merged
merged 6 commits into from
Jan 26, 2025

Conversation

tb38r
Copy link
Contributor

@tb38r tb38r commented Jan 6, 2025

@noahm , addressed the virtualisation suggestion by implementing React Windows, only a subset of the lists are rendered at a time now. Additionally, used lodash's throttling functionality to minimise the frequency of the requests as it'd overload the servers.

Both appear fine, however, an issue remains in that the value returned useListSize is always '1'. That wasn't always the case as I'd initially tested the endpoint provided by @thieflord06 on Postman and would get varied results depending on the url provided however whether via Postman, or the app itself, it now always returns 1. Unsure why that may be, any suggestions?

Screenshot 2025-01-06 at 17 28 55

Copy link

vercel bot commented Jan 6, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
clearsky-ui ❌ Failed (Inspect) Jan 10, 2025 5:35pm

@thieflord06
Copy link
Member

I will look into why the results have changed.

@thieflord06
Copy link
Member

This is fixed now @tb38r

@thieflord06
Copy link
Member

thieflord06 commented Jan 6, 2025

@tb38r We need to find a consistent spot for the list count number. Also, there is a view within the view.

Desktop:
image

Mobile:
image

@tb38r
Copy link
Contributor Author

tb38r commented Jan 6, 2025

count now returns 150 on my end but your screenshots are closer to what's expected so I'll continue on the assumption that it might be a local issue.

will work on fixing the view within a view issue as well as applying some consistency amongst different the screen sizes.

@thieflord06 thieflord06 requested a review from noahm January 6, 2025 18:14
@thieflord06 thieflord06 linked an issue Jan 6, 2025 that may be closed by this pull request
@thieflord06
Copy link
Member

thieflord06 commented Jan 6, 2025

Once we get the visuals sorted out we can verify the count logic. Staging is significantly behind prod so if you're comparing counts from in the bsky app, they will definitely be off.

I'm a little concerned about how many even numbered list counts there are (100, 50, 150, etc.) but I think people are also using tools to create lists so that kind of makes sense.

@noahm
Copy link
Collaborator

noahm commented Jan 6, 2025

Off to a good start, but there's a few issues to fix. One is the double nested scroll areas which you're already aware of. Another is the fact that this has removed the behavior of loading additional pages of data once you scroll near to the bottom of the existing data.

@thieflord06 thieflord06 changed the title virtualized list with react window & throttled the request to prevent 429 errors virtualized list with react window & throttled the request to prevent 429 errors // add list counts to list view Jan 8, 2025
@thieflord06
Copy link
Member

This is shaping up good. There are some spacing issues with larger numbers.

Screenshot_20250110_051243_Chrome.jpg

Screenshot_20250110_051340_Chrome.jpg

@thieflord06
Copy link
Member

Build failed

    at Module._compile (node:internal/modules/cjs/loader:1565:14)
    at Object..js (node:internal/modules/cjs/loader:1708:10)
    at Module.load (node:internal/modules/cjs/loader:1318:32)
    at Function._load (node:internal/modules/cjs/loader:1128:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:219:24)
    at cjsLoader (node:internal/modules/esm/translators:263:5)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:196:7) {
  [cause]: Error: Cannot find module '@rollup/rollup-linux-x64-gnu'
  Require stack:
  - /vercel/path0/node_modules/rollup/dist/native.js
      at Function._resolveFilename (node:internal/modules/cjs/loader:1249:15)
      at Function._load (node:internal/modules/cjs/loader:1075:27)
      at TracingChannel.traceSync (node:diagnostics_channel:322:14)
      at wrapModuleLoad (node:internal/modules/cjs/loader:219:24)
      at Module.require (node:internal/modules/cjs/loader:1340:12)
      at require (node:internal/modules/helpers:138:16)
      at requireWithFriendlyError (/vercel/path0/node_modules/rollup/dist/native.js:46:10)
      at Object.<anonymous> (/vercel/path0/node_modules/rollup/dist/native.js:73:76)
      at Module._compile (node:internal/modules/cjs/loader:1565:14)
      at Object..js (node:internal/modules/cjs/loader:1708:10) {
    code: 'MODULE_NOT_FOUND',
    requireStack: [ '/vercel/path0/node_modules/rollup/dist/native.js' ]
  }
}
Node.js v22.12.0
Error: Command "npm run build" exited with 1

@thieflord06
Copy link
Member

I think if we moved the list description to a new line and wrap the description that will allow for the list count to stay in the same spot and there won't be any spacing issues.

Being that there is a ticket out for this IDK if you want to do this in this PR or make a new one @tb38r

reference: #262

@tb38r
Copy link
Contributor Author

tb38r commented Jan 10, 2025

Hey @thieflord06, apologies for going ghost, been tied up with various things as well as getting this issue done.

In trying to implement the windowing feature, the initial attempt via react-window added an additional scrollbar as previously highlighted where it'd be preferred to manage the browsing behaviour directly via the window. After a bit of research, found a comment from the package's creator noting it wouldn't be possible to do so with that specific package, was working perfectly otherwise - with the loading additional pages behaviour as well.

Found this --> https://tanstack.com/virtual/latest/docs/framework/react/examples/window which seemed perfect for the job. Spent some time trying to implement it but I'm possibly missing something minor to get it working perfectly

Screenshot 2025-01-10 at 16 57 06

note the whitespace below, where the new data should be on scroll

const rowVirtualizer = useWindowVirtualizer({
count: filteredLists?.length,
estimateSize: () => 54,
overscan: 20,
gap: 50,
scrollMargin: listRef.current ? listRef.current.offsetTop : 0,
});

The result of --> rowVirtualizer.getVirtualItems().length represents the number of the items being rendered initially, working on figuring out why the rest of the data isn't showing, as I say, hoping finding solution involves a minor oversight on my end.

@tb38r
Copy link
Contributor Author

tb38r commented Jan 10, 2025

branched off from list-counts, pushed updated work for review (branch 'window-throttling')

@thieflord06
Copy link
Member

@tb38r No worries. I was just letting you know that your build is failing for some reason. maybe @noahm can give guidance on why.

I thought we may get this into the next release but it's ok if it doesn't make it because of the needed tweaks.

@noahm
Copy link
Collaborator

noahm commented Jan 11, 2025

Your build issues are probably being caused by some strange changes I'm seeing in the package-lock.json file. If you revert that file to what is currently on dev branch, then run npm install, then commit the final state of the lockfile I expect things would be back into a good state.

@thieflord06
Copy link
Member

@tb38r

#264 may affect your efforts.

@tb38r
Copy link
Contributor Author

tb38r commented Jan 14, 2025

noted!

@tb38r tb38r mentioned this pull request Jan 14, 2025
thieflord06 added a commit that referenced this pull request Jan 26, 2025
Withheld working on the virtualisation element to get a viable working
product out.
Throttled getListSize to prevent server overload.
Updated with css changes from #264.
List count appears to the right of of the list name.
resolves #255.

![Screenshot 2025-01-14 at 17 38
38](https://github.com/user-attachments/assets/bf70d00d-5c2a-4861-92b6-110a9ca71258)
@thieflord06 thieflord06 merged commit b0c953e into ClearskyApp06:list-counts Jan 26, 2025
1 of 2 checks passed
@thieflord06
Copy link
Member

@tb38r You'll have to make a new PR for this if you want to keep working on it.

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.

Display list count
3 participants