Fixed unfurling urls #52
Conversation
jjasghar
force-pushed
the
jjasghar/unfurling
branch
from
5153c1c to
2cefcf6
Compare
jjasghar
force-pushed
the
jjasghar/unfurling
branch
from
2cefcf6 to
47f13a8
Compare
Created an optional flag for unfurling urls. It looks like this fixes the original problem, and now gives a option to turn it on or off via an environmental variable. Signed-off-by: JJ Asghar <[email protected]>
jjasghar
force-pushed
the
jjasghar/unfurling
branch
from
47f13a8 to
e3f67b4
Compare
|
Is there a case where you'd want to disable unfurling of links globally for the whole instance? |
| auto_link h(ContentFilters::TextMessagePresentationFilters.apply(message.body.body)), html: { target: "_blank" } | ||
| # Render the full ActionText content to preserve unfurled links and other attachments | ||
| # Use ActionText's built-in rendering | ||
| message.body.to_s.html_safe |
There was a problem hiding this comment.
I'm somewhat concerned about calling .html_safe on user inputs. The TextMessagePresentationFilters did a few things like sanitize the input, and style the unfurled links.
| perform() { | ||
| // Check if unfurling is enabled via environment variable | ||
| if (!this.#isUnfurlingEnabled()) { | ||
| console.log("URL unfurling is disabled via ENABLE_URL_UNFURLING environment variable") |
There was a problem hiding this comment.
This seems a bit noisy, maybe it would be better to just resolve the promise?
| .then(response => { | ||
| console.log(`Unfurl response for ${this.url}:`, response.status, response.statusText) | ||
|
|
||
| if (!response.ok) { | ||
| console.warn(`Failed to fetch OpenGraph metadata for ${this.url}: ${response.status} ${response.statusText}`) | ||
| return null | ||
| } | ||
| return response.json | ||
| }) | ||
| .then(data => { | ||
| if (data) { | ||
| console.log(`Successfully unfurled ${this.url}:`, data) | ||
| this.#insertOpengraphAttachment(data) | ||
| } else { | ||
| console.log(`No data returned for ${this.url}`) | ||
| } | ||
| }) | ||
| .catch(error => { | ||
| console.warn(`Error unfurling URL ${this.url}:`, error) | ||
| }) |
There was a problem hiding this comment.
This looks like the same code just with logging?
Maybe it would be simpler to just log the error in the catch branch?
| #escapeHtml(text) { | ||
| const div = document.createElement('div') | ||
| div.textContent = text | ||
| return div.innerHTML | ||
| } |
There was a problem hiding this comment.
I think we can remove this sanitization completely since the unfurl payload is already sanitizied.
There was a problem hiding this comment.
I don't really understand how this improves the reliability of the fetch process?
Could you walk me through you r thought process here?
| <%= link_to truncate(opengraph_embed.filename, length: 280, omission: "…"), opengraph_embed.href, rel: "noreferrer", target: "_blank" %> | ||
| </div> | ||
| <div class="og-embed__description"><%= truncate(opengraph_embed.caption, length: 560, omission: "…").html_safe %></div> | ||
| <div class="og-embed__description"><%= truncate(opengraph_embed.description, length: 560, omission: "…").html_safe %></div> |
There was a problem hiding this comment.
I think #caption is the correct method to call in this case?
There was a problem hiding this comment.
This shouldn't be part of the PR :)
| config.autoload_paths += %w[ test/test_helpers ] | ||
|
|
||
| # Enable URL unfurling for tests | ||
| ENV["ENABLE_URL_UNFURLING"] = "true" |
There was a problem hiding this comment.
You should test everything unfurling related with this flag on and off
| event.target.addEventListener("trix-paste", this.#didPaste.bind(this)) | ||
| event.target.addEventListener("trix-change", this.#didChange.bind(this)) |
There was a problem hiding this comment.
We prefere is the events are bound from the HTML elements, this makes the controllers more reusable.
| #didChange(event) { | ||
| this.#debouncedCheckForUrls(event) | ||
| } |
There was a problem hiding this comment.
Maybe there is a better name for #didChange that communicates what it does better? E.g. checkForUrls or unfurl?
|
Awesome, I'll answer your suggestions/questions as quickly as I can, thank you for the review! |
2 participants
Created an optional flag for unfurling urls.
It looks like this fixes the original problem, and now gives a option to turn it on or off via an environmental variable.