From 3fbe8e914dd37aa0d3b67416a187aab14655ece8 Mon Sep 17 00:00:00 2001 From: Ryan Bahan Date: Tue, 7 Apr 2026 15:39:45 -0600 Subject: [PATCH 1/3] Reapply "use central unmount for ink render tree; stop messing with node's event loop" This reverts commit f3a5b94094a2aa669d283a574c486282af2d3cbf. --- .../dev/processes/theme-app-extension.test.ts | 13 +++++- .../cli-kit/src/private/node/testing/ui.ts | 8 ++-- packages/cli-kit/src/private/node/ui.tsx | 46 +++++++++++++++++-- .../node/ui/components/AutocompletePrompt.tsx | 9 ++-- .../ui/components/ConcurrentOutput.test.tsx | 32 +++++++++++-- .../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 +++++-- packages/cli-kit/src/public/node/ui.test.ts | 28 +++++++++++ packages/theme/src/cli/services/init.test.ts | 7 +++ .../cli/utilities/theme-downloader.test.ts | 13 ++++++ 15 files changed, 201 insertions(+), 70 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/testing/ui.ts b/packages/cli-kit/src/private/node/testing/ui.ts index 58b8a7df29c..76f31f36b69 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), @@ -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 06a91c7ba45..d7cc567d0aa 100644 --- a/packages/cli-kit/src/private/node/ui.tsx +++ b/packages/cli-kit/src/private/node/ui.tsx @@ -3,11 +3,49 @@ 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) | null>(null) + +/** + * 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 { + const complete = useContext(CompletionContext) + if (!complete) { + throw new Error('useComplete() called outside InkLifecycleRoot') + } + return complete +} + +/** + * 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 +65,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.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/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]) } 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') + }) +}) 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 128f408689b526359bee6972d1427fdeea62a1d7 Mon Sep 17 00:00:00 2001 From: Ryan Bahan Date: Tue, 7 Apr 2026 16:14:04 -0600 Subject: [PATCH 2/3] update task rendering --- packages/cli-kit/src/public/node/ui.test.ts | 13 ++++---- packages/cli-kit/src/public/node/ui.tsx | 37 +++++++++++++++------ 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/packages/cli-kit/src/public/node/ui.test.ts b/packages/cli-kit/src/public/node/ui.test.ts index a4dc1797043..901870f83b4 100644 --- a/packages/cli-kit/src/public/node/ui.test.ts +++ b/packages/cli-kit/src/public/node/ui.test.ts @@ -485,18 +485,17 @@ describe('renderSingleTask', async () => { await expect(renderSingleTask({title, task})).rejects.toThrow('Delayed failure') }) - test('handles concurrent single tasks', async () => { + test('handles sequential single tasks', async () => { // Given const task1 = () => new Promise((resolve) => setTimeout(() => resolve('result1'), 50)) const task2 = () => new Promise((resolve) => setTimeout(() => resolve('result2'), 100)) const task3 = () => new Promise((resolve) => setTimeout(() => resolve('result3'), 25)) - // When - const [result1, result2, result3] = await Promise.all([ - renderSingleTask({title: new TokenizedString('Task 1'), task: task1}), - renderSingleTask({title: new TokenizedString('Task 2'), task: task2}), - renderSingleTask({title: new TokenizedString('Task 3'), task: task3}), - ]) + // When — ink only supports one render instance per stdout at a time, + // so sequential execution is the correct pattern + const result1 = await renderSingleTask({title: new TokenizedString('Task 1'), task: task1}) + const result2 = await renderSingleTask({title: new TokenizedString('Task 2'), task: task2}) + const result3 = await renderSingleTask({title: new TokenizedString('Task 3'), task: task3}) // Then expect(result1).toBe('result1') diff --git a/packages/cli-kit/src/public/node/ui.tsx b/packages/cli-kit/src/public/node/ui.tsx index 42e08e9b3d7..6a291287c2d 100644 --- a/packages/cli-kit/src/public/node/ui.tsx +++ b/packages/cli-kit/src/public/node/ui.tsx @@ -487,14 +487,21 @@ export async function renderTasks( tasks: Task[], {renderOptions, noProgressBar}: RenderTasksOptions = {}, ) { - return new Promise((resolve, reject) => { - render(, { + let taskResult: TContext + await render( + { + taskResult = ctx + }} + noProgressBar={noProgressBar} + />, + { ...renderOptions, exitOnCtrlC: false, - }) - .then(() => {}) - .catch(reject) - }) + }, + ) + return taskResult! } export interface RenderSingleTaskOptions { @@ -521,12 +528,22 @@ export async function renderSingleTask({ onAbort, renderOptions, }: RenderSingleTaskOptions): Promise { - return new Promise((resolve, reject) => { - render(, { + let taskResult: T + await render( + { + taskResult = result + }} + onAbort={onAbort} + />, + { ...renderOptions, exitOnCtrlC: false, - }).catch(reject) - }) + }, + ) + return taskResult! } export interface RenderTextPromptOptions extends Omit { From 9219ad1aae1c7e29f147e4cb149aa85d0c06d52b Mon Sep 17 00:00:00 2001 From: Ryan Bahan Date: Tue, 7 Apr 2026 16:17:17 -0600 Subject: [PATCH 3/3] fix render lifecycle race: await ink unmount before resolving renderTasks and renderSingleTask resolved their promises via onComplete={resolve}, which fires inside the React tree before ink unmounts. This caused the caller to write to stdout while ink was still tearing down, resulting in loading bars staying visible and last lines of output being cut off. Changed both functions to await render() (which awaits waitUntilExit()) before returning, matching the pattern the prompt functions already use. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/cli-kit/src/public/node/ui.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli-kit/src/public/node/ui.tsx b/packages/cli-kit/src/public/node/ui.tsx index 6a291287c2d..a6fdb7797c0 100644 --- a/packages/cli-kit/src/public/node/ui.tsx +++ b/packages/cli-kit/src/public/node/ui.tsx @@ -486,7 +486,7 @@ interface RenderTasksOptions { export async function renderTasks( tasks: Task[], {renderOptions, noProgressBar}: RenderTasksOptions = {}, -) { +): Promise { let taskResult: TContext await render(