ECOPROJECT-3446 | feat: Implement asynchronous job handling for RVToo… #215
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughRVTools assessment creation switched from synchronous to asynchronous: the frontend starts an RVTools job via AssessmentService, polls job status until completion/failure (interval + timeout), refreshes assessments on success, and returns a minimal Assessment-like object (id + name) while full data is refreshed separately. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Provider Component
participant Service as AssessmentService
participant API as Backend API
UI->>Service: createAssessment(rvToolFile)
Service->>API: POST /api/v1/assessments/rvtools
API-->>Service: AsyncJob {id, status: pending}
Service-->>UI: AsyncJob
rect rgba(100,150,200,0.12)
Note over UI,Service: Polling loop (every 2s, overall timeout)
loop until status == completed or failed or timeout
UI->>Service: getRVToolsJobStatus(jobId)
Service->>API: GET /api/v1/rvtools/jobs/{jobId}
API-->>Service: AsyncJob {status: running|completed|failed}
Service-->>UI: AsyncJob
end
end
rect rgba(100,200,120,0.12)
Note over UI: On success -> refresh assessments list
UI->>Service: refreshAssessmentsList()
UI-->>UI: Return minimal Assessment { id: assessment_id, name }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/retest |
Jos-Jerus
force-pushed
the
rvtools-upload-async
branch
2 times, most recently
from
95a7566 to
8c05fa9
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/assessment/assessmentService.ts (1)
77-85: Incomplete implementation: missing inventory and agent handling.The
createAssessmentmethod only handles the RVTools case, but theCreateAssessmentRequestinterface definessourceTypeas'inventory' | 'rvtools' | 'agent'. This means:
- Inventory-based assessments will fail with "RVTools file is required"
- Agent-based assessments will fail with the same error
- The method signature doesn't match its actual behavior
Either:
- Update this method to handle all three source types (inventory, agent, rvtools), or
- Create separate methods for each source type and update the interface accordingly, or
- If this service is RVTools-only by design, update the interface to remove inventory and agent from sourceType
Apply this diff if the service should remain RVTools-only:
export interface CreateAssessmentRequest { name: string; - sourceType: 'inventory' | 'rvtools' | 'agent'; - inventory?: Inventory; - sourceId?: string; + sourceType: 'rvtools'; rvToolFile?: File; }Or implement the missing cases:
async createAssessment(request: CreateAssessmentRequest): Promise<AsyncJob> { const assessmentName = request.name || `Assessment-${new Date().toISOString()}`; - if (!request.rvToolFile) { - throw new Error('RVTools file is required for RVTools assessment'); + if (request.sourceType === 'rvtools') { + if (!request.rvToolFile) { + throw new Error('RVTools file is required for RVTools assessment'); + } + return this.createFromRVTools(assessmentName, request.rvToolFile); + } else if (request.sourceType === 'inventory') { + // TODO: Implement inventory assessment creation + throw new Error('Inventory assessment creation not yet implemented'); + } else if (request.sourceType === 'agent') { + // TODO: Implement agent assessment creation + throw new Error('Agent assessment creation not yet implemented'); + } else { + throw new Error(`Unsupported source type: ${request.sourceType}`); } - return this.createFromRVTools(assessmentName, request.rvToolFile); }
🧹 Nitpick comments (3)
src/migration-wizard/contexts/discovery-sources/Provider.tsx (3)
100-107: Consider a more robust approach for accessing the authenticated fetch function.The type assertion
(assessmentApi as any)to access internal configuration is fragile and could break if the API client implementation changes. Consider:
- Exposing the fetch function through a proper public API
- Using dependency injection to provide the authenticated fetch function
- Documenting this workaround with a TODO comment if it must remain
174-183: Extract polling interval as a named constant.The 2-second interval is hardcoded and appears in both the comment and the setTimeout call. Extract it as a constant for maintainability.
Apply this diff:
+ const POLL_INTERVAL_MS = 2000; + // Create different request based on sourceType if (sourceType === 'inventory' && jsonValue) { // ... inventory handling ... } else if (sourceType === 'rvtools' && rvToolFile) { // Start async job const job = await assessmentService.createFromRVTools( assessmentName, rvToolFile, ); - // Poll job status every 2 seconds until completed or failed + // Poll job status until completed or failed let currentJob = job; while ( currentJob.status === 'pending' || currentJob.status === 'running' ) { - // Wait 2 seconds before checking again - await new Promise((resolve) => setTimeout(resolve, 2000)); + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); currentJob = await assessmentService.getRVToolsJobStatus(job.id); }
195-201: Document the type safety limitation of the minimal Assessment object.Returning a minimal object with a type assertion (
as Assessment) could lead to runtime errors if calling code expects a fully populated Assessment object with properties beyondidandname. The comments explain the workaround, but this remains fragile.Consider one of these alternatives:
- Update the return type of
createAssessmentto bePromise<Assessment | { id: string; name: string }>for the RVTools path- Return
nullor a distinct type for async operations and update callers accordingly- Add runtime validation in consuming code to check for complete Assessment objects
If keeping the current approach, add a comment warning about the incomplete object:
// Return a minimal assessment object (the actual assessment is in the list) // We need to return something for type compatibility, but the actual // assessment data will come from the refreshed list + // WARNING: This is not a complete Assessment object - only id and name are populated return { id: currentJob.assessment_id || '', name: assessmentName, } as Assessment;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/migration-wizard/contexts/discovery-sources/Provider.tsx(1 hunks)src/pages/assessment/assessmentService.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / migration-planner-ui-on-pull-request
🔇 Additional comments (5)
src/migration-wizard/contexts/discovery-sources/Provider.tsx (2)
168-172: LGTM - Async job initiation is correct.The job is properly initiated with the assessment name and file.
186-190: LGTM - Error handling is appropriate.The error handling properly checks the final job status and throws a descriptive error with the backend error message or a fallback.
src/pages/assessment/assessmentService.ts (3)
11-17: LGTM - AsyncJob interface is well-defined.The interface appropriately models an asynchronous job with status tracking, error handling, and optional assessment ID reference.
31-56: LGTM - RVTools job creation is properly implemented.The method correctly:
- Uses FormData for file upload
- Converts File to Blob with generic content type to avoid MIME issues
- Returns AsyncJob for tracking
- Provides clear error messages
58-75: LGTM - Job status polling is correctly implemented.The method properly fetches job status from the backend and handles errors with descriptive messages.
Jos-Jerus
force-pushed
the
rvtools-upload-async
branch
from
8c05fa9 to
1b21124
Compare
…ls assessment creation - Updated the assessment service to return job status for RVTools file processing. - Added polling mechanism to check job status until completion or failure. - Enhanced error handling for failed job status. - Adjusted the Provider component to accommodate new job handling logic. Signed-off-by: yjacobi <[email protected]>
Jos-Jerus
force-pushed
the
rvtools-upload-async
branch
from
1b21124 to
578ea34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/migration-wizard/contexts/discovery-sources/Provider.tsx(1 hunks)src/pages/assessment/assessmentService.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / migration-planner-ui-on-pull-request
🔇 Additional comments (4)
src/pages/assessment/assessmentService.ts (3)
11-17: LGTM! Well-defined AsyncJob interface.The interface properly models an asynchronous job with appropriate fields and status enum values.
77-85: LGTM! Signature change aligns with async workflow.The return type change from
Promise<Assessment>toPromise<AsyncJob>is appropriate for the new asynchronous RVTools processing flow.
41-48: Verify backend implementation with the backend service team.This is a frontend-only repository, and the backend implementation of these endpoints is not present in this codebase. The frontend code is calling:
- POST
/api/v1/assessments/rvtools(line 42)- GET
/api/v1/rvtools/jobs/{jobId}(line 63)Please confirm with the backend team that these endpoints are implemented and match the expected contracts (FormData file upload for the first, AsyncJob response for both).
src/migration-wizard/contexts/discovery-sources/Provider.tsx (1)
168-197: LGTM! Polling logic with proper timeout and error handling.The implementation correctly:
- Polls job status every 2 seconds
- Implements a 5-minute timeout (150 attempts × 2 seconds)
- Handles both timeout and failed job status with clear error messages
- Includes error details from the job when available
| return { | ||
| id: currentJob.assessment_id || '', | ||
| name: assessmentName, | ||
| } as Assessment; |
There was a problem hiding this comment.
Guard against missing assessment_id in completed job.
If currentJob.assessment_id is undefined or null after successful completion, using an empty string as fallback (line 206) could cause downstream issues in code that expects a valid assessment ID. This likely indicates a backend problem that should be surfaced rather than silently handled.
Consider adding validation:
+ // Validate that we got an assessment_id from the completed job
+ if (!currentJob.assessment_id) {
+ throw new Error(
+ 'RVTools job completed but no assessment_id was returned',
+ );
+ }
+
// Return a minimal assessment object (the actual assessment is in the list)
// We need to return something for type compatibility, but the actual
// assessment data will come from the refreshed list
return {
- id: currentJob.assessment_id || '',
+ id: currentJob.assessment_id,
name: assessmentName,
} as Assessment;📝 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.
| return { | |
| id: currentJob.assessment_id || '', | |
| name: assessmentName, | |
| } as Assessment; | |
| // Validate that we got an assessment_id from the completed job | |
| if (!currentJob.assessment_id) { | |
| throw new Error( | |
| 'RVTools job completed but no assessment_id was returned', | |
| ); | |
| } | |
| // Return a minimal assessment object (the actual assessment is in the list) | |
| // We need to return something for type compatibility, but the actual | |
| // assessment data will come from the refreshed list | |
| return { | |
| id: currentJob.assessment_id, | |
| name: assessmentName, | |
| } as Assessment; |
🤖 Prompt for AI Agents
In src/migration-wizard/contexts/discovery-sources/Provider.tsx around lines 205
to 208, the code currently falls back to an empty string when
currentJob.assessment_id is missing; instead validate that assessment_id exists
after job completion and surface an error if it does not. Update the function to
check if currentJob.assessment_id is null/undefined and, if so, either throw a
descriptive Error or return a rejected Promise (and log the condition) so the
caller can handle it, otherwise return the Assessment object with the guaranteed
id; ensure the error message includes job id/context to aid debugging.
2 participants
…ls assessment creation
Summary by CodeRabbit