fix(evalTree): guard against empty parsed updates array - #42042
fix(evalTree): guard against empty parsed updates array#42042PedroHenrique0713 wants to merge 3 commits into
Conversation
When evaluation produced an empty tree, parsedUpdates[0] was undefined and accessing .rhs threw TypeError. Now checks .length before access.
WalkthroughEvaluation worker code now documents failed expression and JSON cloning behavior, while guarding previous-state updates when tree parsing produces no updates. ChangesEvaluation safety
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client/src/workers/Evaluation/helpers.ts`:
- Around line 114-116: Update the serialization comment near fnValue.toString()
to state that functions are converted to source strings and reinserted as
strings, not preserved as callable functions. Describe the handling of
non-JSON-safe values as potentially lossy conversions, rather than claiming all
such types are stripped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f450c2b-1c29-410a-bf7c-688a5485789f
📒 Files selected for processing (3)
app/client/src/workers/Evaluation/formEval.tsapp/client/src/workers/Evaluation/handlers/evalTree.tsapp/client/src/workers/Evaluation/helpers.ts
| // Note: JSON round-trip strips Dates, Sets, Maps, RegExps, and undefined values. | ||
| // Functions are preserved (collected before, re-injected after), but other | ||
| // non-JSON-safe types may be lost. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that functions are serialized as strings, not preserved as functions.
fnValue.toString() followed by set(...) reinserts function source text, so consumers cannot call the restored value as a function. Also describe the type conversions as lossy rather than saying every type is stripped.
Suggested wording
- // Note: JSON round-trip strips Dates, Sets, Maps, RegExps, and undefined values.
- // Functions are preserved (collected before, re-injected after), but other
- // non-JSON-safe types may be lost.
+ // Note: JSON round-trip loses non-JSON-safe values; for example, Dates become
+ // strings, Sets/Maps/RegExps lose their original types, and undefined values
+ // are omitted or converted to null. Functions are re-injected as source strings.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Note: JSON round-trip strips Dates, Sets, Maps, RegExps, and undefined values. | |
| // Functions are preserved (collected before, re-injected after), but other | |
| // non-JSON-safe types may be lost. | |
| // Note: JSON round-trip loses non-JSON-safe values; for example, Dates become | |
| // strings, Sets/Maps/RegExps lose their original types, and undefined values | |
| // are omitted or converted to null. Functions are re-injected as source strings. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/client/src/workers/Evaluation/helpers.ts` around lines 114 - 116, Update
the serialization comment near fnValue.toString() to state that functions are
converted to source strings and reinserted as strings, not preserved as callable
functions. Describe the handling of non-JSON-safe values as potentially lossy
conversions, rather than claiming all such types are stripped.
|
Closing this one. I opened a stack of PRs in this repo today and, because each branch was cut from the previous one instead of from the base, they overlap: this PR carries the commits of the earlier ones as well. I am consolidating the work in #42033 and will resubmit the remaining fixes individually, on top of the base branch, once that one has been reviewed. Sorry for the noise. |
1 participant
Empty evaluation tree → parsedUpdates [0] → TypeError. Guard with .length check.
Summary by CodeRabbit
Bug Fixes
Refactor