Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parent Message of thread chat on LHN doesn't updated when the parent message is updated #23777

Merged
merged 10 commits into from
Jul 31, 2023
Merged

Parent Message of thread chat on LHN doesn't updated when the parent message is updated #23777

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
71fd420
fix: message of thread chat on LHN doesn't updated
dukenv0307 Jul 28, 2023
d53c115
Merge branch 'main' into fix/23424
dukenv0307 Jul 28, 2023
258743a
fix: test fails
dukenv0307 Jul 28, 2023
95b694f
fix: parentReportActions type
dukenv0307 Jul 28, 2023
59635b0
fix: memo value
dukenv0307 Jul 28, 2023
eeb8927
add comment
dukenv0307 Jul 29, 2023
a369e76
remove useMemo
dukenv0307 Jul 29, 2023
9b70ac7
fix: lint issue
dukenv0307 Jul 31, 2023
cd5ea63
Update src/components/LHNOptionsList/OptionRowLHNData.js
dukenv0307 Jul 31, 2023
a0df9fe
Update src/components/LHNOptionsList/OptionRowLHNData.js
dukenv0307 Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/components/LHNOptionsList/OptionRowLHNData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import * as Report from '../../libs/actions/Report';
import * as UserUtils from '../../libs/UserUtils';
import participantPropTypes from '../participantPropTypes';
import CONST from '../../CONST';
import reportActionPropTypes from '../../pages/home/report/reportActionPropTypes';

const propTypes = {
/** If true will disable ever setting the OptionRowLHN to focused */
Expand Down Expand Up @@ -41,6 +42,9 @@ const propTypes = {
}),
),

/** The list of parent report action of the report */
dukenv0307 marked this conversation as resolved.
Show resolved Hide resolved
parentReportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),

...withCurrentReportIDPropTypes,
...basePropTypes,
};
Expand All @@ -50,6 +54,7 @@ const defaultProps = {
personalDetails: {},
fullReport: {},
policies: {},
parentReportActions: {},
preferredLocale: CONST.LOCALES.DEFAULT,
...withCurrentReportIDDefaultProps,
...baseDefaultProps,
Expand All @@ -61,14 +66,16 @@ const defaultProps = {
* The OptionRowLHN component is memoized, so it will only
* re-render if the data really changed.
*/
function OptionRowLHNData({shouldDisableFocusOptions, currentReportID, fullReport, personalDetails, preferredLocale, comment, policies, ...propsToForward}) {
function OptionRowLHNData({shouldDisableFocusOptions, currentReportID, fullReport, personalDetails, preferredLocale, comment, policies, parentReportActions, ...propsToForward}) {
const reportID = propsToForward.reportID;
// We only want to pass a boolean to the memoized component,
// instead of a changing number (so we prevent unnecessary re-renders).
const isFocused = !shouldDisableFocusOptions && currentReportID === reportID;

const policy = lodashGet(policies, [`${ONYXKEYS.COLLECTION.POLICY}${fullReport.policyID}`], '');

const parentReportAction = useMemo(() => parentReportActions[fullReport.parentReportActionID], [parentReportActions, fullReport.parentReportActionID]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of unnecessary. Please revert this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I late realized it. Sorry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dukenv0307 Bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat We use parentntReportAction in dependency (line 90)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not put parentReportActions (all report actions) into dependency, we only should listen a parent report of that thread

Copy link
Member

@parasharrajat parasharrajat Jul 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not saying to put the whole parentReportActions into dependency but to remove useMemo and directly get the parent action from the object as

const parentReportAction = parentReportActions[id]

There is nothing being calculated from useMemo its use is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat Sorry about that 😄 , I Just updated


const optionItemRef = useRef();
const optionItem = useMemo(() => {
// Note: ideally we'd have this as a dependent selector in onyx!
Expand All @@ -78,7 +85,8 @@ function OptionRowLHNData({shouldDisableFocusOptions, currentReportID, fullRepor
}
optionItemRef.current = item;
return item;
}, [fullReport, personalDetails, preferredLocale, policy]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [fullReport, personalDetails, preferredLocale, policy, parentReportAction]);
dukenv0307 marked this conversation as resolved.
Show resolved Hide resolved
dukenv0307 marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) {
Expand Down Expand Up @@ -156,5 +164,11 @@ export default React.memo(
key: ONYXKEYS.COLLECTION.POLICY,
},
}),
withOnyx({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have withOnyx HOC already we why we are adding another one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops, I didn't notice. May be it came during merging main. Will fix this in a followup in not done already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s77rt The second withOnyx is needed since the fullReport is only available after we successfully retrieve the data from Onyx with the previous withOnyx HOC
Please let me know if any concerns
cc @parasharrajat

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh, so dependent keys... @hannojg was talking about this earlier. Not sure what is the status..

parentReportActions: {
key: ({fullReport}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${fullReport.parentReportID}`,
canEvict: false,
},
Comment on lines +169 to +172
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make the option row re-render whenever any report action change (and not just the parent report action). This can be fixed in two ways:

  1. Using Onyx selector but this actually does not take props (can't access fullReport.parentReportID), so we can't use this solution unfortunately https://expensify.slack.com/archives/C01GTK53T8Q/p1691169417094889
  2. Use React.memo custom comparator, by default the memo will compare prevProps.parentReportActions with nextProps.parentReportActions (which will be different if any report action chane), we should instead compare prevProps.parentReportActions[fullReport.parentReportActionID] with nextProps.parentReportActions[fullReport.parentReportActionID]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that because we are already listening to a particular collection and when any item in that collection changes it will update.

Here

  1. When any action on the thread changes, it will rerender.
  2. But only one option row will be affected not all.

I couldn't think of any other optimization during PR review so moved as it is. The memo idea is good. @dukenv0307 Do you want to create a followup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat The PR to implement the memo custom comparator is ready for review: #24351
cc @s77rt

}),
)(OptionRowLHNData),
);
1 change: 1 addition & 0 deletions tests/unit/SidebarFilterTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ describe('Sidebar', () => {
Onyx.init({
keys: ONYXKEYS,
registerStorageEventListener: () => {},
safeEvictionKeys: [ONYXKEYS.COLLECTION.REPORT_ACTIONS],
}),
);

Expand Down
1 change: 1 addition & 0 deletions tests/unit/SidebarOrderTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ describe('Sidebar', () => {
Onyx.init({
keys: ONYXKEYS,
registerStorageEventListener: () => {},
safeEvictionKeys: [ONYXKEYS.COLLECTION.REPORT_ACTIONS],
dukenv0307 marked this conversation as resolved.
Show resolved Hide resolved
}),
);

Expand Down
1 change: 1 addition & 0 deletions tests/unit/SidebarTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ describe('Sidebar', () => {
Onyx.init({
keys: ONYXKEYS,
registerStorageEventListener: () => {},
safeEvictionKeys: [ONYXKEYS.COLLECTION.REPORT_ACTIONS],
}),
);

Expand Down