🐛 Fix multiple custom rules upload #2636
Conversation
When uploading multiple custom rules in analyzer wizard, only last one was corectly processed for it s rules, updating code to make sure each uploaded file processed. Used claude code to help with this change. Signed-off-by: Marek Aufart <[email protected]>
WalkthroughPer-file UploadFile contexts are created and stored internally when files begin uploading; these contexts are threaded through upload calls and delivered to onSuccess/onError callbacks. useFileUploader and uploadFile signatures were extended to accept an optional ruleFileContext, and the internal map clears contexts after callback invocation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as CustomRuleFilesUpload
participant Timer as setTimeout
participant Verifier as readVerifyAndUploadFile
participant Hook as useFileUploader
participant API as createRuleFile / uploadTaskgroupFile
User->>UI: Add rule files
UI->>Timer: Defer processing incomingFiles
Timer-->>UI: Iterate files
loop For each file
UI->>Verifier: readVerifyAndUploadFile(ruleFile, file)
Verifier->>Hook: uploadFile(file, taskgroupId?, ruleFileContext)
Hook->>Hook: fileContextRef[file.name] = ruleFileContext
Hook->>API: createRuleFile / uploadTaskgroupFile (uses stored context)
alt Upload success
API-->>Hook: success + hubFile
Hook->>Hook: ctx = retrieve & delete fileContextRef[file.name]
Hook-->>UI: onSuccess(file, hubFile, ctx)
UI->>UI: update ruleFile (uploadProgress/status)
else Upload error
API-->>Hook: error (AxiosError)
Hook->>Hook: ctx = retrieve & delete fileContextRef[file.name]
Hook-->>UI: onError(error, file, ctx)
UI->>UI: update ruleFile (loadError/status)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/app/components/CustomRuleFilesUpload.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/app/components/CustomRuleFilesUpload.tsx (1)
client/src/app/utils/utils.ts (1)
getAxiosErrorMessage(10-16)
⏰ 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). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
Signed-off-by: Marek Aufart <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/src/app/components/CustomRuleFilesUpload.tsx (1)
153-159: Simplify: Remove redundant updatedRuleFile construction.Lines 153-158 duplicate the state update already performed on lines 144-149. Both create an object with
uploadProgress: 20,status: "validated", andcontents: fileContents.Apply this diff to eliminate the duplication and directly pass the updated ruleFile:
onChangeRuleFile({ ...ruleFile, uploadProgress: 20, status: "validated", contents: fileContents, }); // Upload the file to hub! // TODO: Provide an onUploadProgress handler so the actual upload can be tracked from 20% to 100% - const updatedRuleFile = { - ...ruleFile, - uploadProgress: 20, - status: "validated" as const, - contents: fileContents, - }; - uploadFile(file, taskgroupId, updatedRuleFile); + uploadFile(file, taskgroupId, { + ...ruleFile, + uploadProgress: 20, + status: "validated", + contents: fileContents, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/app/components/CustomRuleFilesUpload.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/app/components/CustomRuleFilesUpload.tsx (4)
client/src/app/utils/utils.ts (1)
getAxiosErrorMessage(10-16)client/src/app/api/models.ts (2)
HubFile(815-819)UploadFile(602-611)client/src/app/queries/targets.ts (1)
useCreateFileMutation(103-124)client/src/app/queries/taskgroups.ts (1)
useUploadTaskgroupFileMutation(54-73)
⏰ 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: run-global-ci / e2e-ui-integration-tests
🔇 Additional comments (3)
client/src/app/components/CustomRuleFilesUpload.tsx (3)
170-193: Excellent fix: Context propagation eliminates fragile state lookups.This implementation directly addresses the critical issues from past reviews by:
- Passing
ruleFileContextthrough the upload chain instead of usingsetTimeout+ruleFileByNamelookups that could retrieve stale state.- Preserving all file properties (including
contents) via the spread operator, ensuring success/error handlers update the correct file with complete data.- Safe handling via
if (ruleFileContext)guards that prevent crashes if context is missing.The pattern of storing context in a Map (see lines 276-313) and retrieving it in callbacks ensures state updates reference the correct file snapshot without timing dependencies.
268-303: Clean context management pattern.The Map-based context storage correctly:
- Associates each file with its upload context using
file.nameas the key (line 312).- Retrieves context in callbacks (lines 281, 286, 294, 299) without relying on timing-sensitive state lookups.
- Cleans up after use (lines 283, 288, 296, 301) to prevent memory leaks.
The filename uniqueness assumption is safe because
setupFilesToUpload(lines 68-78) removes files with duplicate names before adding new ones, preventing key collisions.
305-324: Backward-compatible signature extension.The optional
ruleFileContextparameter (line 308) cleanly extends theuploadFileAPI without breaking existing callers. Context storage (lines 311-313) is isolated from the core upload logic, maintaining separation of concerns.
sjd78
added
the
cherry-pick/release-0.8
sjd78
left a comment
sjd78
left a comment
There was a problem hiding this comment.
Maybe some extra code in there that doesn't need to be there, and one style change, but it did work in my testing.
Co-authored-by: Scott Dickerson <[email protected]> Signed-off-by: Marek Aufart <[email protected]>
Signed-off-by: Marek Aufart <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/app/components/CustomRuleFilesUpload.tsx (1)
151-175: Defensiveif (ruleFileContext)guards may silently skip updates.Both callbacks skip the
onChangeRuleFileupdate whenruleFileContextis undefined. While this prevents crashes, it silently hides missing context, which could mask bugs ifuploadFileis ever called without providing the context.Consider adding a warning log or comment explaining this defensive behavior, especially since the current flow (line 141) always provides the context.
Example:
- (file, hubFile, ruleFileContext) => { - if (ruleFileContext) { + (file, hubFile, ruleFileContext) => { + if (!ruleFileContext) { + console.warn(`Missing context for file ${file.name}; skipping state update`); + return; + } + { onChangeRuleFile({ ...ruleFileContext, fileId: hubFile?.id, uploadProgress: 100, status: "uploaded", }); } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/app/components/CustomRuleFilesUpload.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/app/components/CustomRuleFilesUpload.tsx (4)
client/src/app/api/models.ts (2)
UploadFile(602-611)HubFile(815-819)client/src/app/utils/utils.ts (1)
getAxiosErrorMessage(10-16)client/src/app/queries/targets.ts (1)
useCreateFileMutation(103-124)client/src/app/queries/taskgroups.ts (1)
useUploadTaskgroupFileMutation(54-73)
⏰ 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). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (2)
client/src/app/components/CustomRuleFilesUpload.tsx (2)
132-141: LGTM! Clean context propagation.The flow correctly creates the complete
updatedRuleFilecontext (includingcontents), updates the parent state, then passes that context touploadFile. This ensures callbacks receive the full per-file context rather than relying on stale state lookups.
250-311: LGTM! Solid per-file context implementation.The Map-based context storage correctly preserves per-file state through async callbacks, fixing the original issue where only the last file was processed. Key strengths:
- Uses
file.nameas the Map key (safe givensetupFilesToUploadremoves duplicates before adding)- Retrieves and deletes context in both success and error paths (good memory hygiene)
- Consistent pattern for both
createRuleFileanduploadTaskgroupFileflowsMinor note: If an upload hangs indefinitely (callbacks never fire), the context remains in the Map. This is unlikely in practice and the Map is scoped to component lifecycle, so impact is minimal.
|
PR was updated removing not needed code, local test works well for me, asking for re-review @sjd78, thanks! |
sjd78
left a comment
sjd78
left a comment
There was a problem hiding this comment.
Changes look good and my local testing works out great.
Resolves: https://issues.redhat.com/browse/MTA-6029 Resolves: #2654 When uploading multiple custom rules in analyzer wizard, only last one was corectly processed for it s rules, updating code to make sure each uploaded file processed. Used claude code to assist with this change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Rule files now begin processing immediately after selection with initial progress/status visible. * Each file carries its own per-file context through the upload lifecycle for clearer handling and callbacks. * **Bug Fixes** * Upload progress and status updates are more consistent and visible. * Success and error callbacks reliably map to the correct file, preventing stale state and improving error attribution. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Marek Aufart <[email protected]> Signed-off-by: Cherry Picker <[email protected]>
Resolves: https://issues.redhat.com/browse/MTA-6029 Resolves: #2654 When uploading multiple custom rules in analyzer wizard, only last one was corectly processed for it s rules, updating code to make sure each uploaded file processed. Used claude code to assist with this change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Rule files now begin processing immediately after selection with initial progress/status visible. * Each file carries its own per-file context through the upload lifecycle for clearer handling and callbacks. * **Bug Fixes** * Upload progress and status updates are more consistent and visible. * Success and error callbacks reliably map to the correct file, preventing stale state and improving error attribution. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Marek Aufart <[email protected]> Signed-off-by: Cherry Picker <[email protected]> Co-authored-by: Marek Aufart <[email protected]>
Resolves: https://issues.redhat.com/browse/MTA-6029 Resolves: konveyor#2654 When uploading multiple custom rules in analyzer wizard, only last one was corectly processed for it s rules, updating code to make sure each uploaded file processed. Used claude code to assist with this change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Rule files now begin processing immediately after selection with initial progress/status visible. * Each file carries its own per-file context through the upload lifecycle for clearer handling and callbacks. * **Bug Fixes** * Upload progress and status updates are more consistent and visible. * Success and error callbacks reliably map to the correct file, preventing stale state and improving error attribution. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Marek Aufart <[email protected]>
2 participants
Resolves: https://issues.redhat.com/browse/MTA-6029
Resolves: #2654
When uploading multiple custom rules in analyzer wizard, only last one was corectly processed for it s rules, updating code to make sure each uploaded file processed.
Used claude code to assist with this change.
Summary by CodeRabbit
New Features
Bug Fixes