Feat/transmission #35
Conversation
Reviewer's Guide by SourceryThis pull request introduces several new features and improvements, including real-time progress updates for data loading and transmission, enhanced error handling, improved user experience in the SelectorForm component, and the ability to edit site configurations. The changes include modifications to the DataExtraction, TestMappings, SelectorForm, SiteConfigsList, MainRoutes, and EditSiteConfigDetails components, as well as updates to the site configurations queries. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @MaryKilewe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using environment variables for WebSocket URLs instead of hardcoding them.
- The websocket connection logic is duplicated, consider refactoring into a shared function.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 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.
|
|
||
| newProgressSocket.onclose = function () { | ||
| console.log("ws connection closed"); | ||
| setSendingSpinner(false); |
There was a problem hiding this comment.
issue: Possible incorrect spinner state update in checkLoadedCount.
The function calls setSendingSpinner but the rest of the component uses setSpinner. Verify that setSendingSpinner exists or consider using setSpinner consistently.
| @@ -96,6 +109,7 @@ const SelectorForm = () => { | |||
| formData.push({ | |||
| "base_repository": baselookup, | |||
| "base_variable_mapped_to": baseVariable, | |||
| "is_required": baseVariable, | |||
There was a problem hiding this comment.
issue (bug_risk): Potential type issue for the 'is_required' property.
In other parts of the code, 'is_required' is set to a boolean (e.g., o.is_required). Passing baseVariable directly might be unintended; confirm that the correct boolean value is assigned.
| @@ -25,10 +25,16 @@ const DataExtraction = ({baseRepo}) =>{ | |||
| const [progress, setProgress] = useState(null); | |||
There was a problem hiding this comment.
issue (complexity): Consider creating a custom hook to encapsulate the WebSocket logic and reduce code duplication across the component.
Consider extracting your duplicated WebSocket handling into a custom hook. This reduces duplication and isolates event handling logic. For example, you could implement a hook like:
// useWebSocket.js
import { useEffect, useRef } from 'react';
export function useWebSocket(url, { onOpen, onMessage, onError, onClose, sendMsg } = {}) {
const socketRef = useRef(null);
useEffect(() => {
const ws = new WebSocket(url);
socketRef.current = ws;
ws.onopen = (event) => {
if (onOpen) onOpen(event);
if (sendMsg) ws.send(sendMsg);
};
ws.onmessage = (event) => onMessage && onMessage(event);
ws.onerror = (error) => onError && onError(error);
ws.onclose = (event) => onClose && onClose(event);
return () => {
ws.close();
};
}, [url, onOpen, onMessage, onError, onClose, sendMsg]);
return socketRef.current;
}Then refactor your WebSocket-based functions (both for sending and counting data) to use this hook:
// Example for checkLoadedCount
function checkLoadedCount(baseRepo) {
console.log("count loaded data...");
setDataLoadedCount(0);
useWebSocket(`ws://${WS_API}/api/dictionary_mapper/ws/load/progress/${baseRepo}`, {
onOpen: (event) => {
event.target.send(baseRepo);
},
onMessage: (event) => {
const data = event.data;
if (data.includes("Error")) {
console.error(data);
event.target.close();
} else {
setDataLoadedCount(Number(data.replace("%", "")));
}
},
onError: (error) => console.error("connection failed:", error),
onClose: () => {
console.log("ws connection closed");
setSendingSpinner(false);
setAlertType("success");
setLoadMessage("Loading data completed");
}
});
}Repeat a similar refactoring for the WebSocket in sendData or any other component needing WebSocket functionality. This approach centralizes the connection logic and makes maintenance easier.
| function checkLoadedCount(baseRepo) { | ||
|
|
||
| console.log("count loaded data... ") | ||
|
|
||
| setDataLoadedCount(0); // Reset progress to 0 | ||
| const newProgressSocket = new WebSocket(`ws://${WS_API}/api/dictionary_mapper/ws/load/progress/${baseRepo}`); | ||
| console.log("count newProgressSocket... ",newProgressSocket) | ||
|
|
||
| newProgressSocket.onopen = () => { | ||
| newProgressSocket.send(baseRepo); | ||
| }; | ||
| // Set up the WebSocket connection | ||
| newProgressSocket.onmessage = function (event) { | ||
| console.log("ws connection established ") | ||
|
|
||
| const data = event.data; | ||
| if (data.includes("Error")) { | ||
| console.error(data); | ||
| newProgressSocket.close(); | ||
| } else { | ||
| console.log("data count--->",data) | ||
| setDataLoadedCount(Number(data.replace("%", ""))); // Update progress | ||
| } | ||
| }; | ||
|
|
||
| newProgressSocket.onerror = function (error) { | ||
| console.log("connection failed with error: ", error) | ||
| }; | ||
|
|
||
| newProgressSocket.onclose = function () { | ||
| console.log("ws connection closed"); | ||
| setSendingSpinner(false); | ||
| setAlertType("success"); | ||
| setLoadMessage("Loading data completed"); | ||
| }; | ||
|
|
||
| setSocket(newProgressSocket); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.| newProgressSocket.onmessage = function (event) { | ||
| console.log("ws connection established ") | ||
|
|
||
| const data = event.data; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const data = event.data; | |
| const {data} = event; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
Summary by Sourcery
Implements data transmission functionality, including WebSocket communication for progress updates and data loading counts. It also enhances the site configuration management by adding the ability to edit site configurations.
New Features:
Tests: