feat: Creating url with selected text in editor#2148
feat: Creating url with selected text in editor#2148killianmathias wants to merge 2 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables creating a link using the currently selected text in the rich HTML editor (prefilling the “Add link” alert based on whether the selection looks like a URL or a title).
Changes:
- Switch
swift-rich-html-editordependency to a feature branch that exposes selection data. - Thread selected text state from
ComposeMessageViewinto the editor and mobile formatting toolbar. - Update link alert presentation to an item-based flow and prefill
AddLinkViewfrom selection.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Tuist/Package.swift | Pins swift-rich-html-editor to a feature branch to access selection support. |
| Tuist/Package.resolved | Updates resolved state to the branch + new revision. |
| Mail/Views/New Message/ComposeMessageView.swift | Adds selectedText state and passes it into body + mobile toolbar. |
| Mail/Views/New Message/ComposeMessageBodyView.swift | Passes selection into RichHTMLEditor and changes link alert presentation. |
| Mail/Views/New Message/ComposeEditor/Mobile/MobileFormattingToolbarView.swift | Detects whether selected text is a URL/title and opens a prefilled link modal. |
| Mail/Views/New Message/ComposeEditor/Mobile/EditorMobileToolbarView.swift | Plumbs selectedText binding down to the formatting toolbar. |
| Mail/Views/Alerts/AddLinkView.swift | Accepts a SelectionLink to prefill title/url fields when adding a link. |
| .onChange(of: selectedText) { newValue in | ||
| print("newValue: \(newValue)") | ||
| } |
There was a problem hiding this comment.
print("newValue: ...") will log every selection change in production builds (and can be very noisy). Please remove this debug logging or replace it with Logger behind a debug-only compilation flag if it’s still needed for troubleshooting.
| .onChange(of: selectedText) { newValue in | |
| print("newValue: \(newValue)") | |
| } |
| .onEditorLoaded(perform: editorDidLoad) | ||
| .editorCSS(Self.customCSS) | ||
| .introspectEditor(perform: setupEditor) | ||
| .onJavaScriptFunctionFail(perform: reportJavaScriptError) |
There was a problem hiding this comment.
On macCatalyst (and macOS if applicable), the desktop toolbar toggles isShowingLinkAlert, but the view no longer presents an alert bound to that state (it now presents isShowingLink). This makes the “insert link” action from EditorDesktopToolbarView stop working. Either restore a .mailCustomAlert(isPresented: $isShowingLinkAlert) for the desktop flow, or update the desktop action to set isShowingLink (e.g., .empty) and drive the same item-based alert.
| .onJavaScriptFunctionFail(perform: reportJavaScriptError) | |
| .onJavaScriptFunctionFail(perform: reportJavaScriptError) | |
| .mailCustomAlert(isPresented: $isShowingLinkAlert) { | |
| AddLinkView(selectionLink: .empty, actionHandler: didCreateLink) | |
| } |
| } | ||
|
|
||
| struct MobileFormattingToolbarView: View { | ||
| @ModalState(context: ContextKeys.compose) private var isShowingLinkAlert = false |
There was a problem hiding this comment.
isShowingLinkAlert is declared but no longer used in this view (the link UI is now driven by isShowingLink). Please remove this unused state to avoid confusion and keep the modal state surface minimal.
| @ModalState(context: ContextKeys.compose) private var isShowingLinkAlert = false |
| .mailCustomAlert(item: $isShowingLink) { link in | ||
| AddLinkView(selectionLink: link, actionHandler: textAttributes.addLink) | ||
| } |
There was a problem hiding this comment.
The .mailCustomAlert(item: $isShowingLink) modifier is currently applied inside the ForEach for every toolbar button, which can lead to multiple identical alert presenters in the view tree. Prefer attaching the alert once to a stable parent container (e.g., the outer HStack/ScrollView) and keep the buttons responsible only for setting isShowingLink.
| if Regex(pattern: MobileFormattingToolbarView.urlRegex)?.numberOfMatches(in: selectedText) == 1 { | ||
| if let url = URL(string: selectedText) { | ||
| isShowingLink = .url(url) | ||
| } | ||
| } else { | ||
| isShowingLink = .title(selectedText) | ||
| } |
There was a problem hiding this comment.
If selectedText matches the URL regex but doesn’t include a scheme (e.g. www.example.com), URL(string:) returns nil and isShowingLink is never set—so tapping the link button appears to do nothing. Consider either normalizing the selection before building the URL (prepend https:// when needed, similar to AddLinkView.didTapPrimaryButton) or falling back to .title(selectedText) / .empty when URL parsing fails so the alert still opens.
| await attachmentsManager.processHTMLAttachments(htmlAttachments, draftContentManager: | ||
| draftContentManager) |
There was a problem hiding this comment.
This argument wrap introduces an awkward line break (draftContentManager: on one line and the value on the next), which hurts readability and looks like a formatting mistake. Please keep the label/value together (or wrap using standard Swift formatting) so future diffs stay clean.
| await attachmentsManager.processHTMLAttachments(htmlAttachments, draftContentManager: | |
| draftContentManager) | |
| await attachmentsManager.processHTMLAttachments( | |
| htmlAttachments, | |
| draftContentManager: draftContentManager | |
| ) |
| enum SelectionLink: Identifiable { | ||
| case url(URL) | ||
| case title(String) | ||
| case empty | ||
|
|
||
| var id: String { | ||
| switch self { | ||
| case .url(let url): | ||
| return url.absoluteString | ||
| case .title(let string): | ||
| return string | ||
| case .empty: | ||
| return "empty" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
SelectionLink is used by multiple views (AddLinkView, ComposeMessageBodyView, and this toolbar), but it’s declared inside MobileFormattingToolbarView.swift (under a Mobile-specific folder). Consider moving it to a shared location (e.g., alongside AddLinkView or in a small SelectionLink.swift model file) to avoid surprising cross-file dependencies and make reuse clearer.
| .package(url: "https://github.com/Infomaniak/swift-concurrency", .upToNextMajor(from: "1.0.0")), | ||
| .package(url: "https://github.com/Infomaniak/swift-modal-presentation", .upToNextMajor(from: "1.1.0")), | ||
| .package(url: "https://github.com/Infomaniak/swift-rich-html-editor", .upToNextMajor(from: "3.2.0")), | ||
| .package(url: "https://github.com/Infomaniak/swift-rich-html-editor", branch: "feat/get-selection"), |
There was a problem hiding this comment.
The dependency on swift-rich-html-editor is now tracking a branch (feat/get-selection). Branch-based dependencies make builds non-reproducible and can change without a corresponding update in this repo; prefer pinning to a tagged version or at least a specific revision (e.g., .revision(...)) and updating back to a released version once the feature is merged upstream.
| .package(url: "https://github.com/Infomaniak/swift-rich-html-editor", branch: "feat/get-selection"), | |
| .package( | |
| url: "https://github.com/Infomaniak/swift-rich-html-editor", | |
| revision: "REPLACE_WITH_THE_APPROVED_COMMIT_SHA_FROM_FEAT_GET_SELECTION" | |
| ), |
|
No description provided.