-
Notifications
You must be signed in to change notification settings - Fork 108
ios: fix app flickers on start #3498
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
base: master
Are you sure you want to change the base?
Conversation
a756c26
to
57f7676
Compare
frontends/ios/BitBoxApp/BitBoxApp/Assets.xcassets/dbb_logo_light.imageset/dbb_logo_dark.svg
Outdated
Show resolved
Hide resolved
57f7676
to
6173c48
Compare
6173c48
to
a97dd49
Compare
PTAL @benma |
3640f24
to
f5f9bb5
Compare
checkReactAppReady(webView: webView) | ||
} | ||
|
||
private func checkReactAppReady(webView: WKWebView, retryCount: Int = 0) { |
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.
Instead of using an arbitrary-valued delay, here we're checking for reactAppReady property instead. Check runs recursively until that value is set to true (with max retry).
This property is set by the useAppReady hook from the react app. cc @benma
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.
Nice!
Instead of polling, please call directly into Swift. See this as an example where TS is calling goCall(), and the call is handled in Swift:
contentController.add(bridge, name: "goCall") |
This would be better as it requires fewer calls and has an even smaller delay until ready. When the react app is ready, tell Swift to transition.
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.
(see comment)
f5f9bb5
to
4d188c7
Compare
@benma , PTAL thanks a lot for the review! |
71464f8
to
9cd288d
Compare
* until the React app has finished loading its initial data. | ||
*/ | ||
export const useAppReady = (accounts?: IAccount[], devices?: TDevices) => { | ||
useEffect(() => { |
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.
Would it make sense to add a fallback?
useEffect(() => {
const timer = setTimeout(() => {
/* send appReady */
}, 3000);
return () => clearTimeout(timer);
Just in case there is some weird delay in getting accounts
and devices
results.
Otherwise code looks good, I'll test it on a device tomorrow.
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.
Okay - added the fallback to call regardless of the devices
|| accounts
results.
9cd288d
to
49040ea
Compare
@benma thanks 👍 PTAL (also rebased). |
49040ea
to
c7fd4e5
Compare
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.
Unfortunately it does not work for me, WebView.swift never gets the appReady message and the webview stays invisible.
I think it may be the |
c7fd4e5
to
145d212
Compare
Fixing it by adding a check in WebView.swift for a message sent from the react app signaling react app is ready. On ready, it reveals the webview smoothly with transition.
145d212
to
3fe986e
Compare
@benma , thanks for the review. Fixed the bug and simplified the hooks. PTAL 🙏 |
Fixing it by adding a check in WebView.swift for a message
sent from the react app signaling react app is ready.
On ready, it reveals the webview smoothly with transition.