ENG-785 Use commonjs in whole project #377
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughRepo-wide shift from ESM to CommonJS across multiple packages, associated TypeScript config updates to emit builds, and script entry-point checks adjusted. Several Roam utilities switched to ES imports and centralized types. One prompt in hyde.ts was condensed with an added output constraint. Next.js website config removed transpilePackages. Changes
Sequence Diagram(s)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: 7
🧹 Nitpick comments (10)
apps/roam/src/utils/upsertNodesAsContentWithEmbeddings.ts (4)
48-52: Safer URL construction to prevent double slashes and missing protocolConcatenating strings can yield
//embeddings...or miss a slash depending onnextApiRoot(). Using URL is more robust.Apply this minimal change:
- const response = await fetch(nextApiRoot() + "/embeddings/openai/small", { + const response = await fetch( + new URL("/embeddings/openai/small", nextApiRoot()).toString(), + { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ input: batch }), - }); + } + );Optional follow-up: add a simple retry with exponential backoff for transient 429/5xx responses.
// place near top-level async function fetchWithRetry(input: RequestInfo, init: RequestInit, max = 3) { let attempt = 0, lastErr: unknown; while (attempt++ < max) { try { const res = await fetch(input, init); if (res.ok) return res; // Retry on throttling and 5xx if (res.status === 429 || (res.status >= 500 && res.status < 600)) { await new Promise(r => setTimeout(r, 250 * 2 ** (attempt - 1))); } else { return res; // non-retryable } } catch (e) { lastErr = e; await new Promise(r => setTimeout(r, 250 * 2 ** (attempt - 1))); } } if (lastErr) throw lastErr; // Final attempt return fetch(input, init); }Then replace the
fetch(...)call accordingly.
128-131: Early return hides actionable errors when missing Supabase contextThe function logs and returns when
userIdis missing. Upstream callers won’t know it silently aborted.Consider either:
- Throwing a typed error (preferred), or
- Returning a discriminated union
{ ok: false, error: 'NoSupabaseContext' }and documenting it.This keeps failure handling explicit.
151-156: Mismatch case should fail loudly (throw) to avoid partial ingestionA mismatch between nodes and embeddings indicates a data integrity issue. Logging and returning can leave the system in an inconsistent or confusing state.
Throw an error here and let the caller decide whether to retry or abort the whole ingestion batch.
5-7: Replace localanyaliases with exported database typesTo strengthen type safety and catch mismatches at compile time, import the actual types from
@repo/databaseinstead of usinganyaliases. We’ve located the canonical definitions:
- LocalContentDataInput is exported from
@repo/database/inputTypes- DGSupabaseClient is exported from
@repo/database/lib/client- Json is exported from
@repo/database/dbTypesIn
apps/roam/src/utils/upsertNodesAsContentWithEmbeddings.ts, replace the local aliases at the top of the file:- type LocalContentDataInput = any; - type DGSupabaseClient = any; - type Json = any; + import type { LocalContentDataInput } from '@repo/database/inputTypes'; + import type { DGSupabaseClient } from '@repo/database/lib/client'; + import type { Json } from '@repo/database/dbTypes';This will ensure that any structural changes to your database types are caught by the compiler rather than at runtime.
packages/database/scripts/deploy.ts (2)
75-75: Prefer Node’s canonical CJS entry-point guard
__filename === process.argv[1]can fail with symlinks or when invoked through wrappers. The conventional CJS guard is more robust.Apply:
-if (__filename === process.argv[1]) main(); +if (require.main === module) main();
16-27: Make the “on main and pristine” check resilient to ahead/behind decorationsThe exact comparison to
"## main...origin/main"breaks when Git adds[ahead N],[behind N], or additional status hints. This can mistakenly skip or proceed with deployments.Refactor to:
- Determine branch via
git rev-parse --abbrev-ref HEADand compare to"main".- Ensure a clean tree via
git status --porcelainbeing empty.I can propose a minimal diff if you want this hardened in this PR.
packages/database/package.json (1)
14-22: Dev/build/genenv flow looks good—consider an explicit env-generation stepThe repository’s
tsconfig.jsoninpackages/databasealready excludes thesupabasedirectory, so TypeScript won’t traverse serverless files during builds.
- LGTM on using
tsx scripts/createEnv.mts(ESM exception).- The current
buildscript (tsc && tsx scripts/build.ts && npm run genenv -- local) still writes environment artifacts as part of a full build. To prevent accidental file mutations in CI or downstream workspace builds, consider:
- Extracting env generation into a dedicated npm target (e.g.,
"build:env": "npm run genenv -- local").- Optionally invoking that target in a
postbuildhook gated by a CI-only environment variable.packages/database/scripts/lint.ts (1)
6-8: Path base is correctly switched to __filename/__dirname for CJSThis removes the import.meta.url dependency and matches the repo-wide CommonJS target. One nit: for consistency with other Node built-ins in this file (node:child_process, node:fs), consider importing from node:path rather than path.
apps/roam/src/utils/supabaseContext.ts (1)
9-16: Type-only imports + centralizing db types: LGTM; minor consistency nitGreat move using
import typeforEnumsandDGSupabaseClient, and pulling runtime fns from a single module. One nit: you defineconst platform: Platform = "Roam"but still pass string literals later—prefer the variable for consistency and fewer typos.Apply:
- const userId = await fetchOrCreatePlatformAccount({ - platform: "Roam", + const userId = await fetchOrCreatePlatformAccount({ + platform, accountLocalId, name: personName, email: personEmail, spaceId, password: spacePassword, }); - _contextCache = { - platform: "Roam", + _contextCache = { + platform, spaceId, userId, spacePassword, };apps/roam/src/utils/hyde.ts (1)
96-101: Tighten output-shaping in prompt; trim responseThe added constraint helps. Make it even stricter to avoid quotes/fences leaking into downstream embedding/search, and trim the returned text.
Update the prompt:
-Only return the text of the hypothetical node. +Only return the text of the hypothetical node, with no surrounding quotes, code fences, or commentary. Do not prefix or suffix anything.Additionally, trim the LLM response (outside this hunk):
- return await response.text(); + return (await response.text()).trim();Consider a safe timeout fallback—
AbortSignal.timeoutisn’t supported in all browsers/Roam environments:// near the fetch call const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), API_CONFIG.LLM.TIMEOUT_MS); try { response = await fetch(API_CONFIG.LLM.URL, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify(requestBody), signal: controller.signal, }); } finally { clearTimeout(timer); }Please verify whether
AbortSignal.timeoutexists in your target runtime; if yes, keep it. If not, use the fallback shown above.
📜 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 (17)
apps/roam/src/utils/conceptConversion.ts(1 hunks)apps/roam/src/utils/hyde.ts(2 hunks)apps/roam/src/utils/supabaseContext.ts(1 hunks)apps/roam/src/utils/upsertNodesAsContentWithEmbeddings.ts(1 hunks)apps/website/next.config.ts(0 hunks)packages/database/cucumber.json(1 hunks)packages/database/package.json(1 hunks)packages/database/scripts/build.ts(2 hunks)packages/database/scripts/deploy.ts(1 hunks)packages/database/scripts/lint.ts(2 hunks)packages/database/src/dbDotEnv.ts(1 hunks)packages/database/tsconfig.json(1 hunks)packages/eslint-config/package.json(0 hunks)packages/ui/package.json(1 hunks)packages/ui/tsconfig.json(1 hunks)packages/utils/package.json(1 hunks)packages/utils/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/eslint-config/package.json
- apps/website/next.config.ts
🧰 Additional context used
🧠 Learnings (11)
📚 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/upsertNodesAsContentWithEmbeddings.tsapps/roam/src/utils/hyde.tsapps/roam/src/utils/supabaseContext.ts
📚 Learning: 2025-06-22T10:40:21.679Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.
Applied to files:
apps/roam/src/utils/upsertNodesAsContentWithEmbeddings.ts
📚 Learning: 2025-06-25T18:11:58.352Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#241
File: apps/website/eslint.config.mjs:6-13
Timestamp: 2025-06-25T18:11:58.352Z
Learning: In TypeScript ESLint parser options, `parserOptions.project: true` is a valid boolean value that has been supported since version 5.52.0. When set to `true`, it automatically finds the nearest `tsconfig.json` file for each source file being linted, eliminating the need to specify explicit paths like `"./tsconfig.json"`. This is especially useful for monorepos and projects with multiple TypeScript configurations.
Applied to files:
packages/utils/tsconfig.json
📚 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/package.jsonpackages/database/scripts/build.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
📚 Learning: 2025-05-22T23:48:45.450Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/scripts/deploy.ts:0-0
Timestamp: 2025-05-22T23:48:45.450Z
Learning: The database deployment script in packages/database/scripts/deploy.ts requires that the branch is 'main' and pristine (no uncommitted or unpushed changes) before allowing deployment to proceed.
Applied to files:
packages/database/scripts/deploy.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/utils/conceptConversion.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/utils/conceptConversion.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:
apps/roam/src/utils/supabaseContext.tspackages/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/roam/src/utils/supabaseContext.ts
📚 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.ts
🔇 Additional comments (6)
apps/roam/src/utils/upsertNodesAsContentWithEmbeddings.ts (1)
4-4: Named export confirmed; no change requiredThe function
nextApiRootis declared as a named export inpackages/utils/src/execContext.ts(line 3), and all existing imports use named syntax. There is no default export fornextApiRoot, so the current importimport { nextApiRoot } from "@repo/utils/execContext";is correct and poses no risk of runtime breakage. You can safely ignore the suggestion to switch to a default import.
Likely an incorrect or invalid review comment.
packages/database/scripts/lint.ts (1)
69-69: Entry-point guard works under tsx/Node for CJSif (__filename === process.argv[1]) is a reliable direct-execution check after the CJS migration. LGTM.
apps/roam/src/utils/hyde.ts (1)
6-6: Import migration to ES module is correctSwitching
nextApiRootto an ES import from@repo/utils/execContextmatches the repo-wide CommonJS/ESM consolidation. No functional changes.apps/roam/src/utils/conceptConversion.ts (1)
6-6: CentralizingLocalConceptDataInputtype import: LGTMImporting the shared type from
@repo/database/inputTypesreduces duplication and keeps schema coupling in one place. The functions using it remain type-safe, and usage ofnode.typeas UID aligns with prior decisions in this codebase.packages/database/scripts/build.ts (2)
17-20: Write file restructuring: LGTMMultiline
writeFileSyncwith a header improves readability; behavior stays identical.
6-6: Confirmed CJS context for build script; no changes needed
- packages/database/package.json has no
"type"field (defaults to CommonJS)- The build command (
tsc && tsx scripts/build.ts …) runs under that CJS context, so__filenamewill be defined anddirname(__filename)safe
maparent
force-pushed
the
eng-785-use-commonjs-in-whole-project
branch
from
2884b73 to
c73ec8c
Compare
|
@mdroidian ok, this is functional. cucumber works. build, dev and check-types succeed locally. |
|
ok check-types was easy to solve, now stumped by deploy issue. Works locally (even after wiping all dist folders.) |
|
Ok, did fail locally after wiping dist after all. Not sure what I did wrong the first time. So... does deploy not come after a build? |
FYI, This is still being imported via Search for |
| import { readFileSync, existsSync } from "fs"; | ||
| import { join, dirname } from "path"; | ||
| import { join, dirname, basename } from "path"; | ||
| import { fileURLToPath } from "url"; |
| "dev": "supabase start && tsx scripts/createEnv.mts && supabase functions serve", | ||
| "stop": "supabase stop", | ||
| "check-types": "tsc --noEmit --skipLibCheck", | ||
| "check-types": "tsc --emitDeclarationOnly --skipLibCheck", |
There was a problem hiding this comment.
Why do we want the check-types to emit declarations? Aren't we just running a check here?
There was a problem hiding this comment.
If the package declarations are not generated, the check of the apps, that refer to those declarations, fails.
There was a problem hiding this comment.
@maparent
Can you show me an example of what you are referring to? This wasn't the case previously when I initially checked in check-types, and I just just changed them all to noEmit and it ran successfully for me
I also cd packages/database and ran npm run check-types successfully
No, but it did ultimately point to the issue. I asked if roam was built before deploy, and it is, through compile. But compile now uses dbDotEnv, and that is only built when the database is built.
2 and 3 are more complex, I want your feedback on whether it's worth doing. I went to import, and then back to require so I could give a better error message when the database is not built. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
- quick and dirty, already pushed: build the database before calling deploy.
If deploy always requires this, then I think we could achieve this via turbo.json with something like "dependsOn": ["@repo/database#build"], It would then be in a single place.
But I'll leave that up to you.
One exception gets kept as a mts file. Ensure all projects' builds result in a dist folder. Revert most of #359, so we use imports instead of require.
maparent
force-pushed
the
eng-785-use-commonjs-in-whole-project
branch
from
f65b855 to
dac8623
Compare
|
I got your latest comment by mail but do not see it here? Solved in #801. |
|
I think |
|
Process to reproduce the issue :
|
https://linear.app/discourse-graphs/issue/ENG-785/use-commonjs-in-whole-project
Note: At this point, cucumber tests are broken, still working on that.
Summary by CodeRabbit
Bug Fixes
Chores