Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WEB: Discard changes working fine when switching tab of app settings #312

Merged
merged 5 commits into from
Oct 8, 2024
Merged

WEB: Discard changes working fine when switching tab of app settings #312

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f7950f
testing app settings
nxtcoder19 Oct 8, 2024
384a3c1
updated discard-changes function
tulsiojha Oct 8, 2024
f5bd8ea
fixed default diff in env from backend and frontend by setting defaul…
tulsiojha Oct 8, 2024
4034a10
environment discard changes on tab switching works fine
nxtcoder19 Oct 8, 2024
d94539f
minor refactoring
nxtcoder19 Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions lib/client/hooks/use-unsaved-changes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ import {
useRevalidator,
} from '@remix-run/react';
import {
ReactNode,
createContext,
useCallback,
useContext,
useEffect,
useMemo,
useState,
} from 'react';
import { ChildrenProps } from '~/components/types';
import Popup from '~/components/molecule/popup';
import { useReload } from '../helpers/reloader';

Expand Down Expand Up @@ -41,7 +41,13 @@ const UnsavedChanges = createContext<{
loading: false,
});

export const UnsavedChangesProvider = ({ children }: ChildrenProps) => {
export const UnsavedChangesProvider = ({
children,
onProceed,
}: {
children?: ReactNode;
onProceed?: (props: { setPerformAction?: (action: string) => void }) => void;
}) => {
const [hasChanges, setHasChanges] = useState<boolean>(false);
const [reload, setReload] = useState(false);
const [ignorePaths, setIgnorePaths] = useState<string[]>([]);
Expand Down Expand Up @@ -139,13 +145,23 @@ export const UnsavedChangesProvider = ({ children }: ChildrenProps) => {
<Popup.Button
content="Discard"
variant="warning"
onClick={() => proceed?.()}
onClick={() => {
proceed?.();
onProceed?.({ setPerformAction });
}}
/>
</Popup.Footer>
</Popup.Root>
</UnsavedChanges.Provider>
);
};

export const DISCARD_ACTIONS = {
DISCARD_CHANGES: 'discard-changes',
VIEW_CHANGES: 'view-changes',
INIT: 'init',
};

export const useUnsavedChanges = () => {
return useContext(UnsavedChanges);
};
35 changes: 35 additions & 0 deletions src/apps/console/hooks/use-is-owner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { useCallback } from 'react';
import useCustomSwr from '~/root/lib/client/hooks/use-custom-swr';
import { useConsoleApi } from '../server/gql/api-provider';

export const useIsOwner = ({ accountName }: { accountName: string }) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
  }

const api = useConsoleApi();
const { data: teamMembers, isLoading: teamMembersLoading } = useCustomSwr(
`${accountName}-owners`,
async () => {
return api.listMembershipsForAccount({
accountName,
});
}
);

const { data: currentUser, isLoading: currentUserLoading } = useCustomSwr(
'current-user',
async () => {
return api.whoAmI();
}
);

const isOwner = useCallback(() => {
if (!teamMembers || !currentUser) return false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)

Suggested change
if (!teamMembers || !currentUser) return false;
if (!teamMembers || !currentUser) {


ExplanationIt 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).


const owner = teamMembers.find((member) => member.role === 'account_owner');

return owner?.user?.email === currentUser?.email;
}, [teamMembers, currentUser]);

return {
isOwner: isOwner(),
isLoading: teamMembersLoading || currentUserLoading,
};
};
11 changes: 7 additions & 4 deletions src/apps/console/page-components/app/compute.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ import ExtendedFilledTab from '~/console/components/extended-filled-tab';
import { useAppState } from '~/console/page-components/app-states';
import { FadeIn, parseValue } from '~/console/page-components/util';
import useForm, { dummyEvent } from '~/root/lib/client/hooks/use-form';
import { useUnsavedChanges } from '~/root/lib/client/hooks/use-unsaved-changes';
import {
DISCARD_ACTIONS,
useUnsavedChanges,
} from '~/root/lib/client/hooks/use-unsaved-changes';
import Yup from '~/root/lib/server/helpers/yup';
import appInitialFormValues, { mapFormValuesToApp } from './app-utils';
import { plans } from './datas';
Expand Down Expand Up @@ -79,7 +82,7 @@ const AppCompute = ({ mode = 'new' }: { mode: 'edit' | 'new' }) => {
mapFormValuesToApp({
appIn: val,
oldAppIn: s,
})
}),
);
},
});
Expand All @@ -97,7 +100,7 @@ const AppCompute = ({ mode = 'new' }: { mode: 'edit' | 'new' }) => {
}, [values, mode]);

