Skip to content

fix: login crashes on Node 24 (readline resize event)#14

Merged
ryanmcmillan merged 1 commit intomainfrom
fix/login-node24-crash
Mar 25, 2026
Merged

fix: login crashes on Node 24 (readline resize event)#14
ryanmcmillan merged 1 commit intomainfrom
fix/login-node24-crash

Conversation

@ryanmcmillan
Copy link
Copy Markdown
Member

Problem

delega login crashes immediately on Node 24 with:

TypeError: output.on is not a function

Node 24 readline calls output.on('resize', onresize) during construction. The existing fake muted output object only implemented write(), so it throws.

Fix

Replace the plain object with a PassThrough stream that has all EventEmitter methods. Forward resize events from the real stdout so readline can track terminal width. Clean up listeners on close.

Testing

  • npm run build passes
  • Node 24.14.0 compatible

Node 24's readline calls output.on('resize', ...) during construction.
The fake muted output object only implemented write(), causing a TypeError.
Replace with a PassThrough stream that supports all EventEmitter methods.
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 25, 2026

Greptile Summary

This PR fixes a crash in delega login on Node 24 where readline.createInterface calls output.on('resize', ...) during construction — something the old plain mutedOutput object couldn't handle because it only implemented write(). The fix replaces that plain object with a PassThrough stream (which inherits all EventEmitter methods), proxies resize events from process.stdout, and exposes columns/rows getters so readline can query terminal dimensions.

Key changes in src/commands/login.ts:

  • mutedOutput is now a node:stream.PassThrough instead of a plain object with a custom write().
  • A data handler on the stream filters output (suppressing echoed characters while preserving the question prompt), replicating the original muting logic.
  • process.stdout resize events are forwarded to mutedOutput so readline keeps terminal-width tracking working.
  • columns and rows properties are proxied from process.stdout to satisfy readline's dimension checks.
  • Cleanup (removeListener, destroy) happens inside the rl.question callback.

Minor issues found:

  • node:stream is imported via a dynamic await import() instead of a static import at the top of the file, inconsistent with how node:readline is imported and potentially problematic for bundlers.
  • The onResize listener on process.stdout is only cleaned up inside the rl.question callback; an early close of the readline interface (e.g. EOF / Ctrl+D) would leave the listener attached.

Confidence Score: 4/5

  • Safe to merge — the crash fix is correct and complete; the two remaining notes are non-blocking style/robustness improvements.
  • The root cause is clearly diagnosed and the fix is well-targeted. The PassThrough approach is the standard way to solve this problem. The two open items (dynamic import and cleanup leak on early close) are P2 suggestions that don't affect the happy-path login flow.
  • No files require special attention beyond the two minor suggestions in src/commands/login.ts.

Important Files Changed

Filename Overview
src/commands/login.ts Replaces the plain muted-output object with a PassThrough stream to fix the Node 24 readline crash. Resize forwarding and columns/rows proxying are correctly implemented. Two minor issues: dynamic import of node:stream instead of a static import, and a potential onResize listener leak if readline closes before the question callback fires.

Sequence Diagram

sequenceDiagram
    participant user as User (stdin)
    participant rl as node:readline
    participant mt as PassThrough (mutedOutput)
    participant stdout as process.stdout

    Note over rl,mt: createInterface({ output: mutedOutput })
    rl->>mt: output.on('resize', onresize)  [Node 24 — was crashing here]
    stdout->>mt: onResize() forwards resize events

    rl->>mt: write(questionText)
    mt->>stdout: data handler — muted=false → pass through

    Note over rl: muted = true

    rl->>user: rl.question(prompt)
    user->>rl: keystrokes echoed to output
    rl->>mt: write(echoedChars)
    mt-->>stdout: data handler — muted=true, no question text → suppressed

    user->>rl: Enter key
    rl->>rl: rl.close()
    rl->>stdout: removeListener('resize', onResize)
    rl->>mt: mutedOutput.destroy()
    rl->>stdout: write('\n')
Loading

Comments Outside Diff (1)

  1. src/commands/login.ts, line 46-54 (link)

    P2 Resize listener leaks on early readline close

    The onResize listener is only removed inside the rl.question callback. If readline closes before that callback fires — for example when the user sends EOF (Ctrl+D) and Node resolves the question with an empty string, or if stdin closes for any other reason — the listener on the global process.stdout object is never cleaned up.

    Consider also removing it on the 'close' event of the readline interface as a safety net:

      return new Promise((resolve) => {
        const cleanup = () => {
          process.stdout.removeListener("resize", onResize);
          mutedOutput.destroy();
        };
        rl.once("close", cleanup);
        rl.question(question, (answer) => {
          rl.close();
          process.stdout.write("\n");
          resolve(answer.trim());
        });
      });

    This guarantees cleanup regardless of how the interface closes, and removes the need to call removeListener manually inside the question callback.

Reviews (1): Last reviewed commit: "fix: login crashes on Node 24 (readline ..." | Re-trigger Greptile

Comment on lines +21 to +23
const mutedOutput = new (await import("node:stream")).PassThrough({
decodeStrings: false,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Use static import for node:stream

node:stream is a built-in Node.js module already used statically elsewhere (e.g. node:readline at the top). Using a dynamic await import() for it is inconsistent, slightly harder to read, and can confuse bundlers like esbuild or rollup that tree-shake native imports. A static import at the top of the file is simpler and idiomatic:

Suggested change
const mutedOutput = new (await import("node:stream")).PassThrough({
decodeStrings: false,
});
const mutedOutput = new PassThrough({
decodeStrings: false,
});

And add at the top of the file:

import { PassThrough } from "node:stream";

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@ryanmcmillan ryanmcmillan merged commit 3e21dd8 into main Mar 25, 2026
2 checks passed
@ryanmcmillan ryanmcmillan deleted the fix/login-node24-crash branch March 25, 2026 22:28
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.

2 participants