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

feat(platofrm): Added online/offline status detection in the platform #585

Conversation

Allan2000-Git
Copy link
Contributor

@Allan2000-Git Allan2000-Git commented Dec 9, 2024

User description

Description

This PR introduces functionality to detect and display the online/offline status of the platform using a toast message.

Fixes #499

Dependencies

N/A

Future Improvements

N/A

Mentions

@rajdip-b

Screenshots of relevant screens

I have throttled the network for testing the functionality.

https://www.loom.com/share/10eb3fcf518b40179a4ce52f92b46fa7

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

enhancement


Description

  • Introduced OnlineStatusHandler component to manage online/offline status detection.
  • Implemented useOnlineStatus hook to listen for network status changes and display toast notifications.
  • Integrated the OnlineStatusHandler into the main application layout to provide real-time feedback on internet connectivity.

Changes walkthrough 📝

Relevant files
Enhancement
layout.tsx
Integrate online status handler in the application layout

apps/platform/src/app/layout.tsx

  • Added OnlineStatusHandler component to the layout.
  • Ensured OnlineStatusHandler is wrapped by JotaiProvider.
  • +5/-1     
    online-status-handler.tsx
    Create OnlineStatusHandler component for status detection

    apps/platform/src/components/common/online-status-handler.tsx

  • Created OnlineStatusHandler component.
  • Utilized useOnlineStatus hook within the component.
  • +10/-0   
    use-online-status.ts
    Implement useOnlineStatus hook for connection status         

    apps/platform/src/hooks/use-online-status.ts

  • Implemented useOnlineStatus hook.
  • Added event listeners for online and offline status.
  • Displayed toast notifications for status changes.
  • +25/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    499 - Partially compliant

    Compliant requirements:

    • Show toaster notification when user goes offline
    • Show toaster notification when user comes back online
    • Reset and sync platform state when reconnecting (via page reload)

    Non-compliant requirements:

    • Display a red bar at the top of interface when user is offline
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    User Experience
    The 1 second delay before refresh may be too short for the user to read the success message. Consider increasing the timeout duration.

    Error Handling
    The page reload functionality should have error handling in case the reload fails or network is still unstable

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Dec 9, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Avoid forcing page reload when reconnecting to prevent potential data loss and unnecessary refreshes

    Add a check to prevent the page reload if the user is already on the latest version
    or if there are unsaved changes. Consider using a more graceful refresh strategy.

    apps/platform/src/hooks/use-online-status.ts [5-10]

     if (navigator.onLine) {
    -    toast.success("You are back online! Refreshing...");
    -    setTimeout(() => {
    -        window.location.reload();
    -    }, 1000);
    +    toast.success("You are back online!");
    +    if (document.documentElement.dataset.needsRefresh === 'true') {
    +        toast.info("Refreshing to get latest changes...");
    +        setTimeout(() => {
    +            window.location.reload();
    +        }, 1000);
    +    }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical UX issue by preventing unnecessary page reloads that could lead to data loss. The improved code adds a smart check for determining if a refresh is actually needed.

    9
    ✅ Prevent multiple notifications and reloads during unstable network conditions

    Add debouncing to the status handler to prevent rapid-fire notifications and reloads
    during unstable connections.

    apps/platform/src/hooks/use-online-status.ts [4-10]

    +let statusTimeout: NodeJS.Timeout;
     const statusHandler = () => {
    -    if (navigator.onLine) {
    -        toast.success("You are back online! Refreshing...");
    -        setTimeout(() => {
    -            window.location.reload();
    -        }, 1000);
    -    }
    +    clearTimeout(statusTimeout);
    +    statusTimeout = setTimeout(() => {
    +        if (navigator.onLine) {
    +            toast.success("You are back online! Refreshing...");
    +            setTimeout(() => {
    +                window.location.reload();
    +            }, 1000);
    +        }
    +    }, 2000);
    +}

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: This suggestion addresses an important edge case where unstable connections could trigger multiple notifications and reloads in quick succession, significantly improving user experience during network instability.

    8
    Add error handling to prevent component crashes from network monitoring failures

    Add error boundary to prevent potential crashes from network status monitoring
    failures.

    apps/platform/src/components/common/online-status-handler.tsx [5-8]

     function OnlineStatusHandler() {
    -    useOnlineStatus();
    +    try {
    +        useOnlineStatus();
    +    } catch (error) {
    +        console.error('Failed to initialize online status handler:', error);
    +    }
         return null;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While error handling is good practice, network status monitoring is a fairly stable browser API, and the current implementation is simple enough that catastrophic failures are unlikely. The improvement adds defensive programming but has moderate impact.

    4

    💡 Need additional feedback ? start a PR chat

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Dec 9, 2024

    @kriptonian1 @poswalsameer can you drop your reviews?

    @Allan2000-Git
    Copy link
    Contributor Author

    @rajdip-b I can implement suggestions given my code bot reviewer.

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Dec 9, 2024

    Just the one that I marked, the others are not needed

    @rajdip-b I can implement suggestions given my code bot reviewer.

    @poswalsameer
    Copy link
    Contributor

    @kriptonian1 @poswalsameer can you drop your reviews?

    Yeah sure1

    @poswalsameer
    Copy link
    Contributor

    Hi @Allan2000-Git, can you please attach some screenshots with the PR?

    @poswalsameer
    Copy link
    Contributor

    @kriptonian1 @poswalsameer can you drop your reviews?

    All looks good to me. I think instead of using setTimeout, we should have a boolean state to check the online/offline status of the user. And as this state changes, we can re-render the page using useEffect and show the corresponding toast message.

    @Allan2000-Git
    Copy link
    Contributor Author

    @kriptonian1 @poswalsameer can you drop your reviews?

    All looks good to me. I think instead of using setTimeout, we should have a boolean state to check the online/offline status of the user. And as this state changes, we can re-render the page using useEffect and show the corresponding toast message.

    Cool, will make the change & update

    @poswalsameer
    Copy link
    Contributor

    @kriptonian1 @poswalsameer can you drop your reviews?

    All looks good to me. I think instead of using setTimeout, we should have a boolean state to check the online/offline status of the user. And as this state changes, we can re-render the page using useEffect and show the corresponding toast message.

    Cool, will make the change & update

    did the state thing worked?

    @Allan2000-Git
    Copy link
    Contributor Author

    Allan2000-Git commented Dec 10, 2024

    @kriptonian1 @poswalsameer can you drop your reviews?

    All looks good to me. I think instead of using setTimeout, we should have a boolean state to check the online/offline status of the user. And as this state changes, we can re-render the page using useEffect and show the corresponding toast message.

    Cool, will make the change & update

    did the state thing worked?

    went through code, to keep it much cleaner, we do not require state actually as we're directly using the hook in the client component. state usage would be much more useful if we're using that state anywhere apart from self component

    @poswalsameer
    Copy link
    Contributor

    @kriptonian1 @poswalsameer can you drop your reviews?

    All looks good to me. I think instead of using setTimeout, we should have a boolean state to check the online/offline status of the user. And as this state changes, we can re-render the page using useEffect and show the corresponding toast message.

    Cool, will make the change & update

    did the state thing worked?

    went through code, to keep it much cleaner, we do not require state actually as we're directly using the hook in the client component. state usage would be much more useful if we're using that state anywhere apart from self component

    Sounds fine. @rajdip-b, I think we can merge this.

    @rajdip-b
    Copy link
    Member

    @kriptonian1 final thoughts? if you can approve it, I'll merge it asap

    @rajdip-b rajdip-b changed the title feat: Added online/offline status detection in the platform feat(platofrm): Added online/offline status detection in the platform Dec 13, 2024
    Copy link
    Contributor

    @kriptonian1 kriptonian1 left a comment

    Choose a reason for hiding this comment

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

    LGTM

    @rajdip-b rajdip-b merged commit 89aa84f into keyshade-xyz:develop Dec 14, 2024
    4 checks passed
    @Allan2000-Git Allan2000-Git deleted the feat/internet-connection-status-feedback branch December 14, 2024 07:47
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    PLATFORM: No Visual Feedback for Internet Connection Status
    4 participants