Browse Source

Refactor navigation hooks: improve handling of client-side routing and data fetching logic

Yuki Takei 2 months ago
parent
commit
0b0426949b

+ 2 - 2
apps/app/src/pages/[[...path]]/index.page.tsx

@@ -94,7 +94,7 @@ const EditablePageEffects = dynamic(
 type Props = EachProps | InitialProps;
 
 const isInitialProps = (props: Props): props is InitialProps =>
-  props.nextjsRoutingType === NextjsRoutingType.INITIAL;
+  props.nextjsRoutingType !== NextjsRoutingType.SAME_ROUTE;
 
 const Page: NextPageWithLayout<Props> = (props: Props) => {
   // Initialize Jotai atoms with initial data - must be called unconditionally
@@ -124,7 +124,7 @@ const Page: NextPageWithLayout<Props> = (props: Props) => {
   useSameRouteNavigation();
   useShallowRouting(props);
 
-  // Fetch page data on client-side
+  // Fetch page data on client-side when SSR is skipped or navigating from outside
   useInitialCSRFetch({
     nextjsRoutingType: props.nextjsRoutingType,
     skipSSR: isInitialProps(props) ? props.skipSSR : false,

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

@@ -57,11 +57,15 @@ describe('useSameRouteNavigation', () => {
   });
 
   it('should call fetchCurrentPage and mutateEditingMarkdown on path change', async () => {
-    // Arrange
+    // Arrange: Initial render (SSR case - no fetch on initial render)
     mockRouter.asPath = '/initial-path';
     const { rerender } = renderHook(() => useSameRouteNavigation());
 
-    // Act: Simulate navigation
+    // Assert: No fetch on initial render (useRef previousPath is null)
+    expect(mockFetchCurrentPage).not.toHaveBeenCalled();
+    expect(mockSetEditingMarkdown).not.toHaveBeenCalled();
+
+    // Act: Simulate CSR navigation to a new path
     mockRouter.asPath = '/new-path';
     rerender();
 
@@ -78,49 +82,46 @@ describe('useSameRouteNavigation', () => {
   });
 
   it('should not trigger effects if the path does not change', async () => {
-    // Arrange
+    // Arrange: Initial render
     mockRouter.asPath = '/same-path';
     const { rerender } = renderHook(() => useSameRouteNavigation());
-    // call on initial render
-    await waitFor(() => {
-      expect(mockFetchCurrentPage).toHaveBeenCalledTimes(1);
-      expect(mockSetEditingMarkdown).toHaveBeenCalledTimes(1);
-    });
 
-    // Act: Rerender with the same path
+    // Initial render should not trigger fetch (previousPath is null initially)
+    expect(mockFetchCurrentPage).not.toHaveBeenCalled();
+    expect(mockSetEditingMarkdown).not.toHaveBeenCalled();
+
+    // Act: Rerender with the same path (simulates a non-navigation re-render)
     rerender();
 
-    // Assert
-    // A short delay to ensure no async operations are triggered
+    // Assert: Still no fetch because path hasn't changed
     await new Promise((resolve) => setTimeout(resolve, 100));
-    expect(mockFetchCurrentPage).toHaveBeenCalledTimes(1); // Should not be called again
-    expect(mockSetEditingMarkdown).toHaveBeenCalledTimes(1);
+    expect(mockFetchCurrentPage).not.toHaveBeenCalled();
+    expect(mockSetEditingMarkdown).not.toHaveBeenCalled();
   });
 
   it('should not call mutateEditingMarkdown if pageData or revision is null', async () => {
-    // Arrange: first, fetch successfully
+    // Arrange: Initial render
     mockRouter.asPath = '/initial-path';
     const { rerender } = renderHook(() => useSameRouteNavigation());
-    await waitFor(() => {
-      expect(mockFetchCurrentPage).toHaveBeenCalledTimes(1);
-      expect(mockSetEditingMarkdown).toHaveBeenCalledTimes(1);
-    });
 
-    // Arrange: next, fetch fails (returns null)
+    // Initial render should not trigger fetch
+    expect(mockFetchCurrentPage).not.toHaveBeenCalled();
+
+    // Arrange: Mock fetch to return null for the next navigation
     mockFetchCurrentPage.mockResolvedValue(null);
 
-    // Act
+    // Act: Navigate to a new path
     mockRouter.asPath = '/path-with-no-data';
     rerender();
 
     // Assert
     await waitFor(() => {
-      // fetch should be called again
+      // fetch should be called
       expect(mockFetchCurrentPage).toHaveBeenCalledWith({
         path: '/path-with-no-data',
       });
-      // but mutate should not be called again
-      expect(mockSetEditingMarkdown).toHaveBeenCalledTimes(1);
+      // but mutate should not be called because pageData is null
+      expect(mockSetEditingMarkdown).not.toHaveBeenCalled();
     });
   });
 });

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

@@ -1,33 +1,55 @@
-import { useEffect } from 'react';
+import { useEffect, useRef } from 'react';
 import { useRouter } from 'next/router';
 
 import { useFetchCurrentPage, useIsIdenticalPath } from '~/states/page';
 import { useSetEditingMarkdown } from '~/states/ui/editor';
 
 /**
- * This hook is a trigger to fetch page data on client-side navigation.
- * It detects changes in `router.asPath` and calls `fetchCurrentPage`.
- * The responsibility for determining whether to actually fetch data
- * is delegated to `useFetchCurrentPage`.
+ * Hook for handling SAME_ROUTE client-side navigation within [[...path]] route.
+ *
+ * Responsibilities:
+ * - Detects path changes during SAME_ROUTE CSR navigation
+ * - Fetches page data when navigating to a different page within the same route
+ * - Updates editing markdown state with fetched content
+ *
+ * Note: FROM_OUTSIDE initial navigation is handled by useInitialCSRFetch.
+ *
+ * This hook uses useRef to track the previous path, enabling detection of
+ * client-side navigations. On initial render (including FROM_OUTSIDE),
+ * previousPathRef is null, so no fetch is triggered.
  */
 export const useSameRouteNavigation = (): void => {
   const router = useRouter();
+  const previousPathRef = useRef<string | null>(null);
 
   const isIdenticalPath = useIsIdenticalPath();
   const { fetchCurrentPage } = useFetchCurrentPage();
   const setEditingMarkdown = useSetEditingMarkdown();
 
-  // useEffect to trigger data fetching when the path changes
   useEffect(() => {
-    // If the path is identical, do not fetch
+    const currentPath = router.asPath;
+    const previousPath = previousPathRef.current;
+
+    // Update ref for next render
+    previousPathRef.current = currentPath;
+
+    // Skip on initial render (SSR data is already available)
+    if (previousPath === null) return;
+
+    // Skip if path hasn't changed
+    if (previousPath === currentPath) return;
+
+    // Skip if this is an identical path page
     if (isIdenticalPath) return;
 
+    // CSR navigation detected - fetch page data
     const fetch = async () => {
-      const pageData = await fetchCurrentPage({ path: router.asPath });
+      const pageData = await fetchCurrentPage({ path: currentPath });
       if (pageData?.revision?.body != null) {
         setEditingMarkdown(pageData.revision.body);
       }
     };
+
     fetch();
   }, [router.asPath, isIdenticalPath, fetchCurrentPage, setEditingMarkdown]);
 };

+ 22 - 12
apps/app/src/pages/general-page/use-initial-csr-fetch.ts

@@ -1,30 +1,40 @@
 import { useEffect } from 'react';
+import { useRouter } from 'next/router';
 
 import { useFetchCurrentPage } from '~/states/page';
 
 import { NextjsRoutingType } from '../utils/nextjs-routing-utils';
 
 /**
- * useInitialCSRFetch
+ * Hook for handling initial CSR fetch when SSR data is not available.
  *
- * Fetches current page data on client-side by conditionally
+ * Responsibilities:
+ * - Fetches page data on client-side when skipSSR is true
+ *   (e.g., when page content exceeds ssrMaxRevisionBodyLength)
+ * - Fetches page data on client-side when navigating from outside routes (FROM_OUTSIDE)
+ *   (e.g., when navigating from /_search to /page)
+ *
+ * Note: SAME_ROUTE navigation is handled by useSameRouteNavigation.
  */
 export const useInitialCSRFetch = (condition: {
   nextjsRoutingType: NextjsRoutingType;
   skipSSR?: boolean;
 }): void => {
+  const router = useRouter();
   const { fetchCurrentPage } = useFetchCurrentPage();
 
-  // Should fetch page data on client-side or not
-  const shouldFetch =
-    condition.nextjsRoutingType === NextjsRoutingType.FROM_OUTSIDE ||
-    condition.skipSSR;
-
-  // Note: When the nextjsRoutingType is SAME_ROUTE, the data fetching is handled by useSameRouteNavigation
-
   useEffect(() => {
-    if (shouldFetch) {
-      fetchCurrentPage({ force: true });
+    const isFromOutside =
+      condition.nextjsRoutingType === NextjsRoutingType.FROM_OUTSIDE;
+    if (condition.skipSSR || isFromOutside) {
+      // Pass current path to ensure fetching the correct page
+      // (atoms may contain stale data from the previous page)
+      fetchCurrentPage({ force: true, path: router.asPath });
     }
-  }, [fetchCurrentPage, shouldFetch]);
+  }, [
+    fetchCurrentPage,
+    condition.skipSSR,
+    condition.nextjsRoutingType,
+    router.asPath,
+  ]);
 };

+ 1 - 1
apps/app/src/pages/share/[[...path]]/index.page.tsx

@@ -60,7 +60,7 @@ const SharedPage: NextPageWithLayout<Props> = (props: Props) => {
   // Use custom hooks for navigation and routing
   // useSameRouteNavigation();
 
-  // Fetch page data on client-side
+  // Fetch page data on client-side when SSR is skipped
   useInitialCSRFetch({
     nextjsRoutingType: props.nextjsRoutingType,
     skipSSR: isInitialProps(props) ? props.skipSSR : false,