Skip to content

fix(terminal): fix race condition preventing onData listeners from receiving data when set immediately after construction#37

Open
MBanucu wants to merge 2 commits intosursaone:mainfrom
MBanucu:terminal-fixes
Open

fix(terminal): fix race condition preventing onData listeners from receiving data when set immediately after construction#37
MBanucu wants to merge 2 commits intosursaone:mainfrom
MBanucu:terminal-fixes

Conversation

@MBanucu
Copy link

@MBanucu MBanucu commented Jan 28, 2026

Closes #35

Summary

  • Updates src/terminal.ts with core logic improvements
  • Updates src/terminal.integration.test.ts with enhanced integration tests

Summary by CodeRabbit

  • Bug Fixes

    • Resolved a timing issue so event listeners (onData/onExit) reliably receive events when attached immediately after Terminal creation.
  • Tests

    • Added integration tests verifying listeners receive terminal data and exit events when set immediately, including conditional sync coverage and cross-platform timeouts.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 28, 2026

Walkthrough

The PTY read loop startup in Terminal was deferred to a microtask (queueMicrotask) so constructors complete before reading begins. Two integration tests were added to validate immediate onData wiring and a conditional sync test with cross-platform timeouts.

Changes

Cohort / File(s) Summary
Read Loop Deferral
src/terminal.ts
Start PTY read loop via queueMicrotask instead of starting synchronously in the constructor; comment added explaining rationale.
Integration Tests
src/terminal.integration.test.ts
Added two tests: one verifying onData receives data when attached immediately after construction, and a conditional "sync tests" verifying onExit/onData behavior with timeouts and kill flow.

Sequence Diagram(s)

(omitted — changes are limited to deferring a read loop start and added tests; no multi-component sequential flow requiring visualization)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

I nibble on a microtask light,
Listeners settle snug and tight.
Data waits till setup's done,
No race, no sprint — just gentle fun. 🐇✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: addressing a race condition where onData listeners couldn't receive data when attached immediately after Terminal construction.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@MBanucu MBanucu changed the title Update terminal integration tests and core logic fix(terminal): fix race condition preventing onData listeners from receiving data when set immediately after construction Jan 28, 2026
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/terminal.integration.test.ts`:
- Around line 503-533: The test creates a Terminal instance but never registers
it with the shared terminals array used by the global afterEach cleanup; after
constructing the terminal (const terminal = new Terminal(cmd, args)) add
terminals.push(terminal) so the test's Terminal is cleaned up on exceptions via
the existing afterEach logic (referencing the terminals array and the Terminal
instance variable).

… calls

Instead of calling terminal.kill() directly in the 'Terminal sync tests',
push the terminal to the terminals array for automatic cleanup in afterEach.
This ensures consistent cleanup behavior across all integration tests.
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.

Potential bug in event loop

1 participant