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

[Asset graph] Add query refresh button with "Date at most X seconds old" tooltip #16886

Merged
merged 6 commits into from
Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const RefreshableCountdown = (props: Props) => {
);
};

const RefreshButton = styled.button`
export const RefreshButton = styled.button`
border: none;
cursor: pointer;
padding: 8px;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import {Box, Button, Colors, Icon, Tooltip} from '@dagster-io/ui-components';
import dayjs from 'dayjs';
import relativeTime from 'dayjs/plugin/relativeTime';
import updateLocale from 'dayjs/plugin/updateLocale';
import React from 'react';

dayjs.extend(relativeTime);
dayjs.extend(updateLocale);

dayjs.updateLocale('en', {
relativeTime: {
future: 'in %s',
past: '%s ago',
s: '%d seconds',
m: 'a minute',
mm: '%d minutes',
h: 'an hour',
hh: '%d hours',
d: 'a day',
dd: '%d days',
M: 'a month',
MM: '%d months',
y: 'a year',
yy: '%d years',
},
});

export const AssetDataRefreshButton = ({
isRefreshing,
onRefresh,
oldestDataTimestamp,
}: {
isRefreshing: boolean;
onRefresh: () => void;
oldestDataTimestamp: number;
}) => {
return (
<Tooltip
content={
isRefreshing ? (
'Refreshing asset data…'
) : (
<Box flex={{direction: 'column', gap: 4}}>
<TimeFromNowWithSeconds timestamp={oldestDataTimestamp} />
<div>Click to refresh now</div>
</Box>
)
}
>
<Button
icon={<Icon name="refresh" color={Colors.Gray400} />}
onClick={() => {
Comment on lines +50 to +52
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a huge fan of this looking different than all our other refresh buttons but I'd defer to josh on this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was josh's design, we thought the text makes the button too long

if (!isRefreshing) {
onRefresh();
}
}}
/>
</Tooltip>
);
};

const TimeFromNowWithSeconds: React.FC<{timestamp: number}> = ({timestamp}) => {
const [text, setText] = React.useState(dayjs(timestamp).fromNow(true));
React.useEffect(() => {
const interval = setInterval(() => {
setText(dayjs(timestamp).fromNow(true));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, is this too flickery to display this outside a tooltip? Would be nice to have data age onscreen in some way vs just a refresh button. Would "At most 22 sec old" in gray text be too long? Feels like it'd match our refreshableCountdown UI a bit more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nick had some qualms with the countdown counting upwards, he said it gave him anxiety

}, 1000);
return () => {
clearInterval(interval);
};
}, [timestamp]);
return <>{text === '0s' ? 'Refreshing asset data…' : `Data is at most ${text} old`}</>;
};
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import {AssetKeyInput} from '../graphql/types';
import {isDocumentVisible, useDocumentVisibility} from '../hooks/useDocumentVisibility';
import {useDidLaunchEvent} from '../runs/RunUtils';

import {AssetDataRefreshButton} from './AssetDataRefreshButton';

const _assetKeyListeners: Record<string, Array<DataForNodeListener>> = {};
let providerListener = (_key: string, _data?: LiveDataForNode) => {};
const _cache: Record<string, LiveDataForNode> = {};
Expand Down Expand Up @@ -56,11 +58,13 @@ export function useAssetsLiveData(assetKeys: AssetKeyInput[]) {

return {
liveDataByNode: data,

refresh: React.useCallback(() => {
_resetLastFetchedOrRequested(assetKeys);
setNeedsImmediateFetch();
setIsRefreshing(true);
}, [setNeedsImmediateFetch, assetKeys]),

refreshing: React.useMemo(() => {
for (const key of assetKeys) {
const stringKey = tokenForAssetKey(key);
Expand Down Expand Up @@ -120,6 +124,16 @@ const AssetLiveDataContext = React.createContext<{
onUnsubscribed: () => {},
});

const AssetLiveDataRefreshContext = React.createContext<{
isGloballyRefreshing: boolean;
oldestDataTimestamp: number;
refresh: () => void;
}>({
isGloballyRefreshing: false,
oldestDataTimestamp: Infinity,
refresh: () => {},
});

// Map of asset keys to their last fetched time and last requested time
const lastFetchedOrRequested: Record<
string,
Expand All @@ -143,33 +157,55 @@ export const AssetLiveDataProvider = ({children}: {children: React.ReactNode}) =

const isDocumentVisible = useDocumentVisibility();

const [isGloballyRefreshing, setIsGloballyRefreshing] = React.useState(false);
const [oldestDataTimestamp, setOldestDataTimestamp] = React.useState(0);

Comment on lines +160 to +162
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels a bit odd that oldestDataTimestamp is a separate piece of state when it's a pure derivation of allAssetKeys and cache? (eg: given a set of asset keys and the cache, I should be able to tell you the oldest data timestamp)

It might be nice to write it as a useMemo instead of imperatively updating it in all the correct places where you call fetchData. It seems like it'd be super easy to forget it in one of those callsites and have this state fall out of sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it depends on lastFetchedOrRequested which is a global so a useMemo wouldn't work here. lastFetchedOrRequested is only mutated by one function _batchedAssetsFetch which is the same function that calls onUpdatedOrUpdating() so it's all centralized.

const onUpdatingOrUpdated = React.useCallback(() => {
const allAssetKeys = Object.keys(_assetKeyListeners).filter(
(key) => _assetKeyListeners[key]?.length,
);
let isRefreshing = allAssetKeys.length ? true : false;
let oldestDataTimestamp = Infinity;
for (const key of allAssetKeys) {
if (lastFetchedOrRequested[key]?.fetched) {
isRefreshing = false;
}
salazarm marked this conversation as resolved.
Show resolved Hide resolved
oldestDataTimestamp = Math.min(
oldestDataTimestamp,
lastFetchedOrRequested[key]?.fetched ?? Infinity,
);
}
setIsGloballyRefreshing(isRefreshing);
setOldestDataTimestamp(oldestDataTimestamp === Infinity ? 0 : oldestDataTimestamp);
}, []);

React.useEffect(() => {
if (!isDocumentVisible) {
return;
}
// Check for assets to fetch every 5 seconds to simplify logic
// This means assets will be fetched at most 5 + SUBSCRIPTION_IDLE_POLL_RATE after their first fetch
// but then will be fetched every SUBSCRIPTION_IDLE_POLL_RATE after that
const interval = setInterval(() => fetchData(client), 5000);
fetchData(client);
const interval = setInterval(() => fetchData(client, onUpdatingOrUpdated), 5000);
fetchData(client, onUpdatingOrUpdated);
return () => {
clearInterval(interval);
};
}, [client, isDocumentVisible]);
}, [client, isDocumentVisible, onUpdatingOrUpdated]);

React.useEffect(() => {
if (!needsImmediateFetch) {
return;
}
const timeout = setTimeout(() => {
fetchData(client);
fetchData(client, onUpdatingOrUpdated);
setNeedsImmediateFetch(false);
// Wait BATCHING_INTERVAL before doing fetch in case the component is unmounted quickly (eg. in the case of scrolling/filtering quickly)
}, BATCHING_INTERVAL);
return () => {
clearTimeout(timeout);
};
}, [client, needsImmediateFetch]);
}, [client, needsImmediateFetch, onUpdatingOrUpdated]);

