feat(api): add admin endpoint for updating org feature flags #2891
Conversation
|
WalkthroughThis change introduces a new admin API route file that provides authenticated management of organization feature flags. The file exports two functions: a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/webapp/app/routes/admin.api.v1.orgs`.$organizationId.feature-flags.ts:
- Around line 145-151: The catch block currently returns the raw error via
json({ error: error instanceof Error ? error.message : String(error) }, {
status: 400 }), which can leak internal details; change it to return a generic
message (e.g., json({ error: "An unexpected error occurred" }, { status: 400 })
or appropriate 5xx) and log the original error server-side using your app logger
or console.error before returning; update the catch in the route handler that
calls json(...) so the response is generic while the real error is recorded
internally.
🧹 Nitpick comments (4)
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts (4)
42-42: Unhandled parse exception could return 500 error.
ParamsSchema.parse()throws aZodErrorif validation fails. While this is unlikely in practice since Remix guarantees the param exists, wrapping this in a try-catch or using.safeParse()would provide more consistent error responses.🔧 Suggested improvement
- const { organizationId } = ParamsSchema.parse(params); + const parsedParams = ParamsSchema.safeParse(params); + if (!parsedParams.success) { + return json({ error: "Invalid organization ID parameter" }, { status: 400 }); + } + const { organizationId } = parsedParams.data;
59-63: Silent fallback on invalid stored feature flags.If the organization has stored
featureFlagsthat fail validation, the loader silently returns an empty object. This could mask data corruption. Consider logging or including a warning in the response when stored flags fail validation.🔧 Suggested improvement
const flagsResult = organization.featureFlags ? validatePartialFeatureFlags(organization.featureFlags as Record<string, unknown>) : { success: false as const }; - const featureFlags = flagsResult.success ? flagsResult.data : {}; + const featureFlags = flagsResult.success ? flagsResult.data : {}; + const hasInvalidStoredFlags = organization.featureFlags && !flagsResult.success; return json({ organizationId: organization.id, organizationSlug: organization.slug, featureFlags, + ...(hasInvalidStoredFlags && { warning: "Stored feature flags failed validation and were reset" }), });
72-77: Consider restricting HTTP methods.The
actionfunction handles all non-GET methods (POST, PUT, PATCH, DELETE). For a feature flags update endpoint, consider explicitly checking the request method to only allow PUT or PATCH.🔧 Suggested improvement
export async function action({ request, params }: ActionFunctionArgs) { + if (request.method !== "PUT" && request.method !== "PATCH") { + return json({ error: "Method not allowed" }, { status: 405 }); + } + const authResult = await authenticateAdmin(request);
81-89: Minor inconsistency: missingslugin initial organization query.The initial query (lines 81-89) doesn't select
slug, but the final response includesorganizationSlug. This works because the update query fetches it, but for consistency (and to support future changes like early returns), consider selectingslugin the initial query as well.🔧 Suggested improvement
const organization = await prisma.organization.findUnique({ where: { id: organizationId, }, select: { id: true, + slug: true, featureFlags: true, }, });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects insteadEvery Trigger.dev task must be exported; use task() function with unique id and run async function
Files:
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webappIn webapp development, access environment variables via env export from apps/webapp/app/env.server.ts; never use process.env directly
Files:
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: Import from@trigger.dev/coreusing subpaths only; never import from root of@trigger.dev/core
Always import task definitions from@trigger.dev/sdk, never from@trigger.dev/sdk/v3or deprecated client.defineJob pattern
Files:
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts
🧬 Code graph analysis (1)
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts (3)
apps/webapp/app/services/personalAccessToken.server.ts (1)
authenticateApiRequestWithPersonalAccessToken(110-119)packages/core/src/v3/apps/http.ts (1)
json(65-75)apps/webapp/app/v3/featureFlags.server.ts (1)
validatePartialFeatureFlags(104-106)
⏰ 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 / webapp / 🧪 Unit Tests: Webapp (5, 8)
- 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 (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- 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 - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts (4)
1-5: LGTM!Imports are appropriate and follow the coding guidelines - using Zod for validation and proper Remix server-runtime imports.
7-9: LGTM!The params schema follows Zod validation conventions as per coding guidelines.
11-33: LGTM!The authentication function properly validates the token, checks user existence, and verifies admin status with appropriate HTTP status codes (401 for auth failures, 403 for authorization failures).
110-118: LGTM!The merge logic correctly combines existing validated flags with new validated input. Since both sides are validated against the same partial schema before merging, the result should remain valid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
No description provided.