Add room name to the top of searched chats #50135

zfurtak · 2024-10-03T08:47:59Z

Details

Fixed Issues

$ #48897
PROPOSAL:

Tests

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

Kicu

Left you some comments with small code tweaks

Kicu · 2024-10-03T13:11:38Z

src/libs/ReportUtils.ts

+    draftReports?: OnyxCollection<Report>,
+    reportNameValuePairs?: OnyxCollection<ReportNameValuePairs>,
+    policyTagLists?: OnyxCollection<PolicyTagLists>,
+    policies?: OnyxCollection<Policy>,


@luacmartins What do you think about this? that kinda messy :/ Frankly I'm surprised that we need some many arguments to generate the name.
I'm Afraid we are making the code very complex, perhaps it's a good moment to take a step back and redesign this a bit?

If we have to compute the reportName in the frontend codebase, then maybe temporary could somehow split this function for reports and for Search?

Clearly this was written specifically for the ReportUtils with the assumption that a lot of local values will be used. We are now breaking this assumption, I wonder if we can develop the "getReportForSearch" part somehow smarter 🤔

Frankly I'm surprised that we need some many arguments to generate the name.

Me too 😄

If we have to compute the reportName in the frontend codebase, then maybe temporary could somehow split this function for reports and for Search?

I raised this in Slack with the team and didn't get much support since that'd create another fork in the logic. How exactly would you propose the split? Maybe I can bring this up again because I also agree with a separate approach since the refactor might get out of hand.

One thing that I was thinking, is that even after this refactor, there's nothing stopping others from directly connecting these methods to Onyx in the future and the whole logic breaking for Search. We should probably think of a way to enforce that once the refactor is done.

src/libs/ReportUtils.ts

luacmartins · 2024-10-18T22:53:51Z

zfurtak · 2024-10-24T19:45:36Z

The only functions that left to refactor is Parser.htmlToText I'm still figuring that out 🔍
Others are done ✅

zfurtak · 2024-10-25T10:12:43Z

Never mind! The function Parser.htmlToText doesn't need refactoring. So all functions are ready 😊

luacmartins · 2024-10-25T15:32:48Z

Nice! That's awesome. The team is still discussing if we can simplify the report names for the Search page only (especially for threads since those add a lot of extra data). I'll report back once we land on a solution.

zfurtak · 2024-11-07T17:14:39Z

@luacmartins how is the discussion going? 😊

luacmartins · 2024-11-07T17:37:52Z

There hasn't been any movement, but last I checked the decision was to:

On Search only, show the highest context report name (e.g. From David, Jason, Tom) as the report name for all types of threads

Additionally, we started returning additional data for Search > Expenses. I'm hoping to do the same and include report and policy data in the Search results for chats. This + the simplification above might solve a lot of the naming issues 🤞

luacmartins · 2024-11-07T17:39:04Z

So I think next steps here:

Could you make a list of Onyxkeys we'd need to send with Search response?
Work on a function to return the simplified thread name on Search

zfurtak · 2024-11-18T08:38:36Z

Could you make a list of Onyxkeys we'd need to send with Search response?

By this you mean the keys that were meant to be used to calculate the report name? 🤔

Work on a function to return the simplified thread name on Search

Are we waiting with this for the other PR to be merged? 😊

luacmartins · 2024-11-18T16:30:09Z

By this you mean the keys that were meant to be used to calculate the report name? 🤔

Yes, we'll need to send those keys to the client in order to compute the report name.

Are we waiting with this for the other PR to be merged?

Yea, I think we need to map out the necessary keys to compute the report name. Then I'd implement the necessary changes in the API to send this data to the client and finally, we can use it to compute the name.

luacmartins · 2024-12-19T20:27:47Z

@zfurtak I started working on the backend PR to return the necessary data to display the room names. Here's a preliminary result:

Screen.Recording.2024-12-19.at.2.26.14.PM.mov

I think we need to add some more keys to make it work for some cases like Workspace chats, etc. But I think we could start with this as v1

luacmartins · 2024-12-19T20:32:11Z

I needed to make some small changes to the solution you have in this PR, you can see a rough draft here. Please incorporate those changes to your PR. The main idea is:

Extract the data needed by getReportName from the data passed to getReportActionsSections
Call getReportName within getReportActionsSections
Add a reportName key to SearchReportAction
Display it when rendering the item in ChatListItem

luacmartins · 2024-12-30T23:40:57Z

@zfurtak backend PR is merged. Let's resume work on this one taking my comment above into account.

luacmartins · 2025-01-09T17:49:21Z

@zfurtak let me know when you are ready to resume work on this issue

luacmartins · 2025-01-14T17:01:18Z

@zfurtak how's work going on this one?

zfurtak · 2025-01-15T14:14:29Z

I used the code from your draft PR and right now I'm adjusting it and fixing lint mistakes connected to the new eslint rule.

luacmartins · 2025-01-15T15:07:38Z

Nice! Thanks for the update!

zfurtak · 2025-01-16T20:42:41Z

Ok, so update from me: the code is ready 🎉 As I have been tested, it all works!
I asked my colleagues from swm for a review, to have the code checked by someone more experienced with search, so after that it will be done 💪
I'll be away for a few days, but I'll be back on Wednesday to adjust the code and then we'll be good to go forward with it 😃

About the failing check Changed files ESLint check / Changed files ESLint check (pull_request):
Since this PR changes a lot of different files, and there are a ton of bugs due to newly introduced eslint rules, I think it's better not to cover them in this PR, just in case.

luacmartins · 2025-01-16T21:57:38Z

