[HOLD for payment 2023-09-11] [$2000] Fix issue with Map panning behavior

[hayata-suenaga]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Context

A new feature was added in this PR recently where we display a map. We discovered an issue with the map's behavior though. We need a fix as soon as possible.

We also introduced a new library for this map feature. The library's repo is here: https://github.com/Expensify/react-native-x-maps.

The map should zoom into an area when a single waypoint is passed. When several waypoints are passed, the map should pan out to fix all waypoints passed on map. These zoom in and pan out movements are not working on iOS and Android.

The last version of the library above is 1.0.10 and the version specified in App's package.json on the main branch is 1.0.9. But this should not be the cause of the issue, I believe

Related issues

• Android - IOU Distance - Map does not zoom in to the selected location on Start point #25871 (comment)

Hint

The root cause of this issue might be inside react-native-x-maps. If you need to modify the library code and test changes in App, you can push changes in x-maps to a forked repo on GitHub and use the link of your branch on GitHub to install the changed code in your App. More info on this here

Action Performed:

This issue exists on the main branch. It should be re-producible on iOS and Android.

1. Pull the latest main

2. Open NewDot on iOS simulator or Android emulator

3. Go to any chat room where you can request money (i.e. IOU)

4. Click the "+" icon next to the chat message text field. Click "Request Money" on the pop up menu that appears.

5. Check "Distance" tab on the Right Hand Panel that appears

6. Click the "Start" item from the list of waypoints.

7. In the text field that appears in the next screen, type "88 Kearny Street". Click the first option "88 Kearny Street, San Francisco, CA, USA" that appears.
   

8. Check that the map doesn't move when it should zoom into a part of San Francisco. Check the video below for the expected behavior of the map (taken on Web).

   Current behavior

   Screen.Recording.2023-08-23.at.1.25.35.PM.mov
   Expected behavior

   Screen.Recording.2023-08-22.at.4.01.02.PM.mov

9. Click the "Stop" item from the list of waypoints.

10. In the text field that appears in the next screen, type "Golden Gate Park". Click the first option "88 Kearny Street, San Francisco, CA, USA" that appears.

11. Check that the map doesn't move at all when it should pan out to fix the new point added. Check the video below for the expected behavior of the map (taken on Web).

    Current behavior

    Screen.Recording.2023-08-23.at.1.28.41.PM.mov
    Expected behavior

    Screen.Recording.2023-08-22.at.4.08.56.PM.mov

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Reproducible in staging?: Only on main
Reproducible in production?: Nop
Slack conversation: #25161 (comment)

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0167825771cb68ae59
• Upwork Job ID: 1694128479546347520
• Last Price Increase: 2023-08-24

[melvin-bot]

Triggered auto assignment to @NicMendonca (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0167825771cb68ae59

[melvin-bot]

Current assignee @NicMendonca is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Current assignee @allroundexperts is eligible for the External assigner, not assigning anyone new.

[hayata-suenaga]

@NicMendonca I created this issue from this Slack discussion. This is probably regression from a PR that I created for the Wave 5 Distance Request project. You don't have to reproduce this issue. I already confirmed that the issue exists.

I assigned @allroundexperts because they're very involved in the Wave 5 project and has the necessary context.

@allroundexperts if you come up with the solution, you can also create a PR yourself 🙇

[ShogunFire]

I have an error:
"Mapbox error MapLoad error Failed to load glyphs:
HTTP status code 403"

[hayata-suenaga]

I have an error:
"Mapbox error MapLoad error Failed to load glyphs:
HTTP status code 403"

thank you for reporting but this issue is known and the fix should be merged soon

[akinwale]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Map panning behaviour not working as expected on native mobile devices. Also, Add Stop continuously moves the map for the same location after a marker has been placed.

What is the root cause of that problem?

There are two issues:

1. On iOS and Android native, flyTo (which is used to set the first marker coordinate) does not set the center of the map correctly. A subsequent call to zoomTo zooms to the original center coordinate (set in defaultSettings), instead of the point where the first marker was placed.
2. On iOS native, the camera operations to adjust the map viewport (flyTo, zoomTo, setCamera, etc) do not work when the screen containing the map is not in focus.

What changes do you think we should make in order to solve the problem?

1. Replace flyTo and zoomTo with setCamera
This will set the center coordinate to the first marker, and then zoom correctly on the marker. This change will be made in MapView.tsx.

cameraRef.current?.setCamera({
    centerCoordinate: waypoints[0].coordinate,
    zoomLevel: 15
});

2. Camera update on iOS when the screen is not in focus
We can make use of a combination of MapView events and the withNavigationFocus HOC to get the camera working after adding or updating markers.

The following changes will need to be made.

react-native-x-maps changes

1. In MapViewTypes.ts, add onMapLoad to MapViewProps which will be an optional function type.

onMapLoad?: Function;

2. In MapView.tsx, add the onMapLoad prop as one of the function arguments.

const MapView = forwardRef<MapViewHandle, MapViewProps>(function MapView(
    {accessToken, style, styleURL, pitchEnabled, mapPadding, initialState, waypoints, directionCoordinates, directionStyle, onMapLoad},
    ref,
) { // ...

3. Add the onWillStartLoadingMap prop to MapView.Mapbox and call the onMapLoad prop in the handler if it's set.

onWillStartLoadingMap={() => {
    if (onMapLoad) {
        onMapLoad();
    }
}}

DistanceRequest changes

1. Add the withNavigationFocus HOC.

export default compose(
    withNavigationFocus,
    withOnyx({
    transaction: {
        key: (props) => `${ONYXKEYS.COLLECTION.TRANSACTION}${props.transactionID}`,
    },
    mapboxAccessToken: {
        key: ONYXKEYS.MAPBOX_ACCESS_TOKEN,
    },
}))(DistanceRequest);

2. Add the destructured isFocused prop as a parameter to the function component.

function DistanceRequest({transactionID, transaction, mapboxAccessToken, isFocused}) { // ...

3. Add a useState hook for a flag to indicate whether or not the map is loaded.

 const [mapLoaded, setMapLoaded] = useState(false);

4. Add a useEffect hook to set the map loaded to false when the screen is not focused.

useEffect(() => {
    if (!isFocused) {
        setMapLoaded(false);
    }
}, [isFocused]); 

5. Update the condition for displaying the MapView component in DistanceRequest:

{!isOffline && isFocused && Boolean(mapboxAccessToken.token) ? (

6. Add a handler for the onMapLoad prop for MapView to set the map loaded state to true.

onMapLoad={() => setMapLoaded(true)}

7. Add a useEffect hook to update the waypoint markers with the mapLoaded state as a dependency. waypointMarkers and other associated values (numberOfWaypoints, lastWaypointIndex, etc) will have to be refactored to make use of useState hooks.

useEffect(() => {
    if (mapLoaded) {
        // set waypoint markers
    }
}, [mapLoaded]);

What alternative solutions did you explore? (Optional)

None.

25732-demo-2.mp4

[allroundexperts]

@akinwale Thanks for your proposal. I can not understand your RCA. Can you please elaborate it further? Specifically, why does it work on Android/web and not on iOS?

[akinwale]

@akinwale Thanks for your proposal. I can not understand your RCA. Can you please elaborate it further? Specifically, why does it work on Android/web and not on iOS?

There are two issues here.

1. The issue with the "Add Stop" button.
2. The MapView.Camera not updating when the map view is not visible on screen.

"Add Stop" button
The Camera's flyTo method is supposed to set the map's center to the provided coordinates, but it appears the map's actual center coordinate is not being updated by this method, probably due to a buggy implementation. Calling the zoomTo method afterwards will zoom in on the map's center coordinate (which is what was set in the default).

Since flyTo does not actually set the center coordinate, every time the "Add Stop" button is pressed, the map reanimates the operations. Setting the centerCoordinate and the zoomLevel makes sure that the camera only updates when these values change. Also, note that the implementation for web is different from the native implementation, so that's probably why it works differently.

MapView.Camera not updating
When trying to edit the addresses for the waypoints using the Waypoint Editor, the map view is not displayed on the screen. After selecting an address (for the Start marker for example), the Onyx key ONYXKEYS.COLLECTION.TRANSACTION_<transactionID> gets updated with the address details. This Onyx merge will trigger an update on DistanceRequest because we have the following line:

const waypoints = lodashGet(transaction, 'comment.waypoints', {});

The marker location will be displayed on the map, but since the DistanceRequest screen containing the map view was not visible on the screen during the Onyx update, none of the camera operations to adjust the view will work (flyTo, zoomTo, etc). Setting any of the props (bounds, centerCoordinate, zoomLevel) on the camera will also do nothing. The map has to be visible on the screen before the camera view can be adjusted.

The difference in behaviour is probably due to the underlying rnmapbox implementation for the different platforms. Perhaps, when the map is not visible, visual updates or camera operations are suspended on iOS to save memory usage or for improved performance? I would have to dive really deep into the native implementation to understand why there is a difference.

Hope this helps. Please let me know if you require further clarifications and I'll be happy to explain.

[hayata-suenaga]

Thank you very much @akinwale for your detailed explanations. I like how you identified two issues and address each of them separately. I will do the same and write my response for the two issue and suggested solutions separately

1. The issue with the "Add Stop" button.

This is not actually an issue. I'd say this is an expected behavior. As long as there is a single waypoint, the map keeps adjusting itself to zoom into that single waypoint. But, if the user doesn't move the map, the map stays the same. This issue is also a little bit different from the original issue: "Map doesn't zoom in to a single waypoint location unless Add stop button is clicked."

Please see the following video:

Video

Screen.Recording.2023-08-24.at.8.46.28.AM.mov

2. The MapView.Camera not updating when the map view is not visible on screen.

This is actually not true. The map is still mounted even when the waypoint editor page (the page with text input for address) is visible. The iOS maps used to work normally (cc: @allroundexperts) so this probably is not the reason.

[akinwale]

1. The issue with the "Add Stop" button.

This is not actually an issue. I'd say this is an expected behavior. As long as there is a single waypoint, the map keeps adjusting itself to zoom into that single waypoint. But, if the user doesn't move the map, the map stays the same. This issue is also a little bit different from the original issue: "Map doesn't zoom in to a single waypoint location unless Add stop button is clicked."

Ah, maybe I misunderstood. I was referring to the issue in this comment which you linked in the original post: #25161 (comment). This currently happens on iOS native.

2. The MapView.Camera not updating when the map view is not visible on screen.

This is actually not true. The map is still mounted even when the waypoint editor page (the page with text input for address) is visible. The iOS maps used to work normally (cc: @allroundexperts) so this probably is not the reason.

This is what I observed based on my testing. The map remains mounted (I verified this by subscribing to available map events, etc), but the camera operations (zoom, move, fly) won't work.

I have a branch with my Solution 2 (which is bad because setTimeout is bad) applied if you'd like take a look. With my changes, the behaviour matches the videos for web that you included in the first post. If the maps previously worked properly on iOS native, then maybe something else has changed? After spending quite a number of hours investigating yesterday, I wasn't able to identify anything else that could be causing the camera not to update.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[akinwale]

@hayata-suenaga @allroundexperts

To summarise the fixes to get the map to work as expected:

1. A change is required in MapView.tsx (react-native-x-maps) to remove the flyTo and zoomTo calls, and add a setCamera call with the centerCoordinate and a zoomLevel. This will also fix Android - IOU Distance - Map does not zoom in to the selected location on Start point #25871 which happens on Android native.

2. Use one of the solutions from my proposal for iOS. We can create a separate implementation of the DistanceRequest component for just iOS using index.ios.js and apply the necessary changes there. I have included a demo video showing Solution 2 (setTimeout after the screen containing the map is in focus) in action on the iOS simulator.

25732-demo-2.mp4

[melvin-bot]

📣 @gegham-khachatryan! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[gegham-khachatryan]

Contributor details
Your Expensify account email: gegham.k90@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~0114efdf1fe8fd88a6

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[gegham-khachatryan]

Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2023-08-25.at.01.26.12.mp4

[melvin-bot]

Upwork job price has been updated to $2000

[hayata-suenaga]

doubled the price 💰

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.62-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Map panning fix (2) #26500

If no regressions arise, payment will be issued on 2023-09-11. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @gegham-khachatryan requires payment
• @allroundexperts does not require payment (Eligible for Manual Requests)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@NicMendonca] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[hayata-suenaga]

@hayata-suenaga Can you explain what this means? ⬆️ ⬆️
Thank you

I found this on Upwork:
Payment timelines are all based on the day the contributor has an accepted proposal and is assigned to the Github issue

Merged PR within 3 business days - 50% bonus
Merged PR within 6 business days - 0% bonus
Merged PR within 9 business days - 50% penalty
No PR within 12 business days - Contract terminated

The percentages are against the original offer price. @NicMendonca gonna be back with you about payment after the regression period is over.

[NicMendonca]

• Reporter: N/A
• Contributor: @gegham-khachatryan - $2,000
• C+: @allroundexperts - $2,000
• Urgent help reviewing PR: @parasharrajat - $500

[NicMendonca]

@gegham-khachatryan sent you the offer in Upwork

@allroundexperts @parasharrajat can you please request payment via Expensify?

[parasharrajat]

Did we open a new PR for follow-up changes? #25977 (comment)

I wouldn't call that a new issue or out of the scope of this issue as those are code suggestions and technically must be solved in the PR itself. (part of documented C+ review process). cc: @allroundexperts

[parasharrajat]

Payment requested as per #25732 (comment)

[gegham-khachatryan]

@parasharrajat not yet, I'm on vacation.

[NicMendonca]

@allroundexperts don't forget to request payment!

@parasharrajat am I keeping this open until this happens?

Did we open a new PR for follow-up changes? #25977 (comment)

I wouldn't call that a new issue or out of the scope of this issue as those are code suggestions and technically must be solved in the PR itself. (part of documented C+ review process). cc: @allroundexperts

[allroundexperts]

Requested payment.

[gegham-khachatryan]

@parasharrajat The changes you've requested aren't relevant to the current issue. Initially, this code was housed in a separate library, but I received a request to simply move it from the library to the app. However, addressing these changes is not within the scope of the current issue or my assigned task.
@NicMendonca

[parasharrajat]

We have a standard for code being merged to the main repo. and any code suggestion to adhere to that is counted in the scope of each issue. I understand that you moved the code from some repo to our App but now it is part of the app so it should follow best practices. We skipped these suggestions in the main PR due to the urgency of the task at hand.

In general case, those changes should have been followed before merging the PR.

Anyways, I leave that decision to @luacmartins @allroundexperts

[gegham-khachatryan]

I'd like to clarify that the repository is an internal company repository, not a third-party one. You can find it at the following URL: https://github.com/Expensify/react-native-x-maps

[allroundexperts]

@parasharrajat I think @gegham-khachatryan has a very fair point. Initially, this ticket was just about fixing the map panning behaviour. However, as we went into the PR stage, it was decided that we should just simply stop using the library and include all its code in the main repository. In my opinion, the expectation was to just move the library code into the main repository without any refactoring of the actual library code.

I would also like to highlight that we went above and beyond the scope of this issue and made several other un-related fixes as well since this was such an important feature for our release. Ref, Ref

[parasharrajat]

Thanks for making extra efforts to get this done quickly.

No worries, I will create the PR for those suggestions as soon as I get the time to do so.

[allroundexperts]

We can create a follow up issue.

[NicMendonca]

Cool! Thanks everyone!

@allroundexperts don't forget BZ checklist please!

[parasharrajat]

Started a PR #27507 for follow up changes.

[allroundexperts]

Checklist

1. The PR that introduced the bug has been identified. Link to the PR: Implement basic map view react-native-x-maps#16
2. The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/react-native-x-maps/pull/16/files#r1328141633
3. A Slack discussion is not needed here. I think the initial PR had this issue and we intentionally let it merge to the main (behind a feature flag) so people can test this out.
4. A regression test would be helpful here.

Regression test steps

1. On Android or iOS app, click the FAB menu in the LHN.
2. Request money and select the distance tab.
3. Verify that the map is panned to San Francisco.
4. Enter the starting point as: "585 Castro Street, San Francisco" and save.
5. Verify that the map is panned to the correct start location.
6. Enter the ending address as: "88 Kearney Street, San Francisco" and save.
7. Verify that the map is panned to the correct end location.

Do we 👍 or 👎 ?

[NicMendonca]

Thanks!

Reporter: N/A
Contributor: @gegham-khachatryan - $2,000
C+: @allroundexperts - $2,000
Urgent help reviewing PR: @parasharrajat - $500

[JmillsExpensify]

$500 payment for @parasharrajat approved based on BZ summary.

[JmillsExpensify]

$2,000 payment for @allroundexperts approved based on BZ summary.