fix(timeline): stabilize popovers on internal scroll + expand test coverage
B-1: useViewportPopover — ignoreScrollContainers option; scroll events originating inside the timeline canvas no longer close point-anchor popovers B-2: AllocationPopover, DemandPopover, NewAllocationPopover — thread scrollContainerRef through so horizontal timeline scroll is ignored B-3: AllocationPopover — staleTime 0 so SSE reconnect triggers immediate refetch B-4: useViewportPopover.test.ts — 6 new tests (scroll close, ignore container, resize close, style clamping) B-5: AllocationPopover.test.tsx — loading state + happy-path tests added Co-Authored-By: claude-flow <ruv@ruv.net>
This commit is contained in:
@@ -42,6 +42,14 @@ vi.mock("~/hooks/useViewportPopover.js", () => ({
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("~/components/ui/DateInput.js", () => ({
|
||||
DateInput: () => null,
|
||||
}));
|
||||
|
||||
vi.mock("~/components/ui/ProjectCombobox.js", () => ({
|
||||
ProjectCombobox: () => null,
|
||||
}));
|
||||
|
||||
describe("AllocationPopover", () => {
|
||||
beforeEach(() => {
|
||||
mockUseQuery.mockReset();
|
||||
@@ -93,4 +101,67 @@ describe("AllocationPopover", () => {
|
||||
expect(html).toContain("data-testid=\"timeline-allocation-popover-unavailable\"");
|
||||
expect(html).toContain("The selected booking could not be resolved from the current timeline data.");
|
||||
});
|
||||
|
||||
it("renders a loading skeleton when the allocation is being fetched", () => {
|
||||
mockUseQuery.mockReturnValue({
|
||||
data: undefined,
|
||||
isLoading: true,
|
||||
error: null,
|
||||
});
|
||||
|
||||
const html = renderToStaticMarkup(
|
||||
<AllocationPopover
|
||||
allocationId="assignment_loading"
|
||||
projectId="project_1"
|
||||
anchorX={120}
|
||||
anchorY={40}
|
||||
onClose={() => {}}
|
||||
onOpenPanel={() => {}}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(html).toContain("data-testid=\"timeline-allocation-popover-loading\"");
|
||||
});
|
||||
|
||||
it("renders allocation data when provided as initialAllocation", () => {
|
||||
// useQuery is always called (with enabled: false), so we need a baseline return value
|
||||
mockUseQuery.mockReturnValue({
|
||||
data: undefined,
|
||||
isLoading: false,
|
||||
error: null,
|
||||
});
|
||||
|
||||
const allocation = {
|
||||
id: "assignment_1",
|
||||
entityId: "assignment_1",
|
||||
projectId: "project_1",
|
||||
resourceId: "resource_1",
|
||||
startDate: new Date("2026-01-01"),
|
||||
endDate: new Date("2026-01-31"),
|
||||
hoursPerDay: 8,
|
||||
role: "Developer",
|
||||
metadata: null,
|
||||
resource: {
|
||||
displayName: "Alice Smith",
|
||||
eid: "alice.smith",
|
||||
lcrCents: 0,
|
||||
},
|
||||
};
|
||||
|
||||
const html = renderToStaticMarkup(
|
||||
<AllocationPopover
|
||||
allocationId="assignment_1"
|
||||
projectId="project_1"
|
||||
initialAllocation={allocation as any}
|
||||
anchorX={120}
|
||||
anchorY={40}
|
||||
onClose={() => {}}
|
||||
onOpenPanel={() => {}}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(html).not.toContain("data-testid=\"timeline-allocation-popover-error\"");
|
||||
expect(html).not.toContain("data-testid=\"timeline-allocation-popover-unavailable\"");
|
||||
expect(html).not.toContain("data-testid=\"timeline-allocation-popover-loading\"");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
"use client";
|
||||
|
||||
import React from "react";
|
||||
import React, { type RefObject } from "react";
|
||||
import { clsx } from "clsx";
|
||||
import { useEffect, useState } from "react";
|
||||
import { createPortal } from "react-dom";
|
||||
@@ -21,6 +21,7 @@ interface AllocationPopoverProps {
|
||||
anchorX: number;
|
||||
anchorY: number;
|
||||
contextDate?: Date;
|
||||
ignoreScrollContainers?: RefObject<HTMLElement | null>[];
|
||||
}
|
||||
|
||||
type AllocationPopoverAssignment = Assignment<AllocationLike>;
|
||||
@@ -34,6 +35,7 @@ export function AllocationPopover({
|
||||
anchorX,
|
||||
anchorY,
|
||||
contextDate,
|
||||
ignoreScrollContainers,
|
||||
}: AllocationPopoverProps) {
|
||||
const utils = trpc.useUtils();
|
||||
const invalidateTimeline = useInvalidateTimeline();
|
||||
@@ -42,13 +44,14 @@ export function AllocationPopover({
|
||||
width: 300,
|
||||
estimatedHeight: 360,
|
||||
onClose,
|
||||
...(ignoreScrollContainers ? { ignoreScrollContainers } : {}),
|
||||
});
|
||||
|
||||
const shouldLoadAllocation = !initialAllocation;
|
||||
const allocationQuery = trpc.allocation.getAssignmentById.useQuery(
|
||||
{ id: allocationId },
|
||||
{
|
||||
staleTime: 10_000,
|
||||
staleTime: 0,
|
||||
enabled: shouldLoadAllocation,
|
||||
retry: false,
|
||||
},
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
"use client";
|
||||
|
||||
import type { RefObject } from "react";
|
||||
import { createPortal } from "react-dom";
|
||||
import type { TimelineDemandEntry } from "./TimelineContext.js";
|
||||
import { formatCents, formatDateLong } from "~/lib/format.js";
|
||||
@@ -12,6 +13,7 @@ interface DemandPopoverProps {
|
||||
onFillDemand: (demand: TimelineDemandEntry) => void;
|
||||
anchorX: number;
|
||||
anchorY: number;
|
||||
ignoreScrollContainers?: RefObject<HTMLElement | null>[];
|
||||
}
|
||||
|
||||
export function DemandPopover({
|
||||
@@ -21,12 +23,14 @@ export function DemandPopover({
|
||||
onFillDemand,
|
||||
anchorX,
|
||||
anchorY,
|
||||
ignoreScrollContainers,
|
||||
}: DemandPopoverProps) {
|
||||
const { ref, style } = useViewportPopover({
|
||||
anchor: { kind: "point", x: anchorX, y: anchorY },
|
||||
width: 300,
|
||||
estimatedHeight: 340,
|
||||
onClose,
|
||||
...(ignoreScrollContainers ? { ignoreScrollContainers } : {}),
|
||||
});
|
||||
|
||||
const roleName = demand.roleEntity?.name ?? demand.role ?? "Unspecified";
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
"use client";
|
||||
|
||||
import { clsx } from "clsx";
|
||||
import type { RefObject } from "react";
|
||||
import { useState } from "react";
|
||||
import { createPortal } from "react-dom";
|
||||
import { AllocationStatus } from "@capakraken/shared";
|
||||
@@ -20,6 +21,7 @@ interface NewAllocationPopoverProps {
|
||||
anchorY: number;
|
||||
onClose: () => void;
|
||||
onCreated: () => void;
|
||||
ignoreScrollContainers?: RefObject<HTMLElement | null>[];
|
||||
}
|
||||
|
||||
function toDateInput(d: Date): string {
|
||||
@@ -38,6 +40,7 @@ export function NewAllocationPopover({
|
||||
anchorY,
|
||||
onClose,
|
||||
onCreated,
|
||||
ignoreScrollContainers,
|
||||
}: NewAllocationPopoverProps) {
|
||||
const { ref, style } = useViewportPopover({
|
||||
anchor: { kind: "point", x: anchorX - 10, y: anchorY },
|
||||
@@ -45,6 +48,7 @@ export function NewAllocationPopover({
|
||||
estimatedHeight: 440,
|
||||
onClose,
|
||||
ignoreSelectors: ["[data-entity-combobox-overlay='true']"],
|
||||
...(ignoreScrollContainers ? { ignoreScrollContainers } : {}),
|
||||
});
|
||||
const invalidateTimeline = useInvalidateTimeline();
|
||||
|
||||
|
||||
@@ -942,6 +942,7 @@ function TimelineViewContent({
|
||||
}}
|
||||
anchorX={popover.x}
|
||||
anchorY={popover.y}
|
||||
ignoreScrollContainers={[scrollContainerRef]}
|
||||
/>
|
||||
);
|
||||
}
|
||||
@@ -957,6 +958,7 @@ function TimelineViewContent({
|
||||
}}
|
||||
anchorX={popover.x}
|
||||
anchorY={popover.y}
|
||||
ignoreScrollContainers={[scrollContainerRef]}
|
||||
{...(popover.contextDate ? { contextDate: popover.contextDate } : {})}
|
||||
/>
|
||||
);
|
||||
@@ -988,6 +990,7 @@ function TimelineViewContent({
|
||||
}}
|
||||
anchorX={demandPopover.x}
|
||||
anchorY={demandPopover.y}
|
||||
ignoreScrollContainers={[scrollContainerRef]}
|
||||
/>
|
||||
)}
|
||||
|
||||
@@ -1002,6 +1005,7 @@ function TimelineViewContent({
|
||||
anchorY={newAllocPopover.anchorY}
|
||||
onClose={() => setNewAllocPopover(null)}
|
||||
onCreated={() => setNewAllocPopover(null)}
|
||||
ignoreScrollContainers={[scrollContainerRef]}
|
||||
/>
|
||||
)}
|
||||
|
||||
|
||||
@@ -0,0 +1,222 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
// Capture effect callbacks so we can run them manually (same pattern as useTimelineSSE.test.ts)
|
||||
const effectCleanups: Array<() => void> = [];
|
||||
|
||||
vi.mock("react", async () => {
|
||||
const actual = await vi.importActual<typeof import("react")>("react");
|
||||
return {
|
||||
...actual,
|
||||
useEffect: (callback: () => void | (() => void)) => {
|
||||
const cleanup = callback();
|
||||
if (typeof cleanup === "function") {
|
||||
effectCleanups.push(cleanup);
|
||||
}
|
||||
},
|
||||
useRef: <T,>(value: T) => ({ current: value }),
|
||||
useState: <T,>(initializer: T | (() => T)) => {
|
||||
const value = typeof initializer === "function" ? (initializer as () => T)() : initializer;
|
||||
return [value, vi.fn()] as [T, ReturnType<typeof vi.fn>];
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
import { useViewportPopover } from "./useViewportPopover.js";
|
||||
|
||||
// ---- Minimal window event emitter used in tests ----
|
||||
|
||||
type AnyHandler = (event: Event) => void;
|
||||
|
||||
class MockWindow {
|
||||
private readonly listeners = new Map<string, Set<AnyHandler>>();
|
||||
|
||||
addEventListener(type: string, handler: AnyHandler) {
|
||||
if (!this.listeners.has(type)) {
|
||||
this.listeners.set(type, new Set());
|
||||
}
|
||||
this.listeners.get(type)!.add(handler);
|
||||
}
|
||||
|
||||
removeEventListener(type: string, handler: AnyHandler) {
|
||||
this.listeners.get(type)?.delete(handler);
|
||||
}
|
||||
|
||||
emit(type: string, event: Partial<Event> = {}) {
|
||||
for (const handler of this.listeners.get(type) ?? []) {
|
||||
handler(event as Event);
|
||||
}
|
||||
}
|
||||
|
||||
readonly innerWidth = 1280;
|
||||
readonly innerHeight = 800;
|
||||
readonly visualViewport = null;
|
||||
}
|
||||
|
||||
// In the node test environment `Node` is not defined. Provide a stub class so that
|
||||
// `scrollTarget instanceof Node` works as expected in handleScroll.
|
||||
class MockNode {
|
||||
// subclasses can override to signal membership
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a pair of mock Node-like objects where `container.contains(inner)`
|
||||
* returns true but `container.contains(outside)` returns false.
|
||||
* This satisfies the `r.current.contains(scrollTarget)` check in handleScroll
|
||||
* without needing a real DOM.
|
||||
*/
|
||||
function makeMockNodes() {
|
||||
const inner = new MockNode() as unknown as Node;
|
||||
const outside = new MockNode() as unknown as Node;
|
||||
const container = Object.assign(new MockNode(), {
|
||||
contains(node: Node | null) {
|
||||
return node === inner;
|
||||
},
|
||||
}) as unknown as HTMLElement;
|
||||
return { container, inner, outside };
|
||||
}
|
||||
|
||||
describe("useViewportPopover", () => {
|
||||
let mockWindow: MockWindow;
|
||||
let onClose: ReturnType<typeof vi.fn>;
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
effectCleanups.length = 0;
|
||||
|
||||
mockWindow = new MockWindow();
|
||||
onClose = vi.fn();
|
||||
|
||||
// Stub globals needed by useViewportPopover effects
|
||||
vi.stubGlobal("window", mockWindow);
|
||||
vi.stubGlobal("document", {
|
||||
addEventListener: vi.fn(),
|
||||
removeEventListener: vi.fn(),
|
||||
});
|
||||
// `Node` is unavailable in the node test environment; stub it so that
|
||||
// `scrollTarget instanceof Node` works correctly in handleScroll.
|
||||
vi.stubGlobal("Node", MockNode);
|
||||
vi.stubGlobal("ResizeObserver", class {
|
||||
observe() {}
|
||||
disconnect() {}
|
||||
});
|
||||
vi.stubGlobal("requestAnimationFrame", (cb: () => void) => {
|
||||
cb();
|
||||
return 1;
|
||||
});
|
||||
vi.stubGlobal("cancelAnimationFrame", () => {});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
while (effectCleanups.length > 0) {
|
||||
effectCleanups.pop()?.();
|
||||
}
|
||||
vi.unstubAllGlobals();
|
||||
});
|
||||
|
||||
// Test 1: anchor "point" → onClose called on window scroll
|
||||
it("calls onClose when window scroll fires for a point anchor", () => {
|
||||
useViewportPopover({
|
||||
anchor: { kind: "point", x: 100, y: 50 },
|
||||
width: 300,
|
||||
estimatedHeight: 200,
|
||||
onClose,
|
||||
});
|
||||
|
||||
// Fire a scroll event with a non-Node target so the ignore-check is irrelevant
|
||||
mockWindow.emit("scroll", { target: null });
|
||||
|
||||
expect(onClose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
// Test 2: anchor "point" + ignoreScrollContainers → onClose NOT called when scroll target is inside the container
|
||||
it("does NOT call onClose when scroll target is inside an ignored scroll container", () => {
|
||||
const { container, inner } = makeMockNodes();
|
||||
const containerRef = { current: container };
|
||||
|
||||
useViewportPopover({
|
||||
anchor: { kind: "point", x: 100, y: 50 },
|
||||
width: 300,
|
||||
estimatedHeight: 200,
|
||||
onClose,
|
||||
ignoreScrollContainers: [containerRef],
|
||||
});
|
||||
|
||||
// Scroll event whose target is inside the ignored container
|
||||
mockWindow.emit("scroll", { target: inner });
|
||||
|
||||
expect(onClose).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Test 3: anchor "point" + ignoreScrollContainers → onClose IS called when scroll target is outside container
|
||||
it("calls onClose when scroll target is outside the ignored scroll container", () => {
|
||||
const { container, outside } = makeMockNodes();
|
||||
const containerRef = { current: container };
|
||||
|
||||
useViewportPopover({
|
||||
anchor: { kind: "point", x: 100, y: 50 },
|
||||
width: 300,
|
||||
estimatedHeight: 200,
|
||||
onClose,
|
||||
ignoreScrollContainers: [containerRef],
|
||||
});
|
||||
|
||||
// Scroll event whose target is NOT inside the container
|
||||
mockWindow.emit("scroll", { target: outside });
|
||||
|
||||
expect(onClose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
// Test 4: anchor "point" → onClose called on window resize
|
||||
it("calls onClose when window resize fires for a point anchor", () => {
|
||||
useViewportPopover({
|
||||
anchor: { kind: "point", x: 100, y: 50 },
|
||||
width: 300,
|
||||
estimatedHeight: 200,
|
||||
onClose,
|
||||
});
|
||||
|
||||
mockWindow.emit("resize", {});
|
||||
|
||||
expect(onClose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
// Test 5: initial style for anchor "point" with x=100, y=50 → position fixed, left/top set
|
||||
it("computes fixed position style with left and top for a point anchor", () => {
|
||||
const result = useViewportPopover({
|
||||
anchor: { kind: "point", x: 100, y: 50 },
|
||||
width: 300,
|
||||
estimatedHeight: 200,
|
||||
onClose,
|
||||
viewportPadding: 16,
|
||||
});
|
||||
|
||||
const style = result.style as import("react").CSSProperties;
|
||||
|
||||
expect(style.position).toBe("fixed");
|
||||
// anchor.x = 100, align = "start" → raw left = 100
|
||||
// clamped: min(max(100, 16), max(16, 1280-300-16)) = min(100, 964) = 100
|
||||
expect(style.left).toBe(100);
|
||||
// anchor.y = 50, offset = 8 → raw top = 58
|
||||
// clamped: min(max(58, 16), max(16, 800-200-16)) = min(58, 584) = 58
|
||||
expect(style.top).toBe(58);
|
||||
});
|
||||
|
||||
// Test 6: bounds clamping — if anchor x + width > viewport width, left is clamped
|
||||
it("clamps left so the popover stays within viewport bounds", () => {
|
||||
// anchor at x=1250, width=300, innerWidth=1280, padding=16
|
||||
// raw left = 1250, maxLeft = max(16, 1280 - 300 - 16) = 964 → clamped to 964
|
||||
const result = useViewportPopover({
|
||||
anchor: { kind: "point", x: 1250, y: 50 },
|
||||
width: 300,
|
||||
estimatedHeight: 200,
|
||||
onClose,
|
||||
viewportPadding: 16,
|
||||
});
|
||||
|
||||
const style = result.style as import("react").CSSProperties;
|
||||
expect(style.position).toBe("fixed");
|
||||
expect(style.left as number).toBeLessThanOrEqual(1280 - 300 - 16);
|
||||
// Should be clamped to 964
|
||||
expect(style.left).toBe(964);
|
||||
});
|
||||
});
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useEffect, useRef, useState, type CSSProperties } from "react";
|
||||
import React, { useEffect, useRef, useState, type CSSProperties } from "react";
|
||||
|
||||
type PopoverAnchor =
|
||||
| { kind: "point"; x: number; y: number }
|
||||
@@ -18,6 +18,7 @@ interface UseViewportPopoverOptions {
|
||||
viewportPadding?: number;
|
||||
ignoreElements?: Array<HTMLElement | null>;
|
||||
ignoreSelectors?: string[];
|
||||
ignoreScrollContainers?: React.RefObject<HTMLElement | null>[];
|
||||
zIndex?: number;
|
||||
}
|
||||
|
||||
@@ -32,6 +33,7 @@ export function useViewportPopover({
|
||||
viewportPadding = 16,
|
||||
ignoreElements = [],
|
||||
ignoreSelectors = [],
|
||||
ignoreScrollContainers,
|
||||
zIndex = 9998,
|
||||
}: UseViewportPopoverOptions) {
|
||||
const ref = useRef<HTMLDivElement>(null);
|
||||
@@ -182,8 +184,19 @@ export function useViewportPopover({
|
||||
|
||||
updateOrClose();
|
||||
|
||||
const handleScroll = () => {
|
||||
const handleScroll = (event: Event) => {
|
||||
if (closeOnViewportChange) {
|
||||
const scrollTarget = (event as Event).target;
|
||||
if (
|
||||
ignoreScrollContainers?.some(
|
||||
(r) =>
|
||||
r.current != null &&
|
||||
scrollTarget instanceof Node &&
|
||||
r.current.contains(scrollTarget),
|
||||
)
|
||||
) {
|
||||
return;
|
||||
}
|
||||
cancelScheduledFrame();
|
||||
onClose();
|
||||
return;
|
||||
@@ -215,6 +228,7 @@ export function useViewportPopover({
|
||||
align,
|
||||
anchor,
|
||||
estimatedHeight,
|
||||
ignoreScrollContainers,
|
||||
offset,
|
||||
onClose,
|
||||
side,
|
||||
|
||||
Reference in New Issue
Block a user