-
Notifications
You must be signed in to change notification settings - Fork 411
RI-7659: Enhance RDI connection testing with per-source error handling #5123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RI-7659: Enhance RDI connection testing with per-source error handling #5123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors error handling for testing RDI source connections to provide more granular feedback. Instead of catching all errors at the top level and silently ignoring them, the code now handles errors per-source and differentiates between unsupported RDI versions (405 errors) and actual connection failures.
Key Changes:
- Error handling moved from try-catch wrapper to per-source error handling within the Promise.all map
- 405 status code now returns a user-friendly message indicating RDI version upgrade is needed
- Other errors return a generic "Failed to test source connection" message
- Early return added when no sources are configured
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| api.rdi.client.ts | Refactored testConnections method to handle source connection test errors individually rather than globally, with specific handling for 405 responses |
| api.rdi.client.spec.ts | Added comprehensive test coverage for the new error handling behavior including single/multiple source failures and mixed error scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Coverage - Frontend unit tests
Test suite run success5229 tests passing in 681 suites. Report generated by 🧪jest coverage report action from dda7a61 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
| sources[source] = { | ||
| connected: false, | ||
| error: | ||
| 'Testing source connections is not supported in your RDI version. Please upgrade to version 1.6.0 or later.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we promote latest version instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge it this way, as I showed the texts to @ViktarStarastsenka. Honestly, I don't expect this message to be seen that much, as the RDI team is pushing for a lot of releases.
Description
Currently, there’s a bug that causes the connection check to appear as
successful 0.The expected outcome is either
successful 1orfailed 1- i.e., one definitive result per source.The root cause is missing backward compatibility handling. Specifically, there are a few scenarios that need to be covered:
405.This PR addresses these cases by:
After
source2- is an expected connection problem, so it's properly reported as failingsource- is handled by the changes here, as RDI has a problem reporting the connection status (a.k.a. runtime error)