[HOLD for Payment OCT 08] [$250] [Search v2.3] - Default name is not displayed correctly for multiline system message thread

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.36-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to DM.
3. Submit an expense with a multiline description.
4. Go to transaction thread.
5. Click Description.
6. Edit the description while maintaining the multiline format and save it.
7. Right click on the system message > Reply in thread.
8. Send a reply in thread.
9. Go to Search > Chats.
10. Click Filters > Select "In".
11. Select the thread in Step 8 > Save.
12. Click Save search.

Expected Result:

The title of the saved search should only have one line.

Actual Result:

The title of the saved search has multiple lines and it is not displayed correctly.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6606270_1726554285396.20240917_141656.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021836683698134866207
• Upwork Job ID: 1836683698134866207
• Last Price Increase: 2024-09-19
• Automatic offers:
• paultsimura | Reviewer | 104047003
• Krishna2323 | Contributor | 104047004

Issue Owner

Current Issue Owner: @paultsimura

[melvin-bot]

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

We think that this bug might be related to #wave-control

[IuliiaHerets]

@abekkala FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[CyberAndrii]

Proposal

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

Saved search - Default name is not displayed correctly for multiline system message thread

What is the root cause of that problem?

The title text is not wrapped correctly.

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

Add titleStyle: styles.textNoWrap, here

App/src/pages/Search/SearchTypeMenu.tsx

Line 110 in 14b99ca

title,

[drminh2807]

Proposal

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

Saved search - Default name is not displayed correctly for multiline system message thread

What is the root cause of that problem?

The title text is not wrapped correctly.

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

add numberOfLinesTitle: 2, in

App/src/pages/Search/SearchTypeMenu.tsx

