From c04f8a846cc153c7fc7d2ede43c3f048e74c25d6 Mon Sep 17 00:00:00 2001
From: claude code agent 227 <claude_code@vvzvlad.xyz>
Date: Sun, 21 Jun 2026 05:39:35 +0300
Subject: [PATCH] test(page): cover movePage server-side cycle guard (#102)

Adds the missing tests for the #67 guard: self-move and a destination inside the
moved page's subtree both throw BadRequestException before updatePage; a
legitimate move proceeds. Mocks pageRepo + spies getPageBreadCrumbs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 .../core/page/services/page.service.spec.ts   | 121 ++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/apps/server/src/core/page/services/page.service.spec.ts b/apps/server/src/core/page/services/page.service.spec.ts
index ec3a39db..f923c432 100644
--- a/apps/server/src/core/page/services/page.service.spec.ts
+++ b/apps/server/src/core/page/services/page.service.spec.ts
@@ -1,4 +1,7 @@
+import { BadRequestException } from '@nestjs/common';
 import { PageService } from './page.service';
+import { MovePageDto } from '../dto/move-page.dto';
+import { Page } from '@docmost/db/types/entity.types';
 
 // Direct instantiation with stub deps. The Test.createTestingModule form failed
 // to resolve the @InjectKysely()/@InjectQueue() tokens at compile(), and this
@@ -26,4 +29,122 @@ describe('PageService', () => {
   it('should be defined', () => {
     expect(service).toBeDefined();
   });
+
+  describe('movePage cycle guard (#67)', () => {
+    // A valid fractional-indexing key — movePage validates `position` by feeding
+    // it to generateJitteredKeyBetween(position, null) before anything else.
+    const VALID_POSITION = 'a0';
+    const SPACE_ID = 'space-1';
+
+    // Build a PageService whose pageRepo (findById/updatePage) and own
+    // getPageBreadCrumbs are mockable, while every other collaborator stays a
+    // bare stub. We only need to drive the three cycle-guard branches, so we
+    // mock minimally rather than standing up the whole DI graph.
+    const makeService = (overrides?: {
+      breadcrumbs?: Array<{ id: string }>;
+    }) => {
+      const pageRepo = {
+        // Destination parent lookup: a valid, non-deleted, same-space page.
+        findById: jest.fn().mockResolvedValue({
+          id: 'dest-parent',
+          deletedAt: null,
+          spaceId: SPACE_ID,
+        }),
+        // numUpdatedRows must be 1n so the #64 phantom-broadcast gate passes and
+        // movePage proceeds to emit PAGE_MOVED instead of early-returning.
+        updatePage: jest.fn().mockResolvedValue({ numUpdatedRows: 1n }),
+      };
+
+      const eventEmitter = { emit: jest.fn() };
+
+      const svc = new PageService(
+        pageRepo as any, // pageRepo
+        {} as any, // pagePermissionRepo
+        {} as any, // attachmentRepo
+        {} as any, // db
+        {} as any, // storageService
+        {} as any, // attachmentQueue
+        {} as any, // aiQueue
+        {} as any, // generalQueue
+        eventEmitter as any, // eventEmitter
+        {} as any, // collaborationGateway
+        {} as any, // watcherService
+        {} as any, // transclusionService
+      );
+
+      // getPageBreadCrumbs is a method on PageService itself (it runs a recursive
+      // ancestor CTE against the db). Spy on the instance method so we can return
+      // a synthetic ancestor chain without a real database.
+      jest
+        .spyOn(svc, 'getPageBreadCrumbs')
+        .mockResolvedValue((overrides?.breadcrumbs ?? []) as any);
+
+      return { svc, pageRepo, eventEmitter };
+    };
+
+    // movePage takes `movedPage` as a param. Keep its parentPageId distinct from
+    // the dto's parentPageId so the re-parent branch (and thus the cycle guard)
+    // actually runs instead of short-circuiting to a same-parent reorder.
+    const makeMovedPage = (): Page =>
+      ({
+        id: 'page-1',
+        parentPageId: 'old-parent',
+        spaceId: SPACE_ID,
+        workspaceId: 'ws-1',
+        slugId: 'slug-1',
+        title: 'Page 1',
+        icon: null,
+      }) as any;
+
+    it('rejects a self-move (parentPageId === pageId) without updating', async () => {
+      const { svc, pageRepo } = makeService();
+      const dto: MovePageDto = {
+        pageId: 'page-1',
+        position: VALID_POSITION,
+        parentPageId: 'page-1', // moving the page into itself
+      };
+
+      await expect(svc.movePage(dto, makeMovedPage())).rejects.toThrow(
+        BadRequestException,
+      );
+      expect(pageRepo.updatePage).not.toHaveBeenCalled();
+    });
+
+    it('rejects moving a page into its own subtree (cycle) before updating', async () => {
+      // Destination's ancestor chain includes the page being moved -> the
+      // destination lives inside the moved page's subtree -> cycle.
+      const { svc, pageRepo } = makeService({
+        breadcrumbs: [
+          { id: 'dest-parent' },
+          { id: 'page-1' }, // the moved page appears among the destination's ancestors
+          { id: 'root' },
+        ],
+      });
+      const dto: MovePageDto = {
+        pageId: 'page-1',
+        position: VALID_POSITION,
+        parentPageId: 'dest-parent',
+      };
+
+      await expect(svc.movePage(dto, makeMovedPage())).rejects.toThrow(
+        BadRequestException,
+      );
+      expect(pageRepo.updatePage).not.toHaveBeenCalled();
+    });
+
+    it('allows a legitimate move when the destination is not in the subtree', async () => {
+      // Destination's ancestor chain does NOT contain the moved page -> no cycle.
+      const { svc, pageRepo } = makeService({
+        breadcrumbs: [{ id: 'dest-parent' }, { id: 'root' }],
+      });
+      const dto: MovePageDto = {
+        pageId: 'page-1',
+        position: VALID_POSITION,
+        parentPageId: 'dest-parent',
+      };
+
+      await expect(svc.movePage(dto, makeMovedPage())).resolves.not.toThrow();
+      expect(pageRepo.updatePage).toHaveBeenCalledTimes(1);
+    });
+  });
 });