fix: empty catch blocks, falsy JSON escape, and stale closure in workers - #42039
Closed
PedroHenrique0713 wants to merge 6 commits into
Closed
fix: empty catch blocks, falsy JSON escape, and stale closure in workers#42039PedroHenrique0713 wants to merge 6 commits into
PedroHenrique0713 wants to merge 6 commits into
Conversation
… lookup fix(AppsmithUtils): escape regex special characters in getNextEntityName and getDuplicateName. Entity name prefixes containing regex metacharacters (. $ * + etc.) caused incorrect auto-increment numbering and potential name collisions. fix(helpers): guard against division by zero in isElementVisibleInContainer. When an element has zero dimensions, elementArea === 0 caused Infinity/NaN comparisons that falsely reported the element as visible. fix(helpers): add null guard to getWidgetElementToScroll. Accessing widget.parentId without checking if canvasWidgets[widgetId] exists caused TypeError on stale/deleted widget IDs.
fix(retryPromise): use reject() instead of Promise.reject() inside setTimeout.
The setTimeout callback return value is ignored, so Promise.reject() created
a dangling rejection that never reached the outer Promise's reject handler,
causing the promise to hang forever when retries were exhausted.
fix(areArraysEqual): use element-wise comparison instead of join(",").
Array elements containing commas caused false positives (e.g.,
["a,b"] and ["a","b"] incorrectly reported as equal).
fix(helpers): pushToArray no longer mutates input array in-place. Previously arr1.push(item) mutated the caller's array, violating immutability expectations in Redux state. Now creates a new array. fix(getPathAndValueFromActionDiffObject): fix reduce accumulator path computation and early return. The reduce callback referenced the outer 'path' variable instead of 'acc', causing path to grow incorrectly across multiple diff elements. The return statement was also inside the for loop, exiting prematurely instead of checking all elements.
fix(JSPaneUtils): add missing return to findIndex callback. Arrow function with curly braces returned undefined, so findIndex always returned -1, making the archived action lookup silently fail. fix(TypeHelpers): add null guard to isURL before regex test. RegExp.prototype.test throws TypeError on null/undefined input from dynamic data sources. fix(URLUtils): add null guard to sanitizeString. str.toLowerCase() throws TypeError on null/undefined from form fields.
When action is not found, action?.id ?? '' produced an empty string '', which was used as key in state.evaluations.triggers[''], returning undefined and silently passing through extractConditionalOutput. Fix: return early with empty/default object when no action found.
fix(jsLibrary): replace empty catch with comment explaining fallback behavior.
Empty catch silently swallowed URL parsing errors with no context.
fix(formEval): replace empty catch with comment explaining default fallback.
Dynamic form evaluation errors were silently swallowed with no indication.
fix(evaluationSubstitution): decouple JSON detection from truthiness.
JSON.parse('false') returned false, skipping escape for valid JSON
string values that happen to be falsy ('false','0','null').
Contributor
WalkthroughThis PR updates client selectors, utilities, helpers, diff processing, and evaluation logic to handle missing identifiers, invalid inputs, regex metacharacters, immutable arrays, corrected predicates, and JSON parsing outcomes. ChangesClient safeguards
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/client/src/utils/AppsmithUtils.tsx (1)
438-443: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSort both arrays once before comparing.
[...arr2].sort()is recomputed on everyeveryiteration, causing unnecessary allocations and O(n²) behavior. Create sorted copies of both arrays once, then compare them.Suggested refactor
export function areArraysEqual(arr1: string[], arr2: string[]) { if (arr1.length !== arr2.length) return false; - return [...arr1].sort().every((val, i) => val === [...arr2].sort()[i]); + const sortedArr1 = [...arr1].sort(); + const sortedArr2 = [...arr2].sort(); + return sortedArr1.every((val, i) => val === sortedArr2[i]); }🤖 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/utils/AppsmithUtils.tsx` around lines 438 - 443, Update areArraysEqual to create sorted copies of both arr1 and arr2 once before the comparison, then compare those cached arrays in every. Preserve the existing length check and avoid sorting or allocating arr2 inside the iteration.
🤖 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/selectors/formSelectors.ts`:
- Around line 77-78: Update the missing-action branch in the selector containing
actionId to return a fully typed DynamicValues default with safe initialized
values, especially data as an empty array, instead of casting an empty object.
Preserve the existing consumer contract in SegmentedControl so
dynamicFetchedValues.data remains defined when no action ID is available.
In `@app/client/src/utils/JSPaneUtils.tsx`:
- Line 103: Format the findIndex predicate in the updateExisting flow using the
repository’s Prettier configuration so the declaration is wrapped to the
expected line length; do not change its behavior.
---
Nitpick comments:
In `@app/client/src/utils/AppsmithUtils.tsx`:
- Around line 438-443: Update areArraysEqual to create sorted copies of both
arr1 and arr2 once before the comparison, then compare those cached arrays in
every. Preserve the existing length check and avoid sorting or allocating arr2
inside the iteration.
🪄 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: f2f07b28-c2af-49ca-93e5-fbae32cc4d26
📒 Files selected for processing (10)
app/client/src/selectors/formSelectors.tsapp/client/src/utils/AppsmithUtils.tsxapp/client/src/utils/JSPaneUtils.tsxapp/client/src/utils/TypeHelpers.tsapp/client/src/utils/URLUtils.tsapp/client/src/utils/getPathAndValueFromActionDiffObject.tsapp/client/src/utils/helpers.tsxapp/client/src/workers/Evaluation/evaluationSubstitution.tsapp/client/src/workers/Evaluation/formEval.tsapp/client/src/workers/Evaluation/handlers/jsLibrary.ts
Comment on lines
+77
to
+78
| const actionId = action?.id; | ||
| if (!actionId) return {} as DynamicValues; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return a safe DynamicValues shape for missing actions.
Casting {} to DynamicValues hides missing fields. app/client/src/components/formControls/SegmentedControl.tsx assigns dynamicFetchedValues.data directly to options, so this path changes the initialized [] to undefined when actionId is unavailable. Return typed defaults or coalesce these fields in the consumer.
As per coding guidelines, ensure TypeScript types are correct; this cast masks the required DynamicValues fields.
🤖 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/selectors/formSelectors.ts` around lines 77 - 78, Update the
missing-action branch in the selector containing actionId to return a fully
typed DynamicValues default with safe initialized values, especially data as an
empty array, instead of casting an empty object. Preserve the existing consumer
contract in SegmentedControl so dynamicFetchedValues.data remains defined when
no action ID is available.
Source: Coding guidelines
| const indexOfArchived = toBearchivedActions.findIndex((js) => { | ||
| js.id === updateExisting.id; | ||
| }); | ||
| const indexOfArchived = toBearchivedActions.findIndex((js) => js.id === updateExisting.id); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Format the changed predicate with Prettier.
Line 103 is overlong and should be wrapped to satisfy the repository’s formatting check.
As per coding guidelines, files under app/client/{src,cypress}/**/*.{ts,tsx,js} must pass Prettier via yarn run prettier.
Suggested formatting
- const indexOfArchived = toBearchivedActions.findIndex((js) => js.id === updateExisting.id);
+ const indexOfArchived = toBearchivedActions.findIndex(
+ (js) => js.id === updateExisting.id,
+ );📝 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
| const indexOfArchived = toBearchivedActions.findIndex((js) => js.id === updateExisting.id); | |
| const indexOfArchived = toBearchivedActions.findIndex( | |
| (js) => js.id === updateExisting.id, | |
| ); |
🤖 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/utils/JSPaneUtils.tsx` at line 103, Format the findIndex
predicate in the updateExisting flow using the repository’s Prettier
configuration so the declaration is wrapped to the expected line length; do not
change its behavior.
Source: Coding guidelines
This was referenced Jul 23, 2026
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three fixes in Appsmith worker/evaluation engine.
jsLibrary.tsformEval.tsevaluationSubstitution.tsJSON.parse('false')returnedfalse, skipping escape for valid JSON strings that are falsySummary by CodeRabbit