ENG-801 Compile database before dev #386
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Does this suggest we need to have docker/supabase running to run roam dev?
We want to try and avoid this if at all possible.
|
Why is it the current situation? What changed to make it the current situation (as it was not recently). Is it avoidable? If not, why not? |
|
Replied elsewhere. |
Make build depend on building the schema, but not check-types. (Still thinking about the pros and cons here)
Introduce SUPABASE_USE_DB=none. (Now the default value.) dev and build in database will not activate supabase if that is set.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntroduces a “none” database variant and makes it the default. Updates db environment utilities and exports to support it, adds a dev script that compiles and conditionally serves, modifies the build script to early-exit for “none”, adjusts Turbo tasks to isolate schema generation, and updates documentation accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant DevScript as scripts/dev.ts
participant Env as dbDotEnv.getVariant()
participant Serve as npm run serve
participant Compile as npm run compile
Dev->>DevScript: npm run dev
DevScript->>Compile: Build TS (local only)
DevScript->>Env: getVariant()
alt variant == "none"
DevScript-->>Dev: Log "Not using the database" and exit 0
else variant in {local, branch, production}
DevScript->>Serve: Start local database services
end
sequenceDiagram
autonumber
actor CI as CI/Local
participant BuildScript as scripts/build.ts
participant Env as dbDotEnv.getVariant()
participant SB as Supabase CLI
participant Types as Generate db types
CI->>BuildScript: build-schema
BuildScript->>Env: getVariant()
alt variant == "none"
BuildScript-->>CI: Log and exit 0 (skip DB ops)
else
BuildScript->>SB: supabase start
BuildScript->>SB: supabase migrations up
BuildScript->>Types: Generate src/dbTypes.ts
end
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/database/README.md (2)
44-48: Update steps to reflect new build-schema taskREADME still says types are generated during “turbo build”, but turbo.json moves this to “build-schema”. Please align docs.
-7. `turbo build`, which will do the following: +7. `turbo run build-schema`, which will do the following: 1. Start Supabase 2. Apply the new migration locally - 3. Regenerate the types file with `supabase gen types typescript --local > src/dbTypes.ts` - 4. Copy it where appropriate + 3. Regenerate the types file with `supabase gen types typescript --local --schema public > src/dbTypes.ts` + 4. Copy it where appropriate
70-72: Typo: duplicated word“another other terminal” → “another terminal”.
-2. In another other terminal, `cd` to this directory (`packages/database`) and run the tests with `npm run test` +2. In another terminal, `cd` to this directory (`packages/database`) and run the tests with `npm run test`packages/database/scripts/build.ts (1)
1-29: Use ESM-safe __dirname & stop Supabase in finallyImport
fileURLToPathfrom"url"and derive__filename/__dirnameviaimport.meta.url. Wrap the Supabase start/migrations sequence in atry…finally, tracking astartedflag to invokesupabase stopand avoid orphaned containers:import { fileURLToPath } from "url"; import { dirname, join } from "path"; import { execSync } from "node:child_process"; import { writeFileSync } from "fs"; import { getVariant } from "@repo/database/dbDotEnv"; const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); const projectRoot = join(__dirname, ".."); if (process.env.HOME !== "/vercel") { const variant = getVariant(); if (variant === "none") { console.log("Not using the database"); process.exit(0); } let started = false; try { execSync("supabase start", { cwd: projectRoot, stdio: "inherit" }); started = true; execSync("supabase migrations up",{ cwd: projectRoot, stdio: "inherit" }); const stdout = execSync( "supabase gen types typescript --local --schema public", { encoding: "utf8", cwd: projectRoot } ); writeFileSync( join(projectRoot, "src", "dbTypes.ts"), "// Generated by supabase -- do not edit\n" + stdout ); } finally { if (started) { try { execSync("supabase stop", { cwd: projectRoot, stdio: "inherit" }); } catch {} } } }
🧹 Nitpick comments (7)
packages/database/README.md (4)
3-9: Clarify the “no DB” default and add a quick-start noteConsider adding a one-liner like “Default: SUPABASE_USE_DB=none; nothing DB-related runs during dev/build” to make the behavior unmistakable for newcomers. Also mention how to override at runtime (e.g.,
SUPABASE_USE_DB=local turbo dev).
12-15: Grammar/style: Docker capitalization and clearer phrasing
- “within docker” → “in Docker”.
- “It does mean you will have a fresh database with minimal data.” → “This means you’ll have a fresh, minimally seeded database.”
-Normal scenario: Your backend and frontend will work against a database instance within docker. -It does mean you will have a fresh database with minimal data. +Normal scenario: Your backend and frontend will work against a database instance in Docker. +This means you’ll have a fresh, minimally seeded database.
24-26: Minor copy edit“its url” → “its URL”.
-2. You can use the Supabase studio to look over things; its url is given by the database section of `supabase start`. +2. You can use the Supabase Studio to look over things; its URL is shown in the database section of `supabase start`.
91-93: Minor copy edit“set up vercel” → “Set up Vercel”. Capitalize brand and fix verb form.
-But in most other cases, you will want your code to talk to the production database. set up vercel as above, and set `SUPABASE_USE_DB=production` in your console before running `turbo dev`. +But in most other cases, you will want your code to talk to the production database. Set up Vercel as above, and set `SUPABASE_USE_DB=production` in your console before running `turbo dev`.packages/database/src/dbDotEnv.js (1)
29-33: Minor: null check can be simplified after getVariant() changeWith CI returning
"none",variant === nullis no longer needed.- const variant = getVariant(); - if (variant === null || variant === "none") return null; + const variant = getVariant(); + if (variant === "none") return null;packages/database/package.json (2)
45-48: Optionally gate “serve” behind SUPABASE_USE_DB.If “none” is default, avoid starting Supabase by default in
serve. Keeps behavior consistent with PR intent.- "serve": "supabase start && tsx scripts/createEnv.mts && supabase functions serve", + "serve": "node -e \"const v=process.env.SUPABASE_USE_DB||'none'; if(v==='none'){console.log('SUPABASE_USE_DB=none: skipping supabase'); process.exit(0)} else process.exit(1)\" || (supabase start && tsx scripts/createEnv.mts && supabase functions serve)",Confirm this aligns with how you intend to use
servelocally/CI.
53-55: DRY build script and verify build-schema semantics.Alias
buildtocompileto avoid divergence; ensuregenenv -- localis desired when SUPABASE_USE_DB=none.- "build": "tsc", + "build": "npm run compile",If
build-schemashould no-op whenSUPABASE_USE_DB=none, mirror the guard used inserve.
📜 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 selected for processing (7)
packages/database/README.md(1 hunks)packages/database/package.json(2 hunks)packages/database/scripts/build.ts(1 hunks)packages/database/scripts/dev.ts(1 hunks)packages/database/src/dbDotEnv.d.ts(1 hunks)packages/database/src/dbDotEnv.js(1 hunks)turbo.json(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/scripts/dev.tspackages/database/scripts/build.tspackages/database/src/dbDotEnv.d.tspackages/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/dev.ts
📚 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.json
🪛 LanguageTool
packages/database/README.md
[grammar] ~12-~12: There might be a mistake here.
Context: ...ainst a database instance within docker. It does mean you will have a fresh datab...
(QB_NEW_EN)
🔇 Additional comments (3)
turbo.json (2)
66-66: Ensure build precedes deploy
Verify your CI workflows enforce the build step before runningturbo deploy, since removing the explicitdependsOncould allow deployment without a prior build.
47-51: build-schema is correctly wired
Both website and roam depend on @repo/database, so they automatically inherit itsbuild-schematask via the Turbo pipeline and have the DB types available.packages/database/scripts/dev.ts (1)
10-16: Scripts verified—no changes needed
Bothcompile(“tsc”) andserve(“supabase start && tsx scripts/createEnv.mts && supabase functions serve”) are defined inpackages/database/package.json. Theservecommand is never invoked whenSUPABASE_USE_DB=none, andcompilesimply runs the TypeScript compiler, so no DB access occurs.
https://linear.app/discourse-graphs/issue/ENG-801/compile-database-before-dev
Took on much of the scope of ENG-799
Summary by CodeRabbit