Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat : Query manager separated to different tabs #9823

Merged
merged 49 commits into from
Jun 14, 2024

Conversation

stepinfwd
Copy link
Collaborator

closes #9438

@stepinfwd stepinfwd added appbuilder wip Work in progeress labels May 22, 2024
@stepinfwd stepinfwd removed the wip Work in progeress label May 23, 2024
@stepinfwd stepinfwd linked an issue May 24, 2024 that may be closed by this pull request
@johnsoncherian
Copy link
Collaborator

/review

@johnsoncherian johnsoncherian merged commit 371b54c into appbuilder-1.8 Jun 14, 2024
3 checks passed
@johnsoncherian johnsoncherian deleted the feature/query-manager-body branch June 14, 2024 11:10
@TooljetBot
Copy link
Collaborator

TooljetBot commented Jun 14, 2024

Code Review Agent Run #46c012

  • AI Based Review: ✔️ Successful

Code Review Overview

  • Summary: The changes primarily focus on updating styles in SCSS files, enhancing QueryManager components, improving UI/UX, and adding new properties and logic for better functionality. Significant updates include the addition of tabs in QueryManagerHeader, restructuring of the Transformation component, and various styling adjustments for better layout and user experience.
  • Files: 40
  • Issue found: Total - 10, High importance - 10      See detailed feedback ->
  • Code change type: Refactoring, UI/UX Improvement, Performance Improvement
  • Unit tests added: False
  • Estimated effort to review (1-5, lower is better): 3

High-level Feedback

Consider moving inline styles to CSS classes for better maintainability, ensure parent containers have defined heights to avoid layout issues, and validate new props to prevent unexpected behavior. Additionally, optimize useEffect dependencies to ensure consistent behavior and remove redundant props to simplify component logic.

Detailed Feedback

📄 frontend/src/Editor/QueryManager/Components/QueryManagerBody.jsx
Issues: Total- 1, High importance- 1
Line 342-342 🔴 High importance - 1   
📄 frontend/src/_styles/colors.scss
Issues: Total- 2, High importance- 2
Line 43-43 🔴 High importance - 1   
Line 223-223 🔴 High importance - 1   
📄 frontend/src/_styles/queryManager.scss
Issues: Total- 1, High importance- 1
Line 308-308 🔴 High importance - 1   
📄 frontend/src/Editor/CodeEditor/MultiLineCodeEditor.jsx
Issues: Total- 1, High importance- 1
Line 257-257 🔴 High importance - 1   
📄 frontend/src/Editor/QueryManager/Components/QueryManagerHeader.jsx
Issues: Total- 1, High importance- 1
Line 94-102 🔴 High importance - 1   
📄 frontend/src/_helpers/appUtils.js
Issues: Total- 1, High importance- 1
Line 1144-1148 🔴 High importance - 1   
📄 frontend/src/_ui/Icon/solidIcons/ArrowDownTriangle.jsx
Issues: Total- 1, High importance- 1
Line 3-3 🔴 High importance - 1   
📄 frontend/src/Editor/QueryManager/Components/Preview.jsx
Issues: Total- 1, High importance- 1
Line 33-33 🔴 High importance - 1   
📄 frontend/src/Editor/QueryManager/QueryManager.jsx
Issues: Total- 1, High importance- 1
Line 28-32 🔴 High importance - 1   

AI Code Review powered by Bito Logo

