diff --git a/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index 3f4bde97..7c2e3c6f 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -1573,5 +1573,6 @@ "Move right": "Move right", "Delete tab": "Delete tab", "Tab label": "Tab label", - "Tab actions for {{name}}": "Tab actions for {{name}}" + "Tab actions for {{name}}": "Tab actions for {{name}}", + "Inline base": "Inline base" } diff --git a/apps/client/src/ee/base/components/cells/cell-email.tsx b/apps/client/src/ee/base/components/cells/cell-email.tsx index 0d93f69f..78b78e19 100644 --- a/apps/client/src/ee/base/components/cells/cell-email.tsx +++ b/apps/client/src/ee/base/components/cells/cell-email.tsx @@ -15,13 +15,12 @@ type CellEmailProps = { const toDraft = (value: unknown) => (typeof value === "string" ? value : ""); const parse = (draft: string) => draft || null; -export function CellEmail({ value, property, rowId, isEditing, onCommit, onCancel }: CellEmailProps) { +export function CellEmail({ value, property, rowId, isEditing, onCommit }: CellEmailProps) { const { draft, setDraft, inputRef, handleKeyDown, handleBlur } = useEditableTextCell({ value, isEditing, onCommit, - onCancel, toDraft, parse, rowId, diff --git a/apps/client/src/ee/base/components/cells/cell-long-text.test.tsx b/apps/client/src/ee/base/components/cells/cell-long-text.test.tsx new file mode 100644 index 00000000..2527e4a8 --- /dev/null +++ b/apps/client/src/ee/base/components/cells/cell-long-text.test.tsx @@ -0,0 +1,107 @@ +import { MantineProvider } from "@mantine/core"; +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import type { ReactNode } from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { IBaseProperty } from "@/ee/base/types/base.types"; +import { CellLongText } from "./cell-long-text"; + +vi.mock("@/main.tsx", async () => { + const { QueryClient } = await import("@tanstack/react-query"); + return { queryClient: new QueryClient() }; +}); + +const property = { + id: "p1", + pageId: "base-1", + name: "Long text", + type: "text", + position: "a", + typeOptions: {}, + isPrimary: false, +} as unknown as IBaseProperty; + +function setMediaMatch() { + Object.defineProperty(window, "matchMedia", { + configurable: true, + value: vi.fn().mockImplementation((query: string) => ({ + matches: false, + media: query, + onchange: null, + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + addListener: vi.fn(), + removeListener: vi.fn(), + dispatchEvent: vi.fn(), + })), + }); +} + +beforeEach(() => { + setMediaMatch(); + // Textarea autosize listens for font loads, which jsdom doesn't implement. + Object.defineProperty(document, "fonts", { + configurable: true, + value: { addEventListener: vi.fn(), removeEventListener: vi.fn() }, + }); +}); + +afterEach(cleanup); + +describe("CellLongText", () => { + it("saves what was typed when Esc closes the editor", () => { + const onCommit = vi.fn(); + const onCancel = vi.fn(); + render( + , + { + wrapper: ({ children }: { children: ReactNode }) => ( + {children} + ), + }, + ); + const textarea = screen.getByRole("textbox"); + + fireEvent.change(textarea, { target: { value: "what is love" } }); + fireEvent.keyDown(textarea, { key: "Escape" }); + + expect(onCommit).toHaveBeenCalledWith("what is love"); + expect(onCancel).not.toHaveBeenCalled(); + }); + + it("autosaves after a 1 s typing pause", () => { + vi.useFakeTimers(); + const onValueChange = vi.fn(); + render( + , + { + wrapper: ({ children }: { children: ReactNode }) => ( + {children} + ), + }, + ); + + fireEvent.change(screen.getByRole("textbox"), { + target: { value: "what is love" }, + }); + act(() => vi.advanceTimersByTime(1000)); + + expect(onValueChange).toHaveBeenCalledWith("what is love"); + vi.useRealTimers(); + }); +}); diff --git a/apps/client/src/ee/base/components/cells/cell-long-text.tsx b/apps/client/src/ee/base/components/cells/cell-long-text.tsx index 2c390056..32616db4 100644 --- a/apps/client/src/ee/base/components/cells/cell-long-text.tsx +++ b/apps/client/src/ee/base/components/cells/cell-long-text.tsx @@ -4,6 +4,7 @@ import { useDebouncedCallback } from "@mantine/hooks"; import { IBaseProperty } from "@/ee/base/types/base.types"; import { formatLongTextPreview } from "@/ee/base/formatters/cell-formatters"; import cellClasses from "@/ee/base/styles/cells.module.css"; +import { AUTOSAVE_DELAY_MS } from "@/ee/base/constants/autosave"; type CellLongTextProps = { value: unknown; @@ -27,11 +28,9 @@ export function CellLongText({ isEditing, onCommit, onValueChange, - onCancel, onTabNavigate, }: CellLongTextProps) { const [draft, setDraft] = useState(() => toText(value)); - const cancelledRef = useRef(false); const committedRef = useRef(false); const wasEditingRef = useRef(false); const textareaRef = useRef(null); @@ -40,7 +39,6 @@ export function CellLongText({ // value changes mid-edit so the user's typing is not clobbered. useEffect(() => { if (isEditing && !wasEditingRef.current) { - cancelledRef.current = false; committedRef.current = false; setDraft(toText(value)); requestAnimationFrame(() => { @@ -53,11 +51,11 @@ export function CellLongText({ wasEditingRef.current = isEditing; }, [isEditing, value]); - // Autosave after a typing pause; commit/cancel clear the pending fire so - // a closed editor can never write a stale or discarded draft. + // Autosave after a typing pause; commit clears the pending fire so a + // closed editor can never write a stale draft. const debouncedAutosave = useDebouncedCallback(() => { onValueChange(normalize(draft)); - }, 10_000); + }, AUTOSAVE_DELAY_MS); const commit = () => { if (committedRef.current) return; @@ -65,11 +63,6 @@ export function CellLongText({ debouncedAutosave.cancel(); onCommit(normalize(draft)); }; - const cancel = () => { - cancelledRef.current = true; - debouncedAutosave.cancel(); - onCancel(); - }; const preview = formatLongTextPreview(toText(value)); @@ -78,11 +71,6 @@ export function CellLongText({ opened={isEditing} onChange={(opened) => { if (opened) return; - // Programmatic close after cancel must not re-commit. - if (cancelledRef.current) { - cancelledRef.current = false; - return; - } commit(); }} position="bottom-start" @@ -135,7 +123,7 @@ export function CellLongText({ onTabNavigate?.(e.shiftKey); } else if (e.key === "Escape") { e.preventDefault(); - cancel(); + commit(); } else if ((e.metaKey || e.ctrlKey) && e.key === "Enter") { e.preventDefault(); commit(); diff --git a/apps/client/src/ee/base/components/cells/cell-number.tsx b/apps/client/src/ee/base/components/cells/cell-number.tsx index cbb50a99..d964de06 100644 --- a/apps/client/src/ee/base/components/cells/cell-number.tsx +++ b/apps/client/src/ee/base/components/cells/cell-number.tsx @@ -95,7 +95,6 @@ export function CellNumber({ rowId, isEditing, onCommit, - onCancel, }: CellNumberProps) { const typeOptions = property.typeOptions as NumberTypeOptions | undefined; const { draft, setDraft, inputRef, handleKeyDown, handleBlur } = @@ -103,7 +102,6 @@ export function CellNumber({ value, isEditing, onCommit, - onCancel, toDraft, parse: parseNumberDraft, rowId, diff --git a/apps/client/src/ee/base/components/cells/cell-text.test.tsx b/apps/client/src/ee/base/components/cells/cell-text.test.tsx new file mode 100644 index 00000000..68dd0138 --- /dev/null +++ b/apps/client/src/ee/base/components/cells/cell-text.test.tsx @@ -0,0 +1,45 @@ +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { IBaseProperty } from "@/ee/base/types/base.types"; +import { CellText } from "./cell-text"; + +vi.mock("@/main.tsx", async () => { + const { QueryClient } = await import("@tanstack/react-query"); + return { queryClient: new QueryClient() }; +}); + +const property = { + id: "p1", + pageId: "base-1", + name: "Text", + type: "text", + position: "a", + typeOptions: {}, + isPrimary: false, +} as unknown as IBaseProperty; + +afterEach(cleanup); + +describe("CellText", () => { + it("saves what was typed when Esc ends the edit", () => { + const onCommit = vi.fn(); + const onCancel = vi.fn(); + render( + , + ); + const input = screen.getByRole("textbox"); + + fireEvent.change(input, { target: { value: "hello" } }); + fireEvent.keyDown(input, { key: "Escape" }); + + expect(onCommit).toHaveBeenCalledWith("hello"); + expect(onCancel).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/client/src/ee/base/components/cells/cell-text.tsx b/apps/client/src/ee/base/components/cells/cell-text.tsx index 172e0350..12cd8e01 100644 --- a/apps/client/src/ee/base/components/cells/cell-text.tsx +++ b/apps/client/src/ee/base/components/cells/cell-text.tsx @@ -16,13 +16,12 @@ type CellTextProps = { const toDraft = (value: unknown) => (typeof value === "string" ? value : ""); const parse = (draft: string) => draft; -export function CellText({ value, property, rowId, isEditing, onCommit, onCancel }: CellTextProps) { +export function CellText({ value, property, rowId, isEditing, onCommit }: CellTextProps) { const { draft, setDraft, inputRef, handleKeyDown, handleBlur } = useEditableTextCell({ value, isEditing, onCommit, - onCancel, toDraft, parse, rowId, diff --git a/apps/client/src/ee/base/components/cells/cell-url.tsx b/apps/client/src/ee/base/components/cells/cell-url.tsx index c7c4de35..cef3f262 100644 --- a/apps/client/src/ee/base/components/cells/cell-url.tsx +++ b/apps/client/src/ee/base/components/cells/cell-url.tsx @@ -16,13 +16,12 @@ type CellUrlProps = { const toDraft = (value: unknown) => (typeof value === "string" ? value : ""); const parse = (draft: string) => draft || null; -export function CellUrl({ value, property, rowId, isEditing, onCommit, onCancel }: CellUrlProps) { +export function CellUrl({ value, property, rowId, isEditing, onCommit }: CellUrlProps) { const { draft, setDraft, inputRef, handleKeyDown, handleBlur } = useEditableTextCell({ value, isEditing, onCommit, - onCancel, toDraft, parse, rowId, diff --git a/apps/client/src/ee/base/components/row-detail-modal/fields/field-long-text.tsx b/apps/client/src/ee/base/components/row-detail-modal/fields/field-long-text.tsx index 7fd11e8a..a7b96d30 100644 --- a/apps/client/src/ee/base/components/row-detail-modal/fields/field-long-text.tsx +++ b/apps/client/src/ee/base/components/row-detail-modal/fields/field-long-text.tsx @@ -1,7 +1,9 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useState } from "react"; import { Textarea } from "@mantine/core"; +import { useDebouncedCallback } from "@mantine/hooks"; import { FieldProps, FieldShell } from "./detail-field"; import classes from "@/ee/base/styles/row-detail-modal.module.css"; +import { AUTOSAVE_DELAY_MS } from "@/ee/base/constants/autosave"; const toText = (value: unknown) => (typeof value === "string" ? value : ""); const normalize = (s: string) => { @@ -19,23 +21,21 @@ export function FieldLongText({ const text = toText(value); const [draft, setDraft] = useState(text); const [focused, setFocused] = useState(false); - // Esc sets this; blur() then runs commit synchronously with the stale - // draft, so the revert must be decided here, not via setDraft. - const cancelRef = useRef(false); useEffect(() => { if (!focused) setDraft(text); }, [text, focused]); + const save = () => { + if (normalize(draft) !== normalize(text)) onChange(normalize(draft)); + }; + const autosave = useDebouncedCallback(save, AUTOSAVE_DELAY_MS); + const commit = () => { setFocused(false); onEditingChange?.(false); - if (cancelRef.current) { - cancelRef.current = false; - setDraft(text); - return; - } - if (normalize(draft) !== normalize(text)) onChange(normalize(draft)); + autosave.cancel(); + save(); }; if (readOnly) { @@ -61,11 +61,13 @@ export function FieldLongText({ setFocused(true); onEditingChange?.(true); }} - onChange={(e) => setDraft(e.currentTarget.value)} + onChange={(e) => { + setDraft(e.currentTarget.value); + autosave(); + }} onBlur={commit} onKeyDown={(e) => { if (e.key === "Escape") { - cancelRef.current = true; e.currentTarget.blur(); } else if ((e.metaKey || e.ctrlKey) && e.key === "Enter") { e.preventDefault(); diff --git a/apps/client/src/ee/base/components/row-detail-modal/fields/field-number.tsx b/apps/client/src/ee/base/components/row-detail-modal/fields/field-number.tsx index 62ab2255..8ceaf3f6 100644 --- a/apps/client/src/ee/base/components/row-detail-modal/fields/field-number.tsx +++ b/apps/client/src/ee/base/components/row-detail-modal/fields/field-number.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useState } from "react"; import { NumberTypeOptions } from "@/ee/base/types/base.types"; import { formatNumber, @@ -22,9 +22,6 @@ export function FieldNumber({ const numValue = typeof value === "number" ? value : null; const [draft, setDraft] = useState(toDraft(value)); const [focused, setFocused] = useState(false); - // Esc sets this; blur() then runs commit synchronously with the stale - // draft, so the revert must be decided here, not via setDraft. - const cancelRef = useRef(false); useEffect(() => { if (!focused) setDraft(toDraft(value)); @@ -43,11 +40,6 @@ export function FieldNumber({ const commit = () => { setFocused(false); onEditingChange?.(false); - if (cancelRef.current) { - cancelRef.current = false; - setDraft(toDraft(value)); - return; - } if (parseNumberDraft(draft) !== numValue) onChange(parseNumberDraft(draft)); }; @@ -86,7 +78,6 @@ export function FieldNumber({ e.preventDefault(); e.currentTarget.blur(); } else if (e.key === "Escape") { - cancelRef.current = true; e.currentTarget.blur(); } }} diff --git a/apps/client/src/ee/base/components/row-detail-modal/fields/field-text.tsx b/apps/client/src/ee/base/components/row-detail-modal/fields/field-text.tsx index dbf1abbd..d2002a92 100644 --- a/apps/client/src/ee/base/components/row-detail-modal/fields/field-text.tsx +++ b/apps/client/src/ee/base/components/row-detail-modal/fields/field-text.tsx @@ -1,7 +1,9 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useState } from "react"; +import { useDebouncedCallback } from "@mantine/hooks"; import { IconExternalLink, IconMail } from "@tabler/icons-react"; import { FieldProps, FieldShell } from "./detail-field"; import classes from "@/ee/base/styles/row-detail-modal.module.css"; +import { AUTOSAVE_DELAY_MS } from "@/ee/base/constants/autosave"; const toText = (value: unknown) => (typeof value === "string" ? value : ""); @@ -15,24 +17,22 @@ export function FieldText({ const text = toText(value); const [draft, setDraft] = useState(text); const [focused, setFocused] = useState(false); - // Esc sets this; blur() then runs commit synchronously with the stale - // draft, so the revert must be decided here, not via setDraft. - const cancelRef = useRef(false); // Track remote/navigation updates while not typing. useEffect(() => { if (!focused) setDraft(text); }, [text, focused]); + const save = () => { + if (draft !== text) onChange(draft); + }; + const autosave = useDebouncedCallback(save, AUTOSAVE_DELAY_MS); + const commit = () => { setFocused(false); onEditingChange?.(false); - if (cancelRef.current) { - cancelRef.current = false; - setDraft(text); - return; - } - if (draft !== text) onChange(draft); + autosave.cancel(); + save(); }; if (readOnly) { @@ -65,14 +65,16 @@ export function FieldText({ setFocused(true); onEditingChange?.(true); }} - onChange={(e) => setDraft(e.currentTarget.value)} + onChange={(e) => { + setDraft(e.currentTarget.value); + autosave(); + }} onBlur={commit} onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); e.currentTarget.blur(); } else if (e.key === "Escape") { - cancelRef.current = true; e.currentTarget.blur(); } }} diff --git a/apps/client/src/ee/base/components/row-detail-modal/row-detail-fields.test.tsx b/apps/client/src/ee/base/components/row-detail-modal/row-detail-fields.test.tsx new file mode 100644 index 00000000..cce45da4 --- /dev/null +++ b/apps/client/src/ee/base/components/row-detail-modal/row-detail-fields.test.tsx @@ -0,0 +1,185 @@ +import { MantineProvider } from "@mantine/core"; +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import type { ComponentType } from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { IBaseProperty, IBaseRow } from "@/ee/base/types/base.types"; +import { RowDetailTitle } from "./row-detail-title"; +import { FieldProps } from "./fields/detail-field"; +import { FieldText } from "./fields/field-text"; +import { FieldNumber } from "./fields/field-number"; +import { FieldLongText } from "./fields/field-long-text"; + +vi.mock("@/main.tsx", async () => { + const { QueryClient } = await import("@tanstack/react-query"); + return { queryClient: new QueryClient() }; +}); + +vi.mock("react-i18next", async (importOriginal) => ({ + ...(await importOriginal()), + useTranslation: () => ({ t: (value: string) => value }), +})); + +const property = (type: IBaseProperty["type"]) => + ({ + id: "p1", + pageId: "base-1", + name: "Field", + type, + position: "a", + typeOptions: {}, + isPrimary: false, + }) as IBaseProperty; + +function setMediaMatch() { + Object.defineProperty(window, "matchMedia", { + configurable: true, + value: vi.fn().mockImplementation((query: string) => ({ + matches: false, + media: query, + onchange: null, + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + addListener: vi.fn(), + removeListener: vi.fn(), + dispatchEvent: vi.fn(), + })), + }); +} + +// Real focus, so the blur that Esc triggers reaches onBlur as in the browser. +function typeThenEscape(value: string) { + const input = screen.getByRole("textbox"); + act(() => input.focus()); + fireEvent.change(input, { target: { value } }); + fireEvent.keyDown(input, { key: "Escape" }); + return input; +} + +beforeEach(() => { + setMediaMatch(); + // Textarea autosize listens for font loads, which jsdom doesn't implement. + Object.defineProperty(document, "fonts", { + configurable: true, + value: { addEventListener: vi.fn(), removeEventListener: vi.fn() }, + }); +}); +afterEach(cleanup); + +describe("Esc in the row detail modal", () => { + it("saves a typed title before leaving the field", () => { + const onCommit = vi.fn(); + render( + , + ); + + const input = typeThenEscape("dddddd"); + + expect(onCommit).toHaveBeenCalledWith("dddddd"); + expect(document.activeElement).not.toBe(input); + }); + + it.each([ + ["text", FieldText, "text", "hello", "hello"], + ["number", FieldNumber, "number", "42", 42], + ["long text", FieldLongText, "text", "notes", "notes"], + ] as [string, ComponentType, IBaseProperty["type"], string, unknown][])( + "saves a typed %s field before leaving it", + (_, Field, type, typed, saved) => { + const onChange = vi.fn(); + render( + , + { wrapper: MantineProvider }, + ); + + const input = typeThenEscape(typed); + + expect(onChange).toHaveBeenCalledWith(saved); + expect(document.activeElement).not.toBe(input); + }, + ); +}); + +describe("Typing in the row detail modal", () => { + const titleRow = (title: string) => + ({ id: "r1", cells: { p1: title } }) as unknown as IBaseRow; + const primary = { ...property("text"), isPrimary: true }; + + function type(value: string) { + const input = screen.getByRole("textbox"); + act(() => input.focus()); + fireEvent.change(input, { target: { value } }); + return input; + } + + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it("saves a typed title after a 1 s pause, without leaving the field", () => { + const onCommit = vi.fn(); + render( + , + ); + + const input = type("dddddd"); + act(() => vi.advanceTimersByTime(999)); + expect(onCommit).not.toHaveBeenCalled(); + act(() => vi.advanceTimersByTime(1)); + + expect(onCommit).toHaveBeenCalledWith("dddddd"); + expect(document.activeElement).toBe(input); + }); + + it("keeps a typed title when the row's title changes elsewhere", () => { + const props = { primaryProperty: primary, canEdit: true, onCommit: vi.fn() }; + const { rerender } = render( + , + ); + + const input = type("My draft"); + rerender(); + + expect(input).toHaveProperty("value", "My draft"); + }); + + it.each([ + ["text", FieldText, "hello"], + ["long text", FieldLongText, "notes"], + ] as [string, ComponentType, string][])( + "saves a typed %s field after a 1 s pause", + (_, Field, typed) => { + const onChange = vi.fn(); + render( + , + { wrapper: MantineProvider }, + ); + + const input = type(typed); + act(() => vi.advanceTimersByTime(1000)); + + expect(onChange).toHaveBeenCalledWith(typed); + expect(document.activeElement).toBe(input); + }, + ); +}); diff --git a/apps/client/src/ee/base/components/row-detail-modal/row-detail-modal.test.tsx b/apps/client/src/ee/base/components/row-detail-modal/row-detail-modal.test.tsx new file mode 100644 index 00000000..34bd9371 --- /dev/null +++ b/apps/client/src/ee/base/components/row-detail-modal/row-detail-modal.test.tsx @@ -0,0 +1,116 @@ +import { MantineProvider } from "@mantine/core"; +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import type { ReactNode } from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { IBase, IBaseRow } from "@/ee/base/types/base.types"; +import { RowDetailModal } from "./row-detail-modal"; + +const mocks = vi.hoisted(() => ({ updateRow: vi.fn() })); + +vi.mock("@/main.tsx", async () => { + const { QueryClient } = await import("@tanstack/react-query"); + return { queryClient: new QueryClient() }; +}); + +vi.mock("react-i18next", async (importOriginal) => ({ + ...(await importOriginal()), + useTranslation: () => ({ t: (value: string) => value }), +})); + +vi.mock("@/ee/base/queries/base-row-query", () => ({ + useBaseRowQuery: () => ({ data: undefined, isError: false }), + useUpdateRowMutation: () => ({ mutate: mocks.updateRow, isPending: false }), + useDeleteRowMutation: () => ({ mutate: vi.fn() }), +})); + +vi.mock("@/ee/base/context/base-editable", () => ({ + useBaseEditable: () => true, +})); + +vi.mock("@/ee/base/components/property/create-property-popover", () => ({ + CreatePropertyPopover: () => null, +})); + +const base = { + id: "base-1", + properties: [ + { + id: "p1", + pageId: "base-1", + name: "Name", + type: "text", + position: "a", + typeOptions: {}, + isPrimary: true, + }, + ], +} as unknown as IBase; + +const row = { + id: "r1", + pageId: "base-1", + cells: {}, + position: "a", +} as unknown as IBaseRow; + +function setMediaMatch() { + Object.defineProperty(window, "matchMedia", { + configurable: true, + value: vi.fn().mockImplementation((query: string) => ({ + matches: false, + media: query, + onchange: null, + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + addListener: vi.fn(), + removeListener: vi.fn(), + dispatchEvent: vi.fn(), + })), + }); +} + +function renderModal(onClose: () => void) { + render( + , + { + wrapper: ({ children }: { children: ReactNode }) => ( + {children} + ), + }, + ); +} + +beforeEach(() => { + setMediaMatch(); + mocks.updateRow.mockReset(); +}); + +afterEach(cleanup); + +describe("RowDetailModal Esc", () => { + it("saves a typed title, then closes, in one press", () => { + const onClose = vi.fn(); + renderModal(onClose); + const title = screen.getByRole("textbox", { name: "Name" }); + + act(() => title.focus()); + fireEvent.change(title, { target: { value: "dddddd" } }); + fireEvent.keyDown(title, { key: "Escape" }); + + expect(mocks.updateRow).toHaveBeenCalledWith({ + rowId: "r1", + pageId: "base-1", + cells: { p1: "dddddd" }, + }); + expect(onClose).toHaveBeenCalledTimes(1); + expect(mocks.updateRow.mock.invocationCallOrder[0]).toBeLessThan( + onClose.mock.invocationCallOrder[0], + ); + }); +}); diff --git a/apps/client/src/ee/base/components/row-detail-modal/row-detail-modal.tsx b/apps/client/src/ee/base/components/row-detail-modal/row-detail-modal.tsx index be303a0f..9efe8e32 100644 --- a/apps/client/src/ee/base/components/row-detail-modal/row-detail-modal.tsx +++ b/apps/client/src/ee/base/components/row-detail-modal/row-detail-modal.tsx @@ -168,10 +168,11 @@ export function RowDetailModal({ // Mantine's closeOnEscape runs a capture-phase window listener that fires // before inner popovers and inputs see the key, so we manage Esc ourselves - // and yield to: nested dialogs (delete confirm), open popovers - // ([data-position]) and editable elements. Arrows step records under the - // same yield rules. Mantine puts role="dialog" and our content class on - // the same element, which distinguishes this modal from nested ones. + // and yield to nested dialogs (delete confirm) and open popovers + // ([data-position]). Esc in an inline field blurs it first, which saves the + // edit, then closes; arrows stay with the field. Mantine puts role="dialog" + // and our content class on the same element, which distinguishes this + // modal from nested ones. const handleKeyDown = useCallback( (event: KeyboardEvent) => { const isEscape = event.key === "Escape"; @@ -183,11 +184,10 @@ export function RowDetailModal({ if (dialog && !dialog.classList.contains(classes.modalContent)) { return; } - if ( - target.closest("[data-position]") || - target.matches("input, textarea, select, [contenteditable='true']") - ) { - return; + if (target.closest("[data-position]")) return; + if (target.matches("input, textarea, select, [contenteditable='true']")) { + if (!isEscape) return; + target.blur(); } } if (isEscape) { @@ -383,7 +383,7 @@ export function RowDetailModal({ Esc - {t("to reset")} + {t("to close")} ) : ( diff --git a/apps/client/src/ee/base/components/row-detail-modal/row-detail-title.tsx b/apps/client/src/ee/base/components/row-detail-modal/row-detail-title.tsx index 2e023815..f254f756 100644 --- a/apps/client/src/ee/base/components/row-detail-modal/row-detail-title.tsx +++ b/apps/client/src/ee/base/components/row-detail-modal/row-detail-title.tsx @@ -1,7 +1,9 @@ import { useEffect, useRef, useState } from "react"; +import { useDebouncedCallback } from "@mantine/hooks"; import { useTranslation } from "react-i18next"; import { IBaseProperty, IBaseRow } from "@/ee/base/types/base.types"; import { timeAgo } from "@/lib/time.ts"; +import { AUTOSAVE_DELAY_MS } from "@/ee/base/constants/autosave"; import classes from "@/ee/base/styles/row-detail-modal.module.css"; type RowDetailTitleProps = { @@ -24,21 +26,23 @@ export function RowDetailTitle({ ? (((row.cells ?? {})[primaryProperty.id] as string) ?? "") : ""; const [value, setValue] = useState(initial); - const cancelRef = useRef(false); + const focusedRef = useRef(false); - const commit = () => { - onEditingChange?.(false); - if (cancelRef.current) { - cancelRef.current = false; - setValue(initial); - return; - } + const save = () => { if (value !== initial) onCommit(value); }; + const autosave = useDebouncedCallback(save, AUTOSAVE_DELAY_MS); - // Re-sync when the row changes underneath us (navigation or remote edit). + const commit = () => { + focusedRef.current = false; + onEditingChange?.(false); + autosave.cancel(); + save(); + }; + + // Re-sync when the row changes underneath us (navigation or remote edit), but not mid-typing. useEffect(() => { - setValue(initial); + if (!focusedRef.current) setValue(initial); }, [initial]); const updatedAgo = row.updatedAt ? timeAgo(new Date(row.updatedAt)) : ""; @@ -55,13 +59,16 @@ export function RowDetailTitle({ value={value} maxLength={1000} onFocus={() => { + focusedRef.current = true; onEditingChange?.(true); }} - onChange={(e) => setValue(e.currentTarget.value)} + onChange={(e) => { + setValue(e.currentTarget.value); + autosave(); + }} onBlur={commit} onKeyDown={(e) => { if (e.key === "Escape") { - cancelRef.current = true; e.currentTarget.blur(); } else if (e.key === "Enter") { e.preventDefault(); diff --git a/apps/client/src/ee/base/constants/autosave.ts b/apps/client/src/ee/base/constants/autosave.ts new file mode 100644 index 00000000..b06524b3 --- /dev/null +++ b/apps/client/src/ee/base/constants/autosave.ts @@ -0,0 +1,2 @@ +// Typing pause after which a text edit saves itself. +export const AUTOSAVE_DELAY_MS = 1000; diff --git a/apps/client/src/ee/base/hooks/use-editable-text-cell.ts b/apps/client/src/ee/base/hooks/use-editable-text-cell.ts index 8fa5d934..21489797 100644 --- a/apps/client/src/ee/base/hooks/use-editable-text-cell.ts +++ b/apps/client/src/ee/base/hooks/use-editable-text-cell.ts @@ -6,7 +6,6 @@ export type UseEditableTextCellParams = { value: unknown; isEditing: boolean; onCommit: (value: unknown) => void; - onCancel: () => void; /** value -> the draft string shown in the input when editing begins */ toDraft: (value: unknown) => string; /** draft string -> the value passed to onCommit */ @@ -27,7 +26,6 @@ export function useEditableTextCell({ value, isEditing, onCommit, - onCancel, toDraft, parse, rowId, @@ -85,11 +83,10 @@ export function useEditableTextCell({ commitOnce(parse(draft)); } else if (e.key === "Escape") { e.preventDefault(); - committedRef.current = true; - onCancel(); + commitOnce(parse(draft)); } }, - [draft, parse, commitOnce, onCancel], + [draft, parse, commitOnce], ); const handleBlur = useCallback(() => { diff --git a/apps/client/src/ee/base/hooks/use-grid-keyboard-nav.test.ts b/apps/client/src/ee/base/hooks/use-grid-keyboard-nav.test.ts new file mode 100644 index 00000000..bac36567 --- /dev/null +++ b/apps/client/src/ee/base/hooks/use-grid-keyboard-nav.test.ts @@ -0,0 +1,56 @@ +import { renderHook } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { useGridKeyboardNav } from "./use-grid-keyboard-nav"; + +type Options = Parameters[0]; + +afterEach(() => { + document.body.innerHTML = ""; +}); + +describe("useGridKeyboardNav while editing", () => { + it("blurs the cell input, which saves it, before leaving edit mode on Esc", () => { + const container = document.createElement("div"); + const input = document.createElement("input"); + container.appendChild(input); + document.body.appendChild(container); + + const order: string[] = []; + input.addEventListener("blur", () => order.push("blur")); + const setEditingCell = vi.fn(() => { + order.push("stop editing"); + }); + + renderHook(() => + useGridKeyboardNav({ + table: { + getVisibleLeafColumns: () => [], + getRowModel: () => ({ rows: [] }), + }, + properties: [], + containerRef: { current: container }, + focusedCell: null, + setFocusedCell: vi.fn(), + editingCell: { rowId: "r1", propertyId: "p1" }, + setEditingCell, + openEditor: vi.fn(), + clearCell: vi.fn(), + beginTypeToEdit: vi.fn(), + scrollCellIntoView: vi.fn(), + selectionCount: 0, + clearSelection: vi.fn(), + deleteSelected: vi.fn(), + toggleRowSelection: vi.fn(), + expandRow: vi.fn(), + addRow: vi.fn(), + } as unknown as Options), + ); + + input.focus(); + input.dispatchEvent( + new KeyboardEvent("keydown", { key: "Escape", bubbles: true }), + ); + + expect(order).toEqual(["blur", "stop editing"]); + }); +}); diff --git a/apps/client/src/ee/base/hooks/use-grid-keyboard-nav.ts b/apps/client/src/ee/base/hooks/use-grid-keyboard-nav.ts index 09d140e4..168ebf9e 100644 --- a/apps/client/src/ee/base/hooks/use-grid-keyboard-nav.ts +++ b/apps/client/src/ee/base/hooks/use-grid-keyboard-nav.ts @@ -171,6 +171,8 @@ export function useGridKeyboardNav({ } case "Escape": { e.preventDefault(); + // Blurring saves the draft, as on Enter. + (document.activeElement as HTMLElement | null)?.blur(); setEditingCell(null); setFocusedCell(editingCell); break; diff --git a/apps/client/src/features/editor/components/base-embed/base-embed-view.tsx b/apps/client/src/features/editor/components/base-embed/base-embed-view.tsx index 69f7db1e..12a453ce 100644 --- a/apps/client/src/features/editor/components/base-embed/base-embed-view.tsx +++ b/apps/client/src/features/editor/components/base-embed/base-embed-view.tsx @@ -8,8 +8,12 @@ import { useBaseQuery } from "@/ee/base/queries/base-query"; import { pinOffsetWatcher } from "@docmost/editor-ext"; import { useHasFeature } from "@/ee/hooks/use-feature"; import { Feature } from "@/ee/features"; -import { IconDots, IconTable, IconX } from "@tabler/icons-react"; +import { IconDots, IconLink, IconTable, IconX } from "@tabler/icons-react"; +import { notifications } from "@mantine/notifications"; import { usePageQuery } from "@/features/page/queries/page-query"; +import { useClipboard } from "@/hooks/use-clipboard"; +import { getAppUrl } from "@/lib/config.ts"; +import { buildPageUrl } from "@/features/page/page.utils.ts"; import classes from "./base-embed.module.css"; const SIDE_GUTTER = 8; @@ -63,6 +67,14 @@ export function BaseEmbedView({ node, editor, deleteNode }: NodeViewProps) { pendingKey ? "" : pageId ?? "", ); const { data: page } = usePageQuery({ pageId: pageId ?? undefined }); + const clipboard = useClipboard({ timeout: 500 }); + + const handleCopyLink = () => { + clipboard.copy( + getAppUrl() + buildPageUrl(page.space?.slug, page.slugId, page.title), + ); + notifications.show({ message: t("Link copied") }); + }; useEffect(() => { const wrapper = wrapperRef.current; @@ -159,6 +171,13 @@ export function BaseEmbedView({ node, editor, deleteNode }: NodeViewProps) { + } + disabled={!page} + onClick={handleCopyLink} + > + {t("Copy link")} + } onClick={() => deleteNode()} diff --git a/apps/client/src/features/editor/components/base-embed/base-paste-menu.test.tsx b/apps/client/src/features/editor/components/base-embed/base-paste-menu.test.tsx new file mode 100644 index 00000000..115e7e88 --- /dev/null +++ b/apps/client/src/features/editor/components/base-embed/base-paste-menu.test.tsx @@ -0,0 +1,125 @@ +import { MantineProvider } from "@mantine/core"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import type { ComponentProps } from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { BasePasteMenu } from "./base-paste-menu"; + +const mocks = vi.hoisted(() => ({ + pasteState: null as { pos: number } | null, + hasBases: true, + convert: vi.fn(), +})); + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (value: string) => value }), +})); + +vi.mock("@tiptap/react", () => ({ + posToDOMRect: vi.fn(), + useEditorState: ({ editor, selector }: Record) => + selector({ editor }), +})); + +vi.mock("@tiptap/react/menus", () => ({ + BubbleMenu: ({ children }: { children: React.ReactNode }) => children, +})); + +vi.mock("@/features/editor/extensions/base-paste-menu", () => ({ + basePasteMenuKey: { getState: () => mocks.pasteState }, +})); + +vi.mock("@/ee/hooks/use-feature", () => ({ + useHasFeature: () => mocks.hasBases, +})); + +vi.mock("./convert-mention-to-base-embed", () => ({ + convertMentionToBaseEmbed: mocks.convert, +})); + +function setMediaMatch() { + Object.defineProperty(window, "matchMedia", { + configurable: true, + value: vi.fn().mockImplementation((query: string) => ({ + matches: false, + media: query, + onchange: null, + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + addListener: vi.fn(), + removeListener: vi.fn(), + dispatchEvent: vi.fn(), + })), + }); +} + +function renderMenu() { + const editor = { + state: { + doc: { nodeAt: () => ({ type: { name: "mention" }, nodeSize: 1 }) }, + tr: { setMeta: vi.fn() }, + }, + view: { dispatch: vi.fn(), nodeDOM: vi.fn(), hasFocus: () => true }, + commands: { focus: vi.fn() }, + isDestroyed: false, + on: vi.fn(), + off: vi.fn(), + }; + + render( + + ["editor"] + } + /> + , + ); + + return editor; +} + +describe("BasePasteMenu", () => { + beforeEach(() => { + setMediaMatch(); + mocks.pasteState = { pos: 3 }; + mocks.hasBases = true; + mocks.convert.mockReset(); + }); + + afterEach(cleanup); + + it("embeds the pasted base when its option is clicked", () => { + const editor = renderMenu(); + + fireEvent.click(screen.getByRole("option", { name: "Inline base" })); + + expect(mocks.convert).toHaveBeenCalledWith(editor, 3); + }); + + it("leaves Enter to the editor until the option is highlighted", () => { + const editor = renderMenu(); + + expect(fireEvent.keyDown(window, { key: "Enter" })).toBe(true); + expect(mocks.convert).not.toHaveBeenCalled(); + + fireEvent.keyDown(window, { key: "ArrowDown" }); + expect(fireEvent.keyDown(window, { key: "Enter" })).toBe(false); + expect(mocks.convert).toHaveBeenCalledWith(editor, 3); + }); + + it("closes on Escape and keeps the mention", () => { + const editor = renderMenu(); + + fireEvent.keyDown(window, { key: "Escape" }); + + expect(editor.view.dispatch).toHaveBeenCalled(); + expect(mocks.convert).not.toHaveBeenCalled(); + }); + + it("stays hidden without the Bases feature", () => { + mocks.hasBases = false; + renderMenu(); + + expect(screen.queryByRole("option")).toBeNull(); + }); +}); diff --git a/apps/client/src/features/editor/components/base-embed/base-paste-menu.tsx b/apps/client/src/features/editor/components/base-embed/base-paste-menu.tsx new file mode 100644 index 00000000..9da99fcf --- /dev/null +++ b/apps/client/src/features/editor/components/base-embed/base-paste-menu.tsx @@ -0,0 +1,186 @@ +import { Button, Paper, Text, VisuallyHidden } from "@mantine/core"; +import { posToDOMRect, useEditorState } from "@tiptap/react"; +import { BubbleMenu as BaseBubbleMenu } from "@tiptap/react/menus"; +import { useCallback, useEffect, useId, useRef, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { EditorMenuProps } from "@/features/editor/components/table/types/types.ts"; +import { basePasteMenuKey } from "@/features/editor/extensions/base-paste-menu"; +import { useHasFeature } from "@/ee/hooks/use-feature"; +import { Feature } from "@/ee/features"; +import { convertMentionToBaseEmbed } from "./convert-mention-to-base-embed"; +import classes from "../integration-link/integration-paste-menu.module.css"; + +function getPastedBasePos(editor: EditorMenuProps["editor"]): number | null { + const pasteState = basePasteMenuKey.getState(editor.state); + if (!pasteState) return null; + const node = editor.state.doc.nodeAt(pasteState.pos); + return node?.type.name === "mention" ? pasteState.pos : null; +} + +export function BasePasteMenu({ editor }: EditorMenuProps) { + const { t } = useTranslation(); + const hasBases = useHasFeature(Feature.BASES); + const [highlighted, setHighlighted] = useState(false); + const optionId = useId(); + const promptRef = useRef(null); + + const pastedPos = useEditorState({ + editor, + selector: ({ editor: currentEditor }) => + currentEditor ? getPastedBasePos(currentEditor) : null, + }); + const isOpen = hasBases && pastedPos !== null; + + const shouldShow = useCallback( + () => hasBases && getPastedBasePos(editor) !== null, + [editor, hasBases], + ); + + const getReferencedVirtualElement = useCallback(() => { + const pos = getPastedBasePos(editor); + if (pos === null) return undefined; + const node = editor.state.doc.nodeAt(pos); + if (!node) return undefined; + + const dom = editor.view.nodeDOM(pos) as HTMLElement | null; + const domRect = + dom?.getBoundingClientRect?.() ?? + posToDOMRect(editor.view, pos, pos + node.nodeSize); + + return { + getBoundingClientRect: () => domRect, + getClientRects: () => [domRect], + }; + }, [editor]); + + const dismiss = useCallback(() => { + if (!basePasteMenuKey.getState(editor.state)) return; + editor.view.dispatch(editor.state.tr.setMeta(basePasteMenuKey, null)); + + requestAnimationFrame(() => { + const activeElement = document.activeElement; + if ( + !activeElement || + activeElement === document.body || + !activeElement.isConnected + ) { + editor.commands.focus(undefined, { scrollIntoView: false }); + } + }); + }, [editor]); + + const embed = useCallback(() => { + const pos = getPastedBasePos(editor); + if (pos === null) return; + + dismiss(); + if (convertMentionToBaseEmbed(editor, pos)) { + editor.commands.focus(undefined, { scrollIntoView: false }); + } + }, [dismiss, editor]); + + useEffect(() => setHighlighted(false), [pastedPos]); + + // Focus stays in the editor, so capture keys before ProseMirror handles them. + useEffect(() => { + if (!isOpen) return; + + const ownsFocus = () => + editor.view.hasFocus() || + Boolean(promptRef.current?.contains(document.activeElement)); + + const onKeyDown = (event: KeyboardEvent) => { + if (!ownsFocus()) return; + + if (event.key === "Escape") { + event.preventDefault(); + event.stopPropagation(); + dismiss(); + return; + } + + if (event.key === "ArrowDown" || event.key === "ArrowUp") { + event.preventDefault(); + event.stopPropagation(); + setHighlighted(true); + return; + } + + // Until the option is highlighted, Enter is still a new line. + if (event.key === "Enter" && highlighted) { + event.preventDefault(); + event.stopPropagation(); + embed(); + } + }; + + // Leaving the editor dismisses the prompt, unless focus moved into the prompt itself. + const onBlur = ({ event }: { event: FocusEvent }) => { + const next = event.relatedTarget; + if (next instanceof Node && promptRef.current?.contains(next)) return; + if (editor.isDestroyed) return; + editor.view.dispatch(editor.state.tr.setMeta(basePasteMenuKey, null)); + }; + + window.addEventListener("keydown", onKeyDown, true); + editor.on("blur", onBlur); + return () => { + window.removeEventListener("keydown", onKeyDown, true); + editor.off("blur", onBlur); + }; + }, [dismiss, editor, embed, highlighted, isOpen]); + + return ( + + {isOpen ? ( + + + {highlighted ? t("Inline base") : ""} + + + {t("Paste as")} + + + + ) : null} + + ); +} diff --git a/apps/client/src/features/editor/components/base-embed/base-paste.test.ts b/apps/client/src/features/editor/components/base-embed/base-paste.test.ts new file mode 100644 index 00000000..8211b6ff --- /dev/null +++ b/apps/client/src/features/editor/components/base-embed/base-paste.test.ts @@ -0,0 +1,151 @@ +import { BaseEmbed, Mention } from "@docmost/editor-ext"; +import { Editor, type JSONContent } from "@tiptap/core"; +import { StarterKit } from "@tiptap/starter-kit"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + BasePasteMenuExtension, + basePasteMenuKey, +} from "@/features/editor/extensions/base-paste-menu"; +import { handlePaste } from "@/features/editor/components/common/editor-paste-handler"; +import { convertMentionToBaseEmbed } from "./convert-mention-to-base-embed"; + +const mocks = vi.hoisted(() => ({ getPageById: vi.fn() })); + +vi.mock("@/main.tsx", async () => { + const { QueryClient } = await import("@tanstack/react-query"); + return { queryClient: new QueryClient() }; +}); + +vi.mock("@/features/page/services/page-service.ts", async (importOriginal) => ({ + ...(await importOriginal()), + getPageById: mocks.getPageById, +})); + +const PAGE_ID = "019a2b3c-0000-7000-8000-000000000001"; +const LINK = `${window.location.origin}/s/general/p/roadmap-x1y2z3`; + +let editor: Editor; + +function createEditor(content: JSONContent[], cursor: number) { + editor = new Editor({ + extensions: [StarterKit, Mention, BaseEmbed, BasePasteMenuExtension], + content: { type: "doc", content }, + }); + editor.commands.setTextSelection(cursor); +} + +function mentionPos(): number | null { + let found: number | null = null; + editor.state.doc.descendants((node, pos) => { + if (node.type.name === "mention") found = pos; + }); + return found; +} + +async function pasteLink({ + isBase, + url = LINK, +}: { + isBase: boolean; + url?: string; +}) { + mocks.getPageById.mockResolvedValue({ + id: PAGE_ID, + slugId: "x1y2z3", + title: "Roadmap", + isBase, + }); + const event = { + clipboardData: { + getData: (type: string) => (type === "text/plain" ? url : ""), + files: [], + }, + preventDefault: vi.fn(), + }; + handlePaste(editor, event as unknown as ClipboardEvent, "page-id"); + await vi.waitFor(() => expect(mentionPos()).not.toBeNull()); +} + +afterEach(() => editor?.destroy()); + +describe("pasting a link to a base", () => { + it("offers to embed it", async () => { + createEditor([{ type: "paragraph" }], 1); + + await pasteLink({ isBase: true }); + + expect(basePasteMenuKey.getState(editor.state)).toEqual({ + pos: mentionPos(), + }); + }); + + it("offers to embed a link to one of its views", async () => { + createEditor([{ type: "paragraph" }], 1); + + await pasteLink({ + isBase: true, + url: `${LINK}?view=0195f3c2-1111-7000-8000-000000000abc`, + }); + + expect(mocks.getPageById).toHaveBeenCalledWith({ pageId: "x1y2z3" }); + expect(basePasteMenuKey.getState(editor.state)).toEqual({ + pos: mentionPos(), + }); + }); + + it("does not offer it for a document", async () => { + createEditor([{ type: "paragraph" }], 1); + + await pasteLink({ isBase: false }); + + expect(basePasteMenuKey.getState(editor.state)).toBeNull(); + }); + + it("does not offer it where a block cannot go", async () => { + createEditor( + [ + { + type: "heading", + attrs: { level: 2 }, + content: [{ type: "text", text: "Plan " }], + }, + ], + 6, + ); + + await pasteLink({ isBase: true }); + + expect(basePasteMenuKey.getState(editor.state)).toBeNull(); + }); +}); + +describe("embedding a pasted base", () => { + it("replaces a mention on its own line with the base", async () => { + createEditor([{ type: "paragraph" }], 1); + await pasteLink({ isBase: true }); + + expect(convertMentionToBaseEmbed(editor, mentionPos()!)).toBe(true); + + // The trailing paragraph comes from the trailing node, as in the page editor. + expect(editor.getJSON().content).toMatchObject([ + { type: "base", attrs: { pageId: PAGE_ID } }, + { type: "paragraph" }, + ]); + }); + + it("splits the paragraph around a mention pasted mid-sentence", async () => { + createEditor( + [{ type: "paragraph", content: [{ type: "text", text: "See for dates" }] }], + 5, + ); + await pasteLink({ isBase: true }); + + expect(convertMentionToBaseEmbed(editor, mentionPos()!)).toBe(true); + + expect(editor.getJSON().content).toMatchObject([ + { type: "paragraph", content: [{ type: "text", text: "See" }] }, + { type: "base", attrs: { pageId: PAGE_ID } }, + { type: "paragraph", content: [{ type: "text", text: "for dates" }] }, + ]); + }); +}); diff --git a/apps/client/src/features/editor/components/base-embed/convert-mention-to-base-embed.ts b/apps/client/src/features/editor/components/base-embed/convert-mention-to-base-embed.ts new file mode 100644 index 00000000..4db885d9 --- /dev/null +++ b/apps/client/src/features/editor/components/base-embed/convert-mention-to-base-embed.ts @@ -0,0 +1,33 @@ +import type { Editor } from "@tiptap/core"; +import type { Node as ProseMirrorNode } from "@tiptap/pm/model"; +import { getBlockPlacement } from "@/features/editor/utils/block-placement"; + +function getEmbedPlacement(doc: ProseMirrorNode, pos: number) { + const mention = doc.nodeAt(pos); + const baseType = doc.type.schema.nodes.base; + if (mention?.type.name !== "mention" || !baseType) return null; + + return getBlockPlacement( + doc, + pos, + pos + mention.nodeSize, + baseType.create({ pageId: mention.attrs.entityId }), + ); +} + +export function canConvertMentionToBaseEmbed( + doc: ProseMirrorNode, + pos: number, +): boolean { + return getEmbedPlacement(doc, pos) !== null; +} + +export function convertMentionToBaseEmbed(editor: Editor, pos: number): boolean { + const placement = getEmbedPlacement(editor.state.doc, pos); + if (!placement) return false; + + return editor.commands.command(({ tr }) => { + tr.replaceWith(placement.range.from, placement.range.to, placement.content); + return true; + }); +} diff --git a/apps/client/src/features/editor/components/common/editor-paste-handler.tsx b/apps/client/src/features/editor/components/common/editor-paste-handler.tsx index 642d05a9..212af981 100644 --- a/apps/client/src/features/editor/components/common/editor-paste-handler.tsx +++ b/apps/client/src/features/editor/components/common/editor-paste-handler.tsx @@ -131,10 +131,12 @@ export const handlePaste = ( } } - if (INTERNAL_LINK_REGEX.test(clipboardData)) { + // A query, such as a base's ?view=, still links to the same page. + const pageLink = clipboardData.replace(/\?[^#]*/, ""); + if (INTERNAL_LINK_REGEX.test(pageLink)) { // we have to do this validation here to allow the default link extension to takeover if needs be event.preventDefault(); - const url = clipboardData.trim(); + const url = pageLink.trim(); const { from: pos, empty } = editor.state.selection; const match = INTERNAL_LINK_REGEX.exec(url); diff --git a/apps/client/src/features/editor/components/fixed-toolbar/groups/more-inserts-group.tsx b/apps/client/src/features/editor/components/fixed-toolbar/groups/more-inserts-group.tsx index f2ae8b91..d2f8f1a1 100644 --- a/apps/client/src/features/editor/components/fixed-toolbar/groups/more-inserts-group.tsx +++ b/apps/client/src/features/editor/components/fixed-toolbar/groups/more-inserts-group.tsx @@ -8,6 +8,7 @@ import { IconChevronDown, IconInfoCircle, IconLayoutKanban, + IconLayoutNavbar, IconMath, IconMathFunction, IconRotate2, @@ -87,6 +88,13 @@ export const MoreInsertsGroup: FC = ({ editor, templateMode }) => { > {t("Toggle block")} + } + disabled={!editor.can().insertTabs()} + onClick={() => editor.chain().focus().insertTabs().run()} + > + {t("Tabs")} + } onClick={() => diff --git a/apps/client/src/features/editor/components/integration-link/integration-display.ts b/apps/client/src/features/editor/components/integration-link/integration-display.ts index 61b6e330..838ac027 100644 --- a/apps/client/src/features/editor/components/integration-link/integration-display.ts +++ b/apps/client/src/features/editor/components/integration-link/integration-display.ts @@ -1,6 +1,7 @@ import { getMarkRange, type Editor } from "@tiptap/core"; -import { Fragment, type Node as ProseMirrorNode } from "@tiptap/pm/model"; +import type { Fragment } from "@tiptap/pm/model"; import { NodeSelection } from "@tiptap/pm/state"; +import { getBlockPlacement } from "@/features/editor/utils/block-placement"; export type IntegrationDisplay = "card" | "mention" | "url"; @@ -66,74 +67,13 @@ function getCardPlacement( range: null; content: null; } { - const $from = editor.state.doc.resolve(from); - if ($from.parent.type.name !== "paragraph") { - return { canPlace: false, range: null, content: null }; - } - - const parentStart = $from.start(); - const parentEnd = parentStart + $from.parent.content.size; - if (from < parentStart || to > parentEnd || from > to) { - return { canPlace: false, range: null, content: null }; - } - - const depth = $from.depth; - const parentContainer = $from.node(depth - 1); - const parentIndex = $from.index(depth - 1); const cardType = editor.state.schema.nodes.integrationCard; - if (!cardType) { - return { canPlace: false, range: null, content: null }; - } - - const before = trimInlineBoundary( - $from.parent.content.cut(0, from - parentStart), - "end", - ); - const after = trimInlineBoundary( - $from.parent.content.cut(to - parentStart), - "start", - ); - const replacement: ProseMirrorNode[] = []; - if (before.size) replacement.push($from.parent.copy(before)); - replacement.push(cardType.create(attrs)); - if (after.size) replacement.push($from.parent.copy(after)); - - const content = Fragment.fromArray(replacement); - if (!parentContainer.canReplace(parentIndex, parentIndex + 1, content)) { - return { canPlace: false, range: null, content: null }; - } - - return { - canPlace: true, - range: { from: $from.before(depth), to: $from.after(depth) }, - content, - }; -} - -function trimInlineBoundary( - fragment: Fragment, - boundary: "start" | "end", -): Fragment { - const children: ProseMirrorNode[] = []; - fragment.forEach((child) => children.push(child)); - - while (children.length) { - const index = boundary === "start" ? 0 : children.length - 1; - const child = children[index]; - if (!child?.isText) break; - - const text = - boundary === "start" - ? child.text?.replace(/^\s+/, "") - : child.text?.replace(/\s+$/, ""); - if (text) { - children[index] = child.type.schema.text(text, child.marks); - break; - } - children.splice(index, 1); - } - - return Fragment.fromArray(children); + const placement = cardType + ? getBlockPlacement(editor.state.doc, from, to, cardType.create(attrs)) + : null; + return placement + ? { canPlace: true, ...placement } + : { canPlace: false, range: null, content: null }; } function replaceWithCard( diff --git a/apps/client/src/features/editor/components/link/internal-link-paste.ts b/apps/client/src/features/editor/components/link/internal-link-paste.ts index 61aa2b69..b59d9b49 100644 --- a/apps/client/src/features/editor/components/link/internal-link-paste.ts +++ b/apps/client/src/features/editor/components/link/internal-link-paste.ts @@ -3,6 +3,8 @@ import { getPageById } from "@/features/page/services/page-service.ts"; import { IPage } from "@/features/page/types/page.types.ts"; import { v7 } from "uuid"; import { extractPageSlugId } from "@/lib"; +import { basePasteMenuKey } from "@/features/editor/extensions/base-paste-menu"; +import { canConvertMentionToBaseEmbed } from "@/features/editor/components/base-embed/convert-mention-to-base-embed"; export type LinkFn = ( url: string, @@ -42,6 +44,9 @@ export const handleInternalLink = if (!node) return; const transaction = view.state.tr.replaceWith(pos, pos, node); + if (page.isBase && canConvertMentionToBaseEmbed(transaction.doc, pos)) { + transaction.setMeta(basePasteMenuKey, { pos }); + } view.dispatch(transaction); }, () => { diff --git a/apps/client/src/features/editor/extensions/base-paste-menu.ts b/apps/client/src/features/editor/extensions/base-paste-menu.ts new file mode 100644 index 00000000..d9d08b95 --- /dev/null +++ b/apps/client/src/features/editor/extensions/base-paste-menu.ts @@ -0,0 +1,39 @@ +import { Extension } from "@tiptap/core"; +import { Plugin, PluginKey } from "@tiptap/pm/state"; + +export type BasePasteMenuState = { pos: number } | null; + +export const basePasteMenuKey = new PluginKey( + "basePasteMenu", +); + +// Position of a just-pasted base mention, where the "Paste as" menu offers its embed. +export const BasePasteMenuExtension = Extension.create({ + name: "basePasteMenu", + + addProseMirrorPlugins() { + return [ + new Plugin({ + key: basePasteMenuKey, + state: { + init: (): BasePasteMenuState => null, + apply(tr, prev): BasePasteMenuState { + const meta = tr.getMeta(basePasteMenuKey); + if (meta !== undefined) return meta; + if (!prev) return null; + // Clicking or typing elsewhere dismisses. + if (tr.selectionSet) return null; + // Follow-up transactions (unique ids, trailing node) keep the anchor. + if (tr.docChanged) { + const pos = tr.mapping.map(prev.pos); + return tr.doc.nodeAt(pos)?.type.name === "mention" + ? { pos } + : null; + } + return prev; + }, + }, + }), + ]; + }, +}); diff --git a/apps/client/src/features/editor/extensions/extensions.ts b/apps/client/src/features/editor/extensions/extensions.ts index 8079eaf7..484b6d7c 100644 --- a/apps/client/src/features/editor/extensions/extensions.ts +++ b/apps/client/src/features/editor/extensions/extensions.ts @@ -104,6 +104,7 @@ import SubpagesView from "@/features/editor/components/subpages/subpages-view.ts import IntegrationCardView from "@/features/editor/components/integration-link/integration-card-view.tsx"; import IntegrationMentionView from "@/features/editor/components/integration-link/integration-mention-view.tsx"; import { IntegrationPasteMenuExtension } from "@/features/editor/extensions/integration-paste-menu"; +import { BasePasteMenuExtension } from "@/features/editor/extensions/base-paste-menu"; import TabsView from "@/features/editor/components/tabs/tabs-view.tsx"; import TransclusionView from "@/features/editor/components/transclusion/transclusion-view.tsx"; import TransclusionReferenceView from "@/features/editor/components/transclusion/transclusion-reference-view.tsx"; @@ -408,6 +409,7 @@ export const mainExtensions = [ view: IntegrationMentionView, }), IntegrationPasteMenuExtension, + BasePasteMenuExtension, Status.configure({ view: StatusView, }), diff --git a/apps/client/src/features/editor/page-editor.tsx b/apps/client/src/features/editor/page-editor.tsx index 4f7a8b18..79b57ec6 100644 --- a/apps/client/src/features/editor/page-editor.tsx +++ b/apps/client/src/features/editor/page-editor.tsx @@ -83,6 +83,7 @@ import { EditorAiMenu } from "@/ee/ai/components/editor/ai-menu/ai-menu"; import { EditorLinkMenu } from "@/features/editor/components/link/link-menu"; import ColumnsMenu from "@/features/editor/components/columns/columns-menu.tsx"; import { IntegrationPasteMenu } from "@/features/editor/components/integration-link/integration-paste-menu.tsx"; +import { BasePasteMenu } from "@/features/editor/components/base-embed/base-paste-menu.tsx"; import { getInstalledIntegrations } from "@/features/integration/services/integration-service"; import { TransclusionLookupProvider } from "@/features/editor/components/transclusion/transclusion-lookup-context"; import { useTranslation } from "react-i18next"; @@ -527,6 +528,7 @@ function CollabPageEditor({ + )} {editor && !editorIsEditable && (editable || canComment) && ( diff --git a/apps/client/src/features/editor/utils/block-placement.ts b/apps/client/src/features/editor/utils/block-placement.ts new file mode 100644 index 00000000..e60084c2 --- /dev/null +++ b/apps/client/src/features/editor/utils/block-placement.ts @@ -0,0 +1,74 @@ +import { Fragment, type Node as ProseMirrorNode } from "@tiptap/pm/model"; + +export type BlockPlacement = { + range: { from: number; to: number }; + content: Fragment; +}; + +// Swaps an inline range for a block, splitting its paragraph around it; null where the block can't go. +export function getBlockPlacement( + doc: ProseMirrorNode, + from: number, + to: number, + block: ProseMirrorNode, +): BlockPlacement | null { + const $from = doc.resolve(from); + if ($from.parent.type.name !== "paragraph") return null; + + const parentStart = $from.start(); + const parentEnd = parentStart + $from.parent.content.size; + if (from < parentStart || to > parentEnd || from > to) return null; + + const depth = $from.depth; + const parentContainer = $from.node(depth - 1); + const parentIndex = $from.index(depth - 1); + + const before = trimInlineBoundary( + $from.parent.content.cut(0, from - parentStart), + "end", + ); + const after = trimInlineBoundary( + $from.parent.content.cut(to - parentStart), + "start", + ); + const replacement: ProseMirrorNode[] = []; + if (before.size) replacement.push($from.parent.copy(before)); + replacement.push(block); + if (after.size) replacement.push($from.parent.copy(after)); + + const content = Fragment.fromArray(replacement); + if (!parentContainer.canReplace(parentIndex, parentIndex + 1, content)) { + return null; + } + + return { + range: { from: $from.before(depth), to: $from.after(depth) }, + content, + }; +} + +function trimInlineBoundary( + fragment: Fragment, + boundary: "start" | "end", +): Fragment { + const children: ProseMirrorNode[] = []; + fragment.forEach((child) => children.push(child)); + + while (children.length) { + const index = boundary === "start" ? 0 : children.length - 1; + const child = children[index]; + if (!child?.isText) break; + + const text = + boundary === "start" + ? child.text?.replace(/^\s+/, "") + : child.text?.replace(/\s+$/, ""); + if (text) { + children[index] = child.type.schema.text(text, child.marks); + break; + } + children.splice(index, 1); + } + + return Fragment.fromArray(children); +} diff --git a/apps/server/src/core/integration/integration.module.ts b/apps/server/src/core/integration/integration.module.ts index 3b152145..d6aad24a 100644 --- a/apps/server/src/core/integration/integration.module.ts +++ b/apps/server/src/core/integration/integration.module.ts @@ -11,11 +11,17 @@ import { IntegrationRepo } from './repos/integration.repo'; import { IntegrationConnectionRepo } from './repos/integration-connection.repo'; import { IntegrationListener } from './integration.listener'; import { IntegrationProcessor } from './integration.processor'; +import { EnvironmentService } from '../../integrations/environment/environment.service'; @Module({ controllers: [IntegrationController, OAuthController, UnfurlController], providers: [ - IntegrationRegistry, + { + provide: IntegrationRegistry, + useFactory: (environmentService: EnvironmentService) => + new IntegrationRegistry(environmentService.isCloud()), + inject: [EnvironmentService], + }, IntegrationService, IntegrationConnectionService, OAuthService, diff --git a/apps/server/src/core/integration/providers/github/github.provider.spec.ts b/apps/server/src/core/integration/providers/github/github.provider.spec.ts index 4afb54f0..0e0cf3e2 100644 --- a/apps/server/src/core/integration/providers/github/github.provider.spec.ts +++ b/apps/server/src/core/integration/providers/github/github.provider.spec.ts @@ -61,7 +61,7 @@ describe('GitHubProvider host matching', () => { const { provider, unfurlPullRequest } = buildProvider(); await unfurlWith(provider, PUBLIC_PR); expect(unfurlPullRequest).toHaveBeenCalledWith( - 'token', 'https://api.github.com', 'o', 'r', 1, PUBLIC_PR, + 'Bearer token', 'https://api.github.com', 'o', 'r', 1, PUBLIC_PR, ); }); }); @@ -87,7 +87,7 @@ describe('GitHubProvider host matching', () => { const { provider, unfurlPullRequest } = buildProvider(); await unfurlWith(provider, GHE_PR); expect(unfurlPullRequest).toHaveBeenCalledWith( - 'token', `${GHE_BASE}/api/v3`, 'o', 'r', 1, GHE_PR, + 'Bearer token', `${GHE_BASE}/api/v3`, 'o', 'r', 1, GHE_PR, ); }); diff --git a/apps/server/src/core/integration/providers/github/github.provider.ts b/apps/server/src/core/integration/providers/github/github.provider.ts index a179e029..6850eb94 100644 --- a/apps/server/src/core/integration/providers/github/github.provider.ts +++ b/apps/server/src/core/integration/providers/github/github.provider.ts @@ -5,6 +5,7 @@ import { LinkDescription, OAuthConfig, ProviderAccount, + PublicUnfurlOpts, ResolveAccountOpts, UnfurlPattern, UnfurlOpts, @@ -13,6 +14,7 @@ import { import { GitHubService } from './github.service'; import { buildGitHubPatterns } from './github-patterns'; import { normalizeBaseUrl } from '../../utils/integration.utils'; +import { credentialEnvKey } from '../../oauth/credential-env'; const DEFAULT_BASE_URL = 'https://github.com'; @@ -63,13 +65,35 @@ export class GitHubProvider extends IntegrationProvider { async resolveAccount(opts: ResolveAccountOpts): Promise { return this.githubService.getAccount( - opts.accessToken, + `Bearer ${opts.accessToken}`, this.resolveApiBaseUrl(), ); } async unfurl(opts: UnfurlOpts): Promise { - const { match, patternType, accessToken, url } = opts; + return this.unfurlWith(`Bearer ${opts.accessToken}`, opts); + } + + // The OAuth app's own credentials only see public repos, so private ones 404. + async unfurlPublic(opts: PublicUnfurlOpts): Promise { + const clientId = + process.env[credentialEnvKey('CLIENT_ID', this.definition.type)]; + const clientSecret = + process.env[credentialEnvKey('CLIENT_SECRET', this.definition.type)]; + if (!clientId || !clientSecret) { + throw new Error('GitHub client ID or secret is not set'); + } + const credentials = Buffer.from(`${clientId}:${clientSecret}`).toString( + 'base64', + ); + return this.unfurlWith(`Basic ${credentials}`, opts); + } + + private async unfurlWith( + authorization: string, + opts: PublicUnfurlOpts, + ): Promise { + const { match, patternType, url } = opts; const apiBaseUrl = this.resolveApiBaseUrl(); const owner = match[1]; const repo = match[2]; @@ -78,33 +102,33 @@ export class GitHubProvider extends IntegrationProvider { case 'github-pr': { const number = parseInt(match[3], 10); return this.githubService.unfurlPullRequest( - accessToken, apiBaseUrl, owner, repo, number, url, + authorization, apiBaseUrl, owner, repo, number, url, ); } case 'github-issue': { const number = parseInt(match[3], 10); return this.githubService.unfurlIssue( - accessToken, apiBaseUrl, owner, repo, number, url, + authorization, apiBaseUrl, owner, repo, number, url, ); } case 'github-repo': return this.githubService.unfurlRepo( - accessToken, apiBaseUrl, owner, repo, url, + authorization, apiBaseUrl, owner, repo, url, ); case 'github-commit': { const sha = match[3]; return this.githubService.unfurlCommit( - accessToken, apiBaseUrl, owner, repo, sha, url, + authorization, apiBaseUrl, owner, repo, sha, url, ); } case 'github-pr-commit': { const sha = match[4]; return this.githubService.unfurlCommit( - accessToken, apiBaseUrl, owner, repo, sha, url, + authorization, apiBaseUrl, owner, repo, sha, url, ); } @@ -122,7 +146,7 @@ export class GitHubProvider extends IntegrationProvider { case 'github-issues-list': case 'github-releases-list': return this.githubService.unfurlCollectionPage( - accessToken, apiBaseUrl, owner, repo, patternType.replace('github-', ''), url, + authorization, apiBaseUrl, owner, repo, patternType.replace('github-', ''), url, ); default: diff --git a/apps/server/src/core/integration/providers/github/github.service.spec.ts b/apps/server/src/core/integration/providers/github/github.service.spec.ts index 63703b7b..24d6b0ab 100644 --- a/apps/server/src/core/integration/providers/github/github.service.spec.ts +++ b/apps/server/src/core/integration/providers/github/github.service.spec.ts @@ -28,11 +28,11 @@ const BODY = { const service = new GitHubService(); const REQUESTS: [string, (api: string, owner: string, repo: string) => Promise, string][] = [ - ['pull request', (api, owner, repo) => service.unfurlPullRequest('token', api, owner, repo, 1, PAGE), '/pulls/1'], - ['issue', (api, owner, repo) => service.unfurlIssue('token', api, owner, repo, 2, PAGE), '/issues/2'], - ['repo', (api, owner, repo) => service.unfurlRepo('token', api, owner, repo, PAGE), ''], - ['commit', (api, owner, repo) => service.unfurlCommit('token', api, owner, repo, 'abc1234', PAGE), '/commits/abc1234'], - ['collection page', (api, owner, repo) => service.unfurlCollectionPage('token', api, owner, repo, 'pulls-list', PAGE), ''], + ['pull request', (api, owner, repo) => service.unfurlPullRequest('Bearer token', api, owner, repo, 1, PAGE), '/pulls/1'], + ['issue', (api, owner, repo) => service.unfurlIssue('Bearer token', api, owner, repo, 2, PAGE), '/issues/2'], + ['repo', (api, owner, repo) => service.unfurlRepo('Bearer token', api, owner, repo, PAGE), ''], + ['commit', (api, owner, repo) => service.unfurlCommit('Bearer token', api, owner, repo, 'abc1234', PAGE), '/commits/abc1234'], + ['collection page', (api, owner, repo) => service.unfurlCollectionPage('Bearer token', api, owner, repo, 'pulls-list', PAGE), ''], ]; function requestedUrls(): string[] { @@ -144,3 +144,57 @@ describe('GitHub connected account', () => { ); }); }); + +describe('GitHub public unfurl', () => { + const originalEnv = process.env; + + beforeEach(() => { + process.env = { + ...originalEnv, + INTEGRATION_GITHUB_CLIENT_ID: 'app-id', + INTEGRATION_GITHUB_CLIENT_SECRET: 'app-secret', + }; + delete process.env.INTEGRATION_GITHUB_URL; + fetchMock.mockReset(); + fetchMock.mockImplementation( + async () => + new Response(JSON.stringify(BODY), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ); + }); + + afterEach(() => { + process.env = originalEnv; + }); + + function unfurlPublic(url: string) { + const provider = new GitHubProvider(new GitHubService()); + for (const pattern of provider.getUnfurlPatterns({})) { + const match = url.match(pattern.regex); + if (match) { + return provider.unfurlPublic({ url, match, patternType: pattern.type }); + } + } + throw new Error(`No GitHub pattern matches ${url}`); + } + + it('reads a pull request with the app client ID and secret instead of a member token', async () => { + await expect(unfurlPublic(PAGE)).resolves.toMatchObject({ + title: 'Add dark mode', + }); + + const [url, init] = fetchMock.mock.calls[0]; + expect(url).toBe('https://api.github.com/repos/o/r/pulls/1'); + // base64 of "app-id:app-secret" + expect(init.headers.Authorization).toBe('Basic YXBwLWlkOmFwcC1zZWNyZXQ='); + }); + + it('never calls GitHub without the app credentials', async () => { + delete process.env.INTEGRATION_GITHUB_CLIENT_SECRET; + + await expect(unfurlPublic(PAGE)).rejects.toThrow(); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/src/core/integration/providers/github/github.service.ts b/apps/server/src/core/integration/providers/github/github.service.ts index e5f02241..9d32d3b5 100644 --- a/apps/server/src/core/integration/providers/github/github.service.ts +++ b/apps/server/src/core/integration/providers/github/github.service.ts @@ -11,10 +11,10 @@ import { repoApiPath } from './github.utils'; @Injectable() export class GitHubService { async getAccount( - accessToken: string, + authorization: string, apiBaseUrl: string, ): Promise { - const data = await this.apiGet(accessToken, apiBaseUrl, '/user'); + const data = await this.apiGet(authorization, apiBaseUrl, '/user'); return toProviderAccount({ id: data.id, displayName: data.name, @@ -23,7 +23,7 @@ export class GitHubService { } async unfurlPullRequest( - accessToken: string, + authorization: string, apiBaseUrl: string, owner: string, repo: string, @@ -31,7 +31,7 @@ export class GitHubService { url: string, ): Promise { const data = await this.apiGet( - accessToken, + authorization, apiBaseUrl, `${repoApiPath(owner, repo)}/pulls/${number}`, ); @@ -66,7 +66,7 @@ export class GitHubService { } async unfurlIssue( - accessToken: string, + authorization: string, apiBaseUrl: string, owner: string, repo: string, @@ -74,7 +74,7 @@ export class GitHubService { url: string, ): Promise { const data = await this.apiGet( - accessToken, + authorization, apiBaseUrl, `${repoApiPath(owner, repo)}/issues/${number}`, ); @@ -107,14 +107,14 @@ export class GitHubService { } async unfurlRepo( - accessToken: string, + authorization: string, apiBaseUrl: string, owner: string, repo: string, url: string, ): Promise { const data = await this.apiGet( - accessToken, + authorization, apiBaseUrl, repoApiPath(owner, repo), ); @@ -143,7 +143,7 @@ export class GitHubService { } async unfurlCommit( - accessToken: string, + authorization: string, apiBaseUrl: string, owner: string, repo: string, @@ -151,7 +151,7 @@ export class GitHubService { url: string, ): Promise { const data = await this.apiGet( - accessToken, + authorization, apiBaseUrl, `${repoApiPath(owner, repo)}/commits/${sha}`, ); @@ -217,7 +217,7 @@ export class GitHubService { } async unfurlCollectionPage( - accessToken: string, + authorization: string, apiBaseUrl: string, owner: string, repo: string, @@ -225,7 +225,7 @@ export class GitHubService { url: string, ): Promise { const data = await this.apiGet( - accessToken, + authorization, apiBaseUrl, repoApiPath(owner, repo), ); @@ -265,13 +265,13 @@ export class GitHubService { } private async apiGet( - accessToken: string, + authorization: string, apiBaseUrl: string, path: string, ): Promise { const response = await providerApiFetch('GitHub', `${apiBaseUrl}${path}`, { headers: { - Authorization: `Bearer ${accessToken}`, + Authorization: authorization, Accept: 'application/vnd.github.v3+json', 'User-Agent': 'Docmost', }, diff --git a/apps/server/src/core/integration/registry/integration-provider.interface.ts b/apps/server/src/core/integration/registry/integration-provider.interface.ts index 43d1aab4..df00a528 100644 --- a/apps/server/src/core/integration/registry/integration-provider.interface.ts +++ b/apps/server/src/core/integration/registry/integration-provider.interface.ts @@ -140,6 +140,8 @@ export type UnfurlOpts = { integrationId: string; }; +export type PublicUnfurlOpts = Pick; + // UnfurlService shows no card for it instead of logging an error. export class UnfurlForbiddenError extends Error { constructor(message = 'Not authorized to unfurl this link') { @@ -221,6 +223,9 @@ export abstract class IntegrationProvider { unfurl?(opts: UnfurlOpts): Promise; + // For members without a connection; must only read public data, since every workspace shares the card. + unfurlPublic?(opts: PublicUnfurlOpts): Promise; + // Tokenless summary for the connect prompt, e.g. "Pull Request #13337". describeLink?( patternType: string, diff --git a/apps/server/src/core/integration/registry/integration-registry.spec.ts b/apps/server/src/core/integration/registry/integration-registry.spec.ts index 48ad4f53..7607d0db 100644 --- a/apps/server/src/core/integration/registry/integration-registry.spec.ts +++ b/apps/server/src/core/integration/registry/integration-registry.spec.ts @@ -22,4 +22,30 @@ describe('IntegrationRegistry', () => { 'Integration provider "linear" is already registered', ); }); + + describe('available integrations', () => { + const types = ['github', 'figma', 'linear', 'jira', 'gitlab']; + + it('offers only GitHub, GitLab and Linear on Cloud', () => { + const registry = new IntegrationRegistry(true); + types.forEach((type) => registry.register(provider(type))); + + expect(registry.getAvailableIntegrations().map((d) => d.type)).toEqual([ + 'github', + 'linear', + 'gitlab', + ]); + // Existing installs of the others keep unfurling. + expect(registry.getProvider('figma')).toBeDefined(); + }); + + it('offers every provider on self-hosted', () => { + const registry = new IntegrationRegistry(); + types.forEach((type) => registry.register(provider(type))); + + expect(registry.getAvailableIntegrations().map((d) => d.type)).toEqual( + types, + ); + }); + }); }); diff --git a/apps/server/src/core/integration/registry/integration-registry.ts b/apps/server/src/core/integration/registry/integration-registry.ts index 02edfd4f..5c1ba44d 100644 --- a/apps/server/src/core/integration/registry/integration-registry.ts +++ b/apps/server/src/core/integration/registry/integration-registry.ts @@ -4,15 +4,23 @@ import { IntegrationProvider, } from './integration-provider.interface'; +// Docmost Cloud only offers the providers it has OAuth apps for. +const CLOUD_INTEGRATION_TYPES = new Set(['github', 'gitlab', 'linear']); + @Injectable() export class IntegrationRegistry { private providers = new Map(); + constructor(private readonly cloud = false) {} + register(provider: IntegrationProvider): void { const { type } = provider.definition; if (this.providers.has(type)) { throw new Error(`Integration provider "${type}" is already registered`); } + if (this.cloud && !CLOUD_INTEGRATION_TYPES.has(type)) { + provider.definition.hidden = true; + } this.providers.set(type, provider); } diff --git a/apps/server/src/core/integration/unfurl/unfurl.service.spec.ts b/apps/server/src/core/integration/unfurl/unfurl.service.spec.ts index 5621389d..0c755373 100644 --- a/apps/server/src/core/integration/unfurl/unfurl.service.spec.ts +++ b/apps/server/src/core/integration/unfurl/unfurl.service.spec.ts @@ -1,16 +1,32 @@ import * as crypto from 'crypto'; +import { Logger } from '@nestjs/common'; import { UnfurlService } from './unfurl.service'; import { IntegrationRegistry } from '../registry/integration-registry'; import { GitHubProvider } from '../providers/github/github.provider'; import { IntegrationDefinition, IntegrationProvider, + ProviderApiError, TokenExpiredError, TokenInvalidError, } from '../registry/integration-provider.interface'; const GHE_BASE = 'https://github.acme.com'; const CONNECTED_AT = new Date('2026-10-01T09:00:00Z'); +const ORIGINAL_ENV = process.env; + +// A configured server always has the app credentials, since GitHub cannot be installed without them. +beforeEach(() => { + process.env = { + ...ORIGINAL_ENV, + INTEGRATION_GITHUB_CLIENT_ID: 'app-id', + INTEGRATION_GITHUB_CLIENT_SECRET: 'app-secret', + }; +}); + +afterEach(() => { + process.env = ORIGINAL_ENV; +}); class WorkspaceScopedProvider extends IntegrationProvider { definition: IntegrationDefinition = { @@ -102,7 +118,7 @@ describe('UnfurlService provider resolution', () => { title: 'PR', }); expect(unfurlPullRequest).toHaveBeenCalledWith( - 'token', apiBaseUrl, 'o', 'r', 1, url, + 'Bearer token', apiBaseUrl, 'o', 'r', 1, url, ); }, ); @@ -216,16 +232,42 @@ describe('UnfurlService cache', () => { }); it('asks a disconnected member to connect instead of serving their cached card', async () => { - const { service, connectionRepo, redis } = build(); - redis.get.mockResolvedValue(cachedCard); + const { service, unfurlPullRequest, connectionRepo, redis } = build(); + redis.get.mockImplementation(async (key: string) => + key.startsWith('unfurl:w1:') ? cachedCard : null, + ); connectionRepo.findByIntegrationAndUser.mockResolvedValue(undefined); + unfurlPullRequest.mockRejectedValue( + new ProviderApiError('GitHub', 404, 'Not Found'), + ); await expect(service.unfurl(url, 'u1', 'w1')).resolves.toMatchObject({ needsConnection: true, }); - expect(redis.get).not.toHaveBeenCalled(); - expect(redis.set).not.toHaveBeenCalled(); + expect(redis.get).not.toHaveBeenCalledWith( + expect.stringMatching(/^unfurl:w1:/), + ); + }); + + it('checks a link the member cannot see again a minute later', async () => { + const { service, unfurlPullRequest, redis } = build(); + unfurlPullRequest.mockRejectedValue( + new ProviderApiError('GitHub', 404, 'Not Found'), + ); + const error = jest + .spyOn(Logger.prototype, 'error') + .mockImplementation(() => undefined); + + await expect(service.unfurl(url, 'u1', 'w1')).resolves.toBeNull(); + + expect(redis.set).toHaveBeenCalledWith( + `unfurl:w1:u1:c1:${CONNECTED_AT.getTime()}:${urlDigest}`, + 'null', + 'EX', + 60, + ); + error.mockRestore(); }); it('serves no cached card once the integration is uninstalled', async () => { @@ -322,3 +364,68 @@ describe('UnfurlService cache', () => { }); }); }); + +describe('UnfurlService public cards', () => { + const url = 'https://github.com/o/r/pull/1'; + let warn: jest.SpyInstance; + + beforeEach(() => { + delete process.env.INTEGRATION_GITHUB_URL; + warn = jest + .spyOn(Logger.prototype, 'warn') + .mockImplementation(() => undefined); + }); + + afterEach(() => warn.mockRestore()); + + function buildDisconnected() { + const built = build(); + built.connectionRepo.findByIntegrationAndUser.mockResolvedValue(undefined); + return built; + } + + it('shows a member without a connection the card for a public repo', async () => { + const { service, unfurlPullRequest } = buildDisconnected(); + + await expect(service.unfurl(url, 'u1', 'w1')).resolves.toEqual({ + title: 'PR', + }); + // base64 of "app-id:app-secret" + expect(unfurlPullRequest).toHaveBeenCalledWith( + 'Basic YXBwLWlkOmFwcC1zZWNyZXQ=', 'https://api.github.com', 'o', 'r', 1, url, + ); + }); + + it('fetches a public card once for every member and workspace', async () => { + const { service, unfurlPullRequest, redis } = buildDisconnected(); + const store = new Map(); + redis.get.mockImplementation(async (key: string) => store.get(key) ?? null); + redis.set.mockImplementation(async (key: string, value: string) => { + store.set(key, value); + return 'OK'; + }); + + await service.unfurl(url, 'u1', 'w1'); + await expect(service.unfurl(url, 'u2', 'w2')).resolves.toEqual({ + title: 'PR', + }); + + expect(unfurlPullRequest).toHaveBeenCalledTimes(1); + }); + + it.each([ + ['a repo that is not public', new ProviderApiError('GitHub', 404, 'Not Found'), 0], + ['a rate limit', new ProviderApiError('GitHub', 403, 'rate limit exceeded'), 1], + ['rejected app credentials', new TokenInvalidError(), 1], + ])('asks to connect after %s and retries a minute later', async (_, error, warnings) => { + const { service, unfurlPullRequest, redis } = buildDisconnected(); + unfurlPullRequest.mockRejectedValue(error); + + await expect(service.unfurl(url, 'u1', 'w1')).resolves.toMatchObject({ + needsConnection: true, + }); + + expect(redis.set).toHaveBeenCalledWith(expect.any(String), 'null', 'EX', 60); + expect(warn).toHaveBeenCalledTimes(warnings); + }); +}); diff --git a/apps/server/src/core/integration/unfurl/unfurl.service.ts b/apps/server/src/core/integration/unfurl/unfurl.service.ts index 98071c35..8d190137 100644 --- a/apps/server/src/core/integration/unfurl/unfurl.service.ts +++ b/apps/server/src/core/integration/unfurl/unfurl.service.ts @@ -19,7 +19,7 @@ import type { Redis } from 'ioredis'; import * as crypto from 'crypto'; const UNFURL_CACHE_TTL = 300; // 5 minutes -// Short negative cache for transient failures; a 404 uses the normal TTL. +// Short negative cache, so a card shows up soon after access changes. const UNFURL_ERROR_CACHE_TTL = 60; const UNFURL_CACHE_PREFIX = 'unfurl:'; @@ -71,6 +71,15 @@ export class UnfurlService { if (connectionScope === 'workspace') { return null; } + const publicResult = await this.unfurlPublic( + provider, + patternType, + match, + url, + ); + if (publicResult) { + return publicResult; + } // Not cached, so the card loads once the user connects. return this.buildNeedsConnection( provider, @@ -152,11 +161,7 @@ export class UnfurlService { ); } this.logger.error(`Unfurl failed for ${url}: ${(err as Error).message}`); - const ttl = - err instanceof ProviderApiError && err.status === 404 - ? UNFURL_CACHE_TTL - : UNFURL_ERROR_CACHE_TTL; - await this.cacheNull(cacheKey, ttl); + await this.cacheNull(cacheKey, UNFURL_ERROR_CACHE_TTL); return null; } } @@ -165,6 +170,50 @@ export class UnfurlService { await this.redis.set(cacheKey, 'null', 'EX', ttl); } + // One cached card per URL for every member and workspace. + private async unfurlPublic( + provider: IntegrationProvider, + patternType: string, + match: RegExpMatchArray, + url: string, + ): Promise { + if (!provider.unfurlPublic) { + return null; + } + + const hash = crypto.createHash('sha256').update(url).digest('hex'); + const cacheKey = `${UNFURL_CACHE_PREFIX}public:${hash}`; + const cached = await this.redis.get(cacheKey); + if (cached) { + return JSON.parse(cached); + } + + try { + const unfurlResult = await provider.unfurlPublic({ + url, + match, + patternType, + }); + await this.redis.set( + cacheKey, + JSON.stringify(unfurlResult), + 'EX', + UNFURL_CACHE_TTL, + ); + return unfurlResult; + } catch (err) { + // A 404 is the normal answer for a private repo. + const notFound = err instanceof ProviderApiError && err.status === 404; + if (!notFound) { + this.logger.warn( + `Public unfurl failed for ${url}: ${(err as Error).message}`, + ); + } + await this.cacheNull(cacheKey, UNFURL_ERROR_CACHE_TTL); + return null; + } + } + private buildNeedsConnection( provider: IntegrationProvider, integrationId: string,