chore: constrain the diff on only affected JS actions #33435
Conversation
WalkthroughWalkthroughThe changes optimize the evaluation process for JavaScript (JS) collections by introducing mechanisms to handle only the affected JS objects during evaluations, rather than evaluating the entire list. This involves creating constants, functions, and parameters to identify and process affected JS objects, thereby improving performance. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant Saga
participant Evaluator
Client->>Saga: Dispatch JS Action
Saga->>Evaluator: Extract affected JS objects
Evaluator->>Saga: Return affected JS objects
Saga->>Client: Perform constrained diff and update
Assessment against linked issues
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review due to trivial changes (1)
Additional Context UsedBiome (3)
Additional comments not posted (4)
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: 2
Out of diff range and nitpick comments (1)
app/client/src/workers/common/DataTreeEvaluator/index.ts (1)
478-493: Ensure consistent naming conventions.The method name
getOnlyAffectJsWidgetsshould begetOnlyAffectedJsWidgetsto maintain consistency and clarity.
| @@ -484,6 +500,7 @@ export default class DataTreeEvaluator { | |||
| unEvalTree: any, | |||
| configTree: ConfigTree, | |||
| webworkerTelemetry: Record<string, WebworkerSpanData> = {}, | |||
| onlyAffectedJSWidgets: string[] | undefined = undefined, | |||
There was a problem hiding this comment.
Consider adding a JSDoc comment for the new parameter.
Add a JSDoc comment for the onlyAffectedJSWidgets parameter to explain its purpose and usage.
/**
* @param onlyAffectedJSWidgets - List of affected JS widgets to limit the scope of updates.
*/| microDiff( | ||
| this.getOnlyAffectJsWidgets( | ||
| oldUnEvalTreeJSCollections, | ||
| onlyAffectedJSWidgets, | ||
| ), | ||
| this.getOnlyAffectJsWidgets( | ||
| localUnEvalTreeJSCollection, | ||
| onlyAffectedJSWidgets, | ||
| ), | ||
| ) || [], |
There was a problem hiding this comment.
Ensure proper error handling.
The getOnlyAffectJsWidgets method calls should include error handling to manage potential issues with undefined or malformed inputs.
const oldUnEvalTreeJSCollections = this.getOnlyAffectJsWidgets(
oldUnEvalTreeJSCollections,
onlyAffectedJSWidgets,
);
const localUnEvalTreeJSCollection = this.getOnlyAffectJsWidgets(
localUnEvalTreeJSCollection,
onlyAffectedJSWidgets,
);Consider wrapping these calls in a try-catch block or adding validation checks.
vsvamsi1
changed the title
github-actions
bot
added
the
skip-changelog
vsvamsi1
force-pushed
the
constrainedSetup
branch
from
a6f351a
to
623dccc
Compare
vsvamsi1
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9107533828. |
|
Deploy-Preview-URL: https://ce-33435.dp.appsmith.com |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9109020926. |
|
/ci-test-limit |
vsvamsi1
added
ok-to-test
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9113153223. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9113153223. |
vsvamsi1
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
app/client/cypress/limited-tests.txt (1)
Line range hint
6-6: Add the missing article "the".- #ci-test-limit uses this file to run minimum of specs. Do not run entire suite with this command. + #ci-test-limit uses this file to run the minimum of specs. Do not run the entire suite with this command.
| export interface BufferedReduxAction<T> { | ||
| affectedJSActionIds?: string[]; | ||
| type: ReduxActionType | ReduxActionErrorType; | ||
| payload: T; | ||
| } |
There was a problem hiding this comment.
Consider adding a comment to explain the purpose of the affectedJSActionIds property in the BufferedReduxAction interface.
+ // Optional array of affected JS action IDs for tracking purposes
affectedJSActionIds?: string[];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.
| export interface BufferedReduxAction<T> { | |
| affectedJSActionIds?: string[]; | |
| type: ReduxActionType | ReduxActionErrorType; | |
| payload: T; | |
| } | |
| export interface BufferedReduxAction<T> { | |
| // Optional array of affected JS action IDs for tracking purposes | |
| affectedJSActionIds?: string[]; | |
| type: ReduxActionType | ReduxActionErrorType; | |
| payload: T; | |
| } |
vsvamsi1
force-pushed
the
constrainedSetup
branch
2 times, most recently
from
ffcb97e
to
910d78d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
app/client/src/workers/common/DataTreeEvaluator/utils.test.ts (1)
1-3: Ensure consistent import grouping and ordering.Consider grouping and ordering imports by their source or relevance to improve readability and maintainability.
app/client/src/sagas/EvaluationSaga.utils.ts (1)
Line range hint
491-505: Optimize the merging ofAffectedJSObjects.The function
mergeJSBufferedActionscan be optimized by directly returning the new object whenisAllAffectedistruefor either argument. This avoids unnecessary array operations:- if (prevAffectedJSAction.isAllAffected || newAffectedJSAction.isAllAffected) { - return { - isAllAffected: true, - ids: [], - }; - } + return (prevAffectedJSAction.isAllAffected || newAffectedJSAction.isAllAffected) ? { isAllAffected: true, ids: [] } : { + isAllAffected: false, + ids: [...prevAffectedJSAction.ids, ...newAffectedJSAction.ids], + };app/client/src/sagas/EvaluationsSaga.test.ts (1)
Line range hint
1-16: Ensure consistent import grouping and ordering.Consider grouping and ordering imports by their source or relevance to improve readability and maintainability.
| import type { AffectedJSObjects } from "./EvaluationSaga.utils"; | ||
| import { | ||
| getAffectedJSObjectIdsFromAction, | ||
| getAffectedJSObjectIdsFromJSAction, | ||
| parseUpdatesAndDeleteUndefinedUpdates, | ||
| } from "./EvaluationSaga.utils"; |
There was a problem hiding this comment.
Use specific types instead of any.
The use of any type for affectedJSObjects in the import statement reduces type safety. It would be beneficial to define a more specific type structure for affectedJSObjects to ensure better type checking and to avoid potential runtime errors.
brayn003
added
ok-to-test
vsvamsi1
force-pushed
the
constrainedSetup
branch
from
910d78d
to
df45894
Compare
vsvamsi1
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range and nitpick comments (12)
app/client/src/workers/common/DataTreeEvaluator/utils.ts (2)
Line range hint
52-52: Remove unnecessarycontinuestatement.- if (!isJSAction(entity)) continue; + if (!isJSAction(entity)) return;The
continuestatement is unnecessary here since it's already the last statement in the loop's current iteration.
Line range hint
79-79: Specify a more accurate type instead ofany.- entity: DataTreeEntity, + entity: SpecificEntityType,Using
anycan lead to potential type safety issues. It's better to specify a more accurate type.app/client/src/sagas/EvaluationsSaga.ts (1)
Line range hint
255-288: Consider refining the default parameter placement inevaluateTreeSaga.The default parameter
affectedJSObjectsshould ideally follow the last required parameter or be made a required parameter to avoid confusion and potential errors in function calls.- affectedJSObjects: AffectedJSObjects = defaultAffectedJSObjects, + affectedJSObjects: AffectedJSObjects,app/client/src/workers/common/DataTreeEvaluator/index.ts (5)
115-119: Consider adding a JSDoc comment for the new imports to explain their purpose and usage./** * @module utils * - getFixedTimeDifference: Calculates the fixed time difference between two timestamps. * - getOnlyAffectedJSObjects: Filters JSActionEntities based on affected action IDs. * - replaceThisDotParams: Replaces 'this.' with 'params.' in JS object methods. */
Line range hint
467-480: ReplaceforEachwithfor...offor better performance and readability.- Object.keys(unEvalJSCollection).forEach((update) => { + for (const update of Object.keys(unEvalJSCollection)) { const updates = unEvalJSCollection[update]; if (!!unevalTree[update]) { - Object.keys(updates).forEach((key) => { + for (const key of Object.keys(updates)) { const data = get(unevalTree, `${update}.${key}.data`, undefined); if (isJSObjectFunction(unevalTree, update, key, configTree)) { set(unevalTree, `${update}.${key}`, new String(updates[key])); set(unevalTree, `${update}.${key}.data`, data); } else { set(unevalTree, `${update}.${key}`, updates[key]); } } } }Also applies to: 470-478
Line range hint
783-785: ReplaceforEachwithfor...ofin multiple locations for consistency and performance.- differences.forEach((diff) => { + for (const diff of differences) { if (!Array.isArray(diff.path) || diff.path.length === 0) continue; // Null check for typescript // Apply the changes into the evalTree so that it gets the latest changes applyChange(this.evalTree, undefined, diff); }Also applies to: 930-936, 945-951, 969-976, 985-993
Line range hint
3-3: Avoid shadowing the globalEvalErrorproperty. Consider renaming the imported or locally declaredEvalErrorto avoid confusion and potential bugs.import type { DataTreeEvaluationProps, EvaluationError as EvalError, // Renamed to avoid shadowing the global EvalError } from "utils/DynamicBindingUtils";
Line range hint
1306-1342: This else clause can be omitted because previous branches break early, simplifying the control flow.- if (condition) { - // code - break; - } else { - // other code - } + if (condition) { + // code + break; + } + // other codeapp/client/src/ce/constants/ReduxActionConstants.tsx (4)
19-19: Add a brief comment describing the purpose ofAffectedJSObjects.Adding a comment for the
AffectedJSObjectsimport will help maintainers understand its usage context quickly.
Line range hint
1196-1196: Specify a more appropriate type instead ofanyforreject.- reject: any; + reject: (reason?: any) => void;Using
anyis generally discouraged due to the lack of type safety. Specifying a function type forrejectenhances clarity and type safety.
Line range hint
1197-1197: Specify a more appropriate type instead ofanyforresolve.- resolve: any; + resolve: (value?: unknown) => void;Using
anyis generally discouraged due to the lack of type safety. Specifying a function type forresolveenhances clarity and type safety.
Line range hint
1289-1289: Specify a more appropriate type instead ofanyforapplications.- applications: any[]; + applications: ApplicationPayload[];Using
anyis generally discouraged due to the lack of type safety. SpecifyingApplicationPayload[]forapplicationsenhances clarity and type safety.
| import type { AffectedJSObjects } from "./EvaluationSaga.utils"; | ||
| import { | ||
| getAffectedJSObjectIdsFromAction, | ||
| parseUpdatesAndDeleteUndefinedUpdates, | ||
| } from "./EvaluationSaga.utils"; |
There was a problem hiding this comment.
Use specific types instead of any.
The use of any type for affectedJSObjects in the import statement reduces type safety. It would be beneficial to define a more specific type structure for affectedJSObjects to ensure better type checking and to avoid potential runtime errors.
| // extract the affected JS action ids from the action and pass them | ||
| // as a part of the buffered action | ||
| const affectedJSObjects = getAffectedJSObjectIdsFromAction(action); | ||
| collectedAffectedJSObjects = mergeJSBufferedActions( | ||
| collectedAffectedJSObjects, | ||
| affectedJSObjects, | ||
| ); |
There was a problem hiding this comment.
Clarify the handling of affectedJSObjects in buffered actions.
The comments and code in the put function of evalQueueBuffer could be enhanced for better clarity. Specifically, the comment about extracting affected JS action IDs could be more descriptive about how and why this is done:
- // extract the affected JS action ids from the action and pass them
- // as a part of the buffered action
+ // Extract affected JS action IDs from the action to ensure that only relevant JS actions are processed in the buffered action.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.
| // extract the affected JS action ids from the action and pass them | |
| // as a part of the buffered action | |
| const affectedJSObjects = getAffectedJSObjectIdsFromAction(action); | |
| collectedAffectedJSObjects = mergeJSBufferedActions( | |
| collectedAffectedJSObjects, | |
| affectedJSObjects, | |
| ); | |
| // Extract affected JS action IDs from the action to ensure that only relevant JS actions are processed in the buffered action. | |
| const affectedJSObjects = getAffectedJSObjectIdsFromAction(action); | |
| collectedAffectedJSObjects = mergeJSBufferedActions( | |
| collectedAffectedJSObjects, | |
| affectedJSObjects, | |
| ); |
| export interface BufferedReduxAction<T> extends ReduxAction<T> { | ||
| affectedJSObjects: AffectedJSObjects; | ||
| } |
There was a problem hiding this comment.
Define the BufferedReduxAction interface more clearly.
- export interface BufferedReduxAction<T> extends ReduxAction<T> {
- affectedJSObjects: AffectedJSObjects;
- }
+ /**
+ * Represents a Redux action that is buffered and includes information about affected JavaScript objects.
+ * @template T The type of the payload.
+ */
+ export interface BufferedReduxAction<T> extends ReduxAction<T> {
+ affectedJSObjects: AffectedJSObjects;
+ }Adding documentation comments will improve code readability and maintainability.
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.
| export interface BufferedReduxAction<T> extends ReduxAction<T> { | |
| affectedJSObjects: AffectedJSObjects; | |
| } | |
| /** | |
| * Represents a Redux action that is buffered and includes information about affected JavaScript objects. | |
| * @template T The type of the payload. | |
| */ | |
| export interface BufferedReduxAction<T> extends ReduxAction<T> { | |
| affectedJSObjects: AffectedJSObjects; | |
| } |
| export function getAffectedJSObjectIdsFromJSAction( | ||
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | ||
| ): AffectedJSObjects { | ||
| if (!JS_ACTIONS.includes(action.type)) { | ||
| return { | ||
| ids: [], | ||
| isAllAffected: false, | ||
| }; | ||
| } | ||
| // only JS actions here | ||
| action as ReduxAction<unknown>; | ||
| // When fetching JSActions fails, we need to diff all JSObjects because the reducer updates it | ||
| // to empty collection | ||
| if ( | ||
| action.type === ReduxActionErrorTypes.FETCH_JS_ACTIONS_ERROR || | ||
| action.type === ReduxActionErrorTypes.FETCH_JS_ACTIONS_VIEW_MODE_ERROR | ||
| ) { | ||
| return { | ||
| isAllAffected: true, | ||
| ids: [], | ||
| }; | ||
| } | ||
|
|
||
| const { payload } = action as ReduxAction<{ | ||
| data: JSCollection; | ||
| }> & | ||
| ReduxAction<JSCollection>; | ||
| // some actions have within data property of the action payload, we need to extract it from there | ||
| const innerData = payload?.data || payload; | ||
|
|
||
| const ids = Array.isArray(innerData) | ||
| ? innerData.map(({ id }) => id) | ||
| : [innerData.id]; | ||
|
|
||
| return { ids, isAllAffected: false }; | ||
| } |
There was a problem hiding this comment.
Ensure proper type annotations for better type safety.
- action: ReduxAction<unknown> | BufferedReduxAction<unknown>,
+ action: ReduxAction<anySpecificType> | BufferedReduxAction<anySpecificType>,Consider replacing unknown with more specific types to enhance type safety and code maintainability.
Committable suggestion was skipped due low confidence.
| function getAffectedJSObjectIdsFromBufferedAction( | ||
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | ||
| ): AffectedJSObjects { | ||
| if (action.type !== ReduxActionTypes.BUFFERED_ACTION) { | ||
| return { | ||
| ids: [], | ||
| isAllAffected: false, | ||
| }; | ||
| } | ||
| // only Buffered actions here | ||
| return ( | ||
| (action as BufferedReduxAction<unknown>).affectedJSObjects || { | ||
| ids: [], | ||
| isAllAffected: false, | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Refactor to avoid duplication and enhance clarity.
- if (action.type !== ReduxActionTypes.BUFFERED_ACTION) {
- return {
- ids: [],
- isAllAffected: false,
- };
- }
- // only Buffered actions here
- return (
- (action as BufferedReduxAction<unknown>).affectedJSObjects || {
- ids: [],
- isAllAffected: false,
- }
- );
+ return action.type === ReduxActionTypes.BUFFERED_ACTION
+ ? (action as BufferedReduxAction<unknown>).affectedJSObjects || { ids: [], isAllAffected: false }
+ : { ids: [], isAllAffected: false };This change reduces the complexity and increases the readability of the function.
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.
| function getAffectedJSObjectIdsFromBufferedAction( | |
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | |
| ): AffectedJSObjects { | |
| if (action.type !== ReduxActionTypes.BUFFERED_ACTION) { | |
| return { | |
| ids: [], | |
| isAllAffected: false, | |
| }; | |
| } | |
| // only Buffered actions here | |
| return ( | |
| (action as BufferedReduxAction<unknown>).affectedJSObjects || { | |
| ids: [], | |
| isAllAffected: false, | |
| } | |
| ); | |
| } | |
| function getAffectedJSObjectIdsFromBufferedAction( | |
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | |
| ): AffectedJSObjects { | |
| return action.type === ReduxActionTypes.BUFFERED_ACTION | |
| ? (action as BufferedReduxAction<unknown>).affectedJSObjects || { ids: [], isAllAffected: false } | |
| : { ids: [], isAllAffected: false }; | |
| } |
| @@ -16,6 +16,7 @@ | |||
| import type { DataTreeDiff } from "@appsmith/workers/Evaluation/evaluationUtils"; | |||
| import type { APP_MODE } from "entities/App"; | |||
| import type { WebworkerSpanData } from "UITelemetry/generateWebWorkerTraces"; | |||
| import type { AffectedJSObjects } from "sagas/EvaluationSaga.utils"; | |||
There was a problem hiding this comment.
Specify a more accurate type instead of any.
- export type EvalWorkerSyncRequest<T = any> = WorkerRequest<
+ export type EvalWorkerSyncRequest<T = SpecificType> = WorkerRequest<Using any can lead to potential type safety issues. It's better to specify a more accurate type.
Committable suggestion was skipped due low confidence.
| const mergeAffectedJSObjects = ( | ||
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | ||
| ) => { | ||
| return affectedJSObjectsFns.reduce( | ||
| (acc, affectedJSObjectsFn) => { | ||
| // when either of the action isAllJSObjectsAffected return true. | ||
| // In this case perform diff on all js objects | ||
| if (acc.isAllAffected) { | ||
| return acc; | ||
| } | ||
| acc = { | ||
| isAllAffected: | ||
| acc.isAllAffected || affectedJSObjectsFn(action).isAllAffected, | ||
| ids: [...acc.ids, ...affectedJSObjectsFn(action).ids], | ||
| }; | ||
|
|
||
| return acc; | ||
| }, | ||
| { ids: [], isAllAffected: false } as AffectedJSObjects, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Consider refactoring to avoid reassigning function parameters.
- acc = {
- isAllAffected:
- acc.isAllAffected || affectedJSObjectsFn(action).isAllAffected,
- ids: [...acc.ids, ...affectedJSObjectsFn(action).ids],
- };
+ const result = affectedJSObjectsFn(action);
+ return {
+ isAllAffected: acc.isAllAffected || result.isAllAffected,
+ ids: [...acc.ids, ...result.ids],
+ };Reassigning function parameters can lead to unexpected behavior and makes the code harder to understand. This refactor avoids direct mutation of the accumulator.
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.
| const mergeAffectedJSObjects = ( | |
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | |
| ) => { | |
| return affectedJSObjectsFns.reduce( | |
| (acc, affectedJSObjectsFn) => { | |
| // when either of the action isAllJSObjectsAffected return true. | |
| // In this case perform diff on all js objects | |
| if (acc.isAllAffected) { | |
| return acc; | |
| } | |
| acc = { | |
| isAllAffected: | |
| acc.isAllAffected || affectedJSObjectsFn(action).isAllAffected, | |
| ids: [...acc.ids, ...affectedJSObjectsFn(action).ids], | |
| }; | |
| return acc; | |
| }, | |
| { ids: [], isAllAffected: false } as AffectedJSObjects, | |
| ); | |
| }; | |
| const mergeAffectedJSObjects = ( | |
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | |
| ) => { | |
| return affectedJSObjectsFns.reduce( | |
| (acc, affectedJSObjectsFn) => { | |
| // when either of the action isAllJSObjectsAffected return true. | |
| // In this case perform diff on all js objects | |
| if (acc.isAllAffected) { | |
| return acc; | |
| } | |
| const result = affectedJSObjectsFn(action); | |
| return { | |
| isAllAffected: acc.isAllAffected || result.isAllAffected, | |
| ids: [...acc.ids, ...result.ids], | |
| }; | |
| }, | |
| { ids: [], isAllAffected: false } as AffectedJSObjects, | |
| ); | |
| }; |
vsvamsi1
force-pushed
the
constrainedSetup
branch
from
d2148c7
to
c6e3932
Compare
vsvamsi1
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (12)
app/client/src/sagas/EvaluationsSaga.ts (2)
Line range hint
255-288: Review the handling of default parameters in function signatures.The default parameter
defaultAffectedJSObjectsis used before it is defined in the file. This might lead to runtime errors if the function is called before the variable is initialized. Consider moving the definition ofdefaultAffectedJSObjectsbefore its usage or initializing it directly in the function signature.- affectedJSObjects: AffectedJSObjects = defaultAffectedJSObjects, + affectedJSObjects: AffectedJSObjects = { isAllAffected: false, ids: [] },
Line range hint
591-591: Review the placement of default parameters in function signatures.The default parameter
affectedJSObjectsis placed before required parameters in theevalAndLintingHandlerfunction. This is unconventional and could lead to confusion. Consider rearranging the parameters to follow the typical pattern of required parameters first, followed by optional ones.- function evalAndLintingHandler(isBlockingCall = true, action: ReduxAction<unknown>, options: Partial<{ shouldReplay: boolean; forceEvaluation: boolean; requiresLogging: boolean; affectedJSObjects: AffectedJSObjects; }>) { + function evalAndLintingHandler(isBlockingCall = true, action: ReduxAction<unknown>, affectedJSObjects: AffectedJSObjects, options: Partial<{ shouldReplay: boolean; forceEvaluation: boolean; requiresLogging: boolean; }>) {app/client/src/workers/common/DataTreeEvaluator/index.ts (7)
Line range hint
3-3: Avoid shadowing the globalEvalErrorproperty.Consider renaming the
EvalErrortype imported from "utils/DynamicBindingUtils" to avoid confusion with the globalEvalErrorobject in JavaScript.
Line range hint
179-179: Specify explicit types instead of usingany.Using
anycan lead to potential runtime errors due to lack of type safety. Please specify more explicit types for better type checking and code clarity.Also applies to: 233-233, 489-489, 733-733, 1047-1047, 1351-1351
Line range hint
467-480: Preferfor...ofloops overforEachfor better performance and readability.Using
for...ofloops can enhance performance in some cases and generally improves readability, especially when dealing with asynchronous operations or needing to break/continue.Also applies to: 470-478, 783-785, 930-936, 945-951, 969-976, 985-993
Line range hint
469-469: Avoid redundant double-negation.Simplify the condition by removing the double negation (
!!) which can lead to confusion and is generally unnecessary.
Line range hint
1000-1008: Ensure default parameters follow the last required parameter.Default parameters should be listed after all required parameters in function/method signatures to avoid confusion and potential errors in function calls.
Line range hint
1061-1061: Avoid implicitanytypes.Implicit
anytypes can lead to potential runtime errors and lack of clarity in the codebase. Please specify explicit types.
Line range hint
1306-1342: Remove unnecessaryelseclauses.Since the preceding branches of the conditional statements end with
breakorcontinue, theelseclauses are redundant and can be omitted for cleaner and more readable code.Also applies to: 1417-1419
app/client/src/ce/constants/ReduxActionConstants.tsx (3)
Line range hint
1196-1196: Consider specifying a more appropriate type instead ofanyfor therejectfield inPromisePayload.- reject: any; + reject: (reason?: any) => void;
Line range hint
1197-1197: Consider specifying a more appropriate type instead ofanyfor theresolvefield inPromisePayload.- resolve: any; + resolve: (value?: unknown) => void;
Line range hint
1289-1289: Consider specifying a more appropriate type instead ofanyfor theapplicationsfield inWorkspaceDetails.- applications: any[]; + applications: ApplicationPayload[];
| function mergeJSBufferedActions( | ||
| prevAffectedJSAction: AffectedJSObjects, | ||
| newAffectedJSAction: AffectedJSObjects, | ||
| ) { | ||
| if (prevAffectedJSAction.isAllAffected || newAffectedJSAction.isAllAffected) { | ||
| return { | ||
| isAllAffected: true, | ||
| ids: [], | ||
| }; | ||
| } | ||
| return { | ||
| isAllAffected: false, | ||
| ids: [...prevAffectedJSAction.ids, ...newAffectedJSAction.ids], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Optimize the merging logic for affected JS objects.
The function mergeJSBufferedActions can be optimized by directly returning the new object when isAllAffected is true for either argument. This avoids unnecessary array operations and simplifies the logic.
- if (prevAffectedJSAction.isAllAffected || newAffectedJSAction.isAllAffected) {
- return {
- isAllAffected: true,
- ids: [],
- };
- }
+ return (prevAffectedJSAction.isAllAffected || newAffectedJSAction.isAllAffected) ? { isAllAffected: true, ids: [] } : {
+ isAllAffected: false,
+ ids: [...prevAffectedJSAction.ids, ...newAffectedJSAction.ids],
+ };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.
| function mergeJSBufferedActions( | |
| prevAffectedJSAction: AffectedJSObjects, | |
| newAffectedJSAction: AffectedJSObjects, | |
| ) { | |
| if (prevAffectedJSAction.isAllAffected || newAffectedJSAction.isAllAffected) { | |
| return { | |
| isAllAffected: true, | |
| ids: [], | |
| }; | |
| } | |
| return { | |
| isAllAffected: false, | |
| ids: [...prevAffectedJSAction.ids, ...newAffectedJSAction.ids], | |
| }; | |
| } | |
| function mergeJSBufferedActions( | |
| prevAffectedJSAction: AffectedJSObjects, | |
| newAffectedJSAction: AffectedJSObjects, | |
| ) { | |
| return (prevAffectedJSAction.isAllAffected || newAffectedJSAction.isAllAffected) ? { isAllAffected: true, ids: [] } : { | |
| isAllAffected: false, | |
| ids: [...prevAffectedJSAction.ids, ...newAffectedJSAction.ids], | |
| }; | |
| } |
| const mergeAffectedJSObjects = ( | ||
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | ||
| ) => { | ||
| return affectedJSObjectsFns.reduce( | ||
| (acc, affectedJSObjectsFn) => { | ||
| // when either of the action isAllJSObjectsAffected return true. | ||
| // In this case perform diff on all js objects | ||
| if (acc.isAllAffected) { | ||
| return acc; | ||
| } | ||
| acc = { | ||
| isAllAffected: | ||
| acc.isAllAffected || affectedJSObjectsFn(action).isAllAffected, | ||
| ids: [...acc.ids, ...affectedJSObjectsFn(action).ids], | ||
| }; | ||
|
|
||
| return acc; | ||
| }, | ||
| { ids: [], isAllAffected: false } as AffectedJSObjects, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Refactor to improve clarity in mergeAffectedJSObjects.
- acc = {
+ let newAcc = {Using a separate variable instead of reassigning the accumulator inside the reduce function enhances clarity and maintainability.
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.
| const mergeAffectedJSObjects = ( | |
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | |
| ) => { | |
| return affectedJSObjectsFns.reduce( | |
| (acc, affectedJSObjectsFn) => { | |
| // when either of the action isAllJSObjectsAffected return true. | |
| // In this case perform diff on all js objects | |
| if (acc.isAllAffected) { | |
| return acc; | |
| } | |
| acc = { | |
| isAllAffected: | |
| acc.isAllAffected || affectedJSObjectsFn(action).isAllAffected, | |
| ids: [...acc.ids, ...affectedJSObjectsFn(action).ids], | |
| }; | |
| return acc; | |
| }, | |
| { ids: [], isAllAffected: false } as AffectedJSObjects, | |
| ); | |
| }; | |
| const mergeAffectedJSObjects = ( | |
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | |
| ) => { | |
| return affectedJSObjectsFns.reduce( | |
| (acc, affectedJSObjectsFn) => { | |
| // when either of the action isAllJSObjectsAffected return true. | |
| // In this case perform diff on all js objects | |
| if (acc.isAllAffected) { | |
| return acc; | |
| } | |
| let newAcc = { | |
| isAllAffected: | |
| acc.isAllAffected || affectedJSObjectsFn(action).isAllAffected, | |
| ids: [...acc.ids, ...affectedJSObjectsFn(action).ids], | |
| }; | |
| return newAcc; | |
| }, | |
| { ids: [], isAllAffected: false } as AffectedJSObjects, | |
| ); | |
| }; |
| } from "@appsmith/entities/DataTree/types"; | ||
| import { isObject } from "lodash"; | ||
| import type { AffectedJSObjects } from "sagas/EvaluationsSagaUtils"; |
There was a problem hiding this comment.
Consider using template literals for better readability in getFixedTimeDifference.
- return (endTime - startTime).toFixed(2) + " ms";
+ return `${(endTime - startTime).toFixed(2)} ms`;This change enhances readability by using template literals.
Committable suggestion was skipped due low confidence.
| test.each([ | ||
| [createJSCollectionSuccess, jsObject1, ["1234"]], | ||
| [deleteJSCollectionSuccess, jsObject1, ["1234"]], | ||
| [copyJSCollectionSuccess, jsObject1, ["1234"]], | ||
| [moveJSCollectionSuccess, jsObject1, ["1234"]], | ||
| [updateJSCollectionBodySuccess, { data: jsObject1 }, ["1234"]], | ||
| [fetchJSCollectionsForPageSuccess, jsCollection, ["1234", "5678"]], | ||
| ])( | ||
| "should return the correct affected JSObject ids for action %p with input %p and expected to be %p", | ||
| (action, input, expected) => { | ||
| const result = getAffectedJSObjectIdsFromAction( | ||
| action(input as JSCollection & JSCollection[] & { data: JSCollection }), | ||
| ); | ||
| expect(result).toEqual({ ids: expected, isAllAffected: false }); | ||
| }, | ||
| ); | ||
| test("should return isAllAffected to be true when there are JS errors", () => { | ||
| [ | ||
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_ERROR, | ||
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_VIEW_MODE_ERROR, | ||
| ].forEach((actionType) => { | ||
| const result = getAffectedJSObjectIdsFromAction({ | ||
| type: actionType, | ||
| } as ReduxAction<unknown>); | ||
| expect(result).toEqual({ isAllAffected: true, ids: [] }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Consider using for...of instead of forEach for better performance and readability in asynchronous contexts.
- [].forEach((actionType) => {
+ for (const actionType of []) {This change enhances readability and performance, especially in asynchronous contexts.
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.
| test.each([ | |
| [createJSCollectionSuccess, jsObject1, ["1234"]], | |
| [deleteJSCollectionSuccess, jsObject1, ["1234"]], | |
| [copyJSCollectionSuccess, jsObject1, ["1234"]], | |
| [moveJSCollectionSuccess, jsObject1, ["1234"]], | |
| [updateJSCollectionBodySuccess, { data: jsObject1 }, ["1234"]], | |
| [fetchJSCollectionsForPageSuccess, jsCollection, ["1234", "5678"]], | |
| ])( | |
| "should return the correct affected JSObject ids for action %p with input %p and expected to be %p", | |
| (action, input, expected) => { | |
| const result = getAffectedJSObjectIdsFromAction( | |
| action(input as JSCollection & JSCollection[] & { data: JSCollection }), | |
| ); | |
| expect(result).toEqual({ ids: expected, isAllAffected: false }); | |
| }, | |
| ); | |
| test("should return isAllAffected to be true when there are JS errors", () => { | |
| [ | |
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_ERROR, | |
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_VIEW_MODE_ERROR, | |
| ].forEach((actionType) => { | |
| const result = getAffectedJSObjectIdsFromAction({ | |
| type: actionType, | |
| } as ReduxAction<unknown>); | |
| expect(result).toEqual({ isAllAffected: true, ids: [] }); | |
| }); | |
| }); | |
| test.each([ | |
| [createJSCollectionSuccess, jsObject1, ["1234"]], | |
| [deleteJSCollectionSuccess, jsObject1, ["1234"]], | |
| [copyJSCollectionSuccess, jsObject1, ["1234"]], | |
| [moveJSCollectionSuccess, jsObject1, ["1234"]], | |
| [updateJSCollectionBodySuccess, { data: jsObject1 }, ["1234"]], | |
| [fetchJSCollectionsForPageSuccess, jsCollection, ["1234", "5678"]], | |
| ])( | |
| "should return the correct affected JSObject ids for action %p with input %p and expected to be %p", | |
| (action, input, expected) => { | |
| const result = getAffectedJSObjectIdsFromAction( | |
| action(input as JSCollection & JSCollection[] & { data: JSCollection }), | |
| ); | |
| expect(result).toEqual({ ids: expected, isAllAffected: false }); | |
| }, | |
| ); | |
| test("should return isAllAffected to be true when there are JS errors", () => { | |
| for (const actionType of [ | |
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_ERROR, | |
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_VIEW_MODE_ERROR, | |
| ]) { | |
| const result = getAffectedJSObjectIdsFromAction({ | |
| type: actionType, | |
| } as ReduxAction<unknown>); | |
| expect(result).toEqual({ isAllAffected: true, ids: [] }); | |
| } | |
| }); |
vsvamsi1
force-pushed
the
constrainedSetup
branch
from
3b530be
to
71e302b
Compare
vsvamsi1
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (25)
app/client/src/workers/common/DataTreeEvaluator/utils.ts (2)
Line range hint
52-52: Remove unnecessarycontinuestatement.- if (!isJSAction(entity)) continue; + if (!isJSAction(entity)) return;Replacing
continuewithreturnsimplifies the control flow and improves readability.
Line range hint
53-60: Simplify the conditional logic by removing the redundantelseclause.- } else { - const reacheableAsyncNodes = dependencyMap.getAllReachableNodes( - `${entityName}.${propertyName}`, - allAsyncNodes, - ); - reacheableAsyncNodes.length && - allAsyncNodes.push(`${entityName}.${propertyName}`); - }The
elseclause is unnecessary because the previous branch of theifstatement continues the loop, making this code unreachable in that case.app/client/src/sagas/EvaluationsSaga.test.ts (1)
Line range hint
30-30: Specify a more accurate type instead ofany.- buffer.put(fetchPluginFormConfigsSuccess({} as any)); + buffer.put(fetchPluginFormConfigsSuccess({} as SpecificType));Using
anycan lead to potential type safety issues. It's better to specify a more accurate type.app/client/src/workers/Evaluation/handlers/evalTree.ts (1)
Line range hint
5-5: Avoid shadowing the globalEvalErrorproperty.- import type { DependencyMap, EvalError } from "utils/DynamicBindingUtils"; + import type { DependencyMap, EvaluationError as EvalError } from "utils/DynamicBindingUtils";Renaming the imported
EvalErrortoEvaluationErroravoids confusion with the globalEvalErrorobject and enhances code clarity.app/client/src/workers/Evaluation/__tests__/generateOpimisedUpdates.test.ts (7)
Line range hint
313-317: Prefer usingfor...ofinstead offorEachfor better performance and readability.- parsedUpdates.forEach((v: any) => { + for (const v of parsedUpdates) { applyChange(draft, undefined, v); - }); + }
Line range hint
352-356: Consider replacingforEachwithfor...offor consistency and potential performance benefits.- action.payload.forEach((batchedAction) => { + for (const batchedAction of action.payload) { if (batchedAction.postEvalActions) { postEvalActions.push( ...(batchedAction.postEvalActions as AnyReduxAction[]), ); } - }); + }
Line range hint
357-358: Avoid using thedeleteoperator as it can lead to performance issues and hinder JavaScript engine optimizations.- delete draft.Table1.pageSize; - delete draft.Table1.__evaluation__.errors.transientTableData; + draft.Table1.pageSize = undefined; + draft.Table1.__evaluation__.errors.transientTableData = undefined;Also applies to: 358-361
Line range hint
405-409: ReplaceforEachwithfor...ofto enhance clarity and performance.- parsedUpdates.forEach((v: any) => { + for (const v of parsedUpdates) { applyChange(draft, undefined, v); - }); + }
Line range hint
410-411: Consider avoiding thedeleteoperator to maintain optimal performance.- delete draft.Table1.pageSize; - delete draft.Table1.__evaluation__.errors.transientTableData; + draft.Table1.pageSize = undefined; + draft.Table1.__evaluation__.errors.transientTableData = undefined;Also applies to: 411-414
Line range hint
458-462: Usefor...ofinstead offorEachfor iterating over arrays for better performance.- action.payload.forEach((batchedAction) => { + for (const batchedAction of action.payload) { if (batchedAction.postEvalActions) { postEvalActions.push( ...(batchedAction.postEvalActions as AnyReduxAction[]), ); } - }); + }
Line range hint
510-515: Preferfor...ofoverforEachto improve performance and readability in loops.- action.payload.forEach((batchedAction) => { + for (const batchedAction of action.payload) { if (batchedAction.postEvalActions) { postEvalActions.push( ...(batchedAction.postEvalActions as AnyReduxAction[]), ); } - }); + }app/client/src/sagas/EvaluationsSaga.ts (5)
Line range hint
29-29: Avoid shadowing the globalEvalErrorproperty. Consider renaming local instances to prevent potential conflicts.- import type { EvalError, EvaluationError } from "utils/DynamicBindingUtils"; + import type { EvaluationError } from "utils/DynamicBindingUtils"; + type AppsmithEvalError = EvalError; // Rename to avoid shadowing global `EvalError`
Line range hint
193-193: ReplaceforEachwithfor...offor better performance and readability in array iterations.- action.payload.forEach((batchedAction) => { + for (const batchedAction of action.payload) { if (batchedAction.postEvalActions) { postEvalActions.push( ...(batchedAction.postEvalActions as AnyReduxAction[]), ); } - }); + }
Line range hint
232-232: Use optional chaining to safely access nested properties.- if (action.payload && action.payload.shouldReplay) { + if (action.payload?.shouldReplay) {
Line range hint
579-585: Preferfor...ofoverforEachfor iterating over arrays to enhance performance and readability.- action.payload.forEach((batchedAction) => { + for (const batchedAction of action.payload) { if (batchedAction.postEvalActions) { postEvalActions.push( ...(batchedAction.postEvalActions as AnyReduxAction[]), ); } - }); + }
Line range hint
591-591: Ensure default parameters follow the last required parameter or are marked as required to maintain clarity in function signatures.- function* evalAndLintingHandler( - isBlockingCall = true, - action: ReduxAction<unknown>, - options: Partial<{ + function* evalAndLintingHandler( + action: ReduxAction<unknown>, + options: Partial<{ + isBlockingCall = true,app/client/src/workers/common/DataTreeEvaluator/index.ts (7)
Line range hint
492-600: Ensure proper error handling for thesetupUpdateTreemethod.try { // existing code here } catch (error) { // handle error appropriately }
Line range hint
467-480: ReplaceforEachwithfor...offor better performance and readability.for (const update of Object.keys(unEvalJSCollection)) { const updates = unEvalJSCollection[update]; if (unevalTree[update]) { for (const key of Object.keys(updates)) { const data = get(unevalTree, `${update}.${key}.data`, undefined); if (isJSObjectFunction(unevalTree, update, key, configTree)) { set(unevalTree, `${update}.${key}`, new String(updates[key])); set(unevalTree, `${update}.${key}.data`, data); } else { set(unevalTree, `${update}.${key}`, updates[key]); } } } }Also applies to: 469-469, 470-478
Line range hint
783-785: ReplaceforEachwithfor...ofin multiple locations for consistency and performance.// Example conversion for one of the forEach loops for (const diff of differences) { translateDiffEventToDataTreeDiffEvent(diff, localUnEvalTree); }Also applies to: 930-936, 945-951, 969-976, 985-993
Line range hint
1000-1008: Adjust the default parameteraffectedJSObjectsto follow the last required parameter.// Correct parameter order affectedJSObjects: AffectedJSObjects = { isAllAffected: false, ids: [] }, webworkerTelemetry: Record<string, WebworkerSpanData> = {},
Line range hint
1269-1274: Avoid unsafe usage of 'return' in loops.// Example fix if (condition) { return value; // Ensure that return is safe and intended }
Line range hint
1306-1342: Remove unnecessary else clause after early return.// Before if (condition) { return value; } else { // code } // After if (condition) { return value; } // code
Line range hint
1351-1351: Specify a more precise type thananyforconfigTree.// Suggested type change type ConfigTree = { // properties of ConfigTree };app/client/src/ce/constants/ReduxActionConstants.tsx (2)
19-19: Add documentation forAffectedJSObjectsimport.Consider adding a comment above the import statement to explain why
AffectedJSObjectsis needed here, especially since it's a new addition and might not be immediately clear to other developers.
Line range hint
1196-1196: Replaceanywith specific types to enhance type safety.Using
anytype can lead to potential runtime errors due to lack of type checking. Consider specifying more appropriate types or using generics if applicable.- applications: any[]; + applications: ApplicationPayload[]; - reject: any; - resolve: any; + reject: (reason?: any) => void; + resolve: (value: unknown) => void; - workspace: Workspace; + workspace: WorkspaceDetails;Also applies to: 1197-1197, 1289-1289
| microDiff( | ||
| getOnlyAffectedJSObjects( | ||
| oldUnEvalTreeJSCollections, | ||
| affectedJSObjects, | ||
| ), | ||
| getOnlyAffectedJSObjects( | ||
| localUnEvalTreeJSCollection, | ||
| affectedJSObjects, | ||
| ), | ||
| ) || [], |
There was a problem hiding this comment.
Optimize the use of microDiff and getOnlyAffectedJSObjects by directly passing the result of getOnlyAffectedJSObjects to microDiff.
const jsDifferences: Diff<
Record<string, JSActionEntity>,
Record<string, JSActionEntity>
>[] = profileFn(
"SetupUpdateTree.Diff1",
undefined,
webworkerTelemetry,
() => convertMicroDiffToDeepDiff(
microDiff(
getOnlyAffectedJSObjects(
oldUnEvalTreeJSCollections,
affectedJSObjects,
),
getOnlyAffectedJSObjects(
localUnEvalTreeJSCollection,
affectedJSObjects,
),
) || [],
),
);| @@ -41,6 +42,7 @@ export interface EvalTreeRequestData { | |||
| appMode?: APP_MODE; | |||
| widgetsMeta: Record<string, any>; | |||
| shouldRespondWithLogs?: boolean; | |||
| affectedJSObjects: AffectedJSObjects; | |||
There was a problem hiding this comment.
Ensure affectedJSObjects is marked as optional to align with TypeScript best practices.
- affectedJSObjects: AffectedJSObjects;
+ affectedJSObjects?: AffectedJSObjects;Marking affectedJSObjects as optional will enhance type safety, especially in cases where it might not be provided.
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.
| affectedJSObjects: AffectedJSObjects; | |
| affectedJSObjects?: AffectedJSObjects; |
| @@ -16,6 +16,7 @@ | |||
| import type { DataTreeDiff } from "@appsmith/workers/Evaluation/evaluationUtils"; | |||
| import type { APP_MODE } from "entities/App"; | |||
| import type { WebworkerSpanData } from "UITelemetry/generateWebWorkerTraces"; | |||
| import type { AffectedJSObjects } from "sagas/EvaluationsSagaUtils"; | |||
There was a problem hiding this comment.
Add missing import for AffectedJSObjects.
+ import type { AffectedJSObjects } from "sagas/EvaluationsSagaUtils";The AffectedJSObjects type is used in this file but not imported, which could lead to a compilation error.
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.
| import type { AffectedJSObjects } from "sagas/EvaluationsSagaUtils"; | |
| import type { AffectedJSObjects } from "sagas/EvaluationsSagaUtils"; |
|
|
||
| //delete all undefined properties from the state | ||
| const { deleteUpdates, regularUpdates } = parsedUpdates.reduce( | ||
| (acc: any, curr: any) => { |
There was a problem hiding this comment.
Specify more accurate types instead of any in the reducer function.
- (acc: any, curr: any) => {
+ (acc: DiffWithNewTreeState[], curr: DiffWithNewTreeState) => {Using more specific types instead of any enhances type safety and code readability.
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.
| (acc: any, curr: any) => { | |
| (acc: DiffWithNewTreeState[], curr: DiffWithNewTreeState) => { |
| @@ -484,6 +489,7 @@ | |||
| unEvalTree: any, | |||
| configTree: ConfigTree, | |||
| webworkerTelemetry: Record<string, WebworkerSpanData> = {}, | |||
| affectedJSObjects: AffectedJSObjects = { isAllAffected: false, ids: [] }, | |||
There was a problem hiding this comment.
Consider specifying a more precise type than any for affectedJSObjects.
// Suggested type change for affectedJSObjects
type AffectedJSObjects = {
isAllAffected: boolean;
ids: string[];
};| if (acc.isAllAffected) { | ||
| return acc; | ||
| } | ||
| acc = { |
There was a problem hiding this comment.
Avoid reassigning function parameters.
- acc = {
+ let newAcc = {Reassigning function parameters can lead to unexpected behavior and makes the code harder to understand. Use a new variable instead.
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.
| acc = { | |
| let newAcc = { |
| [ | ||
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_ERROR, | ||
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_VIEW_MODE_ERROR, | ||
| ].forEach((actionType) => { | ||
| const result = getAffectedJSObjectIdsFromAction({ | ||
| type: actionType, | ||
| } as ReduxAction<unknown>); | ||
| expect(result).toEqual({ isAllAffected: true, ids: [] }); | ||
| }); |
There was a problem hiding this comment.
Prefer for...of instead of forEach for better performance and readability.
- [].forEach((actionType) => {
+ for (const actionType of []) {Using for...of enhances readability and performance, especially in asynchronous contexts.
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.
| [ | |
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_ERROR, | |
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_VIEW_MODE_ERROR, | |
| ].forEach((actionType) => { | |
| const result = getAffectedJSObjectIdsFromAction({ | |
| type: actionType, | |
| } as ReduxAction<unknown>); | |
| expect(result).toEqual({ isAllAffected: true, ids: [] }); | |
| }); | |
| [ | |
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_ERROR, | |
| ReduxActionErrorTypes.FETCH_JS_ACTIONS_VIEW_MODE_ERROR, | |
| ].forEach((actionType) => { | |
| + for (const actionType of [ | |
| + ReduxActionErrorTypes.FETCH_JS_ACTIONS_ERROR, | |
| + ReduxActionErrorTypes.FETCH_JS_ACTIONS_VIEW_MODE_ERROR, | |
| + ]) { | |
| const result = getAffectedJSObjectIdsFromAction({ | |
| type: actionType, | |
| } as ReduxAction<unknown>); | |
| expect(result).toEqual({ isAllAffected: true, ids: [] }); | |
| }); |
| const mergeAffectedJSObjects = ( | ||
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | ||
| ) => { | ||
| return AFFECTED_JS_OBJECTS_FNS.reduce( | ||
| (acc, affectedJSObjectsFn) => { | ||
| // when either of the action isAllJSObjectsAffected return true. | ||
| // In this case perform diff on all js objects | ||
| if (acc.isAllAffected) { | ||
| return acc; | ||
| } | ||
| acc = { | ||
| isAllAffected: | ||
| acc.isAllAffected || affectedJSObjectsFn(action).isAllAffected, | ||
| ids: [...acc.ids, ...affectedJSObjectsFn(action).ids], | ||
| }; | ||
|
|
||
| return acc; | ||
| }, | ||
| { ids: [], isAllAffected: false } as AffectedJSObjects, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Avoid reassigning function parameters in mergeAffectedJSObjects to enhance clarity and maintainability.
- acc = {
+ let newAcc = {Using a separate variable instead of reassigning the accumulator inside the reduce function enhances clarity and maintainability.
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.
| const mergeAffectedJSObjects = ( | |
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | |
| ) => { | |
| return AFFECTED_JS_OBJECTS_FNS.reduce( | |
| (acc, affectedJSObjectsFn) => { | |
| // when either of the action isAllJSObjectsAffected return true. | |
| // In this case perform diff on all js objects | |
| if (acc.isAllAffected) { | |
| return acc; | |
| } | |
| acc = { | |
| isAllAffected: | |
| acc.isAllAffected || affectedJSObjectsFn(action).isAllAffected, | |
| ids: [...acc.ids, ...affectedJSObjectsFn(action).ids], | |
| }; | |
| return acc; | |
| }, | |
| { ids: [], isAllAffected: false } as AffectedJSObjects, | |
| ); | |
| }; | |
| const mergeAffectedJSObjects = ( | |
| action: ReduxAction<unknown> | BufferedReduxAction<unknown>, | |
| ) => { | |
| return AFFECTED_JS_OBJECTS_FNS.reduce( | |
| (acc, affectedJSObjectsFn) => { | |
| // when either of the action isAllJSObjectsAffected return true. | |
| // In this case perform diff on all js objects | |
| if (acc.isAllAffected) { | |
| return acc; | |
| } | |
| let newAcc = { | |
| isAllAffected: | |
| acc.isAllAffected || affectedJSObjectsFn(action).isAllAffected, | |
| ids: [...acc.ids, ...affectedJSObjectsFn(action).ids], | |
| }; | |
| return newAcc; | |
| }, | |
| { ids: [], isAllAffected: false } as AffectedJSObjects, | |
| ); | |
| }; |
| export const parseUpdatesAndDeleteUndefinedUpdates = ( | ||
| updates: string, | ||
| ): DiffWithNewTreeState[] => { | ||
| let parsedUpdates = []; | ||
| try { | ||
| //Parse updates from a string | ||
| parsedUpdates = JSON.parse(updates); | ||
| } catch (e) { | ||
| log.error("Failed to parse updates", e, updates); | ||
| return []; | ||
| } | ||
|
|
||
| //delete all undefined properties from the state | ||
| const { deleteUpdates, regularUpdates } = parsedUpdates.reduce( | ||
| (acc: any, curr: any) => { | ||
| const { kind, path, rhs } = curr; | ||
|
|
||
| if (rhs === undefined) { | ||
| //ignore any new undefined updates to the state if the value is undefined | ||
| if (kind === "N") { | ||
| return acc; | ||
| } | ||
| //convert undefined updates to delete updates | ||
| if (kind === "E") { | ||
| acc.deleteUpdates.push({ kind: "D", path }); | ||
| return acc; | ||
| } | ||
| } | ||
|
|
||
| acc.regularUpdates.push(curr); | ||
| return acc; | ||
| }, | ||
| { regularUpdates: [], deleteUpdates: [] }, | ||
| ); | ||
|
|
||
| const consolidatedUpdates = [...regularUpdates, ...deleteUpdates]; | ||
| return consolidatedUpdates; | ||
| }; |
There was a problem hiding this comment.
Consider specifying more accurate types instead of any in parseUpdatesAndDeleteUndefinedUpdates to enhance type safety.
- (acc: any, curr: any) => {
+ (acc: DiffWithNewTreeState[], curr: DiffWithNewTreeState) => {This change improves type safety by avoiding the use of any.
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.
| export const parseUpdatesAndDeleteUndefinedUpdates = ( | |
| updates: string, | |
| ): DiffWithNewTreeState[] => { | |
| let parsedUpdates = []; | |
| try { | |
| //Parse updates from a string | |
| parsedUpdates = JSON.parse(updates); | |
| } catch (e) { | |
| log.error("Failed to parse updates", e, updates); | |
| return []; | |
| } | |
| //delete all undefined properties from the state | |
| const { deleteUpdates, regularUpdates } = parsedUpdates.reduce( | |
| (acc: any, curr: any) => { | |
| const { kind, path, rhs } = curr; | |
| if (rhs === undefined) { | |
| //ignore any new undefined updates to the state if the value is undefined | |
| if (kind === "N") { | |
| return acc; | |
| } | |
| //convert undefined updates to delete updates | |
| if (kind === "E") { | |
| acc.deleteUpdates.push({ kind: "D", path }); | |
| return acc; | |
| } | |
| } | |
| acc.regularUpdates.push(curr); | |
| return acc; | |
| }, | |
| { regularUpdates: [], deleteUpdates: [] }, | |
| ); | |
| const consolidatedUpdates = [...regularUpdates, ...deleteUpdates]; | |
| return consolidatedUpdates; | |
| }; | |
| export const parseUpdatesAndDeleteUndefinedUpdates = ( | |
| updates: string, | |
| ): DiffWithNewTreeState[] => { | |
| let parsedUpdates = []; | |
| try { | |
| //Parse updates from a string | |
| parsedUpdates = JSON.parse(updates); | |
| } catch (e) { | |
| log.error("Failed to parse updates", e, updates); | |
| return []; | |
| } | |
| //delete all undefined properties from the state | |
| const { deleteUpdates, regularUpdates } = parsedUpdates.reduce( | |
| (acc: DiffWithNewTreeState[], curr: DiffWithNewTreeState) => { | |
| const { kind, path, rhs } = curr; | |
| if (rhs === undefined) { | |
| //ignore any new undefined updates to the state if the value is undefined | |
| if (kind === "N") { | |
| return acc; | |
| } | |
| //convert undefined updates to delete updates | |
| if (kind === "E") { | |
| acc.deleteUpdates.push({ kind: "D", path }); | |
| return acc; | |
| } | |
| } | |
| acc.regularUpdates.push(curr); | |
| return acc; | |
| }, | |
| { regularUpdates: [], deleteUpdates: [] }, | |
| ); | |
| const consolidatedUpdates = [...regularUpdates, ...deleteUpdates]; | |
| return consolidatedUpdates; | |
| }; |
vsvamsi1
added
ok-to-test
Description
Perform diff of only the affected JS collections not the entire list of JS collections and for non JS updates evaluation we should not perform a diff on any JS collection.. This should improve the p100 of each evaluation.
Fixes #33508
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="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9248167182
Commit: e178485
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?