Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[HybridApp] Fix for push notifications in HybridApp on Android #46090

Merged
merged 5 commits into from
Jul 29, 2024
Merged

[HybridApp] Fix for push notifications in HybridApp on Android #46090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
210003c
fix transitions from notifications on android hybrid app
staszekscp Jul 23, 2024
aab2091
fix unreliable navigation
staszekscp Jul 24, 2024
8675cee
prettier
staszekscp Jul 24, 2024
c88769d
add timeout to ios
staszekscp Jul 26, 2024
7155fed
merge main
staszekscp Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/components/HybridAppMiddleware/index.ios.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type React from 'react';
import {useContext, useEffect, useState} from 'react';
import {useContext, useEffect, useRef, useState} from 'react';
import {NativeEventEmitter, NativeModules} from 'react-native';
import type {NativeModule} from 'react-native';
import {useOnyx} from 'react-native-onyx';
Expand All @@ -15,6 +15,7 @@ import * as Welcome from '@userActions/Welcome';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {HybridAppRoute, Route} from '@src/ROUTES';
import ROUTES from '@src/ROUTES';
import type {TryNewDot} from '@src/types/onyx';

type HybridAppMiddlewareProps = {
Expand Down Expand Up @@ -50,6 +51,20 @@ function HybridAppMiddleware({children, authenticated}: HybridAppMiddlewareProps
const [sessionEmail] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.email});
const [completedHybridAppOnboarding] = useOnyx(ONYXKEYS.NVP_TRYNEWDOT, {selector: onboardingStatusSelector});

const maxTimeoutRef = useRef<NodeJS.Timeout | null>(null);

