Repository navigation
Conversation
Contributor
Author
|
Would love thoughts on this @jorgemanrubia @zoltanhosszu if you have time for a quick glace 🙏 |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approved
The implementation is consistent across selection, commands, serialization, sanitization, UI state, and automated coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Adds image attachment linking while preserving existing text-link behavior and Action Text compatibility.
Changes:
- Persists attachment
hrefthrough HTML, JSON, sanitization, and reloads. - Supports linking, relinking, unlinking, and toolbar state for selected images.
- Adds browser and Rails round-trip coverage.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
src/nodes/action_text_attachment_node.js |
Imports and exports attachment links. |
src/extensions/attachments_extension.js |
Allows sanitized attachment href. |
src/editor/command_dispatcher.js |
Handles image link and unlink commands. |
src/editor/selection.js |
Exposes attachment link state and URL. |
src/elements/toolbar.js |
Reflects linked node selections. |
src/elements/dropdown/link.js |
Prefills attachment link URLs. |
test/browser/tests/attachments/link_attachment.test.js |
Covers attachment-link interactions. |
test/system/attachments/linked_attachment_round_trip_test.rb |
Verifies Action Text persistence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1271.
Linking a selected image did nothing:
dispatchLinkbailed on anything but a range selection. Attachments now carryhref, read on import, written on export and in JSON, andnullwhen absent so it is omitted. The link command sets it on the selected image attachments and unlink clears it; other attachment types are ignored. Text links are unchanged, that path has only moved into$linkSelectedText.Selection#linkUrlanswers for both an attachment and the nearestLinkNode, so the dropdown prefills andisInLinklights the toolbar button either way.hrefjoins the attachment sanitiser allowlist.Rendering the anchor is the host's job, as it is for
caption:<action-text-attachment href>is what Trix wrote and whatActionText::Attachment::ATTRIBUTESalready preserves.Test
link_attachment.test.js: link, relink, unlink, dropdown prefill, button state, reload, refusing a PDF, and linking an image inside a gallery.linked_attachment_round_trip_test.rbtakes an upload through link, save, render and re-edit and checkshrefsurvives Loofah both ways.Full Playwright suite passes in Chromium. The tables Alt+F10 focus tests fail on
mainfor me too.Not addressed here