diff --git a/tests/web/comments-viewport-resize.spec.ts b/tests/web/comments-viewport-resize.spec.ts index 332722d..2c9f599 100644 --- a/tests/web/comments-viewport-resize.spec.ts +++ b/tests/web/comments-viewport-resize.spec.ts @@ -29,10 +29,11 @@ async function bootAs(page: Page, user: string): Promise { intervals: [1000], }) .toMatch(TITLE); - await openOverlayMenu(page); // the comment bar lives in the overlay menu (0010) - await expect(page.getByTestId('comment-bar-toggle')).toBeVisible({ timeout: 30000 }); - await page.getByTestId('comment-bar-toggle').click(); - await expect(page.getByTestId('comment-mode-toggle')).toBeVisible(); + // The comment controls live in the overlay menu's "Comments" section and are + // visible as soon as it opens — the nested expand toggle (comment-bar-toggle) + // is gone, since the section itself is the group. + await openOverlayMenu(page); // the comment controls live in the overlay menu (0010) + await expect(page.getByTestId('comment-mode-toggle')).toBeVisible({ timeout: 30000 }); } /** Comments persist in the room's ydoc — start each run from a clean slate. */ diff --git a/tests/web/comments.spec.ts b/tests/web/comments.spec.ts index a90d483..ac6fe5f 100644 --- a/tests/web/comments.spec.ts +++ b/tests/web/comments.spec.ts @@ -27,12 +27,12 @@ async function bootAs(page: Page, user: string): Promise { intervals: [1000], }) .toMatch(TITLE); - // The comment controls mount once the collab session + bridge are up; the - // action buttons live inside the expandable bar — open it for the test. - await openOverlayMenu(page); // the comment bar lives in the overlay menu (0010) - await expect(page.getByTestId('comment-bar-toggle')).toBeVisible({ timeout: 30000 }); - await page.getByTestId('comment-bar-toggle').click(); - await expect(page.getByTestId('comment-mode-toggle')).toBeVisible(); + // The comment controls mount once the collab session + bridge are up. They + // live in the overlay menu's "Comments" section and are visible as soon as it + // opens — the old nested expand toggle (comment-bar-toggle) is gone, since the + // section itself is the group. + await openOverlayMenu(page); // the comment controls live in the overlay menu (0010) + await expect(page.getByTestId('comment-mode-toggle')).toBeVisible({ timeout: 30000 }); } /** Delete every leftover thread from previous runs — comments PERSIST in the diff --git a/web/pcbjam-shared b/web/pcbjam-shared index e044c86..10ef7c1 160000 --- a/web/pcbjam-shared +++ b/web/pcbjam-shared @@ -1 +1 @@ -Subproject commit e044c86ea538a1ec6dd2120d1f85fcf9da6b8aa2 +Subproject commit 10ef7c1a0278970277cfb67c7b0360e3f1d03ce9 diff --git a/web/standalone/src/components/CommentLayer.tsx b/web/standalone/src/components/CommentLayer.tsx index 1010857..f102067 100644 --- a/web/standalone/src/components/CommentLayer.tsx +++ b/web/standalone/src/components/CommentLayer.tsx @@ -1,7 +1,8 @@ import * as React from "react"; import { createPortal } from "react-dom"; +import { overlayRowClass } from "@/components/OverlayMenu"; import type { CommentAnchor } from "@pcbjam/shared"; -import { Eye, EyeOff, List, MessageSquarePlus, MessageSquareText, X } from "lucide-react"; +import { Eye, EyeOff, List, MessageSquarePlus, X } from "lucide-react"; import { screenToWorld, worldToScreen, @@ -29,6 +30,28 @@ interface CssRect { height: number; } + +/** + * What to show for a comment author, and what to reveal on hover. + * + * `author`/`createdBy` is the SLUG — the identity key used for colors and + * ownership. It is what used to be rendered, which is why comments showed a + * scope-looking string instead of a person. Prefer the denormalized display + * name; legacy messages (written before authorName existed) have none and fall + * back to the slug, so old threads keep working. + */ +function authorLabel(a: { author?: string; createdBy?: string; authorName?: string; authorEmail?: string }): { + text: string; + title: string; +} { + const slug = a.author ?? a.createdBy ?? ""; + const text = a.authorName || slug; + // Tooltip: email when we captured one, otherwise the slug — always something + // more identifying than the label itself. + const title = a.authorEmail ? `${text} <${a.authorEmail}>` : slug; + return { text, title }; +} + function glCanvasRect(): CssRect | null { const el = Array.from(document.querySelectorAll('[id^="glcanvas-"]')).find((c) => { const r = (c as HTMLElement).getBoundingClientRect(); @@ -66,7 +89,6 @@ export function CommentLayer({ menuSlot: HTMLElement | null; }) { const [threads, setThreads] = React.useState(controller.threads()); - const [barOpen, setBarOpen] = React.useState(false); const [mode, setMode] = React.useState(false); const [openId, setOpenId] = React.useState(null); const [panel, setPanel] = React.useState(false); @@ -211,54 +233,57 @@ export function CommentLayer({ const menuUi = menuSlot ? createPortal( <> -
+ {/* The overlay menu's "Comments" section IS the group, so there is no + nested open/close toggle here any more — that was a collapsible + inside a collapsible. And every action is a LABELLED row: the old + bar was four bare icons whose meanings you had to hover to learn. */} +
+ + + + - {barOpen && ( -
- - - -
- )}
{/* Threads panel (filter + jump-to). */} @@ -294,8 +319,9 @@ export function CommentLayer({ - {t.createdBy} + {authorLabel(t).text} {" "} {timeAgo(t.createdAt)} ago{t.resolved ? " · resolved" : ""} @@ -336,7 +362,7 @@ export function CommentLayer({ key={t.id} data-testid="comment-pin" data-thread-id={t.id} - title={`${t.createdBy}: ${t.messages[0]?.body ?? ""}${t.detached ? " (detached)" : ""} — drag to move`} + title={`${authorLabel(t).text}: ${t.messages[0]?.body ?? ""}${t.detached ? " (detached)" : ""} — drag to move`} onPointerDown={onPinPointerDown(t)} onPointerMove={onPinPointerMove} onPointerUp={onPinPointerUp(t)} @@ -448,10 +474,15 @@ function ThreadPopover({ } }; + // z-[60] beats the overlay menu's z-50 ON PURPOSE: the menu panel is tall + // enough to cover a pin popover, and a popover can be opened FROM the menu + // (the thread list's jump-to), so the menu would otherwise swallow clicks on + // the popover it just spawned. The focused surface wins; the menu stays one + // click-away from dismissal. return (
(
- - {m.author} + + {authorLabel(m).text} {timeAgo(m.createdAt)} ago{m.editedAt ? " · edited" : ""} diff --git a/web/standalone/src/components/OverlayMenu.tsx b/web/standalone/src/components/OverlayMenu.tsx index c11fa5d..375f135 100644 --- a/web/standalone/src/components/OverlayMenu.tsx +++ b/web/standalone/src/components/OverlayMenu.tsx @@ -13,8 +13,48 @@ import { Users } from "lucide-react"; * toasts) by decision: it is trivially dismissed (click-away, Esc, the FAB) * and can be dragged out of the way. Stays up in chrome-hidden mode — it is * the canvas-only survivor the chrome toggle used to be. + * + * VISUAL SYSTEM. The panel previously stacked whatever its children happened to + * look like — free-floating pills of different heights, radii and surfaces, all + * left-aligned with a gap. It read as debris rather than a menu. The primitives + * below are the fix and the contract: + * + * OverlayMenuSection — a labelled group, separated from its neighbours. + * overlayRowClass — the shared row shape for anything interactive. + * + * Children compose these instead of inventing their own chrome. Two deliberate + * exceptions stay self-styled because they are shared with light-background + * pages (SourceChip) or own a nontrivial internal layout (PresenceRoster) — + * those are wrapped in a section rather than restyled. */ +/** Row shape for any interactive item in the panel. Full-width so the panel + * reads as a list; the hover/active states are the only affordance needed. */ +export const overlayRowClass = + "flex w-full items-center gap-2 rounded-md px-2 py-1.5 text-left text-xs " + + "text-white/90 transition-colors hover:bg-white/10 " + + "focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-white/40"; + +/** A labelled group. `label` is omitted for the first/unnamed group. */ +export function OverlayMenuSection({ + label, + children, +}: { + label?: string; + children: React.ReactNode; +}) { + return ( +
+ {label && ( +
+ {label} +
+ )} + {children} +
+ ); +} + const POS_KEY = "pcbjam:overlay-menu-pos"; const FAB_SIZE = 36; const DRAG_THRESHOLD_PX = 4; @@ -156,8 +196,10 @@ export function OverlayMenu({ onPointerDown={onFabPointerDown} onPointerMove={onFabPointerMove} onPointerUp={onFabPointerUp} - className={`relative flex h-9 w-9 items-center justify-center rounded-full shadow-md ring-1 ring-inset ring-white/25 ${ - open ? "bg-sky-600 text-white" : "bg-black/75 text-white hover:bg-black/90" + className={`relative flex h-9 w-9 items-center justify-center rounded-full text-white shadow-lg ring-1 ring-inset transition-colors ${ + open + ? "bg-sky-600 ring-sky-300/40" + : "bg-neutral-950/80 ring-white/15 backdrop-blur-sm hover:bg-neutral-900/90" }`} style={{ touchAction: "none" }} > @@ -175,10 +217,20 @@ export function OverlayMenu({ {open && (
+
+ + Session + + {badge > 0 && ( + + {badge} {badge === 1 ? "other" : "others"} here + + )} +
{children}
)} diff --git a/web/standalone/src/components/PresenceRoster.tsx b/web/standalone/src/components/PresenceRoster.tsx index 92fb812..71b1e44 100644 --- a/web/standalone/src/components/PresenceRoster.tsx +++ b/web/standalone/src/components/PresenceRoster.tsx @@ -1,21 +1,28 @@ +import { Eye } from "lucide-react"; import type { PresencePeer } from "@/wasm/collab/presence"; import type { FollowTarget } from "@/wasm/collab/follow-user"; +import { overlayRowClass } from "@/components/OverlayMenu"; /** - * "Who else is in this file" (collab-presence 0001/0003): a compact facepile of - * the room's OTHER users, one colored initial-avatar per person, fed by the - * collab session's awareness. For eeschema (per-sheet rooms + warm-pool - * skeleton states) peers on a DIFFERENT sheet render dimmed, with the sheet - * they're on in the tooltip. Rendered in the editor's top-right overlay stack - * next to SourceChip; the parent hides it when there are no peers. Chip styling - * mirrors SourceChip (solid fill + inset ring) so it is legible on any backdrop. + * "Who else is in this file" (collab-presence 0001/0003), as a readable LIST. * - * Follow-user (0008): when `onFollow` is provided, clicking a same-sheet - * peer's avatar follows their viewport (click again to stop); the followed - * peer gets a ring in their color. The parent renders the "Following…" - * banner — the roster only toggles. + * This used to be a facepile — overlapping 20px circles showing one initial + * each. That is a fine density trade in a cramped toolbar, but it lives in the + * overlay menu now (0010), which is 288px wide: there is room for names, and a + * row of colored initials told you neither who was here nor that clicking one + * followed them. One row per person instead: color dot, name, where they are, + * and an explicit follow affordance. + * + * For eeschema (per-sheet rooms) peers on a DIFFERENT sheet are dimmed, say + * which sheet they're on, and are not followable — following someone you cannot + * see would just teleport you. + * + * Follow-user (0008): clicking a same-sheet peer follows their viewport; the + * followed peer's row stays highlighted with a "Stop" action, so the state and + * its exit live on the same row (the parent no longer renders a separate + * "Following…" banner). */ -const MAX_AVATARS = 5; +const MAX_ROWS = 6; function sheetLabel(sheetPath?: string): string { if (!sheetPath) return ""; @@ -42,13 +49,10 @@ export function PresenceRoster({ const sameSheet = (p: PresencePeer) => (p.sheetPath ?? undefined) === (activeSheetPath ?? undefined); // Same-sheet peers first, then elsewhere (dimmed) — stable within each group. - const ordered = [...peers].sort((a, b) => Number(sameSheet(b)) - Number(sameSheet(a))); - const names = ordered - .map((p) => - sameSheet(p) ? p.user.name : `${p.user.name} (on ${sheetLabel(p.sheetPath) || "another sheet"})`, - ) - .join(", "); - const shown = ordered.slice(0, MAX_AVATARS); + const ordered = [...peers].sort( + (a, b) => Number(sameSheet(b)) - Number(sameSheet(a)), + ); + const shown = ordered.slice(0, MAX_ROWS); const followable = (p: PresencePeer) => !!onFollow && sameSheet(p); const isFollowed = (p: PresencePeer) => following?.clientId === p.clientId; @@ -56,46 +60,69 @@ export function PresenceRoster({ const toggleFollow = (p: PresencePeer) => { if (!onFollow) return; onFollow( - isFollowed(p) ? null : { clientId: p.clientId, userId: p.user.id, name: p.user.name }, + isFollowed(p) + ? null + : { clientId: p.clientId, userId: p.user.id, name: p.user.name }, ); }; return ( - - {shown.map((p) => ( - - ))} - {peers.length > MAX_AVATARS && ( - - +{peers.length - MAX_AVATARS} +
+ {shown.map((p) => { + const here = sameSheet(p); + const followed = isFollowed(p); + const elsewhere = sheetLabel(p.sheetPath) || "another sheet"; + return ( + + ); + })} + {peers.length > MAX_ROWS && ( + + +{peers.length - MAX_ROWS} more )} - +
); } diff --git a/web/standalone/src/components/SourceChip.tsx b/web/standalone/src/components/SourceChip.tsx index 1fff2e3..d45ca8d 100644 --- a/web/standalone/src/components/SourceChip.tsx +++ b/web/standalone/src/components/SourceChip.tsx @@ -23,14 +23,41 @@ const TONES: Record = { "remote-rw": "bg-sky-600 text-white ring-sky-300/30", }; +// MUTED variant, for use on a known-dark surface (the editor's overlay menu). +// The saturated fills above exist because the chip must survive an unknown +// backdrop; inside the menu the backdrop IS known, and a solid sky/emerald pill +// just shouts. Here the colour drops to a small leading dot and the chip itself +// becomes another neutral row. +const MUTED_TONES: Record = { + local: "text-emerald-300/90", + "remote-ro": "text-amber-300/90", + "remote-rw": "text-sky-300/90", +}; + export function SourceChip({ descriptor, className = "", + tone = "solid", }: { descriptor: SourceDescriptor; className?: string; + /** `muted` for known-dark surfaces (see MUTED_TONES). */ + tone?: "solid" | "muted"; }) { const Icon = ICONS[descriptor.kind]; + + if (tone === "muted") { + return ( + + + {descriptor.label} + + ); + } + return ( + {/* PEOPLE — who else is here, and whose view you're locked to. The + follow state lives on each person's own row (PresenceRoster), so + there is no separate "Following…" banner to keep in sync. */} {peers.length > 0 && ( - { - if (t) followRef.current?.follow(t); - else followRef.current?.unfollow(); - }} - /> + + { + if (t) followRef.current?.follow(t); + else followRef.current?.unfollow(); + }} + /> + )} - {sourceDescriptor && } - {readOnly && ( - - View only - - )} - {followingTarget && ( -
- - Following {followingTarget.name} - - -
+ + {/* DOCUMENT — where this file came from and whether you may edit it. + SourceChip is shared with the light project pages, so instead of + restyling it we ask for its `muted` tone: colour drops to a dot, + and the chip sits in a normal row like everything else. */} + {(sourceDescriptor || readOnly) && ( + + {sourceDescriptor && ( +
+ +
+ )} + {readOnly && ( +
+ + View only + + read-only + +
+ )} + )} + {commentsCtl && ( -
+ +
+ )} + {setChromeFn !== null && !readOnly && ( - + + + )} )} diff --git a/web/standalone/src/lib/config.ts b/web/standalone/src/lib/config.ts index 3b12593..ae61947 100644 --- a/web/standalone/src/lib/config.ts +++ b/web/standalone/src/lib/config.ts @@ -214,6 +214,28 @@ export function presenceUser(): PresenceUser { return { id: slug, name, color: colorForUser(slug) }; } +/** Author identity stamped onto comments. */ +export interface CommentAuthor { + /** Slug — the IDENTITY key (colors, "is this mine?"). Never a display name. */ + id: string; + /** Display name; falls back to the slug when no session identity is loaded. */ + name: string; + /** Only present for a real authenticated session. */ + email?: string; +} + +/** + * Who to attribute a new comment to. Same identity as `presenceUser()`, plus the + * email, and kept separate because comments DENORMALIZE these at write time + * (comments-wire.ts) whereas presence re-broadcasts them live. + */ +export function commentAuthor(): CommentAuthor { + const slug = userSlug(); + const session = sessionIdentity(); + const mine = session && session.slug === slug ? session : null; + return { id: slug, name: mine?.name ?? slug, email: mine?.email }; +} + /** * DEV-TIME presence style tuner (collab-presence): VITE_PRESENCE_TUNER=1 mounts * a floating panel that live-patches the wasm overlay style diff --git a/web/standalone/src/lib/session-identity.ts b/web/standalone/src/lib/session-identity.ts index 0f5a1bf..655e777 100644 --- a/web/standalone/src/lib/session-identity.ts +++ b/web/standalone/src/lib/session-identity.ts @@ -7,7 +7,12 @@ * without the endpoint (example backend, demo/static) simply yield null and * the pre-auth slug fallback in config.ts stays in effect. */ -export type SessionIdentity = { slug: string; name: string }; +export type SessionIdentity = { + slug: string; + name: string; + /** Undefined for backends that don't return one (example/demo/static). */ + email?: string; +}; let identity: SessionIdentity | null = null; let pending: Promise | null = null; @@ -35,12 +40,11 @@ export function loadSessionIdentity( } | null )?.user; if (u && typeof u.slug === "string" && u.slug) { + const email = typeof u.email === "string" && u.email ? u.email : undefined; identity = { slug: u.slug, - name: - (typeof u.name === "string" && u.name) || - (typeof u.email === "string" && u.email) || - u.slug, + name: (typeof u.name === "string" && u.name) || email || u.slug, + email, }; } return identity; diff --git a/web/standalone/src/wasm/collab/comments.ts b/web/standalone/src/wasm/collab/comments.ts index f4f3ce4..7bc5e81 100644 --- a/web/standalone/src/wasm/collab/comments.ts +++ b/web/standalone/src/wasm/collab/comments.ts @@ -115,8 +115,12 @@ const PUSH_THROTTLE_MS = 30; export function createComments(opts: { doc: Y.Doc; mod: CommentPinsModule; - /** Author slug for new messages (presence identity). */ - user: string; + /** + * Author for new messages. `id` is the slug (identity key); `name`/`email` + * are denormalized onto each message at write time so a comment still shows + * a real author when they are offline, renamed, or gone (comments-wire.ts). + */ + user: { id: string; name?: string; email?: string }; tool: string; /** Presence color resolver (nth-in-room); undefined falls back to the hash. */ colorFor?: (userId: string) => string | undefined; @@ -223,10 +227,21 @@ export function createComments(opts: { return { pos: { x: world.x, y: world.y } }; }, create(anchor, body) { - return createThread(doc, { anchor, author: user, body }); + return createThread(doc, { + anchor, + author: user.id, + authorName: user.name, + authorEmail: user.email, + body, + }); }, reply(threadId, body) { - addMessage(doc, threadId, { author: user, body }); + addMessage(doc, threadId, { + author: user.id, + authorName: user.name, + authorEmail: user.email, + body, + }); }, edit(threadId, messageId, body) { return editMessage(doc, threadId, messageId, body);