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

Jump to bottom

Search result duplication #43164

Merged
merged 7 commits into from
Jun 18, 2024
Merged

Search result duplication #43164

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6e49019
Unify useEffect to create the optionList in one place, clearing all t…
gedu Jun 6, 2024
09297b6
removed console logs
gedu Jun 12, 2024
89cd42c
Updated comments
gedu Jun 13, 2024
2dabaac
removed extra comment
gedu Jun 13, 2024
b54df89
Merge branch 'main' into gedu/43094_duplicate_search_result
gedu Jun 17, 2024
19d862a
Merge branch 'main' into gedu/43094_duplicate_search_result
gedu Jun 17, 2024
bf61954
Merge branch 'main' into gedu/43094_duplicate_search_result
gedu Jun 18, 2024
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
33 changes: 3 additions & 30 deletions src/components/OptionListContextProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,26 +53,6 @@ function OptionsListContextProvider({reports, children}: OptionsListProviderProp

const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
const prevPersonalDetails = usePrevious(personalDetails);
const prevReports = usePrevious(reports);

/**
* This effect is used to update the options list when a report is updated.
*/
useEffect(() => {
// there is no need to update the options if the options are not initialized
if (!areOptionsInitialized.current) {
return;
}

const newReports = OptionsListUtils.createOptionList(personalDetails, reports).reports;

setOptions((prevOptions) => {
const newOptions = {...prevOptions};
newOptions.reports = newReports;
return newOptions;
});
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [reports]);

/**
* This effect is used to add a new report option or remove a report option from the list of options when a new report is added to/removed from the collection.
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -81,21 +61,14 @@ function OptionsListContextProvider({reports, children}: OptionsListProviderProp
if (!areOptionsInitialized.current || !reports) {
return;
}
const missingReportIds = Object.keys(reports).filter((key) => prevReports && !(key in prevReports));
const newReports = OptionsListUtils.createOptionList(personalDetails, reports).reports;

setOptions((prevOptions) => {
const newOptions = {
...prevOptions,
reports: prevOptions.reports.filter((report) => reports[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`] !== null),
reports: newReports,
};
missingReportIds.forEach((missingReportId) => {
const report = missingReportId ? reports[missingReportId] : null;
if (!missingReportId || !report) {
return;
}
const reportOption = OptionsListUtils.createOptionFromReport(report, personalDetails);
newOptions.reports.push(reportOption);
});

return newOptions;
});
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
4 changes: 1 addition & 3 deletions src/libs/actions/Report.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,9 +787,7 @@ function openReport(
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
errorFields: {
notFound: null,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you commit this change by false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry I should add a comment on the PR, this change is because of this discussion: https://callstack-hq.slack.com/archives/C05LX9D6E07/p1717509334788509?thread_ts=1717508731.782149&cid=C05LX9D6E07

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't have access to this channel. Is there is any test steps for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it was a bug that happen to David, and with @mountiny didn't know well how it happens. But it seems that not all the errors were cleaned up successfully.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gedu this change seems wrong and is incorrectly clearing all report errors. I'm intending on reverting it. I'm not sure which issue you were trying to fix with this change (I don't have access to that slack conversation either) but this is probably not the right way to fix it.

errorFields: null,
},
},
{
Expand Down
Loading