selectedDataSource?.kind === 'tooljetdb' ? 'tooljetdb-query-details' : ''
}`}
className={`query-details ${selectedDataSource?.kind === 'tooljetdb' ? 'tooljetdb-query-details' : ''}`}
style={{ height: `calc(100% - ${previewHeight + 40}px )`, overflowY: 'auto' }} //40px in preview header height
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: Inline styles are used for setting the height and overflow properties. This can lead to difficulties in maintaining and updating the styles, especially if these styles need to be reused or modified in multiple places.
Fix: Move the inline styles to a CSS class to ensure better maintainability and consistency across the codebase.
Code suggestion

 @@ -341,7 +341,7 @@
       className={'query-details ${selectedDataSource?.kind === 'tooljetdb' ? 'tooljetdb-query-details' : ''}'}
 -     style={{ height: 'calc(100% - ${previewHeight + 40}px )', overflowY: 'auto' }} //40px in preview header height
 +     className={'query-details ${selectedDataSource?.kind === 'tooljetdb' ? 'tooljetdb-query-details' : ''} custom-query-details'}
    >

$color-light-indigo-03: #F0F4FF;
$color-dark-indigo-03: #192140;
$color-light-indigo-04: #E6EDFE;
;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: There is an extraneous semicolon on line 43, which could cause issues in the CSS compilation or lead to unexpected behavior.
Fix: Remove the extraneous semicolon to ensure proper CSS syntax.
Code suggestion

 @@ -42,7 +42,6 @@
  $color-light-indigo-04: #E6EDFE;
 -;
  $color-dark-indigo-04: #1C274F;

}

.text-default {
color: var(--text-default)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: Missing semicolon in the CSS rule for '.text-default'. This can cause issues in CSS parsing and lead to unexpected behavior in styles.
Fix: Add a semicolon at the end of the 'color' property in the '.text-default' rule.
Code suggestion

 @@ -222,7 +222,7 @@
  .text-default {
    color: var(--text-default);
  }

@@ -251,6 +305,8 @@ $border-radius: 4px;
}

.query-definition-pane {
height: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: Setting the height to 100% for the '.query-definition-pane' class without considering its parent container's height can lead to layout issues, especially if the parent container does not have a defined height.
Fix: Ensure that the parent container of '.query-definition-pane' has a defined height or use a more flexible layout approach such as flexbox or grid to manage the height.
Code suggestion

 @@ -307,7 +307,7 @@
  .query-definition-pane {
 -    height: 100%;
 +    flex: 1;

@@ -252,6 +254,8 @@ const MultiLineCodeEditor = (props) => {
}}
className={`codehinter-multi-line-input`}
indentWithTab={true}
readOnly={readOnly}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: The 'readOnly' and 'editable' props are both being used, which can cause confusion and potential conflicts in the component's behavior. Typically, 'readOnly' should suffice for controlling editability.
Fix: Remove the 'editable' prop and rely solely on the 'readOnly' prop to control the editability of the component.
Code suggestion

 @@ -42,8 +42,7 @@ const MultiLineCodeEditor = (props) => {
      showPreview,
      paramLabel = '',
      delayOnChange = true, // Added this prop to immediately update the onBlurUpdate callback
 -    readOnly = false,
 -    editable = true,
 +    readOnly = false,
    } = props;
const [currentValue, setCurrentValue] = React.useState(() => initialValue);

@@ -254,8 +253,7 @@ const MultiLineCodeEditor = (props) => {
overflowY: 'auto',
}}
className={'codehinter-multi-line-input'}
indentWithTab={true}

  •        readOnly={readOnly}
    
  •        editable={editable} //for transformations in query manager
    
  •        readOnly={readOnly}
         />
       </div>
       {showPreview && (
    


Comment on lines +94 to +102
const tabs = [
{ id: 1, label: 'Setup' },
{
id: 2,
label: 'Transformation',
condition: selectedQuery?.kind !== 'runpy' && selectedQuery?.kind !== 'runjs',
},
{ id: 3, label: 'Settings' },
];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: The 'tabs' array is defined within the component function, which means it will be recreated on every render. This can lead to unnecessary re-renders and performance issues, especially if the component re-renders frequently.
Fix: Move the 'tabs' array definition outside of the component function to ensure it is only created once.
Code suggestion

 @@ -93,6 +93,15 @@
  };

+const tabs = [

  • { id: 1, label: 'Setup' },
  • {
  • id: 2,
  • label: 'Transformation',
  • condition: selectedQuery?.kind !== 'runpy' && selectedQuery?.kind !== 'runjs',
  • },
  • { id: 3, label: 'Settings' },
    +];

const QueryManagerHeader = forwardRef(({ darkMode, options, editorRef, selectedQuery, queries, runQuery, previewButtonOnClick, buttonLoadingState, isInDraft, executeQueryNameUpdation, queryName, isVersionReleased, showCreateQuery }, ref) => {
const renderRunButton = () => {


Comment on lines +1144 to +1148
if (shouldSetPreviewData) {
setPreviewPanelExpanded(true);
setPreviewLoading(true);
queryPreviewData && setPreviewData('');
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: The new conditional block that sets the preview panel state ('setPreviewPanelExpanded', 'setPreviewLoading', 'setPreviewData') is executed before checking if the 'query' exists. This can lead to unnecessary state updates and potential performance issues if the query is not found.
Fix: Move the conditional block inside the 'if (query)' block to ensure state updates only occur when the query exists.
Code suggestion

 @@ -1143,11 +1143,6 @@
  • if (shouldSetPreviewData) {
  • setPreviewPanelExpanded(true);
  • setPreviewLoading(true);
  • queryPreviewData && setPreviewData('');
  • }
    if (query) {
    dataQuery = JSON.parse(JSON.stringify(query));
  • if (shouldSetPreviewData) {
  •  setPreviewPanelExpanded(true);
    
  •  setPreviewLoading(true);
    
  •  queryPreviewData && setPreviewData('');
    
  • }
    } else {
    toast.error('No query has been associated with the action.');
    return;

@@ -1,13 +1,14 @@
import React from 'react';

const ArrowDownTriangle = ({ fill = '#C1C8CD', width = '24', className = '', viewBox = '0 0 24 24' }) => (
const ArrowDownTriangle = ({ fill = '#C1C8CD', width = '24', className = '', viewBox = '0 0 24 24', style = {} }) => (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: The addition of the 'style' prop to the 'ArrowDownTriangle' component without validation or default values can lead to unexpected styling issues if incorrect styles are passed.
Fix: Add prop-types to validate the 'style' prop and provide default values to ensure consistent behavior.
Code suggestion

 @@ -1,13 +1,14 @@
 +import PropTypes from 'prop-types';
 const ArrowDownTriangle = ({ fill = '#C1C8CD', width = '24', className = '', viewBox = '0 0 24 24', style = {} }) => (
   <svg
     xmlns="http://www.w3.org/2000/svg"
     width={width}
     height={width}
     viewBox={viewBox}
     fill="none"
     className={className}
     style={style}
   >
     <path
       fill-rule="evenodd"
       clip-rule="evenodd"
       d="M9.20923 10.7525C9.00495 10.4589 8.94384 10.0173 9.05439 9.63373C9.16495 9.25012 9.4254 9 9.7143 9H18.2857C18.5746 9 18.835 9.25012 18.9456 9.63373C19.0562 10.0173 18.995 10.4589 18.7907 10.7525L14.7576 16.5491C14.3392 17.1503 13.6608 17.1503 13.2424 16.5491L9.20923 10.7525Z"
       fill={fill}
     />
   </svg>
 );
 +ArrowDownTriangle.propTypes = {
 +  fill: PropTypes.string,
 +  width: PropTypes.string,
 +  className: PropTypes.string,
 +  viewBox: PropTypes.string,
 +  style: PropTypes.object,
 +};
 export default ArrowDownTriangle;

const previewPanelRef = useRef();
const queryPanelHeight = usePanelHeight();

useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: The 'calculatePreviewHeight' function is called inside a 'useEffect' hook without any dependencies, which means it will only run once when the component mounts. This could lead to issues if the 'height' or 'previewPanelExpanded' values change after the initial render, as the height calculation will not be updated.
Fix: Add 'height' and 'previewPanelExpanded' as dependencies to the 'useEffect' hook to ensure the height calculation is updated whenever these values change.
Code suggestion

 @@ -33,7 +33,7 @@
   useEffect(() => {
     calculatePreviewHeight(height, previewPanelExpanded);
     // eslint-disable-next-line react-hooks/exhaustive-deps
 -  }, []);
 +  }, [height, previewPanelExpanded]);

Comment on lines +28 to +32
useEffect(() => {
if (selectedQuery?.kind == 'runjs' || selectedQuery?.kind == 'runpy') {
setActiveTab(1);
}
}, [selectedQuery?.id]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bito Code Review Agent Run #46c012 - 06/14/2024, 11:19 am

🔴 High importance
Issue: The useEffect hook that sets the active tab based on the selected query kind is missing a dependency on 'selectedQuery?.kind'. This can lead to inconsistent behavior if 'selectedQuery' changes but its 'id' does not.
Fix: Add 'selectedQuery?.kind' to the dependency array of the useEffect hook to ensure it re-runs when the kind of the selected query changes.
Code suggestion

 @@ -32,7 +32,7 @@ const QueryManager = ({ mode, appId, darkMode, apps, allComponents, appDefinitio
   }, [selectedQuery?.id, selectedQuery?.kind]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query manager position and preview enhancement
7 participants