-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🪟 🎉 Show notification if Airbyte got updated #22480
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just left some non-blocking comments. Tested locally ✨
.subscribe(async () => { | ||
try { | ||
// Fetch the buildInfo.json file without using any browser cache | ||
const buildInfoResp = await fetch("/buildInfo.json", { cache: "no-store" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checking - our nginx config doesn't have any server-side caching set up, right? This will be a route we'll want to avoid caching on the CDN if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has by default. In general I wanted to make this independant from whatever the infrastructure might have (miss)configured. So I thought forcing this here, makes more sense?
Co-authored-by: Joey Marshment-Howell <josephkmh@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, only issue I saw was a trivial/non-blocking style thing. Testing locally now (insert xkcd about waiting for code to compile here); assuming everything is in order, I'll approve shortly
// Trigger the check every ${INTERVAL} milliseconds or whenever the window regains focus again | ||
const subscription = merge(interval(INTERVAL), fromEvent(window, "focus")) | ||
// Throttle it to maximum once every 10 seconds | ||
.pipe(throttleTime(10 * 1000)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice clean way to avoid bunching up requests if the focus event comes right before the next interval; I briefly wondered if some switchMap
shenanigans would be needed to avoid that scenario (and whether it would actually be a problem, given our current scale and the CDN deployment discussions), but this is much better.
nit: I do think that it would be better to be consistent with how the time windows are defined; so 10 * 1000
should become 10_000
and either both or neither number should be extracted to a named binding (no strong preference which):
// either this...
const INTERVAL = 60_000;
const MINIMUM_WAIT_BEFORE_REFETCH = 10_000;
merge(interval(INTERVAL), fromEvent(window, "focus"))
.pipe(throttleTime(MINIMUM_WAIT_BEFORE_REFETCH))
// ...or this
merge(interval(60_000), fromEvent(window, "focus"))
.pipe(throttleTime(10_000))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested as working, only trivial style suggestions: LGTM 🚀 🎸 🔥 ✨
.subscribe(async () => { | ||
try { | ||
// Fetch the buildInfo.json file without using any browser cache | ||
const buildInfoResp = await fetch("/buildInfo.json", { cache: "no-store" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if you parse the response as json in a then
, there's no need to bind any intermediate variable for the raw response. I'm not big on mixing async/await and then syntaxes randomly, but here I think it makes the intent clearer by reducing "fetch some json from a file" to a single step.
const buildInfo: BuildInfo = await fetch("/buildInfo.json", { cache: "no-store" }).then((res) => res.json());
* WIP * Pass through data-testid * Also check on window.focus * Add more documentation * Update airbyte-webapp/src/hooks/services/useBuildUpdateCheck.ts Co-authored-by: Joey Marshment-Howell <josephkmh@users.noreply.github.com> * Address review * Address review --------- Co-authored-by: Joey Marshment-Howell <josephkmh@users.noreply.github.com>
What
Closes #19767
Show an update notification once the airbyte instance got updated and a user still has a webapp tab open.
How
Every time Vite runs and loads our
build-info
plugin (i.e. on every build) we'll just generate a new random UUID and write this out to/public/buildInfo.json
as well as making it available during the build asprocess.env.BUILD_HASH
.The build check than periodically every minute query that file without using a browser cache, and in case you kept the webpage open (and thus you still have the old
process.env.BUILD_HASH
, but the backend was updated, we now see a missmatch between those versions and show the notification. We also check every time the window gets refocused (which might happen if the browser tab was in the background and refocused), but make sure to throttle the overall calls to once every 10 seconds, which would be more than enough).Why exactly those values? I don't have any really strong opinion about them, but I wanted to make sure the notification isn't coming "too late" after the user might already run into the error that some assets might not load for them anymore, since they're gone, and also wanted to make sure focusing works, so in case you had it in the background for very long (in which case some browser might stop your interval), you still get a pretty immediate check if it's still up to date.
I also added
data-testid
to all notification as part of that, so we can easier filter for those and create watched elements in fullstory.How to test this locally?
To test this locally, you can do the following: