fix(ui): Link rendering in comment editor

[widal001]

Summary

Issue: Fixes: #1370

1. Uses new npm dependency @tiptap/extension-link to fix link rendering. Previously links would be stripped from the editor in rich text mode. This extension allows users to:
   • Insert links using markdown syntax in markdown mode
   • Paste links directly in rich text mode
   • Preserve links when toggling between modes
2. Adds a LinkInsertModal component and an insert link button to the MenuBar of the CommentEditor
3. Increases the max width of the ShareFeedback component to avoid wrapping MenuBar items on desktop
4. Sanitizes markdown syntax and html characters (e.g. & and ') from auto-generated title

Validation

Here's a loom explaining the current bug and highlighting the new fix. I updated the loom link to include the full demo of all features.

Notes

• Testing: I didn't see an existing file for testing the CommentEditor component, but I might have missed it. I tried creating a new test file but was having a little trouble hooking it into the existing test harness. If you want an accompanying unit test, I can spend bit more time getting it to work!
• Linking the issue: I used the "Fixes" keyword to link this PR to the related issue. If you'd prefer to not have them linked directly I can edit the PR description.
• Link menu button: I added an insert link menu button, but there are a few known limitations:
  • The insert link callback will only insert a link into the highlighted text, even if the highlighted text is part of a longer link. Ideally we'd replace the entire link if the cursor is highlighting or placed on any part of the link text.
  • The insert link modal doesn't get pre-filled with the existing URL -- this was surprisingly difficult to get working correctly along with the behavior described above.

[mattwoberts]

Hey @widal001

Thanks very much, I'm not sure how I missed support for links actually, this is a good start!

You're right, there aren't any decent tests for the CommentEditor, and really there should be. It was a bit nasty trying to work out the best way to test this stuff, since it's so embedded in the UI. I couldn't work out if adding some web (playwright) tests was the best way to go, or if there was a nicer way to isolate it.. If you have any ideas?

Just played with this and it's working really well.. I was wondering if we should also add a button to the editor to make it easy to create a link on the rich text side... that might be a bit tricky though since you're going to need to prompt for a URL to create the link for... so perhaps we shouldn't bother adding that now.

I also spotted when I added a post with a link, that the link was added to the title of the post in markdown format, which obs didn't look great, so we'll need to do something about that too:

I was under the impression that all markdown was being stripped for the title, I guess not!

[widal001]

Thanks for this additional context and feedback @mattwoberts

Based on your notes, here's a proposal for next steps:

1. Title: I'll prioritize sanitizing links from the auto-generated title and get that in this PR.
2. Link button: I'll try to add a new button to insert links via the UI in rich text editor mode, seeing if I can find some example code for a straightforward popover component to enter the URL and link text. If this winds up being a bigger effort though would you be open to me splitting this out into a different PR, so we're not blocking this one? The bug that inspired this fix is a pretty big pain point for us, especially when trying to edit posts that were drafted in markdown with multiple links.
3. Tests: I would love to help out with this, but it might need to be a separate effort, since I need to spend a bit more time understanding the current test harness and investigating better patterns for testing frontend components in isolation. I'm more of a backend developer and data engineer, but I love TDD and would like to learn how to better incorporate that into FE component development.

I'll prioritize 1) today and try to get to 2) as well and comment back on this ticket in the next day or two with an update. Does that sound good to you?

[mattwoberts]

Thanks @widal001 that sounds like a perfect plan!

[widal001]

@mattwoberts See the last three commits for updates!

Here's a quick summary below as well:

• Title: I was able to fix this issue by updating and repurposing a plainText() function that didn't seem to be used elsewhere in the codebase. I added a test for stripping markdown links and converting HTML-encoded characters back to plain text.
• Link button: I added a first version of an insert link button and modal that also has keyboard shortcut support. There are a few known limitations (mostly around pre-filling and replacing existing links) that were getting increasingly complicated to handle correctly. So I decided to descope some of that behavior from this first version. I've noted them in the PR description.

Next steps: I might suggest tackling some of those link modal limitations as part of a PR that adds more robust testing. That PR might also benefit from some refactoring of the CommentEditor component, since that file has gotten pretty large and a little hard to follow with all of the react code for the different menu buttons. At a minimum splitting out the logic for MenuBar into its own component might make it easier to test. I probably can't get to that this week, but I can try to take a look into it next week.

[mattwoberts]

Just something I remembered...

Someone recently added support for crypto-style links, since they were being removed as part of the parsing... We'll possibly need to make sure all that continues to work?

#1332

[widal001]

@mattwoberts It's been a busy end to the week. I'll try to incorporate these remaining updates this weekend:

• Better code comment explaining why we need to use plainText(editor.getText()) 21649cb
• Update LinkInsertModal to use the Input component with localization support. This might also remove the need for the jury-rigged error handling I had to do in the form. 218e1e5
• Double check how we're handling crypto-style links. I think the main adjustment we'll need is in the validation of the URL before its inserted. 6ebcdfa

[widal001]

Just something I remembered...

Someone recently added support for crypto-style links, since they were being removed as part of the parsing... We'll possibly need to make sure all that continues to work?

#1332

@mattwoberts This commit should add support for crypto links and any other schemes added to the "Accepted schemes" configuration section by the Fider admin (e.g. mailto: or tel:) 6ebcdfa

[mattwoberts]

Thanks @widal001, I'll get onto this asap, I'm not very well at the moment so not running on full speed....

[widal001]

Thanks @widal001, I'll get onto this asap, I'm not very well at the moment so not running on full speed....

Oh no! Hope you're feeling better soon!