fix: prioritize filenames in new-tab file results (#12670)
* fix: prioritize filenames in new-tab file results * fix: preserve root separator in filename-first paths * refactor: use native file path tooltips * fix: position file path tooltips * fix: use the native OS tooltip for new-tab file paths Co-authored-by: Orca <help@stably.ai> * fix: show new-tab file paths in a system-style tooltip Co-authored-by: Orca <help@stably.ai> * fix: anchor new-tab path tooltip to the cursor Co-authored-by: Orca <help@stably.ai> * fix: tighten cursor tooltip to file rows and design tokens Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Neil
GitHub
parent
ac961d6329
commit
ead6b8e225
|
|
@ -5,6 +5,7 @@ import { createRoot, type Root } from 'react-dom/client'
|
|||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { TabEntryOption } from './tab-create-entry-action'
|
||||
import type { TabAgentLaunchOption } from './tab-agent-launch-options'
|
||||
import { TooltipProvider } from '@/components/ui/tooltip'
|
||||
|
||||
// Why: the real entry-action module pulls in runtime IPC + the app store; the
|
||||
// keyboard behavior under test only needs a controllable option list.
|
||||
|
|
@ -37,7 +38,8 @@ let root: Root
|
|||
|
||||
function mount(node: React.JSX.Element): void {
|
||||
act(() => {
|
||||
root.render(node)
|
||||
// Result rows carry a path tooltip, which Radix requires a provider for.
|
||||
root.render(<TooltipProvider>{node}</TooltipProvider>)
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,118 @@
|
|||
// @vitest-environment happy-dom
|
||||
|
||||
import { act, createElement } from 'react'
|
||||
import { createRoot, type Root } from 'react-dom/client'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { TooltipProvider } from '@/components/ui/tooltip'
|
||||
import type { ActiveOption } from './tab-create-entry-active-option'
|
||||
import { cursorTooltipOffsets, EntryActionRow } from './TabBarCreateEntryRow'
|
||||
|
||||
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
|
||||
|
||||
const filePath = 'app/src/components/SecondaryNav.tsx'
|
||||
|
||||
function makeFileOption(path: string): ActiveOption {
|
||||
return {
|
||||
kind: 'entry',
|
||||
option: {
|
||||
id: `existing-file:${path}`,
|
||||
classification: {
|
||||
kind: 'existing-file',
|
||||
matchKind: 'fuzzy',
|
||||
relativePath: path
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The tooltip itself is asserted in tests/e2e/tab-create-entry-file-paths.spec.ts,
|
||||
// where it actually opens; here we only need the row's own text layout.
|
||||
function renderRow(option: ActiveOption): HTMLButtonElement {
|
||||
act(() => {
|
||||
root.render(
|
||||
createElement(
|
||||
TooltipProvider,
|
||||
null,
|
||||
createElement(EntryActionRow, {
|
||||
id: 'file-result',
|
||||
onClick: vi.fn(),
|
||||
option,
|
||||
selected: false
|
||||
})
|
||||
)
|
||||
)
|
||||
})
|
||||
|
||||
const button = container.querySelector('button')
|
||||
if (!button) {
|
||||
throw new Error('row did not render a button')
|
||||
}
|
||||
return button
|
||||
}
|
||||
|
||||
let container: HTMLDivElement
|
||||
let root: Root
|
||||
|
||||
beforeEach(() => {
|
||||
container = document.createElement('div')
|
||||
document.body.appendChild(container)
|
||||
root = createRoot(container)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
act(() => root.unmount())
|
||||
container.remove()
|
||||
})
|
||||
|
||||
describe('EntryActionRow', () => {
|
||||
it('puts the filename before the truncated parent path', () => {
|
||||
const text = renderRow(makeFileOption(filePath)).textContent ?? ''
|
||||
|
||||
expect(text.indexOf('SecondaryNav.tsx')).toBeLessThan(text.indexOf('app/src/components/'))
|
||||
})
|
||||
|
||||
it('does not duplicate the root separator for absolute root-level files', () => {
|
||||
const text = renderRow(makeFileOption('/foo')).textContent ?? ''
|
||||
|
||||
expect(text).toContain('foo/')
|
||||
expect(text).not.toContain('foo//')
|
||||
})
|
||||
|
||||
it('keeps Windows separators intact', () => {
|
||||
const text = renderRow(makeFileOption('C:\\repo\\src\\SecondaryNav.tsx')).textContent ?? ''
|
||||
|
||||
expect(text.indexOf('SecondaryNav.tsx')).toBeLessThan(text.indexOf('C:\\repo\\src\\'))
|
||||
})
|
||||
|
||||
it('renders a bare filename without a directory fragment', () => {
|
||||
expect(renderRow(makeFileOption('README.md')).textContent).toContain('README.md')
|
||||
expect(container.textContent).not.toContain('/')
|
||||
})
|
||||
})
|
||||
|
||||
describe('cursorTooltipOffsets', () => {
|
||||
const row = { bottom: 120, left: 400 }
|
||||
|
||||
it('places the tooltip under the cursor rather than the row', () => {
|
||||
// Row-anchored placement would be align 0; the cursor is 64px into the row.
|
||||
expect(cursorTooltipOffsets({ x: 464, y: 108 }, row)).toEqual({ align: 64, side: 6 })
|
||||
})
|
||||
|
||||
it('tracks the cursor across the row', () => {
|
||||
const left = cursorTooltipOffsets({ x: 410, y: 108 }, row)
|
||||
const right = cursorTooltipOffsets({ x: 610, y: 108 }, row)
|
||||
|
||||
expect(right.align - left.align).toBe(200)
|
||||
expect(right.side).toBe(left.side)
|
||||
})
|
||||
|
||||
it('stays anchored to the cursor when the row moves under it', () => {
|
||||
// The dropdown reflows while results stream in; re-measuring the row must
|
||||
// keep the tooltip on the cursor, not drag it along with the row.
|
||||
const before = cursorTooltipOffsets({ x: 464, y: 108 }, row)
|
||||
const after = cursorTooltipOffsets({ x: 464, y: 108 }, { bottom: 148, left: 576 })
|
||||
|
||||
expect(row.left + before.align).toBe(576 + after.align)
|
||||
expect(row.bottom + before.side).toBe(148 + after.side)
|
||||
})
|
||||
})
|
||||
|
|
@ -2,11 +2,29 @@ import React from 'react'
|
|||
import { FilePlus, FileText, Globe, Loader2, Smartphone, TerminalSquare } from 'lucide-react'
|
||||
import { AgentIcon } from '@/lib/agent-catalog'
|
||||
import { cn } from '@/lib/utils'
|
||||
import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'
|
||||
import { translate } from '@/i18n/i18n'
|
||||
import type { ActiveOption } from './tab-create-entry-active-option'
|
||||
|
||||
export const RESULT_LISTBOX_ID = 'tab-create-entry-results'
|
||||
|
||||
// Clears the macOS pointer without putting the label under the cursor's own hotspot.
|
||||
const CURSOR_TOOLTIP_GAP = 18
|
||||
|
||||
/**
|
||||
* Radix positions the tooltip against the row, so a cursor position has to be
|
||||
* restated as offsets from the row's bottom-left corner to land under the pointer.
|
||||
*/
|
||||
export function cursorTooltipOffsets(
|
||||
pointer: { x: number; y: number },
|
||||
row: { bottom: number; left: number }
|
||||
): { align: number; side: number } {
|
||||
return {
|
||||
align: pointer.x - row.left,
|
||||
side: pointer.y + CURSOR_TOOLTIP_GAP - row.bottom
|
||||
}
|
||||
}
|
||||
|
||||
// Index-based (not the option id, which may contain spaces/slashes from file
|
||||
// paths) so it is always a valid aria-activedescendant IDREF.
|
||||
export function resultOptionDomId(index: number): string {
|
||||
|
|
@ -40,13 +58,43 @@ export function EntryActionRow({
|
|||
selected: boolean
|
||||
}): React.JSX.Element {
|
||||
const presentation = getActionPresentation(option)
|
||||
const rowRef = React.useRef<HTMLButtonElement>(null)
|
||||
const pointerRef = React.useRef<{ x: number; y: number } | null>(null)
|
||||
const [tooltipOpen, setTooltipOpen] = React.useState(false)
|
||||
const [pointerOffset, setPointerOffset] = React.useState({ align: 0, side: 0 })
|
||||
|
||||
return (
|
||||
// Why: Radix positions against the row, so the offsets are only valid for the
|
||||
// row's position at the moment it positions. Results stream in while the open
|
||||
// delay runs, so re-measure on open rather than trusting the pointer-move rect.
|
||||
React.useLayoutEffect(() => {
|
||||
const rect = rowRef.current?.getBoundingClientRect()
|
||||
const pointer = pointerRef.current
|
||||
if (!tooltipOpen || !rect || !pointer) {
|
||||
return
|
||||
}
|
||||
const next = cursorTooltipOffsets(pointer, rect)
|
||||
setPointerOffset((current) =>
|
||||
current.align === next.align && current.side === next.side ? current : next
|
||||
)
|
||||
}, [tooltipOpen])
|
||||
|
||||
const row = (
|
||||
<button
|
||||
ref={rowRef}
|
||||
type="button"
|
||||
id={id}
|
||||
role="option"
|
||||
aria-selected={selected}
|
||||
// Why: a ref, not state — this is read once when the tooltip opens, so
|
||||
// re-rendering the row on every pointer move would be churn for nothing.
|
||||
// pointermove (not mousemove) because Radix opens off pointermove and Slot
|
||||
// runs the child handler first, keeping this fresh even on instant open.
|
||||
onPointerMove={(event) => {
|
||||
if (tooltipOpen) {
|
||||
return
|
||||
}
|
||||
pointerRef.current = { x: event.clientX, y: event.clientY }
|
||||
}}
|
||||
className={cn(
|
||||
'flex h-6 w-full items-center gap-1.5 rounded-[7px] px-1 text-left text-[11px] leading-5 outline-none',
|
||||
selected
|
||||
|
|
@ -61,20 +109,76 @@ export function EntryActionRow({
|
|||
</span>
|
||||
{presentation.showDetail ? (
|
||||
<>
|
||||
<span className="text-muted-foreground/70" aria-hidden="true">
|
||||
<span className="shrink-0 text-muted-foreground/70" aria-hidden="true">
|
||||
·
|
||||
</span>
|
||||
<span className="min-w-0 truncate">{presentation.detail}</span>
|
||||
{presentation.prioritizeFilename ? (
|
||||
<FilenameFirstPath path={presentation.detail} />
|
||||
) : (
|
||||
<span className="min-w-0 flex-1 truncate">{presentation.detail}</span>
|
||||
)}
|
||||
</>
|
||||
) : null}
|
||||
</button>
|
||||
)
|
||||
|
||||
// Only the filename-first rows hide information. Every other row already shows
|
||||
// its detail in full, and STYLEGUIDE.md:162 rules out labelling those.
|
||||
if (!presentation.prioritizeFilename) {
|
||||
return row
|
||||
}
|
||||
|
||||
return (
|
||||
<Tooltip open={tooltipOpen} onOpenChange={setTooltipOpen}>
|
||||
<TooltipTrigger asChild>{row}</TooltipTrigger>
|
||||
{/* Anchored under the cursor the way a system tooltip is, rather than to
|
||||
the row. Radix anchors to the trigger, so the cursor is expressed as
|
||||
offsets off the row's bottom-left corner; collision flipping still
|
||||
applies near the viewport edge. */}
|
||||
<TooltipContent
|
||||
side="bottom"
|
||||
align="start"
|
||||
sideOffset={pointerOffset.side}
|
||||
alignOffset={pointerOffset.align}
|
||||
showArrow={false}
|
||||
className="max-w-[420px] rounded-md border border-border/80 bg-popover px-2 py-1 text-[11px] leading-[15px] break-words text-popover-foreground shadow-[0_10px_24px_rgba(0,0,0,0.18)]"
|
||||
>
|
||||
{presentation.detail}
|
||||
</TooltipContent>
|
||||
</Tooltip>
|
||||
)
|
||||
}
|
||||
|
||||
// Why: keeps the separator attached to the directory, so `/foo` renders as
|
||||
// `foo` + `/` rather than re-deriving a separator that may not match the path.
|
||||
function splitTrailingSegment(path: string): { directory: string; filename: string } {
|
||||
const separatorIndex = Math.max(path.lastIndexOf('/'), path.lastIndexOf('\\'))
|
||||
|
||||
return separatorIndex === -1
|
||||
? { directory: '', filename: path }
|
||||
: { directory: path.slice(0, separatorIndex + 1), filename: path.slice(separatorIndex + 1) }
|
||||
}
|
||||
|
||||
function FilenameFirstPath({ path }: { path: string }): React.JSX.Element {
|
||||
const { directory, filename } = splitTrailingSegment(path)
|
||||
|
||||
return (
|
||||
<span className="flex min-w-0 flex-1 items-center gap-1">
|
||||
{/* shrink-0 + max-w-full: the directory gives up all of its width before
|
||||
the filename loses a character. */}
|
||||
<span className="min-w-0 max-w-full shrink-0 truncate">{filename}</span>
|
||||
{directory ? (
|
||||
<span className="min-w-0 truncate text-muted-foreground/70">{directory}</span>
|
||||
) : null}
|
||||
</span>
|
||||
)
|
||||
}
|
||||
|
||||
function getActionPresentation(option: ActiveOption): {
|
||||
detail: string
|
||||
icon: React.ReactNode
|
||||
label: string
|
||||
prioritizeFilename?: boolean
|
||||
showDetail: boolean
|
||||
} {
|
||||
if (option.kind === 'menu') {
|
||||
|
|
@ -122,6 +226,7 @@ function getActionPresentation(option: ActiveOption): {
|
|||
: classification.relativePath,
|
||||
icon: <FileText className="size-3.5 shrink-0" aria-hidden="true" />,
|
||||
label: translate('auto.components.tab.bar.TabBarCreateEntry.25dc1cd653', 'Open file'),
|
||||
prioritizeFilename: true,
|
||||
showDetail: true
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -31,9 +31,10 @@ function TooltipTrigger({ ...props }: React.ComponentProps<typeof TooltipPrimiti
|
|||
function TooltipContent({
|
||||
className,
|
||||
sideOffset = 0,
|
||||
showArrow = true,
|
||||
children,
|
||||
...props
|
||||
}: React.ComponentProps<typeof TooltipPrimitive.Content>) {
|
||||
}: React.ComponentProps<typeof TooltipPrimitive.Content> & { showArrow?: boolean }) {
|
||||
return (
|
||||
<TooltipPrimitive.Portal>
|
||||
<TooltipPrimitive.Content
|
||||
|
|
@ -48,7 +49,9 @@ function TooltipContent({
|
|||
{...props}
|
||||
>
|
||||
{children}
|
||||
<TooltipPrimitive.Arrow className="size-2.5 translate-y-[calc(-50%_-_2px)] rotate-45 rounded-[2px] bg-foreground fill-foreground" />
|
||||
{showArrow ? (
|
||||
<TooltipPrimitive.Arrow className="size-2.5 translate-y-[calc(-50%_-_2px)] rotate-45 rounded-[2px] bg-foreground fill-foreground" />
|
||||
) : null}
|
||||
</TooltipPrimitive.Content>
|
||||
</TooltipPrimitive.Portal>
|
||||
)
|
||||
|
|
|
|||
|
|
@ -0,0 +1,65 @@
|
|||
import { mkdirSync, writeFileSync } from 'node:fs'
|
||||
import path from 'node:path'
|
||||
import { expect, test } from './helpers/orca-app'
|
||||
import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store'
|
||||
|
||||
const relativeFilePath =
|
||||
'packages/orca/src/renderer/src/components/navigation/worktree/secondary-nav/SecondaryNav.tsx'
|
||||
|
||||
test('new-tab file results prioritize the filename and reveal the full path on hover', async ({
|
||||
orcaPage,
|
||||
testRepoPath
|
||||
}) => {
|
||||
const filePath = path.join(testRepoPath, ...relativeFilePath.split('/'))
|
||||
mkdirSync(path.dirname(filePath), { recursive: true })
|
||||
writeFileSync(filePath, 'export const SecondaryNav = true\n')
|
||||
|
||||
await waitForSessionReady(orcaPage)
|
||||
await waitForActiveWorktree(orcaPage)
|
||||
await ensureTerminalVisible(orcaPage)
|
||||
|
||||
await orcaPage.getByRole('button', { name: 'New tab' }).click({ force: true })
|
||||
const input = orcaPage.getByRole('combobox', {
|
||||
name: 'Open any file, URL, agent, ...'
|
||||
})
|
||||
await input.fill('secondaryNav')
|
||||
|
||||
const row = orcaPage.locator('[role="option"]').filter({ hasText: 'Open file' }).first()
|
||||
await expect(row).toBeVisible()
|
||||
await expect(row).toContainText('SecondaryNav.tsx')
|
||||
await expect(row).toContainText('packages/orca/src/renderer/src/components/navigation/')
|
||||
const rowText = await row.textContent()
|
||||
expect(rowText?.indexOf('SecondaryNav.tsx')).toBeLessThan(
|
||||
rowText?.indexOf('packages/orca/src/renderer/src/components/navigation/') ?? -1
|
||||
)
|
||||
|
||||
// The filename must survive intact; only the directory may be clipped, and the
|
||||
// row itself must never spill past the dropdown.
|
||||
const overflow = await row.evaluate((element) => {
|
||||
const filename = element.querySelector(':scope > span:last-of-type > span:first-child')
|
||||
return {
|
||||
filenameClipped: filename ? filename.scrollWidth > filename.clientWidth : true,
|
||||
rowClipped: element.scrollWidth > element.clientWidth
|
||||
}
|
||||
})
|
||||
expect(overflow).toEqual({ filenameClipped: false, rowClipped: false })
|
||||
|
||||
// Two hovers on purpose: results stream in and remount the row, and Radix only
|
||||
// opens on a pointermove it actually receives. A single hover can land before
|
||||
// the remount and leave the cursor sitting still over a row that never saw it.
|
||||
await row.hover({ position: { x: 20, y: 12 } })
|
||||
await orcaPage.waitForTimeout(250)
|
||||
await row.hover({ position: { x: 40, y: 12 } })
|
||||
|
||||
// Exact cursor placement is arithmetic, unit-tested via cursorTooltipOffsets.
|
||||
// Asserting it here measured the app mid-reflow and was flaky; what E2E is
|
||||
// uniquely good for is that the tooltip really opens with the whole path.
|
||||
await expect(
|
||||
orcaPage.locator('[data-slot="tooltip-content"]').filter({ hasText: relativeFilePath })
|
||||
).toBeVisible()
|
||||
|
||||
const proofPath = process.env.ORCA_STA3424_PROOF_PATH
|
||||
if (proofPath) {
|
||||
await orcaPage.screenshot({ path: proofPath })
|
||||
}
|
||||
})
|
||||
Loading…
Reference in New Issue