Better thread slugs #3762
Better thread slugs #3762
Conversation
|
@brianlovin I think this is ready apart from replacing all links to
This all works because we literally ignore everything except for the ID, and then redirect to the correct URL once loaded. 😜 That makes everything work, including moving threads between channels! Can you think of anything else we might need to handle apart from the link updates? |
|
Really interesting solution, thanks for digging into this! It's interesting because we don't actually care about the url at all when performing the fetch, but the canonical url will be super helpful with SEO. In your solution, if I'm reading this correctly, it seems like moving channels won't affect any url at all - if someone shares an old So this seems like good work, let's give it a go! Want to update all the Also: am I correct that this won't change the |
Nope, it won't change anything in the slider or inbox. (or at least shouldn't...)
Will finish this off tomorrow!
It will render the thread, but then redirect to |
Awesome, great work man! |
|
This will definitely need a lot of testing on alpha before shipping! |
|
Looks like maybe that failing e2e test has something to do with privacy and permissions on private threads? |
|
Ah yeah of course, private threads can't redirect to the proper URL since the data doesn't exist 🤦♂️ Will dig into private thread handling! |
That way the error and null states propagate properly
|
Aha tests still failing because I forgot to forward |
|
Yesss now I think I've actually covered all edge cases! 💯 This should also be ready for testing on alpha |
|
@brianlovin this is ready to ship to |
|
Actually I might be able to make Going to try that, that'd be much nicer than ~ |
Sounds good, I'll hang tight! |
New thread urls look like this: /spectrum/general/the-thread-is-too-small-mnbkzlqp-jkld-utei-tqg4-jliaeti1itud Much better than with the tilde 👍
|
Latest commit makes it work without the tilde! Now thread URLs look like this:
😍😍😍 This is ready for a review, and then we should get it on alpha for some solid testing! |
|
Hm looks like the e2e tests for the thread view are failing. I like where this is going though! |
src/routes.js
Outdated
| // - /id-123-123-123-id => id-123-123-123-id, easy start that works | ||
| // - /some-custom-slug-id-123-123-123-id => id-123-123-123-id, custom slug also works | ||
| // - /-id-123-123-123-id => id-123-123-123-id, empty custom slug also works | ||
| path="/:communitySlug/:channelSlug/(.*?)(\w*-\w*-\w*-\w*-\w*)" |
There was a problem hiding this comment.
Fairly sure this will break for all firebase-imported threads from way back in the day.
|
Shit that's such a good catch @brianlovin, I totally forgot about those! 🤦♂️ Ugh. Been brewing over how to solve that for the last couple days, here are some options I came up with:
What are your thoughts on either of these? |
This sounds pretty risky - I like that your current version doesn't actually care about the slug, since it just uses the uuid for the lookup. Storing a slug and using it for lookups seems more fragile and can become stale if threads are edited.
This seems...easiest? Just slightly less aesthetic.
This sounds very not fun. Backfilling the usersThreads records to use updated uuids sounds like a pita and prone to bugs. Although, technically, this would be the best solution for future-proofing id patterns :/ |
We have 1105 threads that have Firebase-style IDs: db.table('threads').filter(
db.not(db.row('id').match("^\\w{8}-\\w{4}-\\w{4}-\\w{4}-\\w{12}$"))
).count()
// => 1105Actually, what's going to block this is the way we store notifications. Those have the thread IDs hardcoded in the payload string and there isn't much we can do about it 😕 Ugh |
|
Reverted back to |
|
Damn. I mean, we could always just burn notifications to the ground and redo those :P |
|
Clicked to different threads from their timestamps from the slider, got this:
First, that shouldn't work. Second, that shouldn't happen 😅 |
|
@brianlovin this has been tested on alpha and I couldn't find any more bugs, so I'm inclined to say this is ready to ship! 🎉 |
|
Sounds good to me! I'll do a cut today |
|
I found a place this breaks locally, will investigate |
Status
Deploy after merge (delete what needn't be deployed)
Related issues (delete if you don't know of any)
Closes #2770
Implements nicer thread URLs based on the scheme
/community/channel/slug(threadTitle)~threadId. Note that I had to use tilde~as the delimiter between slug and id because our thread id's include hyphens, so we can't use that, and the only other options were dots.or underscores_and both of those look worse imo.TODO
/:community/:channel/:id/:community/:channel/:threadSlug-:threadIdor something like that...from hyperion?the frontend redirect also works on SSR, so we're good hereFix param redirect?this is already handled by the redirect from the/thread/:idroutes to the new ones. Redirecting the param to the new routes automatically would require hyperion to be connected to the db, which it currently isn't, so it's imo not worth it.srcto/thread/xyzwith links to/community/channel/xyz