Yuki Takei 4 месяцев назад
Родитель
Сommit
807f5b0a55

+ 218 - 0
apps/app/src/features/page-tree/hooks/use-page-create.spec.tsx

@@ -0,0 +1,218 @@
+import type { ItemInstance } from '@headless-tree/core';
+import { fireEvent, render, screen } from '@testing-library/react';
+
+import type { IPageForItem } from '~/interfaces/page';
+
+import { resetCreatingFlagForTesting } from '../states/_inner/page-tree-create';
+import { CreateButtonInner } from './use-page-create';
+
+// Mock the dependencies
+vi.mock('~/client/components/NotAvailableForGuest', () => ({
+  NotAvailableForGuest: ({ children }: { children: React.ReactNode }) => (
+    <>{children}</>
+  ),
+}));
+
+vi.mock('~/client/components/NotAvailableForReadOnlyUser', () => ({
+  NotAvailableForReadOnlyUser: ({
+    children,
+  }: {
+    children: React.ReactNode;
+  }) => <>{children}</>,
+}));
+
+// Mock useCreatingParentId to control isCreating state
+const mockUseCreatingParentId = vi.fn(() => null);
+vi.mock('../states/_inner', () => ({
+  useCreatingParentId: () => mockUseCreatingParentId(),
+  usePageTreeCreateActions: vi.fn(() => ({
+    startCreating: vi.fn(),
+    cancelCreating: vi.fn(),
+  })),
+}));
+
+/**
+ * Create a mock item instance for testing
+ */
+const createMockItem = (
+  id: string,
+  path: string = '/test/path',
+): ItemInstance<IPageForItem> => {
+  return {
+    getId: () => id,
+    getItemData: () => ({ _id: id, path }) as IPageForItem,
+  } as unknown as ItemInstance<IPageForItem>;
+};
+
+describe('CreateButtonInner', () => {
+  beforeEach(() => {
+    resetCreatingFlagForTesting();
+    mockUseCreatingParentId.mockReturnValue(null);
+  });
+
+  describe('rendering', () => {
+    test('should render button for regular page', () => {
+      const mockItem = createMockItem('page-id', '/regular/path');
+      const onStartCreating = vi.fn();
+
+      render(
+        <CreateButtonInner item={mockItem} onStartCreating={onStartCreating} />,
+      );
+
+      expect(screen.getByRole('button')).toBeInTheDocument();
+    });
+
+    test('should NOT render button for users top page', () => {
+      const mockItem = createMockItem('users-top', '/user');
+      const onStartCreating = vi.fn();
+
+      render(
+        <CreateButtonInner item={mockItem} onStartCreating={onStartCreating} />,
+      );
+
+      expect(screen.queryByRole('button')).not.toBeInTheDocument();
+    });
+  });
+
+  describe('onMouseDown behavior', () => {
+    test('should call preventDefault on mousedown to prevent focus change', () => {
+      const mockItem = createMockItem('page-id');
+      const onStartCreating = vi.fn();
+
+      render(
+        <CreateButtonInner item={mockItem} onStartCreating={onStartCreating} />,
+      );
+
+      const button = screen.getByRole('button');
+
+      // Use fireEvent which triggers React's synthetic event handler
+      // and check if the event was prevented by examining defaultPrevented
+      const mouseDownEvent = fireEvent.mouseDown(button);
+
+      // fireEvent returns false if preventDefault was called
+      // (the event's default action was prevented)
+      expect(mouseDownEvent).toBe(false);
+    });
+
+    test('should preserve focus on input when clicking CreateButton', () => {
+      const mockItem = createMockItem('page-id');
+      const onStartCreating = vi.fn();
+
+      render(
+        <div>
+          <input data-testid="placeholder-input" />
+          <CreateButtonInner
+            item={mockItem}
+            onStartCreating={onStartCreating}
+          />
+        </div>,
+      );
+
+      const input = screen.getByTestId('placeholder-input');
+      const button = screen.getByRole('button');
+
+      // Focus the input
+      input.focus();
+      expect(document.activeElement).toBe(input);
+
+      // mousedown on button - React's onMouseDown with preventDefault should fire
+      const mouseDownEvent = fireEvent.mouseDown(button);
+
+      // Verify preventDefault was called
+      expect(mouseDownEvent).toBe(false);
+
+      // Note: jsdom doesn't fully simulate focus behavior with preventDefault,
+      // but we've verified preventDefault is called
+    });
+  });
+
+  describe('onClick behavior', () => {
+    test('should call onStartCreating with item when not already creating', () => {
+      const mockItem = createMockItem('page-id');
+      const onStartCreating = vi.fn();
+
+      // isCreating = false (creatingParentId is null)
+      mockUseCreatingParentId.mockReturnValue(null);
+
+      render(
+        <CreateButtonInner item={mockItem} onStartCreating={onStartCreating} />,
+      );
+
+      const button = screen.getByRole('button');
+      fireEvent.click(button);
+
+      expect(onStartCreating).toHaveBeenCalledTimes(1);
+      expect(onStartCreating).toHaveBeenCalledWith(mockItem);
+    });
+
+    test('should NOT call onStartCreating when already creating', () => {
+      const mockItem = createMockItem('page-id');
+      const onStartCreating = vi.fn();
+
+      // isCreating = true (creatingParentId is not null)
+      mockUseCreatingParentId.mockReturnValue('some-parent-id');
+
+      render(
+        <CreateButtonInner item={mockItem} onStartCreating={onStartCreating} />,
+      );
+
+      const button = screen.getByRole('button');
+      fireEvent.click(button);
+
+      expect(onStartCreating).not.toHaveBeenCalled();
+    });
+
+    test('should call stopPropagation on click to prevent parent handlers', () => {
+      const mockItem = createMockItem('page-id');
+      const onStartCreating = vi.fn();
+      const parentClickHandler = vi.fn();
+
+      render(
+        // eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events
+        <div onClick={parentClickHandler}>
+          <CreateButtonInner
+            item={mockItem}
+            onStartCreating={onStartCreating}
+          />
+        </div>,
+      );
+
+      const button = screen.getByRole('button');
+      fireEvent.click(button);
+
+      // Parent should not receive the click event
+      expect(parentClickHandler).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('rapid click prevention', () => {
+    test('should ignore clicks when already in creating mode', () => {
+      const mockItem = createMockItem('page-id');
+      const onStartCreating = vi.fn();
+
+      // Start with not creating
+      mockUseCreatingParentId.mockReturnValue(null);
+
+      const { rerender } = render(
+        <CreateButtonInner item={mockItem} onStartCreating={onStartCreating} />,
+      );
+
+      const button = screen.getByRole('button');
+
+      // First click should work
+      fireEvent.click(button);
+      expect(onStartCreating).toHaveBeenCalledTimes(1);
+
+      // Simulate that creating mode is now active
+      mockUseCreatingParentId.mockReturnValue('page-id');
+
+      rerender(
+        <CreateButtonInner item={mockItem} onStartCreating={onStartCreating} />,
+      );
+
+      // Second click should be ignored
+      fireEvent.click(button);
+      expect(onStartCreating).toHaveBeenCalledTimes(1);
+    });
+  });
+});

+ 4 - 1
apps/app/src/features/page-tree/hooks/use-page-create.tsx

@@ -28,7 +28,10 @@ type CreateButtonInnerProps = {
   onStartCreating: (item: ItemInstance<IPageForItem>) => void;
   onStartCreating: (item: ItemInstance<IPageForItem>) => void;
 };
 };
 
 
-const CreateButtonInner: FC<CreateButtonInnerProps> = ({
+/**
+ * @internal Exported for testing purposes
+ */
+export const CreateButtonInner: FC<CreateButtonInnerProps> = ({
   item,
   item,
   onStartCreating,
   onStartCreating,
 }) => {
 }) => {

+ 249 - 0
apps/app/src/features/page-tree/hooks/use-placeholder-rename-effect.spec.tsx

@@ -0,0 +1,249 @@
+import type { ItemInstance } from '@headless-tree/core';
+import { renderHook } from '@testing-library/react';
+
+import type { IPageForItem } from '~/interfaces/page';
+
+import { CREATING_PAGE_VIRTUAL_ID } from '../constants/_inner';
+import { usePlaceholderRenameEffect } from './use-placeholder-rename-effect';
+
+/**
+ * Create a mock item instance for testing
+ */
+const createMockItem = (
+  id: string,
+  options: {
+    isRenaming?: boolean;
+  } = {},
+): ItemInstance<IPageForItem> & { setIsRenaming: (value: boolean) => void } => {
+  let isRenaming = options.isRenaming ?? false;
+
+  const item = {
+    getId: () => id,
+    getItemData: () => ({ _id: id }) as IPageForItem,
+    isRenaming: vi.fn(() => isRenaming),
+    startRenaming: vi.fn(() => {
+      isRenaming = true;
+    }),
+    // Helper to simulate external renaming state changes (e.g., ESC key press)
+    setIsRenaming: (value: boolean) => {
+      isRenaming = value;
+    },
+  } as unknown as ItemInstance<IPageForItem> & { setIsRenaming: (value: boolean) => void };
+
+  return item;
+};
+
+describe('usePlaceholderRenameEffect', () => {
+  describe('when item is a placeholder', () => {
+    test('should call startRenaming when placeholder is not in renaming mode', () => {
+      const mockItem = createMockItem(CREATING_PAGE_VIRTUAL_ID, { isRenaming: false });
+      const onCancelCreate = vi.fn();
+
+      renderHook(() =>
+        usePlaceholderRenameEffect({
+          item: mockItem,
+          onCancelCreate,
+        }),
+      );
+
+      expect(mockItem.startRenaming).toHaveBeenCalledTimes(1);
+      expect(onCancelCreate).not.toHaveBeenCalled();
+    });
+
+    test('should not call startRenaming when placeholder is already in renaming mode', () => {
+      const mockItem = createMockItem(CREATING_PAGE_VIRTUAL_ID, { isRenaming: true });
+      const onCancelCreate = vi.fn();
+
+      renderHook(() =>
+        usePlaceholderRenameEffect({
+          item: mockItem,
+          onCancelCreate,
+        }),
+      );
+
+      expect(mockItem.startRenaming).not.toHaveBeenCalled();
+      expect(onCancelCreate).not.toHaveBeenCalled();
+    });
+
+    test('should call onCancelCreate when renaming mode ends (ESC key simulation)', () => {
+      const mockItem = createMockItem(CREATING_PAGE_VIRTUAL_ID, { isRenaming: false });
+      const onCancelCreate = vi.fn();
+
+      const { rerender } = renderHook(() =>
+        usePlaceholderRenameEffect({
+          item: mockItem,
+          onCancelCreate,
+        }),
+      );
+
+      // Initial render: startRenaming should be called
+      expect(mockItem.startRenaming).toHaveBeenCalledTimes(1);
+
+      // Simulate renaming becoming active (startRenaming succeeded)
+      mockItem.setIsRenaming(true);
+      rerender();
+
+      // onCancelCreate should not be called yet
+      expect(onCancelCreate).not.toHaveBeenCalled();
+
+      // Simulate ESC key press - renaming mode ends
+      mockItem.setIsRenaming(false);
+      rerender();
+
+      // onCancelCreate should be called
+      expect(onCancelCreate).toHaveBeenCalledTimes(1);
+    });
+
+    test('should not call onCancelCreate if renaming was never active', () => {
+      const mockItem = createMockItem(CREATING_PAGE_VIRTUAL_ID, { isRenaming: false });
+      const onCancelCreate = vi.fn();
+
+      // Mock startRenaming to NOT actually change isRenaming state
+      // This simulates a scenario where startRenaming fails
+      mockItem.startRenaming = vi.fn();
+
+      const { rerender } = renderHook(() =>
+        usePlaceholderRenameEffect({
+          item: mockItem,
+          onCancelCreate,
+        }),
+      );
+
+      // startRenaming was called but didn't change state
+      expect(mockItem.startRenaming).toHaveBeenCalled();
+
+      // Rerender with still false
+      rerender();
+
+      // onCancelCreate should NOT be called because wasRenamingRef was never true
+      expect(onCancelCreate).not.toHaveBeenCalled();
+    });
+
+    test('should re-call startRenaming when isRenaming becomes false after rapid clicks', () => {
+      const mockItem = createMockItem(CREATING_PAGE_VIRTUAL_ID, { isRenaming: false });
+      const onCancelCreate = vi.fn();
+
+      const { rerender } = renderHook(() =>
+        usePlaceholderRenameEffect({
+          item: mockItem,
+          onCancelCreate,
+        }),
+      );
+
+      // First render: startRenaming called
+      expect(mockItem.startRenaming).toHaveBeenCalledTimes(1);
+
+      // Simulate renaming becoming active
+      mockItem.setIsRenaming(true);
+      rerender();
+
+      // Simulate rapid click resetting the renaming state (this was the bug)
+      mockItem.setIsRenaming(false);
+      // Reset the mock to track new calls
+      mockItem.startRenaming = vi.fn(() => {
+        mockItem.setIsRenaming(true);
+      });
+      rerender();
+
+      // startRenaming should be called again because isRenaming is now false
+      // This is the key fix - the effect re-runs when isRenaming changes
+      expect(mockItem.startRenaming).toHaveBeenCalledTimes(1);
+    });
+  });
+
+  describe('when item is NOT a placeholder', () => {
+    test('should not call startRenaming for regular items', () => {
+      const mockItem = createMockItem('regular-page-id', { isRenaming: false });
+      const onCancelCreate = vi.fn();
+
+      renderHook(() =>
+        usePlaceholderRenameEffect({
+          item: mockItem,
+          onCancelCreate,
+        }),
+      );
+
+      expect(mockItem.startRenaming).not.toHaveBeenCalled();
+      expect(onCancelCreate).not.toHaveBeenCalled();
+    });
+
+    test('should not call onCancelCreate for regular items even when renaming state changes', () => {
+      const mockItem = createMockItem('regular-page-id', { isRenaming: true });
+      const onCancelCreate = vi.fn();
+
+      const { rerender } = renderHook(() =>
+        usePlaceholderRenameEffect({
+          item: mockItem,
+          onCancelCreate,
+        }),
+      );
+
+      // Simulate renaming mode ending
+      mockItem.setIsRenaming(false);
+      rerender();
+
+      // onCancelCreate should NOT be called for regular items
+      expect(onCancelCreate).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('edge cases', () => {
+    test('should handle multiple rerender cycles correctly', () => {
+      const mockItem = createMockItem(CREATING_PAGE_VIRTUAL_ID, { isRenaming: false });
+      const onCancelCreate = vi.fn();
+
+      const { rerender } = renderHook(() =>
+        usePlaceholderRenameEffect({
+          item: mockItem,
+          onCancelCreate,
+        }),
+      );
+
+      // Cycle 1: Start renaming
+      expect(mockItem.startRenaming).toHaveBeenCalledTimes(1);
+      mockItem.setIsRenaming(true);
+      rerender();
+
+      // Cycle 1: Cancel (ESC)
+      mockItem.setIsRenaming(false);
+      rerender();
+      expect(onCancelCreate).toHaveBeenCalledTimes(1);
+
+      // After cancel, isRenaming is false, so startRenaming will be called again on next rerender
+      // This simulates the placeholder being re-rendered after cancel
+      // (In real app, the placeholder would be removed, but this tests the hook's behavior)
+      rerender();
+
+      // The hook will try to start renaming again because isRenaming is false
+      // This is expected behavior - the hook always tries to ensure placeholder is in renaming mode
+      expect(mockItem.startRenaming).toHaveBeenCalledTimes(2);
+    });
+
+    test('should not call onCancelCreate multiple times for the same cancel event', () => {
+      const mockItem = createMockItem(CREATING_PAGE_VIRTUAL_ID, { isRenaming: false });
+      const onCancelCreate = vi.fn();
+
+      const { rerender } = renderHook(() =>
+        usePlaceholderRenameEffect({
+          item: mockItem,
+          onCancelCreate,
+        }),
+      );
+
+      // Start renaming
+      mockItem.setIsRenaming(true);
+      rerender();
+
+      // Cancel
+      mockItem.setIsRenaming(false);
+      rerender();
+      expect(onCancelCreate).toHaveBeenCalledTimes(1);
+
+      // Multiple rerenders with same state should not trigger additional calls
+      rerender();
+      rerender();
+      rerender();
+      expect(onCancelCreate).toHaveBeenCalledTimes(1);
+    });
+  });
+});

+ 170 - 0
apps/app/src/features/page-tree/states/_inner/page-tree-create.spec.tsx

@@ -0,0 +1,170 @@
+import { act, renderHook } from '@testing-library/react';
+
+import {
+  resetCreatingFlagForTesting,
+  useCreatingParentId,
+  usePageTreeCreateActions,
+} from './page-tree-create';
+
+describe('page-tree-create', () => {
+  beforeEach(() => {
+    // Reset the module-level flag before each test
+    resetCreatingFlagForTesting();
+  });
+
+  describe('usePageTreeCreateActions', () => {
+    describe('startCreating', () => {
+      test('should set creatingParentInfo on first call', () => {
+        const { result: actionsResult } = renderHook(() =>
+          usePageTreeCreateActions(),
+        );
+        const { result: parentIdResult } = renderHook(() =>
+          useCreatingParentId(),
+        );
+
+        act(() => {
+          actionsResult.current.startCreating('parent-id-1', '/parent/path');
+        });
+
+        expect(parentIdResult.current).toBe('parent-id-1');
+      });
+
+      test('should ignore rapid clicks (multiple startCreating calls)', () => {
+        const { result: actionsResult } = renderHook(() =>
+          usePageTreeCreateActions(),
+        );
+        const { result: parentIdResult } = renderHook(() =>
+          useCreatingParentId(),
+        );
+
+        // First call should succeed
+        act(() => {
+          actionsResult.current.startCreating('parent-id-1', '/parent/path1');
+        });
+
+        expect(parentIdResult.current).toBe('parent-id-1');
+
+        // Second call should be ignored (rapid click)
+        act(() => {
+          actionsResult.current.startCreating('parent-id-2', '/parent/path2');
+        });
+
+        // Should still be the first parent
+        expect(parentIdResult.current).toBe('parent-id-1');
+      });
+
+      test('should ignore rapid clicks even from different hook instances', () => {
+        // Simulate different components calling the hook
+        const { result: actionsResult1 } = renderHook(() =>
+          usePageTreeCreateActions(),
+        );
+        const { result: actionsResult2 } = renderHook(() =>
+          usePageTreeCreateActions(),
+        );
+        const { result: parentIdResult } = renderHook(() =>
+          useCreatingParentId(),
+        );
+
+        // First call from instance 1
+        act(() => {
+          actionsResult1.current.startCreating('parent-id-1', '/parent/path1');
+        });
+
+        expect(parentIdResult.current).toBe('parent-id-1');
+
+        // Second call from instance 2 should be ignored
+        act(() => {
+          actionsResult2.current.startCreating('parent-id-2', '/parent/path2');
+        });
+
+        // Should still be the first parent
+        expect(parentIdResult.current).toBe('parent-id-1');
+      });
+    });
+
+    describe('cancelCreating', () => {
+      test('should reset creatingParentInfo to null', () => {
+        const { result: actionsResult } = renderHook(() =>
+          usePageTreeCreateActions(),
+        );
+        const { result: parentIdResult } = renderHook(() =>
+          useCreatingParentId(),
+        );
+
+        // Start creating
+        act(() => {
+          actionsResult.current.startCreating('parent-id-1', '/parent/path');
+        });
+
+        expect(parentIdResult.current).toBe('parent-id-1');
+
+        // Cancel
+        act(() => {
+          actionsResult.current.cancelCreating();
+        });
+
+        expect(parentIdResult.current).toBeNull();
+      });
+
+      test('should allow startCreating to work again after cancelCreating', () => {
+        const { result: actionsResult } = renderHook(() =>
+          usePageTreeCreateActions(),
+        );
+        const { result: parentIdResult } = renderHook(() =>
+          useCreatingParentId(),
+        );
+
+        // First cycle: start and cancel
+        act(() => {
+          actionsResult.current.startCreating('parent-id-1', '/parent/path1');
+        });
+
+        expect(parentIdResult.current).toBe('parent-id-1');
+
+        act(() => {
+          actionsResult.current.cancelCreating();
+        });
+
+        expect(parentIdResult.current).toBeNull();
+
+        // Second cycle: should be able to start again
+        act(() => {
+          actionsResult.current.startCreating('parent-id-2', '/parent/path2');
+        });
+
+        expect(parentIdResult.current).toBe('parent-id-2');
+      });
+
+      test('should reset flag correctly so different hook instance can start', () => {
+        const { result: actionsResult1 } = renderHook(() =>
+          usePageTreeCreateActions(),
+        );
+        const { result: actionsResult2 } = renderHook(() =>
+          usePageTreeCreateActions(),
+        );
+        const { result: parentIdResult } = renderHook(() =>
+          useCreatingParentId(),
+        );
+
+        // Instance 1 starts
+        act(() => {
+          actionsResult1.current.startCreating('parent-id-1', '/parent/path1');
+        });
+
+        // Instance 2 cancels (this can happen from a different component)
+        act(() => {
+          actionsResult2.current.cancelCreating();
+        });
+
+        expect(parentIdResult.current).toBeNull();
+
+        // Instance 2 should now be able to start
+        act(() => {
+          actionsResult2.current.startCreating('parent-id-2', '/parent/path2');
+        });
+
+        expect(parentIdResult.current).toBe('parent-id-2');
+      });
+    });
+  });
+});

+ 8 - 0
apps/app/src/features/page-tree/states/_inner/page-tree-create.ts

@@ -36,6 +36,14 @@ const creatingParentInfoAtom = atom<CreatingParentInfo>(null);
 // This is shared across all hook instances
 // This is shared across all hook instances
 let isCreatingFlag = false;
 let isCreatingFlag = false;
 
 
+/**
+ * Reset the creating flag (for testing purposes only)
+ * @internal
+ */
+export const resetCreatingFlagForTesting = (): void => {
+  isCreatingFlag = false;
+};
+
 /**
 /**
  * Hook to get the current creating parent ID
  * Hook to get the current creating parent ID
  */
  */