Skip to content

fix(formEval): replace remaining empty catch with descriptive comment - #42041

Closed
PedroHenrique0713 wants to merge 2 commits into
appsmithorg:releasefrom
PedroHenrique0713:fix/form-eval-empty-catch
Closed

fix(formEval): replace remaining empty catch with descriptive comment#42041
PedroHenrique0713 wants to merge 2 commits into
appsmithorg:releasefrom
PedroHenrique0713:fix/form-eval-empty-catch

Conversation

@PedroHenrique0713

@PedroHenrique0713 PedroHenrique0713 commented Jul 23, 2026

Copy link
Copy Markdown

Summary

Second empty catch in formEval.ts replaced with a comment explaining the fallback behavior. This prevents silent error swallowing during form config evaluation — any future debugger/logger will still see the uncaught exception path without the empty block hiding it.

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of form configuration evaluation errors so invalid entries are skipped without overwriting existing output.
  • Documentation

    • Added clarification about how object sanitization handles non-JSON values while preserving functions.

@PedroHenrique0713
PedroHenrique0713 requested a review from a team as a code owner July 23, 2026 20:25
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The evaluation worker now documents skipped entries when expression evaluation fails and clarifies which values are removed during JSON-based cloning while functions are re-injected.

Changes

Evaluation worker updates

Layer / File(s) Summary
Evaluation error handling
app/client/src/workers/Evaluation/formEval.ts
Expression failures are caught with an explicit skip-entry comment, leaving the existing output unchanged.
Stringification behavior documentation
app/client/src/workers/Evaluation/helpers.ts
Comments describe JSON-unsafe values removed during cloning and the separate preservation of functions.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

Poem

Expressions stumble, entries step aside,
While functions safely take the ride.
Dates and Maps fade from the clone,
Clearer comments make intent known.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description only includes a brief summary and omits required template sections like Fixes, Automation, and Communication. Add the missing template sections, especially Fixes, Automation, Cypress test results, and Communication, plus any needed context.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the main change to formEval's catch block.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 JSON round-trip note near the helper
implementation to state that Dates become strings, Maps, Sets, and RegExps
become plain objects, and undefined properties are omitted. Clarify that
functions are collected and re-injected as source strings rather than preserved
as callable functions, matching helpers.test.ts.
🪄 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: e0ffa0e5-4bc4-4e10-b28d-29e5da99a1c8

📥 Commits

Reviewing files that changed from the base of the PR and between 583811d and aac20ef.

📒 Files selected for processing (2)
  • app/client/src/workers/Evaluation/formEval.ts
  • app/client/src/workers/Evaluation/helpers.ts

Comment on lines +114 to +116
// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the JSON-round-trip documentation.

Dates are serialized to strings, while Maps, Sets, and RegExps become plain objects; undefined properties are omitted. Functions are re-injected as source strings, not preserved as callable functions, as shown by helpers.test.ts.

Proposed 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.
+  // JSON round-tripping converts or drops values that JSON cannot represent:
+  // Dates become strings, Maps/Sets/RegExps become plain objects, and
+  // undefined properties are omitted. Functions are re-injected as 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.

Suggested change
// 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.
// JSON round-tripping converts or drops values that JSON cannot represent:
// Dates become strings, Maps/Sets/RegExps become plain objects, and
// undefined properties are omitted. Functions are re-injected as 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 JSON round-trip note near the helper implementation to state that Dates
become strings, Maps, Sets, and RegExps become plain objects, and undefined
properties are omitted. Clarify that functions are collected and re-injected as
source strings rather than preserved as callable functions, matching
helpers.test.ts.

@PedroHenrique0713

Copy link
Copy Markdown
Author

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.

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.

1 participant