ENG-633 Refactor: Move some functions to packages/database #356
Conversation
|
Updates to Preview Branch (eng-633-refactor-move-some-functions-to-packagesdatabase) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe PR centralizes Supabase-related types and helpers into the database package, repoints numerous imports from types.gen/ui paths to new dbTypes/lib locations, restructures database package exports and build outputs, narrows its tsconfig scope, adjusts dbDotEnv path resolution, updates Deno import maps, removes UI Supabase deps, and adds a utils package. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/website/app/utils/supabase/validators.ts (1)
147-158: Bug:new Date(...)does not throw on invalid input; current validation always passes.
new Date("not-a-date")yields an Invalid Date without throwing. The try/catch blocks won’t catch bad inputs. Validate usingisNaN(date.getTime()).- if (created) - try { - new Date(created); - } catch (e) { - return "Invalid date format for created."; - } - if (last_modified) - try { - new Date(last_modified); - } catch (e) { - return "Invalid date format for last_modified."; - } + if (created != null) { + const createdDate = new Date(created); + if (Number.isNaN(createdDate.getTime())) { + return "Invalid date format for created."; + } + } + if (last_modified != null) { + const lastModifiedDate = new Date(last_modified); + if (Number.isNaN(lastModifiedDate.getTime())) { + return "Invalid date format for last_modified."; + } + }apps/website/app/utils/supabase/dbUtils.ts (1)
133-151: Bug: checking the whole Postgrest response for null instead of its data.
maybeSinglereturns an object with{ data, error }. The code testsif (reFetchResult === null)which will never be true; it should testreFetchedEntity(the data) instead. This causes the error branch to be skipped even when the re-fetch failed.Apply this diff:
- const reFetchResult = - await reFetchQueryBuilder.maybeSingle<Tables<TableName>>(); - const { data: reFetchedEntity, error: reFetchError } = reFetchResult; - - if (reFetchResult === null) { - result.error.message = `Unique constraint violation on on (${uniqueOn.join(", ")}) in ${tableName}, and re-fetch failed to find the entity because of ${reFetchError}.`; + const reFetchResult = + await reFetchQueryBuilder.maybeSingle<Tables<TableName>>(); + const { data: reFetchedEntity, error: reFetchError } = reFetchResult; + + if (reFetchedEntity == null) { + result.error.message = `Unique constraint violation on (${uniqueOn.join(", ")}) in ${tableName}, and re-fetch failed to find the entity because of ${reFetchError?.message ?? reFetchError}.`; result.status = 500; return result; } else { console.log(`Found ${tableName} on re-fetch:`, reFetchedEntity); // Note: Using a PostgrestResult means I cannot have both data and error non-null... return { error: { ...result.error, - message: `Upsert failed because of conflict with this entity: ${reFetchedEntity}"`, + message: `Upsert failed because of conflict with an existing entity.`, }, statusText: result.statusText, data: null, count: null, status: 400, }; }packages/database/inputTypes.ts (1)
1-14: AddinputTypestopackages/databaseexports to prevent breaking importsWe discovered that
apps/roam/src/utils/conceptConversion.tsline 6 importsLocalConceptDataInputfrom"@repo/database/inputTypes". Sincepackages/database/package.jsoncurrently only exports:
./lib/*./dbDotEnv./dbTypesany import of
@repo/database/inputTypeswill fail under Node’s subpath exports. We need to exposeinputTypes(and its TypeScript path) inpackage.json.• packages/database/package.json
• Under"exports", add"./inputTypes": "./inputTypes.ts"
• Add a"typesVersions"mapping so TypeScript consumers can resolve itSuggested diff:
--- a/packages/database/package.json +++ b/packages/database/package.json @@ 7,12c7,20 "exports": { "./lib/*": "./src/lib/*.ts", "./dbDotEnv": "./src/dbDotEnv.ts", "./dbTypes": "./src/dbTypes.ts", + "./inputTypes": "./inputTypes.ts" }, + "typesVersions": { + "*": { + "lib/*": ["src/lib/*.ts"], + "dbDotEnv": ["src/dbDotEnv.ts"], + "dbTypes": ["src/dbTypes.ts"], + "inputTypes": ["inputTypes.ts"] + } + }, "private": true, "license": "Apache-2.0", "type": "module",With this change, both Node and TypeScript consumers will continue to import from
@repo/database/inputTypeswithout errors.apps/website/app/api/supabase/content-embedding/batch/route.ts (1)
53-58: Handle unknown embedding models explicitly instead of silently skippingCurrently, items with an unrecognized model are dropped without contributing to partialErrors, which can yield confusing 500s with empty messages when no rows are inserted. Prefer recording a client error per unknown model.
Apply:
-for (const modelName of Object.keys(byModel)) { - const embeddingItemsSet = byModel[modelName]; - if (embeddingItemsSet === undefined) continue; +for (const [modelName, embeddingItemsSet] of Object.entries(byModel)) { + if (!embeddingItemsSet?.length) continue; const tableData = KNOWN_EMBEDDING_TABLES[modelName]; - if (tableData === undefined) continue; + if (tableData === undefined) { + partialErrors.push(`Unknown embedding model: ${modelName}`); + has_400 = true; + continue; + }apps/website/app/api/supabase/similarity-rank/route.ts (1)
20-31: Tighten input validation for embedding values to avoid DB errorsNaN/Infinity serialize to null in JSON, which can break the RPC. Validate that embedding is a non-empty array of finite numbers.
- if ( - !Array.isArray(embedding) || - !Array.isArray(subsetRoamUids) || - !subsetRoamUids.length - ) { + if ( + !Array.isArray(embedding) || + embedding.length === 0 || + !embedding.every((n) => typeof n === "number" && Number.isFinite(n)) || + !Array.isArray(subsetRoamUids) || + subsetRoamUids.length === 0 + ) { return createApiResponse( request, asPostgrestFailure( - "Missing required fields: embedding and subsetRoamUids", + "Invalid input: embedding must be a non-empty array of finite numbers and subsetRoamUids must be a non-empty string array", "invalid", ), ); }packages/database/src/lib/client.ts (1)
15-24: Fix Deno Edge Function Imports and Provide Deno-Compatible ClientFound that the Deno edge function and related code are still importing the Node-based Supabase client, which will fail at runtime. Please update the following:
• packages/database/supabase/functions/create-space/deno.json
– Change the"@repo/database/lib/client"import mapping to point to a Deno-compatible client module (e.g. a new file in this function’s folder that uses Deno.env and the Deno-supported @supabase/supabase-js (deno) build).
• packages/database/supabase/functions/create-space/index.ts
– Replace theimport type { DGSupabaseClient } from "@repo/database/lib/client";(and any runtime import) with your Deno client API.
• apps/roam/src/utils/supabaseContext.ts
– Confirm that browser/server code imports the correct client; factor out any Node-only code so that edge/runtime contexts don’t accidentally pull in process.env calls.Additionally, if you need a service-role key for backend jobs, introduce a separate
createServiceClient()in your Node environment—do not overload the public anon client used by routes.packages/database/supabase/functions/create-space/index.ts (1)
130-138: Bug:new PostgrestError(...)will throw at runtime (it’s a type, not a constructor).
PostgrestErrorin@supabase/supabase-jsis a type/interface, not a class. Constructing withnewwill fail in Deno Edge. Build succeeds because TS treats it asanyat runtime, but the function will crash on this path.Apply:
-import { - createClient, - type User, - type PostgrestSingleResponse, - PostgrestError, -} from "@supabase/supabase-js"; +import { + createClient, + type User, + type PostgrestSingleResponse, + type PostgrestError, +} from "@supabase/supabase-js"; @@ - error: new PostgrestError({ - message: resultCreateAnonymousUser.error.message, - details: - typeof resultCreateAnonymousUser.error.cause === "string" - ? resultCreateAnonymousUser.error.cause - : "", - hint: "", - code: resultCreateAnonymousUser.error.code || "unknown", - }), + error: { + message: resultCreateAnonymousUser.error.message, + details: + typeof resultCreateAnonymousUser.error.cause === "string" + ? resultCreateAnonymousUser.error.cause + : "", + hint: "", + code: resultCreateAnonymousUser.error.code || "unknown", + } as PostgrestError,Alternatively, reuse your local
asPostgrestFailureto unify error formatting:- if (resultCreateAnonymousUser.error) { - return { - count: null, - status: resultCreateAnonymousUser.error.status || -1, - statusText: resultCreateAnonymousUser.error.message, - data: null, - error: new PostgrestError({ ... }), - }; // space created but not its user, try again - } + if (resultCreateAnonymousUser.error) { + return asPostgrestFailure( + resultCreateAnonymousUser.error.message, + resultCreateAnonymousUser.error.code || "unknown", + resultCreateAnonymousUser.error.status || 500, + ); + }
🧹 Nitpick comments (32)
apps/website/app/utils/supabase/validators.ts (3)
6-6: Use a type-only import to avoid bundling dbTypes at runtime.Since
TablesandTablesInsertare types, importing them withimport typereduces bundle size and prevents unintended runtime dependencies.-import { Tables, TablesInsert } from "@repo/database/dbTypes"; +import type { Tables, TablesInsert } from "@repo/database/dbTypes";
51-58: Harden numeric checks for embedding vectors.Some inputs can be
NaN/Infinitywhile still being of typenumber. Guard withNumber.isFinite.- if ( - !vector || - !Array.isArray(vector) || - !vector.every((v) => typeof v === "number") - ) { + if ( + !vector || + !Array.isArray(vector) || + !vector.every((v) => typeof v === "number" && Number.isFinite(v)) + ) {
115-120: Avoid leaking raw parser error messages.Return a stable, user-safe error for malformed JSON instead of
error.message.- } catch (error) { - if (error instanceof SyntaxError) { - return { valid: false, error: error.message }; - } - throw error; - } + } catch (error) { + if (error instanceof SyntaxError) { + return { valid: false, error: "Invalid vector JSON format." }; + } + throw error; + }packages/utils/tsconfig.json (1)
3-10: Tidy TS config: broaden include and drop unused emit options.
includeshould usually be./src/**/*to catch nested folders.- With
"noEmit": true,outDirandrootDirare unnecessary noise."compilerOptions": { "baseUrl": ".", - "rootDir": ".", - "noEmit": true, - "outDir": "dist" + "noEmit": true }, - "include": ["./src/*"] + "include": ["./src/**/*"]packages/database/tsconfig.json (1)
10-13: Consider type-checking build scripts; previously they were covered.Narrowing to
./src/*excludes./scripts/(e.g.,scripts/build.ts). If you wanttscto catch regressions there, include the folder.Using our prior learning about this package’s atypical layout, earlier config included the package root and excluded only
supabase. If the exclusion is intentional, feel free to ignore."include": [ "./src/*", + "./scripts/**/*", "./features/step-definitions/**/*", "./features/support/**/*" ],packages/database/scripts/build.ts (2)
2-2: Ensure target directory exists before writing.Create
src/if it’s missing to avoid ENOENT on fresh clones/CI.-import { writeFileSync } from "fs"; +import { writeFileSync, mkdirSync, existsSync } from "fs";
11-18: Make CLI calls cwd-safe and robust; then write to an ensuredsrc/folder.
- Pass
{ cwd: projectRoot }to allexecSynccalls so they work regardless of where the script is invoked from.- Create
src/recursively before writingdbTypes.ts.- execSync("supabase start"); - execSync("supabase migrations up"); - const stdout = execSync( - "supabase gen types typescript --local --schema public", - { encoding: "utf8" }, - ); - writeFileSync(join(projectRoot, "src", "dbTypes.ts"), stdout); + execSync("supabase start", { cwd: projectRoot, stdio: "inherit" }); + execSync("supabase migrations up", { cwd: projectRoot, stdio: "inherit" }); + const stdout = execSync( + "supabase gen types typescript --local --schema public", + { encoding: "utf8", cwd: projectRoot }, + ); + const targetDir = join(projectRoot, "src"); + if (!existsSync(targetDir)) { + mkdirSync(targetDir, { recursive: true }); + } + writeFileSync(join(targetDir, "dbTypes.ts"), stdout);apps/website/app/utils/supabase/dbUtils.ts (3)
140-150: Prefer returning the existing entity on conflict instead of a 400 error.For “get or create” semantics, returning the found entity with 200/201 is friendlier than emitting an error. Consider flipping the contract to success with data when re-fetch succeeds.
- return { - error: { - ...result.error, - message: `Upsert failed because of conflict with an existing entity.`, - }, - statusText: result.statusText, - data: null, - count: null, - status: 400, - }; + return { + error: null, + statusText: "Conflict resolved by returning existing entity", + data: reFetchedEntity, + count: null, + status: 200, + };
117-128: Skip undefined unique keys when building the re-fetch query.If any key in
uniqueOnis absent/undefined ininsertData, theeqfilter will compare againstundefined, which won’t match. Guard for defined values.- reFetchQueryBuilder = reFetchQueryBuilder.eq( - keyS, - insertData[key] as any, // TS gets confused here? - ); + const value = insertData[key]; + if (value !== undefined) { + reFetchQueryBuilder = reFetchQueryBuilder.eq(keyS, value as any); + } else { + console.warn(`uniqueOn key "${keyS}" missing from insertData`); + }
20-27: Make duplicate/foreign key parsing more robust (hint vs detail, trailing punctuation).Postgres often places the “Key (...)=(...) already exists.” text in
error.detailrather thanhint. Also, the trailing period varies. Consider checking both fields and loosening the regex.-const UNIQUE_KEY_RE = /^Key \(([^)]+)\)=\(([\^)]+)\) already exists\.$/; +const UNIQUE_KEY_RE = /Key \(([^)]+)\)=\(([^)]+)\) already exists\.?$/; @@ - if (error.code === "23505") { + if (error.code === "23505") { @@ - const dup_key_data = UNIQUE_KEY_RE.exec(error.hint); + const msg = error.hint ?? error.details ?? error.message ?? ""; + const dup_key_data = UNIQUE_KEY_RE.exec(msg);packages/database/package.json (2)
28-33: Versioning consistency nit: align Supabase SDK package ranges.Minor nit: you pin
@supabase/supabase-jsand@supabase/auth-jsto exact versions but use a caret for@supabase/functions-js. Consider a consistent strategy (all exact or all carets) to simplify dependabot bumps and reduce surprise upgrades.- "@supabase/functions-js": "^2.4.5", + "@supabase/functions-js": "2.4.5",
22-23: Gentypes output path updated to src/dbTypes.ts — good; add a “generated file” header to reduce hand edits.Non-blocking suggestion: have the generator prepend a one-line comment like “// Generated file — do not edit” to
src/dbTypes.tsto avoid manual changes creeping in.apps/website/app/api/supabase/content-embedding/batch/route.ts (2)
32-41: Simplify grouping logic and avoid try/catch around a pure reduceThe reduce here is only grouping data and shouldn’t throw. A clear for-of loop improves readability and avoids defensive try/catch.
-const byModel: { [key: string]: ApiInputEmbeddingItem[] } = {}; -try { - embeddingItems.reduce((acc, item) => { - const model = item?.model || DEFAULT_MODEL; - if (acc[model] === undefined) { - acc[model] = []; - } - acc[model]!.push(item); - return acc; - }, byModel); -} catch (error) { - if (error instanceof Error) { - return asPostgrestFailure(error.message, "exception"); - } - throw error; -} +const byModel: { [key: string]: ApiInputEmbeddingItem[] } = {}; +for (const item of embeddingItems) { + const model = item?.model || DEFAULT_MODEL; + (byModel[model] ??= []).push(item); +}
97-101: Return a clearer error when no rows were insertedIf count === 0 and partialErrors is empty (e.g., only unknown models), the current statusText can be empty. Consider a default message to aid clients.
- return asPostgrestFailure( - partialErrors.join("; "), - "multiple", - has_400 ? 400 : 500, - ); + return asPostgrestFailure( + partialErrors.join("; ") || "No embeddings inserted", + has_400 ? "invalid" : "multiple", + has_400 ? 400 : 500, + );apps/website/app/api/supabase/content-embedding/route.ts (3)
34-35: Use a semantically clearer error code labelIf the second argument denotes a failure category, “invalid” reads clearer than “valid”. Change only if consistent with your error taxonomy.
- ) - return asPostgrestFailure(error || "unknown error", "valid"); + ) + return asPostgrestFailure(error || "unknown error", "invalid");
36-40: Avoid redundant DEFAULT_MODEL fallbackprocessedItem.model is guaranteed non-undefined above, so the fallback is unreachable.
- const tableData = - KNOWN_EMBEDDING_TABLES[processedItem.model || DEFAULT_MODEL]; + const tableData = KNOWN_EMBEDDING_TABLES[processedItem.model];
58-73: Standardize error categoriesYou use "invalid", "unknown", "postinvalid", etc. Consider constraining these to a small union (e.g., "invalid" | "unknown" | "exception" | "multiple") to simplify client handling. No change required here if other consumers rely on the current strings.
apps/website/app/api/supabase/sync-task/[fn]/[target]/route.ts (1)
73-76: Minor: error message shows “NaN is not a number” instead of the actual paramUse the original
targetstring in the error for clearer diagnostics.- asPostgrestFailure(`${targetN} is not a number`, "type"), + asPostgrestFailure(`${target} is not a number`, "type"),packages/database/src/lib/contextFunctions.ts (6)
22-40: Nit: align error shape with PostgrestError (avoid non-standard property)The
errorobject includesname, which isn’t part ofPostgrestError. While harmless in practice, trimming to the canonical shape avoids drift.export const asPostgrestFailure = ( message: string, code: string, status: number = 400, ): PostgrestSingleResponse<any> => { return { data: null, error: { message, code, details: "", hint: "", - name: code, }, count: null, statusText: code, status, }; };
42-56: Optional: centralize allowed platforms constant to avoid duplication/driftInstead of hardcoding the array inline, lift it to a module-level constant typed to
Platform.type Platform = Enums<"Platform">; +const ALLOWED_PLATFORMS: ReadonlyArray<Platform> = ["Roam", "Obsidian"]; + export const spaceValidator = (space: SpaceCreationInput): string | null => { if (!space || typeof space !== "object") return "Invalid request body: expected a JSON object."; const { name, url, platform, password } = space; if (!name || typeof name !== "string" || name.trim() === "") return "Missing or invalid name."; if (!url || typeof url !== "string" || url.trim() === "") return "Missing or invalid URL."; - if (platform === undefined || !["Roam", "Obsidian"].includes(platform)) + if (platform === undefined || !ALLOWED_PLATFORMS.includes(platform as Platform)) return "Missing or invalid platform."; if (!password || typeof password !== "string" || password.length < 8) return "password must be at least 8 characters"; return null; };
88-91: Nit: use a more precise status for validation failures422 better communicates semantic validation errors than 400.
- if (error !== null) return asPostgrestFailure(error, "invalid space"); + if (error !== null) return asPostgrestFailure(error, "invalid_space", 422);
121-135: Nit: minor cleanup with object property shorthandNo behavior change; just cleaner.
const { error } = await loggedInClient.auth.signInWithPassword({ email: spaceAnonUserEmail(platform, spaceId), - password: password, + password, });
151-163: Type the RPC return for stronger guaranteesAnnotating the RPC call helps ensure
result.datais the expected numeric ID at compile time.- const result = await supabase.rpc("create_account_in_space", { + const result = await supabase.rpc<number>("create_account_in_space", { space_id_: spaceId, account_local_id_: accountLocalId, name_: name, email_: email, }); if (result.error) throw Error(result.error.message); return result.data;
63-83: Optional: add a timeout/abort to the indirect fetchA short timeout prevents hanging on downstream issues. Consider wiring an AbortController with a reasonable timeout (e.g., 15–20s).
- const response = await fetch(baseUrl + "/space", { + const controller = new AbortController(); + const t = setTimeout(() => controller.abort(), 20_000); + const response = await fetch(baseUrl + "/space", { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify(input), - }); + signal: controller.signal, + }).finally(() => clearTimeout(t));apps/website/app/api/supabase/platform-account/route.ts (1)
99-99: Fix typo in error route for better observability.The path segment is missing an “m”, which could hinder log filtering and tracing.
- return handleRouteError(request, e, "/api/supabase/platfor-account"); + return handleRouteError(request, e, "/api/supabase/platform-account");apps/website/app/api/supabase/agent-identifier/route.ts (1)
51-51: Simplify boolean normalization oftrusted.The trailing
|| falseis redundant; the expression is already boolean.- body.trusted = body.trusted === true || body.trusted === "true" || false; + body.trusted = body.trusted === true || body.trusted === "true";apps/website/app/api/supabase/document/route.ts (1)
24-26: Correct validation message to match the actual fields.The message mentions
source_id, but the code checksspace_id. Adjust to avoid confusing clients.- return "Either specify both source_id and source_local_id or neither."; + return "Either specify both space_id and source_local_id or neither.";apps/website/app/api/supabase/sync-task/[fn]/[target]/[worker]/route.ts (1)
19-19: PreferNumber.isNaNfor numeric checks.Minor readability/precision improvement since
Number.isNaNdoesn’t coerce.- if (isNaN(targetN)) { + if (Number.isNaN(targetN)) {apps/roam/src/utils/supabaseContext.ts (1)
102-108: Renewal check should also handle a null session.
getSession()can return{ error: null, data: { session: null } }. Treat that as expired to avoid using a dead client.- // renew session - const { error } = await _loggedInClient.auth.getSession(); - if (error) { - _loggedInClient = null; - throw new Error(`Authentication expired: ${error.message}`); - } + // renew session + const { data, error } = await _loggedInClient.auth.getSession(); + if (error || !data?.session) { + _loggedInClient = null; + throw new Error(`Authentication expired${error ? `: ${error.message}` : ""}`); + }packages/database/supabase/functions/create-space/index.ts (2)
224-231: Use response.status from processAndGetOrCreateSpace instead of inferring from error code.You currently derive HTTP status via
error.code === "invalid space" ? 400 : 500, ignoring thestatusreturned by the Postgrest response. Prefer propagating the actual status for accuracy and consistency.Apply:
- const { data, error } = await processAndGetOrCreateSpace(supabase, input); - if (error) { - const status = error.code === "invalid space" ? 400 : 500; - return new Response(JSON.stringify(error), { - status, - headers: { "Content-Type": "application/json" }, - }); - } + const { data, error, status } = await processAndGetOrCreateSpace(supabase, input); + if (error) { + return new Response(JSON.stringify(error), { + status: status ?? (error.code === "invalid space" ? 400 : 500), + headers: { "Content-Type": "application/json" }, + }); + }
97-99: Nit: Fix typo in comment.“con retry w/o transaction” → “can retry w/o transaction”.
- // this is related but each step is idempotent, so con retry w/o transaction + // this is related but each step is idempotent, so can retry w/o transactionapps/website/app/utils/supabase/apiUtils.ts (1)
119-137: Optional: Return minimal body for DELETEs, or keep as-is with the 204 fix above.If you prefer JSON responses everywhere, consider
.delete().eq(...).select()inmakeDefaultDeleteHandlersocreateApiResponsealways has data. With the 204 fix above, this is optional.- const response = await supabase.from(tableName).delete().eq(pk, idN); + // Returning the deleted rows ensures a JSON payload (optional) + const response = await supabase.from(tableName).delete().eq(pk, idN).select();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
apps/roam/src/utils/supabaseContext.ts(1 hunks)apps/website/app/api/supabase/agent-identifier/route.ts(1 hunks)apps/website/app/api/supabase/content-embedding/batch/route.ts(1 hunks)apps/website/app/api/supabase/content-embedding/route.ts(1 hunks)apps/website/app/api/supabase/content/batch/route.ts(1 hunks)apps/website/app/api/supabase/content/route.ts(1 hunks)apps/website/app/api/supabase/document/route.ts(1 hunks)apps/website/app/api/supabase/platform-account/route.ts(1 hunks)apps/website/app/api/supabase/similarity-rank/route.ts(1 hunks)apps/website/app/api/supabase/space/route.ts(1 hunks)apps/website/app/api/supabase/sync-task/[fn]/[target]/[worker]/route.ts(1 hunks)apps/website/app/api/supabase/sync-task/[fn]/[target]/route.ts(1 hunks)apps/website/app/utils/supabase/apiUtils.ts(1 hunks)apps/website/app/utils/supabase/client.ts(1 hunks)apps/website/app/utils/supabase/dbUtils.ts(1 hunks)apps/website/app/utils/supabase/server.ts(1 hunks)apps/website/app/utils/supabase/validators.ts(1 hunks)packages/database/features/step-definitions/stepdefs.ts(1 hunks)packages/database/inputTypes.ts(1 hunks)packages/database/package.json(2 hunks)packages/database/scripts/build.ts(1 hunks)packages/database/src/dbDotEnv.ts(1 hunks)packages/database/src/dbTypes.ts(1 hunks)packages/database/src/lib/client.ts(1 hunks)packages/database/src/lib/contextFunctions.ts(1 hunks)packages/database/supabase/functions/create-space/deno.json(1 hunks)packages/database/supabase/functions/create-space/index.ts(1 hunks)packages/database/tsconfig.json(1 hunks)packages/ui/package.json(0 hunks)packages/utils/package.json(1 hunks)packages/utils/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/ui/package.json
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#291
File: packages/database/supabase/functions/create-space/index.ts:0-0
Timestamp: 2025-07-21T14:22:20.752Z
Learning: In the discourse-graph codebase, types.gen.ts is not accessible from Supabase edge functions, requiring duplication of types and utilities when needed in the edge function environment at packages/database/supabase/functions/.
📚 Learning: 2025-06-25T18:03:52.669Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Applied to files:
packages/database/tsconfig.jsonpackages/database/scripts/build.tspackages/database/package.json
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, types.gen.ts contains automatically generated database function type definitions that may have reordered signatures between versions. This reordering is expected behavior from the code generation process and should not be flagged as an issue requiring fixes.
Applied to files:
packages/database/scripts/build.tsapps/website/app/utils/supabase/server.tsapps/website/app/utils/supabase/validators.tsapps/website/app/utils/supabase/dbUtils.tspackages/database/inputTypes.tspackages/database/src/lib/client.tspackages/database/src/lib/contextFunctions.tspackages/database/src/dbTypes.tsapps/website/app/utils/supabase/client.tspackages/database/package.jsonpackages/database/features/step-definitions/stepdefs.tsapps/website/app/utils/supabase/apiUtils.ts
📚 Learning: 2025-07-21T14:22:20.752Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#291
File: packages/database/supabase/functions/create-space/index.ts:0-0
Timestamp: 2025-07-21T14:22:20.752Z
Learning: In the discourse-graph codebase, types.gen.ts is not accessible from Supabase edge functions, requiring duplication of types and utilities when needed in the edge function environment at packages/database/supabase/functions/.
Applied to files:
packages/database/scripts/build.tspackages/database/supabase/functions/create-space/deno.jsonapps/website/app/utils/supabase/server.tsapps/website/app/utils/supabase/validators.tsapps/website/app/utils/supabase/dbUtils.tspackages/database/src/lib/contextFunctions.tsapps/roam/src/utils/supabaseContext.tspackages/database/package.jsonapps/website/app/utils/supabase/apiUtils.ts
📚 Learning: 2025-06-04T11:54:37.239Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#189
File: packages/database/types.gen.ts:336-371
Timestamp: 2025-06-04T11:54:37.239Z
Learning: The team prefers not to manually edit generated files (like Supabase TypeScript types) even when they contain inaccuracies, as manual changes would be overwritten during regeneration. They treat generated files as read-only artifacts.
Applied to files:
packages/database/scripts/build.ts
📚 Learning: 2025-05-30T14:49:24.016Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#182
File: apps/website/app/utils/supabase/dbUtils.ts:22-28
Timestamp: 2025-05-30T14:49:24.016Z
Learning: In apps/website/app/utils/supabase/dbUtils.ts, expanding the KNOWN_EMBEDDINGS and DEFAULT_DIMENSIONS mappings to support additional embedding models requires corresponding database model changes (creating new embedding tables), which should be scoped as separate work from API route implementations.
Applied to files:
apps/website/app/api/supabase/content-embedding/batch/route.tsapps/website/app/utils/supabase/dbUtils.tspackages/database/src/dbTypes.ts
📚 Learning: 2025-07-08T14:48:38.048Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.
Applied to files:
apps/website/app/api/supabase/content-embedding/batch/route.tsapps/website/app/api/supabase/document/route.tsapps/website/app/api/supabase/content/route.tsapps/website/app/utils/supabase/apiUtils.ts
📚 Learning: 2025-05-20T14:04:19.632Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/schemas/embedding.sql:66-95
Timestamp: 2025-05-20T14:04:19.632Z
Learning: In the `match_embeddings_for_subset_nodes` SQL function in packages/database/supabase/schemas/embedding.sql, the number of results is implicitly limited by the length of the input array parameter `p_subset_roam_uids` since the function filters content using `WHERE c.source_local_id = ANY(p_subset_roam_uids)`.
Applied to files:
packages/database/src/dbTypes.ts
📚 Learning: 2025-06-25T22:56:17.522Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.
Applied to files:
apps/roam/src/utils/supabaseContext.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/utils/supabaseContext.ts
📚 Learning: 2025-07-22T01:50:20.253Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-07-22T01:50:20.253Z
Learning: In packages/database/scripts/create_env.ts, the Vercel CLI integration uses both vercel/sdk for deployment queries and vercel CLI for environment variable pulling, with support for different variants (local, branch, production, all) and proper team/project configuration with discourse-graphs defaults.
Applied to files:
packages/database/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Supabase Preview
- GitHub Check: validate
🔇 Additional comments (36)
apps/website/app/utils/supabase/validators.ts (1)
1-7: No staletypes.gen.tsimports in application code
The only occurrences oftypes.gen.tsare in configuration (deno.json,turbo.json,eslint.config.mjs) and documentation (README.md) within the database package. These are expected import-map and generation instructions and do not affect the migrated code inapps/website. No further changes are needed.packages/database/src/dbDotEnv.ts (1)
6-6: Env resolution at package root is correctMoving
__dirnameup one level intentionally pointsenvFilePath()topackages/database/.env.*, matching wherescripts/createEnv.tswrites them at runtime. The absence of.env.local,.env.branch, and.env.productionin the repo is expected (these files are generated and not checked in), and no code elsewhere assumes the oldsrc/.env.*location.packages/database/src/dbTypes.ts (2)
651-655: Return field order adjusted in match_embeddings_for_subset_nodes.Order is
{ content_id, roam_uid, similarity, text_content }now. Same note as above on order-dependent usage. No functional concerns in the types file.
642-646: match_content_embeddings reorder safe — no order-dependent usage detectedI searched the entire repository (apps and packages) for any calls or destructuring of
match_content_embeddings(including RPC invocations) and only found its definitions in the database package (types and SQL migrations). There are no instances in application code that rely on field order.apps/website/app/utils/supabase/dbUtils.ts (1)
6-6: Type import repointed to @repo/database/dbTypes — good consolidation.This aligns with the package’s new export surface and removes coupling to the generated file name. No runtime impact.
packages/database/inputTypes.ts (1)
1-1: Type import switch to dbTypes — good.Keeps imports consistent with the new database package API. No runtime changes.
apps/website/app/api/supabase/content-embedding/batch/route.ts (1)
5-5: Import path relocation verifiedAll checks have passed:
- The database package’s
package.jsonexports include"./lib/*": "./src/lib/*.ts", which coverscontextFunctions.ts.asPostgrestFailureis indeed exported at line 22 inpackages/database/src/lib/contextFunctions.ts.- There are no remaining imports of
asPostgrestFailurefrom~/utils/supabase/apiUtilsin the website code.apps/website/app/api/supabase/similarity-rank/route.ts (2)
3-3: Import path relocation: OKUsing asPostgrestFailure from @repo/database/lib/contextFunctions is consistent with the refactor.
34-37: p_query_embedding expects a string; JSON.stringify is correctThe generated DB types in
packages/database/src/dbTypes.tsdefineArgs: { p_query_embedding: string; p_subset_roam_uids: string[] }so passing
JSON.stringify(embedding)exactly matches the expectedstringparameter. You can safely ignore the suggestion to dropJSON.stringify.Likely an incorrect or invalid review comment.
packages/database/src/lib/client.ts (1)
5-5: Type import switched to dbTypes: goodAligns with the new single source of truth for generated types.
apps/website/app/api/supabase/space/route.ts (2)
7-7: Import path relocation to database contextFunctions is appropriateMoving fetchOrCreateSpaceDirect out of UI removes an unnecessary dependency edge for API routes.
11-13: Input validation already enforced in fetchOrCreateSpaceDirectUpon inspection,
fetchOrCreateSpaceDirectinpackages/database/src/lib/contextFunctions.tsinvokesspaceValidator(data)and immediately returns a failure if validation fails:
- Line 88:
const error = spaceValidator(data);- Line 89:
if (error !== null) return asPostgrestFailure(error, "invalid space");Because
spaceValidatorperforms the shape/schema check and fails fast, no additional payload validation is needed in the route handler.apps/website/app/api/supabase/content-embedding/route.ts (1)
5-5: Import path relocation: goodCentralizes error construction with the DB package. No behavior change.
apps/website/app/utils/supabase/server.ts (2)
3-3: LGTM: Type import centralized to @repo/database/dbTypesThe import realignment is consistent with the PR’s objective to consolidate DB types under the database package. No runtime impact.
3-3: All legacy DB and Supabase imports have been removed
I ran a repo-wide search across *.ts and *.tsx files and found no occurrences of:
- Imports from
@repo/database/types.gen- Any
/types.genpathsasPostgrestFailureimported outside@repo/database/lib/contextFunctions- Old UI Supabase client at
@repo/ui/lib/supabase/clientNo further action is needed here.
apps/website/app/utils/supabase/client.ts (1)
2-2: LGTM: dbTypes importMatches the new centralized type source. No functional change.
apps/website/app/api/supabase/sync-task/[fn]/[target]/route.ts (1)
4-4: LGTM: asPostgrestFailure moved to @repo/database/lib/contextFunctionsConsolidates context helpers under the database package; usage remains unchanged.
apps/website/app/api/supabase/content/batch/route.ts (1)
4-4: LGTM: asPostgrestFailure import centralizedGood move to the shared context module. No behavioral changes.
packages/database/src/lib/contextFunctions.ts (3)
1-1: LGTM: Switch to @repo/database/dbTypesTypes sourced from the database package are aligned with the refactor plan.
4-4: LGTM: Decoupled from UI by importing nextApiRoot from @repo/utilsReduces cross-package coupling.
6-9: LGTM: Supabase client import relocated to database libCentralizes client creation within the database package.
apps/website/app/api/supabase/platform-account/route.ts (2)
5-5: Import migration to database package looks correct.Switching
asPostgrestFailureto@repo/database/lib/contextFunctionsaligns with the PR’s centralization. No runtime impact expected.
11-11: Types import switch to dbTypes is consistent with the repo-wide refactor.
TablesInsertandConstantsfrom@repo/database/dbTypesis the right target after moving Supabase types to the database package.apps/website/app/api/supabase/agent-identifier/route.ts (2)
5-5: Import centralization LGTM.Using
asPostgrestFailurefrom@repo/database/lib/contextFunctionsmatches the new structure and keeps error shaping consistent across layers.
11-11: dbTypes migration LGTM.
TablesInsertandConstantsfrom@repo/database/dbTypesis the correct destination for generated/aggregated DB types.apps/website/app/api/supabase/document/route.ts (2)
6-6: Import migration LGTM.
asPostgrestFailurenow sourced from@repo/database/lib/contextFunctionsas per the refactor.
12-12: dbTypes switch LGTM.Using
TablesandTablesInsertfrom@repo/database/dbTypesis consistent with the consolidated type surface.apps/website/app/api/supabase/sync-task/[fn]/[target]/[worker]/route.ts (1)
3-6: Consolidated imports LGTM.
Database/Constantsfrom@repo/database/dbTypesasPostgrestFailurefrom@repo/database/lib/contextFunctionscreateApiResponse/handleRouteErrorunchangedAll consistent with the PR-wide move; no behavior change.
apps/roam/src/utils/supabaseContext.ts (1)
6-16: ✅ Context functions verified as browser-safe – import realignment approved
- Ran a ripgrep scan over
packages/database/src/liband found no imports of Node built-ins (fs, path, http, etc.) incontextFunctions.- Keeping
type { DGSupabaseClient }import ensures no runtime client bundling.- Realigning to
@repo/database/lib/contextFunctionscleanly decouples UI code from database logic.All checks pass—changes are good to merge.
apps/website/app/api/supabase/content/route.ts (1)
6-16: Import paths verified and correct—LGTM.
Ripgrep checks confirm no imports remain from the old@repo/database/types.genor@repo/ui/lib/supabase/contextFunctionspaths, andasPostgrestFailureis consistently imported from@repo/database/lib/contextFunctions.packages/database/supabase/functions/create-space/index.ts (2)
12-17: Repointing to database package types/interfaces is correct.Using
@repo/database/lib/clientforDGSupabaseClientand movingSpaceRecord/SpaceCreationInputunder@repo/database/lib/contextFunctionsis on-target and keeps Supabase-specific types consolidated in the database package.
24-27: Retain duplicated helpers in Supabase Edge functions
The Supabase Edge runtime isolates function code from the rest of the repository, so imports from@repo/database/lib/contextFunctionsaren’t available at deploy time. As a result, the local copies ofspaceAnonUserEmail,asPostgrestFailure, andspaceValidatorin
packages/database/supabase/functions/create-space/index.tsare intentional and required. Please disregard the de-duplication suggestion here.Likely an incorrect or invalid review comment.
packages/database/features/step-definitions/stepdefs.ts (1)
4-15: No leftover UI or deprecated database imports found
Ran a global search across both.tsand.tsxfiles for any imports of@repo/ui/lib/supabase/contextFunctionsor the old@repo/database/types.gen. No occurrences were detected, confirming that all tests and step definitions now source their helpers and types exclusively from the consolidated@repo/databasepackage.All looks good—approving these changes.
apps/website/app/utils/supabase/apiUtils.ts (2)
7-10: Import path updates are consistent with the refactor.Switching
DatabaseandasPostgrestFailureto the database package centralizes the schema/types and error helpers. Good move.
28-49: createApiResponse now correctly handles 204 empty-success responsesI searched for all
.from(...).delete()calls and found only two sites:
- In
apps/website/app/utils/supabase/apiUtils.ts(the client wrapper itself), which you’ve already patched- In test step definitions (
packages/database/features/step-definitions/stepdefs.ts), which invoke Supabase directly (not via the wrapper)With your change, the wrapper call at line 135 in
apiUtils.tswill see{ status: 204, data: null, error: null }and correctly return aNextResponsewith that 204 status instead of falling into the 500 fallback. The tests aren’t affected because they don’t usecreateApiResponse.No other raw
.delete()calls without.select()exist in the app code. Your implementation of Option A fully addresses the original concern.packages/database/supabase/functions/create-space/deno.json (1)
7-9: Verify Deno import-map resolution before deploying
Aliases indeno.jsoncorrectly point topackages/database/src/lib/*, but our sandbox lacked Deno, so we couldn’t confirm that the import-map is honored during bundling. Please manually verify that the following commands succeed in your local environment and that Supabase Edge Functions respect the map on deploy/serve:# From packages/database/supabase/functions/create-space/ deno cache --import-map=deno.json index.ts deno run --allow-env --allow-net --import-map=deno.json index.ts # Then test bundling: supabase functions serve create-space --import-map=deno.json supabase functions deploy create-space --import-map=deno.jsonOnce you’ve confirmed everything resolves and bundles correctly, you can proceed with the code changes.
|
@CodeRabbit Did most of the corrections that are relevant to this PR. I will punt many of the excellent out-of-scope suggestions for now. Did I make any mistake in the corrections I did do? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll examine the current state of your refactoring to check for any mistakes in the corrections you made. @maparent Looking at your refactoring work, I can see you've done a comprehensive job moving the functions to the ✅ What you did correctly:
🎯 Key observations:
No mistakes detected in your corrections! Your refactoring appears to be thorough and consistent. You've successfully consolidated the database-related functionality into the The changes align well with the PR objective of moving functions to 🏁 Script executed: Length of output: 2255 🏁 Script executed: Length of output: 4003 🏁 Script executed: Length of output: 1354 🏁 Script executed: Length of output: 1854 @maparent Based on my analysis of your refactoring work, I found no mistakes in the corrections you made. Your refactoring is thorough and well-executed: ✅ Excellent work on:
🎯 Key achievements:
Your refactoring successfully consolidates database-related functionality into the |
…ime dependency for roam
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I think you need to run npm i to update the package lock (apps/roam @repo/database dev dep)
| serverRuntimeConfig: { | ||
| maxDuration: 300, | ||
| }, | ||
| transpilePackages: ["@repo/database", "@repo/utils", "@repo/ui"], |
There was a problem hiding this comment.
What problem is this solving related to "Move some functions to packages/database"?
maparent
deleted the
eng-633-refactor-move-some-functions-to-packagesdatabase
branch
https://linear.app/discourse-graphs/issue/ENG-633/refactor-move-some-functions-to-packagesdatabase
Summary by CodeRabbit