feat: Add getImportFailInfoBulk function to comfyManagerService#4626
feat: Add getImportFailInfoBulk function to comfyManagerService#4626viva-jinyi wants to merge 1 commit intomainfrom
Conversation
- Added new bulk API endpoint route for v2/customnode/import_fail_info_bulk - Implemented getImportFailInfoBulk function with proper validation and error handling - Added temporary type definitions (to be replaced with generated types after ComfyUI-Manager PR merge) - Includes JSDoc documentation and follows existing service patterns - Validates that either cnr_ids or urls arrays are provided This prepares the frontend for the new bulk import failure info API endpoint. Once the ComfyUI-Manager PR is merged and types are generated, the temporary types will be replaced with the auto-generated ones from the OpenAPI specification.
|
christian-byrne
added
area:manager
claude-review
|
Claude finished @viva-jinyi's task —— View job Comprehensive PR ReviewTodo List:
Review Complete! Comprehensive PR ReviewThis review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another. Review SummaryPR: feat: Add getImportFailInfoBulk function to comfyManagerService (#4626) Issue Distribution
Category Breakdown
Key FindingsArchitecture & DesignThe implementation follows established service patterns well, using the existing Code QualityThe main concern is internationalization compliance. According to CLAUDE.md line 35, all user-facing strings must use vue-i18n, but this implementation includes hard-coded error messages. This violates the project's strict internationalization requirements. Integration PointsThe function integrates well with the existing service architecture and follows the established conventions for:
Positive Observations
Required Changes Before Merge
Optional Improvements
ReferencesNext Steps
This is a comprehensive automated review. The internationalization issue is critical and must be addressed before merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ) => { | ||
| const errorContext = 'Fetching bulk import failure information' | ||
| const routeSpecificErrors = { | ||
| 400: 'Bad Request: Either cnr_ids or urls field is required' |
There was a problem hiding this comment.
[quality] high Priority\n\nIssue: Hard-coded error message violates internationalization requirements\nContext: According to CLAUDE.md line 35, all user-facing strings must use vue-i18n - no hard-coded text in services/utilities\nSuggestion: Replace with i18n key and add translation entry in src/locales/en/main.json
|
|
||
| // Validate that at least one field is provided | ||
| if (!params.cnr_ids?.length && !params.urls?.length) { | ||
| throw new Error('Either cnr_ids or urls field is required') |
There was a problem hiding this comment.
[quality] high Priority\n\nIssue: Hard-coded error message violates internationalization requirements\nContext: According to CLAUDE.md line 35, all user-facing strings must use vue-i18n - no hard-coded text in services/utilities\nSuggestion: Replace with i18n key and add translation entry in src/locales/en/main.json
| params: ImportFailInfoBulkRequest, | ||
| signal?: AbortSignal | ||
| ) => { | ||
| const errorContext = 'Fetching bulk import failure information' |
There was a problem hiding this comment.
[quality] medium Priority\n\nIssue: Hard-coded error message violates internationalization requirements\nContext: According to CLAUDE.md line 35, all user-facing strings must use vue-i18n - no hard-coded text in services/utilities\nSuggestion: Replace with i18n key and add translation entry in src/locales/en/main.json
| } | ||
|
|
||
| // Validate that at least one field is provided | ||
| if (!params.cnr_ids?.length && !params.urls?.length) { |
There was a problem hiding this comment.
[architecture] medium Priority\n\nIssue: Validation logic inconsistency between client and server\nContext: Client throws error before making API call, but the API also handles 400 errors for the same validation. This creates potential race conditions and inconsistent error handling paths\nSuggestion: Either handle all validation on the server side and let the API return 400, or ensure client validation exactly matches server validation rules
|
|
||
| // TODO: These types will be auto-generated once ComfyUI-Manager types are updated | ||
| // After the ComfyUI-Manager PR is merged and types are generated, replace these with the generated types | ||
| interface ImportFailInfoBulkRequest { |
There was a problem hiding this comment.
[quality] low Priority\n\nIssue: Missing JSDoc documentation for type interfaces\nContext: While the function has good JSDoc, the temporary type interfaces lack documentation explaining their structure and usage\nSuggestion: Add JSDoc comments to ImportFailInfoBulkRequest, ImportFailInfoItem, and ImportFailInfoBulkResponse interfaces
2 participants
Summary
Adds support for the new bulk import failure info API endpoint in the ComfyUI Manager service.
Changes
IMPORT_FAIL_INFO_BULK = 'v2/customnode/import_fail_info_bulk'getImportFailInfoBulk()with proper validation and error handlingImplementation Details
cnr_idsorurlsarrays are providedDependencies
This PR is prepared for the bulk API endpoint that is being implemented in ComfyUI-Manager:
Next Steps
Once the ComfyUI-Manager PR is merged:
Usage Example
┆Issue is synchronized with this Notion page by Unito