Closed m-natarajan closed 3 months ago
Triggered auto assignment to @JmillsExpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989
What is this issue waiting on? It's already been reproduced by me and Applause.
@JmillsExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!
Bump; what are next steps @JmillsExpensify ?
Ah missed this. Let me spin up an Android device in Browserstack to test.
Hmm, I'm locked out of testing, so I'm going to add the external label so the community might re-produce the same.
Job added to Upwork: https://www.upwork.com/jobs/~01b4f219078951c444
Triggered auto assignment to Contributor-plus team member for initial proposal review - @c3024 (External
)
@c3024 can you give an ETA please? This is super urgent, thanks!
@Ollyws , coming from here, you're able to reproduce and would like to be assigned, right? If so, please comment and I'll assign.
Yeah go ahead thanks @mallenexpensify
Upwork job price has been updated to $500
π£ @Ollyws π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
Done, raised price to $500 since it's critical. Thx @Ollyws
This is reproducable for me just by clearing the app data (and thereby removing the location permission), it also seems to happen on any distance request not specifically track expense.
@Ollyws thanks for taking this on, what's your ETA?
Unfortunately we have no proposals yet, but as soon as we get a decent proposal we should have it merged in a day or two. I'll post on slack to try and get some proposals coming in.
Hm, no luck on proposals so far, what's the next step?
Given the urgency perhaps we should up the bounty to see if we can get some proposals coming in.
Upwork job price has been updated to $1000
Bumped to $1000. If it doesn't get picked up by early next week we can check with an agency to see if they might be able to help.
[P2P Distance] Opens to ocean on first load
There are several factors causing this issue.
The main cause remains unknown, but it occurs when the flyTo method is called during the initial app installation. The problem's source is unclear, but I am unable to replicate it in the mapbox-map-android library.
Within the rnmapbox setInitialCamera function, the CameraMode and duration are hardcoded, causing defaultSettings.animationDuration
and defaultSetting.animationMode
to be disregarded during the initial rendering, preventing changes from the react-native side.
If we want to make changes in the upstream, we can suggest to the rnmapbox team:
Option 1. Eliminate the hardcoded value in setInitialCamera function. This way, Expensify will utilize defaultSetting.animationMode='easeTo'
instead of none
, and using easeTo
works effectively.
Or Option 2. Modify the flyTo in this code to map.setCamera(mCameraUpdate)
, where the duration is always 0, eliminating the need for animation.
This is workaround:
Updating @rnmapbox/maps to the latest version (10.1.19)
And set the centerCoordinate
directly in Mapbox.Camera
will resolve the issue
<Mapbox.Camera
ref={cameraRef}
defaultSettings={{
centerCoordinate: currentPosition ? [currentPosition.longitude, currentPosition.latitude] : initialState?.location,
zoomLevel: initialState?.zoom,
}}
centerCoordinate ={currentPosition ? [currentPosition.longitude, currentPosition.latitude] : initialState?.location}
/>
In this case, can we hold on RN upgrade?
Proposal Updated
I can confirm bump rnmapbox
doesn't solve the issue.
Add workaround to the proposal
@wildan-m Thanks for the proposal but your solution of adding centerCoordinate
is not fixing the issue for me.
The upstream issue seems to have been reported here https://github.com/rnmapbox/maps/issues/3449 but it's been sitting there for two weeks with no activity. Perhaps we could patch it ourselves?
The map opens to the incorrect coordinates on first app load.
We are setting the currentPosition
in useCallback
hooks, e.g. here:
We don't have currentPosition
as a dependency, which seems to be causing a race condition. If you set the coordinates directly here:
e.g. using centerCoordinate: initialState?.location
, then it loads fine.
Add currentPosition
to the dependency array of both useCallback
hooks, here:
and here:
@Ollyws I couldn't reproduce the upstream issue locally in the app, e.g. by setting centerCoordinate: initialState?.location
here:
e.g. using centerCoordinate: initialState?.location, then it loads fine.
@jjcoffee This works for you? For me it still opens to the ocean:
https://github.com/Expensify/App/assets/11609254/b22fbccd-a180-4ed0-8429-d2a2d31fde29
This works for you? For me it still opens to the ocean
@Ollyws Sorry I should've been more specific there :smile: It works if you reject the location permission request, otherwise there's another useEffect
that gets triggered that has the same issue I think (so it's initially set to initialState?.location
and then gets set again).
Ah I see yeah it seems to work for me with a fixed location suggesting it's not an upstream issue.
I think that's enought to be going on with, let's move forward with @jjcoffee's solution.
πππ C+ reviewed
Triggered auto assignment to @luacmartins, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@Ollyws using @jjcoffee solution, have you tried to clear storage/uninstall the app first? I think that will give different result. I can reproduce it in rnmapbox example
@Ollyws
Proposal Updated
My solution will work in combination of bumping the rnmapbox version to 10.1.19
@Ollyws
Proposal Updated
The root cause has been found in the upstream Add new main solution Move prev solution to alternative
@JmillsExpensify @Ollyws @luacmartins this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
Actually on further investigation it doesn't seem to be loading correctly with a fixed location, let's lay off any assignment while I investigate this further.
Actually on further investigation it doesn't seem to be loading correctly with a fixed location
@Ollyws Hmm interesting, how are you testing that?
@jjcoffee Just replacing centerCoordinate: [-122.083922,37.4220936],
in the defaultSettings
. Those are the coordinates for San Fransico.
@Ollyws It works fine for me if I comment out our additional code, i.e. lines 74-124. This is how it looks after doing that and setting centerCoordinate: [-122.083922,37.4220936]
(and deleting app data).
https://github.com/Expensify/App/assets/27287420/ada0ef8a-6753-425b-be28-f2467484de37
@jjcoffee Strange, I'm getting different behaviour. Are you testing on an emulator or real device?
Updating the mapbox version and adding centerCoordinate ={currentPosition ? [currentPosition.longitude, currentPosition.latitude] : initialState?.location}
as suggested by @wildan-m is working well for me, I'm thinking this may be sufficient until this gets fixed upstream.
@jjcoffee try clearing storage (not cache), and select 'Don't allow' when it asks for location permission.
@Ollyws My proposal updated.
Previous queue solution isn't working after retesting, but I have another suggestion for a potential upstream fix.
Updating the mapbox version and adding centerCoordinate ={currentPosition ? [currentPosition.longitude, currentPosition.latitude] : initialState?.location} as suggested by @wildan-m is working well for me, I'm thinking this may be sufficient until this gets fixed upstream.
I agree with this; some of their examples also fulfill both values or only use Camera.centerCoordinate
without defaultSettings.centerCoordinate
.I'm not sure why, but there might be a reason.
<Camera
defaultSettings={{ centerCoordinate: basePosition, zoomLevel: 15 }}
centerCoordinate={basePosition}
zoomLevel={15}
/>
<Camera
zoomLevel={zoomUnderTest}
centerCoordinate={coordinatesUnderTest}
animationMode="none"
animationDuration={0}
/>
@jjcoffee try clearing storage (not cache), and select 'Don't allow' when it asks for location permission.
@wildan-m I've tried all location permission options and always clear the storage fully (I can repro the issue itself consistently pre-changes). Not sure what could be causing the inconsistency!
Ok thanks for the discussion, I think the best option here is going with @wildan-m's suggestion of updating the mapbox version and adding centerCoordinate = {currentPosition ? [currentPosition.longitude, currentPosition.latitude] : initialState?.location}
, especially as this is the way mapbox is implemented in the official examples.
πππ C+ reviewed
cc: @luacmartins
Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
π£ @wildan-m π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
Woohoo! @wildan-m can you please give an ETA for completion? Thanks!
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: v1.4.62-5 Reproducible in staging?: y Reproducible in production?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @quinthar Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712974090053049
Action Performed:
Expected Result:
Should not show the location in the ocean
Actual Result:
Shows somewhere in the middle of the ocean
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/38435837/b70083c1-9763-497a-ad13-9bdc50d423c7
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @JmillsExpensifyIssue Owner
Current Issue Owner: @JmillsExpensify