mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-08 16:03:27 +00:00
feat: show the new-tab icon on a chat path pill while the modifier is held (#10976)
* feat: show the new-tab icon on a chat path pill while the modifier is held Fixes WIN-2477 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LAa9nNcYxDN3qAZPrYg4Lq * fix: read the new-tab modifier in the capture phase Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LAa9nNcYxDN3qAZPrYg4Lq * refactor: track the new-tab modifier only while a pill is hovered The window key listeners were installed at import time and never removed, so every page that loaded the module paid for them whether or not a pill existed. They now attach on mouseenter and detach on mouseleave or destroy, which is the only window in which the answer is read. Seeding the flag from the hover event also removes the limitation the previous version documented: a mouse event carries the same modifier flags as a key event, so a modifier held before the pointer arrived, or while this window was unfocused, now reads correctly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LAa9nNcYxDN3qAZPrYg4Lq * refactor: export the new-tab modifier as a read-only view `newTabModifier` handed every consumer a writable handle on module-global state, so any of them could drive the icon of every pill on the page. The getter form is what frontend/AGENTS.md prescribes for shared reactive state. Tearing each attachment down in the test's afterEach as well: the module state and its window listeners outlive the DOM, so emptying the body left `held` and the hovered node set for the following case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LAa9nNcYxDN3qAZPrYg4Lq * refactor: only track the modifier for pills whose icon can change The attachment went on every path pill, so hovering a drawer or plain-link pill installed three window listeners for a flag its icon never reads. Only a preview pill can flip, so only it gets them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LAa9nNcYxDN3qAZPrYg4Lq * fix: re-read the new-tab modifier from pointer movement A modifier held across a keyboard app switch was cleared by the blur and never restored: the key was down the whole time so no keydown arrived on the way back, and the pointer parked on the pill fired no fresh mouseenter either. The pill then showed the panel icon while the click would have opened a tab. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LAa9nNcYxDN3qAZPrYg4Lq * refactor: give each pill its own modifier state The shared module state forced a node-identity guard: one hovered element owned the window listeners, so a pill destroyed elsewhere in the transcript had to be stopped from tearing them down. A factory per pill removes the guard, its test case, and the whole class of cross-instance interference, and narrows re-renders to the hovered pill instead of every preview pill on screen. Listener teardown now goes through AbortController signals, so leaving a pill drops the whole set at once rather than through a remove list that has to mirror every option exactly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LAa9nNcYxDN3qAZPrYg4Lq * fix: abort the previous hover controller on re-entry A second mouseenter with no mouseleave between replaced the controller without aborting it, so the four listeners registered under the first signal outlived even the element's destruction: neither leave nor the destroy path held a reference to reach them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LAa9nNcYxDN3qAZPrYg4Lq --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
AlexRV12
co-authored by
Claude Opus 5
Claude Opus 5
parent
7643e9bd77
commit
5da4ea43fb
@@ -0,0 +1,106 @@
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import { newTabModifier } from './newTabModifier.svelte'
|
||||
|
||||
const onPlatform = (userAgent: string) => vi.stubGlobal('navigator', { userAgent })
|
||||
const LINUX = 'Mozilla/5.0 (X11; Linux x86_64)'
|
||||
const MAC = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)'
|
||||
|
||||
const attached: (() => void)[] = []
|
||||
|
||||
/** Attach to a fresh element and return it with its cleanup, as `{@attach}` would. */
|
||||
function pill() {
|
||||
const node = document.createElement('span')
|
||||
document.body.append(node)
|
||||
const modifier = newTabModifier()
|
||||
const cleanup = modifier.attach(node) as () => void
|
||||
attached.push(cleanup)
|
||||
const hover = (init: MouseEventInit = {}) =>
|
||||
node.dispatchEvent(new MouseEvent('mouseenter', init))
|
||||
const move = (init: MouseEventInit = {}) => node.dispatchEvent(new MouseEvent('mousemove', init))
|
||||
const unhover = () => node.dispatchEvent(new MouseEvent('mouseleave'))
|
||||
return { modifier, hover, move, unhover, cleanup }
|
||||
}
|
||||
|
||||
const keydown = (init: KeyboardEventInit) =>
|
||||
window.dispatchEvent(new KeyboardEvent('keydown', init))
|
||||
|
||||
describe('newTabModifier', () => {
|
||||
// The window listeners outlive the DOM, so every case has to be torn down through the
|
||||
// attachment rather than by emptying the body.
|
||||
afterEach(() => {
|
||||
attached.splice(0).forEach((cleanup) => cleanup())
|
||||
document.body.replaceChildren()
|
||||
vi.unstubAllGlobals()
|
||||
})
|
||||
|
||||
// The hover event carries the live modifier state, so a modifier pressed before the pointer
|
||||
// arrived (or while this window was unfocused) is picked up rather than read as false.
|
||||
it('seeds from the hover event, per platform', () => {
|
||||
onPlatform(LINUX)
|
||||
const linux = pill()
|
||||
linux.hover({ ctrlKey: true })
|
||||
expect(linux.modifier.held).toBe(true)
|
||||
|
||||
onPlatform(MAC)
|
||||
const mac = pill()
|
||||
// macOS ctrl+click is a secondary click, so it must not read as a new-tab modifier.
|
||||
mac.hover({ ctrlKey: true })
|
||||
expect(mac.modifier.held).toBe(false)
|
||||
mac.hover({ metaKey: true })
|
||||
expect(mac.modifier.held).toBe(true)
|
||||
})
|
||||
|
||||
// Editors and menus stop keydown propagation to keep their own shortcuts, so a bubble-phase
|
||||
// listener would go blind whenever focus sits in one.
|
||||
it('sees a keydown that a focused element stops from propagating', () => {
|
||||
onPlatform(LINUX)
|
||||
const { modifier, hover } = pill()
|
||||
hover()
|
||||
const input = document.createElement('input')
|
||||
input.addEventListener('keydown', (e) => e.stopPropagation())
|
||||
document.body.append(input)
|
||||
|
||||
input.dispatchEvent(
|
||||
new KeyboardEvent('keydown', { key: 'Control', ctrlKey: true, bubbles: true })
|
||||
)
|
||||
expect(modifier.held).toBe(true)
|
||||
})
|
||||
|
||||
// A modifier held across a keyboard app switch is cleared by the blur and delivers no keydown
|
||||
// on the way back, while the pointer parked on the pill fires no fresh mouseenter either.
|
||||
it('re-seeds from pointer movement after the window lost focus', () => {
|
||||
onPlatform(LINUX)
|
||||
const { modifier, hover, move } = pill()
|
||||
hover({ ctrlKey: true })
|
||||
window.dispatchEvent(new Event('blur'))
|
||||
expect(modifier.held).toBe(false)
|
||||
|
||||
move({ ctrlKey: true })
|
||||
expect(modifier.held).toBe(true)
|
||||
})
|
||||
|
||||
it('stops tracking once unhovered', () => {
|
||||
onPlatform(LINUX)
|
||||
const { modifier, hover, unhover } = pill()
|
||||
hover({ ctrlKey: true })
|
||||
unhover()
|
||||
expect(modifier.held).toBe(false)
|
||||
|
||||
keydown({ key: 'Control', ctrlKey: true })
|
||||
expect(modifier.held).toBe(false)
|
||||
})
|
||||
|
||||
it('stops tracking when the element is destroyed while hovered', () => {
|
||||
onPlatform(LINUX)
|
||||
const { modifier, hover, cleanup } = pill()
|
||||
hover({ ctrlKey: true })
|
||||
// Hovering again without leaving must not strand the first hover's listeners, which nothing
|
||||
// would then hold a reference to.
|
||||
hover({ ctrlKey: true })
|
||||
cleanup()
|
||||
expect(modifier.held).toBe(false)
|
||||
|
||||
keydown({ key: 'Control', ctrlKey: true })
|
||||
expect(modifier.held).toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,66 @@
|
||||
import type { Attachment } from 'svelte/attachments'
|
||||
import { isMac } from '$lib/utils'
|
||||
|
||||
/**
|
||||
* Tracks whether the modifier that turns a click into a new browser tab is held, but only while
|
||||
* the attached element is hovered, which is the only moment the answer is used.
|
||||
*/
|
||||
export function newTabModifier() {
|
||||
let held = $state(false)
|
||||
|
||||
// Only the modifier that actually yields a tab: shift opens a window, alt can start a
|
||||
// download, and on macOS ctrl+click is a secondary click.
|
||||
// Taken from each event rather than accumulated across keydown/keyup pairs, so a keyup lost to
|
||||
// a focus change cannot strand the flag on.
|
||||
const sync = (event: KeyboardEvent | MouseEvent) => {
|
||||
held = isMac() ? event.metaKey : event.ctrlKey
|
||||
}
|
||||
const clear = () => {
|
||||
held = false
|
||||
}
|
||||
|
||||
const attach: Attachment<HTMLElement> = (node) => {
|
||||
// One controller per hover: a mirrored remove list leaks any listener whose options drift.
|
||||
let hover: AbortController | undefined
|
||||
const leave = () => {
|
||||
hover?.abort()
|
||||
hover = undefined
|
||||
clear()
|
||||
}
|
||||
const enter = (event: MouseEvent) => {
|
||||
// Seeded from the hover itself: mouse events carry the same modifier flags as key events,
|
||||
// so a modifier already held before the pointer arrived reads correctly.
|
||||
sync(event)
|
||||
// Re-entering without an intervening leave would strand the previous controller: nothing
|
||||
// else references it, so its listeners could never be removed.
|
||||
hover?.abort()
|
||||
hover = new AbortController()
|
||||
const { signal } = hover
|
||||
// Same reason the hover seeds: a modifier held across a keyboard app switch delivers no
|
||||
// keydown on the way back, so the pointer is all that is left to re-read it from.
|
||||
node.addEventListener('mousemove', sync, { signal })
|
||||
// Capture: editors and menus stopPropagation the keys they handle, hiding the modifier
|
||||
// from a bubble-phase listener whenever focus sits in one.
|
||||
window.addEventListener('keydown', sync, { capture: true, signal })
|
||||
window.addEventListener('keyup', sync, { capture: true, signal })
|
||||
// Not capture, unlike the two above: blur does not bubble but does reach the window while
|
||||
// capturing, so it would fire for every element that loses focus.
|
||||
window.addEventListener('blur', clear, { signal })
|
||||
}
|
||||
|
||||
const life = new AbortController()
|
||||
node.addEventListener('mouseenter', enter, { signal: life.signal })
|
||||
node.addEventListener('mouseleave', leave, { signal: life.signal })
|
||||
return () => {
|
||||
life.abort()
|
||||
leave()
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
get held() {
|
||||
return held
|
||||
},
|
||||
attach
|
||||
}
|
||||
}
|
||||
@@ -3,6 +3,7 @@
|
||||
import { ExternalLink, PanelRight } from 'lucide-svelte'
|
||||
import { Button } from '$lib/components/common'
|
||||
import RowIcon from '$lib/components/common/table/RowIcon.svelte'
|
||||
import { newTabModifier } from '$lib/attachments/newTabModifier.svelte'
|
||||
import {
|
||||
hasToolDisplayActionHandler,
|
||||
runToolDisplayAction
|
||||
@@ -44,6 +45,8 @@
|
||||
const previewAction = $derived(available?.type === 'open_item_preview' ? available : undefined)
|
||||
const drawerAction = $derived(available?.type === 'open_created_resource' ? available : undefined)
|
||||
|
||||
const modifier = newTabModifier()
|
||||
|
||||
const hint = $derived(
|
||||
previewAction ? `Open ${wmPath} in the preview panel` : `Open ${wmPath} in a new tab`
|
||||
)
|
||||
@@ -67,7 +70,11 @@
|
||||
|
||||
{#if href}
|
||||
{#if wmKind}
|
||||
<span class="group inline-flex items-baseline">
|
||||
<!-- Only a preview pill can change icon, so only it is worth tracking the modifier for. -->
|
||||
<span
|
||||
class="group inline-flex items-baseline"
|
||||
{@attach previewAction ? modifier.attach : undefined}
|
||||
>
|
||||
<a
|
||||
{href}
|
||||
target={previewAction ? undefined : '_blank'}
|
||||
@@ -85,7 +92,9 @@
|
||||
<span
|
||||
class="absolute inset-0 flex items-center justify-center text-tertiary opacity-0 transition-opacity group-hover:opacity-100"
|
||||
>
|
||||
{#if previewAction}
|
||||
<!-- Narrower than the modifier list `onclick` bails on: only the modifier that
|
||||
really opens a tab may show the icon for one. -->
|
||||
{#if previewAction && !modifier.held}
|
||||
<PanelRight size={12} />
|
||||
{:else}
|
||||
<ExternalLink size={11} />
|
||||
|
||||
Reference in New Issue
Block a user