Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions products/signals/frontend/inbox/components/config/AgentsRoster.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -495,11 +495,30 @@ const AgentRow = memo(function AgentRow({
</div>
<span className="truncate text-xs leading-4 text-muted">{agent.watches}</span>
</div>
{tag && (
{toolOff && tool?.enablement ? (
// A live badge, not a dead one: clicking it turns the tool on, the same action the
// expansion offers one disclosure level below.
<Tooltip title={`${tool.toolName} is off, so this source has nothing to read. Turn it on.`}>
<LemonTag
type="warning"
size="small"
forceClickable
icon={enablingTool ? <Spinner /> : undefined}
onClick={(e) => {
e.stopPropagation()
if (!enablingTool) {
onEnableTool(tool)
Comment thread
posthog[bot] marked this conversation as resolved.
}
}}
>
Turn it on
</LemonTag>
</Tooltip>
Comment on lines +503 to +516

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The new tool action cannot be used with a keyboard

should_fix

Why we think it's a valid issue
  • Checked: The shared component at frontend/src/lib/lemon-ui/LemonTag/LemonTag.tsx, the new control at products/signals/frontend/inbox/components/config/AgentsRoster.tsx:498-516, the row container and the expansion body in the same file, and every forceClickable call site in the repository.
  • Found: The premise is exact. LemonTag sets role={isClickable ? 'button' : undefined} at LemonTag.tsx:85 on a plain <div>. A grep for tabIndex, onKeyDown, onKeyUp, and onKeyPress across that file returns nothing. The element therefore takes a button role, but it accepts no keyboard focus and it handles no key press.
  • Found: The new control does reach that branch. forceClickable at AgentsRoster.tsx:505 overrides the tooltip-trigger guard in LemonTag.tsx:71, so isClickable resolves true and the button role is applied.
  • Found: The same file already holds a keyboard-operable equivalent. AgentsRoster.tsx:312-319 renders a real LemonButton with the same "Turn it on" label, the same onEnableTool(tool) call, and loading={enablingTool}. LemonButton is already imported and is used again for the "Connect" action in the same row, so the swap the reviewer proposes is local and cheap.
  • Found: The keyboard path to that expansion button does not exist either. The row container at AgentsRoster.tsx:469-473 is a plain div with onClick={onExpand} and no role, no tabIndex, and no key handler. This code sits outside the diff, so the roster was already closed to keyboard users before this change.
  • Found: The shape is a house-wide pattern, not a local slip. The repository holds about 55 clickable LemonTag usages, and 15 call sites across frontend/src and products opt in through the dedicated forceClickable prop.
  • Impact: There is a small new harm. The slot previously held an inert tag with no role. It now announces a button that a screen-reader user can land on through button navigation and then cannot activate. A control that names itself a button and does nothing is worse for assistive technology than a plain label.
  • Impact: No keyboard user loses a capability. The badge duplicates an action that was already out of keyboard reach, because the row that discloses it never accepted focus. So this change does not remove a working path; it fails to add one.
  • Priority: Lowered to consider. The diagnosis is verified and the harm is real, so the finding stays on record. The root cause sits in the shared LemonTag, where a proper fix serves all 55 clickable usages, and the surrounding row is already not operable by keyboard. Repairing this one badge would still leave keyboard users unable to work the roster, which puts the item below the should_fix bar for this PR.
Issue description

LemonTag renders a div with role="button". It does not add tabIndex or keyboard activation. Keyboard users cannot focus or activate this new action.

Suggested fix

Use a LemonButton styled for this compact warning action. If LemonTag must remain, add focus support and handle Enter and Space activation.

Prompt to fix with AI (copy-paste)
## Context
@products/signals/frontend/inbox/components/config/AgentsRoster.tsx#L503-516

<issue_description>
`LemonTag` renders a `div` with `role="button"`. It does not add `tabIndex` or keyboard activation. Keyboard users cannot focus or activate this new action.
</issue_description>

<issue_validation>
- **Checked:** The shared component at `frontend/src/lib/lemon-ui/LemonTag/LemonTag.tsx`, the new control at `products/signals/frontend/inbox/components/config/AgentsRoster.tsx:498-516`, the row container and the expansion body in the same file, and every `forceClickable` call site in the repository.
- **Found:** The premise is exact. `LemonTag` sets `role={isClickable ? 'button' : undefined}` at `LemonTag.tsx:85` on a plain `<div>`. A grep for `tabIndex`, `onKeyDown`, `onKeyUp`, and `onKeyPress` across that file returns nothing. The element therefore takes a button role, but it accepts no keyboard focus and it handles no key press.
- **Found:** The new control does reach that branch. `forceClickable` at `AgentsRoster.tsx:505` overrides the tooltip-trigger guard in `LemonTag.tsx:71`, so `isClickable` resolves true and the button role is applied.
- **Found:** The same file already holds a keyboard-operable equivalent. `AgentsRoster.tsx:312-319` renders a real `LemonButton` with the same "Turn it on" label, the same `onEnableTool(tool)` call, and `loading={enablingTool}`. `LemonButton` is already imported and is used again for the "Connect" action in the same row, so the swap the reviewer proposes is local and cheap.
- **Found:** The keyboard path to that expansion button does not exist either. The row container at `AgentsRoster.tsx:469-473` is a plain `div` with `onClick={onExpand}` and no role, no `tabIndex`, and no key handler. This code sits outside the diff, so the roster was already closed to keyboard users before this change.
- **Found:** The shape is a house-wide pattern, not a local slip. The repository holds about 55 clickable `LemonTag` usages, and 15 call sites across `frontend/src` and `products` opt in through the dedicated `forceClickable` prop.
- **Impact:** There is a small new harm. The slot previously held an inert tag with no role. It now announces a button that a screen-reader user can land on through button navigation and then cannot activate. A control that names itself a button and does nothing is worse for assistive technology than a plain label.
- **Impact:** No keyboard user loses a capability. The badge duplicates an action that was already out of keyboard reach, because the row that discloses it never accepted focus. So this change does not remove a working path; it fails to add one.
- **Priority:** Lowered to `consider`. The diagnosis is verified and the harm is real, so the finding stays on record. The root cause sits in the shared `LemonTag`, where a proper fix serves all 55 clickable usages, and the surrounding row is already not operable by keyboard. Repairing this one badge would still leave keyboard users unable to work the roster, which puts the item below the `should_fix` bar for this PR.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Use a `LemonButton` styled for this compact warning action. If `LemonTag` must remain, add focus support and handle Enter and Space activation.
</potential_solution>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a real gap and worth fixing — I've left it for a human to choose the approach rather than pick one unattended. Confirmed: the badge renders with a button role (via forceClickable) but LemonTag adds no keyboard focus or Enter/Space handling, so keyboard and screen-reader users can land on it and not activate it. It matters more than the badge alone suggests: turning the tool on is currently mouse-only, because the equivalent 'Turn it on' button lives inside the row expansion, and the row only expands on a mouse click (a plain clickable div with no keyboard support — pre-existing, outside this PR).

The reason it needs a human call is that the three sensible fixes each carry a trade-off:

  • Swap the badge to a LemonButton styled as a compact warning action (the reviewer's preference, and it matches our 'interactive elements are real buttons' convention). This changes the deliberate warning-tag look and should get a quick visual check in the row.
  • Keep the tag and add tabIndex + Enter/Space handling at this call site. Smallest change, but it doubles down on a clickable-div pattern our frontend guide explicitly says to avoid.
  • Fix it once in the shared LemonTag so all ~55 clickable tags become keyboard-operable. Best long-term, but it's a shared-component change well beyond this PR and needs its own review.

My recommendation: go with the LemonButton swap here (it's local, matches the existing expansion button, and is convention-compliant), and file the shared LemonTag keyboard-support fix separately. The row-expansion keyboard gap is a separate pre-existing issue worth its own ticket.

) : tag ? (
<LemonTag type={tag.type} size="small">
{tag.label}
</LemonTag>
)}
) : null}
<span className="w-38 shrink-0 truncate text-right text-xs text-muted">
{entities.length > 0 && `${enabledCount} of ${entities.length} ${agent.entityNoun} on`}
</span>
Expand Down Expand Up @@ -585,7 +604,7 @@ export function AgentsRoster(): JSX.Element {
isHealthChecksToggling,
isCiSignalsToggling,
toolStatusBySource,
enablingTool,
enablingTools,
} = useValues(signalSourcesLogic)
const {
toggleConversations,
Expand Down Expand Up @@ -832,16 +851,15 @@ export function AgentsRoster(): JSX.Element {
agent.steerable && state.sourceConfig && !state.sourceConfig.id.startsWith('new_')
? state.sourceConfig
: null
const enablement = toolStatusBySource[agent.source]?.enablement
return (
<AgentRow
key={agent.source}
agent={agent}
state={state}
tool={toolStatusBySource[agent.source]}
expanded={expandedSource === agent.source}
enablingTool={
!!enablingTool && enablingTool === toolStatusBySource[agent.source]?.enablement
}
enablingTool={!!enablement && enablingTools.has(enablement)}
onExpand={() =>
setExpandedSource((current) => (current === agent.source ? null : agent.source))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,28 @@
animation: none;
}
}

// A click on an example card scrolls to the setup command and pulses this ring, so the click has a
// visible answer instead of a silent one.
.InboxOnboarding__commandPulse {
animation: InboxOnboarding__commandPulse 0.9s ease-out 2;
}

@keyframes InboxOnboarding__commandPulse {
0%,
100% {
box-shadow: 0 0 0 0 transparent;
}

35% {
box-shadow: 0 0 0 4px var(--color-accent);
}
}

@media (prefers-reduced-motion: reduce) {
// No pulse, but keep the ring as a static highlight so the click still has a visible answer.
.InboxOnboarding__commandPulse {
animation: none;
box-shadow: 0 0 0 4px var(--color-accent);
}
Comment thread
posthog[bot] marked this conversation as resolved.
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import '@testing-library/jest-dom'

import { cleanup, fireEvent, render } from '@testing-library/react'

import { InboxOnboardingTakeover } from './InboxOnboarding'

// The manual-setup escape hatch pulls in `inboxOnboardingLogic` (kea); it is unrelated to the card click.
jest.mock('./ManualSetupAction', () => ({ ManualSetupAction: () => null }))
jest.mock('../../inboxAnalytics', () => ({
captureInboxWelcomeViewed: jest.fn(),
captureInboxWelcomeCommandCopied: jest.fn(),
}))
jest.mock('./meep', () => ({ playMeep: jest.fn() }))
jest.mock('lib/components/TZLabel', () => ({ TZLabel: ({ time }: { time: string }) => <span>{time}</span> }))

describe('InboxOnboardingTakeover example cards', () => {
beforeAll(() => {
// jsdom does not implement scrollIntoView; stub it so the click handler can call it.
Element.prototype.scrollIntoView = jest.fn()
})
afterEach(cleanup)

it('answers a click on an example card by scrolling to the setup command and pulsing it', () => {
const { container, getAllByLabelText } = render(<InboxOnboardingTakeover />)
// A dead click leaves the DOM unchanged; the pulse ring must be absent before the click.
expect(container.querySelector('.InboxOnboarding__commandPulse')).toBeNull()

const overlays = getAllByLabelText(/Jump to the setup command/i)
expect(overlays.length).toBeGreaterThan(0)
fireEvent.click(overlays[0])

expect(Element.prototype.scrollIntoView).toHaveBeenCalled()
expect(container.querySelector('.InboxOnboarding__commandPulse')).not.toBeNull()
})
})
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import './InboxOnboarding.scss'

import { useActions } from 'kea'
import { useEffect } from 'react'
import { useCallback, useEffect, useRef } from 'react'

import { IconBolt, IconGithub, IconInfo, IconNotebook, IconPause, IconX } from '@posthog/icons'
import { LemonButton, Tooltip } from '@posthog/lemon-ui'
Expand Down Expand Up @@ -32,7 +32,7 @@ interface Beat {
label: string
description: JSX.Element
subtext: string | JSX.Element
preview: JSX.Element
Preview: (props: { onExampleClick: () => void }) => JSX.Element
}

/**
Expand Down Expand Up @@ -74,7 +74,7 @@ const BEATS: Beat[] = [
Your first 3 PRs each month are free, then it's $15 per PR after that. <PrPricingInfo />
</>
),
preview: <PullRequestPreview />,
Preview: PullRequestPreview,
},
{
label: 'Reports, when it needs your call.',
Expand All @@ -85,7 +85,7 @@ const BEATS: Beat[] = [
</>
),
subtext: 'Reports without PRs are free.',
preview: <ReportPreview />,
Preview: ReportPreview,
},
]

Expand All @@ -97,25 +97,30 @@ const BEATS: Beat[] = [
function SelfDrivingCommand({
size = 'md',
surface,
containerRef,
}: {
size?: 'sm' | 'md'
surface: InboxWelcomeCopySurface
// The example cards scroll to this ref and pulse it, so a click on an example has a visible answer.
containerRef?: React.RefObject<HTMLDivElement>
}): JSX.Element {
return (
<CommandBlock
command={SELF_DRIVING_WIZARD_COMMAND}
copyLabel="self-driving setup command"
ariaLabel="Copy self-driving setup command"
decoration="rainbow"
size={size}
// The takeover is the control arm of the welcome experiment; the banner shows one
// fixed layout regardless of arm, so its copies carry no variant.
onCopy={() =>
captureInboxWelcomeCommandCopied({ variant: surface === 'takeover' ? 'control' : null, surface })
}
// rounded-md sits one step inside the rounded-lg card/banner it nests in.
className="!m-0 rounded-md border border-primary bg-surface-secondary hover:border-accent"
/>
<div ref={containerRef} className="rounded-md">
<CommandBlock
command={SELF_DRIVING_WIZARD_COMMAND}
copyLabel="self-driving setup command"
ariaLabel="Copy self-driving setup command"
decoration="rainbow"
size={size}
// The takeover is the control arm of the welcome experiment; the banner shows one
// fixed layout regardless of arm, so its copies carry no variant.
onCopy={() =>
captureInboxWelcomeCommandCopied({ variant: surface === 'takeover' ? 'control' : null, surface })
}
// rounded-md sits one step inside the rounded-lg card/banner it nests in.
className="!m-0 rounded-md border border-primary bg-surface-secondary hover:border-accent"
/>
</div>
)
}

Expand All @@ -137,7 +142,7 @@ function Hero(): JSX.Element {
)
}

function CommandCard(): JSX.Element {
function CommandCard({ commandRef }: { commandRef?: React.RefObject<HTMLDivElement> }): JSX.Element {
return (
<div className="flex flex-col gap-3 rounded-lg border border-primary bg-surface-primary p-5">
<div>
Expand All @@ -146,7 +151,7 @@ function CommandCard(): JSX.Element {
Run it in your project's repo, or set it up yourself below.
</p>
</div>
<SelfDrivingCommand size="md" surface="takeover" />
<SelfDrivingCommand size="md" surface="takeover" containerRef={commandRef} />
<ul className="m-0 flex list-none flex-col gap-1.5 p-0">
{WIZARD_SETS_UP.map((item) => (
<li key={item.label} className="flex items-center gap-2.5 text-sm text-secondary">
Expand All @@ -162,7 +167,15 @@ function CommandCard(): JSX.Element {
)
}

function BeatRow({ beat, index }: { beat: Beat; index: number }): JSX.Element {
function BeatRow({
beat,
index,
onExampleClick,
}: {
beat: Beat
index: number
onExampleClick: () => void
}): JSX.Element {
return (
<div className="flex flex-col gap-2">
<div className="flex items-baseline gap-3 mb-1">
Expand All @@ -174,7 +187,9 @@ function BeatRow({ beat, index }: { beat: Beat; index: number }): JSX.Element {
</div>
{/* Real inbox cards, marked as examples and kept inert by the preview wrapper.
Full-width on mobile; indented to align under the beat text from sm up. */}
<div className="select-none pl-0 sm:pl-8">{beat.preview}</div>
<div className="select-none pl-0 sm:pl-8">
<beat.Preview onExampleClick={onExampleClick} />
</div>
{beat.subtext ? (
<span className="text-[13px] text-secondary leading-snug text-right">{beat.subtext}</span>
) : null}
Expand All @@ -189,17 +204,33 @@ function BeatRow({ beat, index }: { beat: Beat; index: number }): JSX.Element {
* report list – a plain centered column (not itself a card) that eases in with a subtle scale + fade.
*/
export function InboxOnboardingTakeover(): JSX.Element {
const commandRef = useRef<HTMLDivElement>(null)

useEffect(() => {
captureInboxWelcomeViewed({ variant: 'control' })
}, [])

const highlightCommand = useCallback(() => {
const el = commandRef.current
if (!el) {
return
}
// Jump instead of animating the scroll when the user asked for reduced motion.
const reduceMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches
el.scrollIntoView({ behavior: reduceMotion ? 'auto' : 'smooth', block: 'center' })
// Restart the pulse on repeat clicks: drop the class, force a reflow, then add it back.
el.classList.remove('InboxOnboarding__commandPulse')
void el.offsetWidth
el.classList.add('InboxOnboarding__commandPulse')
}, [])

Comment thread
posthog[bot] marked this conversation as resolved.
return (
<div className="InboxOnboardingTakeover mx-auto flex w-full max-w-3xl flex-col gap-6 px-4 py-6 sm:px-6 sm:py-12">
<Hero />
<CommandCard />
<CommandCard commandRef={commandRef} />
<div className="flex flex-col gap-7">
{BEATS.map((beat, index) => (
<BeatRow key={beat.label} beat={beat} index={index} />
<BeatRow key={beat.label} beat={beat} index={index} onExampleClick={highlightCommand} />
))}
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import { playMeep } from './meep'
* requests / Reports tabs use), fed mock data – so they read as the genuine article rather than a
* lookalike. Because they look real, they're marked plainly as examples: an "Example" tag sits on
* each card, the card itself is made non-interactive (so its Review/Archive buttons don't offer
* live hover states or misleading tooltips), and a single click surface explains that the real work
* arrives once you run the setup command. The meep stays as flair, but it's no longer the only sign
* a click did anything.
* live hover states or misleading tooltips), and a single click surface sits on top. A click on that
* surface scrolls to the setup command and pulses it, so the click has a visible answer. The meep
* stays as flair on top of that.
*
* The sample work is a wink at Silicon Valley (the show): Pied Piper's middle-out compression and
* the ever-looming Hooli.
Expand Down Expand Up @@ -62,10 +62,19 @@ const REPORT_SAMPLE: Omit<SignalReport, 'created_at' | 'updated_at'> = {
* Wraps a real `ReportCard` and makes it legibly a sample. The card is rendered non-interactive
* (`pointer-events-none`, `aria-hidden`) so its Review/Archive buttons and links no longer offer
* live hover states or misleading tooltips ("Archive this report") that invite dead clicks. An
* "Example" tag labels it at a glance, and a single click surface on top plays the meep flair while
* a tooltip explains that real work lands here once the setup command runs.
* "Example" tag labels it at a glance, and a single click surface on top scrolls to the setup
* command and pulses it (via `onExampleClick`), so a click gets a visible answer rather than only a
* sound.
*/
function PreviewCard({ report, tabKey }: { report: SignalReport; tabKey: 'pulls' | 'reports' }): JSX.Element {
function PreviewCard({
report,
tabKey,
onExampleClick,
}: {
report: SignalReport
tabKey: 'pulls' | 'reports'
onExampleClick: () => void
}): JSX.Element {
return (
// `@container` so ReportCard's `@lg:` row layout resolves against the preview width (it has no
// inbox-list container here). `role="presentation"` – the whole thing is decorative.
Expand All @@ -83,26 +92,41 @@ function PreviewCard({ report, tabKey }: { report: SignalReport; tabKey: 'pulls'
Example
</LemonTag>

{/* One interactive surface over the whole card: a click plays the meep flair, and the
tooltip is the real signal – it says this is a preview and how to get the real thing. */}
<Tooltip title="This is an example. Run the command above to get real ones in your inbox.">
{/* One interactive surface over the whole card. A click scrolls to the setup command and
pulses it, so the click has a visible answer, then plays the meep flair on top. */}
<Tooltip title="This is an example. Run the setup command to get real ones. Click to jump to it.">
<button
type="button"
aria-label="Example card – run the setup command to get real ones in your inbox"
aria-label="Jump to the setup command that brings real ones to your inbox"
className="absolute inset-0 z-10 h-full w-full cursor-pointer"
onClick={() => playMeep()}
onClick={() => {
onExampleClick()
playMeep()
}}
/>
</Tooltip>
</div>
)
}

export function PullRequestPreview(): JSX.Element {
export function PullRequestPreview({ onExampleClick }: { onExampleClick: () => void }): JSX.Element {
const landed = landedHoursAgo(2)
return <PreviewCard report={{ ...PULL_REQUEST_SAMPLE, created_at: landed, updated_at: landed }} tabKey="pulls" />
return (
<PreviewCard
report={{ ...PULL_REQUEST_SAMPLE, created_at: landed, updated_at: landed }}
tabKey="pulls"
onExampleClick={onExampleClick}
/>
)
}

export function ReportPreview(): JSX.Element {
export function ReportPreview({ onExampleClick }: { onExampleClick: () => void }): JSX.Element {
const landed = landedHoursAgo(4)
return <PreviewCard report={{ ...REPORT_SAMPLE, created_at: landed, updated_at: landed }} tabKey="reports" />
return (
<PreviewCard
report={{ ...REPORT_SAMPLE, created_at: landed, updated_at: landed }}
tabKey="reports"
onExampleClick={onExampleClick}
/>
)
}
Loading
Loading