useEffect(() => {
if (performAction === 'discard-changes') {
if (performAction === DISCARD_ACTIONS.DISCARD_CHANGES) {
resetValues();
}
}, [performAction]);
Expand Down Expand Up @@ -167,7 +170,7 @@ const AppCompute = ({ mode = 'new' }: { mode: 'edit' | 'new' }) => {
handleChange('selectedPlan')(dummyEvent(v.value));
handleChange('memPerCpu')(dummyEvent(v.memoryPerCpu));
handleChange('cpuMode')(
dummyEvent(v.isShared ? 'shared' : 'dedicated')
dummyEvent(v.isShared ? 'shared' : 'dedicated'),
);
}}
/>
Expand Down
15 changes: 9 additions & 6 deletions src/apps/console/page-components/app/general.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ import { ensureAccountClientSide } from '~/console/server/utils/auth-utils';
import { constants } from '~/console/server/utils/constants';
import useDebounce from '~/root/lib/client/hooks/use-debounce';
import useForm, { dummyEvent } from '~/root/lib/client/hooks/use-form';
import { useUnsavedChanges } from '~/root/lib/client/hooks/use-unsaved-changes';
import {
DISCARD_ACTIONS,
useUnsavedChanges,
} from '~/root/lib/client/hooks/use-unsaved-changes';
import Yup from '~/root/lib/server/helpers/yup';
import { handleError } from '~/root/lib/utils/common';

Expand Down Expand Up @@ -119,7 +122,7 @@ const AppGeneral = ({ mode = 'new' }: { mode: 'edit' | 'new' }) => {
setImageLoaded(false);
}
},
[]
[],
);

useEffect(() => {
Expand All @@ -133,7 +136,7 @@ const AppGeneral = ({ mode = 'new' }: { mode: 'edit' | 'new' }) => {
}
},
300,
[imageSearchText]
[imageSearchText],
);

const {
Expand Down Expand Up @@ -176,7 +179,7 @@ const AppGeneral = ({ mode = 'new' }: { mode: 'edit' | 'new' }) => {
displayName: Yup.string().required(),
imageUrl: Yup.string().matches(
constants.dockerImageFormatRegex,
'Invalid image format'
'Invalid image format',
),
manualRepo: Yup.string().when(
['imageUrl', 'imageMode'],
Expand All @@ -189,7 +192,7 @@ const AppGeneral = ({ mode = 'new' }: { mode: 'edit' | 'new' }) => {
return schema.required().matches(regex, 'Invalid image format');
}
return schema;
}
},
),
imageMode: Yup.string().required(),
source: Yup.object()
Expand Down Expand Up @@ -242,7 +245,7 @@ const AppGeneral = ({ mode = 'new' }: { mode: 'edit' | 'new' }) => {
}, [values, mode]);

