use central unmount for ink render tree; stop messing with node's event loop by ryancbahan · Pull Request #7153 · Shopify/cli

ryancbahan · 2026-04-01T23:53:50Z

WHY are these changes introduced?

The Ink UI layer accumulated setImmediate(() => setTimeout(() => unmountInk())) workarounds to deal with React 19's concurrent scheduler. Components were calling useApp().exit() from .then() callbacks and event listeners — outside React's render cycle — which raced with the batch flush, tearing down the tree before the final state rendered (e.g. loading bars persisting in the terminal).

This resulted in components calling exit at the wrong point in React's lifecycle. The workarounds tried to manually yield to the event loop to let React catch up. This was fragile and forced us to mess with Node.js scheduler internals.

Demo fix: kitchen sink no longer double-renders:
Screen Recording 2026-04-01 at 5.52.39 PM.mov (uploaded via Graphite)

WHAT is this pull request doing?

Introduces InkLifecycleRoot, a single root wrapper component that is the only place useApp().exit() is ever called. Components signal completion through React context (useComplete()) — a normal state change, not an imperative unmount.

The exit chain follows React's lifecycle:

Component reaches terminal state → sets state
React renders the terminal state (flushing batched updates)
Component's useEffect fires post-render → calls complete()
Root wrapper's state updates
Root wrapper's useEffect fires → calls exit()

This guarantees all state is flushed before teardown.

Measuring impact

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes

ryancbahan · 2026-04-01T23:54:04Z

use central unmount for ink render tree; stop messing with node's event loop #7153 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2026-04-02T00:02:04Z

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

dmerand

My AI pal who's been looking at Ink/React has some thoughts on the design here. I'm still not an expert in Ink/React lifecycle, but I left the stuff worth sharing/considering.

packages/cli-kit/src/private/node/testing/ui.ts

packages/cli-kit/src/private/node/ui.tsx

dmerand · 2026-04-02T00:48:07Z

packages/cli-kit/src/private/node/ui.tsx

+ * 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 <CompletionContext.Provider value={complete}>{children}</CompletionContext.Provider>
+}
+
 interface RenderOnceOptions {


🎉 Praise: This is the first change in this area that feels like it's addressing the root architectural problem instead of compensating for it.

Moving exit() ownership into a single root effect is a much better model than continuing to let leaf components imperatively tear down Ink from async callbacks / event handlers. Even if we still need to harden the contract a bit, this is the right direction.

dmerand · 2026-04-02T00:48:07Z

packages/cli-kit/src/private/node/ui.tsx

+  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 <CompletionContext.Provider value={complete}>{children}</CompletionContext.Provider>
+}


🏗️ Design: The root still needs an explicit completion policy.

Right now complete(error?) just updates exitResult, which means multiple terminal signals can race and overwrite one another. In practice I could imagine:

normal success

abort

error

all trying to complete the same tree from different effects. If that happens, the final exit outcome becomes timing-dependent again — just at the root instead of in leaf components.

Suggestion: Encode an explicit policy — either first terminal signal wins, or error/abort beats success, etc. Even a simple "first write wins" guard would make this much easier to reason about:

const complete = useCallback((error?: Error) => { setExitResult((prev) => prev ?? {error}) }, [])

Whichever policy we want, it should be encoded here rather than left to update ordering.

I can't make heads or tails of this comment. wrapping the state change in a useCallback seems like arbitrary complexity. I think the pattern here is correct.

packages/cli-kit/src/private/node/ui.tsx

dmerand · 2026-04-02T00:48:07Z

packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.tsx

+          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



💡 Improvement: This is a much healthier direction than the previous setImmediate(() => setTimeout(...)) pattern.

The main thing missing now is direct regression coverage for terminal-state ordering. Since completion is now mediated through state/effects, we need tests that exercise the exact cases we're trying to stabilize:

success renders final state before exit

error renders final state before exit

abort renders final shutdown state before exit

abort racing with natural completion resolves deterministically

Suggestion: Prioritize one ConcurrentOutput test and one abort-vs-completion test, since those seem like the highest-risk paths historically. Without that, we may still have a timing bug here — just a different one than before.

- 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) <noreply@anthropic.com>

ryancbahan · 2026-04-02T01:27:37Z

packages/app/src/cli/services/dev/processes/theme-app-extension.test.ts


-  return {...realModule, ...mockModule}
+  return {
+    ...realModule,


this will stabilize theme app timeouts. previously only part of the render flow was mocked, but ink/react isn't fully set up in the test env, so we were getting timeouts. making renderTasks async mimics the existing concurrentoutput/etc behavior.

ryancbahan · 2026-04-02T01:33:36Z

packages/cli-kit/src/private/node/ui.tsx

+    setExitResult({error})
+  }, [])
+
+  useEffect(() => {


this still feels incredibly cursed. the ink docs mention it's a common case, but i want to keep thinking about this (or remove ink). imperatively managing unmount is exactly the thing i don't want to do if i have react. but at least for now it removes the mnual event loop ticks and race conditions

ryancbahan · 2026-04-02T01:38:12Z

packages/cli-kit/src/private/node/ui.tsx

+ * fires post-render, guaranteeing all batched state updates have been flushed
+ * before the tree is torn down.
+ */
+export function InkLifecycleRoot({children}: {children: React.ReactNode}) {


i don't know ink that well, but i am hoping there's a better way to do this. having to manually manage teardown

- 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) <noreply@anthropic.com>

github-actions · 2026-04-02T01:56:15Z

Differences in type declarations

We 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:

Some seemingly private modules might be re-exported through public modules.
If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/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<unknown>;
+export declare function render(element: JSX.Element, options?: RenderOptions): Promise<void>;
 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<unknown>;
+export declare function renderConcurrent({ renderOptions, ...props }: RenderConcurrentOptions): Promise<void>;
 export type AlertCustomSection = CustomSection;
 export type RenderAlertOptions = Omit<AlertOptions, 'type'>;
 /**

ryancbahan · 2026-04-02T01:56:47Z

packages/cli-kit/src/private/node/ui/components/ConcurrentOutput.test.tsx

    // Given
    const backendSync = new Synchronizer()
    const frontendSync = new Synchronizer()
+    const gate = new Synchronizer()


this needs a better solution. these tests are already relying on promise flushing in a roundabout way. we should rethink the entire approach.

ryancbahan · 2026-04-02T01:57:44Z

packages/cli-kit/src/private/node/ui/hooks/use-async-and-unmount.ts

      })
  }, [])
+
+  useEffect(() => {


all these components are way too effectful at the moment. we need to follow up on this.

isaacroldan

🎩'ed and working as expected, great work! 👏

use central unmount; stop messing with node's event loop

0893a42

ryancbahan changed the title ~~use central unmount; stop messing with node's event loop~~ use central unmount for ink render tree; stop messing with node's event loop Apr 2, 2026

ryancbahan marked this pull request as ready for review April 2, 2026 00:01

ryancbahan requested a review from a team as a code owner April 2, 2026 00:01

dmerand reviewed Apr 2, 2026

View reviewed changes

This was referenced Apr 2, 2026

Add Tasks regression coverage for deferred Ink teardown #7118

Closed

Defer Ink teardown in prompt components #7121

Closed

ryancbahan force-pushed the rcb/rendering-changes-react-fix branch from db1ae11 to 0893a42 Compare April 2, 2026 01:14

ryancbahan requested a review from a team as a code owner April 2, 2026 01:18

ryancbahan commented Apr 2, 2026

View reviewed changes

ryancbahan requested a review from dmerand April 2, 2026 02:15

isaacroldan approved these changes Apr 2, 2026

View reviewed changes

isaacroldan added this pull request to the merge queue Apr 2, 2026

Merged via the queue into main with commit f26556f Apr 2, 2026
27 checks passed

isaacroldan deleted the rcb/rendering-changes-react-fix branch April 2, 2026 08:08

Conversation

ryancbahan commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

Measuring impact

Checklist

Uh oh!

ryancbahan commented Apr 1, 2026

Uh oh!

github-actions bot commented Apr 2, 2026

Uh oh!

dmerand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 2, 2026

Differences in type declarations

New type declarations

Existing type declarations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacroldan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ryancbahan commented Apr 1, 2026 •

edited

Loading