Skip to content
This repository was archived by the owner on Oct 4, 2023. It is now read-only.

[C-2066] Refactor offline-mode to use redux-persist#2788

Merged
dylanjeffers merged 9 commits into
mainfrom
dj-c-offline-queue
Feb 7, 2023
Merged

[C-2066] Refactor offline-mode to use redux-persist#2788
dylanjeffers merged 9 commits into
mainfrom
dj-c-offline-queue

Conversation

@dylanjeffers

Copy link
Copy Markdown
Contributor

Description

Refactors offline mode to use redux persist

Dragons

Lots of changes, so let's triage things in am

@dylanjeffers dylanjeffers requested review from a team and Kyle-Shanks and removed request for a team February 7, 2023 10:14
dispatch(recordListen(trackId))
} else if (isOfflineModeEnabled) {
queue.addJob<PlayCountWorkerPayload>(PLAY_COUNTER_WORKER, { trackId })
// TODO fix offline play counts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah i see. yes

@sliptype sliptype left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clean!! Generally looks good to me, but still a lot of things missing

downloadNewFavoriteIfNecessary
)
}
import { clearOfflineDownloadsSaga } from './sagas/clearOfflineDownloadsSaga'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am such a big fan of the individually named saga files


const { getUserId } = accountSelectors

// TODO add favorites collection task

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This todo still valid?

yield* put(startDownload({ type: 'collection', id: collectionId }))

// "favorites" collection short circuit
if (typeof collectionId === 'string') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this condition safe? Is there a better way to check for favorites collection?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I meant to check favorites explicitly cause I changed the collectionId type to be number or "favorites"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes great catch, should just be able to check if its equal to DOWNLOAD_REASONS_FAVORITE


export function* processDownloadQueueSaga() {
while (true) {
const downloadTask = yield* takeEvery(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, I would like to check the perf of this new approach

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed! It goes one at a time at the very least, and it's forked

type CachedUser = { id: ID; uid: UID; metadata: UserMetadata }
type CachedTrack = { id: ID; uid: UID; metadata: TrackMetadata }

// Load offline data into redux on app start

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So this is just hydrating the cache right? Maybe we can comment about that

And theoretically this goes away if we were to persist the entire cache eventually?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah exactly, can update comment

for (const collectionId of downloadedCollectionIds) {
if (collectionId === DOWNLOAD_REASON_FAVORITES) continue
try {
const userCollection = yield* call(getCollectionJson, collectionId)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this still correct, do we need to getCollectionJson?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes cause this is the actual metadata for the cache!

const offlineDownloadsPersistConfig = {
key: 'offline-downloads',
storage: AsyncStorage,
blacklist: ['isDoneLoadingFromDisk', 'queueStatus']

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why does queue status need to be blacklisted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it doesn't really need to, but i thought probably best to not have "in-progress" saved so when we come back offline it starts going initially. but we can def think about this

@@ -0,0 +1,15 @@
import { select, take } from 'typed-redux-saga'

export function* monitorSelector(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Would be cool to be able to pass equalityFn into this too

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i ended up not using this! we can keep around if you think useful, but i might delete just to avoid the bloat?

@dylanjeffers dylanjeffers merged commit c4080b4 into main Feb 7, 2023
@dylanjeffers dylanjeffers deleted the dj-c-offline-queue branch February 7, 2023 19:13
audius-infra pushed a commit that referenced this pull request Feb 11, 2023
[3cbe169] [C-2070] show unsave drawer on all unfavorite buttons (#2823) Andrew Mendelsohn
[7de990a] [C-2040] Improve duration counter accuracy/performance (#2816) Dylan Jeffers
[e975303] [C-2065] Only show collection download switch if favorited (#2821) Sebastian Klingler
[8b02196] [C-2031] Fix confirmation drawer flashes (#2817) Dylan Jeffers
[6d9df4a] [C-2067] Add 'play-count' queue (#2814) Andrew Mendelsohn
[960de2c] Don't download on save if all favorites toggle off (#2815) Andrew Mendelsohn
[5310d81] Fix onPressRail animation error (#2813) Dylan Jeffers
[921e5eb] [C-2090] Reduce file reads on play (#2812) Sebastian Klingler
[a77f53c] Remove clear offline downloads button (#2811) Dylan Jeffers
[a2815e6] [C-2038] Fix unresponsive reactions (#2808) Dylan Jeffers
[a18afa2] [C-2058] prevent restart loop (#2810) Sebastian Klingler
[3478660] [C-1984] Don't use nativeDriver for slider and tracking bar anims (#2807) Sebastian Klingler
[3868712] [C-2083] Add offline sync to queue (#2803) Dylan Jeffers
[afac00a] [C-2080] Update value change logic for download toggles for favorites and collection switches (#2805) Kyle Shanks
[fb32d69] Fix audio tx modal where metadata is null (#2793) Reed
[62108a3] Update sdk to v1.0.45 (#2806) Reed
[d52491f] [C-2039] Fix search query persist (#2804) Dylan Jeffers
[aee4d8e] [C-2056] Fix lineup scrolling (#2802) Sebastian Klingler
[e776860] [C-2072] Add stale collection logic (#2801) Kyle Shanks
[a701c8d] [C-1975] Fix safari stream mp3 (#2800) Raymond Jacobson
[7e83831] Update challenges copy (#2799) Reed
[092bab6] Add script to help with linking sdk (#2798) Raymond Jacobson
[6b4f7a1] [PAY-898] Design fixes for NavMenuPopup (#2787) Marcus Pasell
[11f57c2] Remove identity artist pick path (#2781) Michelle Brier
[a9a135a] [C-2068] Add downloadQueue worker cancellation (#2797) Dylan Jeffers
[316b91f] Decode artist pick IDs and fix artist pick reads from discovery (#2795) Michelle Brier
[1dcc33a] [C-2079] Fix mobile sign up analytics (#2791) Sebastian Klingler
[e2b95ac] Add redownload logic for rehydration and stale tracks (#2790) Kyle Shanks
[c4080b4] [C-2066] Refactor offline-mode to use redux-persist (#2788) Dylan Jeffers
[4bf7c22] [PAY-891] Handle remix and availability settings on mobile (#2765) Saliou Diallo
[6f4dc13] Fix TikTok association (#2786) Sebastian Klingler
[ccee882] [C-2011] move whole tracks out of offline slice (#2784) Andrew Mendelsohn
[fa9ef86] Default offline mode off (#2785) Andrew Mendelsohn
[0d4692e] C-2041 Fix collection lineup not loading when library is only partially downloaded (#2782) nicoback2
[1159bb7] Fix remix (#2783) Saliou Diallo
[c2be8e4] Improve styling of TrackList on Favorites Screen (#2778) Sebastian Klingler
[dc17f41] Fix download progress numerator going down in the middle of downloading C-2037 (#2777) nicoback2
@AudiusProject AudiusProject deleted a comment from linear Bot Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants