WEB: Discard changes working fine when switching tab of app settings #312
Conversation
Reviewer's Guide by SourceryThis pull request implements changes to improve the functionality of discarding changes when switching tabs in app settings. It introduces a new hook for checking owner status, updates the UnsavedChangesProvider to handle discarding changes, and refactors the use of action constants across multiple components. Class diagram for useIsOwner hookclassDiagram
class useIsOwner {
+boolean isOwner
+boolean isLoading
+useCallback()
}
useIsOwner --> useCustomSwr
useIsOwner --> useConsoleApi
useCustomSwr --> useConsoleApi
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nxtCoder19 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using the
DISCARD_ACTIONSobject consistently throughout the codebase instead of hardcoded strings for action types. This will improve maintainability and reduce the chance of typos. - Remove or add TODO comments for commented-out code blocks to improve code cleanliness and maintainability.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| import useCustomSwr from '~/root/lib/client/hooks/use-custom-swr'; | ||
| import { useConsoleApi } from '../server/gql/api-provider'; | ||
|
|
||
| export const useIsOwner = ({ accountName }: { accountName: string }) => { |
There was a problem hiding this comment.
suggestion: Add error handling to API calls in useIsOwner hook
Consider adding error handling for the API calls in this custom hook. This will make the hook more robust and provide better feedback in case of network issues or API errors.
export const useIsOwner = ({ accountName }: { accountName: string }) => {
const api = useConsoleApi();
const { data: teamMembers, isLoading: teamMembersLoading, error } = useCustomSwr(
// ... existing code ...
);
if (error) {
console.error('Error fetching team members:', error);
}
| @@ -205,9 +209,11 @@ const EnvironmentVariablesList = ({ | |||
| }; | |||
|
|
|||
| export const EnvironmentVariables = () => { | |||
| const { setContainer, getContainer } = useAppState(); | |||
| const { setContainer, getContainer, getReadOnlyContainer, readOnlyApp } = | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the component to reduce complexity and improve code organization.
The changes have introduced unnecessary complexity to this component. While the new functionality is valuable, it can be implemented more efficiently. Consider the following improvements:
- Consolidate form handling logic:
const { values, setValues, submit, reset } = useForm({
initialValues: getReadOnlyContainer().env || [],
validationSchema: Yup.array(entry),
onSubmit: (val) => {
setContainer((c) => ({ ...c, env: val }));
},
});
useEffect(() => {
submit();
}, [values]);-
Move unsaved changes logic to a higher-level component or a more appropriate place in the application structure. This component should focus on form handling.
-
Simplify useEffect hooks:
useEffect(() => {
if (performAction === DISCARD_ACTIONS.DISCARD_CHANGES) {
reset();
}
}, [performAction, reset]);
useEffect(() => {
reset();
}, [readOnlyApp, reset]);These changes will maintain the new functionality while significantly reducing the complexity of the component. The form handling becomes more straightforward, and the concerns of managing unsaved changes are separated from the direct form manipulation.
| ); | ||
|
|
||
| const isOwner = useCallback(() => { | ||
| if (!teamMembers || !currentUser) return false; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!teamMembers || !currentUser) return false; | |
| if (!teamMembers || !currentUser) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
WEB: Discard changes working fine when switching tab of app settings
WEB: Discard changes working fine when switching tab of app settings
WEB: Discard changes working fine when switching tab of app settings
Summary by Sourcery
Implement a new hook to check account ownership and enhance the unsaved changes handling by introducing a callback mechanism and a standardized action management system.
New Features:
useIsOwnerto determine if the current user is the owner of an account by checking their role against the account's team members.Enhancements:
UnsavedChangesProviderto accept anonProceedcallback, allowing actions to be performed when changes are discarded.DISCARD_ACTIONSobject to manage different discard actions consistently across components.