[PAY-1202] Refactor saved collections fetching by schottra · Pull Request #3337 · AudiusProject/audius-client

schottra · 2023-05-04T21:34:10Z

Description

This is the first round of changes to improve the saved albums/playlists experience. This PR focuses on updates to the fetching logic.
Previously, the fetching was done in account sagas (fetchSavedAlbumsAsync and fetchSavedPlaylistsAsync), and all records were fetched in one API call. This breaks for more than about 80 ids passed. We were also using a large provider to fetch the data and then prop-drilling it down to SavedPage and its children.

Changes

Added new slice, selectors, and saga specifically for fetching saved collections. The saga includes a helper that will break lists of collections to be fetched into smaller chunks if they exceed a conservative limit (50) that might cause an issue with the API
Updated all places using retrieveCollections directly to to use the new saga helper.
Added hooks for fetching saved albums and playlists(TODO). Implemented a saved albums hook which allows for fetchMore style pagination of full playlist metadata. It uses derived selectors to determine which playlists from the fully fetched album id list do not have local metadata. fetchMore will collect the first page_size ids and initiate a fetch for them. This will be used in future work to implement an InfiniteScroll component. (Note: For now, we are using the new hook with a page size of 9999, which has the same effect as the previous "load everything at once" behavior)
Removed albums-related logic from SavedPageProvider
Refactored album rendering code in web to follow the mobile patterns
- Added content components for displaying saved albums which use the new hook to fetch the data instead of consuming it through props
- Moved logic for album cards into separate components
- Removed a lot of prop drilling for utilities which could be consumed directly through hooks or constants
Added useGoToRoute hook for components to consume if they need to push routes. We don't need to pass this down through props.
Fixed some propType warnings with card components

There are some follow-up issues that I will address in future PRs, but nothing that wasn't already broken :-)

Dragons

This does change fetching logic on both mobile and web, though it should be in a way that doesn't break anything for users which were not already broken (i.e. users who had too many playlists to fetch safely)

How Has This Been Tested?

Tested on desktop, mobile web, and local emulator to ensure playlists and albums still load correctly.

How will this change be monitored?

Will test as part of client release process. Will monitor sentry and rollback if any major issues occur.

Feature Flags

N/A

audius-infra · 2023-05-04T21:51:57Z

Preview this change https://demo.audius.co/pay-1202-refactor-saved-collections

audius-infra · 2023-05-05T20:33:52Z

Preview this change https://demo.audius.co/pay-1202-refactor-saved-collections

sliptype

Awesome work! All looks good to me

sliptype · 2023-05-15T16:19:52Z

+  const dispatch = useDispatch()
+  const [hasFetched, setHasFetched] = useState(false)
+  const { unfetched: unfetchedAlbums, fetched: albumsWithDetails } =
+    useSelector(getAlbumsWithDetails)


The return value isn't really implied by the name getAlbumsWithDetails, is there a better name for this selector?

I can update the selector name and add a comment

sliptype · 2023-05-15T16:21:44Z

