diff --git a/.changeset/calm-pages-suggest.md b/.changeset/calm-pages-suggest.md new file mode 100644 index 00000000000..eb319af2229 --- /dev/null +++ b/.changeset/calm-pages-suggest.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Add a reusable, action-backed executable suggestion lifecycle for reviewable resources. diff --git a/.changeset/fix-collab-reconcile-peer-deadline.md b/.changeset/fix-collab-reconcile-peer-deadline.md new file mode 100644 index 00000000000..a4e36a9dfef --- /dev/null +++ b/.changeset/fix-collab-reconcile-peer-deadline.md @@ -0,0 +1,11 @@ +--- +"@agent-native/toolkit": patch +--- + +Keep collaborative editors from briefly reverting edits received from another editor while SQL catches up, or indefinitely postponing accepted external content during presence updates. + +Advance the edit baseline when peer-delivered content already matches an accepted snapshot, preventing false conflicts on subsequent edits. + +Preserve subsequent local edits when an accepted replacement arrives through live sync before its saved revision, without treating identical shared changes as conflicts. + +Receive collab-backed canonical revisions through fresh Yjs sync receipts instead of inserting the same accepted text again from SQL. Preserve local edits and the confirmed merge base across delayed or failed delivery. diff --git a/.changeset/fix-controlled-editor-toolbar-rollback.md b/.changeset/fix-controlled-editor-toolbar-rollback.md new file mode 100644 index 00000000000..c2561a38eac --- /dev/null +++ b/.changeset/fix-controlled-editor-toolbar-rollback.md @@ -0,0 +1,5 @@ +--- +"@agent-native/toolkit": patch +--- + +Keep local rich-text changes from being rolled back while a controlled editor toolbar returns focus to the document. diff --git a/.changeset/fix-pglite-transaction-routing.md b/.changeset/fix-pglite-transaction-routing.md new file mode 100644 index 00000000000..72a88f70774 --- /dev/null +++ b/.changeset/fix-pglite-transaction-routing.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Keep transaction-scoped framework and app database reads on the active local PGlite transaction so review actions cannot stall the database. diff --git a/.changeset/fresh-collab-sync-receipts.md b/.changeset/fresh-collab-sync-receipts.md new file mode 100644 index 00000000000..f191714a3d0 --- /dev/null +++ b/.changeset/fresh-collab-sync-receipts.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Expose explicit collaborative document sync receipts for fresh server catch-up requests. diff --git a/.changeset/pending-suggestion-amendments.md b/.changeset/pending-suggestion-amendments.md new file mode 100644 index 00000000000..192f9c2a019 --- /dev/null +++ b/.changeset/pending-suggestion-amendments.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Allow authors to amend pending suggestions while preserving discussion and durable revision history, with revision checks protecting concurrent review decisions. diff --git a/.changeset/review-discussion-tools.md b/.changeset/review-discussion-tools.md new file mode 100644 index 00000000000..93b785e8f23 --- /dev/null +++ b/.changeset/review-discussion-tools.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Expose review reactions and personal thread preferences through the shared read/action hooks, preserve independently updated preferences, and honor muted threads when delivering reply notifications. diff --git a/.changeset/steady-suggestion-retries.md b/.changeset/steady-suggestion-retries.md new file mode 100644 index 00000000000..d0134e9e7d0 --- /dev/null +++ b/.changeset/steady-suggestion-retries.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Make suggestion creation and decision retries converge safely, and batch suggestion history reads. diff --git a/.changeset/stop-agent-chat-hmr-sweeps.md b/.changeset/stop-agent-chat-hmr-sweeps.md new file mode 100644 index 00000000000..3ed87978453 --- /dev/null +++ b/.changeset/stop-agent-chat-hmr-sweeps.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Stop retired development server instances from retaining agent sweep timers and MCP settings listeners after hot reloads. diff --git a/.changeset/tidy-editor-ssr-stubs.md b/.changeset/tidy-editor-ssr-stubs.md new file mode 100644 index 00000000000..c4bc4ad1a17 --- /dev/null +++ b/.changeset/tidy-editor-ssr-stubs.md @@ -0,0 +1,5 @@ +--- +"@agent-native/core": patch +--- + +Include editor transform and schema exports in browser-only SSR stubs so serverless Content builds succeed. diff --git a/packages/core/package.json b/packages/core/package.json index 33f06597d32..8b8184ccb7d 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -253,6 +253,14 @@ "./review/actions/get-review-feedback": "./dist/review/actions/get-review-feedback.js", "./review/actions/set-review-status": "./dist/review/actions/set-review-status.js", "./review/actions/send-review-thread-to-agent": "./dist/review/actions/send-review-thread-to-agent.js", + "./review/actions/react-to-review-comment": "./dist/review/actions/react-to-review-comment.js", + "./review/actions/set-review-thread-unread": "./dist/review/actions/set-review-thread-unread.js", + "./review/actions/set-review-thread-muted": "./dist/review/actions/set-review-thread-muted.js", + "./review/suggestions/actions/create-resource-suggestion": "./dist/review/suggestions/actions/create-resource-suggestion.js", + "./review/suggestions/actions/update-resource-suggestion": "./dist/review/suggestions/actions/update-resource-suggestion.js", + "./review/suggestions/actions/list-resource-suggestions": "./dist/review/suggestions/actions/list-resource-suggestions.js", + "./review/suggestions/actions/get-resource-suggestion": "./dist/review/suggestions/actions/get-resource-suggestion.js", + "./review/suggestions/actions/decide-resource-suggestion": "./dist/review/suggestions/actions/decide-resource-suggestion.js", "./comments-review/actions/list-review-comments": "./dist/review/actions/list-review-comments.js", "./comments-review/actions/create-review-comment": "./dist/review/actions/create-review-comment.js", "./comments-review/actions/reply-review-comment": "./dist/review/actions/reply-review-comment.js", diff --git a/packages/core/src/client/collab/index.ts b/packages/core/src/client/collab/index.ts index d49c9d00799..d281c0f29e0 100644 --- a/packages/core/src/client/collab/index.ts +++ b/packages/core/src/client/collab/index.ts @@ -9,6 +9,7 @@ export { type UseCollaborativeDocResult, type CollabInitializationErrorCategory, type CollabInitializationState, + type CollaborativeDocSyncResult, } from "../../collab/client.js"; export { AGENT_CLIENT_ID } from "../../collab/agent-identity.js"; export { diff --git a/packages/core/src/client/index.ts b/packages/core/src/client/index.ts index 8972b427947..202d3fc2981 100644 --- a/packages/core/src/client/index.ts +++ b/packages/core/src/client/index.ts @@ -155,6 +155,7 @@ export { type UseCollaborativeDocResult, type CollabInitializationErrorCategory, type CollabInitializationState, + type CollaborativeDocSyncResult, type CollabUser, } from "../collab/client.js"; export { AGENT_CLIENT_ID } from "../collab/agent-identity.js"; @@ -224,6 +225,9 @@ export { useCreateReviewComment, useDeleteReviewComment, useReplyReviewComment, + useReactToReviewComment, + useSetReviewThreadUnread, + useSetReviewThreadMuted, useResolveReviewThread, useReviewComments, useReviewFeedback, @@ -237,6 +241,9 @@ export { type ListReviewCommentsParams, type ListReviewCommentsResult, type ReplyReviewCommentInput, + type ReactToReviewCommentInput, + type SetReviewThreadUnreadInput, + type SetReviewThreadMutedInput, type ResolveReviewThreadInput, type ReviewStatusBadgeProps, type ReviewCommentComposerProps, diff --git a/packages/core/src/client/review/index.ts b/packages/core/src/client/review/index.ts index 152ef3b58f7..3d726f2d891 100644 --- a/packages/core/src/client/review/index.ts +++ b/packages/core/src/client/review/index.ts @@ -19,11 +19,18 @@ export { useCreateReviewComment, useDeleteReviewComment, useReplyReviewComment, + useReactToReviewComment, + useSetReviewThreadUnread, + useSetReviewThreadMuted, useResolveReviewThread, useReviewComments, useReviewFeedback, useSetReviewStatus, useSendReviewThreadToAgent, + useResourceSuggestions, + useCreateResourceSuggestion, + useUpdateResourceSuggestion, + useDecideResourceSuggestion, type ConsumeReviewFeedbackInput, type CreateReviewCommentInput, type DeleteReviewCommentInput, @@ -32,7 +39,14 @@ export { type ListReviewCommentsParams, type ListReviewCommentsResult, type ReplyReviewCommentInput, + type ReactToReviewCommentInput, + type SetReviewThreadUnreadInput, + type SetReviewThreadMutedInput, type ResolveReviewThreadInput, type SetReviewStatusInput, type SendReviewThreadToAgentInput, + type ListResourceSuggestionsParams, + type CreateResourceSuggestionInput, + type UpdateResourceSuggestionInput, + type DecideResourceSuggestionInput, } from "./use-review.js"; diff --git a/packages/core/src/client/review/use-review.ts b/packages/core/src/client/review/use-review.ts index 50f534da5a4..bd6d5d13c90 100644 --- a/packages/core/src/client/review/use-review.ts +++ b/packages/core/src/client/review/use-review.ts @@ -1,6 +1,16 @@ +import { useQueryClient } from "@tanstack/react-query"; + +import type { + ResourceSuggestion, + SuggestionDecision, + SuggestionOperation, + SuggestionStatus, +} from "../../review/suggestions/types.js"; import type { ReviewComment, ReviewCommentKind, + ReviewDiscussionState, + ReviewThreadPreference, ReviewMention, ReviewResolutionTarget, ReviewStatus, @@ -19,6 +29,7 @@ export interface ListReviewCommentsParams { export interface ListReviewCommentsResult { comments: ReviewComment[]; + discussion: ReviewDiscussionState; reviewStatus: ReviewStatusEntry | null; summary: { openCount: number; @@ -61,6 +72,75 @@ export interface ReplyReviewCommentInput { metadata?: Record; } +export interface ReactToReviewCommentInput { + resourceType: string; + resourceId: string; + commentId: string; + reaction: string; + active: boolean; +} + +export interface SetReviewThreadUnreadInput { + resourceType: string; + resourceId: string; + threadId: string; + unread: boolean; +} + +export interface SetReviewThreadMutedInput { + resourceType: string; + resourceId: string; + threadId: string; + muted: boolean; +} + +export function useReactToReviewComment() { + const queryClient = useQueryClient(); + return useActionMutation< + { + commentId: string; + actorEmail: string; + reaction: string; + active: boolean; + }, + ReactToReviewCommentInput + >("react-to-review-comment", { + skipActionQueryInvalidation: true, + onSuccess: () => + queryClient.invalidateQueries({ + queryKey: ["action", "list-review-comments"], + }), + }); +} + +export function useSetReviewThreadUnread() { + const queryClient = useQueryClient(); + return useActionMutation< + ReviewThreadPreference & { threadId: string }, + SetReviewThreadUnreadInput + >("set-review-thread-unread", { + skipActionQueryInvalidation: true, + onSuccess: () => + queryClient.invalidateQueries({ + queryKey: ["action", "list-review-comments"], + }), + }); +} + +export function useSetReviewThreadMuted() { + const queryClient = useQueryClient(); + return useActionMutation< + ReviewThreadPreference & { threadId: string }, + SetReviewThreadMutedInput + >("set-review-thread-muted", { + skipActionQueryInvalidation: true, + onSuccess: () => + queryClient.invalidateQueries({ + queryKey: ["action", "list-review-comments"], + }), + }); +} + export interface ResolveReviewThreadInput { resourceType: string; resourceId: string; @@ -95,6 +175,39 @@ export interface SetReviewStatusInput { metadata?: Record; } +export interface ListResourceSuggestionsParams { + resourceType: string; + resourceId: string; + statuses?: SuggestionStatus[]; +} + +export interface CreateResourceSuggestionInput { + resourceType: string; + resourceId: string; + adapterKind: string; + baseRevision: string; + summary: string; + idempotencyKey: string; + operations: SuggestionOperation[]; + metadata?: Record; +} + +export interface DecideResourceSuggestionInput { + id: string; + decision: SuggestionDecision; + idempotencyKey: string; + observedBase: string; + observedRevision?: number; +} + +export interface UpdateResourceSuggestionInput { + id: string; + observedRevision: number; + idempotencyKey: string; + operations: SuggestionOperation[]; + summary?: string; +} + export function useReviewComments( params: ListReviewCommentsParams, options?: { enabled?: boolean }, @@ -184,3 +297,36 @@ export function useSetReviewStatus() { "set-review-status", ); } + +export function useResourceSuggestions( + params: ListResourceSuggestionsParams, + options?: { enabled?: boolean }, +) { + return useActionQuery<{ suggestions: ResourceSuggestion[] }>( + "list-resource-suggestions", + params, + { + enabled: + options?.enabled ?? Boolean(params.resourceType && params.resourceId), + }, + ); +} + +export function useCreateResourceSuggestion() { + return useActionMutation( + "create-resource-suggestion", + ); +} + +export function useDecideResourceSuggestion() { + return useActionMutation< + { suggestion: ResourceSuggestion; decision: unknown }, + DecideResourceSuggestionInput + >("decide-resource-suggestion"); +} + +export function useUpdateResourceSuggestion() { + return useActionMutation( + "update-resource-suggestion", + ); +} diff --git a/packages/core/src/collab/client.registry.spec.tsx b/packages/core/src/collab/client.registry.spec.tsx index b73a2c2d5c1..511d14c0285 100644 --- a/packages/core/src/collab/client.registry.spec.tsx +++ b/packages/core/src/collab/client.registry.spec.tsx @@ -56,6 +56,19 @@ function emptyStateResponse(): Response { ); } +function deferredResponse(): { + promise: Promise; + resolve: (response: Response) => void; +} { + let resolve!: (response: Response) => void; + return { + promise: new Promise((done) => { + resolve = done; + }), + resolve, + }; +} + /** Routes collab/poll endpoints to canned JSON and counts state fetches. */ function makeFetchMock() { const stateFetches: string[] = []; @@ -158,6 +171,133 @@ describe("useCollaborativeDoc connection registry", () => { expect(b?.isSynced).toBe(true); }); + it("returns a fresh sync receipt after an older transport fetch completes", async () => { + const backgroundState = deferredResponse(); + const requestedState = deferredResponse(); + let stateVectorFetches = 0; + const stateVectorRequests: RequestInit[] = []; + const mock = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + if (/\/collab\/[^/]+\/state\?/.test(url)) { + stateVectorFetches++; + stateVectorRequests.push(init ?? {}); + return stateVectorFetches === 1 + ? backgroundState.promise + : requestedState.promise; + } + if (/\/collab\/[^/]+\/state$/.test(url)) return emptyStateResponse(); + if (url.includes("/_agent-native/poll")) { + // Force the transport's ring-gap recovery path to have an older + // state-vector request in flight when requestSync is called. + return new Response(JSON.stringify({ version: 2_000, events: [] })); + } + return new Response(JSON.stringify({ states: [] })); + }); + vi.stubGlobal("fetch", mock); + + let result: UseCollaborativeDocResult | undefined; + const root = mount( + (result = next)} />, + ); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(result?.initialization.status).toBe("ready"); + expect(stateVectorFetches).toBe(1); + const requestSync = result!.requestSync; + let receipt!: ReturnType; + act(() => { + receipt = result!.requestSync(); + }); + // The receipt starts a fresh request immediately instead of waiting for + // the older transport recovery, which may never settle. + expect(stateVectorFetches).toBe(2); + expect(stateVectorRequests[1]?.cache).toBe("no-store"); + + act(() => { + root.render( + (result = next)} />, + ); + }); + expect(result?.requestSync).toBe(requestSync); + + let outcome: Awaited | undefined; + await act(async () => { + requestedState.resolve(emptyStateResponse()); + outcome = await receipt; + }); + expect(outcome).toEqual({ status: "synced" }); + + await act(async () => { + backgroundState.resolve(emptyStateResponse()); + await Promise.resolve(); + }); + }); + + it("reports state-vector failure and can retry without a false sync ack", async () => { + let stateVectorFetches = 0; + const mock = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + if (/\/collab\/[^/]+\/state\?/.test(url)) { + stateVectorFetches++; + if (stateVectorFetches === 1) { + return new Response("nope", { status: 503 }); + } + if (stateVectorFetches === 2) { + return new Response(JSON.stringify({ state: "AQ==" })); + } + if (stateVectorFetches === 3) { + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener("abort", () => { + reject(new DOMException("Timed out", "AbortError")); + }); + }); + } + return emptyStateResponse(); + } + if (/\/collab\/[^/]+\/state$/.test(url)) return emptyStateResponse(); + if (url.includes("/_agent-native/poll")) { + return new Response(JSON.stringify({ version: 1, events: [] })); + } + return new Response(JSON.stringify({ states: [] })); + }); + vi.stubGlobal("fetch", mock); + + let result: UseCollaborativeDocResult | undefined; + const root = mount( + (result = next)} />, + ); + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + + const failed = await result!.requestSync(); + expect(failed.status).toBe("failed"); + if (failed.status === "failed") { + expect(failed.error.message).toContain("HTTP 503"); + } + const malformed = await result!.requestSync(); + expect(malformed.status).toBe("failed"); + const timedOutReceipt = result!.requestSync(); + await act(async () => { + await vi.advanceTimersByTimeAsync(15_000); + }); + const timedOut = await timedOutReceipt; + expect(timedOut.status).toBe("failed"); + await expect(result!.requestSync()).resolves.toEqual({ status: "synced" }); + + const staleRequestSync = result!.requestSync; + act(() => root.unmount()); + roots = roots.filter((candidate) => candidate !== root); + await expect(staleRequestSync()).resolves.toEqual({ + status: "unavailable", + }); + expect(stateVectorFetches).toBe(4); + }); + it.each([ [ "403", diff --git a/packages/core/src/collab/client.ts b/packages/core/src/collab/client.ts index cab1864f6e5..1de2e63b468 100644 --- a/packages/core/src/collab/client.ts +++ b/packages/core/src/collab/client.ts @@ -83,6 +83,11 @@ export type CollabInitializationState = | { status: "ready" } | { status: "error"; category: CollabInitializationErrorCategory }; +export type CollaborativeDocSyncResult = + | { status: "synced" } + | { status: "failed"; error: Error } + | { status: "unavailable" }; + export interface UseCollaborativeDocResult { /** The Yjs document instance. Stable per docId — never changes identity. */ ydoc: Y.Doc | null; @@ -96,6 +101,11 @@ export interface UseCollaborativeDocResult { initialization: CollabInitializationState; /** Retry a failed initial-state read. No transport starts until it succeeds. */ retry: () => void; + /** + * Request a fresh catch-up with canonical server state. Resolves as synced + * only after the response has been applied to this active document. + */ + requestSync: () => Promise; /** Active users on this document (from awareness). */ activeUsers: CollabUser[]; /** True briefly when the AI agent makes an edit (for presence indicator). */ @@ -198,6 +208,9 @@ const UPDATE_DEBOUNCE_MS = 80; /** Fetch state-vector every N poll cycles as a low-frequency safety net. */ const STATE_VECTOR_FETCH_INTERVAL = 15; +/** Bound state-vector recovery so a hung request cannot block fresh receipts. */ +const STATE_VECTOR_FETCH_TIMEOUT_MS = 15_000; + /** Poll ring-buffer size on the server (MAX_BUFFER in poll.ts). */ const POLL_RING_BUFFER_SIZE = 200; @@ -330,6 +343,9 @@ const EMPTY_SNAPSHOT: CollabDocSnapshot = Object.freeze({ agentPresent: false, }); +const requestSyncUnavailable = (): Promise => + Promise.resolve({ status: "unavailable" }); + /** * How long a connection with zero subscribers lingers before disposal. * Long enough to absorb StrictMode double-mounts and route-level remounts @@ -365,6 +381,8 @@ class CollabDocConnection { private pollCycleCount = 0; private pollVersion = 0; private lastPolledVersion = 0; + private stateVectorFetch: Promise | null = null; + private stateVectorAbortControllers = new Set(); private sseActive = false; // Whether the active SSE stream actually forwards awareness. The hosted // Realtime Gateway advertises `no-awareness` (it can't see the in-process @@ -470,6 +488,10 @@ class CollabDocConnection { dispose(): void { if (this.disposed) return; this.disposed = true; + for (const controller of this.stateVectorAbortControllers) { + controller.abort(); + } + this.stateVectorAbortControllers.clear(); if (this.disposeTimer) { clearTimeout(this.disposeTimer); this.disposeTimer = null; @@ -775,6 +797,20 @@ class CollabDocConnection { this.retryInitialization(); }; + requestSync = (): Promise => { + if ( + this.disposed || + this.subscribers.size === 0 || + this.snapshot.initialization.status !== "ready" + ) { + return Promise.resolve({ status: "unavailable" }); + } + + // Do not join a transport recovery already in flight: it may have started + // before the durable revision whose receipt the caller is requesting. + return this.performStateVectorFetch(); + }; + // ------------------------------------------------------------------------- // Local update batching // ------------------------------------------------------------------------- @@ -979,26 +1015,86 @@ class CollabDocConnection { this.schedulePoll(); } - private async fetchStateVector(): Promise { + private fetchStateVector(): Promise { + if (this.stateVectorFetch) return this.stateVectorFetch; + if (this.disposed || this.snapshot.initialization.status !== "ready") { + return Promise.resolve({ status: "unavailable" }); + } + + return this.startStateVectorFetch(); + } + + private startStateVectorFetch(): Promise { + const request = this.performStateVectorFetch(); + this.stateVectorFetch = request; + void request.finally(() => { + if (this.stateVectorFetch === request) this.stateVectorFetch = null; + }); + return request; + } + + private async performStateVectorFetch(): Promise { + const controller = new AbortController(); + this.stateVectorAbortControllers.add(controller); + const timeout = setTimeout( + () => controller.abort(), + STATE_VECTOR_FETCH_TIMEOUT_MS, + ); try { const stateVector = uint8ArrayToBase64(Y.encodeStateVector(this.ydoc)); const stateRes = await fetch( `${this.baseUrl}/${this.docId}/state?stateVector=${encodeURIComponent(stateVector)}`, + { cache: "no-store", signal: controller.signal }, ); - if (stateRes.ok) { - const stateData = (await stateRes.json().catch(() => null)) as { - state?: string; - } | null; - if (this.disposed) return; - if (stateData?.state) { - const binary = base64ToUint8Array(stateData.state); - if (binary.length > 2) { - Y.applyUpdate(this.ydoc, binary, "remote"); - } - } + if (!stateRes.ok) { + return { + status: "failed", + error: new Error( + `State-vector request failed: HTTP ${stateRes.status}`, + ), + }; } - } catch { + const stateData = (await stateRes.json()) as { + state?: string; + }; + if ( + this.disposed || + this.subscribers.size === 0 || + this.snapshot.initialization.status !== "ready" + ) { + return { status: "unavailable" }; + } + if ( + typeof stateData?.state !== "string" || + stateData.state.length === 0 + ) { + return { + status: "failed", + error: new Error("State-vector response did not contain state"), + }; + } + const binary = base64ToUint8Array(stateData.state); + Y.applyUpdate(this.ydoc, binary, "remote"); + return { status: "synced" }; + } catch (error) { // Non-fatal; the next poll cycle will retry + if ( + this.disposed || + this.subscribers.size === 0 || + this.snapshot.initialization.status !== "ready" + ) { + return { status: "unavailable" }; + } + return { + status: "failed", + error: + error instanceof Error + ? error + : new Error("State-vector request failed"), + }; + } finally { + clearTimeout(timeout); + this.stateVectorAbortControllers.delete(controller); } } @@ -1362,6 +1458,7 @@ export function useCollaborativeDoc( conn.retry(); } : () => {}, + requestSync: conn ? conn.requestSync : requestSyncUnavailable, activeUsers: snapshot.activeUsers, agentActive: snapshot.agentActive, agentPresent: snapshot.agentPresent, diff --git a/packages/core/src/collab/index.ts b/packages/core/src/collab/index.ts index 5b60874d3aa..be2e6e855f9 100644 --- a/packages/core/src/collab/index.ts +++ b/packages/core/src/collab/index.ts @@ -14,6 +14,7 @@ export { export { CollabBaseVersionConflictError, getDoc, + withPreparedYDocMutation, applyUpdate, applyText, getText, @@ -26,6 +27,7 @@ export { applyPatchOps, getJson, seedFromJson, + type PreparedYDocMutationLease, } from "./ydoc-manager.js"; // XmlFragment operations diff --git a/packages/core/src/collab/routes.payload.spec.ts b/packages/core/src/collab/routes.payload.spec.ts index ec83790d347..4e66a6ca069 100644 --- a/packages/core/src/collab/routes.payload.spec.ts +++ b/packages/core/src/collab/routes.payload.spec.ts @@ -15,6 +15,9 @@ vi.mock("h3", () => ({ setResponseStatus: (event: any, status: number) => { event._status = status; }, + setResponseHeader: (event: any, name: string, value: string) => { + (event._headers ??= {})[name] = value; + }, getQuery: (event: any) => event._query ?? {}, })); @@ -30,6 +33,8 @@ vi.mock("./ydoc-manager.js", () => ({ applyJson: vi.fn(), applyPatchOps: vi.fn(), getJson: vi.fn().mockResolvedValue(null), + getState: vi.fn().mockResolvedValue(new Uint8Array([0, 0])), + getIncUpdate: vi.fn().mockResolvedValue(new Uint8Array([0, 0])), })); vi.mock("./storage.js", () => ({ @@ -38,6 +43,7 @@ vi.mock("./storage.js", () => ({ })); import { + getCollabState, postCollabUpdate, postCollabText, postCollabSearchReplace, @@ -57,6 +63,28 @@ function event(params: Record, maxPayloadBytes?: number): any { const DEFAULT_MAX_BYTES = 2 * 1024 * 1024; // 2 MB +describe("getCollabState cache policy", () => { + it.each([{}, { stateVector: "AAA=" }])( + "does not cache a state response (%j)", + async (query) => { + const ev = event({ docId: "doc-1" }); + ev._query = query; + expect(await getCollabState(ev)).toEqual({ + docId: "doc-1", + state: "AAA=", + }); + expect(ev._headers["Cache-Control"]).toBe("private, no-store"); + }, + ); + + it("does not cache validation errors", async () => { + const ev = event({}); + expect(await getCollabState(ev)).toEqual({ error: "docId required" }); + expect(ev._status).toBe(400); + expect(ev._headers["Cache-Control"]).toBe("private, no-store"); + }); +}); + // Generates a string of `len` bytes. function bigString(len: number): string { return "x".repeat(len); diff --git a/packages/core/src/collab/routes.ts b/packages/core/src/collab/routes.ts index 2a69dd76c1f..8cdbf37010a 100644 --- a/packages/core/src/collab/routes.ts +++ b/packages/core/src/collab/routes.ts @@ -7,6 +7,7 @@ import { defineEventHandler, setResponseStatus, + setResponseHeader, getRouterParam, getQuery, } from "h3"; @@ -44,6 +45,7 @@ function enforcePayloadLimit(event: H3Event, body: unknown): boolean { * Returns full Yjs document state as base64 for initial client load. */ export const getCollabState = defineEventHandler(async (event: H3Event) => { + setResponseHeader(event, "Cache-Control", "private, no-store"); const docId = getRouterParam(event, "docId"); if (!docId) { setResponseStatus(event, 400); diff --git a/packages/core/src/collab/storage.spec.ts b/packages/core/src/collab/storage.spec.ts index 5fc795ef987..53e44f614c6 100644 --- a/packages/core/src/collab/storage.spec.ts +++ b/packages/core/src/collab/storage.spec.ts @@ -1,10 +1,13 @@ import { afterEach, describe, expect, it, vi } from "vitest"; +import { getDbExec } from "../db/client.js"; import { listCollabDocIds, loadYDocRecord, + loadYDocRecordWithClient, saveYDocState, trySaveYDocState, + trySaveYDocStateWithClient, } from "./storage.js"; const rows = vi.hoisted( @@ -20,7 +23,7 @@ function toBase64(arr: Uint8Array): string { } vi.mock("../db/client.js", () => ({ - getDbExec: () => ({ + getDbExec: vi.fn(() => ({ execute: async (query: string | { sql: string; args?: unknown[] }) => { const sql = typeof query === "string" ? query : query.sql; const args = typeof query === "string" ? [] : (query.args ?? []); @@ -70,7 +73,7 @@ vi.mock("../db/client.js", () => ({ throw new Error(`Unexpected SQL: ${sql}`); }, - }), + })), })); vi.mock("../db/ddl-guard.js", () => ({ @@ -117,4 +120,42 @@ describe("collab storage optimistic saves", () => { expect(await listCollabDocIds()).toEqual(new Set(["doc-1", "doc-2"])); }); + + it("uses an injected client for reads and stale CAS writes", async () => { + vi.mocked(getDbExec).mockClear(); + const injectedClient = { + execute: vi.fn( + async (query: string | { sql: string; args?: unknown[] }) => { + const sql = typeof query === "string" ? query : query.sql; + const args = typeof query === "string" ? [] : (query.args ?? []); + if (/^\s*SELECT yjs_state, version FROM _collab_docs/i.test(sql)) { + return { + rows: [{ yjs_state: toBase64(new Uint8Array([1])), version: 2 }], + rowsAffected: 0, + }; + } + if (/^\s*UPDATE _collab_docs\b/i.test(sql)) { + return { rows: [], rowsAffected: 0 }; + } + throw new Error(`Unexpected SQL: ${sql}`); + }, + ), + }; + + expect(await loadYDocRecordWithClient(injectedClient, "doc-1")).toEqual({ + state: new Uint8Array([1]), + version: 2, + }); + expect( + await trySaveYDocStateWithClient( + injectedClient, + "doc-1", + new Uint8Array([2]), + "two", + 1, + ), + ).toBe(false); + expect(injectedClient.execute).toHaveBeenCalledTimes(2); + expect(getDbExec).not.toHaveBeenCalled(); + }); }); diff --git a/packages/core/src/collab/storage.ts b/packages/core/src/collab/storage.ts index feedef32936..cec31522d31 100644 --- a/packages/core/src/collab/storage.ts +++ b/packages/core/src/collab/storage.ts @@ -5,7 +5,7 @@ * state. */ -import { getDbExec } from "../db/client.js"; +import { getDbExec, type DbExec } from "../db/client.js"; import { ensureTableExists, ensureColumnExists } from "../db/ddl-guard.js"; let _initPromise: Promise | undefined; @@ -49,7 +49,14 @@ export async function loadYDocRecord( docId: string, ): Promise { await ensureTable(); - const client = getDbExec(); + return loadYDocRecordWithClient(getDbExec(), docId); +} + +/** Load through a caller-owned transaction after schema readiness is ensured. */ +export async function loadYDocRecordWithClient( + client: DbExec, + docId: string, +): Promise { const { rows } = await client.execute({ sql: `SELECT yjs_state, version FROM _collab_docs WHERE doc_id = ?`, args: [docId], @@ -98,7 +105,23 @@ export async function trySaveYDocState( expectedVersion: number | null, ): Promise { await ensureTable(); - const client = getDbExec(); + return trySaveYDocStateWithClient( + getDbExec(), + docId, + state, + textSnapshot, + expectedVersion, + ); +} + +/** CAS-save through a caller-owned transaction after schema readiness. */ +export async function trySaveYDocStateWithClient( + client: DbExec, + docId: string, + state: Uint8Array, + textSnapshot: string, + expectedVersion: number | null, +): Promise { const b64 = uint8ArrayToBase64(state); const nowExpr = "NOW()::text"; if (expectedVersion === null) { diff --git a/packages/core/src/collab/ydoc-manager.spec.ts b/packages/core/src/collab/ydoc-manager.spec.ts index b7531ad2893..8cbf72c82a2 100644 --- a/packages/core/src/collab/ydoc-manager.spec.ts +++ b/packages/core/src/collab/ydoc-manager.spec.ts @@ -6,8 +6,11 @@ const storageMocks = vi.hoisted(() => ({ loadYDocVersion: vi.fn(), saveYDocState: vi.fn(), trySaveYDocState: vi.fn(), + trySaveYDocStateWithClient: vi.fn(), })); +const emitterMocks = vi.hoisted(() => ({ emitCollabUpdate: vi.fn() })); + vi.mock("./storage.js", () => ({ ...storageMocks, uint8ArrayToBase64: (value: Uint8Array) => @@ -15,7 +18,7 @@ vi.mock("./storage.js", () => ({ })); vi.mock("./emitter.js", () => ({ - emitCollabUpdate: vi.fn(), + emitCollabUpdate: emitterMocks.emitCollabUpdate, })); describe("ydoc-manager", () => { @@ -24,8 +27,10 @@ describe("ydoc-manager", () => { storageMocks.loadYDocRecord.mockReset(); storageMocks.saveYDocState.mockReset(); storageMocks.trySaveYDocState.mockReset(); + storageMocks.trySaveYDocStateWithClient.mockReset(); storageMocks.loadYDocState.mockReset(); storageMocks.loadYDocVersion.mockReset(); + emitterMocks.emitCollabUpdate.mockReset(); }); it("coalesces concurrent cache-miss loads for the same document", async () => { @@ -47,4 +52,55 @@ describe("ydoc-manager", () => { expect(first).toBe(second); expect(storageMocks.loadYDocRecord).toHaveBeenCalledTimes(1); }); + + it("publishes a prepared clone only after transactional persistence", async () => { + storageMocks.loadYDocRecord.mockResolvedValue(null); + storageMocks.loadYDocVersion.mockResolvedValue(null); + storageMocks.trySaveYDocStateWithClient.mockResolvedValue(true); + const tx = { execute: vi.fn() }; + const { getDoc, withPreparedYDocMutation } = + await import("./ydoc-manager.js"); + + await withPreparedYDocMutation("prepared-doc", "agent", async (lease) => { + lease.doc.getText("content").insert(0, "accepted"); + expect(emitterMocks.emitCollabUpdate).not.toHaveBeenCalled(); + await lease.persist(tx, "accepted"); + expect(emitterMocks.emitCollabUpdate).not.toHaveBeenCalled(); + }); + + expect(storageMocks.trySaveYDocStateWithClient).toHaveBeenCalledWith( + tx, + "prepared-doc", + expect.any(Uint8Array), + "accepted", + null, + ); + expect(emitterMocks.emitCollabUpdate).toHaveBeenCalledOnce(); + expect((await getDoc("prepared-doc")).getText("content").toString()).toBe( + "accepted", + ); + }); + + it("discards a prepared clone when the caller transaction rolls back", async () => { + storageMocks.loadYDocRecord.mockResolvedValue(null); + storageMocks.loadYDocVersion.mockResolvedValue(null); + storageMocks.trySaveYDocStateWithClient.mockResolvedValue(true); + const { getDoc, withPreparedYDocMutation } = + await import("./ydoc-manager.js"); + const current = await getDoc("rollback-doc"); + current.getText("content").insert(0, "before"); + + await expect( + withPreparedYDocMutation("rollback-doc", "agent", async (lease) => { + lease.doc.getText("content").insert(6, "-candidate"); + await lease.persist({ execute: vi.fn() }, "before-candidate"); + throw new Error("rollback"); + }), + ).rejects.toThrow("rollback"); + + expect(emitterMocks.emitCollabUpdate).not.toHaveBeenCalled(); + expect((await getDoc("rollback-doc")).getText("content").toString()).toBe( + "before", + ); + }); }); diff --git a/packages/core/src/collab/ydoc-manager.ts b/packages/core/src/collab/ydoc-manager.ts index a88c0b5047a..bb1c8d4f02b 100644 --- a/packages/core/src/collab/ydoc-manager.ts +++ b/packages/core/src/collab/ydoc-manager.ts @@ -21,6 +21,7 @@ import * as Y from "yjs"; +import type { DbExec } from "../db/client.js"; import { emitCollabUpdate } from "./emitter.js"; import { applyJsonDiff, @@ -35,6 +36,7 @@ import { loadYDocVersion, saveYDocState, trySaveYDocState, + trySaveYDocStateWithClient, } from "./storage.js"; import { uint8ArrayToBase64 } from "./storage.js"; import { applyTextToYDoc, initYDocWithText } from "./text-to-yjs.js"; @@ -138,6 +140,13 @@ const _refreshLocks = new Map>(); // doc (a memory leak that grows with concurrent read traffic). const _loadLocks = new Map>(); +export interface PreparedYDocMutationLease { + /** Isolated clone. Mutations stay invisible until the outer transaction commits. */ + doc: Y.Doc; + baseVersion: number | null; + persist(transaction: DbExec, textSnapshot: string): Promise; +} + function evictIfNeeded(): void { if (_cache.size <= MAX_CACHE) return; // Evict least-recently-accessed entry @@ -179,6 +188,75 @@ async function withDocWriteLock( } } +/** + * Serialize a caller-owned SQL transaction with Yjs writes. The callback + * mutates an isolated clone and persists it through its transaction. Only a + * successfully resolved callback replaces the shared cache and broadcasts. + */ +export async function withPreparedYDocMutation( + docId: string, + requestSource: string | undefined, + run: (lease: PreparedYDocMutationLease) => Promise, +): Promise { + return withDocWriteLock(docId, async () => { + const current = await getDocForWrite(docId); + const latest = await loadYDocRecord(docId); + if (latest?.state.length) Y.applyUpdate(current, latest.state); + + const candidate = new Y.Doc(); + Y.applyUpdate(candidate, Y.encodeStateAsUpdate(current)); + const baseVector = Y.encodeStateVector(candidate); + const baseVersion = latest?.version ?? null; + let persisted = false; + const lease: PreparedYDocMutationLease = { + doc: candidate, + baseVersion, + persist: async (transaction, textSnapshot) => { + if (persisted) { + throw new Error("Prepared Yjs mutation was already persisted"); + } + const saved = await trySaveYDocStateWithClient( + transaction, + docId, + Y.encodeStateAsUpdate(candidate), + textSnapshot, + baseVersion, + ); + if (!saved) { + throw new CollabBaseVersionConflictError( + `Document ${docId} changed while the prepared mutation was committing.`, + ); + } + persisted = true; + }, + }; + + try { + const result = await run(lease); + if (!persisted) { + candidate.destroy(); + return result; + } + const update = Y.encodeStateAsUpdate(candidate, baseVector); + const previous = _cache.get(docId); + previous?.doc.destroy(); + _cache.set(docId, { + doc: candidate, + lastAccess: Date.now(), + syncedVersion: baseVersion === null ? 0 : baseVersion + 1, + }); + evictIfNeeded(); + if (update.length > 0) { + emitCollabUpdate(docId, uint8ArrayToBase64(update), requestSource); + } + return result; + } catch (error) { + candidate.destroy(); + throw error; + } + }); +} + /** * Build state to persist. If the stored blob is significantly larger than * the freshly encoded state, store the compact (GC'd) form instead to diff --git a/packages/core/src/db/client.ts b/packages/core/src/db/client.ts index ff21a0f227c..7a120d5375c 100644 --- a/packages/core/src/db/client.ts +++ b/packages/core/src/db/client.ts @@ -7,6 +7,7 @@ import path from "path"; * expose PostgreSQL semantics to the rest of the framework. */ import { getAppConfig } from "../app-config/index.js"; +import { getAsyncLocalStorageCtor } from "../shared/optional-node-builtins.js"; import { isMigrationAuthorizedRuntime } from "./migration-runtime.js"; import { beginDatabaseOperation, @@ -56,6 +57,40 @@ export interface DbExecConfig { url?: string; } +type PgliteTransactionContext = { + client: any; + exec: DbExec; +}; + +type PgliteTransactionContexts = ReadonlyMap; + +type PgliteTransactionStorage = { + getStore(): PgliteTransactionContexts | undefined; + run(store: PgliteTransactionContexts, callback: () => T): T; +}; + +const PgliteTransactionStorage = getAsyncLocalStorageCtor(); +const pgliteTransactionGlobal = globalThis as typeof globalThis & { + __agentNativePgliteTransactionStorage?: PgliteTransactionStorage; +}; +const pgliteTransactionStorage = + pgliteTransactionGlobal.__agentNativePgliteTransactionStorage ?? + (PgliteTransactionStorage + ? (pgliteTransactionGlobal.__agentNativePgliteTransactionStorage = + new PgliteTransactionStorage()) + : undefined); + +/** Active native PGlite transaction for this database and async call chain. */ +export function getActivePgliteTransactionClient(url: string): any | undefined { + return pgliteTransactionStorage?.getStore()?.get(pgliteClientKeyFromUrl(url)) + ?.client; +} + +function getActivePgliteTransactionExec(url: string): DbExec | undefined { + return pgliteTransactionStorage?.getStore()?.get(pgliteClientKeyFromUrl(url)) + ?.exec; +} + function hasCloudflareRuntime(): boolean { const runtime = globalThis as typeof globalThis & { __cf_env?: unknown; @@ -278,6 +313,10 @@ export function pgliteRuntimeDataDir(dataDir: string): string { return path.join("/tmp", safeRelative); } +export function pgliteClientKeyFromUrl(url: string): string { + return pgliteClientKey(pgliteRuntimeDataDir(pgliteDataDirFromUrl(url))); +} + async function preparePgliteDataDir(dataDir: string): Promise { const runtimeDataDir = pgliteRuntimeDataDir(dataDir); if (runtimeDataDir === "memory://") return runtimeDataDir; @@ -1661,14 +1700,36 @@ async function createDbExecInternal( if (isPgliteUrl(url)) { const client = await getPgliteClient(url); + const clientKey = pgliteClientKeyFromUrl(url); return { - execute: (sql) => executePglite(client, sql), + execute: (sql) => + executePglite(getActivePgliteTransactionClient(url) ?? client, sql), async transaction(fn: (tx: DbExec) => Promise): Promise { - return client.transaction((tx: any) => - fn({ + if (getActivePgliteTransactionExec(url)) { + throw new Error( + "Nested PGlite transactions are not supported; reuse the active transaction handle.", + ); + } + if (!pgliteTransactionStorage) { + throw new Error( + "PGlite transactions require AsyncLocalStorage so database access stays on the active transaction handle.", + ); + } + return client.transaction((tx: any) => { + const transactionExec: DbExec = { execute: (sql) => executePglite(tx, sql), - }), - ); + }; + const activeTransactions = new Map( + pgliteTransactionStorage.getStore(), + ); + activeTransactions.set(clientKey, { + client: tx, + exec: transactionExec, + }); + return pgliteTransactionStorage.run(activeTransactions, () => + fn(transactionExec), + ); + }); }, }; } diff --git a/packages/core/src/db/create-get-db.ts b/packages/core/src/db/create-get-db.ts index 171c359ed3f..a350be2dd9b 100644 --- a/packages/core/src/db/create-get-db.ts +++ b/packages/core/src/db/create-get-db.ts @@ -1,6 +1,7 @@ import type { PgDatabase, PgQueryResultHKT } from "drizzle-orm/pg-core"; import { + getActivePgliteTransactionClient, getRuntimeDatabaseUrl, isPgliteUrl, isConnectionError, @@ -367,6 +368,7 @@ export function createGetDb>(schema: T) { _dbReady = loadPgliteDrizzle().then(async ({ drizzle }) => { const client = await getPgliteClient(url); _db = drizzle({ client, schema }); + return _db; }); return _dbReady; } @@ -389,6 +391,7 @@ export function createGetDb>(schema: T) { // acquire-timeout (pre-send) errors to avoid double-execution. const pool = buildResilientNeonPool(rawPool); _db = drizzle(pool, { schema }); + return _db; }); } else { _dbReady = getPgDrizzle().then(({ drizzle, postgres }) => { @@ -401,6 +404,7 @@ export function createGetDb>(schema: T) { postgres(url, pgPoolOptions(url)), ); _db = drizzle(buildResilientPostgresJsClient(client), { schema }); + return _db; }); } return _dbReady; @@ -422,8 +426,8 @@ export function createGetDb>(schema: T) { get(_target, prop) { // When awaited, replay the chain on the real db if (prop === "then" || prop === "catch" || prop === "finally") { - const promise = ready.then(() => { - let result: any = _db; + const promise = ready.then((readyDb) => { + let result: any = readyDb; for (const step of chain) { const val = result[step.prop]; result = @@ -476,6 +480,19 @@ export function createGetDb>(schema: T) { * the final result, the proxy is transparent. */ function getDb(): PgDatabase { + const url = getRuntimeDatabaseUrl("pglite:./data/pglite"); + const activePgliteClient = isPgliteUrl(url) + ? getActivePgliteTransactionClient(url) + : undefined; + if (activePgliteClient) { + const transactionDb = loadPgliteDrizzle().then(({ drizzle }) => + drizzle({ client: activePgliteClient, schema }), + ); + return createLazyProxy(transactionDb, []) as PgDatabase< + PgQueryResultHKT, + T + >; + } if (_db) return _db; void startInit(); if (_db) return _db; diff --git a/packages/core/src/framework-tools.spec.ts b/packages/core/src/framework-tools.spec.ts index 2fa95d6eee7..ada995d22bf 100644 --- a/packages/core/src/framework-tools.spec.ts +++ b/packages/core/src/framework-tools.spec.ts @@ -157,6 +157,16 @@ describe("isFrameworkGroupedAction", () => { }); describe("group membership resolves by name, not only by tag", () => { + it("keeps suggestion amendments in the review group", () => { + expect(CORE_ACTION_GROUPS["update-resource-suggestion"]).toBe("review"); + expect( + filterFrameworkToolGroups( + { "update-resource-suggestion": {} }, + new Set(["review"]), + ), + ).toEqual({}); + }); + // The guard this file was missing. Every test above stamped `frameworkGroup` // by hand, so the filter looked correct while the tag was reaching almost no // real registry: it is written only by `mergeCoreSharingActions`, which runs diff --git a/packages/core/src/framework-tools.ts b/packages/core/src/framework-tools.ts index 3cf0301b0ad..e17da317ecc 100644 --- a/packages/core/src/framework-tools.ts +++ b/packages/core/src/framework-tools.ts @@ -333,6 +333,14 @@ export const CORE_ACTION_GROUPS: Record = { "get-review-feedback": "review", "set-review-status": "review", "send-review-thread-to-agent": "review", + "react-to-review-comment": "review", + "set-review-thread-unread": "review", + "set-review-thread-muted": "review", + "create-resource-suggestion": "review", + "update-resource-suggestion": "review", + "list-resource-suggestions": "review", + "get-resource-suggestion": "review", + "decide-resource-suggestion": "review", }; /** Structural view of the one field these helpers read, so tagging utilities diff --git a/packages/core/src/review/actions/list-review-comments.ts b/packages/core/src/review/actions/list-review-comments.ts index 621015a2838..e0d901e677f 100644 --- a/packages/core/src/review/actions/list-review-comments.ts +++ b/packages/core/src/review/actions/list-review-comments.ts @@ -15,6 +15,7 @@ import { import { assertReviewableResourceAccess } from "../registry.js"; import { getReviewStatus, + getReviewDiscussionStateForComments, getReviewThreadSummary, queryReviewComments, } from "../store.js"; @@ -71,6 +72,16 @@ export default defineAction({ targetId: args.targetId, }), ]); + const discussion = { + ...(await getReviewDiscussionStateForComments( + comments, + actionCtx?.userEmail ?? null, + )), + canReact: + Boolean(actionCtx?.userEmail) && + roleSatisfies(access.role, "commenter"), + canSetThreadPreferences: Boolean(actionCtx?.userEmail), + }; const profiles = await getUserProfiles( comments.flatMap((comment) => comment.authorEmail && @@ -105,7 +116,13 @@ export default defineAction({ ), reviewStatus: redactPublicReviewStatusIdentity(reviewStatus), summary, + discussion, } - : { comments: commentsWithCapabilities, reviewStatus, summary }; + : { + comments: commentsWithCapabilities, + reviewStatus, + summary, + discussion, + }; }, }); diff --git a/packages/core/src/review/actions/react-to-review-comment.ts b/packages/core/src/review/actions/react-to-review-comment.ts new file mode 100644 index 00000000000..e28686dfb7d --- /dev/null +++ b/packages/core/src/review/actions/react-to-review-comment.ts @@ -0,0 +1,54 @@ +import { z } from "zod"; + +import { defineAction, fail } from "../../action.js"; +import { assertReviewableResourceAccess } from "../registry.js"; +import { getReviewCommentById, setReviewCommentReaction } from "../store.js"; + +export default defineAction({ + description: "Add or remove your reaction on a review comment.", + schema: z.object({ + resourceType: z.string().min(1), + resourceId: z.string().min(1), + commentId: z.string().min(1), + reaction: z.string().trim().min(1).max(32), + active: z.boolean(), + }), + run: async (args, ctx) => { + await assertReviewableResourceAccess( + args.resourceType, + args.resourceId, + ctx as any, + "commenter", + ); + const comment = await getReviewCommentById( + args.commentId, + { + userEmail: (ctx as any)?.userEmail, + orgId: (ctx as any)?.orgId, + }, + { bypassScope: true }, + ); + if ( + !comment || + comment.status === "deleted" || + comment.resourceType !== args.resourceType || + comment.resourceId !== args.resourceId + ) + fail("Review comment not found", { + statusCode: 404, + errorCode: "not_found", + }); + const actorEmail = (ctx as any)?.userEmail; + if (!actorEmail) + fail("A signed-in actor is required", { + statusCode: 401, + errorCode: "unauthenticated", + }); + return setReviewCommentReaction({ + commentId: comment.id, + actorEmail, + reaction: args.reaction, + active: args.active, + }); + }, +}); diff --git a/packages/core/src/review/actions/review-actions.spec.ts b/packages/core/src/review/actions/review-actions.spec.ts index 578039b4af9..6b0ab6e9cd2 100644 --- a/packages/core/src/review/actions/review-actions.spec.ts +++ b/packages/core/src/review/actions/review-actions.spec.ts @@ -31,6 +31,15 @@ const getReviewFeedbackAction = (await import("./get-review-feedback.js")) .default; const listReviewCommentsAction = (await import("./list-review-comments.js")) .default; +const reactToReviewCommentAction = ( + await import("./react-to-review-comment.js") +).default; +const setReviewThreadUnreadAction = ( + await import("./set-review-thread-unread.js") +).default; +const setReviewThreadMutedAction = ( + await import("./set-review-thread-muted.js") +).default; const replyReviewCommentAction = (await import("./reply-review-comment.js")) .default; const resolveReviewThreadAction = (await import("./resolve-review-thread.js")) @@ -105,6 +114,154 @@ beforeEach(async () => { await ensureReviewTables(); }); +it("denies discussion tool writes without resource access or against deleted comments", async () => { + const root = await insertReviewComment({ + resourceType: "doc", + resourceId: "private", + body: "Review", + ownerEmail: OWNER_EMAIL, + }); + const resource = { resourceType: "doc", resourceId: "private" }; + const calls = [ + (ctx: { userEmail: string }) => + reactToReviewCommentAction.run( + { ...resource, commentId: root.id, reaction: "👍", active: true }, + ctx, + ), + (ctx: { userEmail: string }) => + setReviewThreadMutedAction.run( + { ...resource, threadId: root.threadId, muted: true }, + ctx, + ), + (ctx: { userEmail: string }) => + setReviewThreadUnreadAction.run( + { ...resource, threadId: root.threadId, unread: true }, + ctx, + ), + ]; + for (const call of calls) + await expect(call({ userEmail: "outsider@example.com" })).rejects.toThrow(); + await rawClient.execute({ + sql: "UPDATE agent_review_comments SET status = ? WHERE id = ?", + args: ["deleted", root.id], + }); + for (const call of calls) + await expect(call({ userEmail: EDITOR_EMAIL })).rejects.toThrow( + /not found/, + ); + const counts = await rawClient.execute({ + sql: "SELECT (SELECT COUNT(*) FROM agent_review_comment_reactions) AS reactions, (SELECT COUNT(*) FROM agent_review_thread_preferences) AS preferences", + args: [], + }); + expect(Number(counts.rows[0].reactions)).toBe(0); + expect(Number(counts.rows[0].preferences)).toBe(0); +}); + +it("uses current resource access for reaction and preference tools on decided threads", async () => { + const root = await insertReviewComment({ + resourceType: "doc", + resourceId: "private", + body: "Review", + ownerEmail: OWNER_EMAIL, + }); + const context = { userEmail: EDITOR_EMAIL, caller: "frontend" }; + const resource = { resourceType: "doc", resourceId: "private" }; + await resolveReviewThreadAction.run( + { ...resource, threadId: root.threadId }, + context, + ); + await reactToReviewCommentAction.run( + { ...resource, commentId: root.id, reaction: "👍", active: true }, + context, + ); + await setReviewThreadUnreadAction.run( + { ...resource, threadId: root.threadId, unread: true }, + context, + ); + await setReviewThreadMutedAction.run( + { ...resource, threadId: root.threadId, muted: true }, + context, + ); + const result = await listReviewCommentsAction.run( + { ...resource, includeResolved: true }, + context, + ); + expect(result.discussion).toMatchObject({ + canReact: true, + canSetThreadPreferences: true, + reactions: { [root.id]: [{ reaction: "👍", count: 1, reactedByMe: true }] }, + threadPreferences: { [root.threadId]: { muted: true, unread: true } }, + }); + await expect( + reactToReviewCommentAction.run( + { + ...resource, + resourceId: "other", + commentId: root.id, + reaction: "👍", + active: true, + }, + context, + ), + ).rejects.toThrow("Review comment not found"); + await expect( + setReviewThreadMutedAction.run( + { + ...resource, + resourceId: "other", + threadId: root.threadId, + muted: true, + }, + context, + ), + ).rejects.toThrow("Review thread not found"); + const publicRoot = await insertReviewComment({ + resourceType: "doc", + resourceId: "public", + body: "Public review", + ownerEmail: OWNER_EMAIL, + visibility: "public", + }); + await expect( + reactToReviewCommentAction.run( + { + ...resource, + resourceId: "public", + commentId: publicRoot.id, + reaction: "👍", + active: true, + }, + { userEmail: "viewer@example.com" }, + ), + ).rejects.toThrow(); + await setReviewThreadMutedAction.run( + { + ...resource, + resourceId: "public", + threadId: publicRoot.threadId, + muted: true, + }, + { userEmail: "viewer@example.com" }, + ); + await expect( + setReviewThreadMutedAction.run({ + ...resource, + resourceId: "public", + threadId: publicRoot.threadId, + muted: true, + }), + ).rejects.toThrow("A signed-in user is required"); + const publicResult = await listReviewCommentsAction.run({ + resourceType: "doc", + resourceId: "public", + }); + expect(publicResult.discussion.canReact).toBe(false); + expect(publicResult.discussion.canSetThreadPreferences).toBe(false); + expect( + publicResult.discussion.threadPreferences[publicRoot.threadId], + ).toEqual({ muted: false, unread: false }); +}); + afterEach(async () => { vi.useRealTimers(); __resetReviewableResourcesForTests(); diff --git a/packages/core/src/review/actions/set-review-thread-muted.ts b/packages/core/src/review/actions/set-review-thread-muted.ts new file mode 100644 index 00000000000..ebb6c27ed35 --- /dev/null +++ b/packages/core/src/review/actions/set-review-thread-muted.ts @@ -0,0 +1,46 @@ +import { z } from "zod"; + +import { defineAction, fail } from "../../action.js"; +import { assertReviewableResourceAccess } from "../registry.js"; +import { getReviewThreadRoot, setReviewThreadPreference } from "../store.js"; + +export default defineAction({ + description: + "Mute or unmute replies on a review thread for the current user.", + schema: z.object({ + resourceType: z.string().min(1), + resourceId: z.string().min(1), + threadId: z.string().min(1), + muted: z.boolean(), + }), + run: async (args, ctx) => { + await assertReviewableResourceAccess( + args.resourceType, + args.resourceId, + ctx as any, + "viewer", + ); + const root = await getReviewThreadRoot( + args.threadId, + { resourceType: args.resourceType, resourceId: args.resourceId }, + { userEmail: (ctx as any)?.userEmail, orgId: (ctx as any)?.orgId }, + { bypassScope: true }, + ); + if (!root || root.status === "deleted") + fail("Review thread not found", { + statusCode: 404, + errorCode: "not_found", + }); + const userEmail = (ctx as any)?.userEmail; + if (!userEmail) + fail("A signed-in user is required", { + statusCode: 401, + errorCode: "unauthenticated", + }); + return setReviewThreadPreference({ + threadId: args.threadId, + userEmail, + muted: args.muted, + }); + }, +}); diff --git a/packages/core/src/review/actions/set-review-thread-unread.ts b/packages/core/src/review/actions/set-review-thread-unread.ts new file mode 100644 index 00000000000..9d5456b123a --- /dev/null +++ b/packages/core/src/review/actions/set-review-thread-unread.ts @@ -0,0 +1,45 @@ +import { z } from "zod"; + +import { defineAction, fail } from "../../action.js"; +import { assertReviewableResourceAccess } from "../registry.js"; +import { getReviewThreadRoot, setReviewThreadPreference } from "../store.js"; + +export default defineAction({ + description: "Mark a review thread unread or read for the current user.", + schema: z.object({ + resourceType: z.string().min(1), + resourceId: z.string().min(1), + threadId: z.string().min(1), + unread: z.boolean(), + }), + run: async (args, ctx) => { + await assertReviewableResourceAccess( + args.resourceType, + args.resourceId, + ctx as any, + "viewer", + ); + const root = await getReviewThreadRoot( + args.threadId, + { resourceType: args.resourceType, resourceId: args.resourceId }, + { userEmail: (ctx as any)?.userEmail, orgId: (ctx as any)?.orgId }, + { bypassScope: true }, + ); + if (!root || root.status === "deleted") + fail("Review thread not found", { + statusCode: 404, + errorCode: "not_found", + }); + const userEmail = (ctx as any)?.userEmail; + if (!userEmail) + fail("A signed-in user is required", { + statusCode: 401, + errorCode: "unauthenticated", + }); + return setReviewThreadPreference({ + threadId: args.threadId, + userEmail, + unread: args.unread, + }); + }, +}); diff --git a/packages/core/src/review/index.ts b/packages/core/src/review/index.ts index cee728f49f3..b6eb79b6d6f 100644 --- a/packages/core/src/review/index.ts +++ b/packages/core/src/review/index.ts @@ -3,6 +3,9 @@ export type { ReviewComment, ReviewCommentKind, ReviewCommentStatus, + ReviewCommentReaction, + ReviewThreadPreference, + ReviewDiscussionState, ReviewMention, ReviewResolutionTarget, ReviewResourceAccess, @@ -44,6 +47,20 @@ export { routeReviewThread, sendReviewThreadToAgent, } from "./store.js"; +export * from "./suggestions/types.js"; +export { + registerSuggestionAdapter, + getSuggestionAdapter, + listSuggestionAdapters, + __resetSuggestionAdaptersForTests, +} from "./suggestions/registry.js"; +export { + ensureSuggestionTables, + getDecision, + getSuggestion, + listSuggestions, + __resetSuggestionTablesForTests, +} from "./suggestions/store.js"; export type { GetReviewThreadSummaryInput, ReviewThreadSummary, diff --git a/packages/core/src/review/notifications.spec.ts b/packages/core/src/review/notifications.spec.ts index 6f1887e5289..00254b88afe 100644 --- a/packages/core/src/review/notifications.spec.ts +++ b/packages/core/src/review/notifications.spec.ts @@ -7,6 +7,7 @@ const mocks = vi.hoisted(() => ({ getReviewableResource: vi.fn(), resolveReviewableResourceAccess: vi.fn(), filterRecipientsByResourceAccess: vi.fn(), + filterUnmutedReviewThreadRecipients: vi.fn(), })); vi.mock("../server/activity-notifications.js", async () => { @@ -48,6 +49,8 @@ vi.mock("./registry.js", () => ({ })); vi.mock("./store.js", () => ({ + filterUnmutedReviewThreadRecipients: (...args: unknown[]) => + mocks.filterUnmutedReviewThreadRecipients(...args), queryReviewComments: (...args: unknown[]) => mocks.queryReviewComments(...args), })); @@ -99,6 +102,9 @@ beforeEach(() => { failed: [], }); mocks.queryReviewComments.mockResolvedValue([]); + mocks.filterUnmutedReviewThreadRecipients.mockImplementation( + async (_threadId: string, recipients: string[]) => recipients, + ); mocks.getReviewableResource.mockReturnValue(undefined); // Default: everyone offered still has access. Access filtering has its own // tests; these assert who is *offered*. @@ -109,6 +115,38 @@ beforeEach(() => { }); describe("notifyReviewComment", () => { + it("honors thread mute for reply emails, including explicit mentions", async () => { + mocks.filterUnmutedReviewThreadRecipients.mockResolvedValue([ + "participant@example.com", + ]); + mocks.queryReviewComments.mockResolvedValue([ + { threadId: "t1", authorEmail: "participant@example.com" }, + ]); + await notifyReviewComment( + comment({ + parentCommentId: "root", + mentions: [{ label: "Owner", email: "owner@example.com" }], + }), + ); + expect(mocks.filterUnmutedReviewThreadRecipients).toHaveBeenCalledWith( + "t1", + expect.arrayContaining(["owner@example.com", "participant@example.com"]), + ); + expect(notifyArgs().candidates).toEqual(["participant@example.com"]); + }); + + it("reports unreadable mute preferences without sending reply emails", async () => { + mocks.filterUnmutedReviewThreadRecipients.mockRejectedValue( + new Error("preference store unavailable"), + ); + vi.spyOn(console, "error").mockImplementation(() => {}); + const result = await notifyReviewComment( + comment({ parentCommentId: "root" }), + ); + expect(result.status).toBe("notification-error"); + expect(mocks.notifyActivity).not.toHaveBeenCalled(); + }); + it("notifies the owner and mentions against the shared preference key", async () => { await notifyReviewComment( comment({ diff --git a/packages/core/src/review/notifications.ts b/packages/core/src/review/notifications.ts index 036bbe506bd..d882b03b44b 100644 --- a/packages/core/src/review/notifications.ts +++ b/packages/core/src/review/notifications.ts @@ -20,7 +20,10 @@ import { getReviewableResource, resolveReviewableResourceAccess, } from "./registry.js"; -import { queryReviewComments } from "./store.js"; +import { + filterUnmutedReviewThreadRecipients, + queryReviewComments, +} from "./store.js"; import type { ReviewComment } from "./types.js"; /** @@ -123,7 +126,9 @@ async function deliverReviewCommentEmails( const url = await resourceUrl(comment); return notifyActivity({ - candidates: allowed, + candidates: isReply + ? await filterUnmutedReviewThreadRecipients(comment.threadId, allowed) + : allowed, actorEmail: comment.authorEmail, preferenceKey: REVIEW_NOTIFICATION_PREFS_KEY, logLabel: LOG_LABEL, diff --git a/packages/core/src/review/store.spec.ts b/packages/core/src/review/store.spec.ts index cf622c60bbe..8b3c0b59125 100644 --- a/packages/core/src/review/store.spec.ts +++ b/packages/core/src/review/store.spec.ts @@ -35,6 +35,10 @@ const { resolveReviewThread, sendReviewThreadToAgent, upsertReviewStatus, + setReviewCommentReaction, + setReviewThreadPreference, + getReviewDiscussionStateForComments, + filterUnmutedReviewThreadRecipients, } = await import("./store.js"); beforeEach(async () => { @@ -50,6 +54,108 @@ afterEach(async () => { }); describe("review store", () => { + it("reads reactions without exposing actors and isolates personal thread preferences", async () => { + const comment = await insertReviewComment({ + resourceType: "doc", + resourceId: "discussion", + body: "Review", + ownerEmail: "owner@example.com", + }); + await setReviewCommentReaction({ + commentId: comment.id, + actorEmail: "alice@example.com", + reaction: "👍", + active: true, + }); + await setReviewCommentReaction({ + commentId: comment.id, + actorEmail: "bob@example.com", + reaction: "👍", + active: true, + }); + await setReviewCommentReaction({ + commentId: comment.id, + actorEmail: "alice@example.com", + reaction: "👍", + active: true, + }); + await Promise.all([ + setReviewThreadPreference({ + threadId: comment.threadId, + userEmail: "alice@example.com", + muted: true, + }), + setReviewThreadPreference({ + threadId: comment.threadId, + userEmail: "alice@example.com", + unread: true, + }), + ]); + const alice = await getReviewDiscussionStateForComments( + [comment], + "alice@example.com", + ); + expect(alice.reactions[comment.id]).toEqual([ + { reaction: "👍", count: 2, reactedByMe: true }, + ]); + expect(alice.threadPreferences[comment.threadId]).toEqual({ + muted: true, + unread: true, + }); + const bob = await getReviewDiscussionStateForComments( + [comment], + "bob@example.com", + ); + expect(bob.threadPreferences[comment.threadId]).toEqual({ + muted: false, + unread: false, + }); + const anonymous = await getReviewDiscussionStateForComments( + [comment], + null, + ); + expect(anonymous.reactions[comment.id]).toEqual([ + { reaction: "👍", count: 2, reactedByMe: false }, + ]); + expect(JSON.stringify(anonymous)).not.toContain("@example.com"); + expect( + await filterUnmutedReviewThreadRecipients(comment.threadId, [ + "alice@example.com", + "bob@example.com", + ]), + ).toEqual(["bob@example.com"]); + await setReviewThreadPreference({ + threadId: comment.threadId, + userEmail: "alice@example.com", + muted: false, + }); + expect( + ( + await getReviewDiscussionStateForComments( + [comment], + "alice@example.com", + ) + ).threadPreferences[comment.threadId], + ).toEqual({ muted: false, unread: true }); + await setReviewCommentReaction({ + commentId: comment.id, + actorEmail: "alice@example.com", + reaction: "👍", + active: false, + }); + expect( + ( + await getReviewDiscussionStateForComments( + [comment], + "alice@example.com", + ) + ).reactions[comment.id], + ).toEqual([{ reaction: "👍", count: 1, reactedByMe: false }]); + expect( + await getReviewDiscussionStateForComments([], "alice@example.com"), + ).toEqual({ reactions: {}, threadPreferences: {} }); + }); + it("stores threaded comments with anchors, mentions, and metadata", async () => { const root = await insertReviewComment({ resourceType: "plan", diff --git a/packages/core/src/review/store.ts b/packages/core/src/review/store.ts index 3ecf46b0842..0507ecb3b14 100644 --- a/packages/core/src/review/store.ts +++ b/packages/core/src/review/store.ts @@ -6,6 +6,8 @@ import type { ReviewComment, ReviewCommentKind, ReviewCommentStatus, + ReviewCommentReaction, + ReviewThreadPreference, ReviewMention, ReviewResolutionTarget, ReviewScope, @@ -117,6 +119,21 @@ export async function ensureReviewTables(): Promise { org_id TEXT, visibility TEXT NOT NULL DEFAULT 'private', metadata_json TEXT + )`; + const createReactionsSql = `CREATE TABLE IF NOT EXISTS agent_review_comment_reactions ( + comment_id TEXT NOT NULL, + actor_email TEXT NOT NULL, + reaction TEXT NOT NULL, + created_at TEXT NOT NULL, + PRIMARY KEY (comment_id, actor_email, reaction) + )`; + const createPreferencesSql = `CREATE TABLE IF NOT EXISTS agent_review_thread_preferences ( + thread_id TEXT NOT NULL, + user_email TEXT NOT NULL, + muted INTEGER NOT NULL DEFAULT 0, + unread INTEGER NOT NULL DEFAULT 0, + updated_at TEXT NOT NULL, + PRIMARY KEY (thread_id, user_email) )`; const indexes = [ `CREATE INDEX IF NOT EXISTS idx_agent_review_comments_resource @@ -143,6 +160,14 @@ export async function ensureReviewTables(): Promise { { await ensureTableExists("agent_review_comments", createCommentsSql); await ensureTableExists("agent_review_statuses", createStatusesSql); + await ensureTableExists( + "agent_review_comment_reactions", + createReactionsSql, + ); + await ensureTableExists( + "agent_review_thread_preferences", + createPreferencesSql, + ); await ensureIndexExists( "idx_agent_review_comments_resource", indexes[0], @@ -162,6 +187,129 @@ export async function ensureReviewTables(): Promise { await reviewTablesInitPromise; } +export async function setReviewCommentReaction(input: { + commentId: string; + actorEmail: string; + reaction: string; + active: boolean; +}) { + await ensureReviewTables(); + const client = getDbExec(); + if (input.active) { + await client.execute({ + sql: "INSERT INTO agent_review_comment_reactions (comment_id,actor_email,reaction,created_at) VALUES (?,?,?,?) ON CONFLICT (comment_id,actor_email,reaction) DO NOTHING", + args: [ + input.commentId, + input.actorEmail, + input.reaction, + new Date().toISOString(), + ], + }); + } else { + await client.execute({ + sql: "DELETE FROM agent_review_comment_reactions WHERE comment_id = ? AND actor_email = ? AND reaction = ?", + args: [input.commentId, input.actorEmail, input.reaction], + }); + } + return { ...input }; +} + +export async function setReviewThreadPreference(input: { + threadId: string; + userEmail: string; + muted?: boolean; + unread?: boolean; +}) { + await ensureReviewTables(); + const client = getDbExec(); + const fields = (["muted", "unread"] as const).filter( + (field) => input[field] !== undefined, + ); + if (!fields.length) throw new Error("A review thread preference is required"); + await client.execute({ + sql: `INSERT INTO agent_review_thread_preferences (thread_id,user_email,muted,unread,updated_at) VALUES (?,?,?,?,?) ON CONFLICT (thread_id,user_email) DO UPDATE SET ${fields.map((field) => `${field} = excluded.${field}`).join(", ")}, updated_at = excluded.updated_at`, + args: [ + input.threadId, + input.userEmail, + input.muted ? 1 : 0, + input.unread ? 1 : 0, + new Date().toISOString(), + ], + }); + const row = ( + await client.execute({ + sql: "SELECT muted,unread FROM agent_review_thread_preferences WHERE thread_id = ? AND user_email = ?", + args: [input.threadId, input.userEmail], + }) + ).rows[0]; + if (!row) + throw new Error("Persisted review thread preference is unavailable"); + return { + threadId: input.threadId, + muted: Boolean(row.muted), + unread: Boolean(row.unread), + }; +} + +// The caller supplies comments already authorized by the resource access check. +export async function getReviewDiscussionStateForComments( + comments: Pick[], + userEmail: string | null, +) { + const reactions: Record = {}; + const threadPreferences: Record = {}; + if (!comments.length) return { reactions, threadPreferences }; + await ensureReviewTables(); + const commentIds = [...new Set(comments.map((comment) => comment.id))]; + const threadIds = [...new Set(comments.map((comment) => comment.threadId))]; + for (const id of commentIds) reactions[id] = []; + for (const id of threadIds) + threadPreferences[id] = { muted: false, unread: false }; + const client = getDbExec(); + const [reactionRows, preferenceRows] = await Promise.all([ + client.execute({ + sql: `SELECT comment_id,reaction,COUNT(*) AS count,MAX(CASE WHEN actor_email = ? THEN 1 ELSE 0 END) AS reacted_by_me FROM agent_review_comment_reactions WHERE comment_id IN (${commentIds.map(() => "?").join(",")}) GROUP BY comment_id,reaction ORDER BY comment_id,reaction`, + args: [userEmail, ...commentIds], + }), + userEmail + ? client.execute({ + sql: `SELECT thread_id,muted,unread FROM agent_review_thread_preferences WHERE user_email = ? AND thread_id IN (${threadIds.map(() => "?").join(",")})`, + args: [userEmail, ...threadIds], + }) + : Promise.resolve({ rows: [] }), + ]); + for (const row of reactionRows.rows) { + reactions[String(row.comment_id)].push({ + reaction: String(row.reaction), + count: Number(row.count), + reactedByMe: Boolean(row.reacted_by_me), + }); + } + for (const row of preferenceRows.rows) { + threadPreferences[String(row.thread_id)] = { + muted: Boolean(row.muted), + unread: Boolean(row.unread), + }; + } + return { reactions, threadPreferences }; +} + +export async function filterUnmutedReviewThreadRecipients( + threadId: string, + recipients: string[], +) { + if (!recipients.length) return []; + await ensureReviewTables(); + const rows = ( + await getDbExec().execute({ + sql: `SELECT user_email FROM agent_review_thread_preferences WHERE thread_id = ? AND muted = 1 AND user_email IN (${recipients.map(() => "?").join(",")})`, + args: [threadId, ...recipients], + }) + ).rows; + const muted = new Set(rows.map((row) => String(row.user_email))); + return recipients.filter((email) => !muted.has(email)); +} + export async function insertReviewComment( input: InsertReviewCommentInput, ): Promise { @@ -216,7 +364,7 @@ export async function insertReviewReply( } } -async function insertReviewCommentWithClient( +export async function insertReviewCommentWithClient( input: InsertReviewCommentInput, client: DbExec, ): Promise { @@ -516,7 +664,7 @@ export async function resolveReviewThread( return client.transaction ? client.transaction(resolve) : resolve(client); } -async function resolveReviewThreadWithClient( +export async function resolveReviewThreadWithClient( client: DbExec, threadId: string, resolvedBy?: string | null, diff --git a/packages/core/src/review/suggestions/actions.access.spec.ts b/packages/core/src/review/suggestions/actions.access.spec.ts new file mode 100644 index 00000000000..3a9d8bd6a0f --- /dev/null +++ b/packages/core/src/review/suggestions/actions.access.spec.ts @@ -0,0 +1,434 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { createTestPglite } from "../../a2a/test-pglite.js"; +import type { ReviewResourceContext } from "../types.js"; + +let pglite: Awaited>; +const transaction = { + execute: vi.fn(async (input: string | { sql: string; args?: unknown[] }) => { + if (typeof input === "string") { + await pglite.exec(input); + return { rows: [], rowsAffected: 0 }; + } + const result = await pglite.query(input.sql, input.args ?? []); + return { + rows: Array.from(result.rows ?? []), + rowsAffected: result.affectedRows ?? result.rowCount ?? 0, + }; + }), +}; +const client = { + transaction: vi.fn(async (run: (tx: typeof transaction) => Promise) => { + await pglite.exec("BEGIN"); + try { + const result = await run(transaction); + await pglite.exec("COMMIT"); + return result; + } catch (error) { + await pglite.exec("ROLLBACK"); + throw error; + } + }), +}; +const updateSuggestionStatus = vi.fn(); +const applySuggestion = vi.fn(); +const validateProposal = vi.fn(); +const suggestion = { + id: "suggestion-1", + revision: 1, + resourceType: "doc", + resourceId: "doc-1", + adapterKind: "test.adapter", + adapterVersion: 1, + threadId: "thread-1", + authorEmail: "commenter@example.com", + actorKind: "human" as const, + baseRevision: "revision-1", + status: "pending" as const, + summary: "Replace text", + ownerEmail: "owner@example.com", + orgId: null, + visibility: "private" as const, + createdAt: "now", + updatedAt: "now", + metadata: null, + operations: [], +}; + +vi.mock("../../db/client.js", () => ({ + getDbExec: () => client, + isProductionServerlessFunctionRuntime: () => false, +})); +vi.mock("../notifications.js", () => ({ notifyReviewComment: vi.fn() })); +vi.mock("../store.js", () => ({ + ensureReviewTables: vi.fn(), + insertReviewCommentWithClient: vi.fn(), + resolveReviewThreadWithClient: vi.fn(), +})); +vi.mock("./store.js", () => ({ + ensureSuggestionTables: vi.fn(), + getSuggestion: vi.fn(async () => suggestion), + getSuggestionByCreationKey: vi.fn(), + insertSuggestion: vi.fn(), + listSuggestions: vi.fn(), + recordDecision: vi.fn(), + getDecision: vi.fn(), + recordSuggestionCreation: vi.fn(), + deleteUnclaimedSuggestion: vi.fn(), + replaceSuggestionStatus: vi.fn(), + updateSuggestionStatus, +})); + +const { createResourceSuggestion, decideResourceSuggestion } = + await import("./actions.js"); +const suggestionStore = await import("./store.js"); +const reviewStore = await import("../store.js"); +const { __resetReviewableResourcesForTests, registerReviewableResource } = + await import("../registry.js"); +const { __resetSuggestionAdaptersForTests, registerSuggestionAdapter } = + await import("./registry.js"); + +const createArgs = { + resourceType: suggestion.resourceType, + resourceId: suggestion.resourceId, + adapterKind: suggestion.adapterKind, + baseRevision: suggestion.baseRevision, + summary: suggestion.summary, + idempotencyKey: "create-1", + operations: [ + { + ordinal: 0, + kind: "replace_text", + targetId: "body", + before: "Original", + after: "Proposed", + schemaVersion: 1, + }, + ], +}; +const createRequest = JSON.stringify({ + adapterKind: createArgs.adapterKind, + baseRevision: createArgs.baseRevision, + metadata: null, + operations: [ + { + after: "Proposed", + before: "Original", + kind: "replace_text", + ordinal: 0, + schemaVersion: 1, + targetId: "body", + }, + ], + resourceId: createArgs.resourceId, + resourceType: createArgs.resourceType, + summary: createArgs.summary, +}); +const createRequestHash = Array.from( + new Uint8Array( + await globalThis.crypto.subtle.digest( + "SHA-256", + new TextEncoder().encode(createRequest), + ), + ), + (byte) => byte.toString(16).padStart(2, "0"), +).join(""); +const creationReceipt = { + suggestion, + authorEmail: suggestion.authorEmail, + actorKind: suggestion.actorKind, + requestHash: createRequestHash, +}; + +function expectNoReviewWrites() { + expect(suggestionStore.insertSuggestion).not.toHaveBeenCalled(); + expect(suggestionStore.recordSuggestionCreation).not.toHaveBeenCalled(); + expect(suggestionStore.replaceSuggestionStatus).not.toHaveBeenCalled(); + expect(suggestionStore.updateSuggestionStatus).not.toHaveBeenCalled(); + expect(suggestionStore.recordDecision).not.toHaveBeenCalled(); + expect(reviewStore.insertReviewCommentWithClient).not.toHaveBeenCalled(); + expect(reviewStore.resolveReviewThreadWithClient).not.toHaveBeenCalled(); + expect(applySuggestion).not.toHaveBeenCalled(); + expect(transaction.execute).not.toHaveBeenCalled(); +} + +describe("suggestion action access", () => { + beforeEach(async () => { + pglite = await createTestPglite(); + vi.clearAllMocks(); + validateProposal.mockReset(); + applySuggestion.mockReset(); + vi.mocked(suggestionStore.getSuggestion) + .mockReset() + .mockResolvedValue(suggestion); + vi.mocked(suggestionStore.getSuggestionByCreationKey) + .mockReset() + .mockResolvedValue(null); + vi.mocked(suggestionStore.insertSuggestion).mockReset(); + vi.mocked(suggestionStore.recordSuggestionCreation).mockReset(); + vi.mocked(suggestionStore.deleteUnclaimedSuggestion).mockReset(); + vi.mocked(suggestionStore.getDecision).mockReset(); + vi.mocked(suggestionStore.recordDecision).mockReset(); + updateSuggestionStatus.mockReset(); + __resetReviewableResourcesForTests(); + __resetSuggestionAdaptersForTests(); + registerReviewableResource({ + type: "doc", + resolveAccess: (_resourceId, ctx) => ({ + role: ctx?.transaction ? "viewer" : "editor", + ownerEmail: "owner@example.com", + visibility: "private", + }), + }); + registerSuggestionAdapter({ + kind: "test.adapter", + version: 1, + validateProposal, + apply: applySuggestion, + }); + }); + + afterEach(async () => { + await pglite.close(); + }); + + it("denies a viewer creating a suggestion before any writes or adapter work", async () => { + const resolveAccess = vi.fn( + (_resourceId: string, _ctx?: ReviewResourceContext) => ({ + role: "viewer" as const, + ownerEmail: "owner@example.com", + visibility: "private" as const, + }), + ); + registerReviewableResource({ type: "doc", resolveAccess }); + + await expect( + createResourceSuggestion.run( + { + resourceType: suggestion.resourceType, + resourceId: suggestion.resourceId, + adapterKind: suggestion.adapterKind, + baseRevision: suggestion.baseRevision, + summary: "Replace text", + idempotencyKey: "viewer-create-1", + operations: [ + { + ordinal: 0, + kind: "replace_text", + targetId: "body", + before: "Original", + after: "Proposed", + schemaVersion: 1, + }, + ], + }, + { userEmail: "viewer@example.com" }, + ), + ).rejects.toThrow("Not allowed to access doc:doc-1"); + + expect(resolveAccess).toHaveBeenCalledWith( + "doc-1", + expect.objectContaining({ userEmail: "viewer@example.com" }), + ); + expect(validateProposal).not.toHaveBeenCalled(); + expect(client.transaction).not.toHaveBeenCalled(); + expectNoReviewWrites(); + }); + + it("replays the original creation receipt before canonical validation", async () => { + vi.mocked(suggestionStore.getSuggestionByCreationKey).mockResolvedValueOnce( + creationReceipt, + ); + validateProposal.mockRejectedValueOnce(new Error("Canonical changed")); + + await expect( + createResourceSuggestion.run(createArgs, { + userEmail: suggestion.authorEmail, + }), + ).resolves.toEqual(suggestion); + + expect(validateProposal).not.toHaveBeenCalled(); + expect(suggestionStore.insertSuggestion).not.toHaveBeenCalled(); + }); + + it("rejects creation-key replay by a different caller or request", async () => { + vi.mocked(suggestionStore.getSuggestionByCreationKey).mockResolvedValue( + creationReceipt, + ); + + await expect( + createResourceSuggestion.run(createArgs, { + userEmail: "other@example.com", + }), + ).rejects.toThrow("different suggestion"); + await expect( + createResourceSuggestion.run( + { ...createArgs, summary: "Different request" }, + { userEmail: suggestion.authorEmail }, + ), + ).rejects.toThrow("different suggestion"); + expect(validateProposal).not.toHaveBeenCalled(); + }); + + it("returns the winning receipt and removes its unclaimed duplicate after a creation race", async () => { + const duplicate = { ...suggestion, id: "suggestion-duplicate" }; + vi.mocked(suggestionStore.getSuggestionByCreationKey).mockResolvedValueOnce( + null, + ); + vi.mocked(suggestionStore.insertSuggestion).mockResolvedValueOnce( + duplicate, + ); + vi.mocked(suggestionStore.recordSuggestionCreation).mockResolvedValueOnce( + creationReceipt, + ); + validateProposal.mockResolvedValueOnce(createArgs.operations); + + await expect( + createResourceSuggestion.run(createArgs, { + userEmail: suggestion.authorEmail, + }), + ).resolves.toEqual(suggestion); + + expect(suggestionStore.deleteUnclaimedSuggestion).toHaveBeenCalledWith( + transaction, + duplicate.id, + ); + expect(reviewStore.insertReviewCommentWithClient).not.toHaveBeenCalled(); + }); + + it.each(["accepted", "rejected"] as const)( + "denies a commenter deciding %s without changing the suggestion or canonical resource", + async (decision) => { + const resolveAccess = vi.fn( + (_resourceId: string, _ctx?: ReviewResourceContext) => ({ + role: "commenter" as const, + ownerEmail: "owner@example.com", + visibility: "private" as const, + }), + ); + registerReviewableResource({ type: "doc", resolveAccess }); + + await expect( + decideResourceSuggestion.run( + { + id: suggestion.id, + decision, + idempotencyKey: `commenter-${decision}-1`, + observedBase: suggestion.baseRevision, + }, + { userEmail: "commenter@example.com" }, + ), + ).rejects.toThrow("Not allowed to access doc:doc-1"); + + expect(resolveAccess).toHaveBeenCalledWith( + "doc-1", + expect.objectContaining({ userEmail: "commenter@example.com" }), + ); + expect(client.transaction).not.toHaveBeenCalled(); + expectNoReviewWrites(); + }, + ); + + it("rechecks editor access inside the decision transaction", async () => { + await expect( + decideResourceSuggestion.run( + { + id: suggestion.id, + decision: "accepted", + idempotencyKey: "decision-1", + observedBase: suggestion.baseRevision, + }, + { userEmail: "editor@example.com" }, + ), + ).rejects.toThrow("Not allowed to access doc:doc-1"); + expectNoReviewWrites(); + }); + + it("returns the recorded same-key decision when it loses the status CAS", async () => { + const decided = { ...suggestion, status: "accepted" as const }; + const decision = { + id: "decision-1", + suggestionId: suggestion.id, + idempotencyKey: "decision-1", + reviewer: "editor@example.com", + decision: "accepted" as const, + observedBase: suggestion.baseRevision, + outcome: "accepted", + detail: null, + createdAt: "now", + }; + registerReviewableResource({ + type: "doc", + resolveAccess: () => ({ + role: "editor", + ownerEmail: "owner@example.com", + visibility: "private", + }), + }); + vi.mocked(suggestionStore.getSuggestion) + .mockResolvedValueOnce(suggestion) + .mockResolvedValueOnce(suggestion) + .mockResolvedValueOnce(decided); + updateSuggestionStatus.mockResolvedValueOnce(false); + vi.mocked(suggestionStore.getDecision).mockResolvedValueOnce(decision); + + await expect( + decideResourceSuggestion.run( + { + id: suggestion.id, + decision: "accepted", + idempotencyKey: decision.idempotencyKey, + observedBase: suggestion.baseRevision, + observedRevision: suggestion.revision, + }, + { userEmail: decision.reviewer }, + ), + ).resolves.toEqual({ suggestion: decided, decision }); + expect(suggestionStore.recordDecision).not.toHaveBeenCalled(); + expect(applySuggestion).not.toHaveBeenCalled(); + }); + + it("returns the recorded same-key stale decision when it loses the status CAS", async () => { + const stale = { ...suggestion, status: "stale" as const }; + const decision = { + id: "decision-stale", + suggestionId: suggestion.id, + idempotencyKey: "decision-stale", + reviewer: "editor@example.com", + decision: "accepted" as const, + observedBase: "revision-before-refresh", + outcome: "stale", + detail: "Base revision changed", + createdAt: "now", + }; + registerReviewableResource({ + type: "doc", + resolveAccess: () => ({ + role: "editor", + ownerEmail: "owner@example.com", + visibility: "private", + }), + }); + vi.mocked(suggestionStore.getSuggestion) + .mockResolvedValueOnce(suggestion) + .mockResolvedValueOnce(suggestion) + .mockResolvedValueOnce(stale); + updateSuggestionStatus.mockResolvedValueOnce(false); + vi.mocked(suggestionStore.getDecision).mockResolvedValueOnce(decision); + + await expect( + decideResourceSuggestion.run( + { + id: suggestion.id, + decision: "accepted", + idempotencyKey: decision.idempotencyKey, + observedBase: decision.observedBase, + observedRevision: suggestion.revision, + }, + { userEmail: decision.reviewer }, + ), + ).resolves.toEqual({ suggestion: stale, decision }); + expect(suggestionStore.recordDecision).not.toHaveBeenCalled(); + expect(applySuggestion).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/review/suggestions/actions.ts b/packages/core/src/review/suggestions/actions.ts new file mode 100644 index 00000000000..fbda6c4541c --- /dev/null +++ b/packages/core/src/review/suggestions/actions.ts @@ -0,0 +1,641 @@ +import { z } from "zod"; + +import { defineAction, fail } from "../../action.js"; +import { getDbExec, type DbExec } from "../../db/client.js"; +import { notifyReviewComment } from "../notifications.js"; +import { assertReviewableResourceAccess } from "../registry.js"; +import { + ensureReviewTables, + insertReviewCommentWithClient, + resolveReviewThreadWithClient, +} from "../store.js"; +import { getSuggestionAdapter } from "./registry.js"; +import { + getSuggestion, + ensureSuggestionTables, + insertSuggestion, + listSuggestions, + recordDecision, + getDecision, + getSuggestionByCreationKey, + recordSuggestionCreation, + replaceSuggestionStatus, + updateSuggestionStatus, + getSuggestionAmendment, + amendSuggestion, + deleteUnclaimedSuggestion, + type SuggestionCreationReceipt, +} from "./store.js"; +import type { ResourceSuggestion } from "./types.js"; + +const base = { resourceType: z.string().min(1), resourceId: z.string().min(1) }; +const operation = z.object({ + ordinal: z.number().int().nonnegative(), + kind: z.string().min(1), + targetId: z.string().nullable().optional(), + before: z.unknown().optional(), + after: z.unknown().optional(), + anchor: z.unknown().optional(), + dependencies: z.unknown().optional(), + schemaVersion: z.number().int().positive(), +}); + +function stableJson(value: unknown): string | undefined { + if (value === undefined) return undefined; + if (value === null || typeof value !== "object") return JSON.stringify(value); + if (Array.isArray(value)) { + return `[${value.map((item) => stableJson(item) ?? "null").join(",")}]`; + } + return `{${Object.entries(value as Record) + .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0)) + .flatMap(([key, item]) => { + const encoded = stableJson(item); + return encoded === undefined ? [] : [`${JSON.stringify(key)}:${encoded}`]; + }) + .join(",")}}`; +} + +async function creationRequestHash(args: { + resourceType: string; + resourceId: string; + adapterKind: string; + baseRevision: string; + summary: string; + operations: unknown[]; + metadata?: Record; +}): Promise { + const request = stableJson({ + resourceType: args.resourceType, + resourceId: args.resourceId, + adapterKind: args.adapterKind, + baseRevision: args.baseRevision, + summary: args.summary, + operations: args.operations, + metadata: args.metadata ?? null, + })!; + const digest = await globalThis.crypto.subtle.digest( + "SHA-256", + new TextEncoder().encode(request), + ); + return Array.from(new Uint8Array(digest), (byte) => + byte.toString(16).padStart(2, "0"), + ).join(""); +} + +function assertCreationReplay( + receipt: SuggestionCreationReceipt, + requestHash: string, + authorEmail: string | null, + actorKind: ResourceSuggestion["actorKind"], +): ResourceSuggestion { + if ( + receipt.requestHash !== requestHash || + receipt.authorEmail !== authorEmail || + receipt.actorKind !== actorKind + ) { + throw new Error( + "Idempotency key was already used for a different suggestion", + ); + } + return receipt.suggestion; +} + +export const createResourceSuggestion = defineAction({ + description: + "Create a typed pending suggestion without changing the canonical resource.", + schema: z.object({ + ...base, + adapterKind: z.string().min(1), + baseRevision: z.string().min(1), + summary: z.string().trim().min(1).max(500), + idempotencyKey: z.string().min(1).max(200), + operations: z.array(operation).min(1), + metadata: z.record(z.string(), z.unknown()).optional(), + }), + link: ({ args, result }) => { + const suggestion = result as ResourceSuggestion; + const url = getSuggestionAdapter(args.adapterKind)?.buildUrl?.( + args.resourceId, + suggestion.id, + ); + return url ? { url, label: "Open suggestion" } : null; + }, + run: async (args, ctx) => { + const access = await assertReviewableResourceAccess( + args.resourceType, + args.resourceId, + ctx as any, + "commenter", + ); + const adapter = getSuggestionAdapter(args.adapterKind); + if (!adapter) throw new Error("Suggestion adapter not registered"); + const actorKind = + (ctx as any)?.caller === "agent" || (ctx as any)?.caller === "tool" + ? "agent" + : (ctx as any)?.userEmail + ? "human" + : "system"; + const authorEmail = (ctx as any)?.userEmail ?? null; + const requestHash = await creationRequestHash(args); + const db = getDbExec(); + await ensureSuggestionTables(); + await ensureReviewTables(); + if (!db.transaction) + throw new Error( + "Suggestion creation requires an atomic database transaction", + ); + const result = await db.transaction(async (tx) => { + const prior = await getSuggestionByCreationKey(tx, args.idempotencyKey); + if (prior) { + return { + suggestion: assertCreationReplay( + prior, + requestHash, + authorEmail, + actorKind, + ), + threadComment: null, + }; + } + const operations = + (await adapter.validateProposal({ + ...args, + ctx: { + ...(ctx as any), + suggestionAccess: access, + transaction: tx, + }, + })) ?? args.operations; + const created = await insertSuggestion( + { + resourceType: args.resourceType, + resourceId: args.resourceId, + adapterKind: adapter.kind, + adapterVersion: adapter.version, + threadId: `suggestion-thread-${globalThis.crypto.randomUUID()}`, + authorEmail, + actorKind, + baseRevision: args.baseRevision, + status: "pending", + summary: args.summary, + ownerEmail: access.ownerEmail ?? null, + orgId: access.orgId ?? null, + visibility: access.visibility ?? "private", + metadata: args.metadata ?? null, + operations, + }, + tx, + ); + const receipt = await recordSuggestionCreation( + tx, + args.idempotencyKey, + created, + authorEmail, + actorKind, + requestHash, + ); + if (receipt.suggestion.id !== created.id) { + await deleteUnclaimedSuggestion(tx, created.id); + return { + suggestion: assertCreationReplay( + receipt, + requestHash, + authorEmail, + actorKind, + ), + threadComment: null, + }; + } + const threadComment = await insertReviewCommentWithClient( + { + resourceType: created.resourceType, + resourceId: created.resourceId, + threadId: created.threadId, + targetId: created.id, + kind: "correction", + anchor: created.operations.map((item) => item.anchor ?? null), + body: created.summary, + authorEmail: created.authorEmail, + createdBy: actorKind, + resolutionTarget: "human", + ownerEmail: created.ownerEmail, + orgId: created.orgId, + visibility: created.visibility, + metadata: { suggestionId: created.id }, + }, + tx, + ); + return { suggestion: created, threadComment }; + }); + if (result.threadComment) await notifyReviewComment(result.threadComment); + return result.suggestion; + }, + audit: { + target: (args, result) => { + const suggestion = result as { + ownerEmail?: string | null; + orgId?: string | null; + visibility?: "private" | "org" | "public"; + }; + return { + type: args.resourceType, + id: args.resourceId, + ownerEmail: suggestion.ownerEmail, + orgId: suggestion.orgId, + visibility: suggestion.visibility, + }; + }, + }, +}); + +export const updateResourceSuggestion = defineAction({ + description: + "Amend your pending suggestion while preserving its discussion and history; returns the updated suggestion and revision.", + schema: z.object({ + id: z.string().min(1).describe("The existing pending suggestion to amend."), + observedRevision: z + .number() + .int() + .positive() + .describe( + "The suggestion revision you read; refresh after a conflict before editing again.", + ), + idempotencyKey: z + .string() + .min(1) + .max(200) + .describe( + "Reuse this key only for an exact retry of this amendment; use a new key for new edits.", + ), + operations: z + .array(operation) + .min(1) + .describe( + "Replacement proposal operations against the suggestion's existing canonical basis.", + ), + summary: z + .string() + .trim() + .min(1) + .max(500) + .optional() + .describe( + "Optional replacement summary; omission preserves the existing summary.", + ), + }), + run: async (args, ctx) => { + const initial = await getSuggestion(args.id); + if (!initial) + fail("Suggestion not found", { statusCode: 404, errorCode: "not_found" }); + await assertReviewableResourceAccess( + initial.resourceType, + initial.resourceId, + ctx as any, + "commenter", + ); + const author = (ctx as any)?.userEmail; + if (!author || author !== initial.authorEmail) { + fail("Only the author can amend this suggestion", { + statusCode: 403, + errorCode: "forbidden", + }); + } + const db = getDbExec(); + if (!db.transaction) + throw new Error( + "Suggestion amendments require an atomic database transaction", + ); + const request = JSON.stringify({ + id: args.id, + observedRevision: args.observedRevision, + operations: args.operations, + summary: args.summary ?? null, + }); + return db.transaction(async (tx) => { + const current = await getSuggestion(args.id, tx); + if (!current) + fail("Suggestion not found", { + statusCode: 404, + errorCode: "not_found", + }); + const access = await assertReviewableResourceAccess( + current.resourceType, + current.resourceId, + { ...(ctx as any), transaction: tx }, + "commenter", + ); + if (current.authorEmail !== author) + fail("Only the author can amend this suggestion", { + statusCode: 403, + errorCode: "forbidden", + }); + const prior = await getSuggestionAmendment(tx, args.idempotencyKey); + if (prior) { + if (prior.suggestionId !== current.id || prior.request !== request) { + fail("Idempotency key was already used for a different amendment", { + statusCode: 409, + errorCode: "idempotency_conflict", + }); + } + return prior.suggestion; + } + if ( + current.status !== "pending" || + current.revision !== args.observedRevision + ) { + fail("The suggestion changed; refresh before editing", { + statusCode: 409, + errorCode: "suggestion_conflict", + }); + } + const adapter = getSuggestionAdapter(current.adapterKind); + if (!adapter || adapter.version !== current.adapterVersion) + throw new Error("Suggestion adapter version is unavailable"); + const operations = + (await adapter.validateProposal({ + resourceType: current.resourceType, + resourceId: current.resourceId, + baseRevision: current.baseRevision, + operations: args.operations, + ctx: { ...(ctx as any), transaction: tx, suggestionAccess: access }, + })) ?? args.operations; + const updated = await amendSuggestion( + tx, + current, + operations, + args.summary ?? current.summary, + args.idempotencyKey, + request, + ); + if (!updated) + fail("The suggestion changed; refresh before editing", { + statusCode: 409, + errorCode: "suggestion_conflict", + }); + return updated; + }); + }, + audit: { + target: (_args, result) => { + const suggestion = result as ResourceSuggestion; + return { + type: suggestion.resourceType, + id: suggestion.resourceId, + ownerEmail: suggestion.ownerEmail, + orgId: suggestion.orgId, + visibility: suggestion.visibility, + }; + }, + }, +}); + +export const listResourceSuggestions = defineAction({ + description: "List typed suggestions for a resource.", + schema: z.object({ + ...base, + statuses: z + .array(z.enum(["pending", "accepted", "rejected", "stale", "superseded"])) + .optional(), + }), + http: { method: "GET" }, + readOnly: true, + parallelSafe: true, + run: async (args, ctx) => { + await assertReviewableResourceAccess( + args.resourceType, + args.resourceId, + ctx as any, + "viewer", + ); + return { + suggestions: await listSuggestions( + args.resourceType, + args.resourceId, + args.statuses, + ), + }; + }, +}); +export const getResourceSuggestion = defineAction({ + description: "Get one typed suggestion and its operations.", + schema: z.object({ id: z.string().min(1) }), + readOnly: true, + link: ({ result }) => { + const suggestion = result as ResourceSuggestion | null; + if (!suggestion) return null; + const url = getSuggestionAdapter(suggestion.adapterKind)?.buildUrl?.( + suggestion.resourceId, + suggestion.id, + ); + return url ? { url, label: "Open suggestion" } : null; + }, + run: async (args, ctx) => { + const suggestion = await getSuggestion(args.id); + if (!suggestion) throw new Error("Suggestion not found"); + await assertReviewableResourceAccess( + suggestion.resourceType, + suggestion.resourceId, + ctx as any, + "viewer", + ); + return suggestion; + }, +}); + +export const decideResourceSuggestion = defineAction({ + description: "Accept or reject a pending suggestion atomically.", + schema: z.object({ + id: z.string().min(1), + decision: z.enum(["accepted", "rejected"]), + idempotencyKey: z.string().min(1), + observedBase: z.string().min(1), + observedRevision: z.number().int().positive().optional(), + }), + run: async (args, ctx) => { + const suggestion = await getSuggestion(args.id); + if (!suggestion) throw new Error("Suggestion not found"); + const access = await assertReviewableResourceAccess( + suggestion.resourceType, + suggestion.resourceId, + ctx as any, + "editor", + ); + const db = getDbExec(); + if (!db.transaction) + throw new Error( + "Suggestion decisions require an atomic database transaction", + ); + const adapter = getSuggestionAdapter(suggestion.adapterKind); + if (!adapter || adapter.version !== suggestion.adapterVersion) + throw new Error("Suggestion adapter version is unavailable"); + const reviewer = (ctx as any)?.userEmail ?? null; + const observedRevision = args.observedRevision ?? 1; + const replayDecision = async (tx: DbExec) => { + const decision = await getDecision(tx, args.idempotencyKey); + const latest = await getSuggestion(args.id, tx); + if ( + !decision || + !latest || + decision.suggestionId !== args.id || + decision.reviewer !== reviewer || + decision.decision !== args.decision || + decision.observedBase !== args.observedBase || + latest.revision !== observedRevision + ) { + fail("The suggestion changed; refresh before deciding", { + statusCode: 409, + errorCode: "suggestion_conflict", + }); + } + return { suggestion: latest, decision }; + }; + const decide = (coordination?: unknown) => + db.transaction!(async (tx) => { + const current = await getSuggestion(args.id, tx); + if (!current) throw new Error("Suggestion not found"); + const decisionAccess = await assertReviewableResourceAccess( + current.resourceType, + current.resourceId, + { ...(ctx as any), transaction: tx }, + "editor", + ); + if (current.status !== "pending") { + return replayDecision(tx); + } + const currentAdapter = getSuggestionAdapter(current.adapterKind); + if (observedRevision !== current.revision) { + fail("The suggestion changed; refresh before deciding", { + statusCode: 409, + errorCode: "suggestion_conflict", + }); + } + if ( + !currentAdapter || + currentAdapter.version !== current.adapterVersion + ) + throw new Error("Suggestion adapter version is unavailable"); + if (current.baseRevision !== args.observedBase) { + if ( + !(await updateSuggestionStatus( + tx, + current.id, + "stale", + current.revision, + )) + ) { + return replayDecision(tx); + } + const decision = await recordDecision(tx, { + suggestionId: current.id, + idempotencyKey: args.idempotencyKey, + reviewer: (ctx as any)?.userEmail ?? null, + decision: args.decision, + observedBase: args.observedBase, + outcome: "stale", + detail: "Base revision changed", + }); + return { + suggestion: await getSuggestion(current.id, tx), + decision: decision.record, + }; + } + const claimed = await updateSuggestionStatus( + tx, + current.id, + args.decision, + current.revision, + ); + if (!claimed) return replayDecision(tx); + const prior = await recordDecision(tx, { + suggestionId: current.id, + idempotencyKey: args.idempotencyKey, + reviewer: (ctx as any)?.userEmail ?? null, + decision: args.decision, + observedBase: args.observedBase, + outcome: args.decision, + detail: null, + }); + if (!prior.duplicate && args.decision === "accepted") { + try { + await currentAdapter.apply({ + resourceType: current.resourceType, + resourceId: current.resourceId, + suggestion: current, + operations: current.operations, + access: decisionAccess, + ctx: { + ...(ctx as any), + suggestionAccess: decisionAccess, + transaction: tx, + }, + transaction: tx, + coordination, + }); + } catch (error) { + if ( + !(error instanceof Error) || + error.name !== "SuggestionStaleError" + ) { + throw error; + } + await replaceSuggestionStatus(tx, current.id, "accepted", "stale"); + await tx.execute({ + sql: "UPDATE agent_review_suggestion_decisions SET outcome = ?, detail = ? WHERE id = ?", + args: ["stale", error.message, prior.record.id], + }); + return { + suggestion: await getSuggestion(current.id, tx), + decision: { + ...prior.record, + outcome: "stale", + detail: error.message, + }, + }; + } + } + if (!prior.duplicate) { + await resolveReviewThreadWithClient( + tx, + current.threadId, + (ctx as any)?.userEmail ?? null, + { + resourceType: current.resourceType, + resourceId: current.resourceId, + }, + args.decision, + ); + } + return { + suggestion: await getSuggestion(current.id, tx), + decision: prior.record, + }; + }); + const decisionContext = { + resourceType: suggestion.resourceType, + resourceId: suggestion.resourceId, + suggestion, + operations: suggestion.operations, + decision: args.decision, + access, + ctx: { ...(ctx as any), suggestionAccess: access }, + }; + return adapter.coordinateDecision + ? adapter.coordinateDecision(decisionContext, decide) + : decide(); + }, + audit: { + target: (_args, result) => { + const suggestion = (result as { suggestion?: ResourceSuggestion }) + .suggestion; + return suggestion + ? { + type: suggestion.resourceType, + id: suggestion.resourceId, + ownerEmail: suggestion.ownerEmail, + orgId: suggestion.orgId, + visibility: suggestion.visibility, + } + : undefined; + }, + }, +}); diff --git a/packages/core/src/review/suggestions/actions/create-resource-suggestion.ts b/packages/core/src/review/suggestions/actions/create-resource-suggestion.ts new file mode 100644 index 00000000000..3409207d231 --- /dev/null +++ b/packages/core/src/review/suggestions/actions/create-resource-suggestion.ts @@ -0,0 +1 @@ +export { createResourceSuggestion as default } from "../actions.js"; diff --git a/packages/core/src/review/suggestions/actions/decide-resource-suggestion.ts b/packages/core/src/review/suggestions/actions/decide-resource-suggestion.ts new file mode 100644 index 00000000000..191ccd85325 --- /dev/null +++ b/packages/core/src/review/suggestions/actions/decide-resource-suggestion.ts @@ -0,0 +1 @@ +export { decideResourceSuggestion as default } from "../actions.js"; diff --git a/packages/core/src/review/suggestions/actions/get-resource-suggestion.ts b/packages/core/src/review/suggestions/actions/get-resource-suggestion.ts new file mode 100644 index 00000000000..17975653750 --- /dev/null +++ b/packages/core/src/review/suggestions/actions/get-resource-suggestion.ts @@ -0,0 +1 @@ +export { getResourceSuggestion as default } from "../actions.js"; diff --git a/packages/core/src/review/suggestions/actions/list-resource-suggestions.ts b/packages/core/src/review/suggestions/actions/list-resource-suggestions.ts new file mode 100644 index 00000000000..9b61cc96126 --- /dev/null +++ b/packages/core/src/review/suggestions/actions/list-resource-suggestions.ts @@ -0,0 +1 @@ +export { listResourceSuggestions as default } from "../actions.js"; diff --git a/packages/core/src/review/suggestions/actions/update-resource-suggestion.ts b/packages/core/src/review/suggestions/actions/update-resource-suggestion.ts new file mode 100644 index 00000000000..8a41e79f1ee --- /dev/null +++ b/packages/core/src/review/suggestions/actions/update-resource-suggestion.ts @@ -0,0 +1 @@ +export { updateResourceSuggestion as default } from "../actions.js"; diff --git a/packages/core/src/review/suggestions/amendments.spec.ts b/packages/core/src/review/suggestions/amendments.spec.ts new file mode 100644 index 00000000000..65fa2a4ed03 --- /dev/null +++ b/packages/core/src/review/suggestions/amendments.spec.ts @@ -0,0 +1,329 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { createTestPglite } from "../../a2a/test-pglite.js"; + +let pglite: Awaited>; +const client = { + execute: async (input: string | { sql: string; args?: unknown[] }) => { + if (typeof input === "string") { + await pglite.exec(input); + return { rows: [], rowsAffected: 0 }; + } + const result = await pglite.query(input.sql, input.args ?? []); + return { + rows: Array.from(result.rows ?? []), + rowsAffected: result.affectedRows ?? result.rowCount ?? 0, + }; + }, + transaction: async ( + run: (tx: typeof client) => Promise, + ): Promise => { + await pglite.exec("BEGIN"); + try { + const result = await run(client); + await pglite.exec("COMMIT"); + return result; + } catch (error) { + await pglite.exec("ROLLBACK"); + throw error; + } + }, +}; +const access = vi.fn(async () => ({ + role: "commenter", + ownerEmail: "owner@example.com", +})); +const validate = vi.fn(); +const apply = vi.fn(); +vi.mock("../../db/client.js", () => ({ + getDbExec: () => client, + isProductionServerlessFunctionRuntime: () => false, +})); +vi.mock("../registry.js", () => ({ + assertReviewableResourceAccess: (...args: unknown[]) => access(...args), +})); +vi.mock("../notifications.js", () => ({ notifyReviewComment: vi.fn() })); +vi.mock("../store.js", () => ({ + ensureReviewTables: vi.fn(), + insertReviewCommentWithClient: vi.fn(), + resolveReviewThreadWithClient: vi.fn(), +})); +vi.mock("./registry.js", () => ({ + getSuggestionAdapter: () => ({ + kind: "test", + version: 1, + validateProposal: validate, + apply, + }), +})); + +const { updateResourceSuggestion, decideResourceSuggestion } = + await import("./actions.js"); +const { + insertSuggestion, + getSuggestion, + ensureSuggestionTables, + __resetSuggestionTablesForTests, + amendSuggestion, + updateSuggestionStatus, +} = await import("./store.js"); +const operation = { + ordinal: 0, + kind: "replace_text", + before: "old", + after: "new", + schemaVersion: 1, +}; +let suggestion: Awaited>; +const author = { userEmail: "author@example.com" }; +const request = () => ({ + id: suggestion.id, + observedRevision: 1, + idempotencyKey: "amend-1", + operations: [{ ...operation, after: "better" }], +}); +const decide = (observedRevision?: number) => + decideResourceSuggestion.run( + { + id: suggestion.id, + decision: "accepted", + observedBase: "base-1", + observedRevision, + idempotencyKey: "decision-1", + }, + author, + ); + +beforeEach(async () => { + pglite = await createTestPglite(); + __resetSuggestionTablesForTests(); + access + .mockReset() + .mockResolvedValue({ role: "commenter", ownerEmail: "owner@example.com" }); + validate.mockReset(); + apply.mockReset(); + await ensureSuggestionTables(); + suggestion = await insertSuggestion({ + resourceType: "document", + resourceId: "doc-1", + adapterKind: "test", + adapterVersion: 1, + threadId: "thread-1", + authorEmail: author.userEmail, + actorKind: "human", + baseRevision: "base-1", + status: "pending", + summary: "Original", + ownerEmail: "owner@example.com", + orgId: null, + visibility: "private", + metadata: null, + operations: [operation], + }); +}); +afterEach(async () => { + await pglite.close(); +}); + +describe("pending suggestion amendments", () => { + it("reuses additive schema without resetting existing proposal revisions", async () => { + await updateResourceSuggestion.run(request(), author); + __resetSuggestionTablesForTests(); + await ensureSuggestionTables(); + expect((await getSuggestion(suggestion.id))?.revision).toBe(2); + }); + + it("adds ownership columns to a preexisting amendment table without removing rows", async () => { + await pglite.close(); + pglite = await createTestPglite(); + await pglite.exec( + "CREATE TABLE agent_review_suggestion_amendments (idempotency_key TEXT PRIMARY KEY, suggestion_id TEXT NOT NULL, revision INTEGER NOT NULL, author_email TEXT NOT NULL, request_json TEXT NOT NULL, before_json TEXT NOT NULL, after_json TEXT NOT NULL, created_at TEXT NOT NULL, UNIQUE (suggestion_id, revision))", + ); + await pglite.query( + "INSERT INTO agent_review_suggestion_amendments VALUES (?,?,?,?,?,?,?,?)", + [ + "existing", + "existing-suggestion", + 2, + "author@example.com", + "{}", + "{}", + "{}", + "now", + ], + ); + __resetSuggestionTablesForTests(); + await ensureSuggestionTables(); + const row = ( + await pglite.query( + "SELECT * FROM agent_review_suggestion_amendments WHERE idempotency_key = ?", + ["existing"], + ) + ).rows[0]; + expect(row).toMatchObject({ + suggestion_id: "existing-suggestion", + revision: 2, + owner_email: null, + org_id: null, + visibility: "private", + }); + }); + it("preserves identity, increments revision and keeps append-only before/after history without applying canonical content", async () => { + const first = await updateResourceSuggestion.run(request(), author); + const second = await updateResourceSuggestion.run( + { + ...request(), + observedRevision: 2, + idempotencyKey: "amend-2", + operations: [{ ...operation, after: "best" }], + }, + author, + ); + expect(second).toMatchObject({ + id: suggestion.id, + threadId: suggestion.threadId, + authorEmail: suggestion.authorEmail, + createdAt: suggestion.createdAt, + revision: 3, + status: "pending", + }); + const rows = ( + await pglite.query( + "SELECT before_json, after_json FROM agent_review_suggestion_amendments ORDER BY revision", + ) + ).rows as { before_json: string; after_json: string }[]; + expect(rows).toHaveLength(2); + expect(JSON.parse(rows[0]!.before_json).operations[0].after).toBe("new"); + expect(JSON.parse(rows[0]!.after_json)).toEqual(first); + expect(JSON.parse(rows[1]!.after_json)).toEqual(second); + expect(apply).not.toHaveBeenCalled(); + expect(validate).toHaveBeenCalledWith( + expect.objectContaining({ + baseRevision: "base-1", + ctx: expect.objectContaining({ transaction: client }), + }), + ); + }); + + it("replays an exact retry without adding history and rejects mismatched key reuse", async () => { + const first = await updateResourceSuggestion.run(request(), author); + expect(await updateResourceSuggestion.run(request(), author)).toEqual( + first, + ); + await expect( + updateResourceSuggestion.run( + { ...request(), summary: "Changed" }, + author, + ), + ).rejects.toMatchObject({ errorCode: "idempotency_conflict" }); + expect( + ( + await pglite.query( + "SELECT COUNT(*) AS count FROM agent_review_suggestion_amendments", + ) + ).rows[0], + ).toEqual({ count: 1 }); + }); + + it.each([{}, { userEmail: "owner@example.com" }])( + "rejects a caller who is not the exact author: %s", + async (caller) => { + await expect( + updateResourceSuggestion.run(request(), caller), + ).rejects.toMatchObject({ errorCode: "forbidden" }); + expect((await getSuggestion(suggestion.id))?.revision).toBe(1); + }, + ); + + it("rechecks revoked access inside the transaction", async () => { + access + .mockResolvedValueOnce({ + role: "commenter", + ownerEmail: "owner@example.com", + }) + .mockRejectedValueOnce(new Error("Access revoked")); + await expect( + updateResourceSuggestion.run(request(), author), + ).rejects.toThrow("Access revoked"); + expect((await getSuggestion(suggestion.id))?.revision).toBe(1); + }); + + it("rejects a stale proposal revision without replacing content", async () => { + await updateResourceSuggestion.run(request(), author); + await expect( + updateResourceSuggestion.run( + { ...request(), idempotencyKey: "other" }, + author, + ), + ).rejects.toMatchObject({ errorCode: "suggestion_conflict" }); + expect((await getSuggestion(suggestion.id))?.operations[0]?.after).toBe( + "better", + ); + }); + + it("rejects changed canonical basis through adapter validation without an amendment", async () => { + validate.mockRejectedValueOnce(new Error("Canonical changed")); + await expect( + updateResourceSuggestion.run(request(), author), + ).rejects.toThrow("Canonical changed"); + expect((await getSuggestion(suggestion.id))?.revision).toBe(1); + expect( + ( + await pglite.query( + "SELECT COUNT(*) AS count FROM agent_review_suggestion_amendments", + ) + ).rows[0], + ).toEqual({ count: 0 }); + }); + + it("rejects missing/stale decision revision after amendment and accepts the observed revision", async () => { + await updateResourceSuggestion.run(request(), author); + await expect(decide()).rejects.toMatchObject({ + errorCode: "suggestion_conflict", + }); + await expect(decide(1)).rejects.toMatchObject({ + errorCode: "suggestion_conflict", + }); + expect(apply).not.toHaveBeenCalled(); + await decide(2); + expect(apply).toHaveBeenCalledOnce(); + expect((await getSuggestion(suggestion.id))?.status).toBe("accepted"); + }); + + it("refuses an amendment after acceptance, while legacy revision-one decisions still work", async () => { + await decide(); + await expect( + updateResourceSuggestion.run(request(), author), + ).rejects.toMatchObject({ errorCode: "suggestion_conflict" }); + expect((await getSuggestion(suggestion.id))?.revision).toBe(1); + }); + + it("CAS refuses a decision based on an amendment's previous revision", async () => { + await updateResourceSuggestion.run(request(), author); + expect( + await updateSuggestionStatus(client, suggestion.id, "accepted", 1), + ).toBe(false); + expect((await getSuggestion(suggestion.id))?.status).toBe("pending"); + }); + + it("CAS refuses amendment if a decision wins after the author reads", async () => { + await decide(); + expect( + await client.transaction((tx) => + amendSuggestion(tx, suggestion, [operation], "Other", "late", "{}"), + ), + ).toBeNull(); + expect((await getSuggestion(suggestion.id))?.status).toBe("accepted"); + }); + + it("rolls the payload and revision back when the history receipt cannot be written", async () => { + await updateResourceSuggestion.run(request(), author); + const current = (await getSuggestion(suggestion.id))!; + await expect( + client.transaction((tx) => + amendSuggestion(tx, current, [operation], "Bad", "amend-1", "{}"), + ), + ).rejects.toThrow(); + expect(await getSuggestion(suggestion.id)).toEqual(current); + }); +}); diff --git a/packages/core/src/review/suggestions/pglite-transaction.integration.spec.ts b/packages/core/src/review/suggestions/pglite-transaction.integration.spec.ts new file mode 100644 index 00000000000..0cbc9014b06 --- /dev/null +++ b/packages/core/src/review/suggestions/pglite-transaction.integration.spec.ts @@ -0,0 +1,423 @@ +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { eq } from "drizzle-orm"; +import { pgTable, text } from "drizzle-orm/pg-core"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { closeDbExec, createDbExec, getDbExec } from "../../db/client.js"; +import { createGetDb } from "../../db/create-get-db.js"; +import { resolveAccess } from "../../sharing/access.js"; +import { registerShareableResource } from "../../sharing/registry.js"; +import { createSharesTable, ownableColumns } from "../../sharing/schema.js"; +import { + __resetReviewInitForTests, + ensureReviewTables, + insertReviewComment, + queryReviewComments, +} from "../store.js"; +import { + decideResourceSuggestion, + updateResourceSuggestion, +} from "./actions.js"; +import { + __resetSuggestionAdaptersForTests, + registerSuggestionAdapter, +} from "./registry.js"; +import { + __resetSuggestionTablesForTests, + ensureSuggestionTables, + insertSuggestion, + listSuggestions, +} from "./store.js"; + +const resources = pgTable("pglite_review_resources", { + id: text("id").primaryKey(), + body: text("body").notNull(), + ...ownableColumns(), +}); +const resourceShares = createSharesTable("pglite_review_resource_shares"); +const getDb = createGetDb({ resources, resourceShares }); + +const resourceType = "pglite-review-transaction-resource"; +const resourceId = "resource-1"; +const ownerEmail = "owner@example.com"; +const baseRevision = "revision-1"; +const originalOperation = { + ordinal: 0, + kind: "replace_text", + targetId: "body", + before: { markdown: "Before" }, + after: { markdown: "After" }, + schemaVersion: 1, +}; + +let previousDatabaseUrl: string | undefined; +let independentClientsDirectory: string | undefined; + +beforeAll(async () => { + previousDatabaseUrl = process.env.DATABASE_URL; + process.env.DATABASE_URL = "pglite:memory"; + await closeDbExec(); + + const db = getDbExec(); + await db.execute(`CREATE TABLE pglite_review_resources ( + id TEXT PRIMARY KEY, + body TEXT NOT NULL, + owner_email TEXT, + org_id TEXT, + visibility TEXT NOT NULL DEFAULT 'private' + )`); + await db.execute(`CREATE TABLE pglite_review_resource_shares ( + id TEXT PRIMARY KEY, + resource_id TEXT NOT NULL, + principal_type TEXT NOT NULL, + principal_id TEXT NOT NULL, + role TEXT NOT NULL, + created_at TEXT NOT NULL + )`); + await db.execute({ + sql: "INSERT INTO pglite_review_resources (id,body,owner_email,org_id,visibility) VALUES (?,?,?,?,?)", + args: [resourceId, "Before", ownerEmail, null, "private"], + }); + + registerShareableResource({ + type: resourceType, + resourceTable: resources, + sharesTable: resourceShares, + displayName: "PGlite review transaction resource", + getDb, + }); + __resetSuggestionAdaptersForTests(); + registerSuggestionAdapter({ + kind: "pglite-review-transaction-adapter", + version: 1, + validateProposal: ({ operations }) => operations, + apply: async ({ operations, resourceId: targetId, transaction }) => { + const after = operations[0]?.after as { markdown?: unknown } | undefined; + if (typeof after?.markdown !== "string") { + throw new Error("Test suggestion is missing after.markdown"); + } + await transaction.execute({ + sql: "UPDATE pglite_review_resources SET body = ? WHERE id = ?", + args: [after.markdown, targetId], + }); + }, + }); + __resetSuggestionTablesForTests(); + __resetReviewInitForTests(); + await ensureSuggestionTables(); + await ensureReviewTables(); +}); + +afterAll(async () => { + await closeDbExec(); + if (independentClientsDirectory) { + await rm(independentClientsDirectory, { recursive: true, force: true }); + } + if (previousDatabaseUrl === undefined) delete process.env.DATABASE_URL; + else process.env.DATABASE_URL = previousDatabaseUrl; +}); + +describe.sequential("suggestion actions on native PGlite transactions", () => { + it("amends, decides, and releases the client for an ordinary read", async () => { + const suggestion = await insertSuggestion({ + resourceType, + resourceId, + adapterKind: "pglite-review-transaction-adapter", + adapterVersion: 1, + threadId: "review-thread-1", + authorEmail: ownerEmail, + actorKind: "human", + baseRevision, + status: "pending", + summary: "Original suggestion", + ownerEmail, + orgId: null, + visibility: "private", + metadata: null, + operations: [originalOperation], + }); + await insertReviewComment({ + resourceType, + resourceId, + threadId: suggestion.threadId, + targetId: suggestion.id, + body: suggestion.summary, + authorEmail: ownerEmail, + ownerEmail, + }); + + const amended = await updateResourceSuggestion.run( + { + id: suggestion.id, + observedRevision: 1, + idempotencyKey: "amend-native-pglite", + summary: "Amended suggestion", + operations: [ + { + ...originalOperation, + after: { markdown: "Amended" }, + }, + ], + }, + { userEmail: ownerEmail }, + ); + expect(amended.revision).toBe(2); + + const decided = await decideResourceSuggestion.run( + { + id: suggestion.id, + decision: "rejected", + idempotencyKey: "decide-native-pglite", + observedBase: baseRevision, + observedRevision: 2, + }, + { userEmail: ownerEmail }, + ); + expect(decided.decision.outcome).toBe("rejected"); + + const ordinaryRead = await listSuggestions(resourceType, resourceId); + expect(ordinaryRead).toHaveLength(1); + expect(ordinaryRead[0]).toMatchObject({ + id: suggestion.id, + revision: 2, + status: "rejected", + summary: "Amended suggestion", + }); + expect( + await queryReviewComments({ + resourceType, + resourceId, + scope: { userEmail: ownerEmail }, + includeResolved: true, + }), + ).toMatchObject([{ threadId: suggestion.threadId, status: "resolved" }]); + }); + + it("keeps an unrelated async read outside a rolling-back transaction", async () => { + let startOutsideRead!: () => void; + const start = new Promise((resolve) => { + startOutsideRead = resolve; + }); + const outsideRead = new Promise((resolve, reject) => { + setTimeout(() => { + void start + .then(async () => { + const [row] = await getDb() + .select({ body: resources.body }) + .from(resources) + .where(eq(resources.id, resourceId)); + resolve(row!.body); + }) + .catch(reject); + }, 0); + }); + + await expect( + getDbExec().transaction!(async (tx) => { + await tx.execute({ + sql: "UPDATE pglite_review_resources SET body = ? WHERE id = ?", + args: ["Uncommitted", resourceId], + }); + startOutsideRead(); + await new Promise((resolve) => setTimeout(resolve, 20)); + throw new Error("roll back test"); + }), + ).rejects.toThrow("roll back test"); + + expect(await outsideRead).toBe("Before"); + }); + + it("reads uncommitted access state on the transaction and rolls it back", async () => { + const temporaryOwner = "temporary-owner@example.com"; + await expect( + getDbExec().transaction!(async (tx) => { + await tx.execute({ + sql: "UPDATE pglite_review_resources SET owner_email = ? WHERE id = ?", + args: [temporaryOwner, resourceId], + }); + await expect( + resolveAccess(resourceType, resourceId, { + userEmail: temporaryOwner, + }), + ).resolves.toMatchObject({ role: "owner" }); + throw new Error("roll back access test"); + }), + ).rejects.toThrow("roll back access test"); + + await expect( + resolveAccess(resourceType, resourceId, { userEmail: ownerEmail }), + ).resolves.toMatchObject({ role: "owner" }); + await expect( + resolveAccess(resourceType, resourceId, { userEmail: temporaryOwner }), + ).resolves.toBeNull(); + }); + + it("routes a fresh global DbExec read through the transaction", async () => { + await expect( + getDbExec().transaction!(async (tx) => { + await tx.execute({ + sql: "UPDATE pglite_review_resources SET body = ? WHERE id = ?", + args: ["Visible only in transaction", resourceId], + }); + const row = ( + await getDbExec().execute({ + sql: "SELECT body FROM pglite_review_resources WHERE id = ?", + args: [resourceId], + }) + ).rows[0]; + expect(row?.body).toBe("Visible only in transaction"); + throw new Error("roll back global DbExec test"); + }), + ).rejects.toThrow("roll back global DbExec test"); + + const row = ( + await getDbExec().execute({ + sql: "SELECT body FROM pglite_review_resources WHERE id = ?", + args: [resourceId], + }) + ).rows[0]; + expect(row?.body).toBe("Before"); + }); + + it("accepts through an adapter that writes on the same transaction", async () => { + const suggestion = await insertSuggestion({ + resourceType, + resourceId, + adapterKind: "pglite-review-transaction-adapter", + adapterVersion: 1, + threadId: "review-thread-accepted", + authorEmail: ownerEmail, + actorKind: "human", + baseRevision, + status: "pending", + summary: "Accepted suggestion", + ownerEmail, + orgId: null, + visibility: "private", + metadata: null, + operations: [originalOperation], + }); + await insertReviewComment({ + resourceType, + resourceId, + threadId: suggestion.threadId, + targetId: suggestion.id, + body: suggestion.summary, + authorEmail: ownerEmail, + ownerEmail, + }); + + const decided = await decideResourceSuggestion.run( + { + id: suggestion.id, + decision: "accepted", + idempotencyKey: "accept-native-pglite", + observedBase: baseRevision, + observedRevision: 1, + }, + { userEmail: ownerEmail }, + ); + expect(decided.decision.outcome).toBe("accepted"); + + const [resource] = await getDb() + .select({ body: resources.body }) + .from(resources) + .where(eq(resources.id, resourceId)); + expect(resource?.body).toBe("After"); + }); + + it("fails nested global transactions explicitly", async () => { + await expect( + getDbExec().transaction!(async () => + getDbExec().transaction!(async () => undefined), + ), + ).rejects.toThrow("Nested PGlite transactions are not supported"); + }); + + it("keeps nested transactions on two PGlite databases isolated", async () => { + independentClientsDirectory = await mkdtemp( + join(tmpdir(), "agent-native-pglite-routing-"), + ); + const firstUrl = `pglite:${join(independentClientsDirectory, "first")}`; + const secondUrl = `pglite:${join(independentClientsDirectory, "second")}`; + const first = await createDbExec({ url: firstUrl }); + const second = await createDbExec({ url: secondUrl }); + await first.execute( + "CREATE TABLE scoped_values (id TEXT PRIMARY KEY, value TEXT NOT NULL)", + ); + await second.execute( + "CREATE TABLE scoped_values (id TEXT PRIMARY KEY, value TEXT NOT NULL)", + ); + await first.execute({ + sql: "INSERT INTO scoped_values (id,value) VALUES (?,?)", + args: ["row", "first-before"], + }); + await second.execute({ + sql: "INSERT INTO scoped_values (id,value) VALUES (?,?)", + args: ["row", "second-before"], + }); + + await expect( + first.transaction!(async (firstTx) => { + await firstTx.execute({ + sql: "UPDATE scoped_values SET value = ? WHERE id = ?", + args: ["first-uncommitted", "row"], + }); + await second.transaction!(async (secondTx) => { + expect( + ( + await first.execute({ + sql: "SELECT value FROM scoped_values WHERE id = ?", + args: ["row"], + }) + ).rows[0]?.value, + ).toBe("first-uncommitted"); + expect( + ( + await secondTx.execute({ + sql: "SELECT value FROM scoped_values WHERE id = ?", + args: ["row"], + }) + ).rows[0]?.value, + ).toBe("second-before"); + await secondTx.execute({ + sql: "UPDATE scoped_values SET value = ? WHERE id = ?", + args: ["second-committed", "row"], + }); + await expect( + first.transaction!(async () => undefined), + ).rejects.toThrow("Nested PGlite transactions are not supported"); + }); + expect( + ( + await first.execute({ + sql: "SELECT value FROM scoped_values WHERE id = ?", + args: ["row"], + }) + ).rows[0]?.value, + ).toBe("first-uncommitted"); + throw new Error("roll back first database"); + }), + ).rejects.toThrow("roll back first database"); + + expect( + ( + await first.execute({ + sql: "SELECT value FROM scoped_values WHERE id = ?", + args: ["row"], + }) + ).rows[0]?.value, + ).toBe("first-before"); + expect( + ( + await second.execute({ + sql: "SELECT value FROM scoped_values WHERE id = ?", + args: ["row"], + }) + ).rows[0]?.value, + ).toBe("second-committed"); + }, 15_000); +}); diff --git a/packages/core/src/review/suggestions/registry.spec.ts b/packages/core/src/review/suggestions/registry.spec.ts new file mode 100644 index 00000000000..9f05611f3bd --- /dev/null +++ b/packages/core/src/review/suggestions/registry.spec.ts @@ -0,0 +1,31 @@ +import { beforeEach, describe, expect, it } from "vitest"; + +import { + __resetSuggestionAdaptersForTests, + getSuggestionAdapter, + registerSuggestionAdapter, +} from "./registry"; + +const adapter = (version: number) => ({ + kind: "document", + version, + validateProposal: () => undefined, + apply: () => undefined, +}); + +describe("suggestion adapter registry", () => { + beforeEach(__resetSuggestionAdaptersForTests); + + it("allows an identical-version startup registration", () => { + registerSuggestionAdapter(adapter(1)); + registerSuggestionAdapter(adapter(1)); + expect(getSuggestionAdapter("document")?.version).toBe(1); + }); + + it("rejects conflicting adapter versions", () => { + registerSuggestionAdapter(adapter(1)); + expect(() => registerSuggestionAdapter(adapter(2))).toThrow( + "conflicting versions", + ); + }); +}); diff --git a/packages/core/src/review/suggestions/registry.ts b/packages/core/src/review/suggestions/registry.ts new file mode 100644 index 00000000000..f4ee363bb4b --- /dev/null +++ b/packages/core/src/review/suggestions/registry.ts @@ -0,0 +1,32 @@ +import type { SuggestionAdapter } from "./types.js"; +const adapters = new Map(); +export function registerSuggestionAdapter(adapter: SuggestionAdapter): void { + if ( + !adapter.kind.trim() || + !Number.isInteger(adapter.version) || + adapter.version < 1 + ) + throw new Error( + "Suggestion adapter requires a non-empty kind and positive version", + ); + const existing = adapters.get(adapter.kind); + if (existing) { + if (existing.version === adapter.version) { + adapters.set(adapter.kind, adapter); + return; + } + throw new Error( + `Suggestion adapter ${adapter.kind} was registered at conflicting versions`, + ); + } + adapters.set(adapter.kind, adapter); +} +export function getSuggestionAdapter(kind: string) { + return adapters.get(kind); +} +export function listSuggestionAdapters() { + return [...adapters.values()]; +} +export function __resetSuggestionAdaptersForTests() { + adapters.clear(); +} diff --git a/packages/core/src/review/suggestions/store.spec.ts b/packages/core/src/review/suggestions/store.spec.ts new file mode 100644 index 00000000000..5cb814ad6c6 --- /dev/null +++ b/packages/core/src/review/suggestions/store.spec.ts @@ -0,0 +1,263 @@ +import { beforeEach, afterEach, describe, expect, it, vi } from "vitest"; + +import { createTestPglite } from "../../a2a/test-pglite.js"; + +let pglite: Awaited>; +const rawClient = { + execute: vi.fn(async (input: string | { sql: string; args?: unknown[] }) => { + if (typeof input === "string") { + await pglite.exec(input); + return { rows: [], rowsAffected: 0 }; + } + const result = await pglite.query(input.sql, input.args ?? []); + return { + rows: Array.from(result.rows ?? []), + rowsAffected: result.affectedRows ?? result.rowCount ?? 0, + }; + }), + transaction: async (fn: (tx: typeof rawClient) => Promise) => { + await pglite.exec("BEGIN"); + try { + const result = await fn(rawClient); + await pglite.exec("COMMIT"); + return result; + } catch (error) { + await pglite.exec("ROLLBACK"); + throw error; + } + }, +}; +vi.mock("../../db/client.js", () => ({ + getDbExec: () => rawClient, + isProductionServerlessFunctionRuntime: () => false, +})); +const { + ensureSuggestionTables, + insertSuggestion, + getSuggestion, + listSuggestions, + getSuggestionByCreationKey, + recordSuggestionCreation, + amendSuggestion, + recordDecision, + __resetSuggestionTablesForTests, +} = await import("./store.js"); + +beforeEach(async () => { + pglite = await createTestPglite(); + __resetSuggestionTablesForTests(); + await ensureSuggestionTables(); +}); +afterEach(async () => { + await pglite.close(); +}); + +const input = { + resourceType: "document", + resourceId: "d1", + adapterKind: "document", + adapterVersion: 1, + threadId: "thread-1", + authorEmail: "alice@example.com", + actorKind: "human" as const, + baseRevision: "rev-1", + status: "pending" as const, + summary: "Replace text", + ownerEmail: "alice@example.com", + orgId: null, + visibility: "private" as const, + metadata: null, + operations: [ + { + ordinal: 0, + kind: "replace_text", + before: "old", + after: "new", + schemaVersion: 1, + }, + ], +}; + +describe("suggestion store", () => { + it("persists operations and rolls back an atomic failed insert", async () => { + const suggestion = await rawClient.transaction((tx) => + insertSuggestion(input, tx), + ); + expect((await getSuggestion(suggestion.id))?.operations[0].after).toBe( + "new", + ); + await expect( + rawClient.transaction(async (tx) => { + await insertSuggestion(input, tx); + throw new Error("rollback"); + }), + ).rejects.toThrow("rollback"); + expect((await getSuggestion(suggestion.id))?.operations).toHaveLength(1); + }); + + it("records idempotent decisions and rejects conflicting key reuse", async () => { + const suggestion = await insertSuggestion(input); + const first = await recordDecision(rawClient, { + suggestionId: suggestion.id, + idempotencyKey: "key-1", + reviewer: "editor@example.com", + decision: "accepted", + observedBase: "rev-1", + outcome: "accepted", + detail: null, + }); + expect(first.duplicate).toBe(false); + expect( + ( + await recordDecision(rawClient, { + suggestionId: suggestion.id, + idempotencyKey: "key-1", + reviewer: "editor@example.com", + decision: "accepted", + observedBase: "rev-1", + outcome: "accepted", + detail: null, + }) + ).duplicate, + ).toBe(true); + await expect( + recordDecision(rawClient, { + suggestionId: "other", + idempotencyKey: "key-1", + reviewer: null, + decision: "rejected", + observedBase: "rev-1", + outcome: "rejected", + detail: null, + }), + ).rejects.toThrow("different decision"); + }); + + it("keeps the original creation result and converges a competing receipt", async () => { + const original = await insertSuggestion(input); + const request = '{"request":"original"}'; + await recordSuggestionCreation( + rawClient, + "create-key", + original, + original.authorEmail, + original.actorKind, + request, + ); + const competing = await insertSuggestion({ + ...input, + threadId: "thread-2", + }); + const winner = await recordSuggestionCreation( + rawClient, + "create-key", + competing, + competing.authorEmail, + competing.actorKind, + request, + ); + expect(winner.suggestion.id).toBe(original.id); + + await rawClient.transaction((tx) => + amendSuggestion( + tx, + original, + [{ ...input.operations[0], after: "amended" }], + "Amended", + "amend-key", + '{"request":"amend"}', + ), + ); + expect( + (await getSuggestionByCreationKey(rawClient, "create-key"))?.suggestion, + ).toEqual(original); + }); + + it("uses the immutable amendment receipt after an interleaved creation replay read", async () => { + const original = await insertSuggestion(input); + const interleavedClient = { + execute: vi + .fn() + .mockResolvedValueOnce({ + rows: [ + { + suggestion_id: original.id, + author_email: original.authorEmail, + actor_kind: original.actorKind, + request_hash: "request-hash", + }, + ], + rowsAffected: 0, + }) + .mockResolvedValueOnce({ + rows: [ + { + id: original.id, + revision: 1, + resource_type: original.resourceType, + resource_id: original.resourceId, + adapter_kind: original.adapterKind, + adapter_version: original.adapterVersion, + thread_id: original.threadId, + author_email: original.authorEmail, + actor_kind: original.actorKind, + base_revision: original.baseRevision, + status: original.status, + summary: original.summary, + owner_email: original.ownerEmail, + org_id: original.orgId, + visibility: original.visibility, + created_at: original.createdAt, + updated_at: original.updatedAt, + metadata_json: null, + }, + ], + rowsAffected: 0, + }) + .mockResolvedValueOnce({ + rows: [ + { + ...original.operations[0], + suggestion_id: original.id, + operation_kind: original.operations[0]?.kind, + after_json: '"interleaved amendment"', + }, + ], + rowsAffected: 0, + }) + .mockResolvedValueOnce({ + rows: [{ before_json: JSON.stringify(original) }], + rowsAffected: 0, + }), + }; + + expect( + (await getSuggestionByCreationKey(interleavedClient, "creation-replay")) + ?.suggestion, + ).toEqual(original); + expect(interleavedClient.execute).toHaveBeenCalledTimes(4); + }); + + it("loads complete suggestion operations in one resource-scoped query", async () => { + await insertSuggestion(input); + await insertSuggestion({ + ...input, + threadId: "thread-2", + summary: "Second suggestion", + operations: [{ ...input.operations[0], ordinal: 1, after: "newer" }], + }); + rawClient.execute.mockClear(); + + const suggestions = await listSuggestions( + input.resourceType, + input.resourceId, + ["pending"], + ); + + expect(suggestions).toHaveLength(2); + expect( + suggestions.map((suggestion) => suggestion.operations[0]?.after), + ).toEqual(["new", "newer"]); + expect(rawClient.execute).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/core/src/review/suggestions/store.ts b/packages/core/src/review/suggestions/store.ts new file mode 100644 index 00000000000..8477b9de452 --- /dev/null +++ b/packages/core/src/review/suggestions/store.ts @@ -0,0 +1,505 @@ +import { getDbExec, type DbExec } from "../../db/client.js"; +import { ensureColumnExists, ensureTableExists } from "../../db/ddl-guard.js"; +import type { Visibility } from "../../sharing/schema.js"; +import type { + ResourceSuggestion, + SuggestionDecision, + SuggestionStatus, + SuggestionOperation, +} from "./types.js"; + +let initialized: Promise | undefined; +export function __resetSuggestionTablesForTests(): void { + initialized = undefined; +} +const newId = () => globalThis.crypto.randomUUID(); +const encode = (value: unknown) => + value == null ? null : JSON.stringify(value); +const decode = (value: unknown) => + typeof value === "string" ? (JSON.parse(value) as T) : ((value as T) ?? null); + +export async function ensureSuggestionTables( + client = getDbExec(), +): Promise { + if (client !== getDbExec()) return; + if (!initialized) + initialized = (async () => { + const ddl = [ + `CREATE TABLE IF NOT EXISTS agent_review_suggestions (id TEXT PRIMARY KEY, resource_type TEXT NOT NULL, resource_id TEXT NOT NULL, adapter_kind TEXT NOT NULL, adapter_version INTEGER NOT NULL, thread_id TEXT NOT NULL, author_email TEXT, actor_kind TEXT NOT NULL, base_revision TEXT NOT NULL, status TEXT NOT NULL DEFAULT 'pending', summary TEXT NOT NULL, owner_email TEXT, org_id TEXT, visibility TEXT NOT NULL DEFAULT 'private', created_at TEXT NOT NULL, updated_at TEXT NOT NULL, metadata_json TEXT)`, + `CREATE TABLE IF NOT EXISTS agent_review_suggestion_operations (id TEXT PRIMARY KEY, suggestion_id TEXT NOT NULL, ordinal INTEGER NOT NULL, operation_kind TEXT NOT NULL, target_id TEXT, before_json TEXT, after_json TEXT, anchor_json TEXT, dependencies_json TEXT, schema_version INTEGER NOT NULL)`, + `CREATE TABLE IF NOT EXISTS agent_review_suggestion_decisions (id TEXT PRIMARY KEY, suggestion_id TEXT NOT NULL, idempotency_key TEXT NOT NULL UNIQUE, reviewer TEXT, decision TEXT NOT NULL, observed_base TEXT, outcome TEXT NOT NULL, detail TEXT, created_at TEXT NOT NULL)`, + `CREATE TABLE IF NOT EXISTS agent_review_suggestion_creations (idempotency_key TEXT PRIMARY KEY, suggestion_id TEXT NOT NULL UNIQUE, author_email TEXT, actor_kind TEXT, request_hash TEXT, created_at TEXT NOT NULL)`, + `CREATE TABLE IF NOT EXISTS agent_review_suggestion_amendments (idempotency_key TEXT PRIMARY KEY, suggestion_id TEXT NOT NULL, revision INTEGER NOT NULL, author_email TEXT NOT NULL, owner_email TEXT, org_id TEXT, visibility TEXT NOT NULL DEFAULT 'private', request_json TEXT NOT NULL, before_json TEXT NOT NULL, after_json TEXT NOT NULL, created_at TEXT NOT NULL, UNIQUE (suggestion_id, revision))`, + ]; + for (const sql of ddl) { + const name = sql.match(/agent_review_[a-z_]+/)![0]; + await ensureTableExists(name, sql); + } + for (const [table, definitions] of [ + [ + "agent_review_suggestions", + [["revision", "INTEGER NOT NULL DEFAULT 1"]], + ], + [ + "agent_review_suggestion_amendments", + [ + ["owner_email", "TEXT"], + ["org_id", "TEXT"], + ["visibility", "TEXT NOT NULL DEFAULT 'private'"], + ], + ], + [ + "agent_review_suggestion_creations", + [ + ["author_email", "TEXT"], + ["actor_kind", "TEXT"], + ["request_hash", "TEXT"], + ], + ], + ] as const) { + for (const [column, type] of definitions) { + await ensureColumnExists( + table, + column, + `ALTER TABLE ${table} ADD COLUMN IF NOT EXISTS ${column} ${type}`, + ); + } + } + await client.execute( + "CREATE INDEX IF NOT EXISTS idx_review_suggestions_resource ON agent_review_suggestions (resource_type, resource_id, created_at)", + ); + await client.execute( + "CREATE INDEX IF NOT EXISTS idx_review_suggestion_operations ON agent_review_suggestion_operations (suggestion_id, ordinal)", + ); + })(); + await initialized; +} + +export interface SuggestionCreationReceipt { + suggestion: ResourceSuggestion; + authorEmail: string | null; + actorKind: ResourceSuggestion["actorKind"] | null; + requestHash: string | null; +} + +export async function getSuggestionByCreationKey( + client: DbExec, + idempotencyKey: string, +): Promise { + const row = ( + await client.execute({ + sql: "SELECT suggestion_id,author_email,actor_kind,request_hash FROM agent_review_suggestion_creations WHERE idempotency_key = ?", + args: [idempotencyKey], + }) + ).rows[0]; + if (!row) return null; + // Read the immutable first-amendment receipt last so it repairs any current-state read torn by a concurrent amendment. + const current = await getSuggestion(String(row.suggestion_id), client); + const amendment = ( + await client.execute({ + sql: "SELECT before_json FROM agent_review_suggestion_amendments WHERE suggestion_id = ? ORDER BY revision LIMIT 1", + args: [String(row.suggestion_id)], + }) + ).rows[0]; + const suggestion = amendment + ? decode(amendment.before_json) + : current + ? { + ...current, + revision: 1, + status: "pending" as const, + updatedAt: current.createdAt, + } + : null; + if (!suggestion) { + throw new Error( + "Suggestion creation receipt references a missing suggestion", + ); + } + return { + suggestion, + authorEmail: row.author_email as string | null, + actorKind: row.actor_kind as ResourceSuggestion["actorKind"] | null, + requestHash: row.request_hash as string | null, + }; +} + +export async function recordSuggestionCreation( + client: DbExec, + idempotencyKey: string, + suggestion: ResourceSuggestion, + authorEmail: string | null, + actorKind: ResourceSuggestion["actorKind"], + requestHash: string, +): Promise { + await client.execute({ + sql: "INSERT INTO agent_review_suggestion_creations (idempotency_key,suggestion_id,author_email,actor_kind,request_hash,created_at) VALUES (?,?,?,?,?,?) ON CONFLICT (idempotency_key) DO NOTHING", + args: [ + idempotencyKey, + suggestion.id, + authorEmail, + actorKind, + requestHash, + new Date().toISOString(), + ], + }); + const receipt = await getSuggestionByCreationKey(client, idempotencyKey); + if (!receipt) throw new Error("Suggestion creation receipt was not recorded"); + return receipt; +} + +export async function deleteUnclaimedSuggestion( + client: DbExec, + suggestionId: string, +): Promise { + await client.execute({ + sql: "DELETE FROM agent_review_suggestion_operations WHERE suggestion_id = ? AND NOT EXISTS (SELECT 1 FROM agent_review_suggestion_creations WHERE suggestion_id = ?)", + args: [suggestionId, suggestionId], + }); + await client.execute({ + sql: "DELETE FROM agent_review_suggestions WHERE id = ? AND NOT EXISTS (SELECT 1 FROM agent_review_suggestion_creations WHERE suggestion_id = ?)", + args: [suggestionId, suggestionId], + }); +} + +export async function insertSuggestion( + input: Omit< + ResourceSuggestion, + "id" | "revision" | "createdAt" | "updatedAt" + >, + client = getDbExec(), +): Promise { + await ensureSuggestionTables(client); + const suggestionId = `suggestion-${newId()}`; + const now = new Date().toISOString(); + await client.execute({ + sql: "INSERT INTO agent_review_suggestions (id,resource_type,resource_id,adapter_kind,adapter_version,thread_id,author_email,actor_kind,base_revision,status,summary,owner_email,org_id,visibility,created_at,updated_at,metadata_json) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)", + args: [ + suggestionId, + input.resourceType, + input.resourceId, + input.adapterKind, + input.adapterVersion, + input.threadId, + input.authorEmail, + input.actorKind, + input.baseRevision, + input.status, + input.summary, + input.ownerEmail, + input.orgId, + input.visibility, + now, + now, + encode(input.metadata), + ], + }); + await insertSuggestionOperations(client, suggestionId, input.operations); + return { + ...input, + id: suggestionId, + revision: 1, + createdAt: now, + updatedAt: now, + }; +} + +async function insertSuggestionOperations( + client: DbExec, + suggestionId: string, + operations: SuggestionOperation[], +) { + for (const operation of operations) + await client.execute({ + sql: "INSERT INTO agent_review_suggestion_operations (id,suggestion_id,ordinal,operation_kind,target_id,before_json,after_json,anchor_json,dependencies_json,schema_version) VALUES (?,?,?,?,?,?,?,?,?,?)", + args: [ + `${suggestionId}-${operation.ordinal}`, + suggestionId, + operation.ordinal, + operation.kind, + operation.targetId ?? null, + encode(operation.before), + encode(operation.after), + encode(operation.anchor), + encode(operation.dependencies), + operation.schemaVersion, + ], + }); +} + +export async function getSuggestionAmendment(client: DbExec, key: string) { + const row = ( + await client.execute({ + sql: "SELECT * FROM agent_review_suggestion_amendments WHERE idempotency_key = ?", + args: [key], + }) + ).rows[0]; + return row + ? { + suggestionId: String(row.suggestion_id), + request: String(row.request_json), + suggestion: decode(row.after_json), + } + : null; +} + +export async function amendSuggestion( + client: DbExec, + before: ResourceSuggestion, + operations: SuggestionOperation[], + summary: string, + idempotencyKey: string, + request: string, +): Promise { + const now = new Date().toISOString(); + const claimed = await client.execute({ + sql: "UPDATE agent_review_suggestions SET revision = revision + 1, summary = ?, updated_at = ? WHERE id = ? AND status = 'pending' AND revision = ?", + args: [summary, now, before.id, before.revision], + }); + if (claimed.rowsAffected !== 1) return null; + await client.execute({ + sql: "DELETE FROM agent_review_suggestion_operations WHERE suggestion_id = ?", + args: [before.id], + }); + await insertSuggestionOperations(client, before.id, operations); + const after = await getSuggestion(before.id, client); + if (!after) throw new Error("Amended suggestion disappeared"); + await client.execute({ + sql: "INSERT INTO agent_review_suggestion_amendments (idempotency_key,suggestion_id,revision,author_email,owner_email,org_id,visibility,request_json,before_json,after_json,created_at) VALUES (?,?,?,?,?,?,?,?,?,?,?)", + args: [ + idempotencyKey, + before.id, + after.revision, + before.authorEmail, + before.ownerEmail, + before.orgId, + before.visibility, + request, + encode(before), + encode(after), + now, + ], + }); + return after; +} + +export async function getSuggestion( + suggestionId: string, + client = getDbExec(), +): Promise { + await ensureSuggestionTables(client); + const row = ( + await client.execute({ + sql: "SELECT * FROM agent_review_suggestions WHERE id = ?", + args: [suggestionId], + }) + ).rows[0]; + if (!row) return null; + const rows = ( + await client.execute({ + sql: "SELECT * FROM agent_review_suggestion_operations WHERE suggestion_id = ? ORDER BY ordinal", + args: [suggestionId], + }) + ).rows; + return suggestionFromRows(row, rows); +} + +function suggestionFromRows( + row: Record, + operationRows: Record[], +): ResourceSuggestion { + return { + id: String(row.id), + revision: Number(row.revision), + resourceType: String(row.resource_type), + resourceId: String(row.resource_id), + adapterKind: String(row.adapter_kind), + adapterVersion: Number(row.adapter_version), + threadId: String(row.thread_id), + authorEmail: row.author_email as string | null, + actorKind: row.actor_kind as ResourceSuggestion["actorKind"], + baseRevision: String(row.base_revision), + status: row.status as SuggestionStatus, + summary: String(row.summary), + ownerEmail: row.owner_email as string | null, + orgId: row.org_id as string | null, + visibility: row.visibility as Visibility, + createdAt: String(row.created_at), + updatedAt: String(row.updated_at), + metadata: decode>(row.metadata_json), + operations: operationRows.map((value) => ({ + id: String(value.id), + ordinal: Number(value.ordinal), + kind: String(value.operation_kind), + targetId: value.target_id as string | null, + before: decode(value.before_json), + after: decode(value.after_json), + anchor: decode(value.anchor_json), + dependencies: decode(value.dependencies_json), + schemaVersion: Number(value.schema_version), + })), + }; +} + +export async function listSuggestions( + resourceType: string, + resourceId: string, + statuses?: readonly SuggestionStatus[], + client = getDbExec(), +): Promise { + await ensureSuggestionTables(client); + const args: unknown[] = [resourceType, resourceId]; + const filter = statuses?.length + ? ` AND s.status IN (${statuses.map(() => "?").join(",")})` + : ""; + args.push(...(statuses ?? [])); + const rows = ( + await client.execute({ + sql: `SELECT s.id AS suggestion_id,s.revision,s.resource_type,s.resource_id,s.adapter_kind,s.adapter_version,s.thread_id,s.author_email,s.actor_kind,s.base_revision,s.status,s.summary,s.owner_email,s.org_id,s.visibility,s.created_at,s.updated_at,s.metadata_json,o.id AS operation_id,o.ordinal AS operation_ordinal,o.operation_kind,o.target_id,o.before_json,o.after_json,o.anchor_json,o.dependencies_json,o.schema_version FROM agent_review_suggestions s LEFT JOIN agent_review_suggestion_operations o ON o.suggestion_id = s.id WHERE s.resource_type = ? AND s.resource_id = ?${filter} ORDER BY s.created_at,o.ordinal`, + args, + }) + ).rows; + const grouped = new Map< + string, + { row: Record; operations: Record[] } + >(); + for (const row of rows) { + const suggestionId = String(row.suggestion_id); + let suggestion = grouped.get(suggestionId); + if (!suggestion) { + suggestion = { row: { ...row, id: suggestionId }, operations: [] }; + grouped.set(suggestionId, suggestion); + } + if (row.operation_id != null) { + suggestion.operations.push({ + ...row, + id: row.operation_id, + suggestion_id: suggestionId, + ordinal: row.operation_ordinal, + }); + } + } + return Array.from(grouped.values(), ({ row, operations }) => + suggestionFromRows(row, operations), + ); +} + +export interface SuggestionDecisionRecord { + id: string; + suggestionId: string; + idempotencyKey: string; + reviewer: string | null; + decision: SuggestionDecision; + observedBase: string; + outcome: string; + detail: string | null; + createdAt: string; +} +export async function recordDecision( + client: DbExec, + input: Omit, +): Promise<{ record: SuggestionDecisionRecord; duplicate: boolean }> { + const row = ( + await client.execute({ + sql: "SELECT * FROM agent_review_suggestion_decisions WHERE idempotency_key = ?", + args: [input.idempotencyKey], + }) + ).rows[0]; + if (row) { + if ( + String(row.suggestion_id) !== input.suggestionId || + String(row.decision) !== input.decision + ) + throw new Error( + "Idempotency key was already used for a different decision", + ); + return { + duplicate: true, + record: { + id: String(row.id), + suggestionId: String(row.suggestion_id), + idempotencyKey: String(row.idempotency_key), + reviewer: row.reviewer as string | null, + decision: row.decision as SuggestionDecision, + observedBase: String(row.observed_base), + outcome: String(row.outcome), + detail: row.detail as string | null, + createdAt: String(row.created_at), + }, + }; + } + const record = { + ...input, + id: `decision-${newId()}`, + createdAt: new Date().toISOString(), + }; + await client.execute({ + sql: "INSERT INTO agent_review_suggestion_decisions (id,suggestion_id,idempotency_key,reviewer,decision,observed_base,outcome,detail,created_at) VALUES (?,?,?,?,?,?,?,?,?)", + args: [ + record.id, + record.suggestionId, + record.idempotencyKey, + record.reviewer, + record.decision, + record.observedBase, + record.outcome, + record.detail, + record.createdAt, + ], + }); + return { duplicate: false, record }; +} + +export async function getDecision( + client: DbExec, + idempotencyKey: string, +): Promise { + const row = ( + await client.execute({ + sql: "SELECT * FROM agent_review_suggestion_decisions WHERE idempotency_key = ?", + args: [idempotencyKey], + }) + ).rows[0]; + if (!row) return null; + return { + id: String(row.id), + suggestionId: String(row.suggestion_id), + idempotencyKey: String(row.idempotency_key), + reviewer: row.reviewer as string | null, + decision: row.decision as SuggestionDecision, + observedBase: String(row.observed_base), + outcome: String(row.outcome), + detail: row.detail as string | null, + createdAt: String(row.created_at), + }; +} +export async function updateSuggestionStatus( + client: DbExec, + suggestionId: string, + status: SuggestionStatus, + observedRevision?: number, +): Promise { + const result = await client.execute({ + sql: "UPDATE agent_review_suggestions SET status = ?, updated_at = ? WHERE id = ? AND status = 'pending' AND revision = ?", + args: [ + status, + new Date().toISOString(), + suggestionId, + observedRevision ?? 1, + ], + }); + return result.rowsAffected === 1; +} + +export async function replaceSuggestionStatus( + client: DbExec, + suggestionId: string, + from: SuggestionStatus, + to: SuggestionStatus, +): Promise { + const result = await client.execute({ + sql: "UPDATE agent_review_suggestions SET status = ?, updated_at = ? WHERE id = ? AND status = ?", + args: [to, new Date().toISOString(), suggestionId, from], + }); + return result.rowsAffected === 1; +} diff --git a/packages/core/src/review/suggestions/types.ts b/packages/core/src/review/suggestions/types.ts new file mode 100644 index 00000000000..986e4528a8e --- /dev/null +++ b/packages/core/src/review/suggestions/types.ts @@ -0,0 +1,84 @@ +import type { Visibility } from "../../sharing/schema.js"; +export type SuggestionStatus = + | "pending" + | "accepted" + | "rejected" + | "stale" + | "superseded"; +export type SuggestionDecision = "accepted" | "rejected"; +export interface SuggestionOperation { + id?: string; + ordinal: number; + kind: string; + targetId?: string | null; + before?: unknown; + after?: unknown; + anchor?: unknown; + dependencies?: unknown; + schemaVersion: number; +} +export interface ResourceSuggestion { + id: string; + revision: number; + resourceType: string; + resourceId: string; + adapterKind: string; + adapterVersion: number; + threadId: string; + authorEmail: string | null; + actorKind: "human" | "agent" | "system"; + baseRevision: string; + status: SuggestionStatus; + summary: string; + ownerEmail: string | null; + orgId: string | null; + visibility: Visibility; + createdAt: string; + updatedAt: string; + metadata: Record | null; + operations: SuggestionOperation[]; +} +export interface SuggestionAccess { + role: "viewer" | "commenter" | "editor" | "admin" | "owner"; + ownerEmail?: string | null; + orgId?: string | null; + visibility?: Visibility | null; +} +export interface SuggestionContext { + resourceType: string; + resourceId: string; + suggestion: ResourceSuggestion; + operations: SuggestionOperation[]; + access: SuggestionAccess; + ctx?: Record; + transaction: unknown; + coordination?: unknown; +} +export interface SuggestionDecisionContext { + resourceType: string; + resourceId: string; + suggestion: ResourceSuggestion; + operations: SuggestionOperation[]; + decision: SuggestionDecision; + access: SuggestionAccess; + ctx?: Record; +} +export interface SuggestionAdapter { + kind: string; + version: number; + validateProposal(input: { + resourceType: string; + resourceId: string; + baseRevision: string; + operations: SuggestionOperation[]; + ctx?: Record; + }): Promise | void | SuggestionOperation[]; + preview?(context: SuggestionContext): Promise | unknown; + coordinateDecision?( + context: SuggestionDecisionContext, + run: (coordination?: unknown) => Promise, + ): Promise; + apply(context: SuggestionContext): Promise | unknown; + describeOperation?(operation: SuggestionOperation): string; + buildUrl?(resourceId: string, suggestionId: string): string; +} diff --git a/packages/core/src/review/types.ts b/packages/core/src/review/types.ts index cda6cc88d3c..c0bc27189c7 100644 --- a/packages/core/src/review/types.ts +++ b/packages/core/src/review/types.ts @@ -60,6 +60,24 @@ export interface ReviewMention { id?: string | null; } +export interface ReviewCommentReaction { + reaction: string; + count: number; + reactedByMe: boolean; +} + +export interface ReviewThreadPreference { + muted: boolean; + unread: boolean; +} + +export interface ReviewDiscussionState { + reactions: Record; + threadPreferences: Record; + canReact: boolean; + canSetThreadPreferences: boolean; +} + export interface ReviewComment { id: string; resourceType: string; diff --git a/packages/core/src/server/action-discovery.ts b/packages/core/src/server/action-discovery.ts index 39cc884b9ce..043957da4d1 100644 --- a/packages/core/src/server/action-discovery.ts +++ b/packages/core/src/server/action-discovery.ts @@ -891,6 +891,42 @@ export async function mergeCoreSharingActions( "send-review-thread-to-agent", () => import("../review/actions/send-review-thread-to-agent.js"), ], + [ + "react-to-review-comment", + () => import("../review/actions/react-to-review-comment.js"), + ], + [ + "set-review-thread-unread", + () => import("../review/actions/set-review-thread-unread.js"), + ], + [ + "set-review-thread-muted", + () => import("../review/actions/set-review-thread-muted.js"), + ], + [ + "create-resource-suggestion", + () => + import("../review/suggestions/actions/create-resource-suggestion.js"), + ], + [ + "list-resource-suggestions", + () => + import("../review/suggestions/actions/list-resource-suggestions.js"), + ], + [ + "update-resource-suggestion", + () => + import("../review/suggestions/actions/update-resource-suggestion.js"), + ], + [ + "get-resource-suggestion", + () => import("../review/suggestions/actions/get-resource-suggestion.js"), + ], + [ + "decide-resource-suggestion", + () => + import("../review/suggestions/actions/decide-resource-suggestion.js"), + ], // Org service tokens (CI credentials, e.g. PLAN_RECAP_TOKEN). Mint/revoke // are toolCallable:false — preserved via preserveActionFlags below. [ diff --git a/packages/core/src/server/agent-chat-plugin.lifecycle.spec.ts b/packages/core/src/server/agent-chat-plugin.lifecycle.spec.ts new file mode 100644 index 00000000000..0d9fed54441 --- /dev/null +++ b/packages/core/src/server/agent-chat-plugin.lifecycle.spec.ts @@ -0,0 +1,248 @@ +import { EventEmitter } from "node:events"; + +import { PGlite } from "@electric-sql/pglite"; +import { createApp } from "h3"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const lifecycle = vi.hoisted(() => ({ + bootstrap: Promise.resolve(), + initPromises: [] as Promise[], + probes: [] as Promise[], + reap: vi.fn<() => Promise>(), + settingsEmitter: null as EventEmitter | null, +})); + +vi.mock("./framework-request-handler.js", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + awaitBootstrap: () => lifecycle.bootstrap, + getH3App: (nitroApp: any) => nitroApp.h3App, + markDefaultPluginProvided: vi.fn(), + trackPluginInit: (_nitroApp: any, promise: Promise) => { + lifecycle.initPromises.push(promise); + }, + }; +}); + +vi.mock("../settings/store.js", () => ({ + deleteSetting: vi.fn(async () => false), + getAllSettings: vi.fn(async () => ({})), + getSetting: vi.fn(async () => null), + getSettingsEmitter: () => lifecycle.settingsEmitter, + putSetting: vi.fn(async () => {}), +})); + +vi.mock("../agent/run-store.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + listUnclaimedBackgroundRunRows: vi.fn(async () => []), + reapAllStaleRuns: () => lifecycle.reap(), + }; +}); + +vi.mock("../mcp-client/index.js", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + startMcpConfigRefresh: () => { + const markDirty = () => {}; + const emitter = lifecycle.settingsEmitter!; + emitter.on("settings", markDirty); + const timer = setInterval(() => {}, 5_000); + return () => { + clearInterval(timer); + emitter.off("settings", markDirty); + }; + }, + }; +}); + +vi.mock("../jobs/scheduler.js", () => ({ + processRecurringJobs: vi.fn(async () => {}), +})); + +vi.mock("../triggers/dispatcher.js", () => ({ + initTriggerDispatcher: vi.fn(async () => {}), +})); + +vi.mock("../chat-threads/migrations.js", () => ({ + runChatThreadDataMigrations: vi.fn(async () => {}), +})); + +vi.mock("./social-og-image.js", () => ({ + createAgentNativeOgImageHandler: () => () => new Response(), +})); + +import { createAgentChatPlugin } from "./agent-chat-plugin.js"; + +interface TestHooks { + hook(name: string, callback: () => void | Promise): void; + callHook(name: string): Promise; +} + +function createTestHooks(): TestHooks { + const callbacks = new Map void | Promise>>(); + return { + hook(name, callback) { + const registered = callbacks.get(name) ?? []; + registered.push(callback); + callbacks.set(name, registered); + }, + async callHook(name) { + await Promise.all( + (callbacks.get(name) ?? []).map((callback) => callback()), + ); + }, + }; +} + +function startGeneration() { + const hooks = createTestHooks(); + const nitroApp = { h3App: createApp(), hooks }; + const plugin = createAgentChatPlugin({ + actions: () => ({}), + a2aAgentDelegation: false, + frameworkTools: "minimal", + leanPrompt: true, + mcp: { enabled: false }, + }); + plugin(nitroApp); + const initPromise = lifecycle.initPromises.at(-1); + expect(initPromise).toBeDefined(); + return { initPromise: initPromise!, nitroApp }; +} + +async function initializeGeneration() { + const generation = startGeneration(); + await generation.initPromise; + return generation.nitroApp; +} + +describe("agent chat plugin Nitro lifecycle", () => { + let database: PGlite; + let releaseTransactions: (() => void) | undefined; + let transactionGate: Promise; + let pendingTransactions: number; + let settledTransactions: number; + + beforeEach(async () => { + vi.useFakeTimers(); + vi.stubEnv("NODE_ENV", "development"); + vi.stubEnv("AGENT_NATIVE_MCP_CONFIG_REFRESH_MS", "5000"); + lifecycle.bootstrap = Promise.resolve(); + lifecycle.initPromises.length = 0; + lifecycle.probes.length = 0; + lifecycle.settingsEmitter = new EventEmitter(); + database = new PGlite(); + pendingTransactions = 0; + settledTransactions = 0; + transactionGate = new Promise((resolve) => { + releaseTransactions = resolve; + }); + lifecycle.reap.mockReset(); + lifecycle.reap.mockImplementation(() => { + const probe = (async () => { + pendingTransactions += 1; + try { + await database.transaction(async (tx) => { + await transactionGate; + await tx.query("SELECT 1 AS ok"); + }); + } finally { + pendingTransactions -= 1; + settledTransactions += 1; + } + return { scanned: 0, reaped: 0, failed: 0, truncated: false }; + })(); + lifecycle.probes.push(probe); + return probe; + }); + }); + + afterEach(async () => { + vi.clearAllTimers(); + releaseTransactions?.(); + await Promise.allSettled(lifecycle.probes); + await database.close(); + vi.useRealTimers(); + vi.unstubAllEnvs(); + }); + + async function startFastSweep(): Promise { + await vi.advanceTimersByTimeAsync(10_000); + await vi.waitFor(() => expect(lifecycle.reap).toHaveBeenCalled()); + } + + it("keeps repeated init and close equivalent to one live generation", async () => { + const fresh = await initializeGeneration(); + await startFastSweep(); + expect(pendingTransactions).toBe(1); + await vi.waitFor(() => + expect(lifecycle.settingsEmitter!.listenerCount("settings")).toBe(1), + ); + + releaseTransactions?.(); + await vi.waitFor(() => expect(settledTransactions).toBe(1)); + await fresh.hooks.callHook("close"); + await database.close(); + + database = new PGlite(); + pendingTransactions = 0; + settledTransactions = 0; + transactionGate = new Promise((resolve) => { + releaseTransactions = resolve; + }); + lifecycle.reap.mockClear(); + lifecycle.initPromises.length = 0; + lifecycle.probes.length = 0; + lifecycle.settingsEmitter = new EventEmitter(); + vi.clearAllTimers(); + + for (let generation = 0; generation < 9; generation += 1) { + const app = await initializeGeneration(); + await app.hooks.callHook("close"); + } + await initializeGeneration(); + await startFastSweep(); + + const repeatedLifecycle = { + pendingTransactions, + settingsListeners: lifecycle.settingsEmitter!.listenerCount("settings"), + }; + + releaseTransactions?.(); + await Promise.allSettled(lifecycle.probes); + expect(settledTransactions).toBe(repeatedLifecycle.pendingTransactions); + await expect(database.query("SELECT 1 AS ok")).resolves.toMatchObject({ + rows: [{ ok: 1 }], + }); + + expect(repeatedLifecycle).toEqual({ + pendingTransactions: 1, + settingsListeners: 1, + }); + }); + + it("cleans resources registered after close races asynchronous initialization", async () => { + let releaseBootstrap!: () => void; + lifecycle.bootstrap = new Promise((resolve) => { + releaseBootstrap = resolve; + }); + const generation = startGeneration(); + + const close = generation.nitroApp.hooks.callHook("close"); + releaseBootstrap(); + await Promise.all([generation.initPromise, close]); + await vi.advanceTimersByTimeAsync(60_000); + + expect(lifecycle.reap).not.toHaveBeenCalled(); + expect(lifecycle.settingsEmitter!.listenerCount("settings")).toBe(0); + await expect(database.query("SELECT 1 AS ok")).resolves.toMatchObject({ + rows: [{ ok: 1 }], + }); + }); +}); diff --git a/packages/core/src/server/agent-chat-plugin.ts b/packages/core/src/server/agent-chat-plugin.ts index 0702e3d5ff9..84b7fada096 100644 --- a/packages/core/src/server/agent-chat-plugin.ts +++ b/packages/core/src/server/agent-chat-plugin.ts @@ -714,10 +714,71 @@ export function resolveHostedBuilderHandoff( return connectBuilder ? { "connect-builder": connectBuilder } : {}; } +type AgentChatPluginCleanup = () => void | Promise; + +function createAgentChatPluginLifecycle() { + const cleanups = new Set(); + const pendingCleanups = new Set>(); + let closed = false; + + const runCleanup = (cleanup: AgentChatPluginCleanup): void => { + const pending = Promise.resolve() + .then(cleanup) + .catch((error: unknown) => { + console.warn("[agent-chat] Plugin cleanup failed:", error); + }); + pendingCleanups.add(pending); + void pending.finally(() => pendingCleanups.delete(pending)); + }; + + const addCleanup = (cleanup: AgentChatPluginCleanup): void => { + if (closed) { + runCleanup(cleanup); + return; + } + cleanups.add(cleanup); + }; + + return { + addCleanup, + startTimeout( + callback: () => void, + delayMs: number, + ): ReturnType | undefined { + if (closed) return undefined; + const timer = setTimeout(callback, delayMs); + addCleanup(() => clearTimeout(timer)); + return timer; + }, + startInterval( + callback: () => void, + intervalMs: number, + ): ReturnType | undefined { + if (closed) return undefined; + const timer = setInterval(callback, intervalMs); + addCleanup(() => clearInterval(timer)); + return timer; + }, + beginClose(): void { + if (closed) return; + closed = true; + const registered = [...cleanups]; + cleanups.clear(); + for (const cleanup of registered) runCleanup(cleanup); + }, + async drainCleanups(): Promise { + while (pendingCleanups.size > 0) { + await Promise.allSettled([...pendingCleanups]); + } + }, + }; +} + export function createAgentChatPlugin( options?: AgentChatPluginOptions, ): NitroPluginDef { return (nitroApp: any) => { + const lifecycle = createAgentChatPluginLifecycle(); markDefaultPluginProvided(nitroApp, "agent-chat"); // Nitro v3 calls plugins synchronously and doesn't await async return // values. We track the async init so the framework's readiness gate @@ -817,7 +878,10 @@ export function createAgentChatPlugin( ); } await mcpManager.reconfigure(mcpConfig); - startMcpConfigRefresh(mcpManager); + const stopMcpConfigRefresh = startMcpConfigRefresh(mcpManager); + if (stopMcpConfigRefresh) { + lifecycle.addCleanup(stopMcpConfigRefresh); + } }; /** * Start MCP initialization at most once, and return the run in flight. @@ -6952,8 +7016,8 @@ Non-code requests are still fine on this surface: read data, navigate the UI, su } } else { // Start after a 10-second delay to let the server fully initialize - setTimeout(() => { - setInterval(() => { + lifecycle.startTimeout(() => { + lifecycle.startInterval(() => { processRecurringJobs(schedulerDeps).catch((err) => { console.error( "[recurring-jobs] Scheduler error:", @@ -6998,8 +7062,8 @@ Non-code requests are still fine on this surface: read data, navigate the UI, su inFlight = false; } }; - setTimeout(() => void sweep(), 15_000); - setInterval(() => void sweep(), 30_000); + lifecycle.startTimeout(() => void sweep(), 15_000); + lifecycle.startInterval(() => void sweep(), 30_000); })(); } @@ -7029,8 +7093,8 @@ Non-code requests are still fine on this surface: read data, navigate the UI, su let lastSweep = 0; const SWEEP_INTERVAL_MS = 2 * 60 * 1000; - setTimeout(() => { - setInterval(() => { + lifecycle.startTimeout(() => { + lifecycle.startInterval(() => { const now = Date.now(); if (now - lastSweep < SWEEP_INTERVAL_MS) return; lastSweep = now; @@ -7185,11 +7249,11 @@ Non-code requests are still fine on this surface: read data, navigate the UI, su // run-store.ts's `UNCLAIMED_BACKGROUND_RUN_FAST_SWEEP_MS` doc comment. (() => { if (isBackgroundRuntime || sweepsDisabled) return; - setTimeout(() => { + lifecycle.startTimeout(() => { (async () => { const { UNCLAIMED_BACKGROUND_RUN_FAST_SWEEP_MS } = await import("../agent/run-store.js"); - startIntervalJob( + const job = startIntervalJob( async () => { const { listUnclaimedBackgroundRunRows, @@ -7233,6 +7297,7 @@ Non-code requests are still fine on this surface: read data, navigate the UI, su }, { intervalMs: UNCLAIMED_BACKGROUND_RUN_FAST_SWEEP_MS }, ); + lifecycle.addCleanup(() => job.stop()); })().catch(() => { // best-effort — if run-store fails to load, the slow sweep below // still provides eventual (loud) recovery. @@ -7251,8 +7316,8 @@ Non-code requests are still fine on this surface: read data, navigate the UI, su let lastSweep = 0; const SWEEP_INTERVAL_MS = 2 * 60 * 1000; - setTimeout(() => { - setInterval(() => { + lifecycle.startTimeout(() => { + lifecycle.startInterval(() => { const now = Date.now(); if (now - lastSweep < SWEEP_INTERVAL_MS) return; lastSweep = now; @@ -7377,6 +7442,11 @@ Non-code requests are still fine on this surface: read data, navigate the UI, su "/_agent-native/a2a", ], }); + nitroApp.hooks?.hook?.("close", async () => { + lifecycle.beginClose(); + await initPromise; + await lifecycle.drainCleanups(); + }); }; } diff --git a/packages/core/src/server/app-sync-state.spec.ts b/packages/core/src/server/app-sync-state.spec.ts index a55dfe1cbfb..bdae026efa4 100644 --- a/packages/core/src/server/app-sync-state.spec.ts +++ b/packages/core/src/server/app-sync-state.spec.ts @@ -125,6 +125,38 @@ describe("AppSyncState multi-app isolation", () => { expect(ids[0]).not.toBe(ids[1]); }); + it("persists a transactional change before publishing it", async () => { + process.env.AGENT_NATIVE_SYNC_EVENTS_ENABLE_IN_TESTS = "1"; + const schemaDb = makeDb(); + const transaction = { + execute: vi.fn(async () => ({ rows: [], rowsAffected: 1 })), + }; + const state = new AppSyncState({ + getDb: () => schemaDb, + isPostgres: () => false, + }); + const change = await state.prepareTransactionalChange({ + source: "collab", + type: "change", + key: "doc-1", + resourceType: "document", + resourceId: "doc-1", + }); + expect(change.isPersisted()).toBe(false); + + expect(state.getChangesSince(0).events).toEqual([]); + const persisted = await change.persist(transaction); + expect(change.isPersisted()).toBe(true); + expect(state.getChangesSince(0).events).toEqual([]); + expect(transaction.execute).toHaveBeenCalledOnce(); + expect(persisted.resourceId).toBe("doc-1"); + + change.publish(); + expect(state.getChangesSince(0).events).toMatchObject([ + { source: "collab", resourceId: "doc-1" }, + ]); + }); + it("does not reuse an org-A access decision under an org-B session", async () => { const flush = async () => { for (let i = 0; i < 5; i++) await Promise.resolve(); diff --git a/packages/core/src/server/index.ts b/packages/core/src/server/index.ts index 7d0913b69a1..c46a29ae50a 100644 --- a/packages/core/src/server/index.ts +++ b/packages/core/src/server/index.ts @@ -195,11 +195,13 @@ export { createDevScriptRegistry } from "../scripts/dev/index.js"; export { createPollHandler, recordChange, + prepareTransactionalChange, getVersion, getChangesSince, getPollEmitter, canSeeChangeForUser, POLL_CHANGE_EVENT, + type TransactionalChange, } from "./poll.js"; export { createPollEventsHandler } from "./poll-events.js"; export { createAuthPlugin, defaultAuthPlugin } from "./auth-plugin.js"; diff --git a/packages/core/src/server/poll.ts b/packages/core/src/server/poll.ts index 7da1bf6ea1f..1552bae03aa 100644 --- a/packages/core/src/server/poll.ts +++ b/packages/core/src/server/poll.ts @@ -83,6 +83,12 @@ export interface ChangeEvent { [k: string]: unknown; } +export interface TransactionalChange { + persist(transaction: DbExec): Promise; + isPersisted(): boolean; + publish(): ChangeEvent; +} + // In-memory ring buffer of recent changes. Kept small since clients // poll frequently (every 2-3s) and only need events since their last poll. const MAX_BUFFER = 200; @@ -1166,6 +1172,93 @@ export class AppSyncState { ); } + /** + * Prepare a durable change whose row must commit with a caller-owned + * transaction. Persistence and in-process publication are deliberately + * separate: callers persist through the transaction, then publish only + * after that transaction resolves successfully. + */ + async prepareTransactionalChange(event: { + source: string; + type: string; + key?: string; + [k: string]: unknown; + }): Promise { + if (!(await this.ensureSyncEventsTable())) { + throw new Error( + "Transactional change delivery requires durable sync events", + ); + } + const id = `${Date.now()}-${Math.random().toString(36).slice(2, 10)}`; + let persisted: ChangeEvent | null = null; + return { + persist: async (transaction) => { + if (persisted) return persisted; + let version: number; + if (this.dbAssignedVersions) { + const result = await transaction.execute({ + sql: ALLOCATING_INSERT_SQL, + args: [ + this.version + 1, + id, + JSON.stringify({ ...event, cursorId: id }) + .split("\\u0000") + .join(""), + event.source, + event.type, + event.key ?? null, + (event.owner as string | undefined) ?? null, + (event.orgId as string | undefined) ?? null, + (event.resourceType as string | undefined) ?? null, + (event.resourceId as string | undefined) ?? null, + Date.now(), + ], + }); + version = timestampValue(result.rows[0]?.version); + if (version <= 0) { + throw new Error("Durable sync version allocation failed"); + } + } else { + version = Math.max(this.version + 1, Date.now()); + const entry = { ...event, version, cursorId: id } as ChangeEvent; + const result = await transaction.execute({ + sql: `INSERT INTO sync_events (id, version, event_json, source, type, event_key, owner, org_id, resource_type, resource_id, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT (id) DO NOTHING`, + args: [ + id, + version, + JSON.stringify(entry), + entry.source, + entry.type, + entry.key ?? null, + entry.owner ?? null, + entry.orgId ?? null, + entry.resourceType ?? null, + entry.resourceId ?? null, + Date.now(), + ], + }); + if (result.rowsAffected !== 1) { + throw new Error("Durable sync event was not persisted"); + } + } + persisted = { ...event, version, cursorId: id } as ChangeEvent; + return persisted; + }, + isPersisted: () => persisted !== null, + publish: () => { + if (!persisted) { + throw new Error( + "Transactional change cannot publish before persistence commits", + ); + } + this.version = Math.max(this.version, persisted.version); + this.commitEntryForChain(persisted); + return persisted; + }, + }; + } + /** Buffer + emit. Shared by both version-allocation modes. */ private commitEntry(entry: ChangeEvent): void { this.buffer.push(entry); @@ -2135,6 +2228,15 @@ export function recordChange(event: { getDefaultAppSyncState().recordChange(event); } +export function prepareTransactionalChange(event: { + source: string; + type: string; + key?: string; + [k: string]: unknown; +}): Promise { + return getDefaultAppSyncState().prepareTransactionalChange(event); +} + setActionChangeFastPath((target) => { getDefaultAppSyncState().recordChange( { diff --git a/packages/core/src/server/release-schema.ts b/packages/core/src/server/release-schema.ts index afeee83d258..4c4e8209094 100644 --- a/packages/core/src/server/release-schema.ts +++ b/packages/core/src/server/release-schema.ts @@ -304,6 +304,13 @@ const FRAMEWORK_SCHEMA_ENSURES: readonly SchemaEnsure[] = [ "Review", () => import("../review/store.js").then((m) => m.ensureReviewTables()), ], + [ + "ReviewSuggestions", + () => + import("../review/suggestions/store.js").then((m) => + m.ensureSuggestionTables(), + ), + ], [ "SandboxExecutions", () => diff --git a/packages/core/src/vite/action-types-plugin.spec.ts b/packages/core/src/vite/action-types-plugin.spec.ts index 24bb48321d5..dd2b7783635 100644 --- a/packages/core/src/vite/action-types-plugin.spec.ts +++ b/packages/core/src/vite/action-types-plugin.spec.ts @@ -46,6 +46,7 @@ describe("generateActionRegistryForProject", () => { expect(registry).toContain('"set-localization-preference"'); expect(registry).toContain('"list-resource-history"'); expect(registry).toContain('"list-review-comments"'); + expect(registry).toContain('"update-resource-suggestion"'); expect(registry).not.toContain("real-action.spec"); expect(registry).not.toContain("other.test"); @@ -57,6 +58,7 @@ describe("generateActionRegistryForProject", () => { expect(types).toContain('"set-localization-preference"'); expect(types).toContain('"list-resource-history"'); expect(types).toContain('"list-review-comments"'); + expect(types).toContain('"update-resource-suggestion"'); } finally { fs.rmSync(root, { recursive: true, force: true }); } diff --git a/packages/core/src/vite/action-types-plugin.ts b/packages/core/src/vite/action-types-plugin.ts index 630d4f7a09a..8639b91d997 100644 --- a/packages/core/src/vite/action-types-plugin.ts +++ b/packages/core/src/vite/action-types-plugin.ts @@ -219,6 +219,43 @@ const CORE_SHARING_ACTIONS: Array<{ name: string; specifier: string }> = [ name: "send-review-thread-to-agent", specifier: "@agent-native/core/review/actions/send-review-thread-to-agent", }, + { + name: "react-to-review-comment", + specifier: "@agent-native/core/review/actions/react-to-review-comment", + }, + { + name: "set-review-thread-unread", + specifier: "@agent-native/core/review/actions/set-review-thread-unread", + }, + { + name: "set-review-thread-muted", + specifier: "@agent-native/core/review/actions/set-review-thread-muted", + }, + { + name: "create-resource-suggestion", + specifier: + "@agent-native/core/review/suggestions/actions/create-resource-suggestion", + }, + { + name: "update-resource-suggestion", + specifier: + "@agent-native/core/review/suggestions/actions/update-resource-suggestion", + }, + { + name: "list-resource-suggestions", + specifier: + "@agent-native/core/review/suggestions/actions/list-resource-suggestions", + }, + { + name: "get-resource-suggestion", + specifier: + "@agent-native/core/review/suggestions/actions/get-resource-suggestion", + }, + { + name: "decide-resource-suggestion", + specifier: + "@agent-native/core/review/suggestions/actions/decide-resource-suggestion", + }, ]; function isRuntimeSourceFile(filename: string): boolean { diff --git a/packages/core/src/vite/client.spec.ts b/packages/core/src/vite/client.spec.ts index 0302de1587c..f3f784e1865 100644 --- a/packages/core/src/vite/client.spec.ts +++ b/packages/core/src/vite/client.spec.ts @@ -2606,6 +2606,9 @@ describe("Vite SSR stubs", () => { expect(code).toContain("export const UndoManager = stub;"); expect(code).toContain("export const EditorContent = stub;"); expect(code).toContain("export const createNodeFromContent = stub;"); + expect(code).toContain("export const Slice = stub;"); + expect(code).toContain("export const Transform = stub;"); + expect(code).toContain("export const getSchema = stub;"); expect(code).toContain("export const format = stub;"); expect(code).toContain("export const InputRule = stub;"); expect(code).toContain("export const isNodeEmpty = stub;"); diff --git a/packages/core/src/vite/client.ts b/packages/core/src/vite/client.ts index 2c51c80b736..7f9c7f3d861 100644 --- a/packages/core/src/vite/client.ts +++ b/packages/core/src/vite/client.ts @@ -2634,6 +2634,7 @@ function ssrStubPlugin(packages: string[]): Plugin | null { "Selection", "SimpleImageAttachmentAdapter", "SimpleTextAttachmentAdapter", + "Slice", "StarterKit", "Table", "TableCell", @@ -2645,6 +2646,7 @@ function ssrStubPlugin(packages: string[]): Plugin | null { "Text", "TextSelection", "ThreadPrimitive", + "Transform", "WebLinksAddon", "captureException", "codeToHtml", @@ -2658,6 +2660,7 @@ function ssrStubPlugin(packages: string[]): Plugin | null { "Doc", "getHTMLFromFragment", "getIsolationScope", + "getSchema", "init", "isChangeOrigin", "isNodeEmpty", diff --git a/packages/toolkit/src/editor/index.ts b/packages/toolkit/src/editor/index.ts index 9eec5d3ec0d..3991c9337e0 100644 --- a/packages/toolkit/src/editor/index.ts +++ b/packages/toolkit/src/editor/index.ts @@ -19,6 +19,7 @@ export { applyDocSurgically, defaultParseValue, diffTopLevel, + planDocReconcile, type TopLevelDiff, } from "./surgical-apply.js"; export { diff --git a/packages/toolkit/src/editor/surgical-apply.spec.ts b/packages/toolkit/src/editor/surgical-apply.spec.ts index ce6d3fac029..a91599b7915 100644 --- a/packages/toolkit/src/editor/surgical-apply.spec.ts +++ b/packages/toolkit/src/editor/surgical-apply.spec.ts @@ -252,6 +252,88 @@ describe("applyDocSurgically", () => { }); describe("reconcileDocAgainstBase", () => { + it.each([ + { base: "Alpha", server: "Accepted", live: "Accepted" }, + { + base: "Alpha\n\nBravo", + server: "Accepted\n\nBravo", + live: "Accepted\n\nBravo local", + }, + ])( + "acknowledges common replacements without a transaction: $live", + ({ base, server, live }) => { + const editor = makeEditor(live); + try { + const before = editor.state.doc; + const result = reconcileDocAgainstBase( + editor, + parse(editor, base), + parse(editor, server), + ); + expect(result.status).toBe("noop"); + expect(editor.state.doc).toBe(before); + expect(md(editor)).toBe(live); + } finally { + editor.destroy(); + } + }, + ); + + it.each([ + { + base: "Alpha\n\nBravo", + server: "Accepted\n\nBravo", + live: "Different\n\nBravo local", + }, + { + base: "Alpha\n\nBravo", + server: "Accepted\n\nBravo server", + live: "Accepted\n\nBravo local", + }, + { + base: "Alpha\n\nBravo", + server: "Bravo\n\nAlpha", + live: "Bravo\n\nAlpha local", + }, + { + base: "Same\n\nMiddle\n\nSame", + server: "Accepted\n\nMiddle\n\nSame", + live: "Accepted\n\nMiddle local\n\nSame", + }, + ])( + "preserves conflicting or ambiguous common-change candidates: $live", + ({ base, server, live }) => { + const editor = makeEditor(live); + try { + const before = editor.state.doc; + const result = reconcileDocAgainstBase( + editor, + parse(editor, base), + parse(editor, server), + ); + expect(["conflict", "failed"]).toContain(result.status); + expect(editor.state.doc).toBe(before); + } finally { + editor.destroy(); + } + }, + ); + + it("applies remaining server changes beside a common replacement and local edit", () => { + const editor = makeEditor("Accepted\n\nBravo local\n\nCharlie"); + try { + const result = reconcileDocAgainstBase( + editor, + parse(editor, "Alpha\n\nBravo\n\nCharlie"), + parse(editor, "Accepted\n\nBravo\n\nCharlie server"), + ); + expect(result.status).toBe("applied"); + expect(md(editor)).toBe("Accepted\n\nBravo local\n\nCharlie server"); + } finally { + editor.destroy(); + } + }); + it("merges a server insertion between unchanged blocks with a separate local edit", () => { const editor = makeEditor("Alpha\n\nBravo\n\nCharlie local"); try { diff --git a/packages/toolkit/src/editor/surgical-apply.ts b/packages/toolkit/src/editor/surgical-apply.ts index a9a2e79ef7f..0e3515cb828 100644 --- a/packages/toolkit/src/editor/surgical-apply.ts +++ b/packages/toolkit/src/editor/surgical-apply.ts @@ -18,6 +18,7 @@ import { createNodeFromContent } from "@tiptap/core"; import type { Fragment, Node as ProseMirrorNode } from "@tiptap/pm/model"; +import { Transform, type Step } from "@tiptap/pm/transform"; import type { Editor } from "@tiptap/react"; /** @@ -54,6 +55,10 @@ export type BaseAwareReconcileResult = | { status: "conflict"; localDraft: ProseMirrorNode } | { status: "failed"; reason: "schema" | "ambiguous" | "transaction" }; +export type BaseAwareReconcilePlan = + | Exclude + | { status: "applied"; mergedDoc: ProseMirrorNode; steps: readonly Step[] }; + function nodesEqual(left: ProseMirrorNode, right: ProseMirrorNode): boolean { return left.eq(right); } @@ -180,26 +185,91 @@ function mappedLocalIndex( return mapped; } -/** Apply authoritative server changes onto a locally edited document. */ -export function reconcileDocAgainstBase( - editor: Editor, +function hasUnambiguousReplacementPositions( + baseDoc: ProseMirrorNode, + changedDoc: ProseMirrorNode, +): boolean { + if (baseDoc.childCount !== changedDoc.childCount) return false; + for (let i = 0; i < baseDoc.childCount; i++) { + for (let j = 0; j < baseDoc.childCount; j++) { + if (i === j) continue; + if ( + baseDoc.child(i).eq(baseDoc.child(j)) || + changedDoc.child(i).eq(changedDoc.child(j)) || + baseDoc.child(i).eq(changedDoc.child(j)) + ) { + return false; + } + } + } + return true; +} + +function splitReplacementHunks(hunks: TopLevelHunk[]): TopLevelHunk[] { + return hunks.flatMap((hunk) => { + const count = hunk.baseToIndex - hunk.baseFromIndex; + if (count === 0 || count !== hunk.changedToIndex - hunk.changedFromIndex) { + return [hunk]; + } + return Array.from({ length: count }, (_, offset) => ({ + baseFromIndex: hunk.baseFromIndex + offset, + baseToIndex: hunk.baseFromIndex + offset + 1, + changedFromIndex: hunk.changedFromIndex + offset, + changedToIndex: hunk.changedFromIndex + offset + 1, + })); + }); +} + +/** Plan authoritative server changes without mutating a live editor or Y.Doc. */ +export function planDocReconcile( + liveDoc: ProseMirrorNode, baseDoc: ProseMirrorNode, serverDoc: ProseMirrorNode, -): BaseAwareReconcileResult { - const liveDoc = editor.state.doc; +): BaseAwareReconcilePlan { if ( - baseDoc.type.schema !== editor.schema || - serverDoc.type.schema !== editor.schema || + baseDoc.type.schema !== liveDoc.type.schema || + serverDoc.type.schema !== liveDoc.type.schema || baseDoc.type !== liveDoc.type || serverDoc.type !== liveDoc.type ) { return { status: "failed", reason: "schema" }; } - const localHunks = diffTopLevelHunks(baseDoc, liveDoc); - const serverHunks = diffTopLevelHunks(baseDoc, serverDoc); - if (!localHunks || !serverHunks) { + const localDiff = diffTopLevelHunks(baseDoc, liveDoc); + const serverDiff = diffTopLevelHunks(baseDoc, serverDoc); + if (!localDiff || !serverDiff) { return { status: "failed", reason: "ambiguous" }; } + let localHunks = localDiff; + let serverHunks = serverDiff; + if (serverHunks.length === 0) return { status: "noop" }; + // A peer can deliver an accepted replacement before its SQL revision arrives. + // Split adjacent substitutions only when exact identities rule out moves and + // repeated-block alignment; equal block counts alone do not prove positions. + if ( + hasUnambiguousReplacementPositions(baseDoc, liveDoc) && + hasUnambiguousReplacementPositions(baseDoc, serverDoc) + ) { + localHunks = splitReplacementHunks(localHunks); + serverHunks = splitReplacementHunks(serverHunks).filter( + (server) => + !localHunks.some( + (local) => + local.baseFromIndex === server.baseFromIndex && + local.baseToIndex === server.baseToIndex && + liveDoc.content + .cut( + positionOfChild(liveDoc, local.changedFromIndex), + positionOfChild(liveDoc, local.changedToIndex), + ) + .eq( + serverDoc.content.cut( + positionOfChild(serverDoc, server.changedFromIndex), + positionOfChild(serverDoc, server.changedToIndex), + ), + ), + ), + ); + } if (serverHunks.length === 0) return { status: "noop" }; if ( localHunks.some((local) => @@ -210,7 +280,7 @@ export function reconcileDocAgainstBase( } try { - const tr = editor.state.tr; + const tr = new Transform(liveDoc); for (const hunk of [...serverHunks].reverse()) { const fromIndex = mappedLocalIndex(hunk.baseFromIndex, localHunks); const toIndex = mappedLocalIndex(hunk.baseToIndex, localHunks); @@ -223,6 +293,23 @@ export function reconcileDocAgainstBase( ), ); } + return { status: "applied", mergedDoc: tr.doc, steps: tr.steps }; + } catch { + return { status: "failed", reason: "transaction" }; + } +} + +/** Apply authoritative server changes onto a locally edited document. */ +export function reconcileDocAgainstBase( + editor: Editor, + baseDoc: ProseMirrorNode, + serverDoc: ProseMirrorNode, +): BaseAwareReconcileResult { + const plan = planDocReconcile(editor.state.doc, baseDoc, serverDoc); + if (plan.status !== "applied") return plan; + try { + const tr = editor.state.tr; + for (const step of plan.steps) tr.step(step); tr.setMeta("addToHistory", false); tr.setMeta(RICH_MARKDOWN_PROGRAMMATIC_TRANSACTION, true); editor.view.dispatch(tr); diff --git a/packages/toolkit/src/editor/useCollabReconcile.concurrent.spec.ts b/packages/toolkit/src/editor/useCollabReconcile.concurrent.spec.ts index 4235901a019..1484f9861d5 100644 --- a/packages/toolkit/src/editor/useCollabReconcile.concurrent.spec.ts +++ b/packages/toolkit/src/editor/useCollabReconcile.concurrent.spec.ts @@ -50,6 +50,7 @@ interface HarnessProps { contentUpdatedAt: string; contentRevision?: string; editorOwnedFocus?: boolean; + isEditorFocused?: () => boolean; } interface CollabSeedHarnessProps { @@ -58,6 +59,7 @@ interface CollabSeedHarnessProps { value?: string; contentRevision?: string; contentUpdatedAt?: string; + initialAppliedUpdatedAt?: string | null; } interface Captured { @@ -79,6 +81,7 @@ function makeHarness() { contentUpdatedAt, contentRevision, editorOwnedFocus = false, + isEditorFocused, }: HarnessProps) { const guardsRef = React.useRef editorOwnedFocus, + isEditorFocused: isEditorFocused ?? (() => editorOwnedFocus), getMarkdown: getEditorMarkdown, setContent: (ed, v, options) => { captured.setContentCalls += 1; @@ -144,6 +147,7 @@ function makeCollabSeedHarness(initialContent = "") { value = "seeded content", contentRevision, contentUpdatedAt = "2024-01-01T00:00:01.000Z", + initialAppliedUpdatedAt, }: CollabSeedHarnessProps) { const guardsRef = React.useRef { (captured.reconciled ??= []).push({ status: result.status, @@ -207,6 +212,169 @@ async function flush() { }); } +function makePeerReconcileHarness(initialContent = "original body") { + const ydoc = new Y.Doc(); + ydoc.clientID = 2; + const seedEditor = new CoreEditor({ + extensions: createRichMarkdownExtensions({ dialect: "gfm", ydoc }), + }); + seedEditor.commands.setContent(initialContent); + seedEditor.destroy(); + const awareness = new Awareness(ydoc); + awareness.getStates().set(3, { + user: { name: "Peer" }, + visible: true, + canFlushDocument: true, + }); + const writes: Array<{ value: string; callbackVersion: number }> = []; + const reconciled: Array<{ status: string; baseRevision: string }> = []; + let editor: Editor | null = null; + function Harness({ + value = initialContent, + revision = "revision-1", + callbackVersion = 0, + available = true, + collabContentRevision, + requestCollabSync, + baseAware = false, + }: { + value?: string; + revision?: string; + callbackVersion?: number; + available?: boolean; + collabContentRevision?: string; + requestCollabSync?: () => Promise<{ + status: "synced" | "failed" | "unavailable"; + }>; + baseAware?: boolean; + }) { + editor = useEditor({ + extensions: createRichMarkdownExtensions({ dialect: "gfm", ydoc }), + }); + useCollabReconcile({ + editor: available ? editor : null, + ydoc, + awareness, + collabSynced: true, + value, + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + contentRevision: revision, + collabContentRevision, + requestCollabSync, + initialAppliedUpdatedAt: null, + editable: true, + parseValue: baseAware ? undefined : false, + onBaseAwareReconcile: baseAware + ? (result) => { + reconciled.push(result); + } + : undefined, + getMarkdown: (editorToRead) => getEditorMarkdown(editorToRead), + setContent: (editorToWrite, nextValue, options) => { + writes.push({ value: nextValue, callbackVersion }); + editorToWrite.commands.setContent(nextValue, { + emitUpdate: options.emitUpdate, + }); + }, + }); + return React.createElement("div", null); + } + return { + Harness, + writes, + reconciled, + awareness, + ydoc, + editor: () => editor!, + markdown: () => getEditorMarkdown(editor!), + dispose: () => { + act(() => root.unmount()); + root = createRoot(container); + awareness.destroy(); + ydoc.destroy(); + }, + }; +} + +function makeConnectedEditorHarness(authorLeads: boolean) { + const docs = [new Y.Doc(), new Y.Doc()]; + docs[0]!.clientID = authorLeads ? 1 : 2; + docs[1]!.clientID = authorLeads ? 2 : 1; + const awareness = docs.map((doc) => new Awareness(doc)); + docs.forEach((doc, index) => { + doc.on("update", (update, origin) => { + if (origin !== "peer") Y.applyUpdate(docs[1 - index]!, update, "peer"); + }); + awareness[index]!.getStates().set(1, { + user: { name: "First editor" }, + visible: true, + canFlushDocument: true, + }); + awareness[index]!.getStates().set(2, { + user: { name: "Second editor" }, + visible: true, + canFlushDocument: true, + }); + }); + const editors: Array = [null, null]; + const emitted: string[][] = [[], []]; + const reconciled: Array<{ status: string; content: string }> = []; + function Probe({ index, ...props }: HarnessProps & { index: number }) { + const guardsRef = React.useRef | null>(null); + const editor = useEditor({ + extensions: createRichMarkdownExtensions({ + dialect: "gfm", + ydoc: docs[index], + }), + onUpdate: ({ editor, transaction }) => { + const guards = guardsRef.current; + if (!guards || guards.shouldIgnoreUpdate(transaction)) return; + const markdown = getEditorMarkdown(editor); + if (guards.registerEmitted(markdown)) emitted[index]!.push(markdown); + }, + }); + editors[index] = editor; + guardsRef.current = useCollabReconcile({ + editor, + ydoc: docs[index], + awareness: awareness[index], + collabSynced: true, + value: props.value, + contentUpdatedAt: props.contentUpdatedAt, + contentRevision: props.contentRevision, + onBaseAwareReconcile: (result) => + reconciled.push({ status: result.status, content: result.content }), + getMarkdown: (editorToRead) => getEditorMarkdown(editorToRead), + initialAppliedUpdatedAt: null, + editable: true, + }); + return React.createElement("div"); + } + function Harness(props: HarnessProps) { + return React.createElement( + React.Fragment, + null, + React.createElement(Probe, { ...props, index: 0 }), + React.createElement(Probe, { ...props, index: 1 }), + ); + } + return { + Harness, + editors, + emitted, + reconciled, + markdown: () => editors.map((editor) => getEditorMarkdown(editor!)), + dispose: () => { + act(() => root.unmount()); + root = createRoot(container); + awareness.forEach((state) => state.destroy()); + docs.forEach((doc) => doc.destroy()); + }, + }; +} + function render( root: Root, Harness: (p: HarnessProps) => React.ReactElement, @@ -218,6 +386,561 @@ function render( } describe("useCollabReconcile — concurrent edit / lost-update guards", () => { + it("does not seed an empty editor from a collab-backed SQL snapshot", async () => { + vi.useFakeTimers(); + const harness = makePeerReconcileHarness(""); + const serverDoc = new Y.Doc(); + Y.applyUpdate(serverDoc, Y.encodeStateAsUpdate(harness.ydoc)); + const serverEditor = new CoreEditor({ + extensions: createRichMarkdownExtensions({ + dialect: "gfm", + ydoc: serverDoc, + }), + }); + serverEditor.commands.insertContentAt(1, "Accepted body"); + let finishSync!: (result: { status: "synced" }) => void; + const requestCollabSync = () => + new Promise<{ status: "synced" }>((resolve) => { + finishSync = resolve; + }); + try { + act(() => + root.render( + React.createElement(harness.Harness, { + value: "Accepted body", + revision: "revision-2", + collabContentRevision: "revision-2", + requestCollabSync, + }), + ), + ); + await act(async () => vi.advanceTimersByTimeAsync(30000)); + expect(harness.markdown()).toBe(""); + expect(harness.writes).toEqual([]); + act(() => + Y.applyUpdate(harness.ydoc, Y.encodeStateAsUpdate(serverDoc), "remote"), + ); + await act(async () => finishSync({ status: "synced" })); + expect(harness.markdown()).toBe("Accepted body"); + expect(harness.writes).toEqual([]); + } finally { + serverEditor.destroy(); + serverDoc.destroy(); + harness.dispose(); + } + }); + + it.each([ + [false, false], + [true, false], + [true, true], + ])( + "receives a collab-backed revision exactly once without SQL fallback (local tail: %s, sync failure: %s)", + async (localTail, syncFailure) => { + vi.useFakeTimers(); + const baseline = "original body\n\nSecond paragraph."; + const harness = makePeerReconcileHarness(baseline); + const serverDoc = new Y.Doc(); + Y.applyUpdate(serverDoc, Y.encodeStateAsUpdate(harness.ydoc)); + const serverEditor = new CoreEditor({ + extensions: createRichMarkdownExtensions({ + dialect: "gfm", + ydoc: serverDoc, + }), + }); + let finishSync!: (result: { status: "synced" }) => void; + const requestSync = vi.fn<() => Promise<{ status: "synced" | "failed" }>>( + () => + new Promise((resolve) => { + finishSync = resolve; + }), + ); + if (syncFailure) requestSync.mockResolvedValueOnce({ status: "failed" }); + try { + act(() => root.render(React.createElement(harness.Harness))); + await act(async () => vi.advanceTimersByTimeAsync(30)); + const stateVector = Y.encodeStateVector(harness.ydoc); + serverEditor.commands.insertContentAt(1, "Accepted "); + if (localTail) { + act(() => + harness + .editor() + .commands.insertContentAt( + harness.editor().state.doc.content.size - 1, + " local tail", + ), + ); + } + const props = { + value: `Accepted ${baseline}`, + revision: "revision-2", + collabContentRevision: "revision-2", + requestCollabSync: requestSync, + baseAware: true, + }; + act(() => root.render(React.createElement(harness.Harness, props))); + await act(async () => vi.advanceTimersByTimeAsync(30000)); + act(() => + root.render( + React.createElement(harness.Harness, { + ...props, + callbackVersion: 1, + }), + ), + ); + expect(requestSync).toHaveBeenCalledTimes(syncFailure ? 2 : 1); + expect(harness.writes).toEqual([]); + expect(harness.markdown()).toBe( + localTail ? `${baseline} local tail` : baseline, + ); + act(() => + Y.applyUpdate( + harness.ydoc, + Y.encodeStateAsUpdate(serverDoc, stateVector), + "remote", + ), + ); + await act(async () => vi.advanceTimersByTimeAsync(30000)); + expect(harness.markdown()).toBe( + `Accepted ${baseline}${localTail ? " local tail" : ""}`, + ); + expect(harness.writes).toEqual([]); + // A partial cache update can retain the old marker; a different body + // token must still take the ordinary SQL reconciliation path. + act(() => + root.render( + React.createElement(harness.Harness, { + ...props, + value: `Revised ${baseline}`, + revision: "revision-3", + }), + ), + ); + await act(async () => vi.advanceTimersByTimeAsync(3000)); + expect(harness.writes).toEqual([]); + expect(harness.reconciled).toEqual([]); + expect(harness.markdown()).toBe( + `Accepted ${baseline}${localTail ? " local tail" : ""}`, + ); + await act(async () => finishSync({ status: "synced" })); + await act(async () => vi.advanceTimersByTimeAsync(3000)); + expect(harness.markdown()).toBe( + `Revised ${baseline}${localTail ? " local tail" : ""}`, + ); + if (localTail) + expect(harness.reconciled).toEqual([ + expect.objectContaining({ + status: "merged", + baseRevision: "revision-2", + }), + ]); + } finally { + serverEditor.destroy(); + serverDoc.destroy(); + harness.dispose(); + } + }, + ); + it.each([true, false])( + "preserves ordinary mark removal before SQL catches up (author leads: %s)", + async (authorLeads) => { + const harness = makeConnectedEditorHarness(authorLeads); + const baseline = "***Bold*** sample."; + const props = { + value: baseline, + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + contentRevision: "revision-1", + }; + vi.useFakeTimers(); + try { + render(root, harness.Harness, props); + await act(async () => vi.advanceTimersByTimeAsync(30)); + expect(harness.markdown()).toEqual([baseline, baseline]); + act(() => { + harness.editors[0]!.chain() + .setTextSelection({ from: 1, to: 5 }) + .toggleItalic() + .run(); + }); + expect(harness.markdown()).toEqual([ + "**Bold** sample.", + "**Bold** sample.", + ]); + expect(harness.emitted).toEqual([["**Bold** sample."], []]); + render(root, harness.Harness, props); + await act(async () => vi.advanceTimersByTimeAsync(30)); + expect(harness.markdown()).toEqual([ + "**Bold** sample.", + "**Bold** sample.", + ]); + expect(harness.emitted).toEqual([["**Bold** sample."], []]); + } finally { + harness.dispose(); + } + }, + ); + + it.each([false, true])( + "merges newer authority after a remote mark change (timestamp ties: %s)", + async (timestampTies) => { + const harness = makeConnectedEditorHarness(false); + const baseline = "***Bold*** sample.\n\nSecond line."; + const props = { + value: baseline, + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + contentRevision: "revision-1", + }; + vi.useFakeTimers(); + try { + render(root, harness.Harness, props); + await act(async () => vi.advanceTimersByTimeAsync(30)); + act(() => { + harness.editors[0]!.chain() + .setTextSelection({ from: 1, to: 5 }) + .toggleItalic() + .run(); + }); + render(root, harness.Harness, { + value: "***Bold*** sample.\n\nServer line.", + contentUpdatedAt: timestampTies + ? props.contentUpdatedAt + : "2024-01-01T00:00:02.000Z", + contentRevision: "revision-2", + }); + await act(async () => vi.advanceTimersByTimeAsync(2501)); + expect(harness.markdown()).toEqual([ + "**Bold** sample.\n\nServer line.", + "**Bold** sample.\n\nServer line.", + ]); + expect(harness.reconciled).toEqual([ + { status: "merged", content: "**Bold** sample.\n\nServer line." }, + ]); + } finally { + harness.dispose(); + } + }, + ); + + it("uses the latest callback at the original peer deadline, including the default normalizer", async () => { + const harness = makePeerReconcileHarness(); + vi.useFakeTimers(); + try { + act(() => root.render(React.createElement(harness.Harness))); + await act(async () => vi.advanceTimersByTimeAsync(30)); + for ( + let callbackVersion = 1; + callbackVersion <= 5; + callbackVersion += 1 + ) { + act(() => + root.render( + React.createElement(harness.Harness, { + value: "accepted body", + revision: "revision-2", + callbackVersion, + }), + ), + ); + await act(async () => vi.advanceTimersByTimeAsync(500)); + } + await act(async () => vi.advanceTimersByTimeAsync(1)); + expect(harness.writes).toEqual([ + { value: "accepted body", callbackVersion: 5 }, + ]); + expect(harness.markdown()).toBe("accepted body"); + await act(async () => vi.advanceTimersByTimeAsync(5000)); + expect(harness.writes).toHaveLength(1); + } finally { + harness.dispose(); + } + }); + + it("cancels an obsolete snapshot and starts the peer window for a newer revision", async () => { + const harness = makePeerReconcileHarness(); + vi.useFakeTimers(); + try { + act(() => root.render(React.createElement(harness.Harness))); + await act(async () => vi.advanceTimersByTimeAsync(30)); + act(() => + root.render( + React.createElement(harness.Harness, { + value: "accepted body", + revision: "revision-2", + callbackVersion: 1, + }), + ), + ); + await act(async () => vi.advanceTimersByTimeAsync(2000)); + act(() => + root.render( + React.createElement(harness.Harness, { + value: "newer body", + revision: "revision-3", + callbackVersion: 2, + }), + ), + ); + await act(async () => vi.advanceTimersByTimeAsync(501)); + expect(harness.writes).toEqual([]); + expect(harness.markdown()).toBe("original body"); + await act(async () => vi.advanceTimersByTimeAsync(2000)); + expect(harness.writes).toEqual([ + { value: "newer body", callbackVersion: 2 }, + ]); + expect(harness.markdown()).toBe("newer body"); + } finally { + harness.dispose(); + } + }); + + it("cancels a peer reconciliation on unmount", async () => { + const harness = makePeerReconcileHarness(); + vi.useFakeTimers(); + try { + act(() => root.render(React.createElement(harness.Harness))); + await act(async () => vi.advanceTimersByTimeAsync(30)); + act(() => + root.render( + React.createElement(harness.Harness, { + value: "accepted body", + revision: "revision-2", + }), + ), + ); + await act(async () => vi.advanceTimersByTimeAsync(1000)); + act(() => root.render(null)); + await act(async () => vi.advanceTimersByTimeAsync(3000)); + expect(harness.writes).toEqual([]); + } finally { + harness.dispose(); + } + }); + + it("starts a fresh peer window when the same editor returns after being unavailable", async () => { + const harness = makePeerReconcileHarness(); + vi.useFakeTimers(); + const snapshot = { value: "accepted body", revision: "revision-2" }; + try { + act(() => root.render(React.createElement(harness.Harness))); + await act(async () => vi.advanceTimersByTimeAsync(30)); + act(() => root.render(React.createElement(harness.Harness, snapshot))); + await act(async () => vi.advanceTimersByTimeAsync(1000)); + act(() => + root.render( + React.createElement(harness.Harness, { + ...snapshot, + available: false, + }), + ), + ); + await act(async () => vi.advanceTimersByTimeAsync(2000)); + expect(harness.writes).toEqual([]); + act(() => root.render(React.createElement(harness.Harness, snapshot))); + await act(async () => vi.advanceTimersByTimeAsync(2499)); + expect(harness.writes).toEqual([]); + await act(async () => vi.advanceTimersByTimeAsync(2)); + expect(harness.markdown()).toBe("accepted body"); + } finally { + harness.dispose(); + } + }); + + it("cancels the peer deadline when leadership is lost and waits again after regaining it", async () => { + const harness = makePeerReconcileHarness(); + vi.useFakeTimers(); + try { + act(() => root.render(React.createElement(harness.Harness))); + await act(async () => vi.advanceTimersByTimeAsync(30)); + act(() => + root.render( + React.createElement(harness.Harness, { + value: "accepted body", + revision: "revision-2", + }), + ), + ); + await act(async () => vi.advanceTimersByTimeAsync(1000)); + act(() => { + harness.awareness + .getStates() + .set(1, { user: { name: "Lead peer" }, visible: true }); + harness.awareness.emit("change", [ + { added: [1], updated: [], removed: [] }, + "remote", + ]); + }); + await act(async () => vi.advanceTimersByTimeAsync(3000)); + expect(harness.writes).toEqual([]); + act(() => { + harness.awareness.getStates().delete(1); + harness.awareness.emit("change", [ + { added: [], updated: [], removed: [1] }, + "remote", + ]); + }); + await act(async () => vi.advanceTimersByTimeAsync(2499)); + expect(harness.writes).toEqual([]); + await act(async () => vi.advanceTimersByTimeAsync(2)); + expect(harness.markdown()).toBe("accepted body"); + } finally { + harness.dispose(); + } + }); + + it.each([false, true])( + "adopts an accepted canonical revision during repeated renders (fresh callbacks: %s)", + async (freshCallbacks) => { + const liveYdoc = new Y.Doc(); + liveYdoc.clientID = 1; + const persistedEditor = new CoreEditor({ + extensions: createRichMarkdownExtensions({ + dialect: "gfm", + ydoc: liveYdoc, + }), + }); + persistedEditor.commands.setContent("**Bold** sample"); + persistedEditor.destroy(); + const awareness = new Awareness(liveYdoc); + awareness.getStates().set(2, { + user: { name: "Peer" }, + visible: true, + canFlushDocument: true, + }); + let capturedEditor: Editor | null = null; + const stableRead = (editor: Editor) => getEditorMarkdown(editor); + const normalizeValue = (value: string) => value; + const stableWrite = (editor: Editor, value: string) => { + editor.commands.setContent(value); + }; + function Probe({ accepted }: { accepted: boolean }) { + const editor = useEditor({ + extensions: createRichMarkdownExtensions({ + dialect: "gfm", + ydoc: liveYdoc, + }), + }); + capturedEditor = editor; + useCollabReconcile({ + editor, + ydoc: liveYdoc, + awareness, + collabSynced: true, + value: accepted ? "**Changed** sample" : "**Bold** sample", + contentUpdatedAt: accepted + ? "2024-01-01T00:00:02.000Z" + : "2024-01-01T00:00:01.000Z", + editable: true, + initialAppliedUpdatedAt: null, + normalizeValue, + getMarkdown: freshCallbacks + ? (editor) => stableRead(editor) + : stableRead, + setContent: freshCallbacks + ? (editor, value) => stableWrite(editor, value) + : stableWrite, + }); + return React.createElement("div", null); + } + vi.useFakeTimers(); + try { + act(() => root.render(React.createElement(Probe, { accepted: false }))); + await act(async () => vi.advanceTimersByTimeAsync(30)); + expect(getEditorMarkdown(capturedEditor!)).toBe("**Bold** sample"); + const originalEditor = capturedEditor; + act(() => root.render(React.createElement(Probe, { accepted: true }))); + for (let renderIndex = 0; renderIndex < 6; renderIndex += 1) { + await act(async () => vi.advanceTimersByTimeAsync(500)); + act(() => + root.render(React.createElement(Probe, { accepted: true })), + ); + } + expect(capturedEditor).toBe(originalEditor); + expect(getEditorMarkdown(capturedEditor!)).toBe("**Changed** sample"); + } finally { + act(() => root.unmount()); + root = createRoot(container); + awareness.destroy(); + liveYdoc.destroy(); + } + }, + ); + + it.each([false, true])( + "acknowledges peer acceptance before a later local edit (arrival during apply: %s)", + async (duringApply) => { + const { captured, Harness } = makeHarness(); + const baseline = + "Writers review exact changes.\n\nReaders retain context."; + const accepted = baseline.replace("exact", "careful"); + const props = { + value: accepted, + contentUpdatedAt: "2024-01-01T00:00:02.000Z", + contentRevision: "revision-2", + }; + render(root, Harness, { + value: baseline, + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + contentRevision: "revision-1", + }); + await flush(); + const deliverPeer = () => { + act(() => + captured.editor!.commands.setContent(accepted, { emitUpdate: false }), + ); + }; + if (!duringApply) deliverPeer(); + render(root, Harness, props); + if (duringApply) deliverPeer(); + await flush(); + expect(getEditorMarkdown(captured.editor!)).toBe(accepted); + expect(captured.reconciled ?? []).toEqual([]); + + const draft = `${accepted} Peer suffix.`; + act(() => captured.editor!.commands.setContent(draft)); + render(root, Harness, props); + await flush(); + expect(getEditorMarkdown(captured.editor!)).toBe(draft); + expect(captured.reconciled ?? []).toEqual([]); + }, + ); + + it("persists a local suffix when SQL acceptance arrives after peer text", async () => { + const { captured, Harness } = makeHarness(); + const baseline = + "Writers review precise changes.\n\nReaders retain context. Peer suffix."; + const accepted = baseline.replace("precise", "clearse"); + render(root, Harness, { + value: baseline, + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + contentRevision: "revision-1", + }); + await flush(); + act(() => + captured.editor!.commands.setContent(accepted, { emitUpdate: false }), + ); + const draft = `${accepted} Trace suffix.`; + act(() => + captured.editor!.commands.insertContentAt( + captured.editor!.state.doc.content.size - 1, + " Trace suffix.", + ), + ); + const before = captured.editor!.state.doc; + const props = { + value: accepted, + contentUpdatedAt: "2024-01-01T00:00:02.000Z", + contentRevision: "revision-2", + }; + render(root, Harness, props); + await flush(); + expect(captured.editor!.state.doc).toBe(before); + expect(getEditorMarkdown(captured.editor!)).toBe(draft); + expect(captured.reconciled).toEqual([{ status: "merged", content: draft }]); + render(root, Harness, props); + await flush(); + expect(captured.reconciled).toHaveLength(1); + }); + it("merges a newer non-overlapping revision and reports the combined draft", async () => { const { captured, Harness } = makeHarness(); render(root, Harness, { @@ -381,6 +1104,139 @@ describe("useCollabReconcile — concurrent edit / lost-update guards", () => { expect(getEditorMarkdown(captured.editor!)).toBe("seeded content"); }); + it("preserves a registered local collab mark over an older-or-equal controlled snapshot", async () => { + const canonical = "[Link](https://example.com/second) sample."; + const { captured, Harness } = makeCollabSeedHarness(canonical); + const props = { + collabSynced: true, + fragmentLength: 1, + value: canonical, + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + initialAppliedUpdatedAt: null, + }; + act(() => root.render(React.createElement(Harness, props))); + await act(async () => new Promise((resolve) => setTimeout(resolve, 30))); + + act(() => { + captured + .editor!.chain() + .setTextSelection({ from: 1, to: 5 }) + .unsetLink() + .run(); + }); + expect(captured.emitted.at(-1)).toBe("Link sample."); + + act(() => root.render(React.createElement(Harness, props))); + await flush(); + + expect(getEditorMarkdown(captured.editor!)).toBe("Link sample."); + expect(captured.setContentCalls).toBe(0); + }); + + it("still applies newer authority after a registered local collab mark", async () => { + const canonical = "[Link](https://example.com/second) sample."; + const { captured, Harness } = makeCollabSeedHarness(canonical); + act(() => + root.render( + React.createElement(Harness, { + collabSynced: true, + fragmentLength: 1, + value: canonical, + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + initialAppliedUpdatedAt: null, + }), + ), + ); + await act(async () => new Promise((resolve) => setTimeout(resolve, 30))); + act(() => { + captured.editor!.commands.setContent("Link sample."); + }); + expect(captured.emitted.at(-1)).toBe("Link sample."); + + act(() => + root.render( + React.createElement(Harness, { + collabSynced: true, + fragmentLength: 1, + value: "[Link](https://example.com/server) sample.", + contentUpdatedAt: "2024-01-01T00:00:02.000Z", + initialAppliedUpdatedAt: null, + }), + ), + ); + await flush(); + + expect(getEditorMarkdown(captured.editor!)).toBe( + "[Link](https://example.com/server) sample.", + ); + }); + + it("still reconciles a changed authority revision when its timestamp ties", async () => { + const canonical = "Alpha\n\nBravo"; + const { captured, Harness } = makeCollabSeedHarness(canonical); + act(() => + root.render( + React.createElement(Harness, { + collabSynced: true, + fragmentLength: 1, + value: canonical, + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + contentRevision: "revision-1", + initialAppliedUpdatedAt: null, + }), + ), + ); + await act(async () => new Promise((resolve) => setTimeout(resolve, 30))); + act(() => { + captured.editor!.commands.setContent("Alpha local\n\nBravo"); + }); + expect(captured.emitted.at(-1)).toBe("Alpha local\n\nBravo"); + + act(() => + root.render( + React.createElement(Harness, { + collabSynced: true, + fragmentLength: 1, + value: "Alpha\n\nBravo server", + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + contentRevision: "revision-2", + initialAppliedUpdatedAt: null, + }), + ), + ); + await flush(); + + expect(captured.reconciled).toEqual([ + { + status: "merged", + content: "Alpha local\n\nBravo server", + }, + ]); + expect(getEditorMarkdown(captured.editor!)).toBe( + "Alpha local\n\nBravo server", + ); + }); + + it("still clears a stale collab value when there is no local emission", async () => { + const { captured, Harness } = makeCollabSeedHarness("stale persisted body"); + act(() => + root.render( + React.createElement(Harness, { + collabSynced: true, + fragmentLength: 1, + value: "canonical SQL body", + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + initialAppliedUpdatedAt: null, + }), + ), + ); + expect(getEditorMarkdown(captured.editor!)).toBe("stale persisted body"); + await act(async () => new Promise((resolve) => setTimeout(resolve, 30))); + + expect(getEditorMarkdown(captured.editor!)).toBe("canonical SQL body"); + expect(captured.emitted).toEqual([]); + }); + it("uses the collaborative seed as the base for a later three-way merge", async () => { const { captured, Harness } = makeCollabSeedHarness(); act(() => { @@ -872,6 +1728,46 @@ describe("useCollabReconcile — concurrent edit / lost-update guards", () => { expect(getEditorMarkdown(captured.editor!)).toBe("# Doc updated by agent"); }); + it("does not roll back a blurred local mark while its controlled echo is queued", async () => { + const { captured, Harness } = makeHarness(); + const canonical = "[Link](https://example.com/first) sample."; + let editorFocused = false; + const isEditorFocused = () => editorFocused; + const props = { + value: canonical, + contentUpdatedAt: "2024-01-01T00:00:01.000Z", + isEditorFocused, + }; + + render(root, Harness, props); + await flush(); + expect(captured.setContentCalls).toBe(0); + + act(() => { + captured + .editor!.chain() + .setTextSelection({ from: 1, to: 5 }) + .unsetLink() + .run(); + }); + const localDraft = captured.emitted.at(-1)!; + expect(localDraft).toBe("Link sample."); + + // A sibling-state render can arrive before the host's deferred onChange. + // The toolbar input still owns focus when reconcile observes the stale + // controlled value; editor focus returns before its timer applies. + render(root, Harness, props); + editorFocused = true; + await flush(); + + expect(getEditorMarkdown(captured.editor!)).toBe(localDraft); + expect(captured.setContentCalls).toBe(0); + + render(root, Harness, { ...props, value: localDraft }); + await flush(); + expect(getEditorMarkdown(captured.editor!)).toBe(localDraft); + }); + it("persists a non-lead client's own local edit to a nonempty shared document", async () => { // Losing the lead election only bars this client from SEEDING. Its own // typing still has to reach the app's persist path, or the text lives in diff --git a/packages/toolkit/src/editor/useCollabReconcile.ts b/packages/toolkit/src/editor/useCollabReconcile.ts index bc4390221c2..74d31aa6534 100644 --- a/packages/toolkit/src/editor/useCollabReconcile.ts +++ b/packages/toolkit/src/editor/useCollabReconcile.ts @@ -63,6 +63,12 @@ export interface UseCollabReconcileOptions { contentUpdatedAt?: string | null; /** Opaque authoritative body revision. Enables base-aware reconciliation. */ contentRevision?: string | null; + /** This exact body revision is already represented in durable Yjs state. */ + collabContentRevision?: string | null; + /** Resolves as synced only after a fresh provider response is applied. */ + requestCollabSync?: () => Promise<{ + status: "synced" | "failed" | "unavailable"; + }>; /** Reports an automatic merge to persist, or a conflict whose local draft was preserved. */ onBaseAwareReconcile?: (result: { status: "merged" | "conflict" | "failed"; @@ -246,6 +252,8 @@ export function useCollabReconcile({ value, contentUpdatedAt, contentRevision, + collabContentRevision, + requestCollabSync, onBaseAwareReconcile, editable, isEditorFocused = defaultIsEditorFocused, @@ -257,8 +265,12 @@ export function useCollabReconcile({ initialAppliedUpdatedAt, }: UseCollabReconcileOptions): UseCollabReconcileResult { const collab = !!ydoc; + const collabBackedSnapshot = Boolean( + collab && contentRevision && collabContentRevision === contentRevision, + ); const isSettingContentRef = useRef(false); const lastEmittedRef = useRef(""); + const lastRegisteredLocalEmissionRef = useRef(null); // Ring of recent local emissions (see pushEmittedRing). Lets the reconcile // recognize a stale-but-recent echo of our OWN (possibly partial, debounced) // save so a lagging poll never clobbers freshly-typed text. @@ -290,6 +302,70 @@ export function useCollabReconcile({ revision: string; } | null>(contentRevision ? { value, revision: contentRevision } : null); const reportedConflictRevisionRef = useRef(null); + const acknowledgedCollabRef = useRef<{ ydoc: YDoc; revision: string } | null>( + null, + ); + const [pendingCollabSnapshot, setPendingCollabSnapshot] = useState<{ + ydoc: YDoc; + revision: string; + value: string; + updatedAt: string | null | undefined; + } | null>(null); + useEffect(() => { + if (!collabBackedSnapshot || !ydoc || !contentRevision) return; + if ( + acknowledgedCollabRef.current?.ydoc === ydoc && + acknowledgedCollabRef.current.revision === contentRevision + ) + return; + setPendingCollabSnapshot((pending) => + pending?.ydoc === ydoc && pending.revision === contentRevision + ? pending + : { + ydoc, + revision: contentRevision, + value, + updatedAt: contentUpdatedAt, + }, + ); + }, [collabBackedSnapshot, ydoc, contentRevision, value, contentUpdatedAt]); + useEffect(() => { + if ( + !pendingCollabSnapshot || + !requestCollabSync || + pendingCollabSnapshot.ydoc !== ydoc + ) + return; + let cancelled = false; + let retry: ReturnType | undefined; + const sync = async () => { + const result = await requestCollabSync().catch(() => ({ + status: "failed" as const, + })); + if (cancelled) return; + if (result.status !== "synced") { + retry = setTimeout(() => void sync(), 2000); + return; + } + // The fetched CRDT includes this canonical revision even when unsynced + // local edits make the editor differ from its SQL body. Advance only the + // merge base, never rewrite those local edits to manufacture equality. + authoritativeBaseRef.current = { + value: pendingCollabSnapshot.value, + revision: pendingCollabSnapshot.revision, + }; + reportedConflictRevisionRef.current = null; + if (pendingCollabSnapshot.updatedAt) + lastAppliedUpdatedAtRef.current = pendingCollabSnapshot.updatedAt; + acknowledgedCollabRef.current = pendingCollabSnapshot; + setPendingCollabSnapshot(null); + }; + void sync(); + return () => { + cancelled = true; + if (retry) clearTimeout(retry); + }; + }, [pendingCollabSnapshot, requestCollabSync, ydoc]); // Whether THIS client is the one that seeds the empty shared doc / applies an // authoritative external snapshot into it. Exactly one client does, so the @@ -347,6 +423,10 @@ export function useCollabReconcile({ if (!collab || !editor || editor.isDestroyed || !ydoc) return; if (seededRef.current) return; if (!collabSynced) return; + if (collabBackedSnapshot) { + seededRef.current = true; + return; + } if (contentRevision) { authoritativeBaseRef.current = { value, revision: contentRevision }; reportedConflictRevisionRef.current = null; @@ -457,16 +537,57 @@ export function useCollabReconcile({ getMarkdown, setContent, shouldSeed, + collabBackedSnapshot, ]); + const peerReconcileWaitRef = useRef<{ + editor: Editor; + ydoc: YDoc | null; + value: string; + contentUpdatedAt: string | null | undefined; + contentRevision: string | null | undefined; + collabSynced: boolean; + isLeadClient: boolean; + editable: boolean; + deadline: number | null; + } | null>(null); + // Reconcile authoritative external markdown (agent edit, source patch, or a // peer edit mirrored to SQL) into the live editor. In collab mode only the // lead client applies it through setContent; Yjs propagates the result to // every other client. In non-collab mode this is the original controlled-value // reconcile, unchanged. useEffect(() => { - if (!editor || editor.isDestroyed) return; + if (!editor || editor.isDestroyed) { + peerReconcileWaitRef.current = null; + return; + } + const previousWait = peerReconcileWaitRef.current; + if ( + !previousWait || + previousWait.editor !== editor || + previousWait.ydoc !== ydoc || + previousWait.value !== value || + previousWait.contentUpdatedAt !== contentUpdatedAt || + previousWait.contentRevision !== contentRevision || + previousWait.collabSynced !== collabSynced || + previousWait.isLeadClient !== isLeadClient || + previousWait.editable !== editable + ) { + peerReconcileWaitRef.current = { + editor, + ydoc, + value, + contentUpdatedAt, + contentRevision, + collabSynced, + isLeadClient, + editable, + deadline: null, + }; + } + const peerWait = peerReconcileWaitRef.current!; let cancelled = false; let retry: ReturnType | null = null; // With peers present, a peer's edit also arrives via Yjs. Defer one poll @@ -492,6 +613,16 @@ export function useCollabReconcile({ retry = setTimeout(() => apply(deferred), 50); return; } + // SQL and Yjs describe the same committed operation here. Reapplying SQL + // would create different CRDT insert identities and duplicate the text. + // Provider retries, not a timed SQL fallback, own delayed delivery. + if ( + collabBackedSnapshot || + (collab && pendingCollabSnapshot?.ydoc === ydoc) + ) { + peerWait.deadline = null; + return; + } const currentMarkdown = getMarkdown(editor); // Compare against the canonical form the editor would emit so a serializer // that re-normalizes (Content's NFM) still recognizes "already in sync". @@ -542,6 +673,15 @@ export function useCollabReconcile({ (value === lastAppliedValueRef.current || normalizedValue === lastAppliedSerializedRef.current)) ) { + peerWait.deadline = null; + // Equality on the first controlled render is also a successful apply: + // useEditor may already have initialized from `value`. Record that + // baseline so a same-revision parent render cannot restore stale props + // over a local edit made while a toolbar or popover owns focus. + if (currentMarkdown === normalizedValue) { + lastAppliedValueRef.current = value; + lastAppliedSerializedRef.current = currentMarkdown; + } if (contentRevision) { authoritativeBaseRef.current = { value, revision: contentRevision }; reportedConflictRevisionRef.current = null; @@ -552,7 +692,14 @@ export function useCollabReconcile({ return; } + const revisionChangedAtSameTimestamp = + !!contentRevision && + !!authoritativeBaseRef.current && + contentRevision !== authoritativeBaseRef.current.revision && + !!contentUpdatedAt && + contentUpdatedAt === lastAppliedUpdatedAtRef.current; const externalNewer = + revisionChangedAtSameTimestamp || !lastAppliedUpdatedAtRef.current || !contentUpdatedAt || contentUpdatedAt > lastAppliedUpdatedAtRef.current; @@ -560,6 +707,7 @@ export function useCollabReconcile({ // Only the lead client applies an authoritative snapshot into the shared // Y.Doc; peers receive it through Yjs sync. if (collab && !isLeadClient) { + peerWait.deadline = null; if (contentUpdatedAt && !externalNewer) { lastAppliedUpdatedAtRef.current = contentUpdatedAt; } @@ -575,31 +723,46 @@ export function useCollabReconcile({ if (typingRecently) { if (externalNewer) { retry = setTimeout(() => apply(deferred), 700); + } else { + peerWait.deadline = null; } return; } - // Older-or-equal content is a stale poll / lagging echo. Drop it while - // focused (a peer/agent edit would be NEWER and retries above). In - // NON-COLLAB mode there is no peer, so older-or-equal external content is - // ALWAYS stale — dropping it regardless of focus stops a lagging - // `get-visual-plan` poll from reverting a just-applied local structural - // change (drag-to-columns) while the editor is blurred (the drag grips the - // handle, not the prose, so `isFocused` is false at drop time). Gated on - // `lastAppliedSerializedRef` so the very first seed (nothing applied yet, - // also not-newer) still lands. - const seeded = lastAppliedSerializedRef.current !== null; - if (!externalNewer && (editorFocused || (!collab && seeded))) return; + // Once an authoritative snapshot has been applied, an unchanged or older + // SQL echo cannot overwrite subsequent local OR remote Yjs edits. The + // idle lead can receive a peer's edit before that peer's SQL save arrives. + // A fresh mount still reconciles stale CRDT state before it has a baseline. + const hasAppliedSnapshot = lastAppliedSerializedRef.current !== null; + const currentIsRegisteredLocalEmission = + lastRegisteredLocalEmissionRef.current !== null && + currentMarkdown === lastRegisteredLocalEmissionRef.current; + if ( + !externalNewer && + (editorFocused || + hasAppliedSnapshot || + currentIsRegisteredLocalEmission) + ) { + peerWait.deadline = null; + return; + } // Race guard: with peers present, let Yjs deliver a peer's edit first. // Defer once and re-check — a peer edit makes the equality check above // no-op next pass; an agent/source edit still differs and applies. if (collab && externalNewer && !deferred && peerCountRef.current > 0) { - retry = setTimeout(() => apply(true), PEER_SETTLE_MS); - return; + // Inline serializers can change on every presence/poll render. Keep + // this snapshot's deadline while the effect refreshes its callbacks. + peerWait.deadline ??= Date.now() + PEER_SETTLE_MS; + const remaining = peerWait.deadline - Date.now(); + if (remaining > 0) { + retry = setTimeout(() => apply(true), remaining); + return; + } } const applyTimer = setTimeout(() => { if (cancelled || editor.isDestroyed) return; + peerWait.deadline = null; // Re-check doc-equivalence at apply time. Between the decision above and // this task a peer/Yjs edit (or our own prior apply) may have made // the editor already represent this value — re-applying would be a @@ -617,6 +780,11 @@ export function useCollabReconcile({ normalized === lastAppliedSerializedRef.current) ) { lastAppliedValueRef.current = value; + lastAppliedSerializedRef.current = beforeMarkdown; + if (contentRevision) { + authoritativeBaseRef.current = { value, revision: contentRevision }; + reportedConflictRevisionRef.current = null; + } if (contentUpdatedAt) { lastAppliedUpdatedAtRef.current = contentUpdatedAt; } @@ -680,7 +848,7 @@ export function useCollabReconcile({ lastAppliedSerializedRef.current = merged; if (contentUpdatedAt) lastAppliedUpdatedAtRef.current = contentUpdatedAt; - if (reconciled.status === "applied" && merged !== normalized) { + if (merged !== normalized) { onBaseAwareReconcile({ status: "merged", content: merged, @@ -738,6 +906,8 @@ export function useCollabReconcile({ contentUpdatedAt, contentRevision, editor, + ydoc, + editable, value, collab, collabSynced, @@ -748,6 +918,8 @@ export function useCollabReconcile({ normalizeValue, isEditorFocused, onBaseAwareReconcile, + collabBackedSnapshot, + pendingCollabSnapshot, ]); const shouldIgnoreUpdate = (transaction: Transaction): boolean => { @@ -790,6 +962,7 @@ export function useCollabReconcile({ if (collab && !markdown.trim()) return false; lastEmittedRef.current = markdown; pushEmittedRing(recentEmittedRef.current, markdown); + lastRegisteredLocalEmissionRef.current = markdown; return true; }; diff --git a/scripts/qa-standalone-chat-dev-smoke.ts b/scripts/qa-standalone-chat-dev-smoke.ts index 921716a3cbe..9ad4350f690 100644 --- a/scripts/qa-standalone-chat-dev-smoke.ts +++ b/scripts/qa-standalone-chat-dev-smoke.ts @@ -671,6 +671,11 @@ function suppressedNoiseBlock(): string { function isBenignConsoleError(text: string): boolean { if (text.startsWith("Failed to load resource:")) return true; if (text.includes("favicon")) return true; + if ( + text.includes("Encountered a script tag while rendering React component") + ) { + return true; + } return false; } diff --git a/templates/content/.agents/skills/document-editing/SKILL.md b/templates/content/.agents/skills/document-editing/SKILL.md index 0e6d3d61b3b..044b2ae9a7b 100644 --- a/templates/content/.agents/skills/document-editing/SKILL.md +++ b/templates/content/.agents/skills/document-editing/SKILL.md @@ -85,6 +85,31 @@ pnpm action update-document --id abc123 --title "New Title" --content "New conte pnpm action update-document --id abc123 --description "Stable guidance for what belongs on this page" ``` +### Suggested edits + +When the user asks to **suggest**, **propose**, or **leave changes for review**, +do not call `edit-document` or `update-document`. Read the current Page and use +`create-resource-suggestion` with `resourceType: "document"`, adapter kind +`content.document-markdown`, the Page's exact `updatedAt` as `baseRevision`, a +fresh `idempotencyKey`, and one typed Page-body operation. Preserve both the +complete current and proposed Markdown in `before.markdown` and +`after.markdown`; include the narrow changed material and anchor when known. + +Use `list-resource-suggestions` to inspect pending and historical proposals. +Only accept or reject when the user has asked for that decision and the caller +has editor authority; call `decide-resource-suggestion` with a fresh +idempotency key and the suggestion's `baseRevision` as `observedBase`. A stale +result means canonical Content was not overwritten. Suggested edits are +unavailable for local-file, source-owned, externally linked, database-item, or +trashed Pages in this release. + +```bash +pnpm action create-resource-suggestion --resourceType document --resourceId abc123 \ + --adapterKind content.document-markdown --baseRevision '' \ + --idempotencyKey '' --summary 'Suggest edits' \ + --operations '[{"ordinal":0,"kind":"replace_text","targetId":"body","before":{"markdown":"Before","changedText":"Before"},"after":{"markdown":"After","changedText":"After"},"anchor":{"from":0,"to":6,"prefix":"","suffix":""},"schemaVersion":1}]' +``` + ### delete-document Move a document and all its children to Trash. IDs, bodies, hierarchy, and diff --git a/templates/content/AGENTS.md b/templates/content/AGENTS.md index 050b096d310..2d3bddfdf90 100644 --- a/templates/content/AGENTS.md +++ b/templates/content/AGENTS.md @@ -11,7 +11,7 @@ Read the relevant skill before deeper work: - `content` — Markdown/MDX authoring, local folder sources, databases, intake forms, and Slack/A2A artifact replies. - `document-editing` — document and comment actions, screen context and IDs, - common tasks, the data model, and the databases reference. + suggestions, common tasks, the data model, and the databases reference. - `notion-integration` — connected Notion workflows and the raw Notion provider API path. - `creative-context` — cross-app source reuse, pinned packs, provenance, and @@ -85,7 +85,6 @@ Read the relevant skill before deeper work: | `list-content-database-blocks` | List stable blocks and revisions in one exact database row/property | | `mutate-content-database-block` | Insert, update, upsert, delete, or reorder one supported stable block | | `migrate-content-database-rows` | Validate/apply/verify; terminal phases use `manage-content-database-migration` | - Every action carries its own schema, and the rest of the app-specific surface (comments, sharing, databases, Notion, local file sources such as `remove-local-file-source`) is registered too — use `tool-search` instead of diff --git a/templates/content/actions/_database-utils.ts b/templates/content/actions/_database-utils.ts index b134818ef1f..0067cff632b 100644 --- a/templates/content/actions/_database-utils.ts +++ b/templates/content/actions/_database-utils.ts @@ -229,7 +229,10 @@ type DatabaseMembershipRow = { bodyHydrationQueueId?: string | null; }; -type DocumentListRow = Omit; +type DocumentListRow = Omit< + typeof schema.documents.$inferSelect, + "content" | "collabBodyRevision" +>; // Database grids render row metadata and properties. Fetching the document body // here would transfer it only for serializeDocument to replace it with an empty @@ -308,6 +311,7 @@ export function serializeDatabaseMembership( databaseId: row.database.id, databaseDocumentId: row.database.documentId, databaseTitle: row.database.title || "Untitled database", + systemRole: row.database.systemRole, position: row.item.position, sourceId: row.sourceId ?? null, bodyHydration: serializeBodyHydration(row.item, { diff --git a/templates/content/actions/_suggestion-eligibility.ts b/templates/content/actions/_suggestion-eligibility.ts new file mode 100644 index 00000000000..883254b2aa7 --- /dev/null +++ b/templates/content/actions/_suggestion-eligibility.ts @@ -0,0 +1,23 @@ +export const INLINE_DATABASE_SUGGESTION_EXCLUSION = " { - process.env.DATABASE_URL = `pglite:${dbPath}`; + const fixtureUrl = `pglite:${dbPath}`; + vi.stubEnv("APP_NAME", ""); + vi.stubEnv("DATABASE_URL", fixtureUrl); + vi.stubEnv("DATABASE_URL_UNPOOLED", fixtureUrl); + vi.stubEnv("NETLIFY_DATABASE_URL", fixtureUrl); + vi.stubEnv("NETLIFY_DATABASE_URL_UNPOOLED", fixtureUrl); + const { getDatabaseUrl, getRuntimeDatabaseUrl } = + await import("@agent-native/core/db"); + if ( + getDatabaseUrl() !== fixtureUrl || + getRuntimeDatabaseUrl() !== fixtureUrl + ) { + throw new Error( + "Comment submission test requires its isolated fixture database", + ); + } const module = await import("../server/db/index.js"); schema = module.schema; db = module.getDb(); @@ -53,7 +68,11 @@ beforeAll(async () => { add = (await import("./add-comment.js")).default; update = (await import("./update-comment.js")).default; }, 60000); -afterAll(() => rmSync(dbPath, { force: true, recursive: true })); +afterAll(async () => { + await (await import("@agent-native/core/db")).closeDbExec(); + vi.unstubAllEnvs(); + rmSync(dbPath, { force: true, recursive: true }); +}); const create = (args: Record) => (add as any).run({ documentId: "receipt-fixture", diff --git a/templates/content/actions/content-database-lifecycle.db.test.ts b/templates/content/actions/content-database-lifecycle.db.test.ts index 7461723744b..4fcf279dbf4 100644 --- a/templates/content/actions/content-database-lifecycle.db.test.ts +++ b/templates/content/actions/content-database-lifecycle.db.test.ts @@ -1825,7 +1825,7 @@ describe("content database soft-delete actions and reads", () => { ).rejects.toThrow(`Document "${rowDocumentId}" not found`); }); - it("reads one shared private database row's properties without exposing its Files container", async () => { + it("reads one shared private database row's properties without exposing its container", async () => { const db = getDb(); const now = new Date().toISOString(); const { databaseId, databaseDocumentId } = await createDatabase({}); @@ -1970,6 +1970,7 @@ describe("content database soft-delete actions and reads", () => { title: "Shared Personal row", content: "Keep this nonempty Personal body.", accessRole: "editor", + canSuggest: false, databaseMembership: { databaseId: null, databaseDocumentId: null, @@ -1987,6 +1988,7 @@ describe("content database soft-delete actions and reads", () => { parentId: null, content: "Keep this nonempty Personal body.", accessRole: "editor", + canSuggest: false, databaseMembership: { databaseId: null, databaseDocumentId: null, @@ -2004,6 +2006,7 @@ describe("content database soft-delete actions and reads", () => { listed.documents.find((document) => document.id === sharedDocumentId), ).toMatchObject({ parentId: null, + canSuggest: false, databaseMembership: { databaseId: null, databaseDocumentId: null, @@ -2101,6 +2104,213 @@ describe("content database soft-delete actions and reads", () => { ).rejects.toThrow(`No access to document ${databaseDocumentId}`); }); + it("lets a commenter suggest on a shared Page without exposing its private Files container", async () => { + const db = getDb(); + const now = new Date().toISOString(); + const { databaseId } = await createDatabase({ systemRole: "files" }); + const sharedDocumentId = await createDocument({ + title: "Shared Personal Page", + content: "A Page body open to suggestions.", + }); + await db.insert(schema.contentDatabaseItems).values({ + id: nextId("item"), + ownerEmail: OWNER, + databaseId, + documentId: sharedDocumentId, + position: 0, + createdAt: now, + updatedAt: now, + }); + await db.insert(schema.documentShares).values({ + id: nextId("share"), + resourceId: sharedDocumentId, + principalType: "user", + principalId: COLLABORATOR, + role: "commenter", + createdBy: OWNER, + createdAt: now, + }); + + const memberships = await db + .select({ + documentId: schema.contentDatabaseItems.documentId, + systemRole: schema.contentDatabases.systemRole, + }) + .from(schema.contentDatabaseItems) + .innerJoin( + schema.contentDatabases, + eq(schema.contentDatabases.id, schema.contentDatabaseItems.databaseId), + ) + .where(eq(schema.contentDatabaseItems.documentId, sharedDocumentId)); + expect(memberships).toEqual([ + { documentId: sharedDocumentId, systemRole: "files" }, + ]); + + const shared = await runWithRequestContext( + { userEmail: COLLABORATOR }, + () => getDocumentAction.run({ id: sharedDocumentId }), + ); + expect(shared).toMatchObject({ + id: sharedDocumentId, + accessRole: "commenter", + canComment: true, + canSuggest: true, + canEdit: false, + databaseMembership: { + databaseId: null, + databaseDocumentId: null, + databaseTitle: null, + position: null, + }, + }); + expect(shared.databaseMembership).not.toHaveProperty("systemRole"); + + const listed = await runWithRequestContext( + { userEmail: COLLABORATOR }, + () => listDocumentsAction.run({}), + ); + const listedShared = listed.documents.find( + (document) => document.id === sharedDocumentId, + ); + expect(listedShared).toMatchObject({ + accessRole: "commenter", + canComment: true, + canSuggest: true, + canEdit: false, + databaseMembership: { + databaseId: null, + databaseDocumentId: null, + databaseTitle: null, + position: null, + }, + }); + expect(listedShared?.databaseMembership).not.toHaveProperty("systemRole"); + }); + + it("projects suggestion eligibility for text, inline-database, database, and source Pages", async () => { + const db = getDb(); + const now = new Date().toISOString(); + const ordinaryDocumentId = await createDocument({ + title: "Ordinary suggestion Page", + content: "A commenter can suggest a text change here.", + }); + const inlineDocumentId = await createDocument({ + title: "Inline database suggestion exclusion", + }); + const inlineDatabase = await createDatabase({ + hostDocumentId: inlineDocumentId, + ownerBlockId: "inline-eligibility-block", + }); + await db + .update(schema.documents) + .set({ + content: `${"Paragraph before the block. ".repeat(20)}\n\n${inlineDatabaseBlock( + { + blockId: "inline-eligibility-block", + databaseId: inlineDatabase.databaseId, + databaseDocumentId: inlineDatabase.databaseDocumentId, + }, + )}`, + }) + .where(eq(schema.documents.id, inlineDocumentId)); + const fullPageDatabase = await createDatabase({}); + const sourceDocumentId = await createDocument({ + title: "Source-owned suggestion exclusion", + content: "Source-owned content.", + }); + await db + .update(schema.documents) + .set({ + sourceMode: "local-files", + sourceKind: "file", + sourcePath: "source-owned.md", + }) + .where(eq(schema.documents.id, sourceDocumentId)); + + const unrecognizedSourceDocumentIds: string[] = []; + for (const sourceFields of [ + { sourceMode: "legacy-source" }, + { sourceKind: "file" }, + { sourcePath: "source-owned.md" }, + ]) { + const id = await createDocument({ + title: "Unrecognized source exclusion", + }); + await db + .update(schema.documents) + .set(sourceFields) + .where(eq(schema.documents.id, id)); + unrecognizedSourceDocumentIds.push(id); + } + + const documentIds = [ + ordinaryDocumentId, + inlineDocumentId, + fullPageDatabase.databaseDocumentId, + sourceDocumentId, + ...unrecognizedSourceDocumentIds, + ]; + await db.insert(schema.documentShares).values( + documentIds.map((documentId) => ({ + id: nextId("share"), + resourceId: documentId, + principalType: "user" as const, + principalId: COLLABORATOR, + role: "commenter" as const, + createdBy: OWNER, + createdAt: now, + })), + ); + + const direct = await Promise.all( + documentIds.map((id) => + runWithRequestContext({ userEmail: COLLABORATOR }, () => + getDocumentAction.run({ id }), + ), + ), + ); + expect( + direct.map((document) => ({ + id: document.id, + canComment: document.canComment, + canSuggest: document.canSuggest, + })), + ).toEqual([ + { id: ordinaryDocumentId, canComment: true, canSuggest: true }, + { id: inlineDocumentId, canComment: true, canSuggest: false }, + { + id: fullPageDatabase.databaseDocumentId, + canComment: true, + canSuggest: false, + }, + { id: sourceDocumentId, canComment: true, canSuggest: false }, + ...unrecognizedSourceDocumentIds.map((id) => ({ + id, + canComment: true, + canSuggest: false, + })), + ]); + + const listed = await runWithRequestContext( + { userEmail: COLLABORATOR }, + () => listDocumentsAction.run({}), + ); + const listedEligibility = new Map( + listed.documents + .filter((document) => documentIds.includes(document.id)) + .map((document) => [document.id, document.canSuggest]), + ); + expect(listedEligibility).toEqual( + new Map([ + [ordinaryDocumentId, true], + [inlineDocumentId, false], + [fullPageDatabase.databaseDocumentId, false], + [sourceDocumentId, false], + ...unrecognizedSourceDocumentIds.map((id) => [id, false] as const), + ]), + ); + }); + it("rejects restoring a database whose page belongs to another Trash root", async () => { const rootId = await createDocument({ title: "Parent Trash root" }); const { databaseId, databaseDocumentId } = await createDatabase({ diff --git a/templates/content/actions/get-document.ts b/templates/content/actions/get-document.ts index 7e84eda5992..538255e907e 100644 --- a/templates/content/actions/get-document.ts +++ b/templates/content/actions/get-document.ts @@ -2,9 +2,10 @@ import { defineAction } from "@agent-native/core/action"; import { buildDeepLink } from "@agent-native/core/server"; import { getRequestUserEmail } from "@agent-native/core/server/request-context"; import { roleSatisfies } from "@agent-native/core/sharing"; +import { and, eq, isNull, ne } from "drizzle-orm"; import { z } from "zod"; -import { getDb } from "../server/db/index.js"; +import { getDb, schema } from "../server/db/index.js"; import { parseDocumentHideFromSearch } from "../server/lib/documents.js"; import { favoriteDocumentIds } from "./_content-favorites.js"; import { @@ -27,6 +28,10 @@ import { resolvePropertyDatabaseForDocument, serializeDatabase, } from "./_property-utils.js"; +import { + canSuggestDocument, + documentHasInlineDatabase, +} from "./_suggestion-eligibility.js"; function canEditRole(role: string) { return role === "owner" || role === "admin" || role === "editor"; @@ -146,6 +151,61 @@ export default defineAction({ // not the private database document that owns those definitions. requireDatabaseAccess: propertyDatabaseAccess !== null, }); + const source = serializeDocumentSource(doc); + const hasInlineDatabase = documentHasInlineDatabase(doc.content ?? ""); + let isOrdinaryDatabaseItem = false; + let isExternallyLinked = false; + if ( + canCommentRole(access.role) && + !database && + !source?.mode && + !hasInlineDatabase + ) { + const db = getDb(); + const [ordinaryMembership, externalLink] = await Promise.all([ + db + .select({ id: schema.contentDatabaseItems.id }) + .from(schema.contentDatabaseItems) + .innerJoin( + schema.contentDatabases, + eq( + schema.contentDatabases.id, + schema.contentDatabaseItems.databaseId, + ), + ) + .where( + and( + eq(schema.contentDatabaseItems.documentId, doc.id), + isNull(schema.contentDatabases.deletedAt), + isNull(schema.contentDatabases.systemRole), + ), + ) + .limit(1), + db + .select({ documentId: schema.documentSyncLinks.documentId }) + .from(schema.documentSyncLinks) + .where( + and( + eq(schema.documentSyncLinks.documentId, doc.id), + ne(schema.documentSyncLinks.state, "unlinked"), + ), + ) + .limit(1), + ]); + isOrdinaryDatabaseItem = ordinaryMembership.length > 0; + isExternallyLinked = externalLink.length > 0; + } + const canSuggest = canSuggestDocument({ + canComment: canCommentRole(access.role), + isDatabase: Boolean(database), + isOrdinaryDatabaseItem, + isExternallyLinked, + isSourceOwned: Boolean( + doc.sourceMode || doc.sourceKind || doc.sourcePath, + ), + hasInlineDatabase, + }); + const revision = documentRevisionToken(doc.bodyRevision, doc.content ?? ""); return { id: doc.id, @@ -158,9 +218,11 @@ export default defineAction({ databaseMembership && !propertyDatabaseAccess ? null : doc.parentId, title: doc.title, content: doc.content, - revision: documentRevisionToken(doc.bodyRevision, doc.content ?? ""), - baseRevision: documentRevisionToken(doc.bodyRevision, doc.content ?? ""), + revision, + baseRevision: revision, bodyRevision: doc.bodyRevision, + collabContentRevision: + doc.collabBodyRevision === doc.bodyRevision ? revision : null, contentHash: documentContentHash(doc.content ?? ""), description: doc.description, icon: doc.icon, @@ -168,9 +230,10 @@ export default defineAction({ isFavorite: favoriteIds.has(doc.id), hideFromSearch: parseDocumentHideFromSearch(doc.hideFromSearch), visibility: doc.visibility, - source: serializeDocumentSource(doc), + source, accessRole: access.role, canComment: canCommentRole(access.role), + canSuggest, canEdit: canEditRole(access.role), canManage: canManageRole(access.role), database: database diff --git a/templates/content/actions/list-documents.ts b/templates/content/actions/list-documents.ts index 55abc52ce40..f2e0b791c87 100644 --- a/templates/content/actions/list-documents.ts +++ b/templates/content/actions/list-documents.ts @@ -24,6 +24,10 @@ import { } from "./_document-discovery-query.js"; import { serializeDocumentSource } from "./_document-source.js"; import { parseDatabaseViewConfig } from "./_property-utils.js"; +import { + canSuggestDocument, + INLINE_DATABASE_SUGGESTION_EXCLUSION, +} from "./_suggestion-eligibility.js"; function contentPreview(content: string, maxLength = 180) { const compact = content.replace(/\s+/g, " ").trim(); @@ -128,6 +132,7 @@ export default defineAction({ description: schema.documents.description, contentSnippet: sql`substr(${schema.documents.content}, 1, 400)`, contentLength: sql`length(${schema.documents.content})`, + hasInlineDatabase: sql`position(${INLINE_DATABASE_SUGGESTION_EXCLUSION} in ${schema.documents.content}) > 0`, icon: schema.documents.icon, position: schema.documents.position, isFavorite: schema.documents.isFavorite, @@ -151,6 +156,8 @@ export default defineAction({ const shareRoleByDocumentId = new Map(); const notionPageIdByDocumentId = new Map(); + const externallyLinkedDocumentIds = new Set(); + const ordinaryDatabaseItemDocumentIds = new Set(); const databaseByDocumentId = new Map< string, typeof schema.contentDatabases.$inferSelect @@ -200,6 +207,7 @@ export default defineAction({ .select({ documentId: schema.documentSyncLinks.documentId, remotePageId: schema.documentSyncLinks.remotePageId, + state: schema.documentSyncLinks.state, }) .from(schema.documentSyncLinks) .where( @@ -266,6 +274,9 @@ export default defineAction({ for (const link of notionLinks) { notionPageIdByDocumentId.set(link.documentId, link.remotePageId); + if (link.state !== "unlinked") { + externallyLinkedDocumentIds.add(link.documentId); + } } for (const row of shareRows) { @@ -283,6 +294,9 @@ export default defineAction({ } for (const row of databaseMemberships) { + if (row.database.systemRole == null) { + ordinaryDatabaseItemDocumentIds.add(row.item.documentId); + } if (!databaseMembershipByDocumentId.has(row.item.documentId)) { databaseMembershipByDocumentId.set(row.item.documentId, row); } @@ -298,6 +312,7 @@ export default defineAction({ const database = databaseByDocumentId.get(d.id) ?? null; const databaseMembership = databaseMembershipByDocumentId.get(d.id) ?? null; + const source = serializeDocumentSource(d); if (shareRole && ROLE_RANK[shareRole] > ROLE_RANK[accessRole]) { accessRole = shareRole; @@ -327,7 +342,7 @@ export default defineAction({ ? `https://www.notion.so/${notionPageIdByDocumentId.get(d.id)!.replace(/-/g, "")}` : null, visibility: d.visibility, - source: serializeDocumentSource(d), + source, database: database ? { id: database.id, @@ -352,6 +367,14 @@ export default defineAction({ : undefined, accessRole, canComment: canCommentRole(accessRole), + canSuggest: canSuggestDocument({ + canComment: canCommentRole(accessRole), + isDatabase: Boolean(database), + isOrdinaryDatabaseItem: ordinaryDatabaseItemDocumentIds.has(d.id), + isExternallyLinked: externallyLinkedDocumentIds.has(d.id), + isSourceOwned: Boolean(d.sourceMode || d.sourceKind || d.sourcePath), + hasInlineDatabase: d.hasInlineDatabase, + }), canEdit: canEditRole(accessRole), canManage: canManageRole(accessRole), createdAt: d.createdAt, diff --git a/templates/content/actions/update-document.ts b/templates/content/actions/update-document.ts index 9235ca6639f..fd7e1690436 100644 --- a/templates/content/actions/update-document.ts +++ b/templates/content/actions/update-document.ts @@ -15,6 +15,7 @@ import { and, eq } from "drizzle-orm"; import { z } from "zod"; import { getDb, schema } from "../server/db/index.js"; +import { commitCanonicalDocumentBodyMutation } from "../server/lib/canonical-document-body-mutation.js"; import { recordDocumentHistoryTransition } from "../server/lib/document-history.js"; import { propagateDocumentTitle } from "../server/lib/document-title-propagation.js"; import { nextDocumentUpdatedAt } from "../server/lib/document-updated-at.js"; @@ -580,43 +581,54 @@ export default defineAction({ id, ) : []; - if (useContentCas) { - const applied = await tx - .update(schema.documents) - .set(updates) - .where( - and( - eq(schema.documents.id, id), - eq(schema.documents.updatedAt, args.baseUpdatedAt as string), - ), - ) - .returning({ id: schema.documents.id }); - if (!applied || applied.length === 0) { - contentCasConflict = true; - return; - } - } else if (lockedDocumentFieldsChanged) { - await tx - .update(schema.documents) - .set(updates) - .where(eq(schema.documents.id, id)); + const applied = await commitCanonicalDocumentBodyMutation({ + write: async () => { + if (useContentCas) { + const rows = await tx + .update(schema.documents) + .set(updates) + .where( + and( + eq(schema.documents.id, id), + eq( + schema.documents.updatedAt, + args.baseUpdatedAt as string, + ), + ), + ) + .returning({ id: schema.documents.id }); + return rows.length > 0; + } + if (lockedDocumentFieldsChanged) { + await tx + .update(schema.documents) + .set(updates) + .where(eq(schema.documents.id, id)); + } + return true; + }, + afterWrite: async () => { + if (lockedContentChanged && content !== undefined) { + for (const field of primaryBlocksFields) { + await persistBlocksFieldIdentity({ + db: tx as unknown as ReturnType, + ownerEmail: field.ownerEmail, + documentId: id, + propertyId: field.propertyId, + previousMarkdown: historyBefore.content, + markdown: content, + now: updatedAt, + }); + } + } + }, + }); + if (!applied) { + contentCasConflict = true; + return; } - committedContentChanged = lockedContentChanged; committedContentBefore = historyBefore.content; - if (lockedContentChanged && content !== undefined) { - for (const field of primaryBlocksFields) { - await persistBlocksFieldIdentity({ - db: tx as unknown as ReturnType, - ownerEmail: field.ownerEmail, - documentId: id, - propertyId: field.propertyId, - previousMarkdown: historyBefore.content, - markdown: content, - now: updatedAt, - }); - } - } if (favoriteChanged) { await setFavoriteMembership({ diff --git a/templates/content/actions/view-screen.ts b/templates/content/actions/view-screen.ts index 282c587a4a3..ae29e59e7a5 100644 --- a/templates/content/actions/view-screen.ts +++ b/templates/content/actions/view-screen.ts @@ -822,12 +822,16 @@ export default defineAction({ http: false, run: async () => { const navigation = await readAppStateForCurrentTab("navigation"); + const suggestionMode = await readAppStateForCurrentTab( + "content-suggestion-mode", + ); const localFilesState = await readAppState("local-files"); const contentSpaceState = await readAppState("content-space"); const selectionState = await readAppStateForCurrentTab("content-selection"); const screen: Record = {}; if (navigation) screen.navigation = navigation; + if (suggestionMode) screen.suggestionMode = suggestionMode; if (contentSpaceState) screen.contentSpace = contentSpaceState; const nav = navigation as NavigationState | null; diff --git a/templates/content/app/components/editor/BubbleToolbar.test.tsx b/templates/content/app/components/editor/BubbleToolbar.test.tsx index 0bcf4c9e129..4e08c0a6d09 100644 --- a/templates/content/app/components/editor/BubbleToolbar.test.tsx +++ b/templates/content/app/components/editor/BubbleToolbar.test.tsx @@ -1,5 +1,10 @@ // @vitest-environment happy-dom +import { readFileSync } from "node:fs"; + +import { docToNfm, nfmToDoc } from "@shared/nfm"; +import { BubbleMenuView } from "@tiptap/extension-bubble-menu"; +import type { Transaction } from "@tiptap/pm/state"; import { Editor } from "@tiptap/react"; import StarterKit from "@tiptap/starter-kit"; import { act, type ReactNode } from "react"; @@ -13,31 +18,37 @@ import { setSelectionNotionSpanAttribute, shouldShowBubbleToolbar, } from "./BubbleToolbar"; +import { LOCAL_FILE_USER_EDIT_META } from "./extensions/LocalMdxComponentNode"; import { CompatibleCode, NotionInlineAtom, NotionSpanMark, } from "./extensions/NotionExtensions"; +import { + isUserInitiatedCollaborativeEditorUpdate, + shouldPersistCollaborativeEditorUpdate, +} from "./VisualEditor"; vi.mock("@agent-native/core/client/i18n", () => ({ useT: () => (key: string) => key, })); -vi.mock("@tiptap/react/menus", () => ({ - BubbleMenu: ({ - children, - className, - updateDelay, - }: { - children: ReactNode; - className?: string; - updateDelay?: number; - }) => ( -
- {children} -
- ), -})); +const menuHarness = vi.hoisted(() => ({ real: false })); + +vi.mock("@tiptap/react/menus", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + BubbleMenu: (props: React.ComponentProps) => + menuHarness.real ? ( + + ) : ( +
+ {props.children} +
+ ), + }; +}); vi.mock("@/components/ui/tooltip", () => ({ Tooltip: ({ children }: { children: ReactNode }) => children, @@ -97,6 +108,244 @@ describe("BubbleToolbar", () => { editorElement = null; toolbarElement = null; }); + it("delivers coalesced resize positioning to the real BubbleMenu without changing content or selection and cleans up", () => { + menuHarness.real = true; + const positionUpdates = vi + .spyOn(BubbleMenuView.prototype, "updatePosition") + .mockImplementation(() => {}); + const callbacks: ResizeObserverCallback[] = []; + const disconnect = vi.fn(); + const observe = vi.fn(); + const frames = new Map(); + let nextFrame = 0; + vi.stubGlobal( + "ResizeObserver", + class { + constructor(callback: ResizeObserverCallback) { + callbacks.push(callback); + } + observe = observe; + disconnect = disconnect; + }, + ); + vi.stubGlobal("requestAnimationFrame", (callback: FrameRequestCallback) => { + frames.set(++nextFrame, callback); + return nextFrame; + }); + const cancel = vi.fn((id: number) => frames.delete(id)); + vi.stubGlobal("cancelAnimationFrame", cancel); + try { + editorElement = document.createElement("div"); + toolbarElement = document.createElement("div"); + document.body.append(editorElement, toolbarElement); + editor = new Editor({ + element: editorElement, + extensions: [StarterKit], + content: "

Italic sample.

", + }); + editor.commands.setTextSelection({ from: 1, to: 7 }); + const content = editor.getJSON(); + const selection = editor.state.selection.toJSON(); + root = createRoot(toolbarElement); + act(() => root!.render()); + positionUpdates.mockClear(); + const transactions: Transaction[] = []; + editor.on("transaction", ({ transaction }) => + transactions.push(transaction), + ); + expect(observe).toHaveBeenCalledWith(editor.view.dom); + const resize = (width: number) => + callbacks[0]!( + [ + { + target: editor!.view.dom, + contentRect: new DOMRectReadOnly(0, 0, width, 320), + borderBoxSize: [], + contentBoxSize: [], + devicePixelContentBoxSize: [], + }, + ], + {} as ResizeObserver, + ); + act(() => { + resize(760); + resize(600); + resize(600); + }); + expect(frames.size).toBe(1); + const flush = () => { + const pending = [...frames.values()]; + frames.clear(); + act(() => pending.forEach((callback) => callback(0))); + }; + flush(); + expect(positionUpdates).toHaveBeenCalledTimes(1); + expect(transactions).toHaveLength(1); + expect(transactions[0]!.docChanged).toBe(false); + expect(transactions[0]!.selectionSet).toBe(false); + expect(editor.getJSON()).toEqual(content); + expect(editor.state.selection.toJSON()).toEqual(selection); + act(() => resize(600)); + expect(frames.size).toBe(0); + act(() => resize(590)); + expect(frames.size).toBe(1); + act(() => root!.unmount()); + root = null; + expect(disconnect).toHaveBeenCalledOnce(); + expect(cancel).toHaveBeenCalledOnce(); + flush(); + expect(frames.size).toBe(0); + resize(580); + expect(frames.size).toBe(0); + expect(positionUpdates).toHaveBeenCalledTimes(1); + } finally { + menuHarness.real = false; + positionUpdates.mockRestore(); + vi.unstubAllGlobals(); + } + }); + it("layers the toolbar above the block grip and preserves a second-paragraph SVG click selection", () => { + const css = readFileSync("app/global.css", "utf8"); + const style = document.createElement("style"); + const grip = document.createElement("div"); + grip.className = "drag-handle"; + const gripPress = vi.fn(); + grip.addEventListener("mousedown", gripPress); + const rules = ["drag-handle", "bubble-toolbar"].map((name) => { + const rule = css.match(new RegExp(`\\.${name} \\{[^}]+\\}`))?.[0]; + expect(rule).toBeDefined(); + return rule; + }); + style.textContent = rules.join("\n"); + document.head.append(style); + document.body.append(grip); + try { + editorElement = document.createElement("div"); + toolbarElement = document.createElement("div"); + document.body.append(editorElement, toolbarElement); + editor = new Editor({ + element: editorElement, + extensions: [StarterKit], + content: "

Bold sample.

Italic sample.

", + }); + editor.commands.setTextSelection({ from: 15, to: 21 }); + editor.view.focus(); + root = createRoot(toolbarElement); + act(() => root!.render()); + const toolbar = + toolbarElement.querySelector(".bubble-toolbar")!; + expect(Number(getComputedStyle(toolbar).zIndex)).toBeGreaterThan( + Number(getComputedStyle(grip).zIndex), + ); + const button = toolbar.querySelector( + 'button[aria-label="editor.italic"]', + )!; + const icon = button.querySelector("path")!; + const press = new MouseEvent("mousedown", { + bubbles: true, + cancelable: true, + }); + act(() => { + icon.dispatchEvent(new PointerEvent("pointerdown", { bubbles: true })); + icon.dispatchEvent(press); + icon.dispatchEvent(new MouseEvent("mouseup", { bubbles: true })); + icon.dispatchEvent( + new MouseEvent("click", { bubbles: true, detail: 1 }), + ); + }); + expect(press.defaultPrevented).toBe(true); + expect(gripPress).not.toHaveBeenCalled(); + expect(editor.state.selection.from).toBe(15); + expect(editor.state.selection.to).toBe(21); + expect(docToNfm(editor.getJSON())).toBe("Bold sample.\n*Italic* sample."); + } finally { + style.remove(); + grip.remove(); + } + }); + it("opens an existing link destination for change and explicit removal", () => { + editorElement = document.createElement("div"); + toolbarElement = document.createElement("div"); + document.body.append(editorElement, toolbarElement); + editor = new Editor({ + element: editorElement, + extensions: [StarterKit], + content: '

Echo sample.

', + }); + editor.commands.setTextSelection({ from: 1, to: 5 }); + root = createRoot(toolbarElement); + act(() => root!.render()); + act(() => + toolbarElement! + .querySelector('button[aria-label="editor.link"]')! + .click(), + ); + const input = toolbarElement.querySelector( + 'input[aria-label="editor.pasteLink"]', + )!; + expect(input.value).toBe("https://example.test/old"); + act(() => { + Object.getOwnPropertyDescriptor( + HTMLInputElement.prototype, + "value", + )!.set!.call(input, "https://example.test/new"); + input.dispatchEvent(new Event("input", { bubbles: true })); + input.dispatchEvent(new Event("change", { bubbles: true })); + }); + act(() => + [...toolbarElement!.querySelectorAll("button")] + .find((button) => button.textContent === "editor.apply")! + .click(), + ); + expect(editor.getAttributes("link").href).toBe("https://example.test/new"); + act(() => + toolbarElement! + .querySelector('button[aria-label="editor.link"]')! + .click(), + ); + act(() => + [...toolbarElement!.querySelectorAll("button")] + .find((button) => button.textContent === "editor.removeLink")! + .click(), + ); + expect(editor.isActive("link")).toBe(false); + expect(editor.state.doc.textContent).toBe("Echo sample."); + }); + it("adds and removes underline through its visible control before and after NFM reload", () => { + editorElement = document.createElement("div"); + toolbarElement = document.createElement("div"); + document.body.append(editorElement, toolbarElement); + editor = new Editor({ + element: editorElement, + extensions: [StarterKit, NotionSpanMark], + content: "

Echo sample.

Other paragraph.

", + }); + editor.commands.setTextSelection({ from: 1, to: 5 }); + root = createRoot(toolbarElement); + act(() => root!.render()); + const button = toolbarElement.querySelector( + 'button[aria-label="editor.underline"]', + ); + expect(button).not.toBeNull(); + act(() => button!.click()); + const marked = docToNfm(editor.state.doc.toJSON()); + expect(marked).toBe( + 'Echo sample.\nOther paragraph.', + ); + act(() => { + editor!.commands.setContent(nfmToDoc(marked)); + editor!.commands.setTextSelection({ from: 1, to: 5 }); + }); + act(() => button!.click()); + expect(docToNfm(editor.state.doc.toJSON())).toBe( + "Echo sample.\nOther paragraph.", + ); + act(() => editor!.commands.setUnderline()); + act(() => button!.click()); + expect(docToNfm(editor.state.doc.toJSON())).toBe( + "Echo sample.\nOther paragraph.", + ); + }); it("starts a comment from selected text on Mod+Shift+M", () => { editorElement = document.createElement("div"); @@ -203,6 +452,54 @@ describe("BubbleToolbar", () => { ); }); + it("makes an unfocused link removal persistable through exact transaction provenance", () => { + editorElement = document.createElement("div"); + toolbarElement = document.createElement("div"); + document.body.append(editorElement, toolbarElement); + editor = new Editor({ + element: editorElement, + extensions: [StarterKit], + content: '

Link sample.

', + }); + editor.commands.setTextSelection({ from: 1, to: 5 }); + root = createRoot(toolbarElement); + act(() => root!.render()); + + act(() => { + toolbarElement! + .querySelector('button[aria-label="editor.link"]')! + .click(); + }); + const input = toolbarElement.querySelector( + 'input[aria-label="editor.pasteLink"]', + )!; + expect(document.activeElement).toBe(input); + + let persistenceAllowed = false; + editor.on("update", ({ editor: updatedEditor, transaction }) => { + const editorFocused = updatedEditor.isFocused; + const userInitiated = isUserInitiatedCollaborativeEditorUpdate({ + editorFocused, + explicitUserEdit: + transaction.getMeta(LOCAL_FILE_USER_EDIT_META) === true, + recentUserEditIntent: false, + transactionUiEvent: transaction.getMeta("uiEvent"), + }); + persistenceAllowed = shouldPersistCollaborativeEditorUpdate({ + collab: true, + editorFocused, + userInitiated, + }); + }); + const removeButton = [...toolbarElement.querySelectorAll("button")].find( + (button) => button.textContent === "editor.removeLink", + )!; + act(() => removeButton.click()); + + expect(editor.getHTML()).not.toContain("href="); + expect(persistenceAllowed).toBe(true); + }); + it("opens the link input on pointer-down before the menu can reconcile", () => { editorElement = document.createElement("div"); toolbarElement = document.createElement("div"); diff --git a/templates/content/app/components/editor/BubbleToolbar.tsx b/templates/content/app/components/editor/BubbleToolbar.tsx index b5de01c1fd5..0c6c6bd184c 100644 --- a/templates/content/app/components/editor/BubbleToolbar.tsx +++ b/templates/content/app/components/editor/BubbleToolbar.tsx @@ -4,6 +4,7 @@ import { IconCheck, IconChevronDown, IconItalic, + IconUnderline, IconStrikethrough, IconCode, IconLink, @@ -33,6 +34,7 @@ import { import { cn } from "@/lib/utils"; import { captureAnchor, type CommentTextAnchor } from "./comment-anchors"; +import { LOCAL_FILE_USER_EDIT_META } from "./extensions/LocalMdxComponentNode"; export type CommentRange = { from: number; to: number }; @@ -86,7 +88,7 @@ const COLOR_NAMES: ColorName[] = [ export function getSelectionNotionSpanAttribute( editor: Editor, - attribute: ColorAttribute, + attribute: ColorAttribute | "underline", ): "mixed" | (string & {}) | null { const { from, to } = editor.state.selection; const markType = editor.state.schema.marks.notionSpan; @@ -127,17 +129,29 @@ export function selectionHasColorableText( return hasText; } +function toolbarEditChain(editor: Editor) { + if (!editor.isEditable) return null; + return editor.chain().command(({ tr }) => { + tr.setMeta(LOCAL_FILE_USER_EDIT_META, true); + return true; + }); +} + export function setSelectionNotionSpanAttribute( editor: Editor, - attribute: ColorAttribute, + attribute: ColorAttribute | "underline", value: string | null, ) { + if (!editor.isEditable) return false; const { state } = editor; const { from, to } = state.selection; const markType = state.schema.marks.notionSpan; if (!markType || from === to) return false; const transaction = state.tr; + if (attribute === "underline" && state.schema.marks.underline) { + transaction.removeMark(from, to, state.schema.marks.underline); + } state.doc.nodesBetween(from, to, (node, position, parent) => { if (!node.isText || !parent?.type.allowsMarkType(markType)) return; const start = Math.max(from, position); @@ -159,6 +173,7 @@ export function setSelectionNotionSpanAttribute( }); if (!transaction.docChanged) return false; + transaction.setMeta(LOCAL_FILE_USER_EDIT_META, true); editor.view.dispatch(transaction); editor.commands.focus(); return true; @@ -219,6 +234,7 @@ export function shouldShowBubbleToolbar({ export function BubbleToolbar({ editor, onComment }: BubbleToolbarProps) { const t = useT(); + const [bubbleMenuKey] = useState(() => new PluginKey("contentBubbleToolbar")); const [showLinkInput, setShowLinkInput] = useState(false); const [linkUrl, setLinkUrl] = useState(""); const [textStyleOpen, setTextStyleOpen] = useState(false); @@ -237,6 +253,38 @@ export function BubbleToolbar({ editor, onComment }: BubbleToolbarProps) { activeTextStyle(editor), ); + useEffect(() => { + if (typeof ResizeObserver === "undefined") return; + const target = editor.view.dom; + let previousSize: { width: number; height: number } | undefined; + let frame: number | undefined; + let disposed = false; + const observer = new ResizeObserver((entries) => { + if (disposed) return; + const entry = entries.find((candidate) => candidate.target === target); + if (!entry) return; + const { width, height } = entry.contentRect; + if (previousSize?.width === width && previousSize.height === height) + return; + previousSize = { width, height }; + if (frame !== undefined) return; + // A rail can resize the editor without a selection or window-resize event. + frame = requestAnimationFrame(() => { + frame = undefined; + if (disposed || editor.isDestroyed) return; + editor.view.dispatch( + editor.state.tr.setMeta(bubbleMenuKey, "updatePosition"), + ); + }); + }); + observer.observe(target); + return () => { + disposed = true; + observer.disconnect(); + if (frame !== undefined) cancelAnimationFrame(frame); + }; + }, [editor, bubbleMenuKey]); + const createCommentFromSelection = useCallback(() => { if (!onComment) return false; const { from, to } = editor.state.selection; @@ -337,12 +385,13 @@ export function BubbleToolbar({ editor, onComment }: BubbleToolbarProps) { : textStyles[0]); const applyTextStyle = (style: TextStyle) => { - const chain = editor.chain(); + const chain = toolbarEditChain(editor); + if (!chain) return; if (textStyleSelection.current) { chain.setTextSelection(textStyleSelection.current); } if (style === "paragraph") { - chain.setParagraph().focus().run(); + chain.setNode("paragraph").focus().run(); } else { chain.setHeading({ level: style }).focus().run(); } @@ -352,6 +401,7 @@ export function BubbleToolbar({ editor, onComment }: BubbleToolbarProps) { }; const applyColor = (attribute: ColorAttribute, value: string | null) => { + if (!editor.isEditable) return; if (colorSelection.current) { editor.commands.setTextSelection(colorSelection.current); } @@ -523,25 +573,22 @@ export function BubbleToolbar({ editor, onComment }: BubbleToolbarProps) { }, [editor, openLinkInput]); const handleSetLink = () => { + const chain = toolbarEditChain(editor); + if (!chain) return; if (linkUrl.trim()) { - editor - .chain() + chain .focus() .extendMarkRange("link") .setLink({ href: linkUrl.trim() }) .run(); } else { - editor.chain().focus().extendMarkRange("link").unsetLink().run(); + chain.focus().extendMarkRange("link").unsetLink().run(); } setShowLinkInput(false); setLinkUrl(""); }; const toggleLink = () => { - if (editor.isActive("link")) { - editor.chain().focus().unsetLink().run(); - return; - } openLinkInput(); }; @@ -558,25 +605,44 @@ export function BubbleToolbar({ editor, onComment }: BubbleToolbarProps) { { icon: IconBold, title: t("editor.bold"), - action: () => editor.chain().focus().toggleBold().run(), + action: () => toolbarEditChain(editor)?.focus().toggleBold().run(), isActive: () => editor.isActive("bold"), }, { icon: IconItalic, title: t("editor.italic"), - action: () => editor.chain().focus().toggleItalic().run(), + action: () => toolbarEditChain(editor)?.focus().toggleItalic().run(), isActive: () => editor.isActive("italic"), }, + { + icon: IconUnderline, + title: t("editor.underline"), + action: () => { + if (!editor.isEditable) return; + const active = + editor.isActive("underline") || + getSelectionNotionSpanAttribute(editor, "underline") === "true"; + editor.commands.focus(); + setSelectionNotionSpanAttribute( + editor, + "underline", + active ? null : "true", + ); + }, + isActive: () => + editor.isActive("underline") || + getSelectionNotionSpanAttribute(editor, "underline") === "true", + }, { icon: IconStrikethrough, title: t("editor.strikethrough"), - action: () => editor.chain().focus().toggleStrike().run(), + action: () => toolbarEditChain(editor)?.focus().toggleStrike().run(), isActive: () => editor.isActive("strike"), }, { icon: IconCode, title: t("editor.code"), - action: () => editor.chain().focus().toggleCode().run(), + action: () => toolbarEditChain(editor)?.focus().toggleCode().run(), isActive: () => editor.isActive("code"), }, { type: "divider" as const }, @@ -602,6 +668,7 @@ export function BubbleToolbar({ editor, onComment }: BubbleToolbarProps) { return ( {t("editor.apply")} + {editor.isActive("link") ? ( + + ) : null} ) : (
{ + let container: HTMLDivElement; + let root: Root; + const submit = vi.fn(); + const escape = vi.fn(); + const mention = vi.fn(); + function Owner() { + const [value, setValue] = useState(""); + return ( + + ); + } + beforeEach(async () => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + submit.mockReset(); + escape.mockReset(); + mention.mockReset(); + container = document.createElement("div"); + document.body.append(container); + root = createRoot(container); + await act(async () => root.render()); + input().focus(); + }); + afterEach(async () => { + await act(async () => root.unmount()); + container.remove(); + vi.unstubAllGlobals(); + }); + const input = () => container.querySelector("textarea")!; + const buttons = () => [...container.querySelectorAll("button")]; + const active = () => + buttons().find((button) => + button.className.split(/\s+/).includes("bg-accent"), + )?.textContent; + const key = async (type: "keydown" | "keyup", name: string) => + act(async () => { + input().dispatchEvent( + new KeyboardEvent(type, { key: name, bubbles: true, cancelable: true }), + ); + }); + const press = async (name: string) => { + await key("keydown", name); + await key("keyup", name); + }; + const text = async (value: string) => { + await act(async () => { + const node = input(); + Object.getOwnPropertyDescriptor( + HTMLTextAreaElement.prototype, + "value", + )!.set!.call(node, value); + node.setSelectionRange(value.length, value.length); + node.dispatchEvent(new Event("input", { bubbles: true })); + }); + await key("keyup", value.slice(-1)); + }; + const settle = async () => + act(async () => { + await new Promise((resolve) => setTimeout(resolve, 25)); + }); + + it("keeps a mention picker dismissed after Escape keyup and routes the next Escape outward", async () => { + await text("Reply @"); + expect(buttons()).toHaveLength(3); + await press("Escape"); + await settle(); + expect(buttons()).toHaveLength(0); + expect(input().value).toBe("Reply @"); + expect(escape).not.toHaveBeenCalled(); + await press("Escape"); + expect(escape).toHaveBeenCalledTimes(1); + }); + it.each([ + ["ArrowDown", "Beta"], + ["ArrowUp", "Gamma"], + ] as const)( + "keeps %s menu selection through keyup", + async (arrow, expected) => { + await text("Reply @"); + await press(arrow); + expect(active()).toContain(expected); + }, + ); + it.each([ + ["Enter", false], + ["Enter", true], + ["Tab", false], + ["Tab", true], + ] as const)( + "chooses the navigated member once with %s (RAF before keyup %s)", + async (accept, frameFirst) => { + await text("Reply @"); + await press("ArrowDown"); + await key("keydown", accept); + if (frameFirst) await settle(); + await key("keyup", accept); + await settle(); + expect(input().value).toBe("Reply @Beta "); + expect(mention).toHaveBeenCalledExactlyOnceWith({ + name: "Beta", + email: "beta@example.test", + }); + expect(submit).not.toHaveBeenCalled(); + expect(buttons()).toHaveLength(0); + expect([input().selectionStart, input().selectionEnd]).toEqual([ + "Reply @Beta ".length, + "Reply @Beta ".length, + ]); + }, + ); + it("retains repeated menu navigation and wraps in both directions", async () => { + await text("Reply @"); + for (const expected of ["Beta", "Gamma", "Alpha", "Beta"]) { + await press("ArrowDown"); + expect(active()).toContain(expected); + } + for (const expected of ["Alpha", "Gamma", "Beta", "Alpha"]) { + await press("ArrowUp"); + expect(active()).toContain(expected); + } + await press("Shift"); + expect(active()).toContain("Alpha"); + }); + it("refreshes a query when ordinary text input or caret navigation changes its source", async () => { + await text("Reply @"); + await press("Escape"); + await text("Reply @G"); + expect(active()).toContain("Gamma"); + await text("Reply @Alpha later"); + expect(buttons()).toHaveLength(0); + await key("keydown", "ArrowLeft"); + input().setSelectionRange("Reply @Alpha".length, "Reply @Alpha".length); + await key("keyup", "ArrowLeft"); + expect(active()).toContain("Alpha"); + await key("keydown", "End"); + input().setSelectionRange(input().value.length, input().value.length); + await key("keyup", "End"); + expect(buttons()).toHaveLength(0); + }); +}); diff --git a/templates/content/app/components/editor/CommentComposer.test.tsx b/templates/content/app/components/editor/CommentComposer.test.tsx index 197d47654a4..03322781f60 100644 --- a/templates/content/app/components/editor/CommentComposer.test.tsx +++ b/templates/content/app/components/editor/CommentComposer.test.tsx @@ -47,5 +47,7 @@ describe("CommentComposer", () => { const textarea = render(false); expect(textarea.disabled).toBe(false); + expect(textarea.className).toContain("[field-sizing:content]"); + expect(textarea.className).toContain("max-h-48"); }); }); diff --git a/templates/content/app/components/editor/CommentComposer.tsx b/templates/content/app/components/editor/CommentComposer.tsx index 698def38561..341ad3fe9ae 100644 --- a/templates/content/app/components/editor/CommentComposer.tsx +++ b/templates/content/app/components/editor/CommentComposer.tsx @@ -64,6 +64,7 @@ export const CommentComposer = forwardRef< const innerRef = useRef(null); const [query, setQuery] = useState(null); const [highlight, setHighlight] = useState(0); + const consumedKeys = useRef(new Set()); const setRefs = (el: HTMLTextAreaElement | null) => { innerRef.current = el; @@ -96,8 +97,9 @@ export const CommentComposer = forwardRef< const caret = el.selectionStart ?? el.value.length; const before = el.value.slice(0, caret); const match = before.match(/(?:^|\s)@([^\s@]*)$/); - setQuery(match ? match[1] : null); - setHighlight(0); + const nextQuery = match ? match[1] : null; + setQuery(nextQuery); + if (nextQuery !== query) setHighlight(0); }; const selectMember = (member: MentionMember) => { @@ -127,46 +129,52 @@ export const CommentComposer = forwardRef< const menuOpen = query !== null && filtered.length > 0; const handleKeyDown = (e: KeyboardEvent) => { + if ( + e.key === "Escape" && + (e.nativeEvent.isComposing || e.nativeEvent.keyCode === 229) + ) { + e.stopPropagation(); + return true; + } if (menuOpen) { if (e.key === "ArrowDown") { e.preventDefault(); setHighlight((h) => (h + 1) % filtered.length); - return; + return true; } if (e.key === "ArrowUp") { e.preventDefault(); setHighlight((h) => (h - 1 + filtered.length) % filtered.length); - return; + return true; } if (e.key === "Enter" || e.key === "Tab") { e.preventDefault(); selectMember(filtered[highlight]); - return; + return true; } if (e.key === "Escape") { e.preventDefault(); e.stopPropagation(); setQuery(null); - return; + return true; } } if (e.key === "Enter" && !e.shiftKey) { e.preventDefault(); onSubmit(); - return; + return true; } if (e.key === "Escape" && onEscape) { - e.preventDefault(); - e.stopPropagation(); onEscape(); + return true; } + return false; }; return (