issue: https://linear.app/comm/issue/ENG-4910/message-search-show-unpin-messages
This diff fixes toggle pin robotexts showing up in search results
Details
Details
checked that the robotext does not show up in search results, but the messages do.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- inka/fix_search
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Is there a way we can implement this fix in a way that doesn't require copy-paste between the search and pinned messages views? Besides DRY, I'm worried that a future author of a new, similar interface won't expect that they need to filter these message types out. I'd love if there was some shared utility function that could be used
Comment Actions
Changes look good, thanks for fixing this!
Regarding @ashoat's comment, we could probably just make a utility function in lib called visibleMessagesInResultsModal or something and do the whole filtering logic, but I think we filter off different conditions between search and pinned modals. Even so, it all being in one function would make it easier to make future changes even if we need to use conditionals and filter differently based on the modal. I can try to handle it in a separate diff if it's time consuming
| native/search/message-search.react.js | ||
|---|---|---|
| 111 | Same comment as below | |
| web/modals/search/message-search-utils.react.js | ||
| 46 | Does isComposableMessageType work here (just to keep it consistent with pinned messages)? | |
Comment Actions
Requesting review, because I ended up extracting a lot of common code to lib. This is code that takes messages returned from messageListData and filters them to just include the messages we initaily fetched from the search endpoint
| web/modals/search/message-search-utils.react.js | ||
|---|---|---|
| 46 | Yes! will update! | |
Comment Actions
I removed copy paste between search on web and native. I will create a followup task to use this function in pinned messages, because it seems to me that this logic wan be used there as well.