Revert "use central unmount for ink render tree; stop messing with node's event loop"#7157
Conversation
|
/snapit |
|
🫰✨ Thanks @isaacroldan! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260402103723Caution After installing, validate the version by running |
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/private/node/ui.d.ts import { Logger, LogLevel } from '../../public/node/output.js';
-import React from 'react';
import { Key, RenderOptions } from 'ink';
import { EventEmitter } from 'events';
-/**
- * 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 declare function useComplete(): (error?: Error) => void;
-/**
- * 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 declare function InkLifecycleRoot({ children }: {
- children: React.ReactNode;
-}): React.JSX.Element;
interface RenderOnceOptions {
logLevel?: LogLevel;
logger?: Logger;
renderOptions?: RenderOptions;
}
export declare function renderOnce(element: JSX.Element, { logLevel, renderOptions }: RenderOnceOptions): string | undefined;
-export declare function render(element: JSX.Element, options?: RenderOptions): Promise<void>;
+export declare function render(element: JSX.Element, options?: RenderOptions): Promise<unknown>;
export declare class Stdout extends EventEmitter {
columns: number;
rows: number;
readonly frames: string[];
private _lastFrame?;
constructor(options: {
columns?: number;
rows?: number;
});
write: (frame: string) => void;
lastFrame: () => string | undefined;
}
export declare function handleCtrlC(input: string, key: Key, exit?: () => void): void;
export {};
packages/cli-kit/dist/public/node/ui.d.ts@@ -34,7 +34,7 @@ export interface RenderConcurrentOptions extends PartialBy<ConcurrentOutputProps
* 00:00:00 │ frontend │ third frontend message
*
*/
-export declare function renderConcurrent({ renderOptions, ...props }: RenderConcurrentOptions): Promise<void>;
+export declare function renderConcurrent({ renderOptions, ...props }: RenderConcurrentOptions): Promise<unknown>;
export type AlertCustomSection = CustomSection;
export type RenderAlertOptions = Omit<AlertOptions, 'type'>;
/**
|
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
There was a problem hiding this comment.
Pull request overview
This PR reverts #7153, removing the “centralized unmount” Ink lifecycle wrapper and returning to component-driven useApp().exit() teardown, with additional event-loop yielding to avoid React/Ink teardown timing issues.
Changes:
- Remove
InkLifecycleRoot/useCompleteand switch UI components/hooks back to callinguseApp().exit()directly. - Add event-loop yielding after
waitUntilExit()and adjust unmount deferrals in a few components/hooks. - Simplify several unit-test mocks and remove the sequential-render regression test.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/theme/src/cli/utilities/theme-downloader.test.ts | Removes UI module mocking previously used to override renderTasks. |
| packages/theme/src/cli/services/init.test.ts | Stops mocking renderTasks while keeping other UI mocks. |
| packages/cli-kit/src/public/node/ui.test.ts | Removes sequential-render regression coverage. |
| packages/cli-kit/src/private/node/ui/hooks/use-async-and-unmount.ts | Switches from completion-context signaling to direct Ink exit(). |
| packages/cli-kit/src/private/node/ui/hooks/use-abort-signal.ts | Switches to direct Ink exit(), with deferred unmounting. |
| packages/cli-kit/src/private/node/ui/components/TextPrompt.tsx | Replaces useComplete() with useApp().exit(). |
| packages/cli-kit/src/private/node/ui/components/SingleTask.tsx | Replaces useComplete() with useApp().exit() on completion. |
| packages/cli-kit/src/private/node/ui/components/SelectPrompt.tsx | Replaces useComplete() with useApp().exit() on submit. |
| packages/cli-kit/src/private/node/ui/components/DangerousConfirmationPrompt.tsx | Replaces useComplete() with useApp().exit() on submit/cancel. |
| packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.tsx | Replaces completion-context teardown with deferred useApp().exit(). |
| packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.test.tsx | Removes gating synchronizers and relies on render-wait helpers. |
| packages/cli-kit/src/private/node/ui/components/AutocompletePrompt.tsx | Replaces useComplete() with useApp().exit() on submit. |
| packages/cli-kit/src/private/node/ui.tsx | Removes lifecycle wrapper usage; adds a setImmediate yield after waitUntilExit(). |
| packages/cli-kit/src/private/node/testing/ui.ts | Removes test render wrapping with the lifecycle root. |
| packages/app/src/cli/services/dev/processes/theme-app-extension.test.ts | Simplifies UI mocks to only override renderInfo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| task(setStatus) | ||
| .then((result) => { | ||
| setIsDone(true) | ||
| onComplete?.(result) | ||
| setTaskResult({}) | ||
| unmountInk() | ||
| }) | ||
| .catch((error) => { | ||
| setIsDone(true) | ||
| setTaskResult({error}) | ||
| unmountInk(error) | ||
| }) |
There was a problem hiding this comment.
setIsDone(true) is intended to clear the loading bar by rendering null, but unmountInk() is invoked in the same microtask. With React 19 batching, the final isDone render can be skipped/raced by teardown, leaving the spinner as the last frame in some environments. Consider deferring unmountInk() (similar to useAbortSignal / ConcurrentOutput) or moving the exit() call into a useEffect that runs after isDone becomes true.
| @@ -504,31 +504,3 @@ describe('renderSingleTask', async () => { | |||
| expect(result3).toBe('result3') | |||
| }) | |||
| }) | |||
There was a problem hiding this comment.
This PR removes the only test covering sequential/serial Ink renders not interleaving or leaking teardown output. Since this change reverts the centralized unmount behavior, it would be good to keep equivalent coverage (either restore this test or add a new assertion elsewhere) to prevent regressions where teardown output pollutes subsequent renders.
| }) | |
| }) | |
| describe('sequential Ink renders', async () => { | |
| test('do not interleave or leak teardown output between renderSingleTask calls', async () => { | |
| // Given | |
| const outputMock = mockAndCaptureOutput() | |
| const firstTask = async () => 'first' | |
| const secondTask = async () => 'second' | |
| // When | |
| await renderSingleTask({title: new TokenizedString('First task'), task: firstTask}) | |
| await renderSingleTask({title: new TokenizedString('Second task'), task: secondTask}) | |
| // Then | |
| const {stdout} = outputMock.output() | |
| const firstIndex = stdout.indexOf('First task') | |
| const secondIndex = stdout.indexOf('Second task') | |
| expect(firstIndex).not.toEqual(-1) | |
| expect(secondIndex).not.toEqual(-1) | |
| expect(firstIndex).toBeLessThan(secondIndex) | |
| }) | |
| }) |
| asyncFunction() | ||
| .then(() => { | ||
| onFulfilled() | ||
| setResult({}) | ||
| unmountInk() | ||
| }) | ||
| .catch((error) => { | ||
| onRejected(error) | ||
| setResult({error}) | ||
| unmountInk(error) | ||
| }) |
There was a problem hiding this comment.
unmountInk() is called immediately after onFulfilled() / onRejected(). Callers such as Tasks trigger state updates in these callbacks; exiting in the same tick can prevent React/Ink from flushing the final render before teardown (the same class of race mitigated elsewhere in this PR via setImmediate). Consider deferring unmountInk (e.g., via setImmediate or a state+useEffect teardown) so batched updates from onFulfilled/onRejected render before exit.
| abortSignal?.addEventListener('abort', () => { | ||
| const abortWithError = abortSignal.reason.message === 'AbortError' ? undefined : abortSignal.reason | ||
| onAbort(abortWithError) | ||
| .then(() => setIsAborted(true)) | ||
| .then(() => { |
There was a problem hiding this comment.
abortSignal.reason is treated as an object with a stable .message === 'AbortError'. In practice reason can be non-Error (e.g., strings) and even when it is an Error/DOMException the name is the stable discriminator (the message can vary). Consider normalizing reason (e.g., check reason instanceof Error and/or reason?.name === 'AbortError') and only passing an actual Error | undefined to exit() to avoid brittle behavior and inconsistent waitUntilExit() rejection semantics.
Reverts #7153