ENG-520: break-out infrastructure from ENG-373 #244
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes introduce Supabase integration across multiple project areas. Environment variables for Supabase are added to GitHub Actions, Turbo, and build processes. Supabase client creation utilities are implemented in both the UI and website packages. TypeScript path aliases are updated to simplify imports, and new Supabase dependencies are added to relevant package manifests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NextRequest
participant updateSession
participant SupabaseClient
participant SupabaseAuth
participant NextResponse
User->>NextRequest: Sends HTTP request
NextRequest->>updateSession: Passes request
updateSession->>SupabaseClient: Create client with env vars
updateSession->>SupabaseAuth: supabase.auth.getUser()
alt User not authenticated and not on login route
updateSession->>NextResponse: Redirect to login
else User authenticated or on login route
updateSession->>NextResponse: Return original response
end
sequenceDiagram
participant AppModule
participant createClient
participant SupabaseClient
AppModule->>createClient: Call with env vars
createClient->>SupabaseClient: Initialize with URL and anon key
createClient-->>AppModule: Return Supabase client instance
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 5
🔭 Outside diff range comments (2)
turbo.json (2)
30-31: Remove duplicate environment variables in dev task.
SUPABASE_URLandSUPABASE_ANON_KEYare listed twice in the dev task'spassThroughEnvarray (lines 30-31 and 37-38)."dev": { "passThroughEnv": [ "OBSIDIAN_PLUGIN_PATH", "NODE_ENV", - "SUPABASE_URL", - "SUPABASE_ANON_KEY", "SUPABASE_SERVICE_ROLE_KEY", "POSTGRES_URL", "OPENAI_API_KEY", "ANTHROPIC_API_KEY", "GEMINI_API_KEY", "SUPABASE_ANON_KEY", "SUPABASE_URL" ],Also applies to: 37-38
55-56: Remove duplicate environment variables in deploy task.Similar to the dev task,
SUPABASE_URLandSUPABASE_ANON_KEYare duplicated in the deploy task'spassThroughEnvarray."deploy": { "passThroughEnv": [ "BLOB_READ_WRITE_TOKEN", "GITHUB_REF_NAME", "GITHUB_HEAD_REF", "NODE_ENV", "SUPABASE_PROJECT_ID", "SUPABASE_DB_PASSWORD", "SUPABASE_ACCESS_TOKEN", - "SUPABASE_URL", - "SUPABASE_ANON_KEY", "SUPABASE_SERVICE_ROLE_KEY", "POSTGRES_URL", "OPENAI_API_KEY", "ANTHROPIC_API_KEY", "GEMINI_API_KEY", "SUPABASE_ANON_KEY", "SUPABASE_URL" ] }Also applies to: 62-63
🧹 Nitpick comments (5)
packages/ui/package.json (2)
26-26: Re-ordering only? Consider trimming noise in future bumps.The move of
"@repo/tailwind-config"below"@repo/eslint-config"is benign but adds churn to the diff. Next time, defer cosmetic re-ordering to a dedicated “house-keeping” PR to keep functional changes focused.
37-38: Duplicate Supabase deps across workspaces – hoist or pin identically.
@supabase/supabase-jsis now declared here while the Roam app also depends on^2.50.0. If either package drifts, the monorepo may resolve two copies and inflate bundle size (especially with ESM tree-shaking disabled).Recommendation:
- "@supabase/auth-ui-react": "0.4.7", - "@supabase/supabase-js": "^2.50.0", + "@supabase/auth-ui-react": "0.4.7", + "@supabase/supabase-js": "^2.50.0" # keep but +# TODO: add both to the repo-root package.json “resolutions” / “overrides” +# OR move them to the top-level “dependencies” to guarantee single-instance hoisting.Also consider using a caret or an exact version for both
auth-ui-reactandsupabase-jsconsistently to avoid mismatched peer ranges.apps/roam/package.json (1)
31-32: Version drift risk with Supabase packages.
@supabase/supabase-jsis declared here as^2.50.0, identical to the UI package – good.
However, Roam uses the low-level@supabase/auth-jswhereas UI uses@supabase/auth-ui-react. Confirm that both libraries will never be bundled together in the same runtime (e.g. website + iframe), otherwise expect ~80 kB extra. If duplication is unavoidable, centralise them at the repo root to guarantee a single copy.apps/website/app/utils/supabase/client.ts (1)
1-1: Consider consistent quote usage.Mixed quote styles are used across files (single quotes here vs double quotes in other files). Consider standardizing to maintain consistency.
apps/website/app/utils/supabase/middleware.ts (1)
40-49: Consider more flexible redirect configuration.The hardcoded redirect paths and login URL could benefit from configuration to make the middleware more reusable.
Consider extracting the configuration:
+const PROTECTED_PATHS_CONFIG = { + excludePatterns: ['/login', '/auth'], + loginPath: '/auth/login' +}; + if ( !user && - !request.nextUrl.pathname.startsWith('/login') && - !request.nextUrl.pathname.startsWith('/auth') + !PROTECTED_PATHS_CONFIG.excludePatterns.some(pattern => + request.nextUrl.pathname.startsWith(pattern) + ) ) { const url = request.nextUrl.clone() - url.pathname = '/auth/login' + url.pathname = PROTECTED_PATHS_CONFIG.loginPath return NextResponse.redirect(url) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.github/workflows/roam-pr.yaml(1 hunks)apps/roam/package.json(1 hunks)apps/roam/scripts/compile.ts(1 hunks)apps/roam/tsconfig.json(1 hunks)apps/website/app/utils/supabase/client.ts(1 hunks)apps/website/app/utils/supabase/middleware.ts(1 hunks)apps/website/tsconfig.json(1 hunks)packages/ui/package.json(2 hunks)packages/ui/src/lib/supabase/client.ts(1 hunks)turbo.json(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/website/app/utils/supabase/client.ts (2)
packages/ui/src/lib/supabase/client.ts (1)
createClient(7-12)packages/database/types.gen.ts (1)
Database(9-814)
🔇 Additional comments (4)
apps/roam/tsconfig.json (1)
15-18: Path alias looks good – watch for overlap with upstream config.The new
~/*and@repo/*aliases resolve correctly fromapps/roam, but make sure they don't shadow identical aliases already provided by@repo/typescript-config/react-library.json. If duplication exists the compiler will silently ignore one of them.packages/ui/src/lib/supabase/client.ts (2)
4-5: Verify global constants are properly injected.The global constants
SUPABASE_URLandSUPABASE_ANON_KEYare declared but their injection relies on the build configuration inapps/roam/scripts/compile.ts. Ensure the build process properly defines these constants to avoid runtime errors.
7-12: LGTM! Strongly typed Supabase client creation.The function correctly creates a strongly typed Supabase client with proper generic parameters. The typing ensures type safety for database operations.
apps/website/app/utils/supabase/middleware.ts (1)
4-65: LGTM! Well-implemented Supabase SSR middleware.The middleware correctly implements Supabase session management for Next.js SSR with proper cookie handling and authentication flow. The detailed comments are particularly helpful for understanding the critical aspects of the implementation.
maparent
force-pushed
the
ENG-520-supabase-in-frontend
branch
from
bb62f2a to
9cf8334
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
There are many branches that assume access to supabase infrastructure in the front-end
Could you give an example (link the file or even directly to the code as GitHub will highlight it or embed it) EG:
highlight from another PR
#238 (comment)
embedded
discourse-graph/.github/workflows/roam-pr.yaml
Lines 20 to 21 in 9cf8334
... ui work in the ENG-373 branch, so other branches can benefit from this infrastructure work, notably future ENG-420 work ...
Note: you can use the # in GitHub to link to other branches. This makes it easier for anyone else viewing this PR to navigate to what you are referencing. EG: #238
maparent
force-pushed
the
ENG-520-supabase-in-frontend
branch
from
9cf8334 to
6cfbec9
Compare
|
On PR references: I find using the PR numbers confusing, as I think of the task in terms of linear task numbers. I wish there were a good way to make short linear references in github. But yeah, I also see the value of hyperlinking of course. Just pointing out a cognitive overhead. |
maparent
force-pushed
the
ENG-520-supabase-in-frontend
branch
from
6cfbec9 to
93b7c94
Compare
|
sorry, went too fast in re-request, one aspect is missing: Will it work in obsidian? I don't know, and I don't know that part of the codebase enough to test it. But I checked that, as long as the variables as defined (my warning at the top of the PR), obsidian builds correctly. As for calling it, I see no reason why not, but we may need to add environment passthrough here and there. I think we can wait until we actually call it to check that. |
Because we are adding the linear id's in the PR titles, |
Ah, I was asking because I thought it was added for Obsidian:
|
| ) { | ||
| // no user, potentially respond by redirecting the user to the login page | ||
| const url = request.nextUrl.clone(); | ||
| url.pathname = "/auth/login"; |
|
|
||
| // Inspired by https://supabase.com/ui/docs/nextjs/password-based-auth | ||
|
|
||
| export const updateSession = async (request: NextRequest) => { |
There was a problem hiding this comment.
It's not used yet; I was following the documented pattern in the url above.
There was a problem hiding this comment.
I briefly scanned the URL but didn't explicitly see it's use, only the code. Do you have a sense of where we will use it in the future?
| { | ||
| "extends": "@repo/typescript-config/nextjs.json", | ||
| "compilerOptions": { | ||
| "baseUrl": ".", |
There was a problem hiding this comment.
What is the purpose for adding this? Or problem is this solving?
There was a problem hiding this comment.
That is fine, it is just helpful to add the context.
There are many branches (notably #238, which may be broken in three before merge; and an upcoming branch for ENG-420 which I need to base on both this one and #245) that assume access to supabase infrastructure in the front-end; I just wanted to isolate the infrastructure work from the ui work in the ENG-373 branch, so those other branches build on it.
An unfortunate consequence of this is that turbo now expects
SUPABASE_URLandSUPABASE_ANON_KEYto be defined. We will all need to update our.envaccordingly. This should be fine with vercel and github actions.Note: There are a few non-obvious choices, such as putting the client code in the ui, which I think will benefit obsidian in the longer term.
Summary by CodeRabbit
New Features
Chores