Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert Lemmy and KBin community and user references into local instance links #1462

Merged
merged 15 commits into from
Jun 22, 2023

Conversation

Zetaphor
Copy link
Contributor

@Zetaphor Zetaphor commented Jun 22, 2023

This PR adds a custom plugin to Markdown-It that will add local instance links to any detected community links.

It supports the following formats:

It will rewrite the markdown to include a link to the community from the local instance. It's not actually modifying the underlying content, only the presentation of the markup at render.

This does not handle the possibility of the local instance not having synced any data from the remote community.

I believe resolving the 404 issue is a larger technical discussion worthy of its own PR. This PR simply adds a custom plugin to the markdown parser.

Once merged this would partly address or fully resolve the following issues:
#1048, #369, #1227, #1229, #1277, #1384, #1297, and lemmy/#2987. There's probably more I didn't find.

Screenshots:

Before:
image

After:
image

@Zetaphor Zetaphor changed the title Convert local and remote community references into links Convert local and remote community references into local instance links Jun 22, 2023
@Zetaphor
Copy link
Contributor Author

Zetaphor commented Jun 22, 2023

Upon reflection I realize the /c/localCommunity syntax without the remote server address isn't going to work if the content is federated. I'll remove that tomorrow as well as look into the 404 issue. It's very late

@cloventt
Copy link
Contributor

Such a gnarly regex, I love it!

In terms of 404 behaviour I think it should be that hitting a 404 triggers an attempt to pull the federated community. But I think this should probably only happen if the user is logged in to the instance.

Maybe the 404 page for communities could be adjusted to say something like, "Uh oh, can't find that community on this instance! Would you like to look for it on the host instance?" and a link to the host instance - generated from the URL of the 404 page. If the community truly does not exist at all, the user could then follow that link to find determine that.

@thegabriele97
Copy link

thegabriele97 commented Jun 22, 2023

This is a comment from @dessalines on #1156:

There aren't any 404s currently, because the post and comment nodes already exist on your own instance, or you wouldn't be seeing them (so neither link will 404).

But with searched content like this, the comment with text referencing a federated community or actor, might not exist everywhere.

the home instance will automatically fetch the content from the remote instance instead of 404'ing?

We don't currently do this, but this is the advantage of having a local link. We could do a resolveObject if it 404s, and cache it to our local instance. No guarantees still, since it could be blocked, but at least its possible.

We've chosen for resolve to only work on the search page currently, and by logged-in users only, but I'm open to a discussion for changing that. Def has pros and cons.

@trashhalo
Copy link

Totally agree that the 404 behavior for Lemmy communities needs work. If the complexities there end up blocking this from merging I'd suggest splitting that out as its own body of work and letting this land as is.

Correctly linking communities removing what is currently a 6 step process on mobile using copy and search is a huge leap forward in user experience.

Thank you for picking this up, going to be life changing!

@Undearius
Copy link

Undearius commented Jun 22, 2023

This may be out of the scope of this PR but I imagine that kbin will be doing something similar down the line with linking their magazines (communities). Their syntax uses /m/ instead of /c/.

It might be worth it to have a simple parse for /m/[email protected] and replace it with /c/[email protected]. If kbin implements this (but in reverse) it will greatly improve interoperability between the two link aggregators.

@Zetaphor
Copy link
Contributor Author

This may be out of the scope of this PR but I imagine that kbin will be doing something similar down the line with linking their magazines (communities). Their syntax uses /m/ instead of /c/.

It might be worth it to have a simple parse for /m/[email protected] and replace it with /c/[email protected]. If kbin implements this (but in reverse) it will greatly improve interoperability between the two link aggregators.

Great minds think alike, I was considering this as I was browsing on mobile in bed last night. I'll add that today as well.

@Undearius
Copy link

Undearius commented Jun 22, 2023

Here's the modified regex that will match with /c/[email protected] and [email protected] but not /c/community
/(!\b[^@\s]+@[^@\s]+\.[^.\s]+\b)|\/c\/([^@\s]+@[^@\s]+\.[^.\s]+\b)/g


Here's the regex that will match with /m/[email protected] only
/\/m\/([^@\s]+@[^@\s]+\.[^.\s]+\b)/g


Or if you want that all in one, this will match everything above that has the instance
/(!\b[^@\s]+@[^@\s]+\.[^.\s]+\b)|(\/[cm]\/[^@\s]+@[^@\s]+\.[^.\s]+\b)/g

Unfortunately, I don't know the language well enough to translate /m/ to /c/ and cannot contribute further

Zetaphor and others added 3 commits June 22, 2023 13:38
@Zetaphor
Copy link
Contributor Author

Zetaphor commented Jun 22, 2023

I've updated this with the following changes:

  • Remove support for community links without a remote server (ex: /c/testing) as that would fail on federated instances
  • Add support for rewriting KBin community urls, ex: /m/[email protected] becomes /c/[email protected]
  • Add support for fully qualified users, ex: /u/[email protected]

@Zetaphor
Copy link
Contributor Author

I'm going to move this out of draft, I believe the issue of handling 404's for unsynced communties is out of scope for this PR. That warrants a larger technical discussion and this is just a plugin for the markdown parser.

