fix(activity): coalesce portal readiness updates

This commit is contained in:
Neil
2026-07-29 02:23:38 -07:00
parent 92bb375400
commit 4e49b99b53
6 changed files with 199 additions and 166 deletions
@@ -140,7 +140,6 @@ type ActivityTerminalPortalReadiness = {
}
type ActivityTerminalPortalDomStatus = {
hasSelectedRoot: boolean
ready: boolean
unavailable: boolean
}
@@ -256,7 +255,7 @@ function getSelectedActivityTerminalPortalStatus(
): ActivityTerminalPortalDomStatus {
const parsed = parsePaneKey(paneKey)
if (!parsed) {
return { hasSelectedRoot: false, ready: false, unavailable: true }
return { ready: false, unavailable: true }
}
let selectedRoot: HTMLElement | null = null
for (const candidate of target.querySelectorAll<HTMLElement>('[data-terminal-tab-id]')) {
@@ -266,12 +265,12 @@ function getSelectedActivityTerminalPortalStatus(
}
}
if (!selectedRoot) {
return { hasSelectedRoot: false, ready: false, unavailable: false }
return { ready: false, unavailable: false }
}
const { foundAnyPane, pane: selectedPane } = findActivityTerminalPane(selectedRoot, parsed.leafId)
if (!selectedPane) {
return { hasSelectedRoot: true, ready: false, unavailable: foundAnyPane }
return { ready: false, unavailable: foundAnyPane }
}
const unavailable = hasInlineDisplayNoneBetween(selectedPane, selectedRoot)
@@ -283,7 +282,6 @@ function getSelectedActivityTerminalPortalStatus(
selectedPane.querySelector<HTMLElement>('[data-pty-id]') !== null
const hasXtermScreen = selectedPane.querySelector<HTMLElement>('.xterm-screen') !== null
return {
hasSelectedRoot: true,
ready: isVisibleRoot && !hasUnisolatedSibling && hasPtyBinding && hasXtermScreen,
unavailable
}
@@ -301,74 +299,67 @@ export function useActivityTerminalPortalStatus(
})
useLayoutEffect(() => {
let disposed = false
let readinessFrame: number | null = null
let pendingStatus: ActivityTerminalPortalReadiness['status'] | null = null
// Why: subscription churn can otherwise chain layout-effect updates past React's root-wide limit.
const scheduleReadiness = (status: ActivityTerminalPortalReadiness['status']): void => {
if (disposed) {
return
}
pendingStatus = status
if (readinessFrame !== null) {
return
}
readinessFrame = requestAnimationFrame(() => {
readinessFrame = null
const nextStatus = pendingStatus
pendingStatus = null
if (disposed || nextStatus === null) {
return
}
setReadiness((prev) =>
prev.target === target && prev.paneKey === paneKey && prev.status === nextStatus
? prev
: { target, paneKey, status: nextStatus }
)
})
}
const disposeFrame = (): void => {
disposed = true
if (readinessFrame !== null) {
cancelAnimationFrame(readinessFrame)
readinessFrame = null
}
}
if (!target || !paneKey) {
setReadiness((prev) =>
prev.target === null && prev.paneKey === null && prev.status === 'loading'
? prev
: { target: null, paneKey: null, status: 'loading' }
)
return
scheduleReadiness('loading')
return disposeFrame
}
if (forceUnavailable) {
setReadiness((prev) =>
prev.target === target && prev.paneKey === paneKey && prev.status === 'unavailable'
? prev
: { target, paneKey, status: 'unavailable' }
)
return
scheduleReadiness('unavailable')
return disposeFrame
}
let disposed = false
let readyFrame: number | null = null
let sawUnreadySelectedRoot = false
// Why: scoped to this subscription, so it resets whenever target/paneKey change.
const readinessLatch = createActivityPortalReadinessLatch()
const updateReadiness = (status: ActivityTerminalPortalReadiness['status']): void => {
const latched = readinessLatch.next(status)
setReadiness((prev) =>
prev.target === target && prev.paneKey === paneKey && prev.status === latched
? prev
: { target, paneKey, status: latched }
)
}
const cancelReadyFrame = (): void => {
if (readyFrame !== null) {
cancelAnimationFrame(readyFrame)
readyFrame = null
}
scheduleReadiness(readinessLatch.next(status))
}
const checkReadiness = (): void => {
const status = getSelectedActivityTerminalPortalStatus(target, paneKey)
if (status.unavailable) {
cancelReadyFrame()
updateReadiness('unavailable')
return
}
if (status.ready) {
if (!sawUnreadySelectedRoot) {
cancelReadyFrame()
updateReadiness('ready')
return
}
if (readyFrame !== null) {
return
}
// Why: PTY id can appear before xterm paints replayed output; wait one frame so Activity's cover hides the blank frame.
readyFrame = requestAnimationFrame(() => {
readyFrame = null
if (!disposed && getSelectedActivityTerminalPortalStatus(target, paneKey).ready) {
updateReadiness('ready')
}
})
updateReadiness('ready')
return
}
if (status.hasSelectedRoot) {
sawUnreadySelectedRoot = true
}
cancelReadyFrame()
updateReadiness('loading')
}
@@ -384,8 +375,7 @@ export function useActivityTerminalPortalStatus(
})
return () => {
disposed = true
cancelReadyFrame()
disposeFrame()
observer.disconnect()
}
}, [target, paneKey, forceUnavailable])
@@ -1,7 +1,7 @@
/** @vitest-environment happy-dom */
import { act, useLayoutEffect, useState } from 'react'
import { createRoot, type Root } from 'react-dom/client'
import { afterEach, describe, expect, it } from 'vitest'
import { afterEach, describe, expect, it, vi } from 'vitest'
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
@@ -30,10 +30,10 @@ const thread = (tabId: string, leafId: string): ActivityPortalThreadRef => ({
tab: { id: tabId }
})
// Two panes of the SAME tab: one TerminalPane, swapped in place by isolatedPaneKey.
// Same-tab panes share one TerminalPane and swap via isolatedPaneKey.
const PANE_A = thread(TAB_ID, LEAF_A)
const PANE_B = thread(TAB_ID, LEAF_B)
// A different tab: a genuinely separate TerminalPane, so staging applies.
// Cross-tab panes use separate TerminalPanes, so staging applies.
const PANE_C = thread(OTHER_TAB_ID, LEAF_C)
let root: Root
@@ -43,13 +43,40 @@ afterEach(() => {
root?.unmount()
})
document.body.replaceChildren()
vi.unstubAllGlobals()
})
/**
* Models the DOM one portaled TerminalPane emits: a tab root holding every leaf
* of the tab, with `applyExpandedLayoutTo` hiding the non-isolated siblings
* inline. Readiness only reports 'ready' when the wanted leaf is the unhidden one.
*/
function installAnimationFrameController(): {
flush: () => Promise<void>
pending: () => number
} {
let nextFrameId = 1
const callbacks = new Map<number, FrameRequestCallback>()
vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback): number => {
const frameId = nextFrameId
nextFrameId += 1
callbacks.set(frameId, callback)
return frameId
})
vi.stubGlobal('cancelAnimationFrame', (frameId: number): void => {
callbacks.delete(frameId)
})
return {
async flush() {
const queued = Array.from(callbacks.values())
callbacks.clear()
await act(async () => {
for (const callback of queued) {
callback(performance.now())
}
await Promise.resolve()
})
},
pending: () => callbacks.size
}
}
// Models the tab-root DOM and sibling hiding emitted by a portaled TerminalPane.
function renderPortaledTerminalPane(target: HTMLElement, tabId: string, leafIds: string[]): void {
const isolatedLeafId = leafIds[0]
const tabRoot = document.createElement('div')
@@ -71,17 +98,12 @@ function renderPortaledTerminalPane(target: HTMLElement, tabId: string, leafIds:
target.replaceChildren(tabRoot)
}
/**
* Drives the whole Activity portal loop against the real collaborators:
* reconcile -> publish descriptors -> Terminal's routing -> the readiness hook
* -> swap. Everything runs in useLayoutEffect, React's SYNC lane, which is the
* lane that increments nestedUpdateCount toward the #185 throw at 50.
*/
function runActivityPortalPage(args: {
// Exercises reconciliation, routing, readiness, and swapping on React's sync lane.
async function runActivityPortalPage(args: {
selectedThread: ActivityPortalThreadRef
initialDisplayed: ActivityPortalThreadRef
leafIdsByTabId: Record<string, string[]>
}): { displayedPaneKey: string | null; renders: number } {
}): Promise<{ displayedPaneKey: string | null; renders: number }> {
const { selectedThread, initialDisplayed, leafIdsByTabId } = args
const slotEls = {
primary: document.createElement('div'),
@@ -132,9 +154,7 @@ function runActivityPortalPage(args: {
})
}
// Why: Terminal mounts one TerminalPane per (worktree, tab) and resolves its
// portal target with worktree+tab only -- reproduced here so the test feels
// the same routing the crash did.
// Match Terminal's one-pane-per-(worktree, tab) routing.
useLayoutEffect(() => {
slotEls.primary.replaceChildren()
slotEls.secondary.replaceChildren()
@@ -183,42 +203,36 @@ function runActivityPortalPage(args: {
if (swap?.kind === 'settle-visible') {
setDisplayed(swap.paneKey)
}
// Why these deps: mirror ActivityPrototypePage's swap effect, so a
// convergence failure here means the page fails to converge too.
// Mirror ActivityPrototypePage's swap dependencies.
}, [inactiveSlotId, stagedStatus, stagedThread, visibleStatus, visibleThread])
return null
}
root = createRoot(document.createElement('div'))
act(() => {
await act(async () => {
root.render(<ActivityPortalPage />)
await new Promise((resolve) => setTimeout(resolve, 40))
})
return { displayedPaneKey, renders }
}
describe('Activity portal pane switching', () => {
it('converges on a newly selected pane of the tab already on screen', () => {
// Why this shape: staging a same-tab pane would need a second TerminalPane
// for one (worktree, tab), which Terminal never mounts -- the staged slot
// would stay empty, its readiness would stay 'loading', no swap arm would
// fire, and Activity would show the old pane under the new row forever.
const run = (): { displayedPaneKey: string | null; renders: number } =>
it('converges on a newly selected pane of the tab already on screen', async () => {
// Same-tab staging would wait forever for a second TerminalPane that never mounts.
const run = (): Promise<{ displayedPaneKey: string | null; renders: number }> =>
runActivityPortalPage({
selectedThread: PANE_B,
initialDisplayed: PANE_A,
leafIdsByTabId: { [TAB_ID]: [LEAF_A, LEAF_B] }
})
let result: { displayedPaneKey: string | null; renders: number } | null = null
expect(() => {
result = run()
}).not.toThrow()
expect(result!.displayedPaneKey).toBe(PANE_B.paneKey)
expect(result!.renders).toBeLessThan(50)
const result = await run()
expect(result.displayedPaneKey).toBe(PANE_B.paneKey)
expect(result.renders).toBeLessThan(50)
})
it('stages and swaps when the selected pane belongs to a different tab', () => {
const result = runActivityPortalPage({
it('stages and swaps when the selected pane belongs to a different tab', async () => {
const result = await runActivityPortalPage({
selectedThread: PANE_C,
initialDisplayed: PANE_A,
leafIdsByTabId: { [TAB_ID]: [LEAF_A, LEAF_B], [OTHER_TAB_ID]: [LEAF_C] }
@@ -227,19 +241,12 @@ describe('Activity portal pane switching', () => {
expect(result.renders).toBeLessThan(50)
})
/**
* Defense in depth, driven through the real hook so the assertion covers the
* production wiring and not just the latch module: if any DOM conflation makes
* 'ready' unreachable while 'unavailable' stays reachable, useActivityTerminal-
* PortalStatus must stop the sync-lane churn. nestedUpdateCount is global per
* root, so an unbounded spin surfaces in any unrelated component -- the reason
* this cluster appeared under four different error boundaries.
*/
// Drive the latch through production wiring because React's nested-update limit is root-wide.
it('bounds a readiness oscillation driven through the real portal-status hook', async () => {
const frames = installAnimationFrameController()
const target = document.createElement('div')
document.body.append(target)
// Hidden leaf -> 'unavailable'; nothing hidden -> 'loading' (an unhidden
// sibling suppresses 'ready'). 'ready' is unreachable, so the pair spins.
// Alternate hidden and ambiguous DOM states so ready remains unreachable.
const buildRoot = (hiddenLeafId: string | null): void => {
const tabRoot = document.createElement('div')
tabRoot.dataset.terminalTabId = TAB_ID
@@ -262,17 +269,14 @@ describe('Activity portal pane switching', () => {
let renders = 0
const statuses: ActivityPortalReadinessStatus[] = []
// Why a hard cap: an unlatched spin never settles, so `act` would hang until
// the suite timeout instead of failing on the assertion below. Stop feeding
// the loop past the cap so the test fails fast and legibly.
// Stop feeding an unlatched spin so failure is immediate and legible.
const RENDER_CAP = 50
function ActivityTerminalSlot(): null {
renders += 1
const status = useActivityTerminalPortalStatus(target, PANE_A.paneKey)
statuses.push(status)
// Models Terminal re-applying the opposite isolation, which the hook's
// MutationObserver then reports back as the opposite readiness.
// Reapply opposite isolation so MutationObserver reports opposite readiness.
useLayoutEffect(() => {
if (renders > RENDER_CAP) {
return
@@ -289,23 +293,20 @@ describe('Activity portal pane switching', () => {
root = createRoot(document.createElement('div'))
await act(async () => {
root.render(<ActivityTerminalSlot />)
await new Promise((resolve) => setTimeout(resolve, 60))
})
for (let frame = 0; frame < 20 && frames.pending() > 0; frame += 1) {
await frames.flush()
}
const settledRenders = renders
await frames.flush()
// Latched: the hook stops emitting new states well before React's 50-deep
// sync-update throw, and stays parked on 'unavailable'.
expect(renders).toBeLessThanOrEqual(RENDER_CAP)
expect(renders).toBe(settledRenders)
expect(frames.pending()).toBe(0)
expect(statuses.at(-1)).toBe('unavailable')
})
/**
* The latch's one user-facing risk: pinning "Terminal unavailable" over a
* terminal that churned during a slow attach and then genuinely came up. The
* latch only rewrites the hook's output, never the DOM probe, so a real
* 'ready' still arrives and releases it. Asserted through the real hook
* because the subscription is rebuilt only on target/paneKey change.
*/
it('releases a latched readiness once the terminal genuinely attaches', async () => {
it('releases a latched readiness once the terminal attaches', async () => {
const target = document.createElement('div')
document.body.append(target)
const buildRoot = (mode: 'hidden' | 'sibling' | 'ready'): void => {
@@ -351,14 +352,14 @@ describe('Activity portal pane switching', () => {
root = createRoot(document.createElement('div'))
await act(async () => {
root.render(<ActivityTerminalSlot />)
await new Promise((resolve) => setTimeout(resolve, 60))
await new Promise((resolve) => setTimeout(resolve, 180))
})
expect(statuses.at(-1)).toBe('unavailable')
churning = false
await act(async () => {
buildRoot('ready')
await new Promise((resolve) => setTimeout(resolve, 60))
await new Promise((resolve) => setTimeout(resolve, 40))
})
expect(statuses.at(-1)).toBe('ready')
})
@@ -11,8 +11,7 @@ describe('createActivityPortalReadinessLatch', () => {
for (let i = 0; i < 40; i += 1) {
seen.push(latch.next(i % 2 === 0 ? 'loading' : 'unavailable'))
}
// Why: the whole point is that an unbounded oscillation stops producing new
// states long before React's 50 nested sync updates throw #185.
// Settle well before React's 50 nested sync updates throw #185.
expect(seen.slice(-10).every((status) => status === 'unavailable')).toBe(true)
expect(seen.indexOf('unavailable')).toBeLessThan(ACTIVITY_PORTAL_READINESS_MAX_FLIPS + 2)
})
@@ -41,9 +40,7 @@ describe('createActivityPortalReadinessLatch', () => {
})
it('releases once the terminal genuinely comes up after a churny attach', () => {
// Why: a slow SSH host can burn the flip budget and still attach. The
// subscription is only rebuilt when target/paneKey change, so a latch that
// never released would pin "Terminal unavailable" over a working terminal.
// A slow SSH host may burn the flip budget before attaching successfully.
const latch = createActivityPortalReadinessLatch()
for (let i = 0; i < ACTIVITY_PORTAL_READINESS_MAX_FLIPS + 4; i += 1) {
latch.next(i % 2 === 0 ? 'loading' : 'unavailable')
@@ -1,31 +1,13 @@
export type ActivityPortalReadinessStatus = 'loading' | 'ready' | 'unavailable'
// Why: React throws #185 ("Maximum update depth exceeded") once 50 nested SYNC
// updates land on one root. Readiness updates run from useLayoutEffect, so a
// loading<->unavailable flip-flop rides that sync lane and saturates the
// counter. nestedUpdateCount is global per ROOT, so the throw then lands on
// whichever unrelated component sets state next -- which is why this cluster
// surfaced under four different error boundaries. Latch well below 50 (two
// readiness hooks share the budget) but above the flip or two a legitimately
// slow terminal produces while xterm attaches.
// Why: stop a fixed subscription from repainting forever after frame coalescing breaks its sync cascade.
export const ACTIVITY_PORTAL_READINESS_MAX_FLIPS = 8
export type ActivityPortalReadinessLatch = {
next: (status: ActivityPortalReadinessStatus) => ActivityPortalReadinessStatus
}
/**
* Bounds a loading<->unavailable oscillation for one readiness subscription.
*
* Why: defense in depth. Any DOM conflation that makes 'ready' unreachable
* while 'unavailable' stays reachable spins forever; latching degrades that one
* Activity pane instead of crashing the renderer.
*
* Scope: callers create one latch per (target, paneKey) subscription, so this
* only bounds a spin *within* one subscription. A cycle that also flips paneKey
* rebuilds the latch each pass and stays unbounded; Activity cannot produce one
* because displayedPaneKey only ever advances toward the selected pane.
*/
/** Bounds non-ready status flips for one readiness subscription. */
export function createActivityPortalReadinessLatch(): ActivityPortalReadinessLatch {
let lastStatus: ActivityPortalReadinessStatus | null = null
let flips = 0
@@ -33,11 +15,7 @@ export function createActivityPortalReadinessLatch(): ActivityPortalReadinessLat
return {
next(status) {
// Why 'ready' also releases the latch: it is never part of the
// pathological cycle, so a ready-free spin can never reach it — and the
// subscription is rebuilt only when target/paneKey change, so a latch
// that never released would pin "Terminal unavailable" over a terminal
// that churned during a slow attach and then genuinely came up.
// Why: a slow terminal may become ready after exhausting the flip budget.
if (status === 'ready') {
lastStatus = status
flips = 0
@@ -0,0 +1,80 @@
/** @vitest-environment happy-dom */
import { act, useState } from 'react'
import { flushSync } from 'react-dom'
import { createRoot, type Root } from 'react-dom/client'
import { afterEach, describe, expect, it } from 'vitest'
import { useActivityTerminalPortalStatus } from './ActivityPrototypePage'
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
const TAB_ID = 'tab-readiness-churn'
const LEAF_A = 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaa1'
const LEAF_B = 'bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbb1'
const LEAF_C = 'cccccccc-cccc-4ccc-8ccc-ccccccccccc1'
const PANE_A = `${TAB_ID}:${LEAF_A}`
const PANE_B = `${TAB_ID}:${LEAF_B}`
let root: Root
afterEach(() => {
act(() => {
root?.unmount()
})
document.body.replaceChildren()
})
function buildNeverReadyRoot(target: HTMLElement): void {
const tabRoot = document.createElement('div')
tabRoot.dataset.terminalTabId = TAB_ID
for (const leafId of [LEAF_A, LEAF_C]) {
const pane = document.createElement('div')
pane.dataset.leafId = leafId
pane.setAttribute('data-pty-id', `pty-${leafId}`)
pane.appendChild(Object.assign(document.createElement('div'), { className: 'xterm-screen' }))
Object.defineProperty(pane, 'getClientRects', { value: () => [{}], configurable: true })
tabRoot.appendChild(pane)
}
target.replaceChildren(tabRoot)
}
describe('Activity portal readiness subscription churn', () => {
it('coalesces pane-key churn and commits the latest readiness', async () => {
const target = document.createElement('div')
buildNeverReadyRoot(target)
document.body.append(target)
let selectPane: (paneKey: string) => void = () => {}
let renders = 0
let status = 'loading'
function ActivityTerminalSlot(): null {
renders += 1
const [paneKey, setPaneKey] = useState(PANE_A)
selectPane = setPaneKey
status = useActivityTerminalPortalStatus(target, paneKey)
return null
}
root = createRoot(document.createElement('div'))
act(() => {
root.render(<ActivityTerminalSlot />)
})
act(() => {
expect(() => {
for (let index = 0; index < 29; index += 1) {
flushSync(() => {
selectPane(index % 2 === 0 ? PANE_B : PANE_A)
})
}
}).not.toThrow()
})
expect(renders).toBeLessThanOrEqual(31)
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 30))
})
expect(status).toBe('unavailable')
expect(renders).toBeLessThanOrEqual(32)
})
})
@@ -10,10 +10,7 @@ export type ActivityPortalReconciliation<TThread extends ActivityPortalThreadRef
stagedThread: TThread | null
}
/**
* Picks which thread the Activity portal shows now (visible) and which one is
* warmed up in the inactive slot (staged).
*/
/** Picks the visible and staged Activity portal threads. */
export function reconcileActivityPortalThreads<TThread extends ActivityPortalThreadRef>(args: {
selectedThread: TThread | null
displayedThread: TThread | null
@@ -21,11 +18,7 @@ export function reconcileActivityPortalThreads<TThread extends ActivityPortalThr
displayedHasLiveTab: boolean
}): ActivityPortalReconciliation<TThread> {
const { selectedThread, displayedThread, selectedHasLiveTab, displayedHasLiveTab } = args
// Why worktree+tab, not paneKey: Terminal mounts one TerminalPane per
// (worktree, tab) and routes it by worktree+tab, so a same-tab staged
// descriptor never gets a pane and its readiness never leaves 'loading' —
// no swap arm fires and the old pane sticks. Same tab swaps in place via
// isolatedPaneKey instead of staging.
// Why: same-tab panes share one TerminalPane and swap through isolatedPaneKey, not staging.
const displayedIsSelectedTerminal = Boolean(
selectedThread &&
displayedThread &&
@@ -57,13 +50,7 @@ export type ActivityPortalSwap =
| { kind: 'settle-visible'; paneKey: string }
| null
/**
* Decides how displayedPaneKey advances for one commit.
*
* Why 'unavailable' counts as settled: a stale or unresolvable pane must still
* hand the slot over, otherwise the previous terminal stays on screen under the
* newly selected row and the readiness pass retries forever.
*/
/** Decides how displayedPaneKey advances for one commit. */
export function resolveActivityPortalSwap<TThread extends ActivityPortalThreadRef>(args: {
selectedThread: TThread | null
selectedHasLiveTab: boolean