fix(site): maintain initial workspace list order #7590
Conversation
| if (fetchedWorkspaces) { | ||
| if (displayedWorkspaces.length === 0) { | ||
| setDisplayedWorkspaces(fetchedWorkspaces) | ||
| } else { |
There was a problem hiding this comment.
nit: I have a feeling that this can be refactored into more like a fluent approach.
| fetchedWorkspaces.forEach((fetchedItem) => { | ||
| if (!mergedItems.some((item) => item?.id === fetchedItem.id)) { | ||
| mergedItems.unshift(fetchedItem) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Ok, I understand the "hack", but I'm curious how it behaves while changing pages or playing with the filter bar.
| // If the fetched item already exists, update its data without changing its position | ||
| return { ...item, ...fetchedItem } | ||
| }) | ||
| .filter((item) => item !== null) // Remove the removed items (null values) |
There was a problem hiding this comment.
nit: "Remove the removed" sounds weird :)
| if (displayedWorkspaces.length === 0) { | ||
| setDisplayedWorkspaces(fetchedWorkspaces) | ||
| } else { | ||
| // Merge the fetched workspaces with the displayed onws, without changing the order of the existing items |
|
I'm not sure how this would fix the issue since the BE paginate the results so the order only would work per page which is strange IMO. For me, the fix on FE looks very fragile. |
|
It would be valuable to pull @kylecarbs into this chat. It looks like we're fixing "regression" (not exactly regression) introduced in #6984. |
@BrunoQuaresma Yeah, what we want here is to remove the flickering of workspaces reordering every 5 seconds because the |
There was a problem hiding this comment.
Couple of things:
-
Munging the list is a bit of a hacky workaround
-
I'd be happier if the sorting logic could be extracted to a pure function, but this doesn't seem easy with
useEffect(). -
The original issue does not really change the sorting logic as far as I can tell.
The actual issue as far as I can see is that each actively-used workspaces has its last_used_at continuously updated. Because we sort by last_used_at by default, this means that for looking at workspaces belonging to a single u ser, the ordering issue is less noticeable (users will probably only use one space at a time). However, showing active workspaces from all users (or all workspaces) will definitely result in this ordering behaviour.
What if, instead, we modify the ordering on the backend to order by user_id first (guaranteed stable ordering) and last_used_at second (not stable, but is likely to remain so with the ordering)?
That way, we don't need a complicated frontend hack. There may be some bouncing if a single user has multiple active workspaces.
Alternatively, we order by created_at, end of story.
|
I see but we can properly fix that by just changing the sorting in the BE to not use the "latest used at" field because it changes very frequently. IMO, we just sort by name which is very predictable and does not change frequently. |
|
I am in favor of showing the last used at first because if I just create a workspace I would not expect to go to other pages (or scrolling) to find it in the workspace list. But I agree that for admins (who I believe would like to watch the list for all workspaces more often), the current order can be bothersome. IMO the ranking of possible solutions (from best to worst) would be: |
|
Thanks for the summary, Rodrigo. I wouldn't spend much time debating on options. It looks like option 1. has the majority :) Would you mind jumping into backend area? |
@mtojek getting my backend gloves on. brb with a PR :) |
addresses the issue of annoying visual reorders in the workspace list by maintaining the initial order of workspaces fetched from the backend while handling updates, additions, and removals of workspaces in the list.
Changes:
displayedWorkspacesto store the workspaces to be displayed on the screen.useEffecthook to update thedisplayedWorkspacesstate based on the latest data fetched from the backend.I usually try to avoid using
useEffectas much as I can, but I believe this is an ok use-case for it.