Skip to content

[compiler] Fix Pipeline.ts early-exit, formatting, and style issues#601

Closed
everettbu wants to merge 1 commit into
mainfrom
pr35884
Closed

[compiler] Fix Pipeline.ts early-exit, formatting, and style issues#601
everettbu wants to merge 1 commit into
mainfrom
pr35884

Conversation

@everettbu

@everettbu everettbu commented Feb 23, 2026

Copy link
Copy Markdown

Mirror of facebook/react#35884
Original author: josephsavona


Fix the transformFire early-exit in Pipeline.ts to only trigger on new
errors from transformFire itself, not pre-existing errors from earlier
passes. The previous env.hasErrors() check was too broad — it would
early-exit on validation errors that existed before transformFire ran.

Also add missing blank line in CodegenReactiveFunction.ts Context class,
and fix formatting in ValidateMemoizedEffectDependencies.ts.


Stack created with Sapling. Best reviewed with ReviewStack.

  • #35888
  • -> #35884
  • #35883
  • #35882
  • #35881
  • #35880
  • #35879
  • #35878
  • #35877
  • #35876
  • #35875
  • #35874

@everettbu everettbu added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 23, 2026
@greptile-apps

greptile-apps Bot commented Feb 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds a single blank line in CodegenReactiveFunction.ts between the recordError() method and the get nextCacheIndex getter in the Context class, aligning with the existing convention of blank-line-separated class members. This is a trivial formatting-only change with no functional impact.

  • Adds missing blank line between recordError() and get nextCacheIndex in the Context class
  • No logic, behavior, or API changes

Confidence Score: 5/5

  • This PR is safe to merge — it adds only a blank line with zero functional impact.
  • The change is a single blank line addition for formatting consistency. There is no logic, no new code, and no risk of regression.
  • No files require special attention.

Important Files Changed

Filename Overview
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts Adds a missing blank line between recordError() and the get nextCacheIndex getter to match the class's existing formatting convention of blank-line-separated methods.

Last reviewed commit: 9395a1f

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

79 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

79 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

kind: 'err',
msg:
`Compiler pass(es) threw instead of recording errors:\n` +
unexpectedThrows.map(l => (l.event as any).data).join('\n'),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unsafe cast to any

The (l.event as any).data cast bypasses type safety. Since CompileUnexpectedThrowEvent is a known type with a data field, consider narrowing via a type guard or assertion instead:

Suggested change
unexpectedThrows.map(l => (l.event as any).data).join('\n'),
unexpectedThrows.map(l => (l.event as {data: string}).data).join('\n'),

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!

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/snap/src/compiler.ts
Line: 389

Comment:
**Unsafe cast to `any`**

The `(l.event as any).data` cast bypasses type safety. Since `CompileUnexpectedThrowEvent` is a known type with a `data` field, consider narrowing via a type guard or assertion instead:

```suggestion
        unexpectedThrows.map(l => (l.event as {data: string}).data).join('\n'),
```

<sub>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!</sub>

How can I resolve this? If you propose a fix, please make it concise.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

78 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

78 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

78 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Fix the transformFire early-exit in Pipeline.ts to only trigger on new
errors from transformFire itself, not pre-existing errors from earlier
passes. The previous `env.hasErrors()` check was too broad — it would
early-exit on validation errors that existed before transformFire ran.

Also add missing blank line in CodegenReactiveFunction.ts Context class,
and fix formatting in ValidateMemoizedEffectDependencies.ts.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@everettbu

Copy link
Copy Markdown
Author

Upstream PR was closed or merged. Code is synced via branch mirror.

@everettbu everettbu closed this Feb 24, 2026
@everettbu
everettbu deleted the pr35884 branch February 24, 2026 00:36
@everettbu
everettbu restored the pr35884 branch February 24, 2026 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants