Fix map distance unit getting stuck on wrong value in 1:1 chat#84864
Fix map distance unit getting stuck on wrong value in 1:1 chat#84864luacmartins merged 4 commits intomainfrom
Conversation
In MapView and MapViewImpl, the useEffect that syncs the unit prop to local distanceUnit state had a flawed guard condition that prevented updates once distanceUnit had any truthy value. This caused the map to show the wrong unit (e.g. km instead of mi) when the correct unit arrived on a subsequent render. Replace the guard with a userToggledUnit ref so the unit prop is always respected unless the user has explicitly toggled the unit on the map. Co-authored-by: Mario Mazzotti <MarioExpensify@users.noreply.github.com>
…iewImpl.website.tsx Co-authored-by: Mario Mazzotti <MarioExpensify@users.noreply.github.com>
|
Fixed the failing |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
melvin-bot
bot
requested review from
Ollyws and
trjExpensify
and removed request for
a team
|
Bug - No product review needed. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/components/MapView/MapView.tsx
Outdated
| const [isAccessTokenSet, setIsAccessTokenSet] = useState(false); | ||
|
|
||
| const [distanceUnit, setDistanceUnit] = useState(unit); | ||
| const userToggledUnit = useRef(false); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The useState(unit) + useRef(false) + useEffect sync + toggleDistanceUnit pattern is duplicated identically in both MapView.tsx and MapViewImpl.website.tsx. This duplication means any future bug fix or behavior change must be applied in two places, increasing maintenance overhead and regression risk.
Extract the shared logic into a custom hook, for example:
// useDistanceUnit.ts
function useDistanceUnit(unit: string | undefined) {
const [distanceUnit, setDistanceUnit] = useState(unit);
const userToggledUnit = useRef(false);
useEffect(() => {
if (!unit || userToggledUnit.current) {
return;
}
setDistanceUnit(unit);
}, [unit]);
const toggleDistanceUnit = () => {
userToggledUnit.current = true;
setDistanceUnit((currentUnit) =>
currentUnit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_KILOMETERS
? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES
: CONST.CUSTOM_UNITS.DISTANCE_UNIT_KILOMETERS,
);
};
return {distanceUnit, toggleDistanceUnit};
}Then use const {distanceUnit, toggleDistanceUnit} = useDistanceUnit(unit); in both components.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Extracted the shared logic into a useDistanceUnit custom hook in src/components/MapView/useDistanceUnit.ts. Both MapView.tsx and MapViewImpl.website.tsx now use const {distanceUnit, toggleDistanceUnit} = useDistanceUnit(unit);.
| const {isOffline} = useNetwork(); | ||
| const {translate} = useLocalize(); | ||
| const [distanceUnit, setDistanceUnit] = useState(unit); | ||
| const userToggledUnit = useRef(false); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This is the same duplicated useRef(false) + useEffect sync + toggleDistanceUnit pattern that also exists in MapView.tsx. See the comment on MapView.tsx for the suggested shared useDistanceUnit hook extraction.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Addressed — this file now uses the shared useDistanceUnit hook, same as MapView.tsx.
|
Bumped @Ollyws for review, if they are not available I'll look for another C+ |
|
@MelvinBot please implement the suggested changes. |
Deduplicate the useState + useRef + useEffect + useCallback pattern for distance unit management that was identically repeated in MapView.tsx and MapViewImpl.website.tsx into a single useDistanceUnit custom hook. Co-authored-by: Mario Mazzotti <MarioExpensify@users.noreply.github.com>
melvin-bot
bot
requested review from
MarioExpensify and
eh2077
and removed request for
a team
|
@eh2077 @MarioExpensify One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Co-authored-by: Mario Mazzotti <MarioExpensify@users.noreply.github.com>
|
Fixed the failing Prettier check: reordered imports in |
|
@codex review |
|
The previously failing checks have been resolved by the recent commits and all checks are now passing:
All checks — ESLint, typecheck, tests (all 8 jobs), builds, prettier, storybook, perf-tests, spellcheck — are now green. ✅ |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / SafariMacOS_Chrome.mov |
…tStaleState Fix map distance unit getting stuck on wrong value in 1:1 chat (cherry picked from commit a674b9e) (cherry-picked to staging by luacmartins)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @luacmartins 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! 🧪🧪
|
5 participants
Explanation of Change
Both
MapView.tsxandMapViewImpl.website.tsxmaintain a localdistanceUnitstate that syncs from theunitprop via auseEffect. The guard conditionif (!unit || distanceUnit)was preventing the state from ever updating once it had a truthy value. This meant that if the component initially received the wrong unit (e.g.,kmfrom the active policy fallback before the transaction's P2P rate loaded), it would get stuck on that value even when the correctmiunit arrived on subsequent renders.The fix adds a
userToggledUnitref that tracks whether the user has manually toggled the unit on the map. TheuseEffectnow always syncs from theunitprop unless the user has explicitly toggled, preserving the toggle feature while allowing legitimate prop updates to take effect.Fixed Issues
$ #84801
PROPOSAL: #84801 (comment)
Tests
Offline tests
N/A — This change only affects distance unit display on the map, which requires an online connection to render (Mapbox).
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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