@Zetaphor Zetaphor marked this pull request as ready for review June 22, 2023 17:33
@Zetaphor Zetaphor changed the title Convert local and remote community references into local instance links Convert Lemmy and KBin community and user references into local instance links Jun 22, 2023
src/shared/markdown.ts Outdated Show resolved Hide resolved
@SleeplessOne1917
Copy link
Member

I know we don't have unit tests yet, but a function based on a complicated regex like this is something I'm scared to merge without tests.

@Zetaphor
Copy link
Contributor Author

Zetaphor commented Jun 22, 2023

I know we don't have unit tests yet, but a function based on a complicated regex like this is something I'm scared to merge without tests.

In lieu of proper unit tests, I can offer these unit tests from regex101:

https://regex101.com/r/J0L0gS/2

@Zetaphor
Copy link
Contributor Author

This is also pretty easy to test locally in the frontend as it doesn't require modifying content, you can just switch back and forth between the editor and preview

src/shared/markdown.ts Outdated Show resolved Hide resolved
@Zetaphor
Copy link
Contributor Author

@SleeplessOne1917 I've updated this to use the simpler regex suggested by @Undearius

@Zetaphor
Copy link
Contributor Author

This is now just a basic email regex with a prefix requirement

Copy link
Member

@SleeplessOne1917 SleeplessOne1917 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Zetaphor
Copy link
Contributor Author

Made one last push because I accidentally removed classes. Should be all set for merge now

src/shared/config.ts Outdated Show resolved Hide resolved
@Zetaphor
Copy link
Contributor Author

Unless @Undearius has any additional catches (they're clearly superior in regex-fu) this should be all set.

Latest regex101 link: https://regex101.com/r/J0L0gS/5

@Zetaphor
Copy link
Contributor Author

Going to see what I can do about these linter errors

@Zetaphor
Copy link
Contributor Author

Zetaphor commented Jun 22, 2023

I swear I'm done now. Let me know if you want me to squash these commits

@Dakkaron
Copy link

Dakkaron commented Jun 22, 2023

Would it be possible to convert local community references to global links?

So say, the user test@instanceA posts /c/community on instanceB, it generates a link like /c/community@instanceA from it? So basically just take the user's instance host name and append it to the link of any reference that doesn't have an instance in the name?

Thanks a ton, btw, for fixing this!

@Zetaphor
Copy link
Contributor Author

Would it be possible to convert local community references to global links?

So say, the user test@instanceA posts /c/community on instanceB, it generates a link like /c/community@instanceA from it? So basically just take the user's instance host name and append it to the link of any reference that doesn't have an instance in the name?

Thanks a ton, btw, for fixing this!

I'm not sure I fully understand your question, can you please rephrase it with a more detailed example?

I originally had a version that supported the use of the /c/community shorthand, but decided against that because it would break federation with other platforms. This change only modifies the rendering of the markdown when viewed through a Lemmy instance, not the underlying content. If you were to see that comment from a federated KBin or Mastodon instance you would have no way of actually following the link/reference, since it doesn't include the remote server address.

@Undearius
Copy link

Undearius commented Jun 23, 2023

I believe what they're saying is this.

I write a comment from my account on lemmy.ca ([email protected]) and reference /c/plex. You, on your zemmy.cc instance, click the link to the community.

Instead of it expand /c/plex to zemmy.cc/c/plex it would take the domain from my username and link it to zemmy.cc/c/[email protected]

@Zetaphor
Copy link
Contributor Author

Ahh okay, that makes sense.

I'd want to get a second opinion, but that sounds a bit too opaque from a flow perspective. The current format is more of a helper than something that expands a limited amount of text into a fully qualified route. A user would have to know ahead of time that this functionality exists, where the changes in this PR just assist with what is an already established form of addressing.

There's also still the issue of that not working outside of Lemmy. If you were to view that link on KBin, Mastodon, or another ActivityPub compatible interface you would have no way of obviously tying the intended location to the correct address without knowing that this one specific interface quirk exists on Lemmy's web UI.

@Dakkaron
Copy link

Yeah, @Undearius understood me correctly. I figured it could be working like local dialing on the phone. If someone is not dialing a fully-qualified number, the phone system will dial "inside their scope". But I understand your concerns, they make sense.

@M4rtineski
Copy link

I may have missed something but how does this update/merge behave with links to posts and not communities? Will this work with links like that?:
image
image

@Zetaphor
Copy link
Contributor Author

@M4rtineski It won't affect those at all, it's only matching on the patterns I included above.

@M4rtineski
Copy link

M4rtineski commented Jun 27, 2023

@Zetaphor So from my understanding this merge doesn't work for linking posts? Are you aware of any issues for linking posts without making other's leave their instance? And if does work for linking posts can you show me the syntax please?

Edit: ty for response

@Zetaphor
Copy link
Contributor Author

@M4rtineski The format is in the top of the PR, but here it is again:

There's an ongoing discussion at #1048

I would consider making a PR specifically for links that uses an icon similar to what exists for comments 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants