fix(formSelectors): guard against missing action with empty string key - #42038
Closed
PedroHenrique0713 wants to merge 5 commits into
Closed
fix(formSelectors): guard against missing action with empty string key#42038PedroHenrique0713 wants to merge 5 commits into
PedroHenrique0713 wants to merge 5 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.
Contributor
WalkthroughThis PR adds defensive guards to form selectors and utility functions, escapes regex prefixes, corrects archived-action and diff-path handling, changes array comparison and mutation behavior, and improves retry, URL, visibility, and widget error handling. ChangesClient corrections
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/client/src/utils/AppsmithUtils.tsx (1)
318-330: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake all retry termination paths settle the promise.
retriesLeft === 1retries forever when initialized with0or a negative value. Also,shouldRetry(e) === falseleaves the promise pending, and the recursive call drops the caller’s custom predicate. UseretriesLeft <= 1, reject when retrying is disallowed, and passshouldRetryrecursively.Suggested fix
- if (shouldRetry(e)) { + if (!shouldRetry(e)) { + reject(e); + return; + } + + { setTimeout(async () => { - if (retriesLeft === 1) { + if (retriesLeft <= 1) { reject({ code: ERROR_CODES.SERVER_ERROR, message: createMessage(ERROR_500), show: false, }); return; } - retryPromise(fn, retriesLeft - 1, interval).then(resolve, reject); + retryPromise( + fn, + retriesLeft - 1, + interval, + shouldRetry, + ).then(resolve, reject);🤖 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 - 330, Update retryPromise in the shouldRetry failure path so retriesLeft <= 1 rejects immediately, shouldRetry(e) === false also rejects instead of leaving the promise pending, and recursive retryPromise calls forward the caller’s shouldRetry predicate while preserving existing error details and retry behavior.
🧹 Nitpick comments (1)
app/client/src/utils/AppsmithUtils.tsx (1)
442-442: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSort
arr2once outside the loop.The current expression copies and sorts
arr2for every element, causing unnecessary allocations and worst-caseO(n² log n)work. Hoist both sorted arrays before callingevery.Suggested 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 expression in AppsmithUtils so both arr1 and arr2 are copied and sorted once before every runs, then compare the precomputed sorted arrays by index. Preserve the existing equality behavior while eliminating repeated arr2 sorting inside the callback.
🤖 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 57-58: Update getFormConfigConditionalOutput and
getDynamicFetchedValues to use shared typed empty defaults containing the
consumer-read fields isLoading, data, evaluatedConfig, and config. Replace the
missing-action `{}` casts and the fallback near line 84 with the appropriate
shared defaults, preserving each function’s output contract, then run yarn run
check-types.
---
Outside diff comments:
In `@app/client/src/utils/AppsmithUtils.tsx`:
- Around line 318-330: Update retryPromise in the shouldRetry failure path so
retriesLeft <= 1 rejects immediately, shouldRetry(e) === false also rejects
instead of leaving the promise pending, and recursive retryPromise calls forward
the caller’s shouldRetry predicate while preserving existing error details and
retry behavior.
---
Nitpick comments:
In `@app/client/src/utils/AppsmithUtils.tsx`:
- Line 442: Update the array comparison expression in AppsmithUtils so both arr1
and arr2 are copied and sorted once before every runs, then compare the
precomputed sorted arrays by index. Preserve the existing equality behavior
while eliminating repeated arr2 sorting inside the callback.
🪄 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: b47ebb78-99bf-4a60-b82a-1b338a79f027
📒 Files selected for processing (7)
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.tsx
Comment on lines
+57
to
+58
| const actionId = action?.id; | ||
| if (!actionId) return {} as ConditionalOutput; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
yarn run check-typesRepository: appsmithorg/appsmith
Length of output: 325
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | sed -n '1,120p'
echo
echo "== locate file =="
fd -a 'formSelectors\.ts$' . || true
echo
echo "== inspect formSelectors =="
file="$(fd 'formSelectors\.ts$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
cat -n "$file"
fi
echo
echo "== consumers of selectors =="
rg -n "fetchDynamicValues|dynamicFetchedValues|SegmentedControl|DropDownControl|isLoading|data" app/client/src -g '*.ts' -g '*.tsx' | sed -n '1,240p'Repository: appsmithorg/appsmith
Length of output: 37166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package locations =="
find . -maxdepth 4 -path '*/package.json' | sed 's#^\./##' | head -80
echo
echo "== check-types scripts =="
python3 - <<'PY'
import json, pathlib
for p in pathlib.Path('.').rglob('package.json'):
if 'node_modules' in p.parts or '.git' in p.parts:
continue
try:
data=json.loads(p.read_text())
except Exception:
continue
for section in ('scripts','devDependencies','dependencies'):
if 'typescript' in str(data.get(section,'')) or 'check-types' in str(data.get('scripts',{})):
print(p)
print(json.dumps({section:data.get(section)}, indent=2))
PY
echo
echo "== locate DynamicValues/types =="
rg -n "export (interface|type) .*ConditionalOutput|export (interface|type) .*DynamicValues|DynamicValues|ConditionalOutput" app/client/src -g '*.ts' -g '*.tsx' | sed -n '1,200p'
echo
echo "== precise selectors and imports =="
for f in $(rg -l "getFormConfigConditionalOutput|getDynamicFetchedValues|extractConditionalOutput" app/client/src -g '*.ts' -g '*.tsx'); do
echo "--- $f ---"
wc -l "$f"
rg -n "getFormConfigConditionalOutput|getDynamicFetchedValues|extractConditionalOutput|isLoading|dynamicFetchedValues|SegmentedControl|DropDownControl" "$f" -C 4 | sed -n '1,220p'
doneRepository: appsmithorg/appsmith
Length of output: 45796
Return shared typed default values from the missing-action guards.
When actionId is missing, both getFormConfigConditionalOutput and getDynamicFetchedValues return {} cast to their output types, so consumers receive undefined for fields such as isLoading, data, evaluatedConfig, and config. Define shared empty default values with the fields consumers read and return them here and from the fallback on line 84. Run yarn run check-types to keep TypeScript contracts consistent.
Also applies to: 77-78
🤖 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 57 - 58, Update
getFormConfigConditionalOutput and getDynamicFetchedValues to use shared typed
empty defaults containing the consumer-read fields isLoading, data,
evaluatedConfig, and config. Replace the missing-action `{}` casts and the
fallback near line 84 with the appropriate shared defaults, preserving each
function’s output contract, then run yarn run check-types.
Source: Coding guidelines
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
When
getActionByBaseIdreturns undefined,action?.id ?? ""produces an empty string""used as object key instate.evaluations.triggers[""], which returnsundefined. This silently passes throughextractConditionalOutputproducing incorrect behavior.Fix: Return early with an empty/default object when no action is found, in both
getFormConfigConditionalOutputandgetFormConfigConditionalDynamicValuesselectors.Summary by CodeRabbit