-
Notifications
You must be signed in to change notification settings - Fork 43
Conversation
479a41e
to
983718f
Compare
983718f
to
44f80b0
Compare
Preview this change https://demo.audius.co/rj-c-1411 |
Preview this change https://demo.audius.co/rj-c-1411 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff! a few questions
@@ -6,7 +6,7 @@ import { ReachabilityState } from './types' | |||
type ReachabilityActions = ActionType<typeof actions> | |||
|
|||
const initialState = { | |||
networkReachable: false | |||
networkReachable: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @amendelsohn You think this is safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested:
- going into offline mode with app open and clicking around
- going into app from already offline
Both seem to work well. Lmk if I should test more!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm so iirc the reason for this was to keep us from making and failing a bunch of requests that we assume would succeed.
The tricky case is:
- start the app offline
- wait for the initial feed lineup fetch to fail
- go back online
- make sure the lineup request gets sent again on reconnect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1-4 seems to work fine for me with the changes here! 🤔
RPReplay_Final1669104371.MP4
I'm going to merge this so it gets build to staging apps, but will follow on with some more testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember it mattered whether I actually waited for the previous request to fire and fail while offline. Basically, I think when we do it quickly, either the saga or the request can still be queued up and just proceed when connected. It looks like you turned the network back on quickly after opening. We should make sure it still works after waiting ~20s or until the feedFetchMetadatas
call fails (if we're still making it at all on startup).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this happened because if we default to reachable = true
, we initialize the backend and apiClient on startup before we get a chance to switch reachable to false, and also may briefly render the Feed screen content before we switch to the offline indicator. This may not be a problem anymore, but it's worth testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will push a couple follow-ons today up for review today. I think the yield fetchAccountSucceeded was actually making this work, and I removed that change from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks awesome, and great callouts @sliptype. if you dont mind updating lock @raymondjacobson that would be great
[1264e58] Bump ios version to 1.1.41, lint (#2308) Dylan Jeffers [8e88d8d] [C-1549] Fix mobile share-drawer (#2307) Dylan Jeffers [4acd94b] Bump ios (#2306) Sebastian Klingler [f489853] [C-1326] Fix open in app drawer to open the app properly (#2286) Kyle Shanks [1db362c] [C-1531] Remove open in app from landing page (#2304) Raymond Jacobson [67196b8] iOS fast follows (#2305) Sebastian Klingler [fed67d3] Fix edit images (#2302) Sebastian Klingler [2d17c4a] Bump android image size (#2301) Sebastian Klingler [b16413f] Can be faster (#2300) Raymond Jacobson [0ea02e5] Fix types (#2299) Sebastian Klingler [f02ad81] [C-1411] Fix reachability issues with new splash screen (#2298) Raymond Jacobson [9afd61c] Use navigation.navigate for search (#2297) Sebastian Klingler [996a451] [C-1404] Add initial mobile wallet-connect (#2294) Dylan Jeffers [8eba94e] [C-1427] Fix multiple issues with Collectibles on mobile (#2296) Sebastian Klingler [d776787] Improve mobile image fetching (#2295) Sebastian Klingler [d3eddcc] [C-1535] Add mobile-upload analytics, fix remix field (#2293) Dylan Jeffers [ab6ece8] [C-1411] Improve loading splash (#2289) Raymond Jacobson [9d9ac84] [C-1328] Add edit/delete track mobile overflow options (#2292) Dylan Jeffers [82aec6c] [C-1509] Add input-accessory-view (#2291) Dylan Jeffers [916ada5] Fix premium content types (#2290) Sebastian Klingler
Description
Use https://github.com/zoontek/react-native-bootsplash for loading splash screen
Change a few loading state things to make white screen go away
Some painful slow parts while libs init happens / early access mode shows / feed loads. I think there's more work that could be done here. But I think this is better as is than prod currently.
RPReplay_Final1669075213.MP4
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.