refactor: address PR #15 review feedback

Simplify EmptyState to a single isCreateDisabled prop. When disabled,
show only the PAT hint text instead of both messages, and remove the
redundant Tooltip from the disabled button. Remove Tooltip from the
table-view Create button in FunctionsListPage as well.

Protect FunctionCreatePage against unauthenticated access via URL by
showing a warning alert and hiding the form when no PAT is stored.

Clean up UserAvatar: remove all useCallback wrappers, remove unused
logout, use guard pattern in readStoredUser, extract usePatModal hook,
shrink modal to small variant, disable Cancel and modal close while
validating. Extract shared errorMessage utility.

Remove stale question from features.json and update i18n keys.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matej Vašek <matejvasek@gmail.com>

Author: matejvasek

docs/features.json

@@ -109,8 +109,7 @@
       "On Create and Edit pages, UserAvatar displays the GitHub user but is not clickable (PAT cannot be changed from those pages)",
       "When PatModal is dismissed without a PAT, the Function List Page (both empty state and table view) shows a hint text directing the user to click 'Connect to GitHub' in the top-right corner to set a PAT",
       "PAT change logic is encapsulated in a useUserAvatar custom hook following layered architecture (Hook imports Service, Component imports Hook)",
-      "Question is how would change to pat be propagated, maybe the github service can use \"lazy\" authentication -- callback that reads PAT instead of passing PAT to the service",
-      "If the GH PAT is not set in the session then the Create button is inactive/disabled. The tooltiop of the button should say that PAT is required.",
+      "If the GH PAT is not set in the session then the Create button is inactive/disabled.",
       "The GH PAT must not be compiled/hardcode into code at compile time."
     ],
     "passes": false

locales/en/plugin__console-functions-plugin.json

