Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/SessionHoverCard/HoverCardBase.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ const HoverCardTrigger: React.FC<HoverCardTriggerProps> = ({
[originalRef]
);

// eslint-disable-next-line react-hooks/refs
// eslint-disable-next-line react-hooks/refs -- cloneElement only forwards the composed callback ref for React to invoke during commit; it never reads ref.current during render
return cloneElement(children, {
ref: composedRef,
onMouseEnter: (event: React.MouseEvent) => {
Expand Down
2 changes: 1 addition & 1 deletion src/components/Table/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ function TableComponent<T = unknown>(

const tanstackColumns = useTableColumns(columns, rowSelection);

// eslint-disable-next-line react-hooks/incompatible-library
// eslint-disable-next-line react-hooks/incompatible-library -- TanStack Table returns imperative helpers that React Compiler cannot memoize safely; keep this component outside compiler memoization
const table = useReactTable({
data,
columns: tanstackColumns,
Expand Down
51 changes: 51 additions & 0 deletions src/components/Tooltip/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// @vitest-environment jsdom
import { type ComponentProps, act, createElement } from "react";
import { type Root, createRoot } from "react-dom/client";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";

import Tooltip from ".";

const reactActEnvironment = globalThis as typeof globalThis & {
IS_REACT_ACT_ENVIRONMENT?: boolean;
};
let container: HTMLDivElement;
let root: Root;

beforeEach(() => {
reactActEnvironment.IS_REACT_ACT_ENVIRONMENT = true;
container = document.createElement("div");
document.body.appendChild(container);
root = createRoot(container);
});

afterEach(() => {
act(() => root.unmount());
container.remove();
Reflect.deleteProperty(reactActEnvironment, "IS_REACT_ACT_ENVIRONMENT");
});

describe("Tooltip child refs", () => {
it("hands changing callback refs to React without render-driven state churn", () => {
const firstRef = vi.fn();
const secondRef = vi.fn();
const render = (childRef: (node: HTMLButtonElement | null) => void) =>
createElement(
Tooltip,
{ content: "Details" } as ComponentProps<typeof Tooltip>,
createElement("button", { ref: childRef }, "Trigger")
);

act(() => root.render(render(firstRef)));
const button = container.querySelector("button");
expect(firstRef).toHaveBeenLastCalledWith(button);

act(() => root.render(render(secondRef)));
expect(firstRef).toHaveBeenLastCalledWith(null);
expect(secondRef).toHaveBeenLastCalledWith(button);

for (let index = 0; index < 20; index += 1) {
act(() => root.render(render(vi.fn())));
}
expect(container.querySelector("button")).toBe(button);
});
});
89 changes: 47 additions & 42 deletions src/components/Tooltip/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ function applyRef<T>(ref: React.Ref<T> | undefined, value: T | null): void {
(ref as React.MutableRefObject<T | null>).current = value;
}

type TooltipChildProps = {
ref?: React.Ref<HTMLElement>;
onMouseEnter?: (e: React.MouseEvent) => void;
onMouseLeave?: (e: React.MouseEvent) => void;
onClick?: (e: React.MouseEvent) => void;
onFocus?: (e: React.FocusEvent) => void;
onBlur?: (e: React.FocusEvent) => void;
[key: string]: unknown;
};

type TooltipCoordinates = { top: number; left: number };

type TooltipOverflow = {
Expand Down Expand Up @@ -424,30 +434,35 @@ const Tooltip = forwardRef<HTMLDivElement, TooltipProps>(
const enterTimerRef = useRef<NodeJS.Timeout | undefined>(undefined);
const leaveTimerRef = useRef<NodeJS.Timeout | undefined>(undefined);

// Stable composed ref. Recreating the callback ref every time the child
// element's identity changes makes React detach (call with `null`) and
// reattach (call with the node) on every parent render, so
// `setTriggerElement` fires null→node churn each render. A parent that
// re-renders in a loop then escalates into React error #185
// ("maximum update depth"). Holding the composed ref stable and reading
// the latest child ref from a ref means React only invokes it when the
// DOM node actually changes — mount and unmount, not every render.
const childRefHolder = useRef<React.Ref<HTMLElement> | undefined>(
undefined
const hasElementChild = isValidElement(children);
const childRef = hasElementChild
? (children.props as TooltipChildProps).ref
: undefined;
// React calls the previous callback ref with null before attaching a new
// callback to the same node. Ignore that transient null for positioning;
// otherwise an inline child ref produces null→node state churn and can
// escalate into React #185. A genuinely new node still updates state.
const triggerRef = useCallback(
(node: HTMLElement | null) => {
if (node !== null) {
setTriggerElement((previous) =>
previous === node ? previous : node
);
}
applyRef(childRef, node);
},
[childRef]
);
const triggerRef = useCallback((node: HTMLElement | null) => {
setTriggerElement((prev) => (prev === node ? prev : node));
applyRef(childRefHolder.current, node);
}, []);

const isControlled = popupVisible !== undefined;
const currentVisible = isControlled ? popupVisible : internalVisible;
const usesFramedSurface = framedPanel || (!panelStyle && !backgroundColor);

const updatePosition = useCallback(() => {
if (!triggerElement || !tooltipRef.current) return;
const positionedTrigger = hasElementChild ? triggerElement : null;
if (!positionedTrigger || !tooltipRef.current) return;

const triggerRect = triggerElement.getBoundingClientRect();
const triggerRect = positionedTrigger.getBoundingClientRect();
const tooltipRect = tooltipRef.current.getBoundingClientRect();
const gap = usesFramedSurface ? 8 : 12;

Expand Down Expand Up @@ -509,7 +524,13 @@ const Tooltip = forwardRef<HTMLDivElement, TooltipProps>(
setTooltipPosition({ top, left });
setArrowOffset({ left: arrowLeftOffset, top: arrowTopOffset });
setPositionReady(true);
}, [position, smartPlacement, triggerElement, usesFramedSurface]);
}, [
hasElementChild,
position,
smartPlacement,
triggerElement,
usesFramedSurface,
]);

useEffect(() => {
if (currentVisible) {
Expand Down Expand Up @@ -624,46 +645,30 @@ const Tooltip = forwardRef<HTMLDivElement, TooltipProps>(
}, []);

// Clone child and attach event handlers
type ElementProps = {
ref?: React.Ref<HTMLElement>;
onMouseEnter?: (e: React.MouseEvent) => void;
onMouseLeave?: (e: React.MouseEvent) => void;
onClick?: (e: React.MouseEvent) => void;
onFocus?: (e: React.FocusEvent) => void;
onBlur?: (e: React.FocusEvent) => void;
[key: string]: unknown;
};

// Clone child element and attach event handlers
// Callback refs are safe to pass during render - this is a false positive
const wrappedChildren = useMemo(() => {
if (!isValidElement(children)) {
return children;
}

const getElementProps = (
element: React.ReactElement<ElementProps>
): ElementProps => {
return element.props as ElementProps;
element: React.ReactElement<TooltipChildProps>
): TooltipChildProps => {
return element.props as TooltipChildProps;
};

const originalProps = getElementProps(
children as React.ReactElement<ElementProps>
children as React.ReactElement<TooltipChildProps>
);

// Preserve any ref the child already had (e.g. a parent's forwardRef
// used for dropdown positioning). Without this, wrapping an element
// in Tooltip would silently break refs like useDropdownEngine's
// triggerRef, causing click-to-open dropdowns to never position.
// The value is read through `childRefHolder` inside the STABLE
// `triggerRef`, so a changing child ref never re-thrashes the DOM ref.
// Writing the holder here is idempotent and only read post-commit from
// the ref callback — never during render — so it cannot cause tearing.
// eslint-disable-next-line react-hooks/refs
childRefHolder.current = originalProps.ref;

// eslint-disable-next-line react-hooks/refs
return cloneElement(children as React.ReactElement<ElementProps>, {
// triggerRef, causing click-to-open dropdowns to never position. React's
// refs rule conservatively treats cloneElement as a possible ref read;
// this only forwards the callback for React to invoke during commit.
// eslint-disable-next-line react-hooks/refs -- cloneElement forwards the composed callback ref; it never reads ref.current during render
return cloneElement(children as React.ReactElement<TooltipChildProps>, {
ref: triggerRef,
onMouseEnter: (e: React.MouseEvent) => {
handleMouseEnter();
Expand Down
2 changes: 1 addition & 1 deletion src/components/VirtualizedStickyTree/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ function VirtualizedStickyTreeInner<TNode extends TreeNodeBase>(
// Stable Scroller component - passing ref objects (not .current) is safe
// as they're only accessed in event handlers, not during render
const virtuosoComponents = useMemo(
/* eslint-disable react-hooks/refs */
/* eslint-disable react-hooks/refs -- the factory captures ref objects for later scroll callbacks and never reads ref.current during render */
() => ({
Scroller: createScrollerComponent(scrollHandlerRef, scrollerDomRef),
}),
Expand Down
15 changes: 10 additions & 5 deletions src/contexts/git/GitStatusContext/GitStatusProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import React, {
createContext,
useCallback,
useEffect,
useLayoutEffect,
useMemo,
useRef,
useState,
Expand Down Expand Up @@ -100,12 +101,14 @@ export const GitStatusProvider: React.FC<{ children: React.ReactNode }> = ({
// ============================================

const currentRepoIdRef = useRef<string | null>(null);
// eslint-disable-next-line react-hooks/refs
currentRepoIdRef.current = selectedRepoId;

const gitStatusRef = useRef<GitRepositoryStatus | null>(null);
// eslint-disable-next-line react-hooks/refs
gitStatusRef.current = gitStatus;
// WebSocket callbacks need the latest committed selection/status without
// forcing a listener teardown on every status write. A layout effect closes
// the render-to-external-event gap while keeping render itself pure.
useLayoutEffect(() => {
currentRepoIdRef.current = selectedRepoId;
gitStatusRef.current = gitStatus;
}, [gitStatus, selectedRepoId]);

const registeredReposRef = useRef<Set<string>>(new Set());
const pendingWatcherTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(
Expand Down Expand Up @@ -256,6 +259,7 @@ export const GitStatusProvider: React.FC<{ children: React.ReactNode }> = ({

if (!selectedRepoId) {
intendedRepoIdRef.current = null;
// eslint-disable-next-line react-hooks/set-state-in-effect -- a committed repo-scope transition must invalidate all repo-owned UI state before the external fetch lifecycle can continue
setGitStatus(null);
setGitSuggestedAction(null);
setStatusRepoId(null);
Expand Down Expand Up @@ -352,6 +356,7 @@ export const GitStatusProvider: React.FC<{ children: React.ReactNode }> = ({
if (!gitStatus || statusRepoId || statusRepoPath) return;
if (!selectedRepoId || !currentRepoPath) return;

// eslint-disable-next-line react-hooks/set-state-in-effect -- a push event can bootstrap status before fetch metadata; attach the already-committed repo identity in the follow-up synchronization pass
setStatusRepoId(selectedRepoId);
setStatusRepoPath(currentRepoPath);
}, [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ function renderSubmitHook(store: ReturnType<typeof createStore>) {

function HookProbe(): null {
// Test probe: capture the hook API synchronously from server rendering.
// eslint-disable-next-line react-hooks/globals
// eslint-disable-next-line react-hooks/globals -- server-rendered test probe synchronously exports the hook callback; the component never mounts or re-renders
submit = useUserIntentSubmit({ getSessionId: () => SESSION_ID });
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion src/hooks/navigation/useRoutineResultNavigation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ describe("useRoutineResultNavigation", () => {

function HookProbe(): null {
// Test probe: capture the hook API synchronously from server rendering.
// eslint-disable-next-line react-hooks/globals
// eslint-disable-next-line react-hooks/globals -- server-rendered test probe synchronously exports the hook callback; the component never mounts or re-renders
openResult = useRoutineResultNavigation();
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion src/hooks/ui/tabs/useSessionView.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe("useSessionView", () => {
let sessionView: UseSessionViewReturn | undefined;
function HookProbe(): null {
// Test probe: capture the hook API synchronously from server rendering.
// eslint-disable-next-line react-hooks/globals
// eslint-disable-next-line react-hooks/globals -- server-rendered test probe synchronously exports the hook result; the component never mounts or re-renders
sessionView = useSessionView();
return null;
}
Expand Down
Loading
Loading