chore: [Plugin Action Editor] Query Form evaluation #37224
Conversation
WalkthroughThe changes in this pull request involve modifications to several components within the Plugin Action Editor. Key adjustments include the replacement of the 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
|
hetunandu
requested review from
ankitakinger
and removed request for
ayushpahwa
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: 6
🧹 Outside diff range and nitpick comments (13)
app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useInitFormEvaluation.ts (2)
15-20: Consider adding error handling for the dispatch call.The form evaluation initialization could potentially fail, and it would be beneficial to handle such cases gracefully.
useEffect( function formEvaluationInit() { - dispatch(initFormEvaluations(editorConfig, settingsConfig, baseId)); + try { + dispatch(initFormEvaluations(editorConfig, settingsConfig, baseId)); + } catch (error) { + console.error("Failed to initialize form evaluations:", error); + } }, [baseId, dispatch, editorConfig, settingsConfig], );
7-7: Consider memoizing the dispatch call.For optimal performance, you might want to memoize the dispatch call if it's used in other effects or callbacks within the same component.
-const dispatch = useDispatch(); +const dispatch = useCallback(useDispatch(), []);Don't forget to add the import:
-import { useEffect } from "react"; +import { useEffect, useCallback } from "react";app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useFormData.ts (1)
7-11: Consider using type guards instead of type assertionThe current type assertion could be made safer using type guards to validate the shape of the data at runtime.
type FormData = QueryAction | SaaSAction; const isFormData = (data: unknown): data is FormData => { return data !== null && typeof data === 'object' && 'id' in data; }; export const useFormData = () => { const data = useSelector(getFormValues(QUERY_EDITOR_FORM_NAME)); if (!isFormData(data)) { return { data: null, evaluationState: {} }; } // ... rest of the implementation };app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Section/styles.module.css (1)
23-25: Consider using CSS custom property for border style.The border styling could be moved to a CSS custom property for better maintainability and consistency across the application.
- border-bottom: 1px solid var(--ads-v2-color-border); + border-bottom: var(--ads-v2-border-width, 1px) solid var(--ads-v2-color-border);app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useGoogleSheetsSetDefaultProperty.ts (2)
8-8: Consider using path aliases for deeply nested imports.The relative import path is quite deep. Consider using TypeScript path aliases to improve code maintainability.
-import { usePluginActionContext } from "../../../../../PluginActionContext"; +import { usePluginActionContext } from "@components/PluginActionContext";
21-22: Use a more descriptive function name for the useEffect callback.The current name doesn't fully convey the purpose of synchronizing default values with Redux store.
-function setDefaultValuesForGoogleSheets() { +function synchronizeGoogleSheetsDefaultValuesWithStore() {app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx (1)
27-27: Consider keeping overflow: auto for better UXUsing
scrollwill always show scrollbars even when content fits, whileautoonly shows them when needed. This could lead to unnecessary visual clutter.- overflow: scroll; + overflow: auto;app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/DatasourceConfig.tsx (2)
Line range hint
114-182: Improve type safety in ImportedKeyValue component.Replace
anytypes with proper interfaces to improve type safety and maintainability.+ interface KeyValueData { + key: string; + value: string; + isInvalid?: boolean; + } - function ImportedKeyValue(props: { - datas: { key: string; value: string; isInvalid?: boolean }[]; + function ImportedKeyValue(props: { + datas: KeyValueData[]; keyValueName: string; }) {
Line range hint
109-111: Track and address technical debt systematically.Multiple TODO comments indicate technical debt around type safety and ESLint issues. These should be tracked in the issue tracker for systematic resolution.
Would you like me to create GitHub issues to track these TODOs? The issues would cover:
- Fixing eslint-disable comments
- Implementing proper TypeScript types
- Evaluating the toggle button implementation
Also applies to: 191-193, 207-209
app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx (1)
Line range hint
134-174: Consider improving type safety in the interface.While the new
httpsMethodstype is well-defined, there are several TODO comments indicating the use ofanytype. Consider creating proper types for:
- actionConfigurationHeaders
- actionConfigurationParams
- datasourceHeaders
- datasourceParams
- settingsConfig
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/EmbeddedDatasourcePathField.tsx (3)
Line range hint
324-331: Add type validation before type assertionThe current implementation assumes the event structure without validation, which could lead to runtime errors.
Consider this safer implementation:
- const value: string = - typeof valueOrEvent === "string" - ? valueOrEvent - : valueOrEvent.target.value; + const value: string = typeof valueOrEvent === "string" + ? valueOrEvent + : valueOrEvent && 'target' in valueOrEvent && 'value' in valueOrEvent.target + ? valueOrEvent.target.value + : "";
Line range hint
356-386: Consider breaking down the highlight logicThe method handles multiple responsibilities: auth type checking, error state, and text marking.
Consider extracting the highlight class determination:
+ const getHighlightClassName = (authType: string, hasError: boolean) => { + let className = "datasource-highlight"; + if (authType === AuthType.OAuth2) { + className = `${className} ${ + hasError ? "datasource-highlight-error" : "datasource-highlight-success" + }`; + } + return className; + }; handleDatasourceHighlight = () => { const { datasource } = this.props; const authType: string = get( datasource, "datasourceConfiguration.authentication.authenticationType", "", ); const hasError = !get(datasource, "isValid", true); - let className = "datasource-highlight"; - if (authType === AuthType.OAuth2) { - className = `${className} ${ - hasError ? "datasource-highlight-error" : "datasource-highlight-success" - }`; - } + const className = getHighlightClassName(authType, hasError);
Line range hint
484-493: Add URL parameter sanitizationQuery parameters are being concatenated directly without sanitization, which could lead to XSS vulnerabilities.
Consider using URL encoding:
const evaluatedQueryParameters = entity?.config?.queryParameters ?.filter((p: KeyValuePair) => !!p?.key) .map( - (p: KeyValuePair, i: number) => - `${i === 0 ? "?" : "&"}${p.key}=${p.value}`, + (p: KeyValuePair, i: number) => { + const encodedKey = encodeURIComponent(p.key); + const encodedValue = encodeURIComponent(p.value); + return `${i === 0 ? "?" : "&"}${encodedKey}=${encodedValue}`; + }, ) .join("");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Section/index.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Section/styles.module.css(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx(0 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/InfoFields.tsx(3 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/DatasourceConfig.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/EmbeddedDatasourcePathField.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/styles.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/GraphQLEditor/PostBodyData.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/UQIEditorForm.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useFormData.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useGoogleSheetsSetDefaultProperty.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useInitFormEvaluation.ts(1 hunks)app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx
✅ Files skipped from review due to trivial changes (2)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/styles.ts
🧰 Additional context used
📓 Learnings (1)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx (2)
Learnt from: hetunandu
PR: appsmithorg/appsmith#36380
File: app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx:32-58
Timestamp: 2024-09-19T10:42:56.559Z
Learning: The tab rendering logic of the `RequestTabs` component will be extracted into separate components in a future PR.
Learnt from: hetunandu
PR: appsmithorg/appsmith#36380
File: app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx:32-58
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The tab rendering logic of the `RequestTabs` component will be extracted into separate components in a future PR.
🪛 Biome
app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useFormData.ts
[error] 17-17: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (26)
app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useInitFormEvaluation.ts (2)
6-21: LGTM! Clean and well-structured hook implementation.
The hook follows React conventions and properly handles dependencies.
15-20: Verify the action creator implementation.
Ensure that the initFormEvaluations action creator is properly implemented and handles all the parameters correctly.
✅ Verification successful
Let me verify the reducer implementation to ensure the action is properly handled.
Let me check the saga implementation to verify how the action payload is handled.
Action creator implementation is correctly handling the parameters
The action creator initFormEvaluations is properly implemented in evaluationActions.ts. It correctly:
- Takes the required parameters: editorConfig, settingConfig, and formId
- Creates an action with type
INIT_FORM_EVALUATION - Passes all parameters in the payload
The action is also properly consumed in:
- Form evaluation saga for dynamic value processing
- Form evaluation worker for state updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of initFormEvaluations
ast-grep --pattern 'export const initFormEvaluations = ($$$) => {
$$$
}'
Length of output: 1197
Script:
#!/bin/bash
# Search for the reducer handling INIT_FORM_EVALUATION
ast-grep --pattern 'case ReduxActionTypes.INIT_FORM_EVALUATION: {
$$$
}'
# Also check the action type definition
rg "INIT_FORM_EVALUATION" -A 1 -B 1
Length of output: 1773
Script:
#!/bin/bash
# Search for the saga handling INIT_FORM_EVALUATION
ast-grep --pattern 'function* formEvaluationChangeListener(
$$$
) {
$$$
}'
# Also check for any related utility functions
rg "extractFetchDynamicValueFormConfigs" -A 2
Length of output: 2195
app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Section/index.tsx (3)
7-7: LGTM! Property naming follows React conventions.
The new withoutPadding prop name clearly communicates its purpose and follows React's boolean prop naming conventions.
15-15: LGTM! Default value is appropriately set.
The default value of false ensures backward compatibility, as sections will maintain padding by default.
24-24: Verify usage of data-withoutPadding in consuming components.
The attribute change from data-standalone to data-withoutPadding might affect existing CSS selectors.
✅ Verification successful
Let me gather more information about the Section component usage and styling.
Let me check for any CSS styling related to the data attributes.
Let me check the CSS module file for the styling rules.
No issues found with data-withoutPadding attribute usage
The CSS module properly uses the data-withoutPadding attribute for styling, and there was no previous usage of data-standalone. The attribute is correctly implemented in both the component and its styles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing CSS selectors using the old data-standalone attribute
rg -l 'data-standalone' --type css
# Search for components using the Section component to verify prop updates
ast-grep --pattern 'import { Section } from "$$$"'
Length of output: 344
Script:
#!/bin/bash
# Check the content of the files that import Section
cat app/client/src/pages/Templates/Template/SimilarTemplates.tsx
cat app/client/src/components/editorComponents/WidgetQueryGeneratorForm/DatasourceSpecificControls/index.tsx
# Check the Section component's style definitions
cat app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Section/styles.ts
Length of output: 4192
Script:
#!/bin/bash
# Search for any CSS/styled-components using data attributes
rg 'data-withoutPadding|data-standalone' --type ts --type tsx
# Check the actual Section component implementation
cat app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Section/index.tsx
Length of output: 869
Script:
#!/bin/bash
# Check the CSS module file
cat app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Section/styles.module.css
# Search for any CSS modules using data attributes with correct file extension
rg 'data-withoutPadding|data-standalone' --type css --type scss
Length of output: 988
app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useFormData.ts (1)
1-6: LGTM! Clean imports and type declarations.
app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Section/styles.module.css (3)
9-11: LGTM: Clean implementation of the base padding rule.
The new data-withoutPadding attribute provides a clear and semantic way to control section padding.
18-20: LGTM: Consistent padding for non-first sections.
The use of padding-block is a modern and maintainable approach for vertical padding.
13-16: Consider adding top margin compensation for the first section.
While removing top padding from the first section is logical, you might want to consider if any margin compensation is needed when this section is placed below other elements.
app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/UQIEditorForm.tsx (5)
12-20: LGTM! Hook usage follows React best practices.
The hooks are called in a consistent order and the destructuring is clean. The separation of concerns between form initialization and Google Sheets specific logic is well done.
22-22: Clean implementation of form data management.
Good refactoring to use a custom hook instead of direct Redux selectors. This improves code maintainability and encapsulation.
Line range hint 35-39: LGTM! Standard Redux Form configuration.
The form configuration with enableReinitialize is appropriate for this use case.
7-9: Verify the implementation of newly added hooks.
The code introduces three new custom hooks. Let's verify their implementation to ensure they exist and follow best practices.
✅ Verification successful
All custom hooks are properly implemented and follow React best practices
The hooks are well-organized, properly scoped, and follow the single responsibility principle:
useGoogleSheetsSetDefaultProperty: Handles Google Sheets specific default valuesuseFormData: Manages form state and evaluation datauseInitFormEvaluation: Handles form evaluation initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the hook files exist and examine their implementation
for hook in useGoogleSheetsSetDefaultProperty useFormData useInitFormEvaluation; do
echo "Checking implementation of $hook:"
fd "$hook.ts" --type f --exec cat {}
done
Length of output: 1643
Script:
#!/bin/bash
# Search for hook files in the UQIEditor directory and its subdirectories
fd -e ts -e tsx "useGoogleSheetsSetDefaultProperty|useFormData|useInitFormEvaluation" app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/
# Search for hook implementations using ast-grep
ast-grep --pattern 'export function use$_($$$) {
$$$
}'
# Search for hook implementations using ripgrep
rg -l "export (const|function) (useGoogleSheetsSetDefaultProperty|useFormData|useInitFormEvaluation)"
Length of output: 1023
Script:
#!/bin/bash
# Let's examine the content of these hook files
echo "=== useGoogleSheetsSetDefaultProperty.ts ==="
cat app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useGoogleSheetsSetDefaultProperty.ts
echo -e "\n=== useFormData.ts ==="
cat app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useFormData.ts
echo -e "\n=== useInitFormEvaluation.ts ==="
cat app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useInitFormEvaluation.ts
Length of output: 3790
24-33: Verify FormRender component's prop types.
The prop passing looks clean, but let's verify the FormRender component's type definitions to ensure type safety.
✅ Verification successful
Props are correctly typed and match the component's interface
The FormRender component's Props interface matches exactly with the props being passed:
editorConfig(any)formData(SaaSAction | QueryAction)formEvaluationState(FormEvalOutput)formName(string)uiComponent(UIComponentTypes)
Note: There is a TODO to fix the any type for editorConfig in the future.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for FormRender component definition and its prop types
ast-grep --pattern 'interface $FormRenderProps {
$$$
}'
# Also check for any existing usage patterns
rg -A 5 '<FormRender'
Length of output: 1643
Script:
#!/bin/bash
# Search for FormRender component definition using TypeScript syntax
ast-grep --pattern 'const FormRender: React.FC<$_> = $_'
# Search for FormRender type definitions
ast-grep --pattern 'type FormRender$_ = {
$$$
}'
# Try finding the component file
fd FormRender.tsx
# Look for imports of FormRender to trace its origin
rg "import.*FormRender.*from"
Length of output: 567
Script:
#!/bin/bash
# Check the FormRender component implementation
cat "app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/FormRender.tsx"
# Look for prop type definitions in the same file
ast-grep --pattern 'interface $_ {
editorConfig?: $_
formData?: $_
formEvaluationState?: $_
formName?: $_
uiComponent?: $_
}'
Length of output: 8665
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/InfoFields.tsx (4)
7-7: LGTM: Clean import of styled components
The migration to a separate styles file follows best practices for better organization and maintainability.
18-18: LGTM: Improved Flex container spacing
The explicit gap spacing using design tokens (spaces-4) improves layout consistency.
19-19: LGTM: Clean request method container implementation
The RequestMethodSelectContainer provides better encapsulation of the dropdown styling.
Also applies to: 30-30
31-31: Verify className usage for testing
The t--dataSourceField className is used for testing purposes. Ensure this change is reflected in the corresponding test files.
Also applies to: 41-41
✅ Verification successful
The previous command failed because the typescript file type wasn't recognized. Let's try with explicit file extensions to find any test files using this className. This will help us verify if the className is being used in tests and needs to be maintained for testing purposes.
Let's try a different approach using fd to first find the files and then search within them. This will help us locate any files using this className, whether they are test files or not, to better understand its usage in the codebase.
Keep the t--dataSourceField className as it's used in Cypress tests
The className is actively used in multiple Cypress test files:
app/client/cypress/e2e/Regression/ClientSide/IDE/Command_Click_Navigation_spec.jsapp/client/cypress/locators/ApiEditor.jsapp/client/cypress/support/Pages/ApiPage.tsapp/client/cypress/locators/apiWidgetslocator.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files using this className
rg -l "t--dataSourceField" --type=typescript --type=javascript
Length of output: 97
Script:
#!/bin/bash
# Search for test files using this className with correct file extensions
rg -l "t--dataSourceField" "*.ts" "*.tsx" "*.js" "*.jsx"
# Also search in Cypress test files specifically
rg -l "t--dataSourceField" "*.cy.ts" "*.cy.tsx" "*.test.ts" "*.test.tsx" "*.spec.ts" "*.spec.tsx"
Length of output: 635
Script:
#!/bin/bash
# Use fd to find all test files first, then search within them
fd -e ts -e tsx -e js -e jsx | xargs rg -l "t--dataSourceField"
# Also check for any file containing this className to understand its usage
rg -l "t--dataSourceField"
Length of output: 685
app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/hooks/useGoogleSheetsSetDefaultProperty.ts (2)
11-19: LGTM! Clean hook setup with proper context usage.
The destructuring and dispatch setup follow React hooks best practices.
11-60: Verify the hook usage across the application.
Since this is a new hook that affects Google Sheets functionality, we should verify its integration points.
✅ Verification successful
Based on the search results, I can generate the final response as the usage pattern is clear:
The hook implementation is properly scoped and safely integrated
The hook is only used in UQIEditorForm.tsx and has no conflicting implementations. The Google Sheets package name check ensures the hook only runs for the intended plugin.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential usage of this hook
rg "useGoogleSheetsSetDefaultProperty"
# Check for other Google Sheets related hooks that might conflict
rg "useGoogleSheets.*"
# Look for components handling Google Sheets actions
ast-grep --pattern 'packageName === PluginPackageName.GOOGLE_SHEETS'
Length of output: 1362
app/client/src/PluginActionEditor/components/PluginActionForm/components/GraphQLEditor/PostBodyData.tsx (2)
46-46: LGTM! Improved prop naming.
The withoutPadding prop is more semantic than isStandalone and better describes its layout purpose.
17-19: Verify the reduced editor height works for typical queries.
The CodeMirror min-height reduction should be tested with common query patterns to ensure adequate editing space.
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/DatasourceConfig.tsx (1)
96-96: Verify layout consistency after removing padding.
The simplified KeyValueFlexContainer might affect layout consistency. Consider whether padding should be handled by the parent component or if this was intentionally removed.
app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx (2)
104-104: LGTM! Consistent use of design system spacing.
The padding addition using CSS variables maintains consistency with the design system.
Line range hint 252-252: Verify form submission behavior with noop handler.
While using noop as a default handler is a common pattern, please verify that no important form submission side effects are being lost.
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/components/EmbeddedDatasourcePathField.tsx (1)
95-95: LGTM! Good use of design system spacing.
Using CSS variables for spacing ensures consistency across the application.
| let evaluationState = {}; | ||
|
|
||
| // Fetching evaluations state only once the formData is populated | ||
| if (!!data) { | ||
| evaluationState = formEvaluation[data.id]; | ||
| } | ||
|
|
||
| return { data, evaluationState }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional logic and evaluation state handling
The double negation is unnecessary, and the evaluation state handling could be more concise.
- let evaluationState = {};
- // Fetching evaluations state only once the formData is populated
- if (!!data) {
- evaluationState = formEvaluation[data.id];
- }
+ const evaluationState = data ? formEvaluation[data.id] ?? {} : {};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let evaluationState = {}; | |
| // Fetching evaluations state only once the formData is populated | |
| if (!!data) { | |
| evaluationState = formEvaluation[data.id]; | |
| } | |
| return { data, evaluationState }; | |
| }; | |
| const evaluationState = data ? formEvaluation[data.id] ?? {} : {}; | |
| return { data, evaluationState }; | |
| }; |
🧰 Tools
🪛 Biome
[error] 17-17: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
| // @ts-expect-error: Types are not available | ||
| const actionObjectDiff: undefined | Diff<Action | undefined, Action>[] = | ||
| diff(action, initialValues); |
There was a problem hiding this comment.
Fix TypeScript type definition instead of suppressing the error.
The @ts-expect-error suggests a missing or incorrect type definition. Consider properly typing the Action interface.
-// @ts-expect-error: Types are not available
-const actionObjectDiff: undefined | Diff<Action | undefined, Action>[] =
+interface ActionType {
+ id: string;
+ // Add other required properties
+}
+const actionObjectDiff: undefined | Diff<ActionType | undefined, ActionType>[] =Committable suggestion skipped: line range outside the PR's diff.
| // @ts-expect-error: Types are not available | ||
| const actionObjectDiff: undefined | Diff<Action | undefined, Action>[] = | ||
| diff(action, initialValues); | ||
|
|
||
| const { path = "", value = "" } = { | ||
| ...getPathAndValueFromActionDiffObject(actionObjectDiff), | ||
| }; | ||
|
|
||
| if (value && path) { | ||
| dispatch( | ||
| setActionProperty({ | ||
| actionId: action.id, | ||
| propertyName: path, | ||
| value, | ||
| }), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for the diff operation.
The diff operation could potentially fail, but there's no error handling in place.
-const actionObjectDiff: undefined | Diff<Action | undefined, Action>[] =
- diff(action, initialValues);
+let actionObjectDiff: undefined | Diff<Action | undefined, Action>[] | null = null;
+try {
+ actionObjectDiff = diff(action, initialValues);
+} catch (error) {
+ console.error('Failed to compute differences:', error);
+ return;
+}📝 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.
| // @ts-expect-error: Types are not available | |
| const actionObjectDiff: undefined | Diff<Action | undefined, Action>[] = | |
| diff(action, initialValues); | |
| const { path = "", value = "" } = { | |
| ...getPathAndValueFromActionDiffObject(actionObjectDiff), | |
| }; | |
| if (value && path) { | |
| dispatch( | |
| setActionProperty({ | |
| actionId: action.id, | |
| propertyName: path, | |
| value, | |
| }), | |
| ); | |
| } | |
| } | |
| // @ts-expect-error: Types are not available | |
| let actionObjectDiff: undefined | Diff<Action | undefined, Action>[] | null = null; | |
| try { | |
| actionObjectDiff = diff(action, initialValues); | |
| } catch (error) { | |
| console.error('Failed to compute differences:', error); | |
| return; | |
| } | |
| const { path = "", value = "" } = { | |
| ...getPathAndValueFromActionDiffObject(actionObjectDiff), | |
| }; | |
| if (value && path) { | |
| dispatch( | |
| setActionProperty({ | |
| actionId: action.id, | |
| propertyName: path, | |
| value, | |
| }), | |
| ); | |
| } | |
| } |
| merge( | ||
| initialValues, | ||
| getConfigInitialValues(editorConfig as Record<string, unknown>[]), | ||
| ); | ||
|
|
||
| merge( | ||
| initialValues, | ||
| getConfigInitialValues(settingsConfig as Record<string, unknown>[]), | ||
| ); | ||
|
|
||
| // initialValues contains merge of action, editorConfig, settingsConfig and will be passed to redux form | ||
| merge(initialValues, action); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid object mutation in merge operations.
The current implementation mutates the initialValues object during merge operations. Consider using a more functional approach.
-const initialValues = {};
-merge(
- initialValues,
- getConfigInitialValues(editorConfig as Record<string, unknown>[]),
-);
-merge(
- initialValues,
- getConfigInitialValues(settingsConfig as Record<string, unknown>[]),
-);
-merge(initialValues, action);
+const initialValues = merge(
+ {},
+ getConfigInitialValues(editorConfig as Record<string, unknown>[]),
+ getConfigInitialValues(settingsConfig as Record<string, unknown>[]),
+ 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| merge( | |
| initialValues, | |
| getConfigInitialValues(editorConfig as Record<string, unknown>[]), | |
| ); | |
| merge( | |
| initialValues, | |
| getConfigInitialValues(settingsConfig as Record<string, unknown>[]), | |
| ); | |
| // initialValues contains merge of action, editorConfig, settingsConfig and will be passed to redux form | |
| merge(initialValues, action); | |
| const initialValues = merge( | |
| {}, | |
| getConfigInitialValues(editorConfig as Record<string, unknown>[]), | |
| getConfigInitialValues(settingsConfig as Record<string, unknown>[]), | |
| action | |
| ); |
| tab == "HEADERS" | ||
| ? headersCount | ||
| : tab == "PARAMS" | ||
| ? paramsCount | ||
| : undefined |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use strict equality comparison
While this logic will be extracted in a future PR (per previous discussions), let's fix the equality comparisons for now.
- tab == "HEADERS"
+ tab === "HEADERS"
- : tab == "PARAMS"
+ : tab === "PARAMS"📝 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.
| tab == "HEADERS" | |
| ? headersCount | |
| : tab == "PARAMS" | |
| ? paramsCount | |
| : undefined | |
| tab === "HEADERS" | |
| ? headersCount | |
| : tab === "PARAMS" | |
| ? paramsCount | |
| : undefined |
| <Tabs | ||
| onValueChange={onValueChange} | ||
| style={{ | ||
| height: "calc(100% - 36px)", | ||
| overflow: "hidden", | ||
| maxHeight: "unset", | ||
| }} | ||
| value={value} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move inline styles to a styled component
Inline styles reduce maintainability and reusability. Consider extracting these styles into a styled component.
+const StyledTabs = styled(Tabs)`
+ height: calc(100% - 36px);
+ overflow: hidden;
+ max-height: unset;
+`;
- <Tabs
- onValueChange={onValueChange}
- style={{
- height: "calc(100% - 36px)",
- overflow: "hidden",
- maxHeight: "unset",
- }}
- value={value}
- >
+ <StyledTabs
+ onValueChange={onValueChange}
+ value={value}
+ >Committable suggestion skipped: line range outside the PR's diff.
# Conflicts: # app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx
## Description Adds the Query and Google Sheets initialisation based on Google Sheets ref: https://github.com/appsmithorg/appsmith/blob/release/app/client/src/pages/Editor/QueryEditor/Editor.tsx#L292-L309 https://github.com/appsmithorg/appsmith/blob/release/app/client/src/pages/Editor/QueryEditor/Editor.tsx#L148-L156 Query ref: https://github.com/appsmithorg/appsmith/blob/release/app/client/src/pages/Editor/QueryEditor/Editor.tsx#L131-L137 > [!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.Datasource" ## 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** - Introduced a new `withoutPadding` property for better control over section padding. - Added support for binary data uploads in the `PostBodyData` component. - New styled components created for enhanced layout management. - `httpsMethods` property added to the `CommonFormPropsWithExtraParams` interface. - **Bug Fixes** - Improved tooltip clarity for invalid data in the `ImportedKeyValue` component. - **Refactor** - Simplified component structures and styling, enhancing layout management across several components. - **Chores** - Removed unused imports and streamlined component logic in the `UQIEditorForm`. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Adds the Query and Google Sheets initialisation based on
Google Sheets ref:
https://github.com/appsmithorg/appsmith/blob/release/app/client/src/pages/Editor/QueryEditor/Editor.tsx#L292-L309
https://github.com/appsmithorg/appsmith/blob/release/app/client/src/pages/Editor/QueryEditor/Editor.tsx#L148-L156
Query ref:
https://github.com/appsmithorg/appsmith/blob/release/app/client/src/pages/Editor/QueryEditor/Editor.tsx#L131-L137
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.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11683280374
Commit: 1e40efa
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Tue, 05 Nov 2024 12:59:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
withoutPaddingproperty for better control over section padding.PostBodyDatacomponent.httpsMethodsproperty added to theCommonFormPropsWithExtraParamsinterface.Bug Fixes
ImportedKeyValuecomponent.Refactor
Chores
UQIEditorForm.