apply PR feedback

Author: hotzenklotz

frontend/javascripts/dashboard/dataset/dataset_settings_view.tsx

@@ -13,14 +13,13 @@ import {
 import { Alert, Button, Card, Form, Spin, Tabs, Tooltip } from "antd";
 import dayjs from "dayjs";
 import features from "features";
-import type { UnregisterCallback } from "history";
 import { handleGenericError } from "libs/error_handling";
 import { useWkSelector } from "libs/react_hooks";
 import Toast from "libs/toast";
 import { jsonStringify } from "libs/utils";
 import _ from "lodash";
 import messages from "messages";
-import { useCallback, useEffect, useRef, useState } from "react";
+import { useCallback, useEffect, useState } from "react";
 import { Link } from "react-router-dom";
 import type { APIDataSource, APIDataset, MutableAPIDataset } from "types/api_types";
 import { enforceValidatedDatasetViewConfiguration } from "types/schemas/dataset_view_configuration_defaults";
@@ -86,23 +85,6 @@ const DatasetSettingsView: React.FC<DatasetSettingsViewProps> = ({
     APIDataSource | null | undefined
   >(null);
 
-  const unblockRef = useRef<UnregisterCallback | null>(null);
-  const blockTimeoutIdRef = useRef<number | null>(null);
-
-  const unblockHistory = useCallback(() => {
-    window.onbeforeunload = null;
-
-    if (blockTimeoutIdRef.current != null) {
-      clearTimeout(blockTimeoutIdRef.current);
-      blockTimeoutIdRef.current = null;
-    }
-
-    if (unblockRef.current != null) {
-      unblockRef.current();
-      unblockRef.current = null;
-    }
-  }, []);
-
   const fetchData = useCallback(async (): Promise<void> => {
     try {
       setIsLoading(true);
@@ -384,11 +366,6 @@ const DatasetSettingsView: React.FC<DatasetSettingsViewProps> = ({
     setHasUnsavedChanges(true);
   }, []);
 
-  const handleCancel = useCallback(() => {
-    unblockHistory();
-    onCancel();
-  }, [unblockHistory, onCancel]);
-
   const handleDataSourceEditModeChange = useCallback(
     (activeEditMode: "simple" | "advanced") => {
       syncDataSourceFields(form, activeEditMode);
@@ -611,7 +588,7 @@ const DatasetSettingsView: React.FC<DatasetSettingsViewProps> = ({
               {confirmString}
             </Button>
             {Unicode.NonBreakingSpace}
-            <Button onClick={handleCancel}>Cancel</Button>
+            <Button onClick={onCancel}>Cancel</Button>
           </FormItem>
         </Spin>
       </Card>

frontend/javascripts/dashboard/dataset/team_selection_component.tsx

@@ -1,6 +1,7 @@
 import { getEditableTeams, getTeams } from "admin/rest_api";
 import { Select } from "antd";
 import { useEffectOnlyOnce } from "libs/react_hooks";
+import Toast from "libs/toast";
 import _ from "lodash";
 import { useCallback, useEffect, useState } from "react";
 import type { APITeam } from "types/api_types";
@@ -43,12 +44,13 @@ function TeamSelectionComponent({
     try {
       const possibleTeams = allowNonEditableTeams ? await getTeams() : await getEditableTeams();
       setPossibleTeams(possibleTeams);
-      setIsFetchingData(false);
       if (afterFetchedTeams) {
         afterFetchedTeams(possibleTeams);
       }
     } catch (_exception) {
-      console.error("Could not load teams.");
+      Toast.error("Could not load teams.");
+    } finally {
+      setIsFetchingData(false);
     }
   }
 
@@ -61,11 +63,11 @@ function TeamSelectionComponent({
       ? selectedTeamIdsOrId
       : [selectedTeamIdsOrId];
     const allTeams = getAllTeams();
-    const selected = _.compact(selectedTeamIds.map((id) => allTeams.find((t) => t.id === id)));
+    const selectedTeams = _.compact(selectedTeamIds.map((id) => allTeams.find((t) => t.id === id)));
     if (onChange) {
-      onChange(Array.isArray(selectedTeamIdsOrId) ? selected : selected[0]);
+      onChange(Array.isArray(selectedTeamIdsOrId) ? selectedTeams : selectedTeams[0]);
     }
-    setSelectedTeams(selected);
+    setSelectedTeams(selectedTeams);
   };
 
   return (
@@ -78,7 +80,7 @@ function TeamSelectionComponent({
       onChange={onSelectTeams}
       value={selectedTeams.map((t) => t.id)}
       filterOption
-      disabled={disabled ? disabled : false}
+      disabled={disabled ?? false}
       loading={isFetchingData}
     >
       {getAllTeams().map((team) => (

frontend/javascripts/dashboard/dataset/useBeforeUnload_hook.ts

@@ -28,10 +28,14 @@ const useBeforeUnload = (hasUnsavedChanges: boolean, message: string) => {
       newLocation: HistoryLocation<unknown>,
       action: HistoryAction,
     ): string | false | void => {
+      // Only show the prompt if this is a proper beforeUnload event from the browser
+      // or the pathname changed
+      // This check has to be done because history.block triggers this function even if only the url hash changed
       if (action === undefined || newLocation.pathname !== window.location.pathname) {
         if (hasUnsavedChanges) {
-          window.onbeforeunload = null;
+          window.onbeforeunload = null; // clear the event handler otherwise it would be called twice. Once from history.block once from the beforeunload event
           blockTimeoutIdRef.current = window.setTimeout(() => {
+            // restore the event handler in case a user chose to stay on the page
             // @ts-ignore
             window.onbeforeunload = beforeUnload;
           }, 500);