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

Fix offline collections not loading if started offline C-3187 #6245

Merged
merged 3 commits into from
Oct 6, 2023

Conversation

nicoback2
Copy link
Contributor

Description

Before:
If you started app session offline, your offline collections would not load

After:
Fixed

How Has This Been Tested?

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

@@ -117,38 +117,34 @@ export const useCollectionsScreenData = ({
return []
}

return fetchedCollectionIds.filter((collectionId) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the library work, fetchedCollectionIds was always populated even when offline bc we were using the collections value in the account object. Now, fetchedCollectionIds always comes from API (since we can't rely on the account object collections, since they only include the user's Favorites), so it'll be empty if we start the session offline. So to fix the issue we can just gather the downloaded collection IDS from the offlineCollectionStatus data in the store.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this change and makes a ton of sense

Copy link
Contributor

@dylanjeffers dylanjeffers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff thank you for the quick fix. Next plane ride I'll have my things ❤️

@@ -117,38 +117,34 @@ export const useCollectionsScreenData = ({
return []
}

return fetchedCollectionIds.filter((collectionId) => {
const offlineCollectionsStatus = getOfflineCollectionsStatus(state)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to remember which parts of state here might be best to select from. When we do the initial step where we load the offline data into the store, what do we use to determine which are avail? Is it just a crawl through the directory or do we use the status like you are using here? I wonder if there is a way to add a selector to kinda say "get downloaded collections"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely could roll this into a complex selector per the pattern we adopted during offline mode.

return fetchedCollectionIds.filter((collectionId) => {
const offlineCollectionsStatus = getOfflineCollectionsStatus(state)
const offlineCollectionIds = Object.keys(offlineCollectionsStatus).filter(
(k) => offlineCollectionsStatus[k] !== OfflineDownloadStatus.INACTIVE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can there be error state?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, collections can have an error state. Keep in mind that this status is only for the collection metadata, not necessarily its tracks' download status.

I would say given that here we are offline, we should filter down to only success status. Safest to err on the side of not attempting to display incomplete collections.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked Spotify and they show albums that are not fully downloaded so maybe this is okay?

Copy link
Contributor

@amendelsohn amendelsohn Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, though we still only want to show them if we have the full collection metadata downloaded (name, cover art, owner, etc.)

Collection download success is only an indicator of the collection metadata being downloaded. This is separate from the status we display on the indicator icon which is an aggregate of the collection's download status and the download status of each track in its contents.

Which is to say we should display collections if:

collection_download_status === SUCCESS &&
(downloaded_collection_tracks.length > 0 || collection_tracks.length === 0)

I believe the latter half is the logic we have below, and the only change we need is to use === SUCCESS instead of !== INACTIVE

Copy link
Contributor

@amendelsohn amendelsohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. Seems like the right fix to me!

@@ -117,38 +117,34 @@ export const useCollectionsScreenData = ({
return []
}

return fetchedCollectionIds.filter((collectionId) => {
const offlineCollectionsStatus = getOfflineCollectionsStatus(state)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely could roll this into a complex selector per the pattern we adopted during offline mode.

return fetchedCollectionIds.filter((collectionId) => {
const offlineCollectionsStatus = getOfflineCollectionsStatus(state)
const offlineCollectionIds = Object.keys(offlineCollectionsStatus).filter(
(k) => offlineCollectionsStatus[k] !== OfflineDownloadStatus.INACTIVE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, collections can have an error state. Keep in mind that this status is only for the collection metadata, not necessarily its tracks' download status.

I would say given that here we are offline, we should filter down to only success status. Safest to err on the side of not attempting to display incomplete collections.

@nicoback2 nicoback2 force-pushed the nkang--fix-collections-offline branch from da2588f to a97b364 Compare October 6, 2023 21:29
@nicoback2 nicoback2 enabled auto-merge (squash) October 6, 2023 21:34
@nicoback2 nicoback2 changed the title Fix offline collections not loading if started offline Fix offline collections not loading if started offline C-3187 Oct 6, 2023
@nicoback2 nicoback2 merged commit dfef5c7 into main Oct 6, 2023
2 checks passed
@nicoback2 nicoback2 deleted the nkang--fix-collections-offline branch October 6, 2023 21:50
audius-infra pushed a commit that referenced this pull request Oct 7, 2023
[948683a] Fix gasPrice bug (#6271) Marcus Pasell
[0ccf190] Revert "Revert "[PLAT-259] Upgrade EthereumTx version (#3606)" (#3682)" (#6269) Marcus Pasell
[b6ef1bc] Add react-query to mobile (#6267) Sebastian Klingler
[0754f6a] fix package-lock.json (#6263) Steve Perkins
[dfef5c7] Fix offline collections not loading if started offline (#6245) nicoback2
[8765cb7] Make createAssociatedTokenAccount idempotent for root wallet (#6266) Marcus Pasell
[f8c839a] More web+mobile UI for $AUDIO matching challenges (#6265) Reed
[bbd4cd8] [C-3175] Replace instances of HarmonySelectablePill in web with harmony component (#6246) Kyle Shanks
[1542421] [PAY-1974] Fix explore premium tracks bugs (#6237) Saliou Diallo
[6149349] Backfill challenges amount migration (#6262) Reed
[a9d1a97] trpc: social modal shows list of users who did X (#6253) Steve Perkins
[f732050] [PROTO-1320] Run CI jobs without GCP (#6218) Danny
[6bdf3d4] [PAY-1960] Clean up withdrawals decimal places (#6260) Raymond Jacobson
[36a7802] Bump version to 0.4.25 audius-infra
[28f3e68] [PAY-1975] Fix withdrawal email decimals (#6261) Raymond Jacobson
[eaf3dfe] Wire up UI for $AUDIO matching challenges (#6255) Reed
[ce1c9a9] [C-3174] Add select genre page to sign up (#6241) Dylan Jeffers
[b233a8c] [C-3181, C-3182] Fix collection loading, drawer closing, feature flag perf (#6254) Dylan Jeffers
[ca7f8ee] [C-3133] Port HarmonyButton to harmony package (#6242) Andrew Mendelsohn
[3d294fd] Remove sdk autopublish (#6259) Sebastian Klingler
[cfb519d] bump DN dockerfile python3 dependency to 3.11.6 (#6257) Michelle Brier
[faa348d] @audius/sdk: v3.0.11-beta.21 audius-infra
[949a60d] Remove partition migration again (#6226) Michelle Brier
[a955572] @audius/sdk: v3.0.11-beta.20 audius-infra
[82b278b] Rename access log file (#6256) Isaac Solo
[93a539c] Fix identity push (#6247) Sebastian Klingler
[7c5c10c] rm unneeded error log (#6252) Alec Savvy
[22f2161] [PAY-1945] Add column for amount in user_challenges table + backfill (#6212) Reed
[23c687e] @audius/sdk: v3.0.11-beta.19 audius-infra
[c4862d5] Remove dead code (#6251) Raymond Jacobson
[ca860eb] @audius/sdk: v3.0.11-beta.18 audius-infra
[4f25b11] Remove anon v1 full trending tracks cache (#6250) Isaac Solo
[48a38ff] @audius/sdk: v3.0.11-beta.17 audius-infra
[6fbf250] Add request time in json to server logs (#6235) Isaac Solo
[00921fa] @audius/sdk: v3.0.11-beta.16 audius-infra
[d90c993] Remove broken ethgasstation.info usage, let default gas be set (#6248) Marcus Pasell
[7d021fa] [PAY-1956] Add pay extra column to purchases and sales csv (#6234) Saliou Diallo
[55bea71] @audius/sdk: v3.0.11-beta.15 audius-infra
[7b7586b] [C-3176] Update harmony css variables and import harmony styles into web and embed (#6240) Kyle Shanks
[71b0ae1] @audius/sdk: v3.0.11-beta.14 audius-infra
[139221f] Bump version to 0.4.24 audius-infra
[525f895] @audius/sdk: v3.0.11-beta.13 audius-infra
[b9a71cb] [PROTO-1317] Fix sitemap counts (#6244) Raymond Jacobson
[c9f38bb] @audius/sdk: v3.0.11-beta.12 audius-infra
[931f8c5] [PAY-1944] Bring back AudioBreakdown (#6243) Raymond Jacobson
[97782eb] @audius/sdk: v3.0.11-beta.11 audius-infra
[2e17d03] Always pass test-discovery-api  (#6238) Isaac Solo
[0db1f4b] @audius/sdk: v3.0.11-beta.10 audius-infra
[c360cdb] Update harmony tsconfig to allow for css module classnames without errors (#6236) Kyle Shanks
[fa3c013] @audius/sdk: v3.0.11-beta.9 audius-infra
[b75f5b9] [C-3165] Add finish-account sign-up page (#6196) Dylan Jeffers
[51221e1] Add vercel context for dist jobs (#6233) Sebastian Klingler
[55c2ef7] @audius/sdk: v3.0.11-beta.8 audius-infra
[35c39fb] [PROTO-1239] Fix initting ClaimsManagerClient (#6232) Theo Ilie
[5a450e6] @audius/sdk: v3.0.11-beta.7 audius-infra
[0c510dc] [PAY-1934] Fix some issues with purchase modal state (#6231) Randy Schott
[93cac56] [PAY-1875] Add Premium Tracks Explore Page - Part 1 (#6202) Saliou Diallo
[3b77629] Bump app version to trigger full app release (#6230) nicoback2
[7c03b3c] FollowsYouChip uses trpc (#6227) Steve Perkins
[3207650] @audius/sdk: v3.0.11-beta.6 audius-infra
[14a3b80] [PAY-1903] Default minimum purchase to 1.00 (#6225) Raymond Jacobson
[c25a197] @audius/sdk: v3.0.11-beta.5 audius-infra
[09b6ea0] Fix coming soon tag (#6224) Raymond Jacobson
[c6a90c4] [INF-500] Fix libs tests (#6228) Sebastian Klingler
[9d46861] [INF-501] Disable mobile prod builds on main (#6229) Sebastian Klingler
[1b09f0f] Defer jimp.min.js (#6223) Raymond Jacobson
[f8c1d05] [C-3134] Add SelectablePill component to harmony (#6214) Kyle Shanks
[c4226fe] Bump version to 0.4.23 audius-infra
[889a911] Fix several USDC purchase related bugs (#6222) Raymond Jacobson
[df75ad9] PROTO-1287/1286: add relay reselection logic and relay to healthz (#6140) Alec Savvy
[e0c8f62] As/minor bot fixes (#6221) Alec Savvy
[3fa6a97] PAY-1507, PAY-1769: USDC Withdrawals: SOL Swapping for Uninitiated Accounts (#6220) Marcus Pasell
[558182b] Fix flaky follows on sign up, mobile C-2718 (#6219) nicoback2
[98f6820] Revert "Import harmony styles in web and embed (#6191)" (#6217) Raymond Jacobson
[0de7821] Remove erroneous local-storage (#6216) Raymond Jacobson
[e5901a7] verified bot follow ons (#6215) Alec Savvy
[505b3b0] [PAY-1838] Implement coming soon option for usdc gate (#6189) Raymond Jacobson
[00015d8] Import harmony styles in web and embed (#6191) Kyle Shanks
[fabf67c] PROTO-1304: fix uploads bot (#6206) Alec Savvy
[4181c36] [C-3171] Add Icons to Harmony (#6213) Kyle Shanks
[9f7cf0c] [PAY-1861] Expose Pay Extra details in purchase/sales tables (#6209) Randy Schott
[d90897f] [PAY-1927] Cleanup redux state on purchase modal close (#6210) Reed
[181a6ae] fix import order (#6211) Steve Perkins
[ddf5cb5] Sp es triggers (#6207) Steve Perkins
[786f7a8] [PAY-1936] Allow preview timestamps to be set for "unpreviewable" tracks (#6200) Raymond Jacobson
[1cd4f15] tRPC build + push docker image (#6204) Steve Perkins
[261fb90] [PAY-1939] Convert purchase content drawer to full screen variant (#6205) Randy Schott
[cbcac0f] [PAY-1860] Implements Pay Extra form on mobile (#6184) Randy Schott
[a7fccae] [PROTO-1297] Persist storage/db sizes in db + crudr (#6177) Theo Ilie
[d74e905] [PROTO-1293] Reverse proxy to healthz from mediorum server (#6193) Danny
[2c7e3a0] Optimize trending requests (#6199) Isaac Solo
[f77bf15] [INF-499] Update paths for uploading prober artifacts (#6197) Sebastian Klingler
[127217e] Bump version to 0.4.22 audius-infra
[38525ef] Readall when serving image. (#6201) Steve Perkins
[1362f0e] [PAY-1884] Audio matching challenges for buyer and seller (#6175) Reed
[40325c8] [PAY-1917] Add parens around seconds (#6198) Raymond Jacobson
[15a283d] FollowsYou badge uses trpc (#6195) Steve Perkins
[e72f9ae] [INF-479] [INF-494] Move identity to packages, improve audius-compose dev performance (#6160) Sebastian Klingler
[f04ca8f] @audius/sdk: v3.0.11-beta.4 audius-infra
[6186d98] [C-3168] Add missing submit type on button (#6194) Andrew Mendelsohn
[e254d74] [C-3139] Add create password page (#6186) Dylan Jeffers
[50f9e28] @audius/sdk: v3.0.11-beta.3 audius-infra
[a64b4d8] multiple peer providers with fallback behavior (#6192) Steve Perkins
[a479f4b] @audius/sdk: v3.0.11-beta.2 audius-infra
[a6d1b1f] PROTO-1299: scroll uploads for backfill (#6180) Steve Perkins
[b35c3ea] @audius/sdk: v3.0.11-beta.1 audius-infra
[be5c877] [PAY-1627] Fix stripe onramp layout on moweb (#6174) Raymond Jacobson
[64c899e] Bump version to 0.4.21 audius-infra
[1aa60e3] @audius/sdk: v3.0.11-beta.0 audius-infra
schottra added a commit that referenced this pull request Oct 9, 2023
…states

* origin/main: (54 commits)
  [⚠️] Remove /error page, try again should just refresh (#6258)
  Dispatch $AUDIO matching challenges in non-prod envs (#6276)
  add rc stage cypress commands (#6080)
  Bump version to 0.4.26
  v1.5.45
  Fix gasPrice bug (#6271)
  Revert "Revert "[PLAT-259] Upgrade EthereumTx version (#3606)" (#3682)" (#6269)
  Add react-query to mobile (#6267)
  fix package-lock.json (#6263)
  Fix offline collections not loading if started offline (#6245)
  Make createAssociatedTokenAccount idempotent for root wallet (#6266)
  More web+mobile UI for $AUDIO matching challenges (#6265)
  [C-3175] Replace instances of HarmonySelectablePill in web with harmony component (#6246)
  [PAY-1974] Fix explore premium tracks bugs (#6237)
  Backfill challenges amount migration (#6262)
  trpc: social modal shows list of users who did X (#6253)
  [PROTO-1320] Run CI jobs without GCP (#6218)
  [PAY-1960] Clean up withdrawals decimal places (#6260)
  Bump version to 0.4.25
  [PAY-1975] Fix withdrawal email decimals (#6261)
  ...
schottra added a commit that referenced this pull request Oct 9, 2023
…tripe

* origin/main:
  [PAY-1837] Adds more robust error handling to purchase/sales/withdrawals pages (#6249)
  Fix lint (#6280)
  [⚠️] Remove /error page, try again should just refresh (#6258)
  Dispatch $AUDIO matching challenges in non-prod envs (#6276)
  add rc stage cypress commands (#6080)
  Bump version to 0.4.26
  v1.5.45
  Fix gasPrice bug (#6271)
  Revert "Revert "[PLAT-259] Upgrade EthereumTx version (#3606)" (#3682)" (#6269)
  Add react-query to mobile (#6267)
  fix package-lock.json (#6263)
  Fix offline collections not loading if started offline (#6245)
  Make createAssociatedTokenAccount idempotent for root wallet (#6266)
  More web+mobile UI for $AUDIO matching challenges (#6265)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants