-
-
Notifications
You must be signed in to change notification settings - Fork 837
fix(webapp): org invite scoping #2554
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
Conversation
Fixes some scoping issues with team invites.
|
WalkthroughInvite-related server methods were updated to enforce user-scoped operations and include email where needed: acceptInvite and declineInvite now accept a user object ({id, email}); resendInvite now requires an additional userId; revokeInvite now uses orgSlug and verifies membership by organization slug. invite-accept loader now requires authentication before token fetch. Several route modules changed to pass the new parameters (user or userId/orgSlug) and switched some imports to type-only for Remix Action/Loader types. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/**/*.ts📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
🧬 Code graph analysis (2)apps/webapp/app/routes/invites.tsx (1)
apps/webapp/app/models/member.server.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
🔇 Additional comments (3)
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 |
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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/models/member.server.ts (1)
262-276
: Blocker: invalid Prisma update where; add explicit access check then updateupdate(where: …) must be unique; adding inviterId in where isn’t supported unless it’s part of a unique constraint (and id already is unique). Do a two‑step access check and then update by id.
Replace the function body:
export async function resendInvite({ inviteId, userId }: { inviteId: string; userId: string }) { - return await prisma.orgMemberInvite.update({ - where: { - id: inviteId, - inviterId: userId, - }, - data: { - updatedAt: new Date(), - }, - include: { - inviter: true, - organization: true, - }, - }); + const invite = await prisma.orgMemberInvite.findFirst({ + where: { + id: inviteId, + // Require requester to be a member; adjust policy if only original inviter can resend: + organization: { members: { some: { userId } } }, + }, + include: { inviter: true, organization: true }, + }); + if (!invite) throw new Error("Invite not found or you don't have access"); + return await prisma.orgMemberInvite.update({ + where: { id: invite.id }, + data: { updatedAt: new Date() }, + include: { inviter: true, organization: true }, + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webapp/app/models/member.server.ts
(6 hunks)apps/webapp/app/routes/invite-accept.tsx
(1 hunks)apps/webapp/app/routes/invite-resend.tsx
(2 hunks)apps/webapp/app/routes/invite-revoke.tsx
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}
: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/routes/invite-resend.tsx
apps/webapp/app/routes/invite-revoke.tsx
apps/webapp/app/routes/invite-accept.tsx
apps/webapp/app/models/member.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/routes/invite-resend.tsx
apps/webapp/app/routes/invite-revoke.tsx
apps/webapp/app/routes/invite-accept.tsx
apps/webapp/app/models/member.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/invite-resend.tsx
apps/webapp/app/routes/invite-revoke.tsx
apps/webapp/app/routes/invite-accept.tsx
apps/webapp/app/models/member.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/models/member.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/models/member.server.ts
🧬 Code graph analysis (2)
apps/webapp/app/routes/invite-accept.tsx (2)
apps/webapp/app/models/message.server.ts (1)
redirectWithSuccessMessage
(162-179)apps/webapp/app/models/member.server.ts (1)
getInviteFromToken
(123-140)
apps/webapp/app/models/member.server.ts (1)
apps/webapp/app/presenters/OrganizationsPresenter.server.ts (1)
userId
(126-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
apps/webapp/app/routes/invite-accept.tsx (1)
21-25
: Good: require auth before fetching the inviteEarly redirect for unauthenticated users avoids leaking invite details.
apps/webapp/app/routes/invite-resend.tsx (2)
2-2
: Type-only import LGTMType-only ActionFunction import is correct.
26-29
: Use env via env.server.ts (don’t import from process)Per project guidelines for apps/webapp/app routes, access environment variables via env from app/env.server.ts, not Node’s process env shim.
Change the import and usage:
// replace import { env } from "process"; // with import { env } from "~/env.server";[ suggest_essential_refactor ]
apps/webapp/app/routes/invite-revoke.tsx (2)
2-2
: Type-only import LGTMConsistent with other routes.
25-29
: Param rename to orgSlug is correctMapping submission.value.slug to orgSlug matches the updated server API.
apps/webapp/app/models/member.server.ts (3)
205-212
: Remove membership filter from remainingInvites in accept flowAfter accepting, remainingInvites should be filtered by email; requiring membership would often return zero and is unnecessary.
- organization: - members: - some: - userId, + // No membership gating here; the recipient may not be a member yet[ suggest_recommended_refactor ]
248-254
: Remove membership filter from decline remainingInvitesSame reasoning as accept: filter by email; no membership gating needed.
- organization: - members: - some: - userId, + // No membership constraint here[ suggest_recommended_refactor ]
305-310
: Nit: unreachable null-check after deleteprisma.delete throws if the record doesn’t exist. The if (!invite) check is unreachable once you adopt the findFirst + delete flow above. Remove it.
[ suggest_optional_refactor ]
* fix: org invite scoping Fixes some scoping issues with team invites. * Fix invite flow changes
Fixes some scoping issues with team invites.