Просмотр исходного кода

fix: improve handling of empty pages and update related logic

Yuki Takei 3 месяцев назад
Родитель
Сommit
933a6b9d9f

+ 1 - 1
apps/app/src/features/collaborative-editor/side-effects/index.ts

@@ -15,7 +15,7 @@ export const useCurrentPageYjsDataAutoLoadEffect = (): void => {
   const pageId = useCurrentPageId();
   const currentPage = useCurrentPageData();
   const isGuestUser = useIsGuestUser();
-  const isNotFound = usePageNotFound();
+  const isNotFound = usePageNotFound(false);
 
   // Optimized effects with minimal dependencies
   useEffect(() => {

+ 10 - 16
apps/app/src/server/routes/apiv3/page/index.ts

@@ -1,3 +1,5 @@
+import type { Readable } from 'node:stream';
+import { pipeline } from 'node:stream/promises';
 import type {
   IDataWithMeta,
   IPage,
@@ -19,10 +21,8 @@ import { convertToNewAffiliationPath } from '@growi/core/dist/utils/page-path-ut
 import { normalizePath } from '@growi/core/dist/utils/path-utils';
 import type { HydratedDocument } from 'mongoose';
 import mongoose from 'mongoose';
-import path from 'path';
+import path from 'pathe';
 import sanitize from 'sanitize-filename';
-import type { Readable } from 'stream';
-import { pipeline } from 'stream/promises';
 
 import { SupportedAction, SupportedTargetModel } from '~/interfaces/activity';
 import type { IPageGrantData } from '~/interfaces/page';
@@ -640,7 +640,7 @@ module.exports = (crowi: Crowi) => {
    *                    isGrantNormalized:
    *                      type: boolean
    *          400:
-   *            description: Bad request. Page is unreachable or empty.
+   *            description: Bad request. Page is unreachable.
    *          500:
    *            description: Internal server error.
    */
@@ -656,15 +656,12 @@ module.exports = (crowi: Crowi) => {
       const Page = mongoose.model<IPage, PageModel>('Page');
       const pageGrantService = crowi.pageGrantService as IPageGrantService;
 
-      const page = await Page.findByIdAndViewer(pageId, req.user, null, false);
+      const page = await Page.findByIdAndViewer(pageId, req.user, null, true);
 
       if (page == null) {
         // Empty page should not be related to grant API
         return res.apiv3Err(
-          new ErrorV3(
-            'Page is unreachable or empty.',
-            'page_unreachable_or_empty',
-          ),
+          new ErrorV3('Page is unreachable', 'page_unreachable'),
           400,
         );
       }
@@ -708,7 +705,7 @@ module.exports = (crowi: Crowi) => {
         getIdForRef(page.parent),
         req.user,
         null,
-        false,
+        true,
       );
 
       // user isn't allowed to see parent's grant
@@ -866,7 +863,7 @@ module.exports = (crowi: Crowi) => {
    *                     items:
    *                       type: string
    *         400:
-   *           description: Bad request. Page is unreachable or empty.
+   *           description: Bad request. Page is unreachable.
    *         500:
    *           description: Internal server error.
    */
@@ -880,15 +877,12 @@ module.exports = (crowi: Crowi) => {
       const { pageId } = req.query;
 
       const Page = mongoose.model<IPage, PageModel>('Page');
-      const page = await Page.findByIdAndViewer(pageId, req.user, null);
+      const page = await Page.findByIdAndViewer(pageId, req.user, null, true);
 
       if (page == null) {
         // Empty page should not be related to grant API
         return res.apiv3Err(
-          new ErrorV3(
-            'Page is unreachable or empty.',
-            'page_unreachable_or_empty',
-          ),
+          new ErrorV3('Page is unreachable', 'page_unreachable'),
           400,
         );
       }

+ 6 - 1
apps/app/src/states/page/hooks.ts

@@ -43,7 +43,12 @@ export const useCurrentPageId = (includeEmpty: boolean = false) => {
 
 export const useCurrentPageData = () => useAtomValue(currentPageDataAtom);
 
-export const usePageNotFound = () => useAtomValue(pageNotFoundAtom);
+export const usePageNotFound = (includeEmpty: boolean = true) => {
+  const isPageNotFound = useAtomValue(pageNotFoundAtom);
+  const emptyPageId = useAtomValue(currentPageEmptyIdAtom);
+
+  return includeEmpty ? isPageNotFound || emptyPageId != null : isPageNotFound;
+};
 
 export const useIsIdenticalPath = () => useAtomValue(isIdenticalPathAtom);
 

+ 36 - 25
apps/app/src/states/page/use-fetch-current-page.spec.tsx

@@ -121,9 +121,9 @@ describe('useFetchCurrentPage - Integration Test', () => {
 
   const mockApiResponse = (
     page: IPagePopulatedToShowRevision,
-  ): AxiosResponse<{ page: IPagePopulatedToShowRevision }> => {
+  ): AxiosResponse<{ page: IPagePopulatedToShowRevision; meta: unknown }> => {
     return {
-      data: { page },
+      data: { page, meta: {} },
       status: 200,
       statusText: 'OK',
       headers: {},
@@ -782,6 +782,7 @@ describe('useFetchCurrentPage - Integration Test', () => {
       });
       expect(store.get(currentPageDataAtom)).toBeUndefined();
       expect(store.get(currentPageEntityIdAtom)).toBeUndefined();
+      expect(store.get(currentPageEmptyIdAtom)).toBeUndefined();
       expect(store.get(remoteRevisionBodyAtom)).toBeUndefined();
     });
   });
@@ -849,25 +850,30 @@ describe('useFetchCurrentPage - Integration Test', () => {
     });
   });
 
-  it('should set emptyPageId when page not found with IPageInfoForEmpty', async () => {
-    // Arrange: Mock API rejection with ErrorV3 and IPageInfoForEmpty args
+  it('should set emptyPageId when page not found with IPageInfoForEmpty in meta', async () => {
+    // Arrange: Mock API response with null page and IPageInfoForEmpty meta
     const emptyPageId = 'empty123';
-    const notFoundErrorWithEmptyPage = {
-      code: 'not_found',
-      message: 'Page not found',
-      args: {
-        isNotFound: true,
-        isForbidden: false,
-        isEmpty: true, // Required for isIPageInfoForEmpty check
-        emptyPageId,
+    const notFoundResponseWithEmptyPage = {
+      data: {
+        page: null,
+        meta: {
+          isNotFound: true,
+          isForbidden: false,
+          isEmpty: true, // Required for isIPageInfoForEmpty check
+          emptyPageId,
+        },
       },
-    } as const;
-    mockedApiv3Get.mockRejectedValueOnce([notFoundErrorWithEmptyPage]);
+      status: 200,
+      statusText: 'OK',
+      headers: {},
+      config: {} as AxiosResponse['config'],
+    };
+    mockedApiv3Get.mockResolvedValueOnce(notFoundResponseWithEmptyPage);
 
     const { result } = renderHookWithProvider();
     await result.current.fetchCurrentPage({ path: '/empty/page' });
 
-    // Assert: emptyPageId should be set
+    // Assert: emptyPageId should be set from meta
     await waitFor(() => {
       expect(store.get(pageLoadingAtom)).toBe(false);
       expect(store.get(pageNotFoundAtom)).toBe(true);
@@ -879,17 +885,22 @@ describe('useFetchCurrentPage - Integration Test', () => {
   });
 
   it('should not set emptyPageId when page not found without IPageInfoForEmpty', async () => {
-    // Arrange: Mock API rejection with ErrorV3 but without emptyPageId
-    const notFoundErrorWithoutEmptyPage = {
-      code: 'not_found',
-      message: 'Page not found',
-      args: {
-        isNotFound: true,
-        isForbidden: false,
-        // No emptyPageId property
+    // Arrange: Mock API response with null page and IPageNotFoundInfo meta without emptyPageId
+    const notFoundResponseWithoutEmptyPage = {
+      data: {
+        page: null,
+        meta: {
+          isNotFound: true,
+          isForbidden: false,
+          // No emptyPageId property - not IPageInfoForEmpty
+        },
       },
-    } as const;
-    mockedApiv3Get.mockRejectedValueOnce([notFoundErrorWithoutEmptyPage]);
+      status: 200,
+      statusText: 'OK',
+      headers: {},
+      config: {} as AxiosResponse['config'],
+    };
+    mockedApiv3Get.mockResolvedValueOnce(notFoundResponseWithoutEmptyPage);
 
     const { result } = renderHookWithProvider();
     await result.current.fetchCurrentPage({ path: '/regular/not/found' });

+ 22 - 16
apps/app/src/states/page/use-fetch-current-page.ts

@@ -1,5 +1,6 @@
 import { useCallback } from 'react';
 import {
+  type IPageNotFoundInfo,
   type IPagePopulatedToShowRevision,
   isIPageInfoForEmpty,
   isIPageNotFoundInfo,
@@ -37,6 +38,10 @@ type FetchPageArgs = {
   force?: true;
 };
 
+type FetchedPageResult =
+  | { page: IPagePopulatedToShowRevision; meta: unknown }
+  | { page: null; meta: IPageNotFoundInfo };
+
 /**
  * Process path to handle URL decoding and hash fragment removal
  *
@@ -233,16 +238,22 @@ export const useFetchCurrentPage = (): {
         }
 
         try {
-          const { data } = await apiv3Get<{
-            page: IPagePopulatedToShowRevision;
-          }>('/page', params);
-          const { page: newData } = data;
-
-          set(currentPageDataAtom, newData);
-          set(currentPageEntityIdAtom, newData._id);
-          set(currentPageEmptyIdAtom, undefined);
-          set(pageNotFoundAtom, false);
-          set(isForbiddenAtom, false);
+          const { data } = await apiv3Get<FetchedPageResult>('/page', params);
+          const { page: newData, meta } = data;
+
+          console.log('Fetched page data:', { newData, meta });
+
+          set(currentPageDataAtom, newData ?? undefined);
+          set(currentPageEntityIdAtom, newData?._id);
+          set(
+            currentPageEmptyIdAtom,
+            isIPageInfoForEmpty(meta) ? meta.emptyPageId : undefined,
+          );
+          set(pageNotFoundAtom, isIPageNotFoundInfo(meta));
+          set(
+            isForbiddenAtom,
+            isIPageNotFoundInfo(meta) ? (meta.isForbidden ?? false) : false,
+          );
 
           // Mutate PageInfo to refetch latest metadata including latestRevisionId
           mutatePageInfo();
@@ -264,12 +275,7 @@ export const useFetchCurrentPage = (): {
               set(isForbiddenAtom, error.args.isForbidden ?? false);
               set(currentPageDataAtom, undefined);
               set(currentPageEntityIdAtom, undefined);
-              set(
-                currentPageEmptyIdAtom,
-                isIPageInfoForEmpty(error.args)
-                  ? error.args.emptyPageId
-                  : undefined,
-              );
+              set(currentPageEmptyIdAtom, undefined);
               set(remoteRevisionBodyAtom, undefined);
             }
           }

+ 2 - 5
apps/app/src/states/ui/page-abilities.ts

@@ -123,15 +123,12 @@ export const useIsAbleToChangeEditorMode = (): boolean => {
  */
 export const useIsAbleToShowPageAuthors = (): boolean => {
   const pageId = useCurrentPageId();
-  const isNotFound = usePageNotFound();
   const pagePath = useCurrentPagePath();
 
-  const includesUndefined = [pageId, pagePath, isNotFound].some(
-    (v) => v === undefined,
-  );
+  const includesUndefined = [pageId, pagePath].some((v) => v === undefined);
   if (includesUndefined) return false;
 
-  const isPageExist = pageId != null && !isNotFound;
+  const isPageExist = pageId != null;
   const isUsersTopPagePath = pagePath != null && isUsersTopPage(pagePath);
 
   return isPageExist && !isUsersTopPagePath;