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

Handle starting a chat with secondary login when chat with primary exists #22147

Closed
wants to merge 4 commits into from
Closed

Handle starting a chat with secondary login when chat with primary exists #22147

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8303e6e
Clear out optimistic report when there is preexisting report
puneetlath Jul 4, 2023
08a55b9
Replace central pane instead of navigating
puneetlath Jul 10, 2023
3791c2e
Improve function comments
puneetlath Jul 12, 2023
056c33e
Use getActiveRoute for determining current route
puneetlath Jul 25, 2023
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
14 changes: 14 additions & 0 deletions src/libs/Navigation/Navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,19 @@ function navigate(route = ROUTES.HOME, type) {
linkTo(navigationRef.current, route, type);
}

/**
* Replaces the current route in the main chat pane, without adding the existing route to the stack.
* Makes it so that the back button will not return to the previous route.
* @param {String} route
*/
function replaceCentralPaneScreen(route) {
if (!canNavigate('replace', {route})) {
return;
}

navigationRef.current.dispatch(StackActions.replace(NAVIGATORS.CENTRAL_PANE_NAVIGATOR, route));
}

/**
* @param {String} fallbackRoute - Fallback route if pop/goBack action should, but is not possible within RHP
* @param {Bool} shouldEnforceFallback - Enforces navigation to fallback route
Expand Down Expand Up @@ -224,6 +237,7 @@ function setIsNavigationReady() {
export default {
canNavigate,
navigate,
replaceCentralPaneScreen,
setParams,
dismissModal,
isActiveRoute,
Expand Down
16 changes: 15 additions & 1 deletion src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import Str from 'expensify-common/lib/str';
import ONYXKEYS from '../../ONYXKEYS';
import * as Pusher from '../Pusher/pusher';
import LocalNotification from '../Notification/LocalNotification';
import Navigation from '../Navigation/Navigation';
import Navigation, {navigationRef} from '../Navigation/Navigation';
import * as ActiveClientManager from '../ActiveClientManager';
import Visibility from '../Visibility';
import ROUTES from '../../ROUTES';
Expand Down Expand Up @@ -804,6 +804,20 @@ function handleReportChanged(report) {
return;
}

// It is possible that we optimistically created a DM/group-DM for a set of users for which a report already exists.
// In this case, the API will let us know by returning a preexistingReportID.
// We should clear out the optimistically created report and re-route the user to the preexisting report.
if (report && report.reportID && report.preexistingReportID) {
Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, null);
const currentRoute = navigationRef.current && navigationRef.current.getCurrentRoute();
Copy link
Collaborator

@mananjadhav mananjadhav Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could we have used Navigation.getActiveRoute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right we could.

Copy link
Contributor Author

@puneetlath puneetlath Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Though Navigation.getActiveRoute returns a string like /r/1195783708973664 which we'd have to copmare like this:

if (Navigation.getActiveRoute === `/r/${report.reportID}`) {
...
}

Which doesn't actually feel better to me. So I think I prefer it the way it is. What do you think @mananjadhav?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, I could create a Navigation.getActiveRouteObject function.

Copy link
Collaborator

@mananjadhav mananjadhav Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah that sounds good, although I am on the fence, as I can see other places where we're importing navigationRef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think I prefer to just leave it as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Beamanator do you have an opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm if we use getActiveRoute we could do:

if (Navigation.getActiveRoute().endsWith(`/${report.reportID}`) {

Which doesn't really seem that bad to me, I do feel like that looks better than the current implementation, especially since we're reusing a useful looking function :D

Also I see navigationRef.current.getCurrentRoute().path is undefined in the first navigation. in this comment:

App/src/libs/Navigation/Navigation.js

Lines 205 to 206 in ecf0ec2

* Building path with getPathFromState since navigationRef.current.getCurrentRoute().path
* is undefined in the first navigation.

Which kinda scares me :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh fair points. Ok, I'm down with that approach, will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!


// Only re-route them if they are still looking at the optimistically created report
if (lodashGet(currentRoute, 'params.reportID') === report.reportID) {
Navigation.replaceCentralPaneScreen(ROUTES.getReportRoute(report.preexistingReportID));
}
return;
}

if (report && report.reportID) {
allReports[report.reportID] = report;

Expand Down
Loading