Search code, repositories, users, issues, pull requests...

Line 58 in 78c20f0

Navigation.dismissModal(report?.reportID);

then In PR, we call:

Line 370 in 78c20f0

Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));

In above, function Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID)) can be called before Navigation.dismissModal(report?.reportID) hence the app go back to main chat.

What changes do you think we should make in order to solve the problem?

We can dismiss modal before call editTask in:

Line 55 in 78c20f0

Task.editTask(report, {description: values.description});

The same should be applied in edit title flow.

What alternative solutions did you explore? (Optional)

In case Task.editTask will be called, we can move logic:

Navigation.dismissModal(report?.reportID);

to below:

Line 567 in 6334d36

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

daledah · 2024-08-19T22:35:38Z

Proposal updated

bernhardoj · 2024-08-20T06:00:03Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

The main chat opens after editing task description.

What is the root cause of that problem?

When we edit the task, we notify the report that a new action has been added.

Lines 568 to 569 in 80236f5

    
               Report.notifyNewAction(report.reportID, currentUserAccountID); 
        
           }

It will trigger scrollToBottomForCurrentUserAction and if hasNewestReportActionRef is false, then we navigate to the self-report to clear the reportActionID params.

Lines 362 to 372 in 80236f5

    
           const scrollToBottomForCurrentUserAction = useCallback( 
        
               (isFromCurrentUser: boolean) => { 
        
                   // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where 
        
                   // they are now in the list. 
        
                   if (!isFromCurrentUser) { 
        
                       return; 
        
                   } 
        
                   if (!hasNewestReportActionRef.current) { 
        
                       Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID)); 
        
                       return; 
        
                   }

After editing the task, we call dismissModal to close the RHP.

App/src/libs/Navigation/dismissModalWithReport.ts

Lines 55 to 58 in 80236f5

    
               Task.editTask(report, {description: values.description}); 
        
           } 
        
           Navigation.dismissModal(report?.reportID);

In dismissModal, if the target report ID is the same as the topmost report ID, which is true in our case, we simply pop the current screen.

Lines 70 to 72 in 80236f5

    
           } else { 
        
               navigationRef.dispatch({...StackActions.pop(), target: state.key}); 
        
           }

So, the first navigate to the self-report already closes the RHP and the dismissModal calls pop which brings us back to the parent report.

Even though this happens after my PR where I add the navigate to self-report logic, but we already guard it with hasNewestReportActionRef condition. So, the real issue here is, why is hasNewestReportActionRef false?

The issue is both FE and BE. hasNewestReportAction compares the last report action created with the report lastVisibleActionCreated.

Line 295 in 80236f5

    
           const hasNewestReportAction = sortedVisibleReportActions[0]?.created === report.lastVisibleActionCreated;

In FE, we optimistically add the update task description system message, but we don't update the lastVisibleActionCreated.

Lines 505 to 524 in 80236f5

    
           const optimisticData: OnyxUpdate[] = [ 
        
               { 
        
                   onyxMethod: Onyx.METHOD.MERGE, 
        
                   key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, 
        
                   value: {[editTaskReportAction.reportActionID]: editTaskReportAction as OnyxTypes.ReportAction}, 
        
               }, 
        
               { 
        
                   onyxMethod: Onyx.METHOD.MERGE, 
        
                   key: `${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, 
        
                   value: { 
        
                       reportName, 
        
                       description: reportDescription, 
        
                       pendingFields: { 
        
                           ...(title && {reportName: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}), 
        
                           ...(description && {description: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}), 
        
                       }, 
        
                       errorFields: null, 
        
                   }, 
        
               }, 
        
           ];

For the BE issue, looks like the BE doesn't consider the update task system message created as the report lastVisibleActionCreated.

From the screenshot above, we can see that after the EditTask is completed, there are 2 reports onyx data. The first one contains the correct lastVisibleActionCreated but the second one contains the wrong lastVisbibleActionCreated (it contains the report CREATED action created value).

This makes hasNewestReportActionRef always false even if we fix it optimistically because when scrollToBottomForCurrentUserAction is called, the optimistic data is not merged yet, so hasNewestReportActionRef is still false because the report lastVisibleActionCreated from the BE is wrong.

What changes do you think we should make in order to solve the problem?

The fix must be done for both BE and FE. The BE needs to return the correct lastVisibleActionCreated. In FE, we can optimistically update the report lastVisibleActionCreated in editTask to be the same as the edit task system message created. buildOptimisticEditedTaskFieldReportAction will accept a created param to be used as the created value.

const created = DateUtils.getDBTime();
const editTaskReportAction = ReportUtils.buildOptimisticEditedTaskFieldReportAction({title, description}, created);
const optimisticData: OnyxUpdate[] = [
    {
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`,
        value: {
            ...,
            lastVisibleActionCreated: created,
        },
    },
];

