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

fix: make consistent behavior of displaying green line for assign task #51174

Merged
merged 10 commits into from
Nov 13, 2024
Merged

fix: make consistent behavior of displaying green line for assign task #51174

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3afe1b8
fix: make consistent behavior of displaying green line for assign task
daledah Oct 21, 2024
326332e
fix: typecheck
daledah Oct 21, 2024
6054ad5
fix: update function
daledah Oct 22, 2024
42d4444
Merge branch 'main' into fix/50792
daledah Oct 23, 2024
ce5f2f6
Merge branch 'main' into fix/50792
daledah Oct 30, 2024
7447a12
fix: marker appears briefly on ios
daledah Oct 30, 2024
a8cc00e
fix: apply suggested changes
daledah Oct 31, 2024
f3968e6
fix: change effect deps
daledah Oct 31, 2024
320df1c
fix: keep comment
daledah Oct 31, 2024
597886a
fix: use getReport
daledah Nov 1, 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
22 changes: 21 additions & 1 deletion src/libs/ReportConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,24 @@ function getAllReportsLength() {
return Object.keys(allReports ?? {}).length;
}

export {getAllReports, getAllReportsNameMap, getAllReportsLength};
function updateReportData(reportID: string, reportData?: Partial<Report>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this function. Any data in Onyx that needs to be updated should be done via one of the proper Onyx methods (merge, set, etc.).

This method will introduce bad side-effects which could have unknown consequences and lead to many bugs without the ability to track them down.

Copy link
Contributor

Choose a reason for hiding this comment

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

Context https://expensify.slack.com/archives/C01GTK53T8Q/p1732889804245739

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI I'm working on the PR where ReportConnection file is removed. There updateReportData function usage is replaced with Onyx.merge

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, @stitesExpensify was there any specific reason why you created this function instead of using Onyx.merge?

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, this was written by @daledah I was just the reviewer, so I'll leave that answer to him

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said in the proposal. createTaskAndNavigate will update lastReadTime as currentTime. After that we dismiss modal and navigate to report:

App/src/libs/actions/Task.ts

Line 304 in ce23fb5

Navigation.dismissModal(parentReportID);
, but the lastReadTime here:

App/src/libs/actions/Report.ts

Line 955 in e9de4f2

parameters.clientLastReadTime = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.lastReadTime ?? '';
and here:

App/src/pages/home/report/ReportActionsList.tsx

Line 209 in e9de4f2

const [unreadMarkerTime, setUnreadMarkerTime] = useState(report.lastReadTime ?? '');
have not been updated yet so other logics can use the outdated value.
That’s the reason we decided to use getReport to get the up-to-date data.

I think we can’t use useOnyx in this case since its value can be outdated in this case. Please forgive me If I did something wrong, I’ll raise other PR to fix If we have the better idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

@daledah Can you remove your change and test the original issue? I think it's fixed somewhere else.

const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
daledah marked this conversation as resolved.
Show resolved Hide resolved

if (!allReports || !report || !report.reportID) {
return;
}

allReports[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] = {
...report,
...reportData,
};
}

function getReport(reportID: string) {
if (!reportID || !allReports) {
return;
}
return allReports[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
}

export {getAllReports, getAllReportsNameMap, getAllReportsLength, updateReportData, getReport};
4 changes: 4 additions & 0 deletions src/libs/actions/Task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,10 @@ function createTaskAndNavigate(

API.write(WRITE_COMMANDS.CREATE_TASK, parameters, {optimisticData, successData, failureData});

ReportConnection.updateReportData(parentReportID, {
lastReadTime: currentTime,
});

if (!isCreatedUsingMarkdown) {
clearOutTaskInfo();
Navigation.dismissModal(parentReportID);
Expand Down
18 changes: 7 additions & 11 deletions src/pages/home/report/ReportActionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import useWindowDimensions from '@hooks/useWindowDimensions';
import DateUtils from '@libs/DateUtils';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportConnection from '@libs/ReportConnection';
import * as ReportUtils from '@libs/ReportUtils';
import Visibility from '@libs/Visibility';
import type {AuthScreensParamList} from '@navigation/types';
Expand Down Expand Up @@ -210,20 +211,15 @@ function ReportActionsList({
);
const prevSortedVisibleReportActionsObjects = usePrevious(sortedVisibleReportActionsObjects);

/**
daledah marked this conversation as resolved.
Show resolved Hide resolved
* The timestamp for the unread marker.
*
* This should ONLY be updated when the user
* - switches reports
* - marks a message as read/unread
* - reads a new message as it is received
*/
const [unreadMarkerTime, setUnreadMarkerTime] = useState(report.lastReadTime ?? '');
const reportLastReadTime = useMemo(() => {
return ReportConnection.getReport(report.reportID)?.lastReadTime ?? report.lastReadTime ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

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

This should never use getReport() from a lib. It should be useOnyx instead to connect to the report data.

}, [report.reportID, report.lastReadTime]);
const [unreadMarkerTime, setUnreadMarkerTime] = useState(reportLastReadTime);
useEffect(() => {
setUnreadMarkerTime(report.lastReadTime ?? '');
setUnreadMarkerTime(reportLastReadTime);

// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [report.reportID]);
}, [reportLastReadTime]);
daledah marked this conversation as resolved.
Show resolved Hide resolved

const prevUnreadMarkerReportActionID = useRef<string | null>(null);
/**
Expand Down
Loading