fix(signals): make inbox example cards and tool-off badge answer clicks - #89589
fix(signals): make inbox example cards and tool-off badge answer clicks#89589posthog[bot] wants to merge 4 commits into
Conversation
The self-driving onboarding example cards and the "Tool off" badge in the signal sources roster both took a click and changed nothing visible, so both read as dead clicks. Example cards now scroll to the setup command and pulse it on click, instead of only playing a sound. The "Tool off" badge is now a clickable warning tag that turns the tool on, the same action the row expansion offers one disclosure level below. Generated-By: PostHog Desktop Task-Id: d840838a-874a-4643-b2ef-0a9e06346966
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 3 should fix, 1 consider. Published 4 findings (view the review). Resolved comments: 2 fixed, 1 declined, 1 left for you |
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.36 MiB · 22 files | 🔺 +336 B (+0.0%) | ███░░░░░░░ 30.2% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.77 MiB · 3,236 files | 🔺 +104 B (+0.0%) | █████████░ 90.3% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 306.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 258.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/module.js |
| 246.0 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.6 KiB | src/lib/api.ts |
| 95.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.25 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.25 MiB · 17 files | 🔺 +109 B (+0.0%) | ████░░░░░░ 39.3% of 5.72 MiB |
| Deferred (lazy) | 2.09 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 746.2 KiB | dist/toolbar/toolbar-app-JDJU4DRU.css |
| 582.9 KiB | dist/toolbar/chunk-chunk-WSQWAVBM.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-6S5AAQW2.js |
| 133.8 KiB | dist/toolbar/chunk-chunk-FUPUKVGN.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.3 KiB | dist/toolbar/toolbar-app-OJAF4RZ6.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-6QYURFVS.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-RLNGEW4O.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +71.5 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1442.02 MiB · 🔺 +71.5 KiB (+0.0%)
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| type="warning" | ||
| size="small" | ||
| forceClickable | ||
| icon={enablingTool ? <Spinner /> : undefined} | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| if (!enablingTool) { | ||
| onEnableTool(tool) | ||
| } | ||
| }} | ||
| > | ||
| Turn it on | ||
| </LemonTag> | ||
| </Tooltip> |
There was a problem hiding this comment.
The new tool action cannot be used with a keyboard
Why we think it's a valid issue
- Checked: The shared component at
frontend/src/lib/lemon-ui/LemonTag/LemonTag.tsx, the new control atproducts/signals/frontend/inbox/components/config/AgentsRoster.tsx:498-516, the row container and the expansion body in the same file, and everyforceClickablecall site in the repository. - Found: The premise is exact.
LemonTagsetsrole={isClickable ? 'button' : undefined}atLemonTag.tsx:85on a plain<div>. A grep fortabIndex,onKeyDown,onKeyUp, andonKeyPressacross 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.
forceClickableatAgentsRoster.tsx:505overrides the tooltip-trigger guard inLemonTag.tsx:71, soisClickableresolves true and the button role is applied. - Found: The same file already holds a keyboard-operable equivalent.
AgentsRoster.tsx:312-319renders a realLemonButtonwith the same "Turn it on" label, the sameonEnableTool(tool)call, andloading={enablingTool}.LemonButtonis 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-473is a plaindivwithonClick={onExpand}and no role, notabIndex, 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
LemonTagusages, and 15 call sites acrossfrontend/srcandproductsopt in through the dedicatedforceClickableprop. - 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 sharedLemonTag, 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 theshould_fixbar 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>
There was a problem hiding this comment.
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
LemonButtonstyled 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-divpattern our frontend guide explicitly says to avoid. - Fix it once in the shared
LemonTagso 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.
The `jest-no-byrole-name-queries` semgrep rule blocks role+name queries because they are slow in jsdom. The overlay button carries an aria-label, so `getAllByLabelText` is the preferred query. Generated-By: PostHog Desktop Task-Id: d840838a-874a-4643-b2ef-0a9e06346966
|
The new "Tool off" badge is clickable and takes a button role via forceClickable, but ReviewHog flagged it as unusable via keyboard/screen reader, and the bot's own reply confirms this is a real gap and deliberately left unaddressed pending a human's choice among three fix options — a substantive, unresolved accessibility regression introduced by this PR.
Gate mechanics and policy version
|
The example-card click relied on a pulse animation and a smooth scroll to answer the click. Under prefers-reduced-motion the pulse was disabled with no substitute, and the scroll can be a no-op when the command is already in view, so the click had no visible answer for those users. Keep the accent ring as a static box-shadow in the reduced-motion block, and jump the scroll instead of animating it when reduced motion is set. Generated-By: PostHog Desktop Task-Id: 9dce5554-e7e0-477d-b2ca-8db9bde7771b
The roster now shows a clickable "Turn it on" badge on every collapsed row whose tool is off, so several enable controls can be on screen at once. The in-flight state was a single slot, so a second click overwrote the first: the first badge lost its spinner and reopened its guard while its request was still running, which then accepted a duplicate enable request. Hold the in-flight enablements in a set keyed by enablement, add one on start and remove it on completion, and derive each row's loading state from set membership. Each badge now keeps its own spinner and stays guarded against double-submission until its own request finishes. Adds a logic test for the concurrent case and updates the existing enablement test to the new set-shaped state. Generated-By: PostHog Desktop Task-Id: 9dce5554-e7e0-477d-b2ca-8db9bde7771b
Problem
Changes
onExampleClickcallback.How did you test this code?
InboxOnboarding.test.tsx: it asserts that a click on an example card scrolls to the setup command and adds the pulse class. This catches the exact regression — a click that leaves the DOM unchanged (a dead click).ReportCardpreview tests; both pass.prefers-reduced-motionfallback, and the scroll still fires under reduced motion, so the click is never silent.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Fully autonomous
Created with PostHog Desktop from this inbox report.