-
Notifications
You must be signed in to change notification settings - Fork 2
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
updated load data count progress updates #36
Conversation
Reviewer's Guide by SourceryThis pull request updates the data loading process in the DataExtraction component to use a WebSocket connection. This allows for real-time progress updates and more efficient data retrieval. The previous REST API call implementation was removed and replaced with WebSocket logic. Sequence diagram for data loading with WebSocketsequenceDiagram
participant User
participant DataExtraction Component
participant WebSocket Server
User->>DataExtraction Component: Initiates data loading
DataExtraction Component->>WebSocket Server: Establishes WebSocket connection
activate WebSocket Server
WebSocket Server-->>DataExtraction Component: Sends progress updates and data
loop Receive updates
DataExtraction Component->>DataExtraction Component: Updates data loaded count or displays data
end
alt Error received
WebSocket Server-->>DataExtraction Component: Sends error message
DataExtraction Component->>DataExtraction Component: Displays error message to User
end
deactivate WebSocket Server
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @MaryKilewe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider removing the commented-out code to reduce confusion.
- Ensure that the websocket error messages are user-friendly.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -184,15 +183,37 @@ const DataExtraction = ({baseRepo}) =>{ | |||
}; | |||
// Set up the WebSocket connection | |||
newProgressSocket.onmessage = function (event) { |
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.
issue (complexity): Consider extracting the JSON handling and progress update logic into separate helper functions to reduce nesting and improve readability of the onmessage handler function
Consider extracting the JSON handling and progress update logic into separate helper functions. For example:
function handleJsonData(jsonData, baseRepo) {
if (Array.isArray(jsonData)) {
if (jsonData.length > 0) {
setLoadedRepoData(jsonData);
setAlertType("success");
setLoadSuccessAlert(true);
setLoadMessage("Successfully loaded " + baseRepo + " data");
} else {
setAlertType("error");
setLoadSuccessAlert(true);
setLoadMessage(
"Failed to load " + baseRepo + " data. Check the Site Configured and make sure the data in the source that matches this site is available to extract"
);
}
setSpinner(false);
}
}
function handleProgress(data) {
setDataLoadedCount(Number(data.replace("%", "")));
}
And then simplify the onmessage
handler:
newProgressSocket.onmessage = function (event) {
const data = event.data;
if (data.includes("Error")) {
newProgressSocket.close();
setAlertType("error");
setLoadSuccessAlert(true);
setLoadMessage("Failed to load : ERROR -->" + data);
return;
}
try {
const jsonData = JSON.parse(data);
handleJsonData(jsonData, baseRepo);
} catch (e) {
// If parsing fails, try to handle it as a progress update.
handleProgress(data);
}
};
This refactoring reduces nesting by clearly separating JSON handling and progress updates while keeping the functionality unchanged.
No description provided.