fix(webapp): project scoping for runs #2553
Conversation
|
WalkthroughRemoved organizationSlug from RunPresenter.call. Presenters and routes now require and pass a userId from requireUserId; project/run lookups are scoped to organization.members.some.userId. SpanPresenter.call gained userId, derives environmentId from the parent run, and threads environmentId into getRun and span retrieval. EventRepository.getSpan changed to a single-object parameter that requires environmentId; internal #getSpanEvent, #createSpanFromEvent, and #walkSpanAncestors were updated to accept and propagate environmentId. Routes and action handlers updated to enforce authentication/authorization. One import was changed to a type-only import. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/webapp/app/presenters/v3/SpanPresenter.server.ts (3)
41-56: Project lookup by slug + membership can be ambiguous across orgsIf a user belongs to multiple orgs that each have a project with the same slug, findFirst may pick the wrong project or return none (leading to false "not found"). Avoid preselecting project; instead, resolve the parent run by joining through project slug + membership, then derive projectId from that run.
Apply:
- const project = await this._replica.project.findFirst({ - where: { - slug: projectSlug, - organization: { - members: { - some: { - userId, - }, - }, - }, - }, - }); - - if (!project) { - throw new Error("Project not found"); - } - - const parentRun = await this._prisma.taskRun.findFirst({ + const parentRun = await this._prisma.taskRun.findFirst({ select: { traceId: true, runtimeEnvironmentId: true, projectId: true, taskEventStore: true, createdAt: true, completedAt: true, }, where: { friendlyId: runFriendlyId, - projectId: project.id, + project: { + slug: projectSlug, + organization: { + members: { some: { userId } }, + }, + }, }, });This removes ambiguity without reintroducing organizationSlug into the presenter API.
Also applies to: 68-71
137-142: Constrain findRun by environmentId to prevent cross-project selectionfindRun currently matches by friendlyId or spanId only. If these identifiers aren't globally unique, you could resolve a run from a different project/environment. Thread environmentId into findRun and include it in the where clause.
Apply:
- const run = await this.findRun({ span, spanId }); + const run = await this.findRun({ span, spanId, environmentId });- async findRun({ span, spanId }: { span: GetSpanResult; spanId: string }) { + async findRun({ + span, + spanId, + environmentId, + }: { + span: GetSpanResult; + spanId: string; + environmentId: string; + }) {- where: span.originalRun - ? { - friendlyId: span.originalRun, - } - : { - spanId, - }, + where: span.originalRun + ? { + friendlyId: span.originalRun, + runtimeEnvironmentId: environmentId, + } + : { + spanId, + runtimeEnvironmentId: environmentId, + },Also applies to: 297-301, 407-414
447-463: Scope triggeredRuns to the same environment/projectAdd environmentId and projectId constraints to avoid pulling child runs from other projects/environments that happen to reuse the same parentSpanId.
Apply:
const triggeredRuns = await this._replica.taskRun.findMany({ @@ where: { - parentSpanId: spanId, + parentSpanId: spanId, + runtimeEnvironmentId: environmentId, + projectId, }, });
🧹 Nitpick comments (2)
apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts (1)
16-33: Use read-replica for GET loader to reduce primary DB loadThis is a read-only lookup. Consider $replica.taskRun.findFirst for consistency with other routes (e.g., resources.taskruns.$runParam.debug) and to offload reads.
apps/webapp/app/presenters/v3/RunPresenter.server.ts (1)
90-104: Filter by environmentSlug at the DB layer to avoid mismatch errors and extra workYou validate environmentSlug after fetching the run. Add runtimeEnvironment.slug to the where clause to fetch only matching runs and reduce error handling.
Apply this diff:
where: { friendlyId: runFriendlyId, project: { slug: projectSlug, organization: { members: { some: { userId, }, }, }, }, + runtimeEnvironment: { + slug: environmentSlug, + }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/webapp/app/presenters/v3/RunPresenter.server.ts(1 hunks)apps/webapp/app/presenters/v3/SpanPresenter.server.ts(4 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx(0 hunks)apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts(1 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx(2 hunks)apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts(3 hunks)apps/webapp/app/routes/resources.taskruns.$runParam.debug.ts(1 hunks)apps/webapp/app/v3/eventRepository.server.ts(3 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
🧰 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/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsxapps/webapp/app/routes/resources.taskruns.$runParam.debug.tsapps/webapp/app/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository.server.tsapps/webapp/app/routes/resources.taskruns.$runParam.cancel.tsapps/webapp/app/presenters/v3/SpanPresenter.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/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsxapps/webapp/app/routes/resources.taskruns.$runParam.debug.tsapps/webapp/app/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository.server.tsapps/webapp/app/routes/resources.taskruns.$runParam.cancel.tsapps/webapp/app/presenters/v3/SpanPresenter.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/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsxapps/webapp/app/routes/resources.taskruns.$runParam.debug.tsapps/webapp/app/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository.server.tsapps/webapp/app/routes/resources.taskruns.$runParam.cancel.tsapps/webapp/app/presenters/v3/SpanPresenter.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/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.tsapps/webapp/app/routes/resources.taskruns.$runParam.debug.tsapps/webapp/app/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository.server.tsapps/webapp/app/routes/resources.taskruns.$runParam.cancel.tsapps/webapp/app/presenters/v3/SpanPresenter.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/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.tsapps/webapp/app/routes/resources.taskruns.$runParam.debug.tsapps/webapp/app/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository.server.tsapps/webapp/app/routes/resources.taskruns.$runParam.cancel.tsapps/webapp/app/presenters/v3/SpanPresenter.server.ts
🧬 Code graph analysis (5)
apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts (3)
apps/webapp/app/utils/pathBuilder.ts (1)
ProjectParamSchema(22-24)apps/webapp/app/routes/resources.taskruns.$runParam.debug.ts (1)
loader(13-157)apps/webapp/app/services/session.server.ts (1)
requireUserId(25-35)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx (1)
apps/webapp/app/services/session.server.ts (1)
requireUserId(25-35)
apps/webapp/app/v3/eventRepository.server.ts (2)
apps/webapp/app/v3/taskEventStore.server.ts (1)
TaskEventStoreTable(54-54)apps/webapp/app/v3/tracer.server.ts (1)
startActiveSpan(113-142)
apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts (2)
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/presenters/v3/SpanPresenter.server.ts (2)
apps/webapp/app/v3/taskEventStore.server.ts (2)
getTaskEventStoreTableForRun(56-60)TaskEventStoreTable(54-54)apps/webapp/app/v3/eventRepository.server.ts (1)
eventRepository(1785-1785)
⏰ 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 / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 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 (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- 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 - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
apps/webapp/app/routes/resources.taskruns.$runParam.debug.ts (1)
1-1: Type-only import matches Remix conventionsSwitching
LoaderFunctionArgsto a type-only import removes an unnecessary runtime reference and keeps the loader signature unchanged. Looks good.apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts (1)
13-14: Auth guard now enforced — goodCapturing the userId up front aligns this route with the new membership-based authorization.
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx (1)
81-96: Auth propagation into presenter — goodRequiring userId in the loader and threading it to SpanPresenter.call matches the new access model.
apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts (2)
19-20: Auth guard added — goodEnsures only authenticated users can attempt cancellations.
30-42: Membership-based authorization on cancel — goodFiltering the run by organization.members.some.userId is the right scope guard for this action.
apps/webapp/app/v3/eventRepository.server.ts (1)
971-987: getSpan call sites include environmentId
Both usages in SpanPresenter.server.ts now passenvironmentIdtogetSpan. No further changes required.apps/webapp/app/presenters/v3/SpanPresenter.server.ts (2)
112-131: LGTM: environmentId-based span lookupSwitching getRun to fetch by environmentId + time bounds aligns with project scoping and store partitioning. No issues spotted here.
30-40: All SpanPresenter.call calls include userId Verified that the spans route loader’spresenter.callinvocation passes the newuserIdparameter.
apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts
Show resolved
Hide resolved
myftija
changed the title
Adds proper project scoping for some of the run related pages.