Skip to content

Add warning when refreshing screen in drag and drop notebook#1293

Merged
johbaxter merged 1 commit intodevfrom
notebook_data
Jun 13, 2025
Merged

Add warning when refreshing screen in drag and drop notebook#1293
johbaxter merged 1 commit intodevfrom
notebook_data

Conversation

@ishumita
Copy link
Copy Markdown
Contributor

@ishumita ishumita commented Jun 11, 2025

Description

These changes are to add warning when refreshing screen in drag and drop notebook to prevent the user from losing their work.

Refer ticket: SEMOSS/Semoss#711

Changes Made

Added a BeforeUnloadEvent to the page

How to Test

  1. Create a drag and drop application
  2. Add some components
  3. Try to refresh the page

Notes

image

@ishumita ishumita requested a review from a team as a code owner June 11, 2025 18:44
@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

Add warning when refreshing screen in drag and drop notebook


User description

Description

These changes are to add warning when refreshing screen in drag and drop notebook to prevent the user from losing their work.

Changes Made

Added a BeforeUnloadEvent to the page

How to Test

  1. Create a drag and drop application
  2. Add some components
  3. Try to refresh the page

Notes

image

PR Type

Enhancement


Description

  • Warn on page refresh to prevent data loss

  • Added beforeunload event listener via useEffect

  • Prevent default unload and set returnValue

  • Clean up event listener on unmount


Changes walkthrough 📝

Relevant files
Enhancement
BlocksWorkspace.tsx
Add beforeunload warning handler                                                 

