fix(webapp): org scoping issues in plan selection, alerts, pats and usage #2549
Conversation
Adds proper org scoping in the loader and action in the plans page.
|
WalkthroughLoader functions in several org-related routes now await and store the authenticated user's ID via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (1)
🚧 Files skipped from review as they are similar to previous changes (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). (20)
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 |
myftija
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx (1)
74-79: Guard against invalid month query causing a crash.If month is invalid, startDate.toISOString() will throw. Fallback to current month when Date parsing fails.
Apply:
- const searchMonth = search.get("month"); - const startDate = searchMonth ? new Date(decodeURIComponent(searchMonth)) : months[0]; + const searchMonth = search.get("month"); + const candidateDate = searchMonth ? new Date(decodeURIComponent(searchMonth)) : months[0]; + const startDate = isNaN(candidateDate.getTime()) ? months[0] : candidateDate; startDate.setUTCDate(1); startDate.setUTCHours(0, 0, 0, 0);
🧹 Nitpick comments (3)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx (1)
57-59: Good: membership‑scoped org lookup.Using findFirst with members.some guards access by membership. Consider also filtering out deleted orgs and selecting only needed fields.
Apply:
- const organization = await prisma.organization.findFirst({ - where: { slug: organizationSlug, members: { some: { userId } } }, - }); + const organization = await prisma.organization.findFirst({ + where: { slug: organizationSlug, members: { some: { userId } }, deletedAt: null }, + select: { id: true }, + });apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx (2)
58-60: Good: membership‑scoped org lookup. Also filter deleted and select only id.Tight scoping looks good. For consistency with other presenters, add deletedAt: null. Select only id to reduce payload.
Apply:
- const organization = await prisma.organization.findFirst({ - where: { slug: organizationSlug, members: { some: { userId } } }, - }); + const organization = await prisma.organization.findFirst({ + where: { slug: organizationSlug, members: { some: { userId } }, deletedAt: null }, + select: { id: true }, + });
117-120: Mirror deleted‑org filter in action lookup (and select only id).Keep loader/action logic aligned; avoid acting on deleted orgs and trim selection.
Apply:
- const organization = await prisma.organization.findFirst({ - where: { slug: organizationSlug, members: { some: { userId } } }, - }); + const organization = await prisma.organization.findFirst({ + where: { slug: organizationSlug, members: { some: { userId } }, deletedAt: null }, + select: { id: true }, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx(1 hunks)apps/webapp/app/routes/account.tokens/route.tsx(5 hunks)apps/webapp/app/services/personalAccessToken.server.ts(1 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/_app.orgs.$organizationSlug.settings.usage/route.tsxapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/app/routes/account.tokens/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx
{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/_app.orgs.$organizationSlug.settings.usage/route.tsxapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/app/routes/account.tokens/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx
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/_app.orgs.$organizationSlug.settings.usage/route.tsxapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/app/routes/account.tokens/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx
{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/services/personalAccessToken.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/services/personalAccessToken.server.ts
🧬 Code graph analysis (3)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx (4)
apps/webapp/app/presenters/OrganizationsPresenter.server.ts (1)
userId(126-168)apps/webapp/app/services/session.server.ts (1)
requireUserId(25-35)apps/webapp/app/utils/pathBuilder.ts (2)
OrganizationParamsSchema(18-20)organizationPath(92-94)apps/webapp/app/features.server.ts (1)
featuresForRequest(22-25)
apps/webapp/app/routes/account.tokens/route.tsx (1)
apps/webapp/app/services/personalAccessToken.server.ts (1)
revokePersonalAccessToken(82-92)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx (4)
apps/webapp/app/presenters/OrganizationsPresenter.server.ts (1)
userId(126-168)apps/webapp/app/services/session.server.ts (1)
requireUserId(25-35)apps/webapp/app/utils/pathBuilder.ts (2)
OrganizationParamsSchema(18-20)organizationPath(92-94)apps/webapp/app/features.server.ts (1)
featuresForRequest(22-25)
⏰ 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 (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx (1)
49-49: Auth check order changed — verify redirect behavior on self‑hosted.Requiring auth before the isManagedCloud redirect may now send unauthenticated users to /login instead of the org page on self‑hosted. Confirm this is desired.
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx (1)
50-50: Auth check order changed — verify redirect behavior on self‑hosted.Requiring auth before the isManagedCloud redirect may change unauthenticated behavior. Confirm this is intentional.
apps/webapp/app/routes/account.tokens/route.tsx (4)
6-8: Type‑only imports: nice cleanup.
55-75: Loader signature simplification LGTM.
115-115: Revoke call wired with userId — OK.Ensure service uses an ownership‑guarded update (see suggested updateMany change) and propagates a clear error when no token is affected.
127-127: Exhaustive switch assertion: good defensive typing.
Adds proper org scoping in a few places in the app.