Feat: Add approval and approval approver to openapi spec and get-job-status action
#176
Conversation
WalkthroughThe pull request introduces significant enhancements to the job management functionality within the API and SDK. Key changes include the addition of an Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (9)
packages/node-sdk/src/models/GetJob200ResponseCausedBy.ts (2)
22-41: Consider adding validations in OpenAPI spec.The interface looks good, but consider enhancing the OpenAPI specification with:
- Email format validation for the
- ID format specification for the
idfield- Consider if any fields should be optional
1-86: Overall implementation looks good with some considerations.Since this is an auto-generated file, most suggestions should be addressed in the OpenAPI specification rather than modifying this file directly. The implementation follows standard patterns and correctly implements the required functionality.
Consider:
- Adding validations in the OpenAPI spec
- Documenting the null handling behavior
- Adding test cases for edge cases in the consuming code
packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts (1)
15-15: Remove unused import.The
mapValuesimport from runtime is not used in this file.-import { mapValues } from "../runtime";packages/node-sdk/src/models/GetJob200ResponseApproval.ts (2)
16-16: Remove unused importmapValuesThe
mapValuesimport from runtime is not used in this file.-import { mapValues } from "../runtime";
52-56: Consider using uppercase for enum valuesWhile the current implementation works, consider using uppercase for enum values to follow common TypeScript conventions and make them more distinguishable from regular variables.
export const GetJob200ResponseApprovalStatusEnum = { - Pending: "pending", - Approved: "approved", - Rejected: "rejected", + PENDING: "pending", + APPROVED: "approved", + REJECTED: "rejected", } as const;integrations/github-get-job-inputs/src/index.ts (1)
90-96: Consider consistent formatting for output settingsWhile the logic is correct, consider using single-line format for
approval_approver_emailto maintain consistency with other similar output settings:- setOutputAndLog( - "approval_approver_email", - approval.approver?.email ?? "", - ); + setOutputAndLog("approval_approver_email", approval.approver?.email ?? "");packages/node-sdk/src/models/GetJob200Response.ts (1)
193-195: Consider adding type safety for the 'variables' property.While the JSON conversion for the new properties is well-implemented, the
variablesproperty is typed as a genericobject. Consider defining a more specific type or interface for better type safety.Example improvement:
export interface JobVariables { [key: string]: string | number | boolean | null; }Also applies to: 212-213
openapi.v1.yaml (1)
405-431: Consider adding dedicated approval management endpoints.Since you're introducing an approval system, consider adding dedicated endpoints for:
- Requesting approvals
- Approving/rejecting jobs
- Listing pending approvals
- Retrieving approval history
This would provide a more complete API surface for managing the approval workflow.
packages/api/src/router/job.ts (1)
108-109: Consider enhancing type safety for the approval object.The approval data integration looks good, but we could improve type safety by extracting the approval object type.
Consider creating a dedicated type for the approval object:
type JobApproval = { environmentPolicyApproval: EnvironmentPolicyApproval; approver: User | null; };Then update the return type:
- approval: v[0]!.environment_policy_approval - ? { - ...v[0]!.environment_policy_approval, - approver: v[0]!.approver, - } - : null, + approval: v[0]!.environment_policy_approval + ? { + ...v[0]!.environment_policy_approval, + approver: v[0]!.approver, + } as JobApproval + : null,Also applies to: 150-155
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
- github/get-job-inputs/index.js (6 hunks)
- integrations/github-get-job-inputs/src/index.ts (2 hunks)
- openapi.v1.yaml (1 hunks)
- packages/api/src/router/job.ts (6 hunks)
- packages/node-sdk/openapitools.json (1 hunks)
- packages/node-sdk/src/.openapi-generator/FILES (1 hunks)
- packages/node-sdk/src/models/GetJob200Response.ts (5 hunks)
- packages/node-sdk/src/models/GetJob200ResponseApproval.ts (1 hunks)
- packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts (1 hunks)
- packages/node-sdk/src/models/GetJob200ResponseCausedBy.ts (1 hunks)
- packages/node-sdk/src/models/index.ts (1 hunks)
🔥 Files not summarized due to errors (1)
- github/get-job-inputs/index.js: Error: Server error: no LLM provider could handle the message
🔇 Additional comments (21)
packages/node-sdk/openapitools.json (1)
10-10: LGTM! Verify the new OpenAPI spec file exists.The path update to use versioned OpenAPI spec aligns with API versioning best practices.
Let's verify the existence of the new spec file:
✅ Verification successful
OpenAPI spec file exists and is properly versioned ✓
The new OpenAPI spec file
openapi.v1.yamlexists at the expected location and declares OpenAPI version 3.1.0, which is the latest stable version. The path update inopenapitools.jsonis valid and correctly configured.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the new OpenAPI spec file and its version declaration # Test 1: Check if the new spec file exists if [ -f "openapi.v1.yaml" ]; then echo "✓ openapi.v1.yaml exists" else echo "✗ openapi.v1.yaml not found" fi # Test 2: Verify the OpenAPI version declaration rg -A 2 "^openapi:" "openapi.v1.yaml"Length of output: 169
packages/node-sdk/src/.openapi-generator/FILES (1)
9-11: LGTM! The new model files align with the approval feature.The addition of approval-related model files (
GetJob200ResponseApproval.ts,GetJob200ResponseApprovalApprover.ts, andGetJob200ResponseCausedBy.ts) correctly reflects the new API structure for job approvals.Let's verify that all the required model files are properly generated:
✅ Verification successful
Let me gather more information to verify the exports in the index file since the previous results didn't show the actual content.
✅ All generated model files are properly integrated
The verification confirms that:
- All three new model files exist in the correct location
- The models are correctly exported in
index.ts- The parent model
GetJob200Response.tsproperly imports the new approval and causedBy models- The file structure follows the OpenAPI generator's conventions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of the generated model files # Expected: All model files should exist and contain the expected TypeScript interfaces # Test 1: Check if all new model files exist for model in "GetJob200ResponseApproval" "GetJob200ResponseApprovalApprover" "GetJob200ResponseCausedBy"; do if ! [ -f "packages/node-sdk/src/models/${model}.ts" ]; then echo "Error: Missing model file ${model}.ts" exit 1 fi done # Test 2: Verify the models are properly exported in the index rg -l "export \* from './GetJob200Response(Approval|ApprovalApprover|CausedBy)'" packages/node-sdk/src/models/index.ts # Test 3: Verify the models are properly imported in GetJob200Response rg -l "import.*GetJob200Response(Approval|CausedBy)" packages/node-sdk/src/models/GetJob200Response.tsLength of output: 823
Script:
#!/bin/bash # Check the actual content of index.ts and GetJob200Response.ts echo "=== Content of index.ts ===" cat packages/node-sdk/src/models/index.ts | grep -A 1 "GetJob200Response" echo -e "\n=== Content of GetJob200Response.ts ===" cat packages/node-sdk/src/models/GetJob200Response.ts | grep -A 1 "import.*GetJob200Response"Length of output: 1365
packages/node-sdk/src/models/index.ts (1)
8-10: LGTM! New exports are well-organized.The new exports for approval and causation models are properly organized and follow the existing naming conventions.
Let's verify the existence of the referenced model files:
✅ Verification successful
All referenced model files exist and are properly exported
The verification confirms that all three model files are present in the expected location:
- packages/node-sdk/src/models/GetJob200ResponseApproval.ts
- packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts
- packages/node-sdk/src/models/GetJob200ResponseCausedBy.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the model files exist for the new exports # Test: Check if the model files exist fd -t f "GetJob200ResponseApproval.ts|GetJob200ResponseApprovalApprover.ts|GetJob200ResponseCausedBy.ts" packages/node-sdk/src/models/Length of output: 318
packages/node-sdk/src/models/GetJob200ResponseCausedBy.ts (2)
15-15: Verify unused importmapValues.The
mapValuesimport from runtime is never used in this file.
61-73: Verify null handling in FromJSONTyped.The function returns the input json when null, which could lead to type inconsistencies at runtime. Consider verifying how this is handled in the consuming code.
packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts (1)
1-86: Verify integration with related approval components.The interface looks well-structured, but let's verify its integration with other components.
✅ Verification successful
Based on the shell script results, I can now generate the final response as the integration is clear and well-structured:
Integration verification completed - implementation is correct
The
GetJob200ResponseApprovalApproverinterface is properly integrated within the codebase:
- Correctly exported in the models index file
- Properly imported and used in
GetJob200ResponseApproval.tsas an optional field- All necessary JSON serialization/deserialization methods are properly referenced
- No redundant or conflicting approval interfaces exist in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check how this interface is used across the codebase echo "Checking usage of GetJob200ResponseApprovalApprover..." rg -t ts "GetJob200ResponseApprovalApprover" -A 3 echo "Checking related approval interfaces..." rg -t ts "interface.*Approval" -A 3Length of output: 7937
packages/node-sdk/src/models/GetJob200ResponseApproval.ts (2)
28-47: LGTM! Well-structured interface definitionThe interface is well-documented and properly typed, with appropriate optionality for the approver field.
1-105: Verify OpenAPI specification alignmentLet's ensure this model aligns with the OpenAPI specification and related files.
✅ Verification successful
Let me check the OpenAPI specification to verify the model alignment.
Let me try a different approach to verify the model structure.
Model aligns with OpenAPI specification
The generated TypeScript model
GetJob200ResponseApprovalcorrectly matches the OpenAPI schema definition:
- The required
idfield is of type string- The required
statusfield is an enum with values: pending, approved, rejected- The optional
approverfield is an objectThe model's structure and type definitions are consistent with the API specification, and the generated code follows the expected patterns for OpenAPI-generated models.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the model matches OpenAPI spec and find related files # Test 1: Find the OpenAPI spec file echo "Looking for OpenAPI specification file..." fd -e yaml -e json openapi # Test 2: Find related approval model files echo -e "\nLooking for related approval model files..." fd "GetJob.*Approval.*\.ts$" packages/node-sdk/src/models # Test 3: Find usages of this model echo -e "\nLooking for usages of this model..." rg "GetJob200ResponseApproval" --type tsLength of output: 6632
Script:
#!/bin/bash # Let's examine the OpenAPI spec files to verify the model structure # Check the main OpenAPI spec echo "Content of openapi.v1.yaml:" cat openapi.v1.yaml | grep -A 20 "GetJob200Response" # Check the alternative spec location echo -e "\nContent of apps/webservice/src/app/api/v1/job-agents/[agentId]/queue/openapi.yaml:" cat "apps/webservice/src/app/api/v1/job-agents/[agentId]/queue/openapi.yaml" | grep -A 20 "GetJob200Response"Length of output: 408
Script:
#!/bin/bash # Let's examine the OpenAPI spec files with a broader search # First, let's check if the files exist and their content type echo "Checking OpenAPI spec files:" file openapi.v1.yaml file "apps/webservice/src/app/api/v1/job-agents/[agentId]/queue/openapi.yaml" # Let's look for approval-related schemas in both files echo -e "\nSearching for approval schemas in openapi.v1.yaml:" rg -A 10 "approval:" openapi.v1.yaml || true echo -e "\nSearching for approval schemas in queue/openapi.yaml:" rg -A 10 "approval:" "apps/webservice/src/app/api/v1/job-agents/[agentId]/queue/openapi.yaml" || true # Let's also check for any job-related response schemas echo -e "\nSearching for job response schemas:" rg -A 10 "job.*response" -i openapi.v1.yaml || true rg -A 10 "job.*response" -i "apps/webservice/src/app/api/v1/job-agents/[agentId]/queue/openapi.yaml" || trueLength of output: 1328
integrations/github-get-job-inputs/src/index.ts (2)
54-63: LGTM: Clean destructuring of new propertiesThe addition of
causedByandapprovalto the destructured response properties is well-organized and consistent with the existing pattern.
86-96: Verify documentation for new outputsThe new outputs (
caused_by_*andapproval_*) should be documented for action consumers.#!/bin/bash # Description: Check if new outputs are documented # Expected: Documentation should mention these new outputs # Look for documentation files fd -e md -e mdx . | xargs rg -l "outputs?:"Would you like me to help document these new outputs in the action's documentation?
packages/node-sdk/src/models/GetJob200Response.ts (3)
15-16: LGTM! Import statements are properly structured.The new imports follow the existing pattern and include all necessary type definitions and JSON conversion functions.
Also applies to: 23-32
153-155: LGTM! Instance validation is properly implemented.The validation checks for new required fields follow the existing pattern and ensure data integrity.
112-124: Verify the impact of making 'variables' required.The
variablesproperty has been changed from optional to required, which is a breaking change. This could affect existing consumers of the API who might not be providing this field.The new
causedByandapprovalproperties are well-documented and align with the PR objectives.openapi.v1.yaml (2)
437-439: LGTM! Required fields are properly specified.Making nullable fields required ensures a consistent response structure, which is a good practice.
391-404: LGTM! Verify consistent usage ofcausedByacross the codebase.The
causedByschema is well-structured with appropriate fields for tracking job initiators.Let's verify the consistent usage of these fields:
packages/api/src/router/job.ts (2)
6-6: LGTM: Import changes are appropriate.The new imports are necessary for implementing the approval functionality and follow the project's conventions.
Also applies to: 23-23, 29-29, 40-40
67-90: LGTM: Query changes properly integrate approval data.The implementation correctly:
- Uses left joins to handle optional approval data
- Establishes proper relationships between approvals, releases, and users
- Maintains clean separation using table aliases
Let's verify the approval data relationships:
✅ Verification successful
Query changes correctly implement approval data relationships
The schema verification confirms that the query's join conditions are properly aligned with the database schema:
- The
environmentPolicyApprovaltable has the expected foreign key relationships:
releaseIdreferences thereleasetablepolicyIdreferences theenvironmentPolicytableuserIdis correctly used for linking approversThe left joins in the query are appropriate as they match the schema design:
.leftJoin(environmentPolicyApproval, eq(environmentPolicyApproval.releaseId, release.id)) .leftJoin(approver, and( isNotNull(environmentPolicyApproval.userId), eq(environmentPolicyApproval.userId, approver.id) ) )🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the approval relationships are properly defined in the schema # Test 1: Check if environmentPolicyApproval table has the expected foreign keys rg -A 5 "environmentPolicyApproval.*createTable" # Test 2: Verify the relationship between approval and user ast-grep --pattern 'environmentPolicyApproval.*references.*user'Length of output: 117
Script:
#!/bin/bash # Let's try different approaches to find the schema definitions # Look for schema file with table definitions fd -e ts schema # Search for environmentPolicyApproval table definition using ripgrep rg -A 10 "environmentPolicyApproval\s*=\s*" # Search for any references to environmentPolicyApproval in the codebase rg -A 5 "environmentPolicyApproval" # Look for drizzle schema definitions fd drizzleLength of output: 10341
github/get-job-inputs/index.js (4)
27888-27892: LGTM! Well-defined approval status enum.The approval status enum is well-defined with clear states (pending, approved, rejected).
28154-28156: LGTM! Proper type checking for new required fields.The instance checking correctly validates the presence of new required fields: variables, causedBy, and approval.
28174-28176: LGTM! Consistent JSON conversion implementation.The JSON conversion functions properly handle all new fields, maintaining consistency with the existing pattern.
Also applies to: 28191-28193
28965-28968: LGTM! Proper null handling for approval fields.The code correctly uses optional chaining for approval.approver fields.
packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts
Outdated
Show resolved
Hide resolved
packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts
Outdated
Show resolved
Hide resolved
packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts
Outdated
Show resolved
Hide resolved
openapi.v1.yaml
Outdated
| approval: | ||
| type: object | ||
| nullable: true | ||
| properties: | ||
| id: | ||
| type: string | ||
| status: | ||
| type: string | ||
| enum: [pending, approved, rejected] | ||
| approver: | ||
| type: object | ||
| nullable: true | ||
| description: Null when status is pending, contains approver details when approved or rejected | ||
| properties: | ||
| id: | ||
| type: string | ||
| name: | ||
| type: string | ||
| email: | ||
| type: string | ||
| required: | ||
| - id | ||
| - name | ||
| required: | ||
| - id | ||
| - status |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding timestamps to track approval/rejection events.
While the approval schema is well-structured, consider adding:
approvedAtorrejectedAttimestamp to track when the approval status changedrequestedAttimestamp to track when the approval was initially requested
This would help with audit trails and time-based analytics.
Here's the suggested addition to the schema:
properties:
id:
type: string
status:
type: string
enum: [pending, approved, rejected]
+ requestedAt:
+ type: string
+ format: date-time
+ statusUpdatedAt:
+ type: string
+ format: date-time
approver:📝 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.
| approval: | |
| type: object | |
| nullable: true | |
| properties: | |
| id: | |
| type: string | |
| status: | |
| type: string | |
| enum: [pending, approved, rejected] | |
| approver: | |
| type: object | |
| nullable: true | |
| description: Null when status is pending, contains approver details when approved or rejected | |
| properties: | |
| id: | |
| type: string | |
| name: | |
| type: string | |
| email: | |
| type: string | |
| required: | |
| - id | |
| - name | |
| required: | |
| - id | |
| - status | |
| approval: | |
| type: object | |
| nullable: true | |
| properties: | |
| id: | |
| type: string | |
| status: | |
| type: string | |
| enum: [pending, approved, rejected] | |
| requestedAt: | |
| type: string | |
| format: date-time | |
| statusUpdatedAt: | |
| type: string | |
| format: date-time | |
| approver: | |
| type: object | |
| nullable: true | |
| description: Null when status is pending, contains approver details when approved or rejected | |
| properties: | |
| id: | |
| type: string | |
| name: | |
| type: string | |
| email: | |
| type: string | |
| required: | |
| - id | |
| - name | |
| required: | |
| - id | |
| - status |
There was a problem hiding this comment.
@jsbroks, I had the exact same idea about adding a statusUpdatedAt to the table so we could show in the UI "approved/rejected at Date()" I just wanted to do that in a followup PR.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Still testing this. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
github/get-job-inputs/index.js (1)
28972-28975: Consider simplifying the key sanitization regex.The current regex for sanitizing variable keys could be simplified while maintaining the same functionality.
- const sanitizedKey = key.replace(/[.\-/\s\t]+/g, "_"); + const sanitizedKey = key.replace(/[\-.\s]+/g, "_");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (6 hunks)
- github/get-job-inputs/index.js (6 hunks)
- integrations/github-get-job-inputs/src/index.ts (2 hunks)
🔥 Files not summarized due to errors (1)
- github/get-job-inputs/index.js: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (1)
- integrations/github-get-job-inputs/src/index.ts
🔇 Additional comments (8)
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (4)
9-9: LGTM! Necessary imports added for new functionality.The new imports for
environmentPolicyApprovalanduserschemas are correctly added and align with the existing import pattern.Also applies to: 19-19
31-32: Good naming improvement for clarity.Renaming
usertocallerbetter distinguishes between the authenticated user and user-related data in the response.
126-126: LGTM! Response structure updated correctly.The approval field is properly added to the response structure.
45-45: Verify index existence for optimal join performance.The left join with the user table is correctly implemented. However, we should ensure optimal query performance.
github/get-job-inputs/index.js (4)
27888-27892: LGTM: Well-defined approval status enum.The approval status enum correctly defines the three possible states: pending, approved, and rejected.
28174-28176: LGTM: Consistent JSON transformations.The JSON serialization and deserialization logic is properly implemented for the new fields.
Also applies to: 28191-28193
28945-28945: LGTM: Clean response destructuring.The destructuring properly extracts all required fields from the response.
28154-28156: Verify if these fields should be required.Making
variables,causedBy, andapprovalrequired fields in the type checker could be a breaking change. Please verify that these fields are always present in the API response.✅ Verification successful
The type checks for required fields are correct
Looking at the TypeScript interface definition in
packages/node-sdk/src/models/GetJob200Response.ts, the fieldsvariables,causedBy, andapprovalare indeed defined as required (non-optional) properties:variables: object; causedBy: GetJob200ResponseCausedBy; approval: GetJob200ResponseApproval;The type checks in the code align with the API's type definition, so this is not a breaking change but rather proper validation of the expected response structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for API response handling to verify field presence rg -A 5 "getJob.*then.*response"Length of output: 34
Script:
#!/bin/bash # Search for API response structure and usage patterns rg -l "GetJob200Response" # Look for type definitions or interfaces related to job response ast-grep --pattern 'interface GetJob200Response { $$$ }' # Search for actual API response handling rg -A 5 "getJob.*response" # Search for any tests or mock data that might show the response structure fd "test|mock" --type f --exec rg -l "GetJob.*Response"Length of output: 5407
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts (1)
17-35: Enhance interface documentation.
The JSDoc comments for the properties could be more descriptive to improve code maintainability.
/**
* Null when status is pending, contains approver details when approved or rejected
* @export
* @interface GetJob200ResponseApprovalApprover
*/
export interface GetJob200ResponseApprovalApprover {
/**
- *
+ * Unique identifier of the approver
* @type {string}
* @memberof GetJob200ResponseApprovalApprover
*/
id: string;
/**
- *
+ * Display name of the approver
* @type {string}
* @memberof GetJob200ResponseApprovalApprover
*/
name: string;
}apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
90-96: Consider adding error handling for the approval query.
While the implementation is good, consider adding try-catch error handling for the approval query to prevent potential failures from breaking the entire endpoint.
const approval =
- je.release?.id && policyId
- ? await getApprovalDetails(je.release.id, policyId)
- : null;
+ je.release?.id && policyId
+ ? await getApprovalDetails(je.release.id, policyId).catch((error) => {
+ console.error('Failed to fetch approval details:', error);
+ return null;
+ })
+ : null;Also applies to: 126-126
github/get-job-inputs/index.js (1)
27885-27918: Consider adding JSDoc comments for better documentation.
The approval model implementation is solid with proper enum values and optional approver handling. Consider adding JSDoc comments to document the purpose of each field and their possible values.
+/**
+ * Represents the approval status and details for a job
+ * @property {string} id - Unique identifier for the approval
+ * @property {('pending'|'approved'|'rejected')} status - Current approval status
+ * @property {GetJob200ResponseApprovalApprover} [approver] - Details of the approver if approved/rejected
+ */📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (4 hunks)
- github/get-job-inputs/index.js (7 hunks)
- integrations/github-get-job-inputs/src/index.ts (2 hunks)
- openapi.v1.yaml (1 hunks)
- packages/node-sdk/src/.openapi-generator/FILES (1 hunks)
- packages/node-sdk/src/models/GetJob200Response.ts (5 hunks)
- packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts (1 hunks)
- packages/node-sdk/src/models/index.ts (1 hunks)
🔥 Files not summarized due to errors (1)
- github/get-job-inputs/index.js: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (4)
- integrations/github-get-job-inputs/src/index.ts
- openapi.v1.yaml
- packages/node-sdk/src/.openapi-generator/FILES
- packages/node-sdk/src/models/index.ts
🔇 Additional comments (13)
packages/node-sdk/src/models/GetJob200ResponseApprovalApprover.ts (3)
40-46: Past review comment about type guard implementation is still applicable.
54-65: Past review comment about JSON conversion input validation is still applicable.
22-35: Verify interface integration with related models.
Let's verify that this interface is properly integrated with the related models mentioned in the AI summary.
✅ Verification successful
Interface is properly integrated with related models
The verification shows that GetJob200ResponseApprovalApprover interface is correctly:
- Imported and used as an optional field in
GetJob200ResponseApproval - Has proper JSON serialization/deserialization support
- Exported correctly in the models index file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of GetJob200ResponseApprovalApprover interface
# Test 1: Check for references to this interface in other models
echo "Checking for references to GetJob200ResponseApprovalApprover..."
rg -l "GetJob200ResponseApprovalApprover" packages/node-sdk/src/models/
# Test 2: Check for proper imports in files that use this interface
echo "Checking import statements..."
rg "import.*GetJob200ResponseApprovalApprover" packages/node-sdk/src/models/
Length of output: 670
Script:
#!/bin/bash
# Let's check how GetJob200ResponseApprovalApprover is used in GetJob200ResponseApproval.ts
echo "Checking usage in GetJob200ResponseApproval.ts..."
cat packages/node-sdk/src/models/GetJob200ResponseApproval.ts
# Also check if this model is exported correctly in index.ts
echo -e "\nChecking exports in index.ts..."
cat packages/node-sdk/src/models/index.ts
Length of output: 3942
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (2)
9-9: LGTM! Well-structured type definition and imports.
The ApprovalJoinResult type is well-defined and properly typed using the schema's $inferSelect helper.
Also applies to: 19-19, 27-30
32-59: Great job implementing the suggested improvements!
The approval logic has been nicely extracted into separate functions with clear responsibilities:
getApprovalDetails: Handles data fetchingmapApprovalResponse: Handles response transformation
The implementation also correctly addresses the previous review comments by:
- Extracting the query logic for better maintainability
- Removing the email field from the approver response
packages/node-sdk/src/models/GetJob200Response.ts (4)
15-15: LGTM! Import statements are properly organized.
The new imports for GetJob200ResponseApproval and its JSON conversion utilities are correctly added and follow the existing import pattern.
Also applies to: 22-26
141-142: LGTM! Type checking is properly updated.
The instanceOfGetJob200Response function correctly validates the new required properties.
106-112: Verify the impact of breaking changes in the interface.
The changes make variables required and add a new required approval property. This is a breaking change that could affect existing consumers of the SDK.
Let's verify the usage:
#!/bin/bash
# Search for direct usage of GetJob200Response type
rg -t typescript "GetJob200Response" --no-heading
# Search for job response object destructuring to find potential affected code
rg -t typescript "const.*=.*getJob\(" -A 5Line range hint 1-200: Verify alignment with OpenAPI specification.
Since this is an auto-generated file, we should verify that the OpenAPI specification correctly defines these new required properties.
✅ Verification successful
Let me search for the spec files first to locate them, then we can examine their contents for the Job response schema.
Let's examine the contents of these OpenAPI specification files to verify the Job response schema.
OpenAPI specification and generated code are correctly aligned
The generated TypeScript interface GetJob200Response matches the OpenAPI specification in openapi.v1.yaml. The spec defines all the required properties (id, status, variables, approval) and optional properties (release, deployment, runbook, target, environment) with matching types and structure. The status enum values in the TypeScript code also exactly match those defined in the OpenAPI spec.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the Job response schema in the OpenAPI spec
rg -t yaml "Job.*Response:" -A 20 "openapi.*\.ya?ml"
Length of output: 164
Script:
#!/bin/bash
# Search for OpenAPI/Swagger spec files in the repository
fd -e yaml -e yml
# Also search for any potential JSON spec files
fd -e json --full-path openapi
Length of output: 387
Script:
#!/bin/bash
# Check the main OpenAPI spec file for Job response schema
cat openapi.v1.yaml
# Also check the job agents queue spec as it might contain job-related schemas
cat apps/webservice/src/app/api/v1/job-agents/[agentId]/queue/openapi.yaml
Length of output: 17345
github/get-job-inputs/index.js (4)
27857-27883: Well-structured implementation of the ApprovalApprover model!
The model correctly implements type checking and JSON serialization/deserialization with proper null handling.
28121-28122: LGTM! Proper type checking for new required fields.
The implementation correctly enforces the presence of variables and approval fields in the response type.
28140-28141: LGTM! Consistent JSON serialization implementation.
The JSON conversion methods properly handle the new variables and approval fields in both directions.
Also applies to: 28156-28157
28909-28909: Verify potential naming conflicts in output variables.
While the implementation correctly handles the new approval fields and sanitizes variable keys, there's a potential risk of naming conflicts. The variable_ prefix might not be sufficient to prevent collisions with other outputs if a variable name matches an existing output name after sanitization.
Consider:
- Using a more specific prefix for variables
- Adding validation to prevent conflicts with known output names
Also applies to: 28926-28932
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores