fix(share): guard alias swap/rename against concurrent-delete race; share unique-violation helpers

Address PR #227 re-review (comment 2193).

- Stability: `updatePageId`/`updateAlias` now `executeTakeFirstOrThrow`, so a row
  reaped by a concurrent `removeAlias` between the read and the UPDATE (READ
  COMMITTED) raises `NoResultError` instead of returning `undefined`. The service
  maps that to a retryable `ConflictException` (`ALIAS_PAGE_RACE`) rather than a
  200-without-alias (swap) or a generic 400 from `undefined.id` (rename). Tests
  cover both branches.
- Simplification: drop the redundant secondary "unexpected unique index" warn and
  the now-unused `UNIQUE_ALIAS_INDEX` const (the constraint name is already logged
  unconditionally; both index branches still distinguish "Alias already taken" vs
  ALIAS_PAGE_RACE).
- Architecture: extract `isUniqueViolation`/`violatedConstraint` into
  database/utils.ts; adopt them in the share-alias service and favorite.repo
  (the bare `23505` check). ai-agent-roles (#222) is on a separate unmerged branch
  and should adopt them after #227 merges (noted at the helpers). Helper unit test
  added.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -55,6 +55,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   page), so any previously-live duplicate `/l/<old>` link begins returning the
   generic 404 after upgrade — intended, but not undoable by `down()`. (#226,
   #227)
+- **Typing a custom address already used by another page no longer looks like a
+  dead end.** The share modal previously flagged such a name with a red "This
+  address is already in use" error, hiding the fact that saving offers to MOVE
+  the address to the current page. The field now shows an informational hint —
+  "This address is in use. Saving will move it to this page." — and keeps Save
+  enabled, so the existing reassign-confirm flow (`409 ALIAS_REASSIGN_REQUIRED` →
+  "Move custom address?") is discoverable instead of reading as terminal. (#227)
 
 ## [0.94.0] - 2026-06-26
 

apps/client/public/locales/en-US/translation.json

@@ -1333,6 +1333,7 @@
   "A short, memorable link you can point at any shared page.": "A short, memorable link you can point at any shared page.",
   "Use 2-60 lowercase letters, digits and hyphens": "Use 2-60 lowercase letters, digits and hyphens",
   "This address is already in use": "This address is already in use",
+  "This address is in use. Saving will move it to this page.": "This address is in use. Saving will move it to this page.",
   "Move custom address?": "Move custom address?",
   "Move here": "Move here",
   "The address \"{{alias}}\" currently points to \"{{title}}\". Move it to this page?": "The address \"{{alias}}\" currently points to \"{{title}}\". Move it to this page?",

apps/client/public/locales/ru-RU/translation.json

@@ -1190,6 +1190,7 @@
   "A short, memorable link you can point at any shared page.": "Короткая запоминающаяся ссылка, которую можно направить на любую опубликованную страницу.",
   "Use 2-60 lowercase letters, digits and hyphens": "Используйте 2–60 строчных букв, цифр и дефисов",
   "This address is already in use": "Этот адрес уже занят",
+  "This address is in use. Saving will move it to this page.": "Этот адрес уже используется. При сохранении он будет перемещён на эту страницу.",
   "Move custom address?": "Переместить пользовательский адрес?",
   "Move here": "Переместить сюда",
   "The address \"{{alias}}\" currently points to \"{{title}}\". Move it to this page?": "Адрес «{{alias}}» сейчас указывает на «{{title}}». Переместить его на эту страницу?",

apps/client/src/features/share/components/share-alias-section.test.tsx

@@ -0,0 +1,149 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { render, screen, fireEvent, waitFor } from "@testing-library/react";
+import { MantineProvider } from "@mantine/core";
+import type { IShareAlias } from "@/features/share/types/share.types";
+
+// matchMedia / storage are stubbed globally in vitest.setup.ts.
+
+// The mutation + query hooks reach react-query/network; the availability probe
+// hits the API. Stub them so the section renders in isolation and we can drive
+// the exact branches (taken name -> hint, 409 -> reassign modal).
+const setMutateAsync = vi.fn();
+let currentAlias: IShareAlias | null = null;
+let availabilityResult: {
+  valid: boolean;
+  available: boolean;
+  currentPageId: string | null;
+} = { valid: true, available: true, currentPageId: null };
+
+vi.mock("@/features/share/queries/share-query.ts", () => ({
+  useShareAliasForPageQuery: () => ({ data: currentAlias }),
+  useSetShareAliasMutation: () => ({
+    mutateAsync: setMutateAsync,
+    isPending: false,
+  }),
+  useRemoveShareAliasMutation: () => ({
+    mutateAsync: vi.fn(),
+    isPending: false,
+  }),
+}));
+
+vi.mock("@/features/share/services/share-service.ts", () => ({
+  checkShareAliasAvailability: vi.fn(async () => availabilityResult),
+}));
+
+import ShareAliasSection from "./share-alias-section";
+
+const aliasRow = (alias: string, pageId: string): IShareAlias => ({
+  id: `alias-${alias}`,
+  workspaceId: "ws-1",
+  alias,
+  pageId,
+  creatorId: "user-1",
+  createdAt: new Date().toISOString(),
+  updatedAt: new Date().toISOString(),
+});
+
+function renderSection(pageId = "page-Y") {
+  return render(
+    <MantineProvider>
+      <ShareAliasSection pageId={pageId} readOnly={false} />
+    </MantineProvider>,
+  );
+}
+
+describe("ShareAliasSection — taken-name handling is never a dead end", () => {
+  beforeEach(() => {
+    setMutateAsync.mockReset();
+    currentAlias = null;
+    availabilityResult = { valid: true, available: true, currentPageId: null };
+  });
+
+  it("shows a 'will move it here' HINT (not a terminal error) when the name belongs to another page, and keeps Save enabled", async () => {
+    // Page Y already owns "bee"; the user retypes a name owned by page X.
+    currentAlias = aliasRow("bee", "page-Y");
+    availabilityResult = {
+      valid: true,
+      available: false,
+      currentPageId: "page-X",
+    };
+
+    renderSection("page-Y");
+    const input = screen.getByPlaceholderText("my-page") as HTMLInputElement;
+    fireEvent.change(input, { target: { value: "test2" } });
+
+    // The reassign hint replaces the old dead-end red error.
+    await waitFor(
+      () =>
+        expect(
+          screen.getByText(
+            "This address is in use. Saving will move it to this page.",
+          ),
+        ).toBeDefined(),
+      { timeout: 2000 },
+    );
+    // The old terminal "already in use" error must NOT be shown.
+    expect(screen.queryByText("This address is already in use")).toBeNull();
+
+    // Save stays enabled so the confirm-reassign flow can run.
+    const saveBtn = screen.getByRole("button", {
+      name: "Save",
+    }) as HTMLButtonElement;
+    expect(saveBtn.disabled).toBe(false);
+  });
+
+  it("opens the reassign-confirm modal on a 409 ALIAS_REASSIGN_REQUIRED (path forward, not a dead end)", async () => {
+    currentAlias = aliasRow("bee", "page-Y");
+    availabilityResult = {
+      valid: true,
+      available: false,
+      currentPageId: "page-X",
+    };
+    // The server rejects the un-confirmed save asking the client to confirm.
+    setMutateAsync.mockRejectedValueOnce({
+      status: 409,
+      response: {
+        status: 409,
+        data: {
+          code: "ALIAS_REASSIGN_REQUIRED",
+          currentPageId: "page-X",
+          currentPageTitle: "Alias Test Page X",
+        },
+      },
+    });
+
+    renderSection("page-Y");
+    const input = screen.getByPlaceholderText("my-page") as HTMLInputElement;
+    fireEvent.change(input, { target: { value: "test2" } });
+
+    const saveBtn = screen.getByRole("button", {
+      name: "Save",
+    }) as HTMLButtonElement;
+    await waitFor(() => expect(saveBtn.disabled).toBe(false), {
+      timeout: 2000,
+    });
+    fireEvent.click(saveBtn);
+
+    // First save sent WITHOUT confirmReassign.
+    await waitFor(() =>
+      expect(setMutateAsync).toHaveBeenCalledWith(
+        expect.objectContaining({ alias: "test2", confirmReassign: false }),
+      ),
+    );
+
+    // The "Move custom address?" confirm modal must appear (the path forward).
+    await waitFor(() =>
+      expect(screen.getByText("Move custom address?")).toBeDefined(),
+    );
+    expect(screen.getByRole("button", { name: "Move here" })).toBeDefined();
+
+    // Confirming retries WITH confirmReassign: true.
+    setMutateAsync.mockResolvedValueOnce(aliasRow("test2", "page-Y"));
+    fireEvent.click(screen.getByRole("button", { name: "Move here" }));
+    await waitFor(() =>
+      expect(setMutateAsync).toHaveBeenCalledWith(
+        expect.objectContaining({ alias: "test2", confirmReassign: true }),
+      ),
+    );
+  });
+});

apps/client/src/features/share/components/share-alias-section.tsx

@@ -120,8 +120,13 @@ export default function ShareAliasSection({
   };
 
   const showInvalid = normalized.length > 0 && !isValid;
-  const showTaken =
-    isValid && !unchanged && availability && !availability.available;
+  // The typed name is already in use by ANOTHER page. This is NOT a dead end:
+  // hitting Save triggers the server's 409 `ALIAS_REASSIGN_REQUIRED` and opens
+  // the "Move custom address?" confirm modal that retargets the address here.
+  // So surface it as an informational hint (not a terminal red error) and keep
+  // Save enabled, instead of looking like the address is unusable.
+  const reassignable =
+    isValid && !unchanged && !!availability && !availability.available;
 
   // The slug prefix (e.g. "docs.example.com/l/") is static for the session.
   const prefixLabel = aliasPrefixLabel();
@@ -198,9 +203,12 @@ export default function ShareAliasSection({
         error={
           showInvalid
             ? t("Use 2-60 lowercase letters, digits and hyphens")
-            : showTaken
-              ? t("This address is already in use")
-              : undefined
+            : undefined
+        }
+        description={
+          reassignable
+            ? t("This address is in use. Saving will move it to this page.")
+            : undefined
         }
       />
 

apps/server/src/core/share/share-alias.service.spec.ts

@@ -1,4 +1,5 @@
 import { BadRequestException, ConflictException } from '@nestjs/common';
+import { NoResultError } from 'kysely';
 import { ShareAliasService } from './share-alias.service';
 
 /**
@@ -355,6 +356,68 @@ describe('ShareAliasService', () => {
       }
     });
 
+    it('maps a concurrent-delete race in the SWAP branch to a retryable 409 (not a 200-without-alias)', async () => {
+      const { service, shareAliasRepo } = makeService();
+      // Name points at another page; reassign confirmed -> swap branch.
+      shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue({
+        id: 'a-1',
+        alias: 'foo',
+        pageId: 'p-other',
+      });
+      // A concurrent removeAlias deleted the row between read and UPDATE, so the
+      // repo's executeTakeFirstOrThrow finds 0 rows and throws NoResultError.
+      shareAliasRepo.updatePageId.mockRejectedValue(
+        new NoResultError({} as any),
+      );
+
+      try {
+        await service.setAlias({
+          workspaceId: 'ws-1',
+          pageId: 'p-1',
+          creatorId: 'u-1',
+          alias: 'foo',
+          confirmReassign: true,
+        });
+        fail('expected ConflictException');
+      } catch (err) {
+        // Crucially NOT a resolved 200 carrying `undefined` as the alias.
+        expect(err).toBeInstanceOf(ConflictException);
+        expect((err as ConflictException).getResponse()).toMatchObject({
+          code: 'ALIAS_PAGE_RACE',
+        });
+      }
+    });
+
+    it('maps a concurrent-delete race in the RENAME branch to a retryable 409 (not a generic 400)', async () => {
+      const { service, shareAliasRepo } = makeService();
+      // New slug is free, but the page already owns an alias we rename in place.
+      shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined);
+      shareAliasRepo.findByPageId.mockResolvedValue({
+        id: 'a-1',
+        alias: 'te',
+        pageId: 'p-1',
+      });
+      // The row vanished before the UPDATE; repo throws NoResultError rather
+      // than returning undefined (which would dereference undefined.id -> 400).
+      shareAliasRepo.updateAlias.mockRejectedValue(new NoResultError({} as any));
+
+      try {
+        await service.setAlias({
+          workspaceId: 'ws-1',
+          pageId: 'p-1',
+          creatorId: 'u-1',
+          alias: 'ted',
+        });
+        fail('expected ConflictException');
+      } catch (err) {
+        expect(err).toBeInstanceOf(ConflictException);
+        expect(err).not.toBeInstanceOf(BadRequestException);
+        expect((err as ConflictException).getResponse()).toMatchObject({
+          code: 'ALIAS_PAGE_RACE',
+        });
+      }
+    });
+
     it('maps a non-unique-violation db error to BadRequest (Failed to set alias)', async () => {
       const { service, shareAliasRepo } = makeService();
       shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined);

apps/server/src/core/share/share-alias.service.ts

@@ -11,21 +11,21 @@ import { Page, ShareAlias } from '@docmost/db/types/entity.types';
 import { isValidShareAlias, normalizeShareAlias } from './share-alias.util';
 import { InjectKysely } from 'nestjs-kysely';
 import { KyselyDB } from '@docmost/db/types/kysely.types';
-import { executeTx } from '@docmost/db/utils';
-
-/** Postgres unique_violation. Two unique indexes can raise it on this table. */
-const PG_UNIQUE_VIOLATION = '23505';
+import {
+  executeTx,
+  isUniqueViolation,
+  violatedConstraint,
+} from '@docmost/db/utils';
+import { NoResultError } from 'kysely';
 
 /**
- * Unique index names from the share_aliases migrations. The `postgres@3.x`
- * driver (kysely-postgres-js) surfaces the violated constraint as
- * `err.constraint_name` (NOT `.constraint`); we keep `.constraint` only as a
- * defensive fallback for other drivers.
- *   - ALIAS:  `(workspace_id, alias)`  -> the vanity NAME is taken.
+ * Unique index name from the share_aliases migrations whose violation we map to
+ * a DISTINCT, non-misleading outcome:
  *   - PAGE_ID: partial `(workspace_id, page_id) WHERE page_id IS NOT NULL`
  *             -> a concurrent writer already gave THIS page an alias.
+ * The `(workspace_id, alias)` index (the vanity NAME being taken) needs no
+ * constant: it is the default "Alias already taken" mapping.
  */
-const UNIQUE_ALIAS_INDEX = 'share_aliases_workspace_id_alias_unique';
 const UNIQUE_PAGE_ID_INDEX = 'share_aliases_workspace_id_page_id_unique';
 
 export interface ResolvedAliasTarget {
@@ -171,11 +171,23 @@ export class ShareAliasService {
       ) {
         throw err;
       }
+      // The row we read was deleted (concurrent `removeAlias`) before our UPDATE
+      // matched it, so `executeTakeFirstOrThrow` found no row. Surface a
+      // retryable conflict instead of a 200-without-alias (swap branch) or a
+      // generic 400 from dereferencing `undefined.id` (rename branch).
+      if (err instanceof NoResultError) {
+        this.logger.warn(
+          'share alias update matched no row (concurrent-delete race)',
+        );
+        throw new ConflictException({
+          message: 'The address changed concurrently, please retry',
+          code: 'ALIAS_PAGE_RACE',
+        });
+      }
       // A unique index fired. Which one decides the message — always log the
       // constraint so the race is diagnosable.
-      if (err?.code === PG_UNIQUE_VIOLATION) {
-        const constraint: string | undefined =
-          err?.constraint_name ?? err?.constraint;
+      if (isUniqueViolation(err)) {
+        const constraint = violatedConstraint(err);
         this.logger.warn(
           `share alias unique violation on ${constraint ?? '<unknown>'}`,
         );
@@ -189,13 +201,8 @@ export class ShareAliasService {
             code: 'ALIAS_PAGE_RACE',
           });
         }
-        // `(workspace_id, alias)` (UNIQUE_ALIAS_INDEX) or any other/unknown
-        // unique index: treat as the vanity name being claimed first.
-        if (constraint && constraint !== UNIQUE_ALIAS_INDEX) {
-          this.logger.warn(
-            `unexpected unique index ${constraint} mapped to "Alias already taken"`,
-          );
-        }
+        // `(workspace_id, alias)` or any other/unknown unique index: treat as
+        // the vanity name being claimed first.
         throw new ConflictException({ message: 'Alias already taken' });
       }
       this.logger.error(err);

apps/server/src/database/repos/favorite/favorite.repo.ts

@@ -7,7 +7,7 @@ import { executeWithCursorPagination } from '@docmost/db/pagination/cursor-pagin
 import { jsonObjectFrom } from 'kysely/helpers/postgres';
 import { ExpressionBuilder, SelectQueryBuilder, sql } from 'kysely';
 import { DB } from '@docmost/db/types/db';
-import { dbOrTx } from '@docmost/db/utils';
+import { dbOrTx, isUniqueViolation } from '@docmost/db/utils';
 
 export const FavoriteType = {
   PAGE: 'page',
@@ -29,7 +29,8 @@ export class FavoriteRepo {
         .returningAll()
         .executeTakeFirst();
     } catch (err: any) {
-      if (err?.code === '23505') return undefined;
+      // Idempotent favorite: a duplicate (already-favorited) is not an error.
+      if (isUniqueViolation(err)) return undefined;
       throw err;
     }
   }

apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts

@@ -94,7 +94,9 @@ describe('ShareAliasRepo', () => {
         return builder;
       }),
       returning: jest.fn(() => builder),
-      executeTakeFirst: jest.fn().mockResolvedValue({ id: 'a-1' }),
+      // Retarget uses executeTakeFirstOrThrow so a row reaped by a concurrent
+      // delete (0 rows matched) raises NoResultError instead of returning undefined.
+      executeTakeFirstOrThrow: jest.fn().mockResolvedValue({ id: 'a-1' }),
     };
     const db = { updateTable: jest.fn(() => builder) } as unknown as KyselyDB;
     const repo = new ShareAliasRepo(db);
@@ -121,7 +123,11 @@ describe('ShareAliasRepo', () => {
         return builder;
       }),
       returning: jest.fn(() => builder),
-      executeTakeFirst: jest.fn().mockResolvedValue({ id: 'a-1', alias: 'ted' }),
+      // Rename uses executeTakeFirstOrThrow so a row reaped by a concurrent
+      // delete (0 rows matched) raises NoResultError instead of returning undefined.
+      executeTakeFirstOrThrow: jest
+        .fn()
+        .mockResolvedValue({ id: 'a-1', alias: 'ted' }),
     };
     const db = { updateTable: jest.fn(() => builder) } as unknown as KyselyDB;
     const repo = new ShareAliasRepo(db);

apps/server/src/database/repos/share-alias/share-alias.repo.ts

@@ -92,6 +92,12 @@ export class ShareAliasRepo {
    * Rename an existing alias row in place (the vanity-slug edit, e.g.
    * `te` -> `ted`). Keeps the row's id/page_id/creator so the page's single
    * alias pointer is preserved — only the human-readable name changes.
+   *
+   * Uses `executeTakeFirstOrThrow`: if a concurrent `delete` reaps this row
+   * between the service's read and this UPDATE (READ COMMITTED), the UPDATE
+   * matches 0 rows and kysely throws `NoResultError` rather than returning
+   * `undefined` for a `Promise<ShareAlias>`. The service maps that to a
+   * retryable conflict instead of dereferencing `undefined.id`.
    */
   async updateAlias(
     id: string,
@@ -105,7 +111,7 @@ export class ShareAliasRepo {
       .where('id', '=', id)
       .where('workspaceId', '=', workspaceId)
       .returning(this.baseFields)
-      .executeTakeFirst();
+      .executeTakeFirstOrThrow();
   }
 
   /**
@@ -127,7 +133,15 @@ export class ShareAliasRepo {
       .execute();
   }
 
-  /** Retarget an existing alias to a new page (the "swap" operation). */
+  /**
+   * Retarget an existing alias to a new page (the "swap" operation).
+   *
+   * Uses `executeTakeFirstOrThrow`: if a concurrent `delete` reaps this row
+   * between the service's read and this UPDATE, the UPDATE matches 0 rows and
+   * kysely throws `NoResultError` instead of returning `undefined` into the 200
+   * response (a "success" with no alias). The service maps that to a retryable
+   * conflict.
+   */
   async updatePageId(
     id: string,
     pageId: string,
@@ -140,7 +154,7 @@ export class ShareAliasRepo {
       .where('id', '=', id)
       .where('workspaceId', '=', workspaceId)
       .returning(this.baseFields)
-      .executeTakeFirst();
+      .executeTakeFirstOrThrow();
   }
 
   async delete(

apps/server/src/database/unique-violation.spec.ts

@@ -0,0 +1,51 @@
+import { isUniqueViolation, violatedConstraint } from './utils';
+
+/**
+ * Unit tests for the driver-bound Postgres unique-violation helpers extracted
+ * from the share-alias service (and now shared with favorite.repo). They encode
+ * two `kysely-postgres-js` / `postgres@3.x` quirks: the SQLSTATE is the string
+ * `'23505'`, and the violated index name arrives as `constraint_name` (with
+ * `constraint` only a fallback for other drivers).
+ */
+describe('isUniqueViolation', () => {
+  it('is true for a 23505 error', () => {
+    expect(isUniqueViolation({ code: '23505' })).toBe(true);
+  });
+
+  it('is false for any other code', () => {
+    expect(isUniqueViolation({ code: '08006' })).toBe(false);
+  });
+
+  it('is false when there is no code / not an object', () => {
+    expect(isUniqueViolation({})).toBe(false);
+    expect(isUniqueViolation(null)).toBe(false);
+    expect(isUniqueViolation(undefined)).toBe(false);
+    expect(isUniqueViolation(new Error('boom'))).toBe(false);
+  });
+});
+
+describe('violatedConstraint', () => {
+  it('reads the postgres@3.x `constraint_name` field', () => {
+    expect(
+      violatedConstraint({ code: '23505', constraint_name: 'idx_a' }),
+    ).toBe('idx_a');
+  });
+
+  it('falls back to `constraint` when `constraint_name` is absent', () => {
+    expect(violatedConstraint({ code: '23505', constraint: 'idx_b' })).toBe(
+      'idx_b',
+    );
+  });
+
+  it('prefers `constraint_name` over `constraint` when both are present', () => {
+    expect(
+      violatedConstraint({ constraint_name: 'idx_a', constraint: 'idx_b' }),
+    ).toBe('idx_a');
+  });
+
+  it('is undefined when neither field is present', () => {
+    expect(violatedConstraint({ code: '23505' })).toBeUndefined();
+    expect(violatedConstraint(null)).toBeUndefined();
+    expect(violatedConstraint(undefined)).toBeUndefined();
+  });
+});

apps/server/src/database/utils.ts

@@ -33,6 +33,35 @@ export function dbOrTx(
   }
 }
 
+/** Postgres `unique_violation` SQLSTATE — raised when a write hits a UNIQUE index. */
+const PG_UNIQUE_VIOLATION = '23505';
+
+/**
+ * Whether `err` is a Postgres unique-violation (SQLSTATE `23505`). THE single
+ * check so repos/services stop re-hardcoding the magic code.
+ *
+ * NOTE (#222): `core/ai-chat/roles/ai-agent-roles.service.ts` still carries its
+ * own inline `23505` check on a separate, unmerged branch; it should adopt this
+ * helper (and {@link violatedConstraint}) after #227 lands.
+ */
+export function isUniqueViolation(err: unknown): boolean {
+  return (err as { code?: unknown } | null | undefined)?.code === PG_UNIQUE_VIOLATION;
+}
+
+/**
+ * The name of the UNIQUE index/constraint a `23505` error violated, or
+ * undefined. The `kysely-postgres-js` / `postgres@3.x` driver surfaces it as
+ * `err.constraint_name` (NOT `.constraint`); `.constraint` is kept only as a
+ * defensive fallback for other drivers.
+ */
+export function violatedConstraint(err: unknown): string | undefined {
+  const e = err as
+    | { constraint_name?: string; constraint?: string }
+    | null
+    | undefined;
+  return e?.constraint_name ?? e?.constraint;
+}
+
 /**
  * Bind a JS array/object as a `jsonb` column value, working around a postgres
  * driver double-encoding quirk. THE single implementation — repos that persist