Require max duration in trigger.config. Validate TRIGGER_ACCESS_TOKEN is a PAT.
#1620
Conversation
🦋 Changeset detectedLatest commit: b806c18 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (14)
📒 Files selected for processing (6)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 1
🧹 Nitpick comments (7)
packages/cli-v3/e2e/fixtures/hello-world/trigger.config.ts (1)
6-6: Consider a shorter duration for the hello-world example.While the 1-hour duration maintains consistency across configurations, consider using a shorter duration (e.g., 300 seconds) for this hello-world example to demonstrate best practices in setting appropriate limits.
- maxDuration: 3600, + maxDuration: 300, // 5 minutes should be sufficient for hello-world examplepackages/cli-v3/e2e/fixtures/esm-only-external/trigger.config.ts (1)
9-9: Consider grouping related configuration properties.While the addition of
maxDurationis correct, consider grouping it with other execution-related properties, separate from build configuration, to improve readability.export default defineConfig({ project: "<fixture project>", dirs: ["./src/trigger"], + maxDuration: 3600, build: { external: ["mupdf"], }, - maxDuration: 3600, });packages/cli-v3/e2e/fixtures/otel-telemetry-loader/trigger.config.ts (1)
8-8: Document duration considerations for OpenAI operations.The 1-hour
maxDurationis added correctly. However, consider adding a comment to document duration considerations specific to OpenAI operations, as they might have varying completion times based on the model and task complexity.- maxDuration: 3600, + maxDuration: 3600, // Adjusted for potential long-running AI operations. Modify based on your specific OpenAI usage patterns.packages/cli-v3/src/utilities/isPersonalAccessToken.ts (2)
1-1: Consider making the token prefix readonly.To prevent accidental modification, consider making the token prefix constant readonly:
-const tokenPrefix = "tr_pat_"; +const TOKEN_PREFIX = "tr_pat_" as const;
7-12: Consider enhancing error type safety.The custom error implementation is good, but could benefit from better type safety:
-export class NotPersonalAccessTokenError extends Error { +export class NotPersonalAccessTokenError extends Error { constructor(message: string) { super(message); this.name = "NotPersonalAccessTokenError"; + Object.setPrototypeOf(this, NotPersonalAccessTokenError.prototype); } }references/nextjs-realtime/trigger.config.ts (1)
14-14: Consider grouping related configuration properties.While the addition of
maxDurationis correct, consider grouping it with other execution-related settings for better organization:export default defineConfig({ project: "proj_bzhdaqhlymtuhlrcgbqy", dirs: ["./src/trigger"], + maxDuration: 3600, telemetry: { exporters: [new AISDKExporter()], }, build: { extensions: [rscExtension({ reactDomEnvironment: "worker" })], }, - maxDuration: 3600, });packages/cli-v3/templates/trigger.config.mjs.template (1)
7-10: Add minimum duration to the configuration comments.The comments are clear and informative, but consider adding a note about the minimum allowed duration (5 seconds) to prevent configuration errors.
// The max compute seconds a task is allowed to run. If the task run exceeds this duration, it will be stopped. // You can override this on an individual task. +// Note: The minimum allowed duration is 5 seconds. // See https://trigger.dev/docs/runs/max-duration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.changeset/tiny-seals-appear.md(1 hunks)packages/cli-v3/e2e/fixtures/emit-decorator-metadata/trigger.config.ts(1 hunks)packages/cli-v3/e2e/fixtures/esm-only-external/trigger.config.ts(1 hunks)packages/cli-v3/e2e/fixtures/hello-world/trigger.config.ts(1 hunks)packages/cli-v3/e2e/fixtures/monorepo-react-email/packages/trigger/trigger.config.ts(1 hunks)packages/cli-v3/e2e/fixtures/otel-telemetry-loader/trigger.config.ts(1 hunks)packages/cli-v3/src/commands/login.ts(3 hunks)packages/cli-v3/src/config.ts(1 hunks)packages/cli-v3/src/utilities/isPersonalAccessToken.ts(1 hunks)packages/cli-v3/templates/trigger.config.mjs.template(1 hunks)packages/cli-v3/templates/trigger.config.ts.template(1 hunks)packages/core/src/v3/config.ts(1 hunks)references/bun-catalog/trigger.config.ts(1 hunks)references/nextjs-realtime/trigger.config.ts(1 hunks)references/v3-catalog/trigger.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
packages/cli-v3/e2e/fixtures/monorepo-react-email/packages/trigger/trigger.config.ts (1)
6-6: LGTM! Verify if 1 hour is sufficient for email-related tasks.The addition of
maxDuration: 3600aligns with the PR objectives and maintains consistency across configurations.Consider reviewing typical email task durations in your monitoring system to confirm that 1 hour is an appropriate limit for this specific use case.
packages/cli-v3/e2e/fixtures/emit-decorator-metadata/trigger.config.ts (1)
10-10: LGTM! Verify consistency across all test fixtures.The addition of
maxDuration: 3600aligns with the PR objective and matches the default 1-hour duration.Let's verify this value is consistent across all test fixtures:
✅ Verification successful
✅ Verified: maxDuration is consistent across all test fixtures
All test fixtures consistently use
maxDuration: 3600, aligning with the 1-hour default duration requirement.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check maxDuration value across all test fixtures rg -U "maxDuration:.*(\d+)" "packages/cli-v3/e2e/fixtures/**/trigger.config.ts"Length of output: 171
Script:
#!/bin/bash # First find all trigger.config.ts files fd "trigger.config.ts" packages/cli-v3/e2e/fixtures/ --type f --exec rg -U "maxDuration:\s*\d+" {}Length of output: 205
packages/cli-v3/src/utilities/isPersonalAccessToken.ts (1)
3-5: LGTM! Simple and focused validation.The validation function is well-implemented with a clear single responsibility.
references/bun-catalog/trigger.config.ts (1)
6-6: Verify interaction between maxDuration and retry settings.The maxDuration (3600s) looks good, but please verify its interaction with the retry configuration:
- With max 4 attempts and 10s timeouts that double each retry
- Will tasks have enough time to complete all retries within the maxDuration?
Let's check for any retry-related documentation or tests:
✅ Verification successful
Retry configuration is compatible with maxDuration
The retry settings (4 attempts with exponential backoff) would use at most 150 seconds for retries, leaving plenty of room within the 3600-second maxDuration for task execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for retry-related documentation and tests rg -l "retry.*maxDuration|maxDuration.*retry" \ --type md \ --type ts \ --type testLength of output: 108
Script:
#!/bin/bash # Get the retry configuration from the trigger file cat references/bun-catalog/trigger.config.ts # Search for retry and maxDuration documentation rg -i "retry.*duration|duration.*retry" \ --type ts \ --type md \ -A 3 -B 3 # Find similar configurations fd -e ts -e js "config" -x rg -l "maxDuration.*retry|retry.*maxDuration|retries.*timeout"Length of output: 5520
references/v3-catalog/trigger.config.ts (1)
20-20: LGTM! The maxDuration setting is appropriate.The 1-hour (3600 seconds) maximum duration is a reasonable default that should accommodate most task execution times while preventing runaway processes.
packages/core/src/v3/config.ts (1)
119-119: LGTM! The maxDuration property is now required with clear documentation.The type definition change enforces the requirement for maxDuration while maintaining comprehensive documentation that clearly explains its purpose, constraints, and minimum value.
packages/cli-v3/src/commands/login.ts (3)
26-29: LGTM! Clean import organization.The imports are well-organized and clearly indicate the new token validation functionality.
91-95: LGTM! Proper validation of environment-based tokens.The validation ensures that environment-based tokens are personal access tokens and provides a helpful error message with guidance on how to obtain a valid token.
305-308: LGTM! Appropriate error handling in embedded mode.The error handling correctly propagates token validation errors in embedded mode while maintaining the existing error handling behavior.
.changeset/tiny-seals-appear.md (1)
1-6: LGTM! Changeset correctly describes the changes.The patch version bumps are appropriate for these non-breaking enhancements.
packages/cli-v3/templates/trigger.config.ts.template (1)
7-10: Configuration is consistent with MJS template.The TypeScript template maintains consistency with the JavaScript template, which is good. Please apply the same documentation improvement suggested in the MJS template review.
| if (!config.maxDuration) { | ||
| throw new Error( | ||
| `The "maxDuration" trigger.config option is now required, and must be at least 5 seconds.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the minimum maxDuration value.
While the validation ensures maxDuration is present, it should also verify that the value is at least 5 seconds as mentioned in the error message and documentation.
if (!config.maxDuration) {
throw new Error(
`The "maxDuration" trigger.config option is now required, and must be at least 5 seconds.`
);
}
+if (config.maxDuration < 5) {
+ throw new Error(
+ `The "maxDuration" trigger.config option must be at least 5 seconds, got ${config.maxDuration}.`
+ );
+}📝 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.
| if (!config.maxDuration) { | |
| throw new Error( | |
| `The "maxDuration" trigger.config option is now required, and must be at least 5 seconds.` | |
| ); | |
| } | |
| if (!config.maxDuration) { | |
| throw new Error( | |
| `The "maxDuration" trigger.config option is now required, and must be at least 5 seconds.` | |
| ); | |
| } | |
| if (config.maxDuration < 5) { | |
| throw new Error( | |
| `The "maxDuration" trigger.config option must be at least 5 seconds, got ${config.maxDuration}.` | |
| ); | |
| } |
ericallam
force-pushed
the
require-max-duration
branch
from
ba565be to
2626eb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/cli-v3/src/commands/login.ts (3)
91-95: Improve error message readability.While the validation logic is well-placed, the error message could be more concise and readable.
Consider using template literals for better readability:
- throw new NotPersonalAccessTokenError( - "Your TRIGGER_ACCESS_TOKEN is not a Personal Access Token, they start with 'tr_pat_'. You can generate one here: https://cloud.trigger.dev/account/tokens" - ); + throw new NotPersonalAccessTokenError( + `Invalid TRIGGER_ACCESS_TOKEN format. Expected a Personal Access Token starting with 'tr_pat_'. + Generate a new token at: https://cloud.trigger.dev/account/tokens` + );
305-308: Consider consistent error handling across modes.While the specific handling of
NotPersonalAccessTokenErrorin embedded mode is good, consider applying similar specific handling in non-embedded mode for consistency.Consider handling the error specifically in both modes:
if (options?.embedded) { if (e instanceof NotPersonalAccessTokenError) { throw e; } return { ok: false as const, error: e instanceof Error ? e.message : String(e), }; + } else if (e instanceof NotPersonalAccessTokenError) { + outro(e.message); + throw new SkipLoggingError(e.message); } throw e;
Line range hint
91-308: Overall implementation effectively validates PAT format.The changes successfully implement the validation of Personal Access Tokens with appropriate error handling and user guidance. The implementation is solid, with only minor suggestions for improvement in error message formatting and handling consistency.
Consider documenting the PAT format requirements in the project's API documentation to help users understand the token requirements before they encounter validation errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.changeset/tiny-seals-appear.md(1 hunks)packages/cli-v3/e2e/fixtures/emit-decorator-metadata/trigger.config.ts(1 hunks)packages/cli-v3/e2e/fixtures/esm-only-external/trigger.config.ts(1 hunks)packages/cli-v3/e2e/fixtures/hello-world/trigger.config.ts(1 hunks)packages/cli-v3/e2e/fixtures/monorepo-react-email/packages/trigger/trigger.config.ts(1 hunks)packages/cli-v3/e2e/fixtures/otel-telemetry-loader/trigger.config.ts(1 hunks)packages/cli-v3/src/commands/login.ts(3 hunks)packages/cli-v3/src/config.ts(1 hunks)packages/cli-v3/src/utilities/isPersonalAccessToken.ts(1 hunks)packages/cli-v3/templates/trigger.config.mjs.template(1 hunks)packages/cli-v3/templates/trigger.config.ts.template(1 hunks)packages/core/src/v3/config.ts(1 hunks)references/bun-catalog/trigger.config.ts(1 hunks)references/nextjs-realtime/trigger.config.ts(1 hunks)references/v3-catalog/trigger.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- .changeset/tiny-seals-appear.md
- packages/cli-v3/e2e/fixtures/monorepo-react-email/packages/trigger/trigger.config.ts
- references/nextjs-realtime/trigger.config.ts
- packages/cli-v3/e2e/fixtures/otel-telemetry-loader/trigger.config.ts
- packages/cli-v3/e2e/fixtures/esm-only-external/trigger.config.ts
- references/bun-catalog/trigger.config.ts
- packages/cli-v3/src/config.ts
- packages/cli-v3/templates/trigger.config.mjs.template
- packages/cli-v3/e2e/fixtures/hello-world/trigger.config.ts
- packages/cli-v3/templates/trigger.config.ts.template
- packages/cli-v3/src/utilities/isPersonalAccessToken.ts
- references/v3-catalog/trigger.config.ts
- packages/core/src/v3/config.ts
- packages/cli-v3/e2e/fixtures/emit-decorator-metadata/trigger.config.ts
🔇 Additional comments (1)
packages/cli-v3/src/commands/login.ts (1)
26-29: LGTM! Clean import organization.The new imports for token validation are well-organized and properly grouped with related functionality.
@trigger.dev/core
@trigger.dev/build
@trigger.dev/rsc
@trigger.dev/sdk
trigger.dev
@trigger.dev/react-hooks
commit: |
Require max duration
We now require a
maxDurationto be set in yourtrigger.configfile.initcommand templates now include a 1hr max duration by default, with better comments explaining it.Validate
TRIGGER_ACCESS_TOKENWhen deploying using GitHub Actions (or some other CI system) you need to set the
TRIGGER_ACCESS_TOKENenv var to a Personal Access Token.Sometimes users put a secret key in here instead, which doesn't work… Now we detect that it's not a token (starts with
tr_pat_) and link you to the page to create one.Summary by CodeRabbit
New Features
maxDurationconfiguration parameter to control maximum task execution time.Bug Fixes
Documentation
maxDurationsetting.Chores
maxDurationset to 3600 seconds.maxDurationa mandatory configuration option.