packages/client/src/components/blocks-workspace/BlocksWorkspace.tsx

  • Introduced useEffect to handle beforeunload
  • Prevent default navigation, set returnValue
  • Register/unregister event listener on mount/unmount
  • +13/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @ishumita ishumita requested a review from AAfghahi June 11, 2025 18:45
    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /review

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Unconditional warning

    The beforeunload handler is always active, causing a warning even when there are no unsaved changes. Consider registering it conditionally when the workspace has unsaved work to avoid unnecessary prompts.

    //to throw a warning when the user tried to reload the page
    // this is to prevent the user from losing their work
    useEffect(() => {
        const handleBeforeUnload = (e: BeforeUnloadEvent) => {
            e.preventDefault();
            e.returnValue = '';
        };
        window.addEventListener('beforeunload', handleBeforeUnload);
        return () => {
            window.removeEventListener('beforeunload', handleBeforeUnload);
        };
    }, []);
    Browser compatibility

    Some browsers ignore custom messages in beforeunload dialogs. Verify that setting e.returnValue = '' will produce a prompt across target browsers, or provide a fallback message if needed.

    const handleBeforeUnload = (e: BeforeUnloadEvent) => {
        e.preventDefault();
        e.returnValue = '';
    };

    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /improve

    @QodoAI-Agent
    Copy link
    Copy Markdown

    QodoAI-Agent commented Jun 11, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 6f1ee35

    CategorySuggestion                                                                                                                                    Impact
    General
    Drop redundant preventDefault call

    Remove the redundant e.preventDefault() call since setting e.returnValue is
    sufficient for modern browsers. This simplifies the handler and avoids unnecessary
    API usage.

    packages/client/src/components/blocks-workspace/BlocksWorkspace.tsx [252-261]

     useEffect(() => {
         const handleBeforeUnload = (e: BeforeUnloadEvent) => {
    -        e.preventDefault();
             e.returnValue = '';
         };
         window.addEventListener('beforeunload', handleBeforeUnload);
         return () => {
             window.removeEventListener('beforeunload', handleBeforeUnload);
         };
     }, []);
    Suggestion importance[1-10]: 5

    __

    Why: Removing e.preventDefault() is a safe simplification since e.returnValue is sufficient in modern browsers, but it is a minor stylistic improvement.

    Low

    Previous suggestions

    Suggestions up to commit 6f1ee35
    CategorySuggestion                                                                                                                                    Impact
    General
    Add descriptive unload message

    Provide a clear, user-friendly message in the unload prompt instead of an empty
    string, since most browsers will ignore an empty returnValue. This gives users
    context about what they might lose.

    packages/client/src/components/blocks-workspace/BlocksWorkspace.tsx [253-256]

     const handleBeforeUnload = (e: BeforeUnloadEvent) => {
    +    const confirmationMessage = 'You have unsaved changes—leaving will discard them.';
         e.preventDefault();
    -    e.returnValue = '';
    +    e.returnValue = confirmationMessage;
     };
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion improves user experience by providing a clear custom message in the unload prompt, aligning with best practices since many browsers ignore empty returnValue.

    Low
    Register listener only when dirty

    Only attach the beforeunload listener when there are actual unsaved changes to avoid
    unnecessary warnings. Tie the effect’s dependencies to your “unsaved” state.

    packages/client/src/components/blocks-workspace/BlocksWorkspace.tsx [252-261]

     useEffect(() => {
    +    if (!hasUnsavedChanges) return;
         const handleBeforeUnload = (e: BeforeUnloadEvent) => {
    +        const confirmationMessage = 'You have unsaved changes—leaving will discard them.';
             e.preventDefault();
    -        e.returnValue = '';
    +        e.returnValue = confirmationMessage;
         };
         window.addEventListener('beforeunload', handleBeforeUnload);
         return () => {
             window.removeEventListener('beforeunload', handleBeforeUnload);
         };
    -}, []);
    +}, [hasUnsavedChanges]);
    Suggestion importance[1-10]: 5

    __

    Why: Delaying the beforeunload listener until there are unsaved changes avoids unnecessary prompts, improving performance and UX though it relies on a new hasUnsavedChanges state not yet defined.

    Low
    Suggestions up to commit 6f1ee35
    CategorySuggestion                                                                                                                                    Impact
    General
    Add unload listener only when dirty

    Only warn users when there are actual unsaved changes, to avoid unnecessary prompts
    on every refresh. Use a workspace flag (e.g. workspace.hasUnsavedChanges) as a
    dependency and condition before adding the listener.

    packages/client/src/components/blocks-workspace/BlocksWorkspace.tsx [252-261]

     useEffect(() => {
         const handleBeforeUnload = (e: BeforeUnloadEvent) => {
             e.preventDefault();
             e.returnValue = '';
         };
    -    window.addEventListener('beforeunload', handleBeforeUnload);
    +    if (workspace.hasUnsavedChanges) {
    +        window.addEventListener('beforeunload', handleBeforeUnload);
    +    }
         return () => {
             window.removeEventListener('beforeunload', handleBeforeUnload);
         };
    -}, []);
    +}, [workspace.hasUnsavedChanges]);
    Suggestion importance[1-10]: 7

    __

    Why: Adding the listener only when workspace.hasUnsavedChanges avoids unnecessary prompts and improves UX without breaking functionality.

    Medium
    Use descriptive unload message

    Provide a clear custom message to the user—most modern browsers ignore custom text
    but some will display it. This improves clarity of the warning.

    packages/client/src/components/blocks-workspace/BlocksWorkspace.tsx [254-255]

    -e.preventDefault();
    -e.returnValue = '';
    +e.returnValue = 'You have unsaved changes; leaving will discard your work.';
    Suggestion importance[1-10]: 4

    __

    Why: Providing a custom e.returnValue improves clarity, but most browsers ignore it so impact is limited.

    Low

    @AAfghahi
    Copy link
    Copy Markdown
    Contributor

    Tested and it works in Pages, but not in notebook. Could you add the same type of logic for a notebook page?

    Copy link
    Copy Markdown
    Contributor

    @AAfghahi AAfghahi left a comment

    Choose a reason for hiding this comment

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

    see comment

    Copy link
    Copy Markdown
    Contributor

    @AAfghahi AAfghahi left a comment

    Choose a reason for hiding this comment

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

    Checked notebook with Ishumita and it worked.

    @AAfghahi
    Copy link
    Copy Markdown
    Contributor

    @bfekadu21 Could you and Ishumita connect to make sure that this is the intended behavior?

    @ishumita ishumita requested a review from bfekadu21 June 12, 2025 15:37
    @bfekadu21
    Copy link
    Copy Markdown

    This looks fine from a design perspective. User will see the browser notification when they click on the refresh button and will have the option to cancel or reload. This appears consistent across Google Chrome and Microsoft Edge.

    @johbaxter
    Copy link
    Copy Markdown
    Contributor

    image

    @ishumita
    Copy link
    Copy Markdown
    Contributor Author

    Just had a discussion with @johbaxter , he mentioned all is good with the PR.

    @johbaxter
    Copy link
    Copy Markdown
    Contributor

    Nice ty

    @johbaxter johbaxter merged commit 09e4608 into dev Jun 13, 2025
    4 checks passed
    @johbaxter johbaxter deleted the notebook_data branch June 13, 2025 18:24
    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /update_changelog

    @QodoAI-Agent
    Copy link
    Copy Markdown

    Changelog updates: 🔄

    2025-06-13 *

    Added

    • Warn users before refreshing the drag-and-drop notebook to prevent data loss.

    to commit the new content to the CHANGELOG.md file, please type:
    '/update_changelog --pr_update_changelog.push_changelog_changes=true'

    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.

    5 participants