Introduce support for calculating URL info for pending threads, as currently it could only parse URLs containing thread/<numbers>.
Because pending threads can have different forms of IDs, I added regular expression that recongnizes pending threads and correctly identifies pending ID returning it in URLInfo.
Details
Details
Tested manually by refreshing page in different pending threads (after introducing modifications in next diffs, it doesn't work now because of no mechanism for pending threads creation in nav reducer as for now).
Tested automatically with different examples using: https://regexr.com/6q7os
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jacek/tmp
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/shared/thread-utils.js | ||
|---|---|---|
| 272 | I'm a bit confused what "locally unique" means. It seems like this is just a RegExp for a pending threadID? | |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 260 | Oh, I guess that term started here. Looks like D2294 has some relevant context, but to be honest I'm having trouble recalling the reason we had for using this term instead of something simple like "pending". Can you help remind me? | |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 273 | I don't think it's necessary, since the RegExp object automatically escapes slashes:  However, I do it since I think it's considered good style/practice. Also Regxr complains:  | |
Comment Actions
Based on @yayabosh's analysis, it might be good to add the escapes, but I'm not 100% sure
| lib/shared/thread-utils.js | ||
|---|---|---|
| 260 | Requesting changes for a response to my question | |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 273 | Yes, you are right. I also did the escapes at first. However, both eslint and prettier complained about escaping characters:  So I followed the suggested version without escapes. | |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 260 | Yes, I also needed to recall what was the reason why the name was changed. If I understand correctly, we just wanted to use this function (inside locallyUniqueToRealizedThreadIDsSelector) to calculate "pending names" also for thread types that in app never can exist pending state (like subthreads). As a result, we concluded that the name getPendingThreadID doesn't cover this case. Looking at the code now, I suppose keeping using pendingThreadID instead of LocallyUniqueThreadID would have made the code more readable. | |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 273 | Hey @jacek, I've included a video here showing how to attach images to Phabricator now because they changed some permissions. Now, whenever copying and pasting an image in or uploading an image through the file upload system, you have to manually grant permissions for everyone else to be able to see the image. That is why currently I cannot see the image you attached.  | |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 273 | ||
Comment Actions
Looking at the code now, I suppose keeping using pendingThreadID instead of LocallyUniqueThreadID would have made the code more readable.
Awesome – what do you think the best way to follow up is? Maybe we should create a separate Linear task for it, since it seems like this change will touch a lot of files.
Comment Actions
Accepting, but please make sure to create a Linear task for the naming follow-up before landing!! Adding @yayabosh as blocking to make sure the RegExp escape question gets resolved, but seems like if ESLint / Prettier don't want the escapes we can leave them out
Comment Actions
Adding @yayabosh as blocking to make sure the RegExp escape question gets resolved, but seems like if ESLint / Prettier don't want the escapes we can leave them out
Yup, I agree with this assessment. I defer to ESLint / Prettier, and also JavaScript seems to automatically add escaping characters based on playing around with the console.
Comment Actions
I created the rename task: https://linear.app/comm/issue/ENG-1374/rename-locallyuniquethreadid-to-pendingthreadid
| lib/shared/thread-utils.js | ||
|---|---|---|
| 273 | Oh, I didn't know that. Thanks a lot for the info! | |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 273 | Not sure how closely we would like this regex to match our logic, but currently something like pending/type0/++++ is valid | |
| lib/utils/url-utils.js | ||
| 60 | Maybe add a comment near the regex definition telling that we're using groups and any change to the regex would require updating this logic? | |
| lib/utils/url-utils.js | ||
|---|---|---|
| 60 | Good idea! | |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 273 ↗ | (On Diff #14744) | Basing on https://phab.comm.dev/D4446#inline-28924 i changed the regex so it doesn't support URLs like pending/type6/123+++1. I don't know if there is any better way, but double escape "...\\+..." was the only solution I found, that makes the regex to work properly |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 273 ↗ | (On Diff #14744) | It is strange that in the earlier regex we didn't need to escape the + - pending/(type[0-9]+/[0-9+]+|sidebar/[0-9]+) (it was a part of range [0-9+]) but now we have to. But if escaping works fine, we can use it. |