Lines 108 to 117 in 14b99ca

	const baseMenuItem: SavedSearchMenuItem = {
	key,
	title,
	hash: key,
	query: item.query,
	shouldShowRightComponent: true,
	focused: Number(key) === hash,
	onPress: () => {
	SearchActions.clearAllFilters();
	Navigation.navigate(ROUTES.SEARCH_CENTRAL_PANE.getRoute({query: item?.query ?? ''}));

What alternative solutions did you explore? (Optional)

add noWrap style
titleStyle: styles.textNoWrap,

[Krishna2323]

Proposal


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

Saved search - Default name is not displayed correctly for multiline system message thread

What is the root cause of that problem?

The default value for numberOfLinesTitle is 1 but the text overflows because we are applying styles.pre instead of styles.noWrap.

App/src/components/MenuItem.tsx

Line 350 in 2190f62

numberOfLinesTitle = 1,

App/src/components/MenuItem.tsx

Lines 444 to 457 in 2190f62

	const combinedTitleTextStyle = StyleUtils.combineStyles(
	[
	styles.flexShrink1,
	styles.popoverMenuText,
	// eslint-disable-next-line no-nested-ternary
	shouldPutLeftPaddingWhenNoIcon || (icon && !Array.isArray(icon)) ? (avatarSize === CONST.AVATAR_SIZE.SMALL ? styles.ml2 : styles.ml3) : {},
	shouldShowBasicTitle ? {} : styles.textStrong,
	numberOfLinesTitle !== 1 ? styles.preWrap : styles.pre,
	interactive && disabled ? {...styles.userSelectNone} : {},
	styles.ltr,
	isDeleted ? styles.offlineFeedback.deleted : {},
	],
	titleStyle ?? {},
	);

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


We should update the condition below to numberOfLinesTitle !== 1 ? styles.preWrap : styles.noWrap,.

App/src/components/MenuItem.tsx

Line 451 in 2190f62

numberOfLinesTitle !== 1 ? styles.preWrap : styles.pre,

What alternative solutions did you explore? (Optional)

• Add numberOfLinesTitle: 2 in createSavedSearchMenuItem..
• Same should be applied in SearchTypeMenuNarrow.

Result

[truph01]

Proposal

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

• The title of the saved search has multiple lines and it is not displayed correctly.

What is the root cause of that problem?

• We return the raw report name in this:
  

  App/src/libs/SearchUtils.ts

  Line 715 in 7bd9e53

  return ReportUtils.getReportName(reports?.[`${ONYXKEYS.COLLECTION.REPORT}${filter}`]);

so the saved search name can contain multiple lines.

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

• We can fix the issue by fixing the Text style, but with that approach, we need to apply the fix on all platforms. So I suggest that we can format this:

App/src/libs/SearchUtils.ts

Line 715 in 7bd9e53

return ReportUtils.getReportName(reports?.[`${ONYXKEYS.COLLECTION.REPORT}${filter}`]);

so it can be:

        return ReportUtils.formatReportLastMessageText(ReportUtils.getReportName(reports?.[`${ONYXKEYS.COLLECTION.REPORT}${filter}`]));

like we did in this:

App/src/components/DisplayNames/DisplayNamesWithTooltip.tsx

Line 59 in 7bd9e53

? ReportUtils.formatReportLastMessageText(fullTitle)

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[paultsimura]

Reviewing now 👀

[melvin-bot]

Current assignees @luacmartins and @lakchote are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[Krishna2323]

@paultsimura, my proposal here solves the root cause. Could you please check again?

[paultsimura]

On a side note, @Krishna2323 please focus on finishing your open PRs instead of posting new proposals as the contribution guideline requires.

[paultsimura]

@paultsimura, my proposal here solves the root cause. Could you please check again?

Your proposal is very invasive, it suggests replacing the formatting for all the MenuItem components, which can cause regressions.

@CyberAndrii any chance you could find an example where using this proposal breaks styling in other places>

[truph01]

@paultsimura What do you think about my solution, which just needs to use ReportUtils.formatReportLastMessageText without any style changes? Thank you.

[truph01]

I tried to test your solution but It does not fix in case of IOS safari. Can you help confirm if I was wrong? @CyberAndrii

[Krishna2323]

@paultsimura, I don't think it will cause any regression because we will be only applying styles.noWrap when numberOfLines is 1.

[paultsimura]

Valid point @truph01. Taking another look at the proposals now...

[paultsimura]

Ok, after another look, I tend to agree with @Krishna2323's proposal.

It makes sense to have noWrap where numberOfLines: 1.
However, to be on the safe side, we'll have to go through the places that use MenuItem component with numberOfLines: 1 (or no value provided, meaning it uses the default – still 1), and check if they really are meant to have numberOfLines: 1 or if we should update them by specifying the correct numberOfLines.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignees @luacmartins and @lakchote are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[truph01]

@paultsimura Please note we need to fix the search header page with that solution as well:

[paultsimura]

@paultsimura Please note we need to fix the search header page with that solution as well:

There is no such requirement. From what I see, the search header is explicitly set to have numberOfLines: 2. If the design team requests so, we can change it to numberOfLines: 1.

App/src/components/Search/SearchPageHeader.tsx

Lines 59 to 66 in 14f952a

	subtitle={
	<Text
	numberOfLines={2}
	style={[styles.textLarge, subtitleStyles]}
	>
	{subtitle}
	</Text>
	}

[melvin-bot]

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[paultsimura]

@Expensify/design should we make the search header subtitle 1-lined with no wrap, or keep it as is (currently it's 2 lines)?

[shawnborton]

It should only be 1 line and then truncated but why is it not occupying more horizontal space? There's a lot more space over on the right that it could be using.

[paultsimura]

why is it not occupying more horizontal space?

That's because the text contains line breaks that are preserved. Changing numberOfLines to 1 resolves it:

@Krishna2323 please consider this in your PR.

[shawnborton]

Ah nice, that's much better

[Krishna2323]

@Krishna2323 please consider this in your PR.

Yeah, sure.

[Krishna2323]

@paultsimura, PR ready for review ^

[luacmartins]

PR merged

[paultsimura]

Deployed to production on Oct 1. Payment is due on 2024-10-08

[abekkala]

PAYMENT SUMMARY FOR OCT 08

• Fix: @Krishna2323 [$250, if no regressions] OFFER
• PR Review: @paultsimura [$250, if no regressions] OFFER
  Please complete checklist below

[abekkala]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@paultsimura] The PR that introduced the bug has been identified. Link to the PR:
• [@paultsimura] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@paultsimura] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.
• [@paultsimura] Determine if we should create a regression test for this bug.
• [@paultsimura] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[paultsimura]

• The PR that introduced the bug has been identified. Link to the PR: note: It's not an offending PR as such, just a PR that tried to solve an existing problem, but didn't do it fully: Fix/37448: Incorrect title thread #38267
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/38267/files#r1791506146
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• Determine if we should create a regression test for this bug: Yes
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. Go to DM.
2. Submit an expense with a multiline description.
3. Go to the transaction thread.
4. Click Description.
5. Edit the description while maintaining the multiline format and save it.
6. Right-click on the system message > Reply in thread.
7. Send a reply in the thread.
8. Go to Search > Chats.
9. Click Filters > Select "In".
10. Select the thread in Step 7 > Save.
11. Click "Save search".
12. Verify the title of the saved search is only shown in one line.

Do we agree 👍 or 👎

[abekkala]

@Krishna2323 @paultsimura payments sent and contracts ended - thank you! 🎉