Skip to content

fix(web): stop the "Needs response" tag overlapping the session title - #4375

Merged
Edwinhe03 merged 2 commits into
mainfrom
fix/sidebar-needs-response-overlap
Aug 7, 2026
Merged

fix(web): stop the "Needs response" tag overlapping the session title#4375
Edwinhe03 merged 2 commits into
mainfrom
fix/sidebar-needs-response-overlap

Conversation

@Edwinhe03

@Edwinhe03 Edwinhe03 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Related issue

Closes #4373

Summary

The sidebar's "Needs response" tag is absolutely positioned, so the row's right
padding is the only thing holding the title clear of it. That reserve narrows to
make room for the trailing pin/kebab — but it narrowed on group-focus-within,
while the tag fades out (and the controls appear) on
group-has-[:focus-visible].

focus-within matches a plain mouse click; :focus-visible does not. Clicking
a row therefore cut the reserve from 116px to 56px with the tag still fully
opaque and the controls still hidden
, sliding the title 59px underneath it.
Because the tag surface is translucent (bg-brand-accent/15), the collision
reads as a washed-out opacity glitch rather than the layout problem it is.

Fix: key the reserve on group-has-[:focus-visible] so it narrows exactly
when the tag fades and the controls appear — the three can no longer disagree
about whether that space is free.

Also repoints the Inbox count bubble from the shared amber --warning to
--brand-accent, matching the pink the tag and unread dot already use.

Scope

This is the selected-state defect only. A row at interface font 15px+ still
overlaps in every state, idle included, because the 116px reserve is fixed
while the tag's width tracks the font size (97px at 11px → 147px at 18px). That
is a separate pre-existing bug and is deliberately left alone — the obvious
structural fix for it (rendering the tag in the title flow so the browser derives
the reserve) costs real title width, up to 45px at 18px, so it needs its own
change and its own discussion.

ELI5 + diagram

Three things have to agree about whether the space at the row's right edge is
free: the padding that reserves it, the tag that sits in it, and the buttons that
replace the tag. Two of them listened for "keyboard focus"; the padding listened
for "any focus at all," which includes an ordinary click. So a click moved the
title into space the tag had not vacated.

CLICKED ROW — before                      CLICKED ROW — after
┌────────────────────────────────┐        ┌────────────────────────────────┐
│ Fix badge opacity a[Needs resp]│        │ Fix badge opa…   [Needs resp]  │
│                    ↑ title runs│        │              ↑ title stops     │
│ reserve 56px  (tag opacity 1)  │        │ reserve 116px (tag opacity 1)  │
│ ✗ padding said "free", tag     │        │ ✓ padding and tag agree        │
│   was still there              │        │                                │
└────────────────────────────────┘        └────────────────────────────────┘

Test Plan

Automated — from web/:

pnpm type-check                              # tsc -b, clean
pnpm lint                                    # oxlint, clean
pnpm vitest run src/shell src/components     # 2537 passed, 0 failed

e2e — from the repo root:

uv run pytest tests/e2e_ui/sessions/test_sidebar_awaiting_tag_layout.py --ui-skip-build
# 1 passed

Covered two ways, because each catches what the other can't:

  • Unit (Sidebar.test.tsx) pins the class-level contract: every trigger that
    narrows the reserve must also fade the tag, and the row must not use
    focus-within.
  • e2e (new) measures the real thing — the painted glyph extent (via a Range,
    since the title clips with text-overflow: ellipsis) against the tag's left
    edge, after an actual click with the pointer moved away. jsdom reports every
    box as 0×0, so geometry needs a browser.

Both were confirmed to fail with the focus-within trigger restored, so
neither passes vacuously. The e2e failure reproduces the reported number exactly:
-0.6px when idle → +59.4px selected.

Measured, main vs this branch (260px sidebar, interface font 13px):

state reserve tag opacity main this branch
idle 116px 1 ok ok
hover 56px 0 overlap, but tag invisible unchanged
clicked / selected 56 → 116px 1 +59.4px overlap −0.6px, ok
keyboard focus 56px 0 ok (controls shown) unchanged

Idle title width is unchanged at 120px across 11/13/18px, so nothing
truncates earlier than it did on main.

Manual: open a session with a pending approval prompt and click its sidebar
row. The title should ellipsise before the pink tag instead of running under it.
Hover and keyboard-tab to the row as well — the tag should fade as the pin/kebab
appear, exactly as before.

