feat: background tasks #1160
feat: background tasks #1160
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
WalkthroughThe recent changes primarily focus on enhancing job management by integrating a new job provider with configurations, refining email dispatching through job queues, and updating various dependencies and scripts. Configuration settings were added across multiple files for the seamless functioning of background jobs, reflecting a solid move towards streamlining job executions within the system. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant JobHandler
participant EmailService
User->>App: Signup Request
App->>JobHandler: Trigger Confirmation Job
JobHandler->>EmailService: Send Confirmation Email
EmailService-->>User: Receive Confirmation Email
User->>App: Request Email Verification
App->>JobHandler: Trigger Email Verification Job
JobHandler->>EmailService: Send Verification Email
EmailService-->>User: Receive Verification Email
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
apps: web
|
Thank you for following the naming conventions for pull request titles! 💚🚀 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/lib/jobs/client/local.ts
Outdated
|
|
||
| public getApiHandler() { | ||
| return async (req: NextApiRequest, res: NextApiResponse) => { | ||
| if (req.method === 'POST') { |
There was a problem hiding this comment.
Feels like you should do an early return here for non POST methods to reduce nesting
| @@ -200,7 +200,12 @@ export const profileRouter = router({ | |||
| try { | |||
| const { email } = input; | |||
|
|
|||
| return await sendConfirmationToken({ email }); | |||
| await jobsClient.triggerJob({ | |||
| name: 'send.confirmation.email', | |||
There was a problem hiding this comment.
I think we should add one more layer since confirmation email is a little too generic
send.signup.confirmation.email
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (1)
packages/lib/jobs/client/inngest.ts (1)
39-65: Optimization in Job DefinitionWhen defining a job, logging is performed which might not be necessary in production environments. Consider adding a conditional check to enable logging only in development mode to avoid cluttering production logs.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (32)
- .env.example (1 hunks)
- .vscode/settings.json (1 hunks)
- apps/marketing/package.json (1 hunks)
- apps/web/package.json (1 hunks)
- apps/web/src/pages/api/jobs/[[...handler]].ts (1 hunks)
- docker/development/compose.yml (2 hunks)
- package.json (3 hunks)
- packages/ee/package.json (1 hunks)
- packages/lib/jobs/client.ts (1 hunks)
- packages/lib/jobs/client/_internal/job.ts (1 hunks)
- packages/lib/jobs/client/_internal/json.ts (1 hunks)
- packages/lib/jobs/client/base.ts (1 hunks)
- packages/lib/jobs/client/client.ts (1 hunks)
- packages/lib/jobs/client/inngest.ts (1 hunks)
- packages/lib/jobs/client/local.ts (1 hunks)
- packages/lib/jobs/client/trigger.ts (1 hunks)
- packages/lib/jobs/definitions/send-confirmation-email.ts (1 hunks)
- packages/lib/jobs/definitions/send-signing-email.ts (1 hunks)
- packages/lib/next-auth/auth-options.ts (2 hunks)
- packages/lib/package.json (2 hunks)
- packages/lib/server-only/document/send-document.tsx (3 hunks)
- packages/lib/server-only/user/verify-email.ts (2 hunks)
- packages/lib/universal/extract-request-metadata.ts (1 hunks)
- packages/prisma/migrations/20240516022236_add_background_job_models/migration.sql (1 hunks)
- packages/prisma/migrations/20240516030441_add_more_timestamp_columns_for_background_jobs/migration.sql (1 hunks)
- packages/prisma/migrations/20240516032317_capture_payload_for_background_job/migration.sql (1 hunks)
- packages/prisma/migrations/20240516034546_update_background_job_task_model/migration.sql (1 hunks)
- packages/prisma/schema.prisma (1 hunks)
- packages/trpc/server/auth-router/router.ts (3 hunks)
- packages/trpc/server/profile-router/router.ts (2 hunks)
- packages/tsconfig/process-env.d.ts (1 hunks)
- turbo.json (1 hunks)
Files skipped from review due to trivial changes (7)
- apps/marketing/package.json
- apps/web/package.json
- packages/ee/package.json
- packages/lib/jobs/client/_internal/json.ts
- packages/prisma/migrations/20240516030441_add_more_timestamp_columns_for_background_jobs/migration.sql
- packages/prisma/migrations/20240516032317_capture_payload_for_background_job/migration.sql
- packages/prisma/migrations/20240516034546_update_background_job_task_model/migration.sql
Additional context used
Biome
packages/lib/jobs/client/base.ts
[error] 16-16: void is confusing inside a union type. (lint/suspicious/noConfusingVoidType)
Unsafe fix: Use undefined instead.
packages/lib/jobs/client/_internal/job.ts
[error] 39-39: void is confusing inside a union type. (lint/suspicious/noConfusingVoidType)
Unsafe fix: Use undefined instead.
[error] 44-44: void is confusing inside a union type. (lint/suspicious/noConfusingVoidType)
Unsafe fix: Use undefined instead.
packages/lib/jobs/client/trigger.ts
[error] 20-20: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 27-27: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 30-30: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.packages/lib/jobs/client/inngest.ts
[error] 27-27: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 33-33: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 36-36: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.packages/lib/jobs/client/local.ts
[error] 31-31: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 32-32: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 35-35: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 260-260: void is confusing inside a union type. (lint/suspicious/noConfusingVoidType)
Unsafe fix: Use undefined instead.
packages/lib/next-auth/auth-options.ts
[error] 348-348: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Gitleaks
.env.example
74-75: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches. (hashicorp-tf-password)
123-124: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches. (hashicorp-tf-password)
Additional comments not posted (30)
apps/web/src/pages/api/jobs/[[...handler]].ts (2)
3-8: Ensure appropriate API handler configuration.The configuration disables the
bodyParserand sets amaxDurationof 300 seconds. Ensure that these settings align with the expected load and security considerations.
10-10: LGTM!The default export of
jobsClient.getApiHandler()looks good. Ensure thatgetApiHandleris robustly implemented to handle API requests correctly.packages/lib/jobs/client.ts (1)
5-12: LGTM!The initialization of
jobsClientwith job definitions usingas constensures type safety and correctness in job handling..vscode/settings.json (2)
8-13: Enhanced ESLint validation settings are beneficial.The inclusion of TypeScript and JavaScript React files in ESLint validation helps maintain high code quality across the project.
19-22: Prisma formatter setting is a good practice.Setting Prisma as the default formatter for
.prismafiles ensures consistency in database schema definitions.
[APROVED]packages/lib/jobs/definitions/send-confirmation-email.ts (3)
1-1: Imports and Constants Review:All imports and constants are correctly defined and used appropriately in the job definition.
Also applies to: 3-4, 6-6
8-11: Schema Definition:The use of
zodfor defining the job schema ensures that the data types and requirements are enforced, promoting robustness in job data handling.
13-30: Job Definition:The job definition is robust and well-structured, ensuring clarity and maintainability. The handler is appropriately asynchronous, suitable for the operations it performs.
packages/lib/jobs/client/client.ts (3)
1-1: Imports and Class Definition Review:The imports are appropriate for the functionality, and the
JobClientclass is well-defined with use of TypeScript generics enhancing type safety.Also applies to: 3-9
12-21: Constructor Implementation:The use of pattern matching for provider selection in the constructor is a clean and flexible approach to dependency injection, allowing easy switching between different job providers.
23-29: Method Implementations:The methods
triggerJobandgetApiHandlerare well-implemented, delegating responsibilities to the provider and keeping the class clean and focused.packages/lib/universal/extract-request-metadata.ts (2)
8-11: Schema Definition:The
ZRequestMetadataSchemais effectively defined usingzod, ensuring optional validation for IP addresses and user agents, which enhances flexibility and robustness in metadata extraction.
13-13: Function Implementations:The functions for metadata extraction are well-implemented, utilizing the defined
zodschema to ensure consistency and reliability.packages/prisma/migrations/20240516022236_add_background_job_models/migration.sql (2)
1-2: SQL Statements for Enums and Tables:The SQL statements for creating enums and tables are correctly formatted and logically structured, ensuring data integrity and proper relational setups for background job management.
Also applies to: 4-5, 7-34
36-37: Foreign Key Constraints:The setup of foreign key constraints using cascading updates and deletes is appropriate, ensuring referential integrity and ease of maintenance.
packages/lib/package.json (2)
35-36: Update to Trigger.dev dependenciesYou've updated the versions for
@trigger.dev/nextjsand@trigger.dev/sdkto2.3.18and addedinngestat3.19.13. These updates align with the PR's objective to introduce a pluggable jobs provider. Ensure that these versions are compatible with the rest of your project dependencies to avoid any potential conflicts.Also applies to: 39-39
57-59: Addition of type definitions for new dependenciesThe addition of type definitions for
luxonandpg(@types/luxonand@types/pg) is a good practice to ensure type safety and better development experience in TypeScript. Make sure these versions match with their corresponding main packages to maintain compatibility.packages/lib/server-only/user/verify-email.ts (2)
5-5: Use of Job Client in Email VerificationImporting and using
jobsClientfrom../../jobs/clientto handle email verification through background tasks aligns well with the PR's objective to manage tasks asynchronously. This should enhance the system's responsiveness and scalability.
43-48: Triggering Email Verification JobThe implementation to trigger a job for sending a signup confirmation email when the most recent token is invalid or expired is a robust way to handle user verification. This should help in managing user experience and system performance effectively.
packages/lib/jobs/client/_internal/job.ts (1)
5-10: Type Definitions and Schema for Job OptionsThe introduction of
SimpleTriggerJobOptionsand its validation schemaZSimpleTriggerJobOptionsSchemaare crucial for ensuring that job options are correctly validated. The use of Zod for schema validation enhances type safety and error handling.Also applies to: 12-17
docker/development/compose.yml (1)
7-13: Docker Configuration for ServicesThe updates to the Docker configuration for the
databaseservice and the addition oftriggerdotdevandtriggerdotdev_databaseservices are crucial for supporting the new background job functionality. The detailed setup including health checks, environment variables, and dependency management should facilitate smooth operation and integration of these services.Also applies to: 43-56
package.json (2)
27-28: Review newly added script for potential issues.The
trigger:devscript usesnpxto run a CLI tool, which is fine, but ensure that the handler path is correctly specified and that the environment is properly configured to use this script.
Line range hint
38-59: Review new dependencies for compatibility and licensing issues.New dependencies such as
@trigger.dev/cliandreacthave been added. It's important to ensure that these dependencies are compatible with the project's existing dependencies and that their licenses do not conflict with the project's license.packages/tsconfig/process-env.d.ts (1)
71-83: Validate new environment variable declarations for consistency.The newly declared environment variables
NEXT_PRIVATE_JOBS_PROVIDER,NEXT_PRIVATE_TRIGGER_API_KEY, andNEXT_PRIVATE_TRIGGER_API_URLshould be consistently used throughout the application. Also, ensure that their naming follows the existing naming conventions.Verification successful
The new environment variables
NEXT_PRIVATE_JOBS_PROVIDER,NEXT_PRIVATE_TRIGGER_API_KEY, andNEXT_PRIVATE_TRIGGER_API_URLare consistently used in the identified files:
turbo.jsonincludes them in theglobalEnvsection.packages/lib/jobs/client/trigger.tsusesNEXT_PRIVATE_TRIGGER_API_KEYandNEXT_PRIVATE_TRIGGER_API_URLin theTriggerClientinitialization.packages/lib/jobs/client/client.tsusesNEXT_PRIVATE_JOBS_PROVIDERto determine the job provider.No inconsistencies or naming conventions issues were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of new environment variables across the project. # Expect: Consistent usage and naming conventions. rg "NEXT_PRIVATE_JOBS_PROVIDER|NEXT_PRIVATE_TRIGGER_API_KEY|NEXT_PRIVATE_TRIGGER_API_URL" --files-with-matchesLength of output: 229
Script:
#!/bin/bash # Extract the relevant sections from the identified files to verify the usage of the environment variables. # Check turbo.json for environment variable usage echo "Contents of turbo.json:" cat turbo.json # Check packages/lib/jobs/client/trigger.ts for environment variable usage echo -e "\n\nContents of packages/lib/jobs/client/trigger.ts:" cat packages/lib/jobs/client/trigger.ts # Check packages/lib/jobs/client/client.ts for environment variable usage echo -e "\n\nContents of packages/lib/jobs/client/client.ts:" cat packages/lib/jobs/client/client.tsLength of output: 7473
turbo.json (1)
118-121: Ensure new global environment variables are necessary and correctly used.The newly added global environment variables related to job providers and Trigger.dev should be reviewed to ensure they are necessary and correctly used across the project.
Verification successful
The new global environment variables
NEXT_PRIVATE_JOBS_PROVIDER,NEXT_PRIVATE_TRIGGER_API_KEY,NEXT_PRIVATE_TRIGGER_API_URL, andNEXT_PRIVATE_INNGEST_EVENT_KEYare indeed used in the codebase:
packages/tsconfig/process-env.d.ts: These variables are defined with their expected types.packages/lib/jobs/client/inngest.ts:NEXT_PRIVATE_INNGEST_EVENT_KEYis used.packages/lib/jobs/client/client.ts:NEXT_PRIVATE_JOBS_PROVIDERis used.packages/lib/jobs/client/trigger.ts:NEXT_PRIVATE_TRIGGER_API_KEYandNEXT_PRIVATE_TRIGGER_API_URLare used.These findings confirm that the variables are necessary and correctly used across the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the necessity and correct usage of new global environment variables. # Expect: Correct and necessary usage. rg "NEXT_PRIVATE_JOBS_PROVIDER|NEXT_PRIVATE_TRIGGER_API_KEY|NEXT_PRIVATE_TRIGGER_API_URL|NEXT_PRIVATE_INNGEST_EVENT_KEY" --files-with-matchesLength of output: 296
Script:
#!/bin/bash # Description: Inspect the usage of new global environment variables in the identified files. # Check usage in packages/tsconfig/process-env.d.ts echo "Usage in packages/tsconfig/process-env.d.ts:" rg "NEXT_PRIVATE_JOBS_PROVIDER|NEXT_PRIVATE_TRIGGER_API_KEY|NEXT_PRIVATE_TRIGGER_API_URL|NEXT_PRIVATE_INNGEST_EVENT_KEY" packages/tsconfig/process-env.d.ts # Check usage in packages/lib/jobs/client/inngest.ts echo "Usage in packages/lib/jobs/client/inngest.ts:" rg "NEXT_PRIVATE_JOBS_PROVIDER|NEXT_PRIVATE_TRIGGER_API_KEY|NEXT_PRIVATE_TRIGGER_API_URL|NEXT_PRIVATE_INNGEST_EVENT_KEY" packages/lib/jobs/client/inngest.ts # Check usage in packages/lib/jobs/client/client.ts echo "Usage in packages/lib/jobs/client/client.ts:" rg "NEXT_PRIVATE_JOBS_PROVIDER|NEXT_PRIVATE_TRIGGER_API_KEY|NEXT_PRIVATE_TRIGGER_API_URL|NEXT_PRIVATE_INNGEST_EVENT_KEY" packages/lib/jobs/client/client.ts # Check usage in packages/lib/jobs/client/trigger.ts echo "Usage in packages/lib/jobs/client/trigger.ts:" rg "NEXT_PRIVATE_JOBS_PROVIDER|NEXT_PRIVATE_TRIGGER_API_KEY|NEXT_PRIVATE_TRIGGER_API_URL|NEXT_PRIVATE_INNGEST_EVENT_KEY" packages/lib/jobs/client/trigger.tsLength of output: 1475
packages/lib/server-only/document/send-document.tsx (1)
129-137: Use of Jobs Client for Email SendingThe implementation uses
jobsClientto trigger email sending jobs. This is a good use of background tasks for non-critical operations. Ensure that the job definitions and the payload structure are correctly aligned with what the job expects..env.example (1)
106-111: Security Risk in API Key ManagementThe environment variables related to API keys (
NEXT_PRIVATE_TRIGGER_API_KEY,NEXT_PRIVATE_INNGEST_EVENT_KEY) are set but not provided values in the.env.example. Ensure that these keys are managed securely and not exposed in version control.packages/lib/jobs/definitions/send-signing-email.ts (1)
37-167: Comprehensive Email Sending Job DefinitionThe job definition for sending signing emails is comprehensive and well-structured. It uses various utilities and services to prepare and send the email. Ensure all used services like
mailerandprismaare robust and handle errors appropriately.packages/prisma/schema.prisma (2)
616-621: Enum definition forBackgroundJobStatuslooks good.The enum values appropriately cover the typical states of a background job lifecycle.
643-647: Enum definition forBackgroundJobTaskStatuslooks good.The enum values are consistent and clearly denote the status of tasks within a background job.
| await jobsClient.triggerJob({ | ||
| name: 'send.signup.confirmation.email', | ||
| payload: { | ||
| email, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Refactor: Use enhanced job name for clarity and consistency.
The job name send.signup.confirmation.email could be more descriptive and aligned with the application's naming conventions. Based on previous discussions, consider renaming it to enhance clarity and consistency across the codebase.
- name: 'send.signup.confirmation.email',
+ name: 'send.user.signup.confirmation.email',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.
| await jobsClient.triggerJob({ | |
| name: 'send.signup.confirmation.email', | |
| payload: { | |
| email, | |
| }, | |
| }); | |
| await jobsClient.triggerJob({ | |
| name: 'send.user.signup.confirmation.email', | |
| payload: { | |
| email, | |
| }, | |
| }); |
| await jobsClient.triggerJob({ | ||
| name: 'send.signup.confirmation.email', | ||
| payload: { | ||
| email: user.email, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Refactor: Use enhanced job name for clarity and consistency.
Similar to the previous file, consider using a more descriptive job name for the email sending job. This helps in maintaining a consistent naming convention throughout your application.
- name: 'send.signup.confirmation.email',
+ name: 'send.user.signup.confirmation.email',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.
| await jobsClient.triggerJob({ | |
| name: 'send.signup.confirmation.email', | |
| payload: { | |
| email: user.email, | |
| }, | |
| }); | |
| await jobsClient.triggerJob({ | |
| name: 'send.user.signup.confirmation.email', | |
| payload: { | |
| email: user.email, | |
| }, | |
| }); |
| static getInstance() { | ||
| if (!this._instance) { | ||
| this._instance = new LocalJobProvider(); | ||
| } | ||
|
|
||
| return this._instance; | ||
| } |
There was a problem hiding this comment.
Clarification: Use class name instead of this in static context.
Using this in static methods can lead to confusion as it does not behave as it might in non-static contexts. Consider using the class name directly for clarity.
- if (!this._instance) {
- this._instance = new LocalJobProvider();
- }
- return this._instance;
+ if (!LocalJobProvider._instance) {
+ LocalJobProvider._instance = new LocalJobProvider();
+ }
+ return LocalJobProvider._instance;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.
| static getInstance() { | |
| if (!this._instance) { | |
| this._instance = new LocalJobProvider(); | |
| } | |
| return this._instance; | |
| } | |
| static getInstance() { | |
| if (!LocalJobProvider._instance) { | |
| LocalJobProvider._instance = new LocalJobProvider(); | |
| } | |
| return LocalJobProvider._instance; | |
| } |
Tools
Biome
[error] 31-31: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 32-32: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 35-35: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
|
|
||
| private createJobRunIO(jobId: string): JobRunIO { | ||
| return { | ||
| runTask: async <T extends void | Json>(cacheKey: string, callback: () => Promise<T>) => { |
There was a problem hiding this comment.
Refactor: Replace void with undefined in union type.
Using void in union types can be confusing and misleading. Replace it with undefined for better clarity and to avoid potential issues in type checking.
- runTask: async <T extends void | Json>(cacheKey: string, callback: () => Promise<T>) => {
+ runTask: async <T extends undefined | Json>(cacheKey: string, callback: () => Promise<T>) => {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.
| runTask: async <T extends void | Json>(cacheKey: string, callback: () => Promise<T>) => { | |
| runTask: async <T extends undefined | Json>(cacheKey: string, callback: () => Promise<T>) => { |
Tools
Biome
[error] 260-260: void is confusing inside a union type. (lint/suspicious/noConfusingVoidType)
Unsafe fix: Use undefined instead.
| name: Name; | ||
| schema?: z.ZodType<Schema>; | ||
| }; | ||
| handler: (options: { payload: Schema; io: JobRunIO }) => Promise<Json | void>; |
There was a problem hiding this comment.
Void Type in Union Warning
The static analysis tool flagged the use of void in a union type as potentially confusing. Consider replacing void with undefined to clarify the intent and avoid any ambiguity.
- handler: (options: { payload: Schema; io: JobRunIO }) => Promise<Json | void>;
+ handler: (options: { payload: Schema; io: JobRunIO }) => Promise<Json | undefined>;Also applies to: 44-44
Tools
Biome
[error] 39-39: void is confusing inside a union type. (lint/suspicious/noConfusingVoidType)
Unsafe fix: Use undefined instead.
| public async triggerJob(options: SimpleTriggerJobOptions): Promise<void> { | ||
| await this._client.sendEvent({ | ||
| id: options.id, | ||
| name: options.name, | ||
| payload: options.payload, | ||
| timestamp: options.timestamp ? new Date(options.timestamp) : undefined, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Consider handling errors in triggerJob.
The method triggerJob does not handle errors that might occur during sendEvent. It's good practice to catch such errors to prevent them from crashing the application.
- await this._client.sendEvent({
+ try {
+ await this._client.sendEvent({
id: options.id,
name: options.name,
payload: options.payload,
timestamp: options.timestamp ? new Date(options.timestamp) : undefined,
});
+ } catch (error) {
+ // Handle error appropriately
+ }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.
| public async triggerJob(options: SimpleTriggerJobOptions): Promise<void> { | |
| await this._client.sendEvent({ | |
| id: options.id, | |
| name: options.name, | |
| payload: options.payload, | |
| timestamp: options.timestamp ? new Date(options.timestamp) : undefined, | |
| }); | |
| } | |
| public async triggerJob(options: SimpleTriggerJobOptions): Promise<void> { | |
| try { | |
| await this._client.sendEvent({ | |
| id: options.id, | |
| name: options.name, | |
| payload: options.payload, | |
| timestamp: options.timestamp ? new Date(options.timestamp) : undefined, | |
| }); | |
| } catch (error) { | |
| // Handle error appropriately | |
| } | |
| } |
| if (!this._instance) { | ||
| const client = new TriggerClient({ | ||
| id: 'documenso-app', | ||
| apiKey: process.env.NEXT_PRIVATE_TRIGGER_API_KEY, | ||
| apiUrl: process.env.NEXT_PRIVATE_TRIGGER_API_URL, | ||
| }); | ||
|
|
||
| this._instance = new TriggerJobProvider({ client }); | ||
| } | ||
|
|
||
| return this._instance; |
There was a problem hiding this comment.
Refactor static method to avoid confusion with this.
Static methods should not use this to refer to the class instance as it can lead to confusion and bugs if the class is subclassed. Use the class name TriggerJobProvider instead.
- if (!this._instance) {
+ if (!TriggerJobProvider._instance) {
- this._instance = new TriggerJobProvider({ client });
+ TriggerJobProvider._instance = new TriggerJobProvider({ client });
- return this._instance;
+ return TriggerJobProvider._instance;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 (!this._instance) { | |
| const client = new TriggerClient({ | |
| id: 'documenso-app', | |
| apiKey: process.env.NEXT_PRIVATE_TRIGGER_API_KEY, | |
| apiUrl: process.env.NEXT_PRIVATE_TRIGGER_API_URL, | |
| }); | |
| this._instance = new TriggerJobProvider({ client }); | |
| } | |
| return this._instance; | |
| if (!TriggerJobProvider._instance) { | |
| const client = new TriggerClient({ | |
| id: 'documenso-app', | |
| apiKey: process.env.NEXT_PRIVATE_TRIGGER_API_KEY, | |
| apiUrl: process.env.NEXT_PRIVATE_TRIGGER_API_URL, | |
| }); | |
| TriggerJobProvider._instance = new TriggerJobProvider({ client }); | |
| } | |
| return TriggerJobProvider._instance; |
Tools
Biome
[error] 20-20: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 27-27: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 30-30: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
| public defineJob<N extends string, T>(job: JobDefinition<N, T>): void { | ||
| this._client.defineJob({ | ||
| id: job.id, | ||
| name: job.name, | ||
| version: job.version, | ||
| trigger: eventTrigger({ | ||
| name: job.trigger.name, | ||
| schema: job.trigger.schema, | ||
| }), | ||
| run: async (payload, io) => job.handler({ payload, io: this.convertTriggerIoToJobRunIo(io) }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The inspection of the surrounding code in both trigger.ts and inngest.ts shows that there are no error handling mechanisms (like try-catch blocks) around the job.handler invocations.
packages/lib/jobs/client/trigger.ts: Thejob.handleris invoked without any error handling.packages/lib/jobs/client/inngest.ts: Similarly, thejob.handleris invoked without any error handling.
This confirms the review comment's concern about the lack of error handling in the defineJob method.
- trigger.ts: Line 44
- inngest.ts: Line 44
Analysis chain
Ensure proper error handling in asynchronous job definition.
The method defineJob uses an asynchronous callback but does not handle potential exceptions that might be thrown by job.handler. Consider adding try-catch blocks or error handling mechanisms.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if error handling is implemented in other parts of the codebase for job handlers.
# Expect: Error handling mechanisms around job handler invocations.
rg --type ts "job.handler" --files-with-matches
Length of output: 119
Script:
#!/bin/bash
# Inspect the usage of job.handler in trigger.ts for error handling mechanisms
# Extract the lines around job.handler in trigger.ts
rg --type ts --context 5 "job.handler" packages/lib/jobs/client/trigger.ts
# Extract the lines around job.handler in inngest.ts
rg --type ts --context 5 "job.handler" packages/lib/jobs/client/inngest.ts
Length of output: 707
| public getApiHandler() { | ||
| const { handler } = createPagesRoute(this._client); | ||
|
|
||
| return handler; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The getApiHandler method is used across several files with varying levels of error handling and customization. Notably:
local.ts: Includes error handling for HTTP methods.client.ts: Delegates to the provider'sgetApiHandler.base.ts: Throws a "Not implemented" error.[[...handler]].ts: Used as a default export without additional customization or error handling.trigger.tsandinngest.ts: Lack error handling and customization.
To ensure consistency and robustness, consider adding error handling and customization options in trigger.ts and inngest.ts.
packages/lib/jobs/client/trigger.tspackages/lib/jobs/client/inngest.ts
Analysis chain
Review API handler generation logic.
The method getApiHandler retrieves an API handler but does not provide any customization or error handling capabilities. Consider whether customization options or error handling might be necessary based on the usage context.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how `getApiHandler` is used in the application to determine if additional customization or error handling is needed.
# Expect: Usage patterns of `getApiHandler`.
rg "getApiHandler" --files-with-matches
Length of output: 259
Script:
#!/bin/bash
# Extract lines where `getApiHandler` is used in each of the identified files.
files=(
"packages/lib/jobs/client/trigger.ts"
"packages/lib/jobs/client/local.ts"
"packages/lib/jobs/client/inngest.ts"
"packages/lib/jobs/client/client.ts"
"packages/lib/jobs/client/base.ts"
"apps/web/src/pages/api/jobs/[[...handler]].ts"
)
for file in "${files[@]}"; do
echo "File: $file"
rg "getApiHandler" "$file" -A 5 -B 5
done
Length of output: 2635
| public getApiHandler(): (req: NextApiRequest, res: NextApiResponse) => Promise<Response | void> { | ||
| throw new Error('Not implemented'); | ||
| } |
There was a problem hiding this comment.
Clarify the return type of getApiHandler.
The method getApiHandler has a return type that includes void, which can be confusing. Consider using undefined instead to improve clarity.
- public getApiHandler(): (req: NextApiRequest, res: NextApiResponse) => Promise<Response | void> {
+ public getApiHandler(): (req: NextApiRequest, res: NextApiResponse) => Promise<Response | undefined> {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.
| public getApiHandler(): (req: NextApiRequest, res: NextApiResponse) => Promise<Response | void> { | |
| throw new Error('Not implemented'); | |
| } | |
| public getApiHandler(): (req: NextApiRequest, res: NextApiResponse) => Promise<Response | undefined> { | |
| throw new Error('Not implemented'); | |
| } |
Tools
Biome
[error] 16-16: void is confusing inside a union type. (lint/suspicious/noConfusingVoidType)
Unsafe fix: Use undefined instead.
Description
Currently a work in progress but this adds background tasks to Documenso for handling tasks that take a little longer and aren't part of the overall critical path.
Related Issue
N/A
Changes Made
Added a pluggable jobs provider supporting both a local provider and a Trigger.dev provider.
Testing Performed
This will still have a lot of rough edges but is a perfect MVP for local durable compute which may accept more providers later on
Summary by CodeRabbit
New Features
Chores
Refactor
Documentation
Configuration
Database