fix: api v2 workflow controller trigger validation#23159
Conversation
WalkthroughThe PR adds runtime validation decorators to workflow trigger DTOs: string and enum checks for trigger.type across multiple DTO variants and for offset.unit against TIME_UNITS. It also publicly exports OnBeforeEventTriggerDto and OnAfterEventTriggerDto. End-to-end tests for organization team workflows are updated to import these DTOs, create a workflow with a before-event trigger, and verify offset value/unit, then patch to an after-event trigger and verify type and offset synchronization. No control flow or data structure changes beyond the added validations. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
keithwillcode
added
core
dosubot
bot
added
api
![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
🧹 Nitpick comments (4)
apps/api/v2/src/modules/workflows/inputs/workflow-trigger.input.ts (3)
71-73: LGTM: Enforces unit to one of TIME_UNITSValidation for unit looks correct.
Optionally:
- Add enum metadata to Swagger for better API docs.
- Enforce offset.value as an integer with a sensible minimum (e.g., 1) and coerce input to number.
Outside-change snippet (for reference):
// import additions import { IsIn, IsNumber, IsString, ValidateNested, IsInt, Min, IsDefined } from "class-validator"; // value field export class WorkflowTriggerOffsetDto { @ApiProperty({ description: "Time value for offset before/after event trigger", example: 24, type: Number }) @Type(() => Number) @IsInt() @Min(1) value!: number; @ApiProperty({ description: "Unit for the offset time", example: HOUR, enum: TIME_UNITS }) @IsString() @IsIn(TIME_UNITS) unit!: TimeUnitType; } // ensure presence of offset when required export class TriggerOffsetDTO { @ApiProperty({ description: "Offset before/after the trigger time; required for BEFORE_EVENT and AFTER_EVENT only", type: WorkflowTriggerOffsetDto, }) @IsDefined() @ValidateNested() @Type(() => WorkflowTriggerOffsetDto) offset!: WorkflowTriggerOffsetDto; }
121-129: LGTM: BEFORE_EVENT trigger validationCorrectly constrains the type to BEFORE_EVENT.
To truly enforce offset presence for before/after triggers, consider adding @isdefined() to TriggerOffsetDTO.offset (see suggestion in earlier comment).
136-138: LGTM: AFTER_EVENT trigger validationLooks correct.
Same note as before: add @isdefined() to TriggerOffsetDTO.offset to ensure presence at runtime validation.
apps/api/v2/src/modules/organizations/teams/workflows/controllers/org-team-workflows.controller.e2e-spec.ts (1)
342-348: Prefer constants over string literals for trigger type and unitUsing AFTER_EVENT and MINUTE constants avoids typos and keeps tests aligned with DTO constraints.
Apply these diffs:
Update imports to include AFTER_EVENT and MINUTE:
-import { - BEFORE_EVENT, - DAY, - OnAfterEventTriggerDto, - OnBeforeEventTriggerDto, -} from "@/modules/workflows/inputs/workflow-trigger.input"; +import { + BEFORE_EVENT, + AFTER_EVENT, + DAY, + MINUTE, + OnAfterEventTriggerDto, + OnBeforeEventTriggerDto, +} from "@/modules/workflows/inputs/workflow-trigger.input";Use the constants in the payload:
- trigger: { - type: "afterEvent", - offset: { - unit: "minute", - value: 10, - }, - }, + trigger: { + type: AFTER_EVENT, + offset: { + unit: MINUTE, + value: 10, + }, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/api/v2/src/modules/organizations/teams/workflows/controllers/org-team-workflows.controller.e2e-spec.ts(4 hunks)apps/api/v2/src/modules/workflows/inputs/workflow-trigger.input.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/api/v2/src/modules/organizations/teams/workflows/controllers/org-team-workflows.controller.e2e-spec.tsapps/api/v2/src/modules/workflows/inputs/workflow-trigger.input.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/api/v2/src/modules/organizations/teams/workflows/controllers/org-team-workflows.controller.e2e-spec.tsapps/api/v2/src/modules/workflows/inputs/workflow-trigger.input.ts
🧬 Code Graph Analysis (1)
apps/api/v2/src/modules/organizations/teams/workflows/controllers/org-team-workflows.controller.e2e-spec.ts (1)
apps/api/v2/src/modules/workflows/inputs/workflow-trigger.input.ts (2)
OnBeforeEventTriggerDto(121-129)OnAfterEventTriggerDto(131-139)
⏰ 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). (8)
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build Web App
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
- GitHub Check: Production builds / Build API v2
- GitHub Check: Production builds / Build API v1
- GitHub Check: Tests / Unit
- GitHub Check: Atoms E2E Tests
🔇 Additional comments (9)
apps/api/v2/src/modules/workflows/inputs/workflow-trigger.input.ts (6)
4-4: LGTM: Added runtime validators importImporting IsIn and IsString is appropriate for the new validations.
89-91: LGTM: NEW_EVENT trigger type validationThe constraint is precise and self-documenting.
98-100: LGTM: RESCHEDULE_EVENT trigger type validationLooks good.
106-108: LGTM: EVENT_CANCELLED trigger type validationLooks good.
146-148: LGTM: AFTER_GUESTS_CAL_VIDEO_NO_SHOW trigger validationLooks correct.
156-158: LGTM: AFTER_HOSTS_CAL_VIDEO_NO_SHOW trigger validationLooks correct.
apps/api/v2/src/modules/organizations/teams/workflows/controllers/org-team-workflows.controller.e2e-spec.ts (3)
15-20: LGTM: Imports bring DTOs and constants needed for trigger assertionsUsing the exported DTOs improves test clarity and type coverage.
263-266: LGTM: Verifies created trigger offset round-tripsGood assertion of both value and unit.
367-370: LGTM: Validates updated trigger type and offset fields post-PATCHGood end-to-end assertion of the update path.
| @IsString() | ||
| @IsIn([WORKFLOW_TRIGGER_TYPES]) | ||
| type!: WorkflowTriggerType; |
There was a problem hiding this comment.
Bug: @isin used with nested array causes all validations to fail
@isin([WORKFLOW_TRIGGER_TYPES]) wraps the allowed values array in another array, so no string can ever match. Use @isin(WORKFLOW_TRIGGER_TYPES).
Apply this diff:
@ApiProperty({
description: "Trigger type for the workflow",
})
@IsString()
- @IsIn([WORKFLOW_TRIGGER_TYPES])
+ @IsIn(WORKFLOW_TRIGGER_TYPES)
type!: WorkflowTriggerType;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @IsString() | |
| @IsIn([WORKFLOW_TRIGGER_TYPES]) | |
| type!: WorkflowTriggerType; | |
| @ApiProperty({ | |
| description: "Trigger type for the workflow", | |
| }) | |
| @IsString() | |
| @IsIn(WORKFLOW_TRIGGER_TYPES) | |
| type!: WorkflowTriggerType; |
🤖 Prompt for AI Agents
In apps/api/v2/src/modules/workflows/inputs/workflow-trigger.input.ts around
lines 80 to 82, the @IsIn decorator currently wraps the allowed-values array in
another array which makes validation always fail; replace
@IsIn([WORKFLOW_TRIGGER_TYPES]) with @IsIn(WORKFLOW_TRIGGER_TYPES) so the
decorator receives the actual array of allowed strings (no other changes
needed).
E2E results are ready! |
3 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist