From f8a410588942fc837d62aefb2e1e1bb4f13917e2 Mon Sep 17 00:00:00 2001 From: Marc-Antoine Parent Date: Sat, 25 Jul 2026 11:50:43 -0400 Subject: [PATCH 1/3] eng-2067-refactor-publishNodesToGroups-to-avoid-database-access-in-loops --- apps/roam/src/utils/publishNodesToGroups.ts | 111 +++++++----------- packages/database/src/lib/contextFunctions.ts | 5 + packages/database/src/lib/groups.ts | 56 +++++++++ 3 files changed, 106 insertions(+), 66 deletions(-) diff --git a/apps/roam/src/utils/publishNodesToGroups.ts b/apps/roam/src/utils/publishNodesToGroups.ts index b873fc2be..05eb0db5c 100644 --- a/apps/roam/src/utils/publishNodesToGroups.ts +++ b/apps/roam/src/utils/publishNodesToGroups.ts @@ -2,12 +2,19 @@ import { CrossAppNode } from "@repo/database/crossAppContracts"; import type { DGSupabaseClient } from "@repo/database/lib/client"; import { getAvailableGroupIds } from "@repo/database/lib/groups"; import { nodeUidsWithTypeToCrossApp } from "./roamToCrossAppConverters"; +import { ensurePartialSpaceAccess } from "@repo/database/lib/groups"; +import { isIgnorableUpsertError } from "@repo/database/lib/contextFunctions"; +import { difference, intersection } from "@repo/utils/setOperations"; +import internalError from "./internalError"; export type NodeUidWithType = { uid: string; type: string; }; +const onlyStrings = (values: (string | null)[]): string[] => + values.filter((value): value is string => typeof value === "string"); + type PublishNodesResult = { publishedNodeUids: string[]; skippedUnsyncedUids: string[]; @@ -15,13 +22,6 @@ type PublishNodesResult = { failedGroupIds: string[]; }; -// 23505 = unique_violation: the grant already exists, which counts as success. -const isIgnorableUpsertError = (error: { code?: string } | null): boolean => - !error || error.code === "23505"; - -const onlyStrings = (values: (string | null)[]): string[] => - values.filter((value): value is string => typeof value === "string"); - // Grants a group access to already-synced discourse nodes by mirroring the // Obsidian publish-to-group access model (SpaceAccess + ResourceAccess), // without its file/frontmatter/relation/asset coupling. @@ -50,83 +50,62 @@ export const publishNodesToGroups = async ({ if (nodes.length === 0 || groupIds.length === 0) return result; const availableGroupIds = new Set(await getAvailableGroupIds(client)); - const requestedGroupIds = [...new Set(groupIds)]; - const targetGroupIds = requestedGroupIds.filter((groupId) => - availableGroupIds.has(groupId), - ); - result.failedGroupIds = requestedGroupIds.filter( - (groupId) => !availableGroupIds.has(groupId), - ); - if (targetGroupIds.length === 0) return result; + const requestedGroupIds = new Set(groupIds); + groupIds = [...intersection(requestedGroupIds, availableGroupIds)]; + result.failedGroupIds = [...difference(requestedGroupIds, availableGroupIds)]; + if (groupIds.length === 0) return result; + + const { existing: existingSpaceAccess, missing: missingSpaceAccess } = + await ensurePartialSpaceAccess({ + client, + groupIds, + spaceId, + }); + if (missingSpaceAccess && Object.keys(missingSpaceAccess).length) { + result.failedGroupIds = groupIds.filter( + (id) => !(id in existingSpaceAccess), + ); + groupIds = Object.keys(existingSpaceAccess); + } + if (groupIds.length === 0) return result; - const uids = [...new Set(nodes.map((node) => node.localId))]; + const nodeUids = [...new Set(nodes.map((node) => node.localId))]; const syncedRes = await client .from("my_concepts") .select("source_local_id") .eq("space_id", spaceId) - .eq("is_schema", false) - .in("source_local_id", uids); - if (syncedRes.error) throw syncedRes.error; + .in("source_local_id", nodeUids); + if (syncedRes.error) { + internalError({ error: syncedRes.error }); + return result; + } const syncedUids = new Set( onlyStrings((syncedRes.data ?? []).map((row) => row.source_local_id)), ); + result.skippedUnsyncedUids = nodeUids.filter((uid) => !syncedUids.has(uid)); - result.skippedUnsyncedUids = uids.filter((uid) => !syncedUids.has(uid)); - const syncedNodeUids = uids.filter((uid) => syncedUids.has(uid)); - if (syncedNodeUids.length === 0) return result; - - // Required dependency: the node-type schema concept, when it is synced too. - const types = [ - ...new Set( - nodes - .filter((node) => syncedUids.has(node.localId)) - .map((node) => node.nodeType), - ), - ]; - const schemaRes = await client - .from("my_concepts") - .select("source_local_id") - .eq("space_id", spaceId) - .eq("is_schema", true) - .in("source_local_id", types); - if (schemaRes.error) throw schemaRes.error; - const syncedSchemaIds = onlyStrings( - (schemaRes.data ?? []).map((row) => row.source_local_id), - ); - - const resourceIds = [...syncedNodeUids, ...syncedSchemaIds]; - - for (const groupId of targetGroupIds) { - // Existing reader/editor access is broader than partial, so leave it intact. - const spaceAccessRes = await client - .from("SpaceAccess") - .upsert( - { account_uid: groupId, space_id: spaceId, permissions: "partial" }, - { ignoreDuplicates: true }, - ); - if (!isIgnorableUpsertError(spaceAccessRes.error)) { - result.failedGroupIds.push(groupId); - continue; - } - - const grantRes = await client.from("ResourceAccess").upsert( - resourceIds.map((sourceLocalId) => ({ + const resourceAccesses = []; + for (const groupId of groupIds) { + resourceAccesses.push( + ...nodeUids.map((sourceLocalId) => ({ account_uid: groupId, source_local_id: sourceLocalId, space_id: spaceId, })), - { ignoreDuplicates: true }, ); - if (!isIgnorableUpsertError(grantRes.error)) { - result.failedGroupIds.push(groupId); - continue; - } + } - result.okGroupIds.push(groupId); + const grantRes = await client + .from("ResourceAccess") + .upsert(resourceAccesses, { ignoreDuplicates: true }); + if (!isIgnorableUpsertError(grantRes.error)) { + internalError({ error: grantRes.error }); + return result; } - result.publishedNodeUids = result.okGroupIds.length > 0 ? syncedNodeUids : []; + result.publishedNodeUids = nodeUids; + return result; }; diff --git a/packages/database/src/lib/contextFunctions.ts b/packages/database/src/lib/contextFunctions.ts index b6213a0ef..1eda4f3ad 100644 --- a/packages/database/src/lib/contextFunctions.ts +++ b/packages/database/src/lib/contextFunctions.ts @@ -47,6 +47,11 @@ export const asPostgrestFailure = ( export class FatalError extends Error {} +// 23505 = unique_violation: the grant already exists, which counts as success. +export const isIgnorableUpsertError = ( + error: { code?: string } | null, +): boolean => !error || error.code === "23505"; + export const spaceValidator = (space: SpaceCreationInput): string | null => { if (!space || typeof space !== "object") return "Invalid request body: expected a JSON object."; diff --git a/packages/database/src/lib/groups.ts b/packages/database/src/lib/groups.ts index 6a9b13f89..46aed3935 100644 --- a/packages/database/src/lib/groups.ts +++ b/packages/database/src/lib/groups.ts @@ -1,4 +1,6 @@ import type { DGSupabaseClient } from "./client"; +import type { Tables, Enums } from "../dbTypes"; +import { isIgnorableUpsertError } from "./contextFunctions"; export type MyGroup = { id: string; @@ -47,3 +49,57 @@ export const getMyGroups = async ( name: row.my_groups.name ?? row.group_id, })); }; + +type SpaceAccessPermissions = Enums<"SpaceAccessPermissions">; + +export const ensurePartialSpaceAccess = async ({ + client, + groupIds, + spaceId, +}: { + client: DGSupabaseClient; + groupIds: string[]; + spaceId: number; +}): Promise<{ + existing: Record; + missing?: Record; +}> => { + const existingAccessResult = await client + .from("SpaceAccess") + .select() + .eq("space_id", spaceId) + .in("account_uid", groupIds); + const existingAccessByGroupId = existingAccessResult.data + ? Object.fromEntries( + existingAccessResult.data.map((sa) => [sa.account_uid, sa.permissions]), + ) + : {}; + const missingAccess: Tables<"SpaceAccess">[] = []; + for (const groupId of groupIds) { + if (existingAccessByGroupId[groupId] === undefined) { + missingAccess.push({ + space_id: spaceId, + permissions: "partial", + account_uid: groupId, + }); + } + } + if (missingAccess.length > 0) { + const upsertAccessResult = await client + .from("SpaceAccess") + .upsert(missingAccess, { ignoreDuplicates: true }); + if (!isIgnorableUpsertError(upsertAccessResult.error)) { + // allow partial results + return { + existing: existingAccessByGroupId, + missing: Object.fromEntries( + missingAccess.map((a) => [a.space_id, a.permissions]), + ), + }; + } + } + missingAccess.forEach((a) => { + existingAccessByGroupId[a.account_uid] = "partial"; + }); + return { existing: existingAccessByGroupId }; +}; From 9792f5d5bc7e275469757a929f33cb5b6b52a82c Mon Sep 17 00:00:00 2001 From: Marc-Antoine Parent Date: Mon, 27 Jul 2026 06:22:52 -0400 Subject: [PATCH 2/3] AI fixes --- apps/roam/src/utils/publishNodesToGroups.ts | 11 +++++++---- packages/database/src/lib/groups.ts | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/apps/roam/src/utils/publishNodesToGroups.ts b/apps/roam/src/utils/publishNodesToGroups.ts index 05eb0db5c..acbcd84c9 100644 --- a/apps/roam/src/utils/publishNodesToGroups.ts +++ b/apps/roam/src/utils/publishNodesToGroups.ts @@ -62,14 +62,15 @@ export const publishNodesToGroups = async ({ spaceId, }); if (missingSpaceAccess && Object.keys(missingSpaceAccess).length) { - result.failedGroupIds = groupIds.filter( - (id) => !(id in existingSpaceAccess), - ); + result.failedGroupIds = [ + ...result.failedGroupIds, + ...groupIds.filter((id) => !(id in existingSpaceAccess)), + ]; groupIds = Object.keys(existingSpaceAccess); } if (groupIds.length === 0) return result; - const nodeUids = [...new Set(nodes.map((node) => node.localId))]; + let nodeUids = [...new Set(nodes.map((node) => node.localId))]; const syncedRes = await client .from("my_concepts") @@ -84,6 +85,7 @@ export const publishNodesToGroups = async ({ onlyStrings((syncedRes.data ?? []).map((row) => row.source_local_id)), ); result.skippedUnsyncedUids = nodeUids.filter((uid) => !syncedUids.has(uid)); + nodeUids = [...intersection(syncedUids, new Set(nodeUids))]; const resourceAccesses = []; for (const groupId of groupIds) { @@ -104,6 +106,7 @@ export const publishNodesToGroups = async ({ return result; } + result.okGroupIds = groupIds; result.publishedNodeUids = nodeUids; return result; diff --git a/packages/database/src/lib/groups.ts b/packages/database/src/lib/groups.ts index 46aed3935..66848d617 100644 --- a/packages/database/src/lib/groups.ts +++ b/packages/database/src/lib/groups.ts @@ -93,7 +93,7 @@ export const ensurePartialSpaceAccess = async ({ return { existing: existingAccessByGroupId, missing: Object.fromEntries( - missingAccess.map((a) => [a.space_id, a.permissions]), + missingAccess.map((a) => [a.account_uid, a.permissions]), ), }; } From 068bb46a191cc158a8e69932c5c4e2eb8046a268 Mon Sep 17 00:00:00 2001 From: Marc-Antoine Parent Date: Fri, 31 Jul 2026 09:42:33 -0400 Subject: [PATCH 3/3] codex fix --- apps/roam/src/utils/publishNodesToGroups.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/roam/src/utils/publishNodesToGroups.ts b/apps/roam/src/utils/publishNodesToGroups.ts index acbcd84c9..180439a51 100644 --- a/apps/roam/src/utils/publishNodesToGroups.ts +++ b/apps/roam/src/utils/publishNodesToGroups.ts @@ -103,6 +103,7 @@ export const publishNodesToGroups = async ({ .upsert(resourceAccesses, { ignoreDuplicates: true }); if (!isIgnorableUpsertError(grantRes.error)) { internalError({ error: grantRes.error }); + result.failedGroupIds.push(...groupIds); return result; }