feat: add recent activity hover popover - #3874
Conversation
|
😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details. |
|
React Doctor found 1 issue in 1 file · 1 warning. 1 warning
Reviewed by React Doctor for commit |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 1 consider. Published 2 findings (view the review). |
|
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}
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 2 issues
Files (4)
packages/ui/src/features/canvas/components/ActivityHoverCard.tsxpackages/ui/src/features/canvas/components/ActivityView.tsxpackages/ui/src/features/canvas/components/ChannelNav.tsxpackages/ui/src/features/sidebar/components/items/ActivityItem.tsx
k11kirky
left a comment
k11kirky
left a comment
There was a problem hiding this comment.
Automated review pass (8 findings, verified against the real @base-ui/react 1.3.0 stack where behaviour was in question). Ranked most-severe first as inline comments.
The two highest-impact items are functional regressions in the trigger wiring: disabled={isActivity} genuinely disables the nav bell, and the sidebar trigger has no isActive guard so a plain click both navigates and pops the card open. Both reproduced empirically rather than inferred.
Also checked and refuted (no action needed): the unreadItems.length === unreadCount label logic (both are unread-task counts and the optimistic update decrements them in lockstep — hasNextPage would be strictly worse); the pr-14 "dead click strip" (it's padding inside the button, and the overlay is pointer-events-none, so clicks land fine); and import { ActivityHoverCard } from "./ActivityHoverCard" (AGENTS.md prohibits deep relative imports — same-directory ./ is established practice in that folder).
One lower-severity item not worth its own thread: the view_activity mount effect fires on every 300ms hover-open (both call sites conditionally mount the card), merging cheap hovers into an existing metric that only surface distinguishes. I searched project 2's saved insights and found no currently-broken chart, so it's a trap for future queries rather than a live break — but worth a surface filter in any dashboard that uses it.
Created with PostHog Code
| openOnHover | ||
| delay={300} | ||
| closeDelay={100} | ||
| disabled={isActivity} |
There was a problem hiding this comment.
disabled reaches the DOM and genuinely disables the Activity bell.
Base UI's PopoverTrigger defaults nativeButton = true and passes disabled through useButton → useFocusableWhenDisabled, which sets additionalProps.disabled = disabled; useRenderElement then merges it into the render element. NavButton destructures only icon, label, isActive, onClick, badge, className, ref, so disabled stays in ...buttonProps and is spread onto <button>.
On the Activity view the bell drops out of the tab order and its onClick (track + navigateToActivity) can never fire — unlike Inbox and Command Center, which stay clickable while active.
It's also redundant: open={!isActivity && activityOpen} plus the onOpenChange guard already make the popover unopenable here. Deleting this line loses nothing and restores a clickable, tabbable bell.
There was a problem hiding this comment.
Fixed: removed the disabled prop; the Activity bell stays clickable and tabbable while the open-state guard suppresses the popover.
| if (isActive) return item; | ||
|
|
||
| return ( | ||
| <Popover open={open} onOpenChange={setOpen}> |
There was a problem hiding this comment.
No isActive guard on open/onOpenChange, so a plain click both navigates and toggles the card open over the page it just navigated to.
Reproduced against the real quill / @base-ui/react 1.3.0 stack: a single click with no hover yields navCalls=1 and the hover card present, trigger at aria-expanded="true".
useClick is enabled unconditionally on the trigger (openOnHover only adds a separate hover interaction), and nothing stops propagation between SidebarItem's inner <button> and the trigger — so setOpen(false); onClick() runs, then the trigger toggles the closed popover open.
isActive derives from async router state (navigateToActivity is a fire-and-forget router.navigate), so React commits open=true before navigation lands and the 380px card flashes over the Activity page. if (isActive) return item only rescues this once the route commits.
ChannelNav guards both directions explicitly; this call site has neither.
There was a problem hiding this comment.
Fixed: click-driven opening is suppressed, so clicking navigates without flashing the hover card.
| /> | ||
| ))} | ||
| {hasNextPage && ( | ||
| <div ref={loadMoreRef} className="flex h-8 justify-center py-2"> |
There was a problem hiding this comment.
Conditionally-mounted sentinel + mount-only observer = auto-pagination permanently dead for that popover instance.
useInView does const el = ref.current; if (!el) return; observer.observe(el) with deps [rootMargin, once] — no callback ref, no re-run when the node appears later. This sentinel only renders under items.length > 0 && hasNextPage.
Open the card while the first useTaskActivity page is in flight (cold start, slow network), or before the auth client resolves (enabled: !!client → isLoading false with zero items, so the empty branch renders): loadMoreRef.current is null, the observer is never created, loadMoreInView stays false forever, and scrolling the 480px list never calls fetchNextPage.
The new test can't catch this — ActivityHoverCard.test.tsx:46 mocks useInView to [{ current: null }, true], stubbing out the exact wiring that's broken. The other useInView call sites (ChannelFeedView, WebsiteDashboardsIndex) attach the ref to an unconditionally rendered element; doing the same here (render the sentinel container always, gate only the spinner) fixes it.
There was a problem hiding this comment.
Fixed: the sentinel is always mounted, and the observer reconnects when its element appears.
| openOnHover | ||
| delay={300} | ||
| closeDelay={100} | ||
| render={<div className="w-full">{item}</div>} |
There was a problem hiding this comment.
render={<div>} without nativeButton={false} → invalid type on a div, popup ARIA on a roleless wrapper, a dead extra tab stop, and no keyboard path to the card.
Verified by rendering this markup against @posthog/quill@0.3.0-beta.24 / @base-ui/react@1.3.0. Emitted DOM:
<div class="w-full" type="button" tabindex="0" aria-haspopup="dialog" aria-expanded="false">
<button type="button" tabindex="0">…Base UI also logs a dev-mode error: "A component that acts as a button expected a native <button> because the nativeButton prop is true… Use a real <button> in the render prop, or set nativeButton to false."
Focusing the div and dispatching Enter and Space gives navCalls=0 hovercard=false, because getButtonProps' key synthesis is gated on !isNativeButton. Net: a keyboard or screen-reader user gets two tab stops for one control, is told a popup exists on the one that can't open it, and can never reach the hover card.
ChannelNav's sibling trigger renders a real <button> and is unaffected. Fix is either nativeButton={false} or making the SidebarItem button itself the trigger.
There was a problem hiding this comment.
Fixed: SidebarItem is now the native popover trigger, removing the invalid wrapper and duplicate tab stop.
|
|
||
| return ( | ||
| <PopoverContent | ||
| side="right" |
There was a problem hiding this comment.
Hardcoded side="right" makes the 380px panel cover the Command Center icon on the horizontal nav rail.
ChannelNav renders <div className="flex shrink-0 gap-2 px-2 pt-2 pb-1"> with three size-8 buttons in order Inbox, Activity, Command Center. gap-2 is 8px and sideOffset={8}, so the panel's left edge lands exactly on the Lightning button's left edge and its 380px width fully covers the Command Center icon and its unread badge while open.
Base UI's positioner only flips on viewport collision, and there's ample room in the left sidebar, so nothing rescues it.
ActivityHoverCard({ onClose }) exposes no side/align escape hatch, so the two call sites — which genuinely want different placements (side="right" is correct for the vertical sidebar row) — can't differ. Worth lifting placement to a prop, or to the caller's PopoverContent.
There was a problem hiding this comment.
Fixed: the top rail opens below the bell, while the vertical sidebar opens to the right.
| delay={300} | ||
| closeDelay={100} | ||
| disabled={isActivity} | ||
| render={ |
There was a problem hiding this comment.
The Activity bell silently loses its tooltip.
NavIcon wraps every icon in <Tooltip content={label} shortcut={shortcut} side="bottom">, and Inbox and Command Center still go through it. This branch calls NavButton directly from render, and NavButton only emits <button aria-label={label}> — Tooltip is now used solely by NavIcon.
Hovering the bell on the Activity view shows nothing at all (the popover is suppressed there); off it, the icon-only bell has no name until a 380px panel opens after 300ms. The aria-label survives so the a11y name is intact, but the visual affordance is gone and the rail is inconsistent.
(The old Activity NavIcon passed no shortcut, so no keyboard hint is lost — just the label.)
There was a problem hiding this comment.
Fixed: restored the Activity tooltip, including on the Activity page.
| surface: "activity_panel", | ||
| }); | ||
| }, []); | ||
| useEffect(() => { |
There was a problem hiding this comment.
isFetchingNextPage in the deps makes each settled page re-trigger the fetch, chaining unrequested pages on one 300ms hover.
useInView defaults once = false and keeps inView true while intersecting, so isFetchingNextPage flipping true→false re-runs this effect and fires fetchNextPage() again — no scroll, no click. Compact rows are ~44-46px so ~11 fill the 480px container; if the backend page size is below that, the chain continues until hasNextPage is false and one hover pulls the entire feed sequentially. Bounded above that (overflow clipping empties the sentinel's intersection rect once rows exceed 480px), so a single spurious extra page per hover is the common case.
I couldn't confirm the server page size — getTaskActivity sends no limit and the default lives in the backend repo — so flagging this as plausible rather than certain. Worth checking against the real page size.
Separately: rootMargin: "100px 0px" is inert here. useInView sets no IntersectionObserver root, so the margin expands the viewport rect, not this popover's overflow-y-auto clip — there's no early prefetch, just a spinner stall at the bottom. The pre-existing ActivityView deliberately used an explicit "Load more" button instead.
There was a problem hiding this comment.
Fixed: pagination observes the popover scroll container and no longer retriggers when a request settles.
| import { useEffect, useMemo } from "react"; | ||
|
|
||
| export function ActivityHoverCard({ onClose }: { onClose: () => void }) { | ||
| const client = useOptionalAuthenticatedClient(); |
There was a problem hiding this comment.
The whole activity-feed shell is duplicated from ActivityView, and the two copies have already diverged.
Lines 16-61 mirror ActivityView.tsx:258-296: the same useTaskActivity / useMarkTaskActivityRead / useChannels destructuring, an identical new Map(channels.map(c => [normalizeChannelName(c.name), c.id])), hand-built { task_id, seen_before } payloads, and the same mount-effect track(CHANNEL_ACTION, { action_type: "view_activity" }).
They already differ in a way that's a live bug: this popover relabels its button "Mark visible as read" when loaded unread rows don't cover unreadCount, while the page unconditionally says "Mark all as read" from the same partially-loaded items — so the page's label is now wrong and there's no single place to fix it.
The empty state here is also a hand-rolled div where ActivityView uses quill's <Empty>, which AGENTS.md:235 requires: "Empty/placeholder/loading screens (canvas and elsewhere) are a @posthog/quill <Empty> … Don't hand-roll the centered Flex + dashed icon box." Same PR, two different empty states for the same data.
Extracting a shared useActivityFeed(surface) + ActivityList would remove ~35 duplicated lines and leave each surface only its chrome.
There was a problem hiding this comment.
Fixed: both surfaces now share unread payload, label, and channel-lookup helpers; the popover also uses Quill Empty components.
6ef8485 to
b3b9f68
Compare
Generated-By: PostHog Code Task-Id: 4a1c53d9-0ca7-4ebe-9936-0c6dab92ca96
Generated-By: PostHog Code Task-Id: 4a1c53d9-0ca7-4ebe-9936-0c6dab92ca96
Show recent activity from the shared feed cache and batch unread rows through the existing mark-read endpoint. Update the popover layout to a compact Slack-inspired activity list. Generated-By: PostHog Code Task-Id: 4a1c53d9-0ca7-4ebe-9936-0c6dab92ca96
Use a compact row layout with a fixed top-right timestamp and visible hover state. Close promptly on pointer leave and disable the popover while Activity is already open. Generated-By: PostHog Code Task-Id: 4a1c53d9-0ca7-4ebe-9936-0c6dab92ca96
Label partial read cleanup accurately when unloaded activity remains, preserve pointer travel into the card, and migrate the trigger button to React 19 ref handling. Generated-By: PostHog Code Task-Id: 4a1c53d9-0ca7-4ebe-9936-0c6dab92ca96
Fetch the next activity cursor page when a bottom sentinel enters view and show a compact loading indicator while it loads. Generated-By: PostHog Code Task-Id: 4a1c53d9-0ca7-4ebe-9936-0c6dab92ca96
Generated-By: PostHog Code Task-Id: 4a1c53d9-0ca7-4ebe-9936-0c6dab92ca96
b3b9f68 to
7e5c21a
Compare
Generated-By: PostHog Code Task-Id: 4a1c53d9-0ca7-4ebe-9936-0c6dab92ca96
2 participants
Problem
Recent task activity takes a full-page navigation to review.
Why: People should be able to quickly scan recent updates from the activity bell without leaving their current view.
Changes
How did you test this?
Automatic notifications
Created with PostHog Code