// We need to ensure that the BootSplash is always hidden after a certain period.
useEffect(() => {
if (!NativeModules.HybridAppModule) {
return;
}

maxTimeoutRef.current = setTimeout(() => {
Log.info('[HybridApp] Forcing transition due to unknown problem', true);
setStartedTransition(true);
setExitTo(ROUTES.HOME);
}, 3000);
}, []);
/**
* This useEffect tracks changes of `nvp_tryNewDot` value.
* We propagate it from OldDot to NewDot with native method due to limitations of old app.
Expand Down
25 changes: 21 additions & 4 deletions src/components/HybridAppMiddleware/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type React from 'react';
import {useContext, useEffect, useState} from 'react';
import {useContext, useEffect, useRef, useState} from 'react';
import {NativeModules} from 'react-native';
import {useOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
Expand All @@ -14,6 +14,7 @@ import * as Welcome from '@userActions/Welcome';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {HybridAppRoute, Route} from '@src/ROUTES';
import ROUTES from '@src/ROUTES';
import type {TryNewDot} from '@src/types/onyx';

type HybridAppMiddlewareProps = {
Expand Down Expand Up @@ -49,6 +50,20 @@ function HybridAppMiddleware({children, authenticated}: HybridAppMiddlewareProps
const [sessionEmail] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.email});
const [completedHybridAppOnboarding] = useOnyx(ONYXKEYS.NVP_TRYNEWDOT, {selector: onboardingStatusSelector});

const maxTimeoutRef = useRef<NodeJS.Timeout | null>(null);

// We need to ensure that the BootSplash is always hidden after a certain period.
useEffect(() => {
if (!NativeModules.HybridAppModule) {
return;
}

maxTimeoutRef.current = setTimeout(() => {
Log.info('[HybridApp] Forcing transition due to unknown problem', true);
setStartedTransition(true);
setExitTo(ROUTES.HOME);
}, 3000);
Comment on lines +61 to +65
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can look into the underlying problem later. Is 3 seconds good enough to reliably solve this problem though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a security measure that @mateuuszzzzz already wanted to introduce for some edge cases. The worst thing that will happen is that a user will see some transitioning between screens which we wanted to cover, though 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm yes I think we only want to add a setTimeout as a last resort, is there nothing else we can do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about it, but I wanted to keep it to make sure we never get a "bootsplash got frozen" situation. I tried to play with state, but I think this is the safest from the UX perspecitve anyways. Also, it won't any additional re-renders/state changes.

I'm not 100% happy with this solution either, but for now I think we can stay with it. When we have some bandwidth in the future I think we can return to this issue to solve it in a cleaner way.

}, []);
/**
* This useEffect tracks changes of `nvp_tryNewDot` value.
* We propagate it from OldDot to NewDot with native method due to limitations of old app.
Expand Down Expand Up @@ -93,13 +108,15 @@ function HybridAppMiddleware({children, authenticated}: HybridAppMiddlewareProps
Navigation.isNavigationReady().then(() => {
// We need to remove /transition from route history.
// `useExitTo` returns undefined for routes other than /transition.
if (exitToParam) {
if (exitToParam && Navigation.getActiveRoute().includes(ROUTES.TRANSITION_BETWEEN_APPS)) {
Log.info('[HybridApp] Removing /transition route from history', true);
Navigation.goBack();
}

Log.info('[HybridApp] Navigating to `exitTo` route', true, {exitTo});
Navigation.navigate(Navigation.parseHybridAppUrl(exitTo));
if (exitTo !== ROUTES.HOME) {
Log.info('[HybridApp] Navigating to `exitTo` route', true, {exitTo});
Navigation.navigate(Navigation.parseHybridAppUrl(exitTo));
}
setExitTo(undefined);

setTimeout(() => {
Expand Down
3 changes: 2 additions & 1 deletion src/libs/Navigation/AppNavigator/index.native.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, {memo, useContext, useEffect} from 'react';
import {NativeModules} from 'react-native';
import {InitialURLContext} from '@components/InitialURLContextProvider';
import Navigation from '@libs/Navigation/Navigation';
import ROUTES from '@src/ROUTES';
import type ReactComponentModule from '@src/types/utils/ReactComponentModule';

type AppNavigatorProps = {
Expand All @@ -13,7 +14,7 @@ function AppNavigator({authenticated}: AppNavigatorProps) {
const initUrl = useContext(InitialURLContext);

useEffect(() => {
if (!NativeModules.HybridAppModule || !initUrl) {
if (!NativeModules.HybridAppModule || !initUrl || !initUrl.includes(ROUTES.TRANSITION_BETWEEN_APPS)) {
return;
}

Expand Down
3 changes: 2 additions & 1 deletion src/libs/Navigation/AppNavigator/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, {lazy, memo, Suspense, useContext, useEffect} from 'react';
import {NativeModules} from 'react-native';
import {InitialURLContext} from '@components/InitialURLContextProvider';
import Navigation from '@libs/Navigation/Navigation';
import ROUTES from '@src/ROUTES';
import lazyRetry from '@src/utils/lazyRetry';

const AuthScreens = lazy(() => lazyRetry(() => import('./AuthScreens')));
Expand All @@ -16,7 +17,7 @@ function AppNavigator({authenticated}: AppNavigatorProps) {
const initUrl = useContext(InitialURLContext);

useEffect(() => {
if (!NativeModules.HybridAppModule || !initUrl) {
if (!NativeModules.HybridAppModule || !initUrl || !initUrl.includes(ROUTES.TRANSITION_BETWEEN_APPS)) {
return;
}

Expand Down
5 changes: 5 additions & 0 deletions src/libs/Notification/PushNotification/subscribePushNotification/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {NativeModules} from 'react-native';
import Onyx from 'react-native-onyx';
import applyOnyxUpdatesReliably from '@libs/actions/applyOnyxUpdatesReliably';
import * as ActiveClientManager from '@libs/ActiveClientManager';
Expand Down Expand Up @@ -85,6 +86,10 @@ function navigateToReport({reportID, reportActionID}: ReportActionPushNotificati
// The attachment modal remains open when navigating to the report so we need to close it
Modal.close(() => {
try {
// Get rid of the transition screen, if it is on the top of the stack
if (NativeModules.HybridAppModule && Navigation.getActiveRoute().includes(ROUTES.TRANSITION_BETWEEN_APPS)) {
Navigation.goBack();
}
// If a chat is visible other than the one we are trying to navigate to, then we need to navigate back
if (Navigation.getActiveRoute().slice(1, 2) === ROUTES.REPORT && !Navigation.isActiveRoute(`r/${reportID}`)) {
Navigation.goBack();
Expand Down
Loading