Skip to content

Conversation

@AbanoubGhadban
Copy link
Collaborator

@AbanoubGhadban AbanoubGhadban commented Nov 20, 2025

Summary

Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Add the CHANGELOG entry at the top of the file.

Other Information

Remove this paragraph and mention any other important and relevant information such as benchmarks.

Summary by CodeRabbit

  • Refactor
    • Internal improvements to type safety and control flow handling in test utilities. No changes to public APIs or user-facing functionality.

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

@AbanoubGhadban AbanoubGhadban merged commit 3eb3961 into master Nov 20, 2025
10 checks passed
@AbanoubGhadban AbanoubGhadban deleted the convert-test-utils-to-ts branch November 20, 2025 13:25
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 20, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Refactors type safety in createNodeReadableStream test utility by introducing explicit typing for internal state: pendingChunks becomes Buffer[], and pushFn becomes a nullable function type with tightened Buffer-only signatures and guarded invocations.

Changes

Cohort / File(s) Change Summary
Type refinement for stream state management
packages/react-on-rails-pro/tests/testUtils.ts
Introduces explicit typing: pendingChunks as Buffer[], pushFn as ((chunk: Buffer | undefined) => void) | null. Tightens push function signature to accept only Buffer type. Updates read lifecycle to initialize pushFn with this.push.bind(this) and guards chunk pushes with nullable checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify nullable handling logic for pushFn is correct and prevents runtime errors
  • Confirm Buffer type alignment throughout the stream's push/read lifecycle
  • Validate that type narrowing guards properly protect against undefined scenarios

Poem

🐰 With types now clear as morning dew,
Buffer arrays shine bright and true,
No more generic, vague, or loose,
Our streams now type-safe, no excuse!
Nullable guards and bindings tight—
Test utils gleam with typed delight!

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch convert-test-utils-to-ts

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 840f9c7 and fa5164e.

📒 Files selected for processing (1)
  • packages/react-on-rails-pro/tests/testUtils.ts (2 hunks)

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.

justin808 added a commit that referenced this pull request Nov 20, 2025
Root Cause:
The ci-changes-detector script was missing patterns for test files in
the Pro package. When PR #2074 changed only
`packages/react-on-rails-pro/tests/testUtils.ts`, the detector
incorrectly classified it as a docs-only change because the pattern
only matched `packages/react-on-rails-pro/src/**/*`.

This triggered the ensure-master-docs-safety action, which correctly
failed the workflow since the previous commit had test failures. The
safety mechanism worked as intended, but the root cause was the missing
pattern.

Changes:
- Add patterns for `packages/react-on-rails/tests/**/*` to trigger JS tests
- Add patterns for `packages/react-on-rails-pro/tests/**/*` to trigger Pro tests
- Add patterns for `scripts/`, `package.json`, and `tsconfig.json` in both packages
- Update comments to clarify what's included in each pattern

Impact:
- Pro and open-source package test file changes will now correctly trigger CI
- Package configuration changes will trigger appropriate test suites
- Prevents false-positive "docs-only" detection that blocks deployments

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
justin808 added a commit that referenced this pull request Nov 21, 2025
## Summary

Fixes the ci-changes-detector script to correctly identify test file
changes in both the open-source and Pro packages, preventing
false-positive "docs-only" detection.

## Root Cause

The ci-changes-detector script was missing patterns for test files in
the Pro package. When PR #2074 changed only
`packages/react-on-rails-pro/tests/testUtils.ts`, the detector
incorrectly classified it as a docs-only change because the pattern only
matched `packages/react-on-rails-pro/src/**/*`.

This triggered the ensure-master-docs-safety action, which correctly
failed the workflow since the previous commit had test failures. The
safety mechanism worked as intended, but the root cause was the missing
pattern.

## Changes

- ✅ Add patterns for `packages/react-on-rails/tests/**/*` to trigger JS
tests
- ✅ Add patterns for `packages/react-on-rails-pro/tests/**/*` to trigger
Pro tests
- ✅ Add patterns for `scripts/`, `package.json`, and `tsconfig.json` in
both packages
- ✅ Update comments to clarify what's included in each pattern

## Testing

Tested with multiple file patterns:
- ✅ `packages/react-on-rails-pro/tests/testUtils.ts` → correctly
detected as PRO_JS_CHANGED
- ✅ `packages/react-on-rails/tests/test.ts` → correctly detected as
JS_CHANGED
- ✅ `packages/react-on-rails-pro/package.json` → correctly detected as
PRO_JS_CHANGED
- ✅ `README.md` → correctly detected as docs_only

## Impact

- Pro and open-source package test file changes will now correctly
trigger CI
- Package configuration changes will trigger appropriate test suites
- Prevents false-positive "docs-only" detection that was blocking the
latest master commit
- The ensure-master-docs-safety mechanism will work properly with
correct change detection

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Chores**
* Improved build system detection for JavaScript and TypeScript file
changes to ensure accurate identification of code-related modifications.

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

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Co-authored-by: Claude <[email protected]>
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