React.useEffect(() => {
providerListener = (stringKey, assetData) => {
Expand Down Expand Up @@ -240,7 +276,19 @@ export const AssetLiveDataProvider = ({children}: {children: React.ReactNode}) =
[],
)}
>
{children}
<AssetLiveDataRefreshContext.Provider
value={{
isGloballyRefreshing,
oldestDataTimestamp,
refresh: React.useCallback(() => {
setIsGloballyRefreshing(true);
_resetLastFetchedOrRequested();
setNeedsImmediateFetch(true);
}, [setNeedsImmediateFetch]),
}}
>
{children}
</AssetLiveDataRefreshContext.Provider>
</AssetLiveDataContext.Provider>
);
};
Expand All @@ -250,6 +298,7 @@ async function _batchedQueryAssets(
assetKeys: AssetKeyInput[],
client: ApolloClient<any>,
setData: (data: Record<string, LiveDataForNode>) => void,
onUpdatingOrUpdated: () => void,
) {
// Bail if the document isn't visible
if (!assetKeys.length || isFetching) {
Expand All @@ -263,6 +312,7 @@ async function _batchedQueryAssets(
requested: requestTime,
};
});
onUpdatingOrUpdated();
const data = await _queryAssetKeys(client, assetKeys);
const fetchedTime = Date.now();
assetKeys.forEach((key) => {
Expand All @@ -271,10 +321,11 @@ async function _batchedQueryAssets(
};
});
setData(data);
onUpdatingOrUpdated();
isFetching = false;
const nextAssets = _determineAssetsToFetch();
if (nextAssets.length) {
_batchedQueryAssets(nextAssets, client, setData);
_batchedQueryAssets(nextAssets, client, setData, onUpdatingOrUpdated);
}
}

Expand Down Expand Up @@ -333,19 +384,24 @@ function _determineAssetsToFetch() {
return assetsWithoutData.concat(assetsToFetch).slice(0, BATCH_SIZE);
}

function fetchData(client: ApolloClient<any>) {
_batchedQueryAssets(_determineAssetsToFetch(), client, (data) => {
Object.entries(data).forEach(([key, assetData]) => {
const listeners = _assetKeyListeners[key];
providerListener(key, assetData);
if (!listeners) {
return;
}
listeners.forEach((listener) => {
listener(key, assetData);
function fetchData(client: ApolloClient<any>, onUpdatingOrUpdated: () => void) {
_batchedQueryAssets(
_determineAssetsToFetch(),
client,
(data) => {
Object.entries(data).forEach(([key, assetData]) => {
const listeners = _assetKeyListeners[key];
providerListener(key, assetData);
if (!listeners) {
return;
}
listeners.forEach((listener) => {
listener(key, assetData);
});
});
});
});
},
onUpdatingOrUpdated,
);
}

function getAllAssetKeysWithListeners(): AssetKeyInput[] {
Expand All @@ -370,3 +426,16 @@ export function _setCacheEntryForTest(assetKey: AssetKeyInput, data?: LiveDataFo
});
}
}

export function AssetLiveDataRefresh() {
const {isGloballyRefreshing, oldestDataTimestamp, refresh} = React.useContext(
AssetLiveDataRefreshContext,
);
return (
<AssetDataRefreshButton
isRefreshing={isGloballyRefreshing}
oldestDataTimestamp={oldestDataTimestamp}
onRefresh={refresh}
/>
);
}
Loading