useEffect(() => {
if (performAction === 'discard-changes') {
if (performAction === DISCARD_ACTIONS.DISCARD_CHANGES) {
// if (app.ciBuildId) {
// setIsEdited(false);
// }
Expand Down
13 changes: 9 additions & 4 deletions src/apps/console/routes/_main+/$account+/env+/$environment+/app+/$app+/settings+/_layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { constants } from '~/console/server/utils/constants';
import { useReload } from '~/lib/client/helpers/reloader';
import useForm from '~/lib/client/hooks/use-form';
import {
DISCARD_ACTIONS,
UnsavedChangesProvider,
useUnsavedChanges,
} from '~/lib/client/hooks/use-unsaved-changes';
Expand Down Expand Up @@ -177,7 +178,7 @@ const Layout = () => {
}
toast.success('App updated successfully');
// @ts-ignore
setPerformAction('init');
setPerformAction(DISCARD_ACTIONS.INIT);
if (!gitMode) {
// @ts-ignore
setBuildData(null);
Expand Down Expand Up @@ -221,7 +222,7 @@ const Layout = () => {
}, [rootContext.app]);

useEffect(() => {
if (performAction === 'discard-changes') {
if (performAction === DISCARD_ACTIONS.DISCARD_CHANGES) {
setApp(rootContext.app);
setReadOnlyApp(rootContext.app);
// @ts-ignore
Expand All @@ -237,7 +238,7 @@ const Layout = () => {
'min-w-[1000px]': showDiff,
'min-w-[500px]': !showDiff,
})}
show={performAction === 'view-changes'}
show={performAction === DISCARD_ACTIONS.VIEW_CHANGES}
onOpenChange={(v) => setPerformAction(v)}
>
<Popup.Header>Commit Changes</Popup.Header>
Expand Down Expand Up @@ -302,7 +303,11 @@ const Settings = () => {
const rootContext = useOutletContext<IAppContext>();
return (
<AppContextProvider initialAppState={rootContext.app}>
<UnsavedChangesProvider>
<UnsavedChangesProvider
onProceed={({ setPerformAction }) => {
setPerformAction?.(DISCARD_ACTIONS.DISCARD_CHANGES);
}}
>
<Layout />
</UnsavedChangesProvider>
</AppContextProvider>
Expand Down
37 changes: 34 additions & 3 deletions .../console/routes/_main+/$account+/env+/$environment+/new-app/app-environment-variables.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ import NoResultsFound from '~/console/components/no-results-found';
import { IShowDialog } from '~/console/components/types.d';
import { useAppState } from '~/console/page-components/app-states';
import useForm from '~/root/lib/client/hooks/use-form';
import {
DISCARD_ACTIONS,
useUnsavedChanges,
} from '~/root/lib/client/hooks/use-unsaved-changes';
import Yup from '~/root/lib/server/helpers/yup';
import { NonNullableString } from '~/root/lib/types/common';
import AppDialog from './app-dialogs';
Expand Down Expand Up @@ -205,9 +209,11 @@ const EnvironmentVariablesList = ({
};

export const EnvironmentVariables = () => {
const { setContainer, getContainer } = useAppState();
const { setContainer, getContainer, getReadOnlyContainer, readOnlyApp } =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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]);

  1. 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.

  2. 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.

useAppState();

const [showCSDialog, setShowCSDialog] = useState<IShowDialog>(null);
const { performAction } = useUnsavedChanges();

const entry = Yup.object({
type: Yup.string().oneOf(['config', 'secret']).notRequired(),
Expand All @@ -231,10 +237,16 @@ export const EnvironmentVariables = () => {
.notRequired(),
});

const { values, setValues, submit } = useForm({
initialValues: getContainer().env,
const {
values,
setValues,
submit,
resetValues: reset,
} = useForm({
initialValues: getReadOnlyContainer().env || null,
validationSchema: Yup.array(entry),
onSubmit: (val) => {
// @ts-ignore
setContainer((c) => ({
...c,
env: val,
Expand Down Expand Up @@ -334,6 +346,25 @@ export const EnvironmentVariables = () => {
},
});

useEffect(() => {
if (performAction === DISCARD_ACTIONS.DISCARD_CHANGES) {
// if (app.ciBuildId) {
// setIsEdited(false);
// }
reset();
// @ts-ignore
// setBuildData(readOnlyApp?.build);
}

// else if (performAction === 'init') {
// setIsEdited(false);
// }
}, [performAction]);

useEffect(() => {
reset();
}, [readOnlyApp]);

return (
<>
<form
Expand Down
9 changes: 5 additions & 4 deletions ...+/$account+/$project+/deviceblueprint+/$deviceblueprint+/app+/$app+/settings+/_layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Outlet, useOutletContext } from '@remix-run/react';
import SidebarLayout from '~/iotconsole/components/sidebar-layout';
import {
DISCARD_ACTIONS,
UnsavedChangesProvider,
useUnsavedChanges,
} from '~/lib/client/hooks/use-unsaved-changes';
Expand Down Expand Up @@ -52,8 +53,8 @@ const Layout = () => {
setIgnorePaths(
navItems.map(
(ni) =>
`/${account}/deviceblueprint/${deviceBlueprintName}/app/${appName}/settings/${ni.value}`
)
`/${account}/deviceblueprint/${deviceBlueprintName}/app/${appName}/settings/${ni.value}`,
),
);
}, []);

Expand Down Expand Up @@ -110,7 +111,7 @@ const Layout = () => {
}, [rootContext.app]);

useEffect(() => {
if (performAction === 'discard-changes') {
if (performAction === DISCARD_ACTIONS.DISCARD_CHANGES) {
setApp(rootContext.app);
setPerformAction('');
}
Expand All @@ -120,7 +121,7 @@ const Layout = () => {
<SidebarLayout navItems={navItems} parentPath="/settings">
<Popup.Root
className="w-[90vw] max-w-[1440px] min-w-[1000px]"
show={performAction === 'view-changes'}
show={performAction === DISCARD_ACTIONS.VIEW_CHANGES}
onOpenChange={(v) => setPerformAction(v)}
>
<Popup.Header>Review Changes</Popup.Header>
Expand Down
12 changes: 6 additions & 6 deletions src/design-system/components/atoms/tabs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ const TabBase = ({
'rounded-lg hover:bg-surface-basic-hovered active:bg-surface-basic-pressed':
variant === 'filled',
// 'border border-transparent': variant === 'filled' && !active,
},
}
)}
>
<RovingFocusGroup.Item
Expand Down Expand Up @@ -126,7 +126,7 @@ const TabBase = ({
...(fitted && {
'py-md': variant !== 'filled',
}),
},
}
)}
>
{variant === 'plain' && <div className="h-md bg-none w-full z-0" />}
Expand All @@ -139,7 +139,7 @@ const TabBase = ({
<motion.div
layoutId="underline"
className={cn(
'h-md z-10 absolute left-0 bottom-0 w-full bg-border-primary',
'h-md z-10 absolute left-0 bottom-0 w-full bg-border-primary'
)}
/>
)}
Expand Down Expand Up @@ -183,7 +183,7 @@ const Root = forwardRef<HTMLDivElement, ITabs<any>>(
children,
toLabel,
},
ref,
ref
) => {
const id = useId();
// id = useMemo(() => id, [children, value, basePath, size, variant]);
Expand All @@ -198,7 +198,7 @@ const Root = forwardRef<HTMLDivElement, ITabs<any>>(
'md:gap-4xl': size === 'md' && variant !== 'filled',
'gap-lg': size === 'sm' || variant === 'filled',
},
className,
className
)}
ref={ref}
asChild
Expand Down Expand Up @@ -239,7 +239,7 @@ const Root = forwardRef<HTMLDivElement, ITabs<any>>(
</motion.div>
</RovingFocusGroup.Root>
);
},
}
);

const Tabs = {
Expand Down
Loading