Demo

main (left) vs this branch (right), both showing a clicked/selected row,
rendered against the real compiled CSS:

selected row, main vs fix

Visual baseline

UI Snapshot drifts on exactly one of the five baselines,
test_populated_sidebar — the only frame whose fixture renders an Inbox count
(the landing fixture stubs an empty session list). Regenerated in the
digest-pinned Playwright image the gate itself renders in, so the bytes match.

The diff is a single 16×16px region at (288,118): the Inbox bubble, amber
(218,164,71) → brand pink (227,87,150). Nothing else in the 1280×800 frame
moves, and the reserve fix contributes no pixel delta here (the fixture's
awaiting row is idle, whose geometry is unchanged).

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change

Coverage notes

Added one unit test (the trigger contract) and one Playwright test (the rendered
geometry), and updated the existing padding-class assertion in
Sidebar.test.tsx. Each was verified to fail with the bug reintroduced.

The pre-existing 2536 src/shell + src/components tests still pass. They
covered the badge's behaviour (which state renders which marker, precedence of
the approval tag over the unread dot) but could not catch this: it is a
trigger mismatch whose only symptom is geometric, and jsdom reports every box as
0×0.

Changelog

Clicking a sidebar session that needs a response no longer runs its title under the "Needs response" tag, and the Inbox count badge now matches the sidebar's pink accent

@github-actions github-actions Bot added size/S Pull request size: S P3-low Priority: minor issue, cosmetic, nice-to-have labels Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

UI Snapshot doesn't match the committed baseline.

If this UI change is intentional, update the baseline — each path renders in the same pinned image, so the result matches this gate:

  • Label the PR (recommended): add the update-ui-snapshot label — the bot regenerates the baseline in the pinned image, pushes it back here, and re-runs the checks.
  • Locally with Docker: run tests/e2e_ui/visual/regen_baseline_docker.sh, review the PNG, then commit + push.

Diff PNGs (expected_=baseline, actual_=your render, diff_) are in the run artifact. Full guide: tests/e2e_ui/visual/README.md.

@omnigent-ci

omnigent-ci Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Review: fix(web): stop the "Needs response" tag overlapping the session title

I read the full diff and traced the surrounding ConversationRow render logic, the SESSION_STATE_SLOT_CLASS positioning, SessionStateBadge, and the affected tests. The fix is sound and correctly reasoned.

Blocking issues

None. The core change is correct:

  • isAwaitingInline gates all three touch points consistently — the row reserve (pr-28 md:pr-2 fallback instead of the fixed md:pr-29), the new in-flow tag, and the absolute trailing slot (sessionState !== null && !isAwaitingInline). An awaiting row renders exactly one session-state-badge (now in-flow), so no duplicate badge and no dead reserve branch.
  • Selection mode still short-circuits to the checkbox for awaiting rows (matches prior behavior), and non-awaiting dot markers stay in the absolute slot with their existing reserve.
  • The in-flow tag's hide triggers (md:group-hover/focus-visible/aria-expanded[true]:opacity-0) mirror the slot class, so hover/menu-open behavior is preserved while the pin/kebab take the space — and because the tag stays in layout (opacity, not display), the title's truncation point holds during the fade, which is precisely the bug being fixed.
  • Existing tests (getByTestId("session-state-badge"), data-state="awaiting", collapsed-project header marker) remain valid since the testid and DOM containment within the <li> are unchanged.

Security vulnerabilities

None. Pure presentational CSS/markup change; no new inputs, network calls, or dependency changes.

Non-blocking notes

  • Mobile spacing: the mobile awaiting reserve drops from pr-48 (192px) to pr-28 (112px). On mobile the in-flow tag now ends at the ~112px padding boundary while the always-visible kebab sits at right-1, leaving a noticeable empty gap between the tag and the kebab (previously the absolute badge sat at right-[4.5rem], closer to the controls). Not a correctness issue, but the mobile resting layout is worth a quick visual sanity check since the provided screenshots are desktop-width.
  • Nested tooltip: the awaiting badge (which carries its own Radix Tooltip) now renders inside the row's Link, which in the non-mobile/non-project path is itself a TooltipTrigger. Non-awaiting badges live outside the Link, so this is a newly-nested tooltip. It works in Radix and the tag fades to opacity-0 on desktop hover anyway, but hovering the (still-hit-testable) invisible tag could surface both tooltips; adding pointer-events-none to the in-flow wrapper when hidden would tidy that up.

