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.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- patryk/new-chat-mentioning
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| keyserver/src/push/send.js | ||
|---|---|---|
| 480 | This will be deleted in the next diff. | |
Comment Actions
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 | I don't remember exactly but we might already have a type for this FetchServerThreadInfosResult, but not certain | |
| 439 | 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 | Why are we doing mentionedThreadChildrenTraversePath.length - 2? | |
| 464–471 | Might improve readability if these are separate variables | |
| 493–497 | 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 | |
Comment Actions
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 | We do, but it is not exported. Will export this type and use it here. | |
| 439 | Sure! | |
| 452–455 | mentionedThreadChildrenTraversePath is a path from mentionedThreadID to GENESIS community. baseParentTraverseQuery returns:
First case would never appear since we check if mentionedThreadID is in threadInfos. | |
| 493–497 | You're right, will fix this | |
Comment Actions
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 | Ok makes sense, thanks for the explanation! A code comment might help | |
| keyserver/src/push/send.js | ||
|---|---|---|
| 457–460 ↗ | (On Diff #30647) | 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! |
Comment Actions
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 ↗ | (On Diff #30647) | 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 ↗ | (On Diff #30647) | 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? |
| keyserver/src/push/send.js | ||
|---|---|---|
| 420 ↗ | (On Diff #30647) | Yes, it's way better to use threadInfos as a parameter... It doesn't matter but I will change it. |
| 425 ↗ | (On Diff #30647) | 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 ↗ | (On Diff #30647) | Makes sense! |
Comment Actions