Skip to content

Fix: RHN lag when close #45691

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

Merged
MonilBhavsar merged 14 commits into from
Aug 19, 2024
Merged

Fix: RHN lag when close #45691

Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
511d05e
Fix: RHN lag when close
truph01 Jul 18, 2024
03e3a42
Fix: Lint
truph01 Jul 18, 2024
96fc079
Fix: Lint
truph01 Jul 18, 2024
a877075
Fix: Remove unused change
truph01 Jul 18, 2024
38c12a9
merge main
truph01 Jul 31, 2024
7021034
merge main
truph01 Aug 13, 2024
d40054c
use InteractionManager.runAfterInteractions
truph01 Aug 13, 2024
db0279b
remove redundant changes
truph01 Aug 13, 2024
9e04862
import issue
truph01 Aug 13, 2024
d2973c2
fix import issue
truph01 Aug 13, 2024
217bc06
fix type error
truph01 Aug 13, 2024
6a3c5f7
merge main
truph01 Aug 16, 2024
6021ca4
navigate after onyx merge
truph01 Aug 16, 2024
81276ac
fix lint
truph01 Aug 16, 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
13 changes: 8 additions & 5 deletions src/pages/tasks/TaskAssigneeSelectorModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import type {RouteProp} from '@react-navigation/native';
import {useRoute} from '@react-navigation/native';
import React, {useCallback, useEffect, useMemo, useState} from 'react';
import {View} from 'react-native';
import {InteractionManager, View} from 'react-native';
import {useOnyx, withOnyx} from 'react-native-onyx';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView';
Expand Down Expand Up @@ -194,21 +194,24 @@ function TaskAssigneeSelectorModal({reports, task}: TaskAssigneeSelectorModalPro
undefined, // passing null as report because for editing task the report will be task details report page not the actual report where task was created
OptionsListUtils.isCurrentUser({...option, accountID: option?.accountID ?? -1, login: option?.login ?? ''}),
);

// Pass through the selected assignee
TaskActions.editTaskAssignee(report, session?.accountID ?? -1, option?.login ?? '', option?.accountID, assigneeChatReport);
}
Navigation.dismissModal(report.reportID);
InteractionManager.runAfterInteractions(() => {
Navigation.dismissModal(report.reportID);
});
// If there's no report, we're creating a new task
} else if (option.accountID) {
TaskActions.setAssigneeValue(
option?.login ?? '',
option.accountID,
option.accountID ?? -1,

@allgandalf allgandalf Aug 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is account id a string or number ? if it is a number then we fallback to 0 for numbers, can you help me confirm @truph01 ?

@truph01 truph01 Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@allgandalf the accountID is number. If it is number, I think we should fallback to -1 as we did in other logics.

@allgandalf allgandalf Aug 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it, the other day i was reviewing a PR and they fallback to 0:
#46931 (comment)

@truph01 truph01 Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just refer to our contributor style docs:
https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#default-value-for-inexistent-ids

image

@allgandalf allgandalf Aug 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah that is because in the example reportID is a string:

App/src/types/onyx/Report.ts

Lines 140 to 141 in 9940e99

/** ID of the report */
reportID: string;

and as you said:

@allgandalf the accountID is number.

@allgandalf allgandalf Aug 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Confirming on slack about the same

@truph01 truph01 Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that the fallback -1 should be applied regardless the id is number or string since I just checked this PR, we are using -1 as number, for example https://github.com/Expensify/App/pull/42634/files#diff-ea38863732cfd28f820d12c5b3f2633a8799cb797929c39af675b6a11452e9ee

@allgandalf allgandalf Aug 16, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Confirmed on slack that 0 is a valid account id

@truph01 truph01 Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If -1 is a valid account id, then we should fall back to 0, right?

@allgandalf allgandalf Aug 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry, it is the other way around

task?.shareDestination ?? '',
undefined, // passing null as report is null in this condition
OptionsListUtils.isCurrentUser({...option, accountID: option?.accountID ?? -1, login: option?.login ?? undefined}),
);
Navigation.goBack(ROUTES.NEW_TASK);
InteractionManager.runAfterInteractions(() => {
Navigation.goBack(ROUTES.NEW_TASK);
});
}
},
[session?.accountID, task?.shareDestination, report],
Expand Down