fix: Issue with Api Body content type selection #38597
Conversation
WalkthroughThis pull request involves significant refactoring of the plugin action editor's state management and form handling. The changes primarily focus on removing Redux-related form data management functions, eliminating specific action types, and simplifying the component's approach to handling post body formats. The modifications streamline the state management by removing explicit dispatching and selector functions, moving towards a more form-state-driven approach. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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
Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/client/src/sagas/ApiPaneSagas.ts (2)
131-135: Verify error handling for unsupported content typesThe error logging for unsupported content types could be enhanced to provide more context for debugging.
Consider this improvement:
- log.error("Display format not supported", contentType); + log.error(`Content type "${contentType}" is not supported. Expected one of: ${POST_BODY_FORMAT_OPTIONS_ARRAY.join(", ")}`);Also applies to: 139-139
267-286: Simplify content type validation logicThe nested if conditions for content type validation could be simplified.
Consider this more concise approach:
- if (apiContentType) { - if (Object.values(POST_BODY_FORMAT_OPTIONS).includes(apiContentType)) { - displayFormat = apiContentType; - } else { - displayFormat = POST_BODY_FORMAT_OPTIONS.RAW; - } - } else { - displayFormat = POST_BODY_FORMAT_OPTIONS.NONE; - } + displayFormat = apiContentType + ? Object.values(POST_BODY_FORMAT_OPTIONS).includes(apiContentType) + ? apiContentType + : POST_BODY_FORMAT_OPTIONS.RAW + : POST_BODY_FORMAT_OPTIONS.NONE;app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx (1)
Line range hint
34-38: Consider removing empty CSS rules.The empty CSS rules can be removed to improve code cleanliness.
- .ads-v2-select { - max-width: 250px; - width: 100%; - } - /* margin: 0 30px; - width: 65%; */Also applies to: 44-44
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx(5 hunks)app/client/src/PluginActionEditor/store/pluginActionEditorActions.ts(0 hunks)app/client/src/PluginActionEditor/store/pluginActionEditorSelectors.ts(0 hunks)app/client/src/PluginActionEditor/store/pluginEditorReducer.ts(0 hunks)app/client/src/ce/constants/ReduxActionConstants.tsx(0 hunks)app/client/src/ce/navigation/FocusElements/AppIDE.ts(1 hunks)app/client/src/components/editorComponents/form/fields/DropdownFieldWrapper.tsx(1 hunks)app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx(1 hunks)app/client/src/navigation/FocusElements.ts(0 hunks)app/client/src/sagas/ApiPaneSagas.ts(4 hunks)
💤 Files with no reviewable changes (5)
- app/client/src/navigation/FocusElements.ts
- app/client/src/ce/constants/ReduxActionConstants.tsx
- app/client/src/PluginActionEditor/store/pluginActionEditorSelectors.ts
- app/client/src/PluginActionEditor/store/pluginActionEditorActions.ts
- app/client/src/PluginActionEditor/store/pluginEditorReducer.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (7)
app/client/src/sagas/ApiPaneSagas.ts (2)
55-55: LGTM: New utility importsThe addition of
parseUrlForQueryParamsandqueryParamsRegExutilities is appropriate for URL parsing functionality.
309-312: LGTM: Simplified content type update handlingThe direct call to
handleUpdateBodyContentTypewith the payload is a cleaner approach than passing the entire action object.app/client/src/ce/navigation/FocusElements/AppIDE.ts (2)
84-84: LGTM: Import change aligns with the refactoring.The import of
API_EDITOR_TABSis correctly maintained as it's still used in the REST API plugin configuration.
Line range hint
1-275: Verify the impact of form data management removal.The removal of form data related configurations suggests a significant architectural change in how plugin action form state is managed. Please ensure that:
- All form state is properly handled in the new implementation
- No regressions in the API body content type selection functionality
Run this script to verify the form state management implementation:
✅ Verification successful
Form data management migration verified successfully
The form state management has been properly migrated to dedicated components under PluginActionEditor with maintained API body content type functionality. The removal from AppIDE.ts is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for new form state management implementation echo "Searching for new form state management..." rg -l "formData|setFormData|useForm" "app/client/src/PluginActionEditor" # Check for any remaining references to old form data management echo "Checking for remnant references..." rg -l "getFormData|setExtraFormData|POST_BODY_FORMAT_OPTIONS" "app/client/src"Length of output: 1937
app/client/src/components/editorComponents/form/fields/DropdownFieldWrapper.tsx (1)
43-43: LGTM! Verify dropdown behavior after key prop change.The change from
option.idtooption.valuefor the key prop is correct, as it ensures unique keys for React's reconciliation.Run this script to verify no duplicate values exist that could cause key conflicts:
✅ Verification successful
Change from option.id to option.value is safe due to built-in validation
The codebase enforces unique values in dropdown options through validation, making option.value a reliable choice for React's key prop.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for dropdown option definitions to verify unique values ast-grep --pattern 'options = [ $$$ ]'Length of output: 342
Script:
#!/bin/bash # Search for dropdown option type definitions and usage ast-grep --pattern 'type $_ = { value: $_ $$$ }' # Search for direct option definitions rg -U "options.*=.*\[.*\]" -A 5 # Search specifically for DropdownFieldWrapper usage rg -U "DropdownFieldWrapper" -A 10Length of output: 126568
app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx (1)
93-95: LGTM! Clean implementation of conditional rendering.The conditional rendering based on
showLabelOnlyprop provides better control over the dropdown display.app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx (1)
176-182: Verify SelectField integration with form state.The switch from Select to SelectField component changes how the form state is managed. Ensure that:
- Form initialization works correctly
- Value changes are properly propagated
- Default selections are handled appropriately
✅ Verification successful
SelectField integration is properly implemented
The component correctly integrates with form state through redux-form, handles value propagation, and manages default selections appropriately. No issues found with the form state management implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SelectField usage patterns and initialization ast-grep --pattern 'SelectField name="actionConfiguration.formData.apiContentType" $$$'Length of output: 93
Script:
#!/bin/bash # Search for SelectField component definition and usages echo "=== SelectField Component Definition ===" ast-grep --pattern 'export $_ SelectField = $_' echo -e "\n=== SelectField Usages ===" rg -l "SelectField" --type ts --type tsx echo -e "\n=== Form Initialization and Tests ===" rg -l "actionConfiguration.formData.apiContentType" --type ts --type tsx echo -e "\n=== Test Files for PostBodyData ===" fd "PostBodyData.test" -e tsx -e tsLength of output: 77308
Script:
#!/bin/bash # Search for SelectField component and form state management echo "=== SelectField Component Usage ===" rg -t typescript -t javascriptreact -t typescriptreact "SelectField.*name=\"actionConfiguration.formData.apiContentType\"" -A 2 -B 2 echo -e "\n=== Form Initialization ===" rg -t typescript -t javascriptreact -t typescriptreact "formData.apiContentType" -A 2 -B 2 echo -e "\n=== Test Coverage ===" rg -t typescript -t javascriptreact -t typescriptreact "test.*SelectField.*apiContentType" -A 2 -B 2 echo -e "\n=== Related Files ===" fd "PostBodyData" -e tsx -e tsLength of output: 936
Script:
#!/bin/bash echo "=== PostBodyData.tsx Content ===" cat "app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx" echo -e "\n=== Looking for Form Context ===" rg --type-add 'ts:*.{ts,tsx}' -t ts "FormProvider|useFormContext|initialValues.*apiContentType" -A 2 -B 2Length of output: 25555
| const formValues = useSelector(getFormValues(API_EDITOR_FORM_NAME)) as Action; | ||
| const postBodyFormat = get( | ||
| formValues, | ||
| "actionConfiguration.formData.apiContentType", | ||
| POST_BODY_FORMAT_OPTIONS.NONE, | ||
| ); |
There was a problem hiding this comment.
Add null check for form values retrieval.
The form values retrieval could return undefined. Consider adding a null check or providing a fallback.
- const formValues = useSelector(getFormValues(API_EDITOR_FORM_NAME)) as Action;
+ const formValues = useSelector(getFormValues(API_EDITOR_FORM_NAME)) as 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.
| const formValues = useSelector(getFormValues(API_EDITOR_FORM_NAME)) as Action; | |
| const postBodyFormat = get( | |
| formValues, | |
| "actionConfiguration.formData.apiContentType", | |
| POST_BODY_FORMAT_OPTIONS.NONE, | |
| ); | |
| const formValues = useSelector(getFormValues(API_EDITOR_FORM_NAME)) as Action || {}; | |
| const postBodyFormat = get( | |
| formValues, | |
| "actionConfiguration.formData.apiContentType", | |
| POST_BODY_FORMAT_OPTIONS.NONE, | |
| ); |
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12742357822. |
|
Deploy-Preview-URL: https://ce-38597.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/components/editorComponents/form/fields/SelectField.tsx (1)
Line range hint
1-65: Consider adding JSDoc documentation for the testId prop.While the implementation is correct, adding JSDoc documentation for the testId property would improve component documentation.
interface SelectFieldProps { // ... existing props ... + /** Test identifier for component testing purposes */ testId?: string; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx(5 hunks)app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx(3 hunks)app/client/src/components/editorComponents/form/fields/SelectField.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx
- app/client/src/components/editorComponents/form/fields/DropdownWrapper.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/src/components/editorComponents/form/fields/SelectField.tsx (2)
39-39: LGTM! Good addition of test identifier support.The optional testId property follows TypeScript best practices and enhances component testability.
62-62: LGTM! Proper prop forwarding implementation.The testId prop is correctly forwarded to the Field component, maintaining consistent prop handling patterns.
hetunandu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/cypress/support/Pages/ApiPage.ts (1)
70-70: Good move on removing XPath, but let's make it more robust.While replacing XPath with jQuery selector is a step in the right direction, we can further improve it:
- Consider using a data-* attribute for more reliable selection
- Use template literals for cleaner string interpolation
- ".rc-select-item-option:contains(" + subTab + ")"; + `[data-testid="t--api-body-type-${subTab}"]`;Remember to add the corresponding data-testid attribute to the component.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/support/Pages/ApiPage.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/ApiPage.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.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
Description
Simplifies the API body content type selection
Earlier it used to maintain a different state maintain the current body type,
now we are using the data field of the API Action object itself to control the selection of body type
There two sync behaviors associated with this field that has been updated:
JSONwhen the request type is POST or PUTRemoves Focus retention for body content type as it was no longer needed
Fixes #38066
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/12745533006
Commit: 2a3a7e3
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Mon, 13 Jan 2025 11:33:31 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Refactor
Chores
New Features
testIdprop to dropdown and select components for improved testing.