This is awesome @zfurtak! I did some testing and things look really good. Do you think we can already open the PR for reviews from C+ too? It'd be a big win to wrap this one up soon.

About the failing check Changed files ESLint check / Changed files ESLint check (pull_request):
Since this PR changes a lot of different files, and there are a ton of bugs due to newly introduced eslint rules, I think it's better not to cover them in this PR, just in case.

I'm ok handling them in a follow up since they are all unrelated and I agree that we'd get a lot of conflicts and potential issues by tackling them here.

zfurtak added 7 commits September 17, 2024 13:48

Added policy header to chats in search

c756e74

Added report name

c0086bc

Merge branch 'main' into @zfurtak/search-chat-header

38ee190

First part of refactor

ec5d2fb

Done refactoring functions

664ee13

Style change

2c36ccc

Merge branch 'main' into @zfurtak/search-chat-header

1bac0f2

zfurtak changed the title ~~Add header In to chat search~~ Add room name to the top of searched chats Oct 3, 2024

zfurtak added 2 commits October 3, 2024 15:34

Change props passing to getReportName

0a8ef3e

Merge branch 'main' into @zfurtak/search-chat-header

7c1dae3

Kicu reviewed Oct 3, 2024

View reviewed changes

zfurtak added 3 commits October 7, 2024 09:38

Adjusted to review

47cceca

Adjusted to review

06d6e65

Merge branch 'main' into @zfurtak/search-chat-header

138b819

luacmartins self-requested a review October 18, 2024 21:35

zfurtak added 2 commits October 21, 2024 14:02

Merge main

df25e44

Fixed checks

19a209d

luacmartins mentioned this pull request Oct 21, 2024

[$250] [Search v2.2] Add room name to the top of chats #48897

Open

zfurtak added 3 commits October 24, 2024 20:23

Adjusted to comments

1bf9696

Merge main

0dbd2c0

Refactored getReimbursementQueuedActionMessage

c729073

Fixed ts issue

88cd372

Merge main

86bfff4

zfurtak force-pushed the @zfurtak/search-chat-header branch from 5f346f6 to 86bfff4 Compare December 4, 2024 14:27

Merged main

43d884f

luacmartins mentioned this pull request Jan 2, 2025

Room names #54394

Closed

50 tasks

Merged main

0d26962

Fixing lint errors

472bf48

zfurtak added 2 commits January 15, 2025 14:48

Fix

dfc5f0c

Revert change in package.json

4fcc57a

zfurtak added 8 commits January 15, 2025 16:42

Merge main

a5a2fac

Merge branch 'main' into react-native-wallet-poc

c37dbc3

Fix types errors

cda82b9

Merge branch 'main' into react-native-wallet-poc

31955d9

Fix last typescript errors

bdfefdd

Quick fix

3c26949

Merge main

a2c3c27

Adjust the getReportName mock

8ee8560

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add room name to the top of searched chats #50135

Add room name to the top of searched chats #50135

zfurtak commented Oct 3, 2024 •

edited

Loading

Kicu left a comment

Kicu Oct 3, 2024

luacmartins Oct 21, 2024

luacmartins Oct 22, 2024

luacmartins commented Oct 18, 2024 •

edited

Loading

zfurtak commented Oct 24, 2024

zfurtak commented Oct 25, 2024

luacmartins commented Oct 25, 2024 •

edited

Loading

zfurtak commented Nov 7, 2024

luacmartins commented Nov 7, 2024

luacmartins commented Nov 7, 2024

zfurtak commented Nov 18, 2024

luacmartins commented Nov 18, 2024

luacmartins commented Dec 19, 2024

luacmartins commented Dec 19, 2024 •

edited

Loading

luacmartins commented Dec 30, 2024 •

edited

Loading

luacmartins commented Jan 9, 2025

luacmartins commented Jan 14, 2025

zfurtak commented Jan 15, 2025

luacmartins commented Jan 15, 2025

zfurtak commented Jan 16, 2025

luacmartins commented Jan 16, 2025

Add room name to the top of searched chats #50135

Are you sure you want to change the base?

Add room name to the top of searched chats #50135

Conversation

zfurtak commented Oct 3, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Kicu left a comment

Choose a reason for hiding this comment

Kicu Oct 3, 2024

Choose a reason for hiding this comment

luacmartins Oct 21, 2024

Choose a reason for hiding this comment

luacmartins Oct 22, 2024

Choose a reason for hiding this comment

luacmartins commented Oct 18, 2024 • edited Loading

zfurtak commented Oct 24, 2024

zfurtak commented Oct 25, 2024

luacmartins commented Oct 25, 2024 • edited Loading

zfurtak commented Nov 7, 2024

luacmartins commented Nov 7, 2024

luacmartins commented Nov 7, 2024

zfurtak commented Nov 18, 2024

luacmartins commented Nov 18, 2024

luacmartins commented Dec 19, 2024

luacmartins commented Dec 19, 2024 • edited Loading

luacmartins commented Dec 30, 2024 • edited Loading

luacmartins commented Jan 9, 2025

luacmartins commented Jan 14, 2025

zfurtak commented Jan 15, 2025

luacmartins commented Jan 15, 2025

zfurtak commented Jan 16, 2025

luacmartins commented Jan 16, 2025

zfurtak commented Oct 3, 2024 •

edited

Loading

luacmartins commented Oct 18, 2024 •

edited

Loading

luacmartins commented Oct 25, 2024 •

edited

Loading

luacmartins commented Dec 19, 2024 •

edited

Loading

luacmartins commented Dec 30, 2024 •

edited

Loading