Conversation
Greptile SummaryRenames the Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 8efe0fc |
| if (this.logger != null) { | ||
| this.logger.logEvent(this.filename, { | ||
| kind: 'CompileError', | ||
| detail: error, | ||
| fnLoc: null, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Duplicate error logging with fnLoc: null
recordError logs a CompileError event here with fnLoc: null, but errors accumulated via this path are also logged again by logError() in Program.ts:180-189 (which iterates over err.details and logs each with the real fnLoc). This causes duplicate log entries for the same error — visible in the dynamic-gating-bailout-nopanic.expect.md test output, which now shows two CompileError JSON entries for the same diagnostic (one with "fnLoc":null and one with the actual location).
Is the intent to have early per-error logging here (for streaming diagnostics), with the Program.ts logging being a redundant legacy path that should be removed? Or should this logging be removed from recordError to avoid duplicates?
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 737-743
Comment:
**Duplicate error logging with `fnLoc: null`**
`recordError` logs a `CompileError` event here with `fnLoc: null`, but errors accumulated via this path are also logged again by `logError()` in `Program.ts:180-189` (which iterates over `err.details` and logs each with the real `fnLoc`). This causes duplicate log entries for the same error — visible in the `dynamic-gating-bailout-nopanic.expect.md` test output, which now shows two `CompileError` JSON entries for the same diagnostic (one with `"fnLoc":null` and one with the actual location).
Is the intent to have early per-error logging here (for streaming diagnostics), with the `Program.ts` logging being a redundant legacy path that should be removed? Or should this logging be removed from `recordError` to avoid duplicates?
How can I resolve this? If you propose a fix, please make it concise.Rename `state: Environment` to `env: Environment` in ValidateMemoizedEffectDependencies visitor methods, and `errorState: Environment` to `env: Environment` in ValidatePreservedManualMemoization's validateInferredDep.
|
Upstream PR was closed or merged. Code is synced via branch mirror. |
Mirror of facebook/react#35883
Original author: josephsavona
Rename
state: Environmenttoenv: EnvironmentinValidateMemoizedEffectDependencies visitor methods, and
errorState: Environmenttoenv: EnvironmentinValidatePreservedManualMemoization's validateInferredDep.
Stack created with Sapling. Best reviewed with ReviewStack.