Terminal animations break link detection in integrated terminal

[Adjective-Object]

Description

When parts of the terminal are updated, link detection breaks momentarily. For projects with an in-terminal spinner, for example, this breaks all link detection for areas of the terminal near the spinners.

Reproduction Video:
https://github.com/microsoft/vscode/assets/1174858/12b53246-9922-4d67-88d6-f15188a24a1a

In the video I'm using powertoys to highlight mouse clicks.
Note that both xterm links and link text are only intermittently clickable on updated lines, or lines in-between updated lines

Apologies for not submitting through the integrated help menu, it wasn't able to submit.

Reproduction Steps

Run the following bash snippet in an integrated bash terminal.

for i in 1 2 3 4 5; do printf '      \e]8;;http://example.com\e\\example link\e]8;;\e\\    http://example.com/\r\n'; done;x=1; while true; do echo -en '\033[1A'$x'\033[1A'$x'\033[2A'$x'\033[4B\033[20D'; sleep 0.032; x=$(((x+1) %10)); done

This snippet:

1. Starts by printing 5 base lines, each of which has an xterm-format link and raw text of an http link.
2. repeatedly prints the characters 1..9 on the 5th, 4th, and 2nd line of the previous printout by moving the cursor up and printing.

This breaks link detection on the text on lines 2-5. Ctrl+Clicking those links will sometimes work, but most often will fail.
I believe this is because the link detection is run asynchronously, and is racing with the frame-rate of the animation. If you increase the interval of the sleep in the loop, the link detection mostly works again, unless you happen to click right after an animation frame

Curiously, links are broken on line 3, despite no text being printed on that line in the loop. So apart from not updating the text, there's not a clean way to

I've tested this against Windows Terminal (1.19.10821.0) and it does not have the same issues.

System Information

CPUs	11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz (8 x 2995)
GPU Status	2d_canvas: enabled canvas_oop_rasterization: enabled_on direct_rendering_display_compositor: disabled_off_ok gpu_compositing: enabled multiple_raster_threads: enabled_on opengl: enabled_on rasterization: enabled raw_draw: disabled_off_ok skia_graphite: disabled_off video_decode: enabled video_encode: enabled vulkan: disabled_off webgl: enabled webgl2: enabled webgpu: enabled
Load (avg)
Memory (System)	15.85GB (1.34GB free)
Process Argv	--crash-reporter-id 3c1cf59f-7aee-4956-9491-0667a0ce233d
Screen Reader	no
VM	0%

Extensions

Extensions: none

A/B Experiment Info

vsliv368cf:30146710
vspor879:30202332
vspor708:30202333
vspor363:30204092
vscod805cf:30301675
binariesv615:30325510
vsaa593cf:30376535
py29gd2263:31024239
c4g48928:30535728
azure-dev_surveyone:30548225
962ge761:30959799
pythongtdpath:30769146
welcomedialog:30910333
pythonidxpt:30866567
pythonnoceb:30805159
asynctok:30898717
pythontestfixt:30902429
pythonregdiag2:30936856
pyreplss1:30897532
pythonmypyd1:30879173
pythoncet0:30885854
h48ei257:31000450
pythontbext0:30879054
accentitlementsc:30995553
dsvsc016:30899300
dsvsc017:30899301
dsvsc018:30899302
cppperfnew:31000557
d34g3935:30971562
fegfb526:30981948
bg6jg535:30979843
ccp2r3:30993541
dsvsc020:30976470
pythonait:31006305
gee8j676:31009558
dsvsc021:30996838
01bff139:31013167
pythoncenvpt:31022790

[Adjective-Object]

After some (non expert) poking around, it looks like terminal links are evaluated by some ITerminalLinkDetector, which I think is provided by extensions.

Link detection is run on a per-line basis.

vscode/src/vs/workbench/contrib/terminalContrib/links/browser/terminalUriLinkDetector.ts

Lines 41 to 42 in 56ad631

	async detect(lines: IBufferLine[], startLine: number, endLine: number): Promise<ITerminalSimpleLink[]> {
	const links: ITerminalSimpleLink[] = [];

These are connected to xterm via this adaptor

vscode/src/vs/workbench/contrib/terminalContrib/links/browser/terminalLinkDetectorAdapter.ts

Line 30 in 56ad631

export class TerminalLinkDetectorAdapter extends Disposable implements ILinkProvider {

Tracing into xterm, it looks like links are triggered here, in _askForLink
https://github.com/xtermjs/xterm.js/blob/2edc65baaa50f8b99c67f3df6f8d5d93c70a69e6/src/browser/Linkifier.ts#L134
Which is triggered in this onRenderedViewportChange callback here
https://github.com/xtermjs/xterm.js/blob/2edc65baaa50f8b99c67f3df6f8d5d93c70a69e6/src/browser/Linkifier.ts#L299-L321

Internally, it looks like the start/end here are row indexes in the viewport (buffer)? so that explains the behaviour of why line 3 in the example has its links disposed, since it 's within the line start/end range.
https://github.com/xtermjs/xterm.js/blob/2edc65baaa50f8b99c67f3df6f8d5d93c70a69e6/src/browser/services/RenderService.ts#L168

It seems like sparse updates that re-render only the changed rows isn't tenable; I kept digging around xterm and row re-renders being tied to start/end ranges seems like one of the core assumptions of xterm.

However, each of the built-in linkifiers essentially starts by grabbing the queried line's text:
https://github.com/xtermjs/xterm.js/blob/2edc65baaa50f8b99c67f3df6f8d5d93c70a69e6/src/browser/OscLinkProvider.ts#L20

I think there's way inside xterm.js to work around this issue by caching links against the text they are underneath in Linkifier:

• LinkProviders can define some new trait to opt-into caching, something like cacheLinksAgainstText: bool.
• When a link is resolved from a cacheLinksAgainstBuffer provider, the Linkifier computes a hash of the cell attributes + text within the BufferRange defined by the link.
• in onRenderedViewportChange, instead of immediately disposing all links, it could:
  • compare the link's cached text against the current content of the buffer in the BufferRange provided by the link
    • if equal, reuse the old link
    • if not equal, or the provider has not opted into cacheLinksAgainstBuffer, clear the link for that provider and re-query the link provider.

Though, as expressed above there's a race condition if the buffer is updated while the provider is calculating the link.
Maybe this is better expressed by adding an optional checkCache(buffer, BufferRange) callback on the ILink interface so that the link provider can grab the relevant buffer contents to use for caching before it starts any asynchronous actions.

[Adjective-Object]

@Tyriar - didn't realize you worked at MS too!

Right now I'm just poking around the xterm repo trying to figure out what a solution would look like, as this is causing some pretty awkward UX for Outlook devs at the moment. Our devloop has a logger & a few progress bars sharing the same terminal with some clickable links, and any logging or animations we do break the links.

I'm happy to make the changes to get things fixed, but would love to run an approach by you first before blindly jumping in and proposing changes that are architecturally unsound 😅.

[Tyriar]

@Adjective-Object good investigation 🙂.

This is a known problem and so far there hasn't been that much incentive to fix it. My instinct to fix this would be more aggressive caching similar to your suggestion. In particular we can leverage the fact that if we know the link under the cursor did not change, we don't necessarily need to re-fetch links. So if we tracked the chars of each cell in the line when linkify happens, onRenderedViewportChange could invalidate a portion of that to determine whether we need to re-eval.

Feel free to ping me on Teams or in a draft PR if you need more guidance but that's my thinking. It gets a little hairy in the low level link code so we'll want to be careful about not introducing too much complexity with the caching.