fix(useLocalStorageState): wrap JSON.parse in try/catch in useState initializer - #42045
Closed
PedroHenrique0713 wants to merge 6 commits into
Closed
fix(useLocalStorageState): wrap JSON.parse in try/catch in useState initializer#42045PedroHenrique0713 wants to merge 6 commits into
PedroHenrique0713 wants to merge 6 commits into
Conversation
When evaluation produced an empty tree, parsedUpdates[0] was undefined and accessing .rhs threw TypeError. Now checks .length before access.
… widget children When a ListWidget had no children, mainContainerId was set to empty string '' instead of undefined. Downstream code treating '' as a valid widget ID produced silent failures.
Contributor
WalkthroughThe changes add defensive handling for missing widget containers, malformed local storage JSON, failed form expressions, and empty evaluation updates. They also clarify JSON round-trip behavior when preserving functions. ChangesRuntime safeguards
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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: 4
🤖 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/editor/browserTabsTracking.ts`:
- Line 11: Update the currentTabs initialization in browser tab tracking to
validate the JSON.parse result before assigning it to TabData. Accept only
non-null, non-array objects; otherwise retain the empty object fallback, while
continuing to fall back to {} for parse errors.
In `@app/client/src/utils/hooks/useLocalStorageState.ts`:
- Around line 10-14: Update the useState initializer in useLocalStorageState to
return initialValue immediately when localStorage.getItem(key) returns null,
before calling JSON.parse. Preserve JSON parsing and its existing fallback for
present but invalid values, then run yarn run check-types from app/client/.
In `@app/client/src/workers/Evaluation/formEval.ts`:
- Around line 332-334: Update the catch block in updateEvaluatedSectionConfig’s
form evaluation flow to clear the failed config entry’s existing output before
skipping it, preventing stale results from being reapplied. Preserve the current
skip behavior for the failed entry.
In `@app/client/src/workers/Evaluation/helpers.ts`:
- Around line 114-116: Update the JSON round-trip comments near the helper to
accurately describe transformations: Dates become strings, Sets/Maps/RegExps
become plain objects, and undefined is omitted or converted to null. Clarify
that function source strings are collected and re-injected, but callable
functions are not restored.
🪄 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: 886f7164-6537-4f90-8b04-06ea2c9f5ac1
📒 Files selected for processing (6)
app/client/src/sagas/WidgetOperationUtils.tsapp/client/src/utils/editor/browserTabsTracking.tsapp/client/src/utils/hooks/useLocalStorageState.tsapp/client/src/workers/Evaluation/formEval.tsapp/client/src/workers/Evaluation/handlers/evalTree.tsapp/client/src/workers/Evaluation/helpers.ts
| const getCurrentTabs = (): TabData => { | ||
| const currentTabsJSON = localStorage.getItem(LOCAL_STORAGE_KEY) || "{}"; | ||
| const currentTabs: TabData = JSON.parse(currentTabsJSON); | ||
| let currentTabs: TabData = {}; try { currentTabs = JSON.parse(currentTabsJSON); } catch { /* use empty default */ } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Locate file and outline:\n'
if [ -f app/client/src/utils/editor/browserTabsTracking.ts ]; then
wc -l app/client/src/utils/editor/browserTabsTracking.ts
ast-grep outline app/client/src/utils/editor/browserTabsTracking.ts || true
printf '\nRelevant file contents:\n'
cat -n app/client/src/utils/editor/browserTabsTracking.ts
else
fd -i 'browserTabsTracking' .
fi
printf '\nSearch TabData usages in relevant file(s):\n'
rg -n "TabData|currentTabs|Object\\.values\\(currentTabs\\)|delete currentTabs|JSON\\.parse\\(currentTabsJSON\\)" app/client/src || trueRepository: appsmithorg/appsmith
Length of output: 4807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Behavioral probe for getCurrentTabs parsing edge cases:\n'
node - <<'JS'
function getCurrentTabs(currentTabsJSON) {
let currentTabs = {};
try {
currentTabs = JSON.parse(currentTabsJSON);
} catch {
// default empty
}
return currentTabs;
}
const cases = ["null", "[]", "{}", "{}", "true", "\"x\"", "'invalid'", '{"a":"b"}'];
for (const input of cases) {
try {
const parsed = getCurrentTabs(input);
const values = Object.values(parsed);
delete parsed["tab:1"];
const spread = { ...parsed };
console.log(JSON.stringify(input), { parsed, values, deleteResult: delete parsed["tab:2"], spread });
} catch (e) {
console.log(JSON.stringify(input), { error: `${e.name}: ${e.message}` });
}
}
JS
printf '\nBehavioral probe for Object.values and spread/null/arrays:\n'
node - <<'JS'
for (const input of [null, [1], "x", true, 123, {}, {a: "b"}]) {
try {
console.log(String(input), { values: Object.values(input), spread: { ...input } });
} catch (e) {
console.log(String(input), { error: `${e.name}: ${e.message}` });
}
}
JSRepository: appsmithorg/appsmith
Length of output: 1192
Validate the parsed value before treating it as TabData.
The catch only handles invalid JSON syntax, but valid values like null, arrays, or primitives still reach the tab operations and can throw or be spread/indexed unexpectedly. Fall back to {} unless the parsed value is a non-array object.
Suggested fix
- let currentTabs: TabData = {}; try { currentTabs = JSON.parse(currentTabsJSON); } catch { /* use empty default */ }
+ let currentTabs: TabData = {};
+ try {
+ const parsed: unknown = JSON.parse(currentTabsJSON);
+ if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) {
+ currentTabs = parsed as TabData;
+ }
+ } catch {
+ // Use empty default.
+ }📝 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
| let currentTabs: TabData = {}; try { currentTabs = JSON.parse(currentTabsJSON); } catch { /* use empty default */ } | |
| let currentTabs: TabData = {}; | |
| try { | |
| const parsed: unknown = JSON.parse(currentTabsJSON); | |
| if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { | |
| currentTabs = parsed as TabData; | |
| } | |
| } catch { | |
| // Use empty default. | |
| } |
🤖 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/editor/browserTabsTracking.ts` at line 11, Update the
currentTabs initialization in browser tab tracking to validate the JSON.parse
result before assigning it to TabData. Accept only non-null, non-array objects;
otherwise retain the empty object fallback, while continuing to fall back to {}
for parse errors.
Source: Coding guidelines
Comment on lines
10
to
14
| const [storedValue, setStoredValue] = useState(() => { | ||
| const value = localStorage.getItem(key); | ||
|
|
||
| return value ? (JSON.parse(value) as Tvalue) : initialValue; | ||
| try { return JSON.parse(value) as Tvalue; } catch { return initialValue; } | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify nullable localStorage handling and TypeScript compatibility.
rg -n 'JSON\.parse\(value\)|getItem\(key\)' app/client/src/utils/hooks/useLocalStorageState.ts
yarn run check-typesRepository: appsmithorg/appsmith
Length of output: 455
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo files around useLocalStorageState =="
git ls-files | grep 'useLocalStorageState\.ts$' || true
echo "== file contents =="
if [ -f app/client/src/utils/hooks/useLocalStorageState.ts ]; then
cat -n app/client/src/utils/hooks/useLocalStorageState.ts
fi
echo "== package locations =="
git ls-files | grep -E '(^|/)package\.json$' | sed -n '1,80p'
echo "== local useLocalStorageState usages =="
rg -n 'useLocalStorageState|initialValue|LocalStorage' app/client/src -S || true
echo "== JavaScript null parse behavior (sandbox) =="
node - <<'JS'
console.log(JSON.stringify({
null: JSON.parse(null),
undefined: JSON.parse(undefined),
missing: JSON.parse()
}, null, 2));
try { JSON.parse(123); } catch (e) { console.log("number throws", e.name); }
try { JSON.parse(";"); } catch (e) { console.log("string throws", e.name); }
JSRepository: appsmithorg/appsmith
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -u
echo "== Direct useLocalStorageState import sites =="
python3 - <<'PY'
from pathlib import Path
for p in Path("app/client/src").rglob("*"):
if p.is_file() and p.suffix in {".ts",".tsx",".js",".jsx"}:
try:
txt=p.read_text(errors="ignore")
except Exception:
continue
if "useLocalStorageState" in txt:
lines=txt.splitlines()
for i,l in enumerate(lines,1):
if "useLocalStorageState" in l:
print(f"{p}:{i}:{l.strip()}")
PY
echo "== Relevant debugger tabs layout usages =="
for f in \
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/TabsLayout/index.tsx \
app/client/src/pages/Editor/CustomWidgetBuilder/Preview/Debugger/index.tsx \
app/client/src/pages/Editor/CustomWidgetBuilder/index.tsx \
app/client/src/pages/Editor/CustomWidgetBuilder/useCustomBuilder.tsx
do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,90p' "$f" | cat -n
fi
done
echo "== LocalStorage return/getItem implementation slices =="
sed -n '60,135p' app/client/src/utils/localStorage.tsx | cat -n
echo "== TypeScript null parse static/runtime invariant =="
node - <<'JS'
console.log({
JSON_parse_null: Object.is(JSON.parse(null), null),
JSON_parse_undefined_throws: (() => {
try { JSON.parse(undefined); return false; } catch (e) { return e.name; }
})(),
JSON_parse_string_object_null_invalid_json_throws: (() => {
try { JSON.parse("abc"); return false; } catch (e) { return e.name; }
})()
});
JSRepository: appsmithorg/appsmith
Length of output: 17550
🏁 Script executed:
#!/bin/bash
set -u
echo "== WebStorage implementation =="
sed -n '1,85p' app/client/src/utils/localStorage.tsx | cat -n
echo "== Direct usages with type context =="
python3 - <<'PY'
from pathlib import Path
for p in [
Path("app/client/src/pages/Editor/CustomWidgetBuilder/index.tsx"),
Path("app/client/src/pages/Editor/CustomWidgetBuilder/useCustomBuilder.tsx"),
Path("app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/TabsLayout/index.tsx"),
Path("app/client/src/pages/Editor/CustomWidgetBuilder/Preview/Debugger/index.tsx"),
]:
if p.exists():
print(f"-- {p} --")
for i, line in enumerate(p.read_text().splitlines(), 1):
if "useLocalStorageState" in line or "set" in line.lower() or "editorRef" in line or "\"Debugger\"" in line or "value={selectedTab}" in line:
print(f"{i}: {line.strip()}")
PYRepository: appsmithorg/appsmith
Length of output: 5241
Preserve initialValue when the storage key is missing.
getItem(key) returns string | null, but a missing value is still parsed: JSON.parse(null) is valid and returns null, so the hook no longer falls back to the provided default. Add value === null before JSON.parse; yarn run check-types must run from app/client/.
🤖 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/hooks/useLocalStorageState.ts` around lines 10 - 14,
Update the useState initializer in useLocalStorageState to return initialValue
immediately when localStorage.getItem(key) returns null, before calling
JSON.parse. Preserve JSON parsing and its existing fallback for present but
invalid values, then run yarn run check-types from app/client/.
Source: Coding guidelines
Comment on lines
+332
to
+334
| } catch (e) { | ||
| // form config evaluation error — skip this config entry | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate target file:"
if [ -f app/client/src/workers/Evaluation/formEval.ts ]; then
wc -l app/client/src/workers/Evaluation/formEval.ts
sed -n '280,360p' app/client/src/workers/Evaluation/formEval.ts | cat -n
else
echo "Target file not found"
fd -a 'formEval\.ts$' .
fi
echo
echo "Search related config.output references:"
rg -n "config\[.*\]\.output|\.output|evaluation error|skip this config" app/client/src/workers/Evaluation app/client/src || true
echo
echo "Outline nearby code if available:"
ast-grep outline app/client/src/workers/Evaluation/formEval.ts --view expanded 2>/dev/null | head -200 || trueRepository: appsmithorg/appsmith
Length of output: 8007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect worker interface/types and call sites:"
for f in app/client/src/workers/Evaluation/formEval.ts app/client/src/components/formControls/utils.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
ast-grep outline "$f" --view expanded 2>/dev/null | head -160 || true
fi
done
echo
echo "Search for evaluateFormConfigElements call sites:"
rg -n "evaluateFormConfigElements|FETCH_DYNAMIC_VALUES|EVALUATE_FORM_CONFIG|formDataPaths|evalConfigPaths" app/client/src/workers/Evaluation/formEval.ts app/client/src -g '*.ts' -g '*.tsx'
echo
echo "Read formEval relevant sections with line numbers:"
sed -n '220,430p' app/client/src/workers/Evaluation/formEval.ts | cat -n
sed -n '700,880p' app/client/src/components/formControls/utils.ts | cat -nRepository: appsmithorg/appsmith
Length of output: 21145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Behavioral probe for evaluateFormConfigElements semantics from source:"
python3 - <<'PY'
code = open("app/client/src/workers/Evaluation/formEval.ts", encoding="utf-8").read()
start = code.index("function evaluateFormConfigElements(")
end = code.index("// Function to run the eval for the whole form when data changes")
print(code[start:end])
source_file = "app/client/src/workers/Evaluation/formEval.ts"
updated_config = {
"a": {"expression": "true", "output": 1},
"b": {"expression": "false", "output": 2},
}
for path, item in updated_config.items():
evaluated_val = {"expression": "1/0", "output": 3}.get(path) # simulate eval exception for b
try:
# No eval() call; emulate exception path in catch.
raise Exception("demo")
except Exception:
# current behavior: catch leaves item.output unchanged.
print({"path": path, "output_before_exception": item["output"], "output_after_catch": item["output"]})
print({"output_at_b_after_catch": updated_config["b"]["output"]})
PY
echo
echo "Confirm updateEvaluatedSectionConfig reads output:"
sed -n '12,48p' app/client/src/components/formControls/utils.ts | cat -nRepository: appsmithorg/appsmith
Length of output: 2736
Clear stale output when form evaluation fails.
updateEvaluatedSectionConfig() applies evaluatedConfig[path].output, so a successful path that later fails leaves its previous output in the evaluated section configuration while claiming to skip that entry. Remove the stale value on catch, or document that preserving the last-known-good value is intentional.
Proposed fix
} catch (e) {
// form config evaluation error — skip this config entry
+ delete config[path].output;
}📝 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
| } catch (e) { | |
| // form config evaluation error — skip this config entry | |
| } | |
| } catch (e) { | |
| // form config evaluation error — skip this config entry | |
| delete config[path].output; | |
| } |
🤖 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/workers/Evaluation/formEval.ts` around lines 332 - 334, Update
the catch block in updateEvaluatedSectionConfig’s form evaluation flow to clear
the failed config entry’s existing output before skipping it, preventing stale
results from being reapplied. Preserve the current skip behavior for the failed
entry.
Comment on lines
+114
to
+116
| // Note: JSON round-trip strips Dates, Sets, Maps, RegExps, and undefined values. | ||
| // Functions are preserved (collected before, re-injected after), but other | ||
| // non-JSON-safe types may be lost. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files"
fd -a 'helpers\.(ts|test\.ts)$' app/client/src/workers/Evaluation 2>/dev/null || true
echo
echo "Relevant helper/test sections"
if [ -f app/client/src/workers/Evaluation/helpers.ts ]; then
wc -l app/client/src/workers/Evaluation/helpers.ts
sed -n '80,150p' app/client/src/workers/Evaluation/helpers.ts | cat -n
fi
if [ -f app/client/src/workers/Evaluation/helpers.test.ts ]; then
wc -l app/client/src/workers/Evaluation/helpers.test.ts
rg -n -C 8 'stringifyFnsInObject|restore.*Function|round-trip|JSON|function|Date|Set|Map|RegExp|undefined' app/client/src/workers/Evaluation/helpers.test.ts || true
fi
echo
echo "Precise implementation search"
rg -n 'stringifyFnsInObject|re-inject|re.inject|reinject|JSON\.stringify|JSON\.parse|fnValue\.toString|\(function|function ' app/client/src/workers/Evaluation/helpers.ts app/client/src/workers/Evaluation/helpers.test.tsRepository: appsmithorg/appsmith
Length of output: 9084
Clarify JSON round-trip value transformations.
Avoid implying every listed type is stripped: Dates become strings, Sets/Maps/RegExps become plain objects, and undefined is omitted or converted to null. Also state that function source strings are collected and re-injected; callable functions are not restored.
🤖 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/workers/Evaluation/helpers.ts` around lines 114 - 116, Update
the JSON round-trip comments near the helper to accurately describe
transformations: Dates become strings, Sets/Maps/RegExps become plain objects,
and undefined is omitted or converted to null. Clarify that function source
strings are collected and re-injected, but callable functions are not restored.
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.
Corrupted localStorage crashed React component via uncaught JSON.parse in useState initializer.
Summary by CodeRabbit
Bug Fixes
Documentation