feat: check for intent in URL detection and normalization paste handler#3710
feat: check for intent in URL detection and normalization paste handler#3710palmer-cl wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74b39b559b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
||
| const withProtocol = maybeAddProtocol(trimmed); | ||
| const allowedProtocols = buildAllowedProtocols(protocols); | ||
| if (!hasExplicitUrlIntent(trimmed)) return null; |
There was a problem hiding this comment.
I noticed that if the app is configured with non-default protocols (for example protocols: ['ftp'], the autolinking feature wouldn't work in that case, since hasExplicitUrlIntent hardcodes http/https/mailto/tel/sms. What do you think?
There was a problem hiding this comment.
this is intentional. if ftp doesnt work in app or browser, autolinking it is a worse UX. Happy to change this though if its something we do want to support?
Currently I have a test making sure it does not work from paste, but existing links will be preserved.
I could go either way on this.
|
Hey @palmer-ci! I left a small comment about supporting non-default protocols |
|
@luccas-harbour let me know your desired direction on this. |
This pull request improves the detection logic for automatically converting pasted text into links, making it more precise. The main focus is to ensure that only text with an explicit URL intent (such as starting with
www.,http(s)://,mailto:,tel:, orsms:) is auto-linked, and to prevent plain text or relative paths from being mistakenly treated as URLs. Tests have been updated to reflect these changes and to prevent protocol configuration from expanding what is auto-linked.