feat: Feedback on notebook run completion #1634
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@mscolnick My changes work in development mode, but I can't get the favicons to appear when running in prod. Tried clearing my cache and testing in incognito, but no luck. Any ideas? |
wasimsandhu
changed the title
|
Thanks for this, @wasimsandhu!
I'd say to do just the favicon for now. I'd prefer not to add another app setting (unless people really want it). |
|
|
||
| useEventListener(document, "visibilitychange", (_) => { | ||
| resetFaviconIfComplete(); | ||
| }); |
There was a problem hiding this comment.
can you we move this logic to its own react component
<DynamicFavicon/>
it could be in the same file, but just nicer to encapsulate the logic
| document.getElementsByTagName("head")[0].append(favicon); | ||
| } | ||
|
|
||
| const favicons: Record<string, string> = useMemo( |
There was a problem hiding this comment.
this doesnt need live in the react component.
it could be at the module level:
const FAVICON_ROUTES = {
idle: "./favicon.ico",
success: "./circle-check.ico",
running: "./circle-play.ico",
error: "./circle-x.ico",
}
| }; | ||
| }, [isRunning, errors, favicon, favicons, resetFaviconIfComplete]); | ||
|
|
||
| useEventListener(document, "visibilitychange", (_) => { |
There was a problem hiding this comment.
can you document how you want this to work?
i think i mentioned we could use document.visibilityState/visibilitychange, but that was so when the editor is active, we arent flickering the favicon and only change it when the editor is out of focus. that was only a suggestion, but up-to-you what you feel feels right.
There was a problem hiding this comment.
also - im not sure this actually solves the ticket yet. i think what might happen with the above is:
- goes to running
- shows the success
- then clears after 3 seconds
If you miss that 3 second window, you don't really get the feedback. Maybe the logic could be:
- goes to running
- shows the success
- then clears after 3 seconds ONLY if the document is focused, otherwise keep it there
- clear when the user comes back into focus (could be after 3 seconds or right away)
also worth documenting this logic in a comment on the DynamicFavicon component
There was a problem hiding this comment.
Sure, I'll add some comments in the file.
You're right, the favicon is reset after 3 seconds if the page is visible, but remains if it is not. I agree that it should persist if the document is not in focus.
| return () => { | ||
| favicon.href = favicons.idle; | ||
| }; | ||
| }, [isRunning, errors, favicon, favicons, resetFaviconIfComplete]); |
There was a problem hiding this comment.
we can probably inline resetFaviconIfComplete, because we will want to clear the timeout too on the returned unsubscribe
useEffect(() => {
if (isRunning) {
FAVICONS.href = favicons.running;
return;
}
favicon.href = errors.length > 0 ? FAVICONS.error : FAVICONS.success;
// Reset back to idle after 3 seconds
const timeout = setTimeout(() => {
favicon.href = FAVICONS.idle;
}, 3000);
return () => {
favicon.href = FAVICONS.idle;
clearTimeout();
};
}, [isRunning, errors.length]);There was a problem hiding this comment.
Good catch thanks!
|
if this feels right, you could also use the native browser notification. this way you dont need to add anything to user settings, and they can configure it on the first try (and if they deny access, it wont happen again) function maybeSendNotification() {
// If in focus, skip
if (document.visibilityState === "visible") {
return;
}
const sendNotification = () => {
if (succeeded) {
const notification = new Notification("Execution completed", {icon: icon});
} else {
const notification = new Notification("Execution failed", {icon: icon}););
}
}
if (!("Notification" in window)) {
// Skip no support
} else if (Notification.permission === "granted") {
// Permission already granted
sendNotification();
} else if (Notification.permission !== "denied") {
// We need to ask the user for permission
Notification.requestPermission().then((permission) => {
// If the user accepts, let's create a notification
if (permission === "granted") {
sendNotification();
}
});
}
} |
|
@wasimsandhu for it working in production - you might need to add those assets to here: |
|
nice! looks great! |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.6.20-dev18 |
📝 Summary
Gives user feedback on notebook run completion via dynamic favicon and optional system notifications.
Addresses feature request in #1039.
🔍 Description of Changes
Dynamic favicon
Favicon changes to indicate whether a notebook is running, and whether the run completed successfully or encountered errors.
This favicon persists until the user navigates back to their notebook, or resets back to the marimo favicon after 3 seconds if the notebook is in focus.
Favicon when notebook is running:
Favicon when a run completed successfully:
Favicon when a run produced errors:
Notifications
Implemented by @mscolnick, more info in this comment.
📋 Checklist
📜 Reviewers
@mscolnick