[#15741] Fix error on tapping pinned message

[rasom]

fix #15741

status: ready

[status-im-auto]

Jenkins Builds

Click to see older builds (8)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	c277411	#1	2023-04-26 10:00:06	~5 min	android	🤖apk 📲
✔️	c277411	#1	2023-04-26 10:00:25	~6 min	tests	📄log
✔️	c277411	#1	2023-04-26 10:00:30	~6 min	android-e2e	🤖apk 📲
✔️	c277411	#1	2023-04-26 10:02:28	~8 min	ios	📱ipa 📲
✔️	9d6a1df	#2	2023-04-26 10:13:14	~7 min	android-e2e	🤖apk 📲
✔️	9d6a1df	#2	2023-04-26 10:13:15	~7 min	android	🤖apk 📲
✔️	9d6a1df	#2	2023-04-26 10:13:18	~7 min	ios	📱ipa 📲
✔️	9d6a1df	#2	2023-04-26 10:13:53	~7 min	tests	📄log

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	36ffb63	#3	2023-04-26 13:57:58	~6 min	android-e2e	🤖apk 📲
✔️	36ffb63	#3	2023-04-26 13:58:15	~6 min	tests	📄log
✔️	36ffb63	#3	2023-04-26 13:58:27	~6 min	android	🤖apk 📲
✔️	36ffb63	#3	2023-04-26 14:10:46	~18 min	ios	📱ipa 📲
❌	fb326bb	#4	2023-04-27 08:40:56	~3 min	ios	📄log
✔️	fb326bb	#4	2023-04-27 08:43:46	~6 min	tests	📄log
✔️	fb326bb	#4	2023-04-27 08:44:02	~6 min	android-e2e	🤖apk 📲
✔️	fb326bb	#4	2023-04-27 08:44:03	~6 min	android	🤖apk 📲
✔️	fb326bb	#5	2023-04-27 08:51:16	~9 min	ios	📱ipa 📲

[ilmotta]

@rasom, I understand the scope is for a fix, but what's going on under the hood is more important imo. The atom should be defined in the first place, so the fix to me should be somewhere else.

I traced the argument keyboard-shown and found a place where it's not passed as an arg https://github.com/status-im/status-mobile/blob/develop/src/status_im2/contexts/chat/menus/pinned_messages/view.cljs#L20

Unfortunately hiccup is a bit nasty, because [message/message-with-reactions message context] is a vector, there's nothing checking that the message/message-with-reactions arity is wrong. Look what I get if I change the vector to a function call, I get a warning from clj-kondo and the code won't actually compile.

invalid-arity status-im2.contexts.chat.messages.content.view/message-with-reactions is called with 2 args but expects 3 (lsp)

So I think the correct fix is to fix the bad hiccup https://github.com/status-im/status-mobile/blob/develop/src/status_im2/contexts/chat/menus/pinned_messages/view.cljs#L20 and pass along the keyboard-shown atom. You'll need to create it and pass it as the render-data of the flat-list

status-mobile/src/status_im2/contexts/chat/menus/pinned_messages/view.cljs

Line 51 in 0649c66

:render-data render-data

These days I hardly like hiccup... so much trouble for so little, anyway.

[OmarBasem]

@ilmotta is there something bad with passing (atom false) as a third argument to https://github.com/status-im/status-mobile/blob/develop/src/status_im2/contexts/chat/menus/pinned_messages/view.cljs#L20 ?

[ilmotta]

I think it's fine @OmarBasem, as long as we respect the function arity and don't pass an atom as undefined it's better.

[ilmotta]

Maybe for the future, not in this PR, but the "perfect" fix could be to avoid passing keyboard-shown all the way down do other components and instead use a hook right at the place where it's needed. The hook would probably be similar or identical to the one we discussed these days here #15736 (comment)

[OmarBasem]

@ilmotta I don't think that would be a better solution. The keyboard-shown value is needed when pressing on a message. Defining the hook in the message component we would create a new hook for every message.

[ilmotta]

Correct, hooks are cheap, but not that cheap. For long lists of things it's not a good idea, good point :)

[rasom]

hey guys, changed it to (atom false), thanks

[ilmotta]

Thanks for taking the time to try a different fix @rasom.