Summary

A well-scoped, well-justified fix that replaces a brittle fixed-width reserve with in-flow layout so the title truncates against the tag's real box at every interface font size — the correct structural solution rather than a bigger magic number. The change is internally consistent across all three gated code paths, tests remain valid, the automated + measured-geometry test plan is thorough, and desktop before/after screenshots are attached. No blocking or security concerns; only minor mobile-spacing and nested-tooltip polish worth a glance.


Automated review by Polly · workflow run

@Edwinhe03
Edwinhe03 force-pushed the fix/sidebar-needs-response-overlap branch from f07efca to 5eebc07 Compare August 7, 2026 21:49
@github-actions github-actions Bot added size/L Pull request size: L and removed size/S Pull request size: S labels Aug 7, 2026
The tag is absolutely positioned, so the row's right padding is the only thing
holding the title clear of it. That reserve narrows to make room for the trailing
pin/kebab -- but it narrowed on `group-focus-within`, while the tag fades (and the
controls appear) on `group-has-[:focus-visible]`.

`focus-within` matches a plain mouse click; `:focus-visible` does not. Clicking a
row therefore cut the reserve from 116px to 56px with the tag still fully opaque
and the controls still hidden, sliding the title 59px underneath it. The tag
surface is translucent, so the collision reads as a washed-out opacity glitch
rather than the layout problem it is.

Key the reserve on `group-has-[:focus-visible]` so it narrows exactly when the
tag fades and the controls appear -- the three can no longer disagree about
whether that space is free. Measured on the selected row: +59.4px of overlap ->
-0.6px, with the idle row's title width byte-identical (120px at every interface
font size), so nothing truncates earlier than before.

Note this is the selected-state defect only. A row at interface font 15px+ still
overlaps in *every* state, including idle, because the 116px reserve is fixed
while the tag's width tracks the font size; that is a separate pre-existing bug
and is left alone here.

Covered two ways: a unit test pinning that the reserve and the tag's fade share
their triggers (the class-level contract), and a Playwright test measuring the
real painted glyphs against the tag's edge after a click (jsdom reports every box
as 0x0, so geometry needs a browser). Both were confirmed to fail with the
`focus-within` trigger restored.

Also repoints the Inbox count bubble from the shared amber `--warning` to
`--brand-accent`, matching the pink the tag and unread dot already use.

Signed-off-by: Edwin He <41037314+Edwinhe03@users.noreply.github.com>
Co-authored-by: Isaac
@Edwinhe03
Edwinhe03 force-pushed the fix/sidebar-needs-response-overlap branch from 0e67fa4 to 60007eb Compare August 7, 2026 22:45
… Inbox badge

Regenerated in the digest-pinned Playwright image the gate renders in, so the
bytes match what CI compares against.

Only the populated-sidebar baseline drifts; the other four visual snapshots
render identically. The diff is a single 16x16px region at (288,118) -- the Inbox
count bubble, amber (218,164,71) -> brand pink (227,87,150). Nothing else in the
1280x800 frame changes, and the row-reserve fix contributes no pixel delta here
(the fixture's awaiting row is idle, whose geometry is unchanged).

Signed-off-by: Edwin He <41037314+Edwinhe03@users.noreply.github.com>
Co-authored-by: Isaac
@Edwinhe03
Edwinhe03 enabled auto-merge (squash) August 7, 2026 23:29
@Edwinhe03
Edwinhe03 merged commit 16f9538 into main Aug 7, 2026
39 checks passed
@Edwinhe03
Edwinhe03 deleted the fix/sidebar-needs-response-overlap branch August 7, 2026 23:29
@github-actions github-actions Bot added the no-doc-update Merged PR does not need a docs update label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🏷️ Doc impact: no-doc-update

Internal CSS/layout bugfix to the sidebar row padding and badge color with tests; it fixes a visual glitch without changing any documented user-facing behavior, config, or integration.

Auto-classified on merge. Set the label manually before merging to override. · run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-doc-update Merged PR does not need a docs update P3-low Priority: minor issue, cosmetic, nice-to-have size/L Pull request size: L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Sidebar "Needs response" tag overlaps the session title (fixed padding vs. font-scaled tag)

1 participant