Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ai): invoke OpenerService for markdown links in chat UI #14602

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

planger
Copy link
Contributor

@planger planger commented Dec 9, 2024

What it does

Invoke Theia's OpenerService when users click a link in the chat UI. This enables registering custom implementations of OpenHandler for specific URIs used as href in markdown links.

Fixes #14601

How to test

  1. Let an LLM output a markdown link for which an OpenHandler is registered, e.g. [test](command:sample-command)
  2. Click the rendered link in the chat UI
  3. Observe that the sample-command is executed, as it is correctly handled by the CommandOpenHandler

Follow-ups

None

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Contributed on behalf of STMicroelectronics.

Review checklist

Reminder for reviewers

@planger planger requested a review from sdirix December 9, 2024 19:15
@planger planger force-pushed the planger/handle-links-with-opener-service branch 2 times, most recently from 4cecf16 to bede94c Compare December 10, 2024 07:44
@planger
Copy link
Contributor Author

planger commented Dec 10, 2024

@sdirix Sorry for the force-push after requesting your review... Just saw another potential to simplify things. Now it is ready, I promise :-)

@sdirix
Copy link
Member

sdirix commented Dec 10, 2024

@sdirix Sorry for the force-push after requesting your review... Just saw another potential to simplify things. Now it is ready, I promise :-)

No worries, I'm also guilty of this from time to time ;)

Comment on lines 100 to 112
const handleClick = (event: MouseEvent) => {
if (event.target instanceof HTMLElement && event.target.tagName === 'A') {
const href = event.target.getAttribute('href');
if (href) {
open(openerService, new URI(href));
event.preventDefault();
}
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const handleClick = (event: MouseEvent) => {
if (event.target instanceof HTMLElement && event.target.tagName === 'A') {
const href = event.target.getAttribute('href');
if (href) {
open(openerService, new URI(href));
event.preventDefault();
}
}
};
const handleClick = (event: MouseEvent) => {
let target = event.target as HTMLElement;
while (target && target.tagName !== 'A') {
target = target.parentElement as HTMLElement;
}
if (target && target.tagName === 'A') {
const href = target.getAttribute('href');
if (href) {
open(openerService, new URI(href));
event.preventDefault();
}
}
};

Should we support also links that have children? This could be useful for cases like <a><code>abc</code></a>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to already work. When I instruct the LLM with

Repeat exactly: [`test`](command:sample-command)

It then produces

<a href="command:sample-command"><code>test</code></a>

which is rendered as a clickable link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but the opener handler isn't invoked, because the event.target is the code element.
So I think we should recursively move up to see if we are inside an A element.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! I overlooked that the command was not working properly as in both cases a similar styled popup was shown, the Javascript alert and the xdg-open alert 😅

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me!

Fixes #14601

Contributed on behalf of STMicroelectronics.
@planger planger force-pushed the planger/handle-links-with-opener-service branch from bede94c to 0e6cbe4 Compare December 12, 2024 19:20
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works even better now

@planger planger merged commit 5c7ffaa into master Dec 13, 2024
11 checks passed
@planger planger deleted the planger/handle-links-with-opener-service branch December 13, 2024 09:28
@github-actions github-actions bot added this to the 1.57.0 milestone Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Links for custom OpenHandler in markdown aren't supported in the AI Chat
2 participants