From 3e9b6ddb3ceb0884e3fd8e30269fb4e6424bc49a Mon Sep 17 00:00:00 2001 From: Harry19081 <20519290+Harry19081@users.noreply.github.com> Date: Sun, 23 Aug 2026 04:46:48 +0800 Subject: [PATCH] fix: remove render-phase React hazards --- .../SessionHoverCard/HoverCardBase.tsx | 2 +- src/components/Table/index.tsx | 2 +- src/components/Tooltip/index.test.ts | 51 +++++++++ src/components/Tooltip/index.tsx | 89 ++++++++------- .../VirtualizedStickyTree/index.tsx | 2 +- .../GitStatusContext/GitStatusProvider.tsx | 15 ++- .../useUserIntentSubmit.intervention.test.ts | 2 +- .../useRoutineResultNavigation.test.ts | 2 +- src/hooks/ui/tabs/useSessionView.test.ts | 2 +- .../__tests__/gitFilesDerivation.test.ts | 103 +++++------------- .../hooks/sourceControl/gitFilesDerivation.ts | 45 ++++---- .../hooks/sourceControl/useGitFiles.ts | 41 +++---- .../shared/dataSource/TeamRuntimePanel.tsx | 16 +-- .../dataSource/teamRuntimeClock.test.ts | 59 ++++++++++ .../shared/dataSource/teamRuntimeClock.ts | 63 +++++++++++ .../useAddWorkspaceFlow.resetStorm.test.ts | 2 +- 16 files changed, 311 insertions(+), 185 deletions(-) create mode 100644 src/components/Tooltip/index.test.ts create mode 100644 src/modules/shared/dataSource/teamRuntimeClock.test.ts create mode 100644 src/modules/shared/dataSource/teamRuntimeClock.ts diff --git a/src/components/SessionHoverCard/HoverCardBase.tsx b/src/components/SessionHoverCard/HoverCardBase.tsx index 72f416b803..0cd2f0c443 100644 --- a/src/components/SessionHoverCard/HoverCardBase.tsx +++ b/src/components/SessionHoverCard/HoverCardBase.tsx @@ -190,7 +190,7 @@ const HoverCardTrigger: React.FC = ({ [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) => { diff --git a/src/components/Table/index.tsx b/src/components/Table/index.tsx index 3aed1cbdc2..730303e951 100644 --- a/src/components/Table/index.tsx +++ b/src/components/Table/index.tsx @@ -178,7 +178,7 @@ function TableComponent( 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, diff --git a/src/components/Tooltip/index.test.ts b/src/components/Tooltip/index.test.ts new file mode 100644 index 0000000000..bf41ce068e --- /dev/null +++ b/src/components/Tooltip/index.test.ts @@ -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, + 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); + }); +}); diff --git a/src/components/Tooltip/index.tsx b/src/components/Tooltip/index.tsx index 5963b6f9b7..8c83dbed25 100644 --- a/src/components/Tooltip/index.tsx +++ b/src/components/Tooltip/index.tsx @@ -58,6 +58,16 @@ function applyRef(ref: React.Ref | undefined, value: T | null): void { (ref as React.MutableRefObject).current = value; } +type TooltipChildProps = { + ref?: React.Ref; + 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 = { @@ -424,30 +434,35 @@ const Tooltip = forwardRef( const enterTimerRef = useRef(undefined); const leaveTimerRef = useRef(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 | 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; @@ -509,7 +524,13 @@ const Tooltip = forwardRef( setTooltipPosition({ top, left }); setArrowOffset({ left: arrowLeftOffset, top: arrowTopOffset }); setPositionReady(true); - }, [position, smartPlacement, triggerElement, usesFramedSurface]); + }, [ + hasElementChild, + position, + smartPlacement, + triggerElement, + usesFramedSurface, + ]); useEffect(() => { if (currentVisible) { @@ -624,46 +645,30 @@ const Tooltip = forwardRef( }, []); // Clone child and attach event handlers - type ElementProps = { - ref?: React.Ref; - 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 => { - return element.props as ElementProps; + element: React.ReactElement + ): TooltipChildProps => { + return element.props as TooltipChildProps; }; const originalProps = getElementProps( - children as React.ReactElement + children as React.ReactElement ); // 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, { + // 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, { ref: triggerRef, onMouseEnter: (e: React.MouseEvent) => { handleMouseEnter(); diff --git a/src/components/VirtualizedStickyTree/index.tsx b/src/components/VirtualizedStickyTree/index.tsx index 1d1805cd57..116321d18c 100644 --- a/src/components/VirtualizedStickyTree/index.tsx +++ b/src/components/VirtualizedStickyTree/index.tsx @@ -203,7 +203,7 @@ function VirtualizedStickyTreeInner( // 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), }), diff --git a/src/contexts/git/GitStatusContext/GitStatusProvider.tsx b/src/contexts/git/GitStatusContext/GitStatusProvider.tsx index 091552808a..3d808ea06f 100644 --- a/src/contexts/git/GitStatusContext/GitStatusProvider.tsx +++ b/src/contexts/git/GitStatusContext/GitStatusProvider.tsx @@ -23,6 +23,7 @@ import React, { createContext, useCallback, useEffect, + useLayoutEffect, useMemo, useRef, useState, @@ -100,12 +101,14 @@ export const GitStatusProvider: React.FC<{ children: React.ReactNode }> = ({ // ============================================ const currentRepoIdRef = useRef(null); - // eslint-disable-next-line react-hooks/refs - currentRepoIdRef.current = selectedRepoId; - const gitStatusRef = useRef(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>(new Set()); const pendingWatcherTimeoutRef = useRef | null>( @@ -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); @@ -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); }, [ diff --git a/src/engines/ChatPanel/hooks/useWorkspaceChat/useUserIntentSubmit.intervention.test.ts b/src/engines/ChatPanel/hooks/useWorkspaceChat/useUserIntentSubmit.intervention.test.ts index 50c22660d8..c0011c15d5 100644 --- a/src/engines/ChatPanel/hooks/useWorkspaceChat/useUserIntentSubmit.intervention.test.ts +++ b/src/engines/ChatPanel/hooks/useWorkspaceChat/useUserIntentSubmit.intervention.test.ts @@ -65,7 +65,7 @@ function renderSubmitHook(store: ReturnType) { 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; } diff --git a/src/hooks/navigation/useRoutineResultNavigation.test.ts b/src/hooks/navigation/useRoutineResultNavigation.test.ts index eb8528ea0a..9206b44043 100644 --- a/src/hooks/navigation/useRoutineResultNavigation.test.ts +++ b/src/hooks/navigation/useRoutineResultNavigation.test.ts @@ -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; } diff --git a/src/hooks/ui/tabs/useSessionView.test.ts b/src/hooks/ui/tabs/useSessionView.test.ts index c45aa04ee1..2bc238be08 100644 --- a/src/hooks/ui/tabs/useSessionView.test.ts +++ b/src/hooks/ui/tabs/useSessionView.test.ts @@ -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; } diff --git a/src/modules/WorkStation/CodeEditor/hooks/sourceControl/__tests__/gitFilesDerivation.test.ts b/src/modules/WorkStation/CodeEditor/hooks/sourceControl/__tests__/gitFilesDerivation.test.ts index e1da387d80..c370879024 100644 --- a/src/modules/WorkStation/CodeEditor/hooks/sourceControl/__tests__/gitFilesDerivation.test.ts +++ b/src/modules/WorkStation/CodeEditor/hooks/sourceControl/__tests__/gitFilesDerivation.test.ts @@ -2,10 +2,10 @@ import { describe, expect, it } from "vitest"; import type { GitWorkingDirectoryFile } from "@src/api/http/git"; import { - areBaseFileListsEqual, + baseFileListIdentity, deriveBaseFiles, + deriveBaseFilesFromIdentity, } from "@src/modules/WorkStation/CodeEditor/hooks/sourceControl/gitFilesDerivation"; -import type { GitFile } from "@src/types/git/types"; function createStatusFile( overrides: Partial = {} @@ -19,19 +19,6 @@ function createStatusFile( } as GitWorkingDirectoryFile; } -function createGitFile(overrides: Partial = {}): GitFile { - return { - id: "src/index.ts-0", - path: "src/index.ts", - status: "modified", - additions: 0, - deletions: 0, - staged: false, - original_path: null, - ...overrides, - }; -} - describe("deriveBaseFiles", () => { it("returns an empty list for empty input", () => { expect(deriveBaseFiles([])).toEqual([]); @@ -57,73 +44,41 @@ describe("deriveBaseFiles", () => { }); }); -describe("areBaseFileListsEqual", () => { - it("returns true for the same array reference", () => { - const list = [createGitFile()]; - expect(areBaseFileListsEqual(list, list)).toBe(true); - }); - - it("returns true for two structurally identical (byte-identical) lists", () => { - expect(areBaseFileListsEqual([createGitFile()], [createGitFile()])).toBe( - true +describe("baseFileListIdentity", () => { + it("is stable for structurally identical status payloads", () => { + expect(baseFileListIdentity([createStatusFile()])).toBe( + baseFileListIdentity([createStatusFile()]) ); }); - it("returns false when lengths differ", () => { - expect( - areBaseFileListsEqual( - [createGitFile()], - [createGitFile(), createGitFile({ id: "b-1", path: "b.ts" })] - ) - ).toBe(false); - }); - - it("returns false when a staged flag changes (cannot stick stale)", () => { - expect( - areBaseFileListsEqual( - [createGitFile({ staged: false })], - [createGitFile({ staged: true })] - ) - ).toBe(false); - }); - - it("returns false when status changes", () => { - expect( - areBaseFileListsEqual( - [createGitFile({ status: "modified" })], - [createGitFile({ status: "deleted" })] - ) - ).toBe(false); - }); - - it("returns false when path/id changes", () => { - expect( - areBaseFileListsEqual( - [createGitFile({ id: "a-0", path: "a.ts" })], - [createGitFile({ id: "b-0", path: "b.ts" })] - ) - ).toBe(false); + it.each<[string, Partial]>([ + ["path", { path: "src/other.ts" }], + ["status", { status: "D" }], + ["staged", { staged: true }], + ["rename source", { original_path: "src/old.ts" }], + ])("changes when %s changes", (_label, overrides) => { + expect(baseFileListIdentity([createStatusFile(overrides)])).not.toBe( + baseFileListIdentity([createStatusFile()]) + ); }); - it("returns false when original_path (rename source) changes", () => { + it("distinguishes an omitted rename source from explicit null", () => { expect( - areBaseFileListsEqual( - [createGitFile({ original_path: null })], - [createGitFile({ original_path: "old.ts" })] - ) - ).toBe(false); + baseFileListIdentity([createStatusFile({ original_path: undefined })]) + ).not.toBe(baseFileListIdentity([createStatusFile()])); }); - it("ignores non-identity fields like additions/deletions for the gate", () => { + it("round-trips every field consumed by the derivation", () => { + const statusFiles = [ + createStatusFile({ + path: "src/renamed.ts", + status: "R", + staged: true, + original_path: "src/original.ts", + }), + ]; expect( - areBaseFileListsEqual( - [createGitFile({ additions: 0, deletions: 0 })], - [createGitFile({ additions: 5, deletions: 3 })] - ) - ).toBe(true); - }); - - it("handles two empty lists as equal", () => { - expect(areBaseFileListsEqual([], [])).toBe(true); + deriveBaseFilesFromIdentity(baseFileListIdentity(statusFiles)) + ).toEqual(deriveBaseFiles(statusFiles)); }); }); diff --git a/src/modules/WorkStation/CodeEditor/hooks/sourceControl/gitFilesDerivation.ts b/src/modules/WorkStation/CodeEditor/hooks/sourceControl/gitFilesDerivation.ts index 664a9a8cab..00720c5646 100644 --- a/src/modules/WorkStation/CodeEditor/hooks/sourceControl/gitFilesDerivation.ts +++ b/src/modules/WorkStation/CodeEditor/hooks/sourceControl/gitFilesDerivation.ts @@ -5,7 +5,7 @@ import type { GitFile } from "@src/types/git/types"; /** * Map raw working-directory entries from a git status payload into the * `GitFile` shape used by the Source Control UI. Extracted from `useGitFiles` - * so the derivation and equality gate can be unit-tested without React. + * so the derivation and structural identity key can be tested without React. */ export function deriveBaseFiles( statusFiles: GitWorkingDirectoryFile[] @@ -25,27 +25,26 @@ export function deriveBaseFiles( } /** - * Structural equality for two derived base-file lists. Compares only the - * identity-bearing fields produced by {@link deriveBaseFiles} - * (`id`, `path`, `status`, `staged`, `original_path`) — every other field is a - * constant for a freshly derived list, so this can never "stick" a stale array: - * any working-tree change to those fields yields `false` and forces a new ref. + * Primitive dependency key for {@link deriveBaseFiles}. A status refresh often + * replaces the payload object without changing the working tree; using this + * key lets React memoization retain the derived array without a render-time ref + * cache. JSON preserves ordering and distinguishes a missing `original_path` + * from an explicit null, so every derivation-bearing input is represented. */ -export function areBaseFileListsEqual(a: GitFile[], b: GitFile[]): boolean { - if (a === b) return true; - if (a.length !== b.length) return false; - for (let i = 0; i < a.length; i++) { - const left = a[i]; - const right = b[i]; - if ( - left.id !== right.id || - left.path !== right.path || - left.status !== right.status || - left.staged !== right.staged || - left.original_path !== right.original_path - ) { - return false; - } - } - return true; +export function baseFileListIdentity( + statusFiles: GitWorkingDirectoryFile[] +): string { + return JSON.stringify( + statusFiles.map((file) => ({ + path: file.path, + status: file.status, + staged: file.staged, + original_path: file.original_path, + })) + ); +} + +/** Rebuild the derived list from its complete primitive identity snapshot. */ +export function deriveBaseFilesFromIdentity(identity: string): GitFile[] { + return deriveBaseFiles(JSON.parse(identity) as GitWorkingDirectoryFile[]); } diff --git a/src/modules/WorkStation/CodeEditor/hooks/sourceControl/useGitFiles.ts b/src/modules/WorkStation/CodeEditor/hooks/sourceControl/useGitFiles.ts index 7336eb229e..d4132b1737 100644 --- a/src/modules/WorkStation/CodeEditor/hooks/sourceControl/useGitFiles.ts +++ b/src/modules/WorkStation/CodeEditor/hooks/sourceControl/useGitFiles.ts @@ -25,37 +25,30 @@ import { useGitStatus } from "@src/contexts/git"; import type { GitFile } from "@src/types/git/types"; import type { GitRepositoryStatus } from "@src/types/session/steps"; -import { areBaseFileListsEqual, deriveBaseFiles } from "./gitFilesDerivation"; +import { + baseFileListIdentity, + deriveBaseFilesFromIdentity, +} from "./gitFilesDerivation"; + +const EMPTY_STATUS_FILES: GitRepositoryStatus["working_directory"]["files"] = + []; /** - * Derive the base file list from a git status, returning the SAME array - * reference when a newly-fetched status describes a byte-identical working - * tree. Background status pings replace the `gitStatus` object on every poll; - * without this stabilization each poll cascades a fresh `files` reference into - * `useSourceControlState`'s state memo even when nothing changed. - * - * Mirrors the ref-cached `useMemo` pattern used by `useEventStoreSelector`. + * Derive the base file list from a structural primitive key. Background status + * pings replace the `gitStatus` object on every poll; keying memoization by the + * actual working-tree fields retains the array when nothing changed without + * reading or mutating refs during render. */ function useStableBaseFiles( gitStatus: GitRepositoryStatus | null, selectedRepoId: string | null ): GitFile[] { - const prevRef = useRef([]); - return useMemo(() => { - const next = - !selectedRepoId || !gitStatus - ? [] - : deriveBaseFiles(gitStatus.working_directory?.files || []); - - // The equality gate compares every identity-bearing field, so any real - // working-tree change yields a fresh array and this can never stick stale. - if (areBaseFileListsEqual(prevRef.current, next)) { - return prevRef.current; - } - prevRef.current = next; - return next; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [gitStatus, selectedRepoId]); + const statusFiles = + selectedRepoId && gitStatus + ? (gitStatus.working_directory?.files ?? EMPTY_STATUS_FILES) + : EMPTY_STATUS_FILES; + const identity = baseFileListIdentity(statusFiles); + return useMemo(() => deriveBaseFilesFromIdentity(identity), [identity]); } export interface UseGitFilesOptions { diff --git a/src/modules/shared/dataSource/TeamRuntimePanel.tsx b/src/modules/shared/dataSource/TeamRuntimePanel.tsx index 9392f012d5..06e12f022d 100644 --- a/src/modules/shared/dataSource/TeamRuntimePanel.tsx +++ b/src/modules/shared/dataSource/TeamRuntimePanel.tsx @@ -39,6 +39,7 @@ import TeamMemberCard, { } from "./TeamMemberCard"; import TeamMemberDetail from "./TeamMemberDetail"; import TeamRuntimeToday from "./TeamRuntimeToday"; +import { useTeamRuntimeClock } from "./teamRuntimeClock"; import { hasMemberActivityToday } from "./teamRuntimeData"; import { useTeamRuntimeRoster } from "./useTeamRuntimeRoster"; @@ -198,19 +199,13 @@ export default function TeamRuntimePanel({ const [openMemberId, setOpenMemberId] = useState(null); const [selectedMemberId, setSelectedMemberId] = useState(null); - // One clock per render pass so staleness and the today/7d fold agree across - // every card. Quantized to the whole minute (org intervals are >=15min, so - // ~1min staleness granularity is invisible) so an unrelated re-render (a - // click, a settings change) recomputes the SAME nowMs value instead of a - // strictly-increasing one — otherwise every card's `nowMs` prop would - // differ by construction and the `TeamMemberCard` React.memo comparison - // could never hold. The minute quantization is exactly what makes the read - // render-stable, so the purity rule's concern doesn't apply here. - // eslint-disable-next-line react-hooks/purity -- quantized clock, see above - const nowMs = Math.floor(Date.now() / 60_000) * 60_000; + // One minute-aligned, visibility-aware clock keeps every card on the same + // snapshot without making unrelated renders read wall-clock time. + const nowMs = useTeamRuntimeClock(); // Leaving the org scope or losing the member closes the drilldown. useEffect(() => { + // eslint-disable-next-line react-hooks/set-state-in-effect -- these ids are owned by the committed org/user scope and must not survive a scope transition setOpenMemberId(null); setSelectedMemberId(null); }, [roster.selectedOrgId, roster.currentUserId]); @@ -218,6 +213,7 @@ export default function TeamRuntimePanel({ // A member drilldown belongs to the Members tab; don't retain a hidden // detail surface if the user returns to Today. useEffect(() => { + // eslint-disable-next-line react-hooks/set-state-in-effect -- the controlled tab transition owns teardown of its hidden member drilldown if (view !== "members") setOpenMemberId(null); }, [view]); diff --git a/src/modules/shared/dataSource/teamRuntimeClock.test.ts b/src/modules/shared/dataSource/teamRuntimeClock.test.ts new file mode 100644 index 0000000000..48e9242819 --- /dev/null +++ b/src/modules/shared/dataSource/teamRuntimeClock.test.ts @@ -0,0 +1,59 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { currentMinute, startTeamRuntimeClock } from "./teamRuntimeClock"; + +class VisibilitySourceStub { + visibilityState: DocumentVisibilityState = "visible"; + private listener: (() => void) | undefined; + + addEventListener(_type: "visibilitychange", listener: () => void): void { + this.listener = listener; + } + + removeEventListener(_type: "visibilitychange", listener: () => void): void { + if (this.listener === listener) this.listener = undefined; + } + + setVisibility(visibilityState: DocumentVisibilityState): void { + this.visibilityState = visibilityState; + this.listener?.(); + } +} + +afterEach(() => { + vi.useRealTimers(); +}); + +describe("startTeamRuntimeClock", () => { + it("aligns ticks to minutes, pauses hidden, and disposes its timer", () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-08-23T10:00:42.000Z")); + const source = new VisibilitySourceStub(); + const onTick = vi.fn(); + const stop = startTeamRuntimeClock(source, onTick); + + expect(vi.getTimerCount()).toBe(1); + vi.advanceTimersByTime(17_999); + expect(onTick).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + expect(onTick).toHaveBeenLastCalledWith( + new Date("2026-08-23T10:01:00.000Z").getTime() + ); + expect(vi.getTimerCount()).toBe(1); + + source.setVisibility("hidden"); + expect(vi.getTimerCount()).toBe(0); + vi.advanceTimersByTime(5 * 60_000); + expect(onTick).toHaveBeenCalledOnce(); + + source.setVisibility("visible"); + expect(onTick).toHaveBeenCalledTimes(2); + expect(onTick).toHaveBeenLastCalledWith(currentMinute()); + expect(vi.getTimerCount()).toBe(1); + + stop(); + expect(vi.getTimerCount()).toBe(0); + source.setVisibility("visible"); + expect(onTick).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/modules/shared/dataSource/teamRuntimeClock.ts b/src/modules/shared/dataSource/teamRuntimeClock.ts new file mode 100644 index 0000000000..c1361e6dd6 --- /dev/null +++ b/src/modules/shared/dataSource/teamRuntimeClock.ts @@ -0,0 +1,63 @@ +import { useEffect, useState } from "react"; + +const MINUTE_MS = 60_000; + +interface TeamRuntimeClockSource { + readonly visibilityState: DocumentVisibilityState; + addEventListener(type: "visibilitychange", listener: () => void): void; + removeEventListener(type: "visibilitychange", listener: () => void): void; +} + +export function currentMinute(now = Date.now()): number { + return Math.floor(now / MINUTE_MS) * MINUTE_MS; +} + +/** + * Own one minute-aligned clock for the mounted runtime panel. Hidden documents + * do no periodic work; returning visible catches up immediately. Recursive + * timeouts avoid interval overlap and are always released on disposal. + */ +export function startTeamRuntimeClock( + source: TeamRuntimeClockSource, + onTick: (nowMs: number) => void +): () => void { + let timer: ReturnType | undefined; + let disposed = false; + + const clearTimer = () => { + if (timer === undefined) return; + clearTimeout(timer); + timer = undefined; + }; + const schedule = () => { + clearTimer(); + if (disposed || source.visibilityState === "hidden") return; + const now = Date.now(); + const delay = MINUTE_MS - (now % MINUTE_MS); + timer = setTimeout(() => { + timer = undefined; + onTick(currentMinute()); + schedule(); + }, delay); + }; + const handleVisibilityChange = () => { + clearTimer(); + if (source.visibilityState === "hidden") return; + onTick(currentMinute()); + schedule(); + }; + + source.addEventListener("visibilitychange", handleVisibilityChange); + schedule(); + return () => { + disposed = true; + clearTimer(); + source.removeEventListener("visibilitychange", handleVisibilityChange); + }; +} + +export function useTeamRuntimeClock(): number { + const [nowMs, setNowMs] = useState(currentMinute); + useEffect(() => startTeamRuntimeClock(document, setNowMs), []); + return nowMs; +} diff --git a/src/scaffold/GlobalSpotlight/hooks/forms/__tests__/useAddWorkspaceFlow.resetStorm.test.ts b/src/scaffold/GlobalSpotlight/hooks/forms/__tests__/useAddWorkspaceFlow.resetStorm.test.ts index 82198165a6..22f18f6212 100644 --- a/src/scaffold/GlobalSpotlight/hooks/forms/__tests__/useAddWorkspaceFlow.resetStorm.test.ts +++ b/src/scaffold/GlobalSpotlight/hooks/forms/__tests__/useAddWorkspaceFlow.resetStorm.test.ts @@ -137,7 +137,7 @@ function Harness(): null { // Test-only commit counter; the render-phase mutation is deliberate so the // circuit breaker below can throw mid-storm (verified against the original // buggy effect deps: it turns an OOM'd worker into a clean failure). - // eslint-disable-next-line react-hooks/globals + // eslint-disable-next-line react-hooks/globals -- deliberate render counter is the test's circuit breaker for the regression's self-sustaining commit storm commits += 1; if (commits > 400) { throw new Error(