fix: API Body format focus retention #37150
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Assessment against linked issues
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
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: 3
🧹 Outside diff range and nitpick comments (3)
app/client/src/navigation/FocusElements.ts (1)
6-6: Consider standardizing enum member naming pattern.While the new member works correctly, consider establishing a consistent naming pattern for form-related focus elements. Currently we have mixed patterns:
PluginActionFormDatavsInputField,PropertyField.Consider one of these approaches:
- Prefix pattern:
Form_PluginAction- Consistent suffixes:
PluginActionField(matchingInputField)app/client/src/PluginActionEditor/store/pluginEditorReducer.ts (1)
150-152: Consider adding type assertion for values.The reducer logic is correct, but consider adding a type assertion to ensure values match the expected structure.
- const { values } = action.payload; + const { values }: { values: PluginActionEditorState['formData'] } = action.payload;app/client/src/sagas/ApiPaneSagas.ts (1)
Line range hint
141-227: Consider adding error handling for invalid content types.While the function handles the happy path well, it could benefit from proper error handling when an invalid content type is encountered.
Consider adding a try-catch block:
function* handleUpdateBodyContentType(action: ReduxAction<{ title: string }>) { + try { const { title } = action.payload; const { values } = yield select(getFormData, API_EDITOR_FORM_NAME); const displayFormatValue = POST_BODY_FORMAT_OPTIONS_ARRAY.find( (el) => el === title, ); if (!displayFormatValue) { - log.error("Display format not supported", title); + throw new Error(`Display format not supported: ${title}`); return; } // ... rest of the function + } catch (error) { + yield put({ + type: ReduxActionErrorTypes.API_UPDATE_BODY_CONTENT_TYPE_ERROR, + payload: { + error, + }, + }); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx(3 hunks)app/client/src/PluginActionEditor/store/pluginActionEditorActions.ts(1 hunks)app/client/src/PluginActionEditor/store/pluginActionEditorSelectors.ts(2 hunks)app/client/src/PluginActionEditor/store/pluginEditorReducer.ts(2 hunks)app/client/src/ce/navigation/FocusElements/AppIDE.ts(2 hunks)app/client/src/navigation/FocusElements.ts(1 hunks)app/client/src/sagas/ApiPaneSagas.ts(5 hunks)
🔇 Additional comments (12)
app/client/src/PluginActionEditor/store/pluginActionEditorActions.ts (2)
36-40: LGTM: Well-structured new action creator.
The new setExtraFormData action creator is well-typed and follows Redux best practices with a clear payload structure.
31-34: Verify the removal of apiId parameter impact.
The removal of apiId from the action payload aligns with the PR objective of removing redundant Action ID. However, we should verify this doesn't break existing consumers.
app/client/src/navigation/FocusElements.ts (1)
6-6: LGTM! Verify usage in related components.
The new enum member aligns well with the PR's focus retention objective.
Let's verify the usage of this new focus element:
✅ Verification successful
Implementation verified and properly integrated
The new PluginActionFormData focus element is correctly implemented in the Query entity configuration with:
- Proper Redux integration using
getFormDataselector andsetExtraFormDatasetter - Default value set to handle POST body format
- Consistent placement alongside other plugin action related focus elements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using the new focus element
rg "PluginActionFormData" --type ts
Length of output: 227
Script:
#!/bin/bash
# Let's check the implementation details in AppIDE.ts
cat app/client/src/ce/navigation/FocusElements/AppIDE.ts
# Also check for any other potential usages in action/plugin related files
rg "PluginActionFormData" -B 2 -A 2 --type ts
Length of output: 10443
app/client/src/PluginActionEditor/store/pluginActionEditorSelectors.ts (2)
5-5: LGTM!
The import follows the established pattern and is appropriately sourced from common constants.
44-47: LGTM!
The type definition is clear and properly defines the expected shape of the post body format.
app/client/src/PluginActionEditor/store/pluginEditorReducer.ts (1)
29-29: Verify impact of formData type change.
The simplified type structure aligns with the PR objective of removing Action ID dependency.
Let's verify all usages of this type to ensure compatibility:
app/client/src/ce/navigation/FocusElements/AppIDE.ts (2)
80-81: LGTM: Import statements are well-organized.
The new imports support form data retention functionality as intended.
Also applies to: 86-89
160-166: Verify form data persistence across navigation.
The focus element configuration looks good and aligns with the focus retention requirements. Let's verify the implementation works as expected.
app/client/src/sagas/ApiPaneSagas.ts (3)
64-67: LGTM! Clean import restructuring.
The imports are well-organized and align with the transition from Redux connect to hooks.
364-368: LGTM! Consistent state management.
The update to use setExtraFormData is consistent with the new form data management approach.
257-258: Verify type safety of getPostBodyFormat selector.
The type inference for extraFormData relies on the selector's return type. Let's verify its implementation.
app/client/src/PluginActionEditor/components/PluginActionForm/components/ApiEditor/PostBodyData.tsx (1)
Line range hint 1-192: Good work on refactoring to use React hooks
Transitioning from connect to React hooks (useSelector, useDispatch) improves the readability and maintainability of the component.
| export const getFormData = (state: AppState) => | ||
| state.ui.pluginActionEditor.formData; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider creating memoized selectors for specific form data fields.
Direct state access might cause unnecessary re-renders in components that only need specific parts of formData.
Consider creating specific selectors using createSelector:
export const getSpecificFormData = (key: string) =>
createSelector([getFormData], (formData) => formData[key]);| export const getPostBodyFormat: GetFormPostBodyFormat = (state) => { | ||
| const formData = getFormData(state); | ||
|
|
||
| if (id in formData) { | ||
| return formData[id][POST_BODY_FORM_DATA_KEY]; | ||
| if (POST_BODY_FORM_DATA_KEY in formData) { | ||
| return formData[POST_BODY_FORM_DATA_KEY]; | ||
| } | ||
|
|
||
| return undefined; | ||
| return { | ||
| label: POST_BODY_FORMAT_OPTIONS.NONE, | ||
| value: POST_BODY_FORMAT_OPTIONS.NONE, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Memoize the selector to prevent unnecessary recalculations.
While the implementation is correct, it should be memoized to prevent unnecessary recalculations.
Consider this implementation:
export const getPostBodyFormat: GetFormPostBodyFormat = createSelector(
[getFormData],
(formData) => {
if (POST_BODY_FORM_DATA_KEY in formData) {
return formData[POST_BODY_FORM_DATA_KEY];
}
return {
label: POST_BODY_FORMAT_OPTIONS.NONE,
value: POST_BODY_FORMAT_OPTIONS.NONE,
};
}
);| defaultValue={postBodyFormat.value} | ||
| onSelect={updateBodyContentType} | ||
| value={postBodyFormat.value} |
There was a problem hiding this comment.
Remove redundant defaultValue prop in Select component
Using both defaultValue and value in a controlled component can cause confusion and potential bugs. Since value is already managing the selected option, defaultValue is unnecessary and should be removed.
Apply this diff to remove the redundant prop:
<Select
data-testid="t--api-body-tab-switch"
- defaultValue={postBodyFormat.value}
onSelect={updateBodyContentType}
value={postBodyFormat.value}
>📝 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.
| defaultValue={postBodyFormat.value} | |
| onSelect={updateBodyContentType} | |
| value={postBodyFormat.value} | |
| value={postBodyFormat.value} | |
| onSelect={updateBodyContentType} | |
| value={postBodyFormat.value} |
## Description - Use Focus retention to store user context of the POST body format of APIs - Update component to use hooks instead of redux connect hoc - remove need of passing Action ID in form data as it is redundant for focus retention states Fixes appsmithorg#36984 ## Automation /ok-to-test tags="@tag.Datasource, @tag.IDE" ### 🔍 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/11609483418> > Commit: a3a1a59 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11609483418&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource, @tag.IDE` > Spec: > <hr>Thu, 31 Oct 2024 10:31:09 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 ## Release Notes - **New Features** - Introduced a streamlined method for managing API body content types and form data. - Added new action creator `setExtraFormData` for enhanced form data handling. - **Improvements** - Simplified the `PostBodyData` component by utilizing React hooks for state management. - Updated selectors and reducers to support a flatter structure for form data, improving clarity and maintainability. - **Updates** - Enhanced focus elements configuration to better manage plugin action form data within the application. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #36984
Automation
/ok-to-test tags="@tag.Datasource, @tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11609483418
Commit: a3a1a59
Cypress dashboard.
Tags:
@tag.Datasource, @tag.IDESpec:
Thu, 31 Oct 2024 10:31:09 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
setExtraFormDatafor enhanced form data handling.Improvements
PostBodyDatacomponent by utilizing React hooks for state management.Updates