tsa321 · 2024-08-20T06:06:20Z

Edited by proposal-police: This proposal was edited at 2023-10-01T00:00:00Z.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Tasks - App goes back to main chat or LHN when editing a task's description

What is the root cause of that problem?

When user edit task, dismissModal will be executed:

Lines 50 to 58 in 6334d36

    
           const submit = useCallback( 
        
               (values: FormOnyxValues<typeof ONYXKEYS.FORMS.EDIT_TASK_FORM>) => { 
        
                   if (values.description !== Parser.htmlToMarkdown(report?.description ?? '') && !isEmptyObject(report)) { 
        
                       // Set the description of the report in the store and then call EditTask API 
        
                       // to update the description of the report on the server 
        
                       Task.editTask(report, {description: values.description}); 
        
                   } 
        
                   Navigation.dismissModal(report?.reportID);

and when before calling API write of edit_task:

Lines 567 to 568 in 6334d36

    
           API.write(WRITE_COMMANDS.EDIT_TASK, parameters, {optimisticData, successData, failureData}); 
        
           Report.notifyNewAction(report.reportID, currentUserAccountID);

Report.notifyNewAction will be executed first and will execute it's subcribtion function :

App/src/pages/tasks/TaskTitlePage.tsx

Lines 362 to 372 in 6334d36

    
           const scrollToBottomForCurrentUserAction = useCallback( 
        
               (isFromCurrentUser: boolean) => { 
        
                   // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where 
        
                   // they are now in the list. 
        
                   if (!isFromCurrentUser) { 
        
                       return; 
        
                   } 
        
                   if (!hasNewestReportActionRef.current) { 
        
                       Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID)); 
        
                       return; 
        
                   }

which will navigate to the report or scroll to bottom of the report. The navigate is executed first which will make the report screen go back to previous report.

What changes do you think we should make in order to solve the problem?

If the expected solution is to not scroll to bottom after editing the task, we could remove the Report.notifyNewAction after the API.WRITE EDIT_TASK. This could be used for other similar issues.

What alternative solutions did you explore? (1)

If the expected solution involves scrolling to the bottom, we can move the Report.notifyNewAction from Task.ts to TaskDescriptionPage, placing it below Navigation.dismissModal (ensure to check all usages of the editTaskLibrary function).

If scrolling is only necessary when the page report action list does not have the hasNewestReportAction, we can modify the scrollToBottomForCurrentUserAction function so that it does not scroll when there is a hasNewestReportAction. Alternatively, we could pass a parameter to scrollToBottomForCurrentUserAction to determine whether it should scroll when there is no newestReportAction.

What alternative solutions did you explore? (Optional)

To fix the hasNewestReportAction bug (I think it is different bug), we could use add this code in ReportActionList

    const newestVisibleReportAction = sortedVisibleReportActions[0];
    const newestPendingActionTime = useRef(report.lastVisibleActionCreated);
    const newCreatedUserActions = useMemo(() => {
        const previousLastReportAction = ReportActionsUtils.getReportAction(report.reportID, previousLastIndex.current);
        const previousCreatedDate = new Date(previousLastReportAction.created);
        const previousIndex = sortedVisibleReportActions.findIndex((action) => new Date(action.created) < previousCreatedDate);
        const addedActions = sortedVisibleReportActions.slice(0, previousIndex.current === -1 ? -1 : previousIndex - 1);

        const newCreatedActions = addedActions.filter((action) => (action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) && (new Date(action.created) > new Date(newestPendingActionTime.current)));
        if (newCreatedActions.length > 0) {
            newestPendingActionTime.current = newCreatedActions.reduce((a, b) => Math.max(new Date(a.created), new Date(b.created))).created;
        }
        return newCreatedActions;
   },   [lastActionIndex]);

    const hasNewestReportAction = (newCreatedUserActions.length > 0) || (new Date(newestVisibleReportAction?.created) >= new Date(report.lastVisibleActionCreated));

deetergp · 2024-08-20T14:59:43Z

I think this blocker may be a result of this PR from yesterday's staging deploy #47209

bernhardoj · 2024-08-20T16:13:39Z

It's actually happening after my PR #46724, but the root cause is actually an existing issue coming from both BE and FE optimistic data issues which is why I posted a proposal for this one.

melvin-bot · 2024-08-20T22:16:21Z

Job added to Upwork: https://www.upwork.com/jobs/~0135f56d92cfd4346d

melvin-bot · 2024-08-20T22:16:26Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha (External)

nkdengineer · 2024-08-21T10:53:11Z

Edited by proposal-police: This proposal was edited at 2024-09-16 06:45:28 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

The user is returned back to the LHN on mobile or the chat that the task was in. The same happens on the name field after the description was edited once

What is the root cause of that problem?

When we edit the task title, we will dismiss the modal

Line 55 in 685bb4c

Navigation.dismissModal(report?.reportID);

But before that, we have a logic to that will trigger scrollToBottomForCurrentUserAction here

App/src/pages/tasks/TaskAssigneeSelectorModal.tsx

Line 568 in 685bb4c

Report.notifyNewAction(report.reportID, currentUserAccountID);

In the normal case, we will scroll to the bottom but in this case hasNewestReportActionRef is false because we don't update lastVisibleActionCreated of the task report in optimistic data, and BE also returns the wrong created of the task report then we will navigate to the task report before we dismiss the modal. These actions are called almost simultaneously when the task report is removed from the stack navigator.

It doesn't happen when we update the assignee because we wrap the dismiss logic in InteractionManager.runAfterInteractions. But when we edit the assignee, lastVisibleActionCreated is also not updated in optimistic data

Lines 200 to 204 in 685bb4c

    
               InteractionManager.runAfterInteractions(() => { 
        
                   Navigation.dismissModal(report.reportID); 
        
               }); 
        
               // If there's no report, we're creating a new task 
        
           } else if (option.accountID) {

Screen.Recording.2024-08-12.at.12.34.32.mp4

What changes do you think we should make in order to solve the problem?

On the FE side, we should update lastVisibleActionCreated of task report when we edit the task title, and description here. We can simply update this to editTaskReportAction.created instead of adding new created param

lastVisibleActionCreated: editTaskReportAction.created

Lines 513 to 515 in 685bb4c

    
           key: `${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, 
        
           value: { 
        
               reportName,

and the same when editing the task assignee here

lastVisibleActionCreated: editTaskReportAction.created,

App/src/pages/tasks/TaskTitlePage.tsx

Line 578 in 685bb4c

const optimisticReport: OptimisticReport = {

OPTIONAL: We can also wrap dismiss modal in InteractionManager.runAfterInteractions as we do for task assignee page

Line 55 in 685bb4c

Navigation.dismissModal(report?.reportID);

For BE, we should store and return the correct lastVisibleActionCreated when we call OpenReport API as we did in EditTask API

What alternative solutions did you explore? (Optional)

NA

deetergp · 2024-08-23T16:13:14Z

@getusha We've got a good number of proposals here for you to review 🎉

melvin-bot · 2024-10-08T16:00:36Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

tsa321 · 2024-10-09T12:21:39Z

@deetergp still reproducible in staging:

bug-d.mp4

cc @getusha for my comment here

deetergp · 2024-10-11T00:53:45Z

Well dang. Moving back to Daily.

@getusha, care to weigh in on @tsa321's comment?

melvin-bot · 2024-10-14T18:11:18Z

@deetergp, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot · 2024-10-15T16:00:41Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

tsa321 · 2024-10-16T01:36:16Z

Waiting for @getusha 's reply...
Maybe @getusha is too busy?

getusha · 2024-10-16T08:01:01Z

Sorry this fell out of my radar.

getusha · 2024-10-16T08:44:28Z

@tsa321 the issue occurs only when we have over 10ish edits. until then it doesn't seem to happen when editing the description. any explanation on that?

melvin-bot · 2024-10-21T18:09:59Z

@deetergp, @getusha Eep! 4 days overdue now. Issues have feelings too...

tsa321 · 2024-10-22T01:22:24Z

@getusha Sorry for the late reply. When I open another chat, such as the Concierge chat, and then edit a task (even with minimal edits), the app returns to the Concierge chat after editing. So, it's not a matter of reaching the 10th edit— I can reproduce the issue with just a minimal number of edits:

bug-d.mp4

Either way the root cause is in this line: