[PAYMENT - 01-16: $1000] Android - Console error when any report is opened - reported by @aneequeahmad

[mvtglobally]

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

---

Action Performed:

1. Launch android app.
2. Click on any report.

Expected Result:

No console errors

Actual Result:

Console error Warning: Failed prop type: The prop anchorPosition.horizontal is marked as required in PopoverWithMeasuredContent, but its value is undefined.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Android

Version Number: 1.1.94-3
Reproducible in staging?: need repro
Reproducible in production?: need repro
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:
Issue reported by: @aneequeahmad
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1661901563422729

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01064e852643baf5f4
• Upwork Job ID: 1610433379500457984
• Last Price Increase: 2023-01-04

[melvin-bot]

Triggered auto assignment to @abekkala (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[michaelhaxhiu]

@mvtglobally were you not able to reproduce this one when you tried on android?

[mvtglobally]

@michaelhaxhiu when I use physical device, I can see the console. So I am not sure if it's repro or not

[parasharrajat]

This is not a bug. More of proptype warning which doesn't affect any of the app features.

@michaelhaxhiu, may be discuss it internally if this needs to exported. It can be fixed in no time.

[abekkala]

If we're not able to reproduce to confirm a bug then I'm going to close this for now.

[kavimuru]

@abekkala There is another bug reported https://expensify.slack.com/archives/C01GTK53T8Q/p1665657234159879
Do you want me reopen this bug?

[aneequeahmad]

@kavimuru this is still reproducible in v1.2.20-0. Please see the attached video:

Video:

bug-console-android.mov

[aneequeahmad]

Action Performed:

• Launch android app.
• Click on any report having attachment like image.
• Long press on attachment(image) message.

Expected Result:

• No console errors

Actual Result:

• Console error Warning: Failed prop type: The prop anchorPosition.horizontal is marked as required in PopoverWithMeasuredContent, but its value is undefined.

PROPOSAL

Problem:

As anchorPosition is set to isRequired in component PopoverWithMeasuredContent which is not needed in every case.

App/src/components/PopoverWithMeasuredContent.js

Lines 18 to 22 in 66a05aa

	/** The horizontal and vertical anchors points for the popover */
	anchorPosition: PropTypes.shape({
	horizontal: PropTypes.number.isRequired,
	vertical: PropTypes.number.isRequired,
	}).isRequired,

Solution:

anchorPosition should not be set to isRequired cuz we don't need to pass prop anchorPosition to PopoverWithMeasuredContent like anchorPosition is not set to isRequired in other components. for example

• AddPaymentMethodMenu

App/src/components/AddPaymentMethodMenu.js

Lines 15 to 21 in d6ad76f

	const propTypes = {
	isVisible: PropTypes.bool.isRequired,
	onClose: PropTypes.func.isRequired,
	anchorPosition: PropTypes.shape({
	top: PropTypes.number,
	left: PropTypes.number,
	}),

CODE CHANGE:

FROM

App/src/components/PopoverWithMeasuredContent.js

Lines 18 to 22 in 66a05aa

	/** The horizontal and vertical anchors points for the popover */
	anchorPosition: PropTypes.shape({
	horizontal: PropTypes.number.isRequired,
	vertical: PropTypes.number.isRequired,
	}).isRequired,

TO:

    anchorPosition: PropTypes.shape({
        horizontal: PropTypes.number,
        vertical: PropTypes.number,
    }),

[aneequeahmad]

This is still reproducible on dev main branch v1.2.25-0

VIDEO:

bug-console-log.mov

cc: @abekkala

[aneequeahmad]

@mvtglobally, This error is reproducible on main branch dev in version 1.2.29-6.

[kavimuru]

@abekkala , @aneequeahmad is still able to reproduce on dev main v1.2.39-0. I am reopening this bug again.

[abekkala]

It seems to be on dev only, correct? not reproducible on staging/production?

[aneequeahmad]

@abekkala Yes, it is dev only not reproducible on staging/production.

[melvin-bot]

A Contributor Manager will be assigned to issue payment via Upwork if we deploy an associated Pull Request to production. Per Contributing.md.

[melvin-bot]

Triggered auto assignment to @deetergp (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[abekkala]

@deetergp based on this convo
Can I get your thoughts here?

[melvin-bot]

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

[deetergp]

After some internal discussion, here's where were at with this:

• This only shows in dev because the logging of this kind is only shown in dev.
• This warning doesn't happen in release builds because we turn off propType checking for better performance
• If we did check types, it would warn in staging & production, but wouldn't be shown anywhere due to being sent to the error boundary
• The warn is indicative of an issue and should probably be fixed.
  (h/t to @roryabraham for filling in the gaps for me)

Now, with all that said, I'm not sure that removing the propType requirement for the anchorPosition is the correct solution. @aneequeahmad, can you explain why should this be optional here, or any of the other components you cross-checked?

[jliexpensify]

Going to unassign @johncschuster since it looks like we both got added. I'll go ahead and hire @Ollyws and @aimane-chnaif

[jliexpensify]

@Ollyws can you link me your Upworks profile?

[trjExpensify]

👋 @Ollyws needs to be assigned to this issue right? Is there a PR to link as well, yet?

[melvin-bot]

📣 @Ollyws You have been assigned to this job by @JmillsExpensify!
Please apply to this job in Upwork 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 📖

[JmillsExpensify]

@deetergp @abekkala @johncschuster @jliexpensify Pretty please make sure that your issues correctly have the bug label on them if we're actively looking for proposals. Bug was just added to this issue, so before it was not on the WAQ initiative radar, but now it is. Then given that the bug is well over 30 days old, it's immediately something that we've got to prioritize as a result.

[aimane-chnaif]

@JmillsExpensify For new feature issues, Bug label should be added as well if we're actively looking for proposals?
i.e. #12698 has lack of proposals maybe because of Bug label not applied.

[aimane-chnaif]

@Ollyws you're assigned now. please feel free to raise PR

[aneequeahmad]

@aimane-chnaif, @deetergp I'm still not clear how can we add the wrong/irrelevant data just to make the data consistent and this pageX and pageY property is not provided by the react-native-gesture-handler for LongPressGestureHandler.

Moreover, PageX and PageY and different from absoluteX and absoluteY. Value of x, y and absoluteX and absoluteY are alike and absoluteX and absoluteY value can be used instead of x and y as written in the doc.

[aimane-chnaif]

@aneequeahmad pageX and pageY are custom keys added by @ahmdshrif, not provided by react-native-gesture-handler of course. please check web code for reference.

App/src/components/PressableWithSecondaryInteraction/index.js

Lines 38 to 49 in e732d16

	// Map gesture event to normal Responder event
	const {
	absoluteX, absoluteY, locationX, locationY,
	} = e.nativeEvent;
	const mapEvent = {
	...e,
	nativeEvent: {
	...e.nativeEvent, pageX: absoluteX, pageY: absoluteY, x: locationX, y: locationY,
	},
	};
	this.props.onSecondaryInteraction(mapEvent);
	}

And it doesn't make sense to do Math calculation with undefined value.

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

Lines 156 to 158 in e732d16

	cursorRelativePosition: {
	horizontal: nativeEvent.pageX - x,
	vertical: nativeEvent.pageY - y,

App/src/components/PopoverWithMeasuredContent.js

Lines 131 to 155 in e732d16

	calculateAdjustedAnchorPosition() {
	let horizontalConstraint;
	switch (this.props.anchorOrigin.horizontal) {
	case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.RIGHT:
	horizontalConstraint = {left: this.props.anchorPosition.horizontal - this.popoverWidth};
	break;
	case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.CENTER:
	horizontalConstraint = {
	left: Math.floor(this.props.anchorPosition.horizontal - (this.popoverWidth / 2)),
	};
	break;
	case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT:
	default:
	horizontalConstraint = {left: this.props.anchorPosition.horizontal};
	}
	let verticalConstraint;
	switch (this.props.anchorOrigin.vertical) {
	case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM:
	verticalConstraint = {top: this.props.anchorPosition.vertical - this.popoverHeight};
	break;
	case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.CENTER:
	verticalConstraint = {
	top: Math.floor(this.props.anchorPosition.vertical - (this.popoverHeight / 2)),
	};

[aneequeahmad]

Even in react-native doc pageX and pageY value only exist in nativeEvent of Synthetic Touch Events for View responder props. Also, I can't find any authentic source that says that absoluteX and absoluteY value can be given to pageX and pageY.

The react-native doc says: Method that provides pageX and pageY value is measure(callback) which determines the location on screen, width, and height of the given view and returns the values via an async callback. If successful, the callback will be called with the following arguments:

• x
• y
• width
• height
• pageX
• pageY

[Ollyws]

All yours @aimane-chnaif, let's see if we can get it merged within three days!

[JmillsExpensify]

For new feature issues, Bug label should be added as well if we're actively looking for proposals?
i.e. #12698 has lack of proposals maybe because of Bug label not applied.

Missed this last week, though we aren't added the bug label for NewFeature issues. Main reason is that we still favor bug fixing over features at the moment, though I'm sure there are process improvements we can still make.

[melvin-bot]

@deetergp, @Ollyws, @jliexpensify, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

[aimane-chnaif]

PR was merged and deployed to prod on Jan 9.
Issue title needs updated accordingly.

[jliexpensify]

Job - https://www.upwork.com/ab/applicants/1610433379500457984/job-details

Changed title to reflect payment date (huh, I thought it would update automatically?), hired @Ollyws and @aimane-chnaif and invited @aneequeahmad . Cheers!

[aneequeahmad]

@jliexpensify applied thanks

[jliexpensify]

Everyone's been paid, thanks!

[aimane-chnaif]

@jliexpensify this is eligible for timeline bonus

[jliexpensify]

Hi @aimane-chnaif - apologies: I'll get a new Upworks link for both you and @Ollyws for the $500.

[aimane-chnaif]

Hi @aimane-chnaif - apologies: I'll get a new Upworks link for both you and @Ollyws for the $500.

Thanks @jliexpensify
No need to create new job. You can just pay bonus in closed contract

[jliexpensify]

I ran into some errors on the Upworks site doing that (not sure what's happening), so I've just rehired you on the new job - that seems to have worked.

[jliexpensify]

All sorted, thanks Oliver and Aimane!