fix: Replace cloneDeep with klona #12936 #34477
Conversation
JaiZemoso
requested review from
a team and
marks0351
and removed request for
a team
WalkthroughThe primary change involves replacing the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (22)
app/client/src/reducers/uiReducers/appCollabReducer.ts (1)
Line range hint
41-47: Check the variable name in the return statement.The variable
clonedPointerDatashould bepointerDatain the return statement to reflect the updated state correctly.- clonedPointerData, + pointerData: clonedPointerData,app/client/src/widgets/ChartWidget/component/helpers.ts (1)
Line range hint
90-92: Consider simplifying control flow.The static analysis tool suggests removing unnecessary else clauses. This can simplify the control flow and improve code readability.
- else { - return {}; - }Also applies to: 122-126, 124-126
app/client/src/transformers/RestActionTransformer.ts (1)
Line range hint
39-39: Optimize performance and simplify code.Replace the delete operator with undefined assignment for better performance and use optional chaining to simplify the code.
- delete action.actionConfiguration.body; + action.actionConfiguration.body = undefined; - if (action.actionConfiguration.queryParameters && + if (action.actionConfiguration.queryParameters?.length) {Also applies to: 44-45
app/client/packages/dsl/src/migrate/migrations/009-table-widget-property-pane-migration.ts (1)
Line range hint
70-70: Optimization suggestions from static analysis.Consider implementing optional chaining and simplifying the ternary operation as suggested by static analysis to improve code clarity and reduce complexity.
- if (dynamicBindingPathList?.findIndex((item: { key: string }) => item.key !== "tableData")) + if (dynamicBindingPathList?.findIndex((item: { key: string }) => item.key !== "table - isVisible: hiddenColumns.includes(accessor) ? false : true, + isVisible: !hiddenColumns.includes(accessor),Also applies to: 83-83, 103-103, 109-109, 116-116
app/client/src/widgets/ChartWidget/component/CustomEChartIFrameComponent.tsx (1)
Line range hint
106-106: Recommendation to replacehasOwnPropertywithObject.hasOwn.- if (!config || !config.hasOwnProperty("options")) { + if (!config || !Object.hasOwn(config, "options")) {Replacing
hasOwnPropertywithObject.hasOwnis recommended for improved safety and compatibility with prototypes.app/client/src/widgets/TabsMigrator/widget/index.tsx (1)
Line range hint
29-29: Performance and code quality improvements.Consider replacing the delete operator with undefined assignments and implementing optional chaining as suggested by static analysis to improve performance and reduce complexity.
- delete currentDSL.tabs; + currentDSL.tabs = undefined; - if (currentDSL.children && currentDSL.children.length) { + if (currentDSL.children?.length) {Also, avoid using the spread syntax on accumulators as it can lead to performance issues.
- obj = { - ...obj, - [tab.id]: { - ...tab, - isVisible: tab.isVisible === undefined ? true : tab.isVisible, - index, - }, - }; + obj[tab.id] = { + ...tab, + isVisible: tab.isVisible === undefined ? true : tab.isVisible, + index, + };Also applies to: 32-32, 101-101, 108-108, 111-111, 89-89
app/client/src/utils/migrations/TableWidget.ts (1)
Line range hint
33-33: ReplacehasOwnPropertywithObject.hasOwn.Using
Object.hasOwnis safer and recommended to avoid potential issues with the prototype chain.- if (child.hasOwnProperty("isSortable")) { + if (Object.hasOwn(child, "isSortable")) {Apply similar changes to all other instances where
hasOwnPropertyis used.Also applies to: 566-566, 630-630, 689-689, 709-709, 732-732, 782-782
app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (1)
Line range hint
757-757: Remove Redundant Catch BlockThe catch block at this line is redundant as it only rethrows the error without handling it. Removing this block can simplify the error handling flow.
- } catch (error) { - throw error; - }app/client/src/widgets/ListWidget/widget/index.tsx (2)
Line range hint
1426-1426: ReplaceisNaNwithNumber.isNaNfor safer type checking.The use of
isNaNcan lead to unexpected type coercion. UsingNumber.isNaNprovides a more reliable check.- if (isNaN(templateHeight) || templateHeight > componentHeight - 45) { + if (Number.isNaN(templateHeight) || templateHeight > componentHeight - 45) {
Line range hint
1397-1397: Provide a valid value for thehrefattribute in the anchor tag.The
hrefattribute should contain a valid URL to improve accessibility and ensure proper navigation.- <a className=".modifier" href="#"> + <a className=".modifier" href="/desired-path">app/client/src/widgets/TableWidget/widget/index.tsx (4)
Line range hint
488-489: Suggestion: Use optional chaining for better safety and readability.Consider using optional chaining to simplify the access to nested properties and improve code safety.
- if (columnProperties && columnProperties.hasOwnProperty("boxShadowColor") ... + if (columnProperties?.hasOwnProperty("boxShadowColor") ...
Line range hint
531-531: ReplacehasOwnPropertywithObject.hasOwn.Using
Object.hasOwn()is recommended overhasOwnPropertyfor checking property existence as it is safer and part of the latest JavaScript standards.- if (columnProperties.hasOwnProperty("boxShadowColor") ... + if (Object.hasOwn(columnProperties, "boxShadowColor") ...Also applies to: 532-532
Line range hint
576-665: Remove unnecessaryelseclauses.These
elseclauses are redundant and can be omitted to improve code readability and reduce complexity.- else { - ... - }Also applies to: 590-665, 606-665, 629-665, 654-665
Line range hint
722-722: Wrap declarations in switch cases in a block.To prevent scope leakage, wrap the declarations within each switch case in a block.
case ColumnTypes.DATE: + { let isValidDate = true; ... + }Also applies to: 723-725, 726-726, 764-769
app/client/src/sagas/WidgetOperationUtils.ts (3)
Line range hint
104-105: Refactor to use optional chaining for better safety and readability.Consider using optional chaining to simplify access to nested properties and enhance code safety.
- let root = _.get(widgets, `${widget.parentId}`); + let root = widgets?.[widget.parentId];Also applies to: 134-134, 304-304, 398-398, 441-442, 504-505, 801-801, 1480-1480, 1537-1537, 1631-1631, 1640-1640
Line range hint
306-306: ReplacehasOwnPropertywithObject.hasOwnfor safer property checks.Using
Object.hasOwninstead ofhasOwnPropertydirectly on objects is safer and recommended by modern JavaScript practices.- if (children.hasOwnProperty(childIndex)) { + if (Object.hasOwn(children, childIndex)) {Also applies to: 400-400, 1713-1713
Line range hint
809-818: Remove unnecessary else clauses to simplify control flow.Omitting unnecessary else clauses can simplify the logic and improve code readability.
- else { - // logic here - }Also applies to: 922-922, 1719-1729, 1722-1729, 1730-1733
app/client/src/sagas/WidgetOperationSagas.tsx (3)
Line range hint
360-365: Consider removing unnecessary else clauses.Based on static analysis, several else clauses are redundant as the preceding branches terminate early. Simplifying these constructs can improve readability and reduce nesting.
- else { - // code - }Also applies to: 415-420, 480-484
Line range hint
1714-1714: Avoid using the delete operator.The
deleteoperator is used which can negatively impact performance due to deoptimizations in JavaScript engines. Consider alternatives like setting the property toundefinedor restructuring the object without the property.- delete obj[property]; + obj[property] = undefined;
Line range hint
804-804: Optimize the use of spread syntax in accumulators.Using spread syntax in accumulators can lead to quadratic time complexity. Consider using methods that modify the accumulator directly.
- const result = [...accumulator, newItem]; + accumulator.push(newItem);app/client/src/reflow/reflowUtils.ts (2)
Line range hint
347-347: Refactor to use optional chaining.Consider refactoring the code to use optional chaining for better readability and safety. This would prevent potential runtime errors when accessing properties on undefined objects.
- if (prevMovementMap && prevMovementMap[collidingSpace.id] && prevMovementMap[collidingSpace.id][movementDirectionAccessor]) + if (prevMovementMap?.[collidingSpace.id]?.[movementDirectionAccessor])Also applies to: 352-352, 359-360, 370-374, 588-589, 612-613, 818-818, 1436-1436, 1438-1438, 1488-1489, 1492-1492, 1555-1555, 1558-1558
Line range hint
852-854: Remove unnecessary else clauses.The code can be simplified by removing unnecessary else clauses, which are redundant due to previous branches that break early or return values. This will enhance the readability and maintainability of the code.
- if (condition) { - return value; - } else { - // other code - } + if (condition) { + return value; + } + // other codeAlso applies to: 887-899, 889-899, 894-899, 903-915, 905-915, 910-915
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (20)
- app/client/packages/dsl/src/migrate/migrations/009-table-widget-property-pane-migration.ts (2 hunks)
- app/client/src/layoutSystems/anvil/utils/paste/utils.ts (3 hunks)
- app/client/src/pages/Editor/FirstTimeUserOnboarding/testUtils.ts (2 hunks)
- app/client/src/reducers/uiReducers/appCollabReducer.ts (2 hunks)
- app/client/src/reflow/reflowUtils.ts (2 hunks)
- app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (2 hunks)
- app/client/src/sagas/BuildingBlockSagas/tests/PasteBuildingBlockWidgetSaga.test.ts (2 hunks)
- app/client/src/sagas/CanvasSagas/DraggingCanvasSagas.ts (2 hunks)
- app/client/src/sagas/WidgetOperationSagas.tsx (6 hunks)
- app/client/src/sagas/WidgetOperationUtils.ts (3 hunks)
- app/client/src/transformers/RestActionTransformer.ts (1 hunks)
- app/client/src/utils/WidgetMigrationUtils.test.ts (2 hunks)
- app/client/src/utils/migrations/TableWidget.ts (2 hunks)
- app/client/src/widgets/ChartWidget/component/CustomEChartIFrameComponent.tsx (3 hunks)
- app/client/src/widgets/ChartWidget/component/helpers.ts (2 hunks)
- app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/common.ts (2 hunks)
- app/client/src/widgets/ListWidget/widget/index.tsx (3 hunks)
- app/client/src/widgets/TableWidget/widget/index.tsx (2 hunks)
- app/client/src/widgets/TableWidgetV2/component/header/actions/filter/FilterPaneContent.tsx (3 hunks)
- app/client/src/widgets/TabsMigrator/widget/index.tsx (2 hunks)
Files skipped from review due to trivial changes (2)
- app/client/src/pages/Editor/FirstTimeUserOnboarding/testUtils.ts
- app/client/src/sagas/CanvasSagas/DraggingCanvasSagas.ts
Additional context used
Biome
app/client/src/widgets/ChartWidget/component/helpers.ts
[error] 90-92: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 122-126: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 124-126: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
app/client/src/transformers/RestActionTransformer.ts
[error] 39-39: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 44-45: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/client/packages/dsl/src/migrate/migrations/009-table-widget-property-pane-migration.ts
[error] 70-70: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 83-83: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 103-103: Unnecessary use of boolean literals in conditional expression. (lint/complexity/noUselessTernary)
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
[error] 109-109: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 116-116: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/client/src/widgets/ChartWidget/component/CustomEChartIFrameComponent.tsx
[error] 106-106: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.app/client/src/widgets/TabsMigrator/widget/index.tsx
[error] 29-29: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 32-32: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 101-101: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 108-108: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 111-111: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 89-89: Avoid the use of spread (
...) syntax on accumulators. (lint/performance/noAccumulatingSpread)Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.app/client/src/widgets/TableWidgetV2/component/header/actions/filter/FilterPaneContent.tsx
[error] 199-199: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
app/client/src/utils/migrations/TableWidget.ts
[error] 33-33: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 84-84: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 97-97: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 117-117: Unnecessary use of boolean literals in conditional expression. (lint/complexity/noUselessTernary)
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
[error] 123-123: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 130-130: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 342-343: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 566-566: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 630-630: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 689-689: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 709-709: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 732-732: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 782-782: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts
[error] 757-757: The catch clause that only rethrows the original error is redundant. (lint/complexity/noUselessCatch)
These unnecessary catch clauses can be confusing. It is recommended to remove them.
app/client/src/widgets/ListWidget/widget/index.tsx
[error] 44-44: Do not shadow the global "toString" property. (lint/suspicious/noShadowRestrictedNames)
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
[error] 1397-1397: Provide a valid value for the attribute href. (lint/a11y/useValidAnchor)
The href attribute should be a valid a URL
Check this thorough explanation to better understand the context.
[error] 47-47: Shouldn't redeclare 'klona'. Consider to delete it or rename it. (lint/suspicious/noRedeclare)
'klona' is defined here:
[error] 1426-1426: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.app/client/src/widgets/TableWidget/widget/index.tsx
[error] 488-489: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 531-531: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 532-532: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 576-665: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 590-665: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 606-665: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 629-665: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 654-665: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 722-722: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 723-725: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 726-726: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 764-769: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 896-897: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
app/client/src/sagas/WidgetOperationUtils.ts
[error] 104-105: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 134-134: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 304-304: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 306-306: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 398-398: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 400-400: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 441-442: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 504-505: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 801-801: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 809-818: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 922-922: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1480-1480: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1537-1537: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1631-1631: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1640-1640: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1713-1713: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 1719-1729: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1722-1729: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 1730-1733: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
app/client/src/sagas/WidgetOperationSagas.tsx
[error] 360-365: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 415-420: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 480-484: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1714-1714: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
[error] 804-804: Avoid the use of spread (
...) syntax on accumulators. (lint/performance/noAccumulatingSpread)Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.app/client/src/reflow/reflowUtils.ts
[error] 347-347: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 352-352: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 359-360: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 370-374: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 588-589: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 612-613: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 818-818: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 852-854: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 887-899: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 889-899: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 894-899: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 903-915: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 905-915: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 910-915: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 1436-1436: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1438-1438: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1488-1489: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1492-1492: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1555-1555: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1558-1558: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (33)
app/client/src/utils/WidgetMigrationUtils.test.ts (1)
33-33: Replacement ofcloneDeepwithklonalooks good.This change is part of the PR objectives to replace
cloneDeepwithklona. Ensure thatklonacorrectly replicates the deep cloning functionality needed in this test scenario.app/client/src/reducers/uiReducers/appCollabReducer.ts (1)
5-5: Import ofklonais correctly placed.This change is consistent with the PR’s goal of replacing
cloneDeepwithklona.app/client/src/widgets/ChartWidget/component/helpers.ts (2)
3-4: Import statements are updated correctly.The replacement of
cloneDeepwithklonais consistent with the PR’s objectives.
26-26: Replacement ofcloneDeepwithklonain event data processing.Ensure that
klonaadequately replicates the deep cloning functionality required for handling chart event data.app/client/src/transformers/RestActionTransformer.ts (2)
8-8: Import ofklonais correctly placed.This change aligns with the PR’s objectives to replace
cloneDeepwithklona.
15-15: Replacement ofcloneDeepwithklonafor cloning API action data.Confirm that
klonaprovides the necessary functionality for cloning API data without losing any properties or causing side effects.Verification successful
Replacement of
cloneDeepwithklonafor cloning API action data is appropriate.The extensive usage of
klonaacross the codebase, including in numerous test cases, and the comments indicating its performance benefits and deep cloning capabilities, confirm thatklonaprovides the necessary functionality for cloning API data without losing properties or causing side effects.
- Examples of
klonausage in tests:
app/client/src/sagas/PasteWidgetUtils/PasteWidgetUtils.test.tsapp/client/src/utils/klona.test.tsapp/client/src/widgets/JSONFormWidget/schemaParser.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any issues reported with `klona` in similar scenarios. rg --type markdown 'klona issues'Length of output: 35
Script:
#!/bin/bash # Search for all instances of `klona` usage in the codebase. rg 'klona' # Search for all instances of `cloneDeep` usage in the codebase. rg 'cloneDeep' # Search for test cases involving cloning API action data. rg --type js 'klona' -g '*test*'Length of output: 47934
app/client/src/sagas/BuildingBlockSagas/tests/PasteBuildingBlockWidgetSaga.test.ts (2)
19-19: Import ofklonais correctly placed.This change is consistent with the PR’s goal of replacing
cloneDeepwithklona.
129-129: Use ofklonain test saga for cloning widget lists.Ensure that
klonacorrectly replicates the deep cloning functionality needed in this test scenario.app/client/src/layoutSystems/anvil/utils/paste/utils.ts (3)
3-3: Import statement for klona is correct.The import statement for
klonais correctly formatted and placed among other import statements, ensuring clarity and maintainability.
46-46: Replacement ofcloneDeepwithklonain widget cloning.The change from
cloneDeeptoklonafor cloning widget properties is correctly implemented. Ensure thatklonameets all deep cloning needs as per the widget properties' complexity.
77-77: Ensure correct cloning of layout properties.The replacement of
cloneDeepwithklonafor cloning layout properties within theupdateLayoutPropsfunction is correctly implemented. It's crucial to verify thatklonaaccurately clones nested properties if any.app/client/packages/dsl/src/migrate/migrations/009-table-widget-property-pane-migration.ts (2)
2-2: Import statement for klona is correct.The import statement for
klonais correctly formatted and placed among other import statements, ensuring clarity and maintainability.
28-28: Correct implementation of klona for DSL widget cloning.The use of
klonafor cloning DSL widget instances in the migration function is correctly implemented. This ensures that the widget's state is accurately copied during the migration process.app/client/src/widgets/ChartWidget/component/CustomEChartIFrameComponent.tsx (3)
8-8: Import statement for klona is correct.The import statement for
klonais correctly formatted and placed among other import statements, ensuring clarity and maintainability.
147-147: Correct use of klona for event data cloning in ECharts.The replacement of
cloneDeepwithklonafor cloning event data in the ECharts component is correctly implemented. This ensures accurate and efficient copying of event data for further processing.
163-163: Correct use of klona for cloning chart configuration.The use of
klonafor cloning the chart configuration before modifying it for callback functions is correctly implemented. This ensures that the original configuration is not mutated unintentionally.app/client/src/widgets/TabsMigrator/widget/index.tsx (2)
8-9: Import statements for lodash and klona are correct.The import statements for
lodashandklonaare correctly formatted and placed among other import statements, ensuring clarity and maintainability.
264-264: Correct implementation of klona for props cloning in TabsMigrator.The use of
klonafor cloning props in theTabsMigratorWidgetcomponent is correctly implemented. This ensures that the props are accurately copied during the migration process.app/client/src/widgets/TableWidgetV2/component/header/actions/filter/FilterPaneContent.tsx (2)
21-21: Import statement for klona is correct.The import statement for
klonais correctly formatted and placed among other import statements, ensuring clarity and maintainability.
214-214: Correct use of klona for cloning filter data.The replacement of direct array manipulation with
klonafor cloning filter data in theTableFilterPaneContentcomponent is correctly implemented. This ensures that the filter data is accurately copied before modification, preventing unintended mutations.Also applies to: 241-241
app/client/src/widgets/JSONFormWidget/widget/propertyConfig/properties/common.ts (2)
69-69: Replacement ofcloneDeepwithklonalooks appropriate.The usage of
klonahere is correct and should maintain the intended functionality for deep cloning properties fromprops.
71-71: Good use ofklonafor cloning individual schema items.This ensures that modifications to
currentSchemaItemdo not affect the originalschemaobject, preserving immutability which is crucial in Redux-like state management.app/client/src/utils/migrations/TableWidget.ts (1)
44-44: Proper implementation ofklonafor cloning widget properties.This change correctly replaces
cloneDeepwithklona, which is expected to handle deep cloning of widget properties efficiently.app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (2)
10-10: Correct Import ofklonaThe import statement for
klonais correct and follows standard practices.
525-525: Appropriate Replacement ofcloneDeepwithklonaThe usage of
klonaat line 525 for cloning widget properties is appropriate and should maintain the integrity of deep cloning as required by the application logic.app/client/src/widgets/TableWidget/widget/index.tsx (1)
253-253: Proper use ofklonafor deep cloning.The replacement of
cloneDeepwithklonais correctly implemented here to avoid direct state mutations, which is a best practice in React development.app/client/src/sagas/WidgetOperationUtils.ts (3)
35-35: Approved replacement ofcloneDeepwithklona.The import and usage of
klonafor deep cloning in widget operations is a performance improvement. Ensure it is tested thoroughly to verify that deep cloning is performed as expected.
137-137: Approved use ofklonain list widget handling.The use of
klonafor cloningcurrentWidgetinhandleIfParentIsListWidgetWhilePastingfunction is correct and should help maintain performance with deep cloning operations.
1389-1389: Approved use ofklonafor state cloning infilterOutSelectedWidgets.The use of
klonato clone the state before modifying it ensures immutability, which is a good practice in Redux Saga operations.app/client/src/sagas/WidgetOperationSagas.tsx (2)
556-556: Review ofklonausage for cloning state.The replacement of
cloneDeepwithklonahas been correctly implemented in these lines. It's crucial to ensure thatklonaeffectively handles all edge cases previously managed bycloneDeep, especially with complex nested objects and different data types.Also applies to: 579-579, 615-615, 708-708
1314-1314: Ensureklonaproperly clones widget properties.The use of
klonahere is intended to clone widget properties before modification. Given the complexity of widget properties, verify through additional testing thatklonamaintains all behaviors, especially for non-primitive values.app/client/src/reflow/reflowUtils.ts (2)
3-3: ReplacecloneDeepwithklonais appropriate for deep cloning.
The change fromcloneDeeptoklonaat this import statement is consistent with the PR's objective to enhance performance by using a more efficient deep cloning library.
1424-1424: Ensure correct use ofklonafor deep cloning.The replacement of
cloneDeepwithklonahere is consistent with the PR's objective. It's crucial to ensure thatklonacorrectly handles all data structures involved, particularly since this function deals with potentially complex objects in the state management of UI layouts.Verification successful
Ensure correct use of
klonafor deep cloning.The replacement of
cloneDeepwithklonaappears consistent with the PR's objective. The extensive usage ofklonaacross the codebase and the presence of tests verifying its functionality suggest that it correctly handles the deep cloning of complex objects, including nested objects and arrays.
app/client/src/reflow/reflowUtils.tsline 1424:if (!prevMovementMap) return klona(occupiedSpacesMap);Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `klona` handles all data structures as expected. echo "Manual testing required: Ensure klona handles nested objects and arrays correctly in app/client/src/reflow/reflowUtils.ts"Length of output: 252
Script:
#!/bin/bash # Search for all instances of `klona` usage in the codebase rg --word-regexp 'klona' --context 5 # Search for all instances of `cloneDeep` usage in the codebase rg --word-regexp 'cloneDeep' --context 5Length of output: 260087
JaiZemoso
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
app/client/src/widgets/ListWidget/widget/index.tsx (2)
Line range hint
1425-1425: ReplaceisNaNwithNumber.isNaNfor safer type checking.The use of
isNaNcan lead to unexpected results due to type coercion. UseNumber.isNaNfor a more accurate check.- if (isNaN(templateHeight) || templateHeight > componentHeight - 45) { + if (Number.isNaN(templateHeight) || templateHeight > componentHeight - 45) {
Line range hint
1396-1396: Provide a validhrefattribute in the anchor tag.The
hrefattribute should be a valid URL to ensure proper navigation and accessibility standards.- <a className=".modifier" href="#"> + <a className=".modifier" href="javascript:void(0);" onClick={(e) => e.preventDefault()}>This change prevents the default link behavior, making it clear that this is a placeholder or non-navigational link.
app/client/src/sagas/WidgetOperationSagas.tsx (3)
Line range hint
360-365: Consider removing unnecessary else clause.- else { + // Removed unnecessary else clauseThe else clause after an early break can be safely removed to simplify the code.
Line range hint
1714-1714: Avoid using the delete operator for performance reasons.Consider refactoring the code to avoid using the delete operator, which can degrade performance in JavaScript environments.
Line range hint
804-804: Avoid using spread syntax on accumulators.- const updatedStateWidgets = updatedWidgetsAndActionsToDispatch.reduce( - (allWidgets, eachUpdatedWidgetAndActionsToDispatch) => { - return { - ...allWidgets, - [eachUpdatedWidgetAndActionsToDispatch.updatedWidget.widgetId]: - eachUpdatedWidgetAndActionsToDispatch.updatedWidget, - }; - }, - stateWidgets, - ); + const updatedStateWidgets = {}; + updatedWidgetsAndActionsToDispatch.forEach(each => { + updatedStateWidgets[each.updatedWidget.widgetId] = each.updatedWidget; + });Replacing the spread syntax in the accumulator with a forEach loop improves performance by avoiding the creation of intermediate copies of the object.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/sagas/WidgetOperationSagas.tsx (13 hunks)
- app/client/src/widgets/ListWidget/widget/index.tsx (4 hunks)
Additional context used
Biome
app/client/src/widgets/ListWidget/widget/index.tsx
[error] 1396-1396: Provide a valid value for the attribute href. (lint/a11y/useValidAnchor)
The href attribute should be a valid a URL
Check this thorough explanation to better understand the context.
[error] 1425-1425: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.app/client/src/sagas/WidgetOperationSagas.tsx
[error] 360-365: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 415-420: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 480-484: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1714-1714: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
[error] 804-804: Avoid the use of spread (
...) syntax on accumulators. (lint/performance/noAccumulatingSpread)Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.
Additional comments not posted (7)
app/client/src/widgets/ListWidget/widget/index.tsx (2)
Line range hint
39-39: Import of klona is correctly implemented.The import statement
import { klona } from 'klona/lite';correctly replaces the previouscloneDeepusage. This should improve performance asklona/liteis optimized for typical JavaScript object cloning scenarios.
341-341: Correct use ofklonain dynamic property updating.The usage of
klonain the function to clone widget properties ensures that each widget instance maintains its own state without unintended reference sharing. This is crucial for correct functionality in reactive UI environments.app/client/src/sagas/WidgetOperationSagas.tsx (5)
35-36: Approved changes to import statements.The import of
klonafrom theklonalibrary is correctly replacingcloneDeepfromlodash, aligning with the PR's objectives to improve performance.
556-556: Replacement ofcloneDeepwithklonais correctly implemented inresizeSaga.The use of
klonafor cloning widget states inresizeSagais correctly implemented and should maintain the integrity of the function while potentially improving performance.
589-589: Review the functiongetDynamicTriggerPathListUpdatefor any potential issues.The logic in
getDynamicTriggerPathListUpdateshould be carefully reviewed to ensure it correctly handles dynamic trigger paths updates.
615-615: Batch update function usesklonaeffectively.The
batchUpdateWidgetDynamicPropertySagafunction correctly usesklonafor cloning state widgets, which is in line with the PR's objectives.
579-579: Check the use ofklonainreflowWidgets.The use of
klonainreflowWidgetsneeds to be carefully verified to ensure it handles the cloning requirements without issues.
|
Can you kindly check this Pull request ? |
marks0351
requested review from
rishabhrathod01
and removed request for
marks0351
|
@rishabhrathod01 can you kindly review this PR ? |
|
@JaiZemoso the task is not as simple as the title. We cannot directly replace the library here. Each such replacement will need to be tested either manually or using automation. As this would be quite a huge change hence this task was removed from the |
|
@somangshu code where the library is replaced is majorly widgets related. Could you please assign a reviewer from widget team for this PR. |
|
@rishabhrathod01 this is a very large change and hence risky, its not even a GFI. I suggest we dont take it forward. cc @rajatagrawal what do you think? |
|
@somangshu @rishabhrathod01 @rajatagrawal Please excuse me jumping in between. If you see the changes is only one replace cloneDeep with klona. both the functions do same thing i.e. create deep clone. if I ask chatgpt, it also says klona is a drop in replacement for cloneDeep, and we did it over here. if klona is better and it is a drop-in replacement for cloneDeep, what risk do you see? |
|
ChatGPT does not understand the context of appsmith repo. It does not know the impact that the library change might have. AFAIK, we have tried this before and did not succeed at completing it. There were test failures noticed. |
Yes, i agree. The result from klona and clone of lodash has differences for date objects, regex values and more that we discovered in past. We need to be careful before making such change. |
1 similar comment
Yes, i agree. The result from klona and clone of lodash has differences for date objects, regex values and more that we discovered in past. We need to be careful before making such change. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
Fixes #12936
or
Issue URL
[Issue URL]( https://github.com/appsmithorg/appsmith/issues/12936)Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
lodash.cloneDeepwithklonafor deep cloning objects across various components and utilities.These changes enhance object copying mechanisms, potentially reducing memory usage and improving application speed without altering the existing functionality from the end-user perspective.