Details

Reviewers

rohan
ginsu
atul
tomek

Commits

rCOMMa37290eb224f: [native] Block editing messages on search and pinned messages screens, and of…

Summary

issues: https://linear.app/comm/issue/ENG-4834/editing-sidebar-source-message-behaves-weirdly#comment-e38584c8
https://linear.app/comm/issue/ENG-4847/not-possible-to-pin-the-source-messge#comment-9971bb16
This is a bit of a hacky solution to prevent editing in search, pinned messages and of the first message in a thread. The first two cases will be resolved by https://linear.app/comm/issue/ENG-3172/implement-navigating-to-the-message-picked-from-the-search-screen, where we will implement navigating to a message. So that hack will be reverted. The third one looks like a bug, and we will probably want to investigate it at some point, and fix it.

On web everything works correctly, so I left it for now

Test Plan

checked that "edit" is not present in tooltip in search results, pinned messages screen and for the first message in a thread. Checked that it is present for a message in parent thread from which the thread was created, as well as other messages in the thread and parent chat.

Diff Detail

Repository

rCOMM Comm

Branch

inka/edit_fix

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Sep 7 2023, 1:35 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 7 2023, 1:35 AM

inka edited the summary of this revision. (Show Details)Sep 7 2023, 1:36 AM

inka edited the summary of this revision. (Show Details)Sep 7 2023, 1:45 AM

inka edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B22421: Diff 30825.Sep 7 2023, 1:54 AM

inka requested review of this revision.Sep 7 2023, 1:54 AM

tomek added inline comments.Sep 7 2023, 2:34 AM

native/chat/text-message-tooltip-modal.react.js
134–135 ↗	(On Diff #30825)	This is really hacky. At least, we should use a constant defined somewhere, e.g. `MessageSearchRouteName`. Ideally, a screen should tell the tooltip if the message should be editable. E.g. as a parameter to `createTooltip` function, or through `TooltipContext`... do you think we can do something like this?

Requesting changes for feedback above

This revision now requires changes to proceed.Sep 14 2023, 9:19 AM

Use constant. Move logic to ConnectedTextMessage, where other conditions for whether a message can be edited are checked, and a canEditMessage value used to create visibleEntryIDs which are used by TooltipContext to determine whether the item should be rendered, is created.

Harbormaster completed remote builds in B22756: Diff 31308.Sep 20 2023, 4:06 AM

tomek accepted this revision.Sep 20 2023, 5:42 AM

tomek added inline comments.

native/chat/text-message.react.js
276 ↗	(On Diff #31308)	How about renaming it?
281 ↗	(On Diff #31308)	What do you think about checking screen name inside this hook? (should be possible using nav hooks)

rohan accepted this revision.Sep 25 2023, 6:35 AM

rohan added inline comments.

native/chat/text-message.react.js
281 ↗	(On Diff #31308)	I agree, would make it easier to have the checks in the hook to modify it in the future if needed (and if the hook is used in other places)