[HOLD for payment 2023-11-17] [$500] Distance - Route Rerender and Waypoints grey out when add an empty stop

[kbecciv]

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: 1.3.89.3
Reproducible in staging?: y
Reproducible in production?: y
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: @rakshitjain13
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697893765172309

Action Performed:

1. Click '+' icon on LHS Bar
2. Click on Request Money
3. Select Distance Tab
4. Click on Start and fill Mumbai , Maharashtra , India
5. Click on Finish and fill Delhi ,India
6. Click on Add Stop
7. Don't type anything , just click on Save and Observe

Expected Result:

Route should not get re-render and waypoints fields should not be grey out

Actual Result:

Route getting fetched again and waypoints fields become grey out

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Android: Native

Screen.Recording.2023-10-21.at.6.52.02.PM.1.mov

Android: mWeb Chrome

Screen.Recording.2023-10-21.at.6.55.29.PM.mov

iOS: Native

Screen.Recording.2023-10-21.at.6.48.09.PM.mov

iOS: mWeb Safari

Screen.Recording.2023-10-21.at.6.48.09.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2023-10-21.at.6.49.52.PM.mov

Screen.Recording.2023-10-21.at.6.48.09.PM.mov

Recording.5109.mp4

MacOS: Desktop

Screen.Recording.2023-10-21.at.6.46.15.PM.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0133cf45668e492c7f
• Upwork Job ID: 1716448459730763776
• Last Price Increase: 2023-10-30
• Automatic offers:
• paultsimura | Contributor | 27480023
• rakshitjain13 | Reporter | 27480027

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0133cf45668e492c7f

[melvin-bot]

Triggered auto assignment to @sakluger (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]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

[kbecciv]

Proposal by: @rakshitjain13
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697893765172309

Proposal

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

Route Rerender and Waypoints grey out when add an empty stop

What is the root cause of that problem?

In src/pages/iou/WaypointEditor.js we allow empty waypoint and do a removeWaypoint Transaction

App/src/pages/iou/WaypointEditor.js

Lines 136 to 138 in fcee207

	if (waypointValue === '') {
	Transaction.removeWaypoint(transactionID, waypointIndex);
	}

In removeWaypoint function , we update the onyx state and set coordinates to null

App/src/libs/actions/Transaction.ts

Lines 104 to 155 in fcee207

	function removeWaypoint(transactionID: string, currentIndex: string) {
	// Index comes from the route params and is a string
	const index = Number(currentIndex);
	const transaction = allTransactions?.[transactionID] ?? {};
	const existingWaypoints = transaction?.comment?.waypoints ?? {};
	const totalWaypoints = Object.keys(existingWaypoints).length;
	const waypointValues = Object.values(existingWaypoints);
	const removed = waypointValues.splice(index, 1);
	const isRemovedWaypointEmpty = removed.length > 0 && !TransactionUtils.waypointHasValidAddress(removed[0] ?? {});
	// When there are only two waypoints we are adding empty waypoint back
	if (totalWaypoints === 2 && (index === 0 || index === totalWaypoints - 1)) {
	waypointValues.splice(index, 0, {});
	}
	const reIndexedWaypoints: WaypointCollection = {};
	waypointValues.forEach((waypoint, idx) => {
	reIndexedWaypoints[`waypoint${idx}`] = waypoint;
	});
	// Onyx.merge won't remove the null nested object values, this is a workaround
	// to remove nested keys while also preserving other object keys
	// Doing a deep clone of the transaction to avoid mutating the original object and running into a cache issue when using Onyx.set
	let newTransaction: Transaction = {
	...transaction,
	comment: {
	...transaction.comment,
	waypoints: reIndexedWaypoints,
	},
	};
	if (!isRemovedWaypointEmpty) {
	newTransaction = {
	...newTransaction,
	// Clear any errors that may be present, which apply to the old route
	errorFields: {
	route: null,
	},
	// Clear the existing route so that we don't show an old route
	routes: {
	route0: {
	distance: null,
	geometry: {
	coordinates: null,
	},
	},
	},
	};
	}
	Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, newTransaction);
	}

Now in src/components/DistanceRequest/index.js we check if the route is present or not.

App/src/components/DistanceRequest/index.js

Line 86 in fcee207

const hasRoute = TransactionUtils.hasRoute(transaction);

As in removeWaypoint we have removed route the below situation become true and leads to refetch

App/src/components/DistanceRequest/index.js

Lines 90 to 91 in fcee207

	const isRouteAbsentWithoutErrors = !hasRoute && !hasRouteError;
	const shouldFetchRoute = (isRouteAbsentWithoutErrors || haveValidatedWaypointsChanged) && !isLoadingRoute && _.size(validatedWaypoints) > 1;

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

We should not to do any transaction on empty waypoint thus this can be done by removing these lines

App/src/pages/iou/WaypointEditor.js

