mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-14 17:22:00 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The left navigation mixes agent avatars, task status icons, and page icons. > - Agent avatars used 20px boxes. Task and page icons used 16px boxes. > - The different widths moved agent labels 4px to the right of other labels. > - This pull request gives these icons the same 16px square slot. > - Shared label positions make pinned and recent items easier to scan. ## Linked Issues or Issue Description Refs #19685, which added mixed agent and page pins. No duplicate alignment PR was found. **What happened?** Agent avatars in pinned and recent navigation were wider than task and page icons. Their labels started 4px farther to the right. Inline icon layout also produced different box heights. **Expected behavior** Use the existing 16px task and page icon size for all leading navigation icons. Center each icon in a square slot. Start the labels at the same horizontal position. **Steps to reproduce** 1. Pin an agent conversation and a page such as Routines. 2. Open agent conversations and tasks so both appear in Recents. 3. Compare the icon boxes and label positions across the rows. **Paperclip version or commit** Base commit: `c5e6ab5ca813`. **Deployment mode** Local source checkout. This is a client layout issue. ## What Changed - Use 16px agent avatars for pinned and recent conversation rows. - Give `SidebarNavItem` a fixed 16px square icon slot with centered contents and left-aligned text. The existing token scale supplies the size. - Add light and dark Storybook examples with mixed icons and long labels. The browser assertions compare slot sizes, label positions, and glyph bounds. - Add a production `CoordinatorSidebar` regression test that compares pinned and recent avatar dimensions with a task status icon. ## Verification - `pnpm exec vitest run ui/src/components/SidebarNavItem.test.tsx ui/src/components/CoordinatorSidebar.test.tsx ui/src/components/SidebarAgents.test.tsx` passes: 59 tests across three files. - Regression proof: the new production-avatar test fails with the original 20px avatars (`expected 20 to be 16`) and passes with 16px avatars. This addresses the initial review's test coverage gap. - `pnpm check:token-gates` passes. - `pnpm --filter @paperclipai/ui typecheck` passes. - `pnpm --filter @paperclipai/ui build` passes on the final source tree. - `pnpm verify --base origin/master` returns exit 2 because the new Storybook file needs broad CI coverage. The focused local tests run separately. CI must supply the remaining coverage. - Browser inspection on macOS checked all 13 mixed rows in light and dark themes. Every icon slot measured 16 by 16 CSS pixels. Every label started at the same position. Before and after captures use production components with fixture data; they do not prove server persistence or permissions. - Review `sidebar-icon-alignment--dark` and `sidebar-icon-alignment--light` in Storybook to inspect the mixed rows and run their layout assertions. - [Greptile review](https://github.com/paperclipai/paperclip/pull/20926#issuecomment-6806138954): 5/5 on `e779f4707e7826d577b23a2991f3f36755e954be`. The initial 4/5 review requested production Recents coverage. The added regression test addresses that finding; the thread is resolved. - CI queue recovery: the superseded first run was canceled. The next run stalled before runner assignment; GitHub accepted force-cancel but left it queued and refused the rerun request. The final empty commit requests fresh CI without changing the source tree. Required CI is tracked in [run 43095809539](https://github.com/paperclipai/paperclip/actions/runs/43095809539). Its jobs are still queued; CI completion is not yet claimed. - External blocker: after more than 15 minutes, the fresh run still has zero jobs. Other repository PR runs are also queued. A repository Actions or runner administrator must restore dispatch before the required typecheck, test, build, and E2E gates can report results. The root cause of the dispatch delay is not established. ## Risks - Low risk. The shared icon slot also affects other navigation rows. Existing badge, rail, and expanded-state tests cover the shared component. - Agent avatars become smaller in pinned and recent navigation, as requested. No data or API contracts change. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected. This change is a scoped UI bug fix. ## Model Used - OpenAI GPT-6 via Codex. The host does not expose the exact served model snapshot or context window size. Used reasoning, code editing, shell execution, and browser inspection. ## 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) - [x] 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 - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [x] 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: Scott Tong <scott@scottsmbpm5max.lan> Co-authored-by: Paperclip <noreply@paperclip.ing>
239 lines
9.2 KiB
TypeScript
239 lines
9.2 KiB
TypeScript
import { createContext, useContext, type ReactNode } from "react";
|
|
import { NavLink } from "@/lib/router";
|
|
import type { IssueNavigationPrefetchHandlers } from "@/hooks/useIssueNavigationPrefetch";
|
|
import { SIDEBAR_SCROLL_RESET_STATE } from "../lib/navigation-scroll";
|
|
import { cn, SIDEBAR_RAIL_HIDDEN_LABEL } from "../lib/utils";
|
|
import { useSidebar } from "../context/SidebarContext";
|
|
import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip";
|
|
import type { LucideIcon } from "lucide-react";
|
|
import { Badge } from "@/components/ui/badge";
|
|
import { TruncatedText } from "./TruncatedText";
|
|
|
|
/**
|
|
* Forces the full-label (non-rail) presentation for any `SidebarNavItem`
|
|
* rendered beneath it, regardless of the global `useSidebar().collapsed` state.
|
|
*
|
|
* Contextual takeover routes replace the global navigation inside the same
|
|
* sidebar shell. The user's saved global collapse preference can still be
|
|
* `collapsed=true`, so this provider keeps contextual navigation readable
|
|
* without mutating that preference.
|
|
*/
|
|
const SidebarNavExpandedContext = createContext(false);
|
|
|
|
export function SidebarNavExpandedProvider({ children }: { children: ReactNode }) {
|
|
return (
|
|
<SidebarNavExpandedContext.Provider value={true}>
|
|
{children}
|
|
</SidebarNavExpandedContext.Provider>
|
|
);
|
|
}
|
|
|
|
export function useSidebarNavExpanded() {
|
|
return useContext(SidebarNavExpandedContext);
|
|
}
|
|
|
|
interface SidebarNavItemProps {
|
|
to: string;
|
|
label: string;
|
|
icon?: LucideIcon;
|
|
/**
|
|
* Pre-rendered icon element for rows whose icon isn't a plain Lucide
|
|
* component (e.g. the agent rows' `AgentIcon`). Takes precedence over `icon`;
|
|
* the caller supplies a 16px icon to fit the shared navigation slot.
|
|
*/
|
|
iconNode?: ReactNode;
|
|
end?: boolean;
|
|
className?: string;
|
|
labelClassName?: string;
|
|
badge?: number;
|
|
badgeTone?: "default" | "danger" | "warning";
|
|
/**
|
|
* Accessible noun for the numeric badge when collapsed to the rail, where the
|
|
* count is rendered as a dot (e.g. `badgeLabel="unread"` → "Inbox, 28 unread").
|
|
*/
|
|
badgeLabel?: string;
|
|
textBadge?: string;
|
|
textBadgeTone?: "default" | "amber";
|
|
alert?: boolean;
|
|
liveCount?: number;
|
|
/**
|
|
* Overrides NavLink's own route matching for rows whose active state is
|
|
* computed externally (agent rows match `/agents/:ref` across tab suffixes).
|
|
*/
|
|
active?: boolean;
|
|
/** Rendered after the label, before the right-aligned status cluster. */
|
|
trailing?: ReactNode;
|
|
/** Accessible text for `trailing` status content, surfaced in the collapsed rail (where `trailing` is hidden). */
|
|
trailingLabel?: string;
|
|
/** Rendered inside the right-aligned status cluster, before the live dot. */
|
|
liveAccessory?: ReactNode;
|
|
/** Extra link handlers, such as a task's open-intent prefetch. */
|
|
linkHandlers?: Partial<IssueNavigationPrefetchHandlers>;
|
|
}
|
|
|
|
export function SidebarNavItem({
|
|
to,
|
|
label,
|
|
icon: Icon,
|
|
iconNode,
|
|
end,
|
|
className,
|
|
labelClassName,
|
|
badge,
|
|
badgeTone = "default",
|
|
badgeLabel,
|
|
textBadge,
|
|
textBadgeTone = "default",
|
|
alert = false,
|
|
liveCount,
|
|
active,
|
|
trailing,
|
|
trailingLabel,
|
|
liveAccessory,
|
|
linkHandlers,
|
|
}: SidebarNavItemProps) {
|
|
const { isMobile, setSidebarOpen, collapsed, peeking } = useSidebar();
|
|
// A contextual takeover forces full labels even when the saved global app
|
|
// sidebar preference is collapsed.
|
|
const forceExpanded = useSidebarNavExpanded();
|
|
// The icon-only rail presentation only applies when pinned collapsed and not
|
|
// peeking; a peek/expanded panel — or a contextual takeover — restores
|
|
// the full label + badge.
|
|
const rail = collapsed && !peeking && !forceExpanded;
|
|
|
|
const hasBadge = badge != null && badge > 0;
|
|
const hasLive = liveCount != null && liveCount > 0;
|
|
const showIconSlot = Boolean(iconNode || Icon || alert || (rail && (hasLive || hasBadge)));
|
|
|
|
// Accessible text equivalent for the collapsed dot indicator. The visible
|
|
// label is `sr-only` in the rail, so the count must be surfaced here.
|
|
const railStatusText = hasLive
|
|
? `${liveCount} live`
|
|
: hasBadge
|
|
? `${badge}${badgeLabel ? ` ${badgeLabel}` : ""}`
|
|
: alert
|
|
? "attention needed"
|
|
: undefined;
|
|
const railAriaLabel = !rail || (!railStatusText && !trailingLabel)
|
|
? undefined
|
|
: `${label}${railStatusText ? `, ${railStatusText}` : ""}${trailingLabel ? `, ${trailingLabel}` : ""}`;
|
|
|
|
const link = (
|
|
<NavLink
|
|
to={to}
|
|
state={SIDEBAR_SCROLL_RESET_STATE}
|
|
end={end}
|
|
aria-label={railAriaLabel}
|
|
{...linkHandlers}
|
|
onClick={() => { if (isMobile) setSidebarOpen(false); }}
|
|
className={({ isActive }) =>
|
|
cn(
|
|
// One rhythm and one inset pill highlight: mx-2 floats the row off
|
|
// the sidebar edges, rounded-lg matches the card anchor, px-2 gives
|
|
// the icon breathing room inside the pill. Rows with hover menus
|
|
// (agents/projects) reserve extra right padding via className.
|
|
"flex items-center gap-2.5 mx-2 rounded-lg px-2 py-1.5 pointer-coarse:py-1 text-left text-(length:--text-compact) font-medium transition-colors",
|
|
(active ?? isActive)
|
|
? "bg-sidebar-accent text-sidebar-accent-foreground"
|
|
: "text-foreground/80 hover:bg-sidebar-accent hover:text-sidebar-accent-foreground",
|
|
className,
|
|
)
|
|
}
|
|
>
|
|
{showIconSlot ? (
|
|
<span data-slot="sidebar-nav-icon" className="relative flex size-4 shrink-0 items-center justify-center">
|
|
{iconNode ?? (Icon ? <Icon className="h-4 w-4" /> : null)}
|
|
{alert && (
|
|
<span
|
|
data-slot="sidebar-icon-alert-badge"
|
|
className="absolute -right-0.5 -top-0.5 h-2 w-2 rounded-full bg-red-500 shadow-(--shadow-sidebar-icon-badge)"
|
|
aria-hidden="true"
|
|
/>
|
|
)}
|
|
{/* Collapsed rail: numeric badge / live count collapse to a dot on the
|
|
icon. The icon markup is untouched so it stays pixel-aligned. */}
|
|
{rail && !alert && hasLive && (
|
|
<span className="absolute -right-0.5 -top-0.5 flex h-2 w-2" aria-hidden="true">
|
|
<span className="animate-pulse absolute inline-flex h-full w-full rounded-full bg-blue-600 dark:bg-blue-400 opacity-75" />
|
|
<span className="relative inline-flex h-2 w-2 rounded-full bg-blue-600 dark:bg-blue-400 shadow-(--shadow-sidebar-icon-badge)" />
|
|
</span>
|
|
)}
|
|
{rail && !alert && !hasLive && hasBadge && (
|
|
<span
|
|
className={cn(
|
|
"absolute -right-0.5 -top-0.5 h-2 w-2 rounded-full shadow-(--shadow-sidebar-icon-badge)",
|
|
badgeTone === "danger"
|
|
? "bg-red-600"
|
|
: badgeTone === "warning"
|
|
? "bg-amber-500"
|
|
: "bg-primary",
|
|
)}
|
|
aria-hidden="true"
|
|
/>
|
|
)}
|
|
</span>
|
|
) : null}
|
|
<TruncatedText className={rail ? SIDEBAR_RAIL_HIDDEN_LABEL : cn("min-w-0 flex-1 truncate", labelClassName)}>{label}</TruncatedText>
|
|
{!rail && trailing}
|
|
{!rail && textBadge && (
|
|
<Badge variant="ghost"
|
|
className={cn(
|
|
"ml-auto px-1.5 text-(length:--text-nano) leading-none",
|
|
textBadgeTone === "amber"
|
|
? "bg-amber-100 text-amber-800 dark:bg-amber-900/30 dark:text-amber-400"
|
|
: "bg-muted text-muted-foreground",
|
|
)}
|
|
>
|
|
{textBadge}
|
|
</Badge>
|
|
)}
|
|
{!rail && (hasLive || liveAccessory) && (
|
|
<span className="ml-auto flex shrink-0 items-center gap-1.5 whitespace-nowrap">
|
|
{liveAccessory}
|
|
{hasLive && (
|
|
<>
|
|
<span className="relative flex h-2 w-2">
|
|
<span className="animate-pulse absolute inline-flex h-full w-full rounded-full bg-blue-600 dark:bg-blue-400 opacity-75" />
|
|
<span className="relative inline-flex rounded-full h-2 w-2 bg-blue-600 dark:bg-blue-400" />
|
|
</span>
|
|
<span className="text-(length:--text-micro) font-medium text-blue-600 dark:text-blue-400">{liveCount} live</span>
|
|
</>
|
|
)}
|
|
</span>
|
|
)}
|
|
{!rail && hasBadge && (
|
|
<Badge variant="ghost"
|
|
aria-label={badgeLabel ? `${badge} ${badgeLabel}` : undefined}
|
|
className={cn(
|
|
"ml-auto px-1.5 leading-none",
|
|
badgeTone === "danger"
|
|
? "bg-red-600/90 text-red-50"
|
|
: badgeTone === "warning"
|
|
? "bg-amber-500/90 text-amber-50"
|
|
: "bg-primary text-primary-foreground",
|
|
)}
|
|
>
|
|
{badge}
|
|
</Badge>
|
|
)}
|
|
</NavLink>
|
|
);
|
|
|
|
if (!rail) return link;
|
|
|
|
// The tooltip wraps a plain block element rather than the NavLink directly:
|
|
// Radix `asChild` (Slot) drops React Router's *function* className, which would
|
|
// strip `flex` off the <a> and render it as a block — the in-flow label would
|
|
// then stack under the icon and the row would grow. Anchoring the tooltip to a
|
|
// wrapper keeps the <a> rendering normally (flex), so the row stays 1:1 with
|
|
// the expanded state and the icon never moves (PAP-10676).
|
|
return (
|
|
<Tooltip>
|
|
<TooltipTrigger asChild>
|
|
<div>{link}</div>
|
|
</TooltipTrigger>
|
|
<TooltipContent side="right">{label}</TooltipContent>
|
|
</Tooltip>
|
|
);
|
|
}
|