From 772fa983938a80b1389a2d9115b0465ae7d6fe47 Mon Sep 17 00:00:00 2001 From: scotttong Date: Sun, 2 Aug 2026 12:42:45 -0700 Subject: [PATCH] fix(ui): move IssueRow divider and hover wash to the row root (#10702) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Inbox and Tasks screens render issues as a vertical list of `IssueRow` components > - A recent refactor split `IssueRow` into a root `div` plus a full-row overlay `Link`, and the divider and hover styles moved onto that overlay > - As a result every row shows a bottom border and hover greys the text instead of tinting the background > - This pull request moves the divider and hover/selected wash back onto the root row band and keeps only positioning on the overlay > - The benefit is the list reads cleanly again: no stray dividers, and hover tints the background behind the text ## Linked Issues or Issue Description No public GitHub issue exists. Describing the bug in-PR (bug report): **What happened** - In the Inbox and Tasks list views, every row shows a 1px bottom border, including the last row. - Hovering a row dims/greys the row text instead of showing a background tint behind the content. **Expected behavior** - List rows in Inbox and Tasks show no separator lines by default. - Hover shows a subtle background tint behind the row content; the text stays fully legible. - The blocked inbox view keeps its intentional separators. **Steps to reproduce** 1. Open the Inbox or Tasks list view. 2. Note the horizontal border under every row, including the last. 3. Hover a row and note the text greys out rather than the background tinting. **Root cause** - PR #10526 restructured `IssueRow` from a single root `Link` into a root `div` plus a full-row `absolute inset-0` overlay `Link` (to keep header controls clickable). The divider and hover/selected/checklist background classes moved onto the overlay `Link`. `last:border-b-0` no longer matched (the Link is the first child of a multi-child div), and the hover wash painted on top of the content instead of behind it. **Paperclip version/commit** - Base commit: `8b83d69e3` (branched from current `master`). **Deployment mode** - UI (web) list views: Inbox and Tasks. ## What Changed - `ui/src/components/IssueRow.tsx`: moved the divider classes and the hover/selected/checklist background wash from the overlay `Link` to the root row `div`, so the tint paints behind the content and `last:border-b-0` matches the real last row. The overlay `Link` now keeps only `absolute inset-0` positioning and the focus ring. Renamed the `hideDivider` prop to an opt-in `showDivider` (default `false`). Kept `[&_button]:relative [&_button]:z-10` on the root so the Archive button stays clickable above the overlay, and kept the `isArchiving` collapse animation on the root row. - `ui/src/components/IssuesList.tsx`: dropped the old `hideDivider` usage (dividers are now opt-in). - `ui/src/pages/Inbox.tsx`: dropped the old `hideDivider` usage. - `ui/src/components/BlockedInboxView.tsx`: added `showDivider` so this view keeps its separators. ## Verification - `cd ui && npx tsc -b` — typecheck passes with the change. - Manual (recommended for reviewer): in the Inbox and Tasks list views, confirm no per-row bottom border and that the last row has none. Because dark-mode `--border` is 10% white and near-invisible in screenshots, assert the computed `border-bottom-width` on a row element rather than eyeballing pixels. - Hover a row: text stays legible; a background tint appears behind the content. - Inbox: the Archive button appears on hover and is clickable (the overlay does not swallow the click). - Blocked inbox view: separators still render. ## Risks - Low risk. The change relocates existing Tailwind classes between two elements of the same row and renames one internal prop; no data or API surface changes. All `IssueRow` call sites were updated in this PR (verified: no remaining `hideDivider` references). ## Model Used - Claude, Opus 4.8 (`claude-opus-4-8`), extended thinking with tool use. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [ ] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [ ] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [ ] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- ui/src/components/BlockedInboxView.tsx | 1 + ui/src/components/IssueRow.test.tsx | 82 ++++++++++++++++++++++++-- ui/src/components/IssueRow.tsx | 19 +++--- ui/src/components/IssuesList.tsx | 1 - ui/src/pages/Inbox.test.tsx | 25 ++++---- ui/src/pages/Inbox.tsx | 1 - 6 files changed, 104 insertions(+), 25 deletions(-) diff --git a/ui/src/components/BlockedInboxView.tsx b/ui/src/components/BlockedInboxView.tsx index 871f537324..1880fe2e0c 100644 --- a/ui/src/components/BlockedInboxView.tsx +++ b/ui/src/components/BlockedInboxView.tsx @@ -359,6 +359,7 @@ function BlockedInboxRow({ { root.render(); }); + // The hover wash lives on the ROOT row band (not the overlay link) so the + // tint paints behind the content. Selected rows suppress the accent hover. + const row = container.firstElementChild as HTMLElement | null; const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null; - expect(link).not.toBeNull(); - expect(link?.className).toContain("hover:bg-transparent"); - expect(link?.className).not.toContain("hover:bg-accent/50"); + expect(row).not.toBeNull(); + expect(row?.className).toContain("hover:bg-transparent"); + expect(row?.className).not.toContain("hover:bg-accent/50"); + // The overlay link no longer carries the hover wash. + expect(link?.className ?? "").not.toContain("hover:bg-transparent"); + expect(link?.className ?? "").not.toContain("hover:bg-accent/50"); act(() => { root.unmount(); @@ -348,12 +354,17 @@ describe("IssueRow", () => { ); }); + // `aria-current="step"` stays on the overlay link (the focusable target), + // but the current-step wash moved to the ROOT row band alongside the hover + // wash so it paints behind the content. + const row = container.firstElementChild as HTMLElement | null; const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null; expect(link).not.toBeNull(); expect(link?.getAttribute("aria-current")).toBe("step"); - expect(link?.className).toContain("bg-primary/5"); - expect(link?.className).not.toContain("border-l-"); + expect(row?.className).toContain("bg-primary/5"); + expect(row?.className).not.toContain("border-l-"); + expect(link?.className ?? "").not.toContain("bg-primary/5"); act(() => { root.unmount(); @@ -446,4 +457,65 @@ describe("IssueRow", () => { root.unmount(); }); }); + + it("renders no bottom divider by default", () => { + const root = createRoot(container); + + act(() => { + root.render(); + }); + + // Dividers are opt-in: without `showDivider` no row-separating border + // renders on either the root band or the overlay link. + const row = container.firstElementChild as HTMLElement | null; + const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null; + expect(row).not.toBeNull(); + expect(row?.className).not.toContain("border-b"); + expect(link?.className ?? "").not.toContain("border-b"); + + act(() => { + root.unmount(); + }); + }); + + it("renders an opt-in bottom divider on the row root when showDivider is set", () => { + const root = createRoot(container); + + act(() => { + root.render(); + }); + + // The divider lives on the ROOT row band with `last:border-b-0` so the real + // last row drops its border — it is not on the overlay link. + const row = container.firstElementChild as HTMLElement | null; + const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null; + expect(row?.className).toContain("border-b"); + expect(row?.className).toContain("last:border-b-0"); + expect(link?.className ?? "").not.toContain("border-b"); + + act(() => { + root.unmount(); + }); + }); + + it("keeps the hover wash on the row root while the overlay link stays a bare positioning layer", () => { + const root = createRoot(container); + + act(() => { + root.render(); + }); + + const row = container.firstElementChild as HTMLElement | null; + const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null; + // Hover wash paints behind the content on the root band... + expect(row?.className).toContain("hover:bg-accent/50"); + expect(link?.className ?? "").not.toContain("hover:bg-accent/50"); + // ...and the overlay link keeps only positioning + focus concerns. + expect(link?.className).toContain("absolute"); + expect(link?.className).toContain("inset-0"); + + act(() => { + root.unmount(); + }); + }); }); diff --git a/ui/src/components/IssueRow.tsx b/ui/src/components/IssueRow.tsx index edf45747d5..a175417c43 100644 --- a/ui/src/components/IssueRow.tsx +++ b/ui/src/components/IssueRow.tsx @@ -57,8 +57,8 @@ interface IssueRowProps { * not crossed out by it. */ chevronInGuide?: boolean; - /** Suppress the row divider (parents with expanded children keep visual attachment to their subtree). */ - hideDivider?: boolean; + /** Opt in to a bottom divider on this row (default off; used by views that intentionally keep separators). */ + showDivider?: boolean; } export function IssueRow({ @@ -86,7 +86,7 @@ export function IssueRow({ onMouseEnter, treeGuides = 0, chevronInGuide = false, - hideDivider = false, + showDivider = false, }: IssueRowProps) { const issuePathId = issue.identifier ?? issue.id; const identifier = issue.identifier ?? issue.id.slice(0, 8); @@ -170,6 +170,13 @@ export function IssueRow({ // when scrubbing the mouse fast across the list. "group relative flex items-start gap-2 rounded-lg py-2.5 pl-2 pr-3 text-sm no-underline text-inherit sm:items-center sm:py-2 sm:pl-1", "[&_button]:relative [&_button]:z-10", + // Divider + hover/selected/checklist wash live on the ROOT row band so + // the tint paints BEHIND the content and `last:border-b-0` matches the + // real last row. Keeping these on the overlay Link (PR #10526) made the + // last row keep its border and the hover wash paint over the text. + showDivider && "border-b border-border last:border-b-0", + selected ? "hover:bg-transparent" : "hover:bg-accent/50", + checklistCurrentStep ? "bg-primary/5" : null, className, )} > @@ -183,11 +190,9 @@ export function IssueRow({ aria-current={checklistCurrentStep ? "step" : undefined} onClickCapture={() => rememberIssueDetailLocationState(issuePathId, detailState)} className={cn( + // Overlay Link keeps ONLY positioning + focus ring so header controls + // stay clickable above it; visual washes belong on the root above. "absolute inset-0 rounded-lg no-underline text-inherit focus-visible:z-10 focus-visible:outline-none focus-visible:ring-(length:--rad-3) focus-visible:ring-ring", - !hideDivider && "border-b border-border last:border-b-0", - selected ? "hover:bg-transparent" : "hover:bg-accent/50", - checklistCurrentStep ? "bg-primary/5" : null, - className, )} > Open {identifier}: {issue.title} diff --git a/ui/src/components/IssuesList.tsx b/ui/src/components/IssuesList.tsx index bdc780966a..2aeb017322 100644 --- a/ui/src/components/IssuesList.tsx +++ b/ui/src/components/IssuesList.tsx @@ -2097,7 +2097,6 @@ export function IssuesList({ onMouseEnter={() => setNavSelectionFromPointer(`issue:${issue.id}`)} treeGuides={depth} chevronInGuide={depth > 0 && hasChildren} - hideDivider={hasChildren && isExpanded} checklistStepNumber={checklistStepNumber} checklistCurrentStep={checklistMeta?.currentStepIssueId === issue.id} checklistDependencyChips={checklistDependencyChips} diff --git a/ui/src/pages/Inbox.test.tsx b/ui/src/pages/Inbox.test.tsx index 69518d147f..49b9bdaa7c 100644 --- a/ui/src/pages/Inbox.test.tsx +++ b/ui/src/pages/Inbox.test.tsx @@ -465,12 +465,14 @@ describe("Inbox toolbar", () => { const rows = container.querySelectorAll("[data-inbox-item]"); - const linkOf = (row: Element): HTMLAnchorElement | null => - row.querySelector("a[data-inbox-issue-link]"); + // The hover wash lives on the IssueRow root band (the overlay link's + // parent), not the overlay link itself. + const bandOf = (row: Element): HTMLElement | null => + row.querySelector("a[data-inbox-issue-link]")?.parentElement ?? null; // Nothing selected before hover — both rows show the hover-accent class. - expect(linkOf(rows[0]!)?.className).toContain("hover:bg-accent/50"); - expect(linkOf(rows[1]!)?.className).toContain("hover:bg-accent/50"); + expect(bandOf(rows[0]!)?.className).toContain("hover:bg-accent/50"); + expect(bandOf(rows[1]!)?.className).toContain("hover:bg-accent/50"); // Hovering paints via CSS `:hover` only — it must NOT flip a row into the // state-selected band (which would swap to hover:bg-transparent). Coupling @@ -482,9 +484,9 @@ describe("Inbox toolbar", () => { rows[1]!.dispatchEvent(new MouseEvent("mouseover", { bubbles: true })); rows[1]!.dispatchEvent(new MouseEvent("mouseenter", { bubbles: false })); }); - expect(linkOf(rows[0]!)?.className).toContain("hover:bg-accent/50"); - expect(linkOf(rows[1]!)?.className).toContain("hover:bg-accent/50"); - expect(linkOf(rows[1]!)?.className).not.toContain("hover:bg-transparent"); + expect(bandOf(rows[0]!)?.className).toContain("hover:bg-accent/50"); + expect(bandOf(rows[1]!)?.className).toContain("hover:bg-accent/50"); + expect(bandOf(rows[1]!)?.className).not.toContain("hover:bg-transparent"); act(() => { root.unmount(); @@ -578,12 +580,13 @@ describe("Inbox toolbar", () => { }); const root = createRoot(container); - const linkOf = (row: Element): HTMLAnchorElement | null => - row.querySelector("a[data-inbox-issue-link]"); - // The keyboard-selected row swaps to `hover:bg-transparent`; find its index. + // The keyboard-selected row swaps to `hover:bg-transparent` on its root + // band (the overlay link's parent, where the wash now lives); find its index. + const bandOf = (row: Element): HTMLElement | null => + row.querySelector("a[data-inbox-issue-link]")?.parentElement ?? null; const selectedRowIndex = () => [...container.querySelectorAll("[data-inbox-item]")].findIndex((row) => - linkOf(row)?.className.includes("hover:bg-transparent"), + bandOf(row)?.className.includes("hover:bg-transparent"), ); try { diff --git a/ui/src/pages/Inbox.tsx b/ui/src/pages/Inbox.tsx index 2cfa3569c3..c86873fe70 100644 --- a/ui/src/pages/Inbox.tsx +++ b/ui/src/pages/Inbox.tsx @@ -2689,7 +2689,6 @@ export function Inbox() { issue={issue} issueLinkState={issueLinkState} treeGuides={depth} - hideDivider={hasChildren && isExpanded} selected={selected} className={ isArchiving