Initialise pusher before rendering children#54188
Conversation
|
The test that is failing in CI passes locally |
aldo-expensify
left a comment
aldo-expensify
left a comment
There was a problem hiding this comment.
This looks a bit like a workaround to me. Can you show where this was crashing due to the initialization being too late? why does this only happen for guides? Would be nice to have more details about the root cause
|
|
||
| const pusherInitialized = useRef(false); | ||
|
|
||
| // eslint-disable-next-line react-compiler/react-compiler |
There was a problem hiding this comment.
Why are we disabling eslint here?
|
@aldo-expensify the primary issue with Pusher was that it was initialized asynchronously outside of the React context. Previously, the app would crash when the ReportScreen was accessed directly via a deep link, as discussed here. This problem arose due to a regression introduced in this PR , where I removed an additional state used for conditionally rendering the ActiveGuidesEventListener, forgetting that To resolve this, the initialization of Pusher now occurs before the rendering of any child components within AuthScreens, thereby mitigating the issue. In previous PR I altered the logic of subscribing to Pusher. This update involves maintaining a reference to a Promise that initializes Pusher, where new subscriptions chain onto this promise. As a result, the additional state in AuthScreens is no longer necessary, which reduces re-renders and simplifies the handling of Pusher initialization across the app. This approach ensures that the state of Pusher's initialization does not need to be tracked separately in various components. |
|
@jnowakow thank you very much for all the details. Just a NAB: Usually code that runs during rendering should be just that: rendering code. What about adding a wrapper component like: Not sure if it introduced other problems though. |
|
Yeah but in this case it would cause screen to blink as we would wait with rendering whole app before Pusher initialises. So it's not a case here. But I think I have a better solution. I'll update the PR shortly |
|
@aldo-expensify now it works as it should. All subscription logic is handled by Pusher and React components don't care if its initialisation state. Earlier I chained on the |
src/libs/Pusher/pusher.ts
Outdated
| */ | ||
| function init(args: Args, params?: unknown): Promise<void> { | ||
| initPromise = new Promise((resolve) => { | ||
| return new Promise((resolve) => { |
There was a problem hiding this comment.
Does this strategy of preserving the original promise work fine if the user does this:
- Log in with
Account A - Log out
- Log in with
Account B
?
I imagine that before a new promise would be created, now we reuse the same. Could that cause a bug?
There was a problem hiding this comment.
Good catch! It could lead to this sequence and crash:
User Alogs ininitPromiseis resolvedUser Alogs outsocketis reassigned tonullUser Blogs ininitPromiseis resolved butsocketis not created yet- App crashes here
Lines 249 to 250 in 31adaa8
There was a problem hiding this comment.
But we can avoid this by creating new Promise in disconnect function like that:
/**
* Disconnect from Pusher
*/
function disconnect() {
if (!socket) {
Log.info('[Pusher] Attempting to disconnect from Pusher before initialisation has occurred, ignoring.');
return;
}
socket.disconnect();
socket = null;
pusherSocketID = '';
initPromise = new Promise((resolve) => {
resolveInitPromise = resolve;
});
}
There was a problem hiding this comment.
Good catch! That would cause a bug as now if you call pusher.subscribe() it will fail with [Pusher] instance not found, socket is cleared on logout and not created again since the promise is already resolved.
Actually the bug already exists on main too as we can always call subscribe before init
There was a problem hiding this comment.
Github comments are not loading up in time 😅
There was a problem hiding this comment.
@s77rt yeah, github lags sometimes but it seems that we all agree to the solution :D
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safariweb.movMacOS: Desktop |
|
@jnowakow Can you please change the tests section to the below: Tests
Offline tests QA Steps
|
|
@s77rt done 🫡 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Thank you for handling! |
|
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 9.0.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.77-6 🚀
|
5 participants
Explanation of Change
This change starts Pusher initialisation before any children of
AuthScreensis rendered allowing them to call safelysubscribe. When initialisation was triggered inuseEffectit was run after children was rendered leading to error. Now initialisation is triggered during first render and error doesn't happen.Fixed Issues
$ #54142
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A
Android: mWeb Chrome
N/A
iOS: Native
N/A
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
N/A