Skip to content

Commit

Permalink
Third part of adjustments after review
Browse files Browse the repository at this point in the history
  • Loading branch information
zfurtak committed Sep 25, 2024
1 parent c11cee1 commit 0993f36
Show file tree
Hide file tree
Showing 24 changed files with 64 additions and 76 deletions.
1 change: 1 addition & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1262,6 +1262,7 @@ const CONST = {
PUSHER: {
PRIVATE_USER_CHANNEL_PREFIX: 'private-encrypted-user-accountID-',
PRIVATE_REPORT_CHANNEL_PREFIX: 'private-report-reportID-',
PRESENCE_ACTIVE_GUIDES: 'presence-activeGuides',
},

EMOJI_SPACER: 'SPACER',
Expand Down
14 changes: 7 additions & 7 deletions src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,9 @@ function BaseSelectionList<TItem extends ListItem>(
*/
const scrollToIndex = useCallback(
(index: number, animated = true) => {
const item = flattenedSections.allOptions.at(index) ?? 0;
const item = flattenedSections.allOptions.at(index);

if (!listRef.current || !item) {
if (!listRef.current || !item || index === -1) {
return;
}

Expand Down Expand Up @@ -331,7 +331,7 @@ function BaseSelectionList<TItem extends ListItem>(
};

const selectFocusedOption = () => {
const focusedOption = flattenedSections.allOptions.at(focusedIndex);
const focusedOption = focusedIndex !== -1 ? flattenedSections.allOptions.at(focusedIndex) : undefined;

if (!focusedOption || (focusedOption.isDisabled && !focusedOption.isSelected)) {
return;
Expand All @@ -357,7 +357,7 @@ function BaseSelectionList<TItem extends ListItem>(
const getItemLayout = (data: Array<SectionListData<TItem, SectionWithIndexOffset<TItem>>> | null, flatDataArrayIndex: number) => {
const targetItem = flattenedSections.itemLayouts.at(flatDataArrayIndex);

if (!targetItem) {
if (!targetItem || flatDataArrayIndex === -1) {
return {
length: 0,
offset: 0,
Expand Down Expand Up @@ -622,7 +622,7 @@ function BaseSelectionList<TItem extends ListItem>(
/** Selects row when pressing Enter */
useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {
captureOnInputs: true,
shouldBubble: !flattenedSections.allOptions.at(focusedIndex),
shouldBubble: !flattenedSections.allOptions.at(focusedIndex) || focusedIndex === -1,
shouldStopPropagation,
shouldPreventDefault,
isActive: !disableKeyboardShortcuts && !disableEnterShortcut && isFocused,
Expand All @@ -632,7 +632,7 @@ function BaseSelectionList<TItem extends ListItem>(
useKeyboardShortcut(
CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER,
(e) => {
const focusedOption = flattenedSections.allOptions.at(focusedIndex);
const focusedOption = focusedIndex !== -1 ? flattenedSections.allOptions.at(focusedIndex) : undefined;
if (onConfirm) {
onConfirm(e, focusedOption);
return;
Expand All @@ -641,7 +641,7 @@ function BaseSelectionList<TItem extends ListItem>(
},
{
captureOnInputs: true,
shouldBubble: !flattenedSections.allOptions.at(focusedIndex),
shouldBubble: !flattenedSections.allOptions.at(focusedIndex) || focusedIndex === -1,
isActive: !disableKeyboardShortcuts && isFocused,
},
);
Expand Down
11 changes: 6 additions & 5 deletions src/hooks/useViolations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,29 +98,30 @@ function useViolations(violations: TransactionViolation[], shouldShowOnlyViolati
},
}));
}
const firstViolation = currentViolations.at(0);

// missingTag has special logic for policies with dependent tags, because only one violation is returned for all tags
// when no tags are present, so the tag name isn't set in the violation data. That's why we add it here
if (policyHasDependentTags && currentViolations.at(0)?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) {
if (policyHasDependentTags && firstViolation?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) {
return [
{
...currentViolations.at(0),
...firstViolation,
data: {
...currentViolations.at(0)?.data,
...firstViolation.data,
tagName: data?.tagListName,
},
},
];
}

// tagOutOfPolicy has special logic because we have to account for multi-level tags and use tagName to find the right tag to put the violation on
if (currentViolations.at(0)?.name === CONST.VIOLATIONS.TAG_OUT_OF_POLICY && data?.tagListName !== undefined && currentViolations.at(0)?.data?.tagName) {
if (firstViolation?.name === CONST.VIOLATIONS.TAG_OUT_OF_POLICY && data?.tagListName !== undefined && firstViolation?.data?.tagName) {
return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName);
}

// allTagLevelsRequired has special logic because it is returned when one but not all the tags are set,
// so we need to return the violation for the tag fields without a tag set
if (currentViolations.at(0)?.name === CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED && tagValue) {
if (firstViolation?.name === CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED && tagValue) {
return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName);
}

Expand Down
8 changes: 6 additions & 2 deletions src/libs/EmojiUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -504,8 +504,12 @@ const enrichEmojiReactionWithTimestamps = (emoji: ReportActionReaction, emojiNam
const usersWithTimestamps: UsersReactions = {};
Object.entries(emoji.users ?? {}).forEach(([id, user]) => {
const userTimestamps = Object.values(user?.skinTones ?? {});
// eslint-disable-next-line no-nested-ternary
const oldestUserTimestamp = userTimestamps.reduce((min, curr) => (min ? (curr < min ? curr : min) : curr), userTimestamps.at(0));
const oldestUserTimestamp = userTimestamps.reduce((min, curr) => {
if (min) {
return curr < min ? curr : min;
}
return curr;
}, userTimestamps.at(0));

if (!oldestUserTimestamp) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ function ResponsiveStackNavigator(props: ResponsiveStackNavigatorProps) {
const isSearchCentralPane = (route: RouteProp<ParamListBase>) => getTopmostCentralPaneRoute({routes: [route]} as State<RootStackParamList>)?.name === SCREENS.SEARCH.CENTRAL_PANE;

const lastRoute = routes.at(routes.length - 1);
// eslint-disable-next-line no-nested-ternary
const lastSearchCentralPane = lastRoute ? (isSearchCentralPane(lastRoute) ? lastRoute : undefined) : undefined;
const lastSearchCentralPane = lastRoute && isSearchCentralPane(lastRoute) ? lastRoute : undefined;
const filteredRoutes = routes.filter((route) => !isSearchCentralPane(route));

// On narrow layout, if we are on /search route we want to hide all central pane routes and show only the bottom tab navigator.
Expand Down
4 changes: 2 additions & 2 deletions src/libs/PaginationUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,12 @@ function findFirstItem<TResource>(sortedItems: TResource[], page: string[], getI
*/
function findLastItem<TResource>(sortedItems: TResource[], page: string[], getID: (item: TResource) => string): ItemWithIndex | null {
for (let i = page.length - 1; i >= 0; i--) {
const id = page.at(i);
const id = page[i];
if (id === CONST.PAGINATION_END_ID) {
return {id, index: sortedItems.length - 1};
}
const index = sortedItems.findIndex((item) => getID(item) === id);
if (index !== -1 && id) {
if (index !== -1) {
return {id, index};
}
}
Expand Down
1 change: 0 additions & 1 deletion src/libs/PolicyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,6 @@ function getTagLists(policyTagList: OnyxEntry<PolicyTagLists>): Array<ValueOf<Po
*/
function getTagList(policyTagList: OnyxEntry<PolicyTagLists>, tagIndex: number): ValueOf<PolicyTagLists> {
const tagLists = getTagLists(policyTagList);

return (
tagLists.at(tagIndex) ?? {
name: '',
Expand Down
2 changes: 1 addition & 1 deletion src/libs/ReportActionsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1442,7 +1442,7 @@ function isCurrentActionUnread(report: OnyxEntry<Report>, reportAction: ReportAc
return false;
}
const prevReportAction = sortedReportActions.at(currentActionIndex - 1);
return isReportActionUnread(reportAction, lastReadTime) && (!prevReportAction || !isReportActionUnread(prevReportAction, lastReadTime));
return isReportActionUnread(reportAction, lastReadTime) && (currentActionIndex === 0 || !prevReportAction || !isReportActionUnread(prevReportAction, lastReadTime));
}

/**
Expand Down
5 changes: 0 additions & 5 deletions src/libs/actions/Policy/Tag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ function setWorkspaceTagEnabled(policyID: string, tagsToUpdate: Record<string, {
...policyTag.tags[key],
...tagsToUpdate[key],
errors: null,
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
pendingFields: {
...policyTag.tags[key]?.pendingFields,
enabled: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
Expand Down Expand Up @@ -286,7 +285,6 @@ function setWorkspaceTagEnabled(policyID: string, tagsToUpdate: Record<string, {
...policyTag.tags[key],
...tagsToUpdate[key],
errors: null,
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
pendingFields: {
...policyTag.tags[key].pendingFields,
enabled: null,
Expand Down Expand Up @@ -314,7 +312,6 @@ function setWorkspaceTagEnabled(policyID: string, tagsToUpdate: Record<string, {
...policyTag.tags[key],
...tagsToUpdate[key],
errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('workspace.tags.genericFailureMessage'),
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
pendingFields: {
...policyTag.tags[key].pendingFields,
enabled: null,
Expand Down Expand Up @@ -546,7 +543,6 @@ function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName:
tags: {
[newTagName]: {
pendingAction: null,
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
pendingFields: {
...tag.pendingFields,
name: null,
Expand All @@ -568,7 +564,6 @@ function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName:
[oldTagName]: {
...tag,
pendingAction: null,
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
pendingFields: {
...tag.pendingFields,
name: null,
Expand Down
4 changes: 2 additions & 2 deletions src/pages/Search/SearchTypeMenuNarrow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ function SearchTypeMenuNarrow({typeMenuItems, activeItemIndex, queryJSON, title,
};
}

const item = popoverMenuItems.at(activeItemIndex);
const item = activeItemIndex !== -1 ? popoverMenuItems.at(activeItemIndex) : undefined;
return {
icon: item?.icon ?? Expensicons.Receipt,
title: item?.text,
Expand Down Expand Up @@ -155,7 +155,7 @@ function SearchTypeMenuNarrow({typeMenuItems, activeItemIndex, queryJSON, title,
<View style={[styles.pb4, styles.flexRow, styles.alignItemsCenter, styles.justifyContentBetween, styles.ph5, styles.gap2]}>
<PressableWithFeedback
accessible
accessibilityLabel={popoverMenuItems.at(activeItemIndex)?.text ?? ''}
accessibilityLabel={activeItemIndex !== -1 ? popoverMenuItems.at(activeItemIndex)?.text ?? '' : ''}
ref={buttonRef}
wrapperStyle={styles.flex1}
onPress={openMenu}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ function SuggestionMention(
const leftString = newValue.substring(afterLastBreakLineIndex, selectionEnd);
const words = leftString.split(CONST.REGEX.SPACE_OR_EMOJI);
const lastWord: string = words.at(-1) ?? '';
const secondToLastWord = words.at(words.length - 3);
const secondToLastWord = words.length > 2 ? words.at(words.length - 3) : undefined;

let atSignIndex: number | undefined;
let suggestionWord = '';
Expand Down
7 changes: 1 addition & 6 deletions src/pages/home/report/ReportActionsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -231,12 +231,7 @@ function ReportActionsList({
for (let index = 0; index < sortedVisibleReportActions.length; index++) {
const reportAction = sortedVisibleReportActions.at(index);

if (!reportAction) {
// eslint-disable-next-line no-continue
continue;
}

if (shouldDisplayNewMarker(reportAction, index)) {
if (reportAction && shouldDisplayNewMarker(reportAction, index)) {
return reportAction.reportActionID;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/settings/Wallet/PaymentMethodList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ function PaymentMethodList({
if (assignedCardsGrouped.some((item) => item.isGroupedCardDomain && item.description === card.domainName) && !isAdminIssuedVirtualCard) {
const domainGroupIndex = assignedCardsGrouped.findIndex((item) => item.isGroupedCardDomain && item.description === card.domainName);
const assignedCardsGroupedItem = assignedCardsGrouped.at(domainGroupIndex);
if (assignedCardsGroupedItem) {
if (domainGroupIndex >= 0 && assignedCardsGroupedItem) {
assignedCardsGroupedItem.errors = {...assignedCardsGrouped.at(domainGroupIndex)?.errors, ...card.errors};
if (card.fraud === CONST.EXPENSIFY_CARD.FRAUD_TYPES.DOMAIN || card.fraud === CONST.EXPENSIFY_CARD.FRAUD_TYPES.INDIVIDUAL) {
assignedCardsGroupedItem.brickRoadIndicator = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
Expand Down
2 changes: 0 additions & 2 deletions tests/actions/ReportTest.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
/* eslint-disable @typescript-eslint/naming-convention */

/* eslint-disable rulesdir/prefer-at */
import {afterEach, beforeAll, beforeEach, describe, expect, it} from '@jest/globals';
import {utcToZonedTime} from 'date-fns-tz';
import Onyx from 'react-native-onyx';
Expand Down
10 changes: 5 additions & 5 deletions tests/perf-test/BaseOptionsList.perf-test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {fireEvent} from '@testing-library/react-native';
import type {RenderResult} from '@testing-library/react-native';
import React, {useState} from 'react';
import {measureRenders} from 'reassure';
import {measurePerformance} from 'reassure';
import BaseOptionsList from '@components/OptionsList/BaseOptionsList';
import type {OptionData} from '@libs/ReportUtils';
import variables from '@styles/variables';
Expand Down Expand Up @@ -67,7 +67,7 @@ describe('[BaseOptionsList]', () => {
}

test('Should render 1 section and a thousand items', () => {
measureRenders(<BaseOptionsListWrapper />);
measurePerformance(<BaseOptionsListWrapper />);
});

test('Should press a list item', () => {
Expand All @@ -76,7 +76,7 @@ describe('[BaseOptionsList]', () => {
fireEvent.press(screen.getByText('Item 5'));
};

measureRenders(<BaseOptionsListWrapper />, {scenario});
measurePerformance(<BaseOptionsListWrapper />, {scenario});
});

test('Should render multiple selection and select 4 items', () => {
Expand All @@ -88,7 +88,7 @@ describe('[BaseOptionsList]', () => {
fireEvent.press(screen.getByText('Item 4'));
};

measureRenders(<BaseOptionsListWrapper canSelectMultipleOptions />, {scenario});
measurePerformance(<BaseOptionsListWrapper canSelectMultipleOptions />, {scenario});
});

test('Should scroll and select a few items', () => {
Expand Down Expand Up @@ -120,6 +120,6 @@ describe('[BaseOptionsList]', () => {
fireEvent.press(screen.getByText('Item 15'));
};

measureRenders(<BaseOptionsListWrapper canSelectMultipleOptions />, {scenario});
measurePerformance(<BaseOptionsListWrapper canSelectMultipleOptions />, {scenario});
});
});
6 changes: 3 additions & 3 deletions tests/perf-test/ChatFinderPage.perf-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import React, {useMemo} from 'react';
import type {ComponentType} from 'react';
import Onyx from 'react-native-onyx';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import {measureRenders} from 'reassure';
import {measurePerformance} from 'reassure';
import {LocaleContextProvider} from '@components/LocaleContextProvider';
import OptionListContextProvider, {OptionsListContext} from '@components/OptionListContextProvider';
import {KeyboardStateProvider} from '@components/withKeyboardState';
Expand Down Expand Up @@ -187,7 +187,7 @@ test('[ChatFinderPage] should render list with cached options', async () => {
}),
)
.then(() =>
measureRenders(
measurePerformance(
<ChatFinderPageWithCachedOptions
route={{key: 'ChatFinder_Root', name: 'ChatFinder'}}
navigation={navigation}
Expand Down Expand Up @@ -221,7 +221,7 @@ test('[ChatFinderPage] should interact when text input changes', async () => {
}),
)
.then(() =>
measureRenders(
measurePerformance(
<ChatFinderPageWrapper
route={{key: 'ChatFinder_Root', name: 'ChatFinder'}}
navigation={navigation}
Expand Down
4 changes: 2 additions & 2 deletions tests/perf-test/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ We use Reassure for monitoring performance regression. It helps us check if our
- Reassure builds on the existing React Testing Library setup and adds a performance measurement functionality. It's intended to be used on local machine and on a remote server as part of your continuous integration setup.
- To make sure the results are reliable and consistent, Reassure runs tests twice – once for the current branch and once for the base branch.

## Performance Testing Strategy (`measureRenders`)
## Performance Testing Strategy (`measurePerformance`)

- The primary focus is on testing business cases rather than small, reusable parts that typically don't introduce regressions, although some tests in that area are still necessary.
- To achieve this goal, it's recommended to stay relatively high up in the React tree, targeting whole screens to recreate real-life scenarios that users may encounter.
Expand Down Expand Up @@ -84,7 +84,7 @@ test('Count increments on press', async () => {
await screen.findByText('Count: 2');
};

await measureRenders(
await measurePerformance(
<Counter />,
{ scenario, runs: 20 }
);
Expand Down
Loading

0 comments on commit 0993f36

Please sign in to comment.