-
Notifications
You must be signed in to change notification settings - Fork 2.6k
(DO NOT MERGE) Add Roomote Control CTA for long-running tasks #6731
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
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.
Even I'm confused by my own code, and I wrote it 5 minutes ago.
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.
Is this intentional? The interval cleanup only happens on unmount, but when the task changes (line 76 dependency), a new interval is created without clearing the old one. This could lead to memory leaks with multiple intervals running simultaneously.
| }, [task.ts, currentTaskItem?.id, dismissedCloudNotifications]) | |
| useEffect(() => { | |
| const taskId = currentTaskItem?.id | |
| if (!taskId) return | |
| const interval = setInterval(() => { | |
| const duration = Date.now() - task.ts | |
| // Show notification if task has been running for more than 2 minutes | |
| // and hasn't been dismissed for this task | |
| const shouldShow = duration > 2 * 60 * 1000 && !dismissedCloudNotifications.has(taskId) | |
| setShowCloudNotification(shouldShow) | |
| }, 1000) | |
| return () => clearInterval(interval) | |
| }, [task.ts, currentTaskItem?.id, dismissedCloudNotifications]) |
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.
The Cloud icon button could benefit from better accessibility. Consider adding keyboard navigation support and a more descriptive aria-label that changes based on the notification state:
| aria-label="Cloud"> | |
| <button | |
| className={cn( | |
| "relative inline-flex items-center justify-center", | |
| "bg-transparent border-none p-1.5", | |
| "rounded-md min-w-[28px] min-h-[28px]", | |
| "transition-all duration-150", | |
| "hover:opacity-100 hover:bg-[rgba(255,255,255,0.03)] hover:border-[rgba(255,255,255,0.15)]", | |
| "focus:outline-none focus-visible:ring-1 focus-visible:ring-vscode-focusBorder", | |
| "active:bg-[rgba(255,255,255,0.1)]", | |
| "cursor-pointer", | |
| showCloudNotification | |
| ? "text-vscode-charts-blue opacity-100" | |
| : "text-vscode-foreground opacity-85", | |
| )} | |
| style={{ fontSize: 16.5 }} | |
| aria-label={showCloudNotification ? "Cloud notification active" : "Cloud"}> | |
| <Cloud size={16} /> | |
| </button> |
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.
The dismissedCloudNotifications state isn't persisted, so users will see notifications again for long-running tasks after the extension reloads. Should we consider persisting this state similar to other user preferences?
This could be added to the state persistence logic to maintain user dismissals across sessions.
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.
Minor suggestion: The animation duration (200ms) could be extracted to a constant for better maintainability:
| }, 200) // Match animation duration | |
| const ANIMATION_DURATION_MS = 200 | |
| const handleDismiss = () => { | |
| setIsAnimating(true) | |
| setTimeout(() => { | |
| setIsVisible(false) | |
| onDismiss() | |
| }, ANIMATION_DURATION_MS) | |
| } |
- Add Cloud icon button to TaskHeader that lights up blue when notifications should be shown - Create CloudNotificationBanner component with speech bubble design - Implement task duration tracking with 2+ minute threshold - Add persistence for dismissal state per task using ExtensionStateContext - Enable navigation to Account page on notification click - Include smooth height animations for show/hide transitions - Add comprehensive test coverage for new functionality - Add localization support for notification message
da79e62 to
ea43c87
Compare
Pitches people on Cloud for tasks running longer than 2 minutes, allowing them to dismiss it.
Features Added
Note it doesn't add the actual connection to Cloud, this is laying out the UI groundwork
Technical Details
Important
Add cloud notification system for tasks running over 2 minutes, with user interaction and state management enhancements.
CloudNotificationBannercomponent for tasks running over 2 minutes, with dismissal and navigation features.TaskHeadertracks task duration and shows notifications if not dismissed.TaskActionsincludes a cloud icon that lights up when notifications are active.ExtensionStateContextwithdismissedCloudNotificationsstate and related functions.cloudNotification.messagetochat.jsonfor notification text.CloudNotificationBannerandTaskHeaderin__tests__directory.This description was created by
for da79e62a70d2db0599c64b8af4b8212d5021ccb0. You can customize this summary. It will automatically update as commits are pushed.