Skip to content
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

[Fix] Handled client side network issues #833

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jagadeeswaran-zipstack
Copy link
Contributor

What

  • Added error handling for network issues in Axios requests.

Why

  • To inform users when network connectivity issues affect API calls.

How

  • Used a custom error handler hook to check for ERR_NETWORK errors and navigator.onLine status, showing appropriate error messages.

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • No, because this only affects the Axios hook and its error handling.

Notes on Testing

  • Test by disconnecting from the internet or simulating network failure to verify error message appears.

Screenshots

image

Checklist

I have read and understood the Contribution Guidelines.

Copy link

sonarqubecloud bot commented Nov 7, 2024

Copy link
Contributor

github-actions bot commented Nov 7, 2024

filepath function $$\textcolor{#23d18b}{\tt{passed}}$$ SUBTOTAL
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_logs}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup\_skip}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_client\_init}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image\_exists}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config\_without\_mount}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_run\_container}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{9}}$$ $$\textcolor{#23d18b}{\tt{9}}$$

@jagadeeswaran-zipstack jagadeeswaran-zipstack changed the title added checks for client side network issues [Fix] Handled client side network issues Nov 7, 2024
Comment on lines +21 to +35
if (err.code === "ERR_NETWORK" && !navigator.onLine) {
return {
type: "error",
content: "Please check your internet connection.",
title: title,
duration: duration,
};
} else if (err.code === "ERR_CANCELED") {
return {
type: "error",
content: "Request has been canceled.",
title: title,
duration: duration,
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (err.code === "ERR_NETWORK" && !navigator.onLine) {
return {
type: "error",
content: "Please check your internet connection.",
title: title,
duration: duration,
};
} else if (err.code === "ERR_CANCELED") {
return {
type: "error",
content: "Request has been canceled.",
title: title,
duration: duration,
};
}
const getErrorMessage = (err, title, duration) => {
const errorMessages = {
ERR_NETWORK: "Please check your internet connection.",
ERR_CANCELED: "Request has been canceled.",
};
if (errorMessages[err.code]) {
return {
type: "error",
content: err.code === "ERR_NETWORK" && !navigator.onLine ? errorMessages.ERR_NETWORK : errorMessages[err.code],
title,
duration,
};
}
return null; // or handle other error cases
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check and apply this change if seems okay. @jagadeeswaran-zipstack

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can be used because adding more error codes will make the content condition more complicated. If you think otherwise please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants