refactor

apps/app/src/pages/[[...path]]/use-same-route-navigation.spec.tsx

@@ -5,26 +5,24 @@ import { mockDeep } from 'vitest-mock-extended';
 
 import { useSameRouteNavigation } from './use-same-route-navigation';
 
-// Mock Next.js router first
+// Mock Next.js router
 const mockRouter = mockDeep<NextRouter>();
 vi.mock('next/router', () => ({
   useRouter: vi.fn(() => mockRouter),
 }));
 
-// Mock other dependencies
-vi.mock('~/states/page');
-vi.mock('~/stores/editor');
-
-// Mock hook implementations
+// Mock dependencies and their implementations
 const mockUseCurrentPageData = vi.fn();
 const mockUseCurrentPageId = vi.fn();
 const mockUseFetchCurrentPage = vi.fn();
 const mockUseEditingMarkdown = vi.fn();
+const mockUseCurrentPageLoading = vi.fn();
 
 vi.mock('~/states/page', () => ({
   useCurrentPageData: () => mockUseCurrentPageData(),
   useCurrentPageId: () => mockUseCurrentPageId(),
   useFetchCurrentPage: () => mockUseFetchCurrentPage(),
+  useCurrentPageLoading: () => mockUseCurrentPageLoading(),
 }));
 
 vi.mock('~/stores/editor', () => ({
@@ -37,16 +35,15 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
   const mockFetchCurrentPage = vi.fn();
   const mockMutateEditingMarkdown = vi.fn();
 
-  // Simple props simulation - enough to trigger the actual hook logic
+  // Test helper to create props
   const createProps = (currentPathname: string) => {
-    // Cast to bypass type complexity - focus on behavior testing
     return { currentPathname } as Parameters<typeof useSameRouteNavigation>[0];
   };
 
   beforeEach(() => {
     vi.clearAllMocks();
 
-    // Base router setup - tests will override as needed
+    // Base router setup
     mockRouter.asPath = '/current/path';
     mockRouter.pathname = '/[[...path]]';
     (useRouter as ReturnType<typeof vi.fn>).mockReturnValue(mockRouter);
@@ -56,6 +53,7 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
     mockUseCurrentPageId.mockReturnValue([null, mockSetCurrentPageId]);
     mockUseFetchCurrentPage.mockReturnValue({ fetchCurrentPage: mockFetchCurrentPage });
     mockUseEditingMarkdown.mockReturnValue({ mutate: mockMutateEditingMarkdown });
+    mockUseCurrentPageLoading.mockReturnValue({ isLoading: false, error: null });
 
     // Default fetch behavior
     mockFetchCurrentPage.mockResolvedValue({
@@ -65,9 +63,9 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
 
   describe('CORE FIX: router.asPath dependency', () => {
     it('should fetch when router.asPath differs from props.currentPathname', async() => {
-      // THE EXACT BUG SCENARIO: stale props vs current router state
+      // Bug scenario: stale props vs current router state
       const props = createProps('/stale/props/path');
-      mockRouter.asPath = '/actual/browser/path'; // Browser is actually here
+      mockRouter.asPath = '/actual/browser/path';
 
       renderHook(() => useSameRouteNavigation(props));
 
@@ -75,7 +73,7 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
         await new Promise(resolve => setTimeout(resolve, 0));
       });
 
-      // VERIFY THE FIX: should use router.asPath, not props.currentPathname
+      // Verify fix: should use router.asPath, not props.currentPathname
       expect(mockFetchCurrentPage).toHaveBeenCalledWith('/actual/browser/path');
       expect(mockFetchCurrentPage).not.toHaveBeenCalledWith('/stale/props/path');
     });
@@ -83,21 +81,20 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
     it('should react to router.asPath changes (useEffect dependency)', async() => {
       const props = createProps('/some/path');
 
-      // Start with MISMATCHED state to trigger initial fetch
+      // Start with mismatched state to trigger initial fetch
       mockRouter.asPath = '/some/path';
       mockUseCurrentPageData.mockReturnValue([{ path: '/different/path' }]);
-      mockUseCurrentPageId.mockReturnValue([null, mockSetCurrentPageId]); // null for non-permalink
+      mockUseCurrentPageId.mockReturnValue([null, mockSetCurrentPageId]);
 
       const { rerender } = renderHook(() => useSameRouteNavigation(props));
 
-      // Initial render should trigger fetch (mismatched state)
+      // Initial render should trigger fetch
       await act(async() => {
         await new Promise(resolve => setTimeout(resolve, 0));
       });
       expect(mockFetchCurrentPage).toHaveBeenCalledTimes(1);
       expect(mockFetchCurrentPage).toHaveBeenLastCalledWith('/some/path');
 
-      // Clear the mock to test the change detection
       mockFetchCurrentPage.mockClear();
 
       // Browser navigation changes router.asPath
@@ -126,10 +123,6 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
       });
 
       expect(mockSetCurrentPageId).toHaveBeenCalledWith(undefined);
-      // Since extractPageIdFromPathname returns null for non-permalink paths,
-      // but the hook uses: if (targetPageId) { setCurrentPageId(targetPageId); }
-      // When targetPageId is null, setCurrentPageId is not called with null
-      // Only the clearing call with undefined happens
     });
 
     it('should update editor content on successful fetch', async() => {
@@ -151,15 +144,14 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
 
       // Page already loaded and matches
       mockUseCurrentPageData.mockReturnValue([{ path: '/current/page' }]);
-      mockUseCurrentPageId.mockReturnValue([null, mockSetCurrentPageId]); // null for non-permalink
+      mockUseCurrentPageId.mockReturnValue([null, mockSetCurrentPageId]);
 
       renderHook(() => useSameRouteNavigation(props));
 
-      // Should not fetch when everything is already in sync
       expect(mockFetchCurrentPage).not.toHaveBeenCalled();
     });
 
-    // CRITICAL: Race condition prevention - THE CORE BUG FIX
+    // Race condition prevention - core bug fix
     it('should prevent concurrent fetches during rapid navigation', async() => {
       const props = createProps('/first/page');
       mockRouter.asPath = '/first/page';
@@ -183,8 +175,7 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
       mockRouter.asPath = '/second/page';
       rerender();
 
-      // Should NOT start second fetch due to race condition protection
-      expect(mockFetchCurrentPage).toHaveBeenCalledTimes(1); // Still only 1 call
+      expect(mockFetchCurrentPage).toHaveBeenCalledTimes(1);
 
       // Complete the first fetch
       resolveFetch({ revision: { body: 'content' } });
@@ -200,16 +191,15 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
         await new Promise(resolve => setTimeout(resolve, 0));
       });
 
-      expect(mockFetchCurrentPage).toHaveBeenCalledTimes(2); // Now second call
+      expect(mockFetchCurrentPage).toHaveBeenCalledTimes(2);
     });
 
-    // CRITICAL: State clearing sequence - prevents stale data
+    // State clearing sequence - prevents stale data
     it('should clear pageId before setting new one', async() => {
       const props = createProps('/new/page');
       mockRouter.asPath = '/new/page';
 
-      // Start with existing page
-      mockUseCurrentPageId.mockReturnValue([null, mockSetCurrentPageId]); // null for non-permalink
+      mockUseCurrentPageId.mockReturnValue([null, mockSetCurrentPageId]);
 
       renderHook(() => useSameRouteNavigation(props));
 
@@ -217,10 +207,8 @@ describe('useSameRouteNavigation - Essential Bug Fix Verification', () => {
         await new Promise(resolve => setTimeout(resolve, 0));
       });
 
-      // Verify clearing sequence: undefined first, but no second call for null
-      // because the hook only calls setCurrentPageId(targetPageId) if targetPageId is truthy
       expect(mockSetCurrentPageId).toHaveBeenCalledWith(undefined);
-      expect(mockSetCurrentPageId).toHaveBeenCalledTimes(1); // Only the clear call
+      expect(mockSetCurrentPageId).toHaveBeenCalledTimes(1);
       expect(mockSetCurrentPageId.mock.calls[0][0]).toBe(undefined);
     });
   });

apps/app/src/pages/[[...path]]/use-same-route-navigation.ts

@@ -1,4 +1,6 @@
-import { useEffect, useRef, useMemo } from 'react';
+import {
+  useEffect, useRef, useMemo, useCallback,
+} from 'react';
 
 import { useRouter } from 'next/router';
 
@@ -16,22 +18,58 @@ const logger = loggerFactory('growi:hooks:useSameRouteNavigation');
 
 /**
  * Custom hook to calculate the target pathname for navigation
- * Memoizes the result to prevent unnecessary recalculations
+ * Uses router.asPath as the primary source of truth for current location
  */
 const useNavigationTarget = (router: ReturnType<typeof useRouter>, props: Props): string => {
   return useMemo(() => {
-    // Use router.asPath for browser back/forward compatibility, fallback to props.currentPathname
+    // Always prefer router.asPath for accurate browser state
     return router.asPath || props.currentPathname;
   }, [router.asPath, props.currentPathname]);
 };
 
+/**
+ * Handles page state updates during navigation
+ * Centralizes all page-related state management logic
+ */
+const usePageStateManager = () => {
+  const [pageId, setCurrentPageId] = useCurrentPageId();
+  const { fetchCurrentPage } = useFetchCurrentPage();
+  const { mutate: mutateEditingMarkdown } = useEditingMarkdown();
+
+  const updatePageState = useCallback(async(targetPathname: string, targetPageId: string | null) => {
+    try {
+      // Clear current state for clean transition
+      setCurrentPageId(undefined);
+
+      // Set new pageId if available
+      if (targetPageId) {
+        setCurrentPageId(targetPageId);
+      }
+
+      // Fetch page data
+      const pageData = await fetchCurrentPage(targetPathname);
+
+      // Update editing markdown if we have body content
+      if (pageData?.revision?.body !== undefined) {
+        mutateEditingMarkdown(pageData.revision.body);
+      }
+
+      return true; // Success
+    }
+    catch (error) {
+      logger.error('Navigation failed for pathname:', targetPathname, error);
+      return false; // Failure
+    }
+  }, [setCurrentPageId, fetchCurrentPage, mutateEditingMarkdown]);
+
+  return { pageId, updatePageState };
+};
+
 /**
  * Custom hook to check if initial data should be used
- * Memoizes the result to prevent unnecessary recalculations
  */
 const useInitialDataCheck = (props: Props): boolean => {
   return useMemo(() => {
-    // Skip if we have initial data and don't need to refetch
     const skipSSR = isInitialProps(props) ? props.skipSSR : false;
     return isInitialProps(props) && !skipSSR;
   }, [props]);
@@ -41,18 +79,14 @@ const useInitialDataCheck = (props: Props): boolean => {
  * Custom hook for handling same-route navigation and fetching page data when needed
  * Optimized for minimal re-renders and efficient state updates using centralized navigation state
  */
-export const useSameRouteNavigation = (
-    props: Props,
-): void => {
+export const useSameRouteNavigation = (props: Props): void => {
   const router = useRouter();
   const [currentPage] = useCurrentPageData();
-  const [pageId, setCurrentPageId] = useCurrentPageId();
-  const { fetchCurrentPage } = useFetchCurrentPage();
-  const { mutate: mutateEditingMarkdown } = useEditingMarkdown();
 
   // Use custom hooks for better separation of concerns
   const targetPathname = useNavigationTarget(router, props);
   const hasInitialData = useInitialDataCheck(props);
+  const { pageId, updatePageState } = usePageStateManager();
 
   // Track the last processed pathname to prevent unnecessary operations
   const lastProcessedPathnameRef = useRef<string | null>(null);
@@ -95,44 +129,21 @@ export const useSameRouteNavigation = (
 
     isFetchingRef.current = true;
 
-    const updatePageState = async(): Promise<void> => {
-      try {
-        // Clear current state to ensure clean transition
-        setCurrentPageId(undefined);
-
-        // Set new pageId if available
-        if (targetPageId) {
-          setCurrentPageId(targetPageId);
-        }
+    const performNavigation = async(): Promise<void> => {
+      await updatePageState(targetPathname, targetPageId);
 
-        // Fetch page data
-        const pageData = await fetchCurrentPage(targetPathname);
-
-        // Update editing markdown if we have body content
-        if (pageData?.revision?.body !== undefined) {
-          mutateEditingMarkdown(pageData.revision.body);
-        }
-
-        // Mark as processed
-        lastProcessedPathnameRef.current = targetPathname;
-      }
-      catch (error) {
-        // Log error for debugging while preventing UI disruption
-        logger.error('Navigation failed for pathname:', targetPathname, error);
-        // Keep the last processed pathname to prevent retry loops
-        lastProcessedPathnameRef.current = targetPathname;
-      }
-      finally {
-        isFetchingRef.current = false;
-      }
+      // Mark as processed regardless of success to prevent retry loops
+      lastProcessedPathnameRef.current = targetPathname;
+      isFetchingRef.current = false;
     };
 
-    updatePageState();
+    performNavigation();
 
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [
     targetPathname, // Memoized value that includes both router.asPath and props.currentPathname
     hasInitialData, // Memoized value for initial data check
+    pageId, // Page ID changes
   ]);
 
   // Cleanup on unmount

apps/app/src/states/page/index.ts

@@ -27,7 +27,11 @@ export {
 // Data fetching hooks
 export {
   useFetchCurrentPage,
-} from './fetch-current-page';
+} from './use-fetch-current-page';
+
+export {
+  useCurrentPageLoading,
+} from './use-current-page-loading';
 
 // Re-export types that external consumers might need
 export type { UseAtom } from '../ui/helper';

apps/app/src/states/page/internal-atoms.ts

@@ -14,6 +14,10 @@ export const pageNotFoundAtom = atom(false);
 export const pageNotCreatableAtom = atom(false);
 export const latestRevisionAtom = atom(true);
 
+// Fetch state atoms (internal)
+export const pageLoadingAtom = atom(false);
+export const pageErrorAtom = atom<Error | null>(null);
+
 // Template data atoms (internal)
 export const templateTagsAtom = atom<string[]>([]);
 export const templateBodyAtom = atom<string>('');

apps/app/src/states/page/use-current-page-loading.ts

@@ -0,0 +1,17 @@
+import { useAtomValue } from 'jotai';
+
+import { pageLoadingAtom, pageErrorAtom } from './internal-atoms';
+
+/**
+ * Hook to access current page loading state
+ * Provides consistent loading and error state across the application
+ */
+export const useCurrentPageLoading = (): {
+  isLoading: boolean;
+  error: Error | null;
+} => {
+  const isLoading = useAtomValue(pageLoadingAtom);
+  const error = useAtomValue(pageErrorAtom);
+
+  return { isLoading, error };
+};

apps/app/src/states/page/fetch-current-page.ts → apps/app/src/states/page/use-fetch-current-page.ts

@@ -1,8 +1,9 @@
-import { useCallback, useState } from 'react';
+import { useCallback } from 'react';
 
 import type { IPagePopulatedToShowRevision } from '@growi/core';
 import { isClient } from '@growi/core/dist/utils';
 import { isCreatablePage } from '@growi/core/dist/utils/page-path-utils';
+import { useAtomValue } from 'jotai';
 import { useAtomCallback } from 'jotai/utils';
 
 import { apiv3Get } from '~/client/util/apiv3-client';
@@ -10,12 +11,21 @@ import { useShareLinkId } from '~/stores-universal/context';
 
 import {
   currentPageIdAtom, currentPageDataAtom, pageNotFoundAtom, pageNotCreatableAtom,
+  pageLoadingAtom, pageErrorAtom,
 } from './internal-atoms';
 
+// API parameter types for better type safety
+interface PageApiParams {
+  pageId?: string;
+  path?: string;
+  shareLinkId?: string;
+  revisionId?: string;
+}
+
 
 /**
- * Simplified page fetching hook using Jotai + SWR
- * Replaces the complex useSWRMUTxCurrentPage with cleaner state management
+ * Simplified page fetching hook using Jotai state management
+ * All state is managed through atoms for consistent global state
  */
 export const useFetchCurrentPage = (): {
   fetchCurrentPage: (currentPathname?: string) => Promise<IPagePopulatedToShowRevision | null>,
@@ -24,37 +34,30 @@ export const useFetchCurrentPage = (): {
 } => {
   const { data: shareLinkId } = useShareLinkId();
 
-  const [isLoading, setIsLoading] = useState(false);
-  const [error, setError] = useState<Error | null>(null);
+  // Use atoms for state instead of local state
+  const isLoading = useAtomValue(pageLoadingAtom);
+  const error = useAtomValue(pageErrorAtom);
 
   const fetchCurrentPage = useAtomCallback(
     useCallback(async(get, set, currentPathname?: string) => {
       const currentPath = currentPathname || (isClient() ? decodeURIComponent(window.location.pathname) : '');
 
-      // Determine target pageId from current path
-      // const targetPageId = isPermalink(currentPath) ? removeHeadingSlash(currentPath) : null;
       const currentPageId = get(currentPageIdAtom);
 
-      // NOTE: PageId sync is now handled by useSameRouteNavigation to prevent conflicts
-      // if (currentPageId !== targetPageId) {
-      //   currentPageId = targetPageId || undefined;
-      //   set(currentPageIdAtom, currentPageId);
-      // }
-
       // Get URL parameter for specific revisionId - only when needed
       const revisionId = isClient() && window.location.search
         ? new URLSearchParams(window.location.search).get('revisionId') || undefined
         : undefined;
 
-      setIsLoading(true);
-      setError(null);
+      set(pageLoadingAtom, true);
+      set(pageErrorAtom, null);
 
       try {
-        // Build API parameters efficiently
-        const apiParams: Record<string, string> = {};
-
-        if (shareLinkId) apiParams.shareLinkId = shareLinkId;
-        if (revisionId) apiParams.revisionId = revisionId;
+        // Build API parameters with type safety
+        const apiParams: PageApiParams = {
+          ...(shareLinkId && { shareLinkId }),
+          ...(revisionId && { revisionId }),
+        };
 
         // Use pageId when available, fallback to path
         if (currentPageId) {
@@ -86,15 +89,14 @@ export const useFetchCurrentPage = (): {
         return newData;
       }
       catch (err) {
-        const errorMsg = err instanceof Error ? err.message.toLowerCase() : '';
+        const error = err instanceof Error ? err : new Error('Unknown error occurred');
+        const errorMsg = error.message.toLowerCase();
 
         // Handle specific error types with batch updates
         if (errorMsg.includes('not found') || errorMsg.includes('404')) {
-          // Batch update for 404 errors
           set(pageNotFoundAtom, true);
           set(currentPageDataAtom, undefined);
 
-          // Determine if page is creatable
           if (currentPath) {
             set(pageNotCreatableAtom, !isCreatablePage(currentPath));
           }
@@ -102,18 +104,17 @@ export const useFetchCurrentPage = (): {
         }
 
         if (errorMsg.includes('forbidden') || errorMsg.includes('403')) {
-          // Batch update for 403 errors
           set(pageNotFoundAtom, false);
           set(pageNotCreatableAtom, true);
           set(currentPageDataAtom, undefined);
           return null;
         }
 
-        setError(new Error('Failed to fetch current page'));
+        set(pageErrorAtom, new Error(`Failed to fetch current page: ${error.message}`));
         return null;
       }
       finally {
-        setIsLoading(false);
+        set(pageLoadingAtom, false);
       }
     }, [shareLinkId]),
   );

apps/app/src/states/page/use-page-loading.ts

@@ -0,0 +1,17 @@
+import { useAtomValue } from 'jotai';
+
+import { pageLoadingAtom, pageErrorAtom } from './internal-atoms';
+
+/**
+ * Hook to access current page loading state
+ * Provides consistent loading and error state across the application
+ */
+export const useCurrentPageLoading = (): {
+  isLoading: boolean;
+  error: Error | null;
+} => {
+  const isLoading = useAtomValue(pageLoadingAtom);
+  const error = useAtomValue(pageErrorAtom);
+
+  return { isLoading, error };
+};