chore: decouple editor components #37102
Conversation
WalkthroughThe changes in this pull request primarily focus on implementing lazy loading for various components across multiple files using React's Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![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
🧹 Outside diff range and nitpick comments (7)
app/client/src/pages/Editor/QueryEditor/Form.tsx (1)
54-59: Consider adding Suspense boundary for better error handlingThe lazy loading implementation with retry mechanism is solid. However, to handle loading states and errors gracefully, consider wrapping the component with a Suspense boundary in the parent component.
Example implementation:
import { Suspense } from 'react'; // In parent component <Suspense fallback={<LoadingSpinner />}> <EditorJSONtoForm {...props} /> </Suspense>app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.ts (3)
Line range hint
20-57: Consider memoizing filtered module options.The jsModuleCreationOptions filtering is performed on every render. Consider memoizing this value to optimize performance.
export const useJSAdd = () => { const pageId = useSelector(getCurrentPageId); const dispatch = useDispatch(); const currentEntityInfo = identifyEntityFromPath(location.pathname); const moduleCreationOptions = useModuleOptions(); - const jsModuleCreationOptions = moduleCreationOptions.filter( - (opt) => opt.focusEntityType === FocusEntity.JS_MODULE_INSTANCE, - ); + const jsModuleCreationOptions = useMemo( + () => moduleCreationOptions.filter( + (opt) => opt.focusEntityType === FocusEntity.JS_MODULE_INSTANCE, + ), + [moduleCreationOptions] + );
Line range hint
59-73: Extract common filtering logic.This hook duplicates the module filtering logic from useJSAdd. Consider extracting it into a shared hook.
+const useJSModuleOptions = () => { + const moduleCreationOptions = useModuleOptions(); + return useMemo( + () => moduleCreationOptions.filter( + (opt) => opt.focusEntityType === FocusEntity.JS_MODULE_INSTANCE, + ), + [moduleCreationOptions] + ); +}; export const useIsJSAddLoading = () => { - const moduleCreationOptions = useModuleOptions(); - const jsModuleCreationOptions = moduleCreationOptions.filter( - (opt) => opt.focusEntityType === FocusEntity.JS_MODULE_INSTANCE, - ); + const jsModuleCreationOptions = useJSModuleOptions(); const { isCreating } = useSelector((state) => state.ui.jsPane);
Line range hint
75-91: Move static configuration outside component.Since this configuration is static, it can be moved outside the component to prevent unnecessary recreations.
+const JS_OPERATIONS: GroupedAddOperations = [ + { + className: "t--blank-js", + operations: [ + { + title: createMessage(EDITOR_PANE_TEXTS.js_blank_object_item), + desc: "", + icon: JsFileIconV2(16, 16), + kind: SEARCH_ITEM_TYPES.actionOperation, + action: (pageId) => createNewJSCollection(pageId, "ENTITY_EXPLORER"), + }, + ], + }, +]; export const useGroupedAddJsOperations = (): GroupedAddOperations => { - return [ - { - className: "t--blank-js", - operations: [ - { - title: createMessage(EDITOR_PANE_TEXTS.js_blank_object_item), - desc: "", - icon: JsFileIconV2(16, 16), - kind: SEARCH_ITEM_TYPES.actionOperation, - action: (pageId) => createNewJSCollection(pageId, "ENTITY_EXPLORER"), - }, - ], - }, - ]; + return JS_OPERATIONS; };app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (1)
Line range hint
118-185: Consider implementing error boundaries for lazy-loaded components.While
retryPromisehandles loading failures, it's recommended to implement React Error Boundaries to gracefully handle runtime errors in lazy-loaded components.Consider wrapping the routes with an error boundary component to handle potential chunk loading failures and runtime errors.
app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx (2)
Line range hint
187-391: Consider breaking down this component.The component handles multiple concerns (form rendering, tab management, debugging, etc.). Consider splitting it into smaller, focused components for better maintainability.
Line range hint
52-65: Fix TODO comments regarding TypeScript any types.Replace these TODO comments with proper type definitions:
location: { state: Record<string, unknown>; }, editorConfig: EditorConfig; settingConfig: SettingConfig;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- app/client/src/api/Api.ts (2 hunks)
- app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.ts (3 hunks)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (5 hunks)
- app/client/src/pages/Editor/JSEditor/JSBlankState.tsx (1 hunks)
- app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx (1 hunks)
- app/client/src/pages/Editor/QueryEditor/Form.tsx (2 hunks)
- app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx (1 hunks)
🔇 Additional comments (12)
app/client/src/pages/Editor/JSEditor/JSBlankState.tsx (2)
Line range hint
1-28: LGTM! Clean component implementation.The component follows best practices with proper layout structure and dynamic asset loading.
29-29: Verify all import statements are updated.The change from named export to default export requires updates to all import statements of this component.
✅ Verification successful
The previous regex had syntax issues. Let's search for both imports and usages of the component to verify if there are any files that need updates due to the export change.
No import updates required
The component
JSBlankStateis not imported or used anywhere in the codebase, making the change from named to default export safe with no required updates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining named imports of JSBlankState that need to be updated rg "import.*{.*JSBlankState.*}.*from.*JSBlankState"Length of output: 191
Script:
#!/bin/bash # Search for any imports of JSBlankState component rg "import.*JSBlankState.*from" -g "*.{ts,tsx,js,jsx}" # Also search for any usage of JSBlankState component rg "<JSBlankState" -g "*.{ts,tsx,js,jsx}"Length of output: 100
app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx (2)
Line range hint
1-28: LGTM! Clean component implementation.The component follows React best practices with proper lazy loading of assets and clear component structure.
29-29: Verify import statements are updated for this component.The change from named to default export aligns with the decoupling objective.
✅ Verification successful
The previous search was too specific. Let's broaden it to find any imports or usage of the component, regardless of the import style.
Export change is safe to proceed
The component is only imported once in
hooks.tsxusing dynamic import, which is compatible with both named and default exports. No breaking changes detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining named imports of QueriesBlankState that need updating rg "import.*\{.*QueriesBlankState.*\}.*from.*QueriesBlankState"Length of output: 65
Script:
#!/bin/bash # Search for any imports of QueriesBlankState using ripgrep with more context rg -A 2 "import.*QueriesBlankState.*from" # Also search for any usage of QueriesBlankState component rg "QueriesBlankState"Length of output: 416
app/client/src/pages/Editor/QueryEditor/Form.tsx (2)
15-16: LGTM: Clean import additionsThe new imports support the lazy loading implementation appropriately.
54-59: Verify all EditorJSONtoForm imports are updatedLet's ensure all imports of EditorJSONtoForm are compatible with the new lazy loading implementation.
✅ Verification successful
All EditorJSONtoForm imports are correctly implemented
The lazy loading change is compatible with all current usages:
QueryEditorHeader.tsxonly imports a styled componentStyledFormRowApiFormatSegmentedResponse.tsxonly imports a styled componentSegmentedControlContainerForm.tsxcontains the lazy loading implementation itself🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct imports of EditorJSONtoForm that might need updating rg -l "import.*EditorJSONtoForm.*from" --type ts --type tsxLength of output: 89
Script:
#!/bin/bash # Search for direct imports of EditorJSONtoForm using ripgrep with correct file type rg -l "import.*EditorJSONtoForm" --type-add 'tsx:*.tsx' --type tsx --type ts # Also search for any usage of EditorJSONtoForm component rg "EditorJSONtoForm" --type-add 'tsx:*.tsx' --type tsx --type ts # Check the actual file content to understand the context cat "app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx" 2>/dev/null || echo "File not found"Length of output: 13799
app/client/src/api/Api.ts (1)
10-10: LGTM!Clean import statement for the telemetry function.
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.ts (2)
1-1: LGTM! Clean import organization.The imports are well-structured and the addition of
retryPromiseis appropriate for the lazy loading implementation.Also applies to: 18-18
Line range hint
93-134: Well-implemented lazy loading with proper error handling.The implementation follows best practices for code splitting and includes error handling via retryPromise.
Let's verify the consistency of chunk naming across the codebase:
✅ Verification successful
Chunk naming convention is consistent and properly implemented
The webpack chunk names follow a clear pattern for JS-related modules, with descriptive names like "AddJS", "JSEditor", and "JSEmpty". The additional chunk "EditorJSONtoForm" found in the query editor is appropriately named for its context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for webpack chunk names to ensure consistency rg "webpackChunkName.*JS" --type tsLength of output: 587
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (2)
1-1: LGTM: Import changes for lazy loading implementation.The addition of
lazyfrom React andretryPromiseutility is appropriate for implementing component lazy loading.Also applies to: 35-35
118-125: 🛠️ Refactor suggestionVerify chunk naming consistency across lazy-loaded components.
The implementation of lazy loading with retry logic is good, but there's an inconsistency in chunk naming:
APIEditorandQueryEditorchunks might conflict as they point to different componentsQueryEditorchunk name is used for both SAASEditor and QueryEditor componentsLet's verify the webpack chunk names:
Consider using unique chunk names for each component:
- /* webpackChunkName: "QueryEditor" */ "pages/Editor/QueryEditor" + /* webpackChunkName: "SAASEditor" */ "pages/Editor/QueryEditor"- /* webpackChunkName: "QueryEmpty" */ "pages/Editor/QueryEditor/QueriesBlankState" + /* webpackChunkName: "QueriesBlankState" */ "pages/Editor/QueryEditor/QueriesBlankState"Also applies to: 136-143, 148-155, 165-172, 178-185
app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx (1)
187-187: LGTM! Verify import statements are updated.The change to default export aligns with the decoupling objective and enables lazy loading.
✅ Verification successful
✅ Export change is correctly implemented
The component is properly imported using dynamic import with lazy loading, and the only reference is through type import. The change to default export aligns with the codebase's architecture.
- Type import in Form.tsx:
import type { EditorJSONtoFormProps } from "./EditorJSONtoForm"- Dynamic import:
lazy(() => retryPromise(() => import("./EditorJSONtoForm")))🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining named imports of EditorJSONtoForm rg "import.*\{.*EditorJSONtoForm.*\}.*from.*QueryEditor/EditorJSONtoForm"Length of output: 75
Script:
#!/bin/bash # Search for any imports of EditorJSONtoForm with broader pattern rg -A 2 "import.*EditorJSONtoForm.*from" # Also search for the file name to catch potential dynamic imports rg "EditorJSONtoForm\.tsx"Length of output: 494
Script:
#!/bin/bash # Check Form.tsx to see how EditorJSONtoForm is used cat app/client/src/pages/Editor/QueryEditor/Form.tsxLength of output: 2387
app/client/src/api/Api.ts
Outdated
| axiosInstance.defaults.transformResponse = [ | ||
| function (...args) { | ||
| const transformResponseAr = axios.defaults.transformResponse; | ||
|
|
||
| // Pick up the transformFn from axios defaults and wrap it in with telemetry code so that we can capture how long it takes parse an api response | ||
| if (Array.isArray(transformResponseAr) && transformResponseAr?.[0]) { | ||
| const transfromFn = transformResponseAr?.[0]; | ||
| const resp = startAndEndSpanForFn( | ||
| "axios.transformApiResponse", | ||
| { url: this.url }, | ||
| () => transfromFn.call(this, ...args), | ||
| ); | ||
|
|
||
| return resp; | ||
| } else { | ||
| // eslint-disable-next-line no-console | ||
| console.error("could not find the api transformerFn"); | ||
|
|
||
| // return the data as it is. | ||
| return args[0]; | ||
| } | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting the transform function
The transform function could be extracted for better readability and testability.
Consider this refactor:
const createTelemetryTransform = () => {
return function(this: { url?: string }, data: unknown, headers?: unknown) {
const transformResponseAr = axios.defaults.transformResponse;
if (!Array.isArray(transformResponseAr) || !transformResponseAr[0]) {
console.warn("[Api] Response transform function not found, skipping transformation");
return data;
}
return startAndEndSpanForFn(
"axios.transformApiResponse",
{ url: this?.url || 'unknown' },
() => transformResponseAr[0].call(this, data, headers),
);
};
};
axiosInstance.defaults.transformResponse = [createTelemetryTransform()];Consider improving type safety and error handling
The implementation could benefit from some improvements:
- The
thiscontext might be undefined when accessingthis.url - The error case silently continues execution
- Types could be more explicit
Consider applying these changes:
axiosInstance.defaults.transformResponse = [
- function (...args) {
+ function (this: { url?: string }, data: unknown, headers?: unknown) {
const transformResponseAr = axios.defaults.transformResponse;
// Pick up the transformFn from axios defaults and wrap it in with telemetry code
if (Array.isArray(transformResponseAr) && transformResponseAr?.[0]) {
const transfromFn = transformResponseAr?.[0];
const resp = startAndEndSpanForFn(
"axios.transformApiResponse",
- { url: this.url },
+ { url: this?.url || 'unknown' },
- () => transfromFn.call(this, ...args),
+ () => transfromFn.call(this, data, headers),
);
return resp;
} else {
- // eslint-disable-next-line no-console
- console.error("could not find the api transformerFn");
+ console.warn("[Api] Response transform function not found, skipping transformation");
// return the data as it is.
- return args[0];
+ return data;
}
},
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| axiosInstance.defaults.transformResponse = [ | |
| function (...args) { | |
| const transformResponseAr = axios.defaults.transformResponse; | |
| // Pick up the transformFn from axios defaults and wrap it in with telemetry code so that we can capture how long it takes parse an api response | |
| if (Array.isArray(transformResponseAr) && transformResponseAr?.[0]) { | |
| const transfromFn = transformResponseAr?.[0]; | |
| const resp = startAndEndSpanForFn( | |
| "axios.transformApiResponse", | |
| { url: this.url }, | |
| () => transfromFn.call(this, ...args), | |
| ); | |
| return resp; | |
| } else { | |
| // eslint-disable-next-line no-console | |
| console.error("could not find the api transformerFn"); | |
| // return the data as it is. | |
| return args[0]; | |
| } | |
| }, | |
| ]; | |
| axiosInstance.defaults.transformResponse = [ | |
| function (this: { url?: string }, data: unknown, headers?: unknown) { | |
| const transformResponseAr = axios.defaults.transformResponse; | |
| // Pick up the transformFn from axios defaults and wrap it in with telemetry code | |
| if (Array.isArray(transformResponseAr) && transformResponseAr?.[0]) { | |
| const transfromFn = transformResponseAr?.[0]; | |
| const resp = startAndEndSpanForFn( | |
| "axios.transformApiResponse", | |
| { url: this?.url || 'unknown' }, | |
| () => transfromFn.call(this, data, headers), | |
| ); | |
| return resp; | |
| } else { | |
| console.warn("[Api] Response transform function not found, skipping transformation"); | |
| // return the data as it is. | |
| return data; | |
| } | |
| }, | |
| ]; |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
app/client/src/pages/Editor/QueryEditor/Form.tsx (1)
54-59: Consider adding Suspense boundary and error handlingThe lazy loading implementation is solid, but consider wrapping the component usage with Suspense and ErrorBoundary components to handle loading states and errors gracefully.
Example implementation:
import { Suspense, ErrorBoundary } from 'react'; // Wrap the connected component with error handling const ConnectedEditorJSONtoForm = connect(mapStateToProps)( reduxForm<Action, EditorJSONtoFormProps>({ form: QUERY_EDITOR_FORM_NAME, enableReinitialize: true, })(EditorJSONtoForm), ); // Export with proper error handling export default function EditorJSONtoFormWrapper(props: EditorJSONtoFormProps) { return ( <ErrorBoundary fallback={<div>Error loading editor</div>}> <Suspense fallback={<div>Loading editor...</div>}> <ConnectedEditorJSONtoForm {...props} /> </Suspense> </ErrorBoundary> ); }app/client/src/api/Api.ts (1)
Line range hint
54-54: Consider addressing the TODO comments for type safetyMultiple TODO comments indicate the need to fix eslint-disable directives for explicit any types. Consider adding proper type definitions for these parameters.
Would you like me to propose type definitions for these parameters?
Also applies to: 67-67, 74-74, 89-89, 96-96, 111-111
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.ts (1)
Line range hint
64-95: Add JSDoc comments for the new hooks.The implementation looks good, but adding JSDoc comments would improve maintainability.
/** * Hook to manage loading state for JS module creation * @returns {boolean} True if a JS module is being created */ export const useIsJSAddLoading = () => { // ... existing implementation }; /** * Hook to provide grouped operations for JS creation * @returns {GroupedAddOperations} Array of grouped operations */ export const useGroupedAddJsOperations = (): GroupedAddOperations => { // ... existing implementation };app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (1)
Line range hint
118-185: Consider extracting the lazy loading pattern.The lazy loading implementation is correct, but there's repeated code structure across all component imports.
Consider creating a utility function to reduce repetition:
const lazyLoadComponent = (path: string, chunkName: string) => lazy(async () => retryPromise(async () => import(/* webpackChunkName: "[name]" */ `${path}`), ), ); // Usage in routes: { key: "ApiEditor", component: lazyLoadComponent( "pages/Editor/APIEditor", "APIEditor" ), // ... rest of the route config }app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx (2)
Line range hint
1-186: Consider improving type safety and reducing technical debt.The component contains several areas that could benefit from improved type safety:
- Props interface uses
anytypes with TODO comments- Complex nested type structures could be better defined
Consider addressing these type-related TODOs:
- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - location: { state: any }; + location: { + state: { + // Define specific state shape here + [key: string]: unknown; + } + };
Line range hint
187-365: Consider breaking down this large component.The component handles multiple concerns (query editing, settings, debugging) and could benefit from further decomposition to improve maintainability and reusability.
Consider:
- Extracting the form sections into separate components
- Moving the styled components to a separate file
- Creating a custom hook for the execution permission logic
🛑 Comments failed to post (2)
app/client/src/api/Api.ts (1)
23-45:
⚠️ Potential issueAdd type safety and improve error handling in transform response
Several improvements could make this more robust:
Apply these changes:
axiosInstance.defaults.transformResponse = [ - function (...args) { + function (this: { url?: string }, data: any, headers?: any) { const transformResponseAr = axios.defaults.transformResponse; if (Array.isArray(transformResponseAr) && transformResponseAr?.[0]) { const transfromFn = transformResponseAr?.[0]; const resp = startAndEndSpanForFn( "axios.transformApiResponse", - { url: this.url }, + { url: this?.url || 'unknown' }, - () => transfromFn.call(this, ...args), + () => transfromFn.call(this, data, headers), ); return resp; } else { // eslint-disable-next-line no-console - console.error("could not find the api transformerFn"); + console.error("Axios transform response: Default transformer not found, response may be malformed"); - // return the data as it is. - return args[0]; + // Attempt basic JSON parsing as fallback + try { + return typeof data === 'string' ? JSON.parse(data) : data; + } catch { + return data; + } } }, ];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.axiosInstance.defaults.transformResponse = [ function (this: { url?: string }, data: any, headers?: any) { const transformResponseAr = axios.defaults.transformResponse; // Pick up the transformFn from axios defaults and wrap it in with telemetry code so that we can capture how long it takes parse an api response if (Array.isArray(transformResponseAr) && transformResponseAr?.[0]) { const transfromFn = transformResponseAr?.[0]; const resp = startAndEndSpanForFn( "axios.transformApiResponse", { url: this?.url || 'unknown' }, () => transfromFn.call(this, data, headers), ); return resp; } else { // eslint-disable-next-line no-console console.error("Axios transform response: Default transformer not found, response may be malformed"); // Attempt basic JSON parsing as fallback try { return typeof data === 'string' ? JSON.parse(data) : data; } catch { return data; } } }, ];app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.ts (1)
99-106: 🛠️ Refactor suggestion
Consider preloading critical components.
The lazy loading implementation looks good. For better user experience, consider preloading this component when the user navigates to the JS editor section.
// Add this where JS editor navigation is initiated const preloadAddJS = () => { const AddJS = lazy(() => retryPromise(() => import(/* webpackChunkName: "AddJS" */ "pages/Editor/IDE/EditorPane/JS/Add") )); AddJS.preload?.(); };
vsvamsi1
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx (1)
146-153: Consider optimizing the waitFor configuration.
While the async handling is correct, consider these improvements:
- Use a data-testid instead of text content for more reliable selection
- Consider increasing the timeout for CI environments
- Reduce the polling interval for faster test execution
await waitFor(
async () => {
- const elements = getAllByText("JSObject1"); // Use the common test ID or selector
+ const elements = getAllByTestId("t--js-object-name");
expect(elements).toHaveLength(3);
},
- { timeout: 3000, interval: 500 },
+ { timeout: 5000, interval: 100 },
);📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (3 hunks)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (3 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx (7 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx (8 hunks)
- app/client/src/pages/Editor/JSEditor/JSBlankState.tsx (1 hunks)
- app/client/src/pages/Editor/JSEditor/index.tsx (0 hunks)
- app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx (1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/pages/Editor/JSEditor/index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx
- app/client/src/pages/Editor/JSEditor/JSBlankState.tsx
- app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx
🔇 Additional comments (10)
app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx (2)
2-2: LGTM: Appropriate test utility import.
The addition of waitFor is aligned with the async testing improvements.
Line range hint 22-34: LGTM: Proper async test handling.
The test correctly uses async/await with findByText for reliable component rendering assertions.
app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx (2)
15-15: LGTM: Required import for async testing.
The addition of waitFor from @testing-library/react is necessary for the async test improvements.
158-165: 🛠️ Refactor suggestion
Verify timeout values for API route tests.
The waitFor timeout of 3000ms might be insufficient for slower CI environments. Also, the interval of 500ms might be too long for quick renders.
Consider adjusting the values or using environment variables:
await waitFor(
async () => {
const elements = getAllByText("Api1");
expect(elements).toHaveLength(3);
},
- { timeout: 3000, interval: 500 },
+ { timeout: process.env.TEST_TIMEOUT || 5000, interval: 100 },
);✅ Verification successful
The previous command failed due to incorrect file type handling. Let's try again with proper file type flags and a broader search pattern to catch different variations of waitFor configurations in test files.
Let's search more specifically in test files and look for any existing patterns of environment variable usage for timeouts, as well as any default timeout configurations.
The timeout values are within acceptable ranges
The current timeout of 3000ms is reasonable given that:
- The global Jest testTimeout is set to 9000ms in multiple config files
- The interval of 500ms is appropriate for this specific test case as it's checking for API-related rendering
No changes needed. The values align well with the project's test configuration patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any similar waitFor patterns in other test files
rg -A 2 'waitFor\(' --type ts --type tsx | grep -E 'timeout:|interval:'
Length of output: 101
Script:
#!/bin/bash
# Search for waitFor patterns in test files, adjusting file type flags
rg -t js -t ts 'waitFor\(' -A 2 | grep -E 'timeout:|interval:'
# Also try a broader search to catch any test configuration patterns
rg -t js -t ts 'waitFor.*\{.*timeout' -A 2
Length of output: 457
Script:
#!/bin/bash
# Search specifically in test files with more context
rg -t js -t ts 'waitFor.*\{.*timeout' --glob '**/*.test.*' --glob '**/*.spec.*' -B 1 -A 2
# Look for any existing environment variable usage in test timeouts
rg -t js -t ts 'process\.env.*TEST.*TIMEOUT' --glob '**/*.test.*' --glob '**/*.spec.*'
# Search for jest configuration files that might define default timeouts
fd -t f "jest.config" -x cat {}
Length of output: 10475
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (6)
1-2: Approved: Necessary imports added for lazy loading
The addition of lazy, Suspense, useCallback, and useMemo from React is appropriate for implementing lazy loading and optimizing performance.
19-20: Approved: Imports for retryPromise and Skeleton
Importing retryPromise and Skeleton is correct and necessary for handling asynchronous component loading and providing fallback UI.
96-103: Approved: Lazy loading of AddJS component
The AddJS component is correctly set up for lazy loading using React.lazy and retryPromise, improving code splitting and performance.
104-109: Approved: Lazy loading of JSEditor component
The JSEditor component is properly configured for lazy loading, which enhances load times and user experience.
111-118: Approved: Lazy loading of JSEmpty component
The JSEmpty component is appropriately set up for lazy loading, consistent with the other components.
121-155: Approved: Routes updated with lazy-loaded components and Suspense
The routes now correctly use lazy-loaded components wrapped in Suspense with a Skeleton fallback, and useMemo is properly utilized for performance optimization.
| it("Renders Fullscreen Blank State", async () => { | ||
| const { findByText, getByRole, getByText } = render( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve async test stability with consistent patterns.
The async/await pattern is correctly implemented, but there's inconsistency in how assertions are handled. Some tests use findByText while others mix getByText with async functions.
Consider standardizing the async assertion pattern:
- const { findByText, getByRole, getByText } = render(
+ const { findByText, findByRole } = render(
// ...
- getByText(createMessage(EDITOR_PANE_TEXTS.query_blank_state_description));
+ await findByText(createMessage(EDITOR_PANE_TEXTS.query_blank_state_description));Also applies to: 39-39, 72-86
| await waitFor( | ||
| async () => { | ||
| const elements = getAllByText("Query1"); // Use the common test ID or selector | ||
|
|
||
| expect(elements).toHaveLength(3); // Wait until there are exactly 3 elements | ||
| }, | ||
| { timeout: 3000, interval: 500 }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate waitFor pattern detected.
The waitFor implementation for Postgres routes is identical to the API routes implementation. This is a violation of DRY principle.
Extract the common waitFor logic into a helper function:
const waitForElements = async (getAllByText: Function, text: string, count: number) => {
await waitFor(
async () => {
const elements = getAllByText(text);
expect(elements).toHaveLength(count);
},
{ timeout: process.env.TEST_TIMEOUT || 5000, interval: 100 },
);
};
vsvamsi1
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (1)
96-118: Consider standardizing the import formatting.
The lazy loading implementation is solid, but there's a minor inconsistency in the import statement formatting (some have line breaks, others don't).
Apply this formatting for consistency:
const JSEditor = lazy(async () =>
retryPromise(
async () =>
- import(/* webpackChunkName: "JSEditor" */ "pages/Editor/JSEditor"),
+ import(
+ /* webpackChunkName: "JSEditor" */
+ "pages/Editor/JSEditor"
+ ),
),
);📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (3 hunks)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (3 hunks)
- app/client/src/ce/pages/Editor/IDE/MainPane/useRoutes.tsx (2 hunks)
- app/client/src/pages/Editor/FirstTimeUserOnboarding/Modal.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx (7 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx (8 hunks)
- app/client/src/pages/Editor/JSEditor/JSBlankState.tsx (1 hunks)
- app/client/src/pages/Editor/JSEditor/index.tsx (0 hunks)
- app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx (1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/pages/Editor/JSEditor/index.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx
- app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx
- app/client/src/pages/Editor/JSEditor/JSBlankState.tsx
- app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx
🔇 Additional comments (7)
app/client/src/pages/Editor/FirstTimeUserOnboarding/Modal.tsx (3)
1-1: LGTM: Imports are properly structured
The necessary imports for lazy loading implementation are correctly added, including the retryPromise utility for handling failed imports.
Also applies to: 8-9
69-69: Verify test coverage for conditional rendering
The implementation looks correct, but given the failing Cypress tests mentioned in the PR objectives, we should ensure proper test coverage for this conditional rendering.
Let's check for existing tests:
#!/bin/bash
# Search for related test files
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . app/client/src/pages/Editor/FirstTimeUserOnboarding/11-18: Verify retry mechanism configuration
The lazy loading implementation looks good, but we should ensure the retry mechanism is properly configured for production scenarios.
Let's check if retryPromise is consistently used across other lazy-loaded components:
✅ Verification successful
Let me search for more specific information about the retry mechanism.
Let me check the implementation of retryPromise to verify its configuration.
retryPromise implementation is properly configured for production use
The implementation includes:
- Default 5 retries with 1-second intervals
- Proper error handling with custom error codes
- Consistent usage across lazy-loaded components in the codebase
- Built-in shouldRetry parameter for custom retry conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other lazy loading implementations to verify consistency
rg -l "lazy\(.*retryPromise" app/client/src/
Length of output: 247
Script:
#!/bin/bash
# Search for retryPromise implementation and usage
rg "retryPromise" -A 5 app/client/src/
Length of output: 27431
Script:
#!/bin/bash
# Get the implementation of retryPromise
cat app/client/src/utils/AppsmithUtils.tsx
Length of output: 16479
app/client/src/ce/pages/Editor/IDE/MainPane/useRoutes.tsx (2)
40-44: LGTM: Imports are well-organized and necessary.
All new imports are required for implementing lazy loading functionality.
46-53: Verify chunk size impact.
The lazy loading implementation looks good and follows best practices. The use of retryPromise adds resilience against network issues.
Let's verify the impact on chunk size:
✅ Verification successful
Lazy loading implementation follows established patterns
The implementation aligns with the project's established lazy loading patterns:
- Uses consistent retry mechanism with
retryPromise - Follows same webpack chunk naming convention as other components
- Component size is appropriate for lazy loading as it's a non-critical onboarding feature
- Implementation matches other lazy-loaded components in the codebase like APIEditor, JSEditor, and various widget components
The chunk size impact is acceptable as this follows the same pattern used across 20+ other lazy-loaded components in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential candidates for lazy loading in the same directory
rg -l "FirstTimeUserOnboarding" --type ts --type tsx
Length of output: 80
Script:
#!/bin/bash
# Search for FirstTimeUserOnboarding related files with correct file extensions
rg -l "FirstTimeUserOnboarding" -g "*.ts" -g "*.tsx"
# Get webpack configuration to understand chunk size settings
fd -e js -e ts -e json webpack
# Look for other lazy loaded components to compare implementation
ast-grep --pattern 'lazy(async () => $_)'
Length of output: 13489
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (2)
1-2: LGTM: Imports are well-structured for component decoupling.
The addition of React's lazy loading utilities and error handling through retryPromise shows good architectural planning.
Also applies to: 19-20
96-118: Verify chunk splitting configuration.
Let's confirm that the webpack configuration properly handles these lazy-loaded chunks.
✅ Verification successful
Lazy loading configuration is properly set up
The webpack configuration in craco.common.config.js shows proper chunk splitting setup with optimization.splitChunks enabled. The dynamic imports with webpackChunkName comments will create separate chunks as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check webpack configuration for chunk splitting
rg -l "splitChunks|webpackChunkName" webpack.config.*
# Verify no direct imports of these components elsewhere
rg --type ts --type tsx 'import.*(?:JSEditor|AddJS|JSEmpty).*from.*(?:pages/Editor/JSEditor|pages/Editor/IDE/EditorPane/JS/Add|pages/Editor/JSEditor/JSBlankState)'
Length of output: 308
Script:
#!/bin/bash
# Look for webpack config in common locations
fd -e js -e ts "webpack.config" .
# Look for any build configuration files
fd -e js -e ts -e json "build|config" .
# Check for dynamic imports in the codebase
rg "import\(.*webpackChunkName.*\)" --type js --type typescript
# Look for direct imports of these components
rg "import.*(?:JSEditor|AddJS|JSEmpty)" --type js --type typescript
Length of output: 22523
Script:
#!/bin/bash
# Check craco config for webpack configuration
cat ./app/client/craco.common.config.js
# Look for dynamic imports with webpackChunkName
rg -t js -t ts -t tsx "import\\(.*webpackChunkName.*\\)"
# Check for direct imports of these components
rg -t js -t ts -t tsx "import.*(?:JSEditor|AddJS|JSEmpty)"
Length of output: 5709
| export const LazilyLoadedChecklist = () => { | ||
| return ( | ||
| <Suspense fallback={<Skeleton />}> | ||
| <Checklist /> | ||
| </Suspense> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error boundary
While the Suspense fallback is implemented correctly, consider adding an error boundary to handle loading failures gracefully.
export const LazilyLoadedChecklist = () => {
return (
+ <ErrorBoundary fallback={<ErrorState />}>
<Suspense fallback={<Skeleton />}>
<Checklist />
</Suspense>
+ </ErrorBoundary>
);
};Committable suggestion was skipped due to low confidence.
| component: isPreviewMode | ||
| ? WidgetsEditor | ||
| : FirstTimeUserOnboardingChecklist, |
There was a problem hiding this comment.
Use the wrapper component in route configuration.
The route is using the lazy component directly instead of the wrapper component. This could lead to inconsistent loading states across routes.
- component: isPreviewMode
- ? WidgetsEditor
- : FirstTimeUserOnboardingChecklist,
+ component: isPreviewMode
+ ? WidgetsEditor
+ : LazilyLoadedFirstTimeUserOnboardingChecklist,Committable suggestion was skipped due to low confidence.
| export const LazilyLoadedFirstTimeUserOnboardingChecklist = () => { | ||
| return ( | ||
| <Suspense fallback={<Skeleton />}> | ||
| <FirstTimeUserOnboardingChecklist /> | ||
| </Suspense> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the wrapper component more reusable.
While the implementation is correct, consider creating a generic wrapper to handle lazy loading of components with consistent loading states.
-export const LazilyLoadedFirstTimeUserOnboardingChecklist = () => {
+interface LazyComponentProps {
+ Component: React.LazyExoticComponent<any>;
+}
+
+export const LazyLoadWrapper = ({ Component }: LazyComponentProps) => {
return (
<Suspense fallback={<Skeleton />}>
- <FirstTimeUserOnboardingChecklist />
+ <Component />
</Suspense>
);
};
+
+export const LazilyLoadedFirstTimeUserOnboardingChecklist = () => (
+ <LazyLoadWrapper Component={FirstTimeUserOnboardingChecklist} />
+);Committable suggestion was skipped due to low confidence.
| return useMemo( | ||
| () => [ | ||
| { | ||
| exact: true, | ||
| key: "AddJS", | ||
| component: (args) => ( | ||
| <Suspense fallback={<Skeleton />}> | ||
| <AddJS {...args} /> | ||
| </Suspense> | ||
| ), | ||
| path: [`${path}${ADD_PATH}`, `${path}/:baseCollectionId${ADD_PATH}`], | ||
| }, | ||
| { | ||
| exact: true, | ||
| key: "JSEditor", | ||
| component: (args) => ( | ||
| <Suspense fallback={<Skeleton />}> | ||
| <JSEditor {...args} /> | ||
| </Suspense> | ||
| ), | ||
| path: [path + "/:baseCollectionId"], | ||
| }, | ||
| { | ||
| key: "JSEmpty", | ||
| component: (args) => ( | ||
| <Suspense fallback={<Skeleton />}> | ||
| <JSEmpty {...args} /> | ||
| </Suspense> | ||
| ), | ||
| exact: true, | ||
| path: [path], | ||
| }, | ||
| ], | ||
| [path], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the loading state UX.
The route configuration with Suspense and memoization is well implemented. However, the Skeleton component could benefit from explicit sizing props to match the component it's replacing.
Consider updating the Skeleton implementation:
- <Suspense fallback={<Skeleton />}>
+ <Suspense
+ fallback={
+ <Skeleton
+ className="t--js-editor-skeleton"
+ height="100%"
+ width="100%"
+ />
+ }
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return useMemo( | |
| () => [ | |
| { | |
| exact: true, | |
| key: "AddJS", | |
| component: (args) => ( | |
| <Suspense fallback={<Skeleton />}> | |
| <AddJS {...args} /> | |
| </Suspense> | |
| ), | |
| path: [`${path}${ADD_PATH}`, `${path}/:baseCollectionId${ADD_PATH}`], | |
| }, | |
| { | |
| exact: true, | |
| key: "JSEditor", | |
| component: (args) => ( | |
| <Suspense fallback={<Skeleton />}> | |
| <JSEditor {...args} /> | |
| </Suspense> | |
| ), | |
| path: [path + "/:baseCollectionId"], | |
| }, | |
| { | |
| key: "JSEmpty", | |
| component: (args) => ( | |
| <Suspense fallback={<Skeleton />}> | |
| <JSEmpty {...args} /> | |
| </Suspense> | |
| ), | |
| exact: true, | |
| path: [path], | |
| }, | |
| ], | |
| [path], | |
| ); | |
| return useMemo( | |
| () => [ | |
| { | |
| exact: true, | |
| key: "AddJS", | |
| component: (args) => ( | |
| <Suspense | |
| fallback={ | |
| <Skeleton | |
| className="t--js-editor-skeleton" | |
| height="100%" | |
| width="100%" | |
| /> | |
| } | |
| > | |
| <AddJS {...args} /> | |
| </Suspense> | |
| ), | |
| path: [`${path}${ADD_PATH}`, `${path}/:baseCollectionId${ADD_PATH}`], | |
| }, | |
| { | |
| exact: true, | |
| key: "JSEditor", | |
| component: (args) => ( | |
| <Suspense | |
| fallback={ | |
| <Skeleton | |
| className="t--js-editor-skeleton" | |
| height="100%" | |
| width="100%" | |
| /> | |
| } | |
| > | |
| <JSEditor {...args} /> | |
| </Suspense> | |
| ), | |
| path: [path + "/:baseCollectionId"], | |
| }, | |
| { | |
| key: "JSEmpty", | |
| component: (args) => ( | |
| <Suspense | |
| fallback={ | |
| <Skeleton | |
| className="t--js-editor-skeleton" | |
| height="100%" | |
| width="100%" | |
| /> | |
| } | |
| > | |
| <JSEmpty {...args} /> | |
| </Suspense> | |
| ), | |
| exact: true, | |
| path: [path], | |
| }, | |
| ], | |
| [path], | |
| ); |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11589182779. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/limited-tests.txt (1 hunks)
- app/client/cypress/support/Pages/JSEditor.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/limited-tests.txt (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Pages/JSEditor.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (1)
app/client/cypress/limited-tests.txt (1)
3-4: Reconsider adding failing test specs to limited-tests.txt
According to the PR description, these two specs are currently failing:
- WidgetPropertySetters1_spec.ts
- EntityPropertiesLint_spec.ts
Adding failing tests to limited-tests.txt might mask underlying issues. Consider:
- Fixing the failing tests first
- Adding them to limited-tests.txt only after they pass consistently
Let's verify if these tests are related to the editor component changes:
| @@ -113,6 +113,7 @@ export class JSEditor { | |||
| ); | |||
| //Checking JS object was created successfully | |||
| this.assertHelper.AssertNetworkStatus("@createNewJSCollection", 201); | |||
| cy.wait(800); | |||
There was a problem hiding this comment.
Remove cy.wait() and use Cypress best practices for handling async operations.
The added cy.wait(800) is against our coding guidelines. Cypress tests should avoid arbitrary wait times as they make tests flaky and slow.
Replace the wait with one of these alternatives:
- cy.wait(800);
+ // Option 1: Wait for element to be actionable
+ this.agHelper.AssertElementVisibility(this._jsObjTxt, 0, true);
+ // Option 2: Wait for specific XHR request
+ cy.intercept('POST', '**/api/v1/collections').as('jsCollection');
+ cy.wait('@jsCollection');This will make the test more reliable and maintainable while following Cypress best practices.
Committable suggestion was skipped due to low confidence.
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11591708339. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11591708339. |
vsvamsi1
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11593135485. |
|
Deploy-Preview-URL: https://ce-37102.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (1)
96-118: LGTM: Well-implemented lazy loading with error handling.
The implementation follows best practices with proper chunk naming and error handling via retryPromise.
Consider aligning the import formatting for consistency:
-const AddJS = lazy(async () =>
- retryPromise(
- async () =>
- import(
- /* webpackChunkName: "AddJS" */ "pages/Editor/IDE/EditorPane/JS/Add"
- ),
- ),
-);
+const AddJS = lazy(async () =>
+ retryPromise(async () =>
+ import(/* webpackChunkName: "AddJS" */ "pages/Editor/IDE/EditorPane/JS/Add")
+ )
+);📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- app/client/cypress/support/Pages/JSEditor.ts (1 hunks)
- app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (3 hunks)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (3 hunks)
- app/client/src/ce/pages/Editor/IDE/MainPane/useRoutes.tsx (2 hunks)
- app/client/src/pages/Editor/FirstTimeUserOnboarding/Modal.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx (7 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx (8 hunks)
- app/client/src/pages/Editor/JSEditor/JSBlankState.tsx (1 hunks)
- app/client/src/pages/Editor/JSEditor/index.tsx (0 hunks)
- app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx (1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/pages/Editor/JSEditor/index.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- app/client/cypress/support/Pages/JSEditor.ts
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx
- app/client/src/pages/Editor/FirstTimeUserOnboarding/Modal.tsx
- app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx
- app/client/src/pages/Editor/JSEditor/JSBlankState.tsx
- app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx
🔇 Additional comments (8)
app/client/src/ce/pages/Editor/IDE/MainPane/useRoutes.tsx (5)
40-44: LGTM: Import statements are properly organized.
The necessary imports for lazy loading implementation are correctly added.
46-53: LGTM: Lazy loading implementation with retry mechanism.
The implementation correctly uses retryPromise for resilient loading, which is a good practice for handling network issues.
55-61: Skip comment: Previous review already addressed the wrapper component.
118-120:
Fix component reference in route configuration.
The route is using FirstTimeUserOnboardingChecklist directly instead of the wrapped version LazilyLoadedFirstTimeUserOnboardingChecklist.
Apply this fix:
component: isPreviewMode
? WidgetsEditor
- : FirstTimeUserOnboardingChecklist,
+ : LazilyLoadedFirstTimeUserOnboardingChecklist,Likely invalid or redundant comment.
Line range hint 46-120: Verify lazy loading implementation across the application.
Let's ensure consistent lazy loading implementation across other components in the application.
✅ Verification successful
Lazy loading implementation is consistent with application patterns
The implementation follows the established patterns across the codebase:
- Uses React.lazy with retryPromise wrapper
- Consistently wraps lazy components with Suspense and Skeleton fallback
- Follows the same dynamic import pattern with webpackChunkName comments
The search results show this pattern is consistently used across:
- Widget components (TableWidget, VideoWidget, etc.)
- Editor components (QueryEditor, JSEditor, etc.)
- Modal components (GlobalSearch, WalkthroughRenderer, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other direct uses of React.lazy or lazy imports
rg --type typescript --type typescript "lazy\(" -A 2
# Search for Suspense usage patterns
rg --type typescript --type typescript "<Suspense" -A 2
# Search for potential missing lazy loading opportunities
ast-grep --pattern 'import { $_ } from "pages/Editor/$$$"'
Length of output: 243
Script:
#!/bin/bash
# Search for lazy loading patterns
rg "lazy\(" --type ts --type tsx -A 2
# Search for Suspense usage
rg "<Suspense" --type ts --type tsx -A 2
# Look for direct imports from Editor pages
rg "from ['\"].*pages/Editor" --type ts --type tsx
Length of output: 221
Script:
#!/bin/bash
# Search for lazy loading patterns without file type restrictions
rg "lazy\(" -A 2
# Search for Suspense usage without file type restrictions
rg "<Suspense" -A 2
# Look for direct imports from Editor pages
rg "from ['\"].*pages/Editor"
Length of output: 64200
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (3)
1-2: LGTM: Imports are well organized.
The new imports properly support the lazy loading implementation.
Also applies to: 19-20
121-155: LGTM: Route configuration is well structured.
The memoization is properly implemented with correct dependencies, and the route structure is consistent across all components.
96-118: Verify chunk splitting configuration.
Let's ensure the webpack chunks are properly configured across the codebase.
✅ Verification successful
Based on the results, I can generate the final response as the verification is complete. The webpack chunk configuration appears to be properly implemented:
Webpack chunk configuration is correctly implemented ✅
- Each lazy-loaded component has a unique chunk name
- The uppy-related imports are correctly grouped under the same chunk name
- No conflicting or duplicate chunk names found for the JS editor components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent chunk naming patterns and potential duplicate chunk names
# Search for webpack chunk name comments
rg -g '*.{ts,tsx,js,jsx}' 'webpackChunkName:' --no-filename | sort | uniq -c
Length of output: 2462
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (1)
96-118: Consider consistent formatting for lazy component definitions.
While the implementation is correct, the formatting could be more consistent. Consider aligning the import paths for better readability.
const AddJS = lazy(async () =>
retryPromise(
async () =>
- import(
- /* webpackChunkName: "AddJS" */ "pages/Editor/IDE/EditorPane/JS/Add"
- ),
+ import(/* webpackChunkName: "AddJS" */ "pages/Editor/IDE/EditorPane/JS/Add"),
),
);📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- app/client/cypress/support/Pages/JSEditor.ts (1 hunks)
- app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (3 hunks)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (3 hunks)
- app/client/src/ce/pages/Editor/IDE/MainPane/useRoutes.tsx (2 hunks)
- app/client/src/pages/Editor/FirstTimeUserOnboarding/Modal.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx (7 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx (8 hunks)
- app/client/src/pages/Editor/JSEditor/JSBlankState.tsx (1 hunks)
- app/client/src/pages/Editor/JSEditor/index.tsx (0 hunks)
- app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx (1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/pages/Editor/JSEditor/index.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- app/client/cypress/support/Pages/JSEditor.ts
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx
- app/client/src/pages/Editor/FirstTimeUserOnboarding/Modal.tsx
- app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx
- app/client/src/pages/Editor/JSEditor/JSBlankState.tsx
- app/client/src/pages/Editor/QueryEditor/QueriesBlankState.tsx
🔇 Additional comments (5)
app/client/src/ce/pages/Editor/IDE/MainPane/useRoutes.tsx (3)
40-44: LGTM! All required imports for lazy loading are present.
55-61: Skip comment as the past review suggestion about making the wrapper more reusable is still valid.
118-120: Skip comment as the past review suggestion about using the wrapper component in route configuration is still valid.
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.tsx (2)
1-2: LGTM: Import statements are well-organized.
The new imports for lazy loading functionality are properly structured and all imported items are utilized.
Also applies to: 19-20
121-155: Verify route path consistency across the application.
The route configuration looks good, but let's verify that the path patterns are consistent with other parts of the application.
✅ Verification successful
Let me fix the regex and search for both the ADD_PATH constant and general route path patterns to verify consistency.
Route path patterns are consistent with the codebase standards
The route configuration in the JS editor follows the same pattern used across the application:
- Uses the same
ADD_PATHconstant fromconstants/routes - Follows the same path structure for add/edit routes:
${path}${ADD_PATH}and${path}/:entityId${ADD_PATH} - Maintains consistency with other editor panes like Query editor
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar route patterns to ensure consistency
rg -g '*.{ts,tsx}' -A 2 'path: \[`\${.*}\${ADD_PATH}`'
Length of output: 179
Script:
#!/bin/bash
# Search for route path patterns with ADD_PATH
rg -g '*.{ts,tsx}' 'ADD_PATH' -B 2 -A 2
# Search for similar route configurations
rg -g '*.{ts,tsx}' 'path: \[.*\],' -B 2 -A 2
Length of output: 38249
| const FirstTimeUserOnboardingChecklist = lazy(async () => | ||
| retryPromise( | ||
| async () => | ||
| import( | ||
| /* webpackChunkName: "FirstTimeUserOnboardingChecklist" */ "pages/Editor/FirstTimeUserOnboarding/Checklist" | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding an error boundary for lazy loading failures.
The lazy loading implementation is solid with retry capability. However, adding an error boundary would provide a better user experience when the chunk fails to load.
class LazyLoadErrorBoundary extends React.Component {
state = { hasError: false };
static getDerivedStateFromError() {
return { hasError: true };
}
render() {
if (this.state.hasError) {
return <div>Failed to load component. Please refresh the page.</div>;
}
return this.props.children;
}
}
| @@ -26,4 +26,4 @@ const JSBlankState = () => { | |||
| ); | |||
| }; | |||
|
|
|||
| export { JSBlankState }; | |||
| export default JSBlankState; | |||
There was a problem hiding this comment.
Isn't it preferable to avoid default exports ?
There was a problem hiding this comment.
To import a component lazily i has to be a default export, that is the reason why i had to refactor it from a named export.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11661494395. |
|
Deploy-Preview-URL: https://ce-37102.dp.appsmith.com |
## Description Decouple editor components and codemirror from the main chunk, the main chunk has dropped by 200Kb to 1.1Mb Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!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 <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11593047595> > Commit: 4a5b3bc > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11593047595&attempt=3" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 30 Oct 2024 13:57:44 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Implemented lazy loading for several components, enhancing performance and user experience during loading states. - Introduced new error handling UI with `EntityNotFoundPane` for better feedback when data is unavailable. - Added `LazilyLoadedChecklist` for optimized loading of the checklist component in onboarding. - **Bug Fixes** - Improved asynchronous handling in test cases for more reliable rendering assertions. - **Documentation** - Updated export statements for components to default exports for consistency. - **Chores** - Refactored routing logic to utilize lazy-loaded components across various sections of the application. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Decouple editor components and codemirror from the main chunk, the main chunk has dropped by 200Kb to 1.1Mb
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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/11593047595
Commit: 4a5b3bc
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 30 Oct 2024 13:57:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
EntityNotFoundPanefor better feedback when data is unavailable.LazilyLoadedChecklistfor optimized loading of the checklist component in onboarding.Bug Fixes
Documentation
Chores