@@ -1,5 +1,6 @@
 {
-  "A GitHub Personal Access Token is required to create functions.": "A GitHub Personal Access Token is required to create functions.",
+  "A GitHub Personal Access Token is required to create functions. Click 'Connect to GitHub' in the top-right corner to connect. Once connected, the create button will be enabled.": "A GitHub Personal Access Token is required to create functions. Click 'Connect to GitHub' in the top-right corner to connect. Once connected, the create button will be enabled.",
+  "A GitHub Personal Access Token is required to create functions. Go to the Functions page and click 'Connect to GitHub' to connect.": "A GitHub Personal Access Token is required to create functions. Go to the Functions page and click 'Connect to GitHub' to connect.",
   "Actions": "Actions",
   "Branch": "Branch",
   "Cancel": "Cancel",
@@ -23,7 +24,6 @@
   "Namespace": "Namespace",
   "No functions found": "No functions found",
   "Owner": "Owner",
-  "PAT is required to create functions. Click 'Connect to GitHub' in the top-right corner.": "PAT is required to create functions. Click 'Connect to GitHub' in the top-right corner.",
   "Personal Access Token": "Personal Access Token",
   "Registry": "Registry",
   "Replicas": "Replicas",

src/components/EmptyState.test.tsx

@@ -32,45 +32,30 @@ describe('FunctionsEmptyState', () => {
     expect(link).toHaveAttribute('href', '/faas/create');
   });
 
-  it('renders hint text when provided', () => {
+  it('shows PAT hint and disabled button when isCreateDisabled is true', () => {
     render(
       <MemoryRouter>
-        <FunctionsEmptyState hint="Connect your PAT first." />
+        <FunctionsEmptyState isCreateDisabled />
       </MemoryRouter>,
     );
 
-    expect(screen.getByText('Connect your PAT first.')).toBeInTheDocument();
+    expect(
+      screen.getByText(/A GitHub Personal Access Token is required to create functions/),
+    ).toBeInTheDocument();
+    expect(
+      screen.queryByText('Create a serverless function to get started.'),
+    ).not.toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Create function' })).toBeDisabled();
   });
 
-  it('does not render hint when not provided', () => {
-    render(
-      <MemoryRouter>
-        <FunctionsEmptyState />
-      </MemoryRouter>,
-    );
-
-    // Only the standard body text should be present
-    expect(screen.getByText('Create a serverless function to get started.')).toBeInTheDocument();
-  });
-
-  it('Create button disabled when isCreateDisabled is true', () => {
-    render(
-      <MemoryRouter>
-        <FunctionsEmptyState isCreateDisabled={true} />
-      </MemoryRouter>,
-    );
-
-    const button = screen.getByRole('button', { name: 'Create function' });
-    expect(button).toBeDisabled();
-  });
-
-  it('Create button remains a link when isCreateDisabled is false', () => {
+  it('shows standard body text when isCreateDisabled is false', () => {
     render(
       <MemoryRouter>
         <FunctionsEmptyState isCreateDisabled={false} />
       </MemoryRouter>,
     );
 
+    expect(screen.getByText('Create a serverless function to get started.')).toBeInTheDocument();
     const link = screen.getByRole('link', { name: 'Create function' });
     expect(link).toHaveAttribute('href', '/faas/create');
   });

src/components/EmptyState.tsx

@@ -4,32 +4,33 @@ import {
   EmptyStateActions,
   EmptyStateBody,
   EmptyStateFooter,
-  Tooltip,
 } from '@patternfly/react-core';
 import { CubesIcon } from '@patternfly/react-icons';
 import { useTranslation } from 'react-i18next';
 import { Link } from 'react-router-dom-v5-compat';
 
 interface FunctionsEmptyStateProps {
-  hint?: string;
   isCreateDisabled?: boolean;
 }
 
-export function FunctionsEmptyState({ hint, isCreateDisabled }: FunctionsEmptyStateProps) {
+export function FunctionsEmptyState({ isCreateDisabled }: FunctionsEmptyStateProps) {
   const { t } = useTranslation('plugin__console-functions-plugin');
 
   return (
     <EmptyState headingLevel="h2" icon={CubesIcon} titleText={t('No functions found')}>
-      <EmptyStateBody>{t('Create a serverless function to get started.')}</EmptyStateBody>
-      {hint && <EmptyStateBody>{hint}</EmptyStateBody>}
+      <EmptyStateBody>
+        {isCreateDisabled
+          ? t(
+              "A GitHub Personal Access Token is required to create functions. Click 'Connect to GitHub' in the top-right corner to connect. Once connected, the create button will be enabled.",
+            )
+          : t('Create a serverless function to get started.')}
+      </EmptyStateBody>
       <EmptyStateFooter>
         <EmptyStateActions>
           {isCreateDisabled ? (
-            <Tooltip content={t('A GitHub Personal Access Token is required to create functions.')}>
-              <Button variant="primary" isDisabled>
-                {t('Create function')}
-              </Button>
-            </Tooltip>
+            <Button variant="primary" isDisabled>
+              {t('Create function')}
+            </Button>
           ) : (
             <Button variant="primary" component={(props) => <Link {...props} to="/faas/create" />}>
               {t('Create function')}

src/components/UserAvatar.test.tsx

@@ -155,5 +155,24 @@ describe('UserAvatar', () => {
 
       expect(screen.queryByText('Personal Access Token')).not.toBeInTheDocument();
     });
+
+    it('disables Cancel button while validating', async () => {
+      const user = userEvent.setup();
+      let resolveConnect: () => void;
+      mockFetchUserInfo.mockReturnValue(
+        new Promise<void>((resolve) => {
+          resolveConnect = resolve;
+        }),
+      );
+
+      renderWithContext(<UserAvatar enableReconnect />);
+
+      await user.type(screen.getByLabelText('Personal Access Token'), 'ghp_slow');
+      await user.click(screen.getByRole('button', { name: 'Connect' }));
+
+      expect(screen.getByRole('button', { name: 'Cancel' })).toBeDisabled();
+
+      resolveConnect!();
+    });
   });
 });

src/components/UserAvatar.tsx

@@ -12,9 +12,10 @@ import {
 import { KeyIcon, UserIcon } from '@patternfly/react-icons';
 import { useTranslation } from 'react-i18next';
 import { ForgeUser, PAT_KEY, USER_KEY } from '../services/types';
-import { useCallback, useContext, useState } from 'react';
+import { useContext, useState } from 'react';
 import { ForgeConnectionContext } from '../context/ForgeConnectionProvider';
 import { useSourceControlService } from '../services/source-control/useSourceControlService';
+import { errorMessage } from '../utils/errorMessage';
 
 interface UserAvatarProps {
   enableReconnect: boolean;
@@ -51,39 +52,34 @@ function useUserAvatar(enableReconnect: boolean) {
     () => enableReconnect && !sessionStorage.getItem(PAT_KEY),
   );
 
-  const login = useCallback(async (pat: string) => {
+  const login = async (pat: string) => {
     const forgeUser = await sourceControlService.fetchUserInfo(pat);
     sessionStorage.setItem(PAT_KEY, pat);
     sessionStorage.setItem(USER_KEY, JSON.stringify(forgeUser));
     setUser(forgeUser);
     setIsModalOpen(false);
     connectToForge();
-  }, []);
-
-  // TODO(twoGiants): unused
-  const logout = useCallback(() => {
-    sessionStorage.removeItem(PAT_KEY);
-    sessionStorage.removeItem(USER_KEY);
-    setUser(null);
-  }, []);
+  };
 
-  const openModal = useCallback(() => setIsModalOpen(true), []);
-  const closeModal = useCallback(() => setIsModalOpen(false), []);
+  const openModal = () => setIsModalOpen(true);
+  const closeModal = () => setIsModalOpen(false);
 
-  return { user, isModalOpen, openModal, closeModal, login, logout };
+  return { user, isModalOpen, openModal, closeModal, login };
 }
 
 function readStoredUser(): ForgeUser | null {
   const pat = sessionStorage.getItem(PAT_KEY);
   const userJson = sessionStorage.getItem(USER_KEY);
-  if (pat && userJson) {
-    try {
-      return JSON.parse(userJson) as ForgeUser;
-    } catch {
-      return null;
-    }
+
+  if (!pat || !userJson) {
+    return null;
+  }
+
+  try {
+    return JSON.parse(userJson) as ForgeUser;
+  } catch {
+    return null;
   }
-  return null;
 }
 
 interface PatModalProps {
@@ -94,25 +90,10 @@ interface PatModalProps {
 
 function PatModal({ isOpen, onClose, onConnect }: PatModalProps) {
   const { t } = useTranslation('plugin__console-functions-plugin');
-  // TODO(twoGiants): extract into customHook
-  const [pat, setPat] = useState('');
-  const [isValidating, setIsValidating] = useState(false);
-  const [error, setError] = useState<string | null>(null);
-
-  const handleConnect = async () => {
-    setIsValidating(true);
-    setError(null);
-    try {
-      await onConnect(pat);
-    } catch (err) {
-      setError(err instanceof Error ? err.message : String(err));
-    } finally {
-      setIsValidating(false);
-    }
-  };
+  const { pat, isValidating, error, setPat, handleConnect } = usePatModal(onConnect);
 
   return (
-    <Modal isOpen={isOpen} onClose={onClose} variant="medium">
+    <Modal isOpen={isOpen} onClose={isValidating ? undefined : onClose} variant="small">
       <ModalHeader title={t('Connect to GitHub')} />
       <ModalBody>
         {error && (
@@ -124,7 +105,7 @@ function PatModal({ isOpen, onClose, onConnect }: PatModalProps) {
               id="pat-input"
               type="password"
               value={pat}
-              onChange={(_event, value) => setPat(value)}
+              onChange={(_, value) => setPat(value)}
             />
           </FormGroup>
         </Form>
@@ -139,10 +120,30 @@ function PatModal({ isOpen, onClose, onConnect }: PatModalProps) {
         >
           {t('Connect')}
         </Button>
-        <Button variant="link" onClick={onClose}>
+        <Button variant="link" onClick={onClose} isDisabled={isValidating}>
           {t('Cancel')}
         </Button>
       </ModalFooter>
     </Modal>
   );
 }
+
+function usePatModal(onConnect: (pat: string) => Promise<void>) {
+  const [pat, setPat] = useState('');
+  const [isValidating, setIsValidating] = useState(false);
+  const [error, setError] = useState<string | null>(null);
+
+  const handleConnect = async () => {
+    setIsValidating(true);
+    setError(null);
+    try {
+      await onConnect(pat);
+    } catch (err) {
+      setError(errorMessage(err));
+    } finally {
+      setIsValidating(false);
+    }
+  };
+
+  return { pat, isValidating, error, setPat, handleConnect };
+}

src/utils/errorMessage.ts

@@ -0,0 +1,3 @@
+export function errorMessage(err: unknown): string {
+  return err instanceof Error ? err.message : String(err);
+}

src/views/FunctionCreatePage.test.tsx

@@ -2,6 +2,7 @@ import { render, screen, waitFor } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import { MemoryRouter } from 'react-router-dom';
 import FunctionCreatePage from './FunctionCreatePage';
+import { PAT_KEY } from '../services/types';
 
 const mockGenerateFunction = jest.fn();
 const mockPush = jest.fn();
@@ -43,10 +44,18 @@ jest.mock('../components/UserAvatar', () => ({
   ),
 }));
 
+beforeEach(() => {
+  sessionStorage.clear();
+});
+
 afterEach(() => {
   jest.clearAllMocks();
 });
 
+afterAll(() => {
+  sessionStorage.clear();
+});
+
 const fillForm = async (user: ReturnType<typeof userEvent.setup>) => {
   await user.type(screen.getByRole('textbox', { name: /Owner/ }), 'testuser');
   await user.type(screen.getByRole('textbox', { name: /Repository/ }), 'my-repo');
@@ -58,6 +67,8 @@ const fillForm = async (user: ReturnType<typeof userEvent.setup>) => {
 
 describe('FunctionCreatePage', () => {
   it('renders CreateFunctionForm', () => {
+    sessionStorage.setItem(PAT_KEY, 'ghp_test');
+
     render(
       <MemoryRouter>
         <FunctionCreatePage />
@@ -69,6 +80,7 @@ describe('FunctionCreatePage', () => {
   });
 
   it('calls generateFunction then push on submit, and navigates on success', async () => {
+    sessionStorage.setItem(PAT_KEY, 'ghp_test');
     const user = userEvent.setup();
     const files = [{ path: 'func.yaml', mode: '100644', content: 'name: f', type: 'blob' }];
     mockGenerateFunction.mockResolvedValue(files);
@@ -107,6 +119,7 @@ describe('FunctionCreatePage', () => {
   });
 
   it('shows an alert on error', async () => {
+    sessionStorage.setItem(PAT_KEY, 'ghp_test');
     const user = userEvent.setup();
     mockGenerateFunction.mockRejectedValue(new Error('Backend error'));
 
@@ -133,4 +146,17 @@ describe('FunctionCreatePage', () => {
 
     expect(screen.getByTestId('user-avatar')).toBeInTheDocument();
   });
+
+  it('shows warning and hides form when no PAT is set', () => {
+    render(
+      <MemoryRouter>
+        <FunctionCreatePage />
+      </MemoryRouter>,
+    );
+
+    expect(
+      screen.getByText(/A GitHub Personal Access Token is required to create functions/),
+    ).toBeInTheDocument();
+    expect(screen.queryByRole('textbox', { name: /Owner/ })).not.toBeInTheDocument();
+  });
 });