Add virtualization in sites table #1054

K-Markopoulos · 2024-11-18T13:34:32Z

The goal is to improve the performance of the sites table in /map route, by adding virtualization. Currently we do not paginate the sites and when we fetch a few thousands sites, the UI is slow and may freeze.

Add react-window package which handles virtualization.
Add VirtualTable component "forked" from this sandbox, as react-window only works for lists and we need to make it work with tables.
Rename SiteTableBody -> SiteTable, it now renders both header (received from props) and body.
Rename SiteTable -> SiteTableContainer

pullrequest

✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:

Comments posted to any areas of potential concern or improvement.
Detailed feedback or actions needed to resolve issues that are found.
Turnaround times vary, but we aim to be swift.

@K-Markopoulos you can click here to see the review status or cancel the code review job.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 463
- 322

55% TSX
44% Jest Snapshot (tests)
1% TSX (tests)
<1% JSON
<1% TypeScript

Generated lines of change

+ 27
- 0

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

github-actions · 2024-11-18T13:39:22Z

Build succeeded and deployed at https://aqualink-app-1054.surge.sh
(hash 48b3707 deployed at 2024-11-19T16:19:56)

ericboucher · 2024-11-18T17:19:15Z

Nice work!

I noticed a small visual glitch when scrolling down, the header moves slightly creating a visual glitch
clicking on a reef in the table is super slow on the staging env, but this might be due to the high number of points on the map and not the table? If it's the table then we should investigate.
Sorry I didn't think of that earlier but another option would be to use pagination instead. Do you have any thoughts on pagination vs virtualization in this case? Noting that a short-term goal is to put filters more at the center of the usage of the map "à la" Airbnb.

pullrequest

Nice work, no merge blocking issues found. Well commented code. Added a couple comments about possible (minor) issues.

Frank T

Reviewed with ❤️ by PullRequest

pullrequest · 2024-11-18T19:25:05Z

packages/website/src/common/VirtualTable/index.tsx

+
+/**
+ * Displays a virtualized list as table with optional header and footer.
+ * It basically accepts all of the same params as the original FixedSizeList.


Nice comment here with reference link

◽ Compliment

Frank T

pullrequest · 2024-11-18T19:25:05Z

packages/website/src/routes/HomeMap/SiteTableContainer/table.tsx

  const handleClick = (event: unknown, site: Row) => {
-    setSelectedRow(site.tableData.id);
+    const mapElement = document.getElementById('sites-map');


nit - It is fine as is, but we could grab the mapElement after the dispatch calls.

🔹 Best Practices (Nice to have)

Frank T

Indeed, I'm making the change.

pullrequest · 2024-11-18T19:25:05Z

packages/website/src/routes/HomeMap/SiteTableContainer/table.tsx

@@ -288,28 +336,30 @@
      },
    },
    tableRow: {
+      position: 'static !important' as 'static',
      cursor: 'pointer',
      borderTop: `1px solid ${theme.palette.grey['300']}`,
    },
  });

 type SiteTableBodyIncomingProps = {


Should this be renamed from SiteTableBodyIncomingProps to SiteTableIncomingProps?

🔹 Naming (Nice to have)

Frank T

Right, nice catch 🎣 !

K-Markopoulos · 2024-11-19T12:16:52Z

@ericboucher

The visual glitch is unfortunately present because the items are absolute positioned on scroll. At first look I had not luck mitigating this issue, we will need to investigate it further.
The slowness is most likely because of the map, if there is no interaction with the map the page and the table is very fast. I tried to remove the map and the item clicking does cause any issue.
Pagination (server side) seems like a better solution to avoid fetching thousands of items. Virtualization is a quicker solution to avoid rendering all of them, if we are fetching thousands, let's not render them all at once. But adding pagination here might be tricky because the paginated items have to be in sync with the visible items in the map. There are some edge cases we need to figure out like, what happens when the map is completely zoomed out, if the map is fetching all items to display them then they are not paginated and we don't have a specific page to show. It can be achieved but it needs more thought.

pullrequest

PullRequest reviewed the updates made to #1054 up until the latest commit (48b3707). No further issues were found.

Reviewed by:

Frank T

pullrequest

✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:

Comments posted to any areas of potential concern or improvement.
Detailed feedback or actions needed to resolve issues that are found.
Turnaround times vary, but we aim to be swift.

@K-Markopoulos you can click here to see the review status or cancel the code review job.

pullrequest

✖️ This code review was cancelled. See Details

ericboucher · 2024-12-04T20:14:04Z

Closing for now in favor of pagination but let's keep this in mind !

K-Markopoulos added 2 commits November 18, 2024 14:47

Add virtualization in sites table

78e7d88

Fix scrolling issue

9dfca99

K-Markopoulos requested a review from ericboucher November 18, 2024 13:34

K-Markopoulos self-assigned this Nov 18, 2024

pullrequest bot reviewed Nov 18, 2024

View reviewed changes

Merge branch 'master' into sites-list-virtualization

f4c5a9f

pullrequest bot reviewed Nov 18, 2024

View reviewed changes

K-Markopoulos added 2 commits November 19, 2024 14:19

Move map query after dispatch

becd0e2

Rename props

48b3707

pullrequest bot reviewed Nov 19, 2024

View reviewed changes

ericboucher closed this Nov 19, 2024

ericboucher reopened this Nov 19, 2024

pullrequest bot reviewed Nov 19, 2024

View reviewed changes

ericboucher mentioned this pull request Nov 20, 2024

Add pagination to table #1058

Merged

ericboucher closed this Dec 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add virtualization in sites table #1054

Add virtualization in sites table #1054

K-Markopoulos commented Nov 18, 2024

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

ericboucher commented Nov 18, 2024

pullrequest bot left a comment

pullrequest bot Nov 18, 2024

pullrequest bot Nov 18, 2024

K-Markopoulos Nov 19, 2024

pullrequest bot Nov 18, 2024

K-Markopoulos Nov 19, 2024

K-Markopoulos commented Nov 19, 2024 •

edited

Loading

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

ericboucher commented Dec 4, 2024

Add virtualization in sites table #1054

Add virtualization in sites table #1054

Conversation

K-Markopoulos commented Nov 18, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited Loading

Choose a reason for hiding this comment

PullRequest Breakdown

github-actions bot commented Nov 18, 2024 • edited Loading

ericboucher commented Nov 18, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Nov 18, 2024

Choose a reason for hiding this comment

◽ Compliment

pullrequest bot Nov 18, 2024

Choose a reason for hiding this comment

🔹 Best Practices (Nice to have)

K-Markopoulos Nov 19, 2024

Choose a reason for hiding this comment

pullrequest bot Nov 18, 2024

Choose a reason for hiding this comment

🔹 Naming (Nice to have)

K-Markopoulos Nov 19, 2024

Choose a reason for hiding this comment

K-Markopoulos commented Nov 19, 2024 • edited Loading

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

ericboucher commented Dec 4, 2024

pullrequest bot left a comment •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

K-Markopoulos commented Nov 19, 2024 •

edited

Loading