From 0893a4208c33b844bacb67339e604d29063a6bde Mon Sep 17 00:00:00 2001 From: Ryan Bahan Date: Wed, 1 Apr 2026 17:53:36 -0600 Subject: [PATCH 1/3] use central unmount; stop messing with node's event loop --- .../cli-kit/src/private/node/testing/ui.ts | 6 +-- packages/cli-kit/src/private/node/ui.tsx | 41 ++++++++++++++++--- .../node/ui/components/AutocompletePrompt.tsx | 9 ++-- .../node/ui/components/ConcurrentOutput.tsx | 23 ++++++----- .../DangerousConfirmationPrompt.tsx | 12 +++--- .../node/ui/components/SelectPrompt.tsx | 8 ++-- .../private/node/ui/components/SingleTask.tsx | 19 ++++++--- .../private/node/ui/components/TextPrompt.tsx | 10 ++--- .../private/node/ui/hooks/use-abort-signal.ts | 26 +++++------- .../node/ui/hooks/use-async-and-unmount.ts | 17 +++++--- 10 files changed, 108 insertions(+), 63 deletions(-) diff --git a/packages/cli-kit/src/private/node/testing/ui.ts b/packages/cli-kit/src/private/node/testing/ui.ts index 58b8a7df29c..4278cbc74c2 100644 --- a/packages/cli-kit/src/private/node/testing/ui.ts +++ b/packages/cli-kit/src/private/node/testing/ui.ts @@ -1,5 +1,5 @@ -import {Stdout} from '../ui.js' -import {ReactElement} from 'react' +import {Stdout, InkLifecycleRoot} from '../ui.js' +import React, {ReactElement} from 'react' import {render as inkRender} from 'ink' import {EventEmitter} from 'events' @@ -66,7 +66,7 @@ export const render = (tree: ReactElement, options: RenderOptions = {}): Instanc const stderr = new Stderr() const stdin = new Stdin() - const instance = inkRender(tree, { + const instance = inkRender(React.createElement(InkLifecycleRoot, null, tree), { stdout: options.stdout ?? (stdout as any), stderr: options.stderr ?? (stderr as any), diff --git a/packages/cli-kit/src/private/node/ui.tsx b/packages/cli-kit/src/private/node/ui.tsx index 06a91c7ba45..d4c3561d3b3 100644 --- a/packages/cli-kit/src/private/node/ui.tsx +++ b/packages/cli-kit/src/private/node/ui.tsx @@ -3,11 +3,44 @@ import {Logger, LogLevel} from '../../public/node/output.js' import {isUnitTest} from '../../public/node/context/local.js' import {treeKill} from '../../public/node/tree-kill.js' -import {ReactElement} from 'react' -import {Key, render as inkRender, RenderOptions} from 'ink' +import React, {ReactElement, createContext, useCallback, useContext, useEffect, useState} from 'react' +import {Key, render as inkRender, RenderOptions, useApp} from 'ink' import {EventEmitter} from 'events' +const CompletionContext = createContext<(error?: Error) => void>(() => {}) + +/** + * Signal that the current Ink tree is done. The root wrapper will call + * `exit()` after React finishes rendering. + */ +export function useComplete(): (error?: Error) => void { + return useContext(CompletionContext) +} + +/** + * Root wrapper for Ink trees. Owns the single `exit()` call site — children + * signal completion via `useComplete()`, which sets state here. The `useEffect` + * fires post-render, guaranteeing all batched state updates have been flushed + * before the tree is torn down. + */ +export function InkLifecycleRoot({children}: {children: React.ReactNode}) { + const {exit} = useApp() + const [exitResult, setExitResult] = useState<{error?: Error} | null>(null) + + const complete = useCallback((error?: Error) => { + setExitResult({error}) + }, []) + + useEffect(() => { + if (exitResult !== null) { + exit(exitResult.error) + } + }, [exitResult, exit]) + + return {children} +} + interface RenderOnceOptions { logLevel?: LogLevel logger?: Logger @@ -27,10 +60,8 @@ export function renderOnce(element: JSX.Element, {logLevel = 'info', renderOptio } export async function render(element: JSX.Element, options?: RenderOptions) { - const {waitUntilExit} = inkRender(element, options) + const {waitUntilExit} = inkRender({element}, options) await waitUntilExit() - // We need to wait for other pending tasks -- unmounting of the ink component -- to complete - return new Promise((resolve) => setImmediate(resolve)) } interface Instance { diff --git a/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.tsx b/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.tsx index 80bf98d3ed2..01f277dfe66 100644 --- a/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.tsx +++ b/packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.tsx @@ -5,10 +5,11 @@ import {InfoMessageProps} from './Prompts/InfoMessage.js' import {Message, PromptLayout} from './Prompts/PromptLayout.js' import {throttle} from '../../../../public/common/function.js' import {AbortSignal} from '../../../../public/node/abort.js' +import {useComplete} from '../../ui.js' import usePrompt, {PromptState} from '../hooks/use-prompt.js' import React, {ReactElement, useCallback, useEffect, useRef, useState} from 'react' -import {Box, useApp} from 'ink' +import {Box} from 'ink' export interface SearchResults { data: SelectItem[] @@ -42,7 +43,7 @@ function AutocompletePrompt({ infoMessage, groupOrder, }: React.PropsWithChildren>): ReactElement | null { - const {exit: unmountInk} = useApp() + const complete = useComplete() const [searchTerm, setSearchTerm] = useState('') const [searchResults, setSearchResults] = useState[]>(choices) const canSearch = choices.length > MIN_NUMBER_OF_ITEMS_FOR_SEARCH @@ -72,10 +73,10 @@ function AutocompletePrompt({ useEffect(() => { if (promptState === PromptState.Submitted && answer) { setSearchTerm('') - unmountInk() onSubmit(answer.value) + complete() } - }, [answer, onSubmit, promptState, unmountInk]) + }, [answer, onSubmit, promptState, complete]) const setLoadingWhenSlow = useRef() diff --git a/packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.tsx b/packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.tsx index e447b8814a6..b866189e5ae 100644 --- a/packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.tsx +++ b/packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.tsx @@ -1,7 +1,8 @@ import {OutputProcess} from '../../../../public/node/output.js' import {AbortSignal} from '../../../../public/node/abort.js' +import {useComplete} from '../../ui.js' import React, {FunctionComponent, useCallback, useEffect, useMemo, useState} from 'react' -import {Box, Static, Text, TextProps, useApp} from 'ink' +import {Box, Static, Text, TextProps} from 'ink' import figures from 'figures' import stripAnsi from 'strip-ansi' @@ -92,7 +93,8 @@ const ConcurrentOutput: FunctionComponent = ({ useAlternativeColorPalette = false, }) => { const [processOutput, setProcessOutput] = useState([]) - const {exit: unmountInk} = useApp() + const [completionResult, setCompletionResult] = useState<{error?: Error} | null>(null) + const complete = useComplete() const concurrentColors: TextProps['color'][] = useMemo( () => useAlternativeColorPalette @@ -179,24 +181,25 @@ const ConcurrentOutput: FunctionComponent = ({ }), ) if (!keepRunningAfterProcessesResolve) { - // Defer unmount so React 19 can flush batched setProcessOutput - // state updates before the component tree is torn down. - // Use setImmediate → setTimeout(0) to span two event-loop phases, - // giving React's scheduler (which uses setImmediate in Node.js) - // a full cycle to flush before we tear down the component tree. - setImmediate(() => setTimeout(() => unmountInk(), 0)) + setCompletionResult({}) } // eslint-disable-next-line no-catch-all/no-catch-all } catch (error: unknown) { if (!keepRunningAfterProcessesResolve) { - setImmediate(() => setTimeout(() => unmountInk(error as Error | undefined), 0)) + setCompletionResult({error: error as Error}) } } } // eslint-disable-next-line @typescript-eslint/no-floating-promises runProcesses() - }, [abortSignal, processes, writableStream, unmountInk, keepRunningAfterProcessesResolve]) + }, [abortSignal, processes, writableStream, keepRunningAfterProcessesResolve]) + + useEffect(() => { + if (completionResult !== null) { + complete(completionResult.error) + } + }, [completionResult, complete]) const {lineVertical} = figures diff --git a/packages/cli-kit/src/private/node/ui/components/DangerousConfirmationPrompt.tsx b/packages/cli-kit/src/private/node/ui/components/DangerousConfirmationPrompt.tsx index d877dd98440..533b20ac8c3 100644 --- a/packages/cli-kit/src/private/node/ui/components/DangerousConfirmationPrompt.tsx +++ b/packages/cli-kit/src/private/node/ui/components/DangerousConfirmationPrompt.tsx @@ -1,7 +1,7 @@ import {TextInput} from './TextInput.js' import {InlineToken, TokenItem, TokenizedText} from './TokenizedText.js' import {InfoTable, InfoTableProps} from './Prompts/InfoTable.js' -import {handleCtrlC} from '../../ui.js' +import {handleCtrlC, useComplete} from '../../ui.js' import useLayout from '../hooks/use-layout.js' import {messageWithPunctuation} from '../utilities.js' import {AbortSignal} from '../../../../public/node/abort.js' @@ -9,7 +9,7 @@ import useAbortSignal from '../hooks/use-abort-signal.js' import usePrompt, {PromptState} from '../hooks/use-prompt.js' import React, {FunctionComponent, useCallback, useEffect, useState} from 'react' -import {Box, useApp, useInput, Text} from 'ink' +import {Box, useInput, Text} from 'ink' import figures from 'figures' export interface DangerousConfirmationPromptProps { @@ -38,7 +38,7 @@ const DangerousConfirmationPrompt: FunctionComponent({ initialAnswer: '', }) - const {exit: unmountInk} = useApp() + const complete = useComplete() const [error, setError] = useState | undefined>(undefined) const color = promptState === PromptState.Error ? 'red' : 'cyan' const underline = new Array(oneThird - 3).fill('▔') @@ -67,12 +67,12 @@ const DangerousConfirmationPrompt: FunctionComponent { if (promptState === PromptState.Submitted) { onSubmit(true) - unmountInk() + complete() } else if (promptState === PromptState.Cancelled) { onSubmit(false) - unmountInk() + complete() } - }, [onSubmit, promptState, unmountInk]) + }, [onSubmit, promptState, complete]) const completed = promptState === PromptState.Submitted || promptState === PromptState.Cancelled diff --git a/packages/cli-kit/src/private/node/ui/components/SelectPrompt.tsx b/packages/cli-kit/src/private/node/ui/components/SelectPrompt.tsx index 8e1606efb37..0794bf447c5 100644 --- a/packages/cli-kit/src/private/node/ui/components/SelectPrompt.tsx +++ b/packages/cli-kit/src/private/node/ui/components/SelectPrompt.tsx @@ -3,10 +3,10 @@ import {InfoTableProps} from './Prompts/InfoTable.js' import {InfoMessageProps} from './Prompts/InfoMessage.js' import {Message, PromptLayout} from './Prompts/PromptLayout.js' import {AbortSignal} from '../../../../public/node/abort.js' +import {useComplete} from '../../ui.js' import usePrompt, {PromptState} from '../hooks/use-prompt.js' import React, {ReactElement, useCallback, useEffect} from 'react' -import {useApp} from 'ink' export interface SelectPromptProps { message: Message @@ -32,7 +32,7 @@ function SelectPrompt({ if (choices.length === 0) { throw new Error('SelectPrompt requires at least one choice') } - const {exit: unmountInk} = useApp() + const complete = useComplete() const {promptState, setPromptState, answer, setAnswer} = usePrompt | undefined>({ initialAnswer: undefined, }) @@ -47,10 +47,10 @@ function SelectPrompt({ useEffect(() => { if (promptState === PromptState.Submitted && answer) { - unmountInk() onSubmit(answer.value) + complete() } - }, [answer, onSubmit, promptState, unmountInk]) + }, [answer, onSubmit, promptState, complete]) return ( { title: TokenizedString @@ -16,7 +16,8 @@ interface SingleTaskProps { const SingleTask = ({task, title, onComplete, onAbort, noColor}: SingleTaskProps) => { const [status, setStatus] = useState(title) const [isDone, setIsDone] = useState(false) - const {exit: unmountInk} = useApp() + const [taskResult, setTaskResult] = useState<{error?: Error} | null>(null) + const complete = useComplete() const {isRawModeSupported} = useStdin() useInput( @@ -35,13 +36,19 @@ const SingleTask = ({task, title, onComplete, onAbort, noColor}: SingleTaskP .then((result) => { setIsDone(true) onComplete?.(result) - unmountInk() + setTaskResult({}) }) .catch((error) => { setIsDone(true) - unmountInk(error) + setTaskResult({error}) }) - }, [task, unmountInk, onComplete]) + }, [task, onComplete]) + + useEffect(() => { + if (taskResult !== null) { + complete(taskResult.error) + } + }, [taskResult, complete]) if (isDone) { // clear things once done diff --git a/packages/cli-kit/src/private/node/ui/components/TextPrompt.tsx b/packages/cli-kit/src/private/node/ui/components/TextPrompt.tsx index d9d94e586da..6e398a324a9 100644 --- a/packages/cli-kit/src/private/node/ui/components/TextPrompt.tsx +++ b/packages/cli-kit/src/private/node/ui/components/TextPrompt.tsx @@ -1,6 +1,6 @@ import {InlineToken, TokenItem, TokenizedText} from './TokenizedText.js' import {TextInput} from './TextInput.js' -import {handleCtrlC} from '../../ui.js' +import {handleCtrlC, useComplete} from '../../ui.js' import useLayout from '../hooks/use-layout.js' import {messageWithPunctuation} from '../utilities.js' import {AbortSignal} from '../../../../public/node/abort.js' @@ -8,7 +8,7 @@ import useAbortSignal from '../hooks/use-abort-signal.js' import usePrompt, {PromptState} from '../hooks/use-prompt.js' import React, {FunctionComponent, useCallback, useEffect, useState} from 'react' -import {Box, useApp, useInput, Text} from 'ink' +import {Box, useInput, Text} from 'ink' import figures from 'figures' export interface TextPromptProps { @@ -60,7 +60,7 @@ const TextPrompt: FunctionComponent = ({ const answerOrDefault = answer.length > 0 ? answer : defaultValue const displayEmptyValue = answerOrDefault === '' const displayedAnswer = displayEmptyValue ? emptyDisplayedValue : answerOrDefault - const {exit: unmountInk} = useApp() + const complete = useComplete() const [error, setError] = useState(undefined) const color = promptState === PromptState.Error ? 'red' : 'cyan' const underline = new Array(oneThird - 3).fill('▔') @@ -84,9 +84,9 @@ const TextPrompt: FunctionComponent = ({ useEffect(() => { if (promptState === PromptState.Submitted) { onSubmit(answerOrDefault) - unmountInk() + complete() } - }, [answerOrDefault, onSubmit, promptState, unmountInk]) + }, [answerOrDefault, onSubmit, promptState, complete]) return isAborted ? null : ( diff --git a/packages/cli-kit/src/private/node/ui/hooks/use-abort-signal.ts b/packages/cli-kit/src/private/node/ui/hooks/use-abort-signal.ts index 3cd216ad98f..a2b782b21ed 100644 --- a/packages/cli-kit/src/private/node/ui/hooks/use-abort-signal.ts +++ b/packages/cli-kit/src/private/node/ui/hooks/use-abort-signal.ts @@ -1,32 +1,28 @@ import {AbortSignal} from '../../../../public/node/abort.js' -import {useApp} from 'ink' -import {useLayoutEffect, useState} from 'react' +import {useComplete} from '../../ui.js' +import {useEffect, useLayoutEffect, useState} from 'react' const noop = () => Promise.resolve() export default function useAbortSignal(abortSignal?: AbortSignal, onAbort: (error?: unknown) => Promise = noop) { - const {exit: unmountInk} = useApp() + const complete = useComplete() const [isAborted, setIsAborted] = useState(false) useLayoutEffect(() => { abortSignal?.addEventListener('abort', () => { const abortWithError = abortSignal.reason.message === 'AbortError' ? undefined : abortSignal.reason onAbort(abortWithError) - .then(() => { - setIsAborted(true) - // Defer unmounting to the next setImmediate so React 19 can flush - // batched state updates before the tree is torn down. React 19's - // scheduler also uses setImmediate in Node.js (check phase), and - // since it was queued first (by setIsAborted above), it renders - // before this callback fires (FIFO within the check phase). - // NOTE: setTimeout(fn, 0) is NOT safe here because its timers-phase - // fires BEFORE the check phase on slow CI machines where >1 ms has - // elapsed, causing unmount to race ahead of the render. - setImmediate(() => unmountInk(abortWithError)) - }) + .then(() => setIsAborted(true)) .catch(() => {}) }) }, []) + useEffect(() => { + if (isAborted) { + const abortWithError = abortSignal?.reason?.message === 'AbortError' ? undefined : abortSignal?.reason + complete(abortWithError) + } + }, [isAborted]) + return {isAborted} } diff --git a/packages/cli-kit/src/private/node/ui/hooks/use-async-and-unmount.ts b/packages/cli-kit/src/private/node/ui/hooks/use-async-and-unmount.ts index d2ed970f157..9ba9e5ef26e 100644 --- a/packages/cli-kit/src/private/node/ui/hooks/use-async-and-unmount.ts +++ b/packages/cli-kit/src/private/node/ui/hooks/use-async-and-unmount.ts @@ -1,5 +1,5 @@ -import {useApp} from 'ink' -import {useEffect} from 'react' +import {useComplete} from '../../ui.js' +import {useEffect, useState} from 'react' interface Options { onFulfilled?: () => unknown @@ -10,17 +10,24 @@ export default function useAsyncAndUnmount( asyncFunction: () => Promise, {onFulfilled = () => {}, onRejected = () => {}}: Options = {}, ) { - const {exit: unmountInk} = useApp() + const complete = useComplete() + const [result, setResult] = useState<{error?: Error} | null>(null) useEffect(() => { asyncFunction() .then(() => { onFulfilled() - unmountInk() + setResult({}) }) .catch((error) => { onRejected(error) - unmountInk(error) + setResult({error}) }) }, []) + + useEffect(() => { + if (result !== null) { + complete(result.error) + } + }, [result]) } From 732c4fc8477cf80825d16e6f43b253aed66cbfbc Mon Sep 17 00:00:00 2001 From: Ryan Bahan Date: Wed, 1 Apr 2026 19:18:32 -0600 Subject: [PATCH 2/3] fix: update tests for new completion lifecycle - ConcurrentOutput tests: use gate pattern to keep processes alive during assertions instead of relying on setImmediate timing - Downstream tests (init, theme-downloader, theme-app-extension): mock renderTasks so business logic tests don't spin up real Ink instances Co-Authored-By: Claude Opus 4.6 (1M context) --- .../dev/processes/theme-app-extension.test.ts | 13 ++++++-- .../ui/components/ConcurrentOutput.test.tsx | 32 ++++++++++++++++--- packages/theme/src/cli/services/init.test.ts | 7 ++++ .../cli/utilities/theme-downloader.test.ts | 13 ++++++++ 4 files changed, 59 insertions(+), 6 deletions(-) diff --git a/packages/app/src/cli/services/dev/processes/theme-app-extension.test.ts b/packages/app/src/cli/services/dev/processes/theme-app-extension.test.ts index fad6a5dc1c2..bf9e0d15b01 100644 --- a/packages/app/src/cli/services/dev/processes/theme-app-extension.test.ts +++ b/packages/app/src/cli/services/dev/processes/theme-app-extension.test.ts @@ -22,9 +22,18 @@ vi.mock('@shopify/cli-kit/node/themes/api') vi.mock('@shopify/cli-kit/node/context/fqdn') vi.mock('@shopify/cli-kit/node/ui', async (realImport) => { const realModule = await realImport() - const mockModule = {renderInfo: vi.fn()} - return {...realModule, ...mockModule} + return { + ...realModule, + renderInfo: vi.fn(), + renderTasks: vi.fn(async (tasks: any[]) => { + for (const task of tasks) { + // eslint-disable-next-line no-await-in-loop + await task.task({}, task) + } + return {} + }), + } }) describe('setupPreviewThemeAppExtensionsProcess', () => { diff --git a/packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.test.tsx b/packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.test.tsx index f9d61c55556..d0978a81a0b 100644 --- a/packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.test.tsx +++ b/packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.test.tsx @@ -28,6 +28,7 @@ describe('ConcurrentOutput', () => { // Given const backendSync = new Synchronizer() const frontendSync = new Synchronizer() + const gate = new Synchronizer() const backendProcess = { prefix: 'backend', @@ -37,6 +38,7 @@ describe('ConcurrentOutput', () => { stdout.write('third backend message') backendSync.resolve() + await gate.promise }, } @@ -50,6 +52,7 @@ describe('ConcurrentOutput', () => { stdout.write('third frontend message') frontendSync.resolve() + await gate.promise }, } // When @@ -72,6 +75,8 @@ describe('ConcurrentOutput', () => { 00:00:00 │ frontend │ third frontend message " `) + + gate.resolve() }) test('strips ansi codes from the output by default', async () => { @@ -79,12 +84,14 @@ describe('ConcurrentOutput', () => { // Given const processSync = new Synchronizer() + const gate = new Synchronizer() const processes = [ { prefix: '1', action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { stdout.write(`\u001b[32m${output}\u001b[39m`) processSync.resolve() + await gate.promise }, }, ] @@ -98,6 +105,7 @@ describe('ConcurrentOutput', () => { const logColumns = renderInstance.lastFrame()!.split('│') expect(logColumns.length).toBe(3) expect(logColumns[2]?.trim()).toEqual(output) + gate.resolve() }) test('does not strip ansi codes from the output when stripAnsi is false', async () => { @@ -105,6 +113,7 @@ describe('ConcurrentOutput', () => { // Given const processSync = new Synchronizer() + const gate = new Synchronizer() const processes = [ { prefix: '1', @@ -113,6 +122,7 @@ describe('ConcurrentOutput', () => { stdout.write(output) }) processSync.resolve() + await gate.promise }, }, ] @@ -126,11 +136,13 @@ describe('ConcurrentOutput', () => { const logColumns = renderInstance.lastFrame()!.split('│') expect(logColumns.length).toBe(3) expect(logColumns[2]?.trim()).toEqual(output) + gate.resolve() }) test('renders custom prefixes on log lines', async () => { // Given const processSync = new Synchronizer() + const gate = new Synchronizer() const extensionName = 'my-extension' const processes = [ { @@ -140,6 +152,7 @@ describe('ConcurrentOutput', () => { stdout.write('foo bar') }) processSync.resolve() + await gate.promise }, }, ] @@ -161,12 +174,14 @@ describe('ConcurrentOutput', () => { const logColumns = unstyled(renderInstance.lastFrame()!).split('│') expect(logColumns.length).toBe(3) expect(logColumns[1]?.trim()).toEqual(extensionName) + gate.resolve() }) test('renders prefix column width based on prefixColumnSize', async () => { // Given const processSync1 = new Synchronizer() const processSync2 = new Synchronizer() + const gate = new Synchronizer() const columnSize = 5 const processes = [ @@ -175,6 +190,7 @@ describe('ConcurrentOutput', () => { action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { stdout.write('foo') processSync1.resolve() + await gate.promise }, }, { @@ -182,6 +198,7 @@ describe('ConcurrentOutput', () => { action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { stdout.write('bar') processSync2.resolve() + await gate.promise }, }, ] @@ -206,22 +223,25 @@ describe('ConcurrentOutput', () => { // Including spacing expect(logColumns[1]?.length).toBe(columnSize + 2) }) + gate.resolve() }) test('renders prefix column width based on processes by default', async () => { // Given const processSync = new Synchronizer() + const gate = new Synchronizer() const processes = [ { prefix: '1', action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { stdout.write('foo') processSync.resolve() + await gate.promise }, }, - {prefix: '12', action: async () => {}}, - {prefix: '123', action: async () => {}}, - {prefix: '1234', action: async () => {}}, + {prefix: '12', action: async () => gate.promise}, + {prefix: '123', action: async () => gate.promise}, + {prefix: '1234', action: async () => gate.promise}, ] // When @@ -234,20 +254,23 @@ describe('ConcurrentOutput', () => { expect(logColumns.length).toBe(3) // 4 is largest prefix, plus spacing expect(logColumns[1]?.length).toBe(4 + 2) + gate.resolve() }) test('does not render prefix column larger than max', async () => { // Given const processSync = new Synchronizer() + const gate = new Synchronizer() const processes = [ { prefix: '1', action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { stdout.write('foo') processSync.resolve() + await gate.promise }, }, - {prefix: new Array(26).join('0'), action: async () => {}}, + {prefix: new Array(26).join('0'), action: async () => gate.promise}, ] // When @@ -260,6 +283,7 @@ describe('ConcurrentOutput', () => { expect(logColumns.length).toBe(3) // 25 is largest column allowed, plus spacing expect(logColumns[1]?.length).toBe(25 + 2) + gate.resolve() }) test('rejects with the error thrown inside one of the processes', async () => { diff --git a/packages/theme/src/cli/services/init.test.ts b/packages/theme/src/cli/services/init.test.ts index eb79893e22f..1f8466ebc44 100644 --- a/packages/theme/src/cli/services/init.test.ts +++ b/packages/theme/src/cli/services/init.test.ts @@ -27,6 +27,13 @@ vi.mock('@shopify/cli-kit/node/ui', async () => { return { ...actual, renderSelectPrompt: vi.fn(), + renderTasks: vi.fn(async (tasks: any[]) => { + for (const task of tasks) { + // eslint-disable-next-line no-await-in-loop + await task.task({}, task) + } + return {} + }), } }) diff --git a/packages/theme/src/cli/utilities/theme-downloader.test.ts b/packages/theme/src/cli/utilities/theme-downloader.test.ts index 530b7434f88..6bb6fe27657 100644 --- a/packages/theme/src/cli/utilities/theme-downloader.test.ts +++ b/packages/theme/src/cli/utilities/theme-downloader.test.ts @@ -6,6 +6,19 @@ import {test, describe, expect, vi} from 'vitest' vi.mock('./theme-fs.js') vi.mock('@shopify/cli-kit/node/themes/api') +vi.mock('@shopify/cli-kit/node/ui', async () => { + const actual = await vi.importActual('@shopify/cli-kit/node/ui') + return { + ...actual, + renderTasks: vi.fn(async (tasks: any[]) => { + for (const task of tasks) { + // eslint-disable-next-line no-await-in-loop + await task.task({}, task) + } + return {} + }), + } +}) describe('theme-downloader', () => { describe('downloadTheme', () => { From d67a8ae5ef2a086b03219f1691e455c65cbdf272 Mon Sep 17 00:00:00 2001 From: Ryan Bahan Date: Wed, 1 Apr 2026 19:54:12 -0600 Subject: [PATCH 3/3] fix: address review feedback - useComplete() throws if called outside InkLifecycleRoot instead of silently returning a no-op that hangs - Test render helper wraps rerenders with InkLifecycleRoot so useComplete() works after rerender - Add sequential render regression test verifying no teardown interleaving after setImmediate removal Co-Authored-By: Claude Opus 4.6 (1M context) --- .../cli-kit/src/private/node/testing/ui.ts | 2 +- packages/cli-kit/src/private/node/ui.tsx | 13 ++++++--- packages/cli-kit/src/public/node/ui.test.ts | 28 +++++++++++++++++++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/packages/cli-kit/src/private/node/testing/ui.ts b/packages/cli-kit/src/private/node/testing/ui.ts index 4278cbc74c2..76f31f36b69 100644 --- a/packages/cli-kit/src/private/node/testing/ui.ts +++ b/packages/cli-kit/src/private/node/testing/ui.ts @@ -78,7 +78,7 @@ export const render = (tree: ReactElement, options: RenderOptions = {}): Instanc }) return { - rerender: instance.rerender, + rerender: (tree: ReactElement) => instance.rerender(React.createElement(InkLifecycleRoot, null, tree)), unmount: instance.unmount, cleanup: instance.cleanup, waitUntilExit: () => trackPromise(instance.waitUntilExit().then(() => {})), diff --git a/packages/cli-kit/src/private/node/ui.tsx b/packages/cli-kit/src/private/node/ui.tsx index d4c3561d3b3..d7cc567d0aa 100644 --- a/packages/cli-kit/src/private/node/ui.tsx +++ b/packages/cli-kit/src/private/node/ui.tsx @@ -8,14 +8,19 @@ import {Key, render as inkRender, RenderOptions, useApp} from 'ink' import {EventEmitter} from 'events' -const CompletionContext = createContext<(error?: Error) => void>(() => {}) +const CompletionContext = createContext<((error?: Error) => void) | null>(null) /** - * Signal that the current Ink tree is done. The root wrapper will call - * `exit()` after React finishes rendering. + * Signal that the current Ink tree is done. Must be called within an + * InkLifecycleRoot — throws if the provider is missing so lifecycle + * bugs surface immediately instead of silently hanging. */ export function useComplete(): (error?: Error) => void { - return useContext(CompletionContext) + const complete = useContext(CompletionContext) + if (!complete) { + throw new Error('useComplete() called outside InkLifecycleRoot') + } + return complete } /** diff --git a/packages/cli-kit/src/public/node/ui.test.ts b/packages/cli-kit/src/public/node/ui.test.ts index f119b5de08c..a4dc1797043 100644 --- a/packages/cli-kit/src/public/node/ui.test.ts +++ b/packages/cli-kit/src/public/node/ui.test.ts @@ -504,3 +504,31 @@ describe('renderSingleTask', async () => { expect(result3).toBe('result3') }) }) + +describe('sequential renders', () => { + test('consecutive renders do not interleave or leak teardown output', async () => { + const output = mockAndCaptureOutput() + + await renderTasks([ + { + title: 'First batch', + task: async () => { + await new Promise((resolve) => setTimeout(resolve, 50)) + }, + }, + ]) + + await renderSingleTask({ + title: new TokenizedString('Second batch'), + task: async () => { + await new Promise((resolve) => setTimeout(resolve, 50)) + return 'done' + }, + }) + + // The key assertion: no interleaving. The second render's output should + // not contain fragments from the first render's teardown. + const frames = output.output() + expect(frames).not.toContain('First batch') + }) +})