fix: Open external links in a new tab #120

gadenbuie · 2025-09-05T13:17:13Z

Fixes #11

pkgload::load_all("pkg-r")

output_markdown_stream(
  "foo",
  content = "Here's a link to [wikipedia](https://wikipedia.org)."
)

output_markdown_stream(
  "foo",
  content = "Here's a link to [wikipedia](https://wikipedia.org).",
  content_type = "semi-markdown"
)

cpsievert · 2025-09-05T14:23:13Z

Have you tested that this works? I can confirm the rel="noopener noreferrer" is getting added, but it appears the target="_blank" gets stripped out, probably by DOMpurify cure53/DOMPurify#317

gadenbuie · 2025-09-05T14:28:29Z

Have you tested that this works? I can confirm the rel="noopener noreferrer" is getting added, but it appears the target="_blank" gets stripped out, probably by DOMpurify cure53/DOMPurify#317

Oh shoot, yeah I saw the rel attribute was added and thought it was both. I'll fix the sanitization too

Now a two step process: * Markdown renderer adds `data-external-link` to external links * DOMPurify adds `target="_blank" rel="noopener noreferrer"` to links with that attribute Otherwise sanitization strips out the target attribute.

gadenbuie · 2025-09-05T14:41:29Z

@cpsievert okay, now it works! 😃

js/src/utils/_utils.ts

Makes it easier to stop propagation if you want to prevent the dialog from showing

cpsievert · 2025-09-05T20:31:38Z

js/src/chat/chat.ts

+          window.open(linkEl.href, "_blank", "noopener,noreferrer")
+        }
+      })
+      .catch(() => {
+        // If dialog fails for any reason, fall back to opening the link directly
+        window.open(linkEl.href, "_blank", "noopener,noreferrer")


If we're opening links this way, seems we could drop the afterSanitizeAttributes hook?

Or do we want to keep it for non-chat situations?

I remember deleting them, but maybe they crept back in?

cpsievert · 2025-09-05T20:38:17Z

js/src/chat/chat-external-link.ts

+    })
+  }
+
+  private createDialog(): void {


I'd prefer this broken down into 2 methods: createDialog(): HTMLElement and attachDialogListeners(x: HTMLElement): void. Then do all state mutation at the top-level in connectedCallback()

cpsievert · 2025-09-05T20:40:47Z

js/src/chat/chat-external-link.ts

+  // Check if the browser supports HTMLDialogElement
+  if (typeof window.HTMLDialogElement !== "undefined") {


Hmm, I'm not sure if this is needed? It'd be a surprise to me if Bootstrap depended on <dialog>, and also the browser support is pretty good https://caniuse.com/?search=HTMLDialog

cpsievert · 2025-09-05T20:43:29Z

js/src/chat/chat-external-link.ts

+  }
+}
+
+export class ChatExternalLinkDialog extends LightElement {


This could be used in other non-chat scenarios, right? I don't want to make this a requirement, but perhaps worth noting that we could abstract this and re-use if/when we decide to do tool approval?

Suggested change

export class ChatExternalLinkDialog extends LightElement {

export class ExternalLinkDialog extends LightElement {

Yeah that's a good idea! But we own all the code here so we can just rename this when we get to that

cpsievert · 2025-09-05T20:46:45Z

js/src/chat/chat.ts

@@ -380,6 +382,7 @@ class ChatContainer extends LightElement {
    )

    this.inputSentinelObserver.observe(sentinel)
+    this._boundOnExternalLinkClick = this.#onExternalLinkClick.bind(this)


I'm a bit surprised the .bind() is necessary?

Yeah, we need it because we and the event listener to the window and not the chat container element

gadenbuie force-pushed the fix/11-links-in-chat branch from cca8f4a to a227602 Compare September 5, 2025 13:24

gadenbuie added 2 commits September 5, 2025 09:27

feat(js): Open external links in a new tab

11f5cc0

chore: build and update assets

0b3e7b0

gadenbuie force-pushed the fix/11-links-in-chat branch from a227602 to 0b3e7b0 Compare September 5, 2025 13:28

gadenbuie requested a review from cpsievert September 5, 2025 13:28

gadenbuie added 2 commits September 5, 2025 10:40

fix(js): Add external link attributes

71bb826

Now a two step process: * Markdown renderer adds `data-external-link` to external links * DOMPurify adds `target="_blank" rel="noopener noreferrer"` to links with that attribute Otherwise sanitization strips out the target attribute.

chore: build and update assets

86e7605

cpsievert reviewed Sep 5, 2025

View reviewed changes

js/src/utils/_utils.ts Outdated Show resolved Hide resolved

gadenbuie added 3 commits September 5, 2025 11:51

refactor: Handle external links in DOMPurify layer

b810626

chore: build and update assets

486f4ee

Merge 'origin/main'

7a6d431

gadenbuie mentioned this pull request Sep 5, 2025

Ability to register client-side link onclick handlers #123

Open

Merge 'origin/main'

557d9be

cpsievert approved these changes Sep 5, 2025

View reviewed changes

gadenbuie added 11 commits September 5, 2025 12:57

feat: Add confirmation dialog for external links

33f29a9

chore: build and update assets

cfd5f52

feat: Move external link handler to window

7f7677a

Makes it easier to stop propagation if you want to prevent the dialog from showing

chore: build and update assets

ca4b2ec

fix: bind external link click to correct element

a1c27e1

chore: build and update assets

323aa91

feat: always open external links option

4a5e4d0

chore: build and update assets

94d5cab

fix: always is same as proceed

f4715c9

chore: build and update assets

b370d02

docs: Add changelog items

8383b15

cpsievert reviewed Sep 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Open external links in a new tab #120

fix: Open external links in a new tab #120

gadenbuie commented Sep 5, 2025 •

edited

Loading

Uh oh!

cpsievert commented Sep 5, 2025

Uh oh!

gadenbuie commented Sep 5, 2025

Uh oh!

gadenbuie commented Sep 5, 2025

Uh oh!

Uh oh!

cpsievert Sep 5, 2025

Uh oh!

cpsievert Sep 5, 2025

Uh oh!

gadenbuie Sep 5, 2025

Uh oh!

cpsievert Sep 5, 2025

Uh oh!

cpsievert Sep 5, 2025

Uh oh!

cpsievert Sep 5, 2025 •

edited

Loading

Uh oh!

gadenbuie Sep 5, 2025

Uh oh!

cpsievert Sep 5, 2025

Uh oh!

gadenbuie Sep 5, 2025

Uh oh!

Uh oh!

		// Check if the browser supports HTMLDialogElement
		if (typeof window.HTMLDialogElement !== "undefined") {

	export class ChatExternalLinkDialog extends LightElement {
	export class ExternalLinkDialog extends LightElement {

fix: Open external links in a new tab #120

Are you sure you want to change the base?

fix: Open external links in a new tab #120

Conversation

gadenbuie commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpsievert commented Sep 5, 2025

Uh oh!

gadenbuie commented Sep 5, 2025

Uh oh!

gadenbuie commented Sep 5, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpsievert Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gadenbuie commented Sep 5, 2025 •

edited

Loading

cpsievert Sep 5, 2025 •

edited

Loading