[NO QA] Error: TaskQueue: Error with task : [Pusher] instance not found. Pusher.subscribe().#81811
Conversation
…er.subscribe(). Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@blimpich I don’t think we can add QA test steps for this since it’s difficult to reproduce. I’ve verified the crash fix using Before:before_fix_instance_crash.mp4After:after_fix_instance_crash.mp4 |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
melvin-bot
bot
requested review from
trjExpensify and
truph01
and removed request for
a team
1 similar comment
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 785d20afa0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!socket) { | ||
| throw new Error(`[Pusher] instance not found. Pusher.subscribe() | ||
| most likely has been called before Pusher.init()`); | ||
| const error = new Error('[Pusher] instance not found. Pusher.subscribe() most likely has been called before Pusher.init()'); | ||
|
|
||
| if (__DEV__) { | ||
| throw error; | ||
| } | ||
|
|
||
| // In production, report to Sentry without crashing the app. | ||
| // This can happen when disconnect() is called (e.g. during the "Upgrade Required" | ||
| // teardown) before this deferred InteractionManager callback runs. | ||
| Sentry.captureException(error, { | ||
| tags: {source: 'Pusher.subscribe'}, | ||
| extra: {channelName, eventName}, | ||
| }); | ||
| Log.info('[Pusher] Socket disconnected before subscribe could complete, skipping subscription', false, {channelName, eventName}); | ||
| resolve(); | ||
| return; |
There was a problem hiding this comment.
In production, this path calls resolve() even though no channel is created. That makes callers think the subscription succeeded and bypasses their .catch handlers (e.g., PusherUtils.subscribeToPrivateUserChannelEvent), so they won’t log or retry. If the disconnect was transient (not only the upgrade-required teardown), the channel is never registered and won’t be resubscribed on reconnect, leaving real-time updates silently missing until the caller happens to re-subscribe. Consider rejecting or queueing a retry when socket is null so failures can be handled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We are already calling Sentry.captureException in the production path, so this scenario will still be tracked and visible in Sentry — it's not being silently swallowed. If the frequency or context of these events ever changes (e.g. it starts happening outside of teardown), we'd see it in the Sentry dashboard.
Rejecting would actually be problematic because some callers (e.g. the typing/leaving-room subscribers in Report/index.ts) don't have .catch handlers, which would cause unhandled promise rejections. And for callers that do have .catch (like PusherUtils), the error log ("Failed to subscribe to Pusher channel") would be misleading for what's actually an intentional teardown.
Sounds good. Please add [NO QA] to the title in that case |
|
@blimpich For some reason, I can’t edit the title of any of my PRs — it might be a GitHub issue. Are you able to change the title? Also, the ESLint check failure isn’t related. |
blimpich
changed the title
|
@Krishna2323 lint is faling @truph01 are you able to review this one? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
The original purpose of this PR was to address the Sentry error. However, with the current changes, the Sentry error still occurs—we’ve only refactored how the error is logged. Is this approach acceptable, @rlinoz? |
From the original thread I think so, we are logging so we don't lose context, but not crashing here is benefitial as it doesn't leave the app in a bad state. |
|
@truph01 the error should only crash in dev mode, in prod, the error should not crash the app. |
I mean that, the error is still being sent to Sentry. However, based on this comment, we’re okay with that. |
|
Yeah, can we just confirm it is not being displayed as fatal in sentry anymore, please? |
|
@Krishna2323 Could you address this comment?
|
|
I was able to build the app with the WhatsApp.Video.Feb.13.2026.1.mp4 |
|
@Krishna2323 It looks like in dev, we don't send data to sentry |
|
Yeah, you can comment out that part for testing this. |
|
I'll try to update this today, I was OOO yesterday. |
|
Checking... |
|
So are we good to merge this one? |
|
@rlinoz Yes, I believe we’re good to merge. To summarize:
|
|
🚧 @rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.3.24-1 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.24-3 🚀
|
6 participants
Explanation of Change
Fixed Issues
$ #81781
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari