fix: pushToArray mutation and getPathAndValueFromActionDiffObject logic errors - #42036
fix: pushToArray mutation and getPathAndValueFromActionDiffObject logic errors#42036PedroHenrique0713 wants to merge 3 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.
WalkthroughUtility functions now escape regex prefixes, improve retry rejection, compare arrays element-wise, construct diff paths from accumulator state, guard empty or missing DOM targets, and avoid mutating arrays. ChangesUtility correctness
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/src/utils/AppsmithUtils.tsx (1)
442-442: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSort each array only once.
[...arr2].sort()runs for every element checked byevery, creating unnecessary O(n² log n) work for larger selections.Proposed fix
export function areArraysEqual(arr1: string[], arr2: string[]) { if (arr1.length !== arr2.length) return false; // Because the array is frozen in strict mode, you'll need to copy the array before sorting it - 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 logic around the return expression to sort both copied arrays once before calling every, then compare the pre-sorted values by index. Preserve the existing non-mutating behavior and equality semantics while avoiding repeated sorting of arr2.app/client/src/utils/helpers.tsx (1)
1320-1324: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAudit
pushToArraycall sites for side-effect-only usage.The only runtime call site currently assigns the returned value, but side-effect-only callers would silently stop updating their arrays after this non-mutating update. Also add a test asserting that the original
arr1remains unchanged.🤖 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/helpers.tsx` around lines 1320 - 1324, Audit all runtime call sites of pushToArray for callers that rely on mutation without using its return value, and update those callers to assign the returned array or otherwise preserve their behavior. Add a test covering the pushToArray behavior where the original arr1 remains unchanged after the call.
🤖 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/getPathAndValueFromActionDiffObject.ts`:
- Around line 63-66: Update getPathAndValueFromActionDiffObject so the loop
continues scanning every diff entry instead of returning when the first kind ===
"N" entry is found. Keep updating the path and value accumulator for matching
entries, then return { path, value } only after iteration completes.
---
Nitpick comments:
In `@app/client/src/utils/AppsmithUtils.tsx`:
- Line 442: Update the array comparison logic around the return expression to
sort both copied arrays once before calling every, then compare the pre-sorted
values by index. Preserve the existing non-mutating behavior and equality
semantics while avoiding repeated sorting of arr2.
In `@app/client/src/utils/helpers.tsx`:
- Around line 1320-1324: Audit all runtime call sites of pushToArray for callers
that rely on mutation without using its return value, and update those callers
to assign the returned array or otherwise preserve their behavior. Add a test
covering the pushToArray behavior where the original arr1 remains unchanged
after the call.
🪄 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: 7c2c5bde-8dc3-4be8-bf08-c09d5f643496
📒 Files selected for processing (3)
app/client/src/utils/AppsmithUtils.tsxapp/client/src/utils/getPathAndValueFromActionDiffObject.tsapp/client/src/utils/helpers.tsx
| return { path, value }; | ||
| } | ||
|
|
||
| return { path, value }; | ||
| } | ||
| return { path, value }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return only after scanning all diff entries.
Returning inside the loop stops at the first kind === "N" entry, so later changes are ignored and the final return is unreachable for matching diffs. Keep updating the accumulator during iteration and return { path, value } after the loop, as required by the diff-extraction objective.
🤖 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/getPathAndValueFromActionDiffObject.ts` around lines 63
- 66, Update getPathAndValueFromActionDiffObject so the loop continues scanning
every diff entry instead of returning when the first kind === "N" entry is
found. Keep updating the path and value accumulator for matching entries, then
return { path, value } only after iteration completes.
|
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
Summary
Two bug fixes in Appsmith utilities.
1.
pushToArraymutates input array in-place (helpers.tsx)arr1.push(item)mutates the caller's array, which is unexpected for a utility function in a Redux-based app. Frozen arrays would throw. Other helpers likeconcatWithArrayuse.concat()— this one was inconsistent. Fix: spread-copy and append.2.
getPathAndValueFromActionDiffObjectlogic errors (getPathAndValueFromActionDiffObject.ts)Two bugs: (a) the
reducecallback referenced the outerpathvariable instead ofacc, so for multi-segment paths the accumulator grew incorrectly; (b) thereturn { path, value }was inside the for loop, exiting after the first iteration regardless of whether akind === "N"diff was found. Fix: useaccconsistently and move return outside the loop.Summary by CodeRabbit