Page MenuHomePhabricator

[keyserver] Introduce function which checks if thread is mentionable
AbandonedPublic

Authored by patryk on Aug 29 2023, 5:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jul 12, 12:01 AM
Unknown Object (File)
Tue, Jul 9, 7:17 PM
Unknown Object (File)
Tue, Jul 9, 4:29 PM
Unknown Object (File)
Tue, Jul 9, 4:29 PM
Unknown Object (File)
Tue, Jul 9, 4:29 PM
Unknown Object (File)
Tue, Jul 9, 4:29 PM
Unknown Object (File)
Tue, Jul 9, 4:29 PM
Unknown Object (File)
Tue, Jul 9, 4:28 PM
Subscribers

Details

Summary

This diff introduces a function which checks if thread is mentionable. It will be used when raw mention appears in notification text: we want to render it accordingly to users thread visibility.

Depends on D9005.

Test Plan

Tested later in the stack.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

patryk held this revision as a draft.
patryk edited the test plan for this revision. (Show Details)
patryk added reviewers: tomek, inka, rohan.
patryk edited the summary of this revision. (Show Details)

You cannot mention thread where you write

patryk added inline comments.
keyserver/src/push/send.js
480 ↗(On Diff #30495)

This will be deleted in the next diff.

rohan requested changes to this revision.Aug 29 2023, 11:00 AM

The logic and the if/elses seem a little complicated, but at a first glance I'm not entirely certain if there's a simple way to decrease complexity. I'm also not super familiar with recursive SQL queries, so maybe it'd be good to have a blocking reviewer who is more familiar

keyserver/src/push/send.js
417 ↗(On Diff #30495)

I don't remember exactly but we might already have a type for this FetchServerThreadInfosResult, but not certain

439 ↗(On Diff #30495)

Are you able to remove the if/else since you're returning in the if? This would help reduce the indentation for the long else block

452–455 ↗(On Diff #30495)

Why are we doing mentionedThreadChildrenTraversePath.length - 2?

464–471 ↗(On Diff #30495)

Might improve readability if these are separate variables

493–497 ↗(On Diff #30495)

We try to make awaits as clear as possible, so awaiting it into a variable then checking the variable would be much clearer. Feel free to use any variable name, just a suggestion

This revision now requires changes to proceed.Aug 29 2023, 11:00 AM

maybe it'd be good to have a blocking reviewer who is more familiar [with recursive SQL queries]

This query was taken from here (just changed containing_thread_id to parent_thread_id). It was actually written by me (with Ashoats help) so logically I should be a blocking reviewer since this code is the only recursive query in the keyserver :D. But I will find someone who knows more about recursive queries and add them as a blocking reviewer.

keyserver/src/push/send.js
417 ↗(On Diff #30495)

We do, but it is not exported. Will export this type and use it here.

439 ↗(On Diff #30495)

Sure!

452–455 ↗(On Diff #30495)

mentionedThreadChildrenTraversePath is a path from mentionedThreadID to GENESIS community. baseParentTraverseQuery returns:

  • [ ] when threadID is not found in threads table
  • ['1'] when threadID is equal to GENESIS community
  • [...., 'firstGENESISChildID', '1' ] otherwise

First case would never appear since we check if mentionedThreadID is in threadInfos.
Second case also would never appear: if mentionedThread would be a GENESIS community, then mentionedThread.community !== genesis.id would be true, since GENESIS community thread doesn't have community property set (community = NULL).
So because of above reasons, we check the second to last item in a traverse path.

493–497 ↗(On Diff #30495)

You're right, will fix this

maybe it'd be good to have a blocking reviewer who is more familiar [with recursive SQL queries]

This query was taken from here (just changed containing_thread_id to parent_thread_id). It was actually written by me (with Ashoats help) so logically I should be a blocking reviewer since this code is the only recursive query in the keyserver :D. But I will find someone who knows more about recursive queries and add them as a blocking reviewer.

Ahh ok thanks for the link. Makes sense in that case, was just worried about the performance of it on production where there are a lot more threads but I think it should be ok

keyserver/src/push/send.js
452–455 ↗(On Diff #30495)

Ok makes sense, thanks for the explanation! A code comment might help

keyserver/src/push/send.js
457–460

I think that looking at the name of this variable, above comment in L448-450 and the whole code will explain why we get mentionedThreadChildrenTraversePath.length - 2 element from traverse path. Let me know if I'm wrong!

I spent some time walking through the logic and it makes sense to me. Extracting out messageThread.community, mentionedThread.community and genesis.id to variables and using those instead of accessing the object each time would read better to me, but that may just be personal preference

keyserver/src/push/send.js
420

It's ultimately the same thing either way, you could just destructure threadInfos in getMentionableThreads and pass it in here since that's the only reason you pass in fetchThreadResult. Shouldn't matter though

425

Does this mean, if I am in a chat 'daily updates', and I mention '@daily updates', the chat will not be mentionable? So it'll render as normal text and not a bolded chat mention?

This revision is now accepted and ready to land.Sep 1 2023, 7:40 AM
keyserver/src/push/send.js
420

Yes, it's way better to use threadInfos as a parameter... It doesn't matter but I will change it.

425

Yes, you can't mention a chat where you write (see this comment), thus it will be rendered as normal text.

keyserver/src/push/send.js
425

Makes sense!

ashoat requested changes to this revision.Sep 11 2023, 1:42 PM

See comments in D9007. Given @patryk's limited time left on Comm, I think we need to abandon this approach

keyserver/src/push/send.js
404–415

Please revise this so that the indentation matches the approach elsewhere in the codebase

496

You are doing an await in sequence for a whole collection here!! This is an extreme anti-pattern

This revision now requires changes to proceed.Sep 11 2023, 1:42 PM

Abandoning this diff since I don't have much time left in Comm.