-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-850 Fail gracefully when no database is available #438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENG-850 Fail gracefully when no database is available #438
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
3aa88f6 to
749679f
Compare
|
@CodeRabbit sorry force-pushed a new version, restart the review |
|
@maparent I'll restart the review for the updated PR after your force-push. ✅ Actions performedFull review triggered. |
mdroidian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this PR contains some of the same changes as #431
(turbo.json, package.json, etc)
| "process.env.SUPABASE_ANON_KEY": `"${dbEnv.SUPABASE_ANON_KEY}"`, | ||
| "process.env.SUPABASE_URL": dbEnv.SUPABASE_URL | ||
| ? `"${dbEnv.SUPABASE_URL}"` | ||
| : "null", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this won't trigger (!url | !key) in client.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it does; realize that the "null" string will be bare once substituted.
|
Caution Review failedFailed to post review comments. Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
🧰 Additional context used🧠 Learnings (10)📚 Learning: 2025-08-27T02:23:45.696ZApplied to files:
📚 Learning: 2025-07-19T22:34:08.725ZApplied to files:
📚 Learning: 2025-07-19T22:34:08.725ZApplied to files:
📚 Learning: 2025-08-31T17:37:17.173ZApplied to files:
📚 Learning: 2025-07-08T14:51:55.299ZApplied to files:
📚 Learning: 2025-07-22T01:50:20.253ZApplied to files:
📚 Learning: 2025-05-22T23:48:45.450ZApplied to files:
📚 Learning: 2025-06-25T18:03:52.669ZApplied to files:
📚 Learning: 2025-07-13T16:47:14.352ZApplied to files:
📚 Learning: 2025-07-08T14:48:38.048ZApplied to files:
🪛 GitHub Actions: PR - Roam To Blob Storagepackages/database/src/lib/client.ts[error] 1-1: Build failed: 'npm run build' exited with code 2. packages/database/scripts/createEnv.mts[error] 1-1: Build failed: 'npm run build' exited with code 2. packages/database/package.json[error] 1-1: Build failed: 'npm run build' exited with code 2. 📝 WalkthroughWalkthroughThe PR introduces nullable Supabase client handling across Roam utilities, adjusts Roam compile-time env defines to allow nulls, refactors database env-generation workflow and Turbo tasks, and shifts multiple packages’ exports to reference TypeScript sources. It also changes the database client factory to return null instead of throwing when env vars are missing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Roam App
participant Ctx as supabaseContext
participant DB as database/lib/client
User->>App: Trigger operation (search/sync)
App->>Ctx: getLoggedInClient()
Ctx->>DB: createClient()
alt Env vars present
DB-->>Ctx: DGSupabaseClient
else Missing env vars
DB-->>Ctx: null
end
alt Client available
Ctx-->>App: client
App->>Ctx: getSupabaseContext(client)
alt Context available
Ctx-->>App: context
App->>DB: RPC / operations
DB-->>App: result
else Context null
Ctx-->>App: null
App-->>User: Early return (no-op/empty result)
end
else No client
Ctx-->>App: null
App-->>User: Early return (no-op/empty result)
end
sequenceDiagram
autonumber
participant Turbo as Turbo genenv
participant Env as packages/database/scripts/createEnv.mts
participant Vercel as Vercel API
Turbo->>Env: run (variant from CLI/SUPABASE_USE_DB)
Env->>Env: Detect CI/Vercel, resolve Variant
alt CI/Vercel or Variant none
Env-->>Turbo: Exit early
else Local/Branch/Production
Env->>Env: getVercelToken()
Env->>Vercel: Initialize client
alt Variant branch or all
Env->>Vercel: Query branch deployments
Vercel-->>Env: Preview env vars
Env->>Env: Write .env.branch
end
alt Variant production or all
Env->>Vercel: Get latest production deployment
Vercel-->>Env: Production env vars
Env->>Env: Write .env.production
end
Env-->>Turbo: Done (.env*)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
|
Caution Review failedFailed to post review comments. Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
🧰 Additional context used🧠 Learnings (10)📓 Common learnings📚 Learning: 2025-08-27T02:23:45.696ZApplied to files:
📚 Learning: 2025-07-19T22:34:08.725ZApplied to files:
📚 Learning: 2025-07-19T22:34:08.725ZApplied to files:
📚 Learning: 2025-07-08T14:48:38.048ZApplied to files:
📚 Learning: 2025-08-31T17:37:17.173ZApplied to files:
📚 Learning: 2025-07-08T14:51:55.299ZApplied to files:
📚 Learning: 2025-06-25T18:03:52.669ZApplied to files:
📚 Learning: 2025-07-13T16:47:14.352ZApplied to files:
📚 Learning: 2025-07-22T01:50:20.253ZApplied to files:
📝 WalkthroughWalkthroughUpdates introduce nullable Supabase client handling across Roam and database libs, add early-return guards in callers, and adjust esbuild defines to inject null for missing Supabase env vars. Database build/dev tooling is reworked: env generation script becomes a structured CLI, Turbo tasks add a genenv pipeline, and package exports shift from dist outputs to TS sources in multiple packages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant RoamCaller as Roam Caller (hyde/sync)
participant SupaCtx as getLoggedInClient
participant DBClient as database/lib/createClient
participant Supabase as Supabase API
User->>RoamCaller: invoke feature
RoamCaller->>SupaCtx: getLoggedInClient()
SupaCtx->>DBClient: createClient()
alt Env OK
DBClient-->>SupaCtx: client
SupaCtx-->>RoamCaller: client
RoamCaller->>Supabase: proceed with RPC/queries
Supabase-->>RoamCaller: result
else Missing env / no session
DBClient-->>SupaCtx: null
SupaCtx-->>RoamCaller: null
RoamCaller-->>User: early return ([], no-op)
end
note over RoamCaller,SupaCtx: Exceptions replaced with nullable returns and guards
sequenceDiagram
autonumber
actor Dev
participant Turbo as Turbo(genenv/build/dev)
participant CreateEnv as packages/database/scripts/createEnv.mts
participant Vercel as Vercel API
Dev->>Turbo: run build/dev
Turbo->>CreateEnv: genenv (variant from CLI/env)
alt Vercel/GitHub CI
CreateEnv-->>Turbo: skip (early return)
else local/branch/prod
CreateEnv->>CreateEnv: getVercelToken() (lazy)
opt branch/prod/all
CreateEnv->>Vercel: env pull (token)
Vercel-->>CreateEnv: .env files
end
opt local/all
CreateEnv->>CreateEnv: makeLocalEnv
end
CreateEnv-->>Turbo: .env outputs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
…ndling in roam compile
749679f to
4576257
Compare
…ndling in roam compile (#438)
https://linear.app/discourse-graphs/issue/ENG-850/fail-gracefully-when-no-database-is-available
Summary by CodeRabbit
Bug Fixes
Chores