refactor: fold LeaveModal into EditToolbar, document sub-component rule

Signed-off-by: Stanislav Jakuschevskij <sjakusch@redhat.com>

Author: twoGiants

docs/ARCHITECTURE.md

@@ -26,7 +26,7 @@ Arrows mean "imports / depends on."
 |-------|---------|------------|
 | **Types** | `common/services/types.ts` | nothing |
 | **Services** | `common/services/*/Service.ts` + implementations | Types, Utils |
-| **Hooks** | `common/services/*/use*.ts` | Services, Types, Utils |
+| **Hooks** | `common/services/*/use*.ts`, `common/hooks/`, `pages/<name>/hooks/` | Services, Types, Utils |
 | **Components** | `common/components/` (shared), `pages/<name>/components/` (page-specific) | Hooks, Types, Utils |
 | **Pages** | `pages/<name>/` | Components, Hooks, Utils |
 | **Utils** | `common/utils/` | nothing (cross-cutting) |
@@ -48,17 +48,23 @@ Arrows mean "imports / depends on."
 
 ## React
 
-### Page / Component / Hook Rules
+### Pages
 
-**Components are simple by default** — they receive data via props, render it, and call callbacks. No logic at the top of a component.
+- **Smart for page-specific data** — pages use central hooks (e.g. `useClusterService`, `useSourceControl`) to fetch, prepare, and transform all data needed for downstream components.
 
-**A component may own its own data and state when it encapsulates a self-contained capability that is not specific to any one page** (e.g., forge connection, auth flows, notification subscriptions). The component becomes the single owner of that concern. Pages consume it without orchestrating its internals.
+### Components
 
-**Pages are smart for page-specific data** — they use central hooks (e.g. `useClusterService`, `useSourceControl`) to fetch, prepare, and transform all data needed for downstream components.
+- **Simple by default** — they receive data via props, render it, and call callbacks. No logic at the top of a component.
+- **May own data when self-contained** — a component may own its own data and state when it encapsulates a self-contained capability that is not specific to any one page (e.g., forge connection, auth flows, notification subscriptions). The component becomes the single owner of that concern. Pages consume it without orchestrating its internals.
+- **Sub-components** — if a sub-component is only used by one parent, keep it in the parent's file, unexported. Extract to its own file only when the sub-component is used by multiple siblings.
 
-**Extract logic into hooks** — if a page or component has any logic (state management, data transformation, side effects), extract it into a custom hook. If the hook is reused by multiple components, put it in a separate file: `useFunctionTable.ts`. If the hook is only used by one component, keep it in the same file, do not export it. If there is no logic, no hook is needed.
+### Hooks
 
-**File ordering** — within a file, put the exported component at the top, then its hook below, then helper functions at the bottom. Readers see the main thing first and can drill down.
+- **Extract logic into hooks** — if a page or component has any logic (state management, data transformation, side effects), extract it into a custom hook. If the hook is only used by one component, keep it in the same file, do not export it. If the hook is reused by multiple components within one page, put it in `src/pages/<name>/hooks/`. If reused across pages, put it in `src/common/hooks/`. If there is no logic, no hook is needed.
+
+### File Ordering
+
+Within a file, put the exported component at the top, then its hook below, then sub-components, then helper functions at the bottom. Readers see the main thing first and can drill down.
 
 ### Performance
 
@@ -67,6 +73,7 @@ Arrows mean "imports / depends on."
 ## Architectural Guidance
 
 - PatternFly components preferred over custom HTML
+- PatternFly styling and styling rules over custom CSS
 - Error handling through ErrorProvider/addError pattern
 - Shared utilities in `common/utils/`, not hand-rolled per component
 - Services consumed through hooks, never imported directly

docs/TESTING.md

@@ -24,6 +24,7 @@ Do NOT write all test cases first and then implement everything at once.
 MSW is the primary mocking strategy for anything that hits the network (GitHub API, K8s API, Go backend). K8s API mocking uses MSW WebSocket capability.
 
 `vi.mock` is only for framework and library internals that have no external service:
+
 - `react-i18next` (translation hook)
 - `@openshift-console/dynamic-plugin-sdk` (console shell runtime components like DocumentTitle, ListPageHeader, consoleFetchJSON)
 - `@patternfly/react-icons` (UI library)
@@ -57,11 +58,13 @@ If it makes an HTTP or WebSocket call, mock it with MSW, not `vi.mock`.
 Every component gets its own exhaustive test file. Every page gets its own test file that tests the page's orchestration and integration with its components.
 
 **Component tests** cover:
+
 - Rendering based on props (all states and variants)
 - User interactions that trigger callbacks (clicks, input, form validation)
 - Internal state (expand/collapse, selection)
 
 **Page tests** cover:
+
 - Component is present on the page and wired correctly
 - Data flows from hooks/services to components (correct props)
 - User actions that trigger cross-component effects or service calls (e.g., form submit calls service, then navigates)
@@ -85,6 +88,8 @@ Overlap between component tests and page tests is expected and acceptable. They
    - **Act:** Perform user actions
    - **Assert:** Verify expected state
 
+6. **Scoping** — Place beforeEach, afterEach, and afterAll inside describe blocks.
+
 ## Mocking Patterns
 
 MSW is the primary approach. `vi.mock` is rare (see Mock Strategy above).

src/common/services/cluster/OcpClusterService.test.ts

@@ -9,21 +9,13 @@ vi.mock('@openshift-console/dynamic-plugin-sdk', () => {
   return { consoleFetchJSON: fn };
 });
 
-beforeEach(() => {
-  (window as unknown as Record<string, unknown>).SERVER_FLAGS = {
-    kubeAPIServerURL: 'https://api.cluster.example.com:6443',
-  };
-});
-
-afterEach(() => {
-  vi.clearAllMocks();
-  delete (window as unknown as Record<string, unknown>).SERVER_FLAGS;
-});
-
 describe('OcpClusterService', () => {
   const namespace = 'my-ns';
 
   beforeEach(() => {
+    (window as unknown as Record<string, unknown>).SERVER_FLAGS = {
+      kubeAPIServerURL: 'https://api.cluster.example.com:6443',
+    };
     // POST calls: SA, Role, RoleBinding, ImageBuilderBinding, TokenRequest
     mockPost
       .mockResolvedValueOnce({})
@@ -33,6 +25,11 @@ describe('OcpClusterService', () => {
       .mockResolvedValueOnce({ status: { token: 'sa-token-value' } });
   });
 
+  afterEach(() => {
+    vi.clearAllMocks();
+    delete (window as unknown as Record<string, unknown>).SERVER_FLAGS;
+  });
+
   it('creates SA, Role, RoleBinding, gets token, and returns kubeconfig', async () => {
     const svc = new OcpClusterService();
     const kubeconfig = await svc.generateKubeconfig(namespace);

src/common/services/function/FunctionService.test.ts

@@ -8,10 +8,6 @@ vi.mock('@openshift-console/dynamic-plugin-sdk', () => ({
   },
 }));
 
-afterEach(() => {
-  vi.restoreAllMocks();
-});
-
 const config: FunctionConfig = {
   name: 'my-func',
   runtime: 'node',
@@ -26,6 +22,10 @@ const files: FileEntry[] = [
 ];
 
 describe('FunctionBackendService', () => {
+  afterEach(() => {
+    vi.restoreAllMocks();
+  });
+
   it('calls consoleFetchJSON.post with the proxy URL and returns generated files', async () => {
     vi.mocked(consoleFetchJSON.post).mockResolvedValue(files);
 

src/pages/function-create/FunctionCreatePage.test.tsx

@@ -55,18 +55,6 @@ vi.mock('../../common/components/UserAvatar', () => ({
   ),
 }));
 
-beforeEach(() => {
-  sessionStorage.clear();
-});
-
-afterEach(() => {
-  vi.clearAllMocks();
-});
-
-afterAll(() => {
-  sessionStorage.clear();
-});
-
 const renderPage = () =>
   render(
     <MemoryRouter>
@@ -82,6 +70,18 @@ const fillForm = async (user: ReturnType<typeof userEvent.setup>) => {
 };
 
 describe('FunctionCreatePage', () => {
+  beforeEach(() => {
+    sessionStorage.clear();
+  });
+
+  afterEach(() => {
+    vi.clearAllMocks();
+  });
+
+  afterAll(() => {
+    sessionStorage.clear();
+  });
+
   it('renders CreateFunctionForm', () => {
     sessionStorage.setItem(PAT_KEY, 'ghp_test');
 

src/pages/function-create/components/CreateFunctionForm.test.tsx

@@ -22,17 +22,12 @@ vi.mock('react-i18next', () => ({
   useTranslation: () => ({ t: (key: string) => key }),
 }));
 
-afterEach(() => {
-  vi.restoreAllMocks();
-});
-
 describe('CreateFunctionForm', () => {
   const onSubmit = vi.fn();
   const onCancel = vi.fn();
 
-  beforeEach(() => {
-    onSubmit.mockClear();
-    onCancel.mockClear();
+  afterEach(() => {
+    vi.restoreAllMocks();
   });
 
   it('renders all form fields', () => {

src/pages/function-edit/components/EditToolbar.test.tsx

@@ -0,0 +1,111 @@
+import { render, screen, act, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { EditToolbar } from './EditToolbar';
+
+const mockNavigate = vi.fn();
+
+vi.mock('react-i18next', () => ({
+  useTranslation: () => ({ t: (key: string) => key }),
+}));
+
+vi.mock('react-router-dom-v5-compat', () => ({
+  useNavigate: () => mockNavigate,
+}));
+
+describe('EditToolbar', () => {
+  beforeEach(() => {
+    mockNavigate.mockClear();
+  });
+
+  it('renders Back and Save & Deploy buttons', () => {
+    render(<EditToolbar hasChanges={false} onSave={vi.fn()} />);
+
+    expect(screen.getByRole('button', { name: 'Back to Functions' })).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Save & Deploy' })).toBeInTheDocument();
+  });
+
+  it('disables Save & Deploy when hasChanges is false', () => {
+    render(<EditToolbar hasChanges={false} onSave={vi.fn()} />);
+
+    expect(screen.getByRole('button', { name: 'Save & Deploy' })).toBeDisabled();
+  });
+
+  it('enables Save & Deploy when hasChanges is true', () => {
+    render(<EditToolbar hasChanges={true} onSave={vi.fn()} />);
+
+    expect(screen.getByRole('button', { name: 'Save & Deploy' })).toBeEnabled();
+  });
+
+  it('shows success alert after save and auto-dismisses after 2s', async () => {
+    vi.useFakeTimers({ shouldAdvanceTime: true });
+    const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime });
+    const onSave = vi.fn().mockResolvedValue(undefined);
+    render(<EditToolbar hasChanges={true} onSave={onSave} />);
+
+    await user.click(screen.getByRole('button', { name: 'Save & Deploy' }));
+
+    expect(screen.getByText('Pushed to GitHub. Deployment running...')).toBeInTheDocument();
+
+    act(() => {
+      vi.advanceTimersByTime(2000);
+    });
+
+    await waitFor(() => {
+      expect(screen.queryByText('Pushed to GitHub. Deployment running...')).not.toBeInTheDocument();
+    });
+
+    vi.useRealTimers();
+  });
+
+  it('shows danger alert when save fails', async () => {
+    const user = userEvent.setup();
+    const onSave = vi.fn().mockRejectedValue(new Error('push failed'));
+    render(<EditToolbar hasChanges={true} onSave={onSave} />);
+
+    await user.click(screen.getByRole('button', { name: 'Save & Deploy' }));
+
+    await waitFor(() => {
+      expect(screen.getByText('push failed')).toBeInTheDocument();
+    });
+  });
+
+  it('navigates to /faas when Back is clicked and no changes', async () => {
+    const user = userEvent.setup();
+    render(<EditToolbar hasChanges={false} onSave={vi.fn()} />);
+
+    await user.click(screen.getByRole('button', { name: 'Back to Functions' }));
+
+    expect(mockNavigate).toHaveBeenCalledWith('/faas');
+  });
+
+  it('shows leave modal when Back is clicked with unsaved changes', async () => {
+    const user = userEvent.setup();
+    render(<EditToolbar hasChanges={true} onSave={vi.fn()} />);
+
+    await user.click(screen.getByRole('button', { name: 'Back to Functions' }));
+
+    expect(mockNavigate).not.toHaveBeenCalled();
+    expect(screen.getByText('You have unsaved changes. Leave anyway?')).toBeInTheDocument();
+  });
+
+  it('closes modal and stays on page when Stay is clicked', async () => {
+    const user = userEvent.setup();
+    render(<EditToolbar hasChanges={true} onSave={vi.fn()} />);
+
+    await user.click(screen.getByRole('button', { name: 'Back to Functions' }));
+    await user.click(screen.getByRole('button', { name: 'Stay' }));
+
+    expect(mockNavigate).not.toHaveBeenCalled();
+    expect(screen.queryByText('You have unsaved changes. Leave anyway?')).not.toBeInTheDocument();
+  });
+
+  it('closes modal and navigates when Leave is clicked', async () => {
+    const user = userEvent.setup();
+    render(<EditToolbar hasChanges={true} onSave={vi.fn()} />);
+
+    await user.click(screen.getByRole('button', { name: 'Back to Functions' }));
+    await user.click(screen.getByRole('button', { name: 'Leave' }));
+
+    expect(mockNavigate).toHaveBeenCalledWith('/faas');
+  });
+});

src/pages/function-edit/components/EditToolbar.tsx

@@ -1,6 +1,10 @@
 import {
   Alert,
   Button,
+  Modal,
+  ModalBody,
+  ModalFooter,
+  ModalHeader,
   Toolbar,
   ToolbarContent,
   ToolbarGroup,
@@ -10,7 +14,6 @@ import { ArrowLeftIcon } from '@patternfly/react-icons';
 import { useEffect, useRef, useState } from 'react';
 import { useTranslation } from 'react-i18next';
 import { useNavigate } from 'react-router-dom-v5-compat';
-import { LeaveModal } from './LeaveModal';
 
 interface EditToolbarProps {
   hasChanges: boolean;
@@ -130,3 +133,30 @@ function useEditToolbar(
     onLeaveCancel,
   };
 }
+
+function LeaveModal({
+  isOpen,
+  onStay,
+  onLeave,
+}: {
+  isOpen: boolean;
+  onStay: () => void;
+  onLeave: () => void;
+}) {
+  const { t } = useTranslation('plugin__console-functions-plugin');
+
+  return (
+    <Modal isOpen={isOpen} onClose={onStay} variant="small">
+      <ModalHeader title={t('Unsaved changes')} />
+      <ModalBody>{t('You have unsaved changes. Leave anyway?')}</ModalBody>
+      <ModalFooter>
+        <Button variant="primary" onClick={onStay}>
+          {t('Stay')}
+        </Button>
+        <Button variant="link" onClick={onLeave}>
+          {t('Leave')}
+        </Button>
+      </ModalFooter>
+    </Modal>
+  );
+}