fix: empty catch in BrandingUtils and JSON.parse safety in browserTabsTracking - #42044
fix: empty catch in BrandingUtils and JSON.parse safety in browserTabsTracking#42044PedroHenrique0713 wants to merge 5 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.
WalkthroughThe changes update ListWidgetV2 binding defaults, make browser tab parsing tolerant of invalid storage data, guard empty evaluation updates, and document form evaluation and function serialization behavior. ChangesRuntime robustness updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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/workers/Evaluation/helpers.ts (1)
114-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify that functions are restored as strings, not callable functions.
fnValue.toString()is reinjected after the JSON round-trip, and the accompanying tests confirm the resulting value is function source text. Please describe these as “function source strings” rather than saying functions are preserved.Suggested wording
- // Functions are preserved (collected before, re-injected after), but other - // non-JSON-safe types may be lost. + // Function source strings are collected before and re-injected after the + // JSON round-trip; the resulting values are not callable functions.🤖 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 explanatory comments near the JSON round-trip helper to state that functions are collected and re-injected as function source strings, not preserved as callable functions; keep the existing notes about other non-JSON-safe types unchanged.
🤖 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 and trackOpenEditorTabs parsing
flow to parse storage JSON as unknown, then validate that the result is a
non-null, non-array object whose values are all strings before treating it as
TabData. For invalid shapes, including null, primitives, arrays, or non-string
values, retain the empty-object fallback so Object.values in trackOpenEditorTabs
cannot throw.
---
Nitpick comments:
In `@app/client/src/workers/Evaluation/helpers.ts`:
- Around line 114-116: Update the explanatory comments near the JSON round-trip
helper to state that functions are collected and re-injected as function source
strings, not preserved as callable functions; keep the existing notes about
other non-JSON-safe types unchanged.
🪄 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: fd743343-f800-4eaa-86a1-aa861c786997
📒 Files selected for processing (5)
app/client/src/sagas/WidgetOperationUtils.tsapp/client/src/utils/editor/browserTabsTracking.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 */ } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the parsed storage shape before treating it as TabData.
JSON.parse can successfully return null, an array, or a primitive. For example, stored "null" bypasses this catch, then Object.values(currentTabs) in trackOpenEditorTabs throws and can interrupt editor startup. Parse into unknown and accept only a non-null, non-array object with string values; otherwise return {}.
Proposed fix
- let currentTabs: TabData = {}; try { currentTabs = JSON.parse(currentTabsJSON); } catch { /* use empty default */ }
+ let parsedTabs: unknown;
+ try {
+ parsedTabs = JSON.parse(currentTabsJSON);
+ } catch {
+ return {};
+ }
+
+ if (
+ !parsedTabs ||
+ typeof parsedTabs !== "object" ||
+ Array.isArray(parsedTabs) ||
+ !Object.values(parsedTabs).every(value => typeof value === "string")
+ ) {
+ return {};
+ }
+
+ const currentTabs = parsedTabs as TabData;📝 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.
| let currentTabs: TabData = {}; try { currentTabs = JSON.parse(currentTabsJSON); } catch { /* use empty default */ } | |
| let parsedTabs: unknown; | |
| try { | |
| parsedTabs = JSON.parse(currentTabsJSON); | |
| } catch { | |
| return {}; | |
| } | |
| if ( | |
| !parsedTabs || | |
| typeof parsedTabs !== "object" || | |
| Array.isArray(parsedTabs) || | |
| !Object.values(parsedTabs).every(value => typeof value === "string") | |
| ) { | |
| return {}; | |
| } | |
| const currentTabs = parsedTabs as TabData; |
🤖 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 and trackOpenEditorTabs parsing flow to parse storage
JSON as unknown, then validate that the result is a non-null, non-array object
whose values are all strings before treating it as TabData. For invalid shapes,
including null, primitives, arrays, or non-string values, retain the
empty-object fallback so Object.values in trackOpenEditorTabs cannot throw.
Source: Coding guidelines
|
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
Empty catch swallowed corrupted localStorage silently. browserTabsTracking JSON.parse without try/catch crashes module.
Summary by CodeRabbit
Bug Fixes
Documentation