Lines 136 to 138 in fcee207

	if (waypointValue === '') {
	Transaction.removeWaypoint(transactionID, waypointIndex);
	}

Solution Video:
https://github.com/Expensify/App/assets/54314936/2dcee9f5-a7fe-4c14-9867-60b25656c4f5

What alternative solutions did you explore? (Optional)

We can cache the distance Coordinates and use that again if waypoints didn't change

[paultsimura]

Proposal

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

When adding an empty waypoint, existing waypoints flicker, and the route re-renders.

What is the root cause of that problem?

We are executing a transaction Onyx operation on waypoint removal here:

App/src/pages/iou/WaypointEditor.js

Lines 136 to 138 in 1c6b610

	if (waypointValue === '') {
	Transaction.removeWaypoint(transactionID, waypointIndex);
	}

This piece of code was added in order to remove existing waypoints (when modifying them) by removing the address value. Therefore, it should not be removed.

Inside this transaction action, we are checking if the removed waypoint was not empty. And if it wasn't – we remove the route from the transaction to trigger the route re-fetch.

If we add an empty waypoint, the removed.length === 0 here, so the isRemovedWaypointEmpty is set to false:

App/src/libs/actions/Transaction.ts

Line 113 in 01016dc

const isRemovedWaypointEmpty = removed.length > 0 && !TransactionUtils.waypointHasValidAddress(removed[0] ?? {});

Which results in clearing the transaction route here:

App/src/libs/actions/Transaction.ts

Lines 136 to 153 in 01016dc

	if (!isRemovedWaypointEmpty) {
	newTransaction = {
	...newTransaction,
	// Clear any errors that may be present, which apply to the old route
	errorFields: {
	route: null,
	},
	// Clear the existing route so that we don't show an old route
	routes: {
	route0: {
	distance: null,
	geometry: {
	coordinates: null,
	},
	},
	},
	};
	}

Route re-render happens here because the route was removed:

App/src/components/DistanceRequest/index.js

Lines 86 to 91 in 844b3b9

	const hasRoute = TransactionUtils.hasRoute(transaction);
	const validatedWaypoints = TransactionUtils.getValidWaypoints(waypoints);
	const previousValidatedWaypoints = usePrevious(validatedWaypoints);
	const haveValidatedWaypointsChanged = !_.isEqual(previousValidatedWaypoints, validatedWaypoints);
	const isRouteAbsentWithoutErrors = !hasRoute && !hasRouteError;
	const shouldFetchRoute = (isRouteAbsentWithoutErrors || haveValidatedWaypointsChanged) && !isLoadingRoute && _.size(validatedWaypoints) > 1;

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

We should add an early return in case no waypoint was removed in Transaction.removeWaypoint here:

    const removed = waypointValues.splice(index, 1);
+    if (removed.length === 0) {
+        return;
+    }

App/src/libs/actions/Transaction.ts

Lines 111 to 113 in 01016dc

	const waypointValues = Object.values(existingWaypoints);
	const removed = waypointValues.splice(index, 1);
	const isRemovedWaypointEmpty = removed.length > 0 && !TransactionUtils.waypointHasValidAddress(removed[0] ?? {});

This will ensure that we do not modify the route and do not do all the useless re-indexing calculations along with merging the transaction at the end of the function (as no waypoints were actually changed):

App/src/libs/actions/Transaction.ts

Line 154 in 01016dc

Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, newTransaction);

Result:

screen-recording-2023-10-23-at-163106_tu1cjbMf.mp4

What alternative solutions did you explore? (Optional)

We can modify the logic:

-    const isRemovedWaypointEmpty = removed.length > 0 && !TransactionUtils.waypointHasValidAddress(removed[0] ?? {});
+    const isRemovedWaypointEmpty = removed.length === 0 || !TransactionUtils.waypointHasValidAddress(removed[0] ?? {});

This will stop the route from being removed, but the unnecessary Onyx.set operation at the end of the function will still be called.

[DylanDylann]

Proposal

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

Distance - Route Rerender and Waypoints grey out when add an empty stop

What is the root cause of that problem?

Let's see this logic

App/src/pages/iou/WaypointEditor.js

Lines 135 to 138 in 4430ae2

	// Allows letting you set a waypoint to an empty value
	if (waypointValue === '') {
	Transaction.removeWaypoint(transactionID, waypointIndex);
	}

Currently, with this logic, the user can remove the waypoint by clearing the search input and clicking save button.

Other way, this logic causes this bug because removeWaypoint function make the route is fetch again

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

In here

App/src/pages/iou/WaypointEditor.js

Lines 135 to 138 in 4430ae2

	// Allows letting you set a waypoint to an empty value
	if (waypointValue === '') {
	Transaction.removeWaypoint(transactionID, waypointIndex);
	}

We only run removeWaypoint if the user edits the old waypoint. If user click add stop button (create new waypoint) and leave the input empty and click save, we will don't run removeWaypoint function

