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.

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

Admin panel init #8742

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Admin panel init #8742

wants to merge 7 commits into from

Conversation

ehconitin
Copy link
Contributor

@ehconitin ehconitin commented Nov 25, 2024

WIP
Related issues -
#7090
#8547
Master issue -
#4499

@ehconitin ehconitin marked this pull request as draft November 25, 2024 19:52
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR introduces an admin panel with user impersonation and feature flag management capabilities, along with necessary routing and state management changes.

  • Added useImpersonate hook in /settings/admin/hooks/useImpersonate.ts for user impersonation but missing lastVisitedView clearing (Clear lastVisitedView when impersonating #7090)
  • Added feature flag management table in /settings/admin/components/SettingsAdminFeatureFlags.tsx without loading/error states
  • Added admin panel access control via canImpersonate flag in AppRouter.tsx and SettingsNavigationDrawerItems.tsx
  • Refactored clearSession in useAuth.ts to support both sign-out and impersonation flows
  • Added admin panel route with conditional rendering based on user permissions in SettingsRoutes.tsx

11 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -69,6 +69,49 @@ export const useAuth = () => {

const setDateTimeFormat = useSetRecoilState(dateTimeFormatState);

const clearSession = useRecoilCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: clearSession preserves UI states but doesn't clear lastVisitedView from localStorage as required by issue #7090

margin-top: ${({ theme }) => theme.spacing(0.5)};
`;

const StyledTableHeaderRow = styled(Table)`
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: StyledTableHeaderRow extends Table instead of TableRow, which could cause layout issues

<TableHeader>Value</TableHeader>
</TableRow>
</StyledTableHeaderRow>
{currentWorkspace?.featureFlags?.map((flag) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

style: No handling of empty/undefined featureFlags array - should show empty state message

`;

export const SettingsAdminFeatureFlags = () => {
const currentWorkspace = useRecoilValue(currentWorkspaceState);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: No loading or error state handling when fetching currentWorkspace

<Section>
<H2Title title="Feature Flags" description="Manage feature flags." />

<Table>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Table structure creates new Table component for each row instead of reusing one table with multiple rows

setCurrentUser(user);
setTokenPair(tokens);
await sleep(0);
window.location.href = AppPath.Index;
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: setIsLoading(false) missing before redirect - loading state will remain true if navigation fails

Comment on lines +43 to +45
await clearSession();
setCurrentUser(user);
setTokenPair(tokens);
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: lastVisitedView in localStorage needs to be cleared here per issue #7090

await clearSession();
setCurrentUser(user);
setTokenPair(tokens);
await sleep(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: sleep(0) used without clear purpose - could be removed

Comment on lines +89 to +90
const currentUser = useRecoilValue(currentUserState);
const isAdminPageEnabled = currentUser?.canImpersonate;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding a more explicit name like isAdminEnabled instead of isAdminPageEnabled to match the naming pattern of other feature flags


export const SettingsAdmin = () => {
return (
<SubMenuTopBarContainer title="Admin Panel" links={[{ children: 'Admin' }]}>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding a more descriptive breadcrumb trail (e.g. Settings > Admin Panel) for better navigation context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants