fix: stop firing protected tRPC queries on auth-optional surfaces#3109
Open
Abdul-Moiz31 wants to merge 1 commit into
Open
fix: stop firing protected tRPC queries on auth-optional surfaces#3109Abdul-Moiz31 wants to merge 1 commit into
Abdul-Moiz31 wants to merge 1 commit into
Conversation
|
@Abdul-Moiz31 is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe PR resolves UNAUTHORIZED errors thrown by components that should support both authenticated and unauthenticated users. It introduces an optional auth infrastructure, two optional backend endpoints, and updates four frontend components to use them. ChangesOptional Auth Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (1)
apps/web/client/src/server/api/trpc.ts (1)
127-135: 💤 Low value
optionalAuthProcedureis structurally identical topublicProcedure.Both are defined as
t.procedure.use(timingMiddleware)and, after the context change above, both expose the samectx.user: User | nulltyping. The only thing distinguishing them today is the JSDoc — at the type level and at runtime they are interchangeable. This is fine as a semantic marker for callers, but it means a future refactor of one easily diverges from the other, and reviewers can't tell from a callsite which contract was intended.Consider one of:
- Drop
optionalAuthProcedureand usepublicProcedurefor these endpoints (it already documents "you can still access user session data if they are logged in").- Keep both but differentiate with a tiny marker middleware so the intent is enforced (e.g., a no-op middleware named
optionalAuthMiddlewarethat future-proofs adding logging/metrics distinct from truly public endpoints).♻️ Option B sketch — marker middleware to keep the two procedures distinct
+const optionalAuthMiddleware = t.middleware(async ({ next, ctx }) => { + // Marker middleware: this procedure may be called by anonymous users. + // Endpoints are expected to handle `ctx.user === null` explicitly. + return next({ ctx }); +}); + /** * Optional auth procedure * * Use this for endpoints that surface on both authenticated and anonymous pages * (e.g. marketing, pricing). `ctx.user` is `User | null` — endpoints must handle * both cases and typically return `null` for anonymous callers instead of throwing. */ -export const optionalAuthProcedure = t.procedure.use(timingMiddleware); +export const optionalAuthProcedure = t.procedure + .use(timingMiddleware) + .use(optionalAuthMiddleware);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/client/src/server/api/trpc.ts` around lines 127 - 135, The two procedures optionalAuthProcedure and publicProcedure are identical (both t.procedure.use(timingMiddleware)), so either remove optionalAuthProcedure and update callers to use publicProcedure, or keep it but add a tiny no-op middleware (e.g., optionalAuthMiddleware) and apply it so optionalAuthProcedure = t.procedure.use(optionalAuthMiddleware).use(timingMiddleware); implement optionalAuthMiddleware as a named pass-through middleware to preserve runtime/type distinction and document its intent; update imports/callsites accordingly (search for optionalAuthProcedure and publicProcedure to change or keep consistent).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/client/src/server/api/routers/subscription/subscription.ts`:
- Line 7: The import currently pulling createTRPCRouter, optionalAuthProcedure,
and protectedProcedure from a relative path ('../../trpc') should be switched to
the project's configured path alias (use the `@/`* or ~/* alias form) so the
module is imported via the src alias instead of a relative path; update the
import statement to use the alias (keeping the same named imports
createTRPCRouter, optionalAuthProcedure, protectedProcedure) so the code
resolves through the project's path-mapping.
In `@apps/web/client/src/server/api/routers/user/user.ts`:
- Line 8: Replace the relative import for the tRPC helpers with the configured
path alias: update the import that currently pulls createTRPCRouter,
optionalAuthProcedure, and protectedProcedure from '../../trpc' to use the alias
that maps to src (e.g. '@/server/api/trpc') so the symbols createTRPCRouter,
optionalAuthProcedure, and protectedProcedure are imported via the path-alias
import instead of a relative path.
---
Nitpick comments:
In `@apps/web/client/src/server/api/trpc.ts`:
- Around line 127-135: The two procedures optionalAuthProcedure and
publicProcedure are identical (both t.procedure.use(timingMiddleware)), so
either remove optionalAuthProcedure and update callers to use publicProcedure,
or keep it but add a tiny no-op middleware (e.g., optionalAuthMiddleware) and
apply it so optionalAuthProcedure =
t.procedure.use(optionalAuthMiddleware).use(timingMiddleware); implement
optionalAuthMiddleware as a named pass-through middleware to preserve
runtime/type distinction and document its intent; update imports/callsites
accordingly (search for optionalAuthProcedure and publicProcedure to change or
keep consistent).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5af471f-2598-499b-820a-010608820b63
📒 Files selected for processing (7)
apps/web/client/src/app/_components/top-bar/user.tsxapps/web/client/src/components/telemetry-provider.tsxapps/web/client/src/components/ui/pricing-modal/use-subscription.tsxapps/web/client/src/components/ui/pricing-table/index.tsxapps/web/client/src/server/api/routers/subscription/subscription.tsapps/web/client/src/server/api/routers/user/user.tsapps/web/client/src/server/api/trpc.ts
| import { headers } from 'next/headers'; | ||
| import { z } from 'zod'; | ||
| import { createTRPCRouter, protectedProcedure } from '../../trpc'; | ||
| import { createTRPCRouter, optionalAuthProcedure, protectedProcedure } from '../../trpc'; |
There was a problem hiding this comment.
Switch this import to a configured path alias
Line 7 uses ../../trpc; this should use the project alias form in src code.
As per coding guidelines: apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/client/src/server/api/routers/subscription/subscription.ts` at line
7, The import currently pulling createTRPCRouter, optionalAuthProcedure, and
protectedProcedure from a relative path ('../../trpc') should be switched to the
project's configured path alias (use the `@/`* or ~/* alias form) so the module is
imported via the src alias instead of a relative path; update the import
statement to use the alias (keeping the same named imports createTRPCRouter,
optionalAuthProcedure, protectedProcedure) so the code resolves through the
project's path-mapping.
| import { eq } from 'drizzle-orm'; | ||
| import { z } from 'zod'; | ||
| import { createTRPCRouter, protectedProcedure } from '../../trpc'; | ||
| import { createTRPCRouter, optionalAuthProcedure, protectedProcedure } from '../../trpc'; |
There was a problem hiding this comment.
Use path alias import for tRPC module
Line 8 uses a relative import (../../trpc) in a src file. Please switch this to the configured alias import for consistency and guideline compliance.
As per coding guidelines: apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/client/src/server/api/routers/user/user.ts` at line 8, Replace the
relative import for the tRPC helpers with the configured path alias: update the
import that currently pulls createTRPCRouter, optionalAuthProcedure, and
protectedProcedure from '../../trpc' to use the alias that maps to src (e.g.
'@/server/api/trpc') so the symbols createTRPCRouter, optionalAuthProcedure, and
protectedProcedure are imported via the path-alias import instead of a relative
path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Components that surface on both authenticated and anonymous pages — the telemetry provider (wraps every page), the top bar "Sign In" button (on marketing pages), and the pricing table — call
user.getandsubscription.geton every render. Both endpoints areprotectedProcedureand throwUNAUTHORIZEDfor anonymous visitors. React Query then retries 3× and refetches on window focus, producing a console flood of failed queries on every marketing-page load.This PR introduces
optionalAuthProcedureand parallelgetOptionalendpoints that returnnullfor anonymous callers instead of throwing.Related Issues
closes #3051
Type of Change
Approach
The issue body proposes adding a separate procedure (e.g.
optionalAuthProcedure) and parallel endpoints rather than modifyingprotectedProcedureto allow nullctx.user. The latter would require adding null checks to every existing protected endpoint (~30 of them) and weaken types throughout. This PR follows the issue's recommendation.Changes
createTRPCContext— treatAuthSessionMissingError(Supabase's "no session" signal) asctx.user = nullinstead of throwing. Other errors (malformed JWT, network failures) still surface asUNAUTHORIZED.protectedProcedurestill throws downstream, so no protected endpoint becomes accessible to anonymous users.optionalAuthProcedure— same shape asprotectedProcedurebut does not throw whenctx.useris null. Endpoints opt in explicitly.user.getOptionalandsubscription.getOptional— same return shape as theirgetcounterparts, but returnnull(instead of throwing) when there is no authenticated user.apps/web/client/src/components/telemetry-provider.tsxapps/web/client/src/app/_components/top-bar/user.tsxapps/web/client/src/components/ui/pricing-table/index.tsxapps/web/client/src/components/ui/pricing-modal/use-subscription.tsx(shared hook used byFreeCard/ProCard— the indirect path by whichsubscription.getreaches the public pricing page)All four callsites already branched on
user ?? null/subscription ?? nullin their render logic, so consumer behaviour is unchanged when data exists.What is NOT changed
user.get/subscription.getremainprotectedProcedure. The ~15 other callsites in authenticated routes (/project/[id]/...,/projects/...) continue to use them, where throwing on missing auth is the correct behaviour.protectedProcedureis untouched.Testing
Reproduction on
main(logged out, incognito):bun dev, openhttp://localhost:3000in an Incognito window.user.get,subscription.get, each retried 3× and refetched on focus.With this PR (logged out, incognito):
user.getOptional,subscription.getOptional) returnnull.With this PR (logged in, regression check):
user.getfrom project pages,project.get, etc.) continue to work.user.getOptional/subscription.getOptionalreturn real data.Local checks:
bun typecheck✓bun test✓ (1045/1045)Files Changed
apps/web/client/src/server/api/trpc.ts— context fix +optionalAuthProcedureapps/web/client/src/server/api/routers/user/user.ts—getOptionalapps/web/client/src/server/api/routers/subscription/subscription.ts—getOptionalapps/web/client/src/components/telemetry-provider.tsx— usegetOptionalapps/web/client/src/app/_components/top-bar/user.tsx— usegetOptionalapps/web/client/src/components/ui/pricing-table/index.tsx— usegetOptionalapps/web/client/src/components/ui/pricing-modal/use-subscription.tsx— usegetOptionalSummary by CodeRabbit