fix: retryPromise silent failure and areArraysEqual false positives - #42035
fix: retryPromise silent failure and areArraysEqual false positives#42035PedroHenrique0713 wants to merge 2 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).
WalkthroughUtility functions now handle regex-special prefixes, compare arrays element-wise, preserve terminal retry errors, and guard scroll calculations against zero-area elements and missing widgets. ChangesUtility Defensive Corrections
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 (1)
app/client/src/utils/AppsmithUtils.tsx (1)
442-442: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSort
arr2only once.
[...arr2].sort()currently runs for every element ofarr1, causing repeated allocations and unnecessaryO(n² log n)work. Copy and sort both arrays before callingevery().Proposed fix
- 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 in the surrounding utility function to copy and sort both arr1 and arr2 once before invoking every(). Reuse the sorted arr2 inside the every callback while preserving the existing element-by-element equality behavior.
🤖 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 321-326: Update the terminal error handling in the React.lazy
loading flow to reject the outer promise with the caught error `e`, preserving
its original stack and payload. If UI metadata is required, attach it while
retaining `e` rather than replacing it with a new SERVER_ERROR object.
---
Nitpick comments:
In `@app/client/src/utils/AppsmithUtils.tsx`:
- Line 442: Update the array comparison logic in the surrounding utility
function to copy and sort both arr1 and arr2 once before invoking every(). Reuse
the sorted arr2 inside the every callback while preserving the existing
element-by-element equality behavior.
🪄 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: c454ca09-673a-41b0-a774-cc9b92f6dc9f
📒 Files selected for processing (2)
app/client/src/utils/AppsmithUtils.tsxapp/client/src/utils/helpers.tsx
| reject({ | ||
| code: ERROR_CODES.SERVER_ERROR, | ||
| message: createMessage(ERROR_500), | ||
| show: false, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the terminal retry error.
The caught e is discarded and replaced with a new object, so React.lazy and downstream error boundaries lose the original module-load error, stack, and payload. Reject the outer promise with e (or retain it as the cause while adding the UI metadata).
Proposed fix
- reject({
- code: ERROR_CODES.SERVER_ERROR,
- message: createMessage(ERROR_500),
- show: false,
- });
+ reject(e);📝 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.
| reject({ | |
| code: ERROR_CODES.SERVER_ERROR, | |
| message: createMessage(ERROR_500), | |
| show: false, | |
| }); | |
| return; | |
| reject(e); | |
| return; |
🤖 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 321 - 326, Update the
terminal error handling in the React.lazy loading flow to reject the outer
promise with the caught error `e`, preserving its original stack and payload. If
UI metadata is required, attach it while retaining `e` rather than replacing it
with a new SERVER_ERROR object.
|
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 utility code.
1.
retryPromisehangs forever when retries exhausted (AppsmithUtils.tsx)Inside the
setTimeoutcallback,Promise.reject(...)was used instead ofreject(...). SincesetTimeoutignores its callback's return value, the rejection never reached the outer Promise's reject handler. The promise hung forever when retries were exhausted. Fix: usereject()directly.2.
areArraysEqualfalse positives with comma-containing elements (AppsmithUtils.tsx)Using
join(",")to compare arrays means["a,b"]and["a","b"]are incorrectly reported as equal when the comma in the data collides with the delimiter. Fix: compare elements directly with.every().Summary by CodeRabbit