+}
+
+/* TODO: Handle filtering
+ * Option 1: This hook takes the list of album ids to fetch and computes the unfetched


Option 1 seems good to me

piazzatron

Very clean and easy to review! Only nits :)

piazzatron · 2023-05-15T19:05:22Z

  tracks: TracksCacheState
  users: UsersCacheState

+  savedCollections: SavedCollectionsState


nit: IMO a bit cleaner to use the ReturnType of the reducer here, and then we don't have to export the state

I believe I did try to do this originally, but the value returned from ReturnType had some issues that made it a problem to consume in the calling code (I think a few fields were incorrectly marked as possibly null or any?)

Huh, nevermind I guess It works just fine!

piazzatron · 2023-05-15T19:09:04Z

We've tended to colocate action types in their corresponding slice in the past – this is nice too but would opt for consistency one way or the other (cc @sliptype)

I was using notifications as the most recent example, which puts types in a separate file.

notifications is a bit of a special case since there are so many notification types - from a few weeks ago, looks like are keeping action types in the same file for the AI attributed slice: https://github.com/AudiusProject/audius-client/blob/main/packages/common/src/store/pages/ai/slice.ts

Got it. The other complicating factor here is that there are types shared between the slice and selectors module and I don't really want to import selectors just to get at the types.
Historically, in TS, as soon as a type is used in multiple places I pull it out into a types file to remove the future possibility of circular dependencies.

So I can move just the action types if that's the pattern we are going for. But we will still have the types file.

piazzatron · 2023-05-15T19:15:36Z

+  // Request ids in chunks if we're asked for too many
+  const chunks = chunk(ids, COLLECTIONS_BATCH_LIMIT)
+  for (let i = 0; i < chunks.length; i += 1) {
+    yield call(retrieveCollections, null, chunks[i])


I wonder if we should rename/deprecate retrieveCollections or add a comment indicating that it doesn't paginate and that future callers should hit fetchCollectionChunks instead?

That's not a bad idea. At the very least, I can add a comment to retrieveCollections stating that it's not appropriate for fetching large numbers of collections. Or... I suppose I could have just updated retrieveCollections to do the chunking...

Updated to do this in retrieveCollections. Turns out we were already doing chunking for track fetching! So I updated both patterns to match.

piazzatron · 2023-05-15T19:16:49Z

-  currentUserReposted: PropTypes.boolean,
-  currentUserSaved: PropTypes.boolean,
+  currentUserReposted: PropTypes.bool,
+  currentUserSaved: PropTypes.bool,


ty for this 😍

piazzatron · 2023-05-15T19:17:46Z

+import { push as pushRoute } from 'connected-react-router'
+import { useDispatch } from 'react-redux'
+
+export function useGoToRoute() {


piazzatron · 2023-05-15T19:20:24Z

+      primaryText='You haven’t favorited any albums yet.'
+      secondaryText='Once you have, this is where you’ll find them!'
+      buttonLabel='Go to Trending'


nit: could pull these out into a messages object as campground cleaning (this file might have been around prior to that convention!)

Oh yep. Definitely just moved code around without looking at it here. Will put into a messages object.

piazzatron · 2023-05-15T19:21:37Z

+  album: CollectionWithOwner
+}
+
+const AlbumCard = ({ album, index, isLoading, setDidLoad }: AlbumCardProps) => {


Good looks pulling these out into standalone components

piazzatron · 2023-05-15T19:25:40Z

+      return
+    }
+    const ids = unfetchedAlbums
+      .slice(0, Math.min(pageSize, unfetchedAlbums.length))


Doesn't the saga layer already chunk this via your fetchCollectionChunks fn?

Yes, but there are two layers. This is intended for future use where we are calling fetchMore here and it's fetching the next X album records using the underlying saga. This has a configurable page size based on how many you want to fetch at once in a particular component.
The saga has no notion of page sizes. It's just a bulk fetch that guards against 400s by chunking up the list if you ask for too many at once.
For most purposes, the pageSize here will be much less than the chunk size in the saga. (i.e. we shoudn't ever trigger the saga's chunking.

makes sense!

piazzatron · 2023-05-15T19:34:46Z

Could we lose getFilteredAlbums?

Oh yep. Good catch. I had left it until I decided what to do about filtering. But it won't be used either way.

audius-infra · 2023-05-15T20:21:46Z

Preview this change https://demo.audius.co/pay-1202-refactor-saved-collections

schottra · 2023-05-15T22:49:37Z

@piazzatron @sliptype Mind looking at these two changes as well?
5a48356
2863089

audius-infra · 2023-05-15T23:10:52Z

Preview this change https://demo.audius.co/pay-1202-refactor-saved-collections

audius-infra · 2023-05-16T16:05:03Z

Preview this change https://demo.audius.co/pay-1202-refactor-saved-collections

[9855fd2] Update locks (#3397) Dylan Jeffers [aecef5f] [PAY-1144] [PAY-1182] [PAY-1147] DMs: Delete chat, message permissions (#3390) Marcus Pasell [64ccd9d] Add stylelint to ci (#3373) Dylan Jeffers [b642a16] [C-2518, C-2523, C-2611] Improve playlist create sagas (#3378) Dylan Jeffers [5620d53] [PAY-1197] Mobile inbox unavailable modal from profile screen (#3376) Reed [2ebaef5] [PAY-1248] Initial changes to get ready for upcoming PRs that include track and playlist tiles in DMs (#3391) Saliou Diallo [c2fabf6] [PAY-1218] Mobile block dms drawer switches block/unblock (#3387) Reed [0acad29] [C-2575] Match length of related artists user list to preview (#3395) Andrew Mendelsohn [40525ae] [C-2615] Fix favorite tracks error due to empty track entries on web mobile (#3386) Kyle Shanks [93a8077] [PAY-1145] DMs: Add InboxUnavailableModal (#3369) Marcus Pasell [633484d] Update edit playlist flow and components (#3361) Kyle Shanks [334ea0d] Special case ios safari for stem download (#3385) Raymond Jacobson [d38238d] [C-2607] Pagination wrapper hooks for audius-query (#3375) Andrew Mendelsohn [0a332d8] Hotfix: Fix stems downloads on mobile web (#3382) Marcus Pasell [81da65e] Clean up artist_pick_track_id in APIUser (#3381) Michelle Brier [64d89ef] Fix lint (#3380) Raymond Jacobson [59129b4] [C-2614] Fix download stems mobile web (#3379) Raymond Jacobson [df68727] [PAY-1032][PAY-892] Mobile DMs unread indicator, prefetch chats (#3352) Reed [4a00fad] [PAY-1151] Handle chat reactions near top of screen on mobile (#3370) Reed [bff316f] [PAY-1139] Throttle calls to fetchMessages on web scroll (#3372) Michael Piazza [e723cec] C-2483 Fix queue overshot empty track player bug (#3353) nicoback2 [472a41d] [C-2596] Add disabled option to audius-query hooks (#3367) Andrew Mendelsohn [f518962] [C-2602] Improve playlist library layout (#3364) Dylan Jeffers [b3db8fa] Fix debounce on notif reaction (#3362) Raymond Jacobson [2104b2b] [PAY-1183] Make clicking ChatUser handle/displayname lead to profile (#3368) Michael Piazza [21cb6fc] DMs: Fix click handler in search user list for users you can't chat (#3358) Marcus Pasell [24e7d8a] DMs: Update copy, scroll inbox-settings modal (#3359) Marcus Pasell [e5e8d90] [PAY-941] Fix "1 new messages" unread tag (web) (#3366) Michael Piazza [cfa1b2a] [C-2603] Fix readonly object error in audius-query reducer (#3365) Andrew Mendelsohn [978c993] Fix invite reward claimable state on mobile (#3363) Reed [a174fcc] [C-2556, C-2557] Address AI Attribution QA (#3349) Dylan Jeffers [3c9b0f1] [PAY-1202] Refactor saved collections fetching (#3337) Randy Schott [a0bdad5] Get call to action for chat permissions (#3325) Marcus Pasell [03a2721] DMs: Use the optimistic unread count if applicable (#3354) Marcus Pasell [8b99c2b] [C-2550] Left-nav fixes and improvements (#3357) Dylan Jeffers [e7b0aab] [PAY-1215] Fix create new message crash (#3356) Reed [6e7ece9] [PAY-1196] Mobile dms search users empty state (#3355) Reed [315ae4f] [PAY-1219] Fix mobile chat reactions popup message getting cut off (#3342) Reed

pull-request-size Bot added the size/L label May 4, 2023

schottra marked this pull request as draft May 4, 2023 21:34

pull-request-size Bot added size/XL and removed size/L labels May 5, 2023

schottra marked this pull request as ready for review May 5, 2023 16:19

schottra requested review from dharit-tan, piazzatron and rickyrombo May 5, 2023 20:00

schottra assigned dylanjeffers and sliptype May 5, 2023

sliptype approved these changes May 15, 2023

View reviewed changes

piazzatron approved these changes May 15, 2023

View reviewed changes

schottra added 12 commits May 16, 2023 09:52

add scaffolding for new state

8bedd92

simpler data fetching

e8d031e

getting albums rendering again, without pagination

570cc55

actually include the new sagas :-)

32093f5

fix React warnings during card rendering

c098e33

update albums list to render as it previously did, with batching

331991d

get mobile up and running again with new fetching

111c2c1

update other usages or retrieveCollection and revert setDidLoad changes

f012524

cleanup for PR

eb42091

remove comments

69a91bd

fix some issues with artwork callbacks

a84bf26

PR feedback and moving chunking to retrieveCollections

437e164

schottra force-pushed the pay-1202-refactor-saved-collections branch from 2863089 to 437e164 Compare May 16, 2023 14:18

sliptype approved these changes May 16, 2023

View reviewed changes

change yield style to avoid crashes on mobile

2a97f6a

schottra merged commit 3c9b0f1 into main May 16, 2023

schottra deleted the pay-1202-refactor-saved-collections branch May 16, 2023 16:16

AudiusProject deleted a comment from linear Bot Sep 11, 2023

Uh oh!

Conversation

schottra commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dragons

How Has This Been Tested?

How will this change be monitored?

Feature Flags

Uh oh!

audius-infra commented May 4, 2023

Uh oh!

audius-infra commented May 5, 2023

Uh oh!

sliptype 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!

piazzatron 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!

audius-infra commented May 15, 2023

Uh oh!

schottra commented May 15, 2023

Uh oh!

audius-infra commented May 15, 2023

Uh oh!

audius-infra commented May 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

schottra commented May 4, 2023 •

edited

Loading