fix: findIndex missing return, isURL null guard, sanitizeString null guard - #42037
Closed
PedroHenrique0713 wants to merge 4 commits into
Closed
fix: findIndex missing return, isURL null guard, sanitizeString null guard#42037PedroHenrique0713 wants to merge 4 commits into
PedroHenrique0713 wants to merge 4 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.
Contributor
WalkthroughUtility helpers now escape regex input, validate defensive cases, correct diff path handling, avoid array mutation, and safely handle missing or zero-area UI elements. ChangesUtility hardening
Estimated code review effort: 2 (Simple) | ~10 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: 1
🧹 Nitpick comments (1)
app/client/src/utils/AppsmithUtils.tsx (1)
442-442: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSort
arr2once outsideevery.
[...arr2].sort()is recomputed for every element, creating unnecessary allocations and making this comparisonO(n² log n)instead ofO(n log n).Suggested refactor
- 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` at line 442, Update the array comparison expression in the surrounding utility function to sort arr2 once before calling every, then reuse the sorted result inside the callback. Preserve the existing arr1 sorting and element-by-element equality behavior while avoiding repeated [...arr2].sort() allocations.
🤖 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/utils/AppsmithUtils.tsx`:
- Around line 318-326: Update the retry logic in AppsmithUtils to immediately
reject when shouldRetry(e) is false, use retriesLeft <= 1 to terminate exhausted
retries, and pass shouldRetry into the recursive call so every retry path
settles the promise.
---
Nitpick comments:
In `@app/client/src/utils/AppsmithUtils.tsx`:
- Line 442: Update the array comparison expression in the surrounding utility
function to sort arr2 once before calling every, then reuse the sorted result
inside the callback. Preserve the existing arr1 sorting and element-by-element
equality behavior while avoiding repeated [...arr2].sort() allocations.
🪄 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: 8e1494a0-63c4-4628-b098-aeb5f3f0da07
📒 Files selected for processing (6)
app/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.tsx
Comment on lines
318
to
+326
| if (shouldRetry(e)) { | ||
| setTimeout(async () => { | ||
| if (retriesLeft === 1) { | ||
| return Promise.reject({ | ||
| reject({ | ||
| code: ERROR_CODES.SERVER_ERROR, | ||
| message: createMessage(ERROR_500), | ||
| show: false, | ||
| }); | ||
| return; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ensure every retry path settles the promise.
When shouldRetry(e) returns false, the outer promise remains pending. Also, retriesLeft === 1 misses zero or negative values, which can schedule retries indefinitely. Reject non-retryable errors immediately, use retriesLeft <= 1, and pass shouldRetry through the recursive call at Line 330.
🤖 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 318 - 326, Update the
retry logic in AppsmithUtils to immediately reject when shouldRetry(e) is false,
use retriesLeft <= 1 to terminate exhausted retries, and pass shouldRetry into
the recursive call so every retry path settles the promise.
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 bug fixes in Appsmith utilities.
JSPaneUtils.tsxfindIndexcallback missingreturn, always returned-1TypeHelpers.tsisURLcrashes onnull/undefinedfrom dynamic dataURLUtils.tssanitizeStringcrashes on falsy form field valuesSummary by CodeRabbit