Note that: Current logic, we have Start point (index: 0) and finish point (index: 1), when clicking Add Stop the App will redirect to WaypointEditor with index: 2

So that if waypointIndex < _.size(allWaypoints) It mean that user are editing old waypoint.

Update code like this

if (waypointValue === '' && waypointIndex < _.size(allWaypoints)) {
            Transaction.removeWaypoint(transactionID, waypointIndex);
        }

What alternative solutions did you explore? (Optional)

[neil-marcellini]

Removing from the project since it's a very minor bug and more related to performance (preventing re-renders)

[melvin-bot]

@sakluger, @eVoloshchak Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sakluger]

Hey @eVoloshchak, looks like we have a few proposals to review, mind taking a look on Monday?

[eVoloshchak]

I think we should proceed with @paultsimura's proposal
This is the most simple and straightforward approach

🎀👀🎀 C+ reviewed!

[melvin-bot]

Triggered auto assignment to @NikkiWines, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[dukenv0307]

@eVoloshchak @NikkiWines Sorry for my late. But I want to propose a solution from another perspective

Proposal

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

Distance - Route Rerender and Waypoints grey out when adding an empty stop

What is the root cause of that problem?

I see that while user are online, the save button is useless and we need to hide it if user are online

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

We should disable the save button if search input is empty. It also resolve this bug

What alternative solutions did you explore? (Optional)

We should hide the save button if user online. It also resolve this bug

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[NikkiWines]

Oh, that's an interesting point about the Save button. When an address is added, the user doesn't actually click Save (on any platform, at least from my tests) to use the address. So, the only reason the user would press that button is if the value is empty or the user is offline.

I don't think we should completely hide the button, but I think it's more in line with our existing forms to disable that button when there's no value entered instead of allowing the user to "save" a value that doesn't exist.

If the re-render issue still exists when using < to go back (which it doesn't seem to), we could implement one of the above proposals.

@sakluger @neil-marcellini @eVoloshchak, what do you think?

[paultsimura]

@NikkiWines handling an empty waypoint was implemented specifically to allow removing existing waypoints (in edit mode) by entering an empty value. It's a handy feature, and disabling the "save" button for empty values will basically remove this feature, won't it?

Screen.Recording.2023-10-30.at.22.45.40-compressed.mp4

[NikkiWines]

Ah, good point @paultsimura, that's fair. Though, IMO, that's a bit of an odd flow to go through to remove a waypoint. But I wasn't involved in the design for this feature, so I'll lean on @neil-marcellini's expertise here to determine how we want to move forward.

[paultsimura]

that's a bit of an odd flow to go through to remove a waypoint

Just my 5 cents here: I have been using this feature a lot during development – it's much easier to remove the address & submit than to click three dots – remove the waypoint – confirm the popup.

[melvin-bot]

📣 @rakshitjain13 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[paultsimura]

Thanks. The PR is ready for review: #30754

[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.

[NikkiWines]

Doesn't appear to be the root cause of the blocker 👍

[paultsimura]

@NikkiWines the PR is in production already, but the automation didn't update the [HOLD for payment] title, could you please check? 🙏

[NikkiWines]

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

• fix: Distance: prevent unnecessary operations when adding an empty stop #30754

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

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:

• @eVoloshchak does not require payment (Eligible for Manual Requests)
• @paultsimura requires payment offer (Contributor)
• @rakshitjain13 requires payment via Upwork (Reporter)

[NikkiWines]

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:

• [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
• [@eVoloshchak] 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:
• [@eVoloshchak] 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:
• [@eVoloshchak] Determine if we should create a regression test for this bug.
• [@eVoloshchak] 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.
• [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[NikkiWines]

Not sure why it didn't get updated, looks like everything was linked appropriately. Added all the requisite details

[rakshitjain13]

@NikkiWines Friendly Bump

[NikkiWines]

@sakluger can you issue payments here please 🙇

[sakluger]

Sorry for the delay! I've paid out the reporter and contributor. @eVoloshchak mind finishing the BZ checklist then requesting payment via manual request?

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: fix: prevent a route refresh if the list of valid waypoints have not changed #27496
• 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/App/pull/27496/files#r1402251064
• 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: I don't think an additional discussion is needed in this case
• Determine if we should create a regression test for this bug.
  Is it easy to test for this bug? Yes
  Is the bug related to an important user flow? Yes
  Is it an impactful bug? No
  The bug has near-zero impact, doesn't break the UX and doesn't prevent user from using the app, I think a regression test is not needed here

[JmillsExpensify]

Issue is missing a payment summary.

[sakluger]

@JmillsExpensify here you go!

Summarizing payouts for this issue:

Reporter: @rakshitjain13 $50 (paid via Upwork)
Contributor: @paultsimura $500 (paid via Upwork)
Contributor+: @eVoloshchak $500 (requires payment via Manual Request)

[JmillsExpensify]

$500 payment approved for @eVoloshchak based on comment above.