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

Add support for inline code with space only characters #49038

Merged
merged 6 commits into from
Sep 26, 2024
Merged

Add support for inline code with space only characters #49038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c7bfed
Add support for inline code with space only characters
tsa321 Sep 12, 2024
102455a
Modify comment
tsa321 Sep 12, 2024
3c12da6
Merge main and resolve conflict
tsa321 Sep 18, 2024
d05cfc7
fix reportactionsutils lint errors
tsa321 Sep 20, 2024
6885f11
Merge branch 'main' into supportEmptyInlineCode
tsa321 Sep 20, 2024
81c3fbc
fix lints
tsa321 Sep 20, 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
8 changes: 4 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@
"date-fns-tz": "^2.0.0",
"dom-serializer": "^0.2.2",
"domhandler": "^4.3.0",
"expensify-common": "2.0.84",
"expensify-common": "2.0.88",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double-check, are you available to check if there are no breaking changes when we bump from 2.0.84 to 2.0.88?

Copy link
Contributor Author

@tsa321 tsa321 Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe there are no breaking changes. However, I have noticed another issue that also wants updating the expensify-common package (to version 2.0.87): #46117. Should we wait for a PR to be raised for that issue? or should we proceed with this PR?

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 we should. @deetergp what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've asked them in that PR if they want us to wait or not.

"expo": "51.0.31",
"expo-av": "14.0.7",
"expo-image": "1.12.15",
Expand Down
1 change: 1 addition & 0 deletions src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,7 @@ const CONST = {
MAX_PREVIEW_AVATARS: 4,
MAX_ROOM_NAME_LENGTH: 99,
LAST_MESSAGE_TEXT_MAX_LENGTH: 200,
MIN_LENGTH_LAST_MESSAGE_WITH_ELLIPSIS: 20,
OWNER_EMAIL_FAKE: '__FAKE__',
OWNER_ACCOUNT_ID_FAKE: 0,
DEFAULT_REPORT_NAME: 'Chat Report',
Expand Down
22 changes: 20 additions & 2 deletions src/libs/ReportActionsUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,11 @@ function isActionOfType<T extends ReportActionName[]>(

function getOriginalMessage<T extends ReportActionName>(reportAction: OnyxInputOrEntry<ReportAction<T>>): OriginalMessage<T> | undefined {
if (!Array.isArray(reportAction?.message)) {
// eslint-disable-next-line
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reportAction?.originalMessage is still needed here.

Previously, I was using eslint-disable-next-line deprecation/deprecation. This fixed the lint error for npm run lint-changed, but it caused a lint error for npm run lint because the deprecation/deprecation rule is not available in npm run lint; it only exists in npm run lint-changed.

Therefore, I am now using eslint-disable-next-line following the discussion in this comment.

return reportAction?.message ?? reportAction?.originalMessage;
}

// eslint-disable-next-line
return reportAction.originalMessage;
}

Expand Down Expand Up @@ -593,6 +596,7 @@ function isReportActionDeprecated(reportAction: OnyxEntry<ReportAction>, key: st

// HACK ALERT: We're temporarily filtering out any reportActions keyed by sequenceNumber
// to prevent bugs during the migration from sequenceNumber -> reportActionID
// eslint-disable-next-line
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sequenceNumber is still needed to check the deprecation of the reportAction.

if (String(reportAction.sequenceNumber) === key) {
Log.info('Front-end filtered out reportAction keyed by sequenceNumber!', false, reportAction);
return true;
Expand Down Expand Up @@ -753,6 +757,18 @@ function getLastVisibleAction(reportID: string, actionsToMerge: Record<string, N
return sortedReportActions[0];
}

function formatLastMessageText(lastMessageText: string) {
const trimmedMessage = String(lastMessageText).trim();

// Add support for inline code containing only space characters
// The message will appear as a blank space in the LHN
if ((trimmedMessage === '' && lastMessageText.length > 0) || (trimmedMessage === '?\u2026' && lastMessageText.length > CONST.REPORT.MIN_LENGTH_LAST_MESSAGE_WITH_ELLIPSIS)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share what are contexts that the second condition would be used?

Copy link
Contributor Author

@tsa321 tsa321 Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To test: try sending an inline code with many spaces inside it, for example:

The lastMessageText of the report from the back end is a lot of spaces, with an ellipsis at the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation.

return ' ';
}

return StringUtils.lineBreaksToSpaces(trimmedMessage).substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH).trim();
}

function getLastVisibleMessage(
reportID: string,
actionsToMerge: Record<string, NullishDeep<ReportAction> | null> = {},
Expand All @@ -777,7 +793,7 @@ function getLastVisibleMessage(

let messageText = getReportActionMessageText(lastVisibleAction) ?? '';
if (messageText) {
messageText = StringUtils.lineBreaksToSpaces(String(messageText)).substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH).trim();
messageText = formatLastMessageText(messageText);
}
return {
lastMessageText: messageText,
Expand Down Expand Up @@ -1698,7 +1714,7 @@ function isCardIssuedAction(reportAction: OnyxEntry<ReportAction>) {
}

function getCardIssuedMessage(reportAction: OnyxEntry<ReportAction>, shouldRenderHTML = false) {
const assigneeAccountID = (reportAction?.originalMessage as IssueNewCardOriginalMessage)?.assigneeAccountID;
const assigneeAccountID = (getOriginalMessage(reportAction) as IssueNewCardOriginalMessage)?.assigneeAccountID;
const assigneeDetails = PersonalDetailsUtils.getPersonalDetailsByIDs([assigneeAccountID], currentUserAccountID ?? -1)[0];

const assignee = shouldRenderHTML ? `<mention-user accountID="${assigneeAccountID}"/>` : assigneeDetails?.firstName ?? assigneeDetails.login ?? '';
Expand Down Expand Up @@ -1732,6 +1748,7 @@ function getCardIssuedMessage(reportAction: OnyxEntry<ReportAction>, shouldRende
export {
doesReportHaveVisibleActions,
extractLinksFromMessageHtml,
formatLastMessageText,
getActionableMentionWhisperMessage,
getAllReportActions,
getCombinedReportActions,
Expand All @@ -1754,6 +1771,7 @@ export {
getNumberOfMoneyRequests,
getOneTransactionThreadReportID,
getOriginalMessage,
// eslint-disable-next-line
Copy link
Contributor

Choose a reason for hiding this comment

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

can you double check if we need this disabled line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hoangzinh Yes, we need this. Removing this line will cause errors when I run npm run lint-changed.

getParentReportAction,
getRemovedFromApprovalChainMessage,
getReportAction,
Expand Down
4 changes: 2 additions & 2 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ import * as PolicyUtils from './PolicyUtils';
import type {LastVisibleMessage} from './ReportActionsUtils';
import * as ReportActionsUtils from './ReportActionsUtils';
import * as ReportConnection from './ReportConnection';
import StringUtils from './StringUtils';
import * as TransactionUtils from './TransactionUtils';
import * as Url from './Url';
import type {AvatarSource} from './UserUtils';
Expand Down Expand Up @@ -1852,7 +1851,8 @@ function formatReportLastMessageText(lastMessageText: string, isModifiedExpenseM
if (isModifiedExpenseMessage) {
return String(lastMessageText).trim().replace(CONST.REGEX.LINE_BREAK, '').trim();
}
return StringUtils.lineBreaksToSpaces(String(lastMessageText).trim()).substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH).trim();

return ReportActionsUtils.formatLastMessageText(lastMessageText);
}

/**
Expand Down
Loading