App
App copied to clipboard
Expensify
BUG: Web - Console error when comment in a report is right clicked and context menu is opened - reported by @aneequeahmad
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
- Launch web app.
- Click on any report having links in comments in LHN
- Hover on any comment and right click to open context menu
Expected Result:
no console errors should display
Actual Result:
Console error: Failed prop type: Invalid prop anchor supplied to BaseReportActionContextMenu, expected a ReactNode at BaseReportActionContextMenu
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platform:
Where is this issue occurring?
- Web
Version Number: 1.2.12-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
https://user-images.githubusercontent.com/43995119/196144255-575ad966-bfcf-49d6-b6c1-9931c29b18d5.mov
Expensify/Expensify Issue URL: Issue reported by: @aneequeahmad Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664421573647289
Triggered auto assignment to @muttmuure (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
@mvtglobally 10 days overdue. I'm getting more depressed than Marvin.
@mvtglobally 10 days overdue. I'm getting more depressed than Marvin.
Triggered auto assignment to @tjferriss (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
I was not able to reproduce the issue following the steps outlined. @mvtglobally are you still seeing this issue? If so, can you clarify the steps to reproduce.
https://user-images.githubusercontent.com/8292562/200081258-68a898ec-6ce2-40b5-a8d8-92302f051b8e.mov
@tjferriss This issue is still reproducible on latest dev built. Please right click on a comment that is a link.
@tjferriss, @mvtglobally Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@tjferriss, @mvtglobally Huh... This is 4 days overdue. Who can take care of this?
PROPOSAL
Problem:
The type of anchor is defined as node but it's a reference to the DOM node.
https://github.com/Expensify/App/blob/92eff7267aa7be67463873cdb431f5cd79c22b59/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js#L20-L22
Solution
The type of this DOM node should be object as it's a reference to DOM node not any children/component passed in props. The code will change to the following
/** Target node which is the target of ContentMenu and its object as it is reference to DOM node */
anchor: PropTypes.object
@tjferriss, @mvtglobally 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!
Current assignee @tjferriss is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)
Triggered auto assignment to @dangrous (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Job posting is live on Upworks: https://www.upwork.com/ab/applicants/1592623364413255680/job-details
@dangrous @aimane-chnaif can you both please review @aneequeahmad's proposal?
PROPOSAL
Solution
Either we need to make sure that anchor is getting the same type of value in each case (i.e native & web) or we can do something like this to handle both cases:
anchor: PropTypes.oneOfType([PropTypes.number, PropTypes.object]),
@aimane-chnaif I think my proposal is working as expected on native platforms.
The error you said above is happening on native platform since it is given the type number and it is expecting an object.
Am i missing something ? Please review again if what i said above makes sense. Thanks
@aneequeahmad
it is given the type number
this is what anchor is set to
it is expecting an object
this is what you defined in PropTypes
Please test yourself with both solutions on native: PropTypes.number, PropTypes.object and see which case console warning shows
@aimane-chnaif I'm not sure what you're talking about. Please see the Actual result of the reported bug.
this is what anchor is set to
This is never set to number please see the code of file BaseReportActionContextMenu in latest main branch on Github.
https://github.com/Expensify/App/blob/92eff7267aa7be67463873cdb431f5cd79c22b59/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js#L20-L22
Having said that setting it to object fixes the issue on Web and doesn't show console error on native platforms.
Having said that setting it to object fixes the issue on Web and doesn't show console error on native platforms.
Did you really test this on native?
Before explaining more, please attach screenshot of what console shows after adding console.log(typeof this.props.anchor) in render() on web and iOS respectively
This feels low priority to me as it's not directly tied to anything broken in the app. Not saying we shouldn't fix it. But maybe should be on HOLD for #focus.
The screenshots/Videos are:
WEB:
IOS:
@aimane-chnaif You're right what you said here It was working fine on IOS as the proptype is actually node on IOS (like the official doc says here)
UPDATED PROPOSAL:
Solution:
Need to update the existing code
From:
https://github.com/Expensify/App/blob/34f53dd8020e0224c392e767a910fa7ddee3aee1/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js#L21-L22
To:
/** Target node which is the target of ContentMenu */
anchor: PropTypes.oneOfType([PropTypes.node, PropTypes.object]),
cc: @aimane-chnaif