refactor(web): rename withAuthV2 to withAuth#1073
Merged
brendan-kellam merged 4 commits intomainfrom Apr 2, 2026
Merged
Conversation
Renames withAuthV2/withOptionalAuthV2 to withAuth/withOptionalAuth and relocates them from src/withAuthV2.ts to src/middleware/withAuth.ts. Extracts withMinimumOrgRole and sew into their own files under middleware/. Fixes 'use server' build error by removing logger export from actions.ts and fixing mock path in withAuth.test.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@brendan-kellam your pull request is missing a changelog! |
Contributor
WalkthroughRenames and relocates auth middleware across the web package: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
brendan-kellam
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/ee/features/oauth/actions.ts (1)
47-47:⚠️ Potential issue | 🟡 MinorUse braces for single-line
ifbodies in these handlers.Both
if (state)statements should use block bodies to match repo standards.Suggested patch
- if (state) callbackUrl.searchParams.set('state', state); + if (state) { + callbackUrl.searchParams.set('state', state); + } ... - if (state) callbackUrl.searchParams.set('state', state); + if (state) { + callbackUrl.searchParams.set('state', state); + }As per coding guidelines, “Always use curly braces for
ifstatements, with the body on a new line — even for single-line bodies.”Also applies to: 66-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/ee/features/oauth/actions.ts` at line 47, Two single-line if statements that set the state on callbackUrl (e.g. "if (state) callbackUrl.searchParams.set('state', state);") must use braces per repo style; update both occurrences (the one using callbackUrl.searchParams.set(...) and the similar one at the other occurrence) to use block bodies with the body on a new line, e.g. add { ... } around the callbackUrl.searchParams.set call so the if uses a curly-braced block.
🧹 Nitpick comments (3)
packages/web/src/middleware/withAuth.test.ts (2)
464-464: Update describe block name to match renamed function.The describe block still references
withAuthV2but the tests now use the renamedwithAuthfunction. This inconsistency could cause confusion when reading test output.📝 Suggested fix
-describe('withAuthV2', () => { +describe('withAuth', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/middleware/withAuth.test.ts` at line 464, Update the test suite's describe block name to match the renamed function: change the describe string from "withAuthV2" to "withAuth" so it reflects the actual function under test (withAuth) used in the tests; locate the describe(...) containing 'withAuthV2' and rename it to 'withAuth' to keep test output and intent consistent.
731-731: Update describe block name to match renamed function.Same issue here — the describe block references
withOptionalAuthV2but tests now usewithOptionalAuth.📝 Suggested fix
-describe('withOptionalAuthV2', () => { +describe('withOptionalAuth', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/middleware/withAuth.test.ts` at line 731, Rename the test suite description to match the actual exported function name: change the describe block string from "withOptionalAuthV2" to "withOptionalAuth" so the describe block aligns with the tests calling withOptionalAuth; update any other describe blocks in this file that still reference withOptionalAuthV2 to withOptionalAuth to keep names consistent with the function under test.packages/web/src/middleware/withMinimumOrgRole.ts (1)
12-23: Consider using exhaustive Record mapping for OrgRole precedence.
getAuthorizationPrecedenceuses a switch without a default case. While TypeScript's strict mode (noImplicitReturns enabled) will catch missing returns if OrgRole is expanded, an exhaustiveRecord<OrgRole, number>mapping would be more explicit and maintainable:const precedence: Record<OrgRole, number> = { [OrgRole.GUEST]: 0, [OrgRole.MEMBER]: 1, [OrgRole.OWNER]: 2, };This pattern makes it impossible to forget an enum value and is easier to read.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/middleware/withMinimumOrgRole.ts` around lines 12 - 23, Replace the switch-based getAuthorizationPrecedence with an explicit exhaustive Record mapping for OrgRole (e.g., create a const precedence: Record<OrgRole, number> = { [OrgRole.GUEST]: 0, [OrgRole.MEMBER]: 1, [OrgRole.OWNER]: 2 }) and then use precedence[userRole] and precedence[minRequiredRole] in the comparison; update any reference to getAuthorizationPrecedence to read from the precedence map (or remove the function entirely) so all OrgRole values are statically covered and cannot be missed when the enum grows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/ee/features/audit/actions.ts`:
- Around line 37-38: The fetchAuditRecords action is using the global Prisma
client instead of the auth context's prisma, which bypasses user-scoped
permission enforcement; update the implementation inside
withAuth/withMinimumOrgRole to use the prisma instance passed in the auth
context (e.g., use the prisma parameter available in the withAuth callback) for
all DB calls (replace any direct uses/imports of '@/prisma' with the
context.prisma or prisma argument) so that methods like fetchAuditRecords call
prisma.audit.findMany (and related queries) through the context-provided client
with userScopedPrismaClientExtension applied.
In `@packages/web/src/ee/features/userManagement/actions.ts`:
- Around line 23-24: The actions wrapped with withAuth and withMinimumOrgRole
are currently using the global Prisma client; update both server actions to
destructure prisma from the withAuth context parameter (e.g., change the async
({ user, org, role }) => to async ({ user, org, role, prisma }) =>) and then
replace any use of the global prisma with that context-scoped prisma so the
userScopedPrismaClientExtension is applied (apply this change for the two
occurrences wrapped by withAuth/withMinimumOrgRole).
---
Outside diff comments:
In `@packages/web/src/ee/features/oauth/actions.ts`:
- Line 47: Two single-line if statements that set the state on callbackUrl (e.g.
"if (state) callbackUrl.searchParams.set('state', state);") must use braces per
repo style; update both occurrences (the one using
callbackUrl.searchParams.set(...) and the similar one at the other occurrence)
to use block bodies with the body on a new line, e.g. add { ... } around the
callbackUrl.searchParams.set call so the if uses a curly-braced block.
---
Nitpick comments:
In `@packages/web/src/middleware/withAuth.test.ts`:
- Line 464: Update the test suite's describe block name to match the renamed
function: change the describe string from "withAuthV2" to "withAuth" so it
reflects the actual function under test (withAuth) used in the tests; locate the
describe(...) containing 'withAuthV2' and rename it to 'withAuth' to keep test
output and intent consistent.
- Line 731: Rename the test suite description to match the actual exported
function name: change the describe block string from "withOptionalAuthV2" to
"withOptionalAuth" so the describe block aligns with the tests calling
withOptionalAuth; update any other describe blocks in this file that still
reference withOptionalAuthV2 to withOptionalAuth to keep names consistent with
the function under test.
In `@packages/web/src/middleware/withMinimumOrgRole.ts`:
- Around line 12-23: Replace the switch-based getAuthorizationPrecedence with an
explicit exhaustive Record mapping for OrgRole (e.g., create a const precedence:
Record<OrgRole, number> = { [OrgRole.GUEST]: 0, [OrgRole.MEMBER]: 1,
[OrgRole.OWNER]: 2 }) and then use precedence[userRole] and
precedence[minRequiredRole] in the comparison; update any reference to
getAuthorizationPrecedence to read from the precedence map (or remove the
function entirely) so all OrgRole values are statically covered and cannot be
missed when the enum grows.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b20d8e50-3443-486b-b18e-ebc655380621
📒 Files selected for processing (40)
packages/web/src/actions.tspackages/web/src/app/[domain]/askgh/[owner]/[repo]/api.tspackages/web/src/app/[domain]/repos/[id]/page.tsxpackages/web/src/app/[domain]/repos/page.tsxpackages/web/src/app/[domain]/settings/connections/[id]/page.tsxpackages/web/src/app/[domain]/settings/connections/page.tsxpackages/web/src/app/[domain]/settings/layout.tsxpackages/web/src/app/api/(server)/chat/route.tspackages/web/src/app/api/(server)/ee/accountPermissionSyncJobStatus/api.tspackages/web/src/app/api/(server)/ee/chat/[chatId]/searchMembers/route.tspackages/web/src/app/api/(server)/ee/permissionSyncStatus/api.tspackages/web/src/app/api/(server)/ee/user/route.tspackages/web/src/app/api/(server)/ee/users/route.tspackages/web/src/app/api/(server)/mcp/route.tspackages/web/src/app/api/(server)/models/route.tspackages/web/src/app/api/(server)/repos/listReposApi.tspackages/web/src/app/invite/actions.tspackages/web/src/ee/features/analytics/actions.tspackages/web/src/ee/features/audit/actions.tspackages/web/src/ee/features/oauth/actions.tspackages/web/src/ee/features/sso/actions.tspackages/web/src/ee/features/userManagement/actions.tspackages/web/src/features/chat/actions.tspackages/web/src/features/codeNav/api.tspackages/web/src/features/git/getDiffApi.tspackages/web/src/features/git/getFileSourceApi.tspackages/web/src/features/git/getFilesApi.tspackages/web/src/features/git/getFolderContentsApi.tspackages/web/src/features/git/getTreeApi.tspackages/web/src/features/git/listCommitsApi.tspackages/web/src/features/mcp/askCodebase.tspackages/web/src/features/search/searchApi.tspackages/web/src/features/searchAssist/actions.tspackages/web/src/features/userManagement/actions.tspackages/web/src/features/workerApi/actions.tspackages/web/src/lib/posthog.tspackages/web/src/middleware/sew.tspackages/web/src/middleware/withAuth.test.tspackages/web/src/middleware/withAuth.tspackages/web/src/middleware/withMinimumOrgRole.ts
Update vi.mock paths from @/withAuthV2 to @/middleware/withAuth and from @/actions to @/middleware/sew to match the renamed modules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nd userManagement actions Replace direct @/prisma imports with the prisma instance from the withAuth callback to ensure userScopedPrismaClientExtension is applied. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/features/git/listCommitsApi.test.ts (1)
35-44:⚠️ Potential issue | 🟠 MajorMatch the
sewmock to production error semantics.On Line 35, the mock returns
Promise<T>and currently forwards thrown error messages; productionsewreturnsT | ServiceErrorand uses a fixed generic message for unexpected failures. This mismatch can make error-path tests pass/fail for the wrong reasons.Proposed fix
vi.mock('@/middleware/sew', () => ({ - sew: async <T>(fn: () => Promise<T> | T): Promise<T> => { + sew: async <T>(fn: () => Promise<T> | T): Promise<T | { errorCode: string; message: string }> => { try { return await fn(); } catch (error) { - // Mock sew to convert thrown errors to ServiceError + // Match production sew behavior for unexpected failures return { errorCode: 'UNEXPECTED_ERROR', - message: error instanceof Error ? error.message : String(error), - } as T; + message: 'An unexpected error occurred. Please try again later.', + }; } }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/git/listCommitsApi.test.ts` around lines 35 - 44, The mock implementation of sew in listCommitsApi.test.ts wrongly types/behaves like Promise<T> and forwards thrown error messages; update the mock for sew to return Promise<T | ServiceError> (or T | ServiceError) and, when catching, return a ServiceError object with the fixed generic message used in production (e.g. message set to a generic unexpected-failure string and errorCode 'UNEXPECTED_ERROR') instead of using error.message so test error-path semantics match the real sew; locate and update the sew async mock in the test to change its return type and caught-error behavior accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/features/git/listCommitsApi.test.ts`:
- Around line 51-53: The mock for withOptionalAuth in listCommitsApi.test.ts is
incomplete and the comment references the wrong helper; update the mock callback
shape to match the real withOptionalAuth by passing an object like { user:
undefined, org: mockOrg, role: 'member' (or appropriate role), prisma:
mockPrisma } instead of just { org, prisma }, and change the comment from "Mock
withOptionalAuthV2" to "Mock withOptionalAuth" so the test context and comment
reflect the actual function signature (refer to withOptionalAuth and mockOrg in
the test).
---
Outside diff comments:
In `@packages/web/src/features/git/listCommitsApi.test.ts`:
- Around line 35-44: The mock implementation of sew in listCommitsApi.test.ts
wrongly types/behaves like Promise<T> and forwards thrown error messages; update
the mock for sew to return Promise<T | ServiceError> (or T | ServiceError) and,
when catching, return a ServiceError object with the fixed generic message used
in production (e.g. message set to a generic unexpected-failure string and
errorCode 'UNEXPECTED_ERROR') instead of using error.message so test error-path
semantics match the real sew; locate and update the sew async mock in the test
to change its return type and caught-error behavior accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a49646b0-5bba-449c-bd16-1cb05c746b07
📒 Files selected for processing (1)
packages/web/src/features/git/listCommitsApi.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/web/src/features/git/listCommitsApi.test.ts (1)
51-59:⚠️ Potential issue | 🟡 MinorAlign
withOptionalAuthmock args with real middleware context.On Line 51 and Line 59, the mock only types/passes
{ org, prisma }, but realwithOptionalAuthpasses{ user, org, role, prisma }(packages/web/src/middleware/withAuth.ts, Line 87). This can mask auth-context bugs in tests.Proposed fix
vi.mock('@/middleware/withAuth', () => ({ - withOptionalAuth: async <T>(fn: (args: { org: { id: number; name: string }; prisma: unknown }) => Promise<T>): Promise<T> => { - // Mock withOptionalAuth to provide org and prisma context + withOptionalAuth: async <T>(fn: (args: { + user?: unknown; + org: { id: number; name: string }; + role: string; + prisma: unknown; + }) => Promise<T>): Promise<T> => { + // Mock withOptionalAuth to provide auth context const mockOrg = { id: 1, name: 'test-org' }; const mockPrisma = { repo: { findFirst: mockFindFirst, }, }; - return await fn({ org: mockOrg, prisma: mockPrisma }); + return await fn({ user: undefined, org: mockOrg, role: 'MEMBER', prisma: mockPrisma }); }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/git/listCommitsApi.test.ts` around lines 51 - 59, The withOptionalAuth mock used in the test only supplies { org, prisma } but the real middleware calls handlers with { user, org, role, prisma } (see withOptionalAuth in withAuth.ts); update the test's mock implementation of withOptionalAuth to include and type the missing properties (provide a mockUser and mockRole alongside mockOrg and mockPrisma) and pass { user: mockUser, org: mockOrg, role: mockRole, prisma: mockPrisma } into fn so the test accurately reflects runtime auth context and catches auth-related issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/web/src/features/git/listCommitsApi.test.ts`:
- Around line 51-59: The withOptionalAuth mock used in the test only supplies {
org, prisma } but the real middleware calls handlers with { user, org, role,
prisma } (see withOptionalAuth in withAuth.ts); update the test's mock
implementation of withOptionalAuth to include and type the missing properties
(provide a mockUser and mockRole alongside mockOrg and mockPrisma) and pass {
user: mockUser, org: mockOrg, role: mockRole, prisma: mockPrisma } into fn so
the test accurately reflects runtime auth context and catches auth-related
issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68a527b0-b527-48f9-9faf-7076cb4bdf1b
📒 Files selected for processing (4)
CLAUDE.mdpackages/web/src/features/git/listCommitsApi.test.tspackages/web/src/middleware/withAuth.test.tspackages/web/tools/globToRegexpPlayground.ts
✅ Files skipped from review due to trivial changes (3)
- CLAUDE.md
- packages/web/tools/globToRegexpPlayground.ts
- packages/web/src/middleware/withAuth.test.ts
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.
Summary
withAuthV2/withOptionalAuthV2towithAuth/withOptionalAuthand moves fromsrc/withAuthV2.tstosrc/middleware/withAuth.tswithMinimumOrgRoleandsewinto their own files undersrc/middleware/'use server'build error caused by exportinglogger(a non-async-function) fromactions.ts, and fixes mock path inwithAuth.test.tsTest plan
withAuth.test.tspasses (45 tests)yarn workspace @sourcebot/web build)🤖 Generated with Claude Code
Summary by CodeRabbit