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

feat: Open assistant-generated links in a browser #261

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

dcalhoun
Copy link
Member

Fixes https://github.com/Automattic/dotcom-forge/issues/7772.

Proposed Changes

Enable users to navigate with links included in assistant responses,
opening the URL in the user's default browser.

Testing Instructions

  1. Navigate to the Assistant tab.
  2. Request documentation on a subject; e.g., "How do I install a plugin?"
  3. Verify clicking links within the assistant response open the link destination
    in the default browser.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@dcalhoun dcalhoun added the [Type] Enhancement Improvement upon an existing feature label Jun 17, 2024
@dcalhoun dcalhoun self-assigned this Jun 17, 2024
@dcalhoun
Copy link
Member Author

dcalhoun commented Jun 17, 2024

The proposed changes do not include automated tests as we currently mock react-markdown due to the fact that it is an ESM-only library, for which Jest's support is experimental and unstable. Until we configure Jest for ESM and remove the mock, testing the proposed changes is only possible in the form of asserting props passed to react-markdown and/or unit testing Message and Anchor independently, which does not seem all that valuable.

@dcalhoun dcalhoun marked this pull request as ready for review June 17, 2024 15:37
@dcalhoun dcalhoun requested review from a team June 17, 2024 15:37
@dcalhoun
Copy link
Member Author

My understanding is the failing Windows E2E tests are unrelated to these changes and currently under investigation (p1718622040527389-slack-C06DRMD6VPZ).

@fluiddot
Copy link
Contributor

fluiddot commented Jun 17, 2024

My understanding is the failing Windows E2E tests are unrelated to these changes and currently under investigation (p1718622040527389-slack-C06DRMD6VPZ).

Yes, in fact, we've disabled temporarily E2E tests on Windows until it's solved. Sorry for the inconvenience 🙇 .

Enable users to navigate with links included in assistant responses,
opening the URL in the user's default browser.
@dcalhoun dcalhoun force-pushed the feat/open-assistant-gengerated-links-in-a-browser branch from 63ce507 to 1992253 Compare June 17, 2024 17:35
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

Nice work! I tested it and I confirm the links open the default browser.

I was thinking if we should check that href starts with http to avoid opening other protocols like mailto. But I guess there is no harm in opening other protocols too.

open-links.mp4

@dcalhoun dcalhoun merged commit 845f440 into trunk Jun 17, 2024
10 checks passed
@dcalhoun dcalhoun deleted the feat/open-assistant-gengerated-links-in-a-browser branch June 17, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improvement upon an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants