chore: [Plugin Action Form] Common Editor State #36512
Conversation
WalkthroughThe changes involve significant refactoring across several components in the PluginActionEditor. Key modifications include the removal of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
|
This PR has increased the number of cyclic dependencies by 5, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![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 (9)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/index.ts (1)
1-2: Well done, class! Your code is neat and tidy.I'm pleased to see you've organized your utility functions so nicely. It's like having a well-arranged pencil case – everything in its proper place!
However, let's not forget the importance of good documentation. It's like writing notes in your exercise book for future reference. Consider adding JSDoc comments to explain what these functions do. It'll help your classmates (I mean, fellow developers) understand your work better.
Here's a gold star for your efforts, but remember: there's always room for improvement!
Let's add some JSDoc comments to make our code even better:
/** Retrieves the count of headers */ export { default as getHeadersCount } from "./getHeadersCount"; /** Retrieves the count of parameters */ export { default as getParamsCount } from "./getParamsCount";app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/getParamsCount.ts (1)
4-12: Excellent work on your function, students!Your
getParamsCountfunction is well-implemented and follows good coding practices. I'm particularly impressed with your use of optional parameters and thegetValidPropertiesfunction for input validation.However, let's consider a small improvement to make your code even better:
Consider adding a type annotation for the return value. It would look like this:
function getParamsCount( actionParams?: Property[], datasourceParams?: Property[], -) { +): number { const validActionParams = getValidProperties(actionParams); const validDatasourceParams = getValidProperties(datasourceParams); return validActionParams.length + validDatasourceParams.length; }This makes the function's purpose even clearer to anyone reading the code. Remember, clear code is happy code!
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/getHeadersCount.ts (1)
4-8: A gold star for your function signature!Your
getHeadersCountfunction is well-structured with optional parameters. However, let's add a little homework:Consider adding a JSDoc comment to explain the purpose of the function and describe the parameters. Here's an example:
/** * Calculates the total count of valid headers from different sources. * @param actionHeaders - Headers from the action * @param datasourceHeaders - Headers from the datasource * @param autoGeneratedActionHeaders - Auto-generated headers * @returns The total count of valid headers */ function getHeadersCount( // ... rest of the function signatureapp/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.ts (1)
17-18: Well done optimizing the effect, but let's make it even better!Class, gather 'round! This is a prime example of optimization. By adding this condition, we ensure we only do work when necessary. It's like only grading papers that have new answers!
However, I have a small suggestion to make this even more efficient:
Consider using Object.is() for a more precise comparison:
- if (prevActionId === action.id) return; + if (Object.is(prevActionId, action.id)) return;This ensures that we catch even the trickiest of changes. Remember, in programming, precision is our friend!
Also applies to: 36-36
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (1)
43-44: Good job updating the RequestTabs props!Class, let's examine how we've updated the props for our
RequestTabscomponent. The new values from our custom hook are being passed down correctly. This is a great improvement in our code structure!However, I have a small suggestion to make our code even better:
For consistency, let's update the prop names in the
RequestTabscomponent to match the names we're using from the hook. For example:- actionConfigurationHeaders={actionHeaders} - actionConfigurationParams={actionParams} + actionHeaders={actionHeaders} + actionParams={actionParams}This will make our code even easier to read and understand. Remember, consistency is key in writing clean, maintainable code!
Also applies to: 46-49
app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.test.tsx (1)
112-142: Excellent work on the new test case, but let's add a small improvement!Students, gather around! This new test case is a shining example of thorough unit testing. It covers multiple scenarios and ensures our
useChangeActionCallhook behaves correctly. However, to make it even better, let's add one more assertion:Consider adding an assertion to check the parameters of the second
changeApicall:expect(changeApi).toHaveBeenNthCalledWith(2, "actionId2", false);This will verify not only that
changeApiwas called twice, but also that it was called with the correct parameters on the second invocation.app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx (2)
36-37: Excellent work on improving type safety!Students, take note of this exemplary use of specific types. By changing from
anytoProperty[], we've made our code more robust and easier to understand. This is like using a ruler instead of guessing when measuring - much more precise!However, to make this even better, consider adding a brief comment explaining what a
Propertyis. Remember, good documentation is like leaving breadcrumbs for future explorers of your code!Also applies to: 41-42
67-86: Bravo on improving tab rendering flexibility!Students, observe how this code now adapts to different scenarios. By filtering tabs based on the
showSettingsprop, we're making our component more versatile - it's like a Swiss Army knife of tab rendering!The use of
headersCountandparamsCountfor notifications is spot on, ensuring consistency with our earlier changes. Well done!However, let's not forget our future plans. As per our previous lesson, we'll be extracting this tab rendering logic into separate components. When we do that, make sure to keep this conditional rendering in mind. It's like planning for a group project - think ahead to make the future work smoother!
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts (1)
38-38: Simplify code by removing unnecessary optional chainingSince we've confirmed that
datasourceFromActionexists and has anid, we can simplify the code by removing the unnecessary optional chaining ondatasourceFromAction?.idin line 38.Apply this diff:
-datasourceFromAction = datasources.find( - (d) => d.id === datasourceFromAction?.id, -); +datasourceFromAction = datasources.find( + (d) => d.id === datasourceFromAction.id, +);
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (13)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/APIEditorForm.tsx (0 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (3 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx (3 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/getHeadersCount.ts (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/getParamsCount.ts (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/getValidProperties.ts (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/index.ts (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.test.tsx (3 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.ts (2 hunks)
- app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx (0 hunks)
- app/client/src/pages/Editor/APIEditor/GraphQL/GraphQLEditorForm.tsx (0 hunks)
- app/client/src/pages/Editor/APIEditor/RestAPIForm.tsx (0 hunks)
Files not reviewed due to no reviewable changes (4)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/APIEditorForm.tsx
- app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx
- app/client/src/pages/Editor/APIEditor/GraphQL/GraphQLEditorForm.tsx
- app/client/src/pages/Editor/APIEditor/RestAPIForm.tsx
Additional context used
Learnings (1)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx (1)
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.
Biome
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 36-36: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
Additional comments not posted (16)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/getValidProperties.ts (3)
1-1: Good job on importing the necessary type!Class, I'm pleased to see you've imported the
Propertytype from the correct module. This shows attention to detail and good TypeScript practices. Keep up the good work!
11-11: A gold star for your export statement!Excellent choice, class! Using a default export for this utility function is just right. It's like you've packaged your homework neatly for the teacher to grade. This will make it very easy for your classmates (other developers) to use this function in their own code.
1-11: Overall assessment: A stellar performance!Class, I'm thoroughly impressed with your work on this utility function. It's clear that you've been paying attention in our lessons on TypeScript and clean code practices.
This
getValidPropertiesfunction will be an excellent addition to your Plugin Action Form toolkit. It aligns perfectly with our lesson plan (PR objectives) by helping to process form states efficiently. By filtering out invalid properties, you're ensuring that only the most studious data makes it through to the next stage of your application.Remember, in the grand classroom of code, every little utility function like this one contributes to a more organized and efficient learning environment. Keep up the fantastic work!
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/getParamsCount.ts (3)
1-2: Well done on your import statements, class!You've shown a good understanding of TypeScript practices by importing the
Propertytype. And importinggetValidPropertiesfrom a local file demonstrates good code organization. Keep up the excellent work!
1-14: A gold star for relevance, dear students!Your
getParamsCountfunction is a shining example of how to meet project objectives. It perfectly aligns with our lesson plan of enhancing the Common Editor form. By providing a way to calculate the total number of valid parameters, you're contributing to the larger goal of correctly passing necessary states.This helper function will be invaluable in our
RequestTabscomponents, allowing us to remove similar functionality from the original implementation. Well done on following the project roadmap!
1-14: Class dismissed with flying colors!Your work on this file is commendable. The
getParamsCountfunction is well-implemented, aligns perfectly with our project goals, and demonstrates good coding practices. With just a small tweak to add a return type annotation, this file will be a model example for your classmates.Remember, in the world of coding, there's always room for improvement, but you've set a high bar with this implementation. Keep up the excellent work!
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/utils/getHeadersCount.ts (2)
1-2: Well done on your imports, class!You've shown good judgment in importing the necessary utilities and types. The
getValidPropertiesfunction will be a helpful tool in our lesson on header counting.
22-22: A perfect export statement!You've correctly used the default export for your single-function module. This will make it easy for your classmates to import and use this function in their own work.
app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.ts (2)
7-7: Good job importing the necessary hook!Class, let's take a moment to appreciate the proper import of the
usePrevioushook. This is an excellent example of using the right tools for the job. Remember, children, organization is key in coding!
11-11: Excellent use of theusePrevioushook!Children, pay attention to this line. Here, we're using the
usePrevioushook to keep track of the previousaction.id. This is like leaving breadcrumbs in your code to remember where you've been. Very clever!app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/CommonEditorForm.tsx (2)
8-8: Excellent addition of a custom hook!Class, let's take a moment to appreciate this well-crafted import statement. The new custom hook
useGetFormActionValueshas been properly imported. This hook will help us streamline our code and make it more efficient. Remember, good developers always strive to keep their code organized and reusable!
23-29: A+ for implementing the custom hook!Students, pay attention to this excellent use of our new custom hook. By using
useGetFormActionValues, we've simplified our component and made it much cleaner. Notice how we've reduced the number of props we need to pass down? This is a great example of how custom hooks can help us organize our logic and make our components more focused. Well done!app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.test.tsx (1)
7-7: Well done on importing and mocking theusePrevioushook!Class, let's take a moment to appreciate the good practice demonstrated here. The developer has not only imported the
usePrevioushook but also properly mocked it for our test environment. This shows a thorough understanding of isolated testing principles.Also applies to: 26-27
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/RequestTabs.tsx (3)
14-14: Good job importing utility functions!Class, let's give a round of applause for this excellent addition. By importing
getHeadersCountandgetParamsCount, we're keeping our code tidy and following the golden rule of "Don't Repeat Yourself" (DRY). This is a step in the right direction for better code organization.
52-61: A+ for centralizing count calculations!Class, this is a prime example of code refactoring. By using the
getHeadersCountandgetParamsCountfunctions, we've simplified our component and made it easier to maintain. It's like cleaning up your desk - everything has its place, and you can find what you need more quickly.This change not only makes the code cleaner but also reduces the chance of errors. Remember, in programming as in life, less clutter leads to fewer mistakes!
Line range hint
1-150: Outstanding work on enhancing the RequestTabs component!Class, let's recap the valuable lessons we've learned from these changes:
- We've improved type safety by using specific types instead of
any.- We've centralized our counting logic, making our code more maintainable.
- We've made our tab rendering more flexible, adapting to different scenarios.
These changes are like upgrading from a bicycle to a car - we're moving faster, more efficiently, and with greater control!
To ensure everything is working as expected, let's do a quick pop quiz:
Remember, thorough testing is key to ensuring our code works as intended. Keep up the excellent work!
Verification successful
It looks like our previous attempts encountered an issue because
rgdidn’t recognize thetypescriptfile type. No worries! Let’s adjust our approach to effectively verify the necessary details.Let's give this a try to ensure everything is in order. Keep up the great work!
Excellent work, class! Let's review what we've learned from our code investigation:
Great job on implementing those new utility functions and improving type safety!
You've made some wonderful progress:
- The new
getHeadersCountandgetParamsCountfunctions are being used in multiple files. That's like using your new vocabulary words in different sentences - well done!- The
Propertytype from 'entities/Action' is being imported and used correctly. You're really starting to understand the importance of types!However, we still have some homework to do:
3. There are quite a few 'any' types scattered throughout the codebase. Remember, using 'any' is like writing "whatever" on a math test - it works, but it doesn't show what you really know!For your next assignment, I'd like you to focus on replacing those 'any' types with more specific types wherever possible. This will make our code even safer and easier to understand.
Keep up the great work, and don't forget - in programming, as in life, being specific is always better than being vague!
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new utility functions and prop types # Test 1: Check if the new utility functions are used elsewhere echo "Test 1: Checking usage of new utility functions" rg "getHeadersCount|getParamsCount" --type typescript # Test 2: Verify if Property type is imported and used correctly echo "Test 2: Verifying Property type usage" rg "import.*Property.*from 'entities/Action'" --type typescript rg "Property\[\]" --type typescript # Test 3: Look for any remaining 'any' types that could be replaced with Property[] echo "Test 3: Checking for remaining 'any' types" rg ": any(\[\])?" --type typescriptLength of output: 634
Script:
#!/bin/bash # Description: Verify the usage of new utility functions and prop types within .ts and .tsx files # Test 1: Check if the new utility functions are used elsewhere echo "Test 1: Checking usage of new utility functions" rg "getHeadersCount|getParamsCount" -g "*.ts" -g "*.tsx" # Test 2: Verify if Property type is imported and used correctly echo "Test 2: Verifying Property type usage" rg "import.*Property.*from ['\"]entities/Action['\"]" -g "*.ts" -g "*.tsx" rg "Property\[\]" -g "*.ts" -g "*.tsx" # Test 3: Look for any remaining 'any' types that could be replaced with Property[] echo "Test 3: Checking for remaining 'any' types" rg ": any(\[\])?" -g "*.ts" -g "*.tsx"Length of output: 291932
...onEditor/components/PluginActionForm/components/CommonEditorForm/utils/getValidProperties.ts
Show resolved
Hide resolved
...ctionEditor/components/PluginActionForm/components/CommonEditorForm/utils/getHeadersCount.ts
Show resolved
Hide resolved
...itor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts
Outdated
Show resolved
Hide resolved
|
This PR has increased the number of cyclic dependencies by 6, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts (1)
56-65: A round of applause for your excellent hook structure!The way you've organized and returned the data from this hook is exemplary. It provides a clear and concise interface for other components to use.
To make your code even more helpful for your fellow developers, here's a small suggestion:
Consider adding a JSDoc comment to describe the hook and its return value:
/** * Custom hook to retrieve and organize action-related values from the Redux store. * @returns {Object} An object containing action headers, auto-generated headers, * action parameters, datasource headers, and datasource parameters. */ function useGetFormActionValues() { // ... existing code ... }This addition will make it easier for other developers to understand and use your hook. Remember, good documentation is the key to collaborative coding!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts (1 hunks)
Additional context used
Biome
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 36-36: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
Additional comments not posted (1)
app/client/src/PluginActionEditor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts (1)
1-9: Well done, class! Your imports and hook declaration are spot on!I'm pleased to see that you've imported all the necessary dependencies and types for our custom hook. The naming convention for
useGetFormActionValuesfollows the React best practices for custom hooks. Keep up the good work!
...itor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts
Outdated
Show resolved
Hide resolved
...itor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts
Outdated
Show resolved
Hide resolved
...itor/components/PluginActionForm/components/CommonEditorForm/hooks/useGetFormActionValues.ts
Outdated
Show resolved
Hide resolved
hetunandu
requested review from
AmanAgarwal041,
ankitakinger,
albinAppsmith and
ashit-rath
|
@hetunandu Can we address the comments from coderabbitai? |
hetunandu
dismissed stale reviews from albinAppsmith and ashit-rath
via
2b05378
|
This PR has increased the number of cyclic dependencies by 6, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
| ))} | ||
| {Object.values(API_EDITOR_TABS) | ||
| .filter((tab) => { | ||
| return !(!props.showSettings && tab === API_EDITOR_TABS.SETTINGS); |
There was a problem hiding this comment.
nit: consider adding a comment here for better understanding. (Not a blocker, can be picked up in separate PR)
## Description Passes the correct states for the Common Editor form in the Plugin Action Form. - Uses a hook to pass the form states as already implemented - Uses helper functions to get the header and params count from the said state - As a side effect I can remove these from the original implementation since the invocation is now in the common `RequestTabs` components - Updates the `changeActionCall` hook to only be called if the action changes - Updates the tests to reflect this EE PR to track tests: https://github.com/appsmithorg/appsmith-ee/pull/5217 Parts of appsmithorg#36154 ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced a custom hook for streamlined retrieval of action-related values in forms. - Added utility functions for counting valid headers and parameters. - **Improvements** - Simplified component interfaces by removing unnecessary properties related to headers and parameters. - Enhanced type safety for header and parameter properties. - Refined tab rendering logic for better user experience. - **Bug Fixes** - Adjusted logic to ensure actions are dispatched only on changes to action IDs. - **Documentation** - Updated relevant documentation to reflect changes in component props and functionalities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Passes the correct states for the Common Editor form in the Plugin Action Form.
RequestTabscomponentschangeActionCallhook to only be called if the action changesEE PR to track tests: https://github.com/appsmithorg/appsmith-ee/pull/5217
Parts of #36154
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/11026451216
Commit: 2b05378
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Wed, 25 Sep 2024 08:01:34 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation