Skip to content

fix(@inquirer/core): clear hook effects before settling prompts - #2255

Merged
SBoudrias merged 2 commits into
SBoudrias:mainfrom
JoelTowell:fix-core-hook-cleanup-settlement
Sep 7, 2026
Merged

SBoudrias merged 2 commits into
SBoudrias:mainfrom
JoelTowell:fix-core-hook-cleanup-settlement

Conversation

@JoelTowell

Copy link
Copy Markdown
Contributor

Description

Closes #1816.

createPrompt currently clears hook effects after the prompt promise has already settled. That means listeners installed by hooks can still handle input emitted in the same tick after done() or after the prompt is aborted.

This moves hook cleanup into the shared settlement path instead:

  • resolve: clear hook effects, then resolve
  • reject/cancel: clear hook effects, then reject

The final interface cleanup still stays in finally, so screen.done() and output.end() continue to run after the prompt reaches its done state.

I added regression coverage for both cases: settling from a useKeypress handler, and aborting the prompt.

Commands run

  • corepack yarn pretest
  • corepack yarn vitest --run packages/core/core.test.ts

@codecov

codecov Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@15cd8d3). Learn more about missing BASE report.

Files with missing lines Patch % Lines
packages/core/src/lib/create-prompt.ts 94.87% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2255   +/-   ##
=======================================
  Coverage        ?   89.45%           
=======================================
  Files           ?       48           
  Lines           ?     2266           
  Branches        ?      602           
=======================================
  Hits            ?     2027           
  Misses          ?      230           
  Partials        ?        9           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SBoudrias
SBoudrias force-pushed the fix-core-hook-cleanup-settlement branch from ccde777 to 6d6d3b8 Compare September 7, 2026 16:48
Characterize how a prompt settles when a useEffect cleanup throws:
the prompt rejects with the cleanup error, and the answer (or the
original settlement error, e.g. AbortPromptError) is dropped.

These tests pass on current main; they guard the existing settlement
semantics against regressions while the settlement path is reworked in
the next commit.
@SBoudrias
SBoudrias force-pushed the fix-core-hook-cleanup-settlement branch 3 times, most recently from 4f26d91 to 7518e0a Compare September 7, 2026 17:32
createPrompt cleared hook effects only after the prompt promise settled,
in a microtask. But readline emits keypresses synchronously within a
single data event, so input batched with the key that settled the prompt
still reached useKeypress handlers before cleanup ran.

Clear hook effects in the synchronous settlement path instead:
- resolve: clear effects, then resolve
- reject/cancel/abort: clear effects, then reject

The interface cleanup (screen.done + output.end) still runs in finally,
so it happens after the prompt reaches its done state. As a side effect,
prompts created with an already-aborted signal now also run the terminal
cleanup instead of leaking the readline interface.

The settlement path is kept minimal: the promise resolvers and
effectScheduler.clearAll() are both idempotent, so no settled guard is
needed; the bound cleanup is named clearEffects to avoid shadowing the
hook-engine store field name.

Event listener registrations go through a listenTo() helper returning
the disposer, so each site collapses into a single
cleanups.add(listenTo(...)) statement.

Adds regression coverage for the done, abort, cancel, and
already-aborted signal settlement paths.

Closes SBoudrias#1816

Co-authored-by: Joel Towell <156634308+JoelTowell@users.noreply.github.com>
@SBoudrias
SBoudrias force-pushed the fix-core-hook-cleanup-settlement branch from 7518e0a to 67e6fe2 Compare September 7, 2026 17:37

@SBoudrias SBoudrias left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is a very clean PR! 👏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review async effect teardown

2 participants