C-1958 Show # of downloaded tracks in collection card instead of actual # of tracks in collection when offline by nicoback2 · Pull Request #2757 · AudiusProject/audius-client

nicoback2 · 2023-02-01T22:13:25Z

Description

Dragons

Is there anything the reviewer should be on the lookout for? Are there any dangerous changes?

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.

How will this change be monitored?

For features that are critical or could fail silently please describe the monitoring/alerting being added.

Feature Flags

Are all new features properly feature flagged? Describe added feature flags.

nicoback2 · 2023-02-01T22:15:28Z

  )

+  const numPlayableTracksMap = useMemo(() => {
+    return buildCollectionIdsToNumPlayableTracksMap(


This solution feels very jank but it's the best I could come up with given how the code is structured, and while keeping CollectionList and its children purely presentational. Very open to other solutions if this is too cumbersome!

yeah i think this feeling is potentially resolved with the ideas below?

nicoback2 · 2023-02-01T22:19:36Z

+  const offlineDownloadStatus = useProxySelector(
+    (state: AppState) => {
+      if (isDoneLoadingFromDisk && isOfflineModeEnabled && !isReachable) {
+        return getOfflineDownloadStatus(state)


I'm using offlineDownloadStatus as the source of truth, but it seems to be very error prone right now... like it doesn't always match up with what is ultimately shown on the collection screen, which ultimately uses offlineTracks. So maybe it makes sense more sense to check offlineTracks instead of offlineDownloadStatus to be safe? On the other hand, if offlineDownloadStatus is indeed always supposed to match offlineTracks (i.e. set of tracks with status SUCCESS equals set of tracks in offlineTracks), it's fine to keep it this way. Wdyt?

@amendelsohn i think you might have some ideas here given our discussion about source of truth between cached tracks and offlineTracks?

ooh interested (also add offlineDownloadStatus to that list if we're just talking source of truth for whether a track should be shown when offline or not)

dylanjeffers

approving cause i dont want to block you @nicoback, but im personally hoping we can consider a refactor for these components in general based on the comment below, let me know what youre thinking

dylanjeffers · 2023-02-01T22:28:27Z

+    ]
  )

+  const numPlayableTracksMap = useMemo(() => {


could this just be trackCount, and just pass that into the collectionList? it seems the collection list does the manual work of extracting the num, but could be cleaner to just pass a number into the list?

@dylanjeffers well the collection list takes a list of collections, so we can't just pass a single number (since each collection has a different number of tracks). but i feel like im misunderstanding what you're saying?

Oh right for sure, yeah nvm, cause that then gets it per item

💯, pretty sure i had the same thought when writing this lol

dylanjeffers · 2023-02-01T22:29:19Z

+  const offlineDownloadStatus = useProxySelector(
+    (state: AppState) => {
+      if (isDoneLoadingFromDisk && isOfflineModeEnabled && !isReachable) {
+        return getOfflineDownloadStatus(state)


@amendelsohn i think you might have some ideas here given our discussion about source of truth between cached tracks and offlineTracks?

dylanjeffers · 2023-02-01T22:31:56Z

  const isOfflineModeEnabled = useIsOfflineModeEnabled()
  const isReachable = useSelector(getIsReachable)
  const isDoneLoadingFromDisk = useSelector(getIsDoneLoadingFromDisk)
+  const offlineDownloadStatus = useProxySelector(


based on convos with @sliptype, i think the best setup for playlistsTab would be for its only job to be getting collectionIds that we want to render, online or offline, and pass that into the card list, and then each playlist card would be in charge of getting the tracks, online or offline, which we can then derive the count etc. @nicoback curious how you feel about that as the paradigm here? would it be too much of a refactor?

it'd be a bit of a refactor just because collection card list is used in several places elsewhere in the code (like 10 places), not just the Playlists/Albums tabs. But I'm down to do it, it wouldn't be too bad. cc @sliptype

Like def agree that is the way to go lol. This code is pretty suboptimal.

Yeah agreed it would be a lot, and prob not worth scope of this ticket, thought I'd mention so we are all clear on best state for future, plus I think Sebastian might be looking into this currently?

Ooh gotcha. Yeah thinking about it tho, i think it wouldn't take too much time. @sliptype if you aren't working on this, i can pick it up first thing tomorrow and timebox to half an hour or so

I haven't gotten a chance to look at the CardList yet, got a couple of other refactor to knock out first. Yeah @nicoback definitely feel free to look at it if you want, but maybe let's see if there is anything else more pressing on the backlog first

oh yeah there are definitely more pressing things. ill leave this open and come back to it after hitting more high pri items then

[45f122f] [C-2015] Debounce the callbacks for the download toggles for favorites and collections (#2766) Kyle Shanks [e0cafef] [C-2034] Cancel all download jobs on log out (#2775) Andrew Mendelsohn [ab581b1] C-2022 C-1970 Fix for: When offline and not fully downloaded, queued jobs keep running and fail (#2773) nicoback2 [1c674a9] C-1958 Show # of downloaded tracks in collection card instead of actual # of tracks in collection when offline (#2757) nicoback2 [014afaa] [C-2036] Fix modals not closing on click outside (#2776) Marcus Pasell [07b7961] [DMs] Redo create chat modal (#2706) Marcus Pasell [8b89acf] [C-1994] Add download indicator to the track screen (#2774) Kyle Shanks [70619cd] Improve download toggle, download progress and fetch (#2772) Raymond Jacobson [860291d] [C-1941] Refresh queue when download toggle is flipped (#2770) Raymond Jacobson [3e8e33b] Fix CI (#2771) Michael Piazza [f94bcc9] [C-2012] [C-1999] [C-1734] Fix collection image loading (#2769) Sebastian Klingler [80da94c] Consume .env.git to properly tag Sentry release (#2767) Michael Piazza [b2f96f6] [C-1974] Add batch remove downloads (#2764) Dylan Jeffers [5ff30b4] [C-1921] Make sure to download local saves (#2763) Raymond Jacobson [de5d5fd] [C-2006] Fix download toggle when offline (#2762) Raymond Jacobson [66449a1] [PAY-868] Handle follow and tip send states across (#2756) Saliou Diallo [171e46e] [PAY-866] Add new remix settings - Part 1 (#2708) Saliou Diallo [90579c5] Refactor lineup hooks (#2760) Sebastian Klingler [5bfa0dc] C-2002 Un-download deleted playlists on sync (#2759) nicoback2 [1dd8b68] [C-2001] Remove queued downloads if deleted/unlisted/etc (#2761) Raymond Jacobson [265e666] [C-1968] Fix dequeue track (#2758) Dylan Jeffers [fbaf5c0] [C-1992] Fix unfavorite downloaded track (#2755) Dylan Jeffers [73563f8] [C-1957] [C-1929] Improve TrackList and offline mode performance (#2753) Sebastian Klingler [b771776] [C-1987][C-1936][C-2009] Queue for collection downloads (#2751) Andrew Mendelsohn [016342d] [C-1985] Remove collection tracks when removing favorites (#2749) Dylan Jeffers [5b675c8] [PAY-881] Mobile messages (chats) list screen (#2742) Reed [d2b1c4c] C-1989 Fix "Download is not re-starting even though there are still many tracks that need to be downloaded" (#2746) nicoback2 [fec0134] [PAY-872][PAY-873][PAY-874] More premium content QA fixes (#2741) Saliou Diallo [9530cae] Hide buttons on profiles when offline and remove timeout (#2750) Raymond Jacobson [d658e3e] [C-1952] Show download successful when offline in status indicator (#2733) Raymond Jacobson [bbb58e8] [C-1950] Smooth offline<>online playback (#2748) Raymond Jacobson [059f79e] Hide share/repost when offline C-1942 (#2747) nicoback2 [6458512] [C-2000] Fix collections not downloading on favorites download (#2745) Dylan Jeffers [4ba882c] Do not write subscribe to discovery on auto-subscribe after follow (#2562) Michelle Brier [e21edc7] Fix download counter in error state (#2744) Raymond Jacobson [3ad7a55] [PAY-777] Add premium content track upload on mobile - Part 3 (#2721) Saliou Diallo [ad3b6d2] Update undownload favorites to hide progress earlier (#2743) Kyle Shanks [16a45f5] [C-1938] Remove tracks within collections when toggling off favorites (#2737) Raymond Jacobson [5e008a3] [C-1977] Hide more by this artist when offline (#2734) Raymond Jacobson [b1d261f] Improve useFetchAllFavoritedTracks fetch (#2740) Dylan Jeffers [a39f874] [C-1993 C-1995] Refactor download switch, improve collection header (#2739) Dylan Jeffers [36687e5] [C-1937, C-1964] Add pagination to the mobile favorites screen and fix the title width for deleted track list items (#2738) Kyle Shanks [adc74fa] [C-1976] Hide/disable overflow menus when offline (#2735) Raymond Jacobson [18e63ff] [C-1938] Disable downloading from toggle when offline (#2736) Raymond Jacobson [a0d3097] [C-1991] Fix downloading rive animation location (#2732) Raymond Jacobson [1e8d9d5] [C-1960] Do not show favorite/repost on own (#2723) Raymond Jacobson [563842e] [C-1930] Prevent self favorite/repost/follow (#2724) Raymond Jacobson [4975945] [C-1916 C-1982 QA-328] Fix smart-collection artwork (#2731) Dylan Jeffers [bbc2477] [C-1963] Add download progress bar (#2730) Raymond Jacobson [5ef3d06] [C-1782] Use proxy for sentry dsn (#2684) Raymond Jacobson [2c2cd82] [C-1962] Update offline icons/animations (#2725) Raymond Jacobson [8607f9c] Fix crash on favorites screen if you start the app offline C-1943 C-1951 (#2726) nicoback2 [74c9324] [C-1986] Use RNFetchBlob for offline mode (#2728) Sebastian Klingler [3084ea9] Refactor ArtistChip (#2715) Marcus Pasell [7a9cec6] [C-1973] Improve offline image downloading (#2720) Sebastian Klingler [075aa5b] Fix some web premium content bugs (#2709) Saliou Diallo

nicoback2 requested review from a team, amendelsohn and dylanjeffers February 1, 2023 22:13

pull-request-size Bot added the size/L label Feb 1, 2023

nicoback2 commented Feb 1, 2023

View reviewed changes

dylanjeffers approved these changes Feb 1, 2023

View reviewed changes

all

e082fb5

nicoback2 force-pushed the nkang--playlists-count branch from 96d9aa9 to e082fb5 Compare February 1, 2023 23:35

nicoback2 merged commit 1c674a9 into main Feb 3, 2023

nicoback2 deleted the nkang--playlists-count branch February 3, 2023 22:56

AudiusProject deleted a comment from linear Bot Sep 11, 2023

Uh oh!

Conversation

nicoback2 commented Feb 1, 2023

Description

Dragons

How Has This Been Tested?

How will this change be monitored?

Feature Flags

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!

dylanjeffers left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dylanjeffers Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dylanjeffers left a comment •

edited

Loading

dylanjeffers Feb 1, 2023 •

edited

Loading