Regions – dashboard page, switching default, allowedMasterQueues #2354
Conversation
|
|
Caution Review failedThe pull request is closed. WalkthroughThis set of changes introduces a new Regions management feature to the web application. It adds new database columns to the Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal-packages/database/prisma/migrations/20250805161650_worker_instance_group_added_cloud_provider_location_static_ips_columns/migration.sql (1)
1-5: LGTM! Well-structured metadata addition.The migration correctly adds the necessary cloud provider metadata fields. All columns are appropriately nullable for backward compatibility.
Consider documenting the expected format for
staticIPs(comma-separated, JSON, etc.) to ensure consistent data entry across the application.apps/webapp/app/v3/services/setDefaultRegion.server.ts (1)
25-32: Consider refactoring for improved readability.The access control logic is correct but could be more readable. Consider restructuring for clarity:
- // If their project is restricted, only allow them to set default regions that are allowed - if (project.allowedMasterQueues.length > 0) { - if (!project.allowedMasterQueues.includes(workerGroup.masterQueue)) { - throw new ServiceValidationError("You're not allowed to set this region as default"); - } - } else if (workerGroup.hidden) { - throw new ServiceValidationError("This region is not available to you"); - } + // Check access permissions + const hasRestrictedQueues = project.allowedMasterQueues.length > 0; + + if (hasRestrictedQueues) { + if (!project.allowedMasterQueues.includes(workerGroup.masterQueue)) { + throw new ServiceValidationError("You're not allowed to set this region as default"); + } + } else if (workerGroup.hidden) { + throw new ServiceValidationError("This region is not available to you"); + }apps/webapp/app/assets/icons/CloudProviderIcon.tsx (1)
8-15: Consider using an object map for better maintainability.The switch statement works correctly, but as more providers are added, an object map pattern would be more maintainable and scalable.
+const providerComponents = { + aws: AWS, + digitalocean: DigitalOcean, +} as const; + export function CloudProviderIcon({ provider, className, }: { provider: "aws" | "digitalocean" | (string & {}); className?: string; }) { - switch (provider) { - case "aws": - return <AWS className={className} />; - case "digitalocean": - return <DigitalOcean className={className} />; - default: - return null; - } + const Component = providerComponents[provider as keyof typeof providerComponents]; + return Component ? <Component className={className} /> : null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/webapp/app/assets/icons/CloudProviderIcon.tsx(1 hunks)apps/webapp/app/assets/icons/RegionIcons.tsx(1 hunks)apps/webapp/app/components/CloudProvider.tsx(1 hunks)apps/webapp/app/components/navigation/SideMenu.tsx(4 hunks)apps/webapp/app/components/primitives/Badge.tsx(1 hunks)apps/webapp/app/presenters/v3/RegionsPresenter.server.ts(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx(1 hunks)apps/webapp/app/routes/resources.feedback.ts(1 hunks)apps/webapp/app/utils/pathBuilder.ts(1 hunks)apps/webapp/app/v3/services/setDefaultRegion.server.ts(1 hunks)internal-packages/database/prisma/migrations/20250805161650_worker_instance_group_added_cloud_provider_location_static_ips_columns/migration.sql(1 hunks)internal-packages/database/prisma/migrations/20250806124301_project_allowed_master_queues_column/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/components/primitives/Badge.tsxapps/webapp/app/components/CloudProvider.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsxapps/webapp/app/assets/icons/RegionIcons.tsxapps/webapp/app/presenters/v3/RegionsPresenter.server.tsapps/webapp/app/routes/resources.feedback.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/v3/services/setDefaultRegion.server.tsapps/webapp/app/assets/icons/CloudProviderIcon.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/components/primitives/Badge.tsxapps/webapp/app/components/CloudProvider.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsxapps/webapp/app/assets/icons/RegionIcons.tsxapps/webapp/app/presenters/v3/RegionsPresenter.server.tsapps/webapp/app/routes/resources.feedback.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/v3/services/setDefaultRegion.server.tsapps/webapp/app/assets/icons/CloudProviderIcon.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
Files:
apps/webapp/app/components/primitives/Badge.tsxapps/webapp/app/components/CloudProvider.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsxapps/webapp/app/assets/icons/RegionIcons.tsxapps/webapp/app/presenters/v3/RegionsPresenter.server.tsapps/webapp/app/routes/resources.feedback.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/v3/services/setDefaultRegion.server.tsapps/webapp/app/assets/icons/CloudProviderIcon.tsx
🧠 Learnings (4)
📚 Learning: applies to internal-packages/database/**/*.{ts,tsx} : we use prisma in internal-packages/database fo...
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T17:49:24.468Z
Learning: Applies to internal-packages/database/**/*.{ts,tsx} : We use prisma in internal-packages/database for our database interactions using PostgreSQL
Applied to files:
internal-packages/database/prisma/migrations/20250806124301_project_allowed_master_queues_column/migration.sqlinternal-packages/database/prisma/migrations/20250805161650_worker_instance_group_added_cloud_provider_location_static_ips_columns/migration.sqlinternal-packages/database/prisma/schema.prisma
📚 Learning: in modern postgresql versions (11+), adding a column with a constant default value (like default fal...
Learnt from: ericallam
PR: triggerdotdev/trigger.dev#2086
File: internal-packages/database/prisma/migrations/20250511145836_runtime_environment_add_is_branchable_environment/migration.sql:1-3
Timestamp: 2025-05-27T19:30:34.004Z
Learning: In modern PostgreSQL versions (11+), adding a column with a constant default value (like DEFAULT false, DEFAULT 0, DEFAULT 'text') does NOT require a table rewrite. PostgreSQL stores the default value in the catalog and applies it virtually when reading rows. Only non-constant defaults or more complex scenarios require table rewrites. Avoid suggesting multi-step migrations for simple constant defaults.
Applied to files:
internal-packages/database/prisma/migrations/20250806124301_project_allowed_master_queues_column/migration.sql
📚 Learning: applies to apps/webapp/app/v3/presenters/**/*.server.ts : favor the use of 'presenters' to move comp...
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-07-18T17:49:47.180Z
Learning: Applies to apps/webapp/app/v3/presenters/**/*.server.ts : Favor the use of 'presenters' to move complex loader code into a class, as seen in `app/v3/presenters/**/*.server.ts`.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsxapps/webapp/app/presenters/v3/RegionsPresenter.server.ts
📚 Learning: applies to apps/webapp/app/services/**/*.server.ts : for testable services, separate service logic a...
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-07-18T17:49:47.180Z
Learning: Applies to apps/webapp/app/services/**/*.server.ts : For testable services, separate service logic and configuration, as exemplified by `realtimeClient.server.ts` (service) and `realtimeClientGlobal.server.ts` (configuration).
Applied to files:
apps/webapp/app/v3/services/setDefaultRegion.server.ts
🧬 Code Graph Analysis (1)
apps/webapp/app/components/navigation/SideMenu.tsx (3)
apps/webapp/app/components/navigation/SideMenuItem.tsx (1)
SideMenuItem(7-53)apps/webapp/app/utils/pathBuilder.ts (1)
regionsPath(456-462)apps/webapp/app/components/V4Badge.tsx (1)
V4Badge(5-17)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 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 (4, 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 (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 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 - npm)
🔇 Additional comments (19)
internal-packages/database/prisma/migrations/20250806124301_project_allowed_master_queues_column/migration.sql (1)
1-2: LGTM! Clean and efficient migration.The migration correctly adds the
allowedMasterQueuescolumn with a proper default value. UsingARRAY[]::TEXT[]provides a clean empty array default, and modern PostgreSQL will handle this efficiently without requiring a table rewrite.apps/webapp/app/components/primitives/Badge.tsx (1)
9-10: LGTM! Consistent variant addition.The new "small" variant follows the established styling pattern and provides appropriate size progression between "extra-small" and the default variants.
apps/webapp/app/utils/pathBuilder.ts (1)
456-462: LGTM! Perfect consistency with existing patterns.The
regionsPathfunction follows the established conventions exactly, using the same parameter types and path-building approach as other environment-level functions.apps/webapp/app/routes/resources.feedback.ts (1)
18-18: LGTM! Clean integration with existing feedback system.The new "region" feedback type integrates seamlessly with the existing dynamic schema construction. The label is clear and descriptive.
apps/webapp/app/components/CloudProvider.tsx (1)
1-10: LGTM! Clean and type-safe cloud provider mapping.The function correctly maps known cloud providers to human-readable titles and falls back gracefully for unknown providers. The union type with string extension
(string & {})is a good pattern for allowing specific known values while maintaining flexibility.apps/webapp/app/components/navigation/SideMenu.tsx (4)
13-13: Good import cleanup.The addition of
GlobeAmericasIconimport is appropriate for the new regions feature.
26-26: Correct import retained.The
ListCheckedIconimport is correctly kept as it's used in the "Bulk actions" menu item.
51-51: Proper path utility import.The
regionsPathimport follows the established pattern for route path builders.
315-322: LGTM! Consistent navigation item implementation.The new "Regions" menu item follows the established patterns:
- Appropriate icon (
GlobeAmericasIcon) and active color (text-green-500)- Consistent with other V4 features by including the
V4Badge- Proper
data-actionattribute for tracking- Uses the correct path builder function
internal-packages/database/prisma/schema.prisma (3)
338-340: Well-documented access control field.The
allowedMasterQueuesfield with descriptive comment clearly explains its purpose for controlling project permissions. The default empty array is appropriate for backward compatibility.
1148-1149: Good documentation update.The enhanced comment for the
descriptionfield clarifies its display purpose with concrete examples.
1166-1170: Well-designed metadata fields.The new optional fields provide useful region metadata:
cloudProvider: Clear purpose for identifying the cloud servicelocation: Well-documented as pseudo-enum for geographic flagsstaticIPs: Useful for displaying network informationAll fields are optional, ensuring backward compatibility.
apps/webapp/app/v3/services/setDefaultRegion.server.ts (3)
4-13: LGTM! Proper validation with clear error handling.The region lookup and error handling follow good practices with descriptive error messages.
15-23: LGTM! Consistent project validation.The project lookup follows the same pattern as the region validation with appropriate error handling.
34-47: LGTM! Clean update operation with appropriate return value.The database update and return value provide exactly what's needed for the calling code.
apps/webapp/app/assets/icons/RegionIcons.tsx (3)
1-16: LGTM! Clean conditional icon rendering.The
FlagIconcomponent follows the same pattern asCloudProvider.tsxwith proper type safety and graceful fallback for unknown regions.
18-66: LGTM! Well-structured SVG component.The
FlagUSAcomponent provides a detailed and accurate representation with proper className support for styling.
68-106: LGTM! Consistent SVG implementation.The
FlagEuropecomponent follows the same pattern asFlagUSAwith appropriate styling support and accurate flag representation.apps/webapp/app/presenters/v3/RegionsPresenter.server.ts (1)
63-69: allowedMasterQueues is always a non-null arrayIn schema.prisma (line 339) the field is declared as:
allowedMasterQueues String[] @default([])which guarantees a default empty array rather than null—calling
.lengthis safe.
...pp/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx
Show resolved
Hide resolved
...pp/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/app/presenters/v3/RegionsPresenter.server.ts (1)
17-40: Consider using more specific error types.The project fetching logic is secure and efficient with proper user membership validation. However, the generic
Errorcould be replaced with a more specific error type for better error handling upstream.- if (!project) { - throw new Error("Project not found"); - } + if (!project) { + throw new Response("Project not found", { status: 404 }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/presenters/v3/RegionsPresenter.server.ts(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/presenters/v3/RegionsPresenter.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/presenters/v3/RegionsPresenter.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
Files:
apps/webapp/app/presenters/v3/RegionsPresenter.server.ts
🧠 Learnings (2)
📚 Learning: in regionspresenter.server.ts, the final sort by name that removes the "default first" ordering is i...
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2354
File: apps/webapp/app/presenters/v3/RegionsPresenter.server.ts:117-131
Timestamp: 2025-08-06T14:25:20.389Z
Learning: In RegionsPresenter.server.ts, the final sort by name that removes the "default first" ordering is intentional behavior as a way to prefer the default, as clarified by matt-aitken.
Applied to files:
apps/webapp/app/presenters/v3/RegionsPresenter.server.ts
📚 Learning: applies to apps/webapp/app/v3/presenters/**/*.server.ts : favor the use of 'presenters' to move comp...
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-07-18T17:49:47.180Z
Learning: Applies to apps/webapp/app/v3/presenters/**/*.server.ts : Favor the use of 'presenters' to move complex loader code into a class, as seen in `app/v3/presenters/**/*.server.ts`.
Applied to files:
apps/webapp/app/presenters/v3/RegionsPresenter.server.ts
⏰ 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). (2)
- GitHub Check: units / internal / 📊 Merge Reports
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
🔇 Additional comments (8)
apps/webapp/app/presenters/v3/RegionsPresenter.server.ts (8)
1-15: LGTM! Clean imports and type definition.The imports follow the coding guidelines correctly, and the
Regiontype definition properly usestypeinstead ofinterfaceas preferred in the codebase.
42-49: LGTM! Proper feature flag usage.The feature flag retrieval follows the established pattern and includes appropriate error handling for a critical system dependency.
51-72: LGTM! Well-implemented access control logic.The conditional query properly restricts region visibility based on project permissions, allowing restricted projects to access their allowed master queues while limiting unrestricted projects to non-hidden regions.
74-82: LGTM! Clean data mapping.The mapping properly handles null coalescing to undefined for optional fields and correctly assigns the default flag based on the feature flag value.
84-114: LGTM! Proper default region override handling.The logic correctly handles project-specific default worker groups by unsetting the previous default and adding the new one, maintaining data consistency.
116-127: LGTM! Sorting and deduplication logic is correct.The sorting properly places the default region first, and the deduplication correctly removes later duplicates by ID while preserving the first occurrence.
129-138: LGTM! Appropriate business logic for plan-based features.The filtering correctly hides static IPs from non-paying users to avoid confusion, while properly handling both null and undefined cases.
140-144: LGTM! Return statement with intentional sorting.The final alphabetical sort and return structure are correct. As previously clarified, the alphabetical sorting that removes "default first" ordering is intentional behavior.
Uh oh!